Am 25.06.19 um 02:45 schrieb Guo Ren: > Thx for the patch. No need seperate part into dtsi, Sorry, I know from many arm contributions that using a .dtsi is the right thing here. It logically separates the chip from the board, even if there's only one evaluation board currently. Think about set-top boxes that someone might author a .dts for - they should be able to reuse the .dtsi for the SoC rather than copy it. > just follow: > https://lore.kernel.org/linux-csky/1561376581-19568-1-git-send-email-guoren@xxxxxxxxxx/T/#u Thanks for that pointer! I still think my node names are cleaner and also the structure of keeping clocks and gpio users outside of /soc. I see the value you use is 27 MHz, will try it tomorrow. I see you use nice KEY_ constants, whereas I just took the raw values from the dtb. I notice that your patch doesn't have any Copyright header, how should I credit you in the resulting combined patch? I would then also add your SoB from the patch you linked to. More comments inline... > On Tue, Jun 25, 2019 at 8:28 AM Andreas Färber <afaerber@xxxxxxx> wrote: >> >> Add Device Trees for NationalChip GX6605S SoC (based on CK610 CPU) and its >> dev board. GxLoader expects as filename gx6605s.dtb, so keep that. >> The bootargs are prepared to boot from USB and to output to serial. >> >> Compatibles for the SoC and board are left out for now. >> >> Signed-off-by: Andreas Färber <afaerber@xxxxxxx> >> --- >> arch/csky/boot/dts/gx6605s.dts | 104 ++++++++++++++++++++++++++++++++++++++++ >> arch/csky/boot/dts/gx6605s.dtsi | 82 +++++++++++++++++++++++++++++++ >> 2 files changed, 186 insertions(+) >> create mode 100644 arch/csky/boot/dts/gx6605s.dts >> create mode 100644 arch/csky/boot/dts/gx6605s.dtsi >> >> diff --git a/arch/csky/boot/dts/gx6605s.dts b/arch/csky/boot/dts/gx6605s.dts >> new file mode 100644 >> index 000000000000..f7511024ec6f >> --- /dev/null >> +++ b/arch/csky/boot/dts/gx6605s.dts [...] >> + leds { >> + compatible = "gpio-leds"; >> + >> + led0 { >> + label = "led10"; I forgot to align the numbering here. The label matches the GPIO and what is printed on the board. >> + gpios = <&gpio 10 GPIO_ACTIVE_LOW>; >> + linux,default-trigger = "heartbeat"; This green one stops blinking and stays on. >> + }; >> + >> + led1 { >> + label = "led11"; >> + gpios = <&gpio 11 GPIO_ACTIVE_LOW>; >> + linux,default-trigger = "timer"; This red one keeps blinking after the panic. >> + }; >> + >> + led2 { >> + label = "led12"; >> + gpios = <&gpio 12 GPIO_ACTIVE_LOW>; >> + linux,default-trigger = "default-on"; >> + }; >> + >> + led3 { >> + label = "led13"; >> + gpios = <&gpio 13 GPIO_ACTIVE_LOW>; >> + linux,default-trigger = "default-on"; These two remain off. So I wonder whether the GPIO polarity is wrong? In the example usb.img the gpio-leds module is not loaded by default, so maybe it wasn't noticed before? Also, many arm boards use more complex LED labels with multiple parts separated by colon, like "boardname:name:function" or so. >> + }; >> + }; [...] >> diff --git a/arch/csky/boot/dts/gx6605s.dtsi b/arch/csky/boot/dts/gx6605s.dtsi >> new file mode 100644 >> index 000000000000..956af5674add >> --- /dev/null >> +++ b/arch/csky/boot/dts/gx6605s.dtsi >> @@ -0,0 +1,82 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later OR BSD-2-Clause */ >> +/* >> + * NationalChip GX6605S SoC >> + * >> + * Copyright (c) 2019 Andreas Färber >> + */ >> + >> +/ { >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + cpus { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + cpu0: cpu@0 { >> + device_type = "cpu"; >> + compatible = "csky,ck610"; >> + reg = <0>; >> + }; >> + }; >> + >> + soc { >> + compatible = "simple-bus"; >> + interrupt-parent = <&intc>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + timer0: timer@20a000 { >> + compatible = "csky,gx6605s-timer"; The reason I left out the compatible for the SoC/board is that it looks unclean to me that you're using a "csky," vendor prefix for interrupt controller and timer instead of a new "nationalchip," prefix for the SoC vendor. Did I miss some reasoning for that, or did that slip through patch review? Being the first board we'd need to create a new YAML file to document them, I assume. Not sure what the best scope (=name) would be here. >> + reg = <0x0020a000 0x400>; >> + clocks = <&dummy_apb_clk>; >> + interrupts = <10>; >> + }; [...] >> + intc: interrupt-controller@500000 { >> + compatible = "csky,gx6605s-intc"; Here's the other SoC compatible. >> + reg = <0x00500000 0x400>; >> + interrupt-controller; >> + #interrupt-cells = <1>; >> + }; [snip] Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg)