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

[PATCH v3 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 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 API to retrieve equalization presets from device tree
      PCI: dwc: Improve handling of PCIe lane configuration
      PCI: dwc: Add support for configuring lane equalization presets

 arch/arm64/boot/dts/qcom/x1e80100.dtsi            |  8 ++++
 drivers/pci/controller/dwc/pcie-designware-host.c | 42 +++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.c      | 14 ++++++-
 drivers/pci/controller/dwc/pcie-designware.h      |  4 ++
 drivers/pci/of.c                                  | 45 +++++++++++++++++++++++
 drivers/pci/pci.h                                 | 17 ++++++++-
 include/uapi/linux/pci_regs.h                     |  3 ++
 7 files changed, 130 insertions(+), 3 deletions(-)
---
base-commit: 87d6aab2389e5ce0197d8257d5f8ee965a67c4cd
change-id: 20241212-preset_v2-549b7acda9b7

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

[PATCH v3 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>
---
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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index a36076e3c56b..6a2074297030 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -2993,6 +2993,10 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			phys = <&pcie6a_phy>;
 			phy-names = "pciephy";
 
+			eq-presets-8gts = /bits/ 16 <0x5555 0x5555>;
+
+			eq-presets-16gts = /bits/ 8 <0x55 0x55>;
+
 			status = "disabled";
 		};
 
@@ -3115,6 +3119,8 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			phys = <&pcie5_phy>;
 			phy-names = "pciephy";
 
+			eq-presets-8gts = /bits/ 16 <0x5555 0x5555>;
+
 			status = "disabled";
 		};
 
@@ -3235,6 +3241,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 v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[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, this function
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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h | 17 +++++++++++++++--
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index dacea3fc5128..99e0e7ae12e9 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -826,3 +826,48 @@ 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);
+
+int of_pci_get_equalization_presets(struct device *dev,
+				    struct pci_eq_presets *presets,
+				    int num_lanes)
+{
+	char name[20];
+	void **preset;
+	void *temp;
+	int ret;
+
+	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
+		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
+		if (!presets->eq_presets_8gts)
+			return -ENOMEM;
+
+		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
+						 presets->eq_presets_8gts, num_lanes);
+		if (ret) {
+			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
+			return ret;
+		}
+	}
+
+	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
+		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
+		if (of_property_present(dev->of_node, name)) {
+			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
+			if (!temp)
+				return -ENOMEM;
+
+			ret = of_property_read_u8_array(dev->of_node, name,
+							temp, num_lanes);
+			if (ret) {
+				dev_err(dev, "Error %s %d\n", name, ret);
+				return ret;
+			}
+
+			preset = (void **)((u8 *)presets + i * sizeof(void *));
+			*preset = temp;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 14d00ce45bfa..82362d58bedc 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
 }
 
 struct device_node;
-
+struct pci_eq_presets {
+	void *eq_presets_8gts;
+	void *eq_presets_16gts;
+	void *eq_presets_32gts;
+	void *eq_presets_64gts;
+};
 #ifdef CONFIG_OF
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 int of_get_pci_domain_nr(struct device_node *node);
@@ -746,7 +751,9 @@ void pci_set_bus_of_node(struct pci_bus *bus);
 void pci_release_bus_of_node(struct pci_bus *bus);
 
 int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
-
+int of_pci_get_equalization_presets(struct device *dev,
+				    struct pci_eq_presets *presets,
+				    int num_lanes);
 #else
 static inline int
 of_pci_parse_bus_range(struct device_node *node, struct resource *res)
@@ -793,6 +800,12 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
 	return 0;
 }
 
+static inline int of_pci_get_equalization_presets(struct device *dev,
+						  struct pci_eq_presets *presets,
+						  int num_lanes)
+{
+	return 0;
+}
 #endif /* CONFIG_OF */
 
 struct of_changeset;

-- 
2.34.1

[PATCH v3 3/4] PCI: dwc: Improve handling of PCIe lane configuration

[Krishna Chaitanya Chundru]

Currently even if the number of lanes hardware supports is equal to
the number lanes provided in the devicetree, the driver is trying to
configure again the maximum number of lanes which is not needed.

Update number of lanes only when it is not equal to hardware capability.

And also if the num-lanes property is not present in the devicetree
update the num_lanes with the maximum hardware supports.

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      | 14 +++++++++++++-
 drivers/pci/controller/dwc/pcie-designware.h      |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 3e41865c7290..2cd0acbf9e18 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 6d6cbc8b5b2c..acb2a963ae1a 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -736,6 +736,16 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci)
 
 }
 
+int dw_pcie_link_get_max_link_width(struct dw_pcie *pci)
+{
+	u32 lnkcap;
+	u8 cap;
+
+	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	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;
@@ -1069,6 +1079,7 @@ void dw_pcie_edma_remove(struct dw_pcie *pci)
 
 void dw_pcie_setup(struct dw_pcie *pci)
 {
+	int num_lanes = dw_pcie_link_get_max_link_width(pci);
 	u32 val;
 
 	dw_pcie_link_set_max_speed(pci);
@@ -1102,5 +1113,6 @@ void dw_pcie_setup(struct dw_pcie *pci)
 	val |= PORT_LINK_DLL_LINK_EN;
 	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
 
-	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
+	if (num_lanes != pci->num_lanes)
+		dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
 }
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 347ab74ac35a..500e793c9361 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -486,6 +486,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 v3 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, read the
devicetree property and write it 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 | 39 +++++++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h      |  3 ++
 include/uapi/linux/pci_regs.h                     |  3 ++
 3 files changed, 45 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 2cd0acbf9e18..22d3d350918e 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
@@ -802,6 +806,40 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 	return 0;
 }
 
+static void dw_pcie_program_presets(struct dw_pcie *pci, u8 cap_id, u8 lane_eq_offset,
+				    u8 lane_reg_size, u8 *presets, u8 num_lanes)
+{
+	u32 cap;
+	int i;
+
+	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 < 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];
+
+	/* For data rate of 8 GT/S each lane equalization control is 16bits wide */
+	if (speed >= PCIE_SPEED_8_0GT && pp->presets.eq_presets_8gts)
+		dw_pcie_program_presets(pci, PCI_EXT_CAP_ID_SECPCI, PCI_SECPCI_LE_CTRL,
+					0x2, pp->presets.eq_presets_8gts, pci->num_lanes);
+
+	/* For data rate of 16 GT/S each lane equalization control is 8bits wide */
+	if (speed >= PCIE_SPEED_16_0GT && pp->presets.eq_presets_16gts)
+		dw_pcie_program_presets(pci, PCI_EXT_CAP_ID_PL_16GT, PCI_PL_16GT_LE_CTRL,
+					0x1, pp->presets.eq_presets_16gts, pci->num_lanes);
+}
+
 int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -855,6 +893,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 500e793c9361..b12b33944df4 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
@@ -379,6 +381,7 @@ struct dw_pcie_rp {
 	bool			use_atu_msg;
 	int			msg_atu_index;
 	struct resource		*msg_res;
+	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 12323b3334a9..68fc8873bc60 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1118,6 +1118,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 v3 1/4] arm64: dts: qcom: x1e80100: Add PCIe lane equalization preset properties

[Konrad Dybcio]

On 23.12.2024 7:51 AM, Krishna Chaitanya Chundru wrote:
> 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>
> ---

Does this also apply to PCIe3?

Konrad

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Dmitry Baryshkov]

On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
> 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h | 17 +++++++++++++++--
>  2 files changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index dacea3fc5128..99e0e7ae12e9 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -826,3 +826,48 @@ 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);
> +

kerneldoc? Define who should free the memory and how.

> +int of_pci_get_equalization_presets(struct device *dev,
> +				    struct pci_eq_presets *presets,
> +				    int num_lanes)
> +{
> +	char name[20];
> +	void **preset;
> +	void *temp;
> +	int ret;
> +
> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> +		if (!presets->eq_presets_8gts)
> +			return -ENOMEM;
> +
> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> +						 presets->eq_presets_8gts, num_lanes);
> +		if (ret) {
> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> +		if (of_property_present(dev->of_node, name)) {
> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> +			if (!temp)
> +				return -ENOMEM;
> +
> +			ret = of_property_read_u8_array(dev->of_node, name,
> +							temp, num_lanes);
> +			if (ret) {
> +				dev_err(dev, "Error %s %d\n", name, ret);
> +				return ret;
> +			}
> +
> +			preset = (void **)((u8 *)presets + i * sizeof(void *));

Ugh.

> +			*preset = temp;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 14d00ce45bfa..82362d58bedc 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>  }
>  
>  struct device_node;
> -
> +struct pci_eq_presets {
> +	void *eq_presets_8gts;
> +	void *eq_presets_16gts;
> +	void *eq_presets_32gts;
> +	void *eq_presets_64gts;

Why are all of those void*? 8gts is u16*, all other are u8*.

> +};

Empty lines before and after the struct definition.

>  #ifdef CONFIG_OF
>  int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
>  int of_get_pci_domain_nr(struct device_node *node);
> @@ -746,7 +751,9 @@ void pci_set_bus_of_node(struct pci_bus *bus);
>  void pci_release_bus_of_node(struct pci_bus *bus);
>  
>  int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
> -
> +int of_pci_get_equalization_presets(struct device *dev,
> +				    struct pci_eq_presets *presets,
> +				    int num_lanes);

Keep the empty line.

>  #else
>  static inline int
>  of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> @@ -793,6 +800,12 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>  	return 0;
>  }
>  
> +static inline int of_pci_get_equalization_presets(struct device *dev,
> +						  struct pci_eq_presets *presets,
> +						  int num_lanes)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_OF */
>  
>  struct of_changeset;
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Krishna Chaitanya Chundru]

