Date:	Wed, 14 May 2014 10:12:18 +0800
From:	FanWu <fwu@...vell.com>
To:	<linus.walleij@...aro.org>, <swarren@...dotorg.org>
CC:	<linux-kernel@...r.kernel.org>, <swarren@...dia.com>,
	Chao Xie <cxie4@...vell.com>, Yilu Mao <ylmao@...vell.com>,
	Ning Jiang <njiang1@...vell.com>,
	Xiaofan Tian <tianxf@...vell.com>,
	Fangsuo Wu <fswu@...vell.com>
Subject: Re: [PATCH] pinctrl: add params in disable_setting to differ difference
 usage

On 05/14/2014 09:54 AM, fwu@...vell.com wrote:
> From: Fan Wu <fwu@...vell.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 <fwu@...vell.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 --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index 9248ce4..88544d4 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -465,7 +465,7 @@ err_pin_request:
>   	return ret;
>   }
>
> -void pinmux_disable_setting(struct pinctrl_setting const *setting)
> +void pinmux_disable_setting(struct pinctrl_setting const *setting, int hwops)
>   {
>   	struct pinctrl_dev *pctldev = setting->pctldev;
>   	const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
> @@ -516,7 +516,7 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting)
>   		}
>   	}
>
> -	if (ops->disable)
> +	if (ops->disable && hwops)
>   		ops->disable(pctldev, setting->data.mux.func, setting->data.mux.group);
>   }
>
> diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
> index d1a98b1..cd3a4af 100644
> --- a/drivers/pinctrl/pinmux.h
> +++ b/drivers/pinctrl/pinmux.h
> @@ -29,7 +29,7 @@ int pinmux_map_to_setting(struct pinctrl_map const *map,
>   			  struct pinctrl_setting *setting);
>   void pinmux_free_setting(struct pinctrl_setting const *setting);
>   int pinmux_enable_setting(struct pinctrl_setting const *setting);
> -void pinmux_disable_setting(struct pinctrl_setting const *setting);
> +void pinmux_disable_setting(struct pinctrl_setting const *setting, int hwops);
>
>   #else
>
>

Dear Stephen,

I think we reach agreement on this topic that it is possible that a 
Pin's mux function may be changed if we disable the old setting. We can 
ignore the understanding about the "glitch" because the glitch I 
mentioned just means that the unexpected Pin's mux function change.

Just as the above patch expressed, I think we need to do SW pinmux 
setting disabling for all of the setting in old state ahead enabling 
setting in new state in pinctrl_select_state function.
Only when the user really wants to disable Pin's setting, like the 
disabling operation in "pinctrl_free_setting" function, we can disable 
the pin's mux setting in HW aspect.

Do you think this is a acceptable solution for the topic we discussed ?

Great thanks for this !

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists