[PATCH net-next v3 0/4] net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY

[PATCH net-next v3 0/4] net: ethernet: renesas: rswitch: Modify initialization for SERDES and PHY

[Yoshihiro Shimoda]

- My platform has the 88x2110.
- The MACTYPE setting of strap pin on the platform is SXGMII.
- However, we realized that the SoC cannot communicate the PHY with SXGMII
  because of mismatching hardware specification.
- We have a lot of boards which mismatch the MACTYPE setting.

So, I would like to change the MACTYPE as SGMII by software for the platform.

The patch [1/4] sets phydev->host_interfaces by phylink for Marvell PHY
driver (marvell10g) to initialize the MACTYPE.

The patch [2/4] siplifies the rswitch driver, the patch [3/4] enables
the ovr_host_interfaces flag, and the patch [4/4] phy_power_on() calling
to initialize the Ethernet SERDES PHY driver (r8a779f0-eth-serdes)
for each channel.

Changes from v2:
https://lore.kernel.org/all/20230110050206.116110-1-yoshihiro.shimoda.uh@renesas.com/
 - Add some blank lines for readability.

Changes from v1:
https://lore.kernel.org/all/20221226071425.3895915-1-yoshihiro.shimoda.uh@renesas.com/
 - Add a new flag (ovr_host_interfaces) into phylink_config in the patch [1/4].
 - Add a new patch [3/4] for the new flag.
 - Add a error message to the patch [4/4/] for MLO_AN_INBAND mode.

Yoshihiro Shimoda (4):
  net: phylink: Set host_interfaces for a non-sfp PHY
  net: ethernet: renesas: rswitch: Simplify struct phy * handling
  net: ethernet: renesas: rswitch: Enable ovr_host_interfaces
  net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling

 drivers/net/ethernet/renesas/rswitch.c | 53 +++++++++++++++-----------
 drivers/net/ethernet/renesas/rswitch.h |  1 +
 drivers/net/phy/phylink.c              | 11 ++++++
 include/linux/phylink.h                |  3 ++
 4 files changed, 46 insertions(+), 22 deletions(-)

-- 
2.25.1

[PATCH net-next v3 1/4] net: phylink: Set host_interfaces for a non-sfp PHY

[Yoshihiro Shimoda]

If a new flag (ovr_host_interfaces) in the phylink_config is set,
overwrite the host_interfaces in the phy_device by link_interface.

