[PATCH spi-next 00/11] spi: spi-fsl-lpspi: various cleanup and enhancement patches

[PATCH spi-next 00/11] spi: spi-fsl-lpspi: various cleanup and enhancement patches

[Marc Kleine-Budde]

While optimizing the spi-fsl-lpspi driver, I created some cleanup and
enhacement patches.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Marc Kleine-Budde (11):
      spi: spi-fsl-lpspi: adapt to kernel coding style
      spi: spi-fsl-lpspi: fsl_lpspi_probe(): use FIELD_GET to decode Parameter register
      spi: spi-fsl-lpspi: fsl_lpspi_set_cmd(): use FIELD_PREP to encode Transmit Command register
      spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register
      spi: spi-fsl-lpspi: fsl_lpspi_setup_transfer(): remove useless spi_transfer NULL pointer check
      spi: spi-fsl-lpspi: fsl_lpspi_can_dma(): directly assign return value to fsl_lpspi->usedma
      spi: spi-fsl-lpspi: fsl_lpspi_reset(): convert to void
      spi: spi-fsl-lpspi: fsl_lpspi_write_tx_fifo(): simplify while() loop check
      spi: spi-fsl-lpspi: make struct lpspi_config::mode u32
      spi: spi-fsl-lpspi: fsl_lpspi_set_cmd(): use mode from (struct fsl_lpspi_data::config::mode
      spi: spi-fsl-lpspi: fsl_lpspi_set_bitrate(): don't treat devtype_data->prescale_max == 0 as a special case

 drivers/spi/spi-fsl-lpspi.c | 91 ++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 46 deletions(-)
---
base-commit: 09be2729dfe984c305b402c13292dc097882295a
change-id: 20260314-spi-fsl-lpspi-cleanups-4014f0e6b29d

Best regards,
--  
Marc Kleine-Budde <mkl@pengutronix.de>

[PATCH spi-next 01/11] spi: spi-fsl-lpspi: adapt to kernel coding style

[Marc Kleine-Budde]

Adapt the driver to current kernel coding style standards.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index b361c1bb3e43..989d0ffddc25 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -112,8 +112,8 @@ struct fsl_lpspi_data {
 
 	void *rx_buf;
 	const void *tx_buf;
-	void (*tx)(struct fsl_lpspi_data *);
-	void (*rx)(struct fsl_lpspi_data *);
+	void (*tx)(struct fsl_lpspi_data *fsl_lpspi);
+	void (*rx)(struct fsl_lpspi_data *fsl_lpspi);
 
 	u32 remain;
 	u8 watermark;
@@ -271,8 +271,9 @@ static void fsl_lpspi_write_tx_fifo(struct fsl_lpspi_data *fsl_lpspi)
 		}
 
 		fsl_lpspi_intctrl(fsl_lpspi, IER_FCIE);
-	} else
+	} else {
 		fsl_lpspi_intctrl(fsl_lpspi, IER_TDIE);
+	}
 }
 
 static void fsl_lpspi_read_rx_fifo(struct fsl_lpspi_data *fsl_lpspi)
@@ -348,11 +349,10 @@ static int fsl_lpspi_set_bitrate(struct fsl_lpspi_data *fsl_lpspi)
 		return -EINVAL;
 	}
 
-	if (config.speed_hz > perclk_rate / 2) {
+	if (config.speed_hz > perclk_rate / 2)
 		div = 2;
-	} else {
+	else
 		div = DIV_ROUND_UP(perclk_rate, config.speed_hz);
-	}
 
 	for (prescale = 0; prescale <= prescale_max; prescale++) {
 		scldiv = div / (1 << prescale) - 2;

-- 
2.51.0

[PATCH spi-next 02/11] spi: spi-fsl-lpspi: fsl_lpspi_probe(): use FIELD_GET to decode Parameter register

[Marc Kleine-Budde]

Instead of open coding mask and shift operations and to o increase
readability use FIELD_GET() to decode the Parameter register.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 989d0ffddc25..4c550f740d85 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -55,6 +55,9 @@
 #define IMX7ULP_RDR	0x74
 
 /* General control register field define */
+#define PARAM_PCSNUM	GENMASK(23, 16)
+#define PARAM_RXFIFO	GENMASK(15, 8)
+#define PARAM_TXFIFO	GENMASK(7, 0)
 #define CR_RRF		BIT(9)
 #define CR_RTF		BIT(8)
 #define CR_RST		BIT(1)
@@ -978,12 +981,12 @@ static int fsl_lpspi_probe(struct platform_device *pdev)
 	}
 
 	temp = readl(fsl_lpspi->base + IMX7ULP_PARAM);
-	fsl_lpspi->txfifosize = 1 << (temp & 0x0f);
-	fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f);
+	fsl_lpspi->txfifosize = 1 << FIELD_GET(PARAM_TXFIFO, temp);
+	fsl_lpspi->rxfifosize = 1 << FIELD_GET(PARAM_RXFIFO, temp);
 	if (of_property_read_u32((&pdev->dev)->of_node, "num-cs",
 				 &num_cs)) {
 		if (devtype_data->query_hw_for_num_cs)
-			num_cs = ((temp >> 16) & 0xf);
+			num_cs = FIELD_GET(PARAM_PCSNUM, temp);
 		else
 			num_cs = 1;
 	}

-- 
2.51.0

[PATCH spi-next 03/11] spi: spi-fsl-lpspi: fsl_lpspi_set_cmd(): use FIELD_PREP to encode Transmit Command register

[Marc Kleine-Budde]

Instead of open coding mask and shift operations and to increase
readability use FIELD_GET() to decode the Transmit Command register.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 4c550f740d85..e75b9acaa9e5 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -82,10 +82,14 @@
 #define RSR_RXEMPTY	BIT(1)
 #define TCR_CPOL	BIT(31)
 #define TCR_CPHA	BIT(30)
