Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting

Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting

[FanWu]

On 05/06/2014 01:41 AM, Stephen Warren wrote:
> FanWu,
>
> Questions about the Linux kernel should be sent to the public mailing
> lists so that anyone can see the discussion, join in, and/or see the
> result of the discussion in the list archives. Also, please use
> plain-text email, since that makes it easier to read and reply.
>
> The problem with simply calling pinmux_disable_setting() in all cases as
> you propose is that (at least on some HW), calling
> pinmux_disable_setting() may cause the HW to be programmed, and that may
> glitch the pin. In other words, the current code does this:
>
> old mux function -> new mux function
>
> Presumably the board/SoC HW designers have validated that such
> transitions don't cause invalid signals to be emitted.
>
> However, your proposal might do:
>
> old mux function -> disabled state -> new mux function
>
> That introduces a potential extra HW state (whatever
> pinmux_disable_state() programs) which could cause glitches on the pin,
> which could cause problems.
>
> So, I think what we need is your option (1), although some care will be
> needed to avoid too much nested iteration.
>
> On 05/04/2014 08:12 PM, FanWu wrote:
>> On 04/17/2014 04:05 PM, Will Wu wrote:
>>> On 04/14/2014 11:27 AM, FanWu wrote:
>>>>
>>>> Dear, Linus Walleij
>>>>
>>>> Sorry to bother you. I am a Linux Kernel developer in Marvell.
>>>> Recently, I am focusing on the Pinctrl related driver development and
>>>> found there seems a possible issue lying in Pinctrl related framework.
>>>> I will try to make myself clear and please help to review the
>>>> following statement.
>>>>
>>>> In the following I will listed what I found and the suggested code
>>>> change in the mentioned part.
>>>>
>>>> If there is a DTS node is similar to the following one(only the
>>>> pinctrl related part is listed in the node content), in current
>>>> Kernel Pinctrl solution, it is possible that the "desc->mux_usecount"
>>>> for each physical pin is kept being added without any chance to be
>>>> decreased, when the Pin's owner wants to do Pin's state switching in
>>>> their user scenario.
>>>>      component a {
>>>>          ...
>>>>          pinctrl-names = "default", "sleep";
>>>>          pinctrl-0 = <&a_grp_setting_a &*c_grp_setting*>;
>>>>          pinctrl-1 = <&a_grp_setting_b &*c_grp_setting*>;
>>>>          ...
>>>>      }
>>>> The "c_grp_setting" config node is totally same in two pinctrl state.
>>>> The only difference b/t the two Pinctrl states is the
>>>> "a_group_setting". Pinctrl-0 uses "a_grp_setting_a" setting while
>>>> pinctrl-1 uses "a_grp_setting_b" setting.
>>>>
>>>> If the pins' owner wants to switch the pins' state in some cases, he
>>>> needs to do the following sequence:
>>>>      pin = pinctrl_get();
>>>>      state = pinctrl_lookup_state(wanted_state);
>>>>      pinctrl_select_state(state);
>>>>      pinctrl_put();
>>>>
>>>> In pinctrl_select_state function, with current code logic, the
>>>> setting, existing in old state but not in new state, will be
>>>> disabled(put to safe state) ahead of enabling it.
>>>> However, for the state, existing in both old state and new state,
>>>> will not be disabled ahead of enabling it, i.e, the pins' owner only
>>>> wants to change part of the related pins state.
>>>> So the Physical Pin's "desc->mux_usecount" will be increased without
>>>> any chance to be decreased.
>>>>
>>>>
>>>> Thus, there seems two ways to solve the issue I mentioned above:
>>>> 1. Avoid "enable"(calling pinmux_enable_setting) the Same Pins
>>>> setting repeatedly
>>>> 2. "Disable"(calling pinmux_disable_setting) the Same Pins setting
>>>> ahead of enable them in any case.
>>>>
>>>> I think the 2nd way is easy for code change, shown as the following
>>>> one. Please help to review this.
>>>>
>>>> If there is anything wrong, feel free to correct me and share your
>>>> suggestion for this topic.
>>>>
>>>> Great thanks for this !
>>>>
>>>>
>>>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index
>>>> 2f4edfa..c9b97ae 100644
>>>> --- a/drivers/pinctrl/core.c
>>>> +++ b/drivers/pinctrl/core.c
>>>> @@ -944,30 +944,10 @@ int pinctrl_select_state(struct pinctrl *p,
>>>> struct pinctrl_state *state)
>>>>                  return 0;
>>>>
>>>>          if (p->state) {
>>>> -               /*
>>>> -                * The set of groups with a mux configuration in the
>>>> old state
>>>> -                * may not be identical to the set of groups with a
>>>> mux setting
>>>> -                * in the new state. While this might be unusual,
>>>> it's entirely
>>>> -                * possible for the "user"-supplied mapping table to
>>>> be written
>>>> -                * that way. For each group that was configured in
>>>> the old state
>>>> -                * but not in the new state, this code puts that
>>>> group into a
>>>> -                * safe/disabled state.
>>>> -                */
>>>>                   list_for_each_entry(setting, &p->state->settings,
>>>> node) {
>>>> -                        bool found = false;
>>>>                           if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>>>>                                   continue;
>>>> -                        list_for_each_entry(setting2,
>>>> &state->settings, node) {
>>>> -                                if (setting2->type !=
>>>> PIN_MAP_TYPE_MUX_GROUP)
>>>> -                                        continue;
>>>> -                                if (setting2->data.mux.group ==
>>>> -
>>>> setting->data.mux.group) {
>>>> -                                        found = true;
>>>> -                                        break;
>>>> -                                }
>>>> -                        }
>>>> -                        if (!found)
>>>> -                                pinmux_disable_setting(setting);
>>>> +                        pinmux_disable_setting(setting);
>>>>                   }
>>>>           }
>>> Dear, Linus Walleij
>>>
>>> Do you think what I said above is OK for the possible code change ?
>>>
>>> Looking forward your reply.
>>> Great thanks !
>>>
>> Dear Linus,
>>
>> Great thanks for your reply and your suggestion.
>> Just add Stephen Warren to review what mentioned before
>>
>>
>>
>>
>> Dear Stephen,
>>
>> I am Will, from Marvell, BSP developer, focusing on Pinctrl part recently.
>>
>> I just filed patch to do some code changes in the file of
>> drivers/pinctrl/core.c.
>> The reason why I did this and some possible issue are described in the
>> early mail.
>>
>> Could you please help to review the mail and previous mail.
>>
>> Great thanks for this !
>>
>> Looking forward your reply !
>

