[PATCH v2 0/3] Add multi mode support for omap-mcspi

[PATCH v2 0/3] Add multi mode support for omap-mcspi

[Louis Chauvet]

This series adds the support for the omap-mcspi multi mode which allows
sending SPI messages with a shorter delay between CS and the message.

One drawback of the multi-mode is that the CS is raised between each word, 
so it can only be used with messages containing 1 word transfers and 
asking for cs_change. Few devices, like FPGAs, may easily workaround this 
limitation.

The first patch removes the current implementation, which is working, but 
don't comply with what is asked in the spi transfer (The CS is raised by 
the hardware regardless of cs_change state). No drivers or board file use this 
implementation upstream.

The second patch adds the implementation of the multi-mode, which complies 
with what is asked in the SPI message.

The third patch is the suggested optimization for using MULTI mode in more 
situations.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
Changes in v2:
- Updated the commit line for the first patch to use the correct format;
- Updated the commit message for the second patch, adding precision on how
  the controler works;
- Added the suggestion from Mark Brown to merge multiple transfers word 
  into one when applicable;
- Link to v1: https://lore.kernel.org/r/20240126-spi-omap2-mcspi-multi-mode-v1-0-d143d33f0fe0@bootlin.com

---
Louis Chauvet (3):
      spi: spi-omap2-mcspi.c: revert "Toggle CS after each word"
      spi: omap2-mcspi: Add support for MULTI-mode
      spi: omap2-mcpsi: Enable MULTI-mode in more situations

 drivers/spi/spi-omap2-mcspi.c                 | 96 +++++++++++++++++++++------
 include/linux/platform_data/spi-omap2-mcspi.h |  3 -
 2 files changed, 75 insertions(+), 24 deletions(-)
---
base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
change-id: 20240126-spi-omap2-mcspi-multi-mode-e62f68b78ad3

Best regards,
-- 
Louis Chauvet <louis.chauvet@bootlin.com>

[PATCH v2 1/3] spi: spi-omap2-mcspi.c: revert "Toggle CS after each word"

[Louis Chauvet]

Commit 5cbc7ca987fb ("spi: spi-omap2-mcspi.c: Toggle CS after each
word") introduced the toggling of CS after each word for the omap2-mcspi
controller.

The implementation is not respectful of the actual spi_message
content, so the CS can be raised after each word even if the
transfer structure asks to keep the CS active for the whole operation.

As it is not used anyway in the current Linux tree, it can be safely
removed.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/spi/spi-omap2-mcspi.c                 | 15 ---------------
 include/linux/platform_data/spi-omap2-mcspi.h |  3 ---
 2 files changed, 18 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index a0c9fea908f5..fc7f69973334 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -1292,13 +1292,6 @@ static int omap2_mcspi_transfer_one(struct spi_controller *ctlr,
 		    t->bits_per_word == spi->bits_per_word)
 			par_override = 0;
 	}
-	if (cd && cd->cs_per_word) {
-		chconf = mcspi->ctx.modulctrl;
-		chconf &= ~OMAP2_MCSPI_MODULCTRL_SINGLE;
-		mcspi_write_reg(ctlr, OMAP2_MCSPI_MODULCTRL, chconf);
-		mcspi->ctx.modulctrl =
-			mcspi_read_cs_reg(spi, OMAP2_MCSPI_MODULCTRL);
-	}
 
 	chconf = mcspi_cached_chconf0(spi);
 	chconf &= ~OMAP2_MCSPI_CHCONF_TRM_MASK;
@@ -1361,14 +1354,6 @@ static int omap2_mcspi_transfer_one(struct spi_controller *ctlr,
 		status = omap2_mcspi_setup_transfer(spi, NULL);
 	}
 
