On Tue, May 28, 2019 at 03:54:31PM +0300, Vladimir Oltean wrote:
> On Tue, 28 May 2019 at 12:58, Russell King <rmk+ker...@armlinux.org.uk> wrote:
> >
> > Allow userspace to generate Clause 45 MII access cycles via phylib.
> > This is useful for tools such as mii-diag to be able to inspect Clause
> > 45 PHYs.
> >
> > Reviewed-by: Florian Fainelli <f.faine...@gmail.com>
> > Signed-off-by: Russell King <rmk+ker...@armlinux.org.uk>
> > ---
> > drivers/net/phy/phy.c | 33 ++++++++++++++++++++++++---------
> > 1 file changed, 24 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 3745220c5c98..6d279c2ac1f8 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -386,6 +386,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct
> > ifreq *ifr, int cmd)
> > struct mii_ioctl_data *mii_data = if_mii(ifr);
> > u16 val = mii_data->val_in;
> > bool change_autoneg = false;
> > + int prtad, devad;
> >
> > switch (cmd) {
> > case SIOCGMIIPHY:
> > @@ -393,14 +394,29 @@ int phy_mii_ioctl(struct phy_device *phydev, struct
> > ifreq *ifr, int cmd)
> > /* fall through */
> >
> > case SIOCGMIIREG:
> > - mii_data->val_out = mdiobus_read(phydev->mdio.bus,
> > - mii_data->phy_id,
> > - mii_data->reg_num);
> > + if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> > + prtad = mdio_phy_id_prtad(mii_data->phy_id);
> > + devad = mdio_phy_id_devad(mii_data->phy_id);
> > + devad = MII_ADDR_C45 | devad << 16 |
> > mii_data->reg_num;
> > + } else {
> > + prtad = mii_data->phy_id;
> > + devad = mii_data->reg_num;
> > + }
> > + mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad,
> > + devad);
> > return 0;
> >
> > case SIOCSMIIREG:
> > - if (mii_data->phy_id == phydev->mdio.addr) {
> > - switch (mii_data->reg_num) {
> > + if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> > + prtad = mdio_phy_id_prtad(mii_data->phy_id);
> > + devad = mdio_phy_id_devad(mii_data->phy_id);
> > + devad = MII_ADDR_C45 | devad << 16 |
> > mii_data->reg_num;
> > + } else {
> > + prtad = mii_data->phy_id;
> > + devad = mii_data->reg_num;
> > + }
> > + if (prtad == phydev->mdio.addr) {
> > + switch (devad) {
> > case MII_BMCR:
> > if ((val & (BMCR_RESET | BMCR_ANENABLE)) ==
> > 0) {
> > if (phydev->autoneg ==
> > AUTONEG_ENABLE)
> > @@ -433,11 +449,10 @@ int phy_mii_ioctl(struct phy_device *phydev, struct
> > ifreq *ifr, int cmd)
> > }
> > }
> >
> > - mdiobus_write(phydev->mdio.bus, mii_data->phy_id,
> > - mii_data->reg_num, val);
> > + mdiobus_write(phydev->mdio.bus, prtad, devad, val);
> >
> > - if (mii_data->phy_id == phydev->mdio.addr &&
> > - mii_data->reg_num == MII_BMCR &&
> > + if (prtad == phydev->mdio.addr &&
> > + devad == MII_BMCR &&
> > val & BMCR_RESET)
> > return phy_init_hw(phydev);
> >
> > --
> > 2.7.4
> >
>
> Hi Russell,
>
> I find the SIOCGMIIREG/SIOCGMIIPHY ioctls useful for C45 just as much
> as they are for C22, but I think the way they work is a big hack and
> for that reason they're less than useful when you need them most.
> These ioctls work by hijacking the MDIO bus driver of a PHY that is
> attached to a net_device. Hence they can be used to access at most a
> PHY that lies on the same MDIO bus as one you already have a
> phy-handle to.
> If you have a PHY issue that makes of_phy_connect fail and the
> net_device to fail to probe, basically you're SOL because you lose
> that one handle that userspace had to the MDIO bus.
> Similarly if you're doing a bring-up and all PHY interfaces are fixed-link.
> Maybe it would be better to rethink this and expose some sysfs nodes
> for raw MDIO access in the bus drivers.
I don't see how putting some attributes in sysfs helps - sysfs is
fine for exporting structured information, but with PHYs, it's not
that structured. ioctls on sysfs attributes are certainly very
undesirable, and not supported. So, I don't think sysfs would work.
debugfs is another option, that is more flexible, but that is also
based around the idea of exporting stuff in a relatively structured
way, and I don't think a MII bus with arbitary PHYs with an arbitary
number of layers would be exportable in a particularly nice way.
Consider that a Clause 45 PHY can have up to 32 layers, each with
64Ki of register space - that's a total of 2Mi of 16-bit registers.
What would be better would be for the MDIO layer to have /dev nodes
that userspace could use to access the bus independent of the PHY,
much the same as we have /dev/i2c-* - but I'm not sure if we really
want to invent a whole new interface to MDIO buses.
The MII interface already exists, works for the most part, and is
already used for Clause 45 PHYs for a number of NICs. I do agree
that it is less than perfect though.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
