[v4,1/2] dt-bindings: timer: thead,c900-aclint-mtimer: separate mtime and mtimecmp regs

The timer registers of aclint don't follow the clint layout and can
be mapped on any different offset. As sg2042 uses separated timer
and mswi for its clint, it should follow the aclint spec and have
separated registers.

The previous patch introduced a new type of T-HEAD aclint timer which
has clint timer layout. Although it has the clint timer layout, it
should follow the aclint spec and uses the separated mtime and mtimecmp
regs. So a ABI change is needed to make the timer fit the aclint spec.

To make T-HEAD aclint timer more closer to the aclint spec, use
regs-names to represent the mtimecmp register, which can avoid hack
for unsupport mtime register of T-HEAD aclint timer.

Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
---
 .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

--
2.42.1

Yo,

On Sat, Nov 18, 2023 at 03:10:26PM +0800, Inochi Amaoto wrote:
> The timer registers of aclint don't follow the clint layout and can
> be mapped on any different offset. As sg2042 uses separated timer
> and mswi for its clint, it should follow the aclint spec and have
> separated registers.
> 
> The previous patch introduced a new type of T-HEAD aclint timer which
> has clint timer layout. Although it has the clint timer layout, it
> should follow the aclint spec and uses the separated mtime and mtimecmp
> regs. So a ABI change is needed to make the timer fit the aclint spec.
> 
> To make T-HEAD aclint timer more closer to the aclint spec, use
> regs-names to represent the mtimecmp register, which can avoid hack
> for unsupport mtime register of T-HEAD aclint timer.
> 
> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
> Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
> Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
> ---
>  .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> index fbd235650e52..053488fb1286 100644
> --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> @@ -17,7 +17,20 @@ properties:
>        - const: thead,c900-aclint-mtimer
> 
>    reg:
> -    maxItems: 1
> +    oneOf:
> +      - items:
> +          - description: MTIME Registers
> +          - description: MTIMECMP Registers
> +      - items:
> +          - description: MTIMECMP Registers
> +
> +  reg-names:
> +    oneOf:
> +      - items:
> +          - const: mtime
> +          - const: mtimecmp
> +      - items:
> +          - const: mtimecmp
> 
>    interrupts-extended:
>      minItems: 1
> @@ -28,8 +41,34 @@ additionalProperties: false
>  required:
>    - compatible
>    - reg
> +  - reg-names
>    - interrupts-extended
> 
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: thead,c900-aclint-mtimer

Is this being the c900 compatible correct? You mention in your commit
message that this split is done on the sg2042, but the rule is applied
here for any c900 series "aclint". Do we know if this is a sophgo
specific thing (or even sg2042 specific), or if it applies generally?

> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: MTIMECMP Registers
> +        reg-names:
> +          items:
> +            - const: mtimecmp

> +    else:
> +      properties:
> +        reg:
> +          items:
> +            - description: MTIME Registers
> +            - description: MTIMECMP Registers
> +        reg-names:
> +          items:
> +            - const: mtime
> +            - const: mtimecmp

If it applies generally, I would probably just delete this, but unless
someone can confirm this to be general, I'd probably leave the else
clause and swap for the specific sg2042 compatible above.

Otherwise, this looks like a better fix than you had proposed before :)

Thanks,
Conor.

>Yo,
>
>On Sat, Nov 18, 2023 at 03:10:26PM +0800, Inochi Amaoto wrote:
>> The timer registers of aclint don't follow the clint layout and can
>> be mapped on any different offset. As sg2042 uses separated timer
>> and mswi for its clint, it should follow the aclint spec and have
>> separated registers.
>>
>> The previous patch introduced a new type of T-HEAD aclint timer which
>> has clint timer layout. Although it has the clint timer layout, it
>> should follow the aclint spec and uses the separated mtime and mtimecmp
>> regs. So a ABI change is needed to make the timer fit the aclint spec.
>>
>> To make T-HEAD aclint timer more closer to the aclint spec, use
>> regs-names to represent the mtimecmp register, which can avoid hack
>> for unsupport mtime register of T-HEAD aclint timer.
>>
>> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
>> Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
>> Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
>> Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
>> ---
>>  .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
>>  1 file changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
>> index fbd235650e52..053488fb1286 100644
>> --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
>> +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
>> @@ -17,7 +17,20 @@ properties:
>>        - const: thead,c900-aclint-mtimer
>>
>>    reg:
>> -    maxItems: 1
>> +    oneOf:
>> +      - items:
>> +          - description: MTIME Registers
>> +          - description: MTIMECMP Registers
>> +      - items:
>> +          - description: MTIMECMP Registers
>> +
>> +  reg-names:
>> +    oneOf:
>> +      - items:
>> +          - const: mtime
>> +          - const: mtimecmp
>> +      - items:
>> +          - const: mtimecmp
>>
>>    interrupts-extended:
>>      minItems: 1
>> @@ -28,8 +41,34 @@ additionalProperties: false
>>  required:
>>    - compatible
>>    - reg
>> +  - reg-names
>>    - interrupts-extended
>>
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: thead,c900-aclint-mtimer
>
>Is this being the c900 compatible correct? You mention in your commit
>message that this split is done on the sg2042, but the rule is applied
>here for any c900 series "aclint". Do we know if this is a sophgo
>specific thing (or even sg2042 specific), or if it applies generally?
>

This can be confirmed. The thead c900 series have no mtime support and
there is no evidence that they will implement it. So I think it is OK
to applied this restriction for the whole c900 series.

>> +    then:
>> +      properties:
>> +        reg:
>> +          items:
>> +            - description: MTIMECMP Registers
>> +        reg-names:
>> +          items:
>> +            - const: mtimecmp
>
>> +    else:
>> +      properties:
>> +        reg:
>> +          items:
>> +            - description: MTIME Registers
>> +            - description: MTIMECMP Registers
>> +        reg-names:
>> +          items:
>> +            - const: mtime
>> +            - const: mtimecmp
>
>If it applies generally, I would probably just delete this, but unless
>someone can confirm this to be general, I'd probably leave the else
>clause and swap for the specific sg2042 compatible above.
>

I suggest keeping this. By taking your advice, this binding has actually
become the binding for aclint draft. So I think it is better to preserve
this path, otherwise adding the mtime register seems meaningless. But if
you think it is OK to add this when adding new compatible or converting it
to a generic binding. Feel free to remove it.

>Otherwise, this looks like a better fix than you had proposed before :)
>

