diff mbox series

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

Message ID IA1PR20MB4953B8AC5CB8F8165A09D118BBB7A@IA1PR20MB4953.namprd20.prod.outlook.com
State New
Headers
Series Change the sg2042 timer layout to fit aclint format |

Commit Message

Inochi Amaoto Nov. 17, 2023, 5:07 a.m. UTC 
  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
---
 .../bindings/timer/thead,c900-aclint-mtimer.yaml         | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--
2.42.1

Comments

Krzysztof Kozlowski Nov. 17, 2023, 11:10 a.m. UTC | #1 
On 17/11/2023 06:07, 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.
> 

I don't understand this reasoning. You had one entry, you still have one
entry. Adding reg-names (not regs-names) does not change it.

Best regards,
Krzysztof
Inochi Amaoto Nov. 17, 2023, 12:30 p.m. UTC | #2 
>
>On 17/11/2023 06:07, 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.
>>
>
>I don't understand this reasoning. You had one entry, you still have one
>entry. Adding reg-names (not regs-names) does not change it.
>

If no "reg-names", all the register of ACLINT should be defined. However,
T-HEAD aclint timer of sg2042 only supports mtimecmp register. If no extra
prompt is provided for the SBI, it will fail to recognize aclint timer
registers when parsing the aclint node with one reg entry.

There is another way to avoid this by using an empty entry to identify
unsupported mtime, but Conor have already rejected this. See [1].

Link: https://lore.kernel.org/all/20231114-skedaddle-precinct-66c8897227bb@squawk/ [1]
Conor Dooley Nov. 17, 2023, 2:30 p.m. UTC | #3 
On Fri, Nov 17, 2023 at 08:30:21PM +0800, Inochi Amaoto wrote:
> >
> >On 17/11/2023 06:07, 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.
> >>
> >
> >I don't understand this reasoning. You had one entry, you still have one
> >entry. Adding reg-names (not regs-names) does not change it.
> >
> 
> If no "reg-names", all the register of ACLINT should be defined. However,
> T-HEAD aclint timer of sg2042 only supports mtimecmp register. If no extra
> prompt is provided for the SBI, it will fail to recognize aclint timer
> registers when parsing the aclint node with one reg entry.
> 
> There is another way to avoid this by using an empty entry to identify
> unsupported mtime, but Conor have already rejected this. See [1].
> 
> Link: https://lore.kernel.org/all/20231114-skedaddle-precinct-66c8897227bb@squawk/ [1]

Perhaps you misunderstood my suggestion. I was looking for _both_
registers to be defined in the binding as well as adding reg-names as a
required property. Doing what you have here might work for your use
case, but does not make sense from a bindings point of view as there is
no way to describe the mtime register, should it exist in another SoC.

Cheers,
Conor
Inochi Amaoto Nov. 17, 2023, 11:25 p.m. UTC | #4 
>
>On Fri, Nov 17, 2023 at 08:30:21PM +0800, Inochi Amaoto wrote:
>>>
>>> On 17/11/2023 06:07, 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.
>>>>
>>>
>>> I don't understand this reasoning. You had one entry, you still have one
>>> entry. Adding reg-names (not regs-names) does not change it.
>>>
>>
>> If no "reg-names", all the register of ACLINT should be defined. However,
>> T-HEAD aclint timer of sg2042 only supports mtimecmp register. If no extra
>> prompt is provided for the SBI, it will fail to recognize aclint timer
>> registers when parsing the aclint node with one reg entry.
>>
>> There is another way to avoid this by using an empty entry to identify
>> unsupported mtime, but Conor have already rejected this. See [1].
>>
>> Link: https://lore.kernel.org/all/20231114-skedaddle-precinct-66c8897227bb@squawk/ [1]
>
>Perhaps you misunderstood my suggestion. I was looking for _both_
>registers to be defined in the binding as well as adding reg-names as a
>required property. Doing what you have here might work for your use
>case, but does not make sense from a bindings point of view as there is
>no way to describe the mtime register, should it exist in another SoC.
>

Thanks for your clarification. If I understand you correctly, the binding
should have all registers that ACLINT has. But for specific use case,
it should only contain supported registers and omit unsupported. Please
correct me if I misunderstood. Thanks.

>Cheers,
>Conor
>
Conor Dooley Nov. 18, 2023, 12:58 a.m. UTC | #5 
> Thanks for your clarification. If I understand you correctly, the binding
> should have all registers that ACLINT has. But for specific use case,
> it should only contain supported registers and omit unsupported. Please
> correct me if I misunderstood. Thanks.

Yes.
Inochi Amaoto Nov. 18, 2023, 1:12 a.m. UTC | #6 
>> Thanks for your clarification. If I understand you correctly, the binding
>> should have all registers that ACLINT has. But for specific use case,
>> it should only contain supported registers and omit unsupported. Please
>> correct me if I misunderstood. Thanks.
>
>Yes.
>

Thanks, I will prepare a new binding for this.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
index fbd235650e52..2e92bcdeb423 100644
--- a/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
+++ b/Documentation/devicetree/bindings/timer/thead,c900-aclint-mtimer.yaml
@@ -17,7 +17,12 @@  properties:
       - const: thead,c900-aclint-mtimer

   reg:
-    maxItems: 1
+    items:
+      - description: MTIMECMP Registers
+
+  reg-names:
+    items:
+      - const: mtimecmp

   interrupts-extended:
     minItems: 1
@@ -28,6 +33,7 @@  additionalProperties: false
 required:
   - compatible
   - reg
+  - reg-names
   - interrupts-extended

 examples:
@@ -39,5 +45,6 @@  examples:
                             <&cpu3intc 7>,
                             <&cpu4intc 7>;
       reg = <0xac000000 0x00010000>;
+      reg-names = "mtimecmp";
     };
 ...