Re: [PATCH v6 08/10] ASoC: dt-bindings: mediatek,mt8196-afe: add audio AFE

["Darren Ye (叶飞)" <Darren.Ye@mediatek.com>]

On Tue, 2025-07-15 at 13:09 +0800, Chen-Yu Tsai wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hi,
> 
> On Tue, Jul 8, 2025 at 7:35 PM Darren.Ye <darren.ye@mediatek.com>
> wrote:
> > 
> > From: Darren Ye <darren.ye@mediatek.com>
> > 
> > Add mt8196 audio AFE.
> > 
> > Signed-off-by: Darren Ye <darren.ye@mediatek.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > ---
> >  .../bindings/sound/mediatek,mt8196-afe.yaml   | 157
> > ++++++++++++++++++
> >  1 file changed, 157 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/sound/mediatek,mt8196-afe.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/sound/mediatek,mt8196-afe.yaml
> > b/Documentation/devicetree/bindings/sound/mediatek,mt8196-afe.yaml
> > new file mode 100644
> > index 000000000000..fe147eddf5e7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8196-
> > afe.yaml
> > @@ -0,0 +1,157 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/sound/mediatek,mt8196-afe.yaml*__;Iw!!CTRNKA9wMg0ARbw!iSiBwCYEjWdWSv25XRbl3ky3Niiw3nDpVY-fW1dxyp3eU5YDs0bbZXEgPUQ1_NInbxUIgyz3HJvf-xTH$
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!iSiBwCYEjWdWSv25XRbl3ky3Niiw3nDpVY-fW1dxyp3eU5YDs0bbZXEgPUQ1_NInbxUIgyz3HHUjHsuW$
> > +
> > +title: MediaTek Audio Front End PCM controller for MT8196
> > +
> > +maintainers:
> > +  - Darren Ye <darren.ye@mediatek.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,mt8196-afe
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  memory-region:
> > +    maxItems: 1
> > +
> > +  mediatek,vlpcksys:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: To set up the apll12 tuner
> 
> Looking at the implementation, the configuration is just a fixed
> value.
> Can this be moved to the VLP clock driver instead?
> 
I thinks it's not good to put it in the VLP clock kernel driver,
because this value needs to be adjusted. Usually, it is set in 
coreboot, but it is hard to change later. Audio needs to adjust
this value, so it's better to put it in the audio driver. What do
you think?

> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: mux for audio intbus
> > +      - description: mux for audio engen1
> > +      - description: mux for audio engen2
> > +      - description: mux for audio h
> > +      - description: vlp 26m clock
> > +      - description: audio apll1 clock
> > +      - description: audio apll2 clock
> > +      - description: audio apll1 divide4
> > +      - description: audio apll2 divide4
> > +      - description: audio apll12 divide for i2sin0
> > +      - description: audio apll12 divide for i2sin1
> > +      - description: audio apll12 divide for fmi2s
> > +      - description: audio apll12 divide for tdmout mck
> > +      - description: audio apll12 divide for tdmout bck
> > +      - description: mux for audio apll1
> > +      - description: mux for audio apll2
> > +      - description: mux for i2sin0 mck
> > +      - description: mux for i2sin1 mck
> > +      - description: mux for fmi2s mck
> > +      - description: mux for tdmout mck
> > +      - description: mux for adsp clock
> > +      - description: 26m clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: top_aud_intbus
> > +      - const: top_aud_eng1
> > +      - const: top_aud_eng2
> > +      - const: top_aud_h
> > +      - const: vlp_clk26m
> > +      - const: apll1
> > +      - const: apll2
> > +      - const: apll1_d4
> > +      - const: apll2_d4
> 
> These are parents of the top_apll[12]. They do not feed into the
> hardware directly, so you should not be including them here.
> 
> > +      - const: apll12_div_i2sin0
> > +      - const: apll12_div_i2sin1
> > +      - const: apll12_div_fmi2s
> > +      - const: apll12_div_tdmout_m
> > +      - const: apll12_div_tdmout_b
> 
> In the clock bindings sent by Collabora, these dividers are no longer
> separately modeled; they have been combined with their respective
> top_* clocks.
> 
> > +      - const: top_apll1
> > +      - const: top_apll2
> 
> These two are parents to apll12_div_*, do not feed into the hardware
> directly, so you should not be including them here.
> 
> The clock tree for each audio interface clock looks like the
> following:
> 
>     apll1 -> apll1_d4 -> top_apll1 --
>                      /               \
>               clk26m                  --> top_fmi2s ->
> apll12_div_fmi2s
>                      \               /
>     apll2 -> apll2_d4 -> top_apll2 --
> 
> Only the final "apll12_div_fmi2s" should be referenced.
> 
> On the implementation side, it should simply be a matter of setting
> the
> required rate (24.576 MHz or 22.5792 MHz, or some multiple) on this
> leaf
> clock, and let the clock framework figure out the PLL and dividers to
> use. Same thing for enabling the clock.

I think we have some misunderstandings. I will first draw mtk audio
clock topology diagram, and then we can disscuss which parts can be
optimized together.

