| Message ID | 20231223032548.1680738-5-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Intel PMC Core GBE LTR regression fix | expand |
On Fri, 22 Dec 2023, David E. Box wrote: > Commit 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and > core_configure()") caused a network performance regression due to the GBE > LTR ignore that it added during probe. The fix will move the ignore to > occur at suspend-time (so as to not affect suspend power). This will > require the ability to enable the LTR again on resume. Modify > pmc_core_send_ltr_ignore() to allow enabling an LTR. > > Fixes: 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and core_configure()") > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > drivers/platform/x86/intel/pmc/adl.c | 2 +- > drivers/platform/x86/intel/pmc/cnp.c | 2 +- > drivers/platform/x86/intel/pmc/core.c | 9 ++++++--- > drivers/platform/x86/intel/pmc/core.h | 3 ++- > drivers/platform/x86/intel/pmc/mtl.c | 2 +- > drivers/platform/x86/intel/pmc/tgl.c | 2 +- > 6 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/intel/pmc/adl.c b/drivers/platform/x86/intel/pmc/adl.c > index 882f2d5d8937..fbe0678f766c 100644 > --- a/drivers/platform/x86/intel/pmc/adl.c > +++ b/drivers/platform/x86/intel/pmc/adl.c > @@ -327,7 +327,7 @@ int adl_core_init(struct pmc_dev *pmcdev) > * 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); > + pmc_core_send_ltr_ignore(pmcdev, 3, 1); > > return 0; > } > diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c > index 59298f184d0e..80f73242f9dd 100644 > --- a/drivers/platform/x86/intel/pmc/cnp.c > +++ b/drivers/platform/x86/intel/pmc/cnp.c > @@ -220,7 +220,7 @@ int cnp_core_init(struct pmc_dev *pmcdev) > * 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); > + pmc_core_send_ltr_ignore(pmcdev, 3, 1); > > return 0; > } > diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c > index 31905003bb05..aa44c6612af9 100644 > --- a/drivers/platform/x86/intel/pmc/core.c > +++ b/drivers/platform/x86/intel/pmc/core.c > @@ -476,7 +476,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) > +int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore) Nothing seems to need int for anything and it's used like a bool. So > { > struct pmc *pmc; > const struct pmc_reg_map *map; > @@ -514,7 +514,10 @@ int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value) > mutex_lock(&pmcdev->lock); > > reg = pmc_core_reg_read(pmc, map->ltr_ignore_offset); > - reg |= BIT(ltr_index); > + if (ignore) > + reg |= BIT(ltr_index); > + else > + reg &= ~BIT(ltr_index); > pmc_core_reg_write(pmc, map->ltr_ignore_offset, reg); > > mutex_unlock(&pmcdev->lock); > @@ -537,7 +540,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file, > if (err) > return err; > > - err = pmc_core_send_ltr_ignore(pmcdev, value); > + err = pmc_core_send_ltr_ignore(pmcdev, value, 1); > > return err == 0 ? count : err; > } > diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h > index ce9b9efc9790..90f2dbc4df72 100644 > --- a/drivers/platform/x86/intel/pmc/core.h > +++ b/drivers/platform/x86/intel/pmc/core.h > @@ -567,7 +567,8 @@ extern const struct pmc_reg_map arl_pchs_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); > +extern int > +pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore); extern is unnecessary. > 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 e75431325dda..ef78d14fc27f 100644 > --- a/drivers/platform/x86/intel/pmc/mtl.c > +++ b/drivers/platform/x86/intel/pmc/mtl.c > @@ -1023,7 +1023,7 @@ int mtl_core_init(struct pmc_dev *pmcdev) > * 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); > + pmc_core_send_ltr_ignore(pmcdev, 3, 1); > > if (ssram_init) > 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 91fd725951e5..8213961975fc 100644 > --- a/drivers/platform/x86/intel/pmc/tgl.c > +++ b/drivers/platform/x86/intel/pmc/tgl.c > @@ -315,7 +315,7 @@ int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp) > * 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); > + pmc_core_send_ltr_ignore(pmcdev, 3, 1); > > return 0; > } >
On Wed, 27 Dec 2023, Ilpo Järvinen wrote: > On Fri, 22 Dec 2023, David E. Box wrote: > > > Commit 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and > > core_configure()") caused a network performance regression due to the GBE > > LTR ignore that it added during probe. The fix will move the ignore to > > occur at suspend-time (so as to not affect suspend power). This will > > require the ability to enable the LTR again on resume. Modify > > pmc_core_send_ltr_ignore() to allow enabling an LTR. > > > > Fixes: 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and core_configure()") > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > --- > > drivers/platform/x86/intel/pmc/adl.c | 2 +- > > drivers/platform/x86/intel/pmc/cnp.c | 2 +- > > drivers/platform/x86/intel/pmc/core.c | 9 ++++++--- > > drivers/platform/x86/intel/pmc/core.h | 3 ++- > > drivers/platform/x86/intel/pmc/mtl.c | 2 +- > > drivers/platform/x86/intel/pmc/tgl.c | 2 +- > > 6 files changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/platform/x86/intel/pmc/adl.c b/drivers/platform/x86/intel/pmc/adl.c > > index 882f2d5d8937..fbe0678f766c 100644 > > --- a/drivers/platform/x86/intel/pmc/adl.c > > +++ b/drivers/platform/x86/intel/pmc/adl.c > > @@ -327,7 +327,7 @@ int adl_core_init(struct pmc_dev *pmcdev) > > * 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); > > + pmc_core_send_ltr_ignore(pmcdev, 3, 1); > > > > return 0; > > } > > diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c > > index 59298f184d0e..80f73242f9dd 100644 > > --- a/drivers/platform/x86/intel/pmc/cnp.c > > +++ b/drivers/platform/x86/intel/pmc/cnp.c > > @@ -220,7 +220,7 @@ int cnp_core_init(struct pmc_dev *pmcdev) > > * 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); > > + pmc_core_send_ltr_ignore(pmcdev, 3, 1); > > > > return 0; > > } > > diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c > > index 31905003bb05..aa44c6612af9 100644 > > --- a/drivers/platform/x86/intel/pmc/core.c > > +++ b/drivers/platform/x86/intel/pmc/core.c > > @@ -476,7 +476,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) > > +int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore) > > Nothing seems to need int for anything and it's used like a bool. So Note that I've merged this change without changing these into bool. Please consider making a follow up change to the type after next -rc1 when things have settled down. > > +++ b/drivers/platform/x86/intel/pmc/core.h > > @@ -567,7 +567,8 @@ extern const struct pmc_reg_map arl_pchs_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); > > +extern int > > +pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore); > > extern is unnecessary. While merging, I dropped this one as I had to resolve a conflict here anyway. Patches 3-5 are now in review-ilpo towards fixes branch. Please do double check they're correct because the conflicts were quite large.
diff --git a/drivers/platform/x86/intel/pmc/adl.c b/drivers/platform/x86/intel/pmc/adl.c index 882f2d5d8937..fbe0678f766c 100644 --- a/drivers/platform/x86/intel/pmc/adl.c +++ b/drivers/platform/x86/intel/pmc/adl.c @@ -327,7 +327,7 @@ int adl_core_init(struct pmc_dev *pmcdev) * 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); + pmc_core_send_ltr_ignore(pmcdev, 3, 1); return 0; } diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c index 59298f184d0e..80f73242f9dd 100644 --- a/drivers/platform/x86/intel/pmc/cnp.c +++ b/drivers/platform/x86/intel/pmc/cnp.c @@ -220,7 +220,7 @@ int cnp_core_init(struct pmc_dev *pmcdev) * 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); + pmc_core_send_ltr_ignore(pmcdev, 3, 1); return 0; } diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c index 31905003bb05..aa44c6612af9 100644 --- a/drivers/platform/x86/intel/pmc/core.c +++ b/drivers/platform/x86/intel/pmc/core.c @@ -476,7 +476,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) +int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore) { struct pmc *pmc; const struct pmc_reg_map *map; @@ -514,7 +514,10 @@ int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value) mutex_lock(&pmcdev->lock); reg = pmc_core_reg_read(pmc, map->ltr_ignore_offset); - reg |= BIT(ltr_index); + if (ignore) + reg |= BIT(ltr_index); + else + reg &= ~BIT(ltr_index); pmc_core_reg_write(pmc, map->ltr_ignore_offset, reg); mutex_unlock(&pmcdev->lock); @@ -537,7 +540,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file, if (err) return err; - err = pmc_core_send_ltr_ignore(pmcdev, value); + err = pmc_core_send_ltr_ignore(pmcdev, value, 1); return err == 0 ? count : err; } diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h index ce9b9efc9790..90f2dbc4df72 100644 --- a/drivers/platform/x86/intel/pmc/core.h +++ b/drivers/platform/x86/intel/pmc/core.h @@ -567,7 +567,8 @@ extern const struct pmc_reg_map arl_pchs_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); +extern int +pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore); 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 e75431325dda..ef78d14fc27f 100644 --- a/drivers/platform/x86/intel/pmc/mtl.c +++ b/drivers/platform/x86/intel/pmc/mtl.c @@ -1023,7 +1023,7 @@ int mtl_core_init(struct pmc_dev *pmcdev) * 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); + pmc_core_send_ltr_ignore(pmcdev, 3, 1); if (ssram_init) 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 91fd725951e5..8213961975fc 100644 --- a/drivers/platform/x86/intel/pmc/tgl.c +++ b/drivers/platform/x86/intel/pmc/tgl.c @@ -315,7 +315,7 @@ int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp) * 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); + pmc_core_send_ltr_ignore(pmcdev, 3, 1); return 0; }
Commit 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and core_configure()") caused a network performance regression due to the GBE LTR ignore that it added during probe. The fix will move the ignore to occur at suspend-time (so as to not affect suspend power). This will require the ability to enable the LTR again on resume. Modify pmc_core_send_ltr_ignore() to allow enabling an LTR. Fixes: 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and core_configure()") Signed-off-by: David E. Box <david.e.box@linux.intel.com> --- drivers/platform/x86/intel/pmc/adl.c | 2 +- drivers/platform/x86/intel/pmc/cnp.c | 2 +- drivers/platform/x86/intel/pmc/core.c | 9 ++++++--- drivers/platform/x86/intel/pmc/core.h | 3 ++- drivers/platform/x86/intel/pmc/mtl.c | 2 +- drivers/platform/x86/intel/pmc/tgl.c | 2 +- 6 files changed, 12 insertions(+), 8 deletions(-)