Re: [PATCH v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/04/2014 12:49 AM, Stephen Warren wrote:
On 06/03/2014 01:37 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.
3.Remove the disable ops in struct pinmux_ops
...
Signed-off-by: Fan Wu
Signed-off-by: Stephen Warren
As I mentioned in my previous email, I didn't sign this off. I made some
suggestions for a better alternative in that email.
If I *had* written that s-o-b, then it should be before yours in the
patch description since you handled the patch last.
The Signed-off didn't bother me.
I will Choose your option 2# and thanks for your suggestion about this :)
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
@@ -70,8 +70,6 @@ struct pinmux_ops {
unsigned * const num_groups);
int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector,
unsigned group_selector);
- void (*disable) (struct pinctrl_dev *pctldev, unsigned func_selector,
-unsigned group_selector);
This will cause a compile failure, since many drivers still set the
.disable function pointer. You need to update all the driver files to
remove those functions too. There's quite a bit of code in some of those
functions, so you'd need the relevant driver maintainers to confirm it's
OK to remove it. I think only the owners of pinctrl-egra and
pinctrl-single have ack'd this concept so far.
For this part, I think I mentioned this before, simply removing disable
ops will introduce the compiling error.
I think there are several ways to handle this:
1. Don't remove the disable ops in struct pinmux_ops in this patch but
to remove the disable ops in struct pinmux_ops after the another patch
is merged, which is used to remove all of the disable ops user in all
drivers.
2. Just remove the disable ops in pinmux_ops in this patch, and make a
another patch ASAP to remove all the disable ops user in all drivers.
3. Remove the disable ops in struct pinmux_ops and remove all the
disable ops user in all drivers, all in this patch.
For the solution 2, I just think it may be not a good way to include so
much content in a patch, which are not in a same code level.
I am just inclined to use solution 1# or 3#.
Please share your comments.
Great thanks 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 v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/03/2014 03:37 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.
3.Remove the disable ops in struct pinmux_ops
The reason why to do this is that:
1.To avoid duplicated calling 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 can be used to 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 = <_grp_setting _grp_setting>;
pinctrl-1 = <_grp_setting _grp_setting>;
}
The "c_grp_setting" config node is totaly same, maybe like following one:
c_grp_setting: c_grp_setting {
pinctrl-single,pins = ;
}
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();
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 choose 2# solution, we need to
1) Should mark all the setting in disable state as disabled ahead of enabling.
2) Avoid the possible HW glitch by removing the HW disable ops in
in the pinmux_disable_setting
If we disable all of the settings 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.(HW glitch)
Signed-off-by: Fan Wu
Signed-off-by: Stephen Warren
---
drivers/pinctrl/core.c | 24 +---
drivers/pinctrl/pinmux.c |4
include/linux/pinctrl/pinmux.h |2 --
3 files changed, 5 insertions(+), 25 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..4445a67 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -989,29 +989,15 @@ int pinctrl_select_state(struct pinctrl *p, struct
pinctrl_state *state)
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.
+* 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.
*/
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) {
- found = true;
- break;
- }
- }
- if (!found)
- pinmux_disable_setting(setting);
+ pinmux_disable_setting(setting);
}
}
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 9248ce4..c2c4aff 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -469,7 +469,6 @@ void
Re: [PATCH v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/03/2014 03:37 PM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
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.
3.Remove the disable ops in struct pinmux_ops
The reason why to do this is that:
1.To avoid duplicated calling 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 can be used to 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 = GPIO48 AF6;
}
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();
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 choose 2# solution, we need to
1) Should mark all the setting in disable state as disabled ahead of enabling.
2) Avoid the possible HW glitch by removing the HW disable ops in
in the pinmux_disable_setting
If we disable all of the settings 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.(HW glitch)
Signed-off-by: Fan Wu f...@marvell.com
Signed-off-by: Stephen Warren swar...@nvidia.com
---
drivers/pinctrl/core.c | 24 +---
drivers/pinctrl/pinmux.c |4
include/linux/pinctrl/pinmux.h |2 --
3 files changed, 5 insertions(+), 25 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..4445a67 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -989,29 +989,15 @@ int pinctrl_select_state(struct pinctrl *p, struct
pinctrl_state *state)
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.
+* 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.
*/
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);
}
}
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 9248ce4..c2c4aff 100644
--- a/drivers/pinctrl/pinmux.c
+++
Re: [PATCH v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 06/04/2014 12:49 AM, Stephen Warren wrote:
On 06/03/2014 01:37 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
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.
3.Remove the disable ops in struct pinmux_ops
...
Signed-off-by: Fan Wu f...@marvell.com
Signed-off-by: Stephen Warren swar...@nvidia.com
As I mentioned in my previous email, I didn't sign this off. I made some
suggestions for a better alternative in that email.
If I *had* written that s-o-b, then it should be before yours in the
patch description since you handled the patch last.
The Signed-off didn't bother me.
I will Choose your option 2# and thanks for your suggestion about this :)
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
@@ -70,8 +70,6 @@ struct pinmux_ops {
unsigned * const num_groups);
int (*enable) (struct pinctrl_dev *pctldev, unsigned func_selector,
unsigned group_selector);
- void (*disable) (struct pinctrl_dev *pctldev, unsigned func_selector,
-unsigned group_selector);
This will cause a compile failure, since many drivers still set the
.disable function pointer. You need to update all the driver files to
remove those functions too. There's quite a bit of code in some of those
functions, so you'd need the relevant driver maintainers to confirm it's
OK to remove it. I think only the owners of pinctrl-egra and
pinctrl-single have ack'd this concept so far.
For this part, I think I mentioned this before, simply removing disable
ops will introduce the compiling error.
I think there are several ways to handle this:
1. Don't remove the disable ops in struct pinmux_ops in this patch but
to remove the disable ops in struct pinmux_ops after the another patch
is merged, which is used to remove all of the disable ops user in all
drivers.
2. Just remove the disable ops in pinmux_ops in this patch, and make a
another patch ASAP to remove all the disable ops user in all drivers.
3. Remove the disable ops in struct pinmux_ops and remove all the
disable ops user in all drivers, all in this patch.
For the solution 2, I just think it may be not a good way to include so
much content in a patch, which are not in a same code level.
I am just inclined to use solution 1# or 3#.
Please share your comments.
Great thanks 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 v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 05/30/2014 03:19 AM, Stephen Warren wrote: On 05/25/2014 08:43 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. ... This commit description is way too long for such a simple change. A much shorter summary would be better. Signed-off-by: Fan Wu Signed-off-by: Stephen Warren I'm pretty sure I never signed off on this patch. How come my s-o-b line is there? This patch still doesn't remove ops->disable from the struct pinmux_ops, nor any of its implementations. Shouldn't it? Dear Stephen, For your comments 1: The reason why I want to put a lot of info into the patch comments is that the long term discussion about the topic and the patch is not that easy to understand for a patch reader, or maybe is not easy for us to understand in far future. For your comments 2: I accepted your suggestion of inline code comments and some other suggestions from our discussion, so I added your signed off tailing in the patch comments. If you think it is not fine, I can remove it in the new patch version. For your comments 3: I think I have made myself clear in the last mail: 1) If I remove the ops->disable from the struct pinmux_ops in this patch, the pinctrl-single user will got build error immediately. 2) Thus, I want to merge this patch first and then make other two patches later: One is to remove the ops->disable registration in pinctrl-single driver. And the other is to remove ops->disable in struct pinmux_ops. Could you please give your final suggestion about this and then I will give new patch? Great thanks about this! :) Looking forward your reply ! -- 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 v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 05/30/2014 03:19 AM, Stephen Warren wrote: On 05/25/2014 08:43 PM, f...@marvell.com wrote: From: Fan Wu f...@marvell.com 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. ... This commit description is way too long for such a simple change. A much shorter summary would be better. Signed-off-by: Fan Wu f...@marvell.com Signed-off-by: Stephen Warren swar...@nvidia.com I'm pretty sure I never signed off on this patch. How come my s-o-b line is there? This patch still doesn't remove ops-disable from the struct pinmux_ops, nor any of its implementations. Shouldn't it? Dear Stephen, For your comments 1: The reason why I want to put a lot of info into the patch comments is that the long term discussion about the topic and the patch is not that easy to understand for a patch reader, or maybe is not easy for us to understand in far future. For your comments 2: I accepted your suggestion of inline code comments and some other suggestions from our discussion, so I added your signed off tailing in the patch comments. If you think it is not fine, I can remove it in the new patch version. For your comments 3: I think I have made myself clear in the last mail: 1) If I remove the ops-disable from the struct pinmux_ops in this patch, the pinctrl-single user will got build error immediately. 2) Thus, I want to merge this patch first and then make other two patches later: One is to remove the ops-disable registration in pinctrl-single driver. And the other is to remove ops-disable in struct pinmux_ops. Could you please give your final suggestion about this and then I will give new patch? Great thanks about this! :) Looking forward your reply ! -- 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 v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 05/26/2014 10:52 AM, FanWu wrote:
On 05/26/2014 10:43 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 calling 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 can be used to 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 = <_grp_setting _grp_setting>;
pinctrl-1 = <_grp_setting _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 settings in
old
state and then enable the all of the settings in new state.
Signed-off-by: Fan Wu
Signed-off-by: Stephen Warren
---
drivers/pinctrl/core.c | 24 +---
drivers/pinctrl/pinmux.c |4
2 files changed, 5 insertions(+), 23 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..4445a67 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -989,29 +989,15 @@ int pinctrl_select_state(struct pinctrl *p,
struct pinctrl_state *state)
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.
+ * 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.
*/
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)
-
Re: [PATCH v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 05/26/2014 10:52 AM, FanWu wrote:
On 05/26/2014 10:43 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
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 calling 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 can be used to 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 = GPIO48 AF6;
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 settings in
old
state and then enable the all of the settings in new state.
Signed-off-by: Fan Wu f...@marvell.com
Signed-off-by: Stephen Warren swar...@nvidia.com
---
drivers/pinctrl/core.c | 24 +---
drivers/pinctrl/pinmux.c |4
2 files changed, 5 insertions(+), 23 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..4445a67 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -989,29 +989,15 @@ int pinctrl_select_state(struct pinctrl *p,
struct pinctrl_state *state)
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.
+ * 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.
*/
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
Re: [PATCH v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 05/26/2014 10:43 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 calling 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 can be used to 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 = <_grp_setting _grp_setting>;
pinctrl-1 = <_grp_setting _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 settings in old
state and then enable the all of the settings in new state.
Signed-off-by: Fan Wu
Signed-off-by: Stephen Warren
---
drivers/pinctrl/core.c | 24 +---
drivers/pinctrl/pinmux.c |4
2 files changed, 5 insertions(+), 23 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..4445a67 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -989,29 +989,15 @@ int pinctrl_select_state(struct pinctrl *p, struct
pinctrl_state *state)
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.
+* 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.
*/
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 ==
-
Re: [PATCH v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
On 05/26/2014 10:43 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
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 calling 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 can be used to 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 = GPIO48 AF6;
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 settings in old
state and then enable the all of the settings in new state.
Signed-off-by: Fan Wu f...@marvell.com
Signed-off-by: Stephen Warren swar...@nvidia.com
---
drivers/pinctrl/core.c | 24 +---
drivers/pinctrl/pinmux.c |4
2 files changed, 5 insertions(+), 23 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..4445a67 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -989,29 +989,15 @@ int pinctrl_select_state(struct pinctrl *p, struct
pinctrl_state *state)
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.
+* 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.
*/
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
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/23/2014 07:13 AM, Stephen Warren wrote: On 05/21/2014 09:10 PM, f...@marvell.com wrote: From: Fan Wu f...@marvell.com 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/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 = <_grp_setting _grp_setting>;
pinctrl-1 = <_grp_setting _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, >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) {
- found = true;
- break;
- }
- }
- if (!found)
- pinmux_disable_setting(setting);
+
Re: [PATCH] pinctrl: add params in disable_setting for different usage
On 05/21/2014 02:42 AM, Stephen Warren wrote:
On 05/19/2014 09:05 PM, FanWu wrote:
On 05/20/2014 04:55 AM, Stephen Warren wrote:
On 05/18/2014 08:54 PM, FanWu wrote:
On 05/17/2014 03:53 AM, Stephen Warren wrote:
On 05/16/2014 10:21 AM, Linus Walleij wrote:
On Wed, May 14, 2014 at 4:01 AM, wrote:
From: Fan Wu
The patch added params in disable_setting to differ the two possible
usage,
1.Only want to disable the pin setting in SW aspect, param can be
set to "0"
2.Want to disable the pin setting in both HW and SW aspect, param
can be set to "1";
The reason why to do this is that:
To avoid duplicated enable_setting operation without disabling
operation which will
let Pin's desc->mux_usecount keep being added.
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 the two state is same, like the
following one:
component a {
pinctrl-names = "default", "sleep";
pinctrl-0 = <_grp_setting _grp_setting>;
pinctrl-1 = <_grp_setting _grp_setting>;
}
The "c_grp_setting" config node is totaly same, maybe like following
one:
Hm this is a quite interesting thing if we can get it in place, but
I need Stephen's consent, also Tony should have a look at this as
I know he's had the same problem as you in pinctrl-single.
I only briefly looked at the patch, but it probably solves/hides the
immediate problem.
However, rather than doing this, why not just remove
pinmux_disable_setting() completely. It doesn't make sense to
"disable a
mux selection" (some value is always selected in the mux register
field)
any more than it does to "disable a drive strength selection". We don't
have a pinconf_disable_setting(), and couldn't really add one if we
wanted. For consistency, let's just remove pinmux_disable_setting(). Do
you agree?
Dear, Stephen and Guys,
Sorry for late due to some personal affairs in Weekend time.
I don't think it is a proper way to remove pinmux_disable_setting
directly without changing any other code, like no change on the code in
pinmux_enable_setting.
Talking about the pinmux_disable_operation, in SW aspect, we also need
to consider the "pinmux_enable_setting" operation.
For the "pinmux_enable_setting" operation, there is some SW level code
logic, like pin_request.
Do you think it is a acceptable way to remove the SW level code logic
from the "pinmux_enable_setting" path, because there will be no
corresponding operation in reverse order in pinmux_disable_setting after
applying our possible change?
No, I don't think we should remove the SW aspects of
pinmux_enable_setting(). The pinctrl core currently tracks which pinctrl
state "owns" each pingroup's mux, so that different pinctrl states can't
both attempt to configure a pingroup's mux setting. We need to keep all
the SW aspects of mux enable/disable. All I'm proposing removing is the
HW-programming parts of pinmux_disable_setting().
At least, I think this way may be a considerable change in Pinmux
framework, right?
Yes, removing all of pinmux_en/disable_setting would be a considerable
and likely inappropriate change.
Talking about HW aspect,
I think the solution you mentioned is indeed a good way to solve the
problem for some HW vendor's SoC chip, but may be not that intact
solution.
In my understanding, the pinmux operation, like enabling and disabling,
is to change pin's(pins') multi-function from funcion_M to function_N.
And, the "pinconf" enabling function is used to change the attributes of
the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
The pinmux disabling operation will be called in the following case in
current pinmux framework:
1. when pin(s) is/are freed or error handler when configure it(them) and
finally the pin will be changed to a disabled/safe state if defined by
vendor.
2. in the pinctrl_select_state function
The item 2# is just the thing we talked about in this loop and we reach
agreement that the item 2# is not useful.
I think the item 1# is still useful for some vendor if they defined the
disabled/safe multi-function for a pin. They may expect the pin is
changed to the disabled/safe state for saving power or some security
reason.
The only time item #1 above would happen is an error case. If there's an
error, there shouldn't be any expectation for the specific state of the
pinmux. If this intermediate state is illegal, then that's a problem in
an of itself; the HW is going to be in that state for some (admittedly
small) amount of time while the pinmux is being programmed, error or no
error, hence all the intermediate states had better be legal. I think
it's fine if the HW programming is simply left in whatever partial state
the code managed to get to. It's quite unlikely there's any
Re: [PATCH] pinctrl: add params in disable_setting for different usage
On 05/21/2014 02:42 AM, Stephen Warren wrote:
On 05/19/2014 09:05 PM, FanWu wrote:
On 05/20/2014 04:55 AM, Stephen Warren wrote:
On 05/18/2014 08:54 PM, FanWu wrote:
On 05/17/2014 03:53 AM, Stephen Warren wrote:
On 05/16/2014 10:21 AM, Linus Walleij wrote:
On Wed, May 14, 2014 at 4:01 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
The patch added params in disable_setting to differ the two possible
usage,
1.Only want to disable the pin setting in SW aspect, param can be
set to 0
2.Want to disable the pin setting in both HW and SW aspect, param
can be set to 1;
The reason why to do this is that:
To avoid duplicated enable_setting operation without disabling
operation which will
let Pin's desc-mux_usecount keep being added.
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 the two state is same, 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:
Hm this is a quite interesting thing if we can get it in place, but
I need Stephen's consent, also Tony should have a look at this as
I know he's had the same problem as you in pinctrl-single.
I only briefly looked at the patch, but it probably solves/hides the
immediate problem.
However, rather than doing this, why not just remove
pinmux_disable_setting() completely. It doesn't make sense to
disable a
mux selection (some value is always selected in the mux register
field)
any more than it does to disable a drive strength selection. We don't
have a pinconf_disable_setting(), and couldn't really add one if we
wanted. For consistency, let's just remove pinmux_disable_setting(). Do
you agree?
Dear, Stephen and Guys,
Sorry for late due to some personal affairs in Weekend time.
I don't think it is a proper way to remove pinmux_disable_setting
directly without changing any other code, like no change on the code in
pinmux_enable_setting.
Talking about the pinmux_disable_operation, in SW aspect, we also need
to consider the pinmux_enable_setting operation.
For the pinmux_enable_setting operation, there is some SW level code
logic, like pin_request.
Do you think it is a acceptable way to remove the SW level code logic
from the pinmux_enable_setting path, because there will be no
corresponding operation in reverse order in pinmux_disable_setting after
applying our possible change?
No, I don't think we should remove the SW aspects of
pinmux_enable_setting(). The pinctrl core currently tracks which pinctrl
state owns each pingroup's mux, so that different pinctrl states can't
both attempt to configure a pingroup's mux setting. We need to keep all
the SW aspects of mux enable/disable. All I'm proposing removing is the
HW-programming parts of pinmux_disable_setting().
At least, I think this way may be a considerable change in Pinmux
framework, right?
Yes, removing all of pinmux_en/disable_setting would be a considerable
and likely inappropriate change.
Talking about HW aspect,
I think the solution you mentioned is indeed a good way to solve the
problem for some HW vendor's SoC chip, but may be not that intact
solution.
In my understanding, the pinmux operation, like enabling and disabling,
is to change pin's(pins') multi-function from funcion_M to function_N.
And, the pinconf enabling function is used to change the attributes of
the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
The pinmux disabling operation will be called in the following case in
current pinmux framework:
1. when pin(s) is/are freed or error handler when configure it(them) and
finally the pin will be changed to a disabled/safe state if defined by
vendor.
2. in the pinctrl_select_state function
The item 2# is just the thing we talked about in this loop and we reach
agreement that the item 2# is not useful.
I think the item 1# is still useful for some vendor if they defined the
disabled/safe multi-function for a pin. They may expect the pin is
changed to the disabled/safe state for saving power or some security
reason.
The only time item #1 above would happen is an error case. If there's an
error, there shouldn't be any expectation for the specific state of the
pinmux. If this intermediate state is illegal, then that's a problem in
an of itself; the HW is going to be in that state for some (admittedly
small) amount of time while the pinmux is being programmed, error or no
error, hence all the intermediate states had better be legal. I think
it's fine if the HW programming is simply left in whatever partial state
the code managed to get to. It's quite unlikely there's any safe state
that's /meaningfully/ better for a pin to be in once an error is
detected.
Thus, I think we should keep the disable_pinmux_setting
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 f...@marvell.com
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 = GPIO48 AF6;
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 f...@marvell.com
---
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);
+
Re: [PATCH] pinctrl: add params in disable_setting for different usage
On 05/20/2014 04:55 AM, Stephen Warren wrote:
On 05/18/2014 08:54 PM, FanWu wrote:
On 05/17/2014 03:53 AM, Stephen Warren wrote:
On 05/16/2014 10:21 AM, Linus Walleij wrote:
On Wed, May 14, 2014 at 4:01 AM, wrote:
From: Fan Wu
The patch added params in disable_setting to differ the two possible
usage,
1.Only want to disable the pin setting in SW aspect, param can be
set to "0"
2.Want to disable the pin setting in both HW and SW aspect, param
can be set to "1";
The reason why to do this is that:
To avoid duplicated enable_setting operation without disabling
operation which will
let Pin's desc->mux_usecount keep being added.
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 the two state is same, like the
following one:
component a {
pinctrl-names = "default", "sleep";
pinctrl-0 = <_grp_setting _grp_setting>;
pinctrl-1 = <_grp_setting _grp_setting>;
}
The "c_grp_setting" config node is totaly same, maybe like following
one:
Hm this is a quite interesting thing if we can get it in place, but
I need Stephen's consent, also Tony should have a look at this as
I know he's had the same problem as you in pinctrl-single.
I only briefly looked at the patch, but it probably solves/hides the
immediate problem.
However, rather than doing this, why not just remove
pinmux_disable_setting() completely. It doesn't make sense to "disable a
mux selection" (some value is always selected in the mux register field)
any more than it does to "disable a drive strength selection". We don't
have a pinconf_disable_setting(), and couldn't really add one if we
wanted. For consistency, let's just remove pinmux_disable_setting(). Do
you agree?
Dear, Stephen and Guys,
Sorry for late due to some personal affairs in Weekend time.
I don't think it is a proper way to remove pinmux_disable_setting
directly without changing any other code, like no change on the code in
pinmux_enable_setting.
Talking about the pinmux_disable_operation, in SW aspect, we also need
to consider the "pinmux_enable_setting" operation.
For the "pinmux_enable_setting" operation, there is some SW level code
logic, like pin_request.
Do you think it is a acceptable way to remove the SW level code logic
from the "pinmux_enable_setting" path, because there will be no
corresponding operation in reverse order in pinmux_disable_setting after
applying our possible change?
No, I don't think we should remove the SW aspects of
pinmux_enable_setting(). The pinctrl core currently tracks which pinctrl
state "owns" each pingroup's mux, so that different pinctrl states can't
both attempt to configure a pingroup's mux setting. We need to keep all
the SW aspects of mux enable/disable. All I'm proposing removing is the
HW-programming parts of pinmux_disable_setting().
At least, I think this way may be a considerable change in Pinmux
framework, right?
Yes, removing all of pinmux_en/disable_setting would be a considerable
and likely inappropriate change.
Talking about HW aspect,
I think the solution you mentioned is indeed a good way to solve the
problem for some HW vendor's SoC chip, but may be not that intact solution.
In my understanding, the pinmux operation, like enabling and disabling,
is to change pin's(pins') multi-function from funcion_M to function_N.
And, the "pinconf" enabling function is used to change the attributes of
the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
The pinmux disabling operation will be called in the following case in
current pinmux framework:
1. when pin(s) is/are freed or error handler when configure it(them) and
finally the pin will be changed to a disabled/safe state if defined by
vendor.
2. in the pinctrl_select_state function
The item 2# is just the thing we talked about in this loop and we reach
agreement that the item 2# is not useful.
I think the item 1# is still useful for some vendor if they defined the
disabled/safe multi-function for a pin. They may expect the pin is
changed to the disabled/safe state for saving power or some security
reason.
The only time item #1 above would happen is an error case. If there's an
error, there shouldn't be any expectation for the specific state of the
pinmux. If this intermediate state is illegal, then that's a problem in
an of itself; the HW is going to be in that state for some (admittedly
small) amount of time while the pinmux is being programmed, error or no
error, hence all the intermediate states had better be legal. I think
it's fine if the HW programming is simply left in whatever partial state
the code managed to get to. It's quite unlikely there's any "safe" state
that's /meaningfully/ better for a pin to be in once an error is
Re: [PATCH] pinctrl: add params in disable_setting for different usage
On 05/20/2014 04:55 AM, Stephen Warren wrote:
On 05/18/2014 08:54 PM, FanWu wrote:
On 05/17/2014 03:53 AM, Stephen Warren wrote:
On 05/16/2014 10:21 AM, Linus Walleij wrote:
On Wed, May 14, 2014 at 4:01 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
The patch added params in disable_setting to differ the two possible
usage,
1.Only want to disable the pin setting in SW aspect, param can be
set to 0
2.Want to disable the pin setting in both HW and SW aspect, param
can be set to 1;
The reason why to do this is that:
To avoid duplicated enable_setting operation without disabling
operation which will
let Pin's desc-mux_usecount keep being added.
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 the two state is same, 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:
Hm this is a quite interesting thing if we can get it in place, but
I need Stephen's consent, also Tony should have a look at this as
I know he's had the same problem as you in pinctrl-single.
I only briefly looked at the patch, but it probably solves/hides the
immediate problem.
However, rather than doing this, why not just remove
pinmux_disable_setting() completely. It doesn't make sense to disable a
mux selection (some value is always selected in the mux register field)
any more than it does to disable a drive strength selection. We don't
have a pinconf_disable_setting(), and couldn't really add one if we
wanted. For consistency, let's just remove pinmux_disable_setting(). Do
you agree?
Dear, Stephen and Guys,
Sorry for late due to some personal affairs in Weekend time.
I don't think it is a proper way to remove pinmux_disable_setting
directly without changing any other code, like no change on the code in
pinmux_enable_setting.
Talking about the pinmux_disable_operation, in SW aspect, we also need
to consider the pinmux_enable_setting operation.
For the pinmux_enable_setting operation, there is some SW level code
logic, like pin_request.
Do you think it is a acceptable way to remove the SW level code logic
from the pinmux_enable_setting path, because there will be no
corresponding operation in reverse order in pinmux_disable_setting after
applying our possible change?
No, I don't think we should remove the SW aspects of
pinmux_enable_setting(). The pinctrl core currently tracks which pinctrl
state owns each pingroup's mux, so that different pinctrl states can't
both attempt to configure a pingroup's mux setting. We need to keep all
the SW aspects of mux enable/disable. All I'm proposing removing is the
HW-programming parts of pinmux_disable_setting().
At least, I think this way may be a considerable change in Pinmux
framework, right?
Yes, removing all of pinmux_en/disable_setting would be a considerable
and likely inappropriate change.
Talking about HW aspect,
I think the solution you mentioned is indeed a good way to solve the
problem for some HW vendor's SoC chip, but may be not that intact solution.
In my understanding, the pinmux operation, like enabling and disabling,
is to change pin's(pins') multi-function from funcion_M to function_N.
And, the pinconf enabling function is used to change the attributes of
the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
The pinmux disabling operation will be called in the following case in
current pinmux framework:
1. when pin(s) is/are freed or error handler when configure it(them) and
finally the pin will be changed to a disabled/safe state if defined by
vendor.
2. in the pinctrl_select_state function
The item 2# is just the thing we talked about in this loop and we reach
agreement that the item 2# is not useful.
I think the item 1# is still useful for some vendor if they defined the
disabled/safe multi-function for a pin. They may expect the pin is
changed to the disabled/safe state for saving power or some security
reason.
The only time item #1 above would happen is an error case. If there's an
error, there shouldn't be any expectation for the specific state of the
pinmux. If this intermediate state is illegal, then that's a problem in
an of itself; the HW is going to be in that state for some (admittedly
small) amount of time while the pinmux is being programmed, error or no
error, hence all the intermediate states had better be legal. I think
it's fine if the HW programming is simply left in whatever partial state
the code managed to get to. It's quite unlikely there's any safe state
that's /meaningfully/ better for a pin to be in once an error is detected.
Thus, I think we should keep the disable_pinmux_setting in pinmux code.
Do you think what I mentioned is an acceptable and not that aggressive
solution
Re: [PATCH] pinctrl: add params in disable_setting for different usage
On 05/17/2014 03:53 AM, Stephen Warren wrote:
On 05/16/2014 10:21 AM, Linus Walleij wrote:
On Wed, May 14, 2014 at 4:01 AM, wrote:
From: Fan Wu
The patch added params in disable_setting to differ the two possible usage,
1.Only want to disable the pin setting in SW aspect, param can be set to "0"
2.Want to disable the pin setting in both HW and SW aspect, param can be set to
"1";
The reason why to do this is that:
To avoid duplicated enable_setting operation without disabling operation which
will
let Pin's desc->mux_usecount keep being added.
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 the two state is same, like the following
one:
component a {
pinctrl-names = "default", "sleep";
pinctrl-0 = <_grp_setting _grp_setting>;
pinctrl-1 = <_grp_setting _grp_setting>;
}
The "c_grp_setting" config node is totaly same, maybe like following one:
Hm this is a quite interesting thing if we can get it in place, but
I need Stephen's consent, also Tony should have a look at this as
I know he's had the same problem as you in pinctrl-single.
I only briefly looked at the patch, but it probably solves/hides the
immediate problem.
However, rather than doing this, why not just remove
pinmux_disable_setting() completely. It doesn't make sense to "disable a
mux selection" (some value is always selected in the mux register field)
any more than it does to "disable a drive strength selection". We don't
have a pinconf_disable_setting(), and couldn't really add one if we
wanted. For consistency, let's just remove pinmux_disable_setting(). Do
you agree?
Dear, Stephen and Guys,
Sorry for late due to some personal affairs in Weekend time.
I don't think it is a proper way to remove pinmux_disable_setting
directly without changing any other code, like no change on the code in
pinmux_enable_setting.
Talking about the pinmux_disable_operation, in SW aspect, we also need
to consider the "pinmux_enable_setting" operation.
For the "pinmux_enable_setting" operation, there is some SW level code
logic, like pin_request.
Do you think it is a acceptable way to remove the SW level code logic
from the "pinmux_enable_setting" path, because there will be no
corresponding operation in reverse order in pinmux_disable_setting after
applying our possible change?
At least, I think this way may be a considerable change in Pinmux
framework, right?
Talking about HW aspect,
I think the solution you mentioned is indeed a good way to solve the
problem for some HW vendor's SoC chip, but may be not that intact solution.
In my understanding, the pinmux operation, like enabling and disabling,
is to change pin's(pins') multi-function from funcion_M to function_N.
And, the "pinconf" enabling function is used to change the attributes of
the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
The pinmux disabling operation will be called in the following case in
current pinmux framework:
1. when pin(s) is/are freed or error handler when configure it(them) and
finally the pin will be changed to a disabled/safe state if defined by
vendor.
2. in the pinctrl_select_state function
The item 2# is just the thing we talked about in this loop and we reach
agreement that the item 2# is not useful.
I think the item 1# is still useful for some vendor if they defined the
disabled/safe multi-function for a pin. They may expect the pin is
changed to the disabled/safe state for saving power or some security
reason.
Thus, I think we should keep the disable_pinmux_setting in pinmux code.
Do you think what I mentioned is an acceptable and not that aggressive
solution?
Please correct me if anything wrong.
For another topics:
1. There is no disable_pinconf: I think this is a issue. When the pin's
mux setting is changed, the pinconf setting should also be changed, at
least, the pinmux code here should offer the user a chance(interface) to
decide whether to change the pinconf setting. Thus, we may need to add
pinconf disable function.
2. If the vendor use pinctrl-single driver, the
"pinctrl-single,function-off" implementation is not useful in practical
usage. The "pinctrl-single,function-off" is parsed when pinctrl-single
driver probe phase and the instance setting of
"pinctrl-single,function-off" will be used for all pins setting.
Practically, I think different pins may have different disabled/safe.
Great thanks for this long term discussion :)
--
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] pinctrl: add params in disable_setting for different usage
On 05/17/2014 03:53 AM, Stephen Warren wrote:
On 05/16/2014 10:21 AM, Linus Walleij wrote:
On Wed, May 14, 2014 at 4:01 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
The patch added params in disable_setting to differ the two possible usage,
1.Only want to disable the pin setting in SW aspect, param can be set to 0
2.Want to disable the pin setting in both HW and SW aspect, param can be set to
1;
The reason why to do this is that:
To avoid duplicated enable_setting operation without disabling operation which
will
let Pin's desc-mux_usecount keep being added.
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 the two state is same, 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:
Hm this is a quite interesting thing if we can get it in place, but
I need Stephen's consent, also Tony should have a look at this as
I know he's had the same problem as you in pinctrl-single.
I only briefly looked at the patch, but it probably solves/hides the
immediate problem.
However, rather than doing this, why not just remove
pinmux_disable_setting() completely. It doesn't make sense to disable a
mux selection (some value is always selected in the mux register field)
any more than it does to disable a drive strength selection. We don't
have a pinconf_disable_setting(), and couldn't really add one if we
wanted. For consistency, let's just remove pinmux_disable_setting(). Do
you agree?
Dear, Stephen and Guys,
Sorry for late due to some personal affairs in Weekend time.
I don't think it is a proper way to remove pinmux_disable_setting
directly without changing any other code, like no change on the code in
pinmux_enable_setting.
Talking about the pinmux_disable_operation, in SW aspect, we also need
to consider the pinmux_enable_setting operation.
For the pinmux_enable_setting operation, there is some SW level code
logic, like pin_request.
Do you think it is a acceptable way to remove the SW level code logic
from the pinmux_enable_setting path, because there will be no
corresponding operation in reverse order in pinmux_disable_setting after
applying our possible change?
At least, I think this way may be a considerable change in Pinmux
framework, right?
Talking about HW aspect,
I think the solution you mentioned is indeed a good way to solve the
problem for some HW vendor's SoC chip, but may be not that intact solution.
In my understanding, the pinmux operation, like enabling and disabling,
is to change pin's(pins') multi-function from funcion_M to function_N.
And, the pinconf enabling function is used to change the attributes of
the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
The pinmux disabling operation will be called in the following case in
current pinmux framework:
1. when pin(s) is/are freed or error handler when configure it(them) and
finally the pin will be changed to a disabled/safe state if defined by
vendor.
2. in the pinctrl_select_state function
The item 2# is just the thing we talked about in this loop and we reach
agreement that the item 2# is not useful.
I think the item 1# is still useful for some vendor if they defined the
disabled/safe multi-function for a pin. They may expect the pin is
changed to the disabled/safe state for saving power or some security
reason.
Thus, I think we should keep the disable_pinmux_setting in pinmux code.
Do you think what I mentioned is an acceptable and not that aggressive
solution?
Please correct me if anything wrong.
For another topics:
1. There is no disable_pinconf: I think this is a issue. When the pin's
mux setting is changed, the pinconf setting should also be changed, at
least, the pinmux code here should offer the user a chance(interface) to
decide whether to change the pinconf setting. Thus, we may need to add
pinconf disable function.
2. If the vendor use pinctrl-single driver, the
pinctrl-single,function-off implementation is not useful in practical
usage. The pinctrl-single,function-off is parsed when pinctrl-single
driver probe phase and the instance setting of
pinctrl-single,function-off will be used for all pins setting.
Practically, I think different pins may have different disabled/safe.
Great thanks for this long term discussion :)
--
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] pinctrl: add params in disable_setting to differ difference usage
On 05/14/2014 09:54 AM, f...@marvell.com wrote:
From: Fan Wu
The patch added params in disable_setting to differ the two possible usage,
1.Only want to disable the pin setting in SW aspect, param can be set to "0"
2.Want to disable the pin setting in both HW and SW aspect, param can be set to
"1";
The reason why to do this is that:
To avoid duplicated enable_setting operation without disabling operation which
will
let Pin's desc->mux_usecount keep being added.
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 the two state is same, like the following
one:
component a {
pinctrl-names = "default", "sleep";
pinctrl-0 = <_grp_setting _grp_setting>;
pinctrl-1 = <_grp_setting _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 switch 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 change 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, due to the original reason mentioned in this patch.
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 ahead of
enable 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" in my understanding.
Conclusion:
Thus, the patch handle the issue mentioned above by disabling the c_grp_setting
in SW asepct without
touch the HW corresponding register to avoid unnecessary Pin's mux
function change.
Change-Id: Ib3f7e7b6d4b401796733f5fbf52da73973f2efff
Signed-off-by: Fan Wu
---
drivers/pinctrl/core.c | 17 +++--
drivers/pinctrl/pinmux.c |4 ++--
drivers/pinctrl/pinmux.h |2 +-
3 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..42877c3 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -891,7 +891,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:
@@ -998,20 +998,9 @@ int pinctrl_select_state(struct pinctrl *p, struct
pinctrl_state *state)
* 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) {
- found = true;
- break;
- }
- }
- if (!found)
- pinmux_disable_setting(setting);
+ pinmux_disable_setting(setting, 0);
}
}
@@ -1055,7 +1044,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
Re: [PATCH] pinctrl: add params in disable_setting to differ difference usage
On 05/14/2014 09:54 AM, f...@marvell.com wrote:
From: Fan Wu f...@marvell.com
The patch added params in disable_setting to differ the two possible usage,
1.Only want to disable the pin setting in SW aspect, param can be set to 0
2.Want to disable the pin setting in both HW and SW aspect, param can be set to
1;
The reason why to do this is that:
To avoid duplicated enable_setting operation without disabling operation which
will
let Pin's desc-mux_usecount keep being added.
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 the two state is same, 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 = GPIO48 AF6;
MFP_DEFAULT;
}
3)When switch 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 change 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, due to the original reason mentioned in this patch.
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 ahead of
enable 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 in my understanding.
Conclusion:
Thus, the patch handle the issue mentioned above by disabling the c_grp_setting
in SW asepct without
touch the HW corresponding register to avoid unnecessary Pin's mux
function change.
Change-Id: Ib3f7e7b6d4b401796733f5fbf52da73973f2efff
Signed-off-by: Fan Wu f...@marvell.com
---
drivers/pinctrl/core.c | 17 +++--
drivers/pinctrl/pinmux.c |4 ++--
drivers/pinctrl/pinmux.h |2 +-
3 files changed, 6 insertions(+), 17 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index c0fe609..42877c3 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -891,7 +891,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:
@@ -998,20 +998,9 @@ int pinctrl_select_state(struct pinctrl *p, struct
pinctrl_state *state)
* 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, 0);
}
}
@@ -1055,7 +1044,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
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/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 !=
PI
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
