| Message ID | 20240419001013.28788-4-quic_schintav@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add 16GT/s equalization and margining settings | expand |
On Thu, Apr 18, 2024 at 05:09:36PM -0700, Shashank Babu Chinta Venkata wrote: PCI: qcom: Add RX lane margining settings for 16 GT/s data rate > Add rx lane margining settings for 16GT/s(GEN 4) data rate. These RX 16 GT/s > settings improve link stability while operating at high date rates > and helps to improve signal quality. > > Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-designware.h | 18 ++++++++++++++ > drivers/pci/controller/dwc/pcie-qcom-common.c | 24 +++++++++++++++++++ > drivers/pci/controller/dwc/pcie-qcom-common.h | 1 + > drivers/pci/controller/dwc/pcie-qcom-ep.c | 4 +++- > drivers/pci/controller/dwc/pcie-qcom.c | 4 +++- > 5 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index ad771bb52d29..e8c48855143f 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -203,6 +203,24 @@ > > #define PCIE_PL_CHK_REG_ERR_ADDR 0xB28 > > +/* > + * GEN4 lane margining register definitions 16 GT/s (GEN 4) > + */ > +#define GEN4_LANE_MARGINING_1_OFF 0xb80 > +#define MARGINING_MAX_VOLTAGE_OFFSET(n) FIELD_PREP(GENMASK(29, 24), n) > +#define MARGINING_NUM_VOLTAGE_STEPS(n) FIELD_PREP(GENMASK(22, 16), n) > +#define MARGINING_MAX_TIMING_OFFSET(n) FIELD_PREP(GENMASK(13, 8), n) > +#define MARGINING_NUM_TIMING_STEPS(n) FIELD_PREP(GENMASK(5, 0), n) > + > +#define GEN4_LANE_MARGINING_2_OFF 0xb84 > +#define MARGINING_IND_ERROR_SAMPLER(n) FIELD_PREP(BIT(28), n) > +#define MARGINING_SAMPLE_REPORTING_METHOD(n) FIELD_PREP(BIT(27), n) > +#define MARGINING_IND_LEFT_RIGHT_TIMING(n) FIELD_PREP(BIT(26), n) > +#define MARGINING_IND_UP_DOWN_VOLTAGE(n) FIELD_PREP(BIT(25), n) > +#define MARGINING_VOLTAGE_SUPPORTED(n) FIELD_PREP(BIT(24), n) > +#define MARGINING_MAXLANES(n) FIELD_PREP(GENMASK(20, 16), n) > +#define MARGINING_SAMPLE_RATE_TIMING(n) FIELD_PREP(GENMASK(13, 8), n) > +#define MARGINING_SAMPLE_RATE_VOLTAGE(n) FIELD_PREP(GENMASK(5, 0), n) > /* > * iATU Unroll-specific register definitions > * From 4.80 core version the address translation will be made by unroll > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c > index a6f3eb4c3ee6..3279314ae78c 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom-common.c > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c > @@ -46,6 +46,30 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci) > } > EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings); > > +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci) > +{ > + u32 reg; > + > + reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF); > + reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) | Same comment as previous patch. Are you doing it intentionally for some reason? - Mani
On 4/19/24 02:09, Shashank Babu Chinta Venkata wrote: > Add rx lane margining settings for 16GT/s(GEN 4) data rate. These > settings improve link stability while operating at high date rates > and helps to improve signal quality. > > Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> > --- > drivers/pci/controller/dwc/pcie-designware.h | 18 ++++++++++++++ > drivers/pci/controller/dwc/pcie-qcom-common.c | 24 +++++++++++++++++++ > drivers/pci/controller/dwc/pcie-qcom-common.h | 1 + > drivers/pci/controller/dwc/pcie-qcom-ep.c | 4 +++- > drivers/pci/controller/dwc/pcie-qcom.c | 4 +++- > 5 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index ad771bb52d29..e8c48855143f 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -203,6 +203,24 @@ > > #define PCIE_PL_CHK_REG_ERR_ADDR 0xB28 > > +/* > + * GEN4 lane margining register definitions > + */ > +#define GEN4_LANE_MARGINING_1_OFF 0xb80 > +#define MARGINING_MAX_VOLTAGE_OFFSET(n) FIELD_PREP(GENMASK(29, 24), n) > +#define MARGINING_NUM_VOLTAGE_STEPS(n) FIELD_PREP(GENMASK(22, 16), n) > +#define MARGINING_MAX_TIMING_OFFSET(n) FIELD_PREP(GENMASK(13, 8), n) > +#define MARGINING_NUM_TIMING_STEPS(n) FIELD_PREP(GENMASK(5, 0), n) > + > +#define GEN4_LANE_MARGINING_2_OFF 0xb84 > +#define MARGINING_IND_ERROR_SAMPLER(n) FIELD_PREP(BIT(28), n) > +#define MARGINING_SAMPLE_REPORTING_METHOD(n) FIELD_PREP(BIT(27), n) > +#define MARGINING_IND_LEFT_RIGHT_TIMING(n) FIELD_PREP(BIT(26), n) > +#define MARGINING_IND_UP_DOWN_VOLTAGE(n) FIELD_PREP(BIT(25), n) > +#define MARGINING_VOLTAGE_SUPPORTED(n) FIELD_PREP(BIT(24), n) > +#define MARGINING_MAXLANES(n) FIELD_PREP(GENMASK(20, 16), n) > +#define MARGINING_SAMPLE_RATE_TIMING(n) FIELD_PREP(GENMASK(13, 8), n) > +#define MARGINING_SAMPLE_RATE_VOLTAGE(n) FIELD_PREP(GENMASK(5, 0), n) That's a.. rather unusual.. use of FIELD_/GENMASK.. Usually, the fields are defined with GENMASK and then referenced through FIELD_xyz(BITFIELD_NAME, val) That said, I'm not entirely against this if Mani is ok with it > /* > * iATU Unroll-specific register definitions > * From 4.80 core version the address translation will be made by unroll > diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c > index a6f3eb4c3ee6..3279314ae78c 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom-common.c > +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c > @@ -46,6 +46,30 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci) > } > EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings); > > +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci) > +{ > + u32 reg; > + > + reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF); > + reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) | > + MARGINING_NUM_VOLTAGE_STEPS(0x78) | > + MARGINING_MAX_TIMING_OFFSET(0x32) | > + MARGINING_NUM_TIMING_STEPS(0x10); > + dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_1_OFF, reg); Since this is DW-common, why is this inside the qcom driver? Konrad
On 4/22/24 15:58, Konrad Dybcio wrote: > > > On 4/19/24 02:09, Shashank Babu Chinta Venkata wrote: >> Add rx lane margining settings for 16GT/s(GEN 4) data rate. These >> settings improve link stability while operating at high date rates >> and helps to improve signal quality. >> >> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> >> --- >> drivers/pci/controller/dwc/pcie-designware.h | 18 ++++++++++++++ >> drivers/pci/controller/dwc/pcie-qcom-common.c | 24 +++++++++++++++++++ >> drivers/pci/controller/dwc/pcie-qcom-common.h | 1 + >> drivers/pci/controller/dwc/pcie-qcom-ep.c | 4 +++- >> drivers/pci/controller/dwc/pcie-qcom.c | 4 +++- >> 5 files changed, 49 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index ad771bb52d29..e8c48855143f 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -203,6 +203,24 @@ >> #define PCIE_PL_CHK_REG_ERR_ADDR 0xB28 >> +/* >> + * GEN4 lane margining register definitions >> + */ >> +#define GEN4_LANE_MARGINING_1_OFF 0xb80 >> +#define MARGINING_MAX_VOLTAGE_OFFSET(n) FIELD_PREP(GENMASK(29, 24), n) >> +#define MARGINING_NUM_VOLTAGE_STEPS(n) FIELD_PREP(GENMASK(22, 16), n) >> +#define MARGINING_MAX_TIMING_OFFSET(n) FIELD_PREP(GENMASK(13, 8), n) >> +#define MARGINING_NUM_TIMING_STEPS(n) FIELD_PREP(GENMASK(5, 0), n) >> + >> +#define GEN4_LANE_MARGINING_2_OFF 0xb84 >> +#define MARGINING_IND_ERROR_SAMPLER(n) FIELD_PREP(BIT(28), n) >> +#define MARGINING_SAMPLE_REPORTING_METHOD(n) FIELD_PREP(BIT(27), n) >> +#define MARGINING_IND_LEFT_RIGHT_TIMING(n) FIELD_PREP(BIT(26), n) >> +#define MARGINING_IND_UP_DOWN_VOLTAGE(n) FIELD_PREP(BIT(25), n) >> +#define MARGINING_VOLTAGE_SUPPORTED(n) FIELD_PREP(BIT(24), n) >> +#define MARGINING_MAXLANES(n) FIELD_PREP(GENMASK(20, 16), n) >> +#define MARGINING_SAMPLE_RATE_TIMING(n) FIELD_PREP(GENMASK(13, 8), n) >> +#define MARGINING_SAMPLE_RATE_VOLTAGE(n) FIELD_PREP(GENMASK(5, 0), n) > > That's a.. rather unusual.. use of FIELD_/GENMASK.. Usually, the fields are > defined with GENMASK and then referenced through FIELD_xyz(BITFIELD_NAME, val) > > That said, I'm not entirely against this if Mani is ok with it will fall back to conventional approach in my next series to avoid confusion. > >> /* >> * iATU Unroll-specific register definitions >> * From 4.80 core version the address translation will be made by unroll >> diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c >> index a6f3eb4c3ee6..3279314ae78c 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom-common.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c >> @@ -46,6 +46,30 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci) >> } >> EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings); >> +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci) >> +{ >> + u32 reg; >> + >> + reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF); >> + reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) | >> + MARGINING_NUM_VOLTAGE_STEPS(0x78) | >> + MARGINING_MAX_TIMING_OFFSET(0x32) | >> + MARGINING_NUM_TIMING_STEPS(0x10); >> + dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_1_OFF, reg); > > Since this is DW-common, why is this inside the qcom driver? Though this register space is in dw-common specific, these settings are purely vendor specific . These settings are determined by systems team on vendor hardware, as these settings are used as margin to compensate signal variance due to various physical factors(like connection length, retimers etc). > > Konrad
[...] >>> EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings); >>> +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci) >>> +{ >>> + u32 reg; >>> + >>> + reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF); >>> + reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) | >>> + MARGINING_NUM_VOLTAGE_STEPS(0x78) | >>> + MARGINING_MAX_TIMING_OFFSET(0x32) | >>> + MARGINING_NUM_TIMING_STEPS(0x10); >>> + dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_1_OFF, reg); >> >> Since this is DW-common, why is this inside the qcom driver? > Though this register space is in dw-common specific, these settings are purely vendor specific . These settings are determined by systems team on vendor hardware, as these settings are used as margin to compensate signal variance due to various physical factors(like connection length, retimers etc). Okay, so: 1. is the register layout vendor-specific too? i.e. are the bitfields DW-common? 2. will these settings work on all Qualcomm devices, regardless of SoC/board/ retimers used etc.? Konrad
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index ad771bb52d29..e8c48855143f 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -203,6 +203,24 @@ #define PCIE_PL_CHK_REG_ERR_ADDR 0xB28 +/* + * GEN4 lane margining register definitions + */ +#define GEN4_LANE_MARGINING_1_OFF 0xb80 +#define MARGINING_MAX_VOLTAGE_OFFSET(n) FIELD_PREP(GENMASK(29, 24), n) +#define MARGINING_NUM_VOLTAGE_STEPS(n) FIELD_PREP(GENMASK(22, 16), n) +#define MARGINING_MAX_TIMING_OFFSET(n) FIELD_PREP(GENMASK(13, 8), n) +#define MARGINING_NUM_TIMING_STEPS(n) FIELD_PREP(GENMASK(5, 0), n) + +#define GEN4_LANE_MARGINING_2_OFF 0xb84 +#define MARGINING_IND_ERROR_SAMPLER(n) FIELD_PREP(BIT(28), n) +#define MARGINING_SAMPLE_REPORTING_METHOD(n) FIELD_PREP(BIT(27), n) +#define MARGINING_IND_LEFT_RIGHT_TIMING(n) FIELD_PREP(BIT(26), n) +#define MARGINING_IND_UP_DOWN_VOLTAGE(n) FIELD_PREP(BIT(25), n) +#define MARGINING_VOLTAGE_SUPPORTED(n) FIELD_PREP(BIT(24), n) +#define MARGINING_MAXLANES(n) FIELD_PREP(GENMASK(20, 16), n) +#define MARGINING_SAMPLE_RATE_TIMING(n) FIELD_PREP(GENMASK(13, 8), n) +#define MARGINING_SAMPLE_RATE_VOLTAGE(n) FIELD_PREP(GENMASK(5, 0), n) /* * iATU Unroll-specific register definitions * From 4.80 core version the address translation will be made by unroll diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.c b/drivers/pci/controller/dwc/pcie-qcom-common.c index a6f3eb4c3ee6..3279314ae78c 100644 --- a/drivers/pci/controller/dwc/pcie-qcom-common.c +++ b/drivers/pci/controller/dwc/pcie-qcom-common.c @@ -46,6 +46,30 @@ void qcom_pcie_common_set_16gt_eq_settings(struct dw_pcie *pci) } EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_eq_settings); +void qcom_pcie_common_set_16gt_rx_margining_settings(struct dw_pcie *pci) +{ + u32 reg; + + reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_1_OFF); + reg = MARGINING_MAX_VOLTAGE_OFFSET(0x24) | + MARGINING_NUM_VOLTAGE_STEPS(0x78) | + MARGINING_MAX_TIMING_OFFSET(0x32) | + MARGINING_NUM_TIMING_STEPS(0x10); + dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_1_OFF, reg); + + reg = dw_pcie_readl_dbi(pci, GEN4_LANE_MARGINING_2_OFF); + reg = MARGINING_IND_ERROR_SAMPLER(1) | + MARGINING_SAMPLE_REPORTING_METHOD(1) | + MARGINING_IND_LEFT_RIGHT_TIMING(1) | + MARGINING_VOLTAGE_SUPPORTED(1) | + MARGINING_IND_UP_DOWN_VOLTAGE(0) | + MARGINING_MAXLANES(pci->num_lanes) | + MARGINING_SAMPLE_RATE_TIMING(0x3f) | + MARGINING_SAMPLE_RATE_VOLTAGE(0x3f); + dw_pcie_writel_dbi(pci, GEN4_LANE_MARGINING_2_OFF, reg); +} +EXPORT_SYMBOL_GPL(qcom_pcie_common_set_16gt_rx_margining_settings); + int qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, struct icc_path **icc_mem_p) { *icc_mem_p = devm_of_icc_get(pci->dev, "pcie-mem"); diff --git a/drivers/pci/controller/dwc/pcie-qcom-common.h b/drivers/pci/controller/dwc/pcie-qcom-common.h index e72c651b0d28..b9eb78fcc766 100644 --- a/drivers/pci/controller/dwc/pcie-qcom-common.h +++ b/drivers/pci/controller/dwc/pcie-qcom-common.h @@ -11,3 +11,4 @@ int qcom_pcie_common_icc_get_resource(struct dw_pcie *pci, struct icc_path **icc 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); +void qcom_pcie_common_set_16gt_rx_margining_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 cb75a874f76c..3032dd91514c 100644 --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c @@ -438,8 +438,10 @@ 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) + if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT) { qcom_pcie_common_set_16gt_eq_settings(pci); + qcom_pcie_common_set_16gt_rx_margining_settings(pci); + } /* * The physical address of the MMIO region which is exposed as the BAR diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index acf66f974edc..f69364ecf2de 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -263,8 +263,10 @@ 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) + if (pcie_link_speed[pci->link_gen] == PCIE_SPEED_16_0GT) { qcom_pcie_common_set_16gt_eq_settings(pci); + qcom_pcie_common_set_16gt_rx_margining_settings(pci); + } /* Enable Link Training state machine */ if (pcie->cfg->ops->ltssm_enable)
Add rx lane margining settings for 16GT/s(GEN 4) data rate. These settings improve link stability while operating at high date rates and helps to improve signal quality. Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@quicinc.com> --- drivers/pci/controller/dwc/pcie-designware.h | 18 ++++++++++++++ drivers/pci/controller/dwc/pcie-qcom-common.c | 24 +++++++++++++++++++ drivers/pci/controller/dwc/pcie-qcom-common.h | 1 + drivers/pci/controller/dwc/pcie-qcom-ep.c | 4 +++- drivers/pci/controller/dwc/pcie-qcom.c | 4 +++- 5 files changed, 49 insertions(+), 2 deletions(-)