gpio: tegra186: Add support for T186 GPIO

Add GPIO driver for T186 based platforms.
Adds support for MAIN and AON GPIO's from T186.

Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
---
 drivers/gpio/Kconfig         |   8 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-tegra186.c | 647 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 656 insertions(+)
 create mode 100644 drivers/gpio/gpio-tegra186.c

On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:

> Add GPIO driver for T186 based platforms.
> Adds support for MAIN and AON GPIO's from T186.
>
> Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>

Stephen/Thierry/Alexandre:
Can I get your review on this Tegra thing?

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

On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
> 
> > Add GPIO driver for T186 based platforms.
> > Adds support for MAIN and AON GPIO's from T186.
> >
> > Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
> 
> Stephen/Thierry/Alexandre:
> Can I get your review on this Tegra thing?

Can we hold off on this for a bit? I've got my own implementation of
this in my Tegra186 tree because I thought nobody else was working on
it. From a brief look they seem mostly similar but there are a couple
of differences that I need to take a closer look at (and do some more
intensive testing on).

Thanks,
Thierry

On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
>> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
>>
>> > Add GPIO driver for T186 based platforms.
>> > Adds support for MAIN and AON GPIO's from T186.
>> >
>> > Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
>>
>> Stephen/Thierry/Alexandre:
>> Can I get your review on this Tegra thing?
>
> Can we hold off on this for a bit? I've got my own implementation of
> this in my Tegra186 tree because I thought nobody else was working on
> it. From a brief look they seem mostly similar but there are a couple
> of differences that I need to take a closer look at (and do some more
> intensive testing on).

Be careful about discouraging other developers internally from
participating upstream, Thierry.

Please don't become a bottleneck for your company's contributions.
Rewriting stuff on your own isn't a scalable model.


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

On Mon, Nov 07, 2016 at 05:42:33PM -0800, Olof Johansson wrote:
> On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
> >> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
> >>
> >> > Add GPIO driver for T186 based platforms.
> >> > Adds support for MAIN and AON GPIO's from T186.
> >> >
> >> > Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
> >>
> >> Stephen/Thierry/Alexandre:
> >> Can I get your review on this Tegra thing?
> >
> > Can we hold off on this for a bit? I've got my own implementation of
> > this in my Tegra186 tree because I thought nobody else was working on
> > it. From a brief look they seem mostly similar but there are a couple
> > of differences that I need to take a closer look at (and do some more
> > intensive testing on).
> 
> Be careful about discouraging other developers internally from
> participating upstream, Thierry.

That was certainly not my intention. I do welcome any contributions,
internal or external.

> Please don't become a bottleneck for your company's contributions.
> Rewriting stuff on your own isn't a scalable model.

Like I said, I didn't rewrite this, I merely wrote the GPIO driver
because there wasn't one at the time and I wasn't aware of anyone else
working on one. Waiting for a GPIO driver to emerge would've prevented
me from making progress on other fronts.

Thierry

On 11/08/2016 08:55 AM, Thierry Reding wrote:
> On Mon, Nov 07, 2016 at 05:42:33PM -0800, Olof Johansson wrote:
>> On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
>>> On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
>>>> On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
>>>>
>>>>> Add GPIO driver for T186 based platforms.
>>>>> Adds support for MAIN and AON GPIO's from T186.
>>>>>
>>>>> Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
>>>>
>>>> Stephen/Thierry/Alexandre:
>>>> Can I get your review on this Tegra thing?
>>>
>>> Can we hold off on this for a bit? I've got my own implementation of
>>> this in my Tegra186 tree because I thought nobody else was working on
>>> it. From a brief look they seem mostly similar but there are a couple
>>> of differences that I need to take a closer look at (and do some more
>>> intensive testing on).
>>
>> Be careful about discouraging other developers internally from
>> participating upstream, Thierry.
>
> That was certainly not my intention. I do welcome any contributions,
> internal or external.
>
>> Please don't become a bottleneck for your company's contributions.
>> Rewriting stuff on your own isn't a scalable model.
>
> Like I said, I didn't rewrite this, I merely wrote the GPIO driver
> because there wasn't one at the time and I wasn't aware of anyone else
> working on one. Waiting for a GPIO driver to emerge would've prevented
> me from making progress on other fronts.

One issue here is that there are lots of patches destined for upstream 
in your own personal tree, but they aren't actually upstream yet. I 
think pushing more of your work into linux-next (and further upstream) 
faster would help people out. linux-next and other "standard" repos 
should be easily discoverable by anyway following any upstreaming 
HOWTOs, but those HOWTOs aren't going to mention your personal trees, so 
patches there effectively don't exist. Making the already extant work 
more discoverable will help prevent people duplicating work.

Related to this, I've been waiting rather a while for the Tegra186 DT 
binding patches I sent to be applied. I'd love to see them go in ASAP 
even if there's no kernel driver behind them. The bindings have been 
reviewed, ack'd, and they're in use in U-Boot already.

A thought on Tegra186: For a older supported SoCs, we obviously don't 
want to push changes upstream that aren't too well baked. However, for a 
new SoC that doesn't work yet, I'm tend to prioritize getting as much 
early work upstream as fast as possible (to try and unblock people 
working in other areas) over getting those patches perfect first. 
Release early, release often will help unblock people and parallelize 
work. Pipeline your own work rather than batching it all up to release 
at once.

P.S. don't take this email too personally or anything; I'm not trying to 
be complaining/critical or anything like that. It's just a few mild 
thoughts.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Nov 08, 2016 at 09:49:27AM -0700, Stephen Warren wrote:
> On 11/08/2016 08:55 AM, Thierry Reding wrote:
> > On Mon, Nov 07, 2016 at 05:42:33PM -0800, Olof Johansson wrote:
> > > On Mon, Nov 7, 2016 at 5:21 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > On Mon, Nov 07, 2016 at 08:53:37AM +0100, Linus Walleij wrote:
> > > > > On Wed, Nov 2, 2016 at 11:48 AM, Suresh Mangipudi <smangipudi@nvidia.com> wrote:
> > > > > 
> > > > > > Add GPIO driver for T186 based platforms.
> > > > > > Adds support for MAIN and AON GPIO's from T186.
> > > > > > 
> > > > > > Signed-off-by: Suresh Mangipudi <smangipudi@nvidia.com>
> > > > > 
> > > > > Stephen/Thierry/Alexandre:
> > > > > Can I get your review on this Tegra thing?
> > > > 
> > > > Can we hold off on this for a bit? I've got my own implementation of
> > > > this in my Tegra186 tree because I thought nobody else was working on
> > > > it. From a brief look they seem mostly similar but there are a couple
> > > > of differences that I need to take a closer look at (and do some more
> > > > intensive testing on).
> > > 
> > > Be careful about discouraging other developers internally from
> > > participating upstream, Thierry.
> > 
> > That was certainly not my intention. I do welcome any contributions,
> > internal or external.
> > 
> > > Please don't become a bottleneck for your company's contributions.
> > > Rewriting stuff on your own isn't a scalable model.
> > 
> > Like I said, I didn't rewrite this, I merely wrote the GPIO driver
> > because there wasn't one at the time and I wasn't aware of anyone else
> > working on one. Waiting for a GPIO driver to emerge would've prevented
> > me from making progress on other fronts.
> 
> One issue here is that there are lots of patches destined for upstream in
> your own personal tree, but they aren't actually upstream yet. I think
> pushing more of your work into linux-next (and further upstream) faster
> would help people out. linux-next and other "standard" repos should be
> easily discoverable by anyway following any upstreaming HOWTOs, but those
> HOWTOs aren't going to mention your personal trees, so patches there
> effectively don't exist. Making the already extant work more discoverable
> will help prevent people duplicating work.

