Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
this clock controller needs runtime PM for its operations.
Also do a runtime PM get on the clock controller during the
probing stage to workaround a possible deadlock.
Signed-off-by: Pin-yen Lin <[email protected]>
---
Changes in v3:
- Update the commit message and the comments before runtime PM call
Changes in v2:
- Fix the order of error handling
- Update the commit message and add a comment before the runtime PM call
drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
drivers/clk/mediatek/clk-mtk.h | 2 ++
2 files changed, 21 insertions(+)
diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
index 2e55368dc4d8..ba1d1c495bc2 100644
--- a/drivers/clk/mediatek/clk-mtk.c
+++ b/drivers/clk/mediatek/clk-mtk.c
@@ -13,6 +13,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include "clk-mtk.h"
@@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
}
+
+ if (mcd->need_runtime_pm) {
+ devm_pm_runtime_enable(&pdev->dev);
+ /*
+ * Do a pm_runtime_resume_and_get() to workaround a possible
+ * deadlock between clk_register() and the genpd framework.
+ */
+ r = pm_runtime_resume_and_get(&pdev->dev);
+ if (r)
+ return r;
+ }
+
/* Calculate how many clk_hw_onecell_data entries to allocate */
num_clks = mcd->num_clks + mcd->num_composite_clks;
num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
@@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
goto unregister_clks;
}
+ if (mcd->need_runtime_pm)
+ pm_runtime_put(&pdev->dev);
+
return r;
unregister_clks:
@@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
free_base:
if (mcd->shared_io && base)
iounmap(base);
+
+ if (mcd->need_runtime_pm)
+ pm_runtime_put(&pdev->dev);
return r;
}
diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
index 22096501a60a..c17fe1c2d732 100644
--- a/drivers/clk/mediatek/clk-mtk.h
+++ b/drivers/clk/mediatek/clk-mtk.h
@@ -237,6 +237,8 @@ struct mtk_clk_desc {
int (*clk_notifier_func)(struct device *dev, struct clk *clk);
unsigned int mfg_clk_idx;
+
+ bool need_runtime_pm;
};
int mtk_clk_pdev_probe(struct platform_device *pdev);
--
2.43.0.472.g3155946c3a-goog
mt8183-mfgcfg has a mutual dependency with genpd during the probing
stage, so enable need_runtim_pm to prevent a deadlock in the following
call stack:
CPU0: genpd_lock --> clk_prepare_lock
genpd_power_off_work_fn()
genpd_lock()
generic_pm_domain::power_off()
clk_unprepare()
clk_prepare_lock()
CPU1: clk_prepare_lock --> genpd_lock
clk_register()
__clk_core_init()
clk_prepare_lock()
clk_pm_runtime_get()
genpd_lock()
Do a runtime PM get at the probe function to make sure clk_register()
won't acquire the genpd lock.
Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
Signed-off-by: Pin-yen Lin <[email protected]>
---
(no changes since v1)
drivers/clk/mediatek/clk-mt8183-mfgcfg.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
index ba504e19d420..62d876e150e1 100644
--- a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
+++ b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
@@ -29,6 +29,7 @@ static const struct mtk_gate mfg_clks[] = {
static const struct mtk_clk_desc mfg_desc = {
.clks = mfg_clks,
.num_clks = ARRAY_SIZE(mfg_clks),
+ .need_runtime_pm = true,
};
static const struct of_device_id of_match_clk_mt8183_mfg[] = {
--
2.43.0.472.g3155946c3a-goog
On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
>
> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
> this clock controller needs runtime PM for its operations.
> Also do a runtime PM get on the clock controller during the
> probing stage to workaround a possible deadlock.
>
> Signed-off-by: Pin-yen Lin <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
The patch itself looks fine.
Besides the MT8183 MFG clock issues, we do actually need this for the
MT8192 ADSP clock. Its power domain is not enabled by default.
> ---
>
> Changes in v3:
> - Update the commit message and the comments before runtime PM call
>
> Changes in v2:
> - Fix the order of error handling
> - Update the commit message and add a comment before the runtime PM call
>
> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
> drivers/clk/mediatek/clk-mtk.h | 2 ++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> index 2e55368dc4d8..ba1d1c495bc2 100644
> --- a/drivers/clk/mediatek/clk-mtk.c
> +++ b/drivers/clk/mediatek/clk-mtk.c
> @@ -13,6 +13,7 @@
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/slab.h>
>
> #include "clk-mtk.h"
> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
> }
>
> +
> + if (mcd->need_runtime_pm) {
> + devm_pm_runtime_enable(&pdev->dev);
> + /*
> + * Do a pm_runtime_resume_and_get() to workaround a possible
> + * deadlock between clk_register() and the genpd framework.
> + */
> + r = pm_runtime_resume_and_get(&pdev->dev);
> + if (r)
> + return r;
> + }
> +
> /* Calculate how many clk_hw_onecell_data entries to allocate */
> num_clks = mcd->num_clks + mcd->num_composite_clks;
> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> goto unregister_clks;
> }
>
> + if (mcd->need_runtime_pm)
> + pm_runtime_put(&pdev->dev);
> +
> return r;
>
> unregister_clks:
> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> free_base:
> if (mcd->shared_io && base)
> iounmap(base);
> +
> + if (mcd->need_runtime_pm)
> + pm_runtime_put(&pdev->dev);
> return r;
> }
>
> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> index 22096501a60a..c17fe1c2d732 100644
> --- a/drivers/clk/mediatek/clk-mtk.h
> +++ b/drivers/clk/mediatek/clk-mtk.h
> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
>
> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
> unsigned int mfg_clk_idx;
> +
> + bool need_runtime_pm;
> };
>
> int mtk_clk_pdev_probe(struct platform_device *pdev);
> --
> 2.43.0.472.g3155946c3a-goog
>
On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
>
> mt8183-mfgcfg has a mutual dependency with genpd during the probing
> stage, so enable need_runtim_pm to prevent a deadlock in the following
> call stack:
>
> CPU0: genpd_lock --> clk_prepare_lock
> genpd_power_off_work_fn()
> genpd_lock()
> generic_pm_domain::power_off()
> clk_unprepare()
> clk_prepare_lock()
>
> CPU1: clk_prepare_lock --> genpd_lock
> clk_register()
> __clk_core_init()
> clk_prepare_lock()
> clk_pm_runtime_get()
> genpd_lock()
>
> Do a runtime PM get at the probe function to make sure clk_register()
> won't acquire the genpd lock.
>
> Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
> Signed-off-by: Pin-yen Lin <[email protected]>
Reviewed-by: Chen-Yu Tsai <[email protected]>
Note that this compliments a patch [1] adding the power domain for the mfgcfg
clock controller node, which has been floating around for almost 3 years.
[1] https://lore.kernel.org/linux-mediatek/[email protected]/
> ---
>
> (no changes since v1)
>
> drivers/clk/mediatek/clk-mt8183-mfgcfg.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
> index ba504e19d420..62d876e150e1 100644
> --- a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
> +++ b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
> @@ -29,6 +29,7 @@ static const struct mtk_gate mfg_clks[] = {
> static const struct mtk_clk_desc mfg_desc = {
> .clks = mfg_clks,
> .num_clks = ARRAY_SIZE(mfg_clks),
> + .need_runtime_pm = true,
> };
>
> static const struct of_device_id of_match_clk_mt8183_mfg[] = {
> --
> 2.43.0.472.g3155946c3a-goog
>
On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
> > On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
> >>
> >> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
> >> this clock controller needs runtime PM for its operations.
> >> Also do a runtime PM get on the clock controller during the
> >> probing stage to workaround a possible deadlock.
> >>
> >> Signed-off-by: Pin-yen Lin <[email protected]>
> >
> > Reviewed-by: Chen-Yu Tsai <[email protected]>
> >
> > The patch itself looks fine.
> >
> > Besides the MT8183 MFG clock issues, we do actually need this for the
> > MT8192 ADSP clock. Its power domain is not enabled by default.
> >
>
> ...but on MT8195 the ADSP clock works - because the ADSP node exists.
That's an indirect dependency that should not be relied on. Say the clock
driver probed but the ADSP hasn't, and you try to read out the current
status. What would happen?
- Read out works fine, because the power domain is default on, and hasn't
been turned off by late cleanup
- Read out is bogus (but you can't tell)
- Read out hangs.
The third is what happens on MT8192. There's still some issues on that
front, as even after I applied the ADSP power domain patches from MediaTek,
the readout was still hanging.
> This poses a question: should we make clock controllers depend on power domains,
> or should we keep everything powered off (hence clocks down - no power consumption)
> *unless* the user exists?
That's a policy discussion separate from actual hardware dependencies.
*If* the clock controller needs the power domain to be active for the
registers to be accessed, the clock controller *must* have a direct
dependency on the power domain.
> For the second one, this means that the *device* gets the power domain (adsp), and
> not the clock controller (which clocks are effectively useless if there's no user).
No. See my previous paragraph.
ChenYu
> Angelo
>
> >> ---
> >>
> >> Changes in v3:
> >> - Update the commit message and the comments before runtime PM call
> >>
> >> Changes in v2:
> >> - Fix the order of error handling
> >> - Update the commit message and add a comment before the runtime PM call
> >>
> >> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
> >> drivers/clk/mediatek/clk-mtk.h | 2 ++
> >> 2 files changed, 21 insertions(+)
> >>
> >> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> >> index 2e55368dc4d8..ba1d1c495bc2 100644
> >> --- a/drivers/clk/mediatek/clk-mtk.c
> >> +++ b/drivers/clk/mediatek/clk-mtk.c
> >> @@ -13,6 +13,7 @@
> >> #include <linux/of.h>
> >> #include <linux/of_address.h>
> >> #include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> #include <linux/slab.h>
> >>
> >> #include "clk-mtk.h"
> >> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
> >> }
> >>
> >> +
> >> + if (mcd->need_runtime_pm) {
> >> + devm_pm_runtime_enable(&pdev->dev);
> >> + /*
> >> + * Do a pm_runtime_resume_and_get() to workaround a possible
> >> + * deadlock between clk_register() and the genpd framework.
> >> + */
> >> + r = pm_runtime_resume_and_get(&pdev->dev);
> >> + if (r)
> >> + return r;
> >> + }
> >> +
> >> /* Calculate how many clk_hw_onecell_data entries to allocate */
> >> num_clks = mcd->num_clks + mcd->num_composite_clks;
> >> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
> >> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >> goto unregister_clks;
> >> }
> >>
> >> + if (mcd->need_runtime_pm)
> >> + pm_runtime_put(&pdev->dev);
> >> +
> >> return r;
> >>
> >> unregister_clks:
> >> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >> free_base:
> >> if (mcd->shared_io && base)
> >> iounmap(base);
> >> +
> >> + if (mcd->need_runtime_pm)
> >> + pm_runtime_put(&pdev->dev);
> >> return r;
> >> }
> >>
> >> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> >> index 22096501a60a..c17fe1c2d732 100644
> >> --- a/drivers/clk/mediatek/clk-mtk.h
> >> +++ b/drivers/clk/mediatek/clk-mtk.h
> >> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
> >>
> >> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
> >> unsigned int mfg_clk_idx;
> >> +
> >> + bool need_runtime_pm;
> >> };
> >>
> >> int mtk_clk_pdev_probe(struct platform_device *pdev);
> >> --
> >> 2.43.0.472.g3155946c3a-goog
> >>
>
>
>
Il 29/02/24 11:34, Chen-Yu Tsai ha scritto:
> On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno
> <[email protected]> wrote:
>>
>> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto:
>>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno
>>> <[email protected]> wrote:
>>>>
>>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
>>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
>>>>>>
>>>>>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
>>>>>> this clock controller needs runtime PM for its operations.
>>>>>> Also do a runtime PM get on the clock controller during the
>>>>>> probing stage to workaround a possible deadlock.
>>>>>>
>>>>>> Signed-off-by: Pin-yen Lin <[email protected]>
>>>>>
>>>>> Reviewed-by: Chen-Yu Tsai <[email protected]>
>>>>>
>>>>> The patch itself looks fine.
>>>>>
>>>>> Besides the MT8183 MFG clock issues, we do actually need this for the
>>>>> MT8192 ADSP clock. Its power domain is not enabled by default.
>>>>>
>>>>
>>>> ...but on MT8195 the ADSP clock works - because the ADSP node exists.
>>>
>>> That's an indirect dependency that should not be relied on. Say the clock
>>> driver probed but the ADSP hasn't, and you try to read out the current
>>> status. What would happen?
>>>
>>> - Read out works fine, because the power domain is default on, and hasn't
>>> been turned off by late cleanup
>>> - Read out is bogus (but you can't tell)
>>> - Read out hangs.
>>>
>>> The third is what happens on MT8192. There's still some issues on that
>>> front, as even after I applied the ADSP power domain patches from MediaTek,
>>> the readout was still hanging.
>>>
>>
>> That MT8192 lockup story is getting crazy in my head... anyway, besides that,
>> I get the point - I was somehow ignoring the fact that kernel modules do exist.
>>
>> Eh, sorry about that :-)
>>
>>>> This poses a question: should we make clock controllers depend on power domains,
>>>> or should we keep everything powered off (hence clocks down - no power consumption)
>>>> *unless* the user exists?
>>>
>>> That's a policy discussion separate from actual hardware dependencies.
>>> *If* the clock controller needs the power domain to be active for the
>>> registers to be accessed, the clock controller *must* have a direct
>>> dependency on the power domain.
>>>
>>
>> I admit I should've worded that better.
>>
>> "should we make clock controllers depend on power domains" was actually implying
>> "IF those need one" :-)
>>
>> I really wonder if - at this point - it's simply a better idea to not restrict
>> the call to devm_pm_runtime_enable/resume_and_get to `need_runtime_pm == true`.
>>
>> Do we really need to exclude that on other clock controllers that don't have
>> any power domain dependency? Any side effect?
>>
>> Saying this because if we can avoid yet another per-SoC flag I'm really happy,
>> as readability is also impacted and besides - if we ever find out that one of
>> those need a power domain in the future, we'll need just one commit and just
>> only in the devicetree, instead of enabling a flag in driver X as well as that,
>> avoiding some (potentially unnecessary) noise... I guess.
>>
>> P.S.: I just noticed that the return value for the devm_pm_runtime_enable() call
>> is not being checked!
>>
>> .......
>>
>> In short....
>>
>> Chen-Yu, at this point, do you have any reason why we wouldn't be able and/or it
>> wouldn't be a good idea to just avoid adding the `need_runtime_pm` flag (meaning
>> that we perform pm_runtime calls for all clock drivers unconditionally)?
>>
>> If this is about longer boot time, I don't think that it's going to be more than
>> a millisecond or two, so that should be completely ignorable.
>
> I think it's just more of a "don't enable features you don't need" thing.
> We already ran into a weird deadlock, which is why the devm_pm_runtime_enable()
> call has that comment.
>
> I don't think anyone has actually looked at it. As you said it shouldn't be
> much, at least during boot time. It's one call per clock controller.
>
>> Can you please do a test for that, or should I?
>
> The earliest I can work on it would be some time next week. Does that work
> for you?
>
The earliest I'd be able to work on this myself would be at the end of next
week if not later.. so yes, please take your time, no worries.
Thank you!
> ChenYu
>
>> Cheers
>> Angelo
>>
>>>> For the second one, this means that the *device* gets the power domain (adsp), and
>>>> not the clock controller (which clocks are effectively useless if there's no user).
>>>
>>> No. See my previous paragraph.
>>>
>>> ChenYu
>>>
>>>> Angelo
>>>>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3:
>>>>>> - Update the commit message and the comments before runtime PM call
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Fix the order of error handling
>>>>>> - Update the commit message and add a comment before the runtime PM call
>>>>>>
>>>>>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
>>>>>> drivers/clk/mediatek/clk-mtk.h | 2 ++
>>>>>> 2 files changed, 21 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
>>>>>> index 2e55368dc4d8..ba1d1c495bc2 100644
>>>>>> --- a/drivers/clk/mediatek/clk-mtk.c
>>>>>> +++ b/drivers/clk/mediatek/clk-mtk.c
>>>>>> @@ -13,6 +13,7 @@
>>>>>> #include <linux/of.h>
>>>>>> #include <linux/of_address.h>
>>>>>> #include <linux/platform_device.h>
>>>>>> +#include <linux/pm_runtime.h>
>>>>>> #include <linux/slab.h>
>>>>>>
>>>>>> #include "clk-mtk.h"
>>>>>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>>>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
>>>>>> }
>>>>>>
>>>>>> +
>>>>>> + if (mcd->need_runtime_pm) {
>>>>>> + devm_pm_runtime_enable(&pdev->dev);
>>>>>> + /*
>>>>>> + * Do a pm_runtime_resume_and_get() to workaround a possible
>>>>>> + * deadlock between clk_register() and the genpd framework.
>>>>>> + */
>>>>>> + r = pm_runtime_resume_and_get(&pdev->dev);
>>>>>> + if (r)
>>>>>> + return r;
>>>>>> + }
>>>>>> +
>>>>>> /* Calculate how many clk_hw_onecell_data entries to allocate */
>>>>>> num_clks = mcd->num_clks + mcd->num_composite_clks;
>>>>>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
>>>>>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>>>> goto unregister_clks;
>>>>>> }
>>>>>>
>>>>>> + if (mcd->need_runtime_pm)
>>>>>> + pm_runtime_put(&pdev->dev);
>>>>>> +
>>>>>> return r;
>>>>>>
>>>>>> unregister_clks:
>>>>>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>>>> free_base:
>>>>>> if (mcd->shared_io && base)
>>>>>> iounmap(base);
>>>>>> +
>>>>>> + if (mcd->need_runtime_pm)
>>>>>> + pm_runtime_put(&pdev->dev);
>>>>>> return r;
>>>>>> }
>>>>>>
>>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
>>>>>> index 22096501a60a..c17fe1c2d732 100644
>>>>>> --- a/drivers/clk/mediatek/clk-mtk.h
>>>>>> +++ b/drivers/clk/mediatek/clk-mtk.h
>>>>>> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
>>>>>>
>>>>>> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
>>>>>> unsigned int mfg_clk_idx;
>>>>>> +
>>>>>> + bool need_runtime_pm;
>>>>>> };
>>>>>>
>>>>>> int mtk_clk_pdev_probe(struct platform_device *pdev);
>>>>>> --
>>>>>> 2.43.0.472.g3155946c3a-goog
>>>>>>
>>>>
>>>>
>>>>
>>
>>
>>
On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto:
> > On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno
> > <[email protected]> wrote:
> >>
> >> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
> >>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
> >>>>
> >>>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
> >>>> this clock controller needs runtime PM for its operations.
> >>>> Also do a runtime PM get on the clock controller during the
> >>>> probing stage to workaround a possible deadlock.
> >>>>
> >>>> Signed-off-by: Pin-yen Lin <[email protected]>
> >>>
> >>> Reviewed-by: Chen-Yu Tsai <[email protected]>
> >>>
> >>> The patch itself looks fine.
> >>>
> >>> Besides the MT8183 MFG clock issues, we do actually need this for the
> >>> MT8192 ADSP clock. Its power domain is not enabled by default.
> >>>
> >>
> >> ...but on MT8195 the ADSP clock works - because the ADSP node exists.
> >
> > That's an indirect dependency that should not be relied on. Say the clock
> > driver probed but the ADSP hasn't, and you try to read out the current
> > status. What would happen?
> >
> > - Read out works fine, because the power domain is default on, and hasn't
> > been turned off by late cleanup
> > - Read out is bogus (but you can't tell)
> > - Read out hangs.
> >
> > The third is what happens on MT8192. There's still some issues on that
> > front, as even after I applied the ADSP power domain patches from MediaTek,
> > the readout was still hanging.
> >
>
> That MT8192 lockup story is getting crazy in my head... anyway, besides that,
> I get the point - I was somehow ignoring the fact that kernel modules do exist.
>
> Eh, sorry about that :-)
>
> >> This poses a question: should we make clock controllers depend on power domains,
> >> or should we keep everything powered off (hence clocks down - no power consumption)
> >> *unless* the user exists?
> >
> > That's a policy discussion separate from actual hardware dependencies.
> > *If* the clock controller needs the power domain to be active for the
> > registers to be accessed, the clock controller *must* have a direct
> > dependency on the power domain.
> >
>
> I admit I should've worded that better.
>
> "should we make clock controllers depend on power domains" was actually implying
> "IF those need one" :-)
>
> I really wonder if - at this point - it's simply a better idea to not restrict
> the call to devm_pm_runtime_enable/resume_and_get to `need_runtime_pm == true`.
>
> Do we really need to exclude that on other clock controllers that don't have
> any power domain dependency? Any side effect?
>
> Saying this because if we can avoid yet another per-SoC flag I'm really happy,
> as readability is also impacted and besides - if we ever find out that one of
> those need a power domain in the future, we'll need just one commit and just
> only in the devicetree, instead of enabling a flag in driver X as well as that,
> avoiding some (potentially unnecessary) noise... I guess.
>
> P.S.: I just noticed that the return value for the devm_pm_runtime_enable() call
> is not being checked!
>
> .......
>
> In short....
>
> Chen-Yu, at this point, do you have any reason why we wouldn't be able and/or it
> wouldn't be a good idea to just avoid adding the `need_runtime_pm` flag (meaning
> that we perform pm_runtime calls for all clock drivers unconditionally)?
>
> If this is about longer boot time, I don't think that it's going to be more than
> a millisecond or two, so that should be completely ignorable.
I think it's just more of a "don't enable features you don't need" thing.
We already ran into a weird deadlock, which is why the devm_pm_runtime_enable()
call has that comment.
I don't think anyone has actually looked at it. As you said it shouldn't be
much, at least during boot time. It's one call per clock controller.
> Can you please do a test for that, or should I?
The earliest I can work on it would be some time next week. Does that work
for you?
ChenYu
> Cheers
> Angelo
>
> >> For the second one, this means that the *device* gets the power domain (adsp), and
> >> not the clock controller (which clocks are effectively useless if there's no user).
> >
> > No. See my previous paragraph.
> >
> > ChenYu
> >
> >> Angelo
> >>
> >>>> ---
> >>>>
> >>>> Changes in v3:
> >>>> - Update the commit message and the comments before runtime PM call
> >>>>
> >>>> Changes in v2:
> >>>> - Fix the order of error handling
> >>>> - Update the commit message and add a comment before the runtime PM call
> >>>>
> >>>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
> >>>> drivers/clk/mediatek/clk-mtk.h | 2 ++
> >>>> 2 files changed, 21 insertions(+)
> >>>>
> >>>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> >>>> index 2e55368dc4d8..ba1d1c495bc2 100644
> >>>> --- a/drivers/clk/mediatek/clk-mtk.c
> >>>> +++ b/drivers/clk/mediatek/clk-mtk.c
> >>>> @@ -13,6 +13,7 @@
> >>>> #include <linux/of.h>
> >>>> #include <linux/of_address.h>
> >>>> #include <linux/platform_device.h>
> >>>> +#include <linux/pm_runtime.h>
> >>>> #include <linux/slab.h>
> >>>>
> >>>> #include "clk-mtk.h"
> >>>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
> >>>> }
> >>>>
> >>>> +
> >>>> + if (mcd->need_runtime_pm) {
> >>>> + devm_pm_runtime_enable(&pdev->dev);
> >>>> + /*
> >>>> + * Do a pm_runtime_resume_and_get() to workaround a possible
> >>>> + * deadlock between clk_register() and the genpd framework.
> >>>> + */
> >>>> + r = pm_runtime_resume_and_get(&pdev->dev);
> >>>> + if (r)
> >>>> + return r;
> >>>> + }
> >>>> +
> >>>> /* Calculate how many clk_hw_onecell_data entries to allocate */
> >>>> num_clks = mcd->num_clks + mcd->num_composite_clks;
> >>>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
> >>>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>>> goto unregister_clks;
> >>>> }
> >>>>
> >>>> + if (mcd->need_runtime_pm)
> >>>> + pm_runtime_put(&pdev->dev);
> >>>> +
> >>>> return r;
> >>>>
> >>>> unregister_clks:
> >>>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>>> free_base:
> >>>> if (mcd->shared_io && base)
> >>>> iounmap(base);
> >>>> +
> >>>> + if (mcd->need_runtime_pm)
> >>>> + pm_runtime_put(&pdev->dev);
> >>>> return r;
> >>>> }
> >>>>
> >>>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> >>>> index 22096501a60a..c17fe1c2d732 100644
> >>>> --- a/drivers/clk/mediatek/clk-mtk.h
> >>>> +++ b/drivers/clk/mediatek/clk-mtk.h
> >>>> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
> >>>>
> >>>> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
> >>>> unsigned int mfg_clk_idx;
> >>>> +
> >>>> + bool need_runtime_pm;
> >>>> };
> >>>>
> >>>> int mtk_clk_pdev_probe(struct platform_device *pdev);
> >>>> --
> >>>> 2.43.0.472.g3155946c3a-goog
> >>>>
> >>
> >>
> >>
>
>
>
Il 29/02/24 08:17, Chen-Yu Tsai ha scritto:
> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno
> <[email protected]> wrote:
>>
>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
>>>>
>>>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
>>>> this clock controller needs runtime PM for its operations.
>>>> Also do a runtime PM get on the clock controller during the
>>>> probing stage to workaround a possible deadlock.
>>>>
>>>> Signed-off-by: Pin-yen Lin <[email protected]>
>>>
>>> Reviewed-by: Chen-Yu Tsai <[email protected]>
>>>
>>> The patch itself looks fine.
>>>
>>> Besides the MT8183 MFG clock issues, we do actually need this for the
>>> MT8192 ADSP clock. Its power domain is not enabled by default.
>>>
>>
>> ...but on MT8195 the ADSP clock works - because the ADSP node exists.
>
> That's an indirect dependency that should not be relied on. Say the clock
> driver probed but the ADSP hasn't, and you try to read out the current
> status. What would happen?
>
> - Read out works fine, because the power domain is default on, and hasn't
> been turned off by late cleanup
> - Read out is bogus (but you can't tell)
> - Read out hangs.
>
> The third is what happens on MT8192. There's still some issues on that
> front, as even after I applied the ADSP power domain patches from MediaTek,
> the readout was still hanging.
>
That MT8192 lockup story is getting crazy in my head... anyway, besides that,
I get the point - I was somehow ignoring the fact that kernel modules do exist.
Eh, sorry about that :-)
>> This poses a question: should we make clock controllers depend on power domains,
>> or should we keep everything powered off (hence clocks down - no power consumption)
>> *unless* the user exists?
>
> That's a policy discussion separate from actual hardware dependencies.
> *If* the clock controller needs the power domain to be active for the
> registers to be accessed, the clock controller *must* have a direct
> dependency on the power domain.
>
I admit I should've worded that better.
"should we make clock controllers depend on power domains" was actually implying
"IF those need one" :-)
I really wonder if - at this point - it's simply a better idea to not restrict
the call to devm_pm_runtime_enable/resume_and_get to `need_runtime_pm == true`.
Do we really need to exclude that on other clock controllers that don't have
any power domain dependency? Any side effect?
Saying this because if we can avoid yet another per-SoC flag I'm really happy,
as readability is also impacted and besides - if we ever find out that one of
those need a power domain in the future, we'll need just one commit and just
only in the devicetree, instead of enabling a flag in driver X as well as that,
avoiding some (potentially unnecessary) noise... I guess.
P.S.: I just noticed that the return value for the devm_pm_runtime_enable() call
is not being checked!
......
In short....
Chen-Yu, at this point, do you have any reason why we wouldn't be able and/or it
wouldn't be a good idea to just avoid adding the `need_runtime_pm` flag (meaning
that we perform pm_runtime calls for all clock drivers unconditionally)?
If this is about longer boot time, I don't think that it's going to be more than
a millisecond or two, so that should be completely ignorable.
Can you please do a test for that, or should I?
Cheers
Angelo
>> For the second one, this means that the *device* gets the power domain (adsp), and
>> not the clock controller (which clocks are effectively useless if there's no user).
>
> No. See my previous paragraph.
>
> ChenYu
>
>> Angelo
>>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - Update the commit message and the comments before runtime PM call
>>>>
>>>> Changes in v2:
>>>> - Fix the order of error handling
>>>> - Update the commit message and add a comment before the runtime PM call
>>>>
>>>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
>>>> drivers/clk/mediatek/clk-mtk.h | 2 ++
>>>> 2 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
>>>> index 2e55368dc4d8..ba1d1c495bc2 100644
>>>> --- a/drivers/clk/mediatek/clk-mtk.c
>>>> +++ b/drivers/clk/mediatek/clk-mtk.c
>>>> @@ -13,6 +13,7 @@
>>>> #include <linux/of.h>
>>>> #include <linux/of_address.h>
>>>> #include <linux/platform_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>> #include <linux/slab.h>
>>>>
>>>> #include "clk-mtk.h"
>>>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
>>>> }
>>>>
>>>> +
>>>> + if (mcd->need_runtime_pm) {
>>>> + devm_pm_runtime_enable(&pdev->dev);
>>>> + /*
>>>> + * Do a pm_runtime_resume_and_get() to workaround a possible
>>>> + * deadlock between clk_register() and the genpd framework.
>>>> + */
>>>> + r = pm_runtime_resume_and_get(&pdev->dev);
>>>> + if (r)
>>>> + return r;
>>>> + }
>>>> +
>>>> /* Calculate how many clk_hw_onecell_data entries to allocate */
>>>> num_clks = mcd->num_clks + mcd->num_composite_clks;
>>>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
>>>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>> goto unregister_clks;
>>>> }
>>>>
>>>> + if (mcd->need_runtime_pm)
>>>> + pm_runtime_put(&pdev->dev);
>>>> +
>>>> return r;
>>>>
>>>> unregister_clks:
>>>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>> free_base:
>>>> if (mcd->shared_io && base)
>>>> iounmap(base);
>>>> +
>>>> + if (mcd->need_runtime_pm)
>>>> + pm_runtime_put(&pdev->dev);
>>>> return r;
>>>> }
>>>>
>>>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
>>>> index 22096501a60a..c17fe1c2d732 100644
>>>> --- a/drivers/clk/mediatek/clk-mtk.h
>>>> +++ b/drivers/clk/mediatek/clk-mtk.h
>>>> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
>>>>
>>>> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
>>>> unsigned int mfg_clk_idx;
>>>> +
>>>> + bool need_runtime_pm;
>>>> };
>>>>
>>>> int mtk_clk_pdev_probe(struct platform_device *pdev);
>>>> --
>>>> 2.43.0.472.g3155946c3a-goog
>>>>
>>
>>
>>
Il 23/02/24 05:43, Chen-Yu Tsai ha scritto:
> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
>>
>> mt8183-mfgcfg has a mutual dependency with genpd during the probing
>> stage, so enable need_runtim_pm to prevent a deadlock in the following
>> call stack:
>>
>> CPU0: genpd_lock --> clk_prepare_lock
>> genpd_power_off_work_fn()
>> genpd_lock()
>> generic_pm_domain::power_off()
>> clk_unprepare()
>> clk_prepare_lock()
>>
>> CPU1: clk_prepare_lock --> genpd_lock
>> clk_register()
>> __clk_core_init()
>> clk_prepare_lock()
>> clk_pm_runtime_get()
>> genpd_lock()
>>
>> Do a runtime PM get at the probe function to make sure clk_register()
>> won't acquire the genpd lock.
>>
>> Fixes: acddfc2c261b ("clk: mediatek: Add MT8183 clock support")
>> Signed-off-by: Pin-yen Lin <[email protected]>
>
> Reviewed-by: Chen-Yu Tsai <[email protected]>
>
> Note that this compliments a patch [1] adding the power domain for the mfgcfg
> clock controller node, which has been floating around for almost 3 years.
>
..but why does this happen *only* on MT8183 and not on any other MediaTek SoC?
I understand what you're trying to solve here, but if we explore a bit more, we
can maybe come to the conclusion that we don't need to add this flag, and perhaps
just enable PM runtime as regular flow for all clock controllers.
That would also be cleaner, to some extent.
Cheers,
Angelo
> [1] https://lore.kernel.org/linux-mediatek/[email protected]/
>
>> ---
>>
>> (no changes since v1)
>>
>> drivers/clk/mediatek/clk-mt8183-mfgcfg.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
>> index ba504e19d420..62d876e150e1 100644
>> --- a/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
>> +++ b/drivers/clk/mediatek/clk-mt8183-mfgcfg.c
>> @@ -29,6 +29,7 @@ static const struct mtk_gate mfg_clks[] = {
>> static const struct mtk_clk_desc mfg_desc = {
>> .clks = mfg_clks,
>> .num_clks = ARRAY_SIZE(mfg_clks),
>> + .need_runtime_pm = true,
>> };
>>
>> static const struct of_device_id of_match_clk_mt8183_mfg[] = {
>> --
>> 2.43.0.472.g3155946c3a-goog
>>
Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
>>
>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
>> this clock controller needs runtime PM for its operations.
>> Also do a runtime PM get on the clock controller during the
>> probing stage to workaround a possible deadlock.
>>
>> Signed-off-by: Pin-yen Lin <[email protected]>
>
> Reviewed-by: Chen-Yu Tsai <[email protected]>
>
> The patch itself looks fine.
>
> Besides the MT8183 MFG clock issues, we do actually need this for the
> MT8192 ADSP clock. Its power domain is not enabled by default.
>
..but on MT8195 the ADSP clock works - because the ADSP node exists.
This poses a question: should we make clock controllers depend on power domains,
or should we keep everything powered off (hence clocks down - no power consumption)
*unless* the user exists?
For the second one, this means that the *device* gets the power domain (adsp), and
not the clock controller (which clocks are effectively useless if there's no user).
Angelo
>> ---
>>
>> Changes in v3:
>> - Update the commit message and the comments before runtime PM call
>>
>> Changes in v2:
>> - Fix the order of error handling
>> - Update the commit message and add a comment before the runtime PM call
>>
>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
>> drivers/clk/mediatek/clk-mtk.h | 2 ++
>> 2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
>> index 2e55368dc4d8..ba1d1c495bc2 100644
>> --- a/drivers/clk/mediatek/clk-mtk.c
>> +++ b/drivers/clk/mediatek/clk-mtk.c
>> @@ -13,6 +13,7 @@
>> #include <linux/of.h>
>> #include <linux/of_address.h>
>> #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/slab.h>
>>
>> #include "clk-mtk.h"
>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
>> }
>>
>> +
>> + if (mcd->need_runtime_pm) {
>> + devm_pm_runtime_enable(&pdev->dev);
>> + /*
>> + * Do a pm_runtime_resume_and_get() to workaround a possible
>> + * deadlock between clk_register() and the genpd framework.
>> + */
>> + r = pm_runtime_resume_and_get(&pdev->dev);
>> + if (r)
>> + return r;
>> + }
>> +
>> /* Calculate how many clk_hw_onecell_data entries to allocate */
>> num_clks = mcd->num_clks + mcd->num_composite_clks;
>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>> goto unregister_clks;
>> }
>>
>> + if (mcd->need_runtime_pm)
>> + pm_runtime_put(&pdev->dev);
>> +
>> return r;
>>
>> unregister_clks:
>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>> free_base:
>> if (mcd->shared_io && base)
>> iounmap(base);
>> +
>> + if (mcd->need_runtime_pm)
>> + pm_runtime_put(&pdev->dev);
>> return r;
>> }
>>
>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
>> index 22096501a60a..c17fe1c2d732 100644
>> --- a/drivers/clk/mediatek/clk-mtk.h
>> +++ b/drivers/clk/mediatek/clk-mtk.h
>> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
>>
>> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
>> unsigned int mfg_clk_idx;
>> +
>> + bool need_runtime_pm;
>> };
>>
>> int mtk_clk_pdev_probe(struct platform_device *pdev);
>> --
>> 2.43.0.472.g3155946c3a-goog
>>
Hi Angelo and Chen-yu,
I tried enabling the runtime PM regardless of the .need_pm_runtime
flag, and my MT8183 device works well with that with no obvious boot
regression.
Should I send out another patch that always enables runtime PM in
__mtk_clk_simple_probe()? Or is there anything I should test?
Regards,
Pin-yen
On Thu, Feb 29, 2024 at 6:36 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 29/02/24 11:34, Chen-Yu Tsai ha scritto:
> > On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno
> > <[email protected]> wrote:
> >>
> >> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto:
> >>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno
> >>> <[email protected]> wrote:
> >>>>
> >>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
> >>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
> >>>>>>
> >>>>>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
> >>>>>> this clock controller needs runtime PM for its operations.
> >>>>>> Also do a runtime PM get on the clock controller during the
> >>>>>> probing stage to workaround a possible deadlock.
> >>>>>>
> >>>>>> Signed-off-by: Pin-yen Lin <[email protected]>
> >>>>>
> >>>>> Reviewed-by: Chen-Yu Tsai <[email protected]>
> >>>>>
> >>>>> The patch itself looks fine.
> >>>>>
> >>>>> Besides the MT8183 MFG clock issues, we do actually need this for the
> >>>>> MT8192 ADSP clock. Its power domain is not enabled by default.
> >>>>>
> >>>>
> >>>> ...but on MT8195 the ADSP clock works - because the ADSP node exists.
> >>>
> >>> That's an indirect dependency that should not be relied on. Say the clock
> >>> driver probed but the ADSP hasn't, and you try to read out the current
> >>> status. What would happen?
> >>>
> >>> - Read out works fine, because the power domain is default on, and hasn't
> >>> been turned off by late cleanup
> >>> - Read out is bogus (but you can't tell)
> >>> - Read out hangs.
> >>>
> >>> The third is what happens on MT8192. There's still some issues on that
> >>> front, as even after I applied the ADSP power domain patches from MediaTek,
> >>> the readout was still hanging.
> >>>
> >>
> >> That MT8192 lockup story is getting crazy in my head... anyway, besides that,
> >> I get the point - I was somehow ignoring the fact that kernel modules do exist.
> >>
> >> Eh, sorry about that :-)
> >>
> >>>> This poses a question: should we make clock controllers depend on power domains,
> >>>> or should we keep everything powered off (hence clocks down - no power consumption)
> >>>> *unless* the user exists?
> >>>
> >>> That's a policy discussion separate from actual hardware dependencies.
> >>> *If* the clock controller needs the power domain to be active for the
> >>> registers to be accessed, the clock controller *must* have a direct
> >>> dependency on the power domain.
> >>>
> >>
> >> I admit I should've worded that better.
> >>
> >> "should we make clock controllers depend on power domains" was actually implying
> >> "IF those need one" :-)
> >>
> >> I really wonder if - at this point - it's simply a better idea to not restrict
> >> the call to devm_pm_runtime_enable/resume_and_get to `need_runtime_pm == true`.
> >>
> >> Do we really need to exclude that on other clock controllers that don't have
> >> any power domain dependency? Any side effect?
> >>
> >> Saying this because if we can avoid yet another per-SoC flag I'm really happy,
> >> as readability is also impacted and besides - if we ever find out that one of
> >> those need a power domain in the future, we'll need just one commit and just
> >> only in the devicetree, instead of enabling a flag in driver X as well as that,
> >> avoiding some (potentially unnecessary) noise... I guess.
> >>
> >> P.S.: I just noticed that the return value for the devm_pm_runtime_enable() call
> >> is not being checked!
> >>
> >> .......
> >>
> >> In short....
> >>
> >> Chen-Yu, at this point, do you have any reason why we wouldn't be able and/or it
> >> wouldn't be a good idea to just avoid adding the `need_runtime_pm` flag (meaning
> >> that we perform pm_runtime calls for all clock drivers unconditionally)?
> >>
> >> If this is about longer boot time, I don't think that it's going to be more than
> >> a millisecond or two, so that should be completely ignorable.
> >
> > I think it's just more of a "don't enable features you don't need" thing.
> > We already ran into a weird deadlock, which is why the devm_pm_runtime_enable()
> > call has that comment.
> >
> > I don't think anyone has actually looked at it. As you said it shouldn't be
> > much, at least during boot time. It's one call per clock controller.
> >
> >> Can you please do a test for that, or should I?
> >
> > The earliest I can work on it would be some time next week. Does that work
> > for you?
> >
>
> The earliest I'd be able to work on this myself would be at the end of next
> week if not later.. so yes, please take your time, no worries.
>
> Thank you!
>
> > ChenYu
> >
> >> Cheers
> >> Angelo
> >>
> >>>> For the second one, this means that the *device* gets the power domain (adsp), and
> >>>> not the clock controller (which clocks are effectively useless if there's no user).
> >>>
> >>> No. See my previous paragraph.
> >>>
> >>> ChenYu
> >>>
> >>>> Angelo
> >>>>
> >>>>>> ---
> >>>>>>
> >>>>>> Changes in v3:
> >>>>>> - Update the commit message and the comments before runtime PM call
> >>>>>>
> >>>>>> Changes in v2:
> >>>>>> - Fix the order of error handling
> >>>>>> - Update the commit message and add a comment before the runtime PM call
> >>>>>>
> >>>>>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
> >>>>>> drivers/clk/mediatek/clk-mtk.h | 2 ++
> >>>>>> 2 files changed, 21 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> >>>>>> index 2e55368dc4d8..ba1d1c495bc2 100644
> >>>>>> --- a/drivers/clk/mediatek/clk-mtk.c
> >>>>>> +++ b/drivers/clk/mediatek/clk-mtk.c
> >>>>>> @@ -13,6 +13,7 @@
> >>>>>> #include <linux/of.h>
> >>>>>> #include <linux/of_address.h>
> >>>>>> #include <linux/platform_device.h>
> >>>>>> +#include <linux/pm_runtime.h>
> >>>>>> #include <linux/slab.h>
> >>>>>>
> >>>>>> #include "clk-mtk.h"
> >>>>>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>>>>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
> >>>>>> }
> >>>>>>
> >>>>>> +
> >>>>>> + if (mcd->need_runtime_pm) {
> >>>>>> + devm_pm_runtime_enable(&pdev->dev);
> >>>>>> + /*
> >>>>>> + * Do a pm_runtime_resume_and_get() to workaround a possible
> >>>>>> + * deadlock between clk_register() and the genpd framework.
> >>>>>> + */
> >>>>>> + r = pm_runtime_resume_and_get(&pdev->dev);
> >>>>>> + if (r)
> >>>>>> + return r;
> >>>>>> + }
> >>>>>> +
> >>>>>> /* Calculate how many clk_hw_onecell_data entries to allocate */
> >>>>>> num_clks = mcd->num_clks + mcd->num_composite_clks;
> >>>>>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
> >>>>>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>>>>> goto unregister_clks;
> >>>>>> }
> >>>>>>
> >>>>>> + if (mcd->need_runtime_pm)
> >>>>>> + pm_runtime_put(&pdev->dev);
> >>>>>> +
> >>>>>> return r;
> >>>>>>
> >>>>>> unregister_clks:
> >>>>>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>>>>> free_base:
> >>>>>> if (mcd->shared_io && base)
> >>>>>> iounmap(base);
> >>>>>> +
> >>>>>> + if (mcd->need_runtime_pm)
> >>>>>> + pm_runtime_put(&pdev->dev);
> >>>>>> return r;
> >>>>>> }
> >>>>>>
> >>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> >>>>>> index 22096501a60a..c17fe1c2d732 100644
> >>>>>> --- a/drivers/clk/mediatek/clk-mtk.h
> >>>>>> +++ b/drivers/clk/mediatek/clk-mtk.h
> >>>>>> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
> >>>>>>
> >>>>>> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
> >>>>>> unsigned int mfg_clk_idx;
> >>>>>> +
> >>>>>> + bool need_runtime_pm;
> >>>>>> };
> >>>>>>
> >>>>>> int mtk_clk_pdev_probe(struct platform_device *pdev);
> >>>>>> --
> >>>>>> 2.43.0.472.g3155946c3a-goog
> >>>>>>
> >>>>
> >>>>
> >>>>
> >>
> >>
> >>
>
Il 07/03/24 12:10, Pin-yen Lin ha scritto:
> Hi Angelo and Chen-yu,
>
> I tried enabling the runtime PM regardless of the .need_pm_runtime
> flag, and my MT8183 device works well with that with no obvious boot
> regression.
>
> Should I send out another patch that always enables runtime PM in
> __mtk_clk_simple_probe()? Or is there anything I should test?
>
Hello Pin-yen,
as I discussed with Chen-Yu - yes, we must make sure that this does not
create any regression on machines running on other SoC models.
I think it's unlikely that it does, but since the HW is available, being
extremely careful with validating this change is a good idea :-)
If you can/want to test before we do, sure, please send the new patch and,
when you do, please say that you tested it and on which SoCs; as long as
it's not just one SoC, that'll be good enough for me.
P.S.: Please don't top-post!
Cheers,
Angelo
> Regards,
> Pin-yen
>
> On Thu, Feb 29, 2024 at 6:36 PM AngeloGioacchino Del Regno
> <[email protected]> wrote:
>>
>> Il 29/02/24 11:34, Chen-Yu Tsai ha scritto:
>>> On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno
>>> <[email protected]> wrote:
>>>>
>>>> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto:
>>>>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
>>>>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
>>>>>>>> this clock controller needs runtime PM for its operations.
>>>>>>>> Also do a runtime PM get on the clock controller during the
>>>>>>>> probing stage to workaround a possible deadlock.
>>>>>>>>
>>>>>>>> Signed-off-by: Pin-yen Lin <[email protected]>
>>>>>>>
>>>>>>> Reviewed-by: Chen-Yu Tsai <[email protected]>
>>>>>>>
>>>>>>> The patch itself looks fine.
>>>>>>>
>>>>>>> Besides the MT8183 MFG clock issues, we do actually need this for the
>>>>>>> MT8192 ADSP clock. Its power domain is not enabled by default.
>>>>>>>
>>>>>>
>>>>>> ...but on MT8195 the ADSP clock works - because the ADSP node exists.
>>>>>
>>>>> That's an indirect dependency that should not be relied on. Say the clock
>>>>> driver probed but the ADSP hasn't, and you try to read out the current
>>>>> status. What would happen?
>>>>>
>>>>> - Read out works fine, because the power domain is default on, and hasn't
>>>>> been turned off by late cleanup
>>>>> - Read out is bogus (but you can't tell)
>>>>> - Read out hangs.
>>>>>
>>>>> The third is what happens on MT8192. There's still some issues on that
>>>>> front, as even after I applied the ADSP power domain patches from MediaTek,
>>>>> the readout was still hanging.
>>>>>
>>>>
>>>> That MT8192 lockup story is getting crazy in my head... anyway, besides that,
>>>> I get the point - I was somehow ignoring the fact that kernel modules do exist.
>>>>
>>>> Eh, sorry about that :-)
>>>>
>>>>>> This poses a question: should we make clock controllers depend on power domains,
>>>>>> or should we keep everything powered off (hence clocks down - no power consumption)
>>>>>> *unless* the user exists?
>>>>>
>>>>> That's a policy discussion separate from actual hardware dependencies.
>>>>> *If* the clock controller needs the power domain to be active for the
>>>>> registers to be accessed, the clock controller *must* have a direct
>>>>> dependency on the power domain.
>>>>>
>>>>
>>>> I admit I should've worded that better.
>>>>
>>>> "should we make clock controllers depend on power domains" was actually implying
>>>> "IF those need one" :-)
>>>>
>>>> I really wonder if - at this point - it's simply a better idea to not restrict
>>>> the call to devm_pm_runtime_enable/resume_and_get to `need_runtime_pm == true`.
>>>>
>>>> Do we really need to exclude that on other clock controllers that don't have
>>>> any power domain dependency? Any side effect?
>>>>
>>>> Saying this because if we can avoid yet another per-SoC flag I'm really happy,
>>>> as readability is also impacted and besides - if we ever find out that one of
>>>> those need a power domain in the future, we'll need just one commit and just
>>>> only in the devicetree, instead of enabling a flag in driver X as well as that,
>>>> avoiding some (potentially unnecessary) noise... I guess.
>>>>
>>>> P.S.: I just noticed that the return value for the devm_pm_runtime_enable() call
>>>> is not being checked!
>>>>
>>>> .......
>>>>
>>>> In short....
>>>>
>>>> Chen-Yu, at this point, do you have any reason why we wouldn't be able and/or it
>>>> wouldn't be a good idea to just avoid adding the `need_runtime_pm` flag (meaning
>>>> that we perform pm_runtime calls for all clock drivers unconditionally)?
>>>>
>>>> If this is about longer boot time, I don't think that it's going to be more than
>>>> a millisecond or two, so that should be completely ignorable.
>>>
>>> I think it's just more of a "don't enable features you don't need" thing.
>>> We already ran into a weird deadlock, which is why the devm_pm_runtime_enable()
>>> call has that comment.
>>>
>>> I don't think anyone has actually looked at it. As you said it shouldn't be
>>> much, at least during boot time. It's one call per clock controller.
>>>
>>>> Can you please do a test for that, or should I?
>>>
>>> The earliest I can work on it would be some time next week. Does that work
>>> for you?
>>>
>>
>> The earliest I'd be able to work on this myself would be at the end of next
>> week if not later.. so yes, please take your time, no worries.
>>
>> Thank you!
>>
>>> ChenYu
>>>
>>>> Cheers
>>>> Angelo
>>>>
>>>>>> For the second one, this means that the *device* gets the power domain (adsp), and
>>>>>> not the clock controller (which clocks are effectively useless if there's no user).
>>>>>
>>>>> No. See my previous paragraph.
>>>>>
>>>>> ChenYu
>>>>>
>>>>>> Angelo
>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v3:
>>>>>>>> - Update the commit message and the comments before runtime PM call
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>> - Fix the order of error handling
>>>>>>>> - Update the commit message and add a comment before the runtime PM call
>>>>>>>>
>>>>>>>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
>>>>>>>> drivers/clk/mediatek/clk-mtk.h | 2 ++
>>>>>>>> 2 files changed, 21 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
>>>>>>>> index 2e55368dc4d8..ba1d1c495bc2 100644
>>>>>>>> --- a/drivers/clk/mediatek/clk-mtk.c
>>>>>>>> +++ b/drivers/clk/mediatek/clk-mtk.c
>>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>> #include <linux/of.h>
>>>>>>>> #include <linux/of_address.h>
>>>>>>>> #include <linux/platform_device.h>
>>>>>>>> +#include <linux/pm_runtime.h>
>>>>>>>> #include <linux/slab.h>
>>>>>>>>
>>>>>>>> #include "clk-mtk.h"
>>>>>>>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>>>>>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +
>>>>>>>> + if (mcd->need_runtime_pm) {
>>>>>>>> + devm_pm_runtime_enable(&pdev->dev);
>>>>>>>> + /*
>>>>>>>> + * Do a pm_runtime_resume_and_get() to workaround a possible
>>>>>>>> + * deadlock between clk_register() and the genpd framework.
>>>>>>>> + */
>>>>>>>> + r = pm_runtime_resume_and_get(&pdev->dev);
>>>>>>>> + if (r)
>>>>>>>> + return r;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> /* Calculate how many clk_hw_onecell_data entries to allocate */
>>>>>>>> num_clks = mcd->num_clks + mcd->num_composite_clks;
>>>>>>>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
>>>>>>>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>>>>>> goto unregister_clks;
>>>>>>>> }
>>>>>>>>
>>>>>>>> + if (mcd->need_runtime_pm)
>>>>>>>> + pm_runtime_put(&pdev->dev);
>>>>>>>> +
>>>>>>>> return r;
>>>>>>>>
>>>>>>>> unregister_clks:
>>>>>>>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>>>>>> free_base:
>>>>>>>> if (mcd->shared_io && base)
>>>>>>>> iounmap(base);
>>>>>>>> +
>>>>>>>> + if (mcd->need_runtime_pm)
>>>>>>>> + pm_runtime_put(&pdev->dev);
>>>>>>>> return r;
>>>>>>>> }
>>>>>>>>
>>>>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
>>>>>>>> index 22096501a60a..c17fe1c2d732 100644
>>>>>>>> --- a/drivers/clk/mediatek/clk-mtk.h
>>>>>>>> +++ b/drivers/clk/mediatek/clk-mtk.h
>>>>>>>> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
>>>>>>>>
>>>>>>>> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
>>>>>>>> unsigned int mfg_clk_idx;
>>>>>>>> +
>>>>>>>> + bool need_runtime_pm;
>>>>>>>> };
>>>>>>>>
>>>>>>>> int mtk_clk_pdev_probe(struct platform_device *pdev);
>>>>>>>> --
>>>>>>>> 2.43.0.472.g3155946c3a-goog
>>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>
Hi Angelo
On Thu, Mar 7, 2024 at 7:22 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 07/03/24 12:10, Pin-yen Lin ha scritto:
> > Hi Angelo and Chen-yu,
> >
> > I tried enabling the runtime PM regardless of the .need_pm_runtime
> > flag, and my MT8183 device works well with that with no obvious boot
> > regression.
> >
> > Should I send out another patch that always enables runtime PM in
> > __mtk_clk_simple_probe()? Or is there anything I should test?
> >
>
> Hello Pin-yen,
>
> as I discussed with Chen-Yu - yes, we must make sure that this does not
> create any regression on machines running on other SoC models.
>
> I think it's unlikely that it does, but since the HW is available, being
> extremely careful with validating this change is a good idea :-)
>
> If you can/want to test before we do, sure, please send the new patch and,
> when you do, please say that you tested it and on which SoCs; as long as
> it's not just one SoC, that'll be good enough for me.
>
> P.S.: Please don't top-post!
>
> Cheers,
> Angelo
I just tried this on MT8192 and the system looks healthy as well. I'll
send out a patch to enable the runtime PM for all mediatek clock
controllers.
Thanks for your time on reviewing this, and sorry for the top-posting.
Regards,
Pin-yen
>
> > Regards,
> > Pin-yen
> >
> > On Thu, Feb 29, 2024 at 6:36 PM AngeloGioacchino Del Regno
> > <[email protected]> wrote:
> >>
> >> Il 29/02/24 11:34, Chen-Yu Tsai ha scritto:
> >>> On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno
> >>> <[email protected]> wrote:
> >>>>
> >>>> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto:
> >>>>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno
> >>>>> <[email protected]> wrote:
> >>>>>>
> >>>>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
> >>>>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
> >>>>>>>>
> >>>>>>>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
> >>>>>>>> this clock controller needs runtime PM for its operations.
> >>>>>>>> Also do a runtime PM get on the clock controller during the
> >>>>>>>> probing stage to workaround a possible deadlock.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Pin-yen Lin <[email protected]>
> >>>>>>>
> >>>>>>> Reviewed-by: Chen-Yu Tsai <[email protected]>
> >>>>>>>
> >>>>>>> The patch itself looks fine.
> >>>>>>>
> >>>>>>> Besides the MT8183 MFG clock issues, we do actually need this for the
> >>>>>>> MT8192 ADSP clock. Its power domain is not enabled by default.
> >>>>>>>
> >>>>>>
> >>>>>> ...but on MT8195 the ADSP clock works - because the ADSP node exists.
> >>>>>
> >>>>> That's an indirect dependency that should not be relied on. Say the clock
> >>>>> driver probed but the ADSP hasn't, and you try to read out the current
> >>>>> status. What would happen?
> >>>>>
> >>>>> - Read out works fine, because the power domain is default on, and hasn't
> >>>>> been turned off by late cleanup
> >>>>> - Read out is bogus (but you can't tell)
> >>>>> - Read out hangs.
> >>>>>
> >>>>> The third is what happens on MT8192. There's still some issues on that
> >>>>> front, as even after I applied the ADSP power domain patches from MediaTek,
> >>>>> the readout was still hanging.
> >>>>>
> >>>>
> >>>> That MT8192 lockup story is getting crazy in my head... anyway, besides that,
> >>>> I get the point - I was somehow ignoring the fact that kernel modules do exist.
> >>>>
> >>>> Eh, sorry about that :-)
> >>>>
> >>>>>> This poses a question: should we make clock controllers depend on power domains,
> >>>>>> or should we keep everything powered off (hence clocks down - no power consumption)
> >>>>>> *unless* the user exists?
> >>>>>
> >>>>> That's a policy discussion separate from actual hardware dependencies.
> >>>>> *If* the clock controller needs the power domain to be active for the
> >>>>> registers to be accessed, the clock controller *must* have a direct
> >>>>> dependency on the power domain.
> >>>>>
> >>>>
> >>>> I admit I should've worded that better.
> >>>>
> >>>> "should we make clock controllers depend on power domains" was actually implying
> >>>> "IF those need one" :-)
> >>>>
> >>>> I really wonder if - at this point - it's simply a better idea to not restrict
> >>>> the call to devm_pm_runtime_enable/resume_and_get to `need_runtime_pm == true`.
> >>>>
> >>>> Do we really need to exclude that on other clock controllers that don't have
> >>>> any power domain dependency? Any side effect?
> >>>>
> >>>> Saying this because if we can avoid yet another per-SoC flag I'm really happy,
> >>>> as readability is also impacted and besides - if we ever find out that one of
> >>>> those need a power domain in the future, we'll need just one commit and just
> >>>> only in the devicetree, instead of enabling a flag in driver X as well as that,
> >>>> avoiding some (potentially unnecessary) noise... I guess.
> >>>>
> >>>> P.S.: I just noticed that the return value for the devm_pm_runtime_enable() call
> >>>> is not being checked!
> >>>>
> >>>> .......
> >>>>
> >>>> In short....
> >>>>
> >>>> Chen-Yu, at this point, do you have any reason why we wouldn't be able and/or it
> >>>> wouldn't be a good idea to just avoid adding the `need_runtime_pm` flag (meaning
> >>>> that we perform pm_runtime calls for all clock drivers unconditionally)?
> >>>>
> >>>> If this is about longer boot time, I don't think that it's going to be more than
> >>>> a millisecond or two, so that should be completely ignorable.
> >>>
> >>> I think it's just more of a "don't enable features you don't need" thing.
> >>> We already ran into a weird deadlock, which is why the devm_pm_runtime_enable()
> >>> call has that comment.
> >>>
> >>> I don't think anyone has actually looked at it. As you said it shouldn't be
> >>> much, at least during boot time. It's one call per clock controller.
> >>>
> >>>> Can you please do a test for that, or should I?
> >>>
> >>> The earliest I can work on it would be some time next week. Does that work
> >>> for you?
> >>>
> >>
> >> The earliest I'd be able to work on this myself would be at the end of next
> >> week if not later.. so yes, please take your time, no worries.
> >>
> >> Thank you!
> >>
> >>> ChenYu
> >>>
> >>>> Cheers
> >>>> Angelo
> >>>>
> >>>>>> For the second one, this means that the *device* gets the power domain (adsp), and
> >>>>>> not the clock controller (which clocks are effectively useless if there's no user).
> >>>>>
> >>>>> No. See my previous paragraph.
> >>>>>
> >>>>> ChenYu
> >>>>>
> >>>>>> Angelo
> >>>>>>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> Changes in v3:
> >>>>>>>> - Update the commit message and the comments before runtime PM call
> >>>>>>>>
> >>>>>>>> Changes in v2:
> >>>>>>>> - Fix the order of error handling
> >>>>>>>> - Update the commit message and add a comment before the runtime PM call
> >>>>>>>>
> >>>>>>>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
> >>>>>>>> drivers/clk/mediatek/clk-mtk.h | 2 ++
> >>>>>>>> 2 files changed, 21 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> >>>>>>>> index 2e55368dc4d8..ba1d1c495bc2 100644
> >>>>>>>> --- a/drivers/clk/mediatek/clk-mtk.c
> >>>>>>>> +++ b/drivers/clk/mediatek/clk-mtk.c
> >>>>>>>> @@ -13,6 +13,7 @@
> >>>>>>>> #include <linux/of.h>
> >>>>>>>> #include <linux/of_address.h>
> >>>>>>>> #include <linux/platform_device.h>
> >>>>>>>> +#include <linux/pm_runtime.h>
> >>>>>>>> #include <linux/slab.h>
> >>>>>>>>
> >>>>>>>> #include "clk-mtk.h"
> >>>>>>>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>>>>>>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> +
> >>>>>>>> + if (mcd->need_runtime_pm) {
> >>>>>>>> + devm_pm_runtime_enable(&pdev->dev);
> >>>>>>>> + /*
> >>>>>>>> + * Do a pm_runtime_resume_and_get() to workaround a possible
> >>>>>>>> + * deadlock between clk_register() and the genpd framework.
> >>>>>>>> + */
> >>>>>>>> + r = pm_runtime_resume_and_get(&pdev->dev);
> >>>>>>>> + if (r)
> >>>>>>>> + return r;
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> /* Calculate how many clk_hw_onecell_data entries to allocate */
> >>>>>>>> num_clks = mcd->num_clks + mcd->num_composite_clks;
> >>>>>>>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
> >>>>>>>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>>>>>>> goto unregister_clks;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> + if (mcd->need_runtime_pm)
> >>>>>>>> + pm_runtime_put(&pdev->dev);
> >>>>>>>> +
> >>>>>>>> return r;
> >>>>>>>>
> >>>>>>>> unregister_clks:
> >>>>>>>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
> >>>>>>>> free_base:
> >>>>>>>> if (mcd->shared_io && base)
> >>>>>>>> iounmap(base);
> >>>>>>>> +
> >>>>>>>> + if (mcd->need_runtime_pm)
> >>>>>>>> + pm_runtime_put(&pdev->dev);
> >>>>>>>> return r;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> >>>>>>>> index 22096501a60a..c17fe1c2d732 100644
> >>>>>>>> --- a/drivers/clk/mediatek/clk-mtk.h
> >>>>>>>> +++ b/drivers/clk/mediatek/clk-mtk.h
> >>>>>>>> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
> >>>>>>>>
> >>>>>>>> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
> >>>>>>>> unsigned int mfg_clk_idx;
> >>>>>>>> +
> >>>>>>>> + bool need_runtime_pm;
> >>>>>>>> };
> >>>>>>>>
> >>>>>>>> int mtk_clk_pdev_probe(struct platform_device *pdev);
> >>>>>>>> --
> >>>>>>>> 2.43.0.472.g3155946c3a-goog
> >>>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>>>
> >>>>
> >>
>
>
>
Il 12/03/24 10:35, Pin-yen Lin ha scritto:
> Hi Angelo
>
> On Thu, Mar 7, 2024 at 7:22 PM AngeloGioacchino Del Regno
> <[email protected]> wrote:
>>
>> Il 07/03/24 12:10, Pin-yen Lin ha scritto:
>>> Hi Angelo and Chen-yu,
>>>
>>> I tried enabling the runtime PM regardless of the .need_pm_runtime
>>> flag, and my MT8183 device works well with that with no obvious boot
>>> regression.
>>>
>>> Should I send out another patch that always enables runtime PM in
>>> __mtk_clk_simple_probe()? Or is there anything I should test?
>>>
>>
>> Hello Pin-yen,
>>
>> as I discussed with Chen-Yu - yes, we must make sure that this does not
>> create any regression on machines running on other SoC models.
>>
>> I think it's unlikely that it does, but since the HW is available, being
>> extremely careful with validating this change is a good idea :-)
>>
>> If you can/want to test before we do, sure, please send the new patch and,
>> when you do, please say that you tested it and on which SoCs; as long as
>> it's not just one SoC, that'll be good enough for me.
>>
>> P.S.: Please don't top-post!
>>
>> Cheers,
>> Angelo
>
> I just tried this on MT8192 and the system looks healthy as well. I'll
> send out a patch to enable the runtime PM for all mediatek clock
> controllers.
>
> Thanks for your time on reviewing this, and sorry for the top-posting.
>
You're welcome. Looking forward to see the new patch!
Cheers,
Angelo
> Regards,
> Pin-yen
>
>>
>>> Regards,
>>> Pin-yen
>>>
>>> On Thu, Feb 29, 2024 at 6:36 PM AngeloGioacchino Del Regno
>>> <[email protected]> wrote:
>>>>
>>>> Il 29/02/24 11:34, Chen-Yu Tsai ha scritto:
>>>>> On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto:
>>>>>>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno
>>>>>>> <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto:
>>>>>>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <[email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>> Introduce a new need_pm_runtime variable to mtk_clk_desc to indicate
>>>>>>>>>> this clock controller needs runtime PM for its operations.
>>>>>>>>>> Also do a runtime PM get on the clock controller during the
>>>>>>>>>> probing stage to workaround a possible deadlock.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Pin-yen Lin <[email protected]>
>>>>>>>>>
>>>>>>>>> Reviewed-by: Chen-Yu Tsai <[email protected]>
>>>>>>>>>
>>>>>>>>> The patch itself looks fine.
>>>>>>>>>
>>>>>>>>> Besides the MT8183 MFG clock issues, we do actually need this for the
>>>>>>>>> MT8192 ADSP clock. Its power domain is not enabled by default.
>>>>>>>>>
>>>>>>>>
>>>>>>>> ...but on MT8195 the ADSP clock works - because the ADSP node exists.
>>>>>>>
>>>>>>> That's an indirect dependency that should not be relied on. Say the clock
>>>>>>> driver probed but the ADSP hasn't, and you try to read out the current
>>>>>>> status. What would happen?
>>>>>>>
>>>>>>> - Read out works fine, because the power domain is default on, and hasn't
>>>>>>> been turned off by late cleanup
>>>>>>> - Read out is bogus (but you can't tell)
>>>>>>> - Read out hangs.
>>>>>>>
>>>>>>> The third is what happens on MT8192. There's still some issues on that
>>>>>>> front, as even after I applied the ADSP power domain patches from MediaTek,
>>>>>>> the readout was still hanging.
>>>>>>>
>>>>>>
>>>>>> That MT8192 lockup story is getting crazy in my head... anyway, besides that,
>>>>>> I get the point - I was somehow ignoring the fact that kernel modules do exist.
>>>>>>
>>>>>> Eh, sorry about that :-)
>>>>>>
>>>>>>>> This poses a question: should we make clock controllers depend on power domains,
>>>>>>>> or should we keep everything powered off (hence clocks down - no power consumption)
>>>>>>>> *unless* the user exists?
>>>>>>>
>>>>>>> That's a policy discussion separate from actual hardware dependencies.
>>>>>>> *If* the clock controller needs the power domain to be active for the
>>>>>>> registers to be accessed, the clock controller *must* have a direct
>>>>>>> dependency on the power domain.
>>>>>>>
>>>>>>
>>>>>> I admit I should've worded that better.
>>>>>>
>>>>>> "should we make clock controllers depend on power domains" was actually implying
>>>>>> "IF those need one" :-)
>>>>>>
>>>>>> I really wonder if - at this point - it's simply a better idea to not restrict
>>>>>> the call to devm_pm_runtime_enable/resume_and_get to `need_runtime_pm == true`.
>>>>>>
>>>>>> Do we really need to exclude that on other clock controllers that don't have
>>>>>> any power domain dependency? Any side effect?
>>>>>>
>>>>>> Saying this because if we can avoid yet another per-SoC flag I'm really happy,
>>>>>> as readability is also impacted and besides - if we ever find out that one of
>>>>>> those need a power domain in the future, we'll need just one commit and just
>>>>>> only in the devicetree, instead of enabling a flag in driver X as well as that,
>>>>>> avoiding some (potentially unnecessary) noise... I guess.
>>>>>>
>>>>>> P.S.: I just noticed that the return value for the devm_pm_runtime_enable() call
>>>>>> is not being checked!
>>>>>>
>>>>>> .......
>>>>>>
>>>>>> In short....
>>>>>>
>>>>>> Chen-Yu, at this point, do you have any reason why we wouldn't be able and/or it
>>>>>> wouldn't be a good idea to just avoid adding the `need_runtime_pm` flag (meaning
>>>>>> that we perform pm_runtime calls for all clock drivers unconditionally)?
>>>>>>
>>>>>> If this is about longer boot time, I don't think that it's going to be more than
>>>>>> a millisecond or two, so that should be completely ignorable.
>>>>>
>>>>> I think it's just more of a "don't enable features you don't need" thing.
>>>>> We already ran into a weird deadlock, which is why the devm_pm_runtime_enable()
>>>>> call has that comment.
>>>>>
>>>>> I don't think anyone has actually looked at it. As you said it shouldn't be
>>>>> much, at least during boot time. It's one call per clock controller.
>>>>>
>>>>>> Can you please do a test for that, or should I?
>>>>>
>>>>> The earliest I can work on it would be some time next week. Does that work
>>>>> for you?
>>>>>
>>>>
>>>> The earliest I'd be able to work on this myself would be at the end of next
>>>> week if not later.. so yes, please take your time, no worries.
>>>>
>>>> Thank you!
>>>>
>>>>> ChenYu
>>>>>
>>>>>> Cheers
>>>>>> Angelo
>>>>>>
>>>>>>>> For the second one, this means that the *device* gets the power domain (adsp), and
>>>>>>>> not the clock controller (which clocks are effectively useless if there's no user).
>>>>>>>
>>>>>>> No. See my previous paragraph.
>>>>>>>
>>>>>>> ChenYu
>>>>>>>
>>>>>>>> Angelo
>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> Changes in v3:
>>>>>>>>>> - Update the commit message and the comments before runtime PM call
>>>>>>>>>>
>>>>>>>>>> Changes in v2:
>>>>>>>>>> - Fix the order of error handling
>>>>>>>>>> - Update the commit message and add a comment before the runtime PM call
>>>>>>>>>>
>>>>>>>>>> drivers/clk/mediatek/clk-mtk.c | 19 +++++++++++++++++++
>>>>>>>>>> drivers/clk/mediatek/clk-mtk.h | 2 ++
>>>>>>>>>> 2 files changed, 21 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
>>>>>>>>>> index 2e55368dc4d8..ba1d1c495bc2 100644
>>>>>>>>>> --- a/drivers/clk/mediatek/clk-mtk.c
>>>>>>>>>> +++ b/drivers/clk/mediatek/clk-mtk.c
>>>>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>>>> #include <linux/of.h>
>>>>>>>>>> #include <linux/of_address.h>
>>>>>>>>>> #include <linux/platform_device.h>
>>>>>>>>>> +#include <linux/pm_runtime.h>
>>>>>>>>>> #include <linux/slab.h>
>>>>>>>>>>
>>>>>>>>>> #include "clk-mtk.h"
>>>>>>>>>> @@ -494,6 +495,18 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>>>>>>>> return IS_ERR(base) ? PTR_ERR(base) : -ENOMEM;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> + if (mcd->need_runtime_pm) {
>>>>>>>>>> + devm_pm_runtime_enable(&pdev->dev);
>>>>>>>>>> + /*
>>>>>>>>>> + * Do a pm_runtime_resume_and_get() to workaround a possible
>>>>>>>>>> + * deadlock between clk_register() and the genpd framework.
>>>>>>>>>> + */
>>>>>>>>>> + r = pm_runtime_resume_and_get(&pdev->dev);
>>>>>>>>>> + if (r)
>>>>>>>>>> + return r;
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>> /* Calculate how many clk_hw_onecell_data entries to allocate */
>>>>>>>>>> num_clks = mcd->num_clks + mcd->num_composite_clks;
>>>>>>>>>> num_clks += mcd->num_fixed_clks + mcd->num_factor_clks;
>>>>>>>>>> @@ -574,6 +587,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>>>>>>>> goto unregister_clks;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> + if (mcd->need_runtime_pm)
>>>>>>>>>> + pm_runtime_put(&pdev->dev);
>>>>>>>>>> +
>>>>>>>>>> return r;
>>>>>>>>>>
>>>>>>>>>> unregister_clks:
>>>>>>>>>> @@ -604,6 +620,9 @@ static int __mtk_clk_simple_probe(struct platform_device *pdev,
>>>>>>>>>> free_base:
>>>>>>>>>> if (mcd->shared_io && base)
>>>>>>>>>> iounmap(base);
>>>>>>>>>> +
>>>>>>>>>> + if (mcd->need_runtime_pm)
>>>>>>>>>> + pm_runtime_put(&pdev->dev);
>>>>>>>>>> return r;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
>>>>>>>>>> index 22096501a60a..c17fe1c2d732 100644
>>>>>>>>>> --- a/drivers/clk/mediatek/clk-mtk.h
>>>>>>>>>> +++ b/drivers/clk/mediatek/clk-mtk.h
>>>>>>>>>> @@ -237,6 +237,8 @@ struct mtk_clk_desc {
>>>>>>>>>>
>>>>>>>>>> int (*clk_notifier_func)(struct device *dev, struct clk *clk);
>>>>>>>>>> unsigned int mfg_clk_idx;
>>>>>>>>>> +
>>>>>>>>>> + bool need_runtime_pm;
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> int mtk_clk_pdev_probe(struct platform_device *pdev);
>>>>>>>>>> --
>>>>>>>>>> 2.43.0.472.g3155946c3a-goog
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>
>>
>>