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:

quoted

On Fri, Mar 23, 2018 at 09:11:12PM +0100, Alexandre Belloni wrote:

quoted

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.

quoted

+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(&miim->lock);

What are you locking against here?

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

quoted

+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
0xffff 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 0xffffffff and return. The scan code
should then do the correct thing.

       Andrew