Hi Srinivas, srinivas.kandagatla@xxxxxxxxxx wrote on Thu, 17 Feb 2022 11:00:23 +0000: > On 17/02/2022 08:48, Miquel Raynal wrote: > > Hi Srinivas, > > > > srinivas.kandagatla@xxxxxxxxxx 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@xxxxxxxxxxx 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@xxxxxxxxxxx> > >>>>>>>> --- > >>>>>>>> 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
