[PATCH v2 0/2] Add support for components requiring trailing clock after transfer

[PATCH v2 0/2] Add support for components requiring trailing clock after transfer

[Christophe Leroy]

Some components require a few clock cycles with chipselect off before
or/and after the data transfer done with CS on.

Typically IDT 801034 QUAD PCM CODEC datasheet states "Note *: CCLK
should have one cycle before CS goes low, and two cycles after
CS goes high".

The cycles "before" are implicitely provided by all previous activity
on the SPI bus. But the cycles "after" must be provided in order to
achieve the SPI transfer.

In order to use that kind of component, implement a new option for
SPI slaves in order to implement trailing clock of a given number of
bits with ChipSelect off at the end of the transfer.

This is based on a discussion we had a few years ago, see
https://lore.kernel.org/linux-spi/20160824112701.GE22076@sirena.org.uk/

IDT 801034 QUAD PCM CODEC datasheet can be found at
https://www.renesas.com/eu/en/document/dst/821034-data-sheet?language=en&r=24763

Changes in v2:
- Provide the number of wanted clock cycles instead of just a bool. 

Christophe Leroy (2):
  spi: Add optional number of additional clock cycles to be generated
  spi: fsl-spi: Implement trailing bits

 .../bindings/spi/spi-peripheral-props.yaml    |  5 +++++
 drivers/spi/spi-fsl-spi.c                     | 21 +++++++++++++++++--
 drivers/spi/spi.c                             |  7 +++++--
 include/linux/spi/spi.h                       |  1 +
 4 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] spi: Add optional number of additional clock cycles to be generated

[Christophe Leroy]

Some components require a few clock cycles with chipselect off before
or/and after the data transfer done with CS on.

Typically IDT 801034 QUAD PCM CODEC datasheet states "Note *: CCLK
should have one cycle before CS goes low, and two cycles after
CS goes high".

The cycles "before" are implicitely provided by all previous activity
on the SPI bus. But the cycles "after" must be provided in order to
achieve the SPI transfer.

In order to use that kind of component, implement a new option for
SPI slaves in order to implement trailing clock of a given number of
bits with ChipSelect off at the end of the transfer.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 .../devicetree/bindings/spi/spi-peripheral-props.yaml      | 5 +++++
 drivers/spi/spi.c                                          | 7 +++++--
 include/linux/spi/spi.h                                    | 1 +
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 5dd209206e88..2fd5a5084dbe 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -82,6 +82,11 @@ properties:
     description:
       Delay, in microseconds, after a write transfer.
 
+  spi-trailing-bits:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Number of clock cycles with chipselect OFF after transfers.
+
 # The controller specific properties go here.
 allOf:
   - $ref: cdns,qspi-nor-peripheral-props.yaml#
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4599b121d744..2b204fd20337 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2159,6 +2159,9 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 		}
 	}
 