I had assumed that I had properly communicated what the canonical
temporary location for Tegra186 patches would be, but you're right that
it's probably easy to miss, and linux-next would be a more obvious
target.

> Related to this, I've been waiting rather a while for the Tegra186 DT
> binding patches I sent to be applied. I'd love to see them go in ASAP even
> if there's no kernel driver behind them. The bindings have been reviewed,
> ack'd, and they're in use in U-Boot already.

It's true, I've been promising this for weeks now. I'll get around to it
within the week. Please do prod me in case I don't. I promise I won't
get mad =)

> A thought on Tegra186: For a older supported SoCs, we obviously don't want
> to push changes upstream that aren't too well baked. However, for a new SoC
> that doesn't work yet, I'm tend to prioritize getting as much early work
> upstream as fast as possible (to try and unblock people working in other
> areas) over getting those patches perfect first. Release early, release
> often will help unblock people and parallelize work. Pipeline your own work
> rather than batching it all up to release at once.

I'm always hesitant to merge code that isn't functional or tested, but
perhaps I'm being a little overzealous here, and I'm evidently not doing
so great, so let me try to be more aggressive.

> P.S. don't take this email too personally or anything; I'm not trying to be
> complaining/critical or anything like that. It's just a few mild thoughts.

No offense taken, thanks for being constructive about it.

Thierry

On 11/02/2016 04:48 AM, Suresh Mangipudi wrote:
> Add GPIO driver for T186 based platforms.
> Adds support for MAIN and AON GPIO's from T186.

I'm not sure how you/Thierry will approach merging this with the other 
GPIO driver he has, but here's a very quick review of this one in case 
it's useful.

> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c

> +#define TEGRA_MAIN_GPIO_PORT_INFO(port, cid, cind, npins)	\

A comment indicating what "cid" and "cind" mean (and perhaps the other 
parameters too) would be useful.

A brief description of the overall register layout and structure and 
differences between the MAIN/AON controllers would be useful.

