[PATCH v3 0/5] dma: cppi41: some trivial fixes and support for suspend/resume

[PATCH v3 0/5] dma: cppi41: some trivial fixes and support for suspend/resume

[Daniel Mack]

Here are some patches to teach the cppi41 DMA driver support for
suspend and resume. Patches 1-3 are simply cosmetic things that emerged
during my debugging sessions.

Patch #4 is actually a real bugfix which I would like Sebastian
Andrzej Siewior to have a look at. Quite frankly, the allocation scheme
in this driver and the logic to determine a descriptor's index number
seems quite complicated to me, but that's probably a different story.
Allocating and freeing the exact same pointer more than once is
certainly a bug.

Patch #5 adds support for suspend and resume. As the commit log says,
the code needed to achive this is the result of a trial-and-error
session. This is the minimum that's I'm left with now and which works
for me.

On a different note, I'm also working on patches for the musb core to
make it suspend and resume. Currently, I still have to rmmod/insmod
musb_dsps before/after the resume cycle, but I'm hoping to make
progress here soon.


Thanks,
Daniel

v2 -> v3:
	* Patch #5: kill DEV_PM_OPS macro hack and fix a typo
	  (Reported by Sergei Shtylyov)

v1 -> v2:
	* Patch #5: depend on CONFIG_PM_SLEEP rather than on
	  CONFIG_PM and use SIMPLE_DEV_PM_OPS
	  (Reported by Sergei Shtylyov)

Daniel Mack (5):
  dma: cppi41: pass around device instead of platform_device
  dma: cppi41: s/deinit_cpii41/deinit_cppi41/
  dma: cppi41: add shortcut to &pdev->dev in cppi41_dma_probe()
  dma: cppi41: only allocate descriptor memory once
  dma: cppi41: add support for suspend and resume

 drivers/dma/cppi41.c | 115 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 70 insertions(+), 45 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/5] dma: cppi41: pass around device instead of platform_device

[Daniel Mack]

Instead of passing around struct plafform_device, use struct device and
save one level of dereferencing. This affects the following functions:

 * cppi41_add_chans
 * purge_descs
 * deinit_cpii41
 * init_descs
 * init_cppi41
 * cppi_glue_infos

It's just a cosmetic cleanup that makes the code more readable.

Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/dma/cppi41.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index 7c82b92..53d1d31 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -674,14 +674,14 @@ static void cleanup_chans(struct cppi41_dd *cdd)
 	}
 }
 
-static int cppi41_add_chans(struct platform_device *pdev, struct cppi41_dd *cdd)
+static int cppi41_add_chans(struct device *dev, struct cppi41_dd *cdd)
 {
 	struct cppi41_channel *cchan;
 	int i;
 	int ret;
 	u32 n_chans;
 
-	ret = of_property_read_u32(pdev->dev.of_node, "#dma-channels",
+	ret = of_property_read_u32(dev->of_node, "#dma-channels",
 			&n_chans);
 	if (ret)
 		return ret;
@@ -719,7 +719,7 @@ err:
 	return -ENOMEM;
 }
 
-static void purge_descs(struct platform_device *pdev, struct cppi41_dd *cdd)
+static void purge_descs(struct device *dev, struct cppi41_dd *cdd)
 {
 	unsigned int mem_decs;
 	int i;
@@ -731,7 +731,7 @@ static void purge_descs(struct platform_device *pdev, struct cppi41_dd *cdd)
 		cppi_writel(0, cdd->qmgr_mem + QMGR_MEMBASE(i));
 		cppi_writel(0, cdd->qmgr_mem + QMGR_MEMCTRL(i));
 
-		dma_free_coherent(&pdev->dev, mem_decs, cdd->cd,
+		dma_free_coherent(dev, mem_decs, cdd->cd,
 				cdd->descs_phys);
 	}
 }
@@ -741,19 +741,19 @@ static void disable_sched(struct cppi41_dd *cdd)
 	cppi_writel(0, cdd->sched_mem + DMA_SCHED_CTRL);
 }
 
