| Message ID | 1473710494-4084-1-git-send-email-kdasu.kdev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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.
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.
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. >
"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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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?
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 ?
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,
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(-)