On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
>> 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/pci/pci.h | 17 +++++++++++++++--
>>   2 files changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index dacea3fc5128..99e0e7ae12e9 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -826,3 +826,48 @@ 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);
>> +
> 
> kerneldoc? Define who should free the memory and how.
> 
I will update this in next series.
as we are allocating using devm_kzalloc it should be freed on driver
detach, as no special freeing is required.
>> +int of_pci_get_equalization_presets(struct device *dev,
>> +				    struct pci_eq_presets *presets,
>> +				    int num_lanes)
>> +{
>> +	char name[20];
>> +	void **preset;
>> +	void *temp;
>> +	int ret;
>> +
>> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>> +		if (!presets->eq_presets_8gts)
>> +			return -ENOMEM;
>> +
>> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>> +						 presets->eq_presets_8gts, num_lanes);
>> +		if (ret) {
>> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>> +		if (of_property_present(dev->of_node, name)) {
>> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>> +			if (!temp)
>> +				return -ENOMEM;
>> +
>> +			ret = of_property_read_u8_array(dev->of_node, name,
>> +							temp, num_lanes);
>> +			if (ret) {
>> +				dev_err(dev, "Error %s %d\n", name, ret);
>> +				return ret;
>> +			}
>> +
>> +			preset = (void **)((u8 *)presets + i * sizeof(void *));
> 
> Ugh.
> 
I was trying iterate over each element on the structure as presets holds 
the starting address of the structure and to that we are adding size of 
the void * point to go to each element. I did this way to reduce the
redundant code to read all the gts which has same way of storing the 
data from the device tree. I will add comments here in the next series.
>> +			*preset = temp;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 14d00ce45bfa..82362d58bedc 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>   }
>>   
>>   struct device_node;
>> -
>> +struct pci_eq_presets {
>> +	void *eq_presets_8gts;
>> +	void *eq_presets_16gts;
>> +	void *eq_presets_32gts;
>> +	void *eq_presets_64gts;
> 
> Why are all of those void*? 8gts is u16*, all other are u8*.
> 
To have common parsing logic I moved them to void*, as these are 
pointers actual memory is allocated by of_pci_get_equalization_presets()
based upon the gts these should not give any issues.
>> +};
> 
> Empty lines before and after the struct definition.
> 
ack.

- Krishna Chaitanya.
>>   #ifdef CONFIG_OF
>>   int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
>>   int of_get_pci_domain_nr(struct device_node *node);
>> @@ -746,7 +751,9 @@ void pci_set_bus_of_node(struct pci_bus *bus);
>>   void pci_release_bus_of_node(struct pci_bus *bus);
>>   
>>   int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
>> -
>> +int of_pci_get_equalization_presets(struct device *dev,
>> +				    struct pci_eq_presets *presets,
>> +				    int num_lanes);
> 
> Keep the empty line.
> 
>>   #else
>>   static inline int
>>   of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>> @@ -793,6 +800,12 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>>   	return 0;
>>   }
>>   
>> +static inline int of_pci_get_equalization_presets(struct device *dev,
>> +						  struct pci_eq_presets *presets,
>> +						  int num_lanes)
>> +{
>> +	return 0;
>> +}
>>   #endif /* CONFIG_OF */
>>   
>>   struct of_changeset;
>>
>> -- 
>> 2.34.1
>>
> 

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

[Krishna Chaitanya Chundru]

On 12/23/2024 5:06 PM, Konrad Dybcio wrote:
> On 23.12.2024 7:51 AM, Krishna Chaitanya Chundru wrote:
>> 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>
>> ---
> 
> Does this also apply to PCIe3?
> 
> Konrad
>It will be applicable to PCIe3 also, I will update this in next patch

- Krishna Chaitanya.

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Dmitry Baryshkov]

On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> > On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
> > > 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > >   drivers/pci/pci.h | 17 +++++++++++++++--
> > >   2 files changed, 60 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > index dacea3fc5128..99e0e7ae12e9 100644
> > > --- a/drivers/pci/of.c
> > > +++ b/drivers/pci/of.c
> > > @@ -826,3 +826,48 @@ 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);
> > > +
> > 
> > kerneldoc? Define who should free the memory and how.
> > 
> I will update this in next series.
> as we are allocating using devm_kzalloc it should be freed on driver
> detach, as no special freeing is required.
> > > +int of_pci_get_equalization_presets(struct device *dev,
> > > +				    struct pci_eq_presets *presets,
> > > +				    int num_lanes)
> > > +{
> > > +	char name[20];
> > > +	void **preset;
> > > +	void *temp;
> > > +	int ret;
> > > +
> > > +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> > > +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> > > +		if (!presets->eq_presets_8gts)
> > > +			return -ENOMEM;
> > > +
> > > +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> > > +						 presets->eq_presets_8gts, num_lanes);
> > > +		if (ret) {
> > > +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> > > +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> > > +		if (of_property_present(dev->of_node, name)) {
> > > +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> > > +			if (!temp)
> > > +				return -ENOMEM;
> > > +
> > > +			ret = of_property_read_u8_array(dev->of_node, name,
> > > +							temp, num_lanes);
> > > +			if (ret) {
> > > +				dev_err(dev, "Error %s %d\n", name, ret);
> > > +				return ret;
> > > +			}
> > > +
> > > +			preset = (void **)((u8 *)presets + i * sizeof(void *));
> > 
> > Ugh.
> > 
> I was trying iterate over each element on the structure as presets holds the
> starting address of the structure and to that we are adding size of the void
> * point to go to each element. I did this way to reduce the
> redundant code to read all the gts which has same way of storing the data
> from the device tree. I will add comments here in the next series.

Please rewrite this in a cleaner way. The code shouldn't raise
questions.

> > > +			*preset = temp;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 14d00ce45bfa..82362d58bedc 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
> > >   }
> > >   struct device_node;
> > > -
> > > +struct pci_eq_presets {
> > > +	void *eq_presets_8gts;
> > > +	void *eq_presets_16gts;
> > > +	void *eq_presets_32gts;
> > > +	void *eq_presets_64gts;
> > 
> > Why are all of those void*? 8gts is u16*, all other are u8*.
> > 
> To have common parsing logic I moved them to void*, as these are pointers
> actual memory is allocated by of_pci_get_equalization_presets()
> based upon the gts these should not give any issues.

Please, don't. They have types. void pointers are for the opaque data.

> > > +};
> > 
> > Empty lines before and after the struct definition.
> > 
> ack.
> 
> - Krishna Chaitanya.
> > >   #ifdef CONFIG_OF
> > >   int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
> > >   int of_get_pci_domain_nr(struct device_node *node);
> > > @@ -746,7 +751,9 @@ void pci_set_bus_of_node(struct pci_bus *bus);
> > >   void pci_release_bus_of_node(struct pci_bus *bus);
> > >   int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
> > > -
> > > +int of_pci_get_equalization_presets(struct device *dev,
> > > +				    struct pci_eq_presets *presets,
> > > +				    int num_lanes);
> > 
> > Keep the empty line.
> > 
> > >   #else
> > >   static inline int
> > >   of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> > > @@ -793,6 +800,12 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
> > >   	return 0;
> > >   }
> > > +static inline int of_pci_get_equalization_presets(struct device *dev,
> > > +						  struct pci_eq_presets *presets,
> > > +						  int num_lanes)
> > > +{
> > > +	return 0;
> > > +}
> > >   #endif /* CONFIG_OF */
> > >   struct of_changeset;
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Krishna Chaitanya Chundru]

