[RFC 0/3] pinctrl: add a driver for NVIDIA Tegra

[RFC 0/3] pinctrl: add a driver for NVIDIA Tegra

[Stephen Warren]

This isn't quite ready for merging yet; there are some issues that need
to be sorted out w.r.t. how to replace the existing Tegra pinmux driver
first. However, I figured it'd be good to get a review of the pinctrl
driver itself now that it should be complete.

The first patch is the driver. The other 2 patches are hacks that I use
to test it, just in case anyone is interested. Patch 2 would be fixed
before actually submitting, and patch 3 dropped.

Stephen Warren (3):
  pinctrl: add a driver for NVIDIA Tegra
  arm/tegra: Select PINMUX Kconfig variables
  New pinmux testing hacks

 arch/arm/boot/dts/tegra20.dtsi                   |    8 +
 arch/arm/mach-tegra/Kconfig                      |    3 +
 arch/arm/mach-tegra/include/mach/pinconf-tegra.h |   58 +
 arch/arm/mach-tegra/pinmux.c                     |    2 +
 drivers/pinctrl/Kconfig                          |   15 +
 drivers/pinctrl/Makefile                         |    3 +
 drivers/pinctrl/pinctrl-tegra.c                  |  604 ++++
 drivers/pinctrl/pinctrl-tegra.h                  |   83 +
 drivers/pinctrl/pinctrl-tegra20.c                | 2731 +++++++++++++++++
 drivers/pinctrl/pinctrl-tegra30.c                | 3387 ++++++++++++++++++++++
 10 files changed, 6894 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-tegra/include/mach/pinconf-tegra.h
 create mode 100644 drivers/pinctrl/pinctrl-tegra.c
 create mode 100644 drivers/pinctrl/pinctrl-tegra.h
 create mode 100644 drivers/pinctrl/pinctrl-tegra20.c
 create mode 100644 drivers/pinctrl/pinctrl-tegra30.c

[RFC 1/3] pinctrl: add a driver for NVIDIA Tegra

[Linus Walleij]

On Thu, Dec 8, 2011 at 11:13 PM, Stephen Warren <swarren@nvidia.com> wrote:

> This adds a driver for the Tegra pinmux, and required parameterization
> data for Tegra20 and Tegra30.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

