| Message ID | 20241029155440.3499273-6-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86/amd/pmc: Updates to AMD PMC driver | expand |
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@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 ++++++++++++++++++++++++++++-- > 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? > 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? > } > > return 0; >
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@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 ++++++++++++++++++++++++++++-- >> 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[]. 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; >> >
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@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 ++++++++++++++++++++++++++++-- > >> 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.
On 11/5/2024 15:29, Ilpo Järvinen wrote: > 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@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 ++++++++++++++++++++++++++++-- >>>> 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. > Sure. Will do it. Thanks, Shyam
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; 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]); + } } return 0;