| Message ID | 1406555876-11989-8-git-send-email-haojian.zhuang@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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
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
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
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
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
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
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
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.
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
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
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.
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
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 --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>; + }; + + }; +};
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