mmc: jz4740: Don't change parent clock rate for some SoCs

PATCH In review Siarhei Volkau 4 participants 11 messages Nov 8, 2022 – Nov 18, 2022

Subsystems (4)

Device tree bindings MIPS MMC, SD and SDIO Ingenic JZ47xx SoCs

lore

Share & tools

lore

Siarhei Volkau

author

2022-11-08 04:57 UTC #0 lore raw

cover mmc: jz4740: Don't change parent clock rate for some SoCs

Some SoCs have one clock divider for all MMC units, thus changing one affects others as well. This leads to random hangs and memory corruptions, observed on the JZ4755 based device with two MMC slots used at the same time.

List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and JZ4760b.

The MMC core has its own clock divisor and it goes to the first plan in that case.

Siarhei Volkau (2):
mmc: jz4740: Don't change parent clock rate for some SoCs
MIPS: ingenic: rs90: set MMC_MUX clock

 arch/mips/boot/dts/ingenic/rs90.dts |  5 +++--
 drivers/mmc/host/jz4740_mmc.c       | 10 +++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

Signature · 1 line

2.36.1

Siarhei Volkau

author

2022-11-08 04:57 UTC #1 lore raw

PATCH 1/2 mmc: jz4740: Don't change parent clock rate for some SoCs

List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and JZ4760b. However, the MMC driver doesn't distinguish JZ4760 and JZ4770 which shall remain its behavior. For the JZ4755 is sufficient to use JZ4725b's binding. JZ4750 is outside of the patch.

The MMC core has its own clock divisor, rather coarse but suitable well, and it shall keep the role of tuning clock for the MMC host in that case.

Signed-off-by Siarhei Volkau <[email protected]>

---
 drivers/mmc/host/jz4740_mmc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

drivers/mmc/host/jz4740_mmc.c +9 -1

index dc2db9c18..d390ff31d 100644
@@ -114,6 +114,7 @@ enum jz4740_mmc_version {
 	JZ_MMC_JZ4740,
 	JZ_MMC_JZ4725B,
 	JZ_MMC_JZ4760,
+	JZ_MMC_JZ4770,
 	JZ_MMC_JZ4780,
 	JZ_MMC_X1000,
 };

@@ -887,7 +888,13 @@ static int jz4740_mmc_set_clock_rate(struct jz4740_mmc_host *host, int rate)
 	int real_rate;
 
 	jz4740_mmc_clock_disable(host);
-	clk_set_rate(host->clk, host->mmc->f_max);
+
+	/*
+	 * Changing rate on these SoCs affects other MMC units too.
+	 * Make sure the rate is configured properly by the CGU driver.
+	 */
+	if (host->version != JZ_MMC_JZ4725B && host->version != JZ_MMC_JZ4760)
+		clk_set_rate(host->clk, host->mmc->f_max);
 
 	real_rate = clk_get_rate(host->clk);

