linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Add multi mode support for omap-mcspi
@ 2024-02-06 10:00 Louis Chauvet
  2024-02-06 10:00 ` [PATCH 1/2] Revert "spi: spi-omap2-mcspi.c: Toggle CS after each word" Louis Chauvet
  2024-02-06 10:00 ` [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode Louis Chauvet
  0 siblings, 2 replies; 9+ messages in thread
From: Louis Chauvet @ 2024-02-06 10:00 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-spi, linux-kernel, thomas.petazzoni, Miquel Raynal,
	yen-mei.goh, koon-kee.lie, Louis Chauvet

This series add the support for the omap-mcspi multi mode. It allow 
sending SPI message 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 easly workaround this 
limitation.

The first patch removes the current implementation, which is working, but 
don't respect 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 respects 
what is asked in the SPI message.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
Louis Chauvet (2):
      Revert "spi: spi-omap2-mcspi.c: Toggle CS after each word"
      spi: omap2-mcspi: Add support for MULTI-mode

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

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] Revert "spi: spi-omap2-mcspi.c: Toggle CS after each word"
  2024-02-06 10:00 [PATCH 0/2] Add multi mode support for omap-mcspi Louis Chauvet
@ 2024-02-06 10:00 ` Louis Chauvet
  2024-02-06 10:35   ` Mark Brown
  2024-02-06 10:00 ` [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode Louis Chauvet
  1 sibling, 1 reply; 9+ messages in thread
From: Louis Chauvet @ 2024-02-06 10:00 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-spi, linux-kernel, thomas.petazzoni, Miquel Raynal,
	yen-mei.goh, koon-kee.lie, Louis Chauvet

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

The implementation does not respect what is indicated in the spi_message
structure, 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode
  2024-02-06 10:00 [PATCH 0/2] Add multi mode support for omap-mcspi Louis Chauvet
  2024-02-06 10:00 ` [PATCH 1/2] Revert "spi: spi-omap2-mcspi.c: Toggle CS after each word" Louis Chauvet
@ 2024-02-06 10:00 ` Louis Chauvet
  2024-02-06 10:56   ` Mark Brown
                     ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: Louis Chauvet @ 2024-02-06 10:00 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-spi, linux-kernel, thomas.petazzoni, Miquel Raynal,
	yen-mei.goh, koon-kee.lie, 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 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 | 57 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index fc7f69973334..ab22b1b062f3 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -133,6 +133,8 @@ 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 +260,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 +292,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 +1383,48 @@ 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;
+	u8 bits_per_word;
+	u32 speed_hz;
 