top_aud_intbus: as read/write reg clock source;
top_aud_eng1/top_aud_eng2: as i2s bck clock source;
apll12_div_xxx: as mclk clock source;
top_audio_h: as afe other ip clock source;


vlp_clk26m
        \
apll1   --> top_audio_h
	/
apll2


                vlp_clk26m
                     \
apll1  --> apll1_d4  --> top_aud_eng1 
       \
clk26m --> top_apll1 
		\
		 --> top_fmi2s --> apll12_div_fmi2s  
		/
clk26m  --> top_apll2
       /
apll2  --> apll2_d4  --> top_aud_eng2
                     /
	       vlp_clk26m


vlp_clk26m -> top_aud_intbus


> 
> > +      - const: top_i2sin0
> > +      - const: top_i2sin1
> > +      - const: top_fmi2s
> > +      - const: top_tdmout
> > +      - const: top_adsp
> > +      - const: clk26m
> 
> Is this one directly needed? It is similar to vlp_clk26m, and I
> suspect
> only that one is needed.
> 
vlp_clk26m belongs to the VLP clock domain, and clk26 belongs to the
system clock domain;

top_apll1/top_apll2 can only select system apll1/apll2 or clk26m, they
cannot use vlp_clk26m;

BR
Darren Ye

> 
> ChenYu
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - memory-region
> > +  - mediatek,vlpcksys
> > +  - power-domains
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        afe@1a110000 {
> > +            compatible = "mediatek,mt8196-afe";
> > +            reg = <0 0x1a110000 0 0x9000>;
> > +            interrupts = <GIC_SPI 351 IRQ_TYPE_LEVEL_HIGH 0>;
> > +            memory-region = <&afe_dma_mem_reserved>;
> > +            mediatek,vlpcksys = <&vlp_cksys_clk>;
> > +            power-domains = <&scpsys 14>;
> > //MT8196_POWER_DOMAIN_AUDIO
> > +            clocks = <&vlp_cksys_clk 40>,
> > //CLK_VLP_CK_AUD_INTBUS_SEL
> > +                     <&vlp_cksys_clk 38>,
> > //CLK_VLP_CK_AUD_ENGEN1_SEL
> > +                     <&vlp_cksys_clk 39>,
> > //CLK_VLP_CK_AUD_ENGEN2_SEL
> > +                     <&vlp_cksys_clk 37>, //CLK_VLP_CK_AUDIO_H_SEL
> > +                     <&vlp_cksys_clk 45>, //CLK_VLP_CK_CLKSQ
> > +                     <&cksys_clk 129>, //CLK_CK_APLL1
> > +                     <&cksys_clk 132>, //CLK_CK_APLL2
> > +                     <&cksys_clk 130>, //CLK_CK_APLL1_D4
> > +                     <&cksys_clk 133>, //CLK_CK_APLL2_D4
> > +                     <&cksys_clk 80>,
> > //CLK_CK_APLL12_CK_DIV_I2SIN0
> > +                     <&cksys_clk 81>,
> > //CLK_CK_APLL12_CK_DIV_I2SIN1
> > +                     <&cksys_clk 92>, //CLK_CK_APLL12_CK_DIV_FMI2S
> > +                     <&cksys_clk 93>,
> > //CLK_CK_APLL12_CK_DIV_TDMOUT_M
> > +                     <&cksys_clk 94>,
> > //CLK_CK_APLL12_CK_DIV_TDMOUT_B
> > +                     <&cksys_clk 43>, //CLK_CK_AUD_1_SEL
> > +                     <&cksys_clk 44>, //CLK_CK_AUD_2_SEL
> > +                     <&cksys_clk 66>, //CLK_CK_APLL_I2SIN0_MCK_SEL
> > +                     <&cksys_clk 67>, //CLK_CK_APLL_I2SIN1_MCK_SEL
> > +                     <&cksys_clk 78>, //CLK_CK_APLL_FMI2S_MCK_SEL
> > +                     <&cksys_clk 79>, //CLK_CK_APLL_TDMOUT_MCK_SEL
> > +                     <&cksys_clk 45>, //CLK_CK_ADSP_SEL
> > +                     <&cksys_clk 140>; //CLK_CK_TCK_26M_MX9
> > +            clock-names = "top_aud_intbus",
> > +                          "top_aud_eng1",
> > +                          "top_aud_eng2",
> > +                          "top_aud_h",
> > +                          "vlp_clk26m",
> > +                          "apll1",
> > +                          "apll2",
> > +                          "apll1_d4",
> > +                          "apll2_d4",
> > +                          "apll12_div_i2sin0",
> > +                          "apll12_div_i2sin1",
> > +                          "apll12_div_fmi2s",
> > +                          "apll12_div_tdmout_m",
> > +                          "apll12_div_tdmout_b",
> > +                          "top_apll1",
> > +                          "top_apll2",
> > +                          "top_i2sin0",
> > +                          "top_i2sin1",
> > +                          "top_fmi2s",
> > +                          "top_tdmout",
> > +                          "top_adsp",
> > +                          "clk26m";
> > +        };
> > +    };
> > +
> > +...
> > --
> > 2.45.2
> > 
> >