| Message ID | 1442560521-19354-3-git-send-email-qiang.zhao@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Scott Wood |
| Headers | show |
On Fri, Sep 18, 2015 at 03:15:19PM +0800, Zhao Qiang wrote: > Use genalloc to manage CPM/QE muram instead of rheap. > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com> > --- > Changes for v9: > - splitted from patch 3/5, modify cpm muram management functions. > Changes for v10: > - modify cpm muram first, then move to qe_common > - modify commit. > > arch/powerpc/platforms/Kconfig | 1 + > arch/powerpc/sysdev/cpm_common.c | 150 +++++++++++++++++++++++++++------------ > 2 files changed, 107 insertions(+), 44 deletions(-) > > diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig > index b7f9c40..01f98a2 100644 > --- a/arch/powerpc/platforms/Kconfig > +++ b/arch/powerpc/platforms/Kconfig > @@ -276,6 +276,7 @@ config QUICC_ENGINE > bool "Freescale QUICC Engine (QE) Support" > depends on FSL_SOC && PPC32 > select PPC_LIB_RHEAP > + select GENERIC_ALLOCATOR > select CRC32 > help > The QUICC Engine (QE) is a new generation of communications > diff --git a/arch/powerpc/sysdev/cpm_common.c b/arch/powerpc/sysdev/cpm_common.c > index 4f78695..453d18c 100644 > --- a/arch/powerpc/sysdev/cpm_common.c > +++ b/arch/powerpc/sysdev/cpm_common.c > @@ -17,6 +17,7 @@ > * published by the Free Software Foundation. > */ > > +#include <linux/genalloc.h> > #include <linux/init.h> > #include <linux/of_device.h> > #include <linux/spinlock.h> > @@ -27,7 +28,6 @@ > > #include <asm/udbg.h> > #include <asm/io.h> > -#include <asm/rheap.h> > #include <asm/cpm.h> > > #include <mm/mmu_decl.h> > @@ -65,14 +65,24 @@ void __init udbg_init_cpm(void) > } > #endif > > +static struct gen_pool *muram_pool; > +static struct genpool_data_align muram_pool_data; > +static struct genpool_data_fixed muram_pool_data_fixed; Why are these global? If you keep the data local to the caller (and use gen_pool_alloc_data()) then you probably don't need cpm_muram_lock. > static spinlock_t cpm_muram_lock; > -static rh_block_t cpm_boot_muram_rh_block[16]; > -static rh_info_t cpm_muram_info; > static u8 __iomem *muram_vbase; > static phys_addr_t muram_pbase; > > -/* Max address size we deal with */ > +struct muram_block { > + struct list_head head; > + unsigned long start; > + int size; > +}; > + > +static LIST_HEAD(muram_block_list); > + > +/* max address size we deal with */ > #define OF_MAX_ADDR_CELLS 4 > +#define GENPOOL_OFFSET 4096 Is 4096 bytes the maximum alignment you'll ever need? Wouldn't it be safer to use a much larger offset? > int cpm_muram_init(void) > { > @@ -86,113 +96,165 @@ int cpm_muram_init(void) > if (muram_pbase) > return 0; > > - spin_lock_init(&cpm_muram_lock); Why are you eliminating the lock init, given that you're not getting rid of the lock? > - /* initialize the info header */ > - rh_init(&cpm_muram_info, 1, > - sizeof(cpm_boot_muram_rh_block) / > - sizeof(cpm_boot_muram_rh_block[0]), > - cpm_boot_muram_rh_block); > - > np = of_find_compatible_node(NULL, NULL, "fsl,cpm-muram-data"); > if (!np) { > /* try legacy bindings */ > np = of_find_node_by_name(NULL, "data-only"); > if (!np) { > - printk(KERN_ERR "Cannot find CPM muram data node"); > + pr_err("Cannot find CPM muram data node"); > ret = -ENODEV; > goto out; > } > } > > + muram_pool = gen_pool_create(1, -1); Do we really need byte-granularity? > muram_pbase = of_translate_address(np, zero); > if (muram_pbase == (phys_addr_t)OF_BAD_ADDR) { > - printk(KERN_ERR "Cannot translate zero through CPM muram node"); > + pr_err("Cannot translate zero through CPM muram node"); > ret = -ENODEV; > - goto out; > + goto err; > } > > while (of_address_to_resource(np, i++, &r) == 0) { > if (r.end > max) > max = r.end; > + ret = gen_pool_add(muram_pool, r.start - muram_pbase + > + GENPOOL_OFFSET, resource_size(&r), -1); > + if (ret) { > + pr_err("QE: couldn't add muram to pool!\n"); > + goto err; > + } > > - rh_attach_region(&cpm_muram_info, r.start - muram_pbase, > - resource_size(&r)); > } > > muram_vbase = ioremap(muram_pbase, max - muram_pbase + 1); > if (!muram_vbase) { > - printk(KERN_ERR "Cannot map CPM muram"); > + pr_err("Cannot map QE muram"); > ret = -ENOMEM; > + goto err; > } > - > + goto out; > +err: > + gen_pool_destroy(muram_pool); > out: > of_node_put(np); > return ret; Having both "err" and "out" is confusing. Instead call it "out_pool" or similar. > } > > -/** > +/* > * cpm_muram_alloc - allocate the requested size worth of multi-user ram > * @size: number of bytes to allocate > * @align: requested alignment, in bytes > * > * This function returns an offset into the muram area. > - * Use cpm_dpram_addr() to get the virtual address of the area. > - * Use cpm_muram_free() to free the allocation. Why did you remove the comments about how to get a virtual address and free the allocation? > */ > unsigned long cpm_muram_alloc(unsigned long size, unsigned long align) > { > unsigned long start; > unsigned long flags; > + struct muram_block *entry; > > spin_lock_irqsave(&cpm_muram_lock, flags); > - cpm_muram_info.alignment = align; > - start = rh_alloc(&cpm_muram_info, size, "commproc"); > + muram_pool_data.align = align; > + gen_pool_set_algo(muram_pool, gen_pool_first_fit_align, > + &muram_pool_data); > + start = gen_pool_alloc(muram_pool, size); Why did you add gen_pool_alloc_data() if you're not going to use it? > EXPORT_SYMBOL(cpm_muram_alloc); > > -/** > - * cpm_muram_free - free a chunk of multi-user ram > - * @offset: The beginning of the chunk as returned by cpm_muram_alloc(). > +/* > + * cpm_muram_alloc_fixed - reserve a specific region of multi-user ram > + * @size: number of bytes to allocate > + * @offset: offset of allocation start address > + * > + * This function returns an offset into the muram area. > */ > -int cpm_muram_free(unsigned long offset) > +unsigned long cpm_muram_alloc_fixed(unsigned long offset, unsigned long size) Why did you reorder these functions, making the diff harder to read? > { > - int ret; > + > + unsigned long start; > unsigned long flags; > + unsigned long size_alloc = size; > + struct muram_block *entry; > + int end_bit; > + int order = muram_pool->min_alloc_order; > > spin_lock_irqsave(&cpm_muram_lock, flags); > - ret = rh_free(&cpm_muram_info, offset); > + end_bit = (offset >> order) + ((size + (1UL << order) - 1) >> order); > + if ((offset + size) > (end_bit << order)) > + size_alloc = size + (1UL << order); Why do you need to do all these calculations here? > + muram_pool_data_fixed.offset = offset + GENPOOL_OFFSET; > + gen_pool_set_algo(muram_pool, gen_pool_fixed_alloc, > + &muram_pool_data_fixed); > + start = gen_pool_alloc(muram_pool, size_alloc); > + if (!start) > + goto out2; > + start = start - GENPOOL_OFFSET; > + memset(cpm_muram_addr(start), 0, size_alloc); > + entry = kmalloc(sizeof(*entry), GFP_KERNEL); > + if (!entry) > + goto out1; > + entry->start = start; > + entry->size = size_alloc; > + list_add(&entry->head, &muram_block_list); > spin_unlock_irqrestore(&cpm_muram_lock, flags); Please factor out the common alloc code. -Scott
On Tue, Sep 22, 2015 at 06:47 AM +0800, Wood Scott-B07421 wrote: > -----Original Message----- > From: Wood Scott-B07421 > Sent: Tuesday, September 22, 2015 6:47 AM > To: Zhao Qiang-B45475 > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li > Yang-Leo-R58472; paulus@samba.org > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE muram > > On Fri, Sep 18, 2015 at 03:15:19PM +0800, Zhao Qiang wrote: > > Use genalloc to manage CPM/QE muram instead of rheap. > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com> > > --- > > Changes for v9: > > - splitted from patch 3/5, modify cpm muram management functions. > > Changes for v10: > > - modify cpm muram first, then move to qe_common > > - modify commit. > > > > arch/powerpc/platforms/Kconfig | 1 + > > arch/powerpc/sysdev/cpm_common.c | 150 > > +++++++++++++++++++++++++++------------ > > 2 files changed, 107 insertions(+), 44 deletions(-) > > > > diff --git a/arch/powerpc/platforms/Kconfig > > b/arch/powerpc/platforms/Kconfig index b7f9c40..01f98a2 100644 > > --- a/arch/powerpc/platforms/Kconfig > > +++ b/arch/powerpc/platforms/Kconfig > > @@ -276,6 +276,7 @@ config QUICC_ENGINE > > bool "Freescale QUICC Engine (QE) Support" > > depends on FSL_SOC && PPC32 > > select PPC_LIB_RHEAP > > + select GENERIC_ALLOCATOR > > select CRC32 > > help > > The QUICC Engine (QE) is a new generation of communications diff > > --git a/arch/powerpc/sysdev/cpm_common.c > > b/arch/powerpc/sysdev/cpm_common.c > > index 4f78695..453d18c 100644 > > --- a/arch/powerpc/sysdev/cpm_common.c > > +++ b/arch/powerpc/sysdev/cpm_common.c > > @@ -17,6 +17,7 @@ > > * published by the Free Software Foundation. > > */ > > > > +#include <linux/genalloc.h> > > #include <linux/init.h> > > #include <linux/of_device.h> > > #include <linux/spinlock.h> > > @@ -27,7 +28,6 @@ > > > > #include <asm/udbg.h> > > #include <asm/io.h> > > -#include <asm/rheap.h> > > #include <asm/cpm.h> > > > > #include <mm/mmu_decl.h> > > @@ -65,14 +65,24 @@ void __init udbg_init_cpm(void) } #endif > > > > +static struct gen_pool *muram_pool; > > +static struct genpool_data_align muram_pool_data; static struct > > +genpool_data_fixed muram_pool_data_fixed; > > Why are these global? If you keep the data local to the caller (and use > gen_pool_alloc_data()) then you probably don't need cpm_muram_lock. Ok > > > static spinlock_t cpm_muram_lock; > > -static rh_block_t cpm_boot_muram_rh_block[16]; -static rh_info_t > > cpm_muram_info; static u8 __iomem *muram_vbase; static phys_addr_t > > muram_pbase; > > > > -/* Max address size we deal with */ > > +struct muram_block { > > + struct list_head head; > > + unsigned long start; > > + int size; > > +}; > > + > > +static LIST_HEAD(muram_block_list); > > + > > +/* max address size we deal with */ > > #define OF_MAX_ADDR_CELLS 4 > > +#define GENPOOL_OFFSET 4096 > > Is 4096 bytes the maximum alignment you'll ever need? Wouldn't it be > safer to use a much larger offset? Yes, 4096 is the maximum alignment I ever need. > > > int cpm_muram_init(void) > > { > > @@ -86,113 +96,165 @@ int cpm_muram_init(void) > > if (muram_pbase) > > return 0; > > > > - spin_lock_init(&cpm_muram_lock); > > Why are you eliminating the lock init, given that you're not getting rid > of the lock? > > > - /* initialize the info header */ > > - rh_init(&cpm_muram_info, 1, > > - sizeof(cpm_boot_muram_rh_block) / > > - sizeof(cpm_boot_muram_rh_block[0]), > > - cpm_boot_muram_rh_block); > > - > > np = of_find_compatible_node(NULL, NULL, "fsl,cpm-muram-data"); > > if (!np) { > > /* try legacy bindings */ > > np = of_find_node_by_name(NULL, "data-only"); > > if (!np) { > > - printk(KERN_ERR "Cannot find CPM muram data node"); > > + pr_err("Cannot find CPM muram data node"); > > ret = -ENODEV; > > goto out; > > } > > } > > > > + muram_pool = gen_pool_create(1, -1); > > Do we really need byte-granularity? It is 2byte-granularity, 4byte-granularity is needed > > > muram_pbase = of_translate_address(np, zero); > > if (muram_pbase == (phys_addr_t)OF_BAD_ADDR) { > > - printk(KERN_ERR "Cannot translate zero through CPM muram > node"); > > + pr_err("Cannot translate zero through CPM muram node"); > > ret = -ENODEV; > > - goto out; > > + goto err; > > } > > > > while (of_address_to_resource(np, i++, &r) == 0) { > > if (r.end > max) > > max = r.end; > > + ret = gen_pool_add(muram_pool, r.start - muram_pbase + > > + GENPOOL_OFFSET, resource_size(&r), -1); > > + if (ret) { > > + pr_err("QE: couldn't add muram to pool!\n"); > > + goto err; > > + } > > > > - rh_attach_region(&cpm_muram_info, r.start - muram_pbase, > > - resource_size(&r)); > > } > > > > muram_vbase = ioremap(muram_pbase, max - muram_pbase + 1); > > if (!muram_vbase) { > > - printk(KERN_ERR "Cannot map CPM muram"); > > + pr_err("Cannot map QE muram"); > > ret = -ENOMEM; > > + goto err; > > } > > - > > + goto out; > > +err: > > + gen_pool_destroy(muram_pool); > > out: > > of_node_put(np); > > return ret; > > Having both "err" and "out" is confusing. Instead call it "out_pool" or > similar. Ok > > { > > - int ret; > > + > > + unsigned long start; > > unsigned long flags; > > + unsigned long size_alloc = size; > > + struct muram_block *entry; > > + int end_bit; > > + int order = muram_pool->min_alloc_order; > > > > spin_lock_irqsave(&cpm_muram_lock, flags); > > - ret = rh_free(&cpm_muram_info, offset); > > + end_bit = (offset >> order) + ((size + (1UL << order) - 1) >> > order); > > + if ((offset + size) > (end_bit << order)) > > + size_alloc = size + (1UL << order); > > Why do you need to do all these calculations here? So do it in gen_pool_fixed_alloc? gen_pool_fixed_alloc just Can see numbers of blocks. It can't do calculations in gen_pool_fixed_alloc. > > > + muram_pool_data_fixed.offset = offset + GENPOOL_OFFSET; > > + gen_pool_set_algo(muram_pool, gen_pool_fixed_alloc, > > + &muram_pool_data_fixed); > > + start = gen_pool_alloc(muram_pool, size_alloc); > > + if (!start) > > + goto out2; > > + start = start - GENPOOL_OFFSET; > > + memset(cpm_muram_addr(start), 0, size_alloc); > > + entry = kmalloc(sizeof(*entry), GFP_KERNEL); > > + if (!entry) > > + goto out1; > > + entry->start = start; > > + entry->size = size_alloc; > > + list_add(&entry->head, &muram_block_list); > > spin_unlock_irqrestore(&cpm_muram_lock, flags); > > Please factor out the common alloc code. The common code is as below, what's the function name of this common code, cpm_muram_alloc_common? gen_pool_set_algo(muram_pool, gen_pool_fixed_alloc, &muram_pool_data_fixed); start = gen_pool_alloc(muram_pool, size_alloc); if (!start) goto out2; start = start - GENPOOL_OFFSET; memset(cpm_muram_addr(start), 0, size_alloc); entry = kmalloc(sizeof(*entry), GFP_KERNEL); if (!entry) goto out1; entry->start = start; entry->size = size_alloc; list_add(&entry->head, &muram_block_list); spin_unlock_irqrestore(&cpm_muram_lock, flags); return start; out1: gen_pool_free(muram_pool, start, size_alloc); out2: spin_unlock_irqrestore(&cpm_muram_lock, flags); return (unsigned long) -ENOMEM; -Zhao
On Tue, 2015-09-22 at 03:10 -0500, Zhao Qiang-B45475 wrote: > On Tue, Sep 22, 2015 at 06:47 AM +0800, Wood Scott-B07421 wrote: > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Tuesday, September 22, 2015 6:47 AM > > To: Zhao Qiang-B45475 > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li > > Yang-Leo-R58472; paulus@samba.org > > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE muram > > > > On Fri, Sep 18, 2015 at 03:15:19PM +0800, Zhao Qiang wrote: > > > Use genalloc to manage CPM/QE muram instead of rheap. > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com> > > > --- > > > Changes for v9: > > > - splitted from patch 3/5, modify cpm muram management functions. > > > Changes for v10: > > > - modify cpm muram first, then move to qe_common > > > - modify commit. > > > > > > arch/powerpc/platforms/Kconfig | 1 + > > > arch/powerpc/sysdev/cpm_common.c | 150 > > > +++++++++++++++++++++++++++------------ > > > 2 files changed, 107 insertions(+), 44 deletions(-) > > > > > > diff --git a/arch/powerpc/platforms/Kconfig > > > b/arch/powerpc/platforms/Kconfig index b7f9c40..01f98a2 100644 > > > --- a/arch/powerpc/platforms/Kconfig > > > +++ b/arch/powerpc/platforms/Kconfig > > > @@ -276,6 +276,7 @@ config QUICC_ENGINE > > > bool "Freescale QUICC Engine (QE) Support" > > > depends on FSL_SOC && PPC32 > > > select PPC_LIB_RHEAP > > > + select GENERIC_ALLOCATOR > > > select CRC32 > > > help > > > The QUICC Engine (QE) is a new generation of communications diff > > > --git a/arch/powerpc/sysdev/cpm_common.c > > > b/arch/powerpc/sysdev/cpm_common.c > > > index 4f78695..453d18c 100644 > > > --- a/arch/powerpc/sysdev/cpm_common.c > > > +++ b/arch/powerpc/sysdev/cpm_common.c > > > @@ -17,6 +17,7 @@ > > > * published by the Free Software Foundation. > > > */ > > > > > > +#include <linux/genalloc.h> > > > #include <linux/init.h> > > > #include <linux/of_device.h> > > > #include <linux/spinlock.h> > > > @@ -27,7 +28,6 @@ > > > > > > #include <asm/udbg.h> > > > #include <asm/io.h> > > > -#include <asm/rheap.h> > > > #include <asm/cpm.h> > > > > > > #include <mm/mmu_decl.h> > > > @@ -65,14 +65,24 @@ void __init udbg_init_cpm(void) } #endif > > > > > > +static struct gen_pool *muram_pool; > > > +static struct genpool_data_align muram_pool_data; static struct > > > +genpool_data_fixed muram_pool_data_fixed; > > > > Why are these global? If you keep the data local to the caller (and use > > gen_pool_alloc_data()) then you probably don't need cpm_muram_lock. > > Ok > > > > > > static spinlock_t cpm_muram_lock; > > > -static rh_block_t cpm_boot_muram_rh_block[16]; -static rh_info_t > > > cpm_muram_info; static u8 __iomem *muram_vbase; static phys_addr_t > > > muram_pbase; > > > > > > -/* Max address size we deal with */ > > > +struct muram_block { > > > + struct list_head head; > > > + unsigned long start; > > > + int size; > > > +}; > > > + > > > +static LIST_HEAD(muram_block_list); > > > + > > > +/* max address size we deal with */ > > > #define OF_MAX_ADDR_CELLS 4 > > > +#define GENPOOL_OFFSET 4096 > > > > Is 4096 bytes the maximum alignment you'll ever need? Wouldn't it be > > safer to use a much larger offset? > > Yes, 4096 is the maximum alignment I ever need. Still, I'd be more comfortable with a larger offset. Better yet would be using gen_pool_add_virt() and using virtual addresses for the allocations, similar to http://patchwork.ozlabs.org/patch/504000/ > > > int cpm_muram_init(void) > > > { > > > @@ -86,113 +96,165 @@ int cpm_muram_init(void) > > > if (muram_pbase) > > > return 0; > > > > > > - spin_lock_init(&cpm_muram_lock); > > > > Why are you eliminating the lock init, given that you're not getting rid > > of the lock? > > > > > - /* initialize the info header */ > > > - rh_init(&cpm_muram_info, 1, > > > - sizeof(cpm_boot_muram_rh_block) / > > > - sizeof(cpm_boot_muram_rh_block[0]), > > > - cpm_boot_muram_rh_block); > > > - > > > np = of_find_compatible_node(NULL, NULL, "fsl,cpm-muram-data"); > > > if (!np) { > > > /* try legacy bindings */ > > > np = of_find_node_by_name(NULL, "data-only"); > > > if (!np) { > > > - printk(KERN_ERR "Cannot find CPM muram data node"); > > > + pr_err("Cannot find CPM muram data node"); > > > ret = -ENODEV; > > > goto out; > > > } > > > } > > > > > > + muram_pool = gen_pool_create(1, -1); > > > > Do we really need byte-granularity? > > It is 2byte-granularity, 4byte-granularity is needed > > > > > > muram_pbase = of_translate_address(np, zero); > > > if (muram_pbase == (phys_addr_t)OF_BAD_ADDR) { > > > - printk(KERN_ERR "Cannot translate zero through CPM muram > > node"); > > > + pr_err("Cannot translate zero through CPM muram node"); > > > ret = -ENODEV; > > > - goto out; > > > + goto err; > > > } > > > > > > while (of_address_to_resource(np, i++, &r) == 0) { > > > if (r.end > max) > > > max = r.end; > > > + ret = gen_pool_add(muram_pool, r.start - muram_pbase + > > > + GENPOOL_OFFSET, resource_size(&r), -1); > > > + if (ret) { > > > + pr_err("QE: couldn't add muram to pool!\n"); > > > + goto err; > > > + } > > > > > > - rh_attach_region(&cpm_muram_info, r.start - muram_pbase, > > > - resource_size(&r)); > > > } > > > > > > muram_vbase = ioremap(muram_pbase, max - muram_pbase + 1); > > > if (!muram_vbase) { > > > - printk(KERN_ERR "Cannot map CPM muram"); > > > + pr_err("Cannot map QE muram"); > > > ret = -ENOMEM; > > > + goto err; > > > } > > > - > > > + goto out; > > > +err: > > > + gen_pool_destroy(muram_pool); > > > out: > > > of_node_put(np); > > > return ret; > > > > Having both "err" and "out" is confusing. Instead call it "out_pool" or > > similar. > > Ok > > > > { > > > - int ret; > > > + > > > + unsigned long start; > > > unsigned long flags; > > > + unsigned long size_alloc = size; > > > + struct muram_block *entry; > > > + int end_bit; > > > + int order = muram_pool->min_alloc_order; > > > > > > spin_lock_irqsave(&cpm_muram_lock, flags); > > > - ret = rh_free(&cpm_muram_info, offset); > > > + end_bit = (offset >> order) + ((size + (1UL << order) - 1) >> > > order); > > > + if ((offset + size) > (end_bit << order)) > > > + size_alloc = size + (1UL << order); > > > > Why do you need to do all these calculations here? > > So do it in gen_pool_fixed_alloc? Could you explain why they're needed at all? > gen_pool_fixed_alloc just > Can see numbers of blocks. It can't do calculations in gen_pool_fixed_alloc. Are you saying that this: struct genpool_data_fixed { unsigned long offset; /* The offset of the specific region */ }; refers to blocks and not bytes? And you didn't even mention that in the comment? It should be bytes. Actually, it should be an address, not an offset. It should be the same value that you receive back from the allocator. And I'm not sure how this is going to work with genalloc chunks that don't start at zero. I think it really does need to be a separate toplevel function, and not just an allocation algorithm (or else the allocation algorithm is going to need more context on what chunk it's working on). Speaking of chunks and the allocation function, I wonder what happens if you hit "if (start_bit >= end_bit) continue;" and then enter the loop with a new chunk and start_bit != 0... -Scott
On Wen, Sep 23, 2015 at 8:19 AM +0800, Wood Scott-B07421 wrote: > -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, September 23, 2015 8:19 AM > To: Zhao Qiang-B45475 > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li > Yang-Leo-R58472; paulus@samba.org > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE muram > > On Tue, 2015-09-22 at 03:10 -0500, Zhao Qiang-B45475 wrote: > > On Tue, Sep 22, 2015 at 06:47 AM +0800, Wood Scott-B07421 wrote: > > > -----Original Message----- > > > From: Wood Scott-B07421 > > > Sent: Tuesday, September 22, 2015 6:47 AM > > > To: Zhao Qiang-B45475 > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; > > > Li Yang-Leo-R58472; paulus@samba.org > > > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE > > > muram > > > > > > On Fri, Sep 18, 2015 at 03:15:19PM +0800, Zhao Qiang wrote: > > > > Use genalloc to manage CPM/QE muram instead of rheap. > > > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com> > > > > --- > > > > Changes for v9: > > > > - splitted from patch 3/5, modify cpm muram management functions. > > > > Changes for v10: > > > > - modify cpm muram first, then move to qe_common > > > > - modify commit. > > > > > > > > arch/powerpc/platforms/Kconfig | 1 + > > > > arch/powerpc/sysdev/cpm_common.c | 150 > > > > +++++++++++++++++++++++++++------------ > > > > 2 files changed, 107 insertions(+), 44 deletions(-) > > > > > > > > diff --git a/arch/powerpc/platforms/Kconfig > > > > b/arch/powerpc/platforms/Kconfig index b7f9c40..01f98a2 100644 > > > > --- a/arch/powerpc/platforms/Kconfig > > > > +++ b/arch/powerpc/platforms/Kconfig > > > > @@ -276,6 +276,7 @@ config QUICC_ENGINE > > > > bool "Freescale QUICC Engine (QE) Support" > > > > depends on FSL_SOC && PPC32 > > > > select PPC_LIB_RHEAP > > > > + select GENERIC_ALLOCATOR > > > > select CRC32 > > > > help > > > > The QUICC Engine (QE) is a new generation of communications > > > > diff --git a/arch/powerpc/sysdev/cpm_common.c > > > > b/arch/powerpc/sysdev/cpm_common.c > > > > index 4f78695..453d18c 100644 > > > > --- a/arch/powerpc/sysdev/cpm_common.c > > > > +++ b/arch/powerpc/sysdev/cpm_common.c > > > > @@ -17,6 +17,7 @@ > > > > * published by the Free Software Foundation. > > > > */ > > > > > > > > +#include <linux/genalloc.h> > > > > #include <linux/init.h> > > > > #include <linux/of_device.h> > > > > #include <linux/spinlock.h> > > > > @@ -27,7 +28,6 @@ > > > > > > > > #include <asm/udbg.h> > > > > #include <asm/io.h> > > > > -#include <asm/rheap.h> > > > > #include <asm/cpm.h> > > > > > > > > #include <mm/mmu_decl.h> > > > > @@ -65,14 +65,24 @@ void __init udbg_init_cpm(void) } #endif > > > > > > > > +static struct gen_pool *muram_pool; static struct > > > > +genpool_data_align muram_pool_data; static struct > > > > +genpool_data_fixed muram_pool_data_fixed; > > > > > > Why are these global? If you keep the data local to the caller (and > > > use > > > gen_pool_alloc_data()) then you probably don't need cpm_muram_lock. > > > > Ok > > > > > > > > > static spinlock_t cpm_muram_lock; -static rh_block_t > > > > cpm_boot_muram_rh_block[16]; -static rh_info_t cpm_muram_info; > > > > static u8 __iomem *muram_vbase; static phys_addr_t muram_pbase; > > > > > > > > -/* Max address size we deal with */ > > > > +struct muram_block { > > > > + struct list_head head; > > > > + unsigned long start; > > > > + int size; > > > > +}; > > > > + > > > > +static LIST_HEAD(muram_block_list); > > > > + > > > > +/* max address size we deal with */ > > > > #define OF_MAX_ADDR_CELLS 4 > > > > +#define GENPOOL_OFFSET 4096 > > > > > > Is 4096 bytes the maximum alignment you'll ever need? Wouldn't it > > > be safer to use a much larger offset? > > > > Yes, 4096 is the maximum alignment I ever need. > > Still, I'd be more comfortable with a larger offset. Larger offset is good. > > Better yet would be using gen_pool_add_virt() and using virtual addresses > for the allocations, similar to http://patchwork.ozlabs.org/patch/504000/ > > > > > int cpm_muram_init(void) > > > > { > > > > @@ -86,113 +96,165 @@ int cpm_muram_init(void) > > > > if (muram_pbase) > > > > return 0; > > > > > > > > - spin_lock_init(&cpm_muram_lock); > > > > > > Why are you eliminating the lock init, given that you're not getting > > > rid of the lock? > > > > > > > - /* initialize the info header */ > > > > - rh_init(&cpm_muram_info, 1, > > > > - sizeof(cpm_boot_muram_rh_block) / > > > > - sizeof(cpm_boot_muram_rh_block[0]), > > > > - cpm_boot_muram_rh_block); > > > > - > > > > np = of_find_compatible_node(NULL, NULL, "fsl,cpm-muram-data"); > > > > if (!np) { > > > > /* try legacy bindings */ > > > > np = of_find_node_by_name(NULL, "data-only"); > > > > if (!np) { > > > > - printk(KERN_ERR "Cannot find CPM muram data > node"); > > > > + pr_err("Cannot find CPM muram data node"); > > > > ret = -ENODEV; > > > > goto out; > > > > } > > > > } > > > > > > > > + muram_pool = gen_pool_create(1, -1); > > > > > > Do we really need byte-granularity? > > > > It is 2byte-granularity, 4byte-granularity is needed > > > > > > > > > muram_pbase = of_translate_address(np, zero); > > > > if (muram_pbase == (phys_addr_t)OF_BAD_ADDR) { > > > > - printk(KERN_ERR "Cannot translate zero through CPM muram > > > node"); > > > > + pr_err("Cannot translate zero through CPM muram node"); > > > > ret = -ENODEV; > > > > - goto out; > > > > + goto err; > > > > } > > > > > > > > while (of_address_to_resource(np, i++, &r) == 0) { > > > > if (r.end > max) > > > > max = r.end; > > > > + ret = gen_pool_add(muram_pool, r.start - muram_pbase + > > > > + GENPOOL_OFFSET, resource_size(&r), -1); > > > > + if (ret) { > > > > + pr_err("QE: couldn't add muram to > pool!\n"); > > > > + goto err; > > > > + } > > > > > > > > - rh_attach_region(&cpm_muram_info, r.start - muram_pbase, > > > > - resource_size(&r)); > > > > } > > > > > > > > muram_vbase = ioremap(muram_pbase, max - muram_pbase + 1); > > > > if (!muram_vbase) { > > > > - printk(KERN_ERR "Cannot map CPM muram"); > > > > + pr_err("Cannot map QE muram"); > > > > ret = -ENOMEM; > > > > + goto err; > > > > } > > > > - > > > > + goto out; > > > > +err: > > > > + gen_pool_destroy(muram_pool); > > > > out: > > > > of_node_put(np); > > > > return ret; > > > > > > Having both "err" and "out" is confusing. Instead call it > > > "out_pool" or similar. > > > > Ok > > > > > > { > > > > - int ret; > > > > + > > > > + unsigned long start; > > > > unsigned long flags; > > > > + unsigned long size_alloc = size; struct muram_block *entry; int > > > > + end_bit; int order = muram_pool->min_alloc_order; > > > > > > > > spin_lock_irqsave(&cpm_muram_lock, flags); > > > > - ret = rh_free(&cpm_muram_info, offset); > > > > + end_bit = (offset >> order) + ((size + (1UL << order) - 1) >> > > > order); > > > > + if ((offset + size) > (end_bit << order)) > > > > + size_alloc = size + (1UL << order); > > > > > > Why do you need to do all these calculations here? > > > > So do it in gen_pool_fixed_alloc? > > Could you explain why they're needed at all? Why it does the calculations? If the min block of gen_pool is 8 bytes, and I want to allocate a Region with offset=7, size=8bytes, I actually need block 0 and block 1, And the allocation will give me block 0. > > > gen_pool_fixed_alloc just > > Can see numbers of blocks. It can't do calculations in > gen_pool_fixed_alloc. > > Are you saying that this: > > struct genpool_data_fixed { > unsigned long offset; /* The offset of the specific > region */ > }; > > refers to blocks and not bytes? And you didn't even mention that in the > comment? Offset is bytes. > > It should be bytes. Actually, it should be an address, not an offset. > It should be the same value that you receive back from the allocator. > And I'm not sure how this is going to work with genalloc chunks that > don't start at zero. I think it really does need to be a separate > toplevel function, and not just an allocation algorithm (or else the > allocation algorithm is going to need more context on what chunk it's > working on). > > Speaking of chunks and the allocation function, I wonder what happens if > you hit "if (start_bit >= end_bit) continue;" and then enter the loop > with a new chunk and start_bit != 0... > > -Scott
On Tue, 2015-09-22 at 21:20 -0500, Zhao Qiang-B45475 wrote: > On Wen, Sep 23, 2015 at 8:19 AM +0800, Wood Scott-B07421 wrote: > > > > > > { > > > > > - int ret; > > > > > + > > > > > + unsigned long start; > > > > > unsigned long flags; > > > > > + unsigned long size_alloc = size; struct muram_block *entry; int > > > > > + end_bit; int order = muram_pool->min_alloc_order; > > > > > > > > > > spin_lock_irqsave(&cpm_muram_lock, flags); > > > > > - ret = rh_free(&cpm_muram_info, offset); > > > > > + end_bit = (offset >> order) + ((size + (1UL << order) - 1) >> > > > > order); > > > > > + if ((offset + size) > (end_bit << order)) > > > > > + size_alloc = size + (1UL << order); > > > > > > > > Why do you need to do all these calculations here? > > > > > > So do it in gen_pool_fixed_alloc? > > > > Could you explain why they're needed at all? > > Why it does the calculations? > If the min block of gen_pool is 8 bytes, and I want to allocate a > Region with offset=7, size=8bytes, I actually need block 0 and block 1, > And the allocation will give me block 0. How can you have offset 7 if the minimum order is 2 bytes? -Scott
On Wen, Sep 23, 2015 at 12:03 AM +0800, Wood Scott-B07421 wrote: > -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, September 23, 2015 12:03 PM > To: Zhao Qiang-B45475 > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li > Yang-Leo-R58472; paulus@samba.org > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE muram > > On Tue, 2015-09-22 at 21:20 -0500, Zhao Qiang-B45475 wrote: > > On Wen, Sep 23, 2015 at 8:19 AM +0800, Wood Scott-B07421 wrote: > > > > > > > > { > > > > > > - int ret; > > > > > > + > > > > > > + unsigned long start; > > > > > > unsigned long flags; > > > > > > + unsigned long size_alloc = size; struct muram_block *entry; > > > > > > + int end_bit; int order = muram_pool->min_alloc_order; > > > > > > > > > > > > spin_lock_irqsave(&cpm_muram_lock, flags); > > > > > > - ret = rh_free(&cpm_muram_info, offset); > > > > > > + end_bit = (offset >> order) + ((size + (1UL << order) - 1) > > > > > > + >> > > > > > order); > > > > > > + if ((offset + size) > (end_bit << order)) > > > > > > + size_alloc = size + (1UL << order); > > > > > > > > > > Why do you need to do all these calculations here? > > > > > > > > So do it in gen_pool_fixed_alloc? > > > > > > Could you explain why they're needed at all? > > > > Why it does the calculations? > > If the min block of gen_pool is 8 bytes, and I want to allocate a > > Region with offset=7, size=8bytes, I actually need block 0 and block > > 1, And the allocation will give me block 0. > > How can you have offset 7 if the minimum order is 2 bytes? Offset has no relationship with minimum order, it is not decided by minimum order. I want to allocate a specific region with offset=7, then algo to calculate the block bit. And I just take it for example, it is not I really need to region offset=7. So, now minimum order is 2 bytes. If offset=7, size=4bytes needed, it actually allocate 6-12 to me. so I need to check if it is necessary to plus a block(2bytes) to size before allocation. -Zhao
On Wed, 2015-09-23 at 00:28 -0500, Zhao Qiang-B45475 wrote: > On Wen, Sep 23, 2015 at 12:03 AM +0800, Wood Scott-B07421 wrote: > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Wednesday, September 23, 2015 12:03 PM > > To: Zhao Qiang-B45475 > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li > > Yang-Leo-R58472; paulus@samba.org > > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE muram > > > > On Tue, 2015-09-22 at 21:20 -0500, Zhao Qiang-B45475 wrote: > > > On Wen, Sep 23, 2015 at 8:19 AM +0800, Wood Scott-B07421 wrote: > > > > > > > > > > { > > > > > > > - int ret; > > > > > > > + > > > > > > > + unsigned long start; > > > > > > > unsigned long flags; > > > > > > > + unsigned long size_alloc = size; struct muram_block *entry; > > > > > > > + int end_bit; int order = muram_pool->min_alloc_order; > > > > > > > > > > > > > > spin_lock_irqsave(&cpm_muram_lock, flags); > > > > > > > - ret = rh_free(&cpm_muram_info, offset); > > > > > > > + end_bit = (offset >> order) + ((size + (1UL << order) - 1) > > > > > > > + >> > > > > > > order); > > > > > > > + if ((offset + size) > (end_bit << order)) > > > > > > > + size_alloc = size + (1UL << order); > > > > > > > > > > > > Why do you need to do all these calculations here? > > > > > > > > > > So do it in gen_pool_fixed_alloc? > > > > > > > > Could you explain why they're needed at all? > > > > > > Why it does the calculations? > > > If the min block of gen_pool is 8 bytes, and I want to allocate a > > > Region with offset=7, size=8bytes, I actually need block 0 and block > > > 1, And the allocation will give me block 0. > > > > How can you have offset 7 if the minimum order is 2 bytes? > > Offset has no relationship with minimum order, it is not decided by minimum > order. All allocations begin and end on a multiple of the minimum order. > I want to allocate a specific region with offset=7, then algo to calculate > the block bit. > And I just take it for example, it is not I really need to region offset=7. Do you really need any fixed allocations that begin on an odd address? > So, now minimum order is 2 bytes. If offset=7, size=4bytes needed, it > actually allocate 6-12 to me. Why 6-12 and not 6-10? -Scott
On Fri, Sep 25, 2015 at 7:30 AM +0800, Wood Scott-B07421 wrote: > -----Original Message----- > From: Wood Scott-B07421 > Sent: Friday, September 25, 2015 7:30 AM > To: Zhao Qiang-B45475 > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li > Yang-Leo-R58472; paulus@samba.org > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE muram > > On Wed, 2015-09-23 at 00:28 -0500, Zhao Qiang-B45475 wrote: > > On Wen, Sep 23, 2015 at 12:03 AM +0800, Wood Scott-B07421 wrote: > > > > > -----Original Message----- > > > From: Wood Scott-B07421 > > > Sent: Wednesday, September 23, 2015 12:03 PM > > > To: Zhao Qiang-B45475 > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; > > > Li Yang-Leo-R58472; paulus@samba.org > > > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE > > > muram > > > > > > On Tue, 2015-09-22 at 21:20 -0500, Zhao Qiang-B45475 wrote: > > > > On Wen, Sep 23, 2015 at 8:19 AM +0800, Wood Scott-B07421 wrote: > > > > > > > > > > > > { > > > > > > > > - int ret; > > > > > > > > + > > > > > > > > + unsigned long start; > > > > > > > > unsigned long flags; > > > > > > > > + unsigned long size_alloc = size; struct muram_block > > > > > > > > + *entry; int end_bit; int order = > > > > > > > > + muram_pool->min_alloc_order; > > > > > > > > > > > > > > > > spin_lock_irqsave(&cpm_muram_lock, flags); > > > > > > > > - ret = rh_free(&cpm_muram_info, offset); > > > > > > > > + end_bit = (offset >> order) + ((size + (1UL << order) - > > > > > > > > + 1) > > > > > > > > + >> > > > > > > > order); > > > > > > > > + if ((offset + size) > (end_bit << order)) > > > > > > > > + size_alloc = size + (1UL << order); > > > > > > > > > > > > > > Why do you need to do all these calculations here? > > > > > > > > > > > > So do it in gen_pool_fixed_alloc? > > > > > > > > > > Could you explain why they're needed at all? > > > > > > > > Why it does the calculations? > > > > If the min block of gen_pool is 8 bytes, and I want to allocate a > > > > Region with offset=7, size=8bytes, I actually need block 0 and > > > > block 1, And the allocation will give me block 0. > > > > > > How can you have offset 7 if the minimum order is 2 bytes? > > > > Offset has no relationship with minimum order, it is not decided by > > minimum order. > > All allocations begin and end on a multiple of the minimum order. So it is the problem. CPM require to allocate a specific region, who can ensure that the specific is just the begin of minimum order. So the algo will find the first block covering the specific region's Start address, then because my specific region's start address is not equal To the address returned by algo, the end address is not equal to my specific region's end address, so the calculation is to keep the end address larger than specific region's end address. > > > I want to allocate a specific region with offset=7, then algo to > > calculate the block bit. > > And I just take it for example, it is not I really need to region > offset=7. > > Do you really need any fixed allocations that begin on an odd address? Maybe I don’t need an odd address, but the calculation is needed in case. > > > So, now minimum order is 2 bytes. If offset=7, size=4bytes needed, it > > actually allocate 6-12 to me. > > Why 6-12 and not 6-10? It is just a example -Zhao
On Thu, 2015-09-24 at 21:50 -0500, Zhao Qiang-B45475 wrote: > On Fri, Sep 25, 2015 at 7:30 AM +0800, Wood Scott-B07421 wrote: > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Friday, September 25, 2015 7:30 AM > > To: Zhao Qiang-B45475 > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li > > Yang-Leo-R58472; paulus@samba.org > > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE muram > > > > On Wed, 2015-09-23 at 00:28 -0500, Zhao Qiang-B45475 wrote: > > > On Wen, Sep 23, 2015 at 12:03 AM +0800, Wood Scott-B07421 wrote: > > > > > > > -----Original Message----- > > > > From: Wood Scott-B07421 > > > > Sent: Wednesday, September 23, 2015 12:03 PM > > > > To: Zhao Qiang-B45475 > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; > > > > Li Yang-Leo-R58472; paulus@samba.org > > > > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE > > > > muram > > > > > > > > On Tue, 2015-09-22 at 21:20 -0500, Zhao Qiang-B45475 wrote: > > > > > On Wen, Sep 23, 2015 at 8:19 AM +0800, Wood Scott-B07421 wrote: > > > > > > > > > > > > > > { > > > > > > > > > - int ret; > > > > > > > > > + > > > > > > > > > + unsigned long start; > > > > > > > > > unsigned long flags; > > > > > > > > > + unsigned long size_alloc = size; struct muram_block > > > > > > > > > + *entry; int end_bit; int order = > > > > > > > > > + muram_pool->min_alloc_order; > > > > > > > > > > > > > > > > > > spin_lock_irqsave(&cpm_muram_lock, flags); > > > > > > > > > - ret = rh_free(&cpm_muram_info, offset); > > > > > > > > > + end_bit = (offset >> order) + ((size + (1UL << order) - > > > > > > > > > + 1) > > > > > > > > > + >> > > > > > > > > order); > > > > > > > > > + if ((offset + size) > (end_bit << order)) > > > > > > > > > + size_alloc = size + (1UL << order); > > > > > > > > > > > > > > > > Why do you need to do all these calculations here? > > > > > > > > > > > > > > So do it in gen_pool_fixed_alloc? > > > > > > > > > > > > Could you explain why they're needed at all? > > > > > > > > > > Why it does the calculations? > > > > > If the min block of gen_pool is 8 bytes, and I want to allocate a > > > > > Region with offset=7, size=8bytes, I actually need block 0 and > > > > > block 1, And the allocation will give me block 0. > > > > > > > > How can you have offset 7 if the minimum order is 2 bytes? > > > > > > Offset has no relationship with minimum order, it is not decided by > > > minimum order. > > > > All allocations begin and end on a multiple of the minimum order. > > So it is the problem. CPM require to allocate a specific region, > who can ensure that the specific is just the begin of minimum order. Do you have any reason to believe that there is any caller of this function with an odd address? If so, set the minimum order to zero. If not, what is the problem? > So the algo will find the first block covering the specific region's > Start address, then because my specific region's start address is not equal > To the address returned by algo, the end address is not equal to my > specific region's end address, so the calculation is to keep the end > address larger than specific region's end address. > > > > > > I want to allocate a specific region with offset=7, then algo to > > > calculate the block bit. > > > And I just take it for example, it is not I really need to region > > offset=7. > > > > Do you really need any fixed allocations that begin on an odd address? > > Maybe I don’t need an odd address, but the calculation is needed in case. No. "In case" the caller does something that is not allowed, the allocation should fail. -Scott
On Fri, Sep 25, 2015 at 1:08 PM +0800, Wood Scott-B07421 wrote: > -----Original Message----- > From: Wood Scott-B07421 > Sent: Friday, September 25, 2015 1:08 PM > To: Zhao Qiang-B45475 > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li > Yang-Leo-R58472; paulus@samba.org > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE muram > > On Thu, 2015-09-24 at 21:50 -0500, Zhao Qiang-B45475 wrote: > > On Fri, Sep 25, 2015 at 7:30 AM +0800, Wood Scott-B07421 wrote: > > > -----Original Message----- > > > From: Wood Scott-B07421 > > > Sent: Friday, September 25, 2015 7:30 AM > > > To: Zhao Qiang-B45475 > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; > > > Li Yang-Leo-R58472; paulus@samba.org > > > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage CPM/QE > > > muram > > > > > > On Wed, 2015-09-23 at 00:28 -0500, Zhao Qiang-B45475 wrote: > > > > On Wen, Sep 23, 2015 at 12:03 AM +0800, Wood Scott-B07421 wrote: > > > > > > > > > -----Original Message----- > > > > > From: Wood Scott-B07421 > > > > > Sent: Wednesday, September 23, 2015 12:03 PM > > > > > To: Zhao Qiang-B45475 > > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; > > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org > > > > > Subject: Re: [PATCH v10 3/5] CPM/QE: use genalloc to manage > > > > > CPM/QE muram > > > > > > > > > > On Tue, 2015-09-22 at 21:20 -0500, Zhao Qiang-B45475 wrote: > > > > > > On Wen, Sep 23, 2015 at 8:19 AM +0800, Wood Scott-B07421 wrote: > > > > > > > > > > > > > > > > { > > > > > > > > > > - int ret; > > > > > > > > > > + > > > > > > > > > > + unsigned long start; > > > > > > > > > > unsigned long flags; > > > > > > > > > > + unsigned long size_alloc = size; struct muram_block > > > > > > > > > > + *entry; int end_bit; int order = > > > > > > > > > > + muram_pool->min_alloc_order; > > > > > > > > > > > > > > > > > > > > spin_lock_irqsave(&cpm_muram_lock, flags); > > > > > > > > > > - ret = rh_free(&cpm_muram_info, offset); > > > > > > > > > > + end_bit = (offset >> order) + ((size + (1UL << > > > > > > > > > > + order) - > > > > > > > > > > + 1) > > > > > > > > > > + >> > > > > > > > > > order); > > > > > > > > > > + if ((offset + size) > (end_bit << order)) > > > > > > > > > > + size_alloc = size + (1UL << order); > > > > > > > > > > > > > > > > > > Why do you need to do all these calculations here? > > > > > > > > > > > > > > > > So do it in gen_pool_fixed_alloc? > > > > > > > > > > > > > > Could you explain why they're needed at all? > > > > > > > > > > > > Why it does the calculations? > > > > > > If the min block of gen_pool is 8 bytes, and I want to > > > > > > allocate a Region with offset=7, size=8bytes, I actually need > > > > > > block 0 and block 1, And the allocation will give me block 0. > > > > > > > > > > How can you have offset 7 if the minimum order is 2 bytes? > > > > > > > > Offset has no relationship with minimum order, it is not decided > > > > by minimum order. > > > > > > All allocations begin and end on a multiple of the minimum order. > > > > So it is the problem. CPM require to allocate a specific region, who > > can ensure that the specific is just the begin of minimum order. > > Do you have any reason to believe that there is any caller of this > function with an odd address? > > If so, set the minimum order to zero. If not, what is the problem? Setting minimum order to zero is ok. -Zhao
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig index b7f9c40..01f98a2 100644 --- a/arch/powerpc/platforms/Kconfig +++ b/arch/powerpc/platforms/Kconfig @@ -276,6 +276,7 @@ config QUICC_ENGINE bool "Freescale QUICC Engine (QE) Support" depends on FSL_SOC && PPC32 select PPC_LIB_RHEAP + select GENERIC_ALLOCATOR select CRC32 help The QUICC Engine (QE) is a new generation of communications diff --git a/arch/powerpc/sysdev/cpm_common.c b/arch/powerpc/sysdev/cpm_common.c index 4f78695..453d18c 100644 --- a/arch/powerpc/sysdev/cpm_common.c +++ b/arch/powerpc/sysdev/cpm_common.c @@ -17,6 +17,7 @@ * published by the Free Software Foundation. */ +#include <linux/genalloc.h> #include <linux/init.h> #include <linux/of_device.h> #include <linux/spinlock.h> @@ -27,7 +28,6 @@ #include <asm/udbg.h> #include <asm/io.h> -#include <asm/rheap.h> #include <asm/cpm.h> #include <mm/mmu_decl.h> @@ -65,14 +65,24 @@ void __init udbg_init_cpm(void) } #endif +static struct gen_pool *muram_pool; +static struct genpool_data_align muram_pool_data; +static struct genpool_data_fixed muram_pool_data_fixed; static spinlock_t cpm_muram_lock; -static rh_block_t cpm_boot_muram_rh_block[16]; -static rh_info_t cpm_muram_info; static u8 __iomem *muram_vbase; static phys_addr_t muram_pbase; -/* Max address size we deal with */ +struct muram_block { + struct list_head head; + unsigned long start; + int size; +}; + +static LIST_HEAD(muram_block_list); + +/* max address size we deal with */ #define OF_MAX_ADDR_CELLS 4 +#define GENPOOL_OFFSET 4096 int cpm_muram_init(void) { @@ -86,113 +96,165 @@ int cpm_muram_init(void) if (muram_pbase) return 0; - spin_lock_init(&cpm_muram_lock); - /* initialize the info header */ - rh_init(&cpm_muram_info, 1, - sizeof(cpm_boot_muram_rh_block) / - sizeof(cpm_boot_muram_rh_block[0]), - cpm_boot_muram_rh_block); - np = of_find_compatible_node(NULL, NULL, "fsl,cpm-muram-data"); if (!np) { /* try legacy bindings */ np = of_find_node_by_name(NULL, "data-only"); if (!np) { - printk(KERN_ERR "Cannot find CPM muram data node"); + pr_err("Cannot find CPM muram data node"); ret = -ENODEV; goto out; } } + muram_pool = gen_pool_create(1, -1); muram_pbase = of_translate_address(np, zero); if (muram_pbase == (phys_addr_t)OF_BAD_ADDR) { - printk(KERN_ERR "Cannot translate zero through CPM muram node"); + pr_err("Cannot translate zero through CPM muram node"); ret = -ENODEV; - goto out; + goto err; } while (of_address_to_resource(np, i++, &r) == 0) { if (r.end > max) max = r.end; + ret = gen_pool_add(muram_pool, r.start - muram_pbase + + GENPOOL_OFFSET, resource_size(&r), -1); + if (ret) { + pr_err("QE: couldn't add muram to pool!\n"); + goto err; + } - rh_attach_region(&cpm_muram_info, r.start - muram_pbase, - resource_size(&r)); } muram_vbase = ioremap(muram_pbase, max - muram_pbase + 1); if (!muram_vbase) { - printk(KERN_ERR "Cannot map CPM muram"); + pr_err("Cannot map QE muram"); ret = -ENOMEM; + goto err; } - + goto out; +err: + gen_pool_destroy(muram_pool); out: of_node_put(np); return ret; } -/** +/* * cpm_muram_alloc - allocate the requested size worth of multi-user ram * @size: number of bytes to allocate * @align: requested alignment, in bytes * * This function returns an offset into the muram area. - * Use cpm_dpram_addr() to get the virtual address of the area. - * Use cpm_muram_free() to free the allocation. */ unsigned long cpm_muram_alloc(unsigned long size, unsigned long align) { unsigned long start; unsigned long flags; + struct muram_block *entry; spin_lock_irqsave(&cpm_muram_lock, flags); - cpm_muram_info.alignment = align; - start = rh_alloc(&cpm_muram_info, size, "commproc"); + muram_pool_data.align = align; + gen_pool_set_algo(muram_pool, gen_pool_first_fit_align, + &muram_pool_data); + start = gen_pool_alloc(muram_pool, size); + if (!start) + goto out2; + start = start - GENPOOL_OFFSET; memset(cpm_muram_addr(start), 0, size); + entry = kmalloc(sizeof(*entry), GFP_KERNEL); + if (!entry) + goto out1; + entry->start = start; + entry->size = size; + list_add(&entry->head, &muram_block_list); spin_unlock_irqrestore(&cpm_muram_lock, flags); return start; +out1: + gen_pool_free(muram_pool, start, size); +out2: + spin_unlock_irqrestore(&cpm_muram_lock, flags); + return (unsigned long) -ENOMEM; } EXPORT_SYMBOL(cpm_muram_alloc); -/** - * cpm_muram_free - free a chunk of multi-user ram - * @offset: The beginning of the chunk as returned by cpm_muram_alloc(). +/* + * cpm_muram_alloc_fixed - reserve a specific region of multi-user ram + * @size: number of bytes to allocate + * @offset: offset of allocation start address + * + * This function returns an offset into the muram area. */ -int cpm_muram_free(unsigned long offset) +unsigned long cpm_muram_alloc_fixed(unsigned long offset, unsigned long size) { - int ret; + + unsigned long start; unsigned long flags; + unsigned long size_alloc = size; + struct muram_block *entry; + int end_bit; + int order = muram_pool->min_alloc_order; spin_lock_irqsave(&cpm_muram_lock, flags); - ret = rh_free(&cpm_muram_info, offset); + end_bit = (offset >> order) + ((size + (1UL << order) - 1) >> order); + if ((offset + size) > (end_bit << order)) + size_alloc = size + (1UL << order); + + muram_pool_data_fixed.offset = offset + GENPOOL_OFFSET; + gen_pool_set_algo(muram_pool, gen_pool_fixed_alloc, + &muram_pool_data_fixed); + start = gen_pool_alloc(muram_pool, size_alloc); + if (!start) + goto out2; + start = start - GENPOOL_OFFSET; + memset(cpm_muram_addr(start), 0, size_alloc); + entry = kmalloc(sizeof(*entry), GFP_KERNEL); + if (!entry) + goto out1; + entry->start = start; + entry->size = size_alloc; + list_add(&entry->head, &muram_block_list); spin_unlock_irqrestore(&cpm_muram_lock, flags); - return ret; + return start; +out1: + gen_pool_free(muram_pool, start, size_alloc); +out2: + spin_unlock_irqrestore(&cpm_muram_lock, flags); + return (unsigned long) -ENOMEM; + + } -EXPORT_SYMBOL(cpm_muram_free); +EXPORT_SYMBOL(cpm_muram_alloc_fixed); /** - * cpm_muram_alloc_fixed - reserve a specific region of multi-user ram - * @offset: the offset into the muram area to reserve - * @size: the number of bytes to reserve - * - * This function returns "start" on success, -ENOMEM on failure. - * Use cpm_dpram_addr() to get the virtual address of the area. - * Use cpm_muram_free() to free the allocation. + * cpm_muram_free - free a chunk of multi-user ram + * @offset: The beginning of the chunk as returned by cpm_muram_alloc(). */ -unsigned long cpm_muram_alloc_fixed(unsigned long offset, unsigned long size) +int cpm_muram_free(unsigned long offset) { - unsigned long start; unsigned long flags; + int size; + struct muram_block *tmp; + size = 0; spin_lock_irqsave(&cpm_muram_lock, flags); - cpm_muram_info.alignment = 1; - start = rh_alloc_fixed(&cpm_muram_info, offset, size, "commproc"); + list_for_each_entry(tmp, &muram_block_list, head) { + if (tmp->start == offset) { + size = tmp->size; + list_del(&tmp->head); + kfree(tmp); + break; + } + } + gen_pool_free(muram_pool, offset + GENPOOL_OFFSET, size); spin_unlock_irqrestore(&cpm_muram_lock, flags); - return start; + return size; } -EXPORT_SYMBOL(cpm_muram_alloc_fixed); +EXPORT_SYMBOL(cpm_muram_free); /** * cpm_muram_addr - turn a muram offset into a virtual address
Use genalloc to manage CPM/QE muram instead of rheap. Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com> --- Changes for v9: - splitted from patch 3/5, modify cpm muram management functions. Changes for v10: - modify cpm muram first, then move to qe_common - modify commit. arch/powerpc/platforms/Kconfig | 1 + arch/powerpc/sysdev/cpm_common.c | 150 +++++++++++++++++++++++++++------------ 2 files changed, 107 insertions(+), 44 deletions(-)