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

[Intel-wired-lan] [PATCH V2 2/2] 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
Reviewed-by: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>
---
Changes in V2:
	- Pass the bit position instead of the entire mask to the ltr_ignore
	  fuction
	- Rename the write_ltr_ignore to send_ltr_ignore to differentiate it
	  from the caller, ltr_ignore_write.
	- Rename variables for clarity

 drivers/platform/x86/intel_pmc_core.c | 59 ++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index ee2f757515b0..8d799cfa0e9f 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_send_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 reg;
+	int err = 0;
 
 	mutex_lock(&pmcdev->lock);
 
-	if (val > map->ltr_ignore_max) {
+	if (value > map->ltr_ignore_max) {
 		err = -EINVAL;
 		goto out_unlock;
 	}
 
-	fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset);
-	fd |= (1U << val);
-	pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd);
+	reg = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset);
+	reg |= (1U << value);
+	pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, reg);
 
 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, value;
+	int err;
+
+	buf_size = count < 64 ? count : 64;
+
+	err = kstrtou32_from_user(userbuf, buf_size, 10, &value);
+	if (err)
+		return err;
+
+	err = pmc_core_send_ltr_ignore(value);
+
 	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 value)
+{
+	int err;
+
+	err = pmc_core_send_ltr_ignore(value);
+
+	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(3);
+	}
+
 	pmc_core_dbgfs_register(pmcdev);
 
 	device_initialized = true;
-- 
2.25.1

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

[Jesse Brandeburg]

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.

Hi David, thanks for looking into this. A few review comments follow,
and then I'll Ack it.
 
> 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
> Reviewed-by: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>
> ---
> Changes in V2:
> 	- Pass the bit position instead of the entire mask to the ltr_ignore
> 	  fuction
> 	- Rename the write_ltr_ignore to send_ltr_ignore to differentiate it
> 	  from the caller, ltr_ignore_write.
> 	- Rename variables for clarity
> 
>  drivers/platform/x86/intel_pmc_core.c | 59 ++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index ee2f757515b0..8d799cfa0e9f 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_send_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 reg;
> +	int err = 0;
>  
>  	mutex_lock(&pmcdev->lock);
>  
> -	if (val > map->ltr_ignore_max) {
> +	if (value > map->ltr_ignore_max) {
>  		err = -EINVAL;
>  		goto out_unlock;
>  	}
>  
> -	fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset);
> -	fd |= (1U << val);
> -	pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd);
> +	reg = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset);
> +	reg |= (1U << value);

This should just be BIT(value).

> +	pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, reg);
>  
>  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, value;
> +	int err;
> +
> +	buf_size = count < 64 ? count : 64;

isn't this just an open coded buf_size = min_t(u32, count, 64); ?

> +
> +	err = kstrtou32_from_user(userbuf, buf_size, 10, &value);
> +	if (err)
> +		return err;

Why is there no range check or checking on &value?


> +
> +	err = pmc_core_send_ltr_ignore(value);
> +
>  	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 value)
> +{
> +	int err;
> +
> +	err = pmc_core_send_ltr_ignore(value);
> +
> +	return err;

return pmc_core_send_ltr_ignore(value); ?

> +}
> +
>  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(3);
> +	}
> +
>  	pmc_core_dbgfs_register(pmcdev);
>  
>  	device_initialized = true;

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

[David E. Box]

On Tue, 2021-03-16 at 15:10 -0700, Jesse Brandeburg wrote:
> 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.
> 
> Hi David, thanks for looking into this. A few review comments follow,
> and then I'll Ack it.

Hi Jesse. Thanks for reviewing.

> ?
> > 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
> > Reviewed-by: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>
> > ---
> > Changes in V2:
> > ????????- Pass the bit position instead of the entire mask to the
> > ltr_ignore
> > ????????? fuction
> > ????????- Rename the write_ltr_ignore to send_ltr_ignore to
> > differentiate it
> > ????????? from the caller, ltr_ignore_write.
> > ????????- Rename variables for clarity
> > 
> > ?drivers/platform/x86/intel_pmc_core.c | 59 ++++++++++++++++++++---
> > ----
> > ?1 file changed, 44 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > b/drivers/platform/x86/intel_pmc_core.c
> > index ee2f757515b0..8d799cfa0e9f 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_send_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 reg;
> > +???????int err = 0;
> > ?
> > ????????mutex_lock(&pmcdev->lock);
> > ?
> > -???????if (val > map->ltr_ignore_max) {
> > +???????if (value > map->ltr_ignore_max) {
> > ????????????????err = -EINVAL;
> > ????????????????goto out_unlock;
> > ????????}
> > ?
> > -???????fd = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset);
> > -???????fd |= (1U << val);
> > -???????pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, fd);
> > +???????reg = pmc_core_reg_read(pmcdev, map->ltr_ignore_offset);
> > +???????reg |= (1U << value);
> 
> This should just be BIT(value).

Ack

> 
> > +???????pmc_core_reg_write(pmcdev, map->ltr_ignore_offset, reg);
> > ?
> > ?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, value;
> > +???????int err;
> > +
> > +???????buf_size = count < 64 ? count : 64;
> 
> isn't this just an open coded buf_size = min_t(u32, count, 64); ?

Ack

> 
> > +
> > +???????err = kstrtou32_from_user(userbuf, buf_size, 10, &value);
> > +???????if (err)
> > +???????????????return err;
> 
> Why is there no range check or checking on &value?

This is range checked in pmc_core_send_ltr_ignore which has multiple
callers.

> 
> 
> > +
> > +???????err = pmc_core_send_ltr_ignore(value);
> > +
> > ????????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 value)
> > +{
> > +???????int err;
> > +
> > +???????err = pmc_core_send_ltr_ignore(value);
> > +
> > +???????return err;
> 
> return pmc_core_send_ltr_ignore(value); ?

Doh! We'll just call directly and get rid of this wrapper.

Thanks.

> 
> > +}
> > +
> > ?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(3);
> > +???????}
> > +
> > ????????pmc_core_dbgfs_register(pmcdev);
> > ?
> > ????????device_initialized = true;
> 
>