Hi Ilpo,
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.
>
> 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?
>
>> What am I missing in your comments?
>>
>>
>>>
>>> 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?
Thanks,
Shyam