> +[TEGRA_MAIN_GPIO_PORT_##port] = {				\
> +		.port_name = #port,				\
> +		.cont_id = cid,					\
> +		.port_index = cind,				\

Why not make the parameter names match the field names they're assigned to?

> +		.valid_pins = npins,				\
> +		.scr_offset = cid * 0x1000 + cind * 0x40,	\
> +		.reg_offset = cid * 0x1000 + cind * 0x200,	\

While C does define operator precedence rules that make that expression 
OK, I personally prefer using () to make it explict:

+		.reg_offset = (cid * 0x1000) + (cind * 0x200),	\

That way, the reader doesn't have to think/remember so much.

Also, if these values can be calculated based on .cont_id and 
.port_index, I wonder why we need to pre-calculate them here and/or what 
else could be pre-calculated from .cont_id/.port_index? I'm tend to 
either (a) just store .cont_id and .port_index and calculate everything 
from them always, or (b) store just derived data and not both storing 
.cont_id/.port_index.

> +static struct tegra_gpio_port_soc_info tegra_main_gpio_cinfo[] = {
> +	TEGRA_MAIN_GPIO_PORT_INFO(A, 2, 0, 7),
> +	TEGRA_MAIN_GPIO_PORT_INFO(B, 3, 0, 7),

I assume the entries in this file must be in the same order as the DT 
binding port IDs? A comment to that effect would be useful.

> +struct tegra_gpio_info;
> +
> +struct tegra_gpio_soc_info {
> +	const char *name;
> +	const struct tegra_gpio_port_soc_info *port;
> +	int nports;
> +};

This isn't information about an SoC; it's information about a 
controller, and there are 2 controllers within Tegra186. Rename to 
tegra_gpio_ctlr_info?

> +struct tegra_gpio_controller {
> +	int controller;
> +	int irq;
> +	struct tegra_gpio_info *tgi;
> +};
> +
> +struct tegra_gpio_info {

Is this structure per-bank/-port? Also, "info" seems to be used above 
for static configuration info/data. I think this should be called 
"tegra_gpio_port"?

> +	struct device *dev;
> +	int nbanks;
> +	void __iomem *gpio_regs;
> +	void __iomem *scr_regs;
> +	struct irq_domain *irq_domain;
> +	const struct tegra_gpio_soc_info *soc;
> +	struct tegra_gpio_controller tg_contrlr[MAX_GPIO_CONTROLLERS];
> +	struct gpio_chip gc;
> +	struct irq_chip ic;
> +};

> +#define GPIO_CNTRL_REG(tgi, gpio, roffset)				    \
> +	((tgi)->gpio_regs + (tgi)->soc->port[GPIO_PORT(gpio)].reg_offset + \
> +	(GPIO_REG_DIFF * GPIO_PIN(gpio)) + (roffset))

Writing a static inline function would make formatting and type safety 
easier.

> +static void tegra_gpio_update(struct tegra_gpio_info *tgi, u32 gpio,
> +				     u32 reg_offset,	u32 mask, u32 val)
> +{
> +	u32 rval;
> +
> +	rval = __raw_readl(GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> +	rval = (rval & ~mask) | (val & mask);
> +	__raw_writel(rval, GPIO_CNTRL_REG(tgi, gpio, reg_offset));
> +}

If you use a regmap object rather than a raw MMIO pointer, I believe 
there's already a function that does this read-modify-write.

> +/* This function will return if the GPIO is accessible by CPU */
> +static bool gpio_is_accessible(struct tegra_gpio_info *tgi, u32 offset)

I'd prefer all functions use the same name prefix. "tegra_gpio" seems to 
be used so far. Actually, given there's already an existing Tegra GPIO 
driver, and this driver covers the new controller(s) in Tegra186, I'd 
prefer everything be named tegra186_gpio_xxx.

> +       if (cont_id  < 0)
> +               return false;

That should trigger a checkpatch error due to the presence of two spaces 
in the expression. Try running checkpatch and fixing any issues.

> +	val = __raw_readl(tgi->scr_regs + scr_offset +
> +			(pin * GPIO_SCR_DIFF) + GPIO_SCR_REG);
> +
> +	if ((val & GPIO_FULL_ACCESS) == GPIO_FULL_ACCESS)
> +		return true;

I'm not entirely convinced about this. It's possible to have only read 
or only write access. I believe the CPU can be assigned to an arbitrary 
bus master group, whereas the value of GPIO_FULL_ACCESS assumes it's on 
group 1. Do we actually need this function at all except for debug? I'd 
be tempted to just drop it and let all GPIO accesses be attempted. If 
the SCR is configured such that the CPU doesn't have access, writes 
should be ignored and reads return valid dummy data. That seems fine to me.

Also, this function isn't consistently used by all client-callable APIs. 
I'd expect it to be used from every function that's accessible via a 
function pointer in struct gpio_chip, if it's useful.

> +static void tegra_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +			   int value)

> +	tegra_gpio_writel(tgi, val, offset, GPIO_OUT_VAL_REG);
> +	tegra_gpio_writel(tgi, 0, offset, GPIO_OUT_CTRL_REG);

Shouldn't this function *just* set the output value; any other setup 
should be done solely as part of direction_input/direction_output?

> +static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type)

> +	tegra_gpio_enable(ctrlr->tgi, gpio);

Shouldn't this only happen when the client actually calls enable/disable?

> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> +		irq_set_handler_locked(d, handle_level_irq);
> +	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> +		irq_set_handler_locked(d, handle_edge_irq);

Shouldn't the handler be set before the IRQ is enabled?

> +static void tegra_gpio_irq_handler(struct irq_desc *desc)

> +	for (i = 0; i < MAX_GPIO_PORTS; ++i)
> +		port_map[i] = -1;
> +
> +	for (i = 0; i < tgi->soc->nports; ++i) {
> +		if (tgi->soc->port[i].cont_id == tg_cont->controller)
> +			port_map[tgi->soc->port[i].port_index] = i;
> +	}

I would have expected the code to use simple math here (iterate over all 
ports in the controller) rather than creating some kind of 
lookup/mapping table.

> +	chained_irq_enter(chip, desc);
> +	for (i = 0; i < MAX_GPIO_PORTS; i++) {
> +		port = port_map[i];
> +		if (port == -1)
> +			continue;
> +
> +		addr = tgi->soc->port[port].reg_offset;
> +		val = __raw_readl(tg_cont->tgi->gpio_regs + addr +
> +				GPIO_INT_STATUS_OFFSET + GPIO_STATUS_G1);
> +		gpio = tgi->gc.base + (port * 8);
> +		for_each_set_bit(pin, &val, 8)
> +			generic_handle_irq(gpio_to_irq(gpio + pin));

For edge-sensitive IRQs, doesn't the status need to be cleared before 
calling the handler, so that (a) the latched status is cleared, (b) if a 
new edge occurs after this point, that fact is recorded and the new IRQ 
handled?

> +#ifdef CONFIG_DEBUG_FS

Using a regmap might give you some of the debug logic for free.

> +static int tegra_gpio_probe(struct platform_device *pdev)
> +{
> +	struct tegra_gpio_info *tgi;
> +	struct resource *res;
> +	int bank;
> +	int gpio;
> +	int ret;
> +
> +	for (bank = 0;; bank++) {
> +		res = platform_get_resource(pdev, IORESOURCE_IRQ, bank);
> +		if (!res)
> +			break;
> +	}
> +	if (!bank) {
> +		dev_err(&pdev->dev, "No GPIO Controller found\n");
> +		return -ENODEV;
> +	}
...
 > +	tgi->nbanks = bank;

There should be a fixed number of IRQs in DT based on the controller 
definition; the value shouldn't be variable.

See Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt

> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt

The U-Boot Tegra186 GPIO driver might also be useful as a reference to 
how to use the DT binding and structure the driver:

> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/gpio/tegra186_gpio.c;h=1c681514db9f50e61a9db041ac2067c084db494c;hb=HEAD

> +	tgi->gc.ngpio			= tgi->soc->nports * 8;

This will leave some gaps in the GPIO numbering, since not all ports 
have 8 GPIOs. I think this is the correct thing to do, but IIRC Thierry 
found this caused some issues in the GPIO core since it attempts to 
query initial status of each GPIO. Did you see this issue during testing?

> +static int __init tegra_gpio_init(void)
> +{
> +	return platform_driver_register(&tegra_gpio_driver);
> +}
> +postcore_initcall(tegra_gpio_init);

I would have expected everything to use module_initcall() these days.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Nov 08, 2016 at 12:07:55PM -0700, Stephen Warren wrote:
> On 11/02/2016 04:48 AM, Suresh Mangipudi wrote:
> > Add GPIO driver for T186 based platforms.
> > Adds support for MAIN and AON GPIO's from T186.
> 
> I'm not sure how you/Thierry will approach merging this with the other GPIO
> driver he has, but here's a very quick review of this one in case it's
> useful.

This puts me in an unfortunate situation. I'd really like to avoid being
the maintainer for this driver, but on the other hand, the version of
the driver that I wrote is pretty much what we'd end up if Stephen's
comments were all addressed. Suresh's driver does a couple of things in
addition (like the accessibility checks), but then I find my driver to
be more easily related to the TRM because it uses the register names
from that.

So I don't really know how to go about merging both. I'll reply to this
email later with a copy of the patch that I wrote, maybe we can take it
from there.

> > +	tgi->gc.ngpio			= tgi->soc->nports * 8;
> 
> This will leave some gaps in the GPIO numbering, since not all ports have 8
> GPIOs. I think this is the correct thing to do, but IIRC Thierry found this
> caused some issues in the GPIO core since it attempts to query initial
> status of each GPIO. Did you see this issue during testing?

I think the immediate issue that I was seeing is avoided in this driver
by the call to gpio_is_accessible() in the ->get_direction() callback.
In the driver that I have there's no such check, and hence I would get
an exception on probe.

However there's another problem with this patch. If you try and export
a non-existing GPIO via sysfs and try to read the value file you can
easily make the driver hang a CPU. This only seems to happen for the
AON GPIO controller.

The approach that I chose was to compact the range of GPIOs that the
GPIO subsystem knows about to the ones that actually exist. This has the
slight disadvantage that we can't use a simple port * 8 + offset to
compute the pin number anymore. However for the primary use-case (GPIO
specifier in DT) that's not a problem because we can translate the pin
number into the compacted space. That means the only issue will be with
sysfs support because if we use the simple formula we'll eventually get
a pin number that's outside of the range.

One way to solve this is to make a massive change to the GPIO subsystem
to check for the validity of a GPIO before any access. I'm not sure if
that's desirable, maybe Linus has some thoughts about that.

If we stick with a compacted number space, there are two solutions that
I can think of to remedy the sysfs problem. One would be to register a
separate struct gpio_chip for each controller. That's kind of a sledge-
hammer solution because it will create multiple number spaces and hence
completely avoid the sparse number space for the whole controller. I
think Stephen had originally proposed this as a solution.

The other possibility would be for the GPIO subsystem to gain per-chip
GPIO export via sysfs. That is, instead of the global export file that
you write a global GPIO number to, each per-chip directory would get
an export file. Values written into that file could get translated via
driver-specific callbacks (much like the ->xlate() callback for GPIO
specifiers). I think that's a change that makes sense anyway. Usually
users will know what GPIO controller they want to access and the offset
of the pin therein. Currently they have to somewhat jump through hoops
to get at the right pin (find controller, read GPIO base, add offset to
base and write that to the export file). The new sequence would be much
more straightforward: find controller, write offset to export file. The
new per-chip export file would be flexible enough to deal with compacted
number spaces, which is obviously something we can't do with the global
export file.

Thierry

On Tue, Nov 22, 2016 at 6:30 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> So I don't really know how to go about merging both. I'll reply to this
> email later with a copy of the patch that I wrote, maybe we can take it
> from there.

I trust you nVidia people to sort this out.

I guess in worst case you can have a company strategy meeting about it.

Let me just say: when one of you have a patch that bears the ACK of the
other, I will merge it.

> However there's another problem with this patch. If you try and export
> a non-existing GPIO via sysfs and try to read the value file you can
> easily make the driver hang a CPU. This only seems to happen for the
> AON GPIO controller.

That sounds like a bug. But I strongly suggest you first and foremost
to test your code with the character device and not through sysfs.

sysfs is second priority, and while I don't want it to screw things up, it
is an optional legacy bolt-on not a compiled-in recommended thing.

The character device, on the other hand, is a recommended
practice for userspace GPIO.

> One way to solve this is to make a massive change to the GPIO subsystem
> to check for the validity of a GPIO before any access. I'm not sure if
> that's desirable, maybe Linus has some thoughts about that.

This is already possible and several drivers are doing this.

Everything, all kernel users and all character device users, end up
calling gpiod_request().

We agree violently that if this GPIO is not valid, inaccessible (etc)
it should not return a valid GPIO descriptor.

Consider drivers/gpio/gpio-stmpe.c which has a device tree property
"st,norequest-mask" that mark some GPIO lines as "nonusable".
This is because they are statically muxed for something else.

(It is a subject of debate whether that is a good way to mark the
lines as unusable, probably not, but it is legacy code.)

We know a number of lines are not elegible for request
or to be used for triggering interrupts.

The code does this in .request():

if (stmpe_gpio->norequest_mask & BIT(offset))
                return -EINVAL;

Thus any gpiod_get() will fail. And we are fine.

Further, since it can also be used as an interrupt parent, it
does this in .probe():

of_property_read_u32(np, "st,norequest-mask",
                 &stmpe_gpio->norequest_mask);

if (stmpe_gpio->norequest_mask)
         stmpe_gpio->chip.irq_need_valid_mask = true;
for (i = 0; i < sizeof(u32); i++)
         if (stmpe_gpio->norequest_mask & BIT(i))
                   clear_bit(i, stmpe_gpio->chip.irq_valid_mask);

How this blocks the IRQs from being requested can be seen
in the irqchip helpers in gpiolib.c. The common GPIOLIB_IRQCHIP
gracefully handles this too.

If the sysfs ABI does not block the use of the same lines
as efficiently, it is a bug in the sysfs code. This works fine
to block access for all in-kernel and chardev users.

But as far as I can tell, sysfs ALSO uses gpiod_get() so it should
work fine.

> If we stick with a compacted number space, there are two solutions that
> I can think of to remedy the sysfs problem. One would be to register a
> separate struct gpio_chip for each controller. That's kind of a sledge-
> hammer solution because it will create multiple number spaces and hence
> completely avoid the sparse number space for the whole controller. I
> think Stephen had originally proposed this as a solution.

Note ambigous use of "controller" above. I'm confused.

"One would be to register a separate struct gpio_chip for each controller."
/ "and hence completely avoid the sparse number space for the whole
controller."

Eheheh

It seems that "controller" refer to two different things in the two
sentences. Do you mean your hardware has ONE controller with
several BANKS? (as described in Documentation/gpio/driver.txt?)

> The other possibility would be for the GPIO subsystem to gain per-chip
> GPIO export via sysfs. That is, instead of the global export file that
> you write a global GPIO number to, each per-chip directory would get
> an export file.

No. The sysfs ABI is in
Documentation/ABI/obsolete/sysfs-gpio
for a reason: I hate it and it should not be extended whatsoever.

Use the new character device, and for a new SoC like the tegra186
you can CERTAINLY convince any downstream consumers to
switch to that.

Please just disable CONFIG_GPIO_SYSFS from your defconfig
and in any company-internal builds and point everyone and their
dog to the character device: tools/gpio/*,
and the UAPI <linux/gpio.h>

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

On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:
> On Tue, Nov 22, 2016 at 6:30 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > So I don't really know how to go about merging both. I'll reply to this
> > email later with a copy of the patch that I wrote, maybe we can take it
> > from there.
> 
> I trust you nVidia people to sort this out.
> 
> I guess in worst case you can have a company strategy meeting about it.
> 
> Let me just say: when one of you have a patch that bears the ACK of the
> other, I will merge it.
> 
> > However there's another problem with this patch. If you try and export
> > a non-existing GPIO via sysfs and try to read the value file you can
> > easily make the driver hang a CPU. This only seems to happen for the
> > AON GPIO controller.
> 
> That sounds like a bug. But I strongly suggest you first and foremost
> to test your code with the character device and not through sysfs.
> 
> sysfs is second priority, and while I don't want it to screw things up, it
> is an optional legacy bolt-on not a compiled-in recommended thing.
> 
> The character device, on the other hand, is a recommended
> practice for userspace GPIO.

The root cause here would be that we don't implement .request(), hence
we're not actually preventing access to the non-existing GPIOs.

> > One way to solve this is to make a massive change to the GPIO subsystem
> > to check for the validity of a GPIO before any access. I'm not sure if
> > that's desirable, maybe Linus has some thoughts about that.
> 
> This is already possible and several drivers are doing this.
> 
> Everything, all kernel users and all character device users, end up
> calling gpiod_request().

It looks like I stumbled across the only case where this isn't true.
What I was seeing, and which ultimately led me to implement the compact
numberspace is that gpiochip_add_data() calls ->get_direction() directly
without first going through ->request(). We'd have to guard that one
case as well in order for this to work.

> We agree violently that if this GPIO is not valid, inaccessible (etc)
> it should not return a valid GPIO descriptor.

Yes.

> Consider drivers/gpio/gpio-stmpe.c which has a device tree property
> "st,norequest-mask" that mark some GPIO lines as "nonusable".
> This is because they are statically muxed for something else.
> 
> (It is a subject of debate whether that is a good way to mark the
> lines as unusable, probably not, but it is legacy code.)
> 
> We know a number of lines are not elegible for request
> or to be used for triggering interrupts.
> 
> The code does this in .request():
> 
> if (stmpe_gpio->norequest_mask & BIT(offset))
>                 return -EINVAL;
> 
> Thus any gpiod_get() will fail. And we are fine.
> 
> Further, since it can also be used as an interrupt parent, it
> does this in .probe():
> 
> of_property_read_u32(np, "st,norequest-mask",
>                  &stmpe_gpio->norequest_mask);
> 
> if (stmpe_gpio->norequest_mask)
>          stmpe_gpio->chip.irq_need_valid_mask = true;
> for (i = 0; i < sizeof(u32); i++)
>          if (stmpe_gpio->norequest_mask & BIT(i))
>                    clear_bit(i, stmpe_gpio->chip.irq_valid_mask);
> 
> How this blocks the IRQs from being requested can be seen
> in the irqchip helpers in gpiolib.c. The common GPIOLIB_IRQCHIP
> gracefully handles this too.

I don't think it does. The issue I mentioned for gpiochip_add_data()
exists in gpiochip_lock_as_irq(). That code assumes that all GPIOs from
0 to chip.ngpio - 1 are valid. And perhaps that's exactly how it should
be. But that's not true in this version of the driver. Perhaps I should
clarify what exactly we're talking about here, because I'm not sure it's
obvious.

On Tegra186 there are two "controllers" (perhaps I should say hardware
blocks) that provide GPIOs. One is in the always-on partition of the
chip and is therefore typically called "AON", whereas the other is
called "main". Both of these hardware blocks implement a number of
"controllers": 1 for AON and 6 for main. This is really only apparent in
the number of interrupts that they receive. AON has one interrupt line
whereas main has 6 of them.

Each hardware block also implements a number of "ports": 8 for AON and
23 for main. Each of these ports has a different number of pins. Often
there will be 6, 7 or 8, but a couple of ports have as little as a
single pin. Each port is associated with a given "controller", that is
interrupt, so when a GPIO of a port is configured as input and enabled
to generate an interrupt, the interrupt of it's parent controller will
be asserted. I'm not exactly sure how this association is done, I have
not found anything in the TRM. Perhaps Laxman, Suresh or Stephen can
clarify. Looking at Suresh's patch there's no clear pattern to it, so I
guess I just missed the table that has this information. In my driver I
chose the easy route and just requested each interrupt with the same
handler, which means we potentially waste some time by reading some
interrupt status registers that we don't have to. This could be made
cleverer by filtering for those that match the controller whose
interrupt we're handling.

Anyway, to get back to what we're really talking about here: Suresh's
patch registers 8 pins per port, regardless of the actual number of pins
that the port has. This simplifies things in some ways. For example the
numbering in DT (see include/dt-bindings/gpio/tegra186-gpio.h) is the
same as gpiolib's internal numbering. Which means that everyone can just
use the index of the desired port, multiply it by 8 and add the pin
index to get at the GPIO number relative to the GPIO chip. The downside
is that we make gpiolib believe that there are more GPIOs in the chip
than actually exist. It's not so much that these pins aren't accessible
because they are used for other purposes (that's something that could
happen in addition) but because they simply don't exist.

With the AON controller this seems critical because it will hang a CPU
(though I've also seen exceptions rather than hangs) if registers that
belong to these non-existing GPIOs are accessed (presumably there are
hardware checks for registers that don't exist).

> If the sysfs ABI does not block the use of the same lines
> as efficiently, it is a bug in the sysfs code. This works fine
> to block access for all in-kernel and chardev users.
> 
> But as far as I can tell, sysfs ALSO uses gpiod_get() so it should
> work fine.

Yeah, I think the implementation is fine, it's just that it relies on a
mechanism that this driver doesn't implement.

> > If we stick with a compacted number space, there are two solutions that
> > I can think of to remedy the sysfs problem. One would be to register a
> > separate struct gpio_chip for each controller. That's kind of a sledge-
> > hammer solution because it will create multiple number spaces and hence
> > completely avoid the sparse number space for the whole controller. I
> > think Stephen had originally proposed this as a solution.
> 
> Note ambigous use of "controller" above. I'm confused.
> 
> "One would be to register a separate struct gpio_chip for each controller."
> / "and hence completely avoid the sparse number space for the whole
> controller."
> 
> Eheheh
> 
> It seems that "controller" refer to two different things in the two
> sentences. Do you mean your hardware has ONE controller with
> several BANKS? (as described in Documentation/gpio/driver.txt?)

I hope the above clarifies things. struct gpio_chip represents the
entire hardware block (in current drivers) and the driver deals with
individual controllers and ports internally. The proposal I was talking
about above is to have one driver create multiple struct gpio_chip, each
representing an individual port. Hence each struct gpio_chip would be
assigned the exact number of pins that the port supports, removing all
of the problems with numberspaces. It has its own set of disadvantages,
such as creating a large number of GPIO chips, which may in the end be
just as confusing as the current implementation.

> > The other possibility would be for the GPIO subsystem to gain per-chip
> > GPIO export via sysfs. That is, instead of the global export file that
> > you write a global GPIO number to, each per-chip directory would get
> > an export file.
> 
> No. The sysfs ABI is in
> Documentation/ABI/obsolete/sysfs-gpio
> for a reason: I hate it and it should not be extended whatsoever.
> 
> Use the new character device, and for a new SoC like the tegra186
> you can CERTAINLY convince any downstream consumers to
> switch to that.

I've looked a little at the character device implementation and I think
it would work equally bad with the compact numberspace as sysfs. The
reason is that gpiolib doesn't allow remapping of chip-relative indices
except for DT. I think this might be possible by generalizing the
->of_xlate() to be used in translating internal numbers as well. The way
I could imagine this to work is that an IOCTL providing offsets of the
lines to use would pass the offsets through the chip's ->xlate()
function to retrieve the index (0 to chip->ngpio). The alternative is to
stick with the sparse numberspace and deal with non-existing GPIOs via a
->request() callback.

> Please just disable CONFIG_GPIO_SYSFS from your defconfig
> and in any company-internal builds and point everyone and their
> dog to the character device: tools/gpio/*,
> and the UAPI <linux/gpio.h>

I don't think we can easily do that. People may still rely on the sysfs
interface, or even if they aren't this might be enabled in a multi-
platform image. So I think regardless of whether or not we like the
interface, we have to make sure that our solution plays nicely with it.
There is no problem with the compact numberspace, though it comes with
the inconvenience that numbering in sysfs is different from numbering in
DT.

In summary I think we have three options:

  1) use a sparse numberspace and deal with non-existing GPIOs via the
     ->request() callback

  2) use a compact numberspace and live with separate methods of
     referencing GPIOs

  3) use a compact numberspace and introduce a mechanism to translate
     all hardware numbers into per-chip indices

I think 1) is the simplest, but at the cost of wasting memory and CPU
cycles by maintaining descriptors for GPIOs we know don't exist. 2) is
fairly simple as well, but it's pretty inconvenient for the user. The
most invasive solution would be 3), though I personally like that best
because it is the most explicit. It does have the disavantage of using
separate numbering for sysfs and everyone else, though. Unless you're
willing to make sysfs use per-chip export/unexport files and the
->xlate() callback.

Thierry

On Thursday 24 November 2016 01:10 AM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:
>>
>> This is already possible and several drivers are doing this.
>>
>> Everything, all kernel users and all character device users, end up
>> calling gpiod_request().
> It looks like I stumbled across the only case where this isn't true.
> What I was seeing, and which ultimately led me to implement the compact
> numberspace is that gpiochip_add_data() calls ->get_direction() directly
> without first going through ->request(). We'd have to guard that one
> case as well in order for this to work.
>
In T186, we have 8 pins per PORT and for some of ports, all pins are not 
available. Like Port A has 7 pins valid (0 to 6) and port E have 8 pins 
(0 to 7). The great part is that each port has valid pins start from 0.

So just having the number of valid pins for each port as part of SOC 
data will help to find out whether GPIO exist or not.

        int port = GPIO_PORT(offset);
         int pin = GPIO_PIN(offset);

       if (pin >= tgi->soc->port[port].valid_pins)
                 return false;

Similar logic can be used for APIs which can get called without 
gpio_request().

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

On Thu, Nov 24, 2016 at 12:06:16PM +0530, Laxman Dewangan wrote:
> 
> On Thursday 24 November 2016 01:10 AM, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:
> > > 
> > > This is already possible and several drivers are doing this.
> > > 
> > > Everything, all kernel users and all character device users, end up
> > > calling gpiod_request().
> > It looks like I stumbled across the only case where this isn't true.
> > What I was seeing, and which ultimately led me to implement the compact
> > numberspace is that gpiochip_add_data() calls ->get_direction() directly
> > without first going through ->request(). We'd have to guard that one
> > case as well in order for this to work.
> > 
> In T186, we have 8 pins per PORT and for some of ports, all pins are not
> available. Like Port A has 7 pins valid (0 to 6) and port E have 8 pins (0
> to 7). The great part is that each port has valid pins start from 0.

From what I can tell that's not true. We don't always have 8 pins per
port. That's just the definition that we use to simplify the external
numbering of GPIOs.

> So just having the number of valid pins for each port as part of SOC data
> will help to find out whether GPIO exist or not.
> 
>        int port = GPIO_PORT(offset);
>         int pin = GPIO_PIN(offset);
> 
>       if (pin >= tgi->soc->port[port].valid_pins)
>                 return false;

Yes, that's exactly what tegra186_gpio_of_xlate() does, because its job
is to translate from the (sparse) numbering defined in the bindings to
the internal compact numbering.

> 
> Similar logic can be used for APIs which can get called without
> gpio_request().

No it doesn't. There are currently a couple of cases where these
functions get called without first requesting the GPIO. These are all
internal to gpiolib as far as I can tell, but they all assume that a
GPIO with an index that's within the range from 0 to chip->ngpio will
exist.

On Tegra186 that's not the case, so we have to add code to every
callback to check whether or not a given pin exists. And the subsystem
is not equipped to deal with that properly, because it doesn't allow all
callbacks to return an error.

So, like I said in the other subthread the only way to make this sparse
number scheme work correctly is to implement ->request() and make sure
that all calls to GPIO operations are properly guarded with a call to
->request().

I still don't like very much how we'd be registering GPIOs that don't
exist physically, not only because we waste memory and CPU cycles, but
also because the driver becomes more prone to errors. If ever somebody
added new direct calls to one of the operations in the gpiolib core
without guarding them with ->request() our driver would likely break.

If we absolutely have to use the same numbering internally as in DT, we
need to at least make sure to properly handle accesses to them.

Thierry

On Wed, Nov 23, 2016 at 8:40 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:

>> Everything, all kernel users and all character device users, end up
>> calling gpiod_request().
>
> It looks like I stumbled across the only case where this isn't true.
> What I was seeing, and which ultimately led me to implement the compact
> numberspace is that gpiochip_add_data() calls ->get_direction() directly
> without first going through ->request(). We'd have to guard that one
> case as well in order for this to work.

Hm. So there are two ways we could address that:

- Make irq_valid_mask a pure .valid_mask so we can mask off
  gpios not just for IRQs but for anything, if this is consistent
  with what Mika needs or

- Make the .get_direction() callback return -EINVAL or whatever
  for the non-available lines, exactly how .request() does it,
  and manage that in the core when looping over them to get
  the direction in the gpiochip_add() call.

>> How this blocks the IRQs from being requested can be seen
>> in the irqchip helpers in gpiolib.c. The common GPIOLIB_IRQCHIP
>> gracefully handles this too.
>
> I don't think it does. The issue I mentioned for gpiochip_add_data()
> exists in gpiochip_lock_as_irq(). That code assumes that all GPIOs from
> 0 to chip.ngpio - 1 are valid.

It shouldn't matter since we should deny these to be even
mapped as IRQs before the irqchip callbacks for requesting
resources are called so it's not happening. This is happening
with GPIOLIB_IRQCHIP.

> Each hardware block also implements a number of "ports": 8 for AON and
> 23 for main. Each of these ports has a different number of pins. Often
> there will be 6, 7 or 8, but a couple of ports have as little as a
> single pin.

What a maze. I would be happy if all this weirdness resides
in the device driver ;)

> Each port is associated with a given "controller", that is
> interrupt, so when a GPIO of a port is configured as input and enabled
> to generate an interrupt, the interrupt of it's parent controller will
> be asserted. I'm not exactly sure how this association is done, I have
> not found anything in the TRM.

Is nVidia one of those throw-over-the-wall engineering companies
where hardware engineers toss a chip over the wall to the software
developers and don't expect them to ask any questions?

I'm asking since there are so many nVidia people on this thread.

I would normally expect that you could simply go and ask the
hardware people?

>> It seems that "controller" refer to two different things in the two
>> sentences. Do you mean your hardware has ONE controller with
>> several BANKS? (as described in Documentation/gpio/driver.txt?)
>
> I hope the above clarifies things. struct gpio_chip represents the
> entire hardware block (in current drivers) and the driver deals with
> individual controllers and ports internally. The proposal I was talking
> about above is to have one driver create multiple struct gpio_chip, each
> representing an individual port. Hence each struct gpio_chip would be
> assigned the exact number of pins that the port supports, removing all
> of the problems with numberspaces. It has its own set of disadvantages,
> such as creating a large number of GPIO chips, which may in the end be
> just as confusing as the current implementation.

I have a soft preference toward making one chip for each port/bank.

But it is not strong.

That opionon is stronger if the port/bank has resources such as a
clock, power supply or IRQ line. Then it tends toward mandatory
to have a gpio_chip for each.

>> Use the new character device, and for a new SoC like the tegra186
>> you can CERTAINLY convince any downstream consumers to
>> switch to that.
>
> I've looked a little at the character device implementation and I think
> it would work equally bad with the compact numberspace as sysfs. The
> reason is that gpiolib doesn't allow remapping of chip-relative indices
> except for DT. I think this might be possible by generalizing the
> ->of_xlate() to be used in translating internal numbers as well. The way
> I could imagine this to work is that an IOCTL providing offsets of the
> lines to use would pass the offsets through the chip's ->xlate()
> function to retrieve the index (0 to chip->ngpio). The alternative is to
> stick with the sparse numberspace and deal with non-existing GPIOs via a
> ->request() callback.

I don't understand. Userspace should have no concern about the
numberspace. Lines can be named from DT or ACPI so you can
look up line chip+offset from the names.

Further sysfs sys/bus/gpio (notice: NOT /sys/class/gpio !)
contains topological information, that is the approach taken by
userspace helpers such as udev.

>> Please just disable CONFIG_GPIO_SYSFS from your defconfig
>> and in any company-internal builds and point everyone and their
>> dog to the character device: tools/gpio/*,
>> and the UAPI <linux/gpio.h>
>
> I don't think we can easily do that. People may still rely on the sysfs
> interface, or even if they aren't this might be enabled in a multi-
> platform image.

Good suggestion. I will go and look at the upstream multiplatform
configs and eradicate this.

> So I think regardless of whether or not we like the
> interface, we have to make sure that our solution plays nicely with it.

I would be concerned if you are designing your driver around the
the way the legacy sysfs happens to work.

The thing is broken. Don't design FOR it.

You are making me tempted to require that for new hardware the
driver has to

   depends on !GPIO_SYSFS

In order to make this a non-argument.

Well it would be undiplomatic on my part I guess. But I have to
encourage/nudge a switch somehow.

> There is no problem with the compact numberspace, though it comes with
> the inconvenience that numbering in sysfs is different from numbering in
> DT.

That is fixed in the chardev ABI. Offset on the chardev is the same
as the internal offset of gpio_chip, and therefore the same as used
when doing xlate in the DT for a chip, i.e. the DT offset numbering.

>   1) use a sparse numberspace and deal with non-existing GPIOs via the
>      ->request() callback
>
>   2) use a compact numberspace and live with separate methods of
>      referencing GPIOs
>
>   3) use a compact numberspace and introduce a mechanism to translate
>      all hardware numbers into per-chip indices
>
> I think 1) is the simplest, but at the cost of wasting memory and CPU
> cycles by maintaining descriptors for GPIOs we know don't exist.

You could make a quick calculation of how much that is actually.
I doubt it matters for a contemporary non-IoT platform, but I may
be wrong.

> 2) is
> fairly simple as well, but it's pretty inconvenient for the user. The
> most invasive solution would be

Inconvenient to in-kernel users or userspace users?

Inconvenient to sysfs users or chardev users?

If it is invonvenient to sysfs users is of no concern, as long
as it is no broken.

> 3), though I personally like that best
> because it is the most explicit. It does have the disavantage of using
> separate numbering for sysfs and everyone else, though. Unless you're
> willing to make sysfs use per-chip export/unexport files and the
> ->xlate() callback.

I don't know if I even understand (3).

Consistency in the sysfs ABI does not concern me. It is inherently
inconsistent already. It will be consistently messy for this hardware,
but it shouldn't be used.

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

On Thu, Nov 24, 2016 at 04:08:08PM +0100, Linus Walleij wrote:
> On Wed, Nov 23, 2016 at 8:40 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Wed, Nov 23, 2016 at 02:25:51PM +0100, Linus Walleij wrote:
> 
> >> Everything, all kernel users and all character device users, end up
> >> calling gpiod_request().
> >
> > It looks like I stumbled across the only case where this isn't true.
> > What I was seeing, and which ultimately led me to implement the compact
> > numberspace is that gpiochip_add_data() calls ->get_direction() directly
> > without first going through ->request(). We'd have to guard that one
> > case as well in order for this to work.
> 
> Hm. So there are two ways we could address that:
> 
> - Make irq_valid_mask a pure .valid_mask so we can mask off
>   gpios not just for IRQs but for anything, if this is consistent
>   with what Mika needs or
> 
> - Make the .get_direction() callback return -EINVAL or whatever
>   for the non-available lines, exactly how .request() does it,
>   and manage that in the core when looping over them to get
>   the direction in the gpiochip_add() call.

Another alternative might be to do a ->request() on the line first
before even attempting to call ->get_direction(), which would make
checking for valid lines uniform with all other places. That way a
driver wouldn't have to perform the same check in both callbacks.

> >> How this blocks the IRQs from being requested can be seen
> >> in the irqchip helpers in gpiolib.c. The common GPIOLIB_IRQCHIP
> >> gracefully handles this too.
> >
> > I don't think it does. The issue I mentioned for gpiochip_add_data()
> > exists in gpiochip_lock_as_irq(). That code assumes that all GPIOs from
> > 0 to chip.ngpio - 1 are valid.
> 
> It shouldn't matter since we should deny these to be even
> mapped as IRQs before the irqchip callbacks for requesting
> resources are called so it's not happening. This is happening
> with GPIOLIB_IRQCHIP.

Yes, it looks as though the irqchip->irq_request_resources() will only
be called at IRQ request time, so the gpiochip_lock_as_irq() will be
called after the chip was registered and so I assume the irq_valid_mask
would be properly set up to avoid callbacks for non-existing GPIOs.

> > Each hardware block also implements a number of "ports": 8 for AON and
> > 23 for main. Each of these ports has a different number of pins. Often
> > there will be 6, 7 or 8, but a couple of ports have as little as a
> > single pin.
> 
> What a maze. I would be happy if all this weirdness resides
> in the device driver ;)
> 
> > Each port is associated with a given "controller", that is
> > interrupt, so when a GPIO of a port is configured as input and enabled
> > to generate an interrupt, the interrupt of it's parent controller will
> > be asserted. I'm not exactly sure how this association is done, I have
> > not found anything in the TRM.
> 
> Is nVidia one of those throw-over-the-wall engineering companies
> where hardware engineers toss a chip over the wall to the software
> developers and don't expect them to ask any questions?
> 
> I'm asking since there are so many nVidia people on this thread.
> 
> I would normally expect that you could simply go and ask the
> hardware people?

I can certainly find out, but I was hoping that since Suresh had these
numbers in the patch, he would know where to find the information. It's
not so much that we can't get answers from engineering, quite the
opposite actually. I only refer to the TRM because it is supposed to be
the canonical documentation and if this information isn't there we need
to get it added. Doing that can be somewhat tedious, and me being lazy
I was hoping that I had simply missed it.

> >> It seems that "controller" refer to two different things in the two
> >> sentences. Do you mean your hardware has ONE controller with
> >> several BANKS? (as described in Documentation/gpio/driver.txt?)
> >
> > I hope the above clarifies things. struct gpio_chip represents the
> > entire hardware block (in current drivers) and the driver deals with
> > individual controllers and ports internally. The proposal I was talking
> > about above is to have one driver create multiple struct gpio_chip, each
> > representing an individual port. Hence each struct gpio_chip would be
> > assigned the exact number of pins that the port supports, removing all
> > of the problems with numberspaces. It has its own set of disadvantages,
> > such as creating a large number of GPIO chips, which may in the end be
> > just as confusing as the current implementation.
> 
> I have a soft preference toward making one chip for each port/bank.
> 
> But it is not strong.
> 
> That opionon is stronger if the port/bank has resources such as a
> clock, power supply or IRQ line. Then it tends toward mandatory
> to have a gpio_chip for each.

There really aren't any per-port resources. In fact now that I think
about it adding a gpio_chip per port might actually make things more
difficult because the interrupts are per-controller which may control
one or more ports.

> >> Use the new character device, and for a new SoC like the tegra186
> >> you can CERTAINLY convince any downstream consumers to
> >> switch to that.
> >
> > I've looked a little at the character device implementation and I think
> > it would work equally bad with the compact numberspace as sysfs. The
> > reason is that gpiolib doesn't allow remapping of chip-relative indices
> > except for DT. I think this might be possible by generalizing the
> > ->of_xlate() to be used in translating internal numbers as well. The way
> > I could imagine this to work is that an IOCTL providing offsets of the
> > lines to use would pass the offsets through the chip's ->xlate()
> > function to retrieve the index (0 to chip->ngpio). The alternative is to
> > stick with the sparse numberspace and deal with non-existing GPIOs via a
> > ->request() callback.
> 
> I don't understand. Userspace should have no concern about the
> numberspace. Lines can be named from DT or ACPI so you can
> look up line chip+offset from the names.

Userspace would have to know about the numberspace if it wants to use
the character device, right? The numberspace in DT assumes that each
port has 8 pins (this makes it very easy to create a unique index by
doing (port * 8 + offset) as in:

	#define TEGRA_MAIN_GPIO_PORT_A 0
	#define TEGRA_MAIN_GPIO_PORT_B 1
	...

	#define TEGRA_MAIN_GPIO(port, offset) \
		((TEGRA_MAIN_GPIO_PORT_##port * 8) + offset)

It also provides an easy way for the driver to check for validity: a
port can be obtained by dividing the DT index by 8, and the offset of
the pin is the remainder of the division. If the pin number is higher
than the number of pins supported by the given port we can return an
error. That's what tegra186_gpio_of_xlate() does, it translates from
the DT sparse numberspace into the compact numberspace in gpiolib and
rejects invalid pins at the same time.

> Further sysfs sys/bus/gpio (notice: NOT /sys/class/gpio !)
> contains topological information, that is the approach taken by
> userspace helpers such as udev.
> 
> >> Please just disable CONFIG_GPIO_SYSFS from your defconfig
> >> and in any company-internal builds and point everyone and their
> >> dog to the character device: tools/gpio/*,
> >> and the UAPI <linux/gpio.h>
> >
> > I don't think we can easily do that. People may still rely on the sysfs
> > interface, or even if they aren't this might be enabled in a multi-
> > platform image.
> 
> Good suggestion. I will go and look at the upstream multiplatform
> configs and eradicate this.
> 
> > So I think regardless of whether or not we like the
> > interface, we have to make sure that our solution plays nicely with it.
> 
> I would be concerned if you are designing your driver around the
> the way the legacy sysfs happens to work.
> 
> The thing is broken. Don't design FOR it.

That's not what I meant. I just don't want the driver to crash the
kernel if somebody happened to use it via sysfs.

> You are making me tempted to require that for new hardware the
> driver has to
> 
>    depends on !GPIO_SYSFS
> 
> In order to make this a non-argument.
> 
> Well it would be undiplomatic on my part I guess. But I have to
> encourage/nudge a switch somehow.
> 
> > There is no problem with the compact numberspace, though it comes with
> > the inconvenience that numbering in sysfs is different from numbering in
> > DT.
> 
> That is fixed in the chardev ABI. Offset on the chardev is the same
> as the internal offset of gpio_chip, and therefore the same as used
> when doing xlate in the DT for a chip, i.e. the DT offset numbering.

That's not quite true. In the version of the driver that I have the
->of_xlate() translates the DT numberspace into an internal one that
doesn't have the holes which are problematic (because the associated
registers don't exist). What you are referring to would be what the
of_gpio_simple_xlate() does.

> >   1) use a sparse numberspace and deal with non-existing GPIOs via the
> >      ->request() callback
> >
> >   2) use a compact numberspace and live with separate methods of
> >      referencing GPIOs
> >
> >   3) use a compact numberspace and introduce a mechanism to translate
> >      all hardware numbers into per-chip indices
> >
> > I think 1) is the simplest, but at the cost of wasting memory and CPU
> > cycles by maintaining descriptors for GPIOs we know don't exist.
> 
> You could make a quick calculation of how much that is actually.
> I doubt it matters for a contemporary non-IoT platform, but I may
> be wrong.

For the main controller the number of physically available GPIO lines is
140, whereas the DT numbering assumes there are 184. That's 44 more than
there really are, which is about 30% overhead. For AON it's 47 to 64 and
about 25% overhead.

It's not so much that it's a real problem, but more of a matter of
principle. I'm not at all concerned about this having an adverse effect
in practice, I just think it's wrong.

But if everyone else thinks it's okay, I'm sure I can find ways to live
with it.

> > 2) is
> > fairly simple as well, but it's pretty inconvenient for the user. The
> > most invasive solution would be
> 
> Inconvenient to in-kernel users or userspace users?

in-kernel users would be using DT to refer to the GPIOs and that's a
solved problem because ->of_xlate() gives us a way of translating
between them.

> Inconvenient to sysfs users or chardev users?

It would be inconvenient for both because even though chardev gives us
per-chip offsets, there is no way to translate these offsets from the
external to the internal numberspace. Hence chardev would inevitably
have to use a numberspace different from that of DT.

> If it is invonvenient to sysfs users is of no concern, as long
> as it is no broken.

For sysfs there's no way we could use a compact numberspace and be
compatible with the DT numberspace because it is not possible to
translate from the global numberspace to a per-chip numberspace if
it isn't a 1:1 mapping.

> > 3), though I personally like that best
> > because it is the most explicit. It does have the disavantage of using
> > separate numbering for sysfs and everyone else, though. Unless you're
> > willing to make sysfs use per-chip export/unexport files and the
> > ->xlate() callback.
> 
> I don't know if I even understand (3).

Essentially this would mean generalizing the ->of_xlate() to apply to
all users (DT and chardev alike, and I suppose ACPI as well). I think
it's a superior solution because it's completely generic. Currently I
think all of the GPIO users are forced into a 1:1 mapping and working
around it if that's not how the hardware works. A generic ->xlate()
would completely separate the external numberspace from the internal
contiguous, compact numberspace. And it would allow us to do so in a
unified way.

Thierry

Mostly it seems we are in understanding here, looking forward
to the patches...

On Thu, Nov 24, 2016 at 5:32 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Thu, Nov 24, 2016 at 04:08:08PM +0100, Linus Walleij wrote:

>> I don't understand. Userspace should have no concern about the
>> numberspace. Lines can be named from DT or ACPI so you can
>> look up line chip+offset from the names.
>
> Userspace would have to know about the numberspace if it wants to use
> the character device, right?

No it shouldn't really. Adding gpio-line-names = "foo", "bar" etc
and following a reasonable naming policy (such as using the
rail names on the schematic, which is usually helpful) the
GPIO can easily be looked up from its name.

The chardev tries to overcome the hardships in finding the
GPIO you want this way.

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