2022-11-21 04:33:53

by Icenowy Zheng

[permalink] [raw]
Subject: [PATCH 0/3] Some DT binding quirks for T-Head C9xx

These patchset is just full of DT binding patches related to T-Head
C906/C910. These cores now have an open-source fixed-configuration
edition, which enables everyone to explore with them.

The first patch adds an compatible string set for T-Head CLINT, which is
incompatible with SiFive ones by not having a mtime register. The Linux
CLINT driver, which is only used in M mode now, does not support it at
all -- having a DT binding is for OpenSBI.

The second and third patches are for OpenC906, the open source edition
of C906. They try to add some DT binding strings for it.

By the way, as we discussed in the BoufalloLab series, C906/C910 do not
have so many customizable options for PLIC/CLINT, so maybe we should
not add additional per-SoC-system compatible strings and allow only
"thead,c900-*".

Icenowy Zheng (3):
dt-bindings: timer: sifive,clint: add comaptibles for T-Head's C9xx
dt-bindings: timer: sifive,clint: add compatible for OpenC906
dt-bindings: interrupt-controller: sifive,plic: add OpenC906
compatible

.../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 1 +
.../devicetree/bindings/timer/sifive,clint.yaml | 9 +++++++++
2 files changed, 10 insertions(+)

--
2.37.1



2022-11-21 04:38:34

by Icenowy Zheng

[permalink] [raw]
Subject: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

T-Head OpenC906 is a open-source-licensed fixed-configuration of C906,
which is now public and able to be integrated.

Add a compatible for the CLINT shipped as part of OpenC906, which should
just be ordinary C9xx CLINT.

Signed-off-by: Icenowy Zheng <[email protected]>
---
Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
index aada6957216c..86703e995e31 100644
--- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
+++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
@@ -35,6 +35,7 @@ properties:
- const: sifive,clint0
- items:
- enum:
+ - thead,openc906-clint
- allwinner,sun20i-d1-clint
- const: thead,c900-clint
- items:
--
2.37.1


2022-11-21 05:04:31

by Icenowy Zheng

[permalink] [raw]
Subject: [PATCH 1/3] dt-bindings: timer: sifive,clint: add comaptibles for T-Head's C9xx

T-Head C906/C910 CLINT is not compliant to SiFive ones (and even not
compliant to the newcoming ACLINT spec) because of lack of mtime
register.

Add a compatible string formatted like the C9xx-specific PLIC
compatible, and do not allow a SiFive one as fallback because they're
not really compliant.

Signed-off-by: Icenowy Zheng <[email protected]>
---
Documentation/devicetree/bindings/timer/sifive,clint.yaml | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
index bbad24165837..aada6957216c 100644
--- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
+++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
@@ -20,6 +20,10 @@ description:
property of "/cpus" DT node. The "timebase-frequency" DT property is
described in Documentation/devicetree/bindings/riscv/cpus.yaml

+ T-Head C906/C910 CPU cores include an implementation of CLINT too, however
+ their implementation lacks a memory-mapped MTIME register, thus not
+ compatible with SiFive ones.
+
properties:
compatible:
oneOf:
@@ -29,6 +33,10 @@ properties:
- starfive,jh7100-clint
- canaan,k210-clint
- const: sifive,clint0
+ - items:
+ - enum:
+ - allwinner,sun20i-d1-clint
+ - const: thead,c900-clint
- items:
- const: sifive,clint0
- const: riscv,clint0
--
2.37.1


2022-11-21 05:05:06

by Icenowy Zheng

[permalink] [raw]
Subject: [PATCH 3/3] dt-bindings: interrupt-controller: sifive,plic: add OpenC906 compatible

T-Head OpenC906 is a open-source-licensed fixed-configuration of C906,
which is now public and able to be integrated.

Add a compatible for the PLIC shipped as part of OpenC906, which should
just be ordinary C9xx PLIC.

Signed-off-by: Icenowy Zheng <[email protected]>
---
.../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
index 99e01f4d0a69..4d14c5f5c611 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
@@ -65,6 +65,7 @@ properties:
- items:
- enum:
- allwinner,sun20i-d1-plic
+ - thead,openc906-plic
- const: thead,c900-plic
- items:
- const: sifive,plic-1.0.0
--
2.37.1


2022-11-21 10:16:22

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: timer: sifive,clint: add comaptibles for T-Head's C9xx

On 21/11/2022 05:17, Icenowy Zheng wrote:
> T-Head C906/C910 CLINT is not compliant to SiFive ones (and even not
> compliant to the newcoming ACLINT spec) because of lack of mtime
> register.
>
> Add a compatible string formatted like the C9xx-specific PLIC
> compatible, and do not allow a SiFive one as fallback because they're
> not really compliant.
>
> Signed-off-by: Icenowy Zheng <[email protected]>


Acked-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof


2022-11-21 10:32:36

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

On 21/11/2022 05:17, Icenowy Zheng wrote:
> T-Head OpenC906 is a open-source-licensed fixed-configuration of C906,
> which is now public and able to be integrated.
>
> Add a compatible for the CLINT shipped as part of OpenC906, which should
> just be ordinary C9xx CLINT.
>
> Signed-off-by: Icenowy Zheng <[email protected]>
> ---
> Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> index aada6957216c..86703e995e31 100644
> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> @@ -35,6 +35,7 @@ properties:
> - const: sifive,clint0
> - items:
> - enum:
> + - thead,openc906-clint
> - allwinner,sun20i-d1-clint

Add entries sorted alphabetically. This should be squashed with previous
patch.

Best regards,
Krzysztof


2022-11-21 11:04:17

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 3/3] dt-bindings: interrupt-controller: sifive,plic: add OpenC906 compatible

On 21/11/2022 05:17, Icenowy Zheng wrote:
> T-Head OpenC906 is a open-source-licensed fixed-configuration of C906,
> which is now public and able to be integrated.
>
> Add a compatible for the PLIC shipped as part of OpenC906, which should
> just be ordinary C9xx PLIC.
>
> Signed-off-by: Icenowy Zheng <[email protected]>


Acked-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof


2022-11-22 07:58:07

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

