Re: [PATCH v8 2/4] misc: Generic on-chip SRAM allocation driver

[Philipp Zabel <p.zabel@pengutronix.de>]

Am Dienstag, den 05.02.2013, 00:53 +0900 schrieb Paul Mundt:
> On Mon, Feb 04, 2013 at 12:32:16PM +0100, Philipp Zabel wrote:
> > This driver requests and remaps a memory region as configured in the
> > device tree. It serves memory from this region via the genalloc API.
> > It optionally enables the SRAM clock.
> > 
> > Other drivers can retrieve the genalloc pool from a phandle pointing
> > to this drivers' device node in the device tree.
> > 
> > The allocation granularity is hard-coded to 32 bytes for now,
> > to make the SRAM driver useful for the 6502 remoteproc driver.
> > There is overhead for bigger SRAMs, where only a much coarser
> > allocation granularity is needed: At 32 bytes minimum allocation
> > size, a 256 KiB SRAM needs a 1 KiB bitmap to track allocations.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
> 
> How exactly is this "generic" if you have randomly hard-coded an
> allocation granularity that is larger than half of the in-tree SRAM pool
> users today can even support? Did you even bother to look at in-tree SRAM
> pool users other than the one you are working on?

I agree it should be configurable. As stated in the thread pointed out
by Matt, I'd just like to separate the discussion about the device tree
bindings for the allocation granularity from this patchset.

About the "generic" labeling, I had a look around for users of
gen_pool_create() - I admit to overlooking arch/sh/mm/sram.c, and most
of the others (except arch/arm/kernel/tcm.c drivers/acpi/apei/ghes.c,
both of which I didn't consider potential users of the sram driver) seem
to use a granularity significantly larger than 32 bytes.

> There also doesn't seem to be any real reason for the hard-coding either,
> this information could easily be fetched via platform data or the device
> tree,

True. Although the device tree is supposed to describe the hardware. The
allocation granularity is an implementation detail of the gen_pool
allocator (or possibly kind of a hardware detail of the gen_pool user,
if there are alignment requirements). That is why I didn't want to press
on with the device tree granularity configuration part yet.

> and the driver in question would simply need to be able to
> determine whether the size is suitable for it or not.

Sounds sensible to me. I'd like to follow up with Matt's patch and
something like this in a second step.

regards
Philipp