Re: [PATCH v2, 1/1] mtd: devices: m25p80: Add PM suspend resume support

[Marek Vasut <marek.vasut@xxxxxxxxx>]

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@xxxxxxxxx <mailto:cyrille.pitchen@xxxxxxxxx>> 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 ?


-- 
Best regards,
Marek Vasut
--
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