On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
>>>> 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>    drivers/pci/pci.h | 17 +++++++++++++++--
>>>>    2 files changed, 60 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>> --- a/drivers/pci/of.c
>>>> +++ b/drivers/pci/of.c
>>>> @@ -826,3 +826,48 @@ 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);
>>>> +
>>>
>>> kerneldoc? Define who should free the memory and how.
>>>
>> I will update this in next series.
>> as we are allocating using devm_kzalloc it should be freed on driver
>> detach, as no special freeing is required.
>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>> +				    struct pci_eq_presets *presets,
>>>> +				    int num_lanes)
>>>> +{
>>>> +	char name[20];
>>>> +	void **preset;
>>>> +	void *temp;
>>>> +	int ret;
>>>> +
>>>> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>> +		if (!presets->eq_presets_8gts)
>>>> +			return -ENOMEM;
>>>> +
>>>> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>> +						 presets->eq_presets_8gts, num_lanes);
>>>> +		if (ret) {
>>>> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>> +			return ret;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>> +		if (of_property_present(dev->of_node, name)) {
>>>> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>> +			if (!temp)
>>>> +				return -ENOMEM;
>>>> +
>>>> +			ret = of_property_read_u8_array(dev->of_node, name,
>>>> +							temp, num_lanes);
>>>> +			if (ret) {
>>>> +				dev_err(dev, "Error %s %d\n", name, ret);
>>>> +				return ret;
>>>> +			}
>>>> +
>>>> +			preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>
>>> Ugh.
>>>
>> I was trying iterate over each element on the structure as presets holds the
>> starting address of the structure and to that we are adding size of the void
>> * point to go to each element. I did this way to reduce the
>> redundant code to read all the gts which has same way of storing the data
>> from the device tree. I will add comments here in the next series.
> 
> Please rewrite this in a cleaner way. The code shouldn't raise
> questions.
> 
>>>> +			*preset = temp;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>    }
>>>>    struct device_node;
>>>> -
>>>> +struct pci_eq_presets {
>>>> +	void *eq_presets_8gts;
>>>> +	void *eq_presets_16gts;
>>>> +	void *eq_presets_32gts;
>>>> +	void *eq_presets_64gts;
>>>
>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>
>> To have common parsing logic I moved them to void*, as these are pointers
>> actual memory is allocated by of_pci_get_equalization_presets()
>> based upon the gts these should not give any issues.
> 
> Please, don't. They have types. void pointers are for the opaque data.
> 
ok.

I think then better to use v1 patch 
https://lore.kernel.org/all/20241116-presets-v1-2-878a837a4fee@quicinc.com/

konrad, any objection on using v1 as that will be cleaner way even if we
have some repetitive code.

- Krishna Chaitanya.

>>>> +};
>>>
>>> Empty lines before and after the struct definition.
>>>
>> ack.
>>
>> - Krishna Chaitanya.
>>>>    #ifdef CONFIG_OF
>>>>    int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
>>>>    int of_get_pci_domain_nr(struct device_node *node);
>>>> @@ -746,7 +751,9 @@ void pci_set_bus_of_node(struct pci_bus *bus);
>>>>    void pci_release_bus_of_node(struct pci_bus *bus);
>>>>    int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge);
>>>> -
>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>> +				    struct pci_eq_presets *presets,
>>>> +				    int num_lanes);
>>>
>>> Keep the empty line.
>>>
>>>>    #else
>>>>    static inline int
>>>>    of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>>>> @@ -793,6 +800,12 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
>>>>    	return 0;
>>>>    }
>>>> +static inline int of_pci_get_equalization_presets(struct device *dev,
>>>> +						  struct pci_eq_presets *presets,
>>>> +						  int num_lanes)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>>    #endif /* CONFIG_OF */
>>>>    struct of_changeset;
>>>>
>>>> -- 
>>>> 2.34.1
>>>>
>>>
> 

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Dmitry Baryshkov]

On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
> > On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> > > > On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
> > > > > 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > > > >    drivers/pci/pci.h | 17 +++++++++++++++--
> > > > >    2 files changed, 60 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > > index dacea3fc5128..99e0e7ae12e9 100644
> > > > > --- a/drivers/pci/of.c
> > > > > +++ b/drivers/pci/of.c
> > > > > @@ -826,3 +826,48 @@ 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);
> > > > > +
> > > > 
> > > > kerneldoc? Define who should free the memory and how.
> > > > 
> > > I will update this in next series.
> > > as we are allocating using devm_kzalloc it should be freed on driver
> > > detach, as no special freeing is required.
> > > > > +int of_pci_get_equalization_presets(struct device *dev,
> > > > > +				    struct pci_eq_presets *presets,
> > > > > +				    int num_lanes)
> > > > > +{
> > > > > +	char name[20];
> > > > > +	void **preset;
> > > > > +	void *temp;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> > > > > +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> > > > > +		if (!presets->eq_presets_8gts)
> > > > > +			return -ENOMEM;
> > > > > +
> > > > > +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> > > > > +						 presets->eq_presets_8gts, num_lanes);
> > > > > +		if (ret) {
> > > > > +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> > > > > +			return ret;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> > > > > +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> > > > > +		if (of_property_present(dev->of_node, name)) {
> > > > > +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> > > > > +			if (!temp)
> > > > > +				return -ENOMEM;
> > > > > +
> > > > > +			ret = of_property_read_u8_array(dev->of_node, name,
> > > > > +							temp, num_lanes);
> > > > > +			if (ret) {
> > > > > +				dev_err(dev, "Error %s %d\n", name, ret);
> > > > > +				return ret;
> > > > > +			}
> > > > > +
> > > > > +			preset = (void **)((u8 *)presets + i * sizeof(void *));
> > > > 
> > > > Ugh.
> > > > 
> > > I was trying iterate over each element on the structure as presets holds the
> > > starting address of the structure and to that we are adding size of the void
> > > * point to go to each element. I did this way to reduce the
> > > redundant code to read all the gts which has same way of storing the data
> > > from the device tree. I will add comments here in the next series.
> > 
> > Please rewrite this in a cleaner way. The code shouldn't raise
> > questions.
> > 
> > > > > +			*preset = temp;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > > index 14d00ce45bfa..82362d58bedc 100644
> > > > > --- a/drivers/pci/pci.h
> > > > > +++ b/drivers/pci/pci.h
> > > > > @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
> > > > >    }
> > > > >    struct device_node;
> > > > > -
> > > > > +struct pci_eq_presets {
> > > > > +	void *eq_presets_8gts;
> > > > > +	void *eq_presets_16gts;
> > > > > +	void *eq_presets_32gts;
> > > > > +	void *eq_presets_64gts;
> > > > 
> > > > Why are all of those void*? 8gts is u16*, all other are u8*.
> > > > 
> > > To have common parsing logic I moved them to void*, as these are pointers
> > > actual memory is allocated by of_pci_get_equalization_presets()
> > > based upon the gts these should not give any issues.
> > 
> > Please, don't. They have types. void pointers are for the opaque data.
> > 
> ok.
> 
> I think then better to use v1 patch
> https://lore.kernel.org/all/20241116-presets-v1-2-878a837a4fee@quicinc.com/
> 
> konrad, any objection on using v1 as that will be cleaner way even if we
> have some repetitive code.

Konrad had a nice suggestion about using the array of values. Please use
such an array for 16gts and above. This removes most of repetitive code.


-- 
With best wishes
Dmitry

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Krishna Chaitanya Chundru]

On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
>>>>>> 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>     2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>> --- a/drivers/pci/of.c
>>>>>> +++ b/drivers/pci/of.c
>>>>>> @@ -826,3 +826,48 @@ 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);
>>>>>> +
>>>>>
>>>>> kerneldoc? Define who should free the memory and how.
>>>>>
>>>> I will update this in next series.
>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>> detach, as no special freeing is required.
>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>> +				    struct pci_eq_presets *presets,
>>>>>> +				    int num_lanes)
>>>>>> +{
>>>>>> +	char name[20];
>>>>>> +	void **preset;
>>>>>> +	void *temp;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>> +		if (!presets->eq_presets_8gts)
>>>>>> +			return -ENOMEM;
>>>>>> +
>>>>>> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>> +						 presets->eq_presets_8gts, num_lanes);
>>>>>> +		if (ret) {
>>>>>> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>> +			return ret;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>> +		if (of_property_present(dev->of_node, name)) {
>>>>>> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>> +			if (!temp)
>>>>>> +				return -ENOMEM;
>>>>>> +
>>>>>> +			ret = of_property_read_u8_array(dev->of_node, name,
>>>>>> +							temp, num_lanes);
>>>>>> +			if (ret) {
>>>>>> +				dev_err(dev, "Error %s %d\n", name, ret);
>>>>>> +				return ret;
>>>>>> +			}
>>>>>> +
>>>>>> +			preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>
>>>>> Ugh.
>>>>>
>>>> I was trying iterate over each element on the structure as presets holds the
>>>> starting address of the structure and to that we are adding size of the void
>>>> * point to go to each element. I did this way to reduce the
>>>> redundant code to read all the gts which has same way of storing the data
>>>> from the device tree. I will add comments here in the next series.
>>>
>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>> questions.
>>>
>>>>>> +			*preset = temp;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>> --- a/drivers/pci/pci.h
>>>>>> +++ b/drivers/pci/pci.h
>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>     }
>>>>>>     struct device_node;
>>>>>> -
>>>>>> +struct pci_eq_presets {
>>>>>> +	void *eq_presets_8gts;
>>>>>> +	void *eq_presets_16gts;
>>>>>> +	void *eq_presets_32gts;
>>>>>> +	void *eq_presets_64gts;
>>>>>
>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>
>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>> based upon the gts these should not give any issues.
>>>
>>> Please, don't. They have types. void pointers are for the opaque data.
>>>
>> ok.
>>
>> I think then better to use v1 patch
>> https://lore.kernel.org/all/20241116-presets-v1-2-878a837a4fee@quicinc.com/
>>
>> konrad, any objection on using v1 as that will be cleaner way even if we
>> have some repetitive code.
> 
> Konrad had a nice suggestion about using the array of values. Please use
> such an array for 16gts and above. This removes most of repetitive code.
> 
I don't feel having array in the preset structure looks good, I have
come up with this logic if you feel it is not so good I will go to the
suggested way by having array for 16gts and above.

        if (of_property_present(dev->of_node, "eq-presets-8gts")) {
                 presets->eq_presets_8gts = devm_kzalloc(dev, 
sizeof(u16) * num_lanes, GFP_KERNEL);
                 if (!presets->eq_presets_8gts)
                         return -ENOMEM;

                 ret = of_property_read_u16_array(dev->of_node, 
"eq-presets-8gts",
 
presets->eq_presets_8gts, num_lanes);
                 if (ret) {
                         dev_err(dev, "Error reading eq-presets-8gts 
%d\n", ret);
                         return ret;
                 }
         }

         for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
                 snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
                 if (of_property_present(dev->of_node, name)) {
                         temp = devm_kzalloc(dev, sizeof(u8) * 
num_lanes, GFP_KERNEL);
                         if (!temp)
                                 return -ENOMEM;

                         ret = of_property_read_u8_array(dev->of_node, name,
                                                         temp, num_lanes);
                         if (ret) {
                                 dev_err(dev, "Error %s %d\n", name, ret);
                                 return ret;
                         }

                         switch (i) {
                                 case EQ_PRESET_TYPE_16GTS:
                                         presets->eq_presets_16gts = temp;
                                         break;
                                 case EQ_PRESET_TYPE_32GTS:
                                         presets->eq_presets_32gts = temp;
                                         break;
                                 case EQ_PRESET_TYPE_64GTS:
                                         presets->eq_presets_64gts = temp;
                                         break;
                         }
                 }
         }
- Krishna Chaitanya.

> 

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Dmitry Baryshkov]

On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
> > On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
> > > > On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
> > > > > 
> > > > > 
> > > > > On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> > > > > > On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
> > > > > > > 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >     drivers/pci/pci.h | 17 +++++++++++++++--
> > > > > > >     2 files changed, 60 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > > > > > > index dacea3fc5128..99e0e7ae12e9 100644
> > > > > > > --- a/drivers/pci/of.c
> > > > > > > +++ b/drivers/pci/of.c
> > > > > > > @@ -826,3 +826,48 @@ 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);
> > > > > > > +
> > > > > > 
> > > > > > kerneldoc? Define who should free the memory and how.
> > > > > > 
> > > > > I will update this in next series.
> > > > > as we are allocating using devm_kzalloc it should be freed on driver
> > > > > detach, as no special freeing is required.
> > > > > > > +int of_pci_get_equalization_presets(struct device *dev,
> > > > > > > +				    struct pci_eq_presets *presets,
> > > > > > > +				    int num_lanes)
> > > > > > > +{
> > > > > > > +	char name[20];
> > > > > > > +	void **preset;
> > > > > > > +	void *temp;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> > > > > > > +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> > > > > > > +		if (!presets->eq_presets_8gts)
> > > > > > > +			return -ENOMEM;
> > > > > > > +
> > > > > > > +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> > > > > > > +						 presets->eq_presets_8gts, num_lanes);
> > > > > > > +		if (ret) {
> > > > > > > +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> > > > > > > +			return ret;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> > > > > > > +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> > > > > > > +		if (of_property_present(dev->of_node, name)) {
> > > > > > > +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> > > > > > > +			if (!temp)
> > > > > > > +				return -ENOMEM;
> > > > > > > +
> > > > > > > +			ret = of_property_read_u8_array(dev->of_node, name,
> > > > > > > +							temp, num_lanes);
> > > > > > > +			if (ret) {
> > > > > > > +				dev_err(dev, "Error %s %d\n", name, ret);
> > > > > > > +				return ret;
> > > > > > > +			}
> > > > > > > +
> > > > > > > +			preset = (void **)((u8 *)presets + i * sizeof(void *));
> > > > > > 
> > > > > > Ugh.
> > > > > > 
> > > > > I was trying iterate over each element on the structure as presets holds the
> > > > > starting address of the structure and to that we are adding size of the void
> > > > > * point to go to each element. I did this way to reduce the
> > > > > redundant code to read all the gts which has same way of storing the data
> > > > > from the device tree. I will add comments here in the next series.
> > > > 
> > > > Please rewrite this in a cleaner way. The code shouldn't raise
> > > > questions.
> > > > 
> > > > > > > +			*preset = temp;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> > > > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > > > > index 14d00ce45bfa..82362d58bedc 100644
> > > > > > > --- a/drivers/pci/pci.h
> > > > > > > +++ b/drivers/pci/pci.h
> > > > > > > @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
> > > > > > >     }
> > > > > > >     struct device_node;
> > > > > > > -
> > > > > > > +struct pci_eq_presets {
> > > > > > > +	void *eq_presets_8gts;
> > > > > > > +	void *eq_presets_16gts;
> > > > > > > +	void *eq_presets_32gts;
> > > > > > > +	void *eq_presets_64gts;
> > > > > > 
> > > > > > Why are all of those void*? 8gts is u16*, all other are u8*.
> > > > > > 
> > > > > To have common parsing logic I moved them to void*, as these are pointers
> > > > > actual memory is allocated by of_pci_get_equalization_presets()
> > > > > based upon the gts these should not give any issues.
> > > > 
> > > > Please, don't. They have types. void pointers are for the opaque data.
> > > > 
> > > ok.
> > > 
> > > I think then better to use v1 patch
> > > https://lore.kernel.org/all/20241116-presets-v1-2-878a837a4fee@quicinc.com/
> > > 
> > > konrad, any objection on using v1 as that will be cleaner way even if we
> > > have some repetitive code.
> > 
> > Konrad had a nice suggestion about using the array of values. Please use
> > such an array for 16gts and above. This removes most of repetitive code.
> > 
> I don't feel having array in the preset structure looks good, I have
> come up with this logic if you feel it is not so good I will go to the
> suggested way by having array for 16gts and above.
> 
>        if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>                 presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
> num_lanes, GFP_KERNEL);
>                 if (!presets->eq_presets_8gts)
>                         return -ENOMEM;
> 
>                 ret = of_property_read_u16_array(dev->of_node,
> "eq-presets-8gts",
> 
> presets->eq_presets_8gts, num_lanes);
>                 if (ret) {
>                         dev_err(dev, "Error reading eq-presets-8gts %d\n",
> ret);
>                         return ret;
>                 }
>         }
> 
>         for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>                 snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>                 if (of_property_present(dev->of_node, name)) {
>                         temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
> GFP_KERNEL);
>                         if (!temp)
>                                 return -ENOMEM;
> 
>                         ret = of_property_read_u8_array(dev->of_node, name,
>                                                         temp, num_lanes);
>                         if (ret) {
>                                 dev_err(dev, "Error %s %d\n", name, ret);
>                                 return ret;
>                         }
> 
>                         switch (i) {
>                                 case EQ_PRESET_TYPE_16GTS:
>                                         presets->eq_presets_16gts = temp;
>                                         break;
>                                 case EQ_PRESET_TYPE_32GTS:
>                                         presets->eq_presets_32gts = temp;
>                                         break;
>                                 case EQ_PRESET_TYPE_64GTS:
>                                         presets->eq_presets_64gts = temp;
>                                         break;
>                         }

