[PATCH] mmc: sh_mmcif: mmc->f_max heuristic

[PATCH] mmc: sh_mmcif: mmc->f_max heuristic

[Simon Horman]

In 930f152cc9998388031af577843baae572ac8ab6 ("mmc: sh_mmcif: mmc->f_max
should be half of the bus clock") I changed the setting of mmc->f_max from
the bus clock to half the bus clock based on the manual for the sh7372 SoC.

Inspection of sh_mmcif_clock_control() reveals that it relies on
mmc->f_max being set to the bus speed in order to enable the supplementary
clock, a feature that does not exist on the sh7372.

Armed with this information implement the following heuristic for setting
mmc->f_max:

* Use bus clock if the supplementary clock feature is present
  - Assumed to work on the sh7757lcr board, the only board present
    in the tree which has the feature.
* Use half the bus clock otherwise
  - As per the documentation for the sh7372 SoC.

Reported-by: Magnus Damm <damm@opensource.se>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Regression information:

It seems likely that this resolves a regression for the sh7757lcr which was
introduced by 930f152cc9998388031af577843baae572ac8ab6 ("mmc: sh_mmcif:
mmc->f_max should be half of the bus clock"). However, although I have the
hardware in question I am unsure how to exercise the code to test if there
is a problem or not.

930f152cc9998388031af577843baae572ac8ab6 was added between the 3.3 and 3.4.

Dependency information:

This patch is based on Guennadi Liakhovetski's recent series
"[PATCH 0/5 v4] mmc: sh_mmcif: clock and power updates". It should
be trivial to rebase it on 3.5-rc3.
---
 drivers/mmc/host/sh_mmcif.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index f0f518b..dfcfe83 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -919,10 +919,11 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq)
 static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
 {
 	int ret = clk_enable(host->hclk);
+	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
 
 	if (!ret) {
 		host->clk = clk_get_rate(host->hclk);
-		host->mmc->f_max = host->clk / 2;
+		host->mmc->f_max = p->sup_pclk ? host->clk : host->clk / 2;
 		host->mmc->f_min = host->clk / 512;
 	}
 
-- 
1.7.10.2.484.gcd07cc5

Re: [PATCH] mmc: sh_mmcif: mmc->f_max heuristic

[Guennadi Liakhovetski]

Hi Simon

Thanks for addressing this issue.

On Wed, 20 Jun 2012, Simon Horman wrote:

> In 930f152cc9998388031af577843baae572ac8ab6 ("mmc: sh_mmcif: mmc->f_max
> should be half of the bus clock") I changed the setting of mmc->f_max from
> the bus clock to half the bus clock based on the manual for the sh7372 SoC.
> 
> Inspection of sh_mmcif_clock_control() reveals that it relies on
> mmc->f_max being set to the bus speed in order to enable the supplementary
> clock, a feature that does not exist on the sh7372.
> 
> Armed with this information implement the following heuristic for setting
> mmc->f_max:
> 
> * Use bus clock if the supplementary clock feature is present
>   - Assumed to work on the sh7757lcr board, the only board present
>     in the tree which has the feature.

To be able to better understand this change: do we have access to the 
sh7757 documentation and does it actually explain how the CLK_SUP_PCLK bit 
in MMCIF_CE_CLK_CTRL functions? Does it actually set the MMC bus clock to 
be equal to the host clock?

Thanks
Guennadi

> * Use half the bus clock otherwise
>   - As per the documentation for the sh7372 SoC.
> 
> Reported-by: Magnus Damm <damm@opensource.se>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> ---
> 
> Regression information:
> 
> It seems likely that this resolves a regression for the sh7757lcr which was
> introduced by 930f152cc9998388031af577843baae572ac8ab6 ("mmc: sh_mmcif:
> mmc->f_max should be half of the bus clock"). However, although I have the
> hardware in question I am unsure how to exercise the code to test if there
> is a problem or not.
> 
> 930f152cc9998388031af577843baae572ac8ab6 was added between the 3.3 and 3.4.
> 
> Dependency information:
> 
> This patch is based on Guennadi Liakhovetski's recent series
> "[PATCH 0/5 v4] mmc: sh_mmcif: clock and power updates". It should
> be trivial to rebase it on 3.5-rc3.
> ---
>  drivers/mmc/host/sh_mmcif.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index f0f518b..dfcfe83 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -919,10 +919,11 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
>  {
>  	int ret = clk_enable(host->hclk);
> +	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
>  
>  	if (!ret) {
>  		host->clk = clk_get_rate(host->hclk);
> -		host->mmc->f_max = host->clk / 2;
> +		host->mmc->f_max = p->sup_pclk ? host->clk : host->clk / 2;
>  		host->mmc->f_min = host->clk / 512;
>  	}
>  
> -- 
> 1.7.10.2.484.gcd07cc5
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] mmc: sh_mmcif: mmc->f_max heuristic

[Simon Horman]

On Wed, Jun 20, 2012 at 08:49:38AM +0200, Guennadi Liakhovetski wrote:
> Hi Simon
> 
> Thanks for addressing this issue.
> 
> On Wed, 20 Jun 2012, Simon Horman wrote:
> 
> > In 930f152cc9998388031af577843baae572ac8ab6 ("mmc: sh_mmcif: mmc->f_max
> > should be half of the bus clock") I changed the setting of mmc->f_max from
> > the bus clock to half the bus clock based on the manual for the sh7372 SoC.
> > 
> > Inspection of sh_mmcif_clock_control() reveals that it relies on
> > mmc->f_max being set to the bus speed in order to enable the supplementary
> > clock, a feature that does not exist on the sh7372.
> > 
> > Armed with this information implement the following heuristic for setting
> > mmc->f_max:
> > 
> > * Use bus clock if the supplementary clock feature is present
> >   - Assumed to work on the sh7757lcr board, the only board present
> >     in the tree which has the feature.
> 
> To be able to better understand this change: do we have access to the 
> sh7757 documentation and does it actually explain how the CLK_SUP_PCLK bit 
> in MMCIF_CE_CLK_CTRL functions? Does it actually set the MMC bus clock to 
> be equal to the host clock?

My understanding is that neither Magnus nor I have access to the
documentation for the sh7757. Thus we could only infer things from
examining the source code. I would be very happy if someone with access to
the documentation could shed some further light on this. In particular,
I would be happy if this heuristic was shown not to be necessary. But
in lieu of access to the documentation I think this patch is reasonable.

RE: [PATCH] mmc: sh_mmcif: mmc->f_max heuristic

[Brandt, Chris]

> I would be very happy if someone with access to
> the documentation could shed some further light on this.

According to the SH7757 manual, setting the CLKDIV[3:0] in the CE_CLK_CTRL register 0xF selects Pck as the bus clock frequency. So, unlike other devices where Pck/2 (CLKDIV[3:0]=0) is the max bus speed, the SH7757 can run at Pck when CLKDIV=0xF.

- Chris


-----Original Message-----
From: linux-sh-owner@vger.kernel.org [mailto:linux-sh-owner@vger.kernel.org] On Behalf Of Simon Horman
Sent: Wednesday, June 20, 2012 3:02 AM
To: Guennadi Liakhovetski
Cc: linux-mmc@vger.kernel.org; linux-sh@vger.kernel.org; Chris Ball; Magnus Damm
Subject: Re: [PATCH] mmc: sh_mmcif: mmc->f_max heuristic

On Wed, Jun 20, 2012 at 08:49:38AM +0200, Guennadi Liakhovetski wrote:
> Hi Simon
> 
> Thanks for addressing this issue.
> 
> On Wed, 20 Jun 2012, Simon Horman wrote:
> 
> > In 930f152cc9998388031af577843baae572ac8ab6 ("mmc: sh_mmcif: mmc->f_max
> > should be half of the bus clock") I changed the setting of mmc->f_max from
> > the bus clock to half the bus clock based on the manual for the sh7372 SoC.
> > 
> > Inspection of sh_mmcif_clock_control() reveals that it relies on
> > mmc->f_max being set to the bus speed in order to enable the supplementary
> > clock, a feature that does not exist on the sh7372.
> > 
> > Armed with this information implement the following heuristic for setting
> > mmc->f_max:
> > 
> > * Use bus clock if the supplementary clock feature is present
> >   - Assumed to work on the sh7757lcr board, the only board present
> >     in the tree which has the feature.
> 
> To be able to better understand this change: do we have access to the 
> sh7757 documentation and does it actually explain how the CLK_SUP_PCLK bit 
> in MMCIF_CE_CLK_CTRL functions? Does it actually set the MMC bus clock to 
> be equal to the host clock?

My understanding is that neither Magnus nor I have access to the
documentation for the sh7757. Thus we could only infer things from
examining the source code. I would be very happy if someone with access to
the documentation could shed some further light on this. In particular,
I would be happy if this heuristic was shown not to be necessary. But
in lieu of access to the documentation I think this patch is reasonable.

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: sh_mmcif: mmc->f_max heuristic

[Simon Horman]

On Wed, Jun 20, 2012 at 07:56:44AM -0700, Brandt, Chris wrote:
> > I would be very happy if someone with access to
> > the documentation could shed some further light on this.
> 
> According to the SH7757 manual, setting the CLKDIV[3:0] in the CE_CLK_CTRL register 0xF selects Pck as the bus clock frequency. So, unlike other devices where Pck/2 (CLKDIV[3:0]=0) is the max bus speed, the SH7757 can run at Pck when CLKDIV=0xF.

Thanks Chris,

please correct me if I am wrong, but that seems to indicate the heuristic
is correct. At least for the sh7757 and sh7372.

RE: [PATCH] mmc: sh_mmcif: mmc->f_max heuristic

[Brandt, Chris]

> At least for the sh7757 and sh7372.
Also for the sh7724 (ie, only the sh7757 can run at full bus speed by setting CLKDIV=0xF which no other part can....yet)

But, with this new patch applied:

>  	if (!ret) {
>  		host->clk = clk_get_rate(host->hclk);
> -		host->mmc->f_max = host->clk / 2;
> +		host->mmc->f_max = p->sup_pclk ? host->clk : host->clk / 2;
>  		host->mmc->f_min = host->clk / 512;
>  	}

Then there's no need to check for p->sup_pclk in sh_mmcif_clock_control() because if (clk == host->clk) then p->sup_pclk can be assumed to be set.

-	if (p->sup_pclk && clk == host->clk)
+	if (clk == host->clk)
		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
	else
		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
				((fls(DIV_ROUND_UP(host->clk,
						   clk) - 1) - 1) << 16));


But, that's probably more along the lines of driver cleanup I guess.

-Chris


-----Original Message-----
From: Simon Horman [mailto:horms@verge.net.au] 
Sent: Wednesday, June 20, 2012 8:48 PM
To: Brandt, Chris
Cc: Guennadi Liakhovetski; linux-mmc@vger.kernel.org; linux-sh@vger.kernel.org; Chris Ball; Magnus Damm
Subject: Re: [PATCH] mmc: sh_mmcif: mmc->f_max heuristic

On Wed, Jun 20, 2012 at 07:56:44AM -0700, Brandt, Chris wrote:
> > I would be very happy if someone with access to
> > the documentation could shed some further light on this.
> 
> According to the SH7757 manual, setting the CLKDIV[3:0] in the CE_CLK_CTRL register 0xF selects Pck as the bus clock frequency. So, unlike other devices where Pck/2 (CLKDIV[3:0]=0) is the max bus speed, the SH7757 can run at Pck when CLKDIV=0xF.

Thanks Chris,

please correct me if I am wrong, but that seems to indicate the heuristic
is correct. At least for the sh7757 and sh7372.

Re: [PATCH] mmc: sh_mmcif: mmc->f_max heuristic

[Simon Horman]

On Thu, Jun 21, 2012 at 07:11:42AM -0700, Brandt, Chris wrote:
> > At least for the sh7757 and sh7372.
> Also for the sh7724 (ie, only the sh7757 can run at full bus speed by setting CLKDIV=0xF which no other part can....yet)

Thanks.

> But, with this new patch applied:
> 
> >  	if (!ret) {
> >  		host->clk = clk_get_rate(host->hclk);
> > -		host->mmc->f_max = host->clk / 2;
> > +		host->mmc->f_max = p->sup_pclk ? host->clk : host->clk / 2;
> >  		host->mmc->f_min = host->clk / 512;
> >  	}
> 
> Then there's no need to check for p->sup_pclk in sh_mmcif_clock_control() because if (clk == host->clk) then p->sup_pclk can be assumed to be set.
> 
> -	if (p->sup_pclk && clk == host->clk)
> +	if (clk == host->clk)
> 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
> 	else
> 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
> 				((fls(DIV_ROUND_UP(host->clk,
> 						   clk) - 1) - 1) << 16));
> 
> 
> But, that's probably more along the lines of driver cleanup I guess.

Thanks, I'll add that.

[PATCH] mmc: sh_mmcif: mmc->f_max heuristic

[Simon Horman]

In 930f152cc9998388031af577843baae572ac8ab6 ("mmc: sh_mmcif: mmc->f_max
should be half of the bus clock") I changed the setting of mmc->f_max from
the bus clock to half the bus clock based on the manual for the sh7372 SoC.

Inspection of sh_mmcif_clock_control() reveals that it relies on
mmc->f_max being set to the bus speed in order to enable the supplementary
clock, a feature that is only known to exist on the sh7757 SoC.

Armed with this information implement the following heuristic for setting
mmc->f_max:

* Use bus clock if the supplementary clock feature is present
  - Confirmed in the documentation of the sh7757 SoC.
* Use half the bus clock otherwise
  - As per the documentation for the sh7372, sh7724 and R8A7740 SoCs.

Reported-by: Magnus Damm <damm@opensource.se>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Chris Brandt <Chris.Brandt@renesas.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Regression information:

It seems likely that this resolves a regression for the sh7757lcr which was
introduced by 930f152cc9998388031af577843baae572ac8ab6 ("mmc: sh_mmcif:
mmc->f_max should be half of the bus clock"). However, although I have the
hardware in question I am unsure how to exercise the code to test if there
is a problem or not.

930f152cc9998388031af577843baae572ac8ab6 was added between the 3.3 and 3.4.

Dependency information:

This patch is based on Guennadi Liakhovetski's recent series
"[PATCH 0/5 v4] mmc: sh_mmcif: clock and power updates". It should
be trivial to rebase it on 3.5-rc3.
---
 drivers/mmc/host/sh_mmcif.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 204bced..04b14d0 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -450,7 +450,11 @@ static void sh_mmcif_clock_control(struct sh_mmcif_host *host, unsigned int clk)
 
 	if (!clk)
 		return;
-	if (p->sup_pclk && clk == host->clk)
+	/* clk == host->clk implies that p->sup_pclk is non-zero
+	 * as otherwise the maximum value of clk is host->clk / 2
+	 * as per the value of host->mmc->f_max
+	 */
+	if (clk == host->clk)
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_SUP_PCLK);
 	else
 		sh_mmcif_bitset(host, MMCIF_CE_CLK_CTRL, CLK_CLEAR &
@@ -913,10 +917,11 @@ static void sh_mmcif_request(struct mmc_host *mmc, struct mmc_request *mrq)
 static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
 {
 	int ret = clk_enable(host->hclk);
+	struct sh_mmcif_plat_data *p = host->pd->dev.platform_data;
 
 	if (!ret) {
 		host->clk = clk_get_rate(host->hclk);
-		host->mmc->f_max = host->clk / 2;
+		host->mmc->f_max = p->sup_pclk ? host->clk : host->clk / 2;
 		host->mmc->f_min = host->clk / 512;
 	}
 
-- 
1.7.10.2.484.gcd07cc5