| Message ID | 1411992293-7729-3-git-send-email-zhang.lyra@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 29 September 2014 20:04:49 zhang.lyra@gmail.com wrote: > + > +/memreserve/ 0x80000000 0x00010000; Maybe add a comment explaining why it is reserved? > + chosen { > + bootargs = "earlycon=serial_sprd,0x70000000"; > + }; Just remove this for now, the command line should really be set by the boot loader, not hardcoded in the dts file. IIRC, the earlycon=... syntax is not recommended on DT based systems, better use the "stdout-path" syntax instead. > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + cpu@0 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x0>; > + enable-method = "spin-table"; > + cpu-release-addr = <0x0 0x8000fff8>; > + }; New platforms should avoid using "spin-table" method. Please change the boot loader to implement PSCI if you can. > + memory@80000000 { > + device_type = "memory"; > + reg = <0 0x80000000 0 0x20000000>; > + }; > + > + aliases { > + serial0 = &uart0; > + serial1 = &uart1; > + }; These two belong into a board-specific file. I'd suggest renaming this file to sprd_shark64.dtsi (or whatever the SoC is called) and adding a .dts file that includes this and sets the aliases and memory node according to what the particular board has. > + intc:interrupt-controller@71400000 { > + compatible = "sprd,intc"; > + #interrupt-cells = <0>; > + interrupt-controller; > + reg = <0 0x71400000 0 0x1000>, > + <0 0x71500000 0 0x1000>, > + <0 0x71600000 0 0x1000>, > + <0 0x71700000 0 0x1000>; > + }; This interrupt controller does not have a binding, so please add at least a documentation for the binding as well. The name should probably be more specific that this. Also, explain why you have an interrupt controller that has four memory regions but only one interrupt sink but no interrupt source. I assume this is a nested interrupt controller to which some internal peripherals are connected? > + uart0: uart@70000000 { > + compatible = "sprd,serial"; > + reg = <0 0x70000000 0 0x100>; > + interrupts = <0 2 0xf04>; > + }; > + > + uart1: uart@70100000 { > + compatible = "sprd,serial"; > + reg = <0 0x70100000 0 0x100>; > + interrupts = <0 3 0xf04>; > + }; Please rename these to serial@70000000. Like the interrupt controller, it would be good to have a more specific compatible string. You may want to add a status="disabled" property in both these nodes, and override them from the board.dts file for the ports that are actually available. Arnd
On Mon, Sep 29, 2014 at 02:47:47PM +0100, Arnd Bergmann wrote: > On Monday 29 September 2014 20:04:49 zhang.lyra@gmail.com wrote: > > + cpus { > > + #address-cells = <2>; > > + #size-cells = <0>; > > + > > + cpu@0 { > > + device_type = "cpu"; > > + compatible = "arm,armv8"; > > + reg = <0x0 0x0>; > > + enable-method = "spin-table"; > > + cpu-release-addr = <0x0 0x8000fff8>; > > + }; > > New platforms should avoid using "spin-table" method. Please > change the boot loader to implement PSCI if you can. As usual, I strongly recommend PSCI here if the CPU supports EL3. It would solve the cpuidle kernel support as well with the generic cpuidle driver and the PSCI back-end.
On Mon, Sep 29, 2014 at 01:04:49PM +0100, zhang.lyra@gmail.com wrote: > From: "zhizhou.zhang" <zhizhou.zhang@spreadtrum.com> > > Adds the device tree support for Spreadtrum Shark64 SoC based on ARMv8 architecture. > > Signed-off-by: zhizhou.zhang <zhizhou.zhang@spreadtrum.com> > Signed-off-by: chunyan.zhang <chunyan.zhang@spreadtrum.com> > --- > arch/arm64/boot/dts/sprd_shark64.dts | 110 ++++++++++++++++++++++++++++++++++ > 1 file changed, 110 insertions(+) > create mode 100644 arch/arm64/boot/dts/sprd_shark64.dts > > diff --git a/arch/arm64/boot/dts/sprd_shark64.dts b/arch/arm64/boot/dts/sprd_shark64.dts > new file mode 100644 > index 0000000..537cd6d > --- /dev/null > +++ b/arch/arm64/boot/dts/sprd_shark64.dts > @@ -0,0 +1,110 @@ > +/* > + * dts file for Spreadtrum(sprd) Shark64 SOC > + * > + * Copyright (C) 2014, Spreadtrum Communications Inc. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + */ > + > +/dts-v1/; > + > +/memreserve/ 0x80000000 0x00010000; What is this protecting? Please add a comment. > + > +/ { > + model = "shark64 Board"; > + compatible = "sprd,shark64"; This feels like it would make more sense as an SoC dtsi to be included by various boards (which can override the model and compatible). > + interrupt-parent = <&gic>; > + #address-cells = <2>; > + #size-cells = <2>; > + > + chosen { > + bootargs = "earlycon=serial_sprd,0x70000000"; Can we not use stdout-path for this? > + }; > + > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + cpu@0 { > + device_type = "cpu"; > + compatible = "arm,armv8"; Can we have the particular CPU name, please? > + reg = <0x0 0x0>; > + enable-method = "spin-table"; > + cpu-release-addr = <0x0 0x8000fff8>; No PSCI? What are you using for your bootloader/firmware? If you must use spin-table, please give each CPU a unique release address. > + }; > + cpu@1 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x1>; > + enable-method = "spin-table"; > + cpu-release-addr = <0x0 0x8000fff8>; > + }; > + cpu@2 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x2>; > + enable-method = "spin-table"; > + cpu-release-addr = <0x0 0x8000fff8>; > + }; > + cpu@3 { > + device_type = "cpu"; > + compatible = "arm,armv8"; > + reg = <0x0 0x3>; > + enable-method = "spin-table"; > + cpu-release-addr = <0x0 0x8000fff8>; > + }; > + }; > + > + memory@80000000 { > + device_type = "memory"; > + reg = <0 0x80000000 0 0x20000000>; > + }; > + > + aliases { > + serial0 = &uart0; > + serial1 = &uart1; > + }; Could you move this up above chosen, please? > + > + gic: interrupt-controller@12001000 { > + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; > + #interrupt-cells = <3>; > + #address-cells = <0>; > + interrupt-controller; > + reg = <0 0x12001000 0 0x1000>, > + <0 0x12002000 0 0x1000>; No GICH or GICV? Which exception level do your CPUs boot in? I would strongly recommend booting at EL2. That gives the kernel more flexibility to perform fixups (e.g. zeroing CNTVOFF), and requires less work in your bootloader. > + }; > + > + intc:interrupt-controller@71400000 { > + compatible = "sprd,intc"; > + #interrupt-cells = <0>; > + interrupt-controller; > + reg = <0 0x71400000 0 0x1000>, > + <0 0x71500000 0 0x1000>, > + <0 0x71600000 0 0x1000>, > + <0 0x71700000 0 0x1000>; > + }; This binding doesn't exist in mainline, and isn't added by this series. I'm especially confused by the #interrupt-cells = <0>. What exactly is this, and how do you intend to use it? > + > + timer { > + compatible = "arm,armv8-timer"; > + interrupts = <1 13 0xff01>, > + <1 14 0xff01>, > + <1 11 0xff01>, > + <1 10 0xff01>; > + clock-frequency = <26000000>; Please have your FW or bootloader program CNTFRQ_EL1 on each CPU, and get rid of the clock-frequency property here. > + }; > + > + uart0: uart@70000000 { > + compatible = "sprd,serial"; > + reg = <0 0x70000000 0 0x100>; > + interrupts = <0 2 0xf04>; > + }; No clocks/dmas/etc necessary? Mark. > + > + uart1: uart@70100000 { > + compatible = "sprd,serial"; > + reg = <0 0x70100000 0 0x100>; > + interrupts = <0 3 0xf04>; > + }; > +}; > -- > 1.7.9.5 > >
Hi, Arnd 2014-09-29 21:47 GMT+08:00 Arnd Bergmann <arnd@arndb.de>: > On Monday 29 September 2014 20:04:49 zhang.lyra@gmail.com wrote: >> + >> +/memreserve/ 0x80000000 0x00010000; > > Maybe add a comment explaining why it is reserved? > >> + chosen { >> + bootargs = "earlycon=serial_sprd,0x70000000"; >> + }; > > Just remove this for now, the command line should really be set by the > boot loader, not hardcoded in the dts file. > > IIRC, the earlycon=... syntax is not recommended on DT based systems, > better use the "stdout-path" syntax instead. > I have tried to use "stdout-path" instead of "bootargs= "earlycon= ..." like below : / { ... chosen { stdout-path = "/serial@70000000"; }; uart0: serial@70000000 { status="okay"; }; ... }; But then there is nothing output information on serial console.(I have been testing in Fast Model) I saw the below code in init/main.c /* Check for early params. */ static int __init do_early_param(char *param, char *val, const char *unused) { const struct obs_kernel_param *p; for (p = __setup_start; p < __setup_end; p++) { if ((p->early && parameq(param, p->str)) || (strcmp(param, "console") == 0 && strcmp(p->str, "earlycon") == 0) ) { if (p->setup_func(val) != 0) pr_warn("Malformed early option '%s'\n", param); } } /* We accept everything at this stage. */ return 0; } And I saw a patch from Grant Likely, he had a comment in it : "If the devicetree specifies a serial port as a stdout device, then the kernel can use it as the default console if nothing else was selected on the command line. For any serial port that uses the uart_add_one_port() feature, the uart_add_one_port() has all the information needed to automatically enable the console device, which is what this patch does." So, I guess that the reason why I can't see any output information on console after using "stdout-path" instead of "earlycon" is that I haven't a driver of Spreadtrum's serial, and dose not use the uart_add_one_port() feature. I don't know is correct what I guess. Could you give me some suggestions to solve this problem? Thanks, Chunyan
On Wednesday 15 October 2014 11:17:07 Lyra Zhang wrote: > Hi, Arnd Hi Lyra, Sorry for the late reply, I've been away travelling and am just now catching up on email. Have you found a solution or do you still need help with this? Arnd > 2014-09-29 21:47 GMT+08:00 Arnd Bergmann <arnd@arndb.de>: > > On Monday 29 September 2014 20:04:49 zhang.lyra@gmail.com wrote: > >> + > >> +/memreserve/ 0x80000000 0x00010000; > > > > Maybe add a comment explaining why it is reserved? > > > >> + chosen { > >> + bootargs = "earlycon=serial_sprd,0x70000000"; > >> + }; > > > > Just remove this for now, the command line should really be set by the > > boot loader, not hardcoded in the dts file. > > > > IIRC, the earlycon=... syntax is not recommended on DT based systems, > > better use the "stdout-path" syntax instead. > > > > I have tried to use "stdout-path" instead of "bootargs= "earlycon= > ..." like below : > > / { > ... > > chosen { > stdout-path = "/serial@70000000"; > }; > > uart0: serial@70000000 { > status="okay"; > }; > ... > }; > > But then there is nothing output information on serial console.(I have > been testing in Fast Model) > > > I saw the below code in init/main.c > > /* Check for early params. */ > static int __init do_early_param(char *param, char *val, const char *unused) > { > const struct obs_kernel_param *p; > > for (p = __setup_start; p < __setup_end; p++) { > if ((p->early && parameq(param, p->str)) || > (strcmp(param, "console") == 0 && > strcmp(p->str, "earlycon") == 0) > ) { > if (p->setup_func(val) != 0) > pr_warn("Malformed early option '%s'\n", param); > } > } > /* We accept everything at this stage. */ > return 0; > } > > And I saw a patch from Grant Likely, he had a comment in it : > "If the devicetree specifies a serial port as a stdout device, then the > kernel can use it as the default console if nothing else was selected on > the command line. For any serial port that uses the uart_add_one_port() > feature, the uart_add_one_port() has all the information needed to > automatically enable the console device, which is what this patch does." > > So, I guess that the reason why I can't see any output information on > console after using "stdout-path" instead of "earlycon" is that I > haven't a driver of Spreadtrum's serial, and dose not use the > uart_add_one_port() feature. > > I don't know is correct what I guess. > > Could you give me some suggestions to solve this problem? >
Hi, Arnd This problem have been solved, and was submitted in v2. Thanks for your answer to the question(which Orson asked for me) about this on Freenode. Best regards, Lyra 2014-10-21 3:00 GMT+08:00 Arnd Bergmann <arnd@arndb.de>: > On Wednesday 15 October 2014 11:17:07 Lyra Zhang wrote: >> Hi, Arnd > > Hi Lyra, > > Sorry for the late reply, I've been away travelling and am just > now catching up on email. Have you found a solution or do > you still need help with this? > > Arnd > >> 2014-09-29 21:47 GMT+08:00 Arnd Bergmann <arnd@arndb.de>: >> > On Monday 29 September 2014 20:04:49 zhang.lyra@gmail.com wrote: >> >> + >> >> +/memreserve/ 0x80000000 0x00010000; >> > >> > Maybe add a comment explaining why it is reserved? >> > >> >> + chosen { >> >> + bootargs = "earlycon=serial_sprd,0x70000000"; >> >> + }; >> > >> > Just remove this for now, the command line should really be set by the >> > boot loader, not hardcoded in the dts file. >> > >> > IIRC, the earlycon=... syntax is not recommended on DT based systems, >> > better use the "stdout-path" syntax instead. >> > >> >> I have tried to use "stdout-path" instead of "bootargs= "earlycon= >> ..." like below : >> >> / { >> ... >> >> chosen { >> stdout-path = "/serial@70000000"; >> }; >> >> uart0: serial@70000000 { >> status="okay"; >> }; >> ... >> }; >> >> But then there is nothing output information on serial console.(I have >> been testing in Fast Model) >> >> >> I saw the below code in init/main.c >> >> /* Check for early params. */ >> static int __init do_early_param(char *param, char *val, const char *unused) >> { >> const struct obs_kernel_param *p; >> >> for (p = __setup_start; p < __setup_end; p++) { >> if ((p->early && parameq(param, p->str)) || >> (strcmp(param, "console") == 0 && >> strcmp(p->str, "earlycon") == 0) >> ) { >> if (p->setup_func(val) != 0) >> pr_warn("Malformed early option '%s'\n", param); >> } >> } >> /* We accept everything at this stage. */ >> return 0; >> } >> >> And I saw a patch from Grant Likely, he had a comment in it : >> "If the devicetree specifies a serial port as a stdout device, then the >> kernel can use it as the default console if nothing else was selected on >> the command line. For any serial port that uses the uart_add_one_port() >> feature, the uart_add_one_port() has all the information needed to >> automatically enable the console device, which is what this patch does." >> >> So, I guess that the reason why I can't see any output information on >> console after using "stdout-path" instead of "earlycon" is that I >> haven't a driver of Spreadtrum's serial, and dose not use the >> uart_add_one_port() feature. >> >> I don't know is correct what I guess. >> >> Could you give me some suggestions to solve this problem? >> >
diff --git a/arch/arm64/boot/dts/sprd_shark64.dts b/arch/arm64/boot/dts/sprd_shark64.dts new file mode 100644 index 0000000..537cd6d --- /dev/null +++ b/arch/arm64/boot/dts/sprd_shark64.dts @@ -0,0 +1,110 @@ +/* + * dts file for Spreadtrum(sprd) Shark64 SOC + * + * Copyright (C) 2014, Spreadtrum Communications Inc. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + */ + +/dts-v1/; + +/memreserve/ 0x80000000 0x00010000; + +/ { + model = "shark64 Board"; + compatible = "sprd,shark64"; + interrupt-parent = <&gic>; + #address-cells = <2>; + #size-cells = <2>; + + chosen { + bootargs = "earlycon=serial_sprd,0x70000000"; + }; + + cpus { + #address-cells = <2>; + #size-cells = <0>; + + cpu@0 { + device_type = "cpu"; + compatible = "arm,armv8"; + reg = <0x0 0x0>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x8000fff8>; + }; + cpu@1 { + device_type = "cpu"; + compatible = "arm,armv8"; + reg = <0x0 0x1>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x8000fff8>; + }; + cpu@2 { + device_type = "cpu"; + compatible = "arm,armv8"; + reg = <0x0 0x2>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x8000fff8>; + }; + cpu@3 { + device_type = "cpu"; + compatible = "arm,armv8"; + reg = <0x0 0x3>; + enable-method = "spin-table"; + cpu-release-addr = <0x0 0x8000fff8>; + }; + }; + + memory@80000000 { + device_type = "memory"; + reg = <0 0x80000000 0 0x20000000>; + }; + + aliases { + serial0 = &uart0; + serial1 = &uart1; + }; + + gic: interrupt-controller@12001000 { + compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic"; + #interrupt-cells = <3>; + #address-cells = <0>; + interrupt-controller; + reg = <0 0x12001000 0 0x1000>, + <0 0x12002000 0 0x1000>; + }; + + intc:interrupt-controller@71400000 { + compatible = "sprd,intc"; + #interrupt-cells = <0>; + interrupt-controller; + reg = <0 0x71400000 0 0x1000>, + <0 0x71500000 0 0x1000>, + <0 0x71600000 0 0x1000>, + <0 0x71700000 0 0x1000>; + }; + + timer { + compatible = "arm,armv8-timer"; + interrupts = <1 13 0xff01>, + <1 14 0xff01>, + <1 11 0xff01>, + <1 10 0xff01>; + clock-frequency = <26000000>; + }; + + uart0: uart@70000000 { + compatible = "sprd,serial"; + reg = <0 0x70000000 0 0x100>; + interrupts = <0 2 0xf04>; + }; + + uart1: uart@70100000 { + compatible = "sprd,serial"; + reg = <0 0x70100000 0 0x100>; + interrupts = <0 3 0xf04>; + }; +};