Hello. On 05/12/2016 09:42 PM, Uwe Kleine-König wrote:
[we already talked about this patch in #armlinux, I'm now just forwarding my comments on the list. Background was that I sent an easier and less complete patch with the same idea. See http://patchwork.ozlabs.org/patch/621418/] [added Linus Walleij to Cc, there is a question for you/him below] On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:--- net-next.orig/Documentation/devicetree/bindings/net/phy.txt +++ net-next/Documentation/devicetree/bindings/net/phy.txt @@ -35,6 +35,8 @@ Optional Properties: - broken-turn-around: If set, indicates the PHY device does not correctly release the turn around line low at the end of a MDIO transaction. +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal. + Example: ethernet-phy@0 {This is great.Index: net-next/drivers/net/phy/at803x.c =================================================================== --- net-next.orig/drivers/net/phy/at803x.c +++ net-next/drivers/net/phy/at803x.c @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL"); [...]My patch breaks this driver. I wasn't aware of it.
I tried to be as careful as I could but still it looks that I didn't succeed at that too...
[...]
Index: net-next/drivers/net/phy/mdio_device.c =================================================================== --- net-next.orig/drivers/net/phy/mdio_device.c +++ net-next/drivers/net/phy/mdio_device.c
[...]
@@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev struct mdio_driver *mdiodrv = to_mdio_driver(drv); int err = 0; - if (mdiodrv->probe) + if (mdiodrv->probe) { + /* Deassert the reset signal */ + mdio_device_reset(mdiodev, 0); + err = mdiodrv->probe(mdiodev); + /* Assert the reset signal */ + mdio_device_reset(mdiodev, 1);I wonder if it's safe to do this in general. What if ->probe does something with the phy that is lost by resetting but that is relied on later?
Well, I thought that config_init() method is designed for that but indeed the LXT driver writes to BMCR in its probe() method and hence is broken. Thank you for noticing...
[...]
Index: net-next/drivers/of/of_mdio.c =================================================================== --- net-next.orig/drivers/of/of_mdio.c +++ net-next/drivers/of/of_mdio.c @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n static void of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child, u32 addr) { + struct gpio_desc *gpiod; struct phy_device *phy; bool is_c45; int rc; @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru is_c45 = of_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"); + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios"); + /* Deassert the reset signal */ + if (!IS_ERR(gpiod)) + gpiod_direction_output(gpiod, 0);This is wrong I think. You must only ignore -ENODEV, all other error
At least -ENOSYS should also be ignored (it's returned when gpiolib is not configured), right? When does -ENODEV gets returned (it's not easy to follow)?
codes should be passed to the caller.
The caller doesn't care anyway...
(I see that's not trivial because of_mdiobus_register_phy returns void.)
I've made this function *void* in net-next.
In my patch I used devm_gpiod_get_array which has the nice property that I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime of the gpio to the device which is nice and IMHO the right direction for the phylib (i.e. better embracing of the device model). This cannot be used here easily however because there is no struct device yet and this is only created after the phy id is determined.
Your last patch [1] didn't make use of the managed device API (devm) either, I didn't quite get to the bottom of that...
The phy id is either read from the device tree or must be read from the phy which might fail if reset is not deasserted.
Principally there is no reason however that the phy_id must be known before the struct device is created however.
It's just that the code is cleaner that way... [1] http://paste.debian.net/683630/
Best regards Uwe
MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
