[PATCH v16 09/16] riscv: Update Canaan Kendryte K210 device tree

Damien Le Moal Damien.LeMoal at wdc.com
Mon Feb 8 17:41:01 EST 2021


On 2021/02/09 4:50, Rob Herring wrote:
> On Fri, Feb 5, 2021 at 4:52 PM Sean Anderson <seanga2 at gmail.com> wrote:
>>
>> On 2/5/21 3:25 PM, Rob Herring wrote:
>>> On Fri, Feb 05, 2021 at 03:58:20PM +0900, Damien Le Moal wrote:
>>>> Update the Canaan Kendryte K210 base device tree k210.dtsi to define
>>>> all peripherals of the SoC, their clocks and reset lines. The device
>>>> tree file k210.dts is renamed to k210_generic.dts and becomes the
>>>> default value selection of the SOC_CANAAN_K210_DTB_BUILTIN_SOURCE
>>>> configuration option. No device beside the serial console is defined by
>>>> this device tree. This makes this generic device tree suitable for use
>>>> with a builtin initramfs with all known K210 based boards.
>>>>
>>>> These changes result in the K210_CLK_ACLK clock ID to be unused and
>>>> removed from the dt-bindings k210-clk.h header file.
>>>>
>>>> Most updates to the k210.dtsi file come from Sean Anderson's work on
>>>> U-Boot support for the K210.
>>>>
>>>> Cc: Rob Herring <robh at kernel.org>
>>>> Cc: devicetree at vger.kernel.org
>>>> Signed-off-by: Damien Le Moal <damien.lemoal at wdc.com>
>>>> ---
>>>>   arch/riscv/Kconfig.socs                     |   2 +-
>>>>   arch/riscv/boot/dts/canaan/k210.dts         |  23 -
>>>>   arch/riscv/boot/dts/canaan/k210.dtsi        | 535 +++++++++++++++++++-
>>>>   arch/riscv/boot/dts/canaan/k210_generic.dts |  46 ++
>>>>   include/dt-bindings/clock/k210-clk.h        |   1 -
>>>>   5 files changed, 554 insertions(+), 53 deletions(-)
>>>>   delete mode 100644 arch/riscv/boot/dts/canaan/k210.dts
>>>>   create mode 100644 arch/riscv/boot/dts/canaan/k210_generic.dts
>>>>
>>>> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
>>>> index 6402746c68f3..7efcece8896c 100644
>>>> --- a/arch/riscv/Kconfig.socs
>>>> +++ b/arch/riscv/Kconfig.socs
>>>> @@ -51,7 +51,7 @@ config SOC_CANAAN_K210_DTB_SOURCE
>>>>      string "Source file for the Canaan Kendryte K210 builtin DTB"
>>>>      depends on SOC_CANAAN
>>>>      depends on SOC_CANAAN_K210_DTB_BUILTIN
>>>> -    default "k210"
>>>> +    default "k210_generic"
>>>>      help
>>>>        Base name (without suffix, relative to arch/riscv/boot/dts/canaan)
>>>>        for the DTS file that will be used to produce the DTB linked into the
>>>> diff --git a/arch/riscv/boot/dts/canaan/k210.dts b/arch/riscv/boot/dts/canaan/k210.dts
>>>> deleted file mode 100644
>>>> index 0d1f28fce6b2..000000000000
>>>> --- a/arch/riscv/boot/dts/canaan/k210.dts
>>>> +++ /dev/null
>>>> @@ -1,23 +0,0 @@
>>>> -// SPDX-License-Identifier: GPL-2.0+
>>>> -/*
>>>> - * Copyright (C) 2020 Western Digital Corporation or its affiliates.
>>>> - */
>>>> -
>>>> -/dts-v1/;
>>>> -
>>>> -#include "k210.dtsi"
>>>> -
>>>> -/ {
>>>> -    model = "Kendryte K210 generic";
>>>> -    compatible = "kendryte,k210";
>>>> -
>>>> -    chosen {
>>>> -            bootargs = "earlycon console=ttySIF0";
>>>> -            stdout-path = "serial0";
>>>> -    };
>>>> -};
>>>> -
>>>> -&uarths0 {
>>>> -    status = "okay";
>>>> -};
>>>> -
>>>> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>> index 354b263195a3..63c1f4c98d6c 100644
>>>> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
>>>> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>> @@ -1,9 +1,11 @@
>>>>   // SPDX-License-Identifier: GPL-2.0+
>>>>   /*
>>>> - * Copyright (C) 2019 Sean Anderson <seanga2 at gmail.com>
>>>> + * Copyright (C) 2019-20 Sean Anderson <seanga2 at gmail.com>
>>>>    * Copyright (C) 2020 Western Digital Corporation or its affiliates.
>>>>    */
>>>>   #include <dt-bindings/clock/k210-clk.h>
>>>> +#include <dt-bindings/pinctrl/k210-fpioa.h>
>>>> +#include <dt-bindings/reset/k210-rst.h>
>>>>
>>>>   / {
>>>>      /*
>>>> @@ -12,14 +14,33 @@ / {
>>>>       */
>>>>      #address-cells = <1>;
>>>>      #size-cells = <1>;
>>>> -    compatible = "kendryte,k210";
>>>> +    compatible = "canaan,kendryte-k210";
>>>>
>>>>      aliases {
>>>> +            cpu0 = &cpu0;
>>>> +            cpu1 = &cpu1;
>>>> +            dma0 = &dmac0;
>>>> +            gpio0 = &gpio0;
>>>> +            gpio1 = &gpio1_0;
>>>> +            i2c0 = &i2c0;
>>>> +            i2c1 = &i2c1;
>>>> +            i2c2 = &i2c2;
>>>> +            pinctrl0 = &fpioa;
>>>>              serial0 = &uarths0;
>>>> +            serial1 = &uart1;
>>>> +            serial2 = &uart2;
>>>> +            serial3 = &uart3;
>>>> +            spi0 = &spi0;
>>>> +            spi1 = &spi1;
>>>> +            spi2 = &spi2;
>>>> +            spi3 = &spi3;
>>>> +            timer0 = &timer0;
>>>> +            timer1 = &timer1;
>>>> +            timer2 = &timer2;
>>>
>>> Don't add random aliases. Really, only the serialN ones should be here.
>>> cpu, dma, pinctrl, timer are definitely non-standard.
>>
>>
>> All of these except for cpu and dma are already found in the kernel.
>> They were primarily added for U-Boot.
>>
>>>
>>>>      };
>>>>
>>>>      /*
>>>> -     * The K210 has an sv39 MMU following the priviledge specification v1.9.
>>>> +     * The K210 has an sv39 MMU following the privileged specification v1.9.
>>>>       * Since this is a non-ratified draft specification, the kernel does not
>>>>       * support it and the K210 support enabled only for the !MMU case.
>>>>       * Be consistent with this by setting the CPUs MMU type to "none".
>>>> @@ -30,14 +51,14 @@ cpus {
>>>>              timebase-frequency = <7800000>;
>>>>              cpu0: cpu at 0 {
>>>>                      device_type = "cpu";
>>>> +                    compatible = "canaan,k210", "riscv";
>>>>                      reg = <0>;
>>>> -                    compatible = "kendryte,k210", "sifive,rocket0", "riscv";
>>>>                      riscv,isa = "rv64imafdc";
>>>> -                    mmu-type = "none";
>>>> -                    i-cache-size = <0x8000>;
>>>> +                    mmu-type = "riscv,none";
>>>>                      i-cache-block-size = <64>;
>>>> -                    d-cache-size = <0x8000>;
>>>> +                    i-cache-size = <0x8000>;
>>>>                      d-cache-block-size = <64>;
>>>> +                    d-cache-size = <0x8000>;
>>>>                      cpu0_intc: interrupt-controller {
>>>>                              #interrupt-cells = <1>;
>>>>                              interrupt-controller;
>>>> @@ -46,14 +67,14 @@ cpu0_intc: interrupt-controller {
>>>>              };
>>>>              cpu1: cpu at 1 {
>>>>                      device_type = "cpu";
>>>> +                    compatible = "canaan,k210", "riscv";
>>>>                      reg = <1>;
>>>> -                    compatible = "kendryte,k210", "sifive,rocket0", "riscv";
>>>>                      riscv,isa = "rv64imafdc";
>>>> -                    mmu-type = "none";
>>>> -                    i-cache-size = <0x8000>;
>>>> +                    mmu-type = "riscv,none";
>>>>                      i-cache-block-size = <64>;
>>>> -                    d-cache-size = <0x8000>;
>>>> +                    i-cache-size = <0x8000>;
>>>>                      d-cache-block-size = <64>;
>>>> +                    d-cache-size = <0x8000>;
>>>>                      cpu1_intc: interrupt-controller {
>>>>                              #interrupt-cells = <1>;
>>>>                              interrupt-controller;
>>>> @@ -64,10 +85,15 @@ cpu1_intc: interrupt-controller {
>>>>
>>>>      sram: memory at 80000000 {
>>>>              device_type = "memory";
>>>> +            compatible = "canaan,k210-sram";
>>>>              reg = <0x80000000 0x400000>,
>>>>                    <0x80400000 0x200000>,
>>>>                    <0x80600000 0x200000>;
>>>>              reg-names = "sram0", "sram1", "aisram";
>>>> +            clocks = <&sysclk K210_CLK_SRAM0>,
>>>> +                     <&sysclk K210_CLK_SRAM1>,
>>>> +                     <&sysclk K210_CLK_AI>;
>>>> +            clock-names = "sram0", "sram1", "aisram";
>>>>      };
>>>>
>>>>      clocks {
>>>> @@ -81,40 +107,493 @@ in0: oscillator {
>>>>      soc {
>>>>              #address-cells = <1>;
>>>>              #size-cells = <1>;
>>>> -            compatible = "kendryte,k210-soc", "simple-bus";
>>>> +            compatible = "simple-bus";
>>>>              ranges;
>>>>              interrupt-parent = <&plic0>;
>>>>
>>>> -            sysctl: sysctl at 50440000 {
>>>> -                    compatible = "kendryte,k210-sysctl", "simple-mfd";
>>>> -                    reg = <0x50440000 0x1000>;
>>>> -                    #clock-cells = <1>;
>>>> +            debug0: debug at 0 {
>>>> +                    compatible = "canaan,k210-debug", "riscv,debug";
>>>
>>> Not documented.
>>
>> On 1/14/21 7:06 PM, Sean Anderson wrote:
>>> Here it's because "riscv,debug" doesn't exist. This is the "debug"
>>> device as described in the debug spec. AFAIK Linux never needs to
>>> configure this device. It could probably be removed.
>>
>> No objections here.
>>
>>>
>>>> +                    reg = <0x0 0x1000>;
>>>> +                    status = "disabled";
>>>> +            };
>>>> +
>>>> +            rom0: nvmem at 1000 {
>>>> +                    reg = <0x1000 0x1000>;
>>>> +                    read-only;
>>>> +                    status = "disabled";
>>>>              };
>>>>
>>>>              clint0: clint at 2000000 {
>>>
>>> interrupt-controller at ...
>>
>> Yes, this should be changed.
>>
>>>
>>>> -                    #interrupt-cells = <1>;
>>>> -                    compatible = "riscv,clint0";
>>>> +                    compatible = "canaan,k210-clint", "sifive,clint0";
>>>>                      reg = <0x2000000 0xC000>;
>>>> -                    interrupts-extended =  <&cpu0_intc 3 &cpu0_intc 7
>>>> -                                            &cpu1_intc 3 &cpu1_intc 7>;
>>>> +                    interrupts-extended = <&cpu0_intc 3 &cpu0_intc 7
>>>> +                                          &cpu1_intc 3 &cpu1_intc 7>;
>>>>              };
>>>>
>>>> -            plic0: interrupt-controller at c000000 {
>>>> +            plic0: interrupt-controller at C000000 {
>>>
>>> No, lowercase hex in unit-addresses was correct.
>>
>> Do you have any authoritative source for this? When I was creating this
>> tree, I didn't see anything about what case the addresses should be in
>> (and a quick grep for lower-case in Documentation/devicetree doesn't
>> have any relevant results).
> 
> It's ultimately up to the bus definition, but the DT spec may say
> something. I don't recall offhand. For most bus definitions we have a
> schema enforcement and dtc has some checks. (Though I just noticed the
> simple-bus schema allows uppercase which is an error. dtc does not.)
> This case doesn't get caught because there's not a bus definition for
> root node. IMO, it should be same as 'simple-bus', but there was some
> debate about that when I added the dtc checks.
> 
> The other cases should have been flagged by simple-pm-bus.yaml. You
> all did run 'make dtbs_check' on this, right?

Yes, I did, and rerun before sending any new version of the series. No warnings
at all show about anything.

> 
>>>> +                    otp0: nvmem at 50420000 {
>>>> +                            #address-cells = <1>;
>>>> +                            #size-cells = <1>;
>>>> +                            compatible = "canaan,k210-otp";
>>>> +                            reg = <0x50420000 0x100>,
>>>> +                                  <0x88000000 0x20000>;
>>>> +                            reg-names = "reg", "mem";
>>>> +                            clocks = <&sysclk K210_CLK_ROM>;
>>>> +                            resets = <&sysrst K210_RST_ROM>;
>>>> +                            read-only;
>>>> +                            status = "disabled";
>>>
>>> Your disabled nodes seem a bit excessive. A device should really only be
>>> disabled if it's a board level decision to use or not. I'd assume the
>>> OTP is always there and usable.
>>
>> It's disabled because there is no driver for it (yet). Though see below
>> for the reasoning behind this.
> 
> No driver is not really a reason to disable. Why force a DT change
> when there is a driver?

We can omit the nodes that do not have drivers, but when the drivers are added,
there will be a need to change the DT. So isn't it the same in the end ?

> 
>>>> +
>>>> +                            /* Bootloader */
>>>> +                            firmware at 00000 {
>>>
>>> Drop leading 0s.
>>>
>>> Is this memory mapped? If so, you are missing 'ranges' in the parent to
>>> make it translateable.
>>
>> Yes, there should be ranges.
>>
>> Though I am not sure that the ROM is controllable from this OTP...
>>
>> Perhaps it should be its own node.
>>
>>>
>>>> +                                    reg = <0x00000 0xC200>;
>>>> +                            };
>>>> +
>>>> +                            /*
>>>> +                             * config string as described in RISC-V
>>>> +                             * privileged spec 1.9
>>>> +                             */
>>>> +                            config-1-9 at 1c000 {
>>>> +                                    reg = <0x1C000 0x1000>;
>>>> +                            };
>>>> +
>>>> +                            /*
>>>> +                             * Device tree containing only registers,
>>>> +                             * interrupts, and cpus
>>>> +                             */
>>>> +                            fdt at 1d000 {
>>>> +                                    reg = <0x1D000 0x2000>;
>>>> +                            };
>>>> +
>>>> +                            /* CPU/ROM credits */
>>>> +                            credits at 1f000 {
>>>> +                                    reg = <0x1F000 0x1000>;
>>>> +                            };
>>>> +                    };
>>>> +
>>>> +                    dvp0: camera at 50430000 {
>>>> +                            compatible = "canaan,k210-dvp";
>>>
>>> No documented. Seems to be several of them.
>>
>> Correct. At some point there may be drivers. But there is no
>> authoritative information (memory map, list of registers, list of
>> interrupts) outside of source code for this board.
> 
> Then you should just omit it.

OK. Will omit the unsupported nodes. Sending v18.

> 
> Rob
> 


-- 
Damien Le Moal
Western Digital Research



More information about the linux-riscv mailing list