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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 22 Oct 2022 18:15:05 +0100
From:   Aidan MacDonald <aidanmacdonald.0x0@...il.com>
To:     Zhou Yanjie <zhouyu@...yeetech.com>
Cc:     Paul Cercueil <paul@...pouillou.net>, lgirdwood@...il.com,
        broonie@...nel.org, perex@...ex.cz, tiwai@...e.com,
        linux-mips@...r.kernel.org, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name
 SoC-specific


Zhou Yanjie <zhouyu@...yeetech.com> 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@...yeetech.com>
>> 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.

Since the driver only supports the internal codec, which requires the
"ext" clock, there isn't a problem in practice.

I'm just going to drop this patch and leave .set_sysclk() alone for now.
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.

Regards,
Aidan