[PATCH v2 09/35] mtd: spi-nor: atmel: Use flash late_init() for locking

[Tudor.Ambarus at microchip.com]

On 9/10/21 12:44 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-08-16 21:06, schrieb Pratyush Yadav:
>> On 27/07/21 07:51AM, Tudor Ambarus wrote:
>>> Locking is not described in JESD216 SFDP standard, place the locking
>>> init in late_init().
> 
> Btw, we should differentiate between the block protection
> bits and individual block locking. At least the latter is described
> in the SFDP (I've seen it in the XTX SFDP, haven't checked the
> standard yet).

that's probably a vendor specific table, not something standardized by SFDP.

> 
>> You are chaning the order of setting the locking ops here. Earlier,
>> they
>> were set before we parsed SFDP. Now they are set after we parse SFDP.
>> Though I don't see it making much of a difference.

Right, as the locking is not covered by SFDP, we should place it after
parsing SFDP.

>>
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
>>> ---
>>>  drivers/mtd/spi-nor/atmel.c | 30 +++++++++++-------------------
>>>  1 file changed, 11 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/atmel.c b/drivers/mtd/spi-nor/atmel.c
>>> index 1fea5cab492c..b937ef734e55 100644
>>> --- a/drivers/mtd/spi-nor/atmel.c
>>> +++ b/drivers/mtd/spi-nor/atmel.c
>>> @@ -48,15 +48,11 @@ static const struct spi_nor_locking_ops
>>> atmel_at25fs_locking_ops = {
>>>      .is_locked = atmel_at25fs_is_locked,
>>>  };
>>>
>>> -static void atmel_at25fs_default_init(struct spi_nor *nor)
>>> +static void atmel_at25fs_late_init(struct spi_nor *nor)
>>>  {
>>>      nor->params->locking_ops = &atmel_at25fs_locking_ops;
>>>  }
>>>
>>> -static const struct spi_nor_fixups atmel_at25fs_fixups = {
>>> -    .default_init = atmel_at25fs_default_init,
>>> -};
>>> -
>>>  /**
>>>   * atmel_set_global_protection - Do a Global Protect or Unprotect
>>> command
>>>   * @nor:    pointer to 'struct spi_nor'
>>> @@ -146,34 +142,30 @@ static const struct spi_nor_locking_ops
>>> atmel_global_protection_ops = {
>>>      .is_locked = atmel_is_global_protected,
>>>  };
>>>
>>> -static void atmel_global_protection_default_init(struct spi_nor *nor)
>>> +static void atmel_global_protection_late_init(struct spi_nor *nor)
>>>  {
>>>      nor->params->locking_ops = &atmel_global_protection_ops;
>>>  }
>>>
>>> -static const struct spi_nor_fixups atmel_global_protection_fixups = {
>>> -    .default_init = atmel_global_protection_default_init,
>>> -};
>>> -
>>>  static const struct flash_info atmel_parts[] = {
>>>      /* Atmel -- some are (confusingly) marketed as "DataFlash" */
>>>      { "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K |
>>> SPI_NOR_HAS_LOCK)
>>> -            .fixups = &atmel_at25fs_fixups },
>>> +            .late_init = atmel_at25fs_late_init },
>>>      { "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K |
>>> SPI_NOR_HAS_LOCK)
>>> -            .fixups = &atmel_at25fs_fixups },
>>> +            .late_init = atmel_at25fs_late_init },
>>>
>>>      { "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8,
>>>                           SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
>>> -                    .fixups = &atmel_global_protection_fixups },
>>> +            .late_init = atmel_global_protection_late_init },
>>
>> Won't you be better off setting this in the manufacturer late_init()?
>> It
>> seems common for most atmel flashes.
>>
>> Of course, this would cause a problem for atmel flashes that don't have
>> this at all, since we would set locking for those as well. But I think
>> we can avoid that by checking for SNOR_F_HAS_LOCK in
>> spi_nor_register_locking_ops().
> 
> +1
> 

we also have the atmel_at25fs_late_init() method. setting it per manufacturer will result
in setting the manufacturer locking ops for at25fs as well, which will be overwritten by the
at25fs locking ops. For those that don't support locking at all, we should clear the locking
ops as you said. This will make the code a little difficult to follow and we return a bit
to spaghetti. defining late_init() takes only a line anyway. I would keep the code as it is
if you don't mind. We can ask ourselves about scalability when we have lots of entries,
we can reevaluate this in the future. Tell me if you have strong opinions on this.

Cheers,
ta