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

From: FanWu
Date: Mon May 19 2014 - 23:05:53 EST

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@xxxxxxxxxxx> wrote:

From: Fan Wu <fwu@xxxxxxxxxxx>

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 !

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