[PATCH v1 0/5] Added support for spi-nor device pm in m25p80

[PATCH v1 0/5] Added support for spi-nor device pm in m25p80

[Kamal Dasu]

The changes below implements power management support in m25p80.
m25p80 pm resume() calls newly a implemented spi_nor_pm_rescan()
function that sets up the spi-nor flash to its probed state. 
This is needed on platfroms that turn off power to the spi-nor
part on suspend, in such cases some parts might need to: 
1. remove flash protection (if applicable)
2. set the address width and/or
3. set the read mode to quad mode depending on the probed configuration

The changes use existing spi-nor framework by refactoring code a bit so
that it can be reused in scan and rescan functions. Changes also ensures
that for platforms that do not need pm support shall not be affected. 

Kamal Dasu (5):
  mtd: spi-nor: Added way to rescan spi-nor device
  mtd: m25p80: Added pm ops support
  spi: Added way to check for pm support for flash devices
  mtd: m25p80: Check if the spi flash device has pm support
  spi: bcm-qspi: Implement the master flash_pm_supported() call

 drivers/mtd/devices/m25p80.c  | 21 ++++++++++
 drivers/mtd/spi-nor/spi-nor.c | 93 +++++++++++++++++++++++++++++++++++++------
 drivers/spi/spi-bcm-qspi.c    |  6 +++
 include/linux/mtd/spi-nor.h   | 17 ++++++++
 include/linux/spi/spi.h       |  8 ++++
 5 files changed, 133 insertions(+), 12 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 0/5] Added support for spi-nor device pm in m25p80

[Kamal Dasu]

The changes below implements power management support in m25p80.
m25p80 pm resume() calls newly a implemented spi_nor_pm_rescan()
function that sets up the spi-nor flash to its probed state. 
This is needed on platfroms that turn off power to the spi-nor
part on suspend, in such cases some parts might need to: 
1. remove flash protection (if applicable)
2. set the address width and/or
3. set the read mode to quad mode depending on the probed configuration

The changes use existing spi-nor framework by refactoring code a bit so
that it can be reused in scan and rescan functions. Changes also ensures
that for platforms that do not need pm support shall not be affected. 

Kamal Dasu (5):
  mtd: spi-nor: Added way to rescan spi-nor device
  mtd: m25p80: Added pm ops support
  spi: Added way to check for pm support for flash devices
  mtd: m25p80: Check if the spi flash device has pm support
  spi: bcm-qspi: Implement the master flash_pm_supported() call

 drivers/mtd/devices/m25p80.c  | 21 ++++++++++
 drivers/mtd/spi-nor/spi-nor.c | 93 +++++++++++++++++++++++++++++++++++++------
 drivers/spi/spi-bcm-qspi.c    |  6 +++
 include/linux/mtd/spi-nor.h   | 17 ++++++++
 include/linux/spi/spi.h       |  8 ++++
 5 files changed, 133 insertions(+), 12 deletions(-)

-- 
1.9.1

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

[Kamal Dasu]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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;
+
+	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);
+
+	spi_nor_print_flash_info(nor, info);
+	dev_dbg(dev, "addr width %d read mode %d",
+		nor->addr_width, nor->flash_read);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_nor_pm_rescan);
+
 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
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kamal Dasu]

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;
+
+	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);
+
+	spi_nor_print_flash_info(nor, info);
+	dev_dbg(dev, "addr width %d read mode %d",
+		nor->addr_width, nor->flash_read);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(spi_nor_pm_rescan);
+
 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
-- 
1.9.1

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

[Cyrille Pitchen]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Cyrille Pitchen]

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
> 

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

[Kamal Dasu]

On Mon, Feb 6, 2017 at 6:46 AM, Cyrille Pitchen
<cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kamal Dasu]

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

[PATCH v1 2/5] mtd: m25p80: Added pm ops support

[Kamal Dasu]

Added power management ops for resume to be able to resan spi-nor
device and set it to right transfer modes in its probed state after
poweron. Some SoC implementations might  power down the spi-nor flash
and loose its initial settings on suspend. A resume should retore the
part to its probed state.

Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/devices/m25p80.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9cf7fcd..4528e33 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -319,10 +319,26 @@ static int m25p_remove(struct spi_device *spi)
 };
 MODULE_DEVICE_TABLE(of, m25p_of_table);
 
