lkml.org 
[lkml]   [2014]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] pinctrl: add params in disable_setting for different usage
    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.


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