[PATCH v8 0/4] PCI: dwc: Add support for configuring lane equalization presets

[PATCH v8 0/4] PCI: dwc: Add support for configuring lane equalization presets

[Krishna Chaitanya Chundru]

PCIe equalization presets are predefined settings used to optimize
signal integrity by compensating for signal loss and distortion in
high-speed data transmission.

As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
configure lane equalization presets for each lane to enhance the PCIe
link reliability. Each preset value represents a different combination
of pre-shoot and de-emphasis values. For each data rate, different
registers are defined: for 8.0 GT/s, registers are defined in section
7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
an extra receiver preset hint, requiring 16 bits per lane, while the
remaining data rates use 8 bits per lane.

Based on the number of lanes and the supported data rate, read the
device tree property and stores in the presets structure.

Based upon the lane width and supported data rate update lane
equalization registers.

This patch depends on the this dt binding pull request which got recently
merged: https://github.com/devicetree-org/dt-schema/pull/146

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
Changes in v8:
- Couple of nits by (bjorn & mani)
- Add EQ_PRESET_8GTS by (mani).
- Remove the logic not to update the DWC registers if the num_lanes is
  not equal to maximum lanes (mani)
- Link to v7: https://lore.kernel.org/r/20250225-preset_v6-v7-0-a593f3ef3951@oss.qualcomm.com

Changes in v7:
- Update the 16bit array in the array (mani & konrad)
- Update the couple of nits (comments, error log format etc) (mani)
- remove !num_lanes check as this is not needed with this series (mani)
- Add warning prints if the data rate is not supported and if there is
  no devicetree property for the data rate (mani).
- Link to v6: https://lore.kernel.org/r/20250210-preset_v6-v6-0-cbd837d0028d@oss.qualcomm.com

Changes in v6:
- update the dt properties to match the lane width ( mani & konard)
- move everything to helper function and let the helper function
  determine reg size and offset (mani)
- update the function header (mani)
- move the num_lanes check to the main function (mani)
- Link to v5: https://lore.kernel.org/linux-kernel/20250128-preset_v2-v5-0-4d230d956f8c@oss.qualcomm.com/

Changes in v5:
- Instead of using of_property_present use return value of
  of_property_read_u8_array to know about property is present or not and
  add a macro for reserved value(Konrad).
- Link to v4: https://lore.kernel.org/r/20250124-preset_v2-v4-0-0b512cad08e1@oss.qualcomm.com

Changes in v4:
- use static arrays for storing preset values and use default value 0xff
  to indicate the property is not present (Dimitry & konrad).
- Link to v3: https://lore.kernel.org/r/20241223-preset_v2-v3-0-a339f475caf5@oss.qualcomm.com

Changes in v3:
- In previous series a wrong patch was attached, correct it
- Link to v2: https://lore.kernel.org/r/20241212-preset_v2-v2-0-210430fbcd8a@oss.qualcomm.com

Changes in v2:
- Fix the kernel test robot error
- As suggested by konrad use for loop and read "eq-presets-%ugts", (8 << i)
- Link to v1: https://lore.kernel.org/r/20241116-presets-v1-0-878a837a4fee@quicinc.com

---
Krishna Chaitanya Chundru (4):
      arm64: dts: qcom: x1e80100: Add PCIe lane equalization preset properties
      PCI: of: Add of_pci_get_equalization_presets() API
      PCI: dwc: Update pci->num_lanes to maximum supported link width
      PCI: dwc: Add support for configuring lane equalization presets

 arch/arm64/boot/dts/qcom/x1e80100.dtsi            | 11 ++++
 drivers/pci/controller/dwc/pcie-designware-host.c | 63 +++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.c      |  8 +++
 drivers/pci/controller/dwc/pcie-designware.h      |  4 ++
 drivers/pci/of.c                                  | 44 ++++++++++++++++
 drivers/pci/pci.h                                 | 32 +++++++++++-
 include/uapi/linux/pci_regs.h                     |  3 ++
 7 files changed, 164 insertions(+), 1 deletion(-)
---
base-commit: 3175967ecb3266d0ad7d2ca7ccceaf15fa2f15e2
change-id: 20250210-preset_v6-1e7f560d13ad

Best regards,
-- 
Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

[PATCH v8 1/4] arm64: dts: qcom: x1e80100: Add PCIe lane equalization preset properties

[Krishna Chaitanya Chundru]

Add PCIe lane equalization preset properties for 8 GT/s and 16 GT/s data
rates used in lane equalization procedure.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
This patch depends on the this dt binding pull request which got recently
merged: https://github.com/devicetree-org/dt-schema/pull/146
---
---
 arch/arm64/boot/dts/qcom/x1e80100.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index 4936fa5b98ff..9f14dd13d02e 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -3209,6 +3209,10 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			phys = <&pcie3_phy>;
 			phy-names = "pciephy";
 
+			eq-presets-8gts = /bits/ 16 <0x5555 0x5555 0x5555 0x5555
+						     0x5555 0x5555 0x5555 0x5555>;
+			eq-presets-16gts = /bits/ 8 <0x55 0x55 0x55 0x55 0x55 0x55 0x55 0x55>;
+
 			operating-points-v2 = <&pcie3_opp_table>;
 
 			status = "disabled";
@@ -3411,6 +3415,9 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			phys = <&pcie6a_phy>;
 			phy-names = "pciephy";
 
+			eq-presets-8gts = /bits/ 16 <0x5555 0x5555 0x5555 0x5555>;
+			eq-presets-16gts = /bits/ 8 <0x55 0x55 0x55 0x55>;
+
 			status = "disabled";
 		};
 
@@ -3538,6 +3545,8 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			phys = <&pcie5_phy>;
 			phy-names = "pciephy";
 
+			eq-presets-8gts = /bits/ 16 <0x5555 0x5555>;
+
 			status = "disabled";
 		};
 
@@ -3662,6 +3671,8 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			phys = <&pcie4_phy>;
 			phy-names = "pciephy";
 
+			eq-presets-8gts = /bits/ 16 <0x5555 0x5555>;
+
 			status = "disabled";
 
 			pcie4_port0: pcie@0 {

-- 
2.34.1

[PATCH v8 2/4] PCI: of: Add of_pci_get_equalization_presets() API

[Krishna Chaitanya Chundru]

PCIe equalization presets are predefined settings used to optimize
signal integrity by compensating for signal loss and distortion in
high-speed data transmission.

As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
configure lane equalization presets for each lane to enhance the PCIe
link reliability. Each preset value represents a different combination
of pre-shoot and de-emphasis values. For each data rate, different
registers are defined: for 8.0 GT/s, registers are defined in section
7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
an extra receiver preset hint, requiring 16 bits per lane, while the
remaining data rates use 8 bits per lane.

Based on the number of lanes and the supported data rate,
of_pci_get_equalization_presets() reads the device tree property and
stores in the presets structure.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/of.c  | 44 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h | 32 +++++++++++++++++++++++++++++++-
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 7a806f5c0d20..18691483e108 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -851,3 +851,47 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
 	return slot_power_limit_mw;
 }
 EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
