Hi Ilpo,
On 5/25/2023 5:28 PM, Ilpo Järvinen wrote:
> On Thu, 25 May 2023, Shyam Sundar S K wrote:
>> On 5/25/2023 4:14 PM, Ilpo Järvinen wrote:
>>> On Thu, 25 May 2023, Shyam Sundar S K wrote:
>>>> On 5/25/2023 3:29 PM, Ilpo Järvinen wrote:
>>>>> On Thu, 25 May 2023, Shyam Sundar S K wrote:
>>>>>> On 5/23/2023 1:56 PM, Ilpo Järvinen wrote:
>>>>>>> On Tue, 16 May 2023, Shyam Sundar S K wrote:
>>>>>>>
>>>>>>>> Add a helper routine to check the underlying cpu id, that can be used
>>>>>>>> across the PMC driver to remove the duplicate code.
>>>>>>>>
>>>>>>>> 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.c | 17 ++++++++++++++---
>>>>>>>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
>>>>>>>> index e2439fda5c02..7e5e6afb3410 100644
>>>>>>>> --- a/drivers/platform/x86/amd/pmc.c
>>>>>>>> +++ b/drivers/platform/x86/amd/pmc.c
>>>>>>>> @@ -564,6 +564,18 @@ static void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>>>>>>>> debugfs_remove_recursive(dev->dbgfs_dir);
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static bool amd_pmc_check_sup_cpuid(struct amd_pmc_dev *dev)
>>>>>>>
>>>>>>> Does sup refer to "supported" or some other acronym? If the latter,
>>>>>>
>>>>>> Yes, please read that as "supported"
>>>>>>
>>>>>>> you should mention/open it in the changelog and/or in a comment. If the
>>>>>>> former, the function naming seems too generic (an observation entirely
>>>>>>> based on how/where the function is used, you're not exactly verbose on
>>>>>>> what this actually checks for other than what looks like a set of CPU
>>>>>>> IDs but clearly there's more behind it).
>>>>>>
>>>>>> OK. renaming the function as amd_pmc_is_cpu_supported() would be fine?
>>>>>
>>>>> This makes things odder, it gets used in two places:
>>>>>
>>>>> if (enable_stb) {
>>>>> if (amd_pmc_check_sup_cpuid(dev))
>>>>> debugfs_create_file(..., &amd_pmc_stb_debugfs_fops_v2);
>>>>> else
>>>>> debugfs_create_file(..., &amd_pmc_stb_debugfs_fops);
>>>>> }
>>>>>
>>>>> What about that else branch (PMC is not supported so who does that make
>>>>> sense when the file is called pmc.c)? And here:
>>>>
>>>> I did not understand the actual concern.
>>>
>>> The file is cammed pmc.c and states "AMD SoC Power Management Controller
>>> Driver", so PMC, right?
>>
>> Yes.
>>
>>>
>>> You propose adding function called amd_pmc_is_cpu_supported() which to me
>>> reads "is PMC supported on this CPU?" since you don't have anything else
>>> in the function name to quality a sub-feature that would be be tested for
>>> supported or not.
>>
>> The function naming convention across this file is amd_pmc_*. So would
>> like to have it as "amd_pmc_is_cpu_supported()". And yes, this should be
>> generic helper function that should be used across the PMC and STB
>> functions interchangeably, as the underlying CPU where it runs remains
>> the same.
>
> Okay, so I read this as stating that its testing for a larger set of
> features than what can be read from the code.
>
>>> It begs a question, why probe doesn't always return error when PMC is not
>>> supported by the CPU? Can you see the problem now?
>>
>> PMC driver probe happens based on the _HID amd_pmc_acpi_ids[] and probe
>> failures are handled.
>>
>> The only intention to look for CPU ID's through the PCI root port is
>> handle the Firmware changes across CPU generations.
>>
>> Hope this clears the question.
>>
>>>
>>>> STB is an on-demand debug
>>>> feature and that can only be enabled when enable_stb module param is set.
>>>>
>>>> The check for amd_pmc_check_sup_cpuid() is to see if the underlying CPU
>>>> (with the right PMFW support) supported is pre-Rembrandt, then Spill to
>>>> DRAM is not supported. So reading the STB buffer is a different
>>>> mechanism and that has been handled in the amd_pmc_stb_debugfs_fops().
>>>> But the platforms after Rmebrandt, supports spilling to DRAM, and that
>>>> has been handled in amd_pmc_stb_debugfs_fops_v2().
>>>
>>> This kind of information should be stated the changelog up front.
>>
>> I don't think I am touching that part of the code to explain this stuff
>> in the changelog.
>>
>> Let us purely keep this as a helper function to check the underlying CPU
>> that can be used across the entire pmc.c file. Makes sense?
>>
>>>
>>> So is that function testing support for Spill to DRAM? Clearly,
>>> Spill-to-DRAM != PMC, that's the second problem here related to function
>>> naming.
>>
>> PMC driver is a FW assisted driver for S2Idle path on AMD platforms. But
>> for whatever reason if the Supend/resume fails to happen, we need to
>> have a debug mechanims to address the field issues.
>>
>> Since it's FW assisted, the FW also provides a way to know what happened
>> behind the scenes and that debug mechanism is called STB (Smart Trace
>> Buffer).
>>
>> Intially when we started the STB was supposed to be a small interface
>> and it has evolved a lot over time. And maybe at times you will see that
>> PMC and STB are used in conjunction.
>>
>> If its becoming confusing for the community, maybe I will come up with a
>> way to decouple PMC and STB sometime soon.
>>
>> Thoughts?
>
> This kinda digressed and didn't answer my question at all (you mentioned
> Spill to DRAM zero times in your reply but went to something called STB).
> But it could just that I'm not familiar enough with all the details here.
>
> My question boiled down to if this (your own words) is true or not:
>
> "The check for amd_pmc_check_sup_cpuid() is to see if the underlying CPU
> (with the right PMFW support) supported is pre-Rembrandt, then Spill
> to DRAM is not supported. ... But the platforms after Rmebrandt, supports
> spilling to DRAM, ..."
>
> If the code is solely used for testing whether Spill to DRAM is supported
> or not, it feels odd to name it something more generic than that. But
> given what you said above, I guess the answer here is that it can be used
> to test Spill to DRAM among other things, and this particular patch
> just doesn't do those other things so it looks odd but is okay still.
> Is that the correct interpretation here?
>
There are 3 parts to it:
a) PMC driver[1] used without STB enabled
b) PMC driver used with STB enabled (but on older platforms before to
Rembrandt - where Spilling to DRAM is *not supported*[3])
c) PMC driver used with STB enabled (with platforms starting Rembrandt
and later - where the spilling to DRAM is *supported* [4])
Since we would need helper checks across all 3 cases, it should be okay
to name it as amd_pmc_is_cpu_supported().
[1] which is meant for s2idle suspend/resume transitions
[2] STB (Smart Trace Buffer) which is a shared buffer across several IP
blocks within the SoC.
[3] handled in amd_pmc_stb_debugfs_fops
[4] handled in amd_pmc_stb_debugfs_fops_v2
>>>>> static int amd_pmc_probe(...)
>>>>> {
>>>>> ...
>>>>> if (enable_stb && amd_pmc_check_sup_cpuid(dev)) {
>>>>> err = amd_pmc_s2d_init(dev);
>>>>> if (err)
>>>>> ...goto + returns error
>>>>> }
>>>>>
>>>>>
>>>>> If enable_stb is not set, pmc not being supported is not going to return
>>>>> error?
>>>>>
>>>>>
>>>>
>>>> here we return only whne there is failure in s2d_init() - right?
>>>>
>>>> And yes, if enable_stb is not set, there is no need to init the s2d path.
>>>
>>> s2d is short for Spill to DRAM I guess?
>>
>> Yes.
>>
>>>
>>> So in both occassions amd_pmc_check_sup_cpuid() testing support for s2d
>>> rather than PMC (it certainly looks that way)? If so, name the function
>>> accordingly (I suggest amd_pmc_s2d_supported()) and put a little bit more
>>> explanation into the changelog and we're done here.
>>>
>>>
>>
>> IMHO. Based on the above details, amd_pmc_is_cpu_supported() should be
>> generic name. Do you still see a concern?
>
> Given you seem to be certain there's no error or some detail missing, I
> won't object it being the way you want to put it.
>
>
Thank you, have sent a v4.
Thanks,
Shyam
