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

[Grant Grundler]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
> 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
>
#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");
		}
	}
}