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

[Jiada Wang]

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