| Message ID | 1464021024-29380-5-git-send-email-carlo@caione.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 >
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,
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.
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,
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>; + + ... + }; +