by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v1 3/7] spi: qspi-tegra: Add support for Tegra210 QSPI controller

Hi Sowjanya,

I love your patch! Perhaps something to improve:

[auto build test WARNING on spi/for-next]
[also build test WARNING on robh/for-next tegra/for-next v5.10-rc6 next-20201201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Sowjanya-Komatineni/Add-Tegra-QSPI-driver/20201202-051629
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/a8da475b681c83105cdb8daf7408cc92aca9f65a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sowjanya-Komatineni/Add-Tegra-QSPI-driver/20201202-051629
git checkout a8da475b681c83105cdb8daf7408cc92aca9f65a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/spi/qspi-tegra.c: In function 'tegra_qspi_transfer_one_message':
>> drivers/spi/qspi-tegra.c:935:7: warning: variable 'skip' set but not used [-Wunused-but-set-variable]
935 | bool skip = false;
| ^~~~

vim +/skip +935 drivers/spi/qspi-tegra.c

925
926 static int tegra_qspi_transfer_one_message(struct spi_master *master,
927 struct spi_message *msg)
928 {
929 bool is_first_msg = true;
930 struct tegra_qspi_data *tqspi = spi_master_get_devdata(master);
931 struct spi_transfer *xfer;
932 struct spi_device *spi = msg->spi;
933 u8 dummy_cycles, ntransfers = 0;
934 int ret;
> 935 bool skip = false;
936 int xfer_phase = 0;
937
938 msg->status = 0;
939 msg->actual_length = 0;
940 tqspi->tx_status = 0;
941 tqspi->rx_status = 0;
942
943 /*
944 * Tegra QSPI hardware support dummy bytes transfer based on the
945 * programmed dummy clock cyles in QSPI register.
946 * So, get the total dummy bytes from the dummy bytes transfer in
947 * spi_messages and convert to dummy clock cyles.
948 */
949 list_for_each_entry(xfer, &msg->transfers, transfer_list) {
950 if (ntransfers == DUMMY_BYTES_XFER &&
951 !(list_is_last(&xfer->transfer_list, &msg->transfers)))
952 dummy_cycles = xfer->len * 8 / xfer->tx_nbits;
953 ntransfers++;
954 }
955
956 list_for_each_entry(xfer, &msg->transfers, transfer_list) {
957 u32 cmd1;
958
959 /*
960 * Program dummy clock cycles in Tegra QSPI register only
961 * during address transfer phase.
962 */
963 if (xfer_phase == ADDR_BYTES_XFER)
964 tqspi->dummy_cycles = dummy_cycles;
965 else
966 tqspi->dummy_cycles = 0;
967
968 /*
969 * Tegra QSPI hardware support dummy bytes transfer.
970 * So, skip the transfer of dummy bytes from the software.
971 */
972 if (ntransfers == MAX_XFERS &&
973 xfer_phase == DUMMY_BYTES_XFER) {
974 msg->actual_length += xfer->len;
975 xfer_phase++;
976 continue;
977 }
978
979 reinit_completion(&tqspi->xfer_completion);
980
981 cmd1 = tegra_qspi_setup_transfer_one(spi, xfer, is_first_msg);
982 if (!xfer->len) {
983 ret = 0;
984 skip = true;
985 goto exit;
986 }
987
988 ret = tegra_qspi_start_transfer_one(spi, xfer, cmd1);
989 if (ret < 0) {
990 dev_err(tqspi->dev,
991 "failed to start transfer: %d\n", ret);
992 goto exit;
993 }
994
995 is_first_msg = false;
996 ret = wait_for_completion_timeout(&tqspi->xfer_completion,
997 QSPI_DMA_TIMEOUT);
998 if (WARN_ON(ret == 0)) {
999 dev_err(tqspi->dev,
1000 "qspi transfer timeout: %d\n", ret);
1001 if (tqspi->is_curr_dma_xfer &&
1002 (tqspi->cur_direction & DATA_DIR_TX))
1003 dmaengine_terminate_all(tqspi->tx_dma_chan);
1004 if (tqspi->is_curr_dma_xfer &&
1005 (tqspi->cur_direction & DATA_DIR_RX))
1006 dmaengine_terminate_all(tqspi->rx_dma_chan);
1007 ret = -EIO;
1008 tegra_qspi_dump_regs(tqspi);
1009 tegra_qspi_flush_fifos(tqspi);
1010 reset_control_assert(tqspi->rst);
1011 udelay(2);
1012 reset_control_deassert(tqspi->rst);
1013 goto exit;
1014 }
1015
1016 if (tqspi->tx_status || tqspi->rx_status) {
1017 ret = -EIO;
1018 dev_err(tqspi->dev, "error in transfer: %d\n", ret);
1019 tegra_qspi_dump_regs(tqspi);
1020 goto exit;
1021 }
1022
1023 msg->actual_length += xfer->len;
1024 xfer_phase++;
1025 }
1026
1027 ret = 0;
1028 exit:
1029 tegra_qspi_writel(tqspi, tqspi->def_command1_reg, QSPI_COMMAND1);
1030 msg->status = ret;
1031 spi_finalize_current_message(master);
1032 return ret;
1033 }
1034

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (5.46 kB)
.config.gz (66.10 kB)
Download all attachments

2020-12-02 17:09:39

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v1 4/7] spi: qspi-tegra: Add Tegra186 and Tegra194 QSPI compatibles

On Tue, Dec 01, 2020 at 01:12:45PM -0800, Sowjanya Komatineni wrote:
> QSPI controller on Tegra186 and Tegra194 is similar to Tegra210
> QSPI controller in terms of transferring command, address and data
> in multiple transfers.

Given that you're adding the driver in the same series this doesn't
really seem like it needs a separate patch.

Attachments:

(No filename) (349.00 B)
signature.asc (499.00 B)
Download all attachments

2020-12-02 17:33:17

On Tue, Dec 01, 2020 at 01:12:44PM -0800, Sowjanya Komatineni wrote:
> Tegra SoC has a Quad SPI controller starting from Tegra210.
>
> This patch adds support for Tegra210 QSPI controller.
>
> Signed-off-by: Sowjanya Komatineni <[email protected]>
> ---
> drivers/spi/Kconfig | 9 +
> drivers/spi/Makefile | 1 +
> drivers/spi/qspi-tegra.c | 1418 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1428 insertions(+)
> create mode 100644 drivers/spi/qspi-tegra.c
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 3fd16b7..1a021e8 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -844,6 +844,15 @@ config SPI_MXS
> help
> SPI driver for Freescale MXS devices.
>
> +config QSPI_TEGRA

You already discussed this with Mark, but perhaps a better name would be
SPI_TEGRA210_QUAD or something. SPI_TEGRA210 is too generic because
there is a regular SPI controller on Tegra210 as well.

SPI_TEGRA210_QUAD is in line with the likes of SPI_TEGRA20_SFLASH and
SPI_TEGRA20_SLINK.

> + tristate "Nvidia Tegra QSPI Controller"

NVIDIA

> + depends on (ARCH_TEGRA && TEGRA20_APB_DMA) || COMPILE_TEST

I don't think we need the ARCH_TEGRA part there because TEGRA20_APB_DMA
already depends on that. Also, there's not strictly a dependency on
TEGRA20_APB_DMA specifically, but rather a dependency on DMA_ENGINE,
right? The DMA channels could be coming from some other driver on some
other SoC generation, such as the Tegra186 and later GPCDMA.

> + depends on RESET_CONTROLLER
> + help
> + QSPI driver for Nvidia Tegra QSPI Controller interface. This

NVIDIA

> + controller is different from the spi controller and is available

SPI

> + on Tegra SoCs starting from Tegra210.
> +
> config SPI_TEGRA114
> tristate "NVIDIA Tegra114 SPI Controller"
> depends on (ARCH_TEGRA && TEGRA20_APB_DMA) || COMPILE_TEST
[...]
> diff --git a/drivers/spi/qspi-tegra.c b/drivers/spi/qspi-tegra.c
[...]
> +struct tegra_qspi_client_data {
> + int tx_clk_tap_delay;
> + int rx_clk_tap_delay;
> +};

If this is client data, why are we dealing with this in the controller
driver? Is this perhaps something that could be added to struct
spi_device?

> +
> +struct tegra_qspi_data {

That _data just seems to be 5 extra characters that don't add any value.

> + struct device *dev;
> + struct spi_master *master;
> + /* Lock to protect data accessed by irq */
> + spinlock_t lock;
> +
> + struct clk *clk;
> + struct reset_control *rst;
> + void __iomem *base;
> + phys_addr_t phys;
> + unsigned int irq;
> +
> + u32 cur_speed;
> + unsigned int cur_pos;
> + unsigned int words_per_32bit;
> + unsigned int bytes_per_word;
> + unsigned int curr_dma_words;
> + unsigned int cur_direction;
> +
> + unsigned int cur_rx_pos;
> + unsigned int cur_tx_pos;
> +
> + unsigned int dma_buf_size;
> + unsigned int max_buf_size;
> + bool is_curr_dma_xfer;
> +
> + struct completion rx_dma_complete;
> + struct completion tx_dma_complete;
> +
> + u32 tx_status;
> + u32 rx_status;
> + u32 status_reg;
> + bool is_packed;
> + bool use_dma;
> +
> + u32 command1_reg;
> + u32 dma_control_reg;
> + u32 def_command1_reg;
> + u32 def_command2_reg;
> + u32 spi_cs_timing1;
> + u32 spi_cs_timing2;
> + u8 dummy_cycles;
> +
> + struct completion xfer_completion;
> + struct spi_transfer *curr_xfer;
> +
> + struct dma_chan *rx_dma_chan;
> + u32 *rx_dma_buf;
> + dma_addr_t rx_dma_phys;
> + struct dma_async_tx_descriptor *rx_dma_desc;
> +
> + struct dma_chan *tx_dma_chan;
> + u32 *tx_dma_buf;
> + dma_addr_t tx_dma_phys;
> + struct dma_async_tx_descriptor *tx_dma_desc;
> +};
> +
> +static int tegra_qspi_runtime_suspend(struct device *dev);
> +static int tegra_qspi_runtime_resume(struct device *dev);

Can't we just reorder the code so that these don't have to be forward-
declared?

> +
> +static inline u32 tegra_qspi_readl(struct tegra_qspi_data *tqspi,
> + unsigned long reg)

Nit: I prefer "offset" over "reg" because I think it's slightly more
accurate.

> +{
> + return readl(tqspi->base + reg);
> +}
> +
> +static inline void tegra_qspi_writel(struct tegra_qspi_data *tqspi,
> + u32 val, unsigned long reg)

I also prefer "value" over "val" because "val" could also be short for
"valid". Anyway, I am pedantic that way, so feel free to ignore that.

