Hi Sam,
On 5/30/24 2:06 AM, Sam Protsenko wrote:
On Thu, May 23, 2024 at 9:36 AM Quentin Schulz <quentin.sch...@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?
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?
+ 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 :) ?
Thanks,
Quentin
