Re: [PATCH v2 2/2] EDAC/amd64: Add new register offset support and related changes

[William Roche <william.roche@oracle.com>]

On 16/12/2021 16:46, Yazen Ghannam wrote:
> On Wed, Dec 15, 2021 at 07:07:17PM +0100, Borislav Petkov wrote:
>> On Wed, Dec 15, 2021 at 05:32:27PM +0100, William Roche wrote:
>>>> @@ -2174,8 +2215,13 @@ static int f17_addr_mask_to_cs_size(struct amd64_pvt *pvt, u8 umc,
>>>>    	 * There is one mask per DIMM, and two Chip Selects per DIMM.
>>>>    	 *	CS0 and CS1 -> DIMM0
>>>>    	 *	CS2 and CS3 -> DIMM1
>>>> +	 *
>>>> +	 *	Systems with newer register layout have one mask per Chip Select.
>>> Just a question about this comment: Can it be translated into this ?
>>>
>>> +	 * Except on systems with newer register layout where we have one Chip Select per DIMM.
>> Sure, but without the "we":
>>
>> 	...
>> 	* On systems with the newer register layout there is one Chip Select per DIMM.
>> 	*/
>>
> Hi William,
> Thanks for the suggestion, but it's not quite correct.

That's exactly what I wanted to know. Thanks.

>
> There are still two Chip Selects per DIMM module, i.e. the system can support
> dual-rank (2R) DIMMs. Current AMD systems can support upto 2 DIMMs per Unified
> Memory Controller (UMC). There are two "Address Mask" registers in each UMC,
> and each register covers an entire DIMM (and by extension the two Chip Selects
> available for each DIMM).
>
> Future systems will still support upto 2 DIMMs per UMC. However, the register
> space is updated so that there are now four "Address Mask" registers per UMC.
> And each of these registers is now explicitly related to one of the four Chip
> Selects available per UMC.

 From what I understand, future systems would still support the same 
number of dimms per UMC (2), the same number of Chip Select (2 per 
dimm), the only thing that changes is the number of Address Mask 
registers (going from 2 per UMC  to  4 per UMC).

So I'm confused, we deduce 'dimm' from csrow_nr, which would be in fact 
the Chip Select *masks* number (cs_mask_nr from the dbam_to_cs signature 
in struct low_ops), so why are we saying and dimm=csrow_nr in the case 
of the new layout, but dimm = csrow_nr / 2 in the case on the standard 
layout ?

Should we indicate what this 'dimm' value really is ?

Sorry if I'm missing something very obvious here.

Thanks,
William.


> Does this help? I can update the code comments with these details.
>
> Thanks,
> Yazen