Sparse GPIO maps with pinctrl-msm.c?

Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

I've run into a problem with our ACPI system where it turns out that
one a subset of GPIOs are actually available to Linux.  Attempting to
access anything that's not "approve" generates an XPU violation and
halts the system.

Our pin control driver, pinctrl-qdf2xxx.c, is a client on
pintrl-msm.c.  As such, it has to package the GPIO information in a
way that pinctrl-msm can use.  I just want to get confirmation that
there is no way to provide a list of specific GPIOs.

The actual list are these GPIOs: 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.  These correspond to
qdss_tracedata[0 - 31].  I can create these as gpio0 - gpio31, but
that doesn't work because then no one will know (without using
debug_fs) that "gpio3" is actually GPIO 119.

I can instead create all 150 GPIOs, and then specify NULL data for the
118 unavailable GPIOs, but then if anyone tries to access any of those
(e.g. "echo 7 > /sys/class/gpio/gexport") will case a violation.

Is there a way, in pinctrl-msm, to specify a GPIO that doesn't
actually exist, and therefore should never be exported?

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On Tue, Jun 13, 2017 at 6:35 PM, Timur Tabi <timur@codeaurora.org> wrote:
>
> Is there a way, in pinctrl-msm, to specify a GPIO that doesn't
> actually exist, and therefore should never be exported?

Follow-up question:

In pinctrl-msm, how do pin groups support multiple pins?

