[PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Oleksij Rempel]

This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
auto-negotiation support, we needed to be able to configure the
MASTER-SLAVE role of the port manually or from an application in user
space.

The same UAPI can be used for 1000BASE-T or MultiGBASE-T devices to
force MASTER or SLAVE role. See IEEE 802.3-2018:
22.2.4.3.7 MASTER-SLAVE control register (Register 9)
22.2.4.3.8 MASTER-SLAVE status register (Register 10)
40.5.2 MASTER-SLAVE configuration resolution
45.2.1.185.1 MASTER-SLAVE config value (1.2100.14)
45.2.7.10 MultiGBASE-T AN control 1 register (Register 7.32)

The MASTER-SLAVE role affects the clock configuration:

-------------------------------------------------------------------------------
When the  PHY is configured as MASTER, the PMA Transmit function shall
source TX_TCLK from a local clock source. When configured as SLAVE, the
PMA Transmit function shall source TX_TCLK from the clock recovered from
data stream provided by MASTER.

iMX6Q                     KSZ9031                XXX
------\                /-----------\        /------------\
      |                |           |        |            |
 MAC  |<----RGMII----->| PHY Slave |<------>| PHY Master |
      |<--- 125 MHz ---+-<------/  |        | \          |
------/                \-----------/        \------------/
                                               ^
                                                \-TX_TCLK

-------------------------------------------------------------------------------

Since some clock or link related issues are only reproducible in a
specific MASTER-SLAVE-role, MAC and PHY configuration, it is beneficial
to provide generic (not 100BASE-T1 specific) interface to the user space
for configuration flexibility and trouble shooting.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 Documentation/networking/ethtool-netlink.rst |  2 +
 drivers/net/phy/phy.c                        |  3 +-
 drivers/net/phy/phy_device.c                 | 75 ++++++++++++++++++++
 include/linux/phy.h                          |  1 +
 include/uapi/linux/ethtool.h                 | 11 ++-
 include/uapi/linux/ethtool_netlink.h         |  1 +
 include/uapi/linux/mii.h                     |  2 +
 net/ethtool/linkmodes.c                      |  9 ++-
 8 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index f1f868479ceb8..83127a6e42b17 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -368,6 +368,7 @@ Kernel response contents:
   ``ETHTOOL_A_LINKMODES_PEER``          bitset  partner link modes
   ``ETHTOOL_A_LINKMODES_SPEED``         u32     link speed (Mb/s)
   ``ETHTOOL_A_LINKMODES_DUPLEX``        u8      duplex mode
+  ``ETHTOOL_A_LINKMODES_MASTER_SLAVE``  u8      Master/slave port mode
   ====================================  ======  ==========================
 
 For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
@@ -390,6 +391,7 @@ Request contents:
   ``ETHTOOL_A_LINKMODES_PEER``          bitset  partner link modes
   ``ETHTOOL_A_LINKMODES_SPEED``         u32     link speed (Mb/s)
   ``ETHTOOL_A_LINKMODES_DUPLEX``        u8      duplex mode
+  ``ETHTOOL_A_LINKMODES_MASTER_SLAVE``  u8      Master/slave port mode
   ====================================  ======  ==========================
 
 ``ETHTOOL_A_LINKMODES_OURS`` bit set allows setting advertised link modes. If
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d76e038cf2cb5..9f48141f1e701 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -294,7 +294,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 			 phydev->advertising, autoneg == AUTONEG_ENABLE);
 
 	phydev->duplex = duplex;
-
+	phydev->master_slave = cmd->base.master_slave;
 	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
 
 	/* Restart the PHY */
@@ -313,6 +313,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
 
 	cmd->base.speed = phydev->speed;
 	cmd->base.duplex = phydev->duplex;
+	cmd->base.master_slave = phydev->master_slave;
 	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
 		cmd->base.port = PORT_BNC;
 	else
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c8b0c34030d32..d5edf2bc40e43 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -604,6 +604,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	dev->asym_pause = 0;
 	dev->link = 0;
 	dev->interface = PHY_INTERFACE_MODE_GMII;
+	dev->master_slave = PORT_MODE_UNKNOWN;
 
 	dev->autoneg = AUTONEG_ENABLE;
 
@@ -1772,6 +1773,68 @@ int genphy_setup_forced(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_setup_forced);
 
+static int genphy_setup_master_slave(struct phy_device *phydev)
+{
+	u16 ctl = 0;
+
+	if (!phydev->is_gigabit_capable)
+		return 0;
+
+	switch (phydev->master_slave) {
+	case PORT_MODE_MASTER:
+		ctl |= CTL1000_PREFER_MASTER;
+		/* fallthrough */
+	case PORT_MODE_SLAVE:
+		/* CTL1000_ENABLE_MASTER is zero */
+		break;
+	case PORT_MODE_MASTER_FORCE:
+		ctl |= CTL1000_AS_MASTER;
+		/* fallthrough */
+	case PORT_MODE_SLAVE_FORCE:
+		ctl |= CTL1000_ENABLE_MASTER;
+		break;
+	case PORT_MODE_UNKNOWN:
+		return 0;
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		return 0;
+	}
+
+	return phy_modify_changed(phydev, MII_CTRL1000,
+				  (CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER |
+				   CTL1000_PREFER_MASTER), ctl);
+}
+
+static int genphy_read_master_slave(struct phy_device *phydev)
+{
+	u16 ctl, stat;
+
+	if (!phydev->is_gigabit_capable)
+		return 0;
+
+	ctl = phy_read(phydev, MII_CTRL1000);
+	if (ctl < 0)
+		return ctl;
+
+	stat = phy_read(phydev, MII_STAT1000);
+	if (stat < 0)
+		return stat;
+
+	if (ctl & CTL1000_ENABLE_MASTER) {
+		if (stat & LPA_1000MSRES)
+			phydev->master_slave = PORT_MODE_MASTER_FORCE;
+		else
+			phydev->master_slave = PORT_MODE_SLAVE_FORCE;
+	} else {
+		if (stat & LPA_1000MSRES)
+			phydev->master_slave = PORT_MODE_MASTER;
+		else
+			phydev->master_slave = PORT_MODE_SLAVE;
+	}
+
+	return 0;
+}
+
 /**
  * genphy_restart_aneg - Enable and Restart Autonegotiation
  * @phydev: target phy_device struct
@@ -1830,6 +1893,12 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed)
 	if (genphy_config_eee_advert(phydev))
 		changed = true;
 
+	err = genphy_setup_master_slave(phydev);
+	if (err < 0)
+		return err;
+	else if (err)
+		changed = true;
+
 	if (AUTONEG_ENABLE != phydev->autoneg)
 		return genphy_setup_forced(phydev);
 
@@ -2062,6 +2131,11 @@ int genphy_read_status(struct phy_device *phydev)
 	phydev->duplex = DUPLEX_UNKNOWN;
 	phydev->pause = 0;
 	phydev->asym_pause = 0;
+	phydev->master_slave = PORT_MODE_UNKNOWN;
+
+	err = genphy_read_master_slave(phydev);
+	if (err < 0)
+		return err;
 
 	err = genphy_read_lpa(phydev);
 	if (err < 0)
@@ -2103,6 +2177,7 @@ int genphy_c37_read_status(struct phy_device *phydev)
 	phydev->duplex = DUPLEX_UNKNOWN;
 	phydev->pause = 0;
 	phydev->asym_pause = 0;
+	phydev->master_slave = PORT_MODE_UNKNOWN;
 
 	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
 		lpa = phy_read(phydev, MII_LPA);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c570e162e05e5..de5f934223069 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -421,6 +421,7 @@ struct phy_device {
 	int duplex;
 	int pause;
 	int asym_pause;
+	int master_slave;
 
 	/* Union of PHY and Attached devices' supported link modes */
 	/* See ethtool.h for more info */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 2405ab2263779..bcbc44003c823 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1659,6 +1659,13 @@ static inline int ethtool_validate_duplex(__u8 duplex)
 	return 0;
 }
 
+/* Port mode */
+#define PORT_MODE_MASTER	0x00
+#define PORT_MODE_SLAVE		0x01
+#define PORT_MODE_MASTER_FORCE	0x02
+#define PORT_MODE_SLAVE_FORCE	0x03
+#define PORT_MODE_UNKNOWN	0xff
+
 /* Which connector port. */
 #define PORT_TP			0x00
 #define PORT_AUI		0x01
