Build warning in drivers/dma/mmp_tdma.c

Build warning in drivers/dma/mmp_tdma.c

[Dan Williams]

My build warning test is failing on this driver please fix:

drivers/dma/mmp_tdma.c:236:8: warning: 'tdcr' may be used
uninitialized in this function [-Wuninitialized]

It's valid as mmp_tdma_control as the direction is specified in
mmp_tdma_control() and may not be one of the two tests in that branch.



I'm also carrying patch to add a missing dependency:

commit 1a4ee91dacedde3e82d8ce6eeace7f16884474f9
Author: Dan Williams <dan.j.williams@intel.com>
Date:   Thu Nov 28 12:27:38 2013 -0800

    dma: mmp_dma depends on CPU_MMP2

    It calls sram_get_gpool() which is only defined if CONFIG_CPU_MMP2=y

    Signed-off-by: Dan Williams <dan.j.williams@intel.com>

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 446687cc2334..dad83634cb65 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -287,7 +287,7 @@ config DMA_SA11X0

 config MMP_TDMA
        bool "MMP Two-Channel DMA support"
-       depends on ARCH_MMP
+       depends on ARCH_MMP && CPU_MMP2
        select DMA_ENGINE
        help
          Support the MMP Two-Channel DMA engine.

Is there a different ARCH type that this driver should be depending on?

--
Dan

Build warning in drivers/dma/mmp_tdma.c

[Vinod Koul]

On Thu, Nov 28, 2013 at 02:58:53PM -0800, Dan Williams wrote:
> My build warning test is failing on this driver please fix:
> 
> drivers/dma/mmp_tdma.c:236:8: warning: 'tdcr' may be used
> uninitialized in this function [-Wuninitialized]
> 
> It's valid as mmp_tdma_control as the direction is specified in
> mmp_tdma_control() and may not be one of the two tests in that branch.
Yup, i guess best would be init to zero, which is what i have done here
I did check this driver and got bunch of warns which i fixed in [1]. But i didnt
get this one, i need to redo my build scripts now or use yours :)

From: Vinod Koul <vinod.koul@intel.com>
Date: Fri, 29 Nov 2013 10:52:52 +0530
Subject: [PATCH] dmaengine: mmp: fix uninitialized variable

drivers/dma/mmp_tdma.c:236:8: warning: 'tdcr' may be used
uninitialized in this function [-Wuninitialized]

