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?
> >> + 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?
> >> +
> >> + /* 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.
Rob
