Re: [PATCH RFC 2/4] mmc: sdio: Add capability to skip SDIO reset at scan

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 04/25/2017 07:45 AM, Adrian Hunter wrote:
> On 25/04/17 15:24, Ulf Hansson wrote:
>> On 25 April 2017 at 13:20, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 25/04/17 13:46, Ulf Hansson wrote:
>>>> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 25/04/17 10:52, Ulf Hansson wrote:
>>>>>> +Grygorii Strashko
>>>>>>
>>>>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>> On 24/04/17 23:33, Ulf Hansson wrote:
>>>>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>>>> The SDIO card state might be being preserved during hibernation, for
>>>>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an
>>>>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a
>>>>>>>>> module and simply not load it until after attempting to restore the
>>>>>>>>> hibernation image. However that won't work if the hibernation image is
>>>>>>>>> stored on eMMC which, of course, requires mmc core.
>>>>>>>>
>>>>>>>> I don't follow here. Are you saying the SDIO card is kept powered in
>>>>>>>> hibernation, as to be able to support WOWLAN, right?
>>>>>>>
>>>>>>> Yes
>>>>>>
>>>>>> Okay, makes sense!
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it.
>>>>>>>> That should never happen, unless something is broken of course.
>>>>>>>
>>>>>>> The thing to note about hibernation is that there is a regular boot in
>>>>>>> between saving the hibernation image and restoring it again.  At boot time,
>>>>>>> the kernel knows almost nothing about whether there is a hibernation image
>>>>>>> and whether or not it will be restored.  Consequently it becomes difficult
>>>>>>> to avoid mmc_rescan().  As mentioned above, we need mmc_rescan() to
>>>>>>> initialize the eMMC so that the hibernation image can be read.
>>>>>>
>>>>>> What's wrong with using the hibernation callbacks in the struct
>>>>>> dev_pm_ops? We already do this.
>>>>>
>>>>> Here is the scenario.  The kernel has just booted.  User space wants to try
>>>>> to restore a hibernation image, if there is one.  So user space loads the
>>>>> mmc core because the hibernation image is on eMMC.  Mmc core does an SDIO
>>>>> reset on the SDIO card and the state is lost.  It has little to do with pm
>>>>> callbacks AFAICS.
>>>>
>>>> Ah, now I see what you mean. I thought the problem was during the
>>>> actual restoring of the hibernation image.
>>>>
>>>> Alright, when a boot is triggered by WOWLAN , you want to avoid
>>>> sending the reset command for the SDIO card before the
>>>> re-initialization of the SDIO card starts.
>>>>
>>>> The problem with this approach is that you can't differentiate between
>>>> a cold boot and a boot triggered by WOWLAN, right!? In other words, in
>>>> some cases the reset command may be needed while in other it won't.
>>>
>>> SDIO reset is only needed if the card has not been power-cycled.  The
>>> assumption is that the platform takes care of that when needed. e.g. when
>>> rebooting instead of going to S4.
>>>
>>>>
>>>> Maybe you can elaborate more on what exactly what the problem is with
>>>> sending the reset command when the boot is triggered from WOWLAN? Yes,
>>>> the SDIO card loses its context, but how is that a problem?
>>>
>>> The wifi driver expects to find the function in an initialized state.
>>
>> So how does the the wifi driver distinguish this a boot caused by
>> WOWLAN - and not a cold boot?
> 
> It doesn't have to.  It doesn't get loaded until after the attempt to
> restore the hibernation image.  So in the hibernation case it has its
> ->restore() callback.  

For this case is a good question of how Kernel wifi card driver state will be
synchronized HW state after restore from Hibernation (first of all wireless state -
scan results, connection state and parameters)? Most probably wifi driver will
need to perform mostly full re-initialization on restore, so...

>If there is no hibernation image, it gets loaded and
> probed.

But in this case, as per you patch, there are will be no SDIO reset so
do you expect that wifi driver will still work? 

> 
> AFAICT that is the normal way to stop drivers interfering with hibernation
> i.e. don't load them until after the attempt is made to restore the
> hibernation image.  We could do that with mmc core too, but for the fact
> that the hibernation image is on eMMC.

>From my experience, it pretty hard to restore HW state which runs by FW, so as
W/A we just unloaded remoteproc, wireless and graphic drivers before hibernation and
reloaded them right after. As I know the same approach often used in x86 world also.


> 
>>
>>> Otherwise it would have to re-enable the function and re-do the
>>> function-specific initialization.  I don't know if there are other
>>
>> At boot the SDIO func driver becomes probed, when the mmc core finds
>> and SDIO card and then registers a func device for it. Are you saying
>> that the SDIO func driver can take shortcuts when enabling the func
>> device, when the boot is trigger from WOWLAN?
> 
> No.
> 
>>
>>> consequences.  Presumably it will have lost any information about the nature
>>> of the wake-up trigger.
>>
>> How does that matter?
> 
> I don't know if it matters.
> 

-- 
regards,
-grygorii