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

From: "Michał Mirosław" <mirqus@gmail.com>
To: zhangfei gao <zhangfei.gao@gmail.com>
Cc: Matt Fleming <matt@console-pimps.org>,
	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: Fri, 13 Aug 2010 16:25:35 +0000	[thread overview]
Message-ID: <AANLkTimXNGLC9u+EVoOUurUY08icZpYycECsPf0RGYVh@mail.gmail.com> (raw)
In-Reply-To: <AANLkTi=zvsWHmLqWMnssgZTXd6XDSCzuJ3j8ZiQfdp9z@mail.gmail.com>

2010/8/12 zhangfei gao <zhangfei.gao@gmail.com>:
[...]
> updated patch:
> From b17eb3ca9dd23af63ada240e8f77bdd873e853a7 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>
> Reviewed-by: Matt Fleming <matt@console-pimps.org>
> ---
>  drivers/mmc/host/sdhci.c |   16 ++++++++++++----
>  drivers/mmc/host/sdhci.h |    4 ++++
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 7855121..ccfbc40 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -983,7 +983,7 @@ static void sdhci_finish_command(struct sdhci_host *host)
>
>  static void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
> -       int div;
> +       int div, max_div;
>        u16 clk;
>        unsigned long timeout;
>
> @@ -1001,13 +1001,21 @@ static void sdhci_set_clock(struct sdhci_host
> *host, unsigned int clock)
>        if (clock == 0)
>                goto out;
>
> -       for (div = 1;div < 256;div *= 2) {
> +       if (host->version >= SDHCI_SPEC_300)
> +               max_div = 2046;
> +       else
> +               max_div = 256;
> +
> +       if(host->max_clk <= clock)
> +               div = 1;
> +       for (div = 2; div < max_div; div += 2) {
>                if ((host->max_clk / div) <= clock)
>                        break;
>        }
>        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;

I think the right shift is using wrong #define, SDHCI_DIVIDER_SHIFT
(position of the bitfield in the register). It has a correct value of
8, though. This could be something like the following to reduce
confusion.

clk |= ((div & SDHCI_DIV_HI_MASK) >> SDHCI_DIV_MASK_LEN) <<
SDHCI_DIVIDER_SHIFT_HI;
...
#define SDHCI_DIV_MASK 0xFF
#define SDHCI_DIV_MASK_LEN 8
#define  SDHCI_DIV_HI_MASK 0x300

or maybe:

clk |= ((div >> SDHCI_DIV_MASK_LEN) & SDHCI_DIV_HI_MASK) <<
SDHCI_DIVIDER_SHIFT_HI;
...
#define SDHCI_DIV_MASK 0xFF
#define SDHCI_DIV_MASK_LEN 8
#define  SDHCI_DIV_HI_MASK 0x03

>        clk |= SDHCI_CLOCK_INT_EN;
>        sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>
[...]
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -86,6 +86,9 @@
>
>  #define SDHCI_CLOCK_CONTROL    0x2C
>  #define  SDHCI_DIVIDER_SHIFT   8
> +#define  SDHCI_DIVIDER_SHIFT_HI        6
> +#define  SDHCI_DIV_MASK        0xFF
> +#define  SDHCI_DIV_HI_MASK     0x300
>  #define  SDHCI_CLOCK_CARD_EN   0x0004
>  #define  SDHCI_CLOCK_INT_STABLE        0x0002
>  #define  SDHCI_CLOCK_INT_EN    0x0001
[...]

SDHCI_DIVIDER_SHIFT_HI -> SDHCI_DIVIDER_HI_SHIFT

Best Regards,
Michał Mirosław