[PATCH RFC 0/3] net: phy: marvell10g: Add host speed setting by an ethernet driver

[PATCH RFC 0/3] net: phy: marvell10g: Add host speed setting by an ethernet driver

[Yoshihiro Shimoda]

R-Car S4-8 environment board requires to change the host interface
as SGMII and the host speed as 1000 Mbps because the strap pin was
other mode, but the SoC/board cannot work on that mode. Also, after
the SoC initialized the SERDES once, we cannot re-initialized it
because the initialized procedure seems all black magic...

To communicate between R-Car S4-8 (host MAC) and marvell PHY,
this patch series adds a new function of_phy_connect_with_host_param()
to set host parameters (host_interfaces and host_speed), and
rswitch driver sets the paramter. After that, the marvell10g can
check the paramters and set specific registers for it.

This patch series is based on next-20221017 and the following patches
which are not upstreamed yet:
https://lore.kernel.org/all/20221019083518.933070-1-yoshihiro.shimoda.uh@renesas.com/T/#m1c3de735de018b5cb90cb3720056ac1497112b36

I'm not sure whether this is a correct way or not. So, I marked
this patch series as RFC.

Yoshihiro Shimoda (3):
  net: mdio: Add of_phy_connect_with_host_param()
  net: phy: marvell10g: Add host interface speed configuration
  net: renesas: rswitch: Pass host parameters to phydev

 drivers/net/ethernet/renesas/rswitch.c | 13 ++++++--
 drivers/net/mdio/of_mdio.c             | 42 ++++++++++++++++++++++++++
 drivers/net/phy/marvell10g.c           | 23 ++++++++++++++
 include/linux/of_mdio.h                |  7 +++++
 include/linux/phy.h                    |  8 +++++
 5 files changed, 91 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH RFC 1/3] net: mdio: Add of_phy_connect_with_host_param()

[Yoshihiro Shimoda]

To set host parameters from an ethernet driver, add a new function
of_phy_connect_with_host_param().

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/net/mdio/of_mdio.c | 42 ++++++++++++++++++++++++++++++++++++++
 include/linux/of_mdio.h    |  7 +++++++
 include/linux/phy.h        |  8 ++++++++
 3 files changed, 57 insertions(+)

diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
index 796e9c7857d0..a2b0c3f84eea 100644
--- a/drivers/net/mdio/of_mdio.c
+++ b/drivers/net/mdio/of_mdio.c
@@ -298,6 +298,48 @@ struct phy_device *of_phy_connect(struct net_device *dev,
 }
 EXPORT_SYMBOL(of_phy_connect);
 
+/**
+ * of_phy_connect_with_host_params
+ * - Connect to the phy described in the device tree with host parameters
+ * @dev: pointer to net_device claiming the phy
+ * @phy_np: Pointer to device tree node for the PHY
+ * @hndlr: Link state callback for the network device
+ * @flags: flags to pass to the PHY
+ * @iface: PHY data interface type
+ * @host_interfaces: PHY data interface type by host
+ * @host_speed: PHY data interface speed by host
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure. The
+ * refcount must be dropped by calling phy_disconnect() or phy_detach().
+ */
+struct phy_device *
+of_phy_connect_with_host_params(struct net_device *dev,
+				struct device_node *phy_np,
+				void (*hndlr)(struct net_device *), u32 flags,
+				phy_interface_t iface,
+				unsigned long *host_interfaces,
+				int host_speed)
+{
+	struct phy_device *phy = of_phy_find_device(phy_np);
+	int ret;
+
+	if (!phy)
+		return NULL;
+
+	phy->dev_flags |= flags;
+	phy_interface_copy(phy->host_interfaces, host_interfaces);
+	phy->host_speed = host_speed;
+
+	ret = phy_connect_direct(dev, phy, hndlr, iface);
+
+	/* refcount is held by phy_connect_direct() on success */
+	put_device(&phy->mdio.dev);
+
+	return ret ? NULL : phy;
+}
+EXPORT_SYMBOL(of_phy_connect_with_host_params);
+
 /**
  * of_phy_get_and_connect
  * - Get phy node and connect to the phy described in the device tree
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index da633d34ab86..1df6edca7578 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -24,6 +24,13 @@ of_phy_connect(struct net_device *dev, struct device_node *phy_np,
 	       void (*hndlr)(struct net_device *), u32 flags,
 	       phy_interface_t iface);
 struct phy_device *
+of_phy_connect_with_host_params(struct net_device *dev,
+				struct device_node *phy_np,
+				void (*hndlr)(struct net_device *), u32 flags,
+				phy_interface_t iface,
+				unsigned long *host_interfaces,
+				int host_speed);
+struct phy_device *
 of_phy_get_and_connect(struct net_device *dev, struct device_node *np,
 		       void (*hndlr)(struct net_device *));
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ddf66198f751..000df47f8ae6 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -185,6 +185,12 @@ static inline void phy_interface_or(unsigned long *dst, const unsigned long *a,
 	bitmap_or(dst, a, b, PHY_INTERFACE_MODE_MAX);
 }
 
+static inline void phy_interface_copy(unsigned long *dst,
+				      const unsigned long *src)
+{
+	bitmap_copy(dst, src, PHY_INTERFACE_MODE_MAX);
+}
+
 static inline void phy_interface_set_rgmii(unsigned long *intf)
 {
 	__set_bit(PHY_INTERFACE_MODE_RGMII, intf);
@@ -675,6 +681,8 @@ struct phy_device {
 	/* Host supported PHY interface types. Should be ignored if empty. */
 	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
 
