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

[Miquel Raynal]

Hi Christophe,

christophe.kerello at 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 at 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 at 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