Dear Stephen,

Thanks a lot for your suggestion for the patch and the mail!


I think the glitch you mentioned is indeed a problem for the vendors who 
define the "pinctrl-single,function-off" which will be activated when 
disabling setting.

I considered my previous option 1 of avoiding duplicated calling 
enable_setting. If we want to avoid too much iteration in this solution, 
some mark variables need to be introduced, which may make code more 
complicated than the previous one.

I also tried to add a member variable named "enabled" in "struct 
setting" to mark whether a setting has been enabled before. If the 
"enabled" is set, the setting_enable function will return and the same 
thing happens in setting_disable function.
However, this method seems still not that perfect. Although the 
c_grp_setting is the same device node to config the same Pin to same 
state, the "c_grp_setting" in the two states are two instance.
With this solution, when Pin state is switched from state0->state1, the 
c_grp_setting's two instances will be both enabled and corresponding 
Pin's mux_usecount will be incremented to "2".
I cannot sure whether the result with this solution is correct or not, 
because I am not quite sure the what's the meaning of enabled/disabled 
setting in pinmux framework.

	pinctrl-0 = <&a_grp_setting &c_grp_setting>;
	pinctrl-1 = <&b_grp_setting &c_grp_setting>;


Current patch and conclusion,
Thus, I am still inclined to use option 2, calling "disable" function 
ahead of enabling function. To avoid the possible HW glitch, it will be 
a good way only to do SW aspect disable operation for the setting which 
is existing in both old and new state, and do SW and HW disable 
operation for the setting only existing in old state, which can be 
implemented by adding a params in disable_setting function.

Please help to review what I mentioned above and the following patch.

