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

[Kamal Dasu <kamal.dasu@xxxxxxxxxxxx>]

On Wed, Feb 22, 2017 at 3:38 PM, Cyrille Pitchen
<cyrille.pitchen@xxxxxxxxxx> 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@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?
>

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