[PATCH 00/13] spi: spi-fsl-lpspi: Generic fixes and support for S32G devices

[PATCH 00/13] spi: spi-fsl-lpspi: Generic fixes and support for S32G devices

[James Clark]

Various fixes for LPSI along with some refactorings. None of the fixes
are strictly related to S32G, however these changes all originate from
the work to support S32G devices. The only commits that are strictly
related are for the new s32g2 and s32g3 compatible strings.

Signed-off-by: James Clark <james.clark@linaro.org>
---
To: Frank Li <Frank.Li@nxp.com>
To: Mark Brown <broonie@kernel.org>
To: Clark Wang <xiaoning.wang@nxp.com>
To: Fugang Duan <B38611@freescale.com>
To: Gao Pan <pandy.gao@nxp.com>
To: Fugang Duan <fugang.duan@nxp.com>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: Larisa Grigore <larisa.grigore@oss.nxp.com>
To: Larisa Grigore <larisa.grigore@nxp.com>
To: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
To: Ciprianmarian Costea <ciprianmarian.costea@nxp.com>
To: s32@nxp.com
Cc: linux-spi@vger.kernel.org
Cc: imx@lists.linux.dev
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org

---
James Clark (7):
      spi: spi-fsl-lpspi: Enumerate all pin configuration definitions
      spi: spi-fsl-lpspi: Add DT property to override default pin config
      spi: spi-fsl-lpspi: Constify devtype datas
      spi: spi-fsl-lpspi: Make prescale erratum a bool
      spi: spi-fsl-lpspi: Parameterize reading num-cs from hardware
      dt-bindings: lpspi: Update maximum num-cs value
      dt-bindings: lpspi: Document nxp,lpspi-pincfg property

Larisa Grigore (6):
      spi: spi-fsl-lpspi: Fix transmissions when using CONT
      spi: spi-fsl-lpspi: Set correct chip-select polarity bit
      spi: spi-fsl-lpspi: Reset FIFO and disable module on transfer abort
      spi: spi-fsl-lpspi: Clear status register after disabling the module
      spi: spi-fsl-lpspi: Add compatible for S32G
      dt-bindings: lpspi: Document support for S32G

 .../devicetree/bindings/spi/spi-fsl-lpspi.yaml     | 21 +++++-
 drivers/spi/spi-fsl-lpspi.c                        | 87 +++++++++++++++-------
 2 files changed, 82 insertions(+), 26 deletions(-)
---
base-commit: 0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9
change-id: 20250715-james-nxp-lpspi-1fac58e72a59

Best regards,
-- 
James Clark <james.clark@linaro.org>

[PATCH 01/13] spi: spi-fsl-lpspi: Fix transmissions when using CONT

[James Clark]

From: Larisa Grigore <larisa.grigore@nxp.com>

Commit 6a130448498c ("spi: lpspi: Fix wrong transmission when don't use
CONT") breaks transmissions when CONT is used. The TDIE interrupt should
not be disabled in all cases. If CONT is used and the TX transfer is not
yet completed yet, but the interrupt handler is called because there are
characters to be received, TDIE is replaced with FCIE. When the transfer
is finally completed, SR_TDF is set but the interrupt handler isn't
called again.

Fixes: 6a130448498c ("spi: lpspi: Fix wrong transmission when don't use CONT")
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 67d4000c3cef..d44a23f7d6c1 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -3,7 +3,7 @@
 // Freescale i.MX7ULP LPSPI driver
 //
 // Copyright 2016 Freescale Semiconductor, Inc.
-// Copyright 2018 NXP Semiconductors
+// Copyright 2018, 2023, 2025 NXP
 
 #include <linux/clk.h>
 #include <linux/completion.h>
@@ -787,7 +787,7 @@ static irqreturn_t fsl_lpspi_isr(int irq, void *dev_id)
 	if (temp_SR & SR_MBF ||
 	    readl(fsl_lpspi->base + IMX7ULP_FSR) & FSR_TXCOUNT) {
 		writel(SR_FCF, fsl_lpspi->base + IMX7ULP_SR);
-		fsl_lpspi_intctrl(fsl_lpspi, IER_FCIE);
+		fsl_lpspi_intctrl(fsl_lpspi, IER_FCIE | (temp_IER & IER_TDIE));
 		return IRQ_HANDLED;
 	}
 

-- 
2.34.1

[PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

[James Clark]

From: Larisa Grigore <larisa.grigore@nxp.com>

The driver currently supports multiple chip-selects, but only sets the
polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
the desired chip-select.

Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index d44a23f7d6c1..c65eb6d31ee7 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -70,7 +70,7 @@
 #define DER_TDDE	BIT(0)
 #define CFGR1_PCSCFG	BIT(27)
 #define CFGR1_PINCFG	(BIT(24)|BIT(25))
-#define CFGR1_PCSPOL	BIT(8)
+#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
 #define CFGR1_NOSTALL	BIT(3)
 #define CFGR1_HOST	BIT(0)
 #define FSR_TXCOUNT	(0xFF)
@@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
 	else
 		temp = CFGR1_PINCFG;
 	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
-		temp |= CFGR1_PCSPOL;
+		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
+				   BIT(fsl_lpspi->config.chip_select));
+
 	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
 
 	temp = readl(fsl_lpspi->base + IMX7ULP_CR);

-- 
2.34.1

[PATCH 03/13] spi: spi-fsl-lpspi: Reset FIFO and disable module on transfer abort

[James Clark]

From: Larisa Grigore <larisa.grigore@nxp.com>

In DMA mode fsl_lpspi_reset() is always called at the end, even when the
transfer is aborted. When not using DMA, aborts skip the reset leaving
the FIFO filled and the module enabled.

Fix it by always calling fsl_lpspi_reset().

Fixes: a15dc3d657fa ("spi: lpspi: Fix CLK pin becomes low before one transfer")
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 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 c65eb6d31ee7..aab92ee7eb94 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -734,12 +734,10 @@ static int fsl_lpspi_pio_transfer(struct spi_controller *controller,
 	fsl_lpspi_write_tx_fifo(fsl_lpspi);
 
 	ret = fsl_lpspi_wait_for_completion(controller);
-	if (ret)
-		return ret;
 
 	fsl_lpspi_reset(fsl_lpspi);
 
-	return 0;
+	return ret;
 }
 
 static int fsl_lpspi_transfer_one(struct spi_controller *controller,

-- 
2.34.1

[PATCH 04/13] spi: spi-fsl-lpspi: Clear status register after disabling the module

[James Clark]

From: Larisa Grigore <larisa.grigore@nxp.com>

Clear the error flags after disabling the module to avoid the case when
a flag is set between when the flags were just cleared, and when the
module is disabled.

Although fsl_lpspi_reset() was only introduced in commit a15dc3d657fa
("spi: lpspi: Fix CLK pin becomes low before one transfer"), the
original driver only reset SR in the interrupt handler, making it
vulnerable to the same issue. Therefore the fixes commit is set at the
introduction of the driver.

Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index aab92ee7eb94..79b170426bee 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -82,6 +82,8 @@
 #define TCR_RXMSK	BIT(19)
 #define TCR_TXMSK	BIT(18)
 
+#define SR_CLEAR_MASK	GENMASK(13, 8)
+
 struct fsl_lpspi_devtype_data {
 	u8 prescale_max;
 };
@@ -536,14 +538,13 @@ static int fsl_lpspi_reset(struct fsl_lpspi_data *fsl_lpspi)
 		fsl_lpspi_intctrl(fsl_lpspi, 0);
 	}
 
-	/* W1C for all flags in SR */
-	temp = 0x3F << 8;
-	writel(temp, fsl_lpspi->base + IMX7ULP_SR);
-
 	/* Clear FIFO and disable module */
 	temp = CR_RRF | CR_RTF;
 	writel(temp, fsl_lpspi->base + IMX7ULP_CR);
 
+	/* W1C for all flags in SR */
+	writel(SR_CLEAR_MASK, fsl_lpspi->base + IMX7ULP_SR);
+
 	return 0;
 }
 

-- 
2.34.1

[PATCH 05/13] spi: spi-fsl-lpspi: Enumerate all pin configuration definitions

[James Clark]

Add all the possible options, use names more similar to the reference
manual and convert _OFFSET to _MASK so we can use FIELD_PREP() and
FIELD_FITS() macros etc.

This will make it slightly easier to add a DT property for this in the
next commit.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 79b170426bee..816e48bbc810 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -69,7 +69,11 @@
 #define DER_RDDE	BIT(1)
 #define DER_TDDE	BIT(0)
 #define CFGR1_PCSCFG	BIT(27)
-#define CFGR1_PINCFG	(BIT(24)|BIT(25))
+#define CFGR1_PINCFG_MASK		GENMASK(25, 24)
+#define CFGR1_PINCFG_SIN_IN_SOUT_OUT	0
+#define CFGR1_PINCFG_SIN_ONLY		1
+#define CFGR1_PINCFG_SOUT_ONLY		2
+#define CFGR1_PINCFG_SOUT_IN_SIN_OUT	3
 #define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
 #define CFGR1_NOSTALL	BIT(3)
 #define CFGR1_HOST	BIT(0)
