mtd: devices: m25p80: Add PM suspend resume support

Adding PM support so as to be able to probe spi-nor flash on resume. There are vendor specific commands to setup the transfer mode and enable read/write as part of spi_nor_scan(), done on initial probe and needed on resume(). The spi-nor structure is private to the m25p driver and hence is the only place this can be done without having to duplicate code in controller driver.

Signed-off-by Kamal Dasu <[email protected]>

---
 drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 20 deletions(-)

drivers/mtd/devices/m25p80.c +48 -21

index 9cf7fcd..48c3f64 100644
@@ -186,6 +186,39 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 }
 
 /*
+ * scan for spi nor flash vendor parts and setup
+ * read/write mode
+ */
+static int m25p_nor_flash_scan(struct device *dev)
+{
+       struct m25p *flash = dev_get_drvdata(dev);
+       struct flash_platform_data      *data;
+       char *flash_name = NULL;
+       enum read_mode mode = SPI_NOR_NORMAL;
+
+       data = dev_get_platdata(dev);
+
+       /* For some (historical?) reason many platforms provide two different
+        * names in flash_platform_data: "name" and "type". Quite often name is
+        * set to "m25p80" and then "type" provides a real chip name.
+        * If that's the case, respect "type" and ignore a "name".
+        */
+       if (data && data->type)
+               flash_name = data->type;
+       else if (!strcmp(flash->spi->modalias, "spi-nor"))
+               flash_name = NULL; /* auto-detect */
+       else
+               flash_name = flash->spi->modalias;
+
+       if (flash->spi->mode & SPI_RX_QUAD)
+               mode = SPI_NOR_QUAD;
+       else if (flash->spi->mode & SPI_RX_DUAL)
+               mode = SPI_NOR_DUAL;
+
+       return spi_nor_scan(&flash->spi_nor, flash_name, mode);
+}
+
+/*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver
  * understands FAST_READ (for clocks over 25 MHz).

@@ -195,8 +228,6 @@ static int m25p_probe(struct spi_device *spi)
        struct flash_platform_data      *data;
        struct m25p *flash;
        struct spi_nor *nor;
-       enum read_mode mode = SPI_NOR_NORMAL;
-       char *flash_name;
        int ret;
 
        data = dev_get_platdata(&spi->dev);

@@ -220,27 +251,11 @@ static int m25p_probe(struct spi_device *spi)
        spi_set_drvdata(spi, flash);
        flash->spi = spi;
 
-       if (spi->mode & SPI_RX_QUAD)
-               mode = SPI_NOR_QUAD;
-       else if (spi->mode & SPI_RX_DUAL)
-               mode = SPI_NOR_DUAL;
-
        if (data && data->name)
                nor->mtd.name = data->name;
 
-       /* For some (historical?) reason many platforms provide two different
-        * names in flash_platform_data: "name" and "type". Quite often name is
-        * set to "m25p80" and then "type" provides a real chip name.
-        * If that's the case, respect "type" and ignore a "name".
-        */
-       if (data && data->type)
-               flash_name = data->type;
-       else if (!strcmp(spi->modalias, "spi-nor"))
-               flash_name = NULL; /* auto-detect */
-       else
-               flash_name = spi->modalias;
+       ret = m25p_nor_flash_scan(nor->dev);
 
-       ret = spi_nor_scan(nor, flash_name, mode);
        if (ret)
                return ret;
 

@@ -248,7 +263,6 @@ static int m25p_probe(struct spi_device *spi)
                                   data ? data->nr_parts : 0);
 }
 
-
 static int m25p_remove(struct spi_device *spi)
 {
        struct m25p     *flash = spi_get_drvdata(spi);

@@ -319,10 +333,24 @@ static const struct of_device_id m25p_of_table[] = {
 };
 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)
+{
+       return m25p_nor_flash_scan(dev);
+}
+#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

Adding PM support so as to be able to probe spi-nor flash on resume. There are vendor specific commands to setup the transfer mode and enable read/write as part of spi_nor_scan(), done on initial probe and needed on resume(). The spi-nor structure is private to the m25p driver and hence is the only place this can be done without having to duplicate code in controller driver.

