On 2021/02/09 4:50, Rob Herring wrote:
> On Fri, Feb 5, 2021 at 4:52 PM Sean Anderson <seanga2@xxxxxxxxx> 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@xxxxxxxxxx>
>>>> Cc: devicetree@xxxxxxxxxxxxxxx
>>>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxx>
>>>> + * Copyright (C) 2019-20 Sean Anderson <seanga2@xxxxxxxxx>
>>>> * 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@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@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@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@50440000 {
>>>> - compatible = "kendryte,k210-sysctl", "simple-mfd";
>>>> - reg = <0x50440000 0x1000>;
>>>> - #clock-cells = <1>;
>>>> + debug0: debug@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@1000 {
>>>> + reg = <0x1000 0x1000>;
>>>> + read-only;
>>>> + status = "disabled";
>>>> };
>>>>
>>>> clint0: clint@2000000 {
>>>
>>> interrupt-controller@...
>>
>> 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@c000000 {
>>>> + plic0: interrupt-controller@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@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@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@1c000 {
>>>> + reg = <0x1C000 0x1000>;
>>>> + };
>>>> +
>>>> + /*
>>>> + * Device tree containing only registers,
>>>> + * interrupts, and cpus
>>>> + */
>>>> + fdt@1d000 {
>>>> + reg = <0x1D000 0x2000>;
>>>> + };
>>>> +
>>>> + /* CPU/ROM credits */
>>>> + credits@1f000 {
>>>> + reg = <0x1F000 0x1000>;
>>>> + };
>>>> + };
>>>> +
>>>> + dvp0: camera@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