+#ifdef CONFIG_PM_SLEEP
+static int m25p_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int m25p_resume(struct device *dev)
+{
+	struct m25p *flash = dev_get_drvdata(dev);
+
+	return spi_nor_pm_rescan(&flash->spi-nor, NULL);
+}
+#endif
+static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
+
 static struct spi_driver m25p80_driver = {
 	.driver = {
 		.name	= "m25p80",
 		.of_match_table = m25p_of_table,
+		.pm     = &m25p_pm_ops,
 	},
 	.id_table	= m25p_ids,
 	.probe	= m25p_probe,
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/5] mtd: m25p80: Added pm ops support

[Kamal Dasu]

Added power management ops for resume to be able to resan spi-nor
device and set it to right transfer modes in its probed state after
poweron. Some SoC implementations might  power down the spi-nor flash
and loose its initial settings on suspend. A resume should retore the
part to its probed state.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9cf7fcd..4528e33 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -319,10 +319,26 @@ static int m25p_remove(struct spi_device *spi)
 };
 MODULE_DEVICE_TABLE(of, m25p_of_table);
 
+#ifdef CONFIG_PM_SLEEP
+static int m25p_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int m25p_resume(struct device *dev)
+{
+	struct m25p *flash = dev_get_drvdata(dev);
+
+	return spi_nor_pm_rescan(&flash->spi-nor, NULL);
+}
+#endif
+static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
+
 static struct spi_driver m25p80_driver = {
 	.driver = {
 		.name	= "m25p80",
 		.of_match_table = m25p_of_table,
+		.pm     = &m25p_pm_ops,
 	},
 	.id_table	= m25p_ids,
 	.probe	= m25p_probe,
-- 
1.9.1

[PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Kamal Dasu]

Device drivers can check if the master controller driver has to support
flash pm. The controller driver indicates this using flash_pm_supported()
spi master interface.

Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 include/linux/spi/spi.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 75c6bd0..b5fbc1b 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -539,6 +539,7 @@ struct spi_master {
 	int (*spi_flash_read)(struct  spi_device *spi,
 			      struct spi_flash_read_message *msg);
 	bool (*flash_read_supported)(struct spi_device *spi);
+	bool (*flash_pm_supported)(struct spi_device *spi);
 
 	/*
 	 * These hooks are for drivers that use a generic implementation
@@ -1185,6 +1186,13 @@ static inline bool spi_flash_read_supported(struct spi_device *spi)
 	       spi->master->flash_read_supported(spi));
 }
 
+/* SPI core interface to indicate flash pm support */
+static inline bool spi_flash_pm_supported(struct spi_device *spi)
+{
+	return (spi->master->flash_pm_supported &&
+		spi->master->flash_pm_supported(spi));
+}
+
 int spi_flash_read(struct spi_device *spi,
 		   struct spi_flash_read_message *msg);
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Kamal Dasu]

Device drivers can check if the master controller driver has to support
flash pm. The controller driver indicates this using flash_pm_supported()
spi master interface.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 include/linux/spi/spi.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 75c6bd0..b5fbc1b 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -539,6 +539,7 @@ struct spi_master {
 	int (*spi_flash_read)(struct  spi_device *spi,
 			      struct spi_flash_read_message *msg);
 	bool (*flash_read_supported)(struct spi_device *spi);
+	bool (*flash_pm_supported)(struct spi_device *spi);
 
 	/*
 	 * These hooks are for drivers that use a generic implementation
@@ -1185,6 +1186,13 @@ static inline bool spi_flash_read_supported(struct spi_device *spi)
 	       spi->master->flash_read_supported(spi));
 }
 
+/* SPI core interface to indicate flash pm support */
+static inline bool spi_flash_pm_supported(struct spi_device *spi)
+{
+	return (spi->master->flash_pm_supported &&
+		spi->master->flash_pm_supported(spi));
+}
+
 int spi_flash_read(struct spi_device *spi,
 		   struct spi_flash_read_message *msg);
 
-- 
1.9.1

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

On Fri, Feb 03, 2017 at 06:31:14PM -0500, Kamal Dasu wrote:

> Device drivers can check if the master controller driver has to support
> flash pm. The controller driver indicates this using flash_pm_supported()
> spi master interface.

What is "flash pm" and how would a client use it given that no API for
actually managing power is being added here?  Someone looking at the API
needs to be able to figure these things out and right now I can't see
how they'd do that...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

On Fri, Feb 03, 2017 at 06:31:14PM -0500, Kamal Dasu wrote:

> Device drivers can check if the master controller driver has to support
> flash pm. The controller driver indicates this using flash_pm_supported()
> spi master interface.

What is "flash pm" and how would a client use it given that no API for
actually managing power is being added here?  Someone looking at the API
needs to be able to figure these things out and right now I can't see
how they'd do that...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Kamal Dasu]

On Sat, Feb 4, 2017 at 6:25 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Feb 03, 2017 at 06:31:14PM -0500, Kamal Dasu wrote:
>
>> Device drivers can check if the master controller driver has to support
>> flash pm. The controller driver indicates this using flash_pm_supported()
>> spi master interface.
>
> What is "flash pm" and how would a client use it given that no API for
> actually managing power is being added here?  Someone looking at the API
> needs to be able to figure these things out and right now I can't see
> how they'd do that...o

The flash_pm function just indicates if  m25p80 resume op that does a
spi_nor_pm_rescan() is needed, and by default will do nothing for
other platforms. So the basic idea of the patches was to execute the
only necessary spi_nor_pm_rescan() on resume when flash parts are
reset on suspend. So from controller perspective there is no
additional pm activity to be done. Patches 1/5-2/5 is what is needed
actually. And Patches 3/3 - 5/5 only add a check if m25p80 resume
should call spi_nor_pm_rescan(). And the controller driver shall
implement the flash_pm function and return true if it does need a
rescan to be done on resume.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Kamal Dasu]

On Sat, Feb 4, 2017 at 6:25 AM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Feb 03, 2017 at 06:31:14PM -0500, Kamal Dasu wrote:
>
>> Device drivers can check if the master controller driver has to support
>> flash pm. The controller driver indicates this using flash_pm_supported()
>> spi master interface.
>
> What is "flash pm" and how would a client use it given that no API for
> actually managing power is being added here?  Someone looking at the API
> needs to be able to figure these things out and right now I can't see
> how they'd do that...o

The flash_pm function just indicates if  m25p80 resume op that does a
spi_nor_pm_rescan() is needed, and by default will do nothing for
other platforms. So the basic idea of the patches was to execute the
only necessary spi_nor_pm_rescan() on resume when flash parts are
reset on suspend. So from controller perspective there is no
additional pm activity to be done. Patches 1/5-2/5 is what is needed
actually. And Patches 3/3 - 5/5 only add a check if m25p80 resume
should call spi_nor_pm_rescan(). And the controller driver shall
implement the flash_pm function and return true if it does need a
rescan to be done on resume.

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Cyrille Pitchen]

