[PATCH] pinctrl: add params in disable_setting for different usage

[PATCH] pinctrl: add params in disable_setting for different usage

[fwu]

From: Fan Wu <fwu@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 <fwu@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 --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
 
-- 
1.7.9.5

Re: [PATCH] pinctrl: add params in disable_setting for different usage

[Linus Walleij]

On Wed, May 14, 2014 at 4:01 AM,  <fwu@marvell.com> wrote:

> From: Fan Wu <fwu@marvell.com>
>
> The patch added params in disable_setting to differ the two possible usage,
> 1.Only want to disable the pin setting in SW aspect, param can be set to "0"
> 2.Want to disable the pin setting in both HW and SW aspect, param can be set to "1";
>
> The reason why to do this is that:
> To avoid duplicated enable_setting operation without disabling operation which will
> let Pin's desc->mux_usecount keep being added.
>
> In the following case, the issue can be reproduced:
> 1)There is a driver need to switch Pin state dynamicly, E.g. b/t "sleep" and
> "default" state
> 2)The Pin setting configuration in the two state is same, like the following one:
> component a {
>         pinctrl-names = "default", "sleep";
>         pinctrl-0 = <&a_grp_setting &c_grp_setting>;
>         pinctrl-1 = <&b_grp_setting &c_grp_setting>;
> }
> The "c_grp_setting" config node is totaly same, maybe like following one:

Hm this is a quite interesting thing if we can get it in place, but
I need Stephen's consent, also Tony should have a look at this as
I know he's had the same problem as you in pinctrl-single.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: add params in disable_setting for different usage

[Stephen Warren]

On 05/16/2014 10:21 AM, Linus Walleij wrote:
> On Wed, May 14, 2014 at 4:01 AM,  <fwu@marvell.com> wrote:
> 
>> From: Fan Wu <fwu@marvell.com>
>>
>> The patch added params in disable_setting to differ the two possible usage,
>> 1.Only want to disable the pin setting in SW aspect, param can be set to "0"
>> 2.Want to disable the pin setting in both HW and SW aspect, param can be set to "1";
>>
>> The reason why to do this is that:
>> To avoid duplicated enable_setting operation without disabling operation which will
>> let Pin's desc->mux_usecount keep being added.
>>
>> In the following case, the issue can be reproduced:
>> 1)There is a driver need to switch Pin state dynamicly, E.g. b/t "sleep" and
>> "default" state
>> 2)The Pin setting configuration in the two state is same, like the following one:
>> component a {
>>         pinctrl-names = "default", "sleep";
>>         pinctrl-0 = <&a_grp_setting &c_grp_setting>;
>>         pinctrl-1 = <&b_grp_setting &c_grp_setting>;
>> }
>> The "c_grp_setting" config node is totaly same, maybe like following one:
> 
> Hm this is a quite interesting thing if we can get it in place, but
> I need Stephen's consent, also Tony should have a look at this as
> I know he's had the same problem as you in pinctrl-single.

I only briefly looked at the patch, but it probably solves/hides the
immediate problem.

However, rather than doing this, why not just remove
pinmux_disable_setting() completely. It doesn't make sense to "disable a
mux selection" (some value is always selected in the mux register field)
any more than it does to "disable a drive strength selection". We don't
have a pinconf_disable_setting(), and couldn't really add one if we
wanted. For consistency, let's just remove pinmux_disable_setting(). Do
you agree?

Re: [PATCH] pinctrl: add params in disable_setting for different usage

[FanWu]

On 05/17/2014 03:53 AM, Stephen Warren wrote:
> On 05/16/2014 10:21 AM, Linus Walleij wrote:
>> On Wed, May 14, 2014 at 4:01 AM,  <fwu@marvell.com> wrote:
>>
>>> From: Fan Wu <fwu@marvell.com>
>>>
>>> The patch added params in disable_setting to differ the two possible usage,
>>> 1.Only want to disable the pin setting in SW aspect, param can be set to "0"
>>> 2.Want to disable the pin setting in both HW and SW aspect, param can be set to "1";
>>>
>>> The reason why to do this is that:
>>> To avoid duplicated enable_setting operation without disabling operation which will
>>> let Pin's desc->mux_usecount keep being added.
>>>
>>> In the following case, the issue can be reproduced:
>>> 1)There is a driver need to switch Pin state dynamicly, E.g. b/t "sleep" and
>>> "default" state
>>> 2)The Pin setting configuration in the two state is same, like the following one:
>>> component a {
>>>          pinctrl-names = "default", "sleep";
>>>          pinctrl-0 = <&a_grp_setting &c_grp_setting>;
>>>          pinctrl-1 = <&b_grp_setting &c_grp_setting>;
>>> }
>>> The "c_grp_setting" config node is totaly same, maybe like following one:
>>
>> Hm this is a quite interesting thing if we can get it in place, but
>> I need Stephen's consent, also Tony should have a look at this as
>> I know he's had the same problem as you in pinctrl-single.
>
> I only briefly looked at the patch, but it probably solves/hides the
> immediate problem.
>
> However, rather than doing this, why not just remove
> pinmux_disable_setting() completely. It doesn't make sense to "disable a
> mux selection" (some value is always selected in the mux register field)
> any more than it does to "disable a drive strength selection". We don't
> have a pinconf_disable_setting(), and couldn't really add one if we
> wanted. For consistency, let's just remove pinmux_disable_setting(). Do
> you agree?
>