struct msm_pingroup {
    const char *name;
    const unsigned *pins;
    unsigned npins;

Here I can specify an array of pins.  However, I can't see from the
code how it would be possible to support more than one pin.  In fact,
all of the drivers that call msm_pinctrl_probe() always set 'npins' to
1 in every group.

Each group represents one GPIO in the TLMM.  All of the TLMM register
definitions are associated with a group, not a pin.  Frankly, it seems
that we should stop pretending that npins can be anything other than
1, and change all the code accordingly.

-- 
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: Sparse GPIO maps with pinctrl-msm.c?

[Stephen Boyd]

(Updating Andy's mail and adding Bjorn)

On 06/13, Timur Tabi wrote:
> I've run into a problem with our ACPI system where it turns out that
> one a subset of GPIOs are actually available to Linux.  Attempting to
> access anything that's not "approve" generates an XPU violation and
> halts the system.
> 
> Our pin control driver, pinctrl-qdf2xxx.c, is a client on
> pintrl-msm.c.  As such, it has to package the GPIO information in a
> way that pinctrl-msm can use.  I just want to get confirmation that
> there is no way to provide a list of specific GPIOs.
> 
> The actual list are these GPIOs: 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.  These correspond to
> qdss_tracedata[0 - 31].  I can create these as gpio0 - gpio31, but
> that doesn't work because then no one will know (without using
> debug_fs) that "gpio3" is actually GPIO 119.
> 
> I can instead create all 150 GPIOs, and then specify NULL data for the
> 118 unavailable GPIOs, but then if anyone tries to access any of those
> (e.g. "echo 7 > /sys/class/gpio/gexport") will case a violation.
> 
> Is there a way, in pinctrl-msm, to specify a GPIO that doesn't
> actually exist, and therefore should never be exported?
> 

I'm not aware of anything in pinctrl-msm to support this. Is this
really a problem though? The only user that could cause an XPU
violation would be root. So just "don't do that" and things will
work fine.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On 6/16/17 10:07 AM, Stephen Boyd wrote:
> I'm not aware of anything in pinctrl-msm to support this. 

It seems to me like the 'npins' field in msm_pingroup should be deleted, 
because it can only ever be 1.

> Is this
> really a problem though? The only user that could cause an XPU
> violation would be root. So just "don't do that" and things will
> work fine.

Unfortunately, thanks to 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/qcom?id=8e51533780ba223a3562ff4382c6b6f350c7e9a4 
we now read the direction of every pin at boot, and so we always get an 
XPU violation early in the boot process.

And even so, "don't do that" is just not acceptable on a server platform.

-- 
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: Sparse GPIO maps with pinctrl-msm.c?

[Stephen Boyd]

On 06/16, Timur Tabi wrote:
> On 6/16/17 10:07 AM, Stephen Boyd wrote:
> >I'm not aware of anything in pinctrl-msm to support this.
> 
> It seems to me like the 'npins' field in msm_pingroup should be
> deleted, because it can only ever be 1.

Ok. But does that change anything about this problem?

> 
> >Is this
> >really a problem though? The only user that could cause an XPU
> >violation would be root. So just "don't do that" and things will
> >work fine.
> 
> Unfortunately, thanks to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/qcom?id=8e51533780ba223a3562ff4382c6b6f350c7e9a4
> we now read the direction of every pin at boot, and so we always get
> an XPU violation early in the boot process.

Oops. Why not allow read to work, but write to fail? Can the XPU
be configured so it doesn't blow up when we read registers?

Otherwise, it sounds like some driver surgery is needed to
indicate that the gpio number space has plenty of holes in it and
that we should return failures for those protected pins.

We've already run into this problem on mobile platforms where
certain pins are locked down and the approach has been to not
care. But I don't think we have your patch yet, so you're the
first one to run into this problem.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On 6/16/17 10:41 AM, Stephen Boyd wrote:
> On 06/16, Timur Tabi wrote:
>> On 6/16/17 10:07 AM, Stephen Boyd wrote:
>>> I'm not aware of anything in pinctrl-msm to support this.
>>
>> It seems to me like the 'npins' field in msm_pingroup should be
>> deleted, because it can only ever be 1.
> 
> Ok. But does that change anything about this problem?

No, but at least no one would ever be fooled into thinking that you can 
have sparse GPIO maps when using pinctrl-msm.

>> Unfortunately, thanks to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/qcom?id=8e51533780ba223a3562ff4382c6b6f350c7e9a4
>> we now read the direction of every pin at boot, and so we always get
>> an XPU violation early in the boot process.
> 
> Oops. Why not allow read to work, but write to fail? Can the XPU
> be configured so it doesn't blow up when we read registers?

We debated that, and decided we really want to reject access to any 
GPIOs that are "off limits".  Our TLMM memory map was designed 
specifically to allow this to be enforced.

> Otherwise, it sounds like some driver surgery is needed to
> indicate that the gpio number space has plenty of holes in it and
> that we should return failures for those protected pins.

I was thinking about adding support for "npins == 0", but I don't know 
how to tell the pinctrl/gpio core that specific pins don't actually 
exist.  Is there a place where I can do something like this:

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

> We've already run into this problem on mobile platforms where
> certain pins are locked down and the approach has been to not
> care. But I don't think we have your patch yet, so you're the
> first one to run into this problem.

For now, I've decided that I'm just going to expose the qdss_tracedata[] 
pins as GPIOs, numbered 0 .. n-1.  However, there's no consensus on 
that, either.

Being able to designate specific pins as absent would make everyone happy.

-- 
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: Sparse GPIO maps with pinctrl-msm.c?

[Bjorn Andersson]

On Fri 16 Jun 08:49 PDT 2017, Timur Tabi wrote:

> On 6/16/17 10:41 AM, Stephen Boyd wrote:
> > On 06/16, Timur Tabi wrote:
> > > On 6/16/17 10:07 AM, Stephen Boyd wrote:
> > > > I'm not aware of anything in pinctrl-msm to support this.
> > > 
> > > It seems to me like the 'npins' field in msm_pingroup should be
> > > deleted, because it can only ever be 1.
> > 
> > Ok. But does that change anything about this problem?
> 
> No, but at least no one would ever be fooled into thinking that you can have
> sparse GPIO maps when using pinctrl-msm.
> 

As GPIOs are both identified by name and by index within the controller
I don't see it as sufficient to play games with just lowering
npins/ngpios and compacting the lists.

[..]
> > We've already run into this problem on mobile platforms where
> > certain pins are locked down and the approach has been to not
> > care. But I don't think we have your patch yet, so you're the
> > first one to run into this problem.
> 
> For now, I've decided that I'm just going to expose the qdss_tracedata[]
> pins as GPIOs, numbered 0 .. n-1.  However, there's no consensus on that,
> either.
> 

Exposing a subset of GPIOs with a different numbering than what's in the
hardware documentation is going to be quite confusing for the users.


Just to confirm, are the qdss_tracedata the only GPIOs that you want to
expose from the TLMM now? Are those consecutive?

> Being able to designate specific pins as absent would make everyone happy.
> 

I agree.

Regards,
Bjorn

Re: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On 6/16/17 11:06 AM, Bjorn Andersson wrote:
> Exposing a subset of GPIOs with a different numbering than what's in the
> hardware documentation is going to be quite confusing for the users.

I agree, and that's why I was hoping to expose only the qdss_tracedata pins.

> Just to confirm, are the qdss_tracedata the only GPIOs that you want to
> expose from the TLMM now? Are those consecutive?

Well, ideally would like to expose only those pins that are:

1) Approved by the XPU

and

2) Already set to function 0 on boot

Unfortunately, doing both of these would require significant changes to 
pinctrl-msm.  Considering how unimportant this feature is (we don't need 
the TLMM any more for normal operations), I don't really want to invest 
too much time in it.

