Re: [PATCH] mmc: host: dw-mmc-rockchip: fix handling invalid clock rates

[Ulf Hansson]

On Thu, 3 Mar 2022 at 02:52, Peter Geis <pgwipeout@xxxxxxxxx> wrote:
>
> The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> supports. This leads to a situation during card initialization where the
> ciu clock is set lower than the clock driver can support. The
> dw-mmc-rockchip driver spews errors when this happens.
> For normal operation this only happens a few times during boot, but when
> cd-broken is enabled (in cases such as the SoQuartz module) this fires
> multiple times each poll cycle.
>
> Fix this by testing the minimum frequency the clock driver can support
> that is within the mmc specification, then divide that by the internal
> clock divider. Set the f_min frequency to this value, or if it fails,
> set f_min to the downstream driver's default.
>
> Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
>
> Signed-off-by: Peter Geis <pgwipeout@xxxxxxxxx>
> ---
>  drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index 95d0ec0f5f3a..c198590cd74a 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -15,7 +15,9 @@
>  #include "dw_mmc.h"
>  #include "dw_mmc-pltfm.h"
>
> -#define RK3288_CLKGEN_DIV       2
> +#define RK3288_CLKGEN_DIV      2
> +#define RK3288_MIN_INIT_FREQ   375000
> +#define MMC_MAX_INIT_FREQ      400000
>
>  struct dw_mci_rockchip_priv_data {
>         struct clk              *drv_clk;
> @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
>  static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>  {
>         struct dw_mci_rockchip_priv_data *priv = host->priv;
> +       struct mmc_host *mmc = mmc_from_priv(host);
>         int ret;
>         unsigned int cclkin;
>         u32 bus_hz;
> @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>         if (ios->clock == 0)
>                 return;
>
> +       /* the clock will fail if below the f_min rate */
> +       if (ios->clock < mmc->f_min)
> +               ios->clock = mmc->f_min;
> +

You shouldn't need this. The mmc core should manage this already.

>         /*
>          * cclkin: source clock of mmc controller
>          * bus_hz: card interface clock generated by CLKGEN
> @@ -51,7 +58,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>
>         ret = clk_set_rate(host->ciu_clk, cclkin);
>         if (ret)
> -               dev_warn(host->dev, "failed to set rate %uHz\n", ios->clock);
> +               dev_warn(host->dev, "failed to set rate %uHz err: %d\n", cclkin, ret);
>
>         bus_hz = clk_get_rate(host->ciu_clk) / RK3288_CLKGEN_DIV;
>         if (bus_hz != host->bus_hz) {
> @@ -290,13 +297,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
>
>  static int dw_mci_rockchip_init(struct dw_mci *host)
>  {
> +       struct mmc_host *mmc = mmc_from_priv(host);
> +       int ret;
> +
>         /* It is slot 8 on Rockchip SoCs */
>         host->sdio_id0 = 8;
>
> -       if (of_device_is_compatible(host->dev->of_node,
> -                                   "rockchip,rk3288-dw-mshc"))
> +       if (of_device_is_compatible(host->dev->of_node, "rockchip,rk3288-dw-mshc")) {
>                 host->bus_hz /= RK3288_CLKGEN_DIV;
>
> +               /* clock driver will fail if the clock is less than the lowest source clock
> +                * divided by the internal clock divider. Test for the lowest available
> +                * clock and set the f_min freq to clock / clock divider. If we fail, set
> +                * it to the downstream hardcoded value.
> +                */
> +               ret = clk_round_rate(host->ciu_clk, MMC_MAX_INIT_FREQ * RK3288_CLKGEN_DIV);
> +               if (ret < 0) {
> +                       dev_warn(host->dev, "mmc safe rate failed: %d\n", ret);
> +                       mmc->f_min = RK3288_MIN_INIT_FREQ;
> +               } else {
> +                       mmc->f_min = ret / RK3288_CLKGEN_DIV;
> +               }
> +       }
> +
>         return 0;
>  }
>

Kind regards
Uffe