| Message ID | 20210508002920.19945-19-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Multiple improvement to qca8k stability | expand |
> + > + /* Assume only one port with rgmii-id mode */ Since this is only valid for the RGMII port, please look in that specific node for these properties. Andrew
On Sat, May 08, 2021 at 08:12:26PM +0200, Andrew Lunn wrote: > > + > > + /* Assume only one port with rgmii-id mode */ > > Since this is only valid for the RGMII port, please look in that > specific node for these properties. > > Andrew Sorry, can you clarify this? You mean that I should check in the phandle pointed by the phy-handle or that I should modify the logic to only check for one (and the unique in this case) rgmii port?
On Sun, May 09, 2021 at 01:58:07AM +0200, Ansuel Smith wrote: > On Sat, May 08, 2021 at 08:12:26PM +0200, Andrew Lunn wrote: > > > + > > > + /* Assume only one port with rgmii-id mode */ > > > > Since this is only valid for the RGMII port, please look in that > > specific node for these properties. > > > > Andrew > > Sorry, can you clarify this? You mean that I should check in the phandle > pointed by the phy-handle or that I should modify the logic to only > check for one (and the unique in this case) rgmii port? Despite there only being one register, it should be a property of the port. If future chips have multiple RGMII ports, i expect there will be multiple registers. To avoid confusion in the future, we should make this a proper to the port it applies to. So assuming the RGMII port is port 0: #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; label = "cpu"; ethernet = <&gmac1>; phy-mode = "rgmii"; fixed-link { speed = 1000; full-duplex; }; rx-internal-delay-ps = <2000>; tx-internal-delay-ps = <2000>; }; port@1 { reg = <1>; label = "lan1"; phy-handle = <&phy_port1>; }; port@2 { reg = <2>; label = "lan2"; phy-handle = <&phy_port2>; }; port@3 { reg = <3>; label = "lan3"; phy-handle = <&phy_port3>; }; port@4 { reg = <4>; label = "lan4"; phy-handle = <&phy_port4>; }; port@5 { reg = <5>; label = "wan"; phy-handle = <&phy_port5>; }; }; }; }; You also should validate that phy-mode is rgmii and only rgmii. You get into odd situations if it is any of the other three rgmii modes. Andrew
On Sun, May 09, 2021 at 03:07:15AM +0200, Andrew Lunn wrote: > On Sun, May 09, 2021 at 01:58:07AM +0200, Ansuel Smith wrote: > > On Sat, May 08, 2021 at 08:12:26PM +0200, Andrew Lunn wrote: > > > > + > > > > + /* Assume only one port with rgmii-id mode */ > > > > > > Since this is only valid for the RGMII port, please look in that > > > specific node for these properties. > > > > > > Andrew > > > > Sorry, can you clarify this? You mean that I should check in the phandle > > pointed by the phy-handle or that I should modify the logic to only > > check for one (and the unique in this case) rgmii port? > > Despite there only being one register, it should be a property of the > port. If future chips have multiple RGMII ports, i expect there will > be multiple registers. To avoid confusion in the future, we should > make this a proper to the port it applies to. So assuming the RGMII > port is port 0: > > #address-cells = <1>; > #size-cells = <0>; > port@0 { > reg = <0>; > label = "cpu"; > ethernet = <&gmac1>; > phy-mode = "rgmii"; > fixed-link { > speed = 1000; > full-duplex; > }; > rx-internal-delay-ps = <2000>; > tx-internal-delay-ps = <2000>; > }; > > port@1 { > reg = <1>; > label = "lan1"; > phy-handle = <&phy_port1>; > }; > > port@2 { > reg = <2>; > label = "lan2"; > phy-handle = <&phy_port2>; > }; > > port@3 { > reg = <3>; > label = "lan3"; > phy-handle = <&phy_port3>; > }; > > port@4 { > reg = <4>; > label = "lan4"; > phy-handle = <&phy_port4>; > }; > > port@5 { > reg = <5>; > label = "wan"; > phy-handle = <&phy_port5>; > }; > }; > }; > }; > > You also should validate that phy-mode is rgmii and only rgmii. You > get into odd situations if it is any of the other three rgmii modes. > > Andrew And that is the intention of the port. I didn't want the binding to be set on the switch node but on the rgmii node. Correct me if I'm wrong but isn't what this patch already do? In of_rgmii_delay I get the ports node of the current switch, iterate every port, find the one with the phy-mode set to "rgmii-id" and check if it's present any value. And save the value in the priv struct. The actual delay is applied in the phylink_mac_config only if the mode is set to rgmii-id. (the current code set by default a delay of 3000 with rgmii-id, so this is just to make this value configurable)
> And that is the intention of the port. I didn't want the binding to be > set on the switch node but on the rgmii node. Correct me if I'm wrong > but isn't what this patch already do? > > In of_rgmii_delay I get the ports node of the current switch, iterate > every port, find the one with the phy-mode set to "rgmii-id" You know this hardware only has one RGMII port, and you know which one it is. So search the list of ports to find that one particular port, and see if 'rgmii' is set as phy-mode and if so, what the delays are. This is a port property, so you need to look at that specific port, not any random port that might have rgmii set. Andrew
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c index 285cce4fab9f..0c53b6fdf6ee 100644 --- a/drivers/net/dsa/qca8k.c +++ b/drivers/net/dsa/qca8k.c @@ -789,6 +789,50 @@ 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) + ports = of_get_child_by_name(priv->dev->of_node, "ethernet-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, "tx-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_tx_delay = val; + } + } + + of_node_put(ports); + + return 0; +} + static int qca8k_setup(struct dsa_switch *ds) { @@ -814,6 +858,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); @@ -1026,8 +1074,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 (priv->switch_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 338277978ec0..a878486d9bcd 100644 --- a/drivers/net/dsa/qca8k.h +++ b/drivers/net/dsa/qca8k.h @@ -38,12 +38,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) @@ -254,6 +253,8 @@ struct qca8k_match_data { struct qca8k_priv { u8 switch_id; 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 | 54 +++++++++++++++++++++++++++++++++++++++-- drivers/net/dsa/qca8k.h | 11 +++++---- 2 files changed, 58 insertions(+), 7 deletions(-)