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

["Trevor Wu (吳文良)" <Trevor.Wu@mediatek.com>]

On Mon, 2024-12-09 at 17:07 -0300, Nícolas F. R. A. Prado wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> 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@collabora.com>
> > > > > ---
> > > > >   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..fb8cf286df3f02ac076
> > > > > 528b
> > > > > 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?


If I remember correctly, the reason for assigning 26m as the parent of
a1sys_hp is to save power[1], as APLL is not necessary in all cases.

[1] 
https://lore.kernel.org/r/20230510035526.18137-5-trevor.wu@mediatek.com

Thanks,
Trevor

> 
> 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.
> 
> 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@collabora.com>
> > > > > 
> > > > > 
> > > 
> > >