| Message ID | 1374139586-6344-1-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 18, 2013 at 11:26:26AM +0200, Philipp Zabel wrote: > This driver implements a reset controller device that toggle a gpio > connected to a reset pin of a peripheral IC. The delay between assertion > and de-assertion of the reset signal can be configured via device tree. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > Reviewed-by: Stephen Warren <swarren@nvidia.com> Tested-by: Shawn Guo <shawn.guo@linaro.org>
On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: > This driver implements a reset controller device that toggle a gpio > connected to a reset pin of a peripheral IC. The delay between assertion > and de-assertion of the reset signal can be configured via device tree. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > Reviewed-by: Stephen Warren <swarren@nvidia.com> > --- > Changes since v9: > - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert > should not be called from atomic context. > --- > .../devicetree/bindings/reset/gpio-reset.txt | 35 ++++ > drivers/reset/Kconfig | 11 ++ > drivers/reset/Makefile | 1 + > drivers/reset/gpio-reset.c | 184 +++++++++++++++++++++ > 4 files changed, 231 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.txt > create mode 100644 drivers/reset/gpio-reset.c > > diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt > new file mode 100644 > index 0000000..bca5348 > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/gpio-reset.txt > @@ -0,0 +1,35 @@ > +GPIO reset controller > +===================== > + > +A GPIO reset controller controls a single GPIO that is connected to the reset > +pin of a peripheral IC. Please also refer to reset.txt in this directory for > +common reset controller binding usage. > + > +Required properties: > +- compatible: Should be "gpio-reset" > +- reset-gpios: A gpio used as reset line. The gpio specifier for this property > + depends on the gpio controller that provides the gpio. > +- #reset-cells: 0, see below > + > +Optional properties: > +- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for > + this duration to reset. > +- initially-in-reset: boolean. If not set, the initial state should be a > + deasserted reset line. If this property exists, the > + reset line should be kept in reset. > + > +example: > + > +sii902x_reset: gpio-reset { > + compatible = "gpio-reset"; > + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > + reset-delay-us = <10000>; > + initially-in-reset; > + #reset-cells = <0>; > +}; > + > +/* Device with nRESET pin connected to GPIO5_0 */ > +sii902x@39 { > + /* ... */ > + resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */ > +}; I don't like the approach here. The reset GPIO line is not a device in itself, and surely the way in which it must be toggled to reset a device is a property of that device, not the GPIO. We're leaking a Linux internal abstraction rather than describing teh hardware. I think thie linkage of the gpio would be better described on the node for the device with the reset input, in a binding-specific property for the device (e.g. names for the specific input line on the device): I'd imagine the delay required would be fixed for a device (or possibly influenced by clock inputs), and can either be knwon by the driver without dt information, or discovered elsewhere (e.g. dervied from the rates of clock inputs). We shuold be able to derive that from the compatible string in most cases. I think this should look more like the below: /* Device with nRESET pin connected to GPIO5_0 */ sii902x@39 { /* named for the actual input line */ nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; /* * If there's some configurable property relating to the reset * line, we can describe it */ vendor,some-optional-reset-gpio-property; ... }; If we want a separate device in the Linux driver model to abstract this, that's fine, but it should not be described in the dt. The device driver can call some generic helpers to handle creating said reset device in the driver model. Thanks, Mark.
On 08/02/2013 03:28 AM, Mark Rutland wrote: > On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: >> This driver implements a reset controller device that toggle a gpio >> connected to a reset pin of a peripheral IC. The delay between assertion >> and de-assertion of the reset signal can be configured via device tree. >> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt >> +GPIO reset controller >> +===================== >> + >> +A GPIO reset controller controls a single GPIO that is connected to the reset >> +pin of a peripheral IC. Please also refer to reset.txt in this directory for >> +common reset controller binding usage. >> + >> +Required properties: >> +- compatible: Should be "gpio-reset" >> +- reset-gpios: A gpio used as reset line. The gpio specifier for this property >> + depends on the gpio controller that provides the gpio. >> +- #reset-cells: 0, see below >> + >> +Optional properties: >> +- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for >> + this duration to reset. >> +- initially-in-reset: boolean. If not set, the initial state should be a >> + deasserted reset line. If this property exists, the >> + reset line should be kept in reset. >> + >> +example: >> + >> +sii902x_reset: gpio-reset { >> + compatible = "gpio-reset"; >> + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; >> + reset-delay-us = <10000>; >> + initially-in-reset; >> + #reset-cells = <0>; >> +}; >> + >> +/* Device with nRESET pin connected to GPIO5_0 */ >> +sii902x@39 { >> + /* ... */ >> + resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */ >> +}; > > I don't like the approach here. The reset GPIO line is not a device in > itself, and surely the way in which it must be toggled to reset a device > is a property of that device, not the GPIO. We're leaking a Linux > internal abstraction rather than describing the hardware. At first, I was going to say I disagree completely, but I do see your point. I don't think that having a separate reset controller concept in DT, and having a GPIO implementation of that, is purely exposing Linux concepts in the DT. It's just one way of looking at the HW. As you say, another is that any external chip that needs reset has a GPIO input. The history here is that we have to concept of IP modules inside a chip that can be reset, and that reset is driven by some external entity to the IP block, and hence there's a concept of a "reset controller" that drives that reset. That enables arbitrary different implementations to cover the case of the IP block being dropped into different SoCs with different ways of resetting it. Extend that same concept to external chips that happen to have GPIOs driving the chip's reset inputs, and you get this GPIO reset controller binding. On the surface, any external chip is going to be reset by a GPIO, and hence that should be represented by a property in that external chip's DT node, just as you say. But what if the reset pin is /not/ hooked up to a GPIO? What if there's a dedicated "reset" HW device that drives the reset input, and all it can do is pulse the reset, not maintain it at some static level, and hence that signal can't be represented as a GPIO? Or what if the core of the external chip gets integrated into an SoC, which has a reset controller rather than a GPIO input? Of course, I have no idea how likely this would be. To cover those cases, continuing to abstract the reset input semantically as a reset input, rather than as a plain GPIO, might make sense. > I think thie linkage of the gpio would be better described on the node > for the device with the reset input, in a binding-specific property for > the device (e.g. names for the specific input line on the device): > > I'd imagine the delay required would be fixed for a device (or possibly > influenced by clock inputs), and can either be knwon by the driver > without dt information, or discovered elsewhere (e.g. dervied from the > rates of clock inputs). We shuold be able to derive that from the > compatible string in most cases. > > I think this should look more like the below: > > /* Device with nRESET pin connected to GPIO5_0 */ > sii902x@39 { > /* named for the actual input line */ > nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > /* > * If there's some configurable property relating to the reset > * line, we can describe it > */ > vendor,some-optional-reset-gpio-property; > ... > }; If we decide to ignore the case where the reset signal isn't driven by a GPIO, yes, the above binding does look better.
Hi Mark, Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland: > On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: > > This driver implements a reset controller device that toggle a gpio > > connected to a reset pin of a peripheral IC. The delay between assertion > > and de-assertion of the reset signal can be configured via device tree. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > Reviewed-by: Stephen Warren <swarren@nvidia.com> > > --- > > Changes since v9: > > - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert > > should not be called from atomic context. > > --- > > .../devicetree/bindings/reset/gpio-reset.txt | 35 ++++ > > drivers/reset/Kconfig | 11 ++ > > drivers/reset/Makefile | 1 + > > drivers/reset/gpio-reset.c | 184 +++++++++++++++++++++ > > 4 files changed, 231 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.txt > > create mode 100644 drivers/reset/gpio-reset.c > > > > diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt > > new file mode 100644 > > index 0000000..bca5348 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/reset/gpio-reset.txt > > @@ -0,0 +1,35 @@ > > +GPIO reset controller > > +===================== > > + > > +A GPIO reset controller controls a single GPIO that is connected to the reset > > +pin of a peripheral IC. Please also refer to reset.txt in this directory for > > +common reset controller binding usage. > > + > > +Required properties: > > +- compatible: Should be "gpio-reset" > > +- reset-gpios: A gpio used as reset line. The gpio specifier for this property > > + depends on the gpio controller that provides the gpio. > > +- #reset-cells: 0, see below > > + > > +Optional properties: > > +- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for > > + this duration to reset. > > +- initially-in-reset: boolean. If not set, the initial state should be a > > + deasserted reset line. If this property exists, the > > + reset line should be kept in reset. > > + > > +example: > > + > > +sii902x_reset: gpio-reset { > > + compatible = "gpio-reset"; > > + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > > + reset-delay-us = <10000>; > > + initially-in-reset; > > + #reset-cells = <0>; > > +}; > > + > > +/* Device with nRESET pin connected to GPIO5_0 */ > > +sii902x@39 { > > + /* ... */ > > + resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */ > > +}; > > I don't like the approach here. The reset GPIO line is not a device in > itself, and surely the way in which it must be toggled to reset a device > is a property of that device, not the GPIO. We're leaking a Linux > internal abstraction rather than describing teh hardware. you are right, this results from the reset bindings being designed for actual hardware reset controllers that appeared in several SoCs. Initially I've sent the GPIO reset driver only as an example. > I think thie linkage of the gpio would be better described on the node > for the device with the reset input, in a binding-specific property for > the device (e.g. names for the specific input line on the device): > > I'd imagine the delay required would be fixed for a device (or possibly > influenced by clock inputs), and can either be knwon by the driver > without dt information, or discovered elsewhere (e.g. dervied from the > rates of clock inputs). We shuold be able to derive that from the > compatible string in most cases. > > I think this should look more like the below: > > /* Device with nRESET pin connected to GPIO5_0 */ > sii902x@39 { > /* named for the actual input line */ > nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > /* > * If there's some configurable property relating to the reset > * line, we can describe it > */ > vendor,some-optional-reset-gpio-property; > ... > }; I don't like the arbitrary name, as that makes it difficult to handle this in an automated way. In this case I'd prefer to use 'reset-gpios' and optionally 'reset-gpio-names' analogous to how clocks and interrupts (and resets) are handled. The framework itself could then also check the reset-gpios property and create the gpio reset controllers on the fly: sii902x@39 { reset-gpios <&gpio5 0 GPIO_ACTIVE_LOW>; reset-gpio-names = "nreset"; reset-delay-us = <10000>; initially-in-reset; }; But this reintroduces the (currently theoretical) problem of how to express delays and initially-in-reset information for multiple gpio resets on a single device if that information cannot be determined by the driver automatically. If there is a fixed delay (or a delay calculated from a known clock), we can just extend the API: device_reset_usleep(dev, 1000); instead of currently: device_reset(dev); > If we want a separate device in the Linux driver model to abstract this, > that's fine, but it should not be described in the dt. The device driver > can call some generic helpers to handle creating said reset device in > the driver model. regards Philipp
On Fri, Aug 02, 2013 at 09:09:35PM +0100, Stephen Warren wrote: > On 08/02/2013 03:28 AM, Mark Rutland wrote: > > On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: > >> This driver implements a reset controller device that toggle a gpio > >> connected to a reset pin of a peripheral IC. The delay between assertion > >> and de-assertion of the reset signal can be configured via device tree. > > >> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt > > >> +GPIO reset controller > >> +===================== > >> + > >> +A GPIO reset controller controls a single GPIO that is connected to the reset > >> +pin of a peripheral IC. Please also refer to reset.txt in this directory for > >> +common reset controller binding usage. > >> + > >> +Required properties: > >> +- compatible: Should be "gpio-reset" > >> +- reset-gpios: A gpio used as reset line. The gpio specifier for this property > >> + depends on the gpio controller that provides the gpio. > >> +- #reset-cells: 0, see below > >> + > >> +Optional properties: > >> +- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for > >> + this duration to reset. > >> +- initially-in-reset: boolean. If not set, the initial state should be a > >> + deasserted reset line. If this property exists, the > >> + reset line should be kept in reset. > >> + > >> +example: > >> + > >> +sii902x_reset: gpio-reset { > >> + compatible = "gpio-reset"; > >> + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > >> + reset-delay-us = <10000>; > >> + initially-in-reset; > >> + #reset-cells = <0>; > >> +}; > >> + > >> +/* Device with nRESET pin connected to GPIO5_0 */ > >> +sii902x@39 { > >> + /* ... */ > >> + resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */ > >> +}; > > > > I don't like the approach here. The reset GPIO line is not a device in > > itself, and surely the way in which it must be toggled to reset a device > > is a property of that device, not the GPIO. We're leaking a Linux > > internal abstraction rather than describing the hardware. > > At first, I was going to say I disagree completely, but I do see your point. > > I don't think that having a separate reset controller concept in DT, and > having a GPIO implementation of that, is purely exposing Linux concepts > in the DT. It's just one way of looking at the HW. As you say, another > is that any external chip that needs reset has a GPIO input. > > The history here is that we have to concept of IP modules inside a chip > that can be reset, and that reset is driven by some external entity to > the IP block, and hence there's a concept of a "reset controller" that > drives that reset. That enables arbitrary different implementations to > cover the case of the IP block being dropped into different SoCs with > different ways of resetting it. > > Extend that same concept to external chips that happen to have GPIOs > driving the chip's reset inputs, and you get this GPIO reset controller > binding. > > On the surface, any external chip is going to be reset by a GPIO, and > hence that should be represented by a property in that external chip's > DT node, just as you say. > > But what if the reset pin is /not/ hooked up to a GPIO? What if there's > a dedicated "reset" HW device that drives the reset input, and all it > can do is pulse the reset, not maintain it at some static level, and > hence that signal can't be represented as a GPIO? Or what if the core of > the external chip gets integrated into an SoC, which has a reset > controller rather than a GPIO input? Of course, I have no idea how > likely this would be. > > To cover those cases, continuing to abstract the reset input > semantically as a reset input, rather than as a plain GPIO, might make > sense. That does sound reasonable. Do we have any of the above cases described in any existing DTs? If I've not misunderstood, there seem to be a lot of *reset-gpio* properties Documented (e.g. nvidia,phy-reset-gpio) which look like what I was suggesting (though perhaps that's a different meaning of reset, I'm not that familiar with PHYs). Given it's a common pattern already, I think it would make sense to allow for that style of binding (in addition to supporting more complex reset devices as required). The reset bindings (Documentation/devicetree/bindings/reset/reset.txt) seem to suggest that both are reasonable. > > > I think thie linkage of the gpio would be better described on the node > > for the device with the reset input, in a binding-specific property for > > the device (e.g. names for the specific input line on the device): > > > > I'd imagine the delay required would be fixed for a device (or possibly > > influenced by clock inputs), and can either be knwon by the driver > > without dt information, or discovered elsewhere (e.g. dervied from the > > rates of clock inputs). We shuold be able to derive that from the > > compatible string in most cases. > > > > I think this should look more like the below: > > > > /* Device with nRESET pin connected to GPIO5_0 */ > > sii902x@39 { > > /* named for the actual input line */ > > nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > > /* > > * If there's some configurable property relating to the reset > > * line, we can describe it > > */ > > vendor,some-optional-reset-gpio-property; > > ... > > }; > > If we decide to ignore the case where the reset signal isn't driven by a > GPIO, yes, the above binding does look better. > For the GPIO case, I'd prefer to describe the GPIO input directly. I'm not opposed to also supporting the description of more complex reset devices. I believe it's feasible to support both? Thanks, Mark.
On Mon, Aug 05, 2013 at 08:32:16AM +0100, Philipp Zabel wrote: > Hi Mark, Hi Philipp, > > Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland: > > On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: > > > This driver implements a reset controller device that toggle a gpio > > > connected to a reset pin of a peripheral IC. The delay between assertion > > > and de-assertion of the reset signal can be configured via device tree. > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > Reviewed-by: Stephen Warren <swarren@nvidia.com> > > > --- > > > Changes since v9: > > > - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert > > > should not be called from atomic context. > > > --- > > > .../devicetree/bindings/reset/gpio-reset.txt | 35 ++++ > > > drivers/reset/Kconfig | 11 ++ > > > drivers/reset/Makefile | 1 + > > > drivers/reset/gpio-reset.c | 184 +++++++++++++++++++++ > > > 4 files changed, 231 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.txt > > > create mode 100644 drivers/reset/gpio-reset.c > > > > > > diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt > > > new file mode 100644 > > > index 0000000..bca5348 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/reset/gpio-reset.txt > > > @@ -0,0 +1,35 @@ > > > +GPIO reset controller > > > +===================== > > > + > > > +A GPIO reset controller controls a single GPIO that is connected to the reset > > > +pin of a peripheral IC. Please also refer to reset.txt in this directory for > > > +common reset controller binding usage. > > > + > > > +Required properties: > > > +- compatible: Should be "gpio-reset" > > > +- reset-gpios: A gpio used as reset line. The gpio specifier for this property > > > + depends on the gpio controller that provides the gpio. > > > +- #reset-cells: 0, see below > > > + > > > +Optional properties: > > > +- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for > > > + this duration to reset. > > > +- initially-in-reset: boolean. If not set, the initial state should be a > > > + deasserted reset line. If this property exists, the > > > + reset line should be kept in reset. > > > + > > > +example: > > > + > > > +sii902x_reset: gpio-reset { > > > + compatible = "gpio-reset"; > > > + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > > > + reset-delay-us = <10000>; > > > + initially-in-reset; > > > + #reset-cells = <0>; > > > +}; > > > + > > > +/* Device with nRESET pin connected to GPIO5_0 */ > > > +sii902x@39 { > > > + /* ... */ > > > + resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */ > > > +}; > > > > I don't like the approach here. The reset GPIO line is not a device in > > itself, and surely the way in which it must be toggled to reset a device > > is a property of that device, not the GPIO. We're leaking a Linux > > internal abstraction rather than describing teh hardware. > > you are right, this results from the reset bindings being designed for > actual hardware reset controllers that appeared in several SoCs. > Initially I've sent the GPIO reset driver only as an example. > > > I think thie linkage of the gpio would be better described on the node > > for the device with the reset input, in a binding-specific property for > > the device (e.g. names for the specific input line on the device): > > > > I'd imagine the delay required would be fixed for a device (or possibly > > influenced by clock inputs), and can either be knwon by the driver > > without dt information, or discovered elsewhere (e.g. dervied from the > > rates of clock inputs). We shuold be able to derive that from the > > compatible string in most cases. > > > > I think this should look more like the below: > > > > /* Device with nRESET pin connected to GPIO5_0 */ > > sii902x@39 { > > /* named for the actual input line */ > > nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > > /* > > * If there's some configurable property relating to the reset > > * line, we can describe it > > */ > > vendor,some-optional-reset-gpio-property; > > ... > > }; > > I don't like the arbitrary name, as that makes it difficult to handle > this in an automated way. In this case I'd prefer to use 'reset-gpios' > and optionally 'reset-gpio-names' analogous to how clocks and interrupts > (and resets) are handled. The framework itself could then also check the > reset-gpios property and create the gpio reset controllers on the fly: I'm not sure about the structure you suggest. The main reason we'd want to tell apart multiple reset GPIOs (via reset-gpio-names) would be that the device needs to have said GPIOs poked in a device-specific manner (i.e. with some ordering and timing constraints, possibly only a subset of the GPIOs needing to be poked to reset components of the device). Given that, I'm not sure what separating the GPIOs out into a reset-gpios property buys us, as the driver still needs to handle them (at least to provide some additional information to device reset framework code). > > sii902x@39 { > reset-gpios <&gpio5 0 GPIO_ACTIVE_LOW>; > reset-gpio-names = "nreset"; > reset-delay-us = <10000>; > initially-in-reset; > }; > > But this reintroduces the (currently theoretical) problem of how to > express delays and initially-in-reset information for multiple gpio > resets on a single device if that information cannot be determined by > the driver automatically. > > If there is a fixed delay (or a delay calculated from a known clock), we > can just extend the API: > device_reset_usleep(dev, 1000); > instead of currently: > device_reset(dev); I was working on the assumption that any required delay would be a function of the devices input clocks (or fixed, if the device has some fixed rate internal clock). If that's true, the API extension you suggest makes sense to me. I'm not sure about initially-in-reset. Is it not possible for the driver to poke the GPIO controller to ensure the GPIOs are in the state required? Thanks, Mark.
On 08/05/2013 09:13 AM, Mark Rutland wrote: > On Fri, Aug 02, 2013 at 09:09:35PM +0100, Stephen Warren wrote: >> On 08/02/2013 03:28 AM, Mark Rutland wrote: >>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: >>>> This driver implements a reset controller device that toggle a gpio >>>> connected to a reset pin of a peripheral IC. The delay between assertion >>>> and de-assertion of the reset signal can be configured via device tree. >> >>>> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt >> >>>> +GPIO reset controller >>>> +===================== >>>> + >>>> +A GPIO reset controller controls a single GPIO that is connected to the reset >>>> +pin of a peripheral IC. Please also refer to reset.txt in this directory for >>>> +common reset controller binding usage. >>>> + >>>> +Required properties: >>>> +- compatible: Should be "gpio-reset" >>>> +- reset-gpios: A gpio used as reset line. The gpio specifier for this property >>>> + depends on the gpio controller that provides the gpio. >>>> +- #reset-cells: 0, see below >>>> + >>>> +Optional properties: >>>> +- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for >>>> + this duration to reset. >>>> +- initially-in-reset: boolean. If not set, the initial state should be a >>>> + deasserted reset line. If this property exists, the >>>> + reset line should be kept in reset. >>>> + >>>> +example: >>>> + >>>> +sii902x_reset: gpio-reset { >>>> + compatible = "gpio-reset"; >>>> + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; >>>> + reset-delay-us = <10000>; >>>> + initially-in-reset; >>>> + #reset-cells = <0>; >>>> +}; >>>> + >>>> +/* Device with nRESET pin connected to GPIO5_0 */ >>>> +sii902x@39 { >>>> + /* ... */ >>>> + resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */ >>>> +}; >>> >>> I don't like the approach here. The reset GPIO line is not a device in >>> itself, and surely the way in which it must be toggled to reset a device >>> is a property of that device, not the GPIO. We're leaking a Linux >>> internal abstraction rather than describing the hardware. >> >> At first, I was going to say I disagree completely, but I do see your point. >> >> I don't think that having a separate reset controller concept in DT, and >> having a GPIO implementation of that, is purely exposing Linux concepts >> in the DT. It's just one way of looking at the HW. As you say, another >> is that any external chip that needs reset has a GPIO input. >> >> The history here is that we have to concept of IP modules inside a chip >> that can be reset, and that reset is driven by some external entity to >> the IP block, and hence there's a concept of a "reset controller" that >> drives that reset. That enables arbitrary different implementations to >> cover the case of the IP block being dropped into different SoCs with >> different ways of resetting it. >> >> Extend that same concept to external chips that happen to have GPIOs >> driving the chip's reset inputs, and you get this GPIO reset controller >> binding. >> >> On the surface, any external chip is going to be reset by a GPIO, and >> hence that should be represented by a property in that external chip's >> DT node, just as you say. >> >> But what if the reset pin is /not/ hooked up to a GPIO? What if there's >> a dedicated "reset" HW device that drives the reset input, and all it >> can do is pulse the reset, not maintain it at some static level, and >> hence that signal can't be represented as a GPIO? Or what if the core of >> the external chip gets integrated into an SoC, which has a reset >> controller rather than a GPIO input? Of course, I have no idea how >> likely this would be. >> >> To cover those cases, continuing to abstract the reset input >> semantically as a reset input, rather than as a plain GPIO, might make >> sense. > > That does sound reasonable. Do we have any of the above cases described > in any existing DTs? I haven't explicitly checked, so I don't know. My argument was admittedly entirely theoretical. > If I've not misunderstood, there seem to be a lot of *reset-gpio* > properties Documented (e.g. nvidia,phy-reset-gpio) which look like what > I was suggesting (though perhaps that's a different meaning of reset, > I'm not that familiar with PHYs). I think that nvidia,phy-reset-gpio is exactly what we're talking about here. > Given it's a common pattern already, I > think it would make sense to allow for that style of binding (in > addition to supporting more complex reset devices as required). Yes, certainly. If for nothing else than "legacy" bindings:-) But I think explicit properties are a perfectly reasonable approach even going forward.
On 08/05/2013 01:32 AM, Philipp Zabel wrote: > Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland: >> On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: >>> This driver implements a reset controller device that toggle a gpio >>> connected to a reset pin of a peripheral IC. The delay between assertion >>> and de-assertion of the reset signal can be configured via device tree. ... >> I think this should look more like the below: >> >> /* Device with nRESET pin connected to GPIO5_0 */ >> sii902x@39 { >> /* named for the actual input line */ >> nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; >> /* >> * If there's some configurable property relating to the reset >> * line, we can describe it >> */ >> vendor,some-optional-reset-gpio-property; >> ... >> }; > > I don't like the arbitrary name, as that makes it difficult to handle > this in an automated way. In this case I'd prefer to use 'reset-gpios' > and optionally 'reset-gpio-names' analogous to how clocks and interrupts > (and resets) are handled. Hmm. Just be aware that you can't force existing bindings to be retro-actively modified, or you'll break the DT ABI. So, at the very least we'd have to allow the existing custom-property-based approach for bindings where it's already been used.
Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren: > On 08/05/2013 01:32 AM, Philipp Zabel wrote: > > Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland: > >> On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: > >>> This driver implements a reset controller device that toggle a gpio > >>> connected to a reset pin of a peripheral IC. The delay between assertion > >>> and de-assertion of the reset signal can be configured via device tree. > ... > >> I think this should look more like the below: > >> > >> /* Device with nRESET pin connected to GPIO5_0 */ > >> sii902x@39 { > >> /* named for the actual input line */ > >> nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > >> /* > >> * If there's some configurable property relating to the reset > >> * line, we can describe it > >> */ > >> vendor,some-optional-reset-gpio-property; > >> ... > >> }; > > > > I don't like the arbitrary name, as that makes it difficult to handle > > this in an automated way. In this case I'd prefer to use 'reset-gpios' > > and optionally 'reset-gpio-names' analogous to how clocks and interrupts > > (and resets) are handled. > > Hmm. Just be aware that you can't force existing bindings to be > retro-actively modified, or you'll break the DT ABI. So, at the very > least we'd have to allow the existing custom-property-based approach for > bindings where it's already been used. Of course we have to continue supporting existing bindings. We could continue using the GPIO API directly for those cases, or we could add a function similar to of_get_named_gpio to wrap the GPIO: rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0); reset_control_assert(rstc); usleep(1000); reset_control_deassert(rstc); My point is that for new bindings I'd prefer a well known property name such as reset-gpios and a -names string list (as described Documentation/devicetree/bindings/resource-names.txt) over ad-hoc property names such as nreset-gpios, <vendor>-<submodule>-(n)reset, nrst-gpios etc., both for consistency with existing resource properties and because it is easier to grep for a single property name than for a combination of all possible datasheet spellings of "reset". I'd like rstc = reset_control_get(dev, "nreset"); to go look for resets = <&src 3>; reset-names = "nreset"; or for reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; reset-gpio-names = "nreset"; by default. regards Philipp
Hi Mark, On 08/05/2013 06:35 PM, Mark Rutland wrote: > On Mon, Aug 05, 2013 at 08:32:16AM +0100, Philipp Zabel wrote: >> Hi Mark, > > Hi Philipp, > >> >> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland: > > I'm not sure about initially-in-reset. Is it not possible for the driver > to poke the GPIO controller to ensure the GPIOs are in the state > required? We need initially-in-reset, so that the device can be kept in reset even when the device driver is not available or loaded late in the boot cycle. cheers, -roger
On 08/06/2013 01:32 AM, Philipp Zabel wrote: > Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren: >> On 08/05/2013 01:32 AM, Philipp Zabel wrote: >>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland: >>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: >>>>> This driver implements a reset controller device that toggle a gpio >>>>> connected to a reset pin of a peripheral IC. The delay between assertion >>>>> and de-assertion of the reset signal can be configured via device tree. >> ... >>>> I think this should look more like the below: >>>> >>>> /* Device with nRESET pin connected to GPIO5_0 */ >>>> sii902x@39 { >>>> /* named for the actual input line */ >>>> nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; >>>> /* >>>> * If there's some configurable property relating to the reset >>>> * line, we can describe it >>>> */ >>>> vendor,some-optional-reset-gpio-property; >>>> ... >>>> }; >>> >>> I don't like the arbitrary name, as that makes it difficult to handle >>> this in an automated way. In this case I'd prefer to use 'reset-gpios' >>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts >>> (and resets) are handled. >> >> Hmm. Just be aware that you can't force existing bindings to be >> retro-actively modified, or you'll break the DT ABI. So, at the very >> least we'd have to allow the existing custom-property-based approach for >> bindings where it's already been used. > > Of course we have to continue supporting existing bindings. We could > continue using the GPIO API directly for those cases, or we could add a > function similar to of_get_named_gpio to wrap the GPIO: > > rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0); > reset_control_assert(rstc); > usleep(1000); > reset_control_deassert(rstc); Well, you'd need to pass two names into that function; one would be the name of the legacy property and the other the entry in reset-names or reset-gpio-names. It's quite unlikely that the same string would be used in both places. > My point is that for new bindings I'd prefer a well known property name > such as reset-gpios and a -names string list (as described > Documentation/devicetree/bindings/resource-names.txt) over ad-hoc > property names such as nreset-gpios, <vendor>-<submodule>-(n)reset, > nrst-gpios etc., both for consistency with existing resource properties > and because it is easier to grep for a single property name than for a > combination of all possible datasheet spellings of "reset". > > I'd like > rstc = reset_control_get(dev, "nreset"); > to go look for > resets = <&src 3>; > reset-names = "nreset"; > or for > reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > reset-gpio-names = "nreset"; > by default. That's rather complex for little benefit though. Take a look at existing plain GPIO bindings, regulators, etc. They all simply encode the name you're looking for directly into the property name, which is a lot less overhead; simpler for humans to write and read without having to match n properties together, and simpler to parse in code.
Hi Stephen Am Dienstag, den 06.08.2013, 10:59 -0600 schrieb Stephen Warren: > On 08/06/2013 01:32 AM, Philipp Zabel wrote: > > Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren: > >> On 08/05/2013 01:32 AM, Philipp Zabel wrote: > >>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland: > >>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: > >>>>> This driver implements a reset controller device that toggle a gpio > >>>>> connected to a reset pin of a peripheral IC. The delay between assertion > >>>>> and de-assertion of the reset signal can be configured via device tree. > >> ... > >>>> I think this should look more like the below: > >>>> > >>>> /* Device with nRESET pin connected to GPIO5_0 */ > >>>> sii902x@39 { > >>>> /* named for the actual input line */ > >>>> nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > >>>> /* > >>>> * If there's some configurable property relating to the reset > >>>> * line, we can describe it > >>>> */ > >>>> vendor,some-optional-reset-gpio-property; > >>>> ... > >>>> }; > >>> > >>> I don't like the arbitrary name, as that makes it difficult to handle > >>> this in an automated way. In this case I'd prefer to use 'reset-gpios' > >>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts > >>> (and resets) are handled. > >> > >> Hmm. Just be aware that you can't force existing bindings to be > >> retro-actively modified, or you'll break the DT ABI. So, at the very > >> least we'd have to allow the existing custom-property-based approach for > >> bindings where it's already been used. > > > > Of course we have to continue supporting existing bindings. We could > > continue using the GPIO API directly for those cases, or we could add a > > function similar to of_get_named_gpio to wrap the GPIO: > > > > rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0); > > reset_control_assert(rstc); > > usleep(1000); > > reset_control_deassert(rstc); > > Well, you'd need to pass two names into that function; one would be the > name of the legacy property and the other the entry in reset-names or > reset-gpio-names. It's quite unlikely that the same string would be used > in both places. there is no reset-names here. The legacy properties are only one GPIO per property or addressed by index, currently. I don't want to change that. > > My point is that for new bindings I'd prefer a well known property name > > such as reset-gpios and a -names string list (as described > > Documentation/devicetree/bindings/resource-names.txt) over ad-hoc > > property names such as nreset-gpios, <vendor>-<submodule>-(n)reset, > > nrst-gpios etc., both for consistency with existing resource properties > > and because it is easier to grep for a single property name than for a > > combination of all possible datasheet spellings of "reset". ^ This is my main concern. > > I'd like > > rstc = reset_control_get(dev, "nreset"); > > to go look for > > resets = <&src 3>; > > reset-names = "nreset"; > > or for > > reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > > reset-gpio-names = "nreset"; > > by default. > > That's rather complex for little benefit though. In the majority of cases there will only be one reset per device. In this case, the supplemental names property is not needed. The resets/reset-names binding is using this scheme already, so using the same for reset-gpios improves consistency. If it weren't for the customary *-gpios property names, I'd suggest to allow using GPIOs in the resets property directly: resets = <&gpio5 0 GPIO_ACTIVE_LOW>; > Take a look at existing > plain GPIO bindings, regulators, etc. They all simply encode the name > you're looking for directly into the property name, which is a lot less > overhead; simpler for humans to write and read without having to match n > properties together, and simpler to parse in code. It is not simpler to parse for humans if there is not a clear naming scheme. For regulators, I can at least grep for the common "-supply" suffix to recognize regulator supplies. But the "nrst-gpios" won't be easily found. And if the datasheet calls the pin "RESET_N", or even "CE", how will I call the reset property? I'd strongly prefer to standardize on a recognizable name. regards Philipp
On 08/08/2013 03:42 AM, Philipp Zabel wrote: > Hi Stephen > > Am Dienstag, den 06.08.2013, 10:59 -0600 schrieb Stephen Warren: >> On 08/06/2013 01:32 AM, Philipp Zabel wrote: >>> Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren: >>>> On 08/05/2013 01:32 AM, Philipp Zabel wrote: >>>>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland: >>>>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: >>>>>>> This driver implements a reset controller device that toggle a gpio >>>>>>> connected to a reset pin of a peripheral IC. The delay between assertion >>>>>>> and de-assertion of the reset signal can be configured via device tree. >>>> ... >>>>>> I think this should look more like the below: >>>>>> >>>>>> /* Device with nRESET pin connected to GPIO5_0 */ >>>>>> sii902x@39 { >>>>>> /* named for the actual input line */ >>>>>> nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; >>>>>> /* >>>>>> * If there's some configurable property relating to the reset >>>>>> * line, we can describe it >>>>>> */ >>>>>> vendor,some-optional-reset-gpio-property; >>>>>> ... >>>>>> }; >>>>> >>>>> I don't like the arbitrary name, as that makes it difficult to handle >>>>> this in an automated way. In this case I'd prefer to use 'reset-gpios' >>>>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts >>>>> (and resets) are handled. >>>> >>>> Hmm. Just be aware that you can't force existing bindings to be >>>> retro-actively modified, or you'll break the DT ABI. So, at the very >>>> least we'd have to allow the existing custom-property-based approach for >>>> bindings where it's already been used. >>> >>> Of course we have to continue supporting existing bindings. We could >>> continue using the GPIO API directly for those cases, or we could add a >>> function similar to of_get_named_gpio to wrap the GPIO: >>> >>> rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0); >>> reset_control_assert(rstc); >>> usleep(1000); >>> reset_control_deassert(rstc); >> >> Well, you'd need to pass two names into that function; one would be the >> name of the legacy property and the other the entry in reset-names or >> reset-gpio-names. It's quite unlikely that the same string would be used >> in both places. > > there is no reset-names here. The legacy properties are only one GPIO > per property or addressed by index, currently. I don't want to change > that. So do you foresee: 1) Making zero changes to the existing binding and just keeping the various custom stuff that some have or 2) Migrating existing bindings to the new scheme, and simply deprecating, but still allowing, the old custom properties. If (1), then yes, of_get_named_reset_control() would only need one parameter, either the name of the existing custom property or the reset-names value, depending on whether the binding was old or new. This might be problematic, since if you pass in "foo" expecting that to be a reset-names entry, it would also accidentally allow a property named "foo" to provide the information, even though that wasn't defined in the binding. Equally, if every binding is either-or, you may as well force drivers passing the binding to just call different APIs based on their binding definition. If (2), then you'd need to pass both the legacy property name and reset-names value separately, since I imagine those values would be different, consider: old: nvidia,phy-reset-gpio = <&gpio 1 0>; new: reset-gpios = <&gpio 1 0>; reset-names = "phy"; >>> My point is that for new bindings I'd prefer a well known property name >>> such as reset-gpios and a -names string list (as described >>> Documentation/devicetree/bindings/resource-names.txt) over ad-hoc >>> property names such as nreset-gpios, <vendor>-<submodule>-(n)reset, >>> nrst-gpios etc., both for consistency with existing resource properties >>> and because it is easier to grep for a single property name than for a >>> combination of all possible datasheet spellings of "reset". > > ^ This is my main concern. > >>> I'd like >>> rstc = reset_control_get(dev, "nreset"); >>> to go look for >>> resets = <&src 3>; >>> reset-names = "nreset"; >>> or for >>> reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; >>> reset-gpio-names = "nreset"; >>> by default. >> >> That's rather complex for little benefit though. > > In the majority of cases there will only be one reset per device. In > this case, the supplemental names property is not needed. The > resets/reset-names binding is using this scheme already, so using the > same for reset-gpios improves consistency. I think that if there is just a "foo" property, it should always be accessed by index into the list, whereas if there are both a foo and foo-names property, it should always be accessed by name lookup. This makes the lookup order much clearer. There are some unfortunate exceptions to this today (regs and interrupts can have names, but those aren't used in all cases!), but I'd like to avoid propagating that mistake.
Am Donnerstag, den 08.08.2013, 12:43 -0600 schrieb Stephen Warren: > On 08/08/2013 03:42 AM, Philipp Zabel wrote: > > Hi Stephen > > > > Am Dienstag, den 06.08.2013, 10:59 -0600 schrieb Stephen Warren: > >> On 08/06/2013 01:32 AM, Philipp Zabel wrote: > >>> Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren: > >>>> On 08/05/2013 01:32 AM, Philipp Zabel wrote: > >>>>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland: > >>>>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, Philipp Zabel wrote: > >>>>>>> This driver implements a reset controller device that toggle a gpio > >>>>>>> connected to a reset pin of a peripheral IC. The delay between assertion > >>>>>>> and de-assertion of the reset signal can be configured via device tree. > >>>> ... > >>>>>> I think this should look more like the below: > >>>>>> > >>>>>> /* Device with nRESET pin connected to GPIO5_0 */ > >>>>>> sii902x@39 { > >>>>>> /* named for the actual input line */ > >>>>>> nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; > >>>>>> /* > >>>>>> * If there's some configurable property relating to the reset > >>>>>> * line, we can describe it > >>>>>> */ > >>>>>> vendor,some-optional-reset-gpio-property; > >>>>>> ... > >>>>>> }; > >>>>> > >>>>> I don't like the arbitrary name, as that makes it difficult to handle > >>>>> this in an automated way. In this case I'd prefer to use 'reset-gpios' > >>>>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts > >>>>> (and resets) are handled. > >>>> > >>>> Hmm. Just be aware that you can't force existing bindings to be > >>>> retro-actively modified, or you'll break the DT ABI. So, at the very > >>>> least we'd have to allow the existing custom-property-based approach for > >>>> bindings where it's already been used. > >>> > >>> Of course we have to continue supporting existing bindings. We could > >>> continue using the GPIO API directly for those cases, or we could add a > >>> function similar to of_get_named_gpio to wrap the GPIO: > >>> > >>> rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0); > >>> reset_control_assert(rstc); > >>> usleep(1000); > >>> reset_control_deassert(rstc); > >> > >> Well, you'd need to pass two names into that function; one would be the > >> name of the legacy property and the other the entry in reset-names or > >> reset-gpio-names. It's quite unlikely that the same string would be used > >> in both places. > > > > there is no reset-names here. The legacy properties are only one GPIO > > per property or addressed by index, currently. I don't want to change > > that. > > So do you foresee: > > 1) Making zero changes to the existing binding and just keeping the > various custom stuff that some have > > or > > 2) Migrating existing bindings to the new scheme, and simply > deprecating, but still allowing, the old custom properties. Yes, I'd like to deprecate the (<vendor>,)<custom>-gpio binding scheme for reset gpios, on a case by case basis. > If (1), then yes, of_get_named_reset_control() would only need one > parameter, either the name of the existing custom property or the > reset-names value, depending on whether the binding was old or new. > > This might be problematic, since if you pass in "foo" expecting that to > be a reset-names entry, it would also accidentally allow a property > named "foo" to provide the information, even though that wasn't defined > in the binding. Yes, of_get_named_reset_control() was just an example, to be used for the legacy case only. > Equally, if every binding is either-or, you may as well force drivers > passing the binding to just call different APIs based on their binding > definition. > > If (2), then you'd need to pass both the legacy property name and > reset-names value separately, since I imagine those values would be > different, consider: > > old: > > nvidia,phy-reset-gpio = <&gpio 1 0>; > > new: > > reset-gpios = <&gpio 1 0>; > reset-names = "phy"; Correct. In this case: rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0); if (IS_ERR(rstc)) { ret = PTR_ERR(rstc); if (ret == -EPROBE_DEFER) return ret; rstc = reset_control_get(dev, "phy"); if (IS_ERR(rstc)) return PTR_ERR(rstc); } Maybe encapsulate this in a convenience wrapper: rstc = reset_control_get_legacy(dev, "phy", "nvidia,phy-reset-gpio"); > > In the majority of cases there will only be one reset per device. In > > this case, the supplemental names property is not needed. The > > resets/reset-names binding is using this scheme already, so using the > > same for reset-gpios improves consistency. > > I think that if there is just a "foo" property, it should always be > accessed by index into the list, whereas if there are both a foo and > foo-names property, it should always be accessed by name lookup. This > makes the lookup order much clearer. There are some unfortunate > exceptions to this today (regs and interrupts can have names, but those > aren't used in all cases!), but I'd like to avoid propagating that mistake. Agreed. Looking up regs and interrupts by index is error-prone if there are more than one, but surely leaving the -names property out in case there is only one entry should be fine? regards Philipp
On 08/12/2013 05:04 AM, Philipp Zabel wrote: > Am Donnerstag, den 08.08.2013, 12:43 -0600 schrieb Stephen Warren: ... >> Equally, if every binding is either-or, you may as well force drivers >> passing the binding to just call different APIs based on their binding >> definition. >> >> If (2), then you'd need to pass both the legacy property name and >> reset-names value separately, since I imagine those values would be >> different, consider: >> >> old: >> >> nvidia,phy-reset-gpio = <&gpio 1 0>; >> >> new: >> >> reset-gpios = <&gpio 1 0>; >> reset-names = "phy"; > > Correct. In this case: > > rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0); > if (IS_ERR(rstc)) { > ret = PTR_ERR(rstc); > if (ret == -EPROBE_DEFER) > return ret; > rstc = reset_control_get(dev, "phy"); > if (IS_ERR(rstc)) > return PTR_ERR(rstc); > } > > Maybe encapsulate this in a convenience wrapper: > > rstc = reset_control_get_legacy(dev, "phy", "nvidia,phy-reset-gpio"); Yes, that's exactly what I envisaged. Thanks.
diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt new file mode 100644 index 0000000..bca5348 --- /dev/null +++ b/Documentation/devicetree/bindings/reset/gpio-reset.txt @@ -0,0 +1,35 @@ +GPIO reset controller +===================== + +A GPIO reset controller controls a single GPIO that is connected to the reset +pin of a peripheral IC. Please also refer to reset.txt in this directory for +common reset controller binding usage. + +Required properties: +- compatible: Should be "gpio-reset" +- reset-gpios: A gpio used as reset line. The gpio specifier for this property + depends on the gpio controller that provides the gpio. +- #reset-cells: 0, see below + +Optional properties: +- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for + this duration to reset. +- initially-in-reset: boolean. If not set, the initial state should be a + deasserted reset line. If this property exists, the + reset line should be kept in reset. + +example: + +sii902x_reset: gpio-reset { + compatible = "gpio-reset"; + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>; + reset-delay-us = <10000>; + initially-in-reset; + #reset-cells = <0>; +}; + +/* Device with nRESET pin connected to GPIO5_0 */ +sii902x@39 { + /* ... */ + resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */ +}; diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index c9d04f7..1a862df 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -11,3 +11,14 @@ menuconfig RESET_CONTROLLER via GPIOs or SoC-internal reset controller modules. If unsure, say no. + +if RESET_CONTROLLER + +config RESET_GPIO + tristate "GPIO reset controller support" + depends on GPIOLIB && OF + help + This driver provides support for reset lines that are controlled + directly by GPIOs. + +endif diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index 1e2d83f..b854f20 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -1 +1,2 @@ obj-$(CONFIG_RESET_CONTROLLER) += core.o +obj-$(CONFIG_RESET_GPIO) += gpio-reset.o diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c new file mode 100644 index 0000000..3049b22 --- /dev/null +++ b/drivers/reset/gpio-reset.c @@ -0,0 +1,184 @@ +/* + * GPIO Reset Controller driver + * + * Copyright 2013 Philipp Zabel, Pengutronix + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/module.h> +#include <linux/of_gpio.h> +#include <linux/platform_device.h> +#include <linux/reset-controller.h> + +struct gpio_reset_data { + struct reset_controller_dev rcdev; + unsigned int gpio; + bool active_low; + s32 delay_us; +}; + +static void gpio_reset_set(struct reset_controller_dev *rcdev, int asserted) +{ + struct gpio_reset_data *drvdata = container_of(rcdev, + struct gpio_reset_data, rcdev); + int value = asserted; + + if (drvdata->active_low) + value = !value; + + gpio_set_value_cansleep(drvdata->gpio, value); +} + +static int gpio_reset(struct reset_controller_dev *rcdev, unsigned long id) +{ + struct gpio_reset_data *drvdata = container_of(rcdev, + struct gpio_reset_data, rcdev); + + if (drvdata->delay_us < 0) + return -ENOSYS; + + gpio_reset_set(rcdev, 1); + udelay(drvdata->delay_us); + gpio_reset_set(rcdev, 0); + + return 0; +} + +static int gpio_reset_assert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + gpio_reset_set(rcdev, 1); + + return 0; +} + +static int gpio_reset_deassert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + gpio_reset_set(rcdev, 0); + + return 0; +} + +static struct reset_control_ops gpio_reset_ops = { + .reset = gpio_reset, + .assert = gpio_reset_assert, + .deassert = gpio_reset_deassert, +}; + +static int of_gpio_reset_xlate(struct reset_controller_dev *rcdev, + const struct of_phandle_args *reset_spec) +{ + if (WARN_ON(reset_spec->args_count != 0)) + return -EINVAL; + + return 0; +} + +static int gpio_reset_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct gpio_reset_data *drvdata; + enum of_gpio_flags flags; + unsigned long gpio_flags; + bool initially_in_reset; + int ret; + + drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL); + if (drvdata == NULL) + return -ENOMEM; + + if (of_gpio_named_count(np, "reset-gpios") != 1) { + dev_err(&pdev->dev, + "reset-gpios property missing, or not a single gpio\n"); + return -EINVAL; + } + + drvdata->gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags); + if (drvdata->gpio == -EPROBE_DEFER) { + return drvdata->gpio; + } else if (!gpio_is_valid(drvdata->gpio)) { + dev_err(&pdev->dev, "invalid reset gpio: %d\n", drvdata->gpio); + return drvdata->gpio; + } + + drvdata->active_low = flags & OF_GPIO_ACTIVE_LOW; + + ret = of_property_read_u32(np, "reset-delay-us", &drvdata->delay_us); + if (ret < 0) + drvdata->delay_us = -1; + else if (drvdata->delay_us < 0) + dev_warn(&pdev->dev, "reset delay too high\n"); + + initially_in_reset = of_property_read_bool(np, "initially-in-reset"); + if (drvdata->active_low ^ initially_in_reset) + gpio_flags = GPIOF_OUT_INIT_HIGH; + else + gpio_flags = GPIOF_OUT_INIT_LOW; + + ret = devm_gpio_request_one(&pdev->dev, drvdata->gpio, gpio_flags, NULL); + if (ret < 0) { + dev_err(&pdev->dev, "failed to request gpio %d: %d\n", + drvdata->gpio, ret); + return ret; + } + + platform_set_drvdata(pdev, drvdata); + + drvdata->rcdev.of_node = np; + drvdata->rcdev.owner = THIS_MODULE; + drvdata->rcdev.nr_resets = 1; + drvdata->rcdev.ops = &gpio_reset_ops; + drvdata->rcdev.of_xlate = of_gpio_reset_xlate; + reset_controller_register(&drvdata->rcdev); + + return 0; +} + +static int gpio_reset_remove(struct platform_device *pdev) +{ + struct gpio_reset_data *drvdata = platform_get_drvdata(pdev); + + reset_controller_unregister(&drvdata->rcdev); + + return 0; +} + +static struct of_device_id gpio_reset_dt_ids[] = { + { .compatible = "gpio-reset" }, + { } +}; + +static struct platform_driver gpio_reset_driver = { + .probe = gpio_reset_probe, + .remove = gpio_reset_remove, + .driver = { + .name = "gpio-reset", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(gpio_reset_dt_ids), + }, +}; + +static int __init gpio_reset_init(void) +{ + return platform_driver_register(&gpio_reset_driver); +} +arch_initcall(gpio_reset_init); + +static void __exit gpio_reset_exit(void) +{ + platform_driver_unregister(&gpio_reset_driver); +} +module_exit(gpio_reset_exit); + +MODULE_AUTHOR("Philipp Zabel <p.zabel@pengutronix.de>"); +MODULE_DESCRIPTION("gpio reset controller"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:gpio-reset"); +MODULE_DEVICE_TABLE(of, gpio_reset_dt_ids);