[RFC PATCH 0/6] spi/s3c64xx: clean up and optimization

[RFC PATCH 0/6] spi/s3c64xx: clean up and optimization

[Lukasz Czerwinski]

This patch series includes following modifications:
	1) remove redundant code
	2) add coherent buffers
	3) remove multiple  channel requests and releases

For more info please look into particular commit messages.

I have tested my patches on Exynos 4412(with PL330 DMA) platform
with S5C73M3 driver(firmware upload via SPI).

This patch series depends on:
[1] - https://lkml.org/lkml/2013/8/11/79
[2] - http://www.kernelhub.org/?msg=323408&p=2

When other SAMSUNG platforms will support DMA generic operations then
my patches 1-4,6 can be applied.

This is only proof of concept so any comments are welcome.

Thanks,
Lukasz

Lukasz Czerwinski (6):
  spi: spi-s3c64xx: Remove platform dependent code
  spi: spi-s3c64xx: Correct functions namespacing
  spi: spi-s3c64xx: Add coherent buffers for DMA transfers
  spi: spi-s3c64xx: Remove redundant code
  spi: spi-s3c64xx: Use module_platform_driver()
  spi: spi-s3c64xx: Move DMA initialization

 drivers/spi/spi-s3c64xx.c |  388 +++++++++++++++------------------------------
 1 file changed, 130 insertions(+), 258 deletions(-)

-- 
1.7.9.5

[RFC PATCH 1/6] spi: spi-s3c64xx: Remove platform dependent code

[Lukasz Czerwinski]