-static void deinit_cpii41(struct platform_device *pdev, struct cppi41_dd *cdd)
+static void deinit_cpii41(struct device *dev, struct cppi41_dd *cdd)
 {
 	disable_sched(cdd);
 
-	purge_descs(pdev, cdd);
+	purge_descs(dev, cdd);
 
 	cppi_writel(0, cdd->qmgr_mem + QMGR_LRAM0_BASE);
 	cppi_writel(0, cdd->qmgr_mem + QMGR_LRAM0_BASE);
-	dma_free_coherent(&pdev->dev, QMGR_SCRATCH_SIZE, cdd->qmgr_scratch,
+	dma_free_coherent(dev, QMGR_SCRATCH_SIZE, cdd->qmgr_scratch,
 			cdd->scratch_phys);
 }
 
-static int init_descs(struct platform_device *pdev, struct cppi41_dd *cdd)
+static int init_descs(struct device *dev, struct cppi41_dd *cdd)
 {
 	unsigned int desc_size;
 	unsigned int mem_decs;
@@ -777,7 +777,7 @@ static int init_descs(struct platform_device *pdev, struct cppi41_dd *cdd)
 		reg |= ilog2(ALLOC_DECS_NUM) - 5;
 
 		BUILD_BUG_ON(DESCS_AREAS != 1);
-		cdd->cd = dma_alloc_coherent(&pdev->dev, mem_decs,
+		cdd->cd = dma_alloc_coherent(dev, mem_decs,
 				&cdd->descs_phys, GFP_KERNEL);
 		if (!cdd->cd)
 			return -ENOMEM;
@@ -813,12 +813,12 @@ static void init_sched(struct cppi41_dd *cdd)
 	cppi_writel(reg, cdd->sched_mem + DMA_SCHED_CTRL);
 }
 
-static int init_cppi41(struct platform_device *pdev, struct cppi41_dd *cdd)
+static int init_cppi41(struct device *dev, struct cppi41_dd *cdd)
 {
 	int ret;
 
 	BUILD_BUG_ON(QMGR_SCRATCH_SIZE > ((1 << 14) - 1));
-	cdd->qmgr_scratch = dma_alloc_coherent(&pdev->dev, QMGR_SCRATCH_SIZE,
+	cdd->qmgr_scratch = dma_alloc_coherent(dev, QMGR_SCRATCH_SIZE,
 			&cdd->scratch_phys, GFP_KERNEL);
 	if (!cdd->qmgr_scratch)
 		return -ENOMEM;
@@ -827,7 +827,7 @@ static int init_cppi41(struct platform_device *pdev, struct cppi41_dd *cdd)
 	cppi_writel(QMGR_SCRATCH_SIZE, cdd->qmgr_mem + QMGR_LRAM_SIZE);
 	cppi_writel(0, cdd->qmgr_mem + QMGR_LRAM1_BASE);
 
-	ret = init_descs(pdev, cdd);
+	ret = init_descs(dev, cdd);
 	if (ret)
 		goto err_td;
 
@@ -835,7 +835,7 @@ static int init_cppi41(struct platform_device *pdev, struct cppi41_dd *cdd)
 	init_sched(cdd);
 	return 0;
 err_td:
-	deinit_cpii41(pdev, cdd);
+	deinit_cpii41(dev, cdd);
 	return ret;
 }
 
@@ -914,11 +914,11 @@ static const struct of_device_id cppi41_dma_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, cppi41_dma_ids);
 
-static const struct cppi_glue_infos *get_glue_info(struct platform_device *pdev)
+static const struct cppi_glue_infos *get_glue_info(struct device *dev)
 {
 	const struct of_device_id *of_id;
 
-	of_id = of_match_node(cppi41_dma_ids, pdev->dev.of_node);
+	of_id = of_match_node(cppi41_dma_ids, dev->of_node);
 	if (!of_id)
 		return NULL;
 	return of_id->data;
@@ -931,7 +931,7 @@ static int cppi41_dma_probe(struct platform_device *pdev)
 	int irq;
 	int ret;
 
-	glue_info = get_glue_info(pdev);
+	glue_info = get_glue_info(&pdev->dev);
 	if (!glue_info)
 		return -EINVAL;
 
@@ -970,11 +970,11 @@ static int cppi41_dma_probe(struct platform_device *pdev)
 	cdd->queues_tx = glue_info->queues_tx;
 	cdd->td_queue = glue_info->td_queue;
 
-	ret = init_cppi41(pdev, cdd);
+	ret = init_cppi41(&pdev->dev, cdd);
 	if (ret)
 		goto err_init_cppi;
 
-	ret = cppi41_add_chans(pdev, cdd);
+	ret = cppi41_add_chans(&pdev->dev, cdd);
 	if (ret)
 		goto err_chans;
 
@@ -1009,7 +1009,7 @@ err_irq:
 	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
 	cleanup_chans(cdd);
 err_chans:
-	deinit_cpii41(pdev, cdd);
+	deinit_cpii41(&pdev->dev, cdd);
 err_init_cppi:
 	pm_runtime_put(&pdev->dev);
 err_get_sync:
@@ -1033,7 +1033,7 @@ static int cppi41_dma_remove(struct platform_device *pdev)
 	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
 	free_irq(cdd->irq, cdd);
 	cleanup_chans(cdd);
-	deinit_cpii41(pdev, cdd);
+	deinit_cpii41(&pdev->dev, cdd);
 	iounmap(cdd->usbss_mem);
 	iounmap(cdd->ctrl_mem);
 	iounmap(cdd->sched_mem);
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] dma: cppi41: pass around device instead of platform_device

[Vinod Koul]

On Sun, Sep 22, 2013 at 04:50:00PM +0200, Daniel Mack wrote:
> Instead of passing around struct plafform_device, use struct device and
> save one level of dereferencing. This affects the following functions:
> 
>  * cppi41_add_chans
>  * purge_descs
>  * deinit_cpii41
>  * init_descs
>  * init_cppi41
>  * cppi_glue_infos
> 
> It's just a cosmetic cleanup that makes the code more readable.
Applied, thanks

~Vinod

[PATCH v3 2/5] dma: cppi41: s/deinit_cpii41/deinit_cppi41/

[Daniel Mack]

Fix a misspelled function name.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/cppi41.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index 53d1d31..5469a15 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -741,7 +741,7 @@ static void disable_sched(struct cppi41_dd *cdd)
 	cppi_writel(0, cdd->sched_mem + DMA_SCHED_CTRL);
 }
 
-static void deinit_cpii41(struct device *dev, struct cppi41_dd *cdd)
+static void deinit_cppi41(struct device *dev, struct cppi41_dd *cdd)
 {
 	disable_sched(cdd);
 
@@ -835,7 +835,7 @@ static int init_cppi41(struct device *dev, struct cppi41_dd *cdd)
 	init_sched(cdd);
 	return 0;
 err_td:
-	deinit_cpii41(dev, cdd);
+	deinit_cppi41(dev, cdd);
 	return ret;
 }
 
@@ -1009,7 +1009,7 @@ err_irq:
 	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
 	cleanup_chans(cdd);
 err_chans:
-	deinit_cpii41(&pdev->dev, cdd);
+	deinit_cppi41(&pdev->dev, cdd);
 err_init_cppi:
 	pm_runtime_put(&pdev->dev);
 err_get_sync:
@@ -1033,7 +1033,7 @@ static int cppi41_dma_remove(struct platform_device *pdev)
 	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
 	free_irq(cdd->irq, cdd);
 	cleanup_chans(cdd);
-	deinit_cpii41(&pdev->dev, cdd);
+	deinit_cppi41(&pdev->dev, cdd);
 	iounmap(cdd->usbss_mem);
 	iounmap(cdd->ctrl_mem);
 	iounmap(cdd->sched_mem);
-- 
1.8.3.1

Re: [PATCH v3 2/5] dma: cppi41: s/deinit_cpii41/deinit_cppi41/

[Vinod Koul]

On Sun, Sep 22, 2013 at 04:50:01PM +0200, Daniel Mack wrote:
> Fix a misspelled function name.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
Applied, thanks

~Vinod
-- 

[PATCH v3 3/5] dma: cppi41: add shortcut to &pdev->dev in cppi41_dma_probe()

[Daniel Mack]

Makes the code more readable and compact. No functional change.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/cppi41.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index 5469a15..d689706 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -927,11 +927,12 @@ static const struct cppi_glue_infos *get_glue_info(struct device *dev)
 static int cppi41_dma_probe(struct platform_device *pdev)
 {
 	struct cppi41_dd *cdd;
+	struct device *dev = &pdev->dev;
 	const struct cppi_glue_infos *glue_info;
 	int irq;
 	int ret;
 
-	glue_info = get_glue_info(&pdev->dev);
+	glue_info = get_glue_info(dev);
 	if (!glue_info)
 		return -EINVAL;
 
@@ -946,14 +947,14 @@ static int cppi41_dma_probe(struct platform_device *pdev)
 	cdd->ddev.device_issue_pending = cppi41_dma_issue_pending;
 	cdd->ddev.device_prep_slave_sg = cppi41_dma_prep_slave_sg;
 	cdd->ddev.device_control = cppi41_dma_control;
-	cdd->ddev.dev = &pdev->dev;
+	cdd->ddev.dev = dev;
 	INIT_LIST_HEAD(&cdd->ddev.channels);
 	cpp41_dma_info.dma_cap = cdd->ddev.cap_mask;
 
-	cdd->usbss_mem = of_iomap(pdev->dev.of_node, 0);
-	cdd->ctrl_mem = of_iomap(pdev->dev.of_node, 1);
-	cdd->sched_mem = of_iomap(pdev->dev.of_node, 2);
-	cdd->qmgr_mem = of_iomap(pdev->dev.of_node, 3);
+	cdd->usbss_mem = of_iomap(dev->of_node, 0);
+	cdd->ctrl_mem = of_iomap(dev->of_node, 1);
+	cdd->sched_mem = of_iomap(dev->of_node, 2);
+	cdd->qmgr_mem = of_iomap(dev->of_node, 3);
 
 	if (!cdd->usbss_mem || !cdd->ctrl_mem || !cdd->sched_mem ||
 			!cdd->qmgr_mem) {
@@ -961,8 +962,8 @@ static int cppi41_dma_probe(struct platform_device *pdev)
 		goto err_remap;
 	}
 
-	pm_runtime_enable(&pdev->dev);
-	ret = pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret)
 		goto err_get_sync;
 
@@ -970,22 +971,22 @@ static int cppi41_dma_probe(struct platform_device *pdev)
 	cdd->queues_tx = glue_info->queues_tx;
 	cdd->td_queue = glue_info->td_queue;
 
-	ret = init_cppi41(&pdev->dev, cdd);
+	ret = init_cppi41(dev, cdd);
 	if (ret)
 		goto err_init_cppi;
 
-	ret = cppi41_add_chans(&pdev->dev, cdd);
+	ret = cppi41_add_chans(dev, cdd);
 	if (ret)
 		goto err_chans;
 
-	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	irq = irq_of_parse_and_map(dev->of_node, 0);
 	if (!irq)
 		goto err_irq;
 
 	cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
 
 	ret = request_irq(irq, glue_info->isr, IRQF_SHARED,
-			dev_name(&pdev->dev), cdd);
+			dev_name(dev), cdd);
 	if (ret)
 		goto err_irq;
 	cdd->irq = irq;
@@ -994,7 +995,7 @@ static int cppi41_dma_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_dma_reg;
 
-	ret = of_dma_controller_register(pdev->dev.of_node,
+	ret = of_dma_controller_register(dev->of_node,
 			cppi41_dma_xlate, &cpp41_dma_info);
 	if (ret)
 		goto err_of;
@@ -1009,11 +1010,11 @@ err_irq:
 	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
 	cleanup_chans(cdd);
 err_chans:
-	deinit_cppi41(&pdev->dev, cdd);
+	deinit_cppi41(dev, cdd);
 err_init_cppi:
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 err_get_sync:
-	pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(dev);
 	iounmap(cdd->usbss_mem);
 	iounmap(cdd->ctrl_mem);
 	iounmap(cdd->sched_mem);
-- 
1.8.3.1

Re: [PATCH v3 3/5] dma: cppi41: add shortcut to &pdev->dev in cppi41_dma_probe()

[Vinod Koul]

On Sun, Sep 22, 2013 at 04:50:02PM +0200, Daniel Mack wrote:
> Makes the code more readable and compact. No functional change.
> 
> Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Applied, thanks

~Vinod
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Daniel Mack]

cdd->cd and cdd->descs_phys are allocated DESCS_AREAS times from
init_descs() and freed as often from purge_descs(). This leads to both
memory leaks and double-frees.