Dear, Stephen and Guys,

Sorry for late due to some personal affairs in Weekend time.

I don't think it is a proper way to remove pinmux_disable_setting 
directly without changing any other code, like no change on the code in 
pinmux_enable_setting.

Talking about the pinmux_disable_operation, in SW aspect, we also need 
to consider the "pinmux_enable_setting" operation.
For the "pinmux_enable_setting" operation, there is some SW level code 
logic, like pin_request.
Do you think it is a acceptable way to remove the SW level code logic 
from the "pinmux_enable_setting" path, because there will be no 
corresponding operation in reverse order in pinmux_disable_setting after 
applying our possible change?
At least, I think this way may be a considerable change in Pinmux 
framework, right?

Talking about HW aspect,
I think the solution you mentioned is indeed a good way to solve the 
problem for some HW vendor's SoC chip, but may be not that intact solution.

In my understanding, the pinmux operation, like enabling and disabling, 
is to change pin's(pins') multi-function from funcion_M to function_N.
And, the "pinconf" enabling function is used to change the attributes of 
the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.

The pinmux disabling operation will be called in the following case in 
current pinmux framework:
1. when pin(s) is/are freed or error handler when configure it(them) and 
finally the pin will be changed to a disabled/safe state if defined by 
vendor.
2. in the pinctrl_select_state function

The item 2# is just the thing we talked about in this loop and we reach 
agreement that the item 2# is not useful.

I think the item 1# is still useful for some vendor if they defined the 
disabled/safe multi-function for a pin. They may expect the pin is 
changed to the disabled/safe state for saving power or some security 
reason.

Thus, I think we should keep the disable_pinmux_setting in pinmux code.

Do you think what I mentioned is an acceptable and not that aggressive 
solution?

Please correct me if anything wrong.



For another topics:
1. There is no disable_pinconf: I think this is a issue. When the pin's 
mux setting is changed, the pinconf setting should also be changed, at 
least, the pinmux code here should offer the user a chance(interface) to 
decide whether to change the pinconf setting. Thus, we may need to add 
pinconf disable function.
2. If the vendor use pinctrl-single driver, the 
"pinctrl-single,function-off" implementation is not useful in practical 
usage. The "pinctrl-single,function-off" is parsed when pinctrl-single 
driver probe phase and the instance setting of 
"pinctrl-single,function-off" will be used for all pins setting. 
Practically, I think different pins may have different disabled/safe.

Great thanks for this long term discussion :)

Re: [PATCH] pinctrl: add params in disable_setting for different usage

[Stephen Warren]

On 05/18/2014 08:54 PM, FanWu wrote:
> On 05/17/2014 03:53 AM, Stephen Warren wrote:
>> On 05/16/2014 10:21 AM, Linus Walleij wrote:
>>> On Wed, May 14, 2014 at 4:01 AM,  <fwu@marvell.com> wrote:
>>>
>>>> From: Fan Wu <fwu@marvell.com>
>>>>
>>>> The patch added params in disable_setting to differ the two possible
>>>> usage,
>>>> 1.Only want to disable the pin setting in SW aspect, param can be
>>>> set to "0"
>>>> 2.Want to disable the pin setting in both HW and SW aspect, param
>>>> can be set to "1";
>>>>
>>>> The reason why to do this is that:
>>>> To avoid duplicated enable_setting operation without disabling
>>>> operation which will
>>>> let Pin's desc->mux_usecount keep being added.
>>>>
>>>> In the following case, the issue can be reproduced:
>>>> 1)There is a driver need to switch Pin state dynamicly, E.g. b/t
>>>> "sleep" and
>>>> "default" state
>>>> 2)The Pin setting configuration in the two state is same, like the
>>>> following one:
>>>> component a {
>>>>          pinctrl-names = "default", "sleep";
>>>>          pinctrl-0 = <&a_grp_setting &c_grp_setting>;
>>>>          pinctrl-1 = <&b_grp_setting &c_grp_setting>;
>>>> }
>>>> The "c_grp_setting" config node is totaly same, maybe like following
>>>> one:
>>>
>>> Hm this is a quite interesting thing if we can get it in place, but
>>> I need Stephen's consent, also Tony should have a look at this as
>>> I know he's had the same problem as you in pinctrl-single.
>>
>> I only briefly looked at the patch, but it probably solves/hides the
>> immediate problem.
>>
>> However, rather than doing this, why not just remove
>> pinmux_disable_setting() completely. It doesn't make sense to "disable a
>> mux selection" (some value is always selected in the mux register field)
>> any more than it does to "disable a drive strength selection". We don't
>> have a pinconf_disable_setting(), and couldn't really add one if we
>> wanted. For consistency, let's just remove pinmux_disable_setting(). Do
>> you agree?
>>
> 
> Dear, Stephen and Guys,
> 
> Sorry for late due to some personal affairs in Weekend time.
> 
> I don't think it is a proper way to remove pinmux_disable_setting
> directly without changing any other code, like no change on the code in
> pinmux_enable_setting.
> 
> Talking about the pinmux_disable_operation, in SW aspect, we also need
> to consider the "pinmux_enable_setting" operation.
> For the "pinmux_enable_setting" operation, there is some SW level code
> logic, like pin_request.
> Do you think it is a acceptable way to remove the SW level code logic
> from the "pinmux_enable_setting" path, because there will be no
> corresponding operation in reverse order in pinmux_disable_setting after
> applying our possible change?

