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"
	<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 21:48:05 -0500
Message-ID: <bd74c841-2447-2f11-f924-a501230b3927@gmail.com> (raw)
In-Reply-To: <BYAPR04MB581671F46D3FE67FD3C8B2B7E7140@BYAPR04MB5816.namprd04.prod.outlook.com>

On 2/14/20 9:34 PM, Damien Le Moal wrote:
> 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.

Ah, this should be "sv39", sorry. Ideally we would put something like
the priv spec version in the isa string, or perhaps as a separate
property. From reading the dt docs, it seems like one should try to
describe the hardware as best as possible to allow for
foward-compatibility.

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

At the very least, I would different identifiers for each clock. That
way you can ignore them now and add support later. There isn't a
"natural" ordering (since the clocks are in a different order in every
register), so I am using this arbitrary numbering scheme [1].

[1] https://github.com/Forty-Bot/u-boot/blob/maix_v6/include/dt-bindings/clock/k210-sysctl.h

>>> +			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 :)

My next project after u-boot support was going to be Linux, so I can
lend a hand after I get everything merged on that end.

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

The sizes in my device tree are based on reading device memory and
seeing where it repeats. For example, the memory at 50210000 and
50210100 is the same, so I set the uart1 reg to <50210000 0x100>.

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



  reply index

Thread overview: 34+ 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
2020-02-15  2:34     ` Damien Le Moal
2020-02-15  2:48       ` Sean Anderson [this message]
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

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=bd74c841-2447-2f11-f924-a501230b3927@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