This is looking good from a framework point of view (obviously,
since you've designed the framework with me you sure know what
you're doing).

What we could worry about is the amount of hard-coded chip data
which sort of correlates with the discussion with Tony on how to
provide DT info for pin control drivers.

If say this same controller appear in Tegra 4 with no changes but
different pin names, it makes sense to try to push this into the
DT as soon as possible, so as to avoid the situation Tony is
having with the OMAP muxes. If Tegra 4 will be all-new and not even
related, it doesn't.

So, just think a bit about it.

It will fit way better here than any place under
arch/arm/* in any case, so it's a great achievement!

Yours,
Linus Walleij

[RFC 1/3] pinctrl: add a driver for NVIDIA Tegra

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [111209 05:29]:
> On Thu, Dec 8, 2011 at 11:13 PM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> > This adds a driver for the Tegra pinmux, and required parameterization
> > data for Tegra20 and Tegra30.
> >
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> 
> This is looking good from a framework point of view (obviously,
> since you've designed the framework with me you sure know what
> you're doing).
> 
> What we could worry about is the amount of hard-coded chip data
> which sort of correlates with the discussion with Tony on how to
> provide DT info for pin control drivers.
> 
> If say this same controller appear in Tegra 4 with no changes but
> different pin names, it makes sense to try to push this into the
> DT as soon as possible, so as to avoid the situation Tony is
> having with the OMAP muxes. If Tegra 4 will be all-new and not even
> related, it doesn't.
> 
> So, just think a bit about it.
> 
> It will fit way better here than any place under
> arch/arm/* in any case, so it's a great achievement!

Sorry I still need some more time with pinmux-simple.c, will have
to get pending omap patches merged first. Just want to recap the
findings so far:

- We can have automatically generated device to pinmux driver
  mapping from DT, so that's nice

- Describing pins for each driver in DT is still open.. And
  using mixed-property arrays in DT won't nicely as the data
  gets unaligned easily with string properties..

- We currently have hard time supporting pin groups with pins
  coming from multiple pinmux driver instances

- Mixing pinmux data from various sources is still open; Some
  pinmux data may need to be static, some come from DT, some
  come from loadable modules or even /lib/firmware if the data
  gets insanely big. Basically we may not always have a phandle
  in DT data for the pinmux function, and should somehow allow
  also using names there

Regards,

Tony

[RFC 1/3] pinctrl: add a driver for NVIDIA Tegra

[Linus Walleij]

On Fri, Dec 9, 2011 at 6:07 PM, Tony Lindgren <tony@atomide.com> wrote:

> - We currently have hard time supporting pin groups with pins
> ?coming from multiple pinmux driver instances

Pin groups are per definiton per-controller inctance. That is a
given side effect of not having a global pin space.

But you can create two groups of the same name on two
pin controllers so that e.g. pinctrl.0 and pinctrl.1 have
a group named "foo", then map both to a certain device
in the board pinmux map.

You will have to request them individually however, like
with two hogs or two pinmux_get() calls specifying the
name of each map, else you will just get the first one.

Not that I've tried it, but that's how it's supposed to work...

Yours,
Linus Walleij

[RFC 1/3] pinctrl: add a driver for NVIDIA Tegra

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [111209 15:17]:
> On Fri, Dec 9, 2011 at 6:07 PM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > - We currently have hard time supporting pin groups with pins
> > ?coming from multiple pinmux driver instances
> 
> Pin groups are per definiton per-controller inctance. That is a
> given side effect of not having a global pin space.
> 
> But you can create two groups of the same name on two
> pin controllers so that e.g. pinctrl.0 and pinctrl.1 have
> a group named "foo", then map both to a certain device
> in the board pinmux map.
> 
> You will have to request them individually however, like
> with two hogs or two pinmux_get() calls specifying the
> name of each map, else you will just get the first one.
> 
> Not that I've tried it, but that's how it's supposed to work...

Yes a little bit of trickery is needed there, I'll look into
it more as we already have two controller instances on omap4.

Regards,

Tony

[RFC 1/3] pinctrl: add a driver for NVIDIA Tegra

[Stephen Warren]

Linus Walleij wrote at Friday, December 09, 2011 7:01 AM:
> On Thu, Dec 8, 2011 at 11:13 PM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> > This adds a driver for the Tegra pinmux, and required parameterization
> > data for Tegra20 and Tegra30.
> >
> > Signed-off-by: Stephen Warren <swarren@nvidia.com>
> 
> This is looking good from a framework point of view (obviously,
> since you've designed the framework with me you sure know what
> you're doing).
> 
> What we could worry about is the amount of hard-coded chip data
> which sort of correlates with the discussion with Tony on how to
> provide DT info for pin control drivers.

My thinking here is that irrespective of whether the data in future chips
is the same or different to the current chips, it's better in the driver.

For a given SoC, the data is static; it can never ever change.

Hence, there's no point parsing it from device tree; we end up with exactly
the same data in the driver, yet have spent a bunch of time parsing it out
from device tree instead of just embedding it into the kernel binary.

If parts of Tegra X and Tegra Y are similar, it should be possible to have
them co-ordinate together and share the common data, and do whatever it
takes to create the appropriate completed view before passing it back from
tegraX_pinctrl_init() to the core. You could perhaps do it using /include/
in the .dtsi file too, but I think allowing the SoC init code to unify the
tables gives more flexibility; in your example below of the same data with
different pin names, that'd be implementable with code without too much
difficulty, but probably impossible with dtc since it has no macro/define
support at present.

Also, the representation of the data in a .c file will likely be far
smaller than in the .dtsi file, so this way saves space. Admittedly with
a multi-SoC binary, you end up with all the large per-SoC data in the
binary initially, so the space-savings aren't exactly seen, but it's
all init data, so does get dumped soon after boot.

Another point here: I don't have to maintain a DT binding for the Tegra
pinctrl driver this way, so if the next Tegra's pinmux HW is different,
all I need to do is edit the driver, not be shackled by a need to keep
the DT binding for it backwards-compatible. Although that said, I
/think/ even an obvious DT binding derived from struct
tegra_pinctrl_soc_data would be flexible enough for most things, perhaps
with the addition of allowing config param field definitions for pins
as well as groups.

> If say this same controller appear in Tegra 4 with no changes but
> different pin names, it makes sense to try to push this into the
> DT as soon as possible, so as to avoid the situation Tony is
> having with the OMAP muxes. If Tegra 4 will be all-new and not even
> related, it doesn't.
> 
> So, just think a bit about it.

Sorry if my replying quickly seems like I haven't thought about it; it's
just that I already thought about this a while ago, and came to the
conclusions above.

> It will fit way better here than any place under
> arch/arm/* in any case, so it's a great achievement!

-- 
nvpublic

[RFC 1/3] pinctrl: add a driver for NVIDIA Tegra

[Tony Lindgren]

* Stephen Warren <swarren@nvidia.com> [111209 08:56]:
> Linus Walleij wrote at Friday, December 09, 2011 7:01 AM:
> > 
> > What we could worry about is the amount of hard-coded chip data
> > which sort of correlates with the discussion with Tony on how to
> > provide DT info for pin control drivers.
> 
> My thinking here is that irrespective of whether the data in future chips
> is the same or different to the current chips, it's better in the driver.
> 
> For a given SoC, the data is static; it can never ever change.
> 
> Hence, there's no point parsing it from device tree; we end up with exactly
> the same data in the driver, yet have spent a bunch of time parsing it out
> from device tree instead of just embedding it into the kernel binary.
> 
> If parts of Tegra X and Tegra Y are similar, it should be possible to have
> them co-ordinate together and share the common data, and do whatever it
> takes to create the appropriate completed view before passing it back from
> tegraX_pinctrl_init() to the core. You could perhaps do it using /include/
> in the .dtsi file too, but I think allowing the SoC init code to unify the
> tables gives more flexibility; in your example below of the same data with
> different pin names, that'd be implementable with code without too much
> difficulty, but probably impossible with dtc since it has no macro/define
> support at present.
> 
> Also, the representation of the data in a .c file will likely be far
> smaller than in the .dtsi file, so this way saves space. Admittedly with
> a multi-SoC binary, you end up with all the large per-SoC data in the
> binary initially, so the space-savings aren't exactly seen, but it's
> all init data, so does get dumped soon after boot.
> 
> Another point here: I don't have to maintain a DT binding for the Tegra
> pinctrl driver this way, so if the next Tegra's pinmux HW is different,
> all I need to do is edit the driver, not be shackled by a need to keep
> the DT binding for it backwards-compatible. Although that said, I
> /think/ even an obvious DT binding derived from struct
> tegra_pinctrl_soc_data would be flexible enough for most things, perhaps
> with the addition of allowing config param field definitions for pins
> as well as groups.

What you're describing should be supported for sure.

IMHO the key issue here from pinctrl fwk point of view is: We want to
allow multiple data sources for describing the pinmux functions.

The data sources supported should be any combination of static platform_data,
DT data, loadable modules, and /lib/firmware. Then it's just a question of
using the most suitable method for each SoC.

Regards,

Tony

[RFC 1/3] pinctrl: add a driver for NVIDIA Tegra

[Linus Walleij]

On Fri, Dec 9, 2011 at 6:28 PM, Stephen Warren <swarren@nvidia.com> wrote:

> Hence, there's no point parsing it from device tree; we end up with exactly
> the same data in the driver, yet have spent a bunch of time parsing it out
> from device tree instead of just embedding it into the kernel binary.

For me there are two points:

- Avoiding clash with kernel maintainers who hate firmware-like tables and
  binary data filling up the kernel. Some certain other guy bearing my
  name comes to mind.

- Footprint: the majority of the stuff in your driver ends up in non-discardable
  memory, and will be kept around. Since we have this concept of a
  single zImage for a number of say ARMv7 systems, and since pin
  controllers can be pretty hard to load from modules, *all* of them
  may have to be compiled-in. If every driver for every board takes this
  approach I am afraid it will be end up with a measurable footprint.

Yours,
Linus Walleij

[RFC 1/3] pinctrl: add a driver for NVIDIA Tegra

[Stephen Warren]

Linus Walleij wrote at Friday, December 09, 2011 4:56 PM:
> On Fri, Dec 9, 2011 at 6:28 PM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> > Hence, there's no point parsing it from device tree; we end up with exactly
> > the same data in the driver, yet have spent a bunch of time parsing it out
> > from device tree instead of just embedding it into the kernel binary.
> 
> For me there are two points:
> 
> - Avoiding clash with kernel maintainers who hate firmware-like tables and
>   binary data filling up the kernel. Some certain other guy bearing my
>   name comes to mind.

Well, one of the nice things here is that everything to conctrol the HW
is in the driver, unlike on other systems without all this data that
have to rely on opaque BIOS/ACPI/EFI/... hooks.

> - Footprint: the majority of the stuff in your driver ends up in non-discardable
>   memory, and will be kept around. Since we have this concept of a
>   single zImage for a number of say ARMv7 systems, and since pin
>   controllers can be pretty hard to load from modules, *all* of them
>   may have to be compiled-in. If every driver for every board takes this
>   approach I am afraid it will be end up with a measurable footprint.

Perhaps what we need is a way to build all these drivers as modules, but
still link them into the kernel so they're statically available during
early boot, but allow them to be unloaded like modules after the system
has booted and determined they aren't needed.

This would have the advantage of getting rid of the unused driver code
as well, whereas putting the data into DT only handles ensuring the data
for other SoCs isn't present in the kernel after boot.

-- 
nvpublic

[RFC 1/3] pinctrl: add a driver for NVIDIA Tegra

[Linus Walleij]

On Mon, Dec 12, 2011 at 7:06 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Friday, December 09, 2011 4:56 PM:

>> For me there are two points:
>>
>> - Avoiding clash with kernel maintainers who hate firmware-like tables and
>> ? binary data filling up the kernel. Some certain other guy bearing my
>> ? name comes to mind.
>
> Well, one of the nice things here is that everything to conctrol the HW
> is in the driver, unlike on other systems without all this data that
> have to rely on opaque BIOS/ACPI/EFI/... hooks.

Good point. I will have to play that card if the situation should arise.
Kernel devs also tend to hate stuff they can't control.

>> - Footprint: the majority of the stuff in your driver ends up in non-discardable
>> ? memory, and will be kept around. Since we have this concept of a
>> ? single zImage for a number of say ARMv7 systems, and since pin
>> ? controllers can be pretty hard to load from modules, *all* of them
>> ? may have to be compiled-in. If every driver for every board takes this
>> ? approach I am afraid it will be end up with a measurable footprint.
>
> Perhaps what we need is a way to build all these drivers as modules, but
> still link them into the kernel so they're statically available during
> early boot, but allow them to be unloaded like modules after the system
> has booted and determined they aren't needed.
>
> This would have the advantage of getting rid of the unused driver code
> as well, whereas putting the data into DT only handles ensuring the data
> for other SoCs isn't present in the kernel after boot.

Hm that sounds pretty complicated. But also extremely useful if
we forge ahead on single zImages.

I'd never be able to pull it off thoug :-/

Anyway, we'll deal with this when we run into the problem.

Yours,
Linus Walleij