| Message ID | 1453127331-20616-4-git-send-email-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, 01 Feb 2016, Maxime Ripard wrote: > On Wed, Jan 27, 2016 at 11:51:45PM +0000, André Przywara wrote: > > Hi, > > > > On 18/01/16 14:28, Lee Jones wrote: > > > This call matches clocks which have been marked as critical in DT > > > and sets the appropriate flag. These flags can then be used to > > > mark the clock core flags appropriately prior to registration. > > > > I like the idea of having a generic property very much. Also this solves > > a problem I have in a very elegant way. > > Not really. It has a significant set of drawbacks that we already > detailed in the initial thread, which are mostly related to the fact > that the clocks are to be left on is something that totally depends on > the software support in the kernel. Some clocks should be reported as > critical because they are simply missing a driver for it, some should > be because the driver for it as not been compiled, some should because > we don't have the proper clocks drivers yet for one of their > downstream clocks. Exactly. This is a not a CLK_DRIVER_NOT_{AUTHORED|UPSTREAM} or CLK_DRIVER_NOT_ENABLED implementation, it's for CLK_CRITICALs. Critical clocks must _never_ be turned off, no matter what, else something really bad will happen. In our use-case, if the clocks are turned of, it will be catastrophic to the running system. > Basically, it all boils down to this: some clocks should never ever be > shutdown because <hardware reason>, and I believe it's the case Lee is > in. But most of the current code that would use it might, and might > even need at some point to shut down such a clock. > > Mike's solution with the flags + handover was solving all this, I'm > not sure why he's not pushed it forward. Right, but I think you are missing part of the conversation. Mike and I had a face-to-face meeting in San Francisco last year. The conclusion was that the CLK_CRITICAL and CLK_HANDOVER solutions should be separated. Different handling, different code. This submission only solves the former problem. I believe Mike was going to submit and follow-up on the CLK_HANDOVER solution separately. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Tue, 02 Feb 2016, Andre Przywara wrote: > Hi Maxime, > > On 01/02/16 06:32, Maxime Ripard wrote: > > Hi Andre, > > > > On Wed, Jan 27, 2016 at 11:51:45PM +0000, André Przywara wrote: > >> Hi, > >> > >> On 18/01/16 14:28, Lee Jones wrote: > >>> This call matches clocks which have been marked as critical in DT > >>> and sets the appropriate flag. These flags can then be used to > >>> mark the clock core flags appropriately prior to registration. > >> > >> I like the idea of having a generic property very much. Also this solves > >> a problem I have in a very elegant way. > > > > Not really. It has a significant set of drawbacks that we already > > detailed in the initial thread, which are mostly related to the fact > > that the clocks are to be left on is something that totally depends on > > the software support in the kernel. Some clocks should be reported as > > critical because they are simply missing a driver for it, some should > > be because the driver for it as not been compiled, some should because > > we don't have the proper clocks drivers yet for one of their > > downstream clocks. > > > > Basically, it all boils down to this: some clocks should never ever be > > shutdown because <hardware reason>, and I believe it's the case Lee is > > in. But most of the current code that would use it might, and might > > even need at some point to shut down such a clock. > > I was bascically interested in pushing the critical-clock property into > DT to solve that cumbersome clk-sunxi init scheme - which you have fixed > now in a much better way (thanks for that, btw.) > For that particular case the CPU clock really looks like being actually > critical in the hardware sense - no-one maybe except the mgmt core > should turn the one single CPU clock source off. > > So I wonder if we should document this "for hardware reasons only" and > still have that property in DT? > At the weekend I coded something into the generic DT clock code to let > it parse for basically every clock node - without a particular driver > needing to ask for it. > If this sounds useful to you I can post that one. It sounds very useful. Very useful indeed. But then I would say that, because that's how this all started in the first place: ;) https://lkml.org/lkml/2015/7/22/299 I still think it's a pretty elegant method, but it was NACKed by Mike. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index ffa0b2e..6f178b7 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -707,6 +707,23 @@ const char *of_clk_get_parent_name(struct device_node *np, int index); void of_clk_init(const struct of_device_id *matches); +static inline int of_clk_mark_if_critical(struct device_node *np, + int index, unsigned long *flags) +{ + struct property *prop; + const __be32 *cur; + uint32_t idx; + + if (!np || !flags) + return -EINVAL; + + of_property_for_each_u32(np, "critical-clock", prop, cur, idx) + if (index == idx) + *flags |= CLK_IS_CRITICAL; + + return 0; +} + #else /* !CONFIG_OF */ static inline int of_clk_add_provider(struct device_node *np, @@ -742,6 +759,11 @@ static inline const char *of_clk_get_parent_name(struct device_node *np, { return NULL; } +static inline int of_clk_mark_if_critical(struct device_node *np, int index, + unsigned long *flags) +{ + return 0; +} #define of_clk_init(matches) \ { while (0); } #endif /* CONFIG_OF */
This call matches clocks which have been marked as critical in DT and sets the appropriate flag. These flags can then be used to mark the clock core flags appropriately prior to registration. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- include/linux/clk-provider.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) -- 1.9.1