pinctrl: qcom: add support for sparse GPIOs

drivers/pinctrl/qcom/pinctrl-msm.c +0 -0

drivers/pinctrl/qcom/pinctrl-msm.c +11 -1

@@ -825,13 +897,39 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->owner = THIS_MODULE;
 	chip->of_node = pctrl->dev->of_node;
 
+	/* If the GPIO map is sparse, then we need to disable specific IRQs */
+	chip->irq_need_valid_mask = pctrl->soc->sparse;
+
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed register gpiochip\n");
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
+	/*
+	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
+	 * initialize irq_valid_mask to all 1s.  We need to clear all the
+	 * GPIOs that are unavailable, and we need to find each block
+	 * of consecutive available GPIOs are add them as pin ranges.
+	 */
+	if (chip->irq_need_valid_mask) {
+		for (i = 0; i < ngpio; i++)

Quoted 12 lines

drivers/pinctrl/qcom/pinctrl-msm.c +12 -0

+	/*
+	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
+	 * initialize irq_valid_mask to all 1s.  We need to clear all the
+	 * GPIOs that are unavailable, and we need to find each block
+	 * of consecutive available GPIOs are add them as pin ranges.
+	 */
+	if (chip->irq_need_valid_mask) {
+		for (i = 0; i < ngpio; i++)
+			if (!groups[i].npins)
+				clear_bit(i, pctrl->chip.irq_valid_mask);
+
+		while ((count = msm_gpio_get_next_range(pctrl, &start))) {

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 926

Quoted 6 lines

drivers/pinctrl/qcom/pinctrl-msm.c +6 -0

+			ret = gpiochip_add_pin_range(&pctrl->chip,
+						     dev_name(pctrl->dev),
+						     start, start, count);
+			if (ret)
+				break;
+			start += count;

I do not fancy the idea of specifying a bitmap of valid irq pins and then having the driver register the pin-ranges in-between.

If we provide
a bitmap of validity to the core it should support using this for the pins as well. (Which I believe is what Linus answered in the discussion following patch 0/2)

Quoted 17 lines · Bjorn Andersson

Quoted 17 lines

drivers/pinctrl/qcom/pinctrl-msm.c +12 -0

+       /*
+        * If irq_need_valid_mask is true, then gpiochip_add_data() will
+        * initialize irq_valid_mask to all 1s.  We need to clear all the
+        * GPIOs that are unavailable, and we need to find each block
+        * of consecutive available GPIOs are add them as pin ranges.
+        */
+       if (chip->irq_need_valid_mask) {
+               for (i = 0; i < ngpio; i++)
+                       if (!groups[i].npins)
+                               clear_bit(i, pctrl->chip.irq_valid_mask);
+
+               while ((count = msm_gpio_get_next_range(pctrl, &start)))

{

drivers/pinctrl/qcom/pinctrl-msm.c +2 -0

+                       ret = gpiochip_add_pin_range(&pctrl->chip,
+

dev_name(pctrl->dev),

Quoted 8 lines

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_init(struct msm_pinctrl *pctrl) · line 926

Quoted 5 lines

+ start, start,
count);

drivers/pinctrl/qcom/pinctrl-msm.c +3 -0

+                       if (ret)
+                               break;
+                       start += count;

I do not fancy the idea of specifying a bitmap of valid irq pins and then having the driver register the pin-ranges in-between.

But that's exactly what abx500_gpio_probe() in pinctrl-abx500.c does. Here's even a reference to holes in the GPIO space:

Quoted 5 lines

If we provide

a bitmap of validity to the core it should support using this for the pins as well. (Which I believe is what Linus answered in the discussion following patch 0/2)

So you want to change "gpio_chip" to add an "available" callback? And every time gpiolib wants to call a gpio_chip callback, it should call ->available first? Like this:

if (chip->available && chip->available())

        status = chip->direction_input(chip, gpio_chip_hwgpio(desc));

I mean that you add
unsigned long *line_valid_mask;
to struct gpio_chip using the same type of logic as .irq_valid_mask. The mask should be optional.

Then the operation gpiod_get[_index]() will FAIL if the line is not valid.

There is no need to check on every operation, there should be no way to get a handle on the pin any other way.

All new code should be using descriptors at this point so we only need to design for that case.

I mean that you add
unsigned long *line_valid_mask;
to struct gpio_chip using the same type of logic as .irq_valid_mask. The mask should be optional.

Then the operation gpiod_get[_index]() will FAIL if the line is not valid.

There is no need to check on every operation, there should be no way to get a handle on the pin any other way.

All new code should be using descriptors at this point so we only need to design for that case.

drivers/pinctrl/qcom/pinctrl-msm.c +13 -1

 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed register gpiochip\n");
 		return ret;
 	}
 
-	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
+	/*
+	 * If irq_need_valid_mask is true, then gpiochip_add_data() will
+	 * initialize irq_valid_mask to all 1s.  We need to clear all the
+	 * GPIOs that are unavailable, and we need to find each block
+	 * of consecutive available GPIOs are add them as pin ranges.
+	 */
+	if (chip->irq_need_valid_mask) {
+		for (i = 0; i < ngpio; i++)
+			if (!groups[i].npins)
+				clear_bit(i, pctrl->chip.irq_valid_mask);
+
+		while ((count = msm_gpio_get_next_range(pctrl, &start))) {
+			ret = gpiochip_add_pin_range(&pctrl->chip,

This is the bit that I don't understand. If you only tell gpiolib about the GPIOs that exist, then it won't be initializing the .irq_valid_mask in a wrong way.

In other words, what I'm trying to say is that in your case, ngpio isn't equal to the sum of .npins over all groups.

If instead you make the chip
register only the lines that actually exist, .irq_valid_mask will only contain bits that do exist.

The reason I'm bringing this up is because we had the same discussion back in November last year (yes, that driver still isn't upstream...):

	https://lkml.org/lkml/2016/11/22/543

In a nutshell: the Tegra driver was assuming that each port had a fixed number (8) of lines, but when gpiolib changed to query the direction of GPIOs at driver probe time this broke badly because on of the instances of the GPIO controller is very strict about what it allows access to.

This seems to be similar to what you're experiencing. In our case we'd run into a hard hang trying to access a register for a non-existing GPIO. One of the possibilities discussed in the thread was to introduce something akin to what's being proposed here.

In the end it turned out to be easiest to just don't tell gpiolib about any non-existing GPIOs, because then you don't get to deal with any of these workarounds.

The downside is that you need a little more code to
find out exactly what GPIO you're talking about, or to determine how many you have.

In the end I think it's all worth it by making it a lot easier in general to deal with. I think it's really confusing if you expose, say, 200 pins, and for 50 of them users will get -EACCES, or -ENOENT or whatever if they try to use them.

First patch allows for for pinctrl-msm to understand GPIO groups with no pins. Such pins are "hidden" and can't be exported or accessed.

Second patch updates the QDF2xxx driver to take advantage of all that.

v5:
Since gpiochip_add_data no longer requests GPIOs before scanning for the direction (that patch was reverted), pinctrl-msm.c now specifically checks for special case.

Quoted 3 lines

Since gpiochip_add_data no longer requests GPIOs before scanning for the direction (that patch was reverted), pinctrl-msm.c now specifically checks for special case.

Waiting for Bjorn's review on these.

First patch allows for for pinctrl-msm to understand GPIO groups with no pins. Such pins are "hidden" and can't be exported or accessed.

Second patch updates the QDF2xxx driver to take advantage of all that.

v5:
Since gpiochip_add_data no longer requests GPIOs before scanning for the direction (that patch was reverted), pinctrl-msm.c now specifically checks for special case.

Perhaps we can add another hook for our purposes here that tells gpiolib that the gpio is not usable and to skip it. The semantics would be clear, it's just about probing availability of this pin as a gpio and doesn't mux any pins.

Oh we already have that I think, Mika Westerberg and Andy Shevcheno implemented that for anyone using CONFIG_GPIOLIB_IRQCHIP, and this driver does. Timur please check: irq_need_valid_mask, irq_valid_mask usage.

Quoted 5 lines · Linus Walleij

Oh we already have that I think, Mika Westerberg and Andy Shevcheno implemented that for anyone using CONFIG_GPIOLIB_IRQCHIP, and this driver does. Timur please check: irq_need_valid_mask, irq_valid_mask usage.

These patches already use irq_valid_mask! But that doesn't block complete access to the GPIO.

Doesn't that mean we need something like irq_valid_mask but rather gpio_valid_mask that just block all usage of certain GPIOs?

Quoted 3 lines · Linus Walleij

Doesn't that mean we need something like irq_valid_mask but rather gpio_valid_mask that just block all usage of certain GPIOs?

That raises a lot of questions. In the meantime, my current patches for 4.14 work fine.

Do we replace irq_valid_mask with gpio_valid_mask? That would break drivers where the GPIO is valid but the interrupt is not. If we keep both, what happens if gpio_valid_mask is false but irq_valid_mask is true? And then we would need to audit all gpio drivers to see which ones should be updated for the new infrastructure.

I guess gpio_valid_mask would take precedence over irq_valid_mask. I.e if the GPIO is not valid then the IRQ is per definition not valid either.

Since it is a new thing, we can simply define a semantic like that and document it.

Quoted 7 lines · Linus Walleij

I guess gpio_valid_mask would take precedence over irq_valid_mask. I.e if the GPIO is not valid then the IRQ is per definition not valid either.

Since it is a new thing, we can simply define a semantic like that and document it.

So what about my current patches?

I hope you're not asking me to rewrite
them again.

[Stephen Boyd]
Perhaps we can add another hook for our purposes here that tells gpiolib that the gpio is not usable and to skip it. The semantics would be clear, it's just about probing availability of this pin as a gpio and doesn't mux any pins.

Doesn't that mean we need something like irq_valid_mask but rather gpio_valid_mask that just block all usage of certain GPIOs?

What is your response to Stephen's comment:

Quoted 5 lines

[Stephen Boyd]
Perhaps we can add another hook for our purposes here that tells gpiolib that the gpio is not usable and to skip it. The semantics would be clear, it's just about probing availability of this pin as a gpio and doesn't mux any pins.

I think this kind of related to my response (after I realized it was not just about IRQs):

Quoted 11 lines · Linus Walleij

What is your response to Stephen's comment:

Quoted 5 lines

[Stephen Boyd]
Perhaps we can add another hook for our purposes here that tells gpiolib that the gpio is not usable and to skip it. The semantics would be clear, it's just about probing availability of this pin as a gpio and doesn't mux any pins.

I think this kind of related to my response (after I realized it was not just about IRQs):

We already have 95% of this. We can already specify individual pin ranges, and the vast majority of the code recognizes the ranges. There is only one small loophole, and that's in gpiochip_add_data(). The for-loop iterates over all GPIOs:

for (i = 0; i < chip->ngpio; i++) {
struct gpio_desc *desc = &gdev->descs[i];

desc->gdev = gdev;
/*
* REVISIT: most hardware initializes GPIOs as inputs
* (often with pullups enabled) so power usage is
* minimized. Linux code should set the gpio direction
* first thing; but until it does, and in case
* chip->get_direction is not set, we may expose the
* wrong direction in sysfs.
*/

I believe the real problem is that this for-loop should be moved from gpiochip_add_data() into some other function that is called *after* the pin ranges are defined. We can put it in gpiochip_add_pin_range(), maybe.

My patch covers the loophole by adding a check inside get_direction(). If we fix gpiochip_add_data(), I can remove that patch.

However, I think that change is risky and will require a lot of testing and review.

I've run into this now on our mobile SoCs after I pull in commit 8e51533780ba ("pinctrl: qcom: add get_direction function"). Before that commit we never read each pin of the device. On our mobile SoCs we have devicetree and it feels like having that describe which pins are available and not available is half-duplicating information we would already have via consumers indicating which pins they care about. I don't see any value beyond system wide debug in figuring out the default pin configuration of a pin that doesn't have a consumer in Linux.

Could we remove the pin direction finding part here in gpiochip_add_pin_range() and lazily resolve the pin direction when a pin is requested?

We would need a similar check in the msm
specific debugfs code where we skip pins that aren't requested.

This is basically a revert of commit 72d320006177 ("gpio: set up initial state from .get_direction()").

ACPI can still describe only the pin ranges that they care about exposing, but from the devicetree side it's been working well enough to not touch pins that aren't used by anything in Linux.

Quoted 9 lines · Stephen Boyd

I've run into this now on our mobile SoCs after I pull in commit 8e51533780ba ("pinctrl: qcom: add get_direction function"). Before that commit we never read each pin of the device. On our mobile SoCs we have devicetree and it feels like having that describe which pins are available and not available is half-duplicating information we would already have via consumers indicating which pins they care about. I don't see any value beyond system wide debug in figuring out the default pin configuration of a pin that doesn't have a consumer in Linux.

At the time I wrote that patch, the ACPI tables exposed all of the GPIOs, even the ones it didn't care about. The new ACPI tables list only specific GPIOs, and so we no longer need to blindly read the direction of all GPIOs.

Quoted 3 lines

This is basically a revert of commit 72d320006177 ("gpio: set up initial state from .get_direction()").

I would be in favor of either reverting that patch, or moving the code into gpiochip_add_pin_range().

Quoted 5 lines

At the time I wrote that patch, the ACPI tables exposed all of the GPIOs, even the ones it didn't care about. The new ACPI tables list only specific GPIOs, and so we no longer need to blindly read the direction of all GPIOs.

Do you avoid this problem on new ACPI tables because only pins that are able to be read are exposed?

Quoted 5 lines

Quoted 2 lines

This is basically a revert of commit 72d320006177 ("gpio: set up initial state from .get_direction()").

I would be in favor of either reverting that patch, or moving the code into gpiochip_add_pin_range().

If it's in gpiochip_add_pin_range() would we still read the hardware when creating the pin ranges?

I don't want to have to> describe pin ranges of "valid" pins that

to blow up if we touch them, because those pins are never used by Linux so reading them is not useful.

On 10/04/2017 04:50 PM, Stephen Boyd wrote:

Yes. A recent firmware update enabled the "XPU" block which is being programmed with a select subset of individual GPIOs. On our silicon, each TLMM GPIO is in a separate 64k page, and so the XPU can block any individual GPIO. Any attempt to touch those registers causes an XPU violation which takes the whole system down.

Quoted 4 lines

If it's in gpiochip_add_pin_range() would we still read the hardware when creating the pin ranges?

I presume so. The idea is that pinctrl-qdf2xxx/pinctrl-msm only submit pin ranges that are present in the ACPI tables.

Quoted 5 lines

I don't want to have to describe pin ranges of "valid" pins that won't cause the system
to blow up if we touch them, because those pins are never used by Linux so reading them is not useful.

Well, that's exactly what I'm trying to do with current patch set :-) It seems the most logical approach to me. I don't understand the dislike for it. What else are pin ranges for, other than to specify ranges of pins that can be accessed?

Another alternative was to enumerate all of the GPIOs starting from
0. So the first GPIO in ACPI would be gpio0, regardless of what gpio number it actually was. E.g. GPIO 37 would appear as gpio0, GPIO 38 would appear as gpio1, and so on. That also worked, but it meant that customers would need to figure out which GPIO that "gpio0" actually pointed to. That was not acceptable, so I dropped it.

I'm at a loss on how else to do it. I think a gpio_chip.available callback is far less elegant than define pin ranges. There is no chance that unavailable GPIOs can be accessed because the physical addresses are not in the msm_pingroup array. That is, groups[0].ctrl_reg == 0, not 0xFF02010000.

Could we remove the pin direction finding part here in gpiochip_add_pin_range() and lazily resolve the pin direction when a pin is requested? We would need a similar check in the msm specific debugfs code where we skip pins that aren't requested. This is basically a revert of commit 72d320006177 ("gpio: set up initial state from .get_direction()").

Quoted 6 lines · Stephen Boyd

Could we remove the pin direction finding part here in gpiochip_add_pin_range() and lazily resolve the pin direction when a pin is requested? We would need a similar check in the msm specific debugfs code where we skip pins that aren't requested. This is basically a revert of commit 72d320006177 ("gpio: set up initial state from .get_direction()").

It seems reasonable for the gpiolib to be able to call this function immediately after registering the new GPIO chip with its vtable.

I think it is more up to the driver to numb the reply with some dummy return value (i.e. input mode) or refactor the callback so that it is acceptable for gpiolib to get an -EINVAL or so from the driver (again it will assume input mode) if the driver can't return the direction at this time.

For qcom platforms the driver will never be able to return the direction for these certain pins because reading the register is not allowed by the firmware. Doing so will cause the device to crash with a security violation.

So I guess the driver needs to know what pin registers it can't access so the user does not get a gun to shoot in the foot with.

If we augment gpiolib to just handle -EACCES or something (-EIO?) from the driver .get_direction() callback for these lines, things should be smooth?

I don't see any value
beyond system wide debug in figuring out the default pin configuration of a pin that doesn't have a consumer in Linux.

Could we remove the pin direction finding part here in gpiochip_add_pin_range() and lazily resolve the pin direction when a pin is requested? We would need a similar check in the msm specific debugfs code where we skip pins that aren't requested. This is basically a revert of commit 72d320006177 ("gpio: set up initial state from .get_direction()").

ACPI can still describe only the pin ranges that they care about exposing, but from the devicetree side it's been working well enough to not touch pins that aren't used by anything in Linux.

---8<----

drivers/gpio/gpiolib.c +1 -10

index cd003b74512f..673028823bc5 100644
@@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
  		 * wrong direction in sysfs.
  		 */
 
-		if (chip->get_direction) {
-			/*
-			 * If we have .get_direction, set up the initial
-			 * direction flag from the hardware.
-			 */
-			int dir = chip->get_direction(chip, i);
-
-			if (!dir)
-				set_bit(FLAG_IS_OUT, &desc->flags);
-		} else if (!chip->direction_input) {
+		if (!chip->direction_input) {
  			/*
  			 * If the chip lacks the .direction_input callback
  			 * we logically assume all lines are outputs.

drivers/pinctrl/qcom/pinctrl-msm.c +1 -1

index 273badd92561..4a0aeceb42f1 100644
@@ -24,7 +24,7 @@
  #include <linux/pinctrl/pinconf.h>
  #include <linux/pinctrl/pinconf-generic.h>
  #include <linux/slab.h>
-#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
  #include <linux/interrupt.h>
  #include <linux/spinlock.h>
  #include <linux/reboot.h>

@@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
  	};
 

Quoted 3 lines · Stephen Boyd

I don't see any value
beyond system wide debug in figuring out the default pin configuration of a pin that doesn't have a consumer in Linux.

I can agree with that.

Quoted 47 lines

Could we remove the pin direction finding part here in gpiochip_add_pin_range() and lazily resolve the pin direction when a pin is requested? We would need a similar check in the msm specific debugfs code where we skip pins that aren't requested. This is basically a revert of commit 72d320006177 ("gpio: set up initial state from .get_direction()").

ACPI can still describe only the pin ranges that they care about exposing, but from the devicetree side it's been working well enough to not touch pins that aren't used by anything in Linux.

---8<----

drivers/gpio/gpiolib.c +1 -10

index cd003b74512f..673028823bc5 100644
@@ -1210,16 +1210,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
  		 * wrong direction in sysfs.
  		 */
-		if (chip->get_direction) {
-			/*
-			 * If we have .get_direction, set up the initial
-			 * direction flag from the hardware.
-			 */
-			int dir = chip->get_direction(chip, i);
-
-			if (!dir)
-				set_bit(FLAG_IS_OUT, &desc->flags);
-		} else if (!chip->direction_input) {
+		if (!chip->direction_input) {
  			/*
  			 * If the chip lacks the .direction_input callback
  			 * we logically assume all lines are outputs.

drivers/pinctrl/qcom/pinctrl-msm.c +1 -1

index 273badd92561..4a0aeceb42f1 100644
@@ -24,7 +24,7 @@
  #include <linux/pinctrl/pinconf.h>
  #include <linux/pinctrl/pinconf-generic.h>
  #include <linux/slab.h>
-#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
  #include <linux/interrupt.h>
  #include <linux/spinlock.h>
  #include <linux/reboot.h>

drivers/pinctrl/qcom/pinctrl-msm.c static void msm_gpio_dbg_show_one(struct seq_file *s, · line 503

Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-msm.c +6 -0

@@ -494,6 +494,12 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
  	};
  	g = &pctrl->soc->groups[offset];
+
+	if (!gpiochip_is_requested(chip, gpio)) {
+		seq_printf(s, " %-8s:", g->name);
+		return;
+	}
+
  	ctl_reg = readl(pctrl->regs + g->ctl_reg);
  	is_out = !!(ctl_reg & BIT(g->oe_bit));

In order for this to work, I had to add this function from patch #1:

static int msm_gpio_request(struct gpio_chip *chip, unsigned int offset)

{
	struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
	const struct msm_pingroup *g = &pctrl->soc->groups[offset];

	if (!g->npins)
		return -ENODEV;

	return gpiochip_generic_request(chip, offset);
}

The problem with this is that none of the GPIOs are "requested", so it displays an output like this:

# cat /sys/kernel/debug/gpio
gpiochip0: GPIOs 0-149, parent: platform/QCOM8002:00, QCOM8002:00:
 (null)  :
 (null)  :
 (null)  :
[... truncated ]]
 (null)  :
 (null)  :
 (null)  :
 (null)  :
 gpio36  :
 gpio37  :
 gpio38  :
 gpio39  :
 (null)  :
 (null)  :

It can't differentiate between GPIOs that don't exist and GPIOs that haven't been requested.

This confuses me. Why would you even want to register pins that don't exist? It sounds to me like you're lying to gpiolib and then try to work around it trying to access the GPIOs that don't exist but which you told it were there.

Why not just tell gpiolib about only the GPIOs that exist?

I am waiting for the maintainer, Bjorn Andersson, to provide review.