Reported-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 drivers/dma/mmp_tdma.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
index 61b562b..d4b730c 100644
--- a/drivers/dma/mmp_tdma.c
+++ b/drivers/dma/mmp_tdma.c
@@ -182,7 +182,7 @@ static void mmp_tdma_pause_chan(struct mmp_tdma_chan *tdmac)
 
 static int mmp_tdma_config_chan(struct mmp_tdma_chan *tdmac)
 {
-	unsigned int tdcr;
+	unsigned int tdcr = 0;
 
 	mmp_tdma_disable_chan(tdmac);
 
-- 
[1]: https://patchwork.kernel.org/patch/3253411/

> 
> 
> 
> I'm also carrying patch to add a missing dependency:
> 
> commit 1a4ee91dacedde3e82d8ce6eeace7f16884474f9
> Author: Dan Williams <dan.j.williams@intel.com>
> Date:   Thu Nov 28 12:27:38 2013 -0800
> 
>     dma: mmp_dma depends on CPU_MMP2
> 
>     It calls sram_get_gpool() which is only defined if CONFIG_CPU_MMP2=y
> 
>     Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 446687cc2334..dad83634cb65 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -287,7 +287,7 @@ config DMA_SA11X0
> 
>  config MMP_TDMA
>         bool "MMP Two-Channel DMA support"
> -       depends on ARCH_MMP
> +       depends on ARCH_MMP && CPU_MMP2
>         select DMA_ENGINE
>         help
>           Support the MMP Two-Channel DMA engine.
> 
> Is there a different ARCH type that this driver should be depending on?
This looks good to me though I tried removing CPU_MMP2 manually and compiling.
Kconfig tells me "selected by: MACH_BROWNSTONE [=y] && ARCH_MMP [=y] &&
!CPU_MOHAWK [=n]" so I think its implict here, but I think this patch ensures we
dont have build breakage

--
~Vinod

Build warning in drivers/dma/mmp_tdma.c

[Zhangfei Gao]

Thanks Dan

On Fri, Nov 29, 2013 at 6:58 AM, Dan Williams <dan.j.williams@intel.com> wrote:

> I'm also carrying patch to add a missing dependency:
>
> commit 1a4ee91dacedde3e82d8ce6eeace7f16884474f9
> Author: Dan Williams <dan.j.williams@intel.com>
> Date:   Thu Nov 28 12:27:38 2013 -0800
>
>     dma: mmp_dma depends on CPU_MMP2
>
>     It calls sram_get_gpool() which is only defined if CONFIG_CPU_MMP2=y
>
>     Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 446687cc2334..dad83634cb65 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -287,7 +287,7 @@ config DMA_SA11X0
>
>  config MMP_TDMA
>         bool "MMP Two-Channel DMA support"
> -       depends on ARCH_MMP
> +       depends on ARCH_MMP && CPU_MMP2

Hi, Qiao

Do you have comments?
Does this work on PXA910?

Does sram also use on pxa910?
arch/arm/mach-mmp/Makefile
obj-$(CONFIG_CPU_MMP2)          += mmp2.o sram.o

Zhangfei

Build warning in drivers/dma/mmp_tdma.c

[Qiao Zhou]

On 12/02/2013 02:42 PM, Zhangfei Gao wrote:
> Thanks Dan
>
> On Fri, Nov 29, 2013 at 6:58 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>
>> I'm also carrying patch to add a missing dependency:
>>
>> commit 1a4ee91dacedde3e82d8ce6eeace7f16884474f9
>> Author: Dan Williams <dan.j.williams@intel.com>
>> Date:   Thu Nov 28 12:27:38 2013 -0800
>>
>>      dma: mmp_dma depends on CPU_MMP2
>>
>>      It calls sram_get_gpool() which is only defined if CONFIG_CPU_MMP2=y
>>
>>      Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>
>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>> index 446687cc2334..dad83634cb65 100644
>> --- a/drivers/dma/Kconfig
>> +++ b/drivers/dma/Kconfig
>> @@ -287,7 +287,7 @@ config DMA_SA11X0
>>
>>   config MMP_TDMA
>>          bool "MMP Two-Channel DMA support"
>> -       depends on ARCH_MMP
>> +       depends on ARCH_MMP && CPU_MMP2
>
> Hi, Qiao
>
> Do you have comments?
> Does this work on PXA910?
MMP_TDMA should not depend on CPU_MMP2. PXA910, and other chip set also 
use mmp_tdma.
>
> Does sram also use on pxa910?
> arch/arm/mach-mmp/Makefile
> obj-$(CONFIG_CPU_MMP2)          += mmp2.o sram.o
yes, PXA910 also uses sram.
>
> Zhangfei
>


-- 

Best Regards
Qiao

Build warning in drivers/dma/mmp_tdma.c

[Zhangfei Gao]

Thanks Qiao for confirmation

On Mon, Dec 2, 2013 at 3:34 PM, Qiao Zhou <zhouqiao@marvell.com> wrote:
> On 12/02/2013 02:42 PM, Zhangfei Gao wrote:
>>
>> Thanks Dan
>>
>> On Fri, Nov 29, 2013 at 6:58 AM, Dan Williams <dan.j.williams@intel.com>
>> wrote:
>>
>>> I'm also carrying patch to add a missing dependency:
>>>
>>> commit 1a4ee91dacedde3e82d8ce6eeace7f16884474f9
>>> Author: Dan Williams <dan.j.williams@intel.com>
>>> Date:   Thu Nov 28 12:27:38 2013 -0800
>>>
>>>      dma: mmp_dma depends on CPU_MMP2
>>>
>>>      It calls sram_get_gpool() which is only defined if CONFIG_CPU_MMP2=y
>>>
>>>      Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>>
>>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>>> index 446687cc2334..dad83634cb65 100644
>>> --- a/drivers/dma/Kconfig
>>> +++ b/drivers/dma/Kconfig
>>> @@ -287,7 +287,7 @@ config DMA_SA11X0
>>>
>>>   config MMP_TDMA
>>>          bool "MMP Two-Channel DMA support"
>>> -       depends on ARCH_MMP
>>> +       depends on ARCH_MMP && CPU_MMP2
>>
>>
>> Hi, Qiao
>>
>> Do you have comments?
>> Does this work on PXA910?
>
> MMP_TDMA should not depend on CPU_MMP2. PXA910, and other chip set also use
> mmp_tdma.
>
>>
>> Does sram also use on pxa910?
>> arch/arm/mach-mmp/Makefile
>> obj-$(CONFIG_CPU_MMP2)          += mmp2.o sram.o
>
> yes, PXA910 also uses sram.

Then we may need clarify the dependence about sram.o to solve Dan's question?

Thanks

Build warning in drivers/dma/mmp_tdma.c

[Zhangfei Gao]

On Tue, Dec 3, 2013 at 4:05 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Sun, Dec 1, 2013 at 11:34 PM, Qiao Zhou <zhouqiao@marvell.com> wrote:

>>>> commit 1a4ee91dacedde3e82d8ce6eeace7f16884474f9
>>>> Author: Dan Williams <dan.j.williams@intel.com>
>>>> Date:   Thu Nov 28 12:27:38 2013 -0800
>>>>
>>>>      dma: mmp_dma depends on CPU_MMP2
>>>>
>>>>      It calls sram_get_gpool() which is only defined if CONFIG_CPU_MMP2=y
>>>>
>>>>      Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>>>
>>>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>>>> index 446687cc2334..dad83634cb65 100644
>>>> --- a/drivers/dma/Kconfig
>>>> +++ b/drivers/dma/Kconfig
>>>> @@ -287,7 +287,7 @@ config DMA_SA11X0
>>>>
>>>>   config MMP_TDMA
>>>>          bool "MMP Two-Channel DMA support"
>>>> -       depends on ARCH_MMP
>>>> +       depends on ARCH_MMP && CPU_MMP2
>>>
>>>
>>> Hi, Qiao
>>>
>>> Do you have comments?
>>> Does this work on PXA910?
>>
>> MMP_TDMA should not depend on CPU_MMP2. PXA910, and other chip set also use
>> mmp_tdma.
>
> It does depend on CPU_MMP2, as I can generate the following build
> failure with the attached config.
>
> drivers/built-in.o: In function `mmp_tdma_free_descriptor':
> drivers/dma/mmp_tdma.c:329: undefined reference to `sram_get_gpool'
> drivers/built-in.o: In function `mmp_tdma_alloc_descriptor':
> drivers/dma/mmp_tdma.c:379: undefined reference to `sram_get_gpool'
>

How about modifying arch/arm/mach-mmp/Makefile
Like:
-obj-y                          += common.o devices.o time.o
+obj-y                          += common.o devices.o time.o sram.o
-obj-$(CONFIG_CPU_MMP2)         += mmp2.o sram.o
+obj-$(CONFIG_CPU_MMP2)         += mmp2.o

As Qiao mentioned sram is used both on mmp2 and pxa910.

Thanks

Build warning in drivers/dma/mmp_tdma.c

[Haojian Zhuang]

On 12/03/2013 10:13 AM, Zhangfei Gao wrote:
> On Tue, Dec 3, 2013 at 4:05 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Sun, Dec 1, 2013 at 11:34 PM, Qiao Zhou <zhouqiao@marvell.com> wrote:
>
>>>>> commit 1a4ee91dacedde3e82d8ce6eeace7f16884474f9
>>>>> Author: Dan Williams <dan.j.williams@intel.com>
>>>>> Date:   Thu Nov 28 12:27:38 2013 -0800
>>>>>
>>>>>       dma: mmp_dma depends on CPU_MMP2
>>>>>
>>>>>       It calls sram_get_gpool() which is only defined if CONFIG_CPU_MMP2=y
>>>>>
>>>>>       Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>>>>
>>>>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>>>>> index 446687cc2334..dad83634cb65 100644
>>>>> --- a/drivers/dma/Kconfig
>>>>> +++ b/drivers/dma/Kconfig
>>>>> @@ -287,7 +287,7 @@ config DMA_SA11X0
>>>>>
>>>>>    config MMP_TDMA
>>>>>           bool "MMP Two-Channel DMA support"
>>>>> -       depends on ARCH_MMP
>>>>> +       depends on ARCH_MMP && CPU_MMP2
>>>>
>>>>
>>>> Hi, Qiao
>>>>
>>>> Do you have comments?
>>>> Does this work on PXA910?
>>>
>>> MMP_TDMA should not depend on CPU_MMP2. PXA910, and other chip set also use
>>> mmp_tdma.
>>
>> It does depend on CPU_MMP2, as I can generate the following build
>> failure with the attached config.
>>
>> drivers/built-in.o: In function `mmp_tdma_free_descriptor':
>> drivers/dma/mmp_tdma.c:329: undefined reference to `sram_get_gpool'
>> drivers/built-in.o: In function `mmp_tdma_alloc_descriptor':
>> drivers/dma/mmp_tdma.c:379: undefined reference to `sram_get_gpool'
>>
>
> How about modifying arch/arm/mach-mmp/Makefile
> Like:
> -obj-y                          += common.o devices.o time.o
> +obj-y                          += common.o devices.o time.o sram.o
> -obj-$(CONFIG_CPU_MMP2)         += mmp2.o sram.o
> +obj-$(CONFIG_CPU_MMP2)         += mmp2.o
>
> As Qiao mentioned sram is used both on mmp2 and pxa910.
>
> Thanks
>

I prefer to add CONFIG_MMP_SRAM instead, since sram isn't available in 
pxa168.

Regards
Haojian

Build warning in drivers/dma/mmp_tdma.c

[Qiao Zhou]

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

  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,
  				size);
