diff mbox series

[v3,3/3] PCI: qcom: Add rx margining settings for 16GT/s

Message ID 20240419001013.28788-4-quic_schintav@quicinc.com
State New
Headers show
Series Add 16GT/s equalization and margining settings | expand

Commit Message

Shashank Babu Chinta Venkata April 19, 2024, 12:09 a.m. UTC
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(-)

Comments

Konrad Dybcio April 22, 2024, 10:58 p.m. UTC | #1
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
Shashank Babu Chinta Venkata April 30, 2024, 8:37 p.m. UTC | #2
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
Konrad Dybcio May 1, 2024, 11:13 p.m. UTC | #3
[...]

>>>   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 mbox series

Patch

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)