@@ -411,8 +415,9 @@ static int fsl_lpspi_dma_configure(struct spi_controller *controller)
 
 static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
 {
-	u32 temp;
+	u32 temp = 0;
 	int ret;
+	u8 pincfg;
 
 	if (!fsl_lpspi->is_target) {
 		ret = fsl_lpspi_set_bitrate(fsl_lpspi);
@@ -422,10 +427,14 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
 
 	fsl_lpspi_set_watermark(fsl_lpspi);
 
-	if (!fsl_lpspi->is_target)
-		temp = CFGR1_HOST;
-	else
-		temp = CFGR1_PINCFG;
+	if (!fsl_lpspi->is_target) {
+		temp |= CFGR1_HOST;
+		pincfg = CFGR1_PINCFG_SIN_IN_SOUT_OUT;
+	} else {
+		pincfg = CFGR1_PINCFG_SOUT_IN_SIN_OUT;
+	}
+	temp |= FIELD_PREP(CFGR1_PINCFG_MASK, pincfg);
+
 	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
 		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
 				   BIT(fsl_lpspi->config.chip_select));

-- 
2.34.1

[PATCH 06/13] spi: spi-fsl-lpspi: Add DT property to override default pin config

[James Clark]

This allows the SIN and SOUT pins to be flipped. Use an enum instead of
a bool even though we only support 2 modes so that the other two half
duplex modes can also be used in the future, if the driver ever supports
them.

If no property is specified, continue with the old defaults.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 816e48bbc810..98da6a5d7013 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -88,6 +88,13 @@
 
 #define SR_CLEAR_MASK	GENMASK(13, 8)
 
+static const char * const pincfgs[] = {
+	[CFGR1_PINCFG_SIN_IN_SOUT_OUT] = "sin-in-sout-out",
+	[CFGR1_PINCFG_SIN_ONLY] = "sin-only",
+	[CFGR1_PINCFG_SOUT_ONLY] = "sout-only",
+	[CFGR1_PINCFG_SOUT_IN_SIN_OUT] = "sout-in-sin-out",
+};
+
 struct fsl_lpspi_devtype_data {
 	u8 prescale_max;
 };
@@ -417,18 +424,27 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
 {
 	u32 temp = 0;
 	int ret;
+	const char *pincfg_str;
 	u8 pincfg;
 
 	if (!fsl_lpspi->is_target) {
 		ret = fsl_lpspi_set_bitrate(fsl_lpspi);
 		if (ret)
 			return ret;
+
+		temp |= CFGR1_HOST;
 	}
 
 	fsl_lpspi_set_watermark(fsl_lpspi);
 
-	if (!fsl_lpspi->is_target) {
-		temp |= CFGR1_HOST;
+	if (!of_property_read_string(fsl_lpspi->dev->of_node,
+				     "nxp,lpspi-pincfg", &pincfg_str)) {
+		pincfg = match_string(pincfgs, ARRAY_SIZE(pincfgs), pincfg_str);
+
+		if (pincfg < 0 || pincfg == CFGR1_PINCFG_SOUT_ONLY ||
+		    pincfg == CFGR1_PINCFG_SIN_ONLY)
+			return -EINVAL;
+	} else if (!fsl_lpspi->is_target) {
 		pincfg = CFGR1_PINCFG_SIN_IN_SOUT_OUT;
 	} else {
 		pincfg = CFGR1_PINCFG_SOUT_IN_SIN_OUT;

-- 
2.34.1

[PATCH 07/13] spi: spi-fsl-lpspi: Constify devtype datas

[James Clark]

struct fsl_lpspi_data->devtype_data and fsl_lpspi_dt_ids that point here
are already const, so these can be too.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 98da6a5d7013..332a852b746f 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -145,11 +145,11 @@ struct fsl_lpspi_data {
  * ERR051608 fixed or not:
  * https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
  */
-static struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
+static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
 	.prescale_max = 1,
 };
 
-static struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
+static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
 	.prescale_max = 7,
 };
 

-- 
2.34.1

[PATCH 08/13] spi: spi-fsl-lpspi: Make prescale erratum a bool

[James Clark]

This erratum only ever results in a max value of 1, otherwise the full 3
bits are available. To avoid repeating the same prescale values for
every new device's devdata, make it a bool.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 332a852b746f..1013d5c994e9 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -96,7 +96,7 @@ static const char * const pincfgs[] = {
 };
 
 struct fsl_lpspi_devtype_data {
-	u8 prescale_max;
+	bool prescale_err : 1;
 };
 
 struct lpspi_config {
@@ -144,13 +144,16 @@ struct fsl_lpspi_data {
 /*
  * ERR051608 fixed or not:
  * https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
+ *
+ * Devices with ERR051608 have a max TCR_PRESCALE value of 1, otherwise the full
+ * 3 bits are available (0-7).
  */
 static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
-	.prescale_max = 1,
+	.prescale_err = true,
 };
 
 static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
-	.prescale_max = 7,
+	.prescale_err = false,
 };
 
 static const struct of_device_id fsl_lpspi_dt_ids[] = {
@@ -329,12 +332,11 @@ static int fsl_lpspi_set_bitrate(struct fsl_lpspi_data *fsl_lpspi)
 {
 	struct lpspi_config config = fsl_lpspi->config;
 	unsigned int perclk_rate, div;
-	u8 prescale_max;
+	u8 prescale_max = fsl_lpspi->devtype_data->prescale_err ? 1 : 7;
 	u8 prescale;
 	int scldiv;
 
 	perclk_rate = clk_get_rate(fsl_lpspi->clk_per);
-	prescale_max = fsl_lpspi->devtype_data->prescale_max;
 
 	if (!config.speed_hz) {
 		dev_err(fsl_lpspi->dev,

-- 
2.34.1

[PATCH 09/13] spi: spi-fsl-lpspi: Parameterize reading num-cs from hardware

[James Clark]

To avoid adding more string matching here for every new device, move
this property into devtype data.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 1013d5c994e9..6d0138b27785 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -97,6 +97,7 @@ static const char * const pincfgs[] = {
 
 struct fsl_lpspi_devtype_data {
 	bool prescale_err : 1;
+	bool query_hw_for_num_cs : 1;
 };
 
 struct lpspi_config {
@@ -150,10 +151,12 @@ struct fsl_lpspi_data {
  */
 static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
 	.prescale_err = true,
+	.query_hw_for_num_cs = true,
 };
 
 static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
 	.prescale_err = false,
+	.query_hw_for_num_cs = false,
 };
 
 static const struct of_device_id fsl_lpspi_dt_ids[] = {
@@ -960,7 +963,7 @@ static int fsl_lpspi_probe(struct platform_device *pdev)
 	fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f);
 	if (of_property_read_u32((&pdev->dev)->of_node, "num-cs",
 				 &num_cs)) {
-		if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx93-spi"))
+		if (devtype_data->query_hw_for_num_cs)
 			num_cs = ((temp >> 16) & 0xf);
 		else
 			num_cs = 1;

-- 
2.34.1

[PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for S32G

[James Clark]

From: Larisa Grigore <larisa.grigore@nxp.com>

S32G doesn't have the max prescale erratum and it can query the max
number of CS from hardware, so add those settings.

Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 drivers/spi/spi-fsl-lpspi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
index 6d0138b27785..a4727ca37d90 100644
--- a/drivers/spi/spi-fsl-lpspi.c
+++ b/drivers/spi/spi-fsl-lpspi.c
@@ -159,9 +159,15 @@ static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
 	.query_hw_for_num_cs = false,
 };
 
+static struct fsl_lpspi_devtype_data s32g_lpspi_devtype_data = {
+	.prescale_err = false,
+	.query_hw_for_num_cs = true,
+};
+
 static const struct of_device_id fsl_lpspi_dt_ids[] = {
 	{ .compatible = "fsl,imx7ulp-spi", .data = &imx7ulp_lpspi_devtype_data,},
 	{ .compatible = "fsl,imx93-spi", .data = &imx93_lpspi_devtype_data,},
+	{ .compatible = "nxp,s32g2-lpspi", .data = &s32g_lpspi_devtype_data,},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, fsl_lpspi_dt_ids);

-- 
2.34.1

[PATCH 11/13] dt-bindings: lpspi: Update maximum num-cs value

[James Clark]

As mentioned in commit f46b06e62c86 ("spi: spi-fsl-lpspi: Read
chip-select amount from hardware for i.MX93"), some devices support up
to 3 chip selects so update the max value.

This isn't a fix or functional change because the devices with 3 chip
selects support reading the number of chip selects from hardware, so the
value wouldn't have needed to be set here. However the commit states
that the DT could be used to overwrite any HW value, so the full range
should be supported. This also avoids confusion for any readers about
how many chip selects there are.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
index a65a42ccaafe..ce7bd44ee17e 100644
--- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
@@ -64,7 +64,7 @@ properties:
     description:
       number of chip selects.
     minimum: 1
-    maximum: 2
+    maximum: 3
     default: 1
 
   power-domains:

-- 
2.34.1

[PATCH 12/13] dt-bindings: lpspi: Document nxp,lpspi-pincfg property

[James Clark]

Document the two valid pincfg values and the defaults.

Although the hardware supports two more values for half-duplex modes,
the driver doesn't support them so don't document them.

Signed-off-by: James Clark <james.clark@linaro.org>
---
 Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
index ce7bd44ee17e..3f8833911807 100644
--- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
@@ -70,6 +70,19 @@ properties:
   power-domains:
     maxItems: 1
 
+  nxp,pincfg:
+    description:
+      'Pin configuration value for CFGR1.PINCFG.
+        - "sin-in-sout-out": SIN is used for input data and SOUT is used for
+                             output data
+        - "sout-in-sin-out": SOUT is used for input data and SIN is used for
+                             output data
+      If no value is specified then the default is "sin-in-sout-out" for host
+      mode and "sout-in-sin-out" for target mode.'
+    enum:
+      - sin-in-sout-out
+      - sout-in-sin-out
+
 required:
   - compatible
   - reg
@@ -95,4 +108,5 @@ examples:
         spi-slave;
         fsl,spi-only-use-cs1-sel;
         num-cs = <2>;
+        nxp,pincfg = "sout-in-sin-out";
     };

-- 
2.34.1

[PATCH 13/13] dt-bindings: lpspi: Document support for S32G

[James Clark]

From: Larisa Grigore <larisa.grigore@nxp.com>

S32G2 and S32G3 are currently treated the same way in the driver, so
require that S32G3 is always paired with the S32G2 compatible string
until there is divergence in the future.

Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: James Clark <james.clark@linaro.org>
---
 Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
index 3f8833911807..9fc98b0f3428 100644
--- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
@@ -20,6 +20,7 @@ properties:
       - enum:
           - fsl,imx7ulp-spi
           - fsl,imx8qxp-spi
+          - nxp,s32g2-lpspi
       - items:
           - enum:
               - fsl,imx8ulp-spi
@@ -27,6 +28,10 @@ properties:
               - fsl,imx94-spi
               - fsl,imx95-spi
           - const: fsl,imx7ulp-spi
+      - items:
+          - const: nxp,s32g3-lpspi
+          - const: nxp,s32g2-lpspi
+
   reg:
     maxItems: 1
 

-- 
2.34.1

Re: [PATCH 00/13] spi: spi-fsl-lpspi: Generic fixes and support for S32G devices

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:40PM +0100, James Clark wrote:
> Various fixes for LPSI along with some refactorings.

Fixes and refactor generally is two serise.

Frank

> None of the fixes
> are strictly related to S32G, however these changes all originate from
> the work to support S32G devices. The only commits that are strictly
> related are for the new s32g2 and s32g3 compatible strings.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> To: Frank Li <Frank.Li@nxp.com>
> To: Mark Brown <broonie@kernel.org>
> To: Clark Wang <xiaoning.wang@nxp.com>
> To: Fugang Duan <B38611@freescale.com>
> To: Gao Pan <pandy.gao@nxp.com>
> To: Fugang Duan <fugang.duan@nxp.com>
> To: Rob Herring <robh@kernel.org>
> To: Krzysztof Kozlowski <krzk+dt@kernel.org>
> To: Conor Dooley <conor+dt@kernel.org>
> To: Shawn Guo <shawnguo@kernel.org>
> To: Sascha Hauer <s.hauer@pengutronix.de>
> To: Fabio Estevam <festevam@gmail.com>
> To: Larisa Grigore <larisa.grigore@oss.nxp.com>
> To: Larisa Grigore <larisa.grigore@nxp.com>
> To: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> To: Ciprianmarian Costea <ciprianmarian.costea@nxp.com>
> To: s32@nxp.com
> Cc: linux-spi@vger.kernel.org
> Cc: imx@lists.linux.dev
> Cc: linux-kernel@vger.kernel.org
> Cc: devicetree@vger.kernel.org
>
> ---
> James Clark (7):
>       spi: spi-fsl-lpspi: Enumerate all pin configuration definitions
>       spi: spi-fsl-lpspi: Add DT property to override default pin config
>       spi: spi-fsl-lpspi: Constify devtype datas
>       spi: spi-fsl-lpspi: Make prescale erratum a bool
>       spi: spi-fsl-lpspi: Parameterize reading num-cs from hardware
>       dt-bindings: lpspi: Update maximum num-cs value
>       dt-bindings: lpspi: Document nxp,lpspi-pincfg property
>
> Larisa Grigore (6):
>       spi: spi-fsl-lpspi: Fix transmissions when using CONT
>       spi: spi-fsl-lpspi: Set correct chip-select polarity bit
>       spi: spi-fsl-lpspi: Reset FIFO and disable module on transfer abort
>       spi: spi-fsl-lpspi: Clear status register after disabling the module
>       spi: spi-fsl-lpspi: Add compatible for S32G
>       dt-bindings: lpspi: Document support for S32G
>
>  .../devicetree/bindings/spi/spi-fsl-lpspi.yaml     | 21 +++++-
>  drivers/spi/spi-fsl-lpspi.c                        | 87 +++++++++++++++-------
>  2 files changed, 82 insertions(+), 26 deletions(-)
> ---
> base-commit: 0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9
> change-id: 20250715-james-nxp-lpspi-1fac58e72a59
>
> Best regards,
> --
> James Clark <james.clark@linaro.org>
>

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
> From: Larisa Grigore <larisa.grigore@nxp.com>
>
> The driver currently supports multiple chip-selects, but only sets the
> polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
> the desired chip-select.
>
> Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-lpspi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index d44a23f7d6c1..c65eb6d31ee7 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -70,7 +70,7 @@
>  #define DER_TDDE	BIT(0)
>  #define CFGR1_PCSCFG	BIT(27)
>  #define CFGR1_PINCFG	(BIT(24)|BIT(25))
> -#define CFGR1_PCSPOL	BIT(8)
> +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
>  #define CFGR1_NOSTALL	BIT(3)
>  #define CFGR1_HOST	BIT(0)
>  #define FSR_TXCOUNT	(0xFF)
> @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>  	else
>  		temp = CFGR1_PINCFG;
>  	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
> -		temp |= CFGR1_PCSPOL;
> +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
> +				   BIT(fsl_lpspi->config.chip_select));
> +

Feel like FILED_PREP(..., BIT()) is stranged.

I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)

Frank

>  	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
>
>  	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
>
> --
> 2.34.1
>

Re: [PATCH 03/13] spi: spi-fsl-lpspi: Reset FIFO and disable module on transfer abort

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:43PM +0100, James Clark wrote:
> From: Larisa Grigore <larisa.grigore@nxp.com>
>
> In DMA mode fsl_lpspi_reset() is always called at the end, even when the
> transfer is aborted. When not using DMA, aborts skip the reset leaving

Nit: s/"When not using DMA"/In PIO mode

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> the FIFO filled and the module enabled.
>
> Fix it by always calling fsl_lpspi_reset().
>
> Fixes: a15dc3d657fa ("spi: lpspi: Fix CLK pin becomes low before one transfer")
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  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 c65eb6d31ee7..aab92ee7eb94 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -734,12 +734,10 @@ static int fsl_lpspi_pio_transfer(struct spi_controller *controller,
>  	fsl_lpspi_write_tx_fifo(fsl_lpspi);
>
>  	ret = fsl_lpspi_wait_for_completion(controller);
> -	if (ret)
> -		return ret;
>
>  	fsl_lpspi_reset(fsl_lpspi);
>
> -	return 0;
> +	return ret;
>  }
>
>  static int fsl_lpspi_transfer_one(struct spi_controller *controller,
>
> --
> 2.34.1
>

Re: [PATCH 04/13] spi: spi-fsl-lpspi: Clear status register after disabling the module

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:44PM +0100, James Clark wrote:
> From: Larisa Grigore <larisa.grigore@nxp.com>
>
> Clear the error flags after disabling the module to avoid the case when
> a flag is set between when the flags were just cleared, and when the
> module is disabled.

between flags clear and module disable

And use SR_CLEAR_MASK to replace hardcoded value for improved readability


Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Although fsl_lpspi_reset() was only introduced in commit a15dc3d657fa
> ("spi: lpspi: Fix CLK pin becomes low before one transfer"), the
> original driver only reset SR in the interrupt handler, making it
> vulnerable to the same issue. Therefore the fixes commit is set at the
> introduction of the driver.
>
> Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-lpspi.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index aab92ee7eb94..79b170426bee 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -82,6 +82,8 @@
>  #define TCR_RXMSK	BIT(19)
>  #define TCR_TXMSK	BIT(18)
>
> +#define SR_CLEAR_MASK	GENMASK(13, 8)
> +
>  struct fsl_lpspi_devtype_data {
>  	u8 prescale_max;
>  };
> @@ -536,14 +538,13 @@ static int fsl_lpspi_reset(struct fsl_lpspi_data *fsl_lpspi)
>  		fsl_lpspi_intctrl(fsl_lpspi, 0);
>  	}
>
> -	/* W1C for all flags in SR */
> -	temp = 0x3F << 8;
> -	writel(temp, fsl_lpspi->base + IMX7ULP_SR);
> -
>  	/* Clear FIFO and disable module */
>  	temp = CR_RRF | CR_RTF;
>  	writel(temp, fsl_lpspi->base + IMX7ULP_CR);
>
> +	/* W1C for all flags in SR */
> +	writel(SR_CLEAR_MASK, fsl_lpspi->base + IMX7ULP_SR);
> +
>  	return 0;
>  }
>
>
> --
> 2.34.1
>

Re: [PATCH 05/13] spi: spi-fsl-lpspi: Enumerate all pin configuration definitions

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:45PM +0100, James Clark wrote:
> Add all the possible options, use names more similar to the reference

Add all the possible pincfg options,

> manual and convert _OFFSET to _MASK so we can use FIELD_PREP() and
> FIELD_FITS() macros etc.
>
> This will make it slightly easier to add a DT property for this in the
> next commit.

Make it slightly easier to add a DT property ...

No funtionality change.

>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-lpspi.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index 79b170426bee..816e48bbc810 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -69,7 +69,11 @@
>  #define DER_RDDE	BIT(1)
>  #define DER_TDDE	BIT(0)
>  #define CFGR1_PCSCFG	BIT(27)
> -#define CFGR1_PINCFG	(BIT(24)|BIT(25))
> +#define CFGR1_PINCFG_MASK		GENMASK(25, 24)
> +#define CFGR1_PINCFG_SIN_IN_SOUT_OUT	0
> +#define CFGR1_PINCFG_SIN_ONLY		1
> +#define CFGR1_PINCFG_SOUT_ONLY		2
> +#define CFGR1_PINCFG_SOUT_IN_SIN_OUT	3
>  #define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
>  #define CFGR1_NOSTALL	BIT(3)
>  #define CFGR1_HOST	BIT(0)
> @@ -411,8 +415,9 @@ static int fsl_lpspi_dma_configure(struct spi_controller *controller)
>
>  static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>  {
> -	u32 temp;
> +	u32 temp = 0;
>  	int ret;
> +	u8 pincfg;
>
>  	if (!fsl_lpspi->is_target) {
>  		ret = fsl_lpspi_set_bitrate(fsl_lpspi);
> @@ -422,10 +427,14 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>
>  	fsl_lpspi_set_watermark(fsl_lpspi);
>
> -	if (!fsl_lpspi->is_target)
> -		temp = CFGR1_HOST;
> -	else
> -		temp = CFGR1_PINCFG;
> +	if (!fsl_lpspi->is_target) {
> +		temp |= CFGR1_HOST;
> +		pincfg = CFGR1_PINCFG_SIN_IN_SOUT_OUT;
> +	} else {
> +		pincfg = CFGR1_PINCFG_SOUT_IN_SIN_OUT;
> +	}
> +	temp |= FIELD_PREP(CFGR1_PINCFG_MASK, pincfg);
> +
>  	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
>  		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
>  				   BIT(fsl_lpspi->config.chip_select));
>
> --
> 2.34.1
>

Re: [PATCH 12/13] dt-bindings: lpspi: Document nxp,lpspi-pincfg property

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:52PM +0100, James Clark wrote:
> Document the two valid pincfg values and the defaults.
>
> Although the hardware supports two more values for half-duplex modes,
> the driver doesn't support them so don't document them.

binding doc should be first patch before drivers.

binding descript hardware not driver, you should add all regardless if
driver support it.

>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> index ce7bd44ee17e..3f8833911807 100644
> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> @@ -70,6 +70,19 @@ properties:
>    power-domains:
>      maxItems: 1
>
> +  nxp,pincfg:
> +    description:
> +      'Pin configuration value for CFGR1.PINCFG.
> +        - "sin-in-sout-out": SIN is used for input data and SOUT is used for
> +                             output data
> +        - "sout-in-sin-out": SOUT is used for input data and SIN is used for
> +                             output data
> +      If no value is specified then the default is "sin-in-sout-out" for host
> +      mode and "sout-in-sin-out" for target mode.'

why need this? are there varible at difference boards? look like default
is more make sense.

SPI signal name is MOSI and MISO

Frank

> +    enum:
> +      - sin-in-sout-out
> +      - sout-in-sin-out
> +
>  required:
>    - compatible
>    - reg
> @@ -95,4 +108,5 @@ examples:
>          spi-slave;
>          fsl,spi-only-use-cs1-sel;
>          num-cs = <2>;
> +        nxp,pincfg = "sout-in-sin-out";
>      };
>
> --
> 2.34.1
>

Re: [PATCH 13/13] dt-bindings: lpspi: Document support for S32G

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:53PM +0100, James Clark wrote:
> From: Larisa Grigore <larisa.grigore@nxp.com>
>
> S32G2 and S32G3 are currently treated the same way in the driver, so
> require that S32G3 is always paired with the S32G2 compatible string
> until there is divergence in the future.

Add compatible string 'nxp,s32g2-lpspi' and 'nxp,s32g3-lpspi' for S32G2
and S32G3. Allow nxp,s32g3-lpspi fallback to nxp,s32g2-lpspi since back
compatibity.

This is independent part with other patches, you can send seperately.

Frank
>
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> index 3f8833911807..9fc98b0f3428 100644
> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> @@ -20,6 +20,7 @@ properties:
>        - enum:
>            - fsl,imx7ulp-spi
>            - fsl,imx8qxp-spi
> +          - nxp,s32g2-lpspi
>        - items:
>            - enum:
>                - fsl,imx8ulp-spi
> @@ -27,6 +28,10 @@ properties:
>                - fsl,imx94-spi
>                - fsl,imx95-spi
>            - const: fsl,imx7ulp-spi
> +      - items:
> +          - const: nxp,s32g3-lpspi
> +          - const: nxp,s32g2-lpspi
> +
>    reg:
>      maxItems: 1
>
>
> --
> 2.34.1
>

Re: [PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for S32G

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:50PM +0100, James Clark wrote:
> From: Larisa Grigore <larisa.grigore@nxp.com>
>
> S32G doesn't have the max prescale erratum and it can query the max
> number of CS from hardware, so add those settings.

binding doc should first patch. Create new patch serial for add S32G
support only.

Frank
>
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-lpspi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index 6d0138b27785..a4727ca37d90 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -159,9 +159,15 @@ static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
>  	.query_hw_for_num_cs = false,
>  };
>
> +static struct fsl_lpspi_devtype_data s32g_lpspi_devtype_data = {
> +	.prescale_err = false,
> +	.query_hw_for_num_cs = true,
> +};
> +
>  static const struct of_device_id fsl_lpspi_dt_ids[] = {
>  	{ .compatible = "fsl,imx7ulp-spi", .data = &imx7ulp_lpspi_devtype_data,},
>  	{ .compatible = "fsl,imx93-spi", .data = &imx93_lpspi_devtype_data,},
> +	{ .compatible = "nxp,s32g2-lpspi", .data = &s32g_lpspi_devtype_data,},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, fsl_lpspi_dt_ids);
>
> --
> 2.34.1
>

Re: [PATCH 11/13] dt-bindings: lpspi: Update maximum num-cs value

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:51PM +0100, James Clark wrote:
> As mentioned in commit f46b06e62c86 ("spi: spi-fsl-lpspi: Read
> chip-select amount from hardware for i.MX93"), some devices support up
> to 3 chip selects so update the max value.
>
> This isn't a fix or functional change because the devices with 3 chip
> selects support reading the number of chip selects from hardware, so the
> value wouldn't have needed to be set here. However the commit states
> that the DT could be used to overwrite any HW value, so the full range
> should be supported. This also avoids confusion for any readers about
> how many chip selects there are.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> index a65a42ccaafe..ce7bd44ee17e 100644
> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> @@ -64,7 +64,7 @@ properties:
>      description:
>        number of chip selects.
>      minimum: 1
> -    maximum: 2
> +    maximum: 3

You need keep the same restriction for other compatible string, or need
reason for other platform which also support up to 3.

Frank

>      default: 1
>
>    power-domains:
>
> --
> 2.34.1
>

Re: [PATCH 09/13] spi: spi-fsl-lpspi: Parameterize reading num-cs from hardware

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:49PM +0100, James Clark wrote:
> To avoid adding more string matching here for every new device, move
> this property into devtype data.

Add query_hw_for_num_cs in devtype to avoid directly check compatible
"fsl,imx93-spi".

No functionaltiy change.

Frank
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-lpspi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index 1013d5c994e9..6d0138b27785 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -97,6 +97,7 @@ static const char * const pincfgs[] = {
>
>  struct fsl_lpspi_devtype_data {
>  	bool prescale_err : 1;
> +	bool query_hw_for_num_cs : 1;
>  };
>
>  struct lpspi_config {
> @@ -150,10 +151,12 @@ struct fsl_lpspi_data {
>   */
>  static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
>  	.prescale_err = true,
> +	.query_hw_for_num_cs = true,
>  };
>
>  static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
>  	.prescale_err = false,
> +	.query_hw_for_num_cs = false,
>  };
>
>  static const struct of_device_id fsl_lpspi_dt_ids[] = {
> @@ -960,7 +963,7 @@ static int fsl_lpspi_probe(struct platform_device *pdev)
>  	fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f);
>  	if (of_property_read_u32((&pdev->dev)->of_node, "num-cs",
>  				 &num_cs)) {
> -		if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx93-spi"))
> +		if (devtype_data->query_hw_for_num_cs)
>  			num_cs = ((temp >> 16) & 0xf);
>  		else
>  			num_cs = 1;
>
> --
> 2.34.1
>

Re: [PATCH 00/13] spi: spi-fsl-lpspi: Generic fixes and support for S32G devices

[Mark Brown]

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

On Thu, Aug 14, 2025 at 12:40:52PM -0400, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:40PM +0100, James Clark wrote:
> > Various fixes for LPSI along with some refactorings.

> Fixes and refactor generally is two serise.

It's fine for me if there's dependencies from the fixes in the new code.

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

Re: [PATCH 08/13] spi: spi-fsl-lpspi: Make prescale erratum a bool

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:48PM +0100, James Clark wrote:
> This erratum only ever results in a max value of 1, otherwise the full 3
> bits are available. To avoid repeating the same prescale values for
> every new device's devdata, make it a bool.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-lpspi.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index 332a852b746f..1013d5c994e9 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -96,7 +96,7 @@ static const char * const pincfgs[] = {
>  };
>
>  struct fsl_lpspi_devtype_data {
> -	u8 prescale_max;
> +	bool prescale_err : 1;
>  };
>
>  struct lpspi_config {
> @@ -144,13 +144,16 @@ struct fsl_lpspi_data {
>  /*
>   * ERR051608 fixed or not:
>   * https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
> + *
> + * Devices with ERR051608 have a max TCR_PRESCALE value of 1, otherwise the full
> + * 3 bits are available (0-7).
>   */
>  static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
> -	.prescale_max = 1,
> +	.prescale_err = true,

I think prescale_max is good and clear enough, you can treat 0 as no
limit for prescale.

Frank
>  };
>
>  static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
> -	.prescale_max = 7,
> +	.prescale_err = false,
>  };
>
>  static const struct of_device_id fsl_lpspi_dt_ids[] = {
> @@ -329,12 +332,11 @@ static int fsl_lpspi_set_bitrate(struct fsl_lpspi_data *fsl_lpspi)
>  {
>  	struct lpspi_config config = fsl_lpspi->config;
>  	unsigned int perclk_rate, div;
> -	u8 prescale_max;
> +	u8 prescale_max = fsl_lpspi->devtype_data->prescale_err ? 1 : 7;
>  	u8 prescale;
>  	int scldiv;
>
>  	perclk_rate = clk_get_rate(fsl_lpspi->clk_per);
> -	prescale_max = fsl_lpspi->devtype_data->prescale_max;
>
>  	if (!config.speed_hz) {
>  		dev_err(fsl_lpspi->dev,
>
> --
> 2.34.1
>

Re: [PATCH 07/13] spi: spi-fsl-lpspi: Constify devtype datas

[Frank Li]

On Thu, Aug 14, 2025 at 05:06:47PM +0100, James Clark wrote:
> struct fsl_lpspi_data->devtype_data and fsl_lpspi_dt_ids that point here
> are already const, so these can be too.

Add const for all devtype_data.

Frank
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  drivers/spi/spi-fsl-lpspi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> index 98da6a5d7013..332a852b746f 100644
> --- a/drivers/spi/spi-fsl-lpspi.c
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -145,11 +145,11 @@ struct fsl_lpspi_data {
>   * ERR051608 fixed or not:
>   * https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
>   */
> -static struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
> +static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
>  	.prescale_max = 1,
>  };
>
> -static struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
> +static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
>  	.prescale_max = 7,
>  };
>
>
> --
> 2.34.1
>

Re: [PATCH 11/13] dt-bindings: lpspi: Update maximum num-cs value

[Rob Herring]

On Thu, Aug 14, 2025 at 05:06:51PM +0100, James Clark wrote:
> As mentioned in commit f46b06e62c86 ("spi: spi-fsl-lpspi: Read
> chip-select amount from hardware for i.MX93"), some devices support up
> to 3 chip selects so update the max value.
> 
> This isn't a fix or functional change because the devices with 3 chip
> selects support reading the number of chip selects from hardware, so the
> value wouldn't have needed to be set here. However the commit states
> that the DT could be used to overwrite any HW value, so the full range
> should be supported. This also avoids confusion for any readers about
> how many chip selects there are.

If reading the h/w gives you 3, when would the DT need to override that 
with 3? You only need an override for 2 or less.

> 
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> index a65a42ccaafe..ce7bd44ee17e 100644
> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> @@ -64,7 +64,7 @@ properties:
>      description:
>        number of chip selects.
>      minimum: 1
> -    maximum: 2
> +    maximum: 3
>      default: 1
>  
>    power-domains:
> 
> -- 
> 2.34.1
> 

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

[kernel test robot]

Hi James,

kernel test robot noticed the following build errors:

[auto build test ERROR on 0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9]

url:    https://github.com/intel-lab-lkp/linux/commits/James-Clark/spi-spi-fsl-lpspi-Fix-transmissions-when-using-CONT/20250815-001554
base:   0cc53520e68bea7fb80fdc6bdf8d226d1b6a98d9
patch link:    https://lore.kernel.org/r/20250814-james-nxp-lpspi-v1-2-9586d7815d14%40linaro.org
patch subject: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit
config: csky-randconfig-002-20250815 (https://download.01.org/0day-ci/archive/20250815/202508151101.7lDGTaxi-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250815/202508151101.7lDGTaxi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508151101.7lDGTaxi-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/spi/spi-fsl-lpspi.c: In function 'fsl_lpspi_config':
>> drivers/spi/spi-fsl-lpspi.c:428:25: error: implicit declaration of function 'FIELD_PREP' [-Wimplicit-function-declaration]
     428 |                 temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
         |                         ^~~~~~~~~~


vim +/FIELD_PREP +428 drivers/spi/spi-fsl-lpspi.c

   409	
   410	static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
   411	{
   412		u32 temp;
   413		int ret;
   414	
   415		if (!fsl_lpspi->is_target) {
   416			ret = fsl_lpspi_set_bitrate(fsl_lpspi);
   417			if (ret)
   418				return ret;
   419		}
   420	
   421		fsl_lpspi_set_watermark(fsl_lpspi);
   422	
   423		if (!fsl_lpspi->is_target)
   424			temp = CFGR1_HOST;
   425		else
   426			temp = CFGR1_PINCFG;
   427		if (fsl_lpspi->config.mode & SPI_CS_HIGH)
 > 428			temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
   429					   BIT(fsl_lpspi->config.chip_select));
   430	
   431		writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
   432	
   433		temp = readl(fsl_lpspi->base + IMX7ULP_CR);
   434		temp |= CR_RRF | CR_RTF | CR_MEN;
   435		writel(temp, fsl_lpspi->base + IMX7ULP_CR);
   436	
   437		temp = 0;
   438		if (fsl_lpspi->usedma)
   439			temp = DER_TDDE | DER_RDDE;
   440		writel(temp, fsl_lpspi->base + IMX7ULP_DER);
   441	
   442		return 0;
   443	}
   444	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 11/13] dt-bindings: lpspi: Update maximum num-cs value

[James Clark]

On 14/08/2025 9:59 pm, Rob Herring wrote:
> On Thu, Aug 14, 2025 at 05:06:51PM +0100, James Clark wrote:
>> As mentioned in commit f46b06e62c86 ("spi: spi-fsl-lpspi: Read
>> chip-select amount from hardware for i.MX93"), some devices support up
>> to 3 chip selects so update the max value.
>>
>> This isn't a fix or functional change because the devices with 3 chip
>> selects support reading the number of chip selects from hardware, so the
>> value wouldn't have needed to be set here. However the commit states
>> that the DT could be used to overwrite any HW value, so the full range
>> should be supported. This also avoids confusion for any readers about
>> how many chip selects there are.
> 
> If reading the h/w gives you 3, when would the DT need to override that
> with 3? You only need an override for 2 or less.
> 

Maybe it should say "currently the devices with 3 chip selects". I 
suppose there could be one in the future that has 3 but can't probe it.

TBH the main reason I added it was because it was confusing to work out 
what the actual max was. This says 2 when in reality it's 3. I suppose 
documenting that it should be left blank if reading from the hardware is 
supported might improve it too.

>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> index a65a42ccaafe..ce7bd44ee17e 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> @@ -64,7 +64,7 @@ properties:
>>       description:
>>         number of chip selects.
>>       minimum: 1
>> -    maximum: 2
>> +    maximum: 3
>>       default: 1
>>   
>>     power-domains:
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

[James Clark]

On 14/08/2025 5:49 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
>> From: Larisa Grigore <larisa.grigore@nxp.com>
>>
>> The driver currently supports multiple chip-selects, but only sets the
>> polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
>> the desired chip-select.
>>
>> Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-lpspi.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>> index d44a23f7d6c1..c65eb6d31ee7 100644
>> --- a/drivers/spi/spi-fsl-lpspi.c
>> +++ b/drivers/spi/spi-fsl-lpspi.c
>> @@ -70,7 +70,7 @@
>>   #define DER_TDDE	BIT(0)
>>   #define CFGR1_PCSCFG	BIT(27)
>>   #define CFGR1_PINCFG	(BIT(24)|BIT(25))
>> -#define CFGR1_PCSPOL	BIT(8)
>> +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
>>   #define CFGR1_NOSTALL	BIT(3)
>>   #define CFGR1_HOST	BIT(0)
>>   #define FSR_TXCOUNT	(0xFF)
>> @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>>   	else
>>   		temp = CFGR1_PINCFG;
>>   	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
>> -		temp |= CFGR1_PCSPOL;
>> +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
>> +				   BIT(fsl_lpspi->config.chip_select));
>> +
> 
> Feel like FILED_PREP(..., BIT()) is stranged.
> 
> I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)
> 
> Frank

It's using an existing macro that everyone knows though and I found 65 
instances of exactly this. It can be read as "set bit X and put it into 
the PCSPOL field without any further investigation.

If we make a new macro, first the reader will have to jump to it, then 
it still doesn't immediately explain what the "+ 8" part is. Using 
FIELD_PREP() also has the potential to use autogenerated field masks 
from a machine readable version of the reference manual. You can't 
statically check your macro to see if + 8 is correct or not, and it also 
doesn't catch overflow errors like FIELD_PREP() does.

There might be an argument to add a new global macro like 
FIELD_BIT(mask, bit). But it's not very flexible (can't set multiple 
bits) and you can already accomplish the same thing by adding BIT() to 
the existing one.

Thanks
James

> 
>>   	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
>>
>>   	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
>>
>> --
>> 2.34.1
>>

Re: [PATCH 03/13] spi: spi-fsl-lpspi: Reset FIFO and disable module on transfer abort

[James Clark]

On 14/08/2025 5:51 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:43PM +0100, James Clark wrote:
>> From: Larisa Grigore <larisa.grigore@nxp.com>
>>
>> In DMA mode fsl_lpspi_reset() is always called at the end, even when the
>> transfer is aborted. When not using DMA, aborts skip the reset leaving
> 
> Nit: s/"When not using DMA"/In PIO mode

Ack

> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
>> the FIFO filled and the module enabled.
>>
>> Fix it by always calling fsl_lpspi_reset().
>>
>> Fixes: a15dc3d657fa ("spi: lpspi: Fix CLK pin becomes low before one transfer")
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   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 c65eb6d31ee7..aab92ee7eb94 100644
>> --- a/drivers/spi/spi-fsl-lpspi.c
>> +++ b/drivers/spi/spi-fsl-lpspi.c
>> @@ -734,12 +734,10 @@ static int fsl_lpspi_pio_transfer(struct spi_controller *controller,
>>   	fsl_lpspi_write_tx_fifo(fsl_lpspi);
>>
>>   	ret = fsl_lpspi_wait_for_completion(controller);
>> -	if (ret)
>> -		return ret;
>>
>>   	fsl_lpspi_reset(fsl_lpspi);
>>
>> -	return 0;
>> +	return ret;
>>   }
>>
>>   static int fsl_lpspi_transfer_one(struct spi_controller *controller,
>>
>> --
>> 2.34.1
>>

Re: [PATCH 04/13] spi: spi-fsl-lpspi: Clear status register after disabling the module

[James Clark]

On 14/08/2025 5:58 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:44PM +0100, James Clark wrote:
>> From: Larisa Grigore <larisa.grigore@nxp.com>
>>
>> Clear the error flags after disabling the module to avoid the case when
>> a flag is set between when the flags were just cleared, and when the
>> module is disabled.
> 
> between flags clear and module disable
> 
> And use SR_CLEAR_MASK to replace hardcoded value for improved readability
> 

Ack

> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
>>
>> Although fsl_lpspi_reset() was only introduced in commit a15dc3d657fa
>> ("spi: lpspi: Fix CLK pin becomes low before one transfer"), the
>> original driver only reset SR in the interrupt handler, making it
>> vulnerable to the same issue. Therefore the fixes commit is set at the
>> introduction of the driver.
>>
>> Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-lpspi.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>> index aab92ee7eb94..79b170426bee 100644
>> --- a/drivers/spi/spi-fsl-lpspi.c
>> +++ b/drivers/spi/spi-fsl-lpspi.c
>> @@ -82,6 +82,8 @@
>>   #define TCR_RXMSK	BIT(19)
>>   #define TCR_TXMSK	BIT(18)
>>
>> +#define SR_CLEAR_MASK	GENMASK(13, 8)
>> +
>>   struct fsl_lpspi_devtype_data {
>>   	u8 prescale_max;
>>   };
>> @@ -536,14 +538,13 @@ static int fsl_lpspi_reset(struct fsl_lpspi_data *fsl_lpspi)
>>   		fsl_lpspi_intctrl(fsl_lpspi, 0);
>>   	}
>>
>> -	/* W1C for all flags in SR */
>> -	temp = 0x3F << 8;
>> -	writel(temp, fsl_lpspi->base + IMX7ULP_SR);
>> -
>>   	/* Clear FIFO and disable module */
>>   	temp = CR_RRF | CR_RTF;
>>   	writel(temp, fsl_lpspi->base + IMX7ULP_CR);
>>
>> +	/* W1C for all flags in SR */
>> +	writel(SR_CLEAR_MASK, fsl_lpspi->base + IMX7ULP_SR);
>> +
>>   	return 0;
>>   }
>>
>>
>> --
>> 2.34.1
>>

Re: [PATCH 11/13] dt-bindings: lpspi: Update maximum num-cs value

[James Clark]

On 14/08/2025 7:28 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:51PM +0100, James Clark wrote:
>> As mentioned in commit f46b06e62c86 ("spi: spi-fsl-lpspi: Read
>> chip-select amount from hardware for i.MX93"), some devices support up
>> to 3 chip selects so update the max value.
>>
>> This isn't a fix or functional change because the devices with 3 chip
>> selects support reading the number of chip selects from hardware, so the
>> value wouldn't have needed to be set here. However the commit states
>> that the DT could be used to overwrite any HW value, so the full range
>> should be supported. This also avoids confusion for any readers about
>> how many chip selects there are.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> index a65a42ccaafe..ce7bd44ee17e 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> @@ -64,7 +64,7 @@ properties:
>>       description:
>>         number of chip selects.
>>       minimum: 1
>> -    maximum: 2
>> +    maximum: 3
> 
> You need keep the same restriction for other compatible string, or need

Not sure I follow here. Don't the binding docs only cover the maximum 
range of valid inputs for all covered platforms? They don't go into 
details about which ranges are valid for every individual sub-platform.

For example if a platform didn't support DMA we wouldn't say it's not 
valid to label DMA channels in the binding doc. If someone puts 3 
instead of 2 then that's just a mistake, but documenting valid ranges 
can't really fix a mistake like that. And changing 2 to 3 doesn't break 
existing DTs, only making it smaller would.

> reason for other platform which also support up to 3.

The reason is that some platforms support 3, so I thought it made most 
sense to set the max to 3. I replied more on the thread with Rob, but we 
can just drop this one.

> 
> Frank
> 
>>       default: 1
>>
>>     power-domains:
>>
>> --
>> 2.34.1
>>

Re: [PATCH 05/13] spi: spi-fsl-lpspi: Enumerate all pin configuration definitions

[James Clark]

On 14/08/2025 7:10 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:45PM +0100, James Clark wrote:
>> Add all the possible options, use names more similar to the reference
> 
> Add all the possible pincfg options,
> 
>> manual and convert _OFFSET to _MASK so we can use FIELD_PREP() and
>> FIELD_FITS() macros etc.
>>
>> This will make it slightly easier to add a DT property for this in the
>> next commit.
> 
> Make it slightly easier to add a DT property ...
> 
> No funtionality change.
> 

Ack

>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-lpspi.c | 21 +++++++++++++++------
>>   1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>> index 79b170426bee..816e48bbc810 100644
>> --- a/drivers/spi/spi-fsl-lpspi.c
>> +++ b/drivers/spi/spi-fsl-lpspi.c
>> @@ -69,7 +69,11 @@
>>   #define DER_RDDE	BIT(1)
>>   #define DER_TDDE	BIT(0)
>>   #define CFGR1_PCSCFG	BIT(27)
>> -#define CFGR1_PINCFG	(BIT(24)|BIT(25))
>> +#define CFGR1_PINCFG_MASK		GENMASK(25, 24)
>> +#define CFGR1_PINCFG_SIN_IN_SOUT_OUT	0
>> +#define CFGR1_PINCFG_SIN_ONLY		1
>> +#define CFGR1_PINCFG_SOUT_ONLY		2
>> +#define CFGR1_PINCFG_SOUT_IN_SIN_OUT	3
>>   #define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
>>   #define CFGR1_NOSTALL	BIT(3)
>>   #define CFGR1_HOST	BIT(0)
>> @@ -411,8 +415,9 @@ static int fsl_lpspi_dma_configure(struct spi_controller *controller)
>>
>>   static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>>   {
>> -	u32 temp;
>> +	u32 temp = 0;
>>   	int ret;
>> +	u8 pincfg;
>>
>>   	if (!fsl_lpspi->is_target) {
>>   		ret = fsl_lpspi_set_bitrate(fsl_lpspi);
>> @@ -422,10 +427,14 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>>
>>   	fsl_lpspi_set_watermark(fsl_lpspi);
>>
>> -	if (!fsl_lpspi->is_target)
>> -		temp = CFGR1_HOST;
>> -	else
>> -		temp = CFGR1_PINCFG;
>> +	if (!fsl_lpspi->is_target) {
>> +		temp |= CFGR1_HOST;
>> +		pincfg = CFGR1_PINCFG_SIN_IN_SOUT_OUT;
>> +	} else {
>> +		pincfg = CFGR1_PINCFG_SOUT_IN_SIN_OUT;
>> +	}
>> +	temp |= FIELD_PREP(CFGR1_PINCFG_MASK, pincfg);
>> +
>>   	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
>>   		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
>>   				   BIT(fsl_lpspi->config.chip_select));
>>
>> --
>> 2.34.1
>>

Re: [PATCH 07/13] spi: spi-fsl-lpspi: Constify devtype datas

[James Clark]

On 14/08/2025 7:38 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:47PM +0100, James Clark wrote:
>> struct fsl_lpspi_data->devtype_data and fsl_lpspi_dt_ids that point here
>> are already const, so these can be too.
> 
> Add const for all devtype_data.
> 
> Frank

Ack

>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-lpspi.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>> index 98da6a5d7013..332a852b746f 100644
>> --- a/drivers/spi/spi-fsl-lpspi.c
>> +++ b/drivers/spi/spi-fsl-lpspi.c
>> @@ -145,11 +145,11 @@ struct fsl_lpspi_data {
>>    * ERR051608 fixed or not:
>>    * https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
>>    */
>> -static struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
>> +static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
>>   	.prescale_max = 1,
>>   };
>>
>> -static struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
>> +static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
>>   	.prescale_max = 7,
>>   };
>>
>>
>> --
>> 2.34.1
>>

Re: [PATCH 08/13] spi: spi-fsl-lpspi: Make prescale erratum a bool

[James Clark]

On 14/08/2025 7:36 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:48PM +0100, James Clark wrote:
>> This erratum only ever results in a max value of 1, otherwise the full 3
>> bits are available. To avoid repeating the same prescale values for
>> every new device's devdata, make it a bool.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-lpspi.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>> index 332a852b746f..1013d5c994e9 100644
>> --- a/drivers/spi/spi-fsl-lpspi.c
>> +++ b/drivers/spi/spi-fsl-lpspi.c
>> @@ -96,7 +96,7 @@ static const char * const pincfgs[] = {
>>   };
>>
>>   struct fsl_lpspi_devtype_data {
>> -	u8 prescale_max;
>> +	bool prescale_err : 1;
>>   };
>>
>>   struct lpspi_config {
>> @@ -144,13 +144,16 @@ struct fsl_lpspi_data {
>>   /*
>>    * ERR051608 fixed or not:
>>    * https://www.nxp.com/docs/en/errata/i.MX93_1P87f.pdf
>> + *
>> + * Devices with ERR051608 have a max TCR_PRESCALE value of 1, otherwise the full
>> + * 3 bits are available (0-7).
>>    */
>>   static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
>> -	.prescale_max = 1,
>> +	.prescale_err = true,
> 
> I think prescale_max is good and clear enough, you can treat 0 as no
> limit for prescale.
> 
> Frank

Ack

>>   };
>>
>>   static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
>> -	.prescale_max = 7,
>> +	.prescale_err = false,
>>   };
>>
>>   static const struct of_device_id fsl_lpspi_dt_ids[] = {
>> @@ -329,12 +332,11 @@ static int fsl_lpspi_set_bitrate(struct fsl_lpspi_data *fsl_lpspi)
>>   {
>>   	struct lpspi_config config = fsl_lpspi->config;
>>   	unsigned int perclk_rate, div;
>> -	u8 prescale_max;
>> +	u8 prescale_max = fsl_lpspi->devtype_data->prescale_err ? 1 : 7;
>>   	u8 prescale;
>>   	int scldiv;
>>
>>   	perclk_rate = clk_get_rate(fsl_lpspi->clk_per);
>> -	prescale_max = fsl_lpspi->devtype_data->prescale_max;
>>
>>   	if (!config.speed_hz) {
>>   		dev_err(fsl_lpspi->dev,
>>
>> --
>> 2.34.1
>>

Re: [PATCH 09/13] spi: spi-fsl-lpspi: Parameterize reading num-cs from hardware

[James Clark]

On 14/08/2025 7:31 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:49PM +0100, James Clark wrote:
>> To avoid adding more string matching here for every new device, move
>> this property into devtype data.
> 
> Add query_hw_for_num_cs in devtype to avoid directly check compatible
> "fsl,imx93-spi".
> 
> No functionaltiy change.
> 
> Frank

Ack

>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-lpspi.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>> index 1013d5c994e9..6d0138b27785 100644
>> --- a/drivers/spi/spi-fsl-lpspi.c
>> +++ b/drivers/spi/spi-fsl-lpspi.c
>> @@ -97,6 +97,7 @@ static const char * const pincfgs[] = {
>>
>>   struct fsl_lpspi_devtype_data {
>>   	bool prescale_err : 1;
>> +	bool query_hw_for_num_cs : 1;
>>   };
>>
>>   struct lpspi_config {
>> @@ -150,10 +151,12 @@ struct fsl_lpspi_data {
>>    */
>>   static const struct fsl_lpspi_devtype_data imx93_lpspi_devtype_data = {
>>   	.prescale_err = true,
>> +	.query_hw_for_num_cs = true,
>>   };
>>
>>   static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
>>   	.prescale_err = false,
>> +	.query_hw_for_num_cs = false,
>>   };
>>
>>   static const struct of_device_id fsl_lpspi_dt_ids[] = {
>> @@ -960,7 +963,7 @@ static int fsl_lpspi_probe(struct platform_device *pdev)
>>   	fsl_lpspi->rxfifosize = 1 << ((temp >> 8) & 0x0f);
>>   	if (of_property_read_u32((&pdev->dev)->of_node, "num-cs",
>>   				 &num_cs)) {
>> -		if (of_device_is_compatible(pdev->dev.of_node, "fsl,imx93-spi"))
>> +		if (devtype_data->query_hw_for_num_cs)
>>   			num_cs = ((temp >> 16) & 0xf);
>>   		else
>>   			num_cs = 1;
>>
>> --
>> 2.34.1
>>

Re: [PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for S32G

[James Clark]

On 14/08/2025 7:25 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:50PM +0100, James Clark wrote:
>> From: Larisa Grigore <larisa.grigore@nxp.com>
>>
>> S32G doesn't have the max prescale erratum and it can query the max
>> number of CS from hardware, so add those settings.
> 
> binding doc should first patch. Create new patch serial for add S32G
> support only.
> 
> Frank

I'm not sure putting the binding doc commit first would be right? That 
would imply it was a valid binding before it really was because the code 
change hasn't been made yet. Practically both are required so it doesn't 
really matter which way around they are.

As for splitting the set into two, Mark mentioned that he was ok with a 
single one, so I assume that's fine? The devtype_data changes would 
conflict unless they were applied in the correct order anyway, implying 
the need for a single ordered patchset.

James

>>
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   drivers/spi/spi-fsl-lpspi.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>> index 6d0138b27785..a4727ca37d90 100644
>> --- a/drivers/spi/spi-fsl-lpspi.c
>> +++ b/drivers/spi/spi-fsl-lpspi.c
>> @@ -159,9 +159,15 @@ static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
>>   	.query_hw_for_num_cs = false,
>>   };
>>
>> +static struct fsl_lpspi_devtype_data s32g_lpspi_devtype_data = {
>> +	.prescale_err = false,
>> +	.query_hw_for_num_cs = true,
>> +};
>> +
>>   static const struct of_device_id fsl_lpspi_dt_ids[] = {
>>   	{ .compatible = "fsl,imx7ulp-spi", .data = &imx7ulp_lpspi_devtype_data,},
>>   	{ .compatible = "fsl,imx93-spi", .data = &imx93_lpspi_devtype_data,},
>> +	{ .compatible = "nxp,s32g2-lpspi", .data = &s32g_lpspi_devtype_data,},
>>   	{ /* sentinel */ }
>>   };
>>   MODULE_DEVICE_TABLE(of, fsl_lpspi_dt_ids);
>>
>> --
>> 2.34.1
>>

Re: [PATCH 12/13] dt-bindings: lpspi: Document nxp,lpspi-pincfg property

[James Clark]

On 14/08/2025 7:19 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:52PM +0100, James Clark wrote:
>> Document the two valid pincfg values and the defaults.
>>
>> Although the hardware supports two more values for half-duplex modes,
>> the driver doesn't support them so don't document them.
> 
> binding doc should be first patch before drivers.
> 
> binding descript hardware not driver, you should add all regardless if
> driver support it.
> 

Replied to same on "[PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for 
S32G"

>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> index ce7bd44ee17e..3f8833911807 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> @@ -70,6 +70,19 @@ properties:
>>     power-domains:
>>       maxItems: 1
>>
>> +  nxp,pincfg:
>> +    description:
>> +      'Pin configuration value for CFGR1.PINCFG.
>> +        - "sin-in-sout-out": SIN is used for input data and SOUT is used for
>> +                             output data
>> +        - "sout-in-sin-out": SOUT is used for input data and SIN is used for
>> +                             output data
>> +      If no value is specified then the default is "sin-in-sout-out" for host
>> +      mode and "sout-in-sin-out" for target mode.'
> 
> why need this? are there varible at difference boards? look like default
> is more make sense.
> 

+ Larissa. I think this might also be a question for the hardware 
designers about why the feature to swap the pins was deemed worth including.

I'm assuming the flexibility is given for routing reasons. If you have 
another device with the pins in one order then you can route it without 
a via if they happen to be in the same order.

> SPI signal name is MOSI and MISO
> 
> Frank
> 

As mentioned in the commit message of "[PATCH 05/13] spi: spi-fsl-lpspi: 
Enumerate all pin configuration definitions" the names were taken 
directly from the reference manual and this doc text was too. I think 
diverging from CFGR1_PINCFG could be potentially quite confusing. And 
MOSI isn't mentioned once in S32G3RM rev 4:

   Configures which pins are used for input and output data during serial
   transfers. When performing parallel transfers, the Pin Configuration
   field is ignored.

     00b - SIN is used for input data and SOUT is used for output data
     01b - SIN is used for both input and output data, only half-duplex
           serial transfers are supported
     10b - SOUT is used for both input and output data, only half-duplex
           serial transfers are supported
     11b - SOUT is used for input data and SIN is used for output data

James

>> +    enum:
>> +      - sin-in-sout-out
>> +      - sout-in-sin-out
>> +
>>   required:
>>     - compatible
>>     - reg
>> @@ -95,4 +108,5 @@ examples:
>>           spi-slave;
>>           fsl,spi-only-use-cs1-sel;
>>           num-cs = <2>;
>> +        nxp,pincfg = "sout-in-sin-out";
>>       };
>>
>> --
>> 2.34.1
>>

Re: [PATCH 13/13] dt-bindings: lpspi: Document support for S32G

[James Clark]

On 14/08/2025 7:23 pm, Frank Li wrote:
> On Thu, Aug 14, 2025 at 05:06:53PM +0100, James Clark wrote:
>> From: Larisa Grigore <larisa.grigore@nxp.com>
>>
>> S32G2 and S32G3 are currently treated the same way in the driver, so
>> require that S32G3 is always paired with the S32G2 compatible string
>> until there is divergence in the future.
> 
> Add compatible string 'nxp,s32g2-lpspi' and 'nxp,s32g3-lpspi' for S32G2
> and S32G3. Allow nxp,s32g3-lpspi fallback to nxp,s32g2-lpspi since back
> compatibity.

Ack

> 
> This is independent part with other patches, you can send seperately.
> 

Replied on other threads about this.

> Frank
>>
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> index 3f8833911807..9fc98b0f3428 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>> @@ -20,6 +20,7 @@ properties:
>>         - enum:
>>             - fsl,imx7ulp-spi
>>             - fsl,imx8qxp-spi
>> +          - nxp,s32g2-lpspi
>>         - items:
>>             - enum:
>>                 - fsl,imx8ulp-spi
>> @@ -27,6 +28,10 @@ properties:
>>                 - fsl,imx94-spi
>>                 - fsl,imx95-spi
>>             - const: fsl,imx7ulp-spi
>> +      - items:
>> +          - const: nxp,s32g3-lpspi
>> +          - const: nxp,s32g2-lpspi
>> +
>>     reg:
>>       maxItems: 1
>>
>>
>> --
>> 2.34.1
>>

Re: [PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for S32G

[Mark Brown]

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

On Mon, Aug 18, 2025 at 03:31:08PM +0100, James Clark wrote:
> On 14/08/2025 7:25 pm, Frank Li wrote:

> > binding doc should first patch. Create new patch serial for add S32G
> > support only.

> I'm not sure putting the binding doc commit first would be right? That would
> imply it was a valid binding before it really was because the code change
> hasn't been made yet. Practically both are required so it doesn't really
> matter which way around they are.

It's the general practice everyone has adopted (though in this case the
bugfix bits might want to go before the bindings, possibly it's also a
bit unusual to do that).  An unused binding is more acceptable than an
undocumented one.

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

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

[Frank Li]

On Mon, Aug 18, 2025 at 02:05:16PM +0100, James Clark wrote:
>
>
> On 14/08/2025 5:49 pm, Frank Li wrote:
> > On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
> > > From: Larisa Grigore <larisa.grigore@nxp.com>
> > >
> > > The driver currently supports multiple chip-selects, but only sets the
> > > polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
> > > the desired chip-select.
> > >
> > > Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
> > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > ---
> > >   drivers/spi/spi-fsl-lpspi.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> > > index d44a23f7d6c1..c65eb6d31ee7 100644
> > > --- a/drivers/spi/spi-fsl-lpspi.c
> > > +++ b/drivers/spi/spi-fsl-lpspi.c
> > > @@ -70,7 +70,7 @@
> > >   #define DER_TDDE	BIT(0)
> > >   #define CFGR1_PCSCFG	BIT(27)
> > >   #define CFGR1_PINCFG	(BIT(24)|BIT(25))
> > > -#define CFGR1_PCSPOL	BIT(8)
> > > +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
> > >   #define CFGR1_NOSTALL	BIT(3)
> > >   #define CFGR1_HOST	BIT(0)
> > >   #define FSR_TXCOUNT	(0xFF)
> > > @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
> > >   	else
> > >   		temp = CFGR1_PINCFG;
> > >   	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
> > > -		temp |= CFGR1_PCSPOL;
> > > +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
> > > +				   BIT(fsl_lpspi->config.chip_select));
> > > +
> >
> > Feel like FILED_PREP(..., BIT()) is stranged.
> >
> > I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)
> >
> > Frank
>
> It's using an existing macro that everyone knows though and I found 65
> instances of exactly this. It can be read as "set bit X and put it into the
> PCSPOL field without any further investigation.

Where have such pattern in kernel?

Frank

>
> If we make a new macro, first the reader will have to jump to it, then it
> still doesn't immediately explain what the "+ 8" part is. Using FIELD_PREP()
> also has the potential to use autogenerated field masks from a machine
> readable version of the reference manual. You can't statically check your
> macro to see if + 8 is correct or not, and it also doesn't catch overflow
> errors like FIELD_PREP() does.
>
> There might be an argument to add a new global macro like FIELD_BIT(mask,
> bit). But it's not very flexible (can't set multiple bits) and you can
> already accomplish the same thing by adding BIT() to the existing one.
>
> Thanks
> James
>
> >
> > >   	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
> > >
> > >   	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
> > >
> > > --
> > > 2.34.1
> > >
>

Re: [PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for S32G

[Frank Li]

On Mon, Aug 18, 2025 at 03:31:08PM +0100, James Clark wrote:
>
>
> On 14/08/2025 7:25 pm, Frank Li wrote:
> > On Thu, Aug 14, 2025 at 05:06:50PM +0100, James Clark wrote:
> > > From: Larisa Grigore <larisa.grigore@nxp.com>
> > >
> > > S32G doesn't have the max prescale erratum and it can query the max
> > > number of CS from hardware, so add those settings.
> >
> > binding doc should first patch. Create new patch serial for add S32G
> > support only.
> >
> > Frank
>
> I'm not sure putting the binding doc commit first would be right? That would
> imply it was a valid binding before it really was because the code change
> hasn't been made yet. Practically both are required so it doesn't really
> matter which way around they are.

DT binding descrpit hardware not driver. Verify binding by dt_binding_check
not by drivers. When driver use a dt property, which have to descript it
first at binding doc. So binding doc patch should be before the driver use
property. That's dt team requirement. You can wait for dt team comment this,
But I am pertty sure it is ture.

>
> As for splitting the set into two, Mark mentioned that he was ok with a
> single one, so I assume that's fine? The devtype_data changes would conflict
> unless they were applied in the correct order anyway, implying the need for
> a single ordered patchset.

Yes, if there are dependence.

Frank
>
> James
>
> > >
> > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> > > Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@nxp.com>
> > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > ---
> > >   drivers/spi/spi-fsl-lpspi.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> > > index 6d0138b27785..a4727ca37d90 100644
> > > --- a/drivers/spi/spi-fsl-lpspi.c
> > > +++ b/drivers/spi/spi-fsl-lpspi.c
> > > @@ -159,9 +159,15 @@ static const struct fsl_lpspi_devtype_data imx7ulp_lpspi_devtype_data = {
> > >   	.query_hw_for_num_cs = false,
> > >   };
> > >
> > > +static struct fsl_lpspi_devtype_data s32g_lpspi_devtype_data = {
> > > +	.prescale_err = false,
> > > +	.query_hw_for_num_cs = true,
> > > +};
> > > +
> > >   static const struct of_device_id fsl_lpspi_dt_ids[] = {
> > >   	{ .compatible = "fsl,imx7ulp-spi", .data = &imx7ulp_lpspi_devtype_data,},
> > >   	{ .compatible = "fsl,imx93-spi", .data = &imx93_lpspi_devtype_data,},
> > > +	{ .compatible = "nxp,s32g2-lpspi", .data = &s32g_lpspi_devtype_data,},
> > >   	{ /* sentinel */ }
> > >   };
> > >   MODULE_DEVICE_TABLE(of, fsl_lpspi_dt_ids);
> > >
> > > --
> > > 2.34.1
> > >
>

Re: [PATCH 12/13] dt-bindings: lpspi: Document nxp,lpspi-pincfg property

[Frank Li]

On Mon, Aug 18, 2025 at 03:47:45PM +0100, James Clark wrote:
>
>
> On 14/08/2025 7:19 pm, Frank Li wrote:
> > On Thu, Aug 14, 2025 at 05:06:52PM +0100, James Clark wrote:
> > > Document the two valid pincfg values and the defaults.
> > >
> > > Although the hardware supports two more values for half-duplex modes,
> > > the driver doesn't support them so don't document them.
> >
> > binding doc should be first patch before drivers.
> >
> > binding descript hardware not driver, you should add all regardless if
> > driver support it.
> >
>
> Replied to same on "[PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for
> S32G"
>
> > >
> > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > ---
> > >   Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 14 ++++++++++++++
> > >   1 file changed, 14 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> > > index ce7bd44ee17e..3f8833911807 100644
> > > --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> > > +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> > > @@ -70,6 +70,19 @@ properties:
> > >     power-domains:
> > >       maxItems: 1
> > >
> > > +  nxp,pincfg:
> > > +    description:
> > > +      'Pin configuration value for CFGR1.PINCFG.
> > > +        - "sin-in-sout-out": SIN is used for input data and SOUT is used for
> > > +                             output data
> > > +        - "sout-in-sin-out": SOUT is used for input data and SIN is used for
> > > +                             output data
> > > +      If no value is specified then the default is "sin-in-sout-out" for host
> > > +      mode and "sout-in-sin-out" for target mode.'
> >
> > why need this? are there varible at difference boards? look like default
> > is more make sense.
> >
>
> + Larissa. I think this might also be a question for the hardware designers
> about why the feature to swap the pins was deemed worth including.
>
> I'm assuming the flexibility is given for routing reasons. If you have
> another device with the pins in one order then you can route it without a
> via if they happen to be in the same order.

DT team need reason to judge if a new property is reasonable/neccesary. You
need  mention the reason why need this property. Here, some board design
swap sin/sout.

>
> > SPI signal name is MOSI and MISO
> >
> > Frank
> >
>
> As mentioned in the commit message of "[PATCH 05/13] spi: spi-fsl-lpspi:
> Enumerate all pin configuration definitions" the names were taken directly
> from the reference manual and this doc text was too. I think diverging from
> CFGR1_PINCFG could be potentially quite confusing. And MOSI isn't mentioned
> once in S32G3RM rev 4:
>
>   Configures which pins are used for input and output data during serial
>   transfers. When performing parallel transfers, the Pin Configuration
>   field is ignored.
>
>     00b - SIN is used for input data and SOUT is used for output data
>     01b - SIN is used for both input and output data, only half-duplex
>           serial transfers are supported
>     10b - SOUT is used for both input and output data, only half-duplex
>           serial transfers are supported
>     11b - SOUT is used for input data and SIN is used for output data

dt binding is ABI, design user easy understand property string.  like

enum:
  - normal
  - swap
  - half-duplex-on-sin
  - half-duplex-on-sout

Frank

>
> James
>
> > > +    enum:
> > > +      - sin-in-sout-out
> > > +      - sout-in-sin-out
> > > +
> > >   required:
> > >     - compatible
> > >     - reg
> > > @@ -95,4 +108,5 @@ examples:
> > >           spi-slave;
> > >           fsl,spi-only-use-cs1-sel;
> > >           num-cs = <2>;
> > > +        nxp,pincfg = "sout-in-sin-out";
> > >       };
> > >
> > > --
> > > 2.34.1
> > >
>

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

[James Clark]

On 18/08/2025 4:19 pm, Frank Li wrote:
> On Mon, Aug 18, 2025 at 02:05:16PM +0100, James Clark wrote:
>>
>>
>> On 14/08/2025 5:49 pm, Frank Li wrote:
>>> On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
>>>> From: Larisa Grigore <larisa.grigore@nxp.com>
>>>>
>>>> The driver currently supports multiple chip-selects, but only sets the
>>>> polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
>>>> the desired chip-select.
>>>>
>>>> Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
>>>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>> ---
>>>>    drivers/spi/spi-fsl-lpspi.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
>>>> index d44a23f7d6c1..c65eb6d31ee7 100644
>>>> --- a/drivers/spi/spi-fsl-lpspi.c
>>>> +++ b/drivers/spi/spi-fsl-lpspi.c
>>>> @@ -70,7 +70,7 @@
>>>>    #define DER_TDDE	BIT(0)
>>>>    #define CFGR1_PCSCFG	BIT(27)
>>>>    #define CFGR1_PINCFG	(BIT(24)|BIT(25))
>>>> -#define CFGR1_PCSPOL	BIT(8)
>>>> +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
>>>>    #define CFGR1_NOSTALL	BIT(3)
>>>>    #define CFGR1_HOST	BIT(0)
>>>>    #define FSR_TXCOUNT	(0xFF)
>>>> @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
>>>>    	else
>>>>    		temp = CFGR1_PINCFG;
>>>>    	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
>>>> -		temp |= CFGR1_PCSPOL;
>>>> +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
>>>> +				   BIT(fsl_lpspi->config.chip_select));
>>>> +
>>>
>>> Feel like FILED_PREP(..., BIT()) is stranged.
>>>
>>> I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)
>>>
>>> Frank
>>
>> It's using an existing macro that everyone knows though and I found 65
>> instances of exactly this. It can be read as "set bit X and put it into the
>> PCSPOL field without any further investigation.
> 
> Where have such pattern in kernel?
> 
> Frank
> 

Grep "FIELD_PREP\(.*,\n?.*BIT\("

65 results, e.g:

   return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));

James

>
>> If we make a new macro, first the reader will have to jump to it, then it
>> still doesn't immediately explain what the "+ 8" part is. Using FIELD_PREP()
>> also has the potential to use autogenerated field masks from a machine
>> readable version of the reference manual. You can't statically check your
>> macro to see if + 8 is correct or not, and it also doesn't catch overflow
>> errors like FIELD_PREP() does.
>>
>> There might be an argument to add a new global macro like FIELD_BIT(mask,
>> bit). But it's not very flexible (can't set multiple bits) and you can
>> already accomplish the same thing by adding BIT() to the existing one.
>>
>> Thanks
>> James
>>
>>>
>>>>    	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
>>>>
>>>>    	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>

Re: [PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for S32G

[James Clark]

On 18/08/2025 4:18 pm, Mark Brown wrote:
> On Mon, Aug 18, 2025 at 03:31:08PM +0100, James Clark wrote:
>> On 14/08/2025 7:25 pm, Frank Li wrote:
> 
>>> binding doc should first patch. Create new patch serial for add S32G
>>> support only.
> 
>> I'm not sure putting the binding doc commit first would be right? That would
>> imply it was a valid binding before it really was because the code change
>> hasn't been made yet. Practically both are required so it doesn't really
>> matter which way around they are.
> 
> It's the general practice everyone has adopted (though in this case the
> bugfix bits might want to go before the bindings, possibly it's also a
> bit unusual to do that).  An unused binding is more acceptable than an
> undocumented one.

Fair enough. I can flip them in the next version.

Re: [PATCH 12/13] dt-bindings: lpspi: Document nxp,lpspi-pincfg property

[James Clark]

On 18/08/2025 4:39 pm, Frank Li wrote:
> On Mon, Aug 18, 2025 at 03:47:45PM +0100, James Clark wrote:
>>
>>
>> On 14/08/2025 7:19 pm, Frank Li wrote:
>>> On Thu, Aug 14, 2025 at 05:06:52PM +0100, James Clark wrote:
>>>> Document the two valid pincfg values and the defaults.
>>>>
>>>> Although the hardware supports two more values for half-duplex modes,
>>>> the driver doesn't support them so don't document them.
>>>
>>> binding doc should be first patch before drivers.
>>>
>>> binding descript hardware not driver, you should add all regardless if
>>> driver support it.
>>>
>>
>> Replied to same on "[PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for
>> S32G"
>>
>>>>
>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>> ---
>>>>    Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>>>> index ce7bd44ee17e..3f8833911807 100644
>>>> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>>>> @@ -70,6 +70,19 @@ properties:
>>>>      power-domains:
>>>>        maxItems: 1
>>>>
>>>> +  nxp,pincfg:
>>>> +    description:
>>>> +      'Pin configuration value for CFGR1.PINCFG.
>>>> +        - "sin-in-sout-out": SIN is used for input data and SOUT is used for
>>>> +                             output data
>>>> +        - "sout-in-sin-out": SOUT is used for input data and SIN is used for
>>>> +                             output data
>>>> +      If no value is specified then the default is "sin-in-sout-out" for host
>>>> +      mode and "sout-in-sin-out" for target mode.'
>>>
>>> why need this? are there varible at difference boards? look like default
>>> is more make sense.
>>>
>>
>> + Larissa. I think this might also be a question for the hardware designers
>> about why the feature to swap the pins was deemed worth including.
>>
>> I'm assuming the flexibility is given for routing reasons. If you have
>> another device with the pins in one order then you can route it without a
>> via if they happen to be in the same order.
> 
> DT team need reason to judge if a new property is reasonable/neccesary. You
> need  mention the reason why need this property. Here, some board design
> swap sin/sout.
> 
>>
>>> SPI signal name is MOSI and MISO
>>>
>>> Frank
>>>
>>
>> As mentioned in the commit message of "[PATCH 05/13] spi: spi-fsl-lpspi:
>> Enumerate all pin configuration definitions" the names were taken directly
>> from the reference manual and this doc text was too. I think diverging from
>> CFGR1_PINCFG could be potentially quite confusing. And MOSI isn't mentioned
>> once in S32G3RM rev 4:
>>
>>    Configures which pins are used for input and output data during serial
>>    transfers. When performing parallel transfers, the Pin Configuration
>>    field is ignored.
>>
>>      00b - SIN is used for input data and SOUT is used for output data
>>      01b - SIN is used for both input and output data, only half-duplex
>>            serial transfers are supported
>>      10b - SOUT is used for both input and output data, only half-duplex
>>            serial transfers are supported
>>      11b - SOUT is used for input data and SIN is used for output data
> 
> dt binding is ABI, design user easy understand property string.  like
> 
> enum:
>    - normal
>    - swap
>    - half-duplex-on-sin
>    - half-duplex-on-sout
> 
> Frank
> 

If we're not directly using the names that get programmed into the 
register then it's better to remove the implicit 5th mode that you get 
for leaving it blank and to use that as "normal" instead. Then "swap" is 
to swap whatever "normal" would have picked. Otherwise "normal" being a 
fixed value doesn't match up to the current "normal" behavior which is 
actually different value depending on host or target mode.

So it would look like this with the "if no value is specified..." bit 
reworded too:

    description:
       'Pin configuration value for CFGR1.PINCFG.
         - "normal": Hosts - SIN is used for input data and SOUT is used
                       for output data.
                     Targets - SOUT is used for input data and SIN is 

                       used for output data.
         - "swap": The inverse of "normal"
         - "half-duplex-on-sin": SIN is used for both input and output
                   data. Unsupported.
         - "half-duplex-on-sout": SOUT is used for both input and output
                   data. Unsupported.
       If no value is specified then the default is "normal".

>>
>> James
>>
>>>> +    enum:
>>>> +      - sin-in-sout-out
>>>> +      - sout-in-sin-out
>>>> +
>>>>    required:
>>>>      - compatible
>>>>      - reg
>>>> @@ -95,4 +108,5 @@ examples:
>>>>            spi-slave;
>>>>            fsl,spi-only-use-cs1-sel;
>>>>            num-cs = <2>;
>>>> +        nxp,pincfg = "sout-in-sin-out";
>>>>        };
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>

Re: [PATCH 12/13] dt-bindings: lpspi: Document nxp,lpspi-pincfg property

[James Clark]

On 18/08/2025 4:39 pm, Frank Li wrote:
> On Mon, Aug 18, 2025 at 03:47:45PM +0100, James Clark wrote:
>>
>>
>> On 14/08/2025 7:19 pm, Frank Li wrote:
>>> On Thu, Aug 14, 2025 at 05:06:52PM +0100, James Clark wrote:
>>>> Document the two valid pincfg values and the defaults.
>>>>
>>>> Although the hardware supports two more values for half-duplex modes,
>>>> the driver doesn't support them so don't document them.
>>>
>>> binding doc should be first patch before drivers.
>>>
>>> binding descript hardware not driver, you should add all regardless if
>>> driver support it.
>>>
>>
>> Replied to same on "[PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for
>> S32G"
>>
>>>>
>>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>>> ---
>>>>    Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>>>> index ce7bd44ee17e..3f8833911807 100644
>>>> --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
>>>> @@ -70,6 +70,19 @@ properties:
>>>>      power-domains:
>>>>        maxItems: 1
>>>>
>>>> +  nxp,pincfg:
>>>> +    description:
>>>> +      'Pin configuration value for CFGR1.PINCFG.
>>>> +        - "sin-in-sout-out": SIN is used for input data and SOUT is used for
>>>> +                             output data
>>>> +        - "sout-in-sin-out": SOUT is used for input data and SIN is used for
>>>> +                             output data
>>>> +      If no value is specified then the default is "sin-in-sout-out" for host
>>>> +      mode and "sout-in-sin-out" for target mode.'
>>>
>>> why need this? are there varible at difference boards? look like default
>>> is more make sense.
>>>
>>
>> + Larissa. I think this might also be a question for the hardware designers
>> about why the feature to swap the pins was deemed worth including.
>>
>> I'm assuming the flexibility is given for routing reasons. If you have
>> another device with the pins in one order then you can route it without a
>> via if they happen to be in the same order.
> 
> DT team need reason to judge if a new property is reasonable/neccesary. You
> need  mention the reason why need this property. Here, some board design
> swap sin/sout.
> 

Let's wait for Larisa to reply. If there's no board and it was only for 
testing maybe we can drop it.

Re: [PATCH 12/13] dt-bindings: lpspi: Document nxp,lpspi-pincfg property

[Frank Li]

On Tue, Aug 19, 2025 at 10:51:03AM +0100, James Clark wrote:
>
>
> On 18/08/2025 4:39 pm, Frank Li wrote:
> > On Mon, Aug 18, 2025 at 03:47:45PM +0100, James Clark wrote:
> > >
> > >
> > > On 14/08/2025 7:19 pm, Frank Li wrote:
> > > > On Thu, Aug 14, 2025 at 05:06:52PM +0100, James Clark wrote:
> > > > > Document the two valid pincfg values and the defaults.
> > > > >
> > > > > Although the hardware supports two more values for half-duplex modes,
> > > > > the driver doesn't support them so don't document them.
> > > >
> > > > binding doc should be first patch before drivers.
> > > >
> > > > binding descript hardware not driver, you should add all regardless if
> > > > driver support it.
> > > >
> > >
> > > Replied to same on "[PATCH 10/13] spi: spi-fsl-lpspi: Add compatible for
> > > S32G"
> > >
> > > > >
> > > > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > > > ---
> > > > >    Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml | 14 ++++++++++++++
> > > > >    1 file changed, 14 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> > > > > index ce7bd44ee17e..3f8833911807 100644
> > > > > --- a/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> > > > > +++ b/Documentation/devicetree/bindings/spi/spi-fsl-lpspi.yaml
> > > > > @@ -70,6 +70,19 @@ properties:
> > > > >      power-domains:
> > > > >        maxItems: 1
> > > > >
> > > > > +  nxp,pincfg:
> > > > > +    description:
> > > > > +      'Pin configuration value for CFGR1.PINCFG.
> > > > > +        - "sin-in-sout-out": SIN is used for input data and SOUT is used for
> > > > > +                             output data
> > > > > +        - "sout-in-sin-out": SOUT is used for input data and SIN is used for
> > > > > +                             output data
> > > > > +      If no value is specified then the default is "sin-in-sout-out" for host
> > > > > +      mode and "sout-in-sin-out" for target mode.'
> > > >
> > > > why need this? are there varible at difference boards? look like default
> > > > is more make sense.
> > > >
> > >
> > > + Larissa. I think this might also be a question for the hardware designers
> > > about why the feature to swap the pins was deemed worth including.
> > >
> > > I'm assuming the flexibility is given for routing reasons. If you have
> > > another device with the pins in one order then you can route it without a
> > > via if they happen to be in the same order.
> >
> > DT team need reason to judge if a new property is reasonable/neccesary. You
> > need  mention the reason why need this property. Here, some board design
> > swap sin/sout.
> >
> > >
> > > > SPI signal name is MOSI and MISO
> > > >
> > > > Frank
> > > >
> > >
> > > As mentioned in the commit message of "[PATCH 05/13] spi: spi-fsl-lpspi:
> > > Enumerate all pin configuration definitions" the names were taken directly
> > > from the reference manual and this doc text was too. I think diverging from
> > > CFGR1_PINCFG could be potentially quite confusing. And MOSI isn't mentioned
> > > once in S32G3RM rev 4:
> > >
> > >    Configures which pins are used for input and output data during serial
> > >    transfers. When performing parallel transfers, the Pin Configuration
> > >    field is ignored.
> > >
> > >      00b - SIN is used for input data and SOUT is used for output data
> > >      01b - SIN is used for both input and output data, only half-duplex
> > >            serial transfers are supported
> > >      10b - SOUT is used for both input and output data, only half-duplex
> > >            serial transfers are supported
> > >      11b - SOUT is used for input data and SIN is used for output data
> >
> > dt binding is ABI, design user easy understand property string.  like
> >
> > enum:
> >    - normal
> >    - swap
> >    - half-duplex-on-sin
> >    - half-duplex-on-sout
> >
> > Frank
> >
>
> If we're not directly using the names that get programmed into the register
> then it's better to remove the implicit 5th mode that you get for leaving it
> blank and to use that as "normal" instead.

make sense.

Frank
> Then "swap" is to swap whatever
> "normal" would have picked. Otherwise "normal" being a fixed value doesn't
> match up to the current "normal" behavior which is actually different value
> depending on host or target mode.
>
> So it would look like this with the "if no value is specified..." bit
> reworded too:
>
>    description:
>       'Pin configuration value for CFGR1.PINCFG.
>         - "normal": Hosts - SIN is used for input data and SOUT is used
>                       for output data.
>                     Targets - SOUT is used for input data and SIN is
>
>                       used for output data.
>         - "swap": The inverse of "normal"
>         - "half-duplex-on-sin": SIN is used for both input and output
>                   data. Unsupported.
>         - "half-duplex-on-sout": SOUT is used for both input and output
>                   data. Unsupported.
>       If no value is specified then the default is "normal".
>
> > >
> > > James
> > >
> > > > > +    enum:
> > > > > +      - sin-in-sout-out
> > > > > +      - sout-in-sin-out
> > > > > +
> > > > >    required:
> > > > >      - compatible
> > > > >      - reg
> > > > > @@ -95,4 +108,5 @@ examples:
> > > > >            spi-slave;
> > > > >            fsl,spi-only-use-cs1-sel;
> > > > >            num-cs = <2>;
> > > > > +        nxp,pincfg = "sout-in-sin-out";
> > > > >        };
> > > > >
> > > > > --
> > > > > 2.34.1
> > > > >
> > >
>

Re: [PATCH 02/13] spi: spi-fsl-lpspi: Set correct chip-select polarity bit

[Frank Li]

On Tue, Aug 19, 2025 at 09:21:08AM +0100, James Clark wrote:
>
>
> On 18/08/2025 4:19 pm, Frank Li wrote:
> > On Mon, Aug 18, 2025 at 02:05:16PM +0100, James Clark wrote:
> > >
> > >
> > > On 14/08/2025 5:49 pm, Frank Li wrote:
> > > > On Thu, Aug 14, 2025 at 05:06:42PM +0100, James Clark wrote:
> > > > > From: Larisa Grigore <larisa.grigore@nxp.com>
> > > > >
> > > > > The driver currently supports multiple chip-selects, but only sets the
> > > > > polarity for the first one (CS 0). Fix it by setting the PCSPOL bit for
> > > > > the desired chip-select.
> > > > >
> > > > > Fixes: 5314987de5e5 ("spi: imx: add lpspi bus driver")
> > > > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> > > > > Signed-off-by: James Clark <james.clark@linaro.org>
> > > > > ---
> > > > >    drivers/spi/spi-fsl-lpspi.c | 6 ++++--
> > > > >    1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/spi/spi-fsl-lpspi.c b/drivers/spi/spi-fsl-lpspi.c
> > > > > index d44a23f7d6c1..c65eb6d31ee7 100644
> > > > > --- a/drivers/spi/spi-fsl-lpspi.c
> > > > > +++ b/drivers/spi/spi-fsl-lpspi.c
> > > > > @@ -70,7 +70,7 @@
> > > > >    #define DER_TDDE	BIT(0)
> > > > >    #define CFGR1_PCSCFG	BIT(27)
> > > > >    #define CFGR1_PINCFG	(BIT(24)|BIT(25))
> > > > > -#define CFGR1_PCSPOL	BIT(8)
> > > > > +#define CFGR1_PCSPOL_MASK	GENMASK(11, 8)
> > > > >    #define CFGR1_NOSTALL	BIT(3)
> > > > >    #define CFGR1_HOST	BIT(0)
> > > > >    #define FSR_TXCOUNT	(0xFF)
> > > > > @@ -425,7 +425,9 @@ static int fsl_lpspi_config(struct fsl_lpspi_data *fsl_lpspi)
> > > > >    	else
> > > > >    		temp = CFGR1_PINCFG;
> > > > >    	if (fsl_lpspi->config.mode & SPI_CS_HIGH)
> > > > > -		temp |= CFGR1_PCSPOL;
> > > > > +		temp |= FIELD_PREP(CFGR1_PCSPOL_MASK,
> > > > > +				   BIT(fsl_lpspi->config.chip_select));
> > > > > +
> > > >
> > > > Feel like FILED_PREP(..., BIT()) is stranged.
> > > >
> > > > I suggest #define CFGR1_PCSPOL(x) BIT((x) + 8)
> > > >
> > > > Frank
> > >
> > > It's using an existing macro that everyone knows though and I found 65
> > > instances of exactly this. It can be read as "set bit X and put it into the
> > > PCSPOL field without any further investigation.
> >
> > Where have such pattern in kernel?
> >
> > Frank
> >
>
> Grep "FIELD_PREP\(.*,\n?.*BIT\("
>
> 65 results, e.g:
>
>   return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));

Thanks.

Frank
>
> James
>
> >
> > > If we make a new macro, first the reader will have to jump to it, then it
> > > still doesn't immediately explain what the "+ 8" part is. Using FIELD_PREP()
> > > also has the potential to use autogenerated field masks from a machine
> > > readable version of the reference manual. You can't statically check your
> > > macro to see if + 8 is correct or not, and it also doesn't catch overflow
> > > errors like FIELD_PREP() does.
> > >
> > > There might be an argument to add a new global macro like FIELD_BIT(mask,
> > > bit). But it's not very flexible (can't set multiple bits) and you can
> > > already accomplish the same thing by adding BIT() to the existing one.
> > >
> > > Thanks
> > > James
> > >
> > > >
> > > > >    	writel(temp, fsl_lpspi->base + IMX7ULP_CFGR1);
> > > > >
> > > > >    	temp = readl(fsl_lpspi->base + IMX7ULP_CR);
> > > > >
> > > > > --
> > > > > 2.34.1
> > > > >
> > >
>