| Message ID | 57f2182e-09b4-2cab-5602-9d0e029495be@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Sekhar Nori (2018-04-06 02:37:03) > > Can you please check that and confirm there is no issue with genpd and > using CLK_OF_DECLARE() to initialize clocks? > > Unless you report an issue back, or Mike and Stephen have ideas about > how to handle the dependency between PSC/PLL derived timer clock > initialization and and timer_probe(), I think we need to move back to > using CLK_OF_DECLARE(). In such a case, please use the hybrid approach where the clks required for the clockevent and/or clocksource are registered in the early CLK_OF_DECLARE path but the rest of the clks get registered with a proper platform device and driver. There are examples of this approach on other platforms already.
On 04/06/2018 11:46 AM, Stephen Boyd wrote: > Quoting Sekhar Nori (2018-04-06 02:37:03) >> >> Can you please check that and confirm there is no issue with genpd and >> using CLK_OF_DECLARE() to initialize clocks? >> >> Unless you report an issue back, or Mike and Stephen have ideas about >> how to handle the dependency between PSC/PLL derived timer clock >> initialization and and timer_probe(), I think we need to move back to >> using CLK_OF_DECLARE(). > > In such a case, please use the hybrid approach where the clks required > for the clockevent and/or clocksource are registered in the early > CLK_OF_DECLARE path but the rest of the clks get registered with a > proper platform device and driver. There are examples of this approach > on other platforms already. > I looked at this a bit last week, but I didn't come up with any approach that I was happy with. It seems like it would be nice to just register the absolute minimum clocks needed. On DA8XX, that would just be the PLL0 AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one LPSC clock. The AUXCLKs are easy because they are just a simple gate from the oscillator. The LPSC clocks are a bit more tricky because they have a complex sequence for turning on. Furthermore, on DM646X, we need the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit messy there. The other approach I considered was to make the PLL and PSC drivers work as platform devices or pure clock devices so that when needed, the whole IP block could be registered in very early init. However, this approach will pretty much require massive changes to the drivers to remove all of the uses of devm_, dev_err, etc. so that it works when dev == NULL. Any other bright ideas or a preference for one of these two approaches?
On Monday 23 April 2018 08:29 PM, David Lechner wrote: > On 04/06/2018 11:46 AM, Stephen Boyd wrote: >> Quoting Sekhar Nori (2018-04-06 02:37:03) >>> >>> Can you please check that and confirm there is no issue with genpd and >>> using CLK_OF_DECLARE() to initialize clocks? >>> >>> Unless you report an issue back, or Mike and Stephen have ideas about >>> how to handle the dependency between PSC/PLL derived timer clock >>> initialization and and timer_probe(), I think we need to move back to >>> using CLK_OF_DECLARE(). >> >> In such a case, please use the hybrid approach where the clks required >> for the clockevent and/or clocksource are registered in the early >> CLK_OF_DECLARE path but the rest of the clks get registered with a >> proper platform device and driver. There are examples of this approach >> on other platforms already. >> > > I looked at this a bit last week, but I didn't come up with any approach > that I was happy with. It seems like it would be nice to just register > the absolute minimum clocks needed. On DA8XX, that would just be the PLL0 > AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one > LPSC clock. The AUXCLKs are easy because they are just a simple gate > from the oscillator. The LPSC clocks are a bit more tricky because they > have a complex sequence for turning on. Furthermore, on DM646X, we need > the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit > messy there. Things might change in the context of work being done here by Bartosz for converting clocks to early platform devices. But, keeping that development aside for a moment: I think this means the PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform devices are clocks that are not in the path to get timer clock working (like CFGCHIP clocks). Thanks, Sekhar
On 04/24/2018 03:28 AM, Sekhar Nori wrote: > On Monday 23 April 2018 08:29 PM, David Lechner wrote: >> On 04/06/2018 11:46 AM, Stephen Boyd wrote: >>> Quoting Sekhar Nori (2018-04-06 02:37:03) >>>> >>>> Can you please check that and confirm there is no issue with genpd and >>>> using CLK_OF_DECLARE() to initialize clocks? >>>> >>>> Unless you report an issue back, or Mike and Stephen have ideas about >>>> how to handle the dependency between PSC/PLL derived timer clock >>>> initialization and and timer_probe(), I think we need to move back to >>>> using CLK_OF_DECLARE(). >>> >>> In such a case, please use the hybrid approach where the clks required >>> for the clockevent and/or clocksource are registered in the early >>> CLK_OF_DECLARE path but the rest of the clks get registered with a >>> proper platform device and driver. There are examples of this approach >>> on other platforms already. >>> >> >> I looked at this a bit last week, but I didn't come up with any approach >> that I was happy with. It seems like it would be nice to just register >> the absolute minimum clocks needed. On DA8XX, that would just be the PLL0 >> AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one >> LPSC clock. The AUXCLKs are easy because they are just a simple gate >> from the oscillator. The LPSC clocks are a bit more tricky because they >> have a complex sequence for turning on. Furthermore, on DM646X, we need >> the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit >> messy there. > > Things might change in the context of work being done here by Bartosz > for converting clocks to early platform devices. > > But, keeping that development aside for a moment: I think this means the > PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform > devices are clocks that are not in the path to get timer clock working > (like CFGCHIP clocks). CLK_OF_DECLARE() only matters on DA850. None of the other SoCs use device tree. And on DA850, the timer0 clock is just the PLL0 AUXCLK, so PSCs can still be platform devices there. And in fact, one of the PSC clocks on DA850 depends on async3, which is a CFGCHIP clock, so if we made the PSCs CLK_OF_DECLARE(), then we also have to make at least the async3 CFGCHIP clock CLK_OF_DECLARE(). But, like I said, I think that can be avoided by leaving the PSC clocks as a platform device on DA850 (and DA830). For everything else, we need the legacy board file equivalent of CLK_OF_DECLARE(), which is basically what I had here in v5 of the series. So, if we want this to keep moving before the if/when of Bartosz's early platform device stuff, I'm thinking that I will make the PLL and PSC drivers loadable with or without a platform device and let each SoC pick which one it should use.
On Tuesday 24 April 2018 09:41 PM, David Lechner wrote: > On 04/24/2018 03:28 AM, Sekhar Nori wrote: >> On Monday 23 April 2018 08:29 PM, David Lechner wrote: >>> On 04/06/2018 11:46 AM, Stephen Boyd wrote: >>>> Quoting Sekhar Nori (2018-04-06 02:37:03) >>>>> >>>>> Can you please check that and confirm there is no issue with genpd and >>>>> using CLK_OF_DECLARE() to initialize clocks? >>>>> >>>>> Unless you report an issue back, or Mike and Stephen have ideas about >>>>> how to handle the dependency between PSC/PLL derived timer clock >>>>> initialization and and timer_probe(), I think we need to move back to >>>>> using CLK_OF_DECLARE(). >>>> >>>> In such a case, please use the hybrid approach where the clks required >>>> for the clockevent and/or clocksource are registered in the early >>>> CLK_OF_DECLARE path but the rest of the clks get registered with a >>>> proper platform device and driver. There are examples of this approach >>>> on other platforms already. >>>> >>> >>> I looked at this a bit last week, but I didn't come up with any approach >>> that I was happy with. It seems like it would be nice to just register >>> the absolute minimum clocks needed. On DA8XX, that would just be the >>> PLL0 >>> AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one >>> LPSC clock. The AUXCLKs are easy because they are just a simple gate >>> from the oscillator. The LPSC clocks are a bit more tricky because they >>> have a complex sequence for turning on. Furthermore, on DM646X, we need >>> the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit >>> messy there. >> >> Things might change in the context of work being done here by Bartosz >> for converting clocks to early platform devices. >> >> But, keeping that development aside for a moment: I think this means the >> PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform >> devices are clocks that are not in the path to get timer clock working >> (like CFGCHIP clocks). > > CLK_OF_DECLARE() only matters on DA850. None of the other SoCs use device Thats true today, but lets not make the assumption that none of the other DaVinci SoCs will gain DT support ever. We don't want churn in CCF support once someone tries to add DT support for other platforms. I already have people using DM365 in mainline, for example. > tree. And on DA850, the timer0 clock is just the PLL0 AUXCLK, so PSCs can> still be platform devices there. And in fact, one of the PSC clocks on > DA850 depends on async3, which is a CFGCHIP clock, so if we made the PSCs > CLK_OF_DECLARE(), then we also have to make at least the async3 CFGCHIP > clock CLK_OF_DECLARE(). But, like I said, I think that can be avoided by > leaving the PSC clocks as a platform device on DA850 (and DA830). Okay, I did not realize that there is a dependency between CFGCHIP clk and PSC too. > For everything else, we need the legacy board file equivalent of > CLK_OF_DECLARE(), which is basically what I had here in v5 of the > series. Yeah. I did not realize the dependency with timer when platform devices were suggested. Sorry. > > So, if we want this to keep moving before the if/when of Bartosz's > early platform device stuff, I'm thinking that I will make the PLL After looking at feedback to Bartosz's series, and the fact that previous attempts at doing the same thing were rejected, I think there is benefit in keeping CCF support moving independent Bartosz's work. Otherwise, we have no chance of getting anything merged for v4.18. Early initialization of clocks and dependency with timer is a pretty common theme across SoCs. As and when a better way to support is found, I am sure DaVinci can be converted over along with rest of the devices. > and PSC drivers loadable with or without a platform device and let > each SoC pick which one it should use. This sounds flexible. We have to see how the patches look. But I would caution against adding a lot of dead code. If there is no way some of the clocks will be useful as platform device, I see no point of supporting them as such just in anticipation. Thanks, Sekhar
2018-04-25 8:07 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: > On Tuesday 24 April 2018 09:41 PM, David Lechner wrote: >> On 04/24/2018 03:28 AM, Sekhar Nori wrote: >>> On Monday 23 April 2018 08:29 PM, David Lechner wrote: >>>> On 04/06/2018 11:46 AM, Stephen Boyd wrote: >>>>> Quoting Sekhar Nori (2018-04-06 02:37:03) >>>>>> >>>>>> Can you please check that and confirm there is no issue with genpd and >>>>>> using CLK_OF_DECLARE() to initialize clocks? >>>>>> >>>>>> Unless you report an issue back, or Mike and Stephen have ideas about >>>>>> how to handle the dependency between PSC/PLL derived timer clock >>>>>> initialization and and timer_probe(), I think we need to move back to >>>>>> using CLK_OF_DECLARE(). >>>>> >>>>> In such a case, please use the hybrid approach where the clks required >>>>> for the clockevent and/or clocksource are registered in the early >>>>> CLK_OF_DECLARE path but the rest of the clks get registered with a >>>>> proper platform device and driver. There are examples of this approach >>>>> on other platforms already. >>>>> >>>> >>>> I looked at this a bit last week, but I didn't come up with any approach >>>> that I was happy with. It seems like it would be nice to just register >>>> the absolute minimum clocks needed. On DA8XX, that would just be the >>>> PLL0 >>>> AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one >>>> LPSC clock. The AUXCLKs are easy because they are just a simple gate >>>> from the oscillator. The LPSC clocks are a bit more tricky because they >>>> have a complex sequence for turning on. Furthermore, on DM646X, we need >>>> the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit >>>> messy there. >>> >>> Things might change in the context of work being done here by Bartosz >>> for converting clocks to early platform devices. >>> >>> But, keeping that development aside for a moment: I think this means the >>> PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform >>> devices are clocks that are not in the path to get timer clock working >>> (like CFGCHIP clocks). >> >> CLK_OF_DECLARE() only matters on DA850. None of the other SoCs use device > > Thats true today, but lets not make the assumption that none of the > other DaVinci SoCs will gain DT support ever. We don't want churn in CCF > support once someone tries to add DT support for other platforms. > > I already have people using DM365 in mainline, for example. > >> tree. And on DA850, the timer0 clock is just the PLL0 AUXCLK, so PSCs can> still be platform devices there. And in fact, one of the PSC clocks on >> DA850 depends on async3, which is a CFGCHIP clock, so if we made the PSCs >> CLK_OF_DECLARE(), then we also have to make at least the async3 CFGCHIP >> clock CLK_OF_DECLARE(). But, like I said, I think that can be avoided by >> leaving the PSC clocks as a platform device on DA850 (and DA830). > > Okay, I did not realize that there is a dependency between CFGCHIP clk > and PSC too. > >> For everything else, we need the legacy board file equivalent of >> CLK_OF_DECLARE(), which is basically what I had here in v5 of the >> series. > > Yeah. I did not realize the dependency with timer when platform devices > were suggested. Sorry. > >> >> So, if we want this to keep moving before the if/when of Bartosz's >> early platform device stuff, I'm thinking that I will make the PLL > > After looking at feedback to Bartosz's series, and the fact that > previous attempts at doing the same thing were rejected, I think there > is benefit in keeping CCF support moving independent Bartosz's work. > Otherwise, we have no chance of getting anything merged for v4.18. > It's true, that the feedback was not very positive, but I'm still thinking I can come up with something that would get accepted. Last time someone came up with the idea, the problem it was supposed to solve was different (dependencies between devices) and was eventually fixed with introduction of probe deferral. This time around we really want these devices probed early. The main concern was using a device tree compatible. If I can instead just early probe all devices whose drivers are registered as early without touching the device tree (as Frank Rowand suggested) then maybe the response of the community would be better. Thanks, Bart > Early initialization of clocks and dependency with timer is a pretty > common theme across SoCs. As and when a better way to support is found, > I am sure DaVinci can be converted over along with rest of the devices. > >> and PSC drivers loadable with or without a platform device and let >> each SoC pick which one it should use. > > This sounds flexible. We have to see how the patches look. But I would > caution against adding a lot of dead code. If there is no way some of > the clocks will be useful as platform device, I see no point of > supporting them as such just in anticipation. > > Thanks, > Sekhar
2018-04-25 12:09 GMT+02:00 Bartosz Golaszewski <brgl@bgdev.pl>: > 2018-04-25 8:07 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: >> On Tuesday 24 April 2018 09:41 PM, David Lechner wrote: >>> On 04/24/2018 03:28 AM, Sekhar Nori wrote: >>>> On Monday 23 April 2018 08:29 PM, David Lechner wrote: >>>>> On 04/06/2018 11:46 AM, Stephen Boyd wrote: >>>>>> Quoting Sekhar Nori (2018-04-06 02:37:03) >>>>>>> >>>>>>> Can you please check that and confirm there is no issue with genpd and >>>>>>> using CLK_OF_DECLARE() to initialize clocks? >>>>>>> >>>>>>> Unless you report an issue back, or Mike and Stephen have ideas about >>>>>>> how to handle the dependency between PSC/PLL derived timer clock >>>>>>> initialization and and timer_probe(), I think we need to move back to >>>>>>> using CLK_OF_DECLARE(). >>>>>> >>>>>> In such a case, please use the hybrid approach where the clks required >>>>>> for the clockevent and/or clocksource are registered in the early >>>>>> CLK_OF_DECLARE path but the rest of the clks get registered with a >>>>>> proper platform device and driver. There are examples of this approach >>>>>> on other platforms already. >>>>>> >>>>> >>>>> I looked at this a bit last week, but I didn't come up with any approach >>>>> that I was happy with. It seems like it would be nice to just register >>>>> the absolute minimum clocks needed. On DA8XX, that would just be the >>>>> PLL0 >>>>> AUXCLK. On most of the other SoCs, it would be the PLL AUXCLK plus one >>>>> LPSC clock. The AUXCLKs are easy because they are just a simple gate >>>>> from the oscillator. The LPSC clocks are a bit more tricky because they >>>>> have a complex sequence for turning on. Furthermore, on DM646X, we need >>>>> the whole PLL up to SYSCLK3 plus one LPSC clock, so things get a bit >>>>> messy there. >>>> >>>> Things might change in the context of work being done here by Bartosz >>>> for converting clocks to early platform devices. >>>> >>>> But, keeping that development aside for a moment: I think this means the >>>> PLLs and PSCs need to be CLK_OF_DECLARE(). What we can have as platform >>>> devices are clocks that are not in the path to get timer clock working >>>> (like CFGCHIP clocks). >>> >>> CLK_OF_DECLARE() only matters on DA850. None of the other SoCs use device >> >> Thats true today, but lets not make the assumption that none of the >> other DaVinci SoCs will gain DT support ever. We don't want churn in CCF >> support once someone tries to add DT support for other platforms. >> >> I already have people using DM365 in mainline, for example. >> >>> tree. And on DA850, the timer0 clock is just the PLL0 AUXCLK, so PSCs can> still be platform devices there. And in fact, one of the PSC clocks on >>> DA850 depends on async3, which is a CFGCHIP clock, so if we made the PSCs >>> CLK_OF_DECLARE(), then we also have to make at least the async3 CFGCHIP >>> clock CLK_OF_DECLARE(). But, like I said, I think that can be avoided by >>> leaving the PSC clocks as a platform device on DA850 (and DA830). >> >> Okay, I did not realize that there is a dependency between CFGCHIP clk >> and PSC too. >> >>> For everything else, we need the legacy board file equivalent of >>> CLK_OF_DECLARE(), which is basically what I had here in v5 of the >>> series. >> >> Yeah. I did not realize the dependency with timer when platform devices >> were suggested. Sorry. >> >>> >>> So, if we want this to keep moving before the if/when of Bartosz's >>> early platform device stuff, I'm thinking that I will make the PLL >> >> After looking at feedback to Bartosz's series, and the fact that >> previous attempts at doing the same thing were rejected, I think there >> is benefit in keeping CCF support moving independent Bartosz's work. >> Otherwise, we have no chance of getting anything merged for v4.18. >> > > It's true, that the feedback was not very positive, but I'm still > thinking I can come up with something that would get accepted. Last > time someone came up with the idea, the problem it was supposed to > solve was different (dependencies between devices) and was eventually > fixed with introduction of probe deferral. > > This time around we really want these devices probed early. The main > concern was using a device tree compatible. If I can instead just > early probe all devices whose drivers are registered as early without > touching the device tree (as Frank Rowand suggested) then maybe the * It was actually Mark Rutland. > response of the community would be better. > > Thanks, > Bart > >> Early initialization of clocks and dependency with timer is a pretty >> common theme across SoCs. As and when a better way to support is found, >> I am sure DaVinci can be converted over along with rest of the devices. >> >>> and PSC drivers loadable with or without a platform device and let >>> each SoC pick which one it should use. >> >> This sounds flexible. We have to see how the patches look. But I would >> caution against adding a lot of dead code. If there is no way some of >> the clocks will be useful as platform device, I see no point of >> supporting them as such just in anticipation. >> >> Thanks, >> Sekhar
diff --git a/drivers/clk/davinci/psc.c b/drivers/clk/davinci/psc.c index 9d9f94eee544..7e3d114efdac 100644 --- a/drivers/clk/davinci/psc.c +++ b/drivers/clk/davinci/psc.c @@ -281,12 +281,18 @@ int __davinci_psc_register_clocks(const struct davinci_psc_clk_info *info, } } - DO_ONCE(pm_clk_add_notifier, - &platform_bus_type, &platform_bus_notifier); - return 0; } +static int __init davinci_pm_runtime_init(void) +{ + pm_clk_add_notifier(&platform_bus_type, &platform_bus_notifier); + + return 0; +} +core_initcall(davinci_pm_runtime_init); + int davinci_psc_register_clocks(const struct davinci_psc_clk_info *info, void __iomem *base) {