Re: [PATCH v2 1/3] mmc: k1: handle a missing max-frequency and clk_set_rate()'s return value

[Yixun Lan <dlan@kernel.org>]

Hi Dave,

On 17:55 Mon 05 Oct     , Dave Klotz wrote:
> The driver sets the io clock to the max-frequency the device tree gives
> and fails the probe on any non-zero return from clk_set_rate(). Two
> problems follow:
> 
I can understand your problem and what to solve.. but wouldn't it be
a more simpler way to just setting max-frequency?

currently, only two boards are supported in mainline uboot:
  k1-bananapi-f3 k1-musepi-pro
so, you likely also need to add a customized u-boot dtsi file for
supporting OrangePi R2S, something as file below, where setting the
max-frequency property
 arch/riscv/dts/k1-bananapi-f3-u-boot.dtsi

> - clk_set_rate() returns the new rate on success and a negative error
>   code on failure. The SpacemiT clock driver happens to return 0, but
>   any clock driver that reports the rate makes the probe fail.
Could you elaborate a bit "happens to return 0, ..probe fail" ?

> - A node without max-frequency asks for 0 Hz. On the K1 clock driver
A node with DT property get 0 Hz - that's expected behavior

>   that picks no divider at all and writes whatever the calculation left
>   behind into the io clock's divider field; host->max_clk is then 0,
>   the capabilities register gives no base clock either, and the probe
>   fails in sdhci_setup_cfg().
I occur to me, we should further check these logic, like a rate = 0 input
for clk subsystem - clk_set_rate(, 0) is actually a valid case? also if
need to set host->max_clk ..

> 
> Treat only a negative return as an error. Without max-frequency, leave
> the io clock as an earlier stage set it and run from its current rate.
> 
> Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
> 
> Changes in v2:
> - Use ulong and IS_ERR_VALUE() for the clock API return values.
> - Validate clk_get_rate() before narrowing to cfg.f_max and retain the error.
> 
> ---
>  drivers/mmc/spacemit_sdhci.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/spacemit_sdhci.c b/drivers/mmc/spacemit_sdhci.c
> index f5578f38bd..6488ae0fdb 100644
> --- a/drivers/mmc/spacemit_sdhci.c
> +++ b/drivers/mmc/spacemit_sdhci.c
> @@ -606,10 +606,30 @@
>  		goto err_reset;
>  	}
>  
> -	ret = clk_set_rate(&clk, plat->cfg.f_max);
> -	if (ret) {
> -		log_err("Failed to set io clk: %d\n", ret);
> -		goto err_reset;
> +	/*
> +	 * Without max-frequency there is no rate to ask for: leave the io
> +	 * clock as an earlier stage set it and run from its current rate. The
I'm wondering if we could set a sane default max-frequency if no DT property
provided? as we can't always trust previous earlier stage which may in
an undetermined state, it will be fragile and easy to fail..

> +	 * capabilities register gives no base clock on the K1, so it cannot
> +	 * stand in. clk_set_rate() returns the new rate on success, so only a
> +	 * negative value is an error.
> +	 */
> +	if (plat->cfg.f_max) {
> +		ulong rate = clk_set_rate(&clk, plat->cfg.f_max);
> +
> +		if (IS_ERR_VALUE(rate)) {
> +			ret = (int)rate;
> +			log_err("Failed to set io clk: %d\n", ret);
> +			goto err_reset;
> +		}
> +	} else {
> +		ulong rate = clk_get_rate(&clk);
> +
> +		if (IS_ERR_VALUE(rate) || !rate) {
> +			ret = rate ? (int)rate : -EINVAL;
> +			log_err("No max-frequency and no io clk rate\n");
> +			goto err_reset;
> +		}
> +		plat->cfg.f_max = rate;
>  	}
>  
>  	/* Set quirks */
> -- 
> 2.55.0
> 

-- 
Yixun Lan (dlan)