Re: [RFC PATCH] Consolidate SRAM support

[Detlef Vollmann <dv@vollmann.ch>]

On 04/15/11 17:37, Russell King - ARM Linux wrote:
> On Fri, Apr 15, 2011 at 04:50:49PM +0200, Detlef Vollmann wrote:
>> I'd love to have the mapping inside the create pool, but that might
>> not be possible in general.
>
> No, think about it.  What if you need to map the RAM area with some
> special attributes - eg, where ioremap() doesn't work.  Eg, OMAP you
> need SRAM mapped as normal memory, except for OMAP34xx where it must
> be mapped normal memory non-cacheable.
>
> It's best to leave the mapping to the architecture.
Ok, I agree.

>>> Another question is whether we should allow multiple SRAM pools or not -
>>> this code does allow multiple pools, but so far we only have one pool
>>> per SoC.  Overdesign?  Maybe, but it prevents SoCs wanting to duplicate
>>> it if they want to partition the SRAM, or have peripheral-local SRAMs.
>> Having the option to partition the SRAM is probably useful.
>> What I'm missing is sram_pool_add: on AT91SAM9G20 you have two banks
>> of SRAM, and you might want to combine them into a single pool.
>
> Do you already combine them into a single pool, or is this a wishlist?
Well, I have it on my list for next week to write a driver that needs
that.  So your proposal came just in time :-)

> I'm really not interested in sorting out peoples wishlist items in
> the process of consolidation.
If your code doesn't support it, then I have three options:
  - implementing my own pool (this actually might make sense, as
    my requirement adds quite some overhead that others don't
    want to pay),
  - providing a patch on top of your implementation (as long as
    struct sram_pool is an opaque type, this doesn't change the API), or
  - forget dynamic allocation and assign buffers statically in the
    board setup.

Of course, if the mirroring mentioned by Nicolas I don't need any
of this...

> Second point is that you'll notice that the code converts to a phys
> address using this: phys = phys_base + (virt - virt_base).  As soon as
> we start allowing multiple regions of SRAM, it becomes impossible to
> provide the phys address without a lot more complexity.
Yes, I understand that.  This either requires some intrusive changes
to gen_pool code or quite some additional code in sram_pool_alloc.


And just one minor point: you might consider to rename sram_* to
pram_* (or similar), as there's nothing specific to SRAM in your
pool, the specific thing that sram_pool adds on top of gen_pool
is the physical address.  And a lot of systems have external SRAM
that will normally not be used with your sram_pool.

   Detlef