On 22/11/2022 08:18, Icenowy Zheng wrote:
> 在 2022-11-21星期一的 11:06 +0100,Krzysztof Kozlowski写道:
>> On 21/11/2022 05:17, Icenowy Zheng wrote:
>>> T-Head OpenC906 is a open-source-licensed fixed-configuration of
>>> C906,
>>> which is now public and able to be integrated.
>>>
>>> Add a compatible for the CLINT shipped as part of OpenC906, which
>>> should
>>> just be ordinary C9xx CLINT.
>>>
>>> Signed-off-by: Icenowy Zheng <[email protected]>
>>> ---
>>>  Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> index aada6957216c..86703e995e31 100644
>>> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> @@ -35,6 +35,7 @@ properties:
>>>            - const: sifive,clint0
>>>        - items:
>>>            - enum:
>>> +              - thead,openc906-clint
>>>                - allwinner,sun20i-d1-clint
>>
>> Add entries sorted alphabetically. This should be squashed with
>> previous
>> patch.
>
> I make it a seperated patch because I think it's a questionable
> approach.
>
> If you think it's okay, I will just squash it and put it as the second
> patch in the next iteration, with adding openc906-plic as the first
> one.

What is a questionable approach? Why commit msg is not saying this?

Best regards,
Krzysztof

2022-11-22 08:16:25

by Icenowy Zheng

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906



于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski <[email protected]> 写到:
>On 22/11/2022 08:18, Icenowy Zheng wrote:
>> 在 2022-11-21星期一的 11:06 +0100,Krzysztof Kozlowski写道:
>>> On 21/11/2022 05:17, Icenowy Zheng wrote:
>>>> T-Head OpenC906 is a open-source-licensed fixed-configuration of
>>>> C906,
>>>> which is now public and able to be integrated.
>>>>
>>>> Add a compatible for the CLINT shipped as part of OpenC906, which
>>>> should
>>>> just be ordinary C9xx CLINT.
>>>>
>>>> Signed-off-by: Icenowy Zheng <[email protected]>
>>>> ---
>>>>  Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>>> b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>>> index aada6957216c..86703e995e31 100644
>>>> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>>> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>>> @@ -35,6 +35,7 @@ properties:
>>>>            - const: sifive,clint0
>>>>        - items:
>>>>            - enum:
>>>> +              - thead,openc906-clint
>>>>                - allwinner,sun20i-d1-clint
>>>
>>> Add entries sorted alphabetically. This should be squashed with
>>> previous
>>> patch.
>>
>> I make it a seperated patch because I think it's a questionable
>> approach.
>>
>> If you think it's okay, I will just squash it and put it as the second
>> patch in the next iteration, with adding openc906-plic as the first
>> one.
>
>What is a questionable approach? Why commit msg is not saying this?

Ah I mentioned it in the cover letter. The problem is just I doubt whether
binding strings for single SoCs are necessary.

>
>Best regards,
>Krzysztof
>

2022-11-22 08:21:06

by Icenowy Zheng

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

在 2022-11-21星期一的 11:06 +0100,Krzysztof Kozlowski写道:
> On 21/11/2022 05:17, Icenowy Zheng wrote:
> > T-Head OpenC906 is a open-source-licensed fixed-configuration of
> > C906,
> > which is now public and able to be integrated.
> >
> > Add a compatible for the CLINT shipped as part of OpenC906, which
> > should
> > just be ordinary C9xx CLINT.
> >
> > Signed-off-by: Icenowy Zheng <[email protected]>
> > ---
> >  Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > index aada6957216c..86703e995e31 100644
> > --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > @@ -35,6 +35,7 @@ properties:
> >            - const: sifive,clint0
> >        - items:
> >            - enum:
> > +              - thead,openc906-clint
> >                - allwinner,sun20i-d1-clint
>
> Add entries sorted alphabetically. This should be squashed with
> previous
> patch.

I make it a seperated patch because I think it's a questionable
approach.

If you think it's okay, I will just squash it and put it as the second
patch in the next iteration, with adding openc906-plic as the first
one.

>
> Best regards,
> Krzysztof
>

2022-11-22 10:15:09

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

On 22/11/2022 08:41, Icenowy Zheng wrote:
>
>
> 于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski <[email protected]> 写到:
>> On 22/11/2022 08:18, Icenowy Zheng wrote:
>>> 在 2022-11-21星期一的 11:06 +0100,Krzysztof Kozlowski写道:
>>>> On 21/11/2022 05:17, Icenowy Zheng wrote:
>>>>> T-Head OpenC906 is a open-source-licensed fixed-configuration of
>>>>> C906,
>>>>> which is now public and able to be integrated.
>>>>>
>>>>> Add a compatible for the CLINT shipped as part of OpenC906, which
>>>>> should
>>>>> just be ordinary C9xx CLINT.
>>>>>
>>>>> Signed-off-by: Icenowy Zheng <[email protected]>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>>>> b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>>>> index aada6957216c..86703e995e31 100644
>>>>> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>>>> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>>>> @@ -35,6 +35,7 @@ properties:
>>>>>            - const: sifive,clint0
>>>>>        - items:
>>>>>            - enum:
>>>>> +              - thead,openc906-clint
>>>>>                - allwinner,sun20i-d1-clint
>>>>
>>>> Add entries sorted alphabetically. This should be squashed with
>>>> previous
>>>> patch.
>>>
>>> I make it a seperated patch because I think it's a questionable
>>> approach.
>>>
>>> If you think it's okay, I will just squash it and put it as the second
>>> patch in the next iteration, with adding openc906-plic as the first
>>> one.
>>
>> What is a questionable approach? Why commit msg is not saying this?
>
> Ah I mentioned it in the cover letter. The problem is just I doubt whether
> binding strings for single SoCs are necessary.
>

There is no question in cover letter. Just some minor remark *at the
end* of it...

If you have questions, be explicit, not force people to grep through
several paragraphs and figure out your concerns.

Best regards,
Krzysztof

2022-11-26 20:37:01

by Samuel Holland

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: timer: sifive,clint: add comaptibles for T-Head's C9xx

On 11/20/22 22:17, Icenowy Zheng wrote:
> T-Head C906/C910 CLINT is not compliant to SiFive ones (and even not
> compliant to the newcoming ACLINT spec) because of lack of mtime
> register.
>
> Add a compatible string formatted like the C9xx-specific PLIC
> compatible, and do not allow a SiFive one as fallback because they're
> not really compliant.
>
> Signed-off-by: Icenowy Zheng <[email protected]>
> ---
> Documentation/devicetree/bindings/timer/sifive,clint.yaml | 8 ++++++++
> 1 file changed, 8 insertions(+)

Reviewed-by: Samuel Holland <[email protected]>

2022-11-27 07:58:56

by Icenowy Zheng

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: timer: sifive,clint: add comaptibles for T-Head's C9xx

在 2022-11-21星期一的 12:17 +0800,Icenowy Zheng写道:
> T-Head C906/C910 CLINT is not compliant to SiFive ones (and even not
> compliant to the newcoming ACLINT spec) because of lack of mtime
> register.
>
> Add a compatible string formatted like the C9xx-specific PLIC
> compatible, and do not allow a SiFive one as fallback because they're
> not really compliant.
>
> Signed-off-by: Icenowy Zheng <[email protected]>

Could this patch get applied individually?

I want to drop the practice of the latter two patches and send new
RFCs, and this patch is a dependency of some OpenSBI patch.

> ---
>  Documentation/devicetree/bindings/timer/sifive,clint.yaml | 8
> ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git
> a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> index bbad24165837..aada6957216c 100644
> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> @@ -20,6 +20,10 @@ description:
>    property of "/cpus" DT node. The "timebase-frequency" DT property
> is
>    described in Documentation/devicetree/bindings/riscv/cpus.yaml
>  
> +  T-Head C906/C910 CPU cores include an implementation of CLINT too,
> however
> +  their implementation lacks a memory-mapped MTIME register, thus
> not
> +  compatible with SiFive ones.
> +
>  properties:
>    compatible:
>      oneOf:
> @@ -29,6 +33,10 @@ properties:
>                - starfive,jh7100-clint
>                - canaan,k210-clint
>            - const: sifive,clint0
> +      - items:
> +          - enum:
> +              - allwinner,sun20i-d1-clint
> +          - const: thead,c900-clint
>        - items:
>            - const: sifive,clint0
>            - const: riscv,clint0

2022-11-30 19:15:05

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

On Tue, Nov 22, 2022 at 03:41:27PM +0800, Icenowy Zheng wrote:
>
>
> 于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski <[email protected]> 写到:
> >On 22/11/2022 08:18, Icenowy Zheng wrote:
> >> 在 2022-11-21星期一的 11:06 +0100,Krzysztof Kozlowski写道:
> >>> On 21/11/2022 05:17, Icenowy Zheng wrote:
> >>>> T-Head OpenC906 is a open-source-licensed fixed-configuration of
> >>>> C906,
> >>>> which is now public and able to be integrated.
> >>>>
> >>>> Add a compatible for the CLINT shipped as part of OpenC906, which
> >>>> should
> >>>> just be ordinary C9xx CLINT.
> >>>>
> >>>> Signed-off-by: Icenowy Zheng <[email protected]>
> >>>> ---
> >>>>  Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
> >>>>  1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> >>>> b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> >>>> index aada6957216c..86703e995e31 100644
> >>>> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> >>>> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> >>>> @@ -35,6 +35,7 @@ properties:
> >>>>            - const: sifive,clint0
> >>>>        - items:
> >>>>            - enum:
> >>>> +              - thead,openc906-clint
> >>>>                - allwinner,sun20i-d1-clint
> >>>
> >>> Add entries sorted alphabetically. This should be squashed with
> >>> previous
> >>> patch.
> >>
> >> I make it a seperated patch because I think it's a questionable
> >> approach.
> >>
> >> If you think it's okay, I will just squash it and put it as the second
> >> patch in the next iteration, with adding openc906-plic as the first
> >> one.
> >
> >What is a questionable approach? Why commit msg is not saying this?
>
> Ah I mentioned it in the cover letter. The problem is just I doubt whether
> binding strings for single SoCs are necessary.

They are.

Unless all the quirks/bugs/features are somehow guaranteed to be exactly
the same as other SoCs sharing the same compatible string, or there is
another mechanism to identify the exact version (e.g. a version
register).

Rob

2022-12-01 19:41:36

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

On Wed, Nov 30, 2022 at 12:13:30PM -0600, Rob Herring wrote:
> On Tue, Nov 22, 2022 at 03:41:27PM +0800, Icenowy Zheng wrote:
> >
> >
> > 于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski <[email protected]> 写到:
> > >On 22/11/2022 08:18, Icenowy Zheng wrote:
> > >> 在 2022-11-21星期一的 11:06 +0100,Krzysztof Kozlowski写道:
> > >>> On 21/11/2022 05:17, Icenowy Zheng wrote:
> > >>>> T-Head OpenC906 is a open-source-licensed fixed-configuration of
> > >>>> C906,
> > >>>> which is now public and able to be integrated.
> > >>>>
> > >>>> Add a compatible for the CLINT shipped as part of OpenC906, which
> > >>>> should
> > >>>> just be ordinary C9xx CLINT.
> > >>>>
> > >>>> Signed-off-by: Icenowy Zheng <[email protected]>
> > >>>> ---
> > >>>>  Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
> > >>>>  1 file changed, 1 insertion(+)
> > >>>>
> > >>>> diff --git
> > >>>> a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > >>>> b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > >>>> index aada6957216c..86703e995e31 100644
> > >>>> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > >>>> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > >>>> @@ -35,6 +35,7 @@ properties:
> > >>>>            - const: sifive,clint0
> > >>>>        - items:
> > >>>>            - enum:
> > >>>> +              - thead,openc906-clint
> > >>>>                - allwinner,sun20i-d1-clint
> > >>>
> > >>> Add entries sorted alphabetically. This should be squashed with
> > >>> previous
> > >>> patch.
> > >>
> > >> I make it a seperated patch because I think it's a questionable
> > >> approach.
> > >>
> > >> If you think it's okay, I will just squash it and put it as the second
> > >> patch in the next iteration, with adding openc906-plic as the first
> > >> one.
> > >
> > >What is a questionable approach? Why commit msg is not saying this?
> >
> > Ah I mentioned it in the cover letter. The problem is just I doubt whether
> > binding strings for single SoCs are necessary.
>
> They are.
>
> Unless all the quirks/bugs/features are somehow guaranteed to be exactly
> the same as other SoCs sharing the same compatible string, or there is
> another mechanism to identify the exact version (e.g. a version
> register).

Icenowy,

Having thought about this a little - are we not *more* likely to see
bug/quirk disparity between implementations of the OpenC906 stuff by
the very nature of being an open-source IP?

Thanks,
Conor.

2022-12-02 07:10:08

by Icenowy Zheng

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

