[PATCH] net: phy: realtek: Add dummy stubs for MMD register access for rtl8211b

[PATCH] net: phy: realtek: Add dummy stubs for MMD register access for rtl8211b

[Kevin Hao]

The Ethernet on mpc8315erdb is broken since commit b6b5e8a69118
("gianfar: Disable EEE autoneg by default"). The reason is that
even though the rtl8211b doesn't support the MMD extended registers
access, it does return some random values if we trying to access
the MMD register via indirect method. This makes it seem that the
EEE is supported by this phy device. And the subsequent writing to
the MMD registers does cause the phy malfunction. So add dummy stubs
for the MMD register access to fix this issue.

Fixes: b6b5e8a69118 ("gianfar: Disable EEE autoneg by default")
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/net/phy/realtek.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index ee3ca4a2f12b..0a7301ef4ac9 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -103,6 +103,17 @@ static int rtl8211b_config_intr(struct phy_device *phydev)
 	return err;
 }
 
+static int rtl8211b_read_mmd(struct phy_device *phdev, int devad, u16 regnum)
+{
+	return -EINVAL;
+}
+
+static int rtl8211b_write_mmd(struct phy_device *phdev, int devnum, u16 regnum,
+			      u16 val)
+{
+	return -EINVAL;
+}
+
 static int rtl8211e_config_intr(struct phy_device *phydev)
 {
 	int err;
@@ -172,6 +183,8 @@ static struct phy_driver realtek_drvs[] = {
 		.flags		= PHY_HAS_INTERRUPT,
 		.ack_interrupt	= &rtl821x_ack_interrupt,
 		.config_intr	= &rtl8211b_config_intr,
+		.read_mmd	= &rtl8211b_read_mmd,
+		.write_mmd	= &rtl8211b_write_mmd,
 	}, {
 		.phy_id		= 0x001cc914,
 		.name		= "RTL8211DN Gigabit Ethernet",
-- 
2.9.3

Re: [PATCH] net: phy: realtek: Add dummy stubs for MMD register access for rtl8211b

[Andrew Lunn]

On Mon, Mar 19, 2018 at 08:05:47PM +0800, Kevin Hao wrote:
> The Ethernet on mpc8315erdb is broken since commit b6b5e8a69118
> ("gianfar: Disable EEE autoneg by default"). The reason is that
> even though the rtl8211b doesn't support the MMD extended registers
> access, it does return some random values if we trying to access
> the MMD register via indirect method. This makes it seem that the
> EEE is supported by this phy device. And the subsequent writing to
> the MMD registers does cause the phy malfunction. So add dummy stubs
> for the MMD register access to fix this issue.

Hi Kevin

The Micrel PHY has the same problem. Please add generic
genphy_read_mmd_unsupported() and genphy_write_mmd_unsupported() to
phy_device.c, and use them from both the Micrel and Realtek PHY
drivers.

Also, a return value of -EOPNOTSUPP is more appropriate.

Thanks

	Andrew

Re: [PATCH] net: phy: realtek: Add dummy stubs for MMD register access for rtl8211b

[Kevin Hao]

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

On Mon, Mar 19, 2018 at 01:39:39PM +0100, Andrew Lunn wrote:
> On Mon, Mar 19, 2018 at 08:05:47PM +0800, Kevin Hao wrote:
> > The Ethernet on mpc8315erdb is broken since commit b6b5e8a69118
> > ("gianfar: Disable EEE autoneg by default"). The reason is that
> > even though the rtl8211b doesn't support the MMD extended registers
> > access, it does return some random values if we trying to access
> > the MMD register via indirect method. This makes it seem that the
> > EEE is supported by this phy device. And the subsequent writing to
> > the MMD registers does cause the phy malfunction. So add dummy stubs
> > for the MMD register access to fix this issue.
> 
> Hi Kevin
> 
> The Micrel PHY has the same problem. Please add generic
> genphy_read_mmd_unsupported() and genphy_write_mmd_unsupported() to
> phy_device.c, and use them from both the Micrel and Realtek PHY
> drivers.
> 
> Also, a return value of -EOPNOTSUPP is more appropriate.

Thanks for the suggestion. Will do.

Thanks,
Kevin

> 
> Thanks
> 
> 	Andrew

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] net: phy: realtek: Add dummy stubs for MMD register access for rtl8211b

[Claudiu Manoil]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Monday, March 19, 2018 2:40 PM
> To: Kevin Hao <haokexin@gmail.com>
> Cc: netdev@vger.kernel.org; Florian Fainelli <f.fainelli@gmail.com>; Claudiu
> Manoil <claudiu.manoil@nxp.com>
> Subject: Re: [PATCH] net: phy: realtek: Add dummy stubs for MMD register
> access for rtl8211b
> 
> On Mon, Mar 19, 2018 at 08:05:47PM +0800, Kevin Hao wrote:
> > The Ethernet on mpc8315erdb is broken since commit b6b5e8a69118
> > ("gianfar: Disable EEE autoneg by default"). The reason is that even
> > though the rtl8211b doesn't support the MMD extended registers access,
> > it does return some random values if we trying to access the MMD
> > register via indirect method. This makes it seem that the EEE is
> > supported by this phy device. And the subsequent writing to the MMD
> > registers does cause the phy malfunction. So add dummy stubs for the
> > MMD register access to fix this issue.
> 
> Hi Kevin
> 
> The Micrel PHY has the same problem. Please add generic
> genphy_read_mmd_unsupported() and
> genphy_write_mmd_unsupported() to phy_device.c, and use them from
> both the Micrel and Realtek PHY drivers.
> 
> Also, a return value of -EOPNOTSUPP is more appropriate.
> 

This gianfar patch should have been a temporary workaround.
Obviously, the driver of an (old) eth controller that does not support EEE should
not be modified to have the same eth controller work normally when some new EEE
capable phy happens to be attached to that controller (i.e. on a new board).
It should be up to the phy integration layer to identify that the controller and the phy
are not EEE compatible, and restrict the phy from entering EEE mode. (without any
change to the eth driver)

Claudiu

Re: [PATCH] net: phy: realtek: Add dummy stubs for MMD register access for rtl8211b

[Andrew Lunn]

> This gianfar patch should have been a temporary workaround.
> Obviously, the driver of an (old) eth controller that does not support EEE should
> not be modified to have the same eth controller work normally when some new EEE
> capable phy happens to be attached to that controller (i.e. on a new board).
> It should be up to the phy integration layer to identify that the controller and the phy
> are not EEE compatible, and restrict the phy from entering EEE mode. (without any
> change to the eth driver)

We end up in the same place, needing to patch the RealTek PHY driver.

A MAC driver indicates it can do EEE by calling phy_init_eee(). This
will then turn on EEE in the PHY.

A PHY is not supposed to negotiate EEE unless it is asked to. But some
PHYs do it by default, and the PHY driver is not turning it off. That
is what b6b5e8a69118 fixes for the gianfar.

We could unconditionally disable EEE on all PHYs at probe time, and
then let phy_init_eee() turn it back on again. But then we need the
fix posted here for the RealTek PHY.

There does not appear to be any bit in the PHY status registers to
indicate if MMD is supported or not. So i don't see how we can make
unconditional disable of EEE safe without introducing lots of
regressions.

	Andrew