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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 |
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
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 |
> 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
> ``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
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
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
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 |
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
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
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