Since commit 530b5affc6 ("spi: fsl-dspi: fix use-after-free in remove
path") this driver causes a kernel oops:
[ 1.891065] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000080
[..]
[ 2.056973] Call trace:
[ 2.059425] dspi_setup+0xc8/0x2e0
[ 2.062837] spi_setup+0xcc/0x248
[ 2.066160] spi_add_device+0xb4/0x198
[ 2.069918] of_register_spi_device+0x250/0x370
[ 2.074462] spi_register_controller+0x4f4/0x770
[ 2.079094] dspi_probe+0x5bc/0x7b0
[ 2.082594] platform_drv_probe+0x5c/0xb0
[ 2.086615] really_probe+0xec/0x3c0
[ 2.090200] driver_probe_device+0x60/0xc0
[ 2.094308] device_driver_attach+0x7c/0x88
[ 2.098503] __driver_attach+0x60/0xe8
[ 2.102263] bus_for_each_dev+0x7c/0xd0
[ 2.106109] driver_attach+0x2c/0x38
[ 2.109692] bus_add_driver+0x194/0x1f8
[ 2.113538] driver_register+0x6c/0x128
[ 2.117385] __platform_driver_register+0x50/0x60
[ 2.122105] fsl_dspi_driver_init+0x24/0x30
[ 2.126302] do_one_initcall+0x54/0x2d0
[ 2.130149] kernel_init_freeable+0x1ec/0x258
[ 2.134520] kernel_init+0x1c/0x120
[ 2.138018] ret_from_fork+0x10/0x34
[ 2.141606] Code: 97e0b11d aa0003f3 b4000680 f94006e0 (f9404000)
[ 2.147723] ---[ end trace 26cf63e6cbba33a8 ]---
This is because since this commit, the allocation of the drivers private
data is done explicitly and in this case spi_alloc_master() won't set the
correct pointer.
Also move the platform_set_drvdata() to have both next to each other.
Fixes: 530b5affc6 ("spi: fsl-dspi: fix use-after-free in remove path")
Signed-off-by: Michael Walle <michael@walle.cc>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Link: https://lore.kernel.org/r/20200928085500.28254-1-michael@walle.cc
Signed-off-by: Mark Brown <broonie@kernel.org>
spi_unregister_controller() not only unregisters the controller, but
also frees the controller. This will free the driver data with it, so
we must not access it later dspi_remove().
Solve this by allocating the driver data separately from the SPI
controller.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Link: https://lore.kernel.org/r/20200923131026.20707-1-s.hauer@pengutronix.de
Signed-off-by: Mark Brown <broonie@kernel.org>
The arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi device tree lacks DMA
channels for DSPI, so naturally, the driver fails to probe:
[ 2.945302] fsl-dspi 2100000.spi: rx dma channel not available
[ 2.951134] fsl-dspi 2100000.spi: can't get dma channels
In retrospect, this should have been obvious, because LS2080A, LS2085A
LS2088A and LX2160A don't appear to have an eDMA module at all. Looking
again at their datasheets, the CTARE register (which is specific to XSPI
functionality) seems to be documented, so switch them to XSPI mode
instead.
Fixes: 0feaf8f5af ("spi: spi-fsl-dspi: Convert the instantiations that support it to DMA")
Reported-by: Qiang Zhao <qiang.zhao@nxp.com>
Tested-by: Qiang Zhao <qiang.zhao@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200910121532.1138596-1-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
The arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi device tree lacks DMA
channels for DSPI, so naturally, the driver fails to probe:
[ 2.945302] fsl-dspi 2100000.spi: rx dma channel not available
[ 2.951134] fsl-dspi 2100000.spi: can't get dma channels
In retrospect, this should have been obvious, because LS2080A, LS2085A
LS2088A and LX2160A don't appear to have an eDMA module at all. Looking
again at their datasheets, the CTARE register (which is specific to XSPI
functionality) seems to be documented, so switch them to XSPI mode
instead.
Fixes: 0feaf8f5af ("spi: spi-fsl-dspi: Convert the instantiations that support it to DMA")
Reported-by: Qiang Zhao <qiang.zhao@nxp.com>
Tested-by: Qiang Zhao <qiang.zhao@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200910121532.1138596-1-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
After the only user of the limited EOQ mode has now been converted to
DMA as of commit b09058bbf5 ("spi: spi-fsl-dspi: set ColdFire to DMA
mode"), we can finally delete this code.
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Link: https://lore.kernel.org/r/20200823212657.2400075-1-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Set DMA transfer mode for ColdFire.
After recent fixes to fsl edma engine, this mode can be used
also for ColdFire, and from some raw mtd r/w tests it definitely
improves the transfer rate, so keeping it selected.
Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
Link: https://lore.kernel.org/r/20200816094635.1830006-1-angelo.dureghello@timesys.com
Signed-off-by: Mark Brown <broonie@kernel.org>
The interrupt handler calls completion and is IRQ requested before the
completion is initialized. Logically it should be the other way.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Link: https://lore.kernel.org/r/20200622110543.5035-4-krzk@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
If shared interrupt comes late, during probe error path or device remove
(could be triggered with CONFIG_DEBUG_SHIRQ), the interrupt handler
dspi_interrupt() will access registers with the clock being disabled.
This leads to external abort on non-linefetch on Toradex Colibri VF50
module (with Vybrid VF5xx):
$ echo 4002d000.spi > /sys/devices/platform/soc/40000000.bus/4002d000.spi/driver/unbind
Unhandled fault: external abort on non-linefetch (0x1008) at 0x8887f02c
Internal error: : 1008 [#1] ARM
Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
Backtrace:
(regmap_mmio_read32le)
(regmap_mmio_read)
(_regmap_bus_reg_read)
(_regmap_read)
(regmap_read)
(dspi_interrupt)
(free_irq)
(devm_irq_release)
(release_nodes)
(devres_release_all)
(device_release_driver_internal)
The resource-managed framework should not be used for shared interrupt
handling, because the interrupt handler might be called after releasing
other resources and disabling clocks.
Similar bug could happen during suspend - the shared interrupt handler
could be invoked after suspending the device. Each device sharing this
interrupt line should disable the IRQ during suspend so handler will be
invoked only in following cases:
1. None suspended,
2. All devices resumed.
Fixes: 349ad66c0a ("spi:Add Freescale DSPI driver for Vybrid VF610 platform")
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20200622110543.5035-3-krzk@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
During shutdown, the driver should unregister the SPI controller
and stop the hardware. Otherwise the dspi_transfer_one_message() could
wait on completion infinitely.
Additionally, calling spi_unregister_controller() first in device
shutdown reverse-matches the probe function, where SPI controller is
registered at the end.
Fixes: dc23482599 ("spi: spi-fsl-dspi: Adding shutdown hook")
Reported-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20200622110543.5035-2-krzk@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
During device removal, the driver should unregister the SPI controller
and stop the hardware. Otherwise the dspi_transfer_one_message() could
wait on completion infinitely.
Additionally, calling spi_unregister_controller() first in device
removal reverse-matches the probe function, where SPI controller is
registered at the end.
Fixes: 05209f4570 ("spi: fsl-dspi: add missing clk_disable_unprepare() in dspi_remove()")
Reported-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20200622110543.5035-1-krzk@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
Driver allocates DMA memory with dma_alloc_coherent() but frees it with
dma_unmap_single().
This causes DMA warning during system shutdown (with DMA debugging) on
Toradex Colibri VF50 module:
WARNING: CPU: 0 PID: 1 at ../kernel/dma/debug.c:1036 check_unmap+0x3fc/0xb04
DMA-API: fsl-edma 40098000.dma-controller: device driver frees DMA memory with wrong function
[device address=0x0000000087040000] [size=8 bytes] [mapped as coherent] [unmapped as single]
Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
(unwind_backtrace) from [<8010bb34>] (show_stack+0x10/0x14)
(show_stack) from [<8011ced8>] (__warn+0xf0/0x108)
(__warn) from [<8011cf64>] (warn_slowpath_fmt+0x74/0xb8)
(warn_slowpath_fmt) from [<8017d170>] (check_unmap+0x3fc/0xb04)
(check_unmap) from [<8017d900>] (debug_dma_unmap_page+0x88/0x90)
(debug_dma_unmap_page) from [<80601d68>] (dspi_release_dma+0x88/0x110)
(dspi_release_dma) from [<80601e4c>] (dspi_shutdown+0x5c/0x80)
(dspi_shutdown) from [<805845f8>] (device_shutdown+0x17c/0x220)
(device_shutdown) from [<80143ef8>] (kernel_restart+0xc/0x50)
(kernel_restart) from [<801441cc>] (__do_sys_reboot+0x18c/0x210)
(__do_sys_reboot) from [<80100060>] (ret_fast_syscall+0x0/0x28)
DMA-API: Mapped at:
dma_alloc_attrs+0xa4/0x130
dspi_probe+0x568/0x7b4
platform_drv_probe+0x6c/0xa4
really_probe+0x208/0x348
driver_probe_device+0x5c/0xb4
Fixes: 90ba37033c ("spi: spi-fsl-dspi: Add DMA support for Vybrid")
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/1591803717-11218-1-git-send-email-krzk@kernel.org
Signed-off-by: Mark Brown <broonie@kernel.org>
ColdFire is a big-endian cpu with a big-endian dspi hw module,
so, it uses native access, but memcpy breaks the endianness.
So, if i understand properly, by native copy we would mean
be(cpu)->be(dspi) or le(cpu)->le(dspi) accesses, so my fix
shouldn't break anything, but i couldn't test it on LS family,
so every test is really appreciated.
Fixes: 53fadb4d90 ("spi: spi-fsl-dspi: Simplify bytes_per_word gymnastics")
Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200529195756.184677-1-angelo.dureghello@timesys.com
Signed-off-by: Mark Brown <broonie@kernel.org>
We need to ensure dspi controller could be stopped in order for kexec
to start the next kernel.
So add the shutdown operation support.
Signed-off-by: Peng Ma <peng.ma@nxp.com>
Link: https://lore.kernel.org/r/20200424061216.27445-1-peng.ma@nxp.com
Signed-off-by: Mark Brown <broonie@kernel.org>
This is similar to the DSPI instantiation on LS1028A, except that:
- The A-011218 erratum has been fixed, so DMA works
- The endianness is different, which has implications on XSPI mode
Some benchmarking with the following command:
spidev_test --device /dev/spidev2.0 --bpw 8 --size 256 --cpha --iter 10000000 --speed 20000000
shows that in DMA mode, it can achieve around 2400 kbps, and in XSPI
mode, the same command goes up to 4700 kbps. This is somewhat to be
expected, since the DMA buffer size is extremely small at 8 bytes, the
winner becomes whomever can prepare the buffers for transmission
quicker, and DMA mode has higher overhead there. So XSPI FIFO mode has
been chosen as the operating mode for this chip.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-11-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
The operating mode (DMA, XSPI, EOQ) is not going to change across the
lifetime of the device. So it makes no sense to keep writing to SPI_RSER
on each message. Move this configuration to dspi_init instead.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-10-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Interrupts are not necessary for DMA functionality, since the completion
event is provided by the DMA driver.
But if the driver fails to request the IRQ defined in the device tree,
it will call dspi_poll which would make the driver hang waiting for data
to become available in the RX FIFO.
Fixes: c55be30591 ("spi: spi-fsl-dspi: Use poll mode in case the platform IRQ is missing")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-9-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
The driver does not create the dspi->dma structure unless operating in
DSPI_DMA_MODE, so it makes sense to check for that.
Fixes: f4b323905d ("spi: Introduce dspi_slave_abort() function for NXP's dspi SPI driver")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-8-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Currently the driver puts the process in interruptible sleep waiting for
the interrupt train to finish transfer to/from the tx_buf and rx_buf.
But exiting the process with ctrl-c may make the kernel panic: the
wait_event_interruptible call will return -ERESTARTSYS, which a proper
driver implementation is perhaps supposed to handle, but nonetheless
this one doesn't, and aborts the transfer altogether.
Actually when the task is interrupted, there is still a high chance that
the dspi_interrupt is still triggering. And if dspi_transfer_one_message
returns execution all the way to the spi_device driver, that can free
the spi_message and spi_transfer structures, leaving the interrupts to
access a freed tx_buf and rx_buf.
hexdump -C /dev/mtd0
00000000 00 75 68 75 0a ff ff ff ff ff ff ff ff ff ff ff
|.uhu............|
00000010 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
|................|
*
^C[ 38.495955] fsl-dspi 2120000.spi: Waiting for transfer to complete failed!
[ 38.503097] spi_master spi2: failed to transfer one message from queue
[ 38.509729] Unable to handle kernel paging request at virtual address ffff800095ab3377
[ 38.517676] Mem abort info:
[ 38.520474] ESR = 0x96000045
[ 38.523533] EC = 0x25: DABT (current EL), IL = 32 bits
[ 38.528861] SET = 0, FnV = 0
[ 38.531921] EA = 0, S1PTW = 0
[ 38.535067] Data abort info:
[ 38.537952] ISV = 0, ISS = 0x00000045
[ 38.541797] CM = 0, WnR = 1
[ 38.544771] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000082621000
[ 38.551494] [ffff800095ab3377] pgd=00000020fffff003, p4d=00000020fffff003, pud=0000000000000000
[ 38.560229] Internal error: Oops: 96000045 [#1] PREEMPT SMP
[ 38.565819] Modules linked in:
[ 38.568882] CPU: 0 PID: 2729 Comm: hexdump Not tainted 5.6.0-rc4-next-20200306-00052-gd8730cdc8a0b-dirty #193
[ 38.578834] Hardware name: Kontron SMARC-sAL28 (Single PHY) on SMARC Eval 2.0 carrier (DT)
[ 38.587129] pstate: 20000085 (nzCv daIf -PAN -UAO)
[ 38.591941] pc : ktime_get_real_ts64+0x3c/0x110
[ 38.596487] lr : spi_take_timestamp_pre+0x40/0x90
[ 38.601203] sp : ffff800010003d90
[ 38.604525] x29: ffff800010003d90 x28: ffff80001200e000
[ 38.609854] x27: ffff800011da9000 x26: ffff002079c40400
[ 38.615184] x25: ffff8000117fe018 x24: ffff800011daa1a0
[ 38.620513] x23: ffff800015ab3860 x22: ffff800095ab3377
[ 38.625841] x21: 000000000000146e x20: ffff8000120c3000
[ 38.631170] x19: ffff0020795f6e80 x18: ffff800011da9948
[ 38.636498] x17: 0000000000000000 x16: 0000000000000000
[ 38.641826] x15: ffff800095ab3377 x14: 0720072007200720
[ 38.647155] x13: 0720072007200765 x12: 0775076507750771
[ 38.652483] x11: 0720076d076f0772 x10: 0000000000000040
[ 38.657812] x9 : ffff8000108e2100 x8 : ffff800011dcabe8
[ 38.663139] x7 : 0000000000000000 x6 : ffff800015ab3a60
[ 38.668468] x5 : 0000000007200720 x4 : ffff800095ab3377
[ 38.673796] x3 : 0000000000000000 x2 : 0000000000000ab0
[ 38.679125] x1 : ffff800011daa000 x0 : 0000000000000026
[ 38.684454] Call trace:
[ 38.686905] ktime_get_real_ts64+0x3c/0x110
[ 38.691100] spi_take_timestamp_pre+0x40/0x90
[ 38.695470] dspi_fifo_write+0x58/0x2c0
[ 38.699315] dspi_interrupt+0xbc/0xd0
[ 38.702987] __handle_irq_event_percpu+0x78/0x2c0
[ 38.707706] handle_irq_event_percpu+0x3c/0x90
[ 38.712161] handle_irq_event+0x4c/0xd0
[ 38.716008] handle_fasteoi_irq+0xbc/0x170
[ 38.720115] generic_handle_irq+0x2c/0x40
[ 38.724135] __handle_domain_irq+0x68/0xc0
[ 38.728243] gic_handle_irq+0xc8/0x160
[ 38.732000] el1_irq+0xb8/0x180
[ 38.735149] spi_nor_spimem_read_data+0xe0/0x140
[ 38.739779] spi_nor_read+0xc4/0x120
[ 38.743364] mtd_read_oob+0xa8/0xc0
[ 38.746860] mtd_read+0x4c/0x80
[ 38.750007] mtdchar_read+0x108/0x2a0
[ 38.753679] __vfs_read+0x20/0x50
[ 38.757002] vfs_read+0xa4/0x190
[ 38.760237] ksys_read+0x6c/0xf0
[ 38.763471] __arm64_sys_read+0x20/0x30
[ 38.767319] el0_svc_common.constprop.3+0x90/0x160
[ 38.772125] do_el0_svc+0x28/0x90
[ 38.775449] el0_sync_handler+0x118/0x190
[ 38.779468] el0_sync+0x140/0x180
[ 38.782793] Code: 91000294 1400000f d50339bf f9405e80 (f90002c0)
[ 38.788910] ---[ end trace 55da560db4d6bef7 ]---
[ 38.793540] Kernel panic - not syncing: Fatal exception in interrupt
[ 38.799914] SMP: stopping secondary CPUs
[ 38.803849] Kernel Offset: disabled
[ 38.807344] CPU features: 0x10002,20006008
[ 38.811451] Memory Limit: none
[ 38.814513] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---
So it is clear that the "interruptible" part isn't handled correctly.
When the process receives a signal, one could either attempt a clean
abort (which appears to be difficult with this hardware) or just keep
restarting the sleep until the wait queue really completes. But checking
in a loop for -ERESTARTSYS is a bit too complicated for this driver, so
just make the sleep uninterruptible, to avoid all that nonsense.
The wait queue was actually restructured as a completion, after polling
other drivers for the most "popular" approach.
Fixes: 349ad66c0a ("spi:Add Freescale DSPI driver for Vybrid VF610 platform")
Reported-by: Michael Walle <michael@walle.cc>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-7-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
dspi->words_in_flight is a variable populated in the *_write functions
and used in the dspi_fifo_read function. It is also used in
dspi_fifo_write, immediately after transmission, to update the
message->actual_length variable used by higher layers such as spi-mem
for integrity checking.
But it may happen that the IRQ which calls dspi_fifo_read to be
triggered before the updating of message->actual_length takes place. In
that case, dspi_fifo_read will decrement dspi->words_in_flight to -1,
and that will cause an invalid modification of message->actual_length.
For that, we make the simplest fix possible: to not decrement the actual
shared variable in dspi->words_in_flight from dspi_fifo_read, but
actually a copy of it which is on stack.
But even if dspi_fifo_read from the next IRQ does not interfere with the
dspi_fifo_write of the current chunk, the *next* dspi_fifo_write still
can. So we must assume that everything after the last write to the TX
FIFO can be preempted by the "TX complete" IRQ, and the dspi_fifo_write
function must be safe against that. This means refactoring the 2
flavours of FIFO writes (for EOQ and XSPI) such that the calculation of
the number of words to be written is common and happens a priori. This
way, the code for updating the message->actual_length variable works
with a copy and not with the volatile dspi->words_in_flight.
After some interior debate, the dspi->progress variable used for
software timestamping was *not* backed up against preemption in a copy
on stack. Because if preemption does occur between
spi_take_timestamp_pre and spi_take_timestamp_post, there's really no
point in trying to save anything. The first-in-time
spi_take_timestamp_post call with a dspi->progress higher than the
requested xfer->ptp_sts_word_post will trigger xfer->timestamped = true
anyway and will close the deal.
To understand the above a bit better, consider a transfer with
xfer->ptp_sts_word_pre = xfer->ptp_sts_word_post = 3, and
xfer->bits_per_words = 8 (so byte 3 needs to be timestamped). The DSPI
controller timestamps in chunks of 4 bytes at a time, and preemption
occurs in the middle of timestamping the first chunk:
spi_take_timestamp_pre(0)
.
. (preemption)
.
. spi_take_timestamp_pre(4)
.
. spi_take_timestamp_post(7)
.
spi_take_timestamp_post(3)
So the reason I'm not bothering to back up dspi->progress for that
spi_take_timestamp_post(3) is that spi_take_timestamp_post(7) is going
to (a) be more honest, (b) provide better accuracy and (c) already
render the spi_take_timestamp_post(3) into a noop by setting
xfer->timestamped = true anyway.
Fixes: d59c90a240 ("spi: spi-fsl-dspi: Convert TCFQ users to XSPI FIFO mode")
Reported-by: Michael Walle <michael@walle.cc>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-6-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
If dspi->words_in_flight is populated with the hardware FIFO size,
then in dspi_fifo_read it will attempt to read more data at the end of a
buffer that is not a multiple of 16 bytes in length. It will probably
time out attempting to do so.
So limit the num_fifo_entries variable to the actual number of FIFO
entries that is going to be used.
Fixes: d59c90a240 ("spi: spi-fsl-dspi: Convert TCFQ users to XSPI FIFO mode")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-5-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
In DMA mode, dspi_setup_accel does not get called, which results in the
dspi->oper_word_size variable (which is used by dspi_dma_xfer) to not be
initialized properly.
Because oper_word_size is zero, a few calculations end up being
incorrect, and the DMA transfer eventually times out instead of sending
anything on the wire.
Set up native transfers (or 8-on-16 acceleration) using dspi_setup_accel
for DMA mode too.
Also take the opportunity and simplify the DMA buffer handling a little
bit.
Fixes: 6c1c26ecd9 ("spi: spi-fsl-dspi: Accelerate transfers using larger word size if possible")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-4-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
In XSPI mode, the 32-bit PUSHR register can be written to separately:
the higher 16 bits are for commands and the lower 16 bits are for data.
This has nicely been hacked around, by defining a second regmap with a
width of 16 bits, and effectively splitting a 32-bit register into 2
16-bit ones, from the perspective of this regmap_pushr.
The problem is the assumption about the controller's endianness. If the
controller is little endian (such as anything post-LS1046A), then the
first 2 bytes, in the order imposed by memory layout, will actually hold
the TXDATA, and the last 2 bytes will hold the CMD.
So take the controller's endianness into account when performing split
writes to PUSHR. The obvious and simple solution would have been to call
regmap_get_val_endian(), but that is an internal regmap function and we
don't want to change regmap just for this. Therefore, we just re-read
the "big-endian" device tree property.
Fixes: 58ba07ec79 ("spi: spi-fsl-dspi: Add support for XSPI mode registers")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-3-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
The SPI_MCR_PCSIS macro assumes that the controller has a number of chip
select signals equal to 6. That is not always the case, but actually is
described through the driver-specific "spi-num-chipselects" device tree
binding. LS1028A for example only has 4 chip selects.
Don't write to the upper bits of the PCSIS field, which are reserved in
the reference manual.
Fixes: 349ad66c0a ("spi:Add Freescale DSPI driver for Vybrid VF610 platform")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-2-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Use the correct device to request the DMA mapping. Otherwise the IOMMU
doesn't get the mapping and it will generate a page fault.
The error messages look like:
[ 3.008452] arm-smmu 5000000.iommu: Unhandled context fault: fsr=0x402, iova=0xf9800000, fsynr=0x3f0022, cbfrsynra=0x828, cb=8
[ 3.020123] arm-smmu 5000000.iommu: Unhandled context fault: fsr=0x402, iova=0xf9800000, fsynr=0x3f0022, cbfrsynra=0x828, cb=8
This was tested on a custom board with a LS1028A SoC.
Signed-off-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200310073313.21277-1-michael@walle.cc
Signed-off-by: Mark Brown <broonie@kernel.org>
Vladimir Oltean <vladimir.oltean@nxp.com>:
From: Vladimir Oltean <vladimir.oltean@nxp.com>
This series aims to remove the most inefficient transfer method from the
NXP DSPI driver.
TCFQ (Transfer Complete Flag) mode works by transferring one word,
waiting for its TX confirmation interrupt (or polling on the equivalent
status bit), sending the next word, etc, until the buffer is complete.
The issue with this mode is that it's fundamentally incompatible with
any sort of batching such as writing to a FIFO. But actually, due to
previous patchset ("Compatible string consolidation for NXP DSPI driver"):
https://patchwork.kernel.org/cover/11414593/
all existing users of TCFQ mode today already support a more advanced
feature set, in the form of XSPI (extended SPI). XSPI brings 2 extra
features:
- Word sizes up to 32 bits. This is sub-utilized today, and acceleration
of smaller-than-32 bpw values is provided.
- "Command cycling", basically the ability to write multiple words in a
row and receiving an interrupt only after the completion of the last
one. This is what enables us to make use of the full FIFO depth of
this controller.
Series was tested on the NXP LS1021A-TSN and LS1043A-RDB boards, both
functionally as well as from a performance standpoint.
The command used to benchmark the increased throughput was:
spidev_test --device /dev/spidev1.0 --bpw 8 --size 256 --cpha --iter 10000000 --speed 20000000
where spidev1.0 is a dummy spidev node, using a chip select that no
peripheral responds to.
On LS1021A, which has a 4-entry-deep FIFO and a less powerful CPU, the
performance increase brought by this patchset is from 2700 kbps to 5800
kbps.
On LS1043A, which has a 16-entry-deep FIFO and a more powerful CPU, the
performance increases from 4100 kbps to 13700 kbps.
On average, SPI software timestamping is not adversely affected by the
extra batching, due to the extra patches.
There is one extra patch which clarifies why the TCFQ users were not
converted to the "other" mode in this driver that makes use of the FIFO,
which would be EOQ mode.
My request to the many people on CC (known users and/or contributors) is
to give this series a test to ensure there are no regressions, and for
the Coldfire maintainers to clarify whether the EOQ limitation is
acceptable for them in the long run.
Vladimir Oltean (12):
spi: spi-fsl-dspi: Simplify bytes_per_word gymnastics
spi: spi-fsl-dspi: Remove unused chip->void_write_data
spi: spi-fsl-dspi: Don't mask off undefined bits
spi: spi-fsl-dspi: Add comments around dspi_pop_tx and dspi_push_rx
functions
spi: spi-fsl-dspi: Rename fifo_{read,write} and {tx,cmd}_fifo_write
spi: spi-fsl-dspi: Implement .max_message_size method for EOQ mode
spi: Do spi_take_timestamp_pre for as many times as necessary
spi: spi-fsl-dspi: Convert TCFQ users to XSPI FIFO mode
spi: spi-fsl-dspi: Accelerate transfers using larger word size if
possible
spi: spi-fsl-dspi: Optimize dspi_setup_accel for lowest interrupt
count
spi: spi-fsl-dspi: Use EOQ for last word in buffer even for XSPI mode
spi: spi-fsl-dspi: Take software timestamp in dspi_fifo_write
drivers/spi/spi-fsl-dspi.c | 421 ++++++++++++++++++++++++-------------
drivers/spi/spi.c | 19 +-
include/linux/spi/spi.h | 3 +-
3 files changed, 288 insertions(+), 155 deletions(-)
--
2.17.1
The SPI bus number is completely optional to Linux, so make the
corresponding device tree property optional as well.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Link: https://lore.kernel.org/r/20200305115546.31814-1-s.hauer@pengutronix.de
Signed-off-by: Mark Brown <broonie@kernel.org>
Although the SPI system timestamps are supposed to reflect the moment
that the peripheral has received a word rather than the moment when the
CPU has enqueued that word to the FIFO, in practice it is easier to just
record the latter time than the former (with a smaller error).
With the recent migration of TCFQ users from poll back to interrupt mode
(this time for XSPI FIFO), it's wiser to keep the interrupt latency
outside of the measurement of the PTP system timestamp itself. If there
proves to be any constant offset that requires static compensation, that
can always be added later. So far that does not appear to be the case at
least on the LS1021A-TSN board, where testing shows that the phc2sys
offset is able to remain within +/- 200 ns even after 68 hours of
testing.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-13-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
The EOQ mode has a hardware limitation in that it stops the transmission
(including the deassertion of the chip select signal) once the host CPU
requests end-of-queue for a particular word in the TX FIFO.
And XSPI mode has a limitation in that we need a separate CMD FIFO entry
for the last byte in the buffer, where the chip select signal needs to
be deasserted. It's not a functional limitation, but it's rather clunky
and the fact that we need to halt the pipeline and write a single entry
to the TX FIFO whenever a buffer ends brings the throughput down when
transmitting small buffers.
So the idea here is to use EOQ's limitation in our favor when using XSPI
mode. Stop special-casing that final word in the buffer, and just kill
the chip select signal by issuing an EOQ for that last word. Now it can
be mixed in with all the other words in the current TX FIFO train.
A small trick here is that we still keep using the XSPI-specific
signaling via the CMDTCFQ interrupt in RSER, and not enabling the EOQ
interrupt, in order to avoid hardware weirdness (potential races with
separate interrupts being raised for CMDTCFQ and EOQ for what is in fact
the end of the same transmission). That is just theoretical, but it's
good to be cautious, and the EOQ interrupt isn't needed.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-12-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Currently, a SPI transfer that is not multiple of the highest supported
word width (e.g. 4 bytes) will be transmitted as follows (assume a
30-byte buffer transmitted through a 32-bit wide FIFO that is 32 bytes
deep):
- First 28 bytes are sent as 7 words of 32 bits each
- Last 2 bytes are sent as 1 word of 16 bits size
But if the dspi_setup_accel function had decided to use a lower
oper_bits_per_word value (16 instead of 32), there would have been
enough space in the TX FIFO to fit the entire buffer in one go (15 words
of 16 bits each).
What we're actually trying to avoid is mixing word sizes within the same
run with the TX FIFO, since there is an erratum surrounding this, and
invalid data might get transmitted.
So this patch adds special cases for when the remaining length of the
buffer can be sent in one go as 8-bit or 16-bit words, otherwise it
falls back to the standard logic of sending as many bytes as possible at
the highest oper_bits_per_word value possible.
The benefit is that there will be one less CMDFQ/EOQ interrupt to
service when the entire buffer is transmitted during a single go, and
that will improve the overall latency of the transfer.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-11-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
This patch adds logic in the driver to transmit SPI buffers that use
bits_per_word=8 with a higher bits_per_word count (multiple of 8).
Currently the following (most common) modes are implemented:
- 8 bits_per_word on 32-bit capable controllers
- 8 bits_per_word on 16-bit capable controllers
- 16 bits_per_word on 32-bit capable controllers
Transfers which are not accelerated are transferred with a hardware
bits_per_word value equal to the one of the SPI transfer.
The difference from just extending bits_per_word=32 at the spi_device
driver level is that endianness is different - the SPI core wants to
treat bits_per_word=32 buffers as arrays of u32 (i.e. words in host CPU
endianness). So to preserve endianness when clumping 8x4 bits into
32-bit words, one must perform conversion between CPU and standard (big)
endianness.
All appearances (both on the wire as well as in the buffers presented to
the peripheral driver) are preserved, just that accesses to the PUSHR
and POPR registers are now more efficient, since the same number of
reads/writes can now carry more data (2x more data on TX, 4x more data
on RX).
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-10-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
The Transfer Complete Flag (TCF) interrupt gets raised after each write
to the TX FIFO (PUSHR) which means that it is not possible to devise a
transfer procedure that makes full utilization of the FIFO depth (4
entries on most controllers, 16 entries on some).
On the other hand, XSPI mode has a feature called "command cycling",
which allows a single TX command to be run for a pre-specified number of
TX words. When the command cycle ends, the Command Transfer Complete
Flag bit asserts and raises an interrupt. The advantage in this mode is
that the TX FIFO can be better utilized (more words can be batched at
once).
Other changes brought by this patch:
- The dspi->rx_end variable has been removed, since now the
dspi_fifo_write function sets up dspi->words_in_flight, so
dspi_fifo_read knows how much to read without overrunning the RX
buffer.
- Stop using poll mode unconditionally for TCFQ mode, since XSPI mode
is a little less efficient than that, and so, poll mode doesn't bring
as many improvements for XSPI.
- Stop relying on the hardware transfer counter (SPI_TCR_GET_TCNT) and
instead increment the message->actual_length based on the newly
introduced dspi->words_in_flight variable.
- The CTARE register is now written in the hotpath instead of just at
transfer init time, since it contains the DTCP field (transfer
preload - the counter indicating how many txdata words will follow),
which is a dynamic value.
Due to the fact that the Chip Select toggling setting is part of the
command written to the TX FIFO, the ending word of each buffer needs to
be sent via its own TX command, so that we have a chance to emit a
1-word command with deasserted PCS.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-9-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
When it gets set, End Of Queue Flag halts the DSPI controller and forces
the chip select signal to deassert.
This operating mode is not ideal, but it is used for the DSPI
instantiations where there is no other notification from the controller
that the data in the FIFO has finished transmission. So in practice, it
means that transmitting buffers larger than the FIFO size will yield
unpredictable results.
The only controller that operates in EOQ mode is MCF5441X (Coldfire). I
would say that the way EOQ is used (and documented in the reference
manual, too) on this chip is incorrect, and I would personally migrate
it to TCFQ, but that's notably worse in terms of performance (it can
only use 1 entry of the 16-deep FIFO) and if this limitation didn't
bother any Coldfire DSPI user so far, it's likely that we just need to
throw an error for larger buffers to make sure that callers are aware
their transfers are getting truncated/split.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-7-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
These function names are very generic and it is easy to get confused.
Rename them after the hardware register that they are accessing.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-6-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Their names are confusing, since dspi_pop_tx prepares a word to be
written to the PUSHR register, and dspi_push_rx gets a word from the
POPR register.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-5-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
This is a useless operation, and if the driver needs to do that, there's
something deeply wrong going on.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-4-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
This variable has been present since the initial submission of the
driver, and held, for some reason, the value of zero, to be sent on the
wire in the case there wasn't any TX buffer for the current transfer.
Since quite a while now, however, it isn't doing anything at all.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-3-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Reduce the if-then-else-if-then-else sequence to:
- a simple division in the case of bytes_per_word calculation
- a memcpy command with a variable size. The semantics of larger-than-8
xfer->bits_per_word is that those words are to be interpreted and
transmitted in CPU native endianness.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Link: https://lore.kernel.org/r/20200304220044.11193-2-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
The A-011218 eDMA/DSPI erratum affects most of the older Layerscape SoCs
with DSPI, and its workaround is a bit intrusive.
After this patch, there are no users of TCFQ mode that don't also
support XSPI (previously there was LS2085A).
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Message-Id: <20200302001958.11105-7-olteanv@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
There's no reason to keep this .ptp_sts_supported property explicitly in
devtype_data, since it can be deduced from the operating mode alone.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Message-Id: <20200302001958.11105-6-olteanv@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
XSPI allows for 2 extra features:
- Command cycling (use a single TX command with more than 1 word in the
TX FIFO).
- Increased word size (from 16 bits to 32 bits)
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Message-Id: <20200302001958.11105-5-olteanv@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Get rid of the ifdef for Coldfire and make these hardware
characteristics part of dspi->devtype_data.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Message-Id: <20200302001958.11105-4-olteanv@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Currently, the device tree bindings submitted in mainline for Layerscape
SoCs look like this:
LS1021A:
compatible = "fsl,ls1021a-v1.0-dspi";
LS1012A:
compatible = "fsl,ls1012a-dspi", "fsl,ls1021a-v1.0-dspi";
LS2085A:
compatible = "fsl,ls2085a-dspi";
LS2088A:
compatible = "fsl,ls2080a-dspi", "fsl,ls2085a-dspi";
LX2160A:
compatible = "fsl,lx2160a-dspi", "fsl,ls2085a-dspi";
LS1043A:
compatible = "fsl,ls1043a-dspi", "fsl,ls1021a-v1.0-dspi";
LS1046A:
compatible = "fsl,ls1021a-v1.0-dspi";
Due to a lack of a more specific compatible string, LS1012A, LS1043A and
LS1046A will fall under the LS1021A umbrella, and LS2088A and LX2160A
under the LS2085A umbrella.
They do work in those modes, but there are slight differences in the
hardware instantiations, mostly related to FIFO sizes (with the more
specific compatible strings, the FIFO size can be increased properly).
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Message-Id: <20200302001958.11105-3-olteanv@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
When used in Extended SPI mode on LS1021A, the DSPI controller wants to
have the least significant 16-bit word written first to the TX FIFO.
In fact, the LS1021A reference manual says:
33.5.2.4.2 Draining the TX FIFO
When Extended SPI Mode (DSPIx_MCR[XSPI]) is enabled, if the frame size
of SPI Data to be transmitted is more than 16 bits, then it causes two
Data entries to be popped from TX FIFO simultaneously which are
transferred to the shift register. The first of the two popped entries
forms the 16 least significant bits of the SPI frame to be transmitted.
So given the following TX buffer:
+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 0x0 | 0x1 | 0x2 | 0x3 | 0x4 | 0x5 | 0x6 | 0x7 | 0x8 | 0x9 | 0xa | 0xb |
+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+-----+
| 32-bit word 1 | 32-bit word 2 | 32-bit word 3 |
+-----------------------+-----------------------+-----------------------+
The correct way that a little-endian system should transmit it on the
wire when bits_per_word is 32 is:
0x03020100
0x07060504
0x0b0a0908
But it is actually transmitted as following, as seen with a scope:
0x01000302
0x05040706
0x09080b0a
It appears that this patch has been submitted at least once before:
https://lkml.org/lkml/2018/9/21/286
but in that case Chuanhua Han did not manage to explain the problem
clearly enough and the patch did not get merged, leaving XSPI mode
broken.
Fixes: 8fcd151d26 ("spi: spi-fsl-dspi: XSPI FIFO handling (in TCFQ mode)")
Cc: Esben Haabendal <eha@deif.com>
Cc: Chuanhua Han <chuanhua.han@nxp.com>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Link: https://lore.kernel.org/r/20191228135536.14284-1-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
The API for PTP system timestamping (associating a SPI transaction with
the system time at which it was transferred) is flawed: it assumes that
the xfer->tx_buf pointer will always be present.
This is, of course, not always the case.
So introduce a "progress" variable that denotes how many word have been
transferred.
Fix the Freescale DSPI driver, the only user of the API so far, in the
same patch.
Fixes: b42faeee71 ("spi: Add a PTP system timestamp to the transfer structure")
Fixes: d6b71dfaee ("spi: spi-fsl-dspi: Implement the PTP system timestamping for TCFQ mode")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Link: https://lore.kernel.org/r/20191227012417.1057-1-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
dma_request_slave_channel() is a wrapper on top of dma_request_chan()
eating up the error code.
By using dma_request_chan() directly the driver can support deferred
probing against DMA.
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Link: https://lore.kernel.org/r/20191212135550.4634-8-peter.ujfalusi@ti.com
Signed-off-by: Mark Brown <broonie@kernel.org>
For many places in the spi drivers, using the new `spi_transfer_delay`
helper is straightforward.
It's just replacing:
```
if (t->delay_usecs)
udelay(t->delay_usecs);
```
with `spi_transfer_delay(t)` which handles both `delay_usecs` and the new
`delay` field.
This change replaces in all places (in the spi drivers) where this change
is simple.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Link: https://lore.kernel.org/r/20190926105147.7839-10-alexandru.ardelean@analog.com
Signed-off-by: Mark Brown <broonie@kernel.org>
In this mode, the DSPI controller uses PIO to transfer word by word. In
comparison, in EOQ mode the 4-word deep FIFO is being used, hence the
current logic will need some adaptation for which I do not have the
hardware (Coldfire) to test. It is not clear what is the timing of DMA
transfers and whether timestamping in the driver brings any overall
performance increase compared to regular timestamping done in the core.
Short phc2sys summary after 58 minutes of running on LS1021A-TSN with
interrupts disabled during the critical section:
offset: min -26251 max 16416 mean -21.8672 std dev 863.416
delay: min 4720 max 57280 mean 5182.49 std dev 1607.19
lost servo lock 3 times
Summary of the same phc2sys service running for 120 minutes with
interrupts disabled:
offset: min -378 max 381 mean -0.0083089 std dev 101.495
delay: min 4720 max 5920 mean 5129.38 std dev 154.899
lost servo lock 0 times
The minimum delay (pre to post time) in nanoseconds is the same, but the
maximum delay is quite a bit higher, due to interrupts getting sometimes
executed and interfering with the measurement. Hence set disable_irqs
whenever possible (aka when the driver runs in poll mode - otherwise it
would be a contradiction in terms).
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Link: https://lore.kernel.org/r/20190905010114.26718-4-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
With this patch, the "interrupts" property from the device tree bindings
is ignored, even if present, if the driver runs in TCFQ mode.
Switching to using the DSPI in poll mode has several distinct
benefits:
- With interrupts, the DSPI driver in TCFQ mode raises an IRQ after each
transmitted word. There is more time wasted for the "waitq" event than
for actual I/O. And the DSPI IRQ count can easily get the largest in
/proc/interrupts on Freescale boards with attached SPI devices.
- The SPI I/O time is both lower, and more consistently so. Attached to
some Freescale devices are either PTP switches, or SPI RTCs. For
reading time off of a SPI slave device, it is important that all SPI
transfers take a deterministic time to complete.
- In poll mode there is much less time spent by the CPU in hardirq
context, which helps with the response latency of the system, and at
the same time there is more control over when interrupts must be
disabled (to get a precise timestamp measurement, which will come in a
future patch): win-win.
On the LS1021A-TSN board, where the SPI device is a SJA1105 PTP switch
(with a bits_per_word=8 driver), I created a "benchmark" where I
periodically transferred a 12-byte message once per second, for 120
seconds. I then recorded the time before putting the first byte in the
TX FIFO, and the time after reading the last byte from the RX FIFO. That
is the transfer delay in nanoseconds.
Interrupt mode:
delay: min 125120 max 168320 mean 150286 std dev 17675.3
Poll mode:
delay: min 69440 max 119040 mean 70312.9 std dev 8065.34
Both the mean latency and the standard deviation are more than 50% lower
in poll mode than in interrupt mode, and the 'max' in poll mode is lower
than the 'min' in interrupt mode. This is with an 'ondemand' governor on
an otherwise idle system - therefore running mostly at 600 MHz out of a
max of 1200 MHz.
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Link: https://lore.kernel.org/r/20191001205216.32115-1-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Mark Brown