[PATCH 0/2] Introduce a 'protected-clocks' property

[PATCH 0/2] Introduce a 'protected-clocks' property

[Stephen Boyd]

See full explanation in patch #1. It looks like on qcom platforms
we're increasing the number of situations where we need to have
a set of clks that aren't touched by the OS because firmware
wants them for itself. This series introduces a method to do that
by specifying in DT what clks shouldn't be read or written and
then having the qcom clk driver neve register the ones that 
are protected.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Taniya Das <tdas@codeaurora.org>

Stephen Boyd (2):
  dt-bindings: clk: Introduce 'protected-clocks' property
  clk: qcom: Support 'protected-clocks' property

 .../bindings/clock/clock-bindings.txt          | 16 ++++++++++++++++
 drivers/clk/qcom/common.c                      | 18 ++++++++++++++++++
 2 files changed, 34 insertions(+)

-- 
Sent by a computer through tubes

[PATCH 1/2] dt-bindings: clk: Introduce 'protected-clocks' property

[Stephen Boyd]

Add a generic clk property for clks which are not intended to be used by
the OS due to security restrictions put in place by firmware. For
example, on some Qualcomm firmwares reading or writing certain clk
registers causes the entire system to reboot, but on other firmwares
reading and writing those same registers is required to make devices
like QSPI work. Rather than adding one-off properties each time a new
set of clks appears to be protected, let's add a generic clk property to
describe any set of clks that shouldn't be touched by the OS. This way
we never need to register the clks or use them in certain firmware
configurations.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Taniya Das <tdas@codeaurora.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 .../devicetree/bindings/clock/clock-bindings.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
index 2ec489eebe72..b646bbcf7f92 100644
--- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
+++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
@@ -168,3 +168,19 @@ a shared clock is forbidden.
 
 Configuration of common clocks, which affect multiple consumer devices can
 be similarly specified in the clock provider node.
+
+==Protected clocks==
+
+Some platforms or firmwares may not fully expose all the clocks to the OS, such
+as in situations where those clks are used by drivers running in ARM secure
+execution levels. Such a configuration can be specified in device tree with the
+protected-clocks property in the form of a clock specifier list. This property should
+only be specified in the node that is providing the clocks being protected:
+
+   clock-controller@a000f000 {
+        compatible = "vendor,clk95;
+        reg = <0xa000f000 0x1000>
+        #clocks-cells = <1>;
+        ...
+        protected-clocks = <UART3_CLK>, <SPI5_CLK>;
+   };
-- 
Sent by a computer through tubes

Re: [PATCH 1/2] dt-bindings: clk: Introduce 'protected-clocks' property

[Bjorn Andersson]

On Mon 05 Nov 11:40 PST 2018, Stephen Boyd wrote:

> Add a generic clk property for clks which are not intended to be used by
> the OS due to security restrictions put in place by firmware. For
> example, on some Qualcomm firmwares reading or writing certain clk
> registers causes the entire system to reboot, but on other firmwares
> reading and writing those same registers is required to make devices
> like QSPI work. Rather than adding one-off properties each time a new
> set of clks appears to be protected, let's add a generic clk property to
> describe any set of clks that shouldn't be touched by the OS. This way
> we never need to register the clks or use them in certain firmware
> configurations.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>

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

Regards,
Bjorn

> Cc: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  .../devicetree/bindings/clock/clock-bindings.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> index 2ec489eebe72..b646bbcf7f92 100644
> --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> @@ -168,3 +168,19 @@ a shared clock is forbidden.
>  
>  Configuration of common clocks, which affect multiple consumer devices can
>  be similarly specified in the clock provider node.
> +
> +==Protected clocks==
> +
> +Some platforms or firmwares may not fully expose all the clocks to the OS, such
> +as in situations where those clks are used by drivers running in ARM secure
> +execution levels. Such a configuration can be specified in device tree with the
> +protected-clocks property in the form of a clock specifier list. This property should
> +only be specified in the node that is providing the clocks being protected:
> +
> +   clock-controller@a000f000 {
> +        compatible = "vendor,clk95;
> +        reg = <0xa000f000 0x1000>
> +        #clocks-cells = <1>;
> +        ...
> +        protected-clocks = <UART3_CLK>, <SPI5_CLK>;
> +   };
> -- 
> Sent by a computer through tubes
> 

Re: [PATCH 1/2] dt-bindings: clk: Introduce 'protected-clocks' property

[Bjorn Andersson]

