[PATCH net-next v4 0/2] provide support for PHY master/slave configuration

[PATCH net-next v4 0/2] provide support for PHY master/slave configuration

[Oleksij Rempel]

changes v5:
- set MASTER_SLAVE_CFG_UNSUPPORTED as default value
- send a netlink error message on validation error
- more code fixes

changes v4:
- rename port_mode to master_slave 
- move validation code to net/ethtool/linkmodes.c 
- add UNSUPPORTED state and avoid sending unsupported fields
- more formatting and naming fixes
- tja11xx: support only force mode
- tja11xx: mark state as unsupported

changes v3:
- provide separate field for config and state.
- make state rejected on set
- add validation

changes v2:
- change names. Use MASTER_PREFERRED instead of MULTIPORT
- configure master/slave only on request. Default configuration can be
  provided by PHY or eeprom
- status and configuration to the user space.

Oleksij Rempel (2):
  ethtool: provide UAPI for PHY master/slave configuration.
  net: phy: tja11xx: add support for master-slave configuration

 Documentation/networking/ethtool-netlink.rst | 35 ++++----
 drivers/net/phy/nxp-tja11xx.c                | 43 +++++++++
 drivers/net/phy/phy.c                        |  4 +-
 drivers/net/phy/phy_device.c                 | 94 ++++++++++++++++++++
 include/linux/phy.h                          |  3 +
 include/uapi/linux/ethtool.h                 | 16 +++-
 include/uapi/linux/ethtool_netlink.h         |  2 +
 include/uapi/linux/mii.h                     |  2 +
 net/ethtool/linkmodes.c                      | 45 ++++++++++
 9 files changed, 226 insertions(+), 18 deletions(-)

-- 
2.26.2

[PATCH v5 1/2] 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 | 35 ++++----
 drivers/net/phy/phy.c                        |  4 +-
 drivers/net/phy/phy_device.c                 | 94 ++++++++++++++++++++
 include/linux/phy.h                          |  3 +
 include/uapi/linux/ethtool.h                 | 16 +++-
 include/uapi/linux/ethtool_netlink.h         |  2 +
 include/uapi/linux/mii.h                     |  2 +
 net/ethtool/linkmodes.c                      | 45 ++++++++++
 8 files changed, 183 insertions(+), 18 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 567326491f80b..8f5cefc539cf1 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -392,14 +392,16 @@ Request contents:
 
 Kernel response contents:
 
-  ====================================  ======  ==========================
-  ``ETHTOOL_A_LINKMODES_HEADER``        nested  reply header
-  ``ETHTOOL_A_LINKMODES_AUTONEG``       u8      autonegotiation status
-  ``ETHTOOL_A_LINKMODES_OURS``          bitset  advertised link modes
-  ``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_HEADER``              nested  reply header
+  ``ETHTOOL_A_LINKMODES_AUTONEG``             u8      autonegotiation status
+  ``ETHTOOL_A_LINKMODES_OURS``                bitset  advertised link modes
+  ``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_CFG``    u8      Master/slave port mode
+  ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE``  u8      Master/slave port state
+  ==========================================  ======  ==========================
 
 For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
 represents supported modes. ``ETHTOOL_A_LINKMODES_PEER`` in the reply is a bit
@@ -414,14 +416,15 @@ LINKMODES_SET
 
 Request contents:
 
-  ====================================  ======  ==========================
-  ``ETHTOOL_A_LINKMODES_HEADER``        nested  request header
-  ``ETHTOOL_A_LINKMODES_AUTONEG``       u8      autonegotiation status
-  ``ETHTOOL_A_LINKMODES_OURS``          bitset  advertised link modes
-  ``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_HEADER``              nested  request header
+  ``ETHTOOL_A_LINKMODES_AUTONEG``             u8      autonegotiation status
+  ``ETHTOOL_A_LINKMODES_OURS``                bitset  advertised link modes
+  ``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_CFG``    u8      Master/slave port mode
+  ==========================================  ======  ==========================
 
 ``ETHTOOL_A_LINKMODES_OURS`` bit set allows setting advertised link modes. If
 autonegotiation is on (either set now or kept from before), advertised modes
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 72c69a9c8a98a..8c22d02b4218e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -295,7 +295,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 			 phydev->advertising, autoneg == AUTONEG_ENABLE);
 
 	phydev->duplex = duplex;
-
+	phydev->master_slave_set = cmd->base.master_slave_cfg;
 	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
 
 	/* Restart the PHY */
@@ -314,6 +314,8 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
 
 	cmd->base.speed = phydev->speed;
 	cmd->base.duplex = phydev->duplex;