+	if (!of_property_read_u32(nc, "spi-trailing-bits", &value))
+		spi->trailing_bits = value;
+
 	if (spi_controller_is_slave(ctlr)) {
 		if (!of_node_name_eq(nc, "slave")) {
 			dev_err(&ctlr->dev, "%pOF is not called 'slave'\n",
@@ -3538,14 +3541,14 @@ int spi_setup(struct spi_device *spi)
 
 	trace_spi_setup(spi, status);
 
-	dev_dbg(&spi->dev, "setup mode %lu, %s%s%s%s%u bits/w, %u Hz max --> %d\n",
+	dev_dbg(&spi->dev, "setup mode %lu, %s%s%s%s%u bits/w, %u Hz max, %d trailing bits --> %d\n",
 			spi->mode & SPI_MODE_X_MASK,
 			(spi->mode & SPI_CS_HIGH) ? "cs_high, " : "",
 			(spi->mode & SPI_LSB_FIRST) ? "lsb, " : "",
 			(spi->mode & SPI_3WIRE) ? "3wire, " : "",
 			(spi->mode & SPI_LOOP) ? "loopback, " : "",
 			spi->bits_per_word, spi->max_speed_hz,
-			status);
+			spi->trailing_bits, status);
 
 	return status;
 }
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 7ab3fed7b804..3f1e3d788f08 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -166,6 +166,7 @@ struct spi_device {
 	u32			max_speed_hz;
 	u8			chip_select;
 	u8			bits_per_word;
+	u8			trailing_bits;
 	bool			rt;
 #define SPI_NO_TX	BIT(31)		/* no transmit wire */
 #define SPI_NO_RX	BIT(30)		/* no receive wire */
-- 
2.34.1

[PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Christophe Leroy]

In order to support IDT 801034 QUAD PCM CODEC, implement
trailing clock for the amount of requested bits.

On fsl SPI, the minimum we can implement is a 4 bits shot.
If the requested number is greated than 8, use 16.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 drivers/spi/spi-fsl-spi.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
index bdf94cc7be1a..aefaea439672 100644
--- a/drivers/spi/spi-fsl-spi.c
+++ b/drivers/spi/spi-fsl-spi.c
@@ -424,13 +424,30 @@ static int fsl_spi_do_one_msg(struct spi_master *master,
 		}
 	}
 
-	m->status = status;
-
 	if (status || !cs_change) {
 		ndelay(nsecs);
 		fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
 	}
 
+	if (!status && spi->trailing_bits) {
+		struct spi_transfer t = {
+			.len = 1,
+			.tx_buf = empty_zero_page,
+		};
+
+		if (spi->trailing_bits < 4)
+			t.bits_per_word = 4;
+		else if (spi->trailing_bits > 8)
+			t.bits_per_word = 16;
+		else
+			t.bits_per_word = spi->trailing_bits;
+
+		status = fsl_spi_setup_transfer(spi, &t);
+		if (!status)
+			status = fsl_spi_bufs(spi, &t, 0);
+	}
+	m->status = status;
+
 	fsl_spi_setup_transfer(spi, NULL);
 	spi_finalize_current_message(master);
 	return 0;
-- 
2.34.1

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Mark Brown]

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

On Mon, Feb 28, 2022 at 04:15:46PM +0100, Christophe Leroy wrote:

> +	if (!status && spi->trailing_bits) {
> +		struct spi_transfer t = {
> +			.len = 1,
> +			.tx_buf = empty_zero_page,
> +		};
> +
> +		if (spi->trailing_bits < 4)
> +			t.bits_per_word = 4;
> +		else if (spi->trailing_bits > 8)
> +			t.bits_per_word = 16;
> +		else
> +			t.bits_per_word = spi->trailing_bits;
> +
> +		status = fsl_spi_setup_transfer(spi, &t);
> +		if (!status)
> +			status = fsl_spi_bufs(spi, &t, 0);
> +	}
> +	m->status = status;

The binding looks good now but this is still driver specific code when
it looks like it could easily be implemented in the core - like I said
on the previous version you'd need to update drivers to advertise less
than 8 bits but there's basically nothing driver specific I can see here
so any driver using transfer_one() would get support that way.

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

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Christophe Leroy]

Le 28/02/2022 à 16:29, Mark Brown a écrit :
> On Mon, Feb 28, 2022 at 04:15:46PM +0100, Christophe Leroy wrote:
> 
>> +	if (!status && spi->trailing_bits) {
>> +		struct spi_transfer t = {
>> +			.len = 1,
>> +			.tx_buf = empty_zero_page,
>> +		};
>> +
>> +		if (spi->trailing_bits < 4)
>> +			t.bits_per_word = 4;
>> +		else if (spi->trailing_bits > 8)
>> +			t.bits_per_word = 16;
>> +		else
>> +			t.bits_per_word = spi->trailing_bits;
>> +
>> +		status = fsl_spi_setup_transfer(spi, &t);
>> +		if (!status)
>> +			status = fsl_spi_bufs(spi, &t, 0);
>> +	}
>> +	m->status = status;
> 
> The binding looks good now but this is still driver specific code when
> it looks like it could easily be implemented in the core - like I said
> on the previous version you'd need to update drivers to advertise less
> than 8 bits but there's basically nothing driver specific I can see here
> so any driver using transfer_one() would get support that way.