Fix this by pulling the calls to dma_{alloc,free}_coherent() out of the
loops.

While at it, remove the intermediate variable mem_decs (I guess it was
only there to make the code comply to the 80-chars CodingSytle rule).

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/cppi41.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index d689706..3347321 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -727,13 +727,12 @@ static void purge_descs(struct device *dev, struct cppi41_dd *cdd)
 	mem_decs = ALLOC_DECS_NUM * sizeof(struct cppi41_desc);
 
 	for (i = 0; i < DESCS_AREAS; i++) {
-
 		cppi_writel(0, cdd->qmgr_mem + QMGR_MEMBASE(i));
 		cppi_writel(0, cdd->qmgr_mem + QMGR_MEMCTRL(i));
-
-		dma_free_coherent(dev, mem_decs, cdd->cd,
-				cdd->descs_phys);
 	}
+
+	dma_free_coherent(dev, ALLOC_DECS_NUM * sizeof(struct cppi41_desc),
+			  cdd->cd, cdd->descs_phys);
 }
 
 static void disable_sched(struct cppi41_dd *cdd)
@@ -755,8 +754,7 @@ static void deinit_cppi41(struct device *dev, struct cppi41_dd *cdd)
 
 static int init_descs(struct device *dev, struct cppi41_dd *cdd)
 {
-	unsigned int desc_size;
-	unsigned int mem_decs;
+	unsigned int desc_size = sizeof(struct cppi41_desc);
 	int i;
 	u32 reg;
 	u32 idx;
@@ -765,28 +763,25 @@ static int init_descs(struct device *dev, struct cppi41_dd *cdd)
 			(sizeof(struct cppi41_desc) - 1));
 	BUILD_BUG_ON(sizeof(struct cppi41_desc) < 32);
 	BUILD_BUG_ON(ALLOC_DECS_NUM < 32);
