Consolidate SRAM support

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

lpc32xx and pnx4008 also use iram, but do not have an allocator (only 1 user). Both are doing a copy the suspend code to IRAM and run it which may also be a good thing to have generic code for. Several i.MX chips also need to run from IRAM for suspend.

Quoted 4 lines · Russell King - ARM Linux

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

It's more than that. Several i.MX chips use plat-mxc/iram_alloc.c.

Quoted 6 lines · On Friday 15 April 2011 15:39:55 Rob Herring wrote:

Quoted 4 lines · Russell King - ARM Linux

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Quoted 1 line

It's more than that. Several i.MX chips use plat-mxc/iram_alloc.c.

There are also non-ARM systems doing something like that, arch/blackfin/mm/sram-alloc.c comes to mind. On powerpc, the ppc7410 also has this feature in hardware, but I believe it was never supported in mainline Linux.

How about putting sram-pool.c into the top-level mm/ directory and sram-pool.h into include/linux? They seem to be completely generic to me, aside from the dependency on asm/fncpy.h which should probably remain arch specific.

Quoted 8 lines · Arnd Bergmann

Quoted 6 lines · On Friday 15 April 2011 15:39:55 Rob Herring wrote:

Quoted 4 lines · Russell King - ARM Linux

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Quoted 6 lines

Quoted 1 line

It's more than that. Several i.MX chips use plat-mxc/iram_alloc.c.

There are also non-ARM systems doing something like that, arch/blackfin/mm/sram-alloc.c comes to mind. On powerpc, the ppc7410 also has this feature in hardware, but I believe it was never supported in mainline Linux.

Yes, but there's some horrible bits out there. ?PPC is one of them which looks like it doesn't lend itself to consolidating with this kind of implementation.

Quoted 4 lines

How about putting sram-pool.c into the top-level mm/ directory and sram-pool.h into include/linux? They seem to be completely generic to me, aside from the dependency on asm/fncpy.h which should probably remain arch specific.

I've thought about lib - but first lets see whether other architectures want to use it first. ?I've asked the sh folk off-list about this and waiting for a reply.

Blackfin and PPC look horrible to sort out though, so they can come at a later date if they so wish.

Yes, once the infrastructure is in place, powerpc can do its own migration to the new code. I vote for putting it in lib at the outset.

Quoted 3 lines · Grant Likely

Yes, once the infrastructure is in place, powerpc can do its own migration to the new code. ?I vote for putting it in lib at the outset.

I don't agree with stuffing non-arch directories with code which people haven't already agreed should be shared. ?As I've already said, in my experience it's hard to get agreement in the first place and even when you can the API generally needs to be changed from what you first think would be reasonable.

So, lets wait to see whether the SH folk reply.

Quoted 3 lines · Grant Likely

Yes, once the infrastructure is in place, powerpc can do its own migration to the new code. I vote for putting it in lib at the outset.

I don't agree with stuffing non-arch directories with code which people haven't already agreed should be shared. As I've already said, in my experience it's hard to get agreement in the first place and even when you can the API generally needs to be changed from what you first think would be reasonable.

Russell,

Quoted 4 lines · Russell King - ARM Linux

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

It's more than that. Several i.MX chips use plat-mxc/iram_alloc.c.

lpc32xx and pnx4008 also use iram, but do not have an allocator (only 1 user). Both are doing a copy the suspend code to IRAM and run it which may also be a good thing to have generic code for. Several i.MX chips also need to run from IRAM for suspend.

-----Original Message-----

bounces at lists.infradead.org] On Behalf Of Russell King - ARM Linux
Sent: Friday, April 15, 2011 11:04 AM
To: Rob Herring
Cc: Kevin Hilman; davinci-linux-open-source at linux.davincidsp.com; Tony Lindgren; Sekhar Nori; linux- omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [RFC PATCH] Consolidate SRAM support

Quoted 8 lines · Rob Herring

Russell,

Quoted 4 lines · Russell King - ARM Linux

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Quoted 1 line

It's more than that. Several i.MX chips use plat-mxc/iram_alloc.c.

Hmm, that's nice - except for one issue. According to my grep of arch/arm/ and drivers/, nothing uses iram_alloc(). So, does anything in the MX stuff use iram_alloc.c, or is it dead code left over from a previous experiment?

The commit says:

ARM: imx: Add iram allocator functions

Add IRAM(Internal RAM) allocation functions using GENERIC_ALLOCATOR. The allocation size is 4KB multiples to guarantee alignment. The idea for these functions is for i.MX platforms to use them to dynamically allocate IRAM usage.

   Applies on 2.6.36-rc7

Quoted 4 lines

lpc32xx and pnx4008 also use iram, but do not have an allocator (only 1 user). Both are doing a copy the suspend code to IRAM and run it which may also be a good thing to have generic code for. Several i.MX chips also need to run from IRAM for suspend.

We have support for copying functions to other bits of memory and getting the Thumb-ness right - see asm/fncpy.h. So that's a separate patch to convert them over.

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

Quoted 4 lines

Hmm, that's nice - except for one issue. According to my grep of arch/arm/ and drivers/, nothing uses iram_alloc(). So, does anything in the MX stuff use iram_alloc.c, or is it dead code left over from a previous experiment?

This function will be used for suspend code in the mx5x series. I just got done submitting a series of patches to Sascha for a simple suspend that does not need running code out of IRAM yet. The next set of suspend patches will be using these iram functions.

Quoted 5 lines · On Fri, Apr 15, 2011 at 04:18:23PM +0000, Nguyen Dinh-R00091 wrote:

Quoted 4 lines

Hmm, that's nice - except for one issue. According to my grep of arch/arm/ and drivers/, nothing uses iram_alloc(). So, does anything in the MX stuff use iram_alloc.c, or is it dead code left over from a previous experiment?

Quoted 4 lines

This function will be used for suspend code in the mx5x series. I just got done submitting a series of patches to Sascha for a simple suspend that does not need running code out of IRAM yet. The next set of suspend patches will be using these iram functions.

Okay, so does iram_alloc() need to return a dma_addr_t or a phys_addr_t ? Are you dealing with physical addresses for it or DMA addresses?

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, April 15, 2011 11:59 AM
To: Nguyen Dinh-R00091
Cc: Kevin Hilman; davinci-linux-open-source at linux.davincidsp.com; Tony Lindgren; Sekhar Nori; linux- omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [RFC PATCH] Consolidate SRAM support

Quoted 11 lines · Russell King - ARM Linux

Quoted 9 lines · On Fri, Apr 15, 2011 at 04:18:23PM +0000, Nguyen Dinh-R00091 wrote:

Quoted 4 lines

Hmm, that's nice - except for one issue. According to my grep of arch/arm/ and drivers/, nothing uses iram_alloc(). So, does anything in the MX stuff use iram_alloc.c, or is it dead code left over from a previous experiment?

This function will be used for suspend code in the mx5x series. I just got done submitting a series of patches to Sascha for a simple suspend that does not need running code out of IRAM yet. The next set of suspend patches will be using these iram functions.

Quoted 2 lines

Okay, so does iram_alloc() need to return a dma_addr_t or a phys_addr_t ? Are you dealing with physical addresses for it or DMA addresses?

And another question: why does iram_alloc() return a void __iomem * for the virtual address, and the physical address via a pointer argument. However, iram_free() take an unsigned long for the address.

It seems rather inconsistent that the parameter for free is returned via a pointer argument.

If I convert this to sram-pool, can we change this to:

