| Message ID | 20171205132600.13796-2-dev@g0hl1n.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: fec: fix refclk enable for SMSC LAN8710/20 | expand |
Hi Richard, On Tue, Dec 5, 2017 at 2:25 PM, Richard Leitner <dev@g0hl1n.net> wrote: > From: Richard Leitner <richard.leitner@skidata.com> > > Some PHYs need a minimum time after the reset gpio was asserted and/or > deasserted. To ensure we meet these timing requirements add two new > optional devicetree parameters for the phy: reset-delay-us and > reset-post-delay-us. Thanks for your patch! > This patch depends on the "phylib: Add device reset GPIO support" patch > submitted by Geert Uytterhoeven/Sergei Shtylyov, see: > https://patchwork.kernel.org/patch/10090149/ The above paragraph belongs under the "---" line below, as it is not intended to be preserved in the eternal git history. > Signed-off-by: Richard Leitner <richard.leitner@skidata.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Although I have a few suggestions below: > --- a/drivers/net/phy/mdio_device.c > +++ b/drivers/net/phy/mdio_device.c > @@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove); > > void mdio_device_reset(struct mdio_device *mdiodev, int value) > { > - if (mdiodev->reset) > - gpiod_set_value(mdiodev->reset, value); > + if (!mdiodev->reset) > + return; > + > + gpiod_set_value(mdiodev->reset, value); > + > + if (value && mdiodev->reset_delay) > + usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + 100); > + else if (!value && mdiodev->reset_post_delay) > + usleep_range(mdiodev->reset_post_delay, > + mdiodev->reset_post_delay + 100); I think this can be written simpler using e.g.: unsigned int delay; ... delay = value ? mdiodev->reset_delay : mdiodev->reset_post_delay; if (delay) usleep_range(delay, delay + 100); Perhaps the range extension should be relative, e.g. "delay + min(delay / 10, 100)"? > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, > if (of_property_read_bool(child, "broken-turn-around")) > mdio->phy_ignore_ta_mask |= 1 << addr; > > + if (of_property_read_u32(child, "reset-delay-us", > + &phy->mdio.reset_delay)) > + phy->mdio.reset_delay = 0; > + > + if (of_property_read_u32(child, "reset-post-delay-us", > + &phy->mdio.reset_post_delay)) > + phy->mdio.reset_post_delay = 0; If of_property_read_u32() fails, it doesn't write to its output parameter. As the structure should be zeroed during allocation, you can just write: of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay); of_property_read_u32(child, "reset-post-delay-us", &phy->mdio.reset_post_delay); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On 12/05/2017 02:54 PM, Geert Uytterhoeven wrote: > Hi Richard, > > On Tue, Dec 5, 2017 at 2:25 PM, Richard Leitner <dev@g0hl1n.net> wrote: >> From: Richard Leitner <richard.leitner@skidata.com> >> >> Some PHYs need a minimum time after the reset gpio was asserted and/or >> deasserted. To ensure we meet these timing requirements add two new >> optional devicetree parameters for the phy: reset-delay-us and >> reset-post-delay-us. > > Thanks for your patch! > >> This patch depends on the "phylib: Add device reset GPIO support" patch >> submitted by Geert Uytterhoeven/Sergei Shtylyov, see: >> https://patchwork.kernel.org/patch/10090149/ > > The above paragraph belongs under the "---" line below, as it is not intended > to be preserved in the eternal git history. Ok. Thanks. That makes sense. I'll take it into account for v4. > >> Signed-off-by: Richard Leitner <richard.leitner@skidata.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Although I have a few suggestions below: Thank you for your review and suggestions (they make the code look way more neater). I'll adapt v4 accordingly. > >> --- a/drivers/net/phy/mdio_device.c >> +++ b/drivers/net/phy/mdio_device.c >> @@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove); >> >> void mdio_device_reset(struct mdio_device *mdiodev, int value) >> { >> - if (mdiodev->reset) >> - gpiod_set_value(mdiodev->reset, value); >> + if (!mdiodev->reset) >> + return; >> + >> + gpiod_set_value(mdiodev->reset, value); >> + >> + if (value && mdiodev->reset_delay) >> + usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + 100); >> + else if (!value && mdiodev->reset_post_delay) >> + usleep_range(mdiodev->reset_post_delay, >> + mdiodev->reset_post_delay + 100); > > I think this can be written simpler using e.g.: > > unsigned int delay; > > ... > delay = value ? mdiodev->reset_delay : mdiodev->reset_post_delay; > if (delay) > usleep_range(delay, delay + 100); > > Perhaps the range extension should be relative, e.g. > "delay + min(delay / 10, 100)"? > >> --- a/drivers/of/of_mdio.c >> +++ b/drivers/of/of_mdio.c >> @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, >> if (of_property_read_bool(child, "broken-turn-around")) >> mdio->phy_ignore_ta_mask |= 1 << addr; >> >> + if (of_property_read_u32(child, "reset-delay-us", >> + &phy->mdio.reset_delay)) >> + phy->mdio.reset_delay = 0; >> + >> + if (of_property_read_u32(child, "reset-post-delay-us", >> + &phy->mdio.reset_post_delay)) >> + phy->mdio.reset_post_delay = 0; > > If of_property_read_u32() fails, it doesn't write to its output parameter. > As the structure should be zeroed during allocation, you can just write: > > of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay); > of_property_read_u32(child, "reset-post-delay-us", > &phy->mdio.reset_post_delay);
Hi Richard > +++ b/drivers/of/of_mdio.c > @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, > if (of_property_read_bool(child, "broken-turn-around")) > mdio->phy_ignore_ta_mask |= 1 << addr; > > + if (of_property_read_u32(child, "reset-delay-us", > + &phy->mdio.reset_delay)) > + phy->mdio.reset_delay = 0; > + > + if (of_property_read_u32(child, "reset-post-delay-us", > + &phy->mdio.reset_post_delay)) > + phy->mdio.reset_post_delay = 0; of_property_read_u32() should not change the variable you pass to it, if it does not find the property. So you can change this to: phy->mdio.reset_delay = 0; phy->mdio.reset_post_delay = 0; of_property_read_u32(child, "reset-delay-us", &phy->mdio.reset_delay); of_property_read_u32(child, "reset-post-delay-us", &phy->mdio.reset_post_delay); Andrew
Hi Andrew, On 12/05/2017 06:28 PM, Andrew Lunn wrote: > Hi Richard > >> +++ b/drivers/of/of_mdio.c >> @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, >> if (of_property_read_bool(child, "broken-turn-around")) >> mdio->phy_ignore_ta_mask |= 1 << addr; >> >> + if (of_property_read_u32(child, "reset-delay-us", >> + &phy->mdio.reset_delay)) >> + phy->mdio.reset_delay = 0; >> + >> + if (of_property_read_u32(child, "reset-post-delay-us", >> + &phy->mdio.reset_post_delay)) >> + phy->mdio.reset_post_delay = 0; > > of_property_read_u32() should not change the variable you pass to it, > if it does not find the property. So you can change this to: > > phy->mdio.reset_delay = 0; > phy->mdio.reset_post_delay = 0; > > of_property_read_u32(child, "reset-delay-us", > &phy->mdio.reset_delay); > > of_property_read_u32(child, "reset-post-delay-us", > &phy->mdio.reset_post_delay); Geert already pointed this out, but he said it's possible to omit also the zeroing of the variables. > On 12/05/2017 02:54 PM, Geert Uytterhoeven wrote: >> If of_property_read_u32() fails, it doesn't write to its output >> parameter. >> As the structure should be zeroed during allocation, you can just >> write: >> >> of_property_read_u32(child, "reset-delay-us",&phy->mdio.reset_delay); >> of_property_read_u32(child, "reset-post-delay-us", >> &phy->mdio.reset_post_delay); If that's ok I'll take the shorter (Geerts) suggestion for v4. Nonetheless thanks for your quick feedback! regards;Richard.L
diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt index c05479f5ac7c..72860ce7f610 100644 --- a/Documentation/devicetree/bindings/net/phy.txt +++ b/Documentation/devicetree/bindings/net/phy.txt @@ -55,6 +55,12 @@ Optional Properties: - reset-gpios: The GPIO phandle and specifier for the PHY reset signal. +- reset-delay-us: Delay after the reset was asserted in microseconds. + If this property is missing the delay will be skipped. + +- reset-post-delay-us: Delay after the reset was deasserted in microseconds. + If this property is missing the delay will be skipped. + Example: ethernet-phy@0 { @@ -62,4 +68,8 @@ ethernet-phy@0 { interrupt-parent = <&PIC>; interrupts = <35 IRQ_TYPE_EDGE_RISING>; reg = <0>; + + reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>; + reset-delay-us = <1000>; + reset-post-delay-us = <2000>; }; diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c index 75d97dd9fb28..ca3ff43f8ee8 100644 --- a/drivers/net/phy/mdio_device.c +++ b/drivers/net/phy/mdio_device.c @@ -24,6 +24,7 @@ #include <linux/slab.h> #include <linux/string.h> #include <linux/unistd.h> +#include <linux/delay.h> void mdio_device_free(struct mdio_device *mdiodev) { @@ -118,8 +119,16 @@ EXPORT_SYMBOL(mdio_device_remove); void mdio_device_reset(struct mdio_device *mdiodev, int value) { - if (mdiodev->reset) - gpiod_set_value(mdiodev->reset, value); + if (!mdiodev->reset) + return; + + gpiod_set_value(mdiodev->reset, value); + + if (value && mdiodev->reset_delay) + usleep_range(mdiodev->reset_delay, mdiodev->reset_delay + 100); + else if (!value && mdiodev->reset_post_delay) + usleep_range(mdiodev->reset_post_delay, + mdiodev->reset_post_delay + 100); } EXPORT_SYMBOL(mdio_device_reset); diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index 98258583abb0..fb56486dfaa0 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -77,6 +77,14 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, if (of_property_read_bool(child, "broken-turn-around")) mdio->phy_ignore_ta_mask |= 1 << addr; + if (of_property_read_u32(child, "reset-delay-us", + &phy->mdio.reset_delay)) + phy->mdio.reset_delay = 0; + + if (of_property_read_u32(child, "reset-post-delay-us", + &phy->mdio.reset_post_delay)) + phy->mdio.reset_post_delay = 0; + /* Associate the OF node with the device structure so it * can be looked up later */ of_node_get(child); diff --git a/include/linux/mdio.h b/include/linux/mdio.h index 92d4e55ffe67..e37c21d8eb19 100644 --- a/include/linux/mdio.h +++ b/include/linux/mdio.h @@ -41,6 +41,8 @@ struct mdio_device { int addr; int flags; struct gpio_desc *reset; + unsigned int reset_delay; + unsigned int reset_post_delay; }; #define to_mdio_device(d) container_of(d, struct mdio_device, dev)