Re: [PATCH] csky: dts: Add NationalChip GX6605S

[Andreas FÃrber]

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)