static inline void *iram_alloc(size_t size, phys_addr_t *phys_addr)

{
	return sram_pool_alloc(iram_pool, size, phys_addr);
}

static void iram_free(void *addr, size_t size)

{
	sram_pool_free(iram_pool, addr, size);
}

and:

int __init iram_init(unsigned long base, unsigned long size)

becomes:

int __init iram_init(phys_addr_t base, size_t size)

?

Quoted 21 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, April 15, 2011 11:59 AM
To: Nguyen Dinh-R00091
Cc: Kevin Hilman; davinci-linux-open-source at linux.davincidsp.com; Tony Lindgren; Sekhar Nori; linux- omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [RFC PATCH] Consolidate SRAM support

Quoted 11 lines · Russell King - ARM Linux

Quoted 9 lines · On Fri, Apr 15, 2011 at 04:18:23PM +0000, Nguyen Dinh-R00091 wrote:

Quoted 4 lines

Hmm, that's nice - except for one issue. According to my grep of arch/arm/ and drivers/, nothing uses iram_alloc(). So, does anything in the MX stuff use iram_alloc.c, or is it dead code left over from a previous experiment?

This function will be used for suspend code in the mx5x series. I just got done submitting a series of patches to Sascha for a simple suspend that does not need running code out of IRAM yet. The next set of suspend patches will be using these iram functions.

Quoted 6 lines

Quoted 2 lines

Okay, so does iram_alloc() need to return a dma_addr_t or a phys_addr_t ? Are you dealing with physical addresses for it or DMA addresses?

And another question: why does iram_alloc() return a void __iomem * for the virtual address, and the physical address via a pointer argument. However, iram_free() take an unsigned long for the address.

Yes, should just be a void *iram_alloc().

Quoted 24 lines

It seems rather inconsistent that the parameter for free is returned via a pointer argument.

If I convert this to sram-pool, can we change this to:

static inline void *iram_alloc(size_t size, phys_addr_t *phys_addr)

{
	return sram_pool_alloc(iram_pool, size, phys_addr);
}

static void iram_free(void *addr, size_t size)

{
	sram_pool_free(iram_pool, addr, size);
}

and:

int __init iram_init(unsigned long base, unsigned long size)

becomes:

int __init iram_init(phys_addr_t base, size_t size)

?

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, April 15, 2011 2:40 PM
To: Nguyen Dinh-R00091
Cc: Kevin Hilman; davinci-linux-open-source at linux.davincidsp.com; Tony Lindgren; Sekhar Nori; linux- omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [RFC PATCH] Consolidate SRAM support

Quoted 6 lines · On Fri, Apr 15, 2011 at 07:20:12PM +0000, Nguyen Dinh-R00091 wrote:

Quoted 6 lines

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: Friday, April 15, 2011 11:59 AM
To: Nguyen Dinh-R00091
Cc: Kevin Hilman; davinci-linux-open-source at linux.davincidsp.com; Tony Lindgren; Sekhar Nori;

linux-

Quoted 18 lines

Quoted 18 lines

omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [RFC PATCH] Consolidate SRAM support

Quoted 13 lines · Russell King - ARM Linux

Quoted 9 lines · On Fri, Apr 15, 2011 at 04:18:23PM +0000, Nguyen Dinh-R00091 wrote:

Quoted 4 lines

Hmm, that's nice - except for one issue. According to my grep of arch/arm/ and drivers/, nothing uses iram_alloc(). So, does anything in the MX stuff use iram_alloc.c, or is it dead code left over from a previous experiment?

This function will be used for suspend code in the mx5x series. I just got done submitting a series of patches to Sascha for a simple suspend that does not need running code out of IRAM yet. The next set of suspend patches will be using these iram functions.

Okay, so does iram_alloc() need to return a dma_addr_t or a phys_addr_t ? Are you dealing with physical addresses for it or DMA addresses?

Quoted 5 lines

Quoted 3 lines

And another question: why does iram_alloc() return a void __iomem * for the virtual address, and the physical address via a pointer argument. However, iram_free() take an unsigned long for the address.

Yes, should just be a void *iram_alloc().

Thanks. As you didn't comment against the change below, I'm going to assume that you're happy with it. It means that the usage changes from:

	unsigned long dma;
	void __iomem *addr = iram_alloc(SZ_4K, &dma);
	...
	iram_free(dma, SZ_4K);

to:

	phys_addr_t phys;
	void *addr = iram_alloc(SZ_4K, &phys);
	...
	iram_free(addr, SZ_4K);

Quoted 26 lines

Quoted 24 lines

It seems rather inconsistent that the parameter for free is returned via a pointer argument.

If I convert this to sram-pool, can we change this to:

static inline void *iram_alloc(size_t size, phys_addr_t *phys_addr)

{
	return sram_pool_alloc(iram_pool, size, phys_addr);
}

static void iram_free(void *addr, size_t size)

{
	sram_pool_free(iram_pool, addr, size);
}

and:

int __init iram_init(unsigned long base, unsigned long size)

becomes:

int __init iram_init(phys_addr_t base, size_t size)

?

Hi Russel,

Just small comment,

On Fri, Apr 15, 2011 at 08:06:07AM -0500, Russell King wrote:

arch/arm/plat-omap/sram.c line 78

Quoted 9 lines

arch/arm/plat-omap/sram.c +0 -1

index a3f50b3..3588749 100644
@@ -75,7 +75,6 @@
 static unsigned long omap_sram_start;
 static unsigned long omap_sram_base;
 static unsigned long omap_sram_size;
-static unsigned long omap_sram_ceil;

I think you missed one occurrence of omap_sram_ceil at omap3_sram_restore_context. Probably you have compile-tested w/o CONFIG_PM?

This is work in progress.

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

sram_create(phys, size)
virt = map sram(phys, size)
create sram pool(virt, phys, size, min_alloc_order)

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.

  arch/arm/common/sram-pool.c                 |   69 +++++++++++++++++++++++++++
  arch/arm/include/asm/sram-pool.h            |   20 ++++++++

I'd love to have the mapping inside the create pool, but that might not be possible in general.

Quoted 4 lines

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.

Quoted 2 lines

  arch/arm/common/sram-pool.c                 |   69 +++++++++++++++++++++++++++
  arch/arm/include/asm/sram-pool.h            |   20 ++++++++

Waht is ARM specific about this code?
Why not put it into lib/ and include/linux, respectively?

Quoted 2 lines · Detlef Vollmann

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.

Quoted 7 lines

Quoted 4 lines

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?

I'm really not interested in sorting out peoples wishlist items in the process of consolidation.

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.

Quoted 4 lines

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.

Quoted 1 line

This is work in progress.

Thanks, very useful.

Quoted 4 lines

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.

Le 15/04/2011 16:50, Detlef Vollmann :

Quoted 3 lines · Russell King - ARM Linux

Quoted 1 line

This is work in progress.

Thanks, very useful.

[..]

Quoted 7 lines

Quoted 4 lines

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.

In fact on at91sam9g20 (and some other at91) you can use the mirroring of the SRAM until next bank... so you end up with a single pool.

Base @ sram1 base - sram0 size
size = sram 0 size + sram 1 size

Quoted 5 lines · Nicolas Ferre

In fact on at91sam9g20 (and some other at91) you can use the mirroring of the SRAM until next bank... so you end up with a single pool.

Base @ sram1 base - sram0 size
size = sram 0 size + sram 1 size

Can you provide details?
I couldn't find anything in the documentation about this.

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Let's unify this before we have additional SoCs re-implementing this obviously common functionality themselves.

