| Message ID | 20240126-spi-omap2-mcspi-multi-mode-v1-2-d143d33f0fe0@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add multi mode support for omap-mcspi | expand |
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.
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
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
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
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(-)