+	int host_speed;
+
 	/* Energy efficient ethernet modes which should be prohibited */
 	u32 eee_broken_modes;
 
-- 
2.25.1

[PATCH RFC 2/3] net: phy: marvell10g: Add host interface speed configuration

[Yoshihiro Shimoda]

Add support for selecting host speed mode. For now, only support
1000M bps.

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

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 383a9c9f36e5..daf3242c6078 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -101,6 +101,10 @@ enum {
 	MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS	= BIT(13),
 	MV_AN_21X0_SERDES_CTRL2_RUN_INIT	= BIT(15),
 
+	MV_MOD_CONF		= 0xf000,
+	MV_MOD_CONF_SPEED_MASK	= 0x00c0,
+	MV_MOD_CONF_SPEED_1000	= BIT(7),
+
 	/* These registers appear at 0x800X and 0xa00X - the 0xa00X control
 	 * registers appear to set themselves to the 0x800X when AN is
 	 * restarted, but status registers appear readable from either.
@@ -147,6 +151,7 @@ struct mv3310_chip {
 	int (*get_mactype)(struct phy_device *phydev);
 	int (*set_mactype)(struct phy_device *phydev, int mactype);
 	int (*select_mactype)(unsigned long *interfaces);
+	int (*set_macspeed)(struct phy_device *phydev, int macspeed);
 	int (*init_interface)(struct phy_device *phydev, int mactype);
 
 #ifdef CONFIG_HWMON
@@ -644,6 +649,16 @@ static int mv2110_select_mactype(unsigned long *interfaces)
 		return -1;
 }
 
+static int mv2110_set_macspeed(struct phy_device *phydev, int macspeed)
+{
+	if (macspeed != SPEED_1000)
+		return -EOPNOTSUPP;
+
+	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, MV_MOD_CONF,
+			      MV_MOD_CONF_SPEED_MASK,
+			      MV_MOD_CONF_SPEED_1000);
+}
+
 static int mv3310_get_mactype(struct phy_device *phydev)
 {
 	int mactype;
@@ -778,6 +793,13 @@ static int mv3310_config_init(struct phy_device *phydev)
 	if (err)
 		return err;
 
+	/* If host provided host mac speed, try to set the mac speed */
+	if (phydev->host_speed && chip->set_macspeed) {
+		err = chip->set_macspeed(phydev, phydev->host_speed);
+		if (err)
+			return err;
+	}
+
 	/* If host provided host supported interface modes, try to select the
 	 * best one
 	 */
@@ -1181,6 +1203,7 @@ static const struct mv3310_chip mv2110_type = {
 	.get_mactype = mv2110_get_mactype,
 	.set_mactype = mv2110_set_mactype,
 	.select_mactype = mv2110_select_mactype,
+	.set_macspeed = mv2110_set_macspeed,
 	.init_interface = mv2110_init_interface,
 
 #ifdef CONFIG_HWMON
-- 
2.25.1

[PATCH RFC 3/3] net: renesas: rswitch: Pass host parameters to phydev

[Yoshihiro Shimoda]

Use of_phy_connect_with_host_params() to pass host parameters to
phydev. Otherwise, connected PHY cannot work correctly.

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

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index c604331bfd88..bb2f1e667210 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -16,6 +16,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
+#include <linux/phy.h>
 #include <linux/phy/phy.h>
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
@@ -1234,11 +1235,19 @@ static void rswitch_phy_remove_link_mode(struct rswitch_device *rdev,
 
 static int rswitch_phy_init(struct rswitch_device *rdev, struct device_node *phy)
 {
+	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
 	struct phy_device *phydev;
 	int err = 0;
 
-	phydev = of_phy_connect(rdev->ndev, phy, rswitch_adjust_link, 0,
-				rdev->etha->phy_interface);
+	phy_interface_zero(host_interfaces);
+	if (rdev->etha->phy_interface == PHY_INTERFACE_MODE_SGMII)
+		__set_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces);
+
+	phydev = of_phy_connect_with_host_params(rdev->ndev, phy,
+						 rswitch_adjust_link, 0,
+						 rdev->etha->phy_interface,
+						 host_interfaces,
+						 rdev->etha->speed);
 	if (!phydev) {
 		err = -ENOENT;
 		goto out;
-- 
2.25.1

Re: [PATCH RFC 3/3] net: renesas: rswitch: Pass host parameters to phydev

[Marek Behún]

On Wed, 19 Oct 2022 17:50:52 +0900
Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:

> Use of_phy_connect_with_host_params() to pass host parameters to
> phydev. Otherwise, connected PHY cannot work correctly.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/net/ethernet/renesas/rswitch.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index c604331bfd88..bb2f1e667210 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
> +#include <linux/phy.h>
>  #include <linux/phy/phy.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
> @@ -1234,11 +1235,19 @@ static void rswitch_phy_remove_link_mode(struct rswitch_device *rdev,
>  
>  static int rswitch_phy_init(struct rswitch_device *rdev, struct device_node *phy)
>  {
> +	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
>  	struct phy_device *phydev;
>  	int err = 0;
>  
> -	phydev = of_phy_connect(rdev->ndev, phy, rswitch_adjust_link, 0,
> -				rdev->etha->phy_interface);
> +	phy_interface_zero(host_interfaces);
> +	if (rdev->etha->phy_interface == PHY_INTERFACE_MODE_SGMII)
> +		__set_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces);
> +
> +	phydev = of_phy_connect_with_host_params(rdev->ndev, phy,
> +						 rswitch_adjust_link, 0,
> +						 rdev->etha->phy_interface,
> +						 host_interfaces,
> +						 rdev->etha->speed);
>  	if (!phydev) {
>  		err = -ENOENT;
>  		goto out;

NAK. There already is API for doing this: phylink. Adding new, and so
much specific function for this is a waste. Just convert the rswitch
driver to phylink.

Please look at the documentation at
  Documentation/networking/sfp-phylink.rst

Marek

Re: [PATCH RFC 0/3] net: phy: marvell10g: Add host speed setting by an ethernet driver

[Marek Behún]

On Wed, 19 Oct 2022 17:50:49 +0900
Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:

> R-Car S4-8 environment board requires to change the host interface
> as SGMII and the host speed as 1000 Mbps because the strap pin was
> other mode, but the SoC/board cannot work on that mode. Also, after
> the SoC initialized the SERDES once, we cannot re-initialized it
> because the initialized procedure seems all black magic...

Can you tell us which exact mode is configured by the strapping pins?
Isn't it sufficient to just set the MACTYPE to SGMII, without
configuring speed?

Marek

Re: [PATCH RFC 2/3] net: phy: marvell10g: Add host interface speed configuration

[Marek Behún]

On Wed, 19 Oct 2022 17:50:51 +0900
Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:

> Add support for selecting host speed mode. For now, only support
> 1000M bps.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 383a9c9f36e5..daf3242c6078 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -101,6 +101,10 @@ enum {
>  	MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS	= BIT(13),
>  	MV_AN_21X0_SERDES_CTRL2_RUN_INIT	= BIT(15),
>  
> +	MV_MOD_CONF		= 0xf000,
> +	MV_MOD_CONF_SPEED_MASK	= 0x00c0,
> +	MV_MOD_CONF_SPEED_1000	= BIT(7),
> +

Where did you get these values from? My documentation says:
  Mode Configuration
  Device 31, Register 0xF000
  Bits
  7:6   Reserved  R/W  0x3  This must always be 11.


>  	/* These registers appear at 0x800X and 0xa00X - the 0xa00X control
>  	 * registers appear to set themselves to the 0x800X when AN is
>  	 * restarted, but status registers appear readable from either.
> @@ -147,6 +151,7 @@ struct mv3310_chip {
>  	int (*get_mactype)(struct phy_device *phydev);
>  	int (*set_mactype)(struct phy_device *phydev, int mactype);
>  	int (*select_mactype)(unsigned long *interfaces);
> +	int (*set_macspeed)(struct phy_device *phydev, int macspeed);
>  	int (*init_interface)(struct phy_device *phydev, int mactype);
>  
>  #ifdef CONFIG_HWMON
> @@ -644,6 +649,16 @@ static int mv2110_select_mactype(unsigned long *interfaces)
>  		return -1;
>  }
>  
> +static int mv2110_set_macspeed(struct phy_device *phydev, int macspeed)
> +{
> +	if (macspeed != SPEED_1000)
> +		return -EOPNOTSUPP;
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, MV_MOD_CONF,
> +			      MV_MOD_CONF_SPEED_MASK,
> +			      MV_MOD_CONF_SPEED_1000);
> +}

Why not also support other speeds, if we are doing this already?

Marek

Re: [PATCH RFC 2/3] net: phy: marvell10g: Add host interface speed configuration

[Marek Behún]

On Wed, 19 Oct 2022 12:48:39 +0200
Marek Behún <kabel@kernel.org> wrote:

> On Wed, 19 Oct 2022 17:50:51 +0900
> Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> > Add support for selecting host speed mode. For now, only support
> > 1000M bps.
> > 
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > index 383a9c9f36e5..daf3242c6078 100644
> > --- a/drivers/net/phy/marvell10g.c
> > +++ b/drivers/net/phy/marvell10g.c
> > @@ -101,6 +101,10 @@ enum {
> >  	MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS	= BIT(13),
> >  	MV_AN_21X0_SERDES_CTRL2_RUN_INIT	= BIT(15),
> >  
> > +	MV_MOD_CONF		= 0xf000,
> > +	MV_MOD_CONF_SPEED_MASK	= 0x00c0,
> > +	MV_MOD_CONF_SPEED_1000	= BIT(7),
> > +  
> 
> Where did you get these values from? My documentation says:
>   Mode Configuration
>   Device 31, Register 0xF000
>   Bits
>   7:6   Reserved  R/W  0x3  This must always be 11.

Ah, I see. Probably from the MTD API sources...
But the bits should be set to 0x3 after HW reset, which means 10 Gbps,
and this should not interfere with 1000 Mbps SGMII operation. Do you
really need to set this? Isn't setting the MACTYPE to SGMII sufficient?

Marek

Re: [PATCH RFC 0/3] net: phy: marvell10g: Add host speed setting by an ethernet driver

[Russell King (Oracle)]

On Wed, Oct 19, 2022 at 12:44:42PM +0200, Marek Behún wrote:
> On Wed, 19 Oct 2022 17:50:49 +0900
> Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> > R-Car S4-8 environment board requires to change the host interface
> > as SGMII and the host speed as 1000 Mbps because the strap pin was
> > other mode, but the SoC/board cannot work on that mode. Also, after
> > the SoC initialized the SERDES once, we cannot re-initialized it
> > because the initialized procedure seems all black magic...
> 
> Can you tell us which exact mode is configured by the strapping pins?
> Isn't it sufficient to just set the MACTYPE to SGMII, without
> configuring speed?

(To Yoshihiro Shimoda)

Please note that I don't seem to have the patches to review - and as
maintainer of the driver, I therefore NAK this series until I can
review it.

I'm guessing the reason I don't have them is:

2022-10-19 09:51:17 H=relmlor1.renesas.com (relmlie5.idc.renesas.com) [210.160.252.171]:58218 I=[78.32.30.218]:25 F=<> rejected RCPT <linux@armlinux.org.uk>: Faked bounce
2022-10-19 09:51:17 H=relmlor2.renesas.com (relmlie6.idc.renesas.com) [210.160.252.172]:24399 I=[78.32.30.218]:25 F=<> rejected RCPT <linux@armlinux.org.uk>: Faked bounce
2022-10-19 09:51:18 H=relmlor1.renesas.com (relmlie5.idc.renesas.com) [210.160.252.171]:58218 I=[78.32.30.218]:25 F=<> rejected RCPT <linux@armlinux.org.uk>: Faked bounce
2022-10-19 09:51:19 H=relmlor1.renesas.com (relmlie5.idc.renesas.com) [210.160.252.171]:58218 I=[78.32.30.218]:25 F=<> rejected RCPT <linux@armlinux.org.uk>: Faked bounce

Why are you sending patches using the NULL envelope sender - this is a
common trick used by spammers and scammers to get their messages
through. Please don't.

(In case anyone questions this - as all my email is sent from encoded
envelope from addresses rather than my published addresses, bounces
do still work.)

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC 2/3] net: phy: marvell10g: Add host interface speed configuration

[Russell King (Oracle)]

On Wed, Oct 19, 2022 at 12:48:39PM +0200, Marek Behún wrote:
> On Wed, 19 Oct 2022 17:50:51 +0900
> Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> > Add support for selecting host speed mode. For now, only support
> > 1000M bps.
> > 
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > index 383a9c9f36e5..daf3242c6078 100644
> > --- a/drivers/net/phy/marvell10g.c
> > +++ b/drivers/net/phy/marvell10g.c
> > @@ -101,6 +101,10 @@ enum {
> >  	MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS	= BIT(13),
> >  	MV_AN_21X0_SERDES_CTRL2_RUN_INIT	= BIT(15),
> >  
> > +	MV_MOD_CONF		= 0xf000,
> > +	MV_MOD_CONF_SPEED_MASK	= 0x00c0,
> > +	MV_MOD_CONF_SPEED_1000	= BIT(7),
> > +
> 
> Where did you get these values from? My documentation says:
>   Mode Configuration
>   Device 31, Register 0xF000
>   Bits
>   7:6   Reserved  R/W  0x3  This must always be 11.

The closest is from the 88x3310 documentation that indicates these are
the default speed, which are used when the media side is down. There
is a specific sequence to update these.

However, as we seem to be talking about the 2110 here, that should be
reflected in these definitions.

Finally, using BIT() for definitions of a field which can be one of
four possible values is not acceptable. BIT() is for single bits
not for a multi-bit field which can take any possible value but just
the value we're representing there just happens to have a single bit
set.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH RFC 3/3] net: renesas: rswitch: Pass host parameters to phydev

[Yoshihiro Shimoda]

Hi Marek,

> From: Marek Behún, Sent: Wednesday, October 19, 2022 7:41 PM
> 
> On Wed, 19 Oct 2022 17:50:52 +0900
> Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> > Use of_phy_connect_with_host_params() to pass host parameters to
> > phydev. Otherwise, connected PHY cannot work correctly.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/rswitch.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> > index c604331bfd88..bb2f1e667210 100644
> > --- a/drivers/net/ethernet/renesas/rswitch.c
> > +++ b/drivers/net/ethernet/renesas/rswitch.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/of_irq.h>
> >  #include <linux/of_mdio.h>
> >  #include <linux/of_net.h>
> > +#include <linux/phy.h>
> >  #include <linux/phy/phy.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/slab.h>
> > @@ -1234,11 +1235,19 @@ static void rswitch_phy_remove_link_mode(struct rswitch_device *rdev,
> >
> >  static int rswitch_phy_init(struct rswitch_device *rdev, struct device_node *phy)
> >  {
> > +	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
> >  	struct phy_device *phydev;
> >  	int err = 0;
> >
> > -	phydev = of_phy_connect(rdev->ndev, phy, rswitch_adjust_link, 0,
> > -				rdev->etha->phy_interface);
> > +	phy_interface_zero(host_interfaces);
> > +	if (rdev->etha->phy_interface == PHY_INTERFACE_MODE_SGMII)
> > +		__set_bit(PHY_INTERFACE_MODE_SGMII, host_interfaces);
> > +
> > +	phydev = of_phy_connect_with_host_params(rdev->ndev, phy,
> > +						 rswitch_adjust_link, 0,
> > +						 rdev->etha->phy_interface,
> > +						 host_interfaces,
> > +						 rdev->etha->speed);
> >  	if (!phydev) {
> >  		err = -ENOENT;
> >  		goto out;
> 
> NAK. There already is API for doing this: phylink. Adding new, and so
> much specific function for this is a waste. Just convert the rswitch
> driver to phylink.
> 
> Please look at the documentation at
>   Documentation/networking/sfp-phylink.rst

Thank you for your comments! I'll try to convert the rswitch driver to phylink.

Best regards,
Yoshihiro Shimoda

> Marek

RE: [PATCH RFC 0/3] net: phy: marvell10g: Add host speed setting by an ethernet driver

[Yoshihiro Shimoda]

Hi Marek,

> From: Marek Behún, Sent: Wednesday, October 19, 2022 7:45 PM
> 
> On Wed, 19 Oct 2022 17:50:49 +0900
> Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> > R-Car S4-8 environment board requires to change the host interface
> > as SGMII and the host speed as 1000 Mbps because the strap pin was
> > other mode, but the SoC/board cannot work on that mode. Also, after
> > the SoC initialized the SERDES once, we cannot re-initialized it
> > because the initialized procedure seems all black magic...
> 
> Can you tell us which exact mode is configured by the strapping pins?

Sure. The port 0 is:

CONFIG[0] = 0 0 0
CONFIG[1] = 0 1 0
CONFIG[2] = 0 1 0

So, MACTYPE is SXGMII.

> Isn't it sufficient to just set the MACTYPE to SGMII, without
> configuring speed?

If the host speed was not changed, my environment didn't work.
The SERDES of R-Car cannot linkup with the PHY.

Best regards,
Yoshihiro Shimoda

> Marek

RE: [PATCH RFC 2/3] net: phy: marvell10g: Add host interface speed configuration

[Yoshihiro Shimoda]

Hi Marek,

> From: Marek Behún, Sent: Wednesday, October 19, 2022 7:49 PM
> 
> On Wed, 19 Oct 2022 17:50:51 +0900
> Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> 
> > Add support for selecting host speed mode. For now, only support
> > 1000M bps.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > index 383a9c9f36e5..daf3242c6078 100644
> > --- a/drivers/net/phy/marvell10g.c
> > +++ b/drivers/net/phy/marvell10g.c
> > @@ -101,6 +101,10 @@ enum {
> >  	MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS	= BIT(13),
> >  	MV_AN_21X0_SERDES_CTRL2_RUN_INIT	= BIT(15),
> >
> > +	MV_MOD_CONF		= 0xf000,
> > +	MV_MOD_CONF_SPEED_MASK	= 0x00c0,
> > +	MV_MOD_CONF_SPEED_1000	= BIT(7),
> > +
> 
> Where did you get these values from? My documentation says:
>   Mode Configuration
>   Device 31, Register 0xF000
>   Bits
>   7:6   Reserved  R/W  0x3  This must always be 11.

Hmm, that's true. The register description said that.
But, "loopback control" in the functional description said
"default MAC interface Speed". (I don't use loopback mode though...)

> >  	/* These registers appear at 0x800X and 0xa00X - the 0xa00X control
> >  	 * registers appear to set themselves to the 0x800X when AN is
> >  	 * restarted, but status registers appear readable from either.
> > @@ -147,6 +151,7 @@ struct mv3310_chip {
> >  	int (*get_mactype)(struct phy_device *phydev);
> >  	int (*set_mactype)(struct phy_device *phydev, int mactype);
> >  	int (*select_mactype)(unsigned long *interfaces);
> > +	int (*set_macspeed)(struct phy_device *phydev, int macspeed);
> >  	int (*init_interface)(struct phy_device *phydev, int mactype);
> >
> >  #ifdef CONFIG_HWMON
> > @@ -644,6 +649,16 @@ static int mv2110_select_mactype(unsigned long *interfaces)
> >  		return -1;
> >  }
> >
> > +static int mv2110_set_macspeed(struct phy_device *phydev, int macspeed)
> > +{
> > +	if (macspeed != SPEED_1000)
> > +		return -EOPNOTSUPP;
> > +
> > +	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, MV_MOD_CONF,
> > +			      MV_MOD_CONF_SPEED_MASK,
> > +			      MV_MOD_CONF_SPEED_1000);
> > +}
> 
> Why not also support other speeds, if we are doing this already?

The register seems to support other speeds: 10, 100 Mbps and "speed controlled by
other register". I'll update this function if acceptable...
# I'm thinking this is not acceptable because the register description said "Reserved"
# as you mentioned.

Best regards,
Yoshihiro Shimoda

RE: [PATCH RFC 0/3] net: phy: marvell10g: Add host speed setting by an ethernet driver

[Yoshihiro Shimoda]

Hi Russell,

> From: Russell King, Sent: Wednesday, October 19, 2022 8:39 PM
> 
> On Wed, Oct 19, 2022 at 12:44:42PM +0200, Marek Behún wrote:
> > On Wed, 19 Oct 2022 17:50:49 +0900
> > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> >
> > > R-Car S4-8 environment board requires to change the host interface
> > > as SGMII and the host speed as 1000 Mbps because the strap pin was
> > > other mode, but the SoC/board cannot work on that mode. Also, after
> > > the SoC initialized the SERDES once, we cannot re-initialized it
> > > because the initialized procedure seems all black magic...
> >
> > Can you tell us which exact mode is configured by the strapping pins?
> > Isn't it sufficient to just set the MACTYPE to SGMII, without
> > configuring speed?
> 
> (To Yoshihiro Shimoda)
> 
> Please note that I don't seem to have the patches to review - and as
> maintainer of the driver, I therefore NAK this series until I can
> review it.
> 
> I'm guessing the reason I don't have them is:
> 
> 2022-10-19 09:51:17 H=relmlor1.renesas.com (relmlie5.idc.renesas.com) [210.160.252.171]:58218 I=[78.32.30.218]:25 F=<>
> rejected RCPT <linux@armlinux.org.uk>: Faked bounce
> 2022-10-19 09:51:17 H=relmlor2.renesas.com (relmlie6.idc.renesas.com) [210.160.252.172]:24399 I=[78.32.30.218]:25 F=<>
> rejected RCPT <linux@armlinux.org.uk>: Faked bounce
> 2022-10-19 09:51:18 H=relmlor1.renesas.com (relmlie5.idc.renesas.com) [210.160.252.171]:58218 I=[78.32.30.218]:25 F=<>
> rejected RCPT <linux@armlinux.org.uk>: Faked bounce
> 2022-10-19 09:51:19 H=relmlor1.renesas.com (relmlie5.idc.renesas.com) [210.160.252.171]:58218 I=[78.32.30.218]:25 F=<>
> rejected RCPT <linux@armlinux.org.uk>: Faked bounce
> 
> Why are you sending patches using the NULL envelope sender - this is a
> common trick used by spammers and scammers to get their messages
> through. Please don't.

I'm sorry about this. I'll stop such sending patches.

Best regards,
Yoshihiro Shimoda

> (In case anyone questions this - as all my email is sent from encoded
> envelope from addresses rather than my published addresses, bounces
> do still work.)
> 
> Thanks.

RE: [PATCH RFC 2/3] net: phy: marvell10g: Add host interface speed configuration

[Yoshihiro Shimoda]

Hi Russell,

> From: Russell King, Sent: Wednesday, October 19, 2022 8:59 PM
> 
> On Wed, Oct 19, 2022 at 12:48:39PM +0200, Marek Behún wrote:
> > On Wed, 19 Oct 2022 17:50:51 +0900
> > Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote:
> >
> > > Add support for selecting host speed mode. For now, only support
> > > 1000M bps.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > >  drivers/net/phy/marvell10g.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > > index 383a9c9f36e5..daf3242c6078 100644
> > > --- a/drivers/net/phy/marvell10g.c
> > > +++ b/drivers/net/phy/marvell10g.c
> > > @@ -101,6 +101,10 @@ enum {
> > >  	MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS	= BIT(13),
> > >  	MV_AN_21X0_SERDES_CTRL2_RUN_INIT	= BIT(15),
> > >
> > > +	MV_MOD_CONF		= 0xf000,
> > > +	MV_MOD_CONF_SPEED_MASK	= 0x00c0,
> > > +	MV_MOD_CONF_SPEED_1000	= BIT(7),
> > > +
> >
> > Where did you get these values from? My documentation says:
> >   Mode Configuration
> >   Device 31, Register 0xF000
> >   Bits
> >   7:6   Reserved  R/W  0x3  This must always be 11.
> 
> The closest is from the 88x3310 documentation that indicates these are
> the default speed, which are used when the media side is down. There
> is a specific sequence to update these.
> 
> However, as we seem to be talking about the 2110 here, that should be
> reflected in these definitions.

Yes, this is about 2110. So, I'll find other way for my environment somehow.

> Finally, using BIT() for definitions of a field which can be one of
> four possible values is not acceptable. BIT() is for single bits
> not for a multi-bit field which can take any possible value but just
> the value we're representing there just happens to have a single bit
> set.

I understood it. Thanks!

Best regards,
Yoshihiro Shimoda

> --
> RMK's Patch system: