linux-kernel - Re: [PATCH 5/7] mmc: sdhci: simplify clock frequency calculation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7967c035-52e3-1ce7-a82f-47d28a3cf484@intel.com>
Date:   Wed, 15 Apr 2020 16:54:33 +0300
From:   Adrian Hunter <adrian.hunter@...el.com>
To:     Michał Mirosław <mirq-linux@...e.qmqm.pl>,
        Kevin Liu <kliu5@...vell.com>,
        Michal Simek <michal.simek@...inx.com>,
        Suneel Garapati <suneel.garapati@...inx.com>,
        Ulf Hansson <ulf.hansson@...aro.org>
Cc:     linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org
Subject: Re: [PATCH 5/7] mmc: sdhci: simplify clock frequency calculation

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@...e.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.
>