No, I don't think we should remove the SW aspects of
pinmux_enable_setting(). The pinctrl core currently tracks which pinctrl
state "owns" each pingroup's mux, so that different pinctrl states can't
both attempt to configure a pingroup's mux setting. We need to keep all
the SW aspects of mux enable/disable. All I'm proposing removing is the
HW-programming parts of pinmux_disable_setting().

> At least, I think this way may be a considerable change in Pinmux
> framework, right?

Yes, removing all of pinmux_en/disable_setting would be a considerable
and likely inappropriate change.

> Talking about HW aspect,
> I think the solution you mentioned is indeed a good way to solve the
> problem for some HW vendor's SoC chip, but may be not that intact solution.
> 
> In my understanding, the pinmux operation, like enabling and disabling,
> is to change pin's(pins') multi-function from funcion_M to function_N.
> And, the "pinconf" enabling function is used to change the attributes of
> the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
> 
> The pinmux disabling operation will be called in the following case in
> current pinmux framework:
> 1. when pin(s) is/are freed or error handler when configure it(them) and
> finally the pin will be changed to a disabled/safe state if defined by
> vendor.
> 2. in the pinctrl_select_state function
> 
> The item 2# is just the thing we talked about in this loop and we reach
> agreement that the item 2# is not useful.
> 
> I think the item 1# is still useful for some vendor if they defined the
> disabled/safe multi-function for a pin. They may expect the pin is
> changed to the disabled/safe state for saving power or some security
> reason.

The only time item #1 above would happen is an error case. If there's an
error, there shouldn't be any expectation for the specific state of the
pinmux. If this intermediate state is illegal, then that's a problem in
an of itself; the HW is going to be in that state for some (admittedly
small) amount of time while the pinmux is being programmed, error or no
error, hence all the intermediate states had better be legal. I think
it's fine if the HW programming is simply left in whatever partial state
the code managed to get to. It's quite unlikely there's any "safe" state
that's /meaningfully/ better for a pin to be in once an error is detected.

> Thus, I think we should keep the disable_pinmux_setting in pinmux code.
> 
> Do you think what I mentioned is an acceptable and not that aggressive
> solution?

Not really.

> Please correct me if anything wrong.
> 
> 
> 
> For another topics:
> 1. There is no disable_pinconf: I think this is a issue. When the pin's
> mux setting is changed, the pinconf setting should also be changed, at
> least, the pinmux code here should offer the user a chance(interface) to
> decide whether to change the pinconf setting. Thus, we may need to add
> pinconf disable function.

pinctrl already allows any config options to be changed along with the
mux option.

The only reason any mux or config option is ever changed is in response
to selecting a new pinctrl state. Hence, I don't think you ever want to
"disable" either a mux or config option. Rather, you simply want to
"enable" or "select" or "program" the mux/config options in the new
state. Any mux/config option that needs to differ between states should
simply be listed in all the states, so that when the state is entered,
the appropriate HW programming is performed.

> 2. If the vendor use pinctrl-single driver, the
> "pinctrl-single,function-off" implementation is not useful in practical
> usage. The "pinctrl-single,function-off" is parsed when pinctrl-single
> driver probe phase and the instance setting of
> "pinctrl-single,function-off" will be used for all pins setting.
> Practically, I think different pins may have different disabled/safe.

I'm not sure what you're asking here.

Re: [PATCH] pinctrl: add params in disable_setting for different usage

[FanWu]

