[PATCH] dt-bindings: Add a RISC-V SBI firmware node

[PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Palmer Dabbelt]

The RISC-V privileged ISA mandates the presence of an SBI, but there's
no reason not to put it in the device tree.  This would allow us to
possibly remove the SBI later.

CC: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 .../devicetree/bindings/firmware/riscv.sbi.txt       | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/riscv.sbi.txt

diff --git a/Documentation/devicetree/bindings/firmware/riscv.sbi.txt b/Documentation/devicetree/bindings/firmware/riscv.sbi.txt
new file mode 100644
index 000000000000..42384d5d52cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/riscv.sbi.txt
@@ -0,0 +1,20 @@
+RISC-V Supervisor Binary Interface (SBI)
+
+The RISC-V privileged ISA specification mandates the presence of a supervisor
+binary interface that performs some operations which might otherwise require
+particularly complicated instructions.  This interface includes
+inter-processor interrupts, TLB flushes, i-cache and TLB shootdowns, a
+console, and power management.
+
+Required properties:
+- compatible: must contain one of the following
+ * "riscv,sbi" for the SBI defined by the privileged specification of the
+   system.
+
+Example:
+
+firmware {
+	sbi {
+		compatible = "riscv,sbi";
+	};
+};
-- 
2.13.6

Re: [PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]

On Mon, Nov 20, 2017 at 11:50:00AM -0800, Palmer Dabbelt wrote:
> The RISC-V privileged ISA mandates the presence of an SBI, but there's
> no reason not to put it in the device tree.  This would allow us to
> possibly remove the SBI later.

Thanks!

> 
> CC: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---
>  .../devicetree/bindings/firmware/riscv.sbi.txt       | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/riscv.sbi.txt
> 
> diff --git a/Documentation/devicetree/bindings/firmware/riscv.sbi.txt b/Documentation/devicetree/bindings/firmware/riscv.sbi.txt
> new file mode 100644
> index 000000000000..42384d5d52cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/riscv.sbi.txt

Nit: Other bindings use either a comma (as in the compatible string,
"riscv,sbi.txt") or a dash (vendor-product.txt, "riscv-sbi.txt") in the
file name.

> @@ -0,0 +1,20 @@
> +RISC-V Supervisor Binary Interface (SBI)
> +
> +The RISC-V privileged ISA specification mandates the presence of a supervisor
> +binary interface that performs some operations which might otherwise require
> +particularly complicated instructions.  This interface includes
> +inter-processor interrupts, TLB flushes, i-cache and TLB shootdowns, a
> +console, and power management.
> +
> +Required properties:
> +- compatible: must contain one of the following
> + * "riscv,sbi" for the SBI defined by the privileged specification of the
> +   system.

"of the system" seems to imply that different RISC-V systems (different
RISC-V implementations) can have different privileged specifications.

I think it's better to refer to concrete documents, that don't depend on
the rest of the system, instead. Either:

 * "riscv,sbi" for the SBI defined by the RISC-V Privileged ISA Specification.

Or something like:

 * "sifive,sbi" for the SBI defined by SiFive document XYZ.


[ I know that there currently is no SBI spec, because the chapter has
  been removed from the Priv Spec, but this can be fixed later, once
  the final name of the document describing the SBI is clear. ]

> +
> +Example:
> +
> +firmware {
> +	sbi {
> +		compatible = "riscv,sbi";
> +	};
> +};
> -- 


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [patches] Re: [PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Palmer Dabbelt]

On Mon, 20 Nov 2017 12:28:56 PST (-0800), j.neuschaefer@gmx.net wrote:
> On Mon, Nov 20, 2017 at 11:50:00AM -0800, Palmer Dabbelt wrote:
>> The RISC-V privileged ISA mandates the presence of an SBI, but there's
>> no reason not to put it in the device tree.  This would allow us to
>> possibly remove the SBI later.
>
> Thanks!
>
>> 
>> CC: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
>> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
>> ---
>>  .../devicetree/bindings/firmware/riscv.sbi.txt       | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/firmware/riscv.sbi.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/firmware/riscv.sbi.txt b/Documentation/devicetree/bindings/firmware/riscv.sbi.txt
>> new file mode 100644
>> index 000000000000..42384d5d52cf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/firmware/riscv.sbi.txt
>
> Nit: Other bindings use either a comma (as in the compatible string,
> "riscv,sbi.txt") or a dash (vendor-product.txt, "riscv-sbi.txt") in the
> file name.

That was just a typo, I'll fix it.

>> @@ -0,0 +1,20 @@
>> +RISC-V Supervisor Binary Interface (SBI)
>> +
>> +The RISC-V privileged ISA specification mandates the presence of a supervisor
>> +binary interface that performs some operations which might otherwise require
>> +particularly complicated instructions.  This interface includes
>> +inter-processor interrupts, TLB flushes, i-cache and TLB shootdowns, a
>> +console, and power management.
>> +
>> +Required properties:
>> +- compatible: must contain one of the following
>> + * "riscv,sbi" for the SBI defined by the privileged specification of the
>> +   system.
>
> "of the system" seems to imply that different RISC-V systems (different
> RISC-V implementations) can have different privileged specifications.

Actually, that was intentional -- I wrote it this way because different RISC-V 
systems do have different privileged specifications.  The RISC-V specifications 
aren't frozen in time, they're just guaranteed to be compatible in the future.  
For example, the user ISA document has been updated multiple times (the C spec, 
eliminating some unspecified behavior) and will continue to be updated (V and 
other extensions, the memory model).  The privileged spec will be updated in a 
compatible way just like the user spec will be -- I know there's at least 
hypervisor support in the works, and I saw some things to remove undefined 
behavior go past as well.

In a similar fashion, the ABI and SBI will continue to evolve.  For example, 
we'll probably add new system calls to extend the user ABI and new hyper calls 
to extend the SBI.

> I think it's better to refer to concrete documents, that don't depend on
> the rest of the system, instead. Either:
>
>  * "riscv,sbi" for the SBI defined by the RISC-V Privileged ISA Specification.
>
> Or something like:
>
>  * "sifive,sbi" for the SBI defined by SiFive document XYZ.
>
>
> [ I know that there currently is no SBI spec, because the chapter has
>   been removed from the Priv Spec, but this can be fixed later, once
>   the final name of the document describing the SBI is clear. ]

Ya, well, that's just a bug :).  There'll eventually be a spec, but I don't 
think it changes the wording here.

>
>> +
>> +Example:
>> +
>> +firmware {
>> +	sbi {
>> +		compatible = "riscv,sbi";
>> +	};
>> +};
>> -- 
>
>
> Thanks,
> Jonathan Neuschäfer
>
> -- 
> You received this message because you are subscribed to the Google Groups "RISC-V Patches" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to patches+unsubscribe@groups.riscv.org.
> To post to this group, send email to patches@groups.riscv.org.
> Visit this group at https://groups.google.com/a/groups.riscv.org/group/patches/.
> To view this discussion on the web visit https://groups.google.com/a/groups.riscv.org/d/msgid/patches/20171120202856.nptoirhm5luiamt7%40latitude.
> For more options, visit https://groups.google.com/a/groups.riscv.org/d/optout.

Re: [patches] Re: [PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]

On Mon, Nov 20, 2017 at 01:28:01PM -0800, Palmer Dabbelt wrote:
[...]
> > > +++ b/Documentation/devicetree/bindings/firmware/riscv.sbi.txt
> > 
> > Nit: Other bindings use either a comma (as in the compatible string,
> > "riscv,sbi.txt") or a dash (vendor-product.txt, "riscv-sbi.txt") in the
> > file name.
> 
> That was just a typo, I'll fix it.

Ok

> > > @@ -0,0 +1,20 @@
> > > +RISC-V Supervisor Binary Interface (SBI)
> > > +
> > > +The RISC-V privileged ISA specification mandates the presence of a supervisor
> > > +binary interface that performs some operations which might otherwise require
> > > +particularly complicated instructions.  This interface includes
> > > +inter-processor interrupts, TLB flushes, i-cache and TLB shootdowns, a
> > > +console, and power management.
> > > +
> > > +Required properties:
> > > +- compatible: must contain one of the following
> > > + * "riscv,sbi" for the SBI defined by the privileged specification of the
> > > +   system.
> > 
> > "of the system" seems to imply that different RISC-V systems (different
> > RISC-V implementations) can have different privileged specifications.
> 
> Actually, that was intentional -- I wrote it this way because different
> RISC-V systems do have different privileged specifications.  The RISC-V
> specifications aren't frozen in time, they're just guaranteed to be
> compatible in the future.  For example, the user ISA document has been
> updated multiple times (the C spec, eliminating some unspecified behavior)
> and will continue to be updated (V and other extensions, the memory model).
> The privileged spec will be updated in a compatible way just like the user
> spec will be -- I know there's at least hypervisor support in the works, and
> I saw some things to remove undefined behavior go past as well.
> 
> In a similar fashion, the ABI and SBI will continue to evolve.  For example,
> we'll probably add new system calls to extend the user ABI and new hyper
> calls to extend the SBI.

