Received-SPF: pass (google.com: domain of linux-kernel+bounces-89054-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 1 Mar 2024 14:00:41 -0600
From: Bjorn Helgaas <helgaas@kernel.org>
To: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com>
Cc: agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
	mani@kernel.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>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Serge Semin <fancer.lancer@gmail.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Josh Triplett <josh@joshtriplett.org>,
	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 v1 2/3] PCI: dwc: add equalization settings for gen4
Message-ID: <20240301200041.GA405674@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240301051220.20917-3-quic_schintav@quicinc.com>

On Thu, Feb 29, 2024 at 09:11:35PM -0800, Shashank Babu Chinta Venkata wrote:
> GEN3_RELATED_OFFSET is being used as shadow register for generation4 and
> generation5 data rates based on rate select mask settings on this register.
> Select relevant mask and equalization settings for generation4 operation.

Please capitalize subject lines to match history ("PCI: qcom: Add ...")

s/GEN3_RELATED_OFFSET/GEN3_RELATED_OFF/ (I think?)

I wish these "GEN3_RELATED" things were named with the data rate
instead of "GEN3".  The PCIe spec defines these things based on the
data rate (8GT/s, 16GT/s, etc), not the revision of the spec they
appeared in (gen3/gen4/etc).

Using "GEN3" means we have to first look up the "gen -> rate" mapping
before finding the relevant spec info.

Applies to the subject line, commit log, #defines, function names,
etc.

> +void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
> +{
> +	u32 reg;
> +
> +	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);

Warrants a one-line comment about using "GEN3_..." in a function named
"..._gen4_..."  (But ideally both would be renamed based on the data
rate instead.)

> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> @@ -9,10 +9,29 @@
>  #include "../../pci.h"
>  #include "pcie-designware.h"
>  
> +#define GEN3_EQ_CONTROL_OFF			0x8a8
> +#define GEN3_EQ_CONTROL_OFF_FB_MODE_MASK        GENMASK(3, 0)
> +#define GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE   BIT(4)
> +#define GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK	GENMASK(23, 8)
> +#define GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL	BIT(24)

Are these qcom-specific registers, or should they be added alongside
GEN3_RELATED_OFF in pcie-designware.h?

> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_VAL   0x5
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL  0x5
> +#define GEN3_EQ_FMDC_N_EVALS_VAL          0xD
> +#define GEN3_EQ_FMDC_T_MIN_PHASE23_MASK         GENMASK(4, 0)
> +#define GEN3_EQ_FMDC_N_EVALS_MASK               GENMASK(9, 5)
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK  GENMASK(13, 10)
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_MASK	GENMASK(17, 14)
> +#define GEN3_EQ_FMDC_N_EVALS_SHIFT			5
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT		10
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT	14

> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -438,6 +438,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  		goto err_disable_resources;
>  	}
>  
> +	/* set Gen4 equalization settings */

Pointless comment.

> +	if (pci->link_gen == 4)
> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);

> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -263,6 +263,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>  
> +	/* set Gen4 equalization settings */

Pointless comment.

> +	if (pci->link_gen == 4)
> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);