Re: [PATCH v1 28/30] RISC-V: Add StarFive JH7110 VisionFive2 board device tree

[Hal Feng <hal.feng@xxxxxxxxxxxxxxxxxxxxxx>]

On Sat, 1 Oct 2022 12:14:49 +0100, Conor Dooley wrote:
> On Fri, Sep 30, 2022 at 03:53:53PM +0800, Hal Feng wrote:
> > From: Emil Renner Berthing <kernel@xxxxxxxx>
> > 
> > Add a minimal device tree for StarFive JH7110 VisionFive2 board.
> > Support booting and basic clock/reset/pinctrl/uart drivers.
> >
> 
> I would like to see a link to the publicly available datasheet or
> documentation for the board (and for the SoC in patch 28) please.

All documents can be found at RVspace Documentation Center maintained
by StarFive. The related documents of JH7110 SoC and VisionFive2 board
are as follows.

StarFive JH7110 SoC:
https://doc-en.rvspace.org/Doc_Center/jh7110.html
StarFive VisionFive2 board:
https://doc-en.rvspace.org/Doc_Center/visionfive_2.html

> 
> > Signed-off-by: Emil Renner Berthing <kernel@xxxxxxxx>
> > Signed-off-by: Jianlong Huang <jianlong.huang@xxxxxxxxxxxxxxxx>
> 
> Ditto from patch 28 re: the SoB chain.
> 
> > Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> 
> > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > new file mode 100644
> > index 000000000000..6b9fe32c7eac
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > @@ -0,0 +1,91 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/*
> > + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> > + * Copyright (C) 2022 Emil Renner Berthing <kernel@xxxxxxxx>
> > + */
> > +
> > +/dts-v1/;
> > +#include "jh7110.dtsi"
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/pinctrl/pinctrl-starfive-jh7110.h>
> > +
> > +/ {
> > +	model = "StarFive VisionFive V2";
> > +	compatible = "starfive,visionfive-v2", "starfive,jh7110";
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +	};
> 
> Should we also have a chosen node here?

Will add it. Thanks.

> 
> > +
> > +	cpus {
> > +		timebase-frequency = <4000000>;
> > +	};
> > +
> > +	memory@40000000 {
> > +		device_type = "memory";
> > +		reg = <0x0 0x40000000 0x1 0x0>;
> 
> What is going to happen to the 2 GB variant if they attempt to use this
> devicetree?

The VisionFive2 board now has 4GB version and 8GB version only. Before
linux startup, we will change this property in dtb through u-boot to
make sure the board can boot up with the correct memory size.

> 
> > +	};
> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		linux,cma {
> > +			compatible = "shared-dma-pool";
> > +			reusable;
> > +			size = <0x0 0x20000000>;
> > +			alignment = <0x0 0x1000>;
> > +			alloc-ranges = <0x0 0xa0000000 0x0 0x20000000>;
> > +			linux,cma-default;
> > +		};
> > +
> > +		e24_mem: e24@c0000000 {
> 
> I had a conversation previously with Icenowy [0] about the e24 on the
> jh7100 that didn't really come to a conclusion about how to represent
> it there - but looks like you've decided that it should be a remoteproc
> for the jh7100?

Yes, we treat it as a remoteproc outside the cpus node. But after
communication with my colleagues, I found that all nodes in
"reserved-memory" are not used in the minimal support for VisionFive2
board. So for this series, I would like to remove "reserved-memory"
in v2.

> 
> Is this another situation where peripherals appear at different
> addresses for the e24 compared to the u74s? Or has that changed for the> jh7100, and really the e24 should be described in the CPUs node? If it
> is the latter, you can pick the first patch from [0] into your series.
> 
> 0 - https://lore.kernel.org/linux-riscv/e8543838cd221ab6699da16c985eed7514daa786.camel@xxxxxxxxxx> > +			reg = <0x0 0xc0110000 0x0 0xf0000>;
> > +			no-map;
> > +		};
> > +
> > +		xrp_reserved: xrpbuffer@f0000000 {
> 
> "Following the generic-names recommended practice, node names should
> reflect the purpose of the node (ie. “framebuffer” or “dma-pool”)."
> 
> I tried googling around for an explanation for what the xrp was, and all
> I could find was this out-of-tree text binding:
> https://github.com/foss-xtensa/xrp/blob/master/xrp-kernel/cdns%2Cxrp-hw-simple%2Cv1.txt

The name is from the device driver of HiFi4 DSP provided by Cadence,
which is not in the mainline. "xrp" is a short name of
"Xtensa Remote Processing".

Best regards,
Hal