在 2022-12-01星期四的 19:18 +0000,Conor Dooley写道:
> On Wed, Nov 30, 2022 at 12:13:30PM -0600, Rob Herring wrote:
> > On Tue, Nov 22, 2022 at 03:41:27PM +0800, Icenowy Zheng wrote:
> > >
> > >
> > > 于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski
> > > <[email protected]> 写到:
> > > > On 22/11/2022 08:18, Icenowy Zheng wrote:
> > > > > 在 2022-11-21星期一的 11:06 +0100,Krzysztof Kozlowski写道:
> > > > > > On 21/11/2022 05:17, Icenowy Zheng wrote:
> > > > > > > T-Head OpenC906 is a open-source-licensed fixed-
> > > > > > > configuration of
> > > > > > > C906,
> > > > > > > which is now public and able to be integrated.
> > > > > > >
> > > > > > > Add a compatible for the CLINT shipped as part of
> > > > > > > OpenC906, which
> > > > > > > should
> > > > > > > just be ordinary C9xx CLINT.
> > > > > > >
> > > > > > > Signed-off-by: Icenowy Zheng <[email protected]>
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/timer/sifive,clint.yam
> > > > > > > l | 1 +
> > > > > > >  1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git
> > > > > > > a/Documentation/devicetree/bindings/timer/sifive,clint.ya
> > > > > > > ml
> > > > > > > b/Documentation/devicetree/bindings/timer/sifive,clint.ya
> > > > > > > ml
> > > > > > > index aada6957216c..86703e995e31 100644
> > > > > > > ---
> > > > > > > a/Documentation/devicetree/bindings/timer/sifive,clint.ya
> > > > > > > ml
> > > > > > > +++
> > > > > > > b/Documentation/devicetree/bindings/timer/sifive,clint.ya
> > > > > > > ml
> > > > > > > @@ -35,6 +35,7 @@ properties:
> > > > > > >            - const: sifive,clint0
> > > > > > >        - items:
> > > > > > >            - enum:
> > > > > > > +              - thead,openc906-clint
> > > > > > >                - allwinner,sun20i-d1-clint
> > > > > >
> > > > > > Add entries sorted alphabetically. This should be squashed
> > > > > > with
> > > > > > previous
> > > > > > patch.
> > > > >
> > > > > I make it a seperated patch because I think it's a
> > > > > questionable
> > > > > approach.
> > > > >
> > > > > If you think it's okay, I will just squash it and put it as
> > > > > the second
> > > > > patch in the next iteration, with adding openc906-plic as the
> > > > > first
> > > > > one.
> > > >
> > > > What is a questionable approach? Why commit msg is not saying
> > > > this?
> > >
> > > Ah I mentioned it in the cover letter. The problem is just I
> > > doubt whether
> > > binding strings for single SoCs are necessary.
> >
> > They are.
> >
> > Unless all the quirks/bugs/features are somehow guaranteed to be
> > exactly
> > the same as other SoCs sharing the same compatible string, or there
> > is
> > another mechanism to identify the exact version (e.g. a version
> > register).
>
> Icenowy,
>
> Having thought about this a little - are we not *more* likely to see
> bug/quirk disparity between implementations of the OpenC906 stuff by
> the very nature of being an open-source IP?

It's an open-source edition of a specific version of the commercial IP,
a fixed configuration.

In addition, maybe we can just retrieve the version infomation via a T-
Head custom CPU configuration register, mcpuid. Despite the
implementation of this register is weird -- it contains 7 different
read-only values, with the most significant nibble behaving as an
index.

>
> Thanks,
> Conor.
>

2022-12-05 10:53:32

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

On Fri, Dec 02, 2022 at 02:12:54PM +0800, Icenowy Zheng wrote:
> 在 2022-12-01星期四的 19:18 +0000,Conor Dooley写道:
> > On Wed, Nov 30, 2022 at 12:13:30PM -0600, Rob Herring wrote:
> > > On Tue, Nov 22, 2022 at 03:41:27PM +0800, Icenowy Zheng wrote:
> > > >
> > > >
> > > > 于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski
> > > > <[email protected]> 写到:
> > > > > On 22/11/2022 08:18, Icenowy Zheng wrote:
> > > > > > 在 2022-11-21星期一的 11:06 +0100,Krzysztof Kozlowski写道:
> > > > > > > On 21/11/2022 05:17, Icenowy Zheng wrote:
> > > > > > > > T-Head OpenC906 is a open-source-licensed fixed-
> > > > > > > > configuration of
> > > > > > > > C906,
> > > > > > > > which is now public and able to be integrated.
> > > > > > > >
> > > > > > > > Add a compatible for the CLINT shipped as part of
> > > > > > > > OpenC906, which
> > > > > > > > should
> > > > > > > > just be ordinary C9xx CLINT.
> > > > > > > >
> > > > > > > > Signed-off-by: Icenowy Zheng <[email protected]>
> > > > > > > > ---
> > > > > > > >  Documentation/devicetree/bindings/timer/sifive,clint.yam
> > > > > > > > l | 1 +
> > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > >
> > > > > > > > diff --git
> > > > > > > > a/Documentation/devicetree/bindings/timer/sifive,clint.ya
> > > > > > > > ml
> > > > > > > > b/Documentation/devicetree/bindings/timer/sifive,clint.ya
> > > > > > > > ml
> > > > > > > > index aada6957216c..86703e995e31 100644
> > > > > > > > ---
> > > > > > > > a/Documentation/devicetree/bindings/timer/sifive,clint.ya
> > > > > > > > ml
> > > > > > > > +++
> > > > > > > > b/Documentation/devicetree/bindings/timer/sifive,clint.ya
> > > > > > > > ml
> > > > > > > > @@ -35,6 +35,7 @@ properties:
> > > > > > > >            - const: sifive,clint0
> > > > > > > >        - items:
> > > > > > > >            - enum:
> > > > > > > > +              - thead,openc906-clint
> > > > > > > >                - allwinner,sun20i-d1-clint
> > > > > > >
> > > > > > > Add entries sorted alphabetically. This should be squashed
> > > > > > > with
> > > > > > > previous
> > > > > > > patch.
> > > > > >
> > > > > > I make it a seperated patch because I think it's a
> > > > > > questionable
> > > > > > approach.
> > > > > >
> > > > > > If you think it's okay, I will just squash it and put it as
> > > > > > the second
> > > > > > patch in the next iteration, with adding openc906-plic as the
> > > > > > first
> > > > > > one.
> > > > >
> > > > > What is a questionable approach? Why commit msg is not saying
> > > > > this?
> > > >
> > > > Ah I mentioned it in the cover letter. The problem is just I
> > > > doubt whether
> > > > binding strings for single SoCs are necessary.
> > >
> > > They are.
> > >
> > > Unless all the quirks/bugs/features are somehow guaranteed to be
> > > exactly
> > > the same as other SoCs sharing the same compatible string, or there
> > > is
> > > another mechanism to identify the exact version (e.g. a version
> > > register).
> >
> > Icenowy,
> >
> > Having thought about this a little - are we not *more* likely to see
> > bug/quirk disparity between implementations of the OpenC906 stuff by
> > the very nature of being an open-source IP?
>
> It's an open-source edition of a specific version of the commercial IP,
> a fixed configuration.
>
> In addition, maybe we can just retrieve the version infomation via a T-
> Head custom CPU configuration register, mcpuid. Despite the
> implementation of this register is weird -- it contains 7 different
> read-only values, with the most significant nibble behaving as an
> index.