Thanks.

>Thanks,
>Conor.
>
>

On 2023/11/18 15:10, Inochi Amaoto wrote:
> The timer registers of aclint don't follow the clint layout and can
> be mapped on any different offset. As sg2042 uses separated timer
> and mswi for its clint, it should follow the aclint spec and have
> separated registers.
>
> The previous patch introduced a new type of T-HEAD aclint timer which
> has clint timer layout. Although it has the clint timer layout, it
> should follow the aclint spec and uses the separated mtime and mtimecmp
> regs. So a ABI change is needed to make the timer fit the aclint spec.
>
> To make T-HEAD aclint timer more closer to the aclint spec, use
> regs-names to represent the mtimecmp register, which can avoid hack
> for unsupport mtime register of T-HEAD aclint timer.
>
> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
> Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
> Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
> ---
>   .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
>   1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> index fbd235650e52..053488fb1286 100644
> --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> @@ -17,7 +17,20 @@ properties:
>         - const: thead,c900-aclint-mtimer
>
>     reg:
> -    maxItems: 1
> +    oneOf:
> +      - items:
> +          - description: MTIME Registers
> +          - description: MTIMECMP Registers
> +      - items:
> +          - description: MTIMECMP Registers
> +
> +  reg-names:
> +    oneOf:
> +      - items:
> +          - const: mtime
> +          - const: mtimecmp
> +      - items:
> +          - const: mtimecmp
>
>     interrupts-extended:
>       minItems: 1
> @@ -28,8 +41,34 @@ additionalProperties: false
>   required:
>     - compatible
>     - reg
> +  - reg-names
>     - interrupts-extended
>
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: thead,c900-aclint-mtimer
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: MTIMECMP Registers
> +        reg-names:
> +          items:
> +            - const: mtimecmp
> +    else:
> +      properties:
> +        reg:
> +          items:
> +            - description: MTIME Registers
> +            - description: MTIMECMP Registers
> +        reg-names:
> +          items:
> +            - const: mtime
> +            - const: mtimecmp
> +
>   examples:
>     - |
>       timer@ac000000 {
> @@ -39,5 +78,6 @@ examples:
>                               <&cpu3intc 7>,
>                               <&cpu4intc 7>;
>         reg = <0xac000000 0x00010000>;
> +      reg-names = "mtimecmp";
>       };
>   ...
Reviewed-by: Chen Wang <unicorn_wang@outlook.com>
> --
> 2.42.1
>

On Tue, Nov 21, 2023 at 09:12:12AM +0800, Inochi Amaoto wrote:
> >Yo,
> >
> >On Sat, Nov 18, 2023 at 03:10:26PM +0800, Inochi Amaoto wrote:
> >> The timer registers of aclint don't follow the clint layout and can
> >> be mapped on any different offset. As sg2042 uses separated timer
> >> and mswi for its clint, it should follow the aclint spec and have
> >> separated registers.
> >>
> >> The previous patch introduced a new type of T-HEAD aclint timer which
> >> has clint timer layout. Although it has the clint timer layout, it
> >> should follow the aclint spec and uses the separated mtime and mtimecmp
> >> regs. So a ABI change is needed to make the timer fit the aclint spec.
> >>
> >> To make T-HEAD aclint timer more closer to the aclint spec, use
> >> regs-names to represent the mtimecmp register, which can avoid hack
> >> for unsupport mtime register of T-HEAD aclint timer.
> >>
> >> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> >> Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
> >> Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
> >> Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
> >> ---
> >>  .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
> >>  1 file changed, 41 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> >> index fbd235650e52..053488fb1286 100644
> >> --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> >> +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> >> @@ -17,7 +17,20 @@ properties:
> >>        - const: thead,c900-aclint-mtimer
> >>
> >>    reg:
> >> -    maxItems: 1
> >> +    oneOf:
> >> +      - items:
> >> +          - description: MTIME Registers
> >> +          - description: MTIMECMP Registers
> >> +      - items:
> >> +          - description: MTIMECMP Registers
> >> +
> >> +  reg-names:
> >> +    oneOf:
> >> +      - items:
> >> +          - const: mtime
> >> +          - const: mtimecmp
> >> +      - items:
> >> +          - const: mtimecmp
> >>
> >>    interrupts-extended:
> >>      minItems: 1
> >> @@ -28,8 +41,34 @@ additionalProperties: false
> >>  required:
> >>    - compatible
> >>    - reg
> >> +  - reg-names
> >>    - interrupts-extended
> >>
> >> +allOf:
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            const: thead,c900-aclint-mtimer
> >
> >Is this being the c900 compatible correct? You mention in your commit
> >message that this split is done on the sg2042, but the rule is applied
> >here for any c900 series "aclint". Do we know if this is a sophgo
> >specific thing (or even sg2042 specific), or if it applies generally?
> >
> 
> This can be confirmed. The thead c900 series have no mtime support and
> there is no evidence that they will implement it. So I think it is OK
> to applied this restriction for the whole c900 series.

Okay, great.

> >> +    then:
> >> +      properties:
> >> +        reg:
> >> +          items:
> >> +            - description: MTIMECMP Registers
> >> +        reg-names:
> >> +          items:
> >> +            - const: mtimecmp
> >
> >> +    else:
> >> +      properties:
> >> +        reg:
> >> +          items:
> >> +            - description: MTIME Registers
> >> +            - description: MTIMECMP Registers
> >> +        reg-names:
> >> +          items:
> >> +            - const: mtime
> >> +            - const: mtimecmp
> >
> >If it applies generally, I would probably just delete this, but unless
> >someone can confirm this to be general, I'd probably leave the else
> >clause and swap for the specific sg2042 compatible above.
> >
> 
> I suggest keeping this. By taking your advice, this binding has actually
> become the binding for aclint draft.

Right. It seemed to me from the reports (and the commit message) that this
was a configuration choice made by sophgo for the IP.

> So I think it is better to preserve
> this path, otherwise adding the mtime register seems meaningless.

Yeah, I mistakenly thought that there were cases where we actually had
systems with mtime and mtimecmp registers. I don't know if that was an
assumption I made due to previous commit messages or from reading the
opensbi threads, but clearly that is not the case.

> But if
> you think it is OK to add this when adding new compatible or converting it
> to a generic binding.

I'm a bit conflicted. Since this is c900 specific one part of me says
leave it with only one "reg" entry as that is what the only hardware
actually has & add "reg-names" to make lives easier when someone else
implements the unratified spec (or it gets ratified for some reason).

> Feel free to remove it.

I might've applied the other binding as it was in a series adding
initial support for the SoC, but usually these things go via the
subsystem maintainers with a DT maintainer ack/review.

>On Tue, Nov 21, 2023 at 09:12:12AM +0800, Inochi Amaoto wrote:
>>> Yo,
>>>
>>> On Sat, Nov 18, 2023 at 03:10:26PM +0800, Inochi Amaoto wrote:
>>>> The timer registers of aclint don't follow the clint layout and can
>>>> be mapped on any different offset. As sg2042 uses separated timer
>>>> and mswi for its clint, it should follow the aclint spec and have
>>>> separated registers.
>>>>
>>>> The previous patch introduced a new type of T-HEAD aclint timer which
>>>> has clint timer layout. Although it has the clint timer layout, it
>>>> should follow the aclint spec and uses the separated mtime and mtimecmp
>>>> regs. So a ABI change is needed to make the timer fit the aclint spec.
>>>>
>>>> To make T-HEAD aclint timer more closer to the aclint spec, use
>>>> regs-names to represent the mtimecmp register, which can avoid hack
>>>> for unsupport mtime register of T-HEAD aclint timer.
>>>>
>>>> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
>>>> Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
>>>> Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
>>>> Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
>>>> ---
>>>>  .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
>>>>  1 file changed, 41 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
>>>> index fbd235650e52..053488fb1286 100644
>>>> --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
>>>> +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
>>>> @@ -17,7 +17,20 @@ properties:
>>>>        - const: thead,c900-aclint-mtimer
>>>>
>>>>    reg:
>>>> -    maxItems: 1
>>>> +    oneOf:
>>>> +      - items:
>>>> +          - description: MTIME Registers
>>>> +          - description: MTIMECMP Registers
>>>> +      - items:
>>>> +          - description: MTIMECMP Registers
>>>> +
>>>> +  reg-names:
>>>> +    oneOf:
>>>> +      - items:
>>>> +          - const: mtime
>>>> +          - const: mtimecmp
>>>> +      - items:
>>>> +          - const: mtimecmp
>>>>
>>>>    interrupts-extended:
>>>>      minItems: 1
>>>> @@ -28,8 +41,34 @@ additionalProperties: false
>>>>  required:
>>>>    - compatible
>>>>    - reg
>>>> +  - reg-names
>>>>    - interrupts-extended
>>>>
>>>> +allOf:
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            const: thead,c900-aclint-mtimer
>>>
>>> Is this being the c900 compatible correct? You mention in your commit
>>> message that this split is done on the sg2042, but the rule is applied
>>> here for any c900 series "aclint". Do we know if this is a sophgo
>>> specific thing (or even sg2042 specific), or if it applies generally?
>>>
>>
>> This can be confirmed. The thead c900 series have no mtime support and
>> there is no evidence that they will implement it. So I think it is OK
>> to applied this restriction for the whole c900 series.
>
>Okay, great.
>
>>>> +    then:
>>>> +      properties:
>>>> +        reg:
>>>> +          items:
>>>> +            - description: MTIMECMP Registers
>>>> +        reg-names:
>>>> +          items:
>>>> +            - const: mtimecmp
>>>
>>>> +    else:
>>>> +      properties:
>>>> +        reg:
>>>> +          items:
>>>> +            - description: MTIME Registers
>>>> +            - description: MTIMECMP Registers
>>>> +        reg-names:
>>>> +          items:
>>>> +            - const: mtime
>>>> +            - const: mtimecmp
>>>
>>> If it applies generally, I would probably just delete this, but unless
>>> someone can confirm this to be general, I'd probably leave the else
>>> clause and swap for the specific sg2042 compatible above.
>>>
>>
>> I suggest keeping this. By taking your advice, this binding has actually
>> become the binding for aclint draft.
>
>Right. It seemed to me from the reports (and the commit message) that this
>was a configuration choice made by sophgo for the IP.
>

Yes, that's true.

>> So I think it is better to preserve
>> this path, otherwise adding the mtime register seems meaningless.
>
>Yeah, I mistakenly thought that there were cases where we actually had
>systems with mtime and mtimecmp registers. I don't know if that was an
>assumption I made due to previous commit messages or from reading the
>opensbi threads, but clearly that is not the case.
>
>> But if
>> you think it is OK to add this when adding new compatible or converting it
>> to a generic binding.
>
>I'm a bit conflicted. Since this is c900 specific one part of me says
>leave it with only one "reg" entry as that is what the only hardware
>actually has & add "reg-names" to make lives easier when someone else
>implements the unratified spec (or it gets ratified for some reason).
>

Adding "reg-names" is necessary and does make live easier. It gives a
clear way to avoid hack on skipping mtime register in the ACLINT timer
definition.

Now I think the only problem is whether the "mtime" register should
exist in this binding. IMHO, adding "mtime" seems to be too much to
keep this binding vendor specific. It is better to remove it to achieve
minimum change.

>> Feel free to remove it.
>
>I might've applied the other binding as it was in a series adding
>initial support for the SoC, but usually these things go via the
>subsystem maintainers with a DT maintainer ack/review.
>

Thanks, I will also wait for feedback from the subsystem maintainers.