This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
that.

Also, a strange thought came to my mind: we know that there won't be
more than 16 lanes. Can we have the following structure instead:

#define MAX_LANES 16
enum pcie_gts {
	PCIE_GTS_16GTS,
	PCIE_GTS_32GTS,
	PCIE_GTS_64GTS,
	PCIE_GTS_MAX,
};
struct pci_eq_presets {
	u16 eq_presets_8gts[MAX_LANES];
	u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
};

This should allow you to drop the of_property_present() and
devm_kzalloc(). Just read DT data into a corresponding array.

>                 }
>         }
> - Krishna Chaitanya.
> 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Krishna Chaitanya Chundru]

On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
> On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>>>
>>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
>>>>>>>> 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>      drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>>>      2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>>>> --- a/drivers/pci/of.c
>>>>>>>> +++ b/drivers/pci/of.c
>>>>>>>> @@ -826,3 +826,48 @@ 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);
>>>>>>>> +
>>>>>>>
>>>>>>> kerneldoc? Define who should free the memory and how.
>>>>>>>
>>>>>> I will update this in next series.
>>>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>>>> detach, as no special freeing is required.
>>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>>>> +				    struct pci_eq_presets *presets,
>>>>>>>> +				    int num_lanes)
>>>>>>>> +{
>>>>>>>> +	char name[20];
>>>>>>>> +	void **preset;
>>>>>>>> +	void *temp;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>>> +		presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>>>> +		if (!presets->eq_presets_8gts)
>>>>>>>> +			return -ENOMEM;
>>>>>>>> +
>>>>>>>> +		ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>>>> +						 presets->eq_presets_8gts, num_lanes);
>>>>>>>> +		if (ret) {
>>>>>>>> +			dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>>>> +			return ret;
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>>>> +		snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>>> +		if (of_property_present(dev->of_node, name)) {
>>>>>>>> +			temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>>>> +			if (!temp)
>>>>>>>> +				return -ENOMEM;
>>>>>>>> +
>>>>>>>> +			ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>>> +							temp, num_lanes);
>>>>>>>> +			if (ret) {
>>>>>>>> +				dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>>> +				return ret;
>>>>>>>> +			}
>>>>>>>> +
>>>>>>>> +			preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>>>
>>>>>>> Ugh.
>>>>>>>
>>>>>> I was trying iterate over each element on the structure as presets holds the
>>>>>> starting address of the structure and to that we are adding size of the void
>>>>>> * point to go to each element. I did this way to reduce the
>>>>>> redundant code to read all the gts which has same way of storing the data
>>>>>> from the device tree. I will add comments here in the next series.
>>>>>
>>>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>>>> questions.
>>>>>
>>>>>>>> +			*preset = temp;
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>>>> --- a/drivers/pci/pci.h
>>>>>>>> +++ b/drivers/pci/pci.h
>>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>>>      }
>>>>>>>>      struct device_node;
>>>>>>>> -
>>>>>>>> +struct pci_eq_presets {
>>>>>>>> +	void *eq_presets_8gts;
>>>>>>>> +	void *eq_presets_16gts;
>>>>>>>> +	void *eq_presets_32gts;
>>>>>>>> +	void *eq_presets_64gts;
>>>>>>>
>>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>>>
>>>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>>>> based upon the gts these should not give any issues.
>>>>>
>>>>> Please, don't. They have types. void pointers are for the opaque data.
>>>>>
>>>> ok.
>>>>
>>>> I think then better to use v1 patch
>>>> https://lore.kernel.org/all/20241116-presets-v1-2-878a837a4fee@quicinc.com/
>>>>
>>>> konrad, any objection on using v1 as that will be cleaner way even if we
>>>> have some repetitive code.
>>>
>>> Konrad had a nice suggestion about using the array of values. Please use
>>> such an array for 16gts and above. This removes most of repetitive code.
>>>
>> I don't feel having array in the preset structure looks good, I have
>> come up with this logic if you feel it is not so good I will go to the
>> suggested way by having array for 16gts and above.
>>
>>         if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>                  presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
>> num_lanes, GFP_KERNEL);
>>                  if (!presets->eq_presets_8gts)
>>                          return -ENOMEM;
>>
>>                  ret = of_property_read_u16_array(dev->of_node,
>> "eq-presets-8gts",
>>
>> presets->eq_presets_8gts, num_lanes);
>>                  if (ret) {
>>                          dev_err(dev, "Error reading eq-presets-8gts %d\n",
>> ret);
>>                          return ret;
>>                  }
>>          }
>>
>>          for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>>                  snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>                  if (of_property_present(dev->of_node, name)) {
>>                          temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
>> GFP_KERNEL);
>>                          if (!temp)
>>                                  return -ENOMEM;
>>
>>                          ret = of_property_read_u8_array(dev->of_node, name,
>>                                                          temp, num_lanes);
>>                          if (ret) {
>>                                  dev_err(dev, "Error %s %d\n", name, ret);
>>                                  return ret;
>>                          }
>>
>>                          switch (i) {
>>                                  case EQ_PRESET_TYPE_16GTS:
>>                                          presets->eq_presets_16gts = temp;
>>                                          break;
>>                                  case EQ_PRESET_TYPE_32GTS:
>>                                          presets->eq_presets_32gts = temp;
>>                                          break;
>>                                  case EQ_PRESET_TYPE_64GTS:
>>                                          presets->eq_presets_64gts = temp;
>>                                          break;
>>                          }
> 
> This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
> that.
> 
> Also, a strange thought came to my mind: we know that there won't be
> more than 16 lanes. Can we have the following structure instead:
> 
> #define MAX_LANES 16
> enum pcie_gts {
> 	PCIE_GTS_16GTS,
> 	PCIE_GTS_32GTS,
> 	PCIE_GTS_64GTS,
> 	PCIE_GTS_MAX,
> };
> struct pci_eq_presets {
> 	u16 eq_presets_8gts[MAX_LANES];
> 	u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
> };
> 
> This should allow you to drop the of_property_present() and
> devm_kzalloc(). Just read DT data into a corresponding array.
> 
in the dwc driver patch I was using pointers and memory allocation
to known if the property is present or not. If I use this way I might
end up reading dt property again. I think better to switch to have a
array for above 16gts.

