[PATCH v2 0/4] mtd: rawnand: stm32_fmc2: Add NAND Write Protect support

[PATCH v2 0/4] mtd: rawnand: stm32_fmc2: Add NAND Write Protect support

[Christophe Kerello]

This patchset adds the management of the WP# signal in FMC2 driver.
WP will be disabled in probe/resume callbacks and will be enabled
in remove/suspend callbacks.

This patchset also fixes a conflict on wp-gpios property between
MTD and NVMEN.

Changes in v2:
 - add Rob Acked-by for the bindings part.
 - rework the proposal done to fix a conflict between MTD and NVMEM on
   wp-gpios property.

Christophe Kerello (4):
  dt-binding: mtd: nand: Document the wp-gpios property
  mtd: rawnand: stm32_fmc2: Add NAND Write Protect support
  nvmem: core: Fix a conflict between MTD and NVMEM on wp-gpios property
  mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

 .../bindings/mtd/nand-controller.yaml         |  7 ++++
 drivers/mtd/mtdcore.c                         |  2 +
 drivers/mtd/nand/raw/stm32_fmc2_nand.c        | 40 ++++++++++++++++++-
 drivers/nvmem/core.c                          |  2 +-
 include/linux/nvmem-provider.h                |  4 +-
 5 files changed, 52 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] dt-binding: mtd: nand: Document the wp-gpios property

[Christophe Kerello]

A few drivers use this property to describe the GPIO pin used to protect
the NAND during program/erase operations.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
 - add Rob Acked-by.

 Documentation/devicetree/bindings/mtd/nand-controller.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/nand-controller.yaml b/Documentation/devicetree/bindings/mtd/nand-controller.yaml
index bd217e6f5018..53b21aed0ac5 100644
--- a/Documentation/devicetree/bindings/mtd/nand-controller.yaml
+++ b/Documentation/devicetree/bindings/mtd/nand-controller.yaml
@@ -154,6 +154,13 @@ patternProperties:
           Ready/Busy pins. Active state refers to the NAND ready state and
           should be set to GPIOD_ACTIVE_HIGH unless the signal is inverted.
 
+      wp-gpios:
+        description:
+          Contains one GPIO descriptor for the Write Protect pin.
+          Active state refers to the NAND Write Protect state and should be
+          set to GPIOD_ACTIVE_LOW unless the signal is inverted.
+        maxItems: 1
+
       secure-regions:
         $ref: /schemas/types.yaml#/definitions/uint64-matrix
         description:
-- 
2.25.1

[PATCH v2 2/4] mtd: rawnand: stm32_fmc2: Add NAND Write Protect support

[Christophe Kerello]

This patch adds the support of the WP# signal. WP will be disabled in
probe/resume callbacks and will be enabled in remove/suspend callbacks.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/mtd/nand/raw/stm32_fmc2_nand.c | 40 +++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
index 97b4e02e43e4..87c1c7dd97eb 100644
--- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
+++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
@@ -9,6 +9,7 @@
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
 #include <linux/errno.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
 #include <linux/mfd/syscon.h>
