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.