My problem with the wording was that the OS somehow has to know which
version and variant of the SBI it is talking to -- either through
in-band communication (an SBI call to request SBI information, etc.), or
through devicetree or similar mechanisms.

> 
> > I think it's better to refer to concrete documents, that don't depend on
> > the rest of the system, instead. Either:


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [patches] Re: [PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Palmer Dabbelt]

On Mon, 20 Nov 2017 17:08:44 PST (-0800), j.neuschaefer@gmx.net wrote:
> On Mon, Nov 20, 2017 at 01:28:01PM -0800, Palmer Dabbelt wrote:
>> > > @@ -0,0 +1,20 @@
>> > > +RISC-V Supervisor Binary Interface (SBI)
>> > > +
>> > > +The RISC-V privileged ISA specification mandates the presence of a supervisor
>> > > +binary interface that performs some operations which might otherwise require
>> > > +particularly complicated instructions.  This interface includes
>> > > +inter-processor interrupts, TLB flushes, i-cache and TLB shootdowns, a
>> > > +console, and power management.
>> > > +
>> > > +Required properties:
>> > > +- compatible: must contain one of the following
>> > > + * "riscv,sbi" for the SBI defined by the privileged specification of the
>> > > +   system.
>> > 
>> > "of the system" seems to imply that different RISC-V systems (different
>> > RISC-V implementations) can have different privileged specifications.
>> 
>> Actually, that was intentional -- I wrote it this way because different
>> RISC-V systems do have different privileged specifications.  The RISC-V
>> specifications aren't frozen in time, they're just guaranteed to be
>> compatible in the future.  For example, the user ISA document has been
>> updated multiple times (the C spec, eliminating some unspecified behavior)
>> and will continue to be updated (V and other extensions, the memory model).
>> The privileged spec will be updated in a compatible way just like the user
>> spec will be -- I know there's at least hypervisor support in the works, and
>> I saw some things to remove undefined behavior go past as well.
>> 
>> In a similar fashion, the ABI and SBI will continue to evolve.  For example,
>> we'll probably add new system calls to extend the user ABI and new hyper
>> calls to extend the SBI.
>
> My problem with the wording was that the OS somehow has to know which
> version and variant of the SBI it is talking to -- either through
> in-band communication (an SBI call to request SBI information, etc.), or
> through devicetree or similar mechanisms.

The idea here is that rather than checking for a version of the SBI, you just 
check for various features as you need them.

Re: [patches] Re: [PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Mark Rutland]

Hi Palmer,

On Mon, Nov 20, 2017 at 01:28:01PM -0800, Palmer Dabbelt wrote:
> On Mon, 20 Nov 2017 12:28:56 PST (-0800), j.neuschaefer@gmx.net wrote:
> > On Mon, Nov 20, 2017 at 11:50:00AM -0800, Palmer Dabbelt wrote:
> > > +RISC-V Supervisor Binary Interface (SBI)
> > > +
> > > +The RISC-V privileged ISA specification mandates the presence of a supervisor
> > > +binary interface that performs some operations which might otherwise require
> > > +particularly complicated instructions.  This interface includes
> > > +inter-processor interrupts, TLB flushes, i-cache and TLB shootdowns, a
> > > +console, and power management.
> > > +
> > > +Required properties:
> > > +- compatible: must contain one of the following
> > > + * "riscv,sbi" for the SBI defined by the privileged specification of the
> > > +   system.
> > 
> > "of the system" seems to imply that different RISC-V systems (different
> > RISC-V implementations) can have different privileged specifications.
> 
> Actually, that was intentional -- I wrote it this way because different
> RISC-V systems do have different privileged specifications.  The RISC-V
> specifications aren't frozen in time, they're just guaranteed to be
> compatible in the future. 

If that's the case, then you can define a version of the document that
is a baseline. e.g. 

 * "riscv,sbi" for an SBI implementation compatible with that defined
   in $XYZ_DOCUMENT version $N

If every new feature can be probed from that point onwards, then that's
all you'll ever need. Otherwise, if there are backwards-incompatible
changes or non-probeable features, you can add additional strings, and
there's no ambiguity.