Argh ! Sorry your comment to the previous version ended up in Junk 
mails. I see it now.

We discussed that back in 2016 in 
https://lore.kernel.org/linux-spi/20160824112701.GE22076@sirena.org.uk/ 
and my understanding at that time was that it was not something that 
could be done at core level.

But maybe things have changed since then ?

By the way, fsl-spi driver doesn't implement transfer_one() but 
transfer_one_message() so it takes care of the chipselect changes and 
therefore the final dummy transfer with CS off is to be done there as 
far as I understand.

Would it mean changing fsl-spi driver to implement transfer_one() first ?

Thanks
Christophe

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Mark Brown]

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

On Mon, Feb 28, 2022 at 04:02:30PM +0000, Christophe Leroy wrote:
> Le 28/02/2022 à 16:29, Mark Brown a écrit :

> > The binding looks good now but this is still driver specific code when
> > it looks like it could easily be implemented in the core - like I said
> > on the previous version you'd need to update drivers to advertise less
> > than 8 bits but there's basically nothing driver specific I can see here
> > so any driver using transfer_one() would get support that way.

> Argh ! Sorry your comment to the previous version ended up in Junk 
> mails. I see it now.

No problem.

> We discussed that back in 2016 in 
> https://lore.kernel.org/linux-spi/20160824112701.GE22076@sirena.org.uk/ 
> and my understanding at that time was that it was not something that 
> could be done at core level.

> But maybe things have changed since then ?

What I said then was "it would need a new core feature" which is what
the binding does, I'm suggesting that you also do that for the handling
of the implementation as well.

Actually now I think about it perhaps this shouldn't be a binding at all
but rather something specified by the client driver - presumably any
system using an affected device is going to need these extra clock
cycles so they'll all need to add the same property.

> By the way, fsl-spi driver doesn't implement transfer_one() but 
> transfer_one_message() so it takes care of the chipselect changes and 
> therefore the final dummy transfer with CS off is to be done there as 
> far as I understand.

> Would it mean changing fsl-spi driver to implement transfer_one() first ?

Well, if it can implement transfer_one() without any negative
consequences whichh

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

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[kernel test robot]

