| Message ID | 1423128277-10297-4-git-send-email-bintian.wang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 05, 2015 at 09:24:37AM +0000, Bintian Wang wrote: > Add initial dtsi file to support Hisilicon Hi6220 SoC with > support of Octal core CPUs in two clusters and each cluster > has quard Cortex-A53. > > We now use the "spin-table" method for SMP, and it will be > changed to PSCI later. If that's the case, please don't place the enable-method and related properties in this file. Get your bootloader to add the appropriate properties for its boot protocol. When is PSCI likely to appear? Are we certain of the split between components the PSCI implementation must touch and those the kernel must touch? > Also add dts file to support HiKey development board which > based on Hi6220 SoC and document the devicetree bindings. > > These dts files will be changed later and more nodes will be > added to describe other devices. How is this going to be changed other than the addition of nodes? Will this DTB continue to work in future? Or do you intend to make changes that will break support? > Signed-off-by: Bintian Wang <bintian.wang@huawei.com> > Reviewed-by: Haojian Zhuang <haojian.zhuang@linaro.org> > Reviewed-by: Yiping Xu <xuyiping@hisilicon.com> > --- > .../bindings/arm/hisilicon/hisilicon.txt | 33 ++++ > arch/arm64/boot/dts/Makefile | 1 + > arch/arm64/boot/dts/hisilicon/Makefile | 5 + > arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 31 +++ > arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 204 ++++++++++++++++++++ > 5 files changed, 274 insertions(+) > create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile > create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts > create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi > > diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > index f717c7b..5eb6b41 100644 > --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt > @@ -9,6 +9,9 @@ HiP04 D01 Board > Required root node properties: > - compatible = "hisilicon,hip04-d01"; > > +HiKey Board > +Required root node properties: > + - compatible = "hisilicon,hi6220-hikey"; > > Hisilicon system controller > > @@ -62,6 +65,36 @@ Example: > }; > > ----------------------------------------------------------------------- > +Hisilicon Power Always ON domain controller > + > +Required properties: > +- compatible : "hisilicon,aoctrl" > +- reg : Register address and size > + > +Some clock registers are defined in power always on system controller, > +especially in Hi6220 SoC which is used for mobile platform. > + > +----------------------------------------------------------------------- > +Hisilicon Media domain controller > + > +Required properties: > +- compatible : "hisilicon,mediactrl" > +- reg : Register address and size > + > +Some clock registers of media module are defined in media system > +controller, especially in Hi6220 SoC which is used for mobile platform. > + > +----------------------------------------------------------------------- > +Hisilicon Power Management domain controller > + > +Required properties: > +- compatible : "hisilicon,pmctrl" > +- reg : Register address and size > + > +Some clock registers and PMU registers are defined in power management > +controller, especially in Hin6220 SoC which is used for mobile platform. > + > +----------------------------------------------------------------------- Looking at the dts below, none of these binding docs are sufficient. Define _all_ the properties and what they mean. Please also split documentation into earlier patches. > Fabric: > > Required Properties: > diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile > index c62b0f4..bffd6b7 100644 > --- a/arch/arm64/boot/dts/Makefile > +++ b/arch/arm64/boot/dts/Makefile > @@ -2,5 +2,6 @@ dts-dirs += amd > dts-dirs += apm > dts-dirs += arm > dts-dirs += cavium > +dts-dirs += hisilicon > > subdir-y := $(dts-dirs) > diff --git a/arch/arm64/boot/dts/hisilicon/Makefile b/arch/arm64/boot/dts/hisilicon/Makefile > new file mode 100644 > index 0000000..fa81a6e > --- /dev/null > +++ b/arch/arm64/boot/dts/hisilicon/Makefile > @@ -0,0 +1,5 @@ > +dtb-$(CONFIG_ARCH_HISI) += hi6220-hikey.dtb > + > +always := $(dtb-y) > +subdir-y := $(dts-dirs) > +clean-files := *.dtb > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts > new file mode 100644 > index 0000000..a94da84 > --- /dev/null > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts > @@ -0,0 +1,31 @@ > +/* > + * dts file for Hisilicon HiKey Development Board > + * > + * Copyright (C) 2015, Hisilicon Ltd. > + * > + */ > + > +/dts-v1/; > + > +/memreserve/ 0x0740f000 0x1000; If you're going to use /memreserve/, please add a comment regarding what it is intended to protect, and why that's in memory given to the kernel to begin with. > + > +#include "hi6220.dtsi" > + > +/ { > + model = "HiKey Development Board"; > + compatible = "hisilicon,hi6220-hikey"; > + #address-cells = <2>; > + #size-cells = <2>; > + interrupt-parent = <&gic>; > + > + aliases { > + serial0 = &uart0; > + }; > + > + chosen { }; You should use stdout-path here, ideally with the full UART configuration. > + > + memory@7400000 { > + device_type = "memory"; > + reg = <0x0 0x07400000 0x0 0x38c00000>; > + }; > +}; > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > new file mode 100644 > index 0000000..53ba9cf > --- /dev/null > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > @@ -0,0 +1,204 @@ > +/* > + * dts file for Hisilicon Hi6220 SoC > + * > + * Copyright (C) 2015, Hisilicon Ltd. > + */ > + > +#include <dt-bindings/clock/hi6220-clock.h> > + > +/ { > + cpu-map { Per the binding, this must live under /cpus. Move this within the /cpus node. > + 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>; > + }; > + }; > + }; > + > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + cpu0: cpu@000 { > + compatible = "arm,cortex-a53", "arm,armv8"; > + device_type = "cpu"; > + reg = <0x0 0x0>; > + enable-method = "spin-table"; > + cpu-release-addr = <0x0 0x740fff8>; If you _must_ use spin-table, please give each CPU a unique release address. Using a shared address was a mistake, and we should learn from it. Which CPU does the system boot on? > + clock-latency = <0>; Why is this here? There is no reason for this to be on any CPU node. > + }; [...] > + gic: interrupt-controller@f6800000 { > + compatible = "arm,gic-400", "arm,cortex-a15-gic"; Surely there's no need for the "arm,cortex-a15-gic" fallback entry? What am I missing? > + reg = <0x0 0xf6801000 0x0 0x1000>, /* GICD */ This doesn't match the unit-address. > + <0x0 0xf6802000 0x0 0x2000>, /* GICC */ > + <0x0 0xf6804000 0x0 0x2000>, /* GICH */ > + <0x0 0xf6806000 0x0 0x2000>; /* GICV */ I guess no-one's bothered to consider 64k pages? Given GICH and GICV, I hope that this platform is booted at EL2? > + #interrupt-cells = <3>; > + #address-cells = <0>; > + interrupt-controller; > + }; > + > + > + timer { > + compatible = "arm,armv8-timer"; > + interrupt-parent = <&gic>; > + interrupts = <1 13 0xff08>, > + <1 14 0xff08>, > + <1 11 0xff08>, > + <1 10 0xff08>; > + clock-frequency = <1200000>; > + }; NAK. Fix your firmware to configure CNTFRQ, on all CPUs. That frequency also looks a bit low; timekeeping isn't going to be very precise. > + soc { > + compatible = "simple-bus"; > + #address-cells = <2>; > + #size-cells = <2>; > + interrupt-parent = <&gic>; > + ranges; > + > + ao_ctrl: ao_ctrl { > + compatible = "hisilicon,aoctrl", "syscon"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0x0 0xf7800000 0x0 0x2000>; > + ranges = <0 0x0 0xf7800000 0x2000>; > + > + clock_ao: clock0@0 { > + compatible = "hisilicon,hi6220-clock-ao"; > + reg = <0 0x1000>; > + #clock-cells = <1>; > + }; > + }; > + > + sys_ctrl: sys_ctrl { > + compatible = "hisilicon,sysctrl", "syscon"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0x0 0xf7030000 0x0 0x2000>; > + ranges = <0 0x0 0xf7030000 0x2000>; > + > + clock_sys: clock1@0 { > + compatible = "hisilicon,hi6220-clock-sys"; > + reg = <0 0x1000>; > + #clock-cells = <1>; > + }; > + }; > + > + media_ctrl: media_ctrl { > + compatible = "hisilicon,mediactrl", "syscon"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0x0 0xf4410000 0x0 0x1000>; > + ranges = <0 0x0 0xf4410000 0x1000>; > + > + clock_media: clock2@0 { > + compatible = "hisilicon,hi6220-clock-media"; > + reg = <0 0x1000>; > + #clock-cells = <1>; > + }; > + }; > + > + pm_ctrl: pm_ctrl { > + compatible = "hisilicon,pmctrl", "syscon"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0x0 0xf7032000 0x0 0x1000>; > + ranges = <0 0x0 0xf7032000 0x1000>; > + > + clock_power: clock3@0 { > + compatible = "hisilicon,hi6220-clock-power"; > + reg = <0 0x1000>; > + #clock-cells = <1>; > + }; > + }; I really doesn't see the point in having a sub-device that covers the entirely of the register space of the containing node, and that being the case I am extremely concerned that the containers are marked as syscon compatible. Why are these marked as being syscon devices? Per the dts _all_ their registers are clearly owned by their child nodes, and shouldn't be poked by anything else. Thanks, Mark.
Hello Mark, 2015-02-06 3:30 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: > On Thu, Feb 05, 2015 at 09:24:37AM +0000, Bintian Wang wrote: >> Add initial dtsi file to support Hisilicon Hi6220 SoC with >> support of Octal core CPUs in two clusters and each cluster >> has quard Cortex-A53. >> >> We now use the "spin-table" method for SMP, and it will be >> changed to PSCI later. > > If that's the case, please don't place the enable-method and related > properties in this file. Get your bootloader to add the appropriate > properties for its boot protocol. > > When is PSCI likely to appear? PSCI is under development. > > Are we certain of the split between components the PSCI implementation > must touch and those the kernel must touch? > >> Also add dts file to support HiKey development board which >> based on Hi6220 SoC and document the devicetree bindings. >> >> These dts files will be changed later and more nodes will be >> added to describe other devices. > > How is this going to be changed other than the addition of nodes? > > Will this DTB continue to work in future? Or do you intend to make > changes that will break support? My original idea is: use spin-table for SMP now, when firmware is OK to support PSCI, we submit another patch to replace the spin-table with PSCI. If DTB should continue to work in the future, we really need to remove the spin-table from current dts file, how about just enable one core now? Which one do you favor or any other suggestion? > >> Signed-off-by: Bintian Wang <bintian.wang@huawei.com> >> Reviewed-by: Haojian Zhuang <haojian.zhuang@linaro.org> >> Reviewed-by: Yiping Xu <xuyiping@hisilicon.com> >> --- >> .../bindings/arm/hisilicon/hisilicon.txt | 33 ++++ >> arch/arm64/boot/dts/Makefile | 1 + >> arch/arm64/boot/dts/hisilicon/Makefile | 5 + >> arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 31 +++ >> arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 204 ++++++++++++++++++++ >> 5 files changed, 274 insertions(+) >> create mode 100644 arch/arm64/boot/dts/hisilicon/Makefile >> create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts >> create mode 100644 arch/arm64/boot/dts/hisilicon/hi6220.dtsi >> >> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >> index f717c7b..5eb6b41 100644 >> --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >> +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt >> @@ -9,6 +9,9 @@ HiP04 D01 Board >> Required root node properties: >> - compatible = "hisilicon,hip04-d01"; >> >> +HiKey Board >> +Required root node properties: >> + - compatible = "hisilicon,hi6220-hikey"; >> >> Hisilicon system controller >> >> @@ -62,6 +65,36 @@ Example: >> }; >> >> ----------------------------------------------------------------------- >> +Hisilicon Power Always ON domain controller >> + >> +Required properties: >> +- compatible : "hisilicon,aoctrl" >> +- reg : Register address and size >> + >> +Some clock registers are defined in power always on system controller, >> +especially in Hi6220 SoC which is used for mobile platform. >> + >> +----------------------------------------------------------------------- >> +Hisilicon Media domain controller >> + >> +Required properties: >> +- compatible : "hisilicon,mediactrl" >> +- reg : Register address and size >> + >> +Some clock registers of media module are defined in media system >> +controller, especially in Hi6220 SoC which is used for mobile platform. >> + >> +----------------------------------------------------------------------- >> +Hisilicon Power Management domain controller >> + >> +Required properties: >> +- compatible : "hisilicon,pmctrl" >> +- reg : Register address and size >> + >> +Some clock registers and PMU registers are defined in power management >> +controller, especially in Hin6220 SoC which is used for mobile platform. >> + >> +----------------------------------------------------------------------- > > Looking at the dts below, none of these binding docs are sufficient. > > Define _all_ the properties and what they mean. In fact, Hisilicon designs several system controllers for different function domains, we can control different functions(e.g. clk gate on or off /IP reset) based on the base address of controller + offset, I will give more description in next version. > Please also split documentation into earlier patches. OK, separate document and code in next version. > >> Fabric: >> >> Required Properties: >> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile >> index c62b0f4..bffd6b7 100644 >> --- a/arch/arm64/boot/dts/Makefile >> +++ b/arch/arm64/boot/dts/Makefile >> @@ -2,5 +2,6 @@ dts-dirs += amd >> dts-dirs += apm >> dts-dirs += arm >> dts-dirs += cavium >> +dts-dirs += hisilicon >> >> subdir-y := $(dts-dirs) >> diff --git a/arch/arm64/boot/dts/hisilicon/Makefile b/arch/arm64/boot/dts/hisilicon/Makefile >> new file mode 100644 >> index 0000000..fa81a6e >> --- /dev/null >> +++ b/arch/arm64/boot/dts/hisilicon/Makefile >> @@ -0,0 +1,5 @@ >> +dtb-$(CONFIG_ARCH_HISI) += hi6220-hikey.dtb >> + >> +always := $(dtb-y) >> +subdir-y := $(dts-dirs) >> +clean-files := *.dtb >> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts >> new file mode 100644 >> index 0000000..a94da84 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts >> @@ -0,0 +1,31 @@ >> +/* >> + * dts file for Hisilicon HiKey Development Board >> + * >> + * Copyright (C) 2015, Hisilicon Ltd. >> + * >> + */ >> + >> +/dts-v1/; >> + >> +/memreserve/ 0x0740f000 0x1000; > > If you're going to use /memreserve/, please add a comment regarding what > it is intended to protect, and why that's in memory given to the kernel > to begin with. > >> + >> +#include "hi6220.dtsi" >> + >> +/ { >> + model = "HiKey Development Board"; >> + compatible = "hisilicon,hi6220-hikey"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + interrupt-parent = <&gic>; >> + >> + aliases { >> + serial0 = &uart0; >> + }; >> + >> + chosen { }; > > You should use stdout-path here, ideally with the full UART > configuration. Add in next version. >> + >> + memory@7400000 { >> + device_type = "memory"; >> + reg = <0x0 0x07400000 0x0 0x38c00000>; >> + }; >> +}; >> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi >> new file mode 100644 >> index 0000000..53ba9cf >> --- /dev/null >> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi >> @@ -0,0 +1,204 @@ >> +/* >> + * dts file for Hisilicon Hi6220 SoC >> + * >> + * Copyright (C) 2015, Hisilicon Ltd. >> + */ >> + >> +#include <dt-bindings/clock/hi6220-clock.h> >> + >> +/ { >> + cpu-map { > > Per the binding, this must live under /cpus. > > Move this within the /cpus node. > >> + 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>; >> + }; >> + }; >> + }; >> + >> + cpus { >> + #address-cells = <2>; >> + #size-cells = <0>; >> + >> + cpu0: cpu@000 { >> + compatible = "arm,cortex-a53", "arm,armv8"; >> + device_type = "cpu"; >> + reg = <0x0 0x0>; >> + enable-method = "spin-table"; >> + cpu-release-addr = <0x0 0x740fff8>; > > If you _must_ use spin-table, please give each CPU a unique release > address. Using a shared address was a mistake, and we should learn from > it. > > Which CPU does the system boot on? > >> + clock-latency = <0>; > > Why is this here? > > There is no reason for this to be on any CPU node. Fix in next version. > >> + }; > > [...] > >> + gic: interrupt-controller@f6800000 { >> + compatible = "arm,gic-400", "arm,cortex-a15-gic"; > > Surely there's no need for the "arm,cortex-a15-gic" fallback entry? What > am I missing? Remove it in next version. > >> + reg = <0x0 0xf6801000 0x0 0x1000>, /* GICD */ > > This doesn't match the unit-address. Do you mean change to "<0x0 0xf6801000 0 0x1000>" ? > >> + <0x0 0xf6802000 0x0 0x2000>, /* GICC */ >> + <0x0 0xf6804000 0x0 0x2000>, /* GICH */ >> + <0x0 0xf6806000 0x0 0x2000>; /* GICV */ > > I guess no-one's bothered to consider 64k pages? > > Given GICH and GICV, I hope that this platform is booted at EL2? Transfer from EL3 to EL1 directly, keep these two just for future use. > >> + #interrupt-cells = <3>; >> + #address-cells = <0>; >> + interrupt-controller; >> + }; >> + >> + >> + timer { >> + compatible = "arm,armv8-timer"; >> + interrupt-parent = <&gic>; >> + interrupts = <1 13 0xff08>, >> + <1 14 0xff08>, >> + <1 11 0xff08>, >> + <1 10 0xff08>; >> + clock-frequency = <1200000>; >> + }; > > NAK. Fix your firmware to configure CNTFRQ, on all CPUs. Fix in next version, maybe it will take some time to change firmware. > > That frequency also looks a bit low; timekeeping isn't going to be very > precise. > >> + soc { >> + compatible = "simple-bus"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + interrupt-parent = <&gic>; >> + ranges; >> + >> + ao_ctrl: ao_ctrl { >> + compatible = "hisilicon,aoctrl", "syscon"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + reg = <0x0 0xf7800000 0x0 0x2000>; >> + ranges = <0 0x0 0xf7800000 0x2000>; >> + >> + clock_ao: clock0@0 { >> + compatible = "hisilicon,hi6220-clock-ao"; >> + reg = <0 0x1000>; >> + #clock-cells = <1>; >> + }; >> + }; >> + >> + sys_ctrl: sys_ctrl { >> + compatible = "hisilicon,sysctrl", "syscon"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + reg = <0x0 0xf7030000 0x0 0x2000>; >> + ranges = <0 0x0 0xf7030000 0x2000>; >> + >> + clock_sys: clock1@0 { >> + compatible = "hisilicon,hi6220-clock-sys"; >> + reg = <0 0x1000>; >> + #clock-cells = <1>; >> + }; >> + }; >> + >> + media_ctrl: media_ctrl { >> + compatible = "hisilicon,mediactrl", "syscon"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + reg = <0x0 0xf4410000 0x0 0x1000>; >> + ranges = <0 0x0 0xf4410000 0x1000>; >> + >> + clock_media: clock2@0 { >> + compatible = "hisilicon,hi6220-clock-media"; >> + reg = <0 0x1000>; >> + #clock-cells = <1>; >> + }; >> + }; >> + >> + pm_ctrl: pm_ctrl { >> + compatible = "hisilicon,pmctrl", "syscon"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + reg = <0x0 0xf7032000 0x0 0x1000>; >> + ranges = <0 0x0 0xf7032000 0x1000>; >> + >> + clock_power: clock3@0 { >> + compatible = "hisilicon,hi6220-clock-power"; >> + reg = <0 0x1000>; >> + #clock-cells = <1>; >> + }; >> + }; > > I really doesn't see the point in having a sub-device that covers the > entirely of the register space of the containing node, and that being > the case I am extremely concerned that the containers are marked as > syscon compatible. The SoC clocks are designed and placed under different system controllers, so I define corresponding nodes under different controllers for clock operation. > > Why are these marked as being syscon devices? Per the dts _all_ their > registers are clearly owned by their child nodes, and shouldn't be poked > by anything else. > > Thanks, > Mark. > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On 06/02/15 08:42, Brent Wang wrote: [...] >> >>> + <0x0 0xf6802000 0x0 0x2000>, /* GICC */ >>> + <0x0 0xf6804000 0x0 0x2000>, /* GICH */ >>> + <0x0 0xf6806000 0x0 0x2000>; /* GICV */ >> >> I guess no-one's bothered to consider 64k pages? >> >> Given GICH and GICV, I hope that this platform is booted at EL2? > Transfer from EL3 to EL1 directly, keep these two just for future use. That's a real shame, as it keeps users away from some key aspects of the ARMv8 architecture. >> >>> + #interrupt-cells = <3>; >>> + #address-cells = <0>; >>> + interrupt-controller; And if you're keeping GICH/GICV, where is the maintenance interrupt? >>> + }; >>> + >>> + >>> + timer { >>> + compatible = "arm,armv8-timer"; >>> + interrupt-parent = <&gic>; >>> + interrupts = <1 13 0xff08>, >>> + <1 14 0xff08>, >>> + <1 11 0xff08>, >>> + <1 10 0xff08>; >>> + clock-frequency = <1200000>; >>> + }; >> >> NAK. Fix your firmware to configure CNTFRQ, on all CPUs. > Fix in next version, maybe it will take some time to change firmware. While you're at it, make sure CNTVOFF_EL2 is set to zero on all CPUs before dropping to EL1. This tends to be overlooked. Thanks, M.
On Fri, Feb 06, 2015 at 09:07:10AM +0000, Marc Zyngier wrote: > On 06/02/15 08:42, Brent Wang wrote: > > [...] > > >> > >>> + <0x0 0xf6802000 0x0 0x2000>, /* GICC */ > >>> + <0x0 0xf6804000 0x0 0x2000>, /* GICH */ > >>> + <0x0 0xf6806000 0x0 0x2000>; /* GICV */ > >> > >> I guess no-one's bothered to consider 64k pages? > >> > >> Given GICH and GICV, I hope that this platform is booted at EL2? > > Transfer from EL3 to EL1 directly, keep these two just for future use. > > That's a real shame, as it keeps users away from some key aspects of the > ARMv8 architecture. More importantly (and regardless of whether you wish to use the features provided by EL2), booting at EL2 means that the FW/bootloader needs to set up far less, and that the kernel can fix up some issues that might not be immediately apparent... [...] > >>> + timer { > >>> + compatible = "arm,armv8-timer"; > >>> + interrupt-parent = <&gic>; > >>> + interrupts = <1 13 0xff08>, > >>> + <1 14 0xff08>, > >>> + <1 11 0xff08>, > >>> + <1 10 0xff08>; > >>> + clock-frequency = <1200000>; > >>> + }; > >> > >> NAK. Fix your firmware to configure CNTFRQ, on all CPUs. > > Fix in next version, maybe it will take some time to change firmware. > > While you're at it, make sure CNTVOFF_EL2 is set to zero on all CPUs > before dropping to EL1. This tends to be overlooked. ...like differing values of CNTVOFF_EL2. There seems to be a common misconception that booting at EL2 is a bad thing to do, when in reality booting at EL1 is more likely to result in bugs we can't work around. Is there any reason that you do not wish to boot at EL2, or were you simply unaware that booting at EL2 was possible/preferred? Thanks, Mark.
On Fri, Feb 06, 2015 at 08:42:22AM +0000, Brent Wang wrote: > Hello Mark, > > 2015-02-06 3:30 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: > > On Thu, Feb 05, 2015 at 09:24:37AM +0000, Bintian Wang wrote: > >> Add initial dtsi file to support Hisilicon Hi6220 SoC with > >> support of Octal core CPUs in two clusters and each cluster > >> has quard Cortex-A53. > >> > >> We now use the "spin-table" method for SMP, and it will be > >> changed to PSCI later. > > > > If that's the case, please don't place the enable-method and related > > properties in this file. Get your bootloader to add the appropriate > > properties for its boot protocol. > > > > When is PSCI likely to appear? > PSCI is under development. Sure. Do you have an estimate as to when it will appear? What are you using for your PSCI implementation? The ARM Trusted Firmware? How are you testing it? > > Are we certain of the split between components the PSCI implementation > > must touch and those the kernel must touch? > > > >> Also add dts file to support HiKey development board which > >> based on Hi6220 SoC and document the devicetree bindings. > >> > >> These dts files will be changed later and more nodes will be > >> added to describe other devices. > > > > How is this going to be changed other than the addition of nodes? > > > > Will this DTB continue to work in future? Or do you intend to make > > changes that will break support? > My original idea is: use spin-table for SMP now, when firmware is OK to > support PSCI, we submit another patch to replace the spin-table with PSCI. For any users who have not updated their FW, this will break booting. This is why I suggest having hte bootloader/FW fill this in as it should know what enable-method the FW supports. > If DTB should continue to work in the future, we really need to remove > the spin-table > from current dts file, how about just enable one core now? > > Which one do you favor or any other suggestion? If spin-table is just for testing while you await PSCI, drop spin-table from the dts for now. [...] > >> + timer { > >> + compatible = "arm,armv8-timer"; > >> + interrupt-parent = <&gic>; > >> + interrupts = <1 13 0xff08>, > >> + <1 14 0xff08>, > >> + <1 11 0xff08>, > >> + <1 10 0xff08>; > >> + clock-frequency = <1200000>; > >> + }; > > > > NAK. Fix your firmware to configure CNTFRQ, on all CPUs. > Fix in next version, maybe it will take some time to change firmware. Thanks. This _must_ be fixed. [...] > >> + pm_ctrl: pm_ctrl { > >> + compatible = "hisilicon,pmctrl", "syscon"; > >> + #address-cells = <1>; > >> + #size-cells = <1>; > >> + reg = <0x0 0xf7032000 0x0 0x1000>; > >> + ranges = <0 0x0 0xf7032000 0x1000>; > >> + > >> + clock_power: clock3@0 { > >> + compatible = "hisilicon,hi6220-clock-power"; > >> + reg = <0 0x1000>; > >> + #clock-cells = <1>; > >> + }; > >> + }; > > > > I really doesn't see the point in having a sub-device that covers the > > entirely of the register space of the containing node, and that being > > the case I am extremely concerned that the containers are marked as > > syscon compatible. > The SoC clocks are designed and placed under different system controllers, > so I define corresponding nodes under different controllers for clock operation. What I'm concerned wit hhere is that the pm_ctrl node and the clock3@0 sub-node have the _exact_ same register space. Given this should mean that the clock3@0 node owns that register space, having the container node export this as syscon does not make sense. And the split between pm_ctrl and clock3@) doesn't seem to make sense given they cover the same space. As I asked before, why is pm_ctrl marked as a syscon, and what's the point of the separate sub-node? Thanks, Mark.
Hello Mark, 2015-02-06 18:44 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: > On Fri, Feb 06, 2015 at 08:42:22AM +0000, Brent Wang wrote: >> Hello Mark, >> >> 2015-02-06 3:30 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: >> > On Thu, Feb 05, 2015 at 09:24:37AM +0000, Bintian Wang wrote: >> >> Add initial dtsi file to support Hisilicon Hi6220 SoC with >> >> support of Octal core CPUs in two clusters and each cluster >> >> has quard Cortex-A53. >> >> >> >> We now use the "spin-table" method for SMP, and it will be >> >> changed to PSCI later. >> > >> > If that's the case, please don't place the enable-method and related >> > properties in this file. Get your bootloader to add the appropriate >> > properties for its boot protocol. >> > >> > When is PSCI likely to appear? >> PSCI is under development. > > Sure. Do you have an estimate as to when it will appear? Another team will do the job, I can not give my estimation now. > What are you using for your PSCI implementation? The ARM Trusted > Firmware? Yes, ATF. > > How are you testing it? I think cpu hotplug can test it. > >> > Are we certain of the split between components the PSCI implementation >> > must touch and those the kernel must touch? >> > >> >> Also add dts file to support HiKey development board which >> >> based on Hi6220 SoC and document the devicetree bindings. >> >> >> >> These dts files will be changed later and more nodes will be >> >> added to describe other devices. >> > >> > How is this going to be changed other than the addition of nodes? >> > >> > Will this DTB continue to work in future? Or do you intend to make >> > changes that will break support? >> My original idea is: use spin-table for SMP now, when firmware is OK to >> support PSCI, we submit another patch to replace the spin-table with PSCI. > > For any users who have not updated their FW, this will break booting. > > This is why I suggest having hte bootloader/FW fill this in as it should > know what enable-method the FW supports. > >> If DTB should continue to work in the future, we really need to remove >> the spin-table >> from current dts file, how about just enable one core now? >> >> Which one do you favor or any other suggestion? > > If spin-table is just for testing while you await PSCI, drop spin-table > from the dts for now. So, just booting one core may be the right choice now, right? > > [...] > >> >> + timer { >> >> + compatible = "arm,armv8-timer"; >> >> + interrupt-parent = <&gic>; >> >> + interrupts = <1 13 0xff08>, >> >> + <1 14 0xff08>, >> >> + <1 11 0xff08>, >> >> + <1 10 0xff08>; >> >> + clock-frequency = <1200000>; >> >> + }; >> > >> > NAK. Fix your firmware to configure CNTFRQ, on all CPUs. >> Fix in next version, maybe it will take some time to change firmware. > > Thanks. This _must_ be fixed. > > [...] > >> >> + pm_ctrl: pm_ctrl { >> >> + compatible = "hisilicon,pmctrl", "syscon"; >> >> + #address-cells = <1>; >> >> + #size-cells = <1>; >> >> + reg = <0x0 0xf7032000 0x0 0x1000>; >> >> + ranges = <0 0x0 0xf7032000 0x1000>; >> >> + >> >> + clock_power: clock3@0 { >> >> + compatible = "hisilicon,hi6220-clock-power"; >> >> + reg = <0 0x1000>; >> >> + #clock-cells = <1>; >> >> + }; >> >> + }; >> > >> > I really doesn't see the point in having a sub-device that covers the >> > entirely of the register space of the containing node, and that being >> > the case I am extremely concerned that the containers are marked as >> > syscon compatible. >> The SoC clocks are designed and placed under different system controllers, >> so I define corresponding nodes under different controllers for clock operation. > > What I'm concerned wit hhere is that the pm_ctrl node and the clock3@0 > sub-node have the _exact_ same register space. > > Given this should mean that the clock3@0 node owns that register space, > having the container node export this as syscon does not make sense. And > the split between pm_ctrl and clock3@) doesn't seem to make sense given > they cover the same space. I understand your worry and will find the max offset of those clocks under this controller. > > As I asked before, why is pm_ctrl marked as a syscon, and what's the > point of the separate sub-node? There is no big difference between pm_ctrl and other controller, they are all designed as the base address to control some functions of other modules (certainly include some clock gates). Maybe only one node is enough, not one node plus one sub-node ? > > Thanks, > Mark.
Hello Marc, 2015-02-06 17:07 GMT+08:00 Marc Zyngier <marc.zyngier@arm.com>: > On 06/02/15 08:42, Brent Wang wrote: > > [...] > >>> >>>> + <0x0 0xf6802000 0x0 0x2000>, /* GICC */ >>>> + <0x0 0xf6804000 0x0 0x2000>, /* GICH */ >>>> + <0x0 0xf6806000 0x0 0x2000>; /* GICV */ >>> >>> I guess no-one's bothered to consider 64k pages? >>> >>> Given GICH and GICV, I hope that this platform is booted at EL2? >> Transfer from EL3 to EL1 directly, keep these two just for future use. > > That's a real shame, as it keeps users away from some key aspects of the > ARMv8 architecture. > >>> >>>> + #interrupt-cells = <3>; >>>> + #address-cells = <0>; >>>> + interrupt-controller; > > And if you're keeping GICH/GICV, where is the maintenance interrupt? > >>>> + }; >>>> + >>>> + >>>> + timer { >>>> + compatible = "arm,armv8-timer"; >>>> + interrupt-parent = <&gic>; >>>> + interrupts = <1 13 0xff08>, >>>> + <1 14 0xff08>, >>>> + <1 11 0xff08>, >>>> + <1 10 0xff08>; >>>> + clock-frequency = <1200000>; >>>> + }; >>> >>> NAK. Fix your firmware to configure CNTFRQ, on all CPUs. >> Fix in next version, maybe it will take some time to change firmware. > > While you're at it, make sure CNTVOFF_EL2 is set to zero on all CPUs > before dropping to EL1. This tends to be overlooked. Thank you for reminding me, I will keep that in mind. Thanks, > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
On Fri, Feb 06, 2015 at 03:37:52PM +0000, Brent Wang wrote: > Hello Mark, > > 2015-02-06 18:44 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: > > On Fri, Feb 06, 2015 at 08:42:22AM +0000, Brent Wang wrote: > >> Hello Mark, > >> > >> 2015-02-06 3:30 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: > >> > On Thu, Feb 05, 2015 at 09:24:37AM +0000, Bintian Wang wrote: > >> >> Add initial dtsi file to support Hisilicon Hi6220 SoC with > >> >> support of Octal core CPUs in two clusters and each cluster > >> >> has quard Cortex-A53. > >> >> > >> >> We now use the "spin-table" method for SMP, and it will be > >> >> changed to PSCI later. > >> > > >> > If that's the case, please don't place the enable-method and related > >> > properties in this file. Get your bootloader to add the appropriate > >> > properties for its boot protocol. > >> > > >> > When is PSCI likely to appear? > >> PSCI is under development. > > > > Sure. Do you have an estimate as to when it will appear? > Another team will do the job, I can not give my estimation now. Ok. > > What are you using for your PSCI implementation? The ARM Trusted > > Firmware? > Yes, ATF. > > > > How are you testing it? > I think cpu hotplug can test it. > > > > >> > Are we certain of the split between components the PSCI implementation > >> > must touch and those the kernel must touch? > >> > > >> >> Also add dts file to support HiKey development board which > >> >> based on Hi6220 SoC and document the devicetree bindings. > >> >> > >> >> These dts files will be changed later and more nodes will be > >> >> added to describe other devices. > >> > > >> > How is this going to be changed other than the addition of nodes? > >> > > >> > Will this DTB continue to work in future? Or do you intend to make > >> > changes that will break support? > >> My original idea is: use spin-table for SMP now, when firmware is OK to > >> support PSCI, we submit another patch to replace the spin-table with PSCI. > > > > For any users who have not updated their FW, this will break booting. > > > > This is why I suggest having hte bootloader/FW fill this in as it should > > know what enable-method the FW supports. > > > >> If DTB should continue to work in the future, we really need to remove > >> the spin-table > >> from current dts file, how about just enable one core now? > >> > >> Which one do you favor or any other suggestion? > > > > If spin-table is just for testing while you await PSCI, drop spin-table > > from the dts for now. > So, just booting one core may be the right choice now, right? Without an enable-method for secondary CPUs, that will be all that's possible. If your FW/bootlaoder injects the appropriate enable-method, then you could gain spin-table based SMP bringup while awaiting PSCI, without there being a DTB compatibility issue. [...] > >> >> + pm_ctrl: pm_ctrl { > >> >> + compatible = "hisilicon,pmctrl", "syscon"; > >> >> + #address-cells = <1>; > >> >> + #size-cells = <1>; > >> >> + reg = <0x0 0xf7032000 0x0 0x1000>; > >> >> + ranges = <0 0x0 0xf7032000 0x1000>; > >> >> + > >> >> + clock_power: clock3@0 { > >> >> + compatible = "hisilicon,hi6220-clock-power"; > >> >> + reg = <0 0x1000>; > >> >> + #clock-cells = <1>; > >> >> + }; > >> >> + }; > >> > > >> > I really doesn't see the point in having a sub-device that covers the > >> > entirely of the register space of the containing node, and that being > >> > the case I am extremely concerned that the containers are marked as > >> > syscon compatible. > >> The SoC clocks are designed and placed under different system controllers, > >> so I define corresponding nodes under different controllers for clock operation. > > > > What I'm concerned wit hhere is that the pm_ctrl node and the clock3@0 > > sub-node have the _exact_ same register space. > > > > Given this should mean that the clock3@0 node owns that register space, > > having the container node export this as syscon does not make sense. And > > the split between pm_ctrl and clock3@) doesn't seem to make sense given > > they cover the same space. > I understand your worry and will find the max offset of those clocks > under this controller. > > > > > As I asked before, why is pm_ctrl marked as a syscon, and what's the > > point of the separate sub-node? > There is no big difference between pm_ctrl and other controller, they > are all designed as > the base address to control some functions of other modules (certainly > include some clock gates). Are they just different instances of the same IP block, or are there fundamental differences between them? > Maybe only one node is enough, not one node plus one sub-node ? At least in the case above, I cannot see a reason to have more than a single node without a child. Thanks, Mark.
Hello Mark, 2015-02-10 21:37 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: > On Fri, Feb 06, 2015 at 03:37:52PM +0000, Brent Wang wrote: >> Hello Mark, >> >> 2015-02-06 18:44 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: >> > On Fri, Feb 06, 2015 at 08:42:22AM +0000, Brent Wang wrote: >> >> Hello Mark, >> >> >> >> 2015-02-06 3:30 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: >> >> > On Thu, Feb 05, 2015 at 09:24:37AM +0000, Bintian Wang wrote: >> >> >> Add initial dtsi file to support Hisilicon Hi6220 SoC with >> >> >> support of Octal core CPUs in two clusters and each cluster >> >> >> has quard Cortex-A53. >> >> >> >> >> >> We now use the "spin-table" method for SMP, and it will be >> >> >> changed to PSCI later. >> >> > >> >> > If that's the case, please don't place the enable-method and related >> >> > properties in this file. Get your bootloader to add the appropriate >> >> > properties for its boot protocol. >> >> > >> >> > When is PSCI likely to appear? >> >> PSCI is under development. >> > >> > Sure. Do you have an estimate as to when it will appear? >> Another team will do the job, I can not give my estimation now. > > Ok. > >> > What are you using for your PSCI implementation? The ARM Trusted >> > Firmware? >> Yes, ATF. >> > >> > How are you testing it? >> I think cpu hotplug can test it. >> >> > >> >> > Are we certain of the split between components the PSCI implementation >> >> > must touch and those the kernel must touch? >> >> > >> >> >> Also add dts file to support HiKey development board which >> >> >> based on Hi6220 SoC and document the devicetree bindings. >> >> >> >> >> >> These dts files will be changed later and more nodes will be >> >> >> added to describe other devices. >> >> > >> >> > How is this going to be changed other than the addition of nodes? >> >> > >> >> > Will this DTB continue to work in future? Or do you intend to make >> >> > changes that will break support? >> >> My original idea is: use spin-table for SMP now, when firmware is OK to >> >> support PSCI, we submit another patch to replace the spin-table with PSCI. >> > >> > For any users who have not updated their FW, this will break booting. >> > >> > This is why I suggest having hte bootloader/FW fill this in as it should >> > know what enable-method the FW supports. >> > >> >> If DTB should continue to work in the future, we really need to remove >> >> the spin-table >> >> from current dts file, how about just enable one core now? >> >> >> >> Which one do you favor or any other suggestion? >> > >> > If spin-table is just for testing while you await PSCI, drop spin-table >> > from the dts for now. >> So, just booting one core may be the right choice now, right? > > Without an enable-method for secondary CPUs, that will be all that's > possible. If your FW/bootlaoder injects the appropriate enable-method, > then you could gain spin-table based SMP bringup while awaiting PSCI, > without there being a DTB compatibility issue. For DTB compatibility issue, how about just describe one core in dts file? when PSCI is OK, we can add the CPU topology to the dts file again. > > [...] > >> >> >> + pm_ctrl: pm_ctrl { >> >> >> + compatible = "hisilicon,pmctrl", "syscon"; >> >> >> + #address-cells = <1>; >> >> >> + #size-cells = <1>; >> >> >> + reg = <0x0 0xf7032000 0x0 0x1000>; >> >> >> + ranges = <0 0x0 0xf7032000 0x1000>; >> >> >> + >> >> >> + clock_power: clock3@0 { >> >> >> + compatible = "hisilicon,hi6220-clock-power"; >> >> >> + reg = <0 0x1000>; >> >> >> + #clock-cells = <1>; >> >> >> + }; >> >> >> + }; >> >> > >> >> > I really doesn't see the point in having a sub-device that covers the >> >> > entirely of the register space of the containing node, and that being >> >> > the case I am extremely concerned that the containers are marked as >> >> > syscon compatible. >> >> The SoC clocks are designed and placed under different system controllers, >> >> so I define corresponding nodes under different controllers for clock operation. >> > >> > What I'm concerned wit hhere is that the pm_ctrl node and the clock3@0 >> > sub-node have the _exact_ same register space. >> > >> > Given this should mean that the clock3@0 node owns that register space, >> > having the container node export this as syscon does not make sense. And >> > the split between pm_ctrl and clock3@) doesn't seem to make sense given >> > they cover the same space. >> I understand your worry and will find the max offset of those clocks >> under this controller. >> >> > >> > As I asked before, why is pm_ctrl marked as a syscon, and what's the >> > point of the separate sub-node? >> There is no big difference between pm_ctrl and other controller, they >> are all designed as >> the base address to control some functions of other modules (certainly >> include some clock gates). > > Are they just different instances of the same IP block, or are there > fundamental differences between them? You can understand it as a different instance of the same IP block, there is no fundamental differences between them. > >> Maybe only one node is enough, not one node plus one sub-node ? > > At least in the case above, I cannot see a reason to have more than a > single node without a child. I will fix to one node in next version. Thank you Mark, BR, Bintian > > Thanks, > Mark.
> >> >> > Are we certain of the split between components the PSCI implementation > >> >> > must touch and those the kernel must touch? > >> >> > > >> >> >> Also add dts file to support HiKey development board which > >> >> >> based on Hi6220 SoC and document the devicetree bindings. > >> >> >> > >> >> >> These dts files will be changed later and more nodes will be > >> >> >> added to describe other devices. > >> >> > > >> >> > How is this going to be changed other than the addition of nodes? > >> >> > > >> >> > Will this DTB continue to work in future? Or do you intend to make > >> >> > changes that will break support? > >> >> My original idea is: use spin-table for SMP now, when firmware is OK to > >> >> support PSCI, we submit another patch to replace the spin-table with PSCI. > >> > > >> > For any users who have not updated their FW, this will break booting. > >> > > >> > This is why I suggest having hte bootloader/FW fill this in as it should > >> > know what enable-method the FW supports. > >> > > >> >> If DTB should continue to work in the future, we really need to remove > >> >> the spin-table > >> >> from current dts file, how about just enable one core now? > >> >> > >> >> Which one do you favor or any other suggestion? > >> > > >> > If spin-table is just for testing while you await PSCI, drop spin-table > >> > from the dts for now. > >> So, just booting one core may be the right choice now, right? > > > > Without an enable-method for secondary CPUs, that will be all that's > > possible. If your FW/bootlaoder injects the appropriate enable-method, > > then you could gain spin-table based SMP bringup while awaiting PSCI, > > without there being a DTB compatibility issue. > For DTB compatibility issue, how about just describe one core in dts > file? when PSCI > is OK, we can add the CPU topology to the dts file again. Given that we won't boot secondary CPUs without an enable-method, we could leave them in, but in a currently-unusable state. That would allow a bootloader to patch in the relevant properties to boot them. If you add the nodes later with PSCI properties, users with existing firmware will expect their board to boot. So I'd recommendd that the bootloader patches that in, or that the firmware is ready prior to this being merged. I suspect that the former is more likely to be possible. > > [...] > > > >> >> >> + pm_ctrl: pm_ctrl { > >> >> >> + compatible = "hisilicon,pmctrl", "syscon"; > >> >> >> + #address-cells = <1>; > >> >> >> + #size-cells = <1>; > >> >> >> + reg = <0x0 0xf7032000 0x0 0x1000>; > >> >> >> + ranges = <0 0x0 0xf7032000 0x1000>; > >> >> >> + > >> >> >> + clock_power: clock3@0 { > >> >> >> + compatible = "hisilicon,hi6220-clock-power"; > >> >> >> + reg = <0 0x1000>; > >> >> >> + #clock-cells = <1>; > >> >> >> + }; > >> >> >> + }; > >> >> > > >> >> > I really doesn't see the point in having a sub-device that covers the > >> >> > entirely of the register space of the containing node, and that being > >> >> > the case I am extremely concerned that the containers are marked as > >> >> > syscon compatible. > >> >> The SoC clocks are designed and placed under different system controllers, > >> >> so I define corresponding nodes under different controllers for clock operation. > >> > > >> > What I'm concerned wit hhere is that the pm_ctrl node and the clock3@0 > >> > sub-node have the _exact_ same register space. > >> > > >> > Given this should mean that the clock3@0 node owns that register space, > >> > having the container node export this as syscon does not make sense. And > >> > the split between pm_ctrl and clock3@) doesn't seem to make sense given > >> > they cover the same space. > >> I understand your worry and will find the max offset of those clocks > >> under this controller. > >> > >> > > >> > As I asked before, why is pm_ctrl marked as a syscon, and what's the > >> > point of the separate sub-node? > >> There is no big difference between pm_ctrl and other controller, they > >> are all designed as > >> the base address to control some functions of other modules (certainly > >> include some clock gates). > > > > Are they just different instances of the same IP block, or are there > > fundamental differences between them? > You can understand it as a different instance of the same IP block, > there is no fundamental > differences between them. Ok. If that's the case each should have the same compatible string. Thanks, Mark.
Hello Mark, 2015-02-10 23:27 GMT+08:00 Mark Rutland <mark.rutland@arm.com>: [...] >> > >> >> >> >> + pm_ctrl: pm_ctrl { >> >> >> >> + compatible = "hisilicon,pmctrl", "syscon"; >> >> >> >> + #address-cells = <1>; >> >> >> >> + #size-cells = <1>; >> >> >> >> + reg = <0x0 0xf7032000 0x0 0x1000>; >> >> >> >> + ranges = <0 0x0 0xf7032000 0x1000>; >> >> >> >> + >> >> >> >> + clock_power: clock3@0 { >> >> >> >> + compatible = "hisilicon,hi6220-clock-power"; >> >> >> >> + reg = <0 0x1000>; >> >> >> >> + #clock-cells = <1>; >> >> >> >> + }; >> >> >> >> + }; >> >> >> > >> >> >> > I really doesn't see the point in having a sub-device that covers the >> >> >> > entirely of the register space of the containing node, and that being >> >> >> > the case I am extremely concerned that the containers are marked as >> >> >> > syscon compatible. >> >> >> The SoC clocks are designed and placed under different system controllers, >> >> >> so I define corresponding nodes under different controllers for clock operation. >> >> > >> >> > What I'm concerned wit hhere is that the pm_ctrl node and the clock3@0 >> >> > sub-node have the _exact_ same register space. >> >> > >> >> > Given this should mean that the clock3@0 node owns that register space, >> >> > having the container node export this as syscon does not make sense. And >> >> > the split between pm_ctrl and clock3@) doesn't seem to make sense given >> >> > they cover the same space. >> >> I understand your worry and will find the max offset of those clocks >> >> under this controller. >> >> >> >> > >> >> > As I asked before, why is pm_ctrl marked as a syscon, and what's the >> >> > point of the separate sub-node? >> >> There is no big difference between pm_ctrl and other controller, they >> >> are all designed as >> >> the base address to control some functions of other modules (certainly >> >> include some clock gates). >> > >> > Are they just different instances of the same IP block, or are there >> > fundamental differences between them? >> You can understand it as a different instance of the same IP block, >> there is no fundamental >> differences between them. > > Ok. If that's the case each should have the same compatible string. Although they have same function, they control different domain, I should use different compatible string to distinguish different domains. Thanks, Bintian > > Thanks, > Mark.
Hello Mark, 2015-02-06 16:42 GMT+08:00 Brent Wang <wangbintian@gmail.com>: [...] >> >>> + gic: interrupt-controller@f6800000 { >>> + compatible = "arm,gic-400", "arm,cortex-a15-gic"; >> >> Surely there's no need for the "arm,cortex-a15-gic" fallback entry? What >> am I missing? > Remove it in next version. After remove "arm,cortex-a15-gic", I get the following error during kernel booting: ----- kvm [1]: Using HYP init bounce page @396d9000 kvm [1]: error: no compatible GIC node found kvm [1]: error initializing Hyp mode: -19 ----- Check code "virt/kvm/arm/vgic.c", gicv2 only "cortex-a15-gic" and gicv3 support kvm now, so I think we should keep it, how about your idea? Thanks, Bintian >> >>> + reg = <0x0 0xf6801000 0x0 0x1000>, /* GICD */ >> >> This doesn't match the unit-address. > Do you mean change to "<0x0 0xf6801000 0 0x1000>" ? > >> >>> + <0x0 0xf6802000 0x0 0x2000>, /* GICC */ >>> + <0x0 0xf6804000 0x0 0x2000>, /* GICH */ >>> + <0x0 0xf6806000 0x0 0x2000>; /* GICV */ >> >> I guess no-one's bothered to consider 64k pages? >> >> Given GICH and GICV, I hope that this platform is booted at EL2? > Transfer from EL3 to EL1 directly, keep these two just for future use. > >> >>> + #interrupt-cells = <3>; >>> + #address-cells = <0>; >>> + interrupt-controller; >>> + }; >>> + >>> + [...]
On 2015-04-12 07:40, Brent Wang wrote: > Hello Mark, > > 2015-02-06 16:42 GMT+08:00 Brent Wang <wangbintian@gmail.com>: > [...] >>> >>>> + gic: interrupt-controller@f6800000 { >>>> + compatible = "arm,gic-400", "arm,cortex-a15-gic"; >>> >>> Surely there's no need for the "arm,cortex-a15-gic" fallback entry? >>> What >>> am I missing? >> Remove it in next version. > After remove "arm,cortex-a15-gic", I get the following error during > kernel booting: > ----- > kvm [1]: Using HYP init bounce page @396d9000 > kvm [1]: error: no compatible GIC node found > kvm [1]: error initializing Hyp mode: -19 > ----- > > Check code "virt/kvm/arm/vgic.c", gicv2 only "cortex-a15-gic" and > gicv3 support kvm now, > so I think we should keep it, how about your idea? Please look at commit 0f37247574b3 that is queued for merge in 4.1. It adds the required compatibility strings to KVM, so Mark is perfectly right to ask you to drop this "cortex-a15-gic" from your DT. This DT won't be merged before 4.1 anyway, so there is no point trying to make it work with older kernels. Thanks, M.
Hello Marc, 2015-04-12 18:57 GMT+08:00 Marc Zyngier <marc.zyngier@arm.com>: > On 2015-04-12 07:40, Brent Wang wrote: >> >> Hello Mark, >> >> 2015-02-06 16:42 GMT+08:00 Brent Wang <wangbintian@gmail.com>: >> [...] >>>> >>>> >>>>> + gic: interrupt-controller@f6800000 { >>>>> + compatible = "arm,gic-400", "arm,cortex-a15-gic"; >>>> >>>> >>>> Surely there's no need for the "arm,cortex-a15-gic" fallback entry? What >>>> am I missing? >>> >>> Remove it in next version. >> >> After remove "arm,cortex-a15-gic", I get the following error during >> kernel booting: >> ----- >> kvm [1]: Using HYP init bounce page @396d9000 >> kvm [1]: error: no compatible GIC node found >> kvm [1]: error initializing Hyp mode: -19 >> ----- >> >> Check code "virt/kvm/arm/vgic.c", gicv2 only "cortex-a15-gic" and >> gicv3 support kvm now, >> so I think we should keep it, how about your idea? > > > Please look at commit 0f37247574b3 that is queued for merge in 4.1. > It adds the required compatibility strings to KVM, so Mark is perfectly > right to ask you to drop this "cortex-a15-gic" from your DT. Thanks for the information. > This DT won't be merged before 4.1 anyway, so there is no point trying > to make it work with older kernels. > > Thanks, > > M. > -- > Fast, cheap, reliable. Pick two.
diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt index f717c7b..5eb6b41 100644 --- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt @@ -9,6 +9,9 @@ HiP04 D01 Board Required root node properties: - compatible = "hisilicon,hip04-d01"; +HiKey Board +Required root node properties: + - compatible = "hisilicon,hi6220-hikey"; Hisilicon system controller @@ -62,6 +65,36 @@ Example: }; ----------------------------------------------------------------------- +Hisilicon Power Always ON domain controller + +Required properties: +- compatible : "hisilicon,aoctrl" +- reg : Register address and size + +Some clock registers are defined in power always on system controller, +especially in Hi6220 SoC which is used for mobile platform. + +----------------------------------------------------------------------- +Hisilicon Media domain controller + +Required properties: +- compatible : "hisilicon,mediactrl" +- reg : Register address and size + +Some clock registers of media module are defined in media system +controller, especially in Hi6220 SoC which is used for mobile platform. + +----------------------------------------------------------------------- +Hisilicon Power Management domain controller + +Required properties: +- compatible : "hisilicon,pmctrl" +- reg : Register address and size + +Some clock registers and PMU registers are defined in power management +controller, especially in Hin6220 SoC which is used for mobile platform. + +----------------------------------------------------------------------- Fabric: Required Properties: diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile index c62b0f4..bffd6b7 100644 --- a/arch/arm64/boot/dts/Makefile +++ b/arch/arm64/boot/dts/Makefile @@ -2,5 +2,6 @@ dts-dirs += amd dts-dirs += apm dts-dirs += arm dts-dirs += cavium +dts-dirs += hisilicon subdir-y := $(dts-dirs) diff --git a/arch/arm64/boot/dts/hisilicon/Makefile b/arch/arm64/boot/dts/hisilicon/Makefile new file mode 100644 index 0000000..fa81a6e --- /dev/null +++ b/arch/arm64/boot/dts/hisilicon/Makefile @@ -0,0 +1,5 @@ +dtb-$(CONFIG_ARCH_HISI) += hi6220-hikey.dtb + +always := $(dtb-y) +subdir-y := $(dts-dirs) +clean-files := *.dtb diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts new file mode 100644 index 0000000..a94da84 --- /dev/null +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts @@ -0,0 +1,31 @@ +/* + * dts file for Hisilicon HiKey Development Board + * + * Copyright (C) 2015, Hisilicon Ltd. + * + */ + +/dts-v1/; + +/memreserve/ 0x0740f000 0x1000; + +#include "hi6220.dtsi" + +/ { + model = "HiKey Development Board"; + compatible = "hisilicon,hi6220-hikey"; + #address-cells = <2>; + #size-cells = <2>; + interrupt-parent = <&gic>; + + aliases { + serial0 = &uart0; + }; + + chosen { }; + + memory@7400000 { + device_type = "memory"; + reg = <0x0 0x07400000 0x0 0x38c00000>; + }; +}; diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi new file mode 100644 index 0000000..53ba9cf --- /dev/null +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi @@ -0,0 +1,204 @@ +/* + * dts file for Hisilicon Hi6220 SoC + * + * Copyright (C) 2015, Hisilicon Ltd. + */ + +#include <dt-bindings/clock/hi6220-clock.h> + +/ { + 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>; + }; + }; + }; + + cpus { + #address-cells = <2>; + #size-cells = <0>; + + cpu0: cpu@000 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x0>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x740fff8>; + clock-latency = <0>; + }; + cpu1: cpu@001 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x1>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x740fff8>; + clock-latency = <0>; + }; + cpu2: cpu@002 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x2>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x740fff8>; + clock-latency = <0>; + }; + cpu3: cpu@003 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x3>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x740fff8>; + clock-latency = <0>; + }; + cpu4: cpu@100 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x100>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x740fff8>; + clock-latency = <0>; + }; + cpu5: cpu@101 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x101>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x740fff8>; + clock-latency = <0>; + }; + cpu6: cpu@102 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x102>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x740fff8>; + clock-latency = <0>; + }; + cpu7: cpu@103 { + compatible = "arm,cortex-a53", "arm,armv8"; + device_type = "cpu"; + reg = <0x0 0x103>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x740fff8>; + clock-latency = <0>; + }; + }; + + gic: interrupt-controller@f6800000 { + compatible = "arm,gic-400", "arm,cortex-a15-gic"; + reg = <0x0 0xf6801000 0x0 0x1000>, /* GICD */ + <0x0 0xf6802000 0x0 0x2000>, /* GICC */ + <0x0 0xf6804000 0x0 0x2000>, /* GICH */ + <0x0 0xf6806000 0x0 0x2000>; /* GICV */ + #interrupt-cells = <3>; + #address-cells = <0>; + interrupt-controller; + }; + + + timer { + compatible = "arm,armv8-timer"; + interrupt-parent = <&gic>; + interrupts = <1 13 0xff08>, + <1 14 0xff08>, + <1 11 0xff08>, + <1 10 0xff08>; + clock-frequency = <1200000>; + }; + + soc { + compatible = "simple-bus"; + #address-cells = <2>; + #size-cells = <2>; + interrupt-parent = <&gic>; + ranges; + + ao_ctrl: ao_ctrl { + compatible = "hisilicon,aoctrl", "syscon"; + #address-cells = <1>; + #size-cells = <1>; + reg = <0x0 0xf7800000 0x0 0x2000>; + ranges = <0 0x0 0xf7800000 0x2000>; + + clock_ao: clock0@0 { + compatible = "hisilicon,hi6220-clock-ao"; + reg = <0 0x1000>; + #clock-cells = <1>; + }; + }; + + sys_ctrl: sys_ctrl { + compatible = "hisilicon,sysctrl", "syscon"; + #address-cells = <1>; + #size-cells = <1>; + reg = <0x0 0xf7030000 0x0 0x2000>; + ranges = <0 0x0 0xf7030000 0x2000>; + + clock_sys: clock1@0 { + compatible = "hisilicon,hi6220-clock-sys"; + reg = <0 0x1000>; + #clock-cells = <1>; + }; + }; + + media_ctrl: media_ctrl { + compatible = "hisilicon,mediactrl", "syscon"; + #address-cells = <1>; + #size-cells = <1>; + reg = <0x0 0xf4410000 0x0 0x1000>; + ranges = <0 0x0 0xf4410000 0x1000>; + + clock_media: clock2@0 { + compatible = "hisilicon,hi6220-clock-media"; + reg = <0 0x1000>; + #clock-cells = <1>; + }; + }; + + pm_ctrl: pm_ctrl { + compatible = "hisilicon,pmctrl", "syscon"; + #address-cells = <1>; + #size-cells = <1>; + reg = <0x0 0xf7032000 0x0 0x1000>; + ranges = <0 0x0 0xf7032000 0x1000>; + + clock_power: clock3@0 { + compatible = "hisilicon,hi6220-clock-power"; + reg = <0 0x1000>; + #clock-cells = <1>; + }; + }; + + uart0: uart@f8015000 { /* console */ + compatible = "arm,pl011", "arm,primecell"; + reg = <0x0 0xf8015000 0x0 0x1000>; + interrupts = <0 36 4>; + clocks = <&clock_ao HI6220_UART0_PCLK>, <&clock_ao HI6220_UART0_PCLK>; + clock-names = "uartclk", "apb_pclk"; + }; + }; +};