Signed-off-by Kamal Dasu <[email protected]>

---
 drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 20 deletions(-)

drivers/mtd/devices/m25p80.c +45 -20

index 9cf7fcd..48c3f64 100644
@@ -186,6 +186,39 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 }
 
 /*
+ * scan for spi nor flash vendor parts and setup
+ * read/write mode
+ */
+static int m25p_nor_flash_scan(struct device *dev)
+{
+	struct m25p *flash = dev_get_drvdata(dev);
+	struct flash_platform_data	*data;
+	char *flash_name = NULL;
+	enum read_mode mode = SPI_NOR_NORMAL;
+
+	data = dev_get_platdata(dev);
+
+	/* For some (historical?) reason many platforms provide two different
+	 * names in flash_platform_data: "name" and "type". Quite often name is
+	 * set to "m25p80" and then "type" provides a real chip name.
+	 * If that's the case, respect "type" and ignore a "name".
+	 */
+	if (data && data->type)
+		flash_name = data->type;
+	else if (!strcmp(flash->spi->modalias, "spi-nor"))
+		flash_name = NULL; /* auto-detect */
+	else
+		flash_name = flash->spi->modalias;
+
+	if (flash->spi->mode & SPI_RX_QUAD)
+		mode = SPI_NOR_QUAD;
+	else if (flash->spi->mode & SPI_RX_DUAL)
+		mode = SPI_NOR_DUAL;
+
+	return spi_nor_scan(&flash->spi_nor, flash_name, mode);
+}
+
+/*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver
  * understands FAST_READ (for clocks over 25 MHz).

@@ -195,8 +228,6 @@ static int m25p_probe(struct spi_device *spi)
 	struct flash_platform_data	*data;
 	struct m25p *flash;
 	struct spi_nor *nor;
-	enum read_mode mode = SPI_NOR_NORMAL;
-	char *flash_name;
 	int ret;
 
 	data = dev_get_platdata(&spi->dev);

@@ -220,27 +251,11 @@ static int m25p_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, flash);
 	flash->spi = spi;
 
-	if (spi->mode & SPI_RX_QUAD)
-		mode = SPI_NOR_QUAD;
-	else if (spi->mode & SPI_RX_DUAL)
-		mode = SPI_NOR_DUAL;
-
 	if (data && data->name)
 		nor->mtd.name = data->name;
 
-	/* For some (historical?) reason many platforms provide two different
-	 * names in flash_platform_data: "name" and "type". Quite often name is
-	 * set to "m25p80" and then "type" provides a real chip name.
-	 * If that's the case, respect "type" and ignore a "name".
-	 */
-	if (data && data->type)
-		flash_name = data->type;
-	else if (!strcmp(spi->modalias, "spi-nor"))
-		flash_name = NULL; /* auto-detect */
-	else
-		flash_name = spi->modalias;
+	ret = m25p_nor_flash_scan(nor->dev);
 
-	ret = spi_nor_scan(nor, flash_name, mode);
 	if (ret)
 		return ret;
 

@@ -248,7 +263,6 @@ static int m25p_probe(struct spi_device *spi)
 				   data ? data->nr_parts : 0);
 }
 
-
 static int m25p_remove(struct spi_device *spi)
 {
 	struct m25p	*flash = spi_get_drvdata(spi);

@@ -319,10 +333,24 @@ static const struct of_device_id m25p_of_table[] = {
 };
 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)
+{
+	return m25p_nor_flash_scan(dev);
+}
+#endif

Cyrille,

On Mon, Oct 17, 2016 at 1:08 PM, Cyrille Pitchen
<[email protected] <mailto:[email protected]>> wrote:

Hi Kamal,

Le 12/09/2016 à 22:01, Kamal Dasu a écrit :
> Adding PM support so as to be able to probe spi-nor flash > on resume. There are vendor specific commands to setup > the transfer mode and enable read/write as part of > spi_nor_scan(), done on initial probe and needed on resume(). > The spi-nor structure is private to the m25p driver and hence > is the only place this can be done without having to duplicate > code in controller driver.

