| Message ID | 20230516091308.3905113-4-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Updates to AMD PMC driver | expand |
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, 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).
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? >
Hi Shyam, On 5/25/23 11:46, 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? Yes that should be fine. Regards, Hans
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: 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?
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@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. 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
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.
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@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? 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
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@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? > > 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? > >>> 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.
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@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? >> >> 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
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) +{ + switch (dev->cpu_id) { + case AMD_CPU_ID_YC: + case AMD_CPU_ID_CB: + case AMD_CPU_ID_PS: + return true; + default: + return false; + } +} + static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev) { dev->dbgfs_dir = debugfs_create_dir("amd_pmc", NULL); @@ -575,8 +587,7 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev) &amd_pmc_idlemask_fops); /* Enable STB only when the module_param is set */ if (enable_stb) { - if (dev->cpu_id == AMD_CPU_ID_YC || dev->cpu_id == AMD_CPU_ID_CB || - dev->cpu_id == AMD_CPU_ID_PS) + if (amd_pmc_check_sup_cpuid(dev)) debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev, &amd_pmc_stb_debugfs_fops_v2); else @@ -1036,7 +1047,7 @@ static int amd_pmc_probe(struct platform_device *pdev) mutex_init(&dev->lock); - if (enable_stb && (dev->cpu_id == AMD_CPU_ID_YC || dev->cpu_id == AMD_CPU_ID_CB)) { + if (enable_stb && amd_pmc_check_sup_cpuid(dev)) { err = amd_pmc_s2d_init(dev); if (err) goto err_pci_dev_put;