On Fri, Oct 13, 2023 at 10:08:24AM +0100, Conor Dooley wrote:
> Yo,
>
> On Mon, Oct 09, 2023 at 07:26:35PM +0800, Inochi Amaoto wrote:
> > Move the cpu and the common peripherals of CV181x and CV180x to new file.
> >
...
> > };
> > };
>
> What I wanted to comment on was this though - it seems that both the
> cv1800b and the cv1812h have identical plic and clint nodes, other than
> their compatibles? If that is the case, why create a cv1800b and a
> cv1812h specific file containing entirely new nodes, when overriding the
> compatible would be sufficient? Doubly so if the other SoCs in the
> cv18xx series are going to have identical layouts.
>
> I gave it a quick test locally with the below diff applied on top of
> this series - although I didn't make sure that I didn't re-order the
> plic & clint nodes, I just wanted to demonstrate what I had done.
>
> Cheers,
> Conor.
>
> -- 8< --
>
> diff --git a/arch/riscv/boot/dts/sophgo/cv1800b-milkv-duo.dts b/arch/riscv/boot/dts/sophgo/cv1800b-milkv-duo.dts
> index 3af9e34b3bc7..a9d809a49e7a 100644
> --- a/arch/riscv/boot/dts/sophgo/cv1800b-milkv-duo.dts
> +++ b/arch/riscv/boot/dts/sophgo/cv1800b-milkv-duo.dts
> @@ -5,7 +5,7 @@
>
> /dts-v1/;
>
> -#include "cv1800b.dtsi"
> +#include "cv180x.dtsi"
>
> / {
> model = "Milk-V Duo";
> diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> index 0904154f9829..e69de29bb2d1 100644
> --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> @@ -1,30 +0,0 @@
> -// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> -/*
> - * Copyright (C) 2023 Jisheng Zhang <jszhang@xxxxxxxxxx>
> - */
> -
> -#include "cv180x.dtsi"
> -
> -/ {
> - compatible = "sophgo,cv1800b";
> -
> - soc {
> - interrupt-parent = <&plic>;
> -
> - plic: interrupt-controller@70000000 {
> - compatible = "sophgo,cv1800b-plic", "thead,c900-plic";
> - reg = <0x70000000 0x4000000>;
> - interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>;
> - interrupt-controller;
> - #address-cells = <0>;
> - #interrupt-cells = <2>;
> - riscv,ndev = <101>;
> - };
> -
> - clint: timer@74000000 {
> - compatible = "sophgo,cv1800b-clint", "thead,c900-clint";
> - reg = <0x74000000 0x10000>;
> - interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>;
> - };
> - };
> -};
> diff --git a/arch/riscv/boot/dts/sophgo/cv180x.dtsi b/arch/riscv/boot/dts/sophgo/cv180x.dtsi
> index 64ffb23d3626..1a2c44ba4de9 100644
> --- a/arch/riscv/boot/dts/sophgo/cv180x.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/cv180x.dtsi
> @@ -48,6 +48,7 @@ osc: oscillator {
>
> soc {
> compatible = "simple-bus";
> + interrupt-parent = <&plic>;
> #address-cells = <1>;
> #size-cells = <1>;
> dma-noncoherent;
> @@ -174,5 +175,21 @@ uart4: serial@41c0000 {
> reg-io-width = <4>;
> status = "disabled";
> };
> +
> + plic: interrupt-controller@70000000 {
> + compatible = "sophgo,cv1800b-plic", "thead,c900-plic";
> + reg = <0x70000000 0x4000000>;
> + interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>;
> + interrupt-controller;
> + #address-cells = <0>;
> + #interrupt-cells = <2>;
> + riscv,ndev = <101>;
> + };
> +
> + clint: timer@74000000 {
> + compatible = "sophgo,cv1800b-clint", "thead,c900-clint";
> + reg = <0x74000000 0x10000>;
> + interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>;
> + };
> };
> };
> diff --git a/arch/riscv/boot/dts/sophgo/cv1812h.dtsi b/arch/riscv/boot/dts/sophgo/cv1812h.dtsi
> index 3864d34b0100..c0a8d3290cc8 100644
> --- a/arch/riscv/boot/dts/sophgo/cv1812h.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/cv1812h.dtsi
> @@ -15,22 +15,13 @@ memory@80000000 {
> };
>
> soc {
> - interrupt-parent = <&plic>;
>
> plic: interrupt-controller@70000000 {
> compatible = "sophgo,cv1812h-plic", "thead,c900-plic";
Hi Conor,
Maybe this has been discussed before but I didn't find it. I'm wondering
the reason of adding each plic and clint binding for each SoC, can we
just use the thead,c900-plic for plic?
FWICT, arm gic dt usage follows this way, there's no binding for each SoC's
gic but directly use "arm,gic-v3" and so on.
Thanks in advance
> - reg = <0x70000000 0x4000000>;
> - interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>;
> - interrupt-controller;
> - #address-cells = <0>;
> - #interrupt-cells = <2>;
> - riscv,ndev = <101>;
> };
>
> clint: timer@74000000 {
> compatible = "sophgo,cv1812h-clint", "thead,c900-clint";
> - reg = <0x74000000 0x10000>;
> - interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>;
