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

From: Grant Grundler
Date: Wed Mar 27 2013 - 13:46:31 EST

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