Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
From: Sean Anderson <seanga2@gmail.com>
To: Damien Le Moal <damien.lemoal@wdc.com>,
	linux-riscv@lists.infradead.org,
	Palmer Dabbelt <palmer@dabbelt.com>
Cc: Anup Patel <Anup.Patel@wdc.com>,
	Paul Walmsley <paul.walmsley@sifive.com>
Subject: Re: [PATCH 08/10] riscv: Add Kendryte K210 device tree
Date: Fri, 14 Feb 2020 15:51:31 -0500
Message-ID: <48e10b3d-12f3-a65c-8017-99c780c63040@gmail.com> (raw)
In-Reply-To: <20200212103432.660256-9-damien.lemoal@wdc.com>

On 2/12/20 5:34 AM, Damien Le Moal wrote:
> Add a generic device tree for Kendryte K210 SoC based boards. This (for
> now) very simple device tree works for the Kendryte KD233 development
> board, the Sipeed MAIX M1 Dock based boards and the Sipeed MAIXDUINO
> board.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  arch/riscv/boot/dts/Makefile           |   1 +
>  arch/riscv/boot/dts/kendryte/Makefile  |   2 +
>  arch/riscv/boot/dts/kendryte/k210.dts  |  23 +++++
>  arch/riscv/boot/dts/kendryte/k210.dtsi | 123 +++++++++++++++++++++++++
>  4 files changed, 149 insertions(+)
>  create mode 100644 arch/riscv/boot/dts/kendryte/Makefile
>  create mode 100644 arch/riscv/boot/dts/kendryte/k210.dts
>  create mode 100644 arch/riscv/boot/dts/kendryte/k210.dtsi
> 
> diff --git a/arch/riscv/boot/dts/Makefile b/arch/riscv/boot/dts/Makefile
> index 0bf2669aa12d..87815557f2db 100644
> --- a/arch/riscv/boot/dts/Makefile
> +++ b/arch/riscv/boot/dts/Makefile
> @@ -3,4 +3,5 @@ ifneq ($(CONFIG_BUILTIN_DTB_SOURCE),"")
>  obj-$(CONFIG_USE_BUILTIN_DTB) += $(patsubst "%",%,$(CONFIG_BUILTIN_DTB_SOURCE)).dtb.o
>  else
>  subdir-y += sifive
> +subdir-y += kendryte
>  endif
> diff --git a/arch/riscv/boot/dts/kendryte/Makefile b/arch/riscv/boot/dts/kendryte/Makefile
> new file mode 100644
> index 000000000000..815444e69e89
> --- /dev/null
> +++ b/arch/riscv/boot/dts/kendryte/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_SOC_KENDRYTE) += k210.dtb
> diff --git a/arch/riscv/boot/dts/kendryte/k210.dts b/arch/riscv/boot/dts/kendryte/k210.dts
> new file mode 100644
> index 000000000000..0d1f28fce6b2
> --- /dev/null
> +++ b/arch/riscv/boot/dts/kendryte/k210.dts
> @@ -0,0 +1,23 @@
> +// 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/kendryte/k210.dtsi b/arch/riscv/boot/dts/kendryte/k210.dtsi
> new file mode 100644
> index 000000000000..4b9eeabb07f7
> --- /dev/null
> +++ b/arch/riscv/boot/dts/kendryte/k210.dtsi
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Sean Anderson <seanga2@gmail.com>

Glad to see this is getting some use :)

This appears to be an old-ish version, and I've made some updates in the
past month or so. My current work is availible from [1].

[1] https://github.com/Forty-Bot/u-boot/blob/maix_v6/arch/riscv/dts/k210.dtsi

> + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> + */
> +
> +/ {
> +	/*
> +	 * Although the K210 is a 64-bit CPU, the address bus is only 32-bits
> +	 * wide, and the upper half of all addresses is ignored.
> +	 */
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	compatible = "kendryte,k210";
> +
> +	aliases {
> +		serial0 = &uarths0;
> +	};
> +
> +	clocks {
> +		in0: oscillator {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <26000000>;
> +		};
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		timebase-frequency = <7800000>;

This is true only for the default frequency. I wonder if there is a
better way to encode this.

> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			reg = <0>;
> +			compatible = "riscv";
> +			riscv,isa = "rv64imafdc";
> +			mmu-type = "none";

This should be "sv36".

> +			i-cache-size = <0x8000>;
> +			i-cache-block-size = <64>; /* bogus */

I emailed some people at Kendryte and they confirmed the 64-byte
cacheline. The cpus are rocket cores.

> +			d-cache-size = <0x8000>;
> +			d-cache-block-size = <64>; /* bogus */
> +			clocks = <&sysctl 0>;

This is correct only by coincidence. The clock structure is

in0 -> pll0 -> aclk -> cpu

aclk divides by two by default, so it runs at 390 MHz, which is also
what you set pll1 to. However, if someone else (such as the bootloader)
changes the pll0 frequency then this will be completely off.

> +			clock-frequency = <390000000>;
> +			cpu0_intc: interrupt-controller {
> +				#interrupt-cells = <1>;
> +				interrupt-controller;
> +				compatible = "riscv,cpu-intc";
> +			};
> +		};
> +		cpu1: cpu@1 {
> +			device_type = "cpu";
> +			reg = <1>;
> +			compatible = "riscv";
> +			riscv,isa = "rv64imafdc";
> +			mmu-type = "none";
> +			i-cache-size = <0x8000>;
> +			i-cache-block-size = <64>; /* bogus */
> +			d-cache-size = <0x8000>;
> +			d-cache-block-size = <64>; /* bogus */
> +			clocks = <&sysctl 0>;
> +			clock-frequency = <390000000>;
> +			cpu1_intc: interrupt-controller {
> +				#interrupt-cells = <1>;
> +				interrupt-controller;
> +				compatible = "riscv,cpu-intc";
> +			};
> +		};
> +	};
> +
> +	sram0: memory@80000000 {
> +		device_type = "memory";
> +		reg = <0x80000000 0x400000>;
> +	};
> +
> +	sram1: memory@80400000 {
> +		device_type = "memory";
> +		reg = <0x80400000 0x200000>;
> +	};
> +
> +	kpu_sram: memory@80600000 {
> +		device_type = "memory";
> +		reg = <0x80600000 0x200000>;
> +	};
> +
> +	soc {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "kendryte,k210-soc", "simple-bus";

Should the -soc suffix be here? I saw it was absent from the fu540
device tree.

> +		ranges;
> +		interrupt-parent = <&plic0>;
> +
> +		sysctl: sysctl@50440000 {
> +			compatible = "kendryte,k210-sysctl", "syscon";
> +			reg = <0x50440000 0x1000>;
> +			#clock-cells = <1>;
> +		};

Would it be possible to model this as an MFD? There are a lot of
different registers in here, many of which are unrelated to clocks. For
example, there are also reset registers, a reboot register, and DMA
handshake controls. I think modeling this as a clock controller only
does not correctly reflect the hardware, and will be awkward in the
future.

> +
> +		clint0: interrupt-controller@2000000 {
> +			compatible = "riscv,clint0";
> +			reg = <0x2000000 0xC000>;
> +			interrupts-extended = <&cpu0_intc 3>,  <&cpu1_intc 3>;
> +			clocks = <&sysctl 0>;

Again, this is wrong; it should be running off ACLK.

> +		};
> +
> +		plic0: interrupt-controller@c000000 {
> +			#interrupt-cells = <1>;
> +			interrupt-controller;
> +			compatible = "kendryte,k210-plic0", "riscv,plic0";
> +			reg = <0xC000000 0x3FFF008>;

With regard to the size of registers, I had the following exchange on
the U-Boot mailing list.

On Tue, Feb 4, 2020 at 10:23 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 2/4/20 6:32 AM, Bin Meng wrote:
>> Hi Sean,
>>
>> On Mon, Feb 3, 2020 at 4:10 AM Sean Anderson <seanga2@gmail.com> wrote:
>>> Should the size of a reg be the size of the documented registers, or the size
>>> of the address space which will be routed to that device?
>>
>> Perhaps we need use the size of the address space routed to that
>> device, in case there is some undocumented registers we need handle.
>
> Ok, I'll go with the whole address space then.

You may want to make similar changes for Linux; I didn't see any
documentation about what the preferred size was.

> +			interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 0xffffffff>,
> +					      <&cpu1_intc 11>, <&cpu1_intc 0xffffffff>;
> +			riscv,ndev = <65>;
> +			riscv,max-priority = <0x07>;
> +		};
> +
> +		uarths0: serial@38000000 {
> +			compatible = "kendryte,k210-uart0", "sifive,uart0";

I would change the first compatible string to "kendryte,k210-uarths",
since that is how this uart is described in their documentation.

> +			reg = <0x38000000 0x20>;

Same thing as the size comments above.

> +			interrupts = <33>;
> +			clocks = <&sysctl 0>;

Same clock comments.

> +		};
> +	};
> +};
> 