You lot all know the situation here a lot more than I do...
I don't think "letting" people use the bare "thead,c900-foo" makes much
sense as it gives us no chance to deal with quirks down the line.
I don't think that using "thead,openc906-clint", "thead,c900-clint"
makes all that much sense either, in case someone does something wacky
with the open-source version of the core.

That leaves us with either:
"vendor,soc-clint", "thead,openc906-clint", "thead,c900-clint"
or:
"vendor,soc-clint", "thead,c900-clint"
right?

The first one seems like possibly the better option as you'd kinda
expect that, in a perfect word, all of the open-source IP
implementations would share quirks etc?

Thanks,
Conor.


Attachments:
(No filename) (4.40 kB)
signature.asc (235.00 B)
Download all attachments

2022-12-05 11:12:14

by Icenowy Zheng

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

在 2022-12-05星期一的 10:36 +0000,Conor Dooley写道:
> On Fri, Dec 02, 2022 at 02:12:54PM +0800, Icenowy Zheng wrote:
> > 在 2022-12-01星期四的 19:18 +0000,Conor Dooley写道:
> > > On Wed, Nov 30, 2022 at 12:13:30PM -0600, Rob Herring wrote:
> > > > On Tue, Nov 22, 2022 at 03:41:27PM +0800, Icenowy Zheng wrote:
> > > > >
> > > > >
> > > > > 于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski
> > > > > <[email protected]> 写到:
> > > > > > On 22/11/2022 08:18, Icenowy Zheng wrote:
> > > > > > > 在 2022-11-21星期一的 11:06 +0100,Krzysztof Kozlowski写道:
> > > > > > > > On 21/11/2022 05:17, Icenowy Zheng wrote:
> > > > > > > > > T-Head OpenC906 is a open-source-licensed fixed-
> > > > > > > > > configuration of
> > > > > > > > > C906,
> > > > > > > > > which is now public and able to be integrated.
> > > > > > > > >
> > > > > > > > > Add a compatible for the CLINT shipped as part of
> > > > > > > > > OpenC906, which
> > > > > > > > > should
> > > > > > > > > just be ordinary C9xx CLINT.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Icenowy Zheng <[email protected]>
> > > > > > > > > ---
> > > > > > > > >  Documentation/devicetree/bindings/timer/sifive,clint
> > > > > > > > > .yam
> > > > > > > > > l | 1 +
> > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git
> > > > > > > > > a/Documentation/devicetree/bindings/timer/sifive,clin
> > > > > > > > > t.ya
> > > > > > > > > ml
> > > > > > > > > b/Documentation/devicetree/bindings/timer/sifive,clin
> > > > > > > > > t.ya
> > > > > > > > > ml
> > > > > > > > > index aada6957216c..86703e995e31 100644
> > > > > > > > > ---
> > > > > > > > > a/Documentation/devicetree/bindings/timer/sifive,clin
> > > > > > > > > t.ya
> > > > > > > > > ml
> > > > > > > > > +++
> > > > > > > > > b/Documentation/devicetree/bindings/timer/sifive,clin
> > > > > > > > > t.ya
> > > > > > > > > ml
> > > > > > > > > @@ -35,6 +35,7 @@ properties:
> > > > > > > > >            - const: sifive,clint0
> > > > > > > > >        - items:
> > > > > > > > >            - enum:
> > > > > > > > > +              - thead,openc906-clint
> > > > > > > > >                - allwinner,sun20i-d1-clint
> > > > > > > >
> > > > > > > > Add entries sorted alphabetically. This should be
> > > > > > > > squashed
> > > > > > > > with
> > > > > > > > previous
> > > > > > > > patch.
> > > > > > >
> > > > > > > I make it a seperated patch because I think it's a
> > > > > > > questionable
> > > > > > > approach.
> > > > > > >
> > > > > > > If you think it's okay, I will just squash it and put it
> > > > > > > as
> > > > > > > the second
> > > > > > > patch in the next iteration, with adding openc906-plic as
> > > > > > > the
> > > > > > > first
> > > > > > > one.
> > > > > >
> > > > > > What is a questionable approach? Why commit msg is not
> > > > > > saying
> > > > > > this?
> > > > >
> > > > > Ah I mentioned it in the cover letter. The problem is just I
> > > > > doubt whether
> > > > > binding strings for single SoCs are necessary.
> > > >
> > > > They are.
> > > >
> > > > Unless all the quirks/bugs/features are somehow guaranteed to
> > > > be
> > > > exactly
> > > > the same as other SoCs sharing the same compatible string, or
> > > > there
> > > > is
> > > > another mechanism to identify the exact version (e.g. a version
> > > > register).
> > >
> > > Icenowy,
> > >
> > > Having thought about this a little - are we not *more* likely to
> > > see
> > > bug/quirk disparity between implementations of the OpenC906 stuff
> > > by
> > > the very nature of being an open-source IP?
> >
> > It's an open-source edition of a specific version of the commercial
> > IP,
> > a fixed configuration.
> >
> > In addition, maybe we can just retrieve the version infomation via
> > a T-
> > Head custom CPU configuration register, mcpuid. Despite the
> > implementation of this register is weird -- it contains 7 different
> > read-only values, with the most significant nibble behaving as an
> > index.
>
> You lot all know the situation here a lot more than I do...
> I don't think "letting" people use the bare "thead,c900-foo" makes
> much
> sense as it gives us no chance to deal with quirks down the line.

Well, after rechecking the manual, I found it possible to handle quirks
-- T-Head has a custom "mcpuid" CSR (@ RISC-V CSR 0xFC0), which can be
used to retrieve some identification info of the core, including its
model ID, version, etc; and the T-Head PLIC/CLINT are part of their
C906 SoC design that there's another "mapbaddr" CSR that could be used
to retrieve the base address of them.

