| Message ID | 1447889365-25256-3-git-send-email-andrew@lunn.ch |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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?
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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(+)