@@ -231,6 +232,7 @@ struct stm32_fmc2_timings {
 
 struct stm32_fmc2_nand {
 	struct nand_chip chip;
+	struct gpio_desc *wp_gpio;
 	struct stm32_fmc2_timings timings;
 	int ncs;
 	int cs_used[FMC2_MAX_CE];
@@ -1747,6 +1749,18 @@ static const struct nand_controller_ops stm32_fmc2_nfc_controller_ops = {
 	.setup_interface = stm32_fmc2_nfc_setup_interface,
 };
 
+static void stm32_fmc2_nfc_wp_enable(struct stm32_fmc2_nand *nand)
+{
+	if (nand->wp_gpio)
+		gpiod_set_value(nand->wp_gpio, 1);
+}
+
+static void stm32_fmc2_nfc_wp_disable(struct stm32_fmc2_nand *nand)
+{
+	if (nand->wp_gpio)
+		gpiod_set_value(nand->wp_gpio, 0);
+}
+
 static int stm32_fmc2_nfc_parse_child(struct stm32_fmc2_nfc *nfc,
 				      struct device_node *dn)
 {
@@ -1785,6 +1799,18 @@ static int stm32_fmc2_nfc_parse_child(struct stm32_fmc2_nfc *nfc,
 		nand->cs_used[i] = cs;
 	}
 
+	nand->wp_gpio = devm_gpiod_get_from_of_node(nfc->dev, dn,
+						    "wp-gpios", 0,
+						    GPIOD_OUT_HIGH, "wp");
+	if (IS_ERR(nand->wp_gpio)) {
+		ret = PTR_ERR(nand->wp_gpio);
+		if (ret != -ENOENT)
+			return dev_err_probe(nfc->dev, ret,
+					     "failed to request WP GPIO\n");
+
+		nand->wp_gpio = NULL;
+	}
+
 	nand_set_flash_node(&nand->chip, dn);
 
 	return 0;
@@ -1956,10 +1982,12 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
 	chip->options |= NAND_BUSWIDTH_AUTO | NAND_NO_SUBPAGE_WRITE |
 			 NAND_USES_DMA;
 
+	stm32_fmc2_nfc_wp_disable(nand);
+
 	/* Scan to find existence of the device */
 	ret = nand_scan(chip, nand->ncs);
 	if (ret)
-		goto err_release_dma;
+		goto err_wp_enable;
 
 	ret = mtd_device_register(mtd, NULL, 0);
 	if (ret)
@@ -1972,6 +2000,9 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
 err_nand_cleanup:
 	nand_cleanup(chip);
 
+err_wp_enable:
+	stm32_fmc2_nfc_wp_enable(nand);
+
 err_release_dma:
 	if (nfc->dma_ecc_ch)
 		dma_release_channel(nfc->dma_ecc_ch);
@@ -2012,15 +2043,20 @@ static int stm32_fmc2_nfc_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(nfc->clk);
 
+	stm32_fmc2_nfc_wp_enable(nand);
+
 	return 0;
 }
 
 static int __maybe_unused stm32_fmc2_nfc_suspend(struct device *dev)
 {
 	struct stm32_fmc2_nfc *nfc = dev_get_drvdata(dev);
+	struct stm32_fmc2_nand *nand = &nfc->nand;
 
 	clk_disable_unprepare(nfc->clk);
 
+	stm32_fmc2_nfc_wp_enable(nand);
+
 	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
@@ -2042,6 +2078,8 @@ static int __maybe_unused stm32_fmc2_nfc_resume(struct device *dev)
 
 	stm32_fmc2_nfc_init(nfc);
 
+	stm32_fmc2_nfc_wp_disable(nand);
+
 	for (chip_cs = 0; chip_cs < FMC2_MAX_CE; chip_cs++) {
 		if (!(nfc->cs_assigned & BIT(chip_cs)))
 			continue;
-- 
2.25.1

[PATCH v2 3/4] nvmem: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Christophe Kerello]

Wp-gpios property can be used on NVMEM nodes and the same property can
be also used on MTD NAND nodes. In case of the wp-gpios property is
defined at NAND level node, the GPIO management is done at NAND driver
level. Write protect is disabled when the driver is probed or resumed
and is enabled when the driver is released or suspended.

When no partitions are defined in the NAND DT node, then the NAND DT node
will be passed to NVMEM framework. If wp-gpios property is defined in
this node, the GPIO resource is taken twice and the NAND controller
driver fails to probe.

It would be possible to set config->wp_gpio at MTD level before calling
nvmem_register function but NVMEM framework will toggle this GPIO on
each write when this GPIO should only be controlled at NAND level driver
to ensure that the Write Protect has not been enabled.

A way to fix this conflict is to add a new boolean flag in nvmem_config
named skip_wp_gpio. In case skip_wp_gpio is set, the GPIO resource will
be managed by the provider.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
Changes in v2:
 - rework the proposal done to fix a conflict between MTD and NVMEM on
   wp-gpios property.

 drivers/nvmem/core.c           | 2 +-
 include/linux/nvmem-provider.h | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 23a38dcf0fc4..cb40dca6a51d 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -771,7 +771,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
 	if (config->wp_gpio)
 		nvmem->wp_gpio = config->wp_gpio;
-	else
+	else if (!config->skip_wp_gpio)
 		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(nvmem->wp_gpio)) {
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 98efb7b5660d..4b480023c265 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -70,7 +70,8 @@ struct nvmem_keepout {
  * @word_size:	Minimum read/write access granularity.
  * @stride:	Minimum read/write access stride.
  * @priv:	User context passed to read/write callbacks.
- * @wp-gpio:   Write protect pin
+ * @wp-gpio:	Write protect pin
+ * @skip_wp_gpio: Write Protect pin is managed by the provider.
  *
  * Note: A default "nvmem<id>" name will be assigned to the device if
  * no name is specified in its configuration. In such case "<id>" is
@@ -92,6 +93,7 @@ struct nvmem_config {
 	enum nvmem_type		type;
 	bool			read_only;
 	bool			root_only;
+	bool			skip_wp_gpio;
 	struct device_node	*of_node;
 	bool			no_of_node;
 	nvmem_reg_read_t	reg_read;
-- 
2.25.1

[PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Christophe Kerello]

Wp-gpios property can be used on NVMEM nodes and the same property can
be also used on MTD NAND nodes. In case of the wp-gpios property is
defined at NAND level node, the GPIO management is done at NAND driver
level. Write protect is disabled when the driver is probed or resumed
and is enabled when the driver is released or suspended.

When no partitions are defined in the NAND DT node, then the NAND DT node
will be passed to NVMEM framework. If wp-gpios property is defined in
this node, the GPIO resource is taken twice and the NAND controller
driver fails to probe.

A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
In case skip_wp_gpio is set, it means that the GPIO is handled by the
provider. Lets set this flag in MTD layer to avoid the conflict on
wp_gpios property.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/mtd/mtdcore.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 70f492dce158..e6d251594def 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
 	config.stride = 1;
 	config.read_only = true;
 	config.root_only = true;
+	config.skip_wp_gpio = true;
 	config.no_of_node = !of_device_is_compatible(node, "nvmem-cells");
 	config.priv = mtd;
 
@@ -833,6 +834,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
 	config.owner = THIS_MODULE;
 	config.type = NVMEM_TYPE_OTP;
 	config.root_only = true;
+	config.skip_wp_gpio = true;
 	config.reg_read = reg_read;
 	config.size = size;
 	config.of_node = np;
-- 
2.25.1

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Miquel Raynal]

Hi Vignesh, Tudory, Pratyush,

+ Tudor and Pratyush

christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 +0100:

> Wp-gpios property can be used on NVMEM nodes and the same property can
> be also used on MTD NAND nodes. In case of the wp-gpios property is
> defined at NAND level node, the GPIO management is done at NAND driver
> level. Write protect is disabled when the driver is probed or resumed
> and is enabled when the driver is released or suspended.
> 
> When no partitions are defined in the NAND DT node, then the NAND DT node
> will be passed to NVMEM framework. If wp-gpios property is defined in
> this node, the GPIO resource is taken twice and the NAND controller
> driver fails to probe.
> 
> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
> In case skip_wp_gpio is set, it means that the GPIO is handled by the
> provider. Lets set this flag in MTD layer to avoid the conflict on
> wp_gpios property.
> 
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
>  drivers/mtd/mtdcore.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 70f492dce158..e6d251594def 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>  	config.stride = 1;
>  	config.read_only = true;
>  	config.root_only = true;
> +	config.skip_wp_gpio = true;
>  	config.no_of_node = !of_device_is_compatible(node, "nvmem-cells");
>  	config.priv = mtd;
>  
> @@ -833,6 +834,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
>  	config.owner = THIS_MODULE;
>  	config.type = NVMEM_TYPE_OTP;
>  	config.root_only = true;
> +	config.skip_wp_gpio = true;
>  	config.reg_read = reg_read;
>  	config.size = size;
>  	config.of_node = np;

TLDR: There is a conflict between MTD and NVMEM, who should handle the
WP pin when there is one? At least for raw NAND devices, I don't want
the NVMEM core to handle the wp pin. So we've introduced this
skip_wp_gpio nvmem config option. But there are two places where this
boolean can be set and one of these is for otp regions (see above). In
this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
nvmem protection. Please tell us if you think this is fine for you.

Thanks,
Miquèl

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Pratyush Yadav]

On 31/01/22 02:43PM, Miquel Raynal wrote:
> Hi Vignesh, Tudory, Pratyush,
> 
> + Tudor and Pratyush
> 
> christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 +0100:
> 
> > Wp-gpios property can be used on NVMEM nodes and the same property can
> > be also used on MTD NAND nodes. In case of the wp-gpios property is
> > defined at NAND level node, the GPIO management is done at NAND driver
> > level. Write protect is disabled when the driver is probed or resumed
> > and is enabled when the driver is released or suspended.
> > 
> > When no partitions are defined in the NAND DT node, then the NAND DT node
> > will be passed to NVMEM framework. If wp-gpios property is defined in
> > this node, the GPIO resource is taken twice and the NAND controller
> > driver fails to probe.
> > 
> > A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
> > In case skip_wp_gpio is set, it means that the GPIO is handled by the
> > provider. Lets set this flag in MTD layer to avoid the conflict on
> > wp_gpios property.
> > 
> > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> > ---
> >  drivers/mtd/mtdcore.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index 70f492dce158..e6d251594def 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
> >  	config.stride = 1;
> >  	config.read_only = true;
> >  	config.root_only = true;
> > +	config.skip_wp_gpio = true;
> >  	config.no_of_node = !of_device_is_compatible(node, "nvmem-cells");
> >  	config.priv = mtd;
> >  
> > @@ -833,6 +834,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
> >  	config.owner = THIS_MODULE;
> >  	config.type = NVMEM_TYPE_OTP;
> >  	config.root_only = true;
> > +	config.skip_wp_gpio = true;
> >  	config.reg_read = reg_read;
> >  	config.size = size;
> >  	config.of_node = np;
> 
> TLDR: There is a conflict between MTD and NVMEM, who should handle the
> WP pin when there is one? At least for raw NAND devices, I don't want
> the NVMEM core to handle the wp pin. So we've introduced this
> skip_wp_gpio nvmem config option. But there are two places where this
> boolean can be set and one of these is for otp regions (see above). In
> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
> nvmem protection. Please tell us if you think this is fine for you.

Why does NVMEM touch hardware write protection in the first place? The 
purpose of the framework is to provide a way to retrieve config stored 
in memory. It has no business dealing with details of the chip like the 
WP line. That should be MTD's job (which it should delegate to SPI NOR, 
SPI NAND, etc.). If you want to write protect a cell then do it in 
software. I don't see why NVMEM should be dealing with hardware directly 
at all.

That is my mental model of how things _should_ work. I have not spent 
much time digging into how things actually work currently.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Christophe Kerello]

Hi,

On 2/1/22 11:47, Pratyush Yadav wrote:
> On 31/01/22 02:43PM, Miquel Raynal wrote:
>> Hi Vignesh, Tudory, Pratyush,
>>
>> + Tudor and Pratyush
>>
>> christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 +0100:
>>
>>> Wp-gpios property can be used on NVMEM nodes and the same property can
>>> be also used on MTD NAND nodes. In case of the wp-gpios property is
>>> defined at NAND level node, the GPIO management is done at NAND driver
>>> level. Write protect is disabled when the driver is probed or resumed
>>> and is enabled when the driver is released or suspended.
>>>
>>> When no partitions are defined in the NAND DT node, then the NAND DT node
>>> will be passed to NVMEM framework. If wp-gpios property is defined in
>>> this node, the GPIO resource is taken twice and the NAND controller
>>> driver fails to probe.
>>>
>>> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
>>> In case skip_wp_gpio is set, it means that the GPIO is handled by the
>>> provider. Lets set this flag in MTD layer to avoid the conflict on
>>> wp_gpios property.
>>>
>>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>>> ---
>>>   drivers/mtd/mtdcore.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>>> index 70f492dce158..e6d251594def 100644
>>> --- a/drivers/mtd/mtdcore.c
>>> +++ b/drivers/mtd/mtdcore.c
>>> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>>>   	config.stride = 1;
>>>   	config.read_only = true;
>>>   	config.root_only = true;
>>> +	config.skip_wp_gpio = true;
>>>   	config.no_of_node = !of_device_is_compatible(node, "nvmem-cells");
>>>   	config.priv = mtd;
>>>   
>>> @@ -833,6 +834,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
>>>   	config.owner = THIS_MODULE;
>>>   	config.type = NVMEM_TYPE_OTP;
>>>   	config.root_only = true;
>>> +	config.skip_wp_gpio = true;
>>>   	config.reg_read = reg_read;
>>>   	config.size = size;
>>>   	config.of_node = np;
>>
>> TLDR: There is a conflict between MTD and NVMEM, who should handle the
>> WP pin when there is one? At least for raw NAND devices, I don't want
>> the NVMEM core to handle the wp pin. So we've introduced this
>> skip_wp_gpio nvmem config option. But there are two places where this
>> boolean can be set and one of these is for otp regions (see above). In
>> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
>> nvmem protection. Please tell us if you think this is fine for you.
> 
> Why does NVMEM touch hardware write protection in the first place? The
> purpose of the framework is to provide a way to retrieve config stored
> in memory. It has no business dealing with details of the chip like the
> WP line. That should be MTD's job (which it should delegate to SPI NOR,
> SPI NAND, etc.). If you want to write protect a cell then do it in
> software. I don't see why NVMEM should be dealing with hardware directly
> at all.
> 
> That is my mental model of how things _should_ work. I have not spent
> much time digging into how things actually work currently.
> 

Wp-gpios property management was added in MVMEM framework in January 
2020 => sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
"
     nvmem: add support for the write-protect pin

     The write-protect pin handling looks like a standard property that
     could benefit other users if available in the core nvmem framework.

     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.
"

And this modification was done for EEPROMs flashes => sha1: 
1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
"
     eeprom: at24: remove the write-protect pin support

     NVMEM framework is an interface for the at24 EEPROMs as well as for
     other drivers, instead of passing the wp-gpios over the different
     drivers each time, it would be better to pass it over the NVMEM
     subsystem once and for all.

     Removing the support for the write-protect pin after adding it to
     the NVMEM subsystem.
"

Current NVMEM framework implementation toggles the WP GPIO when 
reg_write nvmem_config API is defined. In case of MTD framework, 
reg_write is not defined in nvmem_config.

Based on the comments made, it seems that we also agree that this write 
protection should be handled by MTD subsystems or associated drivers and 
not by MVMEN framework for MTD use cases.

