Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Paul Cercueil]

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 :
> > >  Zhou Yanjie <zhouyu@xxxxxxxxxxxxxx> writes:
> > >
> > > >   Hi Paul,
> > > >   On 2022/7/13 下午11:07, Paul Cercueil wrote:
> > > > >   Hi Zhou,
> > > > >   Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie
> > > > >  <zhouyu@xxxxxxxxxxxxxx>
> > > > >   a écrit :
> > > > > >   Hi Aidan,
> > > > > >   On 2022/7/9 上午12:02, Aidan MacDonald wrote:
> > > > > > >   @@ -400,6 +402,7 @@ static const struct i2s_soc_info 
> > > > > > > jz4740_i2s_soc_info
> > > > > > >  =
> > > > > > >   {
> > > > > > >         .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 
> > > > > > > 8, 11),
> > > > > > >         .field_i2sdiv_capture    = 
> > > > > > > REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> > > > > > >         .field_i2sdiv_playback    = 
> > > > > > > REG_FIELD(JZ_REG_AIC_CLK_DIV, 0, 3),
> > > > > > >   +    .pll_clk_name        = "pll half",
> > > > > > >         .shared_fifo_flush    = true,
> > > > > > >     };
> > > > > >   Since JZ4760, according to the description of the I2SCDR 
> > > > > > register,
> > > > > >   Ingenic SoCs no longer use PLL/2 clock, but directly use PLL 
> > > > > > clock,
> > > > > >   so it seems also inappropriate to use "pll half" for these 
> > > > > > SoCs.
> > > > >   The device tree passes the clock as "pll half". So the driver 
> > > > > should use
> > > > >  this
> > > > >   name as well...
> > > >   I see...
> > > >   It seems that the device tree of JZ4770 has used "pll half" 
> > > > already,
> > > >   but there is no "pll half" used anywhere in the device tree of 
> > > > JZ4780,
> > > >   maybe we can keep the pll_clk_name of JZ4770 as "pll half", and 
> > > > change
> > > >   the pll_clk_name of JZ4780 to a more reasonable name.
> > > >   Thanks and best regards!
> > >  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.

> 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