lkml.org 
[lkml]   [2020]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1] ethtool: provide UAPI for PHY master/slave configuration.
>  ``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

\
 
 \ /
  Last update: 2020-04-16 00:00    [W:0.292 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site