The proposal implementation should solve this conflict for MTD framework 
without breaking anything for others NVMEM users (EEPROMs flashes for 
example).

Regards,
Christophe Kerello.

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Miquel Raynal]

Hi Christophe,

christophe.kerello@foss.st.com wrote on Wed, 2 Feb 2022 11:44:31 +0100:

> Hi,
> 
> On 2/1/22 11:47, Pratyush Yadav wrote:
> > On 31/01/22 02:43PM, Miquel Raynal wrote:  
> >> Hi Vignesh, Tudory, Pratyush,
> >>
> >> + Tudor and Pratyush
> >>
> >> christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 +0100:
> >>  
> >>> Wp-gpios property can be used on NVMEM nodes and the same property can
> >>> be also used on MTD NAND nodes. In case of the wp-gpios property is
> >>> defined at NAND level node, the GPIO management is done at NAND driver
> >>> level. Write protect is disabled when the driver is probed or resumed
> >>> and is enabled when the driver is released or suspended.
> >>>
> >>> When no partitions are defined in the NAND DT node, then the NAND DT node
> >>> will be passed to NVMEM framework. If wp-gpios property is defined in
> >>> this node, the GPIO resource is taken twice and the NAND controller
> >>> driver fails to probe.
> >>>
> >>> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
> >>> In case skip_wp_gpio is set, it means that the GPIO is handled by the
> >>> provider. Lets set this flag in MTD layer to avoid the conflict on
> >>> wp_gpios property.
> >>>
> >>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> >>> ---
> >>>   drivers/mtd/mtdcore.c | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> >>> index 70f492dce158..e6d251594def 100644
> >>> --- a/drivers/mtd/mtdcore.c
> >>> +++ b/drivers/mtd/mtdcore.c
> >>> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
> >>>   	config.stride = 1;
> >>>   	config.read_only = true;
> >>>   	config.root_only = true;
> >>> +	config.skip_wp_gpio = true;
> >>>   	config.no_of_node = !of_device_is_compatible(node, "nvmem-cells");
> >>>   	config.priv = mtd;  
> >>>   >>> @@ -833,6 +834,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,  
> >>>   	config.owner = THIS_MODULE;
> >>>   	config.type = NVMEM_TYPE_OTP;
> >>>   	config.root_only = true;
> >>> +	config.skip_wp_gpio = true;
> >>>   	config.reg_read = reg_read;
> >>>   	config.size = size;
> >>>   	config.of_node = np;  
> >>
> >> TLDR: There is a conflict between MTD and NVMEM, who should handle the
> >> WP pin when there is one? At least for raw NAND devices, I don't want
> >> the NVMEM core to handle the wp pin. So we've introduced this
> >> skip_wp_gpio nvmem config option. But there are two places where this
> >> boolean can be set and one of these is for otp regions (see above). In
> >> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
> >> nvmem protection. Please tell us if you think this is fine for you.  
> > 
> > Why does NVMEM touch hardware write protection in the first place? The
> > purpose of the framework is to provide a way to retrieve config stored
> > in memory. It has no business dealing with details of the chip like the
> > WP line. That should be MTD's job (which it should delegate to SPI NOR,
> > SPI NAND, etc.). If you want to write protect a cell then do it in
> > software. I don't see why NVMEM should be dealing with hardware directly
> > at all.
> > 
> > That is my mental model of how things _should_ work. I have not spent
> > much time digging into how things actually work currently.
> >   
> 
> Wp-gpios property management was added in MVMEM framework in January 2020 => sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
> "
>      nvmem: add support for the write-protect pin

Perhaps this could be pointed as part of a Fixes tag, because this
might actually break other users which we haven't noticed yet.

> 
>      The write-protect pin handling looks like a standard property that
>      could benefit other users if available in the core nvmem framework.
> 
>      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.
> "
> 
> And this modification was done for EEPROMs flashes => sha1: 1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
> "
>      eeprom: at24: remove the write-protect pin support
> 
>      NVMEM framework is an interface for the at24 EEPROMs as well as for
>      other drivers, instead of passing the wp-gpios over the different
>      drivers each time, it would be better to pass it over the NVMEM
>      subsystem once and for all.
> 
>      Removing the support for the write-protect pin after adding it to
>      the NVMEM subsystem.
> "
> 
> Current NVMEM framework implementation toggles the WP GPIO when reg_write nvmem_config API is defined. In case of MTD framework, reg_write is not defined in nvmem_config.
> 
> Based on the comments made, it seems that we also agree that this write protection should be handled by MTD subsystems or associated drivers and not by MVMEN framework for MTD use cases.
> 
> The proposal implementation should solve this conflict for MTD framework without breaking anything for others NVMEM users (EEPROMs flashes for example).

I'm not sure neither why EEPROM flashes decided to delegate the
wp handling to NVMEM, but in any case we don't want it to be
handled at NVMEM level in the case of MTD, so your series looks fine to
me.

> 
> Regards,
> Christophe Kerello.
> 
> 


Thanks,
Miquèl

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Pratyush Yadav]

+ Khouloud, Linus, Bartosz

On 02/02/22 11:44AM, Christophe Kerello wrote:
> Hi,
> 
> On 2/1/22 11:47, Pratyush Yadav wrote:
> > On 31/01/22 02:43PM, Miquel Raynal wrote:
> > > Hi Vignesh, Tudory, Pratyush,
> > > 
> > > + Tudor and Pratyush
> > > 
> > > christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 +0100:
> > > 
> > > > Wp-gpios property can be used on NVMEM nodes and the same property can
> > > > be also used on MTD NAND nodes. In case of the wp-gpios property is
> > > > defined at NAND level node, the GPIO management is done at NAND driver
> > > > level. Write protect is disabled when the driver is probed or resumed
> > > > and is enabled when the driver is released or suspended.
> > > > 
> > > > When no partitions are defined in the NAND DT node, then the NAND DT node
> > > > will be passed to NVMEM framework. If wp-gpios property is defined in
> > > > this node, the GPIO resource is taken twice and the NAND controller
> > > > driver fails to probe.
> > > > 
> > > > A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
> > > > In case skip_wp_gpio is set, it means that the GPIO is handled by the
> > > > provider. Lets set this flag in MTD layer to avoid the conflict on
> > > > wp_gpios property.
> > > > 
> > > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> > > > ---
> > > >   drivers/mtd/mtdcore.c | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > > > index 70f492dce158..e6d251594def 100644
> > > > --- a/drivers/mtd/mtdcore.c
> > > > +++ b/drivers/mtd/mtdcore.c
> > > > @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
> > > >   	config.stride = 1;
> > > >   	config.read_only = true;
> > > >   	config.root_only = true;
> > > > +	config.skip_wp_gpio = true;
> > > >   	config.no_of_node = !of_device_is_compatible(node, "nvmem-cells");
> > > >   	config.priv = mtd;
> > > > @@ -833,6 +834,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
> > > >   	config.owner = THIS_MODULE;
> > > >   	config.type = NVMEM_TYPE_OTP;
> > > >   	config.root_only = true;
> > > > +	config.skip_wp_gpio = true;
> > > >   	config.reg_read = reg_read;
> > > >   	config.size = size;
> > > >   	config.of_node = np;
> > > 
> > > TLDR: There is a conflict between MTD and NVMEM, who should handle the
> > > WP pin when there is one? At least for raw NAND devices, I don't want
> > > the NVMEM core to handle the wp pin. So we've introduced this
> > > skip_wp_gpio nvmem config option. But there are two places where this
> > > boolean can be set and one of these is for otp regions (see above). In
> > > this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
> > > nvmem protection. Please tell us if you think this is fine for you.
> > 
> > Why does NVMEM touch hardware write protection in the first place? The
> > purpose of the framework is to provide a way to retrieve config stored
> > in memory. It has no business dealing with details of the chip like the
> > WP line. That should be MTD's job (which it should delegate to SPI NOR,
> > SPI NAND, etc.). If you want to write protect a cell then do it in
> > software. I don't see why NVMEM should be dealing with hardware directly
> > at all.
> > 
> > That is my mental model of how things _should_ work. I have not spent
> > much time digging into how things actually work currently.
> > 
> 
> Wp-gpios property management was added in MVMEM framework in January 2020 =>
> sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
> "
>     nvmem: add support for the write-protect pin
> 
>     The write-protect pin handling looks like a standard property that
>     could benefit other users if available in the core nvmem framework.
> 
>     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.
> "
> 
> And this modification was done for EEPROMs flashes => sha1:
> 1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
> "
>     eeprom: at24: remove the write-protect pin support
> 
>     NVMEM framework is an interface for the at24 EEPROMs as well as for
>     other drivers, instead of passing the wp-gpios over the different
>     drivers each time, it would be better to pass it over the NVMEM
>     subsystem once and for all.
> 
>     Removing the support for the write-protect pin after adding it to
>     the NVMEM subsystem.
> "
> 
> Current NVMEM framework implementation toggles the WP GPIO when reg_write
> nvmem_config API is defined. In case of MTD framework, reg_write is not
> defined in nvmem_config.

Thanks for digging these up. I think this was the wrong decision to 
make. NVMEM should just provide the APIs for handling read/write, and 
leave the rest to the drivers.

