Re: [PATCH v2 1/2] mtd: spi-nor: Added spi-nor reset function

[Kamal Dasu <kamal.dasu@xxxxxxxxxxxx>]

On Thu, Feb 9, 2017 at 12:49 PM, Cyrille Pitchen
<cyrille.pitchen@xxxxxxxxx> wrote:
> Hi Kamal,
>
> Le 08/02/2017 à 17:53, Kamal Dasu a écrit :
>> Refactored spi_nor_scan() code to add spi_nor_reset() function that
>> programs the nand device to:
>> 1) remove flash protection if applicable
>> 2) set read mode to quad mode if configured such
>> 3) set the address width based on the flash size and vendor
>>
>> On pm resume spi-nor flash may need to be reconfigured after power
>> reset, there is no need to go through a full spi_nor_scan(), flash
>> device driver needs to call spi_nor_reset() to reprogram the flash
>> to its probed state.
>>
>> Signed-off-by: Kamal Dasu <kdasu.kdev@xxxxxxxxx>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 62 ++++++++++++++++++++++++++++---------------
>>  include/linux/mtd/spi-nor.h   | 21 ++++++++++++++-
>>  2 files changed, 61 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index da7cd69..8e3d8bd 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1312,6 +1312,41 @@ static int spi_nor_check(struct spi_nor *nor)
>>       return 0;
>>  }
>>
>> +int spi_nor_reset(struct spi_nor *nor)
>> +{
>> +     int ret = 0;
>> +     const struct flash_info *info = nor->info;
>> +     struct device *dev = nor->dev;
>> +
>> +     /*
>> +      * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>> +      * with the software protection bits set
>> +      */
>> +
>> +     if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>> +         JEDEC_MFR(info) == SNOR_MFR_INTEL ||
>> +         JEDEC_MFR(info) == SNOR_MFR_SST ||
>> +         info->flags & SPI_NOR_HAS_LOCK) {
>> +             write_enable(nor);
>> +             write_sr(nor, 0);
>> +             spi_nor_wait_till_ready(nor);
>> +     }
>> +
>> +     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) != SNOR_MFR_SPANSION)
>> +             set_4byte(nor, info, 1);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(spi_nor_reset);
>> +
>>  int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>  {
>>       const struct flash_info *info = NULL;
>> @@ -1357,22 +1392,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>               }
>>       }
>>
>> +     nor->info = info;
>>       mutex_init(&nor->lock);
>>
>> -     /*
>> -      * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>> -      * with the software protection bits set
>> -      */
>> -
>> -     if (JEDEC_MFR(info) == SNOR_MFR_ATMEL ||
>> -         JEDEC_MFR(info) == SNOR_MFR_INTEL ||
>> -         JEDEC_MFR(info) == SNOR_MFR_SST ||
>> -         info->flags & SPI_NOR_HAS_LOCK) {
>> -             write_enable(nor);
>> -             write_sr(nor, 0);
>> -             spi_nor_wait_till_ready(nor);
>> -     }
>> -
>>       if (!mtd->name)
>>               mtd->name = dev_name(dev);
>>       mtd->priv = nor;
>> @@ -1447,11 +1469,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>
>>       /* Quad/Dual-read mode takes precedence over fast/normal */
>>       if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
>> -             ret = set_quad_mode(nor, info);
>> -             if (ret) {
>> -                     dev_err(dev, "quad mode not supported\n");
>> -                     return ret;
>> -             }
>>               nor->flash_read = SPI_NOR_QUAD;
>>       } else if (mode == SPI_NOR_DUAL && info->flags & SPI_NOR_DUAL_READ) {
>>               nor->flash_read = SPI_NOR_DUAL;
>> @@ -1503,8 +1520,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>                       /* No small sector erase for 4-byte command set */
>>                       nor->erase_opcode = SPINOR_OP_SE_4B;
>>                       mtd->erasesize = info->sector_size;
>
> It seems that you didn't base your patch on the spi-nor tree. Before your
> patch, you should have something like:

I rebased to the linus tree. I will rebase to the spi-nor tree for the
next version.

>
>         else if (mtd->size > 0x1000000) {
>                 /* enable 4-byte addressing if the device exceeds 16MiB */
>                 nor->addr_width = 4;
>                 if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>                     info->flags & SPI_NOR_4B_OPCODES)
>                         spi_nor_set_4byte_opcodes(nor, info);
>                 else
>                         set_4byte(nor, info, 1);
>         } else {
>                 nor->addr_width = 3;
>         }
>
> That's why in my comment of review 1, I was talking about handling the
> case where spi_nor_set_4byte_opcodes() is called instead of set_4byte():
> indeed when the manufacturer id is Spansion or when the SPI_NOR_4B_OPCODES
> flag is set.
>