+
+/**
+ * of_pci_get_equalization_presets - Parses the "eq-presets-Ngts" property.
+ *
+ * @dev: Device containing the properties.
+ * @presets: Pointer to store the parsed data.
+ * @num_lanes: Maximum number of lanes supported.
+ *
+ * If the property is present, read and store the data in the @presets structure.
+ * Else, assign a default value of PCI_EQ_RESV.
+ *
+ * Return: 0 if the property is not available or successfully parsed else
+ * errno otherwise.
+ */
+int of_pci_get_equalization_presets(struct device *dev,
+				    struct pci_eq_presets *presets,
+				    int num_lanes)
+{
+	char name[20];
+	int ret;
+
+	presets->eq_presets_8gts[0] = PCI_EQ_RESV;
+	ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
+					 presets->eq_presets_8gts, num_lanes);
+	if (ret && ret != -EINVAL) {
+		dev_err(dev, "Error reading eq-presets-8gts :%d\n", ret);
+		return ret;
+	}
+
+	for (int i = 0; i < EQ_PRESET_TYPE_MAX - 1; i++) {
+		presets->eq_presets_Ngts[i][0] = PCI_EQ_RESV;
+		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << (i + 1));
+		ret = of_property_read_u8_array(dev->of_node, name,
+						presets->eq_presets_Ngts[i],
+						num_lanes);
+		if (ret && ret != -EINVAL) {
+			dev_err(dev, "Error reading %s :%d\n", name, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 01e51db8d285..78c9cc0ad8fa 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -9,6 +9,8 @@ struct pcie_tlp_log;
 /* Number of possible devfns: 0.0 to 1f.7 inclusive */
 #define MAX_NR_DEVFNS 256
 
+#define MAX_NR_LANES 16
+
 #define PCI_FIND_CAP_TTL	48
 
 #define PCI_VSEC_ID_INTEL_TBT	0x1234	/* Thunderbolt */
@@ -808,6 +810,21 @@ static inline u64 pci_rebar_size_to_bytes(int size)
 
 struct device_node;
 
+#define PCI_EQ_RESV	0xff
+
+enum equalization_preset_type {
+	EQ_PRESET_TYPE_8GTS,
+	EQ_PRESET_TYPE_16GTS,
+	EQ_PRESET_TYPE_32GTS,
+	EQ_PRESET_TYPE_64GTS,
+	EQ_PRESET_TYPE_MAX
+};
+
+struct pci_eq_presets {
+	u16 eq_presets_8gts[MAX_NR_LANES];
+	u8 eq_presets_Ngts[EQ_PRESET_TYPE_MAX - 1][MAX_NR_LANES];
+};
+
 #ifdef CONFIG_OF
 int of_get_pci_domain_nr(struct device_node *node);
 int of_pci_get_max_link_speed(struct device_node *node);
@@ -822,7 +839,9 @@ void pci_release_bus_of_node(struct pci_bus *bus);
 
 int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
 bool of_pci_supply_present(struct device_node *np);
-
+int of_pci_get_equalization_presets(struct device *dev,
+				    struct pci_eq_presets *presets,
+				    int num_lanes);
 #else
 static inline int
 of_get_pci_domain_nr(struct device_node *node)
@@ -867,6 +886,17 @@ static inline bool of_pci_supply_present(struct device_node *np)
 {
 	return false;
 }
+
+static inline int of_pci_get_equalization_presets(struct device *dev,
+						  struct pci_eq_presets *presets,
+						  int num_lanes)
+{
+	presets->eq_presets_8gts[0] = PCI_EQ_RESV;
+	for (int i = 0; i < EQ_PRESET_TYPE_MAX - 1; i++)
+		presets->eq_presets_Ngts[i][0] = PCI_EQ_RESV;
+
+	return 0;
+}
 #endif /* CONFIG_OF */
 
 struct of_changeset;

-- 
2.34.1

Re: [PATCH v8 2/4] PCI: of: Add of_pci_get_equalization_presets() API

[Manivannan Sadhasivam]

On Sun, Mar 16, 2025 at 09:39:02AM +0530, Krishna Chaitanya Chundru wrote:
> PCIe equalization presets are predefined settings used to optimize
> signal integrity by compensating for signal loss and distortion in
> high-speed data transmission.
> 
> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
> configure lane equalization presets for each lane to enhance the PCIe
> link reliability. Each preset value represents a different combination
> of pre-shoot and de-emphasis values. For each data rate, different
> registers are defined: for 8.0 GT/s, registers are defined in section
> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
> an extra receiver preset hint, requiring 16 bits per lane, while the
> remaining data rates use 8 bits per lane.
> 
> Based on the number of lanes and the supported data rate,
> of_pci_get_equalization_presets() reads the device tree property and
> stores in the presets structure.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/of.c  | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h | 32 +++++++++++++++++++++++++++++++-
>  2 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 7a806f5c0d20..18691483e108 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -851,3 +851,47 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>  	return slot_power_limit_mw;
>  }
>  EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> +
> +/**
> + * of_pci_get_equalization_presets - Parses the "eq-presets-Ngts" property.
> + *
> + * @dev: Device containing the properties.
> + * @presets: Pointer to store the parsed data.
> + * @num_lanes: Maximum number of lanes supported.
> + *
> + * If the property is present, read and store the data in the @presets structure.
> + * Else, assign a default value of PCI_EQ_RESV.
> + *
> + * Return: 0 if the property is not available or successfully parsed else
> + * errno otherwise.
> + */
> +int of_pci_get_equalization_presets(struct device *dev,
> +				    struct pci_eq_presets *presets,
> +				    int num_lanes)
> +{
> +	char name[20];
> +	int ret;
> +
> +	presets->eq_presets_8gts[0] = PCI_EQ_RESV;
> +	ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> +					 presets->eq_presets_8gts, num_lanes);
> +	if (ret && ret != -EINVAL) {
> +		dev_err(dev, "Error reading eq-presets-8gts :%d\n", ret);

'Error reading eq-presets-8gts: %d'

> +		return ret;
> +	}
> +
> +	for (int i = 0; i < EQ_PRESET_TYPE_MAX - 1; i++) {
> +		presets->eq_presets_Ngts[i][0] = PCI_EQ_RESV;
> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << (i + 1));
> +		ret = of_property_read_u8_array(dev->of_node, name,
> +						presets->eq_presets_Ngts[i],
> +						num_lanes);
> +		if (ret && ret != -EINVAL) {
> +			dev_err(dev, "Error reading %s :%d\n", name, ret);

'Error reading %s: %d'

> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 01e51db8d285..78c9cc0ad8fa 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -9,6 +9,8 @@ struct pcie_tlp_log;
>  /* Number of possible devfns: 0.0 to 1f.7 inclusive */
>  #define MAX_NR_DEVFNS 256
>  
> +#define MAX_NR_LANES 16

Why did you limit to 16?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 2/4] PCI: of: Add of_pci_get_equalization_presets() API

[Krishna Chaitanya Chundru]

On 3/28/2025 10:09 AM, Manivannan Sadhasivam wrote:
> On Sun, Mar 16, 2025 at 09:39:02AM +0530, Krishna Chaitanya Chundru wrote:
>> PCIe equalization presets are predefined settings used to optimize
>> signal integrity by compensating for signal loss and distortion in
>> high-speed data transmission.
>>
>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
>> configure lane equalization presets for each lane to enhance the PCIe
>> link reliability. Each preset value represents a different combination
>> of pre-shoot and de-emphasis values. For each data rate, different
>> registers are defined: for 8.0 GT/s, registers are defined in section
>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
>> an extra receiver preset hint, requiring 16 bits per lane, while the
>> remaining data rates use 8 bits per lane.
>>
>> Based on the number of lanes and the supported data rate,
>> of_pci_get_equalization_presets() reads the device tree property and
>> stores in the presets structure.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>   drivers/pci/of.c  | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci.h | 32 +++++++++++++++++++++++++++++++-
>>   2 files changed, 75 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index 7a806f5c0d20..18691483e108 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -851,3 +851,47 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>   	return slot_power_limit_mw;
>>   }
>>   EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>> +
>> +/**
>> + * of_pci_get_equalization_presets - Parses the "eq-presets-Ngts" property.
>> + *
>> + * @dev: Device containing the properties.
>> + * @presets: Pointer to store the parsed data.
>> + * @num_lanes: Maximum number of lanes supported.
>> + *
>> + * If the property is present, read and store the data in the @presets structure.
>> + * Else, assign a default value of PCI_EQ_RESV.
>> + *
>> + * Return: 0 if the property is not available or successfully parsed else
>> + * errno otherwise.
>> + */
>> +int of_pci_get_equalization_presets(struct device *dev,
>> +				    struct pci_eq_presets *presets,
>> +				    int num_lanes)
>> +{
>> +	char name[20];
>> +	int ret;
>> +
>> +	presets->eq_presets_8gts[0] = PCI_EQ_RESV;
>> +	ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>> +					 presets->eq_presets_8gts, num_lanes);
>> +	if (ret && ret != -EINVAL) {
>> +		dev_err(dev, "Error reading eq-presets-8gts :%d\n", ret);
> 
> 'Error reading eq-presets-8gts: %d'
> 
>> +		return ret;
>> +	}
>> +
>> +	for (int i = 0; i < EQ_PRESET_TYPE_MAX - 1; i++) {
>> +		presets->eq_presets_Ngts[i][0] = PCI_EQ_RESV;
>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << (i + 1));
>> +		ret = of_property_read_u8_array(dev->of_node, name,
>> +						presets->eq_presets_Ngts[i],
>> +						num_lanes);
>> +		if (ret && ret != -EINVAL) {
>> +			dev_err(dev, "Error reading %s :%d\n", name, ret);
> 
> 'Error reading %s: %d'
> 
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 01e51db8d285..78c9cc0ad8fa 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -9,6 +9,8 @@ struct pcie_tlp_log;
>>   /* Number of possible devfns: 0.0 to 1f.7 inclusive */
>>   #define MAX_NR_DEVFNS 256
>>   
>> +#define MAX_NR_LANES 16
> 
> Why did you limit to 16?
> 
As per PCIe spec we support maximum of 16 lanes only right

- Krishna Chaitanya.
> - Mani
> 

Re: [PATCH v8 2/4] PCI: of: Add of_pci_get_equalization_presets() API

[Manivannan Sadhasivam]

On Fri, Mar 28, 2025 at 10:54:25AM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 3/28/2025 10:09 AM, Manivannan Sadhasivam wrote:
> > On Sun, Mar 16, 2025 at 09:39:02AM +0530, Krishna Chaitanya Chundru wrote:
> > > PCIe equalization presets are predefined settings used to optimize
> > > signal integrity by compensating for signal loss and distortion in
> > > high-speed data transmission.
> > > 
> > > As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
> > > of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
> > > configure lane equalization presets for each lane to enhance the PCIe
> > > link reliability. Each preset value represents a different combination
> > > of pre-shoot and de-emphasis values. For each data rate, different
> > > registers are defined: for 8.0 GT/s, registers are defined in section
> > > 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
> > > an extra receiver preset hint, requiring 16 bits per lane, while the
> > > remaining data rates use 8 bits per lane.
> > > 
> > > Based on the number of lanes and the supported data rate,
> > > of_pci_get_equalization_presets() reads the device tree property and
> > > stores in the presets structure.
> > > 
> > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > ---
> > >   drivers/pci/of.c  | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > >   drivers/pci/pci.h | 32 +++++++++++++++++++++++++++++++-
> > >   2 files changed, 75 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > index 7a806f5c0d20..18691483e108 100644
> > > --- a/drivers/pci/of.c
> > > +++ b/drivers/pci/of.c
> > > @@ -851,3 +851,47 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
> > >   	return slot_power_limit_mw;
> > >   }
> > >   EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> > > +
> > > +/**
> > > + * of_pci_get_equalization_presets - Parses the "eq-presets-Ngts" property.
> > > + *
> > > + * @dev: Device containing the properties.
> > > + * @presets: Pointer to store the parsed data.
> > > + * @num_lanes: Maximum number of lanes supported.
> > > + *
> > > + * If the property is present, read and store the data in the @presets structure.
> > > + * Else, assign a default value of PCI_EQ_RESV.
> > > + *
> > > + * Return: 0 if the property is not available or successfully parsed else
> > > + * errno otherwise.
> > > + */
> > > +int of_pci_get_equalization_presets(struct device *dev,
> > > +				    struct pci_eq_presets *presets,
> > > +				    int num_lanes)
> > > +{
> > > +	char name[20];
> > > +	int ret;
> > > +
> > > +	presets->eq_presets_8gts[0] = PCI_EQ_RESV;
> > > +	ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> > > +					 presets->eq_presets_8gts, num_lanes);
> > > +	if (ret && ret != -EINVAL) {
> > > +		dev_err(dev, "Error reading eq-presets-8gts :%d\n", ret);
> > 
> > 'Error reading eq-presets-8gts: %d'
> > 
> > > +		return ret;
> > > +	}
> > > +
> > > +	for (int i = 0; i < EQ_PRESET_TYPE_MAX - 1; i++) {
> > > +		presets->eq_presets_Ngts[i][0] = PCI_EQ_RESV;
> > > +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << (i + 1));
> > > +		ret = of_property_read_u8_array(dev->of_node, name,
> > > +						presets->eq_presets_Ngts[i],
> > > +						num_lanes);
> > > +		if (ret && ret != -EINVAL) {
> > > +			dev_err(dev, "Error reading %s :%d\n", name, ret);
> > 
> > 'Error reading %s: %d'
> > 
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 01e51db8d285..78c9cc0ad8fa 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -9,6 +9,8 @@ struct pcie_tlp_log;
> > >   /* Number of possible devfns: 0.0 to 1f.7 inclusive */
> > >   #define MAX_NR_DEVFNS 256
> > > +#define MAX_NR_LANES 16
> > 
> > Why did you limit to 16?
> > 
> As per PCIe spec we support maximum of 16 lanes only right
> 

No. PCIe spec defines Max Link Width up to 32 lanes. Though, we have only seen
16 lanes used widely. This field should correspond to 'Maximum Link Width' value
in the Link Capabilities Register.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 2/4] PCI: of: Add of_pci_get_equalization_presets() API

