* DT overlay applied via pinctrl description
@ 2021-02-16 15:35 Michal Simek
2021-02-17 5:33 ` Frank Rowand
2021-03-01 9:19 ` Linus Walleij
0 siblings, 2 replies; 6+ messages in thread
From: Michal Simek @ 2021-02-16 15:35 UTC (permalink / raw)
To: Linus Walleij
Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Sai Krishna Potthuri
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: DT overlay applied via pinctrl description 2021-02-16 15:35 DT overlay applied via pinctrl description Michal Simek @ 2021-02-17 5:33 ` Frank Rowand 2021-03-01 9:19 ` Linus Walleij 1 sibling, 0 replies; 6+ messages in thread From: Frank Rowand @ 2021-02-17 5:33 UTC (permalink / raw) To: Michal Simek, Linus Walleij Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Sai Krishna Potthuri, Frank Rowand, Rob Herring, devicetree +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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: DT overlay applied via pinctrl description 2021-02-16 15:35 DT overlay applied via pinctrl description Michal Simek 2021-02-17 5:33 ` Frank Rowand @ 2021-03-01 9:19 ` Linus Walleij 2021-03-01 9:31 ` Michal Simek 1 sibling, 1 reply; 6+ messages in thread From: Linus Walleij @ 2021-03-01 9:19 UTC (permalink / raw) To: Michal Simek Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Sai Krishna Potthuri On Tue, Feb 16, 2021 at 4:35 PM Michal Simek <michal.simek@xilinx.com> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: DT overlay applied via pinctrl description 2021-03-01 9:19 ` Linus Walleij @ 2021-03-01 9:31 ` Michal Simek 2021-03-01 14:13 ` Linus Walleij 0 siblings, 1 reply; 6+ messages in thread From: Michal Simek @ 2021-03-01 9:31 UTC (permalink / raw) To: Linus Walleij, Michal Simek Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Sai Krishna Potthuri Hi Linus, On 3/1/21 10:19 AM, Linus Walleij wrote: > On Tue, Feb 16, 2021 at 4:35 PM Michal Simek <michal.simek@xilinx.com> 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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: DT overlay applied via pinctrl description 2021-03-01 9:31 ` Michal Simek @ 2021-03-01 14:13 ` Linus Walleij 2021-03-01 15:04 ` Michal Simek 0 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2021-03-01 14:13 UTC (permalink / raw) To: Michal Simek Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Sai Krishna Potthuri On Mon, Mar 1, 2021 at 10:31 AM Michal Simek <michal.simek@xilinx.com> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: DT overlay applied via pinctrl description 2021-03-01 14:13 ` Linus Walleij @ 2021-03-01 15:04 ` Michal Simek 0 siblings, 0 replies; 6+ messages in thread From: Michal Simek @ 2021-03-01 15:04 UTC (permalink / raw) To: Linus Walleij, Michal Simek Cc: Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Sai Krishna Potthuri On 3/1/21 3:13 PM, Linus Walleij wrote: > On Mon, Mar 1, 2021 at 10:31 AM Michal Simek <michal.simek@xilinx.com> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-01 15:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-16 15:35 DT overlay applied via pinctrl description Michal Simek 2021-02-17 5:33 ` Frank Rowand 2021-03-01 9:19 ` Linus Walleij 2021-03-01 9:31 ` Michal Simek 2021-03-01 14:13 ` Linus Walleij 2021-03-01 15:04 ` Michal Simek
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).