[PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[Timur Tabi]

On Qualcomm Technologies QDF2xxx platforms, only a subset of the GPIOs
are actually available to the HLOS.  The others are blocked by the XPU,
and any attempt to access them will cause an XPU violation that halts
the system.

Instead, the ACPI table now lists only specific GPIOs that are exposed
externally as generic GPIO pins.  To maintain consistency, the GPIOs are
enumerated 0 .. (N-1) as before, so that "gpio0" is really whatever is
the first GPIO listed in ACPI.  The actual mapping is available via
/sys/kernel/debug/gpio.

Tested-by: Tyler Baicar <tbaicar@codeaurora.org>
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 46 ++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
index bb3ce5c..983df72 100644
--- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
+++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c
@@ -32,9 +32,6 @@
 
 static struct msm_pinctrl_soc_data qdf2xxx_pinctrl;
 
-/* A reasonable limit to the number of GPIOS */
-#define MAX_GPIOS	256
-
 /* maximum size of each gpio name (enough room for "gpioXXX" + null) */
 #define NAME_SIZE	8
 
@@ -43,22 +40,35 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 	struct pinctrl_pin_desc *pins;
 	struct msm_pingroup *groups;
 	char (*names)[NAME_SIZE];
-	unsigned int i;
-	u32 num_gpios;
+	unsigned int i, num_gpios;
+	u32 *gpios;
 	int ret;
 
 	/* Query the number of GPIOs from ACPI */
-	ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios);
+	num_gpios = ret = device_property_read_u32_array(&pdev->dev, "gpios",
+							 NULL, 0);
 	if (ret < 0) {
-		dev_warn(&pdev->dev, "missing num-gpios property\n");
+		dev_err(&pdev->dev,
+			"missing or invalid 'gpios' property (ret=%i)\n", ret);
 		return ret;
 	}
-
-	if (!num_gpios || num_gpios > MAX_GPIOS) {
-		dev_warn(&pdev->dev, "invalid num-gpios property\n");
+	if (ret == 0) {
+		dev_warn(&pdev->dev, "no GPIOs defined\n");
 		return -ENODEV;
 	}
 
+	gpios = devm_kcalloc(&pdev->dev, num_gpios, sizeof(u32), GFP_KERNEL);
+	if (!gpios)
+		return -ENOMEM;
+
+	ret = device_property_read_u32_array(&pdev->dev, "gpios", gpios,
+					     num_gpios);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"could not read list of GPIOs (ret=%i)\n", ret);
+		return ret;
+	}
+
 	pins = devm_kcalloc(&pdev->dev, num_gpios,
 		sizeof(struct pinctrl_pin_desc), GFP_KERNEL);
 	groups = devm_kcalloc(&pdev->dev, num_gpios,
@@ -69,7 +79,9 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	for (i = 0; i < num_gpios; i++) {
-		snprintf(names[i], NAME_SIZE, "gpio%u", i);
+		unsigned int gpio = gpios[i];
+
+		snprintf(names[i], NAME_SIZE, "gpio%u", gpio);
 
 		pins[i].number = i;
 		pins[i].name = names[i];
@@ -78,11 +90,11 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 		groups[i].name = names[i];
 		groups[i].pins = &pins[i].number;
 
-		groups[i].ctl_reg = 0x10000 * i;
-		groups[i].io_reg = 0x04 + 0x10000 * i;
-		groups[i].intr_cfg_reg = 0x08 + 0x10000 * i;
-		groups[i].intr_status_reg = 0x0c + 0x10000 * i;
-		groups[i].intr_target_reg = 0x08 + 0x10000 * i;
+		groups[i].ctl_reg = 0x10000 * gpio;
+		groups[i].io_reg = 0x04 + 0x10000 * gpio;
+		groups[i].intr_cfg_reg = 0x08 + 0x10000 * gpio;
+		groups[i].intr_status_reg = 0x0c + 0x10000 * gpio;
+		groups[i].intr_target_reg = 0x08 + 0x10000 * gpio;
 
 		groups[i].mux_bit = 2;
 		groups[i].pull_bit = 0;
@@ -100,6 +112,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev)
 		groups[i].intr_detection_width = 2;
 	}
 
