From: Jiri Pirko <jiri@resnulli.us>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>,
vivien.didelot@gmail.com, Andrew Lunn <andrew@lunn.ch>,
davem@davemloft.net, netdev <netdev@vger.kernel.org>,
linux-kernel@vger.kernel.org,
Georg Waibel <georg.waibel@sensor-technik.de>
Subject: Re: [PATCH v3 net-next 20/24] net: dsa: sja1105: Error out if RGMII delays are requested in DT
Date: Sun, 14 Apr 2019 10:35:21 +0200 [thread overview]
Message-ID: <20190414083521.GB24144@nanopsycho.orion> (raw)
In-Reply-To: <CA+h21hpGCOb8Vrsp-WFz+8ts0eZ=RBTF0u3DqQ9DTJbsBSN+ZQ@mail.gmail.com>
Sat, Apr 13, 2019 at 11:31:01PM CEST, olteanv@gmail.com wrote:
>On Sat, 13 Apr 2019 at 23:47, Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Sat, Apr 13, 2019 at 03:28:18AM CEST, olteanv@gmail.com wrote:
>> >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>
>> >---
>> >Changes in v3:
>> >None.
>> >
>> >Changes in v2:
>> >Patch is new.
>> >
>> > drivers/net/dsa/sja1105/sja1105.h | 3 ++
>> > drivers/net/dsa/sja1105/sja1105_clocking.c | 7 ++++-
>> > drivers/net/dsa/sja1105/sja1105_main.c | 32 +++++++++++++++++++++-
>> > drivers/net/dsa/sja1105/sja1105_spi.c | 6 ++++
>> > 4 files changed, 46 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
>> >index b7e745c0bb3a..3c16b991032c 100644
>> >--- a/drivers/net/dsa/sja1105/sja1105.h
>> >+++ b/drivers/net/dsa/sja1105/sja1105.h
>> >@@ -22,6 +22,8 @@
>> >
>> > struct sja1105_port {
>> > struct dsa_port *dp;
>> >+ bool rgmii_rx_delay;
>> >+ bool rgmii_tx_delay;
>> > struct work_struct xmit_work;
>> > struct sja1105_skb_ring xmit_ring;
>> > };
>> >@@ -61,6 +63,7 @@ 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, bool rx, bool tx);
>> > const char *name;
>> > };
>> >
>> >diff --git a/drivers/net/dsa/sja1105/sja1105_clocking.c b/drivers/net/dsa/sja1105/sja1105_clocking.c
>> >index d40da3d52464..c02fec181676 100644
>> >--- a/drivers/net/dsa/sja1105/sja1105_clocking.c
>> >+++ b/drivers/net/dsa/sja1105/sja1105_clocking.c
>> >@@ -432,7 +432,12 @@ static int 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,
>> >+ priv->ports[port].rgmii_rx_delay,
>> >+ priv->ports[port].rgmii_tx_delay);
>> > }
>> >
>> > 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 e4abf8fb2013..5f7ddb1da006 100644
>> >--- a/drivers/net/dsa/sja1105/sja1105_main.c
>> >+++ b/drivers/net/dsa/sja1105/sja1105_main.c
>> >@@ -555,6 +555,21 @@ static int sja1105_static_config_load(struct sja1105_private *priv,
>> > return sja1105_static_config_upload(priv);
>> > }
>> >
>> >+static void sja1105_parse_rgmii_delay(const struct sja1105_dt_port *in,
>> >+ struct sja1105_port *out)
>> >+{
>> >+ if (in->role == XMII_MAC)
>> >+ return;
>> >+
>> >+ if (in->phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
>> >+ in->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>> >+ out->rgmii_rx_delay = true;
>> >+
>> >+ if (in->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
>> >+ in->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>> >+ out->rgmii_tx_delay = true;
>> >+}
>> >+
>> > static int sja1105_parse_ports_node(struct sja1105_private *priv,
>> > struct sja1105_dt_port *ports,
>> > struct device_node *ports_node)
>> >@@ -1315,13 +1330,28 @@ static int sja1105_setup(struct dsa_switch *ds)
>> > {
>> > struct sja1105_dt_port ports[SJA1105_NUM_PORTS];
>> > struct sja1105_private *priv = ds->priv;
>> >- int rc;
>> >+ int rc, i;
>> >
>> > rc = sja1105_parse_dt(priv, ports);
>> > if (rc < 0) {
>> > 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.
>> >+ */
>> >+ for (i = 0; i < SJA1105_NUM_PORTS; i++) {
>> >+ sja1105_parse_rgmii_delay(&ports[i], &priv->ports[i]);
>> >+
>> >+ if ((priv->ports[i].rgmii_rx_delay ||
>> >+ priv->ports[i].rgmii_tx_delay) &&
>> >+ !priv->info->setup_rgmii_delay) {
>> >+ dev_err(ds->dev, "RGMII delay not supported\n");
>> >+ return -EINVAL;
>> >+ }
>> >+ }
>> >+
>> > /* Create and send configuration down to device */
>> > rc = sja1105_static_config_load(priv, ports);
>> > if (rc < 0) {
>> >diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
>> >index 09cb28e9be20..e4ef4d8048b2 100644
>> >--- a/drivers/net/dsa/sja1105/sja1105_spi.c
>> >+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
>> >@@ -499,6 +499,7 @@ struct sja1105_info sja1105e_info = {
>> > .part_no = SJA1105ET_PART_NO,
>> > .static_ops = sja1105e_table_ops,
>> > .dyn_ops = sja1105et_dyn_ops,
>> >+ .setup_rgmii_delay = NULL,
>> > .reset_cmd = sja1105et_reset_cmd,
>> > .regs = &sja1105et_regs,
>> > .name = "SJA1105E",
>> >@@ -508,6 +509,7 @@ struct sja1105_info sja1105t_info = {
>> > .part_no = SJA1105ET_PART_NO,
>> > .static_ops = sja1105t_table_ops,
>> > .dyn_ops = sja1105et_dyn_ops,
>> >+ .setup_rgmii_delay = NULL,
>> > .reset_cmd = sja1105et_reset_cmd,
>> > .regs = &sja1105et_regs,
>> > .name = "SJA1105T",
>> >@@ -517,6 +519,7 @@ struct sja1105_info sja1105p_info = {
>> > .part_no = SJA1105P_PART_NO,
>> > .static_ops = sja1105p_table_ops,
>> > .dyn_ops = sja1105pqrs_dyn_ops,
>> >+ .setup_rgmii_delay = NULL,
>> > .reset_cmd = sja1105pqrs_reset_cmd,
>> > .regs = &sja1105pqrs_regs,
>> > .name = "SJA1105P",
>> >@@ -526,6 +529,7 @@ struct sja1105_info sja1105q_info = {
>> > .part_no = SJA1105Q_PART_NO,
>> > .static_ops = sja1105q_table_ops,
>> > .dyn_ops = sja1105pqrs_dyn_ops,
>> >+ .setup_rgmii_delay = NULL,
>> > .reset_cmd = sja1105pqrs_reset_cmd,
>> > .regs = &sja1105pqrs_regs,
>> > .name = "SJA1105Q",
>> >@@ -535,6 +539,7 @@ struct sja1105_info sja1105r_info = {
>> > .part_no = SJA1105R_PART_NO,
>> > .static_ops = sja1105r_table_ops,
>> > .dyn_ops = sja1105pqrs_dyn_ops,
>> >+ .setup_rgmii_delay = NULL,
>> > .reset_cmd = sja1105pqrs_reset_cmd,
>> > .regs = &sja1105pqrs_regs,
>> > .name = "SJA1105R",
>> >@@ -545,6 +550,7 @@ struct sja1105_info sja1105s_info = {
>> > .static_ops = sja1105s_table_ops,
>> > .dyn_ops = sja1105pqrs_dyn_ops,
>> > .regs = &sja1105pqrs_regs,
>> >+ .setup_rgmii_delay = NULL,
>>
>> You don't need to set this to NULL. Please avoid that.
>>
>
>Hi Jiri, why not?
If you don't assign, it is already NULL. so the assignment to NULL is
pointless.
>
>Thanks,
>-Vladimir
>
>>
>> > .reset_cmd = sja1105pqrs_reset_cmd,
>> > .name = "SJA1105S",
>> > };
>> >--
>> >2.17.1
>> >
next prev parent reply other threads:[~2019-04-14 8:35 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-13 1:27 [PATCH v3 net-next 00/24] NXP SJA1105 DSA driver Vladimir Oltean
2019-04-13 1:27 ` [PATCH v3 net-next 01/24] lib: Add support for generic packing operations Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 02/24] net: dsa: Fix pharse -> phase typo Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 03/24] net: dsa: Store vlan_filtering as a property of dsa_port Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 04/24] net: dsa: mt7530: Use vlan_filtering property from dsa_port Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 05/24] net: dsa: Add more convenient functions for installing port VLANs Vladimir Oltean
2019-04-16 23:49 ` Florian Fainelli
2019-04-13 1:28 ` [PATCH v3 net-next 06/24] net: dsa: Call driver's setup callback after setting up its switchdev notifier Vladimir Oltean
2019-04-13 15:05 ` Andrew Lunn
2019-04-13 1:28 ` [PATCH v3 net-next 07/24] net: dsa: Optional VLAN-based port separation for switches without tagging Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 08/24] net: dsa: Be aware of switches where VLAN filtering is a global setting Vladimir Oltean
2019-04-16 23:54 ` Florian Fainelli
2019-04-13 1:28 ` [PATCH v3 net-next 09/24] net: dsa: b53: Let DSA handle mismatched VLAN filtering settings Vladimir Oltean
2019-04-16 23:52 ` Florian Fainelli
2019-04-13 1:28 ` [PATCH v3 net-next 10/24] net: dsa: Unset vlan_filtering when ports leave the bridge Vladimir Oltean
2019-04-13 15:11 ` Andrew Lunn
2019-04-16 23:59 ` Florian Fainelli
2019-04-13 1:28 ` [PATCH v3 net-next 11/24] net: dsa: mt7530: Let DSA handle the unsetting of vlan_filtering Vladimir Oltean
2019-04-13 15:12 ` Andrew Lunn
2019-04-16 23:59 ` Florian Fainelli
2019-04-13 1:28 ` [PATCH v3 net-next 12/24] net: dsa: Copy the vlan_filtering setting on the CPU port if it's global Vladimir Oltean
2019-04-13 15:23 ` Andrew Lunn
2019-04-13 15:37 ` Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 13/24] net: dsa: Allow drivers to filter packets they can decode source port from Vladimir Oltean
2019-04-13 15:39 ` Andrew Lunn
2019-04-13 15:48 ` Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 14/24] net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch Vladimir Oltean
2019-04-13 15:42 ` Andrew Lunn
2019-04-13 15:46 ` Vladimir Oltean
2019-04-13 16:44 ` Andrew Lunn
2019-04-13 21:29 ` Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 15/24] net: dsa: sja1105: Add support for FDB and MDB management Vladimir Oltean
2019-04-13 20:58 ` Jiri Pirko
2019-04-13 1:28 ` [PATCH v3 net-next 16/24] net: dsa: sja1105: Add support for VLAN operations Vladimir Oltean
2019-04-13 20:56 ` Jiri Pirko
2019-04-13 21:39 ` Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 17/24] net: dsa: sja1105: Add support for ethtool port counters Vladimir Oltean
2019-04-13 20:53 ` Jiri Pirko
2019-04-13 21:55 ` Vladimir Oltean
2019-04-14 8:34 ` Jiri Pirko
2019-04-13 1:28 ` [PATCH v3 net-next 18/24] net: dsa: sja1105: Add support for traffic through standalone ports Vladimir Oltean
2019-04-13 16:37 ` Andrew Lunn
2019-04-13 21:27 ` Vladimir Oltean
2019-04-13 22:08 ` Vladimir Oltean
2019-04-13 22:26 ` Vladimir Oltean
2019-04-14 16:17 ` Andrew Lunn
2019-04-14 18:53 ` Vladimir Oltean
2019-04-14 19:13 ` Andrew Lunn
2019-04-14 22:30 ` Vladimir Oltean
2019-04-15 3:07 ` Andrew Lunn
2019-04-17 0:09 ` Florian Fainelli
2019-04-14 16:05 ` Andrew Lunn
2019-04-14 18:42 ` Vladimir Oltean
2019-04-14 19:06 ` Andrew Lunn
2019-04-17 0:16 ` Florian Fainelli
2019-04-13 1:28 ` [PATCH v3 net-next 19/24] net: dsa: sja1105: Add support for Spanning Tree Protocol Vladimir Oltean
2019-04-13 16:41 ` Andrew Lunn
2019-04-13 1:28 ` [PATCH v3 net-next 20/24] net: dsa: sja1105: Error out if RGMII delays are requested in DT Vladimir Oltean
2019-04-13 16:49 ` Andrew Lunn
2019-04-13 20:47 ` Jiri Pirko
2019-04-13 21:31 ` Vladimir Oltean
2019-04-14 8:35 ` Jiri Pirko [this message]
2019-04-13 1:28 ` [PATCH v3 net-next 21/24] net: dsa: sja1105: Prevent PHY jabbering during switch reset Vladimir Oltean
2019-04-13 16:54 ` Andrew Lunn
2019-04-13 1:28 ` [PATCH v3 net-next 22/24] net: dsa: sja1105: Reject unsupported link modes for AN Vladimir Oltean
2019-04-13 1:28 ` [PATCH v3 net-next 23/24] Documentation: net: dsa: Add details about NXP SJA1105 driver Vladimir Oltean
2019-04-17 0:20 ` Florian Fainelli
2019-04-13 1:28 ` [PATCH v3 net-next 24/24] dt-bindings: net: dsa: Add documentation for " 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=20190414083521.GB24144@nanopsycho.orion \
--to=jiri@resnulli.us \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=georg.waibel@sensor-technik.de \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=vivien.didelot@gmail.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).