Hi On Tue, Oct 1, 2024 at 12:01 PM Miquel Raynal <miquel.ray...@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.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. > > 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 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