+	BUILD_BUG_ON(DESCS_AREAS != 1);
 
-	desc_size = sizeof(struct cppi41_desc);
-	mem_decs = ALLOC_DECS_NUM * desc_size;
+	cdd->cd = dma_alloc_coherent(dev, ALLOC_DECS_NUM * desc_size,
+				     &cdd->descs_phys, GFP_KERNEL);
+	if (!cdd->cd)
+		return -ENOMEM;
 
 	idx = 0;
 	for (i = 0; i < DESCS_AREAS; i++) {
-
 		reg = idx << QMGR_MEMCTRL_IDX_SH;
 		reg |= (ilog2(desc_size) - 5) << QMGR_MEMCTRL_DESC_SH;
 		reg |= ilog2(ALLOC_DECS_NUM) - 5;
 
-		BUILD_BUG_ON(DESCS_AREAS != 1);
-		cdd->cd = dma_alloc_coherent(dev, mem_decs,
-				&cdd->descs_phys, GFP_KERNEL);
-		if (!cdd->cd)
-			return -ENOMEM;
-
 		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
 		cppi_writel(reg, cdd->qmgr_mem + QMGR_MEMCTRL(i));
 
 		idx += ALLOC_DECS_NUM;
 	}
+
 	return 0;
 }
 
-- 
1.8.3.1

Re: [PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Vinod Koul]

On Sun, Sep 22, 2013 at 04:50:03PM +0200, Daniel Mack wrote:
> cdd->cd and cdd->descs_phys are allocated DESCS_AREAS times from
> init_descs() and freed as often from purge_descs(). This leads to both
> memory leaks and double-frees.
> 
> Fix this by pulling the calls to dma_{alloc,free}_coherent() out of the
> loops.
> 
> While at it, remove the intermediate variable mem_decs (I guess it was
> only there to make the code comply to the 80-chars CodingSytle rule).
>
Looks fine, Sebastian cna you test it pls

~Vinod

Re: [PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Sebastian Andrzej Siewior]

On 09/23/2013 06:17 AM, Vinod Koul wrote:
> Looks fine, Sebastian cna you test it pls

Just noticed that you already applied some of them. I just got back
after a few weeks of. Will review & test as soon as I get to it.

> 
> ~Vinod
> 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Vinod Koul]

On Mon, Sep 23, 2013 at 04:51:06PM +0200, Sebastian Andrzej Siewior wrote:
> On 09/23/2013 06:17 AM, Vinod Koul wrote:
> > Looks fine, Sebastian cna you test it pls
> 
> Just noticed that you already applied some of them. I just got back
> after a few weeks of. Will review & test as soon as I get to it.
Yes, the first three were trvial, 5th one looked fine to me !

~Vinod
-- 

Re: [PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Sebastian Andrzej Siewior]

* Daniel Mack | 2013-09-22 16:50:03 [+0200]:

>cdd->cd and cdd->descs_phys are allocated DESCS_AREAS times from
>init_descs() and freed as often from purge_descs(). This leads to both
>memory leaks and double-frees.
>
>Fix this by pulling the calls to dma_{alloc,free}_coherent() out of the
>loops.
>
>While at it, remove the intermediate variable mem_decs (I guess it was
>only there to make the code comply to the 80-chars CodingSytle rule).
>
>Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Please don't merge the memory descriptors. The idea was initially to
allocate multiple small descriptors instead one big. The descrriptor
turned out to be enough so it looks like the way it looks.
If you want to clean this up, please either remove the for loop and
allocate only one memory area or please prepare for multiple descripors
(I think two is the upper limit).

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Daniel Mack]

Hi Sebastian,

sorry for the long delay, I got distracted by other things.


On 26.09.2013 10:26, Sebastian Andrzej Siewior wrote:
> * Daniel Mack | 2013-09-22 16:50:03 [+0200]:
> 
>> cdd->cd and cdd->descs_phys are allocated DESCS_AREAS times from
>> init_descs() and freed as often from purge_descs(). This leads to both
>> memory leaks and double-frees.
>>
>> Fix this by pulling the calls to dma_{alloc,free}_coherent() out of the
>> loops.
>>
>> While at it, remove the intermediate variable mem_decs (I guess it was
>> only there to make the code comply to the 80-chars CodingSytle rule).
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
> 
> Please don't merge the memory descriptors. The idea was initially to
> allocate multiple small descriptors instead one big. The descrriptor
> turned out to be enough so it looks like the way it looks.
> If you want to clean this up, please either remove the for loop and
> allocate only one memory area or please prepare for multiple descripors
> (I think two is the upper limit).

