diff mbox

[v15,07/12] ARM: dts: append hip04 dts

Message ID 1406555876-11989-8-git-send-email-haojian.zhuang@linaro.org
State Changes Requested
Headers show

Commit Message

Haojian Zhuang July 28, 2014, 1:57 p.m. UTC
Add hip04-d01.dts & hip04.dtsi for hip04 SoC platform.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 arch/arm/boot/dts/Makefile      |   1 +
 arch/arm/boot/dts/hip04-d01.dts |  39 ++++++
 arch/arm/boot/dts/hip04.dtsi    | 267 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 307 insertions(+)
 create mode 100644 arch/arm/boot/dts/hip04-d01.dts
 create mode 100644 arch/arm/boot/dts/hip04.dtsi

Comments

Mark Rutland July 28, 2014, 6:06 p.m. UTC | #1
On Mon, Jul 28, 2014 at 02:57:51PM +0100, Haojian Zhuang wrote:
> Add hip04-d01.dts & hip04.dtsi for hip04 SoC platform.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  arch/arm/boot/dts/Makefile      |   1 +
>  arch/arm/boot/dts/hip04-d01.dts |  39 ++++++
>  arch/arm/boot/dts/hip04.dtsi    | 267 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 307 insertions(+)
>  create mode 100644 arch/arm/boot/dts/hip04-d01.dts
>  create mode 100644 arch/arm/boot/dts/hip04.dtsi
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 721525e..6587bbf 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -86,6 +86,7 @@ dtb-$(CONFIG_ARCH_HI3xxx) += hi3620-hi4511.dtb
>  dtb-$(CONFIG_ARCH_HIX5HD2) += hix5hd2-dkb.dtb
>  dtb-$(CONFIG_ARCH_HIGHBANK) += highbank.dtb \
>         ecx-2000.dtb
> +dtb-$(CONFIG_ARCH_HIP04) += hip04-d01.dtb
>  dtb-$(CONFIG_ARCH_INTEGRATOR) += integratorap.dtb \
>         integratorcp.dtb
>  dtb-$(CONFIG_ARCH_KEYSTONE) += k2hk-evm.dtb \
> diff --git a/arch/arm/boot/dts/hip04-d01.dts b/arch/arm/boot/dts/hip04-d01.dts
> new file mode 100644
> index 0000000..661c8e5
> --- /dev/null
> +++ b/arch/arm/boot/dts/hip04-d01.dts
> @@ -0,0 +1,39 @@
> +/*
> + *  Copyright (C) 2013-2014 Linaro Ltd.
> + *  Author: Haojian Zhuang <haojian.zhuang@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.
> + */
> +
> +/dts-v1/;
> +
> +/* For bootwrapper */
> +/memreserve/ 0x10c00000 0x00010000;

How exactly is this bootwrapper used? Is the kernel compiled into it?

It might make more sense for the wrapper build system to inject
bootwrapper-related properties. Then the DTB is less likely to
amalgamate hacks to workaround differences between versions, and can be
used on systems without a wrapper without throwing away some memory.

> +
> +#include "hip04.dtsi"
> +
> +/ {
> +       /* memory bus is 64-bit */
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +       model = "Hisilicon D01 Development Board";
> +       compatible = "hisilicon,hip04-d01";
> +
> +       memory@00000000,10000000 {
> +               device_type = "memory";
> +               reg = <0x00000000 0x10000000 0x00000000 0xc0000000>;
> +       };
> +
> +       memory@00000004,c0000000 {
> +               device_type = "memory";
> +               reg = <0x00000004 0xc0000000 0x00000003 0x40000000>;
> +       };

You can fold these into a single node.

> +
> +       soc {
> +               uart0: uart@4007000 {
> +                       status = "ok";
> +               };
> +       };
> +};
> diff --git a/arch/arm/boot/dts/hip04.dtsi b/arch/arm/boot/dts/hip04.dtsi
> new file mode 100644
> index 0000000..30942be
> --- /dev/null
> +++ b/arch/arm/boot/dts/hip04.dtsi
> @@ -0,0 +1,267 @@
> +/*
> + * Hisilicon Ltd. HiP04 SoC
> + *
> + * Copyright (C) 2013-2014 Hisilicon Ltd.
> + * Copyright (C) 2013-2014 Linaro Ltd.
> + *
> + * Author: Haojian Zhuang <haojian.zhuang@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.

s/hh/h/

[...]

> +       clock: clock {
> +               compatible = "hisilicon,hip04-clock";
> +               /* dummy register.
> +                * Don't need to access clock registers since they're
> +                * configured in firmware already.
> +                */
> +               reg = <0 0 0 0x1000>;

Huh? Whether or not you need to access the registers should be up to the
kernel, not the DT.

Why can the kernel not access these? This sounds like a hack.

