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

[Kamal Dasu <kamal.dasu@xxxxxxxxxxxx>]

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.


> [...]
>
> --
> 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