[PATCH 0/6] Call phy_config at port probe time for the Felix DSA driver

[PATCH 0/6] Call phy_config at port probe time for the Felix DSA driver

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This series makes the Felix DSA driver initialize all its connected PHYs
regardless of whether those will be used for networking or not. This is
in order to satisfy the expectations of some software in later boot
stages.

To make this work, it is necessary to introduce a new method in struct
dsa_ops: .port_probe().

There is some further refactoring/cleanup along the way.

Vladimir Oltean (6):
  net: dsa: felix: felix_init() can be static
  net: dsa: use "err" instead of "ret" in dsa_port_probe
  net: dsa: refactor the code to set the port MAC address into a
    dedicated function
  net: dsa: introduce a .port_probe() method in struct dsa_ops
  net: dsa: felix: call phy_config at .port_probe() time
  net: dsa: felix: propagate the error code from phy_startup()

 drivers/net/mscc_eswitch/felix_switch.c | 28 ++++++++------
 include/net/dsa.h                       |  5 ++-
 net/dsa-uclass.c                        | 50 ++++++++++++++++---------
 3 files changed, 53 insertions(+), 30 deletions(-)

-- 
2.25.1

[PATCH 1/6] net: dsa: felix: felix_init() can be static

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

No one is calling this function from outside felix_switch.c.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/mscc_eswitch/felix_switch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mscc_eswitch/felix_switch.c b/drivers/net/mscc_eswitch/felix_switch.c
index f20e84e0f10c..75073880cf87 100644
--- a/drivers/net/mscc_eswitch/felix_switch.c
+++ b/drivers/net/mscc_eswitch/felix_switch.c
@@ -233,7 +233,7 @@ static void felix_start_pcs(struct udevice *dev, int port,
 	}
 }
 
