| Message ID | 1486164676-12912-4-git-send-email-kdasu.kdev@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Cyrille Pitchen |
| Headers | show |
On Fri, Feb 03, 2017 at 06:31:14PM -0500, Kamal Dasu wrote: > Device drivers can check if the master controller driver has to support > flash pm. The controller driver indicates this using flash_pm_supported() > spi master interface. What is "flash pm" and how would a client use it given that no API for actually managing power is being added here? Someone looking at the API needs to be able to figure these things out and right now I can't see how they'd do that...
On Sat, Feb 4, 2017 at 6:25 AM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Feb 03, 2017 at 06:31:14PM -0500, Kamal Dasu wrote: > >> Device drivers can check if the master controller driver has to support >> flash pm. The controller driver indicates this using flash_pm_supported() >> spi master interface. > > What is "flash pm" and how would a client use it given that no API for > actually managing power is being added here? Someone looking at the API > needs to be able to figure these things out and right now I can't see > how they'd do that...o The flash_pm function just indicates if m25p80 resume op that does a spi_nor_pm_rescan() is needed, and by default will do nothing for other platforms. So the basic idea of the patches was to execute the only necessary spi_nor_pm_rescan() on resume when flash parts are reset on suspend. So from controller perspective there is no additional pm activity to be done. Patches 1/5-2/5 is what is needed actually. And Patches 3/3 - 5/5 only add a check if m25p80 resume should call spi_nor_pm_rescan(). And the controller driver shall implement the flash_pm function and return true if it does need a rescan to be done on resume.
Hi all, Le 04/02/2017 à 21:47, Kamal Dasu a écrit : > On Sat, Feb 4, 2017 at 6:25 AM, Mark Brown <broonie@kernel.org> wrote: >> On Fri, Feb 03, 2017 at 06:31:14PM -0500, Kamal Dasu wrote: >> >>> Device drivers can check if the master controller driver has to support >>> flash pm. The controller driver indicates this using flash_pm_supported() >>> spi master interface. >> >> What is "flash pm" and how would a client use it given that no API for >> actually managing power is being added here? Someone looking at the API >> needs to be able to figure these things out and right now I can't see >> how they'd do that...o > > The flash_pm function just indicates if m25p80 resume op that does a > spi_nor_pm_rescan() is needed, and by default will do nothing for > other platforms. So the basic idea of the patches was to execute the > only necessary spi_nor_pm_rescan() on resume when flash parts are > reset on suspend. So from controller perspective there is no > additional pm activity to be done. Patches 1/5-2/5 is what is needed > actually. And Patches 3/3 - 5/5 only add a check if m25p80 resume > should call spi_nor_pm_rescan(). And the controller driver shall > implement the flash_pm function and return true if it does need a > rescan to be done on resume. > I don't understand why we extend in the SPI framework API to add power management features but only for SPI flashes. I guess concerning the power management, there is nothing special about SPI flashes: they are just SPI devices like others. So why not extend the SPI framework, if needed, with something working for all SPI devices, not just for SPI flashes. There was a good reason to create a flash specific API in the SPI framework: spi_flash_read(), flash_read_supported(). The reason is that the SPI controller needs protocol info (op code, address, dummy cycles, SPI x-y-z protocol, ...) to handle the read operation correctly and those pieces of information were lost in m25p80.c when it used the regular SPI API with spi_transfer and spi_message structures. Hence spi_flash_read() is a mean to bypass the regular SPI API and provide the SPI controller driver with all the protocol info it needs. Back to the power management use case, I don't see any technical reason to create a SPI flash oriented solution. Best regards, Cyrille
On Mon, Feb 06, 2017 at 11:44:09AM +0100, Cyrille Pitchen wrote: > Le 04/02/2017 à 21:47, Kamal Dasu a écrit : > >> What is "flash pm" and how would a client use it given that no API for > >> actually managing power is being added here? Someone looking at the API > >> needs to be able to figure these things out and right now I can't see > >> how they'd do that...o > > The flash_pm function just indicates if m25p80 resume op that does a > > spi_nor_pm_rescan() is needed, and by default will do nothing for > > other platforms. So the basic idea of the patches was to execute the > > only necessary spi_nor_pm_rescan() on resume when flash parts are > > reset on suspend. So from controller perspective there is no I can't tell what this means, sorry, and writing this e-mail does not address the incomprehensibility of the API. Why would the controller driver have anything to do with whatever this spi_nor_pm_rescan() operation does? > I don't understand why we extend in the SPI framework API to add power > management features but only for SPI flashes. I guess concerning the power > management, there is nothing special about SPI flashes: they are just SPI > devices like others. So why not extend the SPI framework, if needed, with > something working for all SPI devices, not just for SPI flashes. It's already possible for SPI devices to do runtime PM.
Patches 3-5 are not needed. I will refactor patch 1-2 and send a v2 version. Thanks Kamal On Mon, Feb 6, 2017 at 11:46 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Feb 06, 2017 at 11:44:09AM +0100, Cyrille Pitchen wrote: >> Le 04/02/2017 à 21:47, Kamal Dasu a écrit : > >> >> What is "flash pm" and how would a client use it given that no API for >> >> actually managing power is being added here? Someone looking at the API >> >> needs to be able to figure these things out and right now I can't see >> >> how they'd do that...o > >> > The flash_pm function just indicates if m25p80 resume op that does a >> > spi_nor_pm_rescan() is needed, and by default will do nothing for >> > other platforms. So the basic idea of the patches was to execute the >> > only necessary spi_nor_pm_rescan() on resume when flash parts are >> > reset on suspend. So from controller perspective there is no > > I can't tell what this means, sorry, and writing this e-mail does not > address the incomprehensibility of the API. Why would the controller > driver have anything to do with whatever this spi_nor_pm_rescan() > operation does? > >> I don't understand why we extend in the SPI framework API to add power >> management features but only for SPI flashes. I guess concerning the power >> management, there is nothing special about SPI flashes: they are just SPI >> devices like others. So why not extend the SPI framework, if needed, with >> something working for all SPI devices, not just for SPI flashes. > > It's already possible for SPI devices to do runtime PM.
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 75c6bd0..b5fbc1b 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -539,6 +539,7 @@ struct spi_master { int (*spi_flash_read)(struct spi_device *spi, struct spi_flash_read_message *msg); bool (*flash_read_supported)(struct spi_device *spi); + bool (*flash_pm_supported)(struct spi_device *spi); /* * These hooks are for drivers that use a generic implementation @@ -1185,6 +1186,13 @@ static inline bool spi_flash_read_supported(struct spi_device *spi) spi->master->flash_read_supported(spi)); } +/* SPI core interface to indicate flash pm support */ +static inline bool spi_flash_pm_supported(struct spi_device *spi) +{ + return (spi->master->flash_pm_supported && + spi->master->flash_pm_supported(spi)); +} + int spi_flash_read(struct spi_device *spi, struct spi_flash_read_message *msg);
Device drivers can check if the master controller driver has to support flash pm. The controller driver indicates this using flash_pm_supported() spi master interface. Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> --- include/linux/spi/spi.h | 8 ++++++++ 1 file changed, 8 insertions(+)