Hi,
(Ingenic SoCs maintainer here)
Le ven. 18 nov. 2022 à 09:45:48 +0100, Ulf Hansson
<ulf.hansson@xxxxxxxxxx> a écrit :
On Tue, 8 Nov 2022 at 05:53, Siarhei Volkau <lis8215@xxxxxxxxx> wrote:
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.
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?
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
Signed-off-by: Siarhei Volkau <lis8215@xxxxxxxxx>
Kind regards
Uffe
---
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
