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@xxxxxxxxxx> and the Tegra124 and Tegra114 DTS files.
>
> Signed-off-by: Paul Walmsley <paul@xxxxxxxxx>
> Cc: Paul Walmsley <pwalmsley@xxxxxxxxxx>
> Cc: Allen Martin <amartin@xxxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Pawel Moll <pawel.moll@xxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Ian Campbell <ijc+devicetree@xxxxxxxxxxxxxx>
> Cc: Kumar Gala <galak@xxxxxxxxxxxxxx>
> Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
> Cc: Stephen Warren <swarren@xxxxxxxxxxxxx>
> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Alexandre Courbot <gnurou@xxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-tegra@xxxxxxxxxxxxxxx
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
