Clock register in early init

Clock register in early init

[Prashant Gaikwad]

Thanks for the reply Mike!!

On Thu, 2012-05-17 at 11:51 +0530, Mike Turquette wrote:
> On 20120517-09:41, Prashant Gaikwad wrote:
> > Hi Mike,
> > 
> > While porting Tegra to common clock framework I observed that if we try
> > to allocate struct clk dynamically in early init, kernel panics since
> > memory is not initialized yet.
> > 
> > In your clk-next for omap you are registering clocks in early init and
> > clk_register allocates struct clk dynamically.
> > 
> > So, am I missing something here?
> > 
> 
> You have the same problem as my platform (OMAP).  Please check out
> include/linux/clk-private.h.  That is a really nasty terrible hack that
> exists for statically initializing all data.  It exposes the definition
> of struct clk (which is bad and violates the design philosophy that
> struct clk should be an opaque cookie).
> 
> Please review Documentat/clk.txt to understand the rules around
> clk-private.h.  Namely that you MUST separate your data from your clock
> function definitions (the clk_ops should never know the definition of
> struct clk).
> 

Yes, currently I am also doing same for Tegra and it works well. clk_ops
and clk initialization data are in separate files. clk_ops does not
include clk-private.h so it does not know the definition of struct clk.

Will send patches for shortly.

> Finally, I really want to kill off clk-private.h.  Currently OMAP does
> clock registration during early init and that is why this crap exists.
> However we are going to move our clock initialization to be initcall
> based at some point in the future.  No other platform (until now) was
> using these macros in clk-private.h, so I had planned on removing them
> after OMAP migrated.
> 
> What are the plans for Tegra?  I really don't want to have to support
> this stuff.  Can you migrate your clock initialization to be initcall
> based at some point in the future?
> 

We had initcall based implementation in old kernel versions. It was
moved to early init since some clocks were required for delay
calibrations. It may be possible to migrate it be initcall based in
future.

> Regards,
> Mike
> 
> p.s: best to have these kinds of discussions on the list, out in the
> open.  If you want to continue our discussion can you loop in LAKML?
> 

Added LAKML in loop.

> > Same is observed if we try to allocate parent list dynamically.
> > 
> > Thanks & regards,
> > Prashant G

Prashant G

Clock register in early init

[Peter De Schrijver]

On Fri, May 18, 2012 at 06:48:37AM +0200, Prashant Gaikwad wrote:
> Thanks for the reply Mike!!
> 
> On Thu, 2012-05-17 at 11:51 +0530, Mike Turquette wrote:
> > On 20120517-09:41, Prashant Gaikwad wrote:
> > > Hi Mike,
> > > 
> > > While porting Tegra to common clock framework I observed that if we try
> > > to allocate struct clk dynamically in early init, kernel panics since
> > > memory is not initialized yet.
> > > 
> > > In your clk-next for omap you are registering clocks in early init and
> > > clk_register allocates struct clk dynamically.
> > > 
> > > So, am I missing something here?
> > > 
> > 
> > You have the same problem as my platform (OMAP).  Please check out
> > include/linux/clk-private.h.  That is a really nasty terrible hack that
> > exists for statically initializing all data.  It exposes the definition
> > of struct clk (which is bad and violates the design philosophy that
> > struct clk should be an opaque cookie).
> > 
> > Please review Documentat/clk.txt to understand the rules around
> > clk-private.h.  Namely that you MUST separate your data from your clock
> > function definitions (the clk_ops should never know the definition of
> > struct clk).
> > 
> 
> Yes, currently I am also doing same for Tegra and it works well. clk_ops
> and clk initialization data are in separate files. clk_ops does not
> include clk-private.h so it does not know the definition of struct clk.
> 
> Will send patches for shortly.
> 
> > Finally, I really want to kill off clk-private.h.  Currently OMAP does
> > clock registration during early init and that is why this crap exists.
> > However we are going to move our clock initialization to be initcall
> > based at some point in the future.  No other platform (until now) was
> > using these macros in clk-private.h, so I had planned on removing them
> > after OMAP migrated.
> > 
> > What are the plans for Tegra?  I really don't want to have to support
> > this stuff.  Can you migrate your clock initialization to be initcall
> > based at some point in the future?
> > 
> 
> We had initcall based implementation in old kernel versions. It was
> moved to early init since some clocks were required for delay
> calibrations. It may be possible to migrate it be initcall based in
> future.