Since support for generic DMA engine has beed added, there is no need
to keep platform specific code (behind the #ifdef CONFIG_S3C_DMA).

Signed-off-by: Lukasz Czerwinski <l.czerwinski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |  105 +--------------------------------------------
 1 file changed, 1 insertion(+), 104 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 512b889..106bc09 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -34,10 +34,6 @@
 
 #include <linux/platform_data/spi-s3c64xx.h>
 
-#ifdef CONFIG_S3C_DMA
-#include <mach/dma.h>
-#endif
-
 #define MAX_SPI_PORTS		3
 #define S3C64XX_SPI_QUIRK_POLL		(1 << 0)
 
@@ -200,9 +196,7 @@ struct s3c64xx_spi_driver_data {
 	unsigned                        cur_speed;
 	struct s3c64xx_spi_dma_data	rx_dma;
 	struct s3c64xx_spi_dma_data	tx_dma;
-#ifdef CONFIG_S3C_DMA
-	struct samsung_dma_ops		*ops;
-#endif
+
 	struct s3c64xx_spi_port_config	*port_conf;
 	unsigned int			port_id;
 	unsigned long			gpios[4];
@@ -285,102 +279,6 @@ static void s3c64xx_spi_dmacb(void *data)
 	spin_unlock_irqrestore(&sdd->lock, flags);
 }
 
-#ifdef CONFIG_S3C_DMA
-/* FIXME: remove this section once arch/arm/mach-s3c64xx uses dmaengine */
-
-static struct s3c2410_dma_client s3c64xx_spi_dma_client = {
-	.name = "samsung-spi-dma",
-};
-
-static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
-					unsigned len, dma_addr_t buf)
-{
-	struct s3c64xx_spi_driver_data *sdd;
-	struct samsung_dma_prep info;
-	struct samsung_dma_config config;
-
-	if (dma->direction == DMA_DEV_TO_MEM) {
-		sdd = container_of((void *)dma,
-			struct s3c64xx_spi_driver_data, rx_dma);
-		config.direction = sdd->rx_dma.direction;
-		config.fifo = sdd->sfr_start + S3C64XX_SPI_RX_DATA;
-		config.width = sdd->cur_bpw / 8;
-		sdd->ops->config((enum dma_ch)sdd->rx_dma.ch, &config);
-	} else {
-		sdd = container_of((void *)dma,
-			struct s3c64xx_spi_driver_data, tx_dma);
-		config.direction =  sdd->tx_dma.direction;
-		config.fifo = sdd->sfr_start + S3C64XX_SPI_TX_DATA;
-		config.width = sdd->cur_bpw / 8;
-		sdd->ops->config((enum dma_ch)sdd->tx_dma.ch, &config);
-	}
-
-	info.cap = DMA_SLAVE;
-	info.len = len;
-	info.fp = s3c64xx_spi_dmacb;
-	info.fp_param = dma;
-	info.direction = dma->direction;
-	info.buf = buf;
-
-	sdd->ops->prepare((enum dma_ch)dma->ch, &info);
-	sdd->ops->trigger((enum dma_ch)dma->ch);
-}
-
-static int acquire_dma(struct s3c64xx_spi_driver_data *sdd)
-{
-	struct samsung_dma_req req;
-	struct device *dev = &sdd->pdev->dev;
-
-	sdd->ops = samsung_dma_get_ops();
-
-	req.cap = DMA_SLAVE;
-	req.client = &s3c64xx_spi_dma_client;
-
-	sdd->rx_dma.ch = (struct dma_chan *)(unsigned long)sdd->ops->request(
-					sdd->rx_dma.dmach, &req, dev, "rx");
-	sdd->tx_dma.ch = (struct dma_chan *)(unsigned long)sdd->ops->request(
-					sdd->tx_dma.dmach, &req, dev, "tx");
-
-	return 1;
-}
-
-static int s3c64xx_spi_prepare_transfer(struct spi_master *spi)
-{
-	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(spi);
-
-	/*
-	 * If DMA resource was not available during
-	 * probe, no need to continue with dma requests
-	 * else Acquire DMA channels
-	 */
-	while (!is_polling(sdd) && !acquire_dma(sdd))
-		usleep_range(10000, 11000);
-
-	return 0;
-}
-
-static int s3c64xx_spi_unprepare_transfer(struct spi_master *spi)
-{
-	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(spi);
-
-	/* Free DMA channels */
-	if (!is_polling(sdd)) {
-		sdd->ops->release((enum dma_ch)sdd->rx_dma.ch,
-					&s3c64xx_spi_dma_client);
-		sdd->ops->release((enum dma_ch)sdd->tx_dma.ch,
-					&s3c64xx_spi_dma_client);
-	}
-
-	return 0;
-}
-
-static void s3c64xx_spi_dma_stop(struct s3c64xx_spi_driver_data *sdd,
-				 struct s3c64xx_spi_dma_data *dma)
-{
-	sdd->ops->stop((enum dma_ch)dma->ch);
-}
-#else
-
 static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
 					unsigned len, dma_addr_t buf)
 {
@@ -483,7 +381,6 @@ static void s3c64xx_spi_dma_stop(struct s3c64xx_spi_driver_data *sdd,
 {
 	dmaengine_terminate_all(dma->ch);
 }
-#endif
 
 static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 				struct spi_device *spi,
-- 
1.7.9.5

Re: [RFC PATCH 1/6] spi: spi-s3c64xx: Remove platform dependent code

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

On Mon, Sep 09, 2013 at 04:09:21PM +0200, Lukasz Czerwinski wrote:
> Since support for generic DMA engine has beed added, there is no need
> to keep platform specific code (behind the #ifdef CONFIG_S3C_DMA).

This would break s3c64xx at least until Tomasz's dmaengine conversion
for that gets merged (hopefully this time around...).  Given that one 
of my main development systems is based on s3c64xx I'm even more keen
to keep that working than otherwise.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/6] spi: spi-s3c64xx: Remove platform dependent code

[Heiko Stübner]

Am Montag, 9. September 2013, 16:38:01 schrieb Mark Brown:
> On Mon, Sep 09, 2013 at 04:09:21PM +0200, Lukasz Czerwinski wrote:
> > Since support for generic DMA engine has beed added, there is no need
> > to keep platform specific code (behind the #ifdef CONFIG_S3C_DMA).
> 
> This would break s3c64xx at least until Tomasz's dmaengine conversion
> for that gets merged (hopefully this time around...).  Given that one
> of my main development systems is based on s3c64xx I'm even more keen
> to keep that working than otherwise.

Just for completenes sake, the same is true for the s3c2416 and s3c2443 using 
this controller - and similarily with a pending migration to dmaengine.

[RFC PATCH 2/6] spi: spi-s3c64xx: Correct functions namespacing

[Lukasz Czerwinski]

This path adds missing s3c64xx_ prefixes to functions in the driver.

Signed-off-by: Lukasz Czerwinski <l.czerwinski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |   28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 106bc09..28e8c71 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -279,7 +279,7 @@ static void s3c64xx_spi_dmacb(void *data)
 	spin_unlock_irqrestore(&sdd->lock, flags);
 }
 
-static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
+static void s3c64xx_prepare_dma(struct s3c64xx_spi_dma_data *dma,
 					unsigned len, dma_addr_t buf)
 {
 	struct s3c64xx_spi_driver_data *sdd;
@@ -382,7 +382,7 @@ static void s3c64xx_spi_dma_stop(struct s3c64xx_spi_driver_data *sdd,
 	dmaengine_terminate_all(dma->ch);
 }
 
-static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
+static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 				struct spi_device *spi,
 				struct spi_transfer *xfer, int dma_mode)
 {
@@ -413,7 +413,8 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 		chcfg |= S3C64XX_SPI_CH_TXCH_ON;
 		if (dma_mode) {
 			modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
-			prepare_dma(&sdd->tx_dma, xfer->len, xfer->tx_dma);
+			s3c64xx_prepare_dma(&sdd->tx_dma,
+					xfer->len, xfer->tx_dma);
 		} else {
 			switch (sdd->cur_bpw) {
 			case 32:
@@ -445,7 +446,8 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 			writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
 					| S3C64XX_SPI_PACKET_CNT_EN,
 					regs + S3C64XX_SPI_PACKET_CNT);
-			prepare_dma(&sdd->rx_dma, xfer->len, xfer->rx_dma);
+			s3c64xx_prepare_dma(&sdd->rx_dma,
+					xfer->len, xfer->rx_dma);
 		}
 	}
 
@@ -453,7 +455,7 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 	writel(chcfg, regs + S3C64XX_SPI_CH_CFG);
 }
 
-static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
+static inline void s3c64xx_enable_cs(struct s3c64xx_spi_driver_data *sdd,
 						struct spi_device *spi)
 {
 	struct s3c64xx_spi_csinfo *cs;
@@ -498,7 +500,7 @@ static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
 	return RX_FIFO_LVL(status, sdd);
 }
 
-static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
+static int s3c64xx_wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
 				struct spi_transfer *xfer, int dma_mode)
 {
 	void __iomem *regs = sdd->regs;
@@ -596,7 +598,7 @@ static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
 	return 0;
 }
 
