diff mbox

[v2,2/9] mmc: tmio, sh_mobile_sdhi: Add support for variable input clock frequency

Message ID 20160415202803.GA3105@katana (mailing list archive)
State Accepted
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Wolfram Sang April 15, 2016, 8:28 p.m. UTC
> >   1. The SDHI/MMC clocks now run much slower than before. Perhaps this is
> >      intentional, and a consequence of finding the best way to drive the SD
> >      card at the target frequency?
> 
> I don't think is generally a problem.  Probably even saves a little
> power.

If you insert an SD card, frequencies should be back to normal. This
happens on Lager at least.

> 
> >   2. On r8a7740, the situation is worse: the HP ("High-speed Peripheral")
> >      clock is also scaled down from 99 MHz to 12.375 MHz.
> >      As the HP clock is the parent of lots of on-chip devices, this may affect
> >      performance for all of them.
> >
> > On r8a73a4, r8a7791, and sh73a0, the SDHI clocks are children of the pll1_div2
> > clocks, which are fixed.
> > On r8a7740, the SDHI and MMC clocks are children of the HP clock,
> > which is also scaled down, affecting all other siblings.
> [...]
> 
> That seems like a bug in the clock driver.  If it doesn't have
> independent dividers for each clock client then it shouldn't allow any
> client to change the frequency.

I tend to agree.

However, I just found out that we don't check the result of
clk_set_rate(). Probably something like this is missing?

Comments

Wolfram Sang April 15, 2016, 8:41 p.m. UTC | #1
> > That seems like a bug in the clock driver.  If it doesn't have
> > independent dividers for each clock client then it shouldn't allow any
> > client to change the frequency.

So, we need MSTP clocks which do not use CLK_SET_RATE_PARENT on r8a7740.
Probably the best way is to use the "renesas,r8a7740-mstp-clocks"
compatible entry to determine the difference? Geert?
Wolfram Sang April 17, 2016, 12:49 p.m. UTC | #2
On Fri, Apr 15, 2016 at 10:41:57PM +0200, Wolfram Sang wrote:
> > > That seems like a bug in the clock driver.  If it doesn't have
> > > independent dividers for each clock client then it shouldn't allow any
> > > client to change the frequency.
> 
> So, we need MSTP clocks which do not use CLK_SET_RATE_PARENT on r8a7740.
> Probably the best way is to use the "renesas,r8a7740-mstp-clocks"
> compatible entry to determine the difference? Geert?

Well, I like the idea of a virtual sd clock even better. I sent a series
doing that a minute ago to the renesas-soc list.
Geert Uytterhoeven April 18, 2016, 7:33 a.m. UTC | #3
On Sun, Apr 17, 2016 at 2:49 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Fri, Apr 15, 2016 at 10:41:57PM +0200, Wolfram Sang wrote:
>> > > That seems like a bug in the clock driver.  If it doesn't have
>> > > independent dividers for each clock client then it shouldn't allow any
>> > > client to change the frequency.
>>
>> So, we need MSTP clocks which do not use CLK_SET_RATE_PARENT on r8a7740.
>> Probably the best way is to use the "renesas,r8a7740-mstp-clocks"
>> compatible entry to determine the difference? Geert?
>
> Well, I like the idea of a virtual sd clock even better. I sent a series
> doing that a minute ago to the renesas-soc list.

While I don't doubt the "virtual sd" clock will work, this doesn't match the
hardware, so I have to object against it.

Hence I think it should be handled in the driver. This is indeed more difficult
to do with clk-mstp.c than with renesas-cpg-mssr.c...

BTW, this behavior seems to be introduced by
commit e44df332f30bf3040c60c1ed6674d1431fdb48b9
Author: Ben Dooks <ben.dooks@codethink.co.uk>
Date:   Mon Mar 31 18:47:27 2014 +0100

    clk: shmobile: fix setting paretn clock rate

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Geert Uytterhoeven April 26, 2016, 10:18 a.m. UTC | #4
Hi Wolfram,

On Fri, Apr 15, 2016 at 10:41 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> > That seems like a bug in the clock driver.  If it doesn't have
>> > independent dividers for each clock client then it shouldn't allow any
>> > client to change the frequency.
>
> So, we need MSTP clocks which do not use CLK_SET_RATE_PARENT on r8a7740.
> Probably the best way is to use the "renesas,r8a7740-mstp-clocks"
> compatible entry to determine the difference? Geert?

To fix this regression in the short term, can we just enable this feature in
the sh_mobile_sdhi driver on SoCs where we know it's safe to change the clock,
i.e. on R-Car Gen2 and H3?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Wolfram Sang April 26, 2016, 5 p.m. UTC | #5
> To fix this regression in the short term, can we just enable this feature in
> the sh_mobile_sdhi driver on SoCs where we know it's safe to change the clock,
> i.e. on R-Car Gen2 and H3?

I'll try a workaround next week.
diff mbox

Patch

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 5923ce7e0fccb3..e51d7b01d39a3b 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -167,7 +167,7 @@  static unsigned int sh_mobile_sdhi_clk_update(struct tmio_mmc_host *host,
 {
 	struct sh_mobile_sdhi *priv = host_to_priv(host);
 	unsigned int freq, best_freq, diff_min, diff;
-	int i;
+	int i, ret;
 
 	diff_min = ~0;
 	best_freq = 0;
@@ -195,9 +195,9 @@  static unsigned int sh_mobile_sdhi_clk_update(struct tmio_mmc_host *host,
 		}
 	}
 
-	clk_set_rate(priv->clk, best_freq);
+	ret = clk_set_rate(priv->clk, best_freq);
 
-	return best_freq;
+	return ret == 0 ? best_freq : clk_get_rate(priv->clk);
 }
 
 static void sh_mobile_sdhi_clk_disable(struct tmio_mmc_host *host)