| Message ID | 1450290690-32369-1-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 16, 2015 at 07:31:30PM +0100, Gregory CLEMENT wrote: > +Optional properties for PHY child node: > +- reset-gpios : Should specify the gpio for phy reset reset-gpios plural or reset-gpio singular? > + > Examples: > > macb0: ethernet@fffc4000 { > @@ -29,4 +32,8 @@ Examples: > local-mac-address = [3a 0e 03 04 05 06]; > clock-names = "pclk", "hclk", "tx_clk"; > clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>; > + ethernet-phy@1 { > + reg = <0x1>; > + reset-gpios = <&pioE 6 1>; Did you mean "gpioE" ? Thanks, Richard
Hi Richard, On mer., déc. 16 2015, Richard Cochran <richardcochran@gmail.com> wrote: > On Wed, Dec 16, 2015 at 07:31:30PM +0100, Gregory CLEMENT wrote: >> +Optional properties for PHY child node: >> +- reset-gpios : Should specify the gpio for phy reset > > reset-gpios plural or reset-gpio singular? The bindings name is plural but it handle only one gpio. See: https://lkml.org/lkml/2015/12/9/623 > >> + >> Examples: >> >> macb0: ethernet@fffc4000 { >> @@ -29,4 +32,8 @@ Examples: >> local-mac-address = [3a 0e 03 04 05 06]; >> clock-names = "pclk", "hclk", "tx_clk"; >> clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>; >> + ethernet-phy@1 { >> + reg = <0x1>; >> + reset-gpios = <&pioE 6 1>; > > Did you mean "gpioE" ? No, I really mean pioE: it is the name used for the at91 SoCs. Thanks, Gregory
On Wednesday 16 December 2015 19:31:30 Gregory CLEMENT wrote: > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c > index 88c1e1a..35661aa 100644 > --- a/drivers/net/ethernet/cadence/macb.c > +++ b/drivers/net/ethernet/cadence/macb.c > @@ -28,6 +28,7 @@ > #include <linux/phy.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/of_gpio.h> > #include <linux/of_mdio.h> > #include <linux/of_net.h> Is this the patch that is already in linux-next? I needed an additional #include <linux/gpio/consumer.h> to avoid this build error on randconfig builds without GPIOLIB: drivers/net/ethernet/cadence/macb.c: In function 'macb_probe': drivers/net/ethernet/cadence/macb.c:2908:19: error: implicit declaration of function 'devm_gpiod_get_optional' [-Werror=implicit-function-declaration] bp->reset_gpio = devm_gpiod_get_optional(&bp->pdev->dev, "phy-reset", ^ drivers/net/ethernet/cadence/macb.c:2909:8: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function) GPIOD_OUT_HIGH); ^ drivers/net/ethernet/cadence/macb.c:2909:8: note: each undeclared identifier is reported only once for each function it appears in drivers/net/ethernet/cadence/macb.c: In function 'macb_remove': drivers/net/ethernet/cadence/macb.c:2979:3: error: implicit declaration of function 'gpiod_set_value' [-Werror=implicit-function-declaration] Arnd
Hi Arnd, On mer., déc. 16 2015, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 16 December 2015 19:31:30 Gregory CLEMENT wrote: >> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c >> index 88c1e1a..35661aa 100644 >> --- a/drivers/net/ethernet/cadence/macb.c >> +++ b/drivers/net/ethernet/cadence/macb.c >> @@ -28,6 +28,7 @@ >> #include <linux/phy.h> >> #include <linux/of.h> >> #include <linux/of_device.h> >> +#include <linux/of_gpio.h> >> #include <linux/of_mdio.h> >> #include <linux/of_net.h> > > Is this the patch that is already in linux-next? I've just checked and it is the v2 which is in linux-next. David applied it on Monday in his next branche but I was not aware of it. David, if I remebered well you do not remove patch from yout branch. So would you agree to take a follow-up patch on top of 5833e0526820 "net/macb: add support for resetting PHY using GPIO" ? I will fix the error found by Arnd and also use a better device tree binding (more future proof). Thanks, Gregory > > I needed an additional > > #include <linux/gpio/consumer.h> > > to avoid this build error on randconfig builds without GPIOLIB: > > drivers/net/ethernet/cadence/macb.c: In function 'macb_probe': > drivers/net/ethernet/cadence/macb.c:2908:19: error: implicit declaration of function 'devm_gpiod_get_optional' [-Werror=implicit-function-declaration] > bp->reset_gpio = devm_gpiod_get_optional(&bp->pdev->dev, "phy-reset", > ^ > drivers/net/ethernet/cadence/macb.c:2909:8: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function) > GPIOD_OUT_HIGH); > ^ > drivers/net/ethernet/cadence/macb.c:2909:8: note: each undeclared identifier is reported only once for each function it appears in > drivers/net/ethernet/cadence/macb.c: In function 'macb_remove': > drivers/net/ethernet/cadence/macb.c:2979:3: error: implicit declaration of function 'gpiod_set_value' [-Werror=implicit-function-declaration] > > > Arnd
From: Gregory CLEMENT <gregory.clement@free-electrons.com> Date: Thu, 17 Dec 2015 09:39:32 +0100 > if I remebered well you do not remove patch from yout branch. So would > you agree to take a follow-up patch on top of 5833e0526820 "net/macb: > add support for resetting PHY using GPIO" ? Yes.
diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt index b5d7976..38c8e84 100644 --- a/Documentation/devicetree/bindings/net/macb.txt +++ b/Documentation/devicetree/bindings/net/macb.txt @@ -19,6 +19,9 @@ Required properties: Optional elements: 'tx_clk' - clocks: Phandles to input clocks. +Optional properties for PHY child node: +- reset-gpios : Should specify the gpio for phy reset + Examples: macb0: ethernet@fffc4000 { @@ -29,4 +32,8 @@ Examples: local-mac-address = [3a 0e 03 04 05 06]; clock-names = "pclk", "hclk", "tx_clk"; clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>; + ethernet-phy@1 { + reg = <0x1>; + reset-gpios = <&pioE 6 1>; + }; }; diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c index 88c1e1a..35661aa 100644 --- a/drivers/net/ethernet/cadence/macb.c +++ b/drivers/net/ethernet/cadence/macb.c @@ -28,6 +28,7 @@ #include <linux/phy.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/of_gpio.h> #include <linux/of_mdio.h> #include <linux/of_net.h> @@ -2813,6 +2814,7 @@ static int macb_probe(struct platform_device *pdev) = macb_clk_init; int (*init)(struct platform_device *) = macb_init; struct device_node *np = pdev->dev.of_node; + struct device_node *phy_node; const struct macb_config *macb_config = NULL; struct clk *pclk, *hclk, *tx_clk; unsigned int queue_mask, num_queues; @@ -2900,6 +2902,16 @@ static int macb_probe(struct platform_device *pdev) else macb_get_hwaddr(bp); + /* Power up the PHY if there is a GPIO reset */ + phy_node = of_get_next_available_child(np, NULL); + if (phy_node) { + int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); + if (gpio_is_valid(gpio)) + bp->reset_gpio = gpio_to_desc(gpio); + gpiod_set_value(bp->reset_gpio, GPIOD_OUT_HIGH); + } + of_node_put(phy_node); + err = of_get_phy_mode(np); if (err < 0) { pdata = dev_get_platdata(&pdev->dev); @@ -2966,6 +2978,10 @@ static int macb_remove(struct platform_device *pdev) mdiobus_unregister(bp->mii_bus); kfree(bp->mii_bus->irq); mdiobus_free(bp->mii_bus); + + /* Shutdown the PHY if there is a GPIO reset */ + gpiod_set_value(bp->reset_gpio, GPIOD_OUT_LOW); + unregister_netdev(dev); clk_disable_unprepare(bp->tx_clk); clk_disable_unprepare(bp->hclk); diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index 6e1faea..b6ec421 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -824,6 +824,7 @@ struct macb { unsigned int dma_burst_length; phy_interface_t phy_interface; + struct gpio_desc *reset_gpio; /* AT91RM9200 transmit */ struct sk_buff *skb; /* holds skb until xmit interrupt completes */
With device tree it is no more possible to reset the PHY at board level. Furthermore, doing in the driver allow to power down the PHY when the network interface is no more used. This reset can't be done at the PHY driver level. The PHY must be able to answer to the mii bus scan to let the kernel creating a PHY device. The patch introduces a new optional property "reset-gpios" at PHY level. Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- Hi, I agree with Sasha to start with a good binding and indeed the reset is more related to the PHY than to the MAC even if currently we have to manipulate it at MAC level. I also followed Russell advice to not use fwnode functions. However the following code seems to work: struct fwnode_handle *phy_node = device_get_next_child_node(&pdev->dev, NULL); bp->reset_gpio = fwnode_get_named_gpiod(phy_node, "reset-gpios"); if (IS_ERR) bp->reset_gpio = NULL; gpiod_set_value(bp->reset_gpio, GPIOD_OUT_HIGH); Given that it doesn't make the code better I kept the of_get_named_gpio method. Gregory Documentation/devicetree/bindings/net/macb.txt | 7 +++++++ drivers/net/ethernet/cadence/macb.c | 16 ++++++++++++++++ drivers/net/ethernet/cadence/macb.h | 1 + 3 files changed, 24 insertions(+)