-static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd,
+static inline void s3c64xx_disable_cs(struct s3c64xx_spi_driver_data *sdd,
 						struct spi_device *spi)
 {
 	struct s3c64xx_spi_csinfo *cs = spi->controller_data;
@@ -829,14 +831,14 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 		sdd->state &= ~RXBUSY;
 		sdd->state &= ~TXBUSY;
 
-		enable_datapath(sdd, spi, xfer, use_dma);
+		s3c64xx_enable_datapath(sdd, spi, xfer, use_dma);
 
 		/* Slave Select */
-		enable_cs(sdd, spi);
+		s3c64xx_enable_cs(sdd, spi);
 
 		spin_unlock_irqrestore(&sdd->lock, flags);
 
-		status = wait_for_xfer(sdd, xfer, use_dma);
+		status = s3c64xx_wait_for_xfer(sdd, xfer, use_dma);
 
 		if (status) {
 			dev_err(&spi->dev, "I/O Error: rx-%d tx-%d res:rx-%c tx-%c len-%d\n",
@@ -875,7 +877,7 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 
 out:
 	if (!cs_toggle || status)
-		disable_cs(sdd, spi);
+		s3c64xx_disable_cs(sdd, spi);
 	else
 		sdd->tgl_spi = spi;
 
@@ -1014,12 +1016,12 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 	}
 
 	pm_runtime_put(&sdd->pdev->dev);
-	disable_cs(sdd, spi);
+	s3c64xx_disable_cs(sdd, spi);
 	return 0;
 
 setup_exit:
 	/* setup() returns with device de-selected */
-	disable_cs(sdd, spi);
+	s3c64xx_disable_cs(sdd, spi);
 
 	gpio_free(cs->line);
 	spi_set_ctldata(spi, NULL);
-- 
1.7.9.5

[RFC PATCH 3/6] spi: spi-s3c64xx: Add coherent buffers for DMA transfers

[Lukasz Czerwinski]

The spi-s3c64xx currently doesn't support transfers from non-contiguous
client buffers.

This patch adds two coherent buffers which allow transfers from
non-contiguous client buffers without extra coherent memory allocation
in the client driver.

Buffer size is hardcoded to 16kB for Tx/Rx. Client drivers shouldn't
exceed that value.

Signed-off-by: Lukasz Czerwinski <l.czerwinski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |  107 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 90 insertions(+), 17 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 28e8c71..1ec1244 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -113,6 +113,7 @@
 #define S3C64XX_SPI_SWAP_TX_EN			(1<<0)
 
 #define S3C64XX_SPI_FBCLK_MSK		(3<<0)
+#define S3C64XX_SPI_DMA_BUF_SIZE	(16 * SZ_1K)
 
 #define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id])
 #define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \
@@ -133,6 +134,8 @@
 #define TXBUSY    (1<<3)
 
 struct s3c64xx_spi_dma_data {
+	u32 *vbuf;
+	dma_addr_t dma_phys;
 	struct dma_chan *ch;
 	enum dma_transfer_direction direction;
 	unsigned int dmach;
@@ -178,6 +181,7 @@ struct s3c64xx_spi_port_config {
  * @xfer_completion: To indicate completion of xfer task.
  * @cur_mode: Stores the active configuration of the controller.
  * @cur_bpw: Stores the active bits per word settings.
+ * @dma_buf_size: Stores buffer size for Rx/Tx.
  * @cur_speed: Stores the active xfer clock speed.
  */
 struct s3c64xx_spi_driver_data {
@@ -194,6 +198,7 @@ struct s3c64xx_spi_driver_data {
 	unsigned                        state;
 	unsigned                        cur_mode, cur_bpw;
 	unsigned                        cur_speed;
+	unsigned                        dma_buf_size;
 	struct s3c64xx_spi_dma_data	rx_dma;
 	struct s3c64xx_spi_dma_data	tx_dma;
 
@@ -279,8 +284,7 @@ static void s3c64xx_spi_dmacb(void *data)
 	spin_unlock_irqrestore(&sdd->lock, flags);
 }
 
-static void s3c64xx_prepare_dma(struct s3c64xx_spi_dma_data *dma,
-					unsigned len, dma_addr_t buf)
+static void s3c64xx_prepare_dma(struct s3c64xx_spi_dma_data *dma, unsigned len)
 {
 	struct s3c64xx_spi_driver_data *sdd;
 	struct dma_slave_config config;
@@ -306,7 +310,7 @@ static void s3c64xx_prepare_dma(struct s3c64xx_spi_dma_data *dma,
 		dmaengine_slave_config(dma->ch, &config);
 	}
 
-	desc = dmaengine_prep_slave_single(dma->ch, buf, len,
+	desc = dmaengine_prep_slave_single(dma->ch, dma->dma_phys, len,
 					dma->direction, DMA_PREP_INTERRUPT);
 
 	desc->callback = s3c64xx_spi_dmacb;
@@ -382,6 +386,34 @@ static void s3c64xx_spi_dma_stop(struct s3c64xx_spi_driver_data *sdd,
 	dmaengine_terminate_all(dma->ch);
 }
 
+static void s3c64xx_copy_txbuf_to_spi(struct s3c64xx_spi_driver_data *sdd,
+				    struct spi_transfer *xfer)
+{
+	struct device *dev = &sdd->pdev->dev;
+
+	dma_sync_single_for_cpu(dev, sdd->tx_dma.dma_phys,
+			sdd->dma_buf_size, DMA_TO_DEVICE);
+
+	memcpy(sdd->tx_dma.vbuf, xfer->tx_buf, xfer->len);
+
+	dma_sync_single_for_device(dev, sdd->tx_dma.dma_phys,
+			sdd->dma_buf_size, DMA_TO_DEVICE);
+}
+
+static void s3c64xx_copy_spi_to_rxbuf(struct s3c64xx_spi_driver_data *sdd,
+				    struct spi_transfer *xfer)
+{
+	struct device *dev = &sdd->pdev->dev;
+
+	dma_sync_single_for_cpu(dev, sdd->rx_dma.dma_phys,
+			sdd->dma_buf_size, DMA_FROM_DEVICE);
+
+	memcpy(xfer->rx_buf, sdd->rx_dma.vbuf, xfer->len);
+
+	dma_sync_single_for_device(dev, sdd->rx_dma.dma_phys,
+			sdd->dma_buf_size, DMA_FROM_DEVICE);
+}
+
 static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 				struct spi_device *spi,
 				struct spi_transfer *xfer, int dma_mode)
@@ -413,8 +445,8 @@ static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 		chcfg |= S3C64XX_SPI_CH_TXCH_ON;
 		if (dma_mode) {
 			modecfg |= S3C64XX_SPI_MODE_TXDMA_ON;
-			s3c64xx_prepare_dma(&sdd->tx_dma,
-					xfer->len, xfer->tx_dma);
+			s3c64xx_copy_txbuf_to_spi(sdd, xfer);
+			s3c64xx_prepare_dma(&sdd->tx_dma, xfer->len);
 		} else {
 			switch (sdd->cur_bpw) {
 			case 32:
@@ -446,8 +478,8 @@ static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 			writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
 					| S3C64XX_SPI_PACKET_CNT_EN,
 					regs + S3C64XX_SPI_PACKET_CNT);
-			s3c64xx_prepare_dma(&sdd->rx_dma,
-					xfer->len, xfer->rx_dma);
+			s3c64xx_copy_spi_to_rxbuf(sdd, xfer);
+			s3c64xx_prepare_dma(&sdd->rx_dma, xfer->len);
 		}
 	}
 
@@ -782,14 +814,6 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 		s3c64xx_spi_config(sdd);
 	}
 
-	/* Map all the transfers if needed */
-	if (s3c64xx_spi_map_mssg(sdd, msg)) {
-		dev_err(&spi->dev,
-			"Xfer: Unable to map message buffers!\n");
-		status = -ENOMEM;
-		goto out;
-	}
-
 	/* Configure feedback delay */
 	writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
 
@@ -804,6 +828,14 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 		bpw = xfer->bits_per_word;
 		speed = xfer->speed_hz ? : spi->max_speed_hz;
 
+		if (xfer->len > sdd->dma_buf_size) {
+			dev_err(&spi->dev,
+					"Message length exceeds dma buffer size %d>%d\n",
+					xfer->len, sdd->dma_buf_size);
+			status = -EIO;
+			goto out;
+		}
+
 		if (xfer->len % (bpw / 8)) {
 			dev_err(&spi->dev,
 				"Xfer length(%u) not a multiple of word size(%u)\n",
@@ -881,8 +913,6 @@ out:
 	else
 		sdd->tgl_spi = spi;
 
-	s3c64xx_spi_unmap_mssg(sdd, msg);
-
 	msg->status = status;
 
 	spi_finalize_current_message(master);
@@ -1168,6 +1198,28 @@ static inline struct s3c64xx_spi_port_config *s3c64xx_spi_get_port_config(
 			 platform_get_device_id(pdev)->driver_data;
 }
 
+static int s3c64xx_init_buffer(struct s3c64xx_spi_driver_data *sdd,
+			  struct s3c64xx_spi_dma_data *dma)
+{
+	struct device *dev = &sdd->pdev->dev;
+
+	dma->vbuf = dma_alloc_coherent(dev, S3C64XX_SPI_DMA_BUF_SIZE,
+			&dma->dma_phys, GFP_KERNEL);
+
+	if (!dma->vbuf)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void s3c64xx_deinit_buffer(struct s3c64xx_spi_driver_data *sdd,
+			struct s3c64xx_spi_dma_data *dma)
+{
+	struct device *dev = &sdd->pdev->dev;
+
+	dma_free_coherent(dev, sdd->dma_buf_size, dma->vbuf, dma->dma_phys);
+}
+
 static int s3c64xx_spi_probe(struct platform_device *pdev)
 {
 	struct resource	*mem_res;
@@ -1250,6 +1302,16 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 			sdd->rx_dma.dmach = res->start;
 	}
 
+	sdd->dma_buf_size = S3C64XX_SPI_DMA_BUF_SIZE;
+	if (s3c64xx_init_buffer(sdd, &sdd->rx_dma) < 0) {
+		ret = -ENOMEM;
+		goto err0;
+	}
+	if (s3c64xx_init_buffer(sdd, &sdd->tx_dma) < 0) {
+		ret = -ENOMEM;
+		goto err0;
+	}
+
 	sdd->tx_dma.direction = DMA_MEM_TO_DEV;
 	sdd->rx_dma.direction = DMA_DEV_TO_MEM;
 
@@ -1348,6 +1410,11 @@ err3:
 err2:
 	clk_disable_unprepare(sdd->clk);
 err0:
+	if (sdd->rx_dma.vbuf)
+		s3c64xx_deinit_buffer(sdd, &sdd->rx_dma);
+	if (sdd->tx_dma.vbuf)
+		s3c64xx_deinit_buffer(sdd, &sdd->tx_dma);
+
 	spi_master_put(master);
 
 	return ret;
@@ -1364,6 +1431,12 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 
 	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
 
+	if (sdd->rx_dma.vbuf)
+		s3c64xx_deinit_buffer(sdd, &sdd->rx_dma);
+
+	if (sdd->tx_dma.vbuf)
+		s3c64xx_deinit_buffer(sdd, &sdd->tx_dma);
+
 	clk_disable_unprepare(sdd->src_clk);
 
 	clk_disable_unprepare(sdd->clk);
-- 
1.7.9.5

Re: [RFC PATCH 3/6] spi: spi-s3c64xx: Add coherent buffers for DMA transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

On Mon, Sep 09, 2013 at 04:09:23PM +0200, Lukasz Czerwinski wrote:
> The spi-s3c64xx currently doesn't support transfers from non-contiguous
> client buffers.
> 
> This patch adds two coherent buffers which allow transfers from
> non-contiguous client buffers without extra coherent memory allocation
> in the client driver.
> 
> Buffer size is hardcoded to 16kB for Tx/Rx. Client drivers shouldn't
> exceed that value.

This seems like a very low limit to have, consider things like firmware
downloads for example.  It seems reasonable to have a preallocated small
buffer but there should be some fallback for larger transfer sizes.

I also didn't notice any checks in the code for the length of transfers
so this will corrupt memory if a client driver tries to transfer more
than the preallocated buffer as things stand.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 3/6] spi: spi-s3c64xx: Add coherent buffers for DMA transfers

[Łukasz Czerwiński]

On 09/09/2013 04:45 PM, Mark Brown wrote:
> On Mon, Sep 09, 2013 at 04:09:23PM +0200, Lukasz Czerwinski wrote:
>> The spi-s3c64xx currently doesn't support transfers from non-contiguous
>> client buffers.
>>
>> This patch adds two coherent buffers which allow transfers from
>> non-contiguous client buffers without extra coherent memory allocation
>> in the client driver.
>>
>> Buffer size is hardcoded to 16kB for Tx/Rx. Client drivers shouldn't
>> exceed that value.
>
> This seems like a very low limit to have, consider things like firmware
> downloads for example.  It seems reasonable to have a preallocated small
> buffer but there should be some fallback for larger transfer sizes.
>
I have tested my modification with different buffer sizes with S5C73M3 
driver (355560 B upload). I obtained following upload times:

16kB buffer:
	- 83.972 ms
	- 79.196 ms
	- 79.432 ms
128kB buffer:
	- 74.449 ms
	- 80.719 ms
	- 75.599 ms

For 256kB I obtained similar results as in 128kB case. Normally 
non-interrupted SPI transfer should take 56ms (50MHz). Performance loss 
is approximately about 6% between 16kB and 128KB buffer. I my opinion 
there are no need to use bigger buffers.

I propose add extra module parameter which allows to change buffer size.

> I also didn't notice any checks in the code for the length of transfers
> so this will corrupt memory if a client driver tries to transfer more
> than the preallocated buffer as things stand.
>
Right, I will add that.

Thanks
Lukasz

Re: [RFC PATCH 3/6] spi: spi-s3c64xx: Add coherent buffers for DMA transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2454 bytes --]

On Tue, Sep 10, 2013 at 01:23:45PM +0200, Łukasz Czerwiński wrote:
> On 09/09/2013 04:45 PM, Mark Brown wrote:

> >This seems like a very low limit to have, consider things like firmware
> >downloads for example.  It seems reasonable to have a preallocated small
> >buffer but there should be some fallback for larger transfer sizes.

> I have tested my modification with different buffer sizes with
> S5C73M3 driver (355560 B upload). I obtained following upload times:

> 16kB buffer:
> 	- 83.972 ms
> 	- 79.196 ms
> 	- 79.432 ms
> 128kB buffer:
> 	- 74.449 ms
> 	- 80.719 ms
> 	- 75.599 ms

> For 256kB I obtained similar results as in 128kB case. Normally
> non-interrupted SPI transfer should take 56ms (50MHz). Performance
> loss is approximately about 6% between 16kB and 128KB buffer. I my
> opinion there are no need to use bigger buffers.

You're missing the point here.  This requires the client driver to know
the maximum transfer size that the controller supports and split the
transfer up for it.  This isn't something that the SPI API supports now
and while it would be useful to add for controllers that are genuinely
limited in transfer size it doesn't seem sane to just randomly impose a
limit to make life easier for software.  Not all applications will be
able to split their transfers up in the first place and it seems like
the wrong place to put that knowledge.

What I would expect to see the driver doing here is either allocating
a larger buffer on demand or (probably more reliably given the need for
contiguous physical memory) transparently splitting larger transfers.
A super smart implementation would overlap the copy of blocks with the
transfers of preceeding blocks, and there should be something we can do
to avoid having to copy data that's already in suitable memory.

Ideally this would all be factored out into the core based on limit
information but that's probably not required immediately.  Though now I
write that I'm wondering how many other devices should be doing this but
aren't.  The DMA mapping of transfer buffers doesn't seem terribly
device specific...

> I propose add extra module parameter which allows to change buffer size.

No, module parameters are not sensible for modern code.  If you wanted
runtime configuration a sysfs tunable would be better though I do think
that should be tuning optimisation, not a hard limit on the transfer
size.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 3/6] spi: spi-s3c64xx: Add coherent buffers for DMA transfers

[Łukasz Czerwiński]

On 09/10/2013 02:02 PM, Mark Brown wrote:
> On Tue, Sep 10, 2013 at 01:23:45PM +0200, Łukasz Czerwiński wrote:
>> On 09/09/2013 04:45 PM, Mark Brown wrote:
>
>>> This seems like a very low limit to have, consider things like firmware
>>> downloads for example.  It seems reasonable to have a preallocated small
>>> buffer but there should be some fallback for larger transfer sizes.
>
>> I have tested my modification with different buffer sizes with
>> S5C73M3 driver (355560 B upload). I obtained following upload times:
>
>> 16kB buffer:
>> 	- 83.972 ms
>> 	- 79.196 ms
>> 	- 79.432 ms
>> 128kB buffer:
>> 	- 74.449 ms
>> 	- 80.719 ms
>> 	- 75.599 ms
>
>> For 256kB I obtained similar results as in 128kB case. Normally
>> non-interrupted SPI transfer should take 56ms (50MHz). Performance
>> loss is approximately about 6% between 16kB and 128KB buffer. I my
>> opinion there are no need to use bigger buffers.
>
> You're missing the point here.  This requires the client driver to know
> the maximum transfer size that the controller supports and split the
> transfer up for it.  This isn't something that the SPI API supports now
> and while it would be useful to add for controllers that are genuinely
> limited in transfer size it doesn't seem sane to just randomly impose a
> limit to make life easier for software.  Not all applications will be
> able to split their transfers up in the first place and it seems like
> the wrong place to put that knowledge.
>
> What I would expect to see the driver doing here is either allocating
> a larger buffer on demand or (probably more reliably given the need for
> contiguous physical memory) transparently splitting larger transfers.
> A super smart implementation would overlap the copy of blocks with the
> transfers of preceeding blocks, and there should be something we can do
> to avoid having to copy data that's already in suitable memory.
>
Thanks for your comments.
I will try implement transparently splitting larger transfer.

> Ideally this would all be factored out into the core based on limit
> information but that's probably not required immediately.  Though now I
> write that I'm wondering how many other devices should be doing this but
> aren't.  The DMA mapping of transfer buffers doesn't seem terribly
> device specific...
>
>> I propose add extra module parameter which allows to change buffer size.
>
> No, module parameters are not sensible for modern code.  If you wanted
> runtime configuration a sysfs tunable would be better though I do think
> that should be tuning optimisation, not a hard limit on the transfer
> size.
>
ok

Thanks
Lukasz

[RFC PATCH 4/6] spi: spi-s3c64xx: Remove redundant code

[Lukasz Czerwinski]

So far physical address (used to set dma transfer) was obtained from
mapping virtual address of the client buffer address and passed in spi message.
Deprecated code uses dma_map_single() and dma_unmap_single().

Now mapping of virtual address is replaced by data transfer from
client buffer to contiguous regions allocated by dma_alloc_coherent().
Physical address of dma buffers is used to set dma transfer.

This makes functions: s3c64xx_spi_map_mssg(), s3c64xx_spi_unmap()
redundant hence they are removed.

Signed-off-by: Lukasz Czerwinski <l.czerwinski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |   77 ---------------------------------------------
 1 file changed, 77 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 1ec1244..3c20b73 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -716,83 +716,6 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 	}
 }
 
-#define XFER_DMAADDR_INVALID DMA_BIT_MASK(32)
-
-static int s3c64xx_spi_map_mssg(struct s3c64xx_spi_driver_data *sdd,
-						struct spi_message *msg)
-{
-	struct device *dev = &sdd->pdev->dev;
-	struct spi_transfer *xfer;
-
-	if (is_polling(sdd) || msg->is_dma_mapped)
-		return 0;
-
-	/* First mark all xfer unmapped */
-	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
-		xfer->rx_dma = XFER_DMAADDR_INVALID;
-		xfer->tx_dma = XFER_DMAADDR_INVALID;
-	}
-
-	/* Map until end or first fail */
-	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
-
-		if (xfer->len <= ((FIFO_LVL_MASK(sdd) >> 1) + 1))
-			continue;
-
-		if (xfer->tx_buf != NULL) {
-			xfer->tx_dma = dma_map_single(dev,
-					(void *)xfer->tx_buf, xfer->len,
-					DMA_TO_DEVICE);
-			if (dma_mapping_error(dev, xfer->tx_dma)) {
-				dev_err(dev, "dma_map_single Tx failed\n");
-				xfer->tx_dma = XFER_DMAADDR_INVALID;
-				return -ENOMEM;
-			}
-		}
-
-		if (xfer->rx_buf != NULL) {
-			xfer->rx_dma = dma_map_single(dev, xfer->rx_buf,
-						xfer->len, DMA_FROM_DEVICE);
-			if (dma_mapping_error(dev, xfer->rx_dma)) {
-				dev_err(dev, "dma_map_single Rx failed\n");
-				dma_unmap_single(dev, xfer->tx_dma,
-						xfer->len, DMA_TO_DEVICE);
-				xfer->tx_dma = XFER_DMAADDR_INVALID;
-				xfer->rx_dma = XFER_DMAADDR_INVALID;
-				return -ENOMEM;
-			}
-		}
-	}
-
-	return 0;
-}
-
-static void s3c64xx_spi_unmap_mssg(struct s3c64xx_spi_driver_data *sdd,
-						struct spi_message *msg)
-{
-	struct device *dev = &sdd->pdev->dev;
-	struct spi_transfer *xfer;
-
-	if (is_polling(sdd) || msg->is_dma_mapped)
-		return;
-
-	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
-
-		if (xfer->len <= ((FIFO_LVL_MASK(sdd) >> 1) + 1))
-			continue;
-
-		if (xfer->rx_buf != NULL
-				&& xfer->rx_dma != XFER_DMAADDR_INVALID)
-			dma_unmap_single(dev, xfer->rx_dma,
-						xfer->len, DMA_FROM_DEVICE);
-
-		if (xfer->tx_buf != NULL
-				&& xfer->tx_dma != XFER_DMAADDR_INVALID)
-			dma_unmap_single(dev, xfer->tx_dma,
-						xfer->len, DMA_TO_DEVICE);
-	}
-}
-
 static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 					    struct spi_message *msg)
 {
-- 
1.7.9.5

[RFC PATCH 5/6] spi: spi-s3c64xx: Use module_platform_driver()

[Lukasz Czerwinski]

subsys_init_call() initializes driver too early.
It's preventing to move DMA channel allocation at the begining
(driver probe).

This patch reduces and simplifies initalization code by
using module_platform_driver() macro.
It's also efficiently delaying driver startup.

Signed-off-by: Lukasz Czerwinski <l.czerwinski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 3c20b73..9335f66 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1528,22 +1528,13 @@ static struct platform_driver s3c64xx_spi_driver = {
 		.pm = &s3c64xx_spi_pm,
 		.of_match_table = of_match_ptr(s3c64xx_spi_dt_match),
 	},
+	.probe = s3c64xx_spi_probe,
 	.remove = s3c64xx_spi_remove,
 	.id_table = s3c64xx_spi_driver_ids,
 };
 MODULE_ALIAS("platform:s3c64xx-spi");
 
