| Message ID | 1457693731-6966-4-git-send-email-wxt@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 3/11/2016 1:55 PM, Caesar Wang wrote: > This patch adds to support the emac phy reset. > > 1) phy-reset-gpios: > The phy-reset-gpios is an optional property for arc emac device tree boot. > Change the binding document to match the driver code. > > 2) phy-reset-duration: > Different boards may require different phy reset duration. Add property > phy-reset-duration for device tree probe, so that the boards that need > a longer reset duration can specify it in their device tree. > > 3) phy-reset-active-high: > We need that for a custom hardware that needs the reverse reset sequence. > > Of course, this patch will fix the issue on > https://patchwork.kernel.org/patch/8186801/. > > In some cases, the emac couldn't work if you don't have reset the phy. > Let's add it to happy work. > > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > --- > > drivers/net/ethernet/arc/emac_main.c | 41 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c > index 6446af1..42384f6a 100644 > --- a/drivers/net/ethernet/arc/emac_main.c > +++ b/drivers/net/ethernet/arc/emac_main.c > @@ -764,6 +764,45 @@ static const struct net_device_ops arc_emac_netdev_ops = { > #endif > }; > > +#ifdef CONFIG_OF > +static void emac_reset_phy(struct net_device *pdev) > +{ > + int err, phy_reset; > + bool active_high = false; > + int msec = 10; > + struct device *dev = pdev->dev.parent; > + struct device_node *np = dev->of_node; > + > + if (!np) > + return; > + > + of_property_read_u32(np, "phy-reset-duration", &msec); > + /* A sane reset duration should not be longer than 1s */ > + if (msec > 1000) > + msec = 1; > + > + phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); > + if (!gpio_is_valid(phy_reset)) > + return; > + > + active_high = of_property_read_bool(np, "phy-reset-active-high"); > + > + err = devm_gpio_request_one(dev, phy_reset, active_high ? > + GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW, > + "phy-reset"); > + if (err) { > + dev_err(dev, "failed to get phy-reset-gpios: %d\n", err); > + return; > + } > + msleep(msec); > + gpio_set_value_cansleep(phy_reset, !active_high); > +} [...] Why not make it the mii_bus::reset() method? It gets called before the MDIO bus scan. MBR, Sergei
Hi Caesar, Am Freitag, 11. März 2016, 16:47:59 schrieb Sergei Shtylyov: > On 3/11/2016 1:55 PM, Caesar Wang wrote: > > This patch adds to support the emac phy reset. > > > > 1) phy-reset-gpios: > > The phy-reset-gpios is an optional property for arc emac device tree boot. > > Change the binding document to match the driver code. > > > > 2) phy-reset-duration: > > Different boards may require different phy reset duration. Add property > > phy-reset-duration for device tree probe, so that the boards that need > > a longer reset duration can specify it in their device tree. > > > > 3) phy-reset-active-high: > > We need that for a custom hardware that needs the reverse reset sequence. > > > > Of course, this patch will fix the issue on > > https://patchwork.kernel.org/patch/8186801/. > > > > In some cases, the emac couldn't work if you don't have reset the phy. > > Let's add it to happy work. > > > > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > > --- > > > > drivers/net/ethernet/arc/emac_main.c | 41 > > ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) > > > > diff --git a/drivers/net/ethernet/arc/emac_main.c > > b/drivers/net/ethernet/arc/emac_main.c index 6446af1..42384f6a 100644 > > --- a/drivers/net/ethernet/arc/emac_main.c > > +++ b/drivers/net/ethernet/arc/emac_main.c > > @@ -764,6 +764,45 @@ static const struct net_device_ops > > arc_emac_netdev_ops = {> > > #endif > > }; > > > > +#ifdef CONFIG_OF > > +static void emac_reset_phy(struct net_device *pdev) > > +{ > > + int err, phy_reset; > > + bool active_high = false; > > + int msec = 10; > > + struct device *dev = pdev->dev.parent; > > + struct device_node *np = dev->of_node; > > + > > + if (!np) > > + return; > > + > > + of_property_read_u32(np, "phy-reset-duration", &msec); > > + /* A sane reset duration should not be longer than 1s */ > > + if (msec > 1000) > > + msec = 1; > > + > > + phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); > > + if (!gpio_is_valid(phy_reset)) > > + return; > > + > > + active_high = of_property_read_bool(np, "phy-reset-active-high"); > > + > > + err = devm_gpio_request_one(dev, phy_reset, active_high ? > > + GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW, > > + "phy-reset"); > > + if (err) { > > + dev_err(dev, "failed to get phy-reset-gpios: %d\n", err); > > + return; > > + } > > + msleep(msec); > > + gpio_set_value_cansleep(phy_reset, !active_high); > > +} > > [...] > > Why not make it the mii_bus::reset() method? It gets called before the > MDIO bus scan. while meddling with the emac, I've built some sort of preliminary variant of Sergei's suggestion at [0] - maybe you could take a look for some sort of inspiration ;-) The code is lifted from the designware gmac driver, so the binding also is similar. Heiko [0] https://github.com/mmind/linux-rockchip/commit/a943c588783438ff1c508dfa8c79f1709aa5775e
Am Freitag, 11. März 2016, 15:56:04 schrieb Heiko Stübner: > Hi Caesar, > > Am Freitag, 11. März 2016, 16:47:59 schrieb Sergei Shtylyov: > > On 3/11/2016 1:55 PM, Caesar Wang wrote: > > > This patch adds to support the emac phy reset. > > > > > > 1) phy-reset-gpios: > > > The phy-reset-gpios is an optional property for arc emac device tree > > > boot. > > > Change the binding document to match the driver code. > > > > > > 2) phy-reset-duration: > > > Different boards may require different phy reset duration. Add property > > > phy-reset-duration for device tree probe, so that the boards that need > > > a longer reset duration can specify it in their device tree. > > > > > > 3) phy-reset-active-high: > > > We need that for a custom hardware that needs the reverse reset > > > sequence. > > > > > > Of course, this patch will fix the issue on > > > https://patchwork.kernel.org/patch/8186801/. > > > > > > In some cases, the emac couldn't work if you don't have reset the phy. > > > Let's add it to happy work. > > > > > > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > > > --- > > > > > > drivers/net/ethernet/arc/emac_main.c | 41 > > > ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/arc/emac_main.c > > > b/drivers/net/ethernet/arc/emac_main.c index 6446af1..42384f6a 100644 > > > --- a/drivers/net/ethernet/arc/emac_main.c > > > +++ b/drivers/net/ethernet/arc/emac_main.c > > > @@ -764,6 +764,45 @@ static const struct net_device_ops > > > arc_emac_netdev_ops = {> > > > > > > #endif > > > }; > > > > > > +#ifdef CONFIG_OF > > > +static void emac_reset_phy(struct net_device *pdev) > > > +{ > > > + int err, phy_reset; > > > + bool active_high = false; > > > + int msec = 10; > > > + struct device *dev = pdev->dev.parent; > > > + struct device_node *np = dev->of_node; > > > + > > > + if (!np) > > > + return; > > > + > > > + of_property_read_u32(np, "phy-reset-duration", &msec); > > > + /* A sane reset duration should not be longer than 1s */ > > > + if (msec > 1000) > > > + msec = 1; > > > + > > > + phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); > > > + if (!gpio_is_valid(phy_reset)) > > > + return; > > > + > > > + active_high = of_property_read_bool(np, "phy-reset-active-high"); > > > + > > > + err = devm_gpio_request_one(dev, phy_reset, active_high ? > > > + GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW, > > > + "phy-reset"); > > > + if (err) { > > > + dev_err(dev, "failed to get phy-reset-gpios: %d\n", err); > > > + return; > > > + } > > > + msleep(msec); > > > + gpio_set_value_cansleep(phy_reset, !active_high); > > > +} > > > > [...] > > > > Why not make it the mii_bus::reset() method? It gets called before the > > > > MDIO bus scan. > > while meddling with the emac, I've built some sort of preliminary variant of > Sergei's suggestion at [0] - maybe you could take a look for some sort of > inspiration ;-) forgot to add ... of course the newer gpiod_* infrastructure should be used instead of the old integer-based gpio numbers > > The code is lifted from the designware gmac driver, so the binding also is > similar. > > > Heiko > > > [0] > https://github.com/mmind/linux-rockchip/commit/a943c588783438ff1c508dfa8c79 > f1709aa5775e
On 03/11/2016 01:55 PM, Caesar Wang wrote: > This patch adds to support the emac phy reset. > > 1) phy-reset-gpios: > The phy-reset-gpios is an optional property for arc emac device tree boot. > Change the binding document to match the driver code. The binding document is apparently changed in another patch. Not sure what you wanted to say here... > 2) phy-reset-duration: > Different boards may require different phy reset duration. Add property > phy-reset-duration for device tree probe, so that the boards that need > a longer reset duration can specify it in their device tree. > > 3) phy-reset-active-high: > We need that for a custom hardware that needs the reverse reset sequence. No, we don't really need that, "phy-reset-gpio" prop can contain this data. > Of course, this patch will fix the issue on > https://patchwork.kernel.org/patch/8186801/. > > In some cases, the emac couldn't work if you don't have reset the phy. > Let's add it to happy work. > > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > --- > > drivers/net/ethernet/arc/emac_main.c | 41 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c > index 6446af1..42384f6a 100644 > --- a/drivers/net/ethernet/arc/emac_main.c > +++ b/drivers/net/ethernet/arc/emac_main.c > @@ -764,6 +764,45 @@ static const struct net_device_ops arc_emac_netdev_ops = { > #endif > }; > > +#ifdef CONFIG_OF > +static void emac_reset_phy(struct net_device *pdev) > +{ > + int err, phy_reset; > + bool active_high = false; > + int msec = 10; > + struct device *dev = pdev->dev.parent; > + struct device_node *np = dev->of_node; > + > + if (!np) > + return; > + > + of_property_read_u32(np, "phy-reset-duration", &msec); > + /* A sane reset duration should not be longer than 1s */ > + if (msec > 1000) > + msec = 1; > + > + phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); Forgot to say that these old integer-base GPIO APIs shouldn't be used by the new code, there's new 'struct gpio_desc' based APIs, like devm_gpiod_get() etc. > + if (!gpio_is_valid(phy_reset)) > + return; > + > + active_high = of_property_read_bool(np, "phy-reset-active-high"); Well, I still don't understand why this prop is needed, while the GPIO sense is transparently handled by the gpiolib (at least when using DT). [...] MBR, Sergei
On 03/11/2016 01:55 PM, Caesar Wang wrote: > This patch adds to support the emac phy reset. > > 1) phy-reset-gpios: > The phy-reset-gpios is an optional property for arc emac device tree boot. > Change the binding document to match the driver code. > > 2) phy-reset-duration: > Different boards may require different phy reset duration. Add property > phy-reset-duration for device tree probe, so that the boards that need > a longer reset duration can specify it in their device tree. > > 3) phy-reset-active-high: > We need that for a custom hardware that needs the reverse reset sequence. What concerns me the most about the existing (and suggested) PHY reset related props is that they are located in the MAC device node while not having *anything* to do with the MAC at all! These props actually belong to the PHY nodes, and I'm currently looking into how to handle them there, where they belong... > Of course, this patch will fix the issue on > https://patchwork.kernel.org/patch/8186801/. > > In some cases, the emac couldn't work if you don't have reset the phy. > Let's add it to happy work. > > Signed-off-by: Caesar Wang <wxt@rock-chips.com> [...] MBR, Sergei
Hi Sergei, ? 2016?03?12? 02:35, Sergei Shtylyov ??: > On 03/11/2016 01:55 PM, Caesar Wang wrote: > >> This patch adds to support the emac phy reset. >> >> 1) phy-reset-gpios: >> The phy-reset-gpios is an optional property for arc emac device tree >> boot. >> Change the binding document to match the driver code. > > The binding document is apparently changed in another patch. Not > sure what you wanted to say here... > >> 2) phy-reset-duration: >> Different boards may require different phy reset duration. Add property >> phy-reset-duration for device tree probe, so that the boards that need >> a longer reset duration can specify it in their device tree. >> >> 3) phy-reset-active-high: >> We need that for a custom hardware that needs the reverse reset >> sequence. > > No, we don't really need that, "phy-reset-gpio" prop can contain > this data. > >> Of course, this patch will fix the issue on >> https://patchwork.kernel.org/patch/8186801/. >> >> In some cases, the emac couldn't work if you don't have reset the phy. >> Let's add it to happy work. >> >> Signed-off-by: Caesar Wang <wxt@rock-chips.com> >> --- >> >> drivers/net/ethernet/arc/emac_main.c | 41 >> ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 41 insertions(+) >> >> diff --git a/drivers/net/ethernet/arc/emac_main.c >> b/drivers/net/ethernet/arc/emac_main.c >> index 6446af1..42384f6a 100644 >> --- a/drivers/net/ethernet/arc/emac_main.c >> +++ b/drivers/net/ethernet/arc/emac_main.c >> @@ -764,6 +764,45 @@ static const struct net_device_ops >> arc_emac_netdev_ops = { >> #endif >> }; >> >> +#ifdef CONFIG_OF >> +static void emac_reset_phy(struct net_device *pdev) >> +{ >> + int err, phy_reset; >> + bool active_high = false; >> + int msec = 10; >> + struct device *dev = pdev->dev.parent; >> + struct device_node *np = dev->of_node; >> + >> + if (!np) >> + return; >> + >> + of_property_read_u32(np, "phy-reset-duration", &msec); >> + /* A sane reset duration should not be longer than 1s */ >> + if (msec > 1000) >> + msec = 1; >> + >> + phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); > > Forgot to say that these old integer-base GPIO APIs shouldn't be > used by the new code, there's new 'struct gpio_desc' based APIs, like > devm_gpiod_get() etc. > >> + if (!gpio_is_valid(phy_reset)) >> + return; >> + >> + active_high = of_property_read_bool(np, "phy-reset-active-high"); > > Well, I still don't understand why this prop is needed, while the > GPIO sense is transparently handled by the gpiolib (at least when > using DT). Okay, I have to admit that reference the exist way to do... wxt@nb:~/kernel/drivers/net/ethernet$ ag reset-gpios micrel/ks8851.c 1427: gpio = of_get_named_gpio_flags(spi->dev.of_node, "reset-gpios", arc/emac_main.c 787: phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); 797: dev_err(dev, "failed to get phy-reset-gpios: %d\n", err); arc/emac_main.c~ 784: phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); 794: dev_err(dev, "failed to get phy-reset-gpios: %d\n", err); davicom/dm9000.c 1451: reset_gpios = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 0, freescale/fec_main.c 3206: phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); 3216: dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err); cadence/macb.c 2958: int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); ... Yep, the gpiolib that can include the prop to work with the same ways. > > [...] > > MBR, Sergei > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff --git a/drivers/net/ethernet/arc/emac_main.c b/drivers/net/ethernet/arc/emac_main.c index 6446af1..42384f6a 100644 --- a/drivers/net/ethernet/arc/emac_main.c +++ b/drivers/net/ethernet/arc/emac_main.c @@ -764,6 +764,45 @@ static const struct net_device_ops arc_emac_netdev_ops = { #endif }; +#ifdef CONFIG_OF +static void emac_reset_phy(struct net_device *pdev) +{ + int err, phy_reset; + bool active_high = false; + int msec = 10; + struct device *dev = pdev->dev.parent; + struct device_node *np = dev->of_node; + + if (!np) + return; + + of_property_read_u32(np, "phy-reset-duration", &msec); + /* A sane reset duration should not be longer than 1s */ + if (msec > 1000) + msec = 1; + + phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); + if (!gpio_is_valid(phy_reset)) + return; + + active_high = of_property_read_bool(np, "phy-reset-active-high"); + + err = devm_gpio_request_one(dev, phy_reset, active_high ? + GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW, + "phy-reset"); + if (err) { + dev_err(dev, "failed to get phy-reset-gpios: %d\n", err); + return; + } + msleep(msec); + gpio_set_value_cansleep(phy_reset, !active_high); +} +#else +static void emac_reset_phy(struct platform_device *pdev) +{ +} +#endif /* CONFIG_OF */ + int arc_emac_probe(struct net_device *ndev, int interface) { struct device *dev = ndev->dev.parent; @@ -803,6 +842,8 @@ int arc_emac_probe(struct net_device *ndev, int interface) /* FIXME :: no multicast support yet */ ndev->flags &= ~IFF_MULTICAST; + emac_reset_phy(ndev); + priv = netdev_priv(ndev); priv->dev = dev;
This patch adds to support the emac phy reset. 1) phy-reset-gpios: The phy-reset-gpios is an optional property for arc emac device tree boot. Change the binding document to match the driver code. 2) phy-reset-duration: Different boards may require different phy reset duration. Add property phy-reset-duration for device tree probe, so that the boards that need a longer reset duration can specify it in their device tree. 3) phy-reset-active-high: We need that for a custom hardware that needs the reverse reset sequence. Of course, this patch will fix the issue on https://patchwork.kernel.org/patch/8186801/. In some cases, the emac couldn't work if you don't have reset the phy. Let's add it to happy work. Signed-off-by: Caesar Wang <wxt@rock-chips.com> --- drivers/net/ethernet/arc/emac_main.c | 41 ++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)