> +               #clock-cells = <1>;
> +       };
> +
> +       timer {
> +               compatible = "arm,armv7-timer";
> +               interrupt-parent = <&gic>;
> +               interrupts = <1 13 0xf08>,
> +                            <1 14 0xf08>,
> +                            <1 11 0xf08>,
> +                            <1 10 0xf08>;
> +       };
> +
> +       soc {
> +               /* It's a 32-bit SoC. */
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               compatible = "simple-bus";
> +               interrupt-parent = <&gic>;
> +               ranges = <0 0 0xe0000000 0x10000000>;
> +
> +               gic: interrupt-controller@c01000 {
> +                       compatible = "hisilicon,hip04-gic";
> +                       #interrupt-cells = <3>;
> +                       #address-cells = <0>;
> +                       interrupt-controller;
> +                       interrupts = <1 9 0xf04>;
> +
> +                       reg = <0xc01000 0x1000>, <0xc02000 0x1000>,
> +                             <0xc04000 0x2000>, <0xc06000 0x2000>;

Please place these on separate lines. It's easier to read and will match
what you've done for every other node.

> +               };
> +
> +               sysctrl: sysctrl {
> +                       compatible = "hisilicon,sysctrl";
> +                       reg = <0x3e00000 0x00100000>;
> +                       relocation-entry = <0xe0000100>;
> +                       relocation-size = <0x1000>;
> +                       bootwrapper-phys = <0x10c00000>;
> +                       bootwrapper-size = <0x10000>;
> +                       bootwrapper-magic = <0xa5a5a5a5>;

Are these absolute addresses, or translated per ranges above?

Why are they related to the system controller?

> +               };
> +
> +               fabric: fabric {
> +                       compatible = "hisilicon,hip04-fabric";
> +                       reg = <0x302a000 0x1000>;

How is this going to be used?

> +               };
> +
> +               dual_timer0: dual_timer@3000000 {
> +                       compatible = "arm,sp804", "arm,primecell";
> +                       reg = <0x3000000 0x1000>;
> +                       interrupts = <0 224 4>;
> +                       clocks = <&clock HIP04_CLK_50M>;
> +                       clock-names = "apb_pclk";
> +               };

I thought sp804 had two clocks (one for AMBA and one for the actual
timer). What's going on here?

Cheers,
Mark.
Haojian Zhuang July 29, 2014, 2:44 a.m. UTC | #2
On 29 July 2014 02:06, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Jul 28, 2014 at 02:57:51PM +0100, Haojian Zhuang wrote:
>> Add hip04-d01.dts & hip04.dtsi for hip04 SoC platform.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  arch/arm/boot/dts/Makefile      |   1 +
>>  arch/arm/boot/dts/hip04-d01.dts |  39 ++++++
>>  arch/arm/boot/dts/hip04.dtsi    | 267 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 307 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/hip04-d01.dts
>>  create mode 100644 arch/arm/boot/dts/hip04.dtsi
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 721525e..6587bbf 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -86,6 +86,7 @@ dtb-$(CONFIG_ARCH_HI3xxx) += hi3620-hi4511.dtb
>>  dtb-$(CONFIG_ARCH_HIX5HD2) += hix5hd2-dkb.dtb
>>  dtb-$(CONFIG_ARCH_HIGHBANK) += highbank.dtb \
>>         ecx-2000.dtb
>> +dtb-$(CONFIG_ARCH_HIP04) += hip04-d01.dtb
>>  dtb-$(CONFIG_ARCH_INTEGRATOR) += integratorap.dtb \
>>         integratorcp.dtb
>>  dtb-$(CONFIG_ARCH_KEYSTONE) += k2hk-evm.dtb \
>> diff --git a/arch/arm/boot/dts/hip04-d01.dts b/arch/arm/boot/dts/hip04-d01.dts
>> new file mode 100644
>> index 0000000..661c8e5
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hip04-d01.dts
>> @@ -0,0 +1,39 @@
>> +/*
>> + *  Copyright (C) 2013-2014 Linaro Ltd.
>> + *  Author: Haojian Zhuang <haojian.zhuang@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.
>> + */
>> +
>> +/dts-v1/;
>> +
>> +/* For bootwrapper */
>> +/memreserve/ 0x10c00000 0x00010000;
>
> How exactly is this bootwrapper used? Is the kernel compiled into it?
>
> It might make more sense for the wrapper build system to inject
> bootwrapper-related properties. Then the DTB is less likely to
> amalgamate hacks to workaround differences between versions, and can be
> used on systems without a wrapper without throwing away some memory.
>

In this platform, we relied on the bootwrapper. If I discard this,
I'll fail to bring up all secondary cores.

>> +
>> +#include "hip04.dtsi"
>> +
>> +/ {
>> +       /* memory bus is 64-bit */
>> +       #address-cells = <2>;
>> +       #size-cells = <2>;
>> +       model = "Hisilicon D01 Development Board";
>> +       compatible = "hisilicon,hip04-d01";
>> +
>> +       memory@00000000,10000000 {
>> +               device_type = "memory";
>> +               reg = <0x00000000 0x10000000 0x00000000 0xc0000000>;
>> +       };
>> +
>> +       memory@00000004,c0000000 {
>> +               device_type = "memory";
>> +               reg = <0x00000004 0xc0000000 0x00000003 0x40000000>;
>> +       };
>
> You can fold these into a single node.
>

The address spaces of these two memory node are different.
I can't fold them into a single node.

>> +
>> +       soc {
>> +               uart0: uart@4007000 {
>> +                       status = "ok";
>> +               };
>> +       };
>> +};
>> diff --git a/arch/arm/boot/dts/hip04.dtsi b/arch/arm/boot/dts/hip04.dtsi
>> new file mode 100644
>> index 0000000..30942be
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hip04.dtsi
>> @@ -0,0 +1,267 @@
>> +/*
>> + * Hisilicon Ltd. HiP04 SoC
>> + *
>> + * Copyright (C) 2013-2014 Hisilicon Ltd.
>> + * Copyright (C) 2013-2014 Linaro Ltd.
>> + *
>> + * Author: Haojian Zhuang <haojian.zhuang@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.
>
> s/hh/h/
>

Thanks. I'll fix it.

> [...]
>
>> +       clock: clock {
>> +               compatible = "hisilicon,hip04-clock";
>> +               /* dummy register.
>> +                * Don't need to access clock registers since they're
>> +                * configured in firmware already.
>> +                */
>> +               reg = <0 0 0 0x1000>;
>
> Huh? Whether or not you need to access the registers should be up to the
> kernel, not the DT.
>
> Why can the kernel not access these? This sounds like a hack.
>

Because I didn't get these materials yet. All clocks are enabled in bootloader.

>> +               #clock-cells = <1>;
>> +       };
>> +
>> +       timer {
>> +               compatible = "arm,armv7-timer";
>> +               interrupt-parent = <&gic>;
>> +               interrupts = <1 13 0xf08>,
>> +                            <1 14 0xf08>,
>> +                            <1 11 0xf08>,
>> +                            <1 10 0xf08>;
>> +       };
>> +
>> +       soc {
>> +               /* It's a 32-bit SoC. */
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +               compatible = "simple-bus";
>> +               interrupt-parent = <&gic>;
>> +               ranges = <0 0 0xe0000000 0x10000000>;
>> +
>> +               gic: interrupt-controller@c01000 {
>> +                       compatible = "hisilicon,hip04-gic";
>> +                       #interrupt-cells = <3>;
>> +                       #address-cells = <0>;
>> +                       interrupt-controller;
>> +                       interrupts = <1 9 0xf04>;
>> +
>> +                       reg = <0xc01000 0x1000>, <0xc02000 0x1000>,
>> +                             <0xc04000 0x2000>, <0xc06000 0x2000>;
>
> Please place these on separate lines. It's easier to read and will match
> what you've done for every other node.
>

OK

>> +               };
>> +
>> +               sysctrl: sysctrl {
>> +                       compatible = "hisilicon,sysctrl";
>> +                       reg = <0x3e00000 0x00100000>;
>> +                       relocation-entry = <0xe0000100>;
>> +                       relocation-size = <0x1000>;
>> +                       bootwrapper-phys = <0x10c00000>;
>> +                       bootwrapper-size = <0x10000>;
>> +                       bootwrapper-magic = <0xa5a5a5a5>;
>
> Are these absolute addresses, or translated per ranges above?
>
> Why are they related to the system controller?
>

I need these parameters. But I can't find a better place.

>> +               };
>> +
>> +               fabric: fabric {
>> +                       compatible = "hisilicon,hip04-fabric";
>> +                       reg = <0x302a000 0x1000>;
>
> How is this going to be used?
>

Fabric could configure snoop filter of multiple cluster. I don't have
the manual. I only know this.

>> +               };
>> +
>> +               dual_timer0: dual_timer@3000000 {
>> +                       compatible = "arm,sp804", "arm,primecell";
>> +                       reg = <0x3000000 0x1000>;
>> +                       interrupts = <0 224 4>;
>> +                       clocks = <&clock HIP04_CLK_50M>;
>> +                       clock-names = "apb_pclk";
>> +               };
>
> I thought sp804 had two clocks (one for AMBA and one for the actual
> timer). What's going on here?
>

If only one clock is configured at here, sp804 driver believes that
clk2 is same as clk1.

Regards
Haojian
Olof Johansson July 29, 2014, 3:53 a.m. UTC | #3
Hi,


On Tue, Jul 29, 2014 at 10:44:40AM +0800, Haojian Zhuang wrote:
> On 29 July 2014 02:06, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Jul 28, 2014 at 02:57:51PM +0100, Haojian Zhuang wrote:
> >> +/*
> >> + *  Copyright (C) 2013-2014 Linaro Ltd.
> >> + *  Author: Haojian Zhuang <haojian.zhuang@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.
> >> + */
> >> +
> >> +/dts-v1/;
> >> +
> >> +/* For bootwrapper */
> >> +/memreserve/ 0x10c00000 0x00010000;
> >
> > How exactly is this bootwrapper used? Is the kernel compiled into it?
> >
> > It might make more sense for the wrapper build system to inject
> > bootwrapper-related properties. Then the DTB is less likely to
> > amalgamate hacks to workaround differences between versions, and can be
> > used on systems without a wrapper without throwing away some memory.
> >
> 
> In this platform, we relied on the bootwrapper. If I discard this,
> I'll fail to bring up all secondary cores.

What is this memory used for? Is this where the cores are parked on the spin
table, or something else? Does your platform have runtime firmware?

> >> +
> >> +#include "hip04.dtsi"
> >> +
> >> +/ {
> >> +       /* memory bus is 64-bit */
> >> +       #address-cells = <2>;
> >> +       #size-cells = <2>;
> >> +       model = "Hisilicon D01 Development Board";
> >> +       compatible = "hisilicon,hip04-d01";
> >> +
> >> +       memory@00000000,10000000 {
> >> +               device_type = "memory";
> >> +               reg = <0x00000000 0x10000000 0x00000000 0xc0000000>;
> >> +       };
> >> +
> >> +       memory@00000004,c0000000 {
> >> +               device_type = "memory";
> >> +               reg = <0x00000004 0xc0000000 0x00000003 0x40000000>;
> >> +       };
> >
> > You can fold these into a single node.
> >
> 
> The address spaces of these two memory node are different.
> I can't fold them into a single node.

There's no functional difference today between having two memory nodes or one
node with two ranges. It would be:
	memory@0 {
		device_type = "memory";
		reg = <0x00000000 0x10000000 0x00000000 0xc0000000>,
		      <0x00000004 0xc0000000 0x00000003 0x40000000>;

> >> +
> >> +       soc {
> >> +               uart0: uart@4007000 {
> >> +                       status = "ok";
> >> +               };
> >> +       };
> >> +};
> >> diff --git a/arch/arm/boot/dts/hip04.dtsi b/arch/arm/boot/dts/hip04.dtsi
> >> new file mode 100644
> >> index 0000000..30942be
> >> --- /dev/null
> >> +++ b/arch/arm/boot/dts/hip04.dtsi
> >> @@ -0,0 +1,267 @@
> >> +/*
> >> + * Hisilicon Ltd. HiP04 SoC
> >> + *
> >> + * Copyright (C) 2013-2014 Hisilicon Ltd.
> >> + * Copyright (C) 2013-2014 Linaro Ltd.
> >> + *
> >> + * Author: Haojian Zhuang <haojian.zhuang@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.
> >
> > s/hh/h/
> >
> 
> Thanks. I'll fix it.
> 
> > [...]
> >
> >> +       clock: clock {
> >> +               compatible = "hisilicon,hip04-clock";
> >> +               /* dummy register.
> >> +                * Don't need to access clock registers since they're
> >> +                * configured in firmware already.
> >> +                */
> >> +               reg = <0 0 0 0x1000>;
> >
> > Huh? Whether or not you need to access the registers should be up to the
> > kernel, not the DT.
> >
> > Why can the kernel not access these? This sounds like a hack.
> >
> 
> Because I didn't get these materials yet. All clocks are enabled in bootloader.

What materials? Technical documentation for the CPU you're upstreaming?

It seems like a very bad idea to upstream a DT for a CPU that you don't
have documentation for. It seems appropriate to wait until you have
documentation so you can make sure that the hardware you're describing
is actually what is there, and not something made-up or misdescribed.

> >> +               };
> >> +
> >> +               sysctrl: sysctrl {
> >> +                       compatible = "hisilicon,sysctrl";
> >> +                       reg = <0x3e00000 0x00100000>;
> >> +                       relocation-entry = <0xe0000100>;
> >> +                       relocation-size = <0x1000>;
> >> +                       bootwrapper-phys = <0x10c00000>;
> >> +                       bootwrapper-size = <0x10000>;
> >> +                       bootwrapper-magic = <0xa5a5a5a5>;
> >
> > Are these absolute addresses, or translated per ranges above?
> >
> > Why are they related to the system controller?
> >
> 
> I need these parameters. But I can't find a better place.

Again, what is the boot wrapper in this case? And what do you need them for?

> >> +               };
> >> +
> >> +               fabric: fabric {
> >> +                       compatible = "hisilicon,hip04-fabric";
> >> +                       reg = <0x302a000 0x1000>;
> >
> > How is this going to be used?
> >
> 
> Fabric could configure snoop filter of multiple cluster. I don't have
> the manual. I only know this.

What driver is going to bind against this? It probably makes sense to leave
this out until the consumer is also upstreamed, to have something to have as
a base for discussion.


-Olof
Olof Johansson July 29, 2014, 4 a.m. UTC | #4
On Mon, Jul 28, 2014 at 8:53 PM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
>
> On Tue, Jul 29, 2014 at 10:44:40AM +0800, Haojian Zhuang wrote:
>> On 29 July 2014 02:06, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Mon, Jul 28, 2014 at 02:57:51PM +0100, Haojian Zhuang wrote:
>> >> +/*
>> >> + *  Copyright (C) 2013-2014 Linaro Ltd.
>> >> + *  Author: Haojian Zhuang <haojian.zhuang@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.
>> >> + */
>> >> +
>> >> +/dts-v1/;
>> >> +
>> >> +/* For bootwrapper */
>> >> +/memreserve/ 0x10c00000 0x00010000;
>> >
>> > How exactly is this bootwrapper used? Is the kernel compiled into it?
>> >
>> > It might make more sense for the wrapper build system to inject
>> > bootwrapper-related properties. Then the DTB is less likely to
>> > amalgamate hacks to workaround differences between versions, and can be
>> > used on systems without a wrapper without throwing away some memory.
>> >
>>
>> In this platform, we relied on the bootwrapper. If I discard this,
>> I'll fail to bring up all secondary cores.
>
> What is this memory used for? Is this where the cores are parked on the spin
> table, or something else? Does your platform have runtime firmware?
>
>> >> +
>> >> +#include "hip04.dtsi"
>> >> +
>> >> +/ {
>> >> +       /* memory bus is 64-bit */
>> >> +       #address-cells = <2>;
>> >> +       #size-cells = <2>;
>> >> +       model = "Hisilicon D01 Development Board";
>> >> +       compatible = "hisilicon,hip04-d01";
>> >> +
>> >> +       memory@00000000,10000000 {
>> >> +               device_type = "memory";
>> >> +               reg = <0x00000000 0x10000000 0x00000000 0xc0000000>;
>> >> +       };
>> >> +
>> >> +       memory@00000004,c0000000 {
>> >> +               device_type = "memory";
>> >> +               reg = <0x00000004 0xc0000000 0x00000003 0x40000000>;
>> >> +       };
>> >
>> > You can fold these into a single node.
>> >
>>
>> The address spaces of these two memory node are different.
>> I can't fold them into a single node.
>
> There's no functional difference today between having two memory nodes or one
> node with two ranges. It would be:
>         memory@0 {
>                 device_type = "memory";
>                 reg = <0x00000000 0x10000000 0x00000000 0xc0000000>,
>                       <0x00000004 0xc0000000 0x00000003 0x40000000>;
>
>> >> +
>> >> +       soc {
>> >> +               uart0: uart@4007000 {
>> >> +                       status = "ok";
>> >> +               };
>> >> +       };
>> >> +};
>> >> diff --git a/arch/arm/boot/dts/hip04.dtsi b/arch/arm/boot/dts/hip04.dtsi
>> >> new file mode 100644
>> >> index 0000000..30942be
>> >> --- /dev/null
>> >> +++ b/arch/arm/boot/dts/hip04.dtsi
>> >> @@ -0,0 +1,267 @@
>> >> +/*
>> >> + * Hisilicon Ltd. HiP04 SoC
>> >> + *
>> >> + * Copyright (C) 2013-2014 Hisilicon Ltd.
>> >> + * Copyright (C) 2013-2014 Linaro Ltd.
>> >> + *
>> >> + * Author: Haojian Zhuang <haojian.zhuang@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.
>> >
>> > s/hh/h/
>> >
>>
>> Thanks. I'll fix it.
>>
>> > [...]
>> >
>> >> +       clock: clock {
>> >> +               compatible = "hisilicon,hip04-clock";
>> >> +               /* dummy register.
>> >> +                * Don't need to access clock registers since they're
>> >> +                * configured in firmware already.
>> >> +                */
>> >> +               reg = <0 0 0 0x1000>;
>> >
>> > Huh? Whether or not you need to access the registers should be up to the
>> > kernel, not the DT.
>> >
>> > Why can the kernel not access these? This sounds like a hack.
>> >
>>
>> Because I didn't get these materials yet. All clocks are enabled in bootloader.
>
> What materials? Technical documentation for the CPU you're upstreaming?
>
> It seems like a very bad idea to upstream a DT for a CPU that you don't
> have documentation for. It seems appropriate to wait until you have
> documentation so you can make sure that the hardware you're describing
> is actually what is there, and not something made-up or misdescribed.
>
>> >> +               };
>> >> +
>> >> +               sysctrl: sysctrl {
>> >> +                       compatible = "hisilicon,sysctrl";

I think you already got the comment somewhere else that this property
is much to generic, but if not: It needs to be more specific than
this.

>> >> +                       reg = <0x3e00000 0x00100000>;
>> >> +                       relocation-entry = <0xe0000100>;
>> >> +                       relocation-size = <0x1000>;
>> >> +                       bootwrapper-phys = <0x10c00000>;
>> >> +                       bootwrapper-size = <0x10000>;
>> >> +                       bootwrapper-magic = <0xa5a5a5a5>;
>> >
>> > Are these absolute addresses, or translated per ranges above?
>> >
>> > Why are they related to the system controller?
>> >
>>
>> I need these parameters. But I can't find a better place.
>
> Again, what is the boot wrapper in this case? And what do you need them for?

Ok, looking at patch 3/12, it seems that the "bootwrapper" base
address is used to specify where the CPU goes when released by the
system controller.

Isn't there an address that can be written in the system controller
that specifies this base address, such that you can just handle this
dynamically at runtime? It seems really, really odd that the platform
will expect this one page in the middle of ram to just be untouched
otherwise.



-Olof
Haojian Zhuang July 29, 2014, 10:58 a.m. UTC | #5
On 29 July 2014 11:53, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
>
> On Tue, Jul 29, 2014 at 10:44:40AM +0800, Haojian Zhuang wrote:
>> On 29 July 2014 02:06, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Mon, Jul 28, 2014 at 02:57:51PM +0100, Haojian Zhuang wrote:
>> >> +/*
>> >> + *  Copyright (C) 2013-2014 Linaro Ltd.
>> >> + *  Author: Haojian Zhuang <haojian.zhuang@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.
>> >> + */
>> >> +
>> >> +/dts-v1/;
>> >> +
>> >> +/* For bootwrapper */
>> >> +/memreserve/ 0x10c00000 0x00010000;
>> >
>> > How exactly is this bootwrapper used? Is the kernel compiled into it?
>> >
>> > It might make more sense for the wrapper build system to inject
>> > bootwrapper-related properties. Then the DTB is less likely to
>> > amalgamate hacks to workaround differences between versions, and can be
>> > used on systems without a wrapper without throwing away some memory.
>> >
>>
>> In this platform, we relied on the bootwrapper. If I discard this,
>> I'll fail to bring up all secondary cores.
>
> What is this memory used for? Is this where the cores are parked on the spin
> table, or something else? Does your platform have runtime firmware?
>

This memory stores the instructions that secondary cores will execute initially.

They will switch cores into non-secure & hyp mode.

There's no runtime firmware in this platform. And there's no schedule
to support it.

>> >> +
>> >> +#include "hip04.dtsi"
>> >> +
>> >> +/ {
>> >> +       /* memory bus is 64-bit */
>> >> +       #address-cells = <2>;
>> >> +       #size-cells = <2>;
>> >> +       model = "Hisilicon D01 Development Board";
>> >> +       compatible = "hisilicon,hip04-d01";
>> >> +
>> >> +       memory@00000000,10000000 {
>> >> +               device_type = "memory";
>> >> +               reg = <0x00000000 0x10000000 0x00000000 0xc0000000>;
>> >> +       };
>> >> +
>> >> +       memory@00000004,c0000000 {
>> >> +               device_type = "memory";
>> >> +               reg = <0x00000004 0xc0000000 0x00000003 0x40000000>;
>> >> +       };
>> >
>> > You can fold these into a single node.
>> >
>>
>> The address spaces of these two memory node are different.
>> I can't fold them into a single node.
>
> There's no functional difference today between having two memory nodes or one
> node with two ranges. It would be:
>         memory@0 {
>                 device_type = "memory";
>                 reg = <0x00000000 0x10000000 0x00000000 0xc0000000>,
>                       <0x00000004 0xc0000000 0x00000003 0x40000000>;
>

OK.

>> >> +
>> >> +       soc {
>> >> +               uart0: uart@4007000 {
>> >> +                       status = "ok";
>> >> +               };
>> >> +       };
>> >> +};
>> >> diff --git a/arch/arm/boot/dts/hip04.dtsi b/arch/arm/boot/dts/hip04.dtsi
>> >> new file mode 100644
>> >> index 0000000..30942be
>> >> --- /dev/null
>> >> +++ b/arch/arm/boot/dts/hip04.dtsi
>> >> @@ -0,0 +1,267 @@
>> >> +/*
>> >> + * Hisilicon Ltd. HiP04 SoC
>> >> + *
>> >> + * Copyright (C) 2013-2014 Hisilicon Ltd.
>> >> + * Copyright (C) 2013-2014 Linaro Ltd.
>> >> + *
>> >> + * Author: Haojian Zhuang <haojian.zhuang@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.
>> >
>> > s/hh/h/
>> >
>>
>> Thanks. I'll fix it.
>>
>> > [...]
>> >
>> >> +       clock: clock {
>> >> +               compatible = "hisilicon,hip04-clock";
>> >> +               /* dummy register.
>> >> +                * Don't need to access clock registers since they're
>> >> +                * configured in firmware already.
>> >> +                */
>> >> +               reg = <0 0 0 0x1000>;
>> >
>> > Huh? Whether or not you need to access the registers should be up to the
>> > kernel, not the DT.
>> >
>> > Why can the kernel not access these? This sounds like a hack.
>> >
>>
>> Because I didn't get these materials yet. All clocks are enabled in bootloader.
>
> What materials? Technical documentation for the CPU you're upstreaming?
>
> It seems like a very bad idea to upstream a DT for a CPU that you don't
> have documentation for. It seems appropriate to wait until you have
> documentation so you can make sure that the hardware you're describing
> is actually what is there, and not something made-up or misdescribed.
>
>> >> +               };
>> >> +
>> >> +               sysctrl: sysctrl {
>> >> +                       compatible = "hisilicon,sysctrl";
>> >> +                       reg = <0x3e00000 0x00100000>;
>> >> +                       relocation-entry = <0xe0000100>;
>> >> +                       relocation-size = <0x1000>;
>> >> +                       bootwrapper-phys = <0x10c00000>;
>> >> +                       bootwrapper-size = <0x10000>;
>> >> +                       bootwrapper-magic = <0xa5a5a5a5>;
>> >
>> > Are these absolute addresses, or translated per ranges above?
>> >
>> > Why are they related to the system controller?
>> >
>>
>> I need these parameters. But I can't find a better place.
>
> Again, what is the boot wrapper in this case? And what do you need them for?
>

Now UEFI didn't switch secondary cores to non-secure & hyp mode. The
bootwrapper is used to handle this job. Then bootwrapper will jump to
kernel to continue boot SMP. So the relocation area is used to store
the SMP entrance.

SMP feature will fail without bootwrapper in this platform.

>> >> +               };
>> >> +
>> >> +               fabric: fabric {
>> >> +                       compatible = "hisilicon,hip04-fabric";
>> >> +                       reg = <0x302a000 0x1000>;
>> >
>> > How is this going to be used?
>> >
>>
>> Fabric could configure snoop filter of multiple cluster. I don't have
>> the manual. I only know this.
>
> What driver is going to bind against this? It probably makes sense to leave
> this out until the consumer is also upstreamed, to have something to have as
> a base for discussion.

Only one register is configured in the fabric by kernel. Do we need a driver?

This register is only used when we power up/down the cluster. MCPM
framework could help to call the function to configure this register.

Regards
Haojian
Will Deacon July 29, 2014, 11:12 a.m. UTC | #6
On Tue, Jul 29, 2014 at 04:53:21AM +0100, Olof Johansson wrote:
> On Tue, Jul 29, 2014 at 10:44:40AM +0800, Haojian Zhuang wrote:
> > Because I didn't get these materials yet. All clocks are enabled in bootloader.
> 
> What materials? Technical documentation for the CPU you're upstreaming?
> 
> It seems like a very bad idea to upstream a DT for a CPU that you don't
> have documentation for. It seems appropriate to wait until you have
> documentation so you can make sure that the hardware you're describing
> is actually what is there, and not something made-up or misdescribed.

I completely agree. Furthermore, given that this SoC implements a dodgy
custom GIC, I don't think it's unreasonable for us to ask for documentation
describing that too, otherwise it's going to make maintaining a complicated
irqchip driver that much worse.

Will
Haojian Zhuang July 29, 2014, 11:32 a.m. UTC | #7
On 29 July 2014 19:12, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jul 29, 2014 at 04:53:21AM +0100, Olof Johansson wrote:
>> On Tue, Jul 29, 2014 at 10:44:40AM +0800, Haojian Zhuang wrote:
>> > Because I didn't get these materials yet. All clocks are enabled in bootloader.
>>
>> What materials? Technical documentation for the CPU you're upstreaming?
>>
>> It seems like a very bad idea to upstream a DT for a CPU that you don't
>> have documentation for. It seems appropriate to wait until you have
>> documentation so you can make sure that the hardware you're describing
>> is actually what is there, and not something made-up or misdescribed.
>
> I completely agree. Furthermore, given that this SoC implements a dodgy
> custom GIC, I don't think it's unreasonable for us to ask for documentation
> describing that too, otherwise it's going to make maintaining a complicated
> irqchip driver that much worse.
>
> Will

I don't have enough documents on clocks. And all clocks are already
enabled in UEFI. There're also no requirement on change the clock
frequency & disable them. So I set all these clocks as fixed rate.

It's different from GIC. If I don't have documents on GIC, I can't
write code for HIP04 GIC. HiP04 GIC supports 16-cores. ARM GICv2
supports 8-cores. So some registers offsets and bit fields are
different from GICv2.

Regards
Haojian
Will Deacon July 29, 2014, 12:13 p.m. UTC | #8
On Tue, Jul 29, 2014 at 12:32:09PM +0100, Haojian Zhuang wrote:
> On 29 July 2014 19:12, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jul 29, 2014 at 04:53:21AM +0100, Olof Johansson wrote:
> >> On Tue, Jul 29, 2014 at 10:44:40AM +0800, Haojian Zhuang wrote:
> >> > Because I didn't get these materials yet. All clocks are enabled in bootloader.
> >>
> >> What materials? Technical documentation for the CPU you're upstreaming?
> >>
> >> It seems like a very bad idea to upstream a DT for a CPU that you don't
> >> have documentation for. It seems appropriate to wait until you have
> >> documentation so you can make sure that the hardware you're describing
> >> is actually what is there, and not something made-up or misdescribed.
> >
> > I completely agree. Furthermore, given that this SoC implements a dodgy
> > custom GIC, I don't think it's unreasonable for us to ask for documentation
> > describing that too, otherwise it's going to make maintaining a complicated
> > irqchip driver that much worse.
> >
> I don't have enough documents on clocks. And all clocks are already
> enabled in UEFI. There're also no requirement on change the clock
> frequency & disable them. So I set all these clocks as fixed rate.
> 
> It's different from GIC. If I don't have documents on GIC, I can't
> write code for HIP04 GIC. HiP04 GIC supports 16-cores. ARM GICv2
> supports 8-cores. So some registers offsets and bit fields are
> different from GICv2.

Sure, and I'm asking for those documents to be made publicly available
somewhere so that we can refer to them if we need to change this code in
future.

Will
Haojian Zhuang July 29, 2014, 12:15 p.m. UTC | #9
On 29 July 2014 20:13, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jul 29, 2014 at 12:32:09PM +0100, Haojian Zhuang wrote:
>> On 29 July 2014 19:12, Will Deacon <will.deacon@arm.com> wrote:
>> > On Tue, Jul 29, 2014 at 04:53:21AM +0100, Olof Johansson wrote:
>> >> On Tue, Jul 29, 2014 at 10:44:40AM +0800, Haojian Zhuang wrote:
>> >> > Because I didn't get these materials yet. All clocks are enabled in bootloader.
>> >>
>> >> What materials? Technical documentation for the CPU you're upstreaming?
>> >>
>> >> It seems like a very bad idea to upstream a DT for a CPU that you don't
>> >> have documentation for. It seems appropriate to wait until you have
>> >> documentation so you can make sure that the hardware you're describing
>> >> is actually what is there, and not something made-up or misdescribed.
>> >
>> > I completely agree. Furthermore, given that this SoC implements a dodgy
>> > custom GIC, I don't think it's unreasonable for us to ask for documentation
>> > describing that too, otherwise it's going to make maintaining a complicated
>> > irqchip driver that much worse.
>> >
>> I don't have enough documents on clocks. And all clocks are already
>> enabled in UEFI. There're also no requirement on change the clock
>> frequency & disable them. So I set all these clocks as fixed rate.
>>
>> It's different from GIC. If I don't have documents on GIC, I can't
>> write code for HIP04 GIC. HiP04 GIC supports 16-cores. ARM GICv2
>> supports 8-cores. So some registers offsets and bit fields are
>> different from GICv2.
>
> Sure, and I'm asking for those documents to be made publicly available
> somewhere so that we can refer to them if we need to change this code in
> future.
>
> Will

I can send the copy to you since it's already public. But the main
issue is that this document is written in Chinese. I hope it could be
written in English.

Regards
Haojian
Will Deacon July 29, 2014, 12:22 p.m. UTC | #10
On Tue, Jul 29, 2014 at 01:15:45PM +0100, Haojian Zhuang wrote:
> On 29 July 2014 20:13, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jul 29, 2014 at 12:32:09PM +0100, Haojian Zhuang wrote:
> >> On 29 July 2014 19:12, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Tue, Jul 29, 2014 at 04:53:21AM +0100, Olof Johansson wrote:
> >> >> On Tue, Jul 29, 2014 at 10:44:40AM +0800, Haojian Zhuang wrote:
> >> >> > Because I didn't get these materials yet. All clocks are enabled in bootloader.
> >> >>
> >> >> What materials? Technical documentation for the CPU you're upstreaming?
> >> >>
> >> >> It seems like a very bad idea to upstream a DT for a CPU that you don't
> >> >> have documentation for. It seems appropriate to wait until you have
> >> >> documentation so you can make sure that the hardware you're describing
> >> >> is actually what is there, and not something made-up or misdescribed.
> >> >
> >> > I completely agree. Furthermore, given that this SoC implements a dodgy
> >> > custom GIC, I don't think it's unreasonable for us to ask for documentation
> >> > describing that too, otherwise it's going to make maintaining a complicated
> >> > irqchip driver that much worse.
> >> >
> >> I don't have enough documents on clocks. And all clocks are already
> >> enabled in UEFI. There're also no requirement on change the clock
> >> frequency & disable them. So I set all these clocks as fixed rate.
> >>
> >> It's different from GIC. If I don't have documents on GIC, I can't
> >> write code for HIP04 GIC. HiP04 GIC supports 16-cores. ARM GICv2
> >> supports 8-cores. So some registers offsets and bit fields are
> >> different from GICv2.
> >
> > Sure, and I'm asking for those documents to be made publicly available
> > somewhere so that we can refer to them if we need to change this code in
> > future.
> >
> I can send the copy to you since it's already public. But the main
> issue is that this document is written in Chinese. I hope it could be
> written in English.

Any chance of a translation, please? The ARM GIC spec is available in
English, which is largely the language in which the kernel is developed.

Will
Mark Rutland July 29, 2014, 12:32 p.m. UTC | #11
On Tue, Jul 29, 2014 at 03:44:40AM +0100, Haojian Zhuang wrote:
> On 29 July 2014 02:06, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Jul 28, 2014 at 02:57:51PM +0100, Haojian Zhuang wrote:
> >> Add hip04-d01.dts & hip04.dtsi for hip04 SoC platform.
> >>
> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> >> ---
> >>  arch/arm/boot/dts/Makefile      |   1 +
> >>  arch/arm/boot/dts/hip04-d01.dts |  39 ++++++
> >>  arch/arm/boot/dts/hip04.dtsi    | 267 ++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 307 insertions(+)
> >>  create mode 100644 arch/arm/boot/dts/hip04-d01.dts
> >>  create mode 100644 arch/arm/boot/dts/hip04.dtsi
> >>
> >> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> >> index 721525e..6587bbf 100644
> >> --- a/arch/arm/boot/dts/Makefile
> >> +++ b/arch/arm/boot/dts/Makefile
> >> @@ -86,6 +86,7 @@ dtb-$(CONFIG_ARCH_HI3xxx) += hi3620-hi4511.dtb
> >>  dtb-$(CONFIG_ARCH_HIX5HD2) += hix5hd2-dkb.dtb
> >>  dtb-$(CONFIG_ARCH_HIGHBANK) += highbank.dtb \
> >>         ecx-2000.dtb
> >> +dtb-$(CONFIG_ARCH_HIP04) += hip04-d01.dtb
> >>  dtb-$(CONFIG_ARCH_INTEGRATOR) += integratorap.dtb \
> >>         integratorcp.dtb
> >>  dtb-$(CONFIG_ARCH_KEYSTONE) += k2hk-evm.dtb \
> >> diff --git a/arch/arm/boot/dts/hip04-d01.dts b/arch/arm/boot/dts/hip04-d01.dts
> >> new file mode 100644
> >> index 0000000..661c8e5
> >> --- /dev/null
> >> +++ b/arch/arm/boot/dts/hip04-d01.dts
> >> @@ -0,0 +1,39 @@
> >> +/*
> >> + *  Copyright (C) 2013-2014 Linaro Ltd.
> >> + *  Author: Haojian Zhuang <haojian.zhuang@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.
> >> + */
> >> +
> >> +/dts-v1/;
> >> +
> >> +/* For bootwrapper */
> >> +/memreserve/ 0x10c00000 0x00010000;
> >
> > How exactly is this bootwrapper used? Is the kernel compiled into it?
> >
> > It might make more sense for the wrapper build system to inject
> > bootwrapper-related properties. Then the DTB is less likely to
> > amalgamate hacks to workaround differences between versions, and can be
> > used on systems without a wrapper without throwing away some memory.
> >
> 
> In this platform, we relied on the bootwrapper. If I discard this,
> I'll fail to bring up all secondary cores.

What loads the boot wrapper into memory, and from where?

In another reply you mentioned you're using UEFI. Is this wrapper
considered part of your UEFI implementation? If so it should be reserved
in the UEFI memory map, and there should be no need of memory nodes or
memreserves -- all that should come from UEFI.

What exactly is the boot wrapper in charge of?

What is the split in responsiblity between the wrapper and the kernel?

[...]

> >> +               };
> >> +
> >> +               fabric: fabric {
> >> +                       compatible = "hisilicon,hip04-fabric";
> >> +                       reg = <0x302a000 0x1000>;
> >
> > How is this going to be used?
> >
> 
> Fabric could configure snoop filter of multiple cluster. I don't have
> the manual. I only know this.

Is this accessible to the non-secure side?

If the firmware is currently programming things correctly then we
shouldn't need this for now.

> >> +               };
> >> +
> >> +               dual_timer0: dual_timer@3000000 {
> >> +                       compatible = "arm,sp804", "arm,primecell";
> >> +                       reg = <0x3000000 0x1000>;
> >> +                       interrupts = <0 224 4>;
> >> +                       clocks = <&clock HIP04_CLK_50M>;
> >> +                       clock-names = "apb_pclk";
> >> +               };
> >
> > I thought sp804 had two clocks (one for AMBA and one for the actual
> > timer). What's going on here?
> >
> 
> If only one clock is configured at here, sp804 driver believes that
> clk2 is same as clk1.

I'm not keen on relying on that. It's arguably a bug in the driver.

Thanks,
Mark.
Olof Johansson July 29, 2014, 4:56 p.m. UTC | #12
On Tue, Jul 29, 2014 at 3:58 AM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:
> On 29 July 2014 11:53, Olof Johansson <olof@lixom.net> wrote:
>> Hi,
>>
>>
>> On Tue, Jul 29, 2014 at 10:44:40AM +0800, Haojian Zhuang wrote:
>>> On 29 July 2014 02:06, Mark Rutland <mark.rutland@arm.com> wrote:
>>> > On Mon, Jul 28, 2014 at 02:57:51PM +0100, Haojian Zhuang wrote:
>>> >> +/*
>>> >> + *  Copyright (C) 2013-2014 Linaro Ltd.
>>> >> + *  Author: Haojian Zhuang <haojian.zhuang@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.
>>> >> + */
>>> >> +
>>> >> +/dts-v1/;
>>> >> +
>>> >> +/* For bootwrapper */
>>> >> +/memreserve/ 0x10c00000 0x00010000;
>>> >
>>> > How exactly is this bootwrapper used? Is the kernel compiled into it?
>>> >
>>> > It might make more sense for the wrapper build system to inject
>>> > bootwrapper-related properties. Then the DTB is less likely to
>>> > amalgamate hacks to workaround differences between versions, and can be
>>> > used on systems without a wrapper without throwing away some memory.
>>> >
>>>
>>> In this platform, we relied on the bootwrapper. If I discard this,
>>> I'll fail to bring up all secondary cores.
>>
>> What is this memory used for? Is this where the cores are parked on the spin
>> table, or something else? Does your platform have runtime firmware?
>>
>
> This memory stores the instructions that secondary cores will execute initially.
>
> They will switch cores into non-secure & hyp mode.

Lots of SoCs do this, but when it's managed like this, they normally
will either provide the code snippet in a piece of SRAM on-chip, or
the kernel will provide it and set up the trampolines accordingly.

> There's no runtime firmware in this platform. And there's no schedule
> to support it.

Ok.

>
>>> >> +
>>> >> +#include "hip04.dtsi"
>>> >> +
>>> >> +/ {
>>> >> +       /* memory bus is 64-bit */
>>> >> +       #address-cells = <2>;
>>> >> +       #size-cells = <2>;
>>> >> +       model = "Hisilicon D01 Development Board";
>>> >> +       compatible = "hisilicon,hip04-d01";
>>> >> +
>>> >> +       memory@00000000,10000000 {
>>> >> +               device_type = "memory";
>>> >> +               reg = <0x00000000 0x10000000 0x00000000 0xc0000000>;
>>> >> +       };
>>> >> +
>>> >> +       memory@00000004,c0000000 {
>>> >> +               device_type = "memory";
>>> >> +               reg = <0x00000004 0xc0000000 0x00000003 0x40000000>;
>>> >> +       };
>>> >
>>> > You can fold these into a single node.
>>> >
>>>
>>> The address spaces of these two memory node are different.
>>> I can't fold them into a single node.
>>
>> There's no functional difference today between having two memory nodes or one
>> node with two ranges. It would be:
>>         memory@0 {
>>                 device_type = "memory";
>>                 reg = <0x00000000 0x10000000 0x00000000 0xc0000000>,
>>                       <0x00000004 0xc0000000 0x00000003 0x40000000>;
>>
>
> OK.
>
>>> >> +
>>> >> +       soc {
>>> >> +               uart0: uart@4007000 {
>>> >> +                       status = "ok";
>>> >> +               };
>>> >> +       };
>>> >> +};
>>> >> diff --git a/arch/arm/boot/dts/hip04.dtsi b/arch/arm/boot/dts/hip04.dtsi
>>> >> new file mode 100644
>>> >> index 0000000..30942be
>>> >> --- /dev/null
>>> >> +++ b/arch/arm/boot/dts/hip04.dtsi
>>> >> @@ -0,0 +1,267 @@
>>> >> +/*
>>> >> + * Hisilicon Ltd. HiP04 SoC
>>> >> + *
>>> >> + * Copyright (C) 2013-2014 Hisilicon Ltd.
>>> >> + * Copyright (C) 2013-2014 Linaro Ltd.
>>> >> + *
>>> >> + * Author: Haojian Zhuang <haojian.zhuang@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.
>>> >
>>> > s/hh/h/
>>> >
>>>
>>> Thanks. I'll fix it.
>>>
>>> > [...]
>>> >
>>> >> +       clock: clock {
>>> >> +               compatible = "hisilicon,hip04-clock";
>>> >> +               /* dummy register.
>>> >> +                * Don't need to access clock registers since they're
>>> >> +                * configured in firmware already.
>>> >> +                */
>>> >> +               reg = <0 0 0 0x1000>;
>>> >
>>> > Huh? Whether or not you need to access the registers should be up to the
>>> > kernel, not the DT.
>>> >
>>> > Why can the kernel not access these? This sounds like a hack.
>>> >
>>>
>>> Because I didn't get these materials yet. All clocks are enabled in bootloader.
>>
>> What materials? Technical documentation for the CPU you're upstreaming?
>>
>> It seems like a very bad idea to upstream a DT for a CPU that you don't
>> have documentation for. It seems appropriate to wait until you have
>> documentation so you can make sure that the hardware you're describing
>> is actually what is there, and not something made-up or misdescribed.
>>
>>> >> +               };
>>> >> +
>>> >> +               sysctrl: sysctrl {
>>> >> +                       compatible = "hisilicon,sysctrl";
>>> >> +                       reg = <0x3e00000 0x00100000>;
>>> >> +                       relocation-entry = <0xe0000100>;
>>> >> +                       relocation-size = <0x1000>;
>>> >> +                       bootwrapper-phys = <0x10c00000>;
>>> >> +                       bootwrapper-size = <0x10000>;
>>> >> +                       bootwrapper-magic = <0xa5a5a5a5>;
>>> >
>>> > Are these absolute addresses, or translated per ranges above?
>>> >
>>> > Why are they related to the system controller?
>>> >
>>>
>>> I need these parameters. But I can't find a better place.
>>
>> Again, what is the boot wrapper in this case? And what do you need them for?
>>
>
> Now UEFI didn't switch secondary cores to non-secure & hyp mode. The
> bootwrapper is used to handle this job. Then bootwrapper will jump to
> kernel to continue boot SMP. So the relocation area is used to store
> the SMP entrance.
>
> SMP feature will fail without bootwrapper in this platform.

What you call bootwrapper seems to just be the trampoline code used to
launch an onlined processor? Why does that have to live in just that
piece of memory, can't the kernel configure it?

>>> >> +               };
>>> >> +
>>> >> +               fabric: fabric {
>>> >> +                       compatible = "hisilicon,hip04-fabric";
>>> >> +                       reg = <0x302a000 0x1000>;
>>> >
>>> > How is this going to be used?
>>> >
>>>
>>> Fabric could configure snoop filter of multiple cluster. I don't have
>>> the manual. I only know this.
>>
>> What driver is going to bind against this? It probably makes sense to leave
>> this out until the consumer is also upstreamed, to have something to have as
>> a base for discussion.
>
> Only one register is configured in the fabric by kernel. Do we need a driver?

Ah, I see. My main worry is that you're trying to describe in DT a
piece of hardware that you only have some code snippets for, not
documentation. Is the register window on this IP block really 4K in
size? You only touch the first 20 bytes of it in the MCPM code, for
example.


-Olof
Olof Johansson July 29, 2014, 5:01 p.m. UTC | #13
On Tue, Jul 29, 2014 at 5:32 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jul 29, 2014 at 03:44:40AM +0100, Haojian Zhuang wrote:
>> On 29 July 2014 02:06, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Mon, Jul 28, 2014 at 02:57:51PM +0100, Haojian Zhuang wrote:
>> >> Add hip04-d01.dts & hip04.dtsi for hip04 SoC platform.
>> >>
>> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> >> ---
>> >>  arch/arm/boot/dts/Makefile      |   1 +
>> >>  arch/arm/boot/dts/hip04-d01.dts |  39 ++++++
>> >>  arch/arm/boot/dts/hip04.dtsi    | 267 ++++++++++++++++++++++++++++++++++++++++
>> >>  3 files changed, 307 insertions(+)
>> >>  create mode 100644 arch/arm/boot/dts/hip04-d01.dts
>> >>  create mode 100644 arch/arm/boot/dts/hip04.dtsi
>> >>
>> >> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> >> index 721525e..6587bbf 100644
>> >> --- a/arch/arm/boot/dts/Makefile
>> >> +++ b/arch/arm/boot/dts/Makefile
>> >> @@ -86,6 +86,7 @@ dtb-$(CONFIG_ARCH_HI3xxx) += hi3620-hi4511.dtb
>> >>  dtb-$(CONFIG_ARCH_HIX5HD2) += hix5hd2-dkb.dtb
>> >>  dtb-$(CONFIG_ARCH_HIGHBANK) += highbank.dtb \
>> >>         ecx-2000.dtb
>> >> +dtb-$(CONFIG_ARCH_HIP04) += hip04-d01.dtb
>> >>  dtb-$(CONFIG_ARCH_INTEGRATOR) += integratorap.dtb \
>> >>         integratorcp.dtb
>> >>  dtb-$(CONFIG_ARCH_KEYSTONE) += k2hk-evm.dtb \
>> >> diff --git a/arch/arm/boot/dts/hip04-d01.dts b/arch/arm/boot/dts/hip04-d01.dts
>> >> new file mode 100644
>> >> index 0000000..661c8e5
>> >> --- /dev/null
>> >> +++ b/arch/arm/boot/dts/hip04-d01.dts
>> >> @@ -0,0 +1,39 @@
>> >> +/*
>> >> + *  Copyright (C) 2013-2014 Linaro Ltd.
>> >> + *  Author: Haojian Zhuang <haojian.zhuang@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.
>> >> + */
>> >> +
>> >> +/dts-v1/;
>> >> +
>> >> +/* For bootwrapper */
>> >> +/memreserve/ 0x10c00000 0x00010000;
>> >
>> > How exactly is this bootwrapper used? Is the kernel compiled into it?
>> >
>> > It might make more sense for the wrapper build system to inject
>> > bootwrapper-related properties. Then the DTB is less likely to
>> > amalgamate hacks to workaround differences between versions, and can be
>> > used on systems without a wrapper without throwing away some memory.
>> >
>>
>> In this platform, we relied on the bootwrapper. If I discard this,
>> I'll fail to bring up all secondary cores.
>
> What loads the boot wrapper into memory, and from where?
>
> In another reply you mentioned you're using UEFI. Is this wrapper
> considered part of your UEFI implementation? If so it should be reserved
> in the UEFI memory map, and there should be no need of memory nodes or
> memreserves -- all that should come from UEFI.

There's no UEFI memory map parsing on 32-bit, is there?

> What exactly is the boot wrapper in charge of?
>
> What is the split in responsiblity between the wrapper and the kernel?

Sounds like it's just the trampoline code that's executed at CPU
spin-up time. I wouldn't be surprised if there's a register somewhere
on the same controller that you poke the cpu to life that you can
write what address for it to start executing at, or at least some
other location where it can be redirected somewhere else. It's very
unlikely a wired-in address in hardware. Or so I hope. :-)

>> >> +               };
>> >> +
>> >> +               fabric: fabric {
>> >> +                       compatible = "hisilicon,hip04-fabric";
>> >> +                       reg = <0x302a000 0x1000>;
>> >
>> > How is this going to be used?
>> >
>>
>> Fabric could configure snoop filter of multiple cluster. I don't have
>> the manual. I only know this.
>
> Is this accessible to the non-secure side?
>
> If the firmware is currently programming things correctly then we
> shouldn't need this for now.

with MCPM clusters get powered up and down, it's not uncommon to have
to take them out of coherence domains.

>> >> +               };
>> >> +
>> >> +               dual_timer0: dual_timer@3000000 {
>> >> +                       compatible = "arm,sp804", "arm,primecell";
>> >> +                       reg = <0x3000000 0x1000>;
>> >> +                       interrupts = <0 224 4>;
>> >> +                       clocks = <&clock HIP04_CLK_50M>;
>> >> +                       clock-names = "apb_pclk";
>> >> +               };
>> >
>> > I thought sp804 had two clocks (one for AMBA and one for the actual
>> > timer). What's going on here?
>> >
>>
>> If only one clock is configured at here, sp804 driver believes that
>> clk2 is same as clk1.
>
> I'm not keen on relying on that. It's arguably a bug in the driver.

The driver should warn, but we can't make it fail on it now. :(


-Olof
Mark Rutland July 29, 2014, 5:33 p.m. UTC | #14
On Tue, Jul 29, 2014 at 06:01:48PM +0100, Olof Johansson wrote:
> On Tue, Jul 29, 2014 at 5:32 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Jul 29, 2014 at 03:44:40AM +0100, Haojian Zhuang wrote:
> >> On 29 July 2014 02:06, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Mon, Jul 28, 2014 at 02:57:51PM +0100, Haojian Zhuang wrote:
> >> >> Add hip04-d01.dts & hip04.dtsi for hip04 SoC platform.
> >> >>
> >> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> >> >> ---
> >> >>  arch/arm/boot/dts/Makefile      |   1 +
> >> >>  arch/arm/boot/dts/hip04-d01.dts |  39 ++++++
> >> >>  arch/arm/boot/dts/hip04.dtsi    | 267 ++++++++++++++++++++++++++++++++++++++++
> >> >>  3 files changed, 307 insertions(+)
> >> >>  create mode 100644 arch/arm/boot/dts/hip04-d01.dts
> >> >>  create mode 100644 arch/arm/boot/dts/hip04.dtsi
> >> >>
> >> >> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> >> >> index 721525e..6587bbf 100644
> >> >> --- a/arch/arm/boot/dts/Makefile
> >> >> +++ b/arch/arm/boot/dts/Makefile
> >> >> @@ -86,6 +86,7 @@ dtb-$(CONFIG_ARCH_HI3xxx) += hi3620-hi4511.dtb
> >> >>  dtb-$(CONFIG_ARCH_HIX5HD2) += hix5hd2-dkb.dtb
> >> >>  dtb-$(CONFIG_ARCH_HIGHBANK) += highbank.dtb \
> >> >>         ecx-2000.dtb
> >> >> +dtb-$(CONFIG_ARCH_HIP04) += hip04-d01.dtb
> >> >>  dtb-$(CONFIG_ARCH_INTEGRATOR) += integratorap.dtb \
> >> >>         integratorcp.dtb
> >> >>  dtb-$(CONFIG_ARCH_KEYSTONE) += k2hk-evm.dtb \
> >> >> diff --git a/arch/arm/boot/dts/hip04-d01.dts b/arch/arm/boot/dts/hip04-d01.dts
> >> >> new file mode 100644
> >> >> index 0000000..661c8e5
> >> >> --- /dev/null
> >> >> +++ b/arch/arm/boot/dts/hip04-d01.dts
> >> >> @@ -0,0 +1,39 @@
> >> >> +/*
> >> >> + *  Copyright (C) 2013-2014 Linaro Ltd.
> >> >> + *  Author: Haojian Zhuang <haojian.zhuang@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.
> >> >> + */
> >> >> +
> >> >> +/dts-v1/;
> >> >> +
> >> >> +/* For bootwrapper */
> >> >> +/memreserve/ 0x10c00000 0x00010000;
> >> >
> >> > How exactly is this bootwrapper used? Is the kernel compiled into it?
> >> >
> >> > It might make more sense for the wrapper build system to inject
> >> > bootwrapper-related properties. Then the DTB is less likely to
> >> > amalgamate hacks to workaround differences between versions, and can be
> >> > used on systems without a wrapper without throwing away some memory.
> >> >
> >>
> >> In this platform, we relied on the bootwrapper. If I discard this,
> >> I'll fail to bring up all secondary cores.
> >
> > What loads the boot wrapper into memory, and from where?
> >
> > In another reply you mentioned you're using UEFI. Is this wrapper
> > considered part of your UEFI implementation? If so it should be reserved
> > in the UEFI memory map, and there should be no need of memory nodes or
> > memreserves -- all that should come from UEFI.
> 
> There's no UEFI memory map parsing on 32-bit, is there?

Apparently not, so this can be ignored. Sorry for the noise.

That said I'm slightly confused by the decision to use UEFI given that
we can't use the standard EFI boot mechanism and services on 32-bit.

> > What exactly is the boot wrapper in charge of?
> >
> > What is the split in responsiblity between the wrapper and the kernel?
> 
> Sounds like it's just the trampoline code that's executed at CPU
> spin-up time. I wouldn't be surprised if there's a register somewhere
> on the same controller that you poke the cpu to life that you can
> write what address for it to start executing at, or at least some
> other location where it can be redirected somewhere else. It's very
> unlikely a wired-in address in hardware. Or so I hope. :-)

Me too.

> >> >> +               };
> >> >> +
> >> >> +               fabric: fabric {
> >> >> +                       compatible = "hisilicon,hip04-fabric";
> >> >> +                       reg = <0x302a000 0x1000>;
> >> >
> >> > How is this going to be used?
> >> >
> >>
> >> Fabric could configure snoop filter of multiple cluster. I don't have
> >> the manual. I only know this.
> >
> > Is this accessible to the non-secure side?
> >
> > If the firmware is currently programming things correctly then we
> > shouldn't need this for now.
> 
> with MCPM clusters get powered up and down, it's not uncommon to have
> to take them out of coherence domains.

Sure, but that won't be possible if those registers are secure-only
given the comments that secondaries are demoted to hyp by the wrapper.
That might be possible to work around, but we might be in for further
pain given the primary is already in a non-secure mode.

It might be that this is useful for performance monitoring on the
non-secure side anyway, but without a user it might make more sense to
just drop the node for now.

> >> >> +               };
> >> >> +
> >> >> +               dual_timer0: dual_timer@3000000 {
> >> >> +                       compatible = "arm,sp804", "arm,primecell";
> >> >> +                       reg = <0x3000000 0x1000>;
> >> >> +                       interrupts = <0 224 4>;
> >> >> +                       clocks = <&clock HIP04_CLK_50M>;
> >> >> +                       clock-names = "apb_pclk";
> >> >> +               };
> >> >
> >> > I thought sp804 had two clocks (one for AMBA and one for the actual
> >> > timer). What's going on here?
> >> >
> >>
> >> If only one clock is configured at here, sp804 driver believes that
> >> clk2 is same as clk1.
> >
> > I'm not keen on relying on that. It's arguably a bug in the driver.
> 
> The driver should warn, but we can't make it fail on it now. :(

