| Message ID | 20181113234050.7653-3-marek.vasut+renesas@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Peng Fan |
| Headers | show |
| Series | [U-Boot,V3,1/6] mmc: tmio: Switch to clock framework | expand |
On Wed, Nov 14, 2018 at 8:44 AM Marek Vasut <marek.vasut@gmail.com> wrote: > > The TMIO core has a feature where it can automatically disable clock output > when the bus is not in use. While this is useful, it also interferes with > switching the bus to 1.8V and other background tasks of the SD/MMC cards, > which require clock to be enabled. > > This patch respects the mmc->clk_disable and only disables the clock when > the MMC core requests it. Otherwise the clock are continuously generated > on the bus. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > drivers/mmc/tmio-common.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c > index 424b60ce52..fad2816ca5 100644 > --- a/drivers/mmc/tmio-common.c > +++ b/drivers/mmc/tmio-common.c > @@ -612,10 +612,16 @@ static void tmio_sd_set_clk_rate(struct tmio_sd_priv *priv, > tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); > > tmp &= ~TMIO_SD_CLKCTL_DIV_MASK; > - tmp |= val | TMIO_SD_CLKCTL_OFFEN; > + tmp |= val; > tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); > > - tmp |= TMIO_SD_CLKCTL_SCLKEN; > + if (!mmc->clk_disable) { > + tmp &= ~TMIO_SD_CLKCTL_OFFEN; > + tmp |= TMIO_SD_CLKCTL_SCLKEN; > + } else { > + tmp |= TMIO_SD_CLKCTL_OFFEN; > + tmp &= ~TMIO_SD_CLKCTL_SCLKEN; > + } > tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); > > udelay(1000); What is the point of setting TMIO_SD_CLKCTL_OFFEN when TMIO_SD_CLKCTL_SCLKEN is unset? Why isn't the code like this? if (!mmc->clk_disable) tmp |= TMIO_SD_CLKCTL_SCLKEN; else tmp &= ~TMIO_SD_CLKCTL_SCLKEN; /* Never use OFFEN because it interferes with 1.8 switching */ tmp &= ~TMIO_SD_CLKCTL_OFFEN; tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); One more thing, you wrote if (!mmc->clk_disable) ... else ... in this patch. Then, soon you will rewrite it to if (mmc->clk_disable) ... else ... in another patch: http://patchwork.ozlabs.org/patch/998542/ Noise changes.
On 11/27/2018 04:43 AM, Masahiro Yamada wrote: > On Wed, Nov 14, 2018 at 8:44 AM Marek Vasut <marek.vasut@gmail.com> wrote: >> >> The TMIO core has a feature where it can automatically disable clock output >> when the bus is not in use. While this is useful, it also interferes with >> switching the bus to 1.8V and other background tasks of the SD/MMC cards, >> which require clock to be enabled. >> >> This patch respects the mmc->clk_disable and only disables the clock when >> the MMC core requests it. Otherwise the clock are continuously generated >> on the bus. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- >> drivers/mmc/tmio-common.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c >> index 424b60ce52..fad2816ca5 100644 >> --- a/drivers/mmc/tmio-common.c >> +++ b/drivers/mmc/tmio-common.c >> @@ -612,10 +612,16 @@ static void tmio_sd_set_clk_rate(struct tmio_sd_priv *priv, >> tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); >> >> tmp &= ~TMIO_SD_CLKCTL_DIV_MASK; >> - tmp |= val | TMIO_SD_CLKCTL_OFFEN; >> + tmp |= val; >> tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); >> >> - tmp |= TMIO_SD_CLKCTL_SCLKEN; >> + if (!mmc->clk_disable) { >> + tmp &= ~TMIO_SD_CLKCTL_OFFEN; >> + tmp |= TMIO_SD_CLKCTL_SCLKEN; >> + } else { >> + tmp |= TMIO_SD_CLKCTL_OFFEN; >> + tmp &= ~TMIO_SD_CLKCTL_SCLKEN; >> + } >> tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); >> >> udelay(1000); > > > > What is the point of setting TMIO_SD_CLKCTL_OFFEN > when TMIO_SD_CLKCTL_SCLKEN is unset? > > > Why isn't the code like this? > > > if (!mmc->clk_disable) > tmp |= TMIO_SD_CLKCTL_SCLKEN; > else > tmp &= ~TMIO_SD_CLKCTL_SCLKEN; > > /* Never use OFFEN because it interferes with 1.8 switching */ > tmp &= ~TMIO_SD_CLKCTL_OFFEN; > > tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); > I think this could be done, but I'd have to re-run the entire bulk of tests on all boards to see if nothing broke. I'd prefer to get this in (if it doesn't break your system) and then add a patch on top. > One more thing, you wrote > > if (!mmc->clk_disable) > ... > else > ... > > in this patch. > > > Then, soon you will rewrite it to > > if (mmc->clk_disable) > ... > else > ... > > in another patch: > > http://patchwork.ozlabs.org/patch/998542/ I know, I added the other one afterward, since I discovered another card which had clock problems.
diff --git a/drivers/mmc/tmio-common.c b/drivers/mmc/tmio-common.c index 424b60ce52..fad2816ca5 100644 --- a/drivers/mmc/tmio-common.c +++ b/drivers/mmc/tmio-common.c @@ -612,10 +612,16 @@ static void tmio_sd_set_clk_rate(struct tmio_sd_priv *priv, tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); tmp &= ~TMIO_SD_CLKCTL_DIV_MASK; - tmp |= val | TMIO_SD_CLKCTL_OFFEN; + tmp |= val; tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); - tmp |= TMIO_SD_CLKCTL_SCLKEN; + if (!mmc->clk_disable) { + tmp &= ~TMIO_SD_CLKCTL_OFFEN; + tmp |= TMIO_SD_CLKCTL_SCLKEN; + } else { + tmp |= TMIO_SD_CLKCTL_OFFEN; + tmp &= ~TMIO_SD_CLKCTL_SCLKEN; + } tmio_sd_writel(priv, tmp, TMIO_SD_CLKCTL); udelay(1000);
The TMIO core has a feature where it can automatically disable clock output when the bus is not in use. While this is useful, it also interferes with switching the bus to 1.8V and other background tasks of the SD/MMC cards, which require clock to be enabled. This patch respects the mmc->clk_disable and only disables the clock when the MMC core requests it. Otherwise the clock are continuously generated on the bus. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Masahiro Yamada <yamada.masahiro@socionext.com> --- drivers/mmc/tmio-common.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)