| Message ID | 1364338202-10311-1-git-send-email-grundler@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 -- 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
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 -- 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
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 >>
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 > -- 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
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.
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 -- 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
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 -- 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
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"
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(-)