Re: [PATCH v2 2/3] PCI: qcom: Add equalization settings for gen4

From: Manivannan Sadhasivam
Date: Tue Apr 02 2024 - 01:45:56 EST

On Wed, Mar 20, 2024 at 12:14:46AM -0700, Shashank Babu Chinta Venkata wrote:

Here, you are referring to 16 GT/s as the Gen4 datarate. So please mention that
explicitly.

> GEN3_RELATED_OFFSET is being used as shadow register for generation4 and

What is 'GEN3_RELATED_OFFSET' register? Where it is defined? More info on this
register would be helpful.

> generation5 data rates based on rate select mask settings on this register.

Please reword this sentence to make it more readable.

> Select relevant mask and equalization settings for generation4 operation.
>
> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@xxxxxxxxxxx>
> ---
> drivers/pci/controller/dwc/pcie-designware.h | 15 ++++++++
> drivers/pci/controller/dwc/pcie-qcom-cmn.c | 36 ++++++++++++++++++++
> drivers/pci/controller/dwc/pcie-qcom-cmn.h | 5 +++
> drivers/pci/controller/dwc/pcie-qcom-ep.c | 3 ++
> drivers/pci/controller/dwc/pcie-qcom.c | 3 ++
> 5 files changed, 62 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 26dae4837462..064744bfe35a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -122,6 +122,21 @@
> #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
> +#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)
> +
> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF 0x8ac
> +#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
> +

You are adding the definitions in designware header, but funtion definitions in
Qcom driver. Does this mean, this configuration is specific to Qcom and not
applicable to other DWC drivers?

> #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0
> #define PORT_MLTI_UPCFG_SUPPORT BIT(7)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.c b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> index 64fa412ec293..208a55e8e9a1 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> @@ -17,6 +17,42 @@
> #define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
> Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
>
> +void qcom_pcie_cmn_set_16gt_eq_settings(struct dw_pcie *pci)
> +{
> + u32 reg;
> +
> + /*
> + * GEN3_RELATED_OFF is repurposed to be used with GEN4(16GT/s) rate
> + * as well based on RATE_SHADOW_SEL_MASK settings on this register.
> + */
> + 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 |= (0x1 << GEN3_RELATED_OFF_RATE_SHADOW_SEL_SHIFT);

Please use FIELD_* macros where applicable.

> + 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_MASK;
> + reg &= ~GEN3_EQ_FMDC_N_EVALS_MASK;
> + reg |= (GEN3_EQ_FMDC_N_EVALS_16GT_VAL <<
> + GEN3_EQ_FMDC_N_EVALS_SHIFT);
> + reg &= ~GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK;
> + reg |= (GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_16GT_VAL <<
> + GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_SHIFT);
> + reg &= ~GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_MASK;
> + reg |= (GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_16GT_VAL <<
> + GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_SHIFT);
> + 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_MASK;
> + reg &= ~GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE;
> + reg &= ~GEN3_EQ_CONTROL_OFF_FOM_INC_INITIAL_EVAL;
> + reg &= ~GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC_MASK;
> + dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, reg);
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_cmn_set_16gt_eq_settings);
> +
> int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
> {
> if (IS_ERR(pci))
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.h b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> index 845eda23ae59..97302e8fafa8 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> @@ -9,6 +9,11 @@
> #include "../../pci.h"
> #include "pcie-designware.h"
>
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_16GT_VAL 0x5
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_16GT_VAL 0x5
> +#define GEN3_EQ_FMDC_N_EVALS_16GT_VAL 0xD
> +

So these settings are Qcom specific? I'd expect this to be documented in commit
message.

> int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem);
> int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
> void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);
> +void qcom_pcie_cmn_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 ce6343426de8..b6bcab21bb9f 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_cmn_set_16gt_eq_settings(pci);

So this relies on the optional 'max-link-speed' DT property, but not mentioned
in the commit message :/

- Mani

> +
> /*
> * 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 57a08294c561..b0a22a000fa3 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_cmn_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
>

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