| Message ID | 20200902164303.1.I5e598a25222b4534c0083b61dbfa4e0e76f66171@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8c98644bfc45516c81dadb99ebd0f8461688add3 |
| Headers | show |
| Series | mmc: sdhci-msm: Prefer asynchronous probe | expand |
On Thu, 3 Sep 2020 at 01:43, Douglas Anderson <dianders@chromium.org> wrote: > > Turning on initcall debug on one system showed this: > initcall sdhci_msm_driver_init+0x0/0x28 returned 0 after 34782 usecs > > The lion's share of this time (~33 ms) was in mmc_power_up(). This > shouldn't be terribly surprising since there are a few calls to delay > based on "power_delay_ms" and the default delay there is 10 ms. > > Because we haven't specified that we'd prefer asynchronous probe for > this driver then we'll wait for this driver to finish before we start > probes for more drivers. While 33 ms doesn't sound like tons, every > little bit counts. > > There should be little problem with turning on asynchronous probe for > this driver. It's already possible that previous drivers may have > turned on asynchronous probe so we might already have other things > (that probed before us) probing at the same time we are anyway. This > driver isn't really providing resources (clocks, regulators, etc) that > other drivers need to probe and even if it was they should be handling > -EPROBE_DEFER. > > Let's turn this on and get a bit of boot speed back. Thanks for a very well written commit message! Indeed, I am sure many mmc host drivers could benefit from a similar change. At least regular platform drivers and amba drivers are pretty sure to work, but who knows. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Applied for next, thanks! Kind regards Uffe > --- > > drivers/mmc/host/sdhci-msm.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c > index d4c02884cca8..9dd0dbb65382 100644 > --- a/drivers/mmc/host/sdhci-msm.c > +++ b/drivers/mmc/host/sdhci-msm.c > @@ -2542,6 +2542,7 @@ static struct platform_driver sdhci_msm_driver = { > .name = "sdhci_msm", > .of_match_table = sdhci_msm_dt_match, > .pm = &sdhci_msm_pm_ops, > + .probe_type = PROBE_PREFER_ASYNCHRONOUS, > }, > }; > > -- > 2.28.0.402.g5ffc5be6b7-goog >
Hi, On Thu, Sep 3, 2020 at 1:10 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 3 Sep 2020 at 01:43, Douglas Anderson <dianders@chromium.org> wrote: > > > > Turning on initcall debug on one system showed this: > > initcall sdhci_msm_driver_init+0x0/0x28 returned 0 after 34782 usecs > > > > The lion's share of this time (~33 ms) was in mmc_power_up(). This > > shouldn't be terribly surprising since there are a few calls to delay > > based on "power_delay_ms" and the default delay there is 10 ms. > > > > Because we haven't specified that we'd prefer asynchronous probe for > > this driver then we'll wait for this driver to finish before we start > > probes for more drivers. While 33 ms doesn't sound like tons, every > > little bit counts. > > > > There should be little problem with turning on asynchronous probe for > > this driver. It's already possible that previous drivers may have > > turned on asynchronous probe so we might already have other things > > (that probed before us) probing at the same time we are anyway. This > > driver isn't really providing resources (clocks, regulators, etc) that > > other drivers need to probe and even if it was they should be handling > > -EPROBE_DEFER. > > > > Let's turn this on and get a bit of boot speed back. > > Thanks for a very well written commit message! > > Indeed, I am sure many mmc host drivers could benefit from a similar > change. At least regular platform drivers and amba drivers are pretty > sure to work, but who knows. Yeah, and many non-mmc drivers can benefit too, which is why I've been sending several of these patches recently as I optimize boot perf on the device that's sitting in front of me. ;-) I think the idea was that eventually we'd want the kernel to just turn on async by default everywhere, but at the time the flag was introduced there were too many subtle bugs everywhere. It feels like one way to get to the point where we'd be confident that this is OK to turn on everywhere is to just start turning it on in lots of places. Once enough places have it on then perhaps that will give folks confidence that it's OK to turn on by default across the board. If you'd like, I can post patches to update some other set of MMC host drivers, either as one giant patch (hard to backport, but not as spammy) or as a large pile of patches. I've never played with coccinelle so I'd probably fall back to doing this by hand. I could probably only test on a small handful (I think I have easy access to dw-mmc-rockchip and sdhci-of-arasan besides the msm one I already posted), so another option is that I could also just post for those devices... ...or we can just hope others will notice and start posting similar patches themselves after testing. Let me know what you'd prefer. ;-) -Doug
On Thu, 3 Sep 2020 at 16:35, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Sep 3, 2020 at 1:10 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 3 Sep 2020 at 01:43, Douglas Anderson <dianders@chromium.org> wrote: > > > > > > Turning on initcall debug on one system showed this: > > > initcall sdhci_msm_driver_init+0x0/0x28 returned 0 after 34782 usecs > > > > > > The lion's share of this time (~33 ms) was in mmc_power_up(). This > > > shouldn't be terribly surprising since there are a few calls to delay > > > based on "power_delay_ms" and the default delay there is 10 ms. > > > > > > Because we haven't specified that we'd prefer asynchronous probe for > > > this driver then we'll wait for this driver to finish before we start > > > probes for more drivers. While 33 ms doesn't sound like tons, every > > > little bit counts. > > > > > > There should be little problem with turning on asynchronous probe for > > > this driver. It's already possible that previous drivers may have > > > turned on asynchronous probe so we might already have other things > > > (that probed before us) probing at the same time we are anyway. This > > > driver isn't really providing resources (clocks, regulators, etc) that > > > other drivers need to probe and even if it was they should be handling > > > -EPROBE_DEFER. > > > > > > Let's turn this on and get a bit of boot speed back. > > > > Thanks for a very well written commit message! > > > > Indeed, I am sure many mmc host drivers could benefit from a similar > > change. At least regular platform drivers and amba drivers are pretty > > sure to work, but who knows. > > Yeah, and many non-mmc drivers can benefit too, which is why I've been > sending several of these patches recently as I optimize boot perf on > the device that's sitting in front of me. ;-) I think the idea was > that eventually we'd want the kernel to just turn on async by default > everywhere, but at the time the flag was introduced there were too > many subtle bugs everywhere. It feels like one way to get to the > point where we'd be confident that this is OK to turn on everywhere is > to just start turning it on in lots of places. Once enough places > have it on then perhaps that will give folks confidence that it's OK > to turn on by default across the board. Yeah, I guess this is the only way forward at this point. > > If you'd like, I can post patches to update some other set of MMC host > drivers, either as one giant patch (hard to backport, but not as > spammy) or as a large pile of patches. I've never played with > coccinelle so I'd probably fall back to doing this by hand. I could > probably only test on a small handful (I think I have easy access to > dw-mmc-rockchip and sdhci-of-arasan besides the msm one I already > posted), so another option is that I could also just post for those > devices... ...or we can just hope others will notice and start > posting similar patches themselves after testing. Let me know what > you'd prefer. ;-) Honestly, I don't know. You go ahead with the option you prefer - then I will have a look. :-) Don't worry if we break some, as it should be rather easy to fix - as long as we keep an eye on it. Kind regards Uffe
Hi, On Thu, Sep 3, 2020 at 7:44 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 3 Sep 2020 at 16:35, Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Thu, Sep 3, 2020 at 1:10 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Thu, 3 Sep 2020 at 01:43, Douglas Anderson <dianders@chromium.org> wrote: > > > > > > > > Turning on initcall debug on one system showed this: > > > > initcall sdhci_msm_driver_init+0x0/0x28 returned 0 after 34782 usecs > > > > > > > > The lion's share of this time (~33 ms) was in mmc_power_up(). This > > > > shouldn't be terribly surprising since there are a few calls to delay > > > > based on "power_delay_ms" and the default delay there is 10 ms. > > > > > > > > Because we haven't specified that we'd prefer asynchronous probe for > > > > this driver then we'll wait for this driver to finish before we start > > > > probes for more drivers. While 33 ms doesn't sound like tons, every > > > > little bit counts. > > > > > > > > There should be little problem with turning on asynchronous probe for > > > > this driver. It's already possible that previous drivers may have > > > > turned on asynchronous probe so we might already have other things > > > > (that probed before us) probing at the same time we are anyway. This > > > > driver isn't really providing resources (clocks, regulators, etc) that > > > > other drivers need to probe and even if it was they should be handling > > > > -EPROBE_DEFER. > > > > > > > > Let's turn this on and get a bit of boot speed back. > > > > > > Thanks for a very well written commit message! > > > > > > Indeed, I am sure many mmc host drivers could benefit from a similar > > > change. At least regular platform drivers and amba drivers are pretty > > > sure to work, but who knows. > > > > Yeah, and many non-mmc drivers can benefit too, which is why I've been > > sending several of these patches recently as I optimize boot perf on > > the device that's sitting in front of me. ;-) I think the idea was > > that eventually we'd want the kernel to just turn on async by default > > everywhere, but at the time the flag was introduced there were too > > many subtle bugs everywhere. It feels like one way to get to the > > point where we'd be confident that this is OK to turn on everywhere is > > to just start turning it on in lots of places. Once enough places > > have it on then perhaps that will give folks confidence that it's OK > > to turn on by default across the board. > > Yeah, I guess this is the only way forward at this point. > > > > > If you'd like, I can post patches to update some other set of MMC host > > drivers, either as one giant patch (hard to backport, but not as > > spammy) or as a large pile of patches. I've never played with > > coccinelle so I'd probably fall back to doing this by hand. I could > > probably only test on a small handful (I think I have easy access to > > dw-mmc-rockchip and sdhci-of-arasan besides the msm one I already > > posted), so another option is that I could also just post for those > > devices... ...or we can just hope others will notice and start > > posting similar patches themselves after testing. Let me know what > > you'd prefer. ;-) > > Honestly, I don't know. You go ahead with the option you prefer - then > I will have a look. :-) > > Don't worry if we break some, as it should be rather easy to fix - as > long as we keep an eye on it. OK, I probably spent way too much time on it, but here it is in all of its glory: https://lore.kernel.org/r/20200903232441.2694866-1-dianders@chromium.org/ -Doug
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c index d4c02884cca8..9dd0dbb65382 100644 --- a/drivers/mmc/host/sdhci-msm.c +++ b/drivers/mmc/host/sdhci-msm.c @@ -2542,6 +2542,7 @@ static struct platform_driver sdhci_msm_driver = { .name = "sdhci_msm", .of_match_table = sdhci_msm_dt_match, .pm = &sdhci_msm_pm_ops, + .probe_type = PROBE_PREFER_ASYNCHRONOUS, }, };
Turning on initcall debug on one system showed this: initcall sdhci_msm_driver_init+0x0/0x28 returned 0 after 34782 usecs The lion's share of this time (~33 ms) was in mmc_power_up(). This shouldn't be terribly surprising since there are a few calls to delay based on "power_delay_ms" and the default delay there is 10 ms. Because we haven't specified that we'd prefer asynchronous probe for this driver then we'll wait for this driver to finish before we start probes for more drivers. While 33 ms doesn't sound like tons, every little bit counts. There should be little problem with turning on asynchronous probe for this driver. It's already possible that previous drivers may have turned on asynchronous probe so we might already have other things (that probed before us) probing at the same time we are anyway. This driver isn't really providing resources (clocks, regulators, etc) that other drivers need to probe and even if it was they should be handling -EPROBE_DEFER. Let's turn this on and get a bit of boot speed back. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/mmc/host/sdhci-msm.c | 1 + 1 file changed, 1 insertion(+)