SPI: support DUAL and QUAD[patch v1]

Hi,

The patch below is to make spi framework support dual(2x) and quad(4x).
Bus the name of members may still look strange, so I need some suggestions.
Thanks.

From f33f5935776aa0b44b01a36f88e47ba0cfe8fd21 Mon Sep 17 00:00:00 2001
From: wangyuhang <wangyuhang2014@gmail.com>
Date: Wed, 10 Jul 2013 16:22:19 +0800
Subject: [PATCH] spi final patch v2 Signed-off-by: wangyuhang
 <wangyuhang2014@gmail.com>

Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
---
 drivers/spi/spi.c       |    6 ++++++
 include/linux/spi/spi.h |   31 +++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

  * as a partial device template.  They hold information which can't always
@@ -859,6 +884,12 @@ struct spi_board_info {
  * where the default of SPI_CS_HIGH = 0 is wrong.
  */
  u8 mode;
+ /* tx_nbits initialized for spi_device.tx_nbits and
+ * rx_nbits initialized for spi_device.rx_nbits. These members
+ * used to describe the how many lines used in tx and rx.
+ */
+ u8 tx_nbits;
+ u8 rx_nbits;

  /* ... may need additional spi_device chip config data here.
  * avoid stuff protocol drivers can set; but include stuff
--
1.7.9.5

Best Regards.

> Hi,
> 
> The patch below is to make spi framework support dual(2x) and quad(4x).
> Bus the name of members may still look strange, so I need some
> suggestions.
> Thanks.
> 
> From f33f5935776aa0b44b01a36f88e47ba0cfe8fd21 Mon Sep 17 00:00:00 2001
> From: wangyuhang <wangyuhang2014@gmail.com>
> Date: Wed, 10 Jul 2013 16:22:19 +0800
> Subject: [PATCH] spi final patch v2 Signed-off-by: wangyuhang
>  <wangyuhang2014@gmail.com>
> 
> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> ---
>  drivers/spi/spi.c       |    6 ++++++
>  include/linux/spi/spi.h |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 004b10f..af28a62 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -452,6 +452,8 @@ struct spi_device *spi_new_device(struct spi_master
> *master,
>   proxy->irq = chip->irq;
>   strlcpy(proxy->modalias, chip->modalias, sizeof(proxy->modalias));
>   proxy->dev.platform_data = (void *) chip->platform_data;
> + proxy->tx_nbits = chip->tx_nbits ? chip->tx_nbits : SPI_NBITS_SINGLE;
> + proxy->rx_nbits = chip->rx_nbits ? chip->rx_nbits : SPI_NBITS_SINGLE;
>   proxy->controller_data = chip->controller_data;
>   proxy->controller_state = NULL;
> 
[Pekon]: You can use existing 'mode_bits' field in struct spi_master to 
pass on the controller mode.
If (master->mode & QSPI_MODE)
	// configure for QSPI in prepare_transfer_hardware()
else
	// continue with normal SPI


> @@ -1376,6 +1378,10 @@ static int __spi_async(struct spi_device *spi,
> struct spi_message *message)
>   xfer->bits_per_word = spi->bits_per_word;
>   if (!xfer->speed_hz)
>   xfer->speed_hz = spi->max_speed_hz;
> + if (!xfer->tx_nbits)
> + xfer->tx_nbits = SPI_NBITS_SINGLE;
> + if (!xfer->rx_nbits)
> + xfer->rx_nbits = SPI_NBITS_SINGLE;
>   }
> 
>   message->spi = spi;
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 38c2b92..d9b3746 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -59,6 +59,10 @@ extern struct bus_type spi_bus_type;
>   * for driver coldplugging, and in uevents used for hotplugging
>   * @cs_gpio: gpio number of the chipselect line (optional, -EINVAL when
>   * when not using a GPIO line)
> + * @tx_nbits: number of bits used in tx, get from @spi_board_info
> + * (optional, 1bit as init value if not set in @spi_board_info)
> + * @rx_nbits: number of bits used in rx, get from @spi_board_info
> + * (optional, 1bit as init value if not set in @spi_board_info)
>   *
>   * A @spi_device is used to interchange data between an SPI slave
>   * (usually a discrete chip) and CPU memory.
> @@ -93,6 +97,8 @@ struct spi_device {
>   void *controller_data;
>   char modalias[SPI_NAME_SIZE];
>   int cs_gpio; /* chip select gpio */
> + u8 tx_nbits; /* num of bits used in tx */
> + u8 rx_nbits; /* num of bits used in rx */
> 
[Pekon]: Instead of add new fields, You can extend existing  'u8 mode'
 field in struct spi_device to  'u32 mode' and then use it for identifying
 command OPCODES. Like below..
If (device->mode & QSPI_MODE)
	// send QSPI specific OPCODE before transfer
else
	// continue with normal SPI


>   /*
>   * likely need more hooks for more protocol options affecting how
> @@ -437,6 +443,10 @@ extern struct spi_master
> *spi_busnum_to_master(u16 busnum);
>   * @rx_buf: data to be read (dma-safe memory), or NULL
>   * @tx_dma: DMA address of tx_buf, if @spi_message.is_dma_mapped
>   * @rx_dma: DMA address of rx_buf, if @spi_message.is_dma_mapped
> + * @tx_nbits: number of bits used for writting, if 0 default
> + * (SPI_NBITS_SINGLE = 0x01) is used.
> + * @rx_nbits: number of bits used for reading, if 0 default
> + * (SPI_NBITS_SINGLE = 0x01) is used.
>   * @len: size of rx and tx buffers (in bytes)
>   * @speed_hz: Select a speed other than the device default for this
>   *      transfer. If 0 the default (from @spi_device) is used.
> @@ -491,6 +501,11 @@ extern struct spi_master
> *spi_busnum_to_master(u16 busnum);
>   * by the results of previous messages and where the whole transaction
>   * ends when the chipselect goes intactive.
>   *
> + * When SPI can transfer in 1x,2x or 4x. It can get this tranfer information
> + * from device through @tx_nbits and @rx_nbits. In Bi-direction, these
> + * two should both be set. User can set transfer mode with
> SPI_NBITS_SINGLE(1x)
> + * SPI_NBITS_DUAL(2x) and SPI_NBITS_QUAD(4x) to support these three
> transfer.
> + *
>   * The code that submits an spi_message (and its spi_transfers)
>   * to the lower layers is responsible for managing its memory.
>   * Zero-initialize every field you don't set up explicitly, to
> @@ -510,6 +525,12 @@ struct spi_transfer {
>   dma_addr_t tx_dma;
>   dma_addr_t rx_dma;
> 
> + u8 tx_nbits;
> + u8 rx_nbits;
> +#define SPI_NBITS_SINGLE 0x01; /* 1bit transfer */
> +#define SPI_NBITS_DUAL 0x02; /* 2bits transfer */
> +#define SPI_NBITS_QUAD 0x03; /* 4bits transfer */
> +
>   unsigned cs_change:1;
>   u8 bits_per_word;
>   u16 delay_usecs;
> @@ -815,6 +836,10 @@ static inline ssize_t spi_w8r16(struct spi_device
> *spi, u8 cmd)
>   * @mode: Initializes spi_device.mode; based on the chip datasheet, board
>   * wiring (some devices support both 3WIRE and standard modes), and
>   * possibly presence of an inverter in the chipselect path.
> + * @tx_nbits: Initializes spi_device.tx_nbits; depends on the number of bits
> + * the board used in tx.
> + * @rx_nbits: Initializes spi_device.rx_nbits; depends on the number of bits
> + * the board used in rx.
>   *
>   * When adding new SPI devices to the device tree, these structures serve
>   * as a partial device template.  They hold information which can't always
> @@ -859,6 +884,12 @@ struct spi_board_info {
>   * where the default of SPI_CS_HIGH = 0 is wrong.
>   */
>   u8 mode;
> + /* tx_nbits initialized for spi_device.tx_nbits and
> + * rx_nbits initialized for spi_device.rx_nbits. These members
> + * used to describe the how many lines used in tx and rx.
> + */
> + u8 tx_nbits;
> + u8 rx_nbits;
> 
[Pekon]: Instead of adding new fields you can use existing 'mode' field to
pass on the platform specific configurations. And if 'u8 mode' does not 
suffice you can increase it to 'u32'.
#define QSPI_MODE 	1 << 5; // just check which bit-fields are un-used
spi_board_info->mode |= QSPI_MODE;

Same way you can pass the configuration to spi_master and spi_device.

>   /* ... may need additional spi_device chip config data here.
>   * avoid stuff protocol drivers can set; but include stuff
> --
> 1.7.9.5
> 

with regards, pekon

Hi, Gupta


> [Pekon]: Instead of adding new fields you can use existing 'mode' field to
> pass on the platform specific configurations. And if 'u8 mode' does not
> suffice you can increase it to 'u32'.
> #define QSPI_MODE       1 << 5; // just check which bit-fields are un-used
> spi_board_info->mode |= QSPI_MODE;
>

well, can dual and quad be regarded as a spi mode? if so, your comment seems
to be right.

> 
> Hi, Gupta
> 
> 
> > [Pekon]: Instead of adding new fields you can use existing 'mode' field to
> > pass on the platform specific configurations. And if 'u8 mode' does not
> > suffice you can increase it to 'u32'.
> > #define QSPI_MODE       1 << 5; // just check which bit-fields are un-used
> > spi_board_info->mode |= QSPI_MODE;
> >
> 
> well, can dual and quad be regarded as a spi mode? if so, your comment
> seems
> to be right.

Yes, Quad and Dual modes should be regarded as extension of SPI protocol.
- They follow the same basic principle of synchronous data transfer. Right ?
- These modes are not adding any extra side-band | In-band signaling or 
controls to modify the protocol. They are just increasing the width of
 data-channel for better throughput.

with regards, pekon

On Tue, Jul 16, 2013 at 04:59:49PM +0800, yuhang wang wrote:

> > [Pekon]: Instead of adding new fields you can use existing 'mode' field to
> > pass on the platform specific configurations. And if 'u8 mode' does not
> > suffice you can increase it to 'u32'.
> > #define QSPI_MODE       1 << 5; // just check which bit-fields are un-used
> > spi_board_info->mode |= QSPI_MODE;

> well, can dual and quad be regarded as a spi mode? if so, your comment seems
> to be right.

Yes, I think this is a good suggestion.

Hello Pekon,

Gupta, Pekon wrote on 2013-07-16:
> >
> > Hi, Gupta
> >
> >
> > > [Pekon]: Instead of adding new fields you can use existing 'mode' field to
> > > pass on the platform specific configurations. And if 'u8 mode' does not
> > > suffice you can increase it to 'u32'.
> > > #define QSPI_MODE       1 << 5; // just check which bit-fields are un-used
> > > spi_board_info->mode |= QSPI_MODE;
> > >
> >
> > well, can dual and quad be regarded as a spi mode? if so, your comment
> > seems
> > to be right.
> 
> Yes, Quad and Dual modes should be regarded as extension of SPI protocol.
> - They follow the same basic principle of synchronous data transfer. Right ?
> - These modes are not adding any extra side-band | In-band signaling or
> controls to modify the protocol. They are just increasing the width of
>  data-channel for better throughput.
> 
> with regards, pekon
> 

In general, yes. But I think, for the interface we have to take care of more details.

For example, what happens in the following situation:
We have a spi-controller, which supports the QSPI mode, and a spi-flash, which fulfils 
the requirements, but the board has not connected all signals?

And the interface for the slave-driver (like m25p80) should allow to specify the transfer mode
for each spi_message.
This will be necessary, because it depends on the flash and mode, how each phase of "cmd",
"address", and "data" will be transferred.

I don't know, if it is only me, but I would like to see some more abstract description for all 
these scenarios, before the details of an interface really could be discussed and decided.

Best Regards,
Thomas

> 
> Hello Pekon,
> 
> Gupta, Pekon wrote on 2013-07-16:
> > >
> > > Hi, Gupta
> > >
> > >
> > > > [Pekon]: Instead of adding new fields you can use existing 'mode' field
> to
> > > > pass on the platform specific configurations. And if 'u8 mode' does not
> > > > suffice you can increase it to 'u32'.
> > > > #define QSPI_MODE       1 << 5; // just check which bit-fields are un-
> used
> > > > spi_board_info->mode |= QSPI_MODE;
> > > >
> > >
> > > well, can dual and quad be regarded as a spi mode? if so, your comment
> > > seems
> > > to be right.
> >
> > Yes, Quad and Dual modes should be regarded as extension of SPI
> protocol.
> > - They follow the same basic principle of synchronous data transfer. Right ?
> > - These modes are not adding any extra side-band | In-band signaling or
> > controls to modify the protocol. They are just increasing the width of
> >  data-channel for better throughput.
> >
> > with regards, pekon
> >
> 
> In general, yes. But I think, for the interface we have to take care of more
> details.
> 
> For example, what happens in the following situation:
> We have a spi-controller, which supports the QSPI mode, and a spi-flash,
> which fulfils
> the requirements, but the board has not connected all signals?
> 
[Pekon]: So, actual implementation should set QSPI_MODE based on both
(1) device capabilities (detected in probe)
(2) board-DT properties (this would take care of your other constrains)

> And the interface for the slave-driver (like m25p80) should allow to specify
> the transfer mode
> for each spi_message.
> This will be necessary, because it depends on the flash and mode, how each
> phase of "cmd",
> "address", and "data" will be transferred.
> 
[Pekon]: Are you saying that same controller can send interleaved 
Quad-SPI and Single-SPI spi_messages to same device (here flash device)?

Is it due to flash-device constrain that it accepts some flash-commands 
(like erase, etc) only on Single-wire-SPI, whereas  data transfers can be 
Performed on Quad-SPI mode ?
In such case you need to add 'mode' field in struct spi_message, so that
this information moves along with the spi_message in queue. And then
when that message reaches spi_pump_message() and 
transfer_one_message() is called, then you can do needful configs.

However, I still don't think you need to control 'QSPI_MODE' at granularity
of spi_transfer level, because m25p80 flash-driver use spi_write() API to
pass data to SPI driver, which internally creates a spi_message in each call.

Following best explains use of spi_message in /include/linux/spi.h
---------------------------------------------------
* A @spi_message is used to execute an atomic sequence of data transfers,
 * each represented by a struct spi_transfer.  The sequence is "atomic"
 * in the sense that no other spi_message may use that SPI bus until that
 * sequence completes.  On some systems, many such sequences can execute as
 * as single programmed DMA transfer.  On all systems, these messages are
 * queued, and might complete after transactions to other devices.  Messages
 * sent to a given spi_device are alway executed in FIFO order.
---------------------------------------------------


> I don't know, if it is only me, but I would like to see some more abstract
> description for all
> these scenarios, before the details of an interface really could be discussed
> and decided.
> 
Agree.. Does above matches your requirement ?

 
with regards, pekon

> Best Regards,
> Thomas

Hello Pekon,

2013/7/16 Gupta, Pekon <pekon@ti.com>:
>>
>> Hello Pekon,
>>
>> Gupta, Pekon wrote on 2013-07-16:
>> > >
>> > > Hi, Gupta
>> > >
>> > >
>> > > > [Pekon]: Instead of adding new fields you can use existing 'mode' field
>> to
>> > > > pass on the platform specific configurations. And if 'u8 mode' does not
>> > > > suffice you can increase it to 'u32'.
>> > > > #define QSPI_MODE       1 << 5; // just check which bit-fields are un-
>> used
>> > > > spi_board_info->mode |= QSPI_MODE;
>> > > >
>> > >
>> > > well, can dual and quad be regarded as a spi mode? if so, your comment
>> > > seems
>> > > to be right.
>> >
>> > Yes, Quad and Dual modes should be regarded as extension of SPI
>> protocol.
>> > - They follow the same basic principle of synchronous data transfer. Right ?
>> > - These modes are not adding any extra side-band | In-band signaling or
>> > controls to modify the protocol. They are just increasing the width of
>> >  data-channel for better throughput.
>> >
>> > with regards, pekon
>> >
>>
>> In general, yes. But I think, for the interface we have to take care of more
>> details.
>>
>> For example, what happens in the following situation:
>> We have a spi-controller, which supports the QSPI mode, and a spi-flash,
>> which fulfils
>> the requirements, but the board has not connected all signals?
>>
> [Pekon]: So, actual implementation should set QSPI_MODE based on both
> (1) device capabilities (detected in probe)
> (2) board-DT properties (this would take care of your other constrains)
>
I think the name QSPI_MODE is not siutable. Not only QSPI support dual and quad.

>> And the interface for the slave-driver (like m25p80) should allow to specify
>> the transfer mode
>> for each spi_message.
>> This will be necessary, because it depends on the flash and mode, how each
>> phase of "cmd",
>> "address", and "data" will be transferred.
>>
> [Pekon]: Are you saying that same controller can send interleaved
> Quad-SPI and Single-SPI spi_messages to same device (here flash device)?
>
> Is it due to flash-device constrain that it accepts some flash-commands
> (like erase, etc) only on Single-wire-SPI, whereas  data transfers can be
> Performed on Quad-SPI mode ?
> In such case you need to add 'mode' field in struct spi_message, so that
> this information moves along with the spi_message in queue. And then
> when that message reaches spi_pump_message() and
> transfer_one_message() is called, then you can do needful configs.
>
> However, I still don't think you need to control 'QSPI_MODE' at granularity
> of spi_transfer level, because m25p80 flash-driver use spi_write() API to
> pass data to SPI driver, which internally creates a spi_message in each call.
>
> Following best explains use of spi_message in /include/linux/spi.h
> ---------------------------------------------------
> * A @spi_message is used to execute an atomic sequence of data transfers,
>  * each represented by a struct spi_transfer.  The sequence is "atomic"
>  * in the sense that no other spi_message may use that SPI bus until that
>  * sequence completes.  On some systems, many such sequences can execute as
>  * as single programmed DMA transfer.  On all systems, these messages are
>  * queued, and might complete after transactions to other devices.  Messages
>  * sent to a given spi_device are alway executed in FIFO order.
> ---------------------------------------------------
>
>
what thomas means that some flash device will do read or write in
different mode. example:
cmd with single mode;
addr with quad mode;
data with quad mode;
So we really can not decide how to organise the spi_message just based
on QSPI_MODE.
But to this question, because there is no standard for all the
serial-flash and different serial-flash may have different sequences.
Thus what we can do is to provide the interface in spi framework. To
different slave, we should adjust the driver by ourselves.

>> I don't know, if it is only me, but I would like to see some more abstract
>> description for all
>> these scenarios, before the details of an interface really could be discussed
>> and decided.
>>
> Agree.. Does above matches your requirement ?
>
Thanks for your advice and I will provide a patch later. please help me review.
>
> with regards, pekon
>
>> Best Regards,
>> Thomas
>

Hello Pekon,

Gupta, Pekon wrote on 2013-07-16:
> >
> > Hello Pekon,
> >
> > Gupta, Pekon wrote on 2013-07-16:
> > > >
> > > > Hi, Gupta
> > > >
> > > >
> > > > > [Pekon]: Instead of adding new fields you can use existing 'mode'
> field
> > to
> > > > > pass on the platform specific configurations. And if 'u8 mode' does
> not
> > > > > suffice you can increase it to 'u32'.
> > > > > #define QSPI_MODE       1 << 5; // just check which bit-fields are un-
> > used
> > > > > spi_board_info->mode |= QSPI_MODE;
> > > > >
> > > >
> > > > well, can dual and quad be regarded as a spi mode? if so, your comment
> > > > seems
> > > > to be right.
> > >
> > > Yes, Quad and Dual modes should be regarded as extension of SPI
> > protocol.
> > > - They follow the same basic principle of synchronous data transfer. Right
> ?
> > > - These modes are not adding any extra side-band | In-band signaling or
> > > controls to modify the protocol. They are just increasing the width of
> > >  data-channel for better throughput.
> > >
> > > with regards, pekon
> > >
> >
> > In general, yes. But I think, for the interface we have to take care of more
> > details.
> >
> > For example, what happens in the following situation:
> > We have a spi-controller, which supports the QSPI mode, and a spi-flash,
> > which fulfils
> > the requirements, but the board has not connected all signals?
> >
> [Pekon]: So, actual implementation should set QSPI_MODE based on both
> (1) device capabilities (detected in probe)
> (2) board-DT properties (this would take care of your other constrains)
> 
yes, and in the end the slave (as of m25p80) can decide, which actions/modes are usable
and should only use them.

> > And the interface for the slave-driver (like m25p80) should allow to specify
> > the transfer mode
> > for each spi_message.
> > This will be necessary, because it depends on the flash and mode, how
> each
> > phase of "cmd",
> > "address", and "data" will be transferred.
> >
> [Pekon]: Are you saying that same controller can send interleaved
> Quad-SPI and Single-SPI spi_messages to same device (here flash device)?
Yes, for example the S25FL129P from Spansion always expects the command (first 8 bit)
on a single wire and then, depending on the command, switching to dual or quad-mode
for the address and/or data phase.
And other flashes I have seen (don't remember name or manufacturer) support to be switched
into a quad-mode, in which they expect all data (including the command) as quad transfer.
> 
> Is it due to flash-device constrain that it accepts some flash-commands
> (like erase, etc) only on Single-wire-SPI, whereas  data transfers can be
> Performed on Quad-SPI mode ?
> In such case you need to add 'mode' field in struct spi_message, so that
> this information moves along with the spi_message in queue. And then
> when that message reaches spi_pump_message() and
> transfer_one_message() is called, then you can do needful configs.
Yes, I think this would be a good way to allow the flash driver to control these details.

> 
> However, I still don't think you need to control 'QSPI_MODE' at granularity
> of spi_transfer level, because m25p80 flash-driver use spi_write() API to
> pass data to SPI driver, which internally creates a spi_message in each call.
I expect some changes to this driver for adding this modes, but as discussed above,
it should be enough to have the mode per message.

> 
> Following best explains use of spi_message in /include/linux/spi.h
> ---------------------------------------------------
> * A @spi_message is used to execute an atomic sequence of data transfers,
>  * each represented by a struct spi_transfer.  The sequence is "atomic"
>  * in the sense that no other spi_message may use that SPI bus until that
>  * sequence completes.  On some systems, many such sequences can
> execute as
>  * as single programmed DMA transfer.  On all systems, these messages are
>  * queued, and might complete after transactions to other devices.
> Messages
>  * sent to a given spi_device are alway executed in FIFO order.
> ---------------------------------------------------
> 
> 
> > I don't know, if it is only me, but I would like to see some more abstract
> > description for all
> > these scenarios, before the details of an interface really could be discussed
> > and decided.
> >
> Agree.. Does above matches your requirement ?
> 
Yes, looks better. Thanks!

> 
> with regards, pekon

Best Regards,
Thomas