On Mon 05 Nov 17:04 PST 2018, Bjorn Andersson wrote:

> On Mon 05 Nov 11:40 PST 2018, Stephen Boyd wrote:
> 
> > Add a generic clk property for clks which are not intended to be used by
> > the OS due to security restrictions put in place by firmware. For
> > example, on some Qualcomm firmwares reading or writing certain clk
> > registers causes the entire system to reboot, but on other firmwares
> > reading and writing those same registers is required to make devices
> > like QSPI work. Rather than adding one-off properties each time a new
> > set of clks appears to be protected, let's add a generic clk property to
> > describe any set of clks that shouldn't be touched by the OS. This way
> > we never need to register the clks or use them in certain firmware
> > configurations.
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 

Gave this some additional thought. The way this is blacklisting
protected clocks makes it impossible to be backwards compatible with an
older DT while adding new protected clocks to an existing driver.

I don't have better suggestion for handling this and the problem should
primarily be isolated to the beginning of the upstream life of a
platform, so perhaps we can just ignore this issue?

Regards,
Bjorn

> Regards,
> Bjorn
> 
> > Cc: Taniya Das <tdas@codeaurora.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  .../devicetree/bindings/clock/clock-bindings.txt | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/clock-bindings.txt b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > index 2ec489eebe72..b646bbcf7f92 100644
> > --- a/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +++ b/Documentation/devicetree/bindings/clock/clock-bindings.txt
> > @@ -168,3 +168,19 @@ a shared clock is forbidden.
> >  
> >  Configuration of common clocks, which affect multiple consumer devices can
> >  be similarly specified in the clock provider node.
> > +
> > +==Protected clocks==
> > +
> > +Some platforms or firmwares may not fully expose all the clocks to the OS, such
> > +as in situations where those clks are used by drivers running in ARM secure
> > +execution levels. Such a configuration can be specified in device tree with the
> > +protected-clocks property in the form of a clock specifier list. This property should
> > +only be specified in the node that is providing the clocks being protected:
> > +
> > +   clock-controller@a000f000 {
> > +        compatible = "vendor,clk95;
> > +        reg = <0xa000f000 0x1000>
> > +        #clocks-cells = <1>;
> > +        ...
> > +        protected-clocks = <UART3_CLK>, <SPI5_CLK>;
> > +   };
> > -- 
> > Sent by a computer through tubes
> > 

Re: [PATCH 1/2] dt-bindings: clk: Introduce 'protected-clocks' property

[Stephen Boyd]

Quoting Bjorn Andersson (2018-11-07 21:44:52)
> On Mon 05 Nov 17:04 PST 2018, Bjorn Andersson wrote:
> 
> > On Mon 05 Nov 11:40 PST 2018, Stephen Boyd wrote:
> > 
> > > Add a generic clk property for clks which are not intended to be used by
> > > the OS due to security restrictions put in place by firmware. For
> > > example, on some Qualcomm firmwares reading or writing certain clk
> > > registers causes the entire system to reboot, but on other firmwares
> > > reading and writing those same registers is required to make devices
> > > like QSPI work. Rather than adding one-off properties each time a new
> > > set of clks appears to be protected, let's add a generic clk property to
> > > describe any set of clks that shouldn't be touched by the OS. This way
> > > we never need to register the clks or use them in certain firmware
> > > configurations.
> > > 
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> 
> Gave this some additional thought. The way this is blacklisting
> protected clocks makes it impossible to be backwards compatible with an
> older DT while adding new protected clocks to an existing driver.
> 
> I don't have better suggestion for handling this and the problem should
> primarily be isolated to the beginning of the upstream life of a
> platform, so perhaps we can just ignore this issue?
> 

I don't have a better suggestion either besides listing all possible
clks in the binding and header file to start and then specifying any
clks that are known to not work when merging the dts bits. In the future
I think we can move dts and drivers in lockstep if certain drivers add
new clks and those cause problems on locked down systems. The
alternative seems worse, i.e. listing clks that should be added on the
unaffected platforms, so ignoring this issue sounds good for now.

Re: [PATCH 1/2] dt-bindings: clk: Introduce 'protected-clocks' property

[Stephen Boyd]

Quoting Stephen Boyd (2018-11-05 11:40:10)
> Add a generic clk property for clks which are not intended to be used by
> the OS due to security restrictions put in place by firmware. For
> example, on some Qualcomm firmwares reading or writing certain clk
> registers causes the entire system to reboot, but on other firmwares
> reading and writing those same registers is required to make devices
> like QSPI work. Rather than adding one-off properties each time a new
> set of clks appears to be protected, let's add a generic clk property to
> describe any set of clks that shouldn't be touched by the OS. This way
> we never need to register the clks or use them in certain firmware
> configurations.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---

