Re: [PATCH v2 1/2] EDAC/amd64: Add support for AMD family 1Ah-based newer models

["Naik, Avadhut" <avadnaik@amd.com>]

Apologies!

Mistakenly hit send early in my previous response.

On 8/18/2025 16:22, Borislav Petkov wrote:
> On Thu, Aug 07, 2025 at 08:14:53PM +0000, Avadhut Naik wrote:
>> Add support for family 1Ah-based models 50h-57h, 90h-9Fh, A0h-AFh, and
>> C0h-C7h.
>>
>> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
>> ---
>>  drivers/edac/amd64_edac.c | 20 ++++++++++++++++++++
>>  drivers/edac/amd64_edac.h |  2 +-
>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
>> index 07f1e9dc1ca7..2f6ab783bf20 100644
>> --- a/drivers/edac/amd64_edac.c
>> +++ b/drivers/edac/amd64_edac.c
>> @@ -3923,6 +3923,26 @@ static int per_family_init(struct amd64_pvt *pvt)
>>  			pvt->ctl_name           = "F1Ah_M40h";
>>  			pvt->flags.zn_regs_v2   = 1;
>>  			break;
>> +		case 0x50 ... 0x57:
>> +			pvt->ctl_name           = "F1Ah_M50h";
>> +			pvt->max_mcs            = 16;
>> +			pvt->flags.zn_regs_v2   = 1;
>> +			break;
>> +		case 0x90 ... 0x9f:
>> +			pvt->ctl_name           = "F1Ah_M90h";
>> +			pvt->max_mcs            = 8;
>> +			pvt->flags.zn_regs_v2   = 1;
>> +			break;
>> +		case 0xa0 ... 0xaf:
>> +			pvt->ctl_name           = "F1Ah_MA0h";
>> +			pvt->max_mcs            = 8;
>> +			pvt->flags.zn_regs_v2   = 1;
>> +			break;
>> +		case 0xc0 ... 0xc7:
>> +			pvt->ctl_name           = "F1Ah_MC0h";
>> +			pvt->max_mcs            = 16;
>> +			pvt->flags.zn_regs_v2   = 1;
>> +			break;
>>  		}
> 
> Oh boy, this pvt->ctl_name is ridiculous: we're collecting a zoo of string
> objects where this thing can simply be scnprintf()-ed once at driver init and
> then the pointer handed in to mci->ctl_name.
> 
> Something like
> 
> static char *ctl_name[MAX_CTL_NAMELEN]; 
> 
> or so. And then
> 
> 	scnprintf(...)
> 
> into it based on family and model.
> 
> Can you pls do that as a cleanup upfront?
> 
> This is slowly getting out of hand.
> 
> And then you can group them together:
> 
> case 0x50 ... 0x57:
> case 0xc0 ... 0xc7:
> 	pvt->max_mcs            = 16;
> 	pvt->flags.zn_regs_v2   = 1;
> 	break;
> 
> ...
> 
> and so on.
> 
> So that this function turns back into something saner again.
> 
> Thx.
> 
Will try to put this in place and make it the fist patch of this
set.

> 
> 
> 
> 
>>  		break;
>>  
>> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
>> index 17228d07de4c..d70b8a8d0b09 100644
>> --- a/drivers/edac/amd64_edac.h
>> +++ b/drivers/edac/amd64_edac.h
>> @@ -96,7 +96,7 @@
>>  /* Hardware limit on ChipSelect rows per MC and processors per system */
>>  #define NUM_CHIPSELECTS			8
>>  #define DRAM_RANGES			8
>> -#define NUM_CONTROLLERS			12
>> +#define NUM_CONTROLLERS			16
> 
> I've asked this before but can we read out the number of controllers from the
> hw somewhere instead of diddling with this silly define all the time?
> 
> Thx.
> 

Had considered this sometime back. Didn't pursue further as it maybe somewhat
tricky.

Some new CPUIDs were introduced since Zen4 trough which we MAYBE able to
accomplish this i.e get rid of the macro.

But am not not sure as to what we should do for older SOCs in that case.
Zen3, for example.

Will look more if I can find something else.

-- 
Thanks,
Avadhut Naik