@@ -1850,6 +1857,7 @@ enum ethtool_reset_flags {
  *	autonegotiation; 0 if unknown or not applicable.  Read-only.
  * @transceiver: Used to distinguish different possible PHY types,
  *	reported consistently by PHYLIB.  Read-only.
+ * @master_slave: Master or slave port mode.
  *
  * If autonegotiation is disabled, the speed and @duplex represent the
  * fixed link mode and are writable if the driver supports multiple
@@ -1897,7 +1905,8 @@ struct ethtool_link_settings {
 	__u8	eth_tp_mdix_ctrl;
 	__s8	link_mode_masks_nwords;
 	__u8	transceiver;
-	__u8	reserved1[3];
+	__u8	master_slave;
+	__u8	reserved1[2];
 	__u32	reserved[7];
 	__u32	link_mode_masks[0];
 	/* layout of link_mode_masks fields:
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 7e0b460f872c0..e04d47cb5f227 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -185,6 +185,7 @@ enum {
 	ETHTOOL_A_LINKMODES_PEER,		/* bitset */
 	ETHTOOL_A_LINKMODES_SPEED,		/* u32 */
 	ETHTOOL_A_LINKMODES_DUPLEX,		/* u8 */
+	ETHTOOL_A_LINKMODES_MASTER_SLAVE,	/* u8 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_LINKMODES_CNT,
diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index 0b9c3beda345b..78eac603a84fb 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -146,11 +146,13 @@
 /* 1000BASE-T Control register */
 #define ADVERTISE_1000FULL	0x0200  /* Advertise 1000BASE-T full duplex */
 #define ADVERTISE_1000HALF	0x0100  /* Advertise 1000BASE-T half duplex */
+#define CTL1000_PREFER_MASTER	0x0400  /* prefer to operate as master */
 #define CTL1000_AS_MASTER	0x0800
 #define CTL1000_ENABLE_MASTER	0x1000
 
 /* 1000BASE-T Status register */
 #define LPA_1000MSFAIL		0x8000	/* Master/Slave resolution failure */
+#define LPA_1000MSRES		0x4000	/* Master/Slave resolution status */
 #define LPA_1000LOCALRXOK	0x2000	/* Link partner local receiver status */
 #define LPA_1000REMRXOK		0x1000	/* Link partner remote receiver status */
 #define LPA_1000FULL		0x0800	/* Link partner 1000BASE-T full duplex */
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 96f20be64553e..dc15b88e64c6a 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -27,6 +27,7 @@ linkmodes_get_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
 	[ETHTOOL_A_LINKMODES_PEER]		= { .type = NLA_REJECT },
 	[ETHTOOL_A_LINKMODES_SPEED]		= { .type = NLA_REJECT },
 	[ETHTOOL_A_LINKMODES_DUPLEX]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_LINKMODES_MASTER_SLAVE]	= { .type = NLA_REJECT },
 };
 
 static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
@@ -69,6 +70,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
 	len = nla_total_size(sizeof(u8)) /* LINKMODES_AUTONEG */
 		+ nla_total_size(sizeof(u32)) /* LINKMODES_SPEED */
 		+ nla_total_size(sizeof(u8)) /* LINKMODES_DUPLEX */
+		+ nla_total_size(sizeof(u8)) /* LINKMODES_MASTER_SLAVE */
 		+ 0;
 	ret = ethnl_bitset_size(ksettings->link_modes.advertising,
 				ksettings->link_modes.supported,
@@ -119,7 +121,9 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
 	}
 
 	if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
-	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
+	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
+	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE,
+		       lsettings->master_slave))
 		return -EMSGSIZE;
 
 	return 0;
@@ -248,6 +252,7 @@ linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
 	[ETHTOOL_A_LINKMODES_PEER]		= { .type = NLA_REJECT },
 	[ETHTOOL_A_LINKMODES_SPEED]		= { .type = NLA_U32 },
 	[ETHTOOL_A_LINKMODES_DUPLEX]		= { .type = NLA_U8 },
