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

[Peter Geis]

On Thu, Mar 3, 2022 at 10:19 AM Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> On 2022-03-03 10:21, Ulf Hansson wrote:
> > On Thu, 3 Mar 2022 at 10:49, Peter Geis <pgwipeout@xxxxxxxxx> wrote:
> >>
> >> On Thu, Mar 3, 2022 at 2:53 AM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> >>>
> >>> 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.
> >>
> >> I thought so too, but while setting f_min did reduce the number of
> >> errors, it didn't stop them completely.
> >> Each tick I was getting three failures, it turns out mmc core tries
> >> anyways with 300000, 200000, and 100000.
> >> Clamping it here was necessary to stop these.
> >
> > Ohh, that was certainly a surprise to me. Unless the dw_mmc driver
> > invokes this path on it's own in some odd way, that means the mmc core
> > has a bug that we need to fix.
> >
> > Would you mind taking a stack trace or debug this so we understand in
> > what case the mmc core doesn't respect f_min? It really should.
>
> I'm only armed with grep and a hunch, but is dw_mci_init_slot_caps()
> stomping on the same f_min that we've set in the platform init hook?

I suspected this originally, but no, f_min remains intact, so it's
something else.
Also, this clamp wouldn't work if f_min got clobbered.

>
> Robin.