[v2,06/11] dt-bindings: timer: Add Sophgo sg2042 clint

  From: Inochi Amaoto <inochiama@outlook.com>

Add two new compatible string formatted like `C9xx-clint-xxx` to identify
the timer and ipi device separately, and do not allow c900-clint as the
fallback to avoid conflict.

Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
Signed-off-by: Chen Wang <wangchen20@iscas.ac.cn>
---
 Documentation/devicetree/bindings/timer/sifive,clint.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)
  

On Wed, Sep 20, 2023 at 02:39:39PM +0800, Chen Wang wrote:
> From: Inochi Amaoto <inochiama@outlook.com>
> 
> Add two new compatible string formatted like `C9xx-clint-xxx` to identify
> the timer and ipi device separately, and do not allow c900-clint as the
> fallback to avoid conflict.
> 
> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
> Signed-off-by: Chen Wang <wangchen20@iscas.ac.cn>

Have you ignored Krzysztof's comments on this? I don't see a response or
a reaction to his comments about the compatibles on the last version.
Additionally, where is the user for these? I don't see any drivers that
actually make use of these.

Why do you need to have 2 compatibles (and therefore 2 devices) for the
clint? I thought the clint was a single device, of which the mtimer and
mswi bits were just "features"? Having split register ranges isn't a
reason to have two compatibles, so I must be missing something here...

Thanks,
Conor.

> ---
>  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 a0185e15a42f..ae69696c5c75 100644
> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> @@ -39,6 +39,14 @@ properties:
>                - allwinner,sun20i-d1-clint
>                - thead,th1520-clint
>            - const: thead,c900-clint
> +      - items:
> +          - enum:
> +              - sophgo,sg2042-clint-mtimer
> +          - const: thead,c900-clint-mtimer
> +      - items:
> +          - enum:
> +              - sophgo,sg2042-clint-mswi
> +          - const: thead,c900-clint-mswi
>        - items:
>            - const: sifive,clint0
>            - const: riscv,clint0
> -- 
> 2.25.1
>
  

>On Wed, Sep 20, 2023 at 02:39:39PM +0800, Chen Wang wrote:
>> From: Inochi Amaoto <inochiama@outlook.com>
>>
>> Add two new compatible string formatted like `C9xx-clint-xxx` to identify
>> the timer and ipi device separately, and do not allow c900-clint as the
>> fallback to avoid conflict.
>>
>> Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
>> Signed-off-by: Chen Wang <wangchen20@iscas.ac.cn>
>
>Have you ignored Krzysztof's comments on this? I don't see a response or
>a reaction to his comments about the compatibles on the last version.
>Additionally, where is the user for these? I don't see any drivers that
>actually make use of these.
>

Sorry for late reply and wrong message-id.

The clint is parsed by sbi. As use the same compatible, the opensbi will
parse the device twice. This will cause a fault.

>Why do you need to have 2 compatibles (and therefore 2 devices) for the
>clint? I thought the clint was a single device, of which the mtimer and
>mswi bits were just "features"? Having split register ranges isn't a
>reason to have two compatibles, so I must be missing something here...
>
>Thanks,
>Conor.
>

Sorry for late reply, The clint consists of mtimer and ipi devices, which
is defined in [1]. This standard shows clint(or the aclint) has two device,
but not one. In another word, there is no need to defined mtimer and ipi
device on the same base address. So we need two compatibles to allow sbi
to identify them correctly.

[1] https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc

>> ---
>>  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 a0185e15a42f..ae69696c5c75 100644
>> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> @@ -39,6 +39,14 @@ properties:
>>                - allwinner,sun20i-d1-clint
>>                - thead,th1520-clint
>>            - const: thead,c900-clint
>> +      - items:
>> +          - enum:
>> +              - sophgo,sg2042-clint-mtimer
>> +          - const: thead,c900-clint-mtimer
>> +      - items:
>> +          - enum:
>> +              - sophgo,sg2042-clint-mswi
>> +          - const: thead,c900-clint-mswi
>>        - items:
>>            - const: sifive,clint0
>>            - const: riscv,clint0
>> --
>> 2.25.1
>>
>
  

