[PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[Grant Grundler]

Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
But when debugging a related issue (http://crbug.com/221828) I found
the code unreadable.  This rewrite simplifies the computation and
explains each step.

Signed-off-by: Grant Grundler <grundler@chromium.org>
---
Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).

I've written a test program to confirm this patch generates the
same correct values and will share that separately.

 drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)


diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 9834221..6fdf55b 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
 
 	if (slot->clock != host->current_speed || force_clkinit) {
 		div = host->bus_hz / slot->clock;
-		if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
-			/*
-			 * move the + 1 after the divide to prevent
-			 * over-clocking the card.
+		if (host->bus_hz > slot->clock) {
+			/* don't overclock due to integer math losses */
+			if ((div * slot->clock) < host->bus_hz)
+				div++;
+
+			/* don't overclock due to resolution of HW */
+			if (div & 1)
+				div++;
+
+			/* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
+			 * Look for dwc_mobile_storage_db.pdf from Synopsys.
+			 * CLKDIV value 0 means divisor 1, val 1 -> 2, ...
 			 */
-			div += 1;
-
-		div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
+			div /= 2;
+		} else
+			div = 0;        /* use bus_hz */
 
 		dev_info(&slot->mmc->class_dev,
 			 "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ"
-- 
1.8.1.3

Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[Grant Grundler]

[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]

I've attached the test program I wrote to compare the different
flavors of CLKDIV computation: old (3.4 kernel), current upstream, and
my rewrite.

thanks
grant

On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler <grundler@chromium.org> wrote:
> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
> But when debugging a related issue (http://crbug.com/221828) I found
> the code unreadable.  This rewrite simplifies the computation and
> explains each step.
>
> Signed-off-by: Grant Grundler <grundler@chromium.org>
> ---
> Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
> ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).
>
> I've written a test program to confirm this patch generates the
> same correct values and will share that separately.
>
>  drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 9834221..6fdf55b 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>
>         if (slot->clock != host->current_speed || force_clkinit) {
>                 div = host->bus_hz / slot->clock;
> -               if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
> -                       /*
> -                        * move the + 1 after the divide to prevent
> -                        * over-clocking the card.
> +               if (host->bus_hz > slot->clock) {
> +                       /* don't overclock due to integer math losses */
> +                       if ((div * slot->clock) < host->bus_hz)
> +                               div++;
> +
> +                       /* don't overclock due to resolution of HW */
> +                       if (div & 1)
> +                               div++;
> +
> +                       /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
> +                        * Look for dwc_mobile_storage_db.pdf from Synopsys.
> +                        * CLKDIV value 0 means divisor 1, val 1 -> 2, ...
>                          */
> -                       div += 1;
> -
> -               div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
> +                       div /= 2;
> +               } else
> +                       div = 0;        /* use bus_hz */
>
>                 dev_info(&slot->mmc->class_dev,
>                          "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ"
> --
> 1.8.1.3
>

[-- Attachment #2: test_dw_mmc.c --]
[-- Type: text/x-csrc, Size: 2894 bytes --]

#include <stdio.h>

/* from include/linux/kernel.h */
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))

unsigned int original(unsigned int bus_hz, unsigned int clock)
{
	unsigned int div;

	if (bus_hz % clock)
		/*
		 * move the + 1 after the divide to prevent
		 * over-clocking the card.
		 */
		div = ((bus_hz / clock) >> 1) + 1;
	else
		div = (bus_hz  / clock) >> 1;

	return div;
}

unsigned int upstream(unsigned int bus_hz, unsigned int clock)
{
	unsigned int div;

	div = bus_hz / clock;
	if (bus_hz % clock && bus_hz > clock)
		/*
		 * move the + 1 after the divide to prevent
	 	 * over-clocking the card.
		 */
		div += 1;

	div = (bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;

	return div;
}


unsigned int ggg(unsigned int bus_hz, unsigned int clock)
{
	unsigned int div;

	div = bus_hz / clock;
	if (bus_hz > clock) {
		/* don't overclock due to integer math losses */
		if ((div * clock) < bus_hz)
			div++;

		/* don't overclock due to resolution of HW */
		if (div & 1)
			div++;

		/* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
		 * Look for dwc_mobile_storage_db.pdf from Synopsys.
		 * CLKDIV value 0 means divisor 1, value 1 -> 2, val 2 -> 4 etc.
		 */
		div /= 2;
	} else
		div = 0;	/* use bus_hz */

	return div;
}

unsigned int bus_hz_tbl[] = { 13, 17, 19, 20, 50000000, 100000000};
unsigned int slot_hz_tbl[] = { 10, 13, 21, 40, 57, 400000, 784314, 52000000};

static unsigned int div_to_hz(unsigned int bus_hz, unsigned int d)
{
	 return d ? (bus_hz/(d*2)) : bus_hz;
}

static void verify_hz(unsigned int bus_hz, unsigned int requested_hz,
		unsigned int divided_hz, const char *algo_name)
{
	if (divided_hz > bus_hz)
		printf(" [FAIL:  %s > bus hz!]", algo_name);

	if (divided_hz > requested_hz)
		printf(" [FAIL: %s > slot hz!]", algo_name);
}

void main(void)
{
	unsigned int i, j;

	printf("bus/slot hz 	Original	Upstream	GGG\n");
	for(i=0; i < sizeof(bus_hz_tbl)/sizeof(unsigned int); i++) {
		unsigned int bus_hz = bus_hz_tbl[i];
		for (j=0; j < sizeof(slot_hz_tbl)/sizeof(unsigned int); j++) {
			unsigned int slot_hz = slot_hz_tbl[j];
			unsigned int x = original(bus_hz, slot_hz);
			unsigned int y = upstream(bus_hz, slot_hz);
			unsigned int z = ggg(bus_hz, slot_hz);

			unsigned int hz_x, hz_y, hz_z;

			hz_x = div_to_hz(bus_hz, x);
			hz_y = div_to_hz(bus_hz, y);
			hz_z = div_to_hz(bus_hz, z);

			printf("%8d/%6d	%7d %6d	%7d %6d	%7d %6d",
				bus_hz, slot_hz, x, hz_x, y, hz_y, z, hz_z);

			verify_hz(bus_hz, slot_hz, hz_x, "Original");
			verify_hz(bus_hz, slot_hz, hz_y, "Upstream");
			verify_hz(bus_hz, slot_hz, hz_z, "GGG");

			if (x != y)
#if 0
/* We know original (3.4 kernel) version was buggy.
 * Don't have to be quite so loud about it.
 */
				printf("DEBUG: Orig != Upstream\n",
					bus_hz, slot_hz);

#else
				printf(" [Orig Fixed]");
#endif
			if (z != y)
				printf(" WARN: GGG != Upstream");

			printf("\n");
		}
	}
}

RE: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[Seungwon Jeon]

On Wednesday, March 27, 2013, Grant Grundler wrote:
> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
For easily identifying, it would be good to point the commit id and subject.
> But when debugging a related issue (http://crbug.com/221828) I found
It is not easy to catch up issue in your site. What problem is bothering you?
Could you describe the problem and conditions you have found?

Thanks,
Seungwon Jeon

> the code unreadable.  This rewrite simplifies the computation and
> explains each step.
> 
> Signed-off-by: Grant Grundler <grundler@chromium.org>
> ---
> Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
> ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).
> 
> I've written a test program to confirm this patch generates the
> same correct values and will share that separately.
> 
>  drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 9834221..6fdf55b 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
> 
>  	if (slot->clock != host->current_speed || force_clkinit) {
>  		div = host->bus_hz / slot->clock;
> -		if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
> -			/*
> -			 * move the + 1 after the divide to prevent
> -			 * over-clocking the card.
> +		if (host->bus_hz > slot->clock) {
> +			/* don't overclock due to integer math losses */
> +			if ((div * slot->clock) < host->bus_hz)
> +				div++;
> +
> +			/* don't overclock due to resolution of HW */
> +			if (div & 1)
> +				div++;
> +
> +			/* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
> +			 * Look for dwc_mobile_storage_db.pdf from Synopsys.
> +			 * CLKDIV value 0 means divisor 1, val 1 -> 2, ...
>  			 */
> -			div += 1;
> -
> -		div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
> +			div /= 2;
> +		} else
> +			div = 0;        /* use bus_hz */
> 
>  		dev_info(&slot->mmc->class_dev,
>  			 "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ"
> --
> 1.8.1.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" 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: dw_mmc: rewrite CLKDIV computation

[Doug Anderson]

Grant,

Thanks for posting!  See below...

On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler <grundler@chromium.org> wrote:
> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
> But when debugging a related issue (http://crbug.com/221828) I found
> the code unreadable.  This rewrite simplifies the computation and
> explains each step.

The fact that you mention a bug here is confusing.  I think this patch
has nothing to do with fixing that bug and is just a readability
patch.  Is that correct?  Please add to the description if so and
maybe remove unrelated comment about the bug.


> +                       /* don't overclock due to resolution of HW */
> +                       if (div & 1)
> +                               div++;
> +
> +                       /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
> +                        * Look for dwc_mobile_storage_db.pdf from Synopsys.
> +                        * CLKDIV value 0 means divisor 1, val 1 -> 2, ...

You are quoting exynos5250 docs here.  This driver is used for more
than just exynos and so this could be confusing to users on other
platforms.


>                          */
> -                       div += 1;
> -
> -               div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
> +                       div /= 2;

It does look like you're re-implementing DIV_ROUND_UP.  Maybe replace
your "if" test and division with just a DIV_ROUND_UP?


-Doug

Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[Grant Grundler]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

Sorry - please ignore the previous version. Did not include a
copyright (implied to be mine...but it's not) nor license.

Same thing but with proper attribution.

cheers,
grant

On Tue, Mar 26, 2013 at 3:53 PM, Grant Grundler <grundler@chromium.org> wrote:
> I've attached the test program I wrote to compare the different
> flavors of CLKDIV computation: old (3.4 kernel), current upstream, and
> my rewrite.
>
> thanks
> grant
>
> On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler <grundler@chromium.org> wrote:
>> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
>> But when debugging a related issue (http://crbug.com/221828) I found
>> the code unreadable.  This rewrite simplifies the computation and
>> explains each step.
>>
>> Signed-off-by: Grant Grundler <grundler@chromium.org>
>> ---
>> Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
>> ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).
>>
>> I've written a test program to confirm this patch generates the
>> same correct values and will share that separately.
>>
>>  drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 9834221..6fdf55b 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>
>>         if (slot->clock != host->current_speed || force_clkinit) {
>>                 div = host->bus_hz / slot->clock;
>> -               if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
>> -                       /*
>> -                        * move the + 1 after the divide to prevent
>> -                        * over-clocking the card.
>> +               if (host->bus_hz > slot->clock) {
>> +                       /* don't overclock due to integer math losses */
>> +                       if ((div * slot->clock) < host->bus_hz)
>> +                               div++;
>> +
>> +                       /* don't overclock due to resolution of HW */
>> +                       if (div & 1)
>> +                               div++;
>> +
>> +                       /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
>> +                        * Look for dwc_mobile_storage_db.pdf from Synopsys.
>> +                        * CLKDIV value 0 means divisor 1, val 1 -> 2, ...
>>                          */
>> -                       div += 1;
>> -
>> -               div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
>> +                       div /= 2;
>> +               } else
>> +                       div = 0;        /* use bus_hz */
>>
>>                 dev_info(&slot->mmc->class_dev,
>>                          "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ"
>> --
>> 1.8.1.3
>>

[-- Attachment #2: test_dw_mmc.c --]
[-- Type: text/x-csrc, Size: 3725 bytes --]

/* Test dw_mmc driver CLKDIV computation.
 * Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License, or
 * (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 */

#include <stdio.h>

/* from include/linux/kernel.h */
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))

unsigned int original(unsigned int bus_hz, unsigned int clock)
{
	unsigned int div;

	if (bus_hz % clock)
		/*
		 * move the + 1 after the divide to prevent
		 * over-clocking the card.
		 */
		div = ((bus_hz / clock) >> 1) + 1;
	else
		div = (bus_hz  / clock) >> 1;

	return div;
}

unsigned int upstream(unsigned int bus_hz, unsigned int clock)
{
	unsigned int div;

	div = bus_hz / clock;
	if (bus_hz % clock && bus_hz > clock)
		/*
		 * move the + 1 after the divide to prevent
	 	 * over-clocking the card.
		 */
		div += 1;

	div = (bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;

	return div;
}


unsigned int ggg(unsigned int bus_hz, unsigned int clock)
{
	unsigned int div;

	div = bus_hz / clock;
	if (bus_hz > clock) {
		/* don't overclock due to integer math losses */
		if ((div * clock) < bus_hz)
			div++;

		/* don't overclock due to resolution of HW */
		if (div & 1)
			div++;

		/* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
		 * Look for dwc_mobile_storage_db.pdf from Synopsys.
		 * CLKDIV value 0 means divisor 1, value 1 -> 2, val 2 -> 4 etc.
		 */
		div /= 2;
	} else
		div = 0;	/* use bus_hz */

	return div;
}

unsigned int bus_hz_tbl[] = { 13, 17, 19, 20, 50000000, 100000000};
unsigned int slot_hz_tbl[] = { 10, 13, 21, 40, 57, 400000, 784314, 52000000};

static unsigned int div_to_hz(unsigned int bus_hz, unsigned int d)
{
	 return d ? (bus_hz/(d*2)) : bus_hz;
}

static void verify_hz(unsigned int bus_hz, unsigned int requested_hz,
		unsigned int divided_hz, const char *algo_name)
{
	if (divided_hz > bus_hz)
		printf(" [FAIL:  %s > bus hz!]", algo_name);

	if (divided_hz > requested_hz)
		printf(" [FAIL: %s > slot hz!]", algo_name);
}

void main(void)
{
	unsigned int i, j;

	printf("bus/slot hz 	Original	Upstream	GGG\n");
	for(i=0; i < sizeof(bus_hz_tbl)/sizeof(unsigned int); i++) {
		unsigned int bus_hz = bus_hz_tbl[i];
		for (j=0; j < sizeof(slot_hz_tbl)/sizeof(unsigned int); j++) {
			unsigned int slot_hz = slot_hz_tbl[j];
			unsigned int x = original(bus_hz, slot_hz);
			unsigned int y = upstream(bus_hz, slot_hz);
			unsigned int z = ggg(bus_hz, slot_hz);

			unsigned int hz_x, hz_y, hz_z;

			hz_x = div_to_hz(bus_hz, x);
			hz_y = div_to_hz(bus_hz, y);
			hz_z = div_to_hz(bus_hz, z);

			printf("%8d/%6d	%7d %6d	%7d %6d	%7d %6d",
				bus_hz, slot_hz, x, hz_x, y, hz_y, z, hz_z);

			verify_hz(bus_hz, slot_hz, hz_x, "Original");
			verify_hz(bus_hz, slot_hz, hz_y, "Upstream");
			verify_hz(bus_hz, slot_hz, hz_z, "GGG");

			if (x != y)
#if 0
/* We know original (3.4 kernel) version was buggy.
 * Don't have to be quite so loud about it.
 */
				printf("DEBUG: Orig != Upstream\n",
					bus_hz, slot_hz);

#else
				printf(" [Orig Fixed]");
#endif
			if (z != y)
				printf(" WARN: GGG != Upstream");

			printf("\n");
		}
	}
}

Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[Grant Grundler]

On Wed, Mar 27, 2013 at 5:25 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
> On Wednesday, March 27, 2013, Grant Grundler wrote:
>> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
> For easily identifying, it would be good to point the commit id and subject.

commit id will be different for different git trees and branches - not
as useful as one might think.

I will include the following and update the patch:
   Author: Seungwon Jeon <tgih.jun@samsung.com>
   Date:   Tue May 22 13:01:21 2012 +0900
   mmc: dw_mmc: correct the calculation for CLKDIV


>> But when debugging a related issue (http://crbug.com/221828) I found
>
> It is not easy to catch up issue in your site. What problem is bothering you?
> Could you describe the problem and conditions you have found?

The bug is not relevant to this patch - it's a "related bug". I
mentioned the bug only to explain my motivation for looking at this
code. I will move the bug reference out of the commit message.

To summarize, I was trying to backport "mmc: dw_mmc: correct the
calculation for CLKDIV" patch to 3.4 kernel to support faster eMMC
parts since the original computation in 3.4 kernel was returning wrong
CLKDIV value. But then ran into other problems specific to one
firmware combination breaking the eMMC clock settings.

cheers,
grant

>
> Thanks,
> Seungwon Jeon
>
>> the code unreadable.  This rewrite simplifies the computation and
>> explains each step.
>>
>> Signed-off-by: Grant Grundler <grundler@chromium.org>
>> ---
>> Tested on Samsung Series 3 Chromebook (exynos 5250 chipset) using
>> ChromeOS 3.4 kernel (not 3.9-rc3 which this patch is based against).
>>
>> I've written a test program to confirm this patch generates the
>> same correct values and will share that separately.
>>
>>  drivers/mmc/host/dw_mmc.c | 22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 9834221..6fdf55b 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -631,14 +631,22 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
>>
>>       if (slot->clock != host->current_speed || force_clkinit) {
>>               div = host->bus_hz / slot->clock;
>> -             if (host->bus_hz % slot->clock && host->bus_hz > slot->clock)
>> -                     /*
>> -                      * move the + 1 after the divide to prevent
>> -                      * over-clocking the card.
>> +             if (host->bus_hz > slot->clock) {
>> +                     /* don't overclock due to integer math losses */
>> +                     if ((div * slot->clock) < host->bus_hz)
>> +                             div++;
>> +
>> +                     /* don't overclock due to resolution of HW */
>> +                     if (div & 1)
>> +                             div++;
>> +
>> +                     /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
>> +                      * Look for dwc_mobile_storage_db.pdf from Synopsys.
>> +                      * CLKDIV value 0 means divisor 1, val 1 -> 2, ...
>>                        */
>> -                     div += 1;
>> -
>> -             div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
>> +                     div /= 2;
>> +             } else
>> +                     div = 0;        /* use bus_hz */
>>
>>               dev_info(&slot->mmc->class_dev,
>>                        "Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ"
>> --
>> 1.8.1.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" 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: dw_mmc: rewrite CLKDIV computation

[Chris Ball]

Hi Grant,

On Wed, Mar 27 2013, Grant Grundler wrote:
> On Wed, Mar 27, 2013 at 5:25 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
>> On Wednesday, March 27, 2013, Grant Grundler wrote:
>>> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
>> For easily identifying, it would be good to point the commit id and subject.
>
> commit id will be different for different git trees and branches - not
> as useful as one might think.
>
> I will include the following and update the patch:
>    Author: Seungwon Jeon <tgih.jun@samsung.com>
>    Date:   Tue May 22 13:01:21 2012 +0900
>    mmc: dw_mmc: correct the calculation for CLKDIV

Please use the following (standard) syntax in the commit message:

Commit e419990b5e8 ("mmc: dw_mmc: correct the calculation for CLKDIV")
fixed a bug in CLKDIV computation.  [..]

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[Chris Ball]

Hi Grant,

On Wed, Mar 27 2013, Grant Grundler wrote:
> On Wed, Mar 27, 2013 at 5:25 AM, Seungwon Jeon <tgih.jun@samsung.com> wrote:
>> On Wednesday, March 27, 2013, Grant Grundler wrote:
>>> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
>> For easily identifying, it would be good to point the commit id and subject.
>
> commit id will be different for different git trees and branches - not
> as useful as one might think.
>
> I will include the following and update the patch:
>    Author: Seungwon Jeon <tgih.jun@samsung.com>
>    Date:   Tue May 22 13:01:21 2012 +0900
>    mmc: dw_mmc: correct the calculation for CLKDIV

Please use the following (standard) syntax in the commit message:

Commit e419990b5e8 ("mmc: dw_mmc: correct the calculation for CLKDIV")
fixed a bug in CLKDIV computation.  [..]

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[Grant Grundler]

On Wed, Mar 27, 2013 at 8:07 AM, Doug Anderson <dianders@chromium.org> wrote:
> Grant,
>
> Thanks for posting!  See below...

thanks for reviewing/feedback! :)

> On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler <grundler@chromium.org> wrote:
>> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
>> But when debugging a related issue (http://crbug.com/221828) I found
>> the code unreadable.  This rewrite simplifies the computation and
>> explains each step.
>
> The fact that you mention a bug here is confusing.  I think this patch
> has nothing to do with fixing that bug and is just a readability
> patch.  Is that correct?

Correct.  "related" implies "not the same". But you are right - the
reference is causing confusion.

I will move the reference to the bug out of the commit log to below
the '---' area of the patch.

>  Please add to the description if so and
> maybe remove unrelated comment about the bug.

Thanks! Will do and repost later today.

...
>> +                       /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
>> +                        * Look for dwc_mobile_storage_db.pdf from Synopsys.
>> +                        * CLKDIV value 0 means divisor 1, val 1 -> 2, ...
>
> You are quoting exynos5250 docs here.  This driver is used for more
> than just exynos and so this could be confusing to users on other
> platforms.

I'm quoting Synopsys docs - that's the origin of this HW's ip.
You and I looked at exynos5250 docs originally and they say exactly
the same thing. But the section numbers are different.

>
>>                          */
>> -                       div += 1;
>> -
>> -               div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
>> +                       div /= 2;
>
> It does look like you're re-implementing DIV_ROUND_UP.

Yes, it does look like that but by breaking it out into simple steps
AND explaining why we do each step, the code becomes maintainable by
normal developers. The comments are key to *quickly* understanding the
code in this case.

> Maybe replace your "if" test and division with just a DIV_ROUND_UP?

No. I'd rather just drop the patch. This code can and should be stupid
simple. DIV_ROUND_UP just makes it harder to understand and impossible
to document as clearly.

cheers,
grant

Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[Grant Grundler]

Hi Chris,

On Wed, Mar 27, 2013 at 10:58 AM, Chris Ball <cjb@laptop.org> wrote:
> Hi Grant,
...
> Please use the following (standard) syntax in the commit message:
>
> Commit e419990b5e8 ("mmc: dw_mmc: correct the calculation for CLKDIV")
> fixed a bug in CLKDIV computation.  [..]

Ok - I didn't know that was "standard". But easy enough to do.

cheers,
grant