Re: [PATCH] ARM64: dts: rockchip: add core dtsi file for RK3399 SoCs

Heiko Stübner
2016-04-21T22:03:04Z

Hi Jay,

Am Donnerstag, 21. April 2016, 11:58:12 schrieb Jianqun Xu:
> This patch adds rk3399.dtsi for rk3399 found on Rockchip
> RK3399 SoCs, also add rk3399-evb.dts for Rockchip RK3399
> Evaluation Board.
>
> Patch is tested on RK3399 evb.
>
> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>

please split this into
- patch adding the dtsi
- patch adding the evb dts
- patch adding the new board to bindings/arm/rockchip.txt

more inline below

> ---
> arch/arm64/boot/dts/rockchip/Makefile | 1 +
> arch/arm64/boot/dts/rockchip/rk3399-evb.dts | 537 ++++++++
> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1757
> +++++++++++++++++++++++++++ 3 files changed, 2295 insertions(+)
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-evb.dts
> create mode 100644 arch/arm64/boot/dts/rockchip/rk3399.dtsi
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
> b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts new file mode 100644
> index 0000000..4cb0028
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts
> @@ -0,0 +1,537 @@
> +/*
> + * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/pwm/pwm.h>
> +#include "rk3399.dtsi"
> +
> +/ {
> + model = "Rockchip RK3399 Evaluation Board";
> + compatible = "rockchip,evb", "rockchip,rk3399-evb";
> +
> + chosen {
> + bootargs = "console=uart,mmio32,0xff1a0000";

I'd think we'll want a
stdout-path = something
property here, instead of hard-coding bootargs.

[...]

> +&i2c4 {
> + status = "okay";
> + i2c-scl-rising-time-ns = <600>;
> + i2c-scl-falling-time-ns = <20>;
> +
> + gt9xx: gt9xx@14 {
> + compatible = "goodix,gt9xx";

same as Rob said for the ramoops, I don't see this one in the devicetree
bindings. Also gt9xx should instead specify an actual chip, not a chip-family.
See drivers/input/touchscreen/goodix.c and
Documentation/devicetree/bindings/input/touchscreen for supported chips and
the real devicetree bindings.

> + reg = <0x14>;
> + touch-gpio = <&gpio1 20 IRQ_TYPE_LEVEL_LOW>;
> + reset-gpio = <&gpio4 22 GPIO_ACTIVE_HIGH>;
> + max-x = <1200>;
> + max-y = <1900>;
> + tp-size = <911>;
> + tp-supply = <&vcc3v0_tp>;
> + };
> +};

[...]

> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi new file mode 100644
> index 0000000..7c3015c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -0,0 +1,1757 @@
> +/*
> + * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <dt-bindings/clock/rk3399-cru.h>
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include <dt-bindings/power/rk3399-power.h>
> +#include <dt-bindings/thermal/thermal.h>
> +
> +/ {

[...]

> + sdhci: sdhci@fe330000 {
> + compatible = "arasan,sdhci-5.1";

not 100% sure, but we might want a
compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";

allowing us to get more specific, if implementation oddities surface later.

> + reg = <0x0 0xfe330000 0x0 0x10000>;
> + interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
> + clock-names = "clk_xin", "clk_ahb";
> + phys = <&emmc_phy>;
> + phy-names = "phy_arasan";
> + status = "disabled";
> + };
> +
> + usb2phy: usb2phy {
> + compatible = "rockchip,rk3399-usb-phy";

this doesn't look like it got submitted yet.

Also, the newer socs (rk3399. rk3036, rk3228) seem to use a different usbphy
block than rk3288 and before (with a big bunch of new phy-related register
blocks I haven't looked at yet) - so this should probably get a new driver as
well and not be crammed into the current phy driver, which is for the older
picophy (or what it was called).

> + rockchip,grf = <&grf>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + usb2phy0: usb2-phy0 {
> + #phy-cells = <0>;
> + #clock-cells = <0>;
> + reg = <0xe458>;
> + };

When we're doing a new driver, could we please get rid of these subnodes and
instead access phys via something like

phys = <&usb2phy 0>;

> +
> + usb2phy1: usb2-phy1 {
> + #phy-cells = <0>;
> + #clock-cells = <0>;
> + reg = <0xe468>;
> + };
> + };
> +
> + usb_host0_echi: usb@fe380000 {

not "echi" please :-)

> + compatible = "generic-ehci";
> + reg = <0x0 0xfe380000 0x0 0x20000>;
> + interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
> + clock-names = "hclk_host0", "hclk_host0_arb";
> + phys = <&usb2phy0>;
> + phy-names = "usb2_phy0";
> + status = "disabled";
> + };

[...]

> + usbdrd3_0: usb@fe800000 {
> + compatible = "rockchip,dwc3";

is this in some tree already?

> + clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>,
> + <&cru ACLK_USB3OTG0>, <&cru ACLK_USB3_RKSOC_AXI_PERF>,
> + <&cru ACLK_USB3>, <&cru ACLK_USB3_GRF>;
> + clock-names = "clk_usb3otg0_ref", "clk_usb3otg0_suspend",
> + "aclk_usb3otg0", "aclk_usb3_rksoc_axi_perf",
> + "aclk_usb3", "aclk_usb3_grf";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> + status = "disabled";
> + usbdrd_dwc3_0: dwc3 {
> + compatible = "snps,dwc3";
> + reg = <0x0 0xfe800000 0x0 0x100000>;
> + interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
> + dr_mode = "otg";
> + tx-fifo-resize;
> + snps,dis_enblslpm_quirk;
> + snps,phyif_utmi_16_bits;
> + snps,dis_u2_freeclk_exists_quirk;
> + snps,dis_del_phy_power_chg_quirk;
> + status = "disabled";
> + };
> + };
> +
> + usbdrd3_1: usb@fe900000 {
> + compatible = "rockchip,dwc3";

same here

> + clocks = <&cru SCLK_USB3OTG1_REF>, <&cru SCLK_USB3OTG1_SUSPEND>,
> + <&cru ACLK_USB3OTG1>, <&cru ACLK_USB3_RKSOC_AXI_PERF>,
> + <&cru ACLK_USB3>, <&cru ACLK_USB3_GRF>;
> + clock-names = "clk_usb3otg1_ref", "clk_usb3otg1_suspend",
> + "aclk_usb3otg1", "aclk_usb3_rksoc_axi_perf",
> + "aclk_usb3", "aclk_usb3_grf";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> + status = "disabled";
> + usbdrd_dwc3_1: dwc3 {
> + compatible = "snps,dwc3";
> + reg = <0x0 0xfe900000 0x0 0x100000>;
> + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> + dr_mode = "otg";
> + tx-fifo-resize;
> + snps,dis_enblslpm_quirk;
> + snps,phyif_utmi_16_bits;
> + snps,dis_u2_freeclk_exists_quirk;
> + snps,dis_del_phy_power_chg_quirk;
> + status = "disabled";
> + };
> + };
> +

[...]

> + i2c1: i2c@ff110000 {
> + compatible = "rockchip,rk3399-i2c";

David respun the rk3399 i2c-support on tuesday, so this and the others below
are waiting on Wolfram to take a look.

> + reg = <0x0 0xff110000 0x0 0x1000>;
> + clocks = <&cru SCLK_I2C1>, <&cru PCLK_I2C1>;
> + clock-names = "i2c", "pclk";
> + interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c1_xfer>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };

[...]

> + pmu: power-management@ff31000 {

address is missing "0" :-)

[...]

> + };

[...]

> + i2c0: i2c@ff3c0000 {
> + compatible = "rockchip,rk3399-i2c";
> + reg = <0x0 0xff3c0000 0x0 0x1000>;
> + clocks = <&pmucru SCLK_I2C0_PMU>, <&pmucru PCLK_I2C0_PMU>;
> + clock-names = "i2c", "pclk";
> + interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c0_xfer>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + i2c4: i2c@ff3d0000 {
> + compatible = "rockchip,rk3399-i2c";
> + reg = <0x0 0xff3d0000 0x0 0x1000>;
> + clocks = <&pmucru SCLK_I2C4_PMU>, <&pmucru PCLK_I2C4_PMU>;
> + clock-names = "i2c", "pclk";
> + interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c4_xfer>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };
> +
> + i2c8: i2c@ff3e0000 {
> + compatible = "rockchip,rk3399-i2c";
> + reg = <0x0 0xff3e0000 0x0 0x1000>;
> + clocks = <&pmucru SCLK_I2C8_PMU>, <&pmucru PCLK_I2C8_PMU>;
> + clock-names = "i2c", "pclk";
> + interrupts = <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c8_xfer>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> + };

> + rga: rga@ff680000 {
> + compatible = "rockchip,rk3399-rga";

not yet accepted component, please leave out for now

> + reg = <0x0 0xff680000 0x0 0x10000>;
> + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "rga";
> + clocks = <&cru ACLK_RGA>, <&cru HCLK_RGA>, <&cru SCLK_RGA_CORE>;
> + clock-names = "aclk", "hclk", "sclk";
> + resets = <&cru SRST_RGA_CORE>, <&cru SRST_A_RGA>, <&cru SRST_H_RGA>;
> + reset-names = "core", "axi", "ahb";
> + status = "disabled";
> + };
> +

[...]

> + gpu: gpu@ff9a0000 {
> + compatible = "arm,malit860",
> + "arm,malit86x",
> + "arm,malit8xx",
> + "arm,mali-midgard";

mali kernel-part is out-of-tree code with a unreviewed binding, so should not
be part of the mainline devicetree

> + reg = <0x0 0xff9a0000 0x0 0x10000>;
> + interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "GPU", "JOB", "MMU";
> +
> + clocks = <&cru ACLK_GPU>;
> + clock-names = "clk_mali";
> + #cooling-cells = <2>; /* min followed by max */
> + status = "disabled";
> + };