+	cmd->base.master_slave_cfg = phydev->master_slave_get;
+	cmd->base.master_slave_state = phydev->master_slave_state;
 	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 ac2784192472f..42dda9d2082ee 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1768,6 +1768,90 @@ 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_set) {
+	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+		ctl |= CTL1000_PREFER_MASTER;
+		break;
+	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+		break;
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+		ctl |= CTL1000_AS_MASTER;
+		/* fallthrough */
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+		ctl |= CTL1000_ENABLE_MASTER;
+		break;
+	case MASTER_SLAVE_CFG_UNKNOWN:
+	case MASTER_SLAVE_CFG_UNSUPPORTED:
+		return 0;
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		return -EOPNOTSUPP;
+	}
+
+	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)
+{
+	int cfg, state;
+	u16 val;
+
+	if (!phydev->is_gigabit_capable) {
+		phydev->master_slave_get = MASTER_SLAVE_CFG_UNSUPPORTED;
+		phydev->master_slave_state = MASTER_SLAVE_STATE_UNSUPPORTED;
+		return 0;
+	}
+
+	phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
+	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+
+	val = phy_read(phydev, MII_CTRL1000);
+	if (val < 0)
+		return val;
+
+	if (val & CTL1000_ENABLE_MASTER) {
+		if (val & CTL1000_AS_MASTER)
+			cfg = MASTER_SLAVE_CFG_MASTER_FORCE;
+		else
+			cfg = MASTER_SLAVE_CFG_SLAVE_FORCE;
+	} else {
+		if (val & CTL1000_PREFER_MASTER)
+			cfg = MASTER_SLAVE_CFG_MASTER_PREFERRED;
+		else
+			cfg = MASTER_SLAVE_CFG_SLAVE_PREFERRED;
+	}
+
+	val = phy_read(phydev, MII_STAT1000);
+	if (val < 0)
+		return val;
+
+	if (val & LPA_1000MSFAIL) {
+		state = MASTER_SLAVE_STATE_ERR;
+	} else if (phydev->link) {
+		/* this bits are valid only for active link */
+		if (val & LPA_1000MSRES)
+			state = MASTER_SLAVE_STATE_MASTER;
+		else
+			state = MASTER_SLAVE_STATE_SLAVE;
+	} else {
+		state = MASTER_SLAVE_STATE_UNKNOWN;
+	}
+
+	phydev->master_slave_get = cfg;
+	phydev->master_slave_state = state;
+
+	return 0;
+}
+
 /**
  * genphy_restart_aneg - Enable and Restart Autonegotiation
  * @phydev: target phy_device struct
@@ -1826,6 +1910,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);
 
@@ -2060,6 +2150,10 @@ int genphy_read_status(struct phy_device *phydev)
 	phydev->pause = 0;
 	phydev->asym_pause = 0;
 
+	err = genphy_read_master_slave(phydev);
+	if (err < 0)
+		return err;
+
 	err = genphy_read_lpa(phydev);
 	if (err < 0)
 		return err;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2432ca463ddc0..19cd4fe6efbf1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -431,6 +431,9 @@ struct phy_device {
 	int duplex;
 	int pause;
 	int asym_pause;
+	u8 master_slave_get;
+	u8 master_slave_set;
+	u8 master_slave_state;
 
 	/* 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 92f737f101178..f4662b3a9e1ef 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1666,6 +1666,18 @@ static inline int ethtool_validate_duplex(__u8 duplex)
 	return 0;
 }
 
+#define MASTER_SLAVE_CFG_UNSUPPORTED		0
+#define MASTER_SLAVE_CFG_UNKNOWN		1
+#define MASTER_SLAVE_CFG_MASTER_PREFERRED	2
+#define MASTER_SLAVE_CFG_SLAVE_PREFERRED	3
+#define MASTER_SLAVE_CFG_MASTER_FORCE		4
+#define MASTER_SLAVE_CFG_SLAVE_FORCE		5
+#define MASTER_SLAVE_STATE_UNSUPPORTED		0
+#define MASTER_SLAVE_STATE_UNKNOWN		1
+#define MASTER_SLAVE_STATE_MASTER		2
+#define MASTER_SLAVE_STATE_SLAVE		3
+#define MASTER_SLAVE_STATE_ERR			4
+
 /* Which connector port. */
 #define PORT_TP			0x00
 #define PORT_AUI		0x01
