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 <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Grant,
Thanks for posting! See below...
On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler <[email protected]> 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
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> ---
>> 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
>>
On Wed, Mar 27, 2013 at 5:25 AM, Seungwon Jeon <[email protected]> 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 <[email protected]>
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 <[email protected]>
>> ---
>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi Grant,
On Wed, Mar 27 2013, Grant Grundler wrote:
> On Wed, Mar 27, 2013 at 5:25 AM, Seungwon Jeon <[email protected]> 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 <[email protected]>
> 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 <[email protected]> <http://printf.net/>
One Laptop Per Child
On Wed, Mar 27, 2013 at 8:07 AM, Doug Anderson <[email protected]> wrote:
> Grant,
>
> Thanks for posting! See below...
thanks for reviewing/feedback! :)
> On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler <[email protected]> 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
Hi Chris,
On Wed, Mar 27, 2013 at 10:58 AM, Chris Ball <[email protected]> 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