* [PATCH RESEND 0/2] spi: sun4i: add DMA support
@ 2016-02-26 5:56 Priit Laes
2016-02-26 5:56 ` [PATCH 1/2] " Priit Laes
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Priit Laes @ 2016-02-26 5:56 UTC (permalink / raw)
To: Mark Brown, Maxime Ripard, Chen-Yu Tsai
Cc: linux-spi, linux-arm-kernel, linux-kernel, Emilio López,
Michal Suchanek, Priit Laes
While trying to get SPI TFT screen working with Cubietruck, I noticed that
at current state driver didn't actually support SPI data bursts bigger than
n bytes (n is probably ~64 bytes).
Patches below were sent to mailinglists a while ago (May 2015) but apparently
were not addressed to correct maintainers and therefore were never applied.
With those patches, it's now possible to use TFT screens over SPI (fbtft
driver in staging), although there seems to be a possible issue where bigger
data bursts cause the driver to temporarily fall back to PIO mode:
spi_master spi32766: Using DMA mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using DMA mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
spi_master spi32766: Using PIO mode for transfer
Emilio López (1):
spi: sun4i: add DMA support
Michal Suchanek (1):
ARM: sunxi: spi: add notice about SPI FIFO limit.
drivers/spi/spi-sun4i.c | 142 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 130 insertions(+), 12 deletions(-)
--
2.7.2
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/2] spi: sun4i: add DMA support 2016-02-26 5:56 [PATCH RESEND 0/2] spi: sun4i: add DMA support Priit Laes @ 2016-02-26 5:56 ` Priit Laes 2016-02-26 12:25 ` Mark Brown 2016-02-26 5:56 ` [PATCH 2/2] ARM: sunxi: spi: add notice about SPI FIFO limit Priit Laes 2016-02-26 12:15 ` [PATCH RESEND 0/2] spi: sun4i: add DMA support Mark Brown 2 siblings, 1 reply; 16+ messages in thread From: Priit Laes @ 2016-02-26 5:56 UTC (permalink / raw) To: Mark Brown, Maxime Ripard, Chen-Yu Tsai Cc: linux-spi, linux-arm-kernel, linux-kernel, Emilio López, Michal Suchanek From: Emilio López <emilio@elopez.com.ar> This patch adds support for 64 byte or bigger transfers on the sun4i SPI controller. Said transfers will be performed via DMA. Signed-off-by: Emilio López <emilio@elopez.com.ar> Tested-by: Michal Suchanek <hramrach@gmail.com> Tested-by: Priit Laes <plaes@plaes.org> --- drivers/spi/spi-sun4i.c | 140 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 128 insertions(+), 12 deletions(-) diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c index 1ddd9e2..78141a6 100644 --- a/drivers/spi/spi-sun4i.c +++ b/drivers/spi/spi-sun4i.c @@ -14,6 +14,8 @@ #include <linux/clk.h> #include <linux/delay.h> #include <linux/device.h> +#include <linux/dmaengine.h> +#include <linux/dma-mapping.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> @@ -34,6 +36,7 @@ #define SUN4I_CTL_CPHA BIT(2) #define SUN4I_CTL_CPOL BIT(3) #define SUN4I_CTL_CS_ACTIVE_LOW BIT(4) +#define SUN4I_CTL_DMAMC_DEDICATED BIT(5) #define SUN4I_CTL_LMTF BIT(6) #define SUN4I_CTL_TF_RST BIT(8) #define SUN4I_CTL_RF_RST BIT(9) @@ -51,6 +54,8 @@ #define SUN4I_INT_STA_REG 0x10 #define SUN4I_DMA_CTL_REG 0x14 +#define SUN4I_DMA_CTL_RF_READY BIT(0) +#define SUN4I_DMA_CTL_TF_NOT_FULL BIT(10) #define SUN4I_WAIT_REG 0x18 @@ -130,6 +135,13 @@ static inline void sun4i_spi_fill_fifo(struct sun4i_spi *sspi, int len) } } +static bool sun4i_spi_can_dma(struct spi_master *master, + struct spi_device *spi, + struct spi_transfer *tfr) +{ + return tfr->len >= SUN4I_FIFO_DEPTH; +} + static void sun4i_spi_set_cs(struct spi_device *spi, bool enable) { struct sun4i_spi *sspi = spi_master_get_devdata(spi->master); @@ -172,14 +184,11 @@ static int sun4i_spi_transfer_one(struct spi_master *master, struct spi_transfer *tfr) { struct sun4i_spi *sspi = spi_master_get_devdata(master); + struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL; unsigned int mclk_rate, div, timeout; unsigned int tx_len = 0; + u32 reg, trigger = 0; int ret = 0; - u32 reg; - - /* We don't support transfer larger than the FIFO */ - if (tfr->len > SUN4I_FIFO_DEPTH) - return -EINVAL; reinit_completion(&sspi->done); sspi->tx_buf = tfr->tx_buf; @@ -189,7 +198,6 @@ static int sun4i_spi_transfer_one(struct spi_master *master, /* Clear pending interrupts */ sun4i_spi_write(sspi, SUN4I_INT_STA_REG, ~0); - reg = sun4i_spi_read(sspi, SUN4I_CTL_REG); /* Reset FIFOs */ @@ -269,12 +277,65 @@ static int sun4i_spi_transfer_one(struct spi_master *master, sun4i_spi_write(sspi, SUN4I_BURST_CNT_REG, SUN4I_BURST_CNT(tfr->len)); sun4i_spi_write(sspi, SUN4I_XMIT_CNT_REG, SUN4I_XMIT_CNT(tx_len)); - /* Fill the TX FIFO */ - sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH); - /* Enable the interrupts */ sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, SUN4I_INT_CTL_TC); + if (sun4i_spi_can_dma(master, spi, tfr)) { + dev_dbg(&sspi->master->dev, "Using DMA mode for transfer\n"); + + if (sspi->tx_buf) { + desc_tx = dmaengine_prep_slave_sg(master->dma_tx, + tfr->tx_sg.sgl, tfr->tx_sg.nents, + DMA_TO_DEVICE, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + if (!desc_tx) { + dev_err(&sspi->master->dev, + "Couldn't prepare dma slave\n"); + return -EIO; + } + + trigger |= SUN4I_DMA_CTL_TF_NOT_FULL; + + dmaengine_submit(desc_tx); + dma_async_issue_pending(master->dma_tx); + + } + + if (sspi->rx_buf) { + desc_rx = dmaengine_prep_slave_sg(master->dma_rx, + tfr->rx_sg.sgl, tfr->rx_sg.nents, + DMA_FROM_DEVICE, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + if (!desc_rx) { + dev_err(&sspi->master->dev, + "Couldn't prepare dma slave\n"); + return -EIO; + } + + trigger |= SUN4I_DMA_CTL_RF_READY; + + dmaengine_submit(desc_rx); + dma_async_issue_pending(master->dma_rx); + } + + /* Enable Dedicated DMA requests */ + reg = sun4i_spi_read(sspi, SUN4I_CTL_REG); + reg |= SUN4I_CTL_DMAMC_DEDICATED; + sun4i_spi_write(sspi, SUN4I_CTL_REG, reg); + sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, trigger); + } else { + dev_dbg(&sspi->master->dev, "Using PIO mode for transfer\n"); + + /* Disable DMA requests */ + reg = sun4i_spi_read(sspi, SUN4I_CTL_REG); + sun4i_spi_write(sspi, SUN4I_CTL_REG, + reg & ~SUN4I_CTL_DMAMC_DEDICATED); + sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0); + + /* Fill the TX FIFO */ + sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH); + } + /* Start the transfer */ reg = sun4i_spi_read(sspi, SUN4I_CTL_REG); sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH); @@ -286,7 +347,12 @@ static int sun4i_spi_transfer_one(struct spi_master *master, goto out; } - sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH); + if (sun4i_spi_can_dma(master, spi, tfr) && desc_rx) { + /* The receive transfer should be the last one to finish */ + dma_wait_for_async_tx(desc_rx); + } else { + sun4i_spi_drain_fifo(sspi, SUN4I_FIFO_DEPTH); + } out: sun4i_spi_write(sspi, SUN4I_INT_CTL_REG, 0); @@ -351,6 +417,7 @@ static int sun4i_spi_runtime_suspend(struct device *dev) static int sun4i_spi_probe(struct platform_device *pdev) { + struct dma_slave_config dma_sconfig; struct spi_master *master; struct sun4i_spi *sspi; struct resource *res; @@ -386,7 +453,9 @@ static int sun4i_spi_probe(struct platform_device *pdev) goto err_free_master; } + init_completion(&sspi->done); sspi->master = master; + master->can_dma = sun4i_spi_can_dma; master->set_cs = sun4i_spi_set_cs; master->transfer_one = sun4i_spi_transfer_one; master->num_chipselect = 4; @@ -409,7 +478,45 @@ static int sun4i_spi_probe(struct platform_device *pdev) goto err_free_master; } - init_completion(&sspi->done); + master->dma_tx = dma_request_slave_channel_reason(&pdev->dev, "tx"); + if (IS_ERR(master->dma_tx)) { + dev_err(&pdev->dev, "Unable to acquire DMA channel TX\n"); + ret = PTR_ERR(master->dma_tx); + goto err_free_master; + } + + dma_sconfig.direction = DMA_MEM_TO_DEV; + dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE; + dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE; + dma_sconfig.dst_addr = res->start + SUN4I_TXDATA_REG; + dma_sconfig.src_maxburst = 1; + dma_sconfig.dst_maxburst = 1; + + ret = dmaengine_slave_config(master->dma_tx, &dma_sconfig); + if (ret) { + dev_err(&pdev->dev, "Unable to configure TX DMA slave\n"); + goto err_tx_dma_release; + } + + master->dma_rx = dma_request_slave_channel_reason(&pdev->dev, "rx"); + if (IS_ERR(master->dma_rx)) { + dev_err(&pdev->dev, "Unable to acquire DMA channel RX\n"); + ret = PTR_ERR(master->dma_rx); + goto err_tx_dma_release; + } + + dma_sconfig.direction = DMA_DEV_TO_MEM; + dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE; + dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE; + dma_sconfig.src_addr = res->start + SUN4I_RXDATA_REG; + dma_sconfig.src_maxburst = 1; + dma_sconfig.dst_maxburst = 1; + + ret = dmaengine_slave_config(master->dma_rx, &dma_sconfig); + if (ret) { + dev_err(&pdev->dev, "Unable to configure RX DMA slave\n"); + goto err_rx_dma_release; + } /* * This wake-up/shutdown pattern is to be able to have the @@ -418,7 +525,7 @@ static int sun4i_spi_probe(struct platform_device *pdev) ret = sun4i_spi_runtime_resume(&pdev->dev); if (ret) { dev_err(&pdev->dev, "Couldn't resume the device\n"); - goto err_free_master; + goto err_rx_dma_release; } pm_runtime_set_active(&pdev->dev); @@ -436,6 +543,10 @@ static int sun4i_spi_probe(struct platform_device *pdev) err_pm_disable: pm_runtime_disable(&pdev->dev); sun4i_spi_runtime_suspend(&pdev->dev); +err_rx_dma_release: + dma_release_channel(master->dma_rx); +err_tx_dma_release: + dma_release_channel(master->dma_tx); err_free_master: spi_master_put(master); return ret; @@ -443,8 +554,13 @@ err_free_master: static int sun4i_spi_remove(struct platform_device *pdev) { + struct spi_master *master = platform_get_drvdata(pdev); + pm_runtime_disable(&pdev->dev); + dma_release_channel(master->dma_rx); + dma_release_channel(master->dma_tx); + return 0; } -- 2.7.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-02-26 5:56 ` [PATCH 1/2] " Priit Laes @ 2016-02-26 12:25 ` Mark Brown 2016-02-26 12:51 ` Michal Suchanek 0 siblings, 1 reply; 16+ messages in thread From: Mark Brown @ 2016-02-26 12:25 UTC (permalink / raw) To: Priit Laes Cc: Maxime Ripard, Chen-Yu Tsai, linux-spi, linux-arm-kernel, linux-kernel, Emilio López, Michal Suchanek [-- Attachment #1: Type: text/plain, Size: 485 bytes --] On Fri, Feb 26, 2016 at 07:56:56AM +0200, Priit Laes wrote: > From: Emilio López <emilio@elopez.com.ar> > > This patch adds support for 64 byte or bigger transfers on the > sun4i SPI controller. Said transfers will be performed via DMA. > > Signed-off-by: Emilio López <emilio@elopez.com.ar> > Tested-by: Michal Suchanek <hramrach@gmail.com> > Tested-by: Priit Laes <plaes@plaes.org> > --- You *must* sign off any patches you are sending, please see SubmittingPatches. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-02-26 12:25 ` Mark Brown @ 2016-02-26 12:51 ` Michal Suchanek 2016-03-06 21:42 ` Maxime Ripard 0 siblings, 1 reply; 16+ messages in thread From: Michal Suchanek @ 2016-02-26 12:51 UTC (permalink / raw) To: Mark Brown Cc: Priit Laes, Maxime Ripard, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López Hello, On 26 February 2016 at 13:25, Mark Brown <broonie@kernel.org> wrote: > On Fri, Feb 26, 2016 at 07:56:56AM +0200, Priit Laes wrote: >> From: Emilio López <emilio@elopez.com.ar> >> >> This patch adds support for 64 byte or bigger transfers on the >> sun4i SPI controller. Said transfers will be performed via DMA. >> >> Signed-off-by: Emilio López <emilio@elopez.com.ar> >> Tested-by: Michal Suchanek <hramrach@gmail.com> >> Tested-by: Priit Laes <plaes@plaes.org> >> --- > > You *must* sign off any patches you are sending, please see > SubmittingPatches. I have sent these patches in the past. Besides this non-technical objection there were multiple technical objections. IIRC one was that the driver does not handle the case when the DMA channels are not available. As I understand it the channels are exclusively reserved for a particular peripherial on sunxi platform so this ShoulNotHappen(tm). So it's probably fine for the driver to fail probe when you have broken DT or no DMA engine support for sunxi platform. The driver could also work fully without DMA and there is a patch for that also but since nobody will be testing that codepath it's probably better to not include it. I have not addressed the other objections so far. Thanks Michal ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-02-26 12:51 ` Michal Suchanek @ 2016-03-06 21:42 ` Maxime Ripard 2016-03-10 9:01 ` Michal Suchanek 0 siblings, 1 reply; 16+ messages in thread From: Maxime Ripard @ 2016-03-06 21:42 UTC (permalink / raw) To: Michal Suchanek Cc: Mark Brown, Priit Laes, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López [-- Attachment #1: Type: text/plain, Size: 1519 bytes --] On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote: > Hello, > > On 26 February 2016 at 13:25, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Feb 26, 2016 at 07:56:56AM +0200, Priit Laes wrote: > >> From: Emilio López <emilio@elopez.com.ar> > >> > >> This patch adds support for 64 byte or bigger transfers on the > >> sun4i SPI controller. Said transfers will be performed via DMA. > >> > >> Signed-off-by: Emilio López <emilio@elopez.com.ar> > >> Tested-by: Michal Suchanek <hramrach@gmail.com> > >> Tested-by: Priit Laes <plaes@plaes.org> > >> --- > > > > You *must* sign off any patches you are sending, please see > > SubmittingPatches. > > I have sent these patches in the past. Mike's point was that since it's Priit that he's submitting the patches, he must add his SoB. > Besides this non-technical objection there were multiple technical > objections. > > IIRC one was that the driver does not handle the case when the DMA > channels are not available. As I understand it the channels are > exclusively reserved for a particular peripherial on sunxi platform so > this ShoulNotHappen(tm). So it's probably fine for the driver to fail > probe when you have broken DT or no DMA engine support for sunxi > platform. There's a quite trivial scenario that would trigger this: if the dma driver or dmaengine is not enabled / loaded. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-03-06 21:42 ` Maxime Ripard @ 2016-03-10 9:01 ` Michal Suchanek 2016-03-17 7:27 ` Maxime Ripard 0 siblings, 1 reply; 16+ messages in thread From: Michal Suchanek @ 2016-03-10 9:01 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Brown, Priit Laes, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López Hello, On 6 March 2016 at 22:42, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote: >> Besides this non-technical objection there were multiple technical >> objections. >> >> IIRC one was that the driver does not handle the case when the DMA >> channels are not available. As I understand it the channels are >> exclusively reserved for a particular peripherial on sunxi platform so >> this ShoulNotHappen(tm). So it's probably fine for the driver to fail >> probe when you have broken DT or no DMA engine support for sunxi >> platform. > > There's a quite trivial scenario that would trigger this: if the dma > driver or dmaengine is not enabled / loaded. There are other trivial scenarios under which the driver will fail like loading wrong DT, not compiling or loading the sunxi pinmux driver, and whatnot. When you misconfigure your kernel it does not work. So long as the driver just fails and does not crash and burn it's normal. Since the driver is pretty much useless without DMA as it is now (63 byte transfer limit) losing the non-DMA functionality when DMA engine is not compiled-in does not seem that terrible. Thanks Michal ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-03-10 9:01 ` Michal Suchanek @ 2016-03-17 7:27 ` Maxime Ripard 2016-03-17 10:58 ` Michal Suchanek 0 siblings, 1 reply; 16+ messages in thread From: Maxime Ripard @ 2016-03-17 7:27 UTC (permalink / raw) To: Michal Suchanek Cc: Mark Brown, Priit Laes, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López [-- Attachment #1: Type: text/plain, Size: 1937 bytes --] On Thu, Mar 10, 2016 at 10:01:04AM +0100, Michal Suchanek wrote: > Hello, > > On 6 March 2016 at 22:42, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote: > > >> Besides this non-technical objection there were multiple technical > >> objections. > >> > >> IIRC one was that the driver does not handle the case when the DMA > >> channels are not available. As I understand it the channels are > >> exclusively reserved for a particular peripherial on sunxi platform so > >> this ShoulNotHappen(tm). So it's probably fine for the driver to fail > >> probe when you have broken DT or no DMA engine support for sunxi > >> platform. > > > > There's a quite trivial scenario that would trigger this: if the dma > > driver or dmaengine is not enabled / loaded. > > There are other trivial scenarios under which the driver will fail > like loading wrong DT, not compiling or loading the sunxi pinmux > driver, and whatnot. I don't see what the pinmux has to do with SPI and DMA, and you're wrong, the pinmux driver is always compiled in if you enable the sunxi support. And you're missing the whole point. DMA accesses are optional, pinmux and proper machine support are not. > When you misconfigure your kernel it does not work. So long as the > driver just fails and does not crash and burn it's normal. Since the > driver is pretty much useless without DMA as it is now (63 byte > transfer limit) losing the non-DMA functionality when DMA engine is > not compiled-in does not seem that terrible. You're mixing two things up: the fact that we can't do more than the FIFO length in PIO and that we're missing DMA support. We have patches to address both, and there's no depedency between the two. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-03-17 7:27 ` Maxime Ripard @ 2016-03-17 10:58 ` Michal Suchanek 2016-03-17 11:43 ` Mark Brown 0 siblings, 1 reply; 16+ messages in thread From: Michal Suchanek @ 2016-03-17 10:58 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Brown, Priit Laes, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López On 17 March 2016 at 08:27, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Thu, Mar 10, 2016 at 10:01:04AM +0100, Michal Suchanek wrote: >> Hello, >> >> On 6 March 2016 at 22:42, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >> > On Fri, Feb 26, 2016 at 01:51:51PM +0100, Michal Suchanek wrote: >> >> >> Besides this non-technical objection there were multiple technical >> >> objections. >> >> >> >> IIRC one was that the driver does not handle the case when the DMA >> >> channels are not available. As I understand it the channels are >> >> exclusively reserved for a particular peripherial on sunxi platform so >> >> this ShoulNotHappen(tm). So it's probably fine for the driver to fail >> >> probe when you have broken DT or no DMA engine support for sunxi >> >> platform. >> > >> > There's a quite trivial scenario that would trigger this: if the dma >> > driver or dmaengine is not enabled / loaded. >> >> There are other trivial scenarios under which the driver will fail >> like loading wrong DT, not compiling or loading the sunxi pinmux >> driver, and whatnot. > > I don't see what the pinmux has to do with SPI and DMA, and you're > wrong, the pinmux driver is always compiled in if you enable the sunxi > support. > > And you're missing the whole point. DMA accesses are optional, pinmux > and proper machine support are not. > >> When you misconfigure your kernel it does not work. So long as the >> driver just fails and does not crash and burn it's normal. Since the >> driver is pretty much useless without DMA as it is now (63 byte >> transfer limit) losing the non-DMA functionality when DMA engine is >> not compiled-in does not seem that terrible. > > You're mixing two things up: the fact that we can't do more than the > FIFO length in PIO and that we're missing DMA support. We have patches > to address both, and there's no depedency between the two. The only thing is that although DMA is optional on pretty much any system you will have DMA available unless you broke your config. You really do want the DMA support when it is available. So there will be nobody testing the non-DMA part and it will be prone to bitrot. Thanks Michal ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-03-17 10:58 ` Michal Suchanek @ 2016-03-17 11:43 ` Mark Brown 2016-03-17 11:54 ` Michal Suchanek 0 siblings, 1 reply; 16+ messages in thread From: Mark Brown @ 2016-03-17 11:43 UTC (permalink / raw) To: Michal Suchanek Cc: Maxime Ripard, Priit Laes, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López [-- Attachment #1: Type: text/plain, Size: 728 bytes --] On Thu, Mar 17, 2016 at 11:58:05AM +0100, Michal Suchanek wrote: > On 17 March 2016 at 08:27, Maxime Ripard > > You're mixing two things up: the fact that we can't do more than the > > FIFO length in PIO and that we're missing DMA support. We have patches > > to address both, and there's no depedency between the two. > The only thing is that although DMA is optional on pretty much any > system you will have DMA available unless you broke your config. You > really do want the DMA support when it is available. So there will be > nobody testing the non-DMA part and it will be prone to bitrot. Well, it might be worth doing PIO for very short transfers even if you have DMA - it's quite common for this to perform better. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-03-17 11:43 ` Mark Brown @ 2016-03-17 11:54 ` Michal Suchanek 2016-03-17 11:58 ` Mark Brown 2016-03-18 20:23 ` Maxime Ripard 0 siblings, 2 replies; 16+ messages in thread From: Michal Suchanek @ 2016-03-17 11:54 UTC (permalink / raw) To: Mark Brown Cc: Maxime Ripard, Priit Laes, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López On 17 March 2016 at 12:43, Mark Brown <broonie@kernel.org> wrote: > On Thu, Mar 17, 2016 at 11:58:05AM +0100, Michal Suchanek wrote: >> On 17 March 2016 at 08:27, Maxime Ripard > >> > You're mixing two things up: the fact that we can't do more than the >> > FIFO length in PIO and that we're missing DMA support. We have patches >> > to address both, and there's no depedency between the two. > >> The only thing is that although DMA is optional on pretty much any >> system you will have DMA available unless you broke your config. You >> really do want the DMA support when it is available. So there will be >> nobody testing the non-DMA part and it will be prone to bitrot. > > Well, it might be worth doing PIO for very short transfers even if you > have DMA - it's quite common for this to perform better. That's what the driver does. The discussion revolves around the fact that the driver does not attempt to work (even for very short transfers) when the DMA channels are not configured and just bails out. AFAICT the channels are always available when the system is properly configured and the dmaengine driver loaded. Very few device drivers would work with 63byte transfers only and the code for manually driving the CS line in case the DMA engine fails to configure will necessarily go untested most of the time since most systems will have DMA configured properly. Thanks Michal ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-03-17 11:54 ` Michal Suchanek @ 2016-03-17 11:58 ` Mark Brown 2016-03-18 20:23 ` Maxime Ripard 1 sibling, 0 replies; 16+ messages in thread From: Mark Brown @ 2016-03-17 11:58 UTC (permalink / raw) To: Michal Suchanek Cc: Maxime Ripard, Priit Laes, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López [-- Attachment #1: Type: text/plain, Size: 1008 bytes --] On Thu, Mar 17, 2016 at 12:54:08PM +0100, Michal Suchanek wrote: > That's what the driver does. The discussion revolves around the fact > that the driver does not attempt to work (even for very short > transfers) when the DMA channels are not configured and just bails > out. AFAICT the channels are always available when the system is > properly configured and the dmaengine driver loaded. The driver should tell the core about this constraint so the core can split the transfers for it. > Very few device drivers would work with 63byte transfers only and the > code for manually driving the CS line in case the DMA engine fails to > configure will necessarily go untested most of the time since most > systems will have DMA configured properly. A lot of devices will be perfectly happy with 63 byte transfers, register accesses for example tend to be much smaller than that. The manual /CS might be an issue but for most SoCs that is easily addressed by driving the pin as a GPIO if there's an issue. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-03-17 11:54 ` Michal Suchanek 2016-03-17 11:58 ` Mark Brown @ 2016-03-18 20:23 ` Maxime Ripard 2016-03-18 20:33 ` Mark Brown 1 sibling, 1 reply; 16+ messages in thread From: Maxime Ripard @ 2016-03-18 20:23 UTC (permalink / raw) To: Michal Suchanek Cc: Mark Brown, Priit Laes, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López [-- Attachment #1: Type: text/plain, Size: 2045 bytes --] On Thu, Mar 17, 2016 at 12:54:08PM +0100, Michal Suchanek wrote: > On 17 March 2016 at 12:43, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Mar 17, 2016 at 11:58:05AM +0100, Michal Suchanek wrote: > >> On 17 March 2016 at 08:27, Maxime Ripard > > > >> > You're mixing two things up: the fact that we can't do more than the > >> > FIFO length in PIO and that we're missing DMA support. We have patches > >> > to address both, and there's no depedency between the two. > > > >> The only thing is that although DMA is optional on pretty much any > >> system you will have DMA available unless you broke your config. You > >> really do want the DMA support when it is available. So there will be > >> nobody testing the non-DMA part and it will be prone to bitrot. > > > > Well, it might be worth doing PIO for very short transfers even if you > > have DMA - it's quite common for this to perform better. > > That's what the driver does. The discussion revolves around the fact > that the driver does not attempt to work (even for very short > transfers) when the DMA channels are not configured and just bails > out. AFAICT the channels are always available when the system is > properly configured and the dmaengine driver loaded. I guess we just don't have the same defininition of "proper". Let's take an opposite view. Can you have SPI working now if the DMAEngine framework is not there? Yes. Why should it change? And even more so, why should it change silently? > Very few device drivers would work with 63byte transfers only and the > code for manually driving the CS line in case the DMA engine fails to > configure will necessarily go untested most of the time since most > systems will have DMA configured properly. That's not true. A significant portion would work. I don't like to make up numbers, but I guess only the EEPROMs and rather big screens wouldn't work. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: sun4i: add DMA support 2016-03-18 20:23 ` Maxime Ripard @ 2016-03-18 20:33 ` Mark Brown 0 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2016-03-18 20:33 UTC (permalink / raw) To: Maxime Ripard Cc: Michal Suchanek, Priit Laes, Chen-Yu Tsai, linux-spi, linux-arm-kernel, Linux Kernel Mailing List, Emilio López [-- Attachment #1: Type: text/plain, Size: 820 bytes --] On Fri, Mar 18, 2016 at 09:23:10PM +0100, Maxime Ripard wrote: > On Thu, Mar 17, 2016 at 12:54:08PM +0100, Michal Suchanek wrote: > > Very few device drivers would work with 63byte transfers only and the > > code for manually driving the CS line in case the DMA engine fails to > > configure will necessarily go untested most of the time since most > > systems will have DMA configured properly. > That's not true. A significant portion would work. I don't like to > make up numbers, but I guess only the EEPROMs and rather big screens > wouldn't work. You'll probably also see devices doing things like firmware downloads, there's a bunch of applications for larger transfers. Like you say it's definitely not going to be a universal problem though, there's many devices that just use SPI for fast register access. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] ARM: sunxi: spi: add notice about SPI FIFO limit. 2016-02-26 5:56 [PATCH RESEND 0/2] spi: sun4i: add DMA support Priit Laes 2016-02-26 5:56 ` [PATCH 1/2] " Priit Laes @ 2016-02-26 5:56 ` Priit Laes 2016-03-06 18:12 ` Maxime Ripard 2016-02-26 12:15 ` [PATCH RESEND 0/2] spi: sun4i: add DMA support Mark Brown 2 siblings, 1 reply; 16+ messages in thread From: Priit Laes @ 2016-02-26 5:56 UTC (permalink / raw) To: Mark Brown, Maxime Ripard, Chen-Yu Tsai Cc: linux-spi, linux-arm-kernel, linux-kernel, Emilio López, Michal Suchanek From: Michal Suchanek <hramrach@gmail.com> When testing SPI without DMA I noticed that filling the FIFO on the spi controller causes timeout. This should never happen with DMA support so just adding a comment. Signed-off-by: Michal Suchanek <hramrach@gmail.com> --- drivers/spi/spi-sun4i.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c index 78141a6..b750664 100644 --- a/drivers/spi/spi-sun4i.c +++ b/drivers/spi/spi-sun4i.c @@ -333,7 +333,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master, sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0); /* Fill the TX FIFO */ - sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH); + /* Filling the fifo fully causes timeout for some reason - at least on spi2 on a10s */ + /* The can_dma check is txlen >= SUN4I_FIFO_DEPTH so with DMA this should never happen anyway. */ + sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1); } /* Start the transfer */ -- 2.7.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] ARM: sunxi: spi: add notice about SPI FIFO limit. 2016-02-26 5:56 ` [PATCH 2/2] ARM: sunxi: spi: add notice about SPI FIFO limit Priit Laes @ 2016-03-06 18:12 ` Maxime Ripard 0 siblings, 0 replies; 16+ messages in thread From: Maxime Ripard @ 2016-03-06 18:12 UTC (permalink / raw) To: Priit Laes Cc: Mark Brown, Chen-Yu Tsai, linux-spi, linux-arm-kernel, linux-kernel, Emilio López, Michal Suchanek [-- Attachment #1: Type: text/plain, Size: 1303 bytes --] Hi, On Fri, Feb 26, 2016 at 07:56:57AM +0200, Priit Laes wrote: > From: Michal Suchanek <hramrach@gmail.com> > > When testing SPI without DMA I noticed that filling the FIFO on the > spi controller causes timeout. This should never happen with DMA support > so just adding a comment. > > Signed-off-by: Michal Suchanek <hramrach@gmail.com> > --- > drivers/spi/spi-sun4i.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c > index 78141a6..b750664 100644 > --- a/drivers/spi/spi-sun4i.c > +++ b/drivers/spi/spi-sun4i.c > @@ -333,7 +333,9 @@ static int sun4i_spi_transfer_one(struct spi_master *master, > sun4i_spi_write(sspi, SUN4I_DMA_CTL_REG, 0); > > /* Fill the TX FIFO */ > - sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH); > + /* Filling the fifo fully causes timeout for some reason - at least on spi2 on a10s */ > + /* The can_dma check is txlen >= SUN4I_FIFO_DEPTH so with DMA this should never happen anyway. */ > + sun4i_spi_fill_fifo(sspi, SUN4I_FIFO_DEPTH - 1); Please wrap the lines at 80 chars and use the proper multiline comment style. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH RESEND 0/2] spi: sun4i: add DMA support 2016-02-26 5:56 [PATCH RESEND 0/2] spi: sun4i: add DMA support Priit Laes 2016-02-26 5:56 ` [PATCH 1/2] " Priit Laes 2016-02-26 5:56 ` [PATCH 2/2] ARM: sunxi: spi: add notice about SPI FIFO limit Priit Laes @ 2016-02-26 12:15 ` Mark Brown 2 siblings, 0 replies; 16+ messages in thread From: Mark Brown @ 2016-02-26 12:15 UTC (permalink / raw) To: Priit Laes Cc: Maxime Ripard, Chen-Yu Tsai, linux-spi, linux-arm-kernel, linux-kernel, Emilio López, Michal Suchanek [-- Attachment #1: Type: text/plain, Size: 487 bytes --] On Fri, Feb 26, 2016 at 07:56:55AM +0200, Priit Laes wrote: > While trying to get SPI TFT screen working with Cubietruck, I noticed that > at current state driver didn't actually support SPI data bursts bigger than > n bytes (n is probably ~64 bytes). If you're going to add noise like "RESEND" to your subject lines (which makes the visible space for the actual subject smaller so makes handling incoming mail harder) please at least make sure it's true, you've not sent these before. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-03-18 20:34 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-26 5:56 [PATCH RESEND 0/2] spi: sun4i: add DMA support Priit Laes 2016-02-26 5:56 ` [PATCH 1/2] " Priit Laes 2016-02-26 12:25 ` Mark Brown 2016-02-26 12:51 ` Michal Suchanek 2016-03-06 21:42 ` Maxime Ripard 2016-03-10 9:01 ` Michal Suchanek 2016-03-17 7:27 ` Maxime Ripard 2016-03-17 10:58 ` Michal Suchanek 2016-03-17 11:43 ` Mark Brown 2016-03-17 11:54 ` Michal Suchanek 2016-03-17 11:58 ` Mark Brown 2016-03-18 20:23 ` Maxime Ripard 2016-03-18 20:33 ` Mark Brown 2016-02-26 5:56 ` [PATCH 2/2] ARM: sunxi: spi: add notice about SPI FIFO limit Priit Laes 2016-03-06 18:12 ` Maxime Ripard 2016-02-26 12:15 ` [PATCH RESEND 0/2] spi: sun4i: add DMA support Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).