>
>> On Tue, Nov 21, 2023 at 09:12:12AM +0800, Inochi Amaoto wrote:
>>>> Yo,
>>>>
>>>> On Sat, Nov 18, 2023 at 03:10:26PM +0800, Inochi Amaoto wrote:
>>>>> The timer registers of aclint don't follow the clint layout and can
>>>>> be mapped on any different offset. As sg2042 uses separated timer
>>>>> and mswi for its clint, it should follow the aclint spec and have
>>>>> separated registers.
>>>>>
>>>>> The previous patch introduced a new type of T-HEAD aclint timer which
>>>>> has clint timer layout. Although it has the clint timer layout, it
>>>>> should follow the aclint spec and uses the separated mtime and mtimecmp
>>>>> regs. So a ABI change is needed to make the timer fit the aclint spec.
>>>>>
>>>>> To make T-HEAD aclint timer more closer to the aclint spec, use
>>>>> regs-names to represent the mtimecmp register, which can avoid hack
>>>>> for unsupport mtime register of T-HEAD aclint timer.
>>>>>
>>>>> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
>>>>> Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
>>>>> Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
>>>>> Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
>>>>> ---
>>>>>  .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
>>>>>  1 file changed, 41 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
>>>>> index fbd235650e52..053488fb1286 100644
>>>>> --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
>>>>> +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
>>>>> @@ -17,7 +17,20 @@ properties:
>>>>>        - const: thead,c900-aclint-mtimer
>>>>>
>>>>>    reg:
>>>>> -    maxItems: 1
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          - description: MTIME Registers
>>>>> +          - description: MTIMECMP Registers
>>>>> +      - items:
>>>>> +          - description: MTIMECMP Registers
>>>>> +
>>>>> +  reg-names:
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          - const: mtime
>>>>> +          - const: mtimecmp
>>>>> +      - items:
>>>>> +          - const: mtimecmp
>>>>>
>>>>>    interrupts-extended:
>>>>>      minItems: 1
>>>>> @@ -28,8 +41,34 @@ additionalProperties: false
>>>>>  required:
>>>>>    - compatible
>>>>>    - reg
>>>>> +  - reg-names
>>>>>    - interrupts-extended
>>>>>
>>>>> +allOf:
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            const: thead,c900-aclint-mtimer
>>>>
>>>> Is this being the c900 compatible correct? You mention in your commit
>>>> message that this split is done on the sg2042, but the rule is applied
>>>> here for any c900 series "aclint". Do we know if this is a sophgo
>>>> specific thing (or even sg2042 specific), or if it applies generally?
>>>>
>>>
>>> This can be confirmed. The thead c900 series have no mtime support and
>>> there is no evidence that they will implement it. So I think it is OK
>>> to applied this restriction for the whole c900 series.
>>
>> Okay, great.
>>
>>>>> +    then:
>>>>> +      properties:
>>>>> +        reg:
>>>>> +          items:
>>>>> +            - description: MTIMECMP Registers
>>>>> +        reg-names:
>>>>> +          items:
>>>>> +            - const: mtimecmp
>>>>
>>>>> +    else:
>>>>> +      properties:
>>>>> +        reg:
>>>>> +          items:
>>>>> +            - description: MTIME Registers
>>>>> +            - description: MTIMECMP Registers
>>>>> +        reg-names:
>>>>> +          items:
>>>>> +            - const: mtime
>>>>> +            - const: mtimecmp
>>>>
>>>> If it applies generally, I would probably just delete this, but unless
>>>> someone can confirm this to be general, I'd probably leave the else
>>>> clause and swap for the specific sg2042 compatible above.
>>>>
>>>
>>> I suggest keeping this. By taking your advice, this binding has actually
>>> become the binding for aclint draft.
>>
>> Right. It seemed to me from the reports (and the commit message) that this
>> was a configuration choice made by sophgo for the IP.
>>
>
>Yes, that's true.
>
>>> So I think it is better to preserve
>>> this path, otherwise adding the mtime register seems meaningless.
>>
>> Yeah, I mistakenly thought that there were cases where we actually had
>> systems with mtime and mtimecmp registers. I don't know if that was an
>> assumption I made due to previous commit messages or from reading the
>> opensbi threads, but clearly that is not the case.
>>
>>> But if
>>> you think it is OK to add this when adding new compatible or converting it
>>> to a generic binding.
>>
>> I'm a bit conflicted. Since this is c900 specific one part of me says
>> leave it with only one "reg" entry as that is what the only hardware
>> actually has & add "reg-names" to make lives easier when someone else
>> implements the unratified spec (or it gets ratified for some reason).
>>
>
>Adding "reg-names" is necessary and does make live easier. It gives a
>clear way to avoid hack on skipping mtime register in the ACLINT timer
>definition.
>
>Now I think the only problem is whether the "mtime" register should
>exist in this binding. IMHO, adding "mtime" seems to be too much to
>keep this binding vendor specific. It is better to remove it to achieve
>minimum change.
>
>>> Feel free to remove it.
>>
>> I might've applied the other binding as it was in a series adding
>> initial support for the SoC, but usually these things go via the
>> subsystem maintainers with a DT maintainer ack/review.
>>
>
>Thanks, I will also wait for feedback from the subsystem maintainers.
>

Hi, Daniel and Thomas,
Could you share your suggestion about this ABI change?

On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote:
>
> The timer registers of aclint don't follow the clint layout and can
> be mapped on any different offset. As sg2042 uses separated timer
> and mswi for its clint, it should follow the aclint spec and have
> separated registers.
>
> The previous patch introduced a new type of T-HEAD aclint timer which
> has clint timer layout. Although it has the clint timer layout, it
> should follow the aclint spec and uses the separated mtime and mtimecmp
> regs. So a ABI change is needed to make the timer fit the aclint spec.
>
> To make T-HEAD aclint timer more closer to the aclint spec, use
> regs-names to represent the mtimecmp register, which can avoid hack
> for unsupport mtime register of T-HEAD aclint timer.
>
> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
> Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
> Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc

The ratified Priv v1.12 specification defines platform specific M-mode timer
registers without defining any layout of mtime and mtimecmp registers.
(Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)")

The "thead,c900-aclint-mtimer" can be thought of as is one possible
implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton.

If it is not too late then I suggest making this binding into generic
"riscv,mtimer" binding.

Regards,
Anup

> ---
>  .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> index fbd235650e52..053488fb1286 100644
> --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> @@ -17,7 +17,20 @@ properties:
>        - const: thead,c900-aclint-mtimer
>
>    reg:
> -    maxItems: 1
> +    oneOf:
> +      - items:
> +          - description: MTIME Registers
> +          - description: MTIMECMP Registers
> +      - items:
> +          - description: MTIMECMP Registers
> +
> +  reg-names:
> +    oneOf:
> +      - items:
> +          - const: mtime
> +          - const: mtimecmp
> +      - items:
> +          - const: mtimecmp
>
>    interrupts-extended:
>      minItems: 1
> @@ -28,8 +41,34 @@ additionalProperties: false
>  required:
>    - compatible
>    - reg
> +  - reg-names
>    - interrupts-extended
>
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: thead,c900-aclint-mtimer
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: MTIMECMP Registers
> +        reg-names:
> +          items:
> +            - const: mtimecmp
> +    else:
> +      properties:
> +        reg:
> +          items:
> +            - description: MTIME Registers
> +            - description: MTIMECMP Registers
> +        reg-names:
> +          items:
> +            - const: mtime
> +            - const: mtimecmp
> +
>  examples:
>    - |
>      timer@ac000000 {
> @@ -39,5 +78,6 @@ examples:
>                              <&cpu3intc 7>,
>                              <&cpu4intc 7>;
>        reg = <0xac000000 0x00010000>;
> +      reg-names = "mtimecmp";
>      };
>  ...
> --
> 2.42.1
>
>

