[PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

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 <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9cf7fcd..48c3f64 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -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

--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

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 <kdasu.kdev@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9cf7fcd..48c3f64 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -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

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

Is everyone Ok with the latest patch ?

Kamal

On Mon, Sep 12, 2016 at 4:01 PM, Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 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 <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 9cf7fcd..48c3f64 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -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
>
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

Is everyone Ok with the latest patch ?

Kamal

On Mon, Sep 12, 2016 at 4:01 PM, Kamal Dasu <kdasu.kdev@gmail.com> wrote:
> 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 <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 9cf7fcd..48c3f64 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -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
>

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Cyrille Pitchen]

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.

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.

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

Best regards,

Cyrille


> Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 9cf7fcd..48c3f64 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -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,
> 

--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Cyrille Pitchen]

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.

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.

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

Best regards,

Cyrille


> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 9cf7fcd..48c3f64 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -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,
> 

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Florian Fainelli]

On 10/24/2016 11:18 AM, Kamal Dasu wrote:
> 
> Cyrille,
> 
> On Mon, Oct 17, 2016 at 1:08 PM, Cyrille Pitchen
> <cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org <mailto:cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>> 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, are you okay with Kamal's response? I just saw the 4.10 pull
request pass by and since this patch was still in flight, it did not get
included, can it be queued for 4.11 or is there something else to work on?
-- 
Florian
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Florian Fainelli]

On 10/24/2016 11:18 AM, Kamal Dasu wrote:
> 
> Cyrille,
> 
> On Mon, Oct 17, 2016 at 1:08 PM, Cyrille Pitchen
> <cyrille.pitchen@atmel.com <mailto:cyrille.pitchen@atmel.com>> 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, are you okay with Kamal's response? I just saw the 4.10 pull
request pass by and since this patch was still in flight, it did not get
included, can it be queued for 4.11 or is there something else to work on?
-- 
Florian

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Cyrille Pitchen]

Hi Kamal,

+Marek

Le 24/10/2016 à 20:18, Kamal Dasu a écrit :
> 
> Cyrille,
> 
> On Mon, Oct 17, 2016 at 1:08 PM, Cyrille Pitchen <cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org <mailto:cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>> 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.

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.

> 
>  
> 
>     Best regards,
> 
>     Cyrille
> 
> 
> 
> Thanks
> Kamal
> 
>  
> 
> 
>     > Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org <mailto:kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>>
>     > ---
>     >  drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
>     >  1 file changed, 48 insertions(+), 20 deletions(-)
>     >
>     > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>     > index 9cf7fcd..48c3f64 100644
>     > --- a/drivers/mtd/devices/m25p80.c
>     > +++ b/drivers/mtd/devices/m25p80.c
>     > @@ -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;
>     >
>     > @@ -248,7 +263,6 @@ static int m25p_probe(struct spi_device *spi)
>     >                                  data ? data->nr_parts : 0);
>     >  }
>     >
>     > -
I know this just just a detail but this chunk should not be included in
your patch since this is a cosmetic change and it has nothing to do with
the suspend/resume feature.

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

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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Cyrille Pitchen]

Hi Kamal,

+Marek

Le 24/10/2016 à 20:18, Kamal Dasu a écrit :
> 
> Cyrille,
> 
> On Mon, Oct 17, 2016 at 1:08 PM, Cyrille Pitchen <cyrille.pitchen@atmel.com <mailto:cyrille.pitchen@atmel.com>> 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.

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.

> 
>  
> 
>     Best regards,
> 
>     Cyrille
> 
> 
> 
> Thanks
> Kamal
> 
>  
> 
> 
>     > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com <mailto:kdasu.kdev@gmail.com>>
>     > ---
>     >  drivers/mtd/devices/m25p80.c | 68 +++++++++++++++++++++++++++++++-------------
>     >  1 file changed, 48 insertions(+), 20 deletions(-)
>     >
>     > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>     > index 9cf7fcd..48c3f64 100644
>     > --- a/drivers/mtd/devices/m25p80.c
>     > +++ b/drivers/mtd/devices/m25p80.c
>     > @@ -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;
>     >
>     > @@ -248,7 +263,6 @@ static int m25p_probe(struct spi_device *spi)
>     >                                  data ? data->nr_parts : 0);
>     >  }
>     >
>     > -
I know this just just a detail but this chunk should not be included in
your patch since this is a cosmetic change and it has nothing to do with
the suspend/resume feature.

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

Best regards,

Cyrille

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 11/29/2016 02:11 PM, Cyrille Pitchen wrote:
> Hi Kamal,
> 
> +Marek
> 
> Le 24/10/2016 à 20:18, Kamal Dasu a écrit :
>>
>> Cyrille,
>>
>> On Mon, Oct 17, 2016 at 1:08 PM, Cyrille Pitchen <cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org <mailto:cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>> 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.
> 
> 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
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 11/29/2016 02:11 PM, Cyrille Pitchen wrote:
> Hi Kamal,
> 
> +Marek
> 
> Le 24/10/2016 à 20:18, Kamal Dasu a écrit :
>>
>> Cyrille,
>>
>> On Mon, Oct 17, 2016 at 1:08 PM, Cyrille Pitchen <cyrille.pitchen@atmel.com <mailto:cyrille.pitchen@atmel.com>> 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.
> 
> 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

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