-	/* Only a single channel can have the FORCE bit enabled
+	/*
+	 * 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;
+
+		/* Check if the transfer content is only one word */
+		if ((bits_per_word < 8 && tr->len > 1) ||
+		    (bits_per_word >= 8 && tr->len > bits_per_word / 8))
+			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 */
+		if (!mcspi->use_multi_mode)
+			break;
+	}
+
+	omap2_mcspi_set_mode(master);
+
+	/* 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] Revert "spi: spi-omap2-mcspi.c: Toggle CS after each word"
  2024-02-06 10:00 ` [PATCH 1/2] Revert "spi: spi-omap2-mcspi.c: Toggle CS after each word" Louis Chauvet
@ 2024-02-06 10:35   ` Mark Brown
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2024-02-06 10:35 UTC (permalink / raw)
  To: Louis Chauvet
  Cc: linux-spi, linux-kernel, thomas.petazzoni, Miquel Raynal,
	yen-mei.goh, koon-kee.lie

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

On Tue, Feb 06, 2024 at 11:00:49AM +0100, Louis Chauvet wrote:

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

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode
  2024-02-06 10:00 ` [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode Louis Chauvet
@ 2024-02-06 10:56   ` Mark Brown
  2024-02-07 15:25     ` Louis Chauvet
  2024-02-07  1:54   ` kernel test robot
  2024-02-07  9:19   ` kernel test robot
  2 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2024-02-06 10:56 UTC (permalink / raw)
  To: Louis Chauvet
  Cc: linux-spi, linux-kernel, thomas.petazzoni, Miquel Raynal,
	yen-mei.goh, koon-kee.lie

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

On Tue, Feb 06, 2024 at 11:00:50AM +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).

I have no idea based on this commit message what either of these modes
is or how this shorter delay would be achieved, these terms are specific
to the OMAP hardware AFAICT.  Please clarify this, it's hard to follow
what the change does.  It looks like this is just a CS per word thing?

Note that you may not have to tell the hardware the same word length as
the transfer specifies, so long as the wire result is the same it
doesn't matter.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode
  2024-02-06 10:00 ` [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode Louis Chauvet
  2024-02-06 10:56   ` Mark Brown
@ 2024-02-07  1:54   ` kernel test robot
  2024-02-07  9:19   ` kernel test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-02-07  1:54 UTC (permalink / raw)
  To: Louis Chauvet, Mark Brown
  Cc: llvm, oe-kbuild-all, linux-spi, linux-kernel, thomas.petazzoni,
	Miquel Raynal, yen-mei.goh, koon-kee.lie, Louis Chauvet

Hi Louis,

kernel test robot noticed the following build errors:

[auto build test ERROR on 41bccc98fb7931d63d03f326a746ac4d429c1dd3]

url:    https://github.com/intel-lab-lkp/linux/commits/Louis-Chauvet/Revert-spi-spi-omap2-mcspi-c-Toggle-CS-after-each-word/20240206-180243
base:   41bccc98fb7931d63d03f326a746ac4d429c1dd3
patch link:    https://lore.kernel.org/r/20240126-spi-omap2-mcspi-multi-mode-v1-2-d143d33f0fe0%40bootlin.com
patch subject: [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode
config: i386-buildonly-randconfig-003-20240207 (https://download.01.org/0day-ci/archive/20240207/202402070919.C08LpTkR-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240207/202402070919.C08LpTkR-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/202402070919.C08LpTkR-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/spi/spi-omap2-mcspi.c:1415:23: error: use of undeclared identifier 'master'
    1415 |         omap2_mcspi_set_mode(master);
         |                              ^
   1 error generated.


vim +/master +1415 drivers/spi/spi-omap2-mcspi.c

  1379	
  1380	static int omap2_mcspi_prepare_message(struct spi_controller *ctlr,
  1381					       struct spi_message *msg)
  1382	{
  1383		struct omap2_mcspi	*mcspi = spi_controller_get_devdata(ctlr);
  1384		struct omap2_mcspi_regs	*ctx = &mcspi->ctx;
  1385		struct omap2_mcspi_cs	*cs;
  1386		struct spi_transfer	*tr;
  1387		u8 bits_per_word;
  1388		u32 speed_hz;
  1389	
  1390		/*
  1391		 * The conditions are strict, it is mandatory to check each transfer of the list to see if
  1392		 * multi-mode is applicable.
  1393		 */
  1394		mcspi->use_multi_mode = true;
  1395		list_for_each_entry(tr, &msg->transfers, transfer_list) {
  1396			if (!tr->bits_per_word)
  1397				bits_per_word = msg->spi->bits_per_word;
  1398			else
  1399				bits_per_word = tr->bits_per_word;
  1400	
  1401			/* Check if the transfer content is only one word */
  1402			if ((bits_per_word < 8 && tr->len > 1) ||
  1403			    (bits_per_word >= 8 && tr->len > bits_per_word / 8))
  1404				mcspi->use_multi_mode = false;
  1405	
  1406			/* Check if transfer asks to change the CS status after the transfer */
  1407			if (!tr->cs_change)
  1408				mcspi->use_multi_mode = false;
  1409	
  1410			/* If at least one message is not compatible, switch back to single mode */
  1411			if (!mcspi->use_multi_mode)
  1412				break;
  1413		}
  1414	
> 1415		omap2_mcspi_set_mode(master);
  1416	
  1417		/* In single mode only a single channel can have the FORCE bit enabled
  1418		 * in its chconf0 register.
  1419		 * Scan all channels and disable them except the current one.
  1420		 * A FORCE can remain from a last transfer having cs_change enabled
  1421		 *
  1422		 * In multi mode all FORCE bits must be disabled.
  1423		 */
  1424		list_for_each_entry(cs, &ctx->cs, node) {
  1425			if (msg->spi->controller_state == cs && !mcspi->use_multi_mode) {
  1426				continue;
  1427			}
  1428	
  1429			if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE)) {
  1430				cs->chconf0 &= ~OMAP2_MCSPI_CHCONF_FORCE;
  1431				writel_relaxed(cs->chconf0,
  1432						cs->base + OMAP2_MCSPI_CHCONF0);
  1433				readl_relaxed(cs->base + OMAP2_MCSPI_CHCONF0);
  1434			}
  1435		}
  1436	
  1437		return 0;
  1438	}
  1439	

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode
  2024-02-06 10:00 ` [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode Louis Chauvet
  2024-02-06 10:56   ` Mark Brown
  2024-02-07  1:54   ` kernel test robot
@ 2024-02-07  9:19   ` kernel test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-02-07  9:19 UTC (permalink / raw)
  To: Louis Chauvet, Mark Brown
  Cc: oe-kbuild-all, linux-spi, linux-kernel, thomas.petazzoni,
	Miquel Raynal, yen-mei.goh, koon-kee.lie, Louis Chauvet

