Received-SPF: pass (google.com: domain of linux-kernel+bounces-203683-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Thu, 6 Jun 2024 12:13:42 +0530
From: Manivannan Sadhasivam <mani@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>,
	jingoohan1@gmail.com, gustavo.pimentel@synopsys.com,
	manivannan.sadhasivam@linaro.org, andersson@kernel.org,
	agross@kernel.org, konrad.dybcio@linaro.org,
	quic_msarkar@quicinc.com, quic_kraravin@quicinc.com,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Serge Semin <fancer.lancer@gmail.com>,
	Conor Dooley <conor.dooley@microchip.com>,
	linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v4 2/3] PCI: qcom: Add equalization settings for 16 GT/s
Message-ID: <20240606064342.GE4441@thinkpad>
References: <20240501163610.8900-3-quic_schintav@quicinc.com>
 <20240530170208.GA550711@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240530170208.GA550711@bhelgaas>

On Thu, May 30, 2024 at 12:02:08PM -0500, Bjorn Helgaas wrote:
> On Wed, May 01, 2024 at 09:35:33AM -0700, Shashank Babu Chinta Venkata wrote:
> > During high data transmission rates such as 16 GT/s , there is an
> 
> s|GT/s ,|GT/s,|
> 
> > increased risk of signal loss due to poor channel quality and
> > interference. This can impact receiver's ability to capture signals
> > accurately. Hence, signal compensation is achieved through appropriate
> > lane equilization settings at both transmitter and receiver. This will
> 
> s/equilization/equalization/
> 
> How do you get these settings at both transmitter and receiver?  Or
> maybe you mean this patch sets the equalization settings in the qcom
> device, whether the device is a Root Port or an Endpoint?
> 
> I don't see this patch updating "dev" and "pci_upstream_bridge(dev)",
> so if you have a qcom Root Port leading to some non-qcom Endpoint,
> AFAICS only the Root Port would be updated.  If that's all that's
> necessary, that's perfectly fine.  It's just that the commit log
> suggests that we update both ends of a link, and the patch only
> appears to update one end (unless you have a qcom Root Port leading to
> a qcom Endpoint, and the Endpoint is operated by an embedded Linux
> running the qcom-ep driver, of course).
> 

That's a good question. The typical usecase on SA8775 (which is used for testing
this series) is connecting Qcom RC with Qcom EP. So with this series, both ends
would be updated.

But there are also non-Qcom EP devices going to be attached to the Qcom RC (like
NVMe) and on the EP side, Qcom EP can be attached to any x86 host.

So we should get clarified on the requirement for above.

- Mani

> > result in increasing PCIe signal strength.
> > 
> > Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware.h  | 12 ++++++
> >  drivers/pci/controller/dwc/pcie-qcom-common.c | 37 +++++++++++++++++++
> >  drivers/pci/controller/dwc/pcie-qcom-common.h |  1 +
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c     |  3 ++
> >  drivers/pci/controller/dwc/pcie-qcom.c        |  3 ++
> >  5 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 26dae4837462..ed0045043847 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -122,6 +122,18 @@
> >  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT	24
> >  #define GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK	GENMASK(25, 24)
> >  
> > +#define GEN3_EQ_CONTROL_OFF			0x8a8
> 
> s/0x8a8/0x8A8/ to follow existing style of file.
> 
> > +#define GEN3_EQ_CONTROL_OFF_FB_MODE		GENMASK(3, 0)
> > +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE	BIT(4)
> > +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC	GENMASK(23, 8)
> > +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL	BIT(24)
> > +
> > +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
> 
> s/0x8ac/0x8AC/ to follow existing style of file.
> 
> > +#define GEN3_EQ_FMDC_T_MIN_PHASE23		GENMASK(4, 0)
> > +#define GEN3_EQ_FMDC_N_EVALS			GENMASK(9, 5)
> > +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA	GENMASK(13, 10)
> > +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA	GENMASK(17, 14)
> > +
> >  #define PCIE_PORT_MULTI_LANE_CTRL	0x8C0
> >  #define PORT_MLTI_UPCFG_SUPPORT		BIT(7)
> >  
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c
> > index 228d9eec0222..16c277b2e9d4 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-common.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c
> > @@ -16,6 +16,43 @@
> >  #define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
> >  		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
> >  
> > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci)
> > +{
> > +	u32 reg;
> > +
> > +	/*
> > +	 * GEN3_RELATED_OFF register is repurposed to apply equilaztion
> 
> s/equilaztion/equalization/
> 
> > +	 * settings at various data transmission rates through registers
> > +	 * namely GEN3_EQ_*. RATE_SHADOW_SEL bit field of GEN3_RELATED_OFF
> > +	 * determines data rate for which this equilization settings are
> 
> s/this/these/
> s/equilization/equalization/
> 
> > +	 * applied.
> > +	 */
> > +	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> > +	reg &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
> > +	reg &= ~GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK;
> > +	reg |= FIELD_PREP(GEN3_RELATED_OFF_RATE_SHADOW_SEL_MASK, 0x1);
> > +	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, reg);
> > +
> > +	reg = dw_pcie_readl_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF);
> > +	reg &= ~(GEN3_EQ_FMDC_T_MIN_PHASE23 |
> > +		GEN3_EQ_FMDC_N_EVALS |
> > +		GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA |
> > +		GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA);
> > +	reg |= FIELD_PREP(GEN3_EQ_FMDC_T_MIN_PHASE23, 0x1) |
> > +		FIELD_PREP(GEN3_EQ_FMDC_N_EVALS, 0xd) |
> > +		FIELD_PREP(GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA, 0x5) |
> > +		FIELD_PREP(GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA, 0x5);
> > +	dw_pcie_writel_dbi(pci, GEN3_EQ_FB_MODE_DIR_CHANGE_OFF, reg);
> > +
> > +	reg = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
> > +	reg &= ~(GEN3_EQ_CONTROL_OFF_FB_MODE |
> > +		GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE |
> > +		GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL |
> > +		GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
> > +	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings);
> > +
> >  struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path)
> >  {
> >  	struct icc_path *icc_mem_p;
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h
> > index da1760c7e164..5c01f6c18b3b 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-common.h
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h
> > @@ -10,3 +10,4 @@
> >  struct icc_path *qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, const char *path);
> >  int qcom_pcie_common_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
> >  void qcom_pcie_common_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
> > +void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci);
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > index f0c61d847643..7940222d35f6 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -438,6 +438,9 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
> >  		goto err_disable_resources;
> >  	}
> >  
> > +	if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT)
> > +		qcom_pcie_common_set_16gt_eq_settings(pci);
> > +
> >  	/*
> >  	 * The physical address of the MMIO region which is exposed as the BAR
> >  	 * should be written to MHI BASE registers.
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 0095c42aeee0..525942f2cf98 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -263,6 +263,9 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
> >  {
> >  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> >  
> > +	if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT)
> > +		qcom_pcie_common_set_16gt_eq_settings(pci);
> > +
> >  	/* Enable Link Training state machine */
> >  	if (pcie->cfg->ops->ltssm_enable)
> >  		pcie->cfg->ops->ltssm_enable(pcie);
> > -- 
> > 2.43.2
> > 

-- 
மணிவண்ணன் சதாசிவம்