[PATCH] SPI: DUAL and QUAD support
yuhang wang
wangyuhang2014 at gmail.com
Mon Jul 29 10:52:14 EDT 2013
Hi, Pekon
2013/7/29 Gupta, Pekon <pekon at ti.com>:
> 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).
>
>
>
To the above two scenarios, it is necessary to modify the slave driver(m25p80.c)
what about this:
1,set spi_master->mode | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD;
2,set spi_device->mode | SPI_TX_QUAD | SPI_RX_QUAD(if use 4x read and
4x write) through DT.
3,add your own config and code in slave driver, example m25p80 read and write.
if (spi->mode | QUAD)
cmd and addr: tx_nbits=1;
data: tx_nbits = 4;
because no standard for dual and quad to serial flash, so no general
code for all flash.
I just think to provide a spi interface to let flash can set its own
transfer bits.
But now we have to correct the slave driver based on ourselves.
So if you modify your slave driver correctly, case1 and 2 won't happen.
If you use tx_nbits or something similar in spi_master and spi_device,
But the procedure may be same to above.
Best regards.
>> > [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