I don't think we can do it all dynamic. We need to have the clockfrequency
of the local timer at least, which depends on the PLLX frequency and
ultimately on HFOSC input frequency which is measured by the clock code at
boottime. I haven't figured out what else we need, but I wouldn't be surprised
to find some more gotchas. It might be possible to split the clocks into
'early init clocks' and normal clocks, but I would rather not go there.
Which advantages do you see in dynamically allocating all this?

Cheers,

Peter.

Clock register in early init

[Turquette, Mike]

On Fri, May 18, 2012 at 4:21 AM, Peter De Schrijver
<pdeschrijver@nvidia.com> wrote:
> On Fri, May 18, 2012 at 06:48:37AM +0200, Prashant Gaikwad wrote:
>> Thanks for the reply Mike!!
>>
>> On Thu, 2012-05-17 at 11:51 +0530, Mike Turquette wrote:
>> > On 20120517-09:41, Prashant Gaikwad wrote:
>> > > Hi Mike,
>> > >
>> > > While porting Tegra to common clock framework I observed that if we try
>> > > to allocate struct clk dynamically in early init, kernel panics since
>> > > memory is not initialized yet.
>> > >
>> > > In your clk-next for omap you are registering clocks in early init and
>> > > clk_register allocates struct clk dynamically.
>> > >
>> > > So, am I missing something here?
>> > >
>> >
>> > You have the same problem as my platform (OMAP). ?Please check out
>> > include/linux/clk-private.h. ?That is a really nasty terrible hack that
>> > exists for statically initializing all data. ?It exposes the definition
>> > of struct clk (which is bad and violates the design philosophy that
>> > struct clk should be an opaque cookie).
>> >
>> > Please review Documentat/clk.txt to understand the rules around
>> > clk-private.h. ?Namely that you MUST separate your data from your clock
>> > function definitions (the clk_ops should never know the definition of
>> > struct clk).
>> >
>>
>> Yes, currently I am also doing same for Tegra and it works well. clk_ops
>> and clk initialization data are in separate files. clk_ops does not
>> include clk-private.h so it does not know the definition of struct clk.
>>
>> Will send patches for shortly.
>>
>> > Finally, I really want to kill off clk-private.h. ?Currently OMAP does
>> > clock registration during early init and that is why this crap exists.
>> > However we are going to move our clock initialization to be initcall
>> > based at some point in the future. ?No other platform (until now) was
>> > using these macros in clk-private.h, so I had planned on removing them
>> > after OMAP migrated.
>> >
>> > What are the plans for Tegra? ?I really don't want to have to support
>> > this stuff. ?Can you migrate your clock initialization to be initcall
>> > based at some point in the future?
>> >
>>
>> We had initcall based implementation in old kernel versions. It was
>> moved to early init since some clocks were required for delay
>> calibrations. It may be possible to migrate it be initcall based in
>> future.
>
> I don't think we can do it all dynamic. We need to have the clockfrequency
> of the local timer at least, which depends on the PLLX frequency and
> ultimately on HFOSC input frequency which is measured by the clock code at
> boottime. I haven't figured out what else we need, but I wouldn't be surprised
> to find some more gotchas. It might be possible to split the clocks into
> 'early init clocks' and normal clocks, but I would rather not go there.

On OMAP I think the only "gotcha" is setting up the timer.  One
solution is to open code the register reads and the rate calculation
in the timer code.  That is ugly... but it works.

> Which advantages do you see in dynamically allocating all this?
>

There are many but I'll name a couple.  The most significant point is
that we can avoid exposing the definition of struct clk if we
dynamically allocate stuff.  One can use struct clk_hw_init to
statically initialize data, or instead rely on direct calls to
clk_register with a bunch of parameters.

