diff mbox

arm64: dts: Add initial device tree support for Tegra132

Message ID alpine.DEB.2.02.1501161139180.9776@utopia.booyaka.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Walmsley Jan. 16, 2015, 11:45 a.m. UTC
Add an initial device tree file for the Tegra132 SoC.  The DT file is
based on arch/arm/boot/dts/tegra124.dtsi and
arch/arm/boot/dts/tegra114.dtsi, with the following significant
changes:

- Tegra132 uses a Denver CPU cluster rather than an ARMv7 CPU cluster
- Devices are arranged by bus, rather than in a flat topology
- No polling delays have been defined for the thermal zones.  I don't
  believe that this is a property of the SoC hardware, but rather of a
  given use-case.

DT nodes representing IP blocks have generally been labeled according
to the names used in Section 2.1 "System Address Map" of the _Tegra K1
64-Bit Mobile Processor Technical Reference Manual_
(DP-07148-001_ALPHA), with a few exceptions for disambiguation or
abbreviated naming.  Some of the known IP block aliases used by PCB
designers (e.g., "GEN2_I2C" for "I2C2") have been noted in DT node 
comments.

Known future work:

- Add support for the Denver CLUSTER_clocks IP block
- Add support for the CPU thermal zone; now handled by a CCPLEX IP block
- The CPU spin_table enable-method may change to PSCI at some point
- Add support for several missing IP blocks
- Some drivers use unusual address spaces for devices which don't match
  the TRM and/or hardware decode.  At some point these should be
  reconciled.

This patch was originally based on a patch by Allen Martin
<amartin@nvidia.com> and the Tegra124 and Tegra114 DTS files.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Paul Walmsley <pwalmsley@nvidia.com>
Cc: Allen Martin <amartin@nvidia.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-tegra@vger.kernel.org
---
 arch/arm64/boot/dts/Makefile            |   1 +
 arch/arm64/boot/dts/tegra/Makefile      |   3 +
 arch/arm64/boot/dts/tegra/tegra132.dtsi | 997 ++++++++++++++++++++++++++++++++
 3 files changed, 1001 insertions(+)
 create mode 100644 arch/arm64/boot/dts/tegra/Makefile
 create mode 100644 arch/arm64/boot/dts/tegra/tegra132.dtsi

Comments

Mark Rutland Jan. 21, 2015, 4:13 p.m. UTC | #1
Hi Paul,

As mentioned in my reply to the DT list patch [1], there are a couple of
bits I'd like to see cleaned up first, but in the meantime I have some
comments from my first pass of the dtsi below. Some of these may equally
apply to existing dts(i) files.

I see a few undocumented compatible strings (at least when comparing
against mainline). If there are other series or trees I should be
looking at, any pointers would be appreciated. If not, documentation
updates would be nice (checkpatch should complain otherwise).

On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
> 
> Add an initial device tree file for the Tegra132 SoC.  The DT file is
> based on arch/arm/boot/dts/tegra124.dtsi and
> arch/arm/boot/dts/tegra114.dtsi, with the following significant
> changes:
> 
> - Tegra132 uses a Denver CPU cluster rather than an ARMv7 CPU cluster
> - Devices are arranged by bus, rather than in a flat topology
> - No polling delays have been defined for the thermal zones.  I don't
>   believe that this is a property of the SoC hardware, but rather of a
>   given use-case.
> 
> DT nodes representing IP blocks have generally been labeled according
> to the names used in Section 2.1 "System Address Map" of the _Tegra K1
> 64-Bit Mobile Processor Technical Reference Manual_
> (DP-07148-001_ALPHA), with a few exceptions for disambiguation or
> abbreviated naming.  Some of the known IP block aliases used by PCB
> designers (e.g., "GEN2_I2C" for "I2C2") have been noted in DT node
> comments.
> 
> Known future work:
> 
> - Add support for the Denver CLUSTER_clocks IP block
> - Add support for the CPU thermal zone; now handled by a CCPLEX IP block
> - The CPU spin_table enable-method may change to PSCI at some point

That sounds nice. Any idea if/when that would be likely to happen?

> - Add support for several missing IP blocks
> - Some drivers use unusual address spaces for devices which don't match
>   the TRM and/or hardware decode.  At some point these should be
>   reconciled.
> 
> This patch was originally based on a patch by Allen Martin
> <amartin@nvidia.com> and the Tegra124 and Tegra114 DTS files.
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Paul Walmsley <pwalmsley@nvidia.com>
> Cc: Allen Martin <amartin@nvidia.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-tegra@vger.kernel.org
> ---
>  arch/arm64/boot/dts/Makefile            |   1 +
>  arch/arm64/boot/dts/tegra/Makefile      |   3 +
>  arch/arm64/boot/dts/tegra/tegra132.dtsi | 997 ++++++++++++++++++++++++++++++++
>  3 files changed, 1001 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/tegra/Makefile
>  create mode 100644 arch/arm64/boot/dts/tegra/tegra132.dtsi
> 
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index b411251..90f6284 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -3,6 +3,7 @@ dts-dirs += apm
>  dts-dirs += arm
>  dts-dirs += cavium
>  dts-dirs += exynos
> +dts-dirs += tegra