+	[ETHTOOL_A_LINKMODES_MASTER_SLAVE]	= { .type = NLA_U8 },
 };
 
 /* Set advertised link modes to all supported modes matching requested speed
@@ -310,6 +315,8 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
 			 mod);
 	ethnl_update_u8(&lsettings->duplex, tb[ETHTOOL_A_LINKMODES_DUPLEX],
 			mod);
+	ethnl_update_u8(&lsettings->master_slave,
+			tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE], mod);
 
 	if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
 	    (req_speed || req_duplex) &&
-- 
2.26.0.rc2

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 13853 bytes --]

Cc: Marek Vasut <marex@denx.de>

On Wed, Apr 15, 2020 at 02:12:09PM +0200, Oleksij Rempel wrote:
> This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
> auto-negotiation support, we needed to be able to configure the
> MASTER-SLAVE role of the port manually or from an application in user
> space.
> 
> The same UAPI can be used for 1000BASE-T or MultiGBASE-T devices to
> force MASTER or SLAVE role. See IEEE 802.3-2018:
> 22.2.4.3.7 MASTER-SLAVE control register (Register 9)
> 22.2.4.3.8 MASTER-SLAVE status register (Register 10)
> 40.5.2 MASTER-SLAVE configuration resolution
> 45.2.1.185.1 MASTER-SLAVE config value (1.2100.14)
> 45.2.7.10 MultiGBASE-T AN control 1 register (Register 7.32)
> 
> The MASTER-SLAVE role affects the clock configuration:
> 
> -------------------------------------------------------------------------------
> When the  PHY is configured as MASTER, the PMA Transmit function shall
> source TX_TCLK from a local clock source. When configured as SLAVE, the
> PMA Transmit function shall source TX_TCLK from the clock recovered from
> data stream provided by MASTER.
> 
> iMX6Q                     KSZ9031                XXX
> ------\                /-----------\        /------------\
>       |                |           |        |            |
>  MAC  |<----RGMII----->| PHY Slave |<------>| PHY Master |
>       |<--- 125 MHz ---+-<------/  |        | \          |
> ------/                \-----------/        \------------/
>                                                ^
>                                                 \-TX_TCLK
> 
> -------------------------------------------------------------------------------
> 
> Since some clock or link related issues are only reproducible in a
> specific MASTER-SLAVE-role, MAC and PHY configuration, it is beneficial
> to provide generic (not 100BASE-T1 specific) interface to the user space
> for configuration flexibility and trouble shooting.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  Documentation/networking/ethtool-netlink.rst |  2 +
>  drivers/net/phy/phy.c                        |  3 +-
>  drivers/net/phy/phy_device.c                 | 75 ++++++++++++++++++++
>  include/linux/phy.h                          |  1 +
>  include/uapi/linux/ethtool.h                 | 11 ++-
>  include/uapi/linux/ethtool_netlink.h         |  1 +
>  include/uapi/linux/mii.h                     |  2 +
>  net/ethtool/linkmodes.c                      |  9 ++-
>  8 files changed, 101 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index f1f868479ceb8..83127a6e42b17 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -368,6 +368,7 @@ Kernel response contents:
>    ``ETHTOOL_A_LINKMODES_PEER``          bitset  partner link modes
>    ``ETHTOOL_A_LINKMODES_SPEED``         u32     link speed (Mb/s)
>    ``ETHTOOL_A_LINKMODES_DUPLEX``        u8      duplex mode
> +  ``ETHTOOL_A_LINKMODES_MASTER_SLAVE``  u8      Master/slave port mode
>    ====================================  ======  ==========================
>  
>  For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
> @@ -390,6 +391,7 @@ Request contents:
>    ``ETHTOOL_A_LINKMODES_PEER``          bitset  partner link modes
>    ``ETHTOOL_A_LINKMODES_SPEED``         u32     link speed (Mb/s)
>    ``ETHTOOL_A_LINKMODES_DUPLEX``        u8      duplex mode
> +  ``ETHTOOL_A_LINKMODES_MASTER_SLAVE``  u8      Master/slave port mode
>    ====================================  ======  ==========================
>  
>  ``ETHTOOL_A_LINKMODES_OURS`` bit set allows setting advertised link modes. If
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d76e038cf2cb5..9f48141f1e701 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -294,7 +294,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  			 phydev->advertising, autoneg == AUTONEG_ENABLE);
>  
>  	phydev->duplex = duplex;
> -
> +	phydev->master_slave = cmd->base.master_slave;
>  	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>  
>  	/* Restart the PHY */
> @@ -313,6 +313,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  
>  	cmd->base.speed = phydev->speed;
>  	cmd->base.duplex = phydev->duplex;
> +	cmd->base.master_slave = phydev->master_slave;
>  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>  		cmd->base.port = PORT_BNC;
>  	else
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c8b0c34030d32..d5edf2bc40e43 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -604,6 +604,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
>  	dev->asym_pause = 0;
>  	dev->link = 0;
>  	dev->interface = PHY_INTERFACE_MODE_GMII;
> +	dev->master_slave = PORT_MODE_UNKNOWN;
>  
>  	dev->autoneg = AUTONEG_ENABLE;
>  
> @@ -1772,6 +1773,68 @@ int genphy_setup_forced(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(genphy_setup_forced);
>  
> +static int genphy_setup_master_slave(struct phy_device *phydev)
> +{
> +	u16 ctl = 0;
> +
> +	if (!phydev->is_gigabit_capable)
> +		return 0;
> +
> +	switch (phydev->master_slave) {
> +	case PORT_MODE_MASTER:
> +		ctl |= CTL1000_PREFER_MASTER;
> +		/* fallthrough */
> +	case PORT_MODE_SLAVE:
> +		/* CTL1000_ENABLE_MASTER is zero */
> +		break;
> +	case PORT_MODE_MASTER_FORCE:
> +		ctl |= CTL1000_AS_MASTER;
> +		/* fallthrough */
> +	case PORT_MODE_SLAVE_FORCE:
> +		ctl |= CTL1000_ENABLE_MASTER;
> +		break;
> +	case PORT_MODE_UNKNOWN:
> +		return 0;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return 0;
> +	}
> +
> +	return phy_modify_changed(phydev, MII_CTRL1000,
> +				  (CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER |
> +				   CTL1000_PREFER_MASTER), ctl);
> +}
> +
> +static int genphy_read_master_slave(struct phy_device *phydev)
> +{
> +	u16 ctl, stat;
> +
> +	if (!phydev->is_gigabit_capable)
> +		return 0;
> +
> +	ctl = phy_read(phydev, MII_CTRL1000);
> +	if (ctl < 0)
> +		return ctl;
> +
> +	stat = phy_read(phydev, MII_STAT1000);
> +	if (stat < 0)
> +		return stat;
> +
> +	if (ctl & CTL1000_ENABLE_MASTER) {
> +		if (stat & LPA_1000MSRES)
> +			phydev->master_slave = PORT_MODE_MASTER_FORCE;
> +		else
> +			phydev->master_slave = PORT_MODE_SLAVE_FORCE;
> +	} else {
> +		if (stat & LPA_1000MSRES)
> +			phydev->master_slave = PORT_MODE_MASTER;
> +		else
> +			phydev->master_slave = PORT_MODE_SLAVE;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * genphy_restart_aneg - Enable and Restart Autonegotiation
>   * @phydev: target phy_device struct
> @@ -1830,6 +1893,12 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed)
>  	if (genphy_config_eee_advert(phydev))
>  		changed = true;
>  
> +	err = genphy_setup_master_slave(phydev);
> +	if (err < 0)
> +		return err;
> +	else if (err)
> +		changed = true;
> +
>  	if (AUTONEG_ENABLE != phydev->autoneg)
>  		return genphy_setup_forced(phydev);
>  
> @@ -2062,6 +2131,11 @@ int genphy_read_status(struct phy_device *phydev)
>  	phydev->duplex = DUPLEX_UNKNOWN;
>  	phydev->pause = 0;
>  	phydev->asym_pause = 0;
> +	phydev->master_slave = PORT_MODE_UNKNOWN;
> +
> +	err = genphy_read_master_slave(phydev);
> +	if (err < 0)
> +		return err;
>  
>  	err = genphy_read_lpa(phydev);
>  	if (err < 0)
> @@ -2103,6 +2177,7 @@ int genphy_c37_read_status(struct phy_device *phydev)
>  	phydev->duplex = DUPLEX_UNKNOWN;
>  	phydev->pause = 0;
>  	phydev->asym_pause = 0;
> +	phydev->master_slave = PORT_MODE_UNKNOWN;
>  
>  	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>  		lpa = phy_read(phydev, MII_LPA);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index c570e162e05e5..de5f934223069 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -421,6 +421,7 @@ struct phy_device {
>  	int duplex;
>  	int pause;
>  	int asym_pause;
> +	int master_slave;
>  
>  	/* Union of PHY and Attached devices' supported link modes */
>  	/* See ethtool.h for more info */
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 2405ab2263779..bcbc44003c823 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1659,6 +1659,13 @@ static inline int ethtool_validate_duplex(__u8 duplex)
>  	return 0;
>  }
>  
> +/* Port mode */
> +#define PORT_MODE_MASTER	0x00
> +#define PORT_MODE_SLAVE		0x01
> +#define PORT_MODE_MASTER_FORCE	0x02
> +#define PORT_MODE_SLAVE_FORCE	0x03
> +#define PORT_MODE_UNKNOWN	0xff
> +
>  /* Which connector port. */
>  #define PORT_TP			0x00
>  #define PORT_AUI		0x01
> @@ -1850,6 +1857,7 @@ enum ethtool_reset_flags {
>   *	autonegotiation; 0 if unknown or not applicable.  Read-only.
>   * @transceiver: Used to distinguish different possible PHY types,
>   *	reported consistently by PHYLIB.  Read-only.
> + * @master_slave: Master or slave port mode.
>   *
>   * If autonegotiation is disabled, the speed and @duplex represent the
>   * fixed link mode and are writable if the driver supports multiple
> @@ -1897,7 +1905,8 @@ struct ethtool_link_settings {
>  	__u8	eth_tp_mdix_ctrl;
>  	__s8	link_mode_masks_nwords;
>  	__u8	transceiver;
> -	__u8	reserved1[3];
> +	__u8	master_slave;
> +	__u8	reserved1[2];
>  	__u32	reserved[7];
>  	__u32	link_mode_masks[0];
>  	/* layout of link_mode_masks fields:
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 7e0b460f872c0..e04d47cb5f227 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -185,6 +185,7 @@ enum {
>  	ETHTOOL_A_LINKMODES_PEER,		/* bitset */
>  	ETHTOOL_A_LINKMODES_SPEED,		/* u32 */
>  	ETHTOOL_A_LINKMODES_DUPLEX,		/* u8 */
> +	ETHTOOL_A_LINKMODES_MASTER_SLAVE,	/* u8 */
>  
>  	/* add new constants above here */
>  	__ETHTOOL_A_LINKMODES_CNT,
> diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
> index 0b9c3beda345b..78eac603a84fb 100644
> --- a/include/uapi/linux/mii.h
> +++ b/include/uapi/linux/mii.h
> @@ -146,11 +146,13 @@
>  /* 1000BASE-T Control register */
>  #define ADVERTISE_1000FULL	0x0200  /* Advertise 1000BASE-T full duplex */
>  #define ADVERTISE_1000HALF	0x0100  /* Advertise 1000BASE-T half duplex */
> +#define CTL1000_PREFER_MASTER	0x0400  /* prefer to operate as master */
>  #define CTL1000_AS_MASTER	0x0800
>  #define CTL1000_ENABLE_MASTER	0x1000
>  
>  /* 1000BASE-T Status register */
>  #define LPA_1000MSFAIL		0x8000	/* Master/Slave resolution failure */
> +#define LPA_1000MSRES		0x4000	/* Master/Slave resolution status */
>  #define LPA_1000LOCALRXOK	0x2000	/* Link partner local receiver status */
>  #define LPA_1000REMRXOK		0x1000	/* Link partner remote receiver status */
>  #define LPA_1000FULL		0x0800	/* Link partner 1000BASE-T full duplex */
> diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
> index 96f20be64553e..dc15b88e64c6a 100644
> --- a/net/ethtool/linkmodes.c
> +++ b/net/ethtool/linkmodes.c
> @@ -27,6 +27,7 @@ linkmodes_get_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
>  	[ETHTOOL_A_LINKMODES_PEER]		= { .type = NLA_REJECT },
>  	[ETHTOOL_A_LINKMODES_SPEED]		= { .type = NLA_REJECT },
>  	[ETHTOOL_A_LINKMODES_DUPLEX]		= { .type = NLA_REJECT },
> +	[ETHTOOL_A_LINKMODES_MASTER_SLAVE]	= { .type = NLA_REJECT },
>  };
>  
>  static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
> @@ -69,6 +70,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
>  	len = nla_total_size(sizeof(u8)) /* LINKMODES_AUTONEG */
>  		+ nla_total_size(sizeof(u32)) /* LINKMODES_SPEED */
>  		+ nla_total_size(sizeof(u8)) /* LINKMODES_DUPLEX */
> +		+ nla_total_size(sizeof(u8)) /* LINKMODES_MASTER_SLAVE */
>  		+ 0;
>  	ret = ethnl_bitset_size(ksettings->link_modes.advertising,
>  				ksettings->link_modes.supported,
> @@ -119,7 +121,9 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
>  	}
>  
>  	if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
> -	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
> +	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
> +	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE,
> +		       lsettings->master_slave))
>  		return -EMSGSIZE;
>  
>  	return 0;
> @@ -248,6 +252,7 @@ linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
>  	[ETHTOOL_A_LINKMODES_PEER]		= { .type = NLA_REJECT },
>  	[ETHTOOL_A_LINKMODES_SPEED]		= { .type = NLA_U32 },
>  	[ETHTOOL_A_LINKMODES_DUPLEX]		= { .type = NLA_U8 },
> +	[ETHTOOL_A_LINKMODES_MASTER_SLAVE]	= { .type = NLA_U8 },
>  };
>  
>  /* Set advertised link modes to all supported modes matching requested speed
> @@ -310,6 +315,8 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
>  			 mod);
>  	ethnl_update_u8(&lsettings->duplex, tb[ETHTOOL_A_LINKMODES_DUPLEX],
>  			mod);
> +	ethnl_update_u8(&lsettings->master_slave,
> +			tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE], mod);
>  
>  	if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
>  	    (req_speed || req_duplex) &&
> -- 
> 2.26.0.rc2
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Michal Kubecek]

On Wed, Apr 15, 2020 at 02:19:40PM +0200, Oleksij Rempel wrote:
> Cc: Marek Vasut <marex@denx.de>
> 
> On Wed, Apr 15, 2020 at 02:12:09PM +0200, Oleksij Rempel wrote:
> > This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
> > auto-negotiation support, we needed to be able to configure the
> > MASTER-SLAVE role of the port manually or from an application in user
> > space.
> > 
> > The same UAPI can be used for 1000BASE-T or MultiGBASE-T devices to
> > force MASTER or SLAVE role. See IEEE 802.3-2018:
> > 22.2.4.3.7 MASTER-SLAVE control register (Register 9)
> > 22.2.4.3.8 MASTER-SLAVE status register (Register 10)
> > 40.5.2 MASTER-SLAVE configuration resolution
> > 45.2.1.185.1 MASTER-SLAVE config value (1.2100.14)
> > 45.2.7.10 MultiGBASE-T AN control 1 register (Register 7.32)
> > 
> > The MASTER-SLAVE role affects the clock configuration:
> > 
> > -------------------------------------------------------------------------------
> > When the  PHY is configured as MASTER, the PMA Transmit function shall
> > source TX_TCLK from a local clock source. When configured as SLAVE, the
> > PMA Transmit function shall source TX_TCLK from the clock recovered from
> > data stream provided by MASTER.
> > 
> > iMX6Q                     KSZ9031                XXX
> > ------\                /-----------\        /------------\
> >       |                |           |        |            |
> >  MAC  |<----RGMII----->| PHY Slave |<------>| PHY Master |
> >       |<--- 125 MHz ---+-<------/  |        | \          |
> > ------/                \-----------/        \------------/
> >                                                ^
> >                                                 \-TX_TCLK
> > 
> > -------------------------------------------------------------------------------
> > 
> > Since some clock or link related issues are only reproducible in a
> > specific MASTER-SLAVE-role, MAC and PHY configuration, it is beneficial
> > to provide generic (not 100BASE-T1 specific) interface to the user space
> > for configuration flexibility and trouble shooting.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
[...]
> > +/* Port mode */
> > +#define PORT_MODE_MASTER	0x00
> > +#define PORT_MODE_SLAVE		0x01
> > +#define PORT_MODE_MASTER_FORCE	0x02
> > +#define PORT_MODE_SLAVE_FORCE	0x03
> > +#define PORT_MODE_UNKNOWN	0xff

