[PATCH v5 4/4] ARM: hi3xxx: enable hi4511 with device tree
Olof Johansson
olof at lixom.net
Mon Jun 17 18:05:40 EDT 2013
Hi,
Some comments below. I didn't research the history of reviews so some might
already have been pointed out and debated; apoligies if they have.
You should also post the device-tree bindings, in particular the clock ones
since they seem to be a bit different from what else we have seen so far, to
device-tree discuss and get them reviewed by Grant/Rob as well as Mike.
-Olof
On Mon, Jun 17, 2013 at 04:56:16PM +0800, Haojian Zhuang wrote:
> Enable Hisilicon Hi4511 development platform with device tree support.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang at linaro.org>
> ---
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/hi3620.dtsi | 1147 +++++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/hi4511.dts | 648 ++++++++++++++++++++
> arch/arm/configs/multi_v7_defconfig | 1 +
> 4 files changed, 1797 insertions(+)
> create mode 100644 arch/arm/boot/dts/hi3620.dtsi
> create mode 100644 arch/arm/boot/dts/hi4511.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index f0895c5..216f983 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -58,6 +58,7 @@ dtb-$(CONFIG_ARCH_EXYNOS) += exynos4210-origen.dtb \
> exynos5250-smdk5250.dtb \
> exynos5250-snow.dtb \
> exynos5440-ssdk5440.dtb
> +dtb-$(CONFIG_ARCH_HI3xxx) += hi4511.dtb
> dtb-$(CONFIG_ARCH_HIGHBANK) += highbank.dtb \
> ecx-2000.dtb
> dtb-$(CONFIG_ARCH_INTEGRATOR) += integratorap.dtb \
> diff --git a/arch/arm/boot/dts/hi3620.dtsi b/arch/arm/boot/dts/hi3620.dtsi
> new file mode 100644
> index 0000000..1f0a4aa
> --- /dev/null
> +++ b/arch/arm/boot/dts/hi3620.dtsi
> @@ -0,0 +1,1147 @@
> +/*
> + * Hisilicon Ltd. Hi3620 SoC
> + *
> + * Copyright (C) 2012-2013 Hisilicon Ltd.
> + * Copyright (C) 2012-2013 Linaro Ltd.
> + *
> + * Author: Haojian Zhuang <haojian.zhuang at linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * publishhed by the Free Software Foundation.
> + */
> +
> +/include/ "skeleton.dtsi"
> +
> +/ {
> + aliases {
> + serial0 = &uart0;
> + serial1 = &uart1;
> + serial2 = &uart2;
> + serial3 = &uart3;
> + serial4 = &uart4;
> + };
I would expect a cpus entry around here...?
And a #size-cells and #address-cells.
> +
> + osc32k: osc at 0 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <32768>;
> + clock-output-names = "osc32khz";
> + };
> + osc26m: osc at 1 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <26000000>;
> + clock-output-names = "osc26mhz";
> + };
> + pclk: clk at 0 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <26000000>;
> + clock-output-names = "apb_pclk";
> + };
These are not good names. There's nothing inherently addressable by them but
you still have unit addresses on them. Also, if they have an unit address @<x>,
then they need a reg property. It's probably better to give them unique names
(and not abbreviated). "clock-armpclk". Or something.
> + pll_arm0: clk at 1 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <1600000000>;
> + clock-output-names = "armpll0";
> + };
> + pll_arm1: clk at 2 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <1600000000>;
> + clock-output-names = "armpll1";
> + };
> + pll_peri: clk at 3 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <1440000000>;
> + clock-output-names = "armpll2";
> + };
> + pll_usb: clk at 4 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <1440000000>;
> + clock-output-names = "armpll3";
> + };
> + pll_hdmi: clk at 5 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <1188000000>;
> + clock-output-names = "armpll4";
> + };
> + pll_gpu: clk at 6 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <1300000000>;
> + clock-output-names = "armpll5";
> + };
Are these clocks really just there on the board, or are they provided from
a PMIC or similar? Even if you don't have a driver for that chip, describing
the hardware correctly now (with the exception of calling the clocks
fixed-clock) seems like a good idea.
> +
> + amba {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "arm,amba-bus";
> + interrupt-parent = <&intc>;
> + ranges;
> +
> + pmctrl: pmctrl at fca08000 {
> + compatible = "hisilicon,pmctrl";
> + reg = <0xfca08000 0x1000>;
> + };
[...]
> + dtable: dtable {
> + #hisilicon,clkdiv-table-cells = <2>;
This definitely needs some review by the devicetree guys, Rob/Grant. It seems
like a quite unusual binding. What hardware does dtable represent?
> + };
> + div_shareaxi: div_shareaxi {
> + compatible = "hisilicon,hi3620-clk-div";
> + #clock-cells = <0>;
> + clocks = <&rclk_shareAXI>;
> + clock-output-names = "shareAXI_div";
> + hisilicon,clkdiv-table = <
> + &dtable 0 1 &dtable 1 2 &dtable 2 3 &dtable 3 4
> + &dtable 4 5 &dtable 5 6 &dtable 6 7 &dtable 7 8
> + &dtable 8 9 &dtable 9 10 &dtable 10 11 &dtable 11 12
> + &dtable 12 13 &dtable 13 14 &dtable 14 15 &dtable 15 16
> + &dtable 16 17 &dtable 17 18 &dtable 18 19 &dtable 19 20
> + &dtable 20 21 &dtable 21 22 &dtable 22 23 &dtable 23 24
> + &dtable 24 25 &dtable 25 26 &dtable 26 27 &dtable 27 28
> + &dtable 28 29 &dtable 29 30 &dtable 30 31 &dtable 31 32>;
> + /* divider register offset, mask */
> + hisilicon,clkdiv = <0x100 0x1f>;
> + };
> +
> + l2: l2-cache {
> + compatible = "arm,pl310-cache";
> + reg = <0xfc10000 0x100000>;
> + interrupts = <0 15 4>;
> + cache-unified;
> + cache-level = <2>;
> + };
L2 is on amba?
> +
> + intc: interrupt-controller at fc001000 {
It's common to use "gic" as the label instead.
> + compatible = "arm,cortex-a9-gic";
> + #interrupt-cells = <3>;
> + #address-cells = <0>;
> + interrupt-controller;
> + /* gic dist base, gic cpu base */
> + reg = <0xfc001000 0x1000>, <0xfc000100 0x100>;
> + };
> +
[...]
-Olof
More information about the linux-arm-kernel
mailing list