[PATCH v2 0/2] Add support for AMD Family 1Ah-based SOCs

[PATCH v2 0/2] Add support for AMD Family 1Ah-based SOCs

[Avadhut Naik]

This patchset adds support for newer AMD's Family 1Ah-based SOCs.

The first patch adds support for these new SOCs in the amd64_edac module.

The second patch modifies the legacy EDAC sysfs interface so that the 16
UMCs supported by some of these new SOCs are appropriately visible through
the interface.

Changes in v2:
 - Remove extra tabs.
 - Update the legacy EDAC sysfs interface.

Links:
v1: https://lore.kernel.org/all/20250722192855.3108575-1-avadhut.naik@amd.com/

Avadhut Naik (2):
  EDAC/amd64: Add support for AMD family 1Ah-based newer models
  EDAC/mc_sysfs: Increase legacy channel support to 16

 drivers/edac/amd64_edac.c    | 20 ++++++++++++++++++++
 drivers/edac/amd64_edac.h    |  2 +-
 drivers/edac/edac_mc_sysfs.c | 24 ++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 1 deletion(-)


base-commit: 1fb0ddddf5d139089675b86702933cbca992b4d4
-- 
2.43.0

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

[Avadhut Naik]

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;
 		}
 		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
 
 #define ON true
 #define OFF false
-- 
2.43.0

[PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16

[Avadhut Naik]

Newer AMD systems can support up to 16 channels per EDAC "mc" device.
These are detected by the EDAC module running on the device, and the
current EDAC interface is appropriately enumerated.

The legacy EDAC sysfs interface however, provides device attributes for
channels 0 through 11 only. Consequently, the last four channels, 12
through 15, will not be enumerated and will not be visible through the
legacy sysfs interface.

Add additional device attributes to ensure that all 16 channels, if
present, are enumerated by and visible through the legacy EDAC sysfs
interface.

Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
---
 drivers/edac/edac_mc_sysfs.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 0f338adf7d93..8689631f1905 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -305,6 +305,14 @@ DEVICE_CHANNEL(ch10_dimm_label, S_IRUGO | S_IWUSR,
 	channel_dimm_label_show, channel_dimm_label_store, 10);
 DEVICE_CHANNEL(ch11_dimm_label, S_IRUGO | S_IWUSR,
 	channel_dimm_label_show, channel_dimm_label_store, 11);
+DEVICE_CHANNEL(ch12_dimm_label, S_IRUGO | S_IWUSR,
+	channel_dimm_label_show, channel_dimm_label_store, 12);
+DEVICE_CHANNEL(ch13_dimm_label, S_IRUGO | S_IWUSR,
+	channel_dimm_label_show, channel_dimm_label_store, 13);
+DEVICE_CHANNEL(ch14_dimm_label, S_IRUGO | S_IWUSR,
+	channel_dimm_label_show, channel_dimm_label_store, 14);
+DEVICE_CHANNEL(ch15_dimm_label, S_IRUGO | S_IWUSR,
+	channel_dimm_label_show, channel_dimm_label_store, 15);
 
 /* Total possible dynamic DIMM Label attribute file table */
 static struct attribute *dynamic_csrow_dimm_attr[] = {
@@ -320,6 +328,10 @@ static struct attribute *dynamic_csrow_dimm_attr[] = {
 	&dev_attr_legacy_ch9_dimm_label.attr.attr,
 	&dev_attr_legacy_ch10_dimm_label.attr.attr,
 	&dev_attr_legacy_ch11_dimm_label.attr.attr,
+	&dev_attr_legacy_ch12_dimm_label.attr.attr,
+	&dev_attr_legacy_ch13_dimm_label.attr.attr,
+	&dev_attr_legacy_ch14_dimm_label.attr.attr,
+	&dev_attr_legacy_ch15_dimm_label.attr.attr,
 	NULL
 };
 
@@ -348,6 +360,14 @@ DEVICE_CHANNEL(ch10_ce_count, S_IRUGO,
 		   channel_ce_count_show, NULL, 10);
 DEVICE_CHANNEL(ch11_ce_count, S_IRUGO,
 		   channel_ce_count_show, NULL, 11);
+DEVICE_CHANNEL(ch12_ce_count, S_IRUGO,
+		   channel_ce_count_show, NULL, 12);
+DEVICE_CHANNEL(ch13_ce_count, S_IRUGO,
+		   channel_ce_count_show, NULL, 13);
+DEVICE_CHANNEL(ch14_ce_count, S_IRUGO,
+		   channel_ce_count_show, NULL, 14);
+DEVICE_CHANNEL(ch15_ce_count, S_IRUGO,
+		   channel_ce_count_show, NULL, 15);
 
 /* Total possible dynamic ce_count attribute file table */
 static struct attribute *dynamic_csrow_ce_count_attr[] = {
@@ -363,6 +383,10 @@ static struct attribute *dynamic_csrow_ce_count_attr[] = {
 	&dev_attr_legacy_ch9_ce_count.attr.attr,
 	&dev_attr_legacy_ch10_ce_count.attr.attr,
 	&dev_attr_legacy_ch11_ce_count.attr.attr,
+	&dev_attr_legacy_ch12_ce_count.attr.attr,
+	&dev_attr_legacy_ch13_ce_count.attr.attr,
+	&dev_attr_legacy_ch14_ce_count.attr.attr,
+	&dev_attr_legacy_ch15_ce_count.attr.attr,
 	NULL
 };
 
-- 
2.43.0

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

[Borislav Petkov]

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.





>  		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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16

[Borislav Petkov]

On Thu, Aug 07, 2025 at 08:14:54PM +0000, Avadhut Naik wrote:
> Newer AMD systems can support up to 16 channels per EDAC "mc" device.
> These are detected by the EDAC module running on the device, and the
> current EDAC interface is appropriately enumerated.
> 
> The legacy EDAC sysfs interface however, provides device attributes for
> channels 0 through 11 only. Consequently, the last four channels, 12
> through 15, will not be enumerated and will not be visible through the
> legacy sysfs interface.
> 
> Add additional device attributes to ensure that all 16 channels, if
> present, are enumerated by and visible through the legacy EDAC sysfs
> interface.
> 
> Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
> ---
>  drivers/edac/edac_mc_sysfs.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 0f338adf7d93..8689631f1905 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -305,6 +305,14 @@ DEVICE_CHANNEL(ch10_dimm_label, S_IRUGO | S_IWUSR,
>  	channel_dimm_label_show, channel_dimm_label_store, 10);
>  DEVICE_CHANNEL(ch11_dimm_label, S_IRUGO | S_IWUSR,
>  	channel_dimm_label_show, channel_dimm_label_store, 11);
> +DEVICE_CHANNEL(ch12_dimm_label, S_IRUGO | S_IWUSR,
> +	channel_dimm_label_show, channel_dimm_label_store, 12);
> +DEVICE_CHANNEL(ch13_dimm_label, S_IRUGO | S_IWUSR,
> +	channel_dimm_label_show, channel_dimm_label_store, 13);
> +DEVICE_CHANNEL(ch14_dimm_label, S_IRUGO | S_IWUSR,
> +	channel_dimm_label_show, channel_dimm_label_store, 14);
> +DEVICE_CHANNEL(ch15_dimm_label, S_IRUGO | S_IWUSR,
> +	channel_dimm_label_show, channel_dimm_label_store, 15);
>  
>  /* Total possible dynamic DIMM Label attribute file table */
>  static struct attribute *dynamic_csrow_dimm_attr[] = {
> @@ -320,6 +328,10 @@ static struct attribute *dynamic_csrow_dimm_attr[] = {
>  	&dev_attr_legacy_ch9_dimm_label.attr.attr,
>  	&dev_attr_legacy_ch10_dimm_label.attr.attr,
>  	&dev_attr_legacy_ch11_dimm_label.attr.attr,
> +	&dev_attr_legacy_ch12_dimm_label.attr.attr,
> +	&dev_attr_legacy_ch13_dimm_label.attr.attr,
> +	&dev_attr_legacy_ch14_dimm_label.attr.attr,
> +	&dev_attr_legacy_ch15_dimm_label.attr.attr,
>  	NULL
>  };
>  
> @@ -348,6 +360,14 @@ DEVICE_CHANNEL(ch10_ce_count, S_IRUGO,
>  		   channel_ce_count_show, NULL, 10);
>  DEVICE_CHANNEL(ch11_ce_count, S_IRUGO,
>  		   channel_ce_count_show, NULL, 11);
> +DEVICE_CHANNEL(ch12_ce_count, S_IRUGO,
> +		   channel_ce_count_show, NULL, 12);
> +DEVICE_CHANNEL(ch13_ce_count, S_IRUGO,
> +		   channel_ce_count_show, NULL, 13);
> +DEVICE_CHANNEL(ch14_ce_count, S_IRUGO,
> +		   channel_ce_count_show, NULL, 14);
> +DEVICE_CHANNEL(ch15_ce_count, S_IRUGO,
> +		   channel_ce_count_show, NULL, 15);
>  
>  /* Total possible dynamic ce_count attribute file table */
>  static struct attribute *dynamic_csrow_ce_count_attr[] = {
> @@ -363,6 +383,10 @@ static struct attribute *dynamic_csrow_ce_count_attr[] = {
>  	&dev_attr_legacy_ch9_ce_count.attr.attr,
>  	&dev_attr_legacy_ch10_ce_count.attr.attr,
>  	&dev_attr_legacy_ch11_ce_count.attr.attr,
> +	&dev_attr_legacy_ch12_ce_count.attr.attr,
> +	&dev_attr_legacy_ch13_ce_count.attr.attr,
> +	&dev_attr_legacy_ch14_ce_count.attr.attr,
> +	&dev_attr_legacy_ch15_ce_count.attr.attr,
>  	NULL
>  };

This is also slowly getting out of hand. All those should be allocated
and initialized dynamically based on a number of MCs but not keep adding them
here ad absurdum.

Because if we were doing them dynamically, we'd never ever miss any new
channels when the hw grows...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16

[Yazen Ghannam]

On Mon, Aug 18, 2025 at 11:36:51PM +0200, Borislav Petkov wrote:
> On Thu, Aug 07, 2025 at 08:14:54PM +0000, Avadhut Naik wrote:
> > Newer AMD systems can support up to 16 channels per EDAC "mc" device.
> > These are detected by the EDAC module running on the device, and the
> > current EDAC interface is appropriately enumerated.
> > 
> > The legacy EDAC sysfs interface however, provides device attributes for
> > channels 0 through 11 only. Consequently, the last four channels, 12
> > through 15, will not be enumerated and will not be visible through the
> > legacy sysfs interface.
> > 
> > Add additional device attributes to ensure that all 16 channels, if
> > present, are enumerated by and visible through the legacy EDAC sysfs
> > interface.
> > 
> > Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
> > ---
> >  drivers/edac/edac_mc_sysfs.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> > index 0f338adf7d93..8689631f1905 100644
> > --- a/drivers/edac/edac_mc_sysfs.c
> > +++ b/drivers/edac/edac_mc_sysfs.c
> > @@ -305,6 +305,14 @@ DEVICE_CHANNEL(ch10_dimm_label, S_IRUGO | S_IWUSR,
> >  	channel_dimm_label_show, channel_dimm_label_store, 10);
> >  DEVICE_CHANNEL(ch11_dimm_label, S_IRUGO | S_IWUSR,
> >  	channel_dimm_label_show, channel_dimm_label_store, 11);
> > +DEVICE_CHANNEL(ch12_dimm_label, S_IRUGO | S_IWUSR,
> > +	channel_dimm_label_show, channel_dimm_label_store, 12);
> > +DEVICE_CHANNEL(ch13_dimm_label, S_IRUGO | S_IWUSR,
> > +	channel_dimm_label_show, channel_dimm_label_store, 13);
> > +DEVICE_CHANNEL(ch14_dimm_label, S_IRUGO | S_IWUSR,
> > +	channel_dimm_label_show, channel_dimm_label_store, 14);
> > +DEVICE_CHANNEL(ch15_dimm_label, S_IRUGO | S_IWUSR,
> > +	channel_dimm_label_show, channel_dimm_label_store, 15);
> >  
> >  /* Total possible dynamic DIMM Label attribute file table */
> >  static struct attribute *dynamic_csrow_dimm_attr[] = {
> > @@ -320,6 +328,10 @@ static struct attribute *dynamic_csrow_dimm_attr[] = {
> >  	&dev_attr_legacy_ch9_dimm_label.attr.attr,
> >  	&dev_attr_legacy_ch10_dimm_label.attr.attr,
> >  	&dev_attr_legacy_ch11_dimm_label.attr.attr,
> > +	&dev_attr_legacy_ch12_dimm_label.attr.attr,
> > +	&dev_attr_legacy_ch13_dimm_label.attr.attr,
> > +	&dev_attr_legacy_ch14_dimm_label.attr.attr,
> > +	&dev_attr_legacy_ch15_dimm_label.attr.attr,
> >  	NULL
> >  };
> >  
> > @@ -348,6 +360,14 @@ DEVICE_CHANNEL(ch10_ce_count, S_IRUGO,
> >  		   channel_ce_count_show, NULL, 10);
> >  DEVICE_CHANNEL(ch11_ce_count, S_IRUGO,
> >  		   channel_ce_count_show, NULL, 11);
> > +DEVICE_CHANNEL(ch12_ce_count, S_IRUGO,
> > +		   channel_ce_count_show, NULL, 12);
> > +DEVICE_CHANNEL(ch13_ce_count, S_IRUGO,
> > +		   channel_ce_count_show, NULL, 13);
> > +DEVICE_CHANNEL(ch14_ce_count, S_IRUGO,
> > +		   channel_ce_count_show, NULL, 14);
> > +DEVICE_CHANNEL(ch15_ce_count, S_IRUGO,
> > +		   channel_ce_count_show, NULL, 15);
> >  
> >  /* Total possible dynamic ce_count attribute file table */
> >  static struct attribute *dynamic_csrow_ce_count_attr[] = {
> > @@ -363,6 +383,10 @@ static struct attribute *dynamic_csrow_ce_count_attr[] = {
> >  	&dev_attr_legacy_ch9_ce_count.attr.attr,
> >  	&dev_attr_legacy_ch10_ce_count.attr.attr,
> >  	&dev_attr_legacy_ch11_ce_count.attr.attr,
> > +	&dev_attr_legacy_ch12_ce_count.attr.attr,
> > +	&dev_attr_legacy_ch13_ce_count.attr.attr,
> > +	&dev_attr_legacy_ch14_ce_count.attr.attr,
> > +	&dev_attr_legacy_ch15_ce_count.attr.attr,
> >  	NULL
> >  };
> 
> This is also slowly getting out of hand. All those should be allocated
> and initialized dynamically based on a number of MCs but not keep adding them
> here ad absurdum.
> 
> Because if we were doing them dynamically, we'd never ever miss any new
> channels when the hw grows...
> 

Maybe it's time to final remove this legacy interface? It's been
obsolete for more than a decade now.

199747106934 ("edac: add a new per-dimm API and make the old per-virtual-rank API obsolete")

Any guidance on the best way to remove this?

Thanks,
Yazen

Re: [PATCH v2 2/2] EDAC/mc_sysfs: Increase legacy channel support to 16

[Borislav Petkov]

On Tue, Aug 19, 2025 at 09:30:40AM -0400, Yazen Ghannam wrote:
> Maybe it's time to final remove this legacy interface? It's been
> obsolete for more than a decade now.
> 
> 199747106934 ("edac: add a new per-dimm API and make the old per-virtual-rank API obsolete")

Bah, look how time flies... :-\

Good catch - I had completely forgotten about this gunk.

> Any guidance on the best way to remove this?

Yeah, you set it to default n in Kconfig and add a warning when someone reads
the old sysfs nodes. Something along the lines that this interface has been
deprecated for a decade now and that people should switch to the new dimm
interface and that it will be removed in, say, 2-3 kernel releases or so.

I'd say.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Naik, Avadhut]

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.
> 
> 
> 
> 
> 
>>  		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.
> 

Will take a look at this!

-- 
Thanks,
Avadhut Naik

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

[Naik, Avadhut]

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