On Thu, Nov 30, 2023 at 03:01:24PM +0530, Anup Patel wrote:
> On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote:
> >
> > The timer registers of aclint don't follow the clint layout and can
> > be mapped on any different offset. As sg2042 uses separated timer
> > and mswi for its clint, it should follow the aclint spec and have
> > separated registers.
> >
> > The previous patch introduced a new type of T-HEAD aclint timer which
> > has clint timer layout. Although it has the clint timer layout, it
> > should follow the aclint spec and uses the separated mtime and mtimecmp
> > regs. So a ABI change is needed to make the timer fit the aclint spec.
> >
> > To make T-HEAD aclint timer more closer to the aclint spec, use
> > regs-names to represent the mtimecmp register, which can avoid hack
> > for unsupport mtime register of T-HEAD aclint timer.
> >
> > Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> > Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
> > Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
> > Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
> 
> The ratified Priv v1.12 specification defines platform specific M-mode timer
> registers without defining any layout of mtime and mtimecmp registers.
> (Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)")
> 
> The "thead,c900-aclint-mtimer" can be thought of as is one possible
> implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton.
> 
> If it is not too late then I suggest making this binding into generic
> "riscv,mtimer" binding.

We could definitely reorganise things, it's not too late for that as
implementation specific compatibles would be needed regardless, so
software that would've matched on those will continue to do so.

That said, does this platform actually implement the 1.12 priv spec if
there is no mtime register? The section you reference says:
"Platforms provide a real-time counter, exposed as a memory-mapped
machine-mode read-write register, mtime." It seems to me like this
hardware is not suitable for a generic "riscv,mtimer" fallback.

Am I missing something there Anup?

It doesn't even implement the draft aclint spec, given that that says:
"The MTIMER device provides machine-level timer functionality for a set
of HARTs on a RISC-V platform. It has a single fixed-frequency monotonic
time counter (MTIME) register and a time compare register (MTIMECMP) for
each HART connected to the MTIMER device."

But I already said no to having a generic, "riscv" prefixed, compatible
for that, given it is in draft form.

Cheers,
Conor.

> > ---
> >  .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
> >  1 file changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> > index fbd235650e52..053488fb1286 100644
> > --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> > +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> > @@ -17,7 +17,20 @@ properties:
> >        - const: thead,c900-aclint-mtimer
> >
> >    reg:
> > -    maxItems: 1
> > +    oneOf:
> > +      - items:
> > +          - description: MTIME Registers
> > +          - description: MTIMECMP Registers
> > +      - items:
> > +          - description: MTIMECMP Registers
> > +
> > +  reg-names:
> > +    oneOf:
> > +      - items:
> > +          - const: mtime
> > +          - const: mtimecmp
> > +      - items:
> > +          - const: mtimecmp
> >
> >    interrupts-extended:
> >      minItems: 1
> > @@ -28,8 +41,34 @@ additionalProperties: false
> >  required:
> >    - compatible
> >    - reg
> > +  - reg-names
> >    - interrupts-extended
> >
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: thead,c900-aclint-mtimer
> > +    then:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - description: MTIMECMP Registers
> > +        reg-names:
> > +          items:
> > +            - const: mtimecmp
> > +    else:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - description: MTIME Registers
> > +            - description: MTIMECMP Registers
> > +        reg-names:
> > +          items:
> > +            - const: mtime
> > +            - const: mtimecmp
> > +
> >  examples:
> >    - |
> >      timer@ac000000 {
> > @@ -39,5 +78,6 @@ examples:
> >                              <&cpu3intc 7>,
> >                              <&cpu4intc 7>;
> >        reg = <0xac000000 0x00010000>;
> > +      reg-names = "mtimecmp";
> >      };
> >  ...
> > --
> > 2.42.1
> >
> >

On Thu, Nov 30, 2023 at 3:27 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Nov 30, 2023 at 03:01:24PM +0530, Anup Patel wrote:
> > On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote:
> > >
> > > The timer registers of aclint don't follow the clint layout and can
> > > be mapped on any different offset. As sg2042 uses separated timer
> > > and mswi for its clint, it should follow the aclint spec and have
> > > separated registers.
> > >
> > > The previous patch introduced a new type of T-HEAD aclint timer which
> > > has clint timer layout. Although it has the clint timer layout, it
> > > should follow the aclint spec and uses the separated mtime and mtimecmp
> > > regs. So a ABI change is needed to make the timer fit the aclint spec.
> > >
> > > To make T-HEAD aclint timer more closer to the aclint spec, use
> > > regs-names to represent the mtimecmp register, which can avoid hack
> > > for unsupport mtime register of T-HEAD aclint timer.
> > >
> > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> > > Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
> > > Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
> > > Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
> >
> > The ratified Priv v1.12 specification defines platform specific M-mode timer
> > registers without defining any layout of mtime and mtimecmp registers.
> > (Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)")
> >
> > The "thead,c900-aclint-mtimer" can be thought of as is one possible
> > implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton.
> >
> > If it is not too late then I suggest making this binding into generic
> > "riscv,mtimer" binding.
>
> We could definitely reorganise things, it's not too late for that as
> implementation specific compatibles would be needed regardless, so
> software that would've matched on those will continue to do so.
>
> That said, does this platform actually implement the 1.12 priv spec if
> there is no mtime register? The section you reference says:
> "Platforms provide a real-time counter, exposed as a memory-mapped
> machine-mode read-write register, mtime." It seems to me like this
> hardware is not suitable for a generic "riscv,mtimer" fallback.

Yes, the T-Head mtimer does not implement both mtime and mtimecmp
so technically it only implements a portion of the ratified RISC-V mtimer
chapter.

>
> Am I missing something there Anup?
>
> It doesn't even implement the draft aclint spec, given that that says:
> "The MTIMER device provides machine-level timer functionality for a set
> of HARTs on a RISC-V platform. It has a single fixed-frequency monotonic
> time counter (MTIME) register and a time compare register (MTIMECMP) for
> each HART connected to the MTIMER device."
>
> But I already said no to having a generic, "riscv" prefixed, compatible
> for that, given it is in draft form.

I am not suggesting T-Head timer implements aclint spec. Also,
since aclint spec is in draft state it is out of the question.

