Hi Hiep-san,
On Tue, Oct 14, 2014 at 2:57 AM, Cao Minh Hiep <[email protected]> wrote:
> From: Hiep Cao Minh <[email protected]>
>
> In order to transmit and receive data when have 32 bytes of data that
> ready has prepared on Transmit/Receive Buffer to transmit or receive.
> Instead transmits/receives a byte data using Transmit/Receive Buffer
> Data Triggering Number will improve the speed of transfer data.
>
> Signed-off-by: Hiep Cao Minh <[email protected]>
Thanks for your patch!
> --- a/drivers/spi/spi-rspi.c
> +++ b/drivers/spi/spi-rspi.c
> @@ -182,6 +182,13 @@
> #define SPBFCR_RXRST 0x40 /* Receive Buffer Data Reset */
> #define SPBFCR_TXTRG_MASK 0x30 /* Transmit Buffer Data Triggering
> Number */
> #define SPBFCR_RXTRG_MASK 0x07 /* Receive Buffer Data Triggering
> Number */
> +/* QSPI on R-Car H2 */
This applies not only to H2, but to all R-Car Gen2.
> +#define SPBFCR_TXTRG_32B 0x00 /* 32Byte Transmit Buffer Triggering
> */
> +#define SPBFCR_TXTRG_1B 0x30 /* 1Byte Transmit Buffer
> Triggering */
The "32B" and "1B" don't match the documentation I have, which says for bits
5 and 4:
"When the number of bytes of data in the transmit buffer (SPTXB) is equal
to or less than the specified triggering number, the SPTEF flag is set to 1.
00: 31 bytes (1 byte available)
11: 0 byte (32 bytes available)"
(of course this could be attributed to a bad translation from Japanese
to English ;-)
See also qspi_set_send_trigger() below...
> +#define SPBFCR_RXTRG_1B 0x00 /* 32Byte Receive Buffer
> Triggering */
> +#define SPBFCR_RXTRG_32B 0x07 /* 1Byte Receive Buffer Triggering */
The comments seem to be swapped?
> @@ -371,6 +378,52 @@ static int qspi_set_config_register(struct rspi_data
> *rspi, int access_size)
> return 0;
> }
>
> +static void qspi_update(const struct rspi_data *rspi, u8 mask, u8 val, u8
> reg)
> +{
> + u8 data;
> +
> + data = rspi_read8(rspi, reg);
> + data &= ~mask;
> + data |= (val & mask);
> + rspi_write8(rspi, data, reg);
> +}
> +
> +static int qspi_set_send_trigger(struct rspi_data *rspi, int len)
unsigned int len
> +{
> + int n;
unsigned int n;
> +
> + n = min(len, QSPI_BUFFER_SIZE);
> +
> + if (len >= QSPI_BUFFER_SIZE) {
> + /* sets triggering number to 32 bytes */
> + qspi_update(rspi, SPBFCR_TXTRG_MASK,
> + SPBFCR_TXTRG_32B, QSPI_SPBFCR);
> + } else {
> + /* sets triggering number to 1 byte */
> + qspi_update(rspi, SPBFCR_TXTRG_MASK,
> + SPBFCR_TXTRG_1B, QSPI_SPBFCR);
> + }
Haven't you swapped the two branches of the if statement?
If len >= QSPI_BUFFER_SIZE, I'd expect you only want to be woken up
if there are 32 available entries in the FIFO, i.e. when bits 5 and 4 are both
one, which is the case for your definition of SPBFCR_TXTRG_1B.
> +
> + return n;
> +}
> +
> +static void qspi_set_receive_trigger(struct rspi_data *rspi, int len)
unsigned int len
> +{
> + int n;
unsigned int n;
> +
> + n = min(len, QSPI_BUFFER_SIZE);
> +
> + if (len >= QSPI_BUFFER_SIZE) {
> + /* sets triggering number to 32 bytes */
> + qspi_update(rspi, SPBFCR_RXTRG_MASK,
> + SPBFCR_RXTRG_32B, QSPI_SPBFCR);
> + } else {
> + /* sets triggering number to 1 byte */
> + qspi_update(rspi, SPBFCR_RXTRG_MASK,
> + SPBFCR_RXTRG_1B, QSPI_SPBFCR);
> + }
> +}
> +
> #define set_config_register(spi, n) spi->ops->set_config_register(spi, n)
>
> static void rspi_enable_irq(const struct rspi_data *rspi, u8 enable)
> @@ -410,27 +463,40 @@ static inline int rspi_wait_for_rx_full(struct
> rspi_data *rspi)
> return rspi_wait_for_interrupt(rspi, SPSR_SPRF, SPCR_SPRIE);
> }
>
> -static int rspi_data_out(struct rspi_data *rspi, u8 data)
> +static int rspi_wait_for_tx_empty_check(struct rspi_data *rspi)
> {
> int error = rspi_wait_for_tx_empty(rspi);
> if (error < 0) {
> dev_err(&rspi->master->dev, "transmit timeout\n");
> return error;
> }
Perhaps the error check should just be moved inside rspi_wait_for_tx_empty(),
so you don't have to introduce a new function?
> - rspi_write_data(rspi, data);
> +
> return 0;
> }
>
> -static int rspi_data_in(struct rspi_data *rspi)
> +static int rspi_wait_for_rx_full_check(struct rspi_data *rspi)
> {
> - int error;
> - u8 data;
> -
> - error = rspi_wait_for_rx_full(rspi);
> + int error = rspi_wait_for_rx_full(rspi);
> if (error < 0) {
> dev_err(&rspi->master->dev, "receive timeout\n");
> return error;
Perhaps the error check should just be moved inside rspi_wait_for_tx_full(),
so you don't have to introduce a new function?
> }
> +
> + return 0;
> +}
> +
> +static int rspi_data_out(struct rspi_data *rspi, u8 data)
> +{
> + rspi_wait_for_tx_empty_check(rspi);
You forgot to check the return value of rspi_wait_for_tx_empty_check()?
> + rspi_write_data(rspi, data);
> + return 0;
> +}
> +
> +static int rspi_data_in(struct rspi_data *rspi)
> +{
> + u8 data;
> +
> + rspi_wait_for_rx_full_check(rspi);
You forgot to check the return value of rspi_wait_for_rx_full_check()?
> data = rspi_read_data(rspi);
> return data;
> }
> @@ -614,19 +680,28 @@ static bool rspi_can_dma(struct spi_master *master,
> struct spi_device *spi,
> return __rspi_can_dma(rspi, xfer);
> }
>
> -static int rspi_common_transfer(struct rspi_data *rspi,
> - struct spi_transfer *xfer)
> +static int rspi_dma_check_then_transfer(struct rspi_data *rspi,
> + struct spi_transfer *xfer)
> {
> - int ret;
> -
> if (rspi->master->can_dma && __rspi_can_dma(rspi, xfer)) {
> /* rx_buf can be NULL on RSPI on SH in TX-only Mode */
> - ret = rspi_dma_transfer(rspi, &xfer->tx_sg,
> + int ret = rspi_dma_transfer(rspi, &xfer->tx_sg,
> xfer->rx_buf ? &xfer->rx_sg : NULL);
> if (ret != -EAGAIN)
> return ret;
This returns zero on success...
> }
>
> + return 0;
... but this also returns zero if DMA cannot be used?
Shouldn't you return -EAGAIN here?
> +}
> +
> +static int rspi_common_transfer(struct rspi_data *rspi,
> + struct spi_transfer *xfer)
> +{
> + int ret;
> +
> + ret = rspi_dma_check_then_transfer(rspi, xfer);
> + if (ret < 0)
> + return ret;
As rspi_dma_check_then_transfer() returns zero on success,
it will continue below using PIO?
> ret = rspi_pio_transfer(rspi, xfer->tx_buf, xfer->rx_buf, xfer->len);
> if (ret < 0)
> return ret;
> @@ -666,12 +741,49 @@ static int rspi_rz_transfer_one(struct spi_master
> *master,
> return rspi_common_transfer(rspi, xfer);
> }
>
> +static int qspi_trigger_transfer_out_int(struct rspi_data *rspi, const u8
> *tx,
> + u8 *rx, unsigned int len)
> +{
> + int i, n, ret;
unsigned int i, n;
> + while (len > 0) {
> + n = qspi_set_send_trigger(rspi, len);
> + qspi_set_receive_trigger(rspi, len);
> + if (n == QSPI_BUFFER_SIZE) {
> + rspi_wait_for_tx_empty_check(rspi);
> + for (i = 0; i < n; i++)
> + rspi_write_data(rspi, *tx++);
> + rspi_wait_for_rx_full_check(rspi);
> + for (i = 0; i < n; i++)
> + *rx++ = rspi_read_data(rspi);
> + } else {
> + ret = rspi_pio_transfer(rspi, tx, rx, n);
> + if (ret < 0)
> + return ret;
> + }
> + len -= n;
> + }
> +
> + return 0;
> +}
> +
> static int qspi_transfer_out_in(struct rspi_data *rspi,
> struct spi_transfer *xfer)
> {
> + int ret;
> +
> qspi_receive_init(rspi);
>
> - return rspi_common_transfer(rspi, xfer);
> + ret = rspi_dma_check_then_transfer(rspi, xfer);
> + if (ret < 0)
> + return ret;
As rspi_dma_check_then_transfer() returns zero on success,
it will continue below using PIO?
> +
> + ret = qspi_trigger_transfer_out_int(rspi, xfer->tx_buf,
> + xfer->rx_buf, xfer->len);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
