clk: jz4725b: fix mmc0 clock gating
Subsystems (4)
cover clk: jz4725b: fix mmc0 clock gating
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.
Siarhei Volkau (1):
clk: jz4725b: fix mmc0 clock gating
drivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Signature · 1 line
2.35.1
PATCH 1/1 clk: jz4725b: fix mmc0 clock gating
--- drivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/clk/ingenic/jz4725b-cgu.c +1 -2
@@ -139,11 +139,10 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = { }, [JZ4725B_CLK_I2S] = { -"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV |CGU_CLK_GATE , +"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV , .parents = { JZ4725B_CLK_EXT, JZ4725B_CLK_PLL_HALF, -1, -1 }, .mux = { CGU_REG_CPCCR, 31, 1 }, .div = { CGU_REG_I2SCDR, 0, 1, 9, -1, -1, -1 }, - .gate = { CGU_REG_CLKGR, 6 }, }, [JZ4725B_CLK_SPI] = {
Signature · 1 line
2.35.1
Paul Cercueil
maintainerHi Siarhei,
Quoted 6 lines · Siarhei Volkau
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_SLEEPto work properly.
I guess it was introduced by copy-paste from jz4740 driver where it is really controls I2S clock gate.
This is useful information, please use the same text in the commit message.
However... My JZ4725B programming manual does say that the MMC0 clock is gated with bit 6, and the I2S clock has no gating bit.
Where did you find this info?
Cheers,
-Paul
Quoted 10 lines
Siarhei Volkau (1):
clk: jz4725b: fix mmc0 clock gatingdrivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)--
2.35.1
пт, 4 февр. 2022 г. в 09:24, Paul Cercueil <[email protected]>:
Quoted 2 lines
Hi Siarhei,
Hi Paul,
Quoted 11 lines
Quoted 6 lines · Siarhei Volkau
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_SLEEPto work properly.
I guess it was introduced by copy-paste from jz4740 driver where it is really controls I2S clock gate.This is useful information, please use the same text in the commit message.
Ok.
Quoted 5 lines
However... My JZ4725B programming manual does say that the MMC0 clock is gated with bit 6, and the I2S clock has no gating bit.
Where did you find this info?
My programming manual says exactly the same, but look at the clk driver source - the bit 6
is used in both clock domains (i2s and mmc0). That's the patch intended to fix.
Background: I'm trying to port OpenDingux to a Ritmix RZX-27 device. It uses mmc0 as main storage. That's where the problem arises - after init.d/alsa-hack.sh
mmc0 hungs for ~5 minutes till sdmmc core does reset it.
Quoted 16 lines
Cheers,
-PaulQuoted 10 lines
Siarhei Volkau (1):
clk: jz4725b: fix mmc0 clock gatingdrivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)--
2.35.1
v2 cover clk: jz4725b: fix mmc0 clock gating
In reply to Paul CercueilThe 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(-)
Signature · 1 line
2.35.1
PATCH v2 1/1 clk: jz4725b: fix mmc0 clock gating
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.
--- drivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/clk/ingenic/jz4725b-cgu.c +1 -2
@@ -139,11 +139,10 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = { }, [JZ4725B_CLK_I2S] = { -"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV |CGU_CLK_GATE , +"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV , .parents = { JZ4725B_CLK_EXT, JZ4725B_CLK_PLL_HALF, -1, -1 }, .mux = { CGU_REG_CPCCR, 31, 1 }, .div = { CGU_REG_I2SCDR, 0, 1, 9, -1, -1, -1 }, - .gate = { CGU_REG_CLKGR, 6 }, }, [JZ4725B_CLK_SPI] = {
Signature · 1 line
2.35.1
Paul Cercueil
maintainerHi,
Quoted 6 lines · Siarhei Volkau
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_SLEEPto 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
пт, 4 февр. 2022 г. в 14:06, Paul Cercueil <[email protected]>:
Quoted 11 lines
Hi,
Le ven., févr. 4 2022 at 10:21:40 +0300, Siarhei Volkau <[email protected]> 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
Hi,
Quoted 10 lines · Siarhei Volkau
пт, 4 февр. 2022 г. в 14:06, Paul Cercueil <[email protected]>:
Quoted 9 lines
Hi,
Le ven., févr. 4 2022 at 10:21:40 +0300, Siarhei Volkau <[email protected]> a écrit :
Can you confirm that the MMC0 controller will work even with the
bit 6
off?
Quoted 4 lines
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 6 lines
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:
Since it's a bug fix, you need to also add a fixes tag:
And add <[email protected]> in the Cc: list.
Cheers,
-Paul
v3 cover clk: jz4725b: fix mmc0 clock gating
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 v2 .. v3:
- Added tags Fixes and Reviewed-by.
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(-)
Signature · 1 line
2.35.1
PATCH v3 1/1 clk: jz4725b: fix mmc0 clock gating
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.
--- drivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/clk/ingenic/jz4725b-cgu.c +1 -2
@@ -139,11 +139,10 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = { }, [JZ4725B_CLK_I2S] = { -"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV |CGU_CLK_GATE , +"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV , .parents = { JZ4725B_CLK_EXT, JZ4725B_CLK_PLL_HALF, -1, -1 }, .mux = { CGU_REG_CPCCR, 31, 1 }, .div = { CGU_REG_I2SCDR, 0, 1, 9, -1, -1, -1 }, - .gate = { CGU_REG_CLKGR, 6 }, }, [JZ4725B_CLK_SPI] = {
Signature · 1 line
2.35.1
Greg KH
maintainerQuoted 35 lines · Siarhei Volkau
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_SLEEPto work properly.
I guess it was introduced by copy-paste from jz4740 driver where it is really controls I2S clock gate.Fixes 226dfa4726eb ("clk: Add Ingenic jz4725b CGU driver")Signed-off-by Siarhei Volkau <[email protected]>Tested-by Siarhei Volkau <[email protected]>Reviewed-by Paul Cercueil <[email protected]>--- drivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/clk/ingenic/jz4725b-cgu.c +1
-2@@ -139,11 +139,10 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = { }, [JZ4725B_CLK_I2S] = { -"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV |CGU_CLK_GATE , +"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV , .parents = { JZ4725B_CLK_EXT, JZ4725B_CLK_PLL_HALF, -1, -1 }, .mux = { CGU_REG_CPCCR, 31, 1 }, .div = { CGU_REG_I2SCDR, 0, 1, 9, -1, -1, -1 }, - .gate = { CGU_REG_CLKGR, 6 }, }, [JZ4725B_CLK_SPI] = {Signature · 1 line
2.35.1
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
Paul Cercueil
maintainerHi Greg,
Quoted 41 lines · Greg KH
Quoted 33 lines · Siarhei Volkau
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_SLEEPto work properly.
I guess it was introduced by copy-paste from jz4740 driver where it is really controls I2S clock gate.Fixes 226dfa4726eb ("clk: Add Ingenic jz4725b CGU driver")Signed-off-by Siarhei Volkau <[email protected]>Tested-by Siarhei Volkau <[email protected]>Reviewed-by Paul Cercueil <[email protected]>---
drivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)diff --git
a/drivers/clk/ingenic/jz4725b-cgu.cb/drivers/clk/ingenic/jz4725b-cgu.c
index 744d136..15d6179 100644
---a/drivers/clk/ingenic/jz4725b-cgu.c
+++b/drivers/clk/ingenic/jz4725b-cgu.c
@@ -139,11 +139,10 @@ static const structingenic_cgu_clk_info
jz4725b_cgu_clocks[] = {
},[
JZ4725B_CLK_I2S] = {
- "i2s",CGU_CLK_MUX|CGU_CLK_DIV|CGU_CLK_GATE,
+ "i2s",CGU_CLK_MUX|CGU_CLK_DIV,
.parents = {JZ4725B_CLK_EXT,JZ4725B_CLK_PLL_HALF, -1, -1 }, .mux = {CGU_REG_CPCCR, 31, 1 },
.div = {CGU_REG_I2SCDR, 0, 1, 9, -1, -1, -1 },
- .gate = {CGU_REG_CLKGR, 6 },
},Quoted 4 lines
[
JZ4725B_CLK_SPI] = {
--
2.35.1<formletter>
Quoted 7 lines
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.</formletter>
What's wrong with this patch exactly? It looks good to me.
Cheers,
-Paul
Greg KH
maintainerQuoted 45 lines · Paul Cercueil
Hi Greg,
Quoted 41 lines · Greg KH
Quoted 33 lines · Siarhei Volkau
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_SLEEPto work properly.
I guess it was introduced by copy-paste from jz4740 driver where it is really controls I2S clock gate.Fixes 226dfa4726eb ("clk: Add Ingenic jz4725b CGU driver")Signed-off-by Siarhei Volkau <[email protected]>Tested-by Siarhei Volkau <[email protected]>Reviewed-by Paul Cercueil <[email protected]>---
drivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)diff --git
a/drivers/clk/ingenic/jz4725b-cgu.cb/drivers/clk/ingenic/jz4725b-cgu.c
index 744d136..15d6179 100644
---a/drivers/clk/ingenic/jz4725b-cgu.c
+++b/drivers/clk/ingenic/jz4725b-cgu.c
@@ -139,11 +139,10 @@ static const structingenic_cgu_clk_info
jz4725b_cgu_clocks[] = {
},[
JZ4725B_CLK_I2S] = {
- "i2s",CGU_CLK_MUX|CGU_CLK_DIV|CGU_CLK_GATE,
+ "i2s",CGU_CLK_MUX|CGU_CLK_DIV,
.parents = {JZ4725B_CLK_EXT,JZ4725B_CLK_PLL_HALF, -1, -1 }, .mux = {CGU_REG_CPCCR, 31, 1 },
.div = {CGU_REG_I2SCDR, 0, 1, 9, -1, -1, -1 },
- .gate = {CGU_REG_CLKGR, 6 },
},Quoted 4 lines
[
JZ4725B_CLK_SPI] = {
--
2.35.1<formletter>
Quoted 8 lines
Quoted 6 lines
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.</formletter>
What's wrong with this patch exactly? It looks good to me.
No "Cc: stable@..." in the signed-off-by area.
v4 cover clk: jz4725b: fix mmc0 clock gating
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 v3 .. v4:
- Added tag "Cc: stable@..."
Changelog v2 .. v3:
- Added tags Fixes and Reviewed-by.
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(-)
Signature · 1 line
2.35.1
PATCH v4 1/1 clk: jz4725b: fix mmc0 clock gating
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.
--- drivers/clk/ingenic/jz4725b-cgu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/clk/ingenic/jz4725b-cgu.c +1 -2
@@ -139,11 +139,10 @@ static const struct ingenic_cgu_clk_info jz4725b_cgu_clocks[] = { }, [JZ4725B_CLK_I2S] = { -"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV |CGU_CLK_GATE , +"i2s" ,CGU_CLK_MUX |CGU_CLK_DIV , .parents = { JZ4725B_CLK_EXT, JZ4725B_CLK_PLL_HALF, -1, -1 }, .mux = { CGU_REG_CPCCR, 31, 1 }, .div = { CGU_REG_I2SCDR, 0, 1, 9, -1, -1, -1 }, - .gate = { CGU_REG_CLKGR, 6 }, }, [JZ4725B_CLK_SPI] = {
Signature · 1 line
2.35.1
Quoting Siarhei Volkau (2022-02-05 09:18:49)
Quoted 7 lines
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_SLEEPto work properly.
I guess it was introduced by copy-paste from jz4740 driver where it is really controls I2S clock gate.
Quoted 6 lines
Fixes 226dfa4726eb ("clk: Add Ingenic jz4725b CGU driver")Signed-off-by Siarhei Volkau <[email protected]>Tested-by Siarhei Volkau <[email protected]>Reviewed-by Paul Cercueil <[email protected]>---
In the future please don't send patches in reply to previous versions. I don't see in thread view that this has been sent many times.
Applied to clk-fixes