Re: [PATCH 1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 2018-08-30 12:45, Krzysztof Kozlowski wrote:
> On Thu, 30 Aug 2018 at 12:34, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> Hi Krzysztof,
>>
>> On 2018-08-30 12:25, Krzysztof Kozlowski wrote:
>>> On Thu, 30 Aug 2018 at 11:59, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>>> On 2018-08-30 08:29, Krzysztof Kozlowski wrote:
>>>>> On Wed, 29 Aug 2018 at 18:00, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>>>>> Clocks should be suspended as late as possible and resumed as early as
>>>>>> possible to let other drivers do their own suspend/resume tasks. NOIRQ
>>>>>> callbacks better suit this requirement.
>>>>> I think that's not a good reason to use the noirq versions. These are
>>>>> to solve the races with interrupt handlers, not to manually order
>>>>> callbacks.
>>>> Then please tell me which other solution should I use to make clock
>>>> available
>>>> on Exynos5433 during NOIRQ suspend/resume phase. dw-mmc driver requires to
>>>> access its clocks in NOIRQ resume.
>>> Indeed I found the usage of noirq in the dw-mmc driver which made me
>>> wondering why it is there... and if you look at explanation, the noirq
>>> is only for the purpose of clearing wakeup interrupt in CLKSEL
>>> register.
>>>
>>> Further code refactoring moved more and more code to suspend_noirq,
>>> including the runtime PM part. This probably should not be part of
>>> suspend_noirq but regular suspend. Then all the need of manual
>>> ordering would go away. So the answer to your question - try fixing
>>> buggy dw-mmc driver. :)
>> It is not the bug in dw-mmc driver. It clearly needs to access some its
>> registers during NOIRQ phase. However accessing registers requires to
>> have clocks enabled, which is being handled by runtime PM. There is
>> nothing broken here.
> Ah I missed that point and I see you fixed that case in "mmc:
> dw_mmc-exynos: fix potential external abort in resume_noirq()". The
> true reasoning for this patch is that soc clk driver should suspend
> after every other suspend callback which is using clocks... It would
> be nice to explain this particular scenario in commit msg.
>
> However both dw-mmc and clk will be now in suspend noirq phase so do
> you have any guarantees that dw-mmc will be suspended after clk?

Frankly speaking this works now, because the devices are populated in
the order of their presence in device-tree. When the order would be
reverse, dw-mmc driver will defer probe until clocks are registered.
This would be enough to ensure proper suspend/resume order, because all
deferred devices are moved to the end of dpm_list (the list of device
used for system suspend/resume calls), see deferred_probe_work_func()
and comments there.

This is however still not fully resolved problem that has to be
addressed one day. Andrzej Hajda will have a speech on this topic at
Open Source Summit:

https://osseu18.sched.com/event/FxYd/deferred-problem-issues-with-complex-dependencies-between-devices-in-linux-kernel-andrzej-hajda-samsung

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland