Received-SPF: pass (google.com: domain of linux-kernel+bounces-12188-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 27 Dec 2023 19:54:49 +0200 (EET)
From: =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To: "David E. Box" <david.e.box@linux.intel.com>
cc: Hans de Goede <hdegoede@redhat.com>, rajvi.jingar@linux.intel.com, 
    platform-driver-x86@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/8] platform/x86/intel/pmc: Allow reenabling LTRs
In-Reply-To: <20231223032548.1680738-5-david.e.box@linux.intel.com>
Message-ID: <e83492f-8c94-b43b-6fdc-aed6ea6251@linux.intel.com>
References: <20231223032548.1680738-1-david.e.box@linux.intel.com> <20231223032548.1680738-5-david.e.box@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Fri, 22 Dec 2023, David E. Box wrote:

> Commit 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and
> core_configure()") caused a network performance regression due to the GBE
> LTR ignore that it added during probe. The fix will move the ignore to
> occur at suspend-time (so as to not affect suspend power). This will
> require the ability to enable the LTR again on resume. Modify
> pmc_core_send_ltr_ignore() to allow enabling an LTR.
> 
> Fixes: 804951203aa5 ("platform/x86:intel/pmc: Combine core_init() and core_configure()")
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  drivers/platform/x86/intel/pmc/adl.c  | 2 +-
>  drivers/platform/x86/intel/pmc/cnp.c  | 2 +-
>  drivers/platform/x86/intel/pmc/core.c | 9 ++++++---
>  drivers/platform/x86/intel/pmc/core.h | 3 ++-
>  drivers/platform/x86/intel/pmc/mtl.c  | 2 +-
>  drivers/platform/x86/intel/pmc/tgl.c  | 2 +-
>  6 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/adl.c b/drivers/platform/x86/intel/pmc/adl.c
> index 882f2d5d8937..fbe0678f766c 100644
> --- a/drivers/platform/x86/intel/pmc/adl.c
> +++ b/drivers/platform/x86/intel/pmc/adl.c
> @@ -327,7 +327,7 @@ int adl_core_init(struct pmc_dev *pmcdev)
>  	 * when a cable is attached. Tell the PMC to ignore it.
>  	 */
>  	dev_dbg(&pmcdev->pdev->dev, "ignoring GBE LTR\n");
> -	pmc_core_send_ltr_ignore(pmcdev, 3);
> +	pmc_core_send_ltr_ignore(pmcdev, 3, 1);
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c
> index 59298f184d0e..80f73242f9dd 100644
> --- a/drivers/platform/x86/intel/pmc/cnp.c
> +++ b/drivers/platform/x86/intel/pmc/cnp.c
> @@ -220,7 +220,7 @@ int cnp_core_init(struct pmc_dev *pmcdev)
>  	 * when a cable is attached. Tell the PMC to ignore it.
>  	 */
>  	dev_dbg(&pmcdev->pdev->dev, "ignoring GBE LTR\n");
> -	pmc_core_send_ltr_ignore(pmcdev, 3);
> +	pmc_core_send_ltr_ignore(pmcdev, 3, 1);
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index 31905003bb05..aa44c6612af9 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -476,7 +476,7 @@ static int pmc_core_pll_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_pll);
>  
> -int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
> +int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore)

Nothing seems to need int for anything and it's used like a bool. So 

>  {
>  	struct pmc *pmc;
>  	const struct pmc_reg_map *map;
> @@ -514,7 +514,10 @@ int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value)
>  	mutex_lock(&pmcdev->lock);
>  
>  	reg = pmc_core_reg_read(pmc, map->ltr_ignore_offset);
> -	reg |= BIT(ltr_index);
> +	if (ignore)
> +		reg |= BIT(ltr_index);
> +	else
> +		reg &= ~BIT(ltr_index);
>  	pmc_core_reg_write(pmc, map->ltr_ignore_offset, reg);
>  
>  	mutex_unlock(&pmcdev->lock);
> @@ -537,7 +540,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file,
>  	if (err)
>  		return err;
>  
> -	err = pmc_core_send_ltr_ignore(pmcdev, value);
> +	err = pmc_core_send_ltr_ignore(pmcdev, value, 1);
>  
>  	return err == 0 ? count : err;
>  }
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index ce9b9efc9790..90f2dbc4df72 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -567,7 +567,8 @@ extern const struct pmc_reg_map arl_pchs_reg_map;
>  
>  extern void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev);
>  extern int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev);
> -extern int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value);
> +extern int
> +pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore);

extern is unnecessary.

>  int pmc_core_resume_common(struct pmc_dev *pmcdev);
>  int get_primary_reg_base(struct pmc *pmc);
> diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
> index e75431325dda..ef78d14fc27f 100644
> --- a/drivers/platform/x86/intel/pmc/mtl.c
> +++ b/drivers/platform/x86/intel/pmc/mtl.c
> @@ -1023,7 +1023,7 @@ int mtl_core_init(struct pmc_dev *pmcdev)
>  	 * when a cable is attached. Tell the PMC to ignore it.
>  	 */
>  	dev_dbg(&pmcdev->pdev->dev, "ignoring GBE LTR\n");
> -	pmc_core_send_ltr_ignore(pmcdev, 3);
> +	pmc_core_send_ltr_ignore(pmcdev, 3, 1);
>  
>  	if (ssram_init)
>  		return pmc_core_ssram_get_lpm_reqs(pmcdev);
> diff --git a/drivers/platform/x86/intel/pmc/tgl.c b/drivers/platform/x86/intel/pmc/tgl.c
> index 91fd725951e5..8213961975fc 100644
> --- a/drivers/platform/x86/intel/pmc/tgl.c
> +++ b/drivers/platform/x86/intel/pmc/tgl.c
> @@ -315,7 +315,7 @@ int tgl_core_generic_init(struct pmc_dev *pmcdev, int pch_tp)
>  	 * when a cable is attached. Tell the PMC to ignore it.
>  	 */
>  	dev_dbg(&pmcdev->pdev->dev, "ignoring GBE LTR\n");
> -	pmc_core_send_ltr_ignore(pmcdev, 3);
> +	pmc_core_send_ltr_ignore(pmcdev, 3, 1);
>  
>  	return 0;
>  }
> 

-- 
 i.