Re: [RFC 1/4] arm64: amd-seattle: Adding device tree for AMD Seattle platform

From: Mark Rutland
Date: Fri Oct 10 2014 - 09:45:49 EST

Next message: Alexandre Belloni: "[PATCH] ARM: at91/dt: sam9263: fix PLLB frequencies"
Previous message: Takashi Iwai: "[GIT PULL] sound updates for 3.18"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Suravee,

On Sun, Sep 28, 2014 at 09:53:27PM +0100, suravee.suthikulpanit@xxxxxxx wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
>
> Initial revision of device tree for AMD Seattle platform

To check: how is it possible to make use of a DTB generated from this
dts? Can a user update the DTB used by the Seattle firmware?

>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> Signed-off-by: Thomas Lendacky <Thomas.Lendacky@xxxxxxx>
> Signed-off-by: Joel Schopp <Joel.Schopp@xxxxxxx>
> ---
> arch/arm64/boot/dts/Makefile | 1 +
> arch/arm64/boot/dts/amd-seattle-periph.dtsi | 175 ++++++++++++++++++++
> arch/arm64/boot/dts/amd-seattle.dts | 245 ++++++++++++++++++++++++++++
> 3 files changed, 421 insertions(+)
> create mode 100644 arch/arm64/boot/dts/amd-seattle-periph.dtsi
> create mode 100644 arch/arm64/boot/dts/amd-seattle.dts
>
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index c52bdb0..11cb2e3 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -1,5 +1,6 @@
> dtb-$(CONFIG_ARCH_VEXPRESS) += rtsm_ve-aemv8a.dtb foundation-v8.dtb
> dtb-$(CONFIG_ARCH_XGENE) += apm-mustang.dtb
> +dtb-$(CONFIG_ARCH_SEATTLE) += amd-seattle.dtb
>
> targets += dtbs
> targets += $(dtb-y)
> diff --git a/arch/arm64/boot/dts/amd-seattle-periph.dtsi b/arch/arm64/boot/dts/amd-seattle-periph.dtsi
> new file mode 100644
> index 0000000..e5bcf1c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/amd-seattle-periph.dtsi
> @@ -0,0 +1,175 @@
> +/*
> + * DTS file for AMD Seattle Peripheral
> + *
> + * Copyright (C) 2014 Advanced Micro Devices, Inc.
> + */
> +
> +motherboard {
> + arm,v2m-memory-map = "rs1";
> + compatible = "arm,vexpress,v2m-p1", "simple-bus";

The v2m stuff above can go. This isn't a versatile express, and we won't
use those properties anyway.

> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> + adl3clk_100mhz: clk100mhz_0 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <100000000>;
> + clock-output-names = "adl3clk_100mhz";
> + };
> +
> + ccpclk_375mhz: clk375mhz {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <375000000>;
> + clock-output-names = "ccpclk_375mhz";
> + };
> +
> + sataclk_333mhz: clk333mhz {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <333000000>;
> + clock-output-names = "sataclk_333mhz";
> + };
> +
> + pcieclk_500mhz: clk500mhz_0 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <500000000>;
> + clock-output-names = "pcieclk_500mhz";
> + };
> +
> + dmaclk_500mhz: clk500mhz_1 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <500000000>;
> + clock-output-names = "dmaclk_500mhz";
> + };
> +
> + miscclk_250mhz: clk250mhz_4 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <250000000>;
> + clock-output-names = "miscclk_250mhz";
> + };
> +
> + uartspiclk_100mhz: clk100mhz_1 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <100000000>;
> + clock-output-names = "uartspiclk_100mhz";
> + };
> +
> + dma0: dma@1,0500000 {
> + compatible = "arm,pl330", "arm,primecell";
> + reg = <0 0x0500000 0 0x1000>;
> + interrupts =
> + <0 368 4>,
> + <0 369 4>,
> + <0 370 4>,
> + <0 371 4>,
> + <0 372 4>,
> + <0 373 4>,
> + <0 374 4>,
> + <0 375 4>;
> + clocks = <&dmaclk_500mhz>;
> + clock-names = "apb_pclk";
> + #dma-cells = <1>;
> + #stream-id-cells = <32>;

I didn't spot an SMMU, so I think this should go.

> + };
> +
> + sata0: sata@1,00300000 {
> + compatible = "snps,spear-ahci";
> + reg = <0 0x300000 0 0x800>;
> + interrupts = <0 355 4>;
> + clocks = <&sataclk_333mhz>;
> + clock-names = "apb_pclk";
> + #stream-id-cells = <32>;

Likewise.

> + dma-coherent;
> + };
> +
> + i2c@1,1000000 {
> + compatible = "snps,designware-i2c";
> + reg = <0 0x01000000 0 0x1000>;
> + interrupts = <0 357 4>;
> + clocks = <&uartspiclk_100mhz>;
> + clock-names = "apb_pclk";
> + };
> +
> + v2m_serial0: uart@1,1010000 {
> + compatible = "arm,pl011", "arm,primecell";
> + reg = <0 0x1010000 0 0x1000>;
> + interrupts = <0 328 4>;
> + clocks = <&uartspiclk_100mhz>, <&uartspiclk_100mhz>;
> + clock-names = "uartclk", "apb_pclk";
> + };
> +
> + ssp@1,1020000 {
> + #gpio-cells = <2>;
> + compatible = "arm,pl022", "arm,primecell";

Please put the compatible property first in each node.

> + reg = <0 0x1020000 0 0x1000>;
> + spi-controller;
> + interrupts = <0 330 4>;
> + clocks = <&uartspiclk_100mhz>;
> + clock-names = "apb_pclk";
> + };
> +
> + ssp@1,1030000 {
> + #gpio-cells = <2>;
> + compatible = "arm,pl022", "arm,primecell";
> + reg = <0 0x1030000 0 0x1000>;
> + spi-controller;
> + interrupts = <0 329 4>;
> + clocks = <&uartspiclk_100mhz>;
> + clock-names = "apb_pclk";
> + num-cs = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + sdcard@1 {
> + compatible = "mmc-spi-slot";
> + reg = <0>;

The unit-address should match the first reg entry.

> + spi-max-frequency = <20000000>;
> + pl022,hierarchy = <0>;
> + pl022,interface = <0>;
> + pl022,com-mode = <0x0>;
> + pl022,rx-level-trig = <0>;
> + pl022,tx-level-trig = <0>;
> + };
> + };
> +
> + gpio@1,1040000 {
> + #gpio-cells = <2>;
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0 0x1040000 0 0x1000>;
> + gpio-controller;
> + interrupts = <0 359 4>;
> + clocks = <&uartspiclk_100mhz>;
> + clock-names = "apb_pclk";
> + };
> +
> + gpio@1,1050000 {
> + #gpio-cells = <2>;
> + compatible = "arm,pl061", "arm,primecell";
> + reg = <0 0x1050000 0 0x1000>;
> + gpio-controller;
> + interrupts = <0 358 4>;
> + clocks = <&uartspiclk_100mhz>;
> + clock-names = "apb_pclk";
> + };
> +
> + timer@1,1060000 {
> + compatible = "arm,standalone_a5_twd";
> + reg = <0 0x1060000 0 0x40>;
> + interrupts =
> + <0 378 4>,
> + <0 379 4>;
> + };