On 20/09/2023 08:39, Chen Wang wrote:
> From: Inochi Amaoto <inochiama@outlook.com>
> 
> Add two new compatible string formatted like `C9xx-clint-xxx` to identify
> the timer and ipi device separately, and do not allow c900-clint as the

Why?

You received comment about it, so please provide proper explanation in
the commit msg.

Same device does not get two different compatibles.

You also did not respond to my comments, so you basically ignored it and
send the same.

NAK

Best regards,
Krzysztof
  

>On 20/09/2023 08:39, Chen Wang wrote:
>> From: Inochi Amaoto <inochiama@outlook.com>
>>
>> Add two new compatible string formatted like `C9xx-clint-xxx` to identify
>> the timer and ipi device separately, and do not allow c900-clint as the
>
>Why?
>

If use the same compatible, SBI will process this twice in both ipi and
timer, use different compatible will allow SBI to treat these as different.
AFAIK, the aclint in SBI use the same concepts, which make hard to use the
second register range. I have explained in another response.

https://lore.kernel.org/all/IA1PR20MB495313B7E9B2FC529BE0BB2ABBF9A@IA1PR20MB4953.namprd20.prod.outlook.com/

>You received comment about it, so please provide proper explanation in
>the commit msg.
>
>Same device does not get two different compatibles.
>
>You also did not respond to my comments, so you basically ignored it and
>send the same.
>
>NAK
>
>Best regards,
>Krzysztof
>

Sorry for this, as I mistake the idea of the last message. All of this
will be fixed in the next patch.
  

On 20/09/2023 14:15, Inochi Amaoto wrote:
>> On 20/09/2023 08:39, Chen Wang wrote:
>>> From: Inochi Amaoto <inochiama@outlook.com>
>>>
>>> Add two new compatible string formatted like `C9xx-clint-xxx` to identify
>>> the timer and ipi device separately, and do not allow c900-clint as the
>>
>> Why?
>>
> 
> If use the same compatible, SBI will process this twice in both ipi and
> timer, use different compatible will allow SBI to treat these as different.
> AFAIK, the aclint in SBI use the same concepts, which make hard to use the
> second register range. I have explained in another response.

What is a SBI? Linux driver? If so, why some intermediate Linux driver
choice should affect bindings?
Best regards,
Krzysztof
  

>On 20/09/2023 14:15, Inochi Amaoto wrote:
>>> On 20/09/2023 08:39, Chen Wang wrote:
>>>> From: Inochi Amaoto <inochiama@outlook.com>
>>>>
>>>> Add two new compatible string formatted like `C9xx-clint-xxx` to identify
>>>> the timer and ipi device separately, and do not allow c900-clint as the
>>>
>>> Why?
>>>
>>
>> If use the same compatible, SBI will process this twice in both ipi and
>> timer, use different compatible will allow SBI to treat these as different.
>> AFAIK, the aclint in SBI use the same concepts, which make hard to use the
>> second register range. I have explained in another response.
>
>What is a SBI? Linux driver? If so, why some intermediate Linux driver
>choice should affect bindings?
>Best regards,
>Krzysztof
>

SBI (Supervisor Binary Interface) is defined by riscv, which is an interface
between the Supervisor Execution Environment (SEE) and the supervisor. The
detailed documentation can be found in [1].

The implement of SBI needs fdt info of the platform, which is provided by
kernel. So we need a dt-bindings for these devices, and these will be
processed by SBI.

[1] https://github.com/riscv-non-isa/riscv-sbi-doc
  