[Krishna Chaitanya Chundru]

On 3/28/2025 12:13 PM, Manivannan Sadhasivam wrote:
> On Fri, Mar 28, 2025 at 10:54:25AM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 3/28/2025 10:09 AM, Manivannan Sadhasivam wrote:
>>> On Sun, Mar 16, 2025 at 09:39:02AM +0530, Krishna Chaitanya Chundru wrote:
>>>> PCIe equalization presets are predefined settings used to optimize
>>>> signal integrity by compensating for signal loss and distortion in
>>>> high-speed data transmission.
>>>>
>>>> As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
>>>> of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
>>>> configure lane equalization presets for each lane to enhance the PCIe
>>>> link reliability. Each preset value represents a different combination
>>>> of pre-shoot and de-emphasis values. For each data rate, different
>>>> registers are defined: for 8.0 GT/s, registers are defined in section
>>>> 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
>>>> an extra receiver preset hint, requiring 16 bits per lane, while the
>>>> remaining data rates use 8 bits per lane.
>>>>
>>>> Based on the number of lanes and the supported data rate,
>>>> of_pci_get_equalization_presets() reads the device tree property and
>>>> stores in the presets structure.
>>>>
>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>>>    drivers/pci/of.c  | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>>>    drivers/pci/pci.h | 32 +++++++++++++++++++++++++++++++-
>>>>    2 files changed, 75 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>> index 7a806f5c0d20..18691483e108 100644
>>>> --- a/drivers/pci/of.c
>>>> +++ b/drivers/pci/of.c
>>>> @@ -851,3 +851,47 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
>>>>    	return slot_power_limit_mw;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
>>>> +
>>>> +/**
>>>> + * of_pci_get_equalization_presets - Parses the "eq-presets-Ngts" property.
>>>> + *
>>>> + * @dev: Device containing the properties.
>>>> + * @presets: Pointer to store the parsed data.
>>>> + * @num_lanes: Maximum number of lanes supported.
>>>> + *
>>>> + * If the property is present, read and store the data in the @presets structure.
>>>> + * Else, assign a default value of PCI_EQ_RESV.
>>>> + *
>>>> + * Return: 0 if the property is not available or successfully parsed else
>>>> + * errno otherwise.
>>>> + */
>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>> +				    struct pci_eq_presets *presets,
>>>> +				    int num_lanes)
>>>> +{
>>>> +	char name[20];
>>>> +	int ret;
>>>> +
>>>> +	presets->eq_presets_8gts[0] = PCI_EQ_RESV;
>>>> +	ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>> +					 presets->eq_presets_8gts, num_lanes);
>>>> +	if (ret && ret != -EINVAL) {
>>>> +		dev_err(dev, "Error reading eq-presets-8gts :%d\n", ret);
>>>
>>> 'Error reading eq-presets-8gts: %d'
>>>
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	for (int i = 0; i < EQ_PRESET_TYPE_MAX - 1; i++) {
>>>> +		presets->eq_presets_Ngts[i][0] = PCI_EQ_RESV;
>>>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << (i + 1));
>>>> +		ret = of_property_read_u8_array(dev->of_node, name,
>>>> +						presets->eq_presets_Ngts[i],
>>>> +						num_lanes);
>>>> +		if (ret && ret != -EINVAL) {
>>>> +			dev_err(dev, "Error reading %s :%d\n", name, ret);
>>>
>>> 'Error reading %s: %d'
>>>
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index 01e51db8d285..78c9cc0ad8fa 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -9,6 +9,8 @@ struct pcie_tlp_log;
>>>>    /* Number of possible devfns: 0.0 to 1f.7 inclusive */
>>>>    #define MAX_NR_DEVFNS 256
>>>> +#define MAX_NR_LANES 16
>>>
>>> Why did you limit to 16?
>>>
>> As per PCIe spec we support maximum of 16 lanes only right
>>
> 
> No. PCIe spec defines Max Link Width up to 32 lanes. Though, we have only seen
> 16 lanes used widely. This field should correspond to 'Maximum Link Width' value
> in the Link Capabilities Register.
> 
As per spec 6.0.1 section 7.5.3.6 Link Capabilities Register max link
width is x16 only.

- Krishna Chaitanya.
> - Mani
> 

Re: [PATCH v8 2/4] PCI: of: Add of_pci_get_equalization_presets() API

[Manivannan Sadhasivam]

On Fri, Mar 28, 2025 at 12:22:23PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 3/28/2025 12:13 PM, Manivannan Sadhasivam wrote:
> > On Fri, Mar 28, 2025 at 10:54:25AM +0530, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 3/28/2025 10:09 AM, Manivannan Sadhasivam wrote:
> > > > On Sun, Mar 16, 2025 at 09:39:02AM +0530, Krishna Chaitanya Chundru wrote:
> > > > > PCIe equalization presets are predefined settings used to optimize
> > > > > signal integrity by compensating for signal loss and distortion in
> > > > > high-speed data transmission.
> > > > > 
> > > > > As per PCIe spec 6.0.1 revision section 8.3.3.3 & 4.2.4 for data rates
> > > > > of 8.0 GT/s, 16.0 GT/s, 32.0 GT/s, and 64.0 GT/s, there is a way to
> > > > > configure lane equalization presets for each lane to enhance the PCIe
> > > > > link reliability. Each preset value represents a different combination
> > > > > of pre-shoot and de-emphasis values. For each data rate, different
> > > > > registers are defined: for 8.0 GT/s, registers are defined in section
> > > > > 7.7.3.4; for 16.0 GT/s, in section 7.7.5.9, etc. The 8.0 GT/s rate has
> > > > > an extra receiver preset hint, requiring 16 bits per lane, while the
> > > > > remaining data rates use 8 bits per lane.
> > > > > 
> > > > > Based on the number of lanes and the supported data rate,
> > > > > of_pci_get_equalization_presets() reads the device tree property and
> > > > > stores in the presets structure.
> > > > > 
> > > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > > > ---
> > > > >    drivers/pci/of.c  | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >    drivers/pci/pci.h | 32 +++++++++++++++++++++++++++++++-
> > > > >    2 files changed, 75 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > > index 7a806f5c0d20..18691483e108 100644
> > > > > --- a/drivers/pci/of.c
> > > > > +++ b/drivers/pci/of.c
> > > > > @@ -851,3 +851,47 @@ u32 of_pci_get_slot_power_limit(struct device_node *node,
> > > > >    	return slot_power_limit_mw;
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit);
> > > > > +
> > > > > +/**
> > > > > + * of_pci_get_equalization_presets - Parses the "eq-presets-Ngts" property.
> > > > > + *
> > > > > + * @dev: Device containing the properties.
> > > > > + * @presets: Pointer to store the parsed data.
> > > > > + * @num_lanes: Maximum number of lanes supported.
> > > > > + *
> > > > > + * If the property is present, read and store the data in the @presets structure.
> > > > > + * Else, assign a default value of PCI_EQ_RESV.
> > > > > + *
> > > > > + * Return: 0 if the property is not available or successfully parsed else
> > > > > + * errno otherwise.
> > > > > + */
> > > > > +int of_pci_get_equalization_presets(struct device *dev,
> > > > > +				    struct pci_eq_presets *presets,
> > > > > +				    int num_lanes)
> > > > > +{
> > > > > +	char name[20];
> > > > > +	int ret;
> > > > > +
> > > > > +	presets->eq_presets_8gts[0] = PCI_EQ_RESV;
> > > > > +	ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> > > > > +					 presets->eq_presets_8gts, num_lanes);
> > > > > +	if (ret && ret != -EINVAL) {
> > > > > +		dev_err(dev, "Error reading eq-presets-8gts :%d\n", ret);
> > > > 
> > > > 'Error reading eq-presets-8gts: %d'
> > > > 
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	for (int i = 0; i < EQ_PRESET_TYPE_MAX - 1; i++) {
> > > > > +		presets->eq_presets_Ngts[i][0] = PCI_EQ_RESV;
> > > > > +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << (i + 1));
> > > > > +		ret = of_property_read_u8_array(dev->of_node, name,
> > > > > +						presets->eq_presets_Ngts[i],
> > > > > +						num_lanes);
> > > > > +		if (ret && ret != -EINVAL) {
> > > > > +			dev_err(dev, "Error reading %s :%d\n", name, ret);
> > > > 
> > > > 'Error reading %s: %d'
> > > > 
> > > > > +			return ret;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > > index 01e51db8d285..78c9cc0ad8fa 100644
> > > > > --- a/drivers/pci/pci.h
> > > > > +++ b/drivers/pci/pci.h
> > > > > @@ -9,6 +9,8 @@ struct pcie_tlp_log;
> > > > >    /* Number of possible devfns: 0.0 to 1f.7 inclusive */
> > > > >    #define MAX_NR_DEVFNS 256
> > > > > +#define MAX_NR_LANES 16
> > > > 
> > > > Why did you limit to 16?
> > > > 
> > > As per PCIe spec we support maximum of 16 lanes only right
> > > 
> > 
> > No. PCIe spec defines Max Link Width up to 32 lanes. Though, we have only seen
> > 16 lanes used widely. This field should correspond to 'Maximum Link Width' value
> > in the Link Capabilities Register.
> > 
> As per spec 6.0.1 section 7.5.3.6 Link Capabilities Register max link
> width is x16 only.
> 

Interesting! I referred the 5.0 spec and it mentions x32 and they seem to have
removed it in 6.0. So I guess we should go with the latest spec (I hope it stays
the same at 7.0 also).

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v8 3/4] PCI: dwc: Update pci->num_lanes to maximum supported link width

[Krishna Chaitanya Chundru]

If the num-lanes property is not present in the devicetree update the
pci->num_lanes with the hardware supported maximum link width using
the newly introduced dw_pcie_link_get_max_link_width() API.

Introduce dw_pcie_link_get_max_link_width() to get the maximum lane
width the hardware supports.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
 drivers/pci/controller/dwc/pcie-designware.c      | 8 ++++++++
 drivers/pci/controller/dwc/pcie-designware.h      | 1 +
 3 files changed, 12 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index ffaded8f2df7..dd56cc02f4ef 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -504,6 +504,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 
 	dw_pcie_iatu_detect(pci);
 
+	if (pci->num_lanes < 1)
+		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
+
 	/*
 	 * Allocate the resource for MSG TLP before programming the iATU
 	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 145e7f579072..f39e6f5732a9 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -737,6 +737,14 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
 
 }
 
+int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
+{
+	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u32 lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
+
+	return FIELD_GET(PCI_EXP_LNKCAP_MLW, lnkcap);
+}
+
 static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
 {
 	u32 lnkcap, lwsc, plc;
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 501d9ddfea16..61d1fb6b437b 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -488,6 +488,7 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
 int dw_pcie_link_up(struct dw_pcie *pci);
 void dw_pcie_upconfig_setup(struct dw_pcie *pci);
 int dw_pcie_wait_for_link(struct dw_pcie *pci);
+int dw_pcie_link_get_max_link_width(struct dw_pcie *pci);
 int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
 			      const struct dw_pcie_ob_atu_cfg *atu);
 int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,

-- 
2.34.1

Re: [PATCH v8 3/4] PCI: dwc: Update pci->num_lanes to maximum supported link width

[Manivannan Sadhasivam]

On Sun, Mar 16, 2025 at 09:39:03AM +0530, Krishna Chaitanya Chundru wrote:
> If the num-lanes property is not present in the devicetree update the
> pci->num_lanes with the hardware supported maximum link width using
> the newly introduced dw_pcie_link_get_max_link_width() API.
> 
> Introduce dw_pcie_link_get_max_link_width() to get the maximum lane
> width the hardware supports.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 3 +++
>  drivers/pci/controller/dwc/pcie-designware.c      | 8 ++++++++
>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index ffaded8f2df7..dd56cc02f4ef 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -504,6 +504,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  
>  	dw_pcie_iatu_detect(pci);
>  
> +	if (pci->num_lanes < 1)
> +		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
> +
>  	/*
>  	 * Allocate the resource for MSG TLP before programming the iATU
>  	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 145e7f579072..f39e6f5732a9 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -737,6 +737,14 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
>  
>  }
>  
> +int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
> +{
> +	u8 cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u32 lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> +
> +	return FIELD_GET(PCI_EXP_LNKCAP_MLW, lnkcap);
> +}
> +
>  static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  {
>  	u32 lnkcap, lwsc, plc;
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 501d9ddfea16..61d1fb6b437b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -488,6 +488,7 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
>  int dw_pcie_link_up(struct dw_pcie *pci);
>  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
>  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> +int dw_pcie_link_get_max_link_width(struct dw_pcie *pci);
>  int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
>  			      const struct dw_pcie_ob_atu_cfg *atu);
>  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> 
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Krishna Chaitanya Chundru]

PCIe equalization presets are predefined settings used to optimize
signal integrity by compensating for signal loss and distortion in
high-speed data transmission.

Based upon the number of lanes and the data rate supported, write
the preset data read from the device tree in to the lane equalization
control registers.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
 include/uapi/linux/pci_regs.h                     |  3 ++
 3 files changed, 66 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index dd56cc02f4ef..7c6e6a74383b 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (pci->num_lanes < 1)
 		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
 
+	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
+	if (ret)
+		goto err_free_msi;
+
 	/*
 	 * Allocate the resource for MSG TLP before programming the iATU
 	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
@@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 	return 0;
 }
 
+static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	u8 lane_eq_offset, lane_reg_size, cap_id;
+	u8 *presets;
+	u32 cap;
+	int i;
+
+	if (speed == PCIE_SPEED_8_0GT) {
+		presets = (u8 *)pp->presets.eq_presets_8gts;
+		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
+		cap_id = PCI_EXT_CAP_ID_SECPCI;
+		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
+		lane_reg_size = 0x2;
+	} else if (speed == PCIE_SPEED_16_0GT) {
+		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
+		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
+		cap_id = PCI_EXT_CAP_ID_PL_16GT;
+		lane_reg_size = 0x1;
+	} else {
+		return;
+	}
+
+	if (presets[0] == PCI_EQ_RESV)
+		return;
+
+	cap = dw_pcie_find_ext_capability(pci, cap_id);
+	if (!cap)
+		return;
+
+	/*
+	 * Write preset values to the registers byte-by-byte for the given
+	 * number of lanes and register size.
+	 */
+	for (i = 0; i < pci->num_lanes * lane_reg_size; i++)
+		dw_pcie_writeb_dbi(pci, cap + lane_eq_offset + i, presets[i]);
+}
+
+static void dw_pcie_config_presets(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	enum pci_bus_speed speed = pcie_link_speed[pci->max_link_speed];
+
+	/*
+	 * Lane equalization needs to be perfomed for all data rates
+	 * the controller supports and for all supported lanes.
+	 */
+
+	if (speed >= PCIE_SPEED_8_0GT)
+		dw_pcie_program_presets(pp, PCIE_SPEED_8_0GT);
+
+	if (speed >= PCIE_SPEED_16_0GT)
+		dw_pcie_program_presets(pp, PCIE_SPEED_16_0GT);
+}
+
 int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -861,6 +920,7 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
 		PCI_COMMAND_MASTER | PCI_COMMAND_SERR;
 	dw_pcie_writel_dbi(pci, PCI_COMMAND, val);
 
+	dw_pcie_config_presets(pp);
 	/*
 	 * If the platform provides its own child bus config accesses, it means
 	 * the platform uses its own address translation component rather than
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 61d1fb6b437b..30ae8d3f4282 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -25,6 +25,8 @@
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
 
+#include "../../pci.h"
+
 /* DWC PCIe IP-core versions (native support since v4.70a) */
 #define DW_PCIE_VER_365A		0x3336352a
 #define DW_PCIE_VER_460A		0x3436302a
@@ -381,6 +383,7 @@ struct dw_pcie_rp {
 	int			msg_atu_index;
 	struct resource		*msg_res;
 	bool			use_linkup_irq;
+	struct pci_eq_presets	presets;
 };
 
 struct dw_pcie_ep_ops {
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 3445c4970e4d..2cd20170adb4 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1140,6 +1140,9 @@
 #define PCI_DLF_CAP		0x04	/* Capabilities Register */
 #define  PCI_DLF_EXCHANGE_ENABLE	0x80000000  /* Data Link Feature Exchange Enable */
 
+/* Secondary PCIe Capability 8.0 GT/s */
+#define PCI_SECPCI_LE_CTRL	0x0c /* Lane Equalization Control Register */
+
 /* Physical Layer 16.0 GT/s */
 #define PCI_PL_16GT_LE_CTRL	0x20	/* Lane Equalization Control Register */
 #define  PCI_PL_16GT_LE_CTRL_DSP_TX_PRESET_MASK		0x0000000F

-- 
2.34.1

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Manivannan Sadhasivam]

On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
> PCIe equalization presets are predefined settings used to optimize
> signal integrity by compensating for signal loss and distortion in
> high-speed data transmission.
> 
> Based upon the number of lanes and the data rate supported, write
> the preset data read from the device tree in to the lane equalization
> control registers.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
>  include/uapi/linux/pci_regs.h                     |  3 ++
>  3 files changed, 66 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index dd56cc02f4ef..7c6e6a74383b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (pci->num_lanes < 1)
>  		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
>  
> +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
> +	if (ret)
> +		goto err_free_msi;
> +
>  	/*
>  	 * Allocate the resource for MSG TLP before programming the iATU
>  	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
> @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  	return 0;
>  }
>  
> +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	u8 lane_eq_offset, lane_reg_size, cap_id;
> +	u8 *presets;
> +	u32 cap;
> +	int i;
> +
> +	if (speed == PCIE_SPEED_8_0GT) {
> +		presets = (u8 *)pp->presets.eq_presets_8gts;
> +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
> +		cap_id = PCI_EXT_CAP_ID_SECPCI;
> +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
> +		lane_reg_size = 0x2;
> +	} else if (speed == PCIE_SPEED_16_0GT) {
> +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
> +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
> +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
> +		lane_reg_size = 0x1;
> +	} else {

Can you add conditions for other data rates also? Like 32, 64 GT/s. If
controller supports them and if the presets property is defined in DT, then you
should apply the preset values.

If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
safely return.

> +		return;
> +	}
> +
> +	if (presets[0] == PCI_EQ_RESV)
> +		return;
> +
> +	cap = dw_pcie_find_ext_capability(pci, cap_id);
> +	if (!cap)
> +		return;
> +
> +	/*
> +	 * Write preset values to the registers byte-by-byte for the given
> +	 * number of lanes and register size.
> +	 */
> +	for (i = 0; i < pci->num_lanes * lane_reg_size; i++)
> +		dw_pcie_writeb_dbi(pci, cap + lane_eq_offset + i, presets[i]);
> +}
> +
> +static void dw_pcie_config_presets(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	enum pci_bus_speed speed = pcie_link_speed[pci->max_link_speed];
> +
> +	/*
> +	 * Lane equalization needs to be perfomed for all data rates
> +	 * the controller supports and for all supported lanes.
> +	 */
> +
> +	if (speed >= PCIE_SPEED_8_0GT)
> +		dw_pcie_program_presets(pp, PCIE_SPEED_8_0GT);
> +
> +	if (speed >= PCIE_SPEED_16_0GT)
> +		dw_pcie_program_presets(pp, PCIE_SPEED_16_0GT);

Add other data rates also.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Krishna Chaitanya Chundru]

On 3/28/2025 10:23 AM, Manivannan Sadhasivam wrote:
> On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
>> PCIe equalization presets are predefined settings used to optimize
>> signal integrity by compensating for signal loss and distortion in
>> high-speed data transmission.
>>
>> Based upon the number of lanes and the data rate supported, write
>> the preset data read from the device tree in to the lane equalization
>> control registers.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
>>   drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
>>   include/uapi/linux/pci_regs.h                     |  3 ++
>>   3 files changed, 66 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index dd56cc02f4ef..7c6e6a74383b 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>   	if (pci->num_lanes < 1)
>>   		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
>>   
>> +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
>> +	if (ret)
>> +		goto err_free_msi;
>> +
>>   	/*
>>   	 * Allocate the resource for MSG TLP before programming the iATU
>>   	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
>> @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>>   	return 0;
>>   }
>>   
>> +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	u8 lane_eq_offset, lane_reg_size, cap_id;
>> +	u8 *presets;
>> +	u32 cap;
>> +	int i;
>> +
>> +	if (speed == PCIE_SPEED_8_0GT) {
>> +		presets = (u8 *)pp->presets.eq_presets_8gts;
>> +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
>> +		cap_id = PCI_EXT_CAP_ID_SECPCI;
>> +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
>> +		lane_reg_size = 0x2;
>> +	} else if (speed == PCIE_SPEED_16_0GT) {
>> +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
>> +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
>> +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
>> +		lane_reg_size = 0x1;
>> +	} else {
> 
> Can you add conditions for other data rates also? Like 32, 64 GT/s. If
> controller supports them and if the presets property is defined in DT, then you
> should apply the preset values.
> 
> If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
> safely return.
> 
I am fine to add it, but there is no GEN5 or GEN6 controller support
added in dwc, isn't it best to add when that support is added and
tested.

- Krishna Chaitanya.

>> +		return;
>> +	}
>> +
>> +	if (presets[0] == PCI_EQ_RESV)
>> +		return;
>> +
>> +	cap = dw_pcie_find_ext_capability(pci, cap_id);
>> +	if (!cap)
>> +		return;
>> +
>> +	/*
>> +	 * Write preset values to the registers byte-by-byte for the given
>> +	 * number of lanes and register size.
>> +	 */
>> +	for (i = 0; i < pci->num_lanes * lane_reg_size; i++)
>> +		dw_pcie_writeb_dbi(pci, cap + lane_eq_offset + i, presets[i]);
>> +}
>> +
>> +static void dw_pcie_config_presets(struct dw_pcie_rp *pp)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	enum pci_bus_speed speed = pcie_link_speed[pci->max_link_speed];
>> +
>> +	/*
>> +	 * Lane equalization needs to be perfomed for all data rates
>> +	 * the controller supports and for all supported lanes.
>> +	 */
>> +
>> +	if (speed >= PCIE_SPEED_8_0GT)
>> +		dw_pcie_program_presets(pp, PCIE_SPEED_8_0GT);
>> +
>> +	if (speed >= PCIE_SPEED_16_0GT)
>> +		dw_pcie_program_presets(pp, PCIE_SPEED_16_0GT);
> 
> Add other data rates also.
> 
> - Mani
> 

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Manivannan Sadhasivam]