So I think it okay to just use "thead,c900-clint" here, and when
necessary, try to retrieve mcpuid for dealing with quirks.

> I don't think that using "thead,openc906-clint", "thead,c900-clint"
> makes all that much sense either, in case someone does something
> wacky
> with the open-source version of the core.
>
> That leaves us with either:
> "vendor,soc-clint", "thead,openc906-clint", "thead,c900-clint"
> or:
> "vendor,soc-clint", "thead,c900-clint"
> right?
>
> The first one seems like possibly the better option as you'd kinda
> expect that, in a perfect word, all of the open-source IP
> implementations would share quirks etc?
>
> Thanks,
> Conor.
>

2022-12-05 16:49:12

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

On Mon, Dec 05, 2022 at 07:03:17PM +0800, Icenowy Zheng wrote:
> 在 2022-12-05星期一的 10:36 +0000,Conor Dooley写道:

> > You lot all know the situation here a lot more than I do...
> > I don't think "letting" people use the bare "thead,c900-foo" makes
> > much
> > sense as it gives us no chance to deal with quirks down the line.
>
> Well, after rechecking the manual, I found it possible to handle quirks
> -- T-Head has a custom "mcpuid" CSR (@ RISC-V CSR 0xFC0), which can be
> used to retrieve some identification info of the core, including its
> model ID, version, etc; and the T-Head PLIC/CLINT are part of their
> C906 SoC design that there's another "mapbaddr" CSR that could be used
> to retrieve the base address of them.
>
> So I think it okay to just use "thead,c900-clint" here, and when
> necessary, try to retrieve mcpuid for dealing with quirks.

I'm not super sure I follow. What's the relevance of "mapbaddr" here?
We've got a reg property, so I don't think we need "mapbaddr"?

For "mcpuid", can you be sure that implementers will not omit setting
that value to something unique? I'd be happier if we were overly clear
now rather than have some headaches later. Have I missed something?

> > I don't think that using "thead,openc906-clint", "thead,c900-clint"
> > makes all that much sense either, in case someone does something
> > wacky
> > with the open-source version of the core.
> >
> > That leaves us with either:
> > "vendor,soc-clint", "thead,openc906-clint", "thead,c900-clint"
> > or:
> > "vendor,soc-clint", "thead,c900-clint"
> > right?
> >
> > The first one seems like possibly the better option as you'd kinda
> > expect that, in a perfect word, all of the open-source IP
> > implementations would share quirks etc?


Attachments:
(No filename) (1.77 kB)
signature.asc (235.00 B)
Download all attachments

2022-12-05 17:15:17

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906



On 5 December 2022 15:59:44 GMT, Icenowy Zheng <[email protected]> wrote:
>在 2022-12-05星期一的 15:05 +0000,Conor Dooley写道:
>> On Mon, Dec 05, 2022 at 07:03:17PM +0800, Icenowy Zheng wrote:
>> > 在 2022-12-05星期一的 10:36 +0000,Conor Dooley写道:
>>
>> > > You lot all know the situation here a lot more than I do...
>> > > I don't think "letting" people use the bare "thead,c900-foo"
>> > > makes
>> > > much
>> > > sense as it gives us no chance to deal with quirks down the line.
>> >
>> > Well, after rechecking the manual, I found it possible to handle
>> > quirks
>> > -- T-Head has a custom "mcpuid" CSR (@ RISC-V CSR 0xFC0), which can
>> > be
>> > used to retrieve some identification info of the core, including
>> > its
>> > model ID, version, etc; and the T-Head PLIC/CLINT are part of their
>> > C906 SoC design that there's another "mapbaddr" CSR that could be
>> > used
>> > to retrieve the base address of them.
>> >
>> > So I think it okay to just use "thead,c900-clint" here, and when
>> > necessary, try to retrieve mcpuid for dealing with quirks.
>>
>> I'm not super sure I follow. What's the relevance of "mapbaddr" here?
>> We've got a reg property, so I don't think we need "mapbaddr"?
>
>Yes, it's not relevant to us here, it's only to prove that PLIC/CLINT
>is part of C906 "Core Complex".
>
>>
>> For "mcpuid", can you be sure that implementers will not omit setting
>> that value to something unique? I'd be happier if we were overly
>> clear
>> now rather than have some headaches later. Have I missed something?
>
>These values are set by T-Head instead of individual SoC implementers
>as a CPU CSR, and it's not for uniqueness, but it's for identification
>of the CPU core revision (thus the PLIC/CLINT that come with it).

I really am missing something here that must be obvious to you.
Let me try and explain where my gap in understanding is.
If someone takes the open cores & makes a minor tweak in the plic how does knowing mcpuid help us identify that that plic is marginally different?

I must have missed something that should be apparent and look like an eejit right now!

>
>>
>> > > I don't think that using "thead,openc906-clint", "thead,c900-
>> > > clint"
>> > > makes all that much sense either, in case someone does something
>> > > wacky
>> > > with the open-source version of the core.
>> > >
>> > > That leaves us with either:
>> > > "vendor,soc-clint", "thead,openc906-clint", "thead,c900-clint"
>> > > or:
>> > > "vendor,soc-clint", "thead,c900-clint"
>> > > right?
>> > >
>> > > The first one seems like possibly the better option as you'd
>> > > kinda
>> > > expect that, in a perfect word, all of the open-source IP
>> > > implementations would share quirks etc?
>>
>

2022-12-05 17:19:08

by Icenowy Zheng

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

在 2022-12-05星期一的 15:05 +0000,Conor Dooley写道:
> On Mon, Dec 05, 2022 at 07:03:17PM +0800, Icenowy Zheng wrote:
> > 在 2022-12-05星期一的 10:36 +0000,Conor Dooley写道:
>
> > > You lot all know the situation here a lot more than I do...
> > > I don't think "letting" people use the bare "thead,c900-foo"
> > > makes
> > > much
> > > sense as it gives us no chance to deal with quirks down the line.
> >
> > Well, after rechecking the manual, I found it possible to handle
> > quirks
> > -- T-Head has a custom "mcpuid" CSR (@ RISC-V CSR 0xFC0), which can
> > be
> > used to retrieve some identification info of the core, including
> > its
> > model ID, version, etc; and the T-Head PLIC/CLINT are part of their
> > C906 SoC design that there's another "mapbaddr" CSR that could be
> > used
> > to retrieve the base address of them.
> >
> > So I think it okay to just use "thead,c900-clint" here, and when
> > necessary, try to retrieve mcpuid for dealing with quirks.
>
> I'm not super sure I follow. What's the relevance of "mapbaddr" here?
> We've got a reg property, so I don't think we need "mapbaddr"?

