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

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 11/7/2024 16:32, Ilpo Järvinen wrote:
> 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.
> 

ah! I missed this badly.. :-( Thanks for pointing this out.

Thanks,
Shyam

> 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);
>>