On 05/20/2014 04:55 AM, Stephen Warren wrote:
> On 05/18/2014 08:54 PM, FanWu wrote:
>> On 05/17/2014 03:53 AM, Stephen Warren wrote:
>>> On 05/16/2014 10:21 AM, Linus Walleij wrote:
>>>> On Wed, May 14, 2014 at 4:01 AM,  <fwu@marvell.com> wrote:
>>>>
>>>>> From: Fan Wu <fwu@marvell.com>
>>>>>
>>>>> The patch added params in disable_setting to differ the two possible
>>>>> usage,
>>>>> 1.Only want to disable the pin setting in SW aspect, param can be
>>>>> set to "0"
>>>>> 2.Want to disable the pin setting in both HW and SW aspect, param
>>>>> can be set to "1";
>>>>>
>>>>> The reason why to do this is that:
>>>>> To avoid duplicated enable_setting operation without disabling
>>>>> operation which will
>>>>> let Pin's desc->mux_usecount keep being added.
>>>>>
>>>>> In the following case, the issue can be reproduced:
>>>>> 1)There is a driver need to switch Pin state dynamicly, E.g. b/t
>>>>> "sleep" and
>>>>> "default" state
>>>>> 2)The Pin setting configuration in the two state is same, like the
>>>>> following one:
>>>>> component a {
>>>>>           pinctrl-names = "default", "sleep";
>>>>>           pinctrl-0 = <&a_grp_setting &c_grp_setting>;
>>>>>           pinctrl-1 = <&b_grp_setting &c_grp_setting>;
>>>>> }
>>>>> The "c_grp_setting" config node is totaly same, maybe like following
>>>>> one:
>>>>
>>>> Hm this is a quite interesting thing if we can get it in place, but
>>>> I need Stephen's consent, also Tony should have a look at this as
>>>> I know he's had the same problem as you in pinctrl-single.
>>>
>>> I only briefly looked at the patch, but it probably solves/hides the
>>> immediate problem.
>>>
>>> However, rather than doing this, why not just remove
>>> pinmux_disable_setting() completely. It doesn't make sense to "disable a
>>> mux selection" (some value is always selected in the mux register field)
>>> any more than it does to "disable a drive strength selection". We don't
>>> have a pinconf_disable_setting(), and couldn't really add one if we
>>> wanted. For consistency, let's just remove pinmux_disable_setting(). Do
>>> you agree?
>>>
>>
>> Dear, Stephen and Guys,
>>
>> Sorry for late due to some personal affairs in Weekend time.
>>
>> I don't think it is a proper way to remove pinmux_disable_setting
>> directly without changing any other code, like no change on the code in
>> pinmux_enable_setting.
>>
>> Talking about the pinmux_disable_operation, in SW aspect, we also need
>> to consider the "pinmux_enable_setting" operation.
>> For the "pinmux_enable_setting" operation, there is some SW level code
>> logic, like pin_request.
>> Do you think it is a acceptable way to remove the SW level code logic
>> from the "pinmux_enable_setting" path, because there will be no
>> corresponding operation in reverse order in pinmux_disable_setting after
>> applying our possible change?
>
> No, I don't think we should remove the SW aspects of
> pinmux_enable_setting(). The pinctrl core currently tracks which pinctrl
> state "owns" each pingroup's mux, so that different pinctrl states can't
> both attempt to configure a pingroup's mux setting. We need to keep all
> the SW aspects of mux enable/disable. All I'm proposing removing is the
> HW-programming parts of pinmux_disable_setting().
>
>> At least, I think this way may be a considerable change in Pinmux
>> framework, right?
>
> Yes, removing all of pinmux_en/disable_setting would be a considerable
> and likely inappropriate change.
>
>> Talking about HW aspect,
>> I think the solution you mentioned is indeed a good way to solve the
>> problem for some HW vendor's SoC chip, but may be not that intact solution.
>>
>> In my understanding, the pinmux operation, like enabling and disabling,
>> is to change pin's(pins') multi-function from funcion_M to function_N.
>> And, the "pinconf" enabling function is used to change the attributes of
>> the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
>>
>> The pinmux disabling operation will be called in the following case in
>> current pinmux framework:
>> 1. when pin(s) is/are freed or error handler when configure it(them) and
>> finally the pin will be changed to a disabled/safe state if defined by
>> vendor.
>> 2. in the pinctrl_select_state function
>>
>> The item 2# is just the thing we talked about in this loop and we reach
>> agreement that the item 2# is not useful.
>>
>> I think the item 1# is still useful for some vendor if they defined the
>> disabled/safe multi-function for a pin. They may expect the pin is
>> changed to the disabled/safe state for saving power or some security
>> reason.
>
> The only time item #1 above would happen is an error case. If there's an
> error, there shouldn't be any expectation for the specific state of the
> pinmux. If this intermediate state is illegal, then that's a problem in
> an of itself; the HW is going to be in that state for some (admittedly
> small) amount of time while the pinmux is being programmed, error or no
> error, hence all the intermediate states had better be legal. I think
> it's fine if the HW programming is simply left in whatever partial state
> the code managed to get to. It's quite unlikely there's any "safe" state
> that's /meaningfully/ better for a pin to be in once an error is detected.
>
>> Thus, I think we should keep the disable_pinmux_setting in pinmux code.
>>
>> Do you think what I mentioned is an acceptable and not that aggressive
>> solution?
>
> Not really.
>
>> Please correct me if anything wrong.
>>
>>
>>
>> For another topics:
>> 1. There is no disable_pinconf: I think this is a issue. When the pin's
>> mux setting is changed, the pinconf setting should also be changed, at
>> least, the pinmux code here should offer the user a chance(interface) to
>> decide whether to change the pinconf setting. Thus, we may need to add
>> pinconf disable function.
>
> pinctrl already allows any config options to be changed along with the
> mux option.
>
> The only reason any mux or config option is ever changed is in response
> to selecting a new pinctrl state. Hence, I don't think you ever want to
> "disable" either a mux or config option. Rather, you simply want to
> "enable" or "select" or "program" the mux/config options in the new
> state. Any mux/config option that needs to differ between states should
> simply be listed in all the states, so that when the state is entered,
> the appropriate HW programming is performed.
>
>> 2. If the vendor use pinctrl-single driver, the
>> "pinctrl-single,function-off" implementation is not useful in practical
>> usage. The "pinctrl-single,function-off" is parsed when pinctrl-single
>> driver probe phase and the instance setting of
>> "pinctrl-single,function-off" will be used for all pins setting.
>> Practically, I think different pins may have different disabled/safe.
>
> I'm not sure what you're asking here.
>