-void felix_init(struct udevice *dev)
+static void felix_init(struct udevice *dev)
 {
 	struct dsa_pdata *pdata = dev_get_uclass_plat(dev);
 	struct felix_priv *priv = dev_get_priv(dev);
-- 
2.25.1

[PATCH 2/6] net: dsa: use "err" instead of "ret" in dsa_port_probe

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

DM DSA uses "err" for error code values, so use this consistently.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa-uclass.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
index 7ea1cb6949c0..17b33834f585 100644
--- a/net/dsa-uclass.c
+++ b/net/dsa-uclass.c
@@ -248,7 +248,7 @@ static int dsa_port_probe(struct udevice *pdev)
 	struct dsa_port_pdata *port_pdata;
 	struct dsa_priv *dsa_priv;
 	struct udevice *master;
-	int ret;
+	int err;
 
 	port_pdata = dev_get_parent_plat(pdev);
 	dsa_priv = dev_get_uclass_priv(dev);
@@ -268,9 +268,9 @@ static int dsa_port_probe(struct udevice *pdev)
 	 * TODO: we assume the master device is always there and doesn't get
 	 * removed during runtime.
 	 */
-	ret = device_probe(master);
-	if (ret)
-		return ret;
+	err = device_probe(master);
+	if (err)
+		return err;
 
 	/*
 	 * Inherit port's hwaddr from the DSA master, unless the port already
-- 
2.25.1

[PATCH 3/6] net: dsa: refactor the code to set the port MAC address into a dedicated function

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This snippet of code has a bothering "if (...) return 0" in it which
assumes it is the last piece of code running in dsa_port_probe().

This makes it difficult to add further code at the end of dsa_port_probe()
which does not depend on MAC address stuff.

So move the code to a dedicated function which returns void and let the
code flow through.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa-uclass.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
index 17b33834f585..e23da2ed0e95 100644
--- a/net/dsa-uclass.c
+++ b/net/dsa-uclass.c
@@ -240,11 +240,29 @@ static const struct eth_ops dsa_port_ops = {
 	.free_pkt	= dsa_port_free_pkt,
 };
 
-static int dsa_port_probe(struct udevice *pdev)
+/*
+ * Inherit port's hwaddr from the DSA master, unless the port already has a
+ * unique MAC address specified in the environment.
+ */
+static void dsa_port_set_hwaddr(struct udevice *pdev, struct udevice *master)
 {
-	struct udevice *dev = dev_get_parent(pdev);
 	struct eth_pdata *eth_pdata, *master_pdata;
 	unsigned char env_enetaddr[ARP_HLEN];
+
+	eth_env_get_enetaddr_by_index("eth", dev_seq(pdev), env_enetaddr);
+	if (!is_zero_ethaddr(env_enetaddr))
+		return;
+
+	master_pdata = dev_get_plat(master);
+	eth_pdata = dev_get_plat(pdev);
+	memcpy(eth_pdata->enetaddr, master_pdata->enetaddr, ARP_HLEN);
+	eth_env_set_enetaddr_by_index("eth", dev_seq(pdev),
+				      master_pdata->enetaddr);
+}
+
+static int dsa_port_probe(struct udevice *pdev)
+{
+	struct udevice *dev = dev_get_parent(pdev);
 	struct dsa_port_pdata *port_pdata;
 	struct dsa_priv *dsa_priv;
 	struct udevice *master;
@@ -272,19 +290,8 @@ static int dsa_port_probe(struct udevice *pdev)
 	if (err)
 		return err;
 
-	/*
-	 * Inherit port's hwaddr from the DSA master, unless the port already
-	 * has a unique MAC address specified in the environment.
-	 */
-	eth_env_get_enetaddr_by_index("eth", dev_seq(pdev), env_enetaddr);
-	if (!is_zero_ethaddr(env_enetaddr))
-		return 0;
+	dsa_port_set_hwaddr(pdev, master);
 
-	master_pdata = dev_get_plat(master);
-	eth_pdata = dev_get_plat(pdev);
-	memcpy(eth_pdata->enetaddr, master_pdata->enetaddr, ARP_HLEN);
-	eth_env_set_enetaddr_by_index("eth", dev_seq(pdev),
-				      master_pdata->enetaddr);
 
 	return 0;
 }
-- 
2.25.1

[PATCH 4/6] net: dsa: introduce a .port_probe() method in struct dsa_ops

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Some drivers might want to execute code for each port at probe time, as
opposed to executing code just-in-time for the port selected for
networking.

To cater to that use case, introduce a .port_probe() callback method
into the DSA switch operations which is called for each available port,
at the end of dsa_port_probe().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h | 5 ++++-
 net/dsa-uclass.c  | 7 +++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 0f31a908c9d1..ab2a9dfbea2d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -57,7 +57,8 @@
 /**
  * struct dsa_ops - DSA operations
  *
- * @port_enable:  Initialize a switch port for I/O.
+ * @port_probe:   Initialize a switch port.
+ * @port_enable:  Enable I/O for a port.
  * @port_disable: Disable I/O for a port.
  * @xmit:         Insert the DSA tag for transmission.
  *                DSA drivers receive a copy of the packet with headroom and
@@ -69,6 +70,8 @@
  *                master including any additional headers.
  */
 struct dsa_ops {
+	int (*port_probe)(struct udevice *dev, int port,
+			  struct phy_device *phy);
 	int (*port_enable)(struct udevice *dev, int port,
 			   struct phy_device *phy);
 	void (*port_disable)(struct udevice *dev, int port,
diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
index e23da2ed0e95..1db723573e6c 100644
--- a/net/dsa-uclass.c
+++ b/net/dsa-uclass.c
@@ -263,6 +263,7 @@ static void dsa_port_set_hwaddr(struct udevice *pdev, struct udevice *master)
 static int dsa_port_probe(struct udevice *pdev)
 {
 	struct udevice *dev = dev_get_parent(pdev);
+	struct dsa_ops *ops = dsa_get_ops(dev);
 	struct dsa_port_pdata *port_pdata;
 	struct dsa_priv *dsa_priv;
 	struct udevice *master;
@@ -292,6 +293,12 @@ static int dsa_port_probe(struct udevice *pdev)
 
 	dsa_port_set_hwaddr(pdev, master);
 
+	if (ops->port_probe) {
+		err = ops->port_probe(dev, port_pdata->index,
+				      port_pdata->phy);
+		if (err)
+			return err;
+	}
 
 	return 0;
 }
-- 
2.25.1

[PATCH 5/6] net: dsa: felix: call phy_config at .port_probe() time

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

It is an unfortunate reality that some PHY settings done by U-Boot
persist even after the PHY is reset and taken over by Linux, and even
more unfortunate that Linux has come to depend on things being set in a
certain way.

For example, on the NXP LS1028A-RDB, the felix switch ports are
connected to a VSC8514 QSGMII PHY. Between the switch port PCS and the
PHY, the U-Boot drivers enable in-band auto-negotiation which makes the
copper-side negotiated speed and duplex be transmitted from the PHY to
the MAC automatically.

The PHY driver portion that does this is in vsc8514_config():

	/* Enable Serdes Auto-negotiation */
	phy_write(phydev, MDIO_DEVAD_NONE, PHY_EXT_PAGE_ACCESS,
		  PHY_EXT_PAGE_ACCESS_EXTENDED3);
	val = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_VSC8514_MAC_SERDES_CON);
	val = val | MIIM_VSC8574_MAC_SERDES_ANEG;
	phy_write(phydev, MDIO_DEVAD_NONE, MIIM_VSC8514_MAC_SERDES_CON, val);

The point is that in-band autoneg should be turned on in both the PHY
and the MAC, or off in both the PHY and the MAC, otherwise the QSGMII
link will be broken.

And because phy_config() is currently called at .port_enable() time, the
result is that ports on which traffic has been sent in U-Boot will have
in-band autoneg enabled, and the rest won't.

It can be argued that the Linux kernel should not assume one way or
another and just reinitialize everything according to what it expects,
and that is completely fair. In fact, I've already started an attempt to
remove this dependency, although admittedly I am making slow progress at
it:
https://patchwork.kernel.org/project/netdevbpf/cover/20210212172341.3489046-1-olteanv@gmail.com/

Nonetheless, the sad reality is that NXP also has, apart from kernel
drivers, some user space networking (DPDK), and for some reason, the
expectation there is that somebody else initializes the PHYs. The kernel
can't do it because the device ownership doesn't belong to the kernel,
so what remains is for the bootloader to do it (especially since other
drivers generally call phy_config() at probe time). This is a really
weak guarantee that might break at any time, but apparently that is
enough for some.

Since initializing the ports and PHYs at probe time does not break
anything, we can just do that.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/mscc_eswitch/felix_switch.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/mscc_eswitch/felix_switch.c b/drivers/net/mscc_eswitch/felix_switch.c
index 75073880cf87..c8ecf4f19442 100644
--- a/drivers/net/mscc_eswitch/felix_switch.c
+++ b/drivers/net/mscc_eswitch/felix_switch.c
@@ -317,10 +317,23 @@ static int felix_probe(struct udevice *dev)
 	return 0;
 }
 
+static int felix_port_probe(struct udevice *dev, int port,
+			    struct phy_device *phy)
+{
+	int supported = PHY_GBIT_FEATURES | SUPPORTED_2500baseX_Full;
+	struct felix_priv *priv = dev_get_priv(dev);
+
+	phy->supported &= supported;
+	phy->advertising &= supported;
+
+	felix_start_pcs(dev, port, phy, &priv->imdio);
+
+	return phy_config(phy);
+}
+
 static int felix_port_enable(struct udevice *dev, int port,
 			     struct phy_device *phy)
 {
-	int supported = PHY_GBIT_FEATURES | SUPPORTED_2500baseX_Full;
 	struct felix_priv *priv = dev_get_priv(dev);
 	void *base = priv->regs_base;
 
@@ -339,12 +352,6 @@ static int felix_port_enable(struct udevice *dev, int port,
 		 FELIX_QSYS_SYSTEM_SW_PORT_LOSSY |
 		 FELIX_QSYS_SYSTEM_SW_PORT_SCH(1));
 
-	felix_start_pcs(dev, port, phy, &priv->imdio);
-
-	phy->supported &= supported;
-	phy->advertising &= supported;
-	phy_config(phy);
-
 	phy_startup(phy);
 
 	return 0;
@@ -392,6 +399,7 @@ static int felix_rcv(struct udevice *dev, int *pidx, void *packet, int length)
 }
 
 static const struct dsa_ops felix_dsa_ops = {
+	.port_probe	= felix_port_probe,
 	.port_enable	= felix_port_enable,
 	.port_disable	= felix_port_disable,
 	.xmit		= felix_xmit,
-- 
2.25.1

[PATCH 6/6] net: dsa: felix: propagate the error code from phy_startup()

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Make sure that the link status returned by phy_startup() is propagated
to the .start() method of struct eth_ops.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/mscc_eswitch/felix_switch.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/mscc_eswitch/felix_switch.c b/drivers/net/mscc_eswitch/felix_switch.c
index c8ecf4f19442..6aa79784460d 100644
--- a/drivers/net/mscc_eswitch/felix_switch.c
+++ b/drivers/net/mscc_eswitch/felix_switch.c
@@ -352,9 +352,7 @@ static int felix_port_enable(struct udevice *dev, int port,
 		 FELIX_QSYS_SYSTEM_SW_PORT_LOSSY |
 		 FELIX_QSYS_SYSTEM_SW_PORT_SCH(1));
 
-	phy_startup(phy);
-
-	return 0;
+	return phy_startup(phy);
 }
 
 static void felix_port_disable(struct udevice *dev, int pidx,
-- 
2.25.1

Re: [PATCH 1/6] net: dsa: felix: felix_init() can be static

[Ramon Fried]

On Tue, Jun 29, 2021 at 8:09 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> No one is calling this function from outside felix_switch.c.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/mscc_eswitch/felix_switch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/mscc_eswitch/felix_switch.c b/drivers/net/mscc_eswitch/felix_switch.c
> index f20e84e0f10c..75073880cf87 100644
> --- a/drivers/net/mscc_eswitch/felix_switch.c
> +++ b/drivers/net/mscc_eswitch/felix_switch.c
> @@ -233,7 +233,7 @@ static void felix_start_pcs(struct udevice *dev, int port,
>         }
>  }
>
> -void felix_init(struct udevice *dev)
> +static void felix_init(struct udevice *dev)
>  {
>         struct dsa_pdata *pdata = dev_get_uclass_plat(dev);
>         struct felix_priv *priv = dev_get_priv(dev);
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 2/6] net: dsa: use "err" instead of "ret" in dsa_port_probe

[Ramon Fried]

On Tue, Jun 29, 2021 at 8:09 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> DM DSA uses "err" for error code values, so use this consistently.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/dsa-uclass.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> index 7ea1cb6949c0..17b33834f585 100644
> --- a/net/dsa-uclass.c
> +++ b/net/dsa-uclass.c
> @@ -248,7 +248,7 @@ static int dsa_port_probe(struct udevice *pdev)
>         struct dsa_port_pdata *port_pdata;
>         struct dsa_priv *dsa_priv;
>         struct udevice *master;
> -       int ret;
> +       int err;
>
>         port_pdata = dev_get_parent_plat(pdev);
>         dsa_priv = dev_get_uclass_priv(dev);
> @@ -268,9 +268,9 @@ static int dsa_port_probe(struct udevice *pdev)
>          * TODO: we assume the master device is always there and doesn't get
>          * removed during runtime.
>          */
> -       ret = device_probe(master);
> -       if (ret)
> -               return ret;
> +       err = device_probe(master);
> +       if (err)
> +               return err;
>
>         /*
>          * Inherit port's hwaddr from the DSA master, unless the port already
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 3/6] net: dsa: refactor the code to set the port MAC address into a dedicated function

[Ramon Fried]

On Tue, Jun 29, 2021 at 8:09 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> This snippet of code has a bothering "if (...) return 0" in it which
> assumes it is the last piece of code running in dsa_port_probe().
>
> This makes it difficult to add further code at the end of dsa_port_probe()
> which does not depend on MAC address stuff.
>
> So move the code to a dedicated function which returns void and let the
> code flow through.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/dsa-uclass.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> index 17b33834f585..e23da2ed0e95 100644
> --- a/net/dsa-uclass.c
> +++ b/net/dsa-uclass.c
> @@ -240,11 +240,29 @@ static const struct eth_ops dsa_port_ops = {
>         .free_pkt       = dsa_port_free_pkt,
>  };
>
> -static int dsa_port_probe(struct udevice *pdev)
> +/*
> + * Inherit port's hwaddr from the DSA master, unless the port already has a
> + * unique MAC address specified in the environment.
> + */
> +static void dsa_port_set_hwaddr(struct udevice *pdev, struct udevice *master)
>  {
> -       struct udevice *dev = dev_get_parent(pdev);
>         struct eth_pdata *eth_pdata, *master_pdata;
>         unsigned char env_enetaddr[ARP_HLEN];
> +
> +       eth_env_get_enetaddr_by_index("eth", dev_seq(pdev), env_enetaddr);
> +       if (!is_zero_ethaddr(env_enetaddr))
> +               return;
> +
> +       master_pdata = dev_get_plat(master);
> +       eth_pdata = dev_get_plat(pdev);
> +       memcpy(eth_pdata->enetaddr, master_pdata->enetaddr, ARP_HLEN);
> +       eth_env_set_enetaddr_by_index("eth", dev_seq(pdev),
> +                                     master_pdata->enetaddr);
> +}
> +
> +static int dsa_port_probe(struct udevice *pdev)
> +{
> +       struct udevice *dev = dev_get_parent(pdev);
>         struct dsa_port_pdata *port_pdata;
>         struct dsa_priv *dsa_priv;
>         struct udevice *master;
> @@ -272,19 +290,8 @@ static int dsa_port_probe(struct udevice *pdev)
>         if (err)
>                 return err;
>
> -       /*
> -        * Inherit port's hwaddr from the DSA master, unless the port already
> -        * has a unique MAC address specified in the environment.
> -        */
> -       eth_env_get_enetaddr_by_index("eth", dev_seq(pdev), env_enetaddr);
> -       if (!is_zero_ethaddr(env_enetaddr))
> -               return 0;
> +       dsa_port_set_hwaddr(pdev, master);
>
> -       master_pdata = dev_get_plat(master);
> -       eth_pdata = dev_get_plat(pdev);
> -       memcpy(eth_pdata->enetaddr, master_pdata->enetaddr, ARP_HLEN);
> -       eth_env_set_enetaddr_by_index("eth", dev_seq(pdev),
> -                                     master_pdata->enetaddr);
>
>         return 0;
>  }
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 4/6] net: dsa: introduce a .port_probe() method in struct dsa_ops

