improved support for runtime muxing for pinctrl

There's no need to duplicate essentially the same functions. Let's introduce static int pinctrl_pm_select_state() and make the other related functions call that.

This allows us to add support later on for multiple active states, and more optimized dynamic remuxing.

Note that we still need to export the various pinctrl_pm_select functions as we want to keep struct pinctrl_state private to the pinctrl code, and cannot replace those with inline functions.

Cc Stephen Warren <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
  drivers/pinctrl/core.c |   47 +++++++++++++++++++----------------------------
  1 file changed, 19 insertions(+), 28 deletions(-)

drivers/pinctrl/core.c +7 -6

index 5b272bf..bda2c61 100644
@@ -1227,23 +1227,34 @@ EXPORT_SYMBOL_GPL(pinctrl_force_default);
  #ifdef CONFIG_PM
 
  /**
- * pinctrl_pm_select_default_state() - select default pinctrl state for PM
+ * pinctrl_pm_select_state() - select pinctrl state for PM
   * @dev: device to select default state for
+ * @state: state to set
   */
-int pinctrl_pm_select_default_state(struct device *dev)
+static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *state)
  {
  	struct dev_pin_info *pins = dev->pins;
  	int ret;
 
  	if (!pins)
  		return 0;
-	if (IS_ERR(pins->default_state))
-		return 0; /* No default state */
-	ret = pinctrl_select_state(pins->p, pins->default_state);
+	if (IS_ERR(state))
+		return 0; /* No such state */
+	ret = pinctrl_select_state(pins->p, state);
  	if (ret)
-		dev_err(dev, "failed to activate default pinctrl state\n");
+		dev_err(dev, "failed to activate pinctrl state %s\n",

drivers/pinctrl/core.c +2 -1

+	return pinctrl_pm_select_state(dev, dev->pins->default_state);
+}
  EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
 
  /**

@@ -1252,17 +1263,7 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_default_state);
   */
  int pinctrl_pm_select_sleep_state(struct device *dev)
  {
-	struct dev_pin_info *pins = dev->pins;

drivers/pinctrl/core.c +1 -1

+	return pinctrl_pm_select_state(dev, dev->pins->sleep_state);
  }
  EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
 

@@ -1272,17 +1273,7 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_sleep_state);
   */
  int pinctrl_pm_select_idle_state(struct device *dev)
  {
-	struct dev_pin_info *pins = dev->pins;

drivers/pinctrl/core.c +1 -1

+	return pinctrl_pm_select_state(dev, dev->pins->idle_state);
  }
  EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state);
  #endif
 
--

To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Tony,

This patch causes boot failure when I've applied my patch "[RFC] ARM: OMAP2+: omap_device: add pinctrl handling"
https://lkml.org/lkml/2013/6/21/309

on top of it:

[    0.610565] PC is at pinctrl_pm_select_active_state+0x4/0xc

Quoted 7 lines · Grygorii Strashko

Hi Tony,

This patch causes boot failure when I've applied my patch "[RFC] ARM: OMAP2+: omap_device: add pinctrl handling"
https://lkml.org/lkml/2013/6/21/309

on top of it:

Hmm this patch alone removes duplicate code and if it causes issues I must have made a typo somewhere.

I cannot see a typo though, but in your dmesg I see something..

Quoted 1 line

[    0.610565] PC is at pinctrl_pm_select_active_state+0x4/0xc

..looks like you have something applied for the active_state that only gets introduced later on in this series.

Maybe you have the earlier version of drivers/base/pinctrl.c active_state patch that was in linux next for a while but got reverted as we noticed we had to rework some things?

So maybe try with v3.11-rc1 + these four patches + your omap_device patch?

Regards,

Tony

Hi Tony,

Quoted 7 lines · Tony Lindgren

Quoted 6 lines · Grygorii Strashko

Hi Tony,

This patch causes boot failure when I've applied my patch "[RFC] ARM: OMAP2+: omap_device: add pinctrl handling"
https://lkml.org/lkml/2013/6/21/309

Quoted 4 lines

Quoted 1 line

on top of it:

Hmm this patch alone removes duplicate code and if it causes issues I must have made a typo somewhere.

No typo :) You've removed the check for !dev-pins.

And the failure place is:
int pinctrl_pm_select_active_state(struct device *dev)

{
   return pinctrl_pm_select_state(dev, dev->pins->active_state);
                                                ^^^^ here
}

If I understand everything right, the pinctrl support in Device core assumed to be optional - so, It's valid case, when there are no definition for device's pinctrl in DT at all.