Dear Stephen,

I think we have reached the agreement that the HW operation should be 
avoided in disable_pinmux_setting. Just a little difference, I insist 
that the HW operation should only should be removed sometimes not always.

I think the disable_pinmux_setting is not only called by the error 
handler but also the 
"pinctrl_put=>pinctrl_release=>...=>"pinctrl_free_setting" call stack 
when there is no any alive user to use this pin.
In this case,
the pinmux_disable_setting will try to put the pin to a fixed and final 
state, not intermediate state, and should offer the vendor's driver an 
interface to place the pin to the unused(disabled/safe) state(HW aspect).

Thus, I think we should remove HW operation in pinmux_disabl_setting 
only for some cases, just same as what I mentioned in my patch.

Did I got anything wrong ?

Great thanks !

Re: [PATCH] pinctrl: add params in disable_setting for different usage

[Stephen Warren]

On 05/19/2014 09:05 PM, FanWu wrote:
> On 05/20/2014 04:55 AM, Stephen Warren wrote:
>> On 05/18/2014 08:54 PM, FanWu wrote:
>>> On 05/17/2014 03:53 AM, Stephen Warren wrote:
>>>> On 05/16/2014 10:21 AM, Linus Walleij wrote:
>>>>> On Wed, May 14, 2014 at 4:01 AM,  <fwu@marvell.com> wrote:
>>>>>
>>>>>> From: Fan Wu <fwu@marvell.com>
>>>>>>
>>>>>> The patch added params in disable_setting to differ the two possible
>>>>>> usage,
>>>>>> 1.Only want to disable the pin setting in SW aspect, param can be
>>>>>> set to "0"
>>>>>> 2.Want to disable the pin setting in both HW and SW aspect, param
>>>>>> can be set to "1";
>>>>>>
>>>>>> The reason why to do this is that:
>>>>>> To avoid duplicated enable_setting operation without disabling
>>>>>> operation which will
>>>>>> let Pin's desc->mux_usecount keep being added.
>>>>>>
>>>>>> In the following case, the issue can be reproduced:
>>>>>> 1)There is a driver need to switch Pin state dynamicly, E.g. b/t
>>>>>> "sleep" and
>>>>>> "default" state
>>>>>> 2)The Pin setting configuration in the two state is same, like the
>>>>>> following one:
>>>>>> component a {
>>>>>>           pinctrl-names = "default", "sleep";
>>>>>>           pinctrl-0 = <&a_grp_setting &c_grp_setting>;
>>>>>>           pinctrl-1 = <&b_grp_setting &c_grp_setting>;
>>>>>> }
>>>>>> The "c_grp_setting" config node is totaly same, maybe like following
>>>>>> one:
>>>>>
>>>>> Hm this is a quite interesting thing if we can get it in place, but
>>>>> I need Stephen's consent, also Tony should have a look at this as
>>>>> I know he's had the same problem as you in pinctrl-single.
>>>>
>>>> I only briefly looked at the patch, but it probably solves/hides the
>>>> immediate problem.
>>>>
>>>> However, rather than doing this, why not just remove
>>>> pinmux_disable_setting() completely. It doesn't make sense to
>>>> "disable a
>>>> mux selection" (some value is always selected in the mux register
>>>> field)
>>>> any more than it does to "disable a drive strength selection". We don't
>>>> have a pinconf_disable_setting(), and couldn't really add one if we
>>>> wanted. For consistency, let's just remove pinmux_disable_setting(). Do
>>>> you agree?
>>>>
>>>
>>> Dear, Stephen and Guys,
>>>
>>> Sorry for late due to some personal affairs in Weekend time.
>>>
>>> I don't think it is a proper way to remove pinmux_disable_setting
>>> directly without changing any other code, like no change on the code in
>>> pinmux_enable_setting.
>>>
>>> Talking about the pinmux_disable_operation, in SW aspect, we also need
>>> to consider the "pinmux_enable_setting" operation.
>>> For the "pinmux_enable_setting" operation, there is some SW level code
>>> logic, like pin_request.
>>> Do you think it is a acceptable way to remove the SW level code logic
>>> from the "pinmux_enable_setting" path, because there will be no
>>> corresponding operation in reverse order in pinmux_disable_setting after
>>> applying our possible change?
>>
>> No, I don't think we should remove the SW aspects of
>> pinmux_enable_setting(). The pinctrl core currently tracks which pinctrl
>> state "owns" each pingroup's mux, so that different pinctrl states can't
>> both attempt to configure a pingroup's mux setting. We need to keep all
>> the SW aspects of mux enable/disable. All I'm proposing removing is the
>> HW-programming parts of pinmux_disable_setting().
>>
>>> At least, I think this way may be a considerable change in Pinmux
>>> framework, right?
>>
>> Yes, removing all of pinmux_en/disable_setting would be a considerable
>> and likely inappropriate change.
>>
>>> Talking about HW aspect,
>>> I think the solution you mentioned is indeed a good way to solve the
>>> problem for some HW vendor's SoC chip, but may be not that intact
>>> solution.
>>>
>>> In my understanding, the pinmux operation, like enabling and disabling,
>>> is to change pin's(pins') multi-function from funcion_M to function_N.
>>> And, the "pinconf" enabling function is used to change the attributes of
>>> the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
>>>
>>> The pinmux disabling operation will be called in the following case in
>>> current pinmux framework:
>>> 1. when pin(s) is/are freed or error handler when configure it(them) and
>>> finally the pin will be changed to a disabled/safe state if defined by
>>> vendor.
>>> 2. in the pinctrl_select_state function
>>>
>>> The item 2# is just the thing we talked about in this loop and we reach
>>> agreement that the item 2# is not useful.
>>>
>>> I think the item 1# is still useful for some vendor if they defined the
>>> disabled/safe multi-function for a pin. They may expect the pin is
>>> changed to the disabled/safe state for saving power or some security
>>> reason.
>>
>> The only time item #1 above would happen is an error case. If there's an
>> error, there shouldn't be any expectation for the specific state of the
>> pinmux. If this intermediate state is illegal, then that's a problem in
>> an of itself; the HW is going to be in that state for some (admittedly
>> small) amount of time while the pinmux is being programmed, error or no
>> error, hence all the intermediate states had better be legal. I think
>> it's fine if the HW programming is simply left in whatever partial state
>> the code managed to get to. It's quite unlikely there's any "safe" state
>> that's /meaningfully/ better for a pin to be in once an error is
>> detected.
>>
>>> Thus, I think we should keep the disable_pinmux_setting in pinmux code.
>>>
>>> Do you think what I mentioned is an acceptable and not that aggressive
>>> solution?
>>
>> Not really.
>>
>>> Please correct me if anything wrong.
>>>
>>>
>>>
>>> For another topics:
>>> 1. There is no disable_pinconf: I think this is a issue. When the pin's
>>> mux setting is changed, the pinconf setting should also be changed, at
>>> least, the pinmux code here should offer the user a chance(interface) to
>>> decide whether to change the pinconf setting. Thus, we may need to add
>>> pinconf disable function.
>>
>> pinctrl already allows any config options to be changed along with the
>> mux option.
>>
>> The only reason any mux or config option is ever changed is in response
>> to selecting a new pinctrl state. Hence, I don't think you ever want to
>> "disable" either a mux or config option. Rather, you simply want to
>> "enable" or "select" or "program" the mux/config options in the new
>> state. Any mux/config option that needs to differ between states should
>> simply be listed in all the states, so that when the state is entered,
>> the appropriate HW programming is performed.
>>
>>> 2. If the vendor use pinctrl-single driver, the
>>> "pinctrl-single,function-off" implementation is not useful in practical
>>> usage. The "pinctrl-single,function-off" is parsed when pinctrl-single
>>> driver probe phase and the instance setting of
>>> "pinctrl-single,function-off" will be used for all pins setting.
>>> Practically, I think different pins may have different disabled/safe.
>>
>> I'm not sure what you're asking here.
>>
> 
> Dear Stephen,
> 
> I think we have reached the agreement that the HW operation should be
> avoided in disable_pinmux_setting. Just a little difference, I insist
> that the HW operation should only should be removed sometimes not always.
> 
> I think the disable_pinmux_setting is not only called by the error
> handler but also the
> "pinctrl_put=>pinctrl_release=>...=>"pinctrl_free_setting" call stack
> when there is no any alive user to use this pin.
> In this case,
> the pinmux_disable_setting will try to put the pin to a fixed and final
> state, not intermediate state, and should offer the vendor's driver an
> interface to place the pin to the unused(disabled/safe) state(HW aspect).
> 
> Thus, I think we should remove HW operation in pinmux_disabl_setting
> only for some cases, just same as what I mentioned in my patch.
> 
> Did I got anything wrong ?