On 20/09/2023 14:58, Conor Dooley wrote:
> On Wed, Sep 20, 2023 at 08:40:07PM +0800, Inochi Amaoto wrote:
>>> On 20/09/2023 14:15, Inochi Amaoto wrote:
>>>>> On 20/09/2023 08:39, Chen Wang wrote:
>>>>>> From: Inochi Amaoto <inochiama@outlook.com>
>>>>>>
>>>>>> Add two new compatible string formatted like `C9xx-clint-xxx` to identify
>>>>>> the timer and ipi device separately, and do not allow c900-clint as the
>>>>>
>>>>> Why?
>>>>>
>>>>
>>>> If use the same compatible, SBI will process this twice in both ipi and
>>>> timer, use different compatible will allow SBI to treat these as different.
>>>> AFAIK, the aclint in SBI use the same concepts, which make hard to use the
>>>> second register range. I have explained in another response.
>>>
>>> What is a SBI? Linux driver? If so, why some intermediate Linux driver
>>> choice should affect bindings?
>>> Best regards,
>>> Krzysztof
>>>
>>
>> SBI (Supervisor Binary Interface) is defined by riscv, which is an interface
>> between the Supervisor Execution Environment (SEE) and the supervisor. The
>> detailed documentation can be found in [1].
>>
>> The implement of SBI needs fdt info of the platform, which is provided by
>> kernel. So we need a dt-bindings for these devices, and these will be
>> processed by SBI.
>>
>> [1] https://github.com/riscv-non-isa/riscv-sbi-doc
> 
> Yeah, this is the unfortunate problem of half-baked bindings (IMO)
> ending up in OpenSBI (which likely means they also ended up in QEMU).
> This T-Head stuff is coming across our (metaphorical) desks, so we are
> obviously going to try to do things correctly. I may end up speaking to
> Anup later today, if I do I will point him at this thread (if he hasn't
> seen it already).

If the OpenSBI started to work with some half-baked-bindings before
proper review, it's their loss. If we do not push back on such stuff,
how our review can ever matter?

Anyway, firmware/SBI/whatever parsing compatible twice is not really a
reason to model same thing with two different compatibles. Assuming of
course this is the same thing.

Best regards,
Krzysztof
  

On Wed, Sep 20, 2023 at 6:28 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Sep 20, 2023 at 08:40:07PM +0800, Inochi Amaoto wrote:
> > >On 20/09/2023 14:15, Inochi Amaoto wrote:
> > >>> On 20/09/2023 08:39, Chen Wang wrote:
> > >>>> From: Inochi Amaoto <inochiama@outlook.com>
> > >>>>
> > >>>> Add two new compatible string formatted like `C9xx-clint-xxx` to identify
> > >>>> the timer and ipi device separately, and do not allow c900-clint as the
> > >>>
> > >>> Why?
> > >>>
> > >>
> > >> If use the same compatible, SBI will process this twice in both ipi and
> > >> timer, use different compatible will allow SBI to treat these as different.
> > >> AFAIK, the aclint in SBI use the same concepts, which make hard to use the
> > >> second register range. I have explained in another response.
> > >
> > >What is a SBI? Linux driver? If so, why some intermediate Linux driver
> > >choice should affect bindings?
> > >Best regards,
> > >Krzysztof
> > >
> >
> > SBI (Supervisor Binary Interface) is defined by riscv, which is an interface
> > between the Supervisor Execution Environment (SEE) and the supervisor. The
> > detailed documentation can be found in [1].
> >
> > The implement of SBI needs fdt info of the platform, which is provided by
> > kernel. So we need a dt-bindings for these devices, and these will be
> > processed by SBI.
> >
> > [1] https://github.com/riscv-non-isa/riscv-sbi-doc
>
> Yeah, this is the unfortunate problem of half-baked bindings (IMO)
> ending up in OpenSBI (which likely means they also ended up in QEMU).
> This T-Head stuff is coming across our (metaphorical) desks, so we are
> obviously going to try to do things correctly. I may end up speaking to
> Anup later today, if I do I will point him at this thread (if he hasn't
> seen it already).

RISC-V ACLINT is one of those unfortunate non-ISA specs (like
SiFive PLIC) which is implemented by various organizations but
not officially ratified by RVI.

The SiFive CLINT has flexibility related limitations which makes it
not useful for multi-socket and mult-die systems. The SiFive CLINT
is also not useful for systems with AIA because with AIA M-mode has
a new way of doing M-mode IPIs. Due to this reasons, the RISC-V
ACLINT spec breaks down traditional SiFive CLINT into two separate
devices namely mtimer and mswi. This allows platforms to implement
only the required set of devices. The mtimer as defined by the ACLINT
specifications also allows platforms to place mtime and mtimecmp
registers at different locations.

Refer, https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc

We need a separate DT bindings document for ACLINT MTIMER
and ACLINT MSWI because these are separate devices. The
Sophgo sg2042 SoC should add their implementation specific
compatible strings in this document.

Regards,
Anup