Just to be sure I've understood the purpose of this patch: here we suppose
that the SPI NOR memory power supply was cut when the CPU entered its suspend
mode. So when the CPU is resumed, the SPI NOR memory power supply is enabled
again as well and the internal state of the memory is reset.

Memory could be intact and in refresh state. But generally the spi nor part might be powered down.

Depending on the manufacturer, we may need to send dedicated commands so the
memory is ready again (for instance, a Global Unlock Block command for SST
memories so we can perform Sector Erase and Page Program operations). Those commands are sent from spi_nor_scan(). Hence you call spi_nor_scan()
once again from the resume callback.

Yes there are are chain of calls that setup the state of the part from spi_nor_scan that are vendor specific based on the jdec id. So yes some of the unlock commands need to be sent before we can start using the part again.

If so, your patch does make sense but I wonder whether some operations done
inside spi_nor_scan() and not related to the memory itself but more to other
layers like mtd (struct mtd_info) could always be safely performed a second
time. I don't know if that issue already exists or would ever exist, if so
it might be interesting to find a mean to tell spi_nor_scan() whether it's
called for the first time on this memory (boot) or not (resume).

I do see the mtd settings that could be done a second time but should not cause any issues IMHO. I don't see a need to distinguish between a (re)boot or a resume and complicate the code. Unless somebody can point

Cyrille,

On Mon, Oct 17, 2016 at 1:08 PM, Cyrille Pitchen <[email protected] <mailto:[email protected]>> wrote:

Hi Kamal,

Le 12/09/2016 à 22:01, Kamal Dasu a écrit :
> Adding PM support so as to be able to probe spi-nor flash > on resume. There are vendor specific commands to setup > the transfer mode and enable read/write as part of > spi_nor_scan(), done on initial probe and needed on resume(). > The spi-nor structure is private to the m25p driver and hence > is the only place this can be done without having to duplicate > code in controller driver.

Just to be sure I've understood the purpose of this patch: here we suppose that the SPI NOR memory power supply was cut when the CPU entered its suspend mode. So when the CPU is resumed, the SPI NOR memory power supply is enabled again as well and the internal state of the memory is reset.

Memory could be intact and in refresh state. But generally the spi nor part might be powered down.

Depending on the manufacturer, we may need to send dedicated commands so the memory is ready again (for instance, a Global Unlock Block command for SST memories so we can perform Sector Erase and Page Program operations). Those commands are sent from spi_nor_scan(). Hence you call spi_nor_scan() once again from the resume callback.

Yes there are are chain of calls that setup the state of the part from spi_nor_scan that are vendor specific based on the jdec id. So yes some of the unlock commands need to be sent before we can start using the part again.

If so, your patch does make sense but I wonder whether some operations done inside spi_nor_scan() and not related to the memory itself but more to other layers like mtd (struct mtd_info) could always be safely performed a second time. I don't know if that issue already exists or would ever exist, if so it might be interesting to find a mean to tell spi_nor_scan() whether it's called for the first time on this memory (boot) or not (resume).

I do see the mtd settings that could be done a second time but should not cause any issues IMHO. I don't see a need to distinguish between a (re)boot or a resume and complicate the code. Unless somebody can point out a specific issue in doing so.

Best regards,

Cyrille

Thanks
Kamal

Quoted 87 lines

Signed-off-by Kamal Dasu <[email protected] <mailto:[email protected]>>

---
 drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 20 deletions(-)

drivers/mtd/devices/m25p80.c +34 -19

index 9cf7fcd..48c3f64 100644
@@ -186,6 +186,39 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 }
 
 /*
+ * scan for spi nor flash vendor parts and setup
+ * read/write mode
+ */
+static int m25p_nor_flash_scan(struct device *dev)
+{
+     struct m25p *flash = dev_get_drvdata(dev);
+     struct flash_platform_data      *data;
+     char *flash_name = NULL;
+     enum read_mode mode = SPI_NOR_NORMAL;
+
+     data = dev_get_platdata(dev);
+
+     /* For some (historical?) reason many platforms provide two different
+      * names in flash_platform_data: "name" and "type". Quite often name is
+      * set to "m25p80" and then "type" provides a real chip name.
+      * If that's the case, respect "type" and ignore a "name".
+      */
+     if (data && data->type)
+             flash_name = data->type;
+     else if (!strcmp(flash->spi->modalias, "spi-nor"))
+             flash_name = NULL; /* auto-detect */
+     else
+             flash_name = flash->spi->modalias;
+
+     if (flash->spi->mode & SPI_RX_QUAD)
+             mode = SPI_NOR_QUAD;
+     else if (flash->spi->mode & SPI_RX_DUAL)
+             mode = SPI_NOR_DUAL;
+
+     return spi_nor_scan(&flash->spi_nor, flash_name, mode);
+}
+
+/*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver
  * understands FAST_READ (for clocks over 25 MHz).

@@ -195,8 +228,6 @@ static int m25p_probe(struct spi_device *spi)
      struct flash_platform_data      *data;
      struct m25p *flash;
      struct spi_nor *nor;
-     enum read_mode mode = SPI_NOR_NORMAL;
-     char *flash_name;
      int ret;
 
      data = dev_get_platdata(&spi->dev);

@@ -220,27 +251,11 @@ static int m25p_probe(struct spi_device *spi)
      spi_set_drvdata(spi, flash);
      flash->spi = spi;
 
-     if (spi->mode & SPI_RX_QUAD)
-             mode = SPI_NOR_QUAD;
-     else if (spi->mode & SPI_RX_DUAL)
-             mode = SPI_NOR_DUAL;
-
      if (data && data->name)
              nor->mtd.name <http://mtd.name> = data->name;
 
-     /* For some (historical?) reason many platforms provide two different
-      * names in flash_platform_data: "name" and "type". Quite often name is
-      * set to "m25p80" and then "type" provides a real chip name.
-      * If that's the case, respect "type" and ignore a "name".
-      */
-     if (data && data->type)
-             flash_name = data->type;
-     else if (!strcmp(spi->modalias, "spi-nor"))
-             flash_name = NULL; /* auto-detect */
-     else
-             flash_name = spi->modalias;
+     ret = m25p_nor_flash_scan(nor->dev);
 
-     ret = spi_nor_scan(nor, flash_name, mode);
      if (ret)
              return ret;
 

drivers/mtd/devices/m25p80.c static int m25p_probe(struct spi_device *spi) · line 251

Quoted 5 lines

drivers/mtd/devices/m25p80.c +0 -1

@@ -248,7 +263,6 @@ static int m25p_probe(struct spi_device *spi)
                                 data ? data->nr_parts : 0);
 }
 
-

Quoted 21 lines

drivers/mtd/devices/m25p80.c +13 -0

 static int m25p_remove(struct spi_device *spi)
 {
      struct m25p     *flash = spi_get_drvdata(spi);

@@ -319,10 +333,24 @@ static const struct of_device_id m25p_of_table[] = {
 };
 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)
