[PATCH net-next 0/6] New RGMII delay DT bindings for the SJA1105 DSA driver

[PATCH net-next 0/6] New RGMII delay DT bindings for the SJA1105 DSA driver

[Vladimir Oltean]

During recent reviews I've been telling people that new MAC drivers
should adopt a certain DT binding format for RGMII delays in order to
avoid conflicting interpretations. Some suggestions were better received
than others, and it appears we are still far from a consensus.

Part of the problem seems to be that there are still drivers that apply
RGMII delays based on an incorrect interpretation of the device tree,
and these serve as a bad example for others.
I happen to maintain one of those drivers and I am able to test it, so I
figure that one of the ways in which I can make a change is to stop
providing a bad example.

Therefore, this series adds support for the "rx-internal-delay-ps" and
"tx-internal-delay-ps" properties inside sja1105 switch port DT nodes,
and if these are present, they will decide what RGMII delays will the
driver apply.

The in-tree device trees are also updated to follow the new format, as
well as the schema validator.

I assume it's okay to get all changes merged in through the same tree
(net-next). Although the DTS changes could be split, if needed - the
driver works with or without them. There is one more DTS which should be
changed, which is in Shawn's tree but not in net-next:
https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/tree/arch/arm64/boot/dts/freescale/fsl-lx2160a-bluebox3.dts?h=for-next
For that, I'd have to send a separate patch.

Vladimir Oltean (6):
  ARM: dts: imx6qp-prtwd3: update RGMII delays for sja1105 switch
  ARM: dts: ls1021a-tsn: update RGMII delays for sja1105 switch
  dt-bindings: net: dsa: sja1105: fix example so all ports have a
    phy-handle of fixed-link
  dt-bindings: net: dsa: inherit the ethernet-controller DT schema
  dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps
  net: dsa: sja1105: parse {rx,tx}-internal-delay-ps properties for
    RGMII delays

 .../devicetree/bindings/net/dsa/dsa.yaml      |  7 ++
 .../bindings/net/dsa/nxp,sja1105.yaml         | 43 +++++++++
 arch/arm/boot/dts/imx6qp-prtwd3.dts           |  2 +
 arch/arm/boot/dts/ls1021a-tsn.dts             |  2 +
 drivers/net/dsa/sja1105/sja1105.h             | 25 ++++-
 drivers/net/dsa/sja1105/sja1105_clocking.c    | 35 +++----
 drivers/net/dsa/sja1105/sja1105_main.c        | 94 ++++++++++++++-----
 7 files changed, 161 insertions(+), 47 deletions(-)

-- 
2.25.1

[PATCH net-next 1/6] ARM: dts: imx6qp-prtwd3: update RGMII delays for sja1105 switch

[Vladimir Oltean]

In the new behavior, the sja1105 driver expects there to be explicit
RGMII delays present on the fixed-link ports, otherwise it will complain
that it falls back to legacy behavior, which is to apply RGMII delays
incorrectly derived from the phy-mode string.