- Krishna Chaitanya.
>>                  }
>>          }
>> - Krishna Chaitanya.
>>
>>>
> 

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Dmitry Baryshkov]

On Tue, 24 Dec 2024 at 12:36, Krishna Chaitanya Chundru
<quic_krichai@quicinc.com> wrote:
>
>
>
> On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
> > On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
> >>
> >>
> >> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
> >>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
> >>>>
> >>>>
> >>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
> >>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
> >>>>>>>> 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>      drivers/pci/pci.h | 17 +++++++++++++++--
> >>>>>>>>      2 files changed, 60 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> >>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
> >>>>>>>> --- a/drivers/pci/of.c
> >>>>>>>> +++ b/drivers/pci/of.c
> >>>>>>>> @@ -826,3 +826,48 @@ 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);
> >>>>>>>> +
> >>>>>>>
> >>>>>>> kerneldoc? Define who should free the memory and how.
> >>>>>>>
> >>>>>> I will update this in next series.
> >>>>>> as we are allocating using devm_kzalloc it should be freed on driver
> >>>>>> detach, as no special freeing is required.
> >>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
> >>>>>>>> +                                  struct pci_eq_presets *presets,
> >>>>>>>> +                                  int num_lanes)
> >>>>>>>> +{
> >>>>>>>> +      char name[20];
> >>>>>>>> +      void **preset;
> >>>>>>>> +      void *temp;
> >>>>>>>> +      int ret;
> >>>>>>>> +
> >>>>>>>> +      if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> >>>>>>>> +              presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
> >>>>>>>> +              if (!presets->eq_presets_8gts)
> >>>>>>>> +                      return -ENOMEM;
> >>>>>>>> +
> >>>>>>>> +              ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
> >>>>>>>> +                                               presets->eq_presets_8gts, num_lanes);
> >>>>>>>> +              if (ret) {
> >>>>>>>> +                      dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
> >>>>>>>> +                      return ret;
> >>>>>>>> +              }
> >>>>>>>> +      }
> >>>>>>>> +
> >>>>>>>> +      for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
> >>>>>>>> +              snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> >>>>>>>> +              if (of_property_present(dev->of_node, name)) {
> >>>>>>>> +                      temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
> >>>>>>>> +                      if (!temp)
> >>>>>>>> +                              return -ENOMEM;
> >>>>>>>> +
> >>>>>>>> +                      ret = of_property_read_u8_array(dev->of_node, name,
> >>>>>>>> +                                                      temp, num_lanes);
> >>>>>>>> +                      if (ret) {
> >>>>>>>> +                              dev_err(dev, "Error %s %d\n", name, ret);
> >>>>>>>> +                              return ret;
> >>>>>>>> +                      }
> >>>>>>>> +
> >>>>>>>> +                      preset = (void **)((u8 *)presets + i * sizeof(void *));
> >>>>>>>
> >>>>>>> Ugh.
> >>>>>>>
> >>>>>> I was trying iterate over each element on the structure as presets holds the
> >>>>>> starting address of the structure and to that we are adding size of the void
> >>>>>> * point to go to each element. I did this way to reduce the
> >>>>>> redundant code to read all the gts which has same way of storing the data
> >>>>>> from the device tree. I will add comments here in the next series.
> >>>>>
> >>>>> Please rewrite this in a cleaner way. The code shouldn't raise
> >>>>> questions.
> >>>>>
> >>>>>>>> +                      *preset = temp;
> >>>>>>>> +              }
> >>>>>>>> +      }
> >>>>>>>> +
> >>>>>>>> +      return 0;
> >>>>>>>> +}
> >>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
> >>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> >>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
> >>>>>>>> --- a/drivers/pci/pci.h
> >>>>>>>> +++ b/drivers/pci/pci.h
> >>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
> >>>>>>>>      }
> >>>>>>>>      struct device_node;
> >>>>>>>> -
> >>>>>>>> +struct pci_eq_presets {
> >>>>>>>> +      void *eq_presets_8gts;
> >>>>>>>> +      void *eq_presets_16gts;
> >>>>>>>> +      void *eq_presets_32gts;
> >>>>>>>> +      void *eq_presets_64gts;
> >>>>>>>
> >>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
> >>>>>>>
> >>>>>> To have common parsing logic I moved them to void*, as these are pointers
> >>>>>> actual memory is allocated by of_pci_get_equalization_presets()
> >>>>>> based upon the gts these should not give any issues.
> >>>>>
> >>>>> Please, don't. They have types. void pointers are for the opaque data.
> >>>>>
> >>>> ok.
> >>>>
> >>>> I think then better to use v1 patch
> >>>> https://lore.kernel.org/all/20241116-presets-v1-2-878a837a4fee@quicinc.com/
> >>>>
> >>>> konrad, any objection on using v1 as that will be cleaner way even if we
> >>>> have some repetitive code.
> >>>
> >>> Konrad had a nice suggestion about using the array of values. Please use
> >>> such an array for 16gts and above. This removes most of repetitive code.
> >>>
> >> I don't feel having array in the preset structure looks good, I have
> >> come up with this logic if you feel it is not so good I will go to the
> >> suggested way by having array for 16gts and above.
> >>
> >>         if (of_property_present(dev->of_node, "eq-presets-8gts")) {
> >>                  presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
> >> num_lanes, GFP_KERNEL);
> >>                  if (!presets->eq_presets_8gts)
> >>                          return -ENOMEM;
> >>
> >>                  ret = of_property_read_u16_array(dev->of_node,
> >> "eq-presets-8gts",
> >>
> >> presets->eq_presets_8gts, num_lanes);
> >>                  if (ret) {
> >>                          dev_err(dev, "Error reading eq-presets-8gts %d\n",
> >> ret);
> >>                          return ret;
> >>                  }
> >>          }
> >>
> >>          for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
> >>                  snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
> >>                  if (of_property_present(dev->of_node, name)) {
> >>                          temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
> >> GFP_KERNEL);
> >>                          if (!temp)
> >>                                  return -ENOMEM;
> >>
> >>                          ret = of_property_read_u8_array(dev->of_node, name,
> >>                                                          temp, num_lanes);
> >>                          if (ret) {
> >>                                  dev_err(dev, "Error %s %d\n", name, ret);
> >>                                  return ret;
> >>                          }
> >>
> >>                          switch (i) {
> >>                                  case EQ_PRESET_TYPE_16GTS:
> >>                                          presets->eq_presets_16gts = temp;
> >>                                          break;
> >>                                  case EQ_PRESET_TYPE_32GTS:
> >>                                          presets->eq_presets_32gts = temp;
> >>                                          break;
> >>                                  case EQ_PRESET_TYPE_64GTS:
> >>                                          presets->eq_presets_64gts = temp;
> >>                                          break;
> >>                          }
> >
> > This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
> > that.
> >
> > Also, a strange thought came to my mind: we know that there won't be
> > more than 16 lanes. Can we have the following structure instead:
> >
> > #define MAX_LANES 16
> > enum pcie_gts {
> >       PCIE_GTS_16GTS,
> >       PCIE_GTS_32GTS,
> >       PCIE_GTS_64GTS,
> >       PCIE_GTS_MAX,
> > };
> > struct pci_eq_presets {
> >       u16 eq_presets_8gts[MAX_LANES];
> >       u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
> > };
> >
> > This should allow you to drop the of_property_present() and
> > devm_kzalloc(). Just read DT data into a corresponding array.
> >
> in the dwc driver patch I was using pointers and memory allocation
> to known if the property is present or not. If I use this way I might
> end up reading dt property again.

