| Message ID | 20240522233135.26835-7-semen.protsenko@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Jaehoon Chung |
| Headers | show |
| Series | mmc: dw_mmc: Enable eMMC on E850-96 board | expand |
Hi Sam, 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. > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > drivers/mmc/dw_mmc.c | 60 ++++++++++++++++++++++++-------------------- > 1 file changed, 33 insertions(+), 27 deletions(-) > > diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c > index cbfb8d3b8683..40b9b034f793 100644 > --- a/drivers/mmc/dw_mmc.c > +++ b/drivers/mmc/dw_mmc.c > @@ -412,11 +412,33 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, > return ret; > } > > -static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) > +static int dwmci_control_clken(struct dwmci_host *host, bool on) > { > - u32 div, status; > + const u32 val = on ? DWMCI_CLKEN_ENABLE | DWMCI_CLKEN_LOW_PWR : 0; > + const u32 cmd_only_clk = DWMCI_CMD_PRV_DAT_WAIT | DWMCI_CMD_UPD_CLK; > int timeout = 10000; > + u32 status; > + > + dwmci_writel(host, DWMCI_CLKENA, val); > + > + /* Inform CIU */ > + dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_START | cmd_only_clk); > + do { > + status = dwmci_readl(host, DWMCI_CMD); > + if (timeout-- < 0) { > + debug("%s: Timeout!\n", __func__); > + return -ETIMEDOUT; > + } > + } while (status & DWMCI_CMD_START); > + > + return 0; > +} > + > +static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) > +{ > + u32 div; > unsigned long sclk; > + int ret; > > if ((freq == host->clock) || (freq == 0)) > return 0; > @@ -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? > + 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 :/ > - dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_PRV_DAT_WAIT | > - DWMCI_CMD_UPD_CLK | DWMCI_CMD_START); > > - do { > - status = dwmci_readl(host, DWMCI_CMD); > - if (timeout-- < 0) { > - debug("%s: Timeout!\n", __func__); > - return -ETIMEDOUT; > - } > - } while (status & DWMCI_CMD_START); > - > - dwmci_writel(host, DWMCI_CLKENA, DWMCI_CLKEN_ENABLE | > - DWMCI_CLKEN_LOW_PWR); > - > - dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_PRV_DAT_WAIT | > - DWMCI_CMD_UPD_CLK | DWMCI_CMD_START); > - > - timeout = 10000; > - do { > - status = dwmci_readl(host, DWMCI_CMD); > - if (timeout-- < 0) { > - debug("%s: Timeout!\n", __func__); > - return -ETIMEDOUT; > - } > - } while (status & DWMCI_CMD_START); > + /* Enable clock */ > + ret = dwmci_control_clken(host, true); > + if (ret) > + return ret; > > host->clock = freq; > Cheers, Quentin
On Thu, May 23, 2024 at 9:36 AM Quentin Schulz <quentin.schulz@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. 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). > > + 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. [snip] Thanks!
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@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
On Mon, Jun 3, 2024 at 3:52 AM Quentin Schulz <quentin.schulz@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@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
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index cbfb8d3b8683..40b9b034f793 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -412,11 +412,33 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, return ret; } -static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) +static int dwmci_control_clken(struct dwmci_host *host, bool on) { - u32 div, status; + const u32 val = on ? DWMCI_CLKEN_ENABLE | DWMCI_CLKEN_LOW_PWR : 0; + const u32 cmd_only_clk = DWMCI_CMD_PRV_DAT_WAIT | DWMCI_CMD_UPD_CLK; int timeout = 10000; + u32 status; + + dwmci_writel(host, DWMCI_CLKENA, val); + + /* Inform CIU */ + dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_START | cmd_only_clk); + do { + status = dwmci_readl(host, DWMCI_CMD); + if (timeout-- < 0) { + debug("%s: Timeout!\n", __func__); + return -ETIMEDOUT; + } + } while (status & DWMCI_CMD_START); + + return 0; +} + +static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) +{ + u32 div; unsigned long sclk; + int ret; if ((freq == host->clock) || (freq == 0)) return 0; @@ -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); + if (ret) + return ret; + /* Set clock to desired speed */ dwmci_writel(host, DWMCI_CLKDIV, div); - dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_PRV_DAT_WAIT | - DWMCI_CMD_UPD_CLK | DWMCI_CMD_START); - do { - status = dwmci_readl(host, DWMCI_CMD); - if (timeout-- < 0) { - debug("%s: Timeout!\n", __func__); - return -ETIMEDOUT; - } - } while (status & DWMCI_CMD_START); - - dwmci_writel(host, DWMCI_CLKENA, DWMCI_CLKEN_ENABLE | - DWMCI_CLKEN_LOW_PWR); - - dwmci_writel(host, DWMCI_CMD, DWMCI_CMD_PRV_DAT_WAIT | - DWMCI_CMD_UPD_CLK | DWMCI_CMD_START); - - timeout = 10000; - do { - status = dwmci_readl(host, DWMCI_CMD); - if (timeout-- < 0) { - debug("%s: Timeout!\n", __func__); - return -ETIMEDOUT; - } - } while (status & DWMCI_CMD_START); + /* Enable clock */ + ret = dwmci_control_clken(host, true); + if (ret) + return ret; host->clock = freq;
Extract clock control code into a separate routine to avoid code duplication in dwmci_setup_bus(). No functional change. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/mmc/dw_mmc.c | 60 ++++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 27 deletions(-)