Paul Cercueil paul@crapouillou.net writes:
Hi Aidan,
Le dim. 23 oct. 2022 à 14:29:24 +0100, Aidan MacDonald aidanmacdonald.0x0@gmail.com a écrit :
Paul Cercueil paul@crapouillou.net writes:
Hi Aidan, Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald aidanmacdonald.0x0@gmail.com a écrit :
Actually, the clock names in the DT are meaningless. The clk_get() call matches only the clock's name in the CGU driver. So in fact the driver is "broken" for jz4780. It seems jz4770 doesn't work correctly either, it has no "pll half", and three possible parents for its "i2s" clock.
That's not true. The clock names are matched via DT. Only in the case where a corresponding clock cannot be found via DT will it search for the clock name among the clock providers. I believe this is a legacy mechanism and you absolutely shouldn't rely on it. -Paul
What you say is only true for clk_get() with a device argument. When the device argument is NULL -- which is the case in .set_sysclk() -- then the DT name is not matched. Check drivers/clk/clkdev.c, in clk_find(). When the dev_id is NULL, it will not match any lookup entries with a non-null dev_id, and I believe dev_id is the mechanism that implements DT clock lookup. Only the wildcard entries from the CGU driver will be matched if dev_id is NULL, so the DT is being ignored. If you don't believe me, try changing "pll half" in the device tree and the I2S driver to something else. I have done this, and it doesn't work. That proves the name in the device tree is not being used.
Well, let's pass them a device pointer then.
Yes, I'll do that when I revise the patch.
I agree we shouldn't rely on this, it's a legacy behavior, but the fact is that's how the driver already works. I'm dropping this patch because the driver is wrong and needs a different fix...
"How the driver already works" is a bit misleading, I never saw this .set_sysclk() callback being called, so I can't really say that it works.
I think a better approach is to have the DT define an array of parent clocks for .set_sysclk()'s use, instead of hardcoding parents in the driver. If the parent array is missing the driver can default to using "ext" so existing DTs will work.
As much as I like this idea there doesn't seem to be a mechanism for handling a free-floating array of clocks in the DT. Everything has to be put in the main "clocks" array. That makes it pretty hard to figure out which ones are meant to be the parent clocks. Do you know of any way to do this generically from the DT? If there's no way to get away from a hardcoded array of names in the driver, I can at least add a device argument to clk_get() so it'll use the DT names.
In jz4740_i2s_set_sysclk():
#define JZ4740_I2S_FIRST_PARENT_CLK 2 parent = of_clk_get(dev->of_node, JZ4740_I2S_FIRST_PARENT_CLK + clk_id);
is how I'd do it.
The DTs all have "aic", "i2s" as the first two clocks. It is even enforced in the DT schemas.
Cheers, -Paul
Sounds like a plan. I was hoping to avoid adding CONFIG_OF back considering I removed it in an earlier patch since it was unused. :)
Guess it doesn't really matter for this driver since the Ingenic SoCs need CONFIG_OF anyway.
Regards, Aidan