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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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







[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux