[PATCH] net: ethernet: ti: cpdma: switch to use genalloc
TI CPDMA currently uses a bitmap for tracking descriptors alloactions allocations, but The genalloc already handles the same and can be used as with special memory (SRAM) as with DMA cherent memory chank (dma_alloc_coherent()). Hence, switch to using genalloc and add desc_num property for each channel for limitation of max number of allowed descriptors for each CPDMA channel. This patch do not affect on net throuput. Cc: Ivan Khoronzhuk Signed-off-by: Grygorii Strashko --- Testing TCP window: 256K, bandwidth in Mbits/sec: host: iperf -s device: iperf -c 172.22.39.17 -t600 -i5 -d -w128K AM437x-idk, 1Gbps link before: : 341.60, after: 232+123=355 am57xx-beagle-x15, 1Gbps link before: : 1112.80, after: 814+321=1135 am335x-boneblack, 100Mbps link before: : 162.40, after: 72+93=165 drivers/net/ethernet/ti/davinci_cpdma.c | 136 +++- 1 file changed, 62 insertions(+), 74 deletions(-) diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c index 18bf3a8..03b9882 100644 --- a/drivers/net/ethernet/ti/davinci_cpdma.c +++ b/drivers/net/ethernet/ti/davinci_cpdma.c @@ -21,7 +21,7 @@ #include #include #include - +#include #include "davinci_cpdma.h" /* DMA Registers */ @@ -87,9 +87,8 @@ struct cpdma_desc_pool { void*cpumap;/* dma_alloc map */ int desc_size, mem_size; int num_desc, used_desc; - unsigned long *bitmap; struct device *dev; - spinlock_t lock; + struct gen_pool *gen_pool; }; enum cpdma_state { @@ -117,6 +116,7 @@ struct cpdma_chan { int chan_num; spinlock_t lock; int count; + u32 desc_num; u32 mask; cpdma_handler_fnhandler; enum dma_data_direction dir; @@ -145,6 +145,20 @@ struct cpdma_chan { (directed << CPDMA_TO_PORT_SHIFT));\ } while (0) +static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) +{ + if (!pool) + return; + + WARN_ON(pool->used_desc); + if (pool->cpumap) { + dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, + pool->phys); + } else { + iounmap(pool->iomap); + } +} + /* * Utility constructs for a cpdma descriptor pool. Some devices (e.g. davinci * emac) have dedicated on-chip memory for these descriptors. Some other @@ -155,24 +169,25 @@ static struct cpdma_desc_pool * cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t hw_addr, int size, int align) { - int bitmap_size; struct cpdma_desc_pool *pool; + int ret; pool = devm_kzalloc(dev, sizeof(*pool), GFP_KERNEL); if (!pool) - goto fail; - - spin_lock_init(&pool->lock); + goto gen_pool_create_fail; pool->dev = dev; pool->mem_size = size; pool->desc_size = ALIGN(sizeof(struct cpdma_desc), align); pool->num_desc = size / pool->desc_size; - bitmap_size = (pool->num_desc / BITS_PER_LONG) * sizeof(long); - pool->bitmap = devm_kzalloc(dev, bitmap_size, GFP_KERNEL); - if (!pool->bitmap) - goto fail; + pool->gen_pool = devm_gen_pool_create(dev, ilog2(pool->desc_size), -1, + "cpdma"); + if (IS_ERR(pool->gen_pool)) { + dev_err(dev, "pool create failed %ld\n", + PTR_ERR(pool->gen_pool)); + goto gen_pool_create_fail; + } if (phys) { pool->phys = phys; @@ -185,24 +200,22 @@ cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t hw_addr, pool->phys = pool->hw_addr; /* assumes no IOMMU, don't use this value */ } - if (pool->iomap) - return pool; -fail: - return NULL; -} - -static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) -{ - if (!pool) - return; + if (!pool->iomap) + goto gen_pool_create_fail; - WARN_ON(pool->used_desc); - if (pool->cpumap) { - dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, - pool->phys); - } else { - iounmap(pool->iomap); + ret = gen_pool_add_virt(pool->gen_pool, (unsigned long)pool->iomap, + pool->phys, pool->mem_size, -1); + if (ret < 0) { + dev_err(dev, "pool add failed %d\n", ret); + goto gen_pool_add_virt_fail; } + + return pool; + +gen_pool_add_virt_fail: + cpdma_desc_pool_destroy(poo
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc
On 23.06.16 15:36, Grygorii Strashko wrote: TI CPDMA currently uses a bitmap for tracking descriptors alloactions allocations, but The genalloc already handles the same and can be used as with special memory (SRAM) as with DMA cherent memory chank (dma_alloc_coherent()). Hence, switch to using genalloc and add desc_num property for each channel for limitation of max number of allowed descriptors for each CPDMA channel. This patch do not affect on net throuput. Cc: Ivan Khoronzhuk Signed-off-by: Grygorii Strashko --- Testing TCP window: 256K, bandwidth in Mbits/sec: host: iperf -s device: iperf -c 172.22.39.17 -t600 -i5 -d -w128K AM437x-idk, 1Gbps link before: : 341.60, after: 232+123=355 am57xx-beagle-x15, 1Gbps link before: : 1112.80, after: 814+321=1135 am335x-boneblack, 100Mbps link before: : 162.40, after: 72+93=165 drivers/net/ethernet/ti/davinci_cpdma.c | 136 +++- 1 file changed, 62 insertions(+), 74 deletions(-) diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c index 18bf3a8..03b9882 100644 --- a/drivers/net/ethernet/ti/davinci_cpdma.c +++ b/drivers/net/ethernet/ti/davinci_cpdma.c @@ -21,7 +21,7 @@ #include #include #include - +#include #include "davinci_cpdma.h" /* DMA Registers */ @@ -87,9 +87,8 @@ struct cpdma_desc_pool { void*cpumap;/* dma_alloc map */ int desc_size, mem_size; int num_desc, used_desc; - unsigned long *bitmap; struct device *dev; - spinlock_t lock; + struct gen_pool *gen_pool; }; enum cpdma_state { @@ -117,6 +116,7 @@ struct cpdma_chan { int chan_num; spinlock_t lock; int count; + u32 desc_num; u32 mask; cpdma_handler_fnhandler; enum dma_data_direction dir; @@ -145,6 +145,20 @@ struct cpdma_chan { (directed << CPDMA_TO_PORT_SHIFT)); \ } while (0) +static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) +{ + if (!pool) + return; + + WARN_ON(pool->used_desc); + if (pool->cpumap) { + dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, + pool->phys); + } else { + iounmap(pool->iomap); + } +} + single if, brackets? /* * Utility constructs for a cpdma descriptor pool. Some devices (e.g. davinci * emac) have dedicated on-chip memory for these descriptors. Some other @@ -155,24 +169,25 @@ static struct cpdma_desc_pool * cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t hw_addr, int size, int align) { - int bitmap_size; struct cpdma_desc_pool *pool; + int ret; pool = devm_kzalloc(dev, sizeof(*pool), GFP_KERNEL); if (!pool) - goto fail; - - spin_lock_init(&pool->lock); + goto gen_pool_create_fail; pool->dev= dev; pool->mem_size = size; pool->desc_size = ALIGN(sizeof(struct cpdma_desc), align); pool->num_desc = size / pool->desc_size; - bitmap_size = (pool->num_desc / BITS_PER_LONG) * sizeof(long); - pool->bitmap = devm_kzalloc(dev, bitmap_size, GFP_KERNEL); - if (!pool->bitmap) - goto fail; + pool->gen_pool = devm_gen_pool_create(dev, ilog2(pool->desc_size), -1, + "cpdma"); + if (IS_ERR(pool->gen_pool)) { + dev_err(dev, "pool create failed %ld\n", + PTR_ERR(pool->gen_pool)); + goto gen_pool_create_fail; + } if (phys) { pool->phys = phys; @@ -185,24 +200,22 @@ cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t hw_addr, pool->phys = pool->hw_addr; /* assumes no IOMMU, don't use this value */ } - if (pool->iomap) - return pool; -fail: - return NULL; -} - -static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) -{ - if (!pool) - return; + if (!pool->iomap) + goto gen_pool_create_fail; - WARN_ON(pool->used_desc); - if (pool->cpumap) { - dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, - pool->phys); - } else { - iounmap(pool->iomap); + ret = gen_pool_add_virt(pool->gen_pool, (unsigned long)pool->iomap, + pool->phys, pool->mem_size, -1); + if (ret < 0) { + dev_err(dev, "pool add failed %d\n", ret); + goto gen_pool_add_virt_fail;
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc
On 23.06.16 15:36, Grygorii Strashko wrote: TI CPDMA currently uses a bitmap for tracking descriptors alloactions allocations, but The genalloc already handles the same and can be used as with special memory (SRAM) as with DMA cherent memory chank (dma_alloc_coherent()). Hence, switch to using genalloc and add desc_num property for each channel for limitation of max number of allowed descriptors for each CPDMA channel. This patch do not affect on net throuput. Cc: Ivan Khoronzhuk Signed-off-by: Grygorii Strashko Tested-by: Ivan Khoronzhuk --- Testing TCP window: 256K, bandwidth in Mbits/sec: host: iperf -s device: iperf -c 172.22.39.17 -t600 -i5 -d -w128K AM437x-idk, 1Gbps link before: : 341.60, after: 232+123=355 am57xx-beagle-x15, 1Gbps link before: : 1112.80, after: 814+321=1135 am335x-boneblack, 100Mbps link before: : 162.40, after: 72+93=165 drivers/net/ethernet/ti/davinci_cpdma.c | 136 +++- 1 file changed, 62 insertions(+), 74 deletions(-) diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c index 18bf3a8..03b9882 100644 --- a/drivers/net/ethernet/ti/davinci_cpdma.c +++ b/drivers/net/ethernet/ti/davinci_cpdma.c @@ -21,7 +21,7 @@ #include #include #include - +#include #include "davinci_cpdma.h" /* DMA Registers */ @@ -87,9 +87,8 @@ struct cpdma_desc_pool { void*cpumap;/* dma_alloc map */ int desc_size, mem_size; int num_desc, used_desc; - unsigned long *bitmap; struct device *dev; - spinlock_t lock; + struct gen_pool *gen_pool; }; enum cpdma_state { @@ -117,6 +116,7 @@ struct cpdma_chan { int chan_num; spinlock_t lock; int count; + u32 desc_num; u32 mask; cpdma_handler_fnhandler; enum dma_data_direction dir; @@ -145,6 +145,20 @@ struct cpdma_chan { (directed << CPDMA_TO_PORT_SHIFT)); \ } while (0) +static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) +{ + if (!pool) + return; + + WARN_ON(pool->used_desc); + if (pool->cpumap) { + dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, + pool->phys); + } else { + iounmap(pool->iomap); + } +} + /* * Utility constructs for a cpdma descriptor pool. Some devices (e.g. davinci * emac) have dedicated on-chip memory for these descriptors. Some other @@ -155,24 +169,25 @@ static struct cpdma_desc_pool * cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t hw_addr, int size, int align) { - int bitmap_size; struct cpdma_desc_pool *pool; + int ret; pool = devm_kzalloc(dev, sizeof(*pool), GFP_KERNEL); if (!pool) - goto fail; - - spin_lock_init(&pool->lock); + goto gen_pool_create_fail; pool->dev= dev; pool->mem_size = size; pool->desc_size = ALIGN(sizeof(struct cpdma_desc), align); pool->num_desc = size / pool->desc_size; - bitmap_size = (pool->num_desc / BITS_PER_LONG) * sizeof(long); - pool->bitmap = devm_kzalloc(dev, bitmap_size, GFP_KERNEL); - if (!pool->bitmap) - goto fail; + pool->gen_pool = devm_gen_pool_create(dev, ilog2(pool->desc_size), -1, + "cpdma"); + if (IS_ERR(pool->gen_pool)) { + dev_err(dev, "pool create failed %ld\n", + PTR_ERR(pool->gen_pool)); + goto gen_pool_create_fail; + } if (phys) { pool->phys = phys; @@ -185,24 +200,22 @@ cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t hw_addr, pool->phys = pool->hw_addr; /* assumes no IOMMU, don't use this value */ } - if (pool->iomap) - return pool; -fail: - return NULL; -} - -static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) -{ - if (!pool) - return; + if (!pool->iomap) + goto gen_pool_create_fail; - WARN_ON(pool->used_desc); - if (pool->cpumap) { - dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, - pool->phys); - } else { - iounmap(pool->iomap); + ret = gen_pool_add_virt(pool->gen_pool, (unsigned long)pool->iomap, + pool->phys, pool->mem_size, -1); + if (ret < 0) { + dev_err(dev, "pool add failed %d\n", ret); + goto gen_pool_add_vi
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc
On Thursday 23 June 2016 06:06 PM, Grygorii Strashko wrote: > TI CPDMA currently uses a bitmap for tracking descriptors alloactions > allocations, but The genalloc already handles the same and can be used > as with special memory (SRAM) as with DMA cherent memory chank > (dma_alloc_coherent()). Hence, switch to using genalloc and add > desc_num property for each channel for limitation of max number of > allowed descriptors for each CPDMA channel. This patch do not affect > on net throuput. > > Cc: Ivan Khoronzhuk > Signed-off-by: Grygorii Strashko Acked-by: Mugunthan V N Regards Mugunthan V N
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc
On Thursday 23 June 2016 06:26 PM, Ivan Khoronzhuk wrote: > > > On 23.06.16 15:36, Grygorii Strashko wrote: >> TI CPDMA currently uses a bitmap for tracking descriptors alloactions >> allocations, but The genalloc already handles the same and can be used >> as with special memory (SRAM) as with DMA cherent memory chank >> (dma_alloc_coherent()). Hence, switch to using genalloc and add >> desc_num property for each channel for limitation of max number of >> allowed descriptors for each CPDMA channel. This patch do not affect >> on net throuput. >> >> Cc: Ivan Khoronzhuk >> Signed-off-by: Grygorii Strashko >> --- >> Testing >> TCP window: 256K, bandwidth in Mbits/sec: >> host: iperf -s >> device: iperf -c 172.22.39.17 -t600 -i5 -d -w128K >> >> AM437x-idk, 1Gbps link >> before: : 341.60, after: 232+123=355 >> am57xx-beagle-x15, 1Gbps link >> before: : 1112.80, after: 814+321=1135 >> am335x-boneblack, 100Mbps link >> before: : 162.40, after: 72+93=165 >> >> drivers/net/ethernet/ti/davinci_cpdma.c | 136 >> +++- >> 1 file changed, 62 insertions(+), 74 deletions(-) >> >> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c >> b/drivers/net/ethernet/ti/davinci_cpdma.c >> index 18bf3a8..03b9882 100644 >> --- a/drivers/net/ethernet/ti/davinci_cpdma.c >> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c >> @@ -21,7 +21,7 @@ >> #include >> #include >> #include >> - >> +#include >> #include "davinci_cpdma.h" >> >> /* DMA Registers */ >> @@ -87,9 +87,8 @@ struct cpdma_desc_pool { >> void*cpumap;/* dma_alloc map */ >> intdesc_size, mem_size; >> intnum_desc, used_desc; >> -unsigned long*bitmap; >> struct device*dev; >> -spinlock_tlock; >> +struct gen_pool*gen_pool; >> }; >> >> enum cpdma_state { >> @@ -117,6 +116,7 @@ struct cpdma_chan { >> intchan_num; >> spinlock_tlock; >> intcount; >> +u32desc_num; >> u32mask; >> cpdma_handler_fnhandler; >> enum dma_data_directiondir; >> @@ -145,6 +145,20 @@ struct cpdma_chan { >>(directed << CPDMA_TO_PORT_SHIFT));\ >> } while (0) >> >> +static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) >> +{ >> +if (!pool) >> +return; >> + >> +WARN_ON(pool->used_desc); >> +if (pool->cpumap) { >> +dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, >> + pool->phys); >> +} else { >> +iounmap(pool->iomap); >> +} >> +} >> + > single if, brackets? if() has multiple line statement, so brackets are must. Regards Mugunthan V N > >> /* >>* Utility constructs for a cpdma descriptor pool. Some devices >> (e.g. davinci >>* emac) have dedicated on-chip memory for these descriptors. Some >> other >> @@ -155,24 +169,25 @@ static struct cpdma_desc_pool * >> cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t >> hw_addr, >> int size, int align) >> { >> -int bitmap_size; >> struct cpdma_desc_pool *pool; >> +int ret; >> >> pool = devm_kzalloc(dev, sizeof(*pool), GFP_KERNEL); >> if (!pool) >> -goto fail; >> - >> -spin_lock_init(&pool->lock); >> +goto gen_pool_create_fail; >> >> pool->dev= dev; >> pool->mem_size= size; >> pool->desc_size= ALIGN(sizeof(struct cpdma_desc), align); >> pool->num_desc= size / pool->desc_size; >> >> -bitmap_size = (pool->num_desc / BITS_PER_LONG) * sizeof(long); >> -pool->bitmap = devm_kzalloc(dev, bitmap_size, GFP_KERNEL); >> -if (!pool->bitmap) >> -goto fail; >> +pool->gen_pool = devm_gen_pool_create(dev, >> ilog2(pool->desc_size), -1, >> + "cpdma"); >> +if (IS_ERR(pool->gen_pool)) { >> +dev_err(dev, "pool create failed %ld\n", >> +PTR_ERR(pool->gen_pool)); >> +goto gen_pool_create_fail; >> +} >> >> if (phys) { >> pool->phys = phys; >> @@ -185,24 +200,22 @@ cpdma_desc_pool_create(struct device *dev, u32 >> phys, dma_addr_t hw_addr, >> pool->phys = pool->hw_addr; /* assumes no IOMMU, don't use >> this value */ >> } >> >> -if (pool->iomap) >> -return pool; >> -fail: >> -return NULL; >> -} >> - >> -static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) >> -{ >> -if (!pool) >> -return; >> +if (!pool->iomap) >> +goto gen_pool_create_fail; >> >> -WARN_ON(pool->used_desc); >> -if (pool->cpumap) { >> -dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, >> - pool->phys); >> -} else { >> -iounmap(pool->iomap); >> +ret = gen_pool_add_virt(pool->gen_pool, (unsigned long)pool->iomap, >> +pool->phys, pool->mem_size, -1); >> +
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc
Hi, On Fri, Jun 24, 2016 at 11:35:15AM +0530, Mugunthan V N wrote: > On Thursday 23 June 2016 06:26 PM, Ivan Khoronzhuk wrote: > >> +if (pool->cpumap) { > >> +dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, > >> + pool->phys); > >> +} else { > >> +iounmap(pool->iomap); > >> +} > > single if, brackets? > > if() has multiple line statement, so brackets are must. Another paint to the bikeshed, seems documented coding style mentions otherwise. Regards afzal
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc
On Fri, Jun 24, 2016 at 11:35:15AM +0530, Mugunthan V N wrote: > >> +static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) > >> +{ > >> +if (!pool) > >> +return; > >> + > >> +WARN_ON(pool->used_desc); > >> +if (pool->cpumap) { > >> +dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, > >> + pool->phys); > >> +} else { > >> +iounmap(pool->iomap); > >> +} > >> +} > >> + > > single if, brackets? > > if() has multiple line statement, so brackets are must. It is line wrapped, it is still one statement. And you can't argue the else being multiple lines, although the style does require using brackets for the else if the if required them. Style says "Do not unnecessarily use braces where a single statement will do." It says statement, not line. A multiline wrapped statement is still one statement. I may personally hate the lack of brackets, but style wise it seems very clear that the linux kernel only uses brakcets when required, which is only when there is more than one statement. I prefer what you did, but not as much as I prefer consistency. -- Len Sorensen
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc
On 06/24/2016 07:15 PM, Lennart Sorensen wrote: > On Fri, Jun 24, 2016 at 11:35:15AM +0530, Mugunthan V N wrote: +static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool) +{ +if (!pool) +return; + +WARN_ON(pool->used_desc); +if (pool->cpumap) { +dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap, + pool->phys); +} else { +iounmap(pool->iomap); +} +} + >>> single if, brackets? >> >> if() has multiple line statement, so brackets are must. > > It is line wrapped, it is still one statement. And you can't argue the > else being multiple lines, although the style does require using brackets > for the else if the if required them. > > Style says "Do not unnecessarily use braces where a single statement will do." > It says statement, not line. A multiline wrapped statement is still > one statement. > > I may personally hate the lack of brackets, but style wise it seems very > clear that the linux kernel only uses brakcets when required, which is > only when there is more than one statement. I prefer what you did, > but not as much as I prefer consistency. > Oh. nice :( So, seems, I'd need to send v3. Right? By the way, this code hasn't been introduced by this patch - I've just moved whole function from one place to another. -- regards, -grygorii
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc
On Fri, Jun 24, 2016 at 07:58:32PM +0300, Grygorii Strashko wrote: > Oh. nice :( So, seems, I'd need to send v3. Right? > By the way, this code hasn't been introduced by this patch - I've > just moved whole function from one place to another. Well since it is moving I would think that was a handy time to fix the coding style violation too, since it got noticed. That leaves just one place in that file violating that part of the coding style (the other is in cpdma_chan_dump). Somehow it wasn't spotted when the code was put in back in 2010, and since they were wrapped lines, they don't stand out quite as much visually. -- Len Sorensen
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc
Hi, On Fri, Jun 24, 2016 at 12:15:41PM -0400, Lennart Sorensen wrote: > although the style does require using brackets for the else if the > if required them. As an aside, though most of the style rationale is K & R, K & R consistently uses unbalanced braces for if-else-* For a one that learns C unadultered from K & R, probably Kernel coding style comes naturally, except for trivial things like above. ...a brick for the shed. Regards afzal