Shouldn't 0 rather be something like PORT_MODE_UNSUPPORTED or
PORT_MODE_NONE? If I see correctly, all drivers not setting the new
field (which would be all drivers right now and almost all later) will
leave the value at 0 which would be interpreted as PORT_MODE_MASTER.

Michal

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 3314 bytes --]

On Wed, Apr 15, 2020 at 02:43:43PM +0200, Michal Kubecek wrote:
> On Wed, Apr 15, 2020 at 02:19:40PM +0200, Oleksij Rempel wrote:
> > Cc: Marek Vasut <marex@denx.de>
> > 
> > On Wed, Apr 15, 2020 at 02:12:09PM +0200, Oleksij Rempel wrote:
> > > This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
> > > auto-negotiation support, we needed to be able to configure the
> > > MASTER-SLAVE role of the port manually or from an application in user
> > > space.
> > > 
> > > The same UAPI can be used for 1000BASE-T or MultiGBASE-T devices to
> > > force MASTER or SLAVE role. See IEEE 802.3-2018:
> > > 22.2.4.3.7 MASTER-SLAVE control register (Register 9)
> > > 22.2.4.3.8 MASTER-SLAVE status register (Register 10)
> > > 40.5.2 MASTER-SLAVE configuration resolution
> > > 45.2.1.185.1 MASTER-SLAVE config value (1.2100.14)
> > > 45.2.7.10 MultiGBASE-T AN control 1 register (Register 7.32)
> > > 
> > > The MASTER-SLAVE role affects the clock configuration:
> > > 
> > > -------------------------------------------------------------------------------
> > > When the  PHY is configured as MASTER, the PMA Transmit function shall
> > > source TX_TCLK from a local clock source. When configured as SLAVE, the
> > > PMA Transmit function shall source TX_TCLK from the clock recovered from
> > > data stream provided by MASTER.
> > > 
> > > iMX6Q                     KSZ9031                XXX
> > > ------\                /-----------\        /------------\
> > >       |                |           |        |            |
> > >  MAC  |<----RGMII----->| PHY Slave |<------>| PHY Master |
> > >       |<--- 125 MHz ---+-<------/  |        | \          |
> > > ------/                \-----------/        \------------/
> > >                                                ^
> > >                                                 \-TX_TCLK
> > > 
> > > -------------------------------------------------------------------------------
> > > 
> > > Since some clock or link related issues are only reproducible in a
> > > specific MASTER-SLAVE-role, MAC and PHY configuration, it is beneficial
> > > to provide generic (not 100BASE-T1 specific) interface to the user space
> > > for configuration flexibility and trouble shooting.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> [...]
> > > +/* Port mode */
> > > +#define PORT_MODE_MASTER	0x00
> > > +#define PORT_MODE_SLAVE		0x01
> > > +#define PORT_MODE_MASTER_FORCE	0x02
> > > +#define PORT_MODE_SLAVE_FORCE	0x03
> > > +#define PORT_MODE_UNKNOWN	0xff
> 
> Shouldn't 0 rather be something like PORT_MODE_UNSUPPORTED or
> PORT_MODE_NONE? If I see correctly, all drivers not setting the new
> field (which would be all drivers right now and almost all later) will
> leave the value at 0 which would be interpreted as PORT_MODE_MASTER.

Yes, you right. I was thinking about it and decided to follow the duplex
code pattern. Will fix in the next version.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 19051 bytes --]

Here are some examples how it works. Only nxp-tja11xx need additional
patches. Most of other gigabit PHYs: micrel, etheros, realtek ... will work
without patches.

For Broadr-Reach/100Base-T1 PHY (NXP TJA1102):
-------------------------------------------------------------------------------
root@test:~ ip l s dev t1slave up
  [11138.600029] sja1105 spi1.0 t1slave: configuring for phy/rmii link mode
  [11138.657649] sja1105 spi1.0 t1slave: Link is Up - 100Mbps/Full - flow control off
  [11138.665632] IPv6: ADDRCONF(NETDEV_CHANGE): t1slave: link becomes ready

root@test:~ ethtool t1slave
Settings for t1slave:
        Supported ports: [  ]
        Supported link modes:   100baseT1/Full
        Supported pause frame use: No
        Supports auto-negotiation: No
        Supported FEC modes: Not reported
        Advertised link modes:  100baseT1/Full
        Advertised pause frame use: No
        Advertised auto-negotiation: No
        Advertised FEC modes: Not reported
        Speed: 100Mb/s
        Duplex: Full
        Auto-negotiation: off
        Port mode: Slave         <----- new option shows current port role
        Port: MII
        PHYAD: 5
        Transceiver: external
        Supports Wake-on: d
        Wake-on: d
        Link detected: yes

root@test:~ ethtool -s t1slave master-slave master
  [11228.024190] sja1105 spi1.0 t1slave: Link is Down
  [11228.219143] sja1105 spi1.0 t1slave: Link is Up - 100Mbps/Full - flow control off

root@test:~ ethtool t1slave
Settings for t1slave:
        Supported ports: [  ]
        Supported link modes:   100baseT1/Full
        Supported pause frame use: No
        Supports auto-negotiation: No
        Supported FEC modes: Not reported
        Advertised link modes:  100baseT1/Full
        Advertised pause frame use: No
        Advertised auto-negotiation: No
        Advertised FEC modes: Not reported
        Speed: 100Mb/s
        Duplex: Full
        Auto-negotiation: off
        Port mode: Master        <------- the role is changed now
        Port: MII
        PHYAD: 5
        Transceiver: external
        Supports Wake-on: d
        Wake-on: d
        Link detected: yes
root@test:~ 
-------------------------------------------------------------------------------

For Micrel KSZ9031 PHY:
-------------------------------------------------------------------------------
root@test:~ ip l s dev rj45 up
  [11681.778197] sja1105 spi1.0 rj45: configuring for phy/rgmii-id link mode
  [11691.604875] sja1105 spi1.0 rj45: Link is Up - 1Gbps/Full - flow control off
  [11691.611984] IPv6: ADDRCONF(NETDEV_CHANGE): rj45: link becomes ready

root@test:~ ethtool rj45
Settings for rj45:
        Supported ports: [ MII ]
        Supported link modes:   10baseT/Full
                                100baseT/Full
                                1000baseT/Full
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  10baseT/Full
                                100baseT/Full
                                1000baseT/Full
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Link partner advertised link modes:  Not reported
        Link partner advertised pause frame use: No
        Link partner advertised auto-negotiation: No
        Link partner advertised FEC modes: No
        Speed: 1000Mb/s
        Duplex: Full
        Auto-negotiation: on
        Port mode: Slave             <---- by defaul, in most case we will get slave mode
        Port: MII
        PHYAD: 2
        Transceiver: external
        Supports Wake-on: d
        Wake-on: d
        Link detected: yes
        
root@test:~ ethtool -s rj45 master-slave master-force
  [11784.481453] sja1105 spi1.0 rj45: Link is Down
  [11788.383342] sja1105 spi1.0 rj45: Link is Up - 1Gbps/Full - flow control off

root@test:~ ethtool rj45
Settings for rj45:
        Supported ports: [ MII ]
        Supported link modes:   10baseT/Full
                                100baseT/Full
                                1000baseT/Full
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  10baseT/Full
                                100baseT/Full
                                1000baseT/Full
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Link partner advertised link modes:  Not reported
        Link partner advertised pause frame use: No
        Link partner advertised auto-negotiation: No
        Link partner advertised FEC modes: No
        Speed: 1000Mb/s
        Duplex: Full
        Auto-negotiation: on
        Port mode: Master (force)    <-----  now we have forced Master mode
        Port: MII
        PHYAD: 2
        Transceiver: external
        Supports Wake-on: d
        Wake-on: d
        Link detected: yes
-------------------------------------------------------------------------------




