NVIDIA Tegra APB DMA driver fixes and improvements

---
 drivers/dma/tegra20-apb-dma.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/dma/tegra20-apb-dma.c +0 -4

index 3a45079d11ec..319f31d27014 100644
@@ -756,10 +756,6 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	bool was_busy;
 
 	spin_lock_irqsave(&tdc->lock, flags);
-	if (list_empty(&tdc->pending_sg_req)) {
-		spin_unlock_irqrestore(&tdc->lock, flags);
-		return 0;
-	}
 
 	if (!tdc->busy)
 		goto skip_dma_stop;

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:37 UTC #2 lore raw

PATCH v6 2/16 dmaengine: tegra-apb: Prevent race conditions of tasklet vs free list

The interrupt handler puts a half-completed DMA descriptor on a free list and then schedules tasklet to process bottom half of the descriptor that executes client's callback, this creates possibility to pick up the busy descriptor from the free list. Thus, let's disallow descriptor's re-use until it is fully processed.

Cc <[email protected]>

---
 drivers/dma/tegra20-apb-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dma/tegra20-apb-dma.c +1 -1

index 319f31d27014..4a750e29bfb5 100644
@@ -281,7 +281,7 @@ static struct tegra_dma_desc *tegra_dma_desc_get(
 
 	/* Do not allocate if desc are waiting for ack */
 	list_for_each_entry(dma_desc, &tdc->free_dma_desc, node) {
-		if (async_tx_test_ack(&dma_desc->txd)) {
+		if (async_tx_test_ack(&dma_desc->txd) && !dma_desc->cb_count) {
 			list_del(&dma_desc->node);
 			spin_unlock_irqrestore(&tdc->lock, flags);
 			dma_desc->txd.flags = 0;

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:37 UTC #3 lore raw

PATCH v6 3/16 dmaengine: tegra-apb: Implement synchronization hook

The ISR tasklet could be kept scheduled after DMA transfer termination, let's add synchronization hook which blocks until tasklet is finished.

---
 drivers/dma/tegra20-apb-dma.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/dma/tegra20-apb-dma.c +8 -0

index 4a750e29bfb5..f56881500a23 100644
@@ -798,6 +798,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	return 0;
 }
 
+static void tegra_dma_synchronize(struct dma_chan *dc)
+{
+	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
+
+	tasklet_kill(&tdc->tasklet);
+}
+
 static unsigned int tegra_dma_sg_bytes_xferred(struct tegra_dma_channel *tdc,
 					       struct tegra_dma_sg_req *sg_req)
 {

@@ -1506,6 +1513,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
 	tdma->dma_dev.device_config = tegra_dma_slave_config;
 	tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
+	tdma->dma_dev.device_synchronize = tegra_dma_synchronize;
 	tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
 	tdma->dma_dev.device_issue_pending = tegra_dma_issue_pending;

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:37 UTC #4 lore raw

PATCH v6 4/16 dmaengine: tegra-apb: Prevent race conditions on channel's freeing

It's incorrect to check the channel's "busy" state without taking a lock. That shouldn't cause any real troubles, nevertheless it's always better not to have any race conditions in the code.

---
 drivers/dma/tegra20-apb-dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/dma/tegra20-apb-dma.c +1 -2

index f56881500a23..766c2c9eac8e 100644
@@ -1294,8 +1294,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 
 	dev_dbg(tdc2dev(tdc), "Freeing channel %d\n", tdc->id);
 
-	if (tdc->busy)
-		tegra_dma_terminate_all(dc);
+	tegra_dma_terminate_all(dc);
 
 	spin_lock_irqsave(&tdc->lock, flags);
 	list_splice_init(&tdc->pending_sg_req, &sg_req_list);

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:37 UTC #5 lore raw

PATCH v6 5/16 dmaengine: tegra-apb: Clean up tasklet releasing

There is no need to kill tasklet when driver's probe fails because tasklet can't be scheduled at this time. It is also cleaner to kill tasklet on channel's freeing rather than to kill it on driver's removal, otherwise tasklet could perform a dummy execution after channel's releasing, which isn't very nice.

---
 drivers/dma/tegra20-apb-dma.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/dma/tegra20-apb-dma.c +1 -5

index 766c2c9eac8e..aafad50d075e 100644
@@ -1287,7 +1287,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 	struct tegra_dma_sg_req *sg_req;
 	struct list_head dma_desc_list;
 	struct list_head sg_req_list;
-	unsigned long flags;
 
 	INIT_LIST_HEAD(&dma_desc_list);
 	INIT_LIST_HEAD(&sg_req_list);

@@ -1295,15 +1294,14 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 	dev_dbg(tdc2dev(tdc), "Freeing channel %d\n", tdc->id);
 
 	tegra_dma_terminate_all(dc);
+	tasklet_kill(&tdc->tasklet);
 
-	spin_lock_irqsave(&tdc->lock, flags);
 	list_splice_init(&tdc->pending_sg_req, &sg_req_list);
 	list_splice_init(&tdc->free_sg_req, &sg_req_list);
 	list_splice_init(&tdc->free_dma_desc, &dma_desc_list);
 	INIT_LIST_HEAD(&tdc->cb_desc);
 	tdc->config_init = false;
 	tdc->isr_handler = NULL;
-	spin_unlock_irqrestore(&tdc->lock, flags);
 
 	while (!list_empty(&dma_desc_list)) {
 		dma_desc = list_first_entry(&dma_desc_list,

@@ -1542,7 +1540,6 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 
 		free_irq(tdc->irq, tdc);
-		tasklet_kill(&tdc->tasklet);
 	}
 
 	pm_runtime_disable(&pdev->dev);

@@ -1562,7 +1559,6 @@ static int tegra_dma_remove(struct platform_device *pdev)
 	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
 		tdc = &tdma->channels[i];
 		free_irq(tdc->irq, tdc);
-		tasklet_kill(&tdc->tasklet);
 	}
 
 	pm_runtime_disable(&pdev->dev);

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:37 UTC #6 lore raw

PATCH v6 6/16 dmaengine: tegra-apb: Use devm_platform_ioremap_resource

Use devm_platform_ioremap_resource to keep code cleaner a tad.

---
 drivers/dma/tegra20-apb-dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/dma/tegra20-apb-dma.c +1 -2

index aafad50d075e..f44291207928 100644
@@ -1402,8 +1402,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	tdma->chip_data = cdata;
 	platform_set_drvdata(pdev, tdma);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	tdma->base_addr = devm_ioremap_resource(&pdev->dev, res);
+	tdma->base_addr = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(tdma->base_addr))
 		return PTR_ERR(tdma->base_addr);

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:37 UTC #7 lore raw

PATCH v6 7/16 dmaengine: tegra-apb: Use devm_request_irq

Use resource-managed variant of request_irq for brevity.

---
 drivers/dma/tegra20-apb-dma.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

drivers/dma/tegra20-apb-dma.c +11 ~~-24~~

index f44291207928..dff21e80ffa4 100644
@@ -182,7 +182,6 @@ struct tegra_dma_channel {
 	char			name[12];
 	bool			config_init;
 	int			id;
-	int			irq;
 	void __iomem		*chan_addr;
 	spinlock_t		lock;
 	bool			busy;

@@ -1380,7 +1379,6 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
 
 static int tegra_dma_probe(struct platform_device *pdev)
 {
-	struct resource *res;
 	struct tegra_dma *tdma;
 	int ret;
 	int i;

@@ -1446,25 +1444,27 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&tdma->dma_dev.channels);
 	for (i = 0; i < cdata->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
+		int irq;
 
 		tdc->chan_addr = tdma->base_addr +
 				 TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET +
 				 (i * cdata->channel_reg_size);
 
-		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
-		if (!res) {
-			ret = -EINVAL;
+		irq = platform_get_irq(pdev, i);
+		if (irq < 0) {
+			ret = irq;
 			dev_err(&pdev->dev, "No irq resource for chan %d\n", i);
-			goto err_irq;
+			goto err_pm_disable;
 		}
-		tdc->irq = res->start;
+
 		snprintf(tdc->name, sizeof(tdc->name), "apbdma.%d", i);
-		ret = request_irq(tdc->irq, tegra_dma_isr, 0, tdc->name, tdc);
+		ret = devm_request_irq(&pdev->dev, irq, tegra_dma_isr, 0,
+				       tdc->name, tdc);
 		if (ret) {
 			dev_err(&pdev->dev,
 				"request_irq failed with err %d channel %d\n",
 				ret, i);
-			goto err_irq;
+			goto err_pm_disable;
 		}
 
 		tdc->dma_chan.device = &tdma->dma_dev;

@@ -1517,7 +1517,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	if (ret < 0) {
 		dev_err(&pdev->dev,
 			"Tegra20 APB DMA driver registration failed %d\n", ret);
-		goto err_irq;
+		goto err_pm_disable;
 	}
 
 	ret = of_dma_controller_register(pdev->dev.of_node,

@@ -1534,13 +1534,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 
 err_unregister_dma_dev:
 	dma_async_device_unregister(&tdma->dma_dev);
-err_irq:
-	while (--i >= 0) {
-		struct tegra_dma_channel *tdc = &tdma->channels[i];
-
-		free_irq(tdc->irq, tdc);
-	}
-
+err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);

@@ -1550,16 +1544,9 @@ static int tegra_dma_probe(struct platform_device *pdev)
 static int tegra_dma_remove(struct platform_device *pdev)
 {
 	struct tegra_dma *tdma = platform_get_drvdata(pdev);
-	int i;
-	struct tegra_dma_channel *tdc;
 
 	dma_async_device_unregister(&tdma->dma_dev);
 
-	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
-		tdc = &tdma->channels[i];
-		free_irq(tdc->irq, tdc);
-	}
-
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:37 UTC #8 lore raw

PATCH v6 8/16 dmaengine: tegra-apb: Fix coding style problems

This patch fixes few dozens of coding style problems reported by checkpatch and prettifies code where makes sense.

---
 drivers/dma/tegra20-apb-dma.c | 275 ++++++++++++++++++----------------
 1 file changed, 144 insertions(+), 131 deletions(-)

drivers/dma/tegra20-apb-dma.c +144 ~~-131~~

index dff21e80ffa4..b4535b3a07ce 100644
@@ -59,7 +59,7 @@
 #define TEGRA_APBDMA_STATUS_COUNT_MASK		0xFFFC
 
 #define TEGRA_APBDMA_CHAN_CSRE			0x00C
-#define TEGRA_APBDMA_CHAN_CSRE_PAUSE		(1 << 31)
+#define TEGRA_APBDMA_CHAN_CSRE_PAUSE		BIT(31)
 
 /* AHB memory address */
 #define TEGRA_APBDMA_CHAN_AHBPTR		0x010

@@ -120,21 +120,21 @@ struct tegra_dma;
  * @support_separate_wcount_reg: Support separate word count register.
  */
 struct tegra_dma_chip_data {
-	int nr_channels;
-	int channel_reg_size;
-	int max_dma_count;
+	unsigned int nr_channels;
+	unsigned int channel_reg_size;
+	unsigned int max_dma_count;
 	bool support_channel_pause;
 	bool support_separate_wcount_reg;
 };
 
 /* DMA channel registers */
 struct tegra_dma_channel_regs {
-	unsigned long	csr;
-	unsigned long	ahb_ptr;
-	unsigned long	apb_ptr;
-	unsigned long	ahb_seq;
-	unsigned long	apb_seq;
-	unsigned long	wcount;
+	u32 csr;
+	u32 ahb_ptr;
+	u32 apb_ptr;
+	u32 ahb_seq;
+	u32 apb_seq;
+	u32 wcount;
 };
 
 /*

@@ -168,7 +168,7 @@ struct tegra_dma_desc {
 	struct list_head		node;
 	struct list_head		tx_list;
 	struct list_head		cb_node;
-	int				cb_count;
+	unsigned int			cb_count;
 };
 
 struct tegra_dma_channel;

@@ -181,7 +181,7 @@ struct tegra_dma_channel {
 	struct dma_chan		dma_chan;
 	char			name[12];
 	bool			config_init;
-	int			id;
+	unsigned int		id;
 	void __iomem		*chan_addr;
 	spinlock_t		lock;
 	bool			busy;

@@ -201,7 +201,7 @@ struct tegra_dma_channel {
 	/* Channel-slave specific configuration */
 	unsigned int slave_id;
 	struct dma_slave_config dma_sconfig;
-	struct tegra_dma_channel_regs	channel_reg;
+	struct tegra_dma_channel_regs channel_reg;
 };
 
 /* tegra_dma: Tegra DMA specific information */

@@ -239,7 +239,7 @@ static inline u32 tdma_read(struct tegra_dma *tdma, u32 reg)
 }
 
 static inline void tdc_write(struct tegra_dma_channel *tdc,
-		u32 reg, u32 val)
+			     u32 reg, u32 val)
 {
 	writel(val, tdc->chan_addr + reg);
 }

@@ -254,8 +254,8 @@ static inline struct tegra_dma_channel *to_tegra_dma_chan(struct dma_chan *dc)
 	return container_of(dc, struct tegra_dma_channel, dma_chan);
 }
 
-static inline struct tegra_dma_desc *txd_to_tegra_dma_desc(
-		struct dma_async_tx_descriptor *td)
+static inline struct tegra_dma_desc *
+txd_to_tegra_dma_desc(struct dma_async_tx_descriptor *td)
 {
 	return container_of(td, struct tegra_dma_desc, txd);
 }

@@ -270,8 +270,7 @@ static int tegra_dma_runtime_suspend(struct device *dev);
 static int tegra_dma_runtime_resume(struct device *dev);
 
 /* Get DMA desc from free list, if not there then allocate it.  */
-static struct tegra_dma_desc *tegra_dma_desc_get(
-		struct tegra_dma_channel *tdc)
+static struct tegra_dma_desc *tegra_dma_desc_get(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma_desc *dma_desc;
 	unsigned long flags;

@@ -298,11 +297,12 @@ static struct tegra_dma_desc *tegra_dma_desc_get(
 	dma_async_tx_descriptor_init(&dma_desc->txd, &tdc->dma_chan);
 	dma_desc->txd.tx_submit = tegra_dma_tx_submit;
 	dma_desc->txd.flags = 0;
+
 	return dma_desc;
 }
 
 static void tegra_dma_desc_put(struct tegra_dma_channel *tdc,
-		struct tegra_dma_desc *dma_desc)
+			       struct tegra_dma_desc *dma_desc)
 {
 	unsigned long flags;

@@ -313,29 +313,29 @@ static void tegra_dma_desc_put(struct tegra_dma_channel *tdc,
 	spin_unlock_irqrestore(&tdc->lock, flags);
 }
 
-static struct tegra_dma_sg_req *tegra_dma_sg_req_get(
-		struct tegra_dma_channel *tdc)
+static struct tegra_dma_sg_req *
+tegra_dma_sg_req_get(struct tegra_dma_channel *tdc)
 {
-	struct tegra_dma_sg_req *sg_req = NULL;
+	struct tegra_dma_sg_req *sg_req;
 	unsigned long flags;
 
 	spin_lock_irqsave(&tdc->lock, flags);
 	if (!list_empty(&tdc->free_sg_req)) {
-		sg_req = list_first_entry(&tdc->free_sg_req,
-					typeof(*sg_req), node);
+		sg_req = list_first_entry(&tdc->free_sg_req, typeof(*sg_req),
+					  node);
 		list_del(&sg_req->node);
 		spin_unlock_irqrestore(&tdc->lock, flags);
 		return sg_req;
 	}
 	spin_unlock_irqrestore(&tdc->lock, flags);
 
-	sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_NOWAIT);
+	sg_req = kzalloc(sizeof(*sg_req), GFP_NOWAIT);
 
 	return sg_req;
 }
 
 static int tegra_dma_slave_config(struct dma_chan *dc,
-		struct dma_slave_config *sconfig)
+				  struct dma_slave_config *sconfig)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);