@@ -992,6 +999,7 @@ static const struct of_device_id jz4740_mmc_of_match[] = {
 	{ .compatible = "ingenic,jz4740-mmc", .data = (void *) JZ_MMC_JZ4740 },
 	{ .compatible = "ingenic,jz4725b-mmc", .data = (void *)JZ_MMC_JZ4725B },
 	{ .compatible = "ingenic,jz4760-mmc", .data = (void *) JZ_MMC_JZ4760 },
+	{ .compatible = "ingenic,jz4770-mmc", .data = (void *) JZ_MMC_JZ4770 },
 	{ .compatible = "ingenic,jz4775-mmc", .data = (void *) JZ_MMC_JZ4780 },
 	{ .compatible = "ingenic,jz4780-mmc", .data = (void *) JZ_MMC_JZ4780 },
 	{ .compatible = "ingenic,x1000-mmc", .data = (void *) JZ_MMC_X1000 },

Signature · 1 line

2.36.1

Ulf Hansson

2022-11-18 08:47 UTC #2 lore raw

Quoted 5 lines · Siarhei Volkau

Some SoCs have one clock divider for all MMC units, thus changing one affects others as well. This leads to random hangs and memory corruptions, observed on the JZ4755 based device with two MMC slots used at the same time.

Urgh, that sounds like broken HW to me.

The MMC blocks could share a parent clock (that would need a fixed rate for it to be applied), assuming there is a separate gate/divider available per block. But there isn't'?

Quoted 9 lines

List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and JZ4760b. However, the MMC driver doesn't distinguish JZ4760 and JZ4770 which shall remain its behavior. For the JZ4755 is sufficient to use JZ4725b's binding. JZ4750 is outside of the patch.
The MMC core has its own clock divisor, rather coarse but suitable well, and it shall keep the role of tuning clock for the MMC host in that case.

The mmc core doesn't have a clock divisor, but it does control the bus clock frequency through the ->set_ios() host ops. It needs to do that, to be able to conform to the (e)MMC, SD and SDIO specifications.

Can you please try to elaborate on the above, so I can better understand your point?

Quoted 2 lines

Signed-off-by Siarhei Volkau <[email protected]>

Kind regards
Uffe

Quoted 42 lines

---
 drivers/mmc/host/jz4740_mmc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

drivers/mmc/host/jz4740_mmc.c +9 -2

index dc2db9c18..d390ff31d 100644
@@ -114,6 +114,7 @@ enum jz4740_mmc_version {
        JZ_MMC_JZ4740,
        JZ_MMC_JZ4725B,
        JZ_MMC_JZ4760,
+       JZ_MMC_JZ4770,
        JZ_MMC_JZ4780,
        JZ_MMC_X1000,
 };

@@ -887,7 +888,13 @@ static int jz4740_mmc_set_clock_rate(struct jz4740_mmc_host *host, int rate)
        int real_rate;
 
        jz4740_mmc_clock_disable(host);
-       clk_set_rate(host->clk, host->mmc->f_max);
+
+       /*
+        * Changing rate on these SoCs affects other MMC units too.
+        * Make sure the rate is configured properly by the CGU driver.
+        */
+       if (host->version != JZ_MMC_JZ4725B && host->version != JZ_MMC_JZ4760)
+               clk_set_rate(host->clk, host->mmc->f_max);
 
        real_rate = clk_get_rate(host->clk);

@@ -992,6 +999,7 @@ static const struct of_device_id jz4740_mmc_of_match[] = {
        { .compatible = "ingenic,jz4740-mmc", .data = (void *) JZ_MMC_JZ4740 },
        { .compatible = "ingenic,jz4725b-mmc", .data = (void *)JZ_MMC_JZ4725B },
        { .compatible = "ingenic,jz4760-mmc", .data = (void *) JZ_MMC_JZ4760 },
+       { .compatible = "ingenic,jz4770-mmc", .data = (void *) JZ_MMC_JZ4770 },
        { .compatible = "ingenic,jz4775-mmc", .data = (void *) JZ_MMC_JZ4780 },
        { .compatible = "ingenic,jz4780-mmc", .data = (void *) JZ_MMC_JZ4780 },
        { .compatible = "ingenic,x1000-mmc", .data = (void *) JZ_MMC_X1000 },
--

2.36.1

Paul Cercueil

maintainer

2022-11-18 09:27 UTC #3 lore raw

Hi,

(Ingenic SoCs maintainer here)

Quoted 8 lines · Ulf Hansson

Quoted 6 lines · Siarhei Volkau
Some SoCs have one clock divider for all MMC units, thus changing
one
affects others as well. This leads to random hangs and memory corruptions, observed on the JZ4755 based device with two MMC slots used at the same time.

Quoted 5 lines

Urgh, that sounds like broken HW to me.
The MMC blocks could share a parent clock (that would need a fixed rate for it to be applied), assuming there is a separate gate/divider available per block. But there isn't'?

They do share a parent clock and have separate gates, and each MMC IP block has an internal divider for the bus frequency derived from that shared clock.

Quoted 11 lines

Quoted 10 lines
List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and JZ4760b. However, the MMC driver doesn't distinguish JZ4760 and JZ4770 which shall remain its behavior. For the JZ4755 is sufficient to use JZ4725b's binding. JZ4750 is outside of the patch.
The MMC core has its own clock divisor, rather coarse but suitable
well,
and it shall keep the role of tuning clock for the MMC host in that case.

Quoted 6 lines

The mmc core doesn't have a clock divisor, but it does control the bus clock frequency through the ->set_ios() host ops. It needs to do that, to be able to conform to the (e)MMC, SD and SDIO specifications.
Can you please try to elaborate on the above, so I can better understand your point?

Yes, I don't really understand the patch, TBH.

The "clk_set_rate" call will only set the shared clock to the *maximum* clock frequency (host->mmc->f_max) which should be the exact same across all MMC IPs.

So it doesn't matter if it's set 3 times by 3 different instances of the IP, as long as they all request the same value.

Besides, I know for a fact that the mainline driver works fine on the JZ4760(B) and JZ4725B.

Finally... even if it was correct, this change would break compatibility with old Device Tree files.

Cheers,
-Paul

Quoted 60 lines

Quoted 2 lines
Signed-off-by Siarhei Volkau <[email protected]>
Kind regards
Uffe
Quoted 54 lines
---
  drivers/mmc/host/jz4740_mmc.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/jz4740_mmc.c
b/drivers/mmc/host/jz4740_mmc.c
index dc2db9c18..d390ff31d 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
 @@ -114,6 +114,7 @@ enum jz4740_mmc_version {
         JZ_MMC_JZ4740,
         JZ_MMC_JZ4725B,
         JZ_MMC_JZ4760,
 +       JZ_MMC_JZ4770,
         JZ_MMC_JZ4780,
         JZ_MMC_X1000,
  };
 @@ -887,7 +888,13 @@ static int jz4740_mmc_set_clock_rate(struct 
jz4740_mmc_host *host, int rate)
         int real_rate;

         jz4740_mmc_clock_disable(host);
- clk_set_rate(host->clk, host->mmc->f_max);
+
+ /*
+ * Changing rate on these SoCs affects other MMC units too.
+ * Make sure the rate is configured properly by the CGU
driver.
+ */
+ if (host->version != JZ_MMC_JZ4725B && host->version !=
JZ_MMC_JZ4760)
+ clk_set_rate(host->clk, host->mmc->f_max);
         real_rate = clk_get_rate(host->clk);
@@ -992,6 +999,7 @@ static const struct of_device_id
jz4740_mmc_of_match[] = {
         { .compatible = "ingenic,jz4740-mmc", .data = (void *) 
JZ_MMC_JZ4740 },
         { .compatible = "ingenic,jz4725b-mmc", .data = (void 
*)JZ_MMC_JZ4725B },
         { .compatible = "ingenic,jz4760-mmc", .data = (void *) 
JZ_MMC_JZ4760 },
 +       { .compatible = "ingenic,jz4770-mmc", .data = (void *) 
JZ_MMC_JZ4770 },
         { .compatible = "ingenic,jz4775-mmc", .data = (void *) 
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,jz4780-mmc", .data = (void *) 
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,x1000-mmc", .data = (void *) 
JZ_MMC_X1000 },
--
2.36.1

Siarhei Volkau

author

2022-11-18 09:52 UTC #4 lore raw

пт, 18 нояб. 2022 г. в 12:27, Paul Cercueil <[email protected]>:

Quoted 40 lines

Hi,
(Ingenic SoCs maintainer here)
Quoted 8 lines · Ulf Hansson
Quoted 6 lines · Siarhei Volkau
Some SoCs have one clock divider for all MMC units, thus changing
one
affects others as well. This leads to random hangs and memory corruptions, observed on the JZ4755 based device with two MMC slots used at the same time.
Quoted 5 lines
Urgh, that sounds like broken HW to me.
The MMC blocks could share a parent clock (that would need a fixed rate for it to be applied), assuming there is a separate gate/divider available per block. But there isn't'?
They do share a parent clock and have separate gates, and each MMC IP block has an internal divider for the bus frequency derived from that shared clock.
Quoted 15 lines
Quoted 10 lines
List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and JZ4760b. However, the MMC driver doesn't distinguish JZ4760 and JZ4770 which shall remain its behavior. For the JZ4755 is sufficient to use JZ4725b's binding. JZ4750 is outside of the patch.
The MMC core has its own clock divisor, rather coarse but suitable
well,
and it shall keep the role of tuning clock for the MMC host in that case.
The mmc core doesn't have a clock divisor, but it does control the bus clock frequency through the ->set_ios() host ops. It needs to do that, to be able to conform to the (e)MMC, SD and SDIO specifications.

Quoted 8 lines

Quoted 2 lines
Can you please try to elaborate on the above, so I can better understand your point?
Yes, I don't really understand the patch, TBH.
The "clk_set_rate" call will only set the shared clock to the *maximum* clock frequency (host->mmc->f_max) which should be the exact same across all MMC IPs.

That's the case I need different "f_max" for my HW, for some reason internal slot can't do a full rate (48MHz) but the external can, the same card used for checking. So I want to set 24M for mmc0, and 48M for mmc1 with respect to hardware limitation.

Quoted 75 lines

So it doesn't matter if it's set 3 times by 3 different instances of the IP, as long as they all request the same value.
Besides, I know for a fact that the mainline driver works fine on the JZ4760(B) and JZ4725B.
Finally... even if it was correct, this change would break compatibility with old Device Tree files.
Cheers,
-Paul
Quoted 60 lines
Quoted 2 lines
Signed-off-by Siarhei Volkau <[email protected]>
Kind regards
Uffe
Quoted 54 lines
---
  drivers/mmc/host/jz4740_mmc.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/jz4740_mmc.c
b/drivers/mmc/host/jz4740_mmc.c
index dc2db9c18..d390ff31d 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
 @@ -114,6 +114,7 @@ enum jz4740_mmc_version {
         JZ_MMC_JZ4740,
         JZ_MMC_JZ4725B,
         JZ_MMC_JZ4760,
 +       JZ_MMC_JZ4770,
         JZ_MMC_JZ4780,
         JZ_MMC_X1000,
  };
 @@ -887,7 +888,13 @@ static int jz4740_mmc_set_clock_rate(struct
jz4740_mmc_host *host, int rate)
         int real_rate;

         jz4740_mmc_clock_disable(host);
- clk_set_rate(host->clk, host->mmc->f_max);
+
+ /*
+ * Changing rate on these SoCs affects other MMC units too.
+ * Make sure the rate is configured properly by the CGU
driver.
+ */
+ if (host->version != JZ_MMC_JZ4725B && host->version !=
JZ_MMC_JZ4760)
+ clk_set_rate(host->clk, host->mmc->f_max);
         real_rate = clk_get_rate(host->clk);
@@ -992,6 +999,7 @@ static const struct of_device_id
jz4740_mmc_of_match[] = {
         { .compatible = "ingenic,jz4740-mmc", .data = (void *)
JZ_MMC_JZ4740 },
         { .compatible = "ingenic,jz4725b-mmc", .data = (void
*)JZ_MMC_JZ4725B },
         { .compatible = "ingenic,jz4760-mmc", .data = (void *)
JZ_MMC_JZ4760 },
 +       { .compatible = "ingenic,jz4770-mmc", .data = (void *)
JZ_MMC_JZ4770 },
         { .compatible = "ingenic,jz4775-mmc", .data = (void *)
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,jz4780-mmc", .data = (void *)
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,x1000-mmc", .data = (void *)
JZ_MMC_X1000 },
--
2.36.1

Ulf Hansson

2022-11-18 10:07 UTC #5 lore raw

Quoted 47 lines · Siarhei Volkau

пт, 18 нояб. 2022 г. в 12:27, Paul Cercueil <[email protected]>:
Quoted 45 lines
Hi,
(Ingenic SoCs maintainer here)
Quoted 8 lines · Ulf Hansson
Quoted 6 lines · Siarhei Volkau
Some SoCs have one clock divider for all MMC units, thus changing
one
affects others as well. This leads to random hangs and memory corruptions, observed on the JZ4755 based device with two MMC slots used at the same time.
Quoted 5 lines
Urgh, that sounds like broken HW to me.
The MMC blocks could share a parent clock (that would need a fixed rate for it to be applied), assuming there is a separate gate/divider available per block. But there isn't'?
They do share a parent clock and have separate gates, and each MMC IP block has an internal divider for the bus frequency derived from that shared clock.
Quoted 11 lines
Quoted 10 lines
List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and JZ4760b. However, the MMC driver doesn't distinguish JZ4760 and JZ4770 which shall remain its behavior. For the JZ4755 is sufficient to use JZ4725b's binding. JZ4750 is outside of the patch.
The MMC core has its own clock divisor, rather coarse but suitable
well,
and it shall keep the role of tuning clock for the MMC host in that case.
Quoted 6 lines
The mmc core doesn't have a clock divisor, but it does control the bus clock frequency through the ->set_ios() host ops. It needs to do that, to be able to conform to the (e)MMC, SD and SDIO specifications.
Can you please try to elaborate on the above, so I can better understand your point?
Yes, I don't really understand the patch, TBH.

Quoted 8 lines

Quoted 3 lines
The "clk_set_rate" call will only set the shared clock to the *maximum* clock frequency (host->mmc->f_max) which should be the exact same across all MMC IPs.
That's the case I need different "f_max" for my HW, for some reason internal slot can't do a full rate (48MHz) but the external can, the same card used for checking. So I want to set 24M for mmc0, and 48M for mmc1 with respect to hardware limitation.

This sounds like a board specific problem, right?

The simple solution would be to use 24M for both hosts, but that would unnecessarily degrade the speed for the host for the internal slot.

It sounds like we need a new DT binding to describe a capped max-frequency for the "broken slot". And in case that is available in the DTS, the mmc->f_max value should be overridden with it, while also respecting the original f_max value while calling clk_set_rate().

Br
Uffe

Siarhei Volkau

author

2022-11-18 12:48 UTC #6 lore raw

пт, 18 нояб. 2022 г. в 13:06, Ulf Hansson <[email protected]>:

Quoted 53 lines · Siarhei Volkau

Quoted 51 lines
пт, 18 нояб. 2022 г. в 12:27, Paul Cercueil <[email protected]>:
Quoted 48 lines
Hi,
(Ingenic SoCs maintainer here)
Quoted 8 lines · Ulf Hansson
Quoted 6 lines · Siarhei Volkau
Some SoCs have one clock divider for all MMC units, thus changing
one
affects others as well. This leads to random hangs and memory corruptions, observed on the JZ4755 based device with two MMC slots used at the same time.
Quoted 5 lines
Urgh, that sounds like broken HW to me.
The MMC blocks could share a parent clock (that would need a fixed rate for it to be applied), assuming there is a separate gate/divider available per block. But there isn't'?
They do share a parent clock and have separate gates, and each MMC IP block has an internal divider for the bus frequency derived from that shared clock.
Quoted 11 lines
Quoted 10 lines
List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and JZ4760b. However, the MMC driver doesn't distinguish JZ4760 and JZ4770 which shall remain its behavior. For the JZ4755 is sufficient to use JZ4725b's binding. JZ4750 is outside of the patch.
The MMC core has its own clock divisor, rather coarse but suitable
well,
and it shall keep the role of tuning clock for the MMC host in that case.
Quoted 6 lines
The mmc core doesn't have a clock divisor, but it does control the bus clock frequency through the ->set_ios() host ops. It needs to do that, to be able to conform to the (e)MMC, SD and SDIO specifications.
Can you please try to elaborate on the above, so I can better understand your point?
Yes, I don't really understand the patch, TBH.
The "clk_set_rate" call will only set the shared clock to the *maximum* clock frequency (host->mmc->f_max) which should be the exact same across all MMC IPs.

Quoted 6 lines

Quoted 4 lines
That's the case I need different "f_max" for my HW, for some reason internal slot can't do a full rate (48MHz) but the external can, the same card used for checking. So I want to set 24M for mmc0, and 48M for mmc1 with respect to hardware limitation.
This sounds like a board specific problem, right?

Exactly.

Quoted 4 lines

The simple solution would be to use 24M for both hosts, but that would unnecessarily degrade the speed for the host for the internal slot.

Indeed.

Quoted 7 lines

It sounds like we need a new DT binding to describe a capped max-frequency for the "broken slot". And in case that is available in the DTS, the mmc->f_max value should be overridden with it, while also respecting the original f_max value while calling clk_set_rate().
Br
Uffe

I think it is unnecessary, "max-frequency" is enough and clear to use here, it just needs to be tuned independently for each controller. However, the controllers have hidden dependency on each other. Break this dependency that's the subject of the patchset.

Siarhei

Paul Cercueil

maintainer

2022-11-18 13:04 UTC #7 lore raw

In reply to Siarhei Volkau

Hi Siarhei,

Quoted 62 lines · Siarhei Volkau

пт, 18 нояб. 2022 г. в 12:27, Paul Cercueil <[email protected]>:
Quoted 54 lines
Hi,
(Ingenic SoCs maintainer here)
Quoted 1 line · Ulf Hansson
On Tue, 8 Nov 2022 at 05:53, Siarhei Volkau <[email protected]>
Quoted 2 lines · wrote:
Quoted 2 lines
Some SoCs have one clock divider for all MMC units, thus
changing
Quoted 3 lines
Quoted 3 lines
one
affects others as well. This leads to random hangs and memory corruptions, observed on the JZ4755 based device with two MMC
slots
Quoted 6 lines
Quoted 1 line
used at the same time.
Urgh, that sounds like broken HW to me.
The MMC blocks could share a parent clock (that would need a fixed rate for it to be applied), assuming there is a separate
gate/divider
Quoted 1 line
available per block. But there isn't'?
They do share a parent clock and have separate gates, and each MMC
IP
block has an internal divider for the bus frequency derived from
that
shared clock.
Quoted 2 lines
Quoted 2 lines
List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and
JZ4760b.
Quoted 5 lines
Quoted 5 lines
However, the MMC driver doesn't distinguish JZ4760 and JZ4770 which shall remain its behavior. For the JZ4755 is sufficient to use JZ4725b's binding. JZ4750 is outside of the patch.
The MMC core has its own clock divisor, rather coarse but
suitable
Quoted 2 lines
Quoted 2 lines
well,
and it shall keep the role of tuning clock for the MMC host in
that
Quoted 3 lines
Quoted 1 line
case.
The mmc core doesn't have a clock divisor, but it does control
the bus
Quoted 1 line
clock frequency through the ->set_ios() host ops. It needs to do
that,
Quoted 4 lines
to be able to conform to the (e)MMC, SD and SDIO specifications.
Can you please try to elaborate on the above, so I can better understand your point?
Quoted 6 lines
Yes, I don't really understand the patch, TBH.
The "clk_set_rate" call will only set the shared clock to the
*maximum*
clock frequency (host->mmc->f_max) which should be the exact same across all MMC IPs.

Quoted 4 lines

That's the case I need different "f_max" for my HW, for some reason internal slot can't do a full rate (48MHz) but the external can, the same card used for checking. So I want to set 24M for mmc0, and 48M for mmc1 with respect to hardware limitation.

The JZ4760B programming manual states that the controller is "fully compatible with the SD Memory Card Specification 2.0". In that specification, the bus speed is max. 25 MHz.

The programming manual also says: "In data transfer mode, the MSC controller can operate card with clock rate fpp (0 ~ 25Mhz)."

So the max-frequency really should be 25 MHz.

Cheers,
-Paul

Quoted 77 lines

Quoted 77 lines
So it doesn't matter if it's set 3 times by 3 different instances of the IP, as long as they all request the same value.
Besides, I know for a fact that the mainline driver works fine on
the
JZ4760(B) and JZ4725B.
Finally... even if it was correct, this change would break compatibility with old Device Tree files.
Cheers,
-Paul
Quoted 32 lines
Quoted 2 lines
Signed-off-by Siarhei Volkau <[email protected]>
Kind regards
Uffe
Quoted 26 lines
---
  drivers/mmc/host/jz4740_mmc.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/jz4740_mmc.c
b/drivers/mmc/host/jz4740_mmc.c
index dc2db9c18..d390ff31d 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
 @@ -114,6 +114,7 @@ enum jz4740_mmc_version {
         JZ_MMC_JZ4740,
         JZ_MMC_JZ4725B,
         JZ_MMC_JZ4760,
 +       JZ_MMC_JZ4770,
         JZ_MMC_JZ4780,
         JZ_MMC_X1000,
  };
 @@ -887,7 +888,13 @@ static int jz4740_mmc_set_clock_rate(struct
jz4740_mmc_host *host, int rate)
         int real_rate;

         jz4740_mmc_clock_disable(host);
- clk_set_rate(host->clk, host->mmc->f_max);
+
+ /*
+ * Changing rate on these SoCs affects other MMC units
too.
Quoted 28 lines
Quoted 28 lines
+ * Make sure the rate is configured properly by the CGU
driver.
+ */
+ if (host->version != JZ_MMC_JZ4725B && host->version !=
JZ_MMC_JZ4760)
+ clk_set_rate(host->clk, host->mmc->f_max);
         real_rate = clk_get_rate(host->clk);
@@ -992,6 +999,7 @@ static const struct of_device_id
jz4740_mmc_of_match[] = {
         { .compatible = "ingenic,jz4740-mmc", .data = (void *)
JZ_MMC_JZ4740 },
         { .compatible = "ingenic,jz4725b-mmc", .data = (void
*)JZ_MMC_JZ4725B },
         { .compatible = "ingenic,jz4760-mmc", .data = (void *)
JZ_MMC_JZ4760 },
 +       { .compatible = "ingenic,jz4770-mmc", .data = (void *)
JZ_MMC_JZ4770 },
         { .compatible = "ingenic,jz4775-mmc", .data = (void *)
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,jz4780-mmc", .data = (void *)
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,x1000-mmc", .data = (void *)
JZ_MMC_X1000 },
--
2.36.1

Paul Cercueil

maintainer

2022-11-18 13:14 UTC #8 lore raw

Quoted 72 lines · Paul Cercueil

Hi Siarhei,
Quoted 64 lines · Siarhei Volkau
пт, 18 нояб. 2022 г. в 12:27, Paul Cercueil <[email protected]>:
Quoted 56 lines
Hi,
(Ingenic SoCs maintainer here)
Quoted 1 line · Ulf Hansson
On Tue, 8 Nov 2022 at 05:53, Siarhei Volkau <[email protected]>
Quoted 2 lines ·
Quoted 2 lines
Some SoCs have one clock divider for all MMC units, thus
changing
Quoted 3 lines
Quoted 3 lines
one
affects others as well. This leads to random hangs and memory corruptions, observed on the JZ4755 based device with two MMC
slots
Quoted 5 lines
Quoted 1 line
used at the same time.
Urgh, that sounds like broken HW to me.
The MMC blocks could share a parent clock (that would need a
fixed
Quoted 1 line
rate for it to be applied), assuming there is a separate
gate/divider
Quoted 1 line
available per block. But there isn't'?
They do share a parent clock and have separate gates, and each MMC
IP
block has an internal divider for the bus frequency derived from
that
shared clock.
Quoted 2 lines
Quoted 2 lines
List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and
JZ4760b.
Quoted 2 lines
Quoted 2 lines
However, the MMC driver doesn't distinguish JZ4760 and JZ4770 which shall remain its behavior. For the JZ4755 is sufficient
to
Quoted 3 lines
Quoted 3 lines
use JZ4725b's binding. JZ4750 is outside of the patch.
The MMC core has its own clock divisor, rather coarse but
suitable
Quoted 2 lines
Quoted 2 lines
well,
and it shall keep the role of tuning clock for the MMC host in
that
Quoted 3 lines
Quoted 1 line
case.
The mmc core doesn't have a clock divisor, but it does control
the bus
Quoted 1 line
clock frequency through the ->set_ios() host ops. It needs to do
that,
Quoted 4 lines
to be able to conform to the (e)MMC, SD and SDIO specifications.
Can you please try to elaborate on the above, so I can better understand your point?
Quoted 6 lines
Yes, I don't really understand the patch, TBH.
The "clk_set_rate" call will only set the shared clock to the
*maximum*
clock frequency (host->mmc->f_max) which should be the exact same across all MMC IPs.
Quoted 4 lines
That's the case I need different "f_max" for my HW, for some reason internal slot can't do a full rate (48MHz) but the external can, the same card used for checking. So I want to set 24M for mmc0, and 48M for mmc1 with respect to hardware limitation.

Quoted 5 lines

The JZ4760B programming manual states that the controller is "fully compatible with the SD Memory Card Specification 2.0". In that specification, the bus speed is max. 25 MHz.
The programming manual also says: "In data transfer mode, the MSC controller can operate card with clock rate fpp (0 ~ 25Mhz)."
So the max-frequency really should be 25 MHz.

Nevermind. I read wrong, at least the SD spec. (the quote of the programming manual is still concerning though).

It's rated for 25 MB/s, so 50 MHz on 4 lanes.

Cheers,
-Paul

Quoted 81 lines

Quoted 80 lines
Quoted 80 lines
So it doesn't matter if it's set 3 times by 3 different instances
of
the IP, as long as they all request the same value.
Besides, I know for a fact that the mainline driver works fine on
the
JZ4760(B) and JZ4725B.
Finally... even if it was correct, this change would break compatibility with old Device Tree files.
Cheers,
-Paul
Quoted 24 lines
Quoted 2 lines
Signed-off-by Siarhei Volkau <[email protected]>
Kind regards
Uffe
Quoted 18 lines
---
  drivers/mmc/host/jz4740_mmc.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/jz4740_mmc.c
b/drivers/mmc/host/jz4740_mmc.c
index dc2db9c18..d390ff31d 100644
--- a/drivers/mmc/host/jz4740_mmc.c
+++ b/drivers/mmc/host/jz4740_mmc.c
 @@ -114,6 +114,7 @@ enum jz4740_mmc_version {
         JZ_MMC_JZ4740,
         JZ_MMC_JZ4725B,
         JZ_MMC_JZ4760,
 +       JZ_MMC_JZ4770,
         JZ_MMC_JZ4780,
         JZ_MMC_X1000,
};
@@ -887,7 +888,13 @@ static int
jz4740_mmc_set_clock_rate(struct
Quoted 8 lines
Quoted 8 lines
jz4740_mmc_host *host, int rate)
         int real_rate;

         jz4740_mmc_clock_disable(host);
- clk_set_rate(host->clk, host->mmc->f_max);
+
+ /*
+ * Changing rate on these SoCs affects other MMC units
too.
Quoted 28 lines
Quoted 28 lines
+ * Make sure the rate is configured properly by the CGU
driver.
+ */
+ if (host->version != JZ_MMC_JZ4725B && host->version !=
JZ_MMC_JZ4760)
+ clk_set_rate(host->clk, host->mmc->f_max);
         real_rate = clk_get_rate(host->clk);
@@ -992,6 +999,7 @@ static const struct of_device_id
jz4740_mmc_of_match[] = {
         { .compatible = "ingenic,jz4740-mmc", .data = (void *)
JZ_MMC_JZ4740 },
         { .compatible = "ingenic,jz4725b-mmc", .data = (void
*)JZ_MMC_JZ4725B },
         { .compatible = "ingenic,jz4760-mmc", .data = (void *)
JZ_MMC_JZ4760 },
 +       { .compatible = "ingenic,jz4770-mmc", .data = (void *)
JZ_MMC_JZ4770 },
         { .compatible = "ingenic,jz4775-mmc", .data = (void *)
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,jz4780-mmc", .data = (void *)
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,x1000-mmc", .data = (void *)
JZ_MMC_X1000 },
--
2.36.1

Aidan MacDonald

2022-11-18 14:53 UTC #9 lore raw

In reply to Paul Cercueil

Quoted 32 lines · Paul Cercueil

Hi,
(Ingenic SoCs maintainer here)
Le ven. 18 nov. 2022 à 09:45:48 +0100, Ulf Hansson <[email protected]> a écrit :
Quoted 9 lines · Siarhei Volkau
Quoted 4 lines
Some SoCs have one clock divider for all MMC units, thus changing one affects others as well. This leads to random hangs and memory corruptions, observed on the JZ4755 based device with two MMC slots used at the same time.
Urgh, that sounds like broken HW to me.
The MMC blocks could share a parent clock (that would need a fixed rate for it to be applied), assuming there is a separate gate/divider available per block. But there isn't'?
They do share a parent clock and have separate gates, and each MMC IP block has an internal divider for the bus frequency derived from that shared clock.
Quoted 7 lines
Quoted 7 lines
List of SoCs affected includes: JZ4725b, JZ4755, JZ4760 and JZ4760b. However, the MMC driver doesn't distinguish JZ4760 and JZ4770 which shall remain its behavior. For the JZ4755 is sufficient to use JZ4725b's binding. JZ4750 is outside of the patch.
The MMC core has its own clock divisor, rather coarse but suitable well, and it shall keep the role of tuning clock for the MMC host in that case.
Quoted 5 lines
The mmc core doesn't have a clock divisor, but it does control the bus clock frequency through the ->set_ios() host ops. It needs to do that, to be able to conform to the (e)MMC, SD and SDIO specifications. Can you please try to elaborate on the above, so I can better understand your point?

Quoted 7 lines

Yes, I don't really understand the patch, TBH.
The "clk_set_rate" call will only set the shared clock to the *maximum* clock frequency (host->mmc->f_max) which should be the exact same across all MMC IPs.
So it doesn't matter if it's set 3 times by 3 different instances of the IP, as long as they all request the same value.

Isn't the fact that 3 separate driver instances call clk_set_rate() on a shared clock kind of... problematic? Nor is it documented that all controllers need to use the same max-frequency.

Given it's a shared clock which can't realistically be controlled by any of the consumers, I don't think it's unreasonable to assign the clock frequency from the DT, but it needs to be backward-compatible with old DTs.

On another note, shouldn't the MMC mux parent be assigned from the DT? JZ4760 (and most other Ingenic SoCs) have multiple choices and I don't see the parent being assigned anywhere.

Regards,
Aidan

Quoted 62 lines

Besides, I know for a fact that the mainline driver works fine on the JZ4760(B) and JZ4725B.

Finally... even if it was correct, this change would break compatibility with old Device Tree files.

Cheers,
-Paul

Quoted 52 lines

Quoted 1 line

Signed-off-by Siarhei Volkau <[email protected]>

Kind regards
Uffe

Quoted 48 lines

---

  drivers/mmc/host/jz4740_mmc.c | 10 +++++++++-
  1 file changed, 9 insertions(+), 1 deletion(-)
 diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c
 index dc2db9c18..d390ff31d 100644
 --- a/drivers/mmc/host/jz4740_mmc.c
 +++ b/drivers/mmc/host/jz4740_mmc.c
 @@ -114,6 +114,7 @@ enum jz4740_mmc_version {
         JZ_MMC_JZ4740,
         JZ_MMC_JZ4725B,
         JZ_MMC_JZ4760,
 +       JZ_MMC_JZ4770,
         JZ_MMC_JZ4780,
         JZ_MMC_X1000,
  };
 @@ -887,7 +888,13 @@ static int jz4740_mmc_set_clock_rate(struct
jz4740_mmc_host *host, int rate)
         int real_rate;
         jz4740_mmc_clock_disable(host);
 -       clk_set_rate(host->clk, host->mmc->f_max);
 +
 +       /*
 +        * Changing rate on these SoCs affects other MMC units too.
 +        * Make sure the rate is configured properly by the CGU driver.
 +        */
 +       if (host->version != JZ_MMC_JZ4725B && host->version !=
JZ_MMC_JZ4760)
 +               clk_set_rate(host->clk, host->mmc->f_max);
         real_rate = clk_get_rate(host->clk);
 @@ -992,6 +999,7 @@ static const struct of_device_id jz4740_mmc_of_match[]
= {
         { .compatible = "ingenic,jz4740-mmc", .data = (void *)
JZ_MMC_JZ4740 },
         { .compatible = "ingenic,jz4725b-mmc", .data = (void
*)JZ_MMC_JZ4725B },
         { .compatible = "ingenic,jz4760-mmc", .data = (void *)
JZ_MMC_JZ4760 },
 +       { .compatible = "ingenic,jz4770-mmc", .data = (void *)
JZ_MMC_JZ4770 },
         { .compatible = "ingenic,jz4775-mmc", .data = (void *)
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,jz4780-mmc", .data = (void *)
JZ_MMC_JZ4780 },
         { .compatible = "ingenic,x1000-mmc", .data = (void *) JZ_MMC_X1000
},

--
2.36.1

Siarhei Volkau

author

2022-11-08 04:57 UTC #10 lore raw

PATCH 2/2 MIPS: ingenic: rs90: set MMC_MUX clock

In reply to Siarhei Volkau

Since the MMC driver can't change the common MMC_MUX clock anymore, the CGU shall configure that clock properly.

Signed-off-by Siarhei Volkau <[email protected]>

---
 arch/mips/boot/dts/ingenic/rs90.dts | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

arch/mips/boot/dts/ingenic/rs90.dts +3 -2

index e8df70dd4..d874abaa6 100644
@@ -295,8 +295,9 @@ partition@20000 {
 
 &cgu {
 	/* Use 32kHz oscillator as the parent of the RTC clock */
-	assigned-clocks = <&cgu JZ4725B_CLK_RTC>;
-	assigned-clock-parents = <&cgu JZ4725B_CLK_OSC32K>;
+	assigned-clocks = <&cgu JZ4725B_CLK_MMC_MUX>, <&cgu JZ4725B_CLK_RTC>;
+	assigned-clock-parents = <0>, <&cgu JZ4725B_CLK_OSC32K>;
+	assigned-clock-rates = <48000000>;
 };
 
 &tcu {

Signature · 1 line

2.36.1