Re: [PATCH v4 1/2] mtd: spi-nor: Added spi-nor init function

[Marek Vasut <marex@xxxxxxx>]

On 02/19/2017 11:38 PM, Kamal Dasu wrote:
> Marek,
> 
> On Sun, Feb 19, 2017 at 5:37 AM, Marek Vasut <marex@xxxxxxx> wrote:
>> On 02/19/2017 10:47 AM, Kamal Dasu wrote:
>>> On Sat, Feb 18, 2017 at 5:29 PM, Marek Vasut <marex@xxxxxxx> wrote:
>>>> On 02/14/2017 04:32 PM, Kamal Dasu wrote:
>>>>> Refactored spi_nor_scan() code to add spi_nor_init() function that
>>>>> programs the spi-nor flash 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
>>>>>
>>>>> spi_nor_scan() now calls spi_nor_init() to setup nor flash.
>>>>>
>>>>> Signed-off-by: Kamal Dasu <kdasu.kdev@xxxxxxxxx>
>>>>
>>>> Changelog missing ?
>>>
>>> Yes will add it and resend.
>>>
>>>>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/spi-nor.c | 72 +++++++++++++++++++++++++++----------------
>>>>>  include/linux/mtd/spi-nor.h   | 18 ++++++++++-
>>>>>  2 files changed, 62 insertions(+), 28 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>>> index 70e52ff..2bf7f4f 100644
>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>>> @@ -1526,6 +1526,44 @@ static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor)
>>>>>       return 0;
>>>>>  }
>>>>>
>>>>> +int spi_nor_init(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;
>>>>> +             }
>>>>> +     }
>>>
>>> quad mode is being set in the above block of code.
>>>
>>>>> +
>>>>> +     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);
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(spi_nor_init);
>>>>> +
>>>>>  int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>>>  {
>>>>>       const struct flash_info *info = NULL;
>>>>> @@ -1571,6 +1609,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>>>               }
>>>>>       }
>>>>>
>>>>> +     nor->info = info;
>>>>>       mutex_init(&nor->lock);
>>>>>
>>>>>       /*
>>>>> @@ -1581,20 +1620,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>>>       if (info->flags & SPI_S3AN)
>>>>>               nor->flags |=  SNOR_F_READY_XSR_RDY;
>>>>>
>>>>> -     /*
>>>>> -      * 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;
>>>>> @@ -1669,11 +1694,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;
>>>>
>>>> So from here on, any user will expect quad mode to be correctly set, but
>>>> it really isn't. Is that OK ?
>>>
>>> It is being set in spi_nor_init() based on the  nor->flash_read == SPI_NOR_QUAD;
>>>
>>
>> But that's invoked later, so whoever adds code between this place and
>> the invocation of spi_nor_init() will be misled into believing the SPI
>> NOR is set up in quad-mode, while it would not be, right ?
>>
> 
> I made this change based on  previous comments from Cyrlle. All
> spi-nor based settings that need commands to the flash happens in one
> function now. The code before calling spi_nor_init is setting the
> configuration in the nor structure. I don't see how its very different
> from before. I did separate each setting in functions without changing
> the flow in spi_nor_scan()  before and was to told to change it this
> way to be more optimal. I don't see how anyone will have any
> confusion. So please let me know how exactly you want it.

Hmmmm, I wonder if nor->flash_read = SPI_NOR_QUAD; shouldn't be set in
set_quad_mode() ?

-- 
Best regards,
Marek Vasut
--
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