Re: [PATCH 2/5] platform/x86/amd/pmc: Add SMU mailbox offset retrieval for different CPU families

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4809 bytes --]

On Wed, 27 May 2026, Shyam Sundar S K wrote:
> On 5/22/2026 18:06, Ilpo Järvinen wrote:
> > On Thu, 21 May 2026, Shyam Sundar S K wrote:
> > 
> >> Different AMD CPU families have different SMU mailbox register offsets.
> >> Add a helper function amd_pmc_get_smu_mb() to populate the appropriate
> >> SMU message offset based on the CPU ID during probe.
> >>
> >> This infrastructure will help support future features that require
> >> communication with the SMU through the correct mailbox address.
> >>
> >> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >>  drivers/platform/x86/amd/pmc/pmc.c | 24 +++++++++++++++++++++---
> >>  1 file changed, 21 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> >> index 7200f19088fe..7c80fb445138 100644
> >> --- a/drivers/platform/x86/amd/pmc/pmc.c
> >> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> >> @@ -101,6 +101,24 @@ static inline void amd_pmc_reg_write(struct amd_pmc_dev *dev, int reg_offset, u3
> >>  	iowrite32(val, dev->regbase + reg_offset);
> >>  }
> >>  
> >> +static void amd_pmc_get_smu_mb(struct amd_pmc_dev *dev)
> >> +{
> >> +	switch (dev->cpu_id) {
> >> +	case AMD_CPU_ID_PCO:
> >> +	case AMD_CPU_ID_RN:
> >> +	case AMD_CPU_ID_VG:
> >> +	case AMD_CPU_ID_YC:
> >> +	case AMD_CPU_ID_CB:
> >> +	case AMD_CPU_ID_PS:
> >> +		dev->smu_msg = 0x538;
> >> +		break;
> >> +	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> >> +	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> >> +		dev->smu_msg = 0x938;
> >> +		break;
> >> +	}
> >> +}
> >> +
> >>  static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
> >>  {
> >>  	switch (dev->cpu_id) {
> >> @@ -111,12 +129,10 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
> >>  	case AMD_CPU_ID_CB:
> >>  		dev->num_ips = 12;
> >>  		dev->ips_ptr = soc15_ip_blk;
> >> -		dev->smu_msg = 0x538;
> >>  		break;
> >>  	case AMD_CPU_ID_PS:
> >>  		dev->num_ips = 21;
> >>  		dev->ips_ptr = soc15_ip_blk;
> >> -		dev->smu_msg = 0x538;
> >>  		break;
> >>  	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> >>  	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> >> @@ -127,7 +143,6 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
> >>  			dev->num_ips = ARRAY_SIZE(soc15_ip_blk);
> >>  			dev->ips_ptr = soc15_ip_blk;
> >>  		}
> >> -		dev->smu_msg = 0x938;
> >>  		break;
> >>  	}
> >>  }
> >> @@ -782,6 +797,9 @@ static int amd_pmc_probe(struct platform_device *pdev)
> >>  	/* Get num of IP blocks within the SoC */
> >>  	amd_pmc_get_ip_info(dev);
> >>  
> >> +	/* Populate SMU msg offset */
> >> +	amd_pmc_get_smu_mb(dev);
> >> +
> >>  	platform_set_drvdata(pdev, dev);
> >>  	if (IS_ENABLED(CONFIG_SUSPEND)) {
> >>  		err = acpi_register_lps0_dev(&amd_pmc_s2idle_dev_ops);
> > 
> > Could these difference should be recorded into an info struct so we don't 
> > need to make a function for each difference?
> > 
> 
> I could not understand your comment here, can you please elaborate
> what do you mean by info struct?
> 
> Is it something like this?
> 
> +static const struct amd_pmc_cpu_info amd_1ah_cpu_info = {
> +	.smu_msg	= 0x938,
> +	.num_ips	= ARRAY_SIZE(soc15_ip_blk),
> +	.ips_ptr	= soc15_ip_blk,
> +};
> 
> +
> 
> +static const struct amd_pmc_cpu_info amd_1ah_m70_cpu_info = {
> +	.smu_msg	= 0x938,
> +	.num_ips	= ARRAY_SIZE(soc15_ip_blk_v2),
> +	.ips_ptr	= soc15_ip_blk_v2,
> +};
>
> +static void amd_pmc_get_cpu_info(struct amd_pmc_dev *dev)
> {
> ...
> 
> +	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> +	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> +		if (boot_cpu_data.x86_model == 0x70)
> +			info = &amd_1ah_m70_cpu_info;
> +		else
> +			info = &amd_1ah_cpu_info;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	dev->smu_msg = info->smu_msg;
> +	dev->num_ips = info->num_ips;
> +	dev->ips_ptr = info->ips_ptr;


Something alone the lines of above but you could also change PCI_DEVICE() 
-> PCI_DEVICE_DATA(..., &xx_info) and get the struct from there per each 
ID. The variations related to boot_cpu_data.x86_model == 0x70 is a bit 
problematic though so you'll need to handle that case separately from 
getting rest directly from the id table.

There's also amd_pmc_get_os_hint() which could be covered using this 
approach, I think.

If store the info pointer into dev, you don't need to have those fields 
copied to dev anymore.

> 
> ...
> }
> 
> static int amd_pmc_probe(struct platform_device *pdev)
> {
> 
> ...
> 
> -	amd_pmc_get_ip_info(dev);
> 
> +	amd_pmc_get_cpu_info(dev);
> 
> 
> ...
> }
> 
> 
> Thanks,
> Shyam
> 

-- 
 i.