| Message ID | 20191120142038.30746-2-ktouil@baylibre.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | at24: move write-protect pin handling to nvmem core | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success |
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
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
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
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
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 >
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
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
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
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
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>; /* ... */
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(+)