This binding does not exist in mainline.

> +
> + ccp: ccp@1,00100000 {
> + compatible = "amd,ccp-seattle-v1a";
> + reg = <0 0x00100000 0 0x10000>;
> + interrupts = <0 3 4>;
> + dma-coherent;
> + };

Nor does this.

> +};
> diff --git a/arch/arm64/boot/dts/amd-seattle.dts b/arch/arm64/boot/dts/amd-seattle.dts
> new file mode 100644
> index 0000000..3096d1a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/amd-seattle.dts
> @@ -0,0 +1,245 @@
> +/*
> + * DTS file for AMD Seattle
> + *
> + * Copyright (C) 2014 Advanced Micro Devices, Inc.
> + */
> +
> +/dts-v1/;
> +
> +/ {
> + compatible = "amd,seattle";
> + interrupt-parent = <&gic>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + chosen {
> + bootargs = "console=ttyAMA0,115200 earlycon=pl011,0xe1010000";

Please use stdout-path instead.

> + linux,pci-probe-only;

Why is this necessary?

> + };
> +
> + aliases {
> + serial0 = &v2m_serial0;
> + };
> +
> + /* Note: This entry is modified by UEFI */

In what way is this modified?

> + cpus {
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + cpu-map {
> + cluster0 {
> + core0 {
> + cpu = <&CPU0>;
> + };
> + core1 {
> + cpu = <&CPU1>;
> + };
> + };
> + cluster1 {
> + core0 {
> + cpu = <&CPU2>;
> + };
> + core1 {
> + cpu = <&CPU3>;
> + };
> + };
> + cluster2 {
> + core0 {
> + cpu = <&CPU4>;
> + };
> + core1 {
> + cpu = <&CPU5>;
> + };
> + };
> + cluster3 {
> + core0 {
> + cpu = <&CPU6>;
> + };
> + core1 {
> + cpu = <&CPU7>;
> + };
> + };
> + };
> + /* Cluster 0 Core 0 */
> + CPU0: cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,armv8";

Can we have the actual CPU name, please?

> + reg = <0x0 0x0000>;
> + enable-method = "spin-table";
> + cpu-release-addr = <0x80 0x30000050>;

Not PSCI?

> + };
> +
> + /* Cluster 0 Core 1 */
> + CPU1: cpu@1 {
> + device_type = "cpu";
> + compatible = "arm,armv8";
> + reg = <0x0 0x0001>;
> + enable-method = "spin-table";
> + cpu-release-addr = <0x80 0x30000058>;

At least the release addresses are unique...

> + };

[...]

> +
> + /* Note: This entry is modified by UEFI */
> + memory@8000000000 {
> + device_type = "memory";
> + reg = <0x00000080 0x00000000 0x1 0x00000000>; /* 4GB */
> + };

Why does UEFI modify this? When booted via UEFI we use the UEFI memory
map.

How exactly does UEFI modify this?

> +
> + gic: interrupt-controller@e1101000 {
> + compatible = "arm,gic-400", "arm,cortex-a15-gic";
> + #interrupt-cells = <3>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> + interrupt-controller;
> + ranges = <0 0 0 0xe1100000 0 0x100000>;

Please keep this together with #address-cells and #size-cells.

> + reg = <0x0 0xe1110000 0 0x1000>, /* gic dist */
> + <0x0 0xe112f000 0 0x2000>, /* gic cpu */
> + <0x0 0xe1140000 0 0x10000>, /* gic virtual ic*/
> + <0x0 0xe1160000 0 0x10000>; /* gic virtual cpu*/

The comments are confusing, because they don't match the architected
names. I would drop them.

> + interrupts = <1 8 0xf04>;
> + v2m0: v2m@0x8000 {
> + compatible = "arm,gic-v2m-frame";
> + msi-controller;
> + arm,msi-base-spi = <64>;
> + arm,msi-num-spis = <256>;
> + reg = <0x0 0x80000 0 0x1000>;
> + };
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <1 13 0xff01>,
> + <1 14 0xff01>,
> + <1 11 0xff01>,
> + <1 10 0xff01>;
> + };
> +
> + pmu {
> + compatible = "arm,armv8-pmuv3";
> + interrupts = <0 7 4>,
> + <0 8 4>,
> + <0 9 4>,
> + <0 10 4>,
> + <0 11 4>,
> + <0 12 4>,
> + <0 13 4>,
> + <0 14 4>;
> + };
> +
> + /* This entry is modified by UEFI */
> + pcie0: pcie-controller{
> + compatible = "pci-host-ecam-generic";

I unfortunately don't know enough about this binding to comment. I'll
leave that up to someone familiar with PCIe.

> + #address-cells = <3>;
> + #size-cells = <2>;
> + device_type = "pci";
> + bus-range = <0 0xff>;
> + reg = <0 0xf0000000 0 0x10000000>;
> + dma-coherent;
> + msi-parent = <&v2m0>;
> +
> + interrupts =
> + <0 320 4>, /* ioc_soc_serr */
> + <0 321 4>; /* ioc_soc_sci */
> +
> + ranges = <
> + /* I/O Memory (size=64K) */
> + 0x01000000 0x00 0xefff0000 0x00 0xefff0000 0x00 0x00010000

However, please bracket list entries individually.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Alexandre Belloni: "[PATCH] ARM: at91/dt: sam9263: fix PLLB frequencies"
Previous message: Takashi Iwai: "[GIT PULL] sound updates for 3.18"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]