Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20220708160244.21933-1-aidanmacdonald.0x0@gmail.com>
 <20220708160244.21933-8-aidanmacdonald.0x0@gmail.com>
 <0269b850-f33a-7aa9-a3eb-83655bd4e19a@wanyeetech.com>
 <ROSYER.QTJF8J14H2YX1@crapouillou.net>
 <6f2c7a0b-b68b-fc42-1a82-2b69c114823f@wanyeetech.com>
 <UQ597w4FmzOT8p76tdRPhzECStUpFmYe@localhost>
 <HQ76KR.B1PM87E2GIYZ1@crapouillou.net>
From:   Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
To:     Paul Cercueil <paul@crapouillou.net>
Cc:     Zhou Yanjie <zhouyu@wanyeetech.com>, lgirdwood@gmail.com,
        broonie@kernel.org, perex@perex.cz, tiwai@suse.com,
        linux-mips@vger.kernel.org, alsa-devel@alsa-project.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name
 SoC-specific
In-reply-to: <HQ76KR.B1PM87E2GIYZ1@crapouillou.net>
Date:   Sun, 23 Oct 2022 14:29:24 +0100
Message-ID: <JRrsX7Q4m5FUeeBs2yKTRMe5Ihaso8gi@localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


Paul Cercueil <paul@crapouillou.net> writes:

> Hi Aidan,
>
> Le sam. 22 oct. 2022 =C3=A0 18:15:05 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a =C3=A9crit :
>> Zhou Yanjie <zhouyu@wanyeetech.com> writes:
>>
>>>  Hi Paul,
>>>  On 2022/7/13 =E4=B8=8B=E5=8D=8811:07, Paul Cercueil wrote:
>>>>  Hi Zhou,
>>>>  Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie
>>>> <zhouyu@wanyeetech.com>
>>>>  a =C3=A9crit :
>>>>>  Hi Aidan,
>>>>>  On 2022/7/9 =E4=B8=8A=E5=8D=8812:02, Aidan MacDonald wrote:
>>>>>>  @@ -400,6 +402,7 @@ static const struct i2s_soc_info jz4740_i2s_soc=
_info
>>>>>> =3D
>>>>>>  {
>>>>>>        .field_tx_fifo_thresh    =3D REG_FIELD(JZ_REG_AIC_CONF, 8, 11=
),
>>>>>>        .field_i2sdiv_capture    =3D REG_FIELD(JZ_REG_AIC_CLK_DIV, 0,=
 3),
>>>>>>        .field_i2sdiv_playback    =3D REG_FIELD(JZ_REG_AIC_CLK_DIV, 0=
, 3),
>>>>>>  +    .pll_clk_name        =3D "pll half",
>>>>>>        .shared_fifo_flush    =3D 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.

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...

>> 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.

Regards,
Aidan