Re: [PATCH 1/5] platform/x86/amd/pmf: Use per-SoC smu_regs struct for SMU mailbox registers

[Mario Limonciello <mario.limonciello@amd.com>]

On 7/10/26 12:42, Shyam Sundar S K wrote:
> Different AMD platforms use varying SMU register layouts for PMF-SMU
> mailbox communication. The register offsets are currently hardcoded as
> AMD_PMF_REGISTER_MESSAGE, AMD_PMF_REGISTER_RESPONSE and
> AMD_PMF_REGISTER_ARGUMENT directly in amd_pmf_send_cmd() and
> amd_pmf_dump_registers(), making it difficult to support platforms that
> use a different mailbox register layout without scattering per-platform
> conditionals across the send path.
> 
> Introduce struct amd_pmf_smu_regs to capture the SoC-specific SMU
> mailbox register offsets (msg_reg, resp_reg, arg_reg[]) and add a
> pointer to it in struct amd_pmf_dev. RMB, PS, 1AH_M20H and 1AH_M60H
> all share the same legacy register layout and point to a single shared
> amd_pmf_legacy_smu_regs instance, avoiding redundant struct definitions.
> 
> Convert the pmf_pci_ids[] table from PCI_DEVICE() to PCI_DEVICE_DATA(),
> embedding the smu_regs pointer directly as driver_data. Introduce
> amd_pmf_get_smu_mb_offset() which resolves the matching PCI entry via
> pci_match_id() at probe time and assigns driver_data to dev->smu_regs.
> 
> Update all SMU register accesses in amd_pmf_send_cmd() and
> amd_pmf_dump_registers() to go through dev->smu_regs. Remove the
> hardcoded register offset references from the send path. New platform
> support requires only a new smu_regs instance and a corresponding
> PCI_DEVICE_DATA() entry.
> 
> No functional changes for existing platforms.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/platform/x86/amd/pmf/core.c | 51 +++++++++++++++++++++--------
>   drivers/platform/x86/amd/pmf/pmf.h  | 12 +++++++
>   2 files changed, 50 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 58d86b4c2828..1826fc64bf56 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -176,13 +176,13 @@ static void __maybe_unused amd_pmf_dump_registers(struct amd_pmf_dev *dev)
>   {
>   	u32 value;
>   
> -	value = amd_pmf_reg_read(dev, AMD_PMF_REGISTER_RESPONSE);
> +	value = amd_pmf_reg_read(dev, dev->smu_regs->resp_reg);
>   	dev_dbg(dev->dev, "AMD_PMF_REGISTER_RESPONSE:%x\n", value);
>   
> -	value = amd_pmf_reg_read(dev, AMD_PMF_REGISTER_ARGUMENT);
> +	value = amd_pmf_reg_read(dev, dev->smu_regs->arg_reg);
>   	dev_dbg(dev->dev, "AMD_PMF_REGISTER_ARGUMENT:%d\n", value);
>   
> -	value = amd_pmf_reg_read(dev, AMD_PMF_REGISTER_MESSAGE);
> +	value = amd_pmf_reg_read(dev, dev->smu_regs->msg_reg);
>   	dev_dbg(dev->dev, "AMD_PMF_REGISTER_MESSAGE:%x\n", value);
>   }
>   
> @@ -208,7 +208,7 @@ int amd_pmf_send_cmd(struct amd_pmf_dev *dev, u8 message, bool get, u32 arg, u32
>   	guard(mutex)(&dev->lock);
>   
>   	/* Wait until we get a valid response */
> -	rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMF_REGISTER_RESPONSE,
> +	rc = readx_poll_timeout(ioread32, dev->regbase + dev->smu_regs->resp_reg,
>   				val, val != 0, PMF_MSG_DELAY_MIN_US,
>   				PMF_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
>   	if (rc) {
> @@ -217,16 +217,16 @@ int amd_pmf_send_cmd(struct amd_pmf_dev *dev, u8 message, bool get, u32 arg, u32
>   	}
>   
>   	/* Write zero to response register */
> -	amd_pmf_reg_write(dev, AMD_PMF_REGISTER_RESPONSE, 0);
> +	amd_pmf_reg_write(dev, dev->smu_regs->resp_reg, 0);
>   
>   	/* Write argument into argument register */
> -	amd_pmf_reg_write(dev, AMD_PMF_REGISTER_ARGUMENT, arg);
> +	amd_pmf_reg_write(dev, dev->smu_regs->arg_reg, arg);
>   
>   	/* Write message ID to message ID register */
> -	amd_pmf_reg_write(dev, AMD_PMF_REGISTER_MESSAGE, message);
> +	amd_pmf_reg_write(dev, dev->smu_regs->msg_reg, message);
>   
>   	/* Wait until we get a valid response */
> -	rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMF_REGISTER_RESPONSE,
> +	rc = readx_poll_timeout(ioread32, dev->regbase + dev->smu_regs->resp_reg,
>   				val, val != 0, PMF_MSG_DELAY_MIN_US,
>   				PMF_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
>   	if (rc) {
> @@ -239,7 +239,7 @@ int amd_pmf_send_cmd(struct amd_pmf_dev *dev, u8 message, bool get, u32 arg, u32
>   		if (get) {
>   			/* PMFW may take longer time to return back the data */
>   			usleep_range(DELAY_MIN_US, 10 * DELAY_MAX_US);
> -			*data = amd_pmf_reg_read(dev, AMD_PMF_REGISTER_ARGUMENT);
> +			*data = amd_pmf_reg_read(dev, dev->smu_regs->arg_reg);
>   		}
>   		break;
>   	case AMD_PMF_RESULT_CMD_REJECT_BUSY:
> @@ -262,11 +262,18 @@ int amd_pmf_send_cmd(struct amd_pmf_dev *dev, u8 message, bool get, u32 arg, u32
>   	return rc;
>   }
>   
> +/* RMB, PS, 1AH_M20H and 1AH_M60H share the same legacy SMU mailbox registers */
> +static const struct amd_pmf_smu_regs amd_pmf_legacy_smu_regs = {
> +	.msg_reg	= AMD_PMF_REGISTER_MESSAGE,
> +	.resp_reg	= AMD_PMF_REGISTER_RESPONSE,
> +	.arg_reg	= AMD_PMF_REGISTER_ARGUMENT,
> +};
> +
>   static const struct pci_device_id pmf_pci_ids[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_RMB) },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, AMD_CPU_ID_PS) },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_1AH_M20H_ROOT) },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_1AH_M60H_ROOT) },
> +	{ PCI_DEVICE_DATA(AMD, CPU_ID_RMB,    &amd_pmf_legacy_smu_regs) },
> +	{ PCI_DEVICE_DATA(AMD, CPU_ID_PS,     &amd_pmf_legacy_smu_regs) },
> +	{ PCI_DEVICE_DATA(AMD, 1AH_M20H_ROOT, &amd_pmf_legacy_smu_regs) },
> +	{ PCI_DEVICE_DATA(AMD, 1AH_M60H_ROOT, &amd_pmf_legacy_smu_regs) },

