[PATCH v3 0/7] NXP DSPI bugfixes and support for LS1028A

[PATCH v3 0/7] NXP DSPI bugfixes and support for LS1028A

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This series addresses a few issues that were missed during the previous
series "[PATCH 00/12] TCFQ to XSPI migration for NXP DSPI driver", on
SoCs other than LS1021A and LS1043A. DMA mode has been completely broken
by that series, and XSPI mode never worked on little-endian controllers.

Then it introduces support for the LS1028A chip, whose compatible has
recently been documented here:

https://lore.kernel.org/linux-devicetree/20200218171418.18297-1-michael@walle.cc/

The device tree for the LS1028A SoC is extended with DMA channels
definition, such that even though the default operating mode is XSPI,
one can simply change DSPI_XSPI_MODE to DSPI_DMA_MODE in the
devtype_data structure of the driver and use that instead.

I don't expect the "fixes" patches to reach very far down the stable
pipe, since there has been pretty heavy refactoring in this driver.

For testing, benchmarking and debugging, the mikroBUS connector on the
LS1028A-RDB is made available via spidev.

Vladimir Oltean (7):
  spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
  spi: spi-fsl-dspi: Avoid use-after-free in interrupt mode
  spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
  spi: spi-fsl-dspi: Fix bits-per-word acceleration in DMA mode
  spi: spi-fsl-dspi: Add support for LS1028A
  arm64: dts: ls1028a: Specify the DMA channels for the DSPI controllers
  arm64: dts: ls1028a-rdb: Add a spidev node for the mikroBUS

 .../boot/dts/freescale/fsl-ls1028a-rdb.dts    |  14 ++
 .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi |   6 +
 drivers/spi/spi-fsl-dspi.c                    | 188 +++++++++++-------
 3 files changed, 134 insertions(+), 74 deletions(-)

-- 
2.17.1

[PATCH v3 2/7] spi: spi-fsl-dspi: Avoid use-after-free in interrupt mode

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

When the wait_event_interruptible call fails, there is still a chance
that the dspi_interrupt may arrive (for example the kernel thread doing
the SPI pump really is interrupted).

In that case, dspi_transfer_one_message will return execution all the
way to the spi_device driver, which may free the spi_message and
spi_transfer structures.

But when the interrupt arrives, the driver still accesses those
structures, leading to use-after-free issues as can be seen below:

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 ]---

The solution is to mask interrupts when we know we can't deal with them.
Also, to make sure they won't bite when we enable them back, we clear
the status register such that stale interrupt requests will just be lost
(as is expected on timeout or wait interruption).

Fixes: 349ad66c0ab0 ("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>
---
Changes in v3:
Patch is new.

 drivers/spi/spi-fsl-dspi.c | 63 +++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 0ce26c1cbf62..f2ba0731aebe 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -911,15 +911,39 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void dspi_enable_interrupts(struct fsl_dspi *dspi, bool on)
+{
+	u32 spi_rser = 0;
+
+	if (on) {
+		switch (dspi->devtype_data->trans_mode) {
+		case DSPI_EOQ_MODE:
+			spi_rser = SPI_RSER_EOQFE;
+			break;
+		case DSPI_XSPI_MODE:
+			spi_rser = SPI_RSER_CMDTCFE;
+			break;
+		default:
+			/* Interrupts not necessary for DMA mode */
+			return;
+		}
+	}
+
+	regmap_write(dspi->regmap, SPI_SR, SPI_SR_CLEAR);
+	regmap_write(dspi->regmap, SPI_RSER, spi_rser);
+}
+
 static int dspi_transfer_one_message(struct spi_controller *ctlr,
 				     struct spi_message *message)
 {
 	struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
 	struct spi_device *spi = message->spi;
-	enum dspi_trans_mode trans_mode;
 	struct spi_transfer *transfer;
 	int status = 0;
 
+	if (dspi->irq)
+		dspi_enable_interrupts(dspi, true);
+
 	message->actual_length = 0;
 
 	list_for_each_entry(transfer, &message->transfers, transfer_list) {
@@ -965,37 +989,24 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 		spi_take_timestamp_pre(dspi->ctlr, dspi->cur_transfer,
 				       dspi->progress, !dspi->irq);
 
-		trans_mode = dspi->devtype_data->trans_mode;
-		switch (trans_mode) {
-		case DSPI_EOQ_MODE:
-			regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_EOQFE);
-			dspi_fifo_write(dspi);
-			break;
-		case DSPI_XSPI_MODE:
-			regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_CMDTCFE);
-			dspi_fifo_write(dspi);
-			break;
-		case DSPI_DMA_MODE:
+		if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
 			regmap_write(dspi->regmap, SPI_RSER,
 				     SPI_RSER_TFFFE | SPI_RSER_TFFFD |
 				     SPI_RSER_RFDFE | SPI_RSER_RFDFD);
 			status = dspi_dma_xfer(dspi);
-			break;
-		default:
-			dev_err(&dspi->pdev->dev, "unsupported trans_mode %u\n",
-				trans_mode);
-			status = -EINVAL;
-			goto out;
-		}
+			if (status)
+				goto out;
+		} else if (dspi->irq) {
+			/* Kick off the interrupt train */
+			dspi_fifo_write(dspi);
 
-		if (!dspi->irq) {
-			do {
-				status = dspi_poll(dspi);
-			} while (status == -EINPROGRESS);
-		} else if (trans_mode != DSPI_DMA_MODE) {
 			status = wait_event_interruptible(dspi->waitq,
 							  dspi->waitflags);
 			dspi->waitflags = 0;
+		} else {
+			do {
+				status = dspi_poll(dspi);
+			} while (status == -EINPROGRESS);
 		}
 		if (status)
 			dev_err(&dspi->pdev->dev,
@@ -1005,6 +1016,9 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
 	}
 
 out:
+	if (dspi->irq)
+		dspi_enable_interrupts(dspi, false);
+
 	message->status = status;
 	spi_finalize_current_message(ctlr);
 
@@ -1208,7 +1222,6 @@ static void dspi_init(struct fsl_dspi *dspi)
 		mcr |= SPI_MCR_MASTER;
 
 	regmap_write(dspi->regmap, SPI_MCR, mcr);
-	regmap_write(dspi->regmap, SPI_SR, SPI_SR_CLEAR);
 }
 
 static int dspi_slave_abort(struct spi_master *master)
-- 
2.17.1

[PATCH v3 5/7] spi: spi-fsl-dspi: Add support for LS1028A

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

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>
---
Changes in v3:
Removed the dma_bufsize variable (obsoleted by 4/7).

