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

Message ID 20180829160013.9447-2-m.szyprowski@samsung.com
State Superseded
Headers show
Series
  • Prepare Exynos5433 clocks driver for system suspend/resume
Related show

Commit Message

Marek Szyprowski Aug. 29, 2018, 4 p.m.
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.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 drivers/clk/samsung/clk-exynos5433.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.17.1

Comments

Chanwoo Choi Aug. 29, 2018, 11:21 p.m. | #1
Hi,

On 2018년 08월 30일 01:00, Marek Szyprowski 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.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/clk/samsung/clk-exynos5433.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c

> index 162de44df099..426980514e67 100644

> --- a/drivers/clk/samsung/clk-exynos5433.c

> +++ b/drivers/clk/samsung/clk-exynos5433.c

> @@ -5630,7 +5630,7 @@ static const struct of_device_id exynos5433_cmu_of_match[] = {

>  static const struct dev_pm_ops exynos5433_cmu_pm_ops = {

>  	SET_RUNTIME_PM_OPS(exynos5433_cmu_suspend, exynos5433_cmu_resume,

>  			   NULL)

> -	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,

> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,

>  				     pm_runtime_force_resume)

>  };

>  

> 


The suspend_noirq callback for devices are called right before
entering the suspend and then resume_noirq callback for device
are called right after wakeup from suspend.

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics
Krzysztof Kozlowski Aug. 30, 2018, 6:29 a.m. | #2
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.

Best regards,
Krzysztof


> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/clk/samsung/clk-exynos5433.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c

> index 162de44df099..426980514e67 100644

> --- a/drivers/clk/samsung/clk-exynos5433.c

> +++ b/drivers/clk/samsung/clk-exynos5433.c

> @@ -5630,7 +5630,7 @@ static const struct of_device_id exynos5433_cmu_of_match[] = {

>  static const struct dev_pm_ops exynos5433_cmu_pm_ops = {

>         SET_RUNTIME_PM_OPS(exynos5433_cmu_suspend, exynos5433_cmu_resume,

>                            NULL)

> -       SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,

> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,

>                                      pm_runtime_force_resume)

>  };

>

> --

> 2.17.1

>
Marek Szyprowski Aug. 30, 2018, 9:59 a.m. | #3
Hi Krzysztof,

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.

 > ...


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Krzysztof Kozlowski Aug. 30, 2018, 10:25 a.m. | #4
On Thu, 30 Aug 2018 at 11:59, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>

> Hi Krzysztof,

>

> 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. :)

Best regards,
Krzysztof
Marek Szyprowski Aug. 30, 2018, 10:34 a.m. | #5
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.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Krzysztof Kozlowski Aug. 30, 2018, 10:45 a.m. | #6
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?

Best regards,
Krzysztof
Marek Szyprowski Aug. 30, 2018, 11:12 a.m. | #7
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
Krzysztof Kozlowski Aug. 30, 2018, 1:01 p.m. | #8
On Thu, 30 Aug 2018 at 13:12, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>

>

>

> On 2018-08-30 12:45, Krzysztof Kozlowski wrote:

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


Looks quite fragile... but I understand now that your patch is a
logical solution for existing infrastructure with existing dw-mmc
behavior during suspend.

I wonder whether the device links should be used here (in case of
dw-mmc and clk drivers) and in all clk consumers in general. Something
similar to regulators_get().

Can you extend the commit msg with this clk--dw_mmc scenario?

With that change:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>


Best regards,
Krzysztof

Patch

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index 162de44df099..426980514e67 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -5630,7 +5630,7 @@  static const struct of_device_id exynos5433_cmu_of_match[] = {
 static const struct dev_pm_ops exynos5433_cmu_pm_ops = {
 	SET_RUNTIME_PM_OPS(exynos5433_cmu_suspend, exynos5433_cmu_resume,
 			   NULL)
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
 				     pm_runtime_force_resume)
 };