LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: f.fainelli@gmail.com, vivien.didelot@gmail.com, andrew@lunn.ch,
	davem@davemloft.net
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Vladimir Oltean <olteanv@gmail.com>
Subject: [PATCH v4 net-next 04/12] net: dsa: sja1105: Error out if RGMII delays are requested in DT
Date: Mon, 29 Apr 2019 03:16:58 +0300	[thread overview]
Message-ID: <20190429001706.7449-5-olteanv@gmail.com> (raw)
In-Reply-To: <20190429001706.7449-1-olteanv@gmail.com>

Documentation/devicetree/bindings/net/ethernet.txt is confusing because
it says what the MAC should not do, but not what it *should* do:

  * "rgmii-rxid" (RGMII with internal RX delay provided by the PHY, the MAC
     should not add an RX delay in this case)

The gap in semantics is threefold:
1. Is it illegal for the MAC to apply the Rx internal delay by itself,
   and simplify the phy_mode (mask off "rgmii-rxid" into "rgmii") before
   passing it to of_phy_connect? The documentation would suggest yes.
1. For "rgmii-rxid", while the situation with the Rx clock skew is more
   or less clear (needs to be added by the PHY), what should the MAC
   driver do about the Tx delays? Is it an implicit wild card for the
   MAC to apply delays in the Tx direction if it can? What if those were
   already added as serpentine PCB traces, how could that be made more
   obvious through DT bindings so that the MAC doesn't attempt to add
   them twice and again potentially break the link?
3. If the interface is a fixed-link and therefore the PHY object is
   fixed (a purely software entity that obviously cannot add clock
   skew), what is the meaning of the above property?

So an interpretation of the RGMII bindings was chosen that hopefully
does not contradict their intention but also makes them more applied.
The SJA1105 driver understands to act upon "rgmii-*id" phy-mode bindings
if the port is in the PHY role (either explicitly, or if it is a
fixed-link). Otherwise it always passes the duty of setting up delays to
the PHY driver.

The error behavior that this patch adds is required on SJA1105E/T where
the MAC really cannot apply internal delays. If the other end of the
fixed-link cannot apply RGMII delays either (this would be specified
through its own DT bindings), then the situation requires PCB delays.

For SJA1105P/Q/R/S, this is however hardware supported and the error is
thus only temporary. I created a stub function pointer for configuring
delays per-port on RXC and TXC, and will implement it when I have access
to a board with this hardware setup.

Meanwhile do not allow the user to select an invalid configuration.

Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
Changes in v4:
Moved RGMII delay settings to an array in sja1105_private. The
sja1105_port structure will be used to share info with the tagger (in a
future patchset), and RGMII delays are not an example of that.
Not setting to NULL function pointers that don't need to.

Changes in v3:
None.

Changes in v2:
Patch is new.

 drivers/net/dsa/sja1105/sja1105.h          |  3 ++
 drivers/net/dsa/sja1105/sja1105_clocking.c |  5 +++-
 drivers/net/dsa/sja1105/sja1105_main.c     | 34 ++++++++++++++++++++++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 1474830f963c..5c0bd62c9e74 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -48,11 +48,14 @@ struct sja1105_info {
 	const struct sja1105_table_ops *static_ops;
 	const struct sja1105_regs *regs;
 	int (*reset_cmd)(const void *ctx, const void *data);
+	int (*setup_rgmii_delay)(const void *ctx, int port);
 	const char *name;
 };
 
 struct sja1105_private {
 	struct sja1105_static_config static_config;
+	bool rgmii_rx_delay[SJA1105_NUM_PORTS];
+	bool rgmii_tx_delay[SJA1105_NUM_PORTS];
 	const struct sja1105_info *info;
 	struct gpio_desc *reset_gpio;
 	struct spi_device *spidev;
diff --git a/drivers/net/dsa/sja1105/sja1105_clocking.c b/drivers/net/dsa/sja1105/sja1105_clocking.c
index dd20670fff65..01ff395f2893 100644
--- a/drivers/net/dsa/sja1105/sja1105_clocking.c
+++ b/drivers/net/dsa/sja1105/sja1105_clocking.c
@@ -427,7 +427,10 @@ static int sja1105_rgmii_clocking_setup(struct sja1105_private *priv, int port)
 		dev_err(dev, "Failed to configure Tx pad registers\n");
 		return rc;
 	}