Well, I didn't merge the descriptors. Look again at my changes please.

A simplified version of the code as it stands is:

  for (i = 0; i < DESCS_AREAS; i++)
    cdd->cd = dma_alloc_coherent(dev, ..., &cdd->descs_phys, GFP_KERNEL);

  for (i = 0; i < DESCS_AREAS; i++)
    dma_free_coherent(dev, mem_decs, cdd->cd, cdd->descs_phys);

So you're effectively allocating and freeing the same pointer
DESCS_AREAS times, which is certainly not what you wanted.

And this just doesn't hit you because DESCS_AREAS is always 1:

  BUILD_BUG_ON(DESCS_AREAS != 1);


So, after all, my patch doesn't really change any of the runtime
behaviour. Consider it a cosmetic cleanup if you wish :)


Thanks,
Daniel

Re: [PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Sebastian Andrzej Siewior]

On 10/01/2013 03:09 PM, Daniel Mack wrote:
> Hi Sebastian,

Hi Daniel,

> sorry for the long delay, I got distracted by other things.

No problem.

> Well, I didn't merge the descriptors. Look again at my changes please.
> 
> A simplified version of the code as it stands is:
> 
>   for (i = 0; i < DESCS_AREAS; i++)
>     cdd->cd = dma_alloc_coherent(dev, ..., &cdd->descs_phys, GFP_KERNEL);
> 
>   for (i = 0; i < DESCS_AREAS; i++)
>     dma_free_coherent(dev, mem_decs, cdd->cd, cdd->descs_phys);
> 
> So you're effectively allocating and freeing the same pointer
> DESCS_AREAS times, which is certainly not what you wanted.

Well, no but since DESCS_AREAS is 1 it does not matter.

> 
> And this just doesn't hit you because DESCS_AREAS is always 1:

exactly.

> 
>   BUILD_BUG_ON(DESCS_AREAS != 1);
> 
> 
> So, after all, my patch doesn't really change any of the runtime
> behaviour. Consider it a cosmetic cleanup if you wish :)

But it looks strange (in my opinion at least). But now, that I look
again at it yes you moved the alloc out of the loop and replaced
mem_decs with the computation. So nothing changed but you moved it
outside.

Right I think one desc area should be enough so I would just remove the
for loop and DESCS_AREAS as well. How does this sound to you?

> 
> 
> Thanks,
> Daniel
> 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Daniel Mack]

On 01.10.2013 18:22, Sebastian Andrzej Siewior wrote:
> On 10/01/2013 03:09 PM, Daniel Mack wrote:

>> A simplified version of the code as it stands is:
>>
>>   for (i = 0; i < DESCS_AREAS; i++)
>>     cdd->cd = dma_alloc_coherent(dev, ..., &cdd->descs_phys, GFP_KERNEL);
>>
>>   for (i = 0; i < DESCS_AREAS; i++)
>>     dma_free_coherent(dev, mem_decs, cdd->cd, cdd->descs_phys);

...

>> So, after all, my patch doesn't really change any of the runtime
>> behaviour. Consider it a cosmetic cleanup if you wish :)
> 
> But it looks strange (in my opinion at least).

I still disagree, but maybe that doesn't matter any more, because ...

> But now, that I look
> again at it yes you moved the alloc out of the loop and replaced
> mem_decs with the computation. So nothing changed but you moved it
> outside.
> 
> Right I think one desc area should be enough so I would just remove the
> for loop and DESCS_AREAS as well. How does this sound to you?

That sound's like a good idea. Everything that make the driver smaller
and easier to understand is certainly a good thing :)

So we can drop this patch favor of your cleanup. However, I appreciate
if you did it on top of the second round of patches I sent today please,
because my resume() implementation uses DESCS_AREAS as well.


Thanks,
Daniel

Re: [PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Sebastian Andrzej Siewior]

On 10/01/2013 06:57 PM, Daniel Mack wrote:
> That sound's like a good idea. Everything that make the driver smaller
> and easier to understand is certainly a good thing :)
> 
> So we can drop this patch favor of your cleanup. However, I appreciate
> if you did it on top of the second round of patches I sent today please,
> because my resume() implementation uses DESCS_AREAS as well.

Okay, no problem. Let me look at those and once they are merged I do
the cleanup.

> 
> 
> Thanks,
> Daniel

Sebastian

[PATCH v3 5/5] dma: cppi41: add support for suspend and resume

[Daniel Mack]