+	devm_kfree(&pdev->dev, gpios);
+
 	qdf2xxx_pinctrl.pins = pins;
 	qdf2xxx_pinctrl.groups = groups;
 	qdf2xxx_pinctrl.npins = num_gpios;
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[Linus Walleij]

On Thu, Jun 22, 2017 at 10:54 PM, Timur Tabi <timur@codeaurora.org> wrote:

> On Qualcomm Technologies QDF2xxx platforms, only a subset of the GPIOs
> are actually available to the HLOS.  The others are blocked by the XPU,
> and any attempt to access them will cause an XPU violation that halts
> the system.

That's a bummer. Looking for Björn's review on this patch.

> Instead, the ACPI table now lists only specific GPIOs that are exposed
> externally as generic GPIO pins.

The ACPI table or the device tree property "gpios" right?

(But maybe this system is only using ACPI.)

> To maintain consistency, the GPIOs are
> enumerated 0 .. (N-1) as before, so that "gpio0" is really whatever is
> the first GPIO listed in ACPI.  The actual mapping is available via
> /sys/kernel/debug/gpio.

I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect
the available GPIOs.

If you haven't tested the GPIO tools, please try it out.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[Timur Tabi]

On 6/23/17 7:33 AM, Linus Walleij wrote:

>> Instead, the ACPI table now lists only specific GPIOs that are exposed
>> externally as generic GPIO pins.
> 
> The ACPI table or the device tree property "gpios" right?
> 
> (But maybe this system is only using ACPI.)

It's only ACPI.

> I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect
> the available GPIOs.
> 
> If you haven't tested the GPIO tools, please try it out.

I was not aware of these tools.  I will try them out, thanks.



-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[Bjorn Andersson]

On Fri 23 Jun 05:33 PDT 2017, Linus Walleij wrote:

> On Thu, Jun 22, 2017 at 10:54 PM, Timur Tabi <timur@codeaurora.org> wrote:
> 
> > On Qualcomm Technologies QDF2xxx platforms, only a subset of the GPIOs
> > are actually available to the HLOS.  The others are blocked by the XPU,
> > and any attempt to access them will cause an XPU violation that halts
> > the system.
> 
> That's a bummer. Looking for Björn's review on this patch.
> 

The TLMM block on this platform has N gpios, regardless of the XPU
configuration. The ACPI table provides the kernel with a list of
available "logical" GPIOs based on some system specification (or
reference design perhaps?).

I have not seen the Qualcomm server platforms, but I imagine that
customers can alter this list.


I think the numbering of the GPIOs should be that of the TLMM and the
"logical" names should be populated from ACPI by using gpio_chip->names.

The question left is how to represent the XPU locked gpios - a problem
we do share with in the mobile TLMM drivers.


It seems that if we extend the msm_pingroup with a flag to carry the XPU
lock information and then implement pinmux_ops->request() and deny any
requests on the locked pins, we should cover all[*] the normal GPIO code
paths.

[*] Except the reported case of gpiochip_add_data() looping over all
pins and calling get_direction() without first requesting the pin.

@Linus, do you see any flaws in this scheme?


PS. gpiochip_generic_request() ends up calling pinmux_ops->request()

Regards,
Bjorn

