[1/8] dt/bindings: firmware: Add Qualcomm SCM binding
diff mbox

Message ID 1461363432-5730-2-git-send-email-andy.gross@linaro.org
State New, archived
Headers show

Commit Message

Andy Gross April 22, 2016, 10:17 p.m. UTC
This patch adds the device tree support for the Qualcomm SCM firmware.

Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
 .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt

Comments

Bjorn Andersson April 22, 2016, 11:12 p.m. UTC | #1
On Fri 22 Apr 15:17 PDT 2016, Andy Gross wrote:

> This patch adds the device tree support for the Qualcomm SCM firmware.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn
Stanimir Varbanov April 23, 2016, 7:51 a.m. UTC | #2
Hi Andy,

On 04/23/2016 01:17 AM, Andy Gross wrote:
> This patch adds the device tree support for the Qualcomm SCM firmware.
> 
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
>  .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> new file mode 100644
> index 0000000..57b9b3a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> @@ -0,0 +1,31 @@
> +QCOM Secure Channel Manager (SCM)
> +
> +Qualcomm processors include an interface to communicate to the secure firmware.
> +This interface allows for clients to request different types of actions.  These
> +can include CPU power up/down, HDCP requests, loading of firmware, and other
> +assorted actions.
> +
> +Required properties:
> +- compatible: must contain one of the following:
> + * "qcom,scm-apq8064" for APQ8064
> + * "qcom,scm-apq8084" for MSM8084

s/MSM8084/APQ8084

regards,
Stan
Rob Herring April 23, 2016, 4:56 p.m. UTC | #3
On Fri, Apr 22, 2016 at 5:17 PM, Andy Gross <andy.gross@linaro.org> wrote:
> This patch adds the device tree support for the Qualcomm SCM firmware.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
>  .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt
>
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> new file mode 100644
> index 0000000..57b9b3a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> @@ -0,0 +1,31 @@
> +QCOM Secure Channel Manager (SCM)
> +
> +Qualcomm processors include an interface to communicate to the secure firmware.
> +This interface allows for clients to request different types of actions.  These
> +can include CPU power up/down, HDCP requests, loading of firmware, and other
> +assorted actions.
> +
> +Required properties:
> +- compatible: must contain one of the following:
> + * "qcom,scm-apq8064" for APQ8064
> + * "qcom,scm-apq8084" for MSM8084
> + * "qcom,scm-msm8916" for MSM8916
> + * "qcom,scm-msm8974" for MSM8974
> +- clocks: One to three clocks may be required based on compatible.
> + * Only core clock required for "qcom,scm-apq8064"
> + * Core, iface, and bus clocks required for all other compatibles.
> +- clock-names: Must contain "core" for the core clock, "iface" for the interface
> +  clock and "bus" for the bus clock per the requirements of the compatible.
> +
> +Example for MSM8916:
> +
> +       firmware {
> +               compatible = "simple-bus";

Firmware is a bus? Really? Let's not put hacks in the DT just so you
get automatic probing.

> +
> +               scm {
> +                       compatible = "qcom,scm-msm8916";
> +                       clocks = <&gcc GCC_CRYPTO_CLK> , <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
> +                       clock-names = "core", "bus", "iface";

Generally, /firmware defines an interface to firmware. I don't think
clocks belong here. This implies that non-secure world can turn off
clocks to secure world?

Rob
Andy Gross April 23, 2016, 5:33 p.m. UTC | #4
On Sat, Apr 23, 2016 at 11:56:50AM -0500, Rob Herring wrote:
> On Fri, Apr 22, 2016 at 5:17 PM, Andy Gross <andy.gross@linaro.org> wrote:
> > This patch adds the device tree support for the Qualcomm SCM firmware.
> >
> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > ---
> >  .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > new file mode 100644
> > index 0000000..57b9b3a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > @@ -0,0 +1,31 @@
> > +QCOM Secure Channel Manager (SCM)
> > +
> > +Qualcomm processors include an interface to communicate to the secure firmware.
> > +This interface allows for clients to request different types of actions.  These
> > +can include CPU power up/down, HDCP requests, loading of firmware, and other
> > +assorted actions.
> > +
> > +Required properties:
> > +- compatible: must contain one of the following:
> > + * "qcom,scm-apq8064" for APQ8064
> > + * "qcom,scm-apq8084" for MSM8084
> > + * "qcom,scm-msm8916" for MSM8916
> > + * "qcom,scm-msm8974" for MSM8974
> > +- clocks: One to three clocks may be required based on compatible.
> > + * Only core clock required for "qcom,scm-apq8064"
> > + * Core, iface, and bus clocks required for all other compatibles.
> > +- clock-names: Must contain "core" for the core clock, "iface" for the interface
> > +  clock and "bus" for the bus clock per the requirements of the compatible.
> > +
> > +Example for MSM8916:
> > +
> > +       firmware {
> > +               compatible = "simple-bus";
> 
> Firmware is a bus? Really? Let's not put hacks in the DT just so you
> get automatic probing.

So something like:

        firmware {
                compatible = "qcom,scm-apq8084";
                clocks = <&gcc GCC_CE1_CLK> , <&gcc GCC_CE1_AXI_CLK>, <&gcc GCC_CE1_AHB_CLK>;
                clock-names = "core", "bus", "iface";
        };

Seems to work fine.

> > +
> > +               scm {
> > +                       compatible = "qcom,scm-msm8916";
> > +                       clocks = <&gcc GCC_CRYPTO_CLK> , <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
> > +                       clock-names = "core", "bus", "iface";
> 
> Generally, /firmware defines an interface to firmware. I don't think
> clocks belong here. This implies that non-secure world can turn off
> clocks to secure world?

The caller into the SCM is on the hook for making sure the clocks are turned on.
The firmware people decided to not manage the clocks.  In a perfect world, they
would turn on their own clocks and it would all be self contained.  Sadly, it
isn't going to change.

The alternative is every device that makes scm calls needs to manage the clocks
for the firmware.
Rob Herring April 25, 2016, 1:14 p.m. UTC | #5
On Sat, Apr 23, 2016 at 12:33:51PM -0500, Andy Gross wrote:
> On Sat, Apr 23, 2016 at 11:56:50AM -0500, Rob Herring wrote:
> > On Fri, Apr 22, 2016 at 5:17 PM, Andy Gross <andy.gross@linaro.org> wrote:
> > > This patch adds the device tree support for the Qualcomm SCM firmware.
> > >
> > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > ---
> > >  .../devicetree/bindings/firmware/qcom,scm.txt      | 31 ++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > > new file mode 100644
> > > index 0000000..57b9b3a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
> > > @@ -0,0 +1,31 @@
> > > +QCOM Secure Channel Manager (SCM)
> > > +
> > > +Qualcomm processors include an interface to communicate to the secure firmware.
> > > +This interface allows for clients to request different types of actions.  These
> > > +can include CPU power up/down, HDCP requests, loading of firmware, and other
> > > +assorted actions.
> > > +
> > > +Required properties:
> > > +- compatible: must contain one of the following:
> > > + * "qcom,scm-apq8064" for APQ8064
> > > + * "qcom,scm-apq8084" for MSM8084
> > > + * "qcom,scm-msm8916" for MSM8916
> > > + * "qcom,scm-msm8974" for MSM8974
> > > +- clocks: One to three clocks may be required based on compatible.
> > > + * Only core clock required for "qcom,scm-apq8064"
> > > + * Core, iface, and bus clocks required for all other compatibles.
> > > +- clock-names: Must contain "core" for the core clock, "iface" for the interface
> > > +  clock and "bus" for the bus clock per the requirements of the compatible.
> > > +
> > > +Example for MSM8916:
> > > +
> > > +       firmware {
> > > +               compatible = "simple-bus";
> > 
> > Firmware is a bus? Really? Let's not put hacks in the DT just so you
> > get automatic probing.
> 
> So something like:
> 
>         firmware {
>                 compatible = "qcom,scm-apq8084";
>                 clocks = <&gcc GCC_CE1_CLK> , <&gcc GCC_CE1_AXI_CLK>, <&gcc GCC_CE1_AHB_CLK>;
>                 clock-names = "core", "bus", "iface";
>         };
> 
> Seems to work fine.

Yes, because the top level nodes are probed. But then you can't have any 
other firmware nodes. You are going to have to call of_platform_populate 
on the /firmware node or create the device yourself. If there are other 
users of /firmware needing to probe, then we can perhaps do it 
generically.


> > > +
> > > +               scm {
> > > +                       compatible = "qcom,scm-msm8916";
> > > +                       clocks = <&gcc GCC_CRYPTO_CLK> , <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
> > > +                       clock-names = "core", "bus", "iface";
> > 
> > Generally, /firmware defines an interface to firmware. I don't think
> > clocks belong here. This implies that non-secure world can turn off
> > clocks to secure world?
> 
> The caller into the SCM is on the hook for making sure the clocks are turned on.
> The firmware people decided to not manage the clocks.  In a perfect world, they
> would turn on their own clocks and it would all be self contained.  Sadly, it
> isn't going to change.

Okay. Seems like a security problem to me though.

Rob

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
new file mode 100644
index 0000000..57b9b3a
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
@@ -0,0 +1,31 @@ 
+QCOM Secure Channel Manager (SCM)
+
+Qualcomm processors include an interface to communicate to the secure firmware.
+This interface allows for clients to request different types of actions.  These
+can include CPU power up/down, HDCP requests, loading of firmware, and other
+assorted actions.
+
+Required properties:
+- compatible: must contain one of the following:
+ * "qcom,scm-apq8064" for APQ8064
+ * "qcom,scm-apq8084" for MSM8084
+ * "qcom,scm-msm8916" for MSM8916
+ * "qcom,scm-msm8974" for MSM8974
+- clocks: One to three clocks may be required based on compatible.
+ * Only core clock required for "qcom,scm-apq8064"
+ * Core, iface, and bus clocks required for all other compatibles.
+- clock-names: Must contain "core" for the core clock, "iface" for the interface
+  clock and "bus" for the bus clock per the requirements of the compatible.
+
+Example for MSM8916:
+
+	firmware {
+		compatible = "simple-bus";
+
+		scm {
+			compatible = "qcom,scm-msm8916";
+			clocks = <&gcc GCC_CRYPTO_CLK> , <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
+			clock-names = "core", "bus", "iface";
+		};
+	};
+