Re: [PATCH] gpio: tegra186: Add support for T186 GPIO

[Thierry Reding]

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@xxxxxxxxx> 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

Attachment: signature.asc
Description: PGP signature