Hi all,

Le 04/02/2017 à 21:47, Kamal Dasu a écrit :
> On Sat, Feb 4, 2017 at 6:25 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Fri, Feb 03, 2017 at 06:31:14PM -0500, Kamal Dasu wrote:
>>
>>> Device drivers can check if the master controller driver has to support
>>> flash pm. The controller driver indicates this using flash_pm_supported()
>>> spi master interface.
>>
>> What is "flash pm" and how would a client use it given that no API for
>> actually managing power is being added here?  Someone looking at the API
>> needs to be able to figure these things out and right now I can't see
>> how they'd do that...o
> 
> The flash_pm function just indicates if  m25p80 resume op that does a
> spi_nor_pm_rescan() is needed, and by default will do nothing for
> other platforms. So the basic idea of the patches was to execute the
> only necessary spi_nor_pm_rescan() on resume when flash parts are
> reset on suspend. So from controller perspective there is no
> additional pm activity to be done. Patches 1/5-2/5 is what is needed
> actually. And Patches 3/3 - 5/5 only add a check if m25p80 resume
> should call spi_nor_pm_rescan(). And the controller driver shall
> implement the flash_pm function and return true if it does need a
> rescan to be done on resume.
> 

I don't understand why we extend in the SPI framework API to add power
management features but only for SPI flashes. I guess concerning the power
management, there is nothing special about SPI flashes: they are just SPI
devices like others. So why not extend the SPI framework, if needed, with
something working for all SPI devices, not just for SPI flashes.