My suggestion is to treat "3.2.1 Machine Timer Registers (mtime
and mtimecmp)" as RISC-V mtimer defined by the RISC-V privileged
specification and define "riscv" prefixed DT binding for this.

This binding defines two possible values for "reg" property:
1) contains two items: a) mtime register address and,
     b) base address of mtimecmp registers
2) contain one item: a) base address of mtimecmp registers

The t-head mtimer seems to implement #2 whereas the RISC-V
mtimer (Priv spec) aligns with #1.

If we want to keep this DT binding t-head specific then
we should remove option #1 (above) from this DT binding
and add separate "riscv" prefixed DT binding for RISC-V mtimer.

Regards,
Anup

>
> Cheers,
> Conor.
>
> > > ---
> > >  .../timer/thead,c900-aclint-mtimer.yaml       | 42 ++++++++++++++++++-
> > >  1 file changed, 41 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> > > index fbd235650e52..053488fb1286 100644
> > > --- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> > > +++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
> > > @@ -17,7 +17,20 @@ properties:
> > >        - const: thead,c900-aclint-mtimer
> > >
> > >    reg:
> > > -    maxItems: 1
> > > +    oneOf:
> > > +      - items:
> > > +          - description: MTIME Registers
> > > +          - description: MTIMECMP Registers
> > > +      - items:
> > > +          - description: MTIMECMP Registers
> > > +
> > > +  reg-names:
> > > +    oneOf:
> > > +      - items:
> > > +          - const: mtime
> > > +          - const: mtimecmp
> > > +      - items:
> > > +          - const: mtimecmp
> > >
> > >    interrupts-extended:
> > >      minItems: 1
> > > @@ -28,8 +41,34 @@ additionalProperties: false
> > >  required:
> > >    - compatible
> > >    - reg
> > > +  - reg-names
> > >    - interrupts-extended
> > >
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: thead,c900-aclint-mtimer
> > > +    then:
> > > +      properties:
> > > +        reg:
> > > +          items:
> > > +            - description: MTIMECMP Registers
> > > +        reg-names:
> > > +          items:
> > > +            - const: mtimecmp
> > > +    else:
> > > +      properties:
> > > +        reg:
> > > +          items:
> > > +            - description: MTIME Registers
> > > +            - description: MTIMECMP Registers
> > > +        reg-names:
> > > +          items:
> > > +            - const: mtime
> > > +            - const: mtimecmp
> > > +
> > >  examples:
> > >    - |
> > >      timer@ac000000 {
> > > @@ -39,5 +78,6 @@ examples:
> > >                              <&cpu3intc 7>,
> > >                              <&cpu4intc 7>;
> > >        reg = <0xac000000 0x00010000>;
> > > +      reg-names = "mtimecmp";
> > >      };
> > >  ...
> > > --
> > > 2.42.1
> > >
> > >

On Thu, Nov 30, 2023 at 04:51:32PM +0530, Anup Patel wrote:
> On Thu, Nov 30, 2023 at 3:27 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Nov 30, 2023 at 03:01:24PM +0530, Anup Patel wrote:
> > > On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote:
> > > >
> > > > The timer registers of aclint don't follow the clint layout and can
> > > > be mapped on any different offset. As sg2042 uses separated timer
> > > > and mswi for its clint, it should follow the aclint spec and have
> > > > separated registers.
> > > >
> > > > The previous patch introduced a new type of T-HEAD aclint timer which
> > > > has clint timer layout. Although it has the clint timer layout, it
> > > > should follow the aclint spec and uses the separated mtime and mtimecmp
> > > > regs. So a ABI change is needed to make the timer fit the aclint spec.
> > > >
> > > > To make T-HEAD aclint timer more closer to the aclint spec, use
> > > > regs-names to represent the mtimecmp register, which can avoid hack
> > > > for unsupport mtime register of T-HEAD aclint timer.
> > > >
> > > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> > > > Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
> > > > Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
> > > > Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
> > >
> > > The ratified Priv v1.12 specification defines platform specific M-mode timer
> > > registers without defining any layout of mtime and mtimecmp registers.
> > > (Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)")
> > >
> > > The "thead,c900-aclint-mtimer" can be thought of as is one possible
> > > implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton.
> > >
> > > If it is not too late then I suggest making this binding into generic
> > > "riscv,mtimer" binding.
> >
> > We could definitely reorganise things, it's not too late for that as
> > implementation specific compatibles would be needed regardless, so
> > software that would've matched on those will continue to do so.
> >
> > That said, does this platform actually implement the 1.12 priv spec if
> > there is no mtime register? The section you reference says:
> > "Platforms provide a real-time counter, exposed as a memory-mapped
> > machine-mode read-write register, mtime." It seems to me like this
> > hardware is not suitable for a generic "riscv,mtimer" fallback.
> 
> Yes, the T-Head mtimer does not implement both mtime and mtimecmp
> so technically it only implements a portion of the ratified RISC-V mtimer
> chapter.
> 
> >
> > Am I missing something there Anup?
> >
> > It doesn't even implement the draft aclint spec, given that that says:
> > "The MTIMER device provides machine-level timer functionality for a set
> > of HARTs on a RISC-V platform. It has a single fixed-frequency monotonic
> > time counter (MTIME) register and a time compare register (MTIMECMP) for
> > each HART connected to the MTIMER device."
> >
> > But I already said no to having a generic, "riscv" prefixed, compatible
> > for that, given it is in draft form.
> 
> I am not suggesting T-Head timer implements aclint spec. Also,
> since aclint spec is in draft state it is out of the question.

I did not intend to imply that you were suggesting that there should be
one. I was just trying to clarify that I was not trying to bring back
the topic of a generic aclint binding applying here.

> My suggestion is to treat "3.2.1 Machine Timer Registers (mtime
> and mtimecmp)" as RISC-V mtimer defined by the RISC-V privileged
> specification and define "riscv" prefixed DT binding for this.

I'm not against a binding for that at all.

> This binding defines two possible values for "reg" property:
> 1) contains two items: a) mtime register address and,
>      b) base address of mtimecmp registers
> 2) contain one item: a) base address of mtimecmp registers
> 
> The t-head mtimer seems to implement #2 whereas the RISC-V
> mtimer (Priv spec) aligns with #1.
> 
> If we want to keep this DT binding t-head specific then
> we should remove option #1 (above) from this DT binding

This part is already the conclusion of one of the other "branches" of
this thread and is (AFAIU) Inochi's plan for the next version.

