Re: [PATCH] ASoC: mediatek: mt8188: Enable apll1 clock during reg rw to prevent hang

[Chen-Yu Tsai <wenst@xxxxxxxxxxxx>]

On Tue, Dec 10, 2024 at 4:08 AM Nícolas F. R. A. Prado
<nfraprado@xxxxxxxxxxxxx> wrote:
>
> On Fri, Dec 06, 2024 at 06:57:00AM +0000, Trevor Wu (吳文良) wrote:
> > On Thu, 2024-12-05 at 13:51 +0100, AngeloGioacchino Del Regno wrote:
> > >
> > >
> > > Il 04/12/24 13:17, Trevor Wu (吳文良) ha scritto:
> > > > On Tue, 2024-12-03 at 17:07 -0300, Nícolas F. R. A. Prado wrote:
> > > >
> > > > >
> > > > > Currently, booting the Genio 700 EVK board with the MT8188 sound
> > > > > platform driver configured as a module (CONFIG_SND_SOC_MT8188=m)
> > > > > results
> > > > > in a system hang right when the HW registers for the audio
> > > > > controller
> > > > > are read:
> > > > >
> > > > >    mt8188-audio 10b10000.audio-controller: No cache defaults,
> > > > > reading
> > > > > back from HW
> > > > >
> > > > > The hang doesn't occur with the driver configured as builtin as
> > > > > then
> > > > > the
> > > > > unused clocks are still enabled.
> > > > >
> > > > > Enable the apll1 clock during register read/write to prevent the
> > > > > hang.
> > > > >
> > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
> > > > > ---
> > > > >   sound/soc/mediatek/mt8188/mt8188-afe-clk.c | 4 ++++
> > > > >   1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/sound/soc/mediatek/mt8188/mt8188-afe-clk.c
> > > > > b/sound/soc/mediatek/mt8188/mt8188-afe-clk.c
> > > > > index
> > > > > e69c1bb2cb239596dee50b166c20192d5408be10..fb8cf286df3f02ac076528b
> > > > > 898f
> > > > > d0d7a708ec1ea 100644
> > > > > --- a/sound/soc/mediatek/mt8188/mt8188-afe-clk.c
> > > > > +++ b/sound/soc/mediatek/mt8188/mt8188-afe-clk.c
> > > > > @@ -587,6 +587,8 @@ int mt8188_afe_enable_reg_rw_clk(struct
> > > > > mtk_base_afe *afe)
> > > > >          mt8188_afe_enable_clk(afe, afe_priv-
> > > > > > clk[MT8188_CLK_AUD_A1SYS_HP]);
> > > > >
> > > > >          mt8188_afe_enable_clk(afe, afe_priv-
> > > > > > clk[MT8188_CLK_AUD_A1SYS]);
> > > > >
> > > > > +       mt8188_afe_enable_clk(afe, afe_priv-
> > > > > > clk[MT8188_CLK_APMIXED_APLL1]);
> > > > >
> > > > > +
> > > > >          return 0;
> > > > >   }
> > > >
> > > > Hi Nicolas,
> > > >
> > > > If I understand correctly, APLL1 should be the parent clock of
> > > > AUD_A1SYS_HP and AUD_A1SYS, so it should be enabled automatically
> > > > by
> > > > CCF.
> > > >
> > > > I'm not sure why you resolved the hang issue after enabling APLL1.
> > > > Could you share more details about the solution?
> > > >
> > >
> > > Hmm. Now I see what's happening here...
> > >
> > > Nicolas, Trevor,
> > >
> > > Possible parents for top_a1sys_hp are:
> > >   - clk26m
> > >   - apll1_d4
> > >
> > > ...what's happening here most probably is that after the clock gets
> > > disabled as
> > > unused, it gets parented to clk26m by default as that is parent index
> > > 0... and
> > > something else in AFE needs APLL1 to feed a clock to .. something ..
> > > to allow
> > > register access.
> > >
> > > Trevor, do you know why is this IP unaccessible when A1SYS is
> > > parented to clk26m?
> >
> > Hi Angelo,
> >
> > As far as I know, it should work even though the clock is parented to
> > clk26m.
> >
> > Unfortunately, I have no idea about why APLL1 enabling can resolve the
> > hang issue. I'm also curious about how Nicolas found the solution to
> > resolve the problem.
> >
> > From the description, it seems that the problem is related to register
> > r/w hang. If I remembered correctly, when the mtcmos of ADSP_INFRA is
> > ON, register r/w won't cause the cpu to hang. However, ADSP_INFRA has
> > been configured as ALWAYS_ON in the driver. I'm not sure if it doesn't
> > work correctly when the driver is configured as a module. Maybe Nicolas
> > can also check this.
>
> Indeed, as suggested by Angelo, adding
>
>   assigned-clocks = <&topckgen CLK_TOP_A1SYS_HP>;
>   assigned-clock-parents = <&topckgen CLK_TOP_APLL1_D4>;
>
> to the afe node also fixes this issue.
>
> In mt8188.dtsi, we currently have
>
>   afe: audio-controller@10b10000 {
>         ...
>         assigned-clocks = <&topckgen CLK_TOP_A1SYS_HP>;
>         assigned-clock-parents =  <&clk26m>;
>
> So the question is, do other MT8188 platforms need clk26m to be the parent of
> a1sys_hp? Depending on that I can either update the parent on the common
> mt8188.dtsi or on the genio700-evk.dts, which is where I observed the issue. I
> don't have access to other mt8188 platforms. Trevor, do you know?
>
> As for how I identified this issue, I noticed that when booting with the
> platform driver as a module the system would hang, and that passing
> clk_ignore_unused avoided the issue. Then I selectively ignored some unused
> clocks until I narrowed down to ignoring unused only the apll1 clock, meaning
> the apll1 clock needed to be left on during the platform driver probe for the
> system to not hang.

I don't know. The AFE driver supposedly enables all clocks that are
required for register access on runtime PM resume using
mt8188_afe_enable_reg_rw_clk().

Maybe try enabling debug printk in sound/soc/mediatek/mt8188/mt8188-afe-clk.c
and see what that gives you? There are already debug messages around the
clk enable/disable calls.

Either there's a bug or incorrect description of the clock tree,
or the AFE device node is referencing the wrong clocks.


ChenYu

> Thanks,
> Nícolas
>
> >
> > Thanks,
> > Trevor
> >
> > >
> > > That might give Nicolas a definitive hint about how to resolve this
> > > issue.
> > >
> > > Cheers,
> > > Angelo
> > >
> > > > Thanks,
> > > > Trevor
> > > >
> > > > >
> > > > > @@ -594,6 +596,8 @@ int mt8188_afe_disable_reg_rw_clk(struct
> > > > > mtk_base_afe *afe)
> > > > >   {
> > > > >          struct mt8188_afe_private *afe_priv = afe-
> > > > > >platform_priv;
> > > > >
> > > > > +       mt8188_afe_disable_clk(afe, afe_priv-
> > > > > > clk[MT8188_CLK_APMIXED_APLL1]);
> > > > >
> > > > > +
> > > > >          mt8188_afe_disable_clk(afe, afe_priv-
> > > > > > clk[MT8188_CLK_AUD_A1SYS]);
> > > > >
> > > > >          mt8188_afe_disable_clk(afe, afe_priv-
> > > > > > clk[MT8188_CLK_AUD_A1SYS_HP]);
> > > > >
> > > > >          mt8188_afe_disable_clk(afe, afe_priv-
> > > > > > clk[MT8188_CLK_AUD_AFE]);
> > > > >
> > > > > ---
> > > > > base-commit: b852e1e7a0389ed6168ef1d38eb0bad71a6b11e8
> > > > > change-id: 20241203-mt8188-afe-fix-hang-disabled-apll1-clk-
> > > > > b3c11782cbaf
> > > > >
> > > > > Best regards,
> > > > > --
> > > > > Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
> > > > >
> > > > >
> > >
> > >
>