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