@@ -352,11 +352,12 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
 		tdc->slave_id = sconfig->slave_id;
 	}
 	tdc->config_init = true;
+
 	return 0;
 }
 
 static void tegra_dma_global_pause(struct tegra_dma_channel *tdc,
-	bool wait_for_burst_complete)
+				   bool wait_for_burst_complete)
 {
 	struct tegra_dma *tdma = tdc->tdma;

@@ -391,13 +392,13 @@ static void tegra_dma_global_resume(struct tegra_dma_channel *tdc)
 }
 
 static void tegra_dma_pause(struct tegra_dma_channel *tdc,
-	bool wait_for_burst_complete)
+			    bool wait_for_burst_complete)
 {
 	struct tegra_dma *tdma = tdc->tdma;
 
 	if (tdma->chip_data->support_channel_pause) {
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSRE,
-				TEGRA_APBDMA_CHAN_CSRE_PAUSE);
+			  TEGRA_APBDMA_CHAN_CSRE_PAUSE);
 		if (wait_for_burst_complete)
 			udelay(TEGRA_APBDMA_BURST_COMPLETE_TIME);
 	} else {

@@ -409,17 +410,15 @@ static void tegra_dma_resume(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma *tdma = tdc->tdma;
 
-	if (tdma->chip_data->support_channel_pause) {
+	if (tdma->chip_data->support_channel_pause)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSRE, 0);
-	} else {
+	else
 		tegra_dma_global_resume(tdc);
-	}
 }
 
 static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 {
-	u32 csr;
-	u32 status;
+	u32 csr, status;
 
 	/* Disable interrupts */
 	csr = tdc_read(tdc, TEGRA_APBDMA_CHAN_CSR);

@@ -440,7 +439,7 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 }
 
 static void tegra_dma_start(struct tegra_dma_channel *tdc,
-		struct tegra_dma_sg_req *sg_req)
+			    struct tegra_dma_sg_req *sg_req)
 {
 	struct tegra_dma_channel_regs *ch_regs = &sg_req->ch_regs;

@@ -454,11 +453,11 @@ static void tegra_dma_start(struct tegra_dma_channel *tdc,
 
 	/* Start DMA */
 	tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
-				ch_regs->csr | TEGRA_APBDMA_CSR_ENB);
+		  ch_regs->csr | TEGRA_APBDMA_CSR_ENB);
 }
 
 static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
-		struct tegra_dma_sg_req *nsg_req)
+					 struct tegra_dma_sg_req *nsg_req)
 {
 	unsigned long status;

@@ -492,9 +491,9 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
 	tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, nsg_req->ch_regs.ahb_ptr);
 	if (tdc->tdma->chip_data->support_separate_wcount_reg)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
-						nsg_req->ch_regs.wcount);
+			  nsg_req->ch_regs.wcount);
 	tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
-				nsg_req->ch_regs.csr | TEGRA_APBDMA_CSR_ENB);
+		  nsg_req->ch_regs.csr | TEGRA_APBDMA_CSR_ENB);
 	nsg_req->configured = true;
 	nsg_req->words_xferred = 0;

@@ -508,8 +507,7 @@ static void tdc_start_head_req(struct tegra_dma_channel *tdc)
 	if (list_empty(&tdc->pending_sg_req))
 		return;
 
-	sg_req = list_first_entry(&tdc->pending_sg_req,
-					typeof(*sg_req), node);
+	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
 	tegra_dma_start(tdc, sg_req);
 	sg_req->configured = true;
 	sg_req->words_xferred = 0;

@@ -518,34 +516,35 @@ static void tdc_start_head_req(struct tegra_dma_channel *tdc)
 
 static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)
 {
-	struct tegra_dma_sg_req *hsgreq;
-	struct tegra_dma_sg_req *hnsgreq;
+	struct tegra_dma_sg_req *hsgreq, *hnsgreq;
 
 	if (list_empty(&tdc->pending_sg_req))
 		return;
 
 	hsgreq = list_first_entry(&tdc->pending_sg_req, typeof(*hsgreq), node);
 	if (!list_is_last(&hsgreq->node, &tdc->pending_sg_req)) {
-		hnsgreq = list_first_entry(&hsgreq->node,
-					typeof(*hnsgreq), node);
+		hnsgreq = list_first_entry(&hsgreq->node, typeof(*hnsgreq),
+					   node);
 		tegra_dma_configure_for_next(tdc, hnsgreq);
 	}
 }
 
-static inline int get_current_xferred_count(struct tegra_dma_channel *tdc,
-	struct tegra_dma_sg_req *sg_req, unsigned long status)
+static inline unsigned int
+get_current_xferred_count(struct tegra_dma_channel *tdc,
+			  struct tegra_dma_sg_req *sg_req,
+			  unsigned long status)
 {
 	return sg_req->req_len - (status & TEGRA_APBDMA_STATUS_COUNT_MASK) - 4;
 }
 
 static void tegra_dma_abort_all(struct tegra_dma_channel *tdc)
 {
-	struct tegra_dma_sg_req *sgreq;
 	struct tegra_dma_desc *dma_desc;
+	struct tegra_dma_sg_req *sgreq;
 
 	while (!list_empty(&tdc->pending_sg_req)) {
-		sgreq = list_first_entry(&tdc->pending_sg_req,
-						typeof(*sgreq), node);
+		sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq),
+					 node);
 		list_move_tail(&sgreq->node, &tdc->free_sg_req);
 		if (sgreq->last_sg) {
 			dma_desc = sgreq->dma_desc;

@@ -555,7 +554,7 @@ static void tegra_dma_abort_all(struct tegra_dma_channel *tdc)
 			/* Add in cb list if it is not there. */
 			if (!dma_desc->cb_count)
 				list_add_tail(&dma_desc->cb_node,
-							&tdc->cb_desc);
+					      &tdc->cb_desc);
 			dma_desc->cb_count++;
 		}
 	}

@@ -563,9 +562,10 @@ static void tegra_dma_abort_all(struct tegra_dma_channel *tdc)
 }
 
 static bool handle_continuous_head_request(struct tegra_dma_channel *tdc,
-		struct tegra_dma_sg_req *last_sg_req, bool to_terminate)
+					   struct tegra_dma_sg_req *last_sg_req,
+					   bool to_terminate)
 {
-	struct tegra_dma_sg_req *hsgreq = NULL;
+	struct tegra_dma_sg_req *hsgreq;
 
 	if (list_empty(&tdc->pending_sg_req)) {
 		dev_err(tdc2dev(tdc), "DMA is running without req\n");

@@ -589,14 +589,15 @@ static bool handle_continuous_head_request(struct tegra_dma_channel *tdc,
 	/* Configure next request */
 	if (!to_terminate)
 		tdc_configure_next_head_desc(tdc);
+
 	return true;
 }
 
 static void handle_once_dma_done(struct tegra_dma_channel *tdc,
-	bool to_terminate)
+				 bool to_terminate)
 {
-	struct tegra_dma_sg_req *sgreq;
 	struct tegra_dma_desc *dma_desc;
+	struct tegra_dma_sg_req *sgreq;
 
 	tdc->busy = false;
 	sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq), node);

@@ -622,10 +623,10 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
 }
 
 static void handle_cont_sngl_cycle_dma_done(struct tegra_dma_channel *tdc,
-		bool to_terminate)
+					    bool to_terminate)
 {
-	struct tegra_dma_sg_req *sgreq;
 	struct tegra_dma_desc *dma_desc;
+	struct tegra_dma_sg_req *sgreq;
 	bool st;
 
 	sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq), node);

@@ -657,13 +658,13 @@ static void tegra_dma_tasklet(unsigned long data)
 	struct tegra_dma_channel *tdc = (struct tegra_dma_channel *)data;
 	struct dmaengine_desc_callback cb;
 	struct tegra_dma_desc *dma_desc;
+	unsigned int cb_count;
 	unsigned long flags;
-	int cb_count;
 
 	spin_lock_irqsave(&tdc->lock, flags);
 	while (!list_empty(&tdc->cb_desc)) {
-		dma_desc  = list_first_entry(&tdc->cb_desc,
-					typeof(*dma_desc), cb_node);
+		dma_desc = list_first_entry(&tdc->cb_desc, typeof(*dma_desc),
+					    cb_node);
 		list_del(&dma_desc->cb_node);
 		dmaengine_desc_get_callback(&dma_desc->txd, &cb);
 		cb_count = dma_desc->cb_count;

@@ -681,8 +682,8 @@ static void tegra_dma_tasklet(unsigned long data)
 static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
 {
 	struct tegra_dma_channel *tdc = dev_id;
-	unsigned long status;
 	unsigned long flags;
+	u32 status;
 
 	spin_lock_irqsave(&tdc->lock, flags);

@@ -697,8 +698,9 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
 	}
 
 	spin_unlock_irqrestore(&tdc->lock, flags);
-	dev_info(tdc2dev(tdc),
-		"Interrupt already served status 0x%08lx\n", status);
+	dev_info(tdc2dev(tdc), "Interrupt already served status 0x%08x\n",
+		 status);
+
 	return IRQ_NONE;
 }

