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/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? -- 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/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? -- 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) -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: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 == -
[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 == -
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