Re: [PATCH v4 05/11] platform/x86/amd/pmc: Define enum for S2D/PMC msg_port and add helper function

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

On Thu, 7 Nov 2024, Shyam Sundar S K wrote:

> To distinguish between the PMC message port and the S2D (Spill to DRAM)
> message port, replace the use of 0 and 1 with an enum.
> 
> To avoid printing the S2D or PMC port multiple times in debug print,
> add new routine to retrieve the message port information, which can be
> used to print the right msg_port getting used.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 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/mp1_stb.c |  8 ++++----
>  drivers/platform/x86/amd/pmc/pmc.c     | 15 ++++++++++-----
>  drivers/platform/x86/amd/pmc/pmc.h     |  5 +++++
>  3 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> index 5c03ac92558f..9ee629db9af9 100644
> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -155,7 +155,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>  
>  	/* Spill to DRAM num_samples uses separate SMU message port */
> -	dev->msg_port = 1;
> +	dev->msg_port = MSG_PORT_S2D;
>  
>  	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
>  	if (ret)
> @@ -172,7 +172,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	/* Get the num_samples to calculate the last push location */
>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
>  	/* Clear msg_port for other SMU operation */
> -	dev->msg_port = 0;
> +	dev->msg_port = MSG_PORT_PMC;
>  	if (ret) {
>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>  		return ret;
> @@ -267,7 +267,7 @@ int amd_stb_s2d_init(struct amd_pmc_dev *dev)
>  	}
>  
>  	/* Spill to DRAM feature uses separate SMU message port */
> -	dev->msg_port = 1;
> +	dev->msg_port = MSG_PORT_S2D;
>  
>  	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>  	if (size != S2D_TELEMETRY_BYTES_MAX)
> @@ -285,7 +285,7 @@ int amd_stb_s2d_init(struct amd_pmc_dev *dev)
>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>  
>  	/* Clear msg_port for other SMU operation */
> -	dev->msg_port = 0;
> +	dev->msg_port = MSG_PORT_PMC;
>  
>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>  	if (!dev->stb_virt_addr)
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 7726a05091a5..ce0050699a5a 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -449,11 +449,16 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>  			    &amd_pmc_idlemask_fops);
>  }
>  
> +static char *amd_pmc_print_msg_port(struct amd_pmc_dev *dev)

Change name as this doesn't print anything.

> +{
> +	return dev->msg_port ? "S2D" : "PMC";

== ...

Although a switch/case might be preferrable here (return "invalid" or 
something in default branch).

> +}
> +
>  static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
>  {
>  	u32 value, message, argument, response;
>  
> -	if (dev->msg_port) {
> +	if (dev->msg_port == MSG_PORT_S2D) {
>  		message = AMD_S2D_REGISTER_MESSAGE;
>  		argument = AMD_S2D_REGISTER_ARGUMENT;
>  		response = AMD_S2D_REGISTER_RESPONSE;
> @@ -464,13 +469,13 @@ static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
>  	}
>  
>  	value = amd_pmc_reg_read(dev, response);
> -	dev_dbg(dev->dev, "AMD_%s_REGISTER_RESPONSE:%x\n", dev->msg_port ? "S2D" : "PMC", value);
> +	dev_dbg(dev->dev, "AMD_%s_REGISTER_RESPONSE:%x\n", amd_pmc_print_msg_port(dev), value);
>  
>  	value = amd_pmc_reg_read(dev, argument);
> -	dev_dbg(dev->dev, "AMD_%s_REGISTER_ARGUMENT:%x\n", dev->msg_port ? "S2D" : "PMC", value);
> +	dev_dbg(dev->dev, "AMD_%s_REGISTER_ARGUMENT:%x\n", amd_pmc_print_msg_port(dev), value);
>  
>  	value = amd_pmc_reg_read(dev, message);
> -	dev_dbg(dev->dev, "AMD_%s_REGISTER_MESSAGE:%x\n", dev->msg_port ? "S2D" : "PMC", value);
> +	dev_dbg(dev->dev, "AMD_%s_REGISTER_MESSAGE:%x\n", amd_pmc_print_msg_port(dev), value);
>  }
>  
>  int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret)
> @@ -480,7 +485,7 @@ int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool r
>  
>  	mutex_lock(&dev->lock);
>  
> -	if (dev->msg_port) {
> +	if (dev->msg_port == MSG_PORT_S2D) {
>  		message = AMD_S2D_REGISTER_MESSAGE;
>  		argument = AMD_S2D_REGISTER_ARGUMENT;
>  		response = AMD_S2D_REGISTER_RESPONSE;
> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
> index 188284feca72..8252d3a52849 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.h
> +++ b/drivers/platform/x86/amd/pmc/pmc.h
> @@ -14,6 +14,11 @@
>  #include <linux/types.h>
>  #include <linux/mutex.h>
>  
> +enum s2d_msg_port {
> +	MSG_PORT_PMC,
> +	MSG_PORT_S2D,
> +};
> +
>  struct amd_mp2_dev {
>  	void __iomem *mmio;
>  	void __iomem *vslbase;
> 

-- 
 i.