@@ -714,6 +716,7 @@ static dma_cookie_t tegra_dma_tx_submit(struct dma_async_tx_descriptor *txd)
 	cookie = dma_cookie_assign(&dma_desc->txd);
 	list_splice_tail_init(&dma_desc->tx_list, &tdc->pending_sg_req);
 	spin_unlock_irqrestore(&tdc->lock, flags);
+
 	return cookie;
 }

@@ -747,11 +750,10 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
 static int tegra_dma_terminate_all(struct dma_chan *dc)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
-	struct tegra_dma_sg_req *sgreq;
 	struct tegra_dma_desc *dma_desc;
+	struct tegra_dma_sg_req *sgreq;
 	unsigned long flags;
-	unsigned long status;
-	unsigned long wcount;
+	u32 status, wcount;
 	bool was_busy;
 
 	spin_lock_irqsave(&tdc->lock, flags);

@@ -777,8 +779,8 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	tegra_dma_stop(tdc);
 
 	if (!list_empty(&tdc->pending_sg_req) && was_busy) {
-		sgreq = list_first_entry(&tdc->pending_sg_req,
-					typeof(*sgreq), node);
+		sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq),
+					 node);
 		sgreq->dma_desc->bytes_transferred +=
 				get_current_xferred_count(tdc, sgreq, wcount);
 	}

@@ -788,12 +790,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	tegra_dma_abort_all(tdc);
 
 	while (!list_empty(&tdc->cb_desc)) {
-		dma_desc  = list_first_entry(&tdc->cb_desc,
-					typeof(*dma_desc), cb_node);
+		dma_desc = list_first_entry(&tdc->cb_desc, typeof(*dma_desc),
+					    cb_node);
 		list_del(&dma_desc->cb_node);
 		dma_desc->cb_count = 0;
 	}
 	spin_unlock_irqrestore(&tdc->lock, flags);
+
 	return 0;
 }

@@ -807,7 +810,7 @@ static void tegra_dma_synchronize(struct dma_chan *dc)
 static unsigned int tegra_dma_sg_bytes_xferred(struct tegra_dma_channel *tdc,
 					       struct tegra_dma_sg_req *sg_req)
 {
-	unsigned long status, wcount = 0;
+	u32 status, wcount = 0;
 
 	if (!list_is_first(&sg_req->node, &tdc->pending_sg_req))
 		return 0;

@@ -864,7 +867,8 @@ static unsigned int tegra_dma_sg_bytes_xferred(struct tegra_dma_channel *tdc,
 }
 
 static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
-	dma_cookie_t cookie, struct dma_tx_state *txstate)
+					   dma_cookie_t cookie,
+					   struct dma_tx_state *txstate)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
 	struct tegra_dma_desc *dma_desc;

@@ -911,11 +915,12 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 
 	trace_tegra_dma_tx_status(&tdc->dma_chan, cookie, txstate);
 	spin_unlock_irqrestore(&tdc->lock, flags);
+
 	return ret;
 }
 
-static inline int get_bus_width(struct tegra_dma_channel *tdc,
-		enum dma_slave_buswidth slave_bw)
+static inline unsigned int get_bus_width(struct tegra_dma_channel *tdc,
+					 enum dma_slave_buswidth slave_bw)
 {
 	switch (slave_bw) {
 	case DMA_SLAVE_BUSWIDTH_1_BYTE:

@@ -928,16 +933,17 @@ static inline int get_bus_width(struct tegra_dma_channel *tdc,
 		return TEGRA_APBDMA_APBSEQ_BUS_WIDTH_64;
 	default:
 		dev_warn(tdc2dev(tdc),
-			"slave bw is not supported, using 32bits\n");
+			 "slave bw is not supported, using 32bits\n");
 		return TEGRA_APBDMA_APBSEQ_BUS_WIDTH_32;
 	}
 }
 
-static inline int get_burst_size(struct tegra_dma_channel *tdc,
-	u32 burst_size, enum dma_slave_buswidth slave_bw, int len)
+static inline unsigned int get_burst_size(struct tegra_dma_channel *tdc,
+					  u32 burst_size,
+					  enum dma_slave_buswidth slave_bw,
+					  u32 len)
 {
-	int burst_byte;
-	int burst_ahb_width;
+	unsigned int burst_byte, burst_ahb_width;
 
 	/*
 	 * burst_size from client is in terms of the bus_width.

@@ -964,9 +970,12 @@ static inline int get_burst_size(struct tegra_dma_channel *tdc,
 }
 
 static int get_transfer_param(struct tegra_dma_channel *tdc,
-	enum dma_transfer_direction direction, unsigned long *apb_addr,
-	unsigned long *apb_seq,	unsigned long *csr, unsigned int *burst_size,
-	enum dma_slave_buswidth *slave_bw)
+			      enum dma_transfer_direction direction,
+			      u32 *apb_addr,
+			      u32 *apb_seq,
+			      u32 *csr,
+			      unsigned int *burst_size,
+			      enum dma_slave_buswidth *slave_bw)
 {
 	switch (direction) {
 	case DMA_MEM_TO_DEV:

@@ -987,13 +996,15 @@ static int get_transfer_param(struct tegra_dma_channel *tdc,
 
 	default:
 		dev_err(tdc2dev(tdc), "DMA direction is not supported\n");
-		return -EINVAL;
+		break;
 	}
+
 	return -EINVAL;
 }
 
 static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
-	struct tegra_dma_channel_regs *ch_regs, u32 len)
+				  struct tegra_dma_channel_regs *ch_regs,
+				  u32 len)
 {
 	u32 len_field = (len - 4) & 0xFFFC;

@@ -1003,20 +1014,23 @@ static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
 		ch_regs->csr |= len_field;
 }
 
-static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
-	struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
-	enum dma_transfer_direction direction, unsigned long flags,
-	void *context)
+static struct dma_async_tx_descriptor *
+tegra_dma_prep_slave_sg(struct dma_chan *dc,
+			struct scatterlist *sgl,
+			unsigned int sg_len,
+			enum dma_transfer_direction direction,
+			unsigned long flags,
+			void *context)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
+	struct tegra_dma_sg_req *sg_req = NULL;
+	u32 csr, ahb_seq, apb_ptr, apb_seq;
+	enum dma_slave_buswidth slave_bw;
 	struct tegra_dma_desc *dma_desc;
-	unsigned int i;
-	struct scatterlist *sg;
-	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
 	struct list_head req_list;
-	struct tegra_dma_sg_req  *sg_req = NULL;
-	u32 burst_size;
-	enum dma_slave_buswidth slave_bw;
+	struct scatterlist *sg;
+	unsigned int burst_size;
+	unsigned int i;
 
 	if (!tdc->config_init) {
 		dev_err(tdc2dev(tdc), "DMA channel is not configured\n");

@@ -1028,7 +1042,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
 	}
 
 	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
-				&burst_size, &slave_bw) < 0)
+			       &burst_size, &slave_bw) < 0)
 		return NULL;
 
 	INIT_LIST_HEAD(&req_list);

@@ -1074,7 +1088,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
 		len = sg_dma_len(sg);
 
 		if ((len & 3) || (mem & 3) ||
-				(len > tdc->tdma->chip_data->max_dma_count)) {
+		    len > tdc->tdma->chip_data->max_dma_count) {
 			dev_err(tdc2dev(tdc),
 				"DMA length/memory address is not supported\n");
 			tegra_dma_desc_put(tdc, dma_desc);

@@ -1126,20 +1140,21 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
 	return &dma_desc->txd;
 }
 
-static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
-	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
-	size_t period_len, enum dma_transfer_direction direction,
-	unsigned long flags)
+static struct dma_async_tx_descriptor *
+tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
+			  size_t buf_len,
+			  size_t period_len,
+			  enum dma_transfer_direction direction,
+			  unsigned long flags)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
-	struct tegra_dma_desc *dma_desc = NULL;
 	struct tegra_dma_sg_req *sg_req = NULL;
-	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
-	int len;
-	size_t remain_len;
-	dma_addr_t mem = buf_addr;
-	u32 burst_size;
+	u32 csr, ahb_seq, apb_ptr, apb_seq;
 	enum dma_slave_buswidth slave_bw;
+	struct tegra_dma_desc *dma_desc;
+	dma_addr_t mem = buf_addr;
+	unsigned int burst_size;
+	size_t len, remain_len;
 
 	if (!buf_len || !period_len) {
 		dev_err(tdc2dev(tdc), "Invalid buffer/period len\n");

@@ -1173,13 +1188,13 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
 
 	len = period_len;
 	if ((len & 3) || (buf_addr & 3) ||
-			(len > tdc->tdma->chip_data->max_dma_count)) {
+	    len > tdc->tdma->chip_data->max_dma_count) {
 		dev_err(tdc2dev(tdc), "Req len/mem address is not correct\n");
 		return NULL;
 	}
 
 	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
-				&burst_size, &slave_bw) < 0)
+			       &burst_size, &slave_bw) < 0)
 		return NULL;
 
 	ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;

@@ -1303,8 +1318,8 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 	tdc->isr_handler = NULL;
 
 	while (!list_empty(&dma_desc_list)) {
-		dma_desc = list_first_entry(&dma_desc_list,
-					typeof(*dma_desc), node);
+		dma_desc = list_first_entry(&dma_desc_list, typeof(*dma_desc),
+					    node);
 		list_del(&dma_desc->node);
 		kfree(dma_desc);
 	}

@@ -1323,8 +1338,8 @@ static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
 					   struct of_dma *ofdma)
 {
 	struct tegra_dma *tdma = ofdma->of_dma_data;
-	struct dma_chan *chan;
 	struct tegra_dma_channel *tdc;
+	struct dma_chan *chan;
 
 	if (dma_spec->args[0] > TEGRA_APBDMA_CSR_REQ_SEL_MASK) {
 		dev_err(tdma->dev, "Invalid slave id: %d\n", dma_spec->args[0]);

@@ -1379,20 +1394,16 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
 
 static int tegra_dma_probe(struct platform_device *pdev)
 {
+	const struct tegra_dma_chip_data *cdata;
 	struct tegra_dma *tdma;
+	unsigned int i;
+	size_t size;
 	int ret;
-	int i;
-	const struct tegra_dma_chip_data *cdata;
 
 	cdata = of_device_get_match_data(&pdev->dev);
-	if (!cdata) {
-		dev_err(&pdev->dev, "Error: No device match data found\n");
-		return -ENODEV;
-	}
+	size = struct_size(tdma, channels, cdata->nr_channels);
 
-	tdma = devm_kzalloc(&pdev->dev,
-			    struct_size(tdma, channels, cdata->nr_channels),
-			    GFP_KERNEL);
+	tdma = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
 	if (!tdma)
 		return -ENOMEM;

@@ -1424,10 +1435,8 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	else
 		ret = pm_runtime_get_sync(&pdev->dev);
 
-	if (ret < 0) {
-		pm_runtime_disable(&pdev->dev);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_pm_disable;
 
 	/* Reset DMA controller */
 	reset_control_assert(tdma->rst);

@@ -1470,13 +1479,13 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		tdc->dma_chan.device = &tdma->dma_dev;
 		dma_cookie_init(&tdc->dma_chan);
 		list_add_tail(&tdc->dma_chan.device_node,
-				&tdma->dma_dev.channels);
+			      &tdma->dma_dev.channels);
 		tdc->tdma = tdma;
 		tdc->id = i;
 		tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
 
 		tasklet_init(&tdc->tasklet, tegra_dma_tasklet,
-				(unsigned long)tdc);
+			     (unsigned long)tdc);
 		spin_lock_init(&tdc->lock);
 
 		INIT_LIST_HEAD(&tdc->pending_sg_req);

@@ -1528,16 +1537,19 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		goto err_unregister_dma_dev;
 	}
 
-	dev_info(&pdev->dev, "Tegra20 APB DMA driver register %d channels\n",
-			cdata->nr_channels);
+	dev_info(&pdev->dev, "Tegra20 APB DMA driver registered %u channels\n",
+		 cdata->nr_channels);
+
 	return 0;
 
 err_unregister_dma_dev:
 	dma_async_device_unregister(&tdma->dma_dev);
+
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);
+
 	return ret;
 }

@@ -1557,7 +1569,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 static int tegra_dma_runtime_suspend(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	int i;
+	unsigned int i;
 
 	tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL);
 	for (i = 0; i < tdma->chip_data->nr_channels; i++) {

@@ -1586,7 +1598,8 @@ static int tegra_dma_runtime_suspend(struct device *dev)
 static int tegra_dma_runtime_resume(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	int i, ret;
+	unsigned int i;
+	int ret;
 
 	ret = clk_prepare_enable(tdma->dma_clk);
 	if (ret < 0) {

@@ -1614,7 +1627,7 @@ static int tegra_dma_runtime_resume(struct device *dev)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBSEQ, ch_reg->ahb_seq);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, ch_reg->ahb_ptr);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
-			(ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB));
+			  ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB);
 	}
 
 	return 0;

Signature · 1 line

2.24.0

Jon Hunter

maintainer Acked-by on v6 8/16

2020-01-30 14:08 UTC #9 lore raw

Re: [PATCH v6 08/16] dmaengine: tegra-apb: Fix coding style problems

Quoted 7 lines · Dmitry Osipenko

This patch fixes few dozens of coding style problems reported by checkpatch and prettifies code where makes sense.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/dma/tegra20-apb-dma.c | 275 ++++++++++++++++++----------------
 1 file changed, 144 insertions(+), 131 deletions(-)

Thanks!

Cheers
Jon

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-01-30 04:37 UTC #10 lore raw

PATCH v6 9/16 dmaengine: tegra-apb: Remove unneeded initialization of tdc->config_init

There is no need to re-initialize the already initialized variables. The tdc->config_init=false after driver's probe and after channel's freeing, so there is no need to re-initialize it on the channel's allocation.

---
 drivers/dma/tegra20-apb-dma.c | 1 -
 1 file changed, 1 deletion(-)

drivers/dma/tegra20-apb-dma.c +0 -1

index b4535b3a07ce..7158bd3145c4 100644
@@ -1284,7 +1284,6 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
 	int ret;
 
 	dma_cookie_init(&tdc->dma_chan);
-	tdc->config_init = false;
 
 	ret = pm_runtime_get_sync(tdma->dev);
 	if (ret < 0)

Signature · 1 line

2.24.0

Jon Hunter

maintainer Acked-by on v6 9/16

2020-01-30 14:08 UTC #11 lore raw

Re: [PATCH v6 09/16] dmaengine: tegra-apb: Remove unneeded initialization of tdc->config_init

Quoted 16 lines · Dmitry Osipenko

There is no need to re-initialize the already initialized variables. The tdc->config_init=false after driver's probe and after channel's freeing, so there is no need to re-initialize it on the channel's allocation.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/dma/tegra20-apb-dma.c | 1 -
 1 file changed, 1 deletion(-)
drivers/dma/tegra20-apb-dma.c +0 -0
index b4535b3a07ce..7158bd3145c4 100644

drivers/dma/tegra20-apb-dma.c static int tegra_dma_alloc_chan_resources(struct dma_chan *dc) · line 1289

Quoted 7 lines

drivers/dma/tegra20-apb-dma.c +0 -1

@@ -1284,7 +1284,6 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
 	int ret;
 
 	dma_cookie_init(&tdc->dma_chan);
-	tdc->config_init = false;
 
 	ret = pm_runtime_get_sync(tdma->dev);
 	if (ret < 0)

Cheers
Jon

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-01-30 04:37 UTC #12 lore raw

PATCH v6 10/16 dmaengine: tegra-apb: Remove assumptions about unavailable runtime PM

The runtime PM is always available on all Tegra SoCs since the commit 40b2bb1b132a ("ARM: tegra: enforce PM requirement"), so there is no need to handle the case of unavailable RPM in the code anymore.

---
 drivers/dma/tegra20-apb-dma.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

drivers/dma/tegra20-apb-dma.c +1 -9

index 7158bd3145c4..22b88ccff05d 100644
@@ -1429,11 +1429,8 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	spin_lock_init(&tdma->global_lock);
 
 	pm_runtime_enable(&pdev->dev);
-	if (!pm_runtime_enabled(&pdev->dev))
-		ret = tegra_dma_runtime_resume(&pdev->dev);
-	else
-		ret = pm_runtime_get_sync(&pdev->dev);
 
+	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0)
 		goto err_pm_disable;

@@ -1546,8 +1543,6 @@ static int tegra_dma_probe(struct platform_device *pdev)
 
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
-	if (!pm_runtime_status_suspended(&pdev->dev))
-		tegra_dma_runtime_suspend(&pdev->dev);
 
 	return ret;
 }

@@ -1557,10 +1552,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 	struct tegra_dma *tdma = platform_get_drvdata(pdev);
 
 	dma_async_device_unregister(&tdma->dma_dev);
-
 	pm_runtime_disable(&pdev->dev);
-	if (!pm_runtime_status_suspended(&pdev->dev))
-		tegra_dma_runtime_suspend(&pdev->dev);
 
 	return 0;
 }

Signature · 1 line

2.24.0

Jon Hunter

maintainer

2020-01-30 14:09 UTC #13 lore raw

Re: [PATCH v6 10/16] dmaengine: tegra-apb: Remove assumptions about unavailable runtime PM

Quoted 36 lines · Dmitry Osipenko

---
 drivers/dma/tegra20-apb-dma.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

drivers/dma/tegra20-apb-dma.c +1 -6

index 7158bd3145c4..22b88ccff05d 100644
@@ -1429,11 +1429,8 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	spin_lock_init(&tdma->global_lock);
 
 	pm_runtime_enable(&pdev->dev);
-	if (!pm_runtime_enabled(&pdev->dev))
-		ret = tegra_dma_runtime_resume(&pdev->dev);
-	else
-		ret = pm_runtime_get_sync(&pdev->dev);
 
+	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0)
 		goto err_pm_disable;

@@ -1546,8 +1543,6 @@ static int tegra_dma_probe(struct platform_device *pdev)
 
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
-	if (!pm_runtime_status_suspended(&pdev->dev))
-		tegra_dma_runtime_suspend(&pdev->dev);
 
 	return ret;
 }

drivers/dma/tegra20-apb-dma.c static int tegra_dma_remove(struct platform_device *pdev) · line 1558

Quoted 10 lines

drivers/dma/tegra20-apb-dma.c +0 -3

@@ -1557,10 +1552,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 	struct tegra_dma *tdma = platform_get_drvdata(pdev);
 
 	dma_async_device_unregister(&tdma->dma_dev);
-
 	pm_runtime_disable(&pdev->dev);
-	if (!pm_runtime_status_suspended(&pdev->dev))
-		tegra_dma_runtime_suspend(&pdev->dev);
 
 	return 0;
 }

I wonder if we need to make the pm_runtime_put a pm_runtime_put_sync or call pm_runtime_barrier() here, to ensure it has called the callback when freeing the channel? Otherwise this is fine.

Jon

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-01-30 16:09 UTC #14 lore raw

Re: [PATCH v6 10/16] dmaengine: tegra-apb: Remove assumptions about unavailable runtime PM

30.01.2020 17:09, Jon Hunter пишет:

Quoted 46 lines · Dmitry Osipenko

Quoted 44 lines

https://elixir.bootlin.com/linux/v5.5/source/drivers/base/power/runtime.c#L1360

---
 drivers/dma/tegra20-apb-dma.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

drivers/dma/tegra20-apb-dma.c +1 -9

index 7158bd3145c4..22b88ccff05d 100644
@@ -1429,11 +1429,8 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	spin_lock_init(&tdma->global_lock);
 
 	pm_runtime_enable(&pdev->dev);
-	if (!pm_runtime_enabled(&pdev->dev))
-		ret = tegra_dma_runtime_resume(&pdev->dev);
-	else
-		ret = pm_runtime_get_sync(&pdev->dev);
 
+	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0)
 		goto err_pm_disable;

@@ -1546,8 +1543,6 @@ static int tegra_dma_probe(struct platform_device *pdev)
 
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
-	if (!pm_runtime_status_suspended(&pdev->dev))
-		tegra_dma_runtime_suspend(&pdev->dev);
 
 	return ret;
 }

@@ -1557,10 +1552,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 	struct tegra_dma *tdma = platform_get_drvdata(pdev);
 
 	dma_async_device_unregister(&tdma->dma_dev);
-
 	pm_runtime_disable(&pdev->dev);
-	if (!pm_runtime_status_suspended(&pdev->dev))
-		tegra_dma_runtime_suspend(&pdev->dev);

Quoted 6 lines

drivers/dma/tegra20-apb-dma.c static int tegra_dma_remove(struct platform_device *pdev) · line 1558
Quoted 2 lines
drivers/dma/tegra20-apb-dma.c +0 -0
 	return 0;
 }
I wonder if we need to make the pm_runtime_put a pm_runtime_put_sync or call pm_runtime_barrier() here, to ensure it has called the callback when freeing the channel? Otherwise this is fine.

The explicit RPM syncing shouldn't be needed because pm_runtime_disable() takes care of doing that for us, please take a look at:

Dmitry Osipenko

author

2020-01-30 04:37 UTC #15 lore raw

PATCH v6 11/16 dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

It's a bit impractical to enable hardware's clock at the time of DMA channel's allocation because most of DMA client drivers allocate DMA channel at the time of the driver's probing, and thus, DMA clock is kept always-enabled in practice, defeating the whole purpose of runtime PM.

---
 drivers/dma/tegra20-apb-dma.c | 47 ++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 15 deletions(-)

drivers/dma/tegra20-apb-dma.c +32 ~~-15~~

index 22b88ccff05d..0ee28d8e3c96 100644
@@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
 	}
 	tdc->busy = false;
+
+	pm_runtime_put(tdc->tdma->dev);
 }
 
 static void tegra_dma_start(struct tegra_dma_channel *tdc,

@@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
 	tegra_dma_resume(tdc);
 }
 
-static void tdc_start_head_req(struct tegra_dma_channel *tdc)
+static bool tdc_start_head_req(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma_sg_req *sg_req;
+	int err;
 
 	if (list_empty(&tdc->pending_sg_req))
-		return;
+		return false;
+
+	err = pm_runtime_get_sync(tdc->tdma->dev);
+	if (WARN_ON_ONCE(err < 0))
+		return false;
 
 	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
 	tegra_dma_start(tdc, sg_req);
 	sg_req->configured = true;
 	sg_req->words_xferred = 0;
 	tdc->busy = true;
+
+	return true;
 }
 
 static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)

@@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
 	}
 	list_add_tail(&sgreq->node, &tdc->free_sg_req);
 
+	pm_runtime_put(tdc->tdma->dev);
+
 	/* Do not start DMA if it is going to be terminate */
 	if (to_terminate || list_empty(&tdc->pending_sg_req))
 		return;

@@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
 		dev_err(tdc2dev(tdc), "No DMA request\n");
 		goto end;
 	}
-	if (!tdc->busy) {
-		tdc_start_head_req(tdc);
-
+	if (!tdc->busy && tdc_start_head_req(tdc)) {
 		/* Continuous single mode: Configure next req */
 		if (tdc->cyclic) {
 			/*

@@ -775,6 +784,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	else
 		wcount = status;
 
+	/*
+	 * tegra_dma_stop() will drop the RPM's usage refcount, but
+	 * tegra_dma_resume() touches hardware and thus we should keep
+	 * the DMA clock active while it's needed.
+	 */
+	pm_runtime_get(tdc->tdma->dev);
+
 	was_busy = tdc->busy;
 	tegra_dma_stop(tdc);

@@ -786,6 +802,8 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	}
 	tegra_dma_resume(tdc);
 
+	pm_runtime_put(tdc->tdma->dev);
+
 skip_dma_stop:
 	tegra_dma_abort_all(tdc);

@@ -1280,22 +1298,15 @@ tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
 static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
-	struct tegra_dma *tdma = tdc->tdma;
-	int ret;
 
 	dma_cookie_init(&tdc->dma_chan);
 
-	ret = pm_runtime_get_sync(tdma->dev);
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
 static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
-	struct tegra_dma *tdma = tdc->tdma;
 	struct tegra_dma_desc *dma_desc;
 	struct tegra_dma_sg_req *sg_req;
 	struct list_head dma_desc_list;

@@ -1328,7 +1339,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 		list_del(&sg_req->node);
 		kfree(sg_req);
 	}
-	pm_runtime_put(tdma->dev);
 
 	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
 }

