Re: [PATCH 1/4] clk: rockchip: protect critical clocks from getting disabled

["Heiko Stübner" <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>]

Hi Kever,

Am Montag, 11. August 2014, 18:03:33 schrieb Kever Yang:
> On 08/09/2014 06:20 AM, Heiko Stübner wrote:
> > Am Freitag, 8. August 2014, 14:58:11 schrieb Doug Anderson:
> >> Heiko,
> >> 
> >> On Fri, Aug 1, 2014 at 1:15 AM, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> >>> Am Donnerstag, 31. Juli 2014, 17:30:25 schrieb Mike Turquette:
> >>>> Quoting Heiko Stübner (2014-07-31 16:29:34)
> >>>> 
> >>>>> Hi Mike,
> >>>>> 
> >>>>> Am Donnerstag, 31. Juli 2014, 15:45:23 schrieb Mike Turquette:
> >>>>>> Quoting Heiko Stuebner (2014-07-29 12:12:05)
> >>>>>> 
> >>>>>>> The clock-tree contains clocks that should never get disabled
> >>>>>>> automatically. One example are the base ACLKs, the base supplies
> >>>>>>> for
> >>>>>>> all
> >>>>>>> peripherals.
> >>>>>>> 
> >>>>>>> Therefore add a structure similar to the sunxi clock-tree to
> >>>>>>> protect
> >>>>>>> these
> >>>>>>> special clocks from being disabled.
> >>>>>>> 
> >>>>>>> Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>   drivers/clk/rockchip/clk-rk3188.c |  7 +++++++
> >>>>>>>   drivers/clk/rockchip/clk-rk3288.c |  7 +++++++
> >>>>>>>   drivers/clk/rockchip/clk.c        | 13 +++++++++++++
> >>>>>>>   drivers/clk/rockchip/clk.h        |  1 +
> >>>>>>>   4 files changed, 28 insertions(+)
> >>>>>>> 
> >>>>>>> diff --git a/drivers/clk/rockchip/clk-rk3188.c
> >>>>>>> b/drivers/clk/rockchip/clk-rk3188.c index a83a6d8..5aef277 100644
> >>>>>>> --- a/drivers/clk/rockchip/clk-rk3188.c
> >>>>>>> +++ b/drivers/clk/rockchip/clk-rk3188.c
> >>>>>>> @@ -599,6 +599,11 @@ static struct rockchip_clk_branch
> >>>>>>> rk3188_clk_branches[] __initdata = {>
> >>>>>>> 
> >>>>>>>          GATE(ACLK_GPS, "aclk_gps", "aclk_peri", 0,
> >>>>>>>          RK2928_CLKGATE_CON(8),
> >>>>>>>          13, GFLAGS),>
> >>>>>>>   
> >>>>>>>   };
> >>>>>>> 
> >>>>>>> +static const char *rk3188_critical_clocks[] __initconst = {
> >>>>>>> +       "aclk_cpu",
> >>>>>>> +       "aclk_peri",
> >>>>>> 
> >>>>>> I'm not against the idea of critical clocks, but I want to verify
> >>>>>> that
> >>>>>> there is no other driver out there that is a better fit for claiming
> >>>>>> these clks via clk_get and enabling them the normal way via
> >>>>>> clk_enable?
> >>>>> 
> >>>>> In the clock hierarchy of Rockchip SoCs, both aclks listed here, are
> >>>>> sources for pclk and hclk, as well as sourcing some other peripheral
> >>>>> gates further below too. So from what I've seen from the clock
> >>>>> diagrams,
> >>>>> there is nothing that would claim these clocks directly, and it
> >>>>> wouldn't
> >>>>> also make any sense to let them get disabled as there will always be
> >>>>> something using them (for example the dram-controller).
> >>>> 
> >>>> Sounds good. Just out of curiosity, under what circumstances would you
> >>>> want to gate them? Is there a use case for it?
> >>> 
> >>> hmm, I don't see a use-case for gating these at runtime right now,
> >>> simply
> >>> because there should be a user for them all the time. (both aclks
> >>> combined
> >>> have at least 68 consumers on the rk3288 and a similar number on the
> >>> previous socs)
> >>> 
> >>> The only thing I could think of would be something suspend related -
> >>> which
> >>> we don't have yet. But then this would probably happen in the clock
> >>> controller itself anyway in some late suspend-related action, so it
> >>> could
> >>> take into account them being defined as critical clocks.
> >> 
> >> I know Rockchip has some funky stuff planned for memory scaling too.
> >> Perhaps Kever can comment whether these two clocks might need to be
> >> disabled in that case?
> > 
> > hmm looking at the core clock tree, I wouldn't think so.
> > 
> > The only intersection between the ddr-clk, aclk_cpu and aclk_peri is the
> > gpll which can be a source to both. But the ddr-clk is mainly sourced
> > from the dpll anyway.
> > 
> > In any case, turning off aclk_cpu/aclk_peri in this scenario wouldn't
> > normally be possible anyway, as most of the time some pclk_* would be
> > active anyway.
> Basically, aclk_cpu/aclk_peri have very little chance to be gated during
> run-time,
> but both of then may be gated when system enter suspend mode.
> 
> For aclk_cpu, this clock supplies most of clocks in pd_bus actually,
> some clocks not listed
> as a module clock will be needed, like cpu I/D bus fetch
> instruction/data from dram via
> bus based on aclk_cpu. For this situation, can we use a dummy clock to
> hold the
> aclk_cpu not to be gated at run-time?
> 
> For aclk_peri, this clock is able to be gated run-time in theory,
> although it's no use
> in actual system, because we have many devices on this clock and at most
> of the time
> some of then would be active just as you have mentioned.
> 
> The system suspend is another scenario, and we tend to gate both of the
> clock if possible,
> can we do that if this patch is applied?

as we will need suspend operations in the clock driver anyway [likely 
something like the Samsung clk driver does], it shouldn't be a problem to lift 
the hold on the critical clocks when suspending.


Heiko

> 
> -Kever
> 
> >> In any case, this patch fixes a hang at boot when using the PWM driver
> >> that just landed, so:
> >> 
> >> Tested-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > 
> > thanks
> > 
> > 
> > Heiko

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html