Hi Louis,

kernel test robot noticed the following build errors:

[auto build test ERROR on 41bccc98fb7931d63d03f326a746ac4d429c1dd3]

url:    https://github.com/intel-lab-lkp/linux/commits/Louis-Chauvet/Revert-spi-spi-omap2-mcspi-c-Toggle-CS-after-each-word/20240206-180243
base:   41bccc98fb7931d63d03f326a746ac4d429c1dd3
patch link:    https://lore.kernel.org/r/20240126-spi-omap2-mcspi-multi-mode-v1-2-d143d33f0fe0%40bootlin.com
patch subject: [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240207/202402071719.e1p4tUge-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240207/202402071719.e1p4tUge-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/202402071719.e1p4tUge-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/spi/spi-omap2-mcspi.c: In function 'omap2_mcspi_prepare_message':
>> drivers/spi/spi-omap2-mcspi.c:1415:30: error: 'master' undeclared (first use in this function)
    1415 |         omap2_mcspi_set_mode(master);
         |                              ^~~~~~
   drivers/spi/spi-omap2-mcspi.c:1415:30: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/spi/spi-omap2-mcspi.c:1388:13: warning: unused variable 'speed_hz' [-Wunused-variable]
    1388 |         u32 speed_hz;
         |             ^~~~~~~~


vim +/master +1415 drivers/spi/spi-omap2-mcspi.c

  1379	
  1380	static int omap2_mcspi_prepare_message(struct spi_controller *ctlr,
  1381					       struct spi_message *msg)
  1382	{
  1383		struct omap2_mcspi	*mcspi = spi_controller_get_devdata(ctlr);
  1384		struct omap2_mcspi_regs	*ctx = &mcspi->ctx;
  1385		struct omap2_mcspi_cs	*cs;
  1386		struct spi_transfer	*tr;
  1387		u8 bits_per_word;
> 1388		u32 speed_hz;
  1389	
  1390		/*
  1391		 * The conditions are strict, it is mandatory to check each transfer of the list to see if
  1392		 * multi-mode is applicable.
  1393		 */
  1394		mcspi->use_multi_mode = true;
  1395		list_for_each_entry(tr, &msg->transfers, transfer_list) {
  1396			if (!tr->bits_per_word)
  1397				bits_per_word = msg->spi->bits_per_word;
  1398			else
  1399				bits_per_word = tr->bits_per_word;
  1400	
  1401			/* Check if the transfer content is only one word */
  1402			if ((bits_per_word < 8 && tr->len > 1) ||
  1403			    (bits_per_word >= 8 && tr->len > bits_per_word / 8))
  1404				mcspi->use_multi_mode = false;
  1405	
  1406			/* Check if transfer asks to change the CS status after the transfer */
  1407			if (!tr->cs_change)
  1408				mcspi->use_multi_mode = false;
  1409	
  1410			/* If at least one message is not compatible, switch back to single mode */
  1411			if (!mcspi->use_multi_mode)
  1412				break;
  1413		}
  1414	
> 1415		omap2_mcspi_set_mode(master);
  1416	
  1417		/* In single mode only a single channel can have the FORCE bit enabled
  1418		 * in its chconf0 register.
  1419		 * Scan all channels and disable them except the current one.
  1420		 * A FORCE can remain from a last transfer having cs_change enabled
  1421		 *
  1422		 * In multi mode all FORCE bits must be disabled.
  1423		 */
  1424		list_for_each_entry(cs, &ctx->cs, node) {
  1425			if (msg->spi->controller_state == cs && !mcspi->use_multi_mode) {
  1426				continue;
  1427			}
  1428	
  1429			if ((cs->chconf0 & OMAP2_MCSPI_CHCONF_FORCE)) {
  1430				cs->chconf0 &= ~OMAP2_MCSPI_CHCONF_FORCE;
  1431				writel_relaxed(cs->chconf0,
  1432						cs->base + OMAP2_MCSPI_CHCONF0);
  1433				readl_relaxed(cs->base + OMAP2_MCSPI_CHCONF0);
  1434			}
  1435		}
  1436	
  1437		return 0;
  1438	}
  1439	

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode
  2024-02-06 10:56   ` Mark Brown
@ 2024-02-07 15:25     ` Louis Chauvet
  2024-02-07 15:46       ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Louis Chauvet @ 2024-02-07 15:25 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-spi, linux-kernel, thomas.petazzoni, Miquel Raynal,
	yen-mei.goh, koon-kee.lie

Le 06/02/24 - 10:56, Mark Brown a écrit :
> On Tue, Feb 06, 2024 at 11:00:50AM +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).
> 
> I have no idea based on this commit message what either of these modes
> is or how this shorter delay would be achieved, these terms are specific
> to the OMAP hardware AFAICT.  Please clarify this, it's hard to follow
> what the change does.  It looks like this is just a CS per word thing?

