| Message ID | 20210504222915.17206-13-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next,v3,01/20] net: mdio: ipq8064: clean whitespaces in define | expand |
| Context | Check | Description |
|---|---|---|
| netdev/apply | fail | Patch does not apply to net-next |
| netdev/tree_selection | success | Clearly marked for net-next |
On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote: > The legacy qsdk code used a different delay instead of the max value. > Qsdk use 1 ps for rx and 2 ps for tx. Make these values configurable > using the standard rx/tx-internal-delay-ps ethernet binding and apply > qsdk values by default. The connected gmac doesn't add any delay so no > additional delay is added to tx/rx. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/net/dsa/qca8k.c | 51 +++++++++++++++++++++++++++++++++++++++-- > drivers/net/dsa/qca8k.h | 11 +++++---- > 2 files changed, 55 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > index 22334d416f53..cb9b44769e92 100644 > --- a/drivers/net/dsa/qca8k.c > +++ b/drivers/net/dsa/qca8k.c > @@ -779,6 +779,47 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv) > return 0; > } > > +static int > +qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv) > +{ > + struct device_node *ports, *port; > + u32 val; > + > + ports = of_get_child_by_name(priv->dev->of_node, "ports"); > + if (!ports) > + return -EINVAL; > + > + /* Assume only one port with rgmii-id mode */ > + for_each_available_child_of_node(ports, port) { Are delays global? Or per port? They really should be per port. If it is global, one value that applies to all ports, i would not use it. Have the PHY apply the delay, not the MAC. Andrew
On Wed, May 05, 2021 at 03:00:45AM +0200, Andrew Lunn wrote: > On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote: > > The legacy qsdk code used a different delay instead of the max value. > > Qsdk use 1 ps for rx and 2 ps for tx. Make these values configurable > > using the standard rx/tx-internal-delay-ps ethernet binding and apply > > qsdk values by default. The connected gmac doesn't add any delay so no > > additional delay is added to tx/rx. > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > drivers/net/dsa/qca8k.c | 51 +++++++++++++++++++++++++++++++++++++++-- > > drivers/net/dsa/qca8k.h | 11 +++++---- > > 2 files changed, 55 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > index 22334d416f53..cb9b44769e92 100644 > > --- a/drivers/net/dsa/qca8k.c > > +++ b/drivers/net/dsa/qca8k.c > > @@ -779,6 +779,47 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv) > > return 0; > > } > > > > +static int > > +qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv) > > +{ > > + struct device_node *ports, *port; > > + u32 val; > > + > > + ports = of_get_child_by_name(priv->dev->of_node, "ports"); > > + if (!ports) > > + return -EINVAL; > > + > > + /* Assume only one port with rgmii-id mode */ > > + for_each_available_child_of_node(ports, port) { > > Are delays global? Or per port? They really should be per port. If it > is global, one value that applies to all ports, i would not use > it. Have the PHY apply the delay, not the MAC. > > Andrew It's expected to set the delay in the port node. But yes the value is global and assigned to every port (in reality this is only assigned to the unique rgmii cpu port). The switch has only one rgmii port and one sgmii, that's why I skipped any logic about handling more than one case with internal delay. If you want I can try to add some logic to handle this value per port. But again on the switch there is only one reg to set the delay and the qca8k_phylink_mac_config function use this only for the rgmii cpu port.
On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote: > The legacy qsdk code used a different delay instead of the max value. > Qsdk use 1 ps for rx and 2 ps for tx. Make these values configurable > using the standard rx/tx-internal-delay-ps ethernet binding and apply > qsdk values by default. The connected gmac doesn't add any delay so no > additional delay is added to tx/rx. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/net/dsa/qca8k.c | 51 +++++++++++++++++++++++++++++++++++++++-- > drivers/net/dsa/qca8k.h | 11 +++++---- > 2 files changed, 55 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > index 22334d416f53..cb9b44769e92 100644 > --- a/drivers/net/dsa/qca8k.c > +++ b/drivers/net/dsa/qca8k.c > @@ -779,6 +779,47 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv) > return 0; > } > > +static int > +qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv) > +{ > + struct device_node *ports, *port; > + u32 val; > + > + ports = of_get_child_by_name(priv->dev->of_node, "ports"); Consider falling back to searching for the "ethernet-ports" name too, DSA should now support both. > + if (!ports) > + return -EINVAL; > + > + /* Assume only one port with rgmii-id mode */ > + for_each_available_child_of_node(ports, port) { > + if (!of_property_match_string(port, "phy-mode", "rgmii-id")) > + continue; > + > + if (of_property_read_u32(port, "rx-internal-delay-ps", &val)) > + val = 2; > + > + if (val > QCA8K_MAX_DELAY) { > + dev_err(priv->dev, "rgmii rx delay is limited to more than 3ps, setting to the max value"); > + priv->rgmii_rx_delay = 3; ?! 3 picoseconds is not a lot of clock skew for a 125/25/2.5 MHz clock. 3 nanoseconds maybe? > + } else { > + priv->rgmii_rx_delay = val; > + } > + > + if (of_property_read_u32(port, "rx-internal-delay-ps", &val)) > + val = 1; > + > + if (val > QCA8K_MAX_DELAY) { > + dev_err(priv->dev, "rgmii tx delay is limited to more than 3ps, setting to the max value"); > + priv->rgmii_tx_delay = 3; > + } else { > + priv->rgmii_rx_delay = val; > + } > + } > + > + of_node_put(ports); > + > + return 0; > +} > + > static int > qca8k_setup(struct dsa_switch *ds) > { > @@ -808,6 +849,10 @@ qca8k_setup(struct dsa_switch *ds) > if (ret) > return ret; > > + ret = qca8k_setup_of_rgmii_delay(priv); > + if (ret) > + return ret; > + > /* Enable CPU Port */ > ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0, > QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN); > @@ -1018,8 +1063,10 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, > */ > qca8k_write(priv, reg, > QCA8K_PORT_PAD_RGMII_EN | > - QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) | > - QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY)); > + QCA8K_PORT_PAD_RGMII_TX_DELAY(priv->rgmii_tx_delay) | > + QCA8K_PORT_PAD_RGMII_RX_DELAY(priv->rgmii_rx_delay) | > + QCA8K_PORT_PAD_RGMII_TX_DELAY_EN | > + QCA8K_PORT_PAD_RGMII_RX_DELAY_EN); > /* QCA8337 requires to set rgmii rx delay */ > if (data->id == QCA8K_ID_QCA8337) > qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL, > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h > index 0b503f78bf92..80830bb42736 100644 > --- a/drivers/net/dsa/qca8k.h > +++ b/drivers/net/dsa/qca8k.h > @@ -36,12 +36,11 @@ > #define QCA8K_REG_PORT5_PAD_CTRL 0x008 > #define QCA8K_REG_PORT6_PAD_CTRL 0x00c > #define QCA8K_PORT_PAD_RGMII_EN BIT(26) > -#define QCA8K_PORT_PAD_RGMII_TX_DELAY(x) \ > - ((0x8 + (x & 0x3)) << 22) > -#define QCA8K_PORT_PAD_RGMII_RX_DELAY(x) \ > - ((0x10 + (x & 0x3)) << 20) > -#define QCA8K_MAX_DELAY 3 > +#define QCA8K_PORT_PAD_RGMII_TX_DELAY(x) ((x) << 22) > +#define QCA8K_PORT_PAD_RGMII_RX_DELAY(x) ((x) << 20) > +#define QCA8K_PORT_PAD_RGMII_TX_DELAY_EN BIT(25) > #define QCA8K_PORT_PAD_RGMII_RX_DELAY_EN BIT(24) > +#define QCA8K_MAX_DELAY 3 > #define QCA8K_PORT_PAD_SGMII_EN BIT(7) > #define QCA8K_REG_PWS 0x010 > #define QCA8K_PWS_SERDES_AEN_DIS BIT(7) > @@ -251,6 +250,8 @@ struct qca8k_match_data { > > struct qca8k_priv { > u8 switch_revision; > + u8 rgmii_tx_delay; > + u8 rgmii_rx_delay; > struct regmap *regmap; > struct mii_bus *bus; > struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS]; > -- > 2.30.2 >
On Thu, May 06, 2021 at 02:10:33PM +0300, Vladimir Oltean wrote: > On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote: > > The legacy qsdk code used a different delay instead of the max value. > > Qsdk use 1 ps for rx and 2 ps for tx. Make these values configurable > > using the standard rx/tx-internal-delay-ps ethernet binding and apply > > qsdk values by default. The connected gmac doesn't add any delay so no > > additional delay is added to tx/rx. > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > drivers/net/dsa/qca8k.c | 51 +++++++++++++++++++++++++++++++++++++++-- > > drivers/net/dsa/qca8k.h | 11 +++++---- > > 2 files changed, 55 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > index 22334d416f53..cb9b44769e92 100644 > > --- a/drivers/net/dsa/qca8k.c > > +++ b/drivers/net/dsa/qca8k.c > > @@ -779,6 +779,47 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv) > > return 0; > > } > > > > +static int > > +qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv) > > +{ > > + struct device_node *ports, *port; > > + u32 val; > > + > > + ports = of_get_child_by_name(priv->dev->of_node, "ports"); > > Consider falling back to searching for the "ethernet-ports" name too, > DSA should now support both. > The function qca8k_setup_mdio_bus also checks for ports node. Should I also there the fallback correct? > > + if (!ports) > > + return -EINVAL; > > + > > + /* Assume only one port with rgmii-id mode */ > > + for_each_available_child_of_node(ports, port) { > > + if (!of_property_match_string(port, "phy-mode", "rgmii-id")) > > + continue; > > + > > + if (of_property_read_u32(port, "rx-internal-delay-ps", &val)) > > + val = 2; > > + > > + if (val > QCA8K_MAX_DELAY) { > > + dev_err(priv->dev, "rgmii rx delay is limited to more than 3ps, setting to the max value"); > > + priv->rgmii_rx_delay = 3; > > ?! > 3 picoseconds is not a lot of clock skew for a 125/25/2.5 MHz clock. 3 nanoseconds maybe? > > > + } else { > > + priv->rgmii_rx_delay = val; > > + } > > + > > + if (of_property_read_u32(port, "rx-internal-delay-ps", &val)) > > + val = 1; > > + > > + if (val > QCA8K_MAX_DELAY) { > > + dev_err(priv->dev, "rgmii tx delay is limited to more than 3ps, setting to the max value"); > > + priv->rgmii_tx_delay = 3; > > + } else { > > + priv->rgmii_rx_delay = val; > > + } > > + } > > + > > + of_node_put(ports); > > + > > + return 0; > > +} > > + > > static int > > qca8k_setup(struct dsa_switch *ds) > > { > > @@ -808,6 +849,10 @@ qca8k_setup(struct dsa_switch *ds) > > if (ret) > > return ret; > > > > + ret = qca8k_setup_of_rgmii_delay(priv); > > + if (ret) > > + return ret; > > + > > /* Enable CPU Port */ > > ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0, > > QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN); > > @@ -1018,8 +1063,10 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, > > */ > > qca8k_write(priv, reg, > > QCA8K_PORT_PAD_RGMII_EN | > > - QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) | > > - QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY)); > > + QCA8K_PORT_PAD_RGMII_TX_DELAY(priv->rgmii_tx_delay) | > > + QCA8K_PORT_PAD_RGMII_RX_DELAY(priv->rgmii_rx_delay) | > > + QCA8K_PORT_PAD_RGMII_TX_DELAY_EN | > > + QCA8K_PORT_PAD_RGMII_RX_DELAY_EN); > > /* QCA8337 requires to set rgmii rx delay */ > > if (data->id == QCA8K_ID_QCA8337) > > qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL, > > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h > > index 0b503f78bf92..80830bb42736 100644 > > --- a/drivers/net/dsa/qca8k.h > > +++ b/drivers/net/dsa/qca8k.h > > @@ -36,12 +36,11 @@ > > #define QCA8K_REG_PORT5_PAD_CTRL 0x008 > > #define QCA8K_REG_PORT6_PAD_CTRL 0x00c > > #define QCA8K_PORT_PAD_RGMII_EN BIT(26) > > -#define QCA8K_PORT_PAD_RGMII_TX_DELAY(x) \ > > - ((0x8 + (x & 0x3)) << 22) > > -#define QCA8K_PORT_PAD_RGMII_RX_DELAY(x) \ > > - ((0x10 + (x & 0x3)) << 20) > > -#define QCA8K_MAX_DELAY 3 > > +#define QCA8K_PORT_PAD_RGMII_TX_DELAY(x) ((x) << 22) > > +#define QCA8K_PORT_PAD_RGMII_RX_DELAY(x) ((x) << 20) > > +#define QCA8K_PORT_PAD_RGMII_TX_DELAY_EN BIT(25) > > #define QCA8K_PORT_PAD_RGMII_RX_DELAY_EN BIT(24) > > +#define QCA8K_MAX_DELAY 3 > > #define QCA8K_PORT_PAD_SGMII_EN BIT(7) > > #define QCA8K_REG_PWS 0x010 > > #define QCA8K_PWS_SERDES_AEN_DIS BIT(7) > > @@ -251,6 +250,8 @@ struct qca8k_match_data { > > > > struct qca8k_priv { > > u8 switch_revision; > > + u8 rgmii_tx_delay; > > + u8 rgmii_rx_delay; > > struct regmap *regmap; > > struct mii_bus *bus; > > struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS]; > > -- > > 2.30.2 > >
On Thu, May 06, 2021 at 11:53:36PM +0200, Ansuel Smith wrote: > On Thu, May 06, 2021 at 02:10:33PM +0300, Vladimir Oltean wrote: > > On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote: > > > The legacy qsdk code used a different delay instead of the max value. > > > Qsdk use 1 ps for rx and 2 ps for tx. Make these values configurable > > > using the standard rx/tx-internal-delay-ps ethernet binding and apply > > > qsdk values by default. The connected gmac doesn't add any delay so no > > > additional delay is added to tx/rx. > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > --- > > > drivers/net/dsa/qca8k.c | 51 +++++++++++++++++++++++++++++++++++++++-- > > > drivers/net/dsa/qca8k.h | 11 +++++---- > > > 2 files changed, 55 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > > index 22334d416f53..cb9b44769e92 100644 > > > --- a/drivers/net/dsa/qca8k.c > > > +++ b/drivers/net/dsa/qca8k.c > > > @@ -779,6 +779,47 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv) > > > return 0; > > > } > > > > > > +static int > > > +qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv) > > > +{ > > > + struct device_node *ports, *port; > > > + u32 val; > > > + > > > + ports = of_get_child_by_name(priv->dev->of_node, "ports"); > > > > Consider falling back to searching for the "ethernet-ports" name too, > > DSA should now support both. > > > > The function qca8k_setup_mdio_bus also checks for ports node. Should I > also there the fallback correct? Yes, ideally the entire driver should work fine with "ethernet-ports" and not just pieces of it.
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c index 22334d416f53..cb9b44769e92 100644 --- a/drivers/net/dsa/qca8k.c +++ b/drivers/net/dsa/qca8k.c @@ -779,6 +779,47 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv) return 0; } +static int +qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv) +{ + struct device_node *ports, *port; + u32 val; + + ports = of_get_child_by_name(priv->dev->of_node, "ports"); + if (!ports) + return -EINVAL; + + /* Assume only one port with rgmii-id mode */ + for_each_available_child_of_node(ports, port) { + if (!of_property_match_string(port, "phy-mode", "rgmii-id")) + continue; + + if (of_property_read_u32(port, "rx-internal-delay-ps", &val)) + val = 2; + + if (val > QCA8K_MAX_DELAY) { + dev_err(priv->dev, "rgmii rx delay is limited to more than 3ps, setting to the max value"); + priv->rgmii_rx_delay = 3; + } else { + priv->rgmii_rx_delay = val; + } + + if (of_property_read_u32(port, "rx-internal-delay-ps", &val)) + val = 1; + + if (val > QCA8K_MAX_DELAY) { + dev_err(priv->dev, "rgmii tx delay is limited to more than 3ps, setting to the max value"); + priv->rgmii_tx_delay = 3; + } else { + priv->rgmii_rx_delay = val; + } + } + + of_node_put(ports); + + return 0; +} + static int qca8k_setup(struct dsa_switch *ds) { @@ -808,6 +849,10 @@ qca8k_setup(struct dsa_switch *ds) if (ret) return ret; + ret = qca8k_setup_of_rgmii_delay(priv); + if (ret) + return ret; + /* Enable CPU Port */ ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0, QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN); @@ -1018,8 +1063,10 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode, */ qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN | - QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) | - QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY)); + QCA8K_PORT_PAD_RGMII_TX_DELAY(priv->rgmii_tx_delay) | + QCA8K_PORT_PAD_RGMII_RX_DELAY(priv->rgmii_rx_delay) | + QCA8K_PORT_PAD_RGMII_TX_DELAY_EN | + QCA8K_PORT_PAD_RGMII_RX_DELAY_EN); /* QCA8337 requires to set rgmii rx delay */ if (data->id == QCA8K_ID_QCA8337) qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL, diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h index 0b503f78bf92..80830bb42736 100644 --- a/drivers/net/dsa/qca8k.h +++ b/drivers/net/dsa/qca8k.h @@ -36,12 +36,11 @@ #define QCA8K_REG_PORT5_PAD_CTRL 0x008 #define QCA8K_REG_PORT6_PAD_CTRL 0x00c #define QCA8K_PORT_PAD_RGMII_EN BIT(26) -#define QCA8K_PORT_PAD_RGMII_TX_DELAY(x) \ - ((0x8 + (x & 0x3)) << 22) -#define QCA8K_PORT_PAD_RGMII_RX_DELAY(x) \ - ((0x10 + (x & 0x3)) << 20) -#define QCA8K_MAX_DELAY 3 +#define QCA8K_PORT_PAD_RGMII_TX_DELAY(x) ((x) << 22) +#define QCA8K_PORT_PAD_RGMII_RX_DELAY(x) ((x) << 20) +#define QCA8K_PORT_PAD_RGMII_TX_DELAY_EN BIT(25) #define QCA8K_PORT_PAD_RGMII_RX_DELAY_EN BIT(24) +#define QCA8K_MAX_DELAY 3 #define QCA8K_PORT_PAD_SGMII_EN BIT(7) #define QCA8K_REG_PWS 0x010 #define QCA8K_PWS_SERDES_AEN_DIS BIT(7) @@ -251,6 +250,8 @@ struct qca8k_match_data { struct qca8k_priv { u8 switch_revision; + u8 rgmii_tx_delay; + u8 rgmii_rx_delay; struct regmap *regmap; struct mii_bus *bus; struct ar8xxx_port_status port_sts[QCA8K_NUM_PORTS];
The legacy qsdk code used a different delay instead of the max value. Qsdk use 1 ps for rx and 2 ps for tx. Make these values configurable using the standard rx/tx-internal-delay-ps ethernet binding and apply qsdk values by default. The connected gmac doesn't add any delay so no additional delay is added to tx/rx. Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/net/dsa/qca8k.c | 51 +++++++++++++++++++++++++++++++++++++++-- drivers/net/dsa/qca8k.h | 11 +++++---- 2 files changed, 55 insertions(+), 7 deletions(-)