All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver
@ 2021-10-18 19:29 Vladimir Oltean
  2021-10-18 19:29 ` [PATCH net-next 1/4] dt-bindings: net: dsa: sja1105: fix example so all ports have a phy-handle of fixed-link Vladimir Oltean
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-18 19:29 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, netdev, linux-kernel
  Cc: devicetree, Rob Herring, Andrew Lunn, Heiner Kallweit,
	Russell King, Vivien Didelot, Florian Fainelli,
	Prasanna Vengateshan, Ansuel Smith, Alvin Šipraga

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.

Changes in v2:
- removed devicetree patches, to be sent via Shawn Guo's tree
- fixed yamllint indentation warnings

Vladimir Oltean (4):
  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 +++++++++
 drivers/net/dsa/sja1105/sja1105.h             | 25 ++++-
 drivers/net/dsa/sja1105/sja1105_clocking.c    | 35 +++----
 drivers/net/dsa/sja1105/sja1105_main.c        | 94 ++++++++++++++-----
 5 files changed, 157 insertions(+), 47 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net-next 1/4] dt-bindings: net: dsa: sja1105: fix example so all ports have a phy-handle of fixed-link
  2021-10-18 19:29 [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver Vladimir Oltean
@ 2021-10-18 19:29 ` Vladimir Oltean
  2021-10-18 19:29 ` [PATCH net-next 2/4] dt-bindings: net: dsa: inherit the ethernet-controller DT schema Vladimir Oltean
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-18 19:29 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, netdev, linux-kernel
  Cc: devicetree, Rob Herring, Andrew Lunn, Heiner Kallweit,
	Russell King, Vivien Didelot, Florian Fainelli,
	Prasanna Vengateshan, Ansuel Smith, Alvin Šipraga

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>
---
 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH net-next 2/4] dt-bindings: net: dsa: inherit the ethernet-controller DT schema
  2021-10-18 19:29 [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver Vladimir Oltean
  2021-10-18 19:29 ` [PATCH net-next 1/4] dt-bindings: net: dsa: sja1105: fix example so all ports have a phy-handle of fixed-link Vladimir Oltean
