[PATCH 1/3] spi: sirf: provide a shortcut for spi command-data mode

[PATCH 1/3] spi: sirf: provide a shortcut for spi command-data mode

[Barry Song]

From: Qipan Li <Qipan.Li-kQvG35nSl+M@public.gmane.org>

there are many SPI clients which use the following protocal:
step 1: send command bytes to clients(rx buffer is empty)
step 2: send data bytes to clients or receive data bytes from
clients.
SiRFprimaII provides a shortcut for this kind of SPI transfer.
when tx buf is less or equal than 4 bytes and rx buf is null
in a transfer, we think it as 'command' data and use hardware
command register for the transfer.
here we can save some CPU loading than doing both tx and rx
for a normal transfer.

Signed-off-by: Qipan Li <Qipan.Li-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 drivers/spi/spi-sirf.c |   39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-sirf.c b/drivers/spi/spi-sirf.c
index e430689..c400ab7 100644
--- a/drivers/spi/spi-sirf.c
+++ b/drivers/spi/spi-sirf.c
@@ -132,6 +132,7 @@
 #define IS_DMA_VALID(x) (x && ALIGNED(x->tx_buf) && ALIGNED(x->rx_buf) && \
 	ALIGNED(x->len) && (x->len < 2 * PAGE_SIZE))
 
