| Message ID | 529DB5C8.1010500@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Qiao On Tue, Dec 3, 2013 at 6:43 PM, Qiao Zhou <zhouqiao@marvell.com> wrote: > From 5396b1bffd574dbe778387979ecc24c51ee0c7a4 Mon Sep 17 00:00:00 2001 > From: Qiao Zhou <zhouqiao@marvell.com> > Date: Tue, 3 Dec 2013 18:17:02 +0800 > Subject: [PATCH 2/2] dma: mmp-tdma: use gen_pool to get asram > > use generic pool to get audio sram, instead of sram driver > > > Signed-off-by: nhcao <nhcao@marvell.com> > Signed-off-by: Qiao Zhou <zhouqiao@marvell.com> Could you formally update this patch in another thread. Acked-by: Zhangfei Gao <zhangfei.gao@gmail.com> Thanks
On Tue, Dec 3, 2013 at 2:43 AM, Qiao Zhou <zhouqiao@marvell.com> wrote: > On 12/03/2013 10:31 AM, Haojian Zhuang wrote: >> >> I prefer to add CONFIG_MMP_SRAM instead, since sram isn't available in >> pxa168. > > Hi Dan, > > I have two patches. Could you help check whether it can fix the issue? > > BTW, actually the 2nd alone is enough. The 1st patch is just in case that > you still want to use the old sram driver, and you need to enable MMP_SRAM. > > All, please help review the patches. > > From 94601015d525db6d2baf47f4d517f38ae2e5e802 Mon Sep 17 00:00:00 2001 > From: Qiao Zhou <zhouqiao@marvell.com> > Date: Tue, 3 Dec 2013 11:06:29 +0800 > Subject: [PATCH 1/2] arm: mmp: build sram driver alone > > sram driver can be used by many chips besides CPU_MMP2, and so build > it alone. > > Signed-off-by: Qiao Zhou <zhouqiao@marvell.com> > --- > arch/arm/mach-mmp/Makefile | 3 ++- > > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-mmp/Makefile b/arch/arm/mach-mmp/Makefile > index 9b702a1..98f0f63 100644 > --- a/arch/arm/mach-mmp/Makefile > +++ b/arch/arm/mach-mmp/Makefile > @@ -7,7 +7,8 @@ obj-y += common.o devices.o time.o > # SoC support > obj-$(CONFIG_CPU_PXA168) += pxa168.o > obj-$(CONFIG_CPU_PXA910) += pxa910.o > > -obj-$(CONFIG_CPU_MMP2) += mmp2.o sram.o > +obj-$(CONFIG_CPU_MMP2) += mmp2.o > +obj-$(CONFIG_MMP_SRAM) += sram.o Missing the Kconfig change to add "config MMP_SRAM"? > > ifeq ($(CONFIG_COMMON_CLK), ) > obj-y += clock.o > -- > 1.7.0.4 > > > From 5396b1bffd574dbe778387979ecc24c51ee0c7a4 Mon Sep 17 00:00:00 2001 > From: Qiao Zhou <zhouqiao@marvell.com> > Date: Tue, 3 Dec 2013 18:17:02 +0800 > Subject: [PATCH 2/2] dma: mmp-tdma: use gen_pool to get asram > > use generic pool to get audio sram, instead of sram driver > > Signed-off-by: nhcao <nhcao@marvell.com> > Signed-off-by: Qiao Zhou <zhouqiao@marvell.com> > --- > drivers/dma/Kconfig | 1 + > drivers/dma/mmp_tdma.c | 22 +++++++++++++++++----- > 2 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig > index dd2874e..eeffe3c 100644 > --- a/drivers/dma/Kconfig > +++ b/drivers/dma/Kconfig > @@ -288,6 +288,7 @@ config MMP_TDMA > > bool "MMP Two-Channel DMA support" > depends on ARCH_MMP > select DMA_ENGINE > + select GENERIC_ALLOCATOR You still need a depends on MMP_SRAM right? Otherwise this just turns the compile error into a much more subtle runtime failure.
On 12/04/2013 05:02 AM, Dan Williams wrote: > On Tue, Dec 3, 2013 at 2:43 AM, Qiao Zhou <zhouqiao@marvell.com> wrote: >> On 12/03/2013 10:31 AM, Haojian Zhuang wrote: >>> >>> I prefer to add CONFIG_MMP_SRAM instead, since sram isn't available in >>> pxa168. >> >> Hi Dan, >> >> I have two patches. Could you help check whether it can fix the issue? >> >> BTW, actually the 2nd alone is enough. The 1st patch is just in case that >> you still want to use the old sram driver, and you need to enable MMP_SRAM. >> >> All, please help review the patches. >> >> From 94601015d525db6d2baf47f4d517f38ae2e5e802 Mon Sep 17 00:00:00 2001 >> From: Qiao Zhou <zhouqiao@marvell.com> >> Date: Tue, 3 Dec 2013 11:06:29 +0800 >> Subject: [PATCH 1/2] arm: mmp: build sram driver alone >> >> sram driver can be used by many chips besides CPU_MMP2, and so build >> it alone. >> >> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com> >> --- >> arch/arm/mach-mmp/Makefile | 3 ++- >> >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/arch/arm/mach-mmp/Makefile b/arch/arm/mach-mmp/Makefile >> index 9b702a1..98f0f63 100644 >> --- a/arch/arm/mach-mmp/Makefile >> +++ b/arch/arm/mach-mmp/Makefile >> @@ -7,7 +7,8 @@ obj-y += common.o devices.o time.o >> # SoC support >> obj-$(CONFIG_CPU_PXA168) += pxa168.o >> obj-$(CONFIG_CPU_PXA910) += pxa910.o >> >> -obj-$(CONFIG_CPU_MMP2) += mmp2.o sram.o >> +obj-$(CONFIG_CPU_MMP2) += mmp2.o >> +obj-$(CONFIG_MMP_SRAM) += sram.o > > Missing the Kconfig change to add "config MMP_SRAM"? The sram driver is unnecessary if we use generic pool. Zhangfei mentioned to remove this sram driver. It's just for your test. > >> >> ifeq ($(CONFIG_COMMON_CLK), ) >> obj-y += clock.o >> -- >> 1.7.0.4 >> >> >> From 5396b1bffd574dbe778387979ecc24c51ee0c7a4 Mon Sep 17 00:00:00 2001 >> From: Qiao Zhou <zhouqiao@marvell.com> >> Date: Tue, 3 Dec 2013 18:17:02 +0800 >> Subject: [PATCH 2/2] dma: mmp-tdma: use gen_pool to get asram >> >> use generic pool to get audio sram, instead of sram driver >> >> Signed-off-by: nhcao <nhcao@marvell.com> >> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com> >> --- >> drivers/dma/Kconfig | 1 + >> drivers/dma/mmp_tdma.c | 22 +++++++++++++++++----- >> 2 files changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig >> index dd2874e..eeffe3c 100644 >> --- a/drivers/dma/Kconfig >> +++ b/drivers/dma/Kconfig >> @@ -288,6 +288,7 @@ config MMP_TDMA >> >> bool "MMP Two-Channel DMA support" >> depends on ARCH_MMP >> select DMA_ENGINE >> + select GENERIC_ALLOCATOR > > You still need a depends on MMP_SRAM right? Otherwise this just turns > the compile error into a much more subtle runtime failure. > No dependence on MMP_SRAM(sram). You can refer to lib/genalloc.c
On 12/03/2013 07:05 PM, Zhangfei Gao wrote: > Hi, Qiao > > On Tue, Dec 3, 2013 at 6:43 PM, Qiao Zhou <zhouqiao@marvell.com> wrote: > >> From 5396b1bffd574dbe778387979ecc24c51ee0c7a4 Mon Sep 17 00:00:00 2001 >> From: Qiao Zhou <zhouqiao@marvell.com> >> Date: Tue, 3 Dec 2013 18:17:02 +0800 >> Subject: [PATCH 2/2] dma: mmp-tdma: use gen_pool to get asram >> >> use generic pool to get audio sram, instead of sram driver >> >> >> Signed-off-by: nhcao <nhcao@marvell.com> >> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com> > > Could you formally update this patch in another thread. > > Acked-by: Zhangfei Gao <zhangfei.gao@gmail.com> > > > Thanks > OK. Thanks for reviewing.
On Tue, Dec 3, 2013 at 5:08 PM, Qiao Zhou <zhouqiao@marvell.com> wrote: > On 12/04/2013 05:02 AM, Dan Williams wrote: >> >> On Tue, Dec 3, 2013 at 2:43 AM, Qiao Zhou <zhouqiao@marvell.com> wrote: >>> >>> On 12/03/2013 10:31 AM, Haojian Zhuang wrote: >>>> >>>> >>>> I prefer to add CONFIG_MMP_SRAM instead, since sram isn't available in >>>> pxa168. >>> >>> >>> Hi Dan, >>> >>> I have two patches. Could you help check whether it can fix the issue? >>> >>> BTW, actually the 2nd alone is enough. The 1st patch is just in case that >>> you still want to use the old sram driver, and you need to enable >>> MMP_SRAM. >>> >>> All, please help review the patches. >>> >>> From 94601015d525db6d2baf47f4d517f38ae2e5e802 Mon Sep 17 00:00:00 2001 >>> From: Qiao Zhou <zhouqiao@marvell.com> >>> Date: Tue, 3 Dec 2013 11:06:29 +0800 >>> Subject: [PATCH 1/2] arm: mmp: build sram driver alone >>> >>> sram driver can be used by many chips besides CPU_MMP2, and so build >>> it alone. >>> >>> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com> >>> --- >>> arch/arm/mach-mmp/Makefile | 3 ++- >>> >>> 1 files changed, 2 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/arm/mach-mmp/Makefile b/arch/arm/mach-mmp/Makefile >>> index 9b702a1..98f0f63 100644 >>> --- a/arch/arm/mach-mmp/Makefile >>> +++ b/arch/arm/mach-mmp/Makefile >>> @@ -7,7 +7,8 @@ obj-y += common.o devices.o >>> time.o >>> # SoC support >>> obj-$(CONFIG_CPU_PXA168) += pxa168.o >>> obj-$(CONFIG_CPU_PXA910) += pxa910.o >>> >>> -obj-$(CONFIG_CPU_MMP2) += mmp2.o sram.o >>> +obj-$(CONFIG_CPU_MMP2) += mmp2.o >>> +obj-$(CONFIG_MMP_SRAM) += sram.o >> >> >> Missing the Kconfig change to add "config MMP_SRAM"? > > The sram driver is unnecessary if we use generic pool. Zhangfei mentioned to > remove this sram driver. It's just for your test. >> >> >>> >>> ifeq ($(CONFIG_COMMON_CLK), ) >>> obj-y += clock.o >>> -- >>> 1.7.0.4 >>> >>> >>> From 5396b1bffd574dbe778387979ecc24c51ee0c7a4 Mon Sep 17 00:00:00 2001 >>> From: Qiao Zhou <zhouqiao@marvell.com> >>> Date: Tue, 3 Dec 2013 18:17:02 +0800 >>> Subject: [PATCH 2/2] dma: mmp-tdma: use gen_pool to get asram >>> >>> use generic pool to get audio sram, instead of sram driver >>> >>> Signed-off-by: nhcao <nhcao@marvell.com> >>> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com> >>> --- >>> drivers/dma/Kconfig | 1 + >>> drivers/dma/mmp_tdma.c | 22 +++++++++++++++++----- >>> 2 files changed, 18 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig >>> index dd2874e..eeffe3c 100644 >>> --- a/drivers/dma/Kconfig >>> +++ b/drivers/dma/Kconfig >>> @@ -288,6 +288,7 @@ config MMP_TDMA >>> >>> bool "MMP Two-Channel DMA support" >>> depends on ARCH_MMP >>> select DMA_ENGINE >>> + select GENERIC_ALLOCATOR >> >> >> You still need a depends on MMP_SRAM right? Otherwise this just turns >> the compile error into a much more subtle runtime failure. >> > No dependence on MMP_SRAM(sram). You can refer to lib/genalloc.c Please read the question, you can refer to of_get_named_gen_pool() for why I have a question. Something in the system needs to do the devm_gen_pool_create() for that device. If you are removing the mmp2 sram driver are you switching to the generic sram driver? If so shouldn't you ensure it is built? Otherwise this will always fail: + pool = of_get_named_gen_pool(np, "asram", 0); + if (!pool) { + dev_err(&pdev->dev, "asram pool not available\n"); + return -ENOMEM; + } Ultimately I'm not in a position to care as long as this driver builds cleanly.
On 12/04/2013 09:32 AM, Dan Williams wrote: > Please read the question, you can refer to of_get_named_gen_pool() for > why I have a question. Something in the system needs to do the > devm_gen_pool_create() for that device. If you are removing the mmp2 > sram driver are you switching to the generic sram driver? If so > shouldn't you ensure it is built? Otherwise this will always fail: For CPU_MMP2 specifically, it switches to use generic sram driver. But generally it may not use sram only, a DDR buffer(or other buffer) may also be a pool. So here we don't add "select SRAM" directly. In this case we need to enable CONFIG_SRAM in mmp2_defconfig. If no sram or other similar drivers are enabled, it will throw an error for warning. > > + pool = of_get_named_gen_pool(np, "asram", 0); > + if (!pool) { > + dev_err(&pdev->dev, "asram pool not available\n"); > + return -ENOMEM; > + } > > Ultimately I'm not in a position to care as long as this driver builds cleanly.
On Tue, Dec 3, 2013 at 5:58 PM, Qiao Zhou <zhouqiao@marvell.com> wrote: > On 12/04/2013 09:32 AM, Dan Williams wrote: >> >> Please read the question, you can refer to of_get_named_gen_pool() for >> why I have a question. Something in the system needs to do the >> devm_gen_pool_create() for that device. If you are removing the mmp2 >> sram driver are you switching to the generic sram driver? If so >> shouldn't you ensure it is built? Otherwise this will always fail: > > For CPU_MMP2 specifically, it switches to use generic sram driver. But > generally it may not use sram only, a DDR buffer(or other buffer) may also > be a pool. So here we don't add "select SRAM" directly. In this case we need > to enable CONFIG_SRAM in mmp2_defconfig. > > If no sram or other similar drivers are enabled, it will throw an error for > warning. > Ok, So, please turn this into a compile time dependency (depends on (SRAM || MMP_SRAM)) so that someone does not need to boot a platform to figure out they forgot to enable a driver.
On 12/04/2013 10:15 AM, Dan Williams wrote: > On Tue, Dec 3, 2013 at 5:58 PM, Qiao Zhou <zhouqiao@marvell.com> wrote: >> On 12/04/2013 09:32 AM, Dan Williams wrote: >>> >>> Please read the question, you can refer to of_get_named_gen_pool() for >>> why I have a question. Something in the system needs to do the >>> devm_gen_pool_create() for that device. If you are removing the mmp2 >>> sram driver are you switching to the generic sram driver? If so >>> shouldn't you ensure it is built? Otherwise this will always fail: >> >> For CPU_MMP2 specifically, it switches to use generic sram driver. But >> generally it may not use sram only, a DDR buffer(or other buffer) may also >> be a pool. So here we don't add "select SRAM" directly. In this case we need >> to enable CONFIG_SRAM in mmp2_defconfig. >> >> If no sram or other similar drivers are enabled, it will throw an error for >> warning. >> > > Ok, So, please turn this into a compile time dependency (depends on > (SRAM || MMP_SRAM)) so that someone does not need to boot a platform > to figure out they forgot to enable a driver. > Actually I'm not sure it's good to add such dependency. sound/core/memalloc.c provides the same way for sram buffer allocation. It doesn't add such dependency. Zhangfei, Haojian, how do you think?
On Wed, Dec 4, 2013 at 10:28 AM, Qiao Zhou <zhouqiao@marvell.com> wrote: > On 12/04/2013 10:15 AM, Dan Williams wrote: >> >> On Tue, Dec 3, 2013 at 5:58 PM, Qiao Zhou <zhouqiao@marvell.com> wrote: >>> >>> On 12/04/2013 09:32 AM, Dan Williams wrote: >>>> >>>> >>>> Please read the question, you can refer to of_get_named_gen_pool() for >>>> why I have a question. Something in the system needs to do the >>>> devm_gen_pool_create() for that device. If you are removing the mmp2 >>>> sram driver are you switching to the generic sram driver? If so >>>> shouldn't you ensure it is built? Otherwise this will always fail: >>> >>> >>> For CPU_MMP2 specifically, it switches to use generic sram driver. But Do you mean CPU_MMP2 still need arch/arm/mach-mmp/sram.c? Is it can be replaced by drivers/misc/sram.c and be removed at all? So only "SRAM" is depends or selected. >>> generally it may not use sram only, a DDR buffer(or other buffer) may >>> also >>> be a pool. So here we don't add "select SRAM" directly. In this case we >>> need >>> to enable CONFIG_SRAM in mmp2_defconfig. >>> >>> If no sram or other similar drivers are enabled, it will throw an error >>> for >>> warning. >>> >> >> Ok, So, please turn this into a compile time dependency (depends on >> (SRAM || MMP_SRAM)) so that someone does not need to boot a platform >> to figure out they forgot to enable a driver. >> > Actually I'm not sure it's good to add such dependency. > sound/core/memalloc.c provides the same way for sram buffer allocation. It > doesn't add such dependency. SRAM can be selected or depended, while MMP_SRAM should be removed latter. > > Zhangfei, Haojian, how do you think? > > -- > > Best Regards > Qiao
On 12/04/2013 10:34 AM, Zhangfei Gao wrote: > On Wed, Dec 4, 2013 at 10:28 AM, Qiao Zhou <zhouqiao@marvell.com> wrote: >> On 12/04/2013 10:15 AM, Dan Williams wrote: >>> >>> On Tue, Dec 3, 2013 at 5:58 PM, Qiao Zhou <zhouqiao@marvell.com> wrote: >>>> >>>> On 12/04/2013 09:32 AM, Dan Williams wrote: >>>>> >>>>> >>>>> Please read the question, you can refer to of_get_named_gen_pool() for >>>>> why I have a question. Something in the system needs to do the >>>>> devm_gen_pool_create() for that device. If you are removing the mmp2 >>>>> sram driver are you switching to the generic sram driver? If so >>>>> shouldn't you ensure it is built? Otherwise this will always fail: >>>> >>>> >>>> For CPU_MMP2 specifically, it switches to use generic sram driver. But > > Do you mean CPU_MMP2 still need arch/arm/mach-mmp/sram.c? > Is it can be replaced by drivers/misc/sram.c and be removed at all? > So only "SRAM" is depends or selected. drivers/misc/sram.c is enough for CPU_MMP2. > >>>> generally it may not use sram only, a DDR buffer(or other buffer) may >>>> also >>>> be a pool. So here we don't add "select SRAM" directly. In this case we >>>> need >>>> to enable CONFIG_SRAM in mmp2_defconfig. >>>> >>>> If no sram or other similar drivers are enabled, it will throw an error >>>> for >>>> warning. >>>> >>> >>> Ok, So, please turn this into a compile time dependency (depends on >>> (SRAM || MMP_SRAM)) so that someone does not need to boot a platform >>> to figure out they forgot to enable a driver. >>> >> Actually I'm not sure it's good to add such dependency. >> sound/core/memalloc.c provides the same way for sram buffer allocation. It >> doesn't add such dependency. > > SRAM can be selected or depended, while MMP_SRAM should be removed latter. OK. >> >> Zhangfei, Haojian, how do you think? >> >> -- >> >> Best Regards >> Qiao
diff --git a/arch/arm/mach-mmp/Makefile b/arch/arm/mach-mmp/Makefile index 9b702a1..98f0f63 100644 --- a/arch/arm/mach-mmp/Makefile +++ b/arch/arm/mach-mmp/Makefile @@ -7,7 +7,8 @@ obj-y += common.o devices.o time.o # SoC support obj-$(CONFIG_CPU_PXA168) += pxa168.o obj-$(CONFIG_CPU_PXA910) += pxa910.o -obj-$(CONFIG_CPU_MMP2) += mmp2.o sram.o +obj-$(CONFIG_CPU_MMP2) += mmp2.o +obj-$(CONFIG_MMP_SRAM) += sram.o ifeq ($(CONFIG_COMMON_CLK), ) obj-y += clock.o -- 1.7.0.4 From 5396b1bffd574dbe778387979ecc24c51ee0c7a4 Mon Sep 17 00:00:00 2001 From: Qiao Zhou <zhouqiao@marvell.com> Date: Tue, 3 Dec 2013 18:17:02 +0800 Subject: [PATCH 2/2] dma: mmp-tdma: use gen_pool to get asram use generic pool to get audio sram, instead of sram driver Signed-off-by: nhcao <nhcao@marvell.com> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com> --- drivers/dma/Kconfig | 1 + drivers/dma/mmp_tdma.c | 22 +++++++++++++++++----- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig index dd2874e..eeffe3c 100644 --- a/drivers/dma/Kconfig +++ b/drivers/dma/Kconfig @@ -288,6 +288,7 @@ config MMP_TDMA bool "MMP Two-Channel DMA support" depends on ARCH_MMP select DMA_ENGINE + select GENERIC_ALLOCATOR help Support the MMP Two-Channel DMA engine. This engine used for MMP Audio DMA and pxa910 SQU. diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c index d3b6358..93537d1 100644 --- a/drivers/dma/mmp_tdma.c +++ b/drivers/dma/mmp_tdma.c @@ -20,7 +20,7 @@ #include <linux/platform_device.h> #include <linux/device.h> #include <mach/regs-icu.h> -#include <linux/platform_data/dma-mmp_tdma.h> +#include <linux/genalloc.h> #include <linux/of_device.h> #include "dmaengine.h" @@ -121,6 +121,7 @@ struct mmp_tdma_chan { size_t buf_len; size_t period_len; size_t pos; + struct gen_pool *pool; }; #define TDMA_CHANNEL_NUM 2 @@ -296,7 +297,7 @@ static void mmp_tdma_free_descriptor(struct mmp_tdma_chan *tdmac) struct gen_pool *gpool; int size = tdmac->desc_num * sizeof(struct mmp_tdma_desc); - gpool = sram_get_gpool("asram"); + gpool = tdmac->pool; if (tdmac->desc_arr) gen_pool_free(gpool, (unsigned long)tdmac->desc_arr,