Yes, it's not relevant to us here, it's only to prove that PLIC/CLINT
is part of C906 "Core Complex".

>
> For "mcpuid", can you be sure that implementers will not omit setting
> that value to something unique? I'd be happier if we were overly
> clear
> now rather than have some headaches later. Have I missed something?

These values are set by T-Head instead of individual SoC implementers
as a CPU CSR, and it's not for uniqueness, but it's for identification
of the CPU core revision (thus the PLIC/CLINT that come with it).

>
> > > I don't think that using "thead,openc906-clint", "thead,c900-
> > > clint"
> > > makes all that much sense either, in case someone does something
> > > wacky
> > > with the open-source version of the core.
> > >
> > > That leaves us with either:
> > > "vendor,soc-clint", "thead,openc906-clint", "thead,c900-clint"
> > > or:
> > > "vendor,soc-clint", "thead,c900-clint"
> > > right?
> > >
> > > The first one seems like possibly the better option as you'd
> > > kinda
> > > expect that, in a perfect word, all of the open-source IP
> > > implementations would share quirks etc?
>

2022-12-06 04:10:53

by Icenowy Zheng

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906

在 2022-12-05星期一的 16:54 +0000,Conor Dooley写道:
>
>
> On 5 December 2022 15:59:44 GMT, Icenowy Zheng <[email protected]>
> wrote:
> > 在 2022-12-05星期一的 15:05 +0000,Conor Dooley写道:
> > > On Mon, Dec 05, 2022 at 07:03:17PM +0800, Icenowy Zheng wrote:
> > > > 在 2022-12-05星期一的 10:36 +0000,Conor Dooley写道:
> > >
> > > > > You lot all know the situation here a lot more than I do...
> > > > > I don't think "letting" people use the bare "thead,c900-foo"
> > > > > makes
> > > > > much
> > > > > sense as it gives us no chance to deal with quirks down the
> > > > > line.
> > > >
> > > > Well, after rechecking the manual, I found it possible to
> > > > handle
> > > > quirks
> > > > -- T-Head has a custom "mcpuid" CSR (@ RISC-V CSR 0xFC0), which
> > > > can
> > > > be
> > > > used to retrieve some identification info of the core,
> > > > including
> > > > its
> > > > model ID, version, etc; and the T-Head PLIC/CLINT are part of
> > > > their
> > > > C906 SoC design that there's another "mapbaddr" CSR that could
> > > > be
> > > > used
> > > > to retrieve the base address of them.
> > > >
> > > > So I think it okay to just use "thead,c900-clint" here, and
> > > > when
> > > > necessary, try to retrieve mcpuid for dealing with quirks.
> > >
> > > I'm not super sure I follow. What's the relevance of "mapbaddr"
> > > here?
> > > We've got a reg property, so I don't think we need "mapbaddr"?
> >
> > Yes, it's not relevant to us here, it's only to prove that
> > PLIC/CLINT
> > is part of C906 "Core Complex".
> >
> > >
> > > For "mcpuid", can you be sure that implementers will not omit
> > > setting
> > > that value to something unique? I'd be happier if we were overly
> > > clear
> > > now rather than have some headaches later. Have I missed
> > > something?
> >
> > These values are set by T-Head instead of individual SoC
> > implementers
> > as a CPU CSR, and it's not for uniqueness, but it's for
> > identification
> > of the CPU core revision (thus the PLIC/CLINT that come with it).
>
> I really am missing something here that must be obvious to you.
> Let me try and explain where my gap in understanding is.
> If someone takes the open cores & makes a minor tweak in the plic how
> does knowing mcpuid help us identify that that plic is marginally
> different?

No, but my point is that in this situation we shouldn't use C900
compatible at all because it's no longer the vanilla C900 cores.

My assumption is that the same IP cores are the same unless specially
customized.

>
> I must have missed something that should be apparent and look like an
> eejit right now!
>
> >
> > >
> > > > > I don't think that using "thead,openc906-clint", "thead,c900-
> > > > > clint"
> > > > > makes all that much sense either, in case someone does
> > > > > something
> > > > > wacky
> > > > > with the open-source version of the core.
> > > > >
> > > > > That leaves us with either:
> > > > > "vendor,soc-clint", "thead,openc906-clint", "thead,c900-
> > > > > clint"
> > > > > or:
> > > > > "vendor,soc-clint", "thead,c900-clint"
> > > > > right?
> > > > >
> > > > > The first one seems like possibly the better option as you'd
> > > > > kinda
> > > > > expect that, in a perfect word, all of the open-source IP
> > > > > implementations would share quirks etc?
> > >
> >

2022-12-06 07:15:28

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH 2/3] dt-bindings: timer: sifive,clint: add compatible for OpenC906



On 6 December 2022 03:46:11 GMT, Icenowy Zheng <[email protected]> wrote:
>在 2022-12-05星期一的 16:54 +0000,Conor Dooley写道:
>>
>>
>> On 5 December 2022 15:59:44 GMT, Icenowy Zheng <[email protected]>
>> wrote:
>> > 在 2022-12-05星期一的 15:05 +0000,Conor Dooley写道:
>> > > On Mon, Dec 05, 2022 at 07:03:17PM +0800, Icenowy Zheng wrote:
>> > > > 在 2022-12-05星期一的 10:36 +0000,Conor Dooley写道:
>> > >
>> > > > > You lot all know the situation here a lot more than I do...
>> > > > > I don't think "letting" people use the bare "thead,c900-foo"
>> > > > > makes
>> > > > > much
>> > > > > sense as it gives us no chance to deal with quirks down the
>> > > > > line.
>> > > >
>> > > > Well, after rechecking the manual, I found it possible to
>> > > > handle
>> > > > quirks
>> > > > -- T-Head has a custom "mcpuid" CSR (@ RISC-V CSR 0xFC0), which
>> > > > can
>> > > > be
>> > > > used to retrieve some identification info of the core,
>> > > > including
>> > > > its
>> > > > model ID, version, etc; and the T-Head PLIC/CLINT are part of
>> > > > their
>> > > > C906 SoC design that there's another "mapbaddr" CSR that could
>> > > > be
>> > > > used
>> > > > to retrieve the base address of them.
>> > > >
>> > > > So I think it okay to just use "thead,c900-clint" here, and
>> > > > when
>> > > > necessary, try to retrieve mcpuid for dealing with quirks.
>> > >
>> > > I'm not super sure I follow. What's the relevance of "mapbaddr"
>> > > here?
>> > > We've got a reg property, so I don't think we need "mapbaddr"?
>> >
>> > Yes, it's not relevant to us here, it's only to prove that
>> > PLIC/CLINT
>> > is part of C906 "Core Complex".
>> >
>> > >
>> > > For "mcpuid", can you be sure that implementers will not omit
>> > > setting
>> > > that value to something unique? I'd be happier if we were overly
>> > > clear
>> > > now rather than have some headaches later. Have I missed
>> > > something?
>> >
>> > These values are set by T-Head instead of individual SoC
>> > implementers
>> > as a CPU CSR, and it's not for uniqueness, but it's for
>> > identification
>> > of the CPU core revision (thus the PLIC/CLINT that come with it).
>>
>> I really am missing something here that must be obvious to you.
>> Let me try and explain where my gap in understanding is.
>> If someone takes the open cores & makes a minor tweak in the plic how
>> does knowing mcpuid help us identify that that plic is marginally
>> different?
>
>No, but my point is that in this situation we shouldn't use C900
>compatible at all because it's no longer the vanilla C900 cores.
>
>My assumption is that the same IP cores are the same unless specially
>customized.

