* [PATCH RESEND 0/2] Common protected-clocks implementation @ 2020-09-03 4:00 Samuel Holland 2020-09-03 4:00 ` [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF clock providers Samuel Holland ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Samuel Holland @ 2020-09-03 4:00 UTC (permalink / raw) To: Stephen Boyd, Michael Turquette, Andy Gross, Bjorn Andersson, Maxime Ripard Cc: linux-arm-msm, linux-clk, linux-kernel, Samuel Holland Stephen, Maxime, You previously asked me to implement the protected-clocks property in a driver-independent way: https://www.spinics.net/lists/arm-kernel/msg753832.html I provided an implementation 6 months ago, which I am resending now: https://patchwork.kernel.org/patch/11398629/ Do you have any comments on it? Thanks, Samuel Samuel Holland (2): clk: Implement protected-clocks for all OF clock providers Revert "clk: qcom: Support 'protected-clocks' property" drivers/clk/clk-conf.c | 54 +++++++++++++++++++++++++++++++++++++++ drivers/clk/clk.c | 31 ++++++++++++++++++++++ drivers/clk/clk.h | 2 ++ drivers/clk/qcom/common.c | 18 ------------- 4 files changed, 87 insertions(+), 18 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF clock providers 2020-09-03 4:00 [PATCH RESEND 0/2] Common protected-clocks implementation Samuel Holland @ 2020-09-03 4:00 ` Samuel Holland 2020-09-03 7:31 ` Maxime Ripard 2020-09-03 4:00 ` [PATCH RESEND 2/2] Revert "clk: qcom: Support 'protected-clocks' property" Samuel Holland 2021-03-09 8:03 ` [PATCH RESEND 0/2] Common protected-clocks implementation Rasmus Villemoes 2 siblings, 1 reply; 8+ messages in thread From: Samuel Holland @ 2020-09-03 4:00 UTC (permalink / raw) To: Stephen Boyd, Michael Turquette, Andy Gross, Bjorn Andersson, Maxime Ripard Cc: linux-arm-msm, linux-clk, linux-kernel, Samuel Holland This is a generic implementation of the "protected-clocks" property from the common clock binding. It allows firmware to inform the OS about clocks that must not be disabled while the OS is running. This implementation comes with some caveats: 1) Clocks that have CLK_IS_CRITICAL in their init data are prepared/ enabled before they are attached to the clock tree. protected-clocks are only protected once the clock provider is added, which is generally after all of the clocks it provides have been registered. This leaves a window of opportunity where something could disable or modify the clock, such as a driver running on another CPU, or the clock core itself. There is a comment to this effect in __clk_core_init(): /* * Enable CLK_IS_CRITICAL clocks so newly added critical clocks * don't get accidentally disabled when walking the orphan tree and * reparenting clocks */ Similarly, these clocks will be enabled after they are first reparented, unlike other CLK_IS_CRITICAL clocks. See the comment in clk_core_reparent_orphans_nolock(): /* * We need to use __clk_set_parent_before() and _after() to * to properly migrate any prepare/enable count of the orphan * clock. This is important for CLK_IS_CRITICAL clocks, which * are enabled during init but might not have a parent yet. */ Ideally we could detect protected clocks before they are reparented, but there are two problems with that: a) From the clock core's perspective, hw->init is const. b) The clock core doesn't see the device_node until __clk_register is called on the first clock. So the only "race-free" way to detect protected-clocks is to do it in the middle of __clk_register, between when core->flags is initialized and calling __clk_core_init(). That requires scanning the device tree again for each clock, which is part of why I didn't do it that way. 2) __clk_protect needs to be idempotent, for two reasons: a) Clocks with CLK_IS_CRITICAL in their init data are already prepared/enabled, and we don't want to prepare/enable them again. b) of_clk_set_defaults() is called twice for (at least some) clock controllers registered with CLK_OF_DECLARE. It is called first in of_clk_add_provider()/of_clk_add_hw_provider() inside clk_init_cb, and again afterward in of_clk_init(). The second call in of_clk_init() may be unnecessary, but verifying that would require auditing all users of CLK_OF_DECLARE to ensure they called one of the of_clk_add{,_hw}_provider functions. Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/clk/clk-conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++ drivers/clk/clk.c | 31 ++++++++++++++++++++++++ drivers/clk/clk.h | 2 ++ 3 files changed, 87 insertions(+) diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c index 2ef819606c41..a57d28b0f397 100644 --- a/drivers/clk/clk-conf.c +++ b/drivers/clk/clk-conf.c @@ -11,6 +11,54 @@ #include <linux/of.h> #include <linux/printk.h> +#include "clk.h" + +static int __set_clk_flags(struct device_node *node) +{ + struct of_phandle_args clkspec; + struct property *prop; + int i, index = 0, rc; + const __be32 *cur; + struct clk *clk; + u32 nr_cells; + + rc = of_property_read_u32(node, "#clock-cells", &nr_cells); + if (rc < 0) { + pr_err("clk: missing #clock-cells property on %pOF\n", node); + return rc; + } + + clkspec.np = node; + clkspec.args_count = nr_cells; + + of_property_for_each_u32(node, "protected-clocks", prop, cur, clkspec.args[0]) { + /* read the remainder of the clock specifier */ + for (i = 1; i < nr_cells; ++i) { + cur = of_prop_next_u32(prop, cur, &clkspec.args[i]); + if (!cur) { + pr_err("clk: invalid value of protected-clocks" + " property at %pOF\n", node); + return -EINVAL; + } + } + clk = of_clk_get_from_provider(&clkspec); + if (IS_ERR(clk)) { + if (PTR_ERR(clk) != -EPROBE_DEFER) + pr_err("clk: couldn't get protected clock" + " %u for %pOF\n", index, node); + return PTR_ERR(clk); + } + + rc = __clk_protect(clk); + if (rc < 0) + pr_warn("clk: failed to protect %s: %d\n", + __clk_get_name(clk), rc); + clk_put(clk); + index++; + } + return 0; +} + static int __set_clk_parents(struct device_node *node, bool clk_supplier) { struct of_phandle_args clkspec; @@ -135,6 +183,12 @@ int of_clk_set_defaults(struct device_node *node, bool clk_supplier) if (!node) return 0; + if (clk_supplier) { + rc = __set_clk_flags(node); + if (rc < 0) + return rc; + } + rc = __set_clk_parents(node, clk_supplier); if (rc < 0) return rc; diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 0a9261a099bd..31fde39f1bda 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -4187,6 +4187,37 @@ void devm_clk_hw_unregister(struct device *dev, struct clk_hw *hw) } EXPORT_SYMBOL_GPL(devm_clk_hw_unregister); +/* + * clk-conf helpers + */ + +int __clk_protect(struct clk *clk) +{ + struct clk_core *core = clk->core; + int ret = 0; + + clk_prepare_lock(); + + /* + * If CLK_IS_CRITICAL was set in the clock's init data, then + * the clock was already prepared/enabled when it was added. + */ + if (core->flags & CLK_IS_CRITICAL) + goto out; + + core->flags |= CLK_IS_CRITICAL; + ret = clk_core_prepare(core); + if (ret) + goto out; + + ret = clk_core_enable_lock(core); + +out: + clk_prepare_unlock(); + + return ret; +} + /* * clkdev helpers */ diff --git a/drivers/clk/clk.h b/drivers/clk/clk.h index 2d801900cad5..367a0f036b13 100644 --- a/drivers/clk/clk.h +++ b/drivers/clk/clk.h @@ -24,6 +24,7 @@ struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id); #ifdef CONFIG_COMMON_CLK struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id, const char *con_id); +int __clk_protect(struct clk *clk); void __clk_put(struct clk *clk); #else /* All these casts to avoid ifdefs in clkdev... */ @@ -33,6 +34,7 @@ clk_hw_create_clk(struct device *dev, struct clk_hw *hw, const char *dev_id, { return (struct clk *)hw; } +static inline int __clk_protect(struct clk *clk) { return 0; } static inline void __clk_put(struct clk *clk) { } #endif -- 2.26.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF clock providers 2020-09-03 4:00 ` [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF clock providers Samuel Holland @ 2020-09-03 7:31 ` Maxime Ripard 0 siblings, 0 replies; 8+ messages in thread From: Maxime Ripard @ 2020-09-03 7:31 UTC (permalink / raw) To: Samuel Holland Cc: Stephen Boyd, Michael Turquette, Andy Gross, Bjorn Andersson, linux-arm-msm, linux-clk, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2899 bytes --] On Wed, Sep 02, 2020 at 11:00:14PM -0500, Samuel Holland wrote: > This is a generic implementation of the "protected-clocks" property from > the common clock binding. It allows firmware to inform the OS about > clocks that must not be disabled while the OS is running. > > This implementation comes with some caveats: > > 1) Clocks that have CLK_IS_CRITICAL in their init data are prepared/ > enabled before they are attached to the clock tree. protected-clocks are > only protected once the clock provider is added, which is generally > after all of the clocks it provides have been registered. This leaves a > window of opportunity where something could disable or modify the clock, > such as a driver running on another CPU, or the clock core itself. There > is a comment to this effect in __clk_core_init(): > > /* > * Enable CLK_IS_CRITICAL clocks so newly added critical clocks > * don't get accidentally disabled when walking the orphan tree and > * reparenting clocks > */ > > Similarly, these clocks will be enabled after they are first reparented, > unlike other CLK_IS_CRITICAL clocks. See the comment in > clk_core_reparent_orphans_nolock(): > > /* > * We need to use __clk_set_parent_before() and _after() to > * to properly migrate any prepare/enable count of the orphan > * clock. This is important for CLK_IS_CRITICAL clocks, which > * are enabled during init but might not have a parent yet. > */ > > Ideally we could detect protected clocks before they are reparented, but > there are two problems with that: > > a) From the clock core's perspective, hw->init is const. > > b) The clock core doesn't see the device_node until __clk_register is > called on the first clock. > > So the only "race-free" way to detect protected-clocks is to do it in > the middle of __clk_register, between when core->flags is initialized > and calling __clk_core_init(). That requires scanning the device tree > again for each clock, which is part of why I didn't do it that way. > > 2) __clk_protect needs to be idempotent, for two reasons: > > a) Clocks with CLK_IS_CRITICAL in their init data are already > prepared/enabled, and we don't want to prepare/enable them again. > > b) of_clk_set_defaults() is called twice for (at least some) clock > controllers registered with CLK_OF_DECLARE. It is called first in > of_clk_add_provider()/of_clk_add_hw_provider() inside clk_init_cb, > and again afterward in of_clk_init(). The second call in > of_clk_init() may be unnecessary, but verifying that would require > auditing all users of CLK_OF_DECLARE to ensure they called one of > the of_clk_add{,_hw}_provider functions. > > Signed-off-by: Samuel Holland <samuel@sholland.org> Reviewed-by: Maxime Ripard <mripard@kernel.org> Thanks! Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH RESEND 2/2] Revert "clk: qcom: Support 'protected-clocks' property" 2020-09-03 4:00 [PATCH RESEND 0/2] Common protected-clocks implementation Samuel Holland 2020-09-03 4:00 ` [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF clock providers Samuel Holland @ 2020-09-03 4:00 ` Samuel Holland 2021-03-09 8:03 ` [PATCH RESEND 0/2] Common protected-clocks implementation Rasmus Villemoes 2 siblings, 0 replies; 8+ messages in thread From: Samuel Holland @ 2020-09-03 4:00 UTC (permalink / raw) To: Stephen Boyd, Michael Turquette, Andy Gross, Bjorn Andersson, Maxime Ripard Cc: linux-arm-msm, linux-clk, linux-kernel, Samuel Holland Now that protected-clocks is handled in the clk core, this driver-specific implementation is redundant. This reverts commit b181b3b801da8893c8eb706e448dd5111b02de60. Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/clk/qcom/common.c | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c index 60d2a78d1395..6e150fd32dbe 100644 --- a/drivers/clk/qcom/common.c +++ b/drivers/clk/qcom/common.c @@ -194,22 +194,6 @@ 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) { @@ -272,8 +256,6 @@ 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_clk_hws; i++) { ret = devm_clk_hw_register(dev, clk_hws[i]); if (ret) -- 2.26.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH RESEND 0/2] Common protected-clocks implementation 2020-09-03 4:00 [PATCH RESEND 0/2] Common protected-clocks implementation Samuel Holland 2020-09-03 4:00 ` [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF clock providers Samuel Holland 2020-09-03 4:00 ` [PATCH RESEND 2/2] Revert "clk: qcom: Support 'protected-clocks' property" Samuel Holland @ 2021-03-09 8:03 ` Rasmus Villemoes 2021-03-10 8:56 ` Maxime Ripard 2021-03-10 20:13 ` Bjorn Andersson 2 siblings, 2 replies; 8+ messages in thread From: Rasmus Villemoes @ 2021-03-09 8:03 UTC (permalink / raw) To: Samuel Holland, Stephen Boyd, Michael Turquette, Andy Gross, Bjorn Andersson, Maxime Ripard Cc: linux-arm-msm, linux-clk, linux-kernel, Rob Herring On 03/09/2020 06.00, Samuel Holland wrote: > Stephen, Maxime, > > You previously asked me to implement the protected-clocks property in a > driver-independent way: > > https://www.spinics.net/lists/arm-kernel/msg753832.html > > I provided an implementation 6 months ago, which I am resending now: > > https://patchwork.kernel.org/patch/11398629/ > > Do you have any comments on it? I'm also interested [1] in getting something like this supported in a generic fashion - i.e., being able to mark a clock as protected/critical/whatnot by just adding an appropriate property in the clock provider's DT node, but without modifying the driver to opt-in to handling it. Now, as to this implementation, the commit 48d7f160b1 which added the common protected-clocks binding says For example, on some Qualcomm firmwares reading or writing certain clk registers causes the entire system to reboot, so I'm not sure handling protected-clocks by translating it to CLK_CRITICAL and thus calling prepare/enable on it is the right thing to do - clks that behave like above are truly "hands off, kernel", so the current driver-specific implementation of simply not registering those clocks seems to be the right thing to do - or at least the clk framework would need to be taught to not actually call any methods on such protected clocks. For my use case, either "hands off kernel" or "make sure this clock is enabled" would work since the bootloader anyway enables the clock. Rasmus [1] https://lore.kernel.org/lkml/20210226141411.2517368-1-linux@rasmusvillemoes.dk/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RESEND 0/2] Common protected-clocks implementation 2021-03-09 8:03 ` [PATCH RESEND 0/2] Common protected-clocks implementation Rasmus Villemoes @ 2021-03-10 8:56 ` Maxime Ripard 2021-03-10 14:12 ` Samuel Holland 2021-03-10 20:13 ` Bjorn Andersson 1 sibling, 1 reply; 8+ messages in thread From: Maxime Ripard @ 2021-03-10 8:56 UTC (permalink / raw) To: Rasmus Villemoes Cc: Samuel Holland, Stephen Boyd, Michael Turquette, Andy Gross, Bjorn Andersson, linux-arm-msm, linux-clk, linux-kernel, Rob Herring [-- Attachment #1: Type: text/plain, Size: 2794 bytes --] Hi, On Tue, Mar 09, 2021 at 09:03:14AM +0100, Rasmus Villemoes wrote: > On 03/09/2020 06.00, Samuel Holland wrote: > > Stephen, Maxime, > > > > You previously asked me to implement the protected-clocks property in a > > driver-independent way: > > > > https://www.spinics.net/lists/arm-kernel/msg753832.html > > > > I provided an implementation 6 months ago, which I am resending now: > > > > https://patchwork.kernel.org/patch/11398629/ > > > > Do you have any comments on it? > > I'm also interested [1] in getting something like this supported in a > generic fashion - i.e., being able to mark a clock as > protected/critical/whatnot by just adding an appropriate property in the > clock provider's DT node, but without modifying the driver to opt-in to > handling it. > > Now, as to this implementation, the commit 48d7f160b1 which added the > common protected-clocks binding says > > For example, on some Qualcomm firmwares reading or writing certain clk > registers causes the entire system to reboot, > > so I'm not sure handling protected-clocks by translating it to > CLK_CRITICAL and thus calling prepare/enable on it is the right thing to > do - clks that behave like above are truly "hands off, kernel", so the > current driver-specific implementation of simply not registering those > clocks seems to be the right thing to do - or at least the clk framework > would need to be taught to not actually call any methods on such > protected clocks. The idea to use CLK_CRITICAL was also there to allow any clock to be marked as protected, and not just the root ones. Indeed, by just ignoring that clock, the parent clock could end up in a situation where it doesn't have any (registered) child and thus would be disabled, disabling the ignored clock as well. Reparenting could cause the same issue. Calling clk_prepare_enable just allows the kernel to track that it (and thus its parent) should never be disabled, ever. > For my use case, either "hands off kernel" or "make sure this clock is > enabled" would work since the bootloader anyway enables the clock. The current protected clocks semantics have been that the clock shouldn't be disabled by the kernel, but "hands off kernel" clocks imply a slightly different one. I would expect that the bootloader in this case wouldn't expect any parent or rate (or phase, or any other parameter really) change, while it might be ok for some other use cases (like the one Samuel was trying to address for example). And even if we wanted the kernel to behave that way (making sure there's no way to change the rate, parents, phase, etc.), the kernel would have to have knowledge of it to also propagate that restriction to the whole chain of parents. Maxim [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RESEND 0/2] Common protected-clocks implementation 2021-03-10 8:56 ` Maxime Ripard @ 2021-03-10 14:12 ` Samuel Holland 0 siblings, 0 replies; 8+ messages in thread From: Samuel Holland @ 2021-03-10 14:12 UTC (permalink / raw) To: Maxime Ripard, Rasmus Villemoes, Stephen Boyd Cc: Michael Turquette, Andy Gross, Bjorn Andersson, linux-arm-msm, linux-clk, linux-kernel, Rob Herring On 3/10/21 2:56 AM, Maxime Ripard wrote: > Hi, > > On Tue, Mar 09, 2021 at 09:03:14AM +0100, Rasmus Villemoes wrote: >> On 03/09/2020 06.00, Samuel Holland wrote: >>> Stephen, Maxime, >>> >>> You previously asked me to implement the protected-clocks property in a >>> driver-independent way: >>> >>> https://www.spinics.net/lists/arm-kernel/msg753832.html >>> >>> I provided an implementation 6 months ago, which I am resending now: >>> >>> https://patchwork.kernel.org/patch/11398629/ >>> >>> Do you have any comments on it? >> >> I'm also interested [1] in getting something like this supported in a >> generic fashion - i.e., being able to mark a clock as >> protected/critical/whatnot by just adding an appropriate property in the >> clock provider's DT node, but without modifying the driver to opt-in to >> handling it. >> >> Now, as to this implementation, the commit 48d7f160b1 which added the >> common protected-clocks binding says >> >> For example, on some Qualcomm firmwares reading or writing certain clk >> registers causes the entire system to reboot, >> >> so I'm not sure handling protected-clocks by translating it to >> CLK_CRITICAL and thus calling prepare/enable on it is the right thing to >> do - clks that behave like above are truly "hands off, kernel", so the >> current driver-specific implementation of simply not registering those >> clocks seems to be the right thing to do - or at least the clk framework >> would need to be taught to not actually call any methods on such >> protected clocks. > > The idea to use CLK_CRITICAL was also there to allow any clock to be > marked as protected, and not just the root ones. Indeed, by just > ignoring that clock, the parent clock could end up in a situation where > it doesn't have any (registered) child and thus would be disabled, > disabling the ignored clock as well. Reparenting could cause the same > issue. > > Calling clk_prepare_enable just allows the kernel to track that it (and > thus its parent) should never be disabled, ever. > >> For my use case, either "hands off kernel" or "make sure this clock is >> enabled" would work since the bootloader anyway enables the clock. > > The current protected clocks semantics have been that the clock > shouldn't be disabled by the kernel, but "hands off kernel" clocks imply > a slightly different one. I would expect that the bootloader in this > case wouldn't expect any parent or rate (or phase, or any other > parameter really) change, while it might be ok for some other use cases > (like the one Samuel was trying to address for example). Right. In my scenario, the clock is needed for MMIO access to a low-speed peripheral. I don't care what the clock rate is as long as it keeps running. When writing the patch, I initially protected the rate as well, but that had the unintended effect of protecting the rate of every ancestor clock. Maybe you need rate protection for a "hands off, kernel" type of clock? If so, ignoring the clock at probe time does not work, because the kernel does not know to protect its parent rate. So it sounds like the Qualcomm case is less about "don't touch the clock output" as it is "don't touch the clock control registers". > And even if we wanted the kernel to behave that way (making sure there's > no way to change the rate, parents, phase, etc.), the kernel would have > to have knowledge of it to also propagate that restriction to the whole > chain of parents. Yes, you can set additional flags in __clk_protect() to achieve that: CLK_SET_RATE_GATE, CLK_SET_PARENT_GATE. This is a bit of a trick; it relies on CLK_IS_CRITICAL preventing the clock from ever being gated. Cheers, Samuel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RESEND 0/2] Common protected-clocks implementation 2021-03-09 8:03 ` [PATCH RESEND 0/2] Common protected-clocks implementation Rasmus Villemoes 2021-03-10 8:56 ` Maxime Ripard @ 2021-03-10 20:13 ` Bjorn Andersson 1 sibling, 0 replies; 8+ messages in thread From: Bjorn Andersson @ 2021-03-10 20:13 UTC (permalink / raw) To: Rasmus Villemoes Cc: Samuel Holland, Stephen Boyd, Michael Turquette, Andy Gross, Maxime Ripard, linux-arm-msm, linux-clk, linux-kernel, Rob Herring On Tue 09 Mar 02:03 CST 2021, Rasmus Villemoes wrote: > On 03/09/2020 06.00, Samuel Holland wrote: > > Stephen, Maxime, > > > > You previously asked me to implement the protected-clocks property in a > > driver-independent way: > > > > https://www.spinics.net/lists/arm-kernel/msg753832.html > > > > I provided an implementation 6 months ago, which I am resending now: > > > > https://patchwork.kernel.org/patch/11398629/ > > > > Do you have any comments on it? > > I'm also interested [1] in getting something like this supported in a > generic fashion - i.e., being able to mark a clock as > protected/critical/whatnot by just adding an appropriate property in the > clock provider's DT node, but without modifying the driver to opt-in to > handling it. > > Now, as to this implementation, the commit 48d7f160b1 which added the > common protected-clocks binding says > > For example, on some Qualcomm firmwares reading or writing certain clk > registers causes the entire system to reboot, > > so I'm not sure handling protected-clocks by translating it to > CLK_CRITICAL and thus calling prepare/enable on it is the right thing to > do - clks that behave like above are truly "hands off, kernel", so the > current driver-specific implementation of simply not registering those > clocks seems to be the right thing to do - or at least the clk framework > would need to be taught to not actually call any methods on such > protected clocks. > I can confirm that this is the case. Marking the clocks as critical does not prevent the kernel from touching these registers so the boards where this is used doesn't boot with the two patches applied. > For my use case, either "hands off kernel" or "make sure this clock is > enabled" would work since the bootloader anyway enables the clock. > Our use case is that depending on firmware these platform might handle some specific clocks in firmware or in the kernel, and in the prior case security permissions are set up such that these registers are off limit for the kernel. Regards, Bjorn > Rasmus > > [1] > https://lore.kernel.org/lkml/20210226141411.2517368-1-linux@rasmusvillemoes.dk/ > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-03-10 20:14 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-03 4:00 [PATCH RESEND 0/2] Common protected-clocks implementation Samuel Holland 2020-09-03 4:00 ` [PATCH RESEND 1/2] clk: Implement protected-clocks for all OF clock providers Samuel Holland 2020-09-03 7:31 ` Maxime Ripard 2020-09-03 4:00 ` [PATCH RESEND 2/2] Revert "clk: qcom: Support 'protected-clocks' property" Samuel Holland 2021-03-09 8:03 ` [PATCH RESEND 0/2] Common protected-clocks implementation Rasmus Villemoes 2021-03-10 8:56 ` Maxime Ripard 2021-03-10 14:12 ` Samuel Holland 2021-03-10 20:13 ` Bjorn Andersson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.