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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240207/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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
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.