I don't feel super strongly about this, but I generally feel that the 
word 'legacy' ages poorly.  How about a 'v1', 'v2', etc layout?

>   	{ }
>   };
>   
> @@ -536,6 +543,19 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev)
>   	}
>   }
>   
> +static int amd_pmf_get_smu_mb_offset(struct amd_pmf_dev *pdev, struct pci_dev *rdev)
> +{
> +	const struct pci_device_id *id;
> +
> +	id = pci_match_id(pmf_pci_ids, rdev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	pdev->smu_regs = (const struct amd_pmf_smu_regs *)id->driver_data;
> +
> +	return 0;
> +}
> +
>   static const struct acpi_device_id amd_pmf_acpi_ids[] = {
>   	{"AMDI0100", 0x100},
>   	{"AMDI0102", 0},
> @@ -624,6 +644,11 @@ static int amd_pmf_probe(struct platform_device *pdev)
>   	if (err)
>   		return err;
>   
> +	/* Populate smu_regs with SoC-specific SMU mailbox register offsets */
> +	err = amd_pmf_get_smu_mb_offset(dev, rdev);
> +	if (err)
> +		return err;
> +
>   	apmf_acpi_init(dev);
>   	platform_set_drvdata(pdev, dev);
>   	amd_pmf_dbgfs_register(dev);
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 752fa5dd2267..7a8fd9d399de 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -29,6 +29,10 @@
>   #define PCI_DEVICE_ID_AMD_1AH_M20H_ROOT 0x1507
>   #define PCI_DEVICE_ID_AMD_1AH_M60H_ROOT 0x1122
>   
> +/* Aliases required by PCI_DEVICE_DATA() macro naming convention */
> +#define PCI_DEVICE_ID_AMD_CPU_ID_RMB	AMD_CPU_ID_RMB
> +#define PCI_DEVICE_ID_AMD_CPU_ID_PS	AMD_CPU_ID_PS
> +
>   struct cookie_header {
>   	u32 sign;
>   	u32 length;
> @@ -392,6 +396,13 @@ struct pmf_cbi_ring_buffer {
>   	int tail;
>   };
>   
> +/* SoC-specific SMU mailbox register offsets */
> +struct amd_pmf_smu_regs {
> +	u32 msg_reg;
> +	u32 resp_reg;
> +	u32 arg_reg;
> +};
> +
>   struct amd_pmf_dev {
>   	void __iomem *regbase;
>   	void __iomem *smu_virt_addr;
> @@ -444,6 +455,7 @@ struct amd_pmf_dev {
>   	struct mutex cbi_mutex;		     /* Protects ring buffer access */
>   	struct mutex metrics_mutex;
>   	u32 bios_output[BIOS_OUTPUT_MAX];
> +	const struct amd_pmf_smu_regs *smu_regs;
>   };
>   
>   struct apmf_sps_prop_granular_v2 {