From: Adrian Hunter <adrian.hunter@intel.com>
To: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>,
"Kevin Liu" <kliu5@marvell.com>,
"Michal Simek" <michal.simek@xilinx.com>,
"Suneel Garapati" <suneel.garapati@xilinx.com>,
"Ulf Hansson" <ulf.hansson@linaro.org>
Cc: linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org
Subject: Re: [PATCH 5/7] mmc: sdhci: simplify clock frequency calculation
Date: Wed, 15 Apr 2020 16:54:33 +0300 [thread overview]
Message-ID: <7967c035-52e3-1ce7-a82f-47d28a3cf484@intel.com> (raw)
In-Reply-To: <1a7f7f0941314da66acda3c60f44b3d2417133e6.1585827904.git.mirq-linux@rere.qmqm.pl>
On 2/04/20 2:54 pm, Michał Mirosław wrote:
> Make clock frequency calculations simpler by replacing loops
> with divide-and-clamp.
I am sorry, but I am not really sure the simplification is worth the code
churn, risk of introducing new bugs, or validation effort.
IMO, the loops, while perhaps inefficient, are not hard to understand.
>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> drivers/mmc/host/sdhci.c | 56 +++++++++++++++++++---------------------
> drivers/mmc/host/sdhci.h | 4 +--
> 2 files changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index ed88ac4e4cf3..d750c0997c3f 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1756,10 +1756,13 @@ static u16 sdhci_get_preset_value(struct sdhci_host *host)
> u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> unsigned int *actual_clock)
> {
> - int div = 0; /* Initialized for compiler warning */
> + unsigned int div = 0; /* Initialized for compiler warning */
> int real_div = div, clk_mul = 1;
> u16 clk = 0;
> - bool switch_base_clk = false;
> + bool use_base_clk;
> +
> + if (clock == 0)
> + unreachable();
>
> if (host->version >= SDHCI_SPEC_300) {
> if (host->preset_enabled) {
> @@ -1781,13 +1784,12 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> * Check if the Host Controller supports Programmable Clock
> * Mode.
> */
> - if (host->clk_mul) {
> - for (div = 1; div <= 1024; div++) {
> - if ((host->max_clk * host->clk_mul / div)
> - <= clock)
> - break;
> - }
> - if ((host->max_clk * host->clk_mul / div) <= clock) {
> + use_base_clk = !host->clk_mul;
> +
> + if (!use_base_clk) {
> + div = DIV_ROUND_UP(host->max_clk * host->clk_mul, clock);
> +
> + if (div <= SDHCI_MAX_DIV_SPEC_300 / 2 + 1) {
> /*
> * Set Programmable Clock Mode in the Clock
> * Control register.
> @@ -1798,35 +1800,31 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
> div--;
> } else {
> /*
> - * Divisor can be too small to reach clock
> - * speed requirement. Then use the base clock.
> + * Divisor is too big for requested clock rate.
> + * Use the base clock, then.
> */
> - switch_base_clk = true;
> + use_base_clk = true;
> }
> }
>
> - if (!host->clk_mul || switch_base_clk) {
> - /* Version 3.00 divisors must be a multiple of 2. */
> - if (host->max_clk <= clock) {
> - div = 1;
> - if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> - div = 2;
> - } else {
> - for (div = 2; div < SDHCI_MAX_DIV_SPEC_300;
> - div += 2) {
> - if ((host->max_clk / div) <= clock)
> - break;
> - }
> + if (use_base_clk) {
> + /* Version 3.00 divisors must be 1 or a multiple of 2. */
> + div = DIV_ROUND_UP(host->max_clk, clock);
> + if (div > 1) {
> + div = min(div, SDHCI_MAX_DIV_SPEC_300);
> + div = round_up(div, 2);
> }
> - real_div = div;
> div >>= 1;
> + if (host->quirks2 & SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN)
> + div += !div;
> +
> + real_div = div * 2 + !div;
> }
> } else {
> /* Version 2.00 divisors must be a power of 2. */
> - for (div = 1; div < SDHCI_MAX_DIV_SPEC_200; div *= 2) {
> - if ((host->max_clk / div) <= clock)
> - break;
> - }
> + div = DIV_ROUND_UP(host->max_clk, clock);
> + div = min(div, SDHCI_MAX_DIV_SPEC_200);
> + div = roundup_pow_of_two(div);
> real_div = div;
> div >>= 1;
> }
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 79dffbb731d3..ea8aabb3bf16 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -290,8 +290,8 @@
> * End of controller registers.
> */
>
> -#define SDHCI_MAX_DIV_SPEC_200 256
> -#define SDHCI_MAX_DIV_SPEC_300 2046
> +#define SDHCI_MAX_DIV_SPEC_200 256u
> +#define SDHCI_MAX_DIV_SPEC_300 2046u
>
> /*
> * Host SDMA buffer boundary. Valid values from 4K to 512K in powers of 2.
>
next prev parent reply other threads:[~2020-04-15 13:55 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-02 11:54 [PATCH 0/7] SDHCI clock handling fixes and cleanups Michał Mirosław
2020-04-02 11:54 ` [PATCH 3/7] mmc: sdhci: fix SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN Michał Mirosław
2020-04-15 13:06 ` Adrian Hunter
2020-04-15 16:03 ` Michał Mirosław
2020-04-16 7:40 ` Adrian Hunter
2020-04-02 11:54 ` [PATCH 2/7] mmc: sdhci: fix programmable clock config from preset value Michał Mirosław
2020-04-15 12:44 ` Adrian Hunter
2020-04-02 11:54 ` [PATCH 1/7] mmc: sdhci: fix base clock usage in " Michał Mirosław
2020-04-15 12:25 ` Adrian Hunter
2020-04-15 16:28 ` Michał Mirosław
2020-04-16 8:26 ` Adrian Hunter
2020-04-17 13:55 ` Alan Cooper
2020-04-20 5:31 ` Adrian Hunter
2020-04-02 11:54 ` [PATCH 4/7] mmc: sdhci: move SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN frequency limit Michał Mirosław
2020-04-15 13:16 ` Adrian Hunter
2020-04-15 16:16 ` Michał Mirosław
2020-04-16 7:43 ` Adrian Hunter
2020-04-02 11:54 ` [PATCH 6/7] mmc: sdhci: squash v2/v3+ clock calculation differences Michał Mirosław
2020-04-02 11:54 ` [PATCH 5/7] mmc: sdhci: simplify clock frequency calculation Michał Mirosław
2020-04-03 13:02 ` kbuild test robot
2020-04-03 16:15 ` Michał Mirosław
2020-04-15 13:54 ` Adrian Hunter [this message]
2020-04-15 16:45 ` Michał Mirosław
2020-04-02 11:54 ` [PATCH 7/7] mmc: sdhci: respect non-zero div quirk in programmable clock mode Michał Mirosław
2020-04-03 16:03 ` kbuild test robot
2020-04-15 8:52 ` [PATCH 0/7] SDHCI clock handling fixes and cleanups Ulf Hansson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7967c035-52e3-1ce7-a82f-47d28a3cf484@intel.com \
--to=adrian.hunter@intel.com \
--cc=kliu5@marvell.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=michal.simek@xilinx.com \
--cc=mirq-linux@rere.qmqm.pl \
--cc=suneel.garapati@xilinx.com \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).