So assuming I can get approval from all stakeholders, all I want to do 
is expose the qdss_tracedata[0..n-1] pins as gpios 0..n-1.

-- 
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: Sparse GPIO maps with pinctrl-msm.c?

[Andy Gross]

On 16 June 2017 at 12:17, Timur Tabi <timur@codeaurora.org> wrote:
> On 6/16/17 11:06 AM, Bjorn Andersson wrote:
>>
>> Exposing a subset of GPIOs with a different numbering than what's in the
>> hardware documentation is going to be quite confusing for the users.
>
>
> I agree, and that's why I was hoping to expose only the qdss_tracedata pins.
>
>> Just to confirm, are the qdss_tracedata the only GPIOs that you want to
>> expose from the TLMM now? Are those consecutive?
>
>
> Well, ideally would like to expose only those pins that are:
>
> 1) Approved by the XPU

How do you know what this is?  And this changes based on the TZ load.

> and
>
> 2) Already set to function 0 on boot
>
> Unfortunately, doing both of these would require significant changes to
> pinctrl-msm.  Considering how unimportant this feature is (we don't need the
> TLMM any more for normal operations), I don't really want to invest too much
> time in it.
>
> So assuming I can get approval from all stakeholders, all I want to do is
> expose the qdss_tracedata[0..n-1] pins as gpios 0..n-1.
>
>
> --
> 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: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On 6/16/17 11:21 AM, Andy Gross wrote:

>> 1) Approved by the XPU
> 
> How do you know what this is?  And this changes based on the TZ load.

An ACPI property in the TLMM node that lists the approved GPIOs by 
number.  It currently looks like this:

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 can be used as
		// generic GPIOs.
		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'm not crazy about it, but it's a compromise that allows some GPIOs to 
be exposed without a lot of coding.  One idea we're debating is 
forgetting about pinctrl-msm altogether and rewrite the driver from 
scratch as a pure GPIO driver.  I'm hoping to avoid having to do that.

-- 
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: Sparse GPIO maps with pinctrl-msm.c?

[Bjorn Andersson]

On Fri 16 Jun 09:26 PDT 2017, Timur Tabi wrote:

> On 6/16/17 11:21 AM, Andy Gross wrote:
> 
> > > 1) Approved by the XPU
> > 
> > How do you know what this is?  And this changes based on the TZ load.
> 
> An ACPI property in the TLMM node that lists the approved GPIOs by number.
> It currently looks like this:
> 
> 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 can be used as
> 		// generic GPIOs.

But what does this actually mean?

I assume that on this platform the qdss_tracedata is an alternative
pinmux function (configured by someone else). Which normally means that
the pins are routed to some internal hardware block.

Or are you just running these in gpio-function and then have some
software to decode the data? Where is this piece of software?

> 		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}}

Does these pins make up some sort of communication bus? Or is it 32
individual states? Does it really make sense to expose these as 32
"random" GPIOs - which on some platforms will be sequential in your
made-up GPIO controller and on others be scattered.

> 	}
> })
> 
> I'm not crazy about it, but it's a compromise that allows some GPIOs to be
> exposed without a lot of coding.  One idea we're debating is forgetting
> about pinctrl-msm altogether and rewrite the driver from scratch as a pure
> GPIO driver.  I'm hoping to avoid having to do that.
> 

I think that it would be nice to come up with a solution that works for
the other users of pinctrl-msm as well.

Regards,
Bjorn

Re: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On 06/16/2017 12:44 PM, Bjorn Andersson wrote:
>> An ACPI property in the TLMM node that lists the approved GPIOs by number.
>> It currently looks like this:
>>
>> 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 can be used as
>> 		// generic GPIOs.

> But what does this actually mean?

Well, the more I think about it, less it means.

> I assume that on this platform the qdss_tracedata is an alternative
> pinmux function (configured by someone else). Which normally means that
> the pins are routed to some internal hardware block.

Yes, I just realized my mistake.  The qdss_tracedata[] pins are 
alternative functions for the GPIO, so calling them qdss_tracedata GPIOs 
is wrong.  They just happen to be the pins that can be used as 
qdss_tracedata, but we're expecting them to be configured as GPIOs 
(function 0) instead.

Ugh.

> Or are you just running these in gpio-function and then have some
> software to decode the data? Where is this piece of software?

I have another patch to pinctrl-qdf2xxx.c that reads the "gpios" 
property and maps the gpios to gpio0..gpioN.  Basically, the driver 
first does this:

u32 *gpios;
ret = device_property_read_u32_array(&pdev->dev, "gpios", gpios,
				     num_gpios);

so gpios[0] is 116 and gpios[31] is 39.

And then in a loop:

	for (i = 0; i < num_gpios; i++) {
		unsigned int gpio = gpios[i];

		snprintf(names[i], NAME_SIZE, "qdss_tracedata[%u]", i);

		pins[i].number = i;
		pins[i].name = names[i];

		groups[i].npins = 1;
		groups[i].name = names[i];
		groups[i].pins = &pins[i].number;

		groups[i].ctl_reg = 0x10000 * gpio;  <-----

>> 		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}}
> Does these pins make up some sort of communication bus? Or is it 32
> individual states? Does it really make sense to expose these as 32
> "random" GPIOs - which on some platforms will be sequential in your
> made-up GPIO controller and on others be scattered.

Well, these are 32 pins that can be used as qdss_tracedata, and so are 
considered "open" by some arbitrary standard that doesn't seem very stable.

The idea is that pin 116 is qdss_tracedata[0], which I expose as gpio[0].

> I think that it would be nice to come up with a solution that works for
> the other users of pinctrl-msm as well.

I agree. It's hard for me to wrap my head around it, though, because the 
whole groups vs pins things keeps confusing me.  The driver pretends 
that you can have more than one pin per group, but that's just not true, 
and instead it only works if each group represents a single TLMM block.

-- 
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: Sparse GPIO maps with pinctrl-msm.c?

[Bjorn Andersson]

On Fri 16 Jun 11:10 PDT 2017, Timur Tabi wrote:

> On 06/16/2017 12:44 PM, Bjorn Andersson wrote:
> > > An ACPI property in the TLMM node that lists the approved GPIOs by number.
> > > It currently looks like this:
> > > 
> > > 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 can be used as
> > > 		// generic GPIOs.
> 
> > But what does this actually mean?
> 
> Well, the more I think about it, less it means.
> 
> > I assume that on this platform the qdss_tracedata is an alternative
> > pinmux function (configured by someone else). Which normally means that
> > the pins are routed to some internal hardware block.
> 
> Yes, I just realized my mistake.  The qdss_tracedata[] pins are alternative
> functions for the GPIO, so calling them qdss_tracedata GPIOs is wrong.  They
> just happen to be the pins that can be used as qdss_tracedata, but we're
> expecting them to be configured as GPIOs (function 0) instead.
> 
> Ugh.
> 
> > Or are you just running these in gpio-function and then have some
> > software to decode the data? Where is this piece of software?
> 
> I have another patch to pinctrl-qdf2xxx.c that reads the "gpios" property
> and maps the gpios to gpio0..gpioN.  Basically, the driver first does this:
> 
> u32 *gpios;
> ret = device_property_read_u32_array(&pdev->dev, "gpios", gpios,
> 				     num_gpios);
> 
> so gpios[0] is 116 and gpios[31] is 39.
> 
> And then in a loop:
> 
> 	for (i = 0; i < num_gpios; i++) {
> 		unsigned int gpio = gpios[i];
> 
> 		snprintf(names[i], NAME_SIZE, "qdss_tracedata[%u]", i);
> 
> 		pins[i].number = i;
> 		pins[i].name = names[i];
> 
> 		groups[i].npins = 1;
> 		groups[i].name = names[i];
> 		groups[i].pins = &pins[i].number;
> 
> 		groups[i].ctl_reg = 0x10000 * gpio;  <-----
> 
> > > 		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}}
> > Does these pins make up some sort of communication bus? Or is it 32
> > individual states? Does it really make sense to expose these as 32
> > "random" GPIOs - which on some platforms will be sequential in your
> > made-up GPIO controller and on others be scattered.
> 
> Well, these are 32 pins that can be used as qdss_tracedata, and so are
> considered "open" by some arbitrary standard that doesn't seem very stable.
> 
> The idea is that pin 116 is qdss_tracedata[0], which I expose as gpio[0].
> 

So what you're saying is that it's decided that you're not going to use
"qdss_tracedata" and in some document it's described that these 32 TLMM
pins are available for customers to utilize as GPIOs?

If so I think the GPIOs should still be numbered based on their
numbering in the datasheet (i.e. gpio116), but that you should be using
"line-names" to define the logical naming of these 32 gpios based on
your customer documentation.

> > I think that it would be nice to come up with a solution that works for
> > the other users of pinctrl-msm as well.
> 
> I agree. It's hard for me to wrap my head around it, though, because the
> whole groups vs pins things keeps confusing me.  The driver pretends that
> you can have more than one pin per group, but that's just not true, and
> instead it only works if each group represents a single TLMM block.
> 

A pin represents a pad on the chip and a group represents a
"configurable entity" in TLMM.

For GPIOs this doesn't make a difference, but rather than naming the
pins "sdc2_data" there should be pins named "SDC2_DATA_0" ...
"SDC2_DATA_3". But the configurable entity is "sdc2_data", so that's
what should go in the "group".

According to the pinctrl documentation we should actually have called
the pins of the sdc2_data group "P3", "R6", "T7" and "P7" (for
APQ8016E). If nothing else this would probably have made things less
confusing.


That said, I never tested that this works...

Regards,
Bjorn

Re: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On 06/16/2017 01:50 PM, Bjorn Andersson wrote:
> So what you're saying is that it's decided that you're not going to use
> "qdss_tracedata" and in some document it's described that these 32 TLMM
> pins are available for customers to utilize as GPIOs?

Well, nothing is "decided" just yet, but that is the idea.

> If so I think the GPIOs should still be numbered based on their
> numbering in the datasheet (i.e. gpio116), but that you should be using
> "line-names" to define the logical naming of these 32 gpios based on
> your customer documentation.

That's what I was hoping on doing, but that requires a sparse GPIO map. 
gpio116 is valid, but gpio1 is not.  Any attempt to access that gpio 
causes an XPU violation.

>>> I think that it would be nice to come up with a solution that works for
>>> the other users of pinctrl-msm as well.
>> I agree. It's hard for me to wrap my head around it, though, because the
>> whole groups vs pins things keeps confusing me.  The driver pretends that
>> you can have more than one pin per group, but that's just not true, and
>> instead it only works if each group represents a single TLMM block.
>>
> A pin represents a pad on the chip and a group represents a
> "configurable entity" in TLMM.

Fair enough, but each TLMM entry only maps to 0 or 1 pins.

> For GPIOs this doesn't make a difference, but rather than naming the
> pins "sdc2_data" there should be pins named "SDC2_DATA_0" ...
> "SDC2_DATA_3". But the configurable entity is "sdc2_data", so that's
> what should go in the "group".

I don't understand the SDC_PINGROUP() macro.  Most of the values are set 
to -1:

		.intr_cfg_reg = 0,			\
		.intr_status_reg = 0,			\
		.intr_target_reg = 0,			\
		.mux_bit = -1,				\
		.pull_bit = pull,			\
		.drv_bit = drv,				\
		.oe_bit = -1,				\
		.in_bit = -1,				\
		.out_bit = -1,				\

I'm not familiar with that SOC, but this looks to me like it's a 
non-TLMM GPIO. In that case, what's the point of including it?  What 
does this actually do?  It's a "configurable entity", but there doesn't 
appear any way to configure it.

> According to the pinctrl documentation we should actually have called
> the pins of the sdc2_data group "P3", "R6", "T7" and "P7" (for
> APQ8016E). If nothing else this would probably have made things less
> confusing.

Not for me.

-- 
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: Sparse GPIO maps with pinctrl-msm.c?

[Bjorn Andersson]

On Fri 16 Jun 12:07 PDT 2017, Timur Tabi wrote:
> On 06/16/2017 01:50 PM, Bjorn Andersson wrote:
[..]
> > For GPIOs this doesn't make a difference, but rather than naming the
> > pins "sdc2_data" there should be pins named "SDC2_DATA_0" ...
> > "SDC2_DATA_3". But the configurable entity is "sdc2_data", so that's
> > what should go in the "group".
> 
> I don't understand the SDC_PINGROUP() macro.  Most of the values are set to
> -1:
> 
> 		.intr_cfg_reg = 0,			\
> 		.intr_status_reg = 0,			\
> 		.intr_target_reg = 0,			\
> 		.mux_bit = -1,				\
> 		.pull_bit = pull,			\
> 		.drv_bit = drv,				\
> 		.oe_bit = -1,				\
> 		.in_bit = -1,				\
> 		.out_bit = -1,				\
> 

In some cases SDC function can be found muxed among GPIOs, in others
there are dedicated configuration in TLMM for SDC properties. This entry
represent the latter.

You can set the bias and drive-strength, but there's no mux
configuration, no enable/disable, no interrupt and no value.

> I'm not familiar with that SOC, but this looks to me like it's a non-TLMM
> GPIO.

It's a non-GPIO configuration in TLMM.

> In that case, what's the point of including it?  What does this
> actually do?  It's a "configurable entity", but there doesn't appear
> any way to configure it.
> 

It's common to find at least the primary SDC on dedicated pins and hence
separate configuration. But as it's not available for generic IO
operations the properties/operations typical for this are not available.

> > According to the pinctrl documentation we should actually have called
> > the pins of the sdc2_data group "P3", "R6", "T7" and "P7" (for
> > APQ8016E). If nothing else this would probably have made things less
> > confusing.
> 
> Not for me.
> 

:)

Regards,
Bjorn

