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

[Pratyush Yadav <p.yadav@ti.com>]

On 01/10/21 11:40AM, Tudor.Ambarus@microchip.com wrote:
> 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@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.

Okay, this should be fine I think. Looking at the Micron output driver 
strength patch has make me rethink manufacturer-wide settings in 
general. If we add manufacturer-wide settings, and then the manufacturer 
later change their minds and start using different settings for their 
newer flashes, we could easily run into a situation where half the 
flashes are just overwriting the manufacturer-wide default.

> 
> Cheers,
> ta
> 
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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