This patch adds support for suspend/resume functionality to the cppi41
DMA driver. The steps necessary to make the system resume properly were
figured out by trial-and-error. The code as it stands now is the
minimum that has to be done to put the musb host system on an AM33xx
system into an operable state after resume.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/dma/cppi41.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index 3347321..89decc9 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -1040,12 +1040,41 @@ static int cppi41_dma_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int cppi41_suspend(struct device *dev)
+{
+	struct cppi41_dd *cdd = dev_get_drvdata(dev);
+
+	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
+	disable_sched(cdd);
+
+	return 0;
+}
+
+static int cppi41_resume(struct device *dev)
+{
+	struct cppi41_dd *cdd = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < DESCS_AREAS; i++)
+		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
+
+	init_sched(cdd);
+	cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(cppi41_pm_ops, cppi41_suspend, cppi41_resume);
+
 static struct platform_driver cpp41_dma_driver = {
 	.probe  = cppi41_dma_probe,
 	.remove = cppi41_dma_remove,
 	.driver = {
 		.name = "cppi41-dma-engine",
 		.owner = THIS_MODULE,
+		.pm = &cppi41_pm_ops,
 		.of_match_table = of_match_ptr(cppi41_dma_ids),
 	},
 };
-- 
1.8.3.1

Re: [PATCH v3 5/5] dma: cppi41: add support for suspend and resume

[Vinod Koul]

On Sun, Sep 22, 2013 at 04:50:04PM +0200, Daniel Mack wrote:
> This patch adds support for suspend/resume functionality to the cppi41
> DMA driver. The steps necessary to make the system resume properly were
> figured out by trial-and-error. The code as it stands now is the
> minimum that has to be done to put the musb host system on an AM33xx
> system into an operable state after resume.
> 
> Signed-off-by: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/dma/cppi41.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> index 3347321..89decc9 100644
> --- a/drivers/dma/cppi41.c
> +++ b/drivers/dma/cppi41.c
> @@ -1040,12 +1040,41 @@ static int cppi41_dma_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
ahhhh

> +static int cppi41_suspend(struct device *dev)
> +{
> +	struct cppi41_dd *cdd = dev_get_drvdata(dev);
> +
> +	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
> +	disable_sched(cdd);
> +
> +	return 0;
> +}
> +
> +static int cppi41_resume(struct device *dev)
> +{
> +	struct cppi41_dd *cdd = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < DESCS_AREAS; i++)
> +		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
> +
> +	init_sched(cdd);
> +	cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(cppi41_pm_ops, cppi41_suspend, cppi41_resume);
Here is the macro in pm.h

#ifdef CONFIG_PM_SLEEP
#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
        .suspend = suspend_fn, \
        .resume = resume_fn, \
        .freeze = suspend_fn, \
        .thaw = resume_fn, \
        .poweroff = suspend_fn, \
        .restore = resume_fn,
#else
#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
#endif

/*
 * Use this if you want to use the same suspend and resume callbacks for suspend
 * to RAM and hibernation.
 */
#define SIMPLE_DEV_PM_OPS(name, suspend_fn, resume_fn) \
const struct dev_pm_ops name = { \
        SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \

Now since you are using the macro there should be no need to wrap ifdef around
your code, the macro will take care of it. 

~Vinod

> +
>  static struct platform_driver cpp41_dma_driver = {
>  	.probe  = cppi41_dma_probe,
>  	.remove = cppi41_dma_remove,
>  	.driver = {
>  		.name = "cppi41-dma-engine",
>  		.owner = THIS_MODULE,
> +		.pm = &cppi41_pm_ops,
>  		.of_match_table = of_match_ptr(cppi41_dma_ids),
>  	},
>  };
> -- 
> 1.8.3.1
> 

-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/5] dma: cppi41: add support for suspend and resume

[Daniel Mack]

On 23.09.2013 06:09, Vinod Koul wrote:
> On Sun, Sep 22, 2013 at 04:50:04PM +0200, Daniel Mack wrote:

