Re: DT overlay applied via pinctrl description

[Michal Simek <michal.simek@xxxxxxxxxx>]

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@xxxxxxxxxx> 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;