@@ -346,7 +347,7 @@ struct mmp_tdma_desc 
*mmp_tdma_alloc_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 (!gpool)
  		return NULL;

@@ -477,7 +478,8 @@ static int mmp_tdma_remove(struct platform_device *pdev)
  }

  static int mmp_tdma_chan_init(struct mmp_tdma_device *tdev,
-						int idx, int irq, int type)
+					int idx, int irq,
+					int type, struct gen_pool *pool)
  {
  	struct mmp_tdma_chan *tdmac;

@@ -498,6 +500,7 @@ static int mmp_tdma_chan_init(struct mmp_tdma_device 
*tdev,
  	tdmac->chan.device = &tdev->device;
  	tdmac->idx	   = idx;
  	tdmac->type	   = type;
+	tdmac->pool	   = pool;
  	tdmac->reg_base	   = (unsigned long)tdev->base + idx * 4;
  	tdmac->status = DMA_SUCCESS;
  	tdev->tdmac[tdmac->idx] = tdmac;
@@ -525,6 +528,8 @@ static int mmp_tdma_probe(struct platform_device *pdev)
  	int i, ret;
  	int irq = 0, irq_num = 0;
  	int chan_num = TDMA_CHANNEL_NUM;
+	struct device_node *np = pdev->dev.of_node;
+	struct gen_pool *pool;

  	of_id = of_match_device(mmp_tdma_dt_ids, &pdev->dev);
  	if (of_id)
@@ -551,6 +556,13 @@ static int mmp_tdma_probe(struct platform_device *pdev)

  	INIT_LIST_HEAD(&tdev->device.channels);

+	/* Get audio sram from device tree or platform data */
+	pool = of_get_named_gen_pool(np, "asram", 0);
+	if (!pool) {
+		dev_err(&pdev->dev, "asram pool not available\n");
+		return -ENOMEM;
+	}
+
  	if (irq_num != chan_num) {
  		irq = platform_get_irq(pdev, 0);
  		ret = devm_request_irq(&pdev->dev, irq,
@@ -562,7 +574,7 @@ static int mmp_tdma_probe(struct platform_device *pdev)
  	/* initialize channel parameters */
  	for (i = 0; i < chan_num; i++) {
  		irq = (irq_num != chan_num) ? 0 : platform_get_irq(pdev, i);
-		ret = mmp_tdma_chan_init(tdev, i, irq, type);
+		ret = mmp_tdma_chan_init(tdev, i, irq, type, pool);
  		if (ret)
  			return ret;
  	}
-- 
1.7.0.4

-- 

Best Regards
Qiao

Build warning in drivers/dma/mmp_tdma.c

[Zhangfei Gao]

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

Build warning in drivers/dma/mmp_tdma.c

[Qiao Zhou]

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.

-- 

Best Regards
Qiao

Build warning in drivers/dma/mmp_tdma.c

[Dan Williams]

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.

Build warning in drivers/dma/mmp_tdma.c

[Qiao Zhou]

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

-- 

Best Regards
Qiao

Build warning in drivers/dma/mmp_tdma.c

[Dan Williams]

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.

Build warning in drivers/dma/mmp_tdma.c

[Qiao Zhou]

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.


-- 

Best Regards
Qiao

Build warning in drivers/dma/mmp_tdma.c

[Dan Williams]

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.

Build warning in drivers/dma/mmp_tdma.c

[Qiao Zhou]

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?

-- 

Best Regards
Qiao

Build warning in drivers/dma/mmp_tdma.c

[Zhangfei Gao]

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

Build warning in drivers/dma/mmp_tdma.c

[Qiao Zhou]

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

-- 

Best Regards
Qiao