[...]
> +static unsigned int
> +tegra_qspi_calculate_curr_xfer_param(struct tegra_qspi_data *tqspi,
> + struct spi_transfer *t)
> +{
> + unsigned int remain_len = t->len - tqspi->cur_pos;
> + unsigned int max_word;
> + unsigned int bits_per_word = t->bits_per_word;
> + unsigned int max_len;
> + unsigned int total_fifo_words;

You could list some of these on the same line to make this a bit more
compact. Something I've often seen done that makes this really clean is
to have uninitialized variables on one line and then assignments on
separate lines, then sort lines by length:

unsigned int max_word, max_len, total_fifo_words;
unsigned int remain_len = t->len - tqspi->cur_pos;
unsigned int bits_per_word = t->bits_per_word;

This also applies to some other functions further down.

[...]
> +static void
> +tegra_qspi_copy_client_txbuf_to_qspi_txbuf(struct tegra_qspi_data *tqspi,
> + struct spi_transfer *t)
> +{
> + /* Make the dma buffer to read by cpu */

This comment seems a bit redundant. dma_sync_single_for_cpu() is a well-
documented function that doesn't need explanation.

> + dma_sync_single_for_cpu(tqspi->dev, tqspi->tx_dma_phys,
> + tqspi->dma_buf_size, DMA_TO_DEVICE);
> +
> + if (tqspi->is_packed) {
> + unsigned int len = tqspi->curr_dma_words *
> + tqspi->bytes_per_word;
> +
> + memcpy(tqspi->tx_dma_buf, t->tx_buf + tqspi->cur_pos, len);
> + tqspi->cur_tx_pos += tqspi->curr_dma_words *
> + tqspi->bytes_per_word;
> + } else {
> + u8 *tx_buf = (u8 *)t->tx_buf + tqspi->cur_tx_pos;
> + unsigned int i;
> + unsigned int count;
> + unsigned int consume;
> + unsigned int write_bytes;
> +
> + consume = tqspi->curr_dma_words * tqspi->bytes_per_word;
> + if (consume > t->len - tqspi->cur_pos)
> + consume = t->len - tqspi->cur_pos;
> + write_bytes = consume;
> + for (count = 0; count < tqspi->curr_dma_words; count++) {
> + u32 x = 0;
> +
> + for (i = 0; consume && (i < tqspi->bytes_per_word);
> + i++, consume--)
> + x |= (u32)(*tx_buf++) << (i * 8);
> + tqspi->tx_dma_buf[count] = x;
> + }
> +
> + tqspi->cur_tx_pos += write_bytes;
> + }
> +
> + /* Make the dma buffer to read by dma */

Same here.

> + dma_sync_single_for_device(tqspi->dev, tqspi->tx_dma_phys,
> + tqspi->dma_buf_size, DMA_TO_DEVICE);
> +}
> +
> +static void
> +tegra_qspi_copy_qspi_rxbuf_to_client_rxbuf(struct tegra_qspi_data *tqspi,
> + struct spi_transfer *t)
> +{
> + /* Make the dma buffer to read by cpu */

And here.

> + dma_sync_single_for_cpu(tqspi->dev, tqspi->rx_dma_phys,
> + tqspi->dma_buf_size, DMA_FROM_DEVICE);
> +
> + if (tqspi->is_packed) {
> + unsigned int len = tqspi->curr_dma_words *
> + tqspi->bytes_per_word;
> +
> + memcpy(t->rx_buf + tqspi->cur_rx_pos, tqspi->rx_dma_buf, len);
> + tqspi->cur_rx_pos += tqspi->curr_dma_words *
> + tqspi->bytes_per_word;
> + } else {
> + unsigned char *rx_buf = t->rx_buf + tqspi->cur_rx_pos;
> + u32 rx_mask = ((u32)1 << t->bits_per_word) - 1;
> + unsigned int i;
> + unsigned int count;
> + unsigned int consume;
> + unsigned int read_bytes;
> +
> + consume = tqspi->curr_dma_words * tqspi->bytes_per_word;
> + if (consume > t->len - tqspi->cur_pos)
> + consume = t->len - tqspi->cur_pos;
> + read_bytes = consume;
> + for (count = 0; count < tqspi->curr_dma_words; count++) {
> + u32 x = tqspi->rx_dma_buf[count] & rx_mask;
> +
> + for (i = 0; consume && (i < tqspi->bytes_per_word);
> + i++, consume--)
> + *rx_buf++ = (x >> (i * 8)) & 0xFF;
> + }
> +
> + tqspi->cur_rx_pos += read_bytes;
> + }
> +
> + /* Make the dma buffer to read by dma */

And here.

> + dma_sync_single_for_device(tqspi->dev, tqspi->rx_dma_phys,
> + tqspi->dma_buf_size, DMA_FROM_DEVICE);
> +}
> +
> +static void tegra_qspi_dma_complete(void *args)
> +{
> + struct completion *dma_complete = args;
> +
> + complete(dma_complete);
> +}
> +
> +static int tegra_qspi_start_tx_dma(struct tegra_qspi_data *tqspi, int len)
> +{
> + reinit_completion(&tqspi->tx_dma_complete);
> + tqspi->tx_dma_desc = dmaengine_prep_slave_single(tqspi->tx_dma_chan,
> + tqspi->tx_dma_phys, len, DMA_MEM_TO_DEV,
> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);

Looks like most of this driver is wrapped at 80 columns. That rule was
relaxed a bit a while ago and checkpatch now only warns if you exceed
100 columns. There are various places in this driver that could benefit
from longer lines, such as the above.

> + if (!tqspi->tx_dma_desc) {
> + dev_err(tqspi->dev, "Not able to get desc for Tx\n");

Perhaps: "Unable to get TX descriptor\n"?

> + return -EIO;
> + }
> +
> + tqspi->tx_dma_desc->callback = tegra_qspi_dma_complete;
> + tqspi->tx_dma_desc->callback_param = &tqspi->tx_dma_complete;
> +
> + dmaengine_submit(tqspi->tx_dma_desc);
> + dma_async_issue_pending(tqspi->tx_dma_chan);
> + return 0;
> +}
> +
> +static int tegra_qspi_start_rx_dma(struct tegra_qspi_data *tqspi, int len)
> +{
> + reinit_completion(&tqspi->rx_dma_complete);
> + tqspi->rx_dma_desc = dmaengine_prep_slave_single(tqspi->rx_dma_chan,
> + tqspi->rx_dma_phys, len, DMA_DEV_TO_MEM,
> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> + if (!tqspi->rx_dma_desc) {
> + dev_err(tqspi->dev, "Not able to get desc for Rx\n");

Same here.

> + return -EIO;
> + }
> +
> + tqspi->rx_dma_desc->callback = tegra_qspi_dma_complete;
> + tqspi->rx_dma_desc->callback_param = &tqspi->rx_dma_complete;
> +
> + dmaengine_submit(tqspi->rx_dma_desc);
> + dma_async_issue_pending(tqspi->rx_dma_chan);
> + return 0;
> +}
> +
> +static int tegra_qspi_flush_fifos(struct tegra_qspi_data *tqspi)
> +{
> + unsigned long timeout = jiffies + HZ;
> + u32 status;
> +
> + status = tegra_qspi_readl(tqspi, QSPI_FIFO_STATUS);
> + if ((status & QSPI_FIFO_EMPTY) == QSPI_FIFO_EMPTY)
> + return 0;
> +
> + status |= QSPI_RX_FIFO_FLUSH | QSPI_TX_FIFO_FLUSH;
> + tegra_qspi_writel(tqspi, status, QSPI_FIFO_STATUS);
> + while ((status & QSPI_FIFO_EMPTY) != QSPI_FIFO_EMPTY) {
> + status = tegra_qspi_readl(tqspi, QSPI_FIFO_STATUS);
> + if (time_after(jiffies, timeout)) {
> + dev_err(tqspi->dev,
> + "timeout waiting for fifo flush\n");

FIFO is an abbreviation, so it should be all uppercase in text.

> + return -EIO;
> + }
> +
> + udelay(1);

It looks like this function can be called from both interrupt and
process contexts, where the latter is the more common case and it is
only ever used in interrupt context to clean up on error.

I wonder if it's worth adding an "atomic" parameter to the function and
make this a sleeping loop whenever possible. Also, can this not use one
of the helpers from linux/iopoll.h?

> + }
> +
> + return 0;
> +}
> +
> +static void tegra_qspi_unmask_irq(struct tegra_qspi_data *tqspi)
> +{
> + u32 intr_mask;
> +
> + intr_mask = tegra_qspi_readl(tqspi, QSPI_INTR_MASK);
> + intr_mask &= ~(QSPI_INTR_RDY_MASK | QSPI_INTR_RX_TX_FIFO_ERR);
> + tegra_qspi_writel(tqspi, intr_mask, QSPI_INTR_MASK);
> +}
> +
> +static int tegra_qspi_start_dma_based_transfer(struct tegra_qspi_data *tqspi,
> + struct spi_transfer *t)
> +{
> + u32 val;
> + unsigned int len;
> + int ret = 0;
> + u8 dma_burst;
> + struct dma_slave_config dma_sconfig = {0};

I think checkpatch wants spaces after { and before }.

> +
> + val = QSPI_DMA_BLK_SET(tqspi->curr_dma_words - 1);
> + tegra_qspi_writel(tqspi, val, QSPI_DMA_BLK);
> +
> + tegra_qspi_unmask_irq(tqspi);
> +
> + if (tqspi->is_packed)
> + len = DIV_ROUND_UP(tqspi->curr_dma_words *
> + tqspi->bytes_per_word, 4) * 4;
> + else
> + len = tqspi->curr_dma_words * 4;
> +
> + /* Set attention level based on length of transfer */
> + val = 0;
> + if (len & 0xF) {

Nit: hexadecimal literals are usually lowercase.

> + val |= QSPI_TX_TRIG_1 | QSPI_RX_TRIG_1;
> + dma_burst = 1;
> + } else if (((len) >> 4) & 0x1) {
> + val |= QSPI_TX_TRIG_4 | QSPI_RX_TRIG_4;
> + dma_burst = 4;
> + } else {
> + val |= QSPI_TX_TRIG_8 | QSPI_RX_TRIG_8;
> + dma_burst = 8;
> + }
> +
> + tegra_qspi_writel(tqspi, val, QSPI_DMA_CTL);
> + tqspi->dma_control_reg = val;
> +
> + dma_sconfig.device_fc = true;
> + if (tqspi->cur_direction & DATA_DIR_TX) {
> + dma_sconfig.dst_addr = tqspi->phys + QSPI_TX_FIFO;
> + dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> + dma_sconfig.dst_maxburst = dma_burst;
> + ret = dmaengine_slave_config(tqspi->tx_dma_chan, &dma_sconfig);
> + if (ret < 0) {
> + dev_err(tqspi->dev,
> + "DMA slave config failed: %d\n", ret);
> + return ret;
> + }
> +
> + tegra_qspi_copy_client_txbuf_to_qspi_txbuf(tqspi, t);
> + ret = tegra_qspi_start_tx_dma(tqspi, len);
> + if (ret < 0) {
> + dev_err(tqspi->dev,
> + "Starting tx dma failed: %d\n", ret);

"TX" and "DMA"

> + return ret;
> + }
> + }
> +
> + if (tqspi->cur_direction & DATA_DIR_RX) {
> + dma_sconfig.src_addr = tqspi->phys + QSPI_RX_FIFO;
> + dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> + dma_sconfig.src_maxburst = dma_burst;
> + ret = dmaengine_slave_config(tqspi->rx_dma_chan, &dma_sconfig);
> + if (ret < 0) {
> + dev_err(tqspi->dev,
> + "DMA slave config failed: %d\n", ret);
> + return ret;
> + }
> +
> + /* Make the dma buffer to read by dma */

Again, not a useful comment.

> + dma_sync_single_for_device(tqspi->dev, tqspi->rx_dma_phys,
> + tqspi->dma_buf_size,
> + DMA_FROM_DEVICE);
> +
> + ret = tegra_qspi_start_rx_dma(tqspi, len);
> + if (ret < 0) {
> + dev_err(tqspi->dev,
> + "Starting rx dma failed: %d\n", ret);

"RX" and "DMA"

[...]
> +static int tegra_qspi_init_dma_param(struct tegra_qspi_data *tqspi,
> + bool dma_to_memory)
> +{
> + struct dma_chan *dma_chan;
> + u32 *dma_buf;
> + dma_addr_t dma_phys;
> +
> + if (!device_property_present(tqspi->dev, "dmas"))
> + return 0;

If DMA support is optional, then we definitely don't want to depend on
TEGRA20_APB_DMA (or any other specific driver).

> +
> + dma_chan = dma_request_chan(tqspi->dev, dma_to_memory ? "rx" : "tx");

Does this return some specific value when there's no channel for this?
I.e. what happens if the "dmas" property exists in device tree but we
don't have a driver for the provider enabled? Since we have code to use
as fallback, we may want to special case that here and allow the driver
to continue.

> + if (IS_ERR(dma_chan))
> + return dev_err_probe(tqspi->dev, PTR_ERR(dma_chan),
> + "Dma channel is not available\n");

"DMA"

[...]
> +static int tegra_qspi_start_transfer_one(struct spi_device *spi,
> + struct spi_transfer *t, u32 command1)
> +{
[...]
> + command1 &= ~QSPI_INTERFACE_WIDTH_MASK;
> + if (bus_width == SPI_NBITS_QUAD)
> + command1 |= QSPI_INTERFACE_WIDTH_QUAD;
> + else if (bus_width == SPI_NBITS_DUAL)
> + command1 |= QSPI_INTERFACE_WIDTH_DUAL;
> + else
> + command1 |= QSPI_INTERFACE_WIDTH_SINGLE;
> + tqspi->command1_reg = command1;

This (and in some other places in the driver) could use a couple of
blank lines to make this less cluttered.

[...]
> +static void tegra_qspi_cleanup(struct spi_device *spi)
> +{
> + struct tegra_qspi_client_data *cdata = spi->controller_data;
> +
> + spi->controller_data = NULL;
> + if (spi->dev.of_node)

Can this ever fail? Do we support SPI device instantiation from
somewhere else than DT?

> + kfree(cdata);
> +}
> +
> +static int tegra_qspi_setup(struct spi_device *spi)
> +{
> + struct tegra_qspi_data *tqspi = spi_master_get_devdata(spi->master);
> + struct tegra_qspi_client_data *cdata = spi->controller_data;
> + u32 tx_tap = 0, rx_tap = 0;
> + u32 val;
> + unsigned long flags;
> + int ret;
> +
> + dev_dbg(&spi->dev, "setup %d bpw, %scpol, %scpha, %dHz\n",
> + spi->bits_per_word,
> + spi->mode & SPI_CPOL ? "" : "~",
> + spi->mode & SPI_CPHA ? "" : "~",
> + spi->max_speed_hz);
> +
> + if (!cdata) {
> + cdata = tegra_qspi_parse_cdata_dt(spi);

Oh... I see that this is actually parsed from the SPI device node, so
perhaps this is okay to do.

[...]
> +static int tegra_qspi_transfer_one_message(struct spi_master *master,
> + struct spi_message *msg)
> +{
[...]
> + is_first_msg = false;
> + ret = wait_for_completion_timeout(&tqspi->xfer_completion,
> + QSPI_DMA_TIMEOUT);
> + if (WARN_ON(ret == 0)) {
> + dev_err(tqspi->dev,
> + "qspi transfer timeout: %d\n", ret);

"QSPI", or alternatively leave this out completely because it should be
obvious that the transfer that failed is a QSPI transfer just from the
device name.

> + if (tqspi->is_curr_dma_xfer &&
> + (tqspi->cur_direction & DATA_DIR_TX))
> + dmaengine_terminate_all(tqspi->tx_dma_chan);
> + if (tqspi->is_curr_dma_xfer &&
> + (tqspi->cur_direction & DATA_DIR_RX))
> + dmaengine_terminate_all(tqspi->rx_dma_chan);
> + ret = -EIO;
> + tegra_qspi_dump_regs(tqspi);
> + tegra_qspi_flush_fifos(tqspi);
> + reset_control_assert(tqspi->rst);
> + udelay(2);
> + reset_control_deassert(tqspi->rst);
> + goto exit;
> + }
> +
> + if (tqspi->tx_status || tqspi->rx_status) {
> + ret = -EIO;
> + dev_err(tqspi->dev, "error in transfer: %d\n", ret);

Would it make sense to output tx_status and/or rx_status in this message
rather than -EIO unconditionally?

[...]
> +static irqreturn_t handle_cpu_based_xfer(struct tegra_qspi_data *tqspi)
> +{
> + struct spi_transfer *t = tqspi->curr_xfer;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tqspi->lock, flags);
> + if (tqspi->tx_status || tqspi->rx_status) {
> + dev_err(tqspi->dev, "CpuXfer ERROR bit set 0x%x\n",
> + tqspi->status_reg);
> + dev_err(tqspi->dev, "CpuXfer 0x%08x:0x%08x\n",
> + tqspi->command1_reg, tqspi->dma_control_reg);
> + tegra_qspi_dump_regs(tqspi);
> + tegra_qspi_flush_fifos(tqspi);
> + complete(&tqspi->xfer_completion);
> + spin_unlock_irqrestore(&tqspi->lock, flags);
> + reset_control_assert(tqspi->rst);
> + udelay(2);
> + reset_control_deassert(tqspi->rst);
> + return IRQ_HANDLED;
> + }

Can this error handling be somehow unified with the DMA error handling?
Having markers such as "CpuXfer" in error messages is not really helpful
to users. Maybe have the driver output an INFO message or so during
probe when it falls back to PIO mode, that would be a good enough
indicator to someone debugging some issue that PIO mode is being used.
And then you can just treat transfer failures more uniformly.

Thierry

Attachments:

(No filename) (18.87 kB)
signature.asc (849.00 B)
Download all attachments

2020-12-04 12:20:37

by Thierry Reding

[permalink] [raw]

Subject: Re: [PATCH v1 3/7] spi: qspi-tegra: Add support for Tegra210 QSPI controller

On Wed, Dec 02, 2020 at 05:27:21PM +0000, Mark Brown wrote:
> On Tue, Dec 01, 2020 at 01:12:44PM -0800, Sowjanya Komatineni wrote:
> > Tegra SoC has a Quad SPI controller starting from Tegra210.
> >
> > This patch adds support for Tegra210 QSPI controller.
>
> This looks pretty clean but I've got a few questions below about how
> this integrates with the frameworks as well as some more minor issues.
>
> > +config QSPI_TEGRA
> > + tristate "Nvidia Tegra QSPI Controller"
>
> Everything else in this file is SPI_, even the qspi controllers.
>
> > +++ b/drivers/spi/qspi-tegra.c
> > @@ -0,0 +1,1418 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
> > + */
>
> Please make the entire comment a C++ one. It also appears that the "All
> rights reserved" here conflicts with the GPL-2.0-only SPDX statement...

My understanding is that this phrase is pretty much irrelevant these
days. Furthermore, I don't think this is to be interpreted as a claim to
any rights other than the copyright, so it's mostly equivalent to the
"Copyright (C)" on that same line. Any license terms associated with the
file do still apply regardless.

That said, I'm not a lawyer, so don't take this as legal advice. FWIW,
there's something on the order of 8000 occurrences of that phrase in the
Linux kernel sources, so I think with or without the phrase we should be
okay.

Thierry

Attachments:

(No filename) (1.44 kB)
signature.asc (849.00 B)
Download all attachments

2020-12-04 12:28:52

On Thu, Dec 03, 2020 at 04:22:54PM -0800, Sowjanya Komatineni wrote:
> On 12/2/20 11:17 AM, Sowjanya Komatineni wrote:

> > > It seems weird that this device needs us to do a memcpy() to do DMA,
> > > most devices are able to DMA directly from the buffers provided by the
> > > SPI API (and let the SPI core sync things).? What is going on here?

> > For transfers of size more than max DMA transfer limit, data transfer
> > happens in multiple iterations with each iteration transferring up to
> > max DMA transfer limit.

> > So using separate dma buffers and on every iteration copying them to SPI
> > core provided tx/rx buffers.

I don't understand this - there's no restriction on where DMA transfers
can be done from within a DMA mapped region, the driver can do multiple
transfers from different chunks of the source buffer without having to
copy anything. That's a very common scenario.

> Also unpack mode needs to manually put the bytes together from read data to
> SPI core rx buffer.

Could you be more explicit here, I don't know what "unpack mode" is?

> > > This is worrying, the client device might be confused if /CS is doing
> > > things outside of the standard handling.

> > Do you mean to honor spi_transfer cs_change flag?

At least, yes - more generally just if there's any feature to with chip
select then the driver will need to open code it. The driver should at
least be double checking that what it's doing matches what it was told
to do, though just letting this be handled by the generic code if
there's no limitation on the hardware tends to be easier all round.

> > Tegra QSPI is master and is used only with QSPI flash devices. Looking
> > at SPI NOR driver, I see QSPI Flash commands are executed with one flash
> > command per spi_message and I dont see cs_change flag usage w.r.t QSPI
> > flash. So, using SW based CS control for QSPI.

> > Please correct me if I miss something to understand here.

Someone might build a system that does something different, they may see
a spare SPI controller and decide they can use it for something else or
there may be some future change with the flash code which does something
different.

> > > > +??? tegra_qspi_writel(tqspi, tqspi->def_command2_reg, QSPI_COMMAND2);

> > > The setup for one device shouldn't be able to affect the operation of
> > > another, already running, device so either these need to be configured
> > > as part of the controller probe or these configurations need to be
> > > deferred until we're actually doing a transfer.

> > We will only have 1 device on QSPI as we only support single chip select.

It's quite common for people to do things like add additional devices
with GPIO chip selects.

> > > > +??? /*
> > > > +???? * Tegra QSPI hardware support dummy bytes transfer based on the
> > > > +???? * programmed dummy clock cyles in QSPI register.
> > > > +???? * So, get the total dummy bytes from the dummy bytes transfer in
> > > > +???? * spi_messages and convert to dummy clock cyles.
> > > > +???? */
> > > > +??? list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> > > > +??????? if (ntransfers == DUMMY_BYTES_XFER &&
> > > > +??????????? !(list_is_last(&xfer->transfer_list, &msg->transfers)))
> > > > +??????????? dummy_cycles = xfer->len * 8 / xfer->tx_nbits;
> > > > +??????? ntransfers++;
> > > > +??? }

> > > This seems weird, there's some hard coded assumption about particular
> > > patterns that the client device is going to send.? What's going on here?
> > > I don't really understand what this is trying to do.

> > QSPI flash needs dummy cycles for data read operation which is actually
> > the initial read latency and no. of dummy cycles required are vendor
> > specific.

> > SPI NOR driver gets required dummy cycles based on mode clock cycles and
> > wait state clock cycles.

> > During read operations, spi_nor_spimem_read_data() converts dummy cycles
> > to number of dummy bytes.

> > Tegra QSPI controller supports dummy clock cycles register and when
> > programmed QSPI controller sends dummy bytes rather than SW handling
> > extra cycles for transferring dummy bytes.

> > Above equation converts this dummy bytes back to dummy clock cycles to
> > program into QSPI register and avoid manual SW transfer of dummy bytes.

This is not a good idea, attempting to reverse engineer the message and
guess at the contents isn't going to be robust and if it's useful it
will if nothing else lead to a bunch of duplicated code in drivers as
every device that has this feature will need to reimplment it. Instead
we should extend the framework so there's explicit support for
specifying transfers that are padding bytes, then there's no guesswork
that can go wrong and no duplicated code between drivers. A flag in the
transfer struct might work?

Attachments:

(No filename) (4.78 kB)
signature.asc (499.00 B)
Download all attachments

2020-12-04 21:09:31

by Sowjanya Komatineni

[permalink] [raw]

Subject: Re: [PATCH v1 3/7] spi: qspi-tegra: Add support for Tegra210 QSPI controller

On 12/4/20 10:52 AM, Mark Brown wrote:
> On Thu, Dec 03, 2020 at 04:22:54PM -0800, Sowjanya Komatineni wrote:
>> On 12/2/20 11:17 AM, Sowjanya Komatineni wrote:
>>>> It seems weird that this device needs us to do a memcpy() to do DMA,
>>>> most devices are able to DMA directly from the buffers provided by the
>>>> SPI API (and let the SPI core sync things).? What is going on here?
>>> For transfers of size more than max DMA transfer limit, data transfer
>>> happens in multiple iterations with each iteration transferring up to
>>> max DMA transfer limit.
>>> So using separate dma buffers and on every iteration copying them to SPI
>>> core provided tx/rx buffers.
> I don't understand this - there's no restriction on where DMA transfers
> can be done from within a DMA mapped region, the driver can do multiple
> transfers from different chunks of the source buffer without having to
> copy anything. That's a very common scenario.
>
>> Also unpack mode needs to manually put the bytes together from read data to
>> SPI core rx buffer.
> Could you be more explicit here, I don't know what "unpack mode" is?

Tegra SPI/QSPI controller support packed mode and unpacked mode based on
bits per word in a transfer.

Packed Mode: When enabled, all 32-bits of data in FIFO contains valid
data packets of 8-bit/16-bit/32-bit length.

Non packed mode: For transfers like 24-bit data for example we disable
packed mode and only 24-bits of FIFO data are valid and other bits are 0's.
So during TX for FIFO filling and during receive when FIFO data is read,
SW need to skip invalid bits and should align order from/to SPI core
tx/rx buffers.

>
>>>> This is worrying, the client device might be confused if /CS is doing
>>>> things outside of the standard handling.
>>> Do you mean to honor spi_transfer cs_change flag?
> At least, yes - more generally just if there's any feature to with chip
> select then the driver will need to open code it. The driver should at
> least be double checking that what it's doing matches what it was told
> to do, though just letting this be handled by the generic code if
> there's no limitation on the hardware tends to be easier all round.
OK Will honor cs_change flag at end of transfer and will program CS
state based on that.
>
>>> Tegra QSPI is master and is used only with QSPI flash devices. Looking
>>> at SPI NOR driver, I see QSPI Flash commands are executed with one flash
>>> command per spi_message and I dont see cs_change flag usage w.r.t QSPI
>>> flash. So, using SW based CS control for QSPI.
>>> Please correct me if I miss something to understand here.
> Someone might build a system that does something different, they may see
> a spare SPI controller and decide they can use it for something else or
> there may be some future change with the flash code which does something
> different.
>
>>>>> +??? tegra_qspi_writel(tqspi, tqspi->def_command2_reg, QSPI_COMMAND2);
>>>> The setup for one device shouldn't be able to affect the operation of
>>>> another, already running, device so either these need to be configured
>>>> as part of the controller probe or these configurations need to be
>>>> deferred until we're actually doing a transfer.
>>> We will only have 1 device on QSPI as we only support single chip select.
> It's quite common for people to do things like add additional devices
> with GPIO chip selects.
Will move tap delay programming to happen during spi transfer..
>
>>>>> +??? /*
>>>>> +???? * Tegra QSPI hardware support dummy bytes transfer based on the
>>>>> +???? * programmed dummy clock cyles in QSPI register.
>>>>> +???? * So, get the total dummy bytes from the dummy bytes transfer in
>>>>> +???? * spi_messages and convert to dummy clock cyles.
>>>>> +???? */
>>>>> +??? list_for_each_entry(xfer, &msg->transfers, transfer_list) {
>>>>> +??????? if (ntransfers == DUMMY_BYTES_XFER &&
>>>>> +??????????? !(list_is_last(&xfer->transfer_list, &msg->transfers)))
>>>>> +??????????? dummy_cycles = xfer->len * 8 / xfer->tx_nbits;
>>>>> +??????? ntransfers++;
>>>>> +??? }
>>>> This seems weird, there's some hard coded assumption about particular
>>>> patterns that the client device is going to send.? What's going on here?
>>>> I don't really understand what this is trying to do.
>>> QSPI flash needs dummy cycles for data read operation which is actually
>>> the initial read latency and no. of dummy cycles required are vendor
>>> specific.
>>> SPI NOR driver gets required dummy cycles based on mode clock cycles and
>>> wait state clock cycles.
>>> During read operations, spi_nor_spimem_read_data() converts dummy cycles
>>> to number of dummy bytes.
>>> Tegra QSPI controller supports dummy clock cycles register and when
>>> programmed QSPI controller sends dummy bytes rather than SW handling
>>> extra cycles for transferring dummy bytes.
>>> Above equation converts this dummy bytes back to dummy clock cycles to
>>> program into QSPI register and avoid manual SW transfer of dummy bytes.
> This is not a good idea, attempting to reverse engineer the message and
> guess at the contents isn't going to be robust and if it's useful it
> will if nothing else lead to a bunch of duplicated code in drivers as
> every device that has this feature will need to reimplment it. Instead
> we should extend the framework so there's explicit support for
> specifying transfers that are padding bytes, then there's no guesswork
> that can go wrong and no duplicated code between drivers. A flag in the
> transfer struct might work?

As per QSPI spec, Dummy bytes for initial read latency are always FF's.
So its not like guessing the contents.

Tegra QSPI controller HW supports transferring dummy bytes (sending FF's
after address) based on dummy clock cycles programmed.

To allow Tegra QSPI HW transfer dummy bytes directly, controller driver
need number of dummy bytes / actual dummy clock cycles which core driver
gets from flash sfdp read data.

So, we can add flag to transfer and based on this flag if controller HW
supports then we can ignore filling dummy bytes in spi_mem_exec_op but
controller driver still needs dummy_cycles value. So probably along with
flag, do you agree to have dummy_cycle as part of transfer struct which
can be set to nor->read_dummy value?

Thanks

Sowjanya

2020-12-04 22:37:24

by Sowjanya Komatineni

[permalink] [raw]

Subject: Re: [PATCH v1 3/7] spi: qspi-tegra: Add support for Tegra210 QSPI controller

Thanks Jon. Will address below suggestions in v2.

Regards,

Sowjanya

On 12/4/20 6:41 AM, Jon Hunter wrote:
> On 01/12/2020 21:12, Sowjanya Komatineni wrote:
>> Tegra SoC has a Quad SPI controller starting from Tegra210.
>>
>> This patch adds support for Tegra210 QSPI controller.
>>
>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>> ---
>> drivers/spi/Kconfig | 9 +
>> drivers/spi/Makefile | 1 +
>> drivers/spi/qspi-tegra.c | 1418 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 1428 insertions(+)
>> create mode 100644 drivers/spi/qspi-tegra.c
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 3fd16b7..1a021e8 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -844,6 +844,15 @@ config SPI_MXS
>> help
>> SPI driver for Freescale MXS devices.
>>
>> +config QSPI_TEGRA
>> + tristate "Nvidia Tegra QSPI Controller"
>> + depends on (ARCH_TEGRA && TEGRA20_APB_DMA) || COMPILE_TEST
> I assume that the dependency on the APBDMA is for Tegra210. Does it work
> on Tegra210 without the DMA? I am wondering if this is a dependency?
>
>> +static void tegra_qspi_deinit_dma_param(struct tegra_qspi_data *tqspi,
>> + bool dma_to_memory)
>> +{
>> + u32 *dma_buf;
>> + dma_addr_t dma_phys;
>> + struct dma_chan *dma_chan;
>> +
>> + if (dma_to_memory) {
>> + dma_buf = tqspi->rx_dma_buf;
>> + dma_chan = tqspi->rx_dma_chan;
>> + dma_phys = tqspi->rx_dma_phys;
>> + tqspi->rx_dma_chan = NULL;
>> + tqspi->rx_dma_buf = NULL;
>> + } else {
>> + dma_buf = tqspi->tx_dma_buf;
>> + dma_chan = tqspi->tx_dma_chan;
>> + dma_phys = tqspi->tx_dma_phys;
>> + tqspi->tx_dma_buf = NULL;
>> + tqspi->tx_dma_chan = NULL;
>> + }
>> + if (!dma_chan)
>> + return;
> The above seemed odd to me at first, but I guess if a device does not
> support DMA yet, then this will be NULL. However, would it be clearer to
> just ...
>
> if (!tqspi->use_dma)
> return;
>
> You could also do this right at the beginning of the function.
>
>> +static struct tegra_qspi_client_data
>> + *tegra_qspi_parse_cdata_dt(struct spi_device *spi)
>> +{
>> + struct tegra_qspi_client_data *cdata;
>> + struct device_node *slave_np;
>> +
>> + slave_np = spi->dev.of_node;
>> + if (!slave_np) {
> This test should not be necessary as we only support device-tree.
>
>> + dev_dbg(&spi->dev, "device node not found\n");
>> + return NULL;
>> + }
>> +
>> + cdata = kzalloc(sizeof(*cdata), GFP_KERNEL);
>> + if (!cdata)
>> + return NULL;
>> +
>> + of_property_read_u32(slave_np, "nvidia,tx-clk-tap-delay",
>> + &cdata->tx_clk_tap_delay);
>> + of_property_read_u32(slave_np, "nvidia,rx-clk-tap-delay",
>> + &cdata->rx_clk_tap_delay);
>> + return cdata;
>> +}
>> +
>> +static void tegra_qspi_cleanup(struct spi_device *spi)
>> +{
>> + struct tegra_qspi_client_data *cdata = spi->controller_data;
>> +
>> + spi->controller_data = NULL;
>> + if (spi->dev.of_node)
>> + kfree(cdata);
>> +}
>> +
>> +static int tegra_qspi_setup(struct spi_device *spi)
>> +{
>> + struct tegra_qspi_data *tqspi = spi_master_get_devdata(spi->master);
>> + struct tegra_qspi_client_data *cdata = spi->controller_data;
>> + u32 tx_tap = 0, rx_tap = 0;
>> + u32 val;
>> + unsigned long flags;
>> + int ret;
>> +
>> + dev_dbg(&spi->dev, "setup %d bpw, %scpol, %scpha, %dHz\n",
>> + spi->bits_per_word,
>> + spi->mode & SPI_CPOL ? "" : "~",
>> + spi->mode & SPI_CPHA ? "" : "~",
>> + spi->max_speed_hz);
>> +
>> + if (!cdata) {
>> + cdata = tegra_qspi_parse_cdata_dt(spi);
>> + spi->controller_data = cdata;
>> + }
>> +
>> + ret = pm_runtime_get_sync(tqspi->dev);
>> + if (ret < 0) {
>> + dev_err(tqspi->dev, "runtime resume failed: %d\n", ret);
>> + if (cdata)
>> + tegra_qspi_cleanup(spi);
>> + return ret;
>> + }
> Does it simplify the code to do the pm_runtime_get_sync() before the
> parsing of the cdata?
>
>> +static int tegra_qspi_probe(struct platform_device *pdev)
>> +{
>> + struct spi_master *master;
>> + struct tegra_qspi_data *tqspi;
>> + struct resource *r;
>> + int ret, qspi_irq;
>> + int bus_num;
>> +
>> + master = spi_alloc_master(&pdev->dev, sizeof(*tqspi));
>> + if (!master) {
>> + dev_err(&pdev->dev, "master allocation failed\n");
>> + return -ENOMEM;
>> + }
>> +
>> + platform_set_drvdata(pdev, master);
>> + tqspi = spi_master_get_devdata(master);
>> +
>> + if (of_property_read_u32(pdev->dev.of_node, "spi-max-frequency",
>> + &master->max_speed_hz))
>> + master->max_speed_hz = QSPI_MAX_SPEED;
>> +
>> + /* the spi->mode bits understood by this driver: */
>> + 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->setup = tegra_qspi_setup;
>> + master->cleanup = tegra_qspi_cleanup;
>> + master->transfer_one_message = tegra_qspi_transfer_one_message;
>> + master->num_chipselect = 1;
>> + master->auto_runtime_pm = true;
>> + bus_num = of_alias_get_id(pdev->dev.of_node, "spi");
>> + if (bus_num >= 0)
>> + master->bus_num = bus_num;
>> +
>> + tqspi->master = master;
>> + tqspi->dev = &pdev->dev;
>> + spin_lock_init(&tqspi->lock);
>> +
>> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + tqspi->base = devm_ioremap_resource(&pdev->dev, r);
>> + if (IS_ERR(tqspi->base)) {
>> + ret = PTR_ERR(tqspi->base);
>> + goto exit_free_master;
>> + }
>> +
>> + tqspi->phys = r->start;
>> + qspi_irq = platform_get_irq(pdev, 0);
>> + tqspi->irq = qspi_irq;
>> +
>> + tqspi->clk = devm_clk_get(&pdev->dev, "qspi");
>> + if (IS_ERR(tqspi->clk)) {
>> + ret = PTR_ERR(tqspi->clk);
>> + dev_err(&pdev->dev, "failed to get clock: %d\n", ret);
>> + goto exit_free_master;
>> + }
>> +
>> + tqspi->rst = devm_reset_control_get_exclusive(&pdev->dev, "qspi");
>> + if (IS_ERR(tqspi->rst)) {
>> + ret = PTR_ERR(tqspi->rst);
>> + dev_err(&pdev->dev, "failed to get reset control: %d\n", ret);
>> + goto exit_free_master;
>> + }
>> +
>> + tqspi->max_buf_size = QSPI_FIFO_DEPTH << 2;
>> + tqspi->dma_buf_size = DEFAULT_QSPI_DMA_BUF_LEN;
>> +
>> + ret = tegra_qspi_init_dma_param(tqspi, true);
>> + if (ret < 0)
>> + goto exit_free_master;
>> + ret = tegra_qspi_init_dma_param(tqspi, false);
>> + if (ret < 0)
>> + goto exit_rx_dma_free;
> I would be tempted to combine the init for the TX and RX into a single
> function. Then we can have a single function to deinit.
>
>> +
>> + if (tqspi->use_dma)
>> + tqspi->max_buf_size = tqspi->dma_buf_size;
>> +
>> + init_completion(&tqspi->tx_dma_complete);
>> + init_completion(&tqspi->rx_dma_complete);
>> +
> Unnecessary blank line.
>
>> + init_completion(&tqspi->xfer_completion);
>> +
>> + pm_runtime_enable(&pdev->dev);
>> + if (!pm_runtime_enabled(&pdev->dev)) {
> RPM is always enabled for Tegra and so if this fails we should just fail.
>
>> + ret = tegra_qspi_runtime_resume(&pdev->dev);
>> + if (ret)
>> + goto exit_pm_disable;
>> + }
>> +
>> + ret = pm_runtime_get_sync(&pdev->dev);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "runtime resume failed: %d\n", ret);
>> + pm_runtime_put_noidle(&pdev->dev)
> You can use pm_runtime_resume_and_get() now and then you don't need to
> call pm_runtime_put_noidle() on failure.
>
>> + goto exit_pm_disable;
>> + }
>> +
>> + reset_control_assert(tqspi->rst);
>> + udelay(2);
>> + reset_control_deassert(tqspi->rst);
>> + tqspi->def_command1_reg = QSPI_M_S | QSPI_CS_SW_HW | QSPI_CS_SW_VAL;
>> + tegra_qspi_writel(tqspi, tqspi->def_command1_reg, QSPI_COMMAND1);
>> + tqspi->spi_cs_timing1 = tegra_qspi_readl(tqspi, QSPI_CS_TIMING1);
>> + tqspi->spi_cs_timing2 = tegra_qspi_readl(tqspi, QSPI_CS_TIMING2);
>> + tqspi->def_command2_reg = tegra_qspi_readl(tqspi, QSPI_COMMAND2);
>> +
>> + pm_runtime_put(&pdev->dev);
>> +
>> + ret = request_threaded_irq(tqspi->irq, tegra_qspi_isr,
>> + tegra_qspi_isr_thread, IRQF_ONESHOT,
>> + dev_name(&pdev->dev), tqspi);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev,
>> + "failed to request IRQ#%u: %d\n", tqspi->irq, ret);
>> + goto exit_pm_disable;
>> + }
>> +
>> + master->dev.of_node = pdev->dev.of_node;
>> + ret = devm_spi_register_master(&pdev->dev, master);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "failed to register master: %d\n", ret);
>> + goto exit_free_irq;
>> + }
>> + return ret;
> return 0
>
>> +static int tegra_qspi_runtime_resume(struct device *dev)
>> +{
>> + struct spi_master *master = dev_get_drvdata(dev);
>> + struct tegra_qspi_data *tqspi = spi_master_get_devdata(master);
>> + int ret;
>> +
>> + ret = clk_prepare_enable(tqspi->clk);
>> + if (ret < 0) {
>> + dev_err(tqspi->dev, "clk_prepare failed: %d\n", ret);
>> + return ret;
>> + }
>> + return 0;
> Always just 'return ret' here.
>
> Cheers
> Jon
>

2020-12-04 22:46:22

by Sowjanya Komatineni

[permalink] [raw]

Subject: Re: [PATCH v1 3/7] spi: qspi-tegra: Add support for Tegra210 QSPI controller

Thanks Thierry. Will address below suggestions in v2.

Some inline comments below regarding client data part of controller driver.

Regards,

Sowjanya

On 12/4/20 4:11 AM, Thierry Reding wrote:
> On Tue, Dec 01, 2020 at 01:12:44PM -0800, Sowjanya Komatineni wrote:
>> Tegra SoC has a Quad SPI controller starting from Tegra210.
>>
>> This patch adds support for Tegra210 QSPI controller.
>>
>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>> ---
>> drivers/spi/Kconfig | 9 +
>> drivers/spi/Makefile | 1 +
>> drivers/spi/qspi-tegra.c | 1418 ++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 1428 insertions(+)
>> create mode 100644 drivers/spi/qspi-tegra.c
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 3fd16b7..1a021e8 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -844,6 +844,15 @@ config SPI_MXS
>> help
>> SPI driver for Freescale MXS devices.
>>
>> +config QSPI_TEGRA
> You already discussed this with Mark, but perhaps a better name would be
> SPI_TEGRA210_QUAD or something. SPI_TEGRA210 is too generic because
> there is a regular SPI controller on Tegra210 as well.
>
> SPI_TEGRA210_QUAD is in line with the likes of SPI_TEGRA20_SFLASH and
> SPI_TEGRA20_SLINK.
>
>> + tristate "Nvidia Tegra QSPI Controller"
> NVIDIA
>
>> + depends on (ARCH_TEGRA && TEGRA20_APB_DMA) || COMPILE_TEST
> I don't think we need the ARCH_TEGRA part there because TEGRA20_APB_DMA
> already depends on that. Also, there's not strictly a dependency on
> TEGRA20_APB_DMA specifically, but rather a dependency on DMA_ENGINE,
> right? The DMA channels could be coming from some other driver on some
> other SoC generation, such as the Tegra186 and later GPCDMA.
>
>> + depends on RESET_CONTROLLER
>> + help
>> + QSPI driver for Nvidia Tegra QSPI Controller interface. This
> NVIDIA
>
>> + controller is different from the spi controller and is available
> SPI
>
>> + on Tegra SoCs starting from Tegra210.
>> +
>> config SPI_TEGRA114
>> tristate "NVIDIA Tegra114 SPI Controller"
>> depends on (ARCH_TEGRA && TEGRA20_APB_DMA) || COMPILE_TEST
> [...]
>> diff --git a/drivers/spi/qspi-tegra.c b/drivers/spi/qspi-tegra.c
> [...]
>> +struct tegra_qspi_client_data {
>> + int tx_clk_tap_delay;
>> + int rx_clk_tap_delay;
>> +};
> If this is client data, why are we dealing with this in the controller
> driver? Is this perhaps something that could be added to struct
> spi_device?

tx/rx clock tap delays are based on platform design and values may vary
depending on trace lengths to corresponding spi device.

These tap delays are programmed in Tegra QSPI controller.

>
>> +
>> +struct tegra_qspi_data {
> That _data just seems to be 5 extra characters that don't add any value.
>
>> + struct device *dev;
>> + struct spi_master *master;
>> + /* Lock to protect data accessed by irq */
>> + spinlock_t lock;
>> +
>> + struct clk *clk;
>> + struct reset_control *rst;
>> + void __iomem *base;
>> + phys_addr_t phys;
>> + unsigned int irq;
>> +
>> + u32 cur_speed;
>> + unsigned int cur_pos;
>> + unsigned int words_per_32bit;
>> + unsigned int bytes_per_word;
>> + unsigned int curr_dma_words;
>> + unsigned int cur_direction;
>> +
>> + unsigned int cur_rx_pos;
>> + unsigned int cur_tx_pos;
>> +
>> + unsigned int dma_buf_size;
>> + unsigned int max_buf_size;
>> + bool is_curr_dma_xfer;
>> +
>> + struct completion rx_dma_complete;
>> + struct completion tx_dma_complete;
>> +
>> + u32 tx_status;
>> + u32 rx_status;
>> + u32 status_reg;
>> + bool is_packed;
>> + bool use_dma;
>> +
>> + u32 command1_reg;
>> + u32 dma_control_reg;
>> + u32 def_command1_reg;
>> + u32 def_command2_reg;
>> + u32 spi_cs_timing1;
>> + u32 spi_cs_timing2;
>> + u8 dummy_cycles;
>> +
>> + struct completion xfer_completion;
>> + struct spi_transfer *curr_xfer;
>> +
>> + struct dma_chan *rx_dma_chan;
>> + u32 *rx_dma_buf;
>> + dma_addr_t rx_dma_phys;
>> + struct dma_async_tx_descriptor *rx_dma_desc;
>> +
>> + struct dma_chan *tx_dma_chan;
>> + u32 *tx_dma_buf;
>> + dma_addr_t tx_dma_phys;
>> + struct dma_async_tx_descriptor *tx_dma_desc;
>> +};
>> +
>> +static int tegra_qspi_runtime_suspend(struct device *dev);
>> +static int tegra_qspi_runtime_resume(struct device *dev);
> Can't we just reorder the code so that these don't have to be forward-
> declared?
>
>> +
>> +static inline u32 tegra_qspi_readl(struct tegra_qspi_data *tqspi,
>> + unsigned long reg)
> Nit: I prefer "offset" over "reg" because I think it's slightly more
> accurate.
>
>> +{
>> + return readl(tqspi->base + reg);
>> +}
>> +
>> +static inline void tegra_qspi_writel(struct tegra_qspi_data *tqspi,
>> + u32 val, unsigned long reg)
> I also prefer "value" over "val" because "val" could also be short for
> "valid". Anyway, I am pedantic that way, so feel free to ignore that.
>
> [...]
>> +static unsigned int
>> +tegra_qspi_calculate_curr_xfer_param(struct tegra_qspi_data *tqspi,
>> + struct spi_transfer *t)
>> +{
>> + unsigned int remain_len = t->len - tqspi->cur_pos;
>> + unsigned int max_word;
>> + unsigned int bits_per_word = t->bits_per_word;
>> + unsigned int max_len;
>> + unsigned int total_fifo_words;
> You could list some of these on the same line to make this a bit more
> compact. Something I've often seen done that makes this really clean is
> to have uninitialized variables on one line and then assignments on
> separate lines, then sort lines by length:
>
> unsigned int max_word, max_len, total_fifo_words;
> unsigned int remain_len = t->len - tqspi->cur_pos;
> unsigned int bits_per_word = t->bits_per_word;
>
> This also applies to some other functions further down.
>
> [...]
>> +static void
>> +tegra_qspi_copy_client_txbuf_to_qspi_txbuf(struct tegra_qspi_data *tqspi,
>> + struct spi_transfer *t)
>> +{
>> + /* Make the dma buffer to read by cpu */
> This comment seems a bit redundant. dma_sync_single_for_cpu() is a well-
> documented function that doesn't need explanation.
>
>> + dma_sync_single_for_cpu(tqspi->dev, tqspi->tx_dma_phys,
>> + tqspi->dma_buf_size, DMA_TO_DEVICE);
>> +
>> + if (tqspi->is_packed) {
>> + unsigned int len = tqspi->curr_dma_words *
>> + tqspi->bytes_per_word;
>> +
>> + memcpy(tqspi->tx_dma_buf, t->tx_buf + tqspi->cur_pos, len);
>> + tqspi->cur_tx_pos += tqspi->curr_dma_words *
>> + tqspi->bytes_per_word;
>> + } else {
>> + u8 *tx_buf = (u8 *)t->tx_buf + tqspi->cur_tx_pos;
>> + unsigned int i;
>> + unsigned int count;
>> + unsigned int consume;
>> + unsigned int write_bytes;
>> +
>> + consume = tqspi->curr_dma_words * tqspi->bytes_per_word;
>> + if (consume > t->len - tqspi->cur_pos)
>> + consume = t->len - tqspi->cur_pos;
>> + write_bytes = consume;
>> + for (count = 0; count < tqspi->curr_dma_words; count++) {
>> + u32 x = 0;
>> +
>> + for (i = 0; consume && (i < tqspi->bytes_per_word);
>> + i++, consume--)
>> + x |= (u32)(*tx_buf++) << (i * 8);
>> + tqspi->tx_dma_buf[count] = x;
>> + }
>> +
>> + tqspi->cur_tx_pos += write_bytes;
>> + }
>> +
>> + /* Make the dma buffer to read by dma */
> Same here.
>
>> + dma_sync_single_for_device(tqspi->dev, tqspi->tx_dma_phys,
>> + tqspi->dma_buf_size, DMA_TO_DEVICE);
>> +}
>> +
>> +static void
>> +tegra_qspi_copy_qspi_rxbuf_to_client_rxbuf(struct tegra_qspi_data *tqspi,
>> + struct spi_transfer *t)
>> +{
>> + /* Make the dma buffer to read by cpu */
> And here.
>
>> + dma_sync_single_for_cpu(tqspi->dev, tqspi->rx_dma_phys,
>> + tqspi->dma_buf_size, DMA_FROM_DEVICE);
>> +
>> + if (tqspi->is_packed) {
>> + unsigned int len = tqspi->curr_dma_words *
>> + tqspi->bytes_per_word;
>> +
>> + memcpy(t->rx_buf + tqspi->cur_rx_pos, tqspi->rx_dma_buf, len);
>> + tqspi->cur_rx_pos += tqspi->curr_dma_words *
>> + tqspi->bytes_per_word;
>> + } else {
>> + unsigned char *rx_buf = t->rx_buf + tqspi->cur_rx_pos;
>> + u32 rx_mask = ((u32)1 << t->bits_per_word) - 1;
>> + unsigned int i;
>> + unsigned int count;
>> + unsigned int consume;
>> + unsigned int read_bytes;
>> +
>> + consume = tqspi->curr_dma_words * tqspi->bytes_per_word;
>> + if (consume > t->len - tqspi->cur_pos)
>> + consume = t->len - tqspi->cur_pos;
>> + read_bytes = consume;
>> + for (count = 0; count < tqspi->curr_dma_words; count++) {
>> + u32 x = tqspi->rx_dma_buf[count] & rx_mask;
>> +
>> + for (i = 0; consume && (i < tqspi->bytes_per_word);
>> + i++, consume--)
>> + *rx_buf++ = (x >> (i * 8)) & 0xFF;
>> + }
>> +
>> + tqspi->cur_rx_pos += read_bytes;
>> + }
>> +
>> + /* Make the dma buffer to read by dma */
> And here.
>
>> + dma_sync_single_for_device(tqspi->dev, tqspi->rx_dma_phys,
>> + tqspi->dma_buf_size, DMA_FROM_DEVICE);
>> +}
>> +
>> +static void tegra_qspi_dma_complete(void *args)
>> +{
>> + struct completion *dma_complete = args;
>> +
>> + complete(dma_complete);
>> +}
>> +
>> +static int tegra_qspi_start_tx_dma(struct tegra_qspi_data *tqspi, int len)
>> +{
>> + reinit_completion(&tqspi->tx_dma_complete);
>> + tqspi->tx_dma_desc = dmaengine_prep_slave_single(tqspi->tx_dma_chan,
>> + tqspi->tx_dma_phys, len, DMA_MEM_TO_DEV,
>> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> Looks like most of this driver is wrapped at 80 columns. That rule was
> relaxed a bit a while ago and checkpatch now only warns if you exceed
> 100 columns. There are various places in this driver that could benefit
> from longer lines, such as the above.
>
>> + if (!tqspi->tx_dma_desc) {
>> + dev_err(tqspi->dev, "Not able to get desc for Tx\n");
> Perhaps: "Unable to get TX descriptor\n"?
>
>> + return -EIO;
>> + }
>> +
>> + tqspi->tx_dma_desc->callback = tegra_qspi_dma_complete;
>> + tqspi->tx_dma_desc->callback_param = &tqspi->tx_dma_complete;
>> +
>> + dmaengine_submit(tqspi->tx_dma_desc);
>> + dma_async_issue_pending(tqspi->tx_dma_chan);
>> + return 0;
>> +}
>> +
>> +static int tegra_qspi_start_rx_dma(struct tegra_qspi_data *tqspi, int len)
>> +{
>> + reinit_completion(&tqspi->rx_dma_complete);
>> + tqspi->rx_dma_desc = dmaengine_prep_slave_single(tqspi->rx_dma_chan,
>> + tqspi->rx_dma_phys, len, DMA_DEV_TO_MEM,
>> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>> + if (!tqspi->rx_dma_desc) {
>> + dev_err(tqspi->dev, "Not able to get desc for Rx\n");
> Same here.
>
>> + return -EIO;
>> + }
>> +
>> + tqspi->rx_dma_desc->callback = tegra_qspi_dma_complete;
>> + tqspi->rx_dma_desc->callback_param = &tqspi->rx_dma_complete;
>> +
>> + dmaengine_submit(tqspi->rx_dma_desc);
>> + dma_async_issue_pending(tqspi->rx_dma_chan);
>> + return 0;
>> +}
>> +
>> +static int tegra_qspi_flush_fifos(struct tegra_qspi_data *tqspi)
>> +{
>> + unsigned long timeout = jiffies + HZ;
>> + u32 status;
>> +
>> + status = tegra_qspi_readl(tqspi, QSPI_FIFO_STATUS);
>> + if ((status & QSPI_FIFO_EMPTY) == QSPI_FIFO_EMPTY)
>> + return 0;
>> +
>> + status |= QSPI_RX_FIFO_FLUSH | QSPI_TX_FIFO_FLUSH;
>> + tegra_qspi_writel(tqspi, status, QSPI_FIFO_STATUS);
>> + while ((status & QSPI_FIFO_EMPTY) != QSPI_FIFO_EMPTY) {
>> + status = tegra_qspi_readl(tqspi, QSPI_FIFO_STATUS);
>> + if (time_after(jiffies, timeout)) {
>> + dev_err(tqspi->dev,
>> + "timeout waiting for fifo flush\n");
> FIFO is an abbreviation, so it should be all uppercase in text.
>
>> + return -EIO;
>> + }
>> +
>> + udelay(1);
> It looks like this function can be called from both interrupt and
> process contexts, where the latter is the more common case and it is
> only ever used in interrupt context to clean up on error.
>
> I wonder if it's worth adding an "atomic" parameter to the function and
> make this a sleeping loop whenever possible. Also, can this not use one
> of the helpers from linux/iopoll.h?
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void tegra_qspi_unmask_irq(struct tegra_qspi_data *tqspi)
>> +{
>> + u32 intr_mask;
>> +
>> + intr_mask = tegra_qspi_readl(tqspi, QSPI_INTR_MASK);
>> + intr_mask &= ~(QSPI_INTR_RDY_MASK | QSPI_INTR_RX_TX_FIFO_ERR);
>> + tegra_qspi_writel(tqspi, intr_mask, QSPI_INTR_MASK);
>> +}
>> +
>> +static int tegra_qspi_start_dma_based_transfer(struct tegra_qspi_data *tqspi,
>> + struct spi_transfer *t)
>> +{
>> + u32 val;
>> + unsigned int len;
>> + int ret = 0;
>> + u8 dma_burst;
>> + struct dma_slave_config dma_sconfig = {0};
> I think checkpatch wants spaces after { and before }.
>
>> +
>> + val = QSPI_DMA_BLK_SET(tqspi->curr_dma_words - 1);
>> + tegra_qspi_writel(tqspi, val, QSPI_DMA_BLK);
>> +
>> + tegra_qspi_unmask_irq(tqspi);
>> +
>> + if (tqspi->is_packed)
>> + len = DIV_ROUND_UP(tqspi->curr_dma_words *
>> + tqspi->bytes_per_word, 4) * 4;
>> + else
>> + len = tqspi->curr_dma_words * 4;
>> +
>> + /* Set attention level based on length of transfer */
>> + val = 0;
>> + if (len & 0xF) {
> Nit: hexadecimal literals are usually lowercase.
>
>> + val |= QSPI_TX_TRIG_1 | QSPI_RX_TRIG_1;
>> + dma_burst = 1;
>> + } else if (((len) >> 4) & 0x1) {
>> + val |= QSPI_TX_TRIG_4 | QSPI_RX_TRIG_4;
>> + dma_burst = 4;
>> + } else {
>> + val |= QSPI_TX_TRIG_8 | QSPI_RX_TRIG_8;
>> + dma_burst = 8;
>> + }
>> +
>> + tegra_qspi_writel(tqspi, val, QSPI_DMA_CTL);
>> + tqspi->dma_control_reg = val;
>> +
>> + dma_sconfig.device_fc = true;
>> + if (tqspi->cur_direction & DATA_DIR_TX) {
>> + dma_sconfig.dst_addr = tqspi->phys + QSPI_TX_FIFO;
>> + dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> + dma_sconfig.dst_maxburst = dma_burst;
>> + ret = dmaengine_slave_config(tqspi->tx_dma_chan, &dma_sconfig);
>> + if (ret < 0) {
>> + dev_err(tqspi->dev,
>> + "DMA slave config failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + tegra_qspi_copy_client_txbuf_to_qspi_txbuf(tqspi, t);
>> + ret = tegra_qspi_start_tx_dma(tqspi, len);
>> + if (ret < 0) {
>> + dev_err(tqspi->dev,
>> + "Starting tx dma failed: %d\n", ret);
> "TX" and "DMA"
>
>> + return ret;
>> + }
>> + }
>> +
>> + if (tqspi->cur_direction & DATA_DIR_RX) {
>> + dma_sconfig.src_addr = tqspi->phys + QSPI_RX_FIFO;
>> + dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> + dma_sconfig.src_maxburst = dma_burst;
>> + ret = dmaengine_slave_config(tqspi->rx_dma_chan, &dma_sconfig);
>> + if (ret < 0) {
>> + dev_err(tqspi->dev,
>> + "DMA slave config failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + /* Make the dma buffer to read by dma */
> Again, not a useful comment.
>
>> + dma_sync_single_for_device(tqspi->dev, tqspi->rx_dma_phys,
>> + tqspi->dma_buf_size,
>> + DMA_FROM_DEVICE);
>> +
>> + ret = tegra_qspi_start_rx_dma(tqspi, len);
>> + if (ret < 0) {
>> + dev_err(tqspi->dev,
>> + "Starting rx dma failed: %d\n", ret);
> "RX" and "DMA"
>
> [...]
>> +static int tegra_qspi_init_dma_param(struct tegra_qspi_data *tqspi,
>> + bool dma_to_memory)
>> +{
>> + struct dma_chan *dma_chan;
>> + u32 *dma_buf;
>> + dma_addr_t dma_phys;
>> +
>> + if (!device_property_present(tqspi->dev, "dmas"))
>> + return 0;
> If DMA support is optional, then we definitely don't want to depend on
> TEGRA20_APB_DMA (or any other specific driver).
>
>> +
>> + dma_chan = dma_request_chan(tqspi->dev, dma_to_memory ? "rx" : "tx");
> Does this return some specific value when there's no channel for this?
> I.e. what happens if the "dmas" property exists in device tree but we
> don't have a driver for the provider enabled? Since we have code to use
> as fallback, we may want to special case that here and allow the driver
> to continue.

In current Tegra194 device tree, I don't see dma properties populated
and we don't have GPCDMA upstream yet.

So kind of thought we will add dma properties to device tree only when
we have corresponding dma driver support.

Yeah understood there's nothing restricting to follow this. Will fix in
v2 to switch PIO mode even for the case when dma properties are present
but dma request fails.

>
>> + if (IS_ERR(dma_chan))
>> + return dev_err_probe(tqspi->dev, PTR_ERR(dma_chan),
>> + "Dma channel is not available\n");
> "DMA"
>
> [...]
>> +static int tegra_qspi_start_transfer_one(struct spi_device *spi,
>> + struct spi_transfer *t, u32 command1)
>> +{
> [...]
>> + command1 &= ~QSPI_INTERFACE_WIDTH_MASK;
>> + if (bus_width == SPI_NBITS_QUAD)
>> + command1 |= QSPI_INTERFACE_WIDTH_QUAD;
>> + else if (bus_width == SPI_NBITS_DUAL)
>> + command1 |= QSPI_INTERFACE_WIDTH_DUAL;
>> + else
>> + command1 |= QSPI_INTERFACE_WIDTH_SINGLE;
>> + tqspi->command1_reg = command1;
> This (and in some other places in the driver) could use a couple of
> blank lines to make this less cluttered.
>
> [...]
>> +static void tegra_qspi_cleanup(struct spi_device *spi)
>> +{
>> + struct tegra_qspi_client_data *cdata = spi->controller_data;
>> +
>> + spi->controller_data = NULL;
>> + if (spi->dev.of_node)
> Can this ever fail? Do we support SPI device instantiation from
> somewhere else than DT?
>
>> + kfree(cdata);
>> +}
>> +
>> +static int tegra_qspi_setup(struct spi_device *spi)
>> +{
>> + struct tegra_qspi_data *tqspi = spi_master_get_devdata(spi->master);
>> + struct tegra_qspi_client_data *cdata = spi->controller_data;
>> + u32 tx_tap = 0, rx_tap = 0;
>> + u32 val;
>> + unsigned long flags;
>> + int ret;
>> +
>> + dev_dbg(&spi->dev, "setup %d bpw, %scpol, %scpha, %dHz\n",
>> + spi->bits_per_word,
>> + spi->mode & SPI_CPOL ? "" : "~",
>> + spi->mode & SPI_CPHA ? "" : "~",
>> + spi->max_speed_hz);
>> +
>> + if (!cdata) {
>> + cdata = tegra_qspi_parse_cdata_dt(spi);
> Oh... I see that this is actually parsed from the SPI device node, so
> perhaps this is okay to do.
yeah. ok
>
> [...]
>> +static int tegra_qspi_transfer_one_message(struct spi_master *master,
>> + struct spi_message *msg)
>> +{
> [...]
>> + is_first_msg = false;
>> + ret = wait_for_completion_timeout(&tqspi->xfer_completion,
>> + QSPI_DMA_TIMEOUT);
>> + if (WARN_ON(ret == 0)) {
>> + dev_err(tqspi->dev,
>> + "qspi transfer timeout: %d\n", ret);
> "QSPI", or alternatively leave this out completely because it should be
> obvious that the transfer that failed is a QSPI transfer just from the
> device name.
>
>> + if (tqspi->is_curr_dma_xfer &&
>> + (tqspi->cur_direction & DATA_DIR_TX))
>> + dmaengine_terminate_all(tqspi->tx_dma_chan);
>> + if (tqspi->is_curr_dma_xfer &&
>> + (tqspi->cur_direction & DATA_DIR_RX))
>> + dmaengine_terminate_all(tqspi->rx_dma_chan);
>> + ret = -EIO;
>> + tegra_qspi_dump_regs(tqspi);
>> + tegra_qspi_flush_fifos(tqspi);
>> + reset_control_assert(tqspi->rst);
>> + udelay(2);
>> + reset_control_deassert(tqspi->rst);
>> + goto exit;
>> + }
>> +
>> + if (tqspi->tx_status || tqspi->rx_status) {
>> + ret = -EIO;
>> + dev_err(tqspi->dev, "error in transfer: %d\n", ret);
> Would it make sense to output tx_status and/or rx_status in this message
> rather than -EIO unconditionally?

>
> [...]
>> +static irqreturn_t handle_cpu_based_xfer(struct tegra_qspi_data *tqspi)
>> +{
>> + struct spi_transfer *t = tqspi->curr_xfer;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&tqspi->lock, flags);
>> + if (tqspi->tx_status || tqspi->rx_status) {
>> + dev_err(tqspi->dev, "CpuXfer ERROR bit set 0x%x\n",
>> + tqspi->status_reg);
>> + dev_err(tqspi->dev, "CpuXfer 0x%08x:0x%08x\n",
>> + tqspi->command1_reg, tqspi->dma_control_reg);
>> + tegra_qspi_dump_regs(tqspi);
>> + tegra_qspi_flush_fifos(tqspi);
>> + complete(&tqspi->xfer_completion);
>> + spin_unlock_irqrestore(&tqspi->lock, flags);
>> + reset_control_assert(tqspi->rst);
>> + udelay(2);
>> + reset_control_deassert(tqspi->rst);
>> + return IRQ_HANDLED;
>> + }
> Can this error handling be somehow unified with the DMA error handling?
> Having markers such as "CpuXfer" in error messages is not really helpful
> to users. Maybe have the driver output an INFO message or so during
> probe when it falls back to PIO mode, that would be a good enough
> indicator to someone debugging some issue that PIO mode is being used.
> And then you can just treat transfer failures more uniformly.
>
> Thierry

2020-12-04 22:49:21

On Mon, Dec 07, 2020 at 04:02:15PM -0800, Sowjanya Komatineni wrote:

> So by having it in spi_message, controller driver can get dummy cycles info
> a head of transfers so it can program this along with address bytes transfer
> phase.

That sounds fine.

Attachments:

(No filename) (262.00 B)
signature.asc (499.00 B)
Download all attachments

2020-12-09 17:32:22

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v1 2/7] dt-bindings: spi: Add Tegra QSPI device tree binding

On Tue, Dec 01, 2020 at 01:12:43PM -0800, Sowjanya Komatineni wrote:
> This patch adds YAML based device tree binding document for Tegra
> QSPI driver.
>
> Signed-off-by: Sowjanya Komatineni <[email protected]>
> ---
> .../devicetree/bindings/spi/nvidia,tegra-qspi.yaml | 77 ++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
>
> diff --git a/Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml b/Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
> new file mode 100644
> index 0000000..038a085
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/nvidia,tegra-qspi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Tegra Quad SPI Controller
> +
> +maintainers:
> + - Thierry Reding <[email protected]>
> + - Jonathan Hunter <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - nvidia,tegra210-qspi
> + - nvidia,tegra186-qspi
> + - nvidia,tegra194-qspi
> +
> + reg:
> + items:
> + - description: QSPI registers

Just 'maxItems: 1'

> +
> + interrupts:
> + maxItems: 1
> +
> + clock-names:
> + items:
> + - const: qspi

Kind of a pointless name.

> +
> + clocks:
> + maxItems: 1
> +
> + reset-names:
> + minItems: 1
> + items:
> + - const: qspi

Same here.

> +
> + resets:
> + maxItems: 1
> +
> + dmas:
> + maxItems: 2
> +
> + dma-names:
> + items:
> + - const: rx
> + - const: tx
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clock-names
> + - clocks
> + - reset-names
> + - resets
> +
> +additionalProperties: true
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/tegra210-car.h>
> + #include <dt-bindings/reset/tegra210-car.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + spi@70410000 {
> + compatible = "nvidia,tegra210-qspi";
> + reg = <0x70410000 0x1000>;
> + interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&tegra_car TEGRA210_CLK_QSPI>;
> + clock-names = "qspi";
> + resets = <&tegra_car 211>;
> + reset-names = "qspi";
> + dmas = <&apbdma 5>, <&apbdma 5>;
> + dma-names = "rx", "tx";
> + };
> --
> 2.7.4
>

2020-12-09 20:48:31

by Sowjanya Komatineni

[permalink] [raw]

Subject: Re: [PATCH v1 2/7] dt-bindings: spi: Add Tegra QSPI device tree binding

On 12/9/20 9:26 AM, Rob Herring wrote:
> On Tue, Dec 01, 2020 at 01:12:43PM -0800, Sowjanya Komatineni wrote:
>> This patch adds YAML based device tree binding document for Tegra
>> QSPI driver.
>>
>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>> ---
>> .../devicetree/bindings/spi/nvidia,tegra-qspi.yaml | 77 ++++++++++++++++++++++
>> 1 file changed, 77 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml b/Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
>> new file mode 100644
>> index 0000000..038a085
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
>> @@ -0,0 +1,77 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/spi/nvidia,tegra-qspi.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Tegra Quad SPI Controller
>> +
>> +maintainers:
>> + - Thierry Reding <[email protected]>
>> + - Jonathan Hunter <[email protected]>
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - nvidia,tegra210-qspi
>> + - nvidia,tegra186-qspi
>> + - nvidia,tegra194-qspi
>> +
>> + reg:
>> + items:
>> + - description: QSPI registers
> Just 'maxItems: 1'
>
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + clock-names:
>> + items:
>> + - const: qspi
> Kind of a pointless name.
Thanks Rob for feedback. Do you mean to change name something like
qspi-clk instead of qspi?
>
>> +
>> + clocks:
>> + maxItems: 1
>> +
>> + reset-names:
>> + minItems: 1
>> + items:
>> + - const: qspi
> Same here.
>
>> +
>> + resets:
>> + maxItems: 1
>> +
>> + dmas:
>> + maxItems: 2
>> +
>> + dma-names:
>> + items:
>> + - const: rx
>> + - const: tx
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - clock-names
>> + - clocks
>> + - reset-names
>> + - resets
>> +
>> +additionalProperties: true
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/clock/tegra210-car.h>
>> + #include <dt-bindings/reset/tegra210-car.h>
>> + #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> + spi@70410000 {
>> + compatible = "nvidia,tegra210-qspi";
>> + reg = <0x70410000 0x1000>;
>> + interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&tegra_car TEGRA210_CLK_QSPI>;
>> + clock-names = "qspi";
>> + resets = <&tegra_car 211>;
>> + reset-names = "qspi";
>> + dmas = <&apbdma 5>, <&apbdma 5>;
>> + dma-names = "rx", "tx";
>> + };
>> --
>> 2.7.4
>>

2020-12-10 18:11:40

by Sowjanya Komatineni

[permalink] [raw]

Subject: Re: [PATCH v1 2/7] dt-bindings: spi: Add Tegra QSPI device tree binding

On 12/9/20 12:28 PM, Sowjanya Komatineni wrote:
>
> On 12/9/20 9:26 AM, Rob Herring wrote:
>> On Tue, Dec 01, 2020 at 01:12:43PM -0800, Sowjanya Komatineni wrote:
>>> This patch adds YAML based device tree binding document for Tegra
>>> QSPI driver.
>>>
>>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>>> ---
>>> .../devicetree/bindings/spi/nvidia,tegra-qspi.yaml | 77
>>> ++++++++++++++++++++++
>>> 1 file changed, 77 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
>>> b/Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
>>> new file mode 100644
>>> index 0000000..038a085
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spi/nvidia,tegra-qspi.yaml
>>> @@ -0,0 +1,77 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/spi/nvidia,tegra-qspi.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Tegra Quad SPI Controller
>>> +
>>> +maintainers:
>>> + - Thierry Reding <[email protected]>
>>> + - Jonathan Hunter <[email protected]>
>>> +
>>> +properties:
>>> + compatible:
>>> +    enum:
>>> +      - nvidia,tegra210-qspi
>>> +      - nvidia,tegra186-qspi
>>> +      - nvidia,tegra194-qspi
>>> +
>>> + reg:
>>> +    items:
>>> +      - description: QSPI registers
>> Just 'maxItems: 1'
>>
>>> +
>>> + interrupts:
>>> +    maxItems: 1
>>> +
>>> + clock-names:
>>> +    items:
>>> +      - const: qspi
>> Kind of a pointless name.
> Thanks Rob for feedback. Do you mean to change name something like
> qspi-clk instead of qspi?

Rob, reason I added clock name is later when we add ddr mode we will
have additional clock.

So driver uses clock name to retrieve.

Will add both clocks to dt-binding doc in v2 although support for ddr
mode in qspi driver can be implemented later.

>>
>>> +
>>> + clocks:
>>> +    maxItems: 1
>>> +
>>> + reset-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: qspi
>> Same here.
>>
>>> +
>>> + resets:
>>> +    maxItems: 1
>>> +
>>> + dmas:
>>> +    maxItems: 2
>>> +
>>> + dma-names:
>>> +    items:
>>> +      - const: rx
>>> +      - const: tx
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - interrupts
>>> + - clock-names
>>> + - clocks
>>> + - reset-names
>>> + - resets
>>> +
>>> +additionalProperties: true
>>> +
>>> +examples:
>>> + - |
>>> +    #include <dt-bindings/clock/tegra210-car.h>
>>> +    #include <dt-bindings/reset/tegra210-car.h>
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +    spi@70410000 {
>>> +            compatible = "nvidia,tegra210-qspi";
>>> +            reg = <0x70410000 0x1000>;
>>> +            interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>;
>>> +            clocks = <&tegra_car TEGRA210_CLK_QSPI>;
>>> +            clock-names = "qspi";
>>> +            resets = <&tegra_car 211>;
>>> +            reset-names = "qspi";
>>> +            dmas = <&apbdma 5>, <&apbdma 5>;
>>> +            dma-names = "rx", "tx";
>>> +    };
>>> --
>>> 2.7.4
>>>