Re: [PATCH net-next 3/8] net: mscc: Add MDIO driver

[Andrew Lunn]

> > It sounds like the correct fix is for get_phy_id() to look at the
> > error code for mdiobus_read(bus, addr, MII_PHYSID1). If it is EIO and
> > maybe ENODEV, set *phy_id to 0x and return. The scan code
> > should then do the correct thing.
> > 
> 
> That could work indeed. Do you want me to test and send a patch?

Yes please.

Thanks
Andrew

Re: [PATCH net-next 3/8] net: mscc: Add MDIO driver

[Alexandre Belloni]

On 29/03/2018 at 16:40:41 +0200, Andrew Lunn wrote:
> > > > +   for (i = 0; i < PHY_MAX_ADDR; i++) {
> > > > +   if (mscc_miim_read(bus, i, MII_PHYSID1) < 0)
> > > > +   bus->phy_mask |= BIT(i);
> > > > +   }
> > > 
> > > Why do this? Especially so for the external bus, where the PHYs might
> > > have a GPIO reset line, and won't respond until the gpio is
> > > released. The core code does that just before it scans the bus, or
> > > just before it scans the particular address on the bus, depending on
> > > the scope of the GPIO.
> > > 
> > 
> > IIRC, this was needed when probing the bus without DT, in that case, the
> > mdiobus_scan loop of __mdiobus_register() will fail when doing the
> > get_phy_id for phys 0 to 31 because get_phy_id() transforms any error in
> > -EIO and so it is impossible to register the bus. Other drivers have a
> > similar code to handle that case.
> 
> Hi Alexandre
> 
> Do you mean mscc_miim_read() will return -EIO if there is no device on
> the bus at the address trying to be read? Most devices just return
> 0x because there is a pull up on the data line, nothing is driving
> it, so all 1's are read.
> 

It will return -EIO but I tried to be clever and return -ENODEV but this
gets changed to -EIO by get_phy_id.

> It sounds like the correct fix is for get_phy_id() to look at the
> error code for mdiobus_read(bus, addr, MII_PHYSID1). If it is EIO and
> maybe ENODEV, set *phy_id to 0x and return. The scan code
> should then do the correct thing.
> 

That could work indeed. Do you want me to test and send a patch?


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 3/8] net: mscc: Add MDIO driver

[Andrew Lunn]

