| Message ID | 20190625002829.17409-1-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | csky: dts: Add NationalChip GX6605S | expand |
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@kernel.org/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@suse.de> 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@suse.de> >> --- >> 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)
Hi Andreas, On Tue, Jun 25, 2019 at 9:25 AM Andreas Färber <afaerber@suse.de> wrote: > > 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. gx6605s.dts is simple now, it's unnecessary to seperate it into two pieces. Other things from you is all OK for me. > > > just follow: > > https://lore.kernel.org/linux-csky/1561376581-19568-1-git-send-email-guoren@kernel.org/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. Copyright could be the same in arch/csky/kernel/setup.c or add yours in addition. > > More comments inline... > > > On Tue, Jun 25, 2019 at 8:28 AM Andreas Färber <afaerber@suse.de> 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@suse.de> > >> --- > >> 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. leds and button is so specific, that's is just a example. You could keep your own style in the dts. > > >> + gpios = <&gpio 10 GPIO_ACTIVE_LOW>; > >> + linux,default-trigger = "heartbeat"; > > This green one stops blinking and stays on. Seems there is no driver for it. > > >> + }; > >> + > >> + 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? I try this 1 years ago in linux-4.9 and it need verifying. > > Also, many arm boards use more complex LED labels with multiple parts > separated by colon, like "boardname:name:function" or so. Name is Ok for me as long as it's correct. > > >> + }; > >> + }; > [...] > >> 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? csky is my current company and nationalchip is my prior company. The gx6605s is belong to nationachip and gx6605s use csky 610 as its CPU. > > 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. It's defined in irqchip/irq-csky-apb-intc.c. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
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 @@ -0,0 +1,104 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later OR BSD-2-Clause */ +/* + * GX6605S dev board + * + * Copyright (c) 2019 Andreas Färber + */ + +/dts-v1/; + +#include <dt-bindings/gpio/gpio.h> + +#include "gx6605s.dtsi" + +/ { + model = "Nationalchip GX6605S"; + + aliases { + serial0 = &uart; + }; + + chosen { + bootargs = "console=ttyS0,115200n8 root=/dev/sda2 rw rootwait"; + stdout-path = "serial0:115200n8"; + }; + + memory@10000000 { + device_type = "memory"; + reg = <0x10000000 0x04000000>; + }; + + dummy_apb_clk: dummy-apb-clk { + compatible = "fixed-clock"; + clock-frequency = <24000000>; /* guesstimate */ + #clock-cells = <0>; + }; + + buttons { + compatible = "gpio-keys-polled"; + poll-interval = <100>; + autorepeat; + + button5 { + label = "button5"; + linux,code = <103>; + gpios = <&gpio 5 GPIO_ACTIVE_LOW>; + }; + + button6 { + label = "button6"; + linux,code = <106>; + gpios = <&gpio 6 GPIO_ACTIVE_LOW>; + }; + + button7 { + label = "button7"; + linux,code = <28>; + gpios = <&gpio 7 GPIO_ACTIVE_LOW>; + }; + + button8 { + label = "button8"; + linux,code = <105>; + gpios = <&gpio 8 GPIO_ACTIVE_LOW>; + }; + + button9 { + label = "button9"; + linux,code = <108>; + gpios = <&gpio 9 GPIO_ACTIVE_LOW>; + }; + }; + + leds { + compatible = "gpio-leds"; + + led0 { + label = "led10"; + gpios = <&gpio 10 GPIO_ACTIVE_LOW>; + linux,default-trigger = "heartbeat"; + }; + + led1 { + label = "led11"; + gpios = <&gpio 11 GPIO_ACTIVE_LOW>; + linux,default-trigger = "timer"; + }; + + 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"; + }; + }; +}; + +&timer0 { + clocks = <&dummy_apb_clk>; +}; 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"; + reg = <0x0020a000 0x400>; + clocks = <&dummy_apb_clk>; + interrupts = <10>; + }; + + gpio: gpio@305000 { + compatible = "wd,mbl-gpio"; + reg-names = "dirout", "dat", "set", "clr"; + reg = <0x00305000 0x4>, + <0x00305004 0x4>, + <0x00305008 0x4>, + <0x0030500c 0x4>; + gpio-controller; + #gpio-cells = <2>; + }; + + uart: serial@403000 { + compatible = "ns16550a"; + reg = <0x00403000 0x400>; + interrupts = <15>; + clock-frequency = <29491200>; + reg-shift = <2>; + reg-io-width = <1>; + }; + + intc: interrupt-controller@500000 { + compatible = "csky,gx6605s-intc"; + reg = <0x00500000 0x400>; + interrupt-controller; + #interrupt-cells = <1>; + }; + + ehci_hcd: usb@900000 { + compatible = "generic-ehci"; + reg = <0x00900000 0x400>; + interrupts = <59>; + }; + + ohci_hcd0: usb@a00000 { + compatible = "generic-ohci"; + reg = <0x00a00000 0x400>; + interrupts = <58>; + }; + + ohci_hcd1: usb@b00000 { + compatible = "generic-ohci"; + reg = <0x00b00000 0x400>; + interrupts = <57>; + }; + }; +};
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@suse.de> --- 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 -- 2.16.4