Add foo_valid flags to the struct.

>  I think better to switch to have a
> array for above 16gts.

Whichever way works for you.

>
> - Krishna Chaitanya.
> >>                  }
> >>          }
> >> - Krishna Chaitanya.
> >>
> >>>
> >



-- 
With best wishes
Dmitry

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Konrad Dybcio]

On 24.12.2024 11:57 AM, Dmitry Baryshkov wrote:
> On Tue, 24 Dec 2024 at 12:36, Krishna Chaitanya Chundru
> <quic_krichai@quicinc.com> wrote:
>>
>>
>>
>> On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
>>> On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
>>>>
>>>>
>>>> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
>>>>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>>>
>>>>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>>>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
>>>>>>>>>> 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>      drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>>>>>      2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>>>>>> --- a/drivers/pci/of.c
>>>>>>>>>> +++ b/drivers/pci/of.c
>>>>>>>>>> @@ -826,3 +826,48 @@ 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);
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> kerneldoc? Define who should free the memory and how.
>>>>>>>>>
>>>>>>>> I will update this in next series.
>>>>>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>>>>>> detach, as no special freeing is required.
>>>>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>>>>>> +                                  struct pci_eq_presets *presets,
>>>>>>>>>> +                                  int num_lanes)
>>>>>>>>>> +{
>>>>>>>>>> +      char name[20];
>>>>>>>>>> +      void **preset;
>>>>>>>>>> +      void *temp;
>>>>>>>>>> +      int ret;
>>>>>>>>>> +
>>>>>>>>>> +      if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>>>>> +              presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>>>>>> +              if (!presets->eq_presets_8gts)
>>>>>>>>>> +                      return -ENOMEM;
>>>>>>>>>> +
>>>>>>>>>> +              ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>>>>>> +                                               presets->eq_presets_8gts, num_lanes);
>>>>>>>>>> +              if (ret) {
>>>>>>>>>> +                      dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>>>>>> +                      return ret;
>>>>>>>>>> +              }
>>>>>>>>>> +      }
>>>>>>>>>> +
>>>>>>>>>> +      for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>>>>>> +              snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>>>>> +              if (of_property_present(dev->of_node, name)) {
>>>>>>>>>> +                      temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>>>>>> +                      if (!temp)
>>>>>>>>>> +                              return -ENOMEM;
>>>>>>>>>> +
>>>>>>>>>> +                      ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>>>>> +                                                      temp, num_lanes);
>>>>>>>>>> +                      if (ret) {
>>>>>>>>>> +                              dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>>>>> +                              return ret;
>>>>>>>>>> +                      }
>>>>>>>>>> +
>>>>>>>>>> +                      preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>>>>>
>>>>>>>>> Ugh.
>>>>>>>>>
>>>>>>>> I was trying iterate over each element on the structure as presets holds the
>>>>>>>> starting address of the structure and to that we are adding size of the void
>>>>>>>> * point to go to each element. I did this way to reduce the
>>>>>>>> redundant code to read all the gts which has same way of storing the data
>>>>>>>> from the device tree. I will add comments here in the next series.
>>>>>>>
>>>>>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>>>>>> questions.
>>>>>>>
>>>>>>>>>> +                      *preset = temp;
>>>>>>>>>> +              }
>>>>>>>>>> +      }
>>>>>>>>>> +
>>>>>>>>>> +      return 0;
>>>>>>>>>> +}
>>>>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>>>>>> --- a/drivers/pci/pci.h
>>>>>>>>>> +++ b/drivers/pci/pci.h
>>>>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>>>>>      }
>>>>>>>>>>      struct device_node;
>>>>>>>>>> -
>>>>>>>>>> +struct pci_eq_presets {
>>>>>>>>>> +      void *eq_presets_8gts;
>>>>>>>>>> +      void *eq_presets_16gts;
>>>>>>>>>> +      void *eq_presets_32gts;
>>>>>>>>>> +      void *eq_presets_64gts;
>>>>>>>>>
>>>>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>>>>>
>>>>>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>>>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>>>>>> based upon the gts these should not give any issues.
>>>>>>>
>>>>>>> Please, don't. They have types. void pointers are for the opaque data.
>>>>>>>
>>>>>> ok.
>>>>>>
>>>>>> I think then better to use v1 patch
>>>>>> https://lore.kernel.org/all/20241116-presets-v1-2-878a837a4fee@quicinc.com/
>>>>>>
>>>>>> konrad, any objection on using v1 as that will be cleaner way even if we
>>>>>> have some repetitive code.
>>>>>
>>>>> Konrad had a nice suggestion about using the array of values. Please use
>>>>> such an array for 16gts and above. This removes most of repetitive code.
>>>>>
>>>> I don't feel having array in the preset structure looks good, I have
>>>> come up with this logic if you feel it is not so good I will go to the
>>>> suggested way by having array for 16gts and above.
>>>>
>>>>         if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>                  presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
>>>> num_lanes, GFP_KERNEL);
>>>>                  if (!presets->eq_presets_8gts)
>>>>                          return -ENOMEM;
>>>>
>>>>                  ret = of_property_read_u16_array(dev->of_node,
>>>> "eq-presets-8gts",
>>>>
>>>> presets->eq_presets_8gts, num_lanes);
>>>>                  if (ret) {
>>>>                          dev_err(dev, "Error reading eq-presets-8gts %d\n",
>>>> ret);
>>>>                          return ret;
>>>>                  }
>>>>          }
>>>>
>>>>          for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>>>>                  snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>                  if (of_property_present(dev->of_node, name)) {
>>>>                          temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
>>>> GFP_KERNEL);
>>>>                          if (!temp)
>>>>                                  return -ENOMEM;
>>>>
>>>>                          ret = of_property_read_u8_array(dev->of_node, name,
>>>>                                                          temp, num_lanes);
>>>>                          if (ret) {
>>>>                                  dev_err(dev, "Error %s %d\n", name, ret);
>>>>                                  return ret;
>>>>                          }
>>>>
>>>>                          switch (i) {
>>>>                                  case EQ_PRESET_TYPE_16GTS:
>>>>                                          presets->eq_presets_16gts = temp;
>>>>                                          break;
>>>>                                  case EQ_PRESET_TYPE_32GTS:
>>>>                                          presets->eq_presets_32gts = temp;
>>>>                                          break;
>>>>                                  case EQ_PRESET_TYPE_64GTS:
>>>>                                          presets->eq_presets_64gts = temp;
>>>>                                          break;
>>>>                          }
>>>
>>> This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
>>> that.
>>>
>>> Also, a strange thought came to my mind: we know that there won't be
>>> more than 16 lanes. Can we have the following structure instead:
>>>
>>> #define MAX_LANES 16
>>> enum pcie_gts {
>>>       PCIE_GTS_16GTS,
>>>       PCIE_GTS_32GTS,
>>>       PCIE_GTS_64GTS,
>>>       PCIE_GTS_MAX,
>>> };
>>> struct pci_eq_presets {
>>>       u16 eq_presets_8gts[MAX_LANES];
>>>       u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
>>> };
>>>
>>> This should allow you to drop the of_property_present() and
>>> devm_kzalloc(). Just read DT data into a corresponding array.
>>>
>> in the dwc driver patch I was using pointers and memory allocation
>> to known if the property is present or not. If I use this way I might
>> end up reading dt property again.
> 
> Add foo_valid flags to the struct.

Some(u8)/None would be fitting, but we're not there yet :(

Are all 0x00-0xff(ff) values valid for these presets?

>>  I think better to switch to have a
>> array for above 16gts.
> 
> Whichever way works for you.

Sorta-answering the earlier email, I have no concerns either