Another point is that copying the data at registration-time makes
__initdata possible.  I haven't done the math yet to see if this
really makes a difference.  However if we start doing single zImage's
with multiple different ARM SoCs then this could recover some pages.

Regards,
Mike

> Cheers,
>
> Peter.

Clock register in early init

[Peter De Schrijver]

> 
> On OMAP I think the only "gotcha" is setting up the timer.  One
> solution is to open code the register reads and the rate calculation
> in the timer code.  That is ugly... but it works.
> 
> > Which advantages do you see in dynamically allocating all this?
> >
> 
> There are many but I'll name a couple.  The most significant point is
> that we can avoid exposing the definition of struct clk if we
> dynamically allocate stuff.  One can use struct clk_hw_init to
> statically initialize data, or instead rely on direct calls to
> clk_register with a bunch of parameters.
> 

Which means if you make a mistake in specifying parents for example, it will
only fail at runtime, possibly before any console is active. With static
initialization, this will fail at compiletime. Much easier to debug.

> Another point is that copying the data at registration-time makes
> __initdata possible.  I haven't done the math yet to see if this
> really makes a difference.  However if we start doing single zImage's
> with multiple different ARM SoCs then this could recover some pages.
> 

On the other hand most clock structures are small, so there will be internal
fragmentation. Also the arrays of parent clock pointers can be shared between
different clocks. We have about 70 muxes in Tegra30 and 12 different parent
arrays.

Cheers,

Peter.

Clock register in early init

[Turquette, Mike]

On Mon, May 21, 2012 at 1:46 AM, Peter De Schrijver
<pdeschrijver@nvidia.com> wrote:
>>
>> On OMAP I think the only "gotcha" is setting up the timer. ?One
>> solution is to open code the register reads and the rate calculation
>> in the timer code. ?That is ugly... but it works.
>>
>> > Which advantages do you see in dynamically allocating all this?
>> >
>>
>> There are many but I'll name a couple. ?The most significant point is
>> that we can avoid exposing the definition of struct clk if we
>> dynamically allocate stuff. ?One can use struct clk_hw_init to
>> statically initialize data, or instead rely on direct calls to
>> clk_register with a bunch of parameters.
>>
>
> Which means if you make a mistake in specifying parents for example, it will
> only fail at runtime, possibly before any console is active. With static
> initialization, this will fail at compiletime. Much easier to debug.
>

Is this really a problem?  Once you have good data it does not change.
 Debugging bad data when introducing a new chip is just a fact of
life.  Static versus dynamic is irrelevant here.

>> Another point is that copying the data at registration-time makes
>> __initdata possible. ?I haven't done the math yet to see if this
>> really makes a difference. ?However if we start doing single zImage's
>> with multiple different ARM SoCs then this could recover some pages.
>>
>
> On the other hand most clock structures are small, so there will be internal
> fragmentation. Also the arrays of parent clock pointers can be shared between
> different clocks. We have about 70 muxes in Tegra30 and 12 different parent
> arrays.
>

What is missing from struct clk_hw_init to do what your static arrays do today?

Thanks,
Mike

> Cheers,
>
> Peter.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Clock register in early init

[Peter De Schrijver]

On Mon, May 21, 2012 at 08:05:57PM +0200, Turquette, Mike wrote:
> On Mon, May 21, 2012 at 1:46 AM, Peter De Schrijver
> <pdeschrijver@nvidia.com> wrote:
> >>
> >> On OMAP I think the only "gotcha" is setting up the timer. ?One
> >> solution is to open code the register reads and the rate calculation
> >> in the timer code. ?That is ugly... but it works.
> >>
> >> > Which advantages do you see in dynamically allocating all this?
> >> >
> >>
> >> There are many but I'll name a couple. ?The most significant point is
> >> that we can avoid exposing the definition of struct clk if we
> >> dynamically allocate stuff. ?One can use struct clk_hw_init to
> >> statically initialize data, or instead rely on direct calls to
> >> clk_register with a bunch of parameters.
> >>
> >
> > Which means if you make a mistake in specifying parents for example, it will
> > only fail at runtime, possibly before any console is active. With static
> > initialization, this will fail at compiletime. Much easier to debug.
> >
> 
> Is this really a problem?  Once you have good data it does not change.
>  Debugging bad data when introducing a new chip is just a fact of
> life.  Static versus dynamic is irrelevant here.
> 

