As the sg2042 uses different address for timer and mswi of its clint
device, it should follow the aclint format. For the previous patchs,
it only use only one address for both mtime and mtimer, this is can
not be parsed by OpenSBI. To resolve this, separate these two registers
in the dtb.
Link: https://lists.infradead.org/pipermail/opensbi/2023-October/005693.html
Link: https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc
This patch can be tested with upstream SBI with the following patch:
1. https://lists.infradead.org/pipermail/opensbi/2023-November/005926.html
Changed from v3:
1. add all register in the bindings
Changed from v2:
1. Use reg-names to map the registers.
Changed from v1:
1. change the commit to address the reason for ABI change.
2. remove unnecessary link in the commit.
Inochi Amaoto (2):
dt-bindings: timer: thead,c900-aclint-mtimer: separate mtime and
mtimecmp regs
riscv: dts: sophgo: separate sg2042 mtime and mtimecmp to fit aclint
format
.../timer/thead,c900-aclint-mtimer.yaml | 42 +++++++++-
arch/riscv/boot/dts/sophgo/sg2042.dtsi | 80 +++++++++++--------
2 files changed, 89 insertions(+), 33 deletions(-)
--
2.42.1
Change the timer layout in the dtb to fit the format that needed by
the SBI.
Signed-off-by: Inochi Amaoto <[email protected]>
Fixes: 967a94a92aaa ("riscv: dts: add initial Sophgo SG2042 SoC device tree")
---
arch/riscv/boot/dts/sophgo/sg2042.dtsi | 80 +++++++++++++++-----------
1 file changed, 48 insertions(+), 32 deletions(-)
diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
index 93256540d078..ead1cc35d88b 100644
--- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
+++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
@@ -93,144 +93,160 @@ clint_mswi: interrupt-controller@7094000000 {
<&cpu63_intc 3>;
};
- clint_mtimer0: timer@70ac000000 {
+ clint_mtimer0: timer@70ac004000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac000000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac004000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu0_intc 7>,
<&cpu1_intc 7>,
<&cpu2_intc 7>,
<&cpu3_intc 7>;
};
- clint_mtimer1: timer@70ac010000 {
+ clint_mtimer1: timer@70ac014000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac010000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac014000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu4_intc 7>,
<&cpu5_intc 7>,
<&cpu6_intc 7>,
<&cpu7_intc 7>;
};
- clint_mtimer2: timer@70ac020000 {
+ clint_mtimer2: timer@70ac024000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac020000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac024000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu8_intc 7>,
<&cpu9_intc 7>,
<&cpu10_intc 7>,
<&cpu11_intc 7>;
};
- clint_mtimer3: timer@70ac030000 {
+ clint_mtimer3: timer@70ac034000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac030000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac034000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu12_intc 7>,
<&cpu13_intc 7>,
<&cpu14_intc 7>,
<&cpu15_intc 7>;
};
- clint_mtimer4: timer@70ac040000 {
+ clint_mtimer4: timer@70ac044000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac040000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac044000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu16_intc 7>,
<&cpu17_intc 7>,
<&cpu18_intc 7>,
<&cpu19_intc 7>;
};
- clint_mtimer5: timer@70ac050000 {
+ clint_mtimer5: timer@70ac054000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac050000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac054000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu20_intc 7>,
<&cpu21_intc 7>,
<&cpu22_intc 7>,
<&cpu23_intc 7>;
};
- clint_mtimer6: timer@70ac060000 {
+ clint_mtimer6: timer@70ac064000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac060000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac064000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu24_intc 7>,
<&cpu25_intc 7>,
<&cpu26_intc 7>,
<&cpu27_intc 7>;
};
- clint_mtimer7: timer@70ac070000 {
+ clint_mtimer7: timer@70ac074000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac070000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac074000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu28_intc 7>,
<&cpu29_intc 7>,
<&cpu30_intc 7>,
<&cpu31_intc 7>;
};
- clint_mtimer8: timer@70ac080000 {
+ clint_mtimer8: timer@70ac084000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac080000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac084000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu32_intc 7>,
<&cpu33_intc 7>,
<&cpu34_intc 7>,
<&cpu35_intc 7>;
};
- clint_mtimer9: timer@70ac090000 {
+ clint_mtimer9: timer@70ac094000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac090000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac094000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu36_intc 7>,
<&cpu37_intc 7>,
<&cpu38_intc 7>,
<&cpu39_intc 7>;
};
- clint_mtimer10: timer@70ac0a0000 {
+ clint_mtimer10: timer@70ac0a4000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac0a0000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac0a4000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu40_intc 7>,
<&cpu41_intc 7>,
<&cpu42_intc 7>,
<&cpu43_intc 7>;
};
- clint_mtimer11: timer@70ac0b0000 {
+ clint_mtimer11: timer@70ac0b4000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac0b0000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac0b4000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu44_intc 7>,
<&cpu45_intc 7>,
<&cpu46_intc 7>,
<&cpu47_intc 7>;
};
- clint_mtimer12: timer@70ac0c0000 {
+ clint_mtimer12: timer@70ac0c4000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac0c0000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac0c4000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu48_intc 7>,
<&cpu49_intc 7>,
<&cpu50_intc 7>,
<&cpu51_intc 7>;
};
- clint_mtimer13: timer@70ac0d0000 {
+ clint_mtimer13: timer@70ac0d4000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac0d0000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac0d4000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu52_intc 7>,
<&cpu53_intc 7>,
<&cpu54_intc 7>,
<&cpu55_intc 7>;
};
- clint_mtimer14: timer@70ac0e0000 {
+ clint_mtimer14: timer@70ac0e4000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac0e0000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac0e4000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu56_intc 7>,
<&cpu57_intc 7>,
<&cpu58_intc 7>,
<&cpu59_intc 7>;
};
- clint_mtimer15: timer@70ac0f0000 {
+ clint_mtimer15: timer@70ac0f4000 {
compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
- reg = <0x00000070 0xac0f0000 0x00000000 0x00007ff8>;
+ reg = <0x00000070 0xac0f4000 0x00000000 0x0000c000>;
+ reg-names = "mtimecmp";
interrupts-extended = <&cpu60_intc 7>,
<&cpu61_intc 7>,
<&cpu62_intc 7>,
--
2.42.1
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 <[email protected]>
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";
};
...
--
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 <[email protected]>
> 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 <[email protected]>
>> 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 <[email protected]>
> 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 <[email protected]>
> --
> 2.42.1
>
On 2023/11/18 15:10, Inochi Amaoto wrote:
> Change the timer layout in the dtb to fit the format that needed by
> the SBI.
>
> Signed-off-by: Inochi Amaoto <[email protected]>
> Fixes: 967a94a92aaa ("riscv: dts: add initial Sophgo SG2042 SoC device tree")
> ---
> arch/riscv/boot/dts/sophgo/sg2042.dtsi | 80 +++++++++++++++-----------
> 1 file changed, 48 insertions(+), 32 deletions(-)
>
> diff --git a/arch/riscv/boot/dts/sophgo/sg2042.dtsi b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> index 93256540d078..ead1cc35d88b 100644
> --- a/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/sg2042.dtsi
> @@ -93,144 +93,160 @@ clint_mswi: interrupt-controller@7094000000 {
> <&cpu63_intc 3>;
> };
>
> - clint_mtimer0: timer@70ac000000 {
> + clint_mtimer0: timer@70ac004000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac000000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac004000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu0_intc 7>,
> <&cpu1_intc 7>,
> <&cpu2_intc 7>,
> <&cpu3_intc 7>;
> };
>
> - clint_mtimer1: timer@70ac010000 {
> + clint_mtimer1: timer@70ac014000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac010000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac014000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu4_intc 7>,
> <&cpu5_intc 7>,
> <&cpu6_intc 7>,
> <&cpu7_intc 7>;
> };
>
> - clint_mtimer2: timer@70ac020000 {
> + clint_mtimer2: timer@70ac024000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac020000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac024000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu8_intc 7>,
> <&cpu9_intc 7>,
> <&cpu10_intc 7>,
> <&cpu11_intc 7>;
> };
>
> - clint_mtimer3: timer@70ac030000 {
> + clint_mtimer3: timer@70ac034000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac030000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac034000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu12_intc 7>,
> <&cpu13_intc 7>,
> <&cpu14_intc 7>,
> <&cpu15_intc 7>;
> };
>
> - clint_mtimer4: timer@70ac040000 {
> + clint_mtimer4: timer@70ac044000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac040000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac044000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu16_intc 7>,
> <&cpu17_intc 7>,
> <&cpu18_intc 7>,
> <&cpu19_intc 7>;
> };
>
> - clint_mtimer5: timer@70ac050000 {
> + clint_mtimer5: timer@70ac054000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac050000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac054000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu20_intc 7>,
> <&cpu21_intc 7>,
> <&cpu22_intc 7>,
> <&cpu23_intc 7>;
> };
>
> - clint_mtimer6: timer@70ac060000 {
> + clint_mtimer6: timer@70ac064000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac060000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac064000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu24_intc 7>,
> <&cpu25_intc 7>,
> <&cpu26_intc 7>,
> <&cpu27_intc 7>;
> };
>
> - clint_mtimer7: timer@70ac070000 {
> + clint_mtimer7: timer@70ac074000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac070000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac074000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu28_intc 7>,
> <&cpu29_intc 7>,
> <&cpu30_intc 7>,
> <&cpu31_intc 7>;
> };
>
> - clint_mtimer8: timer@70ac080000 {
> + clint_mtimer8: timer@70ac084000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac080000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac084000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu32_intc 7>,
> <&cpu33_intc 7>,
> <&cpu34_intc 7>,
> <&cpu35_intc 7>;
> };
>
> - clint_mtimer9: timer@70ac090000 {
> + clint_mtimer9: timer@70ac094000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac090000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac094000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu36_intc 7>,
> <&cpu37_intc 7>,
> <&cpu38_intc 7>,
> <&cpu39_intc 7>;
> };
>
> - clint_mtimer10: timer@70ac0a0000 {
> + clint_mtimer10: timer@70ac0a4000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac0a0000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac0a4000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu40_intc 7>,
> <&cpu41_intc 7>,
> <&cpu42_intc 7>,
> <&cpu43_intc 7>;
> };
>
> - clint_mtimer11: timer@70ac0b0000 {
> + clint_mtimer11: timer@70ac0b4000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac0b0000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac0b4000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu44_intc 7>,
> <&cpu45_intc 7>,
> <&cpu46_intc 7>,
> <&cpu47_intc 7>;
> };
>
> - clint_mtimer12: timer@70ac0c0000 {
> + clint_mtimer12: timer@70ac0c4000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac0c0000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac0c4000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu48_intc 7>,
> <&cpu49_intc 7>,
> <&cpu50_intc 7>,
> <&cpu51_intc 7>;
> };
>
> - clint_mtimer13: timer@70ac0d0000 {
> + clint_mtimer13: timer@70ac0d4000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac0d0000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac0d4000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu52_intc 7>,
> <&cpu53_intc 7>,
> <&cpu54_intc 7>,
> <&cpu55_intc 7>;
> };
>
> - clint_mtimer14: timer@70ac0e0000 {
> + clint_mtimer14: timer@70ac0e4000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac0e0000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac0e4000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu56_intc 7>,
> <&cpu57_intc 7>,
> <&cpu58_intc 7>,
> <&cpu59_intc 7>;
> };
>
> - clint_mtimer15: timer@70ac0f0000 {
> + clint_mtimer15: timer@70ac0f4000 {
> compatible = "sophgo,sg2042-aclint-mtimer", "thead,c900-aclint-mtimer";
> - reg = <0x00000070 0xac0f0000 0x00000000 0x00007ff8>;
> + reg = <0x00000070 0xac0f4000 0x00000000 0x0000c000>;
> + reg-names = "mtimecmp";
> interrupts-extended = <&cpu60_intc 7>,
> <&cpu61_intc 7>,
> <&cpu62_intc 7>,
Reviewed-by: Chen Wang <[email protected]>
> --
> 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 <[email protected]>
> >> 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 <[email protected]>
>>>> 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 <[email protected]>
>>>>> 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 <[email protected]> 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 <[email protected]>
> 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 <[email protected]> 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 <[email protected]>
> > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
>