Re: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On 06/16/2017 12:44 PM, Bjorn Andersson wrote:
> I think that it would be nice to come up with a solution that works for
> the other users of pinctrl-msm as well.

I need help making that work.  I'm at my wits' end trying to figure it out.

This is what I currently have:

@@ -835,11 +836,14 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
                 return ret;
         }

-       ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 
0, 0, c
-       if (ret) {
-               dev_err(pctrl->dev, "Failed to add pin range\n");
-               gpiochip_remove(&pctrl->chip);
-               return ret;
+       for (i = 0; i < ngpio; i++) {
+               ret = gpiochip_add_pin_range(&pctrl->chip, 
dev_name(pctrl->dev),
+                                            pctrl->soc->groups[i].pins[0],
+ 
pctrl->soc->groups[i].pins[0], 1);
+               if (ret) {
+                       dev_err(pctrl->dev, "Failed to add pin range\n");
+                       gpiochip_remove(&pctrl->chip);
+                       return ret;
+               }

When I run it, I get this out

gpio gpiochip0: (QCOM8001:00): created GPIO range 116->116 ==> 
QCOM8001:00 PIN 116->116
gpio gpiochip0: (QCOM8001:00): created GPIO range 117->117 ==> 
QCOM8001:00 PIN 117->117
gpio gpiochip0: (QCOM8001:00): created GPIO range 118->118 ==> 
QCOM8001:00 PIN 118->118
gpio gpiochip0: (QCOM8001:00): created GPIO range 119->119 ==> 
QCOM8001:00 PIN 119->119
gpio gpiochip0: (QCOM8001:00): created GPIO range 120->120 ==> 
QCOM8001:00 PIN 120->120
gpio gpiochip0: (QCOM8001:00): created GPIO range 121->121 ==> 
QCOM8001:00 PIN 121->121
gpio gpiochip0: (QCOM8001:00): created GPIO range 122->122 ==> 
QCOM8001:00 PIN 122->122
gpio gpiochip0: (QCOM8001:00): created GPIO range 123->123 ==> 
QCOM8001:00 PIN 123->123
gpio gpiochip0: (QCOM8001:00): created GPIO range 124->124 ==> 
QCOM8001:00 PIN 124->124
gpio gpiochip0: (QCOM8001:00): created GPIO range 125->125 ==> 
QCOM8001:00 PIN 125->125
gpio gpiochip0: (QCOM8001:00): created GPIO range 126->126 ==> 
QCOM8001:00 PIN 126->126
gpio gpiochip0: (QCOM8001:00): created GPIO range 127->127 ==> 
QCOM8001:00 PIN 127->127
gpio gpiochip0: (QCOM8001:00): created GPIO range 128->128 ==> 
QCOM8001:00 PIN 128->128
gpio gpiochip0: (QCOM8001:00): created GPIO range 129->129 ==> 
QCOM8001:00 PIN 129->129
gpio gpiochip0: (QCOM8001:00): created GPIO range 130->130 ==> 
QCOM8001:00 PIN 130->130
gpio gpiochip0: (QCOM8001:00): created GPIO range 131->131 ==> 
QCOM8001:00 PIN 131->131
gpio gpiochip0: (QCOM8001:00): created GPIO range 80->80 ==> QCOM8001:00 
PIN 80->80
gpio gpiochip0: (QCOM8001:00): created GPIO range 81->81 ==> QCOM8001:00 
PIN 81->81
gpio gpiochip0: (QCOM8001:00): created GPIO range 82->82 ==> QCOM8001:00 
PIN 82->82
gpio gpiochip0: (QCOM8001:00): created GPIO range 83->83 ==> QCOM8001:00 
PIN 83->83
gpio gpiochip0: (QCOM8001:00): created GPIO range 84->84 ==> QCOM8001:00 
PIN 84->84
gpio gpiochip0: (QCOM8001:00): created GPIO range 85->85 ==> QCOM8001:00 
PIN 85->85
gpio gpiochip0: (QCOM8001:00): created GPIO range 86->86 ==> QCOM8001:00 
PIN 86->86
gpio gpiochip0: (QCOM8001:00): created GPIO range 87->87 ==> QCOM8001:00 
PIN 87->87
gpio gpiochip0: (QCOM8001:00): created GPIO range 88->88 ==> QCOM8001:00 
PIN 88->88
gpio gpiochip0: (QCOM8001:00): created GPIO range 89->89 ==> QCOM8001:00 
PIN 89->89
gpio gpiochip0: (QCOM8001:00): created GPIO range 90->90 ==> QCOM8001:00 
PIN 90->90
gpio gpiochip0: (QCOM8001:00): created GPIO range 50->50 ==> QCOM8001:00 
PIN 50->50
gpio gpiochip0: (QCOM8001:00): created GPIO range 36->36 ==> QCOM8001:00 
PIN 36->36
gpio gpiochip0: (QCOM8001:00): created GPIO range 37->37 ==> QCOM8001:00 
PIN 37->37
gpio gpiochip0: (QCOM8001:00): created GPIO range 38->38 ==> QCOM8001:00 
PIN 38->38
gpio gpiochip0: (QCOM8001:00): created GPIO range 39->39 ==> QCOM8001:00 
PIN 39->39


Which looks promising, except that I cannot export any GPIOs:


root@ubuntu:~# cd /sys/class/gpio/
root@ubuntu:/sys/class/gpio# echo 0 > export
-su: echo: write error: No such device
root@ubuntu:/sys/class/gpio# echo 37 > export
[  229.639992] export_store: invalid GPIO 37
-su: echo: write error: Invalid argument
root@ubuntu:/sys/class/gpio# echo 1 > export
-su: echo: write error: No such device

I was really hoping that "echo 37 > export" would work, but it doesn't.

And then I realized.  Even if I could get this to work, the only way the 
user would know GPIOs are actually exported is via /sys/kernel/debug/gpio:

# cat gpio
gpiochip0: GPIOs 0-31, parent: platform/QCOM8001:00, QCOM8001:00:
  gpio[116]: in  0 2mA pull down
  gpio[117]: in  0 2mA pull down
  gpio[118]: in  0 2mA pull down
  gpio[119]: in  0 2mA pull down
  gpio[120]: in  0 2mA pull down
  gpio[121]: in  0 2mA pull down
  gpio[122]: in  0 2mA pull down
  gpio[123]: in  0 2mA pull down
  gpio[124]: in  0 2mA pull down
  gpio[125]: in  0 2mA pull down
  gpio[126]: in  0 2mA pull down
  gpio[127]: in  0 2mA pull down
  gpio[128]: in  0 2mA pull down
  gpio[129]: in  0 2mA pull down
  gpio[130]: in  0 2mA pull down
  gpio[131]: in  0 2mA pull down
  gpio[80]: in  0 2mA pull down
  gpio[81]: in  0 2mA pull down
  gpio[82]: in  0 2mA pull down
  gpio[83]: in  0 2mA pull down
  gpio[84]: in  0 2mA pull down
  gpio[85]: in  0 2mA pull down
  gpio[86]: in  0 2mA pull down
  gpio[87]: in  0 2mA pull down
  gpio[88]: in  0 2mA pull down
  gpio[89]: in  0 2mA pull down
  gpio[90]: in  0 2mA pull down
  gpio[50]: in  0 2mA pull down
  gpio[36]: in  0 2mA pull down
  gpio[37]: in  0 2mA pull down
  gpio[38]: in  0 2mA pull down
  gpio[39]: in  0 2mA pull down

I'm not sure it's wise to depend on debugfs to know which GPIOs actually 
exist.  I just don't see a good solution here.  My initial solution 
would be to create 32 GPIOs, numbered 0-13, but have 'gpio0" map to 
"gpio 116", and so on.  It would be confusing at but, and you would 
still depend on debugfs to figure out what "gpio0" really is, but at 
least it would work.

-- 
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: Sparse GPIO maps with pinctrl-msm.c?

[Bjorn Andersson]

On Fri 16 Jun 08:15 PDT 2017, Timur Tabi wrote:

> On 6/16/17 10:07 AM, Stephen Boyd wrote:
> > I'm not aware of anything in pinctrl-msm to support this.
> 
> It seems to me like the 'npins' field in msm_pingroup should be deleted,
> because it can only ever be 1.
> 

npins are the number of "pins" handles by the TLMM, while ngpios are the
number of GPIO lines. I.e. npins >= ngpios and non platforms where we
control e.g. sdc properties you can see that npins > ngpios.

> > Is this
> > really a problem though? The only user that could cause an XPU
> > violation would be root. So just "don't do that" and things will
> > work fine.
> 
> Unfortunately, thanks to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/qcom?id=8e51533780ba223a3562ff4382c6b6f350c7e9a4
> we now read the direction of every pin at boot, and so we always get an XPU
> violation early in the boot process.
> 
> And even so, "don't do that" is just not acceptable on a server platform.
> 

It's not an awesome solution for mobile either. But to solve this we
have two problems to solve;

1) as the XPU configuration isn't fixed we need to be dynamic or
configurable in some sensible way

2) the pinctrl framework does have some support for sparse pin spaces,
but this would need to be extended to allow us to (easily) register a
sparse list of pins