+#define TCR_MODE	GENMASK(31, 30)
+#define TCR_PRESCALE	GENMASK(29, 27)
+#define TCR_PCS		GENMASK(25, 24)
 #define TCR_CONT	BIT(21)
 #define TCR_CONTC	BIT(20)
 #define TCR_RXMSK	BIT(19)
 #define TCR_TXMSK	BIT(18)
+#define TCR_FRAMESZ	GENMASK(11, 0)
 
 #define SR_CLEAR_MASK	GENMASK(13, 8)
 
@@ -290,11 +294,11 @@ static void fsl_lpspi_set_cmd(struct fsl_lpspi_data *fsl_lpspi,
 {
 	u32 temp = 0;
 
-	temp |= fsl_lpspi->config.bpw - 1;
-	temp |= (fsl_lpspi->config.mode & 0x3) << 30;
-	temp |= (fsl_lpspi->config.chip_select & 0x3) << 24;
+	temp |= FIELD_PREP(TCR_FRAMESZ, fsl_lpspi->config.bpw - 1);
+	temp |= FIELD_PREP(TCR_MODE, fsl_lpspi->config.mode);
+	temp |= FIELD_PREP(TCR_PCS, fsl_lpspi->config.chip_select);
 	if (!fsl_lpspi->is_target) {
-		temp |= fsl_lpspi->config.prescale << 27;
+		temp |= FIELD_PREP(TCR_PRESCALE, fsl_lpspi->config.prescale);
 		/*
 		 * Set TCR_CONT will keep SS asserted after current transfer.
 		 * For the first transfer, clear TCR_CONTC to assert SS.

-- 
2.51.0

[PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Marc Kleine-Budde]

Instead of open coding mask and shift operations and to increase
readability use FIELD_PREP() to encode the FIFO Status register.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index e75b9acaa9e5..139d6573a013 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -78,6 +78,8 @@
 #define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
 #define CFGR1_NOSTALL	BIT(3)
 #define CFGR1_HOST	BIT(0)
+#define FCR_RXWATER	GENMASK(18, 16)
+#define FCR_TXWATER	GENMASK(2, 0)
 #define FSR_TXCOUNT	(0xFF)
 #define RSR_RXEMPTY	BIT(1)
 #define TCR_CPOL	BIT(31)
@@ -326,17 +328,18 @@ static void fsl_lpspi_set_cmd(struct fsl_lpspi_data *fsl_lpspi,
 
 static void fsl_lpspi_set_watermark(struct fsl_lpspi_data *fsl_lpspi)
 {
+	u8 watermark = fsl_lpspi->watermark >> 1;
 	u32 temp;
 
 	if (!fsl_lpspi->usedma)
-		temp = fsl_lpspi->watermark >> 1 |
-		       (fsl_lpspi->watermark >> 1) << 16;
+		temp = FIELD_PREP(FCR_TXWATER, watermark) |
+			FIELD_PREP(FCR_RXWATER, watermark);
 	else
-		temp = fsl_lpspi->watermark >> 1;
+		temp = FIELD_PREP(FCR_TXWATER, watermark);
 
 	writel(temp, fsl_lpspi->base + IMX7ULP_FCR);
 
-	dev_dbg(fsl_lpspi->dev, "FCR=0x%x\n", temp);
+	dev_dbg(fsl_lpspi->dev, "FCR=0x%08x\n", temp);
 }
 
 static int fsl_lpspi_set_bitrate(struct fsl_lpspi_data *fsl_lpspi)

-- 
2.51.0

[PATCH spi-next 05/11] spi: spi-fsl-lpspi: fsl_lpspi_setup_transfer(): remove useless spi_transfer NULL pointer check

[Marc Kleine-Budde]

fsl_lpspi_setup_transfer() is either called via:

| fsl_lpspi_prepare_message()
| -> fsl_lpspi_setup_transfer()

or

| -> spi_transfer_one_message()
| -> controller->transfer_one == fsl_lpspi_transfer_one()
| -> fsl_lpspi_setup_transfer()

The first call path already has a spi_transfer NULL pointer check, the
second one explicitly iterates over all spi_transfer of the spi_message.

Simplify the code by removing the useless NULL pointer check.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 139d6573a013..1da2084f582d 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -476,9 +476,6 @@ static int fsl_lpspi_setup_transfer(struct spi_controller *controller,
 	struct fsl_lpspi_data *fsl_lpspi =
 				spi_controller_get_devdata(spi->controller);
 
-	if (t == NULL)
-		return -EINVAL;
-
 	fsl_lpspi->config.mode = spi->mode;
 	fsl_lpspi->config.bpw = t->bits_per_word;
 	fsl_lpspi->config.speed_hz = t->speed_hz;

-- 
2.51.0

[PATCH spi-next 06/11] spi: spi-fsl-lpspi: fsl_lpspi_can_dma(): directly assign return value to fsl_lpspi->usedma

[Marc Kleine-Budde]

The function fsl_lpspi_can_dma() returns a bool.

Simplify the code, remove the if/else and assign the return value of
fsl_lpspi_can_dma() directly to fsl_lpspi->usedma.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 1da2084f582d..76dda2c7aa85 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -524,10 +524,7 @@ static int fsl_lpspi_prepare_message(struct spi_controller *controller,
 	fsl_lpspi->usedma = false;
 	ret = fsl_lpspi_setup_transfer(controller, spi, t);
 
-	if (fsl_lpspi_can_dma(controller, spi, t))
-		fsl_lpspi->usedma = true;
-	else
-		fsl_lpspi->usedma = false;
+	fsl_lpspi->usedma = fsl_lpspi_can_dma(controller, spi, t);
 
 	if (ret < 0)
 		return ret;
@@ -801,10 +798,7 @@ static int fsl_lpspi_transfer_one(struct spi_controller *controller,
 					spi_controller_get_devdata(controller);
 	int ret;
 
-	if (fsl_lpspi_can_dma(controller, spi, t))
-		fsl_lpspi->usedma = true;
-	else
-		fsl_lpspi->usedma = false;
+	fsl_lpspi->usedma = fsl_lpspi_can_dma(controller, spi, t);
 
 	ret = fsl_lpspi_setup_transfer(controller, spi, t);
 	if (ret < 0)

-- 
2.51.0

[PATCH spi-next 07/11] spi: spi-fsl-lpspi: fsl_lpspi_reset(): convert to void

[Marc Kleine-Budde]

The function fsl_lpspi_reset() cannot fail and it's return value is never
checked.

Simplify the code and convert it into a bool returning function.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 76dda2c7aa85..920748795314 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -578,7 +578,7 @@ static int fsl_lpspi_wait_for_completion(struct spi_controller *controller)
 	return 0;
 }
 
-static int fsl_lpspi_reset(struct fsl_lpspi_data *fsl_lpspi)
+static void fsl_lpspi_reset(struct fsl_lpspi_data *fsl_lpspi)
 {
 	u32 temp;
 
@@ -593,8 +593,6 @@ static int fsl_lpspi_reset(struct fsl_lpspi_data *fsl_lpspi)
 
 	/* W1C for all flags in SR */
 	writel(SR_CLEAR_MASK, fsl_lpspi->base + IMX7ULP_SR);
-
-	return 0;
 }
 
 static void fsl_lpspi_dma_rx_callback(void *cookie)

-- 
2.51.0

[PATCH spi-next 08/11] spi: spi-fsl-lpspi: fsl_lpspi_write_tx_fifo(): simplify while() loop check

[Marc Kleine-Budde]

To simplify the loop check. Combine both conditions of the while() and the
directly following if() into the while().

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 920748795314..ae06bc06f626 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -265,9 +265,7 @@ static void fsl_lpspi_write_tx_fifo(struct fsl_lpspi_data *fsl_lpspi)
 
 	txfifo_cnt = readl(fsl_lpspi->base + IMX7ULP_FSR) & 0xff;
 
-	while (txfifo_cnt < fsl_lpspi->txfifosize) {
-		if (!fsl_lpspi->remain)
-			break;
+	while (txfifo_cnt < fsl_lpspi->txfifosize && fsl_lpspi->remain) {
 		fsl_lpspi->tx(fsl_lpspi);
 		txfifo_cnt++;
 	}

-- 
2.51.0

[PATCH spi-next 09/11] spi: spi-fsl-lpspi: make struct lpspi_config::mode u32

[Marc Kleine-Budde]

The struct lpspi_config::mode holds a copy of the mode of struct
spi_device::mode. In commit 937e6d756422 ("spi: expand mode support") the
struct spi_device::mode was increased from u16 to u32.

Increase the struct lpspi_config::mode to u32 avoid truncating the mode
variable.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index ae06bc06f626..9ec5554b4fc0 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -104,7 +104,7 @@ struct lpspi_config {
 	u8 bpw;
 	u8 chip_select;
 	u8 prescale;
-	u16 mode;
+	u32 mode;
 	u32 speed_hz;
 	u32 effective_speed_hz;
 };

-- 
2.51.0

[PATCH spi-next 10/11] spi: spi-fsl-lpspi: fsl_lpspi_set_cmd(): use mode from (struct fsl_lpspi_data::config::mode

[Marc Kleine-Budde]

Commit 7ae4d097b752 ("spi: spi-fsl-lpspi: Handle clock polarity and phase")
enhances the driver with clock polarity and phase handling. That commit
adds a 2nd argument ("struct spi_device *spi") to the fsl_lpspi_set_cmd()
function.

The "spi" pointer is used to access the "mode" of the current transfer.
However the mode is already available via "fsl_lpspi->config.mode".

To simplify the driver remove the 2nd argument and use
"fsl_lpspi->config.mode" to access the mode.

Cc: Marek Vasut <marex@nabladev.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 9ec5554b4fc0..3a0e9726fa71 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -289,8 +289,7 @@ static void fsl_lpspi_read_rx_fifo(struct fsl_lpspi_data *fsl_lpspi)
 		fsl_lpspi->rx(fsl_lpspi);
 }
 
-static void fsl_lpspi_set_cmd(struct fsl_lpspi_data *fsl_lpspi,
-			      struct spi_device *spi)
+static void fsl_lpspi_set_cmd(struct fsl_lpspi_data *fsl_lpspi)
 {
 	u32 temp = 0;
 
@@ -313,10 +312,10 @@ static void fsl_lpspi_set_cmd(struct fsl_lpspi_data *fsl_lpspi,
 		}
 	}
 
-	if (spi->mode & SPI_CPOL)
+	if (fsl_lpspi->config.mode & SPI_CPOL)
 		temp |= TCR_CPOL;
 
-	if (spi->mode & SPI_CPHA)
+	if (fsl_lpspi->config.mode & SPI_CPHA)
 		temp |= TCR_CPHA;
 
 	writel(temp, fsl_lpspi->base + IMX7ULP_TCR);
@@ -527,7 +526,7 @@ static int fsl_lpspi_prepare_message(struct spi_controller *controller,
 	if (ret < 0)
 		return ret;
 
-	fsl_lpspi_set_cmd(fsl_lpspi, spi);
+	fsl_lpspi_set_cmd(fsl_lpspi);
 
 	/* No IRQs */
 	writel(0, fsl_lpspi->base + IMX7ULP_IER);
@@ -802,7 +801,7 @@ static int fsl_lpspi_transfer_one(struct spi_controller *controller,
 
 	t->effective_speed_hz = fsl_lpspi->config.effective_speed_hz;
 
-	fsl_lpspi_set_cmd(fsl_lpspi, spi);
+	fsl_lpspi_set_cmd(fsl_lpspi);
 	fsl_lpspi->is_first_byte = false;
 
 	if (fsl_lpspi->usedma)

-- 
2.51.0

[PATCH spi-next 11/11] spi: spi-fsl-lpspi: fsl_lpspi_set_bitrate(): don't treat devtype_data->prescale_max == 0 as a special case

[Marc Kleine-Budde]

The i.MX93 is affected by erratum ERR051608, the maximum prescaler value is
1 not 7 as in the original datasheet.

On unaffected SoC the maximum prescaler is 7. Commit 783bf5d09f86 ("spi:
spi-fsl-lpspi: limit PRESCALE bit in TCR register") added struct
fsl_lpspi_devtype_data to hold the system dependent prescale_max value.

Commit 9bbfb1ec959c ("spi: spi-fsl-lpspi: Treat prescale_max == 0 as no
erratum") introduced the special value 0 to signal the default and
corresponding run-time check with a conditional operator.

To simplify the code, set the prescale_max of imx7ulp_lpspi_devtype_data
and s32g_lpspi_devtype_data to 7 and in fsl_lpspi_set_bitrate() directly
use the fsl_lpspi->devtype_data->prescale_max.

On ARM64 this leads to a reduction of the code of 52 bytes:

| add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-52 (-52)
| Function                                     old     new   delta
| fsl_lpspi_setup_transfer.isra                956     936     -20
| $x                                          7144    7112     -32
| Total: Before=19675, After=19623, chg -0.26%

This partly reverts commit 9bbfb1ec959ce95f91cfab544f705e5257be3be1.

Cc: James Clark <james.clark@linaro.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-fsl-lpspi.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 3a0e9726fa71..f2c7bb3bc4cc 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -96,7 +96,7 @@
 #define SR_CLEAR_MASK	GENMASK(13, 8)
 
 struct fsl_lpspi_devtype_data {
-	u8 prescale_max : 3; /* 0 == no limit */
+	u8 prescale_max : 3;
 	bool query_hw_for_num_cs : 1;
 };
 
@@ -143,8 +143,10 @@ struct fsl_lpspi_data {
 };
 
 /*
- * Devices with ERR051608 have a max TCR_PRESCALE value of 1, otherwise there is
- * no prescale limit: https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
+ * Devices with ERR051608 have a max TCR_PRESCALE value of 1, otherwise use
+ * the register limit of 7.
+ *
+ * https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
  */
 static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
 	.prescale_max = 1,
@@ -152,10 +154,11 @@ static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
 };
 
 static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
-	/* All defaults */
+	.prescale_max = 7,
 };
 
 static const struct fsl_lpspi_devtype_data s32g_lpspi_devtype_data = {
+	.prescale_max = 7,
 	.query_hw_for_num_cs = true,
 };
 
@@ -348,7 +351,7 @@ static int fsl_lpspi_set_bitrate(struct fsl_lpspi_data *fsl_lpspi)
 	int scldiv;
 
 	perclk_rate = clk_get_rate(fsl_lpspi->clk_per);
-	prescale_max = fsl_lpspi->devtype_data->prescale_max ?: 7;
+	prescale_max = fsl_lpspi->devtype_data->prescale_max;
 
 	if (!config.speed_hz) {
 		dev_err(fsl_lpspi->dev,

-- 
2.51.0

Re: [PATCH spi-next 11/11] spi: spi-fsl-lpspi: fsl_lpspi_set_bitrate(): don't treat devtype_data->prescale_max == 0 as a special case

[James Clark]

On 16/03/2026 8:39 am, Marc Kleine-Budde wrote:
> The i.MX93 is affected by erratum ERR051608, the maximum prescaler value is
> 1 not 7 as in the original datasheet.
> 
> On unaffected SoC the maximum prescaler is 7. Commit 783bf5d09f86 ("spi:
> spi-fsl-lpspi: limit PRESCALE bit in TCR register") added struct
> fsl_lpspi_devtype_data to hold the system dependent prescale_max value.
> 
> Commit 9bbfb1ec959c ("spi: spi-fsl-lpspi: Treat prescale_max == 0 as no
> erratum") introduced the special value 0 to signal the default and
> corresponding run-time check with a conditional operator.
> 
> To simplify the code, set the prescale_max of imx7ulp_lpspi_devtype_data

The whole point of the original change was to not repeat the normal 
value for every device in the config just because one device has an 
errata. This new change is exactly the opposite of simplification IMO.

> and s32g_lpspi_devtype_data to 7 and in fsl_lpspi_set_bitrate() directly
> use the fsl_lpspi->devtype_data->prescale_max.
> 
> On ARM64 this leads to a reduction of the code of 52 bytes:
> 
> | add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-52 (-52)
> | Function                                     old     new   delta
> | fsl_lpspi_setup_transfer.isra                956     936     -20
> | $x                                          7144    7112     -32

On clang 15 this change makes it bigger by 220 bytes rather than 
smaller. I checked because I was suspicious that removing a single 
ternary could save 52 bytes, but it looks like the outcome after all the 
optimisations is just unpredictable so there isn't much point in making 
changes like this.

You also didn't say _why_ we need to save 50 bytes, or what the 
performance impact is.

> | Total: Before=19675, After=19623, chg -0.26%
> 
> This partly reverts commit 9bbfb1ec959ce95f91cfab544f705e5257be3be1.
> 
> Cc: James Clark <james.clark@linaro.org>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>   drivers/spi/spi-fsl-lpspi.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index 3a0e9726fa71..f2c7bb3bc4cc 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -96,7 +96,7 @@
>   #define SR_CLEAR_MASK	GENMASK(13, 8)
>   
>   struct fsl_lpspi_devtype_data {
> -	u8 prescale_max : 3; /* 0 == no limit */
> +	u8 prescale_max : 3;
>   	bool query_hw_for_num_cs : 1;
>   };
>   
> @@ -143,8 +143,10 @@ struct fsl_lpspi_data {
>   };
>   
>   /*
> - * Devices with ERR051608 have a max TCR_PRESCALE value of 1, otherwise there is
> - * no prescale limit: https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
> + * Devices with ERR051608 have a max TCR_PRESCALE value of 1, otherwise use
> + * the register limit of 7.
> + *
> + * https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
>    */
>   static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
>   	.prescale_max = 1,
> @@ -152,10 +154,11 @@ static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
>   };
>   
>   static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
> -	/* All defaults */
> +	.prescale_max = 7,
>   };
>   
>   static const struct fsl_lpspi_devtype_data s32g_lpspi_devtype_data = {
> +	.prescale_max = 7,
>   	.query_hw_for_num_cs = true,
>   };
>   
> @@ -348,7 +351,7 @@ static int fsl_lpspi_set_bitrate(struct fsl_lpspi_data *fsl_lpspi)
>   	int scldiv;
>   
>   	perclk_rate = clk_get_rate(fsl_lpspi->clk_per);
> -	prescale_max = fsl_lpspi->devtype_data->prescale_max ?: 7;
> +	prescale_max = fsl_lpspi->devtype_data->prescale_max;
>   
>   	if (!config.speed_hz) {
>   		dev_err(fsl_lpspi->dev,
> 

Re: [PATCH spi-next 11/11] spi: spi-fsl-lpspi: fsl_lpspi_set_bitrate(): don't treat devtype_data->prescale_max == 0 as a special case

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3206 bytes --]

Hello James,

I've no strong feelings about this patch, I can drop it in the v2.

On 16.03.2026 09:57:15, James Clark wrote:
> On 16/03/2026 8:39 am, Marc Kleine-Budde wrote:
> > The i.MX93 is affected by erratum ERR051608, the maximum prescaler value is
> > 1 not 7 as in the original datasheet.
> >
> > On unaffected SoC the maximum prescaler is 7. Commit 783bf5d09f86 ("spi:
> > spi-fsl-lpspi: limit PRESCALE bit in TCR register") added struct
> > fsl_lpspi_devtype_data to hold the system dependent prescale_max value.
> >
> > Commit 9bbfb1ec959c ("spi: spi-fsl-lpspi: Treat prescale_max == 0 as no
> > erratum") introduced the special value 0 to signal the default and
> > corresponding run-time check with a conditional operator.
> >
> > To simplify the code, set the prescale_max of imx7ulp_lpspi_devtype_data
>
> The whole point of the original change was to not repeat the normal value
> for every device in the config just because one device has an errata. This
> new change is exactly the opposite of simplification IMO.

Interesting point of view.

My reasoning is: When I look at the fsl_lpspi_set_bitrate() function,
there's the special case handling with the ternary for the "0".

I asked myself directly special case, why? Why not directly use the
value as it's provided by the devtype_data?

> > and s32g_lpspi_devtype_data to 7 and in fsl_lpspi_set_bitrate() directly
> > use the fsl_lpspi->devtype_data->prescale_max.
> >
> > On ARM64 this leads to a reduction of the code of 52 bytes:
> >
> > | add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-52 (-52)
> > | Function                                     old     new   delta
> > | fsl_lpspi_setup_transfer.isra                956     936     -20
> > | $x                                          7144    7112     -32

> On clang 15 this change makes it bigger by 220 bytes rather than
> smaller.

Quite interesting, with llvm-21 the size increases, too:

| add/remove: 0/0 grow/shrink: 3/1 up/down: 432/-32 (400)
| Function                                     old     new   delta
| fsl_lpspi_setup_transfer                    1072    1216    +144
| .Ltmp1                                      7116    7260    +144
| $x                                          7196    7340    +144
| $d                                          5659    5627     -32
| Total: Before=28126, After=28526, chg +1.42%

> I
> checked because I was suspicious that removing a single ternary could save
> 52 bytes, but it looks like the outcome after all the optimisations is just
> unpredictable so there isn't much point in making changes like this.
>
> You also didn't say _why_ we need to save 50 bytes, or what the performance
> impact is.

I just checked the impact on the code size with gcc and was positively
surprised. :) I don't need to save 50 bytes and there's probably no
measurable performance impact.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH spi-next 10/11] spi: spi-fsl-lpspi: fsl_lpspi_set_cmd(): use mode from (struct fsl_lpspi_data::config::mode

[Marek Vasut]

On 3/16/26 9:39 AM, Marc Kleine-Budde wrote:
> Commit 7ae4d097b752 ("spi: spi-fsl-lpspi: Handle clock polarity and phase")
> enhances the driver with clock polarity and phase handling. That commit
> adds a 2nd argument ("struct spi_device *spi") to the fsl_lpspi_set_cmd()
> function.
> 
> The "spi" pointer is used to access the "mode" of the current transfer.
> However the mode is already available via "fsl_lpspi->config.mode".
> 
> To simplify the driver remove the 2nd argument and use
> "fsl_lpspi->config.mode" to access the mode.

Reviewed-by: Marek Vasut <marex@nabladev.com>

Thanks

Re: [PATCH spi-next 11/11] spi: spi-fsl-lpspi: fsl_lpspi_set_bitrate(): don't treat devtype_data->prescale_max == 0 as a special case

[James Clark]

On 16/03/2026 11:19 am, Marc Kleine-Budde wrote:
> Hello James,
> 
> I've no strong feelings about this patch, I can drop it in the v2.
> 
> On 16.03.2026 09:57:15, James Clark wrote:
>> On 16/03/2026 8:39 am, Marc Kleine-Budde wrote:
>>> The i.MX93 is affected by erratum ERR051608, the maximum prescaler value is
>>> 1 not 7 as in the original datasheet.
>>>
>>> On unaffected SoC the maximum prescaler is 7. Commit 783bf5d09f86 ("spi:
>>> spi-fsl-lpspi: limit PRESCALE bit in TCR register") added struct
>>> fsl_lpspi_devtype_data to hold the system dependent prescale_max value.
>>>
>>> Commit 9bbfb1ec959c ("spi: spi-fsl-lpspi: Treat prescale_max == 0 as no
>>> erratum") introduced the special value 0 to signal the default and
>>> corresponding run-time check with a conditional operator.
>>>
>>> To simplify the code, set the prescale_max of imx7ulp_lpspi_devtype_data
>>
>> The whole point of the original change was to not repeat the normal value
>> for every device in the config just because one device has an errata. This
>> new change is exactly the opposite of simplification IMO.
> 
> Interesting point of view.
> 
> My reasoning is: When I look at the fsl_lpspi_set_bitrate() function,
> there's the special case handling with the ternary for the "0".
> 
> I asked myself directly special case, why? Why not directly use the
> value as it's provided by the devtype_data?
> 

Those structs are configuration for different devices though. Having a 7 
in there implies that it's something that is configurable to different 
values. When I was adding a new device that made be crawl through a load 
of datasheets to see why arbitrary values were needed only to find out 
that it's only for one errata where it's 1 and actually every other 
device should be 7.

I tried to change it to a bool "has the errata" or "doesn't have it", as 
that's the extent of the valid configurations that should be exposed. 
But Frank said he preferred a number with 0 as the default. That seemed 
like a slight improvement to me so that's how it ended up like that.

>>> and s32g_lpspi_devtype_data to 7 and in fsl_lpspi_set_bitrate() directly
>>> use the fsl_lpspi->devtype_data->prescale_max.
>>>
>>> On ARM64 this leads to a reduction of the code of 52 bytes:
>>>
>>> | add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-52 (-52)
>>> | Function                                     old     new   delta
>>> | fsl_lpspi_setup_transfer.isra                956     936     -20
>>> | $x                                          7144    7112     -32
> 
>> On clang 15 this change makes it bigger by 220 bytes rather than
>> smaller.
> 
> Quite interesting, with llvm-21 the size increases, too:
> 
> | add/remove: 0/0 grow/shrink: 3/1 up/down: 432/-32 (400)
> | Function                                     old     new   delta
> | fsl_lpspi_setup_transfer                    1072    1216    +144
> | .Ltmp1                                      7116    7260    +144
> | $x                                          7196    7340    +144
> | $d                                          5659    5627     -32
> | Total: Before=28126, After=28526, chg +1.42%
> 
>> I
>> checked because I was suspicious that removing a single ternary could save
>> 52 bytes, but it looks like the outcome after all the optimisations is just
>> unpredictable so there isn't much point in making changes like this.
>>
>> You also didn't say _why_ we need to save 50 bytes, or what the performance
>> impact is.
> 
> I just checked the impact on the code size with gcc and was positively
> surprised. :) I don't need to save 50 bytes and there's probably no
> measurable performance impact.
> 
> regards,
> Marc
> 

Re: [PATCH spi-next 11/11] spi: spi-fsl-lpspi: fsl_lpspi_set_bitrate(): don't treat devtype_data->prescale_max == 0 as a special case

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

On 16.03.2026 12:08:13, James Clark wrote:
> Those structs are configuration for different devices though. Having a 7 in
> there implies that it's something that is configurable to different values.
> When I was adding a new device that made be crawl through a load of
> datasheets to see why arbitrary values were needed only to find out that
> it's only for one errata where it's 1 and actually every other device should
> be 7.
>
> I tried to change it to a bool "has the errata" or "doesn't have it", as
> that's the extent of the valid configurations that should be exposed. But
> Frank said he preferred a number with 0 as the default. That seemed like a
> slight improvement to me so that's how it ended up like that.

Thanks for the insight, I'll drop the patch.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On Mon, Mar 16, 2026 at 09:39:05AM +0100, Marc Kleine-Budde wrote:

> +#define FCR_RXWATER	GENMASK(18, 16)
> +#define FCR_TXWATER	GENMASK(2, 0)

The source of the watermark values appears to be

	temp = readl(fsl_lpspi->base + IMX7ULP_PARAM);
	fsl_lpspi->txfifosize = 1 << (temp & 0x0f);
	fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f);

which suggests 4 bits of watermark not 3?  Or at least that something
isn't joined up somewhere.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On 16.03.2026 14:39:03, Mark Brown wrote:
> On Mon, Mar 16, 2026 at 09:39:05AM +0100, Marc Kleine-Budde wrote:
>
> > +#define FCR_RXWATER	GENMASK(18, 16)
> > +#define FCR_TXWATER	GENMASK(2, 0)

That's the width from the i.MX93 datasheet.

> The source of the watermark values appears to be
>
> 	temp = readl(fsl_lpspi->base + IMX7ULP_PARAM);
> 	fsl_lpspi->txfifosize = 1 << (temp & 0x0f);
> 	fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f);
>
> which suggests 4 bits of watermark not 3?  Or at least that something
> isn't joined up somewhere.

The param register's default value on the i.MX93 is:

| LPSPI1–LPSPI3:  0002_0303h
| LPSPI4:         0003_0303h
| LPSPI5–LPSPI8:  0002_0303h

This means a RX/TX-FIFO size of 1 << 3 == 8.

In the FCR register the RX/TX watermark are 3 bits. So this is
consistent.

On the imx7ulp and s32g2 the param gives 1 << 2 = 4 and the RX/TX
watermark are just 2 bits wide.


We can increase the the FCR_RXWATER and FCR_TXWATER to be 8 bits wide,
the rest of the register is reserved.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On Mon, Mar 16, 2026 at 05:49:48PM +0100, Marc Kleine-Budde wrote:
> On 16.03.2026 14:39:03, Mark Brown wrote:
> > On Mon, Mar 16, 2026 at 09:39:05AM +0100, Marc Kleine-Budde wrote:

> > The source of the watermark values appears to be

> > 	temp = readl(fsl_lpspi->base + IMX7ULP_PARAM);
> > 	fsl_lpspi->txfifosize = 1 << (temp & 0x0f);
> > 	fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f);

> > which suggests 4 bits of watermark not 3?  Or at least that something
> > isn't joined up somewhere.

> The param register's default value on the i.MX93 is:

> | LPSPI1–LPSPI3:  0002_0303h
> | LPSPI4:         0003_0303h
> | LPSPI5–LPSPI8:  0002_0303h

> This means a RX/TX-FIFO size of 1 << 3 == 8.

Right, so the parsing code is using the wrong mask to extract the width
here but with actual values it's fine.

> We can increase the the FCR_RXWATER and FCR_TXWATER to be 8 bits wide,
> the rest of the register is reserved.

It seems safer to restrict the mask used to extract the FIFO sizes, that
way we don't trample over any bits that get defined in future and people
comparing with the datasheet aren't confused.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

On 16.03.2026 17:37:45, Mark Brown wrote:
> On Mon, Mar 16, 2026 at 05:49:48PM +0100, Marc Kleine-Budde wrote:
> > On 16.03.2026 14:39:03, Mark Brown wrote:
> > > On Mon, Mar 16, 2026 at 09:39:05AM +0100, Marc Kleine-Budde wrote:
>
> > > The source of the watermark values appears to be
>
> > > 	temp = readl(fsl_lpspi->base + IMX7ULP_PARAM);
> > > 	fsl_lpspi->txfifosize = 1 << (temp & 0x0f);
> > > 	fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f);
>
> > > which suggests 4 bits of watermark not 3?  Or at least that something
> > > isn't joined up somewhere.
>
> > The param register's default value on the i.MX93 is:
>
> > | LPSPI1–LPSPI3:  0002_0303h
> > | LPSPI4:         0003_0303h
> > | LPSPI5–LPSPI8:  0002_0303h
>
> > This means a RX/TX-FIFO size of 1 << 3 == 8.
>
> Right, so the parsing code is using the wrong mask to extract the width
> here but with actual values it's fine.

No, the parsing code is correct, the width of the fields are 8 bits
(datasheet of all supported SoCs), but the values are "2" (i.MX7ulp,
S32G2) resp. "3" (i.MX93).

> > We can increase the the FCR_RXWATER and FCR_TXWATER to be 8 bits wide,
> > the rest of the register is reserved.
>
> It seems safer to restrict the mask used to extract the FIFO sizes, that
> way we don't trample over any bits that get defined in future and people
> comparing with the datasheet aren't confused.

The width of the watermark fields are "2" (i.MX7ulp, S32G2) resp. "3"
(i.MX93).

In fsl_lpspi_setup_transfer() the driver restricts the watermark to:

| fsl_lpspi->watermark = min(fsl_lpspi->txfifosize, t->len);

and writes

| fsl_lpspi->watermark >> 1

into the watermark fields. So using the 3 bit mask in FIELD_PREP should
not be a problem, even on the IP cores with just 2 bit wide mask.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

On Mon, Mar 16, 2026 at 06:59:12PM +0100, Marc Kleine-Budde wrote:
> On 16.03.2026 17:37:45, Mark Brown wrote:
> > On Mon, Mar 16, 2026 at 05:49:48PM +0100, Marc Kleine-Budde wrote:

> > > The param register's default value on the i.MX93 is:

> > > | LPSPI1–LPSPI3:  0002_0303h
> > > | LPSPI4:         0003_0303h
> > > | LPSPI5–LPSPI8:  0002_0303h

> > > This means a RX/TX-FIFO size of 1 << 3 == 8.

> > Right, so the parsing code is using the wrong mask to extract the width
> > here but with actual values it's fine.

> No, the parsing code is correct, the width of the fields are 8 bits
> (datasheet of all supported SoCs), but the values are "2" (i.MX7ulp,
> S32G2) resp. "3" (i.MX93).

Oh, dear, so that's potentially going to go badly if there's a SoC with
a bigger FIFO that didn't also increase the watermark field width.  We
should at least warn about that, and ideally do something sensible.

> into the watermark fields. So using the 3 bit mask in FIELD_PREP should
> not be a problem, even on the IP cores with just 2 bit wide mask.

Yeah, it's not broken right now - just landmines for potential future
SoCs to trip over.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]

On 16.03.2026 18:16:01, Mark Brown wrote:
> On Mon, Mar 16, 2026 at 06:59:12PM +0100, Marc Kleine-Budde wrote:
> > On 16.03.2026 17:37:45, Mark Brown wrote:
> > > On Mon, Mar 16, 2026 at 05:49:48PM +0100, Marc Kleine-Budde wrote:
>
> > > > The param register's default value on the i.MX93 is:
>
> > > > | LPSPI1–LPSPI3:  0002_0303h
> > > > | LPSPI4:         0003_0303h
> > > > | LPSPI5–LPSPI8:  0002_0303h
>
> > > > This means a RX/TX-FIFO size of 1 << 3 == 8.
>
> > > Right, so the parsing code is using the wrong mask to extract the width
> > > here but with actual values it's fine.
>
> > No, the parsing code is correct, the width of the fields are 8 bits
> > (datasheet of all supported SoCs), but the values are "2" (i.MX7ulp,
> > S32G2) resp. "3" (i.MX93).
>
> Oh, dear, so that's potentially going to go badly if there's a SoC with
> a bigger FIFO that didn't also increase the watermark field width.  We
> should at least warn about that, and ideally do something sensible.

Good idea, I'll add a warning if the FIFO is bigger than the watermark
field.

To handle this situation I see 2 options:
- warning + limit the FIFO to the usable size due to the currently
  hard-coded watermark width - this will result in reduced throughput
- increase the watermark mask to 8 bits - which is basically how the
  unpatched driver works.
  As outlined in my previous mail, the other code in the driver takes
  care that the watermark stays within the limits.

> > into the watermark fields. So using the 3 bit mask in FIELD_PREP should
> > not be a problem, even on the IP cores with just 2 bit wide mask.
>
> Yeah, it's not broken right now - just landmines for potential future
> SoCs to trip over.

ACK

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

On Mon, Mar 16, 2026 at 07:31:04PM +0100, Marc Kleine-Budde wrote:
> On 16.03.2026 18:16:01, Mark Brown wrote:

> > Oh, dear, so that's potentially going to go badly if there's a SoC with
> > a bigger FIFO that didn't also increase the watermark field width.  We
> > should at least warn about that, and ideally do something sensible.

> Good idea, I'll add a warning if the FIFO is bigger than the watermark
> field.

> To handle this situation I see 2 options:
> - warning + limit the FIFO to the usable size due to the currently
>   hard-coded watermark width - this will result in reduced throughput
> - increase the watermark mask to 8 bits - which is basically how the
>   unpatched driver works.
>   As outlined in my previous mail, the other code in the driver takes
>   care that the watermark stays within the limits.

I guess it depends how optimistic we are about hardware engineers
keeping the FIFO and watermark depths in sync.  The first option is more
defensive, but like you say it'll hit performance on future devices that
do expand the FIFO.  I guess I have a slight preference for that?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH spi-next 02/11] spi: spi-fsl-lpspi: fsl_lpspi_probe(): use FIELD_GET to decode Parameter register

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@nxp.com>

Typo in commit message:

> Instead of open coding mask and shift operations and to o increase

AI: Should be "and to increase" (remove the extra "o").

Frank

Re: [PATCH spi-next 02/11] spi: spi-fsl-lpspi: fsl_lpspi_probe(): use FIELD_GET to decode Parameter register

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 549 bytes --]

On 16.03.2026 21:05:45, Frank Li wrote:
> From: Frank Li (AI-BOT) <frank.li@nxp.com>
>
> Typo in commit message:
>
> > Instead of open coding mask and shift operations and to o increase
>
> AI: Should be "and to increase" (remove the extra "o").

Fixed.

Thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]

On 16.03.2026 18:49:15, Mark Brown wrote:
> > > Oh, dear, so that's potentially going to go badly if there's a SoC with
> > > a bigger FIFO that didn't also increase the watermark field width.  We
> > > should at least warn about that, and ideally do something sensible.
>
> > Good idea, I'll add a warning if the FIFO is bigger than the watermark
> > field.
>
> > To handle this situation I see 2 options:
> > - warning + limit the FIFO to the usable size due to the currently
> >   hard-coded watermark width - this will result in reduced throughput
> > - increase the watermark mask to 8 bits - which is basically how the
> >   unpatched driver works.
> >   As outlined in my previous mail, the other code in the driver takes
> >   care that the watermark stays within the limits.
>
> I guess it depends how optimistic we are about hardware engineers
> keeping the FIFO and watermark depths in sync.

Right, we're talking about FSL/NXP hardware :D

> The first option is more
> defensive, but like you say it'll hit performance on future devices that
> do expand the FIFO.  I guess I have a slight preference for that?

Yes - I'll increase the watermark width to 8 bits and add a comment next
to the GENMASK.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH spi-next 04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

On 17.03.2026 10:44:51, Marc Kleine-Budde wrote:
> On 16.03.2026 18:49:15, Mark Brown wrote:
> > > > Oh, dear, so that's potentially going to go badly if there's a SoC with
> > > > a bigger FIFO that didn't also increase the watermark field width.  We
> > > > should at least warn about that, and ideally do something sensible.
> >
> > > Good idea, I'll add a warning if the FIFO is bigger than the watermark
> > > field.
> >
> > > To handle this situation I see 2 options:
> > > - warning + limit the FIFO to the usable size due to the currently
> > >   hard-coded watermark width - this will result in reduced throughput
> > > - increase the watermark mask to 8 bits - which is basically how the
> > >   unpatched driver works.
> > >   As outlined in my previous mail, the other code in the driver takes
> > >   care that the watermark stays within the limits.
> >
> > I guess it depends how optimistic we are about hardware engineers
> > keeping the FIFO and watermark depths in sync.
>
> Right, we're talking about FSL/NXP hardware :D
>
> > The first option is more
> > defensive, but like you say it'll hit performance on future devices that
> > do expand the FIFO.  I guess I have a slight preference for that?
>
> Yes - I'll increase the watermark width to 8 bits and add a comment next
> to the GENMASK.

FTR: I decided to use the watermark width of the i.MX93 as an upper
limit for the FIFO order and print an info message.

See https://lore.kernel.org/all/20260319-spi-fsl-lpspi-cleanups-v2-3-02b56c5d44a8@pengutronix.de

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH spi-next 00/11] spi: spi-fsl-lpspi: various cleanup and enhancement patches

[Mark Brown]

On Mon, 16 Mar 2026 09:39:01 +0100, Marc Kleine-Budde wrote:
> While optimizing the spi-fsl-lpspi driver, I created some cleanup and
> enhacement patches.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[01/11] spi: spi-fsl-lpspi: adapt to kernel coding style
        https://git.kernel.org/broonie/misc/c/8292eded5998
[02/11] spi: spi-fsl-lpspi: fsl_lpspi_probe(): use FIELD_GET to decode Parameter register
        (no commit info)
[03/11] spi: spi-fsl-lpspi: fsl_lpspi_set_cmd(): use FIELD_PREP to encode Transmit Command register
        https://git.kernel.org/broonie/misc/c/c6e178460434
[04/11] spi: spi-fsl-lpspi: fsl_lpspi_set_watermark(): use FIELD_PREP() to encode FIFO Status register
        (no commit info)
[05/11] spi: spi-fsl-lpspi: fsl_lpspi_setup_transfer(): remove useless spi_transfer NULL pointer check
        https://git.kernel.org/broonie/misc/c/ca431d50bf62
[06/11] spi: spi-fsl-lpspi: fsl_lpspi_can_dma(): directly assign return value to fsl_lpspi->usedma
        https://git.kernel.org/broonie/misc/c/e59fe5e0c418
[07/11] spi: spi-fsl-lpspi: fsl_lpspi_reset(): convert to void
        (no commit info)
[08/11] spi: spi-fsl-lpspi: fsl_lpspi_write_tx_fifo(): simplify while() loop check
        https://git.kernel.org/broonie/misc/c/4ef7fa7bca57
[09/11] spi: spi-fsl-lpspi: make struct lpspi_config::mode u32
        https://git.kernel.org/broonie/misc/c/baa1cb259cc7
[10/11] spi: spi-fsl-lpspi: fsl_lpspi_set_cmd(): use mode from (struct fsl_lpspi_data::config::mode
        (no commit info)
[11/11] spi: spi-fsl-lpspi: fsl_lpspi_set_bitrate(): don't treat devtype_data->prescale_max == 0 as a special case
        (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark