Re: [PATCH v3] mtd: spi-nor: keep lock bits if they are non-volatile

[Michael Walle <michael@walle.cc>]

Hi Tudor,

Am 2020-10-01 13:46, schrieb Tudor.Ambarus@microchip.com:
>> 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.

Most (all?) of these flashes will support the per-sector protection, 
which
we doesn't support. So yes, this is currently a shortcut for the flashes
which were unlocked by default in the past, but doesn't fall into the BP
bits category.

It doesn't feel right to add this as a special case to the current
spi_nor_lock(), which is really a spi_nor_bp_lock(). And this locking
has nothing to do with BP bits. If there would be support for per-sector
protection, then this would end up there. But there isn't.

> Otherwise, people will
> get lazy and just add support for global (unlock) without introducing
> software for smaller granularity locking, which would be a pity.

I can't relate to this. If people need per-sector locking, there will
eventually be code. If people just need a protect all or nothing, well
then.

> If there's such a case, those functions should be manufacturer/flash 
> specific.

Lucky me, this is (at the moment) just Atmel. So is it acceptable,
if I'll put this global unlock ops into atmel.c? I mean eventually, this
will be replaced by proper per-sector locking. It is just there to 
support
the flashes which was already unlocked that way in the past. In terms
of backward compatibility there won't be more flashes which need this
global unlock.

But then this relates to the question how I should name the menuconfig
choices. Because these flashes doesn't have the block protection bits.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/