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

[Paul Cercueil <paul@crapouillou.net>]

Hi Siarhei,

Le ven. 18 nov. 2022 à 12:51:54 +0300, Siarhei Volkau 
<lis8215@gmail.com> a écrit :
> пт, 18 нояб. 2022 г. в 12:27, Paul Cercueil 
> <paul@crapouillou.net>:
>> 
>>  Hi,
>> 
>>  (Ingenic SoCs maintainer here)
>> 
>>  Le ven. 18 nov. 2022 à 09:45:48 +0100, Ulf Hansson
>>  <ulf.hansson@linaro.org> a écrit :
>>  > On Tue, 8 Nov 2022 at 05:53, Siarhei Volkau <lis8215@gmail.com> 
>> 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.
> 
> 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

>> 
>>  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@gmail.com>
>>  >
>>  > 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
>>  >>
>> 
>>