| Message ID | 20190523080654.19155-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: qcom: Enable device links to consumers | expand |
Quoting Linus Walleij (2019-05-23 01:06:54) > A recent core change makes it possible to create device links > between a pin controller and its consumers. This is necessary > to ascertain the right suspend/resume order for the devices: > if a device is using a certain pin control state and want > to switch that before/after going to suspend, then the pin > controller may not be suspended already, and conversely > on the resume path. > > Make sure any qcom pin control consumers are suspended before > the qcom pin control is suspended. > > Since Qualcomm is one of the few pin controllers implementing > suspend/resume I suppose you will see this problem sooner > or later so let's see if we can just fix it right now before > you run into it. > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Cc: Brian Masney <masneyb@onstation.org> > Cc: Lina Iyer <ilina@codeaurora.org> > Cc: Stephen Boyd <swboyd@chromium.org> > Cc: Benjamin Gaignard <benjamin.gaignard@st.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > You can test this patch by pulling in this branch: > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=consumer-links > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 1 + > 1 file changed, 1 insertion(+) I don't know how much it will matter for qcom right now. This pinctrl driver just forces over some suspend pins for the hogs during suspend, so it's not like drivers that are suspended after this moves hogs over will break, unless somehow the hogs change behavior of the pins those other drivers are using which doesn't seem possible. Also, what is the usecase for device links in pinctrl? Doesn't the driver core reorder the suspend list when probing devices so that devices that probe defer get moved later in list and thus suspended first? I can understand that runtime suspend may be important because order of suspend isn't fixed, but system suspend should be unaffected, right? > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index ee8119879c4c..d4a6edbccdb9 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -1139,6 +1139,7 @@ int msm_pinctrl_probe(struct platform_device *pdev, > pctrl->desc.name = dev_name(&pdev->dev); > pctrl->desc.pins = pctrl->soc->pins; > pctrl->desc.npins = pctrl->soc->npins; > + pctrl->desc.link_consumers = true; Why is it an opt-in flag instead of a mandated feature for all pinctrl providers?
On Thu, May 23, 2019 at 7:52 PM Stephen Boyd <swboyd@chromium.org> wrote: > Also, what is the usecase for device links in pinctrl? Most prominent is a device (such as a GPIO block or I2C block or something) that need to suspend before the pin controller itself suspend()s. > Doesn't the > driver core reorder the suspend list when probing devices so that > devices that probe defer get moved later in list and thus suspended > first? AFAIK it does not, the device links however can do this so that the probe order does not have to use deferral at all. But the links can also be added later at runtime (like when pin control states are requested by drivers) and that is what this patch does. By way of the chicken-and-egg problem we cannot really use these device links much for probe ordering, but they can readily be used to control suspend/resume sequencing like this. >I can understand that runtime suspend may be important because > order of suspend isn't fixed, but system suspend should be unaffected, > right? AFAIK both runtime PM and system suspend use the device links, this was implemented especially for system suspend/resume and tested with the STMFX driver on STM32. > > pctrl->desc.npins = pctrl->soc->npins; > > + pctrl->desc.link_consumers = true; > > Why is it an opt-in flag instead of a mandated feature for all pinctrl > providers? I am afraid of breaking stuff. (OK maybe I am chicken...) We slammed in device links in the DRM core and it exploded in our face. Because of fear of causing a similar debacle and having to back out all drivers that definately need this, I am making it opt-in for the moment. Once we have a feeling that this is not breaking (on e.g. qcom) we might just make it default to link all devices getting pinctrl states to their pin controllers. Yours, Linus Walleij
Quoting Linus Walleij (2019-05-23 12:26:20) > On Thu, May 23, 2019 at 7:52 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > Also, what is the usecase for device links in pinctrl? > > Most prominent is a device (such as a GPIO block or I2C > block or something) that need to suspend before the pin > controller itself suspend()s. And this i2c block or GPIO block is using the pins from this pin controller? Wouldn't the i2c or GPIO block have probe deferred if this pin controller wasn't providing the pins yet? > > > Doesn't the > > driver core reorder the suspend list when probing devices so that > > devices that probe defer get moved later in list and thus suspended > > first? > > AFAIK it does not, the device links however can do this so that > the probe order does not have to use deferral at all. I'm not too worried about using device links for probe ordering. I'm saying that this probe defer logic in drivers/base/dd.c already takes care of reordering the list so suspend/resume will be correct. /* * Force the device to the end of the dpm_list since * the PM code assumes that the order we add things to * the list is a good order for suspend but deferred * probe makes that very unsafe. */ device_pm_move_to_tail(dev); Basically all providers will come before consumers and then suspend will run through the list in reverse and suspend consumers first before providers. > But the > links can also be added later at runtime (like when pin control > states are requested by drivers) and that is what this patch > does. Ok, great! So if some consumer is only informing the provider of the driver dependency after probe succeeds then this will help by fixing the list order. This is what I'm looking for. It's unfortunate that devices aren't getting all resources in probe so this could be avoided. > > By way of the chicken-and-egg problem we cannot really use > these device links much for probe ordering, but they can > readily be used to control suspend/resume sequencing > like this. > > >I can understand that runtime suspend may be important because > > order of suspend isn't fixed, but system suspend should be unaffected, > > right? > > AFAIK both runtime PM and system suspend use the device > links, this was implemented especially for system suspend/resume > and tested with the STMFX driver on STM32. Yes that's my understanding too. > > > > pctrl->desc.npins = pctrl->soc->npins; > > > + pctrl->desc.link_consumers = true; > > > > Why is it an opt-in flag instead of a mandated feature for all pinctrl > > providers? > > I am afraid of breaking stuff. (OK maybe I am chicken...) > > We slammed in device links in the DRM core and it exploded in > our face. Because of fear of causing a similar debacle and > having to back out all drivers that definately need this, > I am making it opt-in for the moment. > > Once we have a feeling that this is not breaking (on e.g. > qcom) we might just make it default to link all devices > getting pinctrl states to their pin controllers. > Alright. Thanks!
On Fri, May 24, 2019 at 1:16 AM Stephen Boyd <swboyd@chromium.org> wrote: > Quoting Linus Walleij (2019-05-23 12:26:20) > > On Thu, May 23, 2019 at 7:52 PM Stephen Boyd <swboyd@chromium.org> wrote: > > AFAIK it does not, the device links however can do this so that > > the probe order does not have to use deferral at all. > > I'm not too worried about using device links for probe ordering. I'm > saying that this probe defer logic in drivers/base/dd.c already takes > care of reordering the list so suspend/resume will be correct. > > /* > * Force the device to the end of the dpm_list since > * the PM code assumes that the order we add things to > * the list is a good order for suspend but deferred > * probe makes that very unsafe. > */ > device_pm_move_to_tail(dev); > > Basically all providers will come before consumers and then suspend will > run through the list in reverse and suspend consumers first before > providers. Okay! News2me. :) and very good and intuitive in a way. > > But the > > links can also be added later at runtime (like when pin control > > states are requested by drivers) and that is what this patch > > does. > > Ok, great! So if some consumer is only informing the provider of the > driver dependency after probe succeeds then this will help by fixing the > list order. This is what I'm looking for. It's unfortunate that devices > aren't getting all resources in probe so this could be avoided. Yeah :/ I'm still figuring out the details because device links are new to me: not many callers in the kernel, but the regulators are using it so it seems like the way to go. I will probably have to add something similar for GPIO descriptors as for pin control handles so the GPIO providers get a strict suspend/resume order too. > > >I can understand that runtime suspend may be important because > > > order of suspend isn't fixed, but system suspend should be unaffected, > > > right? > > > > AFAIK both runtime PM and system suspend use the device > > links, this was implemented especially for system suspend/resume > > and tested with the STMFX driver on STM32. > > Yes that's my understanding too. I'm unsure if it's actually the reuse of runtime PM runtime suspend/resume path for the main suspend/resume path that is causing this behaviour. It's good if they both do it the same way. The flag DL_FLAG_PM_RUNTIME should maybe be renamed DL_FLAG_PM but Rafael and Ulf knows these semantics better. Yours, Linus Walleij
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index ee8119879c4c..d4a6edbccdb9 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -1139,6 +1139,7 @@ int msm_pinctrl_probe(struct platform_device *pdev, pctrl->desc.name = dev_name(&pdev->dev); pctrl->desc.pins = pctrl->soc->pins; pctrl->desc.npins = pctrl->soc->npins; + pctrl->desc.link_consumers = true; pctrl->pctrl = devm_pinctrl_register(&pdev->dev, &pctrl->desc, pctrl); if (IS_ERR(pctrl->pctrl)) {
A recent core change makes it possible to create device links between a pin controller and its consumers. This is necessary to ascertain the right suspend/resume order for the devices: if a device is using a certain pin control state and want to switch that before/after going to suspend, then the pin controller may not be suspended already, and conversely on the resume path. Make sure any qcom pin control consumers are suspended before the qcom pin control is suspended. Since Qualcomm is one of the few pin controllers implementing suspend/resume I suppose you will see this problem sooner or later so let's see if we can just fix it right now before you run into it. Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Cc: Brian Masney <masneyb@onstation.org> Cc: Lina Iyer <ilina@codeaurora.org> Cc: Stephen Boyd <swboyd@chromium.org> Cc: Benjamin Gaignard <benjamin.gaignard@st.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- You can test this patch by pulling in this branch: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=consumer-links --- drivers/pinctrl/qcom/pinctrl-msm.c | 1 + 1 file changed, 1 insertion(+) -- 2.20.1