| Message ID | 20190304230220.22374-1-tpiepho@impinj.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0a9c8998e75b69b3c347751a65ddd5bf7e72b2dd |
| Headers | show |
| Series | spi: imx: add module parameter to control DMA use | expand |
On Mon, Mar 04, 2019 at 11:02:36PM +0000, Trent Piepho wrote: > Add the boolean module parameter "use_dma" to control the use of DMA by > the driver. There are about two dozen other drivers with a "use_dma" > parameter of some sort. > > DMA may allow faster and more efficient transfers than using PIO, but it > also adds overhead for small transfers. > > High speed receive operations may be less likely to have issues with > FIFO overflow when using DMA than when using PIO. > > The eCSPI appears to insert a 4 bit pause after each word in DMA mode, > not done in PIO mode, which can make DMA transfers 50% slower than PIO. > > In some cases DMA may be a net win while in others PIO might be. It > depends on the application. So allow DMA to be enabled or disabled at > the driver level. The default will be to have it enabled when possible. > > Signed-off-by: Trent Piepho <tpiepho@impinj.com> Wouldn't it be more sensible to change the driver to only use DMA for big transfers? That would look much more reasonable than a global parameter that affects all transfers on all spi interfaces. The spi-mxs driver does something like that. (In mxs_spi_transfer_one look for "if (t->len < 32) {".) Best regards Uwe
On Wed, Mar 06, 2019 at 09:29:36AM +0100, Uwe Kleine-König wrote: > On Mon, Mar 04, 2019 at 11:02:36PM +0000, Trent Piepho wrote: > > Add the boolean module parameter "use_dma" to control the use of DMA by > > the driver. There are about two dozen other drivers with a "use_dma" > > parameter of some sort. > Wouldn't it be more sensible to change the driver to only use DMA for > big transfers? That would look much more reasonable than a global > parameter that affects all transfers on all spi interfaces. The spi-mxs > driver does something like that. (In mxs_spi_transfer_one look for "if > (t->len < 32) {".) Yes, this is the standard solution and it ensures that things work well for people if they just use the driver without knowing the magic incanation, and it also avoids issues for devices which do a combination of both large and small transfers.
On Wed, 2019-03-06 at 15:00 +0000, Mark Brown wrote: > On Wed, Mar 06, 2019 at 09:29:36AM +0100, Uwe Kleine-König wrote: > > On Mon, Mar 04, 2019 at 11:02:36PM +0000, Trent Piepho wrote: > > > Add the boolean module parameter "use_dma" to control the use of DMA by > > > the driver. There are about two dozen other drivers with a "use_dma" > > > parameter of some sort. > > Wouldn't it be more sensible to change the driver to only use DMA for > > big transfers? That would look much more reasonable than a global > > parameter that affects all transfers on all spi interfaces. The spi-mxs > > driver does something like that. (In mxs_spi_transfer_one look for "if > > (t->len < 32) {".) > > Yes, this is the standard solution and it ensures that things work well > for people if they just use the driver without knowing the magic > incanation, and it also avoids issues for devices which do a combination > of both large and small transfers. spi imx already does this, switching to PIO if the transfer is less than the FIFO size. Assuming future masters don't have huge fifos, the cpu can likely stuff the fifo, without waiting, faster than it could setup a dma transfer. That addresses some of the trade-offs of dma vs pio, but not all. There are still other considerations. From my commit message: High speed receive operations may be less likely to have issues with FIFO overflow when using DMA than when using PIO. The eCSPI appears to insert a 4 bit pause after each word in DMA mode, not done in PIO mode, which can make DMA transfers 50% slower than PIO.
On Wed, Mar 06, 2019 at 06:57:12PM +0000, Trent Piepho wrote: > High speed receive operations may be less likely to have issues with > FIFO overflow when using DMA than when using PIO. > The eCSPI appears to insert a 4 bit pause after each word in DMA mode, > not done in PIO mode, which can make DMA transfers 50% slower than PIO. Wow, that's... innovative. If that's the case can't the decision function for switching to DMA be tweaked to take into account slave mode operation instead?
On Thu, 2019-03-07 at 10:30 +0000, Mark Brown wrote: > On Wed, Mar 06, 2019 at 06:57:12PM +0000, Trent Piepho wrote: > > > High speed receive operations may be less likely to have issues > > with > > FIFO overflow when using DMA than when using PIO. > > The eCSPI appears to insert a 4 bit pause after each word in DMA > > mode, > > not done in PIO mode, which can make DMA transfers 50% slower than > > PIO. > > Wow, that's... innovative. If that's the case can't the decision > function for switching to DMA be tweaked to take into account slave > mode operation instead? Do you mean slave mode as in the imx being the spi slave and the external device the master? That's not being used here. Or do you mean spi mode, as in clock phase and polarity? I'm not sure if that matters or not, though it certainly could. I can give the other modes and try and see what they do. Certainly I've seen spi masters that do funny things with CS pulses between words in certain modes. This is what I see on the SPI clock in DMA mode, https://imagebin.ca/v/4WEkEnvsVSkq That gap after each byte should not be there and isn't in PIO mode.
On Fri, Mar 08, 2019 at 12:55:52AM +0000, Trent Piepho wrote: > On Thu, 2019-03-07 at 10:30 +0000, Mark Brown wrote: > > On Wed, Mar 06, 2019 at 06:57:12PM +0000, Trent Piepho wrote: > > > High speed receive operations may be less likely to have issues > > > with > > > FIFO overflow when using DMA than when using PIO. > > > The eCSPI appears to insert a 4 bit pause after each word in DMA > > > mode, > > > not done in PIO mode, which can make DMA transfers 50% slower than > > > PIO. > > Wow, that's... innovative. If that's the case can't the decision > > function for switching to DMA be tweaked to take into account slave > > mode operation instead? > Do you mean slave mode as in the imx being the spi slave and the > external device the master? That's not being used here. Sorry, not sure where I got that from. Must've just switched from another mail talking about that or something. The point still stands though, can't we handle this by adjusting the decision function? Though there's an unfortunate system load/performance tradeoff... If we can't do that then I think a runtime control with a device property in sysfs would be a good idea as well.
On Fri, 2019-03-08 at 12:16 +0000, Mark Brown wrote: > On Fri, Mar 08, 2019 at 12:55:52AM +0000, Trent Piepho wrote: > > On Thu, 2019-03-07 at 10:30 +0000, Mark Brown wrote: > > > On Wed, Mar 06, 2019 at 06:57:12PM +0000, Trent Piepho wrote: > > > > High speed receive operations may be less likely to have issues > > > > with > > > > FIFO overflow when using DMA than when using PIO. > > > > The eCSPI appears to insert a 4 bit pause after each word in DMA > > > > mode, > > > > not done in PIO mode, which can make DMA transfers 50% slower than > > > > PIO. > > > Wow, that's... innovative. If that's the case can't the decision > > > function for switching to DMA be tweaked to take into account slave > > > mode operation instead? > > Do you mean slave mode as in the imx being the spi slave and the > > external device the master? That's not being used here. > > Sorry, not sure where I got that from. Must've just switched from > another mail talking about that or something. The point still stands I have another patch out for imx spi, to fix a buffer overflow related to rx fifo flushing needed in spi slave mode. > though, can't we handle this by adjusting the decision function? Though > there's an unfortunate system load/performance tradeoff... If we can't > do that then I think a runtime control with a device property in sysfs > would be a good idea as well. The module parameter is writable, so it is a runtime control in sysfs, /sys/module/spi_imx/parameters/use_dma. It's also settable on boot via command line "spi_imx.use_dma=0". Very handy if you have a kernel driver that will use spi on init, which a custom sysfs device attr wouldn't address, since it couldn't be set. A device tree attribute could be used, but this is Linux configuration, not hardware layout, and shouldn't go into the device tree. I also found another reason to not use DMA: qemu can't emulate imx SPI when it uses DMA. It works for PIO. So being able to add spi_imx.use_dma=0 to the kernel command line when booting the kernel in qemu is very handy!
On Fri, Mar 08, 2019 at 08:22:26PM +0000, Trent Piepho wrote: > On Fri, 2019-03-08 at 12:16 +0000, Mark Brown wrote: > > though, can't we handle this by adjusting the decision function? Though > > there's an unfortunate system load/performance tradeoff... If we can't > > do that then I think a runtime control with a device property in sysfs > > would be a good idea as well. > The module parameter is writable, so it is a runtime control in sysfs, > /sys/module/spi_imx/parameters/use_dma. It's also settable on boot via > command line "spi_imx.use_dma=0". Very handy if you have a kernel > driver that will use spi on init, which a custom sysfs device attr > wouldn't address, since it couldn't be set. A device tree attribute > could be used, but this is Linux configuration, not hardware layout, > and shouldn't go into the device tree. Oh, cool - I hadn't realized that those sysfs files were writeable. Nice! > I also found another reason to not use DMA: qemu can't emulate imx SPI > when it uses DMA. It works for PIO. So being able to add > spi_imx.use_dma=0 to the kernel command line when booting the kernel in > qemu is very handy! Right, and qemu won't tell the guest it's qemu either :(
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index a81ae29aa68a..09c9a1edb2c6 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -28,6 +28,10 @@ #define DRIVER_NAME "spi_imx" +static bool use_dma = true; +module_param(use_dma, bool, 0644); +MODULE_PARM_DESC(use_dma, "Enable usage of DMA when available (default)"); + #define MXC_CSPIRXDATA 0x00 #define MXC_CSPITXDATA 0x04 #define MXC_CSPICTRL 0x08 @@ -219,6 +223,9 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi, { struct spi_imx_data *spi_imx = spi_master_get_devdata(master); + if (!use_dma) + return false; + if (!master->dma_rx) return false;
Add the boolean module parameter "use_dma" to control the use of DMA by the driver. There are about two dozen other drivers with a "use_dma" parameter of some sort. DMA may allow faster and more efficient transfers than using PIO, but it also adds overhead for small transfers. High speed receive operations may be less likely to have issues with FIFO overflow when using DMA than when using PIO. The eCSPI appears to insert a 4 bit pause after each word in DMA mode, not done in PIO mode, which can make DMA transfers 50% slower than PIO. In some cases DMA may be a net win while in others PIO might be. It depends on the application. So allow DMA to be enabled or disabled at the driver level. The default will be to have it enabled when possible. Signed-off-by: Trent Piepho <tpiepho@impinj.com> --- drivers/spi/spi-imx.c | 7 +++++++ 1 file changed, 7 insertions(+)