| Message ID | ef56cc2cf2361bea7bdb478b253f9cf3a1cb6ea3.1790975596.git.DaveWK225@protonmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tim Ouyang |
| Headers | show |
| Series | mmc: k1: fixes for the SpacemiT K1 SDHCI driver | expand |
On 03-10-2026 02:59, 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: > > - 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. > - A node without max-frequency asks for 0 Hz. On the K1 clock driver > 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(). > > 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> > --- > drivers/mmc/spacemit_sdhci.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/spacemit_sdhci.c b/drivers/mmc/spacemit_sdhci.c > index f5578f38bd..bbd49b76c6 100644 > --- a/drivers/mmc/spacemit_sdhci.c > +++ b/drivers/mmc/spacemit_sdhci.c > @@ -606,10 +606,28 @@ static int spacemit_sdhci_probe(struct udevice *dev) > 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 > + * 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) { > + long rate = clk_set_rate(&clk, plat->cfg.f_max); The clock API returns ulong; please use ulong rate with IS_ERR_VALUE(rate) for consistency. > + > + if (rate < 0) { > + ret = rate; > + log_err("Failed to set io clk: %d\n", ret); > + goto err_reset; > + } > + } else { > + plat->cfg.f_max = clk_get_rate(&clk); clk_get_rate() returns ulong, but f_max is uint. On RV64, assigning a negative error first truncates it, so IS_ERR_VALUE(f_max) no longer detects the error. Please validate the ulong rate before assigning it: rate = clk_get_rate(&clk); if (IS_ERR_VALUE(rate) || !rate) { ret = rate ? (int)rate : -EINVAL; goto err_reset; } plat->cfg.f_max = rate;
On 03-10-2026 02:59, 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: > > - 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. > - A node without max-frequency asks for 0 Hz. On the K1 clock driver > 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(). > > 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> > --- > drivers/mmc/spacemit_sdhci.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/spacemit_sdhci.c b/drivers/mmc/spacemit_sdhci.c > index f5578f38bd..bbd49b76c6 100644 > --- a/drivers/mmc/spacemit_sdhci.c > +++ b/drivers/mmc/spacemit_sdhci.c > @@ -606,10 +606,28 @@ static int spacemit_sdhci_probe(struct udevice *dev) > 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 > + * 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) { > + long rate = clk_set_rate(&clk, plat->cfg.f_max); The clock API returns ulong; please use ulong rate with IS_ERR_VALUE(rate) for consistency. > + > + if (rate < 0) { > + ret = rate; > + log_err("Failed to set io clk: %d\n", ret); > + goto err_reset; > + } > + } else { > + plat->cfg.f_max = clk_get_rate(&clk); clk_get_rate() returns ulong, but f_max is uint. On RV64, assigning a negative error first truncates it, so IS_ERR_VALUE(f_max) no longer detects the error. Please validate the ulong rate before assigning it: rate = clk_get_rate(&clk); if (IS_ERR_VALUE(rate) || !rate) { ret = rate ? (int)rate : -EINVAL; goto err_reset; } plat->cfg.f_max = rate;
Hi Tanmay, Thanks for the review. I've updated v2 patch 1/3 to keep the clock API return values in ulong and test IS_ERR_VALUE(). The missing-max-frequency path now checks clk_get_rate() before assigning it to cfg.f_max, rejects a zero rate, and preserves the clock API's error code. I've retained your Reviewed-by on patches 2/3 and 3/3. Patch 2/3 now explains the intentional reversal of the driver's v9 single-block SPL policy and the MMC core's CMD12 path; its code is unchanged from v1. The revised three-patch stack replays cleanly on its exact base and passes checkpatch. Both R2S and RV2 configurations build successfully. With the separate DDR firmware overlap fix included, the combined firmware passed DDR initialization, FIT verification and automatic EFI/FreeBSD boot on the 2 GiB R2S and the 4 GiB RV2. These are combined-firmware warm-boot results, not isolated tests of this series. Cold boot of the corrected firmware, alternate cards, missing-frequency/error paths and error recovery remain untested. Thanks, Dave
diff --git a/drivers/mmc/spacemit_sdhci.c b/drivers/mmc/spacemit_sdhci.c index f5578f38bd..bbd49b76c6 100644 --- a/drivers/mmc/spacemit_sdhci.c +++ b/drivers/mmc/spacemit_sdhci.c @@ -606,10 +606,28 @@ static int spacemit_sdhci_probe(struct udevice *dev) 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 + * 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) { + long rate = clk_set_rate(&clk, plat->cfg.f_max); + + if (rate < 0) { + ret = rate; + log_err("Failed to set io clk: %d\n", ret); + goto err_reset; + } + } else { + plat->cfg.f_max = clk_get_rate(&clk); + if (IS_ERR_VALUE(plat->cfg.f_max) || !plat->cfg.f_max) { + ret = -EINVAL; + log_err("No max-frequency and no io clk rate\n"); + goto err_reset; + } } /* Set quirks */
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: - 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. - A node without max-frequency asks for 0 Hz. On the K1 clock driver 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(). 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> --- drivers/mmc/spacemit_sdhci.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)