@ 2021-10-18 19:29 ` Vladimir Oltean
  2021-10-22 23:31   ` Rob Herring
  2021-10-18 19:29 ` [PATCH net-next 3/4] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps Vladimir Oltean
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-18 19:29 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, netdev, linux-kernel
  Cc: devicetree, Rob Herring, Andrew Lunn, Heiner Kallweit,
	Russell King, Vivien Didelot, Florian Fainelli,
	Prasanna Vengateshan, Ansuel Smith, Alvin Šipraga

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>
---
 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH net-next 3/4] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps
  2021-10-18 19:29 [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver Vladimir Oltean
  2021-10-18 19:29 ` [PATCH net-next 1/4] dt-bindings: net: dsa: sja1105: fix example so all ports have a phy-handle of fixed-link Vladimir Oltean
  2021-10-18 19:29 ` [PATCH net-next 2/4] dt-bindings: net: dsa: inherit the ethernet-controller DT schema Vladimir Oltean
@ 2021-10-18 19:29 ` Vladimir Oltean
  2021-10-18 19:29 ` [PATCH net-next 4/4] net: dsa: sja1105: parse {rx, tx}-internal-delay-ps properties for RGMII delays Vladimir Oltean
  2021-10-20 11:10 ` [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver patchwork-bot+netdevbpf
  4 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-18 19:29 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, netdev, linux-kernel
  Cc: devicetree, Rob Herring, Andrew Lunn, Heiner Kallweit,
	Russell King, Vivien Didelot, Florian Fainelli,
	Prasanna Vengateshan, Ansuel Smith, Alvin Šipraga

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>
---
 .../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..24cd733c11d1 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH net-next 4/4] net: dsa: sja1105: parse {rx, tx}-internal-delay-ps properties for RGMII delays
  2021-10-18 19:29 [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver Vladimir Oltean
                   ` (2 preceding siblings ...)
  2021-10-18 19:29 ` [PATCH net-next 3/4] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps Vladimir Oltean
@ 2021-10-18 19:29 ` Vladimir Oltean
  2021-10-20 11:10 ` [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver patchwork-bot+netdevbpf
  4 siblings, 0 replies; 7+ messages in thread
From: Vladimir Oltean @ 2021-10-18 19:29 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, netdev, linux-kernel
  Cc: devicetree, Rob Herring, Andrew Lunn, Heiner Kallweit,
	Russell King, Vivien Didelot, Florian Fainelli,
	Prasanna Vengateshan, Ansuel Smith, Alvin Šipraga

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>
---
 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver
  2021-10-18 19:29 [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver Vladimir Oltean
                   ` (3 preceding siblings ...)
  2021-10-18 19:29 ` [PATCH net-next 4/4] net: dsa: sja1105: parse {rx, tx}-internal-delay-ps properties for RGMII delays Vladimir Oltean
@ 2021-10-20 11:10 ` patchwork-bot+netdevbpf
  4 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-10-20 11:10 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: davem, kuba, netdev, linux-kernel, devicetree, robh+dt, andrew,
	hkallweit1, linux, vivien.didelot, f.fainelli,
	prasanna.vengateshan, ansuelsmth, alsi

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon, 18 Oct 2021 22:29:48 +0300 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net-next,1/4] dt-bindings: net: dsa: sja1105: fix example so all ports have a phy-handle of fixed-link
    https://git.kernel.org/netdev/net-next/c/7a414b6e1a1c
  - [net-next,2/4] dt-bindings: net: dsa: inherit the ethernet-controller DT schema
    https://git.kernel.org/netdev/net-next/c/e00eb643324c
  - [net-next,3/4] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps
    https://git.kernel.org/netdev/net-next/c/ac41ac81e331
  - [net-next,4/4] net: dsa: sja1105: parse {rx, tx}-internal-delay-ps properties for RGMII delays
    https://git.kernel.org/netdev/net-next/c/9ca482a246f0

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next 2/4] dt-bindings: net: dsa: inherit the ethernet-controller DT schema
  2021-10-18 19:29 ` [PATCH net-next 2/4] dt-bindings: net: dsa: inherit the ethernet-controller DT schema Vladimir Oltean
@ 2021-10-22 23:31   ` Rob Herring
  0 siblings, 0 replies; 7+ messages in thread
From: Rob Herring @ 2021-10-22 23:31 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: David S . Miller, Jakub Kicinski, netdev, linux-kernel,
	devicetree, Andrew Lunn, Heiner Kallweit, Russell King,
	Vivien Didelot, Florian Fainelli, Prasanna Vengateshan,
	Ansuel Smith, Alvin Šipraga

On Mon, Oct 18, 2021 at 10:29:50PM +0300, 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>
> ---
>  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#"

$ref: /schemas/net/ethernet-controller.yaml#

And don't need 'allOf'.

> +
>          properties:
>            reg:
>              description: Port number
> -- 
> 2.25.1
> 
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-10-22 23:31 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-18 19:29 [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver Vladimir Oltean
2021-10-18 19:29 ` [PATCH net-next 1/4] dt-bindings: net: dsa: sja1105: fix example so all ports have a phy-handle of fixed-link Vladimir Oltean
2021-10-18 19:29 ` [PATCH net-next 2/4] dt-bindings: net: dsa: inherit the ethernet-controller DT schema Vladimir Oltean
2021-10-22 23:31   ` Rob Herring
2021-10-18 19:29 ` [PATCH net-next 3/4] dt-bindings: net: dsa: sja1105: add {rx,tx}-internal-delay-ps Vladimir Oltean
2021-10-18 19:29 ` [PATCH net-next 4/4] net: dsa: sja1105: parse {rx, tx}-internal-delay-ps properties for RGMII delays Vladimir Oltean
2021-10-20 11:10 ` [PATCH net-next 0/4] New RGMII delay DT bindings for the SJA1105 DSA driver patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.