[PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[David E. Box]

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) {
+		dev_dbg(&pdev->dev, "ignoring GBE LTR\n");
+		quirk_ltr_ignore(1U << 3);
+	}
+
 	pmc_core_dbgfs_register(pmcdev);
 
 	device_initialized = true;
-- 
2.25.1

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[David E. Box]

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 at 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) {
+		dev_dbg(&pdev->dev, "ignoring GBE LTR\n");
+		quirk_ltr_ignore(1U << 3);
+	}
+
 	pmc_core_dbgfs_register(pmcdev);
 
 	device_initialized = true;
-- 
2.25.1

Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Neftin, Sasha]

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;
> 

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Neftin, Sasha]

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 at 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;
> 

Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[David E. Box]

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;
> > 
> 

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[David E. Box]

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 at 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;
> > 
> 

Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Hans de Goede]

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

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Hans de Goede]

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 at 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

Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Rajneesh Bhardwaj]

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
>


-- 
Thanks,
Rajneesh

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Rajneesh Bhardwaj]

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 at 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
>


-- 
Thanks,
Rajneesh

Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Rajneesh Bhardwaj]

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



-- 
Thanks,
Rajneesh

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Rajneesh Bhardwaj]

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 at 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



-- 
Thanks,
Rajneesh

Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[David E. Box]

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
> > 
> 
> 

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[David E. Box]

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 at 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 at 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
> > 
> 
> 

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Rajneesh Bhardwaj]

Sounds good. When you send your v2 feel free to add my RB.

Reviewed-by: Rajneesh Bhardwaj <irenic.raineesh@gmail.com>


On Mon, Mar 8, 2021 at 11:54 AM David E. Box <david.e.box@linux.intel.com>
wrote:

> 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 at 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 at 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
> > >
> >
> >
>
>
> --
Thanks,
Rajneesh
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20210308/cdb289ff/attachment-0001.html>

RE: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Limonciello, Mario]

> 
> [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

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Limonciello, Mario]

> 
> [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 at 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

Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Rajneesh Bhardwaj]

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
>


-- 
Thanks,
Rajneesh

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Rajneesh Bhardwaj]

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 at 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
>


-- 
Thanks,
Rajneesh

RE: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Limonciello, Mario]

> -----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

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[Limonciello, Mario]

> -----Original Message-----
> From: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>
> Sent: Monday, March 8, 2021 11:32
> To: Limonciello, Mario
> Cc: David E. Box; hdegoede at redhat.com; mgross at linux.intel.com;
> sasha.neftin at intel.com; platform-driver-x86 at vger.kernel.org; linux-
> kernel at vger.kernel.org; intel-wired-lan at 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 at 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

Re: [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[David E. Box]

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

[Intel-wired-lan] [PATCH] platform/x86: intel_pmc: Ignore GBE LTR on Tiger Lake platforms

[David E. Box]

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 at redhat.com; mgross at linux.intel.com;
> > sasha.neftin at intel.com; platform-driver-x86 at vger.kernel.org; linux-
> > kernel at vger.kernel.org; intel-wired-lan at 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 at 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