Unfortunately, we end up with code growth through doing this, but that will become a win when we have another SoC using this (which I know there's at least one in the pipeline).

One of the considerations here is that we can easily convert sram-pool.c to hook into device tree stuff, which can tell the sram allocator:
- physical address of sram
- size of sram
- allocation granularity
and then we just need to ensure that it is appropriately mapped.

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

sram_create(phys, size)
virt = map sram(phys, size)
create sram pool(virt, phys, size, min_alloc_order)

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.

Lastly, uio_pruss should probably take the SRAM pool pointer via platform data so that it doesn't have to include Davinci specific includes.

Signed-off-by Russell King <[email protected]>

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Quoted 4 lines · Russell King - ARM Linux

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

I havn't checked the details, but maybe the code in arch/arm/plat-mxc/iram_alloc.c could be migrated to your approach, too?

Quoted 7 lines · Uwe Kleine-K?nig

Quoted 4 lines · Russell King - ARM Linux

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

I havn't checked the details, but maybe the code in arch/arm/plat-mxc/iram_alloc.c could be migrated to your approach, too?

Its already in there now that I have replies from Nguyen Dinh. It looks like this presently:

 arch/arm/plat-mxc/Kconfig                   |    2 +-
 arch/arm/plat-mxc/include/mach/iram.h       |   24 +++++++--
 arch/arm/plat-mxc/iram_alloc.c              |   50 +++++---------------

and if we get rid of the iram_alloc/iram_free wrappers around the sram_pool (now pv_pool) alloc/free in iram.h, in the same way I've done for Davinci, then we get less new additions too.

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Let's unify this before we have additional SoCs re-implementing this obviously common functionality themselves.

Unfortunately, we end up with code growth through doing this, but that will become a win when we have another SoC using this (which I know there's at least one in the pipeline).

One of the considerations here is that we can easily convert sram-pool.c to hook into device tree stuff, which can tell the sram allocator:
? ? ? ?- physical address of sram
? ? ? ?- size of sram
? ? ? ?- allocation granularity
and then we just need to ensure that it is appropriately mapped.

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

sram_create(phys, size)
? ? ? ?virt = map sram(phys, size)
? ? ? ?create sram pool(virt, phys, size, min_alloc_order)

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.

Lastly, uio_pruss should probably take the SRAM pool pointer via platform data so that it doesn't have to include Davinci specific includes.

Signed-off-by Russell King <[email protected]>

Quoted 41 lines · Russell King - ARM Linux

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Let's unify this before we have additional SoCs re-implementing this obviously common functionality themselves.

Unfortunately, we end up with code growth through doing this, but that will become a win when we have another SoC using this (which I know there's at least one in the pipeline).

One of the considerations here is that we can easily convert sram-pool.c to hook into device tree stuff, which can tell the sram allocator:
? ? ? ?- physical address of sram
? ? ? ?- size of sram
? ? ? ?- allocation granularity
and then we just need to ensure that it is appropriately mapped.

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

sram_create(phys, size)
? ? ? ?virt = map sram(phys, size)
? ? ? ?create sram pool(virt, phys, size, min_alloc_order)

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.

Lastly, uio_pruss should probably take the SRAM pool pointer via platform data so that it doesn't have to include Davinci specific includes.

Quoted 1 line

Signed-off-by Russell King <[email protected]>

This common sram driver is good for us. It can benefit us on DMA usage. I just have one question on SRAM for storing instruction. We still need to copy code into SRAM and flush cache & TLB with this SRAM driver.

TCM driver can allocate code into SRAM section in link stage. It needs to update link file and virtual memory layout. Is it worth to make SRAM driver support this behavior? The case of using SRAM as memory for instruction is switching frequency or entering/exiting low power mode in PXA silicons.

Quoted 10 lines · Haojian Zhuang

On Fri, Apr 15, 2011 at 9:06 PM, Russell King - ARM Linux

Quoted 9 lines

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

Quoted 4 lines

TCM driver can allocate code into SRAM section in link stage. It needs to update link file and virtual memory layout. Is it worth to make SRAM driver support this behavior? The case of using SRAM as memory for instruction is switching frequency or entering/exiting low power mode in PXA silicons.

This is more or less the same scenario which OMAP is using its SRAM for, although that's explicitly SRAM and not TCM.

I don't think we need position dependent code requiring fixup for these applications as such things tend to be fairly easily coded in a position independent way.

Also note that C functions can't be copied because C produces position dependent code, and we have no way of extracting the relocation dependencies from such code.

Quoted 11 lines · Russell King - ARM Linux

Quoted 10 lines · Haojian Zhuang

On Fri, Apr 15, 2011 at 9:06 PM, Russell King - ARM Linux

Quoted 9 lines

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

I think we can just remove the omapfb SRAM support for now as it should be optional. That will then leave out that dependency and can be added back once we have a common framework in place.

Tomi do you see any problems with that?

Quoted 13 lines · Tony Lindgren

Quoted 11 lines · Russell King - ARM Linux

Quoted 10 lines · Haojian Zhuang

On Fri, Apr 15, 2011 at 9:06 PM, Russell King - ARM Linux

Quoted 9 lines

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

Quoted 5 lines

I think we can just remove the omapfb SRAM support for now as it should be optional. That will then leave out that dependency and can be added back once we have a common framework in place.

Tomi do you see any problems with that?

I agree. Only OMAP2 has enough SRAM to possibly have a framebuffer there, and even on OMAP2 the SRAM size is so small that it works only for rather small displays. I'm not aware of anyone using omapfb's SRAM support.

The SRAM support also makes our video ram allocator and omapfb more complex, without much benefit, so I'm more than happy to remove it totally. Additionally, I believe there is work going on with memory allocators that omapfb could also use, and migrating to any new mem allocator will no doubt be much easier if we just handle normal RAM.

So, I can make a patch that removes the SRAM support from omapfb, and queue it up for the next merge window.

Quoted 2 lines · Tomi Valkeinen

So, I can make a patch that removes the SRAM support from omapfb, and queue it up for the next merge window.

OK. That patch should probably go into Russell's tree along with other SRAM related patches to avoid conflicts.

Hi Tony and Russell,

Quoted 7 lines · Tony Lindgren

Quoted 2 lines · Tomi Valkeinen

So, I can make a patch that removes the SRAM support from omapfb, and queue it up for the next merge window.

OK. That patch should probably go into Russell's tree along with other SRAM related patches to avoid conflicts.

Here's a simple patch to remove SRAM support from omapfb. Or more precisely, this just disables it.

I started to remove the SRAM support from the drivers, but the patches got quite big and will probably cause conflicts if they reside in somebody else's tree than mine. So I believe it's easiest to divide SRAM FB removal into this patch, handled by Russell (?), and then the rest, handled by me.

Is there something else related to OMAP FB than the code removed in this patch that is hindering the SRAM work?

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Let's unify this before we have additional SoCs re-implementing this obviously common functionality themselves.

Unfortunately, we end up with code growth through doing this, but that will become a win when we have another SoC using this (which I know there's at least one in the pipeline).

One of the considerations here is that we can easily convert sram-pool.c to hook into device tree stuff, which can tell the sram allocator:
- physical address of sram
- size of sram
- allocation granularity
and then we just need to ensure that it is appropriately mapped.

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

sram_create(phys, size)
virt = map sram(phys, size)
create sram pool(virt, phys, size, min_alloc_order)

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.

Lastly, uio_pruss should probably take the SRAM pool pointer via platform data so that it doesn't have to include Davinci specific includes.

This is the second revision of this patch. ?I've not moved it out of ARM yet as I haven't had a positive response from SH yet.

It's now called pv_pool (for phys/virt pool) rather than sram_pool, and I've included MXC's iram support in this. ?Hopefully, if OMAP can remove the FB stuff from SRAM we can clean the OMAP bits up a little more. ?Neither have I sorted out the last reference to omap_sram_ceil. Some comments from OMAP people on what's going on there would be good.

Quoted 37 lines · Russell King - ARM Linux

This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Let's unify this before we have additional SoCs re-implementing this obviously common functionality themselves.

Unfortunately, we end up with code growth through doing this, but that will become a win when we have another SoC using this (which I know there's at least one in the pipeline).

One of the considerations here is that we can easily convert sram-pool.c to hook into device tree stuff, which can tell the sram allocator:
? ? ? - physical address of sram
? ? ? - size of sram
? ? ? - allocation granularity
and then we just need to ensure that it is appropriately mapped.

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

sram_create(phys, size)
? ? ? virt = map sram(phys, size)
? ? ? create sram pool(virt, phys, size, min_alloc_order)

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.

Quoted 3 lines

Lastly, uio_pruss should probably take the SRAM pool pointer via platform data so that it doesn't have to include Davinci specific includes.

This is the second revision of this patch. I've not moved it out of ARM yet as I haven't had a positive response from SH yet.

It's now called pv_pool (for phys/virt pool) rather than sram_pool, and I've included MXC's iram support in this. Hopefully, if OMAP can remove the FB stuff from SRAM we can clean the OMAP bits up a little more. Neither have I sorted out the last reference to omap_sram_ceil. Some comments from OMAP people on what's going on there would be good.

This is the second revision of this patch.

This is the second revision of this patch. I've not moved it out of ARM yet as I haven't had a positive response from SH yet.

Quoted 3 lines

Lastly, uio_pruss should probably take the SRAM pool pointer via platform data so that it doesn't have to include Davinci specific includes.

 arch/arm/Kconfig                            |    2 +
 arch/arm/common/Kconfig                     |    4 ++
 arch/arm/common/Makefile                    |    1 +
 arch/arm/common/pv-pool.c                   |   69 +++++++++++++++++++++++++++
 arch/arm/include/asm/pv-pool.h              |   20 ++++++++
 arch/arm/mach-davinci/da850.c               |    2 +-
 arch/arm/mach-davinci/dm355.c               |    2 +-
 arch/arm/mach-davinci/dm365.c               |    2 +-
 arch/arm/mach-davinci/dm644x.c              |    2 +-
 arch/arm/mach-davinci/dm646x.c              |    2 +-
 arch/arm/mach-davinci/include/mach/common.h |    2 +-
 arch/arm/mach-davinci/include/mach/sram.h   |   13 +----
 arch/arm/mach-davinci/pm.c                  |   12 +----

 arch/arm/mach-davinci/sram.c                |   42 +++--------------
 arch/arm/plat-mxc/Kconfig                   |    2 +-
 arch/arm/plat-mxc/include/mach/iram.h       |   24 +++++++--
 arch/arm/plat-mxc/iram_alloc.c              |   50 +++++---------------
 arch/arm/plat-omap/include/plat/sram.h      |   17 ++++---
 arch/arm/plat-omap/sram.c                   |   34 +++++---------
 drivers/uio/uio_pruss.c                     |    7 ++-
 20 files changed, 171 insertions(+), 138 deletions(-)

I was able to test this on DaVinci (DA850 suspend-to-RAM) with the following additional changes:

There was a sram_free call remaining in pm.c file.

The davinci audio driver in sound/soc/davinci/davinci-pcm.c uses the sram allocator too and would need to be converted to the new API.

--- /dev/null > +++ b/arch/arm/common/pv-pool.c

arch/arm/common/pv-pool.c new file +69 -0

@@ -0,0 +1,69 @@
+/*
+ * Unified Phys/Virt allocator, based on mach-davinci/sram.c, which was
+ * Copyright (C) 2009 David Brownell.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/dma-mapping.h>
+#include <linux/genalloc.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include <asm/pv-pool.h>
+
+struct pv_pool {
+	struct gen_pool *genpool;
+	void *cpu_base;
+	phys_addr_t phys_base;
+};
+
+void *pv_pool_alloc(struct pv_pool *pool, size_t len, phys_addr_t *phys)
+{
+	void *addr = (void *)gen_pool_alloc(pool->genpool, len);
+
+	if (phys)
+		*phys = addr ? (pool->phys_base + (addr - pool->cpu_base)) :
+			 (phys_addr_t)-1ULL;
+
+	return addr;
+}
+EXPORT_SYMBOL_GPL(pv_pool_alloc);
+
+void pv_pool_free(struct pv_pool *pool, void *addr, size_t len)
+{
+	gen_pool_free(pool->genpool, (unsigned long)addr, len);
+}
+EXPORT_SYMBOL_GPL(pv_pool_free);
+
+struct pv_pool *pv_pool_create(void *addr, phys_addr_t phys, size_t len,
+	int min_alloc_order)
+{
+	struct pv_pool *pool = kzalloc(sizeof(struct pv_pool), GFP_KERNEL);
+
+	if (pool) {
+		pool->cpu_base = addr;
+		pool->phys_base = phys;
+		pool->genpool = gen_pool_create(min_alloc_order, -1);
+		if (!pool->genpool) {
+			kfree(pool);
+			pool = NULL;
+		} else {
+			WARN_ON(gen_pool_add(pool->genpool, (unsigned long)addr,
+						len, -1) < 0);
+		}
+	}
+
+	return pool;
+}
+EXPORT_SYMBOL_GPL(pv_pool_create);
+
+void pv_pool_destroy(struct pv_pool *pool)
+{
+	gen_pool_destroy(pool->genpool);
+	kfree(pool);
+}
+EXPORT_SYMBOL_GPL(pv_pool_destroy);

arch/arm/include/asm/pv-pool.h new file +20 -0

new file mode 100644
index 0000000..b7ae871
@@ -0,0 +1,20 @@
+#ifndef __ASMARM_PV_POOL_H
+#define __ASMARM_PV_POOL_H
+
+#include <asm/fncpy.h>
+
+struct pv_pool;
+
+void *pv_pool_alloc(struct pv_pool *, size_t, phys_addr_t *);
+void pv_pool_free(struct pv_pool *, void *, size_t);
+struct pv_pool *pv_pool_create(void *, phys_addr_t, size_t, int);
+void pv_pool_destroy(struct pv_pool *);
+
+/* Macro to copy a function into SRAM, using the fncpy API */
+#define pv_pool_fncpy(pool, funcp, size) ({			\
+	size_t _sz = size;					\
+	void *_sram = pv_pool_alloc(pool, _sz, NULL);		\
+	(_sram ? fncpy(_sram, &(funcp), _sz) : NULL);		\
+})
+
+#endif

arch/arm/mach-davinci/da850.c +1 -1

index 68fe4c2..5eca128 100644
@@ -1099,7 +1099,7 @@ static struct davinci_soc_info davinci_soc_info_da850 = {
 	.gpio_irq		= IRQ_DA8XX_GPIO0,
 	.serial_dev		= &da8xx_serial_device,
 	.emac_pdata		= &da8xx_emac_pdata,
-	.sram_dma		= DA8XX_ARM_RAM_BASE,
+	.sram_phys		= DA8XX_ARM_RAM_BASE,
 	.sram_len		= SZ_8K,
 	.reset_device		= &da8xx_wdt_device,
 };

arch/arm/mach-davinci/dm355.c +1 -1

index 76364d1..3df8730 100644
@@ -850,7 +850,7 @@ static struct davinci_soc_info davinci_soc_info_dm355 = {
 	.gpio_num		= 104,
 	.gpio_irq		= IRQ_DM355_GPIOBNK0,
 	.serial_dev		= &dm355_serial_device,
-	.sram_dma		= 0x00010000,
+	.sram_phys		= 0x00010000,
 	.sram_len		= SZ_32K,
 	.reset_device		= &davinci_wdt_device,
 };

arch/arm/mach-davinci/dm365.c +1 -1

index 4604e72..d306034 100644
@@ -1082,7 +1082,7 @@ static struct davinci_soc_info davinci_soc_info_dm365 = {
 	.gpio_unbanked		= 8,	/* really 16 ... skip muxed GPIOs */
 	.serial_dev		= &dm365_serial_device,
 	.emac_pdata		= &dm365_emac_pdata,
-	.sram_dma		= 0x00010000,
+	.sram_phys		= 0x00010000,
 	.sram_len		= SZ_32K,
 	.reset_device		= &davinci_wdt_device,
 };

arch/arm/mach-davinci/dm644x.c +1 -1

index 9a2376b..4ca7295 100644
@@ -764,7 +764,7 @@ static struct davinci_soc_info davinci_soc_info_dm644x = {
 	.gpio_irq		= IRQ_GPIOBNK0,
 	.serial_dev		= &dm644x_serial_device,
 	.emac_pdata		= &dm644x_emac_pdata,
-	.sram_dma		= 0x00008000,
+	.sram_phys		= 0x00008000,
 	.sram_len		= SZ_16K,
 	.reset_device		= &davinci_wdt_device,
 };

arch/arm/mach-davinci/dm646x.c +1 -1

index 1e0f809..a4365f7 100644
@@ -848,7 +848,7 @@ static struct davinci_soc_info davinci_soc_info_dm646x = {
 	.gpio_irq		= IRQ_DM646X_GPIOBNK0,
 	.serial_dev		= &dm646x_serial_device,
 	.emac_pdata		= &dm646x_emac_pdata,
-	.sram_dma		= 0x10010000,
+	.sram_phys		= 0x10010000,
 	.sram_len		= SZ_32K,
 	.reset_device		= &davinci_wdt_device,
 };

arch/arm/mach-davinci/include/mach/common.h +1 -1

index a57cba2..665d049 100644
@@ -75,7 +75,7 @@ struct davinci_soc_info {
 	int				gpio_ctlrs_num;
 	struct platform_device		*serial_dev;
 	struct emac_platform_data	*emac_pdata;
-	dma_addr_t			sram_dma;
+	phys_addr_t			sram_phys;
 	unsigned			sram_len;
 	struct platform_device		*reset_device;
 	void				(*reset)(struct platform_device *);

arch/arm/mach-davinci/include/mach/sram.h +3 -10

index 111f7cc..7d77e85 100644
@@ -10,18 +10,11 @@
 #ifndef __MACH_SRAM_H
 #define __MACH_SRAM_H
 
+#include <asm/pv-pool.h>
+
 /* ARBITRARY:  SRAM allocations are multiples of this 2^N size */
 #define SRAM_GRANULARITY	512
 
-/*
- * SRAM allocations return a CPU virtual address, or NULL on error.
- * If a DMA address is requested and the SRAM supports DMA, its
- * mapped address is also returned.
- *
- * Errors include SRAM memory not being available, and requesting
- * DMA mapped SRAM on systems which don't allow that.
- */
-extern void *sram_alloc(size_t len, dma_addr_t *dma);
-extern void sram_free(void *addr, size_t len);
+extern struct pv_pool *davinci_pv_pool;
 
 #endif /* __MACH_SRAM_H */

arch/arm/mach-davinci/pm.c +2 -10

index 1bd73a0..f69cd7b 100644
@@ -29,12 +29,6 @@
 static void (*davinci_sram_suspend) (struct davinci_pm_config *);
 static struct davinci_pm_config *pdata;
 
-static void davinci_sram_push(void *dest, void *src, unsigned int size)
-{
-	memcpy(dest, src, size);
-	flush_icache_range((unsigned long)dest, (unsigned long)(dest + size));
-}
-
 static void davinci_pm_suspend(void)
 {
 	unsigned val;

@@ -123,15 +117,13 @@ static int __init davinci_pm_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	davinci_sram_suspend = sram_alloc(davinci_cpu_suspend_sz, NULL);
+	davinci_sram_suspend = pv_pool_fncpy(davinci_pv_pool,
+				davinci_cpu_suspend, davinci_cpu_suspend_sz);
 	if (!davinci_sram_suspend) {
 		dev_err(&pdev->dev, "cannot allocate SRAM memory\n");
 		return -ENOMEM;
 	}
 
-	davinci_sram_push(davinci_sram_suspend, davinci_cpu_suspend,
-						davinci_cpu_suspend_sz);
-
 	suspend_set_ops(&davinci_pm_ops);
 
 	return 0;

arch/arm/mach-davinci/sram.c +7 -35

index db0f778..ebd4d67 100644
@@ -10,40 +10,13 @@
  */
 #include <linux/module.h>
 #include <linux/init.h>
-#include <linux/genalloc.h>
+#include <asm/pv-pool.h>
 
 #include <mach/common.h>
 #include <mach/sram.h>
 
-static struct gen_pool *sram_pool;
-
-void *sram_alloc(size_t len, dma_addr_t *dma)
-{
-	unsigned long vaddr;
-	dma_addr_t dma_base = davinci_soc_info.sram_dma;
-
-	if (dma)
-		*dma = 0;
-	if (!sram_pool || (dma && !dma_base))
-		return NULL;
-
-	vaddr = gen_pool_alloc(sram_pool, len);
-	if (!vaddr)
-		return NULL;
-
-	if (dma)
-		*dma = dma_base + (vaddr - SRAM_VIRT);
-	return (void *)vaddr;
-
-}
-EXPORT_SYMBOL(sram_alloc);
-
-void sram_free(void *addr, size_t len)
-{
-	gen_pool_free(sram_pool, (unsigned long) addr, len);
-}
-EXPORT_SYMBOL(sram_free);
-
+struct pv_pool *davinci_pv_pool;
+EXPORT_SYMBOL_GPL(davinci_pv_pool);
 
 /*
  * REVISIT This supports CPU and DMA access to/from SRAM, but it

@@ -58,13 +31,12 @@ static int __init sram_init(void)
 
 	if (len) {
 		len = min_t(unsigned, len, SRAM_SIZE);
-		sram_pool = gen_pool_create(ilog2(SRAM_GRANULARITY), -1);
-		if (!sram_pool)
+		davinci_pv_pool = pv_pool_create((void *)SRAM_VIRT,
+					davinci_soc_info.sram_phys, len,
+					ilog2(SRAM_GRANULARITY));
+		if (!davinci_pv_pool)
 			status = -ENOMEM;
 	}
-	if (sram_pool)
-		status = gen_pool_add(sram_pool, SRAM_VIRT, len, -1);
-	WARN_ON(status < 0);
 	return status;
 }
 core_initcall(sram_init);

arch/arm/plat-mxc/Kconfig +1 -1

index b0cb425..f60d5ea 100644
@@ -118,6 +118,6 @@ config ARCH_MXC_AUDMUX_V2
 
 config IRAM_ALLOC
 	bool
-	select GENERIC_ALLOCATOR
+	select PV_POOL
 
 endif

arch/arm/plat-mxc/include/mach/iram.h +18 -6

index 022690c..543c6df 100644
@@ -17,25 +17,37 @@
  * MA 02110-1301, USA.
  */
 #include <linux/errno.h>
+#include <asm/pv-pool.h>
 
 #ifdef CONFIG_IRAM_ALLOC
 
-int __init iram_init(unsigned long base, unsigned long size);
-void __iomem *iram_alloc(unsigned int size, unsigned long *dma_addr);
-void iram_free(unsigned long dma_addr, unsigned int size);
+int __init iram_init(phys_addr_t base, size_t size);
+
+extern struct pv_pool *mxc_iram_pool;
+
+static inline void *iram_alloc(size_t size, phys_addr_t *phys)
+{
+	return pv_pool_alloc(iram_pool, size, phys);
+}
+
+static inline void iram_free(void *addr, size_t size)
+{
+	pv_pool_free(iram_pool, addr, size);
+}
 
 #else
 
-static inline int __init iram_init(unsigned long base, unsigned long size)
+static inline int __init iram_init(phys_addr_t base, size_t size)
 {
 	return -ENOMEM;
 }
 
-static inline void __iomem *iram_alloc(unsigned int size, unsigned long *dma_addr)
+static inline void *iram_alloc(size_t size, phys_addr_t *phys)
 {
+	*phys = (phys_addr_t)-1ULL;
 	return NULL;
 }
 
-static inline void iram_free(unsigned long base, unsigned long size) {}
+static inline void iram_free(void *addr, size_t size) {}
 
 #endif

arch/arm/plat-mxc/iram_alloc.c +12 -38

index 074c386..1e3d437 100644
@@ -24,50 +24,24 @@
 #include <linux/genalloc.h>
 #include <mach/iram.h>
 
-static unsigned long iram_phys_base;
-static void __iomem *iram_virt_base;
-static struct gen_pool *iram_pool;
+struct pv_pool *mxc_iram_pool;
+EXPORT_SYMBOL(mxc_iram_pool);
 
-static inline void __iomem *iram_phys_to_virt(unsigned long p)
+int __init iram_init(phys_addr_t base, size_t size)
 {
-	return iram_virt_base + (p - iram_phys_base);
-}
-
-void __iomem *iram_alloc(unsigned int size, unsigned long *dma_addr)
-{
-	if (!iram_pool)
-		return NULL;
-
-	*dma_addr = gen_pool_alloc(iram_pool, size);
-	pr_debug("iram alloc - %dB at 0x%lX\n", size, *dma_addr);
-	if (!*dma_addr)
-		return NULL;
-	return iram_phys_to_virt(*dma_addr);
-}
-EXPORT_SYMBOL(iram_alloc);
-
-void iram_free(unsigned long addr, unsigned int size)
-{
-	if (!iram_pool)
-		return;
-
-	gen_pool_free(iram_pool, addr, size);
-}
-EXPORT_SYMBOL(iram_free);
+	void *addr = /*FIXME*/ ioremap(base, size);
 
-int __init iram_init(unsigned long base, unsigned long size)
-{
-	iram_phys_base = base;
+	if (!addr)
+		return -EIO;
 
-	iram_pool = gen_pool_create(PAGE_SHIFT, -1);
-	if (!iram_pool)
+	mxc_iram_pool = pv_pool_create(addr, base, size, PAGE_SHIFT);
+	if (!mxc_iram_pool) {
+		iounmap(addr);
 		return -ENOMEM;
+	}
 
-	gen_pool_add(iram_pool, base, size, -1);
-	iram_virt_base = ioremap(iram_phys_base, size);
-	if (!iram_virt_base)
-		return -EIO;
+	pr_debug("i.MX IRAM pool: %lu KB at 0x%08llx\n", size / 1024,
+		 (unsigned long long)base);
 
-	pr_debug("i.MX IRAM pool: %ld KB at 0x%p\n", size / 1024, iram_virt_base);
 	return 0;
 }

arch/arm/plat-omap/include/plat/sram.h +10 -7

index f500fc3..9fc27d0 100644
@@ -12,16 +12,19 @@
 #define __ARCH_ARM_OMAP_SRAM_H
 
 #ifndef __ASSEMBLY__
-#include <asm/fncpy.h>
+#include <asm/pv-pool.h>
 
-extern void *omap_sram_push_address(unsigned long size);
+extern struct pv_pool *omap_pv_pool;
 
-/* Macro to push a function to the internal SRAM, using the fncpy API */
+/*
+ * Note that fncpy requires the SRAM address to be aligned to an 8-byte
+ * boundary, so the min_alloc_order for the pool is set appropriately.
+ */
 #define omap_sram_push(funcp, size) ({				\
-	typeof(&(funcp)) _res = NULL;				\
-	void *_sram_address = omap_sram_push_address(size);	\
-	if (_sram_address)					\
-		_res = fncpy(_sram_address, &(funcp), size);	\
+	typeof(&(funcp)) _res;					\
+	_res = pv_pool_fncpy(omap_pv_pool, funcp, size);	\
+	if (!_res)						\
+		pr_err("Not enough space in SRAM\n");		\
 	_res;							\
 })
 

arch/arm/plat-omap/sram.c +12 -22

index a3f50b3..3588749 100644
@@ -75,7 +75,6 @@
 static unsigned long omap_sram_start;
 static unsigned long omap_sram_base;
 static unsigned long omap_sram_size;
-static unsigned long omap_sram_ceil;
 
 /*
  * Depending on the target RAMFS firewall setup, the public usable amount of

@@ -104,6 +103,8 @@ static int is_sram_locked(void)
 		return 1; /* assume locked with no PPA or security driver */
 }
 
+struct pv_pool *omap_pv_pool;
+
 /*
  * The amount of SRAM depends on the core type.
  * Note that we cannot try to test for SRAM here because writes

@@ -182,7 +183,16 @@ static void __init omap_detect_sram(void)
 			omap_sram_size - SRAM_BOOTLOADER_SZ);
 	omap_sram_size -= reserved;
 
-	omap_sram_ceil = omap_sram_base + omap_sram_size;
+	{
+		/* The first SRAM_BOOTLOADER_SZ of SRAM are reserved */
+		void *base = (void *)omap_sram_base + SRAM_BOOTLOADER_SZ;
+		phys_addr_t phys = omap_sram_start + SRAM_BOOTLOADER_SZ;
+		size_t len = omap_sram_size - SRAM_BOOTLOADER_SZ;
+
+		omap_pv_pool = pv_pool_create(base, phys, len,
+						ilog2(FNCPY_ALIGN));
+		WARN_ON(!omap_pv_pool);
+	}
 }
 
 static struct map_desc omap_sram_io_desc[] __initdata = {

@@ -242,26 +252,6 @@ static void __init omap_map_sram(void)
 	       omap_sram_size - SRAM_BOOTLOADER_SZ);
 }
 
