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 03:00:18 +0000
Message-ID: <BYAPR04MB5816ED294439828E562EB085E7140@BYAPR04MB5816.namprd04.prod.outlook.com> (raw)
In-Reply-To: <bd74c841-2447-2f11-f924-a501230b3927@gmail.com>

On 2020/02/15 11:48, Sean Anderson wrote:
>>>> +		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.

OK. But I guess we can keep the "none" here until we get to work on the MMU
support. That definitely sounds safer to me considering the specs difference.

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

Good idea. I had a look at this when I used your device tree but decided on
not using it for simplicity since using the default HW setup led to that
single clock 0. But this is a good point. I will use the identifiers and
for now have all the IDs used defined to "0". As the sysctl driver is
changed and improved, the DT can remain the same and more devices added easily.

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

That would be great !

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

OK. Will make changes and retest.

> 
>>>> +			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: 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
2020-02-15  2:34     ` Damien Le Moal
2020-02-15  2:48       ` Sean Anderson
2020-02-15  3:00         ` Damien Le Moal [this message]
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=BYAPR04MB5816ED294439828E562EB085E7140@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