| Message ID | 20230507182304.2934-4-jszhang@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | Add Sipeed Lichee Pi 4A RISC-V board support | expand |
Hey Jisheng, On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > + c910_0: cpu@0 { > + compatible = "thead,c910", "riscv"; > + device_type = "cpu"; > + riscv,isa = "rv64imafdc"; Does this support more than "rv64imafdc"? I assume there's some _xtheadfoo extensions that it does support, although I am not sure how we are proceeding with those - Heiko might have a more nuanced take. > + reset: reset-sample { > + compatible = "thead,reset-sample"; What is a "reset-sample"? > + entry-reg = <0xff 0xff019050>; > + entry-cnt = <4>; > + control-reg = <0xff 0xff015004>; > + control-val = <0x1c>; > + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>; > + }; > + > + plic: interrupt-controller@ffd8000000 { > + compatible = "thead,c910-plic"; > + reg = <0xff 0xd8000000 0x0 0x01000000>; > + interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>, > + <&cpu1_intc 11>, <&cpu1_intc 9>, > + <&cpu2_intc 11>, <&cpu2_intc 9>, > + <&cpu3_intc 11>, <&cpu3_intc 9>; > + interrupt-controller; > + #interrupt-cells = <1>; > + riscv,ndev = <240>; > + }; > + > + clint: timer@ffdc000000 { > + compatible = "thead,c900-clint"; "c900"? That a typo or intentional. Hard to tell since this compatible is undocumented ;) > + reg = <0xff 0xdc000000 0x0 0x00010000>; > + interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>, > + <&cpu1_intc 3>, <&cpu1_intc 7>, > + <&cpu2_intc 3>, <&cpu2_intc 7>, > + <&cpu3_intc 3>, <&cpu3_intc 7>; > + }; > + > + uart0: serial@ffe7014000 { > + compatible = "snps,dw-apb-uart"; > + reg = <0xff 0xe7014000 0x0 0x4000>; > + interrupts = <36>; > + clocks = <&uart_sclk>; > + clock-names = "baudclk"; dtbs_check complains about this clock name. > + > + dmac0: dmac@ffefc00000 { dma-controller@ As I mentioned in the other patch, please clean up the dtbs_check complaints for v2. Cheers, Conor.
在 2023-05-07星期日的 22:35 +0100，Conor Dooley写道： > Hey Jisheng, > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > + c910_0: cpu@0 { > > + compatible = "thead,c910", "riscv"; > > + device_type = "cpu"; > > + riscv,isa = "rv64imafdc"; > > Does this support more than "rv64imafdc"? > I assume there's some _xtheadfoo extensions that it does support, > although I am not sure how we are proceeding with those - Heiko might > have a more nuanced take. > > > + reset: reset-sample { > > + compatible = "thead,reset-sample"; > > What is a "reset-sample"? > > > + entry-reg = <0xff 0xff019050>; > > + entry-cnt = <4>; > > + control-reg = <0xff 0xff015004>; > > + control-val = <0x1c>; > > + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 > > 0x7c5 0x7cc>; > > + }; > > + > > + plic: interrupt-controller@ffd8000000 { > > + compatible = "thead,c910-plic"; > > + reg = <0xff 0xd8000000 0x0 0x01000000>; > > + interrupts-extended = <&cpu0_intc 11>, > > <&cpu0_intc 9>, > > + <&cpu1_intc 11>, > > <&cpu1_intc 9>, > > + <&cpu2_intc 11>, > > <&cpu2_intc 9>, > > + <&cpu3_intc 11>, > > <&cpu3_intc 9>; > > + interrupt-controller; > > + #interrupt-cells = <1>; > > + riscv,ndev = <240>; > > + }; > > + > > + clint: timer@ffdc000000 { > > + compatible = "thead,c900-clint"; > > "c900"? That a typo or intentional. Hard to tell since this > compatible > is undocumented ;) Intentional, for supporting both C906 and C910. However, as we discussed in some binding patches, there should be a DT binding string per chip. So here should be "thead,light-clint", "thead,c900-clint". (Or use th1520, the marketing name, instead of light, the codename) P.S. which one is preferred by DT binding maintainers, the marketing name or the codename? > > > + reg = <0xff 0xdc000000 0x0 0x00010000>; > > + interrupts-extended = <&cpu0_intc 3>, > > <&cpu0_intc 7>, > > + <&cpu1_intc 3>, > > <&cpu1_intc 7>, > > + <&cpu2_intc 3>, > > <&cpu2_intc 7>, > > + <&cpu3_intc 3>, > > <&cpu3_intc 7>; > > + }; > > + > > + uart0: serial@ffe7014000 { > > + compatible = "snps,dw-apb-uart"; > > + reg = <0xff 0xe7014000 0x0 0x4000>; > > + interrupts = <36>; > > + clocks = <&uart_sclk>; > > + clock-names = "baudclk"; > > dtbs_check complains about this clock name. > > + > > + dmac0: dmac@ffefc00000 { > > dma-controller@ > > As I mentioned in the other patch, please clean up the dtbs_check > complaints for v2. > > Cheers, > Conor. > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, May 08, 2023 at 11:32:17AM +0800, Icenowy Zheng wrote: > P.S. which one is preferred by DT binding maintainers, the marketing > name or the codename? I don't see the benefit of using internal product codenames (even if it did appear on their gitee) over the name by which it'll appear in board datasheets etc etc. Cheers, Conor.
Am Montag, 8. Mai 2023, 05:32:17 CEST schrieb Icenowy Zheng: > 在 2023-05-07星期日的 22:35 +0100，Conor Dooley写道： > > Hey Jisheng, > > > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > > > + c910_0: cpu@0 { > > > + compatible = "thead,c910", "riscv"; > > > + device_type = "cpu"; > > > + riscv,isa = "rv64imafdc"; > > > > Does this support more than "rv64imafdc"? > > I assume there's some _xtheadfoo extensions that it does support, > > although I am not sure how we are proceeding with those - Heiko might > > have a more nuanced take. I guess the interesting question still is, are these part of the isa string or more of an errata? The binding currently says Identifies the specific RISC-V instruction set architecture supported by the hart. These are documented in the RISC-V User-Level ISA document, available from https://riscv.org/specifications/ I guess if we decide to make them part of the isa-string the binding then should get a paragraph mention _xfoo vendor-extensions too. Personally, making these part of the ISA string definitly sounds like the best solution though :-) . > > > + reset: reset-sample { > > > + compatible = "thead,reset-sample"; > > > > What is a "reset-sample"? > > > > > + entry-reg = <0xff 0xff019050>; > > > + entry-cnt = <4>; > > > + control-reg = <0xff 0xff015004>; > > > + control-val = <0x1c>; > > > + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 > > > 0x7c5 0x7cc>; > > > + }; > > > + > > > + plic: interrupt-controller@ffd8000000 { > > > + compatible = "thead,c910-plic"; > > > + reg = <0xff 0xd8000000 0x0 0x01000000>; > > > + interrupts-extended = <&cpu0_intc 11>, > > > <&cpu0_intc 9>, > > > + <&cpu1_intc 11>, > > > <&cpu1_intc 9>, > > > + <&cpu2_intc 11>, > > > <&cpu2_intc 9>, > > > + <&cpu3_intc 11>, > > > <&cpu3_intc 9>; > > > + interrupt-controller; > > > + #interrupt-cells = <1>; > > > + riscv,ndev = <240>; > > > + }; > > > + > > > + clint: timer@ffdc000000 { > > > + compatible = "thead,c900-clint"; > > > > "c900"? That a typo or intentional. Hard to tell since this > > compatible > > is undocumented ;) > > Intentional, for supporting both C906 and C910. > > However, as we discussed in some binding patches, there should be a DT > binding string per chip. > > So here should be "thead,light-clint", "thead,c900-clint". > > (Or use th1520, the marketing name, instead of light, the codename) I'm definitly confused now :-) c900 as well as something like c9xx should not be part of dt-bindings. Binding-names should always denote _actual_ component names. So you can do "thead,c906-clint" and for example "thead,c910-clint", "thead,c906-clint" to describe that the clint in the c910 is compatible with the one in c906 I don't think there should be a "thead,light-clint" ... the clint is part of the cpu core itself so the soc itself shouldn't introduce any changes? Heiko
On Mon, May 08, 2023 at 10:23:02AM +0200, Heiko Stübner wrote: > Am Montag, 8. Mai 2023, 05:32:17 CEST schrieb Icenowy Zheng: > > 在 2023-05-07星期日的 22:35 +0100，Conor Dooley写道： > > > Hey Jisheng, > > > > > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > > > > > + c910_0: cpu@0 { > > > > + compatible = "thead,c910", "riscv"; > > > > + device_type = "cpu"; > > > > + riscv,isa = "rv64imafdc"; > > > > > > Does this support more than "rv64imafdc"? > > > I assume there's some _xtheadfoo extensions that it does support, > > > although I am not sure how we are proceeding with those - Heiko might > > > have a more nuanced take. > > I guess the interesting question still is, are these part of the isa > string or more of an errata? Yeah, I dunno. That's possible a policy decision more than anything else. I don't remember if it was one of your patchsets or elsewhere, but I do recall a split between xtheadba etc and vector, where xtheadba was defined as a vendor extension, whereas vector is not. Their extension spec repo <https://github.com/T-head-Semi/thead-extension-spec> appears to be aligned with that view, apart from the CMOs that we have already called an erratum. > The binding currently says > Identifies the specific RISC-V instruction set architecture > supported by the hart. These are documented in the RISC-V > User-Level ISA document, available from > https://riscv.org/specifications/ > > > I guess if we decide to make them part of the isa-string the binding > then should get a paragraph mention _xfoo vendor-extensions too. I have an idea in the works that may allow dealing with this kind of thing, but it's a bit of a departure from the existing binding. I will hopefully post an early RFC of it later today. That said, the binding does currently allow you to put in _xfoo vendor extensions as-is. > Personally, making these part of the ISA string definitly sounds like > the best solution though :-) . You would say that wouldn't you! In general, I'd rather we filled in as much information as possible here, even if it is not currently in use, to avoid having to retrofit as support becomes available.
On 07/05/2023 20:23, Jisheng Zhang wrote: > Add initial device tree for the light(a.k.a TH1520) RISC-V SoC by > T-HEAD. > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > arch/riscv/boot/dts/thead/light.dtsi | 454 +++++++++++++++++++++++++++ > 1 file changed, 454 insertions(+) > create mode 100644 arch/riscv/boot/dts/thead/light.dtsi > > diff --git a/arch/riscv/boot/dts/thead/light.dtsi b/arch/riscv/boot/dts/thead/light.dtsi > new file mode 100644 > index 000000000000..cdf6d8b04d22 > --- /dev/null > +++ b/arch/riscv/boot/dts/thead/light.dtsi > @@ -0,0 +1,454 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 Alibaba Group Holding Limited. > + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org> > + */ > + > +/ { > + compatible = "thead,light"; Undocumented compatible. Please run scripts/checkpatch.pl and fix reported warnings. > + #address-cells = <2>; > + #size-cells = <2>; > + > + cpus: cpus { > + #address-cells = <1>; > + #size-cells = <0>; > + timebase-frequency = <3000000>; > + > + c910_0: cpu@0 { > + compatible = "thead,c910", "riscv"; Probably the same. (...) > + soc { > + compatible = "simple-bus"; > + interrupt-parent = <&plic>; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + reset: reset-sample { Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "thead,reset-sample"; Undocumented compatible. Please run scripts/checkpatch.pl and fix reported warnings. > + entry-reg = <0xff 0xff019050>; > + entry-cnt = <4>; > + control-reg = <0xff 0xff015004>; > + control-val = <0x1c>; > + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>; > + }; > + > + plic: interrupt-controller@ffd8000000 { > + compatible = "thead,c910-plic"; > + reg = <0xff 0xd8000000 0x0 0x01000000>; > + interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>, > + <&cpu1_intc 11>, <&cpu1_intc 9>, > + <&cpu2_intc 11>, <&cpu2_intc 9>, > + <&cpu3_intc 11>, <&cpu3_intc 9>; > + interrupt-controller; > + #interrupt-cells = <1>; > + riscv,ndev = <240>; > + }; > + > + clint: timer@ffdc000000 { > + compatible = "thead,c900-clint"; Undocumented compatible. Please run scripts/checkpatch.pl and fix reported warnings. > + reg = <0xff 0xdc000000 0x0 0x00010000>; > + interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>, > + <&cpu1_intc 3>, <&cpu1_intc 7>, > + <&cpu2_intc 3>, <&cpu2_intc 7>, > + <&cpu3_intc 3>, <&cpu3_intc 7>; > + }; > + > + uart0: serial@ffe7014000 { > + compatible = "snps,dw-apb-uart"; > + reg = <0xff 0xe7014000 0x0 0x4000>; > + interrupts = <36>; > + clocks = <&uart_sclk>; > + clock-names = "baudclk"; > + reg-shift = <2>; > + reg-io-width = <4>; > + status = "disabled"; > + }; > + > + uart1: serial@ffe7f00000 { > + compatible = "snps,dw-apb-uart"; > + reg = <0xff 0xe7f00000 0x0 0x4000>; > + interrupts = <37>; > + clocks = <&uart_sclk>; > + clock-names = "baudclk"; > + reg-shift = <2>; > + reg-io-width = <4>; > + status = "disabled"; > + }; > + > + uart3: serial@ffe7f04000 { > + compatible = "snps,dw-apb-uart"; > + reg = <0xff 0xe7f04000 0x0 0x4000>; > + interrupts = <39>; > + clocks = <&uart_sclk>; > + clock-names = "baudclk"; > + reg-shift = <2>; > + reg-io-width = <4>; > + status = "disabled"; > + }; > + > + gpio2: gpio@ffe7f34000 { > + compatible = "snps,dw-apb-gpio"; > + reg = <0xff 0xe7f34000 0x0 0x1000>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + portc: gpio-controller@0 { > + compatible = "snps,dw-apb-gpio-port"; > + gpio-controller; > + #gpio-cells = <2>; > + ngpios = <32>; > + reg = <0>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <58>; > + }; > + }; > + > + gpio3: gpio@ffe7f38000 { > + compatible = "snps,dw-apb-gpio"; > + reg = <0xff 0xe7f38000 0x0 0x1000>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + portd: gpio-controller@0 { > + compatible = "snps,dw-apb-gpio-port"; > + gpio-controller; > + #gpio-cells = <2>; > + ngpios = <32>; > + reg = <0>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <59>; > + }; > + }; > + > + gpio0: gpio@ffec005000 { > + compatible = "snps,dw-apb-gpio"; > + reg = <0xff 0xec005000 0x0 0x1000>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + porta: gpio-controller@0 { > + compatible = "snps,dw-apb-gpio-port"; > + gpio-controller; > + #gpio-cells = <2>; > + ngpios = <32>; > + reg = <0>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <56>; > + }; > + }; > + > + gpio1: gpio@ffec006000 { > + compatible = "snps,dw-apb-gpio"; > + reg = <0xff 0xec006000 0x0 0x1000>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + portb: gpio-controller@0 { > + compatible = "snps,dw-apb-gpio-port"; > + gpio-controller; > + #gpio-cells = <2>; > + ngpios = <32>; > + reg = <0>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <57>; > + }; > + }; > + > + uart2: serial@ffec010000 { > + compatible = "snps,dw-apb-uart"; > + reg = <0xff 0xec010000 0x0 0x4000>; > + interrupts = <38>; > + clocks = <&uart_sclk>; > + clock-names = "baudclk"; > + reg-shift = <2>; > + reg-io-width = <4>; > + status = "disabled"; > + }; > + > + dmac0: dmac@ffefc00000 { Does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Best regards, Krzysztof
Am Montag, 8. Mai 2023, 10:35:38 CEST schrieb Conor Dooley: > On Mon, May 08, 2023 at 10:23:02AM +0200, Heiko Stübner wrote: > > Am Montag, 8. Mai 2023, 05:32:17 CEST schrieb Icenowy Zheng: > > > 在 2023-05-07星期日的 22:35 +0100，Conor Dooley写道： > > > > Hey Jisheng, > > > > > > > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > > > > > > > + c910_0: cpu@0 { > > > > > + compatible = "thead,c910", "riscv"; > > > > > + device_type = "cpu"; > > > > > + riscv,isa = "rv64imafdc"; > > > > > > > > Does this support more than "rv64imafdc"? > > > > I assume there's some _xtheadfoo extensions that it does support, > > > > although I am not sure how we are proceeding with those - Heiko might > > > > have a more nuanced take. > > > > I guess the interesting question still is, are these part of the isa > > string or more of an errata? > > Yeah, I dunno. That's possible a policy decision more than anything > else. I don't remember if it was one of your patchsets or elsewhere, but > I do recall a split between xtheadba etc and vector, where xtheadba was > defined as a vendor extension, whereas vector is not. Their extension > spec repo <https://github.com/T-head-Semi/thead-extension-spec> appears > to be aligned with that view, apart from the CMOs that we have already > called an erratum. I think the CMO stuff came a bit before that repo actually existed ;-) . I guess another argument for riscv,isa would be that we don't have to trust MVENDORID, and especially values in MARCHID and MIMPID. Somehow part of me doesn't have enough trust that these values will always be suitably different when they are baked into the hardware ;-) . I guess vector is somewhat special, with it implementing version 0.7.1 it's not a t-head invention but also not the real RISCV "v" . So I _guess_ the jury might still be out on how to handle that everywhere. > > The binding currently says > > Identifies the specific RISC-V instruction set architecture > > supported by the hart. These are documented in the RISC-V > > User-Level ISA document, available from > > https://riscv.org/specifications/ > > > > > > I guess if we decide to make them part of the isa-string the binding > > then should get a paragraph mention _xfoo vendor-extensions too. > > I have an idea in the works that may allow dealing with this kind of > thing, but it's a bit of a departure from the existing binding. > I will hopefully post an early RFC of it later today. > That said, the binding does currently allow you to put in _xfoo vendor > extensions as-is. > > > Personally, making these part of the ISA string definitly sounds like > > the best solution though :-) . > > You would say that wouldn't you! In general, I'd rather we filled in as > much information as possible here, even if it is not currently in use, > to avoid having to retrofit as support becomes available. yep definitively. Especially as switching to expecting _xfoo later on then causes of course compatiblity issues. The fun part will be though to get vendors, toolchains and friends to agree on the naming. Heiko
On Sun, May 07, 2023 at 10:35:12PM +0100, Conor Dooley wrote: > Hey Jisheng, > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > + c910_0: cpu@0 { > > + compatible = "thead,c910", "riscv"; > > + device_type = "cpu"; > > + riscv,isa = "rv64imafdc"; > > Does this support more than "rv64imafdc"? > I assume there's some _xtheadfoo extensions that it does support, > although I am not sure how we are proceeding with those - Heiko might > have a more nuanced take. > > > + reset: reset-sample { > > + compatible = "thead,reset-sample"; > > What is a "reset-sample"? This node is only for opensbi. The compatible string is already in opensbi. Do we also need to add dt-binding for it in linux? > > > + entry-reg = <0xff 0xff019050>; > > + entry-cnt = <4>; > > + control-reg = <0xff 0xff015004>; > > + control-val = <0x1c>; > > + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>; > > + }; > > + > > + plic: interrupt-controller@ffd8000000 { > > + compatible = "thead,c910-plic"; > > + reg = <0xff 0xd8000000 0x0 0x01000000>; > > + interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>, > > + <&cpu1_intc 11>, <&cpu1_intc 9>, > > + <&cpu2_intc 11>, <&cpu2_intc 9>, > > + <&cpu3_intc 11>, <&cpu3_intc 9>; > > + interrupt-controller; > > + #interrupt-cells = <1>; > > + riscv,ndev = <240>; > > + }; > > + > > + clint: timer@ffdc000000 { > > + compatible = "thead,c900-clint"; > > "c900"? That a typo or intentional. Hard to tell since this compatible > is undocumented ;) Per my understanding, this node is only for opensbi too. Add will add dt-binding in v2. > > > + reg = <0xff 0xdc000000 0x0 0x00010000>; > > + interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>, > > + <&cpu1_intc 3>, <&cpu1_intc 7>, > > + <&cpu2_intc 3>, <&cpu2_intc 7>, > > + <&cpu3_intc 3>, <&cpu3_intc 7>; > > + }; > > + > > + uart0: serial@ffe7014000 { > > + compatible = "snps,dw-apb-uart"; > > + reg = <0xff 0xe7014000 0x0 0x4000>; > > + interrupts = <36>; > > + clocks = <&uart_sclk>; > > + clock-names = "baudclk"; > > dtbs_check complains about this clock name. > > + > > + dmac0: dmac@ffefc00000 { > > dma-controller@ > > As I mentioned in the other patch, please clean up the dtbs_check > complaints for v2. > Thanks for the reminding.
On Tue, May 09, 2023 at 12:26:10AM +0800, Jisheng Zhang wrote: > On Sun, May 07, 2023 at 10:35:12PM +0100, Conor Dooley wrote: > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > > > + c910_0: cpu@0 { > > > + compatible = "thead,c910", "riscv"; > > > + device_type = "cpu"; > > > + riscv,isa = "rv64imafdc"; > > > > Does this support more than "rv64imafdc"? > > I assume there's some _xtheadfoo extensions that it does support, > > although I am not sure how we are proceeding with those - Heiko might > > have a more nuanced take. > > > > > + reset: reset-sample { > > > + compatible = "thead,reset-sample"; > > > > What is a "reset-sample"? > > This node is only for opensbi. The compatible string is already in > opensbi. Do we also need to add dt-binding for it in linux? If it's to be included in the kernel's dts, then yes, you do need a dt-binding. If you remove it, then you don't :) That said, "thead,reset-sample" is a strangely named compatible, so if you do keep it it may end up needing a rename! Cheers, Conor.
Am Montag, 8. Mai 2023, 18:44:04 CEST schrieb Conor Dooley: > On Tue, May 09, 2023 at 12:26:10AM +0800, Jisheng Zhang wrote: > > On Sun, May 07, 2023 at 10:35:12PM +0100, Conor Dooley wrote: > > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > > > > > + c910_0: cpu@0 { > > > > + compatible = "thead,c910", "riscv"; > > > > + device_type = "cpu"; > > > > + riscv,isa = "rv64imafdc"; > > > > > > Does this support more than "rv64imafdc"? > > > I assume there's some _xtheadfoo extensions that it does support, > > > although I am not sure how we are proceeding with those - Heiko might > > > have a more nuanced take. > > > > > > > + reset: reset-sample { > > > > + compatible = "thead,reset-sample"; > > > > > > What is a "reset-sample"? > > > > This node is only for opensbi. The compatible string is already in > > opensbi. Do we also need to add dt-binding for it in linux? > > If it's to be included in the kernel's dts, then yes, you do need a > dt-binding. If you remove it, then you don't :) > > That said, "thead,reset-sample" is a strangely named compatible, so if > you do keep it it may end up needing a rename! and you'll need to justify that this describes actual hardware (dt-maintainers iterate all the time that dt is a hardware description, not a configuration scheme). The question also would be if this is part of upstream opensbi at all. In general though, openSBI does something similar with their perf-counter description. Describing the mapping and eventids usable in which counter via a structure passed from u-boot to openSBI. The difference here is, that openSBI then removes the relevant nodes from the dt, so that the kernel never sees them [0] . As you reset-sample seems to fall into a similar category, I guess it would be better suited in an a foo-u-boot.dtsi ? [0] https://github.com/riscv-software-src/opensbi/blob/master/lib/utils/fdt/fdt_pmu.c#L42
On Tue, May 9, 2023 at 12:44 AM Conor Dooley <conor@kernel.org> wrote: > > On Tue, May 09, 2023 at 12:26:10AM +0800, Jisheng Zhang wrote: > > On Sun, May 07, 2023 at 10:35:12PM +0100, Conor Dooley wrote: > > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > > > > > + c910_0: cpu@0 { > > > > + compatible = "thead,c910", "riscv"; > > > > + device_type = "cpu"; > > > > + riscv,isa = "rv64imafdc"; > > > > > > Does this support more than "rv64imafdc"? > > > I assume there's some _xtheadfoo extensions that it does support, > > > although I am not sure how we are proceeding with those - Heiko might > > > have a more nuanced take. > > > > > > > + reset: reset-sample { > > > > + compatible = "thead,reset-sample"; > > > > > > What is a "reset-sample"? > > > > This node is only for opensbi. The compatible string is already in > > opensbi. Do we also need to add dt-binding for it in linux? > > If it's to be included in the kernel's dts, then yes, you do need a > dt-binding. If you remove it, then you don't :) > > That said, "thead,reset-sample" is a strangely named compatible, so if > you do keep it it may end up needing a rename! How about compatible = "thead,reset-th1520" ? > > Cheers, > Conor. >
On Sun, May 21, 2023 at 11:37:58PM +0800, Guo Ren wrote: > On Tue, May 9, 2023 at 12:44 AM Conor Dooley <conor@kernel.org> wrote: > > > > On Tue, May 09, 2023 at 12:26:10AM +0800, Jisheng Zhang wrote: > > > On Sun, May 07, 2023 at 10:35:12PM +0100, Conor Dooley wrote: > > > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > > > > > > > + c910_0: cpu@0 { > > > > > + compatible = "thead,c910", "riscv"; > > > > > + device_type = "cpu"; > > > > > + riscv,isa = "rv64imafdc"; > > > > > > > > Does this support more than "rv64imafdc"? > > > > I assume there's some _xtheadfoo extensions that it does support, > > > > although I am not sure how we are proceeding with those - Heiko might > > > > have a more nuanced take. > > > > > > > > > + reset: reset-sample { > > > > > + compatible = "thead,reset-sample"; > > > > > > > > What is a "reset-sample"? > > > > > > This node is only for opensbi. The compatible string is already in > > > opensbi. Do we also need to add dt-binding for it in linux? > > > > If it's to be included in the kernel's dts, then yes, you do need a > > dt-binding. If you remove it, then you don't :) > > > > That said, "thead,reset-sample" is a strangely named compatible, so if > > you do keep it it may end up needing a rename! > How about compatible = "thead,reset-th1520" ? "vendor,soc-function" is more typical, but "reset" is usually used for reset controllers of which this isn't as far as I can tell. I commented on the v2, hoping that you might actually know what the IP block' full/proper name is: https://lore.kernel.org/all/20230518-driving-secluding-793b3192776e@spud/ Do you? Cheers, Conor.
On Mon, May 22, 2023 at 1:08 AM Conor Dooley <conor@kernel.org> wrote: > > On Sun, May 21, 2023 at 11:37:58PM +0800, Guo Ren wrote: > > On Tue, May 9, 2023 at 12:44 AM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Tue, May 09, 2023 at 12:26:10AM +0800, Jisheng Zhang wrote: > > > > On Sun, May 07, 2023 at 10:35:12PM +0100, Conor Dooley wrote: > > > > > On Mon, May 08, 2023 at 02:23:02AM +0800, Jisheng Zhang wrote: > > > > > > > > > > > + c910_0: cpu@0 { > > > > > > + compatible = "thead,c910", "riscv"; > > > > > > + device_type = "cpu"; > > > > > > + riscv,isa = "rv64imafdc"; > > > > > > > > > > Does this support more than "rv64imafdc"? > > > > > I assume there's some _xtheadfoo extensions that it does support, > > > > > although I am not sure how we are proceeding with those - Heiko might > > > > > have a more nuanced take. > > > > > > > > > > > + reset: reset-sample { > > > > > > + compatible = "thead,reset-sample"; > > > > > > > > > > What is a "reset-sample"? > > > > > > > > This node is only for opensbi. The compatible string is already in > > > > opensbi. Do we also need to add dt-binding for it in linux? > > > > > > If it's to be included in the kernel's dts, then yes, you do need a > > > dt-binding. If you remove it, then you don't :) > > > > > > That said, "thead,reset-sample" is a strangely named compatible, so if > > > you do keep it it may end up needing a rename! > > > How about compatible = "thead,reset-th1520" ? > > "vendor,soc-function" is more typical, but "reset" is usually used for > reset controllers of which this isn't as far as I can tell. > I commented on the v2, hoping that you might actually know what the IP > block' full/proper name is: > https://lore.kernel.org/all/20230518-driving-secluding-793b3192776e@spud/ Oh, sorry, I focused on s64ilp32 these days and missed that. I would reply to that thread. > > Do you? > > Cheers, > Conor.
diff --git a/arch/riscv/boot/dts/thead/light.dtsi b/arch/riscv/boot/dts/thead/light.dtsi new file mode 100644 index 000000000000..cdf6d8b04d22 --- /dev/null +++ b/arch/riscv/boot/dts/thead/light.dtsi @@ -0,0 +1,454 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021 Alibaba Group Holding Limited. + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org> + */ + +/ { + compatible = "thead,light"; + #address-cells = <2>; + #size-cells = <2>; + + cpus: cpus { + #address-cells = <1>; + #size-cells = <0>; + timebase-frequency = <3000000>; + + c910_0: cpu@0 { + compatible = "thead,c910", "riscv"; + device_type = "cpu"; + riscv,isa = "rv64imafdc"; + 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_cache>; + mmu-type = "riscv,sv39"; + + cpu0_intc: interrupt-controller { + compatible = "riscv,cpu-intc"; + interrupt-controller; + #interrupt-cells = <1>; + }; + }; + + c910_1: cpu@1 { + compatible = "thead,c910", "riscv"; + device_type = "cpu"; + riscv,isa = "rv64imafdc"; + reg = <1>; + 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_cache>; + mmu-type = "riscv,sv39"; + + cpu1_intc: interrupt-controller { + compatible = "riscv,cpu-intc"; + interrupt-controller; + #interrupt-cells = <1>; + }; + }; + + c910_2: cpu@2 { + compatible = "thead,c910", "riscv"; + device_type = "cpu"; + riscv,isa = "rv64imafdc"; + reg = <2>; + 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_cache>; + mmu-type = "riscv,sv39"; + + cpu2_intc: interrupt-controller { + compatible = "riscv,cpu-intc"; + interrupt-controller; + #interrupt-cells = <1>; + }; + }; + + c910_3: cpu@3 { + compatible = "thead,c910", "riscv"; + device_type = "cpu"; + riscv,isa = "rv64imafdc"; + reg = <3>; + 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_cache>; + mmu-type = "riscv,sv39"; + + cpu3_intc: interrupt-controller { + compatible = "riscv,cpu-intc"; + interrupt-controller; + #interrupt-cells = <1>; + }; + }; + + cpu-map { + cluster0 { + core0 { + cpu = <&c910_0>; + }; + + core1 { + cpu = <&c910_1>; + }; + + core2 { + cpu = <&c910_2>; + }; + + core3 { + cpu = <&c910_3>; + }; + }; + }; + + l2_cache: l2-cache { + compatible = "cache"; + cache-block-size = <64>; + cache-level = <2>; + cache-size = <1048576>; + cache-sets = <1024>; + cache-unified; + }; + }; + + osc: oscillator { + compatible = "fixed-clock"; + clock-output-names = "osc_24m"; + #clock-cells = <0>; + }; + + osc_32k: 32k-oscillator { + compatible = "fixed-clock"; + clock-output-names = "osc_32k"; + #clock-cells = <0>; + }; + + apb_clk: apb-clk-clock { + compatible = "fixed-clock"; + clock-output-names = "apb_clk"; + #clock-cells = <0>; + }; + + uart_sclk: uart-sclk-clock { + compatible = "fixed-clock"; + clock-output-names = "uart_sclk"; + #clock-cells = <0>; + }; + + soc { + compatible = "simple-bus"; + interrupt-parent = <&plic>; + #address-cells = <2>; + #size-cells = <2>; + ranges; + + reset: reset-sample { + compatible = "thead,reset-sample"; + entry-reg = <0xff 0xff019050>; + entry-cnt = <4>; + control-reg = <0xff 0xff015004>; + control-val = <0x1c>; + csr-copy = <0x7f3 0x7c0 0x7c1 0x7c2 0x7c3 0x7c5 0x7cc>; + }; + + plic: interrupt-controller@ffd8000000 { + compatible = "thead,c910-plic"; + reg = <0xff 0xd8000000 0x0 0x01000000>; + interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>, + <&cpu1_intc 11>, <&cpu1_intc 9>, + <&cpu2_intc 11>, <&cpu2_intc 9>, + <&cpu3_intc 11>, <&cpu3_intc 9>; + interrupt-controller; + #interrupt-cells = <1>; + riscv,ndev = <240>; + }; + + clint: timer@ffdc000000 { + compatible = "thead,c900-clint"; + reg = <0xff 0xdc000000 0x0 0x00010000>; + interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>, + <&cpu1_intc 3>, <&cpu1_intc 7>, + <&cpu2_intc 3>, <&cpu2_intc 7>, + <&cpu3_intc 3>, <&cpu3_intc 7>; + }; + + uart0: serial@ffe7014000 { + compatible = "snps,dw-apb-uart"; + reg = <0xff 0xe7014000 0x0 0x4000>; + interrupts = <36>; + clocks = <&uart_sclk>; + clock-names = "baudclk"; + reg-shift = <2>; + reg-io-width = <4>; + status = "disabled"; + }; + + uart1: serial@ffe7f00000 { + compatible = "snps,dw-apb-uart"; + reg = <0xff 0xe7f00000 0x0 0x4000>; + interrupts = <37>; + clocks = <&uart_sclk>; + clock-names = "baudclk"; + reg-shift = <2>; + reg-io-width = <4>; + status = "disabled"; + }; + + uart3: serial@ffe7f04000 { + compatible = "snps,dw-apb-uart"; + reg = <0xff 0xe7f04000 0x0 0x4000>; + interrupts = <39>; + clocks = <&uart_sclk>; + clock-names = "baudclk"; + reg-shift = <2>; + reg-io-width = <4>; + status = "disabled"; + }; + + gpio2: gpio@ffe7f34000 { + compatible = "snps,dw-apb-gpio"; + reg = <0xff 0xe7f34000 0x0 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + + portc: gpio-controller@0 { + compatible = "snps,dw-apb-gpio-port"; + gpio-controller; + #gpio-cells = <2>; + ngpios = <32>; + reg = <0>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <58>; + }; + }; + + gpio3: gpio@ffe7f38000 { + compatible = "snps,dw-apb-gpio"; + reg = <0xff 0xe7f38000 0x0 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + + portd: gpio-controller@0 { + compatible = "snps,dw-apb-gpio-port"; + gpio-controller; + #gpio-cells = <2>; + ngpios = <32>; + reg = <0>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <59>; + }; + }; + + gpio0: gpio@ffec005000 { + compatible = "snps,dw-apb-gpio"; + reg = <0xff 0xec005000 0x0 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + + porta: gpio-controller@0 { + compatible = "snps,dw-apb-gpio-port"; + gpio-controller; + #gpio-cells = <2>; + ngpios = <32>; + reg = <0>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <56>; + }; + }; + + gpio1: gpio@ffec006000 { + compatible = "snps,dw-apb-gpio"; + reg = <0xff 0xec006000 0x0 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + + portb: gpio-controller@0 { + compatible = "snps,dw-apb-gpio-port"; + gpio-controller; + #gpio-cells = <2>; + ngpios = <32>; + reg = <0>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <57>; + }; + }; + + uart2: serial@ffec010000 { + compatible = "snps,dw-apb-uart"; + reg = <0xff 0xec010000 0x0 0x4000>; + interrupts = <38>; + clocks = <&uart_sclk>; + clock-names = "baudclk"; + reg-shift = <2>; + reg-io-width = <4>; + status = "disabled"; + }; + + dmac0: dmac@ffefc00000 { + compatible = "snps,axi-dma-1.01a"; + reg = <0xff 0xefc00000 0x0 0x1000>; + interrupts = <27>; + clocks = <&apb_clk>, <&apb_clk>; + clock-names = "core-clk", "cfgr-clk"; + #dma-cells = <1>; + dma-channels = <4>; + snps,block-size = <65536 65536 65536 65536>; + snps,priority = <0 1 2 3>; + snps,dma-masters = <1>; + snps,data-width = <4>; + snps,axi-max-burst-len = <16>; + status = "disabled"; + }; + + timer0: timer@ffefc32000 { + compatible = "snps,dw-apb-timer"; + reg = <0xff 0xefc32000 0x0 0x14>; + clocks = <&apb_clk>; + clock-names = "timer"; + interrupts = <16>; + status = "disabled"; + }; + + timer1: timer@ffefc32014 { + compatible = "snps,dw-apb-timer"; + reg = <0xff 0xefc32014 0x0 0x14>; + clocks = <&apb_clk>; + clock-names = "timer"; + interrupts = <17>; + status = "disabled"; + }; + + timer2: timer@ffefc32028 { + compatible = "snps,dw-apb-timer"; + reg = <0xff 0xefc32028 0x0 0x14>; + clocks = <&apb_clk>; + clock-names = "timer"; + interrupts = <18>; + status = "disabled"; + }; + + timer3: timer@ffefc3203c { + compatible = "snps,dw-apb-timer"; + reg = <0xff 0xefc3203c 0x0 0x14>; + clocks = <&apb_clk>; + clock-names = "timer"; + interrupts = <19>; + status = "disabled"; + }; + + uart4: serial@fff7f08000 { + compatible = "snps,dw-apb-uart"; + reg = <0xff 0xf7f08000 0x0 0x4000>; + interrupts = <40>; + clocks = <&uart_sclk>; + clock-names = "baudclk"; + reg-shift = <2>; + reg-io-width = <4>; + status = "disabled"; + }; + + uart5: serial@fff7f0c000 { + compatible = "snps,dw-apb-uart"; + reg = <0xff 0xf7f0c000 0x0 0x4000>; + interrupts = <41>; + clocks = <&uart_sclk>; + clock-names = "baudclk"; + reg-shift = <2>; + reg-io-width = <4>; + status = "disabled"; + }; + + timer4: timer@ffffc33000 { + compatible = "snps,dw-apb-timer"; + reg = <0xff 0xffc33000 0x0 0x14>; + clocks = <&apb_clk>; + clock-names = "timer"; + interrupts = <20>; + status = "disabled"; + }; + + timer5: timer@ffffc33014 { + compatible = "snps,dw-apb-timer"; + reg = <0xff 0xffc33014 0x0 0x14>; + clocks = <&apb_clk>; + clock-names = "timer"; + interrupts = <21>; + status = "disabled"; + }; + + timer6: timer@ffffc33028 { + compatible = "snps,dw-apb-timer"; + reg = <0xff 0xffc33028 0x0 0x14>; + clocks = <&apb_clk>; + clock-names = "timer"; + interrupts = <22>; + status = "disabled"; + }; + + timer7: timer@ffffc3303c { + compatible = "snps,dw-apb-timer"; + reg = <0xff 0xffc3303c 0x0 0x14>; + clocks = <&apb_clk>; + clock-names = "timer"; + interrupts = <23>; + status = "disabled"; + }; + + ao_gpio0: gpio@fffff41000 { + compatible = "snps,dw-apb-gpio"; + reg = <0xff 0xfff41000 0x0 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + + porte: gpio-controller@0 { + compatible = "snps,dw-apb-gpio-port"; + gpio-controller; + #gpio-cells = <2>; + ngpios = <32>; + reg = <0>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <76>; + }; + }; + + ao_gpio1: gpio@fffff52000 { + compatible = "snps,dw-apb-gpio"; + reg = <0xff 0xfff52000 0x0 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + + portf: gpio-controller@0 { + compatible = "snps,dw-apb-gpio-port"; + gpio-controller; + #gpio-cells = <2>; + ngpios = <32>; + reg = <0>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <55>; + }; + }; + }; +};
Add initial device tree for the light(a.k.a TH1520) RISC-V SoC by T-HEAD. Signed-off-by: Jisheng Zhang <jszhang@kernel.org> --- arch/riscv/boot/dts/thead/light.dtsi | 454 +++++++++++++++++++++++++++ 1 file changed, 454 insertions(+) create mode 100644 arch/riscv/boot/dts/thead/light.dtsi