+{
+     return m25p_nor_flash_scan(dev);
+}
+#endif
+static SIMPLE_DEV_PM_OPS(m25p_pm_ops, m25p_suspend, m25p_resume);
+
 static struct spi_driver m25p80_driver = {

drivers/mtd/devices/m25p80.c static const struct of_device_id m25p_of_table[] = { · line 356

Quoted 8 lines

drivers/mtd/devices/m25p80.c +1 -0

      .driver = {
              .name   = "m25p80",
              .of_match_table = m25p_of_table,
+             .pm     = &m25p_pm_ops,
      },
      .id_table       = m25p_ids,
      .probe  = m25p_probe,

Hi Kamal,

+Marek

Quoted 43 lines · Kamal Dasu

Cyrille,

On Mon, Oct 17, 2016 at 1:08 PM, Cyrille Pitchen <[email protected] <mailto:[email protected]>> wrote:

Hi Kamal,

Le 12/09/2016 à 22:01, Kamal Dasu a écrit :
> Adding PM support so as to be able to probe spi-nor flash > on resume. There are vendor specific commands to setup > the transfer mode and enable read/write as part of > spi_nor_scan(), done on initial probe and needed on resume(). > The spi-nor structure is private to the m25p driver and hence > is the only place this can be done without having to duplicate > code in controller driver.

Just to be sure I've understood the purpose of this patch: here we suppose that the SPI NOR memory power supply was cut when the CPU entered its suspend mode. So when the CPU is resumed, the SPI NOR memory power supply is enabled again as well and the internal state of the memory is reset.

Memory could be intact and in refresh state. But generally the spi nor part might be powered down.

Depending on the manufacturer, we may need to send dedicated commands so the memory is ready again (for instance, a Global Unlock Block command for SST memories so we can perform Sector Erase and Page Program operations). Those commands are sent from spi_nor_scan(). Hence you call spi_nor_scan() once again from the resume callback.

Yes there are are chain of calls that setup the state of the part from spi_nor_scan that are vendor specific based on the jdec id. So yes some of the unlock commands need to be sent before we can start using the part again.

If so, your patch does make sense but I wonder whether some operations done inside spi_nor_scan() and not related to the memory itself but more to other layers like mtd (struct mtd_info) could always be safely performed a second time. I don't know if that issue already exists or would ever exist, if so it might be interesting to find a mean to tell spi_nor_scan() whether it's called for the first time on this memory (boot) or not (resume).

Quoted 1 line

I do see the mtd settings that could be done a second time but should not cause any issues IMHO. I don't see a need to distinguish between a (re)boot or a resume and complicate the code. Unless somebody can point out a specific issue in doing so.

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

--
Best regards,
Marek Vasut

Marek,

Quoted 6 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

Quoted 4 lines

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

--
Best regards,
Marek Vasut

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Quoted 22 lines · Kamal Dasu

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

Quoted 10 lines

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Would be much nicer if you could just share the script you used.

Quoted 34 lines · Marek Vasut

Quoted 32 lines · Kamal Dasu

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Quoted 1 line

Would be much nicer if you could just share the script you used.

Not sure if this is even possible, but maybe we could. What part of Kamal's description of the test is not clear here?

Even without doing actual copy across suspend/resume cycles, a device which has a power domain architecture where the SPI-NOR flash gets powered down during a suspend state, and the root filesystem is on that flash would expose issues without Kamal's patch here.

Quoted 35 lines · Florian Fainelli

Quoted 34 lines · Marek Vasut

Quoted 32 lines · Kamal Dasu

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Quoted 4 lines

Quoted 1 line

Would be much nicer if you could just share the script you used.

Not sure if this is even possible, but maybe we could. What part of Kamal's description of the test is not clear here?

I cannot check the test and see what was really done, the code is the best description. If someone tells me a script (which is likely about 20 lines) cannot be shared, that's fishy.

Quoted 5 lines

Even without doing actual copy across suspend/resume cycles, a device which has a power domain architecture where the SPI-NOR flash gets powered down during a suspend state, and the root filesystem is on that flash would expose issues without Kamal's patch here.

At least one problem I suspect will happen here is that if a command is in progress on the SPI NOR and the controller suspends, resumes and rescans, it is well possible the SPI NOR will be in undefined state.

Quoted 36 lines · Marek Vasut

Quoted 35 lines · Florian Fainelli

Quoted 34 lines · Marek Vasut

Quoted 32 lines · Kamal Dasu

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Would be much nicer if you could just share the script you used.

Not sure if this is even possible, but maybe we could. What part of Kamal's description of the test is not clear here?

I cannot check the test and see what was really done, the code is the best description. If someone tells me a script (which is likely about 20 lines) cannot be shared, that's fishy.

The reason why I mentioned that is that it's not like our test suite is somewhere out there on Github and we could just point you to it, it could take some time and effort to extract the script, period. It's not fishy, it's just not ideal, and it is a consequence of working in a corporate environment.

Quoted 9 lines

Quoted 4 lines

Even without doing actual copy across suspend/resume cycles, a device which has a power domain architecture where the SPI-NOR flash gets powered down during a suspend state, and the root filesystem is on that flash would expose issues without Kamal's patch here.

At least one problem I suspect will happen here is that if a command is in progress on the SPI NOR and the controller suspends, resumes and rescans, it is well possible the SPI NOR will be in undefined state.

Fair enough.

Quoted 46 lines · Florian Fainelli

Quoted 36 lines · Marek Vasut

Quoted 35 lines · Florian Fainelli

Quoted 34 lines · Marek Vasut

Quoted 32 lines · Kamal Dasu

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Would be much nicer if you could just share the script you used.

Not sure if this is even possible, but maybe we could. What part of Kamal's description of the test is not clear here?

I cannot check the test and see what was really done, the code is the best description. If someone tells me a script (which is likely about 20 lines) cannot be shared, that's fishy.

Quoted 5 lines

The reason why I mentioned that is that it's not like our test suite is somewhere out there on Github and we could just point you to it, it could take some time and effort to extract the script, period. It's not fishy, it's just not ideal, and it is a consequence of working in a corporate environment.

Somehow, it feels like you're digging yourself in deeper and deeper ... now I'm seriously curious how this was tested. Esp. since this patch has potentially massive impact and could annoy a lot of people if it breaks things, so a lot of testing will have to happen on this one. Having a testcase would help ...

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

Even without doing actual copy across suspend/resume cycles, a device which has a power domain architecture where the SPI-NOR flash gets powered down during a suspend state, and the root filesystem is on that flash would expose issues without Kamal's patch here.

At least one problem I suspect will happen here is that if a command is in progress on the SPI NOR and the controller suspends, resumes and rescans, it is well possible the SPI NOR will be in undefined state.

Quoted 2 lines

Fair enough.

--
Best regards,
Marek Vasut

"At least one problem I suspect will happen here is that if a command is in progress on the SPI NOR and the controller suspends, resumes and rescans, it is well possible the SPI NOR will be in undefined state."

I don't think suspend would happen in middle of a command or transfer. On suspend filesystem drivers do a sync and it is my understanding that the mtd layer, spi masters will stop the queue before the m25p80 suspend will get called.

And on resume new transfers continue after
the spi-nor is setup in its probed state.

Kamal

Quoted 76 lines · Marek Vasut

Quoted 46 lines · Florian Fainelli

Quoted 36 lines · Marek Vasut

Quoted 35 lines · Florian Fainelli

Quoted 34 lines · Marek Vasut

Quoted 32 lines · Kamal Dasu

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Would be much nicer if you could just share the script you used.

Not sure if this is even possible, but maybe we could. What part of Kamal's description of the test is not clear here?

I cannot check the test and see what was really done, the code is the best description. If someone tells me a script (which is likely about 20 lines) cannot be shared, that's fishy.

Quoted 5 lines

The reason why I mentioned that is that it's not like our test suite is somewhere out there on Github and we could just point you to it, it could take some time and effort to extract the script, period. It's not fishy, it's just not ideal, and it is a consequence of working in a corporate environment.

Somehow, it feels like you're digging yourself in deeper and deeper ... now I'm seriously curious how this was tested. Esp. since this patch has potentially massive impact and could annoy a lot of people if it breaks things, so a lot of testing will have to happen on this one. Having a testcase would help ...

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Even without doing actual copy across suspend/resume cycles, a device which has a power domain architecture where the SPI-NOR flash gets powered down during a suspend state, and the root filesystem is on that flash would expose issues without Kamal's patch here.

Quoted 7 lines

Quoted 4 lines

At least one problem I suspect will happen here is that if a command is in progress on the SPI NOR and the controller suspends, resumes and rescans, it is well possible the SPI NOR will be in undefined state.

Fair enough.

--
Best regards,
Marek Vasut

Quoted 46 lines · Florian Fainelli

Quoted 36 lines · Marek Vasut

Quoted 35 lines · Florian Fainelli

Quoted 34 lines · Marek Vasut

Quoted 32 lines · Kamal Dasu

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Would be much nicer if you could just share the script you used.

Not sure if this is even possible, but maybe we could. What part of Kamal's description of the test is not clear here?

I cannot check the test and see what was really done, the code is the best description. If someone tells me a script (which is likely about 20 lines) cannot be shared, that's fishy.

Quoted 5 lines

The reason why I mentioned that is that it's not like our test suite is somewhere out there on Github and we could just point you to it, it could take some time and effort to extract the script, period. It's not fishy, it's just not ideal, and it is a consequence of working in a corporate environment.

Somehow, it feels like you're digging yourself in deeper and deeper ... now I'm seriously curious how this was tested. Esp. since this patch has potentially massive impact and could annoy a lot of people if it breaks things, so a lot of testing will have to happen on this one.

Having a testcase would help ...

Quoted 53 lines · Marek Vasut

Quoted 46 lines · Florian Fainelli

Quoted 36 lines · Marek Vasut

Quoted 35 lines · Florian Fainelli

Quoted 34 lines · Marek Vasut

Quoted 32 lines · Kamal Dasu

Quoted 14 lines · Marek Vasut

Quoted 13 lines · Kamal Dasu

Marek,

Quoted 10 lines

Quoted 5 lines

OK, so I'm fine with leaving the patch as is for now but I would like Marek review just to be sure we didn't miss something: Marek, any comments?

I just have one more comment below but it's only a detail.

What would happen if you have a FS attached on this SPI NOR (like UBIFS) and you suspend with this patch ? Will it survive ?

Quoted 6 lines

Quoted 3 lines

I have tried with both jffs2 rw and ubifs rw rootfs on spi-nor booting from spi-nor partitions while active filesystem operations were going on over suspend/resume cycles. And it survives.

Can you elaborate on your test a bit ?

The power management test puts board in standby mode, during standby the DRAM is in refresh and power to the spi-nor is removed. After a set timeout the board wakes up again and resumes from where it left off. This pm test is run while a cp command is invoked where a file is transferred to the rw rootfs. The test loops through said number of iterations of suspend and resumes cycles. The copy operations is eventually done over a few cycles. Once the test is completed filesystem should have no corruptions, the copied files should compare and rootfs should be intact. This test was repeated with jffs2 as well as ubifs rootfs flashed on spi-nor device.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

Would be much nicer if you could just share the script you used.

Not sure if this is even possible, but maybe we could. What part of Kamal's description of the test is not clear here?

I cannot check the test and see what was really done, the code is the best description. If someone tells me a script (which is likely about 20 lines) cannot be shared, that's fishy.

Quoted 5 lines

The reason why I mentioned that is that it's not like our test suite is somewhere out there on Github and we could just point you to it, it could take some time and effort to extract the script, period. It's not fishy, it's just not ideal, and it is a consequence of working in a corporate environment.

Quoted 4 lines

Somehow, it feels like you're digging yourself in deeper and deeper ... now I'm seriously curious how this was tested. Esp. since this patch has potentially massive impact and could annoy a lot of people if it breaks things, so a lot of testing will have to happen on this one.

And somehow it feels like you don't even try to understand that the test may be part of an existing test framework thus making it harder to extract and send as a standalone shell script. Not everything we do is open source, and although there is not necessarily a reason why not, it just is. Not saying this is impossible, or that our testsuite is the best thing since we discovered hot water, just don't expect it to show up instantly, period.

Keep in mind that asking for an explanation about how this was tested initially is a perfectly reasonable thing to do, and you may, or may not be satisfied with the answer, clearly you are not

but asking for test
case is just not common thing

it also indicates that you absolutely
don't trust Kamal's explanation and understanding of the problem, and that can be misinterpreted at best.

Quoted 1 line

Having a testcase would help ...

Agreed, just like having a proper test suite geared towards MTD/SPI that we could contribute to (not necessarily LTP) would help as well. So, where do we start now?