See Documentation/devicetree/bindings/arm/psci.txt for an similar
example on ARM systems. That's explicitly versioned, though we don't
list each and every document number, and we probably should.

Thanks,
Mark.

Re: [PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Rob Herring]

On Mon, Nov 20, 2017 at 11:50:00AM -0800, Palmer Dabbelt wrote:
> The RISC-V privileged ISA mandates the presence of an SBI, but there's
> no reason not to put it in the device tree.  This would allow us to
> possibly remove the SBI later.

If it is mandatory, then it should not be in DT. DT is not a driver 
instantiation mechanism.

If your ISA can vary, I'd recommend you make that discoverable. DT is 
for what we failed to make discoverable.

Rob

Re: [patches] Re: [PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Palmer Dabbelt]

On Mon, 20 Nov 2017 13:45:20 PST (-0800), robh@kernel.org wrote:
> On Mon, Nov 20, 2017 at 11:50:00AM -0800, Palmer Dabbelt wrote:
>> The RISC-V privileged ISA mandates the presence of an SBI, but there's
>> no reason not to put it in the device tree.  This would allow us to
>> possibly remove the SBI later.
>
> If it is mandatory, then it should not be in DT. DT is not a driver
> instantiation mechanism.
>
> If your ISA can vary, I'd recommend you make that discoverable. DT is
> for what we failed to make discoverable.

OK, that makes sense.  Since this the presence of an SBI is mandated by the ISA 
there's no way to discover it (just like there's no way to discover a load 
instruction).  For extensibility reasons you can, of course, determine which 
SBI calls are implemented, but this mechanism assumes you have something 
running to catch the SBI calls on the other end (SBI calls are just system 
calls from the Linux).  I think the original goal here was to avoid an SBI 
entirely, which isn't something the ISA is designed for.

This isn't really a big deal to me, as I'm only interested in RISC-V systems, 
but there's been some pushback on the concept of an SBI so it seemed like a 
simple way to allow people to build non-SBI (and there for not really RISC-V) 
systems.  One option that wouldn't require a device tree node would be to have 
Linux boot in machine mode (where the SBI implementation lives on systems 
without a hypervisor, if you've got a hypervisor then I assume the SBI isn't a 
problem) and then provide its own SBI implementation.  This wouldn't impose any 
additional burden: if there's no SBI then Linux will need to start in machine 
mode because that's where you need to be in order to do the things the SBI 
implementation needs to go.  This will be awkward to implement, but having a 
device tree entry won't help with any of that.

I think the right thing to do here is just not define a device tree entry.

Re: [patches] Re: [PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 792 bytes --]

On Tue, Nov 21, 2017 at 09:37:02AM -0800, Palmer Dabbelt wrote:
[...]
> This isn't really a big deal to me, as I'm only interested in RISC-V
> systems, but there's been some pushback on the concept of an SBI so it
> seemed like a simple way to allow people to build non-SBI (and there for not
> really RISC-V) systems.

For those reading along: I suggested the /firmware/sbi node to Palmer,
because I'm interested in such "not really RISC-V" systems, (because it
makes the firmware's job easier to not implement the SBI — speaking with
my coreboot hat, here.)

> One option that wouldn't require a device tree node
> would be to have Linux boot in machine mode [...] and then provide its
> own SBI implementation.

I think this can work.


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [patches] Re: [PATCH] dt-bindings: Add a RISC-V SBI firmware node

[Palmer Dabbelt]

On Tue, 21 Nov 2017 12:08:32 PST (-0800), j.neuschaefer@gmx.net wrote:
> On Tue, Nov 21, 2017 at 09:37:02AM -0800, Palmer Dabbelt wrote:
> [...]
>> This isn't really a big deal to me, as I'm only interested in RISC-V
>> systems, but there's been some pushback on the concept of an SBI so it
>> seemed like a simple way to allow people to build non-SBI (and there for not
>> really RISC-V) systems.
>
> For those reading along: I suggested the /firmware/sbi node to Palmer,
> because I'm interested in such "not really RISC-V" systems, (because it
> makes the firmware's job easier to not implement the SBI — speaking with
> my coreboot hat, here.)
>
>> One option that wouldn't require a device tree node
>> would be to have Linux boot in machine mode [...] and then provide its
>> own SBI implementation.
>
> I think this can work.

OK, sounds good!