linux-kernel - Re: [PATCH v9 03/12] dt-bindings: RISC-V CPU Bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 5 Oct 2017 11:16:33 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Palmer Dabbelt <palmer@...belt.com>
Cc:     Arnd Bergmann <arnd@...db.de>, sfr@...b.auug.org.au,
        Olof Johansson <olof@...om.net>, linux-kernel@...r.kernel.org,
        patches@...ups.riscv.org, robh+dt@...nel.org, albert@...ive.com,
        yamada.masahiro@...ionext.com, mmarek@...e.com,
        will.deacon@....com, peterz@...radead.org, boqun.feng@...il.com,
        oleg@...hat.com, mingo@...hat.com, devicetree@...r.kernel.org
Subject: Re: [PATCH v9 03/12] dt-bindings: RISC-V CPU Bindings

Hi Palmer,

Sorry for the delay in getting round to this.

On Tue, Sep 26, 2017 at 06:56:29PM -0700, Palmer Dabbelt wrote:
> This patch adds device tree bindings for RISC-V CPUs, patterned after
> the ARM device tree CPU bindings.

[...]

> +- cpu node
> +
> +        Description: Describes a hart context
> +
> +        PROPERTIES
> +
> +        - device_type
> +                Usage: required
> +                Value type: <string>
> +                Definition: must be "cpu"
> +        - reg
> +                Usage: required
> +                Value type: <u32>
> +                Definition: The hart ID of this CPU node
> +        - compatible:
> +                Usage: required
> +                Value type: <stringlist>
> +                Definition: must contain "riscv", may contain one of
> +                            "sifive,rocket0"

It might make sense to have "riscv,cpu" as your genric RISC-V CPU
compatible string, to avoid ambiguity with the vendor-prefix.

> +        - mmu-type:
> +                Usage: optional
> +                Value type: <string>
> +                Definition: Specifies the CPU's MMU type.  Possible values are
> +                            "riscv,sv32"
> +                            "riscv,sv39"
> +                            "riscv,sv48"

I would *strongly* recommend that from day one, you determine the SMP
bringup mechanism via an enable-method property, and document the
contract with FW/bootloader somewhere in the kernel tree.

For arm64 we started by documenting our spin-table in
Documentation/arm64/booting.txt, and later added PSCI support.

Using an explicit enable-method property from day one makes it easier to
extend thigns without ambiguity. e.g. an old kernel that doesn't know of
some new enable-method can just give up on SMP and continue as UP,
rather than doing something wrong for that system.

Generally, making things explicit in the DT will make support /
extension easier, and help to avoid painful-to-debug bugs.

> +        - riscv,isa:
> +                Usage: required
> +                Value type: <string>
> +                Definition: Contains the RISC-V ISA string of this hart.  These
> +                            ISA strings are defined by the RISC-V ISA manual.
> +

IIUC from the example, this describes a number of ISA extenstions in a
single string. It might make mroe sense to have separate flags for each
extension. will be easier for the kernel to parse as new extensions
are added, and minimized ambiguity / typo problems.

[...]

> +Example: Spike ISA Simulator with 1 Hart
> +----------------------------------------
> +
> +This device tree matches the Spike ISA golden model as run with `spike -p1`.
> +
> +        cpus {
> +                cpu@0 {
> +                        device_type = "cpu";
> +                        reg = <0x00000000>;
> +                        status = "okay";
> +                        compatible = "riscv";
> +                        riscv,isa = "rv64imafdc";
> +                        mmu-type = "riscv,sv48";
> +                        clock-frequency = <0x3b9aca00>;
> +                        interrupt-controller {
> +                                #interrupt-cells = <0x00000001>;

Trivial nit: this would be nicer as:

	#interrupt-cells = <1>;

Thanks,
Mark.