Re: [PATCH] clk: samsung: exynos7: Enable clocks for CMU_CCORE and CMU_FSYS0 blocks

[Alim Akhtar]

Hi

On 04/13/2016 05:01 PM, Tomasz Figa wrote:
> 2016-04-13 13:36 GMT+03:00 Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>:
> > On 04/13/2016 08:26 AM, Tomasz Figa wrote:
> > > > > @@ -205,7 +206,11 @@ static struct samsung_cmu_info topc_cmu_info __initdata = {
> > > > >
> > > > >   static void __init exynos7_clk_topc_init(struct device_node *np)
> > > > >   {
> > > > > +       struct clk *clk;
> > > > > +
> > > > >          samsung_cmu_register_one(np, &topc_cmu_info);
> > > > > +       clk = __clk_lookup("aclk_ccore_133");
> > > > > +       clk_prepare_enable(clk);
> > >
> > > Shouldn't this be rather done before calling
> > > samsung_cmu_register_one()? I don't remember exactly, but wouldn't
> > > clock registration trigger reading back current (mux, div) values from
> > > registers?
> > >
> > > Also, do we have any guarantees on order of initialization of
> > > particular CMUs? I believe this will happen in order of DT nodes and
> > > so would be not any kind of guarantee at all.
> >
> > If these clocks need to be kept enabled perhaps it's better to just set
> > CLK_IS_CRITICAL flag for them? Patches adding this flag are already in
> > clk-next branch in the clk git tree. This way the clocks would get
> > enabled within the clk_register() call.
>
> Oh, I didn't know about this. Sounds like a good idea. :)

I was not aware of this flag, Thanks for pointing this out.
This flag indeed solves my problem.
These clocks should not be gated since the CMU block using this clock 
for SFR access.
Please ignore the $SUBJECT patch, I will send a new patch which uses
CLK_IS_CRITICAL flag instead.

> > The CMU registration order is enforced by listing input clocks to each
> > CMU in DT.
>
> Ah, right, I recall now, I even reviewed relevant patch. Thanks for
> refreshing my memory.
>
> > However, I wouldn't be concerned much about it in context
> > of this patch. We are enabling here clocks which belong to same CMU.
> > samsung_cmu_register_one() needs to be called first for subsequent
> > __clk_lookup() calls to work.
>
> I thought a clock from another CMU has to be enabled for the CPU to be
> able to access the CMU being registered.
>
> > Perhaps related bits need to be set manually in CMU registers before
> > registering a clock provider for the CMU, to fulfil the requirements.
> >
> > Anyway, summary of the $subject patch seems not precise enough:
> >
> > "This patch enables clocks for CMU_CCORE and CMU_FSYS0 blocks. This is
> > required before accessing registers of these blocks."
> >
> > We need to enable selected clocks (i.e. access the CMU's registers)
> > before accessing this CMU's registers?
>
> Yeah, maybe the explanation could be a bit more precise.
>
> Best regards,
> Tomasz