pinctrl: qcom: Enable device links to consumers

Message ID 20190523080654.19155-1-linus.walleij@linaro.org
State New
Headers show
Series
  • pinctrl: qcom: Enable device links to consumers
Related show

Commit Message

Linus Walleij May 23, 2019, 8:06 a.m.
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

Comments

Stephen Boyd May 23, 2019, 5:52 p.m. | #1
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?
Linus Walleij May 23, 2019, 7:26 p.m. | #2
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
Stephen Boyd May 23, 2019, 11:16 p.m. | #3
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!
Linus Walleij May 24, 2019, 8:05 a.m. | #4
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

Patch

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)) {