Re: [PATCH] platform/x86/intel/pmc: Remove GBE LTR ignore

[David E. Box]

On Thu, 2023-12-07 at 13:18 -0600, Mario Limonciello wrote:
> On 12/7/2023 13:02, Hans de Goede wrote:
> > Hi David,
> > 
> > On 12/7/23 19:23, David E. Box wrote:
> > > Commit 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and
> > > core_configure()") moved the GBE LTR ignore workaround from core.c to PCH
> > > code and added it new for Cannon Lake PCH in cnp.c. This introduced a
> > > network performance regression on a CNP PCH system [1] which has been
> > > observed on other PCH architectures during testing. Remove the probe-time
> > > GBE LTR ignore for all platforms. While this will prevent performance
> > > degradation, it will also limit the deepest SoC Package C state that can
> > > be
> > > entered at runtime while a LAN cable is attached.
> > > 
> > > Reported-by: James Hutchinson <jahutchinson99@xxxxxxxxxxxxxx>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218143 [1]
> > > Fixes: 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and
> > > core_configure()")
> > > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > > Tested-by: James Hutchinson <jahutchinson99@xxxxxxxxxxxxxx>
> > 
> > So it seems to me that to fix 804951203aa5 the only thing
> > which needs to be done is remove the pmc_core_send_ltr_ignore()
> > call from cnp_core_init(), the other changes here are NOT
> > related to fixing the regression.
> > 
> > So IMHO it seems better to split this into 2 patches?
> > 
> > Also if this block the system from reaching PC10 should
> > this then not at least be done at suspend time and
> > undone at resume ?  I'm not seeing anything in the current
> > code which does an equivalent on suspend/resume, so it
> > seems to me like this will cause a significant increase
> > on suspended power-usage if an ethernet cable is attached ?

We don't have many systems attached to onboard LAN. I initially tested on an
Alder Lake and D3 during suspend was enough to allow PC10 and s0ix without
ignoring the LTR. But with your comment I tried on a Tiger Lake and indeed it
could not get to PC10 during suspend with D3 only. So I'll send a V2 with those
patches. Thanks.

> 
> In addition to system suspend on these system does GBE support runtime PM?

LTR is the runtime pm mechanism for our GBE. The problem is the hardware. GBE
wasn't provided an LTR register large enough to write a value to get to PC10. As
is the maximum LTR value it can write will only get to PC8 or PC9 depending on
the platform.

David