On Fri, Mar 28, 2025 at 11:04:11AM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 3/28/2025 10:23 AM, Manivannan Sadhasivam wrote:
> > On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
> > > PCIe equalization presets are predefined settings used to optimize
> > > signal integrity by compensating for signal loss and distortion in
> > > high-speed data transmission.
> > > 
> > > Based upon the number of lanes and the data rate supported, write
> > > the preset data read from the device tree in to the lane equalization
> > > control registers.
> > > 
> > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
> > >   drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
> > >   include/uapi/linux/pci_regs.h                     |  3 ++
> > >   3 files changed, 66 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index dd56cc02f4ef..7c6e6a74383b 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >   	if (pci->num_lanes < 1)
> > >   		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
> > > +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
> > > +	if (ret)
> > > +		goto err_free_msi;
> > > +
> > >   	/*
> > >   	 * Allocate the resource for MSG TLP before programming the iATU
> > >   	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
> > > @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > >   	return 0;
> > >   }
> > > +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	u8 lane_eq_offset, lane_reg_size, cap_id;
> > > +	u8 *presets;
> > > +	u32 cap;
> > > +	int i;
> > > +
> > > +	if (speed == PCIE_SPEED_8_0GT) {
> > > +		presets = (u8 *)pp->presets.eq_presets_8gts;
> > > +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
> > > +		cap_id = PCI_EXT_CAP_ID_SECPCI;
> > > +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
> > > +		lane_reg_size = 0x2;
> > > +	} else if (speed == PCIE_SPEED_16_0GT) {
> > > +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
> > > +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
> > > +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
> > > +		lane_reg_size = 0x1;
> > > +	} else {
> > 
> > Can you add conditions for other data rates also? Like 32, 64 GT/s. If
> > controller supports them and if the presets property is defined in DT, then you
> > should apply the preset values.
> > 
> > If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
> > safely return.
> > 
> I am fine to add it, but there is no GEN5 or GEN6 controller support
> added in dwc, isn't it best to add when that support is added and
> tested.
> 

What is the guarantee that this part of the code will be updated once the
capable controllers start showing up? I don't think there will be any issue in
writing to these registers.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Konrad Dybcio]

On 3/28/25 7:45 AM, Manivannan Sadhasivam wrote:
> On Fri, Mar 28, 2025 at 11:04:11AM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 3/28/2025 10:23 AM, Manivannan Sadhasivam wrote:
>>> On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
>>>> PCIe equalization presets are predefined settings used to optimize
>>>> signal integrity by compensating for signal loss and distortion in
>>>> high-speed data transmission.
>>>>
>>>> Based upon the number of lanes and the data rate supported, write
>>>> the preset data read from the device tree in to the lane equalization
>>>> control registers.
>>>>
>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
>>>>   drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
>>>>   include/uapi/linux/pci_regs.h                     |  3 ++
>>>>   3 files changed, 66 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> index dd56cc02f4ef..7c6e6a74383b 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>>   	if (pci->num_lanes < 1)
>>>>   		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
>>>> +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
>>>> +	if (ret)
>>>> +		goto err_free_msi;
>>>> +
>>>>   	/*
>>>>   	 * Allocate the resource for MSG TLP before programming the iATU
>>>>   	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
>>>> @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>>>>   	return 0;
>>>>   }
>>>> +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
>>>> +{
>>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>> +	u8 lane_eq_offset, lane_reg_size, cap_id;
>>>> +	u8 *presets;
>>>> +	u32 cap;
>>>> +	int i;
>>>> +
>>>> +	if (speed == PCIE_SPEED_8_0GT) {
>>>> +		presets = (u8 *)pp->presets.eq_presets_8gts;
>>>> +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
>>>> +		cap_id = PCI_EXT_CAP_ID_SECPCI;
>>>> +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
>>>> +		lane_reg_size = 0x2;
>>>> +	} else if (speed == PCIE_SPEED_16_0GT) {
>>>> +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
>>>> +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
>>>> +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
>>>> +		lane_reg_size = 0x1;
>>>> +	} else {
>>>
>>> Can you add conditions for other data rates also? Like 32, 64 GT/s. If
>>> controller supports them and if the presets property is defined in DT, then you
>>> should apply the preset values.
>>>
>>> If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
>>> safely return.
>>>
>> I am fine to add it, but there is no GEN5 or GEN6 controller support
>> added in dwc, isn't it best to add when that support is added and
>> tested.
>>
> 
> What is the guarantee that this part of the code will be updated once the
> capable controllers start showing up? I don't think there will be any issue in
> writing to these registers.

Let's not make assumptions about the spec of a cross-vendor mass-deployed IP

Konrad

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Manivannan Sadhasivam]

On Fri, Mar 28, 2025 at 10:53:19PM +0100, Konrad Dybcio wrote:
> On 3/28/25 7:45 AM, Manivannan Sadhasivam wrote:
> > On Fri, Mar 28, 2025 at 11:04:11AM +0530, Krishna Chaitanya Chundru wrote:
> >>
> >>
> >> On 3/28/2025 10:23 AM, Manivannan Sadhasivam wrote:
> >>> On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
> >>>> PCIe equalization presets are predefined settings used to optimize
> >>>> signal integrity by compensating for signal loss and distortion in
> >>>> high-speed data transmission.
> >>>>
> >>>> Based upon the number of lanes and the data rate supported, write
> >>>> the preset data read from the device tree in to the lane equalization
> >>>> control registers.
> >>>>
> >>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> >>>> ---
> >>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
> >>>>   drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
> >>>>   include/uapi/linux/pci_regs.h                     |  3 ++
> >>>>   3 files changed, 66 insertions(+)
> >>>>
> >>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> >>>> index dd56cc02f4ef..7c6e6a74383b 100644
> >>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> >>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> >>>> @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >>>>   	if (pci->num_lanes < 1)
> >>>>   		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
> >>>> +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
> >>>> +	if (ret)
> >>>> +		goto err_free_msi;
> >>>> +
> >>>>   	/*
> >>>>   	 * Allocate the resource for MSG TLP before programming the iATU
> >>>>   	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
> >>>> @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >>>>   	return 0;
> >>>>   }
> >>>> +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
> >>>> +{
> >>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>>> +	u8 lane_eq_offset, lane_reg_size, cap_id;
> >>>> +	u8 *presets;
> >>>> +	u32 cap;
> >>>> +	int i;
> >>>> +
> >>>> +	if (speed == PCIE_SPEED_8_0GT) {
> >>>> +		presets = (u8 *)pp->presets.eq_presets_8gts;
> >>>> +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
> >>>> +		cap_id = PCI_EXT_CAP_ID_SECPCI;
> >>>> +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
> >>>> +		lane_reg_size = 0x2;
> >>>> +	} else if (speed == PCIE_SPEED_16_0GT) {
> >>>> +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
> >>>> +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
> >>>> +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
> >>>> +		lane_reg_size = 0x1;
> >>>> +	} else {
> >>>
> >>> Can you add conditions for other data rates also? Like 32, 64 GT/s. If
> >>> controller supports them and if the presets property is defined in DT, then you
> >>> should apply the preset values.
> >>>
> >>> If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
> >>> safely return.
> >>>
> >> I am fine to add it, but there is no GEN5 or GEN6 controller support
> >> added in dwc, isn't it best to add when that support is added and
> >> tested.
> >>
> > 
> > What is the guarantee that this part of the code will be updated once the
> > capable controllers start showing up? I don't think there will be any issue in
> > writing to these registers.
> 
> Let's not make assumptions about the spec of a cross-vendor mass-deployed IP
> 

I have seen the worse... The problem is, if those controllers start to show up
and define preset properties in DT, there will be no errors whatsoever to
indicate that the preset values were not applied, resulting in hard to debug
errors.

I'm not forseeing any issue in this part of the code to support higher GEN
speeds though.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Konrad Dybcio]

On 3/29/25 7:30 AM, Manivannan Sadhasivam wrote:
> On Fri, Mar 28, 2025 at 10:53:19PM +0100, Konrad Dybcio wrote:
>> On 3/28/25 7:45 AM, Manivannan Sadhasivam wrote:
>>> On Fri, Mar 28, 2025 at 11:04:11AM +0530, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 3/28/2025 10:23 AM, Manivannan Sadhasivam wrote:
>>>>> On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>> PCIe equalization presets are predefined settings used to optimize
>>>>>> signal integrity by compensating for signal loss and distortion in
>>>>>> high-speed data transmission.
>>>>>>
>>>>>> Based upon the number of lanes and the data rate supported, write
>>>>>> the preset data read from the device tree in to the lane equalization
>>>>>> control registers.
>>>>>>
>>>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>>>>> ---
>>>>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
>>>>>>   drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
>>>>>>   include/uapi/linux/pci_regs.h                     |  3 ++
>>>>>>   3 files changed, 66 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>> index dd56cc02f4ef..7c6e6a74383b 100644
>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>> @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>>>>   	if (pci->num_lanes < 1)
>>>>>>   		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
>>>>>> +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
>>>>>> +	if (ret)
>>>>>> +		goto err_free_msi;
>>>>>> +
>>>>>>   	/*
>>>>>>   	 * Allocate the resource for MSG TLP before programming the iATU
>>>>>>   	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
>>>>>> @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>>>>>>   	return 0;
>>>>>>   }
>>>>>> +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
>>>>>> +{
>>>>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>>> +	u8 lane_eq_offset, lane_reg_size, cap_id;
>>>>>> +	u8 *presets;
>>>>>> +	u32 cap;
>>>>>> +	int i;
>>>>>> +
>>>>>> +	if (speed == PCIE_SPEED_8_0GT) {
>>>>>> +		presets = (u8 *)pp->presets.eq_presets_8gts;
>>>>>> +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
>>>>>> +		cap_id = PCI_EXT_CAP_ID_SECPCI;
>>>>>> +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
>>>>>> +		lane_reg_size = 0x2;
>>>>>> +	} else if (speed == PCIE_SPEED_16_0GT) {
>>>>>> +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
>>>>>> +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
>>>>>> +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
>>>>>> +		lane_reg_size = 0x1;
>>>>>> +	} else {
>>>>>
>>>>> Can you add conditions for other data rates also? Like 32, 64 GT/s. If
>>>>> controller supports them and if the presets property is defined in DT, then you
>>>>> should apply the preset values.
>>>>>
>>>>> If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
>>>>> safely return.
>>>>>
>>>> I am fine to add it, but there is no GEN5 or GEN6 controller support
>>>> added in dwc, isn't it best to add when that support is added and
>>>> tested.
>>>>
>>>
>>> What is the guarantee that this part of the code will be updated once the
>>> capable controllers start showing up? I don't think there will be any issue in
>>> writing to these registers.
>>
>> Let's not make assumptions about the spec of a cross-vendor mass-deployed IP
>>
> 
> I have seen the worse... The problem is, if those controllers start to show up
> and define preset properties in DT, there will be no errors whatsoever to
> indicate that the preset values were not applied, resulting in hard to debug
> errors.

else {
	dev_warn(pci->dev, "Missing equalization presets programming sequence\n");
}

> 
> I'm not forseeing any issue in this part of the code to support higher GEN
> speeds though.

I would hope so as well, but both not programming and misprogramming are
equally hard to detect

Konrad

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Manivannan Sadhasivam]

On Sat, Mar 29, 2025 at 09:59:46AM +0100, Konrad Dybcio wrote:
> On 3/29/25 7:30 AM, Manivannan Sadhasivam wrote:
> > On Fri, Mar 28, 2025 at 10:53:19PM +0100, Konrad Dybcio wrote:
> >> On 3/28/25 7:45 AM, Manivannan Sadhasivam wrote:
> >>> On Fri, Mar 28, 2025 at 11:04:11AM +0530, Krishna Chaitanya Chundru wrote:
> >>>>
> >>>>
> >>>> On 3/28/2025 10:23 AM, Manivannan Sadhasivam wrote:
> >>>>> On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
> >>>>>> PCIe equalization presets are predefined settings used to optimize
> >>>>>> signal integrity by compensating for signal loss and distortion in
> >>>>>> high-speed data transmission.
> >>>>>>
> >>>>>> Based upon the number of lanes and the data rate supported, write
> >>>>>> the preset data read from the device tree in to the lane equalization
> >>>>>> control registers.
> >>>>>>
> >>>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> >>>>>> ---
> >>>>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
> >>>>>>   drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
> >>>>>>   include/uapi/linux/pci_regs.h                     |  3 ++
> >>>>>>   3 files changed, 66 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> >>>>>> index dd56cc02f4ef..7c6e6a74383b 100644
> >>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> >>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> >>>>>> @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >>>>>>   	if (pci->num_lanes < 1)
> >>>>>>   		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
> >>>>>> +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
> >>>>>> +	if (ret)
> >>>>>> +		goto err_free_msi;
> >>>>>> +
> >>>>>>   	/*
> >>>>>>   	 * Allocate the resource for MSG TLP before programming the iATU
> >>>>>>   	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
> >>>>>> @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >>>>>>   	return 0;
> >>>>>>   }
> >>>>>> +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
> >>>>>> +{
> >>>>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>>>>> +	u8 lane_eq_offset, lane_reg_size, cap_id;
> >>>>>> +	u8 *presets;
> >>>>>> +	u32 cap;
> >>>>>> +	int i;
> >>>>>> +
> >>>>>> +	if (speed == PCIE_SPEED_8_0GT) {
> >>>>>> +		presets = (u8 *)pp->presets.eq_presets_8gts;
> >>>>>> +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
> >>>>>> +		cap_id = PCI_EXT_CAP_ID_SECPCI;
> >>>>>> +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
> >>>>>> +		lane_reg_size = 0x2;
> >>>>>> +	} else if (speed == PCIE_SPEED_16_0GT) {
> >>>>>> +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
> >>>>>> +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
> >>>>>> +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
> >>>>>> +		lane_reg_size = 0x1;
> >>>>>> +	} else {
> >>>>>
> >>>>> Can you add conditions for other data rates also? Like 32, 64 GT/s. If
> >>>>> controller supports them and if the presets property is defined in DT, then you
> >>>>> should apply the preset values.
> >>>>>
> >>>>> If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
> >>>>> safely return.
> >>>>>
> >>>> I am fine to add it, but there is no GEN5 or GEN6 controller support
> >>>> added in dwc, isn't it best to add when that support is added and
> >>>> tested.
> >>>>
> >>>
> >>> What is the guarantee that this part of the code will be updated once the
> >>> capable controllers start showing up? I don't think there will be any issue in
> >>> writing to these registers.
> >>
> >> Let's not make assumptions about the spec of a cross-vendor mass-deployed IP
> >>
> > 
> > I have seen the worse... The problem is, if those controllers start to show up
> > and define preset properties in DT, there will be no errors whatsoever to
> > indicate that the preset values were not applied, resulting in hard to debug
> > errors.
> 
> else {
> 	dev_warn(pci->dev, "Missing equalization presets programming sequence\n");
> }
> 

Then we'd warn for controllers supporting GEN5 or more if they do not pass the
presets property (which is optional).

> > 
> > I'm not forseeing any issue in this part of the code to support higher GEN
> > speeds though.
> 
> I would hope so as well, but both not programming and misprogramming are
> equally hard to detect
> 

I don't disagree. I wanted to have it since there is no sensible way of warning
users that this part of the code needs to be updated in the future.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Konrad Dybcio]

On 3/29/25 10:39 AM, Manivannan Sadhasivam wrote:
> On Sat, Mar 29, 2025 at 09:59:46AM +0100, Konrad Dybcio wrote:
>> On 3/29/25 7:30 AM, Manivannan Sadhasivam wrote:
>>> On Fri, Mar 28, 2025 at 10:53:19PM +0100, Konrad Dybcio wrote:
>>>> On 3/28/25 7:45 AM, Manivannan Sadhasivam wrote:
>>>>> On Fri, Mar 28, 2025 at 11:04:11AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>>>
>>>>>> On 3/28/2025 10:23 AM, Manivannan Sadhasivam wrote:
>>>>>>> On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>> PCIe equalization presets are predefined settings used to optimize
>>>>>>>> signal integrity by compensating for signal loss and distortion in
>>>>>>>> high-speed data transmission.
>>>>>>>>
>>>>>>>> Based upon the number of lanes and the data rate supported, write
>>>>>>>> the preset data read from the device tree in to the lane equalization
>>>>>>>> control registers.
>>>>>>>>
>>>>>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>>>>>>> ---
>>>>>>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
>>>>>>>>   drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
>>>>>>>>   include/uapi/linux/pci_regs.h                     |  3 ++
>>>>>>>>   3 files changed, 66 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>> index dd56cc02f4ef..7c6e6a74383b 100644
>>>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>> @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>>>>>>   	if (pci->num_lanes < 1)
>>>>>>>>   		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
>>>>>>>> +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
>>>>>>>> +	if (ret)
>>>>>>>> +		goto err_free_msi;
>>>>>>>> +
>>>>>>>>   	/*
>>>>>>>>   	 * Allocate the resource for MSG TLP before programming the iATU
>>>>>>>>   	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
>>>>>>>> @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>>>>>>>>   	return 0;
>>>>>>>>   }
>>>>>>>> +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
>>>>>>>> +{
>>>>>>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>>>>> +	u8 lane_eq_offset, lane_reg_size, cap_id;
>>>>>>>> +	u8 *presets;
>>>>>>>> +	u32 cap;
>>>>>>>> +	int i;
>>>>>>>> +
>>>>>>>> +	if (speed == PCIE_SPEED_8_0GT) {
>>>>>>>> +		presets = (u8 *)pp->presets.eq_presets_8gts;
>>>>>>>> +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
>>>>>>>> +		cap_id = PCI_EXT_CAP_ID_SECPCI;
>>>>>>>> +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
>>>>>>>> +		lane_reg_size = 0x2;
>>>>>>>> +	} else if (speed == PCIE_SPEED_16_0GT) {
>>>>>>>> +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
>>>>>>>> +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
>>>>>>>> +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
>>>>>>>> +		lane_reg_size = 0x1;
>>>>>>>> +	} else {
>>>>>>>
>>>>>>> Can you add conditions for other data rates also? Like 32, 64 GT/s. If
>>>>>>> controller supports them and if the presets property is defined in DT, then you
>>>>>>> should apply the preset values.
>>>>>>>
>>>>>>> If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
>>>>>>> safely return.
>>>>>>>
>>>>>> I am fine to add it, but there is no GEN5 or GEN6 controller support
>>>>>> added in dwc, isn't it best to add when that support is added and
>>>>>> tested.
>>>>>>
>>>>>
>>>>> What is the guarantee that this part of the code will be updated once the
>>>>> capable controllers start showing up? I don't think there will be any issue in
>>>>> writing to these registers.
>>>>
>>>> Let's not make assumptions about the spec of a cross-vendor mass-deployed IP
>>>>
>>>
>>> I have seen the worse... The problem is, if those controllers start to show up
>>> and define preset properties in DT, there will be no errors whatsoever to
>>> indicate that the preset values were not applied, resulting in hard to debug
>>> errors.
>>
>> else {
>> 	dev_warn(pci->dev, "Missing equalization presets programming sequence\n");
>> }
>>
> 
> Then we'd warn for controllers supporting GEN5 or more if they do not pass the
> presets property (which is optional).

Ohh, I didn't think about that - and I can only think about solutions that are
rather janky.. with perhaps the least janky one being changing the else case I
proposed above into:

else if (speed >= PCIE_SPEED_32_0GT && eq_presets_Ngts[speed - PCIE_SPEED_16_0GT][0] != PCI_EQ_RESV) {
	...
}> 
>>>
>>> I'm not forseeing any issue in this part of the code to support higher GEN
>>> speeds though.
>>
>> I would hope so as well, but both not programming and misprogramming are
>> equally hard to detect
>>
> 
> I don't disagree. I wanted to have it since there is no sensible way of warning
> users that this part of the code needs to be updated in the future.

I understand, however I'm worried that the programming sequence or register
may change for higher speeds in a way that would be incompatible with what
we assume here

Konrad

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Manivannan Sadhasivam]

On Sat, Mar 29, 2025 at 12:42:02PM +0100, Konrad Dybcio wrote:
> On 3/29/25 10:39 AM, Manivannan Sadhasivam wrote:
> > On Sat, Mar 29, 2025 at 09:59:46AM +0100, Konrad Dybcio wrote:
> >> On 3/29/25 7:30 AM, Manivannan Sadhasivam wrote:
> >>> On Fri, Mar 28, 2025 at 10:53:19PM +0100, Konrad Dybcio wrote:
> >>>> On 3/28/25 7:45 AM, Manivannan Sadhasivam wrote:
> >>>>> On Fri, Mar 28, 2025 at 11:04:11AM +0530, Krishna Chaitanya Chundru wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 3/28/2025 10:23 AM, Manivannan Sadhasivam wrote:
> >>>>>>> On Sun, Mar 16, 2025 at 09:39:04AM +0530, Krishna Chaitanya Chundru wrote:
> >>>>>>>> PCIe equalization presets are predefined settings used to optimize
> >>>>>>>> signal integrity by compensating for signal loss and distortion in
> >>>>>>>> high-speed data transmission.
> >>>>>>>>
> >>>>>>>> Based upon the number of lanes and the data rate supported, write
> >>>>>>>> the preset data read from the device tree in to the lane equalization
> >>>>>>>> control registers.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> >>>>>>>> ---
> >>>>>>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 60 +++++++++++++++++++++++
> >>>>>>>>   drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
> >>>>>>>>   include/uapi/linux/pci_regs.h                     |  3 ++
> >>>>>>>>   3 files changed, 66 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> >>>>>>>> index dd56cc02f4ef..7c6e6a74383b 100644
> >>>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> >>>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> >>>>>>>> @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >>>>>>>>   	if (pci->num_lanes < 1)
> >>>>>>>>   		pci->num_lanes = dw_pcie_link_get_max_link_width(pci);
> >>>>>>>> +	ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes);
> >>>>>>>> +	if (ret)
> >>>>>>>> +		goto err_free_msi;
> >>>>>>>> +
> >>>>>>>>   	/*
> >>>>>>>>   	 * Allocate the resource for MSG TLP before programming the iATU
> >>>>>>>>   	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
> >>>>>>>> @@ -808,6 +812,61 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >>>>>>>>   	return 0;
> >>>>>>>>   }
> >>>>>>>> +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed)
> >>>>>>>> +{
> >>>>>>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>>>>>>> +	u8 lane_eq_offset, lane_reg_size, cap_id;
> >>>>>>>> +	u8 *presets;
> >>>>>>>> +	u32 cap;
> >>>>>>>> +	int i;
> >>>>>>>> +
> >>>>>>>> +	if (speed == PCIE_SPEED_8_0GT) {
> >>>>>>>> +		presets = (u8 *)pp->presets.eq_presets_8gts;
> >>>>>>>> +		lane_eq_offset =  PCI_SECPCI_LE_CTRL;
> >>>>>>>> +		cap_id = PCI_EXT_CAP_ID_SECPCI;
> >>>>>>>> +		/* For data rate of 8 GT/S each lane equalization control is 16bits wide*/
> >>>>>>>> +		lane_reg_size = 0x2;
> >>>>>>>> +	} else if (speed == PCIE_SPEED_16_0GT) {
> >>>>>>>> +		presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS - 1];
> >>>>>>>> +		lane_eq_offset = PCI_PL_16GT_LE_CTRL;
> >>>>>>>> +		cap_id = PCI_EXT_CAP_ID_PL_16GT;
> >>>>>>>> +		lane_reg_size = 0x1;
> >>>>>>>> +	} else {
> >>>>>>>
> >>>>>>> Can you add conditions for other data rates also? Like 32, 64 GT/s. If
> >>>>>>> controller supports them and if the presets property is defined in DT, then you
> >>>>>>> should apply the preset values.
> >>>>>>>
> >>>>>>> If the presets property is not present in DT, then below 'PCI_EQ_RESV' will
> >>>>>>> safely return.
> >>>>>>>
> >>>>>> I am fine to add it, but there is no GEN5 or GEN6 controller support
> >>>>>> added in dwc, isn't it best to add when that support is added and
> >>>>>> tested.
> >>>>>>
> >>>>>
> >>>>> What is the guarantee that this part of the code will be updated once the
> >>>>> capable controllers start showing up? I don't think there will be any issue in
> >>>>> writing to these registers.
> >>>>
> >>>> Let's not make assumptions about the spec of a cross-vendor mass-deployed IP
> >>>>
> >>>
> >>> I have seen the worse... The problem is, if those controllers start to show up
> >>> and define preset properties in DT, there will be no errors whatsoever to
> >>> indicate that the preset values were not applied, resulting in hard to debug
> >>> errors.
> >>
> >> else {
> >> 	dev_warn(pci->dev, "Missing equalization presets programming sequence\n");
> >> }
> >>
> > 
> > Then we'd warn for controllers supporting GEN5 or more if they do not pass the
> > presets property (which is optional).
> 
> Ohh, I didn't think about that - and I can only think about solutions that are
> rather janky.. with perhaps the least janky one being changing the else case I
> proposed above into:
> 
> else if (speed >= PCIE_SPEED_32_0GT && eq_presets_Ngts[speed - PCIE_SPEED_16_0GT][0] != PCI_EQ_RESV) {

s/PCIE_SPEED_16_0GT/PCIE_SPEED_32_0GT

> 	...

So this I read as: Oh, your controller supports 32 GT/s and you firmware also
wanted to apply the custom preset offsets, but sorry we didn't do it because we
don't know if it would work or not. So please let us know so that we can work
with you test it and then finally we can apply the presets.

> }> 
> >>>
> >>> I'm not forseeing any issue in this part of the code to support higher GEN
> >>> speeds though.
> >>
> >> I would hope so as well, but both not programming and misprogramming are
> >> equally hard to detect
> >>
> > 
> > I don't disagree. I wanted to have it since there is no sensible way of warning
> > users that this part of the code needs to be updated in the future.
> 
> I understand, however I'm worried that the programming sequence or register
> may change for higher speeds in a way that would be incompatible with what
> we assume here
> 

Honestly, I don't know why you are having this opinion. This piece of code is
not in Qcom driver and the registers are the same for 8 GT/s, 16 GT/s as per the
PCIe spec. So the hardware programming sequence and other arguments doesn't
apply here (atleast to me).

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v8 4/4] PCI: dwc: Add support for configuring lane equalization presets

