Re: [PATCH v1 3/7] spi: qspi-tegra: Add support for Tegra210 QSPI controller
From: Sowjanya Komatineni
Date: Fri Dec 04 2020 - 17:34:53 EST
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 <skomatineni@xxxxxxxxxx>
---
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