Like I said, I think there's really not much point in doing that, and
it'll just make the code more complicated than it needs to be.

However, if LinusW is OK taking that patch, I don't have any problem
with it; that change won't cause any problems on any HW platform I have.

Re: [PATCH] pinctrl: add params in disable_setting for different usage

[FanWu]

On 05/21/2014 02:42 AM, Stephen Warren wrote:
> On 05/19/2014 09:05 PM, FanWu wrote:
>> On 05/20/2014 04:55 AM, Stephen Warren wrote:
>>> On 05/18/2014 08:54 PM, FanWu wrote:
>>>> On 05/17/2014 03:53 AM, Stephen Warren wrote:
>>>>> On 05/16/2014 10:21 AM, Linus Walleij wrote:
>>>>>> On Wed, May 14, 2014 at 4:01 AM,  <fwu@marvell.com> wrote:
>>>>>>
>>>>>>> From: Fan Wu <fwu@marvell.com>
>>>>>>>
>>>>>>> The patch added params in disable_setting to differ the two possible
>>>>>>> usage,
>>>>>>> 1.Only want to disable the pin setting in SW aspect, param can be
>>>>>>> set to "0"
>>>>>>> 2.Want to disable the pin setting in both HW and SW aspect, param
>>>>>>> can be set to "1";
>>>>>>>
>>>>>>> The reason why to do this is that:
>>>>>>> To avoid duplicated enable_setting operation without disabling
>>>>>>> operation which will
>>>>>>> let Pin's desc->mux_usecount keep being added.
>>>>>>>
>>>>>>> In the following case, the issue can be reproduced:
>>>>>>> 1)There is a driver need to switch Pin state dynamicly, E.g. b/t
>>>>>>> "sleep" and
>>>>>>> "default" state
>>>>>>> 2)The Pin setting configuration in the two state is same, like the
>>>>>>> following one:
>>>>>>> component a {
>>>>>>>            pinctrl-names = "default", "sleep";
>>>>>>>            pinctrl-0 = <&a_grp_setting &c_grp_setting>;
>>>>>>>            pinctrl-1 = <&b_grp_setting &c_grp_setting>;
>>>>>>> }
>>>>>>> The "c_grp_setting" config node is totaly same, maybe like following
>>>>>>> one:
>>>>>>
>>>>>> Hm this is a quite interesting thing if we can get it in place, but
>>>>>> I need Stephen's consent, also Tony should have a look at this as
>>>>>> I know he's had the same problem as you in pinctrl-single.
>>>>>
>>>>> I only briefly looked at the patch, but it probably solves/hides the
>>>>> immediate problem.
>>>>>
>>>>> However, rather than doing this, why not just remove
>>>>> pinmux_disable_setting() completely. It doesn't make sense to
>>>>> "disable a
>>>>> mux selection" (some value is always selected in the mux register
>>>>> field)
>>>>> any more than it does to "disable a drive strength selection". We don't
>>>>> have a pinconf_disable_setting(), and couldn't really add one if we
>>>>> wanted. For consistency, let's just remove pinmux_disable_setting(). Do
>>>>> you agree?
>>>>>
>>>>
>>>> Dear, Stephen and Guys,
>>>>
>>>> Sorry for late due to some personal affairs in Weekend time.
>>>>
>>>> I don't think it is a proper way to remove pinmux_disable_setting
>>>> directly without changing any other code, like no change on the code in
>>>> pinmux_enable_setting.
>>>>
>>>> Talking about the pinmux_disable_operation, in SW aspect, we also need
>>>> to consider the "pinmux_enable_setting" operation.
>>>> For the "pinmux_enable_setting" operation, there is some SW level code
>>>> logic, like pin_request.
>>>> Do you think it is a acceptable way to remove the SW level code logic
>>>> from the "pinmux_enable_setting" path, because there will be no
>>>> corresponding operation in reverse order in pinmux_disable_setting after
>>>> applying our possible change?
>>>
>>> No, I don't think we should remove the SW aspects of
>>> pinmux_enable_setting(). The pinctrl core currently tracks which pinctrl
>>> state "owns" each pingroup's mux, so that different pinctrl states can't
>>> both attempt to configure a pingroup's mux setting. We need to keep all
>>> the SW aspects of mux enable/disable. All I'm proposing removing is the
>>> HW-programming parts of pinmux_disable_setting().
>>>
>>>> At least, I think this way may be a considerable change in Pinmux
>>>> framework, right?
>>>
>>> Yes, removing all of pinmux_en/disable_setting would be a considerable
>>> and likely inappropriate change.
>>>
>>>> Talking about HW aspect,
>>>> I think the solution you mentioned is indeed a good way to solve the
>>>> problem for some HW vendor's SoC chip, but may be not that intact
>>>> solution.
>>>>
>>>> In my understanding, the pinmux operation, like enabling and disabling,
>>>> is to change pin's(pins') multi-function from funcion_M to function_N.
>>>> And, the "pinconf" enabling function is used to change the attributes of
>>>> the pin, like Pull_Up/Down, DriveStrength(Low/Medium/Fast) and etc.
>>>>
>>>> The pinmux disabling operation will be called in the following case in
>>>> current pinmux framework:
>>>> 1. when pin(s) is/are freed or error handler when configure it(them) and
>>>> finally the pin will be changed to a disabled/safe state if defined by
>>>> vendor.
>>>> 2. in the pinctrl_select_state function
>>>>
>>>> The item 2# is just the thing we talked about in this loop and we reach
>>>> agreement that the item 2# is not useful.
>>>>
>>>> I think the item 1# is still useful for some vendor if they defined the
>>>> disabled/safe multi-function for a pin. They may expect the pin is
>>>> changed to the disabled/safe state for saving power or some security
>>>> reason.
>>>
>>> The only time item #1 above would happen is an error case. If there's an
>>> error, there shouldn't be any expectation for the specific state of the
>>> pinmux. If this intermediate state is illegal, then that's a problem in
>>> an of itself; the HW is going to be in that state for some (admittedly
>>> small) amount of time while the pinmux is being programmed, error or no
>>> error, hence all the intermediate states had better be legal. I think
>>> it's fine if the HW programming is simply left in whatever partial state
>>> the code managed to get to. It's quite unlikely there's any "safe" state
>>> that's /meaningfully/ better for a pin to be in once an error is
>>> detected.
>>>
>>>> Thus, I think we should keep the disable_pinmux_setting in pinmux code.
>>>>
>>>> Do you think what I mentioned is an acceptable and not that aggressive
>>>> solution?
>>>
>>> Not really.
>>>
>>>> Please correct me if anything wrong.
>>>>
>>>>
>>>>
>>>> For another topics:
>>>> 1. There is no disable_pinconf: I think this is a issue. When the pin's
>>>> mux setting is changed, the pinconf setting should also be changed, at
>>>> least, the pinmux code here should offer the user a chance(interface) to
>>>> decide whether to change the pinconf setting. Thus, we may need to add
>>>> pinconf disable function.
>>>
>>> pinctrl already allows any config options to be changed along with the
>>> mux option.
>>>
>>> The only reason any mux or config option is ever changed is in response
>>> to selecting a new pinctrl state. Hence, I don't think you ever want to
>>> "disable" either a mux or config option. Rather, you simply want to
>>> "enable" or "select" or "program" the mux/config options in the new
>>> state. Any mux/config option that needs to differ between states should
>>> simply be listed in all the states, so that when the state is entered,
>>> the appropriate HW programming is performed.
>>>
>>>> 2. If the vendor use pinctrl-single driver, the
>>>> "pinctrl-single,function-off" implementation is not useful in practical
>>>> usage. The "pinctrl-single,function-off" is parsed when pinctrl-single
>>>> driver probe phase and the instance setting of
>>>> "pinctrl-single,function-off" will be used for all pins setting.
>>>> Practically, I think different pins may have different disabled/safe.
>>>
>>> I'm not sure what you're asking here.
>>>
>>
>> Dear Stephen,
>>
>> I think we have reached the agreement that the HW operation should be
>> avoided in disable_pinmux_setting. Just a little difference, I insist
>> that the HW operation should only should be removed sometimes not always.
>>
>> I think the disable_pinmux_setting is not only called by the error
>> handler but also the
>> "pinctrl_put=>pinctrl_release=>...=>"pinctrl_free_setting" call stack
>> when there is no any alive user to use this pin.
>> In this case,
>> the pinmux_disable_setting will try to put the pin to a fixed and final
>> state, not intermediate state, and should offer the vendor's driver an
>> interface to place the pin to the unused(disabled/safe) state(HW aspect).
>>
>> Thus, I think we should remove HW operation in pinmux_disabl_setting
>> only for some cases, just same as what I mentioned in my patch.
>>
>> Did I got anything wrong ?
>
> Like I said, I think there's really not much point in doing that, and
> it'll just make the code more complicated than it needs to be.
>
> However, if LinusW is OK taking that patch, I don't have any problem
> with it; that change won't cause any problems on any HW platform I have.
>