Marek,

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

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.


> --
> Best regards,
> Marek Vasut

Thanks
Kamal

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

Marek,

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

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.


> --
> Best regards,
> Marek Vasut

Thanks
Kamal

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 05:33 PM, Kamal Dasu wrote:
> Marek,
> 
>>>
>>> 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 ?
>>
>>
> 
> 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 05:33 PM, Kamal Dasu wrote:
> Marek,
> 
>>>
>>> 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 ?
>>
>>
> 
> 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

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>> Marek,
>>
>>>>
>>>> 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 ?
>>>
>>>
>>
>> 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.

> --
> Best regards,
> Marek Vasut

Thanks
Kamal

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>> Marek,
>>
>>>>
>>>> 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 ?
>>>
>>>
>>
>> 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.

> --
> Best regards,
> Marek Vasut

Thanks
Kamal

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Michal Suchanek]

Hello,

On 20 January 2017 at 19:50, Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>> Marek,
>>>
>>>>>
>>>>> 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 ?
>>>>
>>>>
>>>
>>> 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.

Can some problem arise from not cutting the power in some situation?

Presumably with this patch the system tries to re-initialize the SPI NOR
after resume from suspend.

Can some SPI NOR memories be programmed into state in which they
would not respond to the commands used for identify?

Perhaps flash memories in dual/quad mode should be switched to plain
mode first?

Thanks

Michal
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Michal Suchanek]

Hello,

On 20 January 2017 at 19:50, Kamal Dasu <kdasu.kdev@gmail.com> wrote:
> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>> Marek,
>>>
>>>>>
>>>>> 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 ?
>>>>
>>>>
>>>
>>> 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.

Can some problem arise from not cutting the power in some situation?

Presumably with this patch the system tries to re-initialize the SPI NOR
after resume from suspend.

Can some SPI NOR memories be programmed into state in which they
would not respond to the commands used for identify?

Perhaps flash memories in dual/quad mode should be switched to plain
mode first?

Thanks

Michal

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 07:50 PM, Kamal Dasu wrote:
> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>> Marek,
>>>
>>>>>
>>>>> 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 ?
>>>>
>>>>
>>>
>>> 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.

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


-- 
Best regards,
Marek Vasut
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 07:50 PM, Kamal Dasu wrote:
> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>> Marek,
>>>
>>>>>
>>>>> 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 ?
>>>>
>>>>
>>>
>>> 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.

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


-- 
Best regards,
Marek Vasut

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Florian Fainelli]

On 01/20/2017 12:02 PM, Marek Vasut wrote:
> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>> Marek,
>>>>
>>>>>>
>>>>>> 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 ?
>>>>>
>>>>>
>>>>
>>>> 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.
> 
> 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.
-- 
Florian
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Florian Fainelli]

On 01/20/2017 12:02 PM, Marek Vasut wrote:
> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>> Marek,
>>>>
>>>>>>
>>>>>> 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 ?
>>>>>
>>>>>
>>>>
>>>> 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.
> 
> 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.
-- 
Florian

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 09:07 PM, Florian Fainelli wrote:
> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>> Marek,
>>>>>
>>>>>>>
>>>>>>> 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 ?
>>>>>>
>>>>>>
>>>>>
>>>>> 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.
>>
>> 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.

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

-- 
Best regards,
Marek Vasut
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 09:07 PM, Florian Fainelli wrote:
> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>> Marek,
>>>>>
>>>>>>>
>>>>>>> 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 ?
>>>>>>
>>>>>>
>>>>>
>>>>> 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.
>>
>> 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.

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

-- 
Best regards,
Marek Vasut

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Florian Fainelli]

On 01/20/2017 12:13 PM, Marek Vasut wrote:
> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>> Marek,
>>>>>>
>>>>>>>>
>>>>>>>> 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 ?
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> 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.
>>>
>>> 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.

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Florian Fainelli]

On 01/20/2017 12:13 PM, Marek Vasut wrote:
> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>> Marek,
>>>>>>
>>>>>>>>
>>>>>>>> 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 ?
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> 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.
>>>
>>> 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.

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

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 09:23 PM, Florian Fainelli wrote:
> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>> Marek,
>>>>>>>
>>>>>>>>>
>>>>>>>>> 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 ?
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>
>>>> 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.

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

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


-- 
Best regards,
Marek Vasut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 09:23 PM, Florian Fainelli wrote:
> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>> Marek,
>>>>>>>
>>>>>>>>>
>>>>>>>>> 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 ?
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>
>>>> 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.

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

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