Maybe another topic:
In the original code, the Pin setting will be changed to the 
disabled/safe state when Pin state is switched if the old setting is not 
existed in new state rather than directly switched to the new Pin 
setting. Also a possible glitch?
Do you think this is also a part can be refined ? Do you think it is OK 
to do SW disabled operation for all of the setting in old state?


Great thanks for this!
Looking forward your reply!


  drivers/pinctrl/core.c   | 7 ++++--- 
 

  drivers/pinctrl/pinmux.c | 4 ++--
  drivers/pinctrl/pinmux.h | 2 +-
  3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2f4edfa..a5eae27ae 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -847,7 +847,7 @@ static void pinctrl_free_setting(bool disable_setting,
         switch (setting->type) {
         case PIN_MAP_TYPE_MUX_GROUP:
                 if (disable_setting)
-                       pinmux_disable_setting(setting);
+                       pinmux_disable_setting(setting, 1);
                 pinmux_free_setting(setting);
                 break;
         case PIN_MAP_TYPE_CONFIGS_PIN:
@@ -963,11 +963,12 @@ int pinctrl_select_state(struct pinctrl *p, struct 
pinctrl_state *state)
                                 if (setting2->data.mux.group ==
                                                 setting->data.mux.group) {
                                         found = true;
+                                       pinmux_disable_setting(setting, 0);
                                         break;
                                 }
                         }
                         if (!found)
-                               pinmux_disable_setting(setting);
+                               pinmux_disable_setting(setting, 1);
                 }
         }

@@ -1011,7 +1012,7 @@ unapply_new_state:
                  * are free to be muxed by another apply_setting.
                  */
                 if (setting2->type == PIN_MAP_TYPE_MUX_GROUP)
-                       pinmux_disable_setting(setting2);
+                       pinmux_disable_setting(setting2, 1);
         }

         /* There's no infinite recursive loop here because p->state is 
NULL */
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 88cc509..5c9f953 100644
--- a/drivers/pinctrl/pinmux.c
@@ -452,7 +452,7 @@ err_pin_request: 
 

         return ret;
  }

-void pinmux_disable_setting(struct pinctrl_setting const *setting)
+void pinmux_disable_setting(struct pinctrl_setting const *setting, int 
hwops)
  {
         struct pinctrl_dev *pctldev = setting->pctldev;
         const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
@@ -489,7 +489,7 @@ void pinmux_disable_setting(struct pinctrl_setting 
const *setting)
         for (i = 0; i < num_pins; i++)
                 pin_free(pctldev, pins[i], NULL);

-       if (ops->disable)
+       if (ops->disable && hwops)
                 ops->disable(pctldev, setting->data.mux.func, 
setting->data.mux.group);
  }

diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index d1a98b1..cd3a4af 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -29,7 +29,7 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
                           struct pinctrl_setting *setting);
  void pinmux_free_setting(struct pinctrl_setting const *setting);
  int pinmux_enable_setting(struct pinctrl_setting const *setting);
-void pinmux_disable_setting(struct pinctrl_setting const *setting);
+void pinmux_disable_setting(struct pinctrl_setting const *setting, int 
hwops);

  #else

-- 

Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting

[Stephen Warren]

On 05/07/2014 02:02 AM, FanWu wrote:
...
> I think the glitch you mentioned is indeed a problem for the vendors who
> define the "pinctrl-single,function-off" which will be activated when
> disabling setting.

"pinctrl-single,function-off" is one specific driver's way of
configuring what happens when a mux option is disabled. The issue
applies to all drivers that implement a mux option disable function.

...
> I considered my previous option 1 of avoiding duplicated calling
> enable_setting. If we want to avoid too much iteration in this solution,
> some mark variables need to be introduced, which may make code more
> complicated than the previous one.

Probably the best thing is to just remove the concept of "disable a mux
option". It doesn't make sense. All you can do with mux options is to
select some specific mux option. Disabling a mux option isn't something
that's logically possible. On some HW, perhaps you can do something
similar by tri-stating the pin or something, but that should be an
explicit part of the pinctrl state definition rather than an automatic
side-effect.

