| Message ID | 20180829160013.9447-2-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] clk: samsung: Use NOIRQ stage for Exynos5433 clocks suspend/resume | expand |
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>
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 >
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
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
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
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
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
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
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) };
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(-)