There was a good reason to create a flash specific API in the SPI
framework: spi_flash_read(), flash_read_supported().
The reason is that the SPI controller needs protocol info (op code,
address, dummy cycles, SPI x-y-z protocol, ...) to handle the read
operation correctly and those pieces of information were lost in m25p80.c
when it used the regular SPI API with spi_transfer and spi_message
structures. Hence spi_flash_read() is a mean to bypass the regular SPI API
and provide the SPI controller driver with all the protocol info it needs.

Back to the power management use case, I don't see any technical reason to
create a SPI flash oriented solution.

Best regards,

Cyrille
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Cyrille Pitchen]

Hi all,

Le 04/02/2017 à 21:47, Kamal Dasu a écrit :
> On Sat, Feb 4, 2017 at 6:25 AM, Mark Brown <broonie@kernel.org> wrote:
>> On Fri, Feb 03, 2017 at 06:31:14PM -0500, Kamal Dasu wrote:
>>
>>> Device drivers can check if the master controller driver has to support
>>> flash pm. The controller driver indicates this using flash_pm_supported()
>>> spi master interface.
>>
>> What is "flash pm" and how would a client use it given that no API for
>> actually managing power is being added here?  Someone looking at the API
>> needs to be able to figure these things out and right now I can't see
>> how they'd do that...o
> 
> The flash_pm function just indicates if  m25p80 resume op that does a
> spi_nor_pm_rescan() is needed, and by default will do nothing for
> other platforms. So the basic idea of the patches was to execute the
> only necessary spi_nor_pm_rescan() on resume when flash parts are
> reset on suspend. So from controller perspective there is no
> additional pm activity to be done. Patches 1/5-2/5 is what is needed
> actually. And Patches 3/3 - 5/5 only add a check if m25p80 resume
> should call spi_nor_pm_rescan(). And the controller driver shall
> implement the flash_pm function and return true if it does need a
> rescan to be done on resume.
> 

I don't understand why we extend in the SPI framework API to add power
management features but only for SPI flashes. I guess concerning the power
management, there is nothing special about SPI flashes: they are just SPI
devices like others. So why not extend the SPI framework, if needed, with
something working for all SPI devices, not just for SPI flashes.

There was a good reason to create a flash specific API in the SPI
framework: spi_flash_read(), flash_read_supported().
The reason is that the SPI controller needs protocol info (op code,
address, dummy cycles, SPI x-y-z protocol, ...) to handle the read
operation correctly and those pieces of information were lost in m25p80.c
when it used the regular SPI API with spi_transfer and spi_message
structures. Hence spi_flash_read() is a mean to bypass the regular SPI API
and provide the SPI controller driver with all the protocol info it needs.

Back to the power management use case, I don't see any technical reason to
create a SPI flash oriented solution.

Best regards,

Cyrille

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Mon, Feb 06, 2017 at 11:44:09AM +0100, Cyrille Pitchen wrote:
> Le 04/02/2017 à 21:47, Kamal Dasu a écrit :

> >> What is "flash pm" and how would a client use it given that no API for
> >> actually managing power is being added here?  Someone looking at the API
> >> needs to be able to figure these things out and right now I can't see
> >> how they'd do that...o

> > The flash_pm function just indicates if  m25p80 resume op that does a
> > spi_nor_pm_rescan() is needed, and by default will do nothing for
> > other platforms. So the basic idea of the patches was to execute the
> > only necessary spi_nor_pm_rescan() on resume when flash parts are
> > reset on suspend. So from controller perspective there is no

I can't tell what this means, sorry, and writing this e-mail does not
address the incomprehensibility of the API.  Why would the controller
driver have anything to do with whatever this spi_nor_pm_rescan()
operation does?

