Re: [PATCH v3 6/7] riscv: dts: starfive: Add initial StarFive JH7110 device tree

[Conor Dooley]

On Wed, Feb 01, 2023 at 03:21:48PM +0800, Hal Feng wrote:
> On Wed, 28 Dec 2022 22:48:43 +0000, Conor Dooley wrote:
> > On Tue, Dec 20, 2022 at 09:12:46AM +0800, Hal Feng wrote:

> >> +/ {
> >> +	compatible = "starfive,jh7110";
> >> +	#address-cells = <2>;
> >> +	#size-cells = <2>;
> >> +
> >> +	cpus {
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +
> >> +		S76_0: cpu@0 {
> >> +			compatible = "sifive,u74-mc", "riscv";
> > 
> > The label here says S76 but the compatible says u74-mc.
> 
> U74-MC has 5 cores including 1 * S7 core and 4 * U74 cores.
> 
> > Which is correct? Your docs say S7 and S76, so I would imagine that it
> > is actually an S76?
> 
> I found SiFive website [1] call it S76, but call it S7 in other places.
> So I misunderstood this. Considering the ISA difference you described
> as below, I think it's proper to change the label to "S7_0".

I'm less worried about the label & more interested in the compatible.
hart0 is, as you say, not a u74. Should we not be adding a "sifive,s7"
compatible string to Documentation/devicetree/bindings/riscv/cpus.yaml
and using that here instead?

> 
> [1] https://www.sifive.com/cores/essential
> 
> > 
> >> +			reg = <0>;
> >> +			d-cache-block-size = <64>;
> >> +			d-cache-sets = <64>;
> >> +			d-cache-size = <8192>;
> >> +			d-tlb-sets = <1>;
> >> +			d-tlb-size = <40>;
> >> +			device_type = "cpu";
> >> +			i-cache-block-size = <64>;
> >> +			i-cache-sets = <64>;
> >> +			i-cache-size = <16384>;
> >> +			i-tlb-sets = <1>;
> >> +			i-tlb-size = <40>;
> >> +			mmu-type = "riscv,sv39";
> >> +			next-level-cache = <&ccache>;
> >> +			riscv,isa = "rv64imac";
> > 
> > While I was poking around trying to see if there was some logic behind
> > that compatible, I noticed that SiFive's docs for the S76 say it is
> > RV64GBC *but* the docs for the u74-mc say "4xRV64GBC and 1xRV64IMAC".
> > I assume that rv64imac is the correct one here?
> 
> Yes, "RV64IMAC" is correct. The monitor core in U74-MC is a
> S7-series core, not S76.

Cool, thanks.

> >> +			tlb-split;
> >> +			status = "disabled";
> >> +
> >> +			cpu0_intc: interrupt-controller {
> >> +				compatible = "riscv,cpu-intc";
> >> +				interrupt-controller;
> >> +				#interrupt-cells = <1>;
> >> +			};
> >> +		};
> >> +
> >> +		U74_1: cpu@1 {
> >> +			compatible = "sifive,u74-mc", "riscv";
> >> +			reg = <1>;
> >> +			d-cache-block-size = <64>;
> >> +			d-cache-sets = <64>;
> >> +			d-cache-size = <32768>;
> >> +			d-tlb-sets = <1>;
> >> +			d-tlb-size = <40>;
> >> +			device_type = "cpu";
> >> +			i-cache-block-size = <64>;
> >> +			i-cache-sets = <64>;
> >> +			i-cache-size = <32768>;
> >> +			i-tlb-sets = <1>;
> >> +			i-tlb-size = <40>;
> >> +			mmu-type = "riscv,sv39";
> >> +			next-level-cache = <&ccache>;
> >> +			riscv,isa = "rv64imafdc";
> > 
> > That also begs the question:
> > Do your u74s support RV64GBC, as the (current) SiFive documentation
> > suggests?
> 
> Actually, U74 doesn't support the full B extension, and the SiFive doc [1]

Yeah, I knew asking that question that the "RV64GBC" in SiFive's online
documentation was using outdated terminology. Also, that is not the doc
for your core complex as far as I can tell. That is the document for
impid 0x0621_1222, whereas (IIRC) your core is 0x0421_0427.
Jess and I had a look one evening but could not find the 21G1.02.00
revision of this document, which is the one corresponding to 0x421_0427.
See Table 92 for more details.

> describes the ISA of U74 is "RV64GC_Zba_Zbb_Sscofpmf" which "G" includes
> "IMAFD".

I could not find the 21G1.02.00 version of this document online, but I
was able to find the 21G1.01.00 version of it & that version does not
support the Sscofpmf extension (but does have Zba/Zbb support).

> "_Zba_Zbb_Sscofpmf" is not shown in other device trees such as
> jh7100.dtsi and fu740-c000.dtsi, so I didn't show them here.

Just because other devicetrees omit them, doesn't mean that you should
too!
This compatible should be an accurate description of your hardware, so
you should add what you actually have.
If you have Zba and Zbb, then add them.
I would double check against your internal documentation for 21G2.02.00
as to whether you do have Sscofpmf, and if you do, add that too!

That way, whenever support for those extensions lands, the jh7110 will
automatically pick it up, rather than needing to have them retrofitted.

> [1] https://sifive.cdn.prismic.io/sifive/2dd11994-693c-4360-8aea-5453d8642c42_u74mc_core_complex_manual_21G3.pdf

Thanks,
Conor.

Attachment: signature.asc
Description: PGP signature