And in this case dev->pins == NULL and pinctrl_pm_select_*() API should return 0 always.

drivers/pinctrl/core.c +50 -0

+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+			  struct pinctrl_state *st2)
+{
+	struct pinctrl_setting *s1, *s2;
+
+	list_for_each_entry(s1, &st1->settings, node) {
+		struct pinctrl_dev *pctldev1;
+		const struct pinctrl_ops *pctlops1;
+		const unsigned *pins1;
+		unsigned num_pins1;
+		int res;
+
+		if (s1->type != PIN_MAP_TYPE_MUX_GROUP)
+			continue;
+
+		pctldev1 = s1->pctldev;
+		pctlops1 = pctldev1->desc->pctlops;
+		res = pctlops1->get_group_pins(pctldev1, s1->data.mux.group,
+					       &pins1, &num_pins1);
+		if (res) {
+			dev_dbg(dev, "could not get state1 group pins\n");
+			return -EINVAL;
+		}
+
+		list_for_each_entry(s2, &st2->settings, node) {
+			struct pinctrl_dev *pctldev2;
+			const struct pinctrl_ops *pctlops2;
+			const unsigned *pins2;
+			unsigned num_pins2;
+			int i, j, found = 0;
+
+			if (s2->type != PIN_MAP_TYPE_MUX_GROUP)
+				continue;
+
+			pctldev2 = s2->pctldev;
+			if (pctldev1 != pctldev2) {
+				dev_dbg(dev, "pctldev must be the same for states\n");
+				return -EINVAL;
+			}
+			pctlops2 = pctldev2->desc->pctlops;
+			res = pctlops2->get_group_pins(pctldev2,
+						       s2->data.mux.group,
+						       &pins2, &num_pins2);
+			if (res) {
+				dev_dbg(dev, "could not get state2 group pins\n");
+				return -EINVAL;
+			}
+
+			for (i = 0; i < num_pins1; i++) {
+				int pin1 = pins1[i];

Hi,

Quoted 56 lines · Tony Lindgren

drivers/pinctrl/core.c +56 -0

+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+			  struct pinctrl_state *st2)
+{
+	struct pinctrl_setting *s1, *s2;
+
+	list_for_each_entry(s1, &st1->settings, node) {
+		struct pinctrl_dev *pctldev1;
+		const struct pinctrl_ops *pctlops1;
+		const unsigned *pins1;
+		unsigned num_pins1;
+		int res;
+
+		if (s1->type != PIN_MAP_TYPE_MUX_GROUP)
+			continue;
+
+		pctldev1 = s1->pctldev;
+		pctlops1 = pctldev1->desc->pctlops;
+		res = pctlops1->get_group_pins(pctldev1, s1->data.mux.group,
+					       &pins1, &num_pins1);
+		if (res) {
+			dev_dbg(dev, "could not get state1 group pins\n");
+			return -EINVAL;
+		}
+
+		list_for_each_entry(s2, &st2->settings, node) {
+			struct pinctrl_dev *pctldev2;
+			const struct pinctrl_ops *pctlops2;
+			const unsigned *pins2;
+			unsigned num_pins2;
+			int i, j, found = 0;
+
+			if (s2->type != PIN_MAP_TYPE_MUX_GROUP)
+				continue;
+
+			pctldev2 = s2->pctldev;
+			if (pctldev1 != pctldev2) {
+				dev_dbg(dev, "pctldev must be the same for states\n");
+				return -EINVAL;
+			}
+			pctlops2 = pctldev2->desc->pctlops;
+			res = pctlops2->get_group_pins(pctldev2,
+						       s2->data.mux.group,
+						       &pins2, &num_pins2);
+			if (res) {
+				dev_dbg(dev, "could not get state2 group pins\n");
+				return -EINVAL;
+			}
+
+			for (i = 0; i < num_pins1; i++) {
+				int pin1 = pins1[i];
+
+				for (j = 0; j < num_pins2; j++) {
+					int pin2 = pins2[j];
+
+					if (pin1 == pin2) {
+						found++;

drivers/pinctrl/core.c unapply_new_state: · line 1115

Quoted 4 lines

drivers/pinctrl/core.c +4 -0

+						break;
+					}
+				}
+			}

4 levels of nested loops ? Isn't this way too much ? OTOH, it points to the fact that, perhaps, a list isn't the best data structure for pinctrl ??

Or perhaps you could just assume that if num_pins1 == num_pins2 it's enough ? But that will, likely, leave some uncovered corners...

To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state.

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users.

With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime.

Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks.

Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic().

drivers/pinctrl/core.c +6 -0

+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+			  struct pinctrl_state *st2)
+{
+	struct pinctrl_setting *s1, *s2;
+
+	list_for_each_entry(s1, &st1->settings, node) {

drivers/pinctrl/core.c +1 -0

+		list_for_each_entry(s2, &st2->settings, node) {

drivers/pinctrl/core.c +4 -0

+			for (i = 0; i < num_pins1; i++) {
+				int pin1 = pins1[i];
+
+				for (j = 0; j < num_pins2; j++) {

+int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)

Quoted 24 lines · Tony Lindgren

To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state.

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users.

With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime.

Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks.

Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic().

drivers/pinctrl/core.c static void pinctrl_free(struct pinctrl *p, bool inlist) · line 891

Quoted 5 lines

drivers/pinctrl/core.c +2 -0

@@ -887,6 +887,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
 		list_for_each_entry_safe(setting, n2, &state->settings, node) {
 			pinctrl_free_setting(state == p->state[PINCTRL_STATIC],
 					     setting);
+			pinctrl_free_setting(state == p->state[PINCTRL_DYNAMIC],
+					     setting);

This will cause pinmux_free_setting() or pinconf_free_setting() to be called twice on the setting object. Right now they don't do anything, but if they start to kfree() some dynamically-allocated data that's stored within the setting, that'll cause problems. I would suggest a loop body something more like:

bool disable_setting = state == XXX || state == YYY; pinctrl_free_setting(disable_setting, setting);

Quoted 6 lines

drivers/pinctrl/core.c +6 -0

+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+			  struct pinctrl_state *st2)
+{
+	struct pinctrl_setting *s1, *s2;
+
+	list_for_each_entry(s1, &st1->settings, node) {

...

Quoted 1 line

drivers/pinctrl/core.c +1 -0

+		list_for_each_entry(s2, &st2->settings, node) {

...

drivers/pinctrl/core.c unapply_new_state: · line 1094

Quoted 4 lines

drivers/pinctrl/core.c +4 -0

+			if (pctldev1 != pctldev2) {
+				dev_dbg(dev, "pctldev must be the same for states\n");
+				return -EINVAL;
+			}

I don't think we should require that; it's perfectly legal at the moment for some device's pinctrl configuration to include settings from multiple different pin controllers.

Quoted 4 lines

drivers/pinctrl/core.c +4 -0

+			for (i = 0; i < num_pins1; i++) {
+				int pin1 = pins1[i];
+
+				for (j = 0; j < num_pins2; j++) {

drivers/pinctrl/core.c unapply_new_state: · line 1117

Quoted 10 lines

drivers/pinctrl/core.c +10 -0

+					int pin2 = pins2[j];
+
+					if (pin1 == pin2) {
+						found++;
+						break;
+					}
+				}
+			}
+
+			if (found != num_pins1) {

I guess this make sure that every entry in the dynamic state is in the state state, but not vice-versa; the static state can affect more stuff than the dynamic state?

If so, shouldn't that check be if (found != num_pins2)?

Quoted 1 line

+int pinctrl_select_dynamic(struct pinctrl *p, struct pinctrl_state *state)

The body of this function is rather duplicated with pinctrl_select().

drivers/pinctrl/core.c static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta · line 1375

Quoted 7 lines

drivers/pinctrl/core.c +2 -1

@@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
 		return 0;
 	if (IS_ERR(state))
 		return 0; /* No such state */
-	ret = pinctrl_select_state(pins->p, state);
+
+	/* Configured for proper dynamic muxing? */

I don't think "proper" is correct here; it's just fine not to have any dynamic configuration.

drivers/pinctrl/core.c static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta · line 1379

Quoted 4 lines

drivers/pinctrl/core.c +4 -0

+	if (!IS_ERR(dev->pins->active_state))
+		ret = pinctrl_select_dynamic(pins->p, state);
+	else
+		ret = pinctrl_select_state(pins->p, state);

Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the "default" state and never do anything else.

Looking back at patch 1, I see the are pinctrl_pm_select_{default,sleep,idle}_state(). Shouldn't that list be active/sleep/idle, since I assume default is that "static" state, and the other three are the "dynamic" states?

drivers/pinctrl/core.c int pinctrl_pm_select_idle_state(struct device *dev) · line 1428

Quoted 2 lines

drivers/pinctrl/core.c +2 -0

+static int pinctrl_pm_check_state(struct device *dev,
+				  struct pinctrl_state *state)

Naming this pinctrl_pm_is_state_error() or pinctrl_pm_is_state_ok() might give more clue what its return value is expected to be.

Quoted 10 lines · Stephen Warren

Quoted 6 lines · Tony Lindgren

drivers/pinctrl/core.c +6 -0

+int pinctrl_check_dynamic(struct device *dev, struct pinctrl_state *st1,
+			  struct pinctrl_state *st2)
+{
+	struct pinctrl_setting *s1, *s2;
+
+	list_for_each_entry(s1, &st1->settings, node) {

...

Quoted 1 line

drivers/pinctrl/core.c +1 -0

+		list_for_each_entry(s2, &st2->settings, node) {

...

Quoted 8 lines

drivers/pinctrl/core.c unapply_new_state: · line 1094

Quoted 4 lines

drivers/pinctrl/core.c +4 -0

+			if (pctldev1 != pctldev2) {
+				dev_dbg(dev, "pctldev must be the same for states\n");
+				return -EINVAL;
+			}

I don't think we should require that; it's perfectly legal at the moment for some device's pinctrl configuration to include settings from multiple different pin controllers.

Yes that's fine for pinctrl_select(), but let's not do that for runtime muxing.

Quoted 19 lines

Quoted 4 lines

drivers/pinctrl/core.c +4 -0

+			for (i = 0; i < num_pins1; i++) {
+				int pin1 = pins1[i];
+
+				for (j = 0; j < num_pins2; j++) {

drivers/pinctrl/core.c unapply_new_state: · line 1117

Quoted 10 lines

drivers/pinctrl/core.c +10 -0

+					int pin2 = pins2[j];
+
+					if (pin1 == pin2) {
+						found++;
+						break;
+					}
+				}
+			}
+
+			if (found != num_pins1) {

I guess this make sure that every entry in the dynamic state is in the state state, but not vice-versa; the static state can affect more stuff than the dynamic state?

Quoted 1 line

If so, shouldn't that check be if (found != num_pins2)?

The check is that idle_state and sleep_state pins must match the active_state pins. This is intentionally different from the current pinctrl_select() logic.

Quoted 22 lines · Stephen Warren

Quoted 20 lines · Tony Lindgren

To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state.

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users.

With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime.

Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks.

Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic().

Quoted 15 lines

drivers/pinctrl/core.c static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta · line 1379

Quoted 11 lines

drivers/pinctrl/core.c +6 -1

@@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
 		return 0;
 	if (IS_ERR(state))
 		return 0; /* No such state */
-	ret = pinctrl_select_state(pins->p, state);
+
+	/* Configured for proper dynamic muxing? */
+	if (!IS_ERR(dev->pins->active_state))
+		ret = pinctrl_select_dynamic(pins->p, state);
+	else
+		ret = pinctrl_select_state(pins->p, state);

Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the "default" state and never do anything else.

This keeps the existing behaviour. We should be able to drop the call to pinctrl_select_state() here after the existing use in drivers has been fixed.

Quoted 39 lines · Tony Lindgren

Quoted 22 lines · Stephen Warren

Quoted 20 lines · Tony Lindgren

To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state.

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users.

With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime.

Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks.

Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic().

Quoted 15 lines

drivers/pinctrl/core.c static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta · line 1379

Quoted 11 lines

drivers/pinctrl/core.c +6 -1

@@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
 		return 0;
 	if (IS_ERR(state))
 		return 0; /* No such state */
-	ret = pinctrl_select_state(pins->p, state);
+
+	/* Configured for proper dynamic muxing? */
+	if (!IS_ERR(dev->pins->active_state))
+		ret = pinctrl_select_dynamic(pins->p, state);
+	else
+		ret = pinctrl_select_state(pins->p, state);

Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the "default" state and never do anything else.

Quoted 3 lines

This keeps the existing behaviour. We should be able to drop the call to pinctrl_select_state() here after the existing use in drivers has been fixed.

How many DT-based systems already have multiple of default/idle/sleep states defined in DT? Right now, since the kernel code uses pinctrl_select_state() to switch between those, the state definitions need to define /all/ pins used by those states, not just the dynamic ones. However, with this series in place, the default state should only include the static pins, and the active/idle/sleep states should only include the dynamic pins. That's a change to the DT bindings, since it changes how the DT must be written, and causes older DTs to be incompatible with newer kernels and vice-versa.

So, do we just ignore this DT ABI change again, or insist on doing it in some compatible way? DT ABI-ness is a PITA:-(

Quoted 44 lines · Stephen Warren

Quoted 39 lines · Tony Lindgren

Quoted 22 lines · Stephen Warren

Quoted 20 lines · Tony Lindgren

To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state.

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users.

With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime.

Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks.

Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic().

Quoted 15 lines

drivers/pinctrl/core.c static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta · line 1379

Quoted 11 lines

drivers/pinctrl/core.c +6 -1

@@ -1241,7 +1371,13 @@ static int pinctrl_pm_select_state(struct device *dev, struct pinctrl_state *sta
 		return 0;
 	if (IS_ERR(state))
 		return 0; /* No such state */
-	ret = pinctrl_select_state(pins->p, state);
+
+	/* Configured for proper dynamic muxing? */
+	if (!IS_ERR(dev->pins->active_state))
+		ret = pinctrl_select_dynamic(pins->p, state);
+	else
+		ret = pinctrl_select_state(pins->p, state);

Hmmm. I'm not quite sure this is right... Surely this function should just do nothing if the dynamic states aren't defined? The system should just, and only, be in the "default" state and never do anything else.

Quoted 3 lines

This keeps the existing behaviour. We should be able to drop the call to pinctrl_select_state() here after the existing use in drivers has been fixed.

Quoted 9 lines

How many DT-based systems already have multiple of default/idle/sleep states defined in DT? Right now, since the kernel code uses pinctrl_select_state() to switch between those, the state definitions need to define /all/ pins used by those states, not just the dynamic ones. However, with this series in place, the default state should only include the static pins, and the active/idle/sleep states should only include the dynamic pins. That's a change to the DT bindings, since it changes how the DT must be written, and causes older DTs to be incompatible with newer kernels and vice-versa.

Well we can keep the current behaviour with pinctrl_select_state() around as long as needed. For the legacy cases, there is no active state defined and we fall back to pinctrl_select_state().

Quoted 2 lines

So, do we just ignore this DT ABI change again, or insist on doing it in some compatible way? DT ABI-ness is a PITA:-(

I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined.

Quoted 3 lines · Tony Lindgren

I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined.

Yes, I think that will work, since the active state cannot exist before this new scheme is in place.

But, this needs to be very clearly spell out in the DT binding
documentation: If you have states default/idle/sleep, they're complete alternatives, whereas if you have states default/active/idle/sleep, the latter 3 are alternatives that build on top of the first. I foresee mass confusion, but perhaps I'm being pessimistic.

Quoted 7 lines · Stephen Warren

Quoted 3 lines · Tony Lindgren

I'd vote for keeping the existing behaviour with pinctrl_select_state() when no active state is defined.

Yes, I think that will work, since the active state cannot exist before this new scheme is in place.

Right.

Quoted 5 lines

But, this needs to be very clearly spell out in the DT binding
documentation: If you have states default/idle/sleep, they're complete alternatives, whereas if you have states default/active/idle/sleep, the latter 3 are alternatives that build on top of the first. I foresee mass confusion, but perhaps I'm being pessimistic.

I'm hoping we can automate the runtime PM handling with default/active/idle completely from the consumer driver point of view. And then when that's working, we can probably deprecate any runtime PM related handling using pinctr_select_state() and print warnings. And we can also improve the documentation so no new users will use the default/idle/sleep for runtime PM unless they really want to.

I was thinking more about people writing the device trees that define these states; they need to explicitly make the choice re: overlapping states or independent states. We should not plan to obsolete any current usage of overlapping states since that will mean an incompatible change to the DT ABI (deprecate yes so that no more usage is added, but the kernel should still support the old way).

To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state.

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users.

With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime.

Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks.

Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic().

Quoted 12 lines · Tony Lindgren

To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state.

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users.

With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime.

Quoted 8 lines

Then with the states pre-validated, pinctrl_select_dynamic() can just toggle between the dynamic states without extra checks.

Note that pinctr_select_state() still has valid use cases, such as changing states when the pins can be shared between two drivers and don't necessarily cover the same pingroups. For dynamic runtime toggling of pin states, we should eventually always use just pinctrl_select_dynamic().

Something in this series should edit Documentation/pinctrl.txt to explain the philosophy behind the static/dynamic state split. That philosophy and/or semantics are more important to review than the code...

Related to that, I'm worried that using pinctrl_select_state() and pinctrl_select_dynamic() appear to be mutually-exclusive options here.

Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be?

Quoted 1 line · Stephen Warren

On 07/16/2013 03:05 AM, Tony Lindgren wrote:

Quoted 6 lines

Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be?

The idea is that runtime PM will never touch the basic mux settings at all. The "default" state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device.

For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init.

So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM.

Quoted 2 lines · Tony Lindgren

Quoted 1 line · Stephen Warren

On 07/16/2013 03:05 AM, Tony Lindgren wrote:

...

Quoted 13 lines

Quoted 6 lines

Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be?

The idea is that runtime PM will never touch the basic mux settings at all. The "default" state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device.

Quoted 7 lines

For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init.

So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM.

So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses:

a) bus 1: I2C controller is muxed out onto one set of pins.

b) bus 2: I2C controller is muxed out onto another set of pins.

Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to "the" active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as "active"...

Quoted 32 lines · Stephen Warren

Quoted 2 lines · Tony Lindgren

Quoted 1 line · Stephen Warren

On 07/16/2013 03:05 AM, Tony Lindgren wrote:

...

Quoted 13 lines

Quoted 6 lines

Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be?

The idea is that runtime PM will never touch the basic mux settings at all. The "default" state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device.

Quoted 7 lines

For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init.

So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM.

So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses:

a) bus 1: I2C controller is muxed out onto one set of pins.

b) bus 2: I2C controller is muxed out onto another set of pins.

Quoted 10 lines

Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to "the" active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as "active"...

I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances.

For the pin muxing part, I'd do this:

i2c1 instance i2c2 instance notes
default_state 0 pins 0 pins (or dedicated pins only)
active_state all pins alls pins
idle_state safe mode safe mode

Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2.

Regards,

Tony

Hi Tony, Stephen

Quoted 57 lines · Tony Lindgren

Quoted 33 lines · Stephen Warren

Quoted 2 lines · Tony Lindgren

Quoted 1 line · Stephen Warren

On 07/16/2013 03:05 AM, Tony Lindgren wrote:

...

Quoted 14 lines

Quoted 7 lines

Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or
does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be?

The idea is that runtime PM will never touch the basic mux settings at all. The "default" state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device.

Quoted 7 lines

For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init.

So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM.

So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses:

a) bus 1: I2C controller is muxed out onto one set of pins.

b) bus 2: I2C controller is muxed out onto another set of pins.

Quoted 10 lines

Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to "the" active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as "active"...

I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances.

For the pin muxing part, I'd do this:

i2c1 instance i2c2 instance notes
default_state 0 pins 0 pins (or dedicated pins only)
active_state all pins alls pins
idle_state safe mode safe mode

Quoted 3 lines

Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2.

First of all, I'd like to mention that these patches do *not* connect pinctrl to PM runtime, so until driver will call pinctrl_select_state() or pinctrl_pm_select_*() there will be no pins state changes.

(As result, i2c-mux is not good example, seems:))

Quoted 3 lines · Grygorii Strashko

First of all, I'd like to mention that these patches do *not* connect pinctrl to PM runtime, so until driver will call pinctrl_select_state() or pinctrl_pm_select_*() there will be no pins state changes.

Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be called automatically by the runtime PM core,

so that we don't need to
write code to do this in every single driver, just like we moved the call to pinctrl_select_state(default) into the device core so that we didn't have to make every device do that manually?

Quoted 4 lines · Grygorii Strashko

First of all, I'd like to mention that these patches do *not* connect pinctrl to PM runtime, so until driver will call pinctrl_select_state() or pinctrl_pm_select_*() there will be no pins state changes.

Isn't the whole point of the pinctrl_pm_select*() APIs to eventually be called automatically by the runtime PM core, so that we don't need to write code to do this in every single driver, just like we moved the call to pinctrl_select_state(default) into the device core so that we didn't have to make every device do that manually?

Quoted 1 line

(As result, i2c-mux is not good example, seems:))

As such, I think all situations are good examples, because a generic feature has to work in all cases.

Quoted 32 lines · Stephen Warren

Quoted 2 lines · Tony Lindgren

Quoted 1 line · Stephen Warren

On 07/16/2013 03:05 AM, Tony Lindgren wrote:

...

Quoted 13 lines

Quoted 6 lines

Why shouldn't e.g. a pinctrl-based I2C mux also be able to do runtime PM? Does the mux setting select which states are used for runtime PM, or does runtime PM override the basic mux setting, or must the pincrl-I2C mux manually implement custom runtime-PM/pinctrl interaction since there's no generic answer to those questions? How many more custom exceptions will there be?

The idea is that runtime PM will never touch the basic mux settings at all. The "default" state should be considered a static state that is claimed during driver probe, and released when the driver is unloaded. This is typically let's say 90% of the pins for any device.

Quoted 7 lines

For runtime PM, we can just toggle the PM related pinctrl states as they have been verified to match the active state during init.

So I don't see why pinctrl-I2C would have to do anything specific. All that is required is that the pins are grouped for the consumer driver, and we can provide some automated checks on the states for runtime PM.

So, consider a pinctrl-based I2C mux. It has 2 states to cover two I2C buses:

a) bus 1: I2C controller is muxed out onto one set of pins.

b) bus 2: I2C controller is muxed out onto another set of pins.

Quoted 10 lines

Now, the system could go idle in either of those 2 states, and then later need to return to one of those states. I just don't see how that would work, since the runtime PM code in this series switches to *an* active state when the device becomes non-idle. If the definition of the idle state switches the mux function for both sets of pins to some idle/quiescent value, then you'd need to do different reprogramming when going back to "the" active state; I guess the system would need to remember which state was active before switching to idle, then switch back to that same state rather than hard-coding the active state name as "active"...

I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances.

For the pin muxing part, I'd do this:

i2c1 instance i2c2 instance notes
default_state 0 pins 0 pins (or dedicated pins only)
active_state all pins alls pins
idle_state safe mode safe mode

Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2.

Quoted 8 lines · Tony Lindgren

I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances.

For the pin muxing part, I'd do this:

Quoted 8 lines

i2c1 instance i2c2 instance notes
default_state 0 pins 0 pins (or dedicated pins only)
active_state all pins alls pins
idle_state safe mode safe mode

Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2.

I see two issues with that approach:

1) Runtime PM doesn't always put a device into an idle state as soon as its work is done. Sometimes (always?) there is a delay between when the device is last used and when the HW is actually made idle so that if the device is re-activated quickly, the kernel hasn't wasted time making it idle then active again. You'd have to force that delay to complete when switching between the two virtual controller devices.

2) This requires two separate device objects for the two I2C instances. I guess you could have the driver for the one I2C mux node end up instantiating two child devices for this purpose, and hence make that happen without needing to change the DT ABI. However, that sure feels complex...

I wonder if it wouldn't be better to have active/idle/sleep as "modifiers" to the state name rather than alternatives, so you end up with states named:

default
nobus:active
nobus:idle
nobus:sleep
bus0:active
bus0:idle
bus0:sleep
bus1:active
bus1:idle
bus1:sleep

Of course, if you continue on with that approach (i.e. you add more sub-fields each separated by a colon), you end up with a huge combinatorial mess of state names.

Quoted 18 lines · Stephen Warren

Quoted 8 lines · Tony Lindgren

I think the only sane way to deal with this is to make the I2C controller to show up as two separate I2C controller instances. Then use runtime PM to save and restore the state for each instance, and locking between the two driver instances.

For the pin muxing part, I'd do this:

Quoted 8 lines

i2c1 instance i2c2 instance notes
default_state 0 pins 0 pins (or dedicated pins only)
active_state all pins alls pins
idle_state safe mode safe mode

Then when i2c1 instance is done, it's runtime PM suspend muxes the pins to safe mode, or some nop state. Then when i2c2 instance is woken, it's runtime PM resume muxes pins to i2c2.

Quoted 8 lines

I see two issues with that approach:

1) Runtime PM doesn't always put a device into an idle state as soon as its work is done. Sometimes (always?) there is a delay between when the device is last used and when the HW is actually made idle so that if the device is re-activated quickly, the kernel hasn't wasted time making it idle then active again. You'd have to force that delay to complete when switching between the two virtual controller devices.

There is the autosuspend timeout for delayed transitions, but I think runtime PM already has calls for making the state change immediate also.

Quoted 5 lines

2) This requires two separate device objects for the two I2C instances. I guess you could have the driver for the one I2C mux node end up instantiating two child devices for this purpose, and hence make that happen without needing to change the DT ABI. However, that sure feels complex...

Yes but you will also automatically get quite a bit of sanity to your I2C driver code rather than trying to handle the two separate instances within the driver alone. Consider things like scanning the I2C buses for devices and just dev_dbg().

To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state.

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users.

With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime.

Quoted 4 lines · Tony Lindgren

To toggle dynamic states, let's add the optional active state in addition to the static default state. Then if the optional active state is defined, we can require that idle and sleep states cover the same pingroups as the active state.

OK...

Quoted 3 lines

Then let's add pinctrl_check_dynamic() and pinctrl_select_dynamic() to use instead of pinctrl_select() to avoid breaking existing users.

Quoted 3 lines

With pinctrl_check_dynamic() we can check that idle and sleep states match the active state for pingroups during init, and don't need to do it during runtime.

I do not understand why this complexity need to be exposed outside of the subsystem.

pinctrl_select_state() and the PM accessors are enough IMO. Why should say a driver care about whether it is dynamic or not?

Surely the checking and different paths for static/dynamic configurations can be an intrinsic detail of the pinctrl subsystem, by adding flags and members to private structs like struct pinctrl itself in worst case.

So I'm not buying into this, it looks like it is making things complicated for consumers outside the subsystem for no reason.

We want to have static pin states handled separately from dynamic pin states, so let's add optional state_active.

Then if state_active is defined, let's check and make sure state_idle and state_sleep match state_active for the pin groups to avoid checking them during runtime as the active and idle pins may need to be toggled for many devices every time we enter and exit idle.

drivers/base/pinctrl.c +2 -0

+	 * Note that if active state is defined, sleep and idle states must
+	 * cover the same pin groups as active state.

Quoted 8 lines · Tony Lindgren

We want to have static pin states handled separately from dynamic pin states, so let's add optional state_active.

Then if state_active is defined, let's check and make sure state_idle and state_sleep match state_active for the pin groups to avoid checking them during runtime as the active and idle pins may need to be toggled for many devices every time we enter and exit idle.

drivers/base/pinctrl.c int pinctrl_bind_pins(struct device *dev) · line 83

Quoted 12 lines

drivers/base/pinctrl.c +6 -1

+	 * Note that if active state is defined, sleep and idle states must
+	 * cover the same pin groups as active state.
 	 */
 	dev->pins->sleep_state = pinctrl_lookup_state(dev->pins->p,
 					PINCTRL_STATE_SLEEP);
-	if (IS_ERR(dev->pins->sleep_state))
+	if (IS_ERR(dev->pins->sleep_state)) {
 		/* Not supplying this state is perfectly legal */
 		dev_dbg(dev, "no sleep pinctrl state\n");
+	} else if (!IS_ERR(dev->pins->active_state)) {
+		ret = pinctrl_check_dynamic(dev, dev->pins->active_state,
+					    dev->pins->sleep_state);

Oh, I see you're trying to check that the set of pins in the active, sleep, and idle states are identical.

But I think that pinctrl_check_dynamic() only checks that one state is a subset of the other, not that the two states are equal. Instead, I think you want to comparison coded in pinctrl_check_dynamic() to be:

gen_group_list_of_pinctrl_state(s1, array1);
gen_group_list_of_pinctrl_state(s2, array2);
mismatch = memcmp(array1, array2, length);

Quoted 3 lines · Stephen Warren

Oh, I see you're trying to check that the set of pins in the active, sleep, and idle states are identical.

Right, that's to avoid any further checking during runtime for runtime PM.

Quoted 3 lines

But I think that pinctrl_check_dynamic() only checks that one state is a subset of the other, not that the two states are equal. Instead, I think you want to comparison coded in pinctrl_check_dynamic() to be:

In pinctrl_check_dynamic() we check that the pins match between the states, and the number of found pins matches the first set. I'll take a look if we check the total pins between the two sets.

Quoted 3 lines

gen_group_list_of_pinctrl_state(s1, array1);
gen_group_list_of_pinctrl_state(s2, array2);
mismatch = memcmp(array1, array2, length);

Well we could allocate and sort the pins, but the number of pins for runtime PM is typically very small for each pin consumer device. Typically you just need to toggle RX pin to GPIO mode for idle. And this check is only done during consumer driver probe time. So optimizing it for larger sets could be done at any point later on as needed.

As discussed earlier, the pinctrl support for changing some of the consumer device pins during runtime needs some improvment.

Here are the patches to do that, I'll also post a minimal sample patch as a reply to this thread on how to do the muxing for runtime PM.

Hi all,

As discussed earlier, the pinctrl support for changing some of the consumer device pins during runtime needs some improvment.

Here are the patches to do that, I'll also post a minimal sample patch as a reply to this thread on how to do the muxing for runtime PM.

Regards,

Tony

---

Tony Lindgren (4):
pinctrl: Remove duplicate code in pinctrl_pm_select_state functions
pinctrl: Allow pinctrl to have multiple active states
pinctrl: Add support for additional dynamic states
drivers: Add pinctrl handling for dynamic pin states

  drivers/base/pinctrl.c                |   39 +++++
  drivers/pinctrl/core.c                |  250 ++++++++++++++++++++++++++++-----
  drivers/pinctrl/core.h                |   10 +
  include/linux/pinctrl/consumer.h      |   46 ++++++
  include/linux/pinctrl/devinfo.h       |    4 +
  include/linux/pinctrl/pinctrl-state.h |   15 ++
  6 files changed, 321 insertions(+), 43 deletions(-)