[v2,0/1] clk: jz4725b: fix mmc0 clock gating

Message

Siarhei Volkau Feb. 4, 2022, 7:21 a.m. UTC

The mmc0 clock gate bit was mistakenly assigned to "i2s" clock.
You can find that the same bit is assigned to "mmc0" too.
It leads to mmc0 hang for a long time after any sound activity
also it  prevented PM_SLEEP to work properly.
I guess it was introduced by copy-paste from jz4740 driver
where it is really controls I2S clock gate.

Changelog v1 .. v2:
 - Added useful info above to the commit itself.

Siarhei Volkau (1):
  clk: jz4725b: fix mmc0 clock gating

 drivers/clk/ingenic/jz4725b-cgu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Paul Cercueil Feb. 4, 2022, 11:05 a.m. UTC | #1

Hi,

Le ven., févr. 4 2022 at 10:21:40 +0300, Siarhei Volkau 
<[e-mail]> a écrit :
> The mmc0 clock gate bit was mistakenly assigned to "i2s" clock.
> You can find that the same bit is assigned to "mmc0" too.
> It leads to mmc0 hang for a long time after any sound activity
> also it  prevented PM_SLEEP to work properly.
> I guess it was introduced by copy-paste from jz4740 driver
> where it is really controls I2S clock gate.

Ok, I was wondering why it didn't happen on other JZ4725B devices, it 
turns out they all use NAND + mmc1, and mmc0 is unused.

Can you confirm that the MMC0 controller will work even with the bit 6 
off?

Do you know if the MMC0 bit has been moved elsewhere, or the MMC0 
controller is permanently enabled?

Cheers,
-Paul

Siarhei Volkau Feb. 4, 2022, 6:22 p.m. UTC | #2

пт, 4 февр. 2022 г. в 14:06, Paul Cercueil <[e-mail]>:
>
> Hi,
>
> Le ven., févr. 4 2022 at 10:21:40 +0300, Siarhei Volkau
> <[e-mail]> a écrit :
> Can you confirm that the MMC0 controller will work even with the bit 6
> off?
>
> Do you know if the MMC0 bit has been moved elsewhere, or the MMC0
> controller is permanently enabled?
>
Do you mean when the clock is off (bit == 1) or bit == 0 (the clock is on)?

Anyway what I tried:
I tried not enabling this clock in the UBIBoot - it just works. I guess
boot ROM enables it earlier.
But when I explicitly disable the clock it doesn't boot.
So I assume bit 6 definitely gates the MMC0
clock, as stated in the jz4725b programmer's manual.

Then I disabled MMC0 in Ubiboot to run Linux from MMC1 and tested
the audio with disabled MMC0, in dts mmc0 node was disabled as well.
With this patch applied sound works well. So I guess the bit 6 isn't
related to the I2S clocking at all.

Hope I answered your questions.

BR, Siarhei

Paul Cercueil Feb. 4, 2022, 8:47 p.m. UTC | #3

Hi,

Le ven., févr. 4 2022 at 21:22:04 +0300, Siarhei Volkau 
<[e-mail]> a écrit :
> пт, 4 февр. 2022 г. в 14:06, Paul Cercueil 
> <[e-mail]>:
>> 
>>  Hi,
>> 
>>  Le ven., févr. 4 2022 at 10:21:40 +0300, Siarhei Volkau
>>  <[e-mail]> a écrit :
>>  Can you confirm that the MMC0 controller will work even with the 
>> bit 6
>>  off?
>> 
>>  Do you know if the MMC0 bit has been moved elsewhere, or the MMC0
>>  controller is permanently enabled?
>> 
> Do you mean when the clock is off (bit == 1) or bit == 0 (the clock 
> is on)?

By "permanently enabled" I meant that maybe there is no bit in the 
CLKGR register that can gate this clock.

> Anyway what I tried:
> I tried not enabling this clock in the UBIBoot - it just works. I 
> guess
> boot ROM enables it earlier.

The mmc0 clock is ungated by default at bootup, yes.

> But when I explicitly disable the clock it doesn't boot.
> So I assume bit 6 definitely gates the MMC0
> clock, as stated in the jz4725b programmer's manual.

Ok, so bit 6 is indeed MMC0 and not I2S.

> Then I disabled MMC0 in Ubiboot to run Linux from MMC1 and tested
> the audio with disabled MMC0, in dts mmc0 node was disabled as well.
> With this patch applied sound works well. So I guess the bit 6 isn't
> related to the I2S clocking at all.
> 
> Hope I answered your questions.

Ok, then:
Reviewed-by: Paul Cercueil <[e-mail]>

Since it's a bug fix, you need to also add a fixes tag:
Fixes: 226dfa4726eb ("clk: Add Ingenic jz4725b CGU driver")

And add <[e-mail]> in the Cc: list.

Cheers,
-Paul