| Message ID | 1456466217-6793-2-git-send-email-plaes@plaes.org |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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
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
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.
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
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.
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
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.
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; }