-static int __init s3c64xx_spi_init(void)
-{
-	return platform_driver_probe(&s3c64xx_spi_driver, s3c64xx_spi_probe);
-}
-subsys_initcall(s3c64xx_spi_init);
-
-static void __exit s3c64xx_spi_exit(void)
-{
-	platform_driver_unregister(&s3c64xx_spi_driver);
-}
-module_exit(s3c64xx_spi_exit);
+module_platform_driver(s3c64xx_spi_driver);
 
 MODULE_AUTHOR("Jaswinder Singh <jassi.brar@samsung.com>");
 MODULE_DESCRIPTION("S3C64XX SPI Controller Driver");
-- 
1.7.9.5

Re: [RFC PATCH 5/6] spi: spi-s3c64xx: Use module_platform_driver()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 219 bytes --]

On Mon, Sep 09, 2013 at 04:09:25PM +0200, Lukasz Czerwinski wrote:
> subsys_init_call() initializes driver too early.
> It's preventing to move DMA channel allocation at the begining
> (driver probe).

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization

[Lukasz Czerwinski]

This patch removes DMA channel initialization and deinitialization for each
transfer. Now DMA channel is requested only at the driver initialization
and stored in driver data.

This solution optimizes execution time of the send_message loop because
driver doesn't request DMA channel for each transfer anymore. It uses already
requested DMA channel from s3c64xx_spi_dma_data struct.