> 
> If so, would it make sense to also have a Linux communication path from 
> the GBE driver to this driver as part of the runtime PM callbacks?
> 
> Then if the the GBE goes into runtime PM it could tell this driver to 
> waive LTR and if exits runtime PM it could tell this driver to stop 
> waiving LTR.
> 
> > 
> > Regards,
> > 
> > Hans
> > 
> > 
> > 
> > > ---
> > >   drivers/platform/x86/intel/pmc/adl.c  | 6 ------
> > >   drivers/platform/x86/intel/pmc/cnp.c  | 6 ------
> > >   drivers/platform/x86/intel/pmc/core.c | 2 +-
> > >   drivers/platform/x86/intel/pmc/core.h | 1 -
> > >   drivers/platform/x86/intel/pmc/mtl.c  | 6 ------
> > >   drivers/platform/x86/intel/pmc/tgl.c  | 5 -----
> > >   6 files changed, 1 insertion(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/platform/x86/intel/pmc/adl.c
> > > b/drivers/platform/x86/intel/pmc/adl.c
> > > index 64c492391ede..e4a421ca64be 100644
> > > --- a/drivers/platform/x86/intel/pmc/adl.c
> > > +++ b/drivers/platform/x86/intel/pmc/adl.c
> > > @@ -321,11 +321,5 @@ int adl_core_init(struct pmc_dev *pmcdev)
> > >   
> > >         pmc_core_get_low_power_modes(pmcdev);
> > >   
> > > -       /* Due to a hardware limitation, the GBE LTR blocks PC10
> > > -        * when a cable is attached. Tell the PMC to ignore it.
> > > -        */
> > > -       dev_dbg(&pmcdev->pdev->dev, "ignoring GBE LTR\n");
> > > -       pmc_core_send_ltr_ignore(pmcdev, 3);
> > > -
> > >         return 0;
> > >   }
> > > diff --git a/drivers/platform/x86/intel/pmc/cnp.c
> > > b/drivers/platform/x86/intel/pmc/cnp.c
> > > index 59298f184d0e..416d3a0c3615 100644
> > > --- a/drivers/platform/x86/intel/pmc/cnp.c
> > > +++ b/drivers/platform/x86/intel/pmc/cnp.c
> > > @@ -216,11 +216,5 @@ int cnp_core_init(struct pmc_dev *pmcdev)
> > >   
> > >         pmc_core_get_low_power_modes(pmcdev);
> > >   
> > > -       /* Due to a hardware limitation, the GBE LTR blocks PC10
> > > -        * when a cable is attached. Tell the PMC to ignore it.
> > > -        */
> > > -       dev_dbg(&pmcdev->pdev->dev, "ignoring GBE LTR\n");
> > > -       pmc_core_send_ltr_ignore(pmcdev, 3);
> > > -
> > >         return 0;
> > >   }
> > > diff --git a/drivers/platform/x86/intel/pmc/core.c
> > > b/drivers/platform/x86/intel/pmc/core.c
> > > index 983e3a8f4910..7c6a74957d57 100644
> > > --- a/drivers/platform/x86/intel/pmc/core.c
> > > +++ b/drivers/platform/x86/intel/pmc/core.c
> > > @@ -462,7 +462,7 @@ static int pmc_core_pll_show(struct seq_file *s, void
> > > *unused)
> > >   }
> > >   DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
> > >   
> > > -int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
> > > +static int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
> > >   {
> > >         struct pmc *pmc;
> > >         const struct pmc_reg_map *map;
> > > diff --git a/drivers/platform/x86/intel/pmc/core.h
> > > b/drivers/platform/x86/intel/pmc/core.h
> > > index 6d7673145f90..3bbdb41a754f 100644
> > > --- a/drivers/platform/x86/intel/pmc/core.h
> > > +++ b/drivers/platform/x86/intel/pmc/core.h
> > > @@ -493,7 +493,6 @@ extern const struct pmc_reg_map mtl_ioem_reg_map;
> > >   
> > >   extern void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev);
> > >   extern int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev);
> > > -extern int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value);
> > >   
> > >   int pmc_core_resume_common(struct pmc_dev *pmcdev);
> > >   int get_primary_reg_base(struct pmc *pmc);
> > > diff --git a/drivers/platform/x86/intel/pmc/mtl.c
> > > b/drivers/platform/x86/intel/pmc/mtl.c
> > > index 38c2f946ec23..33d32a76c43a 100644
> > > --- a/drivers/platform/x86/intel/pmc/mtl.c
> > > +++ b/drivers/platform/x86/intel/pmc/mtl.c
> > > @@ -1065,11 +1065,5 @@ int mtl_core_init(struct pmc_dev *pmcdev)
> > >         pmc_core_get_low_power_modes(pmcdev);
> > >         mtl_punit_pmt_init(pmcdev);
> > >   
> > > -       /* Due to a hardware limitation, the GBE LTR blocks PC10
> > > -        * when a cable is attached. Tell the PMC to ignore it.
> > > -        */
> > > -       dev_dbg(&pmcdev->pdev->dev, "ignoring GBE LTR\n");
> > > -       pmc_core_send_ltr_ignore(pmcdev, 3);
> > > -
> > >         return pmc_core_ssram_get_lpm_reqs(pmcdev);
> > >   }
> > > diff --git a/drivers/platform/x86/intel/pmc/tgl.c
> > > b/drivers/platform/x86/intel/pmc/tgl.c
> > > index d5f1d2223c5a..7e6f5739a197 100644
> > > --- a/drivers/platform/x86/intel/pmc/tgl.c
> > > +++ b/drivers/platform/x86/intel/pmc/tgl.c
> > > @@ -265,11 +265,6 @@ int tgl_core_init(struct pmc_dev *pmcdev)
> > >   
> > >         pmc_core_get_low_power_modes(pmcdev);
> > >         pmc_core_get_tgl_lpm_reqs(pmcdev->pdev);
> > > -       /* Due to a hardware limitation, the GBE LTR blocks PC10
> > > -        * when a cable is attached. Tell the PMC to ignore it.
> > > -        */
> > > -       dev_dbg(&pmcdev->pdev->dev, "ignoring GBE LTR\n");
> > > -       pmc_core_send_ltr_ignore(pmcdev, 3);
> > >   
> > >         return 0;
> > >   }
> > > 
> > > base-commit: 35ddd61cf023b5deb2b7e9f1627abef053281c0a
> > 
> > 
>