It might be convenient for some drivers to put the WP GPIO handling to 
NVMEM core but I just don't think it is the job of the framework to deal 
with this, and it just does not know enough about the device to deal 
with correctly and completely anyway. For example, wp-gpio is only one 
of the ways to write protect a chip. SPI NOR flashes have a WP# pin that 
is often toggled via the SPI controller. There could also be registers 
that do the write protection.

One would have to make strong justifications for making nvmem directly 
deal with hardware level details to convince me it is a good idea. IMHO 
if AT24 EEPROM is the only driver relying on this as of now then we 
should just revert the patches and not have to deal with the 
skip_wp_gpio hackery.

> 
> Based on the comments made, it seems that we also agree that this write
> protection should be handled by MTD subsystems or associated drivers and not
> by MVMEN framework for MTD use cases.
> 
> The proposal implementation should solve this conflict for MTD framework
> without breaking anything for others NVMEM users (EEPROMs flashes for
> example).

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Christophe Kerello]

Hi Miquel, Pratyush, Srinivas,

On 2/2/22 12:53, Pratyush Yadav wrote:
> + Khouloud, Linus, Bartosz
> 
> On 02/02/22 11:44AM, Christophe Kerello wrote:
>> Hi,
>>
>> On 2/1/22 11:47, Pratyush Yadav wrote:
>>> On 31/01/22 02:43PM, Miquel Raynal wrote:
>>>> Hi Vignesh, Tudory, Pratyush,
>>>>
>>>> + Tudor and Pratyush
>>>>
>>>> christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 +0100:
>>>>
>>>>> Wp-gpios property can be used on NVMEM nodes and the same property can
>>>>> be also used on MTD NAND nodes. In case of the wp-gpios property is
>>>>> defined at NAND level node, the GPIO management is done at NAND driver
>>>>> level. Write protect is disabled when the driver is probed or resumed
>>>>> and is enabled when the driver is released or suspended.
>>>>>
>>>>> When no partitions are defined in the NAND DT node, then the NAND DT node
>>>>> will be passed to NVMEM framework. If wp-gpios property is defined in
>>>>> this node, the GPIO resource is taken twice and the NAND controller
>>>>> driver fails to probe.
>>>>>
>>>>> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
>>>>> In case skip_wp_gpio is set, it means that the GPIO is handled by the
>>>>> provider. Lets set this flag in MTD layer to avoid the conflict on
>>>>> wp_gpios property.
>>>>>
>>>>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>>>>> ---
>>>>>    drivers/mtd/mtdcore.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>>>>> index 70f492dce158..e6d251594def 100644
>>>>> --- a/drivers/mtd/mtdcore.c
>>>>> +++ b/drivers/mtd/mtdcore.c
>>>>> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>>>>>    	config.stride = 1;
>>>>>    	config.read_only = true;
>>>>>    	config.root_only = true;
>>>>> +	config.skip_wp_gpio = true;
>>>>>    	config.no_of_node = !of_device_is_compatible(node, "nvmem-cells");
>>>>>    	config.priv = mtd;
>>>>> @@ -833,6 +834,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd,
>>>>>    	config.owner = THIS_MODULE;
>>>>>    	config.type = NVMEM_TYPE_OTP;
>>>>>    	config.root_only = true;
>>>>> +	config.skip_wp_gpio = true;
>>>>>    	config.reg_read = reg_read;
>>>>>    	config.size = size;
>>>>>    	config.of_node = np;
>>>>
>>>> TLDR: There is a conflict between MTD and NVMEM, who should handle the
>>>> WP pin when there is one? At least for raw NAND devices, I don't want
>>>> the NVMEM core to handle the wp pin. So we've introduced this
>>>> skip_wp_gpio nvmem config option. But there are two places where this
>>>> boolean can be set and one of these is for otp regions (see above). In
>>>> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
>>>> nvmem protection. Please tell us if you think this is fine for you.
>>>
>>> Why does NVMEM touch hardware write protection in the first place? The
>>> purpose of the framework is to provide a way to retrieve config stored
>>> in memory. It has no business dealing with details of the chip like the
>>> WP line. That should be MTD's job (which it should delegate to SPI NOR,
>>> SPI NAND, etc.). If you want to write protect a cell then do it in
>>> software. I don't see why NVMEM should be dealing with hardware directly
>>> at all.
>>>
>>> That is my mental model of how things _should_ work. I have not spent
>>> much time digging into how things actually work currently.
>>>
>>
>> Wp-gpios property management was added in MVMEM framework in January 2020 =>
>> sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
>> "
>>      nvmem: add support for the write-protect pin
>>
>>      The write-protect pin handling looks like a standard property that
>>      could benefit other users if available in the core nvmem framework.
>>
>>      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.
>> "
>>
>> And this modification was done for EEPROMs flashes => sha1:
>> 1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
>> "
>>      eeprom: at24: remove the write-protect pin support
>>
>>      NVMEM framework is an interface for the at24 EEPROMs as well as for
>>      other drivers, instead of passing the wp-gpios over the different
>>      drivers each time, it would be better to pass it over the NVMEM
>>      subsystem once and for all.
>>
>>      Removing the support for the write-protect pin after adding it to
>>      the NVMEM subsystem.
>> "
>>
>> Current NVMEM framework implementation toggles the WP GPIO when reg_write
>> nvmem_config API is defined. In case of MTD framework, reg_write is not
>> defined in nvmem_config.
> 
> Thanks for digging these up. I think this was the wrong decision to
> make. NVMEM should just provide the APIs for handling read/write, and
> leave the rest to the drivers.
> 
> It might be convenient for some drivers to put the WP GPIO handling to
> NVMEM core but I just don't think it is the job of the framework to deal
> with this, and it just does not know enough about the device to deal
> with correctly and completely anyway. For example, wp-gpio is only one
> of the ways to write protect a chip. SPI NOR flashes have a WP# pin that
> is often toggled via the SPI controller. There could also be registers
> that do the write protection.
> 
> One would have to make strong justifications for making nvmem directly
> deal with hardware level details to convince me it is a good idea. IMHO
> if AT24 EEPROM is the only driver relying on this as of now then we
> should just revert the patches and not have to deal with the
> skip_wp_gpio hackery.
> 

Based on the  bindings documentation, AT24 EEPROM driver is not the only
driver relying on this implementation. At least, AT25 EEPROM driver will
have to be modified to handle the Write Protect management, and there is
probably others drivers relying on this implementation.

So, should we keep the legacy and apply the proposal patch to fix this
conflict (I can send a V3 with a fixes tag on patch 3 and 4 as
recommended by Miquel)?
Or should we revert the Write Protect management in NVMEM framework
but in this case I will not be able to handle such modifications (I am
not able to test those drivers).

Regards,
Christophe Kerello.

>>
>> Based on the comments made, it seems that we also agree that this write
>> protection should be handled by MTD subsystems or associated drivers and not
>> by MVMEN framework for MTD use cases.
>>
>> The proposal implementation should solve this conflict for MTD framework
>> without breaking anything for others NVMEM users (EEPROMs flashes for
>> example).
> 

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Srinivas Kandagatla]