Re: [PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[Timur Tabi]

On 06/28/2017 02:12 PM, Bjorn Andersson wrote:
> On Fri 23 Jun 05:33 PDT 2017, Linus Walleij wrote:
> 
>> On Thu, Jun 22, 2017 at 10:54 PM, Timur Tabi <timur@codeaurora.org> wrote:
>>
>>> On Qualcomm Technologies QDF2xxx platforms, only a subset of the GPIOs
>>> are actually available to the HLOS.  The others are blocked by the XPU,
>>> and any attempt to access them will cause an XPU violation that halts
>>> the system.
>>
>> That's a bummer. Looking for Björn's review on this patch.
>>
> 
> The TLMM block on this platform has N gpios, regardless of the XPU
> configuration. The ACPI table provides the kernel with a list of
> available "logical" GPIOs based on some system specification (or
> reference design perhaps?).

It's the list of available physical GPIOs.  Here's the table:

Name (_DSD, Package ()
{
	ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
	Package ()
	{
		// Expose only the qdss_tracedata pins as GPIOs,
		// numbered sequentially, so that "gpio X" maps
		// to qdss_tracedata[X].  These pins are configured
		// as GPIOs (function 0) and wired externally
		// to a header on the board, so it's safe to
		// expose them to the HLOS driver.
		Package (2) {"gpios", Package ()
			{116, 117, 118, 119, 120, 121, 122, 123,
			 124, 125, 126, 127, 128, 129, 130, 131,
			 80, 81, 82, 83, 84, 85, 86, 87, 88, 89,
			 90, 50, 36, 37, 38, 39}}
	}
})

> I have not seen the Qualcomm server platforms, but I imagine that
> customers can alter this list.

Not easily, as it requires recompiling the ACPI tables.  I also don't 
know how much control customers have over the XPU settings.  The XPU is 
supposed to block access to any GPIOs that have been muxed to other 
purposes, but it's a hand-crafted list that could be anything.

> I think the numbering of the GPIOs should be that of the TLMM and the
> "logical" names should be populated from ACPI by using gpio_chip->names.

Is that not what my driver is doing?  Or do I misunderstand you?

> The question left is how to represent the XPU locked gpios - a problem
> we do share with in the mobile TLMM drivers.
> 
> 
> It seems that if we extend the msm_pingroup with a flag to carry the XPU
> lock information and then implement pinmux_ops->request() and deny any
> requests on the locked pins, we should cover all[*] the normal GPIO code
> paths.

Instead of a flag, I was thinking if npins == 0, then this GPIO should 
be disabled.

> [*] Except the reported case of gpiochip_add_data() looping over all
> pins and calling get_direction() without first requesting the pin.

This is what triggered the problem in the first place.  Every time the 
kernel boots, it's queries all of the GPIOs.  Ironically, I created that 
feature because on server platforms, that was the only way to know the 
current setting.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[Timur Tabi]

On 06/28/2017 02:12 PM, Bjorn Andersson wrote:
> It seems that if we extend the msm_pingroup with a flag to carry the XPU
> lock information and then implement pinmux_ops->request() and deny any
> requests on the locked pins, we should cover all[*] the normal GPIO code
> paths.

I managed to get this to work, so I'll post a patchset by tomorrow that 
implements this.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[Timur Tabi]

On 06/23/2017 07:33 AM, Linus Walleij wrote:
> I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect
> the available GPIOs.
> 
> If you haven't tested the GPIO tools, please try it out.

Unfortunately, this tool isn't very useful:

$ sudo ./lsgpio
GPIO chip: gpiochip0, "QCOM8001:00", 150 GPIO lines
         line  0: unnamed unused [output]
         line  1: unnamed unused [output]
         line  2: unnamed unused [output]
         line  3: unnamed unused [output]
         line  4: unnamed unused [output]
         line  5: unnamed unused [output]
         line  6: unnamed unused [output]
         line  7: unnamed unused [output]
         line  8: unnamed unused [output]
         line  9: unnamed unused [output]
         line 10: unnamed unused [output]
         line 11: unnamed unused [output]
         line 12: unnamed unused [output]
         line 13: unnamed unused [output]
         line 14: unnamed unused [output]
         line 15: unnamed unused [output]
         line 16: unnamed unused [output]
         line 17: unnamed unused [output]
         line 18: unnamed unused [output]
         line 19: unnamed unused [output]
         line 20: unnamed unused [output]
         line 21: unnamed unused [output]
         line 22: unnamed unused [output]
         line 23: unnamed unused [output]
         line 24: unnamed unused [output]
         line 25: unnamed unused [output]
         line 26: unnamed unused [output]
         line 27: unnamed unused [output]
         line 28: unnamed unused [output]
         line 29: unnamed unused [output]
         line 30: unnamed unused [output]
         line 31: unnamed unused [output]
         line 32: unnamed unused [output]
         line 33: unnamed unused [output]
         line 34: unnamed unused [output]
         line 35: unnamed unused [output]
         line 36: unnamed unused
         line 37: unnamed "sysfs" [kernel]
         line 38: unnamed unused
         line 39: unnamed unused
         line 40: unnamed unused [output]
	...

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[Bjorn Andersson]

On Thu 29 Jun 12:37 PDT 2017, Timur Tabi wrote:

> On 06/23/2017 07:33 AM, Linus Walleij wrote:
> > I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect
> > the available GPIOs.
> > 
> > If you haven't tested the GPIO tools, please try it out.
> 
> Unfortunately, this tool isn't very useful:
> 

I believe you have to come up with a separate array of names and attach
that to the gpio_chip in msm_gpio_init() for the _gpio_ pins to have
names. The names we specify in the pin driver is only the name of the
pinctrl pin...

Regards,
Bjorn

> $ sudo ./lsgpio
> GPIO chip: gpiochip0, "QCOM8001:00", 150 GPIO lines
>         line  0: unnamed unused [output]
>         line  1: unnamed unused [output]
>         line  2: unnamed unused [output]
>         line  3: unnamed unused [output]
>         line  4: unnamed unused [output]
>         line  5: unnamed unused [output]
>         line  6: unnamed unused [output]
>         line  7: unnamed unused [output]
>         line  8: unnamed unused [output]
>         line  9: unnamed unused [output]
>         line 10: unnamed unused [output]
>         line 11: unnamed unused [output]
>         line 12: unnamed unused [output]
>         line 13: unnamed unused [output]
>         line 14: unnamed unused [output]
>         line 15: unnamed unused [output]
>         line 16: unnamed unused [output]
>         line 17: unnamed unused [output]
>         line 18: unnamed unused [output]
>         line 19: unnamed unused [output]
>         line 20: unnamed unused [output]
>         line 21: unnamed unused [output]
>         line 22: unnamed unused [output]
>         line 23: unnamed unused [output]
>         line 24: unnamed unused [output]
>         line 25: unnamed unused [output]
>         line 26: unnamed unused [output]
>         line 27: unnamed unused [output]
>         line 28: unnamed unused [output]
>         line 29: unnamed unused [output]
>         line 30: unnamed unused [output]
>         line 31: unnamed unused [output]
>         line 32: unnamed unused [output]
>         line 33: unnamed unused [output]
>         line 34: unnamed unused [output]
>         line 35: unnamed unused [output]
>         line 36: unnamed unused
>         line 37: unnamed "sysfs" [kernel]
>         line 38: unnamed unused
>         line 39: unnamed unused
>         line 40: unnamed unused [output]
> 	...
> 
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] pinctrl: qcom: qdf2xxx: expose only some GPIO pins

[Linus Walleij]

On Thu, Jun 29, 2017 at 9:37 PM, Timur Tabi <timur@codeaurora.org> wrote:
> On 06/23/2017 07:33 AM, Linus Walleij wrote:
>>
>> I think using "lsgpio" fron tools/gpio/lsgpio.c is better to inspect
>> the available GPIOs.
>>
>> If you haven't tested the GPIO tools, please try it out.
>
>
> Unfortunately, this tool isn't very useful:
>
> $ sudo ./lsgpio
> GPIO chip: gpiochip0, "QCOM8001:00", 150 GPIO lines
>         line  0: unnamed unused [output]
>         line  1: unnamed unused [output]
>         line  2: unnamed unused [output]

They can be given reasonable names using the line-names
property in the DTS file, preferably the top-level file.
arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
is a good example.

Do the same with your target board and things will
be very much more helpful.

gpio-hammer can help you test a line (try it with
a LED) and gpio-event-mon can help you listen
at a key for example.

Yours,
Linus Walleij