[1/4] dt-bindings: nvmem: new optional property write-protect-gpios
diff mbox series

Message ID 20191120142038.30746-2-ktouil@baylibre.com
State Superseded
Headers show
Series
  • at24: move write-protect pin handling to nvmem core
Related show

Commit Message

Khouloud Touil Nov. 20, 2019, 2:20 p.m. UTC
Many nvmem memory chips have a write-protect pin which, when pulled
high, blocks the write operations.

On some boards, this pin is connected to a GPIO and pulled high by
default, which forces the user to manually change its state before
writing.

Instead of modifying all the memory drivers to check this pin, make
the NVMEM subsystem check if the write-protect GPIO being passed
through the nvmem_config or defined in the device tree and pull it
low whenever writing to the memory.

Add a new optional property to the device tree binding document, which
allows to specify the GPIO line to which the write-protect pin is
connected.

Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
---
 Documentation/devicetree/bindings/nvmem/nvmem.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Linus Walleij Nov. 22, 2019, 12:41 p.m. UTC | #1
Hi Khouloud,

thanks for your patch!

I just have a semantic comment:

On Wed, Nov 20, 2019 at 3:21 PM Khouloud Touil <ktouil@baylibre.com> wrote:

> Instead of modifying all the memory drivers to check this pin, make
> the NVMEM subsystem check if the write-protect GPIO being passed
> through the nvmem_config or defined in the device tree and pull it
> low whenever writing to the memory.

It is claimed that this should be pulled low to assert it so by
definition it is active low.

> +  wp-gpios:
> +    description:
> +      GPIO to which the write-protect pin of the chip is connected.
> +    maxItems: 1

Mandate that the flag in the second cell should be GPIO_ACTIVE_LOW

