| Message ID | 20240108081834.408403-1-treapking@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] clk: mediatek: Introduce need_pm_runtime to mtk_clk_desc | expand |
On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> 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 >
Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: > On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > > 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 >>
On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: > > On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> > > > > Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > > > > 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 08:17, Chen-Yu Tsai ha scritto: > On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> >> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: >>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> >>> >>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> >>> >>> 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 >>>> >> >> >>
On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 29/02/24 08:17, Chen-Yu Tsai ha scritto: > > On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@collabora.com> wrote: > >> > >> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: > >>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> > >>> > >>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > >>> > >>> 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 11:34, Chen-Yu Tsai ha scritto: > On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> >> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto: >>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno >>> <angelogioacchino.delregno@collabora.com> wrote: >>>> >>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: >>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> >>>>> >>>>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> >>>>> >>>>> 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 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 <angelogioacchino.delregno@collabora.com> wrote: > > Il 29/02/24 11:34, Chen-Yu Tsai ha scritto: > > On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@collabora.com> wrote: > >> > >> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto: > >>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno > >>> <angelogioacchino.delregno@collabora.com> wrote: > >>>> > >>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: > >>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> > >>>>> > >>>>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > >>>>> > >>>>> 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 > <angelogioacchino.delregno@collabora.com> wrote: >> >> Il 29/02/24 11:34, Chen-Yu Tsai ha scritto: >>> On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno >>> <angelogioacchino.delregno@collabora.com> wrote: >>>> >>>> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto: >>>>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno >>>>> <angelogioacchino.delregno@collabora.com> wrote: >>>>>> >>>>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: >>>>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> >>>>>>> >>>>>>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> >>>>>>> >>>>>>> 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 <angelogioacchino.delregno@collabora.com> 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 > > <angelogioacchino.delregno@collabora.com> wrote: > >> > >> Il 29/02/24 11:34, Chen-Yu Tsai ha scritto: > >>> On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno > >>> <angelogioacchino.delregno@collabora.com> wrote: > >>>> > >>>> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto: > >>>>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno > >>>>> <angelogioacchino.delregno@collabora.com> wrote: > >>>>>> > >>>>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: > >>>>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> > >>>>>>> > >>>>>>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> > >>>>>>> > >>>>>>> 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 > <angelogioacchino.delregno@collabora.com> 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 >>> <angelogioacchino.delregno@collabora.com> wrote: >>>> >>>> Il 29/02/24 11:34, Chen-Yu Tsai ha scritto: >>>>> On Thu, Feb 29, 2024 at 5:45 PM AngeloGioacchino Del Regno >>>>> <angelogioacchino.delregno@collabora.com> wrote: >>>>>> >>>>>> Il 29/02/24 08:17, Chen-Yu Tsai ha scritto: >>>>>>> On Mon, Feb 26, 2024 at 7:16 PM AngeloGioacchino Del Regno >>>>>>> <angelogioacchino.delregno@collabora.com> wrote: >>>>>>>> >>>>>>>> Il 23/02/24 05:27, Chen-Yu Tsai ha scritto: >>>>>>>>> On Mon, Jan 8, 2024 at 4:18 PM Pin-yen Lin <treapking@chromium.org> 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 <treapking@chromium.org> >>>>>>>>> >>>>>>>>> Reviewed-by: Chen-Yu Tsai <wenst@chromium.org> >>>>>>>>> >>>>>>>>> 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 >>>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>> >>>>>> >>>>>> >>>> >> >> >>
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);
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 <treapking@chromium.org> --- 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(+)