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