Regards,
Bjorn

Re: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On 6/16/17 10:55 AM, Bjorn Andersson wrote:
> npins are the number of "pins" handles by the TLMM, while ngpios are the
> number of GPIO lines. I.e. npins >= ngpios and non platforms where we
> control e.g. sdc properties you can see that npins > ngpios.

I'm talking about the 'npins' in struct msm_pingroup:

struct msm_pingroup {
	const char *name;
	const unsigned *pins;
	unsigned npins;

Every client driver of pinctrl-msm sets this value to 1 for every group.

> It's not an awesome solution for mobile either. But to solve this we
> have two problems to solve;
> 
> 1) as the XPU configuration isn't fixed we need to be dynamic or
> configurable in some sensible way

I was planning on updating the TLMM ACPI node to include a property that 
lists the acceptable GPIOs.

> 2) the pinctrl framework does have some support for sparse pin spaces,
> but this would need to be extended to allow us to (easily) register a
> sparse list of pins

I was hoping there would be a way in pinctrl-msm to tell the framework, 
"Oh, you want to export this pin? Sorry, I forgot to tell you that it 
doesn't exist."

-- 
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: Sparse GPIO maps with pinctrl-msm.c?

[Bjorn Andersson]

On Fri 16 Jun 09:07 PDT 2017, Timur Tabi wrote:

