| Message ID | 1491568725-14882-1-git-send-email-ckeepax@opensource.wolfsonmicro.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 07, 2017 at 01:38:44PM +0100, Charles Keepax wrote: > The Arizona devices only maintain the state of output GPIOs whilst the > CODEC is active, this can cause issues if the CODEC suspends whilst > something is relying on the state of one of its GPIOs. However, in > many systems the CODEC GPIOs are used for audio related features > and thus the state of the GPIOs is unimportant whilst the CODEC is > suspended. Often keeping the CODEC resumed in such a system would > incur a power impact that is unacceptable. > > Add a flag through the second cell of the GPIO specifier in device > tree, to allow the user to select whether a GPIO being configured as > an output should keep the CODEC resumed. If the whole codec can't be suspended, why does this need to be per GPIO? You could just have a single boolean property. > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> > --- > Documentation/devicetree/bindings/mfd/arizona.txt | 5 ++++- > include/dt-bindings/mfd/arizona.h | 3 +++ > 2 files changed, 7 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-04-10 at 15:17 -0500, Rob Herring wrote: > On Fri, Apr 07, 2017 at 01:38:44PM +0100, Charles Keepax wrote: > > The Arizona devices only maintain the state of output GPIOs whilst the > > CODEC is active, this can cause issues if the CODEC suspends whilst > > something is relying on the state of one of its GPIOs. However, in > > many systems the CODEC GPIOs are used for audio related features > > and thus the state of the GPIOs is unimportant whilst the CODEC is > > suspended. Often keeping the CODEC resumed in such a system would > > incur a power impact that is unacceptable. > > > > Add a flag through the second cell of the GPIO specifier in device > > tree, to allow the user to select whether a GPIO being configured as > > an output should keep the CODEC resumed. > > If the whole codec can't be suspended, why does this need to be per > GPIO? You could just have a single boolean property. > Three reasons I can think of: 1) The GPIO binding already provides for passing extra information through the binding ("Exact meaning of each specifier cell is controller specific" as it says in the main gpio binding doc) so why add yet another custom property to do it? 2) Doing it through the gpio means that if ultimately the child DT node that is using it gets disabled (status="disabled") or that driver isn't in use the codec will be able to go to sleep. That won't happen with a brute-force "big lock". 3) The codec only has to be kept awake while any such GPIO is actually in use. See (2) > > > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> > > --- > > Documentation/devicetree/bindings/mfd/arizona.txt | 5 ++++- > > include/dt-bindings/mfd/arizona.h | 3 +++ > > 2 files changed, 7 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote: > On Mon, 2017-04-10 at 15:17 -0500, Rob Herring wrote: > > On Fri, Apr 07, 2017 at 01:38:44PM +0100, Charles Keepax wrote: > > > The Arizona devices only maintain the state of output GPIOs whilst the > > > CODEC is active, this can cause issues if the CODEC suspends whilst > > > something is relying on the state of one of its GPIOs. However, in > > > many systems the CODEC GPIOs are used for audio related features > > > and thus the state of the GPIOs is unimportant whilst the CODEC is > > > suspended. Often keeping the CODEC resumed in such a system would > > > incur a power impact that is unacceptable. > > > > > > Add a flag through the second cell of the GPIO specifier in device > > > tree, to allow the user to select whether a GPIO being configured as > > > an output should keep the CODEC resumed. > > > > If the whole codec can't be suspended, why does this need to be per > > GPIO? You could just have a single boolean property. > > > > Three reasons I can think of: > > 1) The GPIO binding already provides for passing extra information > through the binding ("Exact meaning of each specifier cell is controller > specific" as it says in the main gpio binding doc) so why add yet > another custom property to do it? > > 2) Doing it through the gpio means that if ultimately the child DT node > that is using it gets disabled (status="disabled") or that driver isn't > in use the codec will be able to go to sleep. That won't happen with a > brute-force "big lock". > > 3) The codec only has to be kept awake while any such GPIO is actually > in use. See (2) > Yeah option 3 is the primary issue here, we only want to keep the CODEC enabled whilst specific GPIOs are in use. As GPIOs can be dynamically requested/released by things in the kernel we want to know which GPIOs require the CODEC to be kept alive. Also in the future one might be tempted to add maintain whilst high and maintain whilst low options for lines with pulls on them to further optimise power. Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 13, 2017 at 11:15 AM, Charles Keepax <ckeepax@opensource.wolfsonmicro.com> wrote: > On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote: >> 3) The codec only has to be kept awake while any such GPIO is actually >> in use. See (2) > > Yeah option 3 is the primary issue here, we only want to keep the > CODEC enabled whilst specific GPIOs are in use. As GPIOs can be > dynamically requested/released by things in the kernel we want to > know which GPIOs require the CODEC to be kept alive. Also in the > future one might be tempted to add maintain whilst high and > maintain whilst low options for lines with pulls on them to > further optimise power. Why does this have to be encoded as information in the device tree? Isn't it better to use a static table in the driver? I don't see what use system integrators and others playing around with the device tree has of this, it will just be confusing to them if it is a chip-internal detail. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-04-13 at 14:14 +0200, Linus Walleij wrote: > On Thu, Apr 13, 2017 at 11:15 AM, Charles Keepax > <ckeepax@opensource.wolfsonmicro.com> wrote: > > On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote: > > >> 3) The codec only has to be kept awake while any such GPIO is actually > >> in use. See (2) > > > > Yeah option 3 is the primary issue here, we only want to keep the > > CODEC enabled whilst specific GPIOs are in use. As GPIOs can be > > dynamically requested/released by things in the kernel we want to > > know which GPIOs require the CODEC to be kept alive. Also in the > > future one might be tempted to add maintain whilst high and > > maintain whilst low options for lines with pulls on them to > > further optimise power. > > Why does this have to be encoded as information in the device > tree? Isn't it better to use a static table in the driver? > > I don't see what use system integrators and others playing > around with the device tree has of this, it will just be confusing > to them if it is a chip-internal detail. > They are GPIOs for connecting to external hardware, we don't know what people are going to connect them to so they have to tell us how they need them to behave. > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 13, 2017 at 2:21 PM, Richard Fitzgerald <rf@opensource.wolfsonmicro.com> wrote: > On Thu, 2017-04-13 at 14:14 +0200, Linus Walleij wrote: >> On Thu, Apr 13, 2017 at 11:15 AM, Charles Keepax >> <ckeepax@opensource.wolfsonmicro.com> wrote: >> > On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote: >> >> >> 3) The codec only has to be kept awake while any such GPIO is actually >> >> in use. See (2) >> > >> > Yeah option 3 is the primary issue here, we only want to keep the >> > CODEC enabled whilst specific GPIOs are in use. As GPIOs can be >> > dynamically requested/released by things in the kernel we want to >> > know which GPIOs require the CODEC to be kept alive. Also in the >> > future one might be tempted to add maintain whilst high and >> > maintain whilst low options for lines with pulls on them to >> > further optimise power. >> >> Why does this have to be encoded as information in the device >> tree? Isn't it better to use a static table in the driver? >> >> I don't see what use system integrators and others playing >> around with the device tree has of this, it will just be confusing >> to them if it is a chip-internal detail. >> > > They are GPIOs for connecting to external hardware, we don't know what > people are going to connect them to so they have to tell us how they > need them to behave. Aha it is a consumer configuration thing, then I see it. I think it seems a bit odd that it is assumed that the default is that we should *not* preserve the GPIO output value if we go to sleep. Should the flag be inverted? Also, why can't we just use a generic flag for this, it seems very very generic. Look in: include/dt-bindings/gpio/gpio.h Is there any reasons why we can't have: /* Bit 3 express GPIO suspend/resume persistance in low power mode */ #define GPIO_MUST_KEEP_VALUE 0 #define GPIO_MAY_LOOSE_VALUE_DURING_SLEEP 8 Yeah it's talkative but informative. This way you can mark up lines that are OK to loose their value during low-power/sleep using just (new) standard bindings that can be reused by others, also optionally making it possible for the gpiolib core to take action on these properties if need be. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 13, 2017 at 02:48:45PM +0200, Linus Walleij wrote: > On Thu, Apr 13, 2017 at 2:21 PM, Richard Fitzgerald > <rf@opensource.wolfsonmicro.com> wrote: > > On Thu, 2017-04-13 at 14:14 +0200, Linus Walleij wrote: > >> On Thu, Apr 13, 2017 at 11:15 AM, Charles Keepax > >> <ckeepax@opensource.wolfsonmicro.com> wrote: > >> > On Tue, Apr 11, 2017 at 10:34:27AM +0100, Richard Fitzgerald wrote: > >> > >> >> 3) The codec only has to be kept awake while any such GPIO is actually > >> >> in use. See (2) > >> > > >> > Yeah option 3 is the primary issue here, we only want to keep the > >> > CODEC enabled whilst specific GPIOs are in use. As GPIOs can be > >> > dynamically requested/released by things in the kernel we want to > >> > know which GPIOs require the CODEC to be kept alive. Also in the > >> > future one might be tempted to add maintain whilst high and > >> > maintain whilst low options for lines with pulls on them to > >> > further optimise power. > >> > >> Why does this have to be encoded as information in the device > >> tree? Isn't it better to use a static table in the driver? > >> > >> I don't see what use system integrators and others playing > >> around with the device tree has of this, it will just be confusing > >> to them if it is a chip-internal detail. > >> > > > > They are GPIOs for connecting to external hardware, we don't know what > > people are going to connect them to so they have to tell us how they > > need them to behave. > > Aha it is a consumer configuration thing, then I see it. > > I think it seems a bit odd that it is assumed that the default is that > we should *not* preserve the GPIO output value if we go to sleep. > Should the flag be inverted? > I agree that is a bit odd, my thinking was keeping the behaviour the same for existing systems. But it only introduces a power regression perhaps it is ok to require people to update their DT to avoid that? > Also, why can't we just use a generic flag for this, it seems very > very generic. > > Look in: > include/dt-bindings/gpio/gpio.h > > Is there any reasons why we can't have: > /* Bit 3 express GPIO suspend/resume persistance in low power mode */ > #define GPIO_MUST_KEEP_VALUE 0 > #define GPIO_MAY_LOOSE_VALUE_DURING_SLEEP 8 > > Yeah it's talkative but informative. This way you can mark up lines > that are OK to loose their value during low-power/sleep using > just (new) standard bindings that can be reused by others, > also optionally making it possible for the gpiolib core to take action > on these properties if need be. > I certainly have no objections to making this a core feature if you are comfortable with that. I will have a look at what that would look like. Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt index 8f2e282..6af0d82 100644 --- a/Documentation/devicetree/bindings/mfd/arizona.txt +++ b/Documentation/devicetree/bindings/mfd/arizona.txt @@ -30,7 +30,10 @@ Required properties: - gpio-controller : Indicates this device is a GPIO controller. - #gpio-cells : Must be 2. The first cell is the pin number and the - second cell is used to specify optional parameters (currently unused). + second cell is used to specify optional parameters, the following flags + are supported: + "ARIZONA_GP_MAINTAIN" the output of the GPIO must be maintained, this + prevents the CODEC going into low power mode. - AVDD-supply, DBVDD1-supply, CPVDD-supply : Power supplies for the device, as covered in Documentation/devicetree/bindings/regulator/regulator.txt diff --git a/include/dt-bindings/mfd/arizona.h b/include/dt-bindings/mfd/arizona.h index dedf46f..68f3782 100644 --- a/include/dt-bindings/mfd/arizona.h +++ b/include/dt-bindings/mfd/arizona.h @@ -77,6 +77,9 @@ #define ARIZONA_GP_INPUT (ARIZONA_GP_FN_GPIO | \ ARIZONA_GPN_DIR) +/* Flags for the GPIO driver properties */ +#define ARIZONA_GP_MAINTAIN 0x80000000 + #define ARIZONA_32KZ_MCLK1 1 #define ARIZONA_32KZ_MCLK2 2 #define ARIZONA_32KZ_NONE 3
The Arizona devices only maintain the state of output GPIOs whilst the CODEC is active, this can cause issues if the CODEC suspends whilst something is relying on the state of one of its GPIOs. However, in many systems the CODEC GPIOs are used for audio related features and thus the state of the GPIOs is unimportant whilst the CODEC is suspended. Often keeping the CODEC resumed in such a system would incur a power impact that is unacceptable. Add a flag through the second cell of the GPIO specifier in device tree, to allow the user to select whether a GPIO being configured as an output should keep the CODEC resumed. Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> --- Documentation/devicetree/bindings/mfd/arizona.txt | 5 ++++- include/dt-bindings/mfd/arizona.h | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-)