Re: [PATCH v2] pinctrl: add params in disable_setting for different usage
* Stephen Warren [140523 09:16]: > On 05/22/2014 07:54 PM, FanWu wrote: > > the pinctrl-single driver is still using ops->disable, if I remove the > > "disable" in ops, there will be build error in the vendor's code base > > who is using pinctrl-single driver. > > I thought Tony said it was fine to simply remove pinctrl-single's > ops->disable code completeley. AFAIK nobody is using it. At least we don't have any references to pinctrl-single,function-off property in the .dts files. So removing pcs_disable seems safe to do, the property we probably want to parse and produce a warning in case somebody is using it out of tree. Regards, Tony -- 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: [PATCH v2] pinctrl: add params in disable_setting for different usage
On 05/22/2014 07:54 PM, FanWu wrote: > On 05/23/2014 07:13 AM, Stephen Warren wrote: >> On 05/21/2014 09:10 PM, f...@marvell.com wrote: >>> From: Fan Wu >>> >>> What the patch did: >>> 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in >>> each time of >>>calling pinctrl_select_state >>> 2.Remove the HW disable operation in in pinmux_disable_setting function. >>> >> ... >>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c >>> index c0fe609..c97491a 100644 >>> --- a/drivers/pinctrl/core.c >>> +++ b/drivers/pinctrl/core.c >>> @@ -993,25 +993,13 @@ int pinctrl_select_state(struct pinctrl *p, >>> struct pinctrl_state *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. >>> + * that way. This code is used for each group that was >>> + * configured in the old state but not in the new state >> >> >> Looking at the code, it's run for every group in the state, not "each >> group that was configured in the old state but not in the new state" > For you question 1: > The disable_pinmux_setting is for the all of the setting in old state. > This is what we really need to do, ahead of enable setting in new state. > In the first patch I filed, which still includes the HW ops in > disable_pinmux_setting, to disable each setting in old state and then to > enable the setting in new state will introduce HW glitch. > But in the current solution, the glitch will not be there, because there > is no HW ops in disable_pinmux_setting. > And please notice the patch is mainly used to avoid the duplicated > enable operation for the same pin. I think you missed the point of my comment. I think the new comment text is incorrect. Instead, how about replacing the entire comment with: /* * For each pinmux setting in the old state, forget SW's record of mux * owner for that pingroup. Any pingroups which are still owned by the * new state will be re-acquired by the call to pinmux_enable_setting() * in the loop below. */ >>> @@ -515,9 +514,6 @@ void pinmux_disable_setting(struct >>> pinctrl_setting const *setting) >>>pins[i], desc->name, gname); >>> } >>> } >>> - >>> -if (ops->disable) >>> -ops->disable(pctldev, setting->data.mux.func, >>> setting->data.mux.group); >>> } >> >> Should that op be removed from the header file and all drivers too? > For your question 2: > the pinctrl-single driver is still using ops->disable, if I remove the > "disable" in ops, there will be build error in the vendor's code base > who is using pinctrl-single driver. I thought Tony said it was fine to simply remove pinctrl-single's ops->disable code completeley. > Just as I said in the last mail, > the next plan for this topic: > > 1. To remove the disable ops registration when defining the > "include/linux/pinctrl/pinmux.h" in inctrl-single driver. > Meanwhile, the related things, like "pinctrl-single,function-off" > property and corresponding flag in "pcs_device", will be also removed. > > 2. To remove the disable ops in "pinmux_ops" in the file of > include/linux/pinctrl/pinmux.h > > Are you OK for this ? I guess splitting that into separate patches is fine. -- 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: [PATCH v2] pinctrl: add params in disable_setting for different usage
On 05/23/2014 07:13 AM, Stephen Warren wrote: On 05/21/2014 09:10 PM, f...@marvell.com wrote: From: Fan Wu What the patch did: 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time of calling pinctrl_select_state 2.Remove the HW disable operation in in pinmux_disable_setting function. ... diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index c0fe609..c97491a 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -993,25 +993,13 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *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. +* that way. This code is used for each group that was +* configured in the old state but not in the new state Looking at the code, it's run for every group in the state, not "each group that was configured in the old state but not in the new state" @@ -515,9 +514,6 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) pins[i], desc->name, gname); } } - - if (ops->disable) - ops->disable(pctldev, setting->data.mux.func, setting->data.mux.group); } Should that op be removed from the header file and all drivers too? Dear Stephen, For you question 1: The disable_pinmux_setting is for the all of the setting in old state. This is what we really need to do, ahead of enable setting in new state. In the first patch I filed, which still includes the HW ops in disable_pinmux_setting, to disable each setting in old state and then to enable the setting in new state will introduce HW glitch. But in the current solution, the glitch will not be there, because there is no HW ops in disable_pinmux_setting. And please notice the patch is mainly used to avoid the duplicated enable operation for the same pin. For your question 2: the pinctrl-single driver is still using ops->disable, if I remove the "disable" in ops, there will be build error in the vendor's code base who is using pinctrl-single driver. Just as I said in the last mail, the next plan for this topic: 1. To remove the disable ops registration when defining the "include/linux/pinctrl/pinmux.h" in inctrl-single driver. Meanwhile, the related things, like "pinctrl-single,function-off" property and corresponding flag in "pcs_device", will be also removed. 2. To remove the disable ops in "pinmux_ops" in the file of include/linux/pinctrl/pinmux.h Are you OK for this ? -- 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: [PATCH v2] pinctrl: add params in disable_setting for different usage
On 05/21/2014 09:10 PM, f...@marvell.com wrote: > From: Fan Wu > > What the patch did: > 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time > of > calling pinctrl_select_state > 2.Remove the HW disable operation in in pinmux_disable_setting function. > ... > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index c0fe609..c97491a 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -993,25 +993,13 @@ int pinctrl_select_state(struct pinctrl *p, struct > pinctrl_state *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. > + * that way. This code is used for each group that was > + * configured in the old state but not in the new state Looking at the code, it's run for every group in the state, not "each group that was configured in the old state but not in the new state" > @@ -515,9 +514,6 @@ void pinmux_disable_setting(struct pinctrl_setting const > *setting) >pins[i], desc->name, gname); > } > } > - > - if (ops->disable) > - ops->disable(pctldev, setting->data.mux.func, > setting->data.mux.group); > } Should that op be removed from the header file and all drivers too? -- 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: [PATCH v2] pinctrl: add params in disable_setting for different usage
On 05/22/2014 11:10 AM, f...@marvell.com wrote:
From: Fan Wu
What the patch did:
1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time of
calling pinctrl_select_state
2.Remove the HW disable operation in in pinmux_disable_setting function.
The reason why to do this is that:
1.To avoid duplicated enable_setting operation without disabling operation
which will let Pin's desc->mux_usecount keep being added.
2.The HW pin disable operation is not useful for most of the vendors' platform.
And this avoid the HW glitch after using the item 1# modification.
In the following case, the issue can be reproduced:
1)There is a driver need to switch Pin state dynamicly, E.g. b/t "sleep" and
"default" state
2)The Pin setting configuration in DTS node may be like the following one:
component a {
pinctrl-names = "default", "sleep";
pinctrl-0 = <&a_grp_setting &c_grp_setting>;
pinctrl-1 = <&b_grp_setting &c_grp_setting>;
}
The "c_grp_setting" config node is totaly same, maybe like following one:
c_grp_setting: c_grp_setting {
pinctrl-single,pins = ;
MFP_DEFAULT;
}
3)When switching the Pin state in the following official Pinctrl sequence:
pin = pinctrl_get();
state = pinctrl_lookup_state(wanted_state);
pinctrl_select_state(state);
pinctrl_put();
Test Result:
1)The switch is completed as expectation, that is: component's
Pins configuration are changed according to the description in the
"wanted_state" group setting
2)The "desc->mux_usecount" of corresponding Pins in "c_group" is added without
being
decreased, because the "desc" is for each physical pin while the "setting" is
for each setting node in the DTS.
Thus, if the "c_grp_setting" in pinctrl-0 is not disabled ahead of enabling
"c_grp_setting" in pinctrl-1, the desc->mux_usecount will be kept added without
any chance to be decreased.
According to the comments in the original code, only the setting, in old state
but not in new state, will be "disable"(calling pinmux_disable_setting), which
is correct logic but not intact. We still need consider case that the setting
is in both old state and new state.
We can do this in the following two ways:
1) Avoid "enable"(calling pinmux_enable_setting) the Same Pins setting
repeatedly.
2) "Disable"(calling pinmux_disable_setting) the "Same Pins setting", actually
two setting instance, ahead of enabling them.
Analysis:
1.The solution 2# is better because it can avoid too much iteration.
2.If we disable all of the setting in the old state and one/ones of the
setting(s) is/are
existed in the new state, the Pin's mux function change may happen when
some SoC vendors defined the "pinctrl-single,function-off" in their DTS file.
old_setting=>disabled_setting=>new_setting.
3.In the pinmux framework, when Pin state is switched, the setting in the old
state should be
marked as "disabled".
Conclusion:
1.To Remove the HW disabling operation to above the glitch mentioned above.
2.Handle the issue mentioned above by disabling all of the setting in old
state and then enable the new setting in new state.
Signed-off-by: Fan Wu
---
drivers/pinctrl/core.c | 18 +++---
drivers/pinctrl/pinmux.c |4
2 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..c97491a 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -993,25 +993,13 @@ int pinctrl_select_state(struct pinctrl *p, struct
pinctrl_state *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.
+* that way. This code is used for each group that was
+* configured in the old state but not in the new 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);
+
[PATCH v2] pinctrl: add params in disable_setting for different usage
From: Fan Wu
What the patch did:
1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time of
calling pinctrl_select_state
2.Remove the HW disable operation in in pinmux_disable_setting function.
The reason why to do this is that:
1.To avoid duplicated enable_setting operation without disabling operation
which will let Pin's desc->mux_usecount keep being added.
2.The HW pin disable operation is not useful for most of the vendors' platform.
And this avoid the HW glitch after using the item 1# modification.
In the following case, the issue can be reproduced:
1)There is a driver need to switch Pin state dynamicly, E.g. b/t "sleep" and
"default" state
2)The Pin setting configuration in DTS node may be like the following one:
component a {
pinctrl-names = "default", "sleep";
pinctrl-0 = <&a_grp_setting &c_grp_setting>;
pinctrl-1 = <&b_grp_setting &c_grp_setting>;
}
The "c_grp_setting" config node is totaly same, maybe like following one:
c_grp_setting: c_grp_setting {
pinctrl-single,pins = ;
MFP_DEFAULT;
}
3)When switching the Pin state in the following official Pinctrl sequence:
pin = pinctrl_get();
state = pinctrl_lookup_state(wanted_state);
pinctrl_select_state(state);
pinctrl_put();
Test Result:
1)The switch is completed as expectation, that is: component's
Pins configuration are changed according to the description in the
"wanted_state" group setting
2)The "desc->mux_usecount" of corresponding Pins in "c_group" is added without
being
decreased, because the "desc" is for each physical pin while the "setting" is
for each setting node in the DTS.
Thus, if the "c_grp_setting" in pinctrl-0 is not disabled ahead of enabling
"c_grp_setting" in pinctrl-1, the desc->mux_usecount will be kept added without
any chance to be decreased.
According to the comments in the original code, only the setting, in old state
but not in new state, will be "disable"(calling pinmux_disable_setting), which
is correct logic but not intact. We still need consider case that the setting
is in both old state and new state.
We can do this in the following two ways:
1) Avoid "enable"(calling pinmux_enable_setting) the Same Pins setting
repeatedly.
2) "Disable"(calling pinmux_disable_setting) the "Same Pins setting", actually
two setting instance, ahead of enabling them.
Analysis:
1.The solution 2# is better because it can avoid too much iteration.
2.If we disable all of the setting in the old state and one/ones of the
setting(s) is/are
existed in the new state, the Pin's mux function change may happen when
some SoC vendors defined the "pinctrl-single,function-off" in their DTS file.
old_setting=>disabled_setting=>new_setting.
3.In the pinmux framework, when Pin state is switched, the setting in the old
state should be
marked as "disabled".
Conclusion:
1.To Remove the HW disabling operation to above the glitch mentioned above.
2.Handle the issue mentioned above by disabling all of the setting in old
state and then enable the new setting in new state.
Signed-off-by: Fan Wu
---
drivers/pinctrl/core.c | 18 +++---
drivers/pinctrl/pinmux.c |4
2 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..c97491a 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -993,25 +993,13 @@ int pinctrl_select_state(struct pinctrl *p, struct
pinctrl_state *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.
+* that way. This code is used for each group that was
+* configured in the old state but not in the new 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);
