[PATCH] SPI: DUAL and QUAD support
Gupta, Pekon
pekon at ti.com
Mon Jul 29 10:21:27 EDT 2013
Hi Yuhang,
> >>
> >> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> >> index 004b10f..3f6dc93 100644
> >> --- a/drivers/spi/spi.c
> >> +++ b/drivers/spi/spi.c
> >> @@ -868,6 +868,14 @@ static void of_register_spi_devices(struct
> spi_master
> >> *master)
> >> spi->mode |= SPI_CS_HIGH;
> >> if (of_find_property(nc, "spi-3wire", NULL))
> >> spi->mode |= SPI_3WIRE;
> >> + if (of_find_property(nc, "spi-tx-dual", NULL))
> >> + spi->mode |= SPI_TX_DUAL;
> >> + if (of_find_property(nc, "spi-tx-quad", NULL))
> >> + spi->mode |= SPI_TX_QUAD;
> >> + if (of_find_property(nc, "spi-rx-dual", NULL))
> >> + spi->mode |= SPI_RX_DUAL;
> >> + if (of_find_property(nc, "spi-rx-quad", NULL))
> >> + spi->mode |= SPI_RX_QUAD;
> >>
> > [Pekon] I think its cleaner to use similar parameters for channel-width
> > info everywhere. Like you have added separate fields 'tx_nbits' and
> > 'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ?
> > And also in 'spi_device'. This can be like below:
> >
> > - SPI_3WIRE : tx_nbit = 0, rx_nbit = 0 (single wire for MISO & MOSI)
> > - SPI_4WIRE : tx_nbit = 1, rx_nbit = 1 (separate wire for MISO & MOSI)
> > - DUAL_SPI : tx_nbit = 2, rx_nbit = 2
> > - QUAD_SPI : tx_nbit = 2 or 4, rx_nbit = 4
> > This way 'mode' field can be explicitly left for other purposes like CPHA,
> CPOL.
> > OR custom modes (like modified SPI mode in Freescale controllers).
> >
> > But if you are touching any existing configuration (like SPI_3WIRE as
> > proposed above), Then please send it as independent patch/proposal,
> > which can be Acked by individual SPI driver maintainers.
> >
> Adding the similar members like 'tx_nbits' and 'rx_nbits' is my
> initial thought. But in different struct, that members meaning may be
> a little different.
> In spi_master, the member is what transfer mode master can support.
> In spi_device, the member is what mode the slave will use.
> In spi_transfer, the member is the transfer wires needed.
> 1: adding the similar members everywhere may looks a little redundant.
> What is more, in spi_master and spi_device, it is more like a mode and
> in spi_device it is a transfer function due to the mode.
> example:
> DUAL mode in spi_device, in spi_transfer tx_nbits or rx_nbits can be
> set to 1 or 2.
> 2: the member in spi_master has to describe all the transfer
> mode(single/dual/quad), because the member means the mode that master
> support, so the member has to be seperated into several bits to cover
> all the single dual and quad and then check it to spi_device. This
> also seems similar to what the existed spi_master->mode_bit and
> spi_device->mode do.
> so added in mode may looks better.
>
[Pekon]: Just review following scenarios ..
_Case-1_: I have a controller which supports
- Quad & Dual modes only for reads,
- Only Single SPI for writes.
This means though bit (spi_master.mode & QUAD_MODE)
is set, but still if MTD layer does a SPI WRITE transfer using tx_nbits=4,
I should get error..
_Case-2_: Some flash devices also have similar constrains.
Example: check Spansion flash S25FL128S datasheet,
"section 10.1 command summary". This device supports
- Single/Dual/Quad READS,
- but only SINGLE & QUAD page-programs (WRITES).
So, if you have only one field ('mode') for storing capability of device or
master, you cannot resolve above scenarios. This is where I realized that
we need to have separate tx_nbits and rx_nbits in spi_device and
spi_master also. (This is contrary to my own earliest posts where I
proposed reusing mode bits for keeping channel-width info).
> > [Pekon]: I think it is correct to assume all these SPI modes as mutually
> > exclusive from of_property() point of view?
> > Instead of defining separate of_properties(), you can define a single
> > of_property() , something like 'max-tx-nbits' and 'max-rx-nbits'
> > which can represent various modes like (DUAL, QUAD, 3WIRE, 4WIRE).
> >
> Yes, seems better. I will correct it.
>
> >> /* Device speed */
> >> prop = of_get_property(nc, "spi-max-frequency", &len);
> >> @@ -1316,6 +1324,12 @@ int spi_setup(struct spi_device *spi)
> >> /* help drivers fail *cleanly* when they need options
> >> * that aren't supported with their current master
> >> */
> >> + if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD))
> >> ||
> >> + ((spi->mode & SPI_RX_DUAL) && (spi->mode &
> >> SPI_RX_QUAD))) {
> >> + dev_err(&spi->dev,
> >> + "setup: can not select dual and quad at the same time\n");
> >> + return -EINVAL;
> >> + }
> > [Pekon]: this check would be not be required if you have single
> > of_property() for all modes.
> >
> >
> >> bad_bits = spi->mode & ~spi->master->mode_bits;
> >> if (bad_bits) {
> >> dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
> >> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> >> index 2e0655d..0b776de 100644
> >> --- a/drivers/spi/spidev.c
> >> +++ b/drivers/spi/spidev.c
> >> @@ -75,6 +75,9 @@ static DECLARE_BITMAP(minors, N_SPI_MINORS);
> >> | SPI_LSB_FIRST | SPI_3WIRE | SPI_LOOP \
> >> | SPI_NO_CS | SPI_READY)
> >>
> >> +#define SPI_EXTMODE_MASK (SPI_MODE_MASK | SPI_TX_DUAL \
> >> + | SPI_TX_QUAD | SPI_RX_DUAL |
> >> SPI_RX_QUAD)
> >> +
> >> struct spidev_data {
> >> dev_t devt;
> >> spinlock_t spi_lock;
> >> @@ -268,6 +271,8 @@ static int spidev_message(struct spidev_data
> *spidev,
> >> k_tmp->bits_per_word = u_tmp->bits_per_word;
> >> k_tmp->delay_usecs = u_tmp->delay_usecs;
> >> k_tmp->speed_hz = u_tmp->speed_hz;
> >> + k_tmp->tx_nbits = u_tmp->tx_nbits;
> >> + k_tmp->rx_nbits = u_tmp->rx_nbits;
> >> #ifdef VERBOSE
> >> dev_dbg(&spidev->spi->dev,
> >> " xfer len %zd %s%s%s%dbits %u usec %uHz\n",
> >> @@ -369,7 +374,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
> >> unsigned long arg)
> >> case SPI_IOC_RD_MAX_SPEED_HZ:
> >> retval = __put_user(spi->max_speed_hz, (__u32 __user
> >> *)arg);
> >> break;
> >> -
> >> + case SPI_IOC_EXTRD_MODE:
> >> + retval = __put_user(spi->mode & SPI_EXTMODE_MASK,
> >> + (__u16 __user *)arg);
> >> + break;
> >> /* write requests */
> >> case SPI_IOC_WR_MODE:
> >> retval = __get_user(tmp, (u8 __user *)arg);
> >> @@ -433,6 +441,25 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
> >> unsigned long arg)
> >> dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
> >> }
> >> break;
> >> + case SPI_IOC_EXTWR_MODE:
> >> + retval = __get_user(tmp, (u16 __user *)arg);
> >> + if (retval == 0) {
> >> + u16 save = spi->mode;
> >> +
> >> + if (tmp & ~SPI_EXTMODE_MASK) {
> >> + retval = -EINVAL;
> >> + break;
> >> + }
> >> +
> >> + tmp |= spi->mode & ~SPI_EXTMODE_MASK;
> >> + spi->mode = (u16)tmp;
> >> + retval = spi_setup(spi);
> >> + if (retval < 0)
> >> + spi->mode = save;
> >> + else
> >> + dev_dbg(&spi->dev, "spi mode %02x\n",
> >> tmp);
> >> + }
> >> + break;
> >>
> >> default:
> >> /* segmented and/or full-duplex I/O request */
> >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> >> index 38c2b92..222e49e 100644
> >> --- a/include/linux/spi/spi.h
> >> +++ b/include/linux/spi/spi.h
> >> @@ -74,7 +74,7 @@ struct spi_device {
> >> struct spi_master *master;
> >> u32 max_speed_hz;
> >> u8 chip_select;
> >> - u8 mode;
> >> + u16 mode;
> >> #define SPI_CPHA 0x01 /* clock phase */
> >> #define SPI_CPOL 0x02 /* clock polarity */
> >> #define SPI_MODE_0 (0|0) /* (original
> >> MicroWire) */
> >> @@ -87,6 +87,10 @@ struct spi_device {
> >> #define SPI_LOOP 0x20 /* loopback mode */
> >> #define SPI_NO_CS 0x40 /* 1 dev/bus, no
> >> chipselect */
> >> #define SPI_READY 0x80 /* slave pulls low to
> >> pause */
> >> +#define SPI_TX_DUAL 0x100 /* transmit with 2
> >> wires */
> >> +#define SPI_TX_QUAD 0x200 /* transmit with 4
> >> wires */
> >> +#define SPI_RX_DUAL 0x400 /* receive with 2
> >> wires */
> >> +#define SPI_RX_QUAD 0x800 /* receive with 4
> >> wires */
> >> u8 bits_per_word;
> >> int irq;
> >> void *controller_state;
> >> @@ -437,6 +441,8 @@ 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
> >> + * @rx_nbits: number of bits used for reading
> >> * @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 +497,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
> >> @@ -511,6 +522,11 @@ struct spi_transfer {
> >> dma_addr_t rx_dma;
> >>
> >> unsigned cs_change:1;
> >> + u8 tx_nbits;
> >> + u8 rx_nbits;
> >> +#define SPI_NBITS_SINGLE 0x0; /* 1bit transfer */
> >> +#define SPI_NBITS_DUAL 0x01; /* 2bits transfer */
> >> +#define SPI_NBITS_QUAD 0x02; /* 4bits transfer */
> >
> > [Pekon]: I don't think it's a good place for #defines. Plz keep it out of struct
> body.
> >
> >> u8 bits_per_word;
> >> u16 delay_usecs;
> >> u32 speed_hz;
> >> @@ -858,7 +874,7 @@ struct spi_board_info {
> >> /* mode becomes spi_device.mode, and is essential for chips
> >> * where the default of SPI_CS_HIGH = 0 is wrong.
> >> */
> >> - u8 mode;
> >> + u16 mode;
> >>
> >> /* ... may need additional spi_device chip config data here.
> >> * avoid stuff protocol drivers can set; but include stuff
> >
> >
with regards, pekon
More information about the linux-mtd
mailing list