Re: [PATCH] mmc: host: dw-mmc-rockchip: avoid logspam when cd-broken

[Peter Geis]

On Tue, Mar 1, 2022 at 7:46 AM Peter Geis <pgwipeout@xxxxxxxxx> wrote:
>
> On Tue, Mar 1, 2022 at 7:38 AM Robin Murphy <robin.murphy@xxxxxxx> wrote:
> >
> > On 2022-03-01 11:49, Peter Geis wrote:
> > > On Tue, Mar 1, 2022 at 6:23 AM Robin Murphy <robin.murphy@xxxxxxx> wrote:
> > >>
> > >> On 2022-02-28 22:36, Peter Geis wrote:
> > >>> The dw_mmc-rockchip driver drops a large amound of logspam constantly
> > >>> when the cd-broken flag is enabled.
> > >>> Set the warning to be debug ratelimited in this case.
> > >>
> > >> Isn't this just papering over some fundamental problem with the clock?
> > >> If it's failing to set the expected rate for communicating with a card,
> > >> then presumably that's an issue for correct operation in general? The
> > >> fact that polling for a card makes a lot more of that communication
> > >> happen seems unrelated :/
> > >
> > > Good Morning,
> > >
> > > This only happens when a card is not inserted, so communication cannot happen.
> >
> > Well, I suppose there's a philosophical question in there about whether
> > shouting into the void counts as "communication", but AFAIR what the
> > polling function does is power up the controller, send a command, and
> > see if it gets a response.
> >
> > If the clock can't be set to the proper rate for low-speed discovery,
> > some or all cards may not be detected properly. Conversely if it is
> > already at a slow enough rate for discovery but can't be set higher once
> > a proper communication mode has been established, data transfer
> > performance will be terrible. Either way, it is not OK in general for
> > clk_set_rate() to fail, hence the warning. You have a clock driver problem.
>
> Alright, I'll look into this.
> It seems only extremely low clock speeds fail and I know rockchip
> chips have a hard time with extremely low clock rates.
> I'll trace out where the failure is happening.

Okay, I hope you can provide me a direction to go from here, because
it looks like it's doing exactly what it should do in this situation.
mmc core is requesting a rate (200k/100k).
clk core tries to find a parent to provide a clock that low and fails,
because the lowest possible parent is 750k.
clk_sdmmc(x) is listed as no-div, so it can't go any lower.

It seems to me that this error is sane, because other results of
einval you want to catch.
But einval in this case is fine, because
The thing that strikes me weird is currently clk_core thinks the
lowest possible freq here is 0, when in actuality it should be 750k,
am I correct here?
The mmc controller has an internal divider, so if my line of thinking
is correct here we should be more flexible here and request a rate
that's acceptable rather than just failing if it doesn't work.
But that's based on my limited understanding of how mmc core is
requesting this and what it expects in return.

>
> Thanks!
>
> >
> > Cheers,
> > Robin.
> >
> > > I found it while lighting off the SoQuartz module.
> > > As it is pin compatible with the RPi CM4, and the CM4 does not have a
> > > card detect line, sdmmc is non functional without cd-broken.
> > > This led to the fun spew when there wasn't a card inserted as this
> > > function is called every poll tick.
> > >
> > > Thanks,
> > > Peter
> > >
> > >>
> > >> Robin.
> > >>
> > >>> Signed-off-by: Peter Geis <pgwipeout@xxxxxxxxx>
> > >>> ---
> > >>>    drivers/mmc/host/dw_mmc-rockchip.c | 9 +++++++--
> > >>>    1 file changed, 7 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> > >>> index 95d0ec0f5f3a..d0ebf0afa42a 100644
> > >>> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > >>> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > >>> @@ -50,8 +50,13 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > >>>                cclkin = ios->clock * RK3288_CLKGEN_DIV;
> > >>>
> > >>>        ret = clk_set_rate(host->ciu_clk, cclkin);
> > >>> -     if (ret)
> > >>> -             dev_warn(host->dev, "failed to set rate %uHz\n", ios->clock);
> > >>> +     if (ret) {
> > >>> +             /* this screams when card detection is broken */
> > >>> +             if (host->slot->mmc->caps & MMC_CAP_NEEDS_POLL)
> > >>> +                     dev_dbg_ratelimited(host->dev, "failed to set rate %uHz\n", ios->clock);
> > >>> +             else
> > >>> +                     dev_warn(host->dev, "failed to set rate %uHz\n", ios->clock);
> > >>> +     }
> > >>>
> > >>>        bus_hz = clk_get_rate(host->ciu_clk) / RK3288_CLKGEN_DIV;
> > >>>        if (bus_hz != host->bus_hz) {