-/*
- * Memory allocator for SRAM: calculates the new ceiling address
- * for pushing a function using the fncpy API.
- *
- * Note that fncpy requires the returned address to be aligned
- * to an 8-byte boundary.
- */
-void *omap_sram_push_address(unsigned long size)
-{
-	if (size > (omap_sram_ceil - (omap_sram_base + SRAM_BOOTLOADER_SZ))) {
-		printk(KERN_ERR "Not enough space in SRAM\n");
-		return NULL;
-	}
-
-	omap_sram_ceil -= size;
-	omap_sram_ceil = ROUND_DOWN(omap_sram_ceil, FNCPY_ALIGN);
-
-	return (void *)omap_sram_ceil;
-}
-
 #ifdef CONFIG_ARCH_OMAP1
 
 static void (*_omap_sram_reprogram_clock)(u32 dpllctl, u32 ckctl);

drivers/uio/uio_pruss.c +4 -3

index daf6e77..2be3155 100644
@@ -62,7 +62,7 @@ MODULE_PARM_DESC(extram_pool_sz, "external ram pool size to allocate");
 struct uio_pruss_dev {
 	struct uio_info *info;
 	struct clk *pruss_clk;
-	dma_addr_t sram_paddr;
+	phys_addr_t sram_paddr;
 	dma_addr_t ddr_paddr;
 	void __iomem *prussio_vaddr;
 	void *sram_vaddr;

@@ -106,7 +106,7 @@ static void pruss_cleanup(struct platform_device *dev,
 			gdev->ddr_paddr);
 	}
 	if (gdev->sram_vaddr)
