Messages in this thread | | | From | <> | Subject | Re: [PATCH v3] mtd: spi-nor: keep lock bits if they are non-volatile | Date | Thu, 1 Oct 2020 11:46:14 +0000 |
| |
On 10/1/20 10:38 AM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Tudor,
Hi, Michael,
> > Am 2020-10-01 09:07, schrieb Tudor.Ambarus@microchip.com: >>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >>>>> index cc68ea84318e..fd1c36d70a13 100644 >>>>> --- a/drivers/mtd/spi-nor/core.c >>>>> +++ b/drivers/mtd/spi-nor/core.c >>>>> @@ -2916,20 +2916,38 @@ static int spi_nor_quad_enable(struct >>>>> spi_nor >>>>> *nor) >>>>> } >>>>> >>>>> /** >>>>> - * spi_nor_unlock_all() - Unlocks the entire flash memory array. >>>>> + * spi_nor_global_unprotect() - Perform a global unprotect of the >>>>> memory area. >>>>> * @nor: pointer to a 'struct spi_nor'. >>>>> * >>>>> * Some SPI NOR flashes are write protected by default after a >>>>> power-on reset >>>>> * cycle, in order to avoid inadvertent writes during power-up. >>>>> Backward >>>>> * compatibility imposes to unlock the entire flash memory array at >>>>> power-up >>>>> - * by default. >>>>> + * by default. Do it only for flashes where the block protection >>>>> bits >>>>> + * are volatile, this is indicated by SNOR_F_NEED_UNPROTECT. >>>>> + * >>>>> + * We cannot use spi_nor_unlock(nor->params.size) here because >>>>> there >>>>> are >>>>> + * legacy devices (eg. AT25DF041A) which need a "global unprotect" >>>>> command. >>>>> + * This is done by writing 0b0x0000xx to the status register. This >>>>> will also >>>>> + * work for all other flashes which have these bits mapped to BP0 >>>>> to >>>>> BP3. >>>>> + * The top most bit is ususally some kind of lock bit for the block >>>>> + * protection bits. >>>>> */ >>>>> -static int spi_nor_unlock_all(struct spi_nor *nor) >>>>> +static int spi_nor_global_unprotect(struct spi_nor *nor) >>>>> { >>>>> - if (nor->flags & SNOR_F_HAS_LOCK) >>>>> - return spi_nor_unlock(&nor->mtd, 0, nor->params->size); >>>>> + int ret; >>>>> >>>>> - return 0; >>>>> + dev_dbg(nor->dev, "unprotecting entire flash\n"); >>>>> + ret = spi_nor_read_sr(nor, nor->bouncebuf); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + nor->bouncebuf[0] &= ~SR_GLOBAL_UNPROTECT_MASK; >>>>> + >>>>> + /* >>>>> + * Don't use spi_nor_write_sr1_and_check() because writing the >>>>> status >>>>> + * register might fail if the flash is hardware write >>>>> protected. >>>>> + */ >>>>> + return spi_nor_write_sr(nor, nor->bouncebuf, 1); >>>>> } >>>> >>>> This won't work for all the flashes. You use a GENMASK(5, 2) to clear >>>> the Status Register even for BP0-2 flashes and you end up clearing >>>> BIT(5) >>>> which can lead to side effects. >>>> >>>> We should instead introduce a >>>> nor->params->locking_ops->global_unlock() >>>> hook >>>> for the flashes that have special opcodes that unlock all the flash >>>> blocks, >>>> or for the flashes that deviate from the "clear just your BP bits" >>>> rule. >>> >>> Wouldn't it make more sense to just set params->locking_ops for these >>> flashes >>> to different functions? or even provide a spi_nor_global_unprotect_ops >>> in >>> core.c and these flashes will just set them. there is no individual >>> sector >>> range lock for these chips. just a lock all or nothing. >> >> I like the idea of having all locking related functions placed in a >> single >> place, thus the global_unlock() should be inside locking_ops struct. > > My point was that this global unlock shouldn't be a special case for the > current spi_nor_unlock() but just another "how to unlock the flash" > function > and thus should replace the original unlock op. For example, it is also > likely > that you need a special global lock (i.e. write all 1's). > > static int spi_nor_global_unlock() > { > write_sr(0); /* actually it will be a read-modify write */ > } > > static int spi_nor_global_lock() > { > write_sr(0x1c); > } > > static int spi_nor_is_global_locked() > { > return read_sr() & 0x1c; > } > > const struct spi_nor_locking_ops spi_nor_sr_locking_ops = { > .lock = spi_nor_global_unlock, > .unlock = spi_nor_global_lock, > .is_locked = spi_nor_is_global_locked, > };
Meh, this would be valid only if the flash supports _just_ global (un)lock, without supporting locking on a smaller granularity. Otherwise, people will get lazy and just add support for global (unlock) without introducing software for smaller granularity locking, which would be a pity.
If there's such a case, those functions should be manufacturer/flash specific.
> > Having the spi_nor_unlock decide what op to choose introduces just > another indirection. Esp. if you think about having support for > individual sector protection which also needs new ops. Btw. to me > it seems that "global (un)lock" is almost always used for the > individual sector protection scheme, i.e. like a shortcut to allow all > sectors be unlocked at once. >
Probably yes, the global unlock command is tied to individual block locking, will have to check. And yes, a global unlock command should offer a single command cycle that unlocks the entire memory array. It should be preferred when one wants to unlock the entire flash.
Cheers, ta
| |