| Message ID | 20250210-preset_v6-v6-0-cbd837d0028d@oss.qualcomm.com |
|---|---|
| Headers | show |
| Series | PCI: dwc: Add support for configuring lane equalization presets | expand |
On Mon, Feb 10, 2025 at 01:00:00PM +0530, 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> > --- > 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 | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > index 4936fa5b98ff..1b815d4eed5c 100644 > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > @@ -3209,6 +3209,11 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, > phys = <&pcie3_phy>; > phy-names = "pciephy"; > > + eq-presets-8gts = /bits/ 16 <0x5555 0x5555 0x5555 0x5555>, > + /bits/ 16 <0x5555 0x5555 0x5555 0x5555>; Why 2 16bit arrays? - Mani
On 2/14/2025 2:14 PM, Manivannan Sadhasivam wrote: > On Mon, Feb 10, 2025 at 01:00:00PM +0530, 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> >> --- >> 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 | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> index 4936fa5b98ff..1b815d4eed5c 100644 >> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >> @@ -3209,6 +3209,11 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, >> phys = <&pcie3_phy>; >> phy-names = "pciephy"; >> >> + eq-presets-8gts = /bits/ 16 <0x5555 0x5555 0x5555 0x5555>, >> + /bits/ 16 <0x5555 0x5555 0x5555 0x5555>; > > Why 2 16bit arrays? > Just to keep line length below 100, if I use single line it is crossing 100 lines. - Krishna Chaitanya. > - Mani >
On Mon, Feb 10, 2025 at 01:00:01PM +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 | 43 +++++++++++++++++++++++++++++++++++++++++++ > drivers/pci/pci.h | 27 ++++++++++++++++++++++++++- > 2 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 7a806f5c0d20..705d5529fa95 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -851,3 +851,46 @@ u32 of_pci_get_slot_power_limit(struct device_node *node, > return slot_power_limit_mw; > } > EXPORT_SYMBOL_GPL(of_pci_get_slot_power_limit); > + > +/** > + * of_pci_get_equalization_presets - Parses the "eq-presets-ngts" property. nit: eq-presets-Ngts > + * > + * @dev: Device containing the properties. > + * @presets: Pointer to store the parsed data. > + * @num_lanes: Maximum number of lanes supported. > + * > + * If the property is present read and store the data in the preset structure > + * assign default value 0xff to indicate property is not present. 'else assign...' > + * > + * Return: 0 if the property is not available or successfully parsed; errno otherwise. > + */ > +int of_pci_get_equalization_presets(struct device *dev, > + struct pci_eq_presets *presets, > + int num_lanes) > +{ > + char name[20]; > + int ret; > + > + presets->eq_presets_8gts[0] = PCI_EQ_RESV; > + ret = of_property_read_u16_array(dev->of_node, "eq-presets-8gts", > + presets->eq_presets_8gts, num_lanes); > + if (ret && ret != -EINVAL) { > + dev_err(dev, "Error reading eq-presets-8gts %d\n", ret); nit: add ': ' before '%d' to make it clear that the printed value is an errno. > + return ret; > + } > + > + for (int i = 0; i < EQ_PRESET_TYPE_MAX; i++) { > + presets->eq_presets_Ngts[i][0] = PCI_EQ_RESV; > + snprintf(name, sizeof(name), "eq-presets-%dgts", 8 << (i + 1)); > + ret = of_property_read_u8_array(dev->of_node, name, > + presets->eq_presets_Ngts[i], > + num_lanes); > + if (ret && ret != -EINVAL) { > + dev_err(dev, "Error reading %s %d\n", name, ret); > + return ret; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(of_pci_get_equalization_presets); > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 01e51db8d285..e87c2ffd1e85 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -9,6 +9,8 @@ struct pcie_tlp_log; > /* Number of possible devfns: 0.0 to 1f.7 inclusive */ > #define MAX_NR_DEVFNS 256 > > +#define MAX_NR_LANES 16 > + > #define PCI_FIND_CAP_TTL 48 > > #define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */ > @@ -808,6 +810,20 @@ static inline u64 pci_rebar_size_to_bytes(int size) > > struct device_node; > > +#define PCI_EQ_RESV 0xff Just a single space is enough after 'define' - Mani
On Mon, Feb 10, 2025 at 01:00:03PM +0530, Krishna Chaitanya Chundru wrote: > PCIe equalization presets are predefined settings used to optimize > signal integrity by compensating for signal loss and distortion in > high-speed data transmission. > > Based upon the number of lanes and the data rate supported, write > the preset data read from the device tree in to the lane equalization > control registers. > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 53 +++++++++++++++++++++++ > drivers/pci/controller/dwc/pcie-designware.h | 3 ++ > include/uapi/linux/pci_regs.h | 3 ++ > 3 files changed, 59 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index dd56cc02f4ef..7d5f16f77e2f 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -507,6 +507,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > if (pci->num_lanes < 1) > pci->num_lanes = dw_pcie_link_get_max_link_width(pci); > > + ret = of_pci_get_equalization_presets(dev, &pp->presets, pci->num_lanes); > + if (ret) > + goto err_free_msi; > + > /* > * Allocate the resource for MSG TLP before programming the iATU > * outbound window in dw_pcie_setup_rc(). Since the allocation depends > @@ -808,6 +812,54 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) > return 0; > } > > +static void dw_pcie_program_presets(struct dw_pcie_rp *pp, enum pci_bus_speed speed) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + u8 lane_eq_offset, lane_reg_size, cap_id; > + u8 *presets; > + u32 cap; > + int i; > + > + if (speed == PCIE_SPEED_8_0GT) { > + presets = (u8 *)pp->presets.eq_presets_8gts; > + lane_eq_offset = PCI_SECPCI_LE_CTRL; > + cap_id = PCI_EXT_CAP_ID_SECPCI; > + /* For data rate of 8 GT/S each lane equalization control is 16bits wide*/ > + lane_reg_size = 0x2; > + } else if (speed == PCIE_SPEED_16_0GT) { > + presets = pp->presets.eq_presets_Ngts[EQ_PRESET_TYPE_16GTS]; > + lane_eq_offset = PCI_PL_16GT_LE_CTRL; > + cap_id = PCI_EXT_CAP_ID_PL_16GT; > + lane_reg_size = 0x1; > + } > + > + if (presets[0] == PCI_EQ_RESV) > + return; > + > + cap = dw_pcie_find_ext_capability(pci, cap_id); > + if (!cap) > + return; > + > + /* > + * Write preset values to the registers byte-by-byte for the given > + * number of lanes and register size. > + */ > + for (i = 0; i < pci->num_lanes * lane_reg_size; i++) > + dw_pcie_writeb_dbi(pci, cap + lane_eq_offset + i, presets[i]); > +} > + > +static void dw_pcie_config_presets(struct dw_pcie_rp *pp) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + enum pci_bus_speed speed = pcie_link_speed[pci->max_link_speed]; > + Please add a comment stating that the equalization needs to be performed at all lower data rates for each lane. > + if (speed >= PCIE_SPEED_8_0GT) > + dw_pcie_program_presets(pp, PCIE_SPEED_8_0GT); > + > + if (speed >= PCIE_SPEED_16_0GT) > + dw_pcie_program_presets(pp, PCIE_SPEED_16_0GT); I think we need to check 'Link Equalization Request' before performing equalization? This will also help us to warn users if they didn't specify the property in DT if hardware expects equalization. Currently, even if DT specifies equalization presets for 32GT/s, driver is not making use of them. - Mani
On 14.02.2025 9:48 AM, Krishna Chaitanya Chundru wrote: > > > On 2/14/2025 2:14 PM, Manivannan Sadhasivam wrote: >> On Mon, Feb 10, 2025 at 01:00:00PM +0530, 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> >>> --- >>> 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 | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >>> index 4936fa5b98ff..1b815d4eed5c 100644 >>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >>> @@ -3209,6 +3209,11 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, >>> phys = <&pcie3_phy>; >>> phy-names = "pciephy"; >>> + eq-presets-8gts = /bits/ 16 <0x5555 0x5555 0x5555 0x5555>, >>> + /bits/ 16 <0x5555 0x5555 0x5555 0x5555>; >> >> Why 2 16bit arrays? >> > Just to keep line length below 100, if I use single line it is crossing > 100 lines. Oh I didn't notice this.. Ideally we would have <A0>, <A1>, ..., <An>; But it seems like /bits/ applies individually to each entry? That's a bit weird, but I'll add it to my list of things I don't like about dtc.. Let's do: eq-presets-8gts = /bits/ 16 <0x5555 0x5555 0x5555 0x5555 0x5555 0x5555 0x5555 0x5555>; for now Konrad
On Fri, Feb 14, 2025 at 02:18:48PM +0530, Krishna Chaitanya Chundru wrote: > > > On 2/14/2025 2:14 PM, Manivannan Sadhasivam wrote: > > On Mon, Feb 10, 2025 at 01:00:00PM +0530, 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> > > > --- > > > 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 | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > > > index 4936fa5b98ff..1b815d4eed5c 100644 > > > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > > > @@ -3209,6 +3209,11 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>, > > > phys = <&pcie3_phy>; > > > phy-names = "pciephy"; > > > + eq-presets-8gts = /bits/ 16 <0x5555 0x5555 0x5555 0x5555>, > > > + /bits/ 16 <0x5555 0x5555 0x5555 0x5555>; > > > > Why 2 16bit arrays? > > > Just to keep line length below 100, if I use single line it is crossing > 100 lines. > You *should* keep it as a single array even if it crosses 100 column width. - Mani
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 v6: - update the dt properties to match the lane width ( mani & konard) - move everything to helper function and let the helper function determine reg size and offset (mani) - update the function header (mani) - move the num_lanes check to the main function (mani) - Link to v5: https://lore.kernel.org/linux-kernel/20250128-preset_v2-v5-0-4d230d956f8c@oss.qualcomm.com/ Changes in v5: - Instead of using of_property_present use return value of of_property_read_u8_array to know about property is present or not and add a macro for reserved value(Konrad). - Link to v4: https://lore.kernel.org/r/20250124-preset_v2-v4-0-0b512cad08e1@oss.qualcomm.com Changes in v4: - use static arrays for storing preset values and use default value 0xff to indicate the property is not present (Dimitry & konrad). - Link to v3: https://lore.kernel.org/r/20241223-preset_v2-v3-0-a339f475caf5@oss.qualcomm.com Changes in v3: - In previous series a wrong patch was attached, correct it - Link to v2: https://lore.kernel.org/r/20241212-preset_v2-v2-0-210430fbcd8a@oss.qualcomm.com Changes in v2: - Fix the kernel test robot error - As suggested by konrad use for loop and read "eq-presets-%ugts", (8 << i) - Link to v1: https://lore.kernel.org/r/20241116-presets-v1-0-878a837a4fee@quicinc.com --- Krishna Chaitanya Chundru (4): arm64: dts: qcom: x1e80100: Add PCIe lane equalization preset properties PCI: of: Add 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 | 13 ++++++ drivers/pci/controller/dwc/pcie-designware-host.c | 56 +++++++++++++++++++++++ drivers/pci/controller/dwc/pcie-designware.c | 11 ++++- drivers/pci/controller/dwc/pcie-designware.h | 4 ++ drivers/pci/of.c | 43 +++++++++++++++++ drivers/pci/pci.h | 27 ++++++++++- include/uapi/linux/pci_regs.h | 3 ++ 7 files changed, 155 insertions(+), 2 deletions(-) --- base-commit: 3175967ecb3266d0ad7d2ca7ccceaf15fa2f15e2 change-id: 20250210-preset_v6-1e7f560d13ad Best regards,