diff mbox series

[4.4.y-cip,15/83] mmc: tmio: stop clock when 0Hz is requested

Message ID 1573115572-13513-16-git-send-email-biju.das@bp.renesas.com (mailing list archive)
State Accepted
Headers show
Series Add RZ/G1C SD/eMMC support | expand

Commit Message

Biju Das Nov. 7, 2019, 8:31 a.m. UTC 
From: Wolfram Sang <wsa+renesas@sang-engineering.com>

commit 148634d24d4a7dc82a49efcf1a215e1d0695f62c upstream.

Setting frequency to 0 is not enough, the clock explicitly has to be
disabled. Otherwise voltage switching (which needs SDCLK to be quiet)
fails for various cards.

Because we now do the 'new_clock == 0' check right at the beginning,
the indentation level of the rest of the code can be decreased a little.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 drivers/mmc/host/tmio_mmc_pio.c | 50 +++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

Comments

Pavel Machek Nov. 8, 2019, 9:15 a.m. UTC | #1 
Hi!

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> commit 148634d24d4a7dc82a49efcf1a215e1d0695f62c upstream.
> 
> Setting frequency to 0 is not enough, the clock explicitly has to be
> disabled. Otherwise voltage switching (which needs SDCLK to be quiet)
> fails for various cards.
> 
> Because we now do the 'new_clock == 0' check right at the beginning,
> the indentation level of the rest of the code can be decreased a
> little.

>  {
>  	u32 clk = 0, clock;
>

clk = 0 initialization is never used.

> +	for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)
> +		clock <<= 1;

This is black magic. Where does 0x80000080 come from? Would it be
possible to get some comment/explanation?

> +	/* 1/1 clock is option */
> +	if ((host->pdata->flags & TMIO_MMC_CLK_ACTUAL) && ((clk >> 22) & 0x1))
> +		clk |= 0xff;
>  
>  	if (host->set_clk_div)
>  		host->set_clk_div(host->pdev, (clk >> 22) & 1);

What does bit 22 in clk mean? Should it go to temporary variable with
explanation? (Also it is & 1 in one operation and & 0x1 in other
operation).

It seems that low bits of clk variable are used as an actual clock
divider, with high bit doing something else. That is quite
confusing...

Best regards,
								Pavel
Biju Das Nov. 8, 2019, 12:50 p.m. UTC | #2 
Hi Pavel,

>
> Subject: Re: [PATCH 4.4.y-cip 15/83] mmc: tmio: stop clock when 0Hz is
> requested
> 
> Hi!
> 
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >
> > commit 148634d24d4a7dc82a49efcf1a215e1d0695f62c upstream.
> >
> > Setting frequency to 0 is not enough, the clock explicitly has to be
> > disabled. Otherwise voltage switching (which needs SDCLK to be quiet)
> > fails for various cards.
> >
> > Because we now do the 'new_clock == 0' check right at the beginning,
> > the indentation level of the rest of the code can be decreased a
> > little.
> 
> >  {
> >  	u32 clk = 0, clock;
> >
> 
> clk = 0 initialization is never used.
> 
> > +	for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)
> > +		clock <<= 1;
> 
> This is black magic. Where does 0x80000080 come from? Would it be possible
> to get some comment/explanation?
> 
> > +	/* 1/1 clock is option */
> > +	if ((host->pdata->flags & TMIO_MMC_CLK_ACTUAL) && ((clk >> 22)
> & 0x1))
> > +		clk |= 0xff;
> >
> >  	if (host->set_clk_div)
> >  		host->set_clk_div(host->pdev, (clk >> 22) & 1);
> 
> What does bit 22 in clk mean? Should it go to temporary variable with
> explanation? (Also it is & 1 in one operation and & 0x1 in other operation).
> 
> It seems that low bits of clk variable are used as an actual clock divider, with
> high bit doing something else. That is quite confusing...

Yes I agree this could have been done with Macro. Not sure,  may be because of HALA [SD Host/Ancillary Product License Agreement],
 it is defined as magic number at that time.

Regards,
Biju
diff mbox series

Patch

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 99139b8..6c09b1d 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -166,25 +166,39 @@  static void tmio_mmc_clk_start(struct tmio_mmc_host *host)
 	}
 }
 
+static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
+{
+	if (host->pdata->flags & TMIO_MMC_HAVE_HIGH_REG) {
+		sd_ctrl_write16(host, CTL_CLK_AND_WAIT_CTL, 0x0000);
+		msleep(10);
+	}
+
+	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
+		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
+	msleep(host->pdata->flags & TMIO_MMC_FAST_CLK_CHG ? 5 : 10);
+}
+
 static void tmio_mmc_set_clock(struct tmio_mmc_host *host,
 				unsigned int new_clock)
 {
 	u32 clk = 0, clock;
 
-	if (new_clock) {
-		if (host->clk_update)
-			clock = host->clk_update(host, new_clock) / 512;
-		else
-			clock = host->mmc->f_min;
+	if (new_clock == 0) {
+		tmio_mmc_clk_stop(host);
+		return;
+	}
 
-		for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)
-			clock <<= 1;
+	if (host->clk_update)
+		clock = host->clk_update(host, new_clock) / 512;
+	else
+		clock = host->mmc->f_min;
 
-		/* 1/1 clock is option */
-		if ((host->pdata->flags & TMIO_MMC_CLK_ACTUAL) &&
-		   ((clk >> 22) & 0x1))
-			clk |= 0xff;
-	}
+	for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)
+		clock <<= 1;
+
+	/* 1/1 clock is option */
+	if ((host->pdata->flags & TMIO_MMC_CLK_ACTUAL) && ((clk >> 22) & 0x1))
+		clk |= 0xff;
 
 	if (host->set_clk_div)
 		host->set_clk_div(host->pdev, (clk >> 22) & 1);
@@ -198,18 +212,6 @@  static void tmio_mmc_set_clock(struct tmio_mmc_host *host,
 	tmio_mmc_clk_start(host);
 }
 
-static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)
-{
-	if (host->pdata->flags & TMIO_MMC_HAVE_HIGH_REG) {
-		sd_ctrl_write16(host, CTL_CLK_AND_WAIT_CTL, 0x0000);
-		msleep(10);
-	}
-
-	sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &
-		sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));
-	msleep(host->pdata->flags & TMIO_MMC_FAST_CLK_CHG ? 5 : 10);
-}
-
 static void tmio_mmc_reset(struct tmio_mmc_host *host)
 {
 	/* FIXME - should we set stop clock reg here */