-- 
Best regards,
Marek Vasut

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

"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

On Fri, Jan 20, 2017 at 3:42 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> On 01/20/2017 09:23 PM, Florian Fainelli wrote:
>> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>>> Marek,
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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 ?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>
>>>>> 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.
>
> 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 ...
>
>>>> 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.
>>
>
>
> --
> Best regards,
> Marek Vasut
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Kamal Dasu]

"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

On Fri, Jan 20, 2017 at 3:42 PM, Marek Vasut <marex@denx.de> wrote:
> On 01/20/2017 09:23 PM, Florian Fainelli wrote:
>> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>>> Marek,
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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 ?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>
>>>>> 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.
>
> 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 ...
>
>>>> 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.
>>
>
>
> --
> Best regards,
> Marek Vasut

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 10:15 PM, Kamal Dasu wrote:
> "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.

Who does guarantee that ?

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

Also please respect the ML netiquette and do NOT top-post.

> Kamal
> 
> On Fri, Jan 20, 2017 at 3:42 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>> On 01/20/2017 09:23 PM, Florian Fainelli wrote:
>>> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>>>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>>>> Marek,
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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 ?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>
>> 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 ...
>>
>>>>> 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.
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 10:15 PM, Kamal Dasu wrote:
> "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.

Who does guarantee that ?

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

Also please respect the ML netiquette and do NOT top-post.

> Kamal
> 
> On Fri, Jan 20, 2017 at 3:42 PM, Marek Vasut <marex@denx.de> wrote:
>> On 01/20/2017 09:23 PM, Florian Fainelli wrote:
>>> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>>>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>>>> Marek,
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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 ?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>
>> 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 ...
>>
>>>>> 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.
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Florian Fainelli]

On 01/20/2017 12:42 PM, Marek Vasut wrote:
> On 01/20/2017 09:23 PM, Florian Fainelli wrote:
>> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>>> Marek,
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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 ?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>
>>>>> 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.
> 
> 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.

> 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?
-- 
Florian
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Florian Fainelli]

On 01/20/2017 12:42 PM, Marek Vasut wrote:
> On 01/20/2017 09:23 PM, Florian Fainelli wrote:
>> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>>> Marek,
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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 ?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>
>>>>> 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.
> 
> 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.

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

Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 11:58 PM, Florian Fainelli wrote:
> On 01/20/2017 12:42 PM, Marek Vasut wrote:
>> On 01/20/2017 09:23 PM, Florian Fainelli wrote:
>>> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>>>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
>>>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>>>> Marek,
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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 ?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>
>> 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.

Probably what I don't like the most about this discussion is the use of
"period" and "it just is" rhetoric.

I don't mind if it takes a bit to get some/any test out, but as I
already stated (and will restate that), this patch is potentially
disruptive and can cause trouble for a lot of people, so I want to
be sure this can and will get a lot of testing.

> 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

I'm not, indeed. It was pretty vague.

> but asking for test
> case is just not common thing

Indeed, please see above (disruptive change).

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

I cannot make any judgment based on the limited amount of code I saw
from Kamal, sorry. It certainly takes time and patches to build any
sort of trust.

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

Let's not mix a test framework into this discussion . My proposition
would be to take a few days off, cool down and revisit the discussion
next week earliest. Agreed ?

-- 
Best regards,
Marek Vasut
--
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 v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut]

On 01/20/2017 11:58 PM, Florian Fainelli wrote:
> On 01/20/2017 12:42 PM, Marek Vasut wrote:
>> On 01/20/2017 09:23 PM, Florian Fainelli wrote:
>>> On 01/20/2017 12:13 PM, Marek Vasut wrote:
>>>> On 01/20/2017 09:07 PM, Florian Fainelli wrote:
>>>>> On 01/20/2017 12:02 PM, Marek Vasut wrote:
>>>>>> On 01/20/2017 07:50 PM, Kamal Dasu wrote:
>>>>>>> On Fri, Jan 20, 2017 at 12:22 PM, Marek Vasut <marex@denx.de> wrote:
>>>>>>>> On 01/20/2017 05:33 PM, Kamal Dasu wrote:
>>>>>>>>> Marek,
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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 ?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>
>> 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.

Probably what I don't like the most about this discussion is the use of
"period" and "it just is" rhetoric.

I don't mind if it takes a bit to get some/any test out, but as I
already stated (and will restate that), this patch is potentially
disruptive and can cause trouble for a lot of people, so I want to
be sure this can and will get a lot of testing.

> 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

I'm not, indeed. It was pretty vague.

> but asking for test
> case is just not common thing

Indeed, please see above (disruptive change).

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

I cannot make any judgment based on the limited amount of code I saw
from Kamal, sorry. It certainly takes time and patches to build any
sort of trust.

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

Let's not mix a test framework into this discussion . My proposition
would be to take a few days off, cool down and revisit the discussion
next week earliest. Agreed ?

-- 
Best regards,
Marek Vasut