Hi,
I have a question about expectations when pinctrl setting is applied. In
DTS all nodes are described in the order available in DT.
uart-default {
mux {
...
};
conf {
...
};
};
I don't know if this standard description or not. I definitely see other
pinctrl drivers which are using different structure.
Anyway when overlay is applied the order has changed to
uart-default {
conf {
...
};
mux {
...
};
};
which is causing issue because pin is configured first via conf node
before it is requested via mux. This is something what firmware is
checking and error out.
That's why I want to check with you if this is issue with DT binding
description we use in zynqmp pinctrl driver posted here
https://lore.kernel.org/linux-arm-kernel/1613131643-60062-1-git-send-email-lakshmi.sai.krishna.potthuri@xilinx.com/
I have also tried to use init and default configuration where init is
called just with mux setting and then default is called just with config
but the issue is there as well because in pinctrl_commit_state()
previous state is checked and for MUXes pinmux_disable_setting() is
called which release a pin. And then configuration in default is called
but without requesting pin which fails for the same reason as above.
That's why my questions are:
Are we using incorrect DT description?
And is there a need sort subnodes in a way that mux should be called
first by core before configuration?
Or is there any different way how to do it?
Thanks,
Michal
+Frank, Rob, devicetree list
On 2/16/21 9:35 AM, Michal Simek wrote:
> Hi,
>
> I have a question about expectations when pinctrl setting is applied. In
> DTS all nodes are described in the order available in DT.
>
> uart-default {
> mux {
> ...
> };
>
> conf {
> ...
> };
> };
>
> I don't know if this standard description or not. I definitely see other
> pinctrl drivers which are using different structure.
>
> Anyway when overlay is applied the order has changed to
> uart-default {
> conf {
> ...
> };
>
> mux {
> ...
> };
> };
>
> which is causing issue because pin is configured first via conf node
> before it is requested via mux. This is something what firmware is
> checking and error out.
>
> That's why I want to check with you if this is issue with DT binding
> description we use in zynqmp pinctrl driver posted here
> https://lore.kernel.org/linux-arm-kernel/1613131643-60062-1-git-send-email-lakshmi.sai.krishna.potthuri@xilinx.com/
>
> I have also tried to use init and default configuration where init is
> called just with mux setting and then default is called just with config
> but the issue is there as well because in pinctrl_commit_state()
> previous state is checked and for MUXes pinmux_disable_setting() is
> called which release a pin. And then configuration in default is called
> but without requesting pin which fails for the same reason as above.
>
> That's why my questions are:
> Are we using incorrect DT description?
> And is there a need sort subnodes in a way that mux should be called
> first by core before configuration?
> Or is there any different way how to do it?
Node ordering and property ordering within a node are not defined
in the Linux kernel. If a subsystem or property is depending upon
a certain order, they must implement a method other than the
order as accessed by of_* functions. And as you noted, use of an
overlay may also change ordering.
-Frank
>
> Thanks,
> Michal
>
On Tue, Feb 16, 2021 at 4:35 PM Michal Simek <[email protected]> wrote:
> I have a question about expectations when pinctrl setting is applied. In
> DTS all nodes are described in the order available in DT.
>
> uart-default {
> mux {
> ...
> };
>
> conf {
> ...
> };
> };
>
> I don't know if this standard description or not. I definitely see other
> pinctrl drivers which are using different structure.
>
> Anyway when overlay is applied the order has changed to
> uart-default {
> conf {
> ...
> };
>
> mux {
> ...
> };
> };
>
> which is causing issue because pin is configured first via conf node
> before it is requested via mux. This is something what firmware is
> checking and error out.
As Frank says the DT ordering has no semantic meaning, it is essentially
a functional language, describes object relations not sequences.
The Linux kernel applies the mux and conf in that order because of how
the code is implemented (this order also makes a lot of sense for the
hardware). I would recommend to trace the execution of an overlay
being applied and try to find the reason conf goes before mux and fix
the bug there. I think it is a bug in how pinctrl handles DT overlays.
Yours,
Linus Walleij
Hi Linus,
On 3/1/21 10:19 AM, Linus Walleij wrote:
> On Tue, Feb 16, 2021 at 4:35 PM Michal Simek <[email protected]> wrote:
>
>> I have a question about expectations when pinctrl setting is applied. In
>> DTS all nodes are described in the order available in DT.
>>
>> uart-default {
>> mux {
>> ...
>> };
>>
>> conf {
>> ...
>> };
>> };
>>
>> I don't know if this standard description or not. I definitely see other
>> pinctrl drivers which are using different structure.
>>
>> Anyway when overlay is applied the order has changed to
>> uart-default {
>> conf {
>> ...
>> };
>>
>> mux {
>> ...
>> };
>> };
>>
>> which is causing issue because pin is configured first via conf node
>> before it is requested via mux. This is something what firmware is
>> checking and error out.
>
> As Frank says the DT ordering has no semantic meaning, it is essentially
> a functional language, describes object relations not sequences.
>
> The Linux kernel applies the mux and conf in that order because of how
> the code is implemented (this order also makes a lot of sense for the
> hardware). I would recommend to trace the execution of an overlay
> being applied and try to find the reason conf goes before mux and fix
> the bug there. I think it is a bug in how pinctrl handles DT overlays.
Does this mean that you prefer to fix how dt overlay applying instead of
fixing code to apply mux configs first before conf one?
Something like this? (just c&p patch below)
Thanks,
Michal
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 7d3370289938..cf56ee5d7e02 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1258,13 +1258,35 @@ static int pinctrl_commit_state(struct pinctrl
*p, struct pinctrl_state *state)
p->state = NULL;
- /* Apply all the settings for the new state */
+ /* Apply all the settings for the new state - pinmux first */
list_for_each_entry(setting, &state->settings, node) {
switch (setting->type) {
case PIN_MAP_TYPE_MUX_GROUP:
ret = pinmux_enable_setting(setting);
break;
case PIN_MAP_TYPE_CONFIGS_PIN:
+ case PIN_MAP_TYPE_CONFIGS_GROUP:
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ 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);
+ }
+
+ /* Apply all the settings for the new state - pinconf after */
+ list_for_each_entry(setting, &state->settings, node) {
+ switch (setting->type) {
+ case PIN_MAP_TYPE_MUX_GROUP:
+ break;
+ case PIN_MAP_TYPE_CONFIGS_PIN:
case PIN_MAP_TYPE_CONFIGS_GROUP:
ret = pinconf_apply_setting(setting);
break;
On Mon, Mar 1, 2021 at 10:31 AM Michal Simek <[email protected]> wrote:
> On 3/1/21 10:19 AM, Linus Walleij wrote:
> Does this mean that you prefer to fix how dt overlay applying instead of
> fixing code to apply mux configs first before conf one?
>
> Something like this? (just c&p patch below)
I think your patch looks correct! Can you make a proper patch,
make sure it solves your problem and submit to the mailing
list for review?
Yours,
Linus Walleij
On 3/1/21 3:13 PM, Linus Walleij wrote:
> On Mon, Mar 1, 2021 at 10:31 AM Michal Simek <[email protected]> wrote:
>> On 3/1/21 10:19 AM, Linus Walleij wrote:
>
>> Does this mean that you prefer to fix how dt overlay applying instead of
>> fixing code to apply mux configs first before conf one?
>>
>> Something like this? (just c&p patch below)
>
> I think your patch looks correct! Can you make a proper patch,
> make sure it solves your problem and submit to the mailing
> list for review?
I will check it on our platform and will send proper patch for it.
Thanks,
Michal