Hi Christophe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.17-rc6]
[cannot apply to broonie-spi/for-next next-20220228]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
base:    7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
config: hexagon-buildonly-randconfig-r004-20220228 (https://download.01.org/0day-ci/archive/20220301/202203010254.tIHIltE2-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7eb07c4d26401389204fcc6cf685c18c89b64ef8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
        git checkout 7eb07c4d26401389204fcc6cf685c18c89b64ef8
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/spi/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/spi/spi-fsl-spi.c:435:14: warning: incompatible integer to pointer conversion initializing 'const void *' with an expression of type 'unsigned long' [-Wint-conversion]
                           .tx_buf = empty_zero_page,
                                     ^~~~~~~~~~~~~~~
   1 warning generated.


vim +435 drivers/spi/spi-fsl-spi.c

   356	
   357	static int fsl_spi_do_one_msg(struct spi_master *master,
   358				      struct spi_message *m)
   359	{
   360		struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(master);
   361		struct spi_device *spi = m->spi;
   362		struct spi_transfer *t, *first;
   363		unsigned int cs_change;
   364		const int nsecs = 50;
   365		int status, last_bpw;
   366	
   367		/*
   368		 * In CPU mode, optimize large byte transfers to use larger
   369		 * bits_per_word values to reduce number of interrupts taken.
   370		 */
   371		if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
   372			list_for_each_entry(t, &m->transfers, transfer_list) {
   373				if (t->len < 256 || t->bits_per_word != 8)
   374					continue;
   375				if ((t->len & 3) == 0)
   376					t->bits_per_word = 32;
   377				else if ((t->len & 1) == 0)
   378					t->bits_per_word = 16;
   379			}
   380		}
   381	
   382		/* Don't allow changes if CS is active */
   383		cs_change = 1;
   384		list_for_each_entry(t, &m->transfers, transfer_list) {
   385			if (cs_change)
   386				first = t;
   387			cs_change = t->cs_change;
   388			if (first->speed_hz != t->speed_hz) {
   389				dev_err(&spi->dev,
   390					"speed_hz cannot change while CS is active\n");
   391				return -EINVAL;
   392			}
   393		}
   394	
   395		last_bpw = -1;
   396		cs_change = 1;
   397		status = -EINVAL;
   398		list_for_each_entry(t, &m->transfers, transfer_list) {
   399			if (cs_change || last_bpw != t->bits_per_word)
   400				status = fsl_spi_setup_transfer(spi, t);
   401			if (status < 0)
   402				break;
   403			last_bpw = t->bits_per_word;
   404	
   405			if (cs_change) {
   406				fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
   407				ndelay(nsecs);
   408			}
   409			cs_change = t->cs_change;
   410			if (t->len)
   411				status = fsl_spi_bufs(spi, t, m->is_dma_mapped);
   412			if (status) {
   413				status = -EMSGSIZE;
   414				break;
   415			}
   416			m->actual_length += t->len;
   417	
   418			spi_transfer_delay_exec(t);
   419	
   420			if (cs_change) {
   421				ndelay(nsecs);
   422				fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   423				ndelay(nsecs);
   424			}
   425		}
   426	
   427		if (status || !cs_change) {
   428			ndelay(nsecs);
   429			fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   430		}
   431	
   432		if (!status && spi->trailing_bits) {
   433			struct spi_transfer t = {
   434				.len = 1,
 > 435				.tx_buf = empty_zero_page,
   436			};
   437	
   438			if (spi->trailing_bits < 4)
   439				t.bits_per_word = 4;
   440			else if (spi->trailing_bits > 8)
   441				t.bits_per_word = 16;
   442			else
   443				t.bits_per_word = spi->trailing_bits;
   444	
   445			status = fsl_spi_setup_transfer(spi, &t);
   446			if (!status)
   447				status = fsl_spi_bufs(spi, &t, 0);
   448		}
   449		m->status = status;
   450	
   451		fsl_spi_setup_transfer(spi, NULL);
   452		spi_finalize_current_message(master);
   453		return 0;
   454	}
   455	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[kernel test robot]

Hi Christophe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.17-rc6]
[cannot apply to broonie-spi/for-next next-20220228]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
base:    7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20220301/202203010220.Yzk53fCj-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7eb07c4d26401389204fcc6cf685c18c89b64ef8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
        git checkout 7eb07c4d26401389204fcc6cf685c18c89b64ef8
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/spi/spi-fsl-spi.c: In function 'fsl_spi_do_one_msg':
>> drivers/spi/spi-fsl-spi.c:435:35: warning: initialization of 'const void *' from 'long unsigned int' makes pointer from integer without a cast [-Wint-conversion]
     435 |                         .tx_buf = empty_zero_page,
         |                                   ^~~~~~~~~~~~~~~
   drivers/spi/spi-fsl-spi.c:435:35: note: (near initialization for 't.tx_buf')