[Ramon Fried]

On Tue, Jun 29, 2021 at 8:09 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> Some drivers might want to execute code for each port at probe time, as
> opposed to executing code just-in-time for the port selected for
> networking.
>
> To cater to that use case, introduce a .port_probe() callback method
> into the DSA switch operations which is called for each available port,
> at the end of dsa_port_probe().
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  include/net/dsa.h | 5 ++++-
>  net/dsa-uclass.c  | 7 +++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 0f31a908c9d1..ab2a9dfbea2d 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -57,7 +57,8 @@
>  /**
>   * struct dsa_ops - DSA operations
>   *
> - * @port_enable:  Initialize a switch port for I/O.
> + * @port_probe:   Initialize a switch port.
> + * @port_enable:  Enable I/O for a port.
>   * @port_disable: Disable I/O for a port.
>   * @xmit:         Insert the DSA tag for transmission.
>   *                DSA drivers receive a copy of the packet with headroom and
> @@ -69,6 +70,8 @@
>   *                master including any additional headers.
>   */
>  struct dsa_ops {
> +       int (*port_probe)(struct udevice *dev, int port,
> +                         struct phy_device *phy);
>         int (*port_enable)(struct udevice *dev, int port,
>                            struct phy_device *phy);
>         void (*port_disable)(struct udevice *dev, int port,
> diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
> index e23da2ed0e95..1db723573e6c 100644
> --- a/net/dsa-uclass.c
> +++ b/net/dsa-uclass.c
> @@ -263,6 +263,7 @@ static void dsa_port_set_hwaddr(struct udevice *pdev, struct udevice *master)
>  static int dsa_port_probe(struct udevice *pdev)
>  {
>         struct udevice *dev = dev_get_parent(pdev);
> +       struct dsa_ops *ops = dsa_get_ops(dev);
>         struct dsa_port_pdata *port_pdata;
>         struct dsa_priv *dsa_priv;
>         struct udevice *master;
> @@ -292,6 +293,12 @@ static int dsa_port_probe(struct udevice *pdev)
>
>         dsa_port_set_hwaddr(pdev, master);
>
> +       if (ops->port_probe) {
> +               err = ops->port_probe(dev, port_pdata->index,
> +                                     port_pdata->phy);
> +               if (err)
> +                       return err;
> +       }
>
>         return 0;
>  }
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 5/6] net: dsa: felix: call phy_config at .port_probe() time

[Ramon Fried]

On Tue, Jun 29, 2021 at 8:09 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> It is an unfortunate reality that some PHY settings done by U-Boot
> persist even after the PHY is reset and taken over by Linux, and even
> more unfortunate that Linux has come to depend on things being set in a
> certain way.
>
> For example, on the NXP LS1028A-RDB, the felix switch ports are
> connected to a VSC8514 QSGMII PHY. Between the switch port PCS and the
> PHY, the U-Boot drivers enable in-band auto-negotiation which makes the
> copper-side negotiated speed and duplex be transmitted from the PHY to
> the MAC automatically.
>
> The PHY driver portion that does this is in vsc8514_config():
>
>         /* Enable Serdes Auto-negotiation */
>         phy_write(phydev, MDIO_DEVAD_NONE, PHY_EXT_PAGE_ACCESS,
>                   PHY_EXT_PAGE_ACCESS_EXTENDED3);
>         val = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_VSC8514_MAC_SERDES_CON);
>         val = val | MIIM_VSC8574_MAC_SERDES_ANEG;
>         phy_write(phydev, MDIO_DEVAD_NONE, MIIM_VSC8514_MAC_SERDES_CON, val);
>
> The point is that in-band autoneg should be turned on in both the PHY
> and the MAC, or off in both the PHY and the MAC, otherwise the QSGMII
> link will be broken.
>
> And because phy_config() is currently called at .port_enable() time, the
> result is that ports on which traffic has been sent in U-Boot will have
> in-band autoneg enabled, and the rest won't.
>
> It can be argued that the Linux kernel should not assume one way or
> another and just reinitialize everything according to what it expects,
> and that is completely fair. In fact, I've already started an attempt to
> remove this dependency, although admittedly I am making slow progress at
> it:
> https://patchwork.kernel.org/project/netdevbpf/cover/20210212172341.3489046-1-olteanv@gmail.com/
>
> Nonetheless, the sad reality is that NXP also has, apart from kernel
> drivers, some user space networking (DPDK), and for some reason, the
> expectation there is that somebody else initializes the PHYs. The kernel
> can't do it because the device ownership doesn't belong to the kernel,
> so what remains is for the bootloader to do it (especially since other
> drivers generally call phy_config() at probe time). This is a really
> weak guarantee that might break at any time, but apparently that is
> enough for some.
>
> Since initializing the ports and PHYs at probe time does not break
> anything, we can just do that.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/mscc_eswitch/felix_switch.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/mscc_eswitch/felix_switch.c b/drivers/net/mscc_eswitch/felix_switch.c
> index 75073880cf87..c8ecf4f19442 100644
> --- a/drivers/net/mscc_eswitch/felix_switch.c
> +++ b/drivers/net/mscc_eswitch/felix_switch.c
> @@ -317,10 +317,23 @@ static int felix_probe(struct udevice *dev)
>         return 0;
>  }
>
> +static int felix_port_probe(struct udevice *dev, int port,
> +                           struct phy_device *phy)
> +{
> +       int supported = PHY_GBIT_FEATURES | SUPPORTED_2500baseX_Full;
> +       struct felix_priv *priv = dev_get_priv(dev);
> +
> +       phy->supported &= supported;
> +       phy->advertising &= supported;
> +
> +       felix_start_pcs(dev, port, phy, &priv->imdio);
> +
> +       return phy_config(phy);
> +}
> +
>  static int felix_port_enable(struct udevice *dev, int port,
>                              struct phy_device *phy)
>  {
> -       int supported = PHY_GBIT_FEATURES | SUPPORTED_2500baseX_Full;
>         struct felix_priv *priv = dev_get_priv(dev);
>         void *base = priv->regs_base;
>
> @@ -339,12 +352,6 @@ static int felix_port_enable(struct udevice *dev, int port,
>                  FELIX_QSYS_SYSTEM_SW_PORT_LOSSY |
>                  FELIX_QSYS_SYSTEM_SW_PORT_SCH(1));
>
> -       felix_start_pcs(dev, port, phy, &priv->imdio);
> -
> -       phy->supported &= supported;
> -       phy->advertising &= supported;
> -       phy_config(phy);
> -
>         phy_startup(phy);
>
>         return 0;
> @@ -392,6 +399,7 @@ static int felix_rcv(struct udevice *dev, int *pidx, void *packet, int length)
>  }
>
>  static const struct dsa_ops felix_dsa_ops = {
> +       .port_probe     = felix_port_probe,
>         .port_enable    = felix_port_enable,
>         .port_disable   = felix_port_disable,
>         .xmit           = felix_xmit,
> --
> 2.25.1
>
Interesting, My two cents is that device drivers by definition
shouldn't assume anything regarding the state of the HW.
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 6/6] net: dsa: felix: propagate the error code from phy_startup()

