[v3,4/4] documentation: Add secure monitor binding documentation
diff mbox

Message ID 1464021024-29380-5-git-send-email-carlo@caione.org
State Superseded
Headers show

Commit Message

Carlo Caione May 23, 2016, 4:30 p.m. UTC
From: Carlo Caione <carlo@endlessm.com>

Add the binding documentation for the Amlogic secure monitor driver.

Signed-off-by: Carlo Caione <carlo@endlessm.com>
---
 .../bindings/firmware/meson/meson_sm.txt           | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/meson/meson_sm.txt

Comments

Mark Rutland May 23, 2016, 4:38 p.m. UTC | #1
Hi,

Minor nit, but please put the binding earlier in the series than the
code implementing it, as per
Documentation/devicetree/bindings/submitting-patches.txt

This makes it possible to review a series in a linear fashion.

On Mon, May 23, 2016 at 06:30:24PM +0200, Carlo Caione wrote:
> From: Carlo Caione <carlo@endlessm.com>
> 
> Add the binding documentation for the Amlogic secure monitor driver.
> 
> Signed-off-by: Carlo Caione <carlo@endlessm.com>
> ---
>  .../bindings/firmware/meson/meson_sm.txt           | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/meson/meson_sm.txt
> 
> diff --git a/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt b/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt
> new file mode 100644
> index 0000000..2b71716
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt
> @@ -0,0 +1,51 @@
> +* Amlogic Secure Monitor
> +
> +In the Amlogic SoCs the Secure Monitor code is used to provide access to the
> +NVMEM, enable JTAG, set USB boot, etc...
> +
> +Required properties for the secure monitor node:
> +- compatible: Should be "amlogic,meson-sm"
> +- amlogic,sm-cmd-input-base: SMC32 function identifier to read the physical
> +                             address of the input buffer
> +- amlogic,sm-cmd-output-base: SMC32 function identifier to read the physical
> +                              address of the output buffer

Do the IDs for these actually differ per board?

Are some functions simply not implemented on some boards?

> +
> +Example:
> +
> +	#include <dt-bindings/firmware/meson-gxbb-sm.h>
> +
> +	...
> +
> +	firmware {
> +		compatible = "simple-bus";
> +
> +		sm: secure-monitor {
> +			compatible = "amlogic,meson-sm";
> +			amlogic,sm-cmd-input-base = <SM_GET_SHARE_MEM_INPUT_BASE>;
> +			amlogic,sm-cmd-output-base = <SM_GET_SHARE_MEM_OUTPUT_BASE>;
> +		};
> +	};
> +
> +Node of a device using the secure monitor must have a custom property to
> +specify the SMC32 function the driver is going to use. The values of all the
> +SMC function identifiers are listed in a SoC-specific header file:
> +
> +"include/dt-bindings/firmware/meson-gxbb-sm.h" - for the Meson GXBB secure monitor.
> +
> +Example of the node using the secure monitor:
> +
> +	#include <dt-bindings/firmware/meson-gxbb-sm.h>
> +
> +	...
> +
> +	efuse: efuse {
> +		compatible = "amlogic,meson-gxbb-efuse";
> +		secure-monitor = <&sm>;
> +		amlogic,sm-cmd-read-efuse = <SM_EFUSE_READ>;
> +		amlogic,sm-cmd-max-efuse = <SM_EFUSE_USER_MAX>;

Likewise for these?

Mark.

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		...
> +	};
> +
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Carlo Caione May 23, 2016, 4:59 p.m. UTC | #2
On 23/05/16 17:38, Mark Rutland wrote:
> Hi,
> 
> Minor nit, but please put the binding earlier in the series than the
> code implementing it, as per
> Documentation/devicetree/bindings/submitting-patches.txt
> 
> This makes it possible to review a series in a linear fashion.

I'll do. Thanks for the pointer.

[...]
> > +
> > +Required properties for the secure monitor node:
> > +- compatible: Should be "amlogic,meson-sm"
> > +- amlogic,sm-cmd-input-base: SMC32 function identifier to read the physical
> > +                             address of the input buffer
> > +- amlogic,sm-cmd-output-base: SMC32 function identifier to read the physical
> > +                              address of the output buffer
> 
> Do the IDs for these actually differ per board?

I expect these to differ per SoC (GXBB in this case), not per board. The
driver is generic enough to be (hopefully) used for several SoCs just
changing the related header file that defines the SCM commands.

> Are some functions simply not implemented on some boards?

I don't think this is possible.

