Re: [PATCH v4 08/11] platform/x86/amd/pmc: Update IP information structure for newer SoCs

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

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

> The latest AMD processors include additional IP blocks that must be turned
> off before transitioning to low power. PMFW provides an interface to
> retrieve debug information from each IP block, which is useful for
> diagnosing issues if the system fails to enter or exit low power states,
> or for profiling which IP block takes more time. Add support for using
> this information within the driver.
> 
> 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/pmc.c | 42 +++++++++++++++++++++++++++---
>  drivers/platform/x86/amd/pmc/pmc.h |  1 +
>  2 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 7c3204110bf8..5b99845d0914 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -95,6 +95,34 @@ struct amd_pmc_bit_map {
>  	u32 bit_mask;
>  };
>  
> +static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
> +	{"DISPLAY",     BIT(0)},
> +	{"CPU",         BIT(1)},
> +	{"GFX",         BIT(2)},
> +	{"VDD",         BIT(3)},
> +	{"VDD_CCX",     BIT(4)},
> +	{"ACP",         BIT(5)},
> +	{"VCN_0",       BIT(6)},
> +	{"VCN_1",       BIT(7)},
> +	{"ISP",         BIT(8)},
> +	{"NBIO",        BIT(9)},
> +	{"DF",          BIT(10)},
> +	{"USB3_0",      BIT(11)},
> +	{"USB3_1",      BIT(12)},
> +	{"LAPIC",       BIT(13)},
> +	{"USB3_2",      BIT(14)},
> +	{"USB4_RT0",	BIT(15)},
> +	{"USB4_RT1",	BIT(16)},
> +	{"USB4_0",      BIT(17)},
> +	{"USB4_1",      BIT(18)},
> +	{"MPM",         BIT(19)},
> +	{"JPEG_0",      BIT(20)},
> +	{"JPEG_1",      BIT(21)},
> +	{"IPU",         BIT(22)},
> +	{"UMSCH",       BIT(23)},
> +	{"VPE",         BIT(24)},
> +};
> +
>  static const struct amd_pmc_bit_map soc15_ip_blk[] = {
>  	{"DISPLAY",	BIT(0)},
>  	{"CPU",		BIT(1)},
> @@ -162,14 +190,22 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
>  	case AMD_CPU_ID_CB:
>  		dev->num_ips = 12;
>  		dev->smu_msg = 0x538;
> +		dev->ips_ptr = (struct amd_pmc_bit_map *)soc15_ip_blk;
>  		break;
>  	case AMD_CPU_ID_PS:
>  		dev->num_ips = 21;
>  		dev->smu_msg = 0x538;
> +		dev->ips_ptr = (struct amd_pmc_bit_map *)soc15_ip_blk;

Could you please put num_ips and ips_ptr lines next to each other as they 
kind of belong together.

I'm a bit surprised you need the casts in these assignments. ...Surprised 
enough I finally went to check what's going on.

And sadly... It turns out you needed the cast only to get rid of const 
which is very bad practice. You really should almost never do that.

The correct solution is highlighted below.

>  		break;
>  	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
>  	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> -		dev->num_ips = ARRAY_SIZE(soc15_ip_blk);
> +		if (boot_cpu_data.x86_model == 0x70) {
> +			dev->num_ips = ARRAY_SIZE(soc15_ip_blk_v2);
> +			dev->ips_ptr = (struct amd_pmc_bit_map *)soc15_ip_blk_v2;
> +		} else {
> +			dev->num_ips = ARRAY_SIZE(soc15_ip_blk);
> +			dev->ips_ptr = (struct amd_pmc_bit_map *)soc15_ip_blk;
> +		}
>  		dev->smu_msg = 0x938;
>  		break;
>  	}
> @@ -337,8 +373,8 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
>  
>  	seq_puts(s, "\n=== Active time (in us) ===\n");
>  	for (idx = 0 ; idx < dev->num_ips ; idx++) {
> -		if (soc15_ip_blk[idx].bit_mask & dev->active_ips)
> -			seq_printf(s, "%-8s : %lld\n", soc15_ip_blk[idx].name,
> +		if (dev->ips_ptr[idx].bit_mask & dev->active_ips)
> +			seq_printf(s, "%-8s : %lld\n", dev->ips_ptr[idx].name,
>  				   table.timecondition_notmet_lastcapture[idx]);
>  	}
>  
> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
> index 69870013c0fc..f6d9a7c37588 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.h
> +++ b/drivers/platform/x86/amd/pmc/pmc.h
> @@ -62,6 +62,7 @@ struct amd_pmc_dev {
>  	bool disable_8042_wakeup;
>  	struct amd_mp2_dev *mp2;
>  	struct stb_arg stb_arg;
> +	struct amd_pmc_bit_map *ips_ptr;

const struct

--
 i.

>  };
>  
>  void amd_pmc_process_restore_quirks(struct amd_pmc_dev *dev);
>