On 16/02/2022 08:46, Christophe Kerello wrote:
> Hi Miquel, Pratyush, Srinivas,
> 
> On 2/2/22 12:53, Pratyush Yadav wrote:
>> + Khouloud, Linus, Bartosz
>>
>> On 02/02/22 11:44AM, Christophe Kerello wrote:
>>> Hi,
>>>
>>> On 2/1/22 11:47, Pratyush Yadav wrote:
>>>> On 31/01/22 02:43PM, Miquel Raynal wrote:
>>>>> Hi Vignesh, Tudory, Pratyush,
>>>>>
>>>>> + Tudor and Pratyush
>>>>>
>>>>> christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 
>>>>> +0100:
>>>>>
>>>>>> Wp-gpios property can be used on NVMEM nodes and the same property 
>>>>>> can
>>>>>> be also used on MTD NAND nodes. In case of the wp-gpios property is
>>>>>> defined at NAND level node, the GPIO management is done at NAND 
>>>>>> driver
>>>>>> level. Write protect is disabled when the driver is probed or resumed
>>>>>> and is enabled when the driver is released or suspended.
>>>>>>
>>>>>> When no partitions are defined in the NAND DT node, then the NAND 
>>>>>> DT node
>>>>>> will be passed to NVMEM framework. If wp-gpios property is defined in
>>>>>> this node, the GPIO resource is taken twice and the NAND controller
>>>>>> driver fails to probe.
>>>>>>
>>>>>> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
>>>>>> In case skip_wp_gpio is set, it means that the GPIO is handled by the
>>>>>> provider. Lets set this flag in MTD layer to avoid the conflict on
>>>>>> wp_gpios property.
>>>>>>
>>>>>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>>>>>> ---
>>>>>>    drivers/mtd/mtdcore.c | 2 ++
>>>>>>    1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>>>>>> index 70f492dce158..e6d251594def 100644
>>>>>> --- a/drivers/mtd/mtdcore.c
>>>>>> +++ b/drivers/mtd/mtdcore.c
>>>>>> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>>>>>>        config.stride = 1;
>>>>>>        config.read_only = true;
>>>>>>        config.root_only = true;
>>>>>> +    config.skip_wp_gpio = true;
>>>>>>        config.no_of_node = !of_device_is_compatible(node, 
>>>>>> "nvmem-cells");
>>>>>>        config.priv = mtd;
>>>>>> @@ -833,6 +834,7 @@ static struct nvmem_device 
>>>>>> *mtd_otp_nvmem_register(struct mtd_info *mtd,
>>>>>>        config.owner = THIS_MODULE;
>>>>>>        config.type = NVMEM_TYPE_OTP;
>>>>>>        config.root_only = true;
>>>>>> +    config.skip_wp_gpio = true;
>>>>>>        config.reg_read = reg_read;
>>>>>>        config.size = size;
>>>>>>        config.of_node = np;
>>>>>
>>>>> TLDR: There is a conflict between MTD and NVMEM, who should handle the
>>>>> WP pin when there is one? At least for raw NAND devices, I don't want
>>>>> the NVMEM core to handle the wp pin. So we've introduced this
>>>>> skip_wp_gpio nvmem config option. But there are two places where this
>>>>> boolean can be set and one of these is for otp regions (see above). In
>>>>> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
>>>>> nvmem protection. Please tell us if you think this is fine for you.
>>>>
>>>> Why does NVMEM touch hardware write protection in the first place? The
>>>> purpose of the framework is to provide a way to retrieve config stored
>>>> in memory. It has no business dealing with details of the chip like the
>>>> WP line. That should be MTD's job (which it should delegate to SPI NOR,
>>>> SPI NAND, etc.). If you want to write protect a cell then do it in
>>>> software. I don't see why NVMEM should be dealing with hardware 
>>>> directly
>>>> at all.
>>>>
>>>> That is my mental model of how things _should_ work. I have not spent
>>>> much time digging into how things actually work currently.
>>>>
>>>
>>> Wp-gpios property management was added in MVMEM framework in January 
>>> 2020 =>
>>> sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
>>> "
>>>      nvmem: add support for the write-protect pin
>>>
>>>      The write-protect pin handling looks like a standard property that
>>>      could benefit other users if available in the core nvmem framework.
>>>
>>>      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.
>>> "
>>>
>>> And this modification was done for EEPROMs flashes => sha1:
>>> 1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
>>> "
>>>      eeprom: at24: remove the write-protect pin support
>>>
>>>      NVMEM framework is an interface for the at24 EEPROMs as well as for
>>>      other drivers, instead of passing the wp-gpios over the different
>>>      drivers each time, it would be better to pass it over the NVMEM
>>>      subsystem once and for all.
>>>
>>>      Removing the support for the write-protect pin after adding it to
>>>      the NVMEM subsystem.
>>> "
>>>
>>> Current NVMEM framework implementation toggles the WP GPIO when 
>>> reg_write
>>> nvmem_config API is defined. In case of MTD framework, reg_write is not
>>> defined in nvmem_config.
>>
>> Thanks for digging these up. I think this was the wrong decision to
>> make. NVMEM should just provide the APIs for handling read/write, and
>> leave the rest to the drivers.
>>
>> It might be convenient for some drivers to put the WP GPIO handling to
>> NVMEM core but I just don't think it is the job of the framework to deal
>> with this, and it just does not know enough about the device to deal
>> with correctly and completely anyway. For example, wp-gpio is only one
>> of the ways to write protect a chip. SPI NOR flashes have a WP# pin that
>> is often toggled via the SPI controller. There could also be registers
>> that do the write protection.
>>
>> One would have to make strong justifications for making nvmem directly
>> deal with hardware level details to convince me it is a good idea. IMHO
>> if AT24 EEPROM is the only driver relying on this as of now then we
>> should just revert the patches and not have to deal with the
>> skip_wp_gpio hackery.
>>
> 
> Based on the  bindings documentation, AT24 EEPROM driver is not the only
> driver relying on this implementation. At least, AT25 EEPROM driver will
> have to be modified to handle the Write Protect management, and there is
> probably others drivers relying on this implementation.
> 
> So, should we keep the legacy and apply the proposal patch to fix this
> conflict (I can send a V3 with a fixes tag on patch 3 and 4 as
> recommended by Miquel)?
> Or should we revert the Write Protect management in NVMEM framework
> but in this case I will not be able to handle such modifications (I am
> not able to test those drivers).

Firstly sorry for such a long delay to reply this thread as I was in travel.

I agree with comments from Pratyush but I see no harm in handling simple 
usecases of write-protect gpio in nvmem core. In fact wp-gpios and 
read-only are part of nvmem provider bindings.

But usecases like the ones described in this patch series which do not 
want nvmem core to deal with this pin should set this new flag. I think 
this is a balanced choice.

reverting the wp-gpio patch is going to break other providers that are 
using this bindings.

--srini


> 
> Regards,
> Christophe Kerello.
> 
>>>
>>> Based on the comments made, it seems that we also agree that this write
>>> protection should be handled by MTD subsystems or associated drivers 
>>> and not
>>> by MVMEN framework for MTD use cases.
>>>
>>> The proposal implementation should solve this conflict for MTD framework
>>> without breaking anything for others NVMEM users (EEPROMs flashes for
>>> example).
>>

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Miquel Raynal]

Hi Srinivas,

srinivas.kandagatla@linaro.org wrote on Wed, 16 Feb 2022 09:24:29 +0000:

> On 16/02/2022 08:46, Christophe Kerello wrote:
> > Hi Miquel, Pratyush, Srinivas,
> > 
> > On 2/2/22 12:53, Pratyush Yadav wrote:  
> >> + Khouloud, Linus, Bartosz
> >>
> >> On 02/02/22 11:44AM, Christophe Kerello wrote:  
> >>> Hi,
> >>>
> >>> On 2/1/22 11:47, Pratyush Yadav wrote:  
> >>>> On 31/01/22 02:43PM, Miquel Raynal wrote:  
> >>>>> Hi Vignesh, Tudory, Pratyush,
> >>>>>
> >>>>> + Tudor and Pratyush
> >>>>>
> >>>>> christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 >>>>> +0100:
> >>>>>  
> >>>>>> Wp-gpios property can be used on NVMEM nodes and the same property >>>>>> can
> >>>>>> be also used on MTD NAND nodes. In case of the wp-gpios property is
> >>>>>> defined at NAND level node, the GPIO management is done at NAND >>>>>> driver
> >>>>>> level. Write protect is disabled when the driver is probed or resumed
> >>>>>> and is enabled when the driver is released or suspended.
> >>>>>>
> >>>>>> When no partitions are defined in the NAND DT node, then the NAND >>>>>> DT node
> >>>>>> will be passed to NVMEM framework. If wp-gpios property is defined in
> >>>>>> this node, the GPIO resource is taken twice and the NAND controller
> >>>>>> driver fails to probe.
> >>>>>>
> >>>>>> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
> >>>>>> In case skip_wp_gpio is set, it means that the GPIO is handled by the
> >>>>>> provider. Lets set this flag in MTD layer to avoid the conflict on
> >>>>>> wp_gpios property.
> >>>>>>
> >>>>>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> >>>>>> ---
> >>>>>>    drivers/mtd/mtdcore.c | 2 ++
> >>>>>>    1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> >>>>>> index 70f492dce158..e6d251594def 100644
> >>>>>> --- a/drivers/mtd/mtdcore.c
> >>>>>> +++ b/drivers/mtd/mtdcore.c
> >>>>>> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
> >>>>>>        config.stride = 1;
> >>>>>>        config.read_only = true;
> >>>>>>        config.root_only = true;
> >>>>>> +    config.skip_wp_gpio = true;
> >>>>>>        config.no_of_node = !of_device_is_compatible(node, >>>>>> "nvmem-cells");
> >>>>>>        config.priv = mtd;
> >>>>>> @@ -833,6 +834,7 @@ static struct nvmem_device >>>>>> *mtd_otp_nvmem_register(struct mtd_info *mtd,
> >>>>>>        config.owner = THIS_MODULE;
> >>>>>>        config.type = NVMEM_TYPE_OTP;
> >>>>>>        config.root_only = true;
> >>>>>> +    config.skip_wp_gpio = true;
> >>>>>>        config.reg_read = reg_read;
> >>>>>>        config.size = size;
> >>>>>>        config.of_node = np;  
> >>>>>
> >>>>> TLDR: There is a conflict between MTD and NVMEM, who should handle the
> >>>>> WP pin when there is one? At least for raw NAND devices, I don't want
> >>>>> the NVMEM core to handle the wp pin. So we've introduced this
> >>>>> skip_wp_gpio nvmem config option. But there are two places where this
> >>>>> boolean can be set and one of these is for otp regions (see above). In
> >>>>> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
> >>>>> nvmem protection. Please tell us if you think this is fine for you.  
> >>>>
> >>>> Why does NVMEM touch hardware write protection in the first place? The
> >>>> purpose of the framework is to provide a way to retrieve config stored
> >>>> in memory. It has no business dealing with details of the chip like the
> >>>> WP line. That should be MTD's job (which it should delegate to SPI NOR,
> >>>> SPI NAND, etc.). If you want to write protect a cell then do it in
> >>>> software. I don't see why NVMEM should be dealing with hardware >>>> directly
> >>>> at all.
> >>>>
> >>>> That is my mental model of how things _should_ work. I have not spent
> >>>> much time digging into how things actually work currently.
> >>>>  
> >>>
> >>> Wp-gpios property management was added in MVMEM framework in January >>> 2020 =>
> >>> sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
> >>> "
> >>>      nvmem: add support for the write-protect pin
> >>>
> >>>      The write-protect pin handling looks like a standard property that
> >>>      could benefit other users if available in the core nvmem framework.
> >>>
> >>>      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.
> >>> "
> >>>
> >>> And this modification was done for EEPROMs flashes => sha1:
> >>> 1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
> >>> "
> >>>      eeprom: at24: remove the write-protect pin support
> >>>
> >>>      NVMEM framework is an interface for the at24 EEPROMs as well as for
> >>>      other drivers, instead of passing the wp-gpios over the different
> >>>      drivers each time, it would be better to pass it over the NVMEM
> >>>      subsystem once and for all.
> >>>
> >>>      Removing the support for the write-protect pin after adding it to
> >>>      the NVMEM subsystem.
> >>> "
> >>>
> >>> Current NVMEM framework implementation toggles the WP GPIO when >>> reg_write
> >>> nvmem_config API is defined. In case of MTD framework, reg_write is not
> >>> defined in nvmem_config.  
> >>
> >> Thanks for digging these up. I think this was the wrong decision to
> >> make. NVMEM should just provide the APIs for handling read/write, and
> >> leave the rest to the drivers.
> >>
> >> It might be convenient for some drivers to put the WP GPIO handling to
> >> NVMEM core but I just don't think it is the job of the framework to deal
> >> with this, and it just does not know enough about the device to deal
> >> with correctly and completely anyway. For example, wp-gpio is only one
> >> of the ways to write protect a chip. SPI NOR flashes have a WP# pin that
> >> is often toggled via the SPI controller. There could also be registers
> >> that do the write protection.
> >>
> >> One would have to make strong justifications for making nvmem directly
> >> deal with hardware level details to convince me it is a good idea. IMHO
> >> if AT24 EEPROM is the only driver relying on this as of now then we
> >> should just revert the patches and not have to deal with the
> >> skip_wp_gpio hackery.
> >>  
> > 
> > Based on the  bindings documentation, AT24 EEPROM driver is not the only
> > driver relying on this implementation. At least, AT25 EEPROM driver will
> > have to be modified to handle the Write Protect management, and there is
> > probably others drivers relying on this implementation.
> > 
> > So, should we keep the legacy and apply the proposal patch to fix this
> > conflict (I can send a V3 with a fixes tag on patch 3 and 4 as
> > recommended by Miquel)?
> > Or should we revert the Write Protect management in NVMEM framework
> > but in this case I will not be able to handle such modifications (I am
> > not able to test those drivers).  
> 
> Firstly sorry for such a long delay to reply this thread as I was in travel.
> 
> I agree with comments from Pratyush but I see no harm in handling simple usecases of write-protect gpio in nvmem core. In fact wp-gpios and read-only are part of nvmem provider bindings.
> 
> But usecases like the ones described in this patch series which do not want nvmem core to deal with this pin should set this new flag. I think this is a balanced choice.
> 
> reverting the wp-gpio patch is going to break other providers that are using this bindings.

