Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting
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 = <_grp_settingA>; > pinctrl-1 = <_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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting
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 = <_grp_settingA>; pinctrl-1 = <_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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting
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 = <_grp_setting_a &*c_grp_setting*>;
pinctrl-1 = <_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, >state->settings,
node) {
-bool found = false;
if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
continue;
-list_for_each_entry(setting2,
>settings, node) {
-if (setting2->type !=
PIN_MAP_TYPE_MUX_GROUP)
-continue;
-if (setting2->data.mux.group ==
-
setting->data.mux.group) {
-
Re: [Pinctrl] A suggestion to avoid duplicated enabling operation on a pin's setting
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;
-
