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

[Inochi Amaoto]

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