Re: [RFC 2/2] net: emac: add support for device-tree based PHY discovery and setup

Le 02/05/17 à 14:25, Christian Lamparter a écrit :

From: Christian Lamparter <redacted>

This patch adds glue-code that allows the EMAC driver to interface
with the existing dt-supported PHYs in drivers/net/phy.

Because currently, the emac driver maintains a small library of
supported phys for in a private phy.c file located in the drivers
directory.

The support is limited to mostly single ethernet transceiver like the:
CIS8201, BCM5248, ET1011C, Marvell 88E1111 and 88E1112, AR8035.
However, routers like the Netgear WNDR4700 and Cisco Meraki MX60(W)
have a 5-port switch (QCA8327N) attached to the MDIO of the EMAC.
The switch chip has already a proper phy-driver (qca8k) that uses
the generic phy library.

Technically, it's a mdio_device in the upstream kernel that registers a
switch with DSA (and a PHY device in the OpenWrt/LEDE downstream
kernel). If your goal is to specifically support that device you should
consider making the EMAC interface with a fixed link PHY to properly
initialize the EMAC <=> CPU port of the switch link, and then declare
the qca8k device as a child MDIO device (not a PHY), similar to what is
done in arch/arm/boot/dts/vf610-zii-dev-rev-b.dts for instance.

PHYLIB already executes grabbing the phy device's mutex, is this really
needed here?

+}
+
+static int emac_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
+{
+	return emac_mdio_read(bus->priv, addr, regnum);
+}
+
+static int emac_mii_bus_write(struct mii_bus *bus, int addr, int regnum,
+			      u16 val)
+{
+	emac_mdio_write(bus->priv, addr, regnum, val);
+	return 0;
+}
+
+static int emac_mii_bus_reset(struct mii_bus *bus)
+{
+	struct emac_instance *dev = netdev_priv(bus->priv);
+
+	emac_mii_reset_phy(&dev->phy);

This seems wrong, emac_mii_reset_phy() does a BMCR software reset, which
PHYLIB is already going to do (phy_init_hw), yet you do this here at the
MDIO bus level towards a specify PHY, whereas this should be affecting
the MDIO bus itself (and/or *all* PHY child devices for quirks).

+	return 0;
+}
+
+static int emac_mdio_probe(struct emac_instance *dev)
+{
+	struct device_node *mii_np;
+	struct mii_bus *bus;
+	int res;
+
+	bus = mdiobus_alloc();
+	if (!bus)
+		return -ENOMEM;
+
+	mii_np = of_get_child_by_name(dev->ofdev->dev.of_node, "mdio");
+	if (!mii_np) {
+		dev_err(&dev->ndev->dev, "no mdio definition found.");
+		return -ENODEV;
+	}
+
+	if (!of_device_is_available(mii_np))
+		return 0;
+
+	bus->priv = dev->ndev;
+	bus->parent = dev->ndev->dev.parent;
+	bus->name = "emac_mdio";
+	bus->read = &emac_mii_bus_read;
+	bus->write = &emac_mii_bus_write;
+	bus->reset = &emac_mii_bus_reset;
+
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", bus->name);

You should pick a more unique name here, if you ever have a second
instance it would just clash with the previous one.

+
+	res = of_mdiobus_register(bus, mii_np);
+	if (res) {
+		dev_err(&dev->ndev->dev, "cannot register MDIO bus %s\n",
+			bus->name);
+		mdiobus_free(bus);
+	}
+
+	dev->mii_bus = bus;
+	return res;
+}
+
+static void emac_mdio_cleanup(struct emac_instance *dev)
+{
+	if (dev->mii_bus) {
+		if (dev->mii_bus->state == MDIOBUS_REGISTERED)
+			mdiobus_unregister(dev->mii_bus);

If you need to make that kind of check, why not separate how the mdio
bus structure's lifecycle is managed? This seems to be avoiding to hit
the BUG_ON() in mdiobus_unregister..

+		mdiobus_free(dev->mii_bus);
+		dev->mii_bus = NULL;
+		kfree(dev->phy.def);
+	}
+}
+
+static int stub_setup_aneg(struct mii_phy *phy, u32 advertise)
+{
+	return 0;
+}
+
+static int stub_setup_forced(struct mii_phy *phy, int speed, int fd)
+{
+	return 0;
+}
+
+static int stub_poll_link(struct mii_phy *phy)
+{
+	struct net_device *ndev = phy->dev;
+	struct emac_instance *dev = netdev_priv(ndev);
+
+	return dev->opened;
+}
+
+static int stub_read_link(struct mii_phy *phy)
+{
+	struct net_device *ndev = phy->dev;
+	struct emac_instance *dev = netdev_priv(ndev);
+
+	phy_start(dev->phy_dev);

Are you sure the read_link function is supposed to start the PHY state
machine? Either the name is confusing, or it's not the right thing to do
here.

+	return 0;
+}
+
+static const struct mii_phy_ops emac_stub_phy_ops = {
+	.setup_aneg	= stub_setup_aneg,
+	.setup_forced	= stub_setup_forced,
+	.poll_link	= stub_poll_link,
+	.read_link	= stub_read_link,
+};
+
+static int emac_probe_dt_phy(struct emac_instance *dev)
+{
+	struct device_node *np = dev->ofdev->dev.of_node;
+	struct device_node *phy_handle;
+	struct net_device *ndev = dev->ndev;
+	int res;
+
+	phy_handle = of_parse_phandle(np, "phy-handle", 0);
+
+	if (phy_handle) {
+		res = emac_mdio_probe(dev);
+		if (res)
+			goto err_cleanup;
+
+		dev->phy.def = kzalloc(sizeof(*dev->phy.def), GFP_KERNEL);
+		if (!dev->phy.def) {
+			res = -ENOMEM;
+			goto err_cleanup;
+		}
+
+		dev->phy_dev = of_phy_connect(ndev, phy_handle,
+					      &emac_adjust_link, 0,
+					      PHY_INTERFACE_MODE_RGMII);

You should call of_get_phy_mode() since there should be a proper
"phy-mode" or "phy-connection-type" property describing how it's
connected to the EMAC.

Why is this limited to EMAC_FTR_HAS_RGMII here?

-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html