On Wed, Apr 15, 2020 at 02:12:09PM +0200, Oleksij Rempel wrote:
> This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
> auto-negotiation support, we needed to be able to configure the
> MASTER-SLAVE role of the port manually or from an application in user
> space.
> 
> The same UAPI can be used for 1000BASE-T or MultiGBASE-T devices to
> force MASTER or SLAVE role. See IEEE 802.3-2018:
> 22.2.4.3.7 MASTER-SLAVE control register (Register 9)
> 22.2.4.3.8 MASTER-SLAVE status register (Register 10)
> 40.5.2 MASTER-SLAVE configuration resolution
> 45.2.1.185.1 MASTER-SLAVE config value (1.2100.14)
> 45.2.7.10 MultiGBASE-T AN control 1 register (Register 7.32)
> 
> The MASTER-SLAVE role affects the clock configuration:
> 
> -------------------------------------------------------------------------------
> When the  PHY is configured as MASTER, the PMA Transmit function shall
> source TX_TCLK from a local clock source. When configured as SLAVE, the
> PMA Transmit function shall source TX_TCLK from the clock recovered from
> data stream provided by MASTER.
> 
> iMX6Q                     KSZ9031                XXX
> ------\                /-----------\        /------------\
>       |                |           |        |            |
>  MAC  |<----RGMII----->| PHY Slave |<------>| PHY Master |
>       |<--- 125 MHz ---+-<------/  |        | \          |
> ------/                \-----------/        \------------/
>                                                ^
>                                                 \-TX_TCLK
> 
> -------------------------------------------------------------------------------
> 
> Since some clock or link related issues are only reproducible in a
> specific MASTER-SLAVE-role, MAC and PHY configuration, it is beneficial
> to provide generic (not 100BASE-T1 specific) interface to the user space
> for configuration flexibility and trouble shooting.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  Documentation/networking/ethtool-netlink.rst |  2 +
>  drivers/net/phy/phy.c                        |  3 +-
>  drivers/net/phy/phy_device.c                 | 75 ++++++++++++++++++++
>  include/linux/phy.h                          |  1 +
>  include/uapi/linux/ethtool.h                 | 11 ++-
>  include/uapi/linux/ethtool_netlink.h         |  1 +
>  include/uapi/linux/mii.h                     |  2 +
>  net/ethtool/linkmodes.c                      |  9 ++-
>  8 files changed, 101 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index f1f868479ceb8..83127a6e42b17 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -368,6 +368,7 @@ Kernel response contents:
>    ``ETHTOOL_A_LINKMODES_PEER``          bitset  partner link modes
>    ``ETHTOOL_A_LINKMODES_SPEED``         u32     link speed (Mb/s)
>    ``ETHTOOL_A_LINKMODES_DUPLEX``        u8      duplex mode
> +  ``ETHTOOL_A_LINKMODES_MASTER_SLAVE``  u8      Master/slave port mode
>    ====================================  ======  ==========================
>  
>  For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
> @@ -390,6 +391,7 @@ Request contents:
>    ``ETHTOOL_A_LINKMODES_PEER``          bitset  partner link modes
>    ``ETHTOOL_A_LINKMODES_SPEED``         u32     link speed (Mb/s)
>    ``ETHTOOL_A_LINKMODES_DUPLEX``        u8      duplex mode
> +  ``ETHTOOL_A_LINKMODES_MASTER_SLAVE``  u8      Master/slave port mode
>    ====================================  ======  ==========================
>  
>  ``ETHTOOL_A_LINKMODES_OURS`` bit set allows setting advertised link modes. If
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d76e038cf2cb5..9f48141f1e701 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -294,7 +294,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  			 phydev->advertising, autoneg == AUTONEG_ENABLE);
>  
>  	phydev->duplex = duplex;
> -
> +	phydev->master_slave = cmd->base.master_slave;
>  	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>  
>  	/* Restart the PHY */
> @@ -313,6 +313,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  
>  	cmd->base.speed = phydev->speed;
>  	cmd->base.duplex = phydev->duplex;
> +	cmd->base.master_slave = phydev->master_slave;
>  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>  		cmd->base.port = PORT_BNC;
>  	else
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c8b0c34030d32..d5edf2bc40e43 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -604,6 +604,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
>  	dev->asym_pause = 0;
>  	dev->link = 0;
>  	dev->interface = PHY_INTERFACE_MODE_GMII;
> +	dev->master_slave = PORT_MODE_UNKNOWN;
>  
>  	dev->autoneg = AUTONEG_ENABLE;
>  
> @@ -1772,6 +1773,68 @@ int genphy_setup_forced(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(genphy_setup_forced);
>  
> +static int genphy_setup_master_slave(struct phy_device *phydev)
> +{
> +	u16 ctl = 0;
> +
> +	if (!phydev->is_gigabit_capable)
> +		return 0;
> +
> +	switch (phydev->master_slave) {
> +	case PORT_MODE_MASTER:
> +		ctl |= CTL1000_PREFER_MASTER;
> +		/* fallthrough */
> +	case PORT_MODE_SLAVE:
> +		/* CTL1000_ENABLE_MASTER is zero */
> +		break;
> +	case PORT_MODE_MASTER_FORCE:
> +		ctl |= CTL1000_AS_MASTER;
> +		/* fallthrough */
> +	case PORT_MODE_SLAVE_FORCE:
> +		ctl |= CTL1000_ENABLE_MASTER;
> +		break;
> +	case PORT_MODE_UNKNOWN:
> +		return 0;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return 0;
> +	}
> +
> +	return phy_modify_changed(phydev, MII_CTRL1000,
> +				  (CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER |
> +				   CTL1000_PREFER_MASTER), ctl);
> +}
> +
> +static int genphy_read_master_slave(struct phy_device *phydev)
> +{
> +	u16 ctl, stat;
> +
> +	if (!phydev->is_gigabit_capable)
> +		return 0;
> +
> +	ctl = phy_read(phydev, MII_CTRL1000);
> +	if (ctl < 0)
> +		return ctl;
> +
> +	stat = phy_read(phydev, MII_STAT1000);
> +	if (stat < 0)
> +		return stat;
> +
> +	if (ctl & CTL1000_ENABLE_MASTER) {
> +		if (stat & LPA_1000MSRES)
> +			phydev->master_slave = PORT_MODE_MASTER_FORCE;
> +		else
> +			phydev->master_slave = PORT_MODE_SLAVE_FORCE;
> +	} else {
> +		if (stat & LPA_1000MSRES)
> +			phydev->master_slave = PORT_MODE_MASTER;
> +		else
> +			phydev->master_slave = PORT_MODE_SLAVE;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * genphy_restart_aneg - Enable and Restart Autonegotiation
>   * @phydev: target phy_device struct
> @@ -1830,6 +1893,12 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed)
>  	if (genphy_config_eee_advert(phydev))
>  		changed = true;
>  
> +	err = genphy_setup_master_slave(phydev);
> +	if (err < 0)
> +		return err;
> +	else if (err)
> +		changed = true;
> +
>  	if (AUTONEG_ENABLE != phydev->autoneg)
>  		return genphy_setup_forced(phydev);
>  
> @@ -2062,6 +2131,11 @@ int genphy_read_status(struct phy_device *phydev)
>  	phydev->duplex = DUPLEX_UNKNOWN;
>  	phydev->pause = 0;
>  	phydev->asym_pause = 0;
> +	phydev->master_slave = PORT_MODE_UNKNOWN;
> +
> +	err = genphy_read_master_slave(phydev);
> +	if (err < 0)
> +		return err;
>  
>  	err = genphy_read_lpa(phydev);
>  	if (err < 0)
> @@ -2103,6 +2177,7 @@ int genphy_c37_read_status(struct phy_device *phydev)
>  	phydev->duplex = DUPLEX_UNKNOWN;
>  	phydev->pause = 0;
>  	phydev->asym_pause = 0;
> +	phydev->master_slave = PORT_MODE_UNKNOWN;
>  
>  	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>  		lpa = phy_read(phydev, MII_LPA);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index c570e162e05e5..de5f934223069 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -421,6 +421,7 @@ struct phy_device {
>  	int duplex;
>  	int pause;
>  	int asym_pause;
> +	int master_slave;
>  
>  	/* Union of PHY and Attached devices' supported link modes */
>  	/* See ethtool.h for more info */
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 2405ab2263779..bcbc44003c823 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1659,6 +1659,13 @@ static inline int ethtool_validate_duplex(__u8 duplex)
>  	return 0;
>  }
>  
> +/* Port mode */
> +#define PORT_MODE_MASTER	0x00
> +#define PORT_MODE_SLAVE		0x01
> +#define PORT_MODE_MASTER_FORCE	0x02
> +#define PORT_MODE_SLAVE_FORCE	0x03
> +#define PORT_MODE_UNKNOWN	0xff
> +
>  /* Which connector port. */
>  #define PORT_TP			0x00
>  #define PORT_AUI		0x01
> @@ -1850,6 +1857,7 @@ enum ethtool_reset_flags {
>   *	autonegotiation; 0 if unknown or not applicable.  Read-only.
>   * @transceiver: Used to distinguish different possible PHY types,
>   *	reported consistently by PHYLIB.  Read-only.
> + * @master_slave: Master or slave port mode.
>   *
>   * If autonegotiation is disabled, the speed and @duplex represent the
>   * fixed link mode and are writable if the driver supports multiple
> @@ -1897,7 +1905,8 @@ struct ethtool_link_settings {
>  	__u8	eth_tp_mdix_ctrl;
>  	__s8	link_mode_masks_nwords;
>  	__u8	transceiver;
> -	__u8	reserved1[3];
> +	__u8	master_slave;
> +	__u8	reserved1[2];
>  	__u32	reserved[7];
>  	__u32	link_mode_masks[0];
>  	/* layout of link_mode_masks fields:
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 7e0b460f872c0..e04d47cb5f227 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -185,6 +185,7 @@ enum {
>  	ETHTOOL_A_LINKMODES_PEER,		/* bitset */
>  	ETHTOOL_A_LINKMODES_SPEED,		/* u32 */
>  	ETHTOOL_A_LINKMODES_DUPLEX,		/* u8 */
> +	ETHTOOL_A_LINKMODES_MASTER_SLAVE,	/* u8 */
>  
>  	/* add new constants above here */
>  	__ETHTOOL_A_LINKMODES_CNT,
> diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
> index 0b9c3beda345b..78eac603a84fb 100644
> --- a/include/uapi/linux/mii.h
> +++ b/include/uapi/linux/mii.h
> @@ -146,11 +146,13 @@
>  /* 1000BASE-T Control register */
>  #define ADVERTISE_1000FULL	0x0200  /* Advertise 1000BASE-T full duplex */
>  #define ADVERTISE_1000HALF	0x0100  /* Advertise 1000BASE-T half duplex */
> +#define CTL1000_PREFER_MASTER	0x0400  /* prefer to operate as master */
>  #define CTL1000_AS_MASTER	0x0800
>  #define CTL1000_ENABLE_MASTER	0x1000
>  
>  /* 1000BASE-T Status register */
>  #define LPA_1000MSFAIL		0x8000	/* Master/Slave resolution failure */
> +#define LPA_1000MSRES		0x4000	/* Master/Slave resolution status */
>  #define LPA_1000LOCALRXOK	0x2000	/* Link partner local receiver status */
>  #define LPA_1000REMRXOK		0x1000	/* Link partner remote receiver status */
>  #define LPA_1000FULL		0x0800	/* Link partner 1000BASE-T full duplex */
> diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
> index 96f20be64553e..dc15b88e64c6a 100644
> --- a/net/ethtool/linkmodes.c
> +++ b/net/ethtool/linkmodes.c
> @@ -27,6 +27,7 @@ linkmodes_get_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
>  	[ETHTOOL_A_LINKMODES_PEER]		= { .type = NLA_REJECT },
>  	[ETHTOOL_A_LINKMODES_SPEED]		= { .type = NLA_REJECT },
>  	[ETHTOOL_A_LINKMODES_DUPLEX]		= { .type = NLA_REJECT },
> +	[ETHTOOL_A_LINKMODES_MASTER_SLAVE]	= { .type = NLA_REJECT },
>  };
>  
>  static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
> @@ -69,6 +70,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
>  	len = nla_total_size(sizeof(u8)) /* LINKMODES_AUTONEG */
>  		+ nla_total_size(sizeof(u32)) /* LINKMODES_SPEED */
>  		+ nla_total_size(sizeof(u8)) /* LINKMODES_DUPLEX */
> +		+ nla_total_size(sizeof(u8)) /* LINKMODES_MASTER_SLAVE */
>  		+ 0;
>  	ret = ethnl_bitset_size(ksettings->link_modes.advertising,
>  				ksettings->link_modes.supported,
> @@ -119,7 +121,9 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
>  	}
>  
>  	if (nla_put_u32(skb, ETHTOOL_A_LINKMODES_SPEED, lsettings->speed) ||
> -	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
> +	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex) ||
> +	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE,
> +		       lsettings->master_slave))
>  		return -EMSGSIZE;
>  
>  	return 0;
> @@ -248,6 +252,7 @@ linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
>  	[ETHTOOL_A_LINKMODES_PEER]		= { .type = NLA_REJECT },
>  	[ETHTOOL_A_LINKMODES_SPEED]		= { .type = NLA_U32 },
>  	[ETHTOOL_A_LINKMODES_DUPLEX]		= { .type = NLA_U8 },
> +	[ETHTOOL_A_LINKMODES_MASTER_SLAVE]	= { .type = NLA_U8 },
>  };
>  
>  /* Set advertised link modes to all supported modes matching requested speed
> @@ -310,6 +315,8 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
>  			 mod);
>  	ethnl_update_u8(&lsettings->duplex, tb[ETHTOOL_A_LINKMODES_DUPLEX],
>  			mod);
> +	ethnl_update_u8(&lsettings->master_slave,
> +			tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE], mod);
>  
>  	if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
>  	    (req_speed || req_duplex) &&
> -- 
> 2.26.0.rc2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Andrew Lunn]

On Wed, Apr 15, 2020 at 02:12:09PM +0200, Oleksij Rempel wrote:
> This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
> auto-negotiation support, we needed to be able to configure the
> MASTER-SLAVE role of the port manually or from an application in user
> space.

Hi Oleksij

This is a nice way to do this.

> +/* Port mode */
> +#define PORT_MODE_MASTER	0x00
> +#define PORT_MODE_SLAVE		0x01
> +#define PORT_MODE_MASTER_FORCE	0x02
> +#define PORT_MODE_SLAVE_FORCE	0x03
> +#define PORT_MODE_UNKNOWN	0xff