[Ramon Fried]

On Tue, Jun 29, 2021 at 8:09 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> Make sure that the link status returned by phy_startup() is propagated
> to the .start() method of struct eth_ops.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/mscc_eswitch/felix_switch.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/mscc_eswitch/felix_switch.c b/drivers/net/mscc_eswitch/felix_switch.c
> index c8ecf4f19442..6aa79784460d 100644
> --- a/drivers/net/mscc_eswitch/felix_switch.c
> +++ b/drivers/net/mscc_eswitch/felix_switch.c
> @@ -352,9 +352,7 @@ static int felix_port_enable(struct udevice *dev, int port,
>                  FELIX_QSYS_SYSTEM_SW_PORT_LOSSY |
>                  FELIX_QSYS_SYSTEM_SW_PORT_SCH(1));
>
> -       phy_startup(phy);
> -
> -       return 0;
> +       return phy_startup(phy);
>  }
>
>  static void felix_port_disable(struct udevice *dev, int pidx,
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 0/6] Call phy_config at port probe time for the Felix DSA driver

[Michael Walle]

Am 2021-06-29 19:08, schrieb Vladimir Oltean:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> This series makes the Felix DSA driver initialize all its connected 
> PHYs
> regardless of whether those will be used for networking or not. This is
> in order to satisfy the expectations of some software in later boot
> stages.
> 
> To make this work, it is necessary to introduce a new method in struct
> dsa_ops: .port_probe().
> 
> There is some further refactoring/cleanup along the way.