-	return 0;
+	if (!priv->info->setup_rgmii_delay)
+		return 0;
+
+	return priv->info->setup_rgmii_delay(priv, port);
 }
 
 static int sja1105_cgu_rmii_ref_clk_config(struct sja1105_private *priv,
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index ec8137eff223..d27b9c178cba 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -518,6 +518,30 @@ 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,
+				      const struct sja1105_dt_port *ports)
+{
+	int i;
+
+	for (i = 0; i < SJA1105_NUM_PORTS; i++) {
+		if (ports->role == XMII_MAC)
+			continue;
+
+		if (ports->phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
+		    ports->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
+			priv->rgmii_rx_delay[i] = true;
+
+		if (ports->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
+		    ports->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
+			priv->rgmii_tx_delay[i] = true;
+
+		if ((priv->rgmii_rx_delay[i] || priv->rgmii_tx_delay[i]) &&
+		     !priv->info->setup_rgmii_delay)
+			return -EINVAL;
+	}
+	return 0;
+}
+
 static int sja1105_parse_ports_node(struct sja1105_private *priv,
 				    struct sja1105_dt_port *ports,
 				    struct device_node *ports_node)
@@ -959,6 +983,16 @@ static int sja1105_setup(struct dsa_switch *ds)
 		dev_err(ds->dev, "Failed to parse DT: %d\n", rc);
 		return rc;
 	}
+
+	/* Error out early if internal delays are required through DT
+	 * and we can't apply them.
+	 */
+	rc = sja1105_parse_rgmii_delays(priv, ports);
+	if (rc < 0) {
+		dev_err(ds->dev, "RGMII delay not supported\n");
+		return rc;
+	}
+
 	/* Create and send configuration down to device */
 	rc = sja1105_static_config_load(priv, ports);
 	if (rc < 0) {
-- 
2.17.1


  parent reply	other threads:[~2019-04-29  0:19 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-29  0:16 [PATCH v4 net-next 00/12] NXP SJA1105 DSA driver Vladimir Oltean
2019-04-29  0:16 ` [PATCH v4 net-next 01/12] lib: Add support for generic packing operations Vladimir Oltean
2019-04-29  0:16 ` [PATCH v4 net-next 02/12] net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch Vladimir Oltean
2019-04-29  0:16 ` [PATCH v4 net-next 03/12] net: dsa: sja1105: Add support for FDB and MDB management Vladimir Oltean
2019-04-29  0:16 ` Vladimir Oltean [this message]
2019-04-29  0:16 ` [PATCH v4 net-next 05/12] ether: Add dedicated Ethertype for pseudo-802.1Q DSA tagging Vladimir Oltean
2019-04-29 22:18   ` Andrew Lunn
2019-04-29  0:17 ` [PATCH v4 net-next 06/12] net: dsa: sja1105: Add support for VLAN operations Vladimir Oltean
2019-04-29  0:17 ` [PATCH v4 net-next 07/12] net: dsa: sja1105: Add support for ethtool port counters Vladimir Oltean
2019-04-29  0:17 ` [PATCH v4 net-next 08/12] net: dsa: sja1105: Add support for configuring address aging time Vladimir Oltean
2019-04-29 22:21   ` Andrew Lunn
2019-04-29  0:17 ` [PATCH v4 net-next 09/12] net: dsa: sja1105: Prevent PHY jabbering during switch reset Vladimir Oltean
2019-04-29  0:17 ` [PATCH v4 net-next 10/12] net: dsa: sja1105: Reject unsupported link modes for AN Vladimir Oltean
2019-04-29  0:17 ` [PATCH v4 net-next 11/12] Documentation: net: dsa: Add details about NXP SJA1105 driver Vladimir Oltean
2019-04-29  0:17 ` [PATCH v4 net-next 12/12] dt-bindings: net: dsa: Add documentation for " Vladimir Oltean
2019-05-01  3:44 ` [PATCH v4 net-next 00/12] NXP SJA1105 DSA driver David Miller
2019-05-02 16:31   ` Vladimir Oltean
2019-05-02 16:55     ` Andrew Lunn
2019-05-02 17:05       ` Vladimir Oltean
2019-05-02 19:44     ` David Miller
2019-05-02 16:25 ` Vladimir Oltean

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190429001706.7449-5-olteanv@gmail.com \
    --to=olteanv@gmail.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vivien.didelot@gmail.com \
    --subject='Re: [PATCH v4 net-next 04/12] net: dsa: sja1105: Error out if RGMII delays are requested in DT' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).