[PATCH net-next 0/2] DSA: GPIO to reset switches

[PATCH net-next 0/2] DSA: GPIO to reset switches

[Andrew Lunn]

These two patches add support for using a GPIO to hard reset a switch
during setup.

Andrew Lunn (2):
  net: dsa: Add support for a switch reset gpio
  dsa: mv88e6xxx.c: Hardware reset the chip if available

 Documentation/devicetree/bindings/net/dsa/dsa.txt |  3 +++
 drivers/net/dsa/mv88e6xxx.c                       | 14 ++++++++++++++
 include/net/dsa.h                                 |  9 +++++++++
 net/dsa/dsa.c                                     | 19 +++++++++++++++++++
 4 files changed, 45 insertions(+)

-- 
2.6.2

[PATCH net-next 1/2] net: dsa: Add support for a switch reset gpio

[Andrew Lunn]

Some boards have a gpio line tied to the switch reset pin. Allow this
gpio to be retrieved from the device tree, and take the switch out of
reset before performing the probe.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/devicetree/bindings/net/dsa/dsa.txt |  3 +++
 include/net/dsa.h                                 |  9 +++++++++
 net/dsa/dsa.c                                     | 19 +++++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt
index 04e6bef3ac3f..5fdbbcdf8c4b 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.txt
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt
@@ -31,6 +31,8 @@ A switch child node has the following optional property:
 			  switch. Must be set if the switch can not detect
 			  the presence and/or size of a connected EEPROM,
 			  otherwise optional.
+- reset-gpios		: phandle and specifier to a gpio line connected to
+			  reset pin of the switch chip.
 
 A switch may have multiple "port" children nodes
 
@@ -114,6 +116,7 @@ Example:
 			#size-cells = <0>;
 			reg = <17 1>;	/* MDIO address 17, switch 1 in tree */
 			mii-bus = <&mii_bus1>;
