| Message ID | 20220228223642.1136229-1-pgwipeout@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: host: dw-mmc-rockchip: avoid logspam when cd-broken | expand |
On Tue, Mar 1, 2022 at 6:23 AM Robin Murphy <robin.murphy@arm.com> 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. 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@gmail.com> > > --- > > 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) {
On 2022-03-01 11:49, Peter Geis wrote: > On Tue, Mar 1, 2022 at 6:23 AM Robin Murphy <robin.murphy@arm.com> 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. 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@gmail.com> >>> --- >>> 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) {
On 2022-03-01 14:49, Peter Geis wrote: > On Tue, Mar 1, 2022 at 7:46 AM Peter Geis <pgwipeout@gmail.com> wrote: >> >> On Tue, Mar 1, 2022 at 7:38 AM Robin Murphy <robin.murphy@arm.com> wrote: >>> >>> On 2022-03-01 11:49, Peter Geis wrote: >>>> On Tue, Mar 1, 2022 at 6:23 AM Robin Murphy <robin.murphy@arm.com> 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. The downstream solution appears to be just to clamp the rate for detection[1][2]. Not sure whether it's feasible to try to be cleverer with the local divider to settle on a more in-spec rate for the final output :/ Robin. [1] https://github.com/JeffyCN/mirrors/commit/d80d5062b22f9c4a559401bdb7b2727c4ced36c0 [2] https://github.com/JeffyCN/mirrors/commit/3f26edfb2392df25efc361ad0a9f41d0917e40ee
On Tue, Mar 1, 2022 at 8:04 PM Shawn Lin <shawn.lin@rock-chips.com> wrote: > > Hi Peter, > > 在 2022/3/1 6:36, Peter Geis 写道: > > 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. > > > > May I know which platform did you use? It's rk3566. > > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > > --- > > 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) { > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Tue, 1 Mar 2022 at 15:49, Peter Geis <pgwipeout@gmail.com> wrote: > > On Tue, Mar 1, 2022 at 7:46 AM Peter Geis <pgwipeout@gmail.com> wrote: > > > > On Tue, Mar 1, 2022 at 7:38 AM Robin Murphy <robin.murphy@arm.com> wrote: > > > > > > On 2022-03-01 11:49, Peter Geis wrote: > > > > On Tue, Mar 1, 2022 at 6:23 AM Robin Murphy <robin.murphy@arm.com> 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. The important point from the eMMC/SD/SDIO spec point of view, is that the initialization of the cards starts at a maximum of 400KHz for the bus interface clock. In many cases, that requires a combination of the provided source clock for the mmc controller to be decreased with some kind of an internal divider managed by the controller itself. Moreover, the mmc host driver shall set mmc->f_min in its corresponding struct mmc_host during ->probe(), to inform the core about its lowest possible rate that can be set - and this must not be higher than 400KHz. Hope that clarifies how things should work. [...] Kind regards Uffe
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) {
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. Signed-off-by: Peter Geis <pgwipeout@gmail.com> --- drivers/mmc/host/dw_mmc-rockchip.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)