Indeed, you're right, the wording is probably very OMAP specific, I
didn't realize that earlier. I'll try to explain better. What about
this addition following the above paragraph, would it be clearer?

  [...] 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 [...]
 
> Note that you may not have to tell the hardware the same word length as
> the transfer specifies, so long as the wire result is the same it
> doesn't matter.

If I understand correclty what you want is: given a message, containing 2
transfers of 4 bits, with cs_change disabled, use the multi mode and send 
only one 8 bits transfer instead of two 4 bits transfer?

This seems very complex to implement, and will only benefit in very 
niche cases.
If I have to add this, I have to:
- detect the very particular pattern "message of multiple transfer and 
those transfer can be packed in bigger transfer"
- reimplement the transfer_one_message method to merge multiple transfer 
into one;
- manage the rx buffer to "unmerge" the answer;
- take care of timings if requested;
- probably other issues I don't see

I agree this kind of optimisation can be nice, and may benefit for this
spi controller, but I don't have the time to work on it. If someone 
want to do it, it could be a nice improvement.

I just see that I miss my rebase, I will push a v2. This v2 will also 
change the commit name for patch 1/2.

Have a nice day,
Louis Chauvet

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode
  2024-02-07 15:25     ` Louis Chauvet
@ 2024-02-07 15:46       ` Mark Brown
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2024-02-07 15:46 UTC (permalink / raw)
  To: linux-spi, linux-kernel, thomas.petazzoni, Miquel Raynal,
	yen-mei.goh, koon-kee.lie

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

On Wed, Feb 07, 2024 at 04:25:16PM +0100, Louis Chauvet wrote:
> Le 06/02/24 - 10:56, Mark Brown a écrit :

> this addition following the above paragraph, would it be clearer?
> 
>   [...] 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 [...]

Yes.

> > Note that you may not have to tell the hardware the same word length as
> > the transfer specifies, so long as the wire result is the same it
> > doesn't matter.

> If I understand correclty what you want is: given a message, containing 2
> transfers of 4 bits, with cs_change disabled, use the multi mode and send 
> only one 8 bits transfer instead of two 4 bits transfer?

> This seems very complex to implement, and will only benefit in very 
> niche cases.

I was hoping that the hardware supports more than 8 bit words, in that
case then it gets useful for common operations like 8 bit register 8 bit
data register writes (and more for larger word sizes) which are
relatively simple.  If it's just 8 bit words then yes, totally not worth
the effort.

> If I have to add this, I have to:
> - detect the very particular pattern "message of multiple transfer and 
> those transfer can be packed in bigger transfer"

Or just a single transfer with two words, it's trivial cases that don't
involve rewriting anything beyond lying about the word lengths that I
was thinking of.  Anything more involved should go in the core.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-02-07 15:46 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-06 10:00 [PATCH 0/2] Add multi mode support for omap-mcspi Louis Chauvet
2024-02-06 10:00 ` [PATCH 1/2] Revert "spi: spi-omap2-mcspi.c: Toggle CS after each word" Louis Chauvet
2024-02-06 10:35   ` Mark Brown
2024-02-06 10:00 ` [PATCH 2/2] spi: omap2-mcspi: Add support for MULTI-mode Louis Chauvet
2024-02-06 10:56   ` Mark Brown
2024-02-07 15:25     ` Louis Chauvet
2024-02-07 15:46       ` Mark Brown
2024-02-07  1:54   ` kernel test robot
2024-02-07  9:19   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).