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

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

[Nícolas F. R. A. Prado]

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..fb8cf286df3f02ac076528b898fd0d7a708ec1ea 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;
 }
 
@@ -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>

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

[Trevor Wu (吳文良)]

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

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

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

[AngeloGioacchino Del Regno]

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

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

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

[Trevor Wu (吳文良)]

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

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

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

[Nícolas F. R. A. Prado]

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

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

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

[Chen-Yu Tsai]

On Tue, Dec 10, 2024 at 4:08 AM Nícolas F. R. A. Prado
<nfraprado@collabora.com> 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@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..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@collabora.com>
> > > > >
> > > > >
> > >
> > >
>

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

[Trevor Wu (吳文良)]

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

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

[Nícolas F. R. A. Prado]

On Wed, Dec 11, 2024 at 01:47:32PM +0000, Trevor Wu (吳文良) wrote:
> 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

Hi,

Following the suggestions, I have enabled the debug logs in 
mt8188-afe-clk.c, though that didn't reveal anything (the logs are only printed
in case of errors, so there are none). I also verified that forcing the
ADSP_INFRA power domain to be powered on during boot (and before these register
reads occur) doesn't change anything (this domain is marked as ALWAYS_ON, so
normally assumed to already be on and not touched).

I've also narrowed the issue further. The hang happens in

mt8188_afe_pcm_dev_probe()
  devm_regmap_init_mmio()
    regcache_hw_init()

specifically in the "fill the reg_defaults" loop.

What's more, it doesn't happen for every register, but rather a few specific
ones.

The following register reads caused the hang:
0x280
0x410 (AFE_GAIN1_CON0)
0x830 (AFE_CONN0_5)

The following ranges were read without a problem:
0x0   (AUDIO_TOP_CON0)     ... 0x27c
0x380 (AFE_SPDIF_OUT_CON0) ... 0x40c
0x550 (SPDIFIN_FREQ_INFO)  ... 0x82c

And again, if I enable the apll1 clock in mt8188_afe_enable_reg_rw_clk(), as
done in this patch, or parent a1sys_hp to apll1_d4 in the AFE DT node, then
those registers are read without any hangs.

So it seems that certain registers require the apll1 clock to be enabled
in order to be read, while others don't. Trevor, is the dependency between those
specific registers and the apll1 clock documented anywhere?

Unless there's a better solution, I think we should go forward with either
this patch or the DT clock parenting suggested by Angelo, as the power savings
come second to the machine actually being bootable.

Thanks,
Nícolas