| Message ID | 20240126-spi-omap2-mcspi-multi-mode-v1-2-d143d33f0fe0@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] Revert "spi: spi-omap2-mcspi.c: Toggle CS after each word" | expand |
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
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.
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;
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(-)