> I don't understand why we extend in the SPI framework API to add power
> management features but only for SPI flashes. I guess concerning the power
> management, there is nothing special about SPI flashes: they are just SPI
> devices like others. So why not extend the SPI framework, if needed, with
> something working for all SPI devices, not just for SPI flashes.

It's already possible for SPI devices to do runtime PM.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Mon, Feb 06, 2017 at 11:44:09AM +0100, Cyrille Pitchen wrote:
> Le 04/02/2017 à 21:47, Kamal Dasu a écrit :

> >> What is "flash pm" and how would a client use it given that no API for
> >> actually managing power is being added here?  Someone looking at the API
> >> needs to be able to figure these things out and right now I can't see
> >> how they'd do that...o

> > The flash_pm function just indicates if  m25p80 resume op that does a
> > spi_nor_pm_rescan() is needed, and by default will do nothing for
> > other platforms. So the basic idea of the patches was to execute the
> > only necessary spi_nor_pm_rescan() on resume when flash parts are
> > reset on suspend. So from controller perspective there is no

I can't tell what this means, sorry, and writing this e-mail does not
address the incomprehensibility of the API.  Why would the controller
driver have anything to do with whatever this spi_nor_pm_rescan()
operation does?

> I don't understand why we extend in the SPI framework API to add power
> management features but only for SPI flashes. I guess concerning the power
> management, there is nothing special about SPI flashes: they are just SPI
> devices like others. So why not extend the SPI framework, if needed, with
> something working for all SPI devices, not just for SPI flashes.

It's already possible for SPI devices to do runtime PM.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Kamal Dasu]

Patches 3-5 are not needed. I will refactor patch 1-2 and send a v2 version.

Thanks
Kamal

On Mon, Feb 6, 2017 at 11:46 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Feb 06, 2017 at 11:44:09AM +0100, Cyrille Pitchen wrote:
>> Le 04/02/2017 à 21:47, Kamal Dasu a écrit :
>
>> >> What is "flash pm" and how would a client use it given that no API for
>> >> actually managing power is being added here?  Someone looking at the API
>> >> needs to be able to figure these things out and right now I can't see
>> >> how they'd do that...o
>
>> > The flash_pm function just indicates if  m25p80 resume op that does a
>> > spi_nor_pm_rescan() is needed, and by default will do nothing for
>> > other platforms. So the basic idea of the patches was to execute the
>> > only necessary spi_nor_pm_rescan() on resume when flash parts are
>> > reset on suspend. So from controller perspective there is no
>
> I can't tell what this means, sorry, and writing this e-mail does not
> address the incomprehensibility of the API.  Why would the controller
> driver have anything to do with whatever this spi_nor_pm_rescan()
> operation does?
>
>> I don't understand why we extend in the SPI framework API to add power
>> management features but only for SPI flashes. I guess concerning the power
>> management, there is nothing special about SPI flashes: they are just SPI
>> devices like others. So why not extend the SPI framework, if needed, with
>> something working for all SPI devices, not just for SPI flashes.
>
> It's already possible for SPI devices to do runtime PM.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/5] spi: Added way to check for pm support for flash devices

[Kamal Dasu]

Patches 3-5 are not needed. I will refactor patch 1-2 and send a v2 version.

Thanks
Kamal

On Mon, Feb 6, 2017 at 11:46 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Feb 06, 2017 at 11:44:09AM +0100, Cyrille Pitchen wrote:
>> Le 04/02/2017 à 21:47, Kamal Dasu a écrit :
>
>> >> What is "flash pm" and how would a client use it given that no API for
>> >> actually managing power is being added here?  Someone looking at the API
>> >> needs to be able to figure these things out and right now I can't see
>> >> how they'd do that...o
>
>> > The flash_pm function just indicates if  m25p80 resume op that does a
>> > spi_nor_pm_rescan() is needed, and by default will do nothing for
>> > other platforms. So the basic idea of the patches was to execute the
>> > only necessary spi_nor_pm_rescan() on resume when flash parts are
>> > reset on suspend. So from controller perspective there is no
>
> I can't tell what this means, sorry, and writing this e-mail does not
> address the incomprehensibility of the API.  Why would the controller
> driver have anything to do with whatever this spi_nor_pm_rescan()
> operation does?
>
>> I don't understand why we extend in the SPI framework API to add power
>> management features but only for SPI flashes. I guess concerning the power
>> management, there is nothing special about SPI flashes: they are just SPI
>> devices like others. So why not extend the SPI framework, if needed, with
>> something working for all SPI devices, not just for SPI flashes.
>
> It's already possible for SPI devices to do runtime PM.