On Thu, Mar 29, 2018 at 04:05:44PM +0200, Alexandre Belloni wrote:
> On 23/03/2018 at 21:49:39 +0100, Andrew Lunn wrote:
> > On Fri, Mar 23, 2018 at 09:11:12PM +0100, Alexandre Belloni wrote:
> > > Add a driver for the Microsemi MII Management controller (MIIM) found on
> > > Microsemi SoCs.
> > > On Ocelot, there are two controllers, one is connected to the internal
> > > PHYs, the other one can communicate with external PHYs.
> > 
> > Hi Alexandre
> > 
> > This looks to be standalone. Such drivers we try to put in
> > drivers/net/phy.
> > 
> > > +static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
> > > +{
> > > + struct mscc_miim_dev *miim = bus->priv;
> > > + u32 val;
> > > + int ret;
> > > +
> > > + mutex_lock(>lock);
> > 
> > What are you locking against here?
> > 
> > And you don't appear to initialize the mutex anywhere.
> > 
> > > +static int mscc_miim_reset(struct mii_bus *bus)
> > > +{
> > > + struct mscc_miim_dev *miim = bus->priv;
> > > + int i;
> > > +
> > > + if (miim->phy_regs) {
> > > + writel(0, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> > > + writel(0x1ff, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> > > + mdelay(500);
> > > + }
> > > +
> > > + for (i = 0; i < PHY_MAX_ADDR; i++) {
> > > + if (mscc_miim_read(bus, i, MII_PHYSID1) < 0)
> > > + bus->phy_mask |= BIT(i);
> > > + }
> > 
> > Why do this? Especially so for the external bus, where the PHYs might
> > have a GPIO reset line, and won't respond until the gpio is
> > released. The core code does that just before it scans the bus, or
> > just before it scans the particular address on the bus, depending on
> > the scope of the GPIO.
> > 
> 
> IIRC, this was needed when probing the bus without DT, in that case, the
> mdiobus_scan loop of __mdiobus_register() will fail when doing the
> get_phy_id for phys 0 to 31 because get_phy_id() transforms any error in
> -EIO and so it is impossible to register the bus. Other drivers have a
> similar code to handle that case.

Hi Alexandre

Do you mean mscc_miim_read() will return -EIO if there is no device on
the bus at the address trying to be read? Most devices just return
0x because there is a pull up on the data line, nothing is driving
it, so all 1's are read.

It sounds like the correct fix is for get_phy_id() to look at the
error code for mdiobus_read(bus, addr, MII_PHYSID1). If it is EIO and
maybe ENODEV, set *phy_id to 0x and return. The scan code
should then do the correct thing.

   Andrew

Re: [PATCH net-next 3/8] net: mscc: Add MDIO driver

[Alexandre Belloni]

On 23/03/2018 at 14:51:19 -0700, Florian Fainelli wrote:
> > +   writel(MSCC_MIIM_CMD_VLD | (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> > +  (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) | MSCC_MIIM_CMD_OPR_READ,
> > +  miim->regs + MSCC_MIIM_REG_CMD);
> > +
> > +   ret = mscc_miim_wait_ready(bus);
> > +   if (ret)
> > +   goto out;
> 
> Your example had an interrupt specified, can't you use that instead of
> polling?
> 

the interrupt doesn't handle that. It is used to detect when a PHY
register has changed once the MIIM controller is configured to poll the
phys. At some point, this could be used to replace the PHY interrupts
but it doesn't correspond to the linux model so I didn't investigate too
much.

>  > +  for (i = 0; i < PHY_MAX_ADDR; i++) {
> > +   if (mscc_miim_read(bus, i, MII_PHYSID1) < 0)
> > +   bus->phy_mask |= BIT(i);
> > +   }
> 
> What is this used for? You have an OF MDIO bus which would create a
> phy_device for each node specified, is this a similar workaround to what
> drivers/net/phy/mdio-bcm-unimac.c has to do? If so, please document it
> as such.
> 

I replied to Andrew who had the same question.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 3/8] net: mscc: Add MDIO driver

[Alexandre Belloni]

On 23/03/2018 at 21:49:39 +0100, Andrew Lunn wrote:
> On Fri, Mar 23, 2018 at 09:11:12PM +0100, Alexandre Belloni wrote:
> > Add a driver for the Microsemi MII Management controller (MIIM) found on
> > Microsemi SoCs.
> > On Ocelot, there are two controllers, one is connected to the internal
> > PHYs, the other one can communicate with external PHYs.
> 
> Hi Alexandre
> 
> This looks to be standalone. Such drivers we try to put in
> drivers/net/phy.
> 
> > +static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
> > +{
> > +   struct mscc_miim_dev *miim = bus->priv;
> > +   u32 val;
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> 
> What are you locking against here?
> 
> And you don't appear to initialize the mutex anywhere.
> 
> > +static int mscc_miim_reset(struct mii_bus *bus)
> > +{
> > +   struct mscc_miim_dev *miim = bus->priv;
> > +   int i;
> > +
> > +   if (miim->phy_regs) {
> > +   writel(0, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> > +   writel(0x1ff, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> > +   mdelay(500);
> > +   }
> > +
> > +   for (i = 0; i < PHY_MAX_ADDR; i++) {
> > +   if (mscc_miim_read(bus, i, MII_PHYSID1) < 0)
> > +   bus->phy_mask |= BIT(i);
> > +   }
> 
> Why do this? Especially so for the external bus, where the PHYs might
> have a GPIO reset line, and won't respond until the gpio is
> released. The core code does that just before it scans the bus, or
> just before it scans the particular address on the bus, depending on
> the scope of the GPIO.
> 

IIRC, this was needed when probing the bus without DT, in that case, the
mdiobus_scan loop of __mdiobus_register() will fail when doing the
get_phy_id for phys 0 to 31 because get_phy_id() transforms any error in
-EIO and so it is impossible to register the bus. Other drivers have a
similar code to handle that case.

Anyway, I'll remove that loop for now because I'm only supporting DT.
I'll get back to that later.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 3/8] net: mscc: Add MDIO driver

[Florian Fainelli]

On 03/23/2018 01:11 PM, Alexandre Belloni wrote:
> Add a driver for the Microsemi MII Management controller (MIIM) found on
> Microsemi SoCs.
> On Ocelot, there are two controllers, one is connected to the internal
> PHYs, the other one can communicate with external PHYs.
> 
> Signed-off-by: Alexandre Belloni 
> ---
>  drivers/net/ethernet/Kconfig  |   1 +
>  drivers/net/ethernet/Makefile |   1 +
>  drivers/net/ethernet/mscc/Kconfig |  22 
>  drivers/net/ethernet/mscc/Makefile|   2 +
>  drivers/net/ethernet/mscc/mscc_miim.c | 210 
> ++
>  5 files changed, 236 insertions(+)
>  create mode 100644 drivers/net/ethernet/mscc/Kconfig
>  create mode 100644 drivers/net/ethernet/mscc/Makefile
>  create mode 100644 drivers/net/ethernet/mscc/mscc_miim.c
> 
> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
> index b6cf4b6962f5..adf643484198 100644
> --- a/drivers/net/ethernet/Kconfig
> +++ b/drivers/net/ethernet/Kconfig
> @@ -115,6 +115,7 @@ source "drivers/net/ethernet/mediatek/Kconfig"
>  source "drivers/net/ethernet/mellanox/Kconfig"
>  source "drivers/net/ethernet/micrel/Kconfig"
>  source "drivers/net/ethernet/microchip/Kconfig"
> +source "drivers/net/ethernet/mscc/Kconfig"
>  source "drivers/net/ethernet/moxa/Kconfig"
>  source "drivers/net/ethernet/myricom/Kconfig"
>  
> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
> index 3cdf01e96e0b..ed7df22de7ff 100644
> --- a/drivers/net/ethernet/Makefile
> +++ b/drivers/net/ethernet/Makefile
> @@ -56,6 +56,7 @@ obj-$(CONFIG_NET_VENDOR_MEDIATEK) += mediatek/
>  obj-$(CONFIG_NET_VENDOR_MELLANOX) += mellanox/
>  obj-$(CONFIG_NET_VENDOR_MICREL) += micrel/
>  obj-$(CONFIG_NET_VENDOR_MICROCHIP) += microchip/
> +obj-$(CONFIG_NET_VENDOR_MICROSEMI) += mscc/
>  obj-$(CONFIG_NET_VENDOR_MOXART) += moxa/
>  obj-$(CONFIG_NET_VENDOR_MYRI) += myricom/
>  obj-$(CONFIG_FEALNX) += fealnx.o
> diff --git a/drivers/net/ethernet/mscc/Kconfig 
> b/drivers/net/ethernet/mscc/Kconfig
> new file mode 100644
> index ..2330de6e7bb6
> --- /dev/null
> +++ b/drivers/net/ethernet/mscc/Kconfig
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +config NET_VENDOR_MICROSEMI
> + bool "Microsemi devices"
> + default y
> + help
> +   If you have a network (Ethernet) card belonging to this class, say Y.
> +
> +   Note that the answer to this question doesn't directly affect the
> +   kernel: saying N will just cause the configurator to skip all
> +   the questions about Microsemi devices.
> +
> +if NET_VENDOR_MICROSEMI
> +
> +config MSCC_MIIM
> + tristate "Microsemi MIIM interface support"
> + depends on HAS_IOMEM
> + select PHYLIB
> + help
> +   This driver supports the MIIM (MDIO) interface found in the network
> +   switches of the Microsemi SoCs
> +
> +endif # NET_VENDOR_MICROSEMI
> diff --git a/drivers/net/ethernet/mscc/Makefile 
> b/drivers/net/ethernet/mscc/Makefile
> new file mode 100644
> index ..4570e8fa4711
> --- /dev/null
> +++ b/drivers/net/ethernet/mscc/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +obj-$(CONFIG_MSCC_MIIM) += mscc_miim.o
> diff --git a/drivers/net/ethernet/mscc/mscc_miim.c 
> b/drivers/net/ethernet/mscc/mscc_miim.c
> new file mode 100644
> index ..95b8d102c90f
> --- /dev/null
> +++ b/drivers/net/ethernet/mscc/mscc_miim.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Driver for the MDIO interface of Microsemi network switches.
> + *
> + * Author: Alexandre Belloni 
> + * Copyright (c) 2017 Microsemi Corporation
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define MSCC_MIIM_REG_STATUS 0x0
> +#define  MSCC_MIIM_STATUS_STAT_BUSY  BIT(3)
> +#define MSCC_MIIM_REG_CMD0x8
> +#define  MSCC_MIIM_CMD_OPR_WRITE BIT(1)
> +#define  MSCC_MIIM_CMD_OPR_READ  BIT(2)
> +#define  MSCC_MIIM_CMD_WRDATA_SHIFT  4
> +#define  MSCC_MIIM_CMD_REGAD_SHIFT   20
> +#define  MSCC_MIIM_CMD_PHYAD_SHIFT   25
> +#define  MSCC_MIIM_CMD_VLD   BIT(31)
> +#define MSCC_MIIM_REG_DATA   0xC
> +#define  MSCC_MIIM_DATA_ERROR(BIT(16) | BIT(17))
> +
> +#define MSCC_PHY_REG_PHY_CFG 0x0
> +#define  PHY_CFG_PHY_ENA (BIT(0) | BIT(1) | BIT(2) | 
> BIT(3))
> +#define  PHY_CFG_PHY_COMMON_RESET BIT(4)
> +#define  PHY_CFG_PHY_RESET   (BIT(5) | BIT(6) | BIT(7) | 
> BIT(8))
> +#define MSCC_PHY_REG_PHY_STATUS  0x4
> +
> +struct mscc_miim_dev {
> + struct mutex lock;
> + void __iomem *regs;
> + void __iomem *phy_regs;
> +};
> +
> +static int mscc_miim_wait_ready(struct mii_bus *bus)
> 

Re: [PATCH net-next 3/8] net: mscc: Add MDIO driver

[Andrew Lunn]

On Fri, Mar 23, 2018 at 09:11:12PM +0100, Alexandre Belloni wrote:
> Add a driver for the Microsemi MII Management controller (MIIM) found on
> Microsemi SoCs.
> On Ocelot, there are two controllers, one is connected to the internal
> PHYs, the other one can communicate with external PHYs.

Hi Alexandre

This looks to be standalone. Such drivers we try to put in
drivers/net/phy.

> +static int mscc_miim_read(struct mii_bus *bus, int mii_id, int regnum)
> +{
> + struct mscc_miim_dev *miim = bus->priv;
> + u32 val;
> + int ret;
> +
> + mutex_lock(>lock);

What are you locking against here?

And you don't appear to initialize the mutex anywhere.

> +static int mscc_miim_reset(struct mii_bus *bus)
> +{
> + struct mscc_miim_dev *miim = bus->priv;
> + int i;
> +
> + if (miim->phy_regs) {
> + writel(0, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> + writel(0x1ff, miim->phy_regs + MSCC_PHY_REG_PHY_CFG);
> + mdelay(500);
> + }
> +
> + for (i = 0; i < PHY_MAX_ADDR; i++) {
> + if (mscc_miim_read(bus, i, MII_PHYSID1) < 0)
> + bus->phy_mask |= BIT(i);
> + }

Why do this? Especially so for the external bus, where the PHYs might
have a GPIO reset line, and won't respond until the gpio is
released. The core code does that just before it scans the bus, or
just before it scans the particular address on the bus, depending on
the scope of the GPIO.

Otherwise, pretty good :-)

   Andrew