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

Message ID 20221121041757.418645-3-uwu@icenowy.me
State New
Headers
Series Some DT binding quirks for T-Head C9xx |

Commit Message

Icenowy Zheng Nov. 21, 2022, 4:17 a.m. UTC
  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 <uwu@icenowy.me>
---
 Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
 1 file changed, 1 insertion(+)
  

Comments

Krzysztof Kozlowski Nov. 21, 2022, 10:06 a.m. UTC | #1
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 <uwu@icenowy.me>
> ---
>  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
  
Icenowy Zheng Nov. 22, 2022, 7:18 a.m. UTC | #2
在 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 <uwu@icenowy.me>
> > ---
> >  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
>
  
Krzysztof Kozlowski Nov. 22, 2022, 7:35 a.m. UTC | #3
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 <uwu@icenowy.me>
>>> ---
>>>  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
  
Icenowy Zheng Nov. 22, 2022, 7:41 a.m. UTC | #4
于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 写到:
>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 <uwu@icenowy.me>
>>>> ---
>>>>  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
>
  
Krzysztof Kozlowski Nov. 22, 2022, 8:47 a.m. UTC | #5
On 22/11/2022 08:41, Icenowy Zheng wrote:
> 
> 
> 于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 写到:
>> 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 <uwu@icenowy.me>
>>>>> ---
>>>>>  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
  
Rob Herring Nov. 30, 2022, 6:13 p.m. UTC | #6
On Tue, Nov 22, 2022 at 03:41:27PM +0800, Icenowy Zheng wrote:
> 
> 
> 于 2022年11月22日 GMT+08:00 下午3:35:48, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 写到:
> >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 <uwu@icenowy.me>
> >>>> ---
> >>>>  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
  
Conor Dooley Dec. 1, 2022, 7:18 p.m. UTC | #7
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 <krzysztof.kozlowski@linaro.org> 写到:
> > >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 <uwu@icenowy.me>
> > >>>> ---
> > >>>>  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.
  
Icenowy Zheng Dec. 2, 2022, 6:12 a.m. UTC | #8
在 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
> > > <krzysztof.kozlowski@linaro.org> 写到:
> > > > 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 <uwu@icenowy.me>
> > > > > > > ---
> > > > > > >  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.
>
  
Conor Dooley Dec. 5, 2022, 10:36 a.m. UTC | #9
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
> > > > <krzysztof.kozlowski@linaro.org> 写到:
> > > > > 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 <uwu@icenowy.me>
> > > > > > > > ---
> > > > > > > >  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.
  
Icenowy Zheng Dec. 5, 2022, 11:03 a.m. UTC | #10
在 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
> > > > > <krzysztof.kozlowski@linaro.org> 写到:
> > > > > > 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 <uwu@icenowy.me>
> > > > > > > > > ---
> > > > > > > > >  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.
>
  
Conor Dooley Dec. 5, 2022, 3:05 p.m. UTC | #11
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?
  
Icenowy Zheng Dec. 5, 2022, 3:59 p.m. UTC | #12
在 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?
>
  
Conor Dooley Dec. 5, 2022, 4:54 p.m. UTC | #13
On 5 December 2022 15:59:44 GMT, Icenowy Zheng <uwu@icenowy.me> 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?
>> 
>
  
Icenowy Zheng Dec. 6, 2022, 3:46 a.m. UTC | #14
在 2022-12-05星期一的 16:54 +0000,Conor Dooley写道:
> 
> 
> On 5 December 2022 15:59:44 GMT, Icenowy Zheng <uwu@icenowy.me>
> 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?
> > > 
> >
  
Conor Dooley Dec. 6, 2022, 6:33 a.m. UTC | #15
On 6 December 2022 03:46:11 GMT, Icenowy Zheng <uwu@icenowy.me> wrote:
>在 2022-12-05星期一的 16:54 +0000,Conor Dooley写道:
>> 
>> 
>> On 5 December 2022 15:59:44 GMT, Icenowy Zheng <uwu@icenowy.me>
>> 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?
>> > > 
>> > 
>
  

Patch

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: