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 <[email protected]> > > 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)
