[RFC 1/1] ARM: imx6q: move clock register map to machine_desc.map_io

[Eric Miao]

On Thu, Nov 17, 2011 at 5:28 PM, Shawn Guo <shawn.guo at freescale.com> wrote:
> On Thu, Nov 17, 2011 at 05:03:15PM +0800, Eric Miao wrote:
>> On Thu, Nov 17, 2011 at 5:07 PM, Shawn Guo <shawn.guo at freescale.com> wrote:
>> > On Thu, Nov 17, 2011 at 08:44:53AM +0000, Russell King - ARM Linux wrote:
>> >> On Thu, Nov 17, 2011 at 11:20:13AM +0800, Shawn Guo wrote:
>> >> > On Thu, Nov 17, 2011 at 10:56:53AM +0800, Eric Miao wrote:
>> >> > > On Thu, Nov 17, 2011 at 9:50 AM, Richard Zhao <richard.zhao at linaro.org> wrote:
>> >> > > > If the map is in machine_desc.timer.init, the registers will not be
>> >> > > > able to access after bootup (probably after "Freeing init memory").
>> >> > >
>> >> > > Given that devicemaps_init() does additional work after creating
>> >> > > the mapping, flushing TLB and cache, machine_desc->map_io() is
>> >> > > what I assume the best place for these static IO mappings than
>> >> > > elsewhere.
>> >> > >
>> >> > > Shawn?
>> >> > >
>> >> > I'm still trying to understand what happened.  I do not understand
>> >> > how the map can become unusable after "Freeing init memory".  Also
>> >> > I can not reproduce the problem on my side.
>> >>
>> >> It's very simple.  iotable_init() allocates memory using memblock.
>> >> memblock marks the memory it allocates in its reserved map.  At the
>> >> end of map_io(), the information stored inside memblock is transferred
>> >> to bootmem and then the normal page allocator.  Once this is done,
>> >> memblock must not be changed because there is no way to couple any
>> >> further memblock allocations into the normal page allocator.
>> >>
>> >> Look.  I've made this point before and I'm getting pissed off with
>> >> people who think they know better.  The callbacks in the machine
>> >> descriptor are NAMED.  They're NAMED for a reason - they have a PURPOSE
>> >> and that PURPOSE is well defined because there are DEPENDENCIES there.
>> >> They tell you what you SHOULD be doing in there.  Doing something else
>> >> will result in BAD THINGS happening.
>> >>
>> >> If people can't understand this, then the only solution is to make the
>> >> backing functions for this stuff (like iotable_init() etc) BUG() when
>> >> they're abused.  That means adding a lot more checks and slowing down
>> >> the boot because people seem to be incapable of comprehending that
>> >> initialisation needs to be done in a well defined order.
>> >>
>> > Russell,
>> >
>> > Thanks for help understand the issue.
>> >
>> > Richard,
>> >
>> > Please revise the patch to introduce function like imx6q_clock_map_io()
>> > to be called in imx6q_map_io().
>>
>> Or simply imx6q_io_desc[], and doing a iotable_init() for all (and
>> considering ANATOP not necessarily include clocks registers only)
>> other possible static IO mappings in the future.
>>
> I do not see any more possible static IO mapping in the future. On the
> contrary, the static mapping for clocks could possibly be removed when
> we migrate to common clock framework and create clocks dynamically from
> device tree.

Fair enough and it would be ideal if static IO mapping is not necessary.

And I don't have a strong preference of having a single array for all these
mappings or having separate function for each of them. It just looks a bit
redundant to have a one-line function here.