It is not clear to me what PORT_MODE_MASTER and PORT_MODE_SLAVE. Do
these mean to negotiate master/slave? Maybe some comments, or clearer
names?

	Andrew

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Oleksij Rempel]

On Wed, Apr 15, 2020 at 03:11:04PM +0200, Andrew Lunn wrote:
> On Wed, Apr 15, 2020 at 02:12:09PM +0200, Oleksij Rempel wrote:
> > This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
> > auto-negotiation support, we needed to be able to configure the
> > MASTER-SLAVE role of the port manually or from an application in user
> > space.
> 
> Hi Oleksij
> 
> This is a nice way to do this.
> 
> > +/* Port mode */
> > +#define PORT_MODE_MASTER	0x00
> > +#define PORT_MODE_SLAVE		0x01
> > +#define PORT_MODE_MASTER_FORCE	0x02
> > +#define PORT_MODE_SLAVE_FORCE	0x03
> > +#define PORT_MODE_UNKNOWN	0xff
> 
> It is not clear to me what PORT_MODE_MASTER and PORT_MODE_SLAVE. Do
> these mean to negotiate master/slave? Maybe some comments, or clearer
> names?

In the IEEE 802.3 it is described as:
---------------------------------------------------------------------------
Port type: Bit 9.10 is to be used to indicate the preference to operate
as MASTER (multiport device) or as SLAVE (single-port device) if the
MASTER-SLAVE Manual Configuration Enable bit, 9.12, is not set.  Usage
of this bit is described in 40.5.2.
1 = Multiport device
0 = single-port device
---------------------------------------------------------------------------

Setting PORT_MODE_MASTER/PORT_MODE_SLAVE will increase the chance to get
MASTER or SLAVE mode, but not force it.

If we will follow strictly to the IEEE 802.3 spec, it should be named:

#define PORT_MODE_UNKNOWN	0x00
/* this two options will not force some specific mode, only influence
 * the chance to get it */
#define PORT_TYPE_MULTI_PORT	0x01
#define PORT_TYPE_SINGLE_PORT	0x02
/* this two options will force master or slave mode */
#define PORT_MODE_MASTER	0x03
#define PORT_MODE_SLAVE		0x04

Please tell, if you have better ideas.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Andrew Lunn]

> In the IEEE 802.3 it is described as:
> ---------------------------------------------------------------------------
> Port type: Bit 9.10 is to be used to indicate the preference to operate
> as MASTER (multiport device) or as SLAVE (single-port device) if the
> MASTER-SLAVE Manual Configuration Enable bit, 9.12, is not set.  Usage
> of this bit is described in 40.5.2.
> 1 = Multiport device
> 0 = single-port device

I really should go read the standard, but...

> ---------------------------------------------------------------------------
> 
> Setting PORT_MODE_MASTER/PORT_MODE_SLAVE will increase the chance to get
> MASTER or SLAVE mode, but not force it.
> 
> If we will follow strictly to the IEEE 802.3 spec, it should be named:
> 
> #define PORT_MODE_UNKNOWN	0x00
> /* this two options will not force some specific mode, only influence
>  * the chance to get it */
> #define PORT_TYPE_MULTI_PORT	0x01
> #define PORT_TYPE_SINGLE_PORT	0x02
> /* this two options will force master or slave mode */
> #define PORT_MODE_MASTER	0x03
> #define PORT_MODE_SLAVE		0x04

I prefer having FORCE in the name.

But let me read the standard and get up to speed.

    Andrew

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Russell King - ARM Linux admin]

