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

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@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 93 +++++++++++++++++++++++++++++++++++++------
 include/linux/mtd/spi-nor.h   | 17 ++++++++
 2 files changed, 98 insertions(+), 12 deletions(-)

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@gmail.com>
> ---
>  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!

> +
> +	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.

> +
> +	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() ?

> +
> +	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.

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

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.


Best regards,

Cyrille

>  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
>

On Mon, Feb 6, 2017 at 6:46 AM, Cyrille Pitchen
<cyrille.pitchen@atmel.com> 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@gmail.com>
>> ---
>>  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
>>
>