...
> Maybe another topic:
> In the original code, the Pin setting will be changed to the
> disabled/safe state when Pin state is switched if the old setting is not
> existed in new state rather than directly switched to the new Pin
> setting. Also a possible glitch?

That's not a glitch, since it's not a temporary state. The mux setting
starts out as configured by the old state, then switches one time to
whatever the disable function sets it to. After that, it won't switch
again, unless there's an explicit SW action to enable a new state.

Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting

[FanWu]

On 05/13/2014 04:20 AM, Stephen Warren wrote:
> On 05/07/2014 02:02 AM, FanWu wrote:
> ...
>> I think the glitch you mentioned is indeed a problem for the vendors who
>> define the "pinctrl-single,function-off" which will be activated when
>> disabling setting.
>
> "pinctrl-single,function-off" is one specific driver's way of
> configuring what happens when a mux option is disabled. The issue
> applies to all drivers that implement a mux option disable function.
>
> ...
>> I considered my previous option 1 of avoiding duplicated calling
>> enable_setting. If we want to avoid too much iteration in this solution,
>> some mark variables need to be introduced, which may make code more
>> complicated than the previous one.
>
> Probably the best thing is to just remove the concept of "disable a mux
> option". It doesn't make sense. All you can do with mux options is to
> select some specific mux option. Disabling a mux option isn't something
> that's logically possible. On some HW, perhaps you can do something
> similar by tri-stating the pin or something, but that should be an
> explicit part of the pinctrl state definition rather than an automatic
> side-effect.
>
> ...
>> Maybe another topic:
>> In the original code, the Pin setting will be changed to the
>> disabled/safe state when Pin state is switched if the old setting is not
>> existed in new state rather than directly switched to the new Pin
>> setting. Also a possible glitch?
>
> That's not a glitch, since it's not a temporary state. The mux setting
> starts out as configured by the old state, then switches one time to
> whatever the disable function sets it to. After that, it won't switch
> again, unless there's an explicit SW action to enable a new state.
>

Dear, Stephen,

Thanks a lot for your reviewing!

I think the solution you mentioned about removing disable_pinmux 
function may introduce other possible code logic change, like change on 
enable_pinmux and "pinctrl-single,function-off" usage, which may be 
complicated thing. I cannot say whether that is a acceptable solution 
for Pinmux/pinctrl subsystem owner and user.

Thus, I think the patch I filed recently may be more easier way to solve 
this case.


About the glitch I mentioned before, I want to make myself clear.
If there is a case like the following one:
pinctrl-0 = <&a_grp_settingA>;
pinctrl-1 = <&a_grp_settingB>;
"a_grp_settingA" and "a_grp_settingB" are used to described the same 
Pin's different mux and function configuration
In my understanding,
When there is a need to switch Pin group state, the current code will 
disable "a_grp_settingA" first ahead of enabling "a_grp_settingB", right ?

Do you mean the case I mentioned will not be a glitch ?

If I got anything wrong, feel free to correct me.

Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting

[Stephen Warren]

On 05/12/2014 11:53 PM, FanWu wrote:
...
> About the glitch I mentioned before, I want to make myself clear.
> If there is a case like the following one:
> pinctrl-0 = <&a_grp_settingA>;
> pinctrl-1 = <&a_grp_settingB>;
> "a_grp_settingA" and "a_grp_settingB" are used to described the same
> Pin's different mux and function configuration
> In my understanding,
> When there is a need to switch Pin group state, the current code will
> disable "a_grp_settingA" first ahead of enabling "a_grp_settingB", right ?

Yes.

> Do you mean the case I mentioned will not be a glitch ?

I guess you're talking about that:

>> In the original code, the Pin setting will be changed to the
>> disabled/safe state when Pin state is switched if the old setting is not
>> existed in new state rather than directly switched to the new Pin
>> setting. Also a possible glitch?

Yes, in this case, there is no glitch. However, there is certainly a
change in HW configuration. A glitch is a temporary short-term
accidental change in output value or configuration. In the case quoted
immediately above, the change is permanent - at least until some other
state is activated later. Hence, there is no glitch. However, there
certainly is a change in HW configuration, and that could be just as
problematic, depending on the HW and exact pin configuration.