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 > -- மணிவண்ணன் சதாசிவம்