-	if (cd && cd->cs_per_word) {
-		chconf = mcspi->ctx.modulctrl;
-		chconf |= OMAP2_MCSPI_MODULCTRL_SINGLE;
-		mcspi_write_reg(ctlr, OMAP2_MCSPI_MODULCTRL, chconf);
-		mcspi->ctx.modulctrl =
-			mcspi_read_cs_reg(spi, OMAP2_MCSPI_MODULCTRL);
-	}
-
 	omap2_mcspi_set_enable(spi, 0);
 
 	if (spi_get_csgpiod(spi, 0))
diff --git a/include/linux/platform_data/spi-omap2-mcspi.h b/include/linux/platform_data/spi-omap2-mcspi.h
index 3b400b1919a9..9e3c15b4ac91 100644
--- a/include/linux/platform_data/spi-omap2-mcspi.h
+++ b/include/linux/platform_data/spi-omap2-mcspi.h
@@ -16,9 +16,6 @@ struct omap2_mcspi_platform_config {
 
 struct omap2_mcspi_device_config {
 	unsigned turbo_mode:1;
-
-	/* toggle chip select after every word */
-	unsigned cs_per_word:1;
 };
 
 #endif

-- 
2.43.0

[PATCH v2 2/3] spi: omap2-mcspi: Add support for MULTI-mode

[Louis Chauvet]

Introduce support for MULTI-mode in the OMAP2 MCSPI driver. Currently, the
driver always uses SINGLE mode to handle the chip select (CS). With this
enhancement, MULTI-mode is enabled for specific messages, allowing for a
shorter delay between CS enable and the message (some FPGA devices are
sensitive to this delay).

The OMAP2 MCSPI device can use two different mode to send messages, SINGLE
and MULTI:
In SINGLE mode, the controller only leverages one single FIFO, and the
host system has to manually select the CS it wants to enable.
In MULTI mode, each CS is bound to a FIFO, the host system then writes the
data to the relevant FIFO, as the hardware will take care of the CS

The drawback of multi-mode is that it's not possible to keep the CS
enabled between each words. Therefore, this patch enables multi-mode only
for specific messages: the spi_message must contain only spi_transfer of 1
word (of any size) with cs_change enabled.

A new member is introduced in the omap2_mcspi structure to keep track of
the current used mode.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/spi/spi-omap2-mcspi.c | 67 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 62 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index fc7f69973334..36075c4416d5 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -133,6 +133,7 @@ struct omap2_mcspi {
 	unsigned int		pin_dir:1;
 	size_t			max_xfer_len;
 	u32			ref_clk_hz;
+	bool			use_multi_mode;
 };
 
 struct omap2_mcspi_cs {
@@ -258,10 +259,15 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
 
 		l = mcspi_cached_chconf0(spi);
 
-		if (enable)
+		/* Only enable chip select manually if single mode is used */
+		if (mcspi->use_multi_mode) {
 			l &= ~OMAP2_MCSPI_CHCONF_FORCE;
-		else
-			l |= OMAP2_MCSPI_CHCONF_FORCE;
+		} else {
+			if (enable)
+				l &= ~OMAP2_MCSPI_CHCONF_FORCE;
+			else
+				l |= OMAP2_MCSPI_CHCONF_FORCE;
+		}
 
 		mcspi_write_chconf0(spi, l);
 
@@ -285,7 +291,12 @@ static void omap2_mcspi_set_mode(struct spi_controller *ctlr)
 		l |= (OMAP2_MCSPI_MODULCTRL_MS);
 	} else {
 		l &= ~(OMAP2_MCSPI_MODULCTRL_MS);
-		l |= OMAP2_MCSPI_MODULCTRL_SINGLE;
+
+		/* Enable single mode if needed */
+		if (mcspi->use_multi_mode)
+			l &= ~OMAP2_MCSPI_MODULCTRL_SINGLE;
+		else
+			l |= OMAP2_MCSPI_MODULCTRL_SINGLE;
 	}
 	mcspi_write_reg(ctlr, OMAP2_MCSPI_MODULCTRL, l);
 
@@ -1371,15 +1382,61 @@ static int omap2_mcspi_prepare_message(struct spi_controller *ctlr,
 	struct omap2_mcspi	*mcspi = spi_controller_get_devdata(ctlr);
 	struct omap2_mcspi_regs	*ctx = &mcspi->ctx;
 	struct omap2_mcspi_cs	*cs;
+	struct spi_transfer	*tr;
+	bool word_delay_is_zero;
+	u8 bits_per_word;
+
+	/*
+	 * The conditions are strict, it is mandatory to check each transfer of the list to see if
+	 * multi-mode is applicable.
+	 */
+	mcspi->use_multi_mode = true;
+	list_for_each_entry(tr, &msg->transfers, transfer_list) {
+		if (!tr->bits_per_word)
+			bits_per_word = msg->spi->bits_per_word;
+		else
+			bits_per_word = tr->bits_per_word;
 
 	/* Only a single channel can have the FORCE bit enabled
+		/*
+		 * Check if this transfer contains only one word;
+		 */
+		if (bits_per_word < 8 && tr->len == 1) {
+			/* multi-mode is applicable, only one word (1..7 bits) */
+		} else if (bits_per_word >= 8 && tr->len == bits_per_word / 8) {
+			/* multi-mode is applicable, only one word (8..32 bits) */
+		} else {
+			/* multi-mode is not applicable: more than one word in the transfer */
+			mcspi->use_multi_mode = false;
+		}
+
+		/* Check if transfer asks to change the CS status after the transfer */
+		if (!tr->cs_change)
+			mcspi->use_multi_mode = false;
+
+		/*
+		 * If at least one message is not compatible, switch back to single mode
+		 *
+		 * The bits_per_word of certain transfer can be different, but it will have no
+		 * impact on the signal itself.
+		 */
+		if (!mcspi->use_multi_mode)
+			break;
+	}
+
+	omap2_mcspi_set_mode(ctlr);
+
+	/* In single mode only a single channel can have the FORCE bit enabled
 	 * in its chconf0 register.
 	 * Scan all channels and disable them except the current one.
 	 * A FORCE can remain from a last transfer having cs_change enabled
+	 *
+	 * In multi mode all FORCE bits must be disabled.
 	 */
 	list_for_each_entry(cs, &ctx->cs, node) {
-		if (msg->spi->controller_state == cs)
+		if (msg->spi->controller_state == cs && !mcspi->use_multi_mode) {
 			continue;
+		}
 
 		if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE)) {
 			cs->chconf0 &= ~OMAP2_MCSPI_CHCONF_FORCE;

-- 
2.43.0

Re: [PATCH v2 2/3] spi: omap2-mcspi: Add support for MULTI-mode

[Mark Brown]

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

On Fri, Feb 23, 2024 at 10:32:12AM +0100, Louis Chauvet wrote:
> Introduce support for MULTI-mode in the OMAP2 MCSPI driver. Currently, the
> driver always uses SINGLE mode to handle the chip select (CS). With this
> enhancement, MULTI-mode is enabled for specific messages, allowing for a
> shorter delay between CS enable and the message (some FPGA devices are
> sensitive to this delay).

This breaks an allmodconfig build:

/build/stage/linux/drivers/spi/spi-omap2-mcspi.c: In function ‘omap2_mcspi_prepare_message’:
/build/stage/linux/drivers/spi/spi-omap2-mcspi.c:1280:17: error: "/*" within comment [-Werror=comment]
 1280 |                 /*
      |                  
/build/stage/linux/drivers/spi/spi-omap2-mcspi.c:1265:14: error: unused variable ‘word_delay_is_zero’ [-Werror=unused-variable]
 1265 |         bool word_delay_is_zero;
      |              ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

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

Re: [PATCH v2 2/3] spi: omap2-mcspi: Add support for MULTI-mode

[Louis Chauvet]

Le 25/03/24 - 14:05, Mark Brown a écrit :
> On Fri, Feb 23, 2024 at 10:32:12AM +0100, Louis Chauvet wrote:
> > Introduce support for MULTI-mode in the OMAP2 MCSPI driver. Currently, the
> > driver always uses SINGLE mode to handle the chip select (CS). With this
> > enhancement, MULTI-mode is enabled for specific messages, allowing for a
> > shorter delay between CS enable and the message (some FPGA devices are
> > sensitive to this delay).
> 
> This breaks an allmodconfig build:
> 
> /build/stage/linux/drivers/spi/spi-omap2-mcspi.c: In function ‘omap2_mcspi_prepare_message’:
> /build/stage/linux/drivers/spi/spi-omap2-mcspi.c:1280:17: error: "/*" within comment [-Werror=comment]
>  1280 |                 /*
>       |                  
> /build/stage/linux/drivers/spi/spi-omap2-mcspi.c:1265:14: error: unused variable ‘word_delay_is_zero’ [-Werror=unused-variable]
>  1265 |         bool word_delay_is_zero;
>       |              ^~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors

Hi Mark,

I missed this, sorry.

I've just sent a v3 [1] without those errors.

Thanks,
Louis Chauvet

[1]: https://lore.kernel.org/all/20240327-spi-omap2-mcspi-multi-mode-v3-0-c4ac329dd5a2@bootlin.com/

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 3/3] spi: omap2-mcpsi: Enable MULTI-mode in more situations

[Louis Chauvet]

Enable multimode when a transfer of multiple small words can be
transformed in a transfer with a single bigger word. This is allowed as
long as the result on the cable is the same, so word_delay must be zero.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

---

I am not sure if this is the best place to put such "optimization". I
think this improvment should be in the core, as it is not depending on the
driver itself, but I think Mark suggested something like this so if that
fits to what was expected, I am happy to share this small improvement.
---
 drivers/spi/spi-omap2-mcspi.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 36075c4416d5..f64cc8cc7587 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -1397,14 +1397,26 @@ static int omap2_mcspi_prepare_message(struct spi_controller *ctlr,
 		else
 			bits_per_word = tr->bits_per_word;
 
-	/* Only a single channel can have the FORCE bit enabled
 		/*
 		 * Check if this transfer contains only one word;
+		 * OR contains 1 to 4 words, with bits_per_word == 8 and no delay between each word
+		 * OR contains 1 to 2 words, with bits_per_word == 16 and no delay between each word
+		 *
+		 * If one of the two last case is true, this also change the bits_per_word of this
+		 * transfer to make it a bit faster.
+		 * It's not an issue to change the bits_per_word here even if the multi-mode is not
+		 * applicable for this message, the signal on the wire will be the same.
 		 */
 		if (bits_per_word < 8 && tr->len == 1) {
 			/* multi-mode is applicable, only one word (1..7 bits) */
+		} else if (tr->word_delay.value == 0 && bits_per_word == 8 && tr->len <= 4) {
+			/* multi-mode is applicable, only one "bigger" word (8,16,24,32 bits) */
+			tr->bits_per_word = tr->len * bits_per_word;
+		} else if (tr->word_delay.value == 0 && bits_per_word == 16 && tr->len <= 2) {
+			/* multi-mode is applicable, only one "bigger" word (16,32 bits) */
+			tr->bits_per_word = tr->len * bits_per_word / 2;
 		} else if (bits_per_word >= 8 && tr->len == bits_per_word / 8) {
-			/* multi-mode is applicable, only one word (8..32 bits) */
+			/* multi-mode is applicable, only one word (9..15,17..32 bits) */
 		} else {
 			/* multi-mode is not applicable: more than one word in the transfer */
 			mcspi->use_multi_mode = false;

-- 
2.43.0

Re: [PATCH v2 0/3] Add multi mode support for omap-mcspi

[Louis Chauvet]

Hello Mark,

Given how far we already are in the current cycle I suppose this series 
will only be considered after -rc1, will you want me to re-send or will 
it stay on your stack? I know some maintainers prefer contributors to 
re-send and others don't, so let met know what suits best your workflow.

Thanks!
Louis

Le 23/02/24 - 10:32, Louis Chauvet a écrit :
> This series adds the support for the omap-mcspi multi mode which allows
> sending SPI messages with a shorter delay between CS and the message.
> 
> One drawback of the multi-mode is that the CS is raised between each word, 
> so it can only be used with messages containing 1 word transfers and 
> asking for cs_change. Few devices, like FPGAs, may easily workaround this 
> limitation.
> 
> The first patch removes the current implementation, which is working, but 
> don't comply with what is asked in the spi transfer (The CS is raised by 
> the hardware regardless of cs_change state). No drivers or board file use this 
> implementation upstream.
> 
> The second patch adds the implementation of the multi-mode, which complies 
> with what is asked in the SPI message.
> 
> The third patch is the suggested optimization for using MULTI mode in more 
> situations.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> Changes in v2:
> - Updated the commit line for the first patch to use the correct format;
> - Updated the commit message for the second patch, adding precision on how
>   the controler works;
> - Added the suggestion from Mark Brown to merge multiple transfers word 
>   into one when applicable;
> - Link to v1: https://lore.kernel.org/r/20240126-spi-omap2-mcspi-multi-mode-v1-0-d143d33f0fe0@bootlin.com
> 
> ---
> Louis Chauvet (3):
>       spi: spi-omap2-mcspi.c: revert "Toggle CS after each word"
>       spi: omap2-mcspi: Add support for MULTI-mode
>       spi: omap2-mcpsi: Enable MULTI-mode in more situations
> 
>  drivers/spi/spi-omap2-mcspi.c                 | 96 +++++++++++++++++++++------
>  include/linux/platform_data/spi-omap2-mcspi.h |  3 -
>  2 files changed, 75 insertions(+), 24 deletions(-)
> ---
> base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
> change-id: 20240126-spi-omap2-mcspi-multi-mode-e62f68b78ad3
> 
> Best regards,
> -- 
> Louis Chauvet <louis.chauvet@bootlin.com>
> 
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 0/3] Add multi mode support for omap-mcspi

[Mark Brown]

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

On Thu, Mar 14, 2024 at 02:56:46PM +0100, Louis Chauvet wrote:
> Hello Mark,
> 
> Given how far we already are in the current cycle I suppose this series 
> will only be considered after -rc1, will you want me to re-send or will 
> it stay on your stack? I know some maintainers prefer contributors to 
> re-send and others don't, so let met know what suits best your workflow.

No need to resend.

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

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

Re: [PATCH v2 0/3] Add multi mode support for omap-mcspi

[Mark Brown]

On Fri, 23 Feb 2024 10:32:10 +0100, Louis Chauvet wrote:
> This series adds the support for the omap-mcspi multi mode which allows
> sending SPI messages with a shorter delay between CS and the message.
> 
> One drawback of the multi-mode is that the CS is raised between each word,
> so it can only be used with messages containing 1 word transfers and
> asking for cs_change. Few devices, like FPGAs, may easily workaround this
> limitation.
> 
> [...]

Applied to

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

Thanks!

[1/3] spi: spi-omap2-mcspi.c: revert "Toggle CS after each word"
      commit: 67bb37c05a6b56e0e1f804706145a52f655af3f1
[2/3] spi: omap2-mcspi: Add support for MULTI-mode
      commit: d153ff4056cb346fd6182a8a1bea6e12b714b64f
[3/3] spi: omap2-mcpsi: Enable MULTI-mode in more situations
      commit: e64d3b6fc9a388d7dc516668651cf4404bffec9b

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

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

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

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

Thanks,
Mark