Message-ID: <537C4605.2090203@marvell.com>
Date:	Wed, 21 May 2014 14:21:57 +0800
From:	FanWu <fwu@...vell.com>
To:	Stephen Warren <swarren@...dotorg.org>
CC:	Linus Walleij <linus.walleij@...aro.org>,
	ext Tony Lindgren <tony@...mide.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Stephen Warren <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 for different
 usage

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@...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:
>>>>>>
>>>>>> 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 ?

--
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