[PATCH 06/42] mmc: dw_mmc: Extract clock on/off code into a separate routine

[Sam Protsenko]

On Mon, Jun 3, 2024 at 3:52 AM Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Sam,
>
> On 5/30/24 2:06 AM, Sam Protsenko wrote:
> > On Thu, May 23, 2024 at 9:36 AM Quentin Schulz <quentin.schulz at cherry.de> wrote:
> >>
> >> Hi Sam,
> >>
> >
> > Hi Quentin,
> >
> > Thanks for reviewing this series! My answers are below (inline).
> >
> >> On 5/23/24 1:30 AM, Sam Protsenko wrote:
> >>> Extract clock control code into a separate routine to avoid code
> >>> duplication in dwmci_setup_bus().
> >>>
> >>> No functional change.
> >>>
> >>
> >> There are some differences though.
> >>
> >
> > With everything discussed below, I guess it still can be argued that
> > this patch doesn't change the functionality of the driver? Depends on
> > definition though. Please let me know if you don't like this bit and
> > I'll remove it in v2.
> >
> > [snip]
> >
> >>> @@ -439,35 +461,19 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq)
> >>>        else
> >>>                div = DIV_ROUND_UP(sclk, 2 * freq);
> >>>
> >>> -     dwmci_writel(host, DWMCI_CLKENA, 0);
> >>> +     /* Disable clock */
> >>>        dwmci_writel(host, DWMCI_CLKSRC, 0);
> >>> +     ret = dwmci_control_clken(host, false);
> >>
> >> Here, CLKENA and CLKSRC are swapped. The kernel still calls CLKENA
> >> before CLKSRC, is this really safe? (I have no documentation so cannot
> >> tell). E.g., we could have CLKSRC register be a way to select the clock
> >> parent/source, 0 could be 24MHz crystal for example, so switching to
> >> this new parent before disabling the clock output would likely result in
> >> glitches?
> >>
> >
> > Yes, you are right. In fact, Exynos850 TRM specifically mentions this.
> > Here is how TRM describes the whole procedure for "CIU Update":
> >
> > 8<------------------------------------------------------------->8
> > To prevent a clock glitch, ensure that software stops the clock when
> > it updates the clock divider and clock source registers.
> >
> > Software uses the following sequence to update the clock divider settings:
> > 1. Disables the clock.
> > 2. Updates the clock divider and/or the clock source registers.
> > 3, Enables the clock.
> >
> > Code Sample to Update CIU:
> >
> > // Disable clock
> > while (rSTATUS & (1 << 9));
> > rCLKENA = 0x0;
> > rCMD = 0x80202000; // inform CIU
> > while (rCMD & 0x80000000);
> >
> > // Update divider and source
> > rCLKDIV = clock_divider;
> > rCLKSRC = clock_source;
> >
> > // Enable clock
> > while (rSTATUS & (1 << 9));
> > rCLKENA= 0x1;
> > rCMD = 0x80202000; // inform CIU
> > while (rCMD & 0x80000000);
> > 8<------------------------------------------------------------->8
> >
> > It's an overlook on my part. And although CLKSRC reset value is 0x0
> > and it's only being written with 0x0 in the driver, it still has to be
> > fixed. I'll move CLKSRC modification below CLKDIV to make it look like
> > TRM suggests.
> >
>
> Did you mean "move CLKENA before CLKSRC"? Because if the clock is
> disabled, I don't think the order in which the parent clock and its
> divider are set matters?
>

Yes, in v2 I'll just pull CLKSRC modification down, so it's executed
after disabling the clock, as recommended in TRM. Something like this:

8<------------------------------------------------------------->8
        /* Disable clock */
-       dwmci_writel(host, DWMCI_CLKSRC, 0);
        ret = dwmci_control_clken(host, false);
        if (ret)
                return ret;

        /* Set clock to desired speed */
        dwmci_writel(host, DWMCI_CLKDIV, div);
+       dwmci_writel(host, DWMCI_CLKSRC, 0);

        /* Enable clock */
        ret = dwmci_control_clken(host, true);
8<------------------------------------------------------------->8

> > Linux kernel implementation also doesn't follow the sequence
> > recommended in TRM as you can see, but it works fine because CLKSRC is
> > always written to 0 there (and it's also 0 at reset time).
> >
>
> OK, so wrong logic but it works because we essentially never change
> CLKSRC so it's virtually a no-op and thus there are no change to the
> parent clock when the clock is enabled, resulting in no glitch. Not sure
> we should ignore the improper logic though? What are you suggesting we
> do here?
>

Yes, exactly. As for the possible actions, I'd say it'd nice to fix it
in kernel too, though I kinda see it as a very minor issue right now.
Not sure if I can find the time to do that soon though.

On a philosophical note: I'm sure a lot of drivers don't follow the
sequences intended by the hardware exactly, but they still work
because IP core design is permissive enough. Over the years I found
peace with that somehow :)

> >>> +     if (ret)
> >>> +             return ret;
> >>>
> >>> +     /* Set clock to desired speed */
> >>>        dwmci_writel(host, DWMCI_CLKDIV, div);
> >>
> >> Same here, CLKDIV is set now after the CIU is informed, is this an
> >> issue? We may want to set the clock speed before we enable the clock
> >> again. Right now it's setting the desired speed while disabled, inform
> >> the CIU, enable the clock, inform the CIU. This now does, disable the
> >> clock, inform the CIU, set the desired speed, enable the clock, inform
> >> the CIU. We may need to wait for the clock to stabilize before enabling
> >> it? Again, just making guesses, no documentation for me here :/
> >>
> >
> > In this case, my implementation is following TRM (as described above).
> > After CLKDIV modification my code informs CIU again, inside of the
> > dwmci_control_clken() call.
> >
> > Just to be thorough, here is what TRM says about DWMCI_CMD_UPD_CLK (bit 21):
> >
> > 8<------------------------------------------------------------->8
> > ...Following are the register values transferred into the card clock domain:
> > * CLKDIV
> > * CLKSRC
> > * CLKENA
> > 8<------------------------------------------------------------->8
> >
> > So you are right that each time this bit is set all 3 values are being
> > transferred to CIU. But because that bit is set two times (before and
> > after updating CLKDIV/CLKSRC) -- it's ok. And also this way is
> > recommended in TRM, as stated above.
> >
>
> Hopefully there aren't other implementations than Exynos 850's that need
> to do something else :)
>
> Aside from the improper logic, looks good to me, can you please add this
> information to the commit log so it's not lost in the mailing list :) ?
>

Yes, will do in v2 (going to submit it today). And will also add some
related comment in the code. If you don't mind, please add your
"Reviewed-by" tags where possible in v2, to keep the ball rolling :)

Thanks for reviewing!

> Thanks,
> Quentin