Hi,
This is the result of this discussion:
https://lore.kernel.org/netdev/[email protected]/
The goal here is to get the GYP215 and LAN8814 running on the Microchip
LAN9668 SoC. The LAN9668 suppports one external bus and unfortunately, the
LAN8814 has a bug which makes it impossible to use C45 on that bus.
Fortunately, it was the intention of the GPY215 driver to be used on a C22
bus. But I think this could have never really worked, because the
phy_get_c45_ids() will always do c45 accesses and thus on MDIO bus drivers
which will correctly check for the MII_ADDR_C45 flag and return -EOPNOTSUPP
the function call will fail and thus gpy_probe() will fail. This series
tries to fix that and will lay the foundation to add a workaround for the
LAN8814 bug by forcing an MDIO bus to be c22-only.
At the moment, the probe_capabilities is taken into account to decide if
we have to use C45-over-C22. What is still missing from this series is the
handling of a device tree property to restrict the probe_capabilities to
c22-only.
Since net-next is closed, this is marked as RFC to get some early feedback.
Michael Walle (5):
net: phy: mscc-miim: reject clause 45 register accesses
net: phy: support indirect c45 access in get_phy_c45_ids()
net: phy: mscc-miim: add probe_capabilities
net: phy: introduce is_c45_over_c22 flag
net: phylink: handle the new is_c45_over_c22 property
drivers/net/mdio/mdio-mscc-miim.c | 7 ++++
drivers/net/phy/mxl-gpy.c | 2 +-
drivers/net/phy/phy_device.c | 65 ++++++++++++++++++++++++++-----
drivers/net/phy/phylink.c | 2 +-
include/linux/phy.h | 4 +-
5 files changed, 68 insertions(+), 12 deletions(-)
--
2.30.2
On Wed, Mar 23, 2022 at 07:34:14PM +0100, Michael Walle wrote:
> Hi,
>
> This is the result of this discussion:
> https://lore.kernel.org/netdev/[email protected]/
>
> The goal here is to get the GYP215 and LAN8814 running on the Microchip
> LAN9668 SoC. The LAN9668 suppports one external bus and unfortunately, the
> LAN8814 has a bug which makes it impossible to use C45 on that bus.
> Fortunately, it was the intention of the GPY215 driver to be used on a C22
> bus. But I think this could have never really worked, because the
> phy_get_c45_ids() will always do c45 accesses and thus on MDIO bus drivers
> which will correctly check for the MII_ADDR_C45 flag and return -EOPNOTSUPP
> the function call will fail and thus gpy_probe() will fail. This series
> tries to fix that and will lay the foundation to add a workaround for the
> LAN8814 bug by forcing an MDIO bus to be c22-only.
>
> At the moment, the probe_capabilities is taken into account to decide if
> we have to use C45-over-C22. What is still missing from this series is the
> handling of a device tree property to restrict the probe_capabilities to
> c22-only.
We have a problem here with phydev->is_c45.
In phy-core.c, functions __phy_read_mmd() and __phy_write_mmd() it
means perform c45 transactions over the bus. We know we want to access
a register in c45 space because we are using an _mmd() function.
In phy.c, it means does this PHY have c45 registers and we should
access that register space, or should we use the c22 register
space. So far example phy_restart_aneg() decides to either call
genphy_c45_restart_aneg() or genphy_restart_aneg() depending on
is_c45.
So a PHY with C45 register space but only accessible by C45 over C22
is probably going to do the wrong thing with the current code.
For this patchset to work, we need to cleanly separate the concepts of
what sort of transactions to do over the bus, from what register
spaces the PHY has. We probably want something like phydev->has_c45 to
indicate the register space is implemented, and phydev->c45_over_c22
to indicate what sort of transaction should be used in the _mmd()
functions.
Your patches start in that direction, but i don't think it goes far
enough.
Andrew
The driver doesn't support clause 45 register access yet, but doesn't
check if the access is a c45 one either. This leads to spurious register
reads and writes. Add the check.
Fixes: 542671fe4d86 ("net: phy: mscc-miim: Add MDIO driver")
Signed-off-by: Michael Walle <[email protected]>
---
drivers/net/mdio/mdio-mscc-miim.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index c483ba67c21f..582969751b4c 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -102,6 +102,9 @@ static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
u32 val;
int ret;
+ if (regnum & MII_ADDR_C45)
+ return -EOPNOTSUPP;
+
ret = mscc_miim_wait_pending(bus);
if (ret)
goto out;
@@ -145,6 +148,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
struct mscc_miim_dev *miim = bus->priv;
int ret;
+ if (regnum & MII_ADDR_C45)
+ return -EOPNOTSUPP;
+
ret = mscc_miim_wait_pending(bus);
if (ret < 0)
goto out;
--
2.30.2
Am 2022-03-23 21:30, schrieb Andrew Lunn:
> On Wed, Mar 23, 2022 at 07:34:14PM +0100, Michael Walle wrote:
>> Hi,
>>
>> This is the result of this discussion:
>> https://lore.kernel.org/netdev/[email protected]/
>>
>> The goal here is to get the GYP215 and LAN8814 running on the
>> Microchip
>> LAN9668 SoC. The LAN9668 suppports one external bus and unfortunately,
>> the
>> LAN8814 has a bug which makes it impossible to use C45 on that bus.
>> Fortunately, it was the intention of the GPY215 driver to be used on a
>> C22
>> bus. But I think this could have never really worked, because the
>> phy_get_c45_ids() will always do c45 accesses and thus on MDIO bus
>> drivers
>> which will correctly check for the MII_ADDR_C45 flag and return
>> -EOPNOTSUPP
>> the function call will fail and thus gpy_probe() will fail. This
>> series
>> tries to fix that and will lay the foundation to add a workaround for
>> the
>> LAN8814 bug by forcing an MDIO bus to be c22-only.
>>
>> At the moment, the probe_capabilities is taken into account to decide
>> if
>> we have to use C45-over-C22. What is still missing from this series is
>> the
>> handling of a device tree property to restrict the probe_capabilities
>> to
>> c22-only.
>
> We have a problem here with phydev->is_c45.
>
> In phy-core.c, functions __phy_read_mmd() and __phy_write_mmd() it
> means perform c45 transactions over the bus. We know we want to access
> a register in c45 space because we are using an _mmd() function.
>
> In phy.c, it means does this PHY have c45 registers and we should
> access that register space, or should we use the c22 register
> space. So far example phy_restart_aneg() decides to either call
> genphy_c45_restart_aneg() or genphy_restart_aneg() depending on
> is_c45.
Yes, that is probably the reason why the gpy215 has explicitly
set .aneg_done to genphy_c45_aneg_done() for example.
> So a PHY with C45 register space but only accessible by C45 over C22
> is probably going to do the wrong thing with the current code.
Oh my, yes. Looks like the whole phy_get_c45_ids() isn't working
at all for the gpy at the moment (or maybe it will work because
it supports AN via the c22 registers, too). I'll have to dig deeper
into that tomorrow. I know that _something_ worked at least ;)
> For this patchset to work, we need to cleanly separate the concepts of
> what sort of transactions to do over the bus, from what register
> spaces the PHY has. We probably want something like phydev->has_c45 to
> indicate the register space is implemented, and phydev->c45_over_c22
> to indicate what sort of transaction should be used in the _mmd()
> functions.
>
> Your patches start in that direction, but i don't think it goes far
> enough.
Thanks for the review!
-michael
The driver is currently only capable of doing c22 accesses. Add the
corresponding probe_capabilities.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/net/mdio/mdio-mscc-miim.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
index 582969751b4c..c9efcfa2a1ce 100644
--- a/drivers/net/mdio/mdio-mscc-miim.c
+++ b/drivers/net/mdio/mdio-mscc-miim.c
@@ -225,6 +225,7 @@ int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
bus->read = mscc_miim_read;
bus->write = mscc_miim_write;
bus->reset = mscc_miim_reset;
+ bus->probe_capabilities = MDIOBUS_C22;
snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(dev));
bus->parent = dev;
--
2.30.2
There are some PHYs, namely the Maxlinear GPY215, whose driver is
explicitly supporting C45-over-C22 access. At least that was the
intention. In practice, it cannot work because get_phy_c45_ids()
will always issue c45 register accesses.
There is another issue at hand: the Microchip LAN8814, which is
a c22 only quad PHY, has issues with c45 accesses on the same bus
and its address decoder will find a match in the middle of another
c45 transaction. This will lead to spurious reads and writes. The
reads will corrupt the c45 in flight. The write will lead to random
writes to the LAN8814 registers. As a workaround for PHYs which
support C45-over-C22 register accesses, we can make the MDIO bus
c22-only.
For both reasons, extend the register accesses in get_phy_c45_ids()
to allow indirect accesses, indicated by the bus->probe_capabilities
bits. The probe_capabilites can then be degraded by a device tree
property, for example. Or it will just work when the MDIO driver
is c22-only and set the capabilities accordingly.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/net/phy/phy_device.c | 46 ++++++++++++++++++++++++++++++++----
1 file changed, 41 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8406ac739def..c766f5bb421a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -649,6 +649,42 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
}
EXPORT_SYMBOL(phy_device_create);
+static int mdiobus_probe_mmd_read(struct mii_bus *bus, int prtad, int devad,
+ u16 regnum)
+{
+ int ret;
+
+ /* For backwards compatibility, treat MDIOBUS_NO_CAP as c45 capable */
+ if (bus->probe_capabilities == MDIOBUS_NO_CAP ||
+ bus->probe_capabilities >= MDIOBUS_C45)
+ return mdiobus_c45_read(bus, prtad, devad, regnum);
+
+ mutex_lock(&bus->mdio_lock);
+
+ /* Write the desired MMD Devad */
+ ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL, devad);
+ if (ret)
+ goto out;
+
+ /* Write the desired MMD register address */
+ ret = __mdiobus_write(bus, prtad, MII_MMD_DATA, regnum);
+ if (ret)
+ goto out;
+
+ /* Select the Function : DATA with no post increment */
+ ret = __mdiobus_write(bus, prtad, MII_MMD_CTRL,
+ devad | MII_MMD_CTRL_NOINCR);
+ if (ret)
+ goto out;
+
+ ret = __mdiobus_read(bus, prtad, MII_MMD_DATA);
+
+out:
+ mutex_unlock(&bus->mdio_lock);
+
+ return ret;
+}
+
/* phy_c45_probe_present - checks to see if a MMD is present in the package
* @bus: the target MII bus
* @prtad: PHY package address on the MII bus
@@ -664,7 +700,7 @@ static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
{
int stat2;
- stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
+ stat2 = mdiobus_probe_mmd_read(bus, prtad, devad, MDIO_STAT2);
if (stat2 < 0)
return stat2;
@@ -687,12 +723,12 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
{
int phy_reg;
- phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS2);
+ phy_reg = mdiobus_probe_mmd_read(bus, addr, dev_addr, MDIO_DEVS2);
if (phy_reg < 0)
return -EIO;
*devices_in_package = phy_reg << 16;
- phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS1);
+ phy_reg = mdiobus_probe_mmd_read(bus, addr, dev_addr, MDIO_DEVS1);
if (phy_reg < 0)
return -EIO;
*devices_in_package |= phy_reg;
@@ -776,12 +812,12 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
continue;
}
- phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
+ phy_reg = mdiobus_probe_mmd_read(bus, addr, i, MII_PHYSID1);
if (phy_reg < 0)
return -EIO;
c45_ids->device_ids[i] = phy_reg << 16;
- phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID2);
+ phy_reg = mdiobus_probe_mmd_read(bus, addr, i, MII_PHYSID2);
if (phy_reg < 0)
return -EIO;
c45_ids->device_ids[i] |= phy_reg;
--
2.30.2
On Wed, Mar 23, 2022 at 07:34:15PM +0100, Michael Walle wrote:
> The driver doesn't support clause 45 register access yet, but doesn't
> check if the access is a c45 one either. This leads to spurious register
> reads and writes. Add the check.
>
> Fixes: 542671fe4d86 ("net: phy: mscc-miim: Add MDIO driver")
> Signed-off-by: Michael Walle <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew