Re: [PATCH v1 1/5] mtd: spi-nor: Added way to rescan spi-nor device

[Kamal Dasu <kdasu.kdev@xxxxxxxxx>]

On Mon, Feb 6, 2017 at 6:46 AM, Cyrille Pitchen
<cyrille.pitchen@xxxxxxxxx> wrote:
> Hi Kamal,
>
> Le 04/02/2017 à 00:31, Kamal Dasu a écrit :
>> On pm resume op spi-nor flash may need to be reconfigured on a power
>> reset, however there is no need to go through a full spi_nor_scan().
>> The driver might need to disable protection, program the address width and
>> transfer mode where applicable. The spi-nor framework has all the generic
>> code to do this. Refactored a few pieces and added a spi_nor_pm_rescan()
>> to be called by the mtd device driver's pm resume() op.
>>
>> Signed-off-by: Kamal Dasu <kdasu.kdev@xxxxxxxxx>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 93 +++++++++++++++++++++++++++++++++++++------
>>  include/linux/mtd/spi-nor.h   | 17 ++++++++
>>  2 files changed, 98 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index da7cd69..e72233b 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1312,26 +1312,28 @@ static int spi_nor_check(struct spi_nor *nor)
>>       return 0;
>>  }
>>
>> -int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>> +static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
>> +                                                    const char *name,
>> +                                                    int *retval)
>> +
>>  {
>>       const struct flash_info *info = NULL;
>>       struct device *dev = nor->dev;
>> -     struct mtd_info *mtd = &nor->mtd;
>> -     struct device_node *np = spi_nor_get_flash_node(nor);
>> -     int ret;
>> -     int i;
>> +     int ret = 0;
>>
>>       ret = spi_nor_check(nor);
>>       if (ret)
>> -             return ret;
>> +             goto info_out;
>>
>>       if (name)
>>               info = spi_nor_match_id(name);
>>       /* Try to auto-detect if chip name wasn't specified or not found */
>>       if (!info)
>>               info = spi_nor_read_id(nor);
>> -     if (IS_ERR_OR_NULL(info))
>> -             return -ENOENT;
>> +     if (IS_ERR_OR_NULL(info)) {
>> +             ret = -ENOENT;
>> +             goto info_out;
>> +     }
>>
>>       /*
>>        * If caller has specified name of flash model that can normally be
>> @@ -1342,7 +1344,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>
>>               jinfo = spi_nor_read_id(nor);
>>               if (IS_ERR(jinfo)) {
>> -                     return PTR_ERR(jinfo);
>> +                     ret = PTR_ERR(jinfo);
>> +                     goto info_out;
>>               } else if (jinfo != info) {
>>                       /*
>>                        * JEDEC knows better, so overwrite platform ID. We
>> @@ -1357,8 +1360,15 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>               }
>>       }
>>
>> -     mutex_init(&nor->lock);
>> +info_out:
>> +
>> +     *retval = ret;
>> +     return info;
>> +}
>>
>> +static void spi_nor_unprotect(struct spi_nor *nor,
>> +                           const struct flash_info *info)
>> +{
>>       /*
>>        * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>>        * with the software protection bits set
>> @@ -1372,6 +1382,35 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>               write_sr(nor, 0);
>>               spi_nor_wait_till_ready(nor);
>>       }
>> +}
>> +
>> +static inline void spi_nor_print_flash_info(struct spi_nor *nor,
>> +                                         const struct flash_info *info)
>> +{
>> +     struct mtd_info *mtd = &nor->mtd;
>> +     struct device *dev = nor->dev;
>> +
>> +     dev_info(dev, "%s (%lld Kbytes)\n", info->name,
>> +                     (long long)mtd->size >> 10);
>> +
>> +}
>> +
>> +int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>> +{
>> +     const struct flash_info *info;
>> +     struct device *dev = nor->dev;
>> +     struct mtd_info *mtd = &nor->mtd;
>> +     struct device_node *np = spi_nor_get_flash_node(nor);
>> +     int ret;
>> +     int i;
>> +
>> +     info = spi_nor_get_flash_info(nor, name, &ret);
>> +     if (ret)
>> +             return ret;
>> +
>> +     mutex_init(&nor->lock);
>> +
>> +     spi_nor_unprotect(nor, info);
>>
>>       if (!mtd->name)
>>               mtd->name = dev_name(dev);
>> @@ -1517,8 +1556,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>
>>       nor->read_dummy = spi_nor_read_dummy_cycles(nor);
>>
>> -     dev_info(dev, "%s (%lld Kbytes)\n", info->name,
>> -                     (long long)mtd->size >> 10);
>> +     spi_nor_print_flash_info(nor, info);
>>
>>       dev_dbg(dev,
>>               "mtd .name = %s, .size = 0x%llx (%lldMiB), "
>> @@ -1540,6 +1578,37 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>  }
>>  EXPORT_SYMBOL_GPL(spi_nor_scan);
>>
>> +int spi_nor_pm_rescan(struct spi_nor *nor, const char *name)
>> +{
>> +     int ret = 0;
>> +     const struct flash_info *info;
>> +     struct device *dev = nor->dev;
>> +
>> +     info = spi_nor_get_flash_info(nor, name, &ret);
>> +     if (ret)
>> +             return ret;
>
> Why don't we save the info pointer in the spi_nor structure from
> spi_nor_scan()?
>
> in include/linux/mtd/spi-nor.h
>
> +struct flash_info;
>
>  struct spi_nor {
> +       const struct flash_info *info;
>         struct mtd_info         mtd;
>         struct mutex            lock;
>
> I would avoid scanning the SPI flash again and again just to always
> retrieve the very same settings.
>
> Besides, when the parsing of the SFDP tables will be added, the scan
> procedure will cost more than a single Read JEDEC ID command.
>
> Hence if we can skip this step I guess it would be better!
>

That's a great suggestion. I will make the necessary changes.

>> +
>> +     spi_nor_unprotect(nor, info);
>> +
>> +     if (nor->flash_read == SPI_NOR_QUAD) {
>> +             ret = set_quad_mode(nor, info);
>> +             if (ret) {
>> +                     dev_err(dev, "quad mode not supported\n");
>> +                     return ret;
>> +             }
>> +     }
>> +
>> +     if (nor->addr_width == 4 && JEDEC_MFR(info) != CFI_MFR_AMD)
>> +             set_4byte(nor, info, 1);
>
> You have forgotten the case where spi_nor_set_4byte_opcodes() is to be
> called instead of set_4byte(), that is when we want to use the stateless
> 4-byte address instruction set instead of the stateful 4-byte address mode.
>

Actually instruction set applies to SNOR_MFR_SPANSION only and is set
in the spi-nor structure. Is there a need to implement and call
spi_nor_set_4byte_opcodes().

>> +
>> +     spi_nor_print_flash_info(nor, info);
>
> IMHO, there is no reason to print this message a second time. It has
> already been displayed from spi_nor_scan() then I guess it's enough.
> Nothing has changed since spi_nor_scan().
>
>> +     dev_dbg(dev, "addr width %d read mode %d",
>> +             nor->addr_width, nor->flash_read);
>
> Why should we display those settings here in spi_nor_pm_rescan() but not in
> spi_nor_scan() ?
>

Will fix this in v2 version as well.

>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_nor_pm_rescan);
>> +
>
> Actually most of the spi_nor_pm_rescan() code can be removed. We only need
> to keep:
> - unlock the flash
> - check the Quad Enable requirement
> - for memory > 128 Mbit, either make it enter its 4-byte address mode or
> use the 4-byte address instruction set.
>

Yes I agree the above is all that is needed on reset.

> Moreover, for the ease of the maintenance, I think something like
>
>
> int spi_nor_reset(...)
> {
>         /* unlock the flash */
>         ...
>
>         /* check the Quad Enable requirement */
>         ...
>
>         /* handle memory > 128 Mbit */
>         ...
> }
> EXPORT_SYMBOL_GPL(spi_nor_reset);
>
> int spi_nor_scan(...)
> {
>         /* scan */
>         ...
>
>         /* reset */
>         spi_nor_reset();
>
>         ...
> }
> EXPORT_SYMBOL_GPL(spi_nor_scan);
>
> could be better than
>