On Wed, Apr 15, 2020 at 03:00:34PM +0200, Oleksij Rempel wrote:
> On Wed, Apr 15, 2020 at 02:43:43PM +0200, Michal Kubecek wrote:
> > On Wed, Apr 15, 2020 at 02:19:40PM +0200, Oleksij Rempel wrote:
> > > Cc: Marek Vasut <marex@denx.de>
> > > 
> > > On Wed, Apr 15, 2020 at 02:12:09PM +0200, Oleksij Rempel wrote:
> > > > This UAPI is needed for BroadR-Reach 100BASE-T1 devices. Due to lack of
> > > > auto-negotiation support, we needed to be able to configure the
> > > > MASTER-SLAVE role of the port manually or from an application in user
> > > > space.
> > > > 
> > > > The same UAPI can be used for 1000BASE-T or MultiGBASE-T devices to
> > > > force MASTER or SLAVE role. See IEEE 802.3-2018:
> > > > 22.2.4.3.7 MASTER-SLAVE control register (Register 9)
> > > > 22.2.4.3.8 MASTER-SLAVE status register (Register 10)
> > > > 40.5.2 MASTER-SLAVE configuration resolution
> > > > 45.2.1.185.1 MASTER-SLAVE config value (1.2100.14)
> > > > 45.2.7.10 MultiGBASE-T AN control 1 register (Register 7.32)
> > > > 
> > > > The MASTER-SLAVE role affects the clock configuration:
> > > > 
> > > > -------------------------------------------------------------------------------
> > > > When the  PHY is configured as MASTER, the PMA Transmit function shall
> > > > source TX_TCLK from a local clock source. When configured as SLAVE, the
> > > > PMA Transmit function shall source TX_TCLK from the clock recovered from
> > > > data stream provided by MASTER.
> > > > 
> > > > iMX6Q                     KSZ9031                XXX
> > > > ------\                /-----------\        /------------\
> > > >       |                |           |        |            |
> > > >  MAC  |<----RGMII----->| PHY Slave |<------>| PHY Master |
> > > >       |<--- 125 MHz ---+-<------/  |        | \          |
> > > > ------/                \-----------/        \------------/
> > > >                                                ^
> > > >                                                 \-TX_TCLK
> > > > 
> > > > -------------------------------------------------------------------------------
> > > > 
> > > > Since some clock or link related issues are only reproducible in a
> > > > specific MASTER-SLAVE-role, MAC and PHY configuration, it is beneficial
> > > > to provide generic (not 100BASE-T1 specific) interface to the user space
> > > > for configuration flexibility and trouble shooting.
> > > > 
> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > > ---
> > [...]
> > > > +/* Port mode */
> > > > +#define PORT_MODE_MASTER	0x00
> > > > +#define PORT_MODE_SLAVE		0x01
> > > > +#define PORT_MODE_MASTER_FORCE	0x02
> > > > +#define PORT_MODE_SLAVE_FORCE	0x03
> > > > +#define PORT_MODE_UNKNOWN	0xff
> > 
> > Shouldn't 0 rather be something like PORT_MODE_UNSUPPORTED or
> > PORT_MODE_NONE? If I see correctly, all drivers not setting the new
> > field (which would be all drivers right now and almost all later) will
> > leave the value at 0 which would be interpreted as PORT_MODE_MASTER.
> 
> Yes, you right. I was thinking about it and decided to follow the duplex
> code pattern. Will fix in the next version.

Wouldn't that make PORT_MODE_UNKNOWN unnecessary?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Andrew Lunn]

>  ``ETHTOOL_A_LINKMODES_OURS`` bit set allows setting advertised link modes. If
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d76e038cf2cb5..9f48141f1e701 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -294,7 +294,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  			 phydev->advertising, autoneg == AUTONEG_ENABLE);
>  
>  	phydev->duplex = duplex;
> -
> +	phydev->master_slave = cmd->base.master_slave;
>  	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>  
>  	/* Restart the PHY */
> @@ -313,6 +313,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  
>  	cmd->base.speed = phydev->speed;
>  	cmd->base.duplex = phydev->duplex;
> +	cmd->base.master_slave = phydev->master_slave;
>  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>  		cmd->base.port = PORT_BNC;
>  	else
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index c8b0c34030d32..d5edf2bc40e43 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -604,6 +604,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
>  	dev->asym_pause = 0;
>  	dev->link = 0;
>  	dev->interface = PHY_INTERFACE_MODE_GMII;
> +	dev->master_slave = PORT_MODE_UNKNOWN;

phydev->master_slave is how we want the PHY to be configured. I don't
think PORT_MODE_UNKNOWN makes any sense in that contest. 802.3 gives
some defaults. 9.12 should be 0, meaning manual master/slave
configuration is disabled. The majority of linux devices are end
systems. So we should default to a single point device. So i would
initialise PORT_MODE_SLAVE, or whatever we end up calling that.
>  
>  	dev->autoneg = AUTONEG_ENABLE;
>  
> @@ -1772,6 +1773,68 @@ int genphy_setup_forced(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(genphy_setup_forced);
>  
> +static int genphy_setup_master_slave(struct phy_device *phydev)
> +{
> +	u16 ctl = 0;
> +
> +	if (!phydev->is_gigabit_capable)
> +		return 0;
> +
> +	switch (phydev->master_slave) {
> +	case PORT_MODE_MASTER:
> +		ctl |= CTL1000_PREFER_MASTER;
> +		/* fallthrough */
> +	case PORT_MODE_SLAVE:
> +		/* CTL1000_ENABLE_MASTER is zero */
> +		break;
> +	case PORT_MODE_MASTER_FORCE:
> +		ctl |= CTL1000_AS_MASTER;
> +		/* fallthrough */
> +	case PORT_MODE_SLAVE_FORCE:
> +		ctl |= CTL1000_ENABLE_MASTER;
> +		break;
> +	case PORT_MODE_UNKNOWN:
> +		return 0;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return 0;
> +	}
> +
> +	return phy_modify_changed(phydev, MII_CTRL1000,
> +				  (CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER |
> +				   CTL1000_PREFER_MASTER), ctl);
> +}
> +
> +static int genphy_read_master_slave(struct phy_device *phydev)
> +{
> +	u16 ctl, stat;
> +
> +	if (!phydev->is_gigabit_capable)
> +		return 0;
> +
> +	ctl = phy_read(phydev, MII_CTRL1000);
> +	if (ctl < 0)
> +		return ctl;
> +
> +	stat = phy_read(phydev, MII_STAT1000);
> +	if (stat < 0)
> +		return stat;
> +
> +	if (ctl & CTL1000_ENABLE_MASTER) {
> +		if (stat & LPA_1000MSRES)
> +			phydev->master_slave = PORT_MODE_MASTER_FORCE;
> +		else
> +			phydev->master_slave = PORT_MODE_SLAVE_FORCE;
> +	} else {
> +		if (stat & LPA_1000MSRES)
> +			phydev->master_slave = PORT_MODE_MASTER;
> +		else
> +			phydev->master_slave = PORT_MODE_SLAVE;
> +	}

This seems wrong. phydev->master_slave should be about how we want the
PHY to be configured. genphy_read_master_slave() should be about what
we actually ended up using. It should not be over-writing
phydev->master_slave, it needs to put it into some other variable.

phy_ethtool_ksettings_set() should allow us to set how we want the
device to behave. phy_ethtool_ksettings_get() should return both how
we have configured it, and if master/slave has been resolved, what the
result of the resolution is. Here something like PORT_MODE_UNKNOWN
does make sense, when the link is down, to resolution has not yet
completed.

       Andrew

RE: [EXT] Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Christian Herber]

Hi Oleksij,

>If we will follow strictly to the IEEE 802.3 spec, it should be named:
>
>#define PORT_MODE_UNKNOWN       0x00
>/* this two options will not force some specific mode, only influence
> * the chance to get it */
>#define PORT_TYPE_MULTI_PORT    0x01
>#define PORT_TYPE_SINGLE_PORT   0x02
>/* this two options will force master or slave mode */
>#define PORT_MODE_MASTER        0x03
>#define PORT_MODE_SLAVE         0x04
>
>Please tell, if you have better ideas.

This would be quite in the spirit of 802.3. My assumption is multiport devices preferably operate as master to reduce the amount of clock domain crossing on the multiport device. Of course, it is a bit use case driven and you could configure a preference for master mode also on a single port device. For such use cases the name is confusing. Here,

>#define PORT_MODE_MASTER_PREFERRED   0x01
>#define PORT_MODE_SLAVE_PREFERRED       0x02

might be better.

Christian

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Russell King - ARM Linux admin]

On Wed, Apr 15, 2020 at 11:57:39PM +0200, Andrew Lunn wrote:
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index c8b0c34030d32..d5edf2bc40e43 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -604,6 +604,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
> >  	dev->asym_pause = 0;
> >  	dev->link = 0;
> >  	dev->interface = PHY_INTERFACE_MODE_GMII;
> > +	dev->master_slave = PORT_MODE_UNKNOWN;
> 
> phydev->master_slave is how we want the PHY to be configured. I don't
> think PORT_MODE_UNKNOWN makes any sense in that contest. 802.3 gives
> some defaults. 9.12 should be 0, meaning manual master/slave
> configuration is disabled. The majority of linux devices are end
> systems. So we should default to a single point device. So i would
> initialise PORT_MODE_SLAVE, or whatever we end up calling that.

I'm not sure that is a good idea given that we use phylib to drive
the built-in PHYs in DSA switches, which ought to prefer master mode
via the "is a multiport device" bit.

Just to be clear, there are three bits that configure 1G PHYs, which
I've framed in briefer terminology:

- 9.12: auto/manual configuration (1= manual 0= slave)
- 9.11: manual master/slave configuration (1= master, 0 = slave)
- 9.10: auto master/slave preference (1= multiport / master)

It is recommended that multiport devices (such as DSA switches) set
9.10 so they prefer to be master.

It's likely that the reason is to reduce cross-talk interference
between neighbouring ports both inside the PHY, magnetics and the
board itself. I would suspect that this becomes critical when
operating at towards the maximum cable length.

I've checked some of my DSA switches, and 9.10 appears to default to
one, as expected given what's in the specs.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]