>> +#ifdef CONFIG_PM_SLEEP
> ahhhh
> 
>> +static int cppi41_suspend(struct device *dev)
>> +{
>> +	struct cppi41_dd *cdd = dev_get_drvdata(dev);
>> +
>> +	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
>> +	disable_sched(cdd);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cppi41_resume(struct device *dev)
>> +{
>> +	struct cppi41_dd *cdd = dev_get_drvdata(dev);
>> +	int i;
>> +
>> +	for (i = 0; i < DESCS_AREAS; i++)
>> +		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
>> +
>> +	init_sched(cdd);
>> +	cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(cppi41_pm_ops, cppi41_suspend, cppi41_resume);
> Here is the macro in pm.h

[...]

> Now since you are using the macro there should be no need to wrap ifdef around
> your code, the macro will take care of it.

Well yes, which is why I put the macro itself *outside* of the #ifdef
block. Without that #ifdef, however, and with CONFIG_PM_SLEEP unset, I get:

drivers/dma/cppi41.c:1043:12: warning: ‘cppi41_suspend’ defined but not
used [-Wunused-function]
 static int cppi41_suspend(struct device *dev)
            ^
drivers/dma/cppi41.c:1053:12: warning: ‘cppi41_resume’ defined but not
used [-Wunused-function]
 static int cppi41_resume(struct device *dev)
            ^

... which doesn't surprise me much. Or do I still not get your point?


Thanks,
Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/5] dma: cppi41: add support for suspend and resume

[Vinod Koul]

On Mon, Sep 23, 2013 at 07:53:11AM +0200, Daniel Mack wrote:
> On 23.09.2013 06:09, Vinod Koul wrote:
> > On Sun, Sep 22, 2013 at 04:50:04PM +0200, Daniel Mack wrote:
> 
> >> +#ifdef CONFIG_PM_SLEEP
> > ahhhh
> > 
> >> +static int cppi41_suspend(struct device *dev)
> >> +{
> >> +	struct cppi41_dd *cdd = dev_get_drvdata(dev);
> >> +
> >> +	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
> >> +	disable_sched(cdd);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int cppi41_resume(struct device *dev)
> >> +{
> >> +	struct cppi41_dd *cdd = dev_get_drvdata(dev);
> >> +	int i;
> >> +
> >> +	for (i = 0; i < DESCS_AREAS; i++)
> >> +		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
> >> +
> >> +	init_sched(cdd);
> >> +	cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
> >> +
> >> +	return 0;
> >> +}
> >> +#endif
> >> +
> >> +static SIMPLE_DEV_PM_OPS(cppi41_pm_ops, cppi41_suspend, cppi41_resume);
> > Here is the macro in pm.h
> 
> [...]
> 
> > Now since you are using the macro there should be no need to wrap ifdef around
> > your code, the macro will take care of it.
> 
> Well yes, which is why I put the macro itself *outside* of the #ifdef
> block. Without that #ifdef, however, and with CONFIG_PM_SLEEP unset, I get:
> 
> drivers/dma/cppi41.c:1043:12: warning: ‘cppi41_suspend’ defined but not
> used [-Wunused-function]
>  static int cppi41_suspend(struct device *dev)
>             ^
> drivers/dma/cppi41.c:1053:12: warning: ‘cppi41_resume’ defined but not
> used [-Wunused-function]
>  static int cppi41_resume(struct device *dev)
>             ^
> 
> ... which doesn't surprise me much. Or do I still not get your point?
And this is what i had expected... I was thinking we should ignore this, but
this is better too, so I will try to apply this now

~Vinod

-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/5] dma: cppi41: add support for suspend and resume

[Vinod Koul]

On Sun, Sep 22, 2013 at 04:50:04PM +0200, Daniel Mack wrote:
> This patch adds support for suspend/resume functionality to the cppi41
> DMA driver. The steps necessary to make the system resume properly were
> figured out by trial-and-error. The code as it stands now is the
> minimum that has to be done to put the musb host system on an AM33xx
> system into an operable state after resume.
Applied, thanks

~Vinod
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  drivers/dma/cppi41.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> index 3347321..89decc9 100644
> --- a/drivers/dma/cppi41.c
> +++ b/drivers/dma/cppi41.c
> @@ -1040,12 +1040,41 @@ static int cppi41_dma_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int cppi41_suspend(struct device *dev)
> +{
> +	struct cppi41_dd *cdd = dev_get_drvdata(dev);
> +
> +	cppi_writel(0, cdd->usbss_mem + USBSS_IRQ_CLEARR);
> +	disable_sched(cdd);
> +
> +	return 0;
> +}
> +
> +static int cppi41_resume(struct device *dev)
> +{
> +	struct cppi41_dd *cdd = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < DESCS_AREAS; i++)
> +		cppi_writel(cdd->descs_phys, cdd->qmgr_mem + QMGR_MEMBASE(i));
> +
> +	init_sched(cdd);
> +	cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(cppi41_pm_ops, cppi41_suspend, cppi41_resume);
> +
>  static struct platform_driver cpp41_dma_driver = {
>  	.probe  = cppi41_dma_probe,
>  	.remove = cppi41_dma_remove,
>  	.driver = {
>  		.name = "cppi41-dma-engine",
>  		.owner = THIS_MODULE,
> +		.pm = &cppi41_pm_ops,
>  		.of_match_table = of_match_ptr(cppi41_dma_ids),
>  	},
>  };
> -- 
> 1.8.3.1
> 

--