Konrad

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Krishna Chaitanya Chundru]

On 12/30/2024 7:11 PM, Konrad Dybcio wrote:
> On 24.12.2024 11:57 AM, Dmitry Baryshkov wrote:
>> On Tue, 24 Dec 2024 at 12:36, Krishna Chaitanya Chundru
>> <quic_krichai@quicinc.com> wrote:
>>>
>>>
>>>
>>> On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
>>>> On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>
>>>>>
>>>>> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
>>>>>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
>>>>>>>>>>> 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>       drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>>>>>>       2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>>>>>>> --- a/drivers/pci/of.c
>>>>>>>>>>> +++ b/drivers/pci/of.c
>>>>>>>>>>> @@ -826,3 +826,48 @@ 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);
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> kerneldoc? Define who should free the memory and how.
>>>>>>>>>>
>>>>>>>>> I will update this in next series.
>>>>>>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>>>>>>> detach, as no special freeing is required.
>>>>>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>>>>>>> +                                  struct pci_eq_presets *presets,
>>>>>>>>>>> +                                  int num_lanes)
>>>>>>>>>>> +{
>>>>>>>>>>> +      char name[20];
>>>>>>>>>>> +      void **preset;
>>>>>>>>>>> +      void *temp;
>>>>>>>>>>> +      int ret;
>>>>>>>>>>> +
>>>>>>>>>>> +      if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>>>>>> +              presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>>>>>>> +              if (!presets->eq_presets_8gts)
>>>>>>>>>>> +                      return -ENOMEM;
>>>>>>>>>>> +
>>>>>>>>>>> +              ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>>>>>>> +                                               presets->eq_presets_8gts, num_lanes);
>>>>>>>>>>> +              if (ret) {
>>>>>>>>>>> +                      dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>>>>>>> +                      return ret;
>>>>>>>>>>> +              }
>>>>>>>>>>> +      }
>>>>>>>>>>> +
>>>>>>>>>>> +      for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>>>>>>> +              snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>>>>>> +              if (of_property_present(dev->of_node, name)) {
>>>>>>>>>>> +                      temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>>>>>>> +                      if (!temp)
>>>>>>>>>>> +                              return -ENOMEM;
>>>>>>>>>>> +
>>>>>>>>>>> +                      ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>>>>>> +                                                      temp, num_lanes);
>>>>>>>>>>> +                      if (ret) {
>>>>>>>>>>> +                              dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>>>>>> +                              return ret;
>>>>>>>>>>> +                      }
>>>>>>>>>>> +
>>>>>>>>>>> +                      preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>>>>>>
>>>>>>>>>> Ugh.
>>>>>>>>>>
>>>>>>>>> I was trying iterate over each element on the structure as presets holds the
>>>>>>>>> starting address of the structure and to that we are adding size of the void
>>>>>>>>> * point to go to each element. I did this way to reduce the
>>>>>>>>> redundant code to read all the gts which has same way of storing the data
>>>>>>>>> from the device tree. I will add comments here in the next series.
>>>>>>>>
>>>>>>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>>>>>>> questions.
>>>>>>>>
>>>>>>>>>>> +                      *preset = temp;
>>>>>>>>>>> +              }
>>>>>>>>>>> +      }
>>>>>>>>>>> +
>>>>>>>>>>> +      return 0;
>>>>>>>>>>> +}
>>>>>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>>>>>>> --- a/drivers/pci/pci.h
>>>>>>>>>>> +++ b/drivers/pci/pci.h
>>>>>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>>>>>>       }
>>>>>>>>>>>       struct device_node;
>>>>>>>>>>> -
>>>>>>>>>>> +struct pci_eq_presets {
>>>>>>>>>>> +      void *eq_presets_8gts;
>>>>>>>>>>> +      void *eq_presets_16gts;
>>>>>>>>>>> +      void *eq_presets_32gts;
>>>>>>>>>>> +      void *eq_presets_64gts;
>>>>>>>>>>
>>>>>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>>>>>>
>>>>>>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>>>>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>>>>>>> based upon the gts these should not give any issues.
>>>>>>>>
>>>>>>>> Please, don't. They have types. void pointers are for the opaque data.
>>>>>>>>
>>>>>>> ok.
>>>>>>>
>>>>>>> I think then better to use v1 patch
>>>>>>> https://lore.kernel.org/all/20241116-presets-v1-2-878a837a4fee@quicinc.com/
>>>>>>>
>>>>>>> konrad, any objection on using v1 as that will be cleaner way even if we
>>>>>>> have some repetitive code.
>>>>>>
>>>>>> Konrad had a nice suggestion about using the array of values. Please use
>>>>>> such an array for 16gts and above. This removes most of repetitive code.
>>>>>>
>>>>> I don't feel having array in the preset structure looks good, I have
>>>>> come up with this logic if you feel it is not so good I will go to the
>>>>> suggested way by having array for 16gts and above.
>>>>>
>>>>>          if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>                   presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
>>>>> num_lanes, GFP_KERNEL);
>>>>>                   if (!presets->eq_presets_8gts)
>>>>>                           return -ENOMEM;
>>>>>
>>>>>                   ret = of_property_read_u16_array(dev->of_node,
>>>>> "eq-presets-8gts",
>>>>>
>>>>> presets->eq_presets_8gts, num_lanes);
>>>>>                   if (ret) {
>>>>>                           dev_err(dev, "Error reading eq-presets-8gts %d\n",
>>>>> ret);
>>>>>                           return ret;
>>>>>                   }
>>>>>           }
>>>>>
>>>>>           for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>>>>>                   snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>                   if (of_property_present(dev->of_node, name)) {
>>>>>                           temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
>>>>> GFP_KERNEL);
>>>>>                           if (!temp)
>>>>>                                   return -ENOMEM;
>>>>>
>>>>>                           ret = of_property_read_u8_array(dev->of_node, name,
>>>>>                                                           temp, num_lanes);
>>>>>                           if (ret) {
>>>>>                                   dev_err(dev, "Error %s %d\n", name, ret);
>>>>>                                   return ret;
>>>>>                           }
>>>>>
>>>>>                           switch (i) {
>>>>>                                   case EQ_PRESET_TYPE_16GTS:
>>>>>                                           presets->eq_presets_16gts = temp;
>>>>>                                           break;
>>>>>                                   case EQ_PRESET_TYPE_32GTS:
>>>>>                                           presets->eq_presets_32gts = temp;
>>>>>                                           break;
>>>>>                                   case EQ_PRESET_TYPE_64GTS:
>>>>>                                           presets->eq_presets_64gts = temp;
>>>>>                                           break;
>>>>>                           }
>>>>
>>>> This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
>>>> that.
>>>>
>>>> Also, a strange thought came to my mind: we know that there won't be
>>>> more than 16 lanes. Can we have the following structure instead:
>>>>
>>>> #define MAX_LANES 16
>>>> enum pcie_gts {
>>>>        PCIE_GTS_16GTS,
>>>>        PCIE_GTS_32GTS,
>>>>        PCIE_GTS_64GTS,
>>>>        PCIE_GTS_MAX,
>>>> };
>>>> struct pci_eq_presets {
>>>>        u16 eq_presets_8gts[MAX_LANES];
>>>>        u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
>>>> };
>>>>
>>>> This should allow you to drop the of_property_present() and
>>>> devm_kzalloc(). Just read DT data into a corresponding array.
>>>>
>>> in the dwc driver patch I was using pointers and memory allocation
>>> to known if the property is present or not. If I use this way I might
>>> end up reading dt property again.
>>
>> Add foo_valid flags to the struct.
> 
> Some(u8)/None would be fitting, but we're not there yet :(
> 
> Are all 0x00-0xff(ff) values valid for these presets?
> 
currently 0xff are reserved not sure in future PCIe spec data rates
can use it or not.

- Krishna Chaitanya.
>>>   I think better to switch to have a
>>> array for above 16gts.
>>
>> Whichever way works for you.
> 
> Sorta-answering the earlier email, I have no concerns either
> 
> Konrad

Re: [PATCH v3 2/4] PCI: of: Add API to retrieve equalization presets from device tree

[Konrad Dybcio]