+#define SIRFSOC_MAX_CMD_BYTES	4
 struct sirfsoc_spi {
 	struct spi_bitbang bitbang;
 	struct completion rx_done;
@@ -161,7 +162,7 @@ struct sirfsoc_spi {
 	dma_addr_t dst_start;
 	void *dummypage;
 	int word_width; /* in bytes */
-
+	bool	tx_by_cmd;
 	int chipselect[0];
 };
 
@@ -260,6 +261,11 @@ static irqreturn_t spi_sirfsoc_irq(int irq, void *dev_id)
 
 	writel(spi_stat, sspi->base + SIRFSOC_SPI_INT_STATUS);
 
+	if (sspi->tx_by_cmd && (spi_stat & SIRFSOC_SPI_FRM_END)) {
+		complete(&sspi->tx_done);
+		writel(0x0, sspi->base + SIRFSOC_SPI_INT_EN);
+		return IRQ_HANDLED;
+	}
 	/* Error Conditions */
 	if (spi_stat & SIRFSOC_SPI_RX_OFLOW ||
 			spi_stat & SIRFSOC_SPI_TX_UFLOW) {
@@ -310,6 +316,32 @@ static int spi_sirfsoc_transfer(struct spi_device *spi, struct spi_transfer *t)
 
 	writel(SIRFSOC_SPI_INT_MASK_ALL, sspi->base + SIRFSOC_SPI_INT_STATUS);
 
+	if (t && t->tx_buf && !t->rx_buf && (t->len <= SIRFSOC_MAX_CMD_BYTES)) {
+		const u32 *cmd_ptr;
+		u32 cmd;
+		sspi->tx_by_cmd = true;
+		cmd_ptr = sspi->tx;
+		cmd = *cmd_ptr;
+		if (sspi->word_width == 1 && !(spi->mode & SPI_LSB_FIRST))
+			cmd = cpu_to_be32(cmd) >>
+				((SIRFSOC_MAX_CMD_BYTES - t->len) * 8);
+		if (sspi->word_width == 2 && t->len == 4 &&
+				(!(spi->mode & SPI_LSB_FIRST)))
+			cmd = ((cmd & 0xffff) << 16) | (cmd >> 16);
+		writel(cmd, sspi->base + SIRFSOC_SPI_CMD);
+
+		writel(SIRFSOC_SPI_FRM_END_INT_EN,
+				sspi->base + SIRFSOC_SPI_INT_EN);
+		writel(SIRFSOC_SPI_CMD_TX_EN,
+				sspi->base + SIRFSOC_SPI_TX_RX_EN);
+
+		if (wait_for_completion_timeout(&sspi->tx_done, timeout) == 0) {
+			dev_err(&spi->dev, "transfer timeout\n");
+			return 0;
+		}
+		return t->len;
+	} else
+		sspi->tx_by_cmd = false;
 	if (sspi->left_tx_word == 1) {
 		writel(readl(sspi->base + SIRFSOC_SPI_CTRL) |
 			SIRFSOC_SPI_ENA_AUTO_CLR,
@@ -519,6 +551,11 @@ spi_sirfsoc_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
 	writel(txfifo_ctrl, sspi->base + SIRFSOC_SPI_TXFIFO_CTRL);
 	writel(rxfifo_ctrl, sspi->base + SIRFSOC_SPI_RXFIFO_CTRL);
 
+	if (t && t->tx_buf && !t->rx_buf && (t->len <= SIRFSOC_MAX_CMD_BYTES))
+		regval |= (SIRFSOC_SPI_CMD_BYTE_NUM((t->len - 1)) |
+				SIRFSOC_SPI_CMD_MODE);
+	else
+		regval &= ~SIRFSOC_SPI_CMD_MODE;
 	writel(regval, sspi->base + SIRFSOC_SPI_CTRL);
 
 	if (IS_DMA_VALID(t)) {
-- 
1.7.9.5

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

[PATCH 2/3] spi: sirf: move to use generic dma dt-binding

[Barry Song]

From: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>

sirf-dma driver enabled generic dt binding for dma channels.
see here we remove self-defined dma channel prop and move to
use generic dma_request_slave_channel.
related changes in dts is something like:
    dmas = <&dmac1 9>,
    <&dmac1 4>;
    dma-names = "rx", "tx";

Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 drivers/spi/spi-sirf.c |   26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-sirf.c b/drivers/spi/spi-sirf.c
index c400ab7..a213e87 100644
--- a/drivers/spi/spi-sirf.c
+++ b/drivers/spi/spi-sirf.c
@@ -22,7 +22,6 @@
 #include <linux/dmaengine.h>
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
-#include <linux/sirfsoc_dma.h>
 
 #define DRIVER_NAME "sirfsoc_spi"
 
@@ -585,8 +584,6 @@ static int spi_sirfsoc_probe(struct platform_device *pdev)
 	struct spi_master *master;
 	struct resource *mem_res;
 	int num_cs, cs_gpio, irq;
-	u32 rx_dma_ch, tx_dma_ch;
-	dma_cap_mask_t dma_cap_mask;
 	int i;
 	int ret;
 
@@ -597,20 +594,6 @@ static int spi_sirfsoc_probe(struct platform_device *pdev)
 		goto err_cs;
 	}
 
-	ret = of_property_read_u32(pdev->dev.of_node,
-			"sirf,spi-dma-rx-channel", &rx_dma_ch);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "Unable to get rx dma channel\n");
-		goto err_cs;
-	}
-
-	ret = of_property_read_u32(pdev->dev.of_node,
-			"sirf,spi-dma-tx-channel", &tx_dma_ch);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "Unable to get tx dma channel\n");
-		goto err_cs;
-	}
-
 	master = spi_alloc_master(&pdev->dev, sizeof(*sspi) + sizeof(int) * num_cs);
 	if (!master) {
 		dev_err(&pdev->dev, "Unable to allocate SPI master\n");
@@ -674,18 +657,13 @@ static int spi_sirfsoc_probe(struct platform_device *pdev)
 	sspi->bitbang.master->dev.of_node = pdev->dev.of_node;
 
 	/* request DMA channels */
-	dma_cap_zero(dma_cap_mask);
-	dma_cap_set(DMA_INTERLEAVE, dma_cap_mask);
-
-	sspi->rx_chan = dma_request_channel(dma_cap_mask, (dma_filter_fn)sirfsoc_dma_filter_id,
-		(void *)rx_dma_ch);
+	sspi->rx_chan = dma_request_slave_channel(&pdev->dev, "rx");
 	if (!sspi->rx_chan) {
 		dev_err(&pdev->dev, "can not allocate rx dma channel\n");
 		ret = -ENODEV;
 		goto free_master;
 	}
-	sspi->tx_chan = dma_request_channel(dma_cap_mask, (dma_filter_fn)sirfsoc_dma_filter_id,
-		(void *)tx_dma_ch);
+	sspi->tx_chan = dma_request_slave_channel(&pdev->dev, "tx");
 	if (!sspi->tx_chan) {
 		dev_err(&pdev->dev, "can not allocate tx dma channel\n");
 		ret = -ENODEV;
-- 
1.7.9.5

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

Re: [PATCH 2/3] spi: sirf: move to use generic dma dt-binding

[Mark Brown]

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

On Thu, Feb 13, 2014 at 12:30:19AM +0800, Barry Song wrote:
> From: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
> 
> sirf-dma driver enabled generic dt binding for dma channels.
> see here we remove self-defined dma channel prop and move to
> use generic dma_request_slave_channel.
> related changes in dts is something like:

Applied, thanks.

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

[PATCH 3/3] spi: sirf: use SET_SYSTEM_SLEEP_PM_OPS to initialize PM entries

[Barry Song]

From: Qipan Li <Qipan.Li-kQvG35nSl+M@public.gmane.org>

use SET_SYSTEM_SLEEP_PM_OPS to initialize PM entries, this makes the codes
clean and also enable the ability of hibernation support for sirf SPI.

Signed-off-by: Qipan Li <Qipan.Li-kQvG35nSl+M@public.gmane.org>
Signed-off-by: Barry Song <Baohua.Song-kQvG35nSl+M@public.gmane.org>
---
 drivers/spi/spi-sirf.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-sirf.c b/drivers/spi/spi-sirf.c
index a213e87..5bffa5f 100644
--- a/drivers/spi/spi-sirf.c
+++ b/drivers/spi/spi-sirf.c
@@ -739,7 +739,7 @@ static int  spi_sirfsoc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
 static int spi_sirfsoc_suspend(struct device *dev)
 {
 	struct spi_master *master = dev_get_drvdata(dev);
@@ -762,12 +762,11 @@ static int spi_sirfsoc_resume(struct device *dev)
 
 	return 0;
 }
+#endif
 
 static const struct dev_pm_ops spi_sirfsoc_pm_ops = {
-	.suspend = spi_sirfsoc_suspend,
-	.resume = spi_sirfsoc_resume,
+	SET_SYSTEM_SLEEP_PM_OPS(spi_sirfsoc_suspend, spi_sirfsoc_resume)
 };
-#endif
 
 static const struct of_device_id spi_sirfsoc_of_match[] = {
 	{ .compatible = "sirf,prima2-spi", },
@@ -780,9 +779,7 @@ static struct platform_driver spi_sirfsoc_driver = {
 	.driver = {
 		.name = DRIVER_NAME,
 		.owner = THIS_MODULE,
-#ifdef CONFIG_PM
 		.pm     = &spi_sirfsoc_pm_ops,
-#endif
 		.of_match_table = spi_sirfsoc_of_match,
 	},
 	.probe = spi_sirfsoc_probe,
-- 
1.7.9.5

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

Re: [PATCH 3/3] spi: sirf: use SET_SYSTEM_SLEEP_PM_OPS to initialize PM entries

[Mark Brown]

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

On Thu, Feb 13, 2014 at 12:30:20AM +0800, Barry Song wrote:
> From: Qipan Li <Qipan.Li-kQvG35nSl+M@public.gmane.org>
> 
> use SET_SYSTEM_SLEEP_PM_OPS to initialize PM entries, this makes the codes
> clean and also enable the ability of hibernation support for sirf SPI.

Applied, thanks.

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

Re: [PATCH 1/3] spi: sirf: provide a shortcut for spi command-data mode

[Mark Brown]

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

On Thu, Feb 13, 2014 at 12:30:18AM +0800, Barry Song wrote:
> From: Qipan Li <Qipan.Li-kQvG35nSl+M@public.gmane.org>
> 
> there are many SPI clients which use the following protocal:
> step 1: send command bytes to clients(rx buffer is empty)
> step 2: send data bytes to clients or receive data bytes from
> clients.

The code here is a bit impenetrable, it could probably benefit from some
comments or possibly refactoring to split out the command based
transfers into a separate function so you end up with this:

> +	if (t && t->tx_buf && !t->rx_buf && (t->len <= SIRFSOC_MAX_CMD_BYTES)) {

		do_cmd_xfer();
	else
		do_normal_xfer();

> +		const u32 *cmd_ptr;
> +		u32 cmd;
> +		sspi->tx_by_cmd = true;
> +		cmd_ptr = sspi->tx;
> +		cmd = *cmd_ptr;
> +		if (sspi->word_width == 1 && !(spi->mode & SPI_LSB_FIRST))
> +			cmd = cpu_to_be32(cmd) >>
> +				((SIRFSOC_MAX_CMD_BYTES - t->len) * 8);
> +		if (sspi->word_width == 2 && t->len == 4 &&
> +				(!(spi->mode & SPI_LSB_FIRST)))
> +			cmd = ((cmd & 0xffff) << 16) | (cmd >> 16);

The above dererferences cmd_ptr no matter what the actual length is.  If
it's 4 bytes that's fine but if it's less this means the code is reading
beyond the end of the buffer.  Most likely this is going to be safe but
it's out of spec and could result in doing something that messes with
DMA mappings.  A memcpy() would be safer.

This is also a bit unclear since the code is so densely written, more
whitespace would help, as would avoiding splitting the set of
assignments you use to avoid casts (but like I say they seem unsafe
anyway).

> +		return t->len;
> +	} else
> +		sspi->tx_by_cmd = false;

You should have { } on either both sides or only one side of the if.

> +	if (t && t->tx_buf && !t->rx_buf && (t->len <= SIRFSOC_MAX_CMD_BYTES))
> +		regval |= (SIRFSOC_SPI_CMD_BYTE_NUM((t->len - 1)) |
> +				SIRFSOC_SPI_CMD_MODE);
> +	else
> +		regval &= ~SIRFSOC_SPI_CMD_MODE;

This check is being repeated in two places which doesn't feel 100% safe,
why not use tx_by_cmd?

What the code is doing is basically fine but it's quite hard to read.

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