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

[Pratyush Yadav <p.yadav@ti.com>]

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.