@@ -1428,6 +1438,11 @@ static int tegra_dma_probe(struct platform_device *pdev)
 
 	spin_lock_init(&tdma->global_lock);
 
+	ret = clk_prepare(tdma->dma_clk);
+	if (ret)
+		return ret;
+
+	pm_runtime_irq_safe(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
 	ret = pm_runtime_get_sync(&pdev->dev);

@@ -1543,6 +1558,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
+	clk_unprepare(tdma->dma_clk);
 
 	return ret;
 }

@@ -1553,6 +1569,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 
 	dma_async_device_unregister(&tdma->dma_dev);
 	pm_runtime_disable(&pdev->dev);
+	clk_unprepare(tdma->dma_clk);
 
 	return 0;
 }

@@ -1581,7 +1598,7 @@ static int tegra_dma_runtime_suspend(struct device *dev)
 						  TEGRA_APBDMA_CHAN_WCOUNT);
 	}
 
-	clk_disable_unprepare(tdma->dma_clk);
+	clk_disable(tdma->dma_clk);
 
 	return 0;
 }

@@ -1592,7 +1609,7 @@ static int tegra_dma_runtime_resume(struct device *dev)
 	unsigned int i;
 	int ret;
 
-	ret = clk_prepare_enable(tdma->dma_clk);
+	ret = clk_enable(tdma->dma_clk);
 	if (ret < 0) {
 		dev_err(dev, "clk_enable failed: %d\n", ret);
 		return ret;

Signature · 1 line

2.24.0

Jon Hunter

maintainer

2020-01-30 14:09 UTC #16 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

Quoted 72 lines · Dmitry Osipenko

---
 drivers/dma/tegra20-apb-dma.c | 47 ++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 15 deletions(-)

drivers/dma/tegra20-apb-dma.c +14 -5

index 22b88ccff05d..0ee28d8e3c96 100644
@@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
 	}
 	tdc->busy = false;
+
+	pm_runtime_put(tdc->tdma->dev);
 }
 
 static void tegra_dma_start(struct tegra_dma_channel *tdc,

@@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
 	tegra_dma_resume(tdc);
 }
 
-static void tdc_start_head_req(struct tegra_dma_channel *tdc)
+static bool tdc_start_head_req(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma_sg_req *sg_req;
+	int err;
 
 	if (list_empty(&tdc->pending_sg_req))
-		return;
+		return false;
+
+	err = pm_runtime_get_sync(tdc->tdma->dev);
+	if (WARN_ON_ONCE(err < 0))
+		return false;
 
 	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
 	tegra_dma_start(tdc, sg_req);
 	sg_req->configured = true;
 	sg_req->words_xferred = 0;
 	tdc->busy = true;
+
+	return true;
 }
 
 static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)

@@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
 	}
 	list_add_tail(&sgreq->node, &tdc->free_sg_req);
 
+	pm_runtime_put(tdc->tdma->dev);
+
 	/* Do not start DMA if it is going to be terminate */
 	if (to_terminate || list_empty(&tdc->pending_sg_req))
 		return;

@@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
 		dev_err(tdc2dev(tdc), "No DMA request\n");
 		goto end;
 	}
-	if (!tdc->busy) {
-		tdc_start_head_req(tdc);
-
+	if (!tdc->busy && tdc_start_head_req(tdc)) {
 		/* Continuous single mode: Configure next req */
 		if (tdc->cyclic) {
 			/*

drivers/dma/tegra20-apb-dma.c static int tegra_dma_terminate_all(struct dma_chan *dc) · line 793

Quoted 10 lines

drivers/dma/tegra20-apb-dma.c +7 -0

@@ -775,6 +784,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	else
 		wcount = status;
 
+	/*
+	 * tegra_dma_stop() will drop the RPM's usage refcount, but
+	 * tegra_dma_resume() touches hardware and thus we should keep
+	 * the DMA clock active while it's needed.
+	 */
+	pm_runtime_get(tdc->tdma->dev);
+

Would it work and make it simpler to just enable in the issue_pending and disable in the handle_once_dma_done or terminate_all?

drivers/dma/tegra20-apb-dma.c +0 -0

index 3a45079d11ec..86bbb45da93d 100644
@@ -616,9 +616,14 @@ static void handle_once_dma_done(struct

tegra_dma_channel *tdc,

        list_add_tail(&sgreq->node, &tdc->free_sg_req);

        /* Do not start DMA if it is going to be terminate */
-       if (to_terminate || list_empty(&tdc->pending_sg_req))
+       if (to_terminate)
                return;

+ if (list_empty(&tdc->pending_sg_req)) {
+ pm_runtime_put(tdc->tdma->dev);
+ return;
+ }
+

        tdc_start_head_req(tdc);

}

@@ -729,6 +734,11 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)

                goto end;
        }
        if (!tdc->busy) {

+ if (pm_runtime_get_sync(tdc->tdma->dev) < 0) {
+ dev_err(tdc2dev(tdc), "Failed to enable DMA!\n");
+ goto end;
+ }
+

                tdc_start_head_req(tdc);

                /* Continuous single mode: Configure next req */
@@ -788,6 +798,7 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
                                get_current_xferred_count(tdc, sgreq,
wcount);
        }
        tegra_dma_resume(tdc);

+ pm_runtime_put(tdc->tdma->dev);

skip_dma_stop:

        tegra_dma_abort_all(tdc);

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-01-30 16:11 UTC #17 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

30.01.2020 17:09, Jon Hunter пишет:

Quoted 130 lines · Dmitry Osipenko

Quoted 72 lines

---
 drivers/dma/tegra20-apb-dma.c | 47 ++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 15 deletions(-)

drivers/dma/tegra20-apb-dma.c +14 -5

index 22b88ccff05d..0ee28d8e3c96 100644
@@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
 	}
 	tdc->busy = false;
+
+	pm_runtime_put(tdc->tdma->dev);
 }
 
 static void tegra_dma_start(struct tegra_dma_channel *tdc,

@@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
 	tegra_dma_resume(tdc);
 }
 
-static void tdc_start_head_req(struct tegra_dma_channel *tdc)
+static bool tdc_start_head_req(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma_sg_req *sg_req;
+	int err;
 
 	if (list_empty(&tdc->pending_sg_req))
-		return;
+		return false;
+
+	err = pm_runtime_get_sync(tdc->tdma->dev);
+	if (WARN_ON_ONCE(err < 0))
+		return false;
 
 	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
 	tegra_dma_start(tdc, sg_req);
 	sg_req->configured = true;
 	sg_req->words_xferred = 0;
 	tdc->busy = true;
+
+	return true;
 }
 
 static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)

@@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
 	}
 	list_add_tail(&sgreq->node, &tdc->free_sg_req);
 
+	pm_runtime_put(tdc->tdma->dev);
+
 	/* Do not start DMA if it is going to be terminate */
 	if (to_terminate || list_empty(&tdc->pending_sg_req))
 		return;

@@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
 		dev_err(tdc2dev(tdc), "No DMA request\n");
 		goto end;
 	}
-	if (!tdc->busy) {
-		tdc_start_head_req(tdc);
-
+	if (!tdc->busy && tdc_start_head_req(tdc)) {
 		/* Continuous single mode: Configure next req */
 		if (tdc->cyclic) {
 			/*

drivers/dma/tegra20-apb-dma.c static int tegra_dma_terminate_all(struct dma_chan *dc) · line 793

Quoted 10 lines

drivers/dma/tegra20-apb-dma.c +7 -0

@@ -775,6 +784,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	else
 		wcount = status;
 
+	/*
+	 * tegra_dma_stop() will drop the RPM's usage refcount, but
+	 * tegra_dma_resume() touches hardware and thus we should keep
+	 * the DMA clock active while it's needed.
+	 */
+	pm_runtime_get(tdc->tdma->dev);
+

Would it work and make it simpler to just enable in the issue_pending and disable in the handle_once_dma_done or terminate_all?

drivers/dma/tegra20-apb-dma.c +11 -1

index 3a45079d11ec..86bbb45da93d 100644
@@ -616,9 +616,14 @@ static void handle_once_dma_done(struct
 tegra_dma_channel *tdc,
        list_add_tail(&sgreq->node, &tdc->free_sg_req);
 
        /* Do not start DMA if it is going to be terminate */
-       if (to_terminate || list_empty(&tdc->pending_sg_req))
+       if (to_terminate)
                return;
 
+       if (list_empty(&tdc->pending_sg_req)) {
+               pm_runtime_put(tdc->tdma->dev);
+               return;
+       }
+
        tdc_start_head_req(tdc);
 }

@@ -729,6 +734,11 @@ static void tegra_dma_issue_pending(struct dma_chan
 *dc)
                goto end;
        }
        if (!tdc->busy) {
+               if (pm_runtime_get_sync(tdc->tdma->dev) < 0) {
+                       dev_err(tdc2dev(tdc), "Failed to enable DMA!\n");
+                       goto end;
+               }
+
                tdc_start_head_req(tdc);

                /* Continuous single mode: Configure next req */

drivers/dma/tegra20-apb-dma.c +1 -0

@@ -788,6 +798,7 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
                                get_current_xferred_count(tdc, sgreq,
 wcount);
        }
        tegra_dma_resume(tdc);
+       pm_runtime_put(tdc->tdma->dev);
 
 skip_dma_stop:

Quoted 2 lines

        tegra_dma_abort_all(tdc);

The tegra_dma_stop() should put RPM anyways, which is missed in yours sample. Please see handle_continuous_head_request().

I'm also finding the explicit get/put a bit easier to follow in the code, don't you think so?

Jon Hunter

maintainer

2020-01-30 18:45 UTC #18 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

Quoted 130 lines · Dmitry Osipenko

30.01.2020 17:09, Jon Hunter пишет:

Quoted 129 lines · Dmitry Osipenko

Quoted 72 lines

---
 drivers/dma/tegra20-apb-dma.c | 47 ++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 15 deletions(-)

drivers/dma/tegra20-apb-dma.c +14 -5

index 22b88ccff05d..0ee28d8e3c96 100644
@@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
 	}
 	tdc->busy = false;
+
+	pm_runtime_put(tdc->tdma->dev);
 }
 
 static void tegra_dma_start(struct tegra_dma_channel *tdc,

@@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
 	tegra_dma_resume(tdc);
 }
 
-static void tdc_start_head_req(struct tegra_dma_channel *tdc)
+static bool tdc_start_head_req(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma_sg_req *sg_req;
+	int err;
 
 	if (list_empty(&tdc->pending_sg_req))
-		return;
+		return false;
+
+	err = pm_runtime_get_sync(tdc->tdma->dev);
+	if (WARN_ON_ONCE(err < 0))
+		return false;
 
 	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
 	tegra_dma_start(tdc, sg_req);
 	sg_req->configured = true;
 	sg_req->words_xferred = 0;
 	tdc->busy = true;
+
+	return true;
 }
 
 static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)

@@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
 	}
 	list_add_tail(&sgreq->node, &tdc->free_sg_req);
 
+	pm_runtime_put(tdc->tdma->dev);
+
 	/* Do not start DMA if it is going to be terminate */
 	if (to_terminate || list_empty(&tdc->pending_sg_req))
 		return;

@@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
 		dev_err(tdc2dev(tdc), "No DMA request\n");
 		goto end;
 	}
