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 <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Taniya Das <[email protected]>
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
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 <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
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
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 <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Cc: Taniya Das <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
.../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
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 <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> Cc: Taniya Das <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> .../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
>
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 <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Tested-by: Bjorn Andersson <[email protected]>
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
>
As of v4.20-rc1 probing the GCC driver on a SDM845 device with the
standard security implementation causes an access violation and an
immediate system restart. Use the protected-clocks property to mark the
offending clocks protected for the MTP, in order to allow it to boot.
Cc: Stephen Boyd <[email protected]>
Signed-off-by: Bjorn Andersson <[email protected]>
---
This depends on the acceptance of
https://lore.kernel.org/lkml/[email protected]/
arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
index d667eee4e6d0..b3def0358177 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
@@ -343,6 +343,12 @@
};
};
+&gcc {
+ protected-clocks = <GCC_QSPI_CORE_CLK>,
+ <GCC_QSPI_CORE_CLK_SRC>,
+ <GCC_QSPI_CNOC_PERIPH_AHB_CLK>;
+};
+
&i2c10 {
status = "okay";
clock-frequency = <400000>;
--
2.18.0
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 <[email protected]>
> > Cc: Bjorn Andersson <[email protected]>
>
> Reviewed-by: Bjorn Andersson <[email protected]>
>
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 <[email protected]>
> > Signed-off-by: Stephen Boyd <[email protected]>
> > ---
> > .../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
> >
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 <[email protected]>
> > > Cc: Bjorn Andersson <[email protected]>
> >
> > Reviewed-by: Bjorn Andersson <[email protected]>
> >
>
> 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.
On Mon, Nov 05, 2018 at 09:50:13PM -0800, Bjorn Andersson wrote:
> As of v4.20-rc1 probing the GCC driver on a SDM845 device with the
> standard security implementation causes an access violation and an
> immediate system restart. Use the protected-clocks property to mark the
> offending clocks protected for the MTP, in order to allow it to boot.
>
> Cc: Stephen Boyd <[email protected]>
> Signed-off-by: Bjorn Andersson <[email protected]>
This looks fine to me.
Acked-by: Andy Gross <[email protected]>
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 <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Cc: Taniya Das <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
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 <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
Quoting Bjorn Andersson (2018-11-05 21:50:13)
> As of v4.20-rc1 probing the GCC driver on a SDM845 device with the
> standard security implementation causes an access violation and an
> immediate system restart. Use the protected-clocks property to mark the
> offending clocks protected for the MTP, in order to allow it to boot.
>
> Cc: Stephen Boyd <[email protected]>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
> This depends on the acceptance of
> https://lore.kernel.org/lkml/[email protected]/
Do you need me to merge this into clk-fixes so that Andy can send this
up for v4.20 final? I thought you may have other boot blocking issues so
this wouldn't be a critical fix.
Quoting Bjorn Andersson (2018-11-05 21:50:13)
> As of v4.20-rc1 probing the GCC driver on a SDM845 device with the
> standard security implementation causes an access violation and an
> immediate system restart. Use the protected-clocks property to mark the
> offending clocks protected for the MTP, in order to allow it to boot.
>
> Cc: Stephen Boyd <[email protected]>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
>
Sorry, I meant to say do you want me to send in the protected clk
patches into clk-fixes so it can be merged for v4.20 final. I expected
Andy to pick up these dts patches.
On Wed 21 Nov 01:01 PST 2018, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2018-11-05 21:50:13)
> > As of v4.20-rc1 probing the GCC driver on a SDM845 device with the
> > standard security implementation causes an access violation and an
> > immediate system restart. Use the protected-clocks property to mark the
> > offending clocks protected for the MTP, in order to allow it to boot.
> >
> > Cc: Stephen Boyd <[email protected]>
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> >
> > This depends on the acceptance of
> > https://lore.kernel.org/lkml/[email protected]/
>
> Do you need me to merge this into clk-fixes so that Andy can send this
> up for v4.20 final? I thought you may have other boot blocking issues so
> this wouldn't be a critical fix.
>
We resolved the gpio-related issues, so iirc this is the only other item
preventing the MTP from booting. So yes please.
Unless you enable USB support, because configuring the first USB
controller in host currently crashes the device - I've not yet found the
cause for this though.
Regards,
Bjorn
Quoting Bjorn Andersson (2018-11-21 23:30:43)
> On Wed 21 Nov 01:01 PST 2018, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2018-11-05 21:50:13)
> > > As of v4.20-rc1 probing the GCC driver on a SDM845 device with the
> > > standard security implementation causes an access violation and an
> > > immediate system restart. Use the protected-clocks property to mark the
> > > offending clocks protected for the MTP, in order to allow it to boot.
> > >
> > > Cc: Stephen Boyd <[email protected]>
> > > Signed-off-by: Bjorn Andersson <[email protected]>
> > > ---
> > >
> > > This depends on the acceptance of
> > > https://lore.kernel.org/lkml/[email protected]/
> >
> > Do you need me to merge this into clk-fixes so that Andy can send this
> > up for v4.20 final? I thought you may have other boot blocking issues so
> > this wouldn't be a critical fix.
> >
>
> We resolved the gpio-related issues, so iirc this is the only other item
> preventing the MTP from booting. So yes please.
>
> Unless you enable USB support, because configuring the first USB
> controller in host currently crashes the device - I've not yet found the
> cause for this though.
>
Ok let me do the necessary shuffling to get this all lined up for
merging later this week, including this dts patch.