[v3,06/11] dt-bindings: timer: Add Sophgo sg2042 CLINT timer

  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.

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.

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>
---
 .../timer/sophgo,sg2042-clint-mtimer.yaml     | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/sophgo,sg2042-clint-mtimer.yaml
  

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.