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@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.
>
Sure. Will do it.
Thanks,
Shyam