vim +435 drivers/spi/spi-fsl-spi.c

   356	
   357	static int fsl_spi_do_one_msg(struct spi_master *master,
   358				      struct spi_message *m)
   359	{
   360		struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(master);
   361		struct spi_device *spi = m->spi;
   362		struct spi_transfer *t, *first;
   363		unsigned int cs_change;
   364		const int nsecs = 50;
   365		int status, last_bpw;
   366	
   367		/*
   368		 * In CPU mode, optimize large byte transfers to use larger
   369		 * bits_per_word values to reduce number of interrupts taken.
   370		 */
   371		if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
   372			list_for_each_entry(t, &m->transfers, transfer_list) {
   373				if (t->len < 256 || t->bits_per_word != 8)
   374					continue;
   375				if ((t->len & 3) == 0)
   376					t->bits_per_word = 32;
   377				else if ((t->len & 1) == 0)
   378					t->bits_per_word = 16;
   379			}
   380		}
   381	
   382		/* Don't allow changes if CS is active */
   383		cs_change = 1;
   384		list_for_each_entry(t, &m->transfers, transfer_list) {
   385			if (cs_change)
   386				first = t;
   387			cs_change = t->cs_change;
   388			if (first->speed_hz != t->speed_hz) {
   389				dev_err(&spi->dev,
   390					"speed_hz cannot change while CS is active\n");
   391				return -EINVAL;
   392			}
   393		}
   394	
   395		last_bpw = -1;
   396		cs_change = 1;
   397		status = -EINVAL;
   398		list_for_each_entry(t, &m->transfers, transfer_list) {
   399			if (cs_change || last_bpw != t->bits_per_word)
   400				status = fsl_spi_setup_transfer(spi, t);
   401			if (status < 0)
   402				break;
   403			last_bpw = t->bits_per_word;
   404	
   405			if (cs_change) {
   406				fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
   407				ndelay(nsecs);
   408			}
   409			cs_change = t->cs_change;
   410			if (t->len)
   411				status = fsl_spi_bufs(spi, t, m->is_dma_mapped);
   412			if (status) {
   413				status = -EMSGSIZE;
   414				break;
   415			}
   416			m->actual_length += t->len;
   417	
   418			spi_transfer_delay_exec(t);
   419	
   420			if (cs_change) {
   421				ndelay(nsecs);
   422				fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   423				ndelay(nsecs);
   424			}
   425		}
   426	
   427		if (status || !cs_change) {
   428			ndelay(nsecs);
   429			fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   430		}
   431	
   432		if (!status && spi->trailing_bits) {
   433			struct spi_transfer t = {
   434				.len = 1,
 > 435				.tx_buf = empty_zero_page,
   436			};
   437	
   438			if (spi->trailing_bits < 4)
   439				t.bits_per_word = 4;
   440			else if (spi->trailing_bits > 8)
   441				t.bits_per_word = 16;
   442			else
   443				t.bits_per_word = spi->trailing_bits;
   444	
   445			status = fsl_spi_setup_transfer(spi, &t);
   446			if (!status)
   447				status = fsl_spi_bufs(spi, &t, 0);
   448		}
   449		m->status = status;
   450	
   451		fsl_spi_setup_transfer(spi, NULL);
   452		spi_finalize_current_message(master);
   453		return 0;
   454	}
   455	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[kernel test robot]

Hi Christophe,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.17-rc6]
[cannot apply to broonie-spi/for-next next-20220228]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
base:    7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20220301/202203010212.oQMKRiPl-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/7eb07c4d26401389204fcc6cf685c18c89b64ef8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christophe-Leroy/Add-support-for-components-requiring-trailing-clock-after-transfer/20220228-231740
        git checkout 7eb07c4d26401389204fcc6cf685c18c89b64ef8
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/spi/spi-fsl-spi.c: In function 'fsl_spi_do_one_msg':
>> drivers/spi/spi-fsl-spi.c:435:35: error: 'empty_zero_page' undeclared (first use in this function)
     435 |                         .tx_buf = empty_zero_page,
         |                                   ^~~~~~~~~~~~~~~
   drivers/spi/spi-fsl-spi.c:435:35: note: each undeclared identifier is reported only once for each function it appears in


