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

[Inochi Amaoto]

>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@xxxxxxxxxxx>
>> 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.
>
>