Re: [PATCH v5 2/2] mtd: m25p80: Added pm ops support

[Cyrille Pitchen <cyrille.pitchen@xxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html