vim +/empty_zero_page +435 drivers/spi/spi-fsl-spi.c

   356	
   357	static int fsl_spi_do_one_msg(struct spi_master *master,
   358				      struct spi_message *m)
   359	{
   360		struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(master);
   361		struct spi_device *spi = m->spi;
   362		struct spi_transfer *t, *first;
   363		unsigned int cs_change;
   364		const int nsecs = 50;
   365		int status, last_bpw;
   366	
   367		/*
   368		 * In CPU mode, optimize large byte transfers to use larger
   369		 * bits_per_word values to reduce number of interrupts taken.
   370		 */
   371		if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
   372			list_for_each_entry(t, &m->transfers, transfer_list) {
   373				if (t->len < 256 || t->bits_per_word != 8)
   374					continue;
   375				if ((t->len & 3) == 0)
   376					t->bits_per_word = 32;
   377				else if ((t->len & 1) == 0)
   378					t->bits_per_word = 16;
   379			}
   380		}
   381	
   382		/* Don't allow changes if CS is active */
   383		cs_change = 1;
   384		list_for_each_entry(t, &m->transfers, transfer_list) {
   385			if (cs_change)
   386				first = t;
   387			cs_change = t->cs_change;
   388			if (first->speed_hz != t->speed_hz) {
   389				dev_err(&spi->dev,
   390					"speed_hz cannot change while CS is active\n");
   391				return -EINVAL;
   392			}
   393		}
   394	
   395		last_bpw = -1;
   396		cs_change = 1;
   397		status = -EINVAL;
   398		list_for_each_entry(t, &m->transfers, transfer_list) {
   399			if (cs_change || last_bpw != t->bits_per_word)
   400				status = fsl_spi_setup_transfer(spi, t);
   401			if (status < 0)
   402				break;
   403			last_bpw = t->bits_per_word;
   404	
   405			if (cs_change) {
   406				fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
   407				ndelay(nsecs);
   408			}
   409			cs_change = t->cs_change;
   410			if (t->len)
   411				status = fsl_spi_bufs(spi, t, m->is_dma_mapped);
   412			if (status) {
   413				status = -EMSGSIZE;
   414				break;
   415			}
   416			m->actual_length += t->len;
   417	
   418			spi_transfer_delay_exec(t);
   419	
   420			if (cs_change) {
   421				ndelay(nsecs);
   422				fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   423				ndelay(nsecs);
   424			}
   425		}
   426	
   427		if (status || !cs_change) {
   428			ndelay(nsecs);
   429			fsl_spi_chipselect(spi, BITBANG_CS_INACTIVE);
   430		}
   431	
   432		if (!status && spi->trailing_bits) {
   433			struct spi_transfer t = {
   434				.len = 1,
 > 435				.tx_buf = empty_zero_page,
   436			};
   437	
   438			if (spi->trailing_bits < 4)
   439				t.bits_per_word = 4;
   440			else if (spi->trailing_bits > 8)
   441				t.bits_per_word = 16;
   442			else
   443				t.bits_per_word = spi->trailing_bits;
   444	
   445			status = fsl_spi_setup_transfer(spi, &t);
   446			if (!status)
   447				status = fsl_spi_bufs(spi, &t, 0);
   448		}
   449		m->status = status;
   450	
   451		fsl_spi_setup_transfer(spi, NULL);
   452		spi_finalize_current_message(master);
   453		return 0;
   454	}
   455	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Christophe Leroy]

Le 28/02/2022 à 17:14, Mark Brown a écrit :
> On Mon, Feb 28, 2022 at 04:02:30PM +0000, Christophe Leroy wrote:
>> Le 28/02/2022 à 16:29, Mark Brown a écrit :
> 
>>> The binding looks good now but this is still driver specific code when
>>> it looks like it could easily be implemented in the core - like I said
>>> on the previous version you'd need to update drivers to advertise less
>>> than 8 bits but there's basically nothing driver specific I can see here
>>> so any driver using transfer_one() would get support that way.
> 
>> Argh ! Sorry your comment to the previous version ended up in Junk
>> mails. I see it now.
> 
> No problem.
> 
>> We discussed that back in 2016 in
>> https://lore.kernel.org/linux-spi/20160824112701.GE22076@sirena.org.uk/
>> and my understanding at that time was that it was not something that
>> could be done at core level.
> 
>> But maybe things have changed since then ?
> 
> What I said then was "it would need a new core feature" which is what
> the binding does, I'm suggesting that you also do that for the handling
> of the implementation as well.
> 
> Actually now I think about it perhaps this shouldn't be a binding at all
> but rather something specified by the client driver - presumably any
> system using an affected device is going to need these extra clock
> cycles so they'll all need to add the same property.
> 
>> By the way, fsl-spi driver doesn't implement transfer_one() but
>> transfer_one_message() so it takes care of the chipselect changes and
>> therefore the final dummy transfer with CS off is to be done there as
>> far as I understand.
> 
>> Would it mean changing fsl-spi driver to implement transfer_one() first ?
> 
> Well, if it can implement transfer_one() without any negative
> consequences whichh