Ah see that is assuming people get it right.
I've been in the mindset of "what if the difference is only noticed after the DT has been shipped".
I guess that's where we've been at odds.

>
>>
>> I must have missed something that should be apparent and look like an
>> eejit right now!
>>
>> >
>> > >
>> > > > > I don't think that using "thead,openc906-clint", "thead,c900-
>> > > > > clint"
>> > > > > makes all that much sense either, in case someone does
>> > > > > something
>> > > > > wacky
>> > > > > with the open-source version of the core.
>> > > > >
>> > > > > That leaves us with either:
>> > > > > "vendor,soc-clint", "thead,openc906-clint", "thead,c900-
>> > > > > clint"
>> > > > > or:
>> > > > > "vendor,soc-clint", "thead,c900-clint"
>> > > > > right?
>> > > > >
>> > > > > The first one seems like possibly the better option as you'd
>> > > > > kinda
>> > > > > expect that, in a perfect word, all of the open-source IP
>> > > > > implementations would share quirks etc?
>> > >
>> >
>

2022-12-07 11:06:35

by Icenowy Zheng

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: timer: sifive,clint: add comaptibles for T-Head's C9xx

在 2022-11-21星期一的 12:17 +0800,Icenowy Zheng写道:
> T-Head C906/C910 CLINT is not compliant to SiFive ones (and even not
> compliant to the newcoming ACLINT spec) because of lack of mtime
> register.
>
> Add a compatible string formatted like the C9xx-specific PLIC
> compatible, and do not allow a SiFive one as fallback because they're
> not really compliant.
>
> Signed-off-by: Icenowy Zheng <[email protected]>

Hi, could this patch be picked ASAP? Becuase it will be used then in
further OpenSBI patches to enable proper operation of T-Head timer.

I know the following 2 patches are in doubt and further rework for them
are needed.

> ---
>  Documentation/devicetree/bindings/timer/sifive,clint.yaml | 8
> ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git
> a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> index bbad24165837..aada6957216c 100644
> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> @@ -20,6 +20,10 @@ description:
>    property of "/cpus" DT node. The "timebase-frequency" DT property
> is
>    described in Documentation/devicetree/bindings/riscv/cpus.yaml
>  
> +  T-Head C906/C910 CPU cores include an implementation of CLINT too,
> however
> +  their implementation lacks a memory-mapped MTIME register, thus
> not
> +  compatible with SiFive ones.
> +
>  properties:
>    compatible:
>      oneOf:
> @@ -29,6 +33,10 @@ properties:
>                - starfive,jh7100-clint
>                - canaan,k210-clint
>            - const: sifive,clint0
> +      - items:
> +          - enum:
> +              - allwinner,sun20i-d1-clint
> +          - const: thead,c900-clint
>        - items:
>            - const: sifive,clint0
>            - const: riscv,clint0

2022-12-07 12:18:48

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH 1/3] dt-bindings: timer: sifive,clint: add comaptibles for T-Head's C9xx

On Wed, Dec 07, 2022 at 06:47:26PM +0800, Icenowy Zheng wrote:
> 在 2022-11-21星期一的 12:17 +0800,Icenowy Zheng写道:
> > T-Head C906/C910 CLINT is not compliant to SiFive ones (and even not
> > compliant to the newcoming ACLINT spec) because of lack of mtime
> > register.
> >
> > Add a compatible string formatted like the C9xx-specific PLIC
> > compatible, and do not allow a SiFive one as fallback because they're
> > not really compliant.
> >
> > Signed-off-by: Icenowy Zheng <[email protected]>
>
> Hi, could this patch be picked ASAP? Becuase it will be used then in
> further OpenSBI patches to enable proper operation of T-Head timer.
>
> I know the following 2 patches are in doubt and further rework for them
> are needed.

Since it's me that's asking the questions about the other patches, but
have no comments about this particular one:
Acked-by: Conor Dooley <[email protected]>

HTH Icenowy!

> > ---
> >  Documentation/devicetree/bindings/timer/sifive,clint.yaml | 8
> > ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > index bbad24165837..aada6957216c 100644
> > --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > @@ -20,6 +20,10 @@ description:
> >    property of "/cpus" DT node. The "timebase-frequency" DT property
> > is
> >    described in Documentation/devicetree/bindings/riscv/cpus.yaml
> >  
> > +  T-Head C906/C910 CPU cores include an implementation of CLINT too,
> > however
> > +  their implementation lacks a memory-mapped MTIME register, thus
> > not
> > +  compatible with SiFive ones.
> > +
> >  properties:
> >    compatible:
> >      oneOf:
> > @@ -29,6 +33,10 @@ properties:
> >                - starfive,jh7100-clint
> >                - canaan,k210-clint
> >            - const: sifive,clint0
> > +      - items:
> > +          - enum:
> > +              - allwinner,sun20i-d1-clint
> > +          - const: thead,c900-clint
> >        - items:
> >            - const: sifive,clint0
> >            - const: riscv,clint0
>
>


Attachments:
(No filename) (2.34 kB)
signature.asc (235.00 B)
Download all attachments