I think it is a problem yes, especially when the tree becomes more
complicated.

Cheers,

Peter.

Clock register in early init

[Ben Dooks]

On 21/05/12 09:46, Peter De Schrijver wrote:
>>
>> On OMAP I think the only "gotcha" is setting up the timer.  One
>> solution is to open code the register reads and the rate calculation
>> in the timer code.  That is ugly... but it works.
>>
>>> Which advantages do you see in dynamically allocating all this?
>>>
>>
>> There are many but I'll name a couple.  The most significant point is
>> that we can avoid exposing the definition of struct clk if we
>> dynamically allocate stuff.  One can use struct clk_hw_init to
>> statically initialize data, or instead rely on direct calls to
>> clk_register with a bunch of parameters.
>>
>
> Which means if you make a mistake in specifying parents for example, it will
> only fail at runtime, possibly before any console is active. With static
> initialization, this will fail at compiletime. Much easier to debug.
>
>> Another point is that copying the data at registration-time makes
>> __initdata possible.  I haven't done the math yet to see if this
>> really makes a difference.  However if we start doing single zImage's
>> with multiple different ARM SoCs then this could recover some pages.
>>
>
> On the other hand most clock structures are small, so there will be internal
> fragmentation. Also the arrays of parent clock pointers can be shared between
> different clocks. We have about 70 muxes in Tegra30 and 12 different parent
> arrays.

We had at-least that on the older Samsung parts and they where still
growing. I would suggest that in a multi-kernel image situation the
more data that can be discarded after init-time the better.

Also, __initdata gets gathered into one place so there's no possibility
of page fragmentation there. If you mean fragmentation of the memory
map, then allocate the size of all the clocks you know of at init time
in one go.

It would possible even be useful to have construction code that took
some brief details of the clocks and constructed the list/tree of clocks
at start time.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Clock register in early init

[Peter De Schrijver]

> We had at-least that on the older Samsung parts and they where still
> growing. I would suggest that in a multi-kernel image situation the
> more data that can be discarded after init-time the better.
> 
> Also, __initdata gets gathered into one place so there's no possibility
> of page fragmentation there. If you mean fragmentation of the memory
> map, then allocate the size of all the clocks you know of at init time
> in one go.
> 

That would work, except that clocks are needed before kmalloc is available.

Cheers,

Peter.

Clock register in early init

[Turquette, Mike]

On Tue, May 22, 2012 at 5:08 AM, Peter De Schrijver
<pdeschrijver@nvidia.com> wrote:
>> We had at-least that on the older Samsung parts and they where still
>> growing. I would suggest that in a multi-kernel image situation the
>> more data that can be discarded after init-time the better.
>>
>> Also, __initdata gets gathered into one place so there's no possibility
>> of page fragmentation there. If you mean fragmentation of the memory
>> map, then allocate the size of all the clocks you know of at init time
>> in one go.
>>
>
> That would work, except that clocks are needed before kmalloc is available.
>

Is static initialization the only way to solve this problem?  What
about using the bootmem allocator for early init clocks?

Regards,
Mike

> Cheers,
>
> Peter.

Clock register in early init

[Peter De Schrijver]

On Tue, May 22, 2012 at 07:37:15PM +0200, Turquette, Mike wrote:
> On Tue, May 22, 2012 at 5:08 AM, Peter De Schrijver
> <pdeschrijver@nvidia.com> wrote:
> >> We had at-least that on the older Samsung parts and they where still
> >> growing. I would suggest that in a multi-kernel image situation the
> >> more data that can be discarded after init-time the better.
> >>
> >> Also, __initdata gets gathered into one place so there's no possibility
> >> of page fragmentation there. If you mean fragmentation of the memory
> >> map, then allocate the size of all the clocks you know of at init time
> >> in one go.
> >>
> >
> > That would work, except that clocks are needed before kmalloc is available.
> >
> 
> Is static initialization the only way to solve this problem?  What
> about using the bootmem allocator for early init clocks?
> 