> On 6/16/17 10:55 AM, Bjorn Andersson wrote:
> > npins are the number of "pins" handles by the TLMM, while ngpios are the
> > number of GPIO lines. I.e. npins >= ngpios and non platforms where we
> > control e.g. sdc properties you can see that npins > ngpios.
> 
> I'm talking about the 'npins' in struct msm_pingroup:
> 
> struct msm_pingroup {
> 	const char *name;
> 	const unsigned *pins;
> 	unsigned npins;
> 
> Every client driver of pinctrl-msm sets this value to 1 for every group.
> 

The npins here would allow us to properly name the multiple pins in e.g.
sdcX_data. But we're not doing this and I would be surprised if anyone
found it useful to get this in.

So it make sense to hardcode this value in msm_get_group_pins() and drop
it from the struct.

> > It's not an awesome solution for mobile either. But to solve this we
> > have two problems to solve;
> > 
> > 1) as the XPU configuration isn't fixed we need to be dynamic or
> > configurable in some sensible way
> 
> I was planning on updating the TLMM ACPI node to include a property that
> lists the acceptable GPIOs.
> 

But this list is related to your XPU configuration and not the TLMM
block, so the list of enabled/disabled pins should not go in the driver.

> > 2) the pinctrl framework does have some support for sparse pin spaces,
> > but this would need to be extended to allow us to (easily) register a
> > sparse list of pins
> 
> I was hoping there would be a way in pinctrl-msm to tell the framework, "Oh,
> you want to export this pin? Sorry, I forgot to tell you that it doesn't
> exist."
> 

There are quite a few functions in pinctrl-msm that would need to be
augmented with such checks, so I don't like this approach.

Regards,
Bjorn

Re: Sparse GPIO maps with pinctrl-msm.c?

[Timur Tabi]

On 06/16/2017 11:35 AM, Bjorn Andersson wrote:

> The npins here would allow us to properly name the multiple pins in e.g.
> sdcX_data. But we're not doing this and I would be surprised if anyone
> found it useful to get this in.

But how would that work?  Each group is a single TLMM block, which only 
has one bit for I/O.  You can't actually program more than one pin from 
a group.

struct msm_pingroup {
...
	unsigned in_bit:5;
	unsigned out_bit:5;

This only lets you define one bit for input, and one bit for output, for 
any group.

> So it make sense to hardcode this value in msm_get_group_pins() and drop
> it from the struct.

I might submit a patch for that later.

> 
>>> It's not an awesome solution for mobile either. But to solve this we
>>> have two problems to solve;
>>>
>>> 1) as the XPU configuration isn't fixed we need to be dynamic or
>>> configurable in some sensible way
>>
>> I was planning on updating the TLMM ACPI node to include a property that
>> lists the acceptable GPIOs.
>>
> 
> But this list is related to your XPU configuration and not the TLMM
> block, so the list of enabled/disabled pins should not go in the driver.

It goes in the ACPI table, and the driver reads that, and then (ideally) 
creates an array of msm_pingroup objects that reflects the list in ACPI.

I'm hoping there's a way to modify msm_gpio_init() so that it parses the 
array, and looks for "unavilable" groups.  For example, if npins == 0, 
that would be one way to know that this group doesn't actually exist.

For example, instead of calling gpiochip_add_pin_range(), maybe it would 
call gpiochip_add_pingroup_range() instead.  Unfortunately, I just don't 
understand the gpiochip functions well enough to know what to do.  I 
need help in this area.

-- 
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.