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

[Michael Walle <michael@walle.cc>]

Am 2021-10-01 13:40, schrieb Tudor.Ambarus@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.

correct.

>>> 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@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.

What about the following:

int atmel_late_init(struct spi_nor *nor)
{
     if (nor->info->flags & SPI_NOR_HAS_LOCK)
         nor->params->locking_ops = &atmel_global_protection_ops;
}

Of course it depends on wether we expect these ops to be the ones
used for most atmel flashes.

The at25fs would then overwrite the ops in their .late_init.

In any case, I don't have a strong opinion either way.

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel