Re: [PATCH v3 2/3] phylib: add support for reset-gpios

[Sam Ravnborg <sam@ravnborg.org>]

Hi Sasha.

To Andrey - please read comment on mdio support in macb driver.

> > +	/* register PHY's as child node to the ethernet node */
> > +	if (bus->parent->device_node)
> > +		of_mdiobus_register(bus, bus->parent->device_node);
> 
> I am not so convinced of this change. Here you assume that bus->parent
> is the ethernet node, but this doesn't necessarily have to be the case
> as some mdio controllers are standalone and not part of an ethernet
> device (see "virtual,mdio-gpio" to get examples).
> 
> So instead of introducing this change I would prefer if you let the code
> below trigger:
> 
> > +
> > +	/* register PHY's as child node to mdio node */
> >  	if (bus->dev.device_node)
> >  		of_mdiobus_register(bus, bus->dev.device_node);
> 
> This means you have to set bus->dev.device_node in your ethernet driver
> like some drivers already do:
> 
> drivers/net/macb.c:666:                 macb->miibus.dev.device_node = mdiobus;

I have analyzed on the current situation.

In barebox we have two drivers that set miibus.dev.device_node,
both in the case where they have found a child named "mdio".
-> fec-imx.c and macb.c
When reading dts/Bindings/net/fsl-fec.txt then this binding specify
an optional mdio child, so fec-imx.c is fine.
But macb.txt or the general bindigns do not mention
a mdio child.
And the Linux macb_main.c do not support a "mdio" node either.
So my best guess is that the "mdio" support in macb was accidently
ported over from fec-imx when adding DT support.

If we only consider the DT cases we will call mdio_register()
in the following cases:

1) Typical from an ethernet driver:
bus->dev.device_node is NULL
bus->parent->device_node is the ethernet node

2) fec-imx case:
bus->dev.device_node is the "mdio" node, that agin hold the PHY nodes
bus->parent->device_node is the ethernet node

3) mdio drivers:
bus->dev.device_node is the "mdio" node
bus->parent->device_node is equal to bus->dev.device_node


Considering the above I will do the following:
a) Remove "mdio" support from macb.c
b) Modify my patch to something like this:

	if (bus->dev.device_node)
		of_mdiobus_register(bus, bus->dev.device_node);
	else if (bus->parent->device_node)
		of_mdiobus_register(bus, bus->parent->device_node);

With the above I have covered point 1 to 3 above in a simple way.
And this looks a simple and straghtforward way to do it.

I also looked at how the kernel does thing.
In of_mdiobus_register it has:

	for_each_child() {
		if (of_node_is_phy(child))
			of_mdiobus_register_phy()
		else
			of_mdiobus_register_device()
	}

I so far failed to follow where the code will iterate over
the PHY nodes inside the mdio node (that trigger the
of_mdiobus_register_device() call).
So therefore I am reluctant to try to implment this,
and the suggested solution is also more in the line
with a simpler and straightforward approach preferred in barebox.

I no-one else objects and I do not get any better ideas I will
implement and test this tomorrow.

	Sam

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox