* [PATCH] pinctrl: Enforce device links
@ 2019-12-13 13:20 Linus Walleij
0 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2019-12-13 13:20 UTC (permalink / raw)
To: linux-gpio
Cc: Linus Walleij, Benjamin GAIGNARD, Rafael J . Wysocki, Ulf Hansson
Instead of opt-in device links, enforce it across the board.
Everyone probably needs this anyway, lest runtime[_pm] suspend
order will be haphazard.
Cc: Benjamin GAIGNARD <benjamin.gaignard@st.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Create links per setting (since settings can be on different
pin controllers) when adding a setting to a pin control handle,
instead of doing this when activating the state.
- Store the link inside the setting.
- Delete the link when we free up the setting.
Benjamin: it would be GREAT if you could test this!
---
drivers/pinctrl/core.c | 35 +++++++++++++++++----------
drivers/pinctrl/core.h | 3 +++
drivers/pinctrl/pinctrl-stmfx.c | 1 -
drivers/pinctrl/stm32/pinctrl-stm32.c | 1 -
include/linux/pinctrl/pinctrl.h | 5 ----
5 files changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2bbd8ee93507..89ac42a145f2 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1000,6 +1000,26 @@ static int add_setting(struct pinctrl *p, struct pinctrl_dev *pctldev,
list_add_tail(&setting->node, &state->settings);
+ /*
+ * Set up a device link for power management.
+ * Do not link hogs (circular dependency)
+ * Do not create more than one link per pinctrl.
+ */
+ if (p->dev != setting->pctldev->dev) {
+ /*
+ * Create a device link to the consumer such that
+ * it will enforce that runtime PM suspend/resume
+ * is done first on consumers before we get to
+ * the pin controller itself. As some devices get
+ * their pin control state even before probe() it is
+ * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
+ */
+ setting->dl = device_link_add(p->dev,
+ setting->pctldev->dev,
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_AUTOREMOVE_CONSUMER);
+ }
+
return 0;
}
@@ -1135,6 +1155,8 @@ EXPORT_SYMBOL_GPL(pinctrl_get);
static void pinctrl_free_setting(bool disable_setting,
struct pinctrl_setting *setting)
{
+ if (setting->dl)
+ device_link_del(setting->dl);
switch (setting->type) {
case PIN_MAP_TYPE_MUX_GROUP:
if (disable_setting)
@@ -1220,15 +1242,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
}
EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
-static void pinctrl_link_add(struct pinctrl_dev *pctldev,
- struct device *consumer)
-{
- if (pctldev->desc->link_consumers)
- device_link_add(consumer, pctldev->dev,
- DL_FLAG_PM_RUNTIME |
- DL_FLAG_AUTOREMOVE_CONSUMER);
-}
-
/**
* pinctrl_commit_state() - select/activate/program a pinctrl state to HW
* @p: the pinctrl handle for the device that requests configuration
@@ -1274,10 +1287,6 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
if (ret < 0) {
goto unapply_new_state;
}
-
- /* Do not link hogs (circular dependency) */
- if (p != setting->pctldev->p)
- pinctrl_link_add(setting->pctldev, p->dev);
}
p->state = state;
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 7f34167a0405..4073a1e4dde1 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -11,6 +11,7 @@
#include <linux/kref.h>
#include <linux/mutex.h>
#include <linux/radix-tree.h>
+#include <linux/device.h>
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/machine.h>
@@ -126,6 +127,7 @@ struct pinctrl_setting_configs {
* @pctldev: pin control device handling to be programmed. Not used for
* PIN_MAP_TYPE_DUMMY_STATE.
* @dev_name: the name of the device using this state
+ * @dl: device link for power management
* @data: Data specific to the setting type
*/
struct pinctrl_setting {
@@ -133,6 +135,7 @@ struct pinctrl_setting {
enum pinctrl_map_type type;
struct pinctrl_dev *pctldev;
const char *dev_name;
+ struct device_link *dl;
union {
struct pinctrl_setting_mux mux;
struct pinctrl_setting_configs configs;
diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
index 16723797fa7c..4306b8444188 100644
--- a/drivers/pinctrl/pinctrl-stmfx.c
+++ b/drivers/pinctrl/pinctrl-stmfx.c
@@ -638,7 +638,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev)
pctl->pctl_desc.pins = stmfx_pins;
pctl->pctl_desc.npins = ARRAY_SIZE(stmfx_pins);
pctl->pctl_desc.owner = THIS_MODULE;
- pctl->pctl_desc.link_consumers = true;
ret = devm_pinctrl_register_and_init(pctl->dev, &pctl->pctl_desc,
pctl, &pctl->pctl_dev);
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 2d5e0435af0a..ec59a58600ce 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -1439,7 +1439,6 @@ int stm32_pctl_probe(struct platform_device *pdev)
pctl->pctl_desc.owner = THIS_MODULE;
pctl->pctl_desc.pins = pins;
pctl->pctl_desc.npins = pctl->npins;
- pctl->pctl_desc.link_consumers = true;
pctl->pctl_desc.confops = &stm32_pconf_ops;
pctl->pctl_desc.pctlops = &stm32_pctrl_ops;
pctl->pctl_desc.pmxops = &stm32_pmx_ops;
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 7ce23450a1cb..c6159f041f4e 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -122,10 +122,6 @@ struct pinctrl_ops {
* the hardware description
* @custom_conf_items: Information how to print @params in debugfs, must be
* the same size as the @custom_params, i.e. @num_custom_params
- * @link_consumers: If true create a device link between pinctrl and its
- * consumers (i.e. the devices requesting pin control states). This is
- * sometimes necessary to ascertain the right suspend/resume order for
- * example.
*/
struct pinctrl_desc {
const char *name;
@@ -140,7 +136,6 @@ struct pinctrl_desc {
const struct pinconf_generic_params *custom_params;
const struct pin_config_item *custom_conf_items;
#endif
- bool link_consumers;
};
/* External interface to pin controller */
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] pinctrl: Enforce device links
2019-12-12 13:47 ` Ulf Hansson
@ 2019-12-12 15:29 ` Benjamin GAIGNARD
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin GAIGNARD @ 2019-12-12 15:29 UTC (permalink / raw)
To: Ulf Hansson; +Cc: Linus Walleij, open list:GPIO SUBSYSTEM, Rafael J . Wysocki
On 12/12/19 2:47 PM, Ulf Hansson wrote:
> On Thu, 12 Dec 2019 at 14:19, Benjamin GAIGNARD
> <benjamin.gaignard@st.com> wrote:
>>
>> On 12/12/19 11:56 AM, Ulf Hansson wrote:
>>> + Benjamin
>>>
>>> On Thu, 12 Dec 2019 at 11:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> Instead of opt-in device links, enforce it across the board.
>>>> Everyone probably needs this anyway, lest runtime[_pm] suspend
>>>> order will be haphazard.
>>>>
>>>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>>> ---
>>>> As there is no progress on opting in drivers for this we can just
>>>> enforce it.
>>>>
>>>> I am a bit concerned that with every pin control state change the
>>>> link reference count will just go up, but does it really matter?
>>>> ---
>>>> drivers/pinctrl/core.c | 25 ++++++++++++++-----------
>>>> drivers/pinctrl/pinctrl-stmfx.c | 1 -
>>>> drivers/pinctrl/stm32/pinctrl-stm32.c | 1 -
>>>> include/linux/pinctrl/pinctrl.h | 5 -----
>>>> 4 files changed, 14 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>>>> index 2bbd8ee93507..1d2cdeebb316 100644
>>>> --- a/drivers/pinctrl/core.c
>>>> +++ b/drivers/pinctrl/core.c
>>>> @@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
>>>> }
>>>> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>>>>
>>>> -static void pinctrl_link_add(struct pinctrl_dev *pctldev,
>>>> - struct device *consumer)
>>>> -{
>>>> - if (pctldev->desc->link_consumers)
>>>> - device_link_add(consumer, pctldev->dev,
>>>> - DL_FLAG_PM_RUNTIME |
>>>> - DL_FLAG_AUTOREMOVE_CONSUMER);
>>>> -}
>>>> -
>>>> /**
>>>> * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
>>>> * @p: the pinctrl handle for the device that requests configuration
>>>> @@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>>>> }
>>>>
>>>> /* Do not link hogs (circular dependency) */
>>>> - if (p != setting->pctldev->p)
>>>> - pinctrl_link_add(setting->pctldev, p->dev);
>>>> + if (p != setting->pctldev->p) {
>>>> + /*
>>>> + * Create a device link to the consumer such that
>>>> + * it will enforce that runtime PM suspend/resume
>>>> + * is done first on consumers before we get to
>>>> + * the pin controller itself. As some devices get
>>>> + * their pin control state even before probe() it is
>>>> + * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
>>>> + */
>>>> + device_link_add(p->dev,
>>>> + setting->pctldev->dev,
>>>> + DL_FLAG_PM_RUNTIME |
>>>> + DL_FLAG_AUTOREMOVE_CONSUMER);
>>> I understand DL_FLAG_PM_RUNTIME is used even prior $subject patch, but
>>> could you please explain some of the reasons behind that?
> Can you please clarify on this part as well?
It is to indicate to PM runtime framework that it have to use the link
so suspend/resume between consumer and producer is well ordered.
That was the primary goal of this patch.
>
>>> In regards to adding a new link every time you commit/select a new
>>> state, that sounds wrong to me. Why are we doing this?
>> If a link already exists it will not add new one so it safe for me.
> Right, but it seems silly to walk the list of links to find out that
> it already exist and then bail out.
>
> The point is, it adds unnecessary overhead, every time there is a new
> state being selected. Don't you think?
>
>>> Instead, don't you want to add a link when the consumer looks up the
>>> pinctrl cookie/handle (devm_pinctrl_get()), thus when create_pinctrl()
>>> is called?
>> Because it was the only place I add found where I get a the same
>> time pointers on the consumer and the producer devices.
> At least create_pinctrl() have the consumer device, but I am pretty
> sure we can lookup the producer device from there, in some way. Linus?
>
>>> Additionally, I didn't find any place where the link is removed. I
>>> looks like that should happen when the consumer drops the reference
>>> for the pinctrl cookie, no?
>> The link is automatically removed when the consumer device is deleted
>>
>> thanks to DL_FLAG_AUTOREMOVE_CONSUMER flag.
> Ahh, I see. That should be fine, I guess.
>
> [...]
>
> Kind regards
> Uffe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pinctrl: Enforce device links
2019-12-12 13:19 ` Benjamin GAIGNARD
@ 2019-12-12 13:47 ` Ulf Hansson
2019-12-12 15:29 ` Benjamin GAIGNARD
0 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2019-12-12 13:47 UTC (permalink / raw)
To: Benjamin GAIGNARD
Cc: Linus Walleij, open list:GPIO SUBSYSTEM, Rafael J . Wysocki
On Thu, 12 Dec 2019 at 14:19, Benjamin GAIGNARD
<benjamin.gaignard@st.com> wrote:
>
>
> On 12/12/19 11:56 AM, Ulf Hansson wrote:
> > + Benjamin
> >
> > On Thu, 12 Dec 2019 at 11:11, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> Instead of opt-in device links, enforce it across the board.
> >> Everyone probably needs this anyway, lest runtime[_pm] suspend
> >> order will be haphazard.
> >>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> >> ---
> >> As there is no progress on opting in drivers for this we can just
> >> enforce it.
> >>
> >> I am a bit concerned that with every pin control state change the
> >> link reference count will just go up, but does it really matter?
> >> ---
> >> drivers/pinctrl/core.c | 25 ++++++++++++++-----------
> >> drivers/pinctrl/pinctrl-stmfx.c | 1 -
> >> drivers/pinctrl/stm32/pinctrl-stm32.c | 1 -
> >> include/linux/pinctrl/pinctrl.h | 5 -----
> >> 4 files changed, 14 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> >> index 2bbd8ee93507..1d2cdeebb316 100644
> >> --- a/drivers/pinctrl/core.c
> >> +++ b/drivers/pinctrl/core.c
> >> @@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
> >> }
> >> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
> >>
> >> -static void pinctrl_link_add(struct pinctrl_dev *pctldev,
> >> - struct device *consumer)
> >> -{
> >> - if (pctldev->desc->link_consumers)
> >> - device_link_add(consumer, pctldev->dev,
> >> - DL_FLAG_PM_RUNTIME |
> >> - DL_FLAG_AUTOREMOVE_CONSUMER);
> >> -}
> >> -
> >> /**
> >> * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
> >> * @p: the pinctrl handle for the device that requests configuration
> >> @@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
> >> }
> >>
> >> /* Do not link hogs (circular dependency) */
> >> - if (p != setting->pctldev->p)
> >> - pinctrl_link_add(setting->pctldev, p->dev);
> >> + if (p != setting->pctldev->p) {
> >> + /*
> >> + * Create a device link to the consumer such that
> >> + * it will enforce that runtime PM suspend/resume
> >> + * is done first on consumers before we get to
> >> + * the pin controller itself. As some devices get
> >> + * their pin control state even before probe() it is
> >> + * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
> >> + */
> >> + device_link_add(p->dev,
> >> + setting->pctldev->dev,
> >> + DL_FLAG_PM_RUNTIME |
> >> + DL_FLAG_AUTOREMOVE_CONSUMER);
> > I understand DL_FLAG_PM_RUNTIME is used even prior $subject patch, but
> > could you please explain some of the reasons behind that?
Can you please clarify on this part as well?
> >
> > In regards to adding a new link every time you commit/select a new
> > state, that sounds wrong to me. Why are we doing this?
>
> If a link already exists it will not add new one so it safe for me.
Right, but it seems silly to walk the list of links to find out that
it already exist and then bail out.
The point is, it adds unnecessary overhead, every time there is a new
state being selected. Don't you think?
>
> >
> > Instead, don't you want to add a link when the consumer looks up the
> > pinctrl cookie/handle (devm_pinctrl_get()), thus when create_pinctrl()
> > is called?
> Because it was the only place I add found where I get a the same
> time pointers on the consumer and the producer devices.
At least create_pinctrl() have the consumer device, but I am pretty
sure we can lookup the producer device from there, in some way. Linus?
>
> >
> > Additionally, I didn't find any place where the link is removed. I
> > looks like that should happen when the consumer drops the reference
> > for the pinctrl cookie, no?
>
> The link is automatically removed when the consumer device is deleted
>
> thanks to DL_FLAG_AUTOREMOVE_CONSUMER flag.
Ahh, I see. That should be fine, I guess.
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pinctrl: Enforce device links
2019-12-12 10:56 ` Ulf Hansson
@ 2019-12-12 13:19 ` Benjamin GAIGNARD
2019-12-12 13:47 ` Ulf Hansson
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin GAIGNARD @ 2019-12-12 13:19 UTC (permalink / raw)
To: Ulf Hansson, Linus Walleij; +Cc: open list:GPIO SUBSYSTEM, Rafael J . Wysocki
On 12/12/19 11:56 AM, Ulf Hansson wrote:
> + Benjamin
>
> On Thu, 12 Dec 2019 at 11:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>> Instead of opt-in device links, enforce it across the board.
>> Everyone probably needs this anyway, lest runtime[_pm] suspend
>> order will be haphazard.
>>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> As there is no progress on opting in drivers for this we can just
>> enforce it.
>>
>> I am a bit concerned that with every pin control state change the
>> link reference count will just go up, but does it really matter?
>> ---
>> drivers/pinctrl/core.c | 25 ++++++++++++++-----------
>> drivers/pinctrl/pinctrl-stmfx.c | 1 -
>> drivers/pinctrl/stm32/pinctrl-stm32.c | 1 -
>> include/linux/pinctrl/pinctrl.h | 5 -----
>> 4 files changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>> index 2bbd8ee93507..1d2cdeebb316 100644
>> --- a/drivers/pinctrl/core.c
>> +++ b/drivers/pinctrl/core.c
>> @@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
>> }
>> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>>
>> -static void pinctrl_link_add(struct pinctrl_dev *pctldev,
>> - struct device *consumer)
>> -{
>> - if (pctldev->desc->link_consumers)
>> - device_link_add(consumer, pctldev->dev,
>> - DL_FLAG_PM_RUNTIME |
>> - DL_FLAG_AUTOREMOVE_CONSUMER);
>> -}
>> -
>> /**
>> * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
>> * @p: the pinctrl handle for the device that requests configuration
>> @@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>> }
>>
>> /* Do not link hogs (circular dependency) */
>> - if (p != setting->pctldev->p)
>> - pinctrl_link_add(setting->pctldev, p->dev);
>> + if (p != setting->pctldev->p) {
>> + /*
>> + * Create a device link to the consumer such that
>> + * it will enforce that runtime PM suspend/resume
>> + * is done first on consumers before we get to
>> + * the pin controller itself. As some devices get
>> + * their pin control state even before probe() it is
>> + * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
>> + */
>> + device_link_add(p->dev,
>> + setting->pctldev->dev,
>> + DL_FLAG_PM_RUNTIME |
>> + DL_FLAG_AUTOREMOVE_CONSUMER);
> I understand DL_FLAG_PM_RUNTIME is used even prior $subject patch, but
> could you please explain some of the reasons behind that?
>
> In regards to adding a new link every time you commit/select a new
> state, that sounds wrong to me. Why are we doing this?
If a link already exists it will not add new one so it safe for me.
>
> Instead, don't you want to add a link when the consumer looks up the
> pinctrl cookie/handle (devm_pinctrl_get()), thus when create_pinctrl()
> is called?
Because it was the only place I add found where I get a the same
time pointers on the consumer and the producer devices.
>
> Additionally, I didn't find any place where the link is removed. I
> looks like that should happen when the consumer drops the reference
> for the pinctrl cookie, no?
The link is automatically removed when the consumer device is deleted
thanks to DL_FLAG_AUTOREMOVE_CONSUMER flag.
Benjamin
>
>> + }
>> }
>>
>> p->state = state;
>> diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
>> index 16723797fa7c..4306b8444188 100644
>> --- a/drivers/pinctrl/pinctrl-stmfx.c
>> +++ b/drivers/pinctrl/pinctrl-stmfx.c
>> @@ -638,7 +638,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev)
>> pctl->pctl_desc.pins = stmfx_pins;
>> pctl->pctl_desc.npins = ARRAY_SIZE(stmfx_pins);
>> pctl->pctl_desc.owner = THIS_MODULE;
>> - pctl->pctl_desc.link_consumers = true;
>>
>> ret = devm_pinctrl_register_and_init(pctl->dev, &pctl->pctl_desc,
>> pctl, &pctl->pctl_dev);
>> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> index 2d5e0435af0a..ec59a58600ce 100644
>> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
>> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> @@ -1439,7 +1439,6 @@ int stm32_pctl_probe(struct platform_device *pdev)
>> pctl->pctl_desc.owner = THIS_MODULE;
>> pctl->pctl_desc.pins = pins;
>> pctl->pctl_desc.npins = pctl->npins;
>> - pctl->pctl_desc.link_consumers = true;
>> pctl->pctl_desc.confops = &stm32_pconf_ops;
>> pctl->pctl_desc.pctlops = &stm32_pctrl_ops;
>> pctl->pctl_desc.pmxops = &stm32_pmx_ops;
>> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
>> index 7ce23450a1cb..c6159f041f4e 100644
>> --- a/include/linux/pinctrl/pinctrl.h
>> +++ b/include/linux/pinctrl/pinctrl.h
>> @@ -122,10 +122,6 @@ struct pinctrl_ops {
>> * the hardware description
>> * @custom_conf_items: Information how to print @params in debugfs, must be
>> * the same size as the @custom_params, i.e. @num_custom_params
>> - * @link_consumers: If true create a device link between pinctrl and its
>> - * consumers (i.e. the devices requesting pin control states). This is
>> - * sometimes necessary to ascertain the right suspend/resume order for
>> - * example.
>> */
>> struct pinctrl_desc {
>> const char *name;
>> @@ -140,7 +136,6 @@ struct pinctrl_desc {
>> const struct pinconf_generic_params *custom_params;
>> const struct pin_config_item *custom_conf_items;
>> #endif
>> - bool link_consumers;
>> };
>>
>> /* External interface to pin controller */
>> --
>> 2.23.0
>>
> Kind regards
> Uffe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pinctrl: Enforce device links
2019-12-12 10:11 Linus Walleij
@ 2019-12-12 10:56 ` Ulf Hansson
2019-12-12 13:19 ` Benjamin GAIGNARD
0 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2019-12-12 10:56 UTC (permalink / raw)
To: Linus Walleij, Benjamin GAIGNARD
Cc: open list:GPIO SUBSYSTEM, Rafael J . Wysocki
+ Benjamin
On Thu, 12 Dec 2019 at 11:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Instead of opt-in device links, enforce it across the board.
> Everyone probably needs this anyway, lest runtime[_pm] suspend
> order will be haphazard.
>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> As there is no progress on opting in drivers for this we can just
> enforce it.
>
> I am a bit concerned that with every pin control state change the
> link reference count will just go up, but does it really matter?
> ---
> drivers/pinctrl/core.c | 25 ++++++++++++++-----------
> drivers/pinctrl/pinctrl-stmfx.c | 1 -
> drivers/pinctrl/stm32/pinctrl-stm32.c | 1 -
> include/linux/pinctrl/pinctrl.h | 5 -----
> 4 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 2bbd8ee93507..1d2cdeebb316 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
> }
> EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>
> -static void pinctrl_link_add(struct pinctrl_dev *pctldev,
> - struct device *consumer)
> -{
> - if (pctldev->desc->link_consumers)
> - device_link_add(consumer, pctldev->dev,
> - DL_FLAG_PM_RUNTIME |
> - DL_FLAG_AUTOREMOVE_CONSUMER);
> -}
> -
> /**
> * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
> * @p: the pinctrl handle for the device that requests configuration
> @@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
> }
>
> /* Do not link hogs (circular dependency) */
> - if (p != setting->pctldev->p)
> - pinctrl_link_add(setting->pctldev, p->dev);
> + if (p != setting->pctldev->p) {
> + /*
> + * Create a device link to the consumer such that
> + * it will enforce that runtime PM suspend/resume
> + * is done first on consumers before we get to
> + * the pin controller itself. As some devices get
> + * their pin control state even before probe() it is
> + * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
> + */
> + device_link_add(p->dev,
> + setting->pctldev->dev,
> + DL_FLAG_PM_RUNTIME |
> + DL_FLAG_AUTOREMOVE_CONSUMER);
I understand DL_FLAG_PM_RUNTIME is used even prior $subject patch, but
could you please explain some of the reasons behind that?
In regards to adding a new link every time you commit/select a new
state, that sounds wrong to me. Why are we doing this?
Instead, don't you want to add a link when the consumer looks up the
pinctrl cookie/handle (devm_pinctrl_get()), thus when create_pinctrl()
is called?
Additionally, I didn't find any place where the link is removed. I
looks like that should happen when the consumer drops the reference
for the pinctrl cookie, no?
> + }
> }
>
> p->state = state;
> diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
> index 16723797fa7c..4306b8444188 100644
> --- a/drivers/pinctrl/pinctrl-stmfx.c
> +++ b/drivers/pinctrl/pinctrl-stmfx.c
> @@ -638,7 +638,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev)
> pctl->pctl_desc.pins = stmfx_pins;
> pctl->pctl_desc.npins = ARRAY_SIZE(stmfx_pins);
> pctl->pctl_desc.owner = THIS_MODULE;
> - pctl->pctl_desc.link_consumers = true;
>
> ret = devm_pinctrl_register_and_init(pctl->dev, &pctl->pctl_desc,
> pctl, &pctl->pctl_dev);
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 2d5e0435af0a..ec59a58600ce 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -1439,7 +1439,6 @@ int stm32_pctl_probe(struct platform_device *pdev)
> pctl->pctl_desc.owner = THIS_MODULE;
> pctl->pctl_desc.pins = pins;
> pctl->pctl_desc.npins = pctl->npins;
> - pctl->pctl_desc.link_consumers = true;
> pctl->pctl_desc.confops = &stm32_pconf_ops;
> pctl->pctl_desc.pctlops = &stm32_pctrl_ops;
> pctl->pctl_desc.pmxops = &stm32_pmx_ops;
> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
> index 7ce23450a1cb..c6159f041f4e 100644
> --- a/include/linux/pinctrl/pinctrl.h
> +++ b/include/linux/pinctrl/pinctrl.h
> @@ -122,10 +122,6 @@ struct pinctrl_ops {
> * the hardware description
> * @custom_conf_items: Information how to print @params in debugfs, must be
> * the same size as the @custom_params, i.e. @num_custom_params
> - * @link_consumers: If true create a device link between pinctrl and its
> - * consumers (i.e. the devices requesting pin control states). This is
> - * sometimes necessary to ascertain the right suspend/resume order for
> - * example.
> */
> struct pinctrl_desc {
> const char *name;
> @@ -140,7 +136,6 @@ struct pinctrl_desc {
> const struct pinconf_generic_params *custom_params;
> const struct pin_config_item *custom_conf_items;
> #endif
> - bool link_consumers;
> };
>
> /* External interface to pin controller */
> --
> 2.23.0
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] pinctrl: Enforce device links
@ 2019-12-12 10:11 Linus Walleij
2019-12-12 10:56 ` Ulf Hansson
0 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2019-12-12 10:11 UTC (permalink / raw)
To: linux-gpio; +Cc: Linus Walleij, Rafael J . Wysocki, Ulf Hansson
Instead of opt-in device links, enforce it across the board.
Everyone probably needs this anyway, lest runtime[_pm] suspend
order will be haphazard.
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
As there is no progress on opting in drivers for this we can just
enforce it.
I am a bit concerned that with every pin control state change the
link reference count will just go up, but does it really matter?
---
drivers/pinctrl/core.c | 25 ++++++++++++++-----------
drivers/pinctrl/pinctrl-stmfx.c | 1 -
drivers/pinctrl/stm32/pinctrl-stm32.c | 1 -
include/linux/pinctrl/pinctrl.h | 5 -----
4 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2bbd8ee93507..1d2cdeebb316 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
}
EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
-static void pinctrl_link_add(struct pinctrl_dev *pctldev,
- struct device *consumer)
-{
- if (pctldev->desc->link_consumers)
- device_link_add(consumer, pctldev->dev,
- DL_FLAG_PM_RUNTIME |
- DL_FLAG_AUTOREMOVE_CONSUMER);
-}
-
/**
* pinctrl_commit_state() - select/activate/program a pinctrl state to HW
* @p: the pinctrl handle for the device that requests configuration
@@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
}
/* Do not link hogs (circular dependency) */
- if (p != setting->pctldev->p)
- pinctrl_link_add(setting->pctldev, p->dev);
+ if (p != setting->pctldev->p) {
+ /*
+ * Create a device link to the consumer such that
+ * it will enforce that runtime PM suspend/resume
+ * is done first on consumers before we get to
+ * the pin controller itself. As some devices get
+ * their pin control state even before probe() it is
+ * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
+ */
+ device_link_add(p->dev,
+ setting->pctldev->dev,
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_AUTOREMOVE_CONSUMER);
+ }
}
p->state = state;
diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
index 16723797fa7c..4306b8444188 100644
--- a/drivers/pinctrl/pinctrl-stmfx.c
+++ b/drivers/pinctrl/pinctrl-stmfx.c
@@ -638,7 +638,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev)
pctl->pctl_desc.pins = stmfx_pins;
pctl->pctl_desc.npins = ARRAY_SIZE(stmfx_pins);
pctl->pctl_desc.owner = THIS_MODULE;
- pctl->pctl_desc.link_consumers = true;
ret = devm_pinctrl_register_and_init(pctl->dev, &pctl->pctl_desc,
pctl, &pctl->pctl_dev);
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 2d5e0435af0a..ec59a58600ce 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -1439,7 +1439,6 @@ int stm32_pctl_probe(struct platform_device *pdev)
pctl->pctl_desc.owner = THIS_MODULE;
pctl->pctl_desc.pins = pins;
pctl->pctl_desc.npins = pctl->npins;
- pctl->pctl_desc.link_consumers = true;
pctl->pctl_desc.confops = &stm32_pconf_ops;
pctl->pctl_desc.pctlops = &stm32_pctrl_ops;
pctl->pctl_desc.pmxops = &stm32_pmx_ops;
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 7ce23450a1cb..c6159f041f4e 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -122,10 +122,6 @@ struct pinctrl_ops {
* the hardware description
* @custom_conf_items: Information how to print @params in debugfs, must be
* the same size as the @custom_params, i.e. @num_custom_params
- * @link_consumers: If true create a device link between pinctrl and its
- * consumers (i.e. the devices requesting pin control states). This is
- * sometimes necessary to ascertain the right suspend/resume order for
- * example.
*/
struct pinctrl_desc {
const char *name;
@@ -140,7 +136,6 @@ struct pinctrl_desc {
const struct pinconf_generic_params *custom_params;
const struct pin_config_item *custom_conf_items;
#endif
- bool link_consumers;
};
/* External interface to pin controller */
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-13 20:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-13 13:20 [PATCH] pinctrl: Enforce device links Linus Walleij
-- strict thread matches above, loose matches on Subject: below --
2019-12-12 10:11 Linus Walleij
2019-12-12 10:56 ` Ulf Hansson
2019-12-12 13:19 ` Benjamin GAIGNARD
2019-12-12 13:47 ` Ulf Hansson
2019-12-12 15:29 ` Benjamin GAIGNARD
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).