-	if (!tdc->busy) {
-		tdc_start_head_req(tdc);
-
+	if (!tdc->busy && tdc_start_head_req(tdc)) {
 		/* Continuous single mode: Configure next req */
 		if (tdc->cyclic) {
 			/*

drivers/dma/tegra20-apb-dma.c static int tegra_dma_terminate_all(struct dma_chan *dc) · line 793

Quoted 10 lines

drivers/dma/tegra20-apb-dma.c +7 -0

@@ -775,6 +784,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	else
 		wcount = status;
 
+	/*
+	 * tegra_dma_stop() will drop the RPM's usage refcount, but
+	 * tegra_dma_resume() touches hardware and thus we should keep
+	 * the DMA clock active while it's needed.
+	 */
+	pm_runtime_get(tdc->tdma->dev);
+

Would it work and make it simpler to just enable in the issue_pending and disable in the handle_once_dma_done or terminate_all?

drivers/dma/tegra20-apb-dma.c +11 -1

index 3a45079d11ec..86bbb45da93d 100644
@@ -616,9 +616,14 @@ static void handle_once_dma_done(struct
 tegra_dma_channel *tdc,
        list_add_tail(&sgreq->node, &tdc->free_sg_req);
 
        /* Do not start DMA if it is going to be terminate */
-       if (to_terminate || list_empty(&tdc->pending_sg_req))
+       if (to_terminate)
                return;
 
+       if (list_empty(&tdc->pending_sg_req)) {
+               pm_runtime_put(tdc->tdma->dev);
+               return;
+       }
+
        tdc_start_head_req(tdc);
 }

@@ -729,6 +734,11 @@ static void tegra_dma_issue_pending(struct dma_chan
 *dc)
                goto end;
        }
        if (!tdc->busy) {
+               if (pm_runtime_get_sync(tdc->tdma->dev) < 0) {
+                       dev_err(tdc2dev(tdc), "Failed to enable DMA!\n");
+                       goto end;
+               }
+
                tdc_start_head_req(tdc);

                /* Continuous single mode: Configure next req */

drivers/dma/tegra20-apb-dma.c +1 -0

@@ -788,6 +798,7 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
                                get_current_xferred_count(tdc, sgreq,
 wcount);
        }
        tegra_dma_resume(tdc);
+       pm_runtime_put(tdc->tdma->dev);

Quoted 6 lines

Quoted 3 lines
drivers/dma/tegra20-apb-dma.c +0 -0
 skip_dma_stop:
        tegra_dma_abort_all(tdc);
The tegra_dma_stop() should put RPM anyways, which is missed in yours sample. Please see handle_continuous_head_request().

Yes and that is deliberate. The cyclic transfers the transfers *should* not stop until terminate_all is called. The tegra_dma_stop in handle_continuous_head_request() is an error condition and so I am not sure it is actually necessary to call pm_runtime_put() here.

Quoted 2 lines

I'm also finding the explicit get/put a bit easier to follow in the code, don't you think so?

I can see that, but I was thinking that in the case of cyclic transfers, it should only really be necessary to call the get/put at the beginning and end. So in my mind there should only be two exit points which are the ISR handler for SG and terminate_all for SG and cyclic.

Jon

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-01-30 20:04 UTC #19 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

30.01.2020 21:45, Jon Hunter пишет:

Quoted 139 lines · Dmitry Osipenko

Quoted 130 lines

30.01.2020 17:09, Jon Hunter пишет:

Quoted 129 lines · Dmitry Osipenko

Quoted 72 lines

---
 drivers/dma/tegra20-apb-dma.c | 47 ++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 15 deletions(-)

drivers/dma/tegra20-apb-dma.c +14 -5

index 22b88ccff05d..0ee28d8e3c96 100644
@@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
 	}
 	tdc->busy = false;
+
+	pm_runtime_put(tdc->tdma->dev);
 }
 
 static void tegra_dma_start(struct tegra_dma_channel *tdc,

@@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
 	tegra_dma_resume(tdc);
 }
 
-static void tdc_start_head_req(struct tegra_dma_channel *tdc)
+static bool tdc_start_head_req(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma_sg_req *sg_req;
+	int err;
 
 	if (list_empty(&tdc->pending_sg_req))
-		return;
+		return false;
+
+	err = pm_runtime_get_sync(tdc->tdma->dev);
+	if (WARN_ON_ONCE(err < 0))
+		return false;
 
 	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
 	tegra_dma_start(tdc, sg_req);
 	sg_req->configured = true;
 	sg_req->words_xferred = 0;
 	tdc->busy = true;
+
+	return true;
 }
 
 static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)

@@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
 	}
 	list_add_tail(&sgreq->node, &tdc->free_sg_req);
 
+	pm_runtime_put(tdc->tdma->dev);
+
 	/* Do not start DMA if it is going to be terminate */
 	if (to_terminate || list_empty(&tdc->pending_sg_req))
 		return;

@@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
 		dev_err(tdc2dev(tdc), "No DMA request\n");
 		goto end;
 	}
-	if (!tdc->busy) {
-		tdc_start_head_req(tdc);
-
+	if (!tdc->busy && tdc_start_head_req(tdc)) {
 		/* Continuous single mode: Configure next req */
 		if (tdc->cyclic) {
 			/*

drivers/dma/tegra20-apb-dma.c static int tegra_dma_terminate_all(struct dma_chan *dc) · line 793

Quoted 10 lines

drivers/dma/tegra20-apb-dma.c +7 -0

@@ -775,6 +784,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	else
 		wcount = status;
 
+	/*
+	 * tegra_dma_stop() will drop the RPM's usage refcount, but
+	 * tegra_dma_resume() touches hardware and thus we should keep
+	 * the DMA clock active while it's needed.
+	 */
+	pm_runtime_get(tdc->tdma->dev);
+

Would it work and make it simpler to just enable in the issue_pending and disable in the handle_once_dma_done or terminate_all?

drivers/dma/tegra20-apb-dma.c +11 -1

index 3a45079d11ec..86bbb45da93d 100644
@@ -616,9 +616,14 @@ static void handle_once_dma_done(struct
 tegra_dma_channel *tdc,
        list_add_tail(&sgreq->node, &tdc->free_sg_req);
 
        /* Do not start DMA if it is going to be terminate */
-       if (to_terminate || list_empty(&tdc->pending_sg_req))
+       if (to_terminate)
                return;
 
+       if (list_empty(&tdc->pending_sg_req)) {
+               pm_runtime_put(tdc->tdma->dev);
+               return;
+       }
+
        tdc_start_head_req(tdc);
 }

@@ -729,6 +734,11 @@ static void tegra_dma_issue_pending(struct dma_chan
 *dc)
                goto end;
        }
        if (!tdc->busy) {
+               if (pm_runtime_get_sync(tdc->tdma->dev) < 0) {
+                       dev_err(tdc2dev(tdc), "Failed to enable DMA!\n");
+                       goto end;
+               }
+
                tdc_start_head_req(tdc);

                /* Continuous single mode: Configure next req */

drivers/dma/tegra20-apb-dma.c +1 -0

@@ -788,6 +798,7 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
                                get_current_xferred_count(tdc, sgreq,
 wcount);
        }
        tegra_dma_resume(tdc);
+       pm_runtime_put(tdc->tdma->dev);

Quoted 6 lines

Quoted 3 lines
drivers/dma/tegra20-apb-dma.c +0 -0
 skip_dma_stop:
        tegra_dma_abort_all(tdc);
The tegra_dma_stop() should put RPM anyways, which is missed in yours sample. Please see handle_continuous_head_request().

Quoted 4 lines

Yes and that is deliberate. The cyclic transfers the transfers *should* not stop until terminate_all is called. The tegra_dma_stop in handle_continuous_head_request() is an error condition and so I am not sure it is actually necessary to call pm_runtime_put() here.

But then tegra_dma_stop() shouldn't unset the "busy" mark.

Quoted 7 lines

Quoted 2 lines
I'm also finding the explicit get/put a bit easier to follow in the code, don't you think so?
I can see that, but I was thinking that in the case of cyclic transfers, it should only really be necessary to call the get/put at the beginning and end. So in my mind there should only be two exit points which are the ISR handler for SG and terminate_all for SG and cyclic.

Alright, I'll update this patch.

Jon Hunter

maintainer

2020-01-31 09:02 UTC #20 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

On 30/01/2020 20:04, Dmitry Osipenko wrote:

...

Quoted 9 lines

Quoted 7 lines
Quoted 2 lines
The tegra_dma_stop() should put RPM anyways, which is missed in yours sample. Please see handle_continuous_head_request().
Yes and that is deliberate. The cyclic transfers the transfers *should* not stop until terminate_all is called. The tegra_dma_stop in handle_continuous_head_request() is an error condition and so I am not sure it is actually necessary to call pm_runtime_put() here.
But then tegra_dma_stop() shouldn't unset the "busy" mark.

True.

Quoted 9 lines

Quoted 7 lines
Quoted 2 lines
I'm also finding the explicit get/put a bit easier to follow in the code, don't you think so?
I can see that, but I was thinking that in the case of cyclic transfers, it should only really be necessary to call the get/put at the beginning and end. So in my mind there should only be two exit points which are the ISR handler for SG and terminate_all for SG and cyclic.
Alright, I'll update this patch.

Hmmm ... I am wondering if we should not mess with that and leave how you have it.

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-01-31 14:22 UTC #21 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

31.01.2020 12:02, Jon Hunter пишет:

Quoted 25 lines · Dmitry Osipenko

...
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
The tegra_dma_stop() should put RPM anyways, which is missed in yours sample. Please see handle_continuous_head_request().
Yes and that is deliberate. The cyclic transfers the transfers *should* not stop until terminate_all is called. The tegra_dma_stop in handle_continuous_head_request() is an error condition and so I am not sure it is actually necessary to call pm_runtime_put() here.
But then tegra_dma_stop() shouldn't unset the "busy" mark.
True.
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
I'm also finding the explicit get/put a bit easier to follow in the code, don't you think so?
I can see that, but I was thinking that in the case of cyclic transfers, it should only really be necessary to call the get/put at the beginning and end. So in my mind there should only be two exit points which are the ISR handler for SG and terminate_all for SG and cyclic.

Quoted 4 lines

Quoted 1 line
Alright, I'll update this patch.
Hmmm ... I am wondering if we should not mess with that and leave how you have it.

I took another look and seems my current v6 should be more correct because:

1. If "busy" is unset in tegra_dma_stop(), then the RPM should be put there since tegra_dma_terminate_all() won't put RPM in this case:

	if (!tdc->busy)
		goto skip_dma_stop;

2. We can't move the "busy" unsetting into the terminate because then tegra_dma_stop() will be invoked twice. Although, one option could be to remove the tegra_dma_stop() from the error paths of handle_continuous_head_request(), but I'm not sure that this is correct to do.

Dmitry Osipenko

author

2020-02-01 15:13 UTC #22 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

31.01.2020 17:22, Dmitry Osipenko пишет:

Quoted 36 lines

31.01.2020 12:02, Jon Hunter пишет:
Quoted 25 lines · Dmitry Osipenko
...
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
The tegra_dma_stop() should put RPM anyways, which is missed in yours sample. Please see handle_continuous_head_request().
Yes and that is deliberate. The cyclic transfers the transfers *should* not stop until terminate_all is called. The tegra_dma_stop in handle_continuous_head_request() is an error condition and so I am not sure it is actually necessary to call pm_runtime_put() here.
But then tegra_dma_stop() shouldn't unset the "busy" mark.
True.
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
I'm also finding the explicit get/put a bit easier to follow in the code, don't you think so?
I can see that, but I was thinking that in the case of cyclic transfers, it should only really be necessary to call the get/put at the beginning and end. So in my mind there should only be two exit points which are the ISR handler for SG and terminate_all for SG and cyclic.
Quoted 4 lines
Quoted 1 line
Alright, I'll update this patch.
Hmmm ... I am wondering if we should not mess with that and leave how you have it.
I took another look and seems my current v6 should be more correct because:
1. If "busy" is unset in tegra_dma_stop(), then the RPM should be put there since tegra_dma_terminate_all() won't put RPM in this case:

Quoted 8 lines

	if (!tdc->busy)
		goto skip_dma_stop;
2. We can't move the "busy" unsetting into the terminate because then tegra_dma_stop() will be invoked twice. Although, one option could be to remove the tegra_dma_stop() from the error paths of handle_continuous_head_request(), but I'm not sure that this is correct to do.

Jon, I realized that my v6 variant is wrong too because tegra_dma_terminate_all() -> tdc->isr_handler() will put RPM, and thus, the RPM enable-count will be wrecked in this case.

I'm now leaning to adopt yours variant and simply remove the tegra_dma_stop() from handle_continuous_head_request() because there shouldn't be any harm in keeping DMA active in the case of error condition. Besides, these error conditions are very extreme cases that should never happen in practice.

The "list_empty(&tdc->pending_sg_req)" error seems couldn't ever happen at all, I'll remove it in v7.

Jon Hunter

maintainer

2020-02-03 11:37 UTC #23 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

Quoted 46 lines · Dmitry Osipenko

31.01.2020 17:22, Dmitry Osipenko пишет:
Quoted 36 lines
31.01.2020 12:02, Jon Hunter пишет:
Quoted 25 lines · Dmitry Osipenko
...
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
The tegra_dma_stop() should put RPM anyways, which is missed in yours sample. Please see handle_continuous_head_request().
Yes and that is deliberate. The cyclic transfers the transfers *should* not stop until terminate_all is called. The tegra_dma_stop in handle_continuous_head_request() is an error condition and so I am not sure it is actually necessary to call pm_runtime_put() here.
But then tegra_dma_stop() shouldn't unset the "busy" mark.
True.
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
I'm also finding the explicit get/put a bit easier to follow in the code, don't you think so?
I can see that, but I was thinking that in the case of cyclic transfers, it should only really be necessary to call the get/put at the beginning and end. So in my mind there should only be two exit points which are the ISR handler for SG and terminate_all for SG and cyclic.
Quoted 4 lines
Quoted 1 line
Alright, I'll update this patch.
Hmmm ... I am wondering if we should not mess with that and leave how you have it.
I took another look and seems my current v6 should be more correct because:
1. If "busy" is unset in tegra_dma_stop(), then the RPM should be put there since tegra_dma_terminate_all() won't put RPM in this case:
Quoted 8 lines
	if (!tdc->busy)
		goto skip_dma_stop;
2. We can't move the "busy" unsetting into the terminate because then tegra_dma_stop() will be invoked twice. Although, one option could be to remove the tegra_dma_stop() from the error paths of handle_continuous_head_request(), but I'm not sure that this is correct to do.

Quoted 3 lines

Jon, I realized that my v6 variant is wrong too because tegra_dma_terminate_all() -> tdc->isr_handler() will put RPM, and thus, the RPM enable-count will be wrecked in this case.

Did you see my other suggestion to move the pm_runtime_put() outside of tegra_dma_stop? There are only a few call sites for tegra_dma_stop and so if we call pm_runtime_put() after calling tegra_dma_stop this should simplify matters.

Jon

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-02-03 16:24 UTC #24 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

03.02.2020 14:37, Jon Hunter пишет:

Quoted 48 lines · Dmitry Osipenko

Quoted 46 lines
31.01.2020 17:22, Dmitry Osipenko пишет:
Quoted 44 lines
31.01.2020 12:02, Jon Hunter пишет:
Quoted 25 lines · Dmitry Osipenko
...
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
The tegra_dma_stop() should put RPM anyways, which is missed in yours sample. Please see handle_continuous_head_request().
Yes and that is deliberate. The cyclic transfers the transfers *should* not stop until terminate_all is called. The tegra_dma_stop in handle_continuous_head_request() is an error condition and so I am not sure it is actually necessary to call pm_runtime_put() here.
But then tegra_dma_stop() shouldn't unset the "busy" mark.
True.
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
I'm also finding the explicit get/put a bit easier to follow in the code, don't you think so?
I can see that, but I was thinking that in the case of cyclic transfers, it should only really be necessary to call the get/put at the beginning and end. So in my mind there should only be two exit points which are the ISR handler for SG and terminate_all for SG and cyclic.
Quoted 4 lines
Quoted 1 line
Alright, I'll update this patch.
Hmmm ... I am wondering if we should not mess with that and leave how you have it.
I took another look and seems my current v6 should be more correct because:
1. If "busy" is unset in tegra_dma_stop(), then the RPM should be put there since tegra_dma_terminate_all() won't put RPM in this case:
	if (!tdc->busy)
		goto skip_dma_stop;
2. We can't move the "busy" unsetting into the terminate because then tegra_dma_stop() will be invoked twice. Although, one option could be to remove the tegra_dma_stop() from the error paths of handle_continuous_head_request(), but I'm not sure that this is correct to do.

Quoted 6 lines

Quoted 3 lines
Jon, I realized that my v6 variant is wrong too because tegra_dma_terminate_all() -> tdc->isr_handler() will put RPM, and thus, the RPM enable-count will be wrecked in this case.
Did you see my other suggestion to move the pm_runtime_put() outside of tegra_dma_stop?

Yes, but seems I skimmed too quickly through the lines and failed to recognize the point you made.

Quoted 3 lines

There are only a few call sites for tegra_dma_stop and
so if we call pm_runtime_put() after calling tegra_dma_stop this should simplify matters.

This is somewhat similar to what I made in the v7. Instead of adding pm_runtime_put() after each tegra_dma_stop(), I removed the tegra_dma_stop().

Looking at it once again, perhaps indeed it will be better to leave the relevant tegra_dma_stop() in place (the irrelevant could be removed).

Please take a look at the v7, I'll drop the "[PATCH v7 13/19] dmaengine:
tegra-apb: Don't stop cyclic DMA in a case of error condition" and make v8 after yours review of the v7. Thanks in advance!

Jon Hunter

maintainer

2020-01-31 09:05 UTC #25 lore raw

Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

Quoted 15 lines · Dmitry Osipenko

It's a bit impractical to enable hardware's clock at the time of DMA channel's allocation because most of DMA client drivers allocate DMA channel at the time of the driver's probing, and thus, DMA clock is kept always-enabled in practice, defeating the whole purpose of runtime PM.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/dma/tegra20-apb-dma.c | 47 ++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 15 deletions(-)
drivers/dma/tegra20-apb-dma.c +0 -0
index 22b88ccff05d..0ee28d8e3c96 100644

drivers/dma/tegra20-apb-dma.c static void tegra_dma_stop(struct tegra_dma_channel *tdc) · line 440

Quoted 5 lines

drivers/dma/tegra20-apb-dma.c +2 -0

@@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
 	}
 	tdc->busy = false;
+
+	pm_runtime_put(tdc->tdma->dev);

There are only 3 places where tegra_dma_stop is called, does it simplify the code if we move the pm_runtime_put() outside of tegra_dma_stop? In other words, everywhere there is a tegra_dma_stop, afterwards we then call pm_runtime_put?

This would allow us to get rid of the extra pm_runtime_get in terminate_all.

Jon

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-01-30 04:38 UTC #26 lore raw

PATCH v6 12/16 dmaengine: tegra-apb: Clean up suspend-resume

It is enough to check whether hardware is busy on suspend and to reset it across of suspend-resume because channel's configuration is fully re-programmed on each DMA transaction anyways and because save-restore of an active channel won't end up well without pausing transfer prior to saving of the state (note that all channels shall be idling at the time of suspend, so save-restore is not needed at all).

[1]
https://elixir.bootlin.com/linux/v5.5/source/sound/core/pcm_dmaengine.c#L199

---
 drivers/dma/tegra20-apb-dma.c | 133 ++++++++++++++++++----------------
 1 file changed, 69 insertions(+), 64 deletions(-)

drivers/dma/tegra20-apb-dma.c +69 ~~-64~~

index 0ee28d8e3c96..4a371891b7c4 100644
@@ -1401,6 +1401,36 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
 	.support_separate_wcount_reg = true,
 };
 