[PATCH v1 4/5] mtd: m25p80: Check if the spi flash device has pm support

[Kamal Dasu]

Call the spi_nor_rescan() only if the controller driver needs this
support. This way SoCs that need this feature can use it.

Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/devices/m25p80.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 4528e33..ffdec60 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -328,8 +328,13 @@ static int m25p_suspend(struct device *dev)
 static int m25p_resume(struct device *dev)
 {
 	struct m25p *flash = dev_get_drvdata(dev);
+	struct spi_device *spi = flash->spi;
+	int ret = 0;
+
+	if (spi_flash_pm_supported(spi))
+		ret = spi_nor_pm_rescan(&flash->spi_nor, NULL);
 
-	return spi_nor_pm_rescan(&flash->spi-nor, NULL);
+	return ret;
 }
 #endif
 static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 4/5] mtd: m25p80: Check if the spi flash device has pm support

[Kamal Dasu]

Call the spi_nor_rescan() only if the controller driver needs this
support. This way SoCs that need this feature can use it.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 4528e33..ffdec60 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -328,8 +328,13 @@ static int m25p_suspend(struct device *dev)
 static int m25p_resume(struct device *dev)
 {
 	struct m25p *flash = dev_get_drvdata(dev);
+	struct spi_device *spi = flash->spi;
+	int ret = 0;
+
+	if (spi_flash_pm_supported(spi))
+		ret = spi_nor_pm_rescan(&flash->spi_nor, NULL);
 
-	return spi_nor_pm_rescan(&flash->spi-nor, NULL);
+	return ret;
 }
 #endif
 static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
-- 
1.9.1

Re: [PATCH v1 4/5] mtd: m25p80: Check if the spi flash device has pm support

[Cyrille Pitchen]

Hi Kamal,

Le 04/02/2017 à 00:31, Kamal Dasu a écrit :
> Call the spi_nor_rescan() only if the controller driver needs this
> support. This way SoCs that need this feature can use it.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/mtd/devices/m25p80.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 4528e33..ffdec60 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -328,8 +328,13 @@ static int m25p_suspend(struct device *dev)
>  static int m25p_resume(struct device *dev)
>  {
>  	struct m25p *flash = dev_get_drvdata(dev);
> +	struct spi_device *spi = flash->spi;
> +	int ret = 0;
> +
> +	if (spi_flash_pm_supported(spi))
> +		ret = spi_nor_pm_rescan(&flash->spi_nor, NULL);
>

Why don't you squash patch 2 into this one? Patch 2 suggests that
spi_nor_pm_rescan() could safely be called in any case but now this patch
suggests that calling that function is not so safe.

I see two cases:

1/ either calling spi_nor_pm_rescan() is safe in any case, then patches 3,
4 and 5 are needless.

2/ or calling spi_nor_pm_rescan() has unwanted side effects in some cases,
then patch 2 should be squashed into this patch: patch 2 will never be
taken alone in the spi-nor tree if it may introduce bugs or regressions.

If something has to be done in the SPI sub-system, I guess it should be
done first: for sure applying patches 3 and 5 would not create any issue.
Then patches 2 and 4 squashed into a single patch may be safely applied after.

However, patches 3 and 5 still need to be discussed first.

Best regards,

Cyrille

> -	return spi_nor_pm_rescan(&flash->spi-nor, NULL);
> +	return ret;
>  }
>  #endif
>  static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/5] mtd: m25p80: Check if the spi flash device has pm support

[Cyrille Pitchen]

Hi Kamal,

