Re: [PATCH v2 5/8] ASoC: mediatek: mt8195: add platform driver | linux-arm-kernel

(off-list ancestor, not in this archive)

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 [off-list ref]
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 <redacted>
---
 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.

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.

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.


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.

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.


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.

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

Regards
ChenYu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help