On Fri, Apr 17, 2020 at 11:11:45AM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Apr 15, 2020 at 11:57:39PM +0200, Andrew Lunn wrote:
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index c8b0c34030d32..d5edf2bc40e43 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -604,6 +604,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
> > >  	dev->asym_pause = 0;
> > >  	dev->link = 0;
> > >  	dev->interface = PHY_INTERFACE_MODE_GMII;
> > > +	dev->master_slave = PORT_MODE_UNKNOWN;
> > 
> > phydev->master_slave is how we want the PHY to be configured. I don't
> > think PORT_MODE_UNKNOWN makes any sense in that contest. 802.3 gives
> > some defaults. 9.12 should be 0, meaning manual master/slave
> > configuration is disabled. The majority of linux devices are end
> > systems. So we should default to a single point device. So i would
> > initialise PORT_MODE_SLAVE, or whatever we end up calling that.
> 
> I'm not sure that is a good idea given that we use phylib to drive
> the built-in PHYs in DSA switches, which ought to prefer master mode
> via the "is a multiport device" bit.
> 
> Just to be clear, there are three bits that configure 1G PHYs, which
> I've framed in briefer terminology:
> 
> - 9.12: auto/manual configuration (1= manual 0= slave)
> - 9.11: manual master/slave configuration (1= master, 0 = slave)
> - 9.10: auto master/slave preference (1= multiport / master)
> 
> It is recommended that multiport devices (such as DSA switches) set
> 9.10 so they prefer to be master.
> 
> It's likely that the reason is to reduce cross-talk interference
> between neighbouring ports both inside the PHY, magnetics and the
> board itself. I would suspect that this becomes critical when
> operating at towards the maximum cable length.
> 
> I've checked some of my DSA switches, and 9.10 appears to default to
> one, as expected given what's in the specs.

Hm..
I've checked one of my DSA devices and 9.10 is by default 0 (proffered
slave). It get slave even if it is preferred master and it is
connected to a workstation (not multiport device) with a e1000e NIC.
The e1000e is configured by default as preferred master.

Grepping over current linux kernel I see following attempts to
configure master/slave modes:
drivers/net/ethernet/intel/e1000e/phy.c:597
  e1000_set_master_slave_mode()

all intel NICs have similar code code and do not touch preferred bit
9.10. Only force master/slave modes. So the preferred master is probably
PHY defaults, bootstrap or eeprom.

drivers/net/ethernet/broadcom/tg3.c
this driver seems to always force master mode

drivers/net/phy/broadcom.c:39
if ethernet controller is BCMA_CHIP_ID_BCM53573 and the PHY is PHY_ID_BCM54210E
then force master mode.

drivers/net/phy/micrel.c:637
Force master mode if devicetree property is set: micrel,force-master

drivers/net/phy/realtek.c:173
	/* RTL8211C has an issue when operating in Gigabit slave mode * 
	return phy_set_bits(phydev, MII_CTRL1000,
		CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER)

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Russell King - ARM Linux admin]

On Fri, Apr 17, 2020 at 01:28:30PM +0200, Oleksij Rempel wrote:
> On Fri, Apr 17, 2020 at 11:11:45AM +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Apr 15, 2020 at 11:57:39PM +0200, Andrew Lunn wrote:
> > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > > index c8b0c34030d32..d5edf2bc40e43 100644
> > > > --- a/drivers/net/phy/phy_device.c
> > > > +++ b/drivers/net/phy/phy_device.c
> > > > @@ -604,6 +604,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
> > > >  	dev->asym_pause = 0;
> > > >  	dev->link = 0;
> > > >  	dev->interface = PHY_INTERFACE_MODE_GMII;
> > > > +	dev->master_slave = PORT_MODE_UNKNOWN;
> > > 
> > > phydev->master_slave is how we want the PHY to be configured. I don't
> > > think PORT_MODE_UNKNOWN makes any sense in that contest. 802.3 gives
> > > some defaults. 9.12 should be 0, meaning manual master/slave
> > > configuration is disabled. The majority of linux devices are end
> > > systems. So we should default to a single point device. So i would
> > > initialise PORT_MODE_SLAVE, or whatever we end up calling that.
> > 
> > I'm not sure that is a good idea given that we use phylib to drive
> > the built-in PHYs in DSA switches, which ought to prefer master mode
> > via the "is a multiport device" bit.
> > 
> > Just to be clear, there are three bits that configure 1G PHYs, which
> > I've framed in briefer terminology:
> > 
> > - 9.12: auto/manual configuration (1= manual 0= slave)
> > - 9.11: manual master/slave configuration (1= master, 0 = slave)
> > - 9.10: auto master/slave preference (1= multiport / master)
> > 
> > It is recommended that multiport devices (such as DSA switches) set
> > 9.10 so they prefer to be master.
> > 
> > It's likely that the reason is to reduce cross-talk interference
> > between neighbouring ports both inside the PHY, magnetics and the
> > board itself. I would suspect that this becomes critical when
> > operating at towards the maximum cable length.
> > 
> > I've checked some of my DSA switches, and 9.10 appears to default to
> > one, as expected given what's in the specs.
> 
> Hm..
> I've checked one of my DSA devices and 9.10 is by default 0 (proffered
> slave). It get slave even if it is preferred master and it is
> connected to a workstation (not multiport device) with a e1000e NIC.
> The e1000e is configured by default as preferred master.
> 
> Grepping over current linux kernel I see following attempts to
> configure master/slave modes:
> drivers/net/ethernet/intel/e1000e/phy.c:597
>   e1000_set_master_slave_mode()
> 
> all intel NICs have similar code code and do not touch preferred bit
> 9.10. Only force master/slave modes. So the preferred master is probably
> PHY defaults, bootstrap or eeprom.
> 
> drivers/net/ethernet/broadcom/tg3.c
> this driver seems to always force master mode
> 
> drivers/net/phy/broadcom.c:39
> if ethernet controller is BCMA_CHIP_ID_BCM53573 and the PHY is PHY_ID_BCM54210E
> then force master mode.
> 
> drivers/net/phy/micrel.c:637
> Force master mode if devicetree property is set: micrel,force-master
> 
> drivers/net/phy/realtek.c:173
> 	/* RTL8211C has an issue when operating in Gigabit slave mode * 
> 	return phy_set_bits(phydev, MII_CTRL1000,
> 		CTL1000_ENABLE_MASTER | CTL1000_AS_MASTER)

Short of working around hardware issues, I wonder whether some of the
above is due to not reading or understanding the 802.3 recommendation.
However, it is just a recommendation, not a requirement.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Andrew Lunn]

On Fri, Apr 17, 2020 at 11:11:45AM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Apr 15, 2020 at 11:57:39PM +0200, Andrew Lunn wrote:
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index c8b0c34030d32..d5edf2bc40e43 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -604,6 +604,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
> > >  	dev->asym_pause = 0;
> > >  	dev->link = 0;
> > >  	dev->interface = PHY_INTERFACE_MODE_GMII;
> > > +	dev->master_slave = PORT_MODE_UNKNOWN;
> > 
> > phydev->master_slave is how we want the PHY to be configured. I don't
> > think PORT_MODE_UNKNOWN makes any sense in that contest. 802.3 gives
> > some defaults. 9.12 should be 0, meaning manual master/slave
> > configuration is disabled. The majority of linux devices are end
> > systems. So we should default to a single point device. So i would
> > initialise PORT_MODE_SLAVE, or whatever we end up calling that.
> 
> I'm not sure that is a good idea given that we use phylib to drive
> the built-in PHYs in DSA switches, which ought to prefer master mode
> via the "is a multiport device" bit.

O.K. So i assume you mean we should read from the PHY at probe time
what it is doing, in order to initialise dev->master_slave?

I would be happy with that.

  Andrew

Re: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.

[Russell King - ARM Linux admin]

On Fri, Apr 17, 2020 at 04:32:39PM +0200, Andrew Lunn wrote:
> On Fri, Apr 17, 2020 at 11:11:45AM +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Apr 15, 2020 at 11:57:39PM +0200, Andrew Lunn wrote:
> > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > > index c8b0c34030d32..d5edf2bc40e43 100644
> > > > --- a/drivers/net/phy/phy_device.c
> > > > +++ b/drivers/net/phy/phy_device.c
> > > > @@ -604,6 +604,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
> > > >  	dev->asym_pause = 0;
> > > >  	dev->link = 0;
> > > >  	dev->interface = PHY_INTERFACE_MODE_GMII;
> > > > +	dev->master_slave = PORT_MODE_UNKNOWN;
> > > 
> > > phydev->master_slave is how we want the PHY to be configured. I don't
> > > think PORT_MODE_UNKNOWN makes any sense in that contest. 802.3 gives
> > > some defaults. 9.12 should be 0, meaning manual master/slave
> > > configuration is disabled. The majority of linux devices are end
> > > systems. So we should default to a single point device. So i would
> > > initialise PORT_MODE_SLAVE, or whatever we end up calling that.
> > 
> > I'm not sure that is a good idea given that we use phylib to drive
> > the built-in PHYs in DSA switches, which ought to prefer master mode
> > via the "is a multiport device" bit.
> 
> O.K. So i assume you mean we should read from the PHY at probe time
> what it is doing, in order to initialise dev->master_slave?
> 
> I would be happy with that.

Yes, I think it's a good idea to preserve the current operating mode
of the PHY as that's essentially what we're doing today by not
currently touching the bit.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up