If it can be handled inside the framework somehow. I would want to avoid
having several places where clocks inside the same SoC are initialized.

Cheers,

Peter.

Clock register in early init

[Turquette, Mike]

On Wed, May 23, 2012 at 2:19 AM, Peter De Schrijver
<pdeschrijver@nvidia.com> wrote:
> On Tue, May 22, 2012 at 07:37:15PM +0200, Turquette, Mike wrote:
>> On Tue, May 22, 2012 at 5:08 AM, Peter De Schrijver
>> <pdeschrijver@nvidia.com> wrote:
>> >> We had at-least that on the older Samsung parts and they where still
>> >> growing. I would suggest that in a multi-kernel image situation the
>> >> more data that can be discarded after init-time the better.
>> >>
>> >> Also, __initdata gets gathered into one place so there's no possibility
>> >> of page fragmentation there. If you mean fragmentation of the memory
>> >> map, then allocate the size of all the clocks you know of at init time
>> >> in one go.
>> >>
>> >
>> > That would work, except that clocks are needed before kmalloc is available.
>> >
>>
>> Is static initialization the only way to solve this problem? ?What
>> about using the bootmem allocator for early init clocks?
>>
>
> If it can be handled inside the framework somehow. I would want to avoid
> having several places where clocks inside the same SoC are initialized.
>

I'll look into the bootmem thing.  Paul mentioned that a long time
ago.  Hopefully it can be done in such a way as to expose the same
interfaces as the dynamic allocation routines.  I'll keep you Cc'd.

Regards,
Mike

> Cheers,
>
> Peter.

Clock register in early init

[Paul Mundt]

On Tue, May 22, 2012 at 10:37:15AM -0700, Turquette, Mike wrote:
> On Tue, May 22, 2012 at 5:08 AM, Peter De Schrijver
> <pdeschrijver@nvidia.com> wrote:
> >> We had at-least that on the older Samsung parts and they where still
> >> growing. I would suggest that in a multi-kernel image situation the
> >> more data that can be discarded after init-time the better.
> >>
> >> Also, __initdata gets gathered into one place so there's no possibility
> >> of page fragmentation there. If you mean fragmentation of the memory
> >> map, then allocate the size of all the clocks you know of at init time
> >> in one go.
> >>
> >
> > That would work, except that clocks are needed before kmalloc is available.
> >
> 
> Is static initialization the only way to solve this problem?  What
> about using the bootmem allocator for early init clocks?
> 
This is what sh does for its asm/clkdev.h __clkdev_alloc(), which is
really the only difference over the ARM version. It always seemed a bit
silly to me to make early allocation an arch-specific property.

Clock register in early init

[Turquette, Mike]

On Wed, May 23, 2012 at 6:02 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Tue, May 22, 2012 at 10:37:15AM -0700, Turquette, Mike wrote:
>> On Tue, May 22, 2012 at 5:08 AM, Peter De Schrijver
>> <pdeschrijver@nvidia.com> wrote:
>> >> We had at-least that on the older Samsung parts and they where still
>> >> growing. I would suggest that in a multi-kernel image situation the
>> >> more data that can be discarded after init-time the better.
>> >>
>> >> Also, __initdata gets gathered into one place so there's no possibility
>> >> of page fragmentation there. If you mean fragmentation of the memory
>> >> map, then allocate the size of all the clocks you know of at init time
>> >> in one go.
>> >>
>> >
>> > That would work, except that clocks are needed before kmalloc is available.
>> >
>>
>> Is static initialization the only way to solve this problem? ?What
>> about using the bootmem allocator for early init clocks?
>>
> This is what sh does for its asm/clkdev.h __clkdev_alloc(), which is
> really the only difference over the ARM version. It always seemed a bit
> silly to me to make early allocation an arch-specific property.

Good point.  clkdev will probably need to be bootmem-able as well as
the common clk stuff.

Regards,
Mike