Signed-off-by: Lukasz Czerwinski <l.czerwinski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |   98 +++++++++++++++++++--------------------------
 1 file changed, 42 insertions(+), 56 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 9335f66..602e8fa 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -323,59 +323,20 @@ static void s3c64xx_prepare_dma(struct s3c64xx_spi_dma_data *dma, unsigned len)
 static int s3c64xx_spi_prepare_transfer(struct spi_master *spi)
 {
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(spi);
-	dma_filter_fn filter = sdd->cntrlr_info->filter;
 	struct device *dev = &sdd->pdev->dev;
-	dma_cap_mask_t mask;
 	int ret;
 
-	if (!is_polling(sdd)) {
-		dma_cap_zero(mask);
-		dma_cap_set(DMA_SLAVE, mask);
-
-		/* Acquire DMA channels */
-		sdd->rx_dma.ch = dma_request_slave_channel_compat(mask, filter,
-				   (void *)sdd->rx_dma.dmach, dev, "rx");
-		if (!sdd->rx_dma.ch) {
-			dev_err(dev, "Failed to get RX DMA channel\n");
-			ret = -EBUSY;
-			goto out;
-		}
-
-		sdd->tx_dma.ch = dma_request_slave_channel_compat(mask, filter,
-				   (void *)sdd->tx_dma.dmach, dev, "tx");
-		if (!sdd->tx_dma.ch) {
-			dev_err(dev, "Failed to get TX DMA channel\n");
-			ret = -EBUSY;
-			goto out_rx;
-		}
-	}
-
 	ret = pm_runtime_get_sync(&sdd->pdev->dev);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(dev, "Failed to enable device: %d\n", ret);
-		goto out_tx;
-	}
 
 	return 0;