Le 04/02/2017 à 00:31, Kamal Dasu a écrit :
> Call the spi_nor_rescan() only if the controller driver needs this
> support. This way SoCs that need this feature can use it.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 4528e33..ffdec60 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -328,8 +328,13 @@ static int m25p_suspend(struct device *dev)
>  static int m25p_resume(struct device *dev)
>  {
>  	struct m25p *flash = dev_get_drvdata(dev);
> +	struct spi_device *spi = flash->spi;
> +	int ret = 0;
> +
> +	if (spi_flash_pm_supported(spi))
> +		ret = spi_nor_pm_rescan(&flash->spi_nor, NULL);
>

Why don't you squash patch 2 into this one? Patch 2 suggests that
spi_nor_pm_rescan() could safely be called in any case but now this patch
suggests that calling that function is not so safe.

I see two cases:

1/ either calling spi_nor_pm_rescan() is safe in any case, then patches 3,
4 and 5 are needless.

2/ or calling spi_nor_pm_rescan() has unwanted side effects in some cases,
then patch 2 should be squashed into this patch: patch 2 will never be
taken alone in the spi-nor tree if it may introduce bugs or regressions.

If something has to be done in the SPI sub-system, I guess it should be
done first: for sure applying patches 3 and 5 would not create any issue.
Then patches 2 and 4 squashed into a single patch may be safely applied after.

However, patches 3 and 5 still need to be discussed first.

Best regards,

Cyrille

> -	return spi_nor_pm_rescan(&flash->spi-nor, NULL);
> +	return ret;
>  }
>  #endif
>  static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
> 

Re: [PATCH v1 4/5] mtd: m25p80: Check if the spi flash device has pm support

[Kamal Dasu]

On Mon, Feb 6, 2017 at 6:01 AM, Cyrille Pitchen
<cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> Hi Kamal,
>
> Le 04/02/2017 à 00:31, Kamal Dasu a écrit :
>> Call the spi_nor_rescan() only if the controller driver needs this
>> support. This way SoCs that need this feature can use it.
>>
>> Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/mtd/devices/m25p80.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 4528e33..ffdec60 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -328,8 +328,13 @@ static int m25p_suspend(struct device *dev)
>>  static int m25p_resume(struct device *dev)
>>  {
>>       struct m25p *flash = dev_get_drvdata(dev);
>> +     struct spi_device *spi = flash->spi;
>> +     int ret = 0;
>> +
>> +     if (spi_flash_pm_supported(spi))
>> +             ret = spi_nor_pm_rescan(&flash->spi_nor, NULL);
>>
>
> Why don't you squash patch 2 into this one? Patch 2 suggests that
> spi_nor_pm_rescan() could safely be called in any case but now this patch
> suggests that calling that function is not so safe.
>
> I see two cases:
>
> 1/ either calling spi_nor_pm_rescan() is safe in any case, then patches 3,
> 4 and 5 are needless.
>

I agree it should be safe to call resume after implementing your
suggestions for patch 1 itself.

> 2/ or calling spi_nor_pm_rescan() has unwanted side effects in some cases,
> then patch 2 should be squashed into this patch: patch 2 will never be
> taken alone in the spi-nor tree if it may introduce bugs or regressions.
>
> If something has to be done in the SPI sub-system, I guess it should be
> done first: for sure applying patches 3 and 5 would not create any issue.
> Then patches 2 and 4 squashed into a single patch may be safely applied after.
>
> However, patches 3 and 5 still need to be discussed first.
>

I agree that these are not needed.

> Best regards,
>
> Cyrille
>
>> -     return spi_nor_pm_rescan(&flash->spi-nor, NULL);
>> +     return ret;
>>  }
>>  #endif
>>  static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
>>
>

Thanks
Kamal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/5] mtd: m25p80: Check if the spi flash device has pm support

[Kamal Dasu]

