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

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

[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.

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

Vladimir Oltean (6):
  spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
  spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
  spi: spi-fsl-dspi: Fix oper_word_size of zero for 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                    | 54 +++++++++++++++----
 3 files changed, 64 insertions(+), 10 deletions(-)

-- 
2.17.1

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

[Vladimir Oltean]

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

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 " 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@nxp.com>
---
 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 2/6] spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA

[Vladimir Oltean]

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

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 define the
offsets per-instantiation, in the devtype_data structure. This means
that we have to know from the driver which controllers are big- and
which are little-endian (which is fine, we do, but it makes the device
tree binding itself a little redundant except for regmap_config).

This patch does not apply cleanly to stable trees, and a punctual fix to
the commit cannot be provided given this constraint of lack of access to
regmap_get_val_endian(). The per-SoC devtype_data structures (and
therefore the premises to fix this bug) have been introduced only a few
commits ago, in commit d35054010b57 ("spi: spi-fsl-dspi: Use specific
compatible strings for all SoC instantiations")

Fixes: 58ba07ec79e6 ("spi: spi-fsl-dspi: Add support for XSPI mode registers")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/spi/spi-fsl-dspi.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 0ce26c1cbf62..a8e56abe20ac 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 {
@@ -124,6 +120,12 @@ struct fsl_dspi_devtype_data {
 	u8			max_clock_factor;
 	int			fifo_size;
 	int			dma_bufsize;
+	/*
+	 * Offsets for CMD and TXDATA within SPI_PUSHR when accessed
+	 * individually (in XSPI mode)
+	 */
+	int			pushr_cmd;
+	int			pushr_tx;
 };
 
 enum {
@@ -150,42 +152,56 @@ static const struct fsl_dspi_devtype_data devtype_data[] = {
 		.trans_mode		= DSPI_XSPI_MODE,
 		.max_clock_factor	= 8,
 		.fifo_size		= 4,
+		.pushr_cmd		= 0,
+		.pushr_tx		= 2,
 	},
 	[LS1012A] = {
 		/* Has A-011218 DMA erratum */
 		.trans_mode		= DSPI_XSPI_MODE,
 		.max_clock_factor	= 8,
 		.fifo_size		= 16,
+		.pushr_cmd		= 0,
+		.pushr_tx		= 2,
 	},
 	[LS1043A] = {
 		/* Has A-011218 DMA erratum */
 		.trans_mode		= DSPI_XSPI_MODE,
 		.max_clock_factor	= 8,
 		.fifo_size		= 16,
+		.pushr_cmd		= 0,
+		.pushr_tx		= 2,
 	},
 	[LS1046A] = {
 		/* Has A-011218 DMA erratum */
 		.trans_mode		= DSPI_XSPI_MODE,
 		.max_clock_factor	= 8,
 		.fifo_size		= 16,
+		.pushr_cmd		= 0,
+		.pushr_tx		= 2,
 	},
 	[LS2080A] = {
 		.trans_mode		= DSPI_DMA_MODE,
 		.dma_bufsize		= 8,
 		.max_clock_factor	= 8,
 		.fifo_size		= 4,
+		.pushr_cmd		= 2,
+		.pushr_tx		= 0,
 	},
 	[LS2085A] = {
 		.trans_mode		= DSPI_DMA_MODE,
 		.dma_bufsize		= 8,
 		.max_clock_factor	= 8,
 		.fifo_size		= 4,
+		.pushr_cmd		= 2,
+		.pushr_tx		= 0,
 	},
 	[LX2160A] = {
 		.trans_mode		= DSPI_DMA_MODE,
 		.dma_bufsize		= 8,
 		.max_clock_factor	= 8,
 		.fifo_size		= 4,
+		.pushr_cmd		= 2,
+		.pushr_tx		= 0,
 	},
 	[MCF5441X] = {
 		.trans_mode		= DSPI_EOQ_MODE,
@@ -670,12 +686,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->devtype_data->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->devtype_data->pushr_tx, txdata);
 }
 
 static void dspi_xspi_write(struct fsl_dspi *dspi, int cnt, bool eoq)
-- 
2.17.1

[PATCH 3/6] spi: spi-fsl-dspi: Fix oper_word_size of zero for 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.

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>
---
 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 a8e56abe20ac..5624b9ee77db 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -372,6 +372,8 @@ static void dspi_rx_dma_callback(void *arg)
 	complete(&dma->cmd_rx_complete);
 }
 
+static void dspi_setup_accel(struct fsl_dspi *dspi);
+
 static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
 {
 	struct device *dev = &dspi->pdev->dev;
@@ -459,9 +461,10 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
 	int bytes_per_buffer;
 	int ret = 0;
 
+	dspi_setup_accel(dspi);
+
 	curr_remaining_bytes = dspi->len;
-	bytes_per_buffer = dspi->devtype_data->dma_bufsize /
-			   dspi->devtype_data->fifo_size;
+	bytes_per_buffer = dspi->devtype_data->dma_bufsize;
 	while (curr_remaining_bytes) {
 		/* Check if current transfer fits the DMA buffer */
 		dma->curr_xfer_len = curr_remaining_bytes /
-- 
2.17.1

[PATCH 4/6] 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>
---
 drivers/spi/spi-fsl-dspi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 5624b9ee77db..264d184e7296 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -131,6 +131,7 @@ struct fsl_dspi_devtype_data {
 enum {
 	LS1021A,
 	LS1012A,
+	LS1028A,
 	LS1043A,
 	LS1046A,
 	LS2080A,
@@ -163,6 +164,14 @@ static const struct fsl_dspi_devtype_data devtype_data[] = {
 		.pushr_cmd		= 0,
 		.pushr_tx		= 2,
 	},
+	[LS1028A] = {
+		.trans_mode		= DSPI_DMA_MODE,
+		.dma_bufsize		= 8,
+		.max_clock_factor	= 8,
+		.fifo_size		= 4,
+		.pushr_cmd		= 2,
+		.pushr_tx		= 0,
+	},
 	[LS1043A] = {
 		/* Has A-011218 DMA erratum */
 		.trans_mode		= DSPI_XSPI_MODE,
@@ -1113,6 +1122,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 5/6] arm64: dts: ls1028a: Specify the DMA channels for the DSPI controllers

[Vladimir Oltean]

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

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-3arQi8VN3Tc@public.gmane.org>
---
 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 6/6] 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>
---
 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..43f403b30dae 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 = "spidev";
+		reg = <0>;
+		spi-max-frequency = <20000000>;
+		fsl,spi-cs-sck-delay = <100>;
+		fsl,spi-sck-cs-delay = <100>;
+	};
+};
+
 &esdhc {
 	sd-uhs-sdr104;
 	sd-uhs-sdr50;
-- 
2.17.1

Re: [PATCH 2/6] spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA

[Michael Walle]

Am 2020-03-09 15:56, schrieb 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 define the
> offsets per-instantiation, in the devtype_data structure. This means
> that we have to know from the driver which controllers are big- and
> which are little-endian (which is fine, we do, but it makes the device
> tree binding itself a little redundant except for regmap_config).
> 
> This patch does not apply cleanly to stable trees, and a punctual fix 
> to
> the commit cannot be provided given this constraint of lack of access 
> to
> regmap_get_val_endian(). The per-SoC devtype_data structures (and
> therefore the premises to fix this bug) have been introduced only a few
> commits ago, in commit d35054010b57 ("spi: spi-fsl-dspi: Use specific
> compatible strings for all SoC instantiations")
> 
> Fixes: 58ba07ec79e6 ("spi: spi-fsl-dspi: Add support for XSPI mode 
> registers")
> Signed-off-by: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
> ---
>  drivers/spi/spi-fsl-dspi.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 0ce26c1cbf62..a8e56abe20ac 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 {
> @@ -124,6 +120,12 @@ struct fsl_dspi_devtype_data {
>  	u8			max_clock_factor;
>  	int			fifo_size;
>  	int			dma_bufsize;
> +	/*
> +	 * Offsets for CMD and TXDATA within SPI_PUSHR when accessed
> +	 * individually (in XSPI mode)
> +	 */
> +	int			pushr_cmd;
> +	int			pushr_tx;
>  };

Shouldn't this just read the "little-endian" property of the
device tree node? Like it worked before with regmap, which takes
the little-endian/big-endian property into account.

If I understand this correctly, this solution would mix the methods
how the IP endianess is determined. Eg. regmap_xx uses the
little-endian property and this driver then also uses the compatible
string to also distinguish between the endianess.

-michael

>  enum {
> @@ -150,42 +152,56 @@ static const struct fsl_dspi_devtype_data
> devtype_data[] = {
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 4,
> +		.pushr_cmd		= 0,
> +		.pushr_tx		= 2,
>  	},
>  	[LS1012A] = {
>  		/* Has A-011218 DMA erratum */
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 16,
> +		.pushr_cmd		= 0,
> +		.pushr_tx		= 2,
>  	},
>  	[LS1043A] = {
>  		/* Has A-011218 DMA erratum */
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 16,
> +		.pushr_cmd		= 0,
> +		.pushr_tx		= 2,
>  	},
>  	[LS1046A] = {
>  		/* Has A-011218 DMA erratum */
>  		.trans_mode		= DSPI_XSPI_MODE,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 16,
> +		.pushr_cmd		= 0,
> +		.pushr_tx		= 2,
>  	},
>  	[LS2080A] = {
>  		.trans_mode		= DSPI_DMA_MODE,
>  		.dma_bufsize		= 8,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 4,
> +		.pushr_cmd		= 2,
> +		.pushr_tx		= 0,
>  	},
>  	[LS2085A] = {
>  		.trans_mode		= DSPI_DMA_MODE,
>  		.dma_bufsize		= 8,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 4,
> +		.pushr_cmd		= 2,
> +		.pushr_tx		= 0,
>  	},
>  	[LX2160A] = {
>  		.trans_mode		= DSPI_DMA_MODE,
>  		.dma_bufsize		= 8,
>  		.max_clock_factor	= 8,
>  		.fifo_size		= 4,
> +		.pushr_cmd		= 2,
> +		.pushr_tx		= 0,
>  	},
>  	[MCF5441X] = {
>  		.trans_mode		= DSPI_EOQ_MODE,
> @@ -670,12 +686,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->devtype_data->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->devtype_data->pushr_tx, 
> txdata);
>  }
> 
>  static void dspi_xspi_write(struct fsl_dspi *dspi, int cnt, bool eoq)

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

[Michael Walle]

Am 2020-03-09 15:56, 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/

If it is not compatible with the LS1021A the second compatible string
should be removed. Depending on the other remark about the endianess,
it might still be compatible, though.


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

wouldn't it make more sense, to use DMA is the dma node is present
in the device tree? otherwise use XSPI mode? I don't think it is
really handy to change the mode inside the driver.

-michael

> 
> For testing, benchmarking and debugging, the mikroBUS connector on the
> LS1028A-RDB is made available via spidev.
> 
> Vladimir Oltean (6):
>   spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
>   spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
>   spi: spi-fsl-dspi: Fix oper_word_size of zero for 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                    | 54 +++++++++++++++----
>  3 files changed, 64 insertions(+), 10 deletions(-)

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

[Michael Walle]

Am 2020-03-09 15:56, schrieb 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 " signals equal to 6. That is not
> always the case, but actually is described through the driver-specific
> "spi-num-chipselects" device tree binding.

Repeated sentence? Was this your intention?

-michael

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

Re: [PATCH 2/6] spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA

[Vladimir Oltean]

Hi Michael,

On Mon, 9 Mar 2020 at 19:59, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>
> Am 2020-03-09 15:56, schrieb 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 define the
> > offsets per-instantiation, in the devtype_data structure. This means
> > that we have to know from the driver which controllers are big- and
> > which are little-endian (which is fine, we do, but it makes the device
> > tree binding itself a little redundant except for regmap_config).
> >
> > This patch does not apply cleanly to stable trees, and a punctual fix
> > to
> > the commit cannot be provided given this constraint of lack of access
> > to
> > regmap_get_val_endian(). The per-SoC devtype_data structures (and
> > therefore the premises to fix this bug) have been introduced only a few
> > commits ago, in commit d35054010b57 ("spi: spi-fsl-dspi: Use specific
> > compatible strings for all SoC instantiations")
> >
> > Fixes: 58ba07ec79e6 ("spi: spi-fsl-dspi: Add support for XSPI mode
> > registers")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
> > ---
> >  drivers/spi/spi-fsl-dspi.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > index 0ce26c1cbf62..a8e56abe20ac 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 {
> > @@ -124,6 +120,12 @@ struct fsl_dspi_devtype_data {
> >       u8                      max_clock_factor;
> >       int                     fifo_size;
> >       int                     dma_bufsize;
> > +     /*
> > +      * Offsets for CMD and TXDATA within SPI_PUSHR when accessed
> > +      * individually (in XSPI mode)
> > +      */
> > +     int                     pushr_cmd;
> > +     int                     pushr_tx;
> >  };
>
> Shouldn't this just read the "little-endian" property of the
> device tree node?

This is exactly what the driver did prior to this commit from 2014:
https://patchwork.kernel.org/patch/4732711/
Since then, "little-endian" and "big-endian" became generic regmap properties.

> Like it worked before with regmap, which takes
> the little-endian/big-endian property into account.
>

So XSPI mode allows you, among other things, to send 32 bits words at a time.
In my opinion this was tested only the big-endian DSPI controllers
(LS1021A, LS1043A etc).
On the little-endian controllers (LS2, LX2, LS1028A) I suspect this
was actually never tested.
The reason why we see it now is because we're "accelerating" even
8-bit words to 32-bit.
So it is incorrect to say "like it worked before": it never worked before.

> If I understand this correctly, this solution would mix the methods
> how the IP endianess is determined. Eg. regmap_xx uses the
> little-endian property and this driver then also uses the compatible
> string to also distinguish between the endianess.
>

Yup. Otherwise we effectively have to duplicate the logic from the
internal regmap_get_val_endian function. I found no other way to
figure out what endianness the regmap_config has. Suggestions welcome.

> -michael
>

Thanks,
-Vladimir

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

[Vladimir Oltean]

On Mon, 9 Mar 2020 at 20:05, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>
> Am 2020-03-09 15:56, schrieb 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 " signals equal to 6. That is not
> > always the case, but actually is described through the driver-specific
> > "spi-num-chipselects" device tree binding.
>
> Repeated sentence? Was this your intention?
>

No, I must have tripped over my vim shortcuts. Sorry.

-Vladimir

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

[Vladimir Oltean]

On Mon, 9 Mar 2020 at 20:03, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
> Am 2020-03-09 15:56, 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/
>
> If it is not compatible with the LS1021A the second compatible string
> should be removed. Depending on the other remark about the endianess,
> it might still be compatible, though.
>

Please feel free to remove it. I wasn't actually planning to add it in
the first place, but now it that it's there it doesn't really bother
anybody either.

>
> > 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.
>
> wouldn't it make more sense, to use DMA is the dma node is present
> in the device tree? otherwise use XSPI mode? I don't think it is
> really handy to change the mode inside the driver.
>

Let's keep it simple. The driver should configure the hardware in the
most efficient and least buggy mode available. Right now that is XSPI.
The hardware description (aka the device tree) is a separate topic. If
there ever arises any situation where there are corner cases with XSPI
mode, it's good to have a fallback in the form of DMA mode, and not
have to worry about yet another problem (which is that there are 2
sets of device tree blobs in deployment).

TL;DR: These DMA channels don't really bother anybody but you never
know when they might come in handy.

> -michael
>
> >
> > For testing, benchmarking and debugging, the mikroBUS connector on the
> > LS1028A-RDB is made available via spidev.
> >
> > Vladimir Oltean (6):
> >   spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
> >   spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
> >   spi: spi-fsl-dspi: Fix oper_word_size of zero for 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                    | 54 +++++++++++++++----
> >  3 files changed, 64 insertions(+), 10 deletions(-)

Thanks,
-Vladimir

Re: [PATCH 2/6] spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA

[Michael Walle]

Am 2020-03-09 19:07, schrieb Vladimir Oltean:
> Hi Michael,
> 
> On Mon, 9 Mar 2020 at 19:59, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>> 
>> Am 2020-03-09 15:56, schrieb 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 define the
>> > offsets per-instantiation, in the devtype_data structure. This means
>> > that we have to know from the driver which controllers are big- and
>> > which are little-endian (which is fine, we do, but it makes the device
>> > tree binding itself a little redundant except for regmap_config).
>> >
>> > This patch does not apply cleanly to stable trees, and a punctual fix
>> > to
>> > the commit cannot be provided given this constraint of lack of access
>> > to
>> > regmap_get_val_endian(). The per-SoC devtype_data structures (and
>> > therefore the premises to fix this bug) have been introduced only a few
>> > commits ago, in commit d35054010b57 ("spi: spi-fsl-dspi: Use specific
>> > compatible strings for all SoC instantiations")
>> >
>> > Fixes: 58ba07ec79e6 ("spi: spi-fsl-dspi: Add support for XSPI mode
>> > registers")
>> > Signed-off-by: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
>> > ---
>> >  drivers/spi/spi-fsl-dspi.c | 28 ++++++++++++++++++++++------
>> >  1 file changed, 22 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> > index 0ce26c1cbf62..a8e56abe20ac 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 {
>> > @@ -124,6 +120,12 @@ struct fsl_dspi_devtype_data {
>> >       u8                      max_clock_factor;
>> >       int                     fifo_size;
>> >       int                     dma_bufsize;
>> > +     /*
>> > +      * Offsets for CMD and TXDATA within SPI_PUSHR when accessed
>> > +      * individually (in XSPI mode)
>> > +      */
>> > +     int                     pushr_cmd;
>> > +     int                     pushr_tx;
>> >  };
>> 
>> Shouldn't this just read the "little-endian" property of the
>> device tree node?
> 
> This is exactly what the driver did prior to this commit from 2014:
> https://patchwork.kernel.org/patch/4732711/
> Since then, "little-endian" and "big-endian" became generic regmap 
> properties.
> 
>> Like it worked before with regmap, which takes
>> the little-endian/big-endian property into account.
>> 
> 
> So XSPI mode allows you, among other things, to send 32 bits words at a 
> time.
> In my opinion this was tested only the big-endian DSPI controllers
> (LS1021A, LS1043A etc).
> On the little-endian controllers (LS2, LX2, LS1028A) I suspect this
> was actually never tested.
> The reason why we see it now is because we're "accelerating" even
> 8-bit words to 32-bit.
> So it is incorrect to say "like it worked before": it never worked 
> before.

I just meant how the endianess is figured out ;) Like by using the
endianess property in the device tree. Not by also taking the compatible
string into account.


>> If I understand this correctly, this solution would mix the methods
>> how the IP endianess is determined. Eg. regmap_xx uses the
>> little-endian property and this driver then also uses the compatible
>> string to also distinguish between the endianess.
>> 
> 
> Yup. Otherwise we effectively have to duplicate the logic from the
> internal regmap_get_val_endian function. I found no other way to
> figure out what endianness the regmap_config has. Suggestions welcome.

If there is no way in determining that, you could just use the
of_property_read_bool(). there is also of_device_is_big_endian()
although I don't know if this is consistent with how the regmap uses it.
Eg. is it big-endian or little-endian if there is no property at all?

I mean something like:

if (little_endian)
     regmap_write(regmap, PUSHR_CMD_LE, cmd)
else
     regmap_write(regmap, PUSHR_CMD_BE, cmd)

Eg. if one would add another SoC to the DSPI driver, you would also need
to specify the cmd/tx offset. But actually its already known because it
just depend on the endianess which is already specified in the device 
tree.

-michael

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

[Michael Walle]

Am 2020-03-09 19:14, schrieb Vladimir Oltean:
> On Mon, 9 Mar 2020 at 20:03, Michael Walle <michael@walle.cc> wrote:
>> Am 2020-03-09 15:56, schrieb 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/
>> 
>> If it is not compatible with the LS1021A the second compatible string
>> should be removed. Depending on the other remark about the endianess,
>> it might still be compatible, though.
>> 
> 
> Please feel free to remove it. I wasn't actually planning to add it in
> the first place, but now it that it's there it doesn't really bother
> anybody either.

But it won't work if the endianess depends on the compatible string ;)

>> 
>> > 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.
>> 
>> wouldn't it make more sense, to use DMA is the dma node is present
>> in the device tree? otherwise use XSPI mode? I don't think it is
>> really handy to change the mode inside the driver.
>> 
> 
> Let's keep it simple. The driver should configure the hardware in the
> most efficient and least buggy mode available. Right now that is XSPI.
> The hardware description (aka the device tree) is a separate topic. If
> there ever arises any situation where there are corner cases with XSPI
> mode, it's good to have a fallback in the form of DMA mode, and not
> have to worry about yet another problem (which is that there are 2
> sets of device tree blobs in deployment).

Point taken. But this is not how other drivers behave, which uses the
DMA if its given in the device node.

Btw. do other SoCs perform better with DMA?

-michael

> TL;DR: These DMA channels don't really bother anybody but you never
> know when they might come in handy.
> 
>> -michael
>> 
>> >
>> > For testing, benchmarking and debugging, the mikroBUS connector on the
>> > LS1028A-RDB is made available via spidev.
>> >
>> > Vladimir Oltean (6):
>> >   spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
>> >   spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
>> >   spi: spi-fsl-dspi: Fix oper_word_size of zero for 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                    | 54 +++++++++++++++----
>> >  3 files changed, 64 insertions(+), 10 deletions(-)
> 
> Thanks,
> -Vladimir

Re: [PATCH 2/6] spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA

[Vladimir Oltean]

On Mon, 9 Mar 2020 at 20:19, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>

> Eg. is it big-endian or little-endian if there is no property at all?
>

I think it is "native endianness" in that case, i.e. big endian on big
endian CPU and little endian on little endian CPU.

Thanks,
-Vladimir

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

[Michael Walle]

Am 2020-03-09 15:56, schrieb 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>
> ---
>  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..43f403b30dae 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 = "spidev";

As far as I know this throws a warning at boot that you
shouldn't use the compatible = "spidev", doesn't it?

/*
  * spidev should never be referenced in DT without a specific
  * compatible string, it is a Linux implementation thing
  * rather than a description of the hardware.
  */

-michael

> +		reg = <0>;
> +		spi-max-frequency = <20000000>;
> +		fsl,spi-cs-sck-delay = <100>;
> +		fsl,spi-sck-cs-delay = <100>;
> +	};
> +};
> +
>  &esdhc {
>  	sd-uhs-sdr104;
>  	sd-uhs-sdr50;

Re: [PATCH 4/6] spi: spi-fsl-dspi: Add support for LS1028A

[Michael Walle]

Am 2020-03-09 15:56, schrieb Vladimir Oltean:
> From: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.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-3arQi8VN3Tc@public.gmane.org>
> ---
>  drivers/spi/spi-fsl-dspi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 5624b9ee77db..264d184e7296 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -131,6 +131,7 @@ struct fsl_dspi_devtype_data {
>  enum {
>  	LS1021A,
>  	LS1012A,
> +	LS1028A,
>  	LS1043A,
>  	LS1046A,
>  	LS2080A,
> @@ -163,6 +164,14 @@ static const struct fsl_dspi_devtype_data
> devtype_data[] = {
>  		.pushr_cmd		= 0,
>  		.pushr_tx		= 2,
>  	},
> +	[LS1028A] = {
> +		.trans_mode		= DSPI_DMA_MODE,

shouldn't this be DSPI_XSPI_MODE according to your cover letter?

-michael

> +		.dma_bufsize		= 8,
> +		.max_clock_factor	= 8,
> +		.fifo_size		= 4,
> +		.pushr_cmd		= 2,
> +		.pushr_tx		= 0,
> +	},
>  	[LS1043A] = {
>  		/* Has A-011218 DMA erratum */
>  		.trans_mode		= DSPI_XSPI_MODE,
> @@ -1113,6 +1122,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],

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

[Vladimir Oltean]

On Mon, 9 Mar 2020 at 20:31, Michael Walle <michael@walle.cc> wrote:
>
> Am 2020-03-09 19:14, schrieb Vladimir Oltean:
> > On Mon, 9 Mar 2020 at 20:03, Michael Walle <michael@walle.cc> wrote:
> >> Am 2020-03-09 15:56, schrieb 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/
> >>
> >> If it is not compatible with the LS1021A the second compatible string
> >> should be removed. Depending on the other remark about the endianess,
> >> it might still be compatible, though.
> >>
> >
> > Please feel free to remove it. I wasn't actually planning to add it in
> > the first place, but now it that it's there it doesn't really bother
> > anybody either.
>
> But it won't work if the endianess depends on the compatible string ;)
>

True.

> >>
> >> > 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.
> >>
> >> wouldn't it make more sense, to use DMA is the dma node is present
> >> in the device tree? otherwise use XSPI mode? I don't think it is
> >> really handy to change the mode inside the driver.
> >>
> >
> > Let's keep it simple. The driver should configure the hardware in the
> > most efficient and least buggy mode available. Right now that is XSPI.
> > The hardware description (aka the device tree) is a separate topic. If
> > there ever arises any situation where there are corner cases with XSPI
> > mode, it's good to have a fallback in the form of DMA mode, and not
> > have to worry about yet another problem (which is that there are 2
> > sets of device tree blobs in deployment).
>
> Point taken. But this is not how other drivers behave, which uses the
> DMA if its given in the device node.
>

Also true.

> Btw. do other SoCs perform better with DMA?
>

Not that I know of.
My general rule of thumb for this controller is: if it supports XSPI
then use that, otherwise use DMA. Luckily there is just one controller
that supports none of those, and that would be Coldfire, which uses
the braindead EOQ mode. I don't have the hardware to do testing on
that, but in principle if I did, I would have converted that as well
to the more functional but less efficient TCFQ mode (now removed).

> -michael
>
> > TL;DR: These DMA channels don't really bother anybody but you never
> > know when they might come in handy.
> >
> >> -michael
> >>
> >> >
> >> > For testing, benchmarking and debugging, the mikroBUS connector on the
> >> > LS1028A-RDB is made available via spidev.
> >> >
> >> > Vladimir Oltean (6):
> >> >   spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
> >> >   spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
> >> >   spi: spi-fsl-dspi: Fix oper_word_size of zero for 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                    | 54 +++++++++++++++----
> >> >  3 files changed, 64 insertions(+), 10 deletions(-)
> >
> > Thanks,
> > -Vladimir

-Vladimir

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

[Vladimir Oltean]

On Mon, 9 Mar 2020 at 20:35, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>
> Am 2020-03-09 15:56, schrieb 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>
> > ---
> >  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..43f403b30dae 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 = "spidev";
>
> As far as I know this throws a warning at boot that you
> shouldn't use the compatible = "spidev", doesn't it?
>
> /*
>   * spidev should never be referenced in DT without a specific
>   * compatible string, it is a Linux implementation thing
>   * rather than a description of the hardware.
>   */
>

If this is supposed to mean that the "spidev" string is less
adequate/expressive than "rohm,dh2228fv", then ok, I'll use that.

> -michael
>
> > +             reg = <0>;
> > +             spi-max-frequency = <20000000>;
> > +             fsl,spi-cs-sck-delay = <100>;
> > +             fsl,spi-sck-cs-delay = <100>;
> > +     };
> > +};
> > +
> >  &esdhc {
> >       sd-uhs-sdr104;
> >       sd-uhs-sdr50;

Re: [PATCH 4/6] spi: spi-fsl-dspi: Add support for LS1028A

[Vladimir Oltean]

On Mon, 9 Mar 2020 at 20:38, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>
> Am 2020-03-09 15:56, schrieb Vladimir Oltean:
> > From: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.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-3arQi8VN3Tc@public.gmane.org>
> > ---
> >  drivers/spi/spi-fsl-dspi.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > index 5624b9ee77db..264d184e7296 100644
> > --- a/drivers/spi/spi-fsl-dspi.c
> > +++ b/drivers/spi/spi-fsl-dspi.c
> > @@ -131,6 +131,7 @@ struct fsl_dspi_devtype_data {
> >  enum {
> >       LS1021A,
> >       LS1012A,
> > +     LS1028A,
> >       LS1043A,
> >       LS1046A,
> >       LS2080A,
> > @@ -163,6 +164,14 @@ static const struct fsl_dspi_devtype_data
> > devtype_data[] = {
> >               .pushr_cmd              = 0,
> >               .pushr_tx               = 2,
> >       },
> > +     [LS1028A] = {
> > +             .trans_mode             = DSPI_DMA_MODE,
>
> shouldn't this be DSPI_XSPI_MODE according to your cover letter?
>
> -michael
>

Yes, sorry, I forgot to change it back after testing it both ways.

> > +             .dma_bufsize            = 8,
> > +             .max_clock_factor       = 8,
> > +             .fifo_size              = 4,
> > +             .pushr_cmd              = 2,
> > +             .pushr_tx               = 0,
> > +     },
> >       [LS1043A] = {
> >               /* Has A-011218 DMA erratum */
> >               .trans_mode             = DSPI_XSPI_MODE,
> > @@ -1113,6 +1122,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],

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

[Michael Walle]

Am 2020-03-09 19:50, schrieb Vladimir Oltean:
> On Mon, 9 Mar 2020 at 20:35, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>> 
>> Am 2020-03-09 15:56, schrieb 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>
>> > ---
>> >  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..43f403b30dae 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 = "spidev";
>> 
>> As far as I know this throws a warning at boot that you
>> shouldn't use the compatible = "spidev", doesn't it?
>> 
>> /*
>>   * spidev should never be referenced in DT without a specific
>>   * compatible string, it is a Linux implementation thing
>>   * rather than a description of the hardware.
>>   */
>> 
> 
> If this is supposed to mean that the "spidev" string is less
> adequate/expressive than "rohm,dh2228fv", then ok, I'll use that.

TBH I don't care, its your board ;) But I suppose that there is no
Rohm DH2228FV on the (pluggable) mikroBUS board. I just noticed
that compatible string. Don't shoot the messenger ;)

-michael

> 
>> -michael
>> 
>> > +             reg = <0>;
>> > +             spi-max-frequency = <20000000>;
>> > +             fsl,spi-cs-sck-delay = <100>;
>> > +             fsl,spi-sck-cs-delay = <100>;
>> > +     };
>> > +};
>> > +
>> >  &esdhc {
>> >       sd-uhs-sdr104;
>> >       sd-uhs-sdr50;

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

[Michael Walle]

Am 2020-03-09 19:48, schrieb Vladimir Oltean:
> On Mon, 9 Mar 2020 at 20:31, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>> 
>> Am 2020-03-09 19:14, schrieb Vladimir Oltean:
>> > On Mon, 9 Mar 2020 at 20:03, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>> >> Am 2020-03-09 15:56, 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/
>> >>
>> >> If it is not compatible with the LS1021A the second compatible string
>> >> should be removed. Depending on the other remark about the endianess,
>> >> it might still be compatible, though.
>> >>
>> >
>> > Please feel free to remove it. I wasn't actually planning to add it in
>> > the first place, but now it that it's there it doesn't really bother
>> > anybody either.
>> 
>> But it won't work if the endianess depends on the compatible string ;)
>> 
> 
> True.

So another reason to not have the endianess (read the CMD/TX register 
offset)
depends on the compatible string. Because then it should work also with 
the
ls1021a version, correct?

-michael


> 
>> >>
>> >> > 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.
>> >>
>> >> wouldn't it make more sense, to use DMA is the dma node is present
>> >> in the device tree? otherwise use XSPI mode? I don't think it is
>> >> really handy to change the mode inside the driver.
>> >>
>> >
>> > Let's keep it simple. The driver should configure the hardware in the
>> > most efficient and least buggy mode available. Right now that is XSPI.
>> > The hardware description (aka the device tree) is a separate topic. If
>> > there ever arises any situation where there are corner cases with XSPI
>> > mode, it's good to have a fallback in the form of DMA mode, and not
>> > have to worry about yet another problem (which is that there are 2
>> > sets of device tree blobs in deployment).
>> 
>> Point taken. But this is not how other drivers behave, which uses the
>> DMA if its given in the device node.
>> 
> 
> Also true.
> 
>> Btw. do other SoCs perform better with DMA?
>> 
> 
> Not that I know of.
> My general rule of thumb for this controller is: if it supports XSPI
> then use that, otherwise use DMA. Luckily there is just one controller
> that supports none of those, and that would be Coldfire, which uses
> the braindead EOQ mode. I don't have the hardware to do testing on
> that, but in principle if I did, I would have converted that as well
> to the more functional but less efficient TCFQ mode (now removed).
> 
>> -michael
>> 
>> > TL;DR: These DMA channels don't really bother anybody but you never
>> > know when they might come in handy.
>> >
>> >> -michael
>> >>
>> >> >
>> >> > For testing, benchmarking and debugging, the mikroBUS connector on the
>> >> > LS1028A-RDB is made available via spidev.
>> >> >
>> >> > Vladimir Oltean (6):
>> >> >   spi: spi-fsl-dspi: Don't access reserved fields in SPI_MCR
>> >> >   spi: spi-fsl-dspi: Fix little endian access to PUSHR CMD and TXDATA
>> >> >   spi: spi-fsl-dspi: Fix oper_word_size of zero for 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                    | 54 +++++++++++++++----
>> >> >  3 files changed, 64 insertions(+), 10 deletions(-)
>> >
>> > Thanks,
>> > -Vladimir
> 
> -Vladimir

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

[Michael Walle]

Am 2020-03-09 15:56, schrieb Vladimir Oltean:
> From: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
> 
> 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-3arQi8VN3Tc@public.gmane.org>
> ---
>  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";

minor nit. Other nodes specified the dma channels as

dma-names = "tx", "rx";
dmas = <&edma0 0 62>,
        <&edma0 0 60>;

-michael

>  			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";

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

[Vladimir Oltean]

On Mon, 9 Mar 2020 at 21:06, Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
>
> Am 2020-03-09 15:56, schrieb Vladimir Oltean:
> > From: Vladimir Oltean <vladimir.oltean-3arQi8VN3Tc@public.gmane.org>
> >
> > 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-3arQi8VN3Tc@public.gmane.org>
> > ---
> >  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";
>
> minor nit. Other nodes specified the dma channels as
>
> dma-names = "tx", "rx";
> dmas = <&edma0 0 62>,
>         <&edma0 0 60>;
>
> -michael
>

Does it matter?

Regards,
-Vladimir

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

[Michael Walle]

Am 2020-03-09 20:59, schrieb Vladimir Oltean:
> On Mon, 9 Mar 2020 at 21:06, Michael Walle <michael@walle.cc> wrote:
>> 
>> Am 2020-03-09 15:56, schrieb 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>
>> > ---
>> >  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";
>> 
>> minor nit. Other nodes specified the dma channels as
>> 
>> dma-names = "tx", "rx";
>> dmas = <&edma0 0 62>,
>>         <&edma0 0 60>;
>> 
>> -michael
>> 
> 
> Does it matter?

No, therefore "minor nit". Its just formatted other then everything else 
in the file.

-michael