-
-out_tx:
-	dma_release_channel(sdd->tx_dma.ch);
-out_rx:
-	dma_release_channel(sdd->rx_dma.ch);
-out:
-	return ret;
 }
 
 static int s3c64xx_spi_unprepare_transfer(struct spi_master *spi)
 {
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(spi);
 
-	/* Free DMA channels */
-	if (!is_polling(sdd)) {
-		dma_release_channel(sdd->rx_dma.ch);
-		dma_release_channel(sdd->tx_dma.ch);
-	}
-
 	pm_runtime_put(&sdd->pdev->dev);
 	return 0;
 }
@@ -428,7 +389,6 @@ static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 	chcfg &= ~S3C64XX_SPI_CH_TXCH_ON;
 
 	if (dma_mode) {
-		chcfg &= ~S3C64XX_SPI_CH_RXCH_ON;
 	} else {
 		/* Always shift in data in FIFO, even if xfer is Tx only,
 		 * this helps setting PCKT_CNT value for generating clocks
@@ -1121,26 +1081,54 @@ static inline struct s3c64xx_spi_port_config *s3c64xx_spi_get_port_config(
 			 platform_get_device_id(pdev)->driver_data;
 }
 
-static int s3c64xx_init_buffer(struct s3c64xx_spi_driver_data *sdd,
-			  struct s3c64xx_spi_dma_data *dma)
+static int s3c64xx_init_dmach(struct s3c64xx_spi_driver_data *sdd,
+			      struct s3c64xx_spi_dma_data *dma,
+			      bool dma_to_memory)
 {
 	struct device *dev = &sdd->pdev->dev;
+	struct dma_chan *dma_chan;
+	dma_filter_fn filter = sdd->cntrlr_info->filter;
+	dma_cap_mask_t mask;
+	int ret;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+
+	dma_chan = dma_request_slave_channel_compat(mask,
+			filter, (void *)dma->dmach, dev,
+			dma_to_memory ? "rx" : "tx");
+	if (!dma_chan) {
+		dev_err(dev, "Failed to get %d DMA channel\n", dma->dmach);
+		return -EBUSY;
+	}
 
 	dma->vbuf = dma_alloc_coherent(dev, S3C64XX_SPI_DMA_BUF_SIZE,
 			&dma->dma_phys, GFP_KERNEL);
+	if (!dma->vbuf) {
+		ret = -ENOMEM;
+		goto release;
+	}
 
-	if (!dma->vbuf)
-		return -ENOMEM;
+	dma->ch = dma_chan;
 
 	return 0;
+release:
+	dma_release_channel(dma_chan);
+
+	return ret;
+
 }
 
-static void s3c64xx_deinit_buffer(struct s3c64xx_spi_driver_data *sdd,
+static void s3c64xx_deinit_dmach(struct s3c64xx_spi_driver_data *sdd,
 			struct s3c64xx_spi_dma_data *dma)
 {
 	struct device *dev = &sdd->pdev->dev;
 
+	if (!dma->dmach)
+		return;
+
 	dma_free_coherent(dev, sdd->dma_buf_size, dma->vbuf, dma->dma_phys);
+	dma_release_channel(dma->ch);
 }
 
 static int s3c64xx_spi_probe(struct platform_device *pdev)
@@ -1226,14 +1214,12 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	}
 
 	sdd->dma_buf_size = S3C64XX_SPI_DMA_BUF_SIZE;
-	if (s3c64xx_init_buffer(sdd, &sdd->rx_dma) < 0) {
-		ret = -ENOMEM;
+	ret = s3c64xx_init_dmach(sdd, &sdd->rx_dma, true);
+	if (ret < 0)
 		goto err0;
-	}
-	if (s3c64xx_init_buffer(sdd, &sdd->tx_dma) < 0) {
-		ret = -ENOMEM;
+	ret = s3c64xx_init_dmach(sdd, &sdd->tx_dma, false);
+	if (ret < 0)
 		goto err0;
-	}
 
 	sdd->tx_dma.direction = DMA_MEM_TO_DEV;
 	sdd->rx_dma.direction = DMA_DEV_TO_MEM;
@@ -1334,9 +1320,9 @@ err2:
 	clk_disable_unprepare(sdd->clk);
 err0:
 	if (sdd->rx_dma.vbuf)
-		s3c64xx_deinit_buffer(sdd, &sdd->rx_dma);
+		s3c64xx_deinit_dmach(sdd, &sdd->rx_dma);
 	if (sdd->tx_dma.vbuf)
-		s3c64xx_deinit_buffer(sdd, &sdd->tx_dma);
+		s3c64xx_deinit_dmach(sdd, &sdd->tx_dma);
 
 	spi_master_put(master);
 
@@ -1355,10 +1341,10 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
 
 	if (sdd->rx_dma.vbuf)
-		s3c64xx_deinit_buffer(sdd, &sdd->rx_dma);
+		s3c64xx_deinit_dmach(sdd, &sdd->rx_dma);
 
 	if (sdd->tx_dma.vbuf)
-		s3c64xx_deinit_buffer(sdd, &sdd->tx_dma);
+		s3c64xx_deinit_dmach(sdd, &sdd->tx_dma);
 
 	clk_disable_unprepare(sdd->src_clk);
 
-- 
1.7.9.5

Re: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On Mon, Sep 09, 2013 at 04:09:26PM +0200, Lukasz Czerwinski wrote:
> This patch removes DMA channel initialization and deinitialization for each
> transfer. Now DMA channel is requested only at the driver initialization
> and stored in driver data.

So, this is fine but I had been under the impression that one of the
reasons that the channels were only being requested at transfer time was
that they could be used for other purposes too.  If that is not the case
then fine, if that is the case then probably moving to prepare/unprepare
transfer hardware (when the clocks are enabled) would make more sense -
that'd avoid repeated allocations and frees while the controller is busy
which should be the main issue performance wise.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization

[Łukasz Czerwiński]

On 09/09/2013 04:53 PM, Mark Brown wrote:
> On Mon, Sep 09, 2013 at 04:09:26PM +0200, Lukasz Czerwinski wrote:
>> This patch removes DMA channel initialization and deinitialization for each
>> transfer. Now DMA channel is requested only at the driver initialization
>> and stored in driver data.
>
> So, this is fine but I had been under the impression that one of the
> reasons that the channels were only being requested at transfer time was
> that they could be used for other purposes too.  If that is not the case
> then fine, if that is the case then probably moving to prepare/unprepare
> transfer hardware (when the clocks are enabled) would make more sense -
> that'd avoid repeated allocations and frees while the controller is busy
> which should be the main issue performance wise.
>
For 16kB buffer upload time is reduced from ~90ms to ~80ms (I tested 
with S5C73M3 355560B transfer). If you think that performance increase 
isn't valuable we should skip that patch.

amba-pl80 uses virtual DMA channels support so driver won't occupy all 
the time physical channel. pl330 driver don't use such framework but It 
will be very hard to occupy all channels at the same time. If I'm wrong 
please correct me.

Thanks
Lukasz

Re: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Tue, Sep 10, 2013 at 01:24:01PM +0200, Łukasz Czerwiński wrote:

> For 16kB buffer upload time is reduced from ~90ms to ~80ms (I tested
> with S5C73M3 355560B transfer). If you think that performance
> increase isn't valuable we should skip that patch.

That does seem worthwhile.  Is that just a single transfer in isolation?

> amba-pl80 uses virtual DMA channels support so driver won't occupy
> all the time physical channel. pl330 driver don't use such framework
> but It will be very hard to occupy all channels at the same time. If
> I'm wrong please correct me.

OK.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization

[Łukasz Czerwiński]

On 09/10/2013 02:03 PM, Mark Brown wrote:
> On Tue, Sep 10, 2013 at 01:24:01PM +0200, Łukasz Czerwiński wrote:
>
>> For 16kB buffer upload time is reduced from ~90ms to ~80ms (I tested
>> with S5C73M3 355560B transfer). If you think that performance
>> increase isn't valuable we should skip that patch.
>
> That does seem worthwhile.  Is that just a single transfer in isolation?
>
I gave you summary time when firmware is transferred via 22x16kB blocks.
Each transfer was isolated.
>> amba-pl80 uses virtual DMA channels support so driver won't occupy
>> all the time physical channel. pl330 driver don't use such framework
>> but It will be very hard to occupy all channels at the same time. If
>> I'm wrong please correct me.
>
> OK.
>
Thanks
Lukasz

Re: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

On Tue, Sep 10, 2013 at 03:16:59PM +0200, Łukasz Czerwiński wrote:
> On 09/10/2013 02:03 PM, Mark Brown wrote:
> >On Tue, Sep 10, 2013 at 01:24:01PM +0200, Łukasz Czerwiński wrote:

> >>For 16kB buffer upload time is reduced from ~90ms to ~80ms (I tested
> >>with S5C73M3 355560B transfer). If you think that performance
> >>increase isn't valuable we should skip that patch.

> >That does seem worthwhile.  Is that just a single transfer in isolation?

> I gave you summary time when firmware is transferred via 22x16kB blocks.
> Each transfer was isolated.

OK, so that's the per-transfer number?  I'm just wondering if moving it
to prepare/unprepare would achieve the same effect here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization

[Łukasz Czerwiński]

On 09/10/2013 06:32 PM, Mark Brown wrote:
> On Tue, Sep 10, 2013 at 03:16:59PM +0200, Łukasz Czerwiński wrote:
>> On 09/10/2013 02:03 PM, Mark Brown wrote:
>>> On Tue, Sep 10, 2013 at 01:24:01PM +0200, Łukasz Czerwiński wrote:
>
>>>> For 16kB buffer upload time is reduced from ~90ms to ~80ms (I tested
>>>> with S5C73M3 355560B transfer). If you think that performance
>>>> increase isn't valuable we should skip that patch.
>
>>> That does seem worthwhile.  Is that just a single transfer in isolation?
>
>> I gave you summary time when firmware is transferred via 22x16kB blocks.
>> Each transfer was isolated.
>
> OK, so that's the per-transfer number?  I'm just wondering if moving it
> to prepare/unprepare would achieve the same effect here.
>
Yes

You are right.

S5C73M3 splits firmware into several spi_messages with single SPI 
transfer.  It caused for each single spi_message transfer 
prepare/unprepare.  I tried mistakenly optimize through moving 
dma_channel_request(in prepare) from prepare to probe(do it once).

I based my patch on the spi-tegra114.c driver. 	There is 
dma_chanel_request at the beginning in the probe function.
I really should correct S5C73M3 firmware upload function.

When I added small modification into S5C73M3 driver I achieved the same 
effect.

Sorry for wasting your time.

Thanks
Lukasz

Re: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Wed, Sep 11, 2013 at 09:45:58AM +0200, Łukasz Czerwiński wrote:

> I based my patch on the spi-tegra114.c driver. 	There is
> dma_chanel_request at the beginning in the probe function.
> I really should correct S5C73M3 firmware upload function.

Yes, that can be the best idea in cases where the DMA channel is tied to
the hardware block it's controlling so there's no possibility that
something else could make use of it when idle.  It does also avoid the
possibility of getting the DMA controller busy when you try to use it
but it depends how oversubscribed the DMA controller is.

> When I added small modification into S5C73M3 driver I achieved the
> same effect.

> Sorry for wasting your time.

No problem.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]