I am always puzzled when the community deeply cares about non-mainline
drivers.

On one side I can understand that creating a 'grab-the-wp-line'
flag would immediately break all external users but this is, as
reported by Pratyush, the more logical approach IMHO. However we might
possibly miss situations where the flag is necessary and break these
devices.

Otherwise the 'ignore-wp' flag is more conservative, it does not
break the existing users but would just address the MTD case for now, we
might have other in-tree subsystem that are affected.

I'm fine either way TBH :-) I would just like this patchset to go in
through the next merge window.

Thanks,
Miquèl

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Srinivas Kandagatla]

On 17/02/2022 08:48, Miquel Raynal wrote:
> Hi Srinivas,
> 
> srinivas.kandagatla@linaro.org wrote on Wed, 16 Feb 2022 09:24:29 +0000:
> 
>> On 16/02/2022 08:46, Christophe Kerello wrote:
>>> Hi Miquel, Pratyush, Srinivas,
>>>
>>> On 2/2/22 12:53, Pratyush Yadav wrote:
>>>> + Khouloud, Linus, Bartosz
>>>>
>>>> On 02/02/22 11:44AM, Christophe Kerello wrote:
>>>>> Hi,
>>>>>
>>>>> On 2/1/22 11:47, Pratyush Yadav wrote:
>>>>>> On 31/01/22 02:43PM, Miquel Raynal wrote:
>>>>>>> Hi Vignesh, Tudory, Pratyush,
>>>>>>>
>>>>>>> + Tudor and Pratyush
>>>>>>>
>>>>>>> christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 >>>>> +0100:
>>>>>>>   
>>>>>>>> Wp-gpios property can be used on NVMEM nodes and the same property >>>>>> can
>>>>>>>> be also used on MTD NAND nodes. In case of the wp-gpios property is
>>>>>>>> defined at NAND level node, the GPIO management is done at NAND >>>>>> driver
>>>>>>>> level. Write protect is disabled when the driver is probed or resumed
>>>>>>>> and is enabled when the driver is released or suspended.
>>>>>>>>
>>>>>>>> When no partitions are defined in the NAND DT node, then the NAND >>>>>> DT node
>>>>>>>> will be passed to NVMEM framework. If wp-gpios property is defined in
>>>>>>>> this node, the GPIO resource is taken twice and the NAND controller
>>>>>>>> driver fails to probe.
>>>>>>>>
>>>>>>>> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
>>>>>>>> In case skip_wp_gpio is set, it means that the GPIO is handled by the
>>>>>>>> provider. Lets set this flag in MTD layer to avoid the conflict on
>>>>>>>> wp_gpios property.
>>>>>>>>
>>>>>>>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>>>>>>>> ---
>>>>>>>>     drivers/mtd/mtdcore.c | 2 ++
>>>>>>>>     1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>>>>>>>> index 70f492dce158..e6d251594def 100644
>>>>>>>> --- a/drivers/mtd/mtdcore.c
>>>>>>>> +++ b/drivers/mtd/mtdcore.c
>>>>>>>> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
>>>>>>>>         config.stride = 1;
>>>>>>>>         config.read_only = true;
>>>>>>>>         config.root_only = true;
>>>>>>>> +    config.skip_wp_gpio = true;
>>>>>>>>         config.no_of_node = !of_device_is_compatible(node, >>>>>> "nvmem-cells");
>>>>>>>>         config.priv = mtd;
>>>>>>>> @@ -833,6 +834,7 @@ static struct nvmem_device >>>>>> *mtd_otp_nvmem_register(struct mtd_info *mtd,
>>>>>>>>         config.owner = THIS_MODULE;
>>>>>>>>         config.type = NVMEM_TYPE_OTP;
>>>>>>>>         config.root_only = true;
>>>>>>>> +    config.skip_wp_gpio = true;
>>>>>>>>         config.reg_read = reg_read;
>>>>>>>>         config.size = size;
>>>>>>>>         config.of_node = np;
>>>>>>>
>>>>>>> TLDR: There is a conflict between MTD and NVMEM, who should handle the
>>>>>>> WP pin when there is one? At least for raw NAND devices, I don't want
>>>>>>> the NVMEM core to handle the wp pin. So we've introduced this
>>>>>>> skip_wp_gpio nvmem config option. But there are two places where this
>>>>>>> boolean can be set and one of these is for otp regions (see above). In
>>>>>>> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
>>>>>>> nvmem protection. Please tell us if you think this is fine for you.
>>>>>>
>>>>>> Why does NVMEM touch hardware write protection in the first place? The
>>>>>> purpose of the framework is to provide a way to retrieve config stored
>>>>>> in memory. It has no business dealing with details of the chip like the
>>>>>> WP line. That should be MTD's job (which it should delegate to SPI NOR,
>>>>>> SPI NAND, etc.). If you want to write protect a cell then do it in
>>>>>> software. I don't see why NVMEM should be dealing with hardware >>>> directly
>>>>>> at all.
>>>>>>
>>>>>> That is my mental model of how things _should_ work. I have not spent
>>>>>> much time digging into how things actually work currently.
>>>>>>   
>>>>>
>>>>> Wp-gpios property management was added in MVMEM framework in January >>> 2020 =>
>>>>> sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
>>>>> "
>>>>>       nvmem: add support for the write-protect pin
>>>>>
>>>>>       The write-protect pin handling looks like a standard property that
>>>>>       could benefit other users if available in the core nvmem framework.
>>>>>
>>>>>       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.
>>>>> "
>>>>>
>>>>> And this modification was done for EEPROMs flashes => sha1:
>>>>> 1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
>>>>> "
>>>>>       eeprom: at24: remove the write-protect pin support
>>>>>
>>>>>       NVMEM framework is an interface for the at24 EEPROMs as well as for
>>>>>       other drivers, instead of passing the wp-gpios over the different
>>>>>       drivers each time, it would be better to pass it over the NVMEM
>>>>>       subsystem once and for all.
>>>>>
>>>>>       Removing the support for the write-protect pin after adding it to
>>>>>       the NVMEM subsystem.
>>>>> "
>>>>>
>>>>> Current NVMEM framework implementation toggles the WP GPIO when >>> reg_write
>>>>> nvmem_config API is defined. In case of MTD framework, reg_write is not
>>>>> defined in nvmem_config.
>>>>
>>>> Thanks for digging these up. I think this was the wrong decision to
>>>> make. NVMEM should just provide the APIs for handling read/write, and
>>>> leave the rest to the drivers.
>>>>
>>>> It might be convenient for some drivers to put the WP GPIO handling to
>>>> NVMEM core but I just don't think it is the job of the framework to deal
>>>> with this, and it just does not know enough about the device to deal
>>>> with correctly and completely anyway. For example, wp-gpio is only one
>>>> of the ways to write protect a chip. SPI NOR flashes have a WP# pin that
>>>> is often toggled via the SPI controller. There could also be registers
>>>> that do the write protection.
>>>>
>>>> One would have to make strong justifications for making nvmem directly
>>>> deal with hardware level details to convince me it is a good idea. IMHO
>>>> if AT24 EEPROM is the only driver relying on this as of now then we
>>>> should just revert the patches and not have to deal with the
>>>> skip_wp_gpio hackery.
>>>>   
>>>
>>> Based on the  bindings documentation, AT24 EEPROM driver is not the only
>>> driver relying on this implementation. At least, AT25 EEPROM driver will
>>> have to be modified to handle the Write Protect management, and there is
>>> probably others drivers relying on this implementation.
>>>
>>> So, should we keep the legacy and apply the proposal patch to fix this
>>> conflict (I can send a V3 with a fixes tag on patch 3 and 4 as
>>> recommended by Miquel)?
>>> Or should we revert the Write Protect management in NVMEM framework
>>> but in this case I will not be able to handle such modifications (I am
>>> not able to test those drivers).
>>
>> Firstly sorry for such a long delay to reply this thread as I was in travel.
>>
>> I agree with comments from Pratyush but I see no harm in handling simple usecases of write-protect gpio in nvmem core. In fact wp-gpios and read-only are part of nvmem provider bindings.
>>
>> But usecases like the ones described in this patch series which do not want nvmem core to deal with this pin should set this new flag. I think this is a balanced choice.
>>
>> reverting the wp-gpio patch is going to break other providers that are using this bindings.
> 
> I am always puzzled when the community deeply cares about non-mainline
> drivers.
> 
> On one side I can understand that creating a 'grab-the-wp-line'
> flag would immediately break all external users but this is, as
> reported by Pratyush, the more logical approach IMHO. However we might
> possibly miss situations where the flag is necessary and break these
> devices.
> 
> Otherwise the 'ignore-wp' flag is more conservative, it does not