In this case, the legacy behavior of the driver is to apply both RX and
TX delays. To preserve that, add explicit 2 nanosecond delays, which are
identical with what the driver used to add (a 90 degree phase shift).
The delays from the phy-mode are ignored by new kernels (it's still
RGMII as long as it's "rgmii*" something), and the explicit
{rx,tx}-internal-delay-ps properties are ignored by old kernels, so the
change works both ways.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 arch/arm/boot/dts/imx6qp-prtwd3.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qp-prtwd3.dts b/arch/arm/boot/dts/imx6qp-prtwd3.dts
index b92e0f2748a5..29dd59bfa73d 100644
--- a/arch/arm/boot/dts/imx6qp-prtwd3.dts
+++ b/arch/arm/boot/dts/imx6qp-prtwd3.dts
@@ -178,6 +178,8 @@ port@4 {
 				label = "cpu";
 				ethernet = <&fec>;
 				phy-mode = "rgmii-id";
+				rx-internal-delay-ps = <2000>;
+				tx-internal-delay-ps = <2000>;
 
 				fixed-link {
 					speed = <100>;
-- 
2.25.1

[PATCH net-next 2/6] ARM: dts: ls1021a-tsn: update RGMII delays for sja1105 switch

[Vladimir Oltean]

In the new behavior, the sja1105 driver expects there to be explicit
RGMII delays present on the fixed-link ports, otherwise it will complain
that it falls back to legacy behavior, which is to apply RGMII delays
incorrectly derived from the phy-mode string.

In this case, the legacy behavior of the driver is to not apply delays
in any direction (mostly because the SJA1105T can't do that, so this
board uses PCB traces). To preserve that but also silence the driver,
use explicit delays of 0 ns. The delay information from the phy-mode is
ignored by new kernels (it's still RGMII as long as it's "rgmii*"
something), and the explicit {rx,tx}-internal-delay-ps properties are
ignored by old kernels, so the change works both ways.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 arch/arm/boot/dts/ls1021a-tsn.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/ls1021a-tsn.dts b/arch/arm/boot/dts/ls1021a-tsn.dts
index 9d8f0c2a8aba..517286073451 100644
--- a/arch/arm/boot/dts/ls1021a-tsn.dts
+++ b/arch/arm/boot/dts/ls1021a-tsn.dts
@@ -90,6 +90,8 @@ port@4 {
 				/* Internal port connected to eth2 */
 				ethernet = <&enet2>;
 				phy-mode = "rgmii";
+				rx-internal-delay-ps = <0>;
+				tx-internal-delay-ps = <0>;
 				reg = <4>;
 
 				fixed-link {
-- 
2.25.1

[PATCH net-next 3/6] dt-bindings: net: dsa: sja1105: fix example so all ports have a phy-handle of fixed-link

[Vladimir Oltean]

All ports require either a phy-handle or a fixed-link, and port 3 in the
example didn't have one. Add it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index f978f8719d8e..f97a22772e6f 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -113,6 +113,7 @@ examples:
                             };
 
                             port@3 {
+                                    phy-handle = <&rgmii_phy4>;
                                     phy-mode = "rgmii-id";
                                     reg = <3>;
                             };
-- 
2.25.1

[PATCH net-next 4/6] dt-bindings: net: dsa: inherit the ethernet-controller DT schema

[Vladimir Oltean]

Since a switch is basically a bunch of Ethernet controllers, just
inherit the common schema for one to get stronger type validation of the
properties of a port.

For example, before this change it was valid to have a phy-mode = "xfi"
even if "xfi" is not part of ethernet-controller.yaml, now it is not.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 Documentation/devicetree/bindings/net/dsa/dsa.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index 224cfa45de9a..9cfd08cd31da 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
@@ -46,6 +46,9 @@ patternProperties:
         type: object
         description: Ethernet switch ports
 
+        allOf:
+          - $ref: "http://devicetree.org/schemas/net/ethernet-controller.yaml#"
+
         properties:
           reg:
             description: Port number
-- 
2.25.1

[PATCH net-next 5/6] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps

[Vladimir Oltean]

Add a schema validator to nxp,sja1105.yaml and to dsa.yaml for explicit
MAC-level RGMII delays. These properties must be per port and must be
present only for a phy-mode that represents RGMII.

We tell dsa.yaml that these port properties might be present, we also
define their valid values for SJA1105. We create a common definition for
the RX and TX valid range, since it's quite a mouthful.

We also modify the example to include the explicit RGMII delay properties.
On the fixed-link ports (in the example, port 4), having these explicit
delays is actually mandatory, since with the new behavior, the driver
shouts that it is interpreting what delays to apply based on phy-mode.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 .../devicetree/bindings/net/dsa/dsa.yaml      |  4 ++
 .../bindings/net/dsa/nxp,sja1105.yaml         | 42 +++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.yaml b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
index 9cfd08cd31da..2ad7f79ad371 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.yaml
@@ -97,6 +97,10 @@ patternProperties:
 
           managed: true
 
+          rx-internal-delay-ps: true
+
+          tx-internal-delay-ps: true
+
         required:
           - reg
 
diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index f97a22772e6f..0bbaefacdaba 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -74,10 +74,42 @@ properties:
           - compatible
           - reg
 
+patternProperties:
+  "^(ethernet-)?ports$":
+    patternProperties:
+      "^(ethernet-)?port@[0-9]+$":
+        allOf:
+        - if:
+            properties:
+                phy-mode:
+                    contains:
+                        enum:
+                          - rgmii
+                          - rgmii-rxid
+                          - rgmii-txid
+                          - rgmii-id
+          then:
+            properties:
+                rx-internal-delay-ps:
+                    $ref: "#/$defs/internal-delay-ps"
+                tx-internal-delay-ps:
+                    $ref: "#/$defs/internal-delay-ps"
+
 required:
   - compatible
   - reg
 
+$defs:
+    internal-delay-ps:
+        description:
+            Disable tunable delay lines using 0 ps, or enable them and select
+            the phase between 1640 ps (73.8 degree shift at 1Gbps) and 2260 ps
+            (101.7 degree shift) in increments of 0.9 degrees (20 ps).
+        enum:
+            [0, 1640, 1660, 1680, 1700, 1720, 1740, 1760, 1780, 1800, 1820,
+            1840, 1860, 1880, 1900, 1920, 1940, 1960, 1980, 2000, 2020, 2040,
+            2060, 2080, 2100, 2120, 2140, 2160, 2180, 2200, 2220, 2240, 2260]
+
 unevaluatedProperties: false
 
 examples:
@@ -97,30 +129,40 @@ examples:
                             port@0 {
                                     phy-handle = <&rgmii_phy6>;
                                     phy-mode = "rgmii-id";
+                                    rx-internal-delay-ps = <0>;
+                                    tx-internal-delay-ps = <0>;
                                     reg = <0>;
                             };
 
                             port@1 {
                                     phy-handle = <&rgmii_phy3>;
                                     phy-mode = "rgmii-id";
+                                    rx-internal-delay-ps = <0>;
+                                    tx-internal-delay-ps = <0>;
                                     reg = <1>;
                             };
 
                             port@2 {
                                     phy-handle = <&rgmii_phy4>;
                                     phy-mode = "rgmii-id";
+                                    rx-internal-delay-ps = <0>;
+                                    tx-internal-delay-ps = <0>;
                                     reg = <2>;
                             };
 
                             port@3 {
                                     phy-handle = <&rgmii_phy4>;
                                     phy-mode = "rgmii-id";
+                                    rx-internal-delay-ps = <0>;
+                                    tx-internal-delay-ps = <0>;
                                     reg = <3>;
                             };
 
                             port@4 {
                                     ethernet = <&enet2>;
                                     phy-mode = "rgmii";
+                                    rx-internal-delay-ps = <0>;
+                                    tx-internal-delay-ps = <0>;
                                     reg = <4>;
 
                                     fixed-link {
-- 
2.25.1

[PATCH net-next 6/6] net: dsa: sja1105: parse {rx,tx}-internal-delay-ps properties for RGMII delays

[Vladimir Oltean]

This change does not fix any functional issue or address any real life
use case that wasn't possible before. It is just a small step in the
process of standardizing the way in which Ethernet MAC drivers may apply
RGMII delays (traditionally these have been applied by PHYs, with no
clear definition of what to do in the case of a fixed-link).

The sja1105 driver used to apply MAC-level RGMII delays on the RX data
lines when in fixed-link mode and using a phy-mode of "rgmii-rxid" or
"rgmii-id" and on the TX data lines when using "rgmii-txid" or "rgmii-id".
But the standard definitions don't say anything about behaving
differently when the port is in fixed-link vs when it isn't, and the new
device tree bindings are about having a way of applying the delays in a
way that is independent of the phy-mode and of the fixed-link property.

When the {rx,tx}-internal-delay-ps properties are present, use them,
otherwise fall back to the old behavior and warn.

One other thing to note is that the SJA1105 hardware applies a delay
value in degrees rather than in picoseconds (the delay in ps changes
depending on the frequency of the RGMII clock - 125 MHz at 1G, 25 MHz at
100M, 2.5MHz at 10M). I assume that is fine, we calculate the phase
shift of the internal delay lines assuming that the device tree meant
gigabit, and we let the hardware scale those according to the link speed.

Link: https://patchwork.kernel.org/project/netdevbpf/patch/20210723173108.459770-6-prasanna.vengateshan@microchip.com/
Link: https://patchwork.ozlabs.org/project/netdev/patch/20200616074955.GA9092@laureti-dev/#2461123
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105.h          | 25 +++++-
 drivers/net/dsa/sja1105/sja1105_clocking.c | 35 ++++----
 drivers/net/dsa/sja1105/sja1105_main.c     | 94 ++++++++++++++++------
 3 files changed, 107 insertions(+), 47 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 618c8d6a8be1..808419f3b808 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -20,6 +20,27 @@
 #define SJA1105_AGEING_TIME_MS(ms)	((ms) / 10)
 #define SJA1105_NUM_L2_POLICERS		SJA1110_MAX_L2_POLICING_COUNT
 
+/* Calculated assuming 1Gbps, where the clock has 125 MHz (8 ns period)
+ * To avoid floating point operations, we'll multiply the degrees by 10
+ * to get a "phase" and get 1 decimal point precision.
+ */
+#define SJA1105_RGMII_DELAY_PS_TO_PHASE(ps) \
+	(((ps) * 360) / 800)
+#define SJA1105_RGMII_DELAY_PHASE_TO_PS(phase) \
+	((800 * (phase)) / 360)
+#define SJA1105_RGMII_DELAY_PHASE_TO_HW(phase) \
+	(((phase) - 738) / 9)
+#define SJA1105_RGMII_DELAY_PS_TO_HW(ps) \
+	SJA1105_RGMII_DELAY_PHASE_TO_HW(SJA1105_RGMII_DELAY_PS_TO_PHASE(ps))
+
+/* Valid range in degrees is a value between 73.8 and 101.7
+ * in 0.9 degree increments
+ */
+#define SJA1105_RGMII_DELAY_MIN_PS \
+	SJA1105_RGMII_DELAY_PHASE_TO_PS(738)
+#define SJA1105_RGMII_DELAY_MAX_PS \
+	SJA1105_RGMII_DELAY_PHASE_TO_PS(1017)
+
 typedef enum {
 	SPI_READ = 0,
 	SPI_WRITE = 1,
@@ -222,8 +243,8 @@ struct sja1105_flow_block {
 
 struct sja1105_private {
 	struct sja1105_static_config static_config;
-	bool rgmii_rx_delay[SJA1105_MAX_NUM_PORTS];
-	bool rgmii_tx_delay[SJA1105_MAX_NUM_PORTS];
+	int rgmii_rx_delay_ps[SJA1105_MAX_NUM_PORTS];
+	int rgmii_tx_delay_ps[SJA1105_MAX_NUM_PORTS];
 	phy_interface_t phy_mode[SJA1105_MAX_NUM_PORTS];
 	bool fixed_link[SJA1105_MAX_NUM_PORTS];
 	unsigned long ucast_egress_floods;
diff --git a/drivers/net/dsa/sja1105/sja1105_clocking.c b/drivers/net/dsa/sja1105/sja1105_clocking.c
index 5bbf1707f2af..e3699f76f6d7 100644
--- a/drivers/net/dsa/sja1105/sja1105_clocking.c
+++ b/drivers/net/dsa/sja1105/sja1105_clocking.c
@@ -498,17 +498,6 @@ sja1110_cfg_pad_mii_id_packing(void *buf, struct sja1105_cfg_pad_mii_id *cmd,
 	sja1105_packing(buf, &cmd->txc_pd,          0,  0, size, op);
 }
 
-/* Valid range in degrees is an integer between 73.8 and 101.7 */
-static u64 sja1105_rgmii_delay(u64 phase)
-{
-	/* UM11040.pdf: The delay in degree phase is 73.8 + delay_tune * 0.9.
-	 * To avoid floating point operations we'll multiply by 10
-	 * and get 1 decimal point precision.
-	 */
-	phase *= 10;
-	return (phase - 738) / 9;
-}
-
 /* The RGMII delay setup procedure is 2-step and gets called upon each
  * .phylink_mac_config. Both are strategic.
  * The reason is that the RX Tunable Delay Line of the SJA1105 MAC has issues
@@ -521,13 +510,15 @@ int sja1105pqrs_setup_rgmii_delay(const void *ctx, int port)
 	const struct sja1105_private *priv = ctx;
 	const struct sja1105_regs *regs = priv->info->regs;
 	struct sja1105_cfg_pad_mii_id pad_mii_id = {0};
+	int rx_delay = priv->rgmii_rx_delay_ps[port];
+	int tx_delay = priv->rgmii_tx_delay_ps[port];
 	u8 packed_buf[SJA1105_SIZE_CGU_CMD] = {0};
 	int rc;
 
-	if (priv->rgmii_rx_delay[port])
-		pad_mii_id.rxc_delay = sja1105_rgmii_delay(90);
-	if (priv->rgmii_tx_delay[port])
-		pad_mii_id.txc_delay = sja1105_rgmii_delay(90);
+	if (rx_delay)
+		pad_mii_id.rxc_delay = SJA1105_RGMII_DELAY_PS_TO_HW(rx_delay);
+	if (tx_delay)
+		pad_mii_id.txc_delay = SJA1105_RGMII_DELAY_PS_TO_HW(tx_delay);
 
 	/* Stage 1: Turn the RGMII delay lines off. */
 	pad_mii_id.rxc_bypass = 1;
@@ -542,11 +533,11 @@ int sja1105pqrs_setup_rgmii_delay(const void *ctx, int port)
 		return rc;
 
 	/* Stage 2: Turn the RGMII delay lines on. */
-	if (priv->rgmii_rx_delay[port]) {
+	if (rx_delay) {
 		pad_mii_id.rxc_bypass = 0;
 		pad_mii_id.rxc_pd = 0;
 	}
-	if (priv->rgmii_tx_delay[port]) {
+	if (tx_delay) {
 		pad_mii_id.txc_bypass = 0;
 		pad_mii_id.txc_pd = 0;
 	}
@@ -561,20 +552,22 @@ int sja1110_setup_rgmii_delay(const void *ctx, int port)
 	const struct sja1105_private *priv = ctx;
 	const struct sja1105_regs *regs = priv->info->regs;
 	struct sja1105_cfg_pad_mii_id pad_mii_id = {0};
+	int rx_delay = priv->rgmii_rx_delay_ps[port];
+	int tx_delay = priv->rgmii_tx_delay_ps[port];
 	u8 packed_buf[SJA1105_SIZE_CGU_CMD] = {0};
 
 	pad_mii_id.rxc_pd = 1;
 	pad_mii_id.txc_pd = 1;
 
-	if (priv->rgmii_rx_delay[port]) {
-		pad_mii_id.rxc_delay = sja1105_rgmii_delay(90);
+	if (rx_delay) {
+		pad_mii_id.rxc_delay = SJA1105_RGMII_DELAY_PS_TO_HW(rx_delay);
 		/* The "BYPASS" bit in SJA1110 is actually a "don't bypass" */
 		pad_mii_id.rxc_bypass = 1;
 		pad_mii_id.rxc_pd = 0;
 	}
 
-	if (priv->rgmii_tx_delay[port]) {
-		pad_mii_id.txc_delay = sja1105_rgmii_delay(90);
+	if (tx_delay) {
+		pad_mii_id.txc_delay = SJA1105_RGMII_DELAY_PS_TO_HW(tx_delay);
 		pad_mii_id.txc_bypass = 1;
 		pad_mii_id.txc_pd = 0;
 	}
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 0f1bba0076a8..1832d4bd3440 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1109,27 +1109,78 @@ static int sja1105_static_config_load(struct sja1105_private *priv)
 	return sja1105_static_config_upload(priv);
 }
 
-static int sja1105_parse_rgmii_delays(struct sja1105_private *priv)
+/* This is the "new way" for a MAC driver to configure its RGMII delay lines,
+ * based on the explicit "rx-internal-delay-ps" and "tx-internal-delay-ps"
+ * properties. It has the advantage of working with fixed links and with PHYs
+ * that apply RGMII delays too, and the MAC driver needs not perform any
+ * special checks.
+ *
+ * Previously we were acting upon the "phy-mode" property when we were
+ * operating in fixed-link, basically acting as a PHY, but with a reversed
+ * interpretation: PHY_INTERFACE_MODE_RGMII_TXID means that the MAC should
+ * behave as if it is connected to a PHY which has applied RGMII delays in the
+ * TX direction. So if anything, RX delays should have been added by the MAC,
+ * but we were adding TX delays.
+ *
+ * If the "{rx,tx}-internal-delay-ps" properties are not specified, we fall
+ * back to the legacy behavior and apply delays on fixed-link ports based on
+ * the reverse interpretation of the phy-mode. This is a deviation from the
+ * expected default behavior which is to simply apply no delays. To achieve
+ * that behavior with the new bindings, it is mandatory to specify
+ * "{rx,tx}-internal-delay-ps" with a value of 0.
+ */
+static int sja1105_parse_rgmii_delays(struct sja1105_private *priv, int port,
+				      struct device_node *port_dn)
 {
-	struct dsa_switch *ds = priv->ds;
-	int port;
+	phy_interface_t phy_mode = priv->phy_mode[port];
+	struct device *dev = &priv->spidev->dev;
+	int rx_delay = -1, tx_delay = -1;
 
-	for (port = 0; port < ds->num_ports; port++) {
-		if (!priv->fixed_link[port])
-			continue;
+	if (!phy_interface_mode_is_rgmii(phy_mode))
+		return 0;
 
-		if (priv->phy_mode[port] == PHY_INTERFACE_MODE_RGMII_RXID ||
-		    priv->phy_mode[port] == PHY_INTERFACE_MODE_RGMII_ID)
-			priv->rgmii_rx_delay[port] = true;
+	of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay);
+	of_property_read_u32(port_dn, "tx-internal-delay-ps", &tx_delay);
 
-		if (priv->phy_mode[port] == PHY_INTERFACE_MODE_RGMII_TXID ||
-		    priv->phy_mode[port] == PHY_INTERFACE_MODE_RGMII_ID)
-			priv->rgmii_tx_delay[port] = true;
+	if (rx_delay == -1 && tx_delay == -1 && priv->fixed_link[port]) {
+		dev_warn(dev,
+			 "Port %d interpreting RGMII delay settings based on \"phy-mode\" property, "
+			 "please update device tree to specify \"rx-internal-delay-ps\" and "
+			 "\"tx-internal-delay-ps\"",
+			 port);
 
-		if ((priv->rgmii_rx_delay[port] || priv->rgmii_tx_delay[port]) &&
-		    !priv->info->setup_rgmii_delay)
-			return -EINVAL;
+		if (phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
+		    phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
+			rx_delay = 2000;
+
+		if (phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
+		    phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
+			tx_delay = 2000;
 	}
+
+	if (rx_delay < 0)
+		rx_delay = 0;
+	if (tx_delay < 0)
+		tx_delay = 0;
+
+	if ((rx_delay || tx_delay) && !priv->info->setup_rgmii_delay) {
+		dev_err(dev, "Chip cannot apply RGMII delays\n");
+		return -EINVAL;
+	}
+
+	if ((rx_delay && rx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
+	    (tx_delay && tx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
+	    (rx_delay > SJA1105_RGMII_DELAY_MAX_PS) ||
+	    (tx_delay > SJA1105_RGMII_DELAY_MAX_PS)) {
+		dev_err(dev,
+			"port %d RGMII delay values out of range, must be between %d and %d ps\n",
+			port, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS);
+		return -ERANGE;
+	}
+
+	priv->rgmii_rx_delay_ps[port] = rx_delay;
+	priv->rgmii_tx_delay_ps[port] = tx_delay;
+
 	return 0;
 }
 
@@ -1180,6 +1231,10 @@ static int sja1105_parse_ports_node(struct sja1105_private *priv,
 		}
 
 		priv->phy_mode[index] = phy_mode;
+
+		err = sja1105_parse_rgmii_delays(priv, index, child);
+		if (err)
+			return err;
 	}
 
 	return 0;
@@ -3317,15 +3372,6 @@ static int sja1105_probe(struct spi_device *spi)
 		return rc;
 	}
 
-	/* Error out early if internal delays are required through DT
-	 * and we can't apply them.
-	 */
-	rc = sja1105_parse_rgmii_delays(priv);
-	if (rc < 0) {
-		dev_err(ds->dev, "RGMII delay not supported\n");
-		return rc;
-	}
-
 	if (IS_ENABLED(CONFIG_NET_SCH_CBS)) {
 		priv->cbs = devm_kcalloc(dev, priv->info->num_cbs_shapers,
 					 sizeof(struct sja1105_cbs_entry),
-- 
2.25.1

Re: [PATCH net-next 6/6] net: dsa: sja1105: parse {rx,tx}-internal-delay-ps properties for RGMII delays

[Jakub Kicinski]

Some take it or leave it comments, checkpatch pointed out some extra
brackets so I had a look at the patch.

On Thu, 14 Oct 2021 01:23:13 +0300 Vladimir Oltean wrote:
> +	int rx_delay = -1, tx_delay = -1;
>  
> +	if (!phy_interface_mode_is_rgmii(phy_mode))
> +		return 0;
>  
> +	of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay);
> +	of_property_read_u32(port_dn, "tx-internal-delay-ps", &tx_delay);

If I'm reading this right you're depending on delays being left as -1
in case the property reads fail. Is this commonly accepted practice?
Why not code it up as:

	u32 rx_delay;

	if (of_property_read_u32(...))
		rx_delay = 0;
	else if (rx_delay != clamp(rx_delay, ...MIN, ...MAX)
		goto err;

or some such?

> +	if ((rx_delay && rx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
> +	    (tx_delay && tx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
> +	    (rx_delay > SJA1105_RGMII_DELAY_MAX_PS) ||
> +	    (tx_delay > SJA1105_RGMII_DELAY_MAX_PS)) {

nit: checkpatch says the brackets around the latter two are unnecessary,
     just in case it's not for symmetry / on purpose

Re: [PATCH net-next 6/6] net: dsa: sja1105: parse {rx,tx}-internal-delay-ps properties for RGMII delays

[Vladimir Oltean]

On Wed, Oct 13, 2021 at 05:24:48PM -0700, Jakub Kicinski wrote:
> Some take it or leave it comments, checkpatch pointed out some extra
> brackets so I had a look at the patch.
> 
> On Thu, 14 Oct 2021 01:23:13 +0300 Vladimir Oltean wrote:
> > +	int rx_delay = -1, tx_delay = -1;
> >  
> > +	if (!phy_interface_mode_is_rgmii(phy_mode))
> > +		return 0;
> >  
> > +	of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay);
> > +	of_property_read_u32(port_dn, "tx-internal-delay-ps", &tx_delay);
> 
> If I'm reading this right you're depending on delays being left as -1
> in case the property reads fail. Is this commonly accepted practice?

It works.

> Why not code it up as:
> 
> 	u32 rx_delay;
> 
> 	if (of_property_read_u32(...))
> 		rx_delay = 0;
> 	else if (rx_delay != clamp(rx_delay, ...MIN, ...MAX)
> 		goto err;
> 
> or some such?

"or some such" is not functionally equivalent.

This is what would be functionally equivalent, and following your
suggestion to check the return code of of_property_read_u32 instead of
assigning default values, and to use clamp() instead of open-coding the
bounds checks.

static int sja1105_parse_rgmii_delays(struct sja1105_private *priv, int port,
				      struct device_node *port_dn)
{
	phy_interface_t phy_mode = priv->phy_mode[port];
	struct device *dev = &priv->spidev->dev;
	int rx_delay, tx_delay;
	int err_rx, err_tx;

	if (!phy_interface_mode_is_rgmii(phy_mode))
		return 0;

	err_rx = of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay);
	if (err_rx)
		rx_delay = 0;

	err_tx = of_property_read_u32(port_dn, "tx-internal-delay-ps", &tx_delay);
	if (err_tx)
		tx_delay = 0;

	if (err_rx && err_tx) {
		if (priv->fixed_link[port]) {
			dev_warn(dev,
				 "Port %d interpreting RGMII delay settings based on \"phy-mode\" property, "
				 "please update device tree to specify \"rx-internal-delay-ps\" and "
				 "\"tx-internal-delay-ps\"",
				 port);

			if (phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
			    phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
				rx_delay = 2000;

			if (phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
			    phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
				tx_delay = 2000;
		}
	} else {
		if ((rx_delay && rx_delay != clamp(rx_delay, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS)) ||
		    (tx_delay && tx_delay != clamp(tx_delay, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS))) {
			dev_err(dev,
				"port %d RGMII delay values out of range, must be between %d and %d ps\n",
				port, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS);
			return -ERANGE;
		}
	}

	priv->rgmii_rx_delay_ps[port] = rx_delay;
	priv->rgmii_tx_delay_ps[port] = tx_delay;

	return 0;
}

> 
> > +	if ((rx_delay && rx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
> > +	    (tx_delay && tx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
> > +	    (rx_delay > SJA1105_RGMII_DELAY_MAX_PS) ||
> > +	    (tx_delay > SJA1105_RGMII_DELAY_MAX_PS)) {
> 
> nit: checkpatch says the brackets around the latter two are unnecessary,
>      just in case it's not for symmetry / on purpose

It is on purpose.

Re: [PATCH net-next 5/6] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps

[Rob Herring]

On Thu, 14 Oct 2021 01:23:12 +0300, Vladimir Oltean wrote:
> Add a schema validator to nxp,sja1105.yaml and to dsa.yaml for explicit
> MAC-level RGMII delays. These properties must be per port and must be
> present only for a phy-mode that represents RGMII.
> 
> We tell dsa.yaml that these port properties might be present, we also
> define their valid values for SJA1105. We create a common definition for
> the RX and TX valid range, since it's quite a mouthful.
> 
> We also modify the example to include the explicit RGMII delay properties.
> On the fixed-link ports (in the example, port 4), having these explicit
> delays is actually mandatory, since with the new behavior, the driver
> shouts that it is interpreting what delays to apply based on phy-mode.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  .../devicetree/bindings/net/dsa/dsa.yaml      |  4 ++
>  .../bindings/net/dsa/nxp,sja1105.yaml         | 42 +++++++++++++++++++
>  2 files changed, 46 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:
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:82:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:84:17: [warning] wrong indentation: expected 14 but found 16 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:85:21: [warning] wrong indentation: expected 18 but found 20 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:86:25: [warning] wrong indentation: expected 22 but found 24 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:93:17: [warning] wrong indentation: expected 14 but found 16 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:94:21: [warning] wrong indentation: expected 18 but found 20 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:96:21: [warning] wrong indentation: expected 18 but found 20 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:103:5: [warning] wrong indentation: expected 2 but found 4 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:104:9: [warning] wrong indentation: expected 6 but found 8 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:105:13: [warning] wrong indentation: expected 10 but found 12 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:109:13: [warning] wrong indentation: expected 10 but found 12 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:110:13: [warning] wrong indentation: expected 13 but found 12 (indentation)
./Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml:111:13: [warning] wrong indentation: expected 13 but found 12 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1540707

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.

Re: [PATCH net-next 3/6] dt-bindings: net: dsa: sja1105: fix example so all ports have a phy-handle of fixed-link

[Florian Fainelli]

On 10/13/21 3:23 PM, Vladimir Oltean wrote:
> All ports require either a phy-handle or a fixed-link, and port 3 in the
> example didn't have one. Add it.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 4/6] dt-bindings: net: dsa: inherit the ethernet-controller DT schema

[Florian Fainelli]

On 10/13/21 3:23 PM, Vladimir Oltean wrote:
> Since a switch is basically a bunch of Ethernet controllers, just
> inherit the common schema for one to get stronger type validation of the
> properties of a port.
> 
> For example, before this change it was valid to have a phy-mode = "xfi"
> even if "xfi" is not part of ethernet-controller.yaml, now it is not.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 5/6] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps

[Florian Fainelli]

On 10/13/21 3:23 PM, Vladimir Oltean wrote:
> Add a schema validator to nxp,sja1105.yaml and to dsa.yaml for explicit
> MAC-level RGMII delays. These properties must be per port and must be
> present only for a phy-mode that represents RGMII.
> 
> We tell dsa.yaml that these port properties might be present, we also
> define their valid values for SJA1105. We create a common definition for
> the RX and TX valid range, since it's quite a mouthful.
> 
> We also modify the example to include the explicit RGMII delay properties.
> On the fixed-link ports (in the example, port 4), having these explicit
> delays is actually mandatory, since with the new behavior, the driver
> shouts that it is interpreting what delays to apply based on phy-mode.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Even if you need to make yamllint happy
-- 
Florian

Re: [PATCH net-next 6/6] net: dsa: sja1105: parse {rx,tx}-internal-delay-ps properties for RGMII delays

[Florian Fainelli]

On 10/13/21 3:23 PM, Vladimir Oltean wrote:
> This change does not fix any functional issue or address any real life
> use case that wasn't possible before. It is just a small step in the
> process of standardizing the way in which Ethernet MAC drivers may apply
> RGMII delays (traditionally these have been applied by PHYs, with no
> clear definition of what to do in the case of a fixed-link).
> 
> The sja1105 driver used to apply MAC-level RGMII delays on the RX data
> lines when in fixed-link mode and using a phy-mode of "rgmii-rxid" or
> "rgmii-id" and on the TX data lines when using "rgmii-txid" or "rgmii-id".
> But the standard definitions don't say anything about behaving
> differently when the port is in fixed-link vs when it isn't, and the new
> device tree bindings are about having a way of applying the delays in a
> way that is independent of the phy-mode and of the fixed-link property.
> 
> When the {rx,tx}-internal-delay-ps properties are present, use them,
> otherwise fall back to the old behavior and warn.
> 
> One other thing to note is that the SJA1105 hardware applies a delay
> value in degrees rather than in picoseconds (the delay in ps changes
> depending on the frequency of the RGMII clock - 125 MHz at 1G, 25 MHz at
> 100M, 2.5MHz at 10M). I assume that is fine, we calculate the phase
> shift of the internal delay lines assuming that the device tree meant
> gigabit, and we let the hardware scale those according to the link speed.
> 
> Link: https://patchwork.kernel.org/project/netdevbpf/patch/20210723173108.459770-6-prasanna.vengateshan@microchip.com/
> Link: https://patchwork.ozlabs.org/project/netdev/patch/20200616074955.GA9092@laureti-dev/#2461123
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

FWIW, this is definitively a step in the right direction, and this is a
good way to deal with the phy-mode RGMII mess that has plagued us, thanks!
-- 
Florian

Re: [PATCH net-next 1/6] ARM: dts: imx6qp-prtwd3: update RGMII delays for sja1105 switch

[Florian Fainelli]

On 10/13/21 3:23 PM, Vladimir Oltean wrote:
> In the new behavior, the sja1105 driver expects there to be explicit
> RGMII delays present on the fixed-link ports, otherwise it will complain
> that it falls back to legacy behavior, which is to apply RGMII delays
> incorrectly derived from the phy-mode string.
> 
> In this case, the legacy behavior of the driver is to apply both RX and
> TX delays. To preserve that, add explicit 2 nanosecond delays, which are
> identical with what the driver used to add (a 90 degree phase shift).
> The delays from the phy-mode are ignored by new kernels (it's still
> RGMII as long as it's "rgmii*" something), and the explicit
> {rx,tx}-internal-delay-ps properties are ignored by old kernels, so the
> change works both ways.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/6] ARM: dts: ls1021a-tsn: update RGMII delays for sja1105 switch

[Florian Fainelli]

On 10/13/21 3:23 PM, Vladimir Oltean wrote:
> In the new behavior, the sja1105 driver expects there to be explicit
> RGMII delays present on the fixed-link ports, otherwise it will complain
> that it falls back to legacy behavior, which is to apply RGMII delays
> incorrectly derived from the phy-mode string.
> 
> In this case, the legacy behavior of the driver is to not apply delays
> in any direction (mostly because the SJA1105T can't do that, so this
> board uses PCB traces). To preserve that but also silence the driver,
> use explicit delays of 0 ns. The delay information from the phy-mode is
> ignored by new kernels (it's still RGMII as long as it's "rgmii*"
> something), and the explicit {rx,tx}-internal-delay-ps properties are
> ignored by old kernels, so the change works both ways.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 5/6] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps

[Jakub Kicinski]

On Thu, 14 Oct 2021 09:31:04 -0500 Rob Herring wrote:
> On Thu, 14 Oct 2021 01:23:12 +0300, Vladimir Oltean wrote:
> > Add a schema validator to nxp,sja1105.yaml and to dsa.yaml for explicit
> > MAC-level RGMII delays. These properties must be per port and must be
> > present only for a phy-mode that represents RGMII.
> > 
> > We tell dsa.yaml that these port properties might be present, we also
> > define their valid values for SJA1105. We create a common definition for
> > the RX and TX valid range, since it's quite a mouthful.
> > 
> > We also modify the example to include the explicit RGMII delay properties.
> > On the fixed-link ports (in the example, port 4), having these explicit
> > delays is actually mandatory, since with the new behavior, the driver
> > shouts that it is interpreting what delays to apply based on phy-mode.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):

FWIW I dropped the set from pw based on Rob's report, I see a mention
of possible issues with fsl-lx2160a-bluebox3.dts, but it's not clear
to me which DT is disagreeing with the schema.. or is the schema itself
not 100?

Re: [PATCH net-next 5/6] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps

[Vladimir Oltean]

On Fri, Oct 15, 2021 at 12:55:27PM -0700, Jakub Kicinski wrote:
> On Thu, 14 Oct 2021 09:31:04 -0500 Rob Herring wrote:
> > On Thu, 14 Oct 2021 01:23:12 +0300, Vladimir Oltean wrote:
> > > Add a schema validator to nxp,sja1105.yaml and to dsa.yaml for explicit
> > > MAC-level RGMII delays. These properties must be per port and must be
> > > present only for a phy-mode that represents RGMII.
> > >
> > > We tell dsa.yaml that these port properties might be present, we also
> > > define their valid values for SJA1105. We create a common definition for
> > > the RX and TX valid range, since it's quite a mouthful.
> > >
> > > We also modify the example to include the explicit RGMII delay properties.
> > > On the fixed-link ports (in the example, port 4), having these explicit
> > > delays is actually mandatory, since with the new behavior, the driver
> > > shouts that it is interpreting what delays to apply based on phy-mode.
> > >
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> FWIW I dropped the set from pw based on Rob's report, I see a mention
> of possible issues with fsl-lx2160a-bluebox3.dts, but it's not clear
> to me which DT is disagreeing with the schema.. or is the schema itself
> not 100?

I am only saying that I am introducing a new DT binding scheme and
warning all users of the old one. That's also why I am updating the
device trees, to silence the newly introduced warnings. I would like
this series to go through net-next, but fsl-lx2160a-bluebox3.dts isn't
in net-next.

Re: [PATCH net-next 5/6] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps

[Rob Herring]

On Fri, Oct 15, 2021 at 5:48 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Fri, Oct 15, 2021 at 12:55:27PM -0700, Jakub Kicinski wrote:
> > On Thu, 14 Oct 2021 09:31:04 -0500 Rob Herring wrote:
> > > On Thu, 14 Oct 2021 01:23:12 +0300, Vladimir Oltean wrote:
> > > > Add a schema validator to nxp,sja1105.yaml and to dsa.yaml for explicit
> > > > MAC-level RGMII delays. These properties must be per port and must be
> > > > present only for a phy-mode that represents RGMII.
> > > >
> > > > We tell dsa.yaml that these port properties might be present, we also
> > > > define their valid values for SJA1105. We create a common definition for
> > > > the RX and TX valid range, since it's quite a mouthful.
> > > >
> > > > We also modify the example to include the explicit RGMII delay properties.
> > > > On the fixed-link ports (in the example, port 4), having these explicit
> > > > delays is actually mandatory, since with the new behavior, the driver
> > > > shouts that it is interpreting what delays to apply based on phy-mode.
> > > >
> > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > >
> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >
> > FWIW I dropped the set from pw based on Rob's report, I see a mention
> > of possible issues with fsl-lx2160a-bluebox3.dts, but it's not clear
> > to me which DT is disagreeing with the schema.. or is the schema itself
> > not 100?
>
> I am only saying that I am introducing a new DT binding scheme and
> warning all users of the old one. That's also why I am updating the
> device trees, to silence the newly introduced warnings. I would like
> this series to go through net-next, but fsl-lx2160a-bluebox3.dts isn't
> in net-next.

.dts changes should go in via the sub-arch's tree, not a subsystem tree.

Rob