Tested-by: Michael Walle <michael@walle.cc>

Seems like our PHYs are already in in-band signalling mode. I can't see
any difference after applying this series. Neither by using the port
in uboot, ie. "bootp; boot" nor booting directly into linux without
using the network.

Also, we have two different variants:
  - one where the switch is used in u-boot and in linux:
    arch/arm/dts/fsl-ls1028a-kontron-sl28-var2.dtb (DSA support still
    pending in u-boot)
  - one where we only use the switch in linux and u-boot doesn't touch
    the device at all:
    arch/arm/dts/fsl-ls1028a-kontron-sl28-var4.dtb

Do I understand it correctly, that for the latter variant DPDK would
be broken if the PHY hasn't in-band signalling enabled by default?
Because u-boot doesn't know the device and it won't setup the PHY
correctly.

Oh and I noticed that for var2 "managed = in-band-status" is missing
in the linux dtb; what a mess ;) I wonder how this ever worked..

-michael

RE: [PATCH 3/6] net: dsa: refactor the code to set the port MAC address into a dedicated function

[Priyanka Jain]

>-----Original Message-----
>From: U-Boot <u-boot-bounces@lists.denx.de> On Behalf Of Vladimir Oltean
>Sent: Tuesday, June 29, 2021 10:39 PM
>To: Joe Hershberger <joe.hershberger@ni.com>; Ramon Fried
><rfried.dev@gmail.com>; u-boot@lists.denx.de
>Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Bin Meng
><bmeng.cn@gmail.com>; Alexandru Marginean
><alexandru.marginean@nxp.com>; Michael Walle <michael@walle.cc>;
>tharvey@gateworks.com; Vladimir Oltean <vladimir.oltean@nxp.com>
>Subject: [PATCH 3/6] net: dsa: refactor the code to set the port MAC address into
>a dedicated function
>
>From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
>This snippet of code has a bothering "if (...) return 0" in it which assumes it is the
>last piece of code running in dsa_port_probe().
>
>This makes it difficult to add further code at the end of dsa_port_probe() which
>does not depend on MAC address stuff.
>
>So move the code to a dedicated function which returns void and let the code
>flow through.
>
>Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>---
> net/dsa-uclass.c | 35 +++++++++++++++++++++--------------
> 1 file changed, 21 insertions(+), 14 deletions(-)
>
>diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index
>17b33834f585..e23da2ed0e95 100644
>--- a/net/dsa-uclass.c
>+++ b/net/dsa-uclass.c
>@@ -240,11 +240,29 @@ static const struct eth_ops dsa_port_ops = {
> 	.free_pkt	= dsa_port_free_pkt,
> };
>
>-static int dsa_port_probe(struct udevice *pdev)
>+/*
>+ * Inherit port's hwaddr from the DSA master, unless the port already
>+has a
>+ * unique MAC address specified in the environment.
>+ */
>+static void dsa_port_set_hwaddr(struct udevice *pdev, struct udevice
>+*master)
> {
>-	struct udevice *dev = dev_get_parent(pdev);
> 	struct eth_pdata *eth_pdata, *master_pdata;
> 	unsigned char env_enetaddr[ARP_HLEN];
>+
>+	eth_env_get_enetaddr_by_index("eth", dev_seq(pdev), env_enetaddr);
>+	if (!is_zero_ethaddr(env_enetaddr))
>+		return;
>+
>+	master_pdata = dev_get_plat(master);
>+	eth_pdata = dev_get_plat(pdev);
>+	memcpy(eth_pdata->enetaddr, master_pdata->enetaddr, ARP_HLEN);
>+	eth_env_set_enetaddr_by_index("eth", dev_seq(pdev),
>+				      master_pdata->enetaddr);
>+}
>+
>+static int dsa_port_probe(struct udevice *pdev) {
>+	struct udevice *dev = dev_get_parent(pdev);
> 	struct dsa_port_pdata *port_pdata;
> 	struct dsa_priv *dsa_priv;
> 	struct udevice *master;
>@@ -272,19 +290,8 @@ static int dsa_port_probe(struct udevice *pdev)
> 	if (err)
> 		return err;
>
>-	/*
>-	 * Inherit port's hwaddr from the DSA master, unless the port already
>-	 * has a unique MAC address specified in the environment.
>-	 */
>-	eth_env_get_enetaddr_by_index("eth", dev_seq(pdev), env_enetaddr);
>-	if (!is_zero_ethaddr(env_enetaddr))
>-		return 0;
>+	dsa_port_set_hwaddr(pdev, master);
>
>-	master_pdata = dev_get_plat(master);
>-	eth_pdata = dev_get_plat(pdev);
>-	memcpy(eth_pdata->enetaddr, master_pdata->enetaddr, ARP_HLEN);
>-	eth_env_set_enetaddr_by_index("eth", dev_seq(pdev),
>-				      master_pdata->enetaddr);
>
> 	return 0;
> }
>--
>2.25.1

Kindly rebase to top of master branch.

Regards
Priyanka