| Message ID | 20240320071527.13443-3-quic_schintav@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Manivannan Sadhasivam |
| Headers | show |
| Series | Add Gen4 equalization and margining settings | expand |
On 20.03.2024 08:14, 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. > > Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > --- [...] > + > +#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 The beauty of bitops.h is you no longer need to define these shifts.. Just use FIELD_GET/FIELD_PREP with the field! Please also drop _MASK from the leftover definitions. > +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. > + */ Given this comment and the commit message, should setting of this field be factored out to a function that would accept a generation argument? > + 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); > + 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); > +}
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@quicinc.com> > --- > 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 >
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 + #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); + 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 + 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); + /* * 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);
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. Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> --- 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(+)