| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190413204737.GB2268@nanopsycho.orion>
Date: Sat, 13 Apr 2019 22:47:37 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Vladimir Oltean <olteanv@...il.com>
Cc: f.fainelli@...il.com, vivien.didelot@...il.com, andrew@...n.ch,
davem@...emloft.net, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, georg.waibel@...sor-technik.de
Subject: Re: [PATCH v3 net-next 20/24] net: dsa: sja1105: Error out if RGMII
delays are requested in DT
Sat, Apr 13, 2019 at 03:28:18AM CEST, olteanv@...il.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@...il.com>
>Reviewed-by: Florian Fainelli <f.fainelli@...il.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.
> .reset_cmd = sja1105pqrs_reset_cmd,
> .name = "SJA1105S",
> };
>--
>2.17.1
>
Powered by blists - more mailing lists