| Message ID | 1422356608-15961-2-git-send-email-bhuvanchandra.dv@toradex.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 27, 2015 at 04:33:28PM +0530, Bhuvanchandra DV wrote: > TCFQ is interrupt of Transfer Complete Flag in DSPI module. > EOQ is interrupt of End of Queue Flag in DSPI module. > For adopting of different platform, either of them is a way of DSPI > transfer data. This patch add TCF support for DSPI module in other platform. I can't really understand what the purpose of this is, sorry. What are these modes and why would one choose one over the other? Your changelog suggests this is being done per board which implies that it's tuning for performance but that's usually something that should be done in device drivers, not the device tree - perhaps the tuning will change over time as the driver is optimized or perhaps it varies depending on the transfer. The code mentions a DMA option as well, generally where it's available DMA is something that's chosen at runtime based on the length of the transfer.
On 01/29/2015 12:33 AM, Mark Brown wrote: > On Tue, Jan 27, 2015 at 04:33:28PM +0530, Bhuvanchandra DV wrote: > >> TCFQ is interrupt of Transfer Complete Flag in DSPI module. >> EOQ is interrupt of End of Queue Flag in DSPI module. >> For adopting of different platform, either of them is a way of DSPI >> transfer data. This patch add TCF support for DSPI module in other platform. > I can't really understand what the purpose of this is, sorry. What are > these modes and why would one choose one over the other? Your changelog > suggests this is being done per board which implies that it's tuning for > performance but that's usually something that should be done in device > drivers, not the device tree - perhaps the tuning will change over time > as the driver is optimized or perhaps it varies depending on the > transfer. > > The code mentions a DMA option as well, generally where it's available > DMA is something that's chosen at runtime based on the length of the > transfer. As far as i understood the major difference between the two modes are when the interrupt to trigger, as EOQ mode will trigger the interrupt at end of queue and TCF will trigger the interrupt at every frame transfer. Probably mode selection shouldn't be a device tree property, but i don't see any automatic way to choose between the modes. Maybe a config would be more appropriate? @Chao, is there any specific reason to implement this mode?
On Thu, Jan 29, 2015 at 11:58:25AM +0000, BhuvanChandra.DV wrote: > As far as i understood the major difference between the two modes are when > the interrupt to trigger, as EOQ mode will trigger the interrupt at end of > queue and TCF will trigger the interrupt at every frame transfer. Probably > mode selection shouldn't be a device tree property, but i don't see any > automatic way to choose between the modes. > Maybe a config would be more appropriate? Or if there's either no particular reason to choose one over the other or one is always better then just pick one in the driver and don't worry about implementing both.
On 01/29/2015 05:43 PM, Mark Brown wrote: > On Thu, Jan 29, 2015 at 11:58:25AM +0000, BhuvanChandra.DV wrote: > >> As far as i understood the major difference between the two modes are when >> the interrupt to trigger, as EOQ mode will trigger the interrupt at end of >> queue and TCF will trigger the interrupt at every frame transfer. Probably >> mode selection shouldn't be a device tree property, but i don't see any >> automatic way to choose between the modes. >> Maybe a config would be more appropriate? > Or if there's either no particular reason to choose one over the other > or one is always better then just pick one in the driver and don't worry > about implementing both. Among the two, EOQ would be better since with TCF, interrupts are generated at every frame transfer, which could be a problem at higher frequencies. I think we can omit this patch then.
On 2015-01-29 13:53, BhuvanChandra.DV wrote: > On 01/29/2015 05:43 PM, Mark Brown wrote: >> On Thu, Jan 29, 2015 at 11:58:25AM +0000, BhuvanChandra.DV wrote: >> >>> As far as i understood the major difference between the two modes are when >>> the interrupt to trigger, as EOQ mode will trigger the interrupt at end of >>> queue and TCF will trigger the interrupt at every frame transfer. Probably >>> mode selection shouldn't be a device tree property, but i don't see any >>> automatic way to choose between the modes. >>> Maybe a config would be more appropriate? >> Or if there's either no particular reason to choose one over the other >> or one is always better then just pick one in the driver and don't worry >> about implementing both. > > Among the two, EOQ would be better since with TCF, interrupts are generated at > every frame transfer, which could be a problem at higher frequencies. > I think we can omit this patch then. It would be interesting to know what the authors original motivation implementing this feature was. Reading the email of the original patchset indicates that there are platforms where only TCF is supported: <quote> For adopting of different platform, either of them is a way of DSPI transfer data. </quote> However, I don't know which platform that would be. Also, it seems that Chao Fu's email is not valid anymore. @Xiubo Li, maybe you can shed some light on this? From the platform I am concerned about, Vybrid, it seems not very useful, so I'm fine with omitting that patch. -- Stefan
On Thu, Jan 29, 2015 at 07:10:31PM +0100, Stefan Agner wrote: > It would be interesting to know what the authors original motivation > implementing this feature was. Reading the email of the original > patchset indicates that there are platforms where only TCF is supported: > <quote> > For adopting of different platform, either of them is a way of DSPI > transfer data. > </quote> > However, I don't know which platform that would be. Also, it seems that > Chao Fu's email is not valid anymore. It's not clear to me if the above means that this is due to hardware differences or due to tuning for the platform.
diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt index cbbe16e..b50439f 100644 --- a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt +++ b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt @@ -15,6 +15,8 @@ Optional property: - big-endian: If present the dspi device's registers are implemented in big endian mode, otherwise in native mode(same with CPU), for more detail please see: Documentation/devicetree/bindings/regmap/regmap.txt. +- tcfq-mode: If present, the data transfer will be done at TCFQ interrupt. + By default, driver chooses EOQ interrupt if 'tcfq-mode' property was not set. Example: diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index e0ce906..feca2fd 100644 --- a/drivers/spi/spi-fsl-dspi.c +++ b/drivers/spi/spi-fsl-dspi.c @@ -63,9 +63,11 @@ #define SPI_CTAR0_SLAVE 0x0c #define SPI_SR 0x2c +#define SPI_SR_TCFQF 0x80000000 #define SPI_SR_EOQF 0x10000000 #define SPI_RSER 0x30 +#define SPI_RSER_TCFQE 0x80000000 #define SPI_RSER_EOQFE 0x10000000 #define SPI_PUSHR 0x34 @@ -105,6 +107,12 @@ struct chip_data { u16 void_write_data; }; +enum dspi_trans_mode { + DSPI_EOQ_MODE = 0, + DSPI_TCFQ_MODE, + DSPI_DMA_MODE, /*TODO*/ +}; + struct fsl_dspi { struct spi_master *master; struct platform_device *pdev; @@ -125,6 +133,7 @@ struct fsl_dspi { u8 cs; u16 void_write_data; u32 cs_change; + enum dspi_trans_mode trans_mode; wait_queue_head_t waitq; u32 waitflags; @@ -225,7 +234,7 @@ static void dspi_data_from_popr(struct fsl_dspi *dspi, int rx_word) } } -static int dspi_transfer_write(struct fsl_dspi *dspi) +static int dspi_eoq_write(struct fsl_dspi *dspi) { int tx_count = 0; int tx_word; @@ -269,7 +278,7 @@ static int dspi_transfer_write(struct fsl_dspi *dspi) return tx_count * (tx_word + 1); } -static int dspi_transfer_read(struct fsl_dspi *dspi) +static int dspi_eoq_read(struct fsl_dspi *dspi) { int rx_count = 0; int rx_word = is_double_byte_mode(dspi); @@ -283,6 +292,37 @@ static int dspi_transfer_read(struct fsl_dspi *dspi) return rx_count; } +static int dspi_tcfq_write(struct fsl_dspi *dspi) +{ + int tx_word; + u32 dspi_pushr = 0; + + tx_word = is_double_byte_mode(dspi); + + if (tx_word && (dspi->len == 1)) { + dspi->dataflags |= TRAN_STATE_WORD_ODD_NUM; + regmap_update_bits(dspi->regmap, SPI_CTAR(dspi->cs), + SPI_FRAME_BITS_MASK, SPI_FRAME_BITS(8)); + tx_word = 0; + } + + dspi_pushr = dspi_data_to_pushr(dspi, tx_word); + + if ((dspi->cs_change) && (!dspi->len)) + dspi_pushr &= ~SPI_PUSHR_CONT; + + regmap_write(dspi->regmap, SPI_PUSHR, dspi_pushr); + + return tx_word + 1; +} + +static void dspi_tcfq_read(struct fsl_dspi *dspi) +{ + int rx_word = is_double_byte_mode(dspi); + + dspi_data_from_popr(dspi, rx_word); +} + static int dspi_transfer_one_message(struct spi_master *master, struct spi_message *message) { @@ -326,8 +366,13 @@ static int dspi_transfer_one_message(struct spi_master *master, regmap_write(dspi->regmap, SPI_CTAR(dspi->cs), dspi->cur_chip->ctar_val); - regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_EOQFE); - message->actual_length += dspi_transfer_write(dspi); + if (dspi->trans_mode == DSPI_EOQ_MODE) { + regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_EOQFE); + message->actual_length += dspi_eoq_write(dspi); + } else if (dspi->trans_mode == DSPI_TCFQ_MODE) { + regmap_write(dspi->regmap, SPI_RSER, SPI_RSER_TCFQE); + message->actual_length += dspi_tcfq_write(dspi); + } if (wait_event_interruptible(dspi->waitq, dspi->waitflags)) dev_err(&dspi->pdev->dev, "wait transfer complete fail!\n"); @@ -399,8 +444,13 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id) struct spi_message *msg = dspi->cur_msg; - regmap_write(dspi->regmap, SPI_SR, SPI_SR_EOQF); - dspi_transfer_read(dspi); + if (dspi->trans_mode == DSPI_EOQ_MODE) { + regmap_write(dspi->regmap, SPI_SR, SPI_SR_EOQF); + dspi_eoq_read(dspi); + } else if (dspi->trans_mode == DSPI_TCFQ_MODE) { + regmap_write(dspi->regmap, SPI_SR, SPI_SR_TCFQF); + dspi_tcfq_read(dspi); + } if (!dspi->len) { if (dspi->dataflags & TRAN_STATE_WORD_ODD_NUM) @@ -409,8 +459,12 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id) dspi->waitflags = 1; wake_up_interruptible(&dspi->waitq); - } else - msg->actual_length += dspi_transfer_write(dspi); + } else { + if (dspi->trans_mode == DSPI_EOQ_MODE) + msg->actual_length += dspi_eoq_write(dspi); + else if (dspi->trans_mode == DSPI_TCFQ_MODE) + msg->actual_length += dspi_tcfq_write(dspi); + } return IRQ_HANDLED; } @@ -470,6 +524,7 @@ static int dspi_probe(struct platform_device *pdev) dspi = spi_master_get_devdata(master); dspi->pdev = pdev; dspi->master = master; + dspi->trans_mode = DSPI_EOQ_MODE; master->transfer = NULL; master->setup = dspi_setup; @@ -481,6 +536,9 @@ static int dspi_probe(struct platform_device *pdev) master->bits_per_word_mask = SPI_BPW_MASK(4) | SPI_BPW_MASK(8) | SPI_BPW_MASK(16); + if (of_property_read_bool(np, "tcfq-mode")) + dspi->trans_mode = DSPI_TCFQ_MODE; + ret = of_property_read_u32(np, "spi-num-chipselects", &cs_num); if (ret < 0) { dev_err(&pdev->dev, "can't get spi-num-chipselects\n");