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

  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(-)
  

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.