Messages in this thread | | | Date | Thu, 22 May 2014 11:46:20 +0800 | From | FanWu <> | Subject | Re: [PATCH v2] pinctrl: add params in disable_setting for different usage |
| |
On 05/22/2014 11:10 AM, fwu@marvell.com wrote: > From: Fan Wu <fwu@marvell.com> > > What the patch did: > 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time of > calling pinctrl_select_state > 2.Remove the HW disable operation in in pinmux_disable_setting function. > > The reason why to do this is that: > 1.To avoid duplicated enable_setting operation without disabling operation > which will let Pin's desc->mux_usecount keep being added. > 2.The HW pin disable operation is not useful for most of the vendors' platform. > And this avoid the HW glitch after using the item 1# modification. > > 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 DTS node may be 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 switching 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 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, because the "desc" is for each physical pin while the "setting" is > for each setting node in the DTS. > Thus, if the "c_grp_setting" in pinctrl-0 is not disabled ahead of enabling > "c_grp_setting" in pinctrl-1, the desc->mux_usecount will be kept added without > any chance to be decreased. > > 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", actually > two setting instance, ahead of enabling 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". > > Conclusion: > 1.To Remove the HW disabling operation to above the glitch mentioned above. > 2.Handle the issue mentioned above by disabling all of the setting in old > state and then enable the new setting in new state. > > Signed-off-by: Fan Wu <fwu@marvell.com> > --- > drivers/pinctrl/core.c | 18 +++--------------- > drivers/pinctrl/pinmux.c | 4 ---- > 2 files changed, 3 insertions(+), 19 deletions(-) > > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index c0fe609..c97491a 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -993,25 +993,13 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) > * may not be identical to the set of groups with a mux setting > * in the new state. While this might be unusual, it's entirely > * possible for the "user"-supplied mapping table to be written > - * that way. For each group that was configured in the old state > - * but not in the new state, this code puts that group into a > - * safe/disabled state. > + * that way. This code is used for each group that was > + * configured in the old state but not in the new 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); > } > } > > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c > index 9248ce4..c2c4aff 100644 > --- a/drivers/pinctrl/pinmux.c > +++ b/drivers/pinctrl/pinmux.c > @@ -469,7 +469,6 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) > { > struct pinctrl_dev *pctldev = setting->pctldev; > const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; > - const struct pinmux_ops *ops = pctldev->desc->pmxops; > int ret; > const unsigned *pins; > unsigned num_pins; > @@ -515,9 +514,6 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) > pins[i], desc->name, gname); > } > } > - > - if (ops->disable) > - ops->disable(pctldev, setting->data.mux.func, setting->data.mux.group); > } > > #ifdef CONFIG_DEBUG_FS >
Dear Guys,
Just as we talked, for my platform, I have no concern about removing the HW disabling operation in the pinmux_disable_setting.
Thus, I filed a new version of the patch we discussed before. Just as the patch comments said, the patch did: What the patch did: 1.To call pinmux_disable_setting ahead of pinmux_enable_setting in each time of calling pinctrl_select_state 2.Remove the HW disable operation in in pinmux_disable_setting function.
Could you please help to review the above patch?
If there is no concern about the above solution from you Guys, I think I will try to do the other two patches:
1. To remove the disable ops registration when defining the "include/linux/pinctrl/pinmux.h" in inctrl-single driver. Meanwhile, the related things, like "pinctrl-single,function-off" property and corresponding flag in "pcs_device", will be also removed.
2. To remove the disable ops in "pinmux_ops"(include/linux/pinctrl/pinmux.h)
Is that OK for you about my plan ?
| |