Re: [patch 1/1]sdhci: sdhc spec 3.0 add some modification

From: Matt Fleming <matt@console-pimps.org>
To: zhangfei gao <zhangfei.gao@gmail.com>
Cc: Kyungmin Park <kmpark@infradead.org>,
	linux-mmc@vger.kernel.org,
	Anton Vorontsov <cbouatmailru@gmail.com>,
	Ben Dooks <ben-linux@fluff.org>,
	Wolfram Sang <w.sang@pengutronix.de>
Subject: Re: [patch 1/1]sdhci: sdhc spec 3.0 add some modification
Date: Mon, 9 Aug 2010 11:10:42 +0100	[thread overview]
Message-ID: <20100809101042.GA8293@console-pimps.org> (raw)
In-Reply-To: <AANLkTinn+Af7kE34kjLEf3XLe2YTbpkJ4MtePFK0vMS8@mail.gmail.com>

On Thu, Aug 05, 2010 at 03:09:14PM +0800, zhangfei gao wrote:
> >>
> >>        1. support 8 bit data transfer width
> > 8 bit buswidth patch is already merged to mm tree.
> >
> > http://marc.info/?l=linux-mmc&m=127783663526810&w=3
> >
> > Thank you,
> > Kyungmin Park
> >
> 
> Thanks Kyungmin, updated without 8 bit support.
> 
> From 2b717444443acdac90c5f31522a1515a2000749a Mon Sep 17 00:00:00 2001
> From: Zhangfei Gao <zgao6@marvell.com>
> Date: Fri, 6 Aug 2010 07:10:01 +0800
> Subject: [PATCH] sdhc: support 10-bit divided clock Mode
> 
> 
> Signed-off-by: Zhangfei Gao <zgao6@marvell.com>
> ---
>  drivers/mmc/host/sdhci.c |    5 +++--
>  drivers/mmc/host/sdhci.h |    4 ++++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c6d1bd8..212dc9e 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1002,7 +1002,8 @@ static void sdhci_set_clock(struct sdhci_host
> *host, unsigned int clock)
>  	}
>  	div >>= 1;
> 
> -	clk = div << SDHCI_DIVIDER_SHIFT;
> +	clk = (div & SDHCI_DIV_MASK) << SDHCI_DIVIDER_SHIFT;
> +	clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIVIDER_SHIFT) <<
> SDHCI_DIVIDER_SHIFT_HI;
>  	clk |= SDHCI_CLOCK_INT_EN;
>  	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> 

This patch is missing a change to the way we calculate 'div'. While your
patch is correct, I think it would be better to fit some more things
into it.

This is the current code for calculating the clock divider,

	for (div = 1;div < 256;div *= 2) {
		if ((host->max_clk / div) <= clock)
			break;
	}
	div >>= 1;

	clk = div << SDHCI_DIVIDER_SHIFT;

even with your patch applied 'div' will never be greater 256 and we
won't be executing your changes. So I think this patch also needs to
bump the upper limit of 'div' to 2046.

We should probably introduce some safety at this point and check that
the divider value is legal for the version of the host controller, to
stop people shooting themselves in the foot if they mess up their clock
settings. I was thinking of something like,

	if (host->version >= SDHCI_SPEC_300)
		max_div = 2046;
	else
		max_div = 256;

	for (div = 1;div < max_div;div *= 2) {
		if ((host->max_clk / div) <= clock)
			break;
	}

What do you think?