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]

Date:   Thu, 6 Dec 2018 05:38:02 +0000
From:   Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
To:     Jiada Wang <jiada_wang@...tor.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 Jiada

> SMSTPCR922 controls input of two clocks "S0D1ϕ" and "S0D4ϕ",

Ahh, OK I could check it via Block diagram.
But it is "Module stop" for these, anyway.
"Module stop" and "S0D1ϕ/S0D4ϕ" are completely different clocks.

> "S0D4ϕ" is input to BRGA,
> "S0D1ϕ" is input to avb_counter8

It seems we need to update "clk_i" portion first for this purpose.
Then, we need to consider about Gen2 vs Gen3, etc, etc...
It seems we can't keep patch simple...

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

No, No, No.
avb_counter8 is just "internal" clock, and one of
parent clock for avb_div8 which is needed for avb_adg_syn[].
We need to control/register is avb_adg_syn[], not avb_counter8.

Your approach on this patch is very "Quick-Hack".
I have no objection for out-of-tree patches.
But, "Quick-Hack approach with changing formal DT on upstream"
is nightmare.

> 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

EAVB/IF driver setups avb_adg_syn[].
Then, ADG considers/searchs avb_counter8/avb_div8/audio_clk_div3
settings internally.
Don't control avb_counter8 directly from EAVB.

Best regards
---
Kuninori Morimoto