>  patternProperties:
>    "^.*@[0-9a-f]+$":
>      type: object
> @@ -66,6 +71,7 @@ examples:
>        qfprom: eeprom@700000 {
>            #address-cells = <1>;
>            #size-cells = <1>;
> +          wp-gpios = <&gpio1 3 0>;

#include <dt-bindings/gpio/gpio.h>
wp-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;

This will in Linux have the semantic effect that you need to
set the output high with gpio_set_val(d, 1) to assert it
(drive it low) but that really doesn't matter to the device tree
bindings, those are OS-agnostic: if the line is active low then
it should use this flag.

It has the upside that the day you need a write-protect that
is active high, it is simple to support that use case too.

Yours,
Linus Walleij
Bartosz Golaszewski Nov. 22, 2019, 12:47 p.m. UTC | #2
pt., 22 lis 2019 o 13:41 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> Hi Khouloud,
>
> thanks for your patch!
>
> I just have a semantic comment:
>
> On Wed, Nov 20, 2019 at 3:21 PM Khouloud Touil <ktouil@baylibre.com> wrote:
>
> > Instead of modifying all the memory drivers to check this pin, make
> > the NVMEM subsystem check if the write-protect GPIO being passed
> > through the nvmem_config or defined in the device tree and pull it
> > low whenever writing to the memory.
>
> It is claimed that this should be pulled low to assert it so by
> definition it is active low.
>
> > +  wp-gpios:
> > +    description:
> > +      GPIO to which the write-protect pin of the chip is connected.
> > +    maxItems: 1
>
> Mandate that the flag in the second cell should be GPIO_ACTIVE_LOW
>
> >  patternProperties:
> >    "^.*@[0-9a-f]+$":
> >      type: object
> > @@ -66,6 +71,7 @@ examples:
> >        qfprom: eeprom@700000 {
> >            #address-cells = <1>;
> >            #size-cells = <1>;
> > +          wp-gpios = <&gpio1 3 0>;
>
> #include <dt-bindings/gpio/gpio.h>
> wp-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;
>
> This will in Linux have the semantic effect that you need to
> set the output high with gpio_set_val(d, 1) to assert it
> (drive it low) but that really doesn't matter to the device tree
> bindings, those are OS-agnostic: if the line is active low then
> it should use this flag.
>
> It has the upside that the day you need a write-protect that
> is active high, it is simple to support that use case too.
>

Linus,

what about the existing bindings for at24 that don't mandate the
active-low flag? I'm afraid this would break the support for this
specific chip or lead to code duplication if we had this in both nvmem
and at24 with different logic.

Bartosz

> Yours,
> Linus Walleij
Linus Walleij Nov. 22, 2019, 12:53 p.m. UTC | #3
On Fri, Nov 22, 2019 at 1:47 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> what about the existing bindings for at24 that don't mandate the
> active-low flag? I'm afraid this would break the support for this
> specific chip or lead to code duplication if we had this in both nvmem
> and at24 with different logic.

Hm yeah I realized this when I read patches 3 & 4.

I would to like this:

1. Add a new generic property
   writeprotect-gpios that mandates to use GPIO_ACTIVE_LOW
   and use this in the new example

2. Deprecate wp-gpios in the binding, keep it around but deprecated.

3. Add a quirk to gpiolib-of in the manner of the other quirks there
   (like for SPI) so that if we are dealing with some EEPROM node
   like at24 and the flag is zero, tag on GPIO_ACTIVE_LOW on
   the descriptor.

The driver will now handle the semantic of both cases
with gpiolib-of providing a quirk for the old binding.

This is how we solved this type of problem before.

Yours,
Linus Walleij
Bartosz Golaszewski Nov. 22, 2019, 1:04 p.m. UTC | #4
pt., 22 lis 2019 o 13:53 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Fri, Nov 22, 2019 at 1:47 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
>
> > what about the existing bindings for at24 that don't mandate the
> > active-low flag? I'm afraid this would break the support for this
> > specific chip or lead to code duplication if we had this in both nvmem
> > and at24 with different logic.
>
> Hm yeah I realized this when I read patches 3 & 4.
>
> I would to like this:
>
> 1. Add a new generic property
>    writeprotect-gpios that mandates to use GPIO_ACTIVE_LOW
>    and use this in the new example
>
> 2. Deprecate wp-gpios in the binding, keep it around but deprecated.

This is a pretty standard property though - for instance it is
documented in the main mmc binding and doesn't mandate GPIO_ACTIVE_LOW
either. I think this is because nobody says that the write-protect
line must always be driver low to be asserted - this is highly
implementation-specific.

Bartosz

>
> 3. Add a quirk to gpiolib-of in the manner of the other quirks there
>    (like for SPI) so that if we are dealing with some EEPROM node
>    like at24 and the flag is zero, tag on GPIO_ACTIVE_LOW on
>    the descriptor.
>
> The driver will now handle the semantic of both cases
> with gpiolib-of providing a quirk for the old binding.
>
> This is how we solved this type of problem before.
>
> Yours,
> Linus Walleij
Peter Rosin Nov. 22, 2019, 1:10 p.m. UTC | #5
On 2019-11-22 13:41, Linus Walleij wrote:
> Hi Khouloud,
> 
> thanks for your patch!
> 
> I just have a semantic comment:
> 
> On Wed, Nov 20, 2019 at 3:21 PM Khouloud Touil <ktouil@baylibre.com> wrote:
> 
>> Instead of modifying all the memory drivers to check this pin, make
>> the NVMEM subsystem check if the write-protect GPIO being passed
>> through the nvmem_config or defined in the device tree and pull it
>> low whenever writing to the memory.
> 
> It is claimed that this should be pulled low to assert it so by
> definition it is active low.
> 
>> +  wp-gpios:
>> +    description:
>> +      GPIO to which the write-protect pin of the chip is connected.
>> +    maxItems: 1
> 
> Mandate that the flag in the second cell should be GPIO_ACTIVE_LOW

What if something along that way from CPU to chip inverts the signal such
that the signal is no longer active-low when viewed from the CPU, even if
it still is active low when looking at the chip only?

Yes, these things happen for all kinds of hysterical reasons...

Cheers,
Peter

> 
>>  patternProperties:
>>    "^.*@[0-9a-f]+$":
>>      type: object
>> @@ -66,6 +71,7 @@ examples:
>>        qfprom: eeprom@700000 {
>>            #address-cells = <1>;
>>            #size-cells = <1>;
>> +          wp-gpios = <&gpio1 3 0>;
> 
> #include <dt-bindings/gpio/gpio.h>
> wp-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;
> 
> This will in Linux have the semantic effect that you need to
> set the output high with gpio_set_val(d, 1) to assert it
> (drive it low) but that really doesn't matter to the device tree
> bindings, those are OS-agnostic: if the line is active low then
> it should use this flag.
> 
> It has the upside that the day you need a write-protect that
> is active high, it is simple to support that use case too.
> 
> Yours,
> Linus Walleij
>
Linus Walleij Nov. 22, 2019, 1:46 p.m. UTC | #6
On Fri, Nov 22, 2019 at 2:04 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> > I would to like this:
> >
> > 1. Add a new generic property
> >    writeprotect-gpios that mandates to use GPIO_ACTIVE_LOW
> >    and use this in the new example
> >
> > 2. Deprecate wp-gpios in the binding, keep it around but deprecated.
>
> This is a pretty standard property though - for instance it is
> documented in the main mmc binding and doesn't mandate GPIO_ACTIVE_LOW
> either. I think this is because nobody says that the write-protect
> line must always be driver low to be asserted - this is highly
> implementation-specific.

The MMC case is actually especially convoluted. It has always
respected the GPIO_ACTIVE_LOW flag, and that is used if
present. At the same time it *also* supported a bool
wp-inverted flag, with the specified semantic that if both
were specified (ACTIVE_LOW and wp-inverted) the result
would be nothing as it is a double logical inversion.

So that is why the quirk looks like this:

       /*
         * Handle MMC "cd-inverted" and "wp-inverted" semantics.
         */
        if (IS_ENABLED(CONFIG_MMC)) {
                /*
                 * Active low is the default according to the
                 * SDHCI specification and the device tree
                 * bindings. However the code in the current
                 * kernel was written such that the phandle
                 * flags were always respected, and "cd-inverted"
                 * would invert the flag from the device phandle.
                 */
                if (!strcmp(propname, "cd-gpios")) {
                        if (of_property_read_bool(np, "cd-inverted"))
                                *flags ^= OF_GPIO_ACTIVE_LOW;
                }
                if (!strcmp(propname, "wp-gpios")) {
                        if (of_property_read_bool(np, "wp-inverted"))
                                *flags ^= OF_GPIO_ACTIVE_LOW;
                }
        }

Nevermind MMC though.

The current code for at24 has an ambiguousness issue: if
the gpios cell 2 is specified as GPIO_ACTIVE_LOW
(which is in some sense correct) then the effect will be
that it is driven high to assert the wp, which is  ... rather
counterintuitive.

I could think of a compromise like this:

1. Keep "wp-gpios"

2. Add a quirk to gpiolib-of.c that will force that as active
   low no matter what flag is specified to the GPIO descriptor.

3. If some other flag that GPIO_ACTIVE_LOW is specified,
  print a warning and say the the (default) GPIO_ACTIVE_HIGH
  i.e. 0 is gonna be ignored and we forced the line to be
  active low.

4. The code still need to be modified to set the value
   to "1" to assert the line since the gpiolib now handles
   the inversion semantics.

5. Hope that no system with an active high wp ever comes
  into existence because then we are screwed and will have
  to create a new binding and deprecate the old binding
  anyway.

Yours,
Linus Walleij
Linus Walleij Nov. 28, 2019, 1:44 p.m. UTC | #7
On Tue, Nov 26, 2019 at 4:18 PM Khouloud Touil <ktouil@baylibre.com> wrote:

> [Me]
>> 4. The code still need to be modified to set the value
>>    to "1" to assert the line since the gpiolib now handles
>>    the inversion semantics.

> By saying "assert the wp" do you mean enable the write operation or
> block it ?

Yeah one more layer of confusion, sorry :/

By "asserting WP" I mean driving the line to a state where
writing to the EEPROM is enabled, i.e. the default state is
that the EEPROM is write protected and when you "assert"
WP it becomes writable.

If you feel the inverse semantics are more intuitive (such that
WP comes up asserted and thus write protected), be my
guest :D

As long as it is unambiguously documented in the bindings
and with comments in the code I'm game for whatever the
at24 people feel is most appropriate. (You will set the standard
for everyone else.)

Yours.
Linus Walleij
Bartosz Golaszewski Nov. 29, 2019, 8:47 a.m. UTC | #8
czw., 28 lis 2019 o 14:45 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Tue, Nov 26, 2019 at 4:18 PM Khouloud Touil <ktouil@baylibre.com> wrote:
>
> > [Me]
> >> 4. The code still need to be modified to set the value
> >>    to "1" to assert the line since the gpiolib now handles
> >>    the inversion semantics.
>
> > By saying "assert the wp" do you mean enable the write operation or
> > block it ?
>
> Yeah one more layer of confusion, sorry :/
>
> By "asserting WP" I mean driving the line to a state where
> writing to the EEPROM is enabled, i.e. the default state is
> that the EEPROM is write protected and when you "assert"
> WP it becomes writable.
>
> If you feel the inverse semantics are more intuitive (such that
> WP comes up asserted and thus write protected), be my
> guest :D
>

Ha! I've always assumed that "to assert the write-protect pin" means
to *protect* the EEPROM from writing. That's why it comes up as
asserted (logical '1' in the driver) and we need to deassert it (drive
it low, logical '0' in the driver) to enable writing. This is the
current behavior and I'd say in this case it's just a matter of very
explicit statement that this is how it works in the DT binding?

Rob: any thoughts on this?

Bartosz

> As long as it is unambiguously documented in the bindings
> and with comments in the code I'm game for whatever the
> at24 people feel is most appropriate. (You will set the standard
> for everyone else.)
>
> Yours.
> Linus Walleij
Rob Herring Dec. 4, 2019, 3:14 p.m. UTC | #9
On Fri, Nov 29, 2019 at 09:47:01AM +0100, Bartosz Golaszewski wrote:
> czw., 28 lis 2019 o 14:45 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > On Tue, Nov 26, 2019 at 4:18 PM Khouloud Touil <ktouil@baylibre.com> wrote:
> >
> > > [Me]
> > >> 4. The code still need to be modified to set the value
> > >>    to "1" to assert the line since the gpiolib now handles
> > >>    the inversion semantics.
> >
> > > By saying "assert the wp" do you mean enable the write operation or
> > > block it ?
> >
> > Yeah one more layer of confusion, sorry :/
> >
> > By "asserting WP" I mean driving the line to a state where
> > writing to the EEPROM is enabled, i.e. the default state is
> > that the EEPROM is write protected and when you "assert"
> > WP it becomes writable.
> >
> > If you feel the inverse semantics are more intuitive (such that
> > WP comes up asserted and thus write protected), be my
> > guest :D
> >
> 
> Ha! I've always assumed that "to assert the write-protect pin" means
> to *protect* the EEPROM from writing. That's why it comes up as
> asserted (logical '1' in the driver) and we need to deassert it (drive
> it low, logical '0' in the driver) to enable writing. This is the
> current behavior and I'd say in this case it's just a matter of very
> explicit statement that this is how it works in the DT binding?
> 
> Rob: any thoughts on this?

I agree with you. If it was called write-enable-gpios, then assert would 
be to enable writing.

Rob

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
index 1c75a059206c..6724764af794 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
@@ -34,6 +34,11 @@  properties:
     description:
       Mark the provider as read only.
 
+  wp-gpios:
+    description:
+      GPIO to which the write-protect pin of the chip is connected.
+    maxItems: 1
+
 patternProperties:
   "^.*@[0-9a-f]+$":
     type: object
@@ -66,6 +71,7 @@  examples:
       qfprom: eeprom@700000 {
           #address-cells = <1>;
           #size-cells = <1>;
+          wp-gpios = <&gpio1 3 0>;
 
           /* ... */