linux-kernel - Re: [PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

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]

Message-ID: <011cfbf7-70d0-a649-52de-1689cdb5a10b@mentor.com>
Date:   Thu, 6 Dec 2018 14:04:32 +0900
From:   Jiada Wang <jiada_wang@...tor.com>
To:     Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
CC:     "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "perex@...ex.cz" <perex@...ex.cz>,
        "tiwai@...e.com" <tiwai@...e.com>,
        "vladimir_zapolskiy@...tor.com" <vladimir_zapolskiy@...tor.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH linux-next v3 6/7] ASoC: rsnd: add avb clocks

Hi Morimoto-san

Thanks for your comments

On 2018/12/06 9:59, Kuninori Morimoto wrote:
> Hi Jiada
>
>>>> +	avb = devm_kzalloc(dev, sizeof(*avb), GFP_KERNEL);
>>>> +	if (!avb)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	parent_name = __clk_get_name(adg->clkadg);
>>> This parent_name is very strange to me.
>>> AVB parent clk is "AUDIO_CLK_A/B/C/I" (= clk_a/b/c/i in this driver)
>>> or "AUDIO_CLK_OUT_A/B/C/D" (= audio_clkout/1/2/3 in this driver).
>>> And we don't have "adg" clock.
>>> Please double check it.
>> I have some local device-tree change, which expends 'adg' register
>> range and add
>> "adg" clock to rcar_sound node which refer to newly added 'adg' clock
>> (S0D1ϕ) in this patch-set
> (snip)
>> -                       clocks = <&cpg CPG_MOD 1005>,
>> +                       clocks = <&cpg CPG_MOD 922>, <&cpg CPG_MOD 1005>,
>>                                   <&cpg CPG_MOD 1006>, <&cpg CPG_MOD 1007>,
>>                                   <&cpg CPG_MOD 1008>, <&cpg CPG_MOD 1009>,
>>                                   <&cpg CPG_MOD 1010>, <&cpg CPG_MOD 1011>,
>> @@ -1856,7 +1856,7 @@
>>                                   <&audio_clk_a>, <&audio_clk_b>,
>>                                   <&audio_clk_c>,
>>                                   <&cpg CPG_CORE R8A7795_CLK_S0D4>;
>> -                       clock-names = "ssi-all",
>> +                       clock-names = "adg", "ssi-all",
>>                                        "ssi.9", "ssi.8", "ssi.7", "ssi.6",
>>                                        "ssi.5", "ssi.4", "ssi.3", "ssi.2",
>>                                        "ssi.1", "ssi.0",
> Noooo !!
> It breaks sound clock / module stop controling !!
Can you let me know how it breaks sound clock / module stop
so that I can come up with a fix
> OK, now I checked more detail of ADG for EAVB/IF.
>
> 1st, I don't think we need to add "adg" clock.
> It is not exist on Gen2, and default ON on Gen3.
> If you want to add it, please add it to "last" of clocks, not "first".
> "first" clock is handling whole SSI power.
as this device-tree change is only local, I will move 'adg' clock to 
'last' of clocks,
when I submit it.
> 2nd, it is "Module stop clock", not "S0D1ϕ".
> We already handling it as "clk_i".
SMSTPCR922 controls input of two clocks "S0D1ϕ" and "S0D4ϕ",
"S0D4ϕ" is input to BRGA,
"S0D1ϕ" is input to avb_counter8
>
> 3rd, we need to create new clock/handler for
> avb_counter8 / audio_clk_div3 / avb_div8 for "internal" purpose,
> and need to create avb_adg_syn[] clock for "external" purpose for EAVB/IF.
> Your code is creating / registering adg->clkavb[],
> but it is for avb_counter8 in my understanding.
> We don't need to register it as formal clock IMO.
I agree, besides avb_counter8 there are other clocks which need to be 
added as you have mentioned,
in this patch-set, I only added avb_counter8, because I want to keep the 
patch-set simple,
other clocks can be added later.

avb_counter8 is registered as formal clock is because,
there is use case that EAVB-IF may use avb_div8, and avb_counter8 is 
input to avb_div8.
if keeps avb_counter8 'internal', I am not sure how EAVB-IF can set 
clock rate for avb_counter8
> 4th, EAVB driver need to get clock from AVB via DT
> as "avb_adg_syn[]" clock, not "avb_counter8".
>
> I'm not sure EAVB side driver, but please double check these.
yes, EAVB driver only needs to get 'avb_adg_sync[]' clock,
and avb_adg_sync clock is child clock of audio_clk_div3 / avb_div8


Thanks,
Jiada
>
>
> Best regards
> ---
> Kuninori Morimoto