Seems like your sentence is truncated.

My understanding today is that this trailing transfer with chipselect 
OFF is to be added at the end of transfer_one_message().

It can be implemented in the core transfer_one_message() for drivers 
implementing transfer_one(). For the other drivers not having 
transfer_one() but having transfer_one_message(), it must be implemented 
in the driver's transfer_one_message().

Am I right ?


fsl-spi driver is the one I need to support this new functionnality and 
it has its own transfer_one_message().

What would you expect ?


Thanks
Christophe

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Mark Brown]

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

On Tue, Mar 01, 2022 at 12:53:58PM +0000, Christophe Leroy wrote:

> My understanding today is that this trailing transfer with chipselect 
> OFF is to be added at the end of transfer_one_message().

Yes.

> It can be implemented in the core transfer_one_message() for drivers 
> implementing transfer_one(). For the other drivers not having 
> transfer_one() but having transfer_one_message(), it must be implemented 
> in the driver's transfer_one_message().

> Am I right ?

Yes.

> fsl-spi driver is the one I need to support this new functionnality and 
> it has its own transfer_one_message().

> What would you expect ?

Well, if the fsl-spi driver has a good reason for open coding
transfer_message() it could continue to do it, however looking at the
driver it seems like it's only the little bit at the start to optimise
the trasfer width in CPU mode which looks like it could easily be moved
to a prepare_message() callback and save a bunch of code so it'd be good
to convert it.  I guess it shouldn't be strictly essential though.

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

Re: [PATCH v2 1/2] spi: Add optional number of additional clock cycles to be generated

[Rob Herring]

On Mon, 28 Feb 2022 16:15:45 +0100, Christophe Leroy wrote:
> Some components require a few clock cycles with chipselect off before
> or/and after the data transfer done with CS on.
> 
> Typically IDT 801034 QUAD PCM CODEC datasheet states "Note *: CCLK
> should have one cycle before CS goes low, and two cycles after
> CS goes high".
> 
> The cycles "before" are implicitely provided by all previous activity
> on the SPI bus. But the cycles "after" must be provided in order to
> achieve the SPI transfer.
> 
> In order to use that kind of component, implement a new option for
> SPI slaves in order to implement trailing clock of a given number of
> bits with ChipSelect off at the end of the transfer.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  .../devicetree/bindings/spi/spi-peripheral-props.yaml      | 5 +++++
>  drivers/spi/spi.c                                          | 7 +++++--
>  include/linux/spi/spi.h                                    | 1 +
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Christophe Leroy]

Le 28/02/2022 à 17:14, Mark Brown a écrit :
> 
>> We discussed that back in 2016 in
>> https://lore.kernel.org/linux-spi/20160824112701.GE22076@sirena.org.uk/
>> and my understanding at that time was that it was not something that
>> could be done at core level.
> 
>> But maybe things have changed since then ?
> 
> What I said then was "it would need a new core feature" which is what
> the binding does, I'm suggesting that you also do that for the handling
> of the implementation as well.
> 
> Actually now I think about it perhaps this shouldn't be a binding at all
> but rather something specified by the client driver - presumably any
> system using an affected device is going to need these extra clock
> cycles so they'll all need to add the same property.

