Message-ID: <CAGXv+5HOL5j4gjgJa0RLjvf3+Vt2sGZcWU1KQXwjn7DOYy-JWg@mail.gmail.com>
Date:   Fri, 23 Jul 2021 14:27:15 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     Trevor Wu <trevor.wu@...iatek.com>
Cc:     Chun-Jie Chen <chun-jie.chen@...iatek.com>, broonie@...nel.org,
        tiwai@...e.com, Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        alsa-devel@...a-project.org, linux-mediatek@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org,
        LKML <linux-kernel@...r.kernel.org>, devicetree@...r.kernel.org,
        bicycle.tsai@...iatek.com, Jiaxin Yu <jiaxin.yu@...iatek.com>,
        Jimmy Cheng-Yi Chiang <cychiang@...gle.com>,
        Li-Yu Yu <aaronyu@...gle.com>
Subject: Re: [PATCH v2 5/8] ASoC: mediatek: mt8195: add platform driver

On Thu, Jul 22, 2021 at 4:56 PM Trevor Wu <trevor.wu@...iatek.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@...iatek.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@...iatek.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@...iatek.com
> > > > > > >
> > > > > >
> > > > > > wrote:
> > > > > > >
> > > > > > > This patch adds mt8195 platform and affiliated driver.
> > > > > > >
> > > > > > > Signed-off-by: Trevor Wu <trevor.wu@...iatek.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@...iatek.com>
> > > > > > > + *         Trevor Wu <trevor.wu@...iatek.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?

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

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

Powered by blists - more mailing lists