On Mon, Feb 6, 2017 at 6:01 AM, Cyrille Pitchen
<cyrille.pitchen@atmel.com> wrote:
> Hi Kamal,
>
> Le 04/02/2017 à 00:31, Kamal Dasu a écrit :
>> Call the spi_nor_rescan() only if the controller driver needs this
>> support. This way SoCs that need this feature can use it.
>>
>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>> ---
>>  drivers/mtd/devices/m25p80.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 4528e33..ffdec60 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -328,8 +328,13 @@ static int m25p_suspend(struct device *dev)
>>  static int m25p_resume(struct device *dev)
>>  {
>>       struct m25p *flash = dev_get_drvdata(dev);
>> +     struct spi_device *spi = flash->spi;
>> +     int ret = 0;
>> +
>> +     if (spi_flash_pm_supported(spi))
>> +             ret = spi_nor_pm_rescan(&flash->spi_nor, NULL);
>>
>
> Why don't you squash patch 2 into this one? Patch 2 suggests that
> spi_nor_pm_rescan() could safely be called in any case but now this patch
> suggests that calling that function is not so safe.
>
> I see two cases:
>
> 1/ either calling spi_nor_pm_rescan() is safe in any case, then patches 3,
> 4 and 5 are needless.
>

I agree it should be safe to call resume after implementing your
suggestions for patch 1 itself.

> 2/ or calling spi_nor_pm_rescan() has unwanted side effects in some cases,
> then patch 2 should be squashed into this patch: patch 2 will never be
> taken alone in the spi-nor tree if it may introduce bugs or regressions.
>
> If something has to be done in the SPI sub-system, I guess it should be
> done first: for sure applying patches 3 and 5 would not create any issue.
> Then patches 2 and 4 squashed into a single patch may be safely applied after.
>
> However, patches 3 and 5 still need to be discussed first.
>

I agree that these are not needed.

> Best regards,
>
> Cyrille
>
>> -     return spi_nor_pm_rescan(&flash->spi-nor, NULL);
>> +     return ret;
>>  }
>>  #endif
>>  static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
>>
>

Thanks
Kamal

[PATCH v1 5/5] spi: bcm-qspi: Implement the master flash_pm_supported() call

[Kamal Dasu]

The Broadcom SoCs needs pm rescan support for the flash device to
to be cofgured in the probed state on pm resume. Implement
flash_pm_supported() spi core interface to return true.

Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi-bcm-qspi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index 14f9dea..0abcde6 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -958,6 +958,11 @@ static int bcm_qspi_transfer_one(struct spi_master *master,
 	return 0;
 }
 
+static bool bcm_qspi_flash_pm_supported(struct spi_device *spi)
+{
+	return true;
+}
+
 static void bcm_qspi_cleanup(struct spi_device *spi)
 {
 	struct bcm_qspi_parms *xp = spi_get_ctldata(spi);
@@ -1194,6 +1199,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	master->setup = bcm_qspi_setup;
 	master->transfer_one = bcm_qspi_transfer_one;
 	master->spi_flash_read = bcm_qspi_flash_read;
+	master->flash_pm_supported = bcm_qspi_flash_pm_supported;
 	master->cleanup = bcm_qspi_cleanup;
 	master->dev.of_node = dev->of_node;
 	master->num_chipselect = NUM_CHIPSELECT;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 5/5] spi: bcm-qspi: Implement the master flash_pm_supported() call

[Kamal Dasu]

The Broadcom SoCs needs pm rescan support for the flash device to
to be cofgured in the probed state on pm resume. Implement
flash_pm_supported() spi core interface to return true.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/spi/spi-bcm-qspi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index 14f9dea..0abcde6 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -958,6 +958,11 @@ static int bcm_qspi_transfer_one(struct spi_master *master,
 	return 0;
 }
 
+static bool bcm_qspi_flash_pm_supported(struct spi_device *spi)
+{
+	return true;
+}
+
 static void bcm_qspi_cleanup(struct spi_device *spi)
 {
 	struct bcm_qspi_parms *xp = spi_get_ctldata(spi);
@@ -1194,6 +1199,7 @@ int bcm_qspi_probe(struct platform_device *pdev,
 	master->setup = bcm_qspi_setup;
 	master->transfer_one = bcm_qspi_transfer_one;
 	master->spi_flash_read = bcm_qspi_flash_read;
+	master->flash_pm_supported = bcm_qspi_flash_pm_supported;
 	master->cleanup = bcm_qspi_cleanup;
 	master->dev.of_node = dev->of_node;
 	master->num_chipselect = NUM_CHIPSELECT;
-- 
1.9.1