On 31.12.2024 5:38 AM, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/30/2024 7:11 PM, Konrad Dybcio wrote:
>> On 24.12.2024 11:57 AM, Dmitry Baryshkov wrote:
>>> On Tue, 24 Dec 2024 at 12:36, Krishna Chaitanya Chundru
>>> <quic_krichai@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 12/24/2024 3:25 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, Dec 24, 2024 at 02:47:00PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>
>>>>>>
>>>>>> On 12/24/2024 12:00 AM, Dmitry Baryshkov wrote:
>>>>>>> On Mon, Dec 23, 2024 at 10:13:29PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 12/23/2024 8:56 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Mon, Dec 23, 2024 at 08:02:23PM +0530, Krishna Chaitanya Chundru wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 12/23/2024 5:17 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Mon, Dec 23, 2024 at 12:21:15PM +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, this function
>>>>>>>>>>>> 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  | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>       drivers/pci/pci.h | 17 +++++++++++++++--
>>>>>>>>>>>>       2 files changed, 60 insertions(+), 2 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>>>>>>>>>>>> index dacea3fc5128..99e0e7ae12e9 100644
>>>>>>>>>>>> --- a/drivers/pci/of.c
>>>>>>>>>>>> +++ b/drivers/pci/of.c
>>>>>>>>>>>> @@ -826,3 +826,48 @@ 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);
>>>>>>>>>>>> +
>>>>>>>>>>>
>>>>>>>>>>> kerneldoc? Define who should free the memory and how.
>>>>>>>>>>>
>>>>>>>>>> I will update this in next series.
>>>>>>>>>> as we are allocating using devm_kzalloc it should be freed on driver
>>>>>>>>>> detach, as no special freeing is required.
>>>>>>>>>>>> +int of_pci_get_equalization_presets(struct device *dev,
>>>>>>>>>>>> +                                  struct pci_eq_presets *presets,
>>>>>>>>>>>> +                                  int num_lanes)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +      char name[20];
>>>>>>>>>>>> +      void **preset;
>>>>>>>>>>>> +      void *temp;
>>>>>>>>>>>> +      int ret;
>>>>>>>>>>>> +
>>>>>>>>>>>> +      if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>>>>>>> +              presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) * num_lanes, GFP_KERNEL);
>>>>>>>>>>>> +              if (!presets->eq_presets_8gts)
>>>>>>>>>>>> +                      return -ENOMEM;
>>>>>>>>>>>> +
>>>>>>>>>>>> +              ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts",
>>>>>>>>>>>> +                                               presets->eq_presets_8gts, num_lanes);
>>>>>>>>>>>> +              if (ret) {
>>>>>>>>>>>> +                      dev_err(dev, "Error reading eq-presets-8gts %d\n", ret);
>>>>>>>>>>>> +                      return ret;
>>>>>>>>>>>> +              }
>>>>>>>>>>>> +      }
>>>>>>>>>>>> +
>>>>>>>>>>>> +      for (int i = 1; i < sizeof(struct pci_eq_presets) / sizeof(void *); i++) {
>>>>>>>>>>>> +              snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>>>>>>> +              if (of_property_present(dev->of_node, name)) {
>>>>>>>>>>>> +                      temp = devm_kzalloc(dev, sizeof(u8) * num_lanes, GFP_KERNEL);
>>>>>>>>>>>> +                      if (!temp)
>>>>>>>>>>>> +                              return -ENOMEM;
>>>>>>>>>>>> +
>>>>>>>>>>>> +                      ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>>>>>>> +                                                      temp, num_lanes);
>>>>>>>>>>>> +                      if (ret) {
>>>>>>>>>>>> +                              dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>>>>>>> +                              return ret;
>>>>>>>>>>>> +                      }
>>>>>>>>>>>> +
>>>>>>>>>>>> +                      preset = (void **)((u8 *)presets + i * sizeof(void *));
>>>>>>>>>>>
>>>>>>>>>>> Ugh.
>>>>>>>>>>>
>>>>>>>>>> I was trying iterate over each element on the structure as presets holds the
>>>>>>>>>> starting address of the structure and to that we are adding size of the void
>>>>>>>>>> * point to go to each element. I did this way to reduce the
>>>>>>>>>> redundant code to read all the gts which has same way of storing the data
>>>>>>>>>> from the device tree. I will add comments here in the next series.
>>>>>>>>>
>>>>>>>>> Please rewrite this in a cleaner way. The code shouldn't raise
>>>>>>>>> questions.
>>>>>>>>>
>>>>>>>>>>>> +                      *preset = temp;
>>>>>>>>>>>> +              }
>>>>>>>>>>>> +      }
>>>>>>>>>>>> +
>>>>>>>>>>>> +      return 0;
>>>>>>>>>>>> +}
>>>>>>>>>>>> +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets);
>>>>>>>>>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>>>>>>>>>> index 14d00ce45bfa..82362d58bedc 100644
>>>>>>>>>>>> --- a/drivers/pci/pci.h
>>>>>>>>>>>> +++ b/drivers/pci/pci.h
>>>>>>>>>>>> @@ -731,7 +731,12 @@ static inline u64 pci_rebar_size_to_bytes(int size)
>>>>>>>>>>>>       }
>>>>>>>>>>>>       struct device_node;
>>>>>>>>>>>> -
>>>>>>>>>>>> +struct pci_eq_presets {
>>>>>>>>>>>> +      void *eq_presets_8gts;
>>>>>>>>>>>> +      void *eq_presets_16gts;
>>>>>>>>>>>> +      void *eq_presets_32gts;
>>>>>>>>>>>> +      void *eq_presets_64gts;
>>>>>>>>>>>
>>>>>>>>>>> Why are all of those void*? 8gts is u16*, all other are u8*.
>>>>>>>>>>>
>>>>>>>>>> To have common parsing logic I moved them to void*, as these are pointers
>>>>>>>>>> actual memory is allocated by of_pci_get_equalization_presets()
>>>>>>>>>> based upon the gts these should not give any issues.
>>>>>>>>>
>>>>>>>>> Please, don't. They have types. void pointers are for the opaque data.
>>>>>>>>>
>>>>>>>> ok.
>>>>>>>>
>>>>>>>> I think then better to use v1 patch
>>>>>>>> https://lore.kernel.org/all/20241116-presets-v1-2-878a837a4fee@quicinc.com/
>>>>>>>>
>>>>>>>> konrad, any objection on using v1 as that will be cleaner way even if we
>>>>>>>> have some repetitive code.
>>>>>>>
>>>>>>> Konrad had a nice suggestion about using the array of values. Please use
>>>>>>> such an array for 16gts and above. This removes most of repetitive code.
>>>>>>>
>>>>>> I don't feel having array in the preset structure looks good, I have
>>>>>> come up with this logic if you feel it is not so good I will go to the
>>>>>> suggested way by having array for 16gts and above.
>>>>>>
>>>>>>          if (of_property_present(dev->of_node, "eq-presets-8gts")) {
>>>>>>                   presets->eq_presets_8gts = devm_kzalloc(dev, sizeof(u16) *
>>>>>> num_lanes, GFP_KERNEL);
>>>>>>                   if (!presets->eq_presets_8gts)
>>>>>>                           return -ENOMEM;
>>>>>>
>>>>>>                   ret = of_property_read_u16_array(dev->of_node,
>>>>>> "eq-presets-8gts",
>>>>>>
>>>>>> presets->eq_presets_8gts, num_lanes);
>>>>>>                   if (ret) {
>>>>>>                           dev_err(dev, "Error reading eq-presets-8gts %d\n",
>>>>>> ret);
>>>>>>                           return ret;
>>>>>>                   }
>>>>>>           }
>>>>>>
>>>>>>           for (int i = EQ_PRESET_TYPE_16GTS; i < EQ_PRESET_TYPE_64GTS; i++) {
>>>>>>                   snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << i);
>>>>>>                   if (of_property_present(dev->of_node, name)) {
>>>>>>                           temp = devm_kzalloc(dev, sizeof(u8) * num_lanes,
>>>>>> GFP_KERNEL);
>>>>>>                           if (!temp)
>>>>>>                                   return -ENOMEM;
>>>>>>
>>>>>>                           ret = of_property_read_u8_array(dev->of_node, name,
>>>>>>                                                           temp, num_lanes);
>>>>>>                           if (ret) {
>>>>>>                                   dev_err(dev, "Error %s %d\n", name, ret);
>>>>>>                                   return ret;
>>>>>>                           }
>>>>>>
>>>>>>                           switch (i) {
>>>>>>                                   case EQ_PRESET_TYPE_16GTS:
>>>>>>                                           presets->eq_presets_16gts = temp;
>>>>>>                                           break;
>>>>>>                                   case EQ_PRESET_TYPE_32GTS:
>>>>>>                                           presets->eq_presets_32gts = temp;
>>>>>>                                           break;
>>>>>>                                   case EQ_PRESET_TYPE_64GTS:
>>>>>>                                           presets->eq_presets_64gts = temp;
>>>>>>                                           break;
>>>>>>                           }
>>>>>
>>>>> This looks like 'presets->eq_presets[i] = temp;', but I won't insist on
>>>>> that.
>>>>>
>>>>> Also, a strange thought came to my mind: we know that there won't be
>>>>> more than 16 lanes. Can we have the following structure instead:
>>>>>
>>>>> #define MAX_LANES 16
>>>>> enum pcie_gts {
>>>>>        PCIE_GTS_16GTS,
>>>>>        PCIE_GTS_32GTS,
>>>>>        PCIE_GTS_64GTS,
>>>>>        PCIE_GTS_MAX,
>>>>> };
>>>>> struct pci_eq_presets {
>>>>>        u16 eq_presets_8gts[MAX_LANES];
>>>>>        u8 eq_presets_Ngts[PCIE_GTS_MAX][MAX_LANES];
>>>>> };
>>>>>
>>>>> This should allow you to drop the of_property_present() and
>>>>> devm_kzalloc(). Just read DT data into a corresponding array.
>>>>>
>>>> in the dwc driver patch I was using pointers and memory allocation
>>>> to known if the property is present or not. If I use this way I might
>>>> end up reading dt property again.
>>>
>>> Add foo_valid flags to the struct.
>>
>> Some(u8)/None would be fitting, but we're not there yet :(
>>
>> Are all 0x00-0xff(ff) values valid for these presets?
>>
> currently 0xff are reserved not sure in future PCIe spec data rates
> can use it or not.

Maybe we could use it as a #define-d placeholder then, and kick the
can down the road. Opinions?

Konrad