Message-ID: <c1a1b920-124c-23b1-6dd5-1a4cc54659cc@gmail.com>
Date:   Mon, 27 Jul 2020 07:47:32 -0400
From:   Sean Anderson <seanga2@...il.com>
To:     Palmer Dabbelt <palmer@...belt.com>, atishp@...shpatra.org
Cc:     devicetree@...r.kernel.org, Damien Le Moal <Damien.LeMoal@....com>,
        daniel.lezcano@...aro.org, kernel@...il.dk,
        Anup Patel <Anup.Patel@....com>, linux-kernel@...r.kernel.org,
        Atish Patra <Atish.Patra@....com>, robh+dt@...nel.org,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Alistair Francis <Alistair.Francis@....com>,
        tglx@...utronix.de, linux-riscv@...ts.infradead.org,
        aou@...s.berkeley.edu
Subject: Re: [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings

On 7/26/20 2:37 PM, Palmer Dabbelt wrote:
> On Tue, 21 Jul 2020 20:55:31 PDT (-0700), anup@...infault.org wrote:
>> On Tue, Jul 21, 2020 at 5:48 PM Sean Anderson <seanga2@...il.com> wrote:
>>>
>>> On 7/20/20 9:15 PM, Atish Patra wrote:
>>> > On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@....com> wrote:
>>> >>
>>> >> We add DT bindings documentation for CLINT device.
>>> >>
>>> >> Signed-off-by: Anup Patel <anup.patel@....com>
>>> >> Reviewed-by: Palmer Dabbelt <palmerdabbelt@...gle.com>
>>> >> Tested-by: Emil Renner Berhing <kernel@...il.dk>
>>> >> ---
>>> >>  .../bindings/timer/sifive,clint.yaml          | 58 +++++++++++++++++++
>>> >>  1 file changed, 58 insertions(+)
>>> >>  create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> >>
>>> >> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> >> new file mode 100644
>>> >> index 000000000000..8ad115611860
>>> >> --- /dev/null
>>> >> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> >> @@ -0,0 +1,58 @@
>>> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> >> +%YAML 1.2
>>> >> +---
>>> >> +$id: http://devicetree.org/schemas/timer/sifive,clint.yaml#
>>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> >> +
>>> >> +title: SiFive Core Local Interruptor
>>> >> +
>>> >> +maintainers:
>>> >> +  - Palmer Dabbelt <palmer@...belt.com>
>>> >> +  - Anup Patel <anup.patel@....com>
>>> >> +
>>> >> +description:
>>> >> +  SiFive (and other RISC-V) SOCs include an implementation of the SiFive
>>> >> +  Core Local Interruptor (CLINT) for M-mode timer and M-mode inter-processor
>>> >> +  interrupts. It directly connects to the timer and inter-processor interrupt
>>> >> +  lines of various HARTs (or CPUs) so RISC-V per-HART (or per-CPU) local
>>> >> +  interrupt controller is the parent interrupt controller for CLINT device.
>>> >> +  The clock frequency of CLINT is specified via "timebase-frequency" DT
>>> >> +  property of "/cpus" DT node. The "timebase-frequency" DT property is
>>> >> +  described in Documentation/devicetree/bindings/riscv/cpus.yaml
>>> >> +
>>> >> +properties:
>>> >> +  compatible:
>>> >> +    items:
>>> >> +      - const: sifive,clint0
>>> >> +      - const: sifive,fu540-c000-clint
>>> >> +
>>> >> +    description:
>>> >> +      Should be "sifive,<chip>-clint" and "sifive,clint<version>".
>>> >> +      Supported compatible strings are -
>>> >> +      "sifive,fu540-c000-clint" for the SiFive CLINT v0 as integrated
>>> >> +      onto the SiFive FU540 chip, and "sifive,clint0" for the SiFive
>>> >> +      CLINT v0 IP block with no chip integration tweaks.
>>> >> +      Please refer to sifive-blocks-ip-versioning.txt for details
>>> >> +
>>> >
>>> > As the DT binding suggests that the clint device should be named as "sifive,**",
>>> > I think we should change the DT property in kendryte dts as well.
>>>
>>> The kendryte device is based on Rocket Chip, not any SiFive IP/device.
>>> If anything, the general binding should be "chipsalliance,clint" and the
>>> specific bindings should be "sifive,clint" and "kendryte,clint" (or
>>> "canaan,clint").
>>
>> AFAIK, Palmer clearly mentioned in previous discussion that CLINT
>> spec is still owned by SiFive. No matter who implements CLINT device
>> in their SOC, we will need one compatible string to represent the
>> spec version (i.e. "sifive,clint0") and another compatible representing
>> specific implementation (for kendryte this can be "kendryte,k210-clint").
> 
> I think "sifive,clint*" is the way to go here.  The Free Chips Foundation owns
> Rocket Chip, which contains an implementation of SiFive's CLINT specification,
> but as far as I can tell Free Chips itself does not produce a specification for
> the CLINT.  The only CLINT specifications I can find are for SiFive's chips and
> are copyright SiFive, and we generally prefer sticking to specifications rather
> than implementations for these sorts of things.

Ah, I wasn't aware that compatibility string assignment was based on
published specifications.

> 
> To be honest, I'm not even sure the Free Chips CLINT is an implementation of
> the SiFive specification: we don't have the source code for those chips, and
> while I'm fairly sure the Chisel source code for the CLINT itself on SiFive's
> chips is very close to what was in Rocket Chip at the time those chips were
> built (though we don't have the source, so we don't know for user), device
> specifications depend on the broader SOC context they are embedded inside.  For
> example: SiFive's CLINT allows us to use simple MMIO reads of mtime to meet the
> RISC-V rdtime requirements, but other bus configurations may not meet those
> requirements.
> 
> If Free Chips publishes a specification for a CLINT and it's compatible with
> this version of SiFive's CLINT then I don't see any reason why we couldn't add
> that as a compatible string, but as it currently stands there is no Free Chips
> CLINT specification.  IMO it would be irresponsible for us to define one on
> their behalf.
> 
> I don't know anything about Kendryte's specifications, as I haven't read them
> myself.  Assuming they define the CLINT's behavior in their SOC manual, then

They don't. I emailed some people from Canaan and they said they used
rocketchip as their core. The best thing we have is the documentation in
their SDK [1]. FWIW, these comments almost exactly match the SiFive's
CLINT documentation [2]. I don't know whether that meets Linux's
standards for a "specification" or not.

> adding something along the lines of "canaan,kendryte-k210-clint" seems
> reasonable to me -- don't really care that much about the specific name, but as
> we don't define the Kendryte SOCs then calling it anything more general than
> this specific SOC's CLINT seems unreasonable.  AFAIK the Kendryte people don't
> get involved with Linux development directly, so that's probably as much as we
> can define.

--Sean

[1] https://github.com/kendryte/kendryte-standalone-sdk/blob/develop/lib/drivers/include/clint.h
[2] e.g. chapter 9 of https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf

Powered by blists - more mailing lists