| Message ID | 1350651909-5337-1-git-send-email-linus.walleij@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-by: Jean Nicolas Graux <jean-nicolas.graux@stericsson.com> Le 10/19/2012 03:05 PM, Linus WALLEIJ a écrit : > From: Linus Walleij<linus.walleij@linaro.org> > > This switches the way that pins are reserved for multiplexing: > > We used to do this when the map was parsed, at the creation of > the settings inside the pinctrl handle, in pinmux_map_to_setting(). > > However this does not work for us, because we want to use the > same set of pins with different devices at different times: the > current code assumes that the pin groups in a pinmux state will > only be used with one single device, albeit different groups can > be active at different times. For example if a single I2C driver > block is used to drive two different busses located on two > pin groups A and B, then the pins for all possible states of a > function are reserved when fetching the pinctrl handle: the > I2C bus can choose either set A or set B by a mux state at > runtime, but all pins in both group A and B (the superset) are > effectively reserved for that I2C function and mapped to the > device. Another device can never get in and use the pins in > group A, even if the device/function is using group B at the > moment. > > Instead: let use reserve the pins when the state is activated > and drop them when the state is disabled, i.e. when we move to > another state. This way different devices/functions can use the > same pins at different times. > > We know that this is an odd way of doing things, but we really > need to switch e.g. an SD-card slot to become a tracing output > sink at runtime: we plug in a special "tracing card" then mux > the pins that used to be an SD slot around to the tracing > unit and push out tracing data there instead of SD-card > traffic. > > As a side effect pinmux_free_setting() is unused and gets > deleted. > > Cc: Patrice Chotard<patrice.chotard@st.com> > Cc: Jean Nicolas Graux<jean-nicolas.graux@stericsson.com> > Cc: Loic Pallardy<loic.pallardy@st.com> > Signed-off-by: Linus Walleij<linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - The code was already accounting for the case where the setting > was not active and called pinmux_disable_setting() > from the core, so skip this and delete the now empty > pinmux_free_setting() altogether. > --- > Documentation/pinctrl.txt | 4 ++- > drivers/pinctrl/core.c | 3 +- > drivers/pinctrl/core.h | 2 ++ > drivers/pinctrl/pinmux.c | 70 ++++++++++++++--------------------------------- > drivers/pinctrl/pinmux.h | 5 ---- > 5 files changed, 28 insertions(+), 56 deletions(-) > > diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt > index 3b4ee53..a1cd2f9 100644 > --- a/Documentation/pinctrl.txt > +++ b/Documentation/pinctrl.txt > @@ -1193,4 +1193,6 @@ foo_switch() > ... > } > > -The above has to be done from process context. > +The above has to be done from process context. The reservation of the pins > +will be done when the state is activated, so in effect one specific pin > +can be used by different functions at different times on a running system. > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index 0f1ec9e..bbd930e 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -563,6 +563,8 @@ static int add_setting(struct pinctrl *p, struct pinctrl_map const *map) > return -EPROBE_DEFER; > } > > + setting->dev_name = map->dev_name; > + > switch (map->type) { > case PIN_MAP_TYPE_MUX_GROUP: > ret = pinmux_map_to_setting(map, setting); > @@ -689,7 +691,6 @@ static void pinctrl_put_locked(struct pinctrl *p, bool inlist) > case PIN_MAP_TYPE_MUX_GROUP: > if (state == p->state) > pinmux_disable_setting(setting); > - pinmux_free_setting(setting); > break; > case PIN_MAP_TYPE_CONFIGS_PIN: > case PIN_MAP_TYPE_CONFIGS_GROUP: > diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h > index 1f40ff6..12f5694 100644 > --- a/drivers/pinctrl/core.h > +++ b/drivers/pinctrl/core.h > @@ -105,12 +105,14 @@ struct pinctrl_setting_configs { > * @type: the type of setting > * @pctldev: pin control device handling to be programmed. Not used for > * PIN_MAP_TYPE_DUMMY_STATE. > + * @dev_name: the name of the device using this state > * @data: Data specific to the setting type > */ > struct pinctrl_setting { > struct list_head node; > enum pinctrl_map_type type; > struct pinctrl_dev *pctldev; > + const char *dev_name; > union { > struct pinctrl_setting_mux mux; > struct pinctrl_setting_configs configs; > diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c > index 9301a7a..0ecdf54 100644 > --- a/drivers/pinctrl/pinmux.c > +++ b/drivers/pinctrl/pinmux.c > @@ -314,14 +314,11 @@ int pinmux_map_to_setting(struct pinctrl_map const *map, > { > struct pinctrl_dev *pctldev = setting->pctldev; > const struct pinmux_ops *pmxops = pctldev->desc->pmxops; > - const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; > char const * const *groups; > unsigned num_groups; > int ret; > const char *group; > int i; > - const unsigned *pins; > - unsigned num_pins; > > if (!pmxops) { > dev_err(pctldev->dev, "does not support mux function\n"); > @@ -376,55 +373,9 @@ int pinmux_map_to_setting(struct pinctrl_map const *map, > } > setting->data.mux.group = ret; > > - ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, &pins, > - &num_pins); > - if (ret) { > - dev_err(pctldev->dev, > - "could not get pins for device %s group selector %d\n", > - pinctrl_dev_get_name(pctldev), setting->data.mux.group); > - return -ENODEV; > - } > - > - /* Try to allocate all pins in this group, one by one */ > - for (i = 0; i < num_pins; i++) { > - ret = pin_request(pctldev, pins[i], map->dev_name, NULL); > - if (ret) { > - dev_err(pctldev->dev, > - "could not request pin %d on device %s\n", > - pins[i], pinctrl_dev_get_name(pctldev)); > - /* On error release all taken pins */ > - i--; /* this pin just failed */ > - for (; i >= 0; i--) > - pin_free(pctldev, pins[i], NULL); > - return -ENODEV; > - } > - } > - > return 0; > } > > -void pinmux_free_setting(struct pinctrl_setting const *setting) > -{ > - struct pinctrl_dev *pctldev = setting->pctldev; > - const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; > - const unsigned *pins; > - unsigned num_pins; > - int ret; > - int i; > - > - ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, > - &pins, &num_pins); > - if (ret) { > - dev_err(pctldev->dev, > - "could not get pins for device %s group selector %d\n", > - pinctrl_dev_get_name(pctldev), setting->data.mux.group); > - return; > - } > - > - for (i = 0; i < num_pins; i++) > - pin_free(pctldev, pins[i], NULL); > -} > - > int pinmux_enable_setting(struct pinctrl_setting const *setting) > { > struct pinctrl_dev *pctldev = setting->pctldev; > @@ -446,6 +397,22 @@ int pinmux_enable_setting(struct pinctrl_setting const *setting) > num_pins = 0; > } > > + /* Try to allocate all pins in this group, one by one */ > + for (i = 0; i < num_pins; i++) { > + ret = pin_request(pctldev, pins[i], setting->dev_name, NULL); > + if (ret) { > + dev_err(pctldev->dev, > + "could not request pin %d on device %s\n", > + pins[i], pinctrl_dev_get_name(pctldev)); > + /* On error release all taken pins */ > + i--; /* this pin just failed */ > + for (; i >= 0; i--) > + pin_free(pctldev, pins[i], NULL); > + return -ENODEV; > + } > + } > + > + /* Now that we have acquired the pins, encode the mux setting */ > for (i = 0; i < num_pins; i++) { > desc = pin_desc_get(pctldev, pins[i]); > if (desc == NULL) { > @@ -482,6 +449,7 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) > num_pins = 0; > } > > + /* Flag the descs that no setting is active */ > for (i = 0; i < num_pins; i++) { > desc = pin_desc_get(pctldev, pins[i]); > if (desc == NULL) { > @@ -493,6 +461,10 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) > desc->mux_setting = NULL; > } > > + /* And release the pins */ > + for (i = 0; i < num_pins; i++) > + pin_free(pctldev, pins[i], NULL); > + > if (ops->disable) > 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..3c2aafa 100644 > --- a/drivers/pinctrl/pinmux.h > +++ b/drivers/pinctrl/pinmux.h > @@ -27,7 +27,6 @@ int pinmux_gpio_direction(struct pinctrl_dev *pctldev, > > 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); > > @@ -69,10 +68,6 @@ static inline int pinmux_map_to_setting(struct pinctrl_map const *map, > return 0; > } > > -static inline void pinmux_free_setting(struct pinctrl_setting const *setting) > -{ > -} > - > static inline int pinmux_enable_setting(struct pinctrl_setting const *setting) > { > return 0;
On 10/19/2012 07:05 AM, Linus Walleij wrote: > From: Linus Walleij <linus.walleij@linaro.org> > > This switches the way that pins are reserved for multiplexing: > > We used to do this when the map was parsed, at the creation of > the settings inside the pinctrl handle, in pinmux_map_to_setting(). > > However this does not work for us, because we want to use the > same set of pins with different devices at different times: the > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > @@ -689,7 +691,6 @@ static void pinctrl_put_locked(struct pinctrl *p, bool inlist) > case PIN_MAP_TYPE_MUX_GROUP: > if (state == p->state) > pinmux_disable_setting(setting); > - pinmux_free_setting(setting); Personally, I would keep that function, and just remove the body. I believe that even though it currently doesn't have to do anything, it provides documentation for where any required cleanup would be placed if needed in the future. If you still want to remove the function, pinconf_free_setting() should also be removed, since that's empty right now. Irrespective, Acked-by: Stephen Warren <swarren@nvidia.com>
* Linus Walleij <linus.walleij@stericsson.com> [121019 06:09]: > > Instead: let use reserve the pins when the state is activated > and drop them when the state is disabled, i.e. when we move to > another state. This way different devices/functions can use the > same pins at different times. Hmm doesn't this mean that we are now doing lots of extra reserving and dropping of pins? Performance is important from latency point of view for cases where we need to remux pins constantly runtime PM. Regards, Tony
On Fri, Oct 19, 2012 at 8:10 PM, Tony Lindgren <tony@atomide.com> wrote: > [Me] >> Instead: let use reserve the pins when the state is activated >> and drop them when the state is disabled, i.e. when we move to >> another state. This way different devices/functions can use the >> same pins at different times. > > Hmm doesn't this mean that we are now doing lots of extra > reserving and dropping of pins? Performance is important from > latency point of view for cases where we need to remux pins > constantly runtime PM. It is only done in case the pinmux state is switched in runtime suspend/resume, so it's e.g. possible to just alter the pin config. But in general what you say is true. We used to to the same thing by having drivers call pinctrl_get()/pinctrl_put() in this case instead, but that went away with the introduction of states, so we cannot encode different pin sets with say pinctrl_get(dev, "foo")/pinctrl_get(dev, "bar") anymore since there is only one pinctrl handle per device, but multiple states. If this turns out to be a severe performance bottleneck, I suggest to add some additional constraint API, like pinctrl_set_pinmux_homegeneous_pinsets(true) that will at runtime select whether the pin allocation is done when getting the pinctrl handle instead. Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [121022 01:22]: > On Fri, Oct 19, 2012 at 8:10 PM, Tony Lindgren <tony@atomide.com> wrote: > > [Me] > >> Instead: let use reserve the pins when the state is activated > >> and drop them when the state is disabled, i.e. when we move to > >> another state. This way different devices/functions can use the > >> same pins at different times. > > > > Hmm doesn't this mean that we are now doing lots of extra > > reserving and dropping of pins? Performance is important from > > latency point of view for cases where we need to remux pins > > constantly runtime PM. > > It is only done in case the pinmux state is switched in runtime > suspend/resume, so it's e.g. possible to just alter the pin config. > > But in general what you say is true. > > We used to to the same thing by having drivers call > pinctrl_get()/pinctrl_put() in this case instead, but that went > away with the introduction of states, so we cannot encode > different pin sets with say > pinctrl_get(dev, "foo")/pinctrl_get(dev, "bar") > anymore since there is only one pinctrl handle per device, > but multiple states. OK > If this turns out to be a severe performance bottleneck, I > suggest to add some additional constraint API, like > pinctrl_set_pinmux_homegeneous_pinsets(true) that will > at runtime select whether the pin allocation is done when > getting the pinctrl handle instead. Or maybe you could release + reserve the pins only if the pins change? Regards, Tony
On 10/22/2012 02:21 AM, Linus Walleij wrote: > On Fri, Oct 19, 2012 at 8:10 PM, Tony Lindgren <tony@atomide.com> wrote: >> [Me] >>> Instead: let use reserve the pins when the state is activated >>> and drop them when the state is disabled, i.e. when we move to >>> another state. This way different devices/functions can use the >>> same pins at different times. >> >> Hmm doesn't this mean that we are now doing lots of extra >> reserving and dropping of pins? Performance is important from >> latency point of view for cases where we need to remux pins >> constantly runtime PM. > > It is only done in case the pinmux state is switched in runtime > suspend/resume, so it's e.g. possible to just alter the pin config. > > But in general what you say is true. > > We used to to the same thing by having drivers call > pinctrl_get()/pinctrl_put() in this case instead, but that went > away with the introduction of states, so we cannot encode > different pin sets with say > pinctrl_get(dev, "foo")/pinctrl_get(dev, "bar") > anymore since there is only one pinctrl handle per device, > but multiple states. > > If this turns out to be a severe performance bottleneck, I > suggest to add some additional constraint API, like > pinctrl_set_pinmux_homegeneous_pinsets(true) that will > at runtime select whether the pin allocation is done when > getting the pinctrl handle instead. That API sounds like something system-wide, which seems like it would be rather presumptuous (CPU/SoC support code couldn't execute it, since that would presume a facet of all board designs that could change in the future). Even a driver shouldn't be assuming this; it can't know what boards it gets used in ahead of time. Instead, it seems like the map registration code could easily look at all states defined for a device, and determine if the set of pins/groups used by those states was identical, and switch between up-front or dynamic registration as needed by the specific map entries.
On Mon, Oct 22, 2012 at 9:07 PM, Tony Lindgren <tony@atomide.com> wrote: > * Linus Walleij <linus.walleij@linaro.org> [121022 01:22]: >> If this turns out to be a severe performance bottleneck, I >> suggest to add some additional constraint API, like >> pinctrl_set_pinmux_homegeneous_pinsets(true) that will >> at runtime select whether the pin allocation is done when >> getting the pinctrl handle instead. > > Or maybe you could release + reserve the pins only if the > pins change? Umm... not quite following but show me the code :-D In the code as it stands currently we only detect if a state changes and then it assumes the old state need to be deactivated before activating a new one. I think that in order to do that for a state A->B transition we need to: 1. Get the pins for mux setting A 2. Get the pins for mux setting B 3. Loop over the pin array and compare 4. If all are the same, goto 7 5. Release pins for setting A 6. Acquire pins for setting B 7. Update all pin descs to point to setting B 8. Done Hm, I can see a patch like that... Yours, Linus Walleij
On Mon, Oct 22, 2012 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/22/2012 02:21 AM, Linus Walleij wrote: >> If this turns out to be a severe performance bottleneck, I >> suggest to add some additional constraint API, like >> pinctrl_set_pinmux_homegeneous_pinsets(true) that will >> at runtime select whether the pin allocation is done when >> getting the pinctrl handle instead. > > That API sounds like something system-wide, which seems like it would be > rather presumptuous (CPU/SoC support code couldn't execute it, since > that would presume a facet of all board designs that could change in the > future). Even a driver shouldn't be assuming this; it can't know what > boards it gets used in ahead of time. Well, yeah. It should rather be part of the pinctrl descriptor then, so it becomes a per-controller runpath simplification. > Instead, it seems like the map registration code could easily look at > all states defined for a device, and determine if the set of pins/groups > used by those states was identical, and switch between up-front or > dynamic registration as needed by the specific map entries. That kind of constraint-resolution in the kernel scares me, soon we will have a prolog runtime ... (but hm maybe that is not such a bad idea considering some other constraint things I've seen around) Yours, Linus Walleij
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt index 3b4ee53..a1cd2f9 100644 --- a/Documentation/pinctrl.txt +++ b/Documentation/pinctrl.txt @@ -1193,4 +1193,6 @@ foo_switch() ... } -The above has to be done from process context. +The above has to be done from process context. The reservation of the pins +will be done when the state is activated, so in effect one specific pin +can be used by different functions at different times on a running system. diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index 0f1ec9e..bbd930e 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -563,6 +563,8 @@ static int add_setting(struct pinctrl *p, struct pinctrl_map const *map) return -EPROBE_DEFER; } + setting->dev_name = map->dev_name; + switch (map->type) { case PIN_MAP_TYPE_MUX_GROUP: ret = pinmux_map_to_setting(map, setting); @@ -689,7 +691,6 @@ static void pinctrl_put_locked(struct pinctrl *p, bool inlist) case PIN_MAP_TYPE_MUX_GROUP: if (state == p->state) pinmux_disable_setting(setting); - pinmux_free_setting(setting); break; case PIN_MAP_TYPE_CONFIGS_PIN: case PIN_MAP_TYPE_CONFIGS_GROUP: diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h index 1f40ff6..12f5694 100644 --- a/drivers/pinctrl/core.h +++ b/drivers/pinctrl/core.h @@ -105,12 +105,14 @@ struct pinctrl_setting_configs { * @type: the type of setting * @pctldev: pin control device handling to be programmed. Not used for * PIN_MAP_TYPE_DUMMY_STATE. + * @dev_name: the name of the device using this state * @data: Data specific to the setting type */ struct pinctrl_setting { struct list_head node; enum pinctrl_map_type type; struct pinctrl_dev *pctldev; + const char *dev_name; union { struct pinctrl_setting_mux mux; struct pinctrl_setting_configs configs; diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 9301a7a..0ecdf54 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -314,14 +314,11 @@ int pinmux_map_to_setting(struct pinctrl_map const *map, { struct pinctrl_dev *pctldev = setting->pctldev; const struct pinmux_ops *pmxops = pctldev->desc->pmxops; - const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; char const * const *groups; unsigned num_groups; int ret; const char *group; int i; - const unsigned *pins; - unsigned num_pins; if (!pmxops) { dev_err(pctldev->dev, "does not support mux function\n"); @@ -376,55 +373,9 @@ int pinmux_map_to_setting(struct pinctrl_map const *map, } setting->data.mux.group = ret; - ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, &pins, - &num_pins); - if (ret) { - dev_err(pctldev->dev, - "could not get pins for device %s group selector %d\n", - pinctrl_dev_get_name(pctldev), setting->data.mux.group); - return -ENODEV; - } - - /* Try to allocate all pins in this group, one by one */ - for (i = 0; i < num_pins; i++) { - ret = pin_request(pctldev, pins[i], map->dev_name, NULL); - if (ret) { - dev_err(pctldev->dev, - "could not request pin %d on device %s\n", - pins[i], pinctrl_dev_get_name(pctldev)); - /* On error release all taken pins */ - i--; /* this pin just failed */ - for (; i >= 0; i--) - pin_free(pctldev, pins[i], NULL); - return -ENODEV; - } - } - return 0; } -void pinmux_free_setting(struct pinctrl_setting const *setting) -{ - struct pinctrl_dev *pctldev = setting->pctldev; - const struct pinctrl_ops *pctlops = pctldev->desc->pctlops; - const unsigned *pins; - unsigned num_pins; - int ret; - int i; - - ret = pctlops->get_group_pins(pctldev, setting->data.mux.group, - &pins, &num_pins); - if (ret) { - dev_err(pctldev->dev, - "could not get pins for device %s group selector %d\n", - pinctrl_dev_get_name(pctldev), setting->data.mux.group); - return; - } - - for (i = 0; i < num_pins; i++) - pin_free(pctldev, pins[i], NULL); -} - int pinmux_enable_setting(struct pinctrl_setting const *setting) { struct pinctrl_dev *pctldev = setting->pctldev; @@ -446,6 +397,22 @@ int pinmux_enable_setting(struct pinctrl_setting const *setting) num_pins = 0; } + /* Try to allocate all pins in this group, one by one */ + for (i = 0; i < num_pins; i++) { + ret = pin_request(pctldev, pins[i], setting->dev_name, NULL); + if (ret) { + dev_err(pctldev->dev, + "could not request pin %d on device %s\n", + pins[i], pinctrl_dev_get_name(pctldev)); + /* On error release all taken pins */ + i--; /* this pin just failed */ + for (; i >= 0; i--) + pin_free(pctldev, pins[i], NULL); + return -ENODEV; + } + } + + /* Now that we have acquired the pins, encode the mux setting */ for (i = 0; i < num_pins; i++) { desc = pin_desc_get(pctldev, pins[i]); if (desc == NULL) { @@ -482,6 +449,7 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) num_pins = 0; } + /* Flag the descs that no setting is active */ for (i = 0; i < num_pins; i++) { desc = pin_desc_get(pctldev, pins[i]); if (desc == NULL) { @@ -493,6 +461,10 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting) desc->mux_setting = NULL; } + /* And release the pins */ + for (i = 0; i < num_pins; i++) + pin_free(pctldev, pins[i], NULL); + if (ops->disable) 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..3c2aafa 100644 --- a/drivers/pinctrl/pinmux.h +++ b/drivers/pinctrl/pinmux.h @@ -27,7 +27,6 @@ int pinmux_gpio_direction(struct pinctrl_dev *pctldev, 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); @@ -69,10 +68,6 @@ static inline int pinmux_map_to_setting(struct pinctrl_map const *map, return 0; } -static inline void pinmux_free_setting(struct pinctrl_setting const *setting) -{ -} - static inline int pinmux_enable_setting(struct pinctrl_setting const *setting) { return 0;