| Message ID | 1487791173-22159-3-git-send-email-kdasu.kdev@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Kamal, Le 22/02/2017 à 20:19, Kamal Dasu a écrit : > Added power management ops for resume to be able to reinitialize > spi-nor device and set it to right transfer mode in its pre-suspend > state. Some SoC implementations might power down the spi-nor flash > and loose its initial settings on suspend. A resume should restore > part settings to its original pre-suspend state. > > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > --- > drivers/mtd/devices/m25p80.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index c4df3b1..3ab30b2 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -324,10 +324,21 @@ static int m25p_remove(struct spi_device *spi) > }; > MODULE_DEVICE_TABLE(of, m25p_of_table); > > +#ifdef CONFIG_PM_SLEEP > +static int m25p_resume(struct device *dev) > +{ > + struct m25p *flash = dev_get_drvdata(dev); > + > + return spi_nor_init(&flash->spi_nor); > +} > +#endif > +static SIMPLE_DEV_PM_OPS(m25p_pm_ops, NULL, 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, > I'm still studying the runtime_pm documentation and source code to understand how the power management works in the Linux kernel. I didn't finish but for what I have understood until now, I think there is an issue. Here you add suspend/resume handlers on the SPI device. So the SPI sub-system is aware of the power state of the SPI flash memory, that fine. However what about the MTD sub-system? I don't see any synchronization between the SPI device and the MTD device. Hence I guess the MTD sub-system is not aware of the actual power state of the hardware memory. So I think that mtd->_read() or mtd->_write() handlers could be called from some mtd driver when the SPI device has already been suspended. For instance, let's image the root file-system is mounted from a ubifs stored a SPI NOR flash: what if the kernel tries to perform some file access when the SPI device has already been suspended? The 'struct m25p' instance makes the link between the SPI device the the spi_nor->mtd device. Sync could be done using this object. That why I think this patch is currently incomplete as the synchronization of the power states of both the SPI and MTD devices is missing. I think the feature you're trying to implement is interesting but some rework seems to needed. I can't tell you more for now since, as I said, I'm still lacking strong knowledge about the runtime_pm framework. Best regards, Cyrille
On Wed, Feb 22, 2017 at 3:38 PM, Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> wrote: > Hi Kamal, > > Le 22/02/2017 à 20:19, Kamal Dasu a écrit : >> Added power management ops for resume to be able to reinitialize >> spi-nor device and set it to right transfer mode in its pre-suspend >> state. Some SoC implementations might power down the spi-nor flash >> and loose its initial settings on suspend. A resume should restore >> part settings to its original pre-suspend state. >> >> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> >> --- >> drivers/mtd/devices/m25p80.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c >> index c4df3b1..3ab30b2 100644 >> --- a/drivers/mtd/devices/m25p80.c >> +++ b/drivers/mtd/devices/m25p80.c >> @@ -324,10 +324,21 @@ static int m25p_remove(struct spi_device *spi) >> }; >> MODULE_DEVICE_TABLE(of, m25p_of_table); >> >> +#ifdef CONFIG_PM_SLEEP >> +static int m25p_resume(struct device *dev) >> +{ >> + struct m25p *flash = dev_get_drvdata(dev); >> + >> + return spi_nor_init(&flash->spi_nor); >> +} >> +#endif >> +static SIMPLE_DEV_PM_OPS(m25p_pm_ops, NULL, 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, >> > > I'm still studying the runtime_pm documentation and source code to > understand how the power management works in the Linux kernel. > I didn't finish but for what I have understood until now, I think there > is an issue. > > Here you add suspend/resume handlers on the SPI device. So the SPI > sub-system is aware of the power state of the SPI flash memory, that > fine. However what about the MTD sub-system? I don't see any > synchronization between the SPI device and the MTD device. Hence I guess > the MTD sub-system is not aware of the actual power state of the > hardware memory. So I think that mtd->_read() or mtd->_write() handlers > could be called from some mtd driver when the SPI device has already > been suspended. For instance, let's image the root file-system is > mounted from a ubifs stored a SPI NOR flash: what if the kernel tries to > perform some file access when the SPI device has already been suspended? > However in the current stack based on spi master bus driver and m25p80 flash device the spi-bcm-qspi does call the spi_master_suspend(), which stops the queue so there should not be any activity. spi-nor may implement something on the lines of the nand_base where it simply sets a state and locks the device in a certain state using nand_get_device() call. But does not actually do any thing with the mtd structures as far as I can tell. mtd->_suspend = spi_nor_suspend; mtd->_resume = spi_nor_resume; mtd->_reboot = spi_nor_shutdown; I am not sure what spi_nor_suspend() or spi_nor_shutdown() will actually do. As mtd layer is just an abstraction in memory and does not change state. But spi-nor can be made aware of the states by maintaining it in the nor structure. Both spi and m25p80 and the controller driver are aware. > The 'struct m25p' instance makes the link between the SPI device the the > spi_nor->mtd device. Sync could be done using this object. > > That why I think this patch is currently incomplete as the > synchronization of the power states of both the SPI and MTD devices is > missing. > > I think the feature you're trying to implement is interesting but some > rework seems to needed. I can't tell you more for now since, as I said, > I'm still lacking strong knowledge about the runtime_pm framework. > Ideally spi-nor driver should handle the mtd ops I thought fro the framework. > Best regards, > > Cyrille Thanks Kamal
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index c4df3b1..3ab30b2 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -324,10 +324,21 @@ static int m25p_remove(struct spi_device *spi) }; MODULE_DEVICE_TABLE(of, m25p_of_table); +#ifdef CONFIG_PM_SLEEP +static int m25p_resume(struct device *dev) +{ + struct m25p *flash = dev_get_drvdata(dev); + + return spi_nor_init(&flash->spi_nor); +} +#endif +static SIMPLE_DEV_PM_OPS(m25p_pm_ops, NULL, 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,
Added power management ops for resume to be able to reinitialize spi-nor device and set it to right transfer mode in its pre-suspend state. Some SoC implementations might power down the spi-nor flash and loose its initial settings on suspend. A resume should restore part settings to its original pre-suspend state. Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> --- drivers/mtd/devices/m25p80.c | 11 +++++++++++ 1 file changed, 11 insertions(+)