This should probably be 'nvidia' (and 'exynos' probably should have been
'samsung', given all the other directories are vendor names rather than
SoC family names.

> 
>  always         := $(dtb-y)
>  subdir-y       := $(dts-dirs)
> diff --git a/arch/arm64/boot/dts/tegra/Makefile b/arch/arm64/boot/dts/tegra/Makefile
> new file mode 100644
> index 0000000..15dbaa0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/tegra/Makefile
> @@ -0,0 +1,3 @@
> +always          := $(dtb-y)
> +subdir-y        := $(dts-dirs)
> +clean-files     := *.dtb
> diff --git a/arch/arm64/boot/dts/tegra/tegra132.dtsi b/arch/arm64/boot/dts/tegra/tegra132.dtsi
> new file mode 100644
> index 0000000..4b93bfe
> --- /dev/null
> +++ b/arch/arm64/boot/dts/tegra/tegra132.dtsi
> @@ -0,0 +1,997 @@
> +#include <dt-bindings/clock/tegra124-car.h>
> +#include <dt-bindings/gpio/tegra-gpio.h>
> +#include <dt-bindings/memory/tegra124-mc.h>
> +#include <dt-bindings/pinctrl/pinctrl-tegra.h>
> +#include <dt-bindings/pinctrl/pinctrl-tegra-xusb.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/thermal/tegra124-soctherm.h>
> +
> +#include "skeleton.dtsi"

I'd recommend against including skeleton.dtsi, it attempts to be helpful
but IMO it's more problematic than it's worth. Especially given it
expects #size-cells = <1> (which you override below), and therefore
creates a memory node for which the reg entry is too small.

> +
> +/ {
> +       compatible = "nvidia,tegra132";
> +       interrupt-parent = <&gic>;
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +
> +       pcie-controller@0,01003000 {
> +               compatible = "nvidia,tegra132-pcie", "nvidia,tegra124-pcie";

I couldn't spot "nvidia,tegra132-pcie" in mainline anywhere. Does it
differ significantly from "nvidia,tegra124-pcie"?

> +               device_type = "pci";
> +               reg = <0x0 0x01003000 0x0 0x00000800   /* PADS registers */
> +                      0x0 0x01003800 0x0 0x00000800   /* AFI registers */
> +                      0x0 0x02000000 0x0 0x10000000>; /* configuration space */

Nit: for properties which are lists, please bracket each entry, e.g.

reg = <0x0 0x01003000 0x0 0x00000800>,   /* PADS registers */
      <0x0 0x01003800 0x0 0x00000800>,
      <0x0 0x02000000 0x0 0x10000000>;

I appreciate the newlines serve as an obvious delimiter here, but in
single-line cases it really helps, and it would be nice to be
consistent. It looks like most instances in this file already do this
anyway.

> +               reg-names = "pads", "afi", "cs";
> +               interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>, /* controller interrupt */
> +                            <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>; /* MSI interrupt */
> +               interrupt-names = "intr", "msi";
> +
> +               #interrupt-cells = <1>;
> +               interrupt-map-mask = <0 0 0 0>;
> +               interrupt-map = <0 0 0 0 &gic GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> +
> +               bus-range = <0x00 0xff>;
> +               #address-cells = <3>;
> +               #size-cells = <2>;
> +
> +               ranges = <0x82000000 0 0x01000000 0x0 0x01000000 0 0x00001000   /* port 0 configuration space */
> +                         0x82000000 0 0x01001000 0x0 0x01001000 0 0x00001000   /* port 1 configuration space */
> +                         0x81000000 0 0x0        0x0 0x12000000 0 0x00010000   /* downstream I/O (64 KiB) */
> +                         0x82000000 0 0x13000000 0x0 0x13000000 0 0x0d000000   /* non-prefetchable memory (208 MiB) */
> +                         0xc2000000 0 0x20000000 0x0 0x20000000 0 0x20000000>; /* prefetchable memory (512 MiB) */

Same here w.r.t. list bracketing.

> +
> +               clocks = <&tegra_car TEGRA124_CLK_PCIE>,
> +                        <&tegra_car TEGRA124_CLK_AFI>,
> +                        <&tegra_car TEGRA124_CLK_PLL_E>,
> +                        <&tegra_car TEGRA124_CLK_CML0>;
> +               clock-names = "pex", "afi", "pll_e", "cml";
> +               resets = <&tegra_car 70>,
> +                        <&tegra_car 72>,
> +                        <&tegra_car 74>;
> +               reset-names = "pex", "afi", "pcie_x";
> +               status = "disabled";
> +
> +               phys = <&xusb_padctl TEGRA_XUSB_PADCTL_PCIE>;
> +               phy-names = "pcie";
> +
> +               pci@1,0 {
> +                       device_type = "pci";
> +                       assigned-addresses = <0x82000800 0 0x01000000 0 0x1000>;
> +                       reg = <0x000800 0 0 0 0>;
> +                       status = "disabled";
> +
> +                       #address-cells = <3>;
> +                       #size-cells = <2>;
> +                       ranges;

I'm not sure why these three properties are here. Surely this is a
separate address space (so isn't ranges invalid?), and do we define any
sub-nodes anywhere?

> +
> +                       nvidia,num-lanes = <2>;
> +               };
> +
> +               pci@2,0 {
> +                       device_type = "pci";
> +                       assigned-addresses = <0x82001000 0 0x01001000 0 0x1000>;
> +                       reg = <0x001000 0 0 0 0>;
> +                       status = "disabled";
> +
> +                       #address-cells = <3>;
> +                       #size-cells = <2>;
> +                       ranges;
> +

Likewise.

> +                       nvidia,num-lanes = <1>;
> +               };
> +       };

[...]

> +       host1x@0,50000000 {
> +               compatible = "nvidia,tegra132-host1x", "nvidia,tegra124-host1x", "simple-bus";

Regarding simple-bus, are the sub-nodes usable if this didn't probe as
"nvidia,tegra124-host1x" or "nvidia,tegra132-host1x"? Do the subnodes
require anything from this node?

It looks like we expect/require the host1x node to be probed and
initialised before we probe the children. Which would imply the
simple-bus annotation is wrong.

> +               reg = <0x0 0x50000000 0x0 0x00034000>;
> +               interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */
> +                            <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */
> +               clocks = <&tegra_car TEGRA124_CLK_HOST1X>;
> +               resets = <&tegra_car 28>;
> +               reset-names = "host1x";
> +
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +
> +               ranges = <0 0x54000000 0 0x54000000 0 0x01000000>;
> +
> +               dc@0,54200000 {
> +                       compatible = "nvidia,tegra132-dc", "nvidia,tegra124-dc";
> +                       reg = <0x0 0x54200000 0x0 0x00040000>;
> +                       interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&tegra_car TEGRA124_CLK_DISP1>,
> +                                <&tegra_car TEGRA124_CLK_PLL_P>;
> +                       clock-names = "dc", "parent";
> +                       resets = <&tegra_car 27>;
> +                       reset-names = "dc";
> +
> +                       iommus = <&mc TEGRA_SWGROUP_DC>;
> +
> +                       nvidia,head = <0>;
> +               };
> +
> +               dc@0,54240000 {
> +                       compatible = "nvidia,tegra132-dc", "nvidia,tegra124-dc";
> +                       reg = <0x0 0x54240000 0x0 0x00040000>;
> +                       interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&tegra_car TEGRA124_CLK_DISP2>,
> +                                <&tegra_car TEGRA124_CLK_PLL_P>;
> +                       clock-names = "dc", "parent";
> +                       resets = <&tegra_car 26>;
> +                       reset-names = "dc";
> +
> +                       iommus = <&mc TEGRA_SWGROUP_DCB>;
> +
> +                       nvidia,head = <1>;
> +               };
> +
> +               hdmi@0,54280000 {
> +                       compatible = "nvidia,tegra132-hdmi", "nvidia,tegra124-hdmi";
> +                       reg = <0x0 0x54280000 0x0 0x00040000>;
> +                       interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&tegra_car TEGRA124_CLK_HDMI>,
> +                                <&tegra_car TEGRA124_CLK_PLL_D2_OUT0>;
> +                       clock-names = "hdmi", "parent";
> +                       resets = <&tegra_car 51>;
> +                       reset-names = "hdmi";
> +                       status = "disabled";
> +               };
> +
> +               sor@0,54540000 {
> +                       compatible = "nvidia,tegra132-sor", "nvidia,tegra124-sor";
> +                       reg = <0x0 0x54540000 0x0 0x00040000>;
> +                       interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&tegra_car TEGRA124_CLK_SOR0>,
> +                                <&tegra_car TEGRA124_CLK_PLL_D_OUT0>,
> +                                <&tegra_car TEGRA124_CLK_PLL_DP>,
> +                                <&tegra_car TEGRA124_CLK_CLK_M>;
> +                       clock-names = "sor", "parent", "dp", "safe";
> +                       resets = <&tegra_car 182>;
> +                       reset-names = "sor";
> +                       status = "disabled";
> +               };
> +
> +               dpaux: dpaux@0,545c0000 {
> +                       compatible = "nvidia,tegra132-dpaux", "nvidia,tegra124-dpaux";
> +                       reg = <0x0 0x545c0000 0x0 0x00040000>;
> +                       interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
> +                                <&tegra_car TEGRA124_CLK_PLL_DP>;
> +                       clock-names = "dpaux", "parent";
> +                       resets = <&tegra_car 181>;
> +                       reset-names = "dpaux";
> +                       status = "disabled";
> +               };
> +       };
> +

[...]

> +       gic: interrupt-controller@0,50041000 {
> +               compatible = "arm,cortex-a15-gic";
> +               #interrupt-cells = <3>;
> +               interrupt-controller;
> +               reg = <0x0 0x50041000 0x0 0x1000>,
> +                     <0x0 0x50042000 0x0 0x1000>,
> +                     <0x0 0x50044000 0x0 0x2000>,
> +                     <0x0 0x50046000 0x0 0x2000>;
> +               interrupts = <GIC_PPI 9
> +                       (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> +       };

/cpus (below) only has two CPUs listed, so that mask doensn't look
right.

[...]

> +       ppsb: ppsb@0,60000000 {
> +               compatible = "simple-bus";
> +               reg = <0x0 0x60000000 0x0 0x01000000>;
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;

This reg overlaps the devices described below, and this node is only
described as a simple-bus (for whch reg is not a valid property). That
doesn't look right to me.

If you want to describe that the bus covers a particular range, you can
encode that in the ranges property.

[...]

> +               ahb_gizmo: ahb-gizmo@0,6000c004 {
> +                       compatible = "nvidia,tegra132-ahb", "nvidia,tegra124-ahb", "nvidia,tegra30-ahb";
> +                       /*
> +                        * This IP block actually starts at 0x6000c000,
> +                        * but all of the register offsets in the driver
> +                        * have 0x4 subtracted from them.  So handle
> +                        * it this way until the driver is fixed.
> +                        */
> +                       reg = <0x0 0x6000c004 0x0 0x14d>;
> +               };

That doesn't sound great. Can we not fix the driver and limit that
mistake to existing DTBs?

[...]

> +       apb: apb@0,70000000 {
> +               compatible = "simple-bus";
> +               reg = <0x0 0x70000000 0x0 0x01000000>;

As with previously, a reg property doesn't make sense for a block that's
only a "simple-bus"

> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;
> +
> +               apbmisc: apbmisc@0,70000800 {
> +                       compatible = "nvidia,tegra132-apbmisc", "nvidia,tegra124-apbmisc", "nvidia,tegra20-apbmisc";
> +                       reg = <0x0 0x70000800 0x0 0x64>,   /* Chip revision */
> +                             <0x0 0x7000E864 0x0 0x04>;   /* Strapping options */
> +               };
> +
> +               pinmux: pinmux@0,70000868 {
> +                       compatible = "nvidia,tegra132-pinmux", "nvidia,tegra124-pinmux";
> +                       reg = <0x0 0x70000868 0x0 0x164>,  /* Pad control registers */
> +                             <0x0 0x70003000 0x0 0x434>;  /* Mux registers */
> +               };
> +
> +               /*
> +                * There are two serial drivers: an 8250 based simple
> +                * serial driver and an APB DMA based serial driver
> +                * for higher baudrate and performance. To enable the
> +                * 8250 based driver, the compatible string is
> +                * "nvidia,tegra132-uart", "nvidia,tegra124-uart",
> +                * "nvidia,tegra20-uart" and to enable the APB DMA
> +                * based serial driver, the compatible string is
> +                * "nvidia,tegra132-hsuart", "nvidia,tegra124-hsuart",
> +                * "nvidia,tegra30-hsuart".
> +                */

Is there any reason to continue with this split?

Surely if the only difference is DMA, the presence of dmas and dma-names
should be sufficient to get the driver to do the right thing, and if you
need to disable DMA for debugging that could be a command-line option.

Does the binding and/or driver support aliases so you can get consistent
numbering? It would be nice to have.

Do these UARTs work with earlycon?

It would be nice to have a /chosen/stdout-path (ideally with rate) so as
to get output consistently without command line options being required.

> +               uarta: serial@0,70006000 {
> +                       compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
> +                       reg = <0x0 0x70006000 0x0 0x40>;
> +                       reg-shift = <2>;
> +                       interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&tegra_car TEGRA124_CLK_UARTA>;
> +                       resets = <&tegra_car 6>;
> +                       reset-names = "serial";
> +                       dmas = <&apbdma 8>, <&apbdma 8>;
> +                       dma-names = "rx", "tx";
> +                       status = "disabled";
> +               };
> +
> +               uartb: serial@0,70006040 {
> +                       compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
> +                       reg = <0x0 0x70006040 0x0 0x40>;
> +                       reg-shift = <2>;
> +                       interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&tegra_car TEGRA124_CLK_UARTB>;
> +                       resets = <&tegra_car 7>;
> +                       reset-names = "serial";
> +                       dmas = <&apbdma 9>, <&apbdma 9>;
> +                       dma-names = "rx", "tx";
> +                       status = "disabled";
> +               };
> +
> +               uartc: serial@0,70006200 {
> +                       compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
> +                       reg = <0x0 0x70006200 0x0 0x40>;
> +                       reg-shift = <2>;
> +                       interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&tegra_car TEGRA124_CLK_UARTC>;
> +                       resets = <&tegra_car 55>;
> +                       reset-names = "serial";
> +                       dmas = <&apbdma 10>, <&apbdma 10>;
> +                       dma-names = "rx", "tx";
> +                       status = "disabled";
> +               };
> +
> +               uartd: serial@0,70006300 {
> +                       compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
> +                       reg = <0x0 0x70006300 0x0 0x40>;
> +                       reg-shift = <2>;
> +                       interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&tegra_car TEGRA124_CLK_UARTD>;
> +                       resets = <&tegra_car 65>;
> +                       reset-names = "serial";
> +                       dmas = <&apbdma 19>, <&apbdma 19>;
> +                       dma-names = "rx", "tx";
> +                       status = "disabled";
> +               };

[...]

> +       ahb_a2: ahb@0,7c000000 {
> +               compatible = "simple-bus";
> +               reg = <0x0 0x7c000000 0x0 0x02000000>;
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;

Again, this doesn't look right for a plain simple-bus.

[...]

> +       cpus {
> +               #address-cells = <2>;
> +               #size-cells = <0>;
> +
> +               cpu@0 {
> +                       device_type = "cpu";
> +                       compatible = "nvidia,denver", "arm,armv8";
> +                       reg = <0x0 0x0>;
> +                       enable-method = "spin-table";
> +                       cpu-release-addr = <0x0 0x80000008>;
> +               };
> +
> +               cpu@1 {
> +                       device_type = "cpu";
> +                       compatible = "nvidia,denver", "arm,armv8";
> +                       reg = <0x0 0x1>;
> +                       enable-method = "spin-table";
> +                       cpu-release-addr = <0x0 0x80000008>;
> +               };
> +       };

It would be nice if this were near the top, as in other dts files.

It's a shame these share the same release address, though I guess that
doesn't matter too much if PSCI is forthcoming.

I didn't spot a memory node or /memreserve/ entries. Is the memory used
here for the spin-table guaranteed to be protected?

If the enable-method might change, it's probably best to leave this to
the bootloader to fill in, or to have it in the board files if the boot
loader isn't that clever (perhaps via a dtsi).

> +
> +       thermal-zones {
> +               /* XXX T132 CPU thermal zone - still TBD */
> +

What is still TBD here?

> +               mem {
> +                       thermal-sensors =
> +                               <&soc_therm TEGRA124_SOCTHERM_SENSOR_MEM>;
> +               };
> +
> +               gpu {
> +                       thermal-sensors =
> +                               <&soc_therm TEGRA124_SOCTHERM_SENSOR_GPU>;
> +               };
> +
> +               pllx {
> +                       thermal-sensors =
> +                               <&soc_therm TEGRA124_SOCTHERM_SENSOR_PLLX>;
> +               };
> +       };
> +
> +       timer {
> +               compatible = "arm,armv8-timer";
> +               interrupts = <GIC_PPI 13
> +                               (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                            <GIC_PPI 14
> +                               (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                            <GIC_PPI 11
> +                               (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                            <GIC_PPI 10
> +                               (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;

Those masks look wrong given there were two CPUs described above.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/317010.html
Paul Walmsley Jan. 23, 2015, 11:31 a.m. UTC | #2
+ Arto, Terje for comments on the host1x section
+ Stephen Warren for comments on the serial DT data

Hi Mark,

thanks for the review.

On Wed, 21 Jan 2015, Mark Rutland wrote:

> As mentioned in my reply to the DT list patch [1], there are a couple of
> bits I'd like to see cleaned up first, but in the meantime I have some
> comments from my first pass of the dtsi below. Some of these may equally
> apply to existing dts(i) files.
> 
> I see a few undocumented compatible strings (at least when comparing 
> against mainline). If there are other series or trees I should be 
> looking at, any pointers would be appreciated. If not, documentation 
> updates would be nice (checkpatch should complain otherwise).

(discussed in the tegra132-pcie comments below)

> 
> On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
> > 
> > Add an initial device tree file for the Tegra132 SoC.  The DT file is
> > based on arch/arm/boot/dts/tegra124.dtsi and
> > arch/arm/boot/dts/tegra114.dtsi, with the following significant
> > changes:
> > 
> > - Tegra132 uses a Denver CPU cluster rather than an ARMv7 CPU cluster
> > - Devices are arranged by bus, rather than in a flat topology
> > - No polling delays have been defined for the thermal zones.  I don't
> >   believe that this is a property of the SoC hardware, but rather of a
> >   given use-case.
> > 
> > DT nodes representing IP blocks have generally been labeled according
> > to the names used in Section 2.1 "System Address Map" of the _Tegra K1
> > 64-Bit Mobile Processor Technical Reference Manual_
> > (DP-07148-001_ALPHA), with a few exceptions for disambiguation or
> > abbreviated naming.  Some of the known IP block aliases used by PCB
> > designers (e.g., "GEN2_I2C" for "I2C2") have been noted in DT node
> > comments.
> > 
> > Known future work:
> > 
> > - Add support for the Denver CLUSTER_clocks IP block
> > - Add support for the CPU thermal zone; now handled by a CCPLEX IP block
> > - The CPU spin_table enable-method may change to PSCI at some point
> 
> That sounds nice. Any idea if/when that would be likely to happen?

The PSCI aspect?  Unfortunately not at the moment due to the secure 
firmware dependency.

> 
> > - Add support for several missing IP blocks
> > - Some drivers use unusual address spaces for devices which don't match
> >   the TRM and/or hardware decode.  At some point these should be
> >   reconciled.
> > 
> > This patch was originally based on a patch by Allen Martin
> > <amartin@nvidia.com> and the Tegra124 and Tegra114 DTS files.
> > 
> > Signed-off-by: Paul Walmsley <paul@pwsan.com>
> > Cc: Paul Walmsley <pwalmsley@nvidia.com>
> > Cc: Allen Martin <amartin@nvidia.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: Kumar Gala <galak@codeaurora.org>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-tegra@vger.kernel.org
> > ---
> >  arch/arm64/boot/dts/Makefile            |   1 +
> >  arch/arm64/boot/dts/tegra/Makefile      |   3 +
> >  arch/arm64/boot/dts/tegra/tegra132.dtsi | 997 ++++++++++++++++++++++++++++++++
> >  3 files changed, 1001 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/tegra/Makefile
> >  create mode 100644 arch/arm64/boot/dts/tegra/tegra132.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> > index b411251..90f6284 100644
> > --- a/arch/arm64/boot/dts/Makefile
> > +++ b/arch/arm64/boot/dts/Makefile
> > @@ -3,6 +3,7 @@ dts-dirs += apm
> >  dts-dirs += arm
> >  dts-dirs += cavium
> >  dts-dirs += exynos
> > +dts-dirs += tegra
> 
> This should probably be 'nvidia' (and 'exynos' probably should have been
> 'samsung', given all the other directories are vendor names rather than
> SoC family names.

OK, will change.

> > 
> >  always         := $(dtb-y)
> >  subdir-y       := $(dts-dirs)
> > diff --git a/arch/arm64/boot/dts/tegra/Makefile b/arch/arm64/boot/dts/tegra/Makefile
> > new file mode 100644
> > index 0000000..15dbaa0
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/tegra/Makefile
> > @@ -0,0 +1,3 @@
> > +always          := $(dtb-y)
> > +subdir-y        := $(dts-dirs)
> > +clean-files     := *.dtb
> > diff --git a/arch/arm64/boot/dts/tegra/tegra132.dtsi b/arch/arm64/boot/dts/tegra/tegra132.dtsi
> > new file mode 100644
> > index 0000000..4b93bfe
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/tegra/tegra132.dtsi
> > @@ -0,0 +1,997 @@
> > +#include <dt-bindings/clock/tegra124-car.h>
> > +#include <dt-bindings/gpio/tegra-gpio.h>
> > +#include <dt-bindings/memory/tegra124-mc.h>
> > +#include <dt-bindings/pinctrl/pinctrl-tegra.h>
> > +#include <dt-bindings/pinctrl/pinctrl-tegra-xusb.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/thermal/tegra124-soctherm.h>
> > +
> > +#include "skeleton.dtsi"
> 
> I'd recommend against including skeleton.dtsi, it attempts to be helpful
> but IMO it's more problematic than it's worth. Especially given it
> expects #size-cells = <1> (which you override below), and therefore
> creates a memory node for which the reg entry is too small.

OK, will drop.

> 
> > +
> > +/ {
> > +       compatible = "nvidia,tegra132";
> > +       interrupt-parent = <&gic>;
> > +       #address-cells = <2>;
> > +       #size-cells = <2>;
> > +
> > +       pcie-controller@0,01003000 {
> > +               compatible = "nvidia,tegra132-pcie", "nvidia,tegra124-pcie";
> 
> I couldn't spot "nvidia,tegra132-pcie" in mainline anywhere. Does it
> differ significantly from "nvidia,tegra124-pcie"?

I'm not sure if the T132 PCIe driver will need any T132-specific 
workarounds.  I just followed what seems to be at least one DT practice 
discussed in the past of adding SoC-specific variants for the IP blocks in 
the event that a SoC-specific driver change is needed in the future.  I'm 
probably out-of-date on the current doctrine here, though.  

Anyway, I have no problem with dropping the tegra132- variants, except in 
cases where changes are specifically known to be needed, although this 
seems likely to strengthen the binding between DT data and kernel 
versions.  Let me know.

> > +               device_type = "pci";
> > +               reg = <0x0 0x01003000 0x0 0x00000800   /* PADS registers */
> > +                      0x0 0x01003800 0x0 0x00000800   /* AFI registers */
> > +                      0x0 0x02000000 0x0 0x10000000>; /* configuration space */
> 
> Nit: for properties which are lists, please bracket each entry, e.g.
> 
> reg = <0x0 0x01003000 0x0 0x00000800>,   /* PADS registers */
>       <0x0 0x01003800 0x0 0x00000800>,
>       <0x0 0x02000000 0x0 0x10000000>;
> 
> I appreciate the newlines serve as an obvious delimiter here, but in
> single-line cases it really helps, and it would be nice to be
> consistent. It looks like most instances in this file already do this
> anyway.

OK will do.

> 
> > +               reg-names = "pads", "afi", "cs";
> > +               interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>, /* controller interrupt */
> > +                            <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>; /* MSI interrupt */
> > +               interrupt-names = "intr", "msi";
> > +
> > +               #interrupt-cells = <1>;
> > +               interrupt-map-mask = <0 0 0 0>;
> > +               interrupt-map = <0 0 0 0 &gic GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +               bus-range = <0x00 0xff>;
> > +               #address-cells = <3>;
> > +               #size-cells = <2>;
> > +
> > +               ranges = <0x82000000 0 0x01000000 0x0 0x01000000 0 0x00001000   /* port 0 configuration space */
> > +                         0x82000000 0 0x01001000 0x0 0x01001000 0 0x00001000   /* port 1 configuration space */
> > +                         0x81000000 0 0x0        0x0 0x12000000 0 0x00010000   /* downstream I/O (64 KiB) */
> > +                         0x82000000 0 0x13000000 0x0 0x13000000 0 0x0d000000   /* non-prefetchable memory (208 MiB) */
> > +                         0xc2000000 0 0x20000000 0x0 0x20000000 0 0x20000000>; /* prefetchable memory (512 MiB) */
> 
> Same here w.r.t. list bracketing.

OK

> 
> > +
> > +               clocks = <&tegra_car TEGRA124_CLK_PCIE>,
> > +                        <&tegra_car TEGRA124_CLK_AFI>,
> > +                        <&tegra_car TEGRA124_CLK_PLL_E>,
> > +                        <&tegra_car TEGRA124_CLK_CML0>;
> > +               clock-names = "pex", "afi", "pll_e", "cml";
> > +               resets = <&tegra_car 70>,
> > +                        <&tegra_car 72>,
> > +                        <&tegra_car 74>;
> > +               reset-names = "pex", "afi", "pcie_x";
> > +               status = "disabled";
> > +
> > +               phys = <&xusb_padctl TEGRA_XUSB_PADCTL_PCIE>;
> > +               phy-names = "pcie";
> > +
> > +               pci@1,0 {
> > +                       device_type = "pci";
> > +                       assigned-addresses = <0x82000800 0 0x01000000 0 0x1000>;
> > +                       reg = <0x000800 0 0 0 0>;
> > +                       status = "disabled";
> > +
> > +                       #address-cells = <3>;
> > +                       #size-cells = <2>;
> > +                       ranges;
> 
> I'm not sure why these three properties are here. Surely this is a
> separate address space (so isn't ranges invalid?), and do we define any
> sub-nodes anywhere?

Hmm not sure.  This was originally copied from 
arch/arm/boot/dts/tegra124.dtsi.  Will plan to drop them for now, and then 
if the properties (or ones like them) turn out to be needed in the future, 
someone else can deal with it :-)

> 
> > +
> > +                       nvidia,num-lanes = <2>;
> > +               };
> > +
> > +               pci@2,0 {
> > +                       device_type = "pci";
> > +                       assigned-addresses = <0x82001000 0 0x01001000 0 0x1000>;
> > +                       reg = <0x001000 0 0 0 0>;
> > +                       status = "disabled";
> > +
> > +                       #address-cells = <3>;
> > +                       #size-cells = <2>;
> > +                       ranges;
> > +
> 
> Likewise.

OK

> 
> > +                       nvidia,num-lanes = <1>;
> > +               };
> > +       };
> 
> [...]
> 
> > +       host1x@0,50000000 {
> > +               compatible = "nvidia,tegra132-host1x", "nvidia,tegra124-host1x", "simple-bus";
> 
> Regarding simple-bus, are the sub-nodes usable if this didn't probe as
> "nvidia,tegra124-host1x" or "nvidia,tegra132-host1x"?
> Do the subnodes require anything from this node?
> 
> It looks like we expect/require the host1x node to be probed and
> initialised before we probe the children. Which would imply the
> simple-bus annotation is wrong.

Haven't tried booting with just simple-bus here.  I believe these devices 
are accessible without the involvement of host1x.  In other words, host1x 
is not a bus; I believe it might be most accurately described as a type of 
DMA controller.  So in theory it should be possible for the CPU complex to 
read and write to these devices directly without the involvement of 
host1x, although I believe NVIDIA discourages this.

Under the theory that DT data should be hardware-specific, not 
software-specific, in an ideal world I think we would list these devices 
outside the embrace of the host1x node.  However the existing Tegra124 DT 
file listed them together, and I am unsure whether that is required for 
the host1x code to function correctly.

Arto may be able to comment further.

> 
> > +               reg = <0x0 0x50000000 0x0 0x00034000>;
> > +               interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */
> > +                            <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */
> > +               clocks = <&tegra_car TEGRA124_CLK_HOST1X>;
> > +               resets = <&tegra_car 28>;
> > +               reset-names = "host1x";
> > +
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +
> > +               ranges = <0 0x54000000 0 0x54000000 0 0x01000000>;
> > +
> > +               dc@0,54200000 {
> > +                       compatible = "nvidia,tegra132-dc", "nvidia,tegra124-dc";
> > +                       reg = <0x0 0x54200000 0x0 0x00040000>;
> > +                       interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&tegra_car TEGRA124_CLK_DISP1>,
> > +                                <&tegra_car TEGRA124_CLK_PLL_P>;
> > +                       clock-names = "dc", "parent";
> > +                       resets = <&tegra_car 27>;
> > +                       reset-names = "dc";
> > +
> > +                       iommus = <&mc TEGRA_SWGROUP_DC>;
> > +
> > +                       nvidia,head = <0>;
> > +               };
> > +
> > +               dc@0,54240000 {
> > +                       compatible = "nvidia,tegra132-dc", "nvidia,tegra124-dc";
> > +                       reg = <0x0 0x54240000 0x0 0x00040000>;
> > +                       interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&tegra_car TEGRA124_CLK_DISP2>,
> > +                                <&tegra_car TEGRA124_CLK_PLL_P>;
> > +                       clock-names = "dc", "parent";
> > +                       resets = <&tegra_car 26>;
> > +                       reset-names = "dc";
> > +
> > +                       iommus = <&mc TEGRA_SWGROUP_DCB>;
> > +
> > +                       nvidia,head = <1>;
> > +               };
> > +
> > +               hdmi@0,54280000 {
> > +                       compatible = "nvidia,tegra132-hdmi", "nvidia,tegra124-hdmi";
> > +                       reg = <0x0 0x54280000 0x0 0x00040000>;
> > +                       interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&tegra_car TEGRA124_CLK_HDMI>,
> > +                                <&tegra_car TEGRA124_CLK_PLL_D2_OUT0>;
> > +                       clock-names = "hdmi", "parent";
> > +                       resets = <&tegra_car 51>;
> > +                       reset-names = "hdmi";
> > +                       status = "disabled";
> > +               };
> > +
> > +               sor@0,54540000 {
> > +                       compatible = "nvidia,tegra132-sor", "nvidia,tegra124-sor";
> > +                       reg = <0x0 0x54540000 0x0 0x00040000>;
> > +                       interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&tegra_car TEGRA124_CLK_SOR0>,
> > +                                <&tegra_car TEGRA124_CLK_PLL_D_OUT0>,
> > +                                <&tegra_car TEGRA124_CLK_PLL_DP>,
> > +                                <&tegra_car TEGRA124_CLK_CLK_M>;
> > +                       clock-names = "sor", "parent", "dp", "safe";
> > +                       resets = <&tegra_car 182>;
> > +                       reset-names = "sor";
> > +                       status = "disabled";
> > +               };
> > +
> > +               dpaux: dpaux@0,545c0000 {
> > +                       compatible = "nvidia,tegra132-dpaux", "nvidia,tegra124-dpaux";
> > +                       reg = <0x0 0x545c0000 0x0 0x00040000>;
> > +                       interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
> > +                                <&tegra_car TEGRA124_CLK_PLL_DP>;
> > +                       clock-names = "dpaux", "parent";
> > +                       resets = <&tegra_car 181>;
> > +                       reset-names = "dpaux";
> > +                       status = "disabled";
> > +               };
> > +       };
> > +
> 
> [...]
> 
> > +       gic: interrupt-controller@0,50041000 {
> > +               compatible = "arm,cortex-a15-gic";
> > +               #interrupt-cells = <3>;
> > +               interrupt-controller;
> > +               reg = <0x0 0x50041000 0x0 0x1000>,
> > +                     <0x0 0x50042000 0x0 0x1000>,
> > +                     <0x0 0x50044000 0x0 0x2000>,
> > +                     <0x0 0x50046000 0x0 0x2000>;
> > +               interrupts = <GIC_PPI 9
> > +                       (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> > +       };
> 
> /cpus (below) only has two CPUs listed, so that mask doensn't look
> right.

OK will fix

> 
> [...]
> 
> > +       ppsb: ppsb@0,60000000 {
> > +               compatible = "simple-bus";
> > +               reg = <0x0 0x60000000 0x0 0x01000000>;
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               ranges;
> 
> This reg overlaps the devices described below, and this node is only
> described as a simple-bus (for whch reg is not a valid property). That
> doesn't look right to me.
> 
> If you want to describe that the bus covers a particular range, you can
> encode that in the ranges property.

OK will fix.

> 
> [...]
> 
> > +               ahb_gizmo: ahb-gizmo@0,6000c004 {
> > +                       compatible = "nvidia,tegra132-ahb", "nvidia,tegra124-ahb", "nvidia,tegra30-ahb";
> > +                       /*
> > +                        * This IP block actually starts at 0x6000c000,
> > +                        * but all of the register offsets in the driver
> > +                        * have 0x4 subtracted from them.  So handle
> > +                        * it this way until the driver is fixed.
> > +                        */
> > +                       reg = <0x0 0x6000c004 0x0 0x14d>;
> > +               };
> 
> That doesn't sound great. Can we not fix the driver and limit that
> mistake to existing DTBs?

We can do it that way if you think it's important to fix now.  Have been 
trying to avoid gating our initial ARM64 support on fixing all of our 
hardware misalignment problems accumulated over the years, but if you 
think it's a blocker, it can be done.  

> 
> [...]
> 
> > +       apb: apb@0,70000000 {
> > +               compatible = "simple-bus";
> > +               reg = <0x0 0x70000000 0x0 0x01000000>;
> 
> As with previously, a reg property doesn't make sense for a block that's
> only a "simple-bus"

OK will fix.

> 
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               ranges;
> > +
> > +               apbmisc: apbmisc@0,70000800 {
> > +                       compatible = "nvidia,tegra132-apbmisc", "nvidia,tegra124-apbmisc", "nvidia,tegra20-apbmisc";
> > +                       reg = <0x0 0x70000800 0x0 0x64>,   /* Chip revision */
> > +                             <0x0 0x7000E864 0x0 0x04>;   /* Strapping options */
> > +               };
> > +
> > +               pinmux: pinmux@0,70000868 {
> > +                       compatible = "nvidia,tegra132-pinmux", "nvidia,tegra124-pinmux";
> > +                       reg = <0x0 0x70000868 0x0 0x164>,  /* Pad control registers */
> > +                             <0x0 0x70003000 0x0 0x434>;  /* Mux registers */
> > +               };
> > +
> > +               /*
> > +                * There are two serial drivers: an 8250 based simple
> > +                * serial driver and an APB DMA based serial driver
> > +                * for higher baudrate and performance. To enable the
> > +                * 8250 based driver, the compatible string is
> > +                * "nvidia,tegra132-uart", "nvidia,tegra124-uart",
> > +                * "nvidia,tegra20-uart" and to enable the APB DMA
> > +                * based serial driver, the compatible string is
> > +                * "nvidia,tegra132-hsuart", "nvidia,tegra124-hsuart",
> > +                * "nvidia,tegra30-hsuart".
> > +                */
> 
> Is there any reason to continue with this split?
> 
> Surely if the only difference is DMA, the presence of dmas and dma-names
> should be sufficient to get the driver to do the right thing, and if you
> need to disable DMA for debugging that could be a command-line option.
> 
> Does the binding and/or driver support aliases so you can get consistent
> numbering? It would be nice to have.
> 
> Do these UARTs work with earlycon?
> 
> It would be nice to have a /chosen/stdout-path (ideally with rate) so as
> to get output consistently without command line options being required.

Stephen Warren might be the best person to directly respond to these 
issues.  This is all legacy DT data from previous Tegra SoCs.

> 
> > +               uarta: serial@0,70006000 {
> > +                       compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
> > +                       reg = <0x0 0x70006000 0x0 0x40>;
> > +                       reg-shift = <2>;
> > +                       interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&tegra_car TEGRA124_CLK_UARTA>;
> > +                       resets = <&tegra_car 6>;
> > +                       reset-names = "serial";
> > +                       dmas = <&apbdma 8>, <&apbdma 8>;
> > +                       dma-names = "rx", "tx";
> > +                       status = "disabled";
> > +               };
> > +
> > +               uartb: serial@0,70006040 {
> > +                       compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
> > +                       reg = <0x0 0x70006040 0x0 0x40>;
> > +                       reg-shift = <2>;
> > +                       interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&tegra_car TEGRA124_CLK_UARTB>;
> > +                       resets = <&tegra_car 7>;
> > +                       reset-names = "serial";
> > +                       dmas = <&apbdma 9>, <&apbdma 9>;
> > +                       dma-names = "rx", "tx";
> > +                       status = "disabled";
> > +               };
> > +
> > +               uartc: serial@0,70006200 {
> > +                       compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
> > +                       reg = <0x0 0x70006200 0x0 0x40>;
> > +                       reg-shift = <2>;
> > +                       interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&tegra_car TEGRA124_CLK_UARTC>;
> > +                       resets = <&tegra_car 55>;
> > +                       reset-names = "serial";
> > +                       dmas = <&apbdma 10>, <&apbdma 10>;
> > +                       dma-names = "rx", "tx";
> > +                       status = "disabled";
> > +               };
> > +
> > +               uartd: serial@0,70006300 {
> > +                       compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
> > +                       reg = <0x0 0x70006300 0x0 0x40>;
> > +                       reg-shift = <2>;
> > +                       interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> > +                       clocks = <&tegra_car TEGRA124_CLK_UARTD>;
> > +                       resets = <&tegra_car 65>;
> > +                       reset-names = "serial";
> > +                       dmas = <&apbdma 19>, <&apbdma 19>;
> > +                       dma-names = "rx", "tx";
> > +                       status = "disabled";
> > +               };
> 
> [...]
> 
> > +       ahb_a2: ahb@0,7c000000 {
> > +               compatible = "simple-bus";
> > +               reg = <0x0 0x7c000000 0x0 0x02000000>;
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               ranges;
> 
> Again, this doesn't look right for a plain simple-bus.

OK will fix.

> 
> [...]
> 
> > +       cpus {
> > +               #address-cells = <2>;
> > +               #size-cells = <0>;
> > +
> > +               cpu@0 {
> > +                       device_type = "cpu";
> > +                       compatible = "nvidia,denver", "arm,armv8";
> > +                       reg = <0x0 0x0>;
> > +                       enable-method = "spin-table";
> > +                       cpu-release-addr = <0x0 0x80000008>;
> > +               };
> > +
> > +               cpu@1 {
> > +                       device_type = "cpu";
> > +                       compatible = "nvidia,denver", "arm,armv8";
> > +                       reg = <0x0 0x1>;
> > +                       enable-method = "spin-table";
> > +                       cpu-release-addr = <0x0 0x80000008>;
> > +               };
> > +       };
> 
> It would be nice if this were near the top, as in other dts files.

OK will move.

> 
> It's a shame these share the same release address, though I guess that
> doesn't matter too much if PSCI is forthcoming.
> 
> I didn't spot a memory node or /memreserve/ entries. Is the memory used
> here for the spin-table guaranteed to be protected?
> 
> If the enable-method might change, it's probably best to leave this to
> the bootloader to fill in, or to have it in the board files if the boot
> loader isn't that clever (perhaps via a dtsi).

OK will drop the enable-method for the time being.

> 
> > +
> > +       thermal-zones {
> > +               /* XXX T132 CPU thermal zone - still TBD */
> > +
> 
> What is still TBD here?

The T132 CPU thermal zone thermal sensor data.  Specifically (and I hope 
this comes as no surprise), there's some power/thermal management data 
missing from this file that is intended to be added later.  The 
placeholder is simply to indicate that.

> 
> > +               mem {
> > +                       thermal-sensors =
> > +                               <&soc_therm TEGRA124_SOCTHERM_SENSOR_MEM>;
> > +               };
> > +
> > +               gpu {
> > +                       thermal-sensors =
> > +                               <&soc_therm TEGRA124_SOCTHERM_SENSOR_GPU>;
> > +               };
> > +
> > +               pllx {
> > +                       thermal-sensors =
> > +                               <&soc_therm TEGRA124_SOCTHERM_SENSOR_PLLX>;
> > +               };
> > +       };
> > +
> > +       timer {
> > +               compatible = "arm,armv8-timer";
> > +               interrupts = <GIC_PPI 13
> > +                               (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > +                            <GIC_PPI 14
> > +                               (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > +                            <GIC_PPI 11
> > +                               (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> > +                            <GIC_PPI 10
> > +                               (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> 
> Those masks look wrong given there were two CPUs described above.

OK will fix.

> 
> Thanks,
> Mark.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/317010.html
> 


- Paul
Mark Rutland Jan. 23, 2015, 11:44 a.m. UTC | #3
On Fri, Jan 23, 2015 at 11:31:24AM +0000, Paul Walmsley wrote:
> + Arto, Terje for comments on the host1x section
> + Stephen Warren for comments on the serial DT data
> 
> Hi Mark,
> 
> thanks for the review.
> 
> On Wed, 21 Jan 2015, Mark Rutland wrote:
> 
> > As mentioned in my reply to the DT list patch [1], there are a couple of
> > bits I'd like to see cleaned up first, but in the meantime I have some
> > comments from my first pass of the dtsi below. Some of these may equally
> > apply to existing dts(i) files.
> >
> > I see a few undocumented compatible strings (at least when comparing
> > against mainline). If there are other series or trees I should be
> > looking at, any pointers would be appreciated. If not, documentation
> > updates would be nice (checkpatch should complain otherwise).
> 
> (discussed in the tegra132-pcie comments below)
> 
> >
> > On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
> > >
> > > Add an initial device tree file for the Tegra132 SoC.  The DT file is
> > > based on arch/arm/boot/dts/tegra124.dtsi and
> > > arch/arm/boot/dts/tegra114.dtsi, with the following significant
> > > changes:
> > >
> > > - Tegra132 uses a Denver CPU cluster rather than an ARMv7 CPU cluster
> > > - Devices are arranged by bus, rather than in a flat topology
> > > - No polling delays have been defined for the thermal zones.  I don't
> > >   believe that this is a property of the SoC hardware, but rather of a
> > >   given use-case.
> > >
> > > DT nodes representing IP blocks have generally been labeled according
> > > to the names used in Section 2.1 "System Address Map" of the _Tegra K1
> > > 64-Bit Mobile Processor Technical Reference Manual_
> > > (DP-07148-001_ALPHA), with a few exceptions for disambiguation or
> > > abbreviated naming.  Some of the known IP block aliases used by PCB
> > > designers (e.g., "GEN2_I2C" for "I2C2") have been noted in DT node
> > > comments.
> > >
> > > Known future work:
> > >
> > > - Add support for the Denver CLUSTER_clocks IP block
> > > - Add support for the CPU thermal zone; now handled by a CCPLEX IP block
> > > - The CPU spin_table enable-method may change to PSCI at some point
> >
> > That sounds nice. Any idea if/when that would be likely to happen?
> 
> The PSCI aspect?  Unfortunately not at the moment due to the secure
> firmware dependency.

Ah, ok.

[...]

> > > +/ {
> > > +       compatible = "nvidia,tegra132";
> > > +       interrupt-parent = <&gic>;
> > > +       #address-cells = <2>;
> > > +       #size-cells = <2>;
> > > +
> > > +       pcie-controller@0,01003000 {
> > > +               compatible = "nvidia,tegra132-pcie", "nvidia,tegra124-pcie";
> >
> > I couldn't spot "nvidia,tegra132-pcie" in mainline anywhere. Does it
> > differ significantly from "nvidia,tegra124-pcie"?
> 
> I'm not sure if the T132 PCIe driver will need any T132-specific
> workarounds.  I just followed what seems to be at least one DT practice
> discussed in the past of adding SoC-specific variants for the IP blocks in
> the event that a SoC-specific driver change is needed in the future.  I'm
> probably out-of-date on the current doctrine here, though.
> 
> Anyway, I have no problem with dropping the tegra132- variants, except in
> cases where changes are specifically known to be needed, although this
> seems likely to strengthen the binding between DT data and kernel
> versions.  Let me know.

I agree we should have the tegra132- variants, even if we don't know how
different the blocks happen to be from their predecessors. So please
don't drop them.

It would just be nice to dump the strings in binding docs so as to not
have checkpatch complaints.

> 
> > > +               device_type = "pci";
> > > +               reg = <0x0 0x01003000 0x0 0x00000800   /* PADS registers */
> > > +                      0x0 0x01003800 0x0 0x00000800   /* AFI registers */
> > > +                      0x0 0x02000000 0x0 0x10000000>; /* configuration space */
> >
> > Nit: for properties which are lists, please bracket each entry, e.g.
> >
> > reg = <0x0 0x01003000 0x0 0x00000800>,   /* PADS registers */
> >       <0x0 0x01003800 0x0 0x00000800>,
> >       <0x0 0x02000000 0x0 0x10000000>;
> >
> > I appreciate the newlines serve as an obvious delimiter here, but in
> > single-line cases it really helps, and it would be nice to be
> > consistent. It looks like most instances in this file already do this
> > anyway.
> 
> OK will do.
> 
> >
> > > +               reg-names = "pads", "afi", "cs";
> > > +               interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>, /* controller interrupt */
> > > +                            <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>; /* MSI interrupt */
> > > +               interrupt-names = "intr", "msi";
> > > +
> > > +               #interrupt-cells = <1>;
> > > +               interrupt-map-mask = <0 0 0 0>;
> > > +               interrupt-map = <0 0 0 0 &gic GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> > > +
> > > +               bus-range = <0x00 0xff>;
> > > +               #address-cells = <3>;
> > > +               #size-cells = <2>;
> > > +
> > > +               ranges = <0x82000000 0 0x01000000 0x0 0x01000000 0 0x00001000   /* port 0 configuration space */
> > > +                         0x82000000 0 0x01001000 0x0 0x01001000 0 0x00001000   /* port 1 configuration space */
> > > +                         0x81000000 0 0x0        0x0 0x12000000 0 0x00010000   /* downstream I/O (64 KiB) */
> > > +                         0x82000000 0 0x13000000 0x0 0x13000000 0 0x0d000000   /* non-prefetchable memory (208 MiB) */
> > > +                         0xc2000000 0 0x20000000 0x0 0x20000000 0 0x20000000>; /* prefetchable memory (512 MiB) */
> >
> > Same here w.r.t. list bracketing.
> 
> OK
> 
> >
> > > +
> > > +               clocks = <&tegra_car TEGRA124_CLK_PCIE>,
> > > +                        <&tegra_car TEGRA124_CLK_AFI>,
> > > +                        <&tegra_car TEGRA124_CLK_PLL_E>,
> > > +                        <&tegra_car TEGRA124_CLK_CML0>;
> > > +               clock-names = "pex", "afi", "pll_e", "cml";
> > > +               resets = <&tegra_car 70>,
> > > +                        <&tegra_car 72>,
> > > +                        <&tegra_car 74>;
> > > +               reset-names = "pex", "afi", "pcie_x";
> > > +               status = "disabled";
> > > +
> > > +               phys = <&xusb_padctl TEGRA_XUSB_PADCTL_PCIE>;
> > > +               phy-names = "pcie";
> > > +
> > > +               pci@1,0 {
> > > +                       device_type = "pci";
> > > +                       assigned-addresses = <0x82000800 0 0x01000000 0 0x1000>;
> > > +                       reg = <0x000800 0 0 0 0>;
> > > +                       status = "disabled";
> > > +
> > > +                       #address-cells = <3>;
> > > +                       #size-cells = <2>;
> > > +                       ranges;
> >
> > I'm not sure why these three properties are here. Surely this is a
> > separate address space (so isn't ranges invalid?), and do we define any
> > sub-nodes anywhere?
> 
> Hmm not sure.  This was originally copied from
> arch/arm/boot/dts/tegra124.dtsi.  Will plan to drop them for now, and then
> if the properties (or ones like them) turn out to be needed in the future,
> someone else can deal with it :-)

That sounds sane to me.

> 
> >
> > > +
> > > +                       nvidia,num-lanes = <2>;
> > > +               };
> > > +
> > > +               pci@2,0 {
> > > +                       device_type = "pci";
> > > +                       assigned-addresses = <0x82001000 0 0x01001000 0 0x1000>;
> > > +                       reg = <0x001000 0 0 0 0>;
> > > +                       status = "disabled";
> > > +
> > > +                       #address-cells = <3>;
> > > +                       #size-cells = <2>;
> > > +                       ranges;
> > > +
> >
> > Likewise.
> 
> OK
> 
> >
> > > +                       nvidia,num-lanes = <1>;
> > > +               };
> > > +       };
> >
> > [...]
> >
> > > +       host1x@0,50000000 {
> > > +               compatible = "nvidia,tegra132-host1x", "nvidia,tegra124-host1x", "simple-bus";
> >
> > Regarding simple-bus, are the sub-nodes usable if this didn't probe as
> > "nvidia,tegra124-host1x" or "nvidia,tegra132-host1x"?
> > Do the subnodes require anything from this node?
> >
> > It looks like we expect/require the host1x node to be probed and
> > initialised before we probe the children. Which would imply the
> > simple-bus annotation is wrong.
> 
> Haven't tried booting with just simple-bus here.  I believe these devices
> are accessible without the involvement of host1x.  In other words, host1x
> is not a bus; I believe it might be most accurately described as a type of
> DMA controller.  So in theory it should be possible for the CPU complex to
> read and write to these devices directly without the involvement of
> host1x, although I believe NVIDIA discourages this.
> 
> Under the theory that DT data should be hardware-specific, not
> software-specific, in an ideal world I think we would list these devices
> outside the embrace of the host1x node.  However the existing Tegra124 DT
> file listed them together, and I am unsure whether that is required for
> the host1x code to function correctly.
>
> Arto may be able to comment further.

Hmm, It would be good to hear from someone familar with that then. I'll
wait for Arto.

[...]

> > > +               ahb_gizmo: ahb-gizmo@0,6000c004 {
> > > +                       compatible = "nvidia,tegra132-ahb", "nvidia,tegra124-ahb", "nvidia,tegra30-ahb";
> > > +                       /*
> > > +                        * This IP block actually starts at 0x6000c000,
> > > +                        * but all of the register offsets in the driver
> > > +                        * have 0x4 subtracted from them.  So handle
> > > +                        * it this way until the driver is fixed.
> > > +                        */
> > > +                       reg = <0x0 0x6000c004 0x0 0x14d>;
> > > +               };
> >
> > That doesn't sound great. Can we not fix the driver and limit that
> > mistake to existing DTBs?
> 
> We can do it that way if you think it's important to fix now.  Have been
> trying to avoid gating our initial ARM64 support on fixing all of our
> hardware misalignment problems accumulated over the years, but if you
> think it's a blocker, it can be done.

I appreciate it's a bit of a pain, but once we allow it we're stuck with
it forever, so I'd prefer that we fix these known binding issues first.

[...]

> > > +               apbmisc: apbmisc@0,70000800 {
> > > +                       compatible = "nvidia,tegra132-apbmisc", "nvidia,tegra124-apbmisc", "nvidia,tegra20-apbmisc";
> > > +                       reg = <0x0 0x70000800 0x0 0x64>,   /* Chip revision */
> > > +                             <0x0 0x7000E864 0x0 0x04>;   /* Strapping options */
> > > +               };
> > > +
> > > +               pinmux: pinmux@0,70000868 {
> > > +                       compatible = "nvidia,tegra132-pinmux", "nvidia,tegra124-pinmux";
> > > +                       reg = <0x0 0x70000868 0x0 0x164>,  /* Pad control registers */
> > > +                             <0x0 0x70003000 0x0 0x434>;  /* Mux registers */
> > > +               };
> > > +
> > > +               /*
> > > +                * There are two serial drivers: an 8250 based simple
> > > +                * serial driver and an APB DMA based serial driver
> > > +                * for higher baudrate and performance. To enable the
> > > +                * 8250 based driver, the compatible string is
> > > +                * "nvidia,tegra132-uart", "nvidia,tegra124-uart",
> > > +                * "nvidia,tegra20-uart" and to enable the APB DMA
> > > +                * based serial driver, the compatible string is
> > > +                * "nvidia,tegra132-hsuart", "nvidia,tegra124-hsuart",
> > > +                * "nvidia,tegra30-hsuart".
> > > +                */
> >
> > Is there any reason to continue with this split?
> >
> > Surely if the only difference is DMA, the presence of dmas and dma-names
> > should be sufficient to get the driver to do the right thing, and if you
> > need to disable DMA for debugging that could be a command-line option.
> >
> > Does the binding and/or driver support aliases so you can get consistent
> > numbering? It would be nice to have.
> >
> > Do these UARTs work with earlycon?
> >
> > It would be nice to have a /chosen/stdout-path (ideally with rate) so as
> > to get output consistently without command line options being required.
> 
> Stephen Warren might be the best person to directly respond to these
> issues.  This is all legacy DT data from previous Tegra SoCs.

Ok. I'll wait for something from Stephen.

[...]

> > > +       cpus {
> > > +               #address-cells = <2>;
> > > +               #size-cells = <0>;
> > > +
> > > +               cpu@0 {
> > > +                       device_type = "cpu";
> > > +                       compatible = "nvidia,denver", "arm,armv8";
> > > +                       reg = <0x0 0x0>;
> > > +                       enable-method = "spin-table";
> > > +                       cpu-release-addr = <0x0 0x80000008>;
> > > +               };
> > > +
> > > +               cpu@1 {
> > > +                       device_type = "cpu";
> > > +                       compatible = "nvidia,denver", "arm,armv8";
> > > +                       reg = <0x0 0x1>;
> > > +                       enable-method = "spin-table";
> > > +                       cpu-release-addr = <0x0 0x80000008>;
> > > +               };
> > > +       };
> >
> > It would be nice if this were near the top, as in other dts files.
> 
> OK will move.
> 
> >
> > It's a shame these share the same release address, though I guess that
> > doesn't matter too much if PSCI is forthcoming.
> >
> > I didn't spot a memory node or /memreserve/ entries. Is the memory used
> > here for the spin-table guaranteed to be protected?
> >
> > If the enable-method might change, it's probably best to leave this to
> > the bootloader to fill in, or to have it in the board files if the boot
> > loader isn't that clever (perhaps via a dtsi).
> 
> OK will drop the enable-method for the time being.

Ok.

> > > +       thermal-zones {
> > > +               /* XXX T132 CPU thermal zone - still TBD */
> > > +
> >
> > What is still TBD here?
> 
> The T132 CPU thermal zone thermal sensor data.  Specifically (and I hope
> this comes as no surprise), there's some power/thermal management data
> missing from this file that is intended to be added later.  The
> placeholder is simply to indicate that.

So the other data is correct, but just incomplete?

I'm trying to get a feel for how this is likely to change and what that
means for long-term compatibility. If what's here is sufficient but
sub-optimal, that's fine.

Thanks,
Mark.
Thierry Reding Jan. 23, 2015, 12:03 p.m. UTC | #4
On Fri, Jan 23, 2015 at 11:44:24AM +0000, Mark Rutland wrote:
> On Fri, Jan 23, 2015 at 11:31:24AM +0000, Paul Walmsley wrote:
> > On Wed, 21 Jan 2015, Mark Rutland wrote:
> > > On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
[...]
> > > > +
> > > > +               clocks = <&tegra_car TEGRA124_CLK_PCIE>,
> > > > +                        <&tegra_car TEGRA124_CLK_AFI>,
> > > > +                        <&tegra_car TEGRA124_CLK_PLL_E>,
> > > > +                        <&tegra_car TEGRA124_CLK_CML0>;
> > > > +               clock-names = "pex", "afi", "pll_e", "cml";
> > > > +               resets = <&tegra_car 70>,
> > > > +                        <&tegra_car 72>,
> > > > +                        <&tegra_car 74>;
> > > > +               reset-names = "pex", "afi", "pcie_x";
> > > > +               status = "disabled";
> > > > +
> > > > +               phys = <&xusb_padctl TEGRA_XUSB_PADCTL_PCIE>;
> > > > +               phy-names = "pcie";
> > > > +
> > > > +               pci@1,0 {
> > > > +                       device_type = "pci";
> > > > +                       assigned-addresses = <0x82000800 0 0x01000000 0 0x1000>;
> > > > +                       reg = <0x000800 0 0 0 0>;
> > > > +                       status = "disabled";
> > > > +
> > > > +                       #address-cells = <3>;
> > > > +                       #size-cells = <2>;
> > > > +                       ranges;
> > >
> > > I'm not sure why these three properties are here. Surely this is a
> > > separate address space (so isn't ranges invalid?), and do we define any
> > > sub-nodes anywhere?
> > 
> > Hmm not sure.  This was originally copied from
> > arch/arm/boot/dts/tegra124.dtsi.  Will plan to drop them for now, and then
> > if the properties (or ones like them) turn out to be needed in the future,
> > someone else can deal with it :-)
> 
> That sounds sane to me.

This follows the bindings defined almost two years ago. There was a lot
of discussion back then and this is what we agreed upon. #address-cells
and #size-cells are needed as per the PCI device tree bindings and the
ranges property needed because the PCIe root ports translate addresses
between the host bridge and the PCI endpoint devices.

> > > > +       host1x@0,50000000 {
> > > > +               compatible = "nvidia,tegra132-host1x", "nvidia,tegra124-host1x", "simple-bus";
> > >
> > > Regarding simple-bus, are the sub-nodes usable if this didn't probe as
> > > "nvidia,tegra124-host1x" or "nvidia,tegra132-host1x"?
> > > Do the subnodes require anything from this node?
> > >
> > > It looks like we expect/require the host1x node to be probed and
> > > initialised before we probe the children. Which would imply the
> > > simple-bus annotation is wrong.
> > 
> > Haven't tried booting with just simple-bus here.  I believe these devices
> > are accessible without the involvement of host1x.  In other words, host1x
> > is not a bus; I believe it might be most accurately described as a type of
> > DMA controller.  So in theory it should be possible for the CPU complex to
> > read and write to these devices directly without the involvement of
> > host1x, although I believe NVIDIA discourages this.
> > 
> > Under the theory that DT data should be hardware-specific, not
> > software-specific, in an ideal world I think we would list these devices
> > outside the embrace of the host1x node.  However the existing Tegra124 DT
> > file listed them together, and I am unsure whether that is required for
> > the host1x code to function correctly.
> >
> > Arto may be able to comment further.
> 
> Hmm, It would be good to hear from someone familar with that then. I'll
> wait for Arto.

We actually rely on the parent-child relationship in drivers, so we
can't just go and reorganize things at will. This is documented in the
existing binding for host1x, which again, was agreed upon a long time
ago.

As for the simple-bus compatible, I think that was used way back to make
sure of_platform_populate() gets called. I suppose we could drop it and
call of_platform_populate() from the driver if its not there. The reason
why we never considered cases where it would probe as simple-bus is that
it was deemed unreasonable for a driver to bind to simple-bus.

> > > > +       cpus {
> > > > +               #address-cells = <2>;
> > > > +               #size-cells = <0>;
> > > > +
> > > > +               cpu@0 {
> > > > +                       device_type = "cpu";
> > > > +                       compatible = "nvidia,denver", "arm,armv8";
> > > > +                       reg = <0x0 0x0>;
> > > > +                       enable-method = "spin-table";
> > > > +                       cpu-release-addr = <0x0 0x80000008>;
> > > > +               };
> > > > +
> > > > +               cpu@1 {
> > > > +                       device_type = "cpu";
> > > > +                       compatible = "nvidia,denver", "arm,armv8";
> > > > +                       reg = <0x0 0x1>;
> > > > +                       enable-method = "spin-table";
> > > > +                       cpu-release-addr = <0x0 0x80000008>;
> > > > +               };
> > > > +       };
> > >
> > > It would be nice if this were near the top, as in other dts files.
> > 
> > OK will move.

Actually this follows the rules that we've been following with every DTS
so far. Nodes with a unit address go first, sorted by unit address. They
are followed by nodes without a unit address, sorted alphabetically.

I'd prefer keeping it this way for consistency across Tegra DTS files.

Thierry
Arto Merilainen Jan. 23, 2015, 12:14 p.m. UTC | #5
Hi all,

On 01/23/2015 01:31 PM, Paul Walmsley wrote:
> + Arto, Terje for comments on the host1x section
> + Stephen Warren for comments on the serial DT data
>
> Hi Mark,
>
> thanks for the review.
>
> On Wed, 21 Jan 2015, Mark Rutland wrote:
>
>> As mentioned in my reply to the DT list patch [1], there are a couple of
>> bits I'd like to see cleaned up first, but in the meantime I have some
>> comments from my first pass of the dtsi below. Some of these may equally
>> apply to existing dts(i) files.
>>
>> I see a few undocumented compatible strings (at least when comparing
>> against mainline). If there are other series or trees I should be
>> looking at, any pointers would be appreciated. If not, documentation
>> updates would be nice (checkpatch should complain otherwise).
>
> (discussed in the tegra132-pcie comments below)
>
>>
>> On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
>>> +       host1x@0,50000000 {
>>> +               compatible = "nvidia,tegra132-host1x", "nvidia,tegra124-host1x", "simple-bus";
>>
>> Regarding simple-bus, are the sub-nodes usable if this didn't probe as
>> "nvidia,tegra124-host1x" or "nvidia,tegra132-host1x"?
>> Do the subnodes require anything from this node?
>>
>> It looks like we expect/require the host1x node to be probed and
>> initialised before we probe the children. Which would imply the
>> simple-bus annotation is wrong.
>
> Haven't tried booting with just simple-bus here.  I believe these devices
> are accessible without the involvement of host1x.  In other words, host1x
> is not a bus; I believe it might be most accurately described as a type of
> DMA controller.  So in theory it should be possible for the CPU complex to
> read and write to these devices directly without the involvement of
> host1x, although I believe NVIDIA discourages this.
>
> Under the theory that DT data should be hardware-specific, not
> software-specific, in an ideal world I think we would list these devices
> outside the embrace of the host1x node.  However the existing Tegra124 DT
> file listed them together, and I am unsure whether that is required for
> the host1x code to function correctly.
>
> Arto may be able to comment further.

Placing the devices behind host1x is an accurate description of 
hardware: Despite the direct register access path (writel/readl 
targeting a host1x client device) is transparent to the CPU, the host1x 
hardware is internally handling the requests and passing them forward to 
the host1x client in interest.

Above implies also a strict parent-child dependency on hardware level: 
If the CPU tries to access a register in a host1x client before the 
host1x clock has been enabled, host1x will not be able to forward the 
requests and the access will fail. This also defines the importance of 
probe order (i.e. host1x must be initialized before client devices).

In addition, the host1x indirect register programming path ("channel 
path") is operational only for the host1x client devices and our current 
host1x driver relies on parent-child relationship in device tree.

- Arto
Mark Rutland Jan. 23, 2015, 12:17 p.m. UTC | #6
On Fri, Jan 23, 2015 at 12:03:57PM +0000, Thierry Reding wrote:
> On Fri, Jan 23, 2015 at 11:44:24AM +0000, Mark Rutland wrote:
> > On Fri, Jan 23, 2015 at 11:31:24AM +0000, Paul Walmsley wrote:
> > > On Wed, 21 Jan 2015, Mark Rutland wrote:
> > > > On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
> [...]
> > > > > +
> > > > > +               clocks = <&tegra_car TEGRA124_CLK_PCIE>,
> > > > > +                        <&tegra_car TEGRA124_CLK_AFI>,
> > > > > +                        <&tegra_car TEGRA124_CLK_PLL_E>,
> > > > > +                        <&tegra_car TEGRA124_CLK_CML0>;
> > > > > +               clock-names = "pex", "afi", "pll_e", "cml";
> > > > > +               resets = <&tegra_car 70>,
> > > > > +                        <&tegra_car 72>,
> > > > > +                        <&tegra_car 74>;
> > > > > +               reset-names = "pex", "afi", "pcie_x";
> > > > > +               status = "disabled";
> > > > > +
> > > > > +               phys = <&xusb_padctl TEGRA_XUSB_PADCTL_PCIE>;
> > > > > +               phy-names = "pcie";
> > > > > +
> > > > > +               pci@1,0 {
> > > > > +                       device_type = "pci";
> > > > > +                       assigned-addresses = <0x82000800 0 0x01000000 0 0x1000>;
> > > > > +                       reg = <0x000800 0 0 0 0>;
> > > > > +                       status = "disabled";
> > > > > +
> > > > > +                       #address-cells = <3>;
> > > > > +                       #size-cells = <2>;
> > > > > +                       ranges;
> > > >
> > > > I'm not sure why these three properties are here. Surely this is a
> > > > separate address space (so isn't ranges invalid?), and do we define any
> > > > sub-nodes anywhere?
> > > 
> > > Hmm not sure.  This was originally copied from
> > > arch/arm/boot/dts/tegra124.dtsi.  Will plan to drop them for now, and then
> > > if the properties (or ones like them) turn out to be needed in the future,
> > > someone else can deal with it :-)
> > 
> > That sounds sane to me.
> 
> This follows the bindings defined almost two years ago. There was a lot
> of discussion back then and this is what we agreed upon. #address-cells
> and #size-cells are needed as per the PCI device tree bindings and the
> ranges property needed because the PCIe root ports translate addresses
> between the host bridge and the PCI endpoint devices.

Ok. That sounds a little surprising to me, but I am admittedly not
familiar with this binding and PCI more generally. I'll dig a bit
deeper.

> > > > > +       host1x@0,50000000 {
> > > > > +               compatible = "nvidia,tegra132-host1x", "nvidia,tegra124-host1x", "simple-bus";
> > > >
> > > > Regarding simple-bus, are the sub-nodes usable if this didn't probe as
> > > > "nvidia,tegra124-host1x" or "nvidia,tegra132-host1x"?
> > > > Do the subnodes require anything from this node?
> > > >
> > > > It looks like we expect/require the host1x node to be probed and
> > > > initialised before we probe the children. Which would imply the
> > > > simple-bus annotation is wrong.
> > > 
> > > Haven't tried booting with just simple-bus here.  I believe these devices
> > > are accessible without the involvement of host1x.  In other words, host1x
> > > is not a bus; I believe it might be most accurately described as a type of
> > > DMA controller.  So in theory it should be possible for the CPU complex to
> > > read and write to these devices directly without the involvement of
> > > host1x, although I believe NVIDIA discourages this.
> > > 
> > > Under the theory that DT data should be hardware-specific, not
> > > software-specific, in an ideal world I think we would list these devices
> > > outside the embrace of the host1x node.  However the existing Tegra124 DT
> > > file listed them together, and I am unsure whether that is required for
> > > the host1x code to function correctly.
> > >
> > > Arto may be able to comment further.
> > 
> > Hmm, It would be good to hear from someone familar with that then. I'll
> > wait for Arto.
> 
> We actually rely on the parent-child relationship in drivers, so we
> can't just go and reorganize things at will. This is documented in the
> existing binding for host1x, which again, was agreed upon a long time
> ago.
> 
> As for the simple-bus compatible, I think that was used way back to make
> sure of_platform_populate() gets called. I suppose we could drop it and
> call of_platform_populate() from the driver if its not there. The reason
> why we never considered cases where it would probe as simple-bus is that
> it was deemed unreasonable for a driver to bind to simple-bus.

Labelling something as simple-bus just to get an implicit
of_platform_populate is an abuse of simple-bus. If you have a driver for
handling the device as something more complex than a simple-bus, it
absolutely must do that probing itself (because there could be some
ordering requirement on re-initialisation of the bus).

If the sub-nodes don't make sense on their own, the "simple-bus" string
is not appropriate regardless.

One thing I've been hoping/trying to do for a while (with little success
so far) was to split simple-bus handling into a simple MMIO bus driver,
which exposes and/or blows up in cases like this.

> > > > > +       cpus {
> > > > > +               #address-cells = <2>;
> > > > > +               #size-cells = <0>;
> > > > > +
> > > > > +               cpu@0 {
> > > > > +                       device_type = "cpu";
> > > > > +                       compatible = "nvidia,denver", "arm,armv8";
> > > > > +                       reg = <0x0 0x0>;
> > > > > +                       enable-method = "spin-table";
> > > > > +                       cpu-release-addr = <0x0 0x80000008>;
> > > > > +               };
> > > > > +
> > > > > +               cpu@1 {
> > > > > +                       device_type = "cpu";
> > > > > +                       compatible = "nvidia,denver", "arm,armv8";
> > > > > +                       reg = <0x0 0x1>;
> > > > > +                       enable-method = "spin-table";
> > > > > +                       cpu-release-addr = <0x0 0x80000008>;
> > > > > +               };
> > > > > +       };
> > > >
> > > > It would be nice if this were near the top, as in other dts files.
> > > 
> > > OK will move.
> 
> Actually this follows the rules that we've been following with every DTS
> so far. Nodes with a unit address go first, sorted by unit address. They
> are followed by nodes without a unit address, sorted alphabetically.
> 
> I'd prefer keeping it this way for consistency across Tegra DTS files.

Ok.

I guess the ordering of this w.r.t. other nodes isn't too important.
While I find it surprising, at least it shouldn't cause issues in the
DTB itself.

However, it would be nice if we aligned all dts a bit better long-term.

Thanks,
Mark.
Mark Rutland Jan. 23, 2015, 12:22 p.m. UTC | #7
On Fri, Jan 23, 2015 at 12:14:00PM +0000, Arto Merilainen wrote:
> Hi all,
> 
> On 01/23/2015 01:31 PM, Paul Walmsley wrote:
> > + Arto, Terje for comments on the host1x section
> > + Stephen Warren for comments on the serial DT data
> >
> > Hi Mark,
> >
> > thanks for the review.
> >
> > On Wed, 21 Jan 2015, Mark Rutland wrote:
> >
> >> As mentioned in my reply to the DT list patch [1], there are a couple of
> >> bits I'd like to see cleaned up first, but in the meantime I have some
> >> comments from my first pass of the dtsi below. Some of these may equally
> >> apply to existing dts(i) files.
> >>
> >> I see a few undocumented compatible strings (at least when comparing
> >> against mainline). If there are other series or trees I should be
> >> looking at, any pointers would be appreciated. If not, documentation
> >> updates would be nice (checkpatch should complain otherwise).
> >
> > (discussed in the tegra132-pcie comments below)
> >
> >>
> >> On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
> >>> +       host1x@0,50000000 {
> >>> +               compatible = "nvidia,tegra132-host1x", "nvidia,tegra124-host1x", "simple-bus";
> >>
> >> Regarding simple-bus, are the sub-nodes usable if this didn't probe as
> >> "nvidia,tegra124-host1x" or "nvidia,tegra132-host1x"?
> >> Do the subnodes require anything from this node?
> >>
> >> It looks like we expect/require the host1x node to be probed and
> >> initialised before we probe the children. Which would imply the
> >> simple-bus annotation is wrong.
> >
> > Haven't tried booting with just simple-bus here.  I believe these devices
> > are accessible without the involvement of host1x.  In other words, host1x
> > is not a bus; I believe it might be most accurately described as a type of
> > DMA controller.  So in theory it should be possible for the CPU complex to
> > read and write to these devices directly without the involvement of
> > host1x, although I believe NVIDIA discourages this.
> >
> > Under the theory that DT data should be hardware-specific, not
> > software-specific, in an ideal world I think we would list these devices
> > outside the embrace of the host1x node.  However the existing Tegra124 DT
> > file listed them together, and I am unsure whether that is required for
> > the host1x code to function correctly.
> >
> > Arto may be able to comment further.
> 
> Placing the devices behind host1x is an accurate description of 
> hardware: Despite the direct register access path (writel/readl 
> targeting a host1x client device) is transparent to the CPU, the host1x 
> hardware is internally handling the requests and passing them forward to 
> the host1x client in interest.
> 
> Above implies also a strict parent-child dependency on hardware level: 
> If the CPU tries to access a register in a host1x client before the 
> host1x clock has been enabled, host1x will not be able to forward the 
> requests and the access will fail. This also defines the importance of 
> probe order (i.e. host1x must be initialized before client devices).

In that case, it sounds like the "simple-bus" string is bogus unless the
host1x is at minimum pre-initialised by some firmware and/or bootloader
into a state where the child devices could theoretically be used on
their own.

So I think "simple-bus" should be dropped here.

> In addition, the host1x indirect register programming path ("channel 
> path") is operational only for the host1x client devices and our current 
> host1x driver relies on parent-child relationship in device tree.

So I guess this is more Linux-specific, but still adds to the fuel for
removing "simple-bus" here.

Thanks,
Mark.
Stephen Warren Jan. 23, 2015, 4:57 p.m. UTC | #8
On 01/23/2015 04:31 AM, Paul Walmsley wrote:
> + Arto, Terje for comments on the host1x section
> + Stephen Warren for comments on the serial DT data

> On Wed, 21 Jan 2015, Mark Rutland wrote:
>
>> As mentioned in my reply to the DT list patch [1], there are a couple of
>> bits I'd like to see cleaned up first, but in the meantime I have some
>> comments from my first pass of the dtsi below. Some of these may equally
>> apply to existing dts(i) files.
>>
>> I see a few undocumented compatible strings (at least when comparing
>> against mainline). If there are other series or trees I should be
>> looking at, any pointers would be appreciated. If not, documentation
>> updates would be nice (checkpatch should complain otherwise).

>> On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
>>>
>>> Add an initial device tree file for the Tegra132 SoC.  The DT file is
>>> based on arch/arm/boot/dts/tegra124.dtsi and
>>> arch/arm/boot/dts/tegra114.dtsi, with the following significant
>>> changes:
>>>
>>> - Tegra132 uses a Denver CPU cluster rather than an ARMv7 CPU cluster
>>> - Devices are arranged by bus, rather than in a flat topology
>>> - No polling delays have been defined for the thermal zones.  I don't
>>>    believe that this is a property of the SoC hardware, but rather of a
>>>    given use-case.

>>> diff --git a/arch/arm64/boot/dts/tegra/tegra132.dtsi b/arch/arm64/boot/dts/tegra/tegra132.dtsi
>>> +               /*
>>> +                * There are two serial drivers: an 8250 based simple
>>> +                * serial driver and an APB DMA based serial driver
>>> +                * for higher baudrate and performance. To enable the
>>> +                * 8250 based driver, the compatible string is
>>> +                * "nvidia,tegra132-uart", "nvidia,tegra124-uart",
>>> +                * "nvidia,tegra20-uart" and to enable the APB DMA
>>> +                * based serial driver, the compatible string is
>>> +                * "nvidia,tegra132-hsuart", "nvidia,tegra124-hsuart",
>>> +                * "nvidia,tegra30-hsuart".
>>> +                */
>>
>> Is there any reason to continue with this split?
>>
>> Surely if the only difference is DMA, the presence of dmas and dma-names
>> should be sufficient to get the driver to do the right thing, and if you
>> need to disable DMA for debugging that could be a command-line option.

I vaguely recall asking for the DMA support to be integrated into the 
regular 8250 driver instead when the separate DMA-capable driver was 
first added. I /think/ there was resistance to this because adding lots 
of different SoC-specific ways to do DMA into the existing 8250 driver 
would complicate it even more, and hence be unmaintainable.

I assume that reasoning is still valid.

Perhaps what we need is more fine-grained driver selection, not just 
based on compatible value. Something like:

if compatible == nvidia,tegra20-uart:
   if node.has_prop("enable-dma"):
     driver = Tegra-specfic DMA capable UART driver
   else:
     driver = Common 8250 driver

Is that something that of_serial.c could/should be enhanced to do? That 
said, the whole reasoning behind separate compatible properties before 
was that compatible is supposed to "drive" driver selection. At least, 
that's what I was told then.

>> Does the binding and/or driver support aliases so you can get consistent
>> numbering? It would be nice to have.

I believe either the serial core or serial driver(s) support DT aliases 
now, yes.

>> Do these UARTs work with earlycon?

I do not know.

>> It would be nice to have a /chosen/stdout-path (ideally with rate) so as
>> to get output consistently without command line options being required.
>
> Stephen Warren might be the best person to directly respond to these
> issues.  This is all legacy DT data from previous Tegra SoCs.
>
>>
>>> +               uarta: serial@0,70006000 {
>>> +                       compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
>>> +                       reg = <0x0 0x70006000 0x0 0x40>;
>>> +                       reg-shift = <2>;
>>> +                       interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>>> +                       clocks = <&tegra_car TEGRA124_CLK_UARTA>;
>>> +                       resets = <&tegra_car 6>;
>>> +                       reset-names = "serial";
>>> +                       dmas = <&apbdma 8>, <&apbdma 8>;
>>> +                       dma-names = "rx", "tx";
>>> +                       status = "disabled";
>>> +               };
Mark Rutland Jan. 23, 2015, 5:34 p.m. UTC | #9
On Fri, Jan 23, 2015 at 04:57:19PM +0000, Stephen Warren wrote:
> On 01/23/2015 04:31 AM, Paul Walmsley wrote:
> > + Arto, Terje for comments on the host1x section
> > + Stephen Warren for comments on the serial DT data
> 
> > On Wed, 21 Jan 2015, Mark Rutland wrote:
> >
> >> As mentioned in my reply to the DT list patch [1], there are a couple of
> >> bits I'd like to see cleaned up first, but in the meantime I have some
> >> comments from my first pass of the dtsi below. Some of these may equally
> >> apply to existing dts(i) files.
> >>
> >> I see a few undocumented compatible strings (at least when comparing
> >> against mainline). If there are other series or trees I should be
> >> looking at, any pointers would be appreciated. If not, documentation
> >> updates would be nice (checkpatch should complain otherwise).
> 
> >> On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
> >>>
> >>> Add an initial device tree file for the Tegra132 SoC.  The DT file is
> >>> based on arch/arm/boot/dts/tegra124.dtsi and
> >>> arch/arm/boot/dts/tegra114.dtsi, with the following significant
> >>> changes:
> >>>
> >>> - Tegra132 uses a Denver CPU cluster rather than an ARMv7 CPU cluster
> >>> - Devices are arranged by bus, rather than in a flat topology
> >>> - No polling delays have been defined for the thermal zones.  I don't
> >>>    believe that this is a property of the SoC hardware, but rather of a
> >>>    given use-case.
> 
> >>> diff --git a/arch/arm64/boot/dts/tegra/tegra132.dtsi b/arch/arm64/boot/dts/tegra/tegra132.dtsi
> >>> +               /*
> >>> +                * There are two serial drivers: an 8250 based simple
> >>> +                * serial driver and an APB DMA based serial driver
> >>> +                * for higher baudrate and performance. To enable the
> >>> +                * 8250 based driver, the compatible string is
> >>> +                * "nvidia,tegra132-uart", "nvidia,tegra124-uart",
> >>> +                * "nvidia,tegra20-uart" and to enable the APB DMA
> >>> +                * based serial driver, the compatible string is
> >>> +                * "nvidia,tegra132-hsuart", "nvidia,tegra124-hsuart",
> >>> +                * "nvidia,tegra30-hsuart".
> >>> +                */
> >>
> >> Is there any reason to continue with this split?
> >>
> >> Surely if the only difference is DMA, the presence of dmas and dma-names
> >> should be sufficient to get the driver to do the right thing, and if you
> >> need to disable DMA for debugging that could be a command-line option.
> 
> I vaguely recall asking for the DMA support to be integrated into the 
> regular 8250 driver instead when the separate DMA-capable driver was 
> first added. I /think/ there was resistance to this because adding lots 
> of different SoC-specific ways to do DMA into the existing 8250 driver 
> would complicate it even more, and hence be unmaintainable.
> 
> I assume that reasoning is still valid.
> 
> Perhaps what we need is more fine-grained driver selection, not just 
> based on compatible value. Something like:
> 
> if compatible == nvidia,tegra20-uart:
>    if node.has_prop("enable-dma"):
>      driver = Tegra-specfic DMA capable UART driver
>    else:
>      driver = Common 8250 driver

Surely we should assume that if the DMAs are listed (and not disabled)
they are usable? I'm not sure I see the point in an additional property
to force Linux-internal driver selection in this way.

What happens if we have a driver for a particular string, but fail to
probe? Can we fall back to a more generic driver?

> Is that something that of_serial.c could/should be enhanced to do? That 
> said, the whole reasoning behind separate compatible properties before 
> was that compatible is supposed to "drive" driver selection. At least, 
> that's what I was told then.

The compatible string is meant to describe the programming interface the
device is compatible with. The kernel should then choose the most
appropriate driver it knows can handle that HW programming interface.

Considering it as a way to choose the driver is backwards. I'd expect
the device node to have both strings (with the DMA capable string first)
unless it really doesn't have any DMAs.

Thanks,
Mark.
Stephen Warren Jan. 23, 2015, 5:47 p.m. UTC | #10
On 01/23/2015 10:34 AM, Mark Rutland wrote:
> On Fri, Jan 23, 2015 at 04:57:19PM +0000, Stephen Warren wrote:
>> On 01/23/2015 04:31 AM, Paul Walmsley wrote:
>>> + Arto, Terje for comments on the host1x section
>>> + Stephen Warren for comments on the serial DT data
>>
>>> On Wed, 21 Jan 2015, Mark Rutland wrote:
>>>
>>>> As mentioned in my reply to the DT list patch [1], there are a couple of
>>>> bits I'd like to see cleaned up first, but in the meantime I have some
>>>> comments from my first pass of the dtsi below. Some of these may equally
>>>> apply to existing dts(i) files.
>>>>
>>>> I see a few undocumented compatible strings (at least when comparing
>>>> against mainline). If there are other series or trees I should be
>>>> looking at, any pointers would be appreciated. If not, documentation
>>>> updates would be nice (checkpatch should complain otherwise).
>>
>>>> On Fri, Jan 16, 2015 at 11:45:29AM +0000, Paul Walmsley wrote:
>>>>>
>>>>> Add an initial device tree file for the Tegra132 SoC.  The DT file is
>>>>> based on arch/arm/boot/dts/tegra124.dtsi and
>>>>> arch/arm/boot/dts/tegra114.dtsi, with the following significant
>>>>> changes:
>>>>>
>>>>> - Tegra132 uses a Denver CPU cluster rather than an ARMv7 CPU cluster
>>>>> - Devices are arranged by bus, rather than in a flat topology
>>>>> - No polling delays have been defined for the thermal zones.  I don't
>>>>>     believe that this is a property of the SoC hardware, but rather of a
>>>>>     given use-case.
>>
>>>>> diff --git a/arch/arm64/boot/dts/tegra/tegra132.dtsi b/arch/arm64/boot/dts/tegra/tegra132.dtsi
>>>>> +               /*
>>>>> +                * There are two serial drivers: an 8250 based simple
>>>>> +                * serial driver and an APB DMA based serial driver
>>>>> +                * for higher baudrate and performance. To enable the
>>>>> +                * 8250 based driver, the compatible string is
>>>>> +                * "nvidia,tegra132-uart", "nvidia,tegra124-uart",
>>>>> +                * "nvidia,tegra20-uart" and to enable the APB DMA
>>>>> +                * based serial driver, the compatible string is
>>>>> +                * "nvidia,tegra132-hsuart", "nvidia,tegra124-hsuart",
>>>>> +                * "nvidia,tegra30-hsuart".
>>>>> +                */
>>>>
>>>> Is there any reason to continue with this split?
>>>>
>>>> Surely if the only difference is DMA, the presence of dmas and dma-names
>>>> should be sufficient to get the driver to do the right thing, and if you
>>>> need to disable DMA for debugging that could be a command-line option.
>>
>> I vaguely recall asking for the DMA support to be integrated into the
>> regular 8250 driver instead when the separate DMA-capable driver was
>> first added. I /think/ there was resistance to this because adding lots
>> of different SoC-specific ways to do DMA into the existing 8250 driver
>> would complicate it even more, and hence be unmaintainable.
>>
>> I assume that reasoning is still valid.
>>
>> Perhaps what we need is more fine-grained driver selection, not just
>> based on compatible value. Something like:
>>
>> if compatible == nvidia,tegra20-uart:
>>     if node.has_prop("enable-dma"):
>>       driver = Tegra-specfic DMA capable UART driver
>>     else:
>>       driver = Common 8250 driver
>
> Surely we should assume that if the DMAs are listed (and not disabled)
> they are usable?

I would assume so, yes.

 > I'm not sure I see the point in an additional property
> to force Linux-internal driver selection in this way.

I think it comes down to whether you want to use DMA for that UART or 
not. I don't recall the exact reason why this might be important. 
Perhaps it's one/more of:

- The Tegra-specific DMA-capable driver can't be used for early debug 
but the core 8250 driver can. I don't know if this is true though.

- There are a limited number of DMA channels on Tegra. If all ports 
blindly used DMA simply because the dmas property was included, then 
we'd risk running out of DMA channels for cases where it absolutely had 
to be used; e.g. a low-volume debug console might prevent a high-volume 
MODEM or BT UART from using DMA if they probed in the "wrong" order.

Laxman might be able to shed more light on this.

> What happens if we have a driver for a particular string, but fail to
> probe? Can we fall back to a more generic driver?

At least in this case, I'd assume so, yes.

>> Is that something that of_serial.c could/should be enhanced to do? That
>> said, the whole reasoning behind separate compatible properties before
>> was that compatible is supposed to "drive" driver selection. At least,
>> that's what I was told then.
>
> The compatible string is meant to describe the programming interface the
> device is compatible with. The kernel should then choose the most
> appropriate driver it knows can handle that HW programming interface.
>
> Considering it as a way to choose the driver is backwards. I'd expect
> the device node to have both strings (with the DMA capable string first)
> unless it really doesn't have any DMAs.

That's a subtly different interpretation of DT compatible usage than was 
given (I think it was by Mitch Bradley???) when this driver was 
introduced. In the conversation then, it was asserted that using 
different compatible values to describe that SW should use a different 
interface to the HW (non-DMA vs. DMA) was exactly in line with the 
semantic concept of compatible.
diff mbox

Patch

diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
index b411251..90f6284 100644
--- a/arch/arm64/boot/dts/Makefile
+++ b/arch/arm64/boot/dts/Makefile
@@ -3,6 +3,7 @@  dts-dirs += apm
 dts-dirs += arm
 dts-dirs += cavium
 dts-dirs += exynos
+dts-dirs += tegra
 
 always		:= $(dtb-y)
 subdir-y	:= $(dts-dirs)
diff --git a/arch/arm64/boot/dts/tegra/Makefile b/arch/arm64/boot/dts/tegra/Makefile
new file mode 100644
index 0000000..15dbaa0
--- /dev/null
+++ b/arch/arm64/boot/dts/tegra/Makefile
@@ -0,0 +1,3 @@ 
+always          := $(dtb-y)
+subdir-y        := $(dts-dirs)
+clean-files     := *.dtb
diff --git a/arch/arm64/boot/dts/tegra/tegra132.dtsi b/arch/arm64/boot/dts/tegra/tegra132.dtsi
new file mode 100644
index 0000000..4b93bfe
--- /dev/null
+++ b/arch/arm64/boot/dts/tegra/tegra132.dtsi
@@ -0,0 +1,997 @@ 
+#include <dt-bindings/clock/tegra124-car.h>
+#include <dt-bindings/gpio/tegra-gpio.h>
+#include <dt-bindings/memory/tegra124-mc.h>
+#include <dt-bindings/pinctrl/pinctrl-tegra.h>
+#include <dt-bindings/pinctrl/pinctrl-tegra-xusb.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/thermal/tegra124-soctherm.h>
+
+#include "skeleton.dtsi"
+
+/ {
+	compatible = "nvidia,tegra132";
+	interrupt-parent = <&gic>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	pcie-controller@0,01003000 {
+		compatible = "nvidia,tegra132-pcie", "nvidia,tegra124-pcie";
+		device_type = "pci";
+		reg = <0x0 0x01003000 0x0 0x00000800   /* PADS registers */
+		       0x0 0x01003800 0x0 0x00000800   /* AFI registers */
+		       0x0 0x02000000 0x0 0x10000000>; /* configuration space */
+		reg-names = "pads", "afi", "cs";
+		interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>, /* controller interrupt */
+			     <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>; /* MSI interrupt */
+		interrupt-names = "intr", "msi";
+
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
+
+		bus-range = <0x00 0xff>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+
+		ranges = <0x82000000 0 0x01000000 0x0 0x01000000 0 0x00001000   /* port 0 configuration space */
+			  0x82000000 0 0x01001000 0x0 0x01001000 0 0x00001000   /* port 1 configuration space */
+			  0x81000000 0 0x0        0x0 0x12000000 0 0x00010000   /* downstream I/O (64 KiB) */
+			  0x82000000 0 0x13000000 0x0 0x13000000 0 0x0d000000   /* non-prefetchable memory (208 MiB) */
+			  0xc2000000 0 0x20000000 0x0 0x20000000 0 0x20000000>; /* prefetchable memory (512 MiB) */
+
+		clocks = <&tegra_car TEGRA124_CLK_PCIE>,
+			 <&tegra_car TEGRA124_CLK_AFI>,
+			 <&tegra_car TEGRA124_CLK_PLL_E>,
+			 <&tegra_car TEGRA124_CLK_CML0>;
+		clock-names = "pex", "afi", "pll_e", "cml";
+		resets = <&tegra_car 70>,
+			 <&tegra_car 72>,
+			 <&tegra_car 74>;
+		reset-names = "pex", "afi", "pcie_x";
+		status = "disabled";
+
+		phys = <&xusb_padctl TEGRA_XUSB_PADCTL_PCIE>;
+		phy-names = "pcie";
+
+		pci@1,0 {
+			device_type = "pci";
+			assigned-addresses = <0x82000800 0 0x01000000 0 0x1000>;
+			reg = <0x000800 0 0 0 0>;
+			status = "disabled";
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges;
+
+			nvidia,num-lanes = <2>;
+		};
+
+		pci@2,0 {
+			device_type = "pci";
+			assigned-addresses = <0x82001000 0 0x01001000 0 0x1000>;
+			reg = <0x001000 0 0 0 0>;
+			status = "disabled";
+
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges;
+
+			nvidia,num-lanes = <1>;
+		};
+	};
+
+	iram_a: iram@0,40000000 {
+		compatible = "mmio-sram";
+		reg = <0x0 0x40000000 0x0 0x10000>;
+	};
+
+	iram_b: iram@0,40010000 {
+		compatible = "mmio-sram";
+		reg = <0x0 0x40010000 0x0 0x10000>;
+	};
+
+	iram_c: iram@0,40020000 {
+		compatible = "mmio-sram";
+		reg = <0x0 0x40020000 0x0 0x10000>;
+	};
+
+	iram_d: iram@0,40030000 {
+		compatible = "mmio-sram";
+		reg = <0x0 0x40030000 0x0 0x10000>;
+	};
+
+	host1x@0,50000000 {
+		compatible = "nvidia,tegra132-host1x", "nvidia,tegra124-host1x", "simple-bus";
+		reg = <0x0 0x50000000 0x0 0x00034000>;
+		interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */
+			     <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */
+		clocks = <&tegra_car TEGRA124_CLK_HOST1X>;
+		resets = <&tegra_car 28>;
+		reset-names = "host1x";
+
+		#address-cells = <2>;
+		#size-cells = <2>;
+
+		ranges = <0 0x54000000 0 0x54000000 0 0x01000000>;
+
+		dc@0,54200000 {
+			compatible = "nvidia,tegra132-dc", "nvidia,tegra124-dc";
+			reg = <0x0 0x54200000 0x0 0x00040000>;
+			interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_DISP1>,
+				 <&tegra_car TEGRA124_CLK_PLL_P>;
+			clock-names = "dc", "parent";
+			resets = <&tegra_car 27>;
+			reset-names = "dc";
+
+			iommus = <&mc TEGRA_SWGROUP_DC>;
+
+			nvidia,head = <0>;
+		};
+
+		dc@0,54240000 {
+			compatible = "nvidia,tegra132-dc", "nvidia,tegra124-dc";
+			reg = <0x0 0x54240000 0x0 0x00040000>;
+			interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_DISP2>,
+				 <&tegra_car TEGRA124_CLK_PLL_P>;
+			clock-names = "dc", "parent";
+			resets = <&tegra_car 26>;
+			reset-names = "dc";
+
+			iommus = <&mc TEGRA_SWGROUP_DCB>;
+
+			nvidia,head = <1>;
+		};
+
+		hdmi@0,54280000 {
+			compatible = "nvidia,tegra132-hdmi", "nvidia,tegra124-hdmi";
+			reg = <0x0 0x54280000 0x0 0x00040000>;
+			interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_HDMI>,
+				 <&tegra_car TEGRA124_CLK_PLL_D2_OUT0>;
+			clock-names = "hdmi", "parent";
+			resets = <&tegra_car 51>;
+			reset-names = "hdmi";
+			status = "disabled";
+		};
+
+		sor@0,54540000 {
+			compatible = "nvidia,tegra132-sor", "nvidia,tegra124-sor";
+			reg = <0x0 0x54540000 0x0 0x00040000>;
+			interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_SOR0>,
+				 <&tegra_car TEGRA124_CLK_PLL_D_OUT0>,
+				 <&tegra_car TEGRA124_CLK_PLL_DP>,
+				 <&tegra_car TEGRA124_CLK_CLK_M>;
+			clock-names = "sor", "parent", "dp", "safe";
+			resets = <&tegra_car 182>;
+			reset-names = "sor";
+			status = "disabled";
+		};
+
+		dpaux: dpaux@0,545c0000 {
+			compatible = "nvidia,tegra132-dpaux", "nvidia,tegra124-dpaux";
+			reg = <0x0 0x545c0000 0x0 0x00040000>;
+			interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
+				 <&tegra_car TEGRA124_CLK_PLL_DP>;
+			clock-names = "dpaux", "parent";
+			resets = <&tegra_car 181>;
+			reset-names = "dpaux";
+			status = "disabled";
+		};
+	};
+
+	gic: interrupt-controller@0,50041000 {
+		compatible = "arm,cortex-a15-gic";
+		#interrupt-cells = <3>;
+		interrupt-controller;
+		reg = <0x0 0x50041000 0x0 0x1000>,
+		      <0x0 0x50042000 0x0 0x1000>,
+		      <0x0 0x50044000 0x0 0x2000>,
+		      <0x0 0x50046000 0x0 0x2000>;
+		interrupts = <GIC_PPI 9
+			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
+	};
+
+	gpu: gpu@0,57000000 {
+		compatible = "nvidia,gk20a";
+		reg = <0x0 0x57000000 0x0 0x01000000>,
+		      <0x0 0x58000000 0x0 0x01000000>;
+		interrupts = <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "stall", "nonstall";
+		clocks = <&tegra_car TEGRA124_CLK_GPU>,
+			 <&tegra_car TEGRA124_CLK_PLL_P_OUT5>;
+		clock-names = "gpu", "pwr";
+		resets = <&tegra_car 184>;
+		reset-names = "gpu";
+		status = "disabled";
+	};
+
+	ppsb: ppsb@0,60000000 {
+		compatible = "simple-bus";
+		reg = <0x0 0x60000000 0x0 0x01000000>;
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		timer@0,60005000 {
+			compatible = "nvidia,tegra132-timer", "nvidia,tegra124-timer", "nvidia,tegra20-timer";
+			reg = <0x0 0x60005000 0x0 0x400>;
+			interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_TIMER>;
+		};
+
+		tegra_car: car@0,60006000 {
+			compatible = "nvidia,tegra132-car", "nvidia,tegra124-car";
+			reg = <0x0 0x60006000 0x0 0x1000>;
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+		};
+
+		flow_controller: flow-controller@0,60007000 {
+			compatible = "nvidia,tegra132-flowctrl", "nvidia,tegra124-flowctrl";
+			reg = <0x0 0x60007000 0x0 0x1000>;
+		};
+
+		ahb_gizmo: ahb-gizmo@0,6000c004 {
+			compatible = "nvidia,tegra132-ahb", "nvidia,tegra124-ahb", "nvidia,tegra30-ahb";
+			/*
+			 * This IP block actually starts at 0x6000c000,
+			 * but all of the register offsets in the driver
+			 * have 0x4 subtracted from them.  So handle
+			 * it this way until the driver is fixed.
+			 */
+			reg = <0x0 0x6000c004 0x0 0x14d>;
+		};
+
+		gpio: gpio@0,6000d000 {
+			/* XXX 8 GPIO blocks exist - this is only the first */
+			compatible = "nvidia,tegra132-gpio", "nvidia,tegra124-gpio", "nvidia,tegra30-gpio";
+			reg = <0x0 0x6000d000 0x0 0x1000>;
+			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>;
+			#gpio-cells = <2>;
+			gpio-controller;
+			#interrupt-cells = <2>;
+			interrupt-controller;
+		};
+
+		apbdma: apbdma@0,60020000 {
+			compatible = "nvidia,tegra132-apbdma", "nvidia,tegra124-apbdma", "nvidia,tegra148-apbdma";
+			reg = <0x0 0x60020000 0x0 0x1400>;
+			interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 129 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 132 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 136 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 139 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 141 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 142 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_APBDMA>;
+			resets = <&tegra_car 34>;
+			reset-names = "dma";
+			#dma-cells = <1>;
+		};
+
+	}; /* ppsb */
+
+	apb: apb@0,70000000 {
+		compatible = "simple-bus";
+		reg = <0x0 0x70000000 0x0 0x01000000>;
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		apbmisc: apbmisc@0,70000800 {
+			compatible = "nvidia,tegra132-apbmisc", "nvidia,tegra124-apbmisc", "nvidia,tegra20-apbmisc";
+			reg = <0x0 0x70000800 0x0 0x64>,   /* Chip revision */
+			      <0x0 0x7000E864 0x0 0x04>;   /* Strapping options */
+		};
+
+		pinmux: pinmux@0,70000868 {
+			compatible = "nvidia,tegra132-pinmux", "nvidia,tegra124-pinmux";
+			reg = <0x0 0x70000868 0x0 0x164>,  /* Pad control registers */
+			      <0x0 0x70003000 0x0 0x434>;  /* Mux registers */
+		};
+
+		/*
+		 * There are two serial drivers: an 8250 based simple
+		 * serial driver and an APB DMA based serial driver
+		 * for higher baudrate and performance. To enable the
+		 * 8250 based driver, the compatible string is
+		 * "nvidia,tegra132-uart", "nvidia,tegra124-uart",
+		 * "nvidia,tegra20-uart" and to enable the APB DMA
+		 * based serial driver, the compatible string is
+		 * "nvidia,tegra132-hsuart", "nvidia,tegra124-hsuart",
+		 * "nvidia,tegra30-hsuart".
+		 */
+		uarta: serial@0,70006000 {
+			compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
+			reg = <0x0 0x70006000 0x0 0x40>;
+			reg-shift = <2>;
+			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_UARTA>;
+			resets = <&tegra_car 6>;
+			reset-names = "serial";
+			dmas = <&apbdma 8>, <&apbdma 8>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		uartb: serial@0,70006040 {
+			compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
+			reg = <0x0 0x70006040 0x0 0x40>;
+			reg-shift = <2>;
+			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_UARTB>;
+			resets = <&tegra_car 7>;
+			reset-names = "serial";
+			dmas = <&apbdma 9>, <&apbdma 9>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		uartc: serial@0,70006200 {
+			compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
+			reg = <0x0 0x70006200 0x0 0x40>;
+			reg-shift = <2>;
+			interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_UARTC>;
+			resets = <&tegra_car 55>;
+			reset-names = "serial";
+			dmas = <&apbdma 10>, <&apbdma 10>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		uartd: serial@0,70006300 {
+			compatible = "nvidia,tegra132-uart", "nvidia,tegra124-uart", "nvidia,tegra20-uart";
+			reg = <0x0 0x70006300 0x0 0x40>;
+			reg-shift = <2>;
+			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_UARTD>;
+			resets = <&tegra_car 65>;
+			reset-names = "serial";
+			dmas = <&apbdma 19>, <&apbdma 19>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		pwm: pwm@0,7000a000 {
+			compatible = "nvidia,tegra132-pwm", "nvidia,tegra124-pwm", "nvidia,tegra20-pwm";
+			reg = <0x0 0x7000a000 0x0 0x100>;
+			#pwm-cells = <2>;
+			clocks = <&tegra_car TEGRA124_CLK_PWM>;
+			resets = <&tegra_car 17>;
+			reset-names = "pwm";
+			status = "disabled";
+		};
+
+		i2c1: i2c@0,7000c000 { /* GEN1_I2C */
+			compatible = "nvidia,tegra132-i2c", "nvidia,tegra124-i2c", "nvidia,tegra114-i2c";
+			reg = <0x0 0x7000c000 0x0 0x100>;
+			interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_I2C1>;
+			clock-names = "div-clk";
+			resets = <&tegra_car 12>;
+			reset-names = "i2c";
+			dmas = <&apbdma 21>, <&apbdma 21>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		i2c2: i2c@0,7000c400 { /* GEN2_I2C */
+			compatible = "nvidia,tegra132-i2c", "nvidia,tegra124-i2c", "nvidia,tegra114-i2c";
+			reg = <0x0 0x7000c400 0x0 0x100>;
+			interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_I2C2>;
+			clock-names = "div-clk";
+			resets = <&tegra_car 54>;
+			reset-names = "i2c";
+			dmas = <&apbdma 22>, <&apbdma 22>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		i2c3: i2c@0,7000c500 { /* CAM_I2C */
+			compatible = "nvidia,tegra132-i2c", "nvidia,tegra124-i2c", "nvidia,tegra114-i2c";
+			reg = <0x0 0x7000c500 0x0 0x100>;
+			interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_I2C3>;
+			clock-names = "div-clk";
+			resets = <&tegra_car 67>;
+			reset-names = "i2c";
+			dmas = <&apbdma 23>, <&apbdma 23>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		i2c4: i2c@0,7000c700 {
+			compatible = "nvidia,tegra132-i2c", "nvidia,tegra124-i2c", "nvidia,tegra114-i2c";
+			reg = <0x0 0x7000c700 0x0 0x100>;
+			interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_I2C4>;
+			clock-names = "div-clk";
+			resets = <&tegra_car 103>;
+			reset-names = "i2c";
+			dmas = <&apbdma 26>, <&apbdma 26>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		i2c5: i2c@0,7000d000 { /* PWR_I2C */
+			compatible = "nvidia,tegra132-i2c", "nvidia,tegra124-i2c", "nvidia,tegra114-i2c";
+			reg = <0x0 0x7000d000 0x0 0x100>;
+			interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_I2C5>;
+			clock-names = "div-clk";
+			resets = <&tegra_car 47>;
+			reset-names = "i2c";
+			dmas = <&apbdma 24>, <&apbdma 24>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		i2c6: i2c@0,7000d100 {
+			compatible = "nvidia,tegra132-i2c", "nvidia,tegra124-i2c", "nvidia,tegra114-i2c";
+			reg = <0x0 0x7000d100 0x0 0x100>;
+			interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_I2C6>;
+			clock-names = "div-clk";
+			resets = <&tegra_car 166>;
+			reset-names = "i2c";
+			dmas = <&apbdma 30>, <&apbdma 30>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		spi2b_1: spi@0,7000d400 {
+			compatible = "nvidia,tegra132-spi", "nvidia,tegra124-spi", "nvidia,tegra114-spi";
+			reg = <0x0 0x7000d400 0x0 0x200>;
+			interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_SBC1>;
+			clock-names = "spi";
+			resets = <&tegra_car 41>;
+			reset-names = "spi";
+			dmas = <&apbdma 15>, <&apbdma 15>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		spi2b_2: spi@0,7000d600 {
+			compatible = "nvidia,tegra132-spi", "nvidia,tegra124-spi", "nvidia,tegra114-spi";
+			reg = <0x0 0x7000d600 0x0 0x200>;
+			interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_SBC2>;
+			clock-names = "spi";
+			resets = <&tegra_car 44>;
+			reset-names = "spi";
+			dmas = <&apbdma 16>, <&apbdma 16>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		spi2b_3: spi@0,7000d800 {
+			compatible = "nvidia,tegra132-spi", "nvidia,tegra124-spi", "nvidia,tegra114-spi";
+			reg = <0x0 0x7000d800 0x0 0x200>;
+			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_SBC3>;
+			clock-names = "spi";
+			resets = <&tegra_car 46>;
+			reset-names = "spi";
+			dmas = <&apbdma 17>, <&apbdma 17>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		spi2b_4: spi@0,7000da00 {
+			compatible = "nvidia,tegra132-spi", "nvidia,tegra124-spi", "nvidia,tegra114-spi";
+			reg = <0x0 0x7000da00 0x0 0x200>;
+			interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_SBC4>;
+			clock-names = "spi";
+			resets = <&tegra_car 68>;
+			reset-names = "spi";
+			dmas = <&apbdma 18>, <&apbdma 18>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		spi2b_5: spi@0,7000dc00 {
+			compatible = "nvidia,tegra132-spi", "nvidia,tegra124-spi", "nvidia,tegra114-spi";
+			reg = <0x0 0x7000dc00 0x0 0x200>;
+			interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_SBC5>;
+			clock-names = "spi";
+			resets = <&tegra_car 104>;
+			reset-names = "spi";
+			dmas = <&apbdma 27>, <&apbdma 27>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		spi2b_6: spi@0,7000de00 {
+			compatible = "nvidia,tegra132-spi", "nvidia,tegra124-spi", "nvidia,tegra114-spi";
+			reg = <0x0 0x7000de00 0x0 0x200>;
+			interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&tegra_car TEGRA124_CLK_SBC6>;
+			clock-names = "spi";
+			resets = <&tegra_car 105>;
+			reset-names = "spi";
+			dmas = <&apbdma 28>, <&apbdma 28>;
+			dma-names = "rx", "tx";
+			status = "disabled";
+		};
+
+		rtc: rtc@0,7000e000 {
+			compatible = "nvidia,tegra132-rtc", "nvidia,tegra124-rtc", "nvidia,tegra20-rtc";
+			reg = <0x0 0x7000e000 0x0 0x100>;
+			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_RTC>;
+		};
+
+		pmc: pmc@0,7000e400 {
+			compatible = "nvidia,tegra132-pmc", "nvidia,tegra124-pmc";
+			reg = <0x0 0x7000e400 0x0 0x400>;
+			clocks = <&tegra_car TEGRA124_CLK_PCLK>, <&clk32k_in>;
+			clock-names = "pclk", "clk32k_in";
+		};
+
+		fuse: fuse@0,7000f800 {
+			compatible = "nvidia,tegra132-efuse", "nvidia,tegra124-efuse";
+			reg = <0x0 0x7000f800 0x0 0x400>;
+			clocks = <&tegra_car TEGRA124_CLK_FUSE>;
+			clock-names = "fuse";
+			resets = <&tegra_car 39>;
+			reset-names = "fuse";
+		};
+
+		mc: memory-controller@0,70019000 {
+			compatible = "nvidia,tegra132-mc", "nvidia,tegra124-mc";
+			reg = <0x0 0x70019000 0x0 0x1000>;
+			clocks = <&tegra_car TEGRA124_CLK_MC>;
+			clock-names = "mc";
+
+			interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
+
+			#iommu-cells = <1>;
+		};
+
+		sata: sata@0,70020000 {
+			compatible = "nvidia,tegra132-ahci", "nvidia,tegra124-ahci";
+
+			reg = <0x0 0x70027000 0x0 0x2000>, /* AHCI */
+				<0x0 0x70020000 0x0 0x7000>; /* SATA */
+
+			interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
+
+			clocks = <&tegra_car TEGRA124_CLK_SATA>,
+				<&tegra_car TEGRA124_CLK_SATA_OOB>,
+				<&tegra_car TEGRA124_CLK_CML1>,
+				<&tegra_car TEGRA124_CLK_PLL_E>;
+			clock-names = "sata", "sata-oob", "cml1", "pll_e";
+
+			resets = <&tegra_car 124>,
+				<&tegra_car 123>,
+				<&tegra_car 129>;
+			reset-names = "sata", "sata-oob", "sata-cold";
+
+			phys = <&xusb_padctl TEGRA_XUSB_PADCTL_SATA>;
+			phy-names = "sata-phy";
+
+			status = "disabled";
+		};
+
+		hda: hda@0,70030000 {
+			compatible = "nvidia,tegra132-hda", "nvidia,tegra124-hda", "nvidia,tegra30-hda";
+			reg = <0x0 0x70030000 0x0 0x10000>;
+			interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_HDA>,
+				 <&tegra_car TEGRA124_CLK_HDA2HDMI>,
+				 <&tegra_car TEGRA124_CLK_HDA2CODEC_2X>;
+			clock-names = "hda", "hda2hdmi", "hdacodec_2x";
+			resets = <&tegra_car 125>, /* hda */
+				 <&tegra_car 128>, /* hda2hdmi */
+				 <&tegra_car 111>; /* hda2codec_2x */
+			reset-names = "hda", "hda2hdmi", "hdacodec_2x";
+			status = "disabled";
+		};
+
+		xusb_padctl: xusb_padctl@0,7009f000 {
+			compatible = "nvidia,tegra132-xusb-padctl", "nvidia,tegra124-xusb-padctl";
+			reg = <0x0 0x7009f000 0x0 0x1000>;
+			resets = <&tegra_car 142>;
+			reset-names = "padctl"; /* XXX support an xusb_padctl alias */
+
+			#phy-cells = <1>;
+		};
+
+		sdmmc1: sdhci@0,700b0000 {
+			compatible = "nvidia,tegra132-sdhci", "nvidia,tegra124-sdhci", "nvidia,tegra114-sdhci";
+			reg = <0x0 0x700b0000 0x0 0x200>;
+			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_SDMMC1>;
+			resets = <&tegra_car 14>;
+			reset-names = "sdhci";
+			status = "disabled";
+		};
+
+		sdmmc2: sdhci@0,700b0200 {
+			compatible = "nvidia,tegra132-sdhci", "nvidia,tegra124-sdhci", "nvidia,tegra114-sdhci";
+			reg = <0x0 0x700b0200 0x0 0x200>;
+			interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_SDMMC2>;
+			resets = <&tegra_car 9>;
+			reset-names = "sdhci";
+			status = "disabled";
+		};
+
+		sdmmc3: sdhci@0,700b0400 {
+			compatible = "nvidia,tegra132-sdhci", "nvidia,tegra124-sdhci", "nvidia,tegra114-sdhci";
+			reg = <0x0 0x700b0400 0x0 0x200>;
+			interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_SDMMC3>;
+			resets = <&tegra_car 69>;
+			reset-names = "sdhci";
+			status = "disabled";
+		};
+
+		sdmmc4: sdhci@0,700b0600 {
+			compatible = "nvidia,tegra132-sdhci", "nvidia,tegra124-sdhci", "nvidia,tegra114-sdhci";
+			reg = <0x0 0x700b0600 0x0 0x200>;
+			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_SDMMC4>;
+			resets = <&tegra_car 15>;
+			reset-names = "sdhci";
+			status = "disabled";
+		};
+
+		soc_therm: thermal-sensor@0,700e2000 {
+			compatible = "nvidia,tegra132-soctherm", "nvidia,tegra124-soctherm";
+			reg = <0x0 0x700e2000 0x0 0x1000>;
+			interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_TSENSOR>,
+				<&tegra_car TEGRA124_CLK_SOC_THERM>;
+			clock-names = "tsensor", "soctherm";
+			resets = <&tegra_car 78>;
+			reset-names = "soctherm";
+			#thermal-sensor-cells = <1>;
+		};
+
+		audio_cluster: ahub@0,70300000 {
+			compatible = "nvidia,tegra132-ahub", "nvidia,tegra124-ahub";
+			reg = <0x0 0x70300000 0x0 0x200>,
+			      <0x0 0x70300800 0x0 0x800>,
+			      <0x0 0x70300200 0x0 0x600>;
+			interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&tegra_car TEGRA124_CLK_D_AUDIO>,
+				 <&tegra_car TEGRA124_CLK_APBIF>;
+			clock-names = "d_audio", "apbif";
+			resets = <&tegra_car 106>, /* d_audio */
+				 <&tegra_car 107>, /* apbif */
+				 <&tegra_car 30>,  /* i2s0 */
+				 <&tegra_car 11>,  /* i2s1 */
+				 <&tegra_car 18>,  /* i2s2 */
+				 <&tegra_car 101>, /* i2s3 */
+				 <&tegra_car 102>, /* i2s4 */
+				 <&tegra_car 108>, /* dam0 */
+				 <&tegra_car 109>, /* dam1 */
+				 <&tegra_car 110>, /* dam2 */
+				 <&tegra_car 10>,  /* spdif */
+				 <&tegra_car 153>, /* amx */
+				 <&tegra_car 185>, /* amx1 */
+				 <&tegra_car 154>, /* adx */
+				 <&tegra_car 180>, /* adx1 */
+				 <&tegra_car 186>, /* afc0 */
+				 <&tegra_car 187>, /* afc1 */
+				 <&tegra_car 188>, /* afc2 */
+				 <&tegra_car 189>, /* afc3 */
+				 <&tegra_car 190>, /* afc4 */
+				 <&tegra_car 191>; /* afc5 */
+			reset-names = "d_audio", "apbif", "i2s0", "i2s1", "i2s2",
+				      "i2s3", "i2s4", "dam0", "dam1", "dam2",
+				      "spdif", "amx", "amx1", "adx", "adx1",
+				      "afc0", "afc1", "afc2", "afc3", "afc4", "afc5";
+			dmas = <&apbdma 1>, <&apbdma 1>,
+			       <&apbdma 2>, <&apbdma 2>,
+			       <&apbdma 3>, <&apbdma 3>,
+			       <&apbdma 4>, <&apbdma 4>,
+			       <&apbdma 6>, <&apbdma 6>,
+			       <&apbdma 7>, <&apbdma 7>,
+			       <&apbdma 12>, <&apbdma 12>,
+			       <&apbdma 13>, <&apbdma 13>,
+			       <&apbdma 14>, <&apbdma 14>,
+			       <&apbdma 29>, <&apbdma 29>;
+			dma-names = "rx0", "tx0", "rx1", "tx1", "rx2", "tx2",
+				    "rx3", "tx3", "rx4", "tx4", "rx5", "tx5",
+				    "rx6", "tx6", "rx7", "tx7", "rx8", "tx8",
+				    "rx9", "tx9";
+			ranges;
+			#address-cells = <2>;
+			#size-cells = <2>;
+
+			i2s0: i2s@0,70301000 {
+				compatible = "nvidia,tegra132-i2s", "nvidia,tegra124-i2s";
+				reg = <0x0 0x70301000 0x0 0x100>;
+				nvidia,ahub-cif-ids = <4 4>;
+				clocks = <&tegra_car TEGRA124_CLK_I2S0>;
+				resets = <&tegra_car 30>;
+				reset-names = "i2s";
+				status = "disabled";
+			};
+
+			i2s1: i2s@0,70301100 {
+				compatible = "nvidia,tegra132-i2s", "nvidia,tegra124-i2s";
+				reg = <0x0 0x70301100 0x0 0x100>;
+				nvidia,ahub-cif-ids = <5 5>;
+				clocks = <&tegra_car TEGRA124_CLK_I2S1>;
+				resets = <&tegra_car 11>;
+				reset-names = "i2s";
+				status = "disabled";
+			};
+
+			i2s2: i2s@0,70301200 {
+				compatible = "nvidia,tegra132-i2s", "nvidia,tegra124-i2s";
+				reg = <0x0 0x70301200 0x0 0x100>;
+				nvidia,ahub-cif-ids = <6 6>;
+				clocks = <&tegra_car TEGRA124_CLK_I2S2>;
+				resets = <&tegra_car 18>;
+				reset-names = "i2s";
+				status = "disabled";
+			};
+
+			i2s3: i2s@0,70301300 {
+				compatible = "nvidia,tegra132-i2s", "nvidia,tegra124-i2s";
+				reg = <0x0 0x70301300 0x0 0x100>;
+				nvidia,ahub-cif-ids = <7 7>;
+				clocks = <&tegra_car TEGRA124_CLK_I2S3>;
+				resets = <&tegra_car 101>;
+				reset-names = "i2s";
+				status = "disabled";
+			};
+
+			i2s4: i2s@0,70301400 {
+				compatible = "nvidia,tegra132-i2s", "nvidia,tegra124-i2s";
+				reg = <0x0 0x70301400 0x0 0x100>;
+				nvidia,ahub-cif-ids = <8 8>;
+				clocks = <&tegra_car TEGRA124_CLK_I2S4>;
+				resets = <&tegra_car 102>;
+				reset-names = "i2s";
+				status = "disabled";
+			};
+		};
+
+	}; /* apb */
+
+	ahb_a2: ahb@0,7c000000 {
+		compatible = "simple-bus";
+		reg = <0x0 0x7c000000 0x0 0x02000000>;
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		usb1: usb@0,7d000000 {
+			compatible = "nvidia,tegra132-ehci", "nvidia,tegra124-ehci", "nvidia,tegra30-ehci", "usb-ehci";
+			reg = <0x0 0x7d000000 0x0 0x4000>;
+			interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;
+			phy_type = "utmi";
+			clocks = <&tegra_car TEGRA124_CLK_USBD>;
+			resets = <&tegra_car 22>;
+			reset-names = "usb";
+			nvidia,phy = <&phy1>;
+			status = "disabled";
+		};
+
+		phy1: usb-phy@0,7d000000 {
+			compatible = "nvidia,tegra132-usb-phy", "nvidia,tegra124-usb-phy", "nvidia,tegra30-usb-phy";
+			reg = <0x0 0x7d000000 0x0 0x4000>,
+			      <0x0 0x7d000000 0x0 0x4000>;
+			phy_type = "utmi";
+			clocks = <&tegra_car TEGRA124_CLK_USBD>,
+				 <&tegra_car TEGRA124_CLK_PLL_U>,
+				 <&tegra_car TEGRA124_CLK_USBD>;
+			clock-names = "reg", "pll_u", "utmi-pads";
+			resets = <&tegra_car 59>, <&tegra_car 22>;
+			reset-names = "usb", "utmi-pads";
+			nvidia,hssync-start-delay = <0>;
+			nvidia,idle-wait-delay = <17>;
+			nvidia,elastic-limit = <16>;
+			nvidia,term-range-adj = <6>;
+			nvidia,xcvr-setup = <9>;
+			nvidia,xcvr-lsfslew = <0>;
+			nvidia,xcvr-lsrslew = <3>;
+			nvidia,hssquelch-level = <2>;
+			nvidia,hsdiscon-level = <5>;
+			nvidia,xcvr-hsslew = <12>;
+			status = "disabled";
+		};
+
+		usb2: usb@0,7d004000 {
+			compatible = "nvidia,tegra132-ehci", "nvidia,tegra124-ehci", "nvidia,tegra30-ehci", "usb-ehci";
+			reg = <0x0 0x7d004000 0x0 0x4000>;
+			interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			phy_type = "utmi";
+			clocks = <&tegra_car TEGRA124_CLK_USB2>;
+			resets = <&tegra_car 58>;
+			reset-names = "usb";
+			nvidia,phy = <&phy2>;
+			status = "disabled";
+		};
+
+		phy2: usb-phy@0,7d004000 {
+			compatible = "nvidia,tegra132-usb-phy", "nvidia,tegra124-usb-phy", "nvidia,tegra30-usb-phy";
+			reg = <0x0 0x7d004000 0x0 0x4000>,
+			      <0x0 0x7d000000 0x0 0x4000>;
+			phy_type = "utmi";
+			clocks = <&tegra_car TEGRA124_CLK_USB2>,
+				 <&tegra_car TEGRA124_CLK_PLL_U>,
+				 <&tegra_car TEGRA124_CLK_USBD>;
+			clock-names = "reg", "pll_u", "utmi-pads";
+			resets = <&tegra_car 22>, <&tegra_car 22>;
+			reset-names = "usb", "utmi-pads";
+			nvidia,hssync-start-delay = <0>;
+			nvidia,idle-wait-delay = <17>;
+			nvidia,elastic-limit = <16>;
+			nvidia,term-range-adj = <6>;
+			nvidia,xcvr-setup = <9>;
+			nvidia,xcvr-lsfslew = <0>;
+			nvidia,xcvr-lsrslew = <3>;
+			nvidia,hssquelch-level = <2>;
+			nvidia,hsdiscon-level = <5>;
+			nvidia,xcvr-hsslew = <12>;
+			nvidia,has-utmi-pad-registers;
+			status = "disabled";
+		};
+
+		usb3: usb@0,7d008000 {
+			compatible = "nvidia,tegra132-ehci", "nvidia,tegra124-ehci", "nvidia,tegra30-ehci", "usb-ehci";
+			reg = <0x0 0x7d008000 0x0 0x4000>;
+			interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
+			phy_type = "utmi";
+			clocks = <&tegra_car TEGRA124_CLK_USB3>;
+			resets = <&tegra_car 59>;
+			reset-names = "usb";
+			nvidia,phy = <&phy3>;
+			status = "disabled";
+		};
+
+		phy3: usb-phy@0,7d008000 {
+			compatible = "nvidia,tegra132-usb-phy", "nvidia,tegra124-usb-phy", "nvidia,tegra30-usb-phy";
+			reg = <0x0 0x7d008000 0x0 0x4000>,
+			      <0x0 0x7d000000 0x0 0x4000>;
+			phy_type = "utmi";
+			clocks = <&tegra_car TEGRA124_CLK_USB3>,
+				 <&tegra_car TEGRA124_CLK_PLL_U>,
+				 <&tegra_car TEGRA124_CLK_USBD>;
+			clock-names = "reg", "pll_u", "utmi-pads";
+			resets = <&tegra_car 58>, <&tegra_car 22>;
+			reset-names = "usb", "utmi-pads";
+			nvidia,hssync-start-delay = <0>;
+			nvidia,idle-wait-delay = <17>;
+			nvidia,elastic-limit = <16>;
+			nvidia,term-range-adj = <6>;
+			nvidia,xcvr-setup = <9>;
+			nvidia,xcvr-lsfslew = <0>;
+			nvidia,xcvr-lsrslew = <3>;
+			nvidia,hssquelch-level = <2>;
+			nvidia,hsdiscon-level = <5>;
+			nvidia,xcvr-hsslew = <12>;
+			status = "disabled";
+		};
+
+	}; /* ahb_a2 */
+
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "nvidia,denver", "arm,armv8";
+			reg = <0x0 0x0>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0x0 0x80000008>;
+		};
+
+		cpu@1 {
+			device_type = "cpu";
+			compatible = "nvidia,denver", "arm,armv8";
+			reg = <0x0 0x1>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0x0 0x80000008>;
+		};
+	};
+
+	thermal-zones {
+		/* XXX T132 CPU thermal zone - still TBD */
+
+		mem {
+			thermal-sensors =
+				<&soc_therm TEGRA124_SOCTHERM_SENSOR_MEM>;
+		};
+
+		gpu {
+			thermal-sensors =
+				<&soc_therm TEGRA124_SOCTHERM_SENSOR_GPU>;
+		};
+
+		pllx {
+			thermal-sensors =
+				<&soc_therm TEGRA124_SOCTHERM_SENSOR_PLLX>;
+		};
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupts = <GIC_PPI 13
+				(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 14
+				(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 11
+				(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 10
+				(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+	};
+};