[...]

> + mipi_dsi: mipi@ff960000 {
> + compatible = "rockchip,rk3399-mipi-dsi", "snps,dw-mipi-dsi";

missing binding in the kernel

> + reg = <0x0 0xff960000 0x0 0x8000>;
> + interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru SCLK_MIPIDPHY_REF>, <&cru PCLK_MIPI_DSI0>,
> + <&cru SCLK_DPHY_TX0_CFG>;
> + clock-names = "ref", "pclk", "phy_cfg";
> + rockchip,grf = <&grf>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> +
> + mipi_in: port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mipi_in_vopb: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&vopb_out_mipi>;
> + };
> + mipi_in_vopl: endpoint@1 {
> + reg = <1>;
> + remote-endpoint = <&vopl_out_mipi>;
> + };
> + };
> + };
> + };
> +
> + edp: edp@ff970000 {
> + compatible = "rockchip,rk3399-edp";

missing binding in the kernel?

> + reg = <0x0 0xff970000 0x0 0x8000>;
> + interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&cru PCLK_EDP>, <&cru PCLK_EDP_CTRL>;
> + clock-names = "dp", "pclk";
> + resets = <&cru SRST_P_EDP_CTRL>;
> + reset-names = "dp";
> + rockchip,grf = <&grf>;
> + status = "disabled";
> + pinctrl-names = "default";
> + pinctrl-0 = <&edp_hpd>;

I think the hotplug detection is pretty optional, so should live in the board
files instead

> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + edp_in: port@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + edp_in_vopb: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&vopb_out_edp>;
> + };
> +
> + edp_in_vopl: endpoint@1 {
> + reg = <1>;
> + remote-endpoint = <&vopl_out_edp>;
> + };
> + };
> + };
> + };