--Sean


  reply index

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-12 10:34 [PATCH 00/10] Kendryte k210 SoC boards support Damien Le Moal
2020-02-12 10:34 ` [PATCH 01/10] riscv: Fix gitignore Damien Le Moal
2020-02-20  0:15   ` Palmer Dabbelt
2020-02-12 10:34 ` [PATCH 02/10] riscv: Force flat memory model with no-mmu Damien Le Moal
2020-02-14 20:18   ` Sean Anderson
2020-02-15  2:15     ` Damien Le Moal
2020-02-15  2:26       ` Sean Anderson
2020-02-15  2:40         ` Damien Le Moal
2020-02-12 10:34 ` [PATCH 03/10] riscv: Unaligned load/store handling for M_MODE Damien Le Moal
2020-02-12 10:34 ` [PATCH 04/10] riscv: Add BUILTIN_DTB support Damien Le Moal
2020-02-12 10:34 ` [PATCH 05/10] riscv: Add SOC early init support Damien Le Moal
2020-02-12 10:34 ` [PATCH 06/10] riscv: Add Kendryte K210 SoC support Damien Le Moal
2020-02-14 20:31   ` Sean Anderson
2020-02-12 10:34 ` [PATCH 07/10] riscv: Select required drivers for Kendryte SOC Damien Le Moal
2020-02-12 10:34 ` [PATCH 08/10] riscv: Add Kendryte K210 device tree Damien Le Moal
2020-02-14 20:51   ` Sean Anderson [this message]
2020-02-15  2:34     ` Damien Le Moal
2020-02-15  2:48       ` Sean Anderson
2020-02-15  3:00         ` Damien Le Moal
2020-02-18 14:12           ` Carlos Eduardo de Paula
2020-02-18 14:18             ` Sean Anderson
2020-02-18 14:30               ` Carlos Eduardo de Paula
2020-02-18 17:48                 ` Sean Anderson
2020-02-18 19:26                   ` Carlos Eduardo de Paula
2020-02-19  9:06                     ` Wladimir J. van der Laan
2020-02-19 22:28                       ` Sean Anderson
2020-02-20 10:48                         ` Wladimir J. van der Laan
2020-02-22 19:07                       ` Wladimir J. van der Laan
2020-02-19  8:50                   ` Wladimir J. van der Laan
2020-02-12 10:34 ` [PATCH 09/10] riscv: Kendryte K210 default config Damien Le Moal
2020-02-12 10:34 ` [PATCH 10/10] riscv: create a loader.bin for the kendryte kflash.py tool Damien Le Moal
2020-02-14 15:05 ` [PATCH 00/10] Kendryte k210 SoC boards support Carlos Eduardo de Paula
2020-02-15  2:02   ` Damien Le Moal
2020-02-17 13:28     ` Carlos Eduardo de Paula
2020-02-26 21:31       ` Carlos Eduardo de Paula
2020-02-27  2:18         ` Damien Le Moal

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=48e10b3d-12f3-a65c-8017-99c780c63040@gmail.com \
    --to=seanga2@gmail.com \
    --cc=Anup.Patel@wdc.com \
    --cc=damien.lemoal@wdc.com \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org
	public-inbox-index linux-riscv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git