| Message ID | 20210305190608.1834164-1-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms | expand |
On 3/5/2021 21:06, David E. Box wrote: > Due to a HW limitation, the Latency Tolerance Reporting (LTR) value > programmed in the Tiger Lake GBE controller is not large enough to allow > the platform to enter Package C10, which in turn prevents the platform from > achieving its low power target during suspend-to-idle. Ignore the GBE LTR > value on Tiger Lake. LTR ignore functionality is currently performed solely > by a debugfs write call. Split out the LTR code into its own function that > can be called by both the debugfs writer and by this work around. > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> > Cc: intel-wired-lan@lists.osuosl.org > --- > drivers/platform/x86/intel_pmc_core.c | 55 ++++++++++++++++++++------- > 1 file changed, 42 insertions(+), 13 deletions(-) > > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c > index ee2f757515b0..ab31eb646a1a 100644 > --- a/drivers/platform/x86/intel_pmc_core.c > +++ b/drivers/platform/x86/intel_pmc_core.c > @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused) > } > DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); > > -static ssize_t pmc_core_ltr_ignore_write(struct file *file, > - const char __user *userbuf, > - size_t count, loff_t *ppos) > +static int pmc_core_write_ltr_ignore(u32 value) > { > struct pmc_dev *pmcdev = &pmc; > const struct pmc_reg_map *map = pmcdev->map; > - u32 val, buf_size, fd; > - int err; > - > - buf_size = count < 64 ? count : 64; > - > - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > - if (err) > - return err; > + u32 fd; > + int err = 0; > > mutex_lock(&pmcdev->lock); > > - if (val > map->ltr_ignore_max) { > + if (fls(value) > map->ltr_ignore_max) { > err = -EINVAL; > goto out_unlock; > } > > fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); > - fd |= (1U << val); > + fd |= value; > pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); > > out_unlock: > mutex_unlock(&pmcdev->lock); > + > + return err; > +} > + > +static ssize_t pmc_core_ltr_ignore_write(struct file *file, > + const char __user *userbuf, > + size_t count, loff_t *ppos) > +{ > + u32 buf_size, val; > + int err; > + > + buf_size = count < 64 ? count : 64; > + > + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > + if (err) > + return err; > + > + err = pmc_core_write_ltr_ignore(1U << val); > + > return err == 0 ? count : err; > } > > @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct dmi_system_id *id) > return 0; > } > > +static int quirk_ltr_ignore(u32 val) > +{ > + int err; > + > + err = pmc_core_write_ltr_ignore(val); > + > + return err; > +} > + > static const struct dmi_system_id pmc_core_dmi_table[] = { > { > .callback = quirk_xtal_ignore, > @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct platform_device *pdev) > pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); > dmi_check_system(pmc_core_dmi_table); > > + /* > + * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when > + * a cable is attached. Tell the PMC to ignore it. > + */ > + if (pmcdev->map == &tgl_reg_map) { I would suggest: if (pmcdev->map >= &tgl_reg_map) > + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); > + quirk_ltr_ignore(1U << 3); > + } > + > pmc_core_dbgfs_register(pmcdev); > > device_initialized = true; >
Hi Sasha, On Sun, 2021-03-07 at 10:39 +0200, Neftin, Sasha wrote: > On 3/5/2021 21:06, David E. Box wrote: > > Due to a HW limitation, the Latency Tolerance Reporting (LTR) value > > programmed in the Tiger Lake GBE controller is not large enough to > > allow > > the platform to enter Package C10, which in turn prevents the > > platform from > > achieving its low power target during suspend-to-idle. Ignore the > > GBE LTR > > value on Tiger Lake. LTR ignore functionality is currently > > performed solely > > by a debugfs write call. Split out the LTR code into its own > > function that > > can be called by both the debugfs writer and by this work around. > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> > > Cc: intel-wired-lan@lists.osuosl.org > > --- > > drivers/platform/x86/intel_pmc_core.c | 55 ++++++++++++++++++++-- > > ----- > > 1 file changed, 42 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/platform/x86/intel_pmc_core.c > > b/drivers/platform/x86/intel_pmc_core.c > > index ee2f757515b0..ab31eb646a1a 100644 > > --- a/drivers/platform/x86/intel_pmc_core.c > > +++ b/drivers/platform/x86/intel_pmc_core.c > > @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file > > *s, void *unused) > > } > > DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); > > > > -static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > - const char __user > > *userbuf, > > - size_t count, loff_t > > *ppos) > > +static int pmc_core_write_ltr_ignore(u32 value) > > { > > struct pmc_dev *pmcdev = &pmc; > > const struct pmc_reg_map *map = pmcdev->map; > > - u32 val, buf_size, fd; > > - int err; > > - > > - buf_size = count < 64 ? count : 64; > > - > > - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > - if (err) > > - return err; > > + u32 fd; > > + int err = 0; > > > > mutex_lock(&pmcdev->lock); > > > > - if (val > map->ltr_ignore_max) { > > + if (fls(value) > map->ltr_ignore_max) { > > err = -EINVAL; > > goto out_unlock; > > } > > > > fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); > > - fd |= (1U << val); > > + fd |= value; > > pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); > > > > out_unlock: > > mutex_unlock(&pmcdev->lock); > > + > > + return err; > > +} > > + > > +static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > + const char __user > > *userbuf, > > + size_t count, loff_t > > *ppos) > > +{ > > + u32 buf_size, val; > > + int err; > > + > > + buf_size = count < 64 ? count : 64; > > + > > + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > + if (err) > > + return err; > > + > > + err = pmc_core_write_ltr_ignore(1U << val); > > + > > return err == 0 ? count : err; > > } > > > > @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct > > dmi_system_id *id) > > return 0; > > } > > > > +static int quirk_ltr_ignore(u32 val) > > +{ > > + int err; > > + > > + err = pmc_core_write_ltr_ignore(val); > > + > > + return err; > > +} > > + > > static const struct dmi_system_id pmc_core_dmi_table[] = { > > { > > .callback = quirk_xtal_ignore, > > @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct > > platform_device *pdev) > > pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); > > dmi_check_system(pmc_core_dmi_table); > > > > + /* > > + * On TGL, due to a hardware limitation, the GBE LTR blocks > > PC10 when > > + * a cable is attached. Tell the PMC to ignore it. > > + */ > > + if (pmcdev->map == &tgl_reg_map) { > I would suggest: if (pmcdev->map >= &tgl_reg_map) This will already cover Rocket Lake since it uses Tiger Lake PCH. Those are the newest platforms this driver covers. Otherwise, I don't want to rely on this as the permanent solution. We can evaluate this on a per platform basis while persuing other measures to more properly resolve it. David > > + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); > > + quirk_ltr_ignore(1U << 3); > > + } > > + > > pmc_core_dbgfs_register(pmcdev); > > > > device_initialized = true; > > >
Hi, On 3/7/21 9:39 AM, Neftin, Sasha wrote: > On 3/5/2021 21:06, David E. Box wrote: >> Due to a HW limitation, the Latency Tolerance Reporting (LTR) value >> programmed in the Tiger Lake GBE controller is not large enough to allow >> the platform to enter Package C10, which in turn prevents the platform from >> achieving its low power target during suspend-to-idle. Ignore the GBE LTR >> value on Tiger Lake. LTR ignore functionality is currently performed solely >> by a debugfs write call. Split out the LTR code into its own function that >> can be called by both the debugfs writer and by this work around. >> >> Signed-off-by: David E. Box <david.e.box@linux.intel.com> >> Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> >> Cc: intel-wired-lan@lists.osuosl.org >> --- >> drivers/platform/x86/intel_pmc_core.c | 55 ++++++++++++++++++++------- >> 1 file changed, 42 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c >> index ee2f757515b0..ab31eb646a1a 100644 >> --- a/drivers/platform/x86/intel_pmc_core.c >> +++ b/drivers/platform/x86/intel_pmc_core.c >> @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused) >> } >> DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); >> -static ssize_t pmc_core_ltr_ignore_write(struct file *file, >> - const char __user *userbuf, >> - size_t count, loff_t *ppos) >> +static int pmc_core_write_ltr_ignore(u32 value) >> { >> struct pmc_dev *pmcdev = &pmc; >> const struct pmc_reg_map *map = pmcdev->map; >> - u32 val, buf_size, fd; >> - int err; >> - >> - buf_size = count < 64 ? count : 64; >> - >> - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); >> - if (err) >> - return err; >> + u32 fd; >> + int err = 0; >> mutex_lock(&pmcdev->lock); >> - if (val > map->ltr_ignore_max) { >> + if (fls(value) > map->ltr_ignore_max) { >> err = -EINVAL; >> goto out_unlock; >> } >> fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); >> - fd |= (1U << val); >> + fd |= value; >> pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); >> out_unlock: >> mutex_unlock(&pmcdev->lock); >> + >> + return err; >> +} >> + >> +static ssize_t pmc_core_ltr_ignore_write(struct file *file, >> + const char __user *userbuf, >> + size_t count, loff_t *ppos) >> +{ >> + u32 buf_size, val; >> + int err; >> + >> + buf_size = count < 64 ? count : 64; >> + >> + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); >> + if (err) >> + return err; >> + >> + err = pmc_core_write_ltr_ignore(1U << val); >> + >> return err == 0 ? count : err; >> } >> @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct dmi_system_id *id) >> return 0; >> } >> +static int quirk_ltr_ignore(u32 val) >> +{ >> + int err; >> + >> + err = pmc_core_write_ltr_ignore(val); >> + >> + return err; >> +} >> + >> static const struct dmi_system_id pmc_core_dmi_table[] = { >> { >> .callback = quirk_xtal_ignore, >> @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct platform_device *pdev) >> pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); >> dmi_check_system(pmc_core_dmi_table); >> + /* >> + * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when >> + * a cable is attached. Tell the PMC to ignore it. >> + */ >> + if (pmcdev->map == &tgl_reg_map) { > I would suggest: if (pmcdev->map >= &tgl_reg_map) Erm, no just no. tgl_reg_map is a global variable you can absolutely NOT rely on the ordering of global variables in memory like this. Moreover using ordered comparisons on pointers generally is a very bad idea, please don't. Regards, Hans
Hi David Overall, it looks like the right thing to do but i have a few comments. See below. On Fri, Mar 5, 2021 at 2:07 PM David E. Box <david.e.box@linux.intel.com> wrote: > > Due to a HW limitation, the Latency Tolerance Reporting (LTR) value > programmed in the Tiger Lake GBE controller is not large enough to allow > the platform to enter Package C10, which in turn prevents the platform from > achieving its low power target during suspend-to-idle. Ignore the GBE LTR > value on Tiger Lake. LTR ignore functionality is currently performed solely > by a debugfs write call. Split out the LTR code into its own function that > can be called by both the debugfs writer and by this work around. > I presume this must be the last resort to use such quirk and you've already considered a user space tuning program or fw patch is not an option on this generation of SOCs. > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> > Cc: intel-wired-lan@lists.osuosl.org > --- > drivers/platform/x86/intel_pmc_core.c | 55 ++++++++++++++++++++------- > 1 file changed, 42 insertions(+), 13 deletions(-) > > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c > index ee2f757515b0..ab31eb646a1a 100644 > --- a/drivers/platform/x86/intel_pmc_core.c > +++ b/drivers/platform/x86/intel_pmc_core.c > @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused) > } > DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); > > -static ssize_t pmc_core_ltr_ignore_write(struct file *file, > - const char __user *userbuf, > - size_t count, loff_t *ppos) > +static int pmc_core_write_ltr_ignore(u32 value) This sounds a bit confusing with pmc_core_ltr_ignore_write. > { > struct pmc_dev *pmcdev = &pmc; > const struct pmc_reg_map *map = pmcdev->map; > - u32 val, buf_size, fd; > - int err; > - > - buf_size = count < 64 ? count : 64; > - > - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > - if (err) > - return err; > + u32 fd; lets just call it value > + int err = 0; > > mutex_lock(&pmcdev->lock); > > - if (val > map->ltr_ignore_max) { > + if (fls(value) > map->ltr_ignore_max) { I am not sure why you're considering a bit position here. We rather use absolute value for this and we already preserve (OR) previously programmed LTR while changing to the new desired value. Current modification would allow users to supply even bigger values than the MAX IP ignore allowed. This can be useful when you want to ignore more than 1 IP at a time but that's not how we usually use it for debug. This is more for a user space debug script to deal with. https://01.org/blogs/rajneesh/2019/using-power-management-controller-drivers-debug-low-power-platform-states > err = -EINVAL; > goto out_unlock; > } > > fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); > - fd |= (1U << val); > + fd |= value; > pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); > > out_unlock: > mutex_unlock(&pmcdev->lock); > + > + return err; > +} > + > +static ssize_t pmc_core_ltr_ignore_write(struct file *file, > + const char __user *userbuf, > + size_t count, loff_t *ppos) > +{ > + u32 buf_size, val; > + int err; > + > + buf_size = count < 64 ? count : 64; > + > + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > + if (err) > + return err; > + > + err = pmc_core_write_ltr_ignore(1U << val); > + > return err == 0 ? count : err; > } > > @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct dmi_system_id *id) > return 0; > } > > +static int quirk_ltr_ignore(u32 val) > +{ > + int err; > + > + err = pmc_core_write_ltr_ignore(val); > + > + return err; > +} > + > static const struct dmi_system_id pmc_core_dmi_table[] = { > { > .callback = quirk_xtal_ignore, > @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct platform_device *pdev) > pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); > dmi_check_system(pmc_core_dmi_table); > > + /* > + * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when > + * a cable is attached. Tell the PMC to ignore it. > + */ > + if (pmcdev->map == &tgl_reg_map) { > + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); > + quirk_ltr_ignore(1U << 3); Can this be made a part of *_reg_map itself if intended to be used for more future platforms? Otherwise we just leave it as a one time quirk. > + } > + > pmc_core_dbgfs_register(pmcdev); > > device_initialized = true; > -- > 2.25.1 >
On Mon, Mar 8, 2021 at 9:04 AM Rajneesh Bhardwaj <irenic.rajneesh@gmail.com> wrote: > > Hi David > > Overall, it looks like the right thing to do but i have a few > comments. See below. > > On Fri, Mar 5, 2021 at 2:07 PM David E. Box <david.e.box@linux.intel.com> wrote: > > > > Due to a HW limitation, the Latency Tolerance Reporting (LTR) value > > programmed in the Tiger Lake GBE controller is not large enough to allow > > the platform to enter Package C10, which in turn prevents the platform from > > achieving its low power target during suspend-to-idle. Ignore the GBE LTR > > value on Tiger Lake. LTR ignore functionality is currently performed solely > > by a debugfs write call. Split out the LTR code into its own function that > > can be called by both the debugfs writer and by this work around. > > > > I presume this must be the last resort to use such quirk and you've > already considered a user space tuning program or fw patch is not an > option on this generation of SOCs. > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> > > Cc: intel-wired-lan@lists.osuosl.org > > --- > > drivers/platform/x86/intel_pmc_core.c | 55 ++++++++++++++++++++------- > > 1 file changed, 42 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c > > index ee2f757515b0..ab31eb646a1a 100644 > > --- a/drivers/platform/x86/intel_pmc_core.c > > +++ b/drivers/platform/x86/intel_pmc_core.c > > @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused) > > } > > DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); > > > > -static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > - const char __user *userbuf, > > - size_t count, loff_t *ppos) > > +static int pmc_core_write_ltr_ignore(u32 value) > > This sounds a bit confusing with pmc_core_ltr_ignore_write. > > > { > > struct pmc_dev *pmcdev = &pmc; > > const struct pmc_reg_map *map = pmcdev->map; > > - u32 val, buf_size, fd; > > - int err; > > - > > - buf_size = count < 64 ? count : 64; > > - > > - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > - if (err) > > - return err; > > + u32 fd; > > lets just call it value I meant a different name than fd is better. I see both value / val are already used here. > > > + int err = 0; > > > > mutex_lock(&pmcdev->lock); > > > > - if (val > map->ltr_ignore_max) { > > + if (fls(value) > map->ltr_ignore_max) { > > I am not sure why you're considering a bit position here. We rather > use absolute value for this and we already preserve (OR) previously > programmed LTR while changing to the new desired value. Current > modification would allow users to supply even bigger values than the > MAX IP ignore allowed. This can be useful when you want to ignore more > than 1 IP at a time but that's not how we usually use it for debug. > This is more for a user space debug script to deal with. > https://01.org/blogs/rajneesh/2019/using-power-management-controller-drivers-debug-low-power-platform-states > > > err = -EINVAL; > > goto out_unlock; > > } > > > > fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); > > - fd |= (1U << val); > > + fd |= value; > > pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); > > > > out_unlock: > > mutex_unlock(&pmcdev->lock); > > + > > + return err; > > +} > > + > > +static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > + const char __user *userbuf, > > + size_t count, loff_t *ppos) > > +{ > > + u32 buf_size, val; > > + int err; > > + > > + buf_size = count < 64 ? count : 64; > > + > > + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > + if (err) > > + return err; > > + > > + err = pmc_core_write_ltr_ignore(1U << val); > > + > > return err == 0 ? count : err; > > } > > > > @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct dmi_system_id *id) > > return 0; > > } > > > > +static int quirk_ltr_ignore(u32 val) > > +{ > > + int err; > > + > > + err = pmc_core_write_ltr_ignore(val); > > + > > + return err; > > +} > > + > > static const struct dmi_system_id pmc_core_dmi_table[] = { > > { > > .callback = quirk_xtal_ignore, > > @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct platform_device *pdev) > > pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); > > dmi_check_system(pmc_core_dmi_table); > > > > + /* > > + * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when > > + * a cable is attached. Tell the PMC to ignore it. > > + */ > > + if (pmcdev->map == &tgl_reg_map) { > > + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); > > + quirk_ltr_ignore(1U << 3); > > Can this be made a part of *_reg_map itself if intended to be used for > more future platforms? Otherwise we just leave it as a one time quirk. > > > + } > > + > > pmc_core_dbgfs_register(pmcdev); > > > > device_initialized = true; > > -- > > 2.25.1 > > > > > -- > Thanks, > Rajneesh
Hi Rajneesh, On Mon, 2021-03-08 at 09:04 -0500, Rajneesh Bhardwaj wrote: > Hi David > > Overall, it looks like the right thing to do but i have a few > comments. See below. > > On Fri, Mar 5, 2021 at 2:07 PM David E. Box < > david.e.box@linux.intel.com> wrote: > > > > Due to a HW limitation, the Latency Tolerance Reporting (LTR) value > > programmed in the Tiger Lake GBE controller is not large enough to > > allow > > the platform to enter Package C10, which in turn prevents the > > platform from > > achieving its low power target during suspend-to-idle. Ignore the > > GBE LTR > > value on Tiger Lake. LTR ignore functionality is currently > > performed solely > > by a debugfs write call. Split out the LTR code into its own > > function that > > can be called by both the debugfs writer and by this work around. > > > > I presume this must be the last resort to use such quirk and you've > already considered a user space tuning program or fw patch is not an > option on this generation of SOCs. This was the suggested work around by the LAN team. A FW solution may be considered for future products but is not in the works for TGL. > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> > > Cc: intel-wired-lan@lists.osuosl.org > > --- > > drivers/platform/x86/intel_pmc_core.c | 55 ++++++++++++++++++++--- > > ---- > > 1 file changed, 42 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/platform/x86/intel_pmc_core.c > > b/drivers/platform/x86/intel_pmc_core.c > > index ee2f757515b0..ab31eb646a1a 100644 > > --- a/drivers/platform/x86/intel_pmc_core.c > > +++ b/drivers/platform/x86/intel_pmc_core.c > > @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file > > *s, void *unused) > > } > > DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); > > > > -static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > - const char __user > > *userbuf, > > - size_t count, loff_t > > *ppos) > > +static int pmc_core_write_ltr_ignore(u32 value) > > This sounds a bit confusing with pmc_core_ltr_ignore_write. Ack > > > { > > struct pmc_dev *pmcdev = &pmc; > > const struct pmc_reg_map *map = pmcdev->map; > > - u32 val, buf_size, fd; > > - int err; > > - > > - buf_size = count < 64 ? count : 64; > > - > > - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > - if (err) > > - return err; > > + u32 fd; > > lets just call it value Yeah, I'll clean it up the names. It was just moved without changing it. > > > + int err = 0; > > > > mutex_lock(&pmcdev->lock); > > > > - if (val > map->ltr_ignore_max) { > > + if (fls(value) > map->ltr_ignore_max) { > > I am not sure why you're considering a bit position here. We rather > use absolute value for this and we already preserve (OR) previously > programmed LTR while changing to the new desired value. Current > modification would allow users to supply even bigger values than the > MAX IP ignore allowed. This can be useful when you want to ignore > more > than 1 IP at a time but that's not how we usually use it for debug. > This is more for a user space debug script to deal with. This was unintentionally added. The line should not have changed at all. Thanks for catching. > > https://01.org/blogs/rajneesh/2019/using-power-management-controller-drivers-debug-low-power-platform-states > > > err = -EINVAL; > > goto out_unlock; > > } > > > > fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); > > - fd |= (1U << val); > > + fd |= value; > > pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); > > > > out_unlock: > > mutex_unlock(&pmcdev->lock); > > + > > + return err; > > +} > > + > > +static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > + const char __user > > *userbuf, > > + size_t count, loff_t > > *ppos) > > +{ > > + u32 buf_size, val; > > + int err; > > + > > + buf_size = count < 64 ? count : 64; > > + > > + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > + if (err) > > + return err; > > + > > + err = pmc_core_write_ltr_ignore(1U << val); > > + > > return err == 0 ? count : err; > > } > > > > @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct > > dmi_system_id *id) > > return 0; > > } > > > > +static int quirk_ltr_ignore(u32 val) > > +{ > > + int err; > > + > > + err = pmc_core_write_ltr_ignore(val); > > + > > + return err; > > +} > > + > > static const struct dmi_system_id pmc_core_dmi_table[] = { > > { > > .callback = quirk_xtal_ignore, > > @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct > > platform_device *pdev) > > pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); > > dmi_check_system(pmc_core_dmi_table); > > > > + /* > > + * On TGL, due to a hardware limitation, the GBE LTR blocks > > PC10 when > > + * a cable is attached. Tell the PMC to ignore it. > > + */ > > + if (pmcdev->map == &tgl_reg_map) { > > + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); > > + quirk_ltr_ignore(1U << 3); > > Can this be made a part of *_reg_map itself if intended to be used > for > more future platforms? Otherwise we just leave it as a one time > quirk. The intent right now is not to use this for future platforms. We'll see if that can happen. David > > > + } > > + > > pmc_core_dbgfs_register(pmcdev); > > > > device_initialized = true; > > -- > > 2.25.1 > > > >
> > [EXTERNAL EMAIL] > > Due to a HW limitation, the Latency Tolerance Reporting (LTR) value > programmed in the Tiger Lake GBE controller is not large enough to allow > the platform to enter Package C10, which in turn prevents the platform from > achieving its low power target during suspend-to-idle. Ignore the GBE LTR > value on Tiger Lake. LTR ignore functionality is currently performed solely > by a debugfs write call. Split out the LTR code into its own function that > can be called by both the debugfs writer and by this work around. > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> > Cc: intel-wired-lan@lists.osuosl.org > --- > drivers/platform/x86/intel_pmc_core.c | 55 ++++++++++++++++++++------- > 1 file changed, 42 insertions(+), 13 deletions(-) I feel like this driver change causes a weak reference between e1000e and intel_pmc_core. It would mean significantly different behavior if you use e1000e but don't have PMC module available for any reason. In this case does it maybe make sense to at least use "imply" in the Kconfig for e1000e so that selecting e1000e gets intel-pmc-core enabled too? > > diff --git a/drivers/platform/x86/intel_pmc_core.c > b/drivers/platform/x86/intel_pmc_core.c > index ee2f757515b0..ab31eb646a1a 100644 > --- a/drivers/platform/x86/intel_pmc_core.c > +++ b/drivers/platform/x86/intel_pmc_core.c > @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file *s, void > *unused) > } > DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); > > -static ssize_t pmc_core_ltr_ignore_write(struct file *file, > - const char __user *userbuf, > - size_t count, loff_t *ppos) > +static int pmc_core_write_ltr_ignore(u32 value) > { > struct pmc_dev *pmcdev = &pmc; > const struct pmc_reg_map *map = pmcdev->map; > - u32 val, buf_size, fd; > - int err; > - > - buf_size = count < 64 ? count : 64; > - > - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > - if (err) > - return err; > + u32 fd; > + int err = 0; > > mutex_lock(&pmcdev->lock); > > - if (val > map->ltr_ignore_max) { > + if (fls(value) > map->ltr_ignore_max) { > err = -EINVAL; > goto out_unlock; > } > > fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); > - fd |= (1U << val); > + fd |= value; > pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); > > out_unlock: > mutex_unlock(&pmcdev->lock); > + > + return err; > +} > + > +static ssize_t pmc_core_ltr_ignore_write(struct file *file, > + const char __user *userbuf, > + size_t count, loff_t *ppos) > +{ > + u32 buf_size, val; > + int err; > + > + buf_size = count < 64 ? count : 64; > + > + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > + if (err) > + return err; > + > + err = pmc_core_write_ltr_ignore(1U << val); > + > return err == 0 ? count : err; > } > > @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct dmi_system_id > *id) > return 0; > } > > +static int quirk_ltr_ignore(u32 val) > +{ > + int err; > + > + err = pmc_core_write_ltr_ignore(val); > + > + return err; > +} > + > static const struct dmi_system_id pmc_core_dmi_table[] = { > { > .callback = quirk_xtal_ignore, > @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct platform_device *pdev) > pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); > dmi_check_system(pmc_core_dmi_table); > > + /* > + * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when > + * a cable is attached. Tell the PMC to ignore it. > + */ > + if (pmcdev->map == &tgl_reg_map) { > + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); > + quirk_ltr_ignore(1U << 3); > + } > + > pmc_core_dbgfs_register(pmcdev); > > device_initialized = true; > -- > 2.25.1
On Mon, Mar 8, 2021 at 12:20 PM Limonciello, Mario <Mario.Limonciello@dell.com> wrote: > > > > > [EXTERNAL EMAIL] > > > > Due to a HW limitation, the Latency Tolerance Reporting (LTR) value > > programmed in the Tiger Lake GBE controller is not large enough to allow > > the platform to enter Package C10, which in turn prevents the platform from > > achieving its low power target during suspend-to-idle. Ignore the GBE LTR > > value on Tiger Lake. LTR ignore functionality is currently performed solely > > by a debugfs write call. Split out the LTR code into its own function that > > can be called by both the debugfs writer and by this work around. > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> > > Cc: intel-wired-lan@lists.osuosl.org > > --- > > drivers/platform/x86/intel_pmc_core.c | 55 ++++++++++++++++++++------- > > 1 file changed, 42 insertions(+), 13 deletions(-) > > I feel like this driver change causes a weak reference between e1000e and intel_pmc_core. > It would mean significantly different behavior if you use e1000e but don't have PMC module > available for any reason. Can you elaborate what would change significantly? This is a FW/HW issue and the driver is just doing a work around to let the platform enter a deep idle state beyond PC10. If the system could enter PC10 and was denied entry by PMC only because of a bad LAN LTR, then that's purely an e1000e driver/GBE fw issue. > > In this case does it maybe make sense to at least use "imply" in the Kconfig for e1000e so > that selecting e1000e gets intel-pmc-core enabled too? This change would tell PMC to ignore GBE LTR, regardless of which GBE driver is selected. This doesn't bind e1000e. > > > > > diff --git a/drivers/platform/x86/intel_pmc_core.c > > b/drivers/platform/x86/intel_pmc_core.c > > index ee2f757515b0..ab31eb646a1a 100644 > > --- a/drivers/platform/x86/intel_pmc_core.c > > +++ b/drivers/platform/x86/intel_pmc_core.c > > @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file *s, void > > *unused) > > } > > DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); > > > > -static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > - const char __user *userbuf, > > - size_t count, loff_t *ppos) > > +static int pmc_core_write_ltr_ignore(u32 value) > > { > > struct pmc_dev *pmcdev = &pmc; > > const struct pmc_reg_map *map = pmcdev->map; > > - u32 val, buf_size, fd; > > - int err; > > - > > - buf_size = count < 64 ? count : 64; > > - > > - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > - if (err) > > - return err; > > + u32 fd; > > + int err = 0; > > > > mutex_lock(&pmcdev->lock); > > > > - if (val > map->ltr_ignore_max) { > > + if (fls(value) > map->ltr_ignore_max) { > > err = -EINVAL; > > goto out_unlock; > > } > > > > fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); > > - fd |= (1U << val); > > + fd |= value; > > pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); > > > > out_unlock: > > mutex_unlock(&pmcdev->lock); > > + > > + return err; > > +} > > + > > +static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > + const char __user *userbuf, > > + size_t count, loff_t *ppos) > > +{ > > + u32 buf_size, val; > > + int err; > > + > > + buf_size = count < 64 ? count : 64; > > + > > + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > + if (err) > > + return err; > > + > > + err = pmc_core_write_ltr_ignore(1U << val); > > + > > return err == 0 ? count : err; > > } > > > > @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct dmi_system_id > > *id) > > return 0; > > } > > > > +static int quirk_ltr_ignore(u32 val) > > +{ > > + int err; > > + > > + err = pmc_core_write_ltr_ignore(val); > > + > > + return err; > > +} > > + > > static const struct dmi_system_id pmc_core_dmi_table[] = { > > { > > .callback = quirk_xtal_ignore, > > @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct platform_device *pdev) > > pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); > > dmi_check_system(pmc_core_dmi_table); > > > > + /* > > + * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when > > + * a cable is attached. Tell the PMC to ignore it. > > + */ > > + if (pmcdev->map == &tgl_reg_map) { > > + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); > > + quirk_ltr_ignore(1U << 3); > > + } > > + > > pmc_core_dbgfs_register(pmcdev); > > > > device_initialized = true; > > -- > > 2.25.1 >
> -----Original Message----- > From: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com> > Sent: Monday, March 8, 2021 11:32 > To: Limonciello, Mario > Cc: David E. Box; hdegoede@redhat.com; mgross@linux.intel.com; > sasha.neftin@intel.com; platform-driver-x86@vger.kernel.org; linux- > kernel@vger.kernel.org; intel-wired-lan@lists.osuosl.org > Subject: Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake > platforms > > > [EXTERNAL EMAIL] > > On Mon, Mar 8, 2021 at 12:20 PM Limonciello, Mario > <Mario.Limonciello@dell.com> wrote: > > > > > > > > [EXTERNAL EMAIL] > > > > > > Due to a HW limitation, the Latency Tolerance Reporting (LTR) value > > > programmed in the Tiger Lake GBE controller is not large enough to allow > > > the platform to enter Package C10, which in turn prevents the platform > from > > > achieving its low power target during suspend-to-idle. Ignore the GBE LTR > > > value on Tiger Lake. LTR ignore functionality is currently performed > solely > > > by a debugfs write call. Split out the LTR code into its own function that > > > can be called by both the debugfs writer and by this work around. > > > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> > > > Cc: intel-wired-lan@lists.osuosl.org > > > --- > > > drivers/platform/x86/intel_pmc_core.c | 55 ++++++++++++++++++++------- > > > 1 file changed, 42 insertions(+), 13 deletions(-) > > > > I feel like this driver change causes a weak reference between e1000e and > intel_pmc_core. > > It would mean significantly different behavior if you use e1000e but don't > have PMC module > > available for any reason. > > Can you elaborate what would change significantly? This is a FW/HW > issue and the driver is just doing a work around to let the platform > enter a deep idle state beyond PC10. If the system could enter PC10 > and was denied entry by PMC only because of a bad LAN LTR, then that's > purely an e1000e driver/GBE fw issue. > Because the workaround is in pmc driver, the platform behavior becomes tied to whether this driver was enabled. Before this the driver was mostly for debugging purpose and really not necessary. Now it has a functional purpose. As such I think it should be made apparent that you need it now for some systems. > > > > In this case does it maybe make sense to at least use "imply" in the Kconfig > for e1000e so > > that selecting e1000e gets intel-pmc-core enabled too? > > This change would tell PMC to ignore GBE LTR, regardless of which GBE > driver is selected. This doesn't bind e1000e. Yeah, that's a good point. Maybe my suggestion can be to take this into documentation somewhere instead. > > > > > > > > > diff --git a/drivers/platform/x86/intel_pmc_core.c > > > b/drivers/platform/x86/intel_pmc_core.c > > > index ee2f757515b0..ab31eb646a1a 100644 > > > --- a/drivers/platform/x86/intel_pmc_core.c > > > +++ b/drivers/platform/x86/intel_pmc_core.c > > > @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file *s, > void > > > *unused) > > > } > > > DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); > > > > > > -static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > > - const char __user *userbuf, > > > - size_t count, loff_t *ppos) > > > +static int pmc_core_write_ltr_ignore(u32 value) > > > { > > > struct pmc_dev *pmcdev = &pmc; > > > const struct pmc_reg_map *map = pmcdev->map; > > > - u32 val, buf_size, fd; > > > - int err; > > > - > > > - buf_size = count < 64 ? count : 64; > > > - > > > - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > > - if (err) > > > - return err; > > > + u32 fd; > > > + int err = 0; > > > > > > mutex_lock(&pmcdev->lock); > > > > > > - if (val > map->ltr_ignore_max) { > > > + if (fls(value) > map->ltr_ignore_max) { > > > err = -EINVAL; > > > goto out_unlock; > > > } > > > > > > fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); > > > - fd |= (1U << val); > > > + fd |= value; > > > pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); > > > > > > out_unlock: > > > mutex_unlock(&pmcdev->lock); > > > + > > > + return err; > > > +} > > > + > > > +static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > > + const char __user *userbuf, > > > + size_t count, loff_t *ppos) > > > +{ > > > + u32 buf_size, val; > > > + int err; > > > + > > > + buf_size = count < 64 ? count : 64; > > > + > > > + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > > + if (err) > > > + return err; > > > + > > > + err = pmc_core_write_ltr_ignore(1U << val); > > > + > > > return err == 0 ? count : err; > > > } > > > > > > @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct > dmi_system_id > > > *id) > > > return 0; > > > } > > > > > > +static int quirk_ltr_ignore(u32 val) > > > +{ > > > + int err; > > > + > > > + err = pmc_core_write_ltr_ignore(val); > > > + > > > + return err; > > > +} > > > + > > > static const struct dmi_system_id pmc_core_dmi_table[] = { > > > { > > > .callback = quirk_xtal_ignore, > > > @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct platform_device > *pdev) > > > pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); > > > dmi_check_system(pmc_core_dmi_table); > > > > > > + /* > > > + * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 > when > > > + * a cable is attached. Tell the PMC to ignore it. > > > + */ > > > + if (pmcdev->map == &tgl_reg_map) { > > > + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); > > > + quirk_ltr_ignore(1U << 3); > > > + } > > > + > > > pmc_core_dbgfs_register(pmcdev); > > > > > > device_initialized = true; > > > -- > > > 2.25.1 > > > > > -- > Thanks, > Rajneesh
On Mon, 2021-03-08 at 18:02 +0000, Limonciello, Mario wrote: > > > > -----Original Message----- > > From: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com> > > Sent: Monday, March 8, 2021 11:32 > > To: Limonciello, Mario > > Cc: David E. Box; hdegoede@redhat.com; mgross@linux.intel.com; > > sasha.neftin@intel.com; platform-driver-x86@vger.kernel.org; linux- > > kernel@vger.kernel.org; intel-wired-lan@lists.osuosl.org > > Subject: Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on > > Tiger Lake > > platforms > > > > > > [EXTERNAL EMAIL] > > > > On Mon, Mar 8, 2021 at 12:20 PM Limonciello, Mario > > <Mario.Limonciello@dell.com> wrote: > > > > > > > > > > > [EXTERNAL EMAIL] > > > > > > > > Due to a HW limitation, the Latency Tolerance Reporting (LTR) > > > > value > > > > programmed in the Tiger Lake GBE controller is not large enough > > > > to allow > > > > the platform to enter Package C10, which in turn prevents the > > > > platform > > from > > > > achieving its low power target during suspend-to-idle. Ignore > > > > the GBE LTR > > > > value on Tiger Lake. LTR ignore functionality is currently > > > > performed > > solely > > > > by a debugfs write call. Split out the LTR code into its own > > > > function that > > > > can be called by both the debugfs writer and by this work > > > > around. > > > > > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > > > Reviewed-by: Sasha Neftin <sasha.neftin@intel.com> > > > > Cc: intel-wired-lan@lists.osuosl.org > > > > --- > > > > drivers/platform/x86/intel_pmc_core.c | 55 > > > > ++++++++++++++++++++------- > > > > 1 file changed, 42 insertions(+), 13 deletions(-) > > > > > > I feel like this driver change causes a weak reference between > > > e1000e and > > intel_pmc_core. > > > It would mean significantly different behavior if you use e1000e > > > but don't > > have PMC module > > > available for any reason. > > > > Can you elaborate what would change significantly? This is a FW/HW > > issue and the driver is just doing a work around to let the > > platform > > enter a deep idle state beyond PC10. If the system could enter PC10 > > and was denied entry by PMC only because of a bad LAN LTR, then > > that's > > purely an e1000e driver/GBE fw issue. > > > Because the workaround is in pmc driver, the platform behavior > becomes tied > to whether this driver was enabled. Before this the driver was > mostly for debugging > purpose and really not necessary. Now it has a functional purpose. > > As such I think it should be made apparent that you need it now for > some systems. Agreed. This is not the first fix either. The driver needs to be built for all platforms we add support for. I'll change the Kconfig. David > > > > > > > In this case does it maybe make sense to at least use "imply" in > > > the Kconfig > > for e1000e so > > > that selecting e1000e gets intel-pmc-core enabled too? > > > > This change would tell PMC to ignore GBE LTR, regardless of which > > GBE > > driver is selected. This doesn't bind e1000e. > > Yeah, that's a good point. > > Maybe my suggestion can be to take this into documentation somewhere > instead. > > > > > > > > > > > > > > diff --git a/drivers/platform/x86/intel_pmc_core.c > > > > b/drivers/platform/x86/intel_pmc_core.c > > > > index ee2f757515b0..ab31eb646a1a 100644 > > > > --- a/drivers/platform/x86/intel_pmc_core.c > > > > +++ b/drivers/platform/x86/intel_pmc_core.c > > > > @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct > > > > seq_file *s, > > void > > > > *unused) > > > > } > > > > DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); > > > > > > > > -static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > > > - const char __user > > > > *userbuf, > > > > - size_t count, loff_t > > > > *ppos) > > > > +static int pmc_core_write_ltr_ignore(u32 value) > > > > { > > > > struct pmc_dev *pmcdev = &pmc; > > > > const struct pmc_reg_map *map = pmcdev->map; > > > > - u32 val, buf_size, fd; > > > > - int err; > > > > - > > > > - buf_size = count < 64 ? count : 64; > > > > - > > > > - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > > > - if (err) > > > > - return err; > > > > + u32 fd; > > > > + int err = 0; > > > > > > > > mutex_lock(&pmcdev->lock); > > > > > > > > - if (val > map->ltr_ignore_max) { > > > > + if (fls(value) > map->ltr_ignore_max) { > > > > err = -EINVAL; > > > > goto out_unlock; > > > > } > > > > > > > > fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); > > > > - fd |= (1U << val); > > > > + fd |= value; > > > > pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); > > > > > > > > out_unlock: > > > > mutex_unlock(&pmcdev->lock); > > > > + > > > > + return err; > > > > +} > > > > + > > > > +static ssize_t pmc_core_ltr_ignore_write(struct file *file, > > > > + const char __user > > > > *userbuf, > > > > + size_t count, loff_t > > > > *ppos) > > > > +{ > > > > + u32 buf_size, val; > > > > + int err; > > > > + > > > > + buf_size = count < 64 ? count : 64; > > > > + > > > > + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); > > > > + if (err) > > > > + return err; > > > > + > > > > + err = pmc_core_write_ltr_ignore(1U << val); > > > > + > > > > return err == 0 ? count : err; > > > > } > > > > > > > > @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const > > > > struct > > dmi_system_id > > > > *id) > > > > return 0; > > > > } > > > > > > > > +static int quirk_ltr_ignore(u32 val) > > > > +{ > > > > + int err; > > > > + > > > > + err = pmc_core_write_ltr_ignore(val); > > > > + > > > > + return err; > > > > +} > > > > + > > > > static const struct dmi_system_id pmc_core_dmi_table[] = { > > > > { > > > > .callback = quirk_xtal_ignore, > > > > @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct > > > > platform_device > > *pdev) > > > > pmcdev->pmc_xram_read_bit = > > > > pmc_core_check_read_lock_bit(); > > > > dmi_check_system(pmc_core_dmi_table); > > > > > > > > + /* > > > > + * On TGL, due to a hardware limitation, the GBE LTR > > > > blocks PC10 > > when > > > > + * a cable is attached. Tell the PMC to ignore it. > > > > + */ > > > > + if (pmcdev->map == &tgl_reg_map) { > > > > + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); > > > > + quirk_ltr_ignore(1U << 3); > > > > + } > > > > + > > > > pmc_core_dbgfs_register(pmcdev); > > > > > > > > device_initialized = true; > > > > -- > > > > 2.25.1 > > > > > > > > > -- > > Thanks, > > Rajneesh
diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index ee2f757515b0..ab31eb646a1a 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -863,34 +863,45 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused) } DEFINE_SHOW_ATTRIBUTE(pmc_core_pll); -static ssize_t pmc_core_ltr_ignore_write(struct file *file, - const char __user *userbuf, - size_t count, loff_t *ppos) +static int pmc_core_write_ltr_ignore(u32 value) { struct pmc_dev *pmcdev = &pmc; const struct pmc_reg_map *map = pmcdev->map; - u32 val, buf_size, fd; - int err; - - buf_size = count < 64 ? count : 64; - - err = kstrtou32_from_user(userbuf, buf_size, 10, &val); - if (err) - return err; + u32 fd; + int err = 0; mutex_lock(&pmcdev->lock); - if (val > map->ltr_ignore_max) { + if (fls(value) > map->ltr_ignore_max) { err = -EINVAL; goto out_unlock; } fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset); - fd |= (1U << val); + fd |= value; pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd); out_unlock: mutex_unlock(&pmcdev->lock); + + return err; +} + +static ssize_t pmc_core_ltr_ignore_write(struct file *file, + const char __user *userbuf, + size_t count, loff_t *ppos) +{ + u32 buf_size, val; + int err; + + buf_size = count < 64 ? count : 64; + + err = kstrtou32_from_user(userbuf, buf_size, 10, &val); + if (err) + return err; + + err = pmc_core_write_ltr_ignore(1U << val); + return err == 0 ? count : err; } @@ -1189,6 +1200,15 @@ static int quirk_xtal_ignore(const struct dmi_system_id *id) return 0; } +static int quirk_ltr_ignore(u32 val) +{ + int err; + + err = pmc_core_write_ltr_ignore(val); + + return err; +} + static const struct dmi_system_id pmc_core_dmi_table[] = { { .callback = quirk_xtal_ignore, @@ -1244,6 +1264,15 @@ static int pmc_core_probe(struct platform_device *pdev) pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(); dmi_check_system(pmc_core_dmi_table); + /* + * On TGL, due to a hardware limitation, the GBE LTR blocks PC10 when + * a cable is attached. Tell the PMC to ignore it. + */ + if (pmcdev->map == &tgl_reg_map) { + dev_dbg(&pdev->dev, "ignoring GBE LTR\n"); + quirk_ltr_ignore(1U << 3); + } + pmc_core_dbgfs_register(pmcdev); device_initialized = true;