| Message ID | 6e263430685732a4f354b45396c7422a37440ac8.1695804418.git.unicornxw@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | Add Milk-V Pioneer RISC-V board support | expand |
Hey, On Wed, Sep 27, 2023 at 05:01:37PM +0800, Chen Wang wrote: > From: Inochi Amaoto <inochiama@outlook.com> > > The clint of Sophgo sg2042 is incompatible with the standard sifive > clint, as the timer and ipi device on the different address, and can > not be handled by the sifive,clint DT. > > In addition, the timers of sg2042 are mapped by per cluster, which is > hard to merge with its ipi device. I think the description here is kinda poor, it needs to be explained that this is an implementation of the not frozen & likely abandoned aclint spec. > To avoid conficts caused by using the same clint compatible string when > this device is parsed by SBI, add a new vendor specific compatible string > to identify the timer of sg2042 soc. And this whole section about avoiding conflicts is not relevant, since the binding is specifically for the timer. It'd be better to mention why a single compatible cannot work for all elements, than bring up a situation that does not exist and would be a misuse of the binding in the first place. > Signed-off-by: Inochi Amaoto <inochiama@outlook.com> > Signed-off-by: Chen Wang <wangchen20@iscas.ac.cn> > Signed-off-by: Chen Wang <unicornxw@gmail.com> You only need to sign this off once. The iscas one looks like it probably is the relevant signoff? > --- > .../timer/sophgo,sg2042-clint-mtimer.yaml | 42 +++++++++++++++++++ > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml > > diff --git a/Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml b/Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml > new file mode 100644 > index 000000000000..5da0947d048a > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml > @@ -0,0 +1,42 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/timer/sophgo,sg2042-clint-mtimer.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Sophgo CLINT Timer > + > +maintainers: > + - Inochi Amaoto <inochiama@outlook.com> > + > +properties: > + compatible: > + oneOf: > + - items: > + - const: sophgo,sg2042-clint-mtimer There's only one of these, so you don't need the oneOf. Also, is the clint here not a thead IP? In which case, you need to add a second compatible IMO. That second compatible then would be the one that appears in opensbi etc. Otherwise, this looks fine. Thanks, Conor. > + > + reg: > + maxItems: 1 > + > + interrupts-extended: > + minItems: 1 > + maxItems: 4095 > + > +additionalProperties: false > + > +required: > + - compatible > + - reg > + - interrupts-extended > + > +examples: > + - | > + timer@ac000000 { > + compatible = "sophgo,sg2042-clint-mtimer"; > + interrupts-extended = <&cpu1intc 7>, > + <&cpu2intc 7>, > + <&cpu3intc 7>, > + <&cpu4intc 7>; > + reg = <0xac000000 0x00010000>; > + }; > +... > -- > 2.25.1 >
>Hey, > >On Wed, Sep 27, 2023 at 05:01:37PM +0800, Chen Wang wrote: >> From: Inochi Amaoto <inochiama@outlook.com> >> >> The clint of Sophgo sg2042 is incompatible with the standard sifive >> clint, as the timer and ipi device on the different address, and can >> not be handled by the sifive,clint DT. >> >> In addition, the timers of sg2042 are mapped by per cluster, which is >> hard to merge with its ipi device. > >I think the description here is kinda poor, it needs to be explained >that this is an implementation of the not frozen & likely abandoned >aclint spec. > Thanks, I will explain this. >> To avoid conficts caused by using the same clint compatible string when >> this device is parsed by SBI, add a new vendor specific compatible string >> to identify the timer of sg2042 soc. > >And this whole section about avoiding conflicts is not relevant, since >the binding is specifically for the timer. It'd be better to mention why >a single compatible cannot work for all elements, than bring up a >situation that does not exist and would be a misuse of the binding in >the first place. > Thanks >> Signed-off-by: Inochi Amaoto <inochiama@outlook.com> >> Signed-off-by: Chen Wang <wangchen20@iscas.ac.cn> >> Signed-off-by: Chen Wang <unicornxw@gmail.com> > >You only need to sign this off once. The iscas one looks like it >probably is the relevant signoff? > >> --- >> .../timer/sophgo,sg2042-clint-mtimer.yaml | 42 +++++++++++++++++++ >> 1 file changed, 42 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml >> >> diff --git a/Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml b/Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml >> new file mode 100644 >> index 000000000000..5da0947d048a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml >> @@ -0,0 +1,42 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/timer/sophgo,sg2042-clint-mtimer.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Sophgo CLINT Timer >> + >> +maintainers: >> + - Inochi Amaoto <inochiama@outlook.com> >> + >> +properties: >> + compatible: >> + oneOf: >> + - items: >> + - const: sophgo,sg2042-clint-mtimer > >There's only one of these, so you don't need the oneOf. Thanks >Also, is the clint here not a thead IP? In which case, you need to add a Yes, The clint is a thead IP, like that of th1520 and allwinner D1. >second compatible IMO. That second compatible then would be the one that >appears in opensbi etc. > As this is a thead IP, maybe use thead,c900-clint-mtimer is fine? If so, whether we should replace the "thead,c900-clint" with these separate DT to describe the thead clint? The DT binding said the thead clint is not compatible with the sifive clint, so maybe this is a chance to just move them out. >Otherwise, this looks fine. > >Thanks, >Conor. > >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts-extended: >> + minItems: 1 >> + maxItems: 4095 >> + >> +additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - interrupts-extended >> + >> +examples: >> + - | >> + timer@ac000000 { >> + compatible = "sophgo,sg2042-clint-mtimer"; >> + interrupts-extended = <&cpu1intc 7>, >> + <&cpu2intc 7>, >> + <&cpu3intc 7>, >> + <&cpu4intc 7>; >> + reg = <0xac000000 0x00010000>; >> + }; >> +... >> -- >> 2.25.1 >> >
On Thu, Sep 28, 2023 at 08:34:42AM +0800, Inochi Amaoto wrote: > >> +properties: > >> + compatible: > >> + oneOf: > >> + - items: > >> + - const: sophgo,sg2042-clint-mtimer > > > >There's only one of these, so you don't need the oneOf. > > Thanks > > >Also, is the clint here not a thead IP? In which case, you need to add a > > Yes, The clint is a thead IP, like that of th1520 and allwinner D1. > > >second compatible IMO. That second compatible then would be the one that > >appears in opensbi etc. > > > > As this is a thead IP, maybe use thead,c900-clint-mtimer is fine? I would suggest calling it -aclint-mtimer instead of clint-mtimer. > If so, whether we should replace the "thead,c900-clint" with these separate > DT to describe the thead clint? No, since that's a different device, right? > The DT binding said the thead clint is not > compatible with the sifive clint, so maybe this is a chance to just move > them out. I don't think that it really makes sense to do that. Thanks, Conor.
> >On Thu, Sep 28, 2023 at 08:34:42AM +0800, Inochi Amaoto wrote: > >>>> +properties: >>>> + compatible: >>>> + oneOf: >>>> + - items: >>>> + - const: sophgo,sg2042-clint-mtimer >>> >>> There's only one of these, so you don't need the oneOf. >> >> Thanks >> >>> Also, is the clint here not a thead IP? In which case, you need to add a >> >> Yes, The clint is a thead IP, like that of th1520 and allwinner D1. >> >>> second compatible IMO. That second compatible then would be the one that >>> appears in opensbi etc. >>> >> >> As this is a thead IP, maybe use thead,c900-clint-mtimer is fine? > >I would suggest calling it -aclint-mtimer instead of clint-mtimer. > It is OK for me. As I describe below, now use sophgo as vendor is better. Anyway, I will add a new second one in the next patch. >> If so, whether we should replace the "thead,c900-clint" with these separate >> DT to describe the thead clint? > >No, since that's a different device, right? > Yes. It seems sophgo defined these by themselves, but the T-HEAD. Sorry for my mistake. >> The DT binding said the thead clint is not >> compatible with the sifive clint, so maybe this is a chance to just move >> them out. > >I don't think that it really makes sense to do that. > >Thanks, >Conor. > >
On Thu, Sep 28, 2023 at 04:24:54PM +0800, Inochi Amaoto wrote: > > > >On Thu, Sep 28, 2023 at 08:34:42AM +0800, Inochi Amaoto wrote: > > > >>>> +properties: > >>>> + compatible: > >>>> + oneOf: > >>>> + - items: > >>>> + - const: sophgo,sg2042-clint-mtimer > >>> > >>> There's only one of these, so you don't need the oneOf. > >> > >> Thanks > >> > >>> Also, is the clint here not a thead IP? In which case, you need to add a > >> > >> Yes, The clint is a thead IP, like that of th1520 and allwinner D1. > >> > >>> second compatible IMO. That second compatible then would be the one that > >>> appears in opensbi etc. > >>> > >> > >> As this is a thead IP, maybe use thead,c900-clint-mtimer is fine? > > > >I would suggest calling it -aclint-mtimer instead of clint-mtimer. > > > > It is OK for me. As I describe below, now use sophgo as vendor is better. > Anyway, I will add a new second one in the next patch. > > >> If so, whether we should replace the "thead,c900-clint" with these separate > >> DT to describe the thead clint? > > > >No, since that's a different device, right? > > > > Yes. It seems sophgo defined these by themselves, but the T-HEAD. Sorry > for my mistake. I'm sorry, I don't quite understand this. Do you mean that the IP is not T-Head, but rather designed by Sophgo? If the IP is made by T-Head, then I would expect to see something like compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer"; in the dts. > > >> The DT binding said the thead clint is not > >> compatible with the sifive clint, so maybe this is a chance to just move > >> them out. > > > >I don't think that it really makes sense to do that.
>>>> If so, whether we should replace the "thead,c900-clint" with these separate >>>> DT to describe the thead clint? >>> >>> No, since that's a different device, right? >>> >> >> Yes. It seems sophgo defined these by themselves, but the T-HEAD. Sorry >> for my mistake. > >I'm sorry, I don't quite understand this. Do you mean that the IP is not >T-Head, but rather designed by Sophgo? If the IP is made by T-Head, then >I would expect to see something like > >compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer"; > >in the dts. > AFAIK, the clint IP is designed by T-HEAD, not Sophgo. Sophgo change this IP layout to fit its weird cpu design. But in my test, the timer and mswi of clint is compatible with the T-HEAD one. So we should treat this as T-HEAD IP, not Sophgo?
On Thu, Sep 28, 2023 at 05:39:17PM +0800, Inochi Amaoto wrote: > >>>> If so, whether we should replace the "thead,c900-clint" with these separate > >>>> DT to describe the thead clint? > >>> > >>> No, since that's a different device, right? > >>> > >> > >> Yes. It seems sophgo defined these by themselves, but the T-HEAD. Sorry > >> for my mistake. > > > >I'm sorry, I don't quite understand this. Do you mean that the IP is not > >T-Head, but rather designed by Sophgo? If the IP is made by T-Head, then > >I would expect to see something like > > > >compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer"; > > > >in the dts. > > > > AFAIK, the clint IP is designed by T-HEAD, not Sophgo. Sophgo change this > IP layout to fit its weird cpu design. But in my test, the timer and mswi > of clint is compatible with the T-HEAD one. > So we should treat this as T-HEAD IP, not Sophgo? Yes, in the way I demonstrated above probably.
diff --git a/Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml b/Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml new file mode 100644 index 000000000000..5da0947d048a --- /dev/null +++ b/Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml @@ -0,0 +1,42 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/timer/sophgo,sg2042-clint-mtimer.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sophgo CLINT Timer + +maintainers: + - Inochi Amaoto <inochiama@outlook.com> + +properties: + compatible: + oneOf: + - items: + - const: sophgo,sg2042-clint-mtimer + + reg: + maxItems: 1 + + interrupts-extended: + minItems: 1 + maxItems: 4095 + +additionalProperties: false + +required: + - compatible + - reg + - interrupts-extended + +examples: + - | + timer@ac000000 { + compatible = "sophgo,sg2042-clint-mtimer"; + interrupts-extended = <&cpu1intc 7>, + <&cpu2intc 7>, + <&cpu3intc 7>, + <&cpu4intc 7>; + reg = <0xac000000 0x00010000>; + }; +...