| Message ID | eeea080649153eefafef8ff93274733fe8f25b01.1552559796.git.joabreu@synopsys.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: phy: Don't assume loopback is supported | expand |
From: Jose Abreu <jose.abreu@synopsys.com> Date: Thu, 14 Mar 2019 11:37:21 +0100 > Some PHYs may not support loopback mode so we need to check if register > is read-only. > > Fixes: f0f9b4ed2338 ("net: phy: Add phy loopback support in net phy framework") > Signed-off-by: Jose Abreu <joabreu@synopsys.com> Andrew et al., please review.
On 3/14/19 3:37 AM, Jose Abreu wrote: > Some PHYs may not support loopback mode so we need to check if register > is read-only. > In that case it may be appropriate to have a specific PHY driver that implements a set_loopback() method returning -EOPNOTSUPP instead of changing the generic PHY implementation. > Fixes: f0f9b4ed2338 ("net: phy: Add phy loopback support in net phy framework") > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Joao Pinto <joao.pinto@synopsys.com> This looks fine anyway: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/net/phy/phy_device.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 49fdd1ee798e..a749639d98c3 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1918,8 +1918,24 @@ EXPORT_SYMBOL(genphy_resume); > > int genphy_loopback(struct phy_device *phydev, bool enable) > { > - return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, > - enable ? BMCR_LOOPBACK : 0); > + int ret; > + > + ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, > + enable ? BMCR_LOOPBACK : 0); > + if (ret < 0) > + return ret; > + > + ret = phy_read(phydev, MII_BMCR); > + if (ret < 0) > + return ret; > + > + if (enable) { > + if (ret & BMCR_LOOPBACK) > + return 0; > + return -EOPNOTSUPP; > + } > + > + return 0; > } > EXPORT_SYMBOL(genphy_loopback); > >
On 14.03.2019 11:37, Jose Abreu wrote: > Some PHYs may not support loopback mode so we need to check if register > is read-only. > As I read Clause 22 this is a mandatory feature, the related bit is specified as R/W. Do you have an actual example of a PHY w/o loopback mode that doesn't allow to change this bit? > Fixes: f0f9b4ed2338 ("net: phy: Add phy loopback support in net phy framework") > Signed-off-by: Jose Abreu <joabreu@synopsys.com> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Joao Pinto <joao.pinto@synopsys.com> > --- > drivers/net/phy/phy_device.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 49fdd1ee798e..a749639d98c3 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1918,8 +1918,24 @@ EXPORT_SYMBOL(genphy_resume); > > int genphy_loopback(struct phy_device *phydev, bool enable) > { > - return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, > - enable ? BMCR_LOOPBACK : 0); > + int ret; > + > + ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, > + enable ? BMCR_LOOPBACK : 0); > + if (ret < 0) > + return ret; > + > + ret = phy_read(phydev, MII_BMCR); > + if (ret < 0) > + return ret; > + > + if (enable) { > + if (ret & BMCR_LOOPBACK) > + return 0; > + return -EOPNOTSUPP; > + } > + > + return 0; > } > EXPORT_SYMBOL(genphy_loopback); > >
On Fri, Mar 15, 2019 at 03:48:41PM -0700, Florian Fainelli wrote: > On 3/14/19 3:37 AM, Jose Abreu wrote: > > Some PHYs may not support loopback mode so we need to check if register > > is read-only. > > > > In that case it may be appropriate to have a specific PHY driver that > implements a set_loopback() method returning -EOPNOTSUPP instead of > changing the generic PHY implementation. Hi Jose Since Heiner says this is a mandatory feature, we should not really penalise conformant PHYs just because there is one broken PHY. Please handle this in the PHY driver, by implementing the set_loopback call. Andrew
Hi Andrew and Heiner, On 3/17/2019 6:38 PM, Andrew Lunn wrote: > On Fri, Mar 15, 2019 at 03:48:41PM -0700, Florian Fainelli wrote: >> On 3/14/19 3:37 AM, Jose Abreu wrote: >>> Some PHYs may not support loopback mode so we need to check if register >>> is read-only. >>> >> >> In that case it may be appropriate to have a specific PHY driver that >> implements a set_loopback() method returning -EOPNOTSUPP instead of >> changing the generic PHY implementation. > > Hi Jose > > Since Heiner says this is a mandatory feature, we should not really > penalise conformant PHYs just because there is one broken PHY. We provide PHYs to our customers and in the documentation I have this can be an optional feature that HW team can choose to have or not, making the bit read-only or r/w. Heiner, can you please confirm there is no Clause 22 "pitfalls" / "hidden comments" that allow this bitfield to be read-only ? Thanks, Jose Miguel Abreu
> We provide PHYs to our customers and in the documentation I have > this can be an optional feature that HW team can choose to have > or not, making the bit read-only or r/w. > > Heiner, can you please confirm there is no Clause 22 "pitfalls" / > "hidden comments" that allow this bitfield to be read-only ? Hi Jose I have the 802.3 standard from 2015. It should be free to download from the IEEE. So you can go get it yourself. Section 22.2.4.1.2 defines loopback. I don't see anything which makes it optional. The only wiggle room you have is where in the PHY the loopback actually takes place. That is implementation specific, but it recommends you make it as late as possible in the path so as to test as much as possible. If your PHY does not implement loopback, i would say it breaks the standard. We try to keep workarounds for brokenness in the specific PHY driver, not the generic code. Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 49fdd1ee798e..a749639d98c3 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1918,8 +1918,24 @@ EXPORT_SYMBOL(genphy_resume); int genphy_loopback(struct phy_device *phydev, bool enable) { - return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, - enable ? BMCR_LOOPBACK : 0); + int ret; + + ret = phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, + enable ? BMCR_LOOPBACK : 0); + if (ret < 0) + return ret; + + ret = phy_read(phydev, MII_BMCR); + if (ret < 0) + return ret; + + if (enable) { + if (ret & BMCR_LOOPBACK) + return 0; + return -EOPNOTSUPP; + } + + return 0; } EXPORT_SYMBOL(genphy_loopback);
Some PHYs may not support loopback mode so we need to check if register is read-only. Fixes: f0f9b4ed2338 ("net: phy: Add phy loopback support in net phy framework") Signed-off-by: Jose Abreu <joabreu@synopsys.com> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Joao Pinto <joao.pinto@synopsys.com> --- drivers/net/phy/phy_device.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)