+static int tegra_dma_init_hw(struct tegra_dma *tdma)
+{
+	int err;
+
+	err = reset_control_assert(tdma->rst);
+	if (err) {
+		dev_err(tdma->dev, "failed to assert reset: %d\n", err);
+		return err;
+	}
+
+	err = clk_enable(tdma->dma_clk);
+	if (err) {
+		dev_err(tdma->dev, "failed to enable clk: %d\n", err);
+		return err;
+	}
+
+	/* reset DMA controller */
+	udelay(2);
+	reset_control_deassert(tdma->rst);
+
+	/* enable global DMA registers */
+	tdma_write(tdma, TEGRA_APBDMA_GENERAL, TEGRA_APBDMA_GENERAL_ENABLE);
+	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
+	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFF);
+
+	clk_disable(tdma->dma_clk);
+
+	return 0;
+}
+
 static int tegra_dma_probe(struct platform_device *pdev)
 {
 	const struct tegra_dma_chip_data *cdata;

@@ -1442,25 +1472,13 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = tegra_dma_init_hw(tdma);
+	if (ret)
+		goto err_clk_unprepare;
+
 	pm_runtime_irq_safe(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
-	ret = pm_runtime_get_sync(&pdev->dev);
-	if (ret < 0)
-		goto err_pm_disable;
-
-	/* Reset DMA controller */
-	reset_control_assert(tdma->rst);
-	udelay(2);
-	reset_control_deassert(tdma->rst);
-
-	/* Enable global DMA registers */
-	tdma_write(tdma, TEGRA_APBDMA_GENERAL, TEGRA_APBDMA_GENERAL_ENABLE);
-	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
-	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
-
-	pm_runtime_put(&pdev->dev);
-
 	INIT_LIST_HEAD(&tdma->dma_dev.channels);
 	for (i = 0; i < cdata->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];

@@ -1558,6 +1576,8 @@ static int tegra_dma_probe(struct platform_device *pdev)
 
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
+
+err_clk_unprepare:
 	clk_unprepare(tdma->dma_clk);
 
 	return ret;

@@ -1577,26 +1597,6 @@ static int tegra_dma_remove(struct platform_device *pdev)
 static int tegra_dma_runtime_suspend(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	unsigned int i;
-
-	tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL);
-	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
-		struct tegra_dma_channel *tdc = &tdma->channels[i];
-		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
-
-		/* Only save the state of DMA channels that are in use */
-		if (!tdc->config_init)
-			continue;
-
-		ch_reg->csr = tdc_read(tdc, TEGRA_APBDMA_CHAN_CSR);
-		ch_reg->ahb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
-		ch_reg->apb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBPTR);
-		ch_reg->ahb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBSEQ);
-		ch_reg->apb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBSEQ);
-		if (tdma->chip_data->support_separate_wcount_reg)
-			ch_reg->wcount = tdc_read(tdc,
-						  TEGRA_APBDMA_CHAN_WCOUNT);
-	}
 
 	clk_disable(tdma->dma_clk);

@@ -1606,46 +1606,51 @@ static int tegra_dma_runtime_suspend(struct device *dev)
 static int tegra_dma_runtime_resume(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	unsigned int i;
-	int ret;
 
-	ret = clk_enable(tdma->dma_clk);
-	if (ret < 0) {
-		dev_err(dev, "clk_enable failed: %d\n", ret);
-		return ret;
-	}
+	return clk_enable(tdma->dma_clk);
+}
 
-	tdma_write(tdma, TEGRA_APBDMA_GENERAL, tdma->reg_gen);
-	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
-	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
+static int __maybe_unused tegra_dma_dev_suspend(struct device *dev)
+{
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
+	unsigned long flags;
+	unsigned int i;
+	bool busy;
 
 	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
-		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
-
-		/* Only restore the state of DMA channels that are in use */
-		if (!tdc->config_init)
-			continue;
-
-		if (tdma->chip_data->support_separate_wcount_reg)
-			tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
-				  ch_reg->wcount);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBSEQ, ch_reg->apb_seq);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBPTR, ch_reg->apb_ptr);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBSEQ, ch_reg->ahb_seq);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, ch_reg->ahb_ptr);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
-			  ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB);
+
+		tasklet_kill(&tdc->tasklet);
+
+		spin_lock_irqsave(&tdc->lock, flags);
+		busy = tdc->busy;
+		spin_unlock_irqrestore(&tdc->lock, flags);
+
+		if (busy) {
+			dev_err(tdma->dev, "channel %u busy\n", i);
+			return -EBUSY;
+		}
 	}
 