Changes in v2:
Switch to DSPI_XSPI_MODE.

 drivers/spi/spi-fsl-dspi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 8f5d18dc78d5..fd1f04b996f7 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -124,6 +124,7 @@ struct fsl_dspi_devtype_data {
 enum {
 	LS1021A,
 	LS1012A,
+	LS1028A,
 	LS1043A,
 	LS1046A,
 	LS2080A,
@@ -151,6 +152,11 @@ static const struct fsl_dspi_devtype_data devtype_data[] = {
 		.max_clock_factor	= 8,
 		.fifo_size		= 16,
 	},
+	[LS1028A] = {
+		.trans_mode		= DSPI_XSPI_MODE,
+		.max_clock_factor	= 8,
+		.fifo_size		= 4,
+	},
 	[LS1043A] = {
 		/* Has A-011218 DMA erratum */
 		.trans_mode		= DSPI_XSPI_MODE,
@@ -1112,6 +1118,9 @@ static const struct of_device_id fsl_dspi_dt_ids[] = {
 	}, {
 		.compatible = "fsl,ls1012a-dspi",
 		.data = &devtype_data[LS1012A],
+	}, {
+		.compatible = "fsl,ls1028a-dspi",
+		.data = &devtype_data[LS1028A],
 	}, {
 		.compatible = "fsl,ls1043a-dspi",
 		.data = &devtype_data[LS1043A],
-- 
2.17.1

[PATCH v3 6/7] arm64: dts: ls1028a: Specify the DMA channels for the DSPI controllers

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

LS1028A has a functional connection to the eDMA module. Even if the
spi-fsl-dspi.c driver is not using DMA for LS1028A now, define the slots
in the DMAMUX for connecting the eDMA channels to the 3 DSPI
controllers.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v3:
None.

Changes in v2:
None.

 arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
index 515e0a1b934f..18155273a46e 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
@@ -298,6 +298,8 @@
 			interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "dspi";
 			clocks = <&clockgen 4 1>;
+			dmas = <&edma0 0 62>, <&edma0 0 60>;
+			dma-names = "tx", "rx";
 			spi-num-chipselects = <4>;
 			little-endian;
 			status = "disabled";
@@ -311,6 +313,8 @@
 			interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "dspi";
 			clocks = <&clockgen 4 1>;
+			dmas = <&edma0 0 58>, <&edma0 0 56>;
+			dma-names = "tx", "rx";
 			spi-num-chipselects = <4>;
 			little-endian;
 			status = "disabled";
@@ -324,6 +328,8 @@
 			interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "dspi";
 			clocks = <&clockgen 4 1>;
+			dmas = <&edma0 0 54>, <&edma0 0 2>;
+			dma-names = "tx", "rx";
 			spi-num-chipselects = <3>;
 			little-endian;
 			status = "disabled";
-- 
2.17.1

[PATCH v3 1/7] spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.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: 349ad66c0ab0 ("spi:Add Freescale DSPI driver for Vybrid VF610 platform")
Signed-off-by: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
---
Changes in v3:
None.

Changes in v2:
Remove duplicate phrase in commit message.

 drivers/spi/spi-fsl-dspi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 0683a3fbd48c..0ce26c1cbf62 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -22,7 +22,7 @@
 
 #define SPI_MCR				0x00
 #define SPI_MCR_MASTER			BIT(31)
-#define SPI_MCR_PCSIS			(0x3F << 16)
+#define SPI_MCR_PCSIS(x)		((x) << 16)
 #define SPI_MCR_CLR_TXF			BIT(11)
 #define SPI_MCR_CLR_RXF			BIT(10)
 #define SPI_MCR_XSPI			BIT(3)
@@ -1197,7 +1197,10 @@ static const struct regmap_config dspi_xspi_regmap_config[] = {
 
 static void dspi_init(struct fsl_dspi *dspi)
 {
-	unsigned int mcr = SPI_MCR_PCSIS;
+	unsigned int mcr;
+
+	/* Set idle states for all chip select signals to high */
+	mcr = SPI_MCR_PCSIS(GENMASK(dspi->ctlr->num_chipselect - 1, 0));
 
 	if (dspi->devtype_data->trans_mode == DSPI_XSPI_MODE)
 		mcr |= SPI_MCR_XSPI;
-- 
2.17.1

[PATCH v3 3/7] spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.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: 58ba07ec79e6 ("spi: spi-fsl-dspi: Add support for XSPI mode registers")
Signed-off-by: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
---
Changes in v3:
None.

Changes in v2:
Parse "big-endian" device tree bindings instead of taking the decision
based on compatible SoC.

 drivers/spi/spi-fsl-dspi.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index f2ba0731aebe..c59b68592283 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -103,10 +103,6 @@
 #define SPI_FRAME_BITS(bits)		SPI_CTAR_FMSZ((bits) - 1)
 #define SPI_FRAME_EBITS(bits)		SPI_CTARE_FMSZE(((bits) - 1) >> 4)
 
-/* Register offsets for regmap_pushr */
-#define PUSHR_CMD			0x0
-#define PUSHR_TX			0x2
-
 #define DMA_COMPLETION_TIMEOUT		msecs_to_jiffies(3000)
 
 struct chip_data {
@@ -240,6 +236,13 @@ struct fsl_dspi {
 
 	int					words_in_flight;
 
+	/*
+	 * Offsets for CMD and TXDATA within SPI_PUSHR when accessed
+	 * individually (in XSPI mode)
+	 */
+	int					pushr_cmd;
+	int					pushr_tx;
+
 	void (*host_to_dev)(struct fsl_dspi *dspi, u32 *txdata);
 	void (*dev_to_host)(struct fsl_dspi *dspi, u32 rxdata);
 };
@@ -670,12 +673,12 @@ static void dspi_pushr_cmd_write(struct fsl_dspi *dspi, u16 cmd)
 	 */
 	if (dspi->len > dspi->oper_word_size)
 		cmd |= SPI_PUSHR_CMD_CONT;
-	regmap_write(dspi->regmap_pushr, PUSHR_CMD, cmd);
+	regmap_write(dspi->regmap_pushr, dspi->pushr_cmd, cmd);
 }
 
 static void dspi_pushr_txdata_write(struct fsl_dspi *dspi, u16 txdata)
 {
-	regmap_write(dspi->regmap_pushr, PUSHR_TX, txdata);
+	regmap_write(dspi->regmap_pushr, dspi->pushr_tx, txdata);
 }
 
 static void dspi_xspi_write(struct fsl_dspi *dspi, int cnt, bool eoq)
@@ -1269,6 +1272,7 @@ static int dspi_probe(struct platform_device *pdev)
 	struct fsl_dspi *dspi;
 	struct resource *res;
 	void __iomem *base;
+	bool big_endian;
 
 	ctlr = spi_alloc_master(&pdev->dev, sizeof(struct fsl_dspi));
 	if (!ctlr)
@@ -1294,6 +1298,7 @@ static int dspi_probe(struct platform_device *pdev)
 
 		/* Only Coldfire uses platform data */
 		dspi->devtype_data = &devtype_data[MCF5441X];
+		big_endian = true;
 	} else {
 
 		ret = of_property_read_u32(np, "spi-num-chipselects", &cs_num);
@@ -1315,6 +1320,15 @@ static int dspi_probe(struct platform_device *pdev)
 			ret = -EFAULT;
 			goto out_ctlr_put;
 		}
+
+		big_endian = of_device_is_big_endian(np);
+	}
+	if (big_endian) {
+		dspi->pushr_cmd = 0;
+		dspi->pushr_tx = 2;
+	} else {
+		dspi->pushr_cmd = 2;
+		dspi->pushr_tx = 0;
 	}
 
 	if (dspi->devtype_data->trans_mode == DSPI_XSPI_MODE)
-- 
2.17.1

[PATCH v3 4/7] spi: spi-fsl-dspi: Fix bits-per-word acceleration in DMA mode

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.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: 6c1c26ecd9a3 ("spi: spi-fsl-dspi: Accelerate transfers using larger word size if possible")
Signed-off-by: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
---
Changes in v3:
Pretty much re-did the patch. Before, dspi_setup_accel was called just
once at the beginning of dspi_dma_xfer. Now it is called in the while
loop. Everything else is just refactoring that follows along.

Changes in v2:
None.

 drivers/spi/spi-fsl-dspi.c | 7 +++++--
 drivers/spi/spi-fsl-dspi.c | 83 +++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 41 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index c59b68592283..8f5d18dc78d5 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -119,7 +119,6 @@ struct fsl_dspi_devtype_data {
 	enum dspi_trans_mode	trans_mode;
 	u8			max_clock_factor;
 	int			fifo_size;
-	int			dma_bufsize;
 };
 
 enum {
@@ -138,7 +137,6 @@ static const struct fsl_dspi_devtype_data devtype_data[] = {
 	[VF610] = {
 		.trans_mode		= DSPI_DMA_MODE,
 		.max_clock_factor	= 2,
-		.dma_bufsize		= 4096,
 		.fifo_size		= 4,
 	},
 	[LS1021A] = {
@@ -167,19 +165,16 @@ static const struct fsl_dspi_devtype_data devtype_data[] = {
 	},
 	[LS2080A] = {
 		.trans_mode		= DSPI_DMA_MODE,
-		.dma_bufsize		= 8,
 		.max_clock_factor	= 8,
 		.fifo_size		= 4,
 	},
 	[LS2085A] = {
 		.trans_mode		= DSPI_DMA_MODE,
-		.dma_bufsize		= 8,
 		.max_clock_factor	= 8,
 		.fifo_size		= 4,
 	},
 	[LX2160A] = {
 		.trans_mode		= DSPI_DMA_MODE,
-		.dma_bufsize		= 8,
 		.max_clock_factor	= 8,
 		.fifo_size		= 4,
 	},
@@ -191,9 +186,6 @@ static const struct fsl_dspi_devtype_data devtype_data[] = {
 };
 
 struct fsl_dspi_dma {
-	/* Length of transfer in words of dspi->fifo_size */
-	u32					curr_xfer_len;
-
 	u32					*tx_dma_buf;
 	struct dma_chan				*chan_tx;
 	dma_addr_t				tx_dma_phys;
@@ -352,7 +344,7 @@ static void dspi_rx_dma_callback(void *arg)
 	int i;
 
 	if (dspi->rx) {
-		for (i = 0; i < dma->curr_xfer_len; i++)
+		for (i = 0; i < dspi->words_in_flight; i++)
 			dspi_push_rx(dspi, dspi->dma->rx_dma_buf[i]);
 	}
 
@@ -366,12 +358,12 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 	int time_left;
 	int i;
 
-	for (i = 0; i < dma->curr_xfer_len; i++)
+	for (i = 0; i < dspi->words_in_flight; i++)
 		dspi->dma->tx_dma_buf[i] = dspi_pop_tx_pushr(dspi);
 
 	dma->tx_desc = dmaengine_prep_slave_single(dma->chan_tx,
 					dma->tx_dma_phys,
-					dma->curr_xfer_len *
+					dspi->words_in_flight *
 					DMA_SLAVE_BUSWIDTH_4_BYTES,
 					DMA_MEM_TO_DEV,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -389,7 +381,7 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 
 	dma->rx_desc = dmaengine_prep_slave_single(dma->chan_rx,
 					dma->rx_dma_phys,
-					dma->curr_xfer_len *
+					dspi->words_in_flight *
 					DMA_SLAVE_BUSWIDTH_4_BYTES,
 					DMA_DEV_TO_MEM,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -437,46 +429,56 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 	return 0;
 }
 
+static void dspi_setup_accel(struct fsl_dspi *dspi);
+
 static int dspi_dma_xfer(struct fsl_dspi *dspi)
 {
 	struct spi_message *message = dspi->cur_msg;
 	struct device *dev = &dspi->pdev->dev;
-	struct fsl_dspi_dma *dma = dspi->dma;
-	int curr_remaining_bytes;
-	int bytes_per_buffer;
+	int bytes_in_flight = dspi->len;
+	int chunk_size;
 	int ret = 0;
 
-	curr_remaining_bytes = dspi->len;
-	bytes_per_buffer = dspi->devtype_data->dma_bufsize /
-			   dspi->devtype_data->fifo_size;
-	while (curr_remaining_bytes) {
+	/*
+	 * dspi->len gets decremented by dspi_pop_tx_pushr in
+	 * dspi_next_xfer_dma_submit
+	 */
+	while (dspi->len) {
+		/* Figure out operational bits-per-word for this chunk */
+		dspi_setup_accel(dspi);
+
+		/*
+		 * If the 16-bit TXDATA of the PUSHR is underutilized, then
+		 * each DMA buffer will be able to hold only up to fifo_size
+		 * useful bytes.
+		 */
+		if (dspi->oper_word_size == 1)
+			chunk_size = dspi->devtype_data->fifo_size;
+		else
+			chunk_size = dspi->devtype_data->fifo_size * 2;
+
 		/* Check if current transfer fits the DMA buffer */
-		dma->curr_xfer_len = curr_remaining_bytes /
-				     dspi->oper_word_size;
-		if (dma->curr_xfer_len > bytes_per_buffer)
-			dma->curr_xfer_len = bytes_per_buffer;
+		bytes_in_flight = dspi->len;
+		if (bytes_in_flight > chunk_size)
+			bytes_in_flight = chunk_size;
+
+		dspi->words_in_flight = bytes_in_flight / dspi->oper_word_size;
 
 		ret = dspi_next_xfer_dma_submit(dspi);
 		if (ret) {
 			dev_err(dev, "DMA transfer failed\n");
-			goto exit;
-
-		} else {
-			const int len = dma->curr_xfer_len *
-					dspi->oper_word_size;
-			curr_remaining_bytes -= len;
-			message->actual_length += len;
-			if (curr_remaining_bytes < 0)
-				curr_remaining_bytes = 0;
+			break;
 		}
+
+		message->actual_length += bytes_in_flight;
 	}
 
-exit:
 	return ret;
 }
 
 static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
 {
+	int dma_bufsize = dspi->devtype_data->fifo_size * 2;
 	struct device *dev = &dspi->pdev->dev;
 	struct dma_slave_config cfg;
 	struct fsl_dspi_dma *dma;
@@ -500,14 +502,14 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
 		goto err_tx_channel;
 	}
 
-	dma->tx_dma_buf = dma_alloc_coherent(dev, dspi->devtype_data->dma_bufsize,
+	dma->tx_dma_buf = dma_alloc_coherent(dev, dma_bufsize,
 					     &dma->tx_dma_phys, GFP_KERNEL);
 	if (!dma->tx_dma_buf) {
 		ret = -ENOMEM;
 		goto err_tx_dma_buf;
 	}
 
-	dma->rx_dma_buf = dma_alloc_coherent(dev, dspi->devtype_data->dma_bufsize,
+	dma->rx_dma_buf = dma_alloc_coherent(dev, dma_bufsize,
 					     &dma->rx_dma_phys, GFP_KERNEL);
 	if (!dma->rx_dma_buf) {
 		ret = -ENOMEM;
@@ -544,10 +546,10 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
 	return 0;
 
 err_slave_config:
-	dma_free_coherent(dev, dspi->devtype_data->dma_bufsize,
+	dma_free_coherent(dev, dma_bufsize,
 			  dma->rx_dma_buf, dma->rx_dma_phys);
 err_rx_dma_buf:
-	dma_free_coherent(dev, dspi->devtype_data->dma_bufsize,
+	dma_free_coherent(dev, dma_bufsize,
 			  dma->tx_dma_buf, dma->tx_dma_phys);
 err_tx_dma_buf:
 	dma_release_channel(dma->chan_tx);
@@ -562,6 +564,7 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
 
 static void dspi_release_dma(struct fsl_dspi *dspi)
 {
+	int dma_bufsize = dspi->devtype_data->fifo_size * 2;
 	struct fsl_dspi_dma *dma = dspi->dma;
 	struct device *dev = &dspi->pdev->dev;
 
@@ -570,15 +573,13 @@ static void dspi_release_dma(struct fsl_dspi *dspi)
 
 	if (dma->chan_tx) {
 		dma_unmap_single(dev, dma->tx_dma_phys,
-				 dspi->devtype_data->dma_bufsize,
-				 DMA_TO_DEVICE);
+				 dma_bufsize, DMA_TO_DEVICE);
 		dma_release_channel(dma->chan_tx);
 	}
 
 	if (dma->chan_rx) {
 		dma_unmap_single(dev, dma->rx_dma_phys,
-				 dspi->devtype_data->dma_bufsize,
-				 DMA_FROM_DEVICE);
+				 dma_bufsize, DMA_FROM_DEVICE);
 		dma_release_channel(dma->chan_rx);
 	}
 }
-- 
2.17.1

[PATCH v3 7/7] arm64: dts: ls1028a-rdb: Add a spidev node for the mikroBUS

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>

For debugging, it is useful to have access to the DSPI controller
signals. On the reference design board, these are exported to either the
mikroBUS1 or mikroBUS2 connector (according to the CPLD register
BRDCFG3[SPI3]).

Signed-off-by: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
---
Changes in v3:
None.

Changes in v2:
Change compatible string for spidev node.

 arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
index bb7ba3bcbe56..13555ed52b89 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-rdb.dts
@@ -83,6 +83,20 @@
 	};
 };
 
+&dspi2 {
+	bus-num = <2>;
+	status = "okay";
+
+	/* mikroBUS1 */
+	spidev@0 {
+		compatible = "rohm,dh2228fv";
+		spi-max-frequency = <20000000>;
+		fsl,spi-cs-sck-delay = <100>;
+		fsl,spi-sck-cs-delay = <100>;
+		reg = <0>;
+	};
+};
+
 &esdhc {
 	sd-uhs-sdr104;
 	sd-uhs-sdr50;
-- 
2.17.1

Re: [PATCH v3 0/7] NXP DSPI bugfixes and support for LS1028A

[Michael Walle]

Hi Vladimir,

Am 2020-03-10 13:55, schrieb Vladimir Oltean:
> From: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
> 
> This series addresses a few issues that were missed during the previous
> series "[PATCH 00/12] TCFQ to XSPI migration for NXP DSPI driver", on
> SoCs other than LS1021A and LS1043A. DMA mode has been completely 
> broken
> by that series, and XSPI mode never worked on little-endian 
> controllers.
> 
> Then it introduces support for the LS1028A chip, whose compatible has
> recently been documented here:
> 
> https://lore.kernel.org/linux-devicetree/20200218171418.18297-1-michael-QKn5cuLxLXY@public.gmane.org/
> 
> The device tree for the LS1028A SoC is extended with DMA channels
> definition, such that even though the default operating mode is XSPI,
> one can simply change DSPI_XSPI_MODE to DSPI_DMA_MODE in the
> devtype_data structure of the driver and use that instead.
> 
> I don't expect the "fixes" patches to reach very far down the stable
> pipe, since there has been pretty heavy refactoring in this driver.
> 
> For testing, benchmarking and debugging, the mikroBUS connector on the
> LS1028A-RDB is made available via spidev.


XSPI mode, while now I cannot reproduce the kernel oops anymore, I've 
found two
other problems (1), (2). Which are likely the same underlying problem. 
DMA mode
works "better" now, still one problem (3).

(1) It seems like the first write/read/erase after the aborted 
instruction
don't get through:

# hexdump -C /dev/mtd0
00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
[  627.914654] fsl-dspi 2120000.spi: Waiting for transfer to complete 
failed!
^C[  627.921649] spi_master spi1: failed to transfer one message from 
queue

#
# echo huhu > /dev/mtd0
# hexdump -C /dev/mtd0
00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
hexdump: /dev/mtd0: Input/output error
003df000
# echo huhu > /dev/mtd0
# hexdump -C /dev/mtd0
00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff  
|huhu............|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
[  642.738905] fsl-dspi 2120000.spi: Waiting for transfer to complete 
failed!
^C[  642.745832] spi_master spi1: failed to transfer one message from 
queue
#
# flash_erase /dev/mtd0 0 1
Erasing 4 Kibyte @ 0 -- 100 % complete
#
# hexdump -C /dev/mtd0
00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff  
|huhu............|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
hexdump: /dev/mtd0: Input/output error
0023d000
# hexdump -C /dev/mtd0
00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff  
|huhu............|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*

(2) Also, reading the flash, every second time there is (reproducibly) 
an
IO error:

# hexdump -C /dev/mtd0
00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff  
|huhu............|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
01000000
# hexdump -C /dev/mtd0
00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff  
|huhu............|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
hexdump: /dev/mtd0: Input/output error
00dc0000
# hexdump -C /dev/mtd0
00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff  
|huhu............|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
01000000
# hexdump -C /dev/mtd0
00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff  
|huhu............|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
hexdump: /dev/mtd0: Input/output error
00e6a000

(3) Depening on the content length there is also an IO error. Funny 
enough,
the content is still written to the SPI flash.

# echo -n 1 > /dev/mtd10
# echo -n 12 > /dev/mtd10
# echo -n 123 > /dev/mtd10
# echo -n 1234 > /dev/mtd10
# echo -n 12345 > /dev/mtd10
sh: write error: Input/output error
# echo -n 123456 > /dev/mtd10
# echo -n 1234567 > /dev/mtd10
sh: write error: Input/output error
# echo -n 12345678 > /dev/mtd10
# echo -n 123456789 > /dev/mtd10
# echo -n 1234567890 > /dev/mtd10
# echo -n 12345678901 > /dev/mtd10
# echo -n 123456789012 > /dev/mtd10
# echo -n 1234567890123 > /dev/mtd10
sh: write error: Input/output error
# echo -n 12345678901234 > /dev/mtd10
# echo -n 123456789012345 > /dev/mtd10
sh: write error: Input/output error
# echo -n 1234567890123456 > /dev/mtd10
# echo -n 12345678901234567 > /dev/mtd10
# echo -n 123456789012345678 > /dev/mtd10


# flash_erase /dev/mtd10 0 1
Erasing 4 Kibyte @ 0 -- 100 % complete
# hexdump -C /dev/mtd10
00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
^C
# echo -n 12345 > /dev/mtd10
sh: write error: Input/output error
# hexdump -C /dev/mtd10
00000000  31 32 33 34 35 ff ff ff  ff ff ff ff ff ff ff ff  
|12345...........|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
^C


-michael

> 
> Vladimir Oltean (7):
>   spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
>   spi: spi-fsl-dspi: Avoid use-after-free in interrupt mode
>   spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
>   spi: spi-fsl-dspi: Fix bits-per-word acceleration in DMA mode
>   spi: spi-fsl-dspi: Add support for LS1028A
>   arm64: dts: ls1028a: Specify the DMA channels for the DSPI 
> controllers
>   arm64: dts: ls1028a-rdb: Add a spidev node for the mikroBUS
> 
>  .../boot/dts/freescale/fsl-ls1028a-rdb.dts    |  14 ++
>  .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi |   6 +
>  drivers/spi/spi-fsl-dspi.c                    | 188 +++++++++++-------
>  3 files changed, 134 insertions(+), 74 deletions(-)

Re: [PATCH v3 0/7] NXP DSPI bugfixes and support for LS1028A

[Vladimir Oltean]

Hi Michael,

On Tue, 10 Mar 2020 at 16:11, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>

>
> XSPI mode, while now I cannot reproduce the kernel oops anymore, I've
> found two
> other problems (1), (2). Which are likely the same underlying problem.
> DMA mode
> works "better" now, still one problem (3).
>
> (1) It seems like the first write/read/erase after the aborted
> instruction
> don't get through:
>
> # hexdump -C /dev/mtd0
> 00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> [  627.914654] fsl-dspi 2120000.spi: Waiting for transfer to complete
> failed!
> ^C[  627.921649] spi_master spi1: failed to transfer one message from
> queue
>
> #
> # echo huhu > /dev/mtd0
> # hexdump -C /dev/mtd0
> 00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> hexdump: /dev/mtd0: Input/output error
> 003df000
> # echo huhu > /dev/mtd0
> # hexdump -C /dev/mtd0
> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> |huhu............|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> [  642.738905] fsl-dspi 2120000.spi: Waiting for transfer to complete
> failed!
> ^C[  642.745832] spi_master spi1: failed to transfer one message from
> queue
> #
> # flash_erase /dev/mtd0 0 1
> Erasing 4 Kibyte @ 0 -- 100 % complete
> #
> # hexdump -C /dev/mtd0
> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> |huhu............|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> hexdump: /dev/mtd0: Input/output error
> 0023d000
> # hexdump -C /dev/mtd0
> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> |huhu............|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
>
> (2) Also, reading the flash, every second time there is (reproducibly)
> an
> IO error:
>
> # hexdump -C /dev/mtd0
> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> |huhu............|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> 01000000
> # hexdump -C /dev/mtd0
> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> |huhu............|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> hexdump: /dev/mtd0: Input/output error
> 00dc0000
> # hexdump -C /dev/mtd0
> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> |huhu............|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> 01000000
> # hexdump -C /dev/mtd0
> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> |huhu............|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> hexdump: /dev/mtd0: Input/output error
> 00e6a000
>

Just to be clear, issue 2 is seen only after you abort another
transaction, right?

> (3) Depening on the content length there is also an IO error. Funny
> enough,
> the content is still written to the SPI flash.
>
> # echo -n 1 > /dev/mtd10
> # echo -n 12 > /dev/mtd10
> # echo -n 123 > /dev/mtd10
> # echo -n 1234 > /dev/mtd10
> # echo -n 12345 > /dev/mtd10
> sh: write error: Input/output error
> # echo -n 123456 > /dev/mtd10
> # echo -n 1234567 > /dev/mtd10
> sh: write error: Input/output error
> # echo -n 12345678 > /dev/mtd10
> # echo -n 123456789 > /dev/mtd10
> # echo -n 1234567890 > /dev/mtd10
> # echo -n 12345678901 > /dev/mtd10
> # echo -n 123456789012 > /dev/mtd10
> # echo -n 1234567890123 > /dev/mtd10
> sh: write error: Input/output error
> # echo -n 12345678901234 > /dev/mtd10
> # echo -n 123456789012345 > /dev/mtd10
> sh: write error: Input/output error
> # echo -n 1234567890123456 > /dev/mtd10
> # echo -n 12345678901234567 > /dev/mtd10
> # echo -n 123456789012345678 > /dev/mtd10
>
>
> # flash_erase /dev/mtd10 0 1
> Erasing 4 Kibyte @ 0 -- 100 % complete
> # hexdump -C /dev/mtd10
> 00000000  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> ^C
> # echo -n 12345 > /dev/mtd10
> sh: write error: Input/output error
> # hexdump -C /dev/mtd10
> 00000000  31 32 33 34 35 ff ff ff  ff ff ff ff ff ff ff ff
> |12345...........|
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> |................|
> *
> ^C
>

For this one, I think the reported message->actual_length is incorrect
in dspi_dma_xfer, which makes spi-mem scream.

>
> -michael
>
> >
> > Vladimir Oltean (7):
> >   spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
> >   spi: spi-fsl-dspi: Avoid use-after-free in interrupt mode
> >   spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
> >   spi: spi-fsl-dspi: Fix bits-per-word acceleration in DMA mode
> >   spi: spi-fsl-dspi: Add support for LS1028A
> >   arm64: dts: ls1028a: Specify the DMA channels for the DSPI
> > controllers
> >   arm64: dts: ls1028a-rdb: Add a spidev node for the mikroBUS
> >
> >  .../boot/dts/freescale/fsl-ls1028a-rdb.dts    |  14 ++
> >  .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi |   6 +
> >  drivers/spi/spi-fsl-dspi.c                    | 188 +++++++++++-------
> >  3 files changed, 134 insertions(+), 74 deletions(-)

Thanks,
-Vladimir

Re: [PATCH v3 0/7] NXP DSPI bugfixes and support for LS1028A

[Michael Walle]

Hi Vladimir,

Am 2020-03-10 15:56, schrieb Vladimir Oltean:
>> (2) Also, reading the flash, every second time there is (reproducibly)
>> an
>> IO error:
>> 
>> # hexdump -C /dev/mtd0
>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>> |huhu............|
>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> |................|
>> *
>> 01000000
>> # hexdump -C /dev/mtd0
>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>> |huhu............|
>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> |................|
>> *
>> hexdump: /dev/mtd0: Input/output error
>> 00dc0000
>> # hexdump -C /dev/mtd0
>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>> |huhu............|
>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> |................|
>> *
>> 01000000
>> # hexdump -C /dev/mtd0
>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>> |huhu............|
>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> |................|
>> *
>> hexdump: /dev/mtd0: Input/output error
>> 00e6a000
>> 
> 
> Just to be clear, issue 2 is seen only after you abort another
> transaction, right?

No, just normal uninterrupted reading. Just tried it right after
reboot. Doesn't seem to be every second time though, just random
which makes me wonder if that is another problem now. Also the
last successful reading is random.

buildroot login: root
# hexdump -C /dev/mtd0
00000000  31 32 33 34 35 ff ff ff  ff ff ff ff ff ff ff ff  
|12345...........|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
[   32.359156] random: crng init done
01000000
# hexdump -C /dev/mtd0
00000000  31 32 33 34 35 ff ff ff  ff ff ff ff ff ff ff ff  
|12345...........|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
01000000
# hexdump -C /dev/mtd0
00000000  31 32 33 34 35 ff ff ff  ff ff ff ff ff ff ff ff  
|12345...........|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
hexdump: /dev/mtd0: Input/output error
00166000
# hexdump -C /dev/mtd0
00000000  31 32 33 34 35 ff ff ff  ff ff ff ff ff ff ff ff  
|12345...........|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
hexdump: /dev/mtd0: Input/output error
00c68000
# hexdump -C /dev/mtd0
00000000  31 32 33 34 35 ff ff ff  ff ff ff ff ff ff ff ff  
|12345...........|
00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff  
|................|
*
hexdump: /dev/mtd0: Input/output error
00243000
#

-michael

Re: [PATCH v3 0/7] NXP DSPI bugfixes and support for LS1028A

[Michael Walle]

Am 2020-03-10 16:22, schrieb Michael Walle:
> Hi Vladimir,
> 
> Am 2020-03-10 15:56, schrieb Vladimir Oltean:
>>> (2) Also, reading the flash, every second time there is 
>>> (reproducibly)
>>> an
>>> IO error:
>>> 
>>> # hexdump -C /dev/mtd0
>>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>>> |huhu............|
>>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>>> |................|
>>> *
>>> 01000000
>>> # hexdump -C /dev/mtd0
>>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>>> |huhu............|
>>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>>> |................|
>>> *
>>> hexdump: /dev/mtd0: Input/output error
>>> 00dc0000
>>> # hexdump -C /dev/mtd0
>>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>>> |huhu............|
>>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>>> |................|
>>> *
>>> 01000000
>>> # hexdump -C /dev/mtd0
>>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>>> |huhu............|
>>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>>> |................|
>>> *
>>> hexdump: /dev/mtd0: Input/output error
>>> 00e6a000
>>> 
>> 
>> Just to be clear, issue 2 is seen only after you abort another
>> transaction, right?
> 
> No, just normal uninterrupted reading. Just tried it right after
> reboot. Doesn't seem to be every second time though, just random
> which makes me wonder if that is another problem now. Also the
> last successful reading is random.


Ok I guess I know what the root cause is. This is an extract of
the current code:

> static int dspi_transfer_one_message(struct spi_controller *ctlr,
> 				     struct spi_message *message)
> {
> ..
> 	/* Kick off the interrupt train */
> 	dspi_fifo_write(dspi);
> 
> 	status = wait_event_interruptible(dspi->waitq,
> 					  dspi->waitflags);
> 	dspi->waitflags = 0;
> ..
> }
> 
> static int dspi_rxtx(struct fsl_dspi *dspi)
> {
> 	dspi_fifo_read(dspi);
> 
> 	if (!dspi->len)
> 		/* Success! */
> 		return 0;
> 
> 	dspi_fifo_write(dspi);
> 
> 	return -EINPROGRESS;
> }

dspi_rxtx() is used in the ISR. Both dspi_fifo_write() and dspi_rxtx()
access shared data like, dspi->words_in_flight. In the EIO error case
the following bytes_sent is -1, because dspi->words_in_flight is -1.

> /* Update total number of bytes that were transferred */
> bytes_sent = dspi->words_in_flight * dspi->oper_word_size;

words_in_flight is always -1 after dspi_fifo_read() was called. In
the error case, the ISR kicks in right in the middle of the execution
of dspi_fifo_write() in dspi_transfer_one_message().

> static void dspi_fifo_write(struct fsl_dspi *dspi)
> {
> ..
> 	if (dspi->devtype_data->trans_mode == DSPI_EOQ_MODE)
> 		dspi_eoq_fifo_write(dspi);
> 	 else
> 		dspi_xspi_fifo_write(dspi);

Now if the ISR is executed right here..

> 
> 	/* Update total number of bytes that were transferred */
> 	bytes_sent = dspi->words_in_flight * dspi->oper_word_size;

.. words_in_flight might be -1.

> 	msg->actual_length += bytes_sent;

and bytes_sent is negative. And this causes an IO error because
the returned overall message length doesn't match.

> 	dspi->progress += bytes_sent / DIV_ROUND_UP(xfer->bits_per_word, 8);
> ..
> }

I could not reproduce the issue with the following patch. I don't
know if I got the locking correct though or if there is a better
way to go.


diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 8b16de9ed382..578fedeb16a0 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -224,6 +224,7 @@ struct fsl_dspi {
         u16                                     tx_cmd;
         const struct fsl_dspi_devtype_data      *devtype_data;

+       spinlock_t lock;
         wait_queue_head_t                       waitq;
         u32                                     waitflags;

@@ -873,14 +874,20 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)

  static int dspi_rxtx(struct fsl_dspi *dspi)
  {
+       unsigned long flags;
+
+       spin_lock_irqsave(&dspi->lock, flags);
         dspi_fifo_read(dspi);

-       if (!dspi->len)
+       if (!dspi->len) {
                 /* Success! */
+               spin_unlock_irqrestore(&dspi->lock, flags);
                 return 0;
+       }

         dspi_fifo_write(dspi);

+       spin_unlock_irqrestore(&dspi->lock, flags);
         return -EINPROGRESS;
  }

@@ -950,7 +957,9 @@ static int dspi_transfer_one_message(struct 
spi_controller *ctlr,
         struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
         struct spi_device *spi = message->spi;
         struct spi_transfer *transfer;
+       unsigned long flags;
         int status = 0;
+       int i = 0;

         if (dspi->irq)
                 dspi_enable_interrupts(dspi, true);
@@ -1009,7 +1018,9 @@ static int dspi_transfer_one_message(struct 
spi_controller *ctlr,
                                 goto out;
                 } else if (dspi->irq) {
                         /* Kick off the interrupt train */
+                       spin_lock_irqsave(&dspi->lock, flags);
                         dspi_fifo_write(dspi);
+                       spin_unlock_irqrestore(&dspi->lock, flags);

                         status = wait_event_interruptible(dspi->waitq,
                                                           
dspi->waitflags);
@@ -1301,6 +1312,7 @@ static int dspi_probe(struct platform_device 
*pdev)
         ctlr->cleanup = dspi_cleanup;
         ctlr->slave_abort = dspi_slave_abort;
         ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
+       spin_lock_init(&dspi->lock);

         pdata = dev_get_platdata(&pdev->dev);
         if (pdata) {



-michael

Re: [PATCH v3 0/7] NXP DSPI bugfixes and support for LS1028A

[Vladimir Oltean]

Hi Michael,

On Fri, 13 Mar 2020 at 18:07, Michael Walle <michael@walle.cc> wrote:
>
> Am 2020-03-10 16:22, schrieb Michael Walle:
> > Hi Vladimir,
> >
> > Am 2020-03-10 15:56, schrieb Vladimir Oltean:
> >>> (2) Also, reading the flash, every second time there is
> >>> (reproducibly)
> >>> an
> >>> IO error:
> >>>
> >>> # hexdump -C /dev/mtd0
> >>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> >>> |huhu............|
> >>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> >>> |................|
> >>> *
> >>> 01000000
> >>> # hexdump -C /dev/mtd0
> >>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> >>> |huhu............|
> >>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> >>> |................|
> >>> *
> >>> hexdump: /dev/mtd0: Input/output error
> >>> 00dc0000
> >>> # hexdump -C /dev/mtd0
> >>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> >>> |huhu............|
> >>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> >>> |................|
> >>> *
> >>> 01000000
> >>> # hexdump -C /dev/mtd0
> >>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
> >>> |huhu............|
> >>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> >>> |................|
> >>> *
> >>> hexdump: /dev/mtd0: Input/output error
> >>> 00e6a000
> >>>
> >>
> >> Just to be clear, issue 2 is seen only after you abort another
> >> transaction, right?
> >
> > No, just normal uninterrupted reading. Just tried it right after
> > reboot. Doesn't seem to be every second time though, just random
> > which makes me wonder if that is another problem now. Also the
> > last successful reading is random.
>
>
> Ok I guess I know what the root cause is. This is an extract of
> the current code:
>
> > static int dspi_transfer_one_message(struct spi_controller *ctlr,
> >                                    struct spi_message *message)
> > {
> > ..
> >       /* Kick off the interrupt train */
> >       dspi_fifo_write(dspi);
> >
> >       status = wait_event_interruptible(dspi->waitq,
> >                                         dspi->waitflags);
> >       dspi->waitflags = 0;
> > ..
> > }
> >
> > static int dspi_rxtx(struct fsl_dspi *dspi)
> > {
> >       dspi_fifo_read(dspi);
> >
> >       if (!dspi->len)
> >               /* Success! */
> >               return 0;
> >
> >       dspi_fifo_write(dspi);
> >
> >       return -EINPROGRESS;
> > }
>
> dspi_rxtx() is used in the ISR. Both dspi_fifo_write() and dspi_rxtx()
> access shared data like, dspi->words_in_flight. In the EIO error case
> the following bytes_sent is -1, because dspi->words_in_flight is -1.
>
> > /* Update total number of bytes that were transferred */
> > bytes_sent = dspi->words_in_flight * dspi->oper_word_size;
>
> words_in_flight is always -1 after dspi_fifo_read() was called. In
> the error case, the ISR kicks in right in the middle of the execution
> of dspi_fifo_write() in dspi_transfer_one_message().
>
> > static void dspi_fifo_write(struct fsl_dspi *dspi)
> > {
> > ..
> >       if (dspi->devtype_data->trans_mode == DSPI_EOQ_MODE)
> >               dspi_eoq_fifo_write(dspi);
> >        else
> >               dspi_xspi_fifo_write(dspi);
>
> Now if the ISR is executed right here..
>
> >
> >       /* Update total number of bytes that were transferred */
> >       bytes_sent = dspi->words_in_flight * dspi->oper_word_size;
>
> .. words_in_flight might be -1.
>
> >       msg->actual_length += bytes_sent;
>
> and bytes_sent is negative. And this causes an IO error because
> the returned overall message length doesn't match.
>
> >       dspi->progress += bytes_sent / DIV_ROUND_UP(xfer->bits_per_word, 8);
> > ..
> > }
>
> I could not reproduce the issue with the following patch. I don't
> know if I got the locking correct though or if there is a better
> way to go.
>
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 8b16de9ed382..578fedeb16a0 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -224,6 +224,7 @@ struct fsl_dspi {
>          u16                                     tx_cmd;
>          const struct fsl_dspi_devtype_data      *devtype_data;
>
> +       spinlock_t lock;
>          wait_queue_head_t                       waitq;
>          u32                                     waitflags;
>
> @@ -873,14 +874,20 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)
>
>   static int dspi_rxtx(struct fsl_dspi *dspi)
>   {
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&dspi->lock, flags);
>          dspi_fifo_read(dspi);
>
> -       if (!dspi->len)
> +       if (!dspi->len) {
>                  /* Success! */
> +               spin_unlock_irqrestore(&dspi->lock, flags);
>                  return 0;
> +       }
>
>          dspi_fifo_write(dspi);
>
> +       spin_unlock_irqrestore(&dspi->lock, flags);
>          return -EINPROGRESS;
>   }
>
> @@ -950,7 +957,9 @@ static int dspi_transfer_one_message(struct
> spi_controller *ctlr,
>          struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
>          struct spi_device *spi = message->spi;
>          struct spi_transfer *transfer;
> +       unsigned long flags;
>          int status = 0;
> +       int i = 0;
>
>          if (dspi->irq)
>                  dspi_enable_interrupts(dspi, true);
> @@ -1009,7 +1018,9 @@ static int dspi_transfer_one_message(struct
> spi_controller *ctlr,
>                                  goto out;
>                  } else if (dspi->irq) {
>                          /* Kick off the interrupt train */
> +                       spin_lock_irqsave(&dspi->lock, flags);
>                          dspi_fifo_write(dspi);
> +                       spin_unlock_irqrestore(&dspi->lock, flags);
>
>                          status = wait_event_interruptible(dspi->waitq,
>
> dspi->waitflags);
> @@ -1301,6 +1312,7 @@ static int dspi_probe(struct platform_device
> *pdev)
>          ctlr->cleanup = dspi_cleanup;
>          ctlr->slave_abort = dspi_slave_abort;
>          ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
> +       spin_lock_init(&dspi->lock);
>
>          pdata = dev_get_platdata(&pdev->dev);
>          if (pdata) {
>
>
>
> -michael

Thanks for taking such a close look. I haven't had the time to follow up.
Indeed, the ISR, and therefore dspi_fifo_read, can execute before
dspi->words_in_flight was populated correctly. And bad things will
happen in that case.
But I wouldn't introduce a spin lock that disables interrupts on the
local CPU just for that - it's too complicated for this driver.
I would just keep the SPI interrupt quiesced via SPI_RSER and enable
it only once it's safe, aka after updating dspi->words_in_flight.

Thanks,
-Vladimir

Re: [PATCH v3 0/7] NXP DSPI bugfixes and support for LS1028A

[Michael Walle]

Am 2020-03-13 17:37, schrieb Vladimir Oltean:
> Hi Michael,
> 
> On Fri, 13 Mar 2020 at 18:07, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>> 
>> Am 2020-03-10 16:22, schrieb Michael Walle:
>> > Hi Vladimir,
>> >
>> > Am 2020-03-10 15:56, schrieb Vladimir Oltean:
>> >>> (2) Also, reading the flash, every second time there is
>> >>> (reproducibly)
>> >>> an
>> >>> IO error:
>> >>>
>> >>> # hexdump -C /dev/mtd0
>> >>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>> >>> |huhu............|
>> >>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> >>> |................|
>> >>> *
>> >>> 01000000
>> >>> # hexdump -C /dev/mtd0
>> >>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>> >>> |huhu............|
>> >>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> >>> |................|
>> >>> *
>> >>> hexdump: /dev/mtd0: Input/output error
>> >>> 00dc0000
>> >>> # hexdump -C /dev/mtd0
>> >>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>> >>> |huhu............|
>> >>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> >>> |................|
>> >>> *
>> >>> 01000000
>> >>> # hexdump -C /dev/mtd0
>> >>> 00000000  68 75 68 75 0a ff ff ff  ff ff ff ff ff ff ff ff
>> >>> |huhu............|
>> >>> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> >>> |................|
>> >>> *
>> >>> hexdump: /dev/mtd0: Input/output error
>> >>> 00e6a000
>> >>>
>> >>
>> >> Just to be clear, issue 2 is seen only after you abort another
>> >> transaction, right?
>> >
>> > No, just normal uninterrupted reading. Just tried it right after
>> > reboot. Doesn't seem to be every second time though, just random
>> > which makes me wonder if that is another problem now. Also the
>> > last successful reading is random.
>> 
>> 
>> Ok I guess I know what the root cause is. This is an extract of
>> the current code:
>> 
>> > static int dspi_transfer_one_message(struct spi_controller *ctlr,
>> >                                    struct spi_message *message)
>> > {
>> > ..
>> >       /* Kick off the interrupt train */
>> >       dspi_fifo_write(dspi);
>> >
>> >       status = wait_event_interruptible(dspi->waitq,
>> >                                         dspi->waitflags);
>> >       dspi->waitflags = 0;
>> > ..
>> > }
>> >
>> > static int dspi_rxtx(struct fsl_dspi *dspi)
>> > {
>> >       dspi_fifo_read(dspi);
>> >
>> >       if (!dspi->len)
>> >               /* Success! */
>> >               return 0;
>> >
>> >       dspi_fifo_write(dspi);
>> >
>> >       return -EINPROGRESS;
>> > }
>> 
>> dspi_rxtx() is used in the ISR. Both dspi_fifo_write() and dspi_rxtx()
>> access shared data like, dspi->words_in_flight. In the EIO error case
>> the following bytes_sent is -1, because dspi->words_in_flight is -1.
>> 
>> > /* Update total number of bytes that were transferred */
>> > bytes_sent = dspi->words_in_flight * dspi->oper_word_size;
>> 
>> words_in_flight is always -1 after dspi_fifo_read() was called. In
>> the error case, the ISR kicks in right in the middle of the execution
>> of dspi_fifo_write() in dspi_transfer_one_message().
>> 
>> > static void dspi_fifo_write(struct fsl_dspi *dspi)
>> > {
>> > ..
>> >       if (dspi->devtype_data->trans_mode == DSPI_EOQ_MODE)
>> >               dspi_eoq_fifo_write(dspi);
>> >        else
>> >               dspi_xspi_fifo_write(dspi);
>> 
>> Now if the ISR is executed right here..
>> 
>> >
>> >       /* Update total number of bytes that were transferred */
>> >       bytes_sent = dspi->words_in_flight * dspi->oper_word_size;
>> 
>> .. words_in_flight might be -1.
>> 
>> >       msg->actual_length += bytes_sent;
>> 
>> and bytes_sent is negative. And this causes an IO error because
>> the returned overall message length doesn't match.
>> 
>> >       dspi->progress += bytes_sent / DIV_ROUND_UP(xfer->bits_per_word, 8);
>> > ..
>> > }
>> 
>> I could not reproduce the issue with the following patch. I don't
>> know if I got the locking correct though or if there is a better
>> way to go.
>> 
>> 
>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> index 8b16de9ed382..578fedeb16a0 100644
>> --- a/drivers/spi/spi-fsl-dspi.c
>> +++ b/drivers/spi/spi-fsl-dspi.c
>> @@ -224,6 +224,7 @@ struct fsl_dspi {
>>          u16                                     tx_cmd;
>>          const struct fsl_dspi_devtype_data      *devtype_data;
>> 
>> +       spinlock_t lock;
>>          wait_queue_head_t                       waitq;
>>          u32                                     waitflags;
>> 
>> @@ -873,14 +874,20 @@ static void dspi_fifo_write(struct fsl_dspi 
>> *dspi)
>> 
>>   static int dspi_rxtx(struct fsl_dspi *dspi)
>>   {
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&dspi->lock, flags);
>>          dspi_fifo_read(dspi);
>> 
>> -       if (!dspi->len)
>> +       if (!dspi->len) {
>>                  /* Success! */
>> +               spin_unlock_irqrestore(&dspi->lock, flags);
>>                  return 0;
>> +       }
>> 
>>          dspi_fifo_write(dspi);
>> 
>> +       spin_unlock_irqrestore(&dspi->lock, flags);
>>          return -EINPROGRESS;
>>   }
>> 
>> @@ -950,7 +957,9 @@ static int dspi_transfer_one_message(struct
>> spi_controller *ctlr,
>>          struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
>>          struct spi_device *spi = message->spi;
>>          struct spi_transfer *transfer;
>> +       unsigned long flags;
>>          int status = 0;
>> +       int i = 0;
>> 
>>          if (dspi->irq)
>>                  dspi_enable_interrupts(dspi, true);
>> @@ -1009,7 +1018,9 @@ static int dspi_transfer_one_message(struct
>> spi_controller *ctlr,
>>                                  goto out;
>>                  } else if (dspi->irq) {
>>                          /* Kick off the interrupt train */
>> +                       spin_lock_irqsave(&dspi->lock, flags);
>>                          dspi_fifo_write(dspi);
>> +                       spin_unlock_irqrestore(&dspi->lock, flags);
>> 
>>                          status = 
>> wait_event_interruptible(dspi->waitq,
>> 
>> dspi->waitflags);
>> @@ -1301,6 +1312,7 @@ static int dspi_probe(struct platform_device
>> *pdev)
>>          ctlr->cleanup = dspi_cleanup;
>>          ctlr->slave_abort = dspi_slave_abort;
>>          ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST;
>> +       spin_lock_init(&dspi->lock);
>> 
>>          pdata = dev_get_platdata(&pdev->dev);
>>          if (pdata) {
>> 
>> 
>> 
>> -michael
> 
> Thanks for taking such a close look. I haven't had the time to follow 
> up.
> Indeed, the ISR, and therefore dspi_fifo_read, can execute before
> dspi->words_in_flight was populated correctly. And bad things will
> happen in that case.
> But I wouldn't introduce a spin lock that disables interrupts on the
> local CPU just for that - it's too complicated for this driver.

Sure. It was just a quick test whether the problem actually goes away.

> I would just keep the SPI interrupt quiesced via SPI_RSER and enable
> it only once it's safe, aka after updating dspi->words_in_flight.

I didn't want to move the interrupt_enable() around. I leave this up to
you ;)

-michael