lkml.org 
[lkml]   [2014]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] pinctrl: add params in disable_setting for different usage
On 05/22/2014 11:10 AM, fwu@marvell.com wrote:
> From: Fan Wu <fwu@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 <fwu@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);
> }
> }
>
> 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 *setting)
> {
> struct pinctrl_dev *pctldev = setting->pctldev;
> const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
> - const struct pinmux_ops *ops = pctldev->desc->pmxops;
> int ret;
> const unsigned *pins;
> unsigned num_pins;
> @@ -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);
> }
>
> #ifdef CONFIG_DEBUG_FS
>

Dear Guys,

Just as we talked, for my platform, I have no concern about removing the
HW disabling operation in the pinmux_disable_setting.

Thus, I filed a new version of the patch we discussed before.
Just as the patch comments said, the patch did:
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.

Could you please help to review the above patch?


If there is no concern about the above solution from you Guys, I think I
will try to do the other two patches:

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"(include/linux/pinctrl/pinmux.h)

Is that OK for you about my plan ?


\
 
 \ /
  Last update: 2014-05-22 06:21    [W:0.065 / U:0.364 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site