+			reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
 
 			switch1port0: port@0 {
 				reg = <0>;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 82a4c6011173..913a61ea81d3 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -16,6 +16,7 @@
 #include <linux/timer.h>
 #include <linux/workqueue.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/phy.h>
 #include <linux/phy_fixed.h>
 #include <linux/ethtool.h>
@@ -64,6 +65,14 @@ struct dsa_chip_data {
 	 * NULL if there is only one switch chip.
 	 */
 	s8		*rtable;
+
+	/*
+	 * A switch may have a GPIO line tied to its reset pin. Parse
+	 * this from the device tree, and use it before performing
+	 * switch soft reset.
+	 */
+	int reset;
+	enum of_gpio_flags reset_flags;
 };
 
 struct dsa_platform_data {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 1eba07feb34a..39cd19eaaf4e 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -21,6 +21,8 @@
 #include <linux/of_mdio.h>
 #include <linux/of_platform.h>
 #include <linux/of_net.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of_gpio.h>
 #include <linux/sysfs.h>
 #include <linux/phy_fixed.h>
 #include "dsa_priv.h"
@@ -688,6 +690,9 @@ static int dsa_of_probe(struct device *dev)
 	const char *port_name;
 	int chip_index, port_index;
 	const unsigned int *sw_addr, *port_reg;
+	int gpio;
+	enum of_gpio_flags flags;
+	int off;
 	u32 eeprom_len;
 	int ret;
 
@@ -767,6 +772,20 @@ static int dsa_of_probe(struct device *dev)
 			cd->host_dev = &mdio_bus_switch->dev;
 		}
 
+		gpio = of_get_named_gpio_flags(child, "reset-gpios", 0,
+					       &flags);
+		if (gpio_is_valid(gpio)) {
+			ret = devm_gpio_request_one(dev, gpio, flags,
+						    "switch_reset");
+			if (ret)
+				goto out_free_chip;
+
+			cd->reset = gpio;
+			cd->reset_flags = flags;
+			off = (flags && OF_GPIO_ACTIVE_LOW ? 1 : 0);
+			gpio_direction_output(cd->reset, off);
+		}
+
 		for_each_available_child_of_node(child, port) {
 			port_reg = of_get_property(port, "reg", NULL);
 			if (!port_reg)
-- 
2.6.2

Re: [PATCH net-next 1/2] net: dsa: Add support for a switch reset gpio

[Andrew Lunn]

> index 1eba07feb34a..39cd19eaaf4e 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -21,6 +21,8 @@
>  #include <linux/of_mdio.h>
>  #include <linux/of_platform.h>
>  #include <linux/of_net.h>
> +#include <linux/nvmem-consumer.h>

Comment to self:

That include should not be there.

I will post a v2 once the discussion has stopped.

  Andrew

Re: [PATCH net-next 1/2] net: dsa: Add support for a switch reset gpio

[Neil Armstrong]

Hi Andrew,

On 11/19/2015 12:29 AM, Andrew Lunn wrote:
> +		gpio = of_get_named_gpio_flags(child, "reset-gpios", 0,
> +					       &flags);
> +		if (gpio_is_valid(gpio)) {
> +			ret = devm_gpio_request_one(dev, gpio, flags,
> +						    "switch_reset");
> +			if (ret)
> +				goto out_free_chip;
> +
> +			cd->reset = gpio;
> +			cd->reset_flags = flags;
> +			off = (flags && OF_GPIO_ACTIVE_LOW ? 1 : 0);
> +			gpio_direction_output(cd->reset, off);
> +		}
> +
>  		for_each_available_child_of_node(child, port) {
>  			port_reg = of_get_property(port, "reg", NULL);
>  			if (!port_reg)
> 

You could also use :
gpio = of_get_named_gpio(child, "reset-gpios", 0)
devm_gpio_request(dev, gpio, "switch_reset")

and :
cd->reset = gpio_to_desc(gpio);

and cd->switch reset to struct gpio_desc *reset

to use the gpiod calls afterward. The flags are no more needed.

Neil

Re: [PATCH net-next 1/2] net: dsa: Add support for a switch reset gpio

[Andrew Lunn]

On Thu, Nov 19, 2015 at 09:39:11AM +0100, Neil Armstrong wrote:
> Hi Andrew,
> 
> On 11/19/2015 12:29 AM, Andrew Lunn wrote:
> > +		gpio = of_get_named_gpio_flags(child, "reset-gpios", 0,
> > +					       &flags);
> > +		if (gpio_is_valid(gpio)) {
> > +			ret = devm_gpio_request_one(dev, gpio, flags,
> > +						    "switch_reset");
> > +			if (ret)
> > +				goto out_free_chip;
> > +
> > +			cd->reset = gpio;
> > +			cd->reset_flags = flags;
> > +			off = (flags && OF_GPIO_ACTIVE_LOW ? 1 : 0);
> > +			gpio_direction_output(cd->reset, off);
> > +		}
> > +
> >  		for_each_available_child_of_node(child, port) {
> >  			port_reg = of_get_property(port, "reg", NULL);
> >  			if (!port_reg)
> > 
> 
> You could also use :
> gpio = of_get_named_gpio(child, "reset-gpios", 0)
> devm_gpio_request(dev, gpio, "switch_reset")
> 
> and :
> cd->reset = gpio_to_desc(gpio);
> 
> and cd->switch reset to struct gpio_desc *reset
> 
> to use the gpiod calls afterward. The flags are no more needed.

Hi Neil

I really wanted to use gpiod, since as you said, it takes care of
active low/active high. I tried all sorts of combinations. The problem
with this one is that desc->flags is set in gpiod_parse_flags(), which
is only called from gpiod_get_index(), which is limited to gpios in
the root of the devices subtree. I was 'lucky' in that my reset is
active low, so i noticed the problem when my switch failed to probe,
being held in reset, because the ACTIVE_LOW flag in DT is being ignored.

It seems like gpiod is backwards compatible to gpio, but gpio is not
forward compatible to gpiod.

	Andrew

[PATCH net-next 2/2] dsa: mv88e6xxx.c: Hardware reset the chip if available

[Andrew Lunn]

The device tree binding now allows a gpio to be specified which is
attached to the switch chips reset line. If it is defined, perform
a hardware reset on the switch during setup.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index b06dba05594a..c0bbbe7713c5 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -19,6 +19,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/gpio/consumer.h>
 #include <linux/phy.h>
 #include <net/dsa.h>
 #include <net/switchdev.h>
@@ -2323,7 +2324,10 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	u16 is_reset = (ppu_active ? 0x8800 : 0xc800);
+	int gpio = ds->pd->reset;
+	int flags = ds->pd->reset_flags;
 	unsigned long timeout;
+	int on = 1;
 	int ret;
 	int i;
 
@@ -2336,6 +2340,16 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
 	/* Wait for transmit queues to drain. */
 	usleep_range(2000, 4000);
 
+	/* If there is a gpio connected to the reset pin, toggle it */
+	if (gpio_is_valid(gpio)) {
+		if (flags && OF_GPIO_ACTIVE_LOW)
+			on = !on;
+		gpio_set_value_cansleep(gpio, on);
+		usleep_range(10000, 20000);
+		gpio_set_value_cansleep(gpio, !on);
+		usleep_range(10000, 20000);
+	}
+
 	/* Reset the switch. Keep the PPU active if requested. The PPU
 	 * needs to be active to support indirect phy register access
 	 * through global registers 0x18 and 0x19.
-- 
2.6.2

Re: [PATCH net-next 2/2] dsa: mv88e6xxx.c: Hardware reset the chip if available

[Florian Fainelli]

On 18/11/15 15:29, Andrew Lunn wrote:
> The device tree binding now allows a gpio to be specified which is
> attached to the switch chips reset line. If it is defined, perform
> a hardware reset on the switch during setup.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mv88e6xxx.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index b06dba05594a..c0bbbe7713c5 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -19,6 +19,7 @@
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/netdevice.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/phy.h>
>  #include <net/dsa.h>
>  #include <net/switchdev.h>
> @@ -2323,7 +2324,10 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
>  {
>  	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>  	u16 is_reset = (ppu_active ? 0x8800 : 0xc800);
> +	int gpio = ds->pd->reset;
> +	int flags = ds->pd->reset_flags;
>  	unsigned long timeout;
> +	int on = 1;
>  	int ret;
>  	int i;
>  
> @@ -2336,6 +2340,16 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
>  	/* Wait for transmit queues to drain. */
>  	usleep_range(2000, 4000);
>  
> +	/* If there is a gpio connected to the reset pin, toggle it */
> +	if (gpio_is_valid(gpio)) {
> +		if (flags && OF_GPIO_ACTIVE_LOW)
> +			on = !on;
> +		gpio_set_value_cansleep(gpio, on);
> +		usleep_range(10000, 20000);
> +		gpio_set_value_cansleep(gpio, !on);
> +		usleep_range(10000, 20000);
> +	}

We are embedding reset logic here about the delays and polarity, while
there is now a proper abstraction for this within the reset controller
subsystem under drivers/reset/core.c. Could we utilize that facility
instead which would make us more robust wrt. non-GPIO reset lines (for
instance some SF2 switches on DSL gateways could definitively benefit
from this).

There does not seem to be a reset controller GPIO binding and generic
driver, but this seems like an appropriate candidate?
-- 
Florian

Re: [PATCH net-next 2/2] dsa: mv88e6xxx.c: Hardware reset the chip if available

[Andrew Lunn]

> We are embedding reset logic here about the delays and polarity, while
> there is now a proper abstraction for this within the reset controller
> subsystem under drivers/reset/core.c. Could we utilize that facility
> instead which would make us more robust wrt. non-GPIO reset lines (for
> instance some SF2 switches on DSL gateways could definitively benefit
> from this).
> 
> There does not seem to be a reset controller GPIO binding and generic
> driver, but this seems like an appropriate candidate?

Hi Florian

That was my first thought as well. But then i went searching and found

http://permalink.gmane.org/gmane.linux.ports.arm.kernel/257149

where Mark Rutland says no to the idea. reset-gpios should be handle
by the device.

Anyway, delays are hard coded, but polarity not. That is in the
generic device tree binding for a GPIO, you can say GPIO_ACTIVE_LOW or
GPIO_ACTIVE_HIGH. The delays are also hard coded in the Marvell
specific part, so should not cause a problem to other manufactures
chips.

	Andrew

Re: [PATCH net-next 2/2] dsa: mv88e6xxx.c: Hardware reset the chip if available

[Phil Reid]

On 19/11/2015 7:29 AM, Andrew Lunn wrote:
> The device tree binding now allows a gpio to be specified which is
> attached to the switch chips reset line. If it is defined, perform
> a hardware reset on the switch during setup.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>   drivers/net/dsa/mv88e6xxx.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index b06dba05594a..c0bbbe7713c5 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -19,6 +19,7 @@
>   #include <linux/list.h>
>   #include <linux/module.h>
>   #include <linux/netdevice.h>
> +#include <linux/gpio/consumer.h>
>   #include <linux/phy.h>
>   #include <net/dsa.h>
>   #include <net/switchdev.h>
> @@ -2323,7 +2324,10 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
>   {
>   	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>   	u16 is_reset = (ppu_active ? 0x8800 : 0xc800);
> +	int gpio = ds->pd->reset;
> +	int flags = ds->pd->reset_flags;
>   	unsigned long timeout;
> +	int on = 1;
>   	int ret;
>   	int i;
>
> @@ -2336,6 +2340,16 @@ int mv88e6xxx_switch_reset(struct dsa_switch *ds, bool ppu_active)
>   	/* Wait for transmit queues to drain. */
>   	usleep_range(2000, 4000);
>
> +	/* If there is a gpio connected to the reset pin, toggle it */
> +	if (gpio_is_valid(gpio)) {
> +		if (flags && OF_GPIO_ACTIVE_LOW)
> +			on = !on;
> +		gpio_set_value_cansleep(gpio, on);
> +		usleep_range(10000, 20000);
> +		gpio_set_value_cansleep(gpio, !on);
> +		usleep_range(10000, 20000);
> +	}
> +
This is a general query about what is the preferred method of allocating gpios.
The gpiod* family of functions provided similar functionality and automatically
deal with active low / high outputs, direction, inital value  etc...
I raise this more for knowledge on what method I should use for my patches.


>   	/* Reset the switch. Keep the PPU active if requested. The PPU
>   	 * needs to be active to support indirect phy register access
>   	 * through global registers 0x18 and 0x19.
>

Other than that the concept looks good and something I has been looking at adding.

Would it be worth considering placing the chip in reset on driver remove?
I have an battery powered hardware platform using one of this marvell devices and
for certain configurations we don't need the switch active. So unloading the
module to place the device in reset and would save power.
Reloading would reinitialise the port.





-- 
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au

Re: [PATCH net-next 2/2] dsa: mv88e6xxx.c: Hardware reset the chip if available

[Andrew Lunn]

> This is a general query about what is the preferred method of allocating gpios.
> The gpiod* family of functions provided similar functionality and automatically
> deal with active low / high outputs, direction, inital value  etc...
> I raise this more for knowledge on what method I should use for my patches.

I first tried using gpiod, but failed. The API requires that the gpios
be in the root of the device's subtree in the DT blob. But here the
gpios are associated to a switch, and the switch part of the subtree
is one level down. gpiod has no way to get them from there.

> Other than that the concept looks good and something I has been
> looking at adding.

Please feel free to test it on your hardware and send a Tested-by :-)

> Would it be worth considering placing the chip in reset on driver
> remove?  I have an battery powered hardware platform using one of
> this marvell devices and for certain configurations we don't need
> the switch active. So unloading the module to place the device in
> reset and would save power.  Reloading would reinitialise the port.

I think we first need to get module unload/load working reliably.
This is being worked on. But i'm not against this in principle.  Power
saving in general needs working on for Marvall devices. There is no
suspend/resume support for example. It would also be good to ensure
the PHYs are powered off when not needed, etc.

    Andrew

Re: [PATCH net-next 2/2] dsa: mv88e6xxx.c: Hardware reset the chip if available

[Phil Reid]

On 19/11/2015 10:25 AM, Andrew Lunn wrote:
 >> This is a general query about what is the preferred method of allocating gpios.
 >> The gpiod* family of functions provided similar functionality and automatically
 >> deal with active low / high outputs, direction, inital value  etc...
 >> I raise this more for knowledge on what method I should use for my patches.
 >
 > I first tried using gpiod, but failed. The API requires that the gpios
 > be in the root of the device's subtree in the DT blob. But here the
 > gpios are associated to a switch, and the switch part of the subtree
 > is one level down. gpiod has no way to get them from there.
Hadn't dug into the gpiod stuff that far. Yes looks like there limited support for
extracting from sub nodes. There's devm_get_gpiod_from_child which looks like it
does something like that but I don't have any idea how to go from an of_node to
a fwnode_handle.

 > +		gpio = of_get_named_gpio_flags(child, "reset-gpios", 0,
 > +					       &flags);
 > +		if (gpio_is_valid(gpio)) {
 > +			ret = devm_gpio_request_one(dev, gpio, flags,
 > +						    "switch_reset");
 > +			if (ret)
 > +				goto out_free_chip;

The flags passed into devm_gpio_request_one are of type GPIOF_* which don't
match the device tree definitions for flags. As your handling the device flags
manually I think devm_gpio_request_one flags should be 0. Or you can translate
the device tree flags and get devm_gpio_request_one to configure the polarity etc.
Then I think you don't need to do your polarity inversions later on.

 >
 >> Other than that the concept looks good and something I has been
 >> looking at adding.
 >
 > Please feel free to test it on your hardware and send a Tested-by :-)
Worked as expected on my hardware, can see the line toggle, chip is configured correctly.

Tested-by: Phil Reid <preid@electromag.com.au>


 >
 >> Would it be worth considering placing the chip in reset on driver
 >> remove?  I have an battery powered hardware platform using one of
 >> this marvell devices and for certain configurations we don't need
 >> the switch active. So unloading the module to place the device in
 >> reset and would save power.  Reloading would reinitialise the port.
 >
 > I think we first need to get module unload/load working reliably.
 > This is being worked on. But i'm not against this in principle.  Power
 > saving in general needs working on for Marvall devices. There is no
 > suspend/resume support for example. It would also be good to ensure
 > the PHYs are powered off when not needed, etc.
Seems a sound plan.




-- 
Regards
Phil Reid