ingore-wp seems to be more sensible flag than skip_wp_gpio flag.


> break the existing users but would just address the MTD case for now, we
> might have other in-tree subsystem that are affected.
> 
> I'm fine either way TBH :-) I would just like this patchset to go in

Am okay either way too, It is just that ingore-wp seems a balanced 
choice in the current situation :-).

> through the next merge window.
Sure.

I can Ack nvmem patch if you wish to carry it via mtd tree.

or

nvmem patches go via Greg's char-misc tree. I can take 4/4 if you ack it 
once V3 is sent


--srini
> 
> Thanks,
> Miquèl

Re: [PATCH v2 3/4] nvmem: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Srinivas Kandagatla]

On 31/01/2022 09:57, Christophe Kerello wrote:
> Wp-gpios property can be used on NVMEM nodes and the same property can
> be also used on MTD NAND nodes. In case of the wp-gpios property is
> defined at NAND level node, the GPIO management is done at NAND driver
> level. Write protect is disabled when the driver is probed or resumed
> and is enabled when the driver is released or suspended.
> 
> When no partitions are defined in the NAND DT node, then the NAND DT node
> will be passed to NVMEM framework. If wp-gpios property is defined in
> this node, the GPIO resource is taken twice and the NAND controller
> driver fails to probe.
> 
> It would be possible to set config->wp_gpio at MTD level before calling
> nvmem_register function but NVMEM framework will toggle this GPIO on
> each write when this GPIO should only be controlled at NAND level driver
> to ensure that the Write Protect has not been enabled.
> 
> A way to fix this conflict is to add a new boolean flag in nvmem_config
> named skip_wp_gpio. In case skip_wp_gpio is set, the GPIO resource will
> be managed by the provider.
> 
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
> Changes in v2:
>   - rework the proposal done to fix a conflict between MTD and NVMEM on
>     wp-gpios property.
> 
>   drivers/nvmem/core.c           | 2 +-
>   include/linux/nvmem-provider.h | 4 +++-
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 23a38dcf0fc4..cb40dca6a51d 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -771,7 +771,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   
>   	if (config->wp_gpio)
>   		nvmem->wp_gpio = config->wp_gpio;
> -	else
> +	else if (!config->skip_wp_gpio)
>   		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
>   						    GPIOD_OUT_HIGH);
>   	if (IS_ERR(nvmem->wp_gpio)) {
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index 98efb7b5660d..4b480023c265 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -70,7 +70,8 @@ struct nvmem_keepout {
>    * @word_size:	Minimum read/write access granularity.
>    * @stride:	Minimum read/write access stride.
>    * @priv:	User context passed to read/write callbacks.
> - * @wp-gpio:   Write protect pin
> + * @wp-gpio:	Write protect pin
> + * @skip_wp_gpio: Write Protect pin is managed by the provider.

Can we rename this to ingore_wp as suggested by Miquel.

--srini

>    *
>    * Note: A default "nvmem<id>" name will be assigned to the device if
>    * no name is specified in its configuration. In such case "<id>" is
> @@ -92,6 +93,7 @@ struct nvmem_config {
>   	enum nvmem_type		type;
>   	bool			read_only;
>   	bool			root_only;
> +	bool			skip_wp_gpio;
>   	struct device_node	*of_node;
>   	bool			no_of_node;
>   	nvmem_reg_read_t	reg_read;

Re: [PATCH v2 3/4] nvmem: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Christophe Kerello]

Hi Srinivas,

On 2/17/22 12:01, Srinivas Kandagatla wrote:
> 
> 
> On 31/01/2022 09:57, Christophe Kerello wrote:
>> Wp-gpios property can be used on NVMEM nodes and the same property can
>> be also used on MTD NAND nodes. In case of the wp-gpios property is
>> defined at NAND level node, the GPIO management is done at NAND driver
>> level. Write protect is disabled when the driver is probed or resumed
>> and is enabled when the driver is released or suspended.
>>
>> When no partitions are defined in the NAND DT node, then the NAND DT node
>> will be passed to NVMEM framework. If wp-gpios property is defined in
>> this node, the GPIO resource is taken twice and the NAND controller
>> driver fails to probe.
>>
>> It would be possible to set config->wp_gpio at MTD level before calling
>> nvmem_register function but NVMEM framework will toggle this GPIO on
>> each write when this GPIO should only be controlled at NAND level driver
>> to ensure that the Write Protect has not been enabled.
>>
>> A way to fix this conflict is to add a new boolean flag in nvmem_config
>> named skip_wp_gpio. In case skip_wp_gpio is set, the GPIO resource will
>> be managed by the provider.
>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>> ---
>> Changes in v2:
>>   - rework the proposal done to fix a conflict between MTD and NVMEM on
>>     wp-gpios property.
>>
>>   drivers/nvmem/core.c           | 2 +-
>>   include/linux/nvmem-provider.h | 4 +++-
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 23a38dcf0fc4..cb40dca6a51d 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -771,7 +771,7 @@ struct nvmem_device *nvmem_register(const struct 
>> nvmem_config *config)
>>       if (config->wp_gpio)
>>           nvmem->wp_gpio = config->wp_gpio;
>> -    else
>> +    else if (!config->skip_wp_gpio)
>>           nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
>>                               GPIOD_OUT_HIGH);
>>       if (IS_ERR(nvmem->wp_gpio)) {
>> diff --git a/include/linux/nvmem-provider.h 
>> b/include/linux/nvmem-provider.h
>> index 98efb7b5660d..4b480023c265 100644
>> --- a/include/linux/nvmem-provider.h
>> +++ b/include/linux/nvmem-provider.h
>> @@ -70,7 +70,8 @@ struct nvmem_keepout {
>>    * @word_size:    Minimum read/write access granularity.
>>    * @stride:    Minimum read/write access stride.
>>    * @priv:    User context passed to read/write callbacks.
>> - * @wp-gpio:   Write protect pin
>> + * @wp-gpio:    Write protect pin
>> + * @skip_wp_gpio: Write Protect pin is managed by the provider.
> 
> Can we rename this to ingore_wp as suggested by Miquel.
> 

OK, It will be part of v3.

Regards,
Christophe Kerello.

> --srini
> 
>>    *
>>    * Note: A default "nvmem<id>" name will be assigned to the device if
>>    * no name is specified in its configuration. In such case "<id>" is
>> @@ -92,6 +93,7 @@ struct nvmem_config {
>>       enum nvmem_type        type;
>>       bool            read_only;
>>       bool            root_only;
>> +    bool            skip_wp_gpio;
>>       struct device_node    *of_node;
>>       bool            no_of_node;
>>       nvmem_reg_read_t    reg_read;

Re: [PATCH v2 4/4] mtd: core: Fix a conflict between MTD and NVMEM on wp-gpios property

[Miquel Raynal]

Hi Srinivas,

srinivas.kandagatla@linaro.org wrote on Thu, 17 Feb 2022 11:00:23 +0000:

> On 17/02/2022 08:48, Miquel Raynal wrote:
> > Hi Srinivas,
> > 
> > srinivas.kandagatla@linaro.org wrote on Wed, 16 Feb 2022 09:24:29 +0000:
> >   
> >> On 16/02/2022 08:46, Christophe Kerello wrote:  
> >>> Hi Miquel, Pratyush, Srinivas,
> >>>
> >>> On 2/2/22 12:53, Pratyush Yadav wrote:  
> >>>> + Khouloud, Linus, Bartosz
> >>>>
> >>>> On 02/02/22 11:44AM, Christophe Kerello wrote:  
> >>>>> Hi,
> >>>>>
> >>>>> On 2/1/22 11:47, Pratyush Yadav wrote:  
> >>>>>> On 31/01/22 02:43PM, Miquel Raynal wrote:  
> >>>>>>> Hi Vignesh, Tudory, Pratyush,
> >>>>>>>
> >>>>>>> + Tudor and Pratyush
> >>>>>>>
> >>>>>>> christophe.kerello@foss.st.com wrote on Mon, 31 Jan 2022 10:57:55 >>>>> +0100:  
> >>>>>>>   >>>>>>>> Wp-gpios property can be used on NVMEM nodes and the same property >>>>>> can  
> >>>>>>>> be also used on MTD NAND nodes. In case of the wp-gpios property is
> >>>>>>>> defined at NAND level node, the GPIO management is done at NAND >>>>>> driver
> >>>>>>>> level. Write protect is disabled when the driver is probed or resumed
> >>>>>>>> and is enabled when the driver is released or suspended.
> >>>>>>>>
> >>>>>>>> When no partitions are defined in the NAND DT node, then the NAND >>>>>> DT node
> >>>>>>>> will be passed to NVMEM framework. If wp-gpios property is defined in
> >>>>>>>> this node, the GPIO resource is taken twice and the NAND controller
> >>>>>>>> driver fails to probe.
> >>>>>>>>
> >>>>>>>> A new Boolean flag named skip_wp_gpio has been added in nvmem_config.
> >>>>>>>> In case skip_wp_gpio is set, it means that the GPIO is handled by the
> >>>>>>>> provider. Lets set this flag in MTD layer to avoid the conflict on
> >>>>>>>> wp_gpios property.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> >>>>>>>> ---
> >>>>>>>>     drivers/mtd/mtdcore.c | 2 ++
> >>>>>>>>     1 file changed, 2 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> >>>>>>>> index 70f492dce158..e6d251594def 100644
> >>>>>>>> --- a/drivers/mtd/mtdcore.c
> >>>>>>>> +++ b/drivers/mtd/mtdcore.c
> >>>>>>>> @@ -546,6 +546,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd)
> >>>>>>>>         config.stride = 1;
> >>>>>>>>         config.read_only = true;
> >>>>>>>>         config.root_only = true;
> >>>>>>>> +    config.skip_wp_gpio = true;
> >>>>>>>>         config.no_of_node = !of_device_is_compatible(node, >>>>>> "nvmem-cells");
> >>>>>>>>         config.priv = mtd;
> >>>>>>>> @@ -833,6 +834,7 @@ static struct nvmem_device >>>>>> *mtd_otp_nvmem_register(struct mtd_info *mtd,
> >>>>>>>>         config.owner = THIS_MODULE;
> >>>>>>>>         config.type = NVMEM_TYPE_OTP;
> >>>>>>>>         config.root_only = true;
> >>>>>>>> +    config.skip_wp_gpio = true;
> >>>>>>>>         config.reg_read = reg_read;
> >>>>>>>>         config.size = size;
> >>>>>>>>         config.of_node = np;  
> >>>>>>>
> >>>>>>> TLDR: There is a conflict between MTD and NVMEM, who should handle the
> >>>>>>> WP pin when there is one? At least for raw NAND devices, I don't want
> >>>>>>> the NVMEM core to handle the wp pin. So we've introduced this
> >>>>>>> skip_wp_gpio nvmem config option. But there are two places where this
> >>>>>>> boolean can be set and one of these is for otp regions (see above). In
> >>>>>>> this case, I don't know if it is safe or if CFI/SPI-NOR rely on the
> >>>>>>> nvmem protection. Please tell us if you think this is fine for you.  
> >>>>>>
> >>>>>> Why does NVMEM touch hardware write protection in the first place? The
> >>>>>> purpose of the framework is to provide a way to retrieve config stored
> >>>>>> in memory. It has no business dealing with details of the chip like the
> >>>>>> WP line. That should be MTD's job (which it should delegate to SPI NOR,
> >>>>>> SPI NAND, etc.). If you want to write protect a cell then do it in
> >>>>>> software. I don't see why NVMEM should be dealing with hardware >>>> directly
> >>>>>> at all.
> >>>>>>
> >>>>>> That is my mental model of how things _should_ work. I have not spent
> >>>>>> much time digging into how things actually work currently.  
> >>>>>>   >>>>>  
> >>>>> Wp-gpios property management was added in MVMEM framework in January >>> 2020 =>
> >>>>> sha1: 2a127da461a9d8d97782d6e82b227041393eb4d2
> >>>>> "
> >>>>>       nvmem: add support for the write-protect pin
> >>>>>
> >>>>>       The write-protect pin handling looks like a standard property that
> >>>>>       could benefit other users if available in the core nvmem framework.
> >>>>>
> >>>>>       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.
> >>>>> "
> >>>>>
> >>>>> And this modification was done for EEPROMs flashes => sha1:
> >>>>> 1c89074bf85068d1b86f2e0f0c2110fdd9b83c9f
> >>>>> "
> >>>>>       eeprom: at24: remove the write-protect pin support
> >>>>>
> >>>>>       NVMEM framework is an interface for the at24 EEPROMs as well as for
> >>>>>       other drivers, instead of passing the wp-gpios over the different
> >>>>>       drivers each time, it would be better to pass it over the NVMEM
> >>>>>       subsystem once and for all.
> >>>>>
> >>>>>       Removing the support for the write-protect pin after adding it to
> >>>>>       the NVMEM subsystem.
> >>>>> "
> >>>>>
> >>>>> Current NVMEM framework implementation toggles the WP GPIO when >>> reg_write
> >>>>> nvmem_config API is defined. In case of MTD framework, reg_write is not
> >>>>> defined in nvmem_config.  
> >>>>
> >>>> Thanks for digging these up. I think this was the wrong decision to
> >>>> make. NVMEM should just provide the APIs for handling read/write, and
> >>>> leave the rest to the drivers.
> >>>>
> >>>> It might be convenient for some drivers to put the WP GPIO handling to
> >>>> NVMEM core but I just don't think it is the job of the framework to deal
> >>>> with this, and it just does not know enough about the device to deal
> >>>> with correctly and completely anyway. For example, wp-gpio is only one
> >>>> of the ways to write protect a chip. SPI NOR flashes have a WP# pin that
> >>>> is often toggled via the SPI controller. There could also be registers
> >>>> that do the write protection.
> >>>>
> >>>> One would have to make strong justifications for making nvmem directly
> >>>> deal with hardware level details to convince me it is a good idea. IMHO
> >>>> if AT24 EEPROM is the only driver relying on this as of now then we
> >>>> should just revert the patches and not have to deal with the
> >>>> skip_wp_gpio hackery.  
> >>>>   >>>  
> >>> Based on the  bindings documentation, AT24 EEPROM driver is not the only
> >>> driver relying on this implementation. At least, AT25 EEPROM driver will
> >>> have to be modified to handle the Write Protect management, and there is
> >>> probably others drivers relying on this implementation.
> >>>
> >>> So, should we keep the legacy and apply the proposal patch to fix this
> >>> conflict (I can send a V3 with a fixes tag on patch 3 and 4 as
> >>> recommended by Miquel)?
> >>> Or should we revert the Write Protect management in NVMEM framework
> >>> but in this case I will not be able to handle such modifications (I am
> >>> not able to test those drivers).  
> >>
> >> Firstly sorry for such a long delay to reply this thread as I was in travel.
> >>
> >> I agree with comments from Pratyush but I see no harm in handling simple usecases of write-protect gpio in nvmem core. In fact wp-gpios and read-only are part of nvmem provider bindings.
> >>
> >> But usecases like the ones described in this patch series which do not want nvmem core to deal with this pin should set this new flag. I think this is a balanced choice.
> >>
> >> reverting the wp-gpio patch is going to break other providers that are using this bindings.  
> > 
> > I am always puzzled when the community deeply cares about non-mainline
> > drivers.
> > 
> > On one side I can understand that creating a 'grab-the-wp-line'
> > flag would immediately break all external users but this is, as
> > reported by Pratyush, the more logical approach IMHO. However we might
> > possibly miss situations where the flag is necessary and break these
> > devices.
> > 
> > Otherwise the 'ignore-wp' flag is more conservative, it does not  
> 
> ingore-wp seems to be more sensible flag than skip_wp_gpio flag.
> 
> 
> > break the existing users but would just address the MTD case for now, we
> > might have other in-tree subsystem that are affected.
> > 
> > I'm fine either way TBH :-) I would just like this patchset to go in  
> 
> Am okay either way too, It is just that ingore-wp seems a balanced choice in the current situation :-).
> 
> > through the next merge window.  
> Sure.
> 
> I can Ack nvmem patch if you wish to carry it via mtd tree.

Yes, that would be ideal to prevent build issues during the merge
window/in linux-next.

> 
> or
> 
> nvmem patches go via Greg's char-misc tree. I can take 4/4 if you ack it once V3 is sent

Thanks,
Miquèl