[PATCH 08/26] power: Add iMX8M block ctrl driver for dispmix

[Michael Nazzareno Trimarchi]

Hi

On Tue, Oct 1, 2024 at 12:01 PM Miquel Raynal <miquel.raynal at bootlin.com> wrote:
>
> Hi Michael,
>
> > > >>>> Ported the patch from patchset
> > > >>>>
> > > >>>> "[PATCH 05/26] clk: imx8mm: Mark IMX8MM_SYS_PLL2 and IMX8MM_SYS_PLL3 as enabled"
> > > >>>>
> > > >>>> to imx8mp [2] and fec ethernet works again for me on imx8mp!
> > > >>>>
> > > >>>> Could you add this if you post a v2 ?
> > > >>>
> > > >>> TBH I don't feel like the below change is the correct one, it is too
> > > >>> specific. The clock core is recursive and thus should handle the
> > > >>> reparenting situations gracefully.
> > > >>>
> > > >>> I posted a series that is targeting the LVDS output on imx8mp. You
> > > >>> should probably consider checking these patches as well if you work
> > > >>> on imx8mp as well. I also had similar breakages with Ethernet which
> > > >>> happened during the assigned-clocks handling. This patch, which is more
> > > >>> future and platform agnostic, fixed it:
> > > >>>
> > > >>> https://lore.kernel.org/u-boot/20240910101344.110633-3-miquel.raynal@bootlin.com/
> > > >>>
> > > >>
> > > >> The clock patches are not specific at all. You need to have it working
> > > >> for the parent for each component.
> > > >
> > > > The diff shown by Heiko is explicitly enabling PLLs by naming them in
> > > > the iMX driver. This is not the correct approach. The problem of
> > > > having non-enabled new parents is global. Parent clocks should be
> > > > enabled before changing muxes, and this should be enforced
> > > > by the clock core/uclass, not the SoC drivers.
> > >
> > > Okay, valid argument.
> > >
> > > >
> > > >> This is a standard way to do it and nothing magic compared to other
> > > >> implementations.
> > > >
> > > > No, naming PLLs explicitly is not the correct approach.
> > > >
> > > >> I don't see
> > > >> in your series any addressing or reparent clock or take in account
> > > >> that a clock should be enable before
> > > >> reparenting.
> > > >
> > > > That's exactly the link above, whose diff is pasted here for reference:
> > > >
> > > > @@ -595,6 +595,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
> > > >       if (!ops->set_parent)
> > > >               return -ENOSYS;
> > > >
> > > > +     ret = clk_enable(parent);
> > > > +     if (ret)
> > > > +             return ret;
> > >
> > > As I said before, I had *exact* the same patch and thought I made a big
> > > hack :-P
> > >
> > > But I wonder ... if this a generic "problem", why nobody had yet problems
> > > with it...
> >
> > I think that a generic approach that takes into account the reparent
> > is more valuable, then
> > this.
>
> I'm sorry I don't fully understand your answer. I assume you agree with
> the generic approach quoted above.
>
> > If a clock is enabled by another stage and we don't aware about
> > it we need to mark
> > as enabled.
>
> clk_enable() already handles this kind of situation.
>
> > I think that this force of enable it's just a short path
> > that does not solve the generic
> > problem.
>
> What "force of enable"? The parent needs to be enabled before we use it
> as parent. The logic is always the same:
> - clk_enable() the new parent
> - change the parent
> - clk_disable() the old parent
> I don't see any short path here.
>
> > I really tried to abstract what is really implemented in
> > other OS on the same topic.
>
> FYI, Linux does the clk_enable(parent) like above.
>

You mean linux enables a clock in set_parent even if the clock_chain
is not yet enabled.
clock a disable
clock b disable

clk_set_parent(a, b) b->a now b is enabled?

Michael

> Thanks,
> Miquèl



-- 
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael at amarulasolutions.com
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info at amarulasolutions.com
www.amarulasolutions.com