Re: [PATCH v2 09/11] riscv: dts: add initial SOPHGO SG2042 SoC device tree

[Chen Wang]

Regards,

unicornx

Conor Dooley <conor.dooley@xxxxxxxxxxxxx> 于2023年9月20日周三 16:58写道：
>
> Yo,
>
> On Wed, Sep 20, 2023 at 02:40:32PM +0800, Chen Wang wrote:
> > Milk-V Pioneer motherboard is powered by SOPHON's SG2042.
> >
> > SG2042 is server grade chip with high performance, low power
> > consumption and high data throughput.
> > Key features:
> > - 64 RISC-V cpu cores which implements IMAFDC
>
> That's not quite true though, is it?

The cpu cores of SG2042 is c902 from T-HEAD, it supports vector, but
it's v0.7, not official v1.0. If we declare it as supporting
v-extension, the latest kernel(since 6.5) will issue rvv1.0
instructions during booting and make c902 crash. So we have to remove
"v" from the capability of ISA to pass the booting of machine. You can
check the "riscv,isa = "rv64imafdc";" in DTS.

>
> > - 4 cores per cluster, 16 clusters on chip
> > - ......
>
> What's a "....."? ;)
I just cited a description from TRM of SG2042 and it looks too long. I
will give a better description here and avoid using "......" in the
next revision.

>
> >
> > More info is available at [1].
> >
> > [1]: https://en.sophgo.com/product/introduce/sg2042.html
>
> Link: please.
>
> > Currently only support booting into console with only uart,
> > other features will be added soon later.
> >
> > Acked-by: Xiaoguang Xing <xiaoguang.xing@xxxxxxxxxx>
> > Signed-off-by: Xiaoguang Xing <xiaoguang.xing@xxxxxxxxxx>
> > Signed-off-by: Inochi Amaoto <inochiama@xxxxxxxxxxx>
> > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@xxxxxxxxxxxxx>
> > Signed-off-by: Chen Wang <wangchen20@xxxxxxxxxxx>
>
> There are 4 sign-offs here. Surely some of these should be
> co-developed-bys?
>
> > +             cpu0: cpu@0 {
> > +                     compatible = "thead,c920", "riscv";
> > +                     device_type = "cpu";
> > +                     riscv,isa = "rv64imafdc";
>
> Please also add riscv,isa-base & riscv,isa-extensions.
>
> > +                     reg = <0>;
> > +                     i-cache-block-size = <64>;
> > +                     i-cache-size = <65536>;
> > +                     i-cache-sets = <512>;
> > +                     d-cache-block-size = <64>;
> > +                     d-cache-size = <65536>;
> > +                     d-cache-sets = <512>;
> > +                     next-level-cache = <&l2_cache0>;
> > +                     mmu-type = "riscv,sv39";
> > +
> > +                     cpu0_intc: interrupt-controller {
> > +                             compatible = "riscv,cpu-intc";
> > +                             interrupt-controller;
> > +                             #interrupt-cells = <1>;
> > +                     };
> > +             };
>
> > diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> > new file mode 100644
> > index 000000000000..747fd9764c95
> > --- /dev/null
> > +++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> > @@ -0,0 +1,439 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
>
> You should add () around the GPL-2.0 OR MIT.
>
> > +/*
> > + * Copyright (C) 2022 Sophgo Technology Inc. All rights reserved.
> > + */
> > +
> > +/dts-v1/;
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +#include "sg2042-cpus.dtsi"
> > +
> > +#define SOC_PERIPHERAL_IRQ(nr) (nr)
>
> Why? What does this do? Where was it copied from?
>
It should be unnecessary， I will remove it in the next revision.

> > +
> > +/ {
> > +     compatible = "sophgo,sg2042";
> > +     #address-cells = <2>;
> > +     #size-cells = <2>;
> > +     dma-noncoherent;
> > +
> > +     aliases {
> > +             serial0 = &uart0;
> > +     };
> > +
> > +     /* the mem node will be updated by ZSBL. */
>
> huh? So these are random numbers below? Either put something useful here
> or delete it please.

The memory for SG2042 is not fixed, the bootloader will detect memory
installed on board during booting and fill the actual address and size
in the memory node. The comment " /* the mem node will be updated by
ZSBL. */" is telling this.
We write memory nodes like this to make them act just as placeholders
and the value is by default written as zero.


>
> > +     memory@0 {
> > +             device_type = "memory";
> > +             reg = <0x00000000 0x00000000 0x00000000 0x00000000>;
> > +     };
> > +
> > +     memory@1 {
> > +             device_type = "memory";
> > +             reg = <0x00000000 0x00000001 0x00000000 0x00000000>;
> > +     };
> > +
> > +     memory@2 {
> > +             device_type = "memory";
> > +             reg = <0x00000000 0x00000002 0x00000000 0x00000000>;
> > +     };
> > +
> > +     memory@3 {
> > +             device_type = "memory";
> > +             reg = <0x00000000 0x00000003 0x00000000 0x00000000>;
> > +     };
>
> > +     soc: soc {
> > +             compatible = "simple-bus";
> > +             #address-cells = <2>;
> > +             #size-cells = <2>;
> > +             ranges;
> > +
> > +             clint_mswi: interrupt-controller@7094000000 {
> > +                     compatible = "sophgo,sg2042-clint-mswi", "thead,c900-clint-mswi";
> > +                     reg = <0x00000070 0x94000000 0x00000000 0x00004000>;
>
> & nak to this without further explanation!
>
> Thanks,
> Conor.
>