Message-ID: <20240703142213.GA2734247@ofsar>
Date: Wed, 3 Jul 2024 14:22:13 +0000
From: Yixun Lan <dlan@...too.org>
To: Emil Renner Berthing <emil.renner.berthing@...onical.com>
Cc: Conor Dooley <conor@...nel.org>, Inochi Amaoto <inochiama@...look.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Samuel Holland <samuel.holland@...ive.com>,
	Anup Patel <anup@...infault.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, Lubomir Rintel <lkundrak@...sk>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org, linux-serial@...r.kernel.org,
	Meng Zhang <zhangmeng.kevin@...cemit.com>,
	Yangyu Chen <cyy@...self.name>
Subject: Re: [PATCH v2 08/10] riscv: dts: add initial SpacemiT K1 SoC device
 tree

On 04:22 Wed 03 Jul     , Emil Renner Berthing wrote:
> Yixun Lan wrote:
> > Hi Conor:
> >
> > On 16:25 Tue 02 Jul     , Conor Dooley wrote:
> > > On Tue, Jul 02, 2024 at 09:35:45AM +0800, Inochi Amaoto wrote:
> > > > On Tue, Jul 02, 2024 at 01:28:47AM GMT, Yixun Lan wrote:
> > > > > On 12:49 Mon 01 Jul     , Emil Renner Berthing wrote:
> > > > > > Yixun Lan wrote:
> > > > > > > From: Yangyu Chen <cyy@...self.name>
> > > > > > >
> > > > > > > Banana Pi BPI-F3 motherboard is powered by SpacemiT K1[1].
> > > > > > >
> > > > > > > Key features:
> > > > > > > - 4 cores per cluster, 2 clusters on chip
> > > > > > > - UART IP is Intel XScale UART
> > > > > > >
> > > > > > > Some key considerations:
> > > > > > > - ISA string is inferred from vendor documentation[2]
> > > > > > > - Cluster topology is inferred from datasheet[1] and L2 in vendor dts[3]
> > > > > > > - No coherent DMA on this board
> > > > > > >     Inferred by taking vendor ethernet and MMC drivers to the mainline
> > > > > > >     kernel. Without dma-noncoherent in soc node, the driver fails.
> > > > > > > - No cache nodes now
> > > > > > >     The parameters from vendor dts are likely to be wrong. It has 512
> > > > > > >     sets for a 32KiB L1 Cache. In this case, each set is 64B in size.
> > > > > > >     When the size of the cache line is 64B, it is a directly mapped
> > > > > > >     cache rather than a set-associative cache, the latter is commonly
> > > > > > >     used. Thus, I didn't use the parameters from vendor dts.
> > > > > > >
> > > > > > > Currently only support booting into console with only uart, other
> > > > > > > features will be added soon later.
> > > > > > >
> > > > > ...
> > > > >
> > > > > > > +		clint: timer@...00000 {
> > > > > > > +			compatible = "spacemit,k1-clint", "sifive,clint0";
> > > > > > > +			reg = <0x0 0xe4000000 0x0 0x10000>;
> > > > > > > +			interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>,
> > > > > > > +					      <&cpu1_intc 3>, <&cpu1_intc 7>,
> > > > > > > +					      <&cpu2_intc 3>, <&cpu2_intc 7>,
> > > > > > > +					      <&cpu3_intc 3>, <&cpu3_intc 7>,
> > > > > > > +					      <&cpu4_intc 3>, <&cpu4_intc 7>,
> > > > > > > +					      <&cpu5_intc 3>, <&cpu5_intc 7>,
> > > > > > > +					      <&cpu6_intc 3>, <&cpu6_intc 7>,
> > > > > > > +					      <&cpu7_intc 3>, <&cpu7_intc 7>;
> > > > > > > +		};
> > > > > > > +
> > > > > > > +		uart0: serial@...17000 {
> > > > > > > +			compatible = "spacemit,k1-uart", "intel,xscale-uart";
> > > > > > > +			reg = <0x0 0xd4017000 0x0 0x100>;
> > > > > > > +			interrupts = <42>;
> > > > > > > +			clock-frequency = <14857000>;
> > > > > > > +			reg-shift = <2>;
> > > > > > > +			reg-io-width = <4>;
> > > > > > > +			status = "disabled";
> > > > > > > +		};
> > > > > > > +
> > > > > > > +		/* note: uart1 skipped */
> > > > > >
> > > > > > The datasheet page you link to above says "-UART (×10)", but here you're
> > > > > > skipping one of them. Why? I can see the vendor tree does the same, but it
> > > > > > would be nice with an explanation of what's going on.
> > > > > >
> > > > > /* note: uart1 in 0xf0612000, reserved for TEE usage */
> > > > > I would put something like this, does this sound ok to you?
> > > > >
> > > > > more detail, iomem range from 0xf000,0000 - 0xf080,0000 are dedicated for TEE purpose,
> > > > > It won't be exposed to Linux once TEE feature is enabled..
> > > > >
> > > > > skipping uart1 may make people confused but we are trying to follow datasheet..
> > > >
> > > > Instead of skipping it, I suggest adding this to reserved-memory area,
> > > > which make all node visible and avoid uart1 being touched by mistake.
> > >
> > > No, don't make it reserved-memory - instead add it as
> > > status = "reserved"; /* explanation for why */
> > Ok, got
> >
> > > Also, I'd appreciate if the nodes were sorted by unit address in the
> > > dtsi.
> > so I would move "plic, clint" after node of uart9 as this suggestion
> >
> > for uart1, its unit-address is 0xf0610000, it should be moved to after clint
> > (once unit-address sorted), if we follow this rule strictly.
> > but it occur to me this is not very intuitive, if no objection, I would put
> > it between uart0 and uart2 (thus slightly break the rule..)
> 
> No, please order nodes by their address as Conor said. It actually says so in
> the DTS coding style:
> 
> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
> 
I was thinking about grouping all same type devices (uart here) together
according to "1. Order of Nodes", but after reconsideration, I'd just follow
yours and Conor's suggestion, thus it will be more straightforward, also match
more well with datasheet[1] if we have to add more "reserved" nodes in the future.

Link: https://developer.spacemit.com/#/documentation?token=LzJyw97BCipK1dkUygrcbT0NnMg [1]

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Powered by blists - more mailing lists