Ok  I need to use the spi-nor tree.

> Hence in spi_nor_reset(), or whatever name you choose, set_4byte() must not
> be called if the SPI_NOR_4B_OPCODES flag is set in info->flags.
>
> Besides, for now I don't know that much about how power management is
> handled by the Linux kernel, especially which events trigger the call of
> .suspend() or .resume().
> I've started to read documentation about that topic so please be patient
> because I need some time to figure out the potential side effects this
> series may introduce.
> I know you test your patches before submitting them, so this is not a
> question of trust but I'd rather understand what patches actually do
> instead of blindly merge them! :)
>

I will make sure I use the right codebase.

> Also maybe this patch can help you:
> http://patchwork.ozlabs.org/patch/719777/
>

I do see that the m25p80 and spi-nor drivers are implementing and
distinguishing spi protocol and cmds based on the address width and
transfer modes. And is spi-nor refactoring the same parts of the code
which I have touched. so will follow up in this change.

> It's not related to power management but it also reworks spi_nor_scan()
> so all settings (op codes, protocols, number of dummy cycles, ...) are
> chosen once for all by spi_nor_init_params() then the actual memory device
> is configured by spi_nor_setup().
>
> I can rework spi_nor_setup() a little bit as the first part of its code is
> needed to be done only once from spi_nor_scan() whereas the final part
> almost matches what's your "spi_nor_reset()" function will do in term.
>
> We should regroup {unlock sectors, enable_quad_io() and set_4byte() (if not
> set_nor_set_4byte_opcodes)} codes. That is to say, the minimum set of SPI
> commands we have to send to the memory at each power on.
>

Yes that sounds right. Regarding the pm  do we want to use the m25p80
resume, or let the spi master somehow invoke the setup from its
resume. If there was a way to do that cleanly within the current
framework of m25p, spi-nor, spi master it might leave that decision to
the spi controller driver. Also in our case we are a spi master
controller driver  mtd and spi-nor m25p80 structures and its ops are
transparent.

> Best regards,
>
> Cyrille
>
>
>> -             } else
>> -                     set_4byte(nor, info, 1);
>> +             }
>>       } else {
>>               nor->addr_width = 3;
>>       }
>> @@ -1517,6 +1533,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>
>>       nor->read_dummy = spi_nor_read_dummy_cycles(nor);
>>
>> +     ret = spi_nor_reset(nor);
>> +     if (ret)
>> +             return ret;
>> +
>>       dev_info(dev, "%s (%lld Kbytes)\n", info->name,
>>                       (long long)mtd->size >> 10);
>>
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index c425c7b..4733c04 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -121,6 +121,8 @@ enum spi_nor_option_flags {
>>       SNOR_F_HAS_SR_TB        = BIT(1),
>>  };
>>
>> +struct flash_info;
>> +
>>  /**
>>   * struct spi_nor - Structure for defining a the SPI NOR layer
>>   * @mtd:             point to a mtd_info structure
>> @@ -154,6 +156,7 @@ enum spi_nor_option_flags {
>>   * @priv:            the private data
>>   */
>>  struct spi_nor {
>> +     const struct flash_info *info;
>>       struct mtd_info         mtd;
>>       struct mutex            lock;
>>       struct device           *dev;
>> @@ -198,12 +201,28 @@ static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor)
>>  }
>>
>>  /**
>> + * spi_nor_reset() - reset scan the SPI NOR
>> + * @nor:     the spi_nor structure
>> + *
>> + * The drivers uses this function to reset 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_reset(struct spi_nor *nor);
>> +
>> +/**
>>   * spi_nor_scan() - scan the SPI NOR
>>   * @nor:     the spi_nor structure
>>   * @name:    the chip type name
>>   * @mode:    the read mode supported by the driver
>>   *
>> - * The drivers can use this fuction to scan the SPI NOR.
>> + * The drivers can use this function to scan the SPI NOR.
>>   * In the scanning, it will try to get all the necessary information to
>>   * fill the mtd_info{} and the spi_nor{}.
>>   *
>>
>
--
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