Yeah. I should revive my series for that. So far everyone seems to be
happy with just hacking around it :(

Thanks,
Mark.
Leif Lindholm July 30, 2014, 11:26 a.m. UTC | #15
On Tue, Jul 29, 2014 at 10:01:48AM -0700, Olof Johansson wrote:
> >> In this platform, we relied on the bootwrapper. If I discard this,
> >> I'll fail to bring up all secondary cores.
> >
> > What loads the boot wrapper into memory, and from where?
> >
> > In another reply you mentioned you're using UEFI. Is this wrapper
> > considered part of your UEFI implementation? If so it should be reserved
> > in the UEFI memory map, and there should be no need of memory nodes or
> > memreserves -- all that should come from UEFI.
> 
> There's no UEFI memory map parsing on 32-bit, is there?

There is in my patch set, but that is still dependent on the
early_ioremap support, which needs someone to comment on the "use
generic fixmap on ARM" bit.
 
/
    Leif
Haojian Zhuang Aug. 1, 2014, 12:02 p.m. UTC | #16
On 29 July 2014 12:00, Olof Johansson <olof@lixom.net> wrote:
>>> >> +                       reg = <0x3e00000 0x00100000>;
>>> >> +                       relocation-entry = <0xe0000100>;
>>> >> +                       relocation-size = <0x1000>;
>>> >> +                       bootwrapper-phys = <0x10c00000>;
>>> >> +                       bootwrapper-size = <0x10000>;
>>> >> +                       bootwrapper-magic = <0xa5a5a5a5>;
>>> >
>>> > Are these absolute addresses, or translated per ranges above?
>>> >
>>> > Why are they related to the system controller?
>>> >
>>>
>>> I need these parameters. But I can't find a better place.
>>
>> Again, what is the boot wrapper in this case? And what do you need them for?
>
> Ok, looking at patch 3/12, it seems that the "bootwrapper" base
> address is used to specify where the CPU goes when released by the
> system controller.
>
> Isn't there an address that can be written in the system controller
> that specifies this base address, such that you can just handle this
> dynamically at runtime? It seems really, really odd that the platform
> will expect this one page in the middle of ram to just be untouched
> otherwise.
>

I'll move these parameters from DTS file to command line. Then I can
reserve these memory when I parse the command line.

Regards
Haojian
diff mbox

Patch

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 721525e..6587bbf 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -86,6 +86,7 @@  dtb-$(CONFIG_ARCH_HI3xxx) += hi3620-hi4511.dtb
 dtb-$(CONFIG_ARCH_HIX5HD2) += hix5hd2-dkb.dtb
 dtb-$(CONFIG_ARCH_HIGHBANK) += highbank.dtb \
 	ecx-2000.dtb
+dtb-$(CONFIG_ARCH_HIP04) += hip04-d01.dtb
 dtb-$(CONFIG_ARCH_INTEGRATOR) += integratorap.dtb \
 	integratorcp.dtb
 dtb-$(CONFIG_ARCH_KEYSTONE) += k2hk-evm.dtb \
diff --git a/arch/arm/boot/dts/hip04-d01.dts b/arch/arm/boot/dts/hip04-d01.dts
new file mode 100644
index 0000000..661c8e5
--- /dev/null
+++ b/arch/arm/boot/dts/hip04-d01.dts
@@ -0,0 +1,39 @@ 
+/*
+ *  Copyright (C) 2013-2014 Linaro Ltd.
+ *  Author: Haojian Zhuang <haojian.zhuang@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.
+ */
+
+/dts-v1/;
+
+/* For bootwrapper */
+/memreserve/ 0x10c00000 0x00010000;
+
+#include "hip04.dtsi"
+
+/ {
+	/* memory bus is 64-bit */
+	#address-cells = <2>;
+	#size-cells = <2>;
+	model = "Hisilicon D01 Development Board";
+	compatible = "hisilicon,hip04-d01";
+
+	memory@00000000,10000000 {
+		device_type = "memory";
+		reg = <0x00000000 0x10000000 0x00000000 0xc0000000>;
+	};
+
+	memory@00000004,c0000000 {
+		device_type = "memory";
+		reg = <0x00000004 0xc0000000 0x00000003 0x40000000>;
+	};
+
+	soc {
+		uart0: uart@4007000 {
+			status = "ok";
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/hip04.dtsi b/arch/arm/boot/dts/hip04.dtsi
new file mode 100644
index 0000000..30942be
--- /dev/null
+++ b/arch/arm/boot/dts/hip04.dtsi
@@ -0,0 +1,267 @@ 
+/*
+ * Hisilicon Ltd. HiP04 SoC
+ *
+ * Copyright (C) 2013-2014 Hisilicon Ltd.
+ * Copyright (C) 2013-2014 Linaro Ltd.
+ *
+ * Author: Haojian Zhuang <haojian.zhuang@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 <dt-bindings/clock/hip04-clock.h>
+
+/ {
+	/* memory bus is 64-bit */
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	aliases {
+		serial0 = &uart0;
+	};
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu-map {
+			cluster0 {
+				core0 {
+					cpu = <&CPU0>;
+				};
+				core1 {
+					cpu = <&CPU1>;
+				};
+				core2 {
+					cpu = <&CPU2>;
+				};
+				core3 {
+					cpu = <&CPU3>;
+				};
+			};
+			cluster1 {
+				core0 {
+					cpu = <&CPU4>;
+				};
+				core1 {
+					cpu = <&CPU5>;
+				};
+				core2 {
+					cpu = <&CPU6>;
+				};
+				core3 {
+					cpu = <&CPU7>;
+				};
+			};
+			cluster2 {
+				core0 {
+					cpu = <&CPU8>;
+				};
+				core1 {
+					cpu = <&CPU9>;
+				};
+				core2 {
+					cpu = <&CPU10>;
+				};
+				core3 {
+					cpu = <&CPU11>;
+				};
+			};
+			cluster3 {
+				core0 {
+					cpu = <&CPU12>;
+				};
+				core1 {
+					cpu = <&CPU13>;
+				};
+				core2 {
+					cpu = <&CPU14>;
+				};
+				core3 {
+					cpu = <&CPU15>;
+				};
+			};
+		};
+		CPU0: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0>;
+		};
+		CPU1: cpu@1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <1>;
+		};
+		CPU2: cpu@2 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <2>;
+		};
+		CPU3: cpu@3 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <3>;
+		};
+		CPU4: cpu@100 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x100>;
+		};
+		CPU5: cpu@101 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x101>;
+		};
+		CPU6: cpu@102 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x102>;
+		};
+		CPU7: cpu@103 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x103>;
+		};
+		CPU8: cpu@200 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x200>;
+		};
+		CPU9: cpu@201 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x201>;
+		};
+		CPU10: cpu@202 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x202>;
+		};
+		CPU11: cpu@203 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x203>;
+		};
+		CPU12: cpu@300 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x300>;
+		};
+		CPU13: cpu@301 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x301>;
+		};
+		CPU14: cpu@302 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x302>;
+		};
+		CPU15: cpu@303 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a15";
+			reg = <0x303>;
+		};
+	};
+
+	clock: clock {
+		compatible = "hisilicon,hip04-clock";
+		/* dummy register.
+		 * Don't need to access clock registers since they're
+		 * configured in firmware already.
+		 */
+		reg = <0 0 0 0x1000>;
+		#clock-cells = <1>;
+	};
+
+	timer {
+		compatible = "arm,armv7-timer";
+		interrupt-parent = <&gic>;
+		interrupts = <1 13 0xf08>,
+			     <1 14 0xf08>,
+			     <1 11 0xf08>,
+			     <1 10 0xf08>;
+	};
+
+	soc {
+		/* It's a 32-bit SoC. */
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		interrupt-parent = <&gic>;
+		ranges = <0 0 0xe0000000 0x10000000>;
+
+		gic: interrupt-controller@c01000 {
+			compatible = "hisilicon,hip04-gic";
+			#interrupt-cells = <3>;
+			#address-cells = <0>;
+			interrupt-controller;
+			interrupts = <1 9 0xf04>;
+
+			reg = <0xc01000 0x1000>, <0xc02000 0x1000>,
+			      <0xc04000 0x2000>, <0xc06000 0x2000>;
+		};
+
+		sysctrl: sysctrl {
+			compatible = "hisilicon,sysctrl";
+			reg = <0x3e00000 0x00100000>;
+			relocation-entry = <0xe0000100>;
+			relocation-size = <0x1000>;
+			bootwrapper-phys = <0x10c00000>;
+			bootwrapper-size = <0x10000>;
+			bootwrapper-magic = <0xa5a5a5a5>;
+		};
+
+		fabric: fabric {
+			compatible = "hisilicon,hip04-fabric";
+			reg = <0x302a000 0x1000>;
+		};
+
+		dual_timer0: dual_timer@3000000 {
+			compatible = "arm,sp804", "arm,primecell";
+			reg = <0x3000000 0x1000>;
+			interrupts = <0 224 4>;
+			clocks = <&clock HIP04_CLK_50M>;
+			clock-names = "apb_pclk";
+		};
+
+		arm-pmu {
+			compatible = "arm,cortex-a15-pmu";
+			interrupts = <0 64 4>,
+				     <0 65 4>,
+				     <0 66 4>,
+				     <0 67 4>,
+				     <0 68 4>,
+				     <0 69 4>,
+				     <0 70 4>,
+				     <0 71 4>,
+				     <0 72 4>,
+				     <0 73 4>,
+				     <0 74 4>,
+				     <0 75 4>,
+				     <0 76 4>,
+				     <0 77 4>,
+				     <0 78 4>,
+				     <0 79 4>;
+		};
+
+		uart0: uart@4007000 {
+			compatible = "snps,dw-apb-uart";
+			reg = <0x4007000 0x1000>;
+			interrupts = <0 381 4>;
+			clocks = <&clock HIP04_CLK_168M>;
+			clock-names = "uartclk";
+			reg-shift = <2>;
+			status = "disabled";
+		};
+
+		sata0: sata@a000000 {
+			compatible = "hisilicon,hisi-ahci";
+			reg = <0xa000000 0x1000000>;
+			interrupts = <0 372 4>;
+		};
+
+	};
+};