Hey,
On Wed, Sep 27, 2023 at 05:01:37PM +0800, Chen Wang wrote:
> From: Inochi Amaoto <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Chen Wang <[email protected]>
> Signed-off-by: Chen Wang <[email protected]>
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 <[email protected]>
> +
> +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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Chen Wang <[email protected]>
>> Signed-off-by: Chen Wang <[email protected]>
>
>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 <[email protected]>
>> +
>> +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.