diff mbox series

[v1,2/3] PCI: dwc: add equalization settings for gen4

Message ID 20240301051220.20917-3-quic_schintav@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Add Gen4 equalization and margining settings | expand

Commit Message

Shashank Babu Chinta Venkata March 1, 2024, 5:11 a.m. UTC
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-qcom-cmn.c | 31 ++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-cmn.h | 23 ++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom-ep.c  |  4 +++
 drivers/pci/controller/dwc/pcie-qcom.c     |  4 +++
 4 files changed, 62 insertions(+)

Comments

Bjorn Helgaas March 1, 2024, 8 p.m. UTC | #1
On Thu, Feb 29, 2024 at 09:11:35PM -0800, 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.

Please capitalize subject lines to match history ("PCI: qcom: Add ...")

s/GEN3_RELATED_OFFSET/GEN3_RELATED_OFF/ (I think?)

I wish these "GEN3_RELATED" things were named with the data rate
instead of "GEN3".  The PCIe spec defines these things based on the
data rate (8GT/s, 16GT/s, etc), not the revision of the spec they
appeared in (gen3/gen4/etc).

Using "GEN3" means we have to first look up the "gen -> rate" mapping
before finding the relevant spec info.

Applies to the subject line, commit log, #defines, function names,
etc.

> +void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
> +{
> +	u32 reg;
> +
> +	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);

Warrants a one-line comment about using "GEN3_..." in a function named
"..._gen4_..."  (But ideally both would be renamed based on the data
rate instead.)

> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> @@ -9,10 +9,29 @@
>  #include "../../pci.h"
>  #include "pcie-designware.h"
>  
> +#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)

Are these qcom-specific registers, or should they be added alongside
GEN3_RELATED_OFF in pcie-designware.h?

> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_VAL   0x5
> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL  0x5
> +#define GEN3_EQ_FMDC_N_EVALS_VAL          0xD
> +#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

> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -438,6 +438,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>  		goto err_disable_resources;
>  	}
>  
> +	/* set Gen4 equalization settings */

Pointless comment.

> +	if (pci->link_gen == 4)
> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);

> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -263,6 +263,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>  
> +	/* set Gen4 equalization settings */

Pointless comment.

> +	if (pci->link_gen == 4)
> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);
Shashank Babu Chinta Venkata March 19, 2024, 11:07 a.m. UTC | #2
On 3/1/24 12:00, Bjorn Helgaas wrote:
> On Thu, Feb 29, 2024 at 09:11:35PM -0800, 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.
> 
> Please capitalize subject lines to match history ("PCI: qcom: Add ...")
> 
> s/GEN3_RELATED_OFFSET/GEN3_RELATED_OFF/ (I think?)
> 
> I wish these "GEN3_RELATED" things were named with the data rate
> instead of "GEN3".  The PCIe spec defines these things based on the
> data rate (8GT/s, 16GT/s, etc), not the revision of the spec they
> appeared in (gen3/gen4/etc).
I have kept it consistent with nomenclature followed in pcie designware
documentation for these registers. For function names and constraint to 
apply these, I will fall back to data rates rather than generation.

> Using "GEN3" means we have to first look up the "gen -> rate" mapping
> before finding the relevant spec info.
> 
> Applies to the subject line, commit log, #defines, function names,
> etc.
> 
>> +void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
>> +{
>> +	u32 reg;
>> +
>> +	reg = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> 
> Warrants a one-line comment about using "GEN3_..." in a function named
> "..._gen4_..."  (But ideally both would be renamed based on the data
> rate instead.)
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
>> @@ -9,10 +9,29 @@
>>   #include "../../pci.h"
>>   #include "pcie-designware.h"
>>   
>> +#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)
> 
> Are these qcom-specific registers, or should they be added alongside
> GEN3_RELATED_OFF in pcie-designware.h?
yes, these are designware register offsets. Will move them to designware 
header file. However, the settings are vendor specific.I will park
settings for these in QCOM specific files.
> 
>> +#define GEN3_EQ_FB_MODE_DIR_CHANGE_OFF          0x8ac
>> +#define GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_VAL   0x5
>> +#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL  0x5
>> +#define GEN3_EQ_FMDC_N_EVALS_VAL          0xD
>> +#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
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> @@ -438,6 +438,10 @@ static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
>>   		goto err_disable_resources;
>>   	}
>>   
>> +	/* set Gen4 equalization settings */
> 
> Pointless comment.
> 
>> +	if (pci->link_gen == 4)
>> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);
> 
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -263,6 +263,10 @@ static int qcom_pcie_start_link(struct dw_pcie *pci)
>>   {
>>   	struct qcom_pcie *pcie = to_qcom_pcie(pci);
>>   
>> +	/* set Gen4 equalization settings */
> 
> Pointless comment.
> 
>> +	if (pci->link_gen == 4)
>> +		qcom_pcie_cmn_set_gen4_eq_settings(pci);
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.c b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
index 0f8d004fbc79..cfdc04eef78c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-cmn.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
@@ -18,6 +18,37 @@ 
 #define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
 		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
 
+void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
+{
+	u32 reg;
+
+	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_VAL <<
+		GEN3_EQ_FMDC_N_EVALS_SHIFT);
+	reg &= ~GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_MASK;
+	reg |= (GEN3_EQ_FMDC_MAX_PRE_CUSROR_DELTA_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_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_gen4_eq_settings);
 
 int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
 {
diff --git a/drivers/pci/controller/dwc/pcie-qcom-cmn.h b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
index 8794dbd4775c..08e1bd179207 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-cmn.h
+++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
@@ -9,10 +9,29 @@ 
 #include "../../pci.h"
 #include "pcie-designware.h"
 
+#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_MAX_PRE_CUSROR_DELTA_VAL   0x5
+#define GEN3_EQ_FMDC_MAX_POST_CUSROR_DELTA_VAL  0x5
+#define GEN3_EQ_FMDC_N_EVALS_VAL          0xD
+#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
+
 #ifdef CONFIG_PCIE_QCOM_CMN
 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_gen4_eq_settings(struct dw_pcie *pci);
 #else
 static inline int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
 {
@@ -27,4 +46,8 @@  static inline int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *i
 static inline void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem)
 {
 }
+
+static inline void qcom_pcie_cmn_set_gen4_eq_settings(struct dw_pcie *pci)
+{
+}
 #endif
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index ce6343426de8..0b169bcd081d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -438,6 +438,10 @@  static int qcom_pcie_perst_deassert(struct dw_pcie *pci)
 		goto err_disable_resources;
 	}
 
+	/* set Gen4 equalization settings */
+	if (pci->link_gen == 4)
+		qcom_pcie_cmn_set_gen4_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..ad0cd55da777 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -263,6 +263,10 @@  static int qcom_pcie_start_link(struct dw_pcie *pci)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pci);
 
+	/* set Gen4 equalization settings */
+	if (pci->link_gen == 4)
+		qcom_pcie_cmn_set_gen4_eq_settings(pci);
+
 	/* Enable Link Training state machine */
 	if (pcie->cfg->ops->ltssm_enable)
 		pcie->cfg->ops->ltssm_enable(pcie);