Re: [PATCH v3 3/4] platform/x86/amd: pmc: Add helper function to check the cpu id

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4219 bytes --]

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@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.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?

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.

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?

> 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.

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.

> 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?

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.


-- 
 i.