> and add separate "riscv" prefixed DT binding for RISC-V mtimer.

Do you know of any users for a "riscv,mtimer" binding that are not
covered by existing bindings for the clint?

Cheers,
Conor.

On Thu, Nov 30, 2023 at 5:15 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Nov 30, 2023 at 04:51:32PM +0530, Anup Patel wrote:
> > On Thu, Nov 30, 2023 at 3:27 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Thu, Nov 30, 2023 at 03:01:24PM +0530, Anup Patel wrote:
> > > > On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote:
> > > > >
> > > > > The timer registers of aclint don't follow the clint layout and can
> > > > > be mapped on any different offset. As sg2042 uses separated timer
> > > > > and mswi for its clint, it should follow the aclint spec and have
> > > > > separated registers.
> > > > >
> > > > > The previous patch introduced a new type of T-HEAD aclint timer which
> > > > > has clint timer layout. Although it has the clint timer layout, it
> > > > > should follow the aclint spec and uses the separated mtime and mtimecmp
> > > > > regs. So a ABI change is needed to make the timer fit the aclint spec.
> > > > >
> > > > > To make T-HEAD aclint timer more closer to the aclint spec, use
> > > > > regs-names to represent the mtimecmp register, which can avoid hack
> > > > > for unsupport mtime register of T-HEAD aclint timer.
> > > > >
> > > > > Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> > > > > Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
> > > > > Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
> > > > > Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
> > > >
> > > > The ratified Priv v1.12 specification defines platform specific M-mode timer
> > > > registers without defining any layout of mtime and mtimecmp registers.
> > > > (Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)")
> > > >
> > > > The "thead,c900-aclint-mtimer" can be thought of as is one possible
> > > > implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton.
> > > >
> > > > If it is not too late then I suggest making this binding into generic
> > > > "riscv,mtimer" binding.
> > >
> > > We could definitely reorganise things, it's not too late for that as
> > > implementation specific compatibles would be needed regardless, so
> > > software that would've matched on those will continue to do so.
> > >
> > > That said, does this platform actually implement the 1.12 priv spec if
> > > there is no mtime register? The section you reference says:
> > > "Platforms provide a real-time counter, exposed as a memory-mapped
> > > machine-mode read-write register, mtime." It seems to me like this
> > > hardware is not suitable for a generic "riscv,mtimer" fallback.
> >
> > Yes, the T-Head mtimer does not implement both mtime and mtimecmp
> > so technically it only implements a portion of the ratified RISC-V mtimer
> > chapter.
> >
> > >
> > > Am I missing something there Anup?
> > >
> > > It doesn't even implement the draft aclint spec, given that that says:
> > > "The MTIMER device provides machine-level timer functionality for a set
> > > of HARTs on a RISC-V platform. It has a single fixed-frequency monotonic
> > > time counter (MTIME) register and a time compare register (MTIMECMP) for
> > > each HART connected to the MTIMER device."
> > >
> > > But I already said no to having a generic, "riscv" prefixed, compatible
> > > for that, given it is in draft form.
> >
> > I am not suggesting T-Head timer implements aclint spec. Also,
> > since aclint spec is in draft state it is out of the question.
>
> I did not intend to imply that you were suggesting that there should be
> one. I was just trying to clarify that I was not trying to bring back
> the topic of a generic aclint binding applying here.
>
> > My suggestion is to treat "3.2.1 Machine Timer Registers (mtime
> > and mtimecmp)" as RISC-V mtimer defined by the RISC-V privileged
> > specification and define "riscv" prefixed DT binding for this.
>
> I'm not against a binding for that at all.

Thanks.

>
> > This binding defines two possible values for "reg" property:
> > 1) contains two items: a) mtime register address and,
> >      b) base address of mtimecmp registers
> > 2) contain one item: a) base address of mtimecmp registers
> >
> > The t-head mtimer seems to implement #2 whereas the RISC-V
> > mtimer (Priv spec) aligns with #1.
> >
> > If we want to keep this DT binding t-head specific then
> > we should remove option #1 (above) from this DT binding
>
> This part is already the conclusion of one of the other "branches" of
> this thread and is (AFAIU) Inochi's plan for the next version.

Sounds good.

>
> > and add separate "riscv" prefixed DT binding for RISC-V mtimer.
>
> Do you know of any users for a "riscv,mtimer" binding that are not
> covered by existing bindings for the clint?

Ventana Veyron-v1 implements a mtimer per-cluster (or chiplet)
which is compatible to "riscv,mtimer" (i.e. we have both mtime
and mtimecmp MMIO registers).

Regards,
Anup

On Thu, Nov 30, 2023 at 05:18:15PM +0530, Anup Patel wrote:
> On Thu, Nov 30, 2023 at 5:15 PM Conor Dooley <conor@kernel.org> wrote:

> > > and add separate "riscv" prefixed DT binding for RISC-V mtimer.
> >
> > Do you know of any users for a "riscv,mtimer" binding that are not
> > covered by existing bindings for the clint?
> 
> Ventana Veyron-v1 implements a mtimer per-cluster (or chiplet)
> which is compatible to "riscv,mtimer" (i.e. we have both mtime
> and mtimecmp MMIO registers).

Okay, thanks. I guess iff veyron-v1 DT support shows up (or other
similar devices) we can go ahead with a "riscv,mtimer" binding then.
I had thought that you guys were going to be using ACPI though, so
I guess the "other similar devices" applies.

