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

[Tudor.Ambarus]

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@xxxxxxxxxxxxx:
>>>>> 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