Yes indeed. Therefore in v3 I took a different approach : a flag .cs_off 
tells to spi_transfer_one_message() that a given transfer has to be 
performed with chipselect OFF, therefore the consumer has full control 
of how and when to add those additional fake clock cycles during a 
transfer, and can eventually add one at anyplace during the transfer.

Here an exemple of what will do the consumer.


static int idt821034_8bit_write(struct idt821034 *idt821034, u8 val)
{
	struct spi_transfer xfer[] = {{
		.tx_buf = &idt821034->spi_tx_buf,
		.len = 1,
	},{
		.tx_buf = &idt821034->spi_tx_buf,
		.len = 1,
		.cs_off = 1,
	}};
	int ret;

	idt821034->spi_tx_buf = val;

	ret = spi_sync_transfer(idt821034->spi, xfer, ARRAY_SIZE(xfer));
	if (ret)
		return ret;

	return 0;
}



Christophe

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Mark Brown]

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

On Thu, Aug 18, 2022 at 06:35:39PM +0000, Christophe Leroy wrote:

> Yes indeed. Therefore in v3 I took a different approach : a flag .cs_off 
> tells to spi_transfer_one_message() that a given transfer has to be 
> performed with chipselect OFF, therefore the consumer has full control 
> of how and when to add those additional fake clock cycles during a 
> transfer, and can eventually add one at anyplace during the transfer.

> Here an exemple of what will do the consumer.

Hrm, we should already be able to synthesize that with cs_change though
there's usability challenges there and AFAICT it doesn't work for the
first transfer which your proposal would so there's a functional benefit
even if you don't need it for your device right now.  It would be good
if you could have a look at using cs_change for your use case.  Sorry, I
don't think I'd fully realised what you were looking to accomplish here
until I saw your proposal.

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

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Christophe Leroy]

Le 22/08/2022 à 19:15, Mark Brown a écrit :
> On Thu, Aug 18, 2022 at 06:35:39PM +0000, Christophe Leroy wrote:
> 
>> Yes indeed. Therefore in v3 I took a different approach : a flag .cs_off
>> tells to spi_transfer_one_message() that a given transfer has to be
>> performed with chipselect OFF, therefore the consumer has full control
>> of how and when to add those additional fake clock cycles during a
>> transfer, and can eventually add one at anyplace during the transfer.
> 
>> Here an exemple of what will do the consumer.
> 
> Hrm, we should already be able to synthesize that with cs_change though
> there's usability challenges there and AFAICT it doesn't work for the
> first transfer which your proposal would so there's a functional benefit
> even if you don't need it for your device right now.  It would be good
> if you could have a look at using cs_change for your use case.  Sorry, I
> don't think I'd fully realised what you were looking to accomplish here
> until I saw your proposal.

I think we already addressed this possibility back in 2016, see 
https://lore.kernel.org/linux-spi/20160824111206.GD22076@sirena.org.uk/

The conclusion was that it was not possible to accomplish it with cs_change.

Or did we miss something at that time ?

My understanding is that if you set cs_change for any transfer of a 
message except the last, then CS goes OFF then ON again between the said 
transfer and the following one. If you set cs_change for the last 
transfer of a message, then CS stays ON until next message instead of 
going OFF as usual.

My need is to transfer a fake byte with CS off at the end of a message. 
I still can't see how cs_change can achieve that.

Thanks
Christophe

Re: [PATCH v2 2/2] spi: fsl-spi: Implement trailing bits

[Mark Brown]

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

On Mon, Aug 22, 2022 at 06:38:22PM +0000, Christophe Leroy wrote:
> Le 22/08/2022 à 19:15, Mark Brown a écrit :

> I think we already addressed this possibility back in 2016, see 
> https://lore.kernel.org/linux-spi/20160824111206.GD22076@sirena.org.uk/

> The conclusion was that it was not possible to accomplish it with cs_change.

> Or did we miss something at that time ?

No, I think that's fine - your proposal has some overlap but is fine.

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