[Konrad Dybcio]

On 4/2/25 8:02 AM, Manivannan Sadhasivam wrote:
> On Sat, Mar 29, 2025 at 12:42:02PM +0100, Konrad Dybcio wrote:
>> On 3/29/25 10:39 AM, Manivannan Sadhasivam wrote:
>>> On Sat, Mar 29, 2025 at 09:59:46AM +0100, Konrad Dybcio wrote:
>>>> On 3/29/25 7:30 AM, Manivannan Sadhasivam wrote:
>>>>> On Fri, Mar 28, 2025 at 10:53:19PM +0100, Konrad Dybcio wrote:
>>>>>> On 3/28/25 7:45 AM, Manivannan Sadhasivam wrote:
>>>>>>> On Fri, Mar 28, 2025 at 11:04:11AM +0530, Krishna Chaitanya Chundru wrote:

[...]

>> Ohh, I didn't think about that - and I can only think about solutions that are
>> rather janky.. with perhaps the least janky one being changing the else case I
>> proposed above into:
>>
>> else if (speed >= PCIE_SPEED_32_0GT && eq_presets_Ngts[speed - PCIE_SPEED_16_0GT][0] != PCI_EQ_RESV) {
> 
> s/PCIE_SPEED_16_0GT/PCIE_SPEED_32_0GT
> 
>> 	...
> 
> So this I read as: Oh, your controller supports 32 GT/s and you firmware also
> wanted to apply the custom preset offsets, but sorry we didn't do it because we
> don't know if it would work or not. So please let us know so that we can work
> with you test it and then finally we can apply the presets.

Good, because that was exactly what I had in mind :)

>>>>> I'm not forseeing any issue in this part of the code to support higher GEN
>>>>> speeds though.
>>>>
>>>> I would hope so as well, but both not programming and misprogramming are
>>>> equally hard to detect
>>>>
>>>
>>> I don't disagree. I wanted to have it since there is no sensible way of warning
>>> users that this part of the code needs to be updated in the future.
>>
>> I understand, however I'm worried that the programming sequence or register
>> may change for higher speeds in a way that would be incompatible with what
>> we assume here
>>
> 
> Honestly, I don't know why you are having this opinion. This piece of code is
> not in Qcom driver and the registers are the same for 8 GT/s, 16 GT/s as per the
> PCIe spec. So the hardware programming sequence and other arguments doesn't
> apply here (atleast to me).

I'm not familiar with the spec, but if you think it's a good idea to extend
the sequence for 32+ GT/s, I won't object anymore

Konrad