[...]
> > +
> > +Example of the node using the secure monitor:
> > +
> > +	#include <dt-bindings/firmware/meson-gxbb-sm.h>
> > +
> > +	...
> > +
> > +	efuse: efuse {
> > +		compatible = "amlogic,meson-gxbb-efuse";
> > +		secure-monitor = <&sm>;
> > +		amlogic,sm-cmd-read-efuse = <SM_EFUSE_READ>;
> > +		amlogic,sm-cmd-max-efuse = <SM_EFUSE_USER_MAX>;
> 
> Likewise for these?

AFAIK the secure monitor code and related commands are SoC specific.

Thanks,
Mark Rutland May 23, 2016, 5:11 p.m. UTC | #3
On Mon, May 23, 2016 at 06:59:31PM +0200, Carlo Caione wrote:
> On 23/05/16 17:38, Mark Rutland wrote:
> > > +Required properties for the secure monitor node:
> > > +- compatible: Should be "amlogic,meson-sm"
> > > +- amlogic,sm-cmd-input-base: SMC32 function identifier to read the physical
> > > +                             address of the input buffer
> > > +- amlogic,sm-cmd-output-base: SMC32 function identifier to read the physical
> > > +                              address of the output buffer
> > 
> > Do the IDs for these actually differ per board?
> 
> I expect these to differ per SoC (GXBB in this case), not per board. The
> driver is generic enough to be (hopefully) used for several SoCs just
> changing the related header file that defines the SCM commands.
> 
> > Are some functions simply not implemented on some boards?
> 
> I don't think this is possible.

Given that, I think it may be better to just have a
"amlogic,meson-gxbb-sm" compatible string, and derive the set of
functions and associated IDs from that.

That is, unless you know that future revisions have functions with the
exact same semantics but differing IDs.

Regardless, it would make sense to have a GXBB-specific compatible
string prepended to the list. That way in the future we can handle
anything specific to the GXBB variant of the secure monitor if
necessary.

Thanks,
Mark.
Carlo Caione May 24, 2016, 8:03 a.m. UTC | #4
On 23/05/16 18:11, Mark Rutland wrote:
> On Mon, May 23, 2016 at 06:59:31PM +0200, Carlo Caione wrote:
> > On 23/05/16 17:38, Mark Rutland wrote:
> > > > +Required properties for the secure monitor node:
> > > > +- compatible: Should be "amlogic,meson-sm"
> > > > +- amlogic,sm-cmd-input-base: SMC32 function identifier to read the physical
> > > > +                             address of the input buffer
> > > > +- amlogic,sm-cmd-output-base: SMC32 function identifier to read the physical
> > > > +                              address of the output buffer
> > > 
> > > Do the IDs for these actually differ per board?
> > 
> > I expect these to differ per SoC (GXBB in this case), not per board. The
> > driver is generic enough to be (hopefully) used for several SoCs just
> > changing the related header file that defines the SCM commands.
> > 
> > > Are some functions simply not implemented on some boards?
> > 
> > I don't think this is possible.
> 
> Given that, I think it may be better to just have a
> "amlogic,meson-gxbb-sm" compatible string, and derive the set of
> functions and associated IDs from that.

I can reference a specific SMC function using an index in the DT, the
same index for all the SoCs. The index is then associated to the actual
SoC-specific command ID in the driver according to the compatible string
used for the secure-monitor node.

Something like:

  // Not SoC-specific
  #include <dt-bindings/firmware/meson.h>

  sm: sm {
  	compatible = "amlogic,meson-gxbb-sm";
  };
  
  efuse {
  	compatible = "amlogic,meson-gxbb-efuse";
  	secure-monitor = <&sm>;
  	amlogic,cmd-read-efuse = <READ_EFUSE>;
  	...
  };

Is this any better?

At this point I wonder if it makes sense having the driver-specific
function IDs (like 'amlogic,cmd-read-efuse' above) defined in the DT.

> That is, unless you know that future revisions have functions with the
> exact same semantics but differing IDs.

This is most probably the case.

Also the driver exports really generic functions to access the
secure-monitor on purpose, so that the driver using it can define the
semantic of the SMC call. I really would like to avoid fixing the
semantic in the SM driver itself since we will end up with: different
semantics for each SMC call and for each SoC.

> Regardless, it would make sense to have a GXBB-specific compatible
> string prepended to the list. That way in the future we can handle
> anything specific to the GXBB variant of the secure monitor if
> necessary.

Agree on this.

Cheers,

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt b/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt
new file mode 100644
index 0000000..2b71716
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/meson/meson_sm.txt
@@ -0,0 +1,51 @@ 
+* Amlogic Secure Monitor
+
+In the Amlogic SoCs the Secure Monitor code is used to provide access to the
+NVMEM, enable JTAG, set USB boot, etc...
+
+Required properties for the secure monitor node:
+- compatible: Should be "amlogic,meson-sm"
+- amlogic,sm-cmd-input-base: SMC32 function identifier to read the physical
+                             address of the input buffer
+- amlogic,sm-cmd-output-base: SMC32 function identifier to read the physical
+                              address of the output buffer
+
+Example:
+
+	#include <dt-bindings/firmware/meson-gxbb-sm.h>
+
+	...
+
+	firmware {
+		compatible = "simple-bus";
+
+		sm: secure-monitor {
+			compatible = "amlogic,meson-sm";
+			amlogic,sm-cmd-input-base = <SM_GET_SHARE_MEM_INPUT_BASE>;
+			amlogic,sm-cmd-output-base = <SM_GET_SHARE_MEM_OUTPUT_BASE>;
+		};
+	};
+
+Node of a device using the secure monitor must have a custom property to
+specify the SMC32 function the driver is going to use. The values of all the
+SMC function identifiers are listed in a SoC-specific header file:
+
+"include/dt-bindings/firmware/meson-gxbb-sm.h" - for the Meson GXBB secure monitor.
+
+Example of the node using the secure monitor:
+
+	#include <dt-bindings/firmware/meson-gxbb-sm.h>
+
+	...
+
+	efuse: efuse {
+		compatible = "amlogic,meson-gxbb-efuse";
+		secure-monitor = <&sm>;
+		amlogic,sm-cmd-read-efuse = <SM_EFUSE_READ>;
+		amlogic,sm-cmd-max-efuse = <SM_EFUSE_USER_MAX>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		...
+	};
+