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

Tue Oct 1 11:50:59 CEST 2024

Hi all

On Tue, Oct 1, 2024 at 10:59 AM Heiko Schocher <hs at denx.de> wrote:
>
> Hello Miquel,
>
> On 01.10.24 10:50, Miquel Raynal wrote:
> > Hi Michael,
> >
> > michael at amarulasolutions.com wrote on Tue, 1 Oct 2024 10:33:56 +0200:
> >
> >> Hi Miguel
> >>
> >> On Tue, Oct 1, 2024 at 10:29 AM Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> >>>
> >>> Hi Heiko,
> >>>
> >>>
> >>>>>>> Hmm.. unfortunately ... I had applied your 2 clock patches, which
> >>>>>>> fixed a problem with enabling parent clocks ... but they broke booting
> >>>>>>> on a carrier which has fec ethernet! After "Net: " output the board hang...
> >>>>>>>
> >>>>>>> I reverted your 2 clock patches and it bootet again ... so there is
> >>>>>>> a problem ... try to get some more time to look into...
> >>>>>>>
> >>>>>>
> >>>>>> We have a fec, but we had I think two patches more on it. I forget to
> >>>>>> answer Marek
> >>>>>> about them because I don't have my board now and because he is
> >>>>>> partially right (or maybe right).
> >>>>>> Anyway when we boot we could have and we have clocks that are enabled
> >>>>>> by bootrom or SPL that
> >>>>>> we need to declare as enable like PLL2/PLL3 those clocks are parts or
> >>>>>> could be part of reparent so,
> >>>>>> you need to have a reference counter on them that allow to not disable
> >>>>>> during the down chain disable
> >>>>>> and up chain enable. I think that what happens to your ethernet it's
> >>>>>> that you disable some clock that is
> >>>>>> critical to the board to survive. I had a patch merged by Tom that
> >>>>>
> >>>>> Yep, thats what I think too! If you access registers in a block for
> >>>>> which the clock is not enabled ... it just hang...
> >>>>>
> >>>>>> prints the clock name so if you enable
> >>>>>> the debug of the clock you will find that your board stops working
> >>>>>> during one of this reparinting.
> >>>>>
> >>>>> I currently work on 2024.07... will rebase if 2024.10 is out...
> >>>>>
> >>>>> Ah, you mean:
> >>>>>
> >>>>> commit a70d991212c9684e09ed80ece69ce1ff7bfd9f08
> >>>>> Author: Michael Trimarchi <michael at amarulasolutions.com>
> >>>>> Date:   Tue Jul 9 08:28:13 2024 +0200
> >>>>>
> >>>>>       clk: clk-uclass: Print clk name in clk_enable/clk_disable
> >>>>>
> >>>>>       Print clk name in clk_enable and clk_disable. Make sense to know
> >>>>>       what clock get disabled/enabled before a system crash or system
> >>>>>       hang.
> >>>>>
> >>>>>       Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
> >>>>>
> >>>>> I have the same change when I debug :-P
> >>>>>
> >>>>> IIRC I did not see the clock names ... but I will recheck!
> >>>>
> >>>> I see with your patch the clock names, so fine... and see [1]
> >>>>
> >>>> Hmm... I am on imx8mp, and I think the changes the patchset do in
> >>>>
> >>>> "clk: imx8mn: Prevent clock critical path from disabling during reparent and set_rate"
> >>>>
> >>>> are in clk-imx8mp already ...
> >>>>
> >>>> 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. If a clock is enabled by another stage and we don't aware about
it we need to mark
as enabled. I think that this force of enable it's just a short path
that does not solve the generic
problem. I really tried to abstract what is really implemented in
other OS on the same topic.
I don't want to have a discussion about who should merge first.

Michael

>
> Thanks!
>
> bye,
> Heiko
>
> >>> My series is complimentary, even though there are some overlaps that we
> >>> need to merge.
> >>
> >> The only collision I can see from your series is that you re-write the
> >> imx approach of power domain.
> >> Can you please expand here a bit your point?
> >
> > Well, that's what I'd call an overlap :) There are also similar changes
> > in the clock core IIRC. Well, there is a bit of merging that needs to
> > happen I guess, but if you don't think there is any then my series can
> > enter as-is (once the comments addressed, ofc).
> >
> > Thanks,
> > Miquèl
> >
>
> --
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

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