-	return 0;
+	return pm_runtime_force_suspend(dev);
+}
+
+static int __maybe_unused tegra_dma_dev_resume(struct device *dev)
+{
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
+	int err;
+
+	err = tegra_dma_init_hw(tdma);
+	if (err)
+		return err;
+
+	return pm_runtime_force_resume(dev);
 }
 
 static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
 			   NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				pm_runtime_force_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_dev_suspend, tegra_dma_dev_resume)
 };
 
 static const struct of_device_id tegra_dma_of_match[] = {

Signature · 1 line

2.24.0

Jon Hunter

maintainer

2020-01-30 14:09 UTC #27 lore raw

Re: [PATCH v6 12/16] dmaengine: tegra-apb: Clean up suspend-resume

Quoted 6 lines · Dmitry Osipenko

It is enough to check whether hardware is busy on suspend and to reset it across of suspend-resume because channel's configuration is fully re-programmed on each DMA transaction anyways and because save-restore of an active channel won't end up well without pausing transfer prior to saving of the state (note that all channels shall be idling at the time of suspend, so save-restore is not needed at all).

I guess if we ever wanted to support SNDRV_PCM_INFO_PAUSE for audio and support the pause callback, then saving and restoring the channels could be needed. Right now I believe that it will just terminate_all transfers for audio on entering suspend. Any value in keeping this?

Jon

Signature · 1 line

nvpublic

Dmitry Osipenko

author

2020-01-30 16:08 UTC #28 lore raw

Re: [PATCH v6 12/16] dmaengine: tegra-apb: Clean up suspend-resume

30.01.2020 17:09, Jon Hunter пишет:

Quoted 9 lines · Dmitry Osipenko

Quoted 6 lines
It is enough to check whether hardware is busy on suspend and to reset it across of suspend-resume because channel's configuration is fully re-programmed on each DMA transaction anyways and because save-restore of an active channel won't end up well without pausing transfer prior to saving of the state (note that all channels shall be idling at the time of suspend, so save-restore is not needed at all).

Quoted 4 lines

I guess if we ever wanted to support SNDRV_PCM_INFO_PAUSE for audio and support the pause callback, then saving and restoring the channels could be needed. Right now I believe that it will just terminate_all transfers for audio on entering suspend. Any value in keeping this?

Indeed, looks like [1] pauses DMA during suspend if SNDRV_PCM_INFO_PAUSE is supported.

So we'll need to save-restore context only if DMA is in a paused state during suspend, I'll adjust this patch to do that and will see if enabling SNDRV_PCM_INFO_PAUSE works.

Dmitry Osipenko

author

2020-01-30 18:06 UTC #29 lore raw

Re: [PATCH v6 12/16] dmaengine: tegra-apb: Clean up suspend-resume

30.01.2020 19:08, Dmitry Osipenko пишет:

Quoted 18 lines

30.01.2020 17:09, Jon Hunter пишет:
Quoted 9 lines · Dmitry Osipenko
Quoted 6 lines
It is enough to check whether hardware is busy on suspend and to reset it across of suspend-resume because channel's configuration is fully re-programmed on each DMA transaction anyways and because save-restore of an active channel won't end up well without pausing transfer prior to saving of the state (note that all channels shall be idling at the time of suspend, so save-restore is not needed at all).
Quoted 4 lines
I guess if we ever wanted to support SNDRV_PCM_INFO_PAUSE for audio and support the pause callback, then saving and restoring the channels could be needed. Right now I believe that it will just terminate_all transfers for audio on entering suspend. Any value in keeping this?
Indeed, looks like [1] pauses DMA during suspend if SNDRV_PCM_INFO_PAUSE is supported.

Quoted 6 lines

[1]
https://elixir.bootlin.com/linux/v5.5/source/sound/core/pcm_dmaengine.c#L199
So we'll need to save-restore context only if DMA is in a paused state during suspend, I'll adjust this patch to do that and will see if enabling SNDRV_PCM_INFO_PAUSE works.

I started to look at it and found that the .device_pause() hook isn't implemented by the driver. So, it's fine to remove the context's save-restore for now.

Jon, what about to keep this patch as-is? Later on I'll take a look at implementing the proper pausing functionality and try to cleanup code a bit further (remove the free list usage, etc).

Dmitry Osipenko

author

2020-01-30 18:26 UTC #30 lore raw

Re: [PATCH v6 12/16] dmaengine: tegra-apb: Clean up suspend-resume

30.01.2020 21:06, Dmitry Osipenko пишет:

Quoted 26 lines

30.01.2020 19:08, Dmitry Osipenko пишет:
Quoted 18 lines
30.01.2020 17:09, Jon Hunter пишет:
Quoted 9 lines · Dmitry Osipenko
Quoted 6 lines
It is enough to check whether hardware is busy on suspend and to reset it across of suspend-resume because channel's configuration is fully re-programmed on each DMA transaction anyways and because save-restore of an active channel won't end up well without pausing transfer prior to saving of the state (note that all channels shall be idling at the time of suspend, so save-restore is not needed at all).
Quoted 4 lines
I guess if we ever wanted to support SNDRV_PCM_INFO_PAUSE for audio and support the pause callback, then saving and restoring the channels could be needed. Right now I believe that it will just terminate_all transfers for audio on entering suspend. Any value in keeping this?
Indeed, looks like [1] pauses DMA during suspend if SNDRV_PCM_INFO_PAUSE is supported.
Quoted 6 lines
[1]
https://elixir.bootlin.com/linux/v5.5/source/sound/core/pcm_dmaengine.c#L199
So we'll need to save-restore context only if DMA is in a paused state during suspend, I'll adjust this patch to do that and will see if enabling SNDRV_PCM_INFO_PAUSE works.

Quoted 7 lines

I started to look at it and found that the .device_pause() hook isn't implemented by the driver. So, it's fine to remove the context's save-restore for now.
Jon, what about to keep this patch as-is? Later on I'll take a look at implementing the proper pausing functionality and try to cleanup code a bit further (remove the free list usage, etc).

Ah, only T114+ supports the per-channel pausing. So, I won't care about implementing the device_pause(), let's leave it to somebody else :)

Jon Hunter

maintainer

2020-01-30 18:58 UTC #31 lore raw

Re: [PATCH v6 12/16] dmaengine: tegra-apb: Clean up suspend-resume

Quoted 31 lines · Dmitry Osipenko

30.01.2020 21:06, Dmitry Osipenko пишет:
Quoted 30 lines
30.01.2020 19:08, Dmitry Osipenko пишет:
Quoted 18 lines
30.01.2020 17:09, Jon Hunter пишет:
Quoted 9 lines · Dmitry Osipenko
Quoted 6 lines
It is enough to check whether hardware is busy on suspend and to reset it across of suspend-resume because channel's configuration is fully re-programmed on each DMA transaction anyways and because save-restore of an active channel won't end up well without pausing transfer prior to saving of the state (note that all channels shall be idling at the time of suspend, so save-restore is not needed at all).
Quoted 4 lines
I guess if we ever wanted to support SNDRV_PCM_INFO_PAUSE for audio and support the pause callback, then saving and restoring the channels could be needed. Right now I believe that it will just terminate_all transfers for audio on entering suspend. Any value in keeping this?
Indeed, looks like [1] pauses DMA during suspend if SNDRV_PCM_INFO_PAUSE is supported.
Quoted 6 lines
[1]
https://elixir.bootlin.com/linux/v5.5/source/sound/core/pcm_dmaengine.c#L199
So we'll need to save-restore context only if DMA is in a paused state during suspend, I'll adjust this patch to do that and will see if enabling SNDRV_PCM_INFO_PAUSE works.
I started to look at it and found that the .device_pause() hook isn't implemented by the driver. So, it's fine to remove the context's save-restore for now.

Quoted 6 lines

Quoted 3 lines
Jon, what about to keep this patch as-is? Later on I'll take a look at implementing the proper pausing functionality and try to cleanup code a bit further (remove the free list usage, etc).
Ah, only T114+ supports the per-channel pausing. So, I won't care about implementing the device_pause(), let's leave it to somebody else :)

That's fine. We have lived with out it for this long.

Jon

Signature · 1 line

nvpublic

Jon Hunter

maintainer Acked-by on v6 12/16

2020-01-30 19:00 UTC #32 lore raw

Re: [PATCH v6 12/16] dmaengine: tegra-apb: Clean up suspend-resume

Quoted 6 lines · Dmitry Osipenko

It is enough to check whether hardware is busy on suspend and to reset it across of suspend-resume because channel's configuration is fully re-programmed on each DMA transaction anyways and because save-restore of an active channel won't end up well without pausing transfer prior to saving of the state (note that all channels shall be idling at the time of suspend, so save-restore is not needed at all).

Maybe just update the commit message to state that the pause callback is not implemented and channel pause is not supported prior to T114. And then ...

Cheers
Jon

Dmitry Osipenko

author

2020-01-30 20:06 UTC #33 lore raw

Re: [PATCH v6 12/16] dmaengine: tegra-apb: Clean up suspend-resume

30.01.2020 22:00, Jon Hunter пишет:

Quoted 9 lines · Dmitry Osipenko

Quoted 6 lines
It is enough to check whether hardware is busy on suspend and to reset it across of suspend-resume because channel's configuration is fully re-programmed on each DMA transaction anyways and because save-restore of an active channel won't end up well without pausing transfer prior to saving of the state (note that all channels shall be idling at the time of suspend, so save-restore is not needed at all).

Quoted 5 lines

Maybe just update the commit message to state that the pause callback is not implemented and channel pause is not supported prior to T114. And then ...
Acked-by Jon Hunter <[email protected]>

Thanks, I'll update the message.

Dmitry Osipenko

author

2020-01-30 04:38 UTC #34 lore raw

PATCH v6 13/16 dmaengine: tegra-apb: Add missing of_dma_controller_free

The DMA controller shall be released on driver's removal.

---
 drivers/dma/tegra20-apb-dma.c | 1 +
 1 file changed, 1 insertion(+)

drivers/dma/tegra20-apb-dma.c +1 -0

index 4a371891b7c4..8f4281ab6296 100644
@@ -1587,6 +1587,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 {
 	struct tegra_dma *tdma = platform_get_drvdata(pdev);
 
+	of_dma_controller_free(pdev->dev.of_node);
 	dma_async_device_unregister(&tdma->dma_dev);
 	pm_runtime_disable(&pdev->dev);
 	clk_unprepare(tdma->dma_clk);

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:38 UTC #35 lore raw

PATCH v6 14/16 dmaengine: tegra-apb: Allow to compile as a loadable kernel module

The driver's removal was fixed by a recent commit and module load/unload is working well now, tested on Tegra30.

---
 drivers/dma/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dma/Kconfig +1 -1

index 5142da401db3..98daf3aafdcc 100644
@@ -616,7 +616,7 @@ config TXX9_DMAC
 	  integrated in chips such as the Toshiba TX4927/38/39.
 
 config TEGRA20_APB_DMA
-	bool "NVIDIA Tegra20 APB DMA support"
+	tristate "NVIDIA Tegra20 APB DMA support"
 	depends on ARCH_TEGRA
 	select DMA_ENGINE
 	help

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:38 UTC #36 lore raw

PATCH v6 15/16 dmaengine: tegra-apb: Remove MODULE_ALIAS

Tegra APB DMA driver is an Open Firmware driver, so it uses OF alias naming scheme which overrides MODULE_ALIAS, meaning that MODULE_ALIAS does nothing and could be removed safely.

---
 drivers/dma/tegra20-apb-dma.c | 1 -
 1 file changed, 1 deletion(-)

drivers/dma/tegra20-apb-dma.c +0 -1

index 8f4281ab6296..756a5dbef118 100644
@@ -1684,7 +1684,6 @@ static struct platform_driver tegra_dmac_driver = {
 
 module_platform_driver(tegra_dmac_driver);
 
-MODULE_ALIAS("platform:tegra20-apbdma");
 MODULE_DESCRIPTION("NVIDIA Tegra APB DMA Controller driver");
 MODULE_AUTHOR("Laxman Dewangan <[email protected]>");
 MODULE_LICENSE("GPL v2");

Signature · 1 line

2.24.0

Dmitry Osipenko

author

2020-01-30 04:38 UTC #37 lore raw

PATCH v6 16/16 dmaengine: tegra-apb: Support COMPILE_TEST

There is nothing arch-specific in the driver's code, so let's enable compile-testing for the driver.

---
 drivers/dma/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dma/Kconfig +1 -1

index 98daf3aafdcc..7fc725d928b2 100644
@@ -617,7 +617,7 @@ config TXX9_DMAC
 
 config TEGRA20_APB_DMA
 	tristate "NVIDIA Tegra20 APB DMA support"
-	depends on ARCH_TEGRA
+	depends on ARCH_TEGRA || COMPILE_TEST
 	select DMA_ENGINE
 	help
 	  Support for the NVIDIA Tegra20 APB DMA controller driver. The

Signature · 1 line

2.24.0

Jon Hunter

maintainer Acked-by on v6 16/16

2020-01-30 14:10 UTC #38 lore raw

Re: [PATCH v6 16/16] dmaengine: tegra-apb: Support COMPILE_TEST

Quoted 13 lines · Dmitry Osipenko

There is nothing arch-specific in the driver's code, so let's enable compile-testing for the driver.
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/dma/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/dma/Kconfig +0 -0
index 98daf3aafdcc..7fc725d928b2 100644

drivers/dma/Kconfig config TXX9_DMAC · line 623

Quoted 8 lines

drivers/dma/Kconfig +1 -1

@@ -617,7 +617,7 @@ config TXX9_DMAC
 
 config TEGRA20_APB_DMA
 	tristate "NVIDIA Tegra20 APB DMA support"
-	depends on ARCH_TEGRA
+	depends on ARCH_TEGRA || COMPILE_TEST
 	select DMA_ENGINE
 	help
 	  Support for the NVIDIA Tegra20 APB DMA controller driver. The