-		sram_free(gdev->sram_vaddr, sram_pool_sz);
+		pv_pool_free(davinci_pv_pool, gdev->sram_vaddr, sram_pool_sz);
 	kfree(gdev->info);
 	clk_put(gdev->pruss_clk);
 	kfree(gdev);

@@ -152,7 +152,8 @@ static int __devinit pruss_probe(struct platform_device *dev)
 		goto out_free;
 	}
 
-	gdev->sram_vaddr = sram_alloc(sram_pool_sz, &(gdev->sram_paddr));
+	gdev->sram_vaddr = pv_pool_alloc(davinci_pv_pool, sram_pool_sz,
+					   &(gdev->sram_paddr));
 	if (!gdev->sram_vaddr) {
 		dev_err(&dev->dev, "Could not allocate SRAM pool\n");
 		goto out_free;

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

Hi,

I do post a patch to add the support to specify a virt and phys address to the generic allocator so the pv-pool.c is not needed we can just use the generic fucntion

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

Hi,

I do post a patch to add the support to specify a virt and phys address to the generic allocator so the pv-pool.c is not needed we can just use the generic fucntion

You've talked about this before in the thread, but the patch never appeared.

Quoted 8 lines · Russell King - ARM Linux

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

Hi,

I do post a patch to add the support to specify a virt and phys address to the generic allocator so the pv-pool.c is not needed we can just use the generic fucntion

You've talked about this before in the thread, but the patch never appeared.

I forget to re-send it sorry
it's the mm tree

Quoted 8 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Jean-Christophe PLAGNIOL-VILLARD

Hi,

I do post a patch to add the support to specify a virt and phys address to the generic allocator so the pv-pool.c is not needed we can just use the generic fucntion

Quoted 3 lines

Quoted 1 line

You've talked about this before in the thread, but the patch never appeared.

I forget to re-send it sorry
it's the mm tree

One obvious issue here is that you're using 'unsigned long' for phys addresses. With LPAE we could start seeing SRAM outside of the 4GB PA range, and so would need this to be phys_addr_t.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Let's unify this before we have additional SoCs re-implementing this obviously common functionality themselves.

On Fri, Apr 15, 2011 at 02:06:07PM +0100, Russell King - ARM Linux wrote:
This is work in progress.

We have two SoCs using SRAM, both with their own allocation systems, and both with their own ways of copying functions into the SRAM.

Let's unify this before we have additional SoCs re-implementing this obviously common functionality themselves.

Unfortunately, we end up with code growth through doing this, but that will become a win when we have another SoC using this (which I know there's at least one in the pipeline).

One of the considerations here is that we can easily convert sram-pool.c to hook into device tree stuff, which can tell the sram allocator:
- physical address of sram
- size of sram
- allocation granularity
and then we just need to ensure that it is appropriately mapped.

This uses the physical address, and unlike Davinci's dma address usage, it always wants to have the physical address, and will always return the corresponding physical address when passed that pointer.

OMAP could probably do with some more work to make the omapfb and other allocations use the sram allocator, rather than hooking in before the sram allocator is initialized - and then further cleanups so that we have an initialization function which just does

sram_create(phys, size)
virt = map sram(phys, size)
create sram pool(virt, phys, size, min_alloc_order)

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.

Lastly, uio_pruss should probably take the SRAM pool pointer via platform data so that it doesn't have to include Davinci specific includes.

Signed-off-by Russell King <[email protected]>

---

This version fixes the davinci pm free, and adds updates for the davinci pcm driver. As I don't know what's happening with Jean's patch tweaking the genpool allocator, I've kept my version.

This still suffers from the "only one region per pvpool" problem which I believe Jean's patch doesn't suffer.

This version fixes the davinci pm free, and adds updates for the davinci pcm driver. As I don't know what's happening with Jean's patch tweaking the genpool allocator, I've kept my version.

This still suffers from the "only one region per pvpool" problem which I believe Jean's patch doesn't suffer.

Quoted 3 lines · Russell King - ARM Linux

This version fixes the davinci pm free, and adds updates for the davinci pcm driver. As I don't know what's happening with Jean's patch tweaking the genpool allocator, I've kept my version.

Sorry I do not get the question. The latest changes I submitted have been merged in mainline as 'ARM: 6649/1: omap: use fncpy to copy the PM code functions to SRAM' [1] . Thanks for that!
This change is only about the simple linear SRAM allocator, not the genpool allocator.

Quoted 6 lines · Jean Pihet

Quoted 3 lines · Russell King - ARM Linux

? ?This version fixes the davinci pm free, and adds updates for the ? ?davinci pcm driver. ?As I don't know what's happening with Jean's ? ?patch tweaking the genpool allocator, I've kept my version.

Quoted 5 lines

Sorry I do not get the question. The latest changes I submitted have been merged in mainline as 'ARM: 6649/1: omap: use fncpy to copy the PM code functions to SRAM' [1] . Thanks for that!
This change is only about the simple linear SRAM allocator, not the genpool allocator.

The wrong Jean! ?I meant Jean-Christophe PLAGNIOL-VILLARD.

On Fri, Apr 15, 2011 at 02:06:07PM +0100, Russell King - ARM Linux wrote:
This is work in progress.

arch/arm/mach-davinci/pm.c +0 -0

index 1bd73a0..c2f9767 100644

arch/arm/mach-davinci/pm.c +1 -2

@@ -123,14 +118,14 @@ static int __init davinci_pm_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	davinci_sram_suspend = sram_alloc(davinci_cpu_suspend_sz, NULL);
-	if (!davinci_sram_suspend) {
+	davinci_sram_suspend_mem = pv_pool_alloc(davinci_pv_pool,

arch/arm/mach-davinci/pm.c +0 -0

index 1bd73a0..c2f9767 100644

arch/arm/mach-davinci/pm.c +1 -2

@@ -123,14 +118,14 @@ static int __init davinci_pm_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
-	davinci_sram_suspend = sram_alloc(davinci_cpu_suspend_sz, NULL);
-	if (!davinci_sram_suspend) {
+	davinci_sram_suspend_mem = pv_pool_alloc(davinci_pv_pool,

The current davinci init sets up SRAM in iotables. There has been an observed failure to boot a da850 with 128K specified in the iotable.

Make the davinci sram allocator -- now based on RMK's consolidated SRAM support -- do an ioremap of the region specified by the entries in davinci_soc_info before registering with pv_pool_create().

This commit breaks runtime of davinci boards since the regions that the sram init is now trying to ioremap have been iomapped by their iotable entries. The iotable entries will be removed in the patches to come.

Signed-off-by Ben Gardiner <[email protected]>

---
 arch/arm/mach-davinci/sram.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

arch/arm/mach-davinci/sram.c +2 -0

index 219d4c5..96026df 100644
@@ -8,6 +8,7 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <asm/pv-pool.h>

@@ -26,16 +27,23 @@ EXPORT_SYMBOL_GPL(davinci_pv_pool);
  */
 static int __init sram_init(void)
 {
+	void *addr;
 	unsigned len = davinci_soc_info.sram_len;
 	int status = 0;
 

Quoted 20 lines · Ben Gardiner

arch/arm/mach-davinci/sram.c +2 -0

index 219d4c5..96026df 100644
@@ -8,6 +8,7 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <asm/pv-pool.h>

@@ -26,16 +27,23 @@ EXPORT_SYMBOL_GPL(davinci_pv_pool);
  */
 static int __init sram_init(void)
 {
+	void *addr;
 	unsigned len = davinci_soc_info.sram_len;
 	int status = 0;
 

arch/arm/mach-davinci/sram.c EXPORT_SYMBOL_GPL(davinci_pv_pool); · line 42

Quoted 10 lines

arch/arm/mach-davinci/sram.c +5 -1

 	if (len) {
 		len = min_t(unsigned, len, SRAM_SIZE);
-		davinci_pv_pool = pv_pool_create((void *)SRAM_VIRT,
+		addr = ioremap(davinci_soc_info.sram_phys, len);
+		if (!addr)
+			return -EIO;
+
+		davinci_pv_pool = pv_pool_create(addr,
 					davinci_soc_info.sram_phys, len,
 					ilog2(SRAM_GRANULARITY));

please use the gennalloc directly as discuss with Andrew and Russell my patch will present in the .40

Hi Russell,

Quoted 22 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 20 lines · Ben Gardiner

arch/arm/mach-davinci/sram.c +2 -0

index 219d4c5..96026df 100644
@@ -8,6 +8,7 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <asm/pv-pool.h>

@@ -26,16 +27,23 @@ EXPORT_SYMBOL_GPL(davinci_pv_pool);
  */
 static int __init sram_init(void)
 {
+	void *addr;
 	unsigned len = davinci_soc_info.sram_len;
 	int status = 0;
 

Quoted 12 lines

arch/arm/mach-davinci/sram.c EXPORT_SYMBOL_GPL(davinci_pv_pool); · line 42

Quoted 10 lines

arch/arm/mach-davinci/sram.c +5 -1

 	if (len) {
 		len = min_t(unsigned, len, SRAM_SIZE);
-		davinci_pv_pool = pv_pool_create((void *)SRAM_VIRT,
+		addr = ioremap(davinci_soc_info.sram_phys, len);
+		if (!addr)
+			return -EIO;
+
+		davinci_pv_pool = pv_pool_create(addr,
 					davinci_soc_info.sram_phys, len,
 					ilog2(SRAM_GRANULARITY));

please use the gennalloc directly as discuss with Andrew and Russell my patch will present in the .40

With Jean-Christophe's patch getting merged, would you be dropping your PV pool patch?

Quoted 39 lines · Sekhar

Hi Russell,

Quoted 34 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 20 lines · Ben Gardiner

arch/arm/mach-davinci/sram.c +2 -0

index 219d4c5..96026df 100644
@@ -8,6 +8,7 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <asm/pv-pool.h>

@@ -26,16 +27,23 @@ EXPORT_SYMBOL_GPL(davinci_pv_pool);
  */
 static int __init sram_init(void)
 {
+	void *addr;
 	unsigned len = davinci_soc_info.sram_len;
 	int status = 0;
 

arch/arm/mach-davinci/sram.c EXPORT_SYMBOL_GPL(davinci_pv_pool); · line 42

Quoted 10 lines

arch/arm/mach-davinci/sram.c +5 -1

 	if (len) {
 		len = min_t(unsigned, len, SRAM_SIZE);
-		davinci_pv_pool = pv_pool_create((void *)SRAM_VIRT,
+		addr = ioremap(davinci_soc_info.sram_phys, len);
+		if (!addr)
+			return -EIO;
+
+		davinci_pv_pool = pv_pool_create(addr,
 					davinci_soc_info.sram_phys, len,
 					ilog2(SRAM_GRANULARITY));

please use the gennalloc directly as discuss with Andrew and Russell my patch will present in the .40

Quoted 2 lines

With Jean-Christophe's patch getting merged, would you be dropping your PV pool patch?

I'll make that decision once JC's patch has actually been merged. JC's patch doesn't do any of the consolidation work, so that needs addressing still.

Quoted 43 lines · Russell King - ARM Linux

Quoted 41 lines · Sekhar

Hi Russell,

Quoted 22 lines · Jean-Christophe PLAGNIOL-VILLARD

Quoted 20 lines · Ben Gardiner

arch/arm/mach-davinci/sram.c +2 -0

index 219d4c5..96026df 100644
@@ -8,6 +8,7 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <asm/pv-pool.h>

@@ -26,16 +27,23 @@ EXPORT_SYMBOL_GPL(davinci_pv_pool);
  */
 static int __init sram_init(void)
 {
+	void *addr;
 	unsigned len = davinci_soc_info.sram_len;
 	int status = 0;
 

Quoted 12 lines

arch/arm/mach-davinci/sram.c EXPORT_SYMBOL_GPL(davinci_pv_pool); · line 42

Quoted 10 lines

arch/arm/mach-davinci/sram.c +5 -1

 	if (len) {
 		len = min_t(unsigned, len, SRAM_SIZE);
-		davinci_pv_pool = pv_pool_create((void *)SRAM_VIRT,
+		addr = ioremap(davinci_soc_info.sram_phys, len);
+		if (!addr)
+			return -EIO;
+
+		davinci_pv_pool = pv_pool_create(addr,
 					davinci_soc_info.sram_phys, len,
 					ilog2(SRAM_GRANULARITY));

please use the gennalloc directly as discuss with Andrew and Russell my patch will present in the .40

With Jean-Christophe's patch getting merged, would you be dropping your PV pool patch?

Quoted 3 lines

I'll make that decision once JC's patch has actually been merged. JC's patch doesn't do any of the consolidation work, so that needs addressing still.

Agreed this patch just add the support to have physicall address link to the pool, the consolidation need to be done on the top of it

Hope see the merge soon

arch/arm/mach-davinci/sram.c +1 -0

index 219d4c5..96026df 100644
@@ -8,6 +8,7 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <asm/pv-pool.h>

@@ -26,16 +27,23 @@ EXPORT_SYMBOL_GPL(davinci_pv_pool);
  */
 static int __init sram_init(void)
 {

Quoted 6 lines

? ? ? if (len) {
? ? ? ? ? ? ? len = min_t(unsigned, len, SRAM_SIZE);
- ? ? ? ? ? ? davinci_pv_pool = pv_pool_create((void *)SRAM_VIRT,
+ ? ? ? ? ? ? addr = ioremap(davinci_soc_info.sram_phys, len);
+ ? ? ? ? ? ? if (!addr)
+ ? ? ? ? ? ? ? ? ? ? return -EIO;

-ENOMEM here (for the next time you post). See Sergei's mail on error codes below. Useful stuff.

https://patchwork.kernel.org/patch/47365/