>
>On Thu, Nov 30, 2023 at 04:51:32PM +0530, Anup Patel wrote:
>> On Thu, Nov 30, 2023 at 3:27 PM Conor Dooley <conor@kernel.org> wrote:
>>>
>>> On Thu, Nov 30, 2023 at 03:01:24PM +0530, Anup Patel wrote:
>>>> On Sat, Nov 18, 2023 at 12:39 PM Inochi Amaoto <inochiama@outlook.com> wrote:
>>>>>
>>>>> The timer registers of aclint don't follow the clint layout and can
>>>>> be mapped on any different offset. As sg2042 uses separated timer
>>>>> and mswi for its clint, it should follow the aclint spec and have
>>>>> separated registers.
>>>>>
>>>>> The previous patch introduced a new type of T-HEAD aclint timer which
>>>>> has clint timer layout. Although it has the clint timer layout, it
>>>>> should follow the aclint spec and uses the separated mtime and mtimecmp
>>>>> regs. So a ABI change is needed to make the timer fit the aclint spec.
>>>>>
>>>>> To make T-HEAD aclint timer more closer to the aclint spec, use
>>>>> regs-names to represent the mtimecmp register, which can avoid hack
>>>>> for unsupport mtime register of T-HEAD aclint timer.
>>>>>
>>>>> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
>>>>> Fixes: 4734449f7311 ("dt-bindings: timer: Add Sophgo sg2042 CLINT timer")
>>>>> Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
>>>>> Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
>>>>
>>>> The ratified Priv v1.12 specification defines platform specific M-mode timer
>>>> registers without defining any layout of mtime and mtimecmp registers.
>>>> (Refer, "3.2.1 Machine Timer Registers (mtime and mtimecmp)")
>>>>
>>>> The "thead,c900-aclint-mtimer" can be thought of as is one possible
>>>> implementation of "riscv,mtimer" defined by the Priv v1.12 specificaiton.
>>>>
>>>> If it is not too late then I suggest making this binding into generic
>>>> "riscv,mtimer" binding.
>>>
>>> We could definitely reorganise things, it's not too late for that as
>>> implementation specific compatibles would be needed regardless, so
>>> software that would've matched on those will continue to do so.
>>>
>>> That said, does this platform actually implement the 1.12 priv spec if
>>> there is no mtime register? The section you reference says:
>>> "Platforms provide a real-time counter, exposed as a memory-mapped
>>> machine-mode read-write register, mtime." It seems to me like this
>>> hardware is not suitable for a generic "riscv,mtimer" fallback.
>>
>> Yes, the T-Head mtimer does not implement both mtime and mtimecmp
>> so technically it only implements a portion of the ratified RISC-V mtimer
>> chapter.
>>
>>>
>>> Am I missing something there Anup?
>>>
>>> It doesn't even implement the draft aclint spec, given that that says:
>>> "The MTIMER device provides machine-level timer functionality for a set
>>> of HARTs on a RISC-V platform. It has a single fixed-frequency monotonic
>>> time counter (MTIME) register and a time compare register (MTIMECMP) for
>>> each HART connected to the MTIMER device."
>>>
>>> But I already said no to having a generic, "riscv" prefixed, compatible
>>> for that, given it is in draft form.
>>
>> I am not suggesting T-Head timer implements aclint spec. Also,
>> since aclint spec is in draft state it is out of the question.
>
>I did not intend to imply that you were suggesting that there should be
>one. I was just trying to clarify that I was not trying to bring back
>the topic of a generic aclint binding applying here.
>
>> My suggestion is to treat "3.2.1 Machine Timer Registers (mtime
>> and mtimecmp)" as RISC-V mtimer defined by the RISC-V privileged
>> specification and define "riscv" prefixed DT binding for this.
>
>I'm not against a binding for that at all.
>
>> This binding defines two possible values for "reg" property:
>> 1) contains two items: a) mtime register address and,
>>      b) base address of mtimecmp registers
>> 2) contain one item: a) base address of mtimecmp registers
>>
>> The t-head mtimer seems to implement #2 whereas the RISC-V
>> mtimer (Priv spec) aligns with #1.
>>
>> If we want to keep this DT binding t-head specific then
>> we should remove option #1 (above) from this DT binding
>
>This part is already the conclusion of one of the other "branches" of
>this thread and is (AFAIU) Inochi's plan for the next version.
>

Yes, I have already made a new version for this.
But in fact, that is just the V3 version of this patch. This is why now I
still wait for some advice.

The V3 version is just T-HEAD specific:
https://lore.kernel.org/all/IA1PR20MB4953B8AC5CB8F8165A09D118BBB7A@IA1PR20MB4953.namprd20.prod.outlook.com/

>> and add separate "riscv" prefixed DT binding for RISC-V mtimer.
>
>Do you know of any users for a "riscv,mtimer" binding that are not
>covered by existing bindings for the clint?
>
>Cheers,
>Conor.
>
>

On Thu, Nov 30, 2023 at 5:40 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Nov 30, 2023 at 05:18:15PM +0530, Anup Patel wrote:
> > On Thu, Nov 30, 2023 at 5:15 PM Conor Dooley <conor@kernel.org> wrote:
>
> > > > and add separate "riscv" prefixed DT binding for RISC-V mtimer.
> > >
> > > Do you know of any users for a "riscv,mtimer" binding that are not
> > > covered by existing bindings for the clint?
> >
> > Ventana Veyron-v1 implements a mtimer per-cluster (or chiplet)
> > which is compatible to "riscv,mtimer" (i.e. we have both mtime
> > and mtimecmp MMIO registers).
>
> Okay, thanks. I guess iff veyron-v1 DT support shows up (or other
> similar devices) we can go ahead with a "riscv,mtimer" binding then.
> I had thought that you guys were going to be using ACPI though, so
> I guess the "other similar devices" applies.

We use ACPI from EDK2 onwards in our boot-flow. The booting
stages prior to EDK2 (such as OpenSBI) use DT. In fact, EDK2
also uses information in DT to populate static parts of the ACPI
table.

Regards,
Anup

>On Thu, Nov 30, 2023 at 5:40 PM Conor Dooley <conor@kernel.org> wrote:
>>
>> On Thu, Nov 30, 2023 at 05:18:15PM +0530, Anup Patel wrote:
>>> On Thu, Nov 30, 2023 at 5:15 PM Conor Dooley <conor@kernel.org> wrote:
>>
>>>>> and add separate "riscv" prefixed DT binding for RISC-V mtimer.
>>>>
>>>> Do you know of any users for a "riscv,mtimer" binding that are not
>>>> covered by existing bindings for the clint?
>>>
>>> Ventana Veyron-v1 implements a mtimer per-cluster (or chiplet)
>>> which is compatible to "riscv,mtimer" (i.e. we have both mtime
>>> and mtimecmp MMIO registers).
>>
>> Okay, thanks. I guess iff veyron-v1 DT support shows up (or other
>> similar devices) we can go ahead with a "riscv,mtimer" binding then.
>> I had thought that you guys were going to be using ACPI though, so
>> I guess the "other similar devices" applies.
>
>We use ACPI from EDK2 onwards in our boot-flow. The booting
>stages prior to EDK2 (such as OpenSBI) use DT. In fact, EDK2
>also uses information in DT to populate static parts of the ACPI
>table.
>

Yes, And the EDK2 implement of sg2042 shares the same boot flow, which is
already in the mainline EDK2 repo.

>Regards,
>Anup
>