Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Sean Anderson <seanga2@gmail.com>,
	"linux-riscv@lists.infradead.org"
	<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: Sat, 15 Feb 2020 02:34:25 +0000
Message-ID: <BYAPR04MB581671F46D3FE67FD3C8B2B7E7140@BYAPR04MB5816.namprd04.prod.outlook.com> (raw)
In-Reply-To: <48e10b3d-12f3-a65c-8017-99c780c63040@gmail.com>

On 2020/02/15 5:51, Sean Anderson wrote:
> 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 :)

Seeing what you did for uboot, I used a lot of it and naturally gave credit
where it is due :)

> 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

OK. Will check again.

>> + * 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.

Yes, I suspected that. Seeing that the CPU frequency can be changed, I
wondered how this should all go together. But since the current code does
not change the cpu frequency, I simply stayed with the default here. I
suspect that we may want the default hard-coded in the code, and use the
value specified here as the one that should be setup by sysctl.

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

If we want to run the MMU, yes. For a nommu kernel, I would rather stick
with "none". Not that it really matters since the nommu kernel will not
look at this entry anyway. No strong opinion either way in the end.
I have not checked the specs yet, but does sv36 necessarily implies older
specs 1.9 too ? If not, then we may want something else in there for this
soc special case.

>> +			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.

Good to know. I will remove the comment then.

> 
>> +			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.

Yes... The clock management needs more work as mentioned in the cover
letter. All of this works for now with direct m-mode boot (no boot loader)
and relies on the hardware defaults which are coded here. The sysctl driver
also relies on those defaults. A more solid implementation will need the
soc_early_init() code to discover and set things up correctly.

As mentioned in the cover letter, this is all a base. It works, but
definitely is not complete.

>> +			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.

Yes, I guess it can be removed.

>> +		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.

Absolutely. It is far from complete. And seeing your complete device tree,
there are likely a lot of peripherals for which Linux already has drivers
and that could be used if the clocks/sysctl are improved. As mentioned in
the cover letter, this is left as an exercise for interested people :)
Note that I am indeed interested in working on this a little more. I simply
lack the time to do it :)

>> +
>> +		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.

Yep. As you said, it works because we use the defaults for everything.

>> +		};
>> +
>> +		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.

I wondered about it too. Not really sure what to do about it.

>> +			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.

OK. It makes sense.

> 
>> +			reg = <0x38000000 0x20>;
> 
> Same thing as the size comments above.
> 
>> +			interrupts = <33>;
>> +			clocks = <&sysctl 0>;
> 
> Same clock comments.
> 
>> +		};
>> +	};
>> +};
>>
> 
> --Sean
> 


-- 
Damien Le Moal
Western Digital Research


  reply index

Thread overview: 89+ 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-03-02  3:48   ` Anup Patel
2020-03-04 18:38   ` Palmer Dabbelt
2020-02-12 10:34 ` [PATCH 03/10] riscv: Unaligned load/store handling for M_MODE Damien Le Moal
2020-03-02  3:57   ` Anup Patel
2020-03-04 19:28   ` Palmer Dabbelt
2020-02-12 10:34 ` [PATCH 04/10] riscv: Add BUILTIN_DTB support Damien Le Moal
2020-03-02  3:58   ` Anup Patel
2020-03-04 19:28   ` Palmer Dabbelt
2020-03-05  4:58     ` Anup Patel
2020-03-05  5:14       ` Damien Le Moal
2020-03-05  5:37         ` Anup Patel
2020-03-05  6:13           ` Damien Le Moal
2020-03-08  6:10             ` Anup Patel
2020-03-05  8:18         ` Atish Patra
2020-03-07  0:02           ` Sean Anderson
2020-03-07  1:51             ` Atish Patra
2020-03-07  2:08               ` Sean Anderson
2020-03-06 23:56         ` Sean Anderson
2020-02-12 10:34 ` [PATCH 05/10] riscv: Add SOC early init support Damien Le Moal
2020-03-04 19:28   ` Palmer Dabbelt
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-03-04 19:38   ` Palmer Dabbelt
2020-02-12 10:34 ` [PATCH 07/10] riscv: Select required drivers for Kendryte SOC Damien Le Moal
2020-03-02  3:59   ` Anup Patel
2020-03-04 19:44   ` Palmer Dabbelt
2020-02-12 10:34 ` [PATCH 08/10] riscv: Add Kendryte K210 device tree Damien Le Moal
2020-02-14 20:51   ` Sean Anderson
2020-02-15  2:34     ` Damien Le Moal [this message]
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-04-01 17:55                         ` Drew Fustini
2020-04-02  2:24                           ` Damien Le Moal
2020-02-19  8:50                   ` Wladimir J. van der Laan
2020-02-27 19:43       ` Sean Anderson
2020-03-02  4:06   ` Anup Patel
2020-03-02  4:15     ` Damien Le Moal
2020-03-02  4:22       ` Anup Patel
2020-03-02  4:51         ` Damien Le Moal
2020-03-02  5:05           ` Anup Patel
2020-03-02  5:08             ` Damien Le Moal
2020-03-07  0:18               ` Sean Anderson
2020-03-07  4:11                 ` Anup Patel
2020-03-04 19:44   ` Palmer Dabbelt
2020-02-12 10:34 ` [PATCH 09/10] riscv: Kendryte K210 default config Damien Le Moal
2020-03-02  4:07   ` Anup Patel
2020-03-04 19:44   ` Palmer Dabbelt
2020-02-12 10:34 ` [PATCH 10/10] riscv: create a loader.bin for the kendryte kflash.py tool Damien Le Moal
2020-03-02  4:08   ` Anup Patel
2020-03-04 19:44   ` Palmer Dabbelt
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
2020-02-28 20:32 ` Sean Anderson
2020-03-02  3:01   ` Damien Le Moal
2020-03-02  3:53     ` Sean Anderson
2020-03-02  4:11       ` Damien Le Moal
2020-03-02  4:18         ` Sean Anderson
2020-03-02  4:54           ` Damien Le Moal
2020-03-02  4:56             ` Sean Anderson
2020-03-02  5:03               ` Damien Le Moal
2020-03-02  4:17       ` Anup Patel
2020-03-02  4:21         ` Sean Anderson
2020-03-02  4:48         ` Damien Le Moal
2020-03-02  4:51           ` Damien Le Moal
2020-03-02  5:02           ` Sean Anderson
2020-03-02  5:11             ` Damien Le Moal
2020-03-02  5:25               ` Sean Anderson
2020-03-02  5:43                 ` Damien Le Moal
2020-03-04 19:44 ` Palmer Dabbelt

Reply instructions:

You may reply publicly 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=BYAPR04MB581671F46D3FE67FD3C8B2B7E7140@BYAPR04MB5816.namprd04.prod.outlook.com \
    --to=damien.lemoal@wdc.com \
    --cc=Anup.Patel@wdc.com \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=seanga2@gmail.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