Message ID | 20190413012822.30931-21-olteanv@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | NXP SJA1105 DSA driver | expand |
On Sat, Apr 13, 2019 at 04:28:18AM +0300, Vladimir Oltean 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. That is a good interpretation. I always recommend the PHY does the delay, because in general the PHY can, and often the MAC cannot. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
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. > .reset_cmd = sja1105pqrs_reset_cmd, > .name = "SJA1105S", > }; >-- >2.17.1 >
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? Thanks, -Vladimir > > > .reset_cmd = sja1105pqrs_reset_cmd, > > .name = "SJA1105S", > > }; > >-- > >2.17.1 > >
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 >> >
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, .reset_cmd = sja1105pqrs_reset_cmd, .name = "SJA1105S", };