Hi all On Tue, Oct 1, 2024 at 10:59 AM Heiko Schocher <h...@denx.de> wrote: > > Hello Miquel, > > On 01.10.24 10:50, Miquel Raynal wrote: > > Hi Michael, > > > > mich...@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.ray...@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 <mich...@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 <mich...@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.ray...@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: h...@denx.de -- Michael Nazzareno Trimarchi Co-Founder & Chief Executive Officer M. +39 347 913 2170 mich...@amarulasolutions.com __________________________________ Amarula Solutions BV Joop Geesinkweg 125, 1114 AB, Amsterdam, NL T. +31 (0)85 111 9172 i...@amarulasolutions.com www.amarulasolutions.com
