On Tue, 5 Nov 2024, Shyam Sundar S K wrote:
> On 11/1/2024 17:34, Ilpo Järvinen wrote:
> > On Tue, 29 Oct 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.
> >>
> >> Co-developed-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> >> ---
> >> drivers/platform/x86/amd/pmc/pmc.c | 42 ++++++++++++++++++++++++++++--
> >> 1 file changed, 40 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> >> index f9900a03391a..0bf4065153da 100644
> >> --- a/drivers/platform/x86/amd/pmc/pmc.c
> >> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> >> @@ -95,6 +95,35 @@ 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)},
> >> @@ -170,7 +199,10 @@ static void amd_pmc_get_ip_info(struct amd_pmc_dev *dev)
> >> break;
> >> case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> >> case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
> >> - dev->num_ips = 22;
> >> + if (boot_cpu_data.x86_model == 0x70)
> >> + dev->num_ips = 25;
> >> + else
> >> + dev->num_ips = 22;
> >
> > Could these use ARRAY_SIZE()? They're related to that array, aren't they?
>
> Yes, they are. ARRAY_SIZE() does return the number of elements in the
> array but there is a catch,
>
> both soc15_ip_blk[] and soc15_ip_blk_v2[] have a last empty element,
> so when ARRAY_SIZE() is used we end up getting n+1 element (i.e along
> with the last empty element). So, what would you advise?
>
> 1) remove the last empty element in the array? i.e. something like this?
>
> static const struct amd_pmc_bit_map soc15_ip_blk_v2[] = {
> {"DISPLAY", BIT(0)},
> ...
> {"VPE", BIT(24)},
> - {} /* remove this */
> };
>
> 2) or just do, `ARRAY_SIZE() - 1` so that we don't need to touch the
> existing soc15_ip_blk[] and soc15_ip_blk_v2[].
The iteration seems to always ->num_ips based so the terminator seems
superfluous. The cases with smaller num_ips cannot use terminator anyway
as they share the larger array.
I propose you make a separate change out removing the terminator and
migrating to use ARRAY_SIZE() in the existing code, don't forget to add
the #include for it. Then add just this new thing in this patch.
--
i.
>
>
> I am inline with your comments on the other patches, will address them
> in v3.
>
> Thanks,
> Shyam
>
> >
> >> dev->smu_msg = 0x938;
> >> break;
> >> }
> >> @@ -338,9 +370,15 @@ 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)
> >> + if (dev->cpu_id == PCI_DEVICE_ID_AMD_1AH_M20H_ROOT &&
> >> + boot_cpu_data.x86_model == 0x70) {
> >> + if (soc15_ip_blk_v2[idx].bit_mask & dev->active_ips)
> >> + seq_printf(s, "%-8s : %lld\n", soc15_ip_blk_v2[idx].name,
> >> + table.timecondition_notmet_lastcapture[idx]);
> >> + } else if (soc15_ip_blk[idx].bit_mask & dev->active_ips) {
> >> seq_printf(s, "%-8s : %lld\n", soc15_ip_blk[idx].name,
> >> table.timecondition_notmet_lastcapture[idx]);
> >> + }
> >
> > Why not have amd_pmc_get_ip_info() prepare a pointer into 'dev' to the
> > relevant soc15_ip_blk_v2/soc15_ip_blk rather than trying to pick one here?
> >
>
> Makes sense. Ack.
>
> >> }
> >>
> >> return 0;
> >>
> >
>