Applied to clk-next

[PATCH 2/2] clk: qcom: Support 'protected-clocks' property

[Stephen Boyd]

Certain firmware configurations "protect" clks and cause the entire
system to reboot when a non-secure OS such as Linux tries to read or
write protected clk registers. But other firmware configurations allow
reading or writing the same registers, and they may actually require
that the OS use the otherwise locked down clks. Support the
'protected-clocks' property by never registering these protected clks
with the common clk framework. This way, when firmware is protecting
these clks we won't have the chance to ever read or write these
registers and take down the entire system.

Cc: Taniya Das <tdas@codeaurora.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/clk/qcom/common.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index db9b2471ac40..0a48ed56833b 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -191,6 +191,22 @@ int qcom_cc_register_sleep_clk(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(qcom_cc_register_sleep_clk);
 
+/* Drop 'protected-clocks' from the list of clocks to register */
+static void qcom_cc_drop_protected(struct device *dev, struct qcom_cc *cc)
+{
+	struct device_node *np = dev->of_node;
+	struct property *prop;
+	const __be32 *p;
+	u32 i;
+
+	of_property_for_each_u32(np, "protected-clocks", prop, p, i) {
+		if (i >= cc->num_rclks)
+			continue;
+
+		cc->rclks[i] = NULL;
+	}
+}
+
 static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
 					 void *data)
 {
@@ -251,6 +267,8 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 	cc->rclks = rclks;
 	cc->num_rclks = num_clks;
 
+	qcom_cc_drop_protected(dev, cc);
+
 	for (i = 0; i < num_clks; i++) {
 		if (!rclks[i])
 			continue;
-- 
Sent by a computer through tubes

Re: [PATCH 2/2] clk: qcom: Support 'protected-clocks' property

[Bjorn Andersson]

On Mon 05 Nov 11:40 PST 2018, Stephen Boyd wrote:

> Certain firmware configurations "protect" clks and cause the entire
> system to reboot when a non-secure OS such as Linux tries to read or
> write protected clk registers. But other firmware configurations allow
> reading or writing the same registers, and they may actually require
> that the OS use the otherwise locked down clks. Support the
> 'protected-clocks' property by never registering these protected clks
> with the common clk framework. This way, when firmware is protecting
> these clks we won't have the chance to ever read or write these
> registers and take down the entire system.
> 
> Cc: Taniya Das <tdas@codeaurora.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

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

Regards,
Bjorn

> ---
>  drivers/clk/qcom/common.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index db9b2471ac40..0a48ed56833b 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -191,6 +191,22 @@ int qcom_cc_register_sleep_clk(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(qcom_cc_register_sleep_clk);
>  
> +/* Drop 'protected-clocks' from the list of clocks to register */
> +static void qcom_cc_drop_protected(struct device *dev, struct qcom_cc *cc)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct property *prop;
> +	const __be32 *p;
> +	u32 i;
> +
> +	of_property_for_each_u32(np, "protected-clocks", prop, p, i) {
> +		if (i >= cc->num_rclks)
> +			continue;
> +
> +		cc->rclks[i] = NULL;
> +	}
> +}
> +
>  static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
>  					 void *data)
>  {
> @@ -251,6 +267,8 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>  	cc->rclks = rclks;
>  	cc->num_rclks = num_clks;
>  
> +	qcom_cc_drop_protected(dev, cc);
> +
>  	for (i = 0; i < num_clks; i++) {
>  		if (!rclks[i])
>  			continue;
> -- 
> Sent by a computer through tubes
> 

Re: [PATCH 2/2] clk: qcom: Support 'protected-clocks' property

[Stephen Boyd]

Quoting Stephen Boyd (2018-11-05 11:40:11)
> Certain firmware configurations "protect" clks and cause the entire
> system to reboot when a non-secure OS such as Linux tries to read or
> write protected clk registers. But other firmware configurations allow
> reading or writing the same registers, and they may actually require
> that the OS use the otherwise locked down clks. Support the
> 'protected-clocks' property by never registering these protected clks
> with the common clk framework. This way, when firmware is protecting
> these clks we won't have the chance to ever read or write these
> registers and take down the entire system.
> 
> Cc: Taniya Das <tdas@codeaurora.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---

Applied to clk-next