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 <[email protected]>
Reviewed-by: Sasha Neftin <[email protected]>
Cc: [email protected]
---
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) {
+ 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 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 <[email protected]>
> Reviewed-by: Sasha Neftin <[email protected]>
> Cc: [email protected]
> ---
> 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 <[email protected]>
> > Reviewed-by: Sasha Neftin <[email protected]>
> > Cc: [email protected]
> > ---
> > 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 <[email protected]>
>> Reviewed-by: Sasha Neftin <[email protected]>
>> Cc: [email protected]
>> ---
>> 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 <[email protected]> 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 <[email protected]>
> Reviewed-by: Sasha Neftin <[email protected]>
> Cc: [email protected]
> ---
> 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
>
--
Thanks,
Rajneesh
On Mon, Mar 8, 2021 at 9:04 AM Rajneesh Bhardwaj
<[email protected]> 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 <[email protected]> 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 <[email protected]>
> > Reviewed-by: Sasha Neftin <[email protected]>
> > Cc: [email protected]
> > ---
> > 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
--
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 <
> [email protected]> 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 <[email protected]>
> > Reviewed-by: Sasha Neftin <[email protected]>
> > Cc: [email protected]
> > ---
> > 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 <[email protected]>
> Reviewed-by: Sasha Neftin <[email protected]>
> Cc: [email protected]
> ---
> 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
<[email protected]> 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 <[email protected]>
> > Reviewed-by: Sasha Neftin <[email protected]>
> > Cc: [email protected]
> > ---
> > 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
>
--
Thanks,
Rajneesh
> -----Original Message-----
> From: Rajneesh Bhardwaj <[email protected]>
> Sent: Monday, March 8, 2021 11:32
> To: Limonciello, Mario
> Cc: David E. Box; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> 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
> <[email protected]> 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 <[email protected]>
> > > Reviewed-by: Sasha Neftin <[email protected]>
> > > Cc: [email protected]
> > > ---
> > > 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 <[email protected]>
> > Sent: Monday, March 8, 2021 11:32
> > To: Limonciello, Mario
> > Cc: David E. Box; [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected]
> > 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
> > <[email protected]> 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 <[email protected]>
> > > > Reviewed-by: Sasha Neftin <[email protected]>
> > > > Cc: [email protected]
> > > > ---
> > > > 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