Hi Ilpo,
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. 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().
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.
Thanks,
Shyam
