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.
The patch introduces a new optional property "phy-reset-gpio" inspired
from the one use for the FEC.
Signed-off-by: Gregory CLEMENT <[email protected]>
---
Documentation/devicetree/bindings/net/macb.txt | 3 +++
drivers/net/ethernet/cadence/macb.c | 26 ++++++++++++++++++++++++++
drivers/net/ethernet/cadence/macb.h | 1 +
3 files changed, 30 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
index b5d7976..546d34d 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:
+- phy-reset-gpio : Should specify the gpio for phy reset
+
Examples:
macb0: ethernet@fffc4000 {
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 88c1e1a..e630c56 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -30,6 +30,7 @@
#include <linux/of_device.h>
#include <linux/of_mdio.h>
#include <linux/of_net.h>
+#include <linux/of_gpio.h>
#include "macb.h"
@@ -2733,6 +2734,28 @@ static int at91ether_init(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_OF
+static void macb_reset_phy(struct macb *bp, struct device_node *np, int state)
+{
+ if (!np)
+ return;
+
+ bp->reset_gpio = of_get_named_gpio(np, "phy-reset-gpio", 0);
+
+ if (gpio_is_valid(bp->reset_gpio))
+ gpio_direction_output(bp->reset_gpio, state);
+}
+#else /* CONFIG_OF */
+static void macb_reset_phy(struct macb *bp, struct device_node *np, int state)
+{
+ /*
+ * In case of platform probe, the reset has been done
+ * by machine code.
+ */
+}
+#endif /* CONFIG_OF */
+
+
static const struct macb_config at91sam9260_config = {
.caps = MACB_CAPS_USRIO_HAS_CLKEN | MACB_CAPS_USRIO_DEFAULT_IS_MII,
.clk_init = macb_clk_init,
@@ -2900,6 +2923,8 @@ static int macb_probe(struct platform_device *pdev)
else
macb_get_hwaddr(bp);
+ macb_reset_phy(bp, np, 1);
+
err = of_get_phy_mode(np);
if (err < 0) {
pdata = dev_get_platdata(&pdev->dev);
@@ -2966,6 +2991,7 @@ static int macb_remove(struct platform_device *pdev)
mdiobus_unregister(bp->mii_bus);
kfree(bp->mii_bus->irq);
mdiobus_free(bp->mii_bus);
+ macb_reset_phy(bp, pdev->dev.of_node, 0);
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..637d22c 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;
+ int reset_gpio;
/* AT91RM9200 transmit */
struct sk_buff *skb; /* holds skb until xmit interrupt completes */
--
2.5.0
On Wed, Dec 09, 2015 at 06:49:43PM +0100, Gregory CLEMENT wrote:
> 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.
>
> The patch introduces a new optional property "phy-reset-gpio" inspired
> from the one use for the FEC.
>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---
> Documentation/devicetree/bindings/net/macb.txt | 3 +++
> drivers/net/ethernet/cadence/macb.c | 26 ++++++++++++++++++++++++++
> drivers/net/ethernet/cadence/macb.h | 1 +
> 3 files changed, 30 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
> index b5d7976..546d34d 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:
> +- phy-reset-gpio : Should specify the gpio for phy reset
Hi Gregory
It is convention to use the plural here. So it should be phy-reset-gpios.
> +
> Examples:
>
> macb0: ethernet@fffc4000 {
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 88c1e1a..e630c56 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -30,6 +30,7 @@
> #include <linux/of_device.h>
> #include <linux/of_mdio.h>
> #include <linux/of_net.h>
> +#include <linux/of_gpio.h>
>
> #include "macb.h"
>
> @@ -2733,6 +2734,28 @@ static int at91ether_init(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static void macb_reset_phy(struct macb *bp, struct device_node *np, int state)
> +{
> + if (!np)
> + return;
> +
> + bp->reset_gpio = of_get_named_gpio(np, "phy-reset-gpio", 0);
> +
> + if (gpio_is_valid(bp->reset_gpio))
> + gpio_direction_output(bp->reset_gpio, state);
This does not handle the flags part of the GPIO descriptor in device
tree. i.e. ACTIVE_LOW or ACTIVE_HIGH. I think the FEC driver has the
same issue, and is not the best driver to copy. Using the gpiod API
will solve this issue.
Andrew
Hi Gregory,
so far dealt with this in u-boot. The power down the PHY part makes
sense, though. Minor nit down inline.
Will need to test on hardware (Zynq).
On Wed, Dec 9, 2015 at 9:49 AM, Gregory CLEMENT
<[email protected]> wrote:
> 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.
>
> The patch introduces a new optional property "phy-reset-gpio" inspired
> from the one use for the FEC.
>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---
> Documentation/devicetree/bindings/net/macb.txt | 3 +++
> drivers/net/ethernet/cadence/macb.c | 26 ++++++++++++++++++++++++++
> drivers/net/ethernet/cadence/macb.h | 1 +
> 3 files changed, 30 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/macb.txt b/Documentation/devicetree/bindings/net/macb.txt
> index b5d7976..546d34d 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:
> +- phy-reset-gpio : Should specify the gpio for phy reset
> +
> Examples:
>
> macb0: ethernet@fffc4000 {
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 88c1e1a..e630c56 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -30,6 +30,7 @@
> #include <linux/of_device.h>
> #include <linux/of_mdio.h>
> #include <linux/of_net.h>
> +#include <linux/of_gpio.h>
>
> #include "macb.h"
>
> @@ -2733,6 +2734,28 @@ static int at91ether_init(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static void macb_reset_phy(struct macb *bp, struct device_node *np, int state)
> +{
> + if (!np)
> + return;
> +
> + bp->reset_gpio = of_get_named_gpio(np, "phy-reset-gpio", 0);
Any reason to do of_get_named() all the time instead of using the one
you stored in bp->reset_gpio?
You could move it to probe and then just use the stored value here ...
not sure if it buys much ;-)
Cheers,
Moritz
Hi Gregory,
[auto build test ERROR on net-next/master]
[also build test ERROR on v4.4-rc4 next-20151209]
url: https://github.com/0day-ci/linux/commits/Gregory-CLEMENT/net-macb-add-support-for-resetting-PHY-using-GPIO/20151210-015931
config: x86_64-acpi-redef (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/net/ethernet/cadence/macb.c: In function 'macb_probe':
>> drivers/net/ethernet/cadence/macb.c:2930:2: error: implicit declaration of function 'macb_reset_phy' [-Werror=implicit-function-declaration]
macb_reset_phy(bp, np, 1);
^
cc1: some warnings being treated as errors
vim +/macb_reset_phy +2930 drivers/net/ethernet/cadence/macb.c
2924 mac = of_get_mac_address(np);
2925 if (mac)
2926 memcpy(bp->dev->dev_addr, mac, ETH_ALEN);
2927 else
2928 macb_get_hwaddr(bp);
2929
> 2930 macb_reset_phy(bp, np, 1);
2931
2932 err = of_get_phy_mode(np);
2933 if (err < 0) {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Gregory,
On Wed, Dec 09, 2015 at 06:49:43PM +0100, Gregory CLEMENT wrote:
> 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.
>
> The patch introduces a new optional property "phy-reset-gpio" inspired
> from the one use for the FEC.
I don't think it's a good idea to further extend the usage of this
binding. The driver should use the phy-handle property and
of_phy_connect() which gives you a proper device node for the phy. Then
the phy device node should get the reset gpio. I know it's more work,
but doing it like this gives you additional goodies like proper handling
of the max-speed property, a fixed-link if necessary and picking the
correct phy if there are muliple phys on the bus.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Sascha,
On jeu., déc. 10 2015, Sascha Hauer <[email protected]> wrote:
> Hi Gregory,
>
> On Wed, Dec 09, 2015 at 06:49:43PM +0100, Gregory CLEMENT wrote:
>> 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.
>>
>> The patch introduces a new optional property "phy-reset-gpio" inspired
>> from the one use for the FEC.
>
> I don't think it's a good idea to further extend the usage of this
> binding. The driver should use the phy-handle property and
> of_phy_connect() which gives you a proper device node for the phy. Then
> the phy device node should get the reset gpio. I know it's more work,
So you suggest to pass from this binding:
macb1: ethernet@fc028000 {
phy-mode = "rmii";
status = "okay";
#address-cells = <1>;
#size-cells = <0>;
status = "okay";
phy-reset-gpio = <&pioE 6 GPIO_ACTIVE_HIGH>;
ethernet-phy@1 {
reg = <0x1>;
interrupt-parent = <&pioB>;
interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
};
};
to this binding
macb1: ethernet@fc028000 {
phy-mode = "rmii";
status = "okay";
#address-cells = <1>;
#size-cells = <0>;
status = "okay";
ethernet-phy@1 {
reg = <0x1>;
interrupt-parent = <&pioB>;
interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
phy-reset-gpio = <&pioE 6 GPIO_ACTIVE_HIGH>;
};
};
> but doing it like this gives you additional goodies like proper handling
> of the max-speed property, a fixed-link if necessary and picking the
Currently there is phy_connect_direct so we can already handle the
preperty of the phy.
> correct phy if there are muliple phys on the bus.
but I agree with this one.
Gregory
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On Thu, Dec 10, 2015 at 04:08:08PM +0100, Gregory CLEMENT wrote:
> Hi Sascha,
>
> On jeu., d?c. 10 2015, Sascha Hauer <[email protected]> wrote:
>
> > Hi Gregory,
> >
> > On Wed, Dec 09, 2015 at 06:49:43PM +0100, Gregory CLEMENT wrote:
> >> 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.
> >>
> >> The patch introduces a new optional property "phy-reset-gpio" inspired
> >> from the one use for the FEC.
> >
> > I don't think it's a good idea to further extend the usage of this
> > binding. The driver should use the phy-handle property and
> > of_phy_connect() which gives you a proper device node for the phy. Then
> > the phy device node should get the reset gpio. I know it's more work,
>
> So you suggest to pass from this binding:
> macb1: ethernet@fc028000 {
> phy-mode = "rmii";
> status = "okay";
> #address-cells = <1>;
> #size-cells = <0>;
> status = "okay";
> phy-reset-gpio = <&pioE 6 GPIO_ACTIVE_HIGH>;
>
> ethernet-phy@1 {
> reg = <0x1>;
> interrupt-parent = <&pioB>;
> interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
>
> };
> };
>
> to this binding
> macb1: ethernet@fc028000 {
> phy-mode = "rmii";
> status = "okay";
> #address-cells = <1>;
> #size-cells = <0>;
> status = "okay";
>
> ethernet-phy@1 {
> reg = <0x1>;
> interrupt-parent = <&pioB>;
> interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> phy-reset-gpio = <&pioE 6 GPIO_ACTIVE_HIGH>;
> };
> };
s/phy-reset-gpio/reset-gpios/, but yes.
I had assumed you do not use a phy description in the device tree at all
since the macb driver doesn't use of_phy_connect(). Apparently there are
other possibilities to connect the phy device nodes with the phys
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Sascha,
On ven., déc. 11 2015, Sascha Hauer <[email protected]> wrote:
> On Thu, Dec 10, 2015 at 04:08:08PM +0100, Gregory CLEMENT wrote:
>> Hi Sascha,
>>
>> On jeu., déc. 10 2015, Sascha Hauer <[email protected]> wrote:
>>
>> > Hi Gregory,
>> >
>> > On Wed, Dec 09, 2015 at 06:49:43PM +0100, Gregory CLEMENT wrote:
>> >> 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.
>> >>
>> >> The patch introduces a new optional property "phy-reset-gpio" inspired
>> >> from the one use for the FEC.
>> >
>> > I don't think it's a good idea to further extend the usage of this
>> > binding. The driver should use the phy-handle property and
>> > of_phy_connect() which gives you a proper device node for the phy. Then
>> > the phy device node should get the reset gpio. I know it's more work,
>>
>> So you suggest to pass from this binding:
>> macb1: ethernet@fc028000 {
>> phy-mode = "rmii";
>> status = "okay";
>> #address-cells = <1>;
>> #size-cells = <0>;
>> status = "okay";
>> phy-reset-gpio = <&pioE 6 GPIO_ACTIVE_HIGH>;
>>
>> ethernet-phy@1 {
>> reg = <0x1>;
>> interrupt-parent = <&pioB>;
>> interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
>>
>> };
>> };
>>
>> to this binding
>> macb1: ethernet@fc028000 {
>> phy-mode = "rmii";
>> status = "okay";
>> #address-cells = <1>;
>> #size-cells = <0>;
>> status = "okay";
>>
>> ethernet-phy@1 {
>> reg = <0x1>;
>> interrupt-parent = <&pioB>;
>> interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
>> phy-reset-gpio = <&pioE 6 GPIO_ACTIVE_HIGH>;
>> };
>> };
>
> s/phy-reset-gpio/reset-gpios/, but yes.
So I took this way. But I had several issues. The first one was that to
be able to create a phy device it must first be detected by
get_phy_device from of_mdiobus_register_phy. But in order to be
detected it must answer during the scan of the mii bus. That means we
can't bind the gpio to the device in order to use it because when we
need to manage the reset gpio the device is not yet created.
So I see 2 options:
- leaving the phy-reset-gpios property in the ethernet node. Thanks to
this we can use the gpiod functions, and it is still possible to use
manage the power management.
- using a reset-gpios property inside the phy node. But then the only
solution to get a reference on it, will be to use
of_get_named_gpio. All the gpiod functions need a reference to the
device that we won't have at this point. Also we will only be able to
power up the reset, but we won't have any reference to it latter.
Thanks,
Gregory
>
> I had assumed you do not use a phy description in the device tree at all
> since the macb driver doesn't use of_phy_connect(). Apparently there are
> other possibilities to connect the phy device nodes with the phys
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On Fri, Dec 11, 2015 at 10:40:22AM +0100, Gregory CLEMENT wrote:
> Hi Sascha,
>
> On ven., d?c. 11 2015, Sascha Hauer <[email protected]> wrote:
>
> > On Thu, Dec 10, 2015 at 04:08:08PM +0100, Gregory CLEMENT wrote:
> >> Hi Sascha,
> >>
> >> On jeu., d?c. 10 2015, Sascha Hauer <[email protected]> wrote:
> >>
> >> > Hi Gregory,
> >> >
> >> > On Wed, Dec 09, 2015 at 06:49:43PM +0100, Gregory CLEMENT wrote:
> >> >> 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.
> >> >>
> >> >> The patch introduces a new optional property "phy-reset-gpio" inspired
> >> >> from the one use for the FEC.
> >> >
> >> > I don't think it's a good idea to further extend the usage of this
> >> > binding. The driver should use the phy-handle property and
> >> > of_phy_connect() which gives you a proper device node for the phy. Then
> >> > the phy device node should get the reset gpio. I know it's more work,
> >>
> >> So you suggest to pass from this binding:
> >> macb1: ethernet@fc028000 {
> >> phy-mode = "rmii";
> >> status = "okay";
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> status = "okay";
> >> phy-reset-gpio = <&pioE 6 GPIO_ACTIVE_HIGH>;
> >>
> >> ethernet-phy@1 {
> >> reg = <0x1>;
> >> interrupt-parent = <&pioB>;
> >> interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> >>
> >> };
> >> };
> >>
> >> to this binding
> >> macb1: ethernet@fc028000 {
> >> phy-mode = "rmii";
> >> status = "okay";
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> status = "okay";
> >>
> >> ethernet-phy@1 {
> >> reg = <0x1>;
> >> interrupt-parent = <&pioB>;
> >> interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> >> phy-reset-gpio = <&pioE 6 GPIO_ACTIVE_HIGH>;
> >> };
> >> };
> >
> > s/phy-reset-gpio/reset-gpios/, but yes.
>
> So I took this way. But I had several issues. The first one was that to
> be able to create a phy device it must first be detected by
> get_phy_device from of_mdiobus_register_phy. But in order to be
> detected it must answer during the scan of the mii bus. That means we
> can't bind the gpio to the device in order to use it because when we
> need to manage the reset gpio the device is not yet created.
>
> So I see 2 options:
>
> - leaving the phy-reset-gpios property in the ethernet node. Thanks to
> this we can use the gpiod functions, and it is still possible to use
> manage the power management.
>
> - using a reset-gpios property inside the phy node. But then the only
> solution to get a reference on it, will be to use
> of_get_named_gpio. All the gpiod functions need a reference to the
> device that we won't have at this point. Also we will only be able to
> power up the reset, but we won't have any reference to it latter.
Have you seen fwnode_get_named_gpiod()? This seems to be the right
function for the job.
I think we should get the binding right. The code can be changed easier
than the binding later.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Fri, Dec 11, 2015 at 11:21:30AM +0100, Sascha Hauer wrote:
> On Fri, Dec 11, 2015 at 10:40:22AM +0100, Gregory CLEMENT wrote:
> > So I see 2 options:
> >
> > - leaving the phy-reset-gpios property in the ethernet node. Thanks to
> > this we can use the gpiod functions, and it is still possible to use
> > manage the power management.
> >
> > - using a reset-gpios property inside the phy node. But then the only
> > solution to get a reference on it, will be to use
> > of_get_named_gpio. All the gpiod functions need a reference to the
> > device that we won't have at this point. Also we will only be able to
> > power up the reset, but we won't have any reference to it latter.
>
> Have you seen fwnode_get_named_gpiod()? This seems to be the right
> function for the job.
Except:
1. You don't have a dev->fwnode pointer setup.
2. Using &dev->of_node->fwnode is really going underneath the covers.
I've pointed this problem out several times with the fwnode code, which
seems to be a half-baked and incomplete design when it comes to DT.
I'd suggest using of_get_named_gpio_flags() et.al. and converting the
resulting gpio number to a gpio descriptor - or trying to push DT and
fwnode people to initialise dev->fwnode so that a proper transition to
fwnode for DT can be made.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.