Hello Everyone,
this series adds PHY latency properties to the Cadence PCIe
driver to improve PTM accuracy, and configures the necessary
values for TI's AM64x processors.
These latencies are implementation specific and need to be
configured in the PCIe IP core's registers to allow the
PCIe controller to exactly determine the RX/TX timestamps for
PCIe PTM messages.
TI doesn't document these values in the datasheet or reference
manual as of now, but provided the necessary data via TI's E2E
forums (see PATCH 3/3).
Best Regards,
Dominic
Alexander Bahle (3):
dt-bindings: PCI: cdns: Add PHY latency properties
PCI: cadence: Use DT bindings to set PHY latencies
arm64: dts: ti: k3-am64-main: Add latency DT binding
.../bindings/pci/cdns,cdns-pcie-ep.yaml | 2 +
.../bindings/pci/cdns,cdns-pcie-host.yaml | 2 +
.../devicetree/bindings/pci/cdns-pcie-ep.yaml | 20 +++++
.../bindings/pci/cdns-pcie-host.yaml | 20 +++++
arch/arm64/boot/dts/ti/k3-am64-main.dtsi | 4 +
.../pci/controller/cadence/pcie-cadence-ep.c | 2 +
.../controller/cadence/pcie-cadence-host.c | 1 +
drivers/pci/controller/cadence/pcie-cadence.c | 81 +++++++++++++++++++
drivers/pci/controller/cadence/pcie-cadence.h | 23 ++++++
9 files changed, 155 insertions(+)
base-commit: a185a0995518a3355c8623c95c36aaaae489de10
--
2.36.0
From: Alexander Bahle <[email protected]>
Use optional "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps"
DeviceTree bindings to set the CDNS_PCIE_LM_PTM_LAT_PARAM(_IDX)
register(s) during PCIe host and ep setup.
The properties expect a list of uint32 PHY latencies in picoseconds for
every supported speed starting at PCIe Gen1, e.g.:
max-link-speed = <2>;
tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
There should be a value for every supported speed but it is not enforced or
necessary. A warning is emitted to let users know that the PTM timestamps
from this PCIe device may not be precise enough for some applications.
Signed-off-by: Alexander Bahle <[email protected]>
Signed-off-by: Dominic Rath <[email protected]>
---
.../pci/controller/cadence/pcie-cadence-ep.c | 2 +
.../controller/cadence/pcie-cadence-host.c | 1 +
drivers/pci/controller/cadence/pcie-cadence.c | 81 +++++++++++++++++++
drivers/pci/controller/cadence/pcie-cadence.h | 23 ++++++
4 files changed, 107 insertions(+)
diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index b8b655d4047e..6e39126922d1 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -664,6 +664,8 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
}
pcie->mem_res = res;
+ cdns_pcie_init_ptm_phy_latency(dev, pcie);
+
ep->max_regions = CDNS_PCIE_MAX_OB;
of_property_read_u32(np, "cdns,max-outbound-regions", &ep->max_regions);
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 940c7dd701d6..8933002f828e 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -510,6 +510,7 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
cdns_pcie_host_enable_ptm_response(pcie);
+ cdns_pcie_init_ptm_phy_latency(dev, pcie);
ret = cdns_pcie_start_link(pcie);
if (ret) {
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
index 13c4032ca379..f0370a10640b 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -5,8 +5,89 @@
#include <linux/kernel.h>
+#include "../../pci.h"
#include "pcie-cadence.h"
+void cdns_pcie_set_ptm_phy_latency_param(struct cdns_pcie *pcie, bool rx,
+ u32 speed_index, u32 latency)
+{
+ u32 val;
+
+ /* Set the speed index */
+ val = cdns_pcie_readl(pcie, CDNS_PCIE_LM_PTM_LAT_PARAM_IDX);
+ val = ((val & ~CDNS_PCIE_LM_PTM_LAT_PARAM_IDX_PTMLATIN_MASK) |
+ CDNS_PCIE_LM_PTM_LAT_PARAM_IDX_PTMLATIN(speed_index));
+ cdns_pcie_writel(pcie, CDNS_PCIE_LM_PTM_LAT_PARAM_IDX, val);
+
+ val = cdns_pcie_readl(pcie, CDNS_PCIE_LM_PTM_LAT_PARAM);
+ if (rx) {
+ /* Set the RX direction latency */
+ val = ((val & ~CDNS_PCIE_LM_PTM_LAT_PARAM_PTMRXLAT_MASK) |
+ CDNS_PCIE_LM_PTM_LAT_PARAM_PTMRXLAT(latency));
+ } else {
+ /* Set TX direction latency */
+ val = ((val & ~CDNS_PCIE_LM_PTM_LAT_PARAM_PTMTXLAT_MASK) |
+ CDNS_PCIE_LM_PTM_LAT_PARAM_PTMTXLAT(latency));
+ }
+ cdns_pcie_writel(pcie, CDNS_PCIE_LM_PTM_LAT_PARAM, val);
+}
+
+static int cdns_pcie_set_ptm_phy_latency(struct device *dev, struct cdns_pcie *pcie,
+ bool rx, const char *key)
+{
+ struct device_node *np = dev->of_node;
+ int max_link_speed;
+ int param_count;
+ u32 latency;
+ int i;
+
+ max_link_speed = of_pci_get_max_link_speed(np);
+ if (max_link_speed < 1)
+ return -EINVAL;
+
+ param_count = of_property_count_u32_elems(np, key);
+ if (param_count < 0 || param_count < max_link_speed) {
+ dev_warn(dev,
+ "no %s set for one or more speeds: %d\n",
+ key, param_count);
+ }
+
+ /* Don't set param for unsupported speed */
+ if (param_count > max_link_speed)
+ param_count = max_link_speed;
+
+ for (i = 0; i < param_count; i++) {
+ if (of_property_read_u32_index(np, key, i,
+ &latency) < 0) {
+ dev_err(dev, "failed to set latency for speed %d. %s\n",
+ i, key);
+ return -EINVAL;
+ }
+
+ /* convert ps to ns */
+ latency /= 1000;
+
+ cdns_pcie_set_ptm_phy_latency_param(pcie, rx,
+ i, latency);
+ }
+
+ return 0;
+}
+
+int cdns_pcie_init_ptm_phy_latency(struct device *dev, struct cdns_pcie *pcie)
+{
+ int ret;
+
+ ret = cdns_pcie_set_ptm_phy_latency(dev, pcie, false,
+ "cdns,tx-phy-latency-ps");
+ if (ret)
+ return ret;
+
+ ret = cdns_pcie_set_ptm_phy_latency(dev, pcie, true,
+ "cdns,rx-phy-latency-ps");
+ return ret;
+}
+
void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
{
u32 delay = 0x3;
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 190786e47df9..483b957a8212 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -120,6 +120,26 @@
#define CDNS_PCIE_LM_PTM_CTRL (CDNS_PCIE_LM_BASE + 0x0da8)
#define CDNS_PCIE_LM_TPM_CTRL_PTMRSEN BIT(17)
+/* PTM Latency Parameters Index Register */
+#define CDNS_PCIE_LM_PTM_LAT_PARAM_IDX \
+ (CDNS_PCIE_LM_BASE + 0x0db0)
+#define CDNS_PCIE_LM_PTM_LAT_PARAM_IDX_PTMLATIN_MASK \
+ GENMASK(3, 0)
+#define CDNS_PCIE_LM_PTM_LAT_PARAM_IDX_PTMLATIN(a) \
+ (((a) << 0) & CDNS_PCIE_LM_PTM_LAT_PARAM_IDX_PTMLATIN_MASK)
+
+/* PTM Latency Parameters Register */
+#define CDNS_PCIE_LM_PTM_LAT_PARAM \
+ (CDNS_PCIE_LM_BASE + 0x0db4)
+#define CDNS_PCIE_LM_PTM_LAT_PARAM_PTMTXLAT_MASK \
+ GENMASK(9, 0)
+#define CDNS_PCIE_LM_PTM_LAT_PARAM_PTMTXLAT(a) \
+ (((a) << 0) & CDNS_PCIE_LM_PTM_LAT_PARAM_PTMTXLAT_MASK)
+#define CDNS_PCIE_LM_PTM_LAT_PARAM_PTMRXLAT_MASK \
+ GENMASK(19, 10)
+#define CDNS_PCIE_LM_PTM_LAT_PARAM_PTMRXLAT(b) \
+ (((b) << 10) & CDNS_PCIE_LM_PTM_LAT_PARAM_PTMRXLAT_MASK)
+
/*
* Endpoint Function Registers (PCI configuration space for endpoint functions)
*/
@@ -541,6 +561,9 @@ static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
#endif
void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
+void cdns_pcie_set_ptm_phy_latency_param(struct cdns_pcie *pcie, bool rx,
+ u32 speed_index, u32 latency);
+int cdns_pcie_init_ptm_phy_latency(struct device *dev, struct cdns_pcie *pcie);
void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
u32 r, bool is_io,
--
2.36.0
From: Alexander Bahle <[email protected]>
Add DT bindings for the PCIe PHY latencies in host and endpoint mode.
Setting these improves the PTM timestamp accuracy.
The values are taken from the Link below.
Signed-off-by: Alexander Bahle <[email protected]>
Signed-off-by: Dominic Rath <[email protected]>
Link: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/998749/am6442-details-regarding-ptm-implementation
---
arch/arm64/boot/dts/ti/k3-am64-main.dtsi | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am64-main.dtsi b/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
index d6aa23681bbe..032abb343c36 100644
--- a/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am64-main.dtsi
@@ -855,6 +855,8 @@ pcie0_rc: pcie@f102000 {
ranges = <0x01000000 0x00 0x68001000 0x00 0x68001000 0x00 0x0010000>,
<0x02000000 0x00 0x68011000 0x00 0x68011000 0x00 0x7fef000>;
dma-ranges = <0x02000000 0x0 0x0 0x0 0x0 0x00000010 0x0>;
+ cdns,tx-phy-latency-ps = <138800 69400>;
+ cdns,rx-phy-latency-ps = <185200 92600>;
};
pcie0_ep: pcie-ep@f102000 {
@@ -873,6 +875,8 @@ pcie0_ep: pcie-ep@f102000 {
clocks = <&k3_clks 114 0>;
clock-names = "fck";
max-functions = /bits/ 8 <1>;
+ cdns,tx-phy-latency-ps = <138800 69400>;
+ cdns,rx-phy-latency-ps = <185200 92600>;
};
epwm0: pwm@23000000 {
--
2.36.0
From: Alexander Bahle <[email protected]>
Add "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps" DT bindings for
setting the PCIe PHY latencies.
The properties expect a list of uint32 PHY latencies in picoseconds for
every supported speed starting at PCIe Gen1, e.g.:
max-link-speed = <2>;
tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
There should be a value for every supported speed.
Signed-off-by: Alexander Bahle <[email protected]>
Signed-off-by: Dominic Rath <[email protected]>
---
.../bindings/pci/cdns,cdns-pcie-ep.yaml | 2 ++
.../bindings/pci/cdns,cdns-pcie-host.yaml | 2 ++
.../devicetree/bindings/pci/cdns-pcie-ep.yaml | 20 +++++++++++++++++++
.../bindings/pci/cdns-pcie-host.yaml | 20 +++++++++++++++++++
4 files changed, 44 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
index e6ef1012a580..ce239da3a592 100644
--- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
@@ -45,6 +45,8 @@ examples:
max-functions = /bits/ 8 <8>;
phys = <&pcie_phy0>;
phy-names = "pcie-phy";
+ cdns,tx-phy-latency-ps = <138800 69400>;
+ cdns,rx-phy-latency-ps = <185200 92600>;
};
};
...
diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
index 293b8ec318bc..a7f4e3909c51 100644
--- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
@@ -70,6 +70,8 @@ examples:
phys = <&pcie_phy0>;
phy-names = "pcie-phy";
+ cdns,tx-phy-latency-ps = <138800 69400>;
+ cdns,rx-phy-latency-ps = <185200 92600>;
};
};
...
diff --git a/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml
index baeafda36ebe..95ea273372d1 100644
--- a/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml
@@ -21,4 +21,24 @@ properties:
maximum: 32
default: 32
+ cdns,tx-phy-latency-ps:
+ description:
+ The PHY latencies for the TX direction applied to the PTM timestamps. Most
+ PCIe PHYs have asynchronous latencies for their RX and TX paths. To obtain
+ accurate PTM timestamps, the PCIe PTM specification requires that the time
+ at which the first serial bit is present on the serial lines be taken.
+ Should contain picosecond latency values for each supported speed,
+ starting with Gen1 latency.
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+
+ cdns,rx-phy-latency-ps:
+ description:
+ The PHY latencies for the RX direction applied to the PTM timestamps. Most
+ PCIe PHYs have asynchronous latencies for their RX and TX paths. To obtain
+ accurate PTM timestamps, the PCIe PTM specification requires that the time
+ at which the first serial bit is present on the serial lines be taken.
+ Should contain picosecond latency values for each supported speed,
+ starting with Gen1 latency.
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+
additionalProperties: true
diff --git a/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml b/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml
index a944f9bfffff..66f5a6449e1e 100644
--- a/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml
@@ -32,6 +32,26 @@ properties:
default: 32
deprecated: true
+ cdns,tx-phy-latency-ps:
+ description:
+ The PHY latencies for the TX direction applied to the PTM timestamps. Most
+ PCIe PHYs have asynchronous latencies for their RX and TX paths. To obtain
+ accurate PTM timestamps, the PCIe PTM specification requires that the time
+ at which the first serial bit is present on the serial lines be taken.
+ Should contain picosecond latency values for each supported speed,
+ starting with Gen1 latency.
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+
+ cdns,rx-phy-latency-ps:
+ description:
+ The PHY latencies for the RX direction applied to the PTM timestamps. Most
+ PCIe PHYs have asynchronous latencies for their RX and TX paths. To obtain
+ accurate PTM timestamps, the PCIe PTM specification requires that the time
+ at which the first serial bit is present on the serial lines be taken.
+ Should contain picosecond latency values for each supported speed,
+ starting with Gen1 latency.
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+
msi-parent: true
additionalProperties: true
--
2.36.0
On Thu, 13 Oct 2022 08:26:47 +0200, Dominic Rath wrote:
> From: Alexander Bahle <[email protected]>
>
> Add "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps" DT bindings for
> setting the PCIe PHY latencies.
> The properties expect a list of uint32 PHY latencies in picoseconds for
> every supported speed starting at PCIe Gen1, e.g.:
>
> max-link-speed = <2>;
> tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
> rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
>
> There should be a value for every supported speed.
>
> Signed-off-by: Alexander Bahle <[email protected]>
> Signed-off-by: Dominic Rath <[email protected]>
> ---
> .../bindings/pci/cdns,cdns-pcie-ep.yaml | 2 ++
> .../bindings/pci/cdns,cdns-pcie-host.yaml | 2 ++
> .../devicetree/bindings/pci/cdns-pcie-ep.yaml | 20 +++++++++++++++++++
> .../bindings/pci/cdns-pcie-host.yaml | 20 +++++++++++++++++++
> 4 files changed, 44 insertions(+)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml: properties:cdns,tx-phy-latency-ps: '$ref' should not be valid under {'const': '$ref'}
hint: Standard unit suffix properties don't need a type $ref
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/cdns-pcie-host.yaml: properties:cdns,rx-phy-latency-ps: '$ref' should not be valid under {'const': '$ref'}
hint: Standard unit suffix properties don't need a type $ref
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
./Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/pci/cdns-pcie-host.yaml
./Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/pci/cdns-pcie-ep.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml: properties:cdns,tx-phy-latency-ps: '$ref' should not be valid under {'const': '$ref'}
hint: Standard unit suffix properties don't need a type $ref
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/cdns-pcie-ep.yaml: properties:cdns,rx-phy-latency-ps: '$ref' should not be valid under {'const': '$ref'}
hint: Standard unit suffix properties don't need a type $ref
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
./Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/pci/cdns-pcie-host.yaml
./Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/pci/cdns-pcie-ep.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
On Thu, Oct 13, 2022 at 08:26:48AM +0200, Dominic Rath wrote:
> From: Alexander Bahle <[email protected]>
>
> Use optional "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps"
> DeviceTree bindings to set the CDNS_PCIE_LM_PTM_LAT_PARAM(_IDX)
> register(s) during PCIe host and ep setup.
> The properties expect a list of uint32 PHY latencies in picoseconds for
> every supported speed starting at PCIe Gen1, e.g.:
s/ep/endpoint/
s/properties expect a list/properties are lists/
Rewrap into a single paragraph or add a blank line between paragraphs.
> max-link-speed = <2>;
> tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
> rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
>
> There should be a value for every supported speed but it is not enforced or
> necessary. A warning is emitted to let users know that the PTM timestamps
> from this PCIe device may not be precise enough for some applications.
Not sure what "it is not enforced or necessary" means. Maybe it just
means that if a value is missing, we don't program LAT_PARAM and we
emit a warning?
> + param_count = of_property_count_u32_elems(np, key);
> + if (param_count < 0 || param_count < max_link_speed) {
> + dev_warn(dev,
> + "no %s set for one or more speeds: %d\n",
> + key, param_count);
> + }
> +
> + /* Don't set param for unsupported speed */
> + if (param_count > max_link_speed)
> + param_count = max_link_speed;
> +
> + for (i = 0; i < param_count; i++) {
> + if (of_property_read_u32_index(np, key, i,
> + &latency) < 0) {
> + dev_err(dev, "failed to set latency for speed %d. %s\n",
> + i, key);
Seems like these messages should contain "PTM" somewhere.
If they're truly optional properties, should these be dev_info()
instead of dev_warn/dev_err?
Bjorn
On Thu, Oct 13, 2022 at 08:26:47AM +0200, Dominic Rath wrote:
> From: Alexander Bahle <[email protected]>
>
> Add "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps" DT bindings for
> setting the PCIe PHY latencies.
> The properties expect a list of uint32 PHY latencies in picoseconds for
> every supported speed starting at PCIe Gen1, e.g.:
>
> max-link-speed = <2>;
> tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
> rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
These are a property of the PHY or PCI host? Sounds like PHY to me and
that should be in the PHY node. No reason the PCI driver can't go read
PHY node properties.
If PTM is a standard PCIe thing, then I don't think these should be
Cadence specific. IOW, drop 'cdns'.
Rob
On Thu, Oct 13, 2022 at 02:12:49PM -0500, Rob Herring wrote:
> On Thu, Oct 13, 2022 at 08:26:47AM +0200, Dominic Rath wrote:
> > From: Alexander Bahle <[email protected]>
> >
> > Add "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps" DT bindings for
> > setting the PCIe PHY latencies.
> > The properties expect a list of uint32 PHY latencies in picoseconds for
> > every supported speed starting at PCIe Gen1, e.g.:
> >
> > max-link-speed = <2>;
> > tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
> > rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
> ...
> If PTM is a standard PCIe thing, then I don't think these should be
> Cadence specific. IOW, drop 'cdns'.
PTM is definitely a standard PCIe thing. I had the same question.
On Thu, Oct 13, 2022 at 02:12:49PM -0500, Rob Herring wrote:
> On Thu, Oct 13, 2022 at 08:26:47AM +0200, Dominic Rath wrote:
> > From: Alexander Bahle <[email protected]>
> >
> > Add "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps" DT bindings for
> > setting the PCIe PHY latencies.
> > The properties expect a list of uint32 PHY latencies in picoseconds for
> > every supported speed starting at PCIe Gen1, e.g.:
> >
> > max-link-speed = <2>;
> > tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
> > rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
>
> These are a property of the PHY or PCI host? Sounds like PHY to me and
> that should be in the PHY node. No reason the PCI driver can't go read
> PHY node properties.
I'm actually not sure if this a property of the PHY, the PCIe host, or
of the combination of the two.
We thought about adding this property to the PHY, too, but we didn't
know how to handle cases where a single PCIe host is linked with
multiple PHYs for multi-lane configurations (see TI's AM65x for
example). Which PHYs latency would you use to configure this PCIe RC?
Personally I don't have a very strong opinion either way - we just
didn't know any better than to put this into the PCIe host that needs
it. If you think this is better put into the PHY node we can of course
send a new version of this patch.
Is there any binding that specifies "generic" PCIe properties, similar
to ethernet-phy.yaml? We couldn't find any.
I guess in the AM64x case the "PHY" is serdes0_pcie_link below serdes0:
&serdes0 {
serdes0_pcie_link: phy@0 {
...
This seems to be described by bindings/phy/phy-cadence-torrent.yaml.
Should we add a generic (without cdns) tx/rx-phy-latency-ps property
there?
> If PTM is a standard PCIe thing, then I don't think these should be
> Cadence specific. IOW, drop 'cdns'.
Yes, it is a standard PCIe thing, but we haven't seen that many
implementations yet, so we didn't want to pretend to know what this
looks like in the generic case. We can of course drop 'cdns'.
Best Regards,
Dominic & Alexander
Hi Dominic,
On 14/10/22 7:11 pm, Dominic Rath wrote:
> On Thu, Oct 13, 2022 at 02:12:49PM -0500, Rob Herring wrote:
>> On Thu, Oct 13, 2022 at 08:26:47AM +0200, Dominic Rath wrote:
>>> From: Alexander Bahle <[email protected]>
>>>
>>> Add "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps" DT bindings for
>>> setting the PCIe PHY latencies.
>>> The properties expect a list of uint32 PHY latencies in picoseconds for
>>> every supported speed starting at PCIe Gen1, e.g.:
>>>
>>> max-link-speed = <2>;
>>> tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
>>> rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
>>
>> These are a property of the PHY or PCI host? Sounds like PHY to me and
>> that should be in the PHY node. No reason the PCI driver can't go read
>> PHY node properties.
>
> I'm actually not sure if this a property of the PHY, the PCIe host, or
> of the combination of the two.
>
Latency is mostly related to propogation latency through SERDES PCS and
PMA layers.
> We thought about adding this property to the PHY, too, but we didn't
> know how to handle cases where a single PCIe host is linked with
> multiple PHYs for multi-lane configurations (see TI's AM65x for
> example). Which PHYs latency would you use to configure this PCIe RC?
On AM65x, all lanes go through SERDES of same design (but just different
instances) and thus latencies will remain same across lanes as the PCS
and PMA logics are same. So, the delays are not lane specific
>
> Personally I don't have a very strong opinion either way - we just
> didn't know any better than to put this into the PCIe host that needs
> it. If you think this is better put into the PHY node we can of course
> send a new version of this patch.
>
I don't have a preference here... Delays are dependent on PHYs being
used but something that host needs, will leave it to framework
maintainers.
> Is there any binding that specifies "generic" PCIe properties, similar
> to ethernet-phy.yaml? We couldn't find any.
>
> I guess in the AM64x case the "PHY" is serdes0_pcie_link below serdes0:
>
> &serdes0 {
> serdes0_pcie_link: phy@0 {
> ...
>
> This seems to be described by bindings/phy/phy-cadence-torrent.yaml.
>
> Should we add a generic (without cdns) tx/rx-phy-latency-ps property
> there?
>
>> If PTM is a standard PCIe thing, then I don't think these should be
>> Cadence specific. IOW, drop 'cdns'.
>
> Yes, it is a standard PCIe thing, but we haven't seen that many
> implementations yet, so we didn't want to pretend to know what this
> looks like in the generic case. We can of course drop 'cdns'.
PTM is definitely standard and vendor specific prefix don't make sense
to me.
>
> Best Regards,
>
> Dominic & Alexander
Hi Dominic,
> >>>
> >>> Add "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps" DT bindings for
> >>> setting the PCIe PHY latencies.
> >>> The properties expect a list of uint32 PHY latencies in picoseconds for
> >>> every supported speed starting at PCIe Gen1, e.g.:
> >>>
> >>> max-link-speed = <2>;
> >>> tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
> >>> rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
> >>
> >> These are a property of the PHY or PCI host? Sounds like PHY to me and
> >> that should be in the PHY node. No reason the PCI driver can't go read
> >> PHY node properties.
> >
> > I'm actually not sure if this a property of the PHY, the PCIe host, or
> > of the combination of the two.
> >
>
> Latency is mostly related to propogation latency through SERDES PCS and
> PMA layers.
>
> > We thought about adding this property to the PHY, too, but we didn't
> > know how to handle cases where a single PCIe host is linked with
> > multiple PHYs for multi-lane configurations (see TI's AM65x for
> > example). Which PHYs latency would you use to configure this PCIe RC?
>
> On AM65x, all lanes go through SERDES of same design (but just different
> instances) and thus latencies will remain same across lanes as the PCS
> and PMA logics are same. So, the delays are not lane specific
>
> >
> > Personally I don't have a very strong opinion either way - we just
> > didn't know any better than to put this into the PCIe host that needs
> > it. If you think this is better put into the PHY node we can of course
> > send a new version of this patch.
> >
>
> I don't have a preference here... Delays are dependent on PHYs being
> used but something that host needs, will leave it to framework
> maintainers.
>
> > Is there any binding that specifies "generic" PCIe properties, similar
> > to ethernet-phy.yaml? We couldn't find any.
> >
> > I guess in the AM64x case the "PHY" is serdes0_pcie_link below serdes0:
> >
> > &serdes0 {
> > serdes0_pcie_link: phy@0 {
> > ...
> >
> > This seems to be described by bindings/phy/phy-cadence-torrent.yaml.
> >
> > Should we add a generic (without cdns) tx/rx-phy-latency-ps property
> > there?
> >
> >> If PTM is a standard PCIe thing, then I don't think these should be
> >> Cadence specific. IOW, drop 'cdns'.
> >
> > Yes, it is a standard PCIe thing, but we haven't seen that many
> > implementations yet, so we didn't want to pretend to know what this
> > looks like in the generic case. We can of course drop 'cdns'.
>
> PTM is definitely standard and vendor specific prefix don't make sense
> to me.
>
Is there any chance you can send a revisited patch series or is there
anything missing for
you to continue?
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy