On Fri, 11 Dec 2020 13:16:00 -0800
Sowjanya Komatineni <skomatineni@xxxxxxxxxx> wrote:
> Tegra Quad SPI controller hardware supports sending dummy cycles
> after address bytes.
>
> This patch adds this support.
>
> Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
> ---
> drivers/spi/spi-tegra210-quad.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
> index 624f395..1d1b125 100644
> --- a/drivers/spi/spi-tegra210-quad.c
> +++ b/drivers/spi/spi-tegra210-quad.c
> @@ -124,6 +124,13 @@
> #define QSPI_DMA_TIMEOUT (msecs_to_jiffies(1000))
> #define DEFAULT_QSPI_DMA_BUF_LEN (64 * 1024)
>
> +enum transfer_phase {
> + CMD_BYTE_XFER = 0,
> + ADDR_BYTES_XFER,
> + DATA_BYTES_XFER,
> + MAX_XFERS,
> +};
> +
> struct tegra_qspi_client_data {
> int tx_clk_tap_delay;
> int rx_clk_tap_delay;
> @@ -857,6 +864,8 @@ static int tegra_qspi_start_transfer_one(struct spi_device *spi,
>
> tqspi->command1_reg = command1;
>
> + tegra_qspi_writel(tqspi, QSPI_NUM_DUMMY_CYCLE(tqspi->dummy_cycles), QSPI_MISC_REG);
> +
> ret = tegra_qspi_flush_fifos(tqspi, false);
> if (ret < 0)
> return ret;
> @@ -977,7 +986,7 @@ static int tegra_qspi_transfer_one_message(struct spi_master *master, struct spi
> struct spi_device *spi = msg->spi;
> struct spi_transfer *xfer;
> bool is_first_msg = true;
> - int ret;
> + int ret, xfer_phase = 0;
>
> msg->status = 0;
> msg->actual_length = 0;
> @@ -987,6 +996,15 @@ static int tegra_qspi_transfer_one_message(struct spi_master *master, struct spi
> list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> u32 cmd1;
>
> + /*
> + * Program dummy clock cycles in Tegra QSPI register only
> + * during address transfer phase.
> + */
> + if (xfer_phase == ADDR_BYTES_XFER)
> + tqspi->dummy_cycles = msg->dummy_cycles;
> + else
> + tqspi->dummy_cycles = 0;
That's fragile. You're trying to guess the phase (which is clearly a
spi-mem concept) from the position of the transfer in the list. What
happens if a spi-mem operation has no address bytes but requires dummy
cycles after the command? What happens if we patch spi_mem_exec_op() to
merge the cmd and address bytes in a single transfer (that's an option
I considered at some point when designing the framework before deciding
it was not worth the extra complexity)?
Besides, I keep thinking the regular transfer path should not assume
it's being passed spi-mem operations, if it is, that means you should
overload the default exec_op(). The more I look at it the less I like
this idea of adding a dummy_cycles field to spi_message. I'm pretty
sure we can find other ways to avoid code duplication if that's your
main concern.
> +
> reinit_completion(&tqspi->xfer_completion);
>
> cmd1 = tegra_qspi_setup_transfer_one(spi, xfer, is_first_msg);
> @@ -1018,6 +1036,7 @@ static int tegra_qspi_transfer_one_message(struct spi_master *master, struct spi
> }
>
> msg->actual_length += xfer->len;
> + xfer_phase++;
>
> complete_xfer:
> if (ret < 0) {
> @@ -1203,6 +1222,7 @@ static int tegra_qspi_probe(struct platform_device *pdev)
> master->mode_bits = SPI_MODE_0 | SPI_MODE_3 | SPI_CS_HIGH |
> SPI_TX_DUAL | SPI_RX_DUAL | SPI_TX_QUAD | SPI_RX_QUAD;
> master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) | SPI_BPW_MASK(8);
> + master->flags = SPI_MASTER_USES_HW_DUMMY_CYCLES;
> master->setup = tegra_qspi_setup;
> master->cleanup = tegra_qspi_cleanup;
> master->transfer_one_message = tegra_qspi_transfer_one_message;
