On Thu, 2 Feb 2023 19:41:33 +0000, Conor Dooley wrote:
> On Fri, Feb 03, 2023 at 02:56:41AM +0800, Hal Feng wrote:
>> On Wed, 1 Feb 2023 08:21:05 +0000, Conor Dooley wrote:
>> > 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?
>>
>> First of all, it's my fault that I didn't check the revision of U74-MC
>> manual, so most of my previous replies might not make sense.
>
> No that's fine. The manual stuff confused me too when I went looking
> initially, and I still get get mixed up by the fact that there are
> core-complex manuals but not core manuals.
>
>> If we add a new compatible string for S7, should we change the compatibles
>> of hart1~3 to "sifive,u74" also? And then, there may be no point keeping some
>> compatible strings of core complex like "sifive,u74-mc" and "sifive,u54-mc".
>> I'm not sure about this.
>
> [...]
>
>> >> Yes, "RV64IMAC" is correct. The monitor core in U74-MC is a
>> >> S7-series core, not S76.
>> >
>> > Cool, thanks.
>>
>> Now I think it might be another version of S76.
>
> The SiFive docs describe the u74-mc core complex, which AFAIU you have,
> as being 1x S7 & 4x U7.
>
> I'd be happy with new binding for "sifive,s7" & then we use that here.
> If you're sure it's S76, we can also use that. S76 is described, in what
> docs I can see, as a core complex containing an S7, so S7 seems likely
> to be correct?
I will add a new binding for "sifive,s7" and modify the code as follows.
S7_0: cpu@0 {
compatible = "sifive,s7", "riscv";
...
riscv,isa = "rv64imac_zicsr_zba_zbb";
...
};
>
> u7, u74 & u74-mc are valid compatibles, added by SiFive, in commit
> 75e6d7248efc ("dt-bindings: riscv: Update DT binding docs to support
> SiFive FU740 SoC"). Unfortunately, they never actually *used* those
> compatibles for anything, and just used "sifive,bullet0" for the fu740.
>
> I'll accept any of u7, u74 or u74-mc for those harts.
>
>> >> >> + 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.
>>
>> I found the 21G1.02.00 revision on StarFive internal net, but I'm not sure
>> whether I can make it public and I am checking this.
>
> Yeah, certainly don't do anything without cross-checking!
>
>> This revision records
>> that the ISA of 21G1.02.00 U74 is "RV64GCB" and ISA of 21G1.02.00 S7 is
>> "RV64IMACB". I am asking someone to check with SiFive whether both 21G1.02.00
>> U74 and S7 support the full B extension.
>
> Having cross-checked a 21G1.01.00 document against a 21G2.01.00 one, I'm
> 99% sure that you have _Zba_Zbb.
> The G2.01 document says _Zba_Zbb & has the same instructions listed as
> supported as the G1.02 one.
> I've also tried the Zbb support patches posted by Heiko [2] on a
> VisionFive V2 and had them work - which is why a definitive statement on
> the version of Zbb supported would be really great to have!
>
> [2] - https://lore.kernel.org/linux-riscv/20230113212301.3534711-1-heiko@xxxxxxxxx/
The 21G1.02.00 document is still not allowed to be public so far. By
comparing with instructions included in b extensions [1], I can confirm
that the 21G1.02.00 only supports Zba and Zbb.
[1] https://github.com/riscv/riscv-bitmanip/blob/main/bitmanip/overview.adoc#
Zicsr is also supported as described in 21G1.02.00 document. So I will
modify as follows.
U74_1: cpu@1 {
compatible = "sifive,u74-mc", "riscv";
...
riscv,isa = "rv64imafdc_zicsr_zba_zbb";
...
};
Best regards,
Hal
>
>> >> 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.
>>
>> Will keep it in mind. Thank you.
>
> FWIW, the deadline for getting material in for v6.3 has already passed,
> so you can send the next version of this series without waiting for
> clarification on the compatibles & ISA string. We should have plenty of
> time to get those fixed up before the series gets applied.
>
> Thanks,
> Conor.
>
>> > 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
