[RFC,net-next,v4,19/28] net: dsa: qca8k: make rgmii delay configurable

Message ID 20210508002920.19945-19-ansuelsmth@gmail.com
State New
Headers show
Series
  • Multiple improvement to qca8k stability
Related show

Commit Message

Ansuel Smith May 8, 2021, 12:29 a.m.
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(-)

Comments

Andrew Lunn May 8, 2021, 6:12 p.m. | #1
> +
> +	/* 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
Ansuel Smith May 8, 2021, 11:58 p.m. | #2
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?
Andrew Lunn May 9, 2021, 1:07 a.m. | #3
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
Ansuel Smith May 9, 2021, 1:17 a.m. | #4
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)
Andrew Lunn May 9, 2021, 1:27 a.m. | #5
> 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

Patch

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];