[PATCH v6] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
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
4.Remove all the disable ops users in current code base.
Notes:
1.Great thanks for the suggestion from Linus, Tony Lindgren and Stephen Warren
and Everyone that shared comments on this patch.
2.The patch also includes comment fixes from Stephen Warren.
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();
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
Acked-by: Stephen Warren
Acked-by: Patrice Chotard
Acked-by: Heiko Stuebner
Acked-by: Maxime Coquelin
---
drivers/pinctrl/core.c| 24 +++---
drivers/pinctrl/pinctrl-abx500.c | 15 -
drivers/pinctrl/pinctrl-adi2.c| 30 -
drivers/pinctrl/pinctrl-at91.c| 21
drivers/pinctrl/pinctrl-bcm2835.c | 11 ---
drivers/pinctrl/pinctrl-exynos5440.c |8 -
drivers/pinctrl/pinctrl-msm.c | 25 --
drivers/pinctrl/pinctrl-nomadik.c | 16 -
drivers/pinctrl/pinctrl-rockchip.c| 18 --
drivers/pinctrl/pinctrl-samsung.c |8 -
drivers/pinctrl/pinctrl-single.c | 56 ---
drivers/pinctrl/pinctrl-st.c |6
drivers/pinctrl/pinctrl-tb10x.c | 17 --
drivers/pinctrl/pinctrl-tegra.c | 13
drivers/pinctrl/pinctrl-tz1090-pdc.c | 28
drivers/pinctrl/pinctrl-tz1090.c | 58 -
drivers/pinctrl/pinctrl-u300.c| 14
drivers/pinctrl/pinmux.c |4 ---
drivers/pinctrl/sh-pfc/pinctrl.c | 22 -
drivers/pinctrl/sirf/pinctrl-sirf.c | 10 --
drivers/pinctrl/spear/pinctrl-spear.c |7
drivers/pinctrl/vt8500/pinctrl-wmt.c | 12 ---
include/linux/pinctrl/pinmux.h|2 --
23 files changed, 5 insertions(+), 420 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
[PATCH v6] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
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
4.Remove all the disable ops users in current code base.
Notes:
1.Great thanks for the suggestion from Linus, Tony Lindgren and Stephen Warren
and Everyone that shared comments on this patch.
2.The patch also includes comment fixes from Stephen Warren.
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();
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
Acked-by: Stephen Warren swar...@nvidia.com
Acked-by: Patrice Chotard patrice.chot...@st.com
Acked-by: Heiko Stuebner he...@sntech.de
Acked-by: Maxime Coquelin maxime.coque...@st.com
---
drivers/pinctrl/core.c| 24 +++---
drivers/pinctrl/pinctrl-abx500.c | 15 -
drivers/pinctrl/pinctrl-adi2.c| 30 -
drivers/pinctrl/pinctrl-at91.c| 21
drivers/pinctrl/pinctrl-bcm2835.c | 11 ---
drivers/pinctrl/pinctrl-exynos5440.c |8 -
drivers/pinctrl/pinctrl-msm.c | 25 --
drivers/pinctrl/pinctrl-nomadik.c | 16 -
drivers/pinctrl/pinctrl-rockchip.c| 18 --
drivers/pinctrl/pinctrl-samsung.c |8 -
drivers/pinctrl/pinctrl-single.c | 56 ---
drivers/pinctrl/pinctrl-st.c |6
drivers/pinctrl/pinctrl-tb10x.c | 17 --
drivers/pinctrl/pinctrl-tegra.c | 13
drivers/pinctrl/pinctrl-tz1090-pdc.c | 28
drivers/pinctrl/pinctrl-tz1090.c | 58 -
drivers/pinctrl/pinctrl-u300.c| 14
drivers/pinctrl/pinmux.c |4 ---
drivers/pinctrl/sh-pfc/pinctrl.c | 22 -
drivers/pinctrl/sirf/pinctrl-sirf.c | 10 --
drivers/pinctrl/spear/pinctrl-spear.c |7
drivers/pinctrl/vt8500/pinctrl-wmt.c | 12 ---
include/linux/pinctrl/pinmux.h|2 --
23 files changed, 5 insertions(+), 420
[PATCH v5] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
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
4.Remove all the disable ops users in current code base.
Notes:
1.Great thanks for the suggestion from Linus, Tony Lindgren and Stephen Warren.
2.The patch also includes comment fixes from Stephen Warren.
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
---
drivers/pinctrl/core.c| 24 +++---
drivers/pinctrl/pinctrl-abx500.c | 15 -
drivers/pinctrl/pinctrl-adi2.c| 30 -
drivers/pinctrl/pinctrl-at91.c| 21
drivers/pinctrl/pinctrl-bcm2835.c | 11 ---
drivers/pinctrl/pinctrl-exynos5440.c |8 -
drivers/pinctrl/pinctrl-msm.c | 25 --
drivers/pinctrl/pinctrl-nomadik.c | 16 -
drivers/pinctrl/pinctrl-rockchip.c| 18 --
drivers/pinctrl/pinctrl-samsung.c |8 -
drivers/pinctrl/pinctrl-single.c | 56 ---
drivers/pinctrl/pinctrl-st.c |6
drivers/pinctrl/pinctrl-tb10x.c | 17 --
drivers/pinctrl/pinctrl-tegra.c | 19 ---
drivers/pinctrl/pinctrl-tz1090-pdc.c | 28
drivers/pinctrl/pinctrl-tz1090.c | 58 -
drivers/pinctrl/pinctrl-u300.c| 14
drivers/pinctrl/pinmux.c |4 ---
drivers/pinctrl/sh-pfc/pinctrl.c | 22 -
drivers/pinctrl/sirf/pinctrl-sirf.c | 10 --
drivers/pinctrl/spear/pinctrl-spear.c |7
drivers/pinctrl/vt8500/pinctrl-wmt.c | 12 ---
include/linux/pinctrl/pinmux.h|2 --
23 files changed, 5 insertions(+), 426 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
[PATCH v5] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
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
4.Remove all the disable ops users in current code base.
Notes:
1.Great thanks for the suggestion from Linus, Tony Lindgren and Stephen Warren.
2.The patch also includes comment fixes from Stephen Warren.
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
---
drivers/pinctrl/core.c| 24 +++---
drivers/pinctrl/pinctrl-abx500.c | 15 -
drivers/pinctrl/pinctrl-adi2.c| 30 -
drivers/pinctrl/pinctrl-at91.c| 21
drivers/pinctrl/pinctrl-bcm2835.c | 11 ---
drivers/pinctrl/pinctrl-exynos5440.c |8 -
drivers/pinctrl/pinctrl-msm.c | 25 --
drivers/pinctrl/pinctrl-nomadik.c | 16 -
drivers/pinctrl/pinctrl-rockchip.c| 18 --
drivers/pinctrl/pinctrl-samsung.c |8 -
drivers/pinctrl/pinctrl-single.c | 56 ---
drivers/pinctrl/pinctrl-st.c |6
drivers/pinctrl/pinctrl-tb10x.c | 17 --
drivers/pinctrl/pinctrl-tegra.c | 19 ---
drivers/pinctrl/pinctrl-tz1090-pdc.c | 28
drivers/pinctrl/pinctrl-tz1090.c | 58 -
drivers/pinctrl/pinctrl-u300.c| 14
drivers/pinctrl/pinmux.c |4 ---
drivers/pinctrl/sh-pfc/pinctrl.c | 22 -
drivers/pinctrl/sirf/pinctrl-sirf.c | 10 --
drivers/pinctrl/spear/pinctrl-spear.c |7
drivers/pinctrl/vt8500/pinctrl-wmt.c | 12 ---
include/linux/pinctrl/pinmux.h|2 --
23 files changed, 5 insertions(+), 426 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
[PATCH v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
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 pinmux_disable_setting(struct pinctrl_setting const
[PATCH v4] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
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
+++ b/drivers/pinctrl/pinmux.c
@@ -469,7 +469,6 @@ void
[PATCH v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
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 ==
-
[PATCH v3] pinctrl: to avoid duplicated calling enable_pinmux_setting for a pin
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 ==
-
[PATCH v2] pinctrl: add params in disable_setting for different usage
From: Fan Wu
What the patch did:
1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time of
calling pinctrl_select_state
2.Remove the HW disable operation in in pinmux_disable_setting function.
The reason why to do this is that:
1.To avoid duplicated enable_setting operation without disabling operation
which will let Pin's desc->mux_usecount keep being added.
2.The HW pin disable operation is not useful for most of the vendors' platform.
And this avoid the HW glitch after using the item 1# modification.
In the following case, the issue can be reproduced:
1)There is a driver need to switch Pin state dynamicly, E.g. b/t "sleep" and
"default" state
2)The Pin setting configuration in DTS node may be like the following one:
component a {
pinctrl-names = "default", "sleep";
pinctrl-0 = <_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);
+ pinmux_disable_setting(setting);
}
[PATCH v2] pinctrl: add params in disable_setting for different usage
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);
+ pinmux_disable_setting(setting);
}
[PATCH] pinctrl: add params in disable_setting for different usage
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 recursive loop here because p->state is NULL */
diff --git
[PATCH] pinctrl: add params in disable_setting for different usage
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 recursive loop here because p-state is NULL */
diff