Note that an ethernet PHY driver like marvell10g will check
PHY_INTERFACE_MODE_SGMII in the host_interfaces whther the host
controller supports a rate matching interface mode or not. So, set
PHY_INTERFACE_MODE_SGMII to the host_interfaces if it is set in
the supported_interfaces.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/phy/phylink.c | 11 +++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 319790221d7f..dee64b4a1175 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1814,6 +1814,17 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	if (pl->config->ovr_host_interfaces) {
+		__set_bit(pl->link_interface, phy_dev->host_interfaces);
+
+		/* An ethernet PHY driver will check PHY_INTERFACE_MODE_SGMII
+		 * in the host_interfaces whether the host controller supports
+		 * a rate matching interface mode or not.
+		 */
+		if (test_bit(PHY_INTERFACE_MODE_SGMII, pl->config->supported_interfaces))
+			__set_bit(PHY_INTERFACE_MODE_SGMII, phy_dev->host_interfaces);
+	}
+
 	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
 				pl->link_interface);
 	if (ret) {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..c8dd53b1e857 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -124,6 +124,8 @@ enum phylink_op_type {
  *		      if MAC link is at %MLO_AN_FIXED mode.
  * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM.
  * @ovr_an_inband: if true, override PCS to MLO_AN_INBAND
+ * @ovr_host_interfaces: if true, override host_interfaces of phy_device from
+ *			 link_interface.
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
  * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
@@ -137,6 +139,7 @@ struct phylink_config {
 	bool poll_fixed_state;
 	bool mac_managed_pm;
 	bool ovr_an_inband;
+	bool ovr_host_interfaces;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
-- 
2.25.1

[PATCH net-next v3 2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling

[Yoshihiro Shimoda]

Simplify struct phy *serdes handling by keeping the valiable in
the struct rswitch_device.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 40 ++++++++++++--------------
 drivers/net/ethernet/renesas/rswitch.h |  1 +
 2 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index 14fc0af304ce..b0c1ea72772e 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1222,49 +1222,40 @@ static void rswitch_phylink_deinit(struct rswitch_device *rdev)
 	phylink_destroy(rdev->phylink);
 }
 
-static int rswitch_serdes_set_params(struct rswitch_device *rdev)
+static int rswitch_serdes_phy_get(struct rswitch_device *rdev)
 {
 	struct device_node *port = rswitch_get_port_node(rdev);
 	struct phy *serdes;
-	int err;
 
 	serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
 	of_node_put(port);
 	if (IS_ERR(serdes))
 		return PTR_ERR(serdes);
+	rdev->serdes = serdes;
+
+	return 0;
+}
+
+static int rswitch_serdes_set_params(struct rswitch_device *rdev)
+{
+	int err;
 
-	err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET,
+	err = phy_set_mode_ext(rdev->serdes, PHY_MODE_ETHERNET,
 			       rdev->etha->phy_interface);
 	if (err < 0)
 		return err;
 
-	return phy_set_speed(serdes, rdev->etha->speed);
+	return phy_set_speed(rdev->serdes, rdev->etha->speed);
 }
 
 static int rswitch_serdes_init(struct rswitch_device *rdev)
 {
-	struct device_node *port = rswitch_get_port_node(rdev);
-	struct phy *serdes;
-
-	serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
-	of_node_put(port);
-	if (IS_ERR(serdes))
-		return PTR_ERR(serdes);
-
-	return phy_init(serdes);
+	return phy_init(rdev->serdes);
 }
 
 static int rswitch_serdes_deinit(struct rswitch_device *rdev)
 {
-	struct device_node *port = rswitch_get_port_node(rdev);
-	struct phy *serdes;
-
-	serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
-	of_node_put(port);
-	if (IS_ERR(serdes))
-		return PTR_ERR(serdes);
-
-	return phy_exit(serdes);
+	return phy_exit(rdev->serdes);
 }
 
 static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
@@ -1286,6 +1277,10 @@ static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
 	if (err < 0)
 		goto err_phylink_init;
 
+	err = rswitch_serdes_phy_get(rdev);
+	if (err < 0)
+		goto err_serdes_phy_get;
+
 	err = rswitch_serdes_set_params(rdev);
 	if (err < 0)
 		goto err_serdes_set_params;
@@ -1293,6 +1288,7 @@ static int rswitch_ether_port_init_one(struct rswitch_device *rdev)
 	return 0;
 
 err_serdes_set_params:
+err_serdes_phy_get:
 	rswitch_phylink_deinit(rdev);
 
 err_phylink_init:
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index 49efb0f31c77..c79b1bdd8072 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -953,6 +953,7 @@ struct rswitch_device {
 
 	int port;
 	struct rswitch_etha *etha;
+	struct phy *serdes;
 };
 
 struct rswitch_mfwd_mac_table_entry {
-- 
2.25.1

Re: [PATCH net-next v3 2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling

[Geert Uytterhoeven]

Hi Shimoda-san,

On Fri, Jan 27, 2023 at 2:49 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Simplify struct phy *serdes handling by keeping the valiable in
> the struct rswitch_device.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Thanks for your patch!

> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -1222,49 +1222,40 @@ static void rswitch_phylink_deinit(struct rswitch_device *rdev)
>         phylink_destroy(rdev->phylink);
>  }
>
> -static int rswitch_serdes_set_params(struct rswitch_device *rdev)
> +static int rswitch_serdes_phy_get(struct rswitch_device *rdev)
>  {
>         struct device_node *port = rswitch_get_port_node(rdev);
>         struct phy *serdes;
> -       int err;
>
>         serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
>         of_node_put(port);
>         if (IS_ERR(serdes))
>                 return PTR_ERR(serdes);

You may as well just return serdes...

> +       rdev->serdes = serdes;

... and move the above assignment into the caller.
That would save one if (...) check.

After that, not much is left in this function, so I'm wondering if it
can just be inlined at the single callsite?

BTW, there seem to be several calls to rswitch_get_port_node(), which
calls into DT tree traversal, so you may want to call it once, and store
a pointer to the port device node, too.  Then rswitch_serdes_phy_get()
becomes a candidate for manual inlining for sure.

> +
> +       return 0;
> +}
> +
> +static int rswitch_serdes_set_params(struct rswitch_device *rdev)
> +{
> +       int err;
>
> -       err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET,
> +       err = phy_set_mode_ext(rdev->serdes, PHY_MODE_ETHERNET,
>                                rdev->etha->phy_interface);
>         if (err < 0)
>                 return err;
>
> -       return phy_set_speed(serdes, rdev->etha->speed);
> +       return phy_set_speed(rdev->serdes, rdev->etha->speed);
>  }
>
>  static int rswitch_serdes_init(struct rswitch_device *rdev)
>  {
> -       struct device_node *port = rswitch_get_port_node(rdev);
> -       struct phy *serdes;
> -
> -       serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
> -       of_node_put(port);
> -       if (IS_ERR(serdes))
> -               return PTR_ERR(serdes);
> -
> -       return phy_init(serdes);
> +       return phy_init(rdev->serdes);
>  }

As this is now a one-line function, just call phy_init() in all
callers instead?

>
>  static int rswitch_serdes_deinit(struct rswitch_device *rdev)
>  {
> -       struct device_node *port = rswitch_get_port_node(rdev);
> -       struct phy *serdes;
> -
> -       serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
> -       of_node_put(port);
> -       if (IS_ERR(serdes))
> -               return PTR_ERR(serdes);
> -
> -       return phy_exit(serdes);
> +       return phy_exit(rdev->serdes);
>  }

Just call phy_exit() in all callers instead?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH net-next v3 2/4] net: ethernet: renesas: rswitch: Simplify struct phy * handling

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Friday, January 27, 2023 5:35 PM
> 
> Hi Shimoda-san,
> 
> On Fri, Jan 27, 2023 at 2:49 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Simplify struct phy *serdes handling by keeping the valiable in
> > the struct rswitch_device.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Thanks for your patch!

Thank you for your review!

> > --- a/drivers/net/ethernet/renesas/rswitch.c
> > +++ b/drivers/net/ethernet/renesas/rswitch.c
> > @@ -1222,49 +1222,40 @@ static void rswitch_phylink_deinit(struct rswitch_device *rdev)
> >         phylink_destroy(rdev->phylink);
> >  }
> >
> > -static int rswitch_serdes_set_params(struct rswitch_device *rdev)
> > +static int rswitch_serdes_phy_get(struct rswitch_device *rdev)
> >  {
> >         struct device_node *port = rswitch_get_port_node(rdev);
> >         struct phy *serdes;
> > -       int err;
> >
> >         serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
> >         of_node_put(port);
> >         if (IS_ERR(serdes))
> >                 return PTR_ERR(serdes);
> 
> You may as well just return serdes...
> 
> > +       rdev->serdes = serdes;
> 
> ... and move the above assignment into the caller.
> That would save one if (...) check.
> 
> After that, not much is left in this function, so I'm wondering if it
> can just be inlined at the single callsite?

I think so. Thank you for your suggestion!

> BTW, there seem to be several calls to rswitch_get_port_node(), which
> calls into DT tree traversal, so you may want to call it once, and store
> a pointer to the port device node, too.  Then rswitch_serdes_phy_get()
> becomes a candidate for manual inlining for sure.

I understood it. I'll modify it on v4 patch.

> > +
> > +       return 0;
> > +}
> > +
> > +static int rswitch_serdes_set_params(struct rswitch_device *rdev)
> > +{
> > +       int err;
> >
> > -       err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET,
> > +       err = phy_set_mode_ext(rdev->serdes, PHY_MODE_ETHERNET,
> >                                rdev->etha->phy_interface);
> >         if (err < 0)
> >                 return err;
> >
> > -       return phy_set_speed(serdes, rdev->etha->speed);
> > +       return phy_set_speed(rdev->serdes, rdev->etha->speed);
> >  }
> >
> >  static int rswitch_serdes_init(struct rswitch_device *rdev)
> >  {
> > -       struct device_node *port = rswitch_get_port_node(rdev);
> > -       struct phy *serdes;
> > -
> > -       serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
> > -       of_node_put(port);
> > -       if (IS_ERR(serdes))
> > -               return PTR_ERR(serdes);
> > -
> > -       return phy_init(serdes);
> > +       return phy_init(rdev->serdes);
> >  }
> 
> As this is now a one-line function, just call phy_init() in all
> callers instead?

I think so.

> >
> >  static int rswitch_serdes_deinit(struct rswitch_device *rdev)
> >  {
> > -       struct device_node *port = rswitch_get_port_node(rdev);
> > -       struct phy *serdes;
> > -
> > -       serdes = devm_of_phy_get(&rdev->priv->pdev->dev, port, NULL);
> > -       of_node_put(port);
> > -       if (IS_ERR(serdes))
> > -               return PTR_ERR(serdes);
> > -
> > -       return phy_exit(serdes);
> > +       return phy_exit(rdev->serdes);
> >  }
> 
> Just call phy_exit() in all callers instead?

I got it. I'll fix it.

Best regards,
Yoshihiro Shimoda

[PATCH net-next v3 3/4] net: ethernet: renesas: rswitch: Enable ovr_host_interfaces

[Yoshihiro Shimoda]

Enable ovr_host_interfaces to set the host_interfaces of phy_dev
for a non-sfp PHY.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index b0c1ea72772e..df8bc150f596 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1195,6 +1195,7 @@ static int rswitch_phylink_init(struct rswitch_device *rdev)
 
 	rdev->phylink_config.dev = &rdev->ndev->dev;
 	rdev->phylink_config.type = PHYLINK_NETDEV;
+	rdev->phylink_config.ovr_host_interfaces = true;
 	__set_bit(PHY_INTERFACE_MODE_SGMII, rdev->phylink_config.supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_USXGMII, rdev->phylink_config.supported_interfaces);
 	rdev->phylink_config.mac_capabilities = MAC_100FD | MAC_1000FD | MAC_2500FD;
-- 
2.25.1

[PATCH net-next v3 4/4] net: ethernet: renesas: rswitch: Add phy_power_{on,off}() calling

[Yoshihiro Shimoda]

Some Ethernet PHYs (like marvell10g) will decide the host interface
mode by the media-side speed. So, the rswitch driver needs to
initialize one of the Ethernet SERDES (r8a779f0-eth-serdes) ports
after linked the Ethernet PHY up. The r8a779f0-eth-serdes driver has
.init() for initializing all ports and .power_on() for initializing
each port. So, add phy_power_{on,off} calling for it.

Notes that in-band mode will not work because the initialization
is not completed. So, output error message if in-band mode.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index df8bc150f596..b4906e9eb50d 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1161,12 +1161,20 @@ static void rswitch_mac_config(struct phylink_config *config,
 			       unsigned int mode,
 			       const struct phylink_link_state *state)
 {
+	struct net_device *ndev = to_net_dev(config->dev);
+
+	if (mode == MLO_AN_INBAND)
+		netdev_err(ndev, "Link up/down will not work because in-band mode\n");
 }
 
 static void rswitch_mac_link_down(struct phylink_config *config,
 				  unsigned int mode,
 				  phy_interface_t interface)
 {
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct rswitch_device *rdev = netdev_priv(ndev);
+
+	phy_power_off(rdev->serdes);
 }
 
 static void rswitch_mac_link_up(struct phylink_config *config,
@@ -1174,7 +1182,11 @@ static void rswitch_mac_link_up(struct phylink_config *config,
 				phy_interface_t interface, int speed,
 				int duplex, bool tx_pause, bool rx_pause)
 {
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct rswitch_device *rdev = netdev_priv(ndev);
+
 	/* Current hardware cannot change speed at runtime */
+	phy_power_on(rdev->serdes);
 }
 
 static const struct phylink_mac_ops rswitch_phylink_ops = {
-- 
2.25.1