I can get rid of unnecessary refactoring and implement a single

> int func1()
> {
>
> }
>
> int func2()
> {
>
> }
>
> int func3()
> {
>
> }
>
> int func4()
> {
>
> }
>
> int spi_nor_scan(...)
> {
>         func1();
>         func2();
>         func3();
>         func4();
>         ...
> }
> EXPORT_SYMBOL_GPL(spi_nor_scan);
>
> int spi_nor_pm_rescan(...)
> {
>         func2();
>         func3();
> }
> EXPORT_SYMBOL_GPL(spi_nor_pm_rescan);
>
> because in the 2nd case it's more difficult to figure out whether some
> piece of code should be called from either spi_nor_scan() or
> spi_nor_pm_rescan() only or from both.
>
> For instance, you have forgotten to call spi_nor_set_4byte_opcodes() when
> needed from spi_nor_pm_rescan().
>
> The reset function should only be a subset of spi_nor_scan().
>
> IMHO, the spi_nor_pm_rescan() approach is a little bit more confusing.
>
>

I will make the necessary changes. Thanks for the detailed review and
explanation.


> Best regards,
>
> Cyrille
>

Thanks
Kamal

>>  static const struct flash_info *spi_nor_match_id(const char *name)
>>  {
>>       const struct flash_info *id = spi_nor_ids;
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index c425c7b..487b473 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -213,4 +213,21 @@ static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor)
>>   */
>>  int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode);
>>
>> +/**
>> + * spi_nor_pm_rescan() - rescan the SPI NOR
>> + * @nor:     the spi_nor structure
>> + * @name:    the chip type name
>> + *
>> + * The drivers can use this function to set the SPI NOR flash device to
>> + * its initial scanned state, it shall use all nor information set on poweron
>> + * for the read mode, address width and enabling write mode for certain
>> + * manufacturers. This would be needed to be called for flash devices that are
>> + * reset during power management.
>> + *
>> + * The chip type name can be provided through the @name parameter.
>> + *
>> + * Return: 0 for success, others for failure.
>> + */
>> +int spi_nor_pm_rescan(struct spi_nor *nor, const char *name);
>> +
>>  #endif
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html