@@ -1904,7 +1916,9 @@ struct ethtool_link_settings {
 	__u8	eth_tp_mdix_ctrl;
 	__s8	link_mode_masks_nwords;
 	__u8	transceiver;
-	__u8	reserved1[3];
+	__u8	master_slave_cfg;
+	__u8	master_slave_state;
+	__u8	reserved1[1];
 	__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 7fde76366ba46..bf1d310e20bc6 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -216,6 +216,8 @@ enum {
 	ETHTOOL_A_LINKMODES_PEER,		/* bitset */
 	ETHTOOL_A_LINKMODES_SPEED,		/* u32 */
 	ETHTOOL_A_LINKMODES_DUPLEX,		/* u8 */
+	ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,	/* u8 */
+	ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,	/* 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 90f9b4e1ba277..39f7c44baf535 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -151,11 +151,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 452608c6d8562..1a6815e5698cf 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -27,6 +27,8 @@ 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_CFG]	= { .type = NLA_REJECT },
+	[ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE]	= { .type = NLA_REJECT },
 };
 
 static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
@@ -63,6 +65,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
 {
 	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
 	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
+	const struct ethtool_link_settings *lsettings = &ksettings->base;
 	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
 	int len, ret;
 
@@ -86,6 +89,12 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
 		len += ret;
 	}
 
+	if (lsettings->master_slave_cfg != MASTER_SLAVE_CFG_UNSUPPORTED)
+		len += nla_total_size(sizeof(u8));
+
+	if (lsettings->master_slave_state != MASTER_SLAVE_STATE_UNSUPPORTED)
+		len += nla_total_size(sizeof(u8));
+
 	return len;
 }
 
@@ -122,6 +131,16 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
 	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
 		return -EMSGSIZE;
 
+	if (lsettings->master_slave_cfg != MASTER_SLAVE_CFG_UNSUPPORTED &&
+	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,
+		       lsettings->master_slave_cfg))
+		return -EMSGSIZE;
+
+	if (lsettings->master_slave_state != MASTER_SLAVE_STATE_UNSUPPORTED &&
+	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,
+		       lsettings->master_slave_state))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
@@ -249,6 +268,8 @@ 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_CFG]	= { .type = NLA_U8 },
+	[ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE]	= { .type = NLA_REJECT },
 };
 
 /* Set advertised link modes to all supported modes matching requested speed
@@ -287,14 +308,37 @@ static bool ethnl_auto_linkmodes(struct ethtool_link_ksettings *ksettings,
 			     __ETHTOOL_LINK_MODE_MASK_NBITS);
 }
 
+static int ethnl_validate_master_slave_cfg(u8 cfg)
+{
+	switch (cfg) {
+	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+		return 1;
+	}
+
+	return 0;
+}
+
 static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
 				  struct ethtool_link_ksettings *ksettings,
 				  bool *mod)
 {
 	struct ethtool_link_settings *lsettings = &ksettings->base;
 	bool req_speed, req_duplex;
+	const struct nlattr *master_slave_cfg;
 	int ret;
 
+	master_slave_cfg = tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG];
+	if (master_slave_cfg) {
+		u8 cfg = nla_get_u8(master_slave_cfg);
+		if (!ethnl_validate_master_slave_cfg(cfg)) {
+			GENL_SET_ERR_MSG(info, "LINKMODES_MASTER_SLAVE_CFG contains not valid value");
+			return -EOPNOTSUPP;
+		}
+	}
+
 	*mod = false;
 	req_speed = tb[ETHTOOL_A_LINKMODES_SPEED];
 	req_duplex = tb[ETHTOOL_A_LINKMODES_DUPLEX];
@@ -311,6 +355,7 @@ 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_cfg, master_slave_cfg, mod);
 
 	if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
 	    (req_speed || req_duplex) &&
-- 
2.26.2

Re: [PATCH v5 1/2] ethtool: provide UAPI for PHY master/slave configuration.

[Oleksij Rempel]

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

@Michal,

i noticed that linkmodes_fill_reply() some times get not enough
tailroom.
if data->peer_empty == 0
linkmodes_reply_size() size: 476
linkmodes_fill_reply() skb tailroom: 724


if data->peer_empty == 1
linkmodes_reply_size() size: 216                                      
linkmodes_fill_reply() skb tailroom: 212

In the last case i won't be able to attach master_lave state and cfg
fields.

It looks like this issue was not introduced by my patches. May be you
have idea, what is missing?

On Mon, May 04, 2020 at 09:12:13AM +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 | 35 ++++----
>  drivers/net/phy/phy.c                        |  4 +-
>  drivers/net/phy/phy_device.c                 | 94 ++++++++++++++++++++
>  include/linux/phy.h                          |  3 +
>  include/uapi/linux/ethtool.h                 | 16 +++-
>  include/uapi/linux/ethtool_netlink.h         |  2 +
>  include/uapi/linux/mii.h                     |  2 +
>  net/ethtool/linkmodes.c                      | 45 ++++++++++
>  8 files changed, 183 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 567326491f80b..8f5cefc539cf1 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -392,14 +392,16 @@ Request contents:
>  
>  Kernel response contents:
>  
> -  ====================================  ======  ==========================
> -  ``ETHTOOL_A_LINKMODES_HEADER``        nested  reply header
> -  ``ETHTOOL_A_LINKMODES_AUTONEG``       u8      autonegotiation status
> -  ``ETHTOOL_A_LINKMODES_OURS``          bitset  advertised link modes
> -  ``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_HEADER``              nested  reply header
> +  ``ETHTOOL_A_LINKMODES_AUTONEG``             u8      autonegotiation status
> +  ``ETHTOOL_A_LINKMODES_OURS``                bitset  advertised link modes
> +  ``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_CFG``    u8      Master/slave port mode
> +  ``ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE``  u8      Master/slave port state
> +  ==========================================  ======  ==========================
>  
>  For ``ETHTOOL_A_LINKMODES_OURS``, value represents advertised modes and mask
>  represents supported modes. ``ETHTOOL_A_LINKMODES_PEER`` in the reply is a bit
> @@ -414,14 +416,15 @@ LINKMODES_SET
>  
>  Request contents:
>  
> -  ====================================  ======  ==========================
> -  ``ETHTOOL_A_LINKMODES_HEADER``        nested  request header
> -  ``ETHTOOL_A_LINKMODES_AUTONEG``       u8      autonegotiation status
> -  ``ETHTOOL_A_LINKMODES_OURS``          bitset  advertised link modes
> -  ``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_HEADER``              nested  request header
> +  ``ETHTOOL_A_LINKMODES_AUTONEG``             u8      autonegotiation status
> +  ``ETHTOOL_A_LINKMODES_OURS``                bitset  advertised link modes
> +  ``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_CFG``    u8      Master/slave port mode
> +  ==========================================  ======  ==========================
>  
>  ``ETHTOOL_A_LINKMODES_OURS`` bit set allows setting advertised link modes. If
>  autonegotiation is on (either set now or kept from before), advertised modes
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 72c69a9c8a98a..8c22d02b4218e 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -295,7 +295,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  			 phydev->advertising, autoneg == AUTONEG_ENABLE);
>  
>  	phydev->duplex = duplex;
> -
> +	phydev->master_slave_set = cmd->base.master_slave_cfg;
>  	phydev->mdix_ctrl = cmd->base.eth_tp_mdix_ctrl;
>  
>  	/* Restart the PHY */
> @@ -314,6 +314,8 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  
>  	cmd->base.speed = phydev->speed;
>  	cmd->base.duplex = phydev->duplex;
> +	cmd->base.master_slave_cfg = phydev->master_slave_get;
> +	cmd->base.master_slave_state = phydev->master_slave_state;
>  	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 ac2784192472f..42dda9d2082ee 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1768,6 +1768,90 @@ 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_set) {
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +		ctl |= CTL1000_PREFER_MASTER;
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +		break;
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +		ctl |= CTL1000_AS_MASTER;
> +		/* fallthrough */
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +		ctl |= CTL1000_ENABLE_MASTER;
> +		break;
> +	case MASTER_SLAVE_CFG_UNKNOWN:
> +	case MASTER_SLAVE_CFG_UNSUPPORTED:
> +		return 0;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	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)
> +{
> +	int cfg, state;
> +	u16 val;
> +
> +	if (!phydev->is_gigabit_capable) {
> +		phydev->master_slave_get = MASTER_SLAVE_CFG_UNSUPPORTED;
> +		phydev->master_slave_state = MASTER_SLAVE_STATE_UNSUPPORTED;
> +		return 0;
> +	}
> +
> +	phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
> +	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> +
> +	val = phy_read(phydev, MII_CTRL1000);
> +	if (val < 0)
> +		return val;
> +
> +	if (val & CTL1000_ENABLE_MASTER) {
> +		if (val & CTL1000_AS_MASTER)
> +			cfg = MASTER_SLAVE_CFG_MASTER_FORCE;
> +		else
> +			cfg = MASTER_SLAVE_CFG_SLAVE_FORCE;
> +	} else {
> +		if (val & CTL1000_PREFER_MASTER)
> +			cfg = MASTER_SLAVE_CFG_MASTER_PREFERRED;
> +		else
> +			cfg = MASTER_SLAVE_CFG_SLAVE_PREFERRED;
> +	}
> +
> +	val = phy_read(phydev, MII_STAT1000);
> +	if (val < 0)
> +		return val;
> +
> +	if (val & LPA_1000MSFAIL) {
> +		state = MASTER_SLAVE_STATE_ERR;
> +	} else if (phydev->link) {
> +		/* this bits are valid only for active link */
> +		if (val & LPA_1000MSRES)
> +			state = MASTER_SLAVE_STATE_MASTER;
> +		else
> +			state = MASTER_SLAVE_STATE_SLAVE;
> +	} else {
> +		state = MASTER_SLAVE_STATE_UNKNOWN;
> +	}
> +
> +	phydev->master_slave_get = cfg;
> +	phydev->master_slave_state = state;
> +
> +	return 0;
> +}
> +
>  /**
>   * genphy_restart_aneg - Enable and Restart Autonegotiation
>   * @phydev: target phy_device struct
> @@ -1826,6 +1910,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);
>  
> @@ -2060,6 +2150,10 @@ int genphy_read_status(struct phy_device *phydev)
>  	phydev->pause = 0;
>  	phydev->asym_pause = 0;
>  
> +	err = genphy_read_master_slave(phydev);
> +	if (err < 0)
> +		return err;
> +
>  	err = genphy_read_lpa(phydev);
>  	if (err < 0)
>  		return err;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 2432ca463ddc0..19cd4fe6efbf1 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -431,6 +431,9 @@ struct phy_device {
>  	int duplex;
>  	int pause;
>  	int asym_pause;
> +	u8 master_slave_get;
> +	u8 master_slave_set;
> +	u8 master_slave_state;
>  
>  	/* 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 92f737f101178..f4662b3a9e1ef 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1666,6 +1666,18 @@ static inline int ethtool_validate_duplex(__u8 duplex)
>  	return 0;
>  }
>  
> +#define MASTER_SLAVE_CFG_UNSUPPORTED		0
> +#define MASTER_SLAVE_CFG_UNKNOWN		1
> +#define MASTER_SLAVE_CFG_MASTER_PREFERRED	2
> +#define MASTER_SLAVE_CFG_SLAVE_PREFERRED	3
> +#define MASTER_SLAVE_CFG_MASTER_FORCE		4
> +#define MASTER_SLAVE_CFG_SLAVE_FORCE		5
> +#define MASTER_SLAVE_STATE_UNSUPPORTED		0
> +#define MASTER_SLAVE_STATE_UNKNOWN		1
> +#define MASTER_SLAVE_STATE_MASTER		2
> +#define MASTER_SLAVE_STATE_SLAVE		3
> +#define MASTER_SLAVE_STATE_ERR			4
> +
>  /* Which connector port. */
>  #define PORT_TP			0x00
>  #define PORT_AUI		0x01
> @@ -1904,7 +1916,9 @@ struct ethtool_link_settings {
>  	__u8	eth_tp_mdix_ctrl;
>  	__s8	link_mode_masks_nwords;
>  	__u8	transceiver;
> -	__u8	reserved1[3];
> +	__u8	master_slave_cfg;
> +	__u8	master_slave_state;
> +	__u8	reserved1[1];
>  	__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 7fde76366ba46..bf1d310e20bc6 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -216,6 +216,8 @@ enum {
>  	ETHTOOL_A_LINKMODES_PEER,		/* bitset */
>  	ETHTOOL_A_LINKMODES_SPEED,		/* u32 */
>  	ETHTOOL_A_LINKMODES_DUPLEX,		/* u8 */
> +	ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,	/* u8 */
> +	ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,	/* 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 90f9b4e1ba277..39f7c44baf535 100644
> --- a/include/uapi/linux/mii.h
> +++ b/include/uapi/linux/mii.h
> @@ -151,11 +151,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 452608c6d8562..1a6815e5698cf 100644
> --- a/net/ethtool/linkmodes.c
> +++ b/net/ethtool/linkmodes.c
> @@ -27,6 +27,8 @@ 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_CFG]	= { .type = NLA_REJECT },
> +	[ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE]	= { .type = NLA_REJECT },
>  };
>  
>  static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
> @@ -63,6 +65,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
>  {
>  	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>  	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
> +	const struct ethtool_link_settings *lsettings = &ksettings->base;
>  	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
>  	int len, ret;
>  
> @@ -86,6 +89,12 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
>  		len += ret;
>  	}
>  
> +	if (lsettings->master_slave_cfg != MASTER_SLAVE_CFG_UNSUPPORTED)
> +		len += nla_total_size(sizeof(u8));
> +
> +	if (lsettings->master_slave_state != MASTER_SLAVE_STATE_UNSUPPORTED)
> +		len += nla_total_size(sizeof(u8));
> +
>  	return len;
>  }
>  
> @@ -122,6 +131,16 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
>  	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_DUPLEX, lsettings->duplex))
>  		return -EMSGSIZE;
>  
> +	if (lsettings->master_slave_cfg != MASTER_SLAVE_CFG_UNSUPPORTED &&
> +	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG,
> +		       lsettings->master_slave_cfg))
> +		return -EMSGSIZE;
> +
> +	if (lsettings->master_slave_state != MASTER_SLAVE_STATE_UNSUPPORTED &&
> +	    nla_put_u8(skb, ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE,
> +		       lsettings->master_slave_state))
> +		return -EMSGSIZE;
> +
>  	return 0;
>  }
>  
> @@ -249,6 +268,8 @@ 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_CFG]	= { .type = NLA_U8 },
> +	[ETHTOOL_A_LINKMODES_MASTER_SLAVE_STATE]	= { .type = NLA_REJECT },
>  };
>  
>  /* Set advertised link modes to all supported modes matching requested speed
> @@ -287,14 +308,37 @@ static bool ethnl_auto_linkmodes(struct ethtool_link_ksettings *ksettings,
>  			     __ETHTOOL_LINK_MODE_MASK_NBITS);
>  }
>  
> +static int ethnl_validate_master_slave_cfg(u8 cfg)
> +{
> +	switch (cfg) {
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
>  				  struct ethtool_link_ksettings *ksettings,
>  				  bool *mod)
>  {
>  	struct ethtool_link_settings *lsettings = &ksettings->base;
>  	bool req_speed, req_duplex;
> +	const struct nlattr *master_slave_cfg;
>  	int ret;
>  
> +	master_slave_cfg = tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG];
> +	if (master_slave_cfg) {
> +		u8 cfg = nla_get_u8(master_slave_cfg);
> +		if (!ethnl_validate_master_slave_cfg(cfg)) {
> +			GENL_SET_ERR_MSG(info, "LINKMODES_MASTER_SLAVE_CFG contains not valid value");
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
>  	*mod = false;
>  	req_speed = tb[ETHTOOL_A_LINKMODES_SPEED];
>  	req_duplex = tb[ETHTOOL_A_LINKMODES_DUPLEX];
> @@ -311,6 +355,7 @@ 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_cfg, master_slave_cfg, mod);
>  
>  	if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
>  	    (req_speed || req_duplex) &&
> -- 
> 2.26.2
> 
> 

-- 
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 v5 1/2] ethtool: provide UAPI for PHY master/slave configuration.

[Michal Kubecek]

On Mon, May 04, 2020 at 10:04:17AM +0200, Oleksij Rempel wrote:
> @Michal,
> 
> i noticed that linkmodes_fill_reply() some times get not enough
> tailroom.
> if data->peer_empty == 0
> linkmodes_reply_size() size: 476
> linkmodes_fill_reply() skb tailroom: 724
> 
> 
> if data->peer_empty == 1
> linkmodes_reply_size() size: 216                                      
> linkmodes_fill_reply() skb tailroom: 212
> 
> In the last case i won't be able to attach master_lave state and cfg
> fields.
> 
> It looks like this issue was not introduced by my patches. May be you
> have idea, what is missing?

It's my mistake, I'm just not sure why I never ran into this while
testing. Please try the patch below.

Michal

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 0c772318c023..ed5357210193 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -342,7 +342,7 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
 	ret = ops->reply_size(req_info, reply_data);
 	if (ret < 0)
 		goto err_cleanup;
-	reply_len = ret;
+	reply_len = ret + ethnl_reply_header_size();
 	ret = -ENOMEM;
 	rskb = ethnl_reply_init(reply_len, req_info->dev, ops->reply_cmd,
 				ops->hdr_attr, info, &reply_payload);
@@ -588,7 +588,7 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
 	ret = ops->reply_size(req_info, reply_data);
 	if (ret < 0)
 		goto err_cleanup;
-	reply_len = ret;
+	reply_len = ret + ethnl_reply_header_size();
 	ret = -ENOMEM;
 	skb = genlmsg_new(reply_len, GFP_KERNEL);
 	if (!skb)
diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
index 95eae5c68a52..0eed4e4909ab 100644
--- a/net/ethtool/strset.c
+++ b/net/ethtool/strset.c
@@ -324,7 +324,6 @@ static int strset_reply_size(const struct ethnl_req_info *req_base,
 	int len = 0;
 	int ret;
 
-	len += ethnl_reply_header_size();
 	for (i = 0; i < ETH_SS_COUNT; i++) {
 		const struct strset_info *set_info = &data->sets[i];
 

Re: [PATCH v5 1/2] ethtool: provide UAPI for PHY master/slave configuration.

[Oleksij Rempel]

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

On Mon, May 04, 2020 at 10:37:34AM +0200, Michal Kubecek wrote:
> On Mon, May 04, 2020 at 10:04:17AM +0200, Oleksij Rempel wrote:
> > @Michal,
> > 
> > i noticed that linkmodes_fill_reply() some times get not enough
> > tailroom.
> > if data->peer_empty == 0
> > linkmodes_reply_size() size: 476
> > linkmodes_fill_reply() skb tailroom: 724
> > 
> > 
> > if data->peer_empty == 1
> > linkmodes_reply_size() size: 216                                      
> > linkmodes_fill_reply() skb tailroom: 212
> > 
> > In the last case i won't be able to attach master_lave state and cfg
> > fields.
> > 
> > It looks like this issue was not introduced by my patches. May be you
> > have idea, what is missing?
> 
> It's my mistake, I'm just not sure why I never ran into this while
> testing. Please try the patch below.

thx! it works now:
[   82.754019] linkmodes_reply_size:103 size: 216
[   82.758523] linkmodes_fill_reply:117 skb tailroom: 724

[  126.781892] linkmodes_reply_size:103 size: 476
[  126.786464] linkmodes_fill_reply:117 skb tailroom: 724


> Michal
> 
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 0c772318c023..ed5357210193 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -342,7 +342,7 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
>  	ret = ops->reply_size(req_info, reply_data);
>  	if (ret < 0)
>  		goto err_cleanup;
> -	reply_len = ret;
> +	reply_len = ret + ethnl_reply_header_size();
>  	ret = -ENOMEM;
>  	rskb = ethnl_reply_init(reply_len, req_info->dev, ops->reply_cmd,
>  				ops->hdr_attr, info, &reply_payload);
> @@ -588,7 +588,7 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
>  	ret = ops->reply_size(req_info, reply_data);
>  	if (ret < 0)
>  		goto err_cleanup;
> -	reply_len = ret;
> +	reply_len = ret + ethnl_reply_header_size();
>  	ret = -ENOMEM;
>  	skb = genlmsg_new(reply_len, GFP_KERNEL);
>  	if (!skb)
> diff --git a/net/ethtool/strset.c b/net/ethtool/strset.c
> index 95eae5c68a52..0eed4e4909ab 100644
> --- a/net/ethtool/strset.c
> +++ b/net/ethtool/strset.c
> @@ -324,7 +324,6 @@ static int strset_reply_size(const struct ethnl_req_info *req_base,
>  	int len = 0;
>  	int ret;
>  
> -	len += ethnl_reply_header_size();
>  	for (i = 0; i < ETH_SS_COUNT; i++) {
>  		const struct strset_info *set_info = &data->sets[i];
>  
> 
> 

-- 
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 v5 1/2] ethtool: provide UAPI for PHY master/slave configuration.

[Michal Kubecek]

On Mon, May 04, 2020 at 10:55:56AM +0200, Oleksij Rempel wrote:
> On Mon, May 04, 2020 at 10:37:34AM +0200, Michal Kubecek wrote:
> > On Mon, May 04, 2020 at 10:04:17AM +0200, Oleksij Rempel wrote:
> > > @Michal,
> > > 
> > > i noticed that linkmodes_fill_reply() some times get not enough
> > > tailroom.
> > > if data->peer_empty == 0
> > > linkmodes_reply_size() size: 476
> > > linkmodes_fill_reply() skb tailroom: 724
> > > 
> > > 
> > > if data->peer_empty == 1
> > > linkmodes_reply_size() size: 216                                      
> > > linkmodes_fill_reply() skb tailroom: 212
> > > 
> > > In the last case i won't be able to attach master_lave state and cfg
> > > fields.
> > > 
> > > It looks like this issue was not introduced by my patches. May be you
> > > have idea, what is missing?
> > 
> > It's my mistake, I'm just not sure why I never ran into this while
> > testing. Please try the patch below.
> 
> thx! it works now:
> [   82.754019] linkmodes_reply_size:103 size: 216
> [   82.758523] linkmodes_fill_reply:117 skb tailroom: 724
> 
> [  126.781892] linkmodes_reply_size:103 size: 476
> [  126.786464] linkmodes_fill_reply:117 skb tailroom: 724

Thank you. It seems so far the natural skb padding was always sufficient
to hide the broken calculation. Some time ago I had a debugging printk
in place which checked calculated an actual size but that was with an
older version of the series and the code has been rewritten few times
before it reached mainline.

I'll run some tests with additional tracing of estimated and actual
message size and if I don't find other problem, I'll submit the fix.

Michal

Re: [PATCH v5 1/2] ethtool: provide UAPI for PHY master/slave configuration.

[Michal Kubecek]

On Mon, May 04, 2020 at 09:12:13AM +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>
> ---
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index ac2784192472f..42dda9d2082ee 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1768,6 +1768,90 @@ 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;

Why did you revert to silently ignoring requests in this case? On the
other hand, we might rather want to do a more generic check which would
handle all drivers not supporting the feature, see below.

[...]
> @@ -287,14 +308,37 @@ static bool ethnl_auto_linkmodes(struct ethtool_link_ksettings *ksettings,
>  			     __ETHTOOL_LINK_MODE_MASK_NBITS);
>  }
>  
> +static int ethnl_validate_master_slave_cfg(u8 cfg)
> +{
> +	switch (cfg) {
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +		return 1;
> +	}
> +
> +	return 0;
> +}

Nitpick: bool would be more appropriate as return value.

> +
>  static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
>  				  struct ethtool_link_ksettings *ksettings,
>  				  bool *mod)
>  {
>  	struct ethtool_link_settings *lsettings = &ksettings->base;
>  	bool req_speed, req_duplex;
> +	const struct nlattr *master_slave_cfg;
>  	int ret;
>  
> +	master_slave_cfg = tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG];
> +	if (master_slave_cfg) {
> +		u8 cfg = nla_get_u8(master_slave_cfg);
> +		if (!ethnl_validate_master_slave_cfg(cfg)) {
> +			GENL_SET_ERR_MSG(info, "LINKMODES_MASTER_SLAVE_CFG contains not valid value");
> +			return -EOPNOTSUPP;
> +		}
> +	}

Please set also the "bad attribute" in extack, it may help
non-interactive clients.

Also, it would be nice to report error if client wants to set master/slave but
driver does not support it. How about this?

	if (master_slave_cfg) {
		u8 cfg = nla_get_u8(master_slave_cfg);

		if (lsettings->master_slave_cfg == MASTER_SLAVE_CFG_UNSUPPORTED) {
			NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
					    "master/slave configuration not supported by device");
			return -EOPNOTSUPP;
		}
		if (!ethnl_validate_master_slave_cfg(cfg)) {
			NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
					    "master/slave value is invalid");
			return -EOPNOTSUPP;
		}
	}


Do you plan to allow handling master/slave also via ioctl()? If yes, we should
also add the sanity checks to ioctl code path. If not, we should prevent
passing non-zero values from userspace to driver.

Other than this, the patch looks good to me.

Michal

>  	*mod = false;
>  	req_speed = tb[ETHTOOL_A_LINKMODES_SPEED];
>  	req_duplex = tb[ETHTOOL_A_LINKMODES_DUPLEX];
> @@ -311,6 +355,7 @@ 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_cfg, master_slave_cfg, mod);
>  
>  	if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
>  	    (req_speed || req_duplex) &&
> -- 
> 2.26.2
> 

Re: [PATCH v5 1/2] ethtool: provide UAPI for PHY master/slave configuration.

[Oleksij Rempel]

On Mon, May 04, 2020 at 11:10:44AM +0200, Michal Kubecek wrote:
> On Mon, May 04, 2020 at 09:12:13AM +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>
> > ---
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index ac2784192472f..42dda9d2082ee 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -1768,6 +1768,90 @@ 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;
> 
> Why did you revert to silently ignoring requests in this case?

genphy_setup_forced() is called by __genphy_config_aneg() and this can
be called by a PHY driver after configuring master slave mode locally by
PHY driver. See tja11xx patch. Same can be potentially done in the phy/realtek.c
driver.

Currently my imagination is not caffeanized enough to
provide a better solution. Do you have ideas?

> On the
> other hand, we might rather want to do a more generic check which would
> handle all drivers not supporting the feature, see below.

> [...]
> > @@ -287,14 +308,37 @@ static bool ethnl_auto_linkmodes(struct ethtool_link_ksettings *ksettings,
> >  			     __ETHTOOL_LINK_MODE_MASK_NBITS);
> >  }
> >  
> > +static int ethnl_validate_master_slave_cfg(u8 cfg)
> > +{
> > +	switch (cfg) {
> > +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> > +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> > +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> > +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> > +		return 1;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Nitpick: bool would be more appropriate as return value.

ok

> > +
> >  static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
> >  				  struct ethtool_link_ksettings *ksettings,
> >  				  bool *mod)
> >  {
> >  	struct ethtool_link_settings *lsettings = &ksettings->base;
> >  	bool req_speed, req_duplex;
> > +	const struct nlattr *master_slave_cfg;
> >  	int ret;
> >  
> > +	master_slave_cfg = tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG];
> > +	if (master_slave_cfg) {
> > +		u8 cfg = nla_get_u8(master_slave_cfg);
> > +		if (!ethnl_validate_master_slave_cfg(cfg)) {
> > +			GENL_SET_ERR_MSG(info, "LINKMODES_MASTER_SLAVE_CFG contains not valid value");
> > +			return -EOPNOTSUPP;
> > +		}
> > +	}
> 
> Please set also the "bad attribute" in extack, it may help
> non-interactive clients.
> 
> Also, it would be nice to report error if client wants to set master/slave but
> driver does not support it. How about this?
> 
> 	if (master_slave_cfg) {
> 		u8 cfg = nla_get_u8(master_slave_cfg);
> 
> 		if (lsettings->master_slave_cfg == MASTER_SLAVE_CFG_UNSUPPORTED) {
> 			NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
> 					    "master/slave configuration not supported by device");
> 			return -EOPNOTSUPP;
> 		}
> 		if (!ethnl_validate_master_slave_cfg(cfg)) {
> 			NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
> 					    "master/slave value is invalid");
> 			return -EOPNOTSUPP;
> 		}
> 	}
> 

looks good. thx!

> 
> Do you plan to allow handling master/slave also via ioctl()?

no.

> If yes, we should
> also add the sanity checks to ioctl code path. If not, we should prevent
> passing non-zero values from userspace to driver.

What is the best place to add this sanity check?

> Other than this, the patch looks good to me.
> 
> Michal
> 
> >  	*mod = false;
> >  	req_speed = tb[ETHTOOL_A_LINKMODES_SPEED];
> >  	req_duplex = tb[ETHTOOL_A_LINKMODES_DUPLEX];
> > @@ -311,6 +355,7 @@ 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_cfg, master_slave_cfg, mod);
> >  
> >  	if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
> >  	    (req_speed || req_duplex) &&
> > -- 
> > 2.26.2
> > 
> 
> 

-- 
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 v5 1/2] ethtool: provide UAPI for PHY master/slave configuration.

[Michal Kubecek]

On Mon, May 04, 2020 at 12:10:29PM +0200, Oleksij Rempel wrote:
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index ac2784192472f..42dda9d2082ee 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -1768,6 +1768,90 @@ 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;
> > 
> > Why did you revert to silently ignoring requests in this case?
> 
> genphy_setup_forced() is called by __genphy_config_aneg() and this can
> be called by a PHY driver after configuring master slave mode locally by
> PHY driver. See tja11xx patch. Same can be potentially done in the phy/realtek.c
> driver.
> 
> Currently my imagination is not caffeanized enough to
> provide a better solution. Do you have ideas?

If we have the check in ethnl_update_linkmodes(), we shouldn't really
get here so I believe we can leave this part as it is.

> > >  static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
> > >  				  struct ethtool_link_ksettings *ksettings,
> > >  				  bool *mod)
> > >  {
> > >  	struct ethtool_link_settings *lsettings = &ksettings->base;
> > >  	bool req_speed, req_duplex;
> > > +	const struct nlattr *master_slave_cfg;
> > >  	int ret;
> > >  
> > > +	master_slave_cfg = tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG];
> > > +	if (master_slave_cfg) {
> > > +		u8 cfg = nla_get_u8(master_slave_cfg);
> > > +		if (!ethnl_validate_master_slave_cfg(cfg)) {
> > > +			GENL_SET_ERR_MSG(info, "LINKMODES_MASTER_SLAVE_CFG contains not valid value");
> > > +			return -EOPNOTSUPP;
> > > +		}
> > > +	}
> > 
> > Please set also the "bad attribute" in extack, it may help
> > non-interactive clients.
> > 
> > Also, it would be nice to report error if client wants to set master/slave but
> > driver does not support it. How about this?
> > 
> > 	if (master_slave_cfg) {
> > 		u8 cfg = nla_get_u8(master_slave_cfg);
> > 
> > 		if (lsettings->master_slave_cfg == MASTER_SLAVE_CFG_UNSUPPORTED) {
> > 			NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
> > 					    "master/slave configuration not supported by device");
> > 			return -EOPNOTSUPP;
> > 		}
> > 		if (!ethnl_validate_master_slave_cfg(cfg)) {
> > 			NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
> > 					    "master/slave value is invalid");
> > 			return -EOPNOTSUPP;
> > 		}
> > 	}
> > 
> 
> looks good. thx!
> 
> > 
> > Do you plan to allow handling master/slave also via ioctl()?
> 
> no.
> 
> > If yes, we should
> > also add the sanity checks to ioctl code path. If not, we should prevent
> > passing non-zero values from userspace to driver.
> 
> What is the best place to add this sanity check?

If there is no plan to allow handling master/slave via ioctl, the best
option would IMHO be zeroing both fields in ethtool_get_link_ksettings()
right before the call to store_link_ksettings_for_user() and either
zeroing master_slave_cfg in ethtool_set_link_ksettings() after the call
load_link_ksettings_from_user(), or checking that it's zero (i.e. that
userspace left it untouched).

Michal

[PATCH v5 2/2] net: phy: tja11xx: add support for master-slave configuration

[Oleksij Rempel]

The TJA11xx PHYs have a vendor specific Master/Slave configuration bit,
which is not compatible with IEEE 803.2-2018 spec for 100Base-T1
devices. So, provide a custom config_ange call back to solve this
problem.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/nxp-tja11xx.c | 43 +++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index cc766b2d4136e..ca5f9d4dc57ed 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -30,6 +30,7 @@
 #define MII_ECTRL_WAKE_REQUEST		BIT(0)
 
 #define MII_CFG1			18
+#define MII_CFG1_MASTER_SLAVE		BIT(15)
 #define MII_CFG1_AUTO_OP		BIT(14)
 #define MII_CFG1_SLEEP_CONFIRM		BIT(6)
 #define MII_CFG1_LED_MODE_MASK		GENMASK(5, 4)
@@ -167,6 +168,32 @@ static int tja11xx_soft_reset(struct phy_device *phydev)
 	return genphy_soft_reset(phydev);
 }
 
+static int tja11xx_config_aneg(struct phy_device *phydev)
+{
+	u16 ctl = 0;
+	int ret;
+
+	switch (phydev->master_slave_set) {
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+		ctl |= MII_CFG1_MASTER_SLAVE;
+		break;
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+		break;
+	case MASTER_SLAVE_CFG_UNKNOWN:
+	case MASTER_SLAVE_CFG_UNSUPPORTED:
+		return 0;
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		return -ENOTSUPP;
+	}
+
+	ret = phy_modify_changed(phydev, MII_CFG1, MII_CFG1_MASTER_SLAVE, ctl);
+	if (ret < 0)
+		return ret;
+
+	return __genphy_config_aneg(phydev, ret);
+}
+
 static int tja11xx_config_init(struct phy_device *phydev)
 {
 	int ret;
@@ -224,10 +251,22 @@ static int tja11xx_read_status(struct phy_device *phydev)
 {
 	int ret;
 
+	phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
+	phydev->master_slave_state = MASTER_SLAVE_STATE_UNSUPPORTED;
+
 	ret = genphy_update_link(phydev);
 	if (ret)
 		return ret;
 
+	ret = phy_read(phydev, MII_CFG1);
+	if (ret < 0)
+		return ret;
+
+	if (ret & MII_CFG1_MASTER_SLAVE)
+		phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
+	else
+		phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
+
 	if (phydev->link) {
 		ret = phy_read(phydev, MII_COMMSTAT);
 		if (ret < 0)
@@ -504,6 +543,7 @@ static struct phy_driver tja11xx_driver[] = {
 		.features       = PHY_BASIC_T1_FEATURES,
 		.probe		= tja11xx_probe,
 		.soft_reset	= tja11xx_soft_reset,
+		.config_aneg	= tja11xx_config_aneg,
 		.config_init	= tja11xx_config_init,
 		.read_status	= tja11xx_read_status,
 		.suspend	= genphy_suspend,
@@ -519,6 +559,7 @@ static struct phy_driver tja11xx_driver[] = {
 		.features       = PHY_BASIC_T1_FEATURES,
 		.probe		= tja11xx_probe,
 		.soft_reset	= tja11xx_soft_reset,
+		.config_aneg	= tja11xx_config_aneg,
 		.config_init	= tja11xx_config_init,
 		.read_status	= tja11xx_read_status,
 		.suspend	= genphy_suspend,
@@ -533,6 +574,7 @@ static struct phy_driver tja11xx_driver[] = {
 		.features       = PHY_BASIC_T1_FEATURES,
 		.probe		= tja1102_p0_probe,
 		.soft_reset	= tja11xx_soft_reset,
+		.config_aneg	= tja11xx_config_aneg,
 		.config_init	= tja11xx_config_init,
 		.read_status	= tja11xx_read_status,
 		.match_phy_device = tja1102_p0_match_phy_device,
@@ -551,6 +593,7 @@ static struct phy_driver tja11xx_driver[] = {
 		.features       = PHY_BASIC_T1_FEATURES,
 		/* currently no probe for Port 1 is need */
 		.soft_reset	= tja11xx_soft_reset,
+		.config_aneg	= tja11xx_config_aneg,
 		.config_init	= tja11xx_config_init,
 		.read_status	= tja11xx_read_status,
 		.match_phy_device = tja1102_p1_match_phy_device,
-- 
2.26.2