Dear Stephen, Linus and Guys,

To remove the HW disable function in pinmux_disable_setting is no effect 
for our SoC platform. I am just not sure whether it has effect for other 
platform just as I described before.
If there is no vendor using the HW disabling operation, I also agree to 
remove this. :)

Could you please give your suggestion about this topic ?

Re: [PATCH] pinctrl: add params in disable_setting for different usage

[Tony Lindgren]

* FanWu <fwu@marvell.com> [140520 23:23]:
> To remove the HW disable function in pinmux_disable_setting is no effect for
> our SoC platform. I am just not sure whether it has effect for other
> platform just as I described before.
> If there is no vendor using the HW disabling operation, I also agree to
> remove this. :)
> 
> Could you please give your suggestion about this topic ?

I agree with Stephen that we should remove the disable as at least
for the SoCs that I've dealt with there is no disable setting. The
closest I can think of is the safe mode that some omaps have, but
that too is just one mode. And disabling input logic can be done
separately from the mux mode typically.

Regards,

Tony

Re: [PATCH] pinctrl: add params in disable_setting for different usage

[Linus Walleij]

On Thu, May 22, 2014 at 12:52 AM, Tony Lindgren <tony@atomide.com> wrote:
> * FanWu <fwu@marvell.com> [140520 23:23]:
>> To remove the HW disable function in pinmux_disable_setting is no effect for
>> our SoC platform. I am just not sure whether it has effect for other
>> platform just as I described before.
>> If there is no vendor using the HW disabling operation, I also agree to
>> remove this. :)
>>
>> Could you please give your suggestion about this topic ?
>
> I agree with Stephen that we should remove the disable as at least
> for the SoCs that I've dealt with there is no disable setting. The
> closest I can think of is the safe mode that some omaps have, but
> that too is just one mode. And disabling input logic can be done
> separately from the mux mode typically.

It seems you are right: we should move the pins between different
states and there is really no "disabled" state.

I mean the pin can be tri-stated with pin config but that is not
strictly a mux option, the pin does not fall off the chip package :-P

So we should:
(A) make the disable hook optional
(B) remove the callback from all drivers not really using it
(C) investigate any remaining cases as they are probably
  doing something wrong

Yours,
Linus Walleij