Paul Cercueil <paul@xxxxxxxxxxxxxxx> writes: > Hi Aidan, > > Le dim. 23 oct. 2022 à 14:29:24 +0100, Aidan MacDonald > <aidanmacdonald.0x0@xxxxxxxxx> a écrit : >> Paul Cercueil <paul@xxxxxxxxxxxxxxx> writes: >> >>> Hi Aidan, >>> Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald >>> <aidanmacdonald.0x0@xxxxxxxxx> 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
