LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Trevor Wu <trevor.wu@mediatek.com>
To: Chen-Yu Tsai <wenst@chromium.org>
Cc: Chun-Jie Chen <chun-jie.chen@mediatek.com>, <broonie@kernel.org>,
	<tiwai@suse.com>, Rob Herring <robh+dt@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	<alsa-devel@alsa-project.org>,
	<linux-mediatek@lists.infradead.org>,
	<linux-arm-kernel@lists.infradead.org>,
	LKML <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<bicycle.tsai@mediatek.com>, Jiaxin Yu <jiaxin.yu@mediatek.com>,
	"Jimmy Cheng-Yi Chiang" <cychiang@google.com>,
	Li-Yu Yu <aaronyu@google.com>
Subject: Re: [PATCH v2 5/8] ASoC: mediatek: mt8195: add platform driver
Date: Mon, 26 Jul 2021 22:31:06 +0800	[thread overview]
Message-ID: <fc12d71febfebda12e57b287de0ad2668db20ffa.camel@mediatek.com> (raw)
In-Reply-To: <CAGXv+5HOL5j4gjgJa0RLjvf3+Vt2sGZcWU1KQXwjn7DOYy-JWg@mail.gmail.com>

On Fri, 2021-07-23 at 14:27 +0800, Chen-Yu Tsai wrote:
> On Thu, Jul 22, 2021 at 4:56 PM Trevor Wu <trevor.wu@mediatek.com>
> wrote:
> > 
> > On Mon, 2021-07-19 at 18:05 +0800, Chen-Yu Tsai wrote:
> > > Hi,
> > > 
> > > On Thu, Jul 15, 2021 at 7:05 PM Trevor Wu <trevor.wu@mediatek.com
> > > >
> > > wrote:
> > > > 
> > > > On Tue, 2021-07-13 at 14:00 +0800, Chen-Yu Tsai wrote:
> > > > > On Mon, Jul 12, 2021 at 11:10 PM Trevor Wu <
> > > > > trevor.wu@mediatek.com>
> > > > > wrote:
> > > > > > 
> > > > > > On Mon, 2021-07-12 at 14:57 +0800, Chen-Yu Tsai wrote:
> > > > > > >  are all internal Hi,
> > > > > > > 
> > > > > > > On Tue, Jun 29, 2021 at 9:49 AM Trevor Wu <
> > > > > > > trevor.wu@mediatek.com
> > > > > > > > 
> > > > > > > 
> > > > > > > wrote:
> > > > > > > > 
> > > > > > > > This patch adds mt8195 platform and affiliated driver.
> > > > > > > > 
> > > > > > > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
> > > > > > > > ---
> > > > > > > >  sound/soc/mediatek/Kconfig                     |    9
> > > > > > > > +
> > > > > > > >  sound/soc/mediatek/Makefile                   |    1 +
> > > > > > > >  sound/soc/mediatek/mt8195/Makefile            |   11 +
> > > > > > > >  sound/soc/mediatek/mt8195/mt8195-afe-clk.c    |  899
> > > > > > > > +++++
> > > > > > > >  sound/soc/mediatek/mt8195/mt8195-afe-clk.h    |  201 +
> > > > > > > >  sound/soc/mediatek/mt8195/mt8195-afe-common.h |  200 +
> > > > > > > >  sound/soc/mediatek/mt8195/mt8195-afe-pcm.c    | 3264
> > > > > > > > +++++++++++++++++
> > > > > > > >  sound/soc/mediatek/mt8195/mt8195-reg.h        | 2793
> > > > > > > > ++++++++++++++
> > > > > > > >  8 files changed, 7378 insertions(+)
> > > > > > > >  create mode 100644 sound/soc/mediatek/mt8195/Makefile
> > > > > > > >  create mode 100644 sound/soc/mediatek/mt8195/mt8195-
> > > > > > > > afe-
> > > > > > > > clk.c
> > > > > > > >  create mode 100644 sound/soc/mediatek/mt8195/mt8195-
> > > > > > > > afe-
> > > > > > > > clk.h
> > > > > > > >  create mode 100644 sound/soc/mediatek/mt8195/mt8195-
> > > > > > > > afe-
> > > > > > > > common.h
> > > > > > > >  create mode 100644 sound/soc/mediatek/mt8195/mt8195-
> > > > > > > > afe-
> > > > > > > > pcm.c
> > > > > > > >  create mode 100644 sound/soc/mediatek/mt8195/mt8195-
> > > > > > > > reg.h
> > > > > > > > 
> > > > > > > > diff --git a/sound/soc/mediatek/Kconfig
> > > > > > > > b/sound/soc/mediatek/Kconfig
> > > > > > > > index 74dae4332d17..3389f382be06 100644
> > > > > > > > --- a/sound/soc/mediatek/Kconfig
> > > > > > > > +++ b/sound/soc/mediatek/Kconfig
> > > > > > > > @@ -184,3 +184,12 @@ config
> > > > > > > > SND_SOC_MT8192_MT6359_RT1015_RT5682
> > > > > > > >           with the MT6359 RT1015 RT5682 audio codec.
> > > > > > > >           Select Y if you have such device.
> > > > > > > >           If unsure select "N".
> > > > > > > > +
> > > > > > > > +config SND_SOC_MT8195
> > > > > > > > +       tristate "ASoC support for Mediatek MT8195
> > > > > > > > chip"
> > > > > > > > +       select SND_SOC_MEDIATEK
> > > > > > > > +       help
> > > > > > > > +         This adds ASoC platform driver support for
> > > > > > > > Mediatek
> > > > > > > > MT8195 chip
> > > > > > > > +         that can be used with other codecs.
> > > > > > > > +         Select Y if you have such device.
> > > > > > > > +         If unsure select "N".
> > > > > > > > diff --git a/sound/soc/mediatek/Makefile
> > > > > > > > b/sound/soc/mediatek/Makefile
> > > > > > > > index f6cb6b8508e3..34778ca12106 100644
> > > > > > > > --- a/sound/soc/mediatek/Makefile
> > > > > > > > +++ b/sound/soc/mediatek/Makefile
> > > > > > > > @@ -5,3 +5,4 @@ obj-$(CONFIG_SND_SOC_MT6797) += mt6797/
> > > > > > > >  obj-$(CONFIG_SND_SOC_MT8173) += mt8173/
> > > > > > > >  obj-$(CONFIG_SND_SOC_MT8183) += mt8183/
> > > > > > > >  obj-$(CONFIG_SND_SOC_MT8192) += mt8192/
> > > > > > > > +obj-$(CONFIG_SND_SOC_MT8195) += mt8195/
> > > > > > > > diff --git a/sound/soc/mediatek/mt8195/Makefile
> > > > > > > > b/sound/soc/mediatek/mt8195/Makefile
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..b2c9fd88f39e
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/sound/soc/mediatek/mt8195/Makefile
> > > > > > > > @@ -0,0 +1,11 @@
> > > > > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > > > > +
> > > > > > > > +# platform driver
> > > > > > > > +snd-soc-mt8195-afe-objs := \
> > > > > > > > +       mt8195-afe-clk.o \
> > > > > > > > +       mt8195-afe-pcm.o \
> > > > > > > > +       mt8195-dai-adda.o \
> > > > > > > > +       mt8195-dai-etdm.o \
> > > > > > > > +       mt8195-dai-pcm.o
> > > > > > > > +
> > > > > > > > +obj-$(CONFIG_SND_SOC_MT8195) += snd-soc-mt8195-afe.o
> > > > > > > > diff --git a/sound/soc/mediatek/mt8195/mt8195-afe-clk.c
> > > > > > > > b/sound/soc/mediatek/mt8195/mt8195-afe-clk.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..57aa799b4f41
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/sound/soc/mediatek/mt8195/mt8195-afe-clk.c
> > > > > > > > @@ -0,0 +1,899 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > +/*
> > > > > > > > + * mt8195-afe-clk.c  --  Mediatek 8195 afe clock ctrl
> > > > > > > > + *
> > > > > > > > + * Copyright (c) 2021 MediaTek Inc.
> > > > > > > > + * Author: Bicycle Tsai <bicycle.tsai@mediatek.com>
> > > > > > > > + *         Trevor Wu <trevor.wu@mediatek.com>
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/clk.h>
> > > > > > > > +
> > > > > > > > +#include "mt8195-afe-common.h"
> > > > > > > > +#include "mt8195-afe-clk.h"
> > > > > > > > +#include "mt8195-reg.h"
> > > > > > > > +
> > > > > > > > +static const char *aud_clks[MT8195_CLK_NUM] = {
> > > > > > > 
> > > > > > > Most of these clocks are not described in the device tree
> > > > > > > binding. If
> > > > > > > the driver needs to reference them, they should be
> > > > > > > described.
> > > > > > > We
> > > > > > > should
> > > > > > > not be hard-coding clock names across different drivers.
> > > > > > > 
> > > > > > 
> > > > > > Sorry, I didn't know I have to list all clocks in the dt-
> > > > > > binding.
> > > > > > Originally, I thought these clocks will be described in the
> > > > > > clock
> > > > > > binding, so I didn't add them to the binding of afe driver.
> > > > > > I will add these clocks to mt8195-afe-pcm.yaml.
> > > > > 
> > > > > If the device consumes clocks, then the clocks that get
> > > > > consumed
> > > > > should
> > > > > be listed in the device's bindings. This is not related to
> > > > > the
> > > > > clock
> > > > > bindings, which is a clock provider.
> > > > > 
> > > > 
> > > > Got it. Thanks.
> > > > 
> > > > > > > The more important question is, why does the driver need
> > > > > > > to
> > > > > > > reference
> > > > > > > all of them? Maybe we should take a step back and draw
> > > > > > > out a
> > > > > > > clock
> > > > > > > tree
> > > > > > > diagram for the hardware?
> > > > > > > 
> > > > > > 
> > > > > > The clock structure is PLL -> MUX -> GATE.
> > > > > > xtal, pll and divider are the possible clock inputs for
> > > > > > MUX.
> > > > > > Because we select the clock input of audio module based on
> > > > > > the
> > > > > > use
> > > > > > case, we use clk_get to retrive all clocks which are
> > > > > > possible
> > > > > > to be
> > > > > > used.
> > > > > 
> > > > > So I see a couple the driver is doing reparenting:
> > > > > 
> > > > >   a. Reparent audio_h to standard oscillator when ADDA is not
> > > > > used,
> > > > >      presumably to let the APLL be turned off
> > > > > 
> > > > > Why not just turn off audio_h? It looks like audio_h feeds a
> > > > > couple
> > > > > clock
> > > > > gates in the audio subsystem. Just a guess, but is this the
> > > > > AHB
> > > > > bus
> > > > > clock?
> > > > > Why not just have it parented to "univpll_d7" all the time
> > > > > then?
> > > > > 
> > > > 
> > > > Sorry, I am not sure if it is the AHB bus clock.
> > > > I only know how audio module uses the clock.
> > > > audio_h feeds to some clock gate like aud_adc_hires, which is
> > > > used
> > > > when
> > > > sampling rate is higher than 48kHz, and hardware designer
> > > > suggests
> > > > us
> > > > use apll1_ck when AFE requrires the clock.
> > > 
> > > I see. So the simplified explanation is high clock rate for high
> > > res
> > > audio.
> > > Would high clock rate work for standard sample rates?
> > 
> > As far as I know, HW will switch clock to hires clock automatically
> > when the required rate is high,(ex: aud_adc and aud_adc_hires) so
> > it
> > can't be controlled by driver.
> 
> I see. That might not be so friendly to the Linux clk driver.
> 
> > > Would using apll1 or univpll all the time work, instead of
> > > reparenting?
> > > What's the gain if we do reparenting?
> > > 
> > 
> > As you said before, the gain is apll can be turned off when the
> > clock
> > is not requrired by ADDA. That's why we didn't use apll all the
> > time.
> 
> Right, and what's the gain from turning it off? Lower power
> consumption?
> 

Yes. Xtal_26m is supplied to most modules, but APLL1 is mainly used by
afe. When audio feature is not used, we hope APLL1 can be turned off to
lower power consumption.

> > > > As I know, DSP also requires audio_h.
> > > > When we disable the clock in AFE driver, the ref count in CCF
> > > > is
> > > > not
> > > > becoming zero if DSP still uses it.
> > > > But only AFE requires higher clock rate, so we reparent audio_h
> > > > to
> > > > 26M
> > > > when it's not required in adda module.
> > > 
> > > I see. Wouldn't reparenting the clock while it is in use by
> > > another
> > > module
> > > cause glitches?
> > 
> > I checked with the DSP owner.
> > audio_h clock is required for DSP bus, but the clock rate is not
> > important.
> > The only thing it cares is audio_h should be powered on, so
> > reparenting
> > is harmless for DSP.
> 
> OK.
> 
> > > > > Also, reparenting really should be done implicitly with
> > > > > clk_set_rate()
> > > > > with the clock driver supporting reparenting on rate changes.
> > > > > 
> > > > >   b. Assignment of PLLs for I2S/PCM MCLK outputs
> > > > > 
> > > > > Is there a reason for explicit assignment, other than clock
> > > > > rate
> > > > > conflicts?
> > > > > CCF supports requesting and locking the clock rate. And
> > > > > again,
> > > > > implicit
> > > > > reparenting should be the norm. The clock driver's purpose is
> > > > > to
> > > > > fulfill
> > > > > any and all clock rate requirements from its consumers. The
> > > > > consumer
> > > > > should
> > > > > only need to ask for the clock rate, not a specific parent,
> > > > > unless
> > > > > there
> > > > > are details that are not yet covered by the CCF.
> > > > > 
> > > > 
> > > > For MCLK output, we should configure divider to get the target
> > > > rate,
> > > > and it can only divide the clock from current parent source.
> > > > So we should do reparent to divider's parent in case the parent
> > > > rate is
> > > > not a multiple of target rate.
> > > 
> > > Right. That is expected. What I'm saying is that the CCF provides
> > > the
> > > framework for automatically reparenting based on the requested
> > > clock
> > > rate. This is done in the clock driver's .determine_rate op.
> > > 
> > > When properly implemented, and also restricting or locking the
> > > clock
> > > rates
> > > of the PLLs, then you can simply request a clock rate on the leaf
> > > clock,
> > > in this case one of the MCLKs, and the CCF and clock driver would
> > > handle
> > > everything else. The consumer should not be reparenting clocks
> > > manually
> > > unless for a very good reason which cannot be satisfied by the
> > > CCF.
> > > 
> > 
> > In some use cases, we really need to reparent clock manually.
> > For example, spdif in(slave) -> .... -> i2s out(master)
> > 
> > APLL3/APLL4 are reserved for slave input like earc in or spdif in,
> > which can refer to the external clock source.(APLL3 syncs with
> > earc,
> > and APLL4 syncs with spdif in.)
> > 
> > When i2s out selects the clock source to APLL4, this makes sure
> > that
> > spdif in and i2s out works in the same clock source.
> > If we just use APLL1/APLL2 on i2s out, there is little rate
> > mismatch
> > between data input and output. Finally, it results in XRUN.
> 
> I see, that makes more sense.
> 
> > If we only use set_rate, it's possible that it can't switch to the
> > expected PLL source, because the rate of APLL3/APLL4 should be
> > close to
> > APLL1/APLL2.
> 
> Well, in theory the CCF should choose the one with the closest rate.
> And if APLL3/APLL4 is already tracking the external clock source, its
> clock rate should match.
> 
> If it's a static requirement, maybe we could replace the *-mclk-
> source
> DT properties with standard assigned-clocks and assigned-clock-
> parents?
> This would get handled by CCF directly, and then the only thing the
> clk driver has to do is make sure it doesn't get reparented again.
> 
> Or is there a need to do reparenting at runtime?
> 

For the use case of APLL3/APLL4, static assignment should be ok.

But I checked with CCF owner, we can't just use clk_set_rate(divider,
rate) to get expected MCLK output, because reparenting MUX
automatically is not supported now. (PLL -> MUX -> divider)

We still have to call clk_set_parent() before clk_set_rate(). Which
means some information should be got from DTS to check whether the PLL
source can be switched or not, so *-mclk-source should be keeped to
identify the case.

Thanks,
Trevor

> > > > > A related question: the chip has five APLLs. How many MCLK
> > > > > combinations
> > > > > does the application need to support? I assume this includes
> > > > > the
> > > > > standard
> > > > > 24.576 MHz and 22.5792 MHz clock rates.
> > > > > 
> > > > 
> > > > APLL1 and APLL2 are used in most AFE modules, so their rate
> > > > should
> > > > be
> > > > fixed.
> > > > APLL1 is fixed to 196608000Hz.
> > > > APLL2 is fixed to 180633600Hz.
> > > > APLL is inputed to the divider(8bit), and MCLK is the output of
> > > > divider.
> > > > Other APLLs are reserved for some special usage which can't be
> > > > supported by APLL1 & APLL2.
> > > > But APLL3~APLL5 aren't used in the series, so I will remove
> > > > them in
> > > > v3.
> > > > 
> > > > > > Some of them are not used in this series, because some
> > > > > > modules
> > > > > > are
> > > > > > still developing. Should I only keep the clocks that have
> > > > > > been
> > > > > > used
> > > > > > in
> > > > > > the series?
> > > > > 
> > > > > Yes please. Only add the ones that are used. Things that
> > > > > aren't
> > > > > used
> > > > > don't get tested and verified, and end up as dead code. If
> > > > > there
> > > > > are
> > > > > plans to extend them in the future, and you can leave
> > > > > comments
> > > > > stating
> > > > > that intent, and also mention it in the cover letter.
> > > > > 
> > > > 
> > > > OK, I will remove the unused clock in v3.
> > > > 
> > > > > > > > +       /* xtal */
> > > > > > > > +       [MT8195_CLK_XTAL_26M] = "clk26m",
> > > > > > > > +       /* pll */
> > > > > > > > +       [MT8195_CLK_APMIXED_APLL1] = "apll1",
> > > > > > > > +       [MT8195_CLK_APMIXED_APLL2] = "apll2",
> > > > > > > > +       [MT8195_CLK_APMIXED_APLL3] = "apll3",
> > > > > > > > +       [MT8195_CLK_APMIXED_APLL4] = "apll4",
> > > > > > > > +       [MT8195_CLK_APMIXED_APLL5] = "apll5",
> > > > > > > > +       [MT8195_CLK_APMIXED_HDMIRX_APLL] =
> > > > > > > > "hdmirx_apll",
> > > > > > > > +       /* divider */
> > > > > > > > +       [MT8195_CLK_TOP_APLL1] = "apll1_ck",
> > > > > > > > +       [MT8195_CLK_TOP_APLL1_D4] = "apll1_d4",
> > > > > > > > +       [MT8195_CLK_TOP_APLL2] = "apll2_ck",
> > > > > > > > +       [MT8195_CLK_TOP_APLL2_D4] = "apll2_d4",
> > > > > > > > +       [MT8195_CLK_TOP_APLL3] = "apll3_ck",
> > > > > > > > +       [MT8195_CLK_TOP_APLL3_D4] = "apll3_d4",
> > > > > > > > +       [MT8195_CLK_TOP_APLL4] = "apll4_ck",
> > > > > > > > +       [MT8195_CLK_TOP_APLL4_D4] = "apll4_d4",
> > > > > > > > +       [MT8195_CLK_TOP_APLL5] = "apll5_ck",
> > > > > > > > +       [MT8195_CLK_TOP_APLL5_D4] = "apll5_d4",
> > > > > > > > +       [MT8195_CLK_TOP_APLL12_DIV0] = "apll12_div0",
> > > > > > > > +       [MT8195_CLK_TOP_APLL12_DIV1] = "apll12_div1",
> > > > > > > > +       [MT8195_CLK_TOP_APLL12_DIV2] = "apll12_div2",
> > > > > > > > +       [MT8195_CLK_TOP_APLL12_DIV3] = "apll12_div3",
> > > > > > > > +       [MT8195_CLK_TOP_APLL12_DIV4] = "apll12_div4",
> > > > > > > > +       [MT8195_CLK_TOP_APLL12_DIV9] = "apll12_div9",
> > > > > > > > +       [MT8195_CLK_TOP_HDMIRX_APLL] =
> > > > > > > > "hdmirx_apll_ck",
> > > > > > > > +       [MT8195_CLK_TOP_MAINPLL_D4_D4] =
> > > > > > > > "mainpll_d4_d4",
> > > > > > > > +       [MT8195_CLK_TOP_MAINPLL_D5_D2] =
> > > > > > > > "mainpll_d5_d2",
> > > > > > > > +       [MT8195_CLK_TOP_MAINPLL_D7_D2] =
> > > > > > > > "mainpll_d7_d2",
> > > > > > > > +       [MT8195_CLK_TOP_UNIVPLL_D4] = "univpll_d4",
> > > > > > > > +       /* mux */
> > > > > > > > +       [MT8195_CLK_TOP_APLL1_SEL] = "apll1_sel",
> > > > > > > > +       [MT8195_CLK_TOP_APLL2_SEL] = "apll2_sel",
> > > > > > > > +       [MT8195_CLK_TOP_APLL3_SEL] = "apll3_sel",
> > > > > > > > +       [MT8195_CLK_TOP_APLL4_SEL] = "apll4_sel",
> > > > > > > > +       [MT8195_CLK_TOP_APLL5_SEL] = "apll5_sel",
> > > > > > > > +       [MT8195_CLK_TOP_A1SYS_HP_SEL] = "a1sys_hp_sel",
> > > > > > > > +       [MT8195_CLK_TOP_A2SYS_SEL] = "a2sys_sel",
> > > > > > > > +       [MT8195_CLK_TOP_A3SYS_SEL] = "a3sys_sel",
> > > > > > > > +       [MT8195_CLK_TOP_A4SYS_SEL] = "a4sys_sel",
> > > > > > > > +       [MT8195_CLK_TOP_ASM_H_SEL] = "asm_h_sel",
> > > > > > > > +       [MT8195_CLK_TOP_ASM_M_SEL] = "asm_m_sel",
> > > > > > > > +       [MT8195_CLK_TOP_ASM_L_SEL] = "asm_l_sel",
> > > > > > > > +       [MT8195_CLK_TOP_AUD_IEC_SEL] = "aud_iec_sel",
> > > > > > > > +       [MT8195_CLK_TOP_AUD_INTBUS_SEL] =
> > > > > > > > "aud_intbus_sel",
> > > > > > > > +       [MT8195_CLK_TOP_AUDIO_H_SEL] = "audio_h_sel",
> > > > > > > > +       [MT8195_CLK_TOP_AUDIO_LOCAL_BUS_SEL] =
> > > > > > > > "audio_local_bus_sel",
> > > > > > > > +       [MT8195_CLK_TOP_DPTX_M_SEL] = "dptx_m_sel",
> > > > > > > > +       [MT8195_CLK_TOP_INTDIR_SEL] = "intdir_sel",
> > > > > > > > +       [MT8195_CLK_TOP_I2SO1_M_SEL] = "i2so1_m_sel",
> > > > > > > > +       [MT8195_CLK_TOP_I2SO2_M_SEL] = "i2so2_m_sel",
> > > > > > > > +       [MT8195_CLK_TOP_I2SI1_M_SEL] = "i2si1_m_sel",
> > > > > > > > +       [MT8195_CLK_TOP_I2SI2_M_SEL] = "i2si2_m_sel",
> > > > > > > > +       /* clock gate */
> > > > > > > > +       [MT8195_CLK_TOP_MPHONE_SLAVE_B] =
> > > > > > > > "mphone_slave_b",
> > > > > > > > +       [MT8195_CLK_TOP_CFG_26M_AUD] = "cfg_26m_aud",
> > > > > > > > +       [MT8195_CLK_INFRA_AO_AUDIO] = "infra_ao_audio",
> > > > > > > > +       [MT8195_CLK_INFRA_AO_AUDIO_26M_B] =
> > > > > > > > "infra_ao_audio_26m_b",
> > > > > > > > +       [MT8195_CLK_SCP_ADSP_AUDIODSP] =
> > > > > > > > "scp_adsp_audiodsp",
> > > > > > > 
> > > > > > > 
> > > > > > > > +       [MT8195_CLK_AUD_AFE] = "aud_afe",
> > > > > > > > +       [MT8195_CLK_AUD_LRCK_CNT] = "aud_lrck_cnt",
> > > > > > > > +       [MT8195_CLK_AUD_SPDIFIN_TUNER_APLL] =
> > > > > > > > "aud_spdifin_tuner_apll",
> > > > > > > > +       [MT8195_CLK_AUD_SPDIFIN_TUNER_DBG] =
> > > > > > > > "aud_spdifin_tuner_dbg",
> > > > > > > > +       [MT8195_CLK_AUD_UL_TML] = "aud_ul_tml",
> > > > > > > > +       [MT8195_CLK_AUD_APLL1_TUNER] =
> > > > > > > > "aud_apll1_tuner",
> > > > > > > > +       [MT8195_CLK_AUD_APLL2_TUNER] =
> > > > > > > > "aud_apll2_tuner",
> > > > > > > > +       [MT8195_CLK_AUD_TOP0_SPDF] = "aud_top0_spdf",
> > > > > > > > +       [MT8195_CLK_AUD_APLL] = "aud_apll",
> > > > > > > > +       [MT8195_CLK_AUD_APLL2] = "aud_apll2",
> > > > > > > > +       [MT8195_CLK_AUD_DAC] = "aud_dac",
> > > > > > > > +       [MT8195_CLK_AUD_DAC_PREDIS] = "aud_dac_predis",
> > > > > > > > +       [MT8195_CLK_AUD_TML] = "aud_tml",
> > > > > > > > +       [MT8195_CLK_AUD_ADC] = "aud_adc",
> > > > > > > > +       [MT8195_CLK_AUD_DAC_HIRES] = "aud_dac_hires",
> > > > > > > > +       [MT8195_CLK_AUD_A1SYS_HP] = "aud_a1sys_hp",
> > > > > > > > +       [MT8195_CLK_AUD_AFE_DMIC1] = "aud_afe_dmic1",
> > > > > > > > +       [MT8195_CLK_AUD_AFE_DMIC2] = "aud_afe_dmic2",
> > > > > > > > +       [MT8195_CLK_AUD_AFE_DMIC3] = "aud_afe_dmic3",
> > > > > > > > +       [MT8195_CLK_AUD_AFE_DMIC4] = "aud_afe_dmic4",
> > > > > > > > +       [MT8195_CLK_AUD_AFE_26M_DMIC_TM] =
> > > > > > > > "aud_afe_26m_dmic_tm",
> > > > > > > > +       [MT8195_CLK_AUD_UL_TML_HIRES] =
> > > > > > > > "aud_ul_tml_hires",
> > > > > > > > +       [MT8195_CLK_AUD_ADC_HIRES] = "aud_adc_hires",
> > > > > > > > +       [MT8195_CLK_AUD_ADDA6_ADC] = "aud_adda6_adc",
> > > > > > > > +       [MT8195_CLK_AUD_ADDA6_ADC_HIRES] =
> > > > > > > > "aud_adda6_adc_hires",
> > > > > > > > +       [MT8195_CLK_AUD_LINEIN_TUNER] =
> > > > > > > > "aud_linein_tuner",
> > > > > > > > +       [MT8195_CLK_AUD_EARC_TUNER] = "aud_earc_tuner",
> > > > > > > > +       [MT8195_CLK_AUD_I2SIN] = "aud_i2sin",
> > > > > > > > +       [MT8195_CLK_AUD_TDM_IN] = "aud_tdm_in",
> > > > > > > > +       [MT8195_CLK_AUD_I2S_OUT] = "aud_i2s_out",
> > > > > > > > +       [MT8195_CLK_AUD_TDM_OUT] = "aud_tdm_out",
> > > > > > > > +       [MT8195_CLK_AUD_HDMI_OUT] = "aud_hdmi_out",
> > > > > > > > +       [MT8195_CLK_AUD_ASRC11] = "aud_asrc11",
> > > > > > > > +       [MT8195_CLK_AUD_ASRC12] = "aud_asrc12",
> > > > > > > > +       [MT8195_CLK_AUD_MULTI_IN] = "aud_multi_in",
> > > > > > > > +       [MT8195_CLK_AUD_INTDIR] = "aud_intdir",
> > > > > > > > +       [MT8195_CLK_AUD_A1SYS] = "aud_a1sys",
> > > > > > > > +       [MT8195_CLK_AUD_A2SYS] = "aud_a2sys",
> > > > > > > > +       [MT8195_CLK_AUD_PCMIF] = "aud_pcmif",
> > > > > > > > +       [MT8195_CLK_AUD_A3SYS] = "aud_a3sys",
> > > > > > > > +       [MT8195_CLK_AUD_A4SYS] = "aud_a4sys",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_UL1] = "aud_memif_ul1",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_UL2] = "aud_memif_ul2",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_UL3] = "aud_memif_ul3",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_UL4] = "aud_memif_ul4",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_UL5] = "aud_memif_ul5",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_UL6] = "aud_memif_ul6",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_UL8] = "aud_memif_ul8",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_UL9] = "aud_memif_ul9",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_UL10] = "aud_memif_ul10",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_DL2] = "aud_memif_dl2",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_DL3] = "aud_memif_dl3",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_DL6] = "aud_memif_dl6",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_DL7] = "aud_memif_dl7",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_DL8] = "aud_memif_dl8",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_DL10] = "aud_memif_dl10",
> > > > > > > > +       [MT8195_CLK_AUD_MEMIF_DL11] = "aud_memif_dl11",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC0] = "aud_gasrc0",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC1] = "aud_gasrc1",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC2] = "aud_gasrc2",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC3] = "aud_gasrc3",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC4] = "aud_gasrc4",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC5] = "aud_gasrc5",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC6] = "aud_gasrc6",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC7] = "aud_gasrc7",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC8] = "aud_gasrc8",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC9] = "aud_gasrc9",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC10] = "aud_gasrc10",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC11] = "aud_gasrc11",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC12] = "aud_gasrc12",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC13] = "aud_gasrc13",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC14] = "aud_gasrc14",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC15] = "aud_gasrc15",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC16] = "aud_gasrc16",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC17] = "aud_gasrc17",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC18] = "aud_gasrc18",
> > > > > > > > +       [MT8195_CLK_AUD_GASRC19] = "aud_gasrc19",
> > > > > > > 
> > > > > > > The MT8195_CLK_AUD_* clocks are all internal to the audio
> > > > > > > subsystem:
> > > > > > > the bits that control these clock gates are in the same
> > > > > > > address
> > > > > > > space
> > > > > > > as the audio parts. Would it be possible to model them as
> > > > > > > internal
> > > > > > > ASoC SUPPLY widgets? The external ones could be modeled
> > > > > > > using
> > > > > > > ASoC
> > > > > > > CLK_SUPPLY widgets, and the dependencies could be modeled
> > > > > > > with
> > > > > > > ASoC
> > > > > > > routes. The ASoC core could then handle power sequencing,
> > > > > > > which
> > > > > > > the
> > > > > > > driver currently does manually.
> > > > > > > 
> > > > > > > IMO this is better than having two drivers handling two
> > > > > > > aspects
> > > > > > > of
> > > > > > > the same piece of hardware, while the two aspects are
> > > > > > > intertwined.
> > > > > > > 
> > > > > > 
> > > > > > Yes, it's ok to use the CLK_SUPPLY and SUPPLY to model such
> > > > > > clocks.
> > > > > > But those clocks are managed by CCF in the preceding SOCs
> > > > > > like
> > > > > > mt2701,
> > > > > > mt6779 and mt8183. Additionally, in some audio modules,
> > > > > > clocks
> > > > > > should
> > > > > 
> > > > > This being a new driver, we have some more freedom to improve
> > > > > the
> > > > > design.
> > > > > 
> > > > > > be enabled before configuring parameters(hw_params). As far
> > > > > > as
> > > > > > I
> > > > > > know,
> > > > > > if we use CLK_SUPPLY or SUPPLY to model clocks, the power
> > > > > > sequence
> > > > > > is
> > > > > > controlled by DAPM. It seems to be impossible to fulfill
> > > > > > all
> > > > > > use
> > > > > > cases.
> > > > > > That's why we just keep the manual control sequence and CCF
> > > > > > seems
> > > > > > to be
> > > > > > the best choice to model such clock gatess.
> > > > > 
> > > > > I see. So yes, using CCF does give you reference counting,
> > > > > dependency
> > > > > tracking and other advantages. And using DAPM supplies means
> > > > > you
> > > > > can't
> > > > > enable the clock gates outside of DAPM without both pieces of
> > > > > code
> > > > > fighting for control.
> > > > > 
> > > > > Can we at least move the audio clock gates into the audio
> > > > > driver
> > > > > though?
> > > > > The arbitrary separation into two devices and drivers is
> > > > > fishy.
> > > > > And
> > > > > with
> > > > > the move the external references to the audio clock gates can
> > > > > be
> > > > > removed.
> > > > > 
> > > > 
> > > > Because DAPM SUPPLY can't fit our control scenario.
> > > > Did you suggest us implement the simple logic control(including
> > > > ref
> > > > count, clock dependency) for clock gate(MT8195_CLK_AUD_*) in
> > > > afe
> > > > driver
> > > > instead of using CCF?
> > > 
> > > I meant simply moving the CCF-based clk driver code (clk-mt8516-
> > > aud.c)
> > > from `drivers/clk` and incorporating it into the audio driver,
> > > likely
> > > in `mt8195-afe-clk.c` or maybe as a separate file. So the audio
> > > driver
> > > would be a clock provider, and a clock consumer. It will directly
> > > use
> > > the clocks it provides, internally, and you could remove all
> > > those
> > > clock references from the device tree.
> > > 
> > > The goal is to have one hardware representation (device node)
> > > only,
> > > so
> > > that it matches the hardware, which is one single unified block.
> > > 
> > > After the driver is completed, we can look for opportunities to
> > > improve
> > > it, if resources are available.
> > 
> > Thanks for your detailed information.
> > I will try to move the CCF-based clk driver code to AFE driver.
> > If there are no other internal concerns and blocking problems, I
> > will
> > include the changes in v3.
> 
> Great.
> 
> > > > > And regarding the clock requirements for different modules,
> > > > > could
> > > > > we
> > > > > have
> > > > > that information put in comments somewhere, so if someone
> > > > > were to
> > > > > revisit
> > > > > it later, they would have the information needed to
> > > > > understand
> > > > > and
> > > > > possibly
> > > > > improve it? Because right now there's just a bunch of clocks
> > > > > enabled
> > > > > and
> > > > > disabled and nothing to explain why that's needed.
> > > > > 
> > > > 
> > > > For example,
> > > > MT8195_CLK_AUD_ADC(clock gate) is one of the clock feeding to
> > > > ADDA
> > > > module.
> > > > Did you want me show the clock gate list feeding to ADDA?
> > > > On the other hand, I didn't know how to show the information
> > > > properly
> > > > in comments. Could you kindly share me an example for
> > > > reference?
> > > 
> > > 
> > > For example, in `mt8195_afe_enable_reg_rw_clk()` in mt8195-afe-
> > > clk.c:
> > > 
> > >         unsigned int clk_array[] = {
> > >                 MT8195_CLK_SCP_ADSP_AUDIODSP,
> > >                 MT8195_CLK_TOP_AUDIO_LOCAL_BUS_SEL,
> > >                 MT8195_CLK_TOP_CFG_26M_AUD,
> > >                 MT8195_CLK_INFRA_AO_AUDIO,
> > >                 MT8195_CLK_INFRA_AO_AUDIO_26M_B,
> > >                 MT8195_CLK_TOP_AUD_INTBUS_SEL,
> > >                 MT8195_CLK_TOP_A1SYS_HP_SEL,
> > >                 MT8195_CLK_AUD_A1SYS_HP,
> > >                 MT8195_CLK_AUD_A1SYS,
> > >                 MT8195_CLK_TOP_AUDIO_H_SEL,
> > >         };
> > > 
> > > You could add a comment after each line stating why that clock
> > > needs
> > > to
> > > be enabled. A simple note like "bus access clock" or "internal
> > > logic
> > > clock"
> > > would suffice.
> > > 
> > 
> > OK, I will add short notes to such clock lists.
> > 
> > > The above list also has some redundancies that could be
> > > eliminated.
> > > MT8195_CLK_TOP_A1SYS_HP_SEL is parent to both
> > > MT8195_CLK_AUD_A1SYS_HP
> > > and
> > > MT8195_CLK_AUD_A1SYS. When clocks are enabled, their parents are
> > > also
> > > enabled by CCF, so there's no need to enable them explicitly,
> > > unless
> > > that clock also directly feeds the clock consumer.
> > > 
> > 
> > OK, I will review all clock usages and remove the unnecessary
> > clocks.
> > 
> > > 
> > > Another thing I wanted to bring up: is any of the code after
> > > 
> > >     struct mt8195_afe_tuner_cfg {
> > > 
> > > used? It looks like it is used to configure the five extra PLLs
> > > in
> > > the audio
> > > subsystem, but the exposed (non-static) functions don't seem to
> > > be
> > > called
> > > anywhere. Are they for modules not yet supported?
> > > 
> > 
> > Yes, tuners are not supported now.
> > I will remove the code and add them back when tuners are required
> > in
> > the future.
> 
> Thanks.
> 
> 
> ChenYu

  reply	other threads:[~2021-07-26 14:31 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-29  1:47 [PATCH v2 0/8] ASoC: mediatek: Add support for MT8195 SoC Trevor Wu
2021-06-29  1:47 ` [PATCH v2 1/8] ASoC: mediatek: mt8195: update mediatek common driver Trevor Wu
2021-06-29  1:47 ` [PATCH v2 2/8] ASoC: mediatek: mt8195: support etdm in platform driver Trevor Wu
2021-06-29  1:47 ` [PATCH v2 3/8] ASoC: mediatek: mt8195: support adda " Trevor Wu
2021-06-29  1:47 ` [PATCH v2 4/8] ASoC: mediatek: mt8195: support pcm " Trevor Wu
2021-06-29  1:47 ` [PATCH v2 5/8] ASoC: mediatek: mt8195: add " Trevor Wu
2021-07-12  6:57   ` Chen-Yu Tsai
2021-07-12 15:09     ` Trevor Wu
2021-07-13  6:00       ` Chen-Yu Tsai
2021-07-15 11:05         ` Trevor Wu
2021-07-19 10:05           ` Chen-Yu Tsai
2021-07-22  8:56             ` Trevor Wu
2021-07-23  6:27               ` Chen-Yu Tsai
2021-07-26 14:31                 ` Trevor Wu [this message]
2021-08-02 10:21                   ` Chen-Yu Tsai
2021-08-03 10:12                     ` Trevor Wu
2021-06-29  1:47 ` [PATCH v2 6/8] dt-bindings: mediatek: mt8195: add audio afe document Trevor Wu
2021-07-01 20:18   ` Rob Herring
2021-07-05  7:01     ` Trevor Wu
2021-06-29  1:47 ` [PATCH v2 7/8] ASoC: mediatek: mt8195: add machine driver with mt6359, rt1019 and rt5682 Trevor Wu
2021-06-29  1:47 ` [PATCH v2 8/8] dt-bindings: mediatek: mt8195: add mt8195-mt6359-rt1019-rt5682 document Trevor Wu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fc12d71febfebda12e57b287de0ad2668db20ffa.camel@mediatek.com \
    --to=trevor.wu@mediatek.com \
    --cc=aaronyu@google.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=bicycle.tsai@mediatek.com \
    --cc=broonie@kernel.org \
    --cc=chun-jie.chen@mediatek.com \
    --cc=cychiang@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jiaxin.yu@mediatek.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=tiwai@suse.com \
    --cc=wenst@chromium.org \
    --subject='Re: [PATCH v2 5/8] ASoC: mediatek: mt8195: add platform driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).