Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752533AbaBGRcY (ORCPT ); Fri, 7 Feb 2014 12:32:24 -0500 Received: from smtp.codeaurora.org ([198.145.11.231]:41324 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752371AbaBGRcM (ORCPT ); Fri, 7 Feb 2014 12:32:12 -0500 Date: Fri, 7 Feb 2014 11:32:07 -0600 From: Andy Gross To: "Ivan T. Ivanov" Cc: Mark Brown , Grant Likely , Rob Herring , linux-spi@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Alok Chauhan , Gilad Avidov , Kiran Gunda , Sagar Dharia Subject: Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support Message-ID: <20140207173207.GA19974@qualcomm.com> References: <1391705868-20091-1-git-send-email-iivanov@mm-sol.com> <1391705868-20091-3-git-send-email-iivanov@mm-sol.com> <20140207073952.GA2610@qualcomm.com> <1391766753.27491.60.camel@iivanov-dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1391766753.27491.60.camel@iivanov-dev> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote: > > Hi Andy, > > On Fri, 2014-02-07 at 01:39 -0600, Andy Gross wrote: > > On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote: > > > From: "Ivan T. Ivanov" > > > > > > Qualcomm Universal Peripheral (QUP) core is an AHB slave that > > > provides a common data path (an output FIFO and an input FIFO) > > > for serial peripheral interface (SPI) mini-core. SPI in master mode > > > support up to 50MHz, up to four chip selects, and a programmable > > > data path from 4 bits to 32 bits; MODE0..3 protocols > > > > > > Signed-off-by: Ivan T. Ivanov > > > Cc: Alok Chauhan > > > Cc: Gilad Avidov > > > Cc: Kiran Gunda > > > Cc: Sagar Dharia > > > --- > > > drivers/spi/Kconfig | 14 + > > > drivers/spi/Makefile | 1 + > > > drivers/spi/spi-qup.c | 898 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 913 insertions(+) > > > create mode 100644 drivers/spi/spi-qup.c > > > > > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig > > > index ba9310b..bf8ce6b 100644 > > > --- a/drivers/spi/Kconfig > > > +++ b/drivers/spi/Kconfig > > > @@ -381,6 +381,20 @@ config SPI_RSPI > > > help > > > SPI driver for Renesas RSPI blocks. > > > > > > +config SPI_QUP > > > + tristate "Qualcomm SPI Support with QUP interface" > > > + depends on ARCH_MSM > > > > I'd change to ARCH_MSM_DT. This ensures the OF component is there. > > Ok. will change. > > > > > > + help > > > + Qualcomm Universal Peripheral (QUP) core is an AHB slave that > > > + provides a common data path (an output FIFO and an input FIFO) > > > + for serial peripheral interface (SPI) mini-core. SPI in master > > > + mode support up to 50MHz, up to four chip selects, and a > > > + programmable data path from 4 bits to 32 bits; supports numerous > > > + protocol variants. > > > + > > > + This driver can also be built as a module. If so, the module > > > + will be called spi_qup. > > > + > > > config SPI_S3C24XX > > > tristate "Samsung S3C24XX series SPI" > > > depends on ARCH_S3C24XX > > > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile > > > index 95af48d..e598147 100644 > > > --- a/drivers/spi/Makefile > > > +++ b/drivers/spi/Makefile > > > @@ -59,6 +59,7 @@ spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_PXADMA) += spi-pxa2xx-pxadma.o > > > spi-pxa2xx-platform-$(CONFIG_SPI_PXA2XX_DMA) += spi-pxa2xx-dma.o > > > obj-$(CONFIG_SPI_PXA2XX) += spi-pxa2xx-platform.o > > > obj-$(CONFIG_SPI_PXA2XX_PCI) += spi-pxa2xx-pci.o > > > +obj-$(CONFIG_SPI_QUP) += spi-qup.o > > > obj-$(CONFIG_SPI_RSPI) += spi-rspi.o > > > obj-$(CONFIG_SPI_S3C24XX) += spi-s3c24xx-hw.o > > > spi-s3c24xx-hw-y := spi-s3c24xx.o > > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c > > > new file mode 100644 > > > index 0000000..5eb5e8f > > > --- /dev/null > > > +++ b/drivers/spi/spi-qup.c > > > @@ -0,0 +1,898 @@ > > > +/* > > > + * Copyright (c) 2008-2014, The Linux foundation. All rights reserved. > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > + * it under the terms of the GNU General Public License rev 2 and > > > + * only rev 2 as published by the free Software foundation. > > > + * > > > + * This program is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or fITNESS fOR A PARTICULAR PURPOSE. See the > > > + * GNU General Public License for more details. > > > + */ > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > > Remove this for now. No runtime support. > > Did you see any particular issue with the implementation > or this is just because this platform didn't have support > for power management? > The platform doesn't have support for PM right now. So it's probably better to remove all this and revisit later when it is in place. > > > +#include > > > + > > > > > > + > > > +static int spi_qup_transfer_do(struct spi_qup *controller, > > > + struct spi_qup_device *chip, > > > + struct spi_transfer *xfer) > > > +{ > > > + unsigned long timeout; > > > + int ret = -EIO; > > > + > > > + reinit_completion(&controller->done); > > > + > > > + timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC); > > > + timeout = DIV_ROUND_UP(xfer->len * 8, timeout); > > > + timeout = 100 * msecs_to_jiffies(timeout); > > > + > > > + controller->rx_bytes = 0; > > > + controller->tx_bytes = 0; > > > + controller->error = 0; > > > + controller->xfer = xfer; > > > + > > > + if (spi_qup_set_state(controller, QUP_STATE_RUN)) { > > > + dev_warn(controller->dev, "cannot set RUN state\n"); > > > + goto exit; > > > + } > > > + > > > + if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) { > > > + dev_warn(controller->dev, "cannot set PAUSE state\n"); > > > + goto exit; > > > + } > > > + > > > + spi_qup_fifo_write(controller, xfer); > > > + > > > + if (spi_qup_set_state(controller, QUP_STATE_RUN)) { > > > + dev_warn(controller->dev, "cannot set EXECUTE state\n"); > > > + goto exit; > > > + } > > > + > > > + if (!wait_for_completion_timeout(&controller->done, timeout)) > > > + ret = -ETIMEDOUT; > > > + else > > > + ret = controller->error; > > > +exit: > > > + controller->xfer = NULL; > > > > Should the manipulation of controller->xfer be protected by spinlock? > > :-). Probably. I am wondering, could I avoid locking if firstly place > QUP into RESET state and then access these field. This should stop > all activities in it, right? It's generally safest to not assume the hardware is going to do sane things. I'm concerned about spurious IRQs. > > > > > + controller->error = 0; > > > + controller->rx_bytes = 0; > > > + controller->tx_bytes = 0; > > > + spi_qup_set_state(controller, QUP_STATE_RESET); > > > + return ret; > > > +} > > > + > > > > > > + > > > +/* set clock freq, clock ramp, bits per work */ > > > +static int spi_qup_io_setup(struct spi_device *spi, > > > + struct spi_transfer *xfer) > > > +{ > > > > > > + > > > + /* > > > + * TODO: In BAM mode mask INPUT and OUTPUT service flags in > > > + * to prevent IRQs on FIFO status change. > > > + */ > > > > Remove the TODO. Not necessary. This stuff can be added when it becomes BAM > > enabled. > > Ok. > > > > > > + writel_relaxed(0, controller->base + QUP_OPERATIONAL_MASK); > > > + > > > + return 0; > > > +} > > > + > > > +static int spi_qup_transfer_one(struct spi_master *master, > > > + struct spi_message *msg) > > > +{ > > > + struct spi_qup *controller = spi_master_get_devdata(master); > > > + struct spi_qup_device *chip = spi_get_ctldata(msg->spi); > > > + struct spi_transfer *xfer; > > > + struct spi_device *spi; > > > + unsigned cs_change; > > > + int status; > > > + > > > + spi = msg->spi; > > > + cs_change = 1; > > > + status = 0; > > > + > > > + list_for_each_entry(xfer, &msg->transfers, transfer_list) { > > > + > > > + status = spi_qup_io_setup(spi, xfer); > > > + if (status) > > > + break; > > > + > > > > no locking? This whole code block needs to have some type of mutex_lock to keep > > others from trouncing the hardware while you are doing this transfer. > > This is handled by SPI framework. > Ah I looked through that and didn't see it the first time. But looking again, I see it. You're right, you can ignore this comment. > > > > > + if (cs_change) > > > + spi_qup_assert_cs(controller, chip); > > > > Should the CS be done outside the loop? I'd expect the following sequence to > > happen: > > - change CS > > - Loop and do some transfers > > - deassert CS > > > > In this code, you reinitialize and assert/deassert CS for every transaction. > > > > > + > > > + cs_change = xfer->cs_change; > > > Not exactly. It is allowed that CS goes inactive after every > transaction. This is how I read struct spi_transfer description. Ah ok. This is fine then. > > > > + > > > + /* Do actual transfer */ > > > + status = spi_qup_transfer_do(controller, chip, xfer); > > > + if (status) > > > + break; > > > + > > > + msg->actual_length += xfer->len; > > > + > > > + if (xfer->delay_usecs) > > > + udelay(xfer->delay_usecs); > > > + > > > + if (cs_change) > > > + spi_qup_deassert_cs(controller, chip); > > > + } > > > + > > > + if (status || !cs_change) > > > + spi_qup_deassert_cs(controller, chip); > > > + > > > + msg->status = status; > > > + spi_finalize_current_message(master); > > > + return status; > > > +} > > > + > > > +static int spi_qup_probe(struct platform_device *pdev) > > > +{ > > > + struct spi_master *master; > > > + struct clk *iclk, *cclk; > > > + struct spi_qup *controller; > > > + struct resource *res; > > > + struct device *dev; > > > + void __iomem *base; > > > + u32 data, max_freq, iomode; > > > + int ret, irq, size; > > > + > > > + dev = &pdev->dev; > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + base = devm_ioremap_resource(dev, res); > > > + if (IS_ERR(base)) > > > + return PTR_ERR(base); > > > + > > > + irq = platform_get_irq(pdev, 0); > > > + > > > + if (irq < 0) > > > + return irq; > > > + > > > + cclk = devm_clk_get(dev, "core"); > > > + if (IS_ERR(cclk)) { > > > + dev_err(dev, "cannot get core clock\n"); > > No need to error print. devm_clk_get already outputs something > > Ok. > > > > + return PTR_ERR(cclk); > > > + } > > > + > > > + iclk = devm_clk_get(dev, "iface"); > > > + if (IS_ERR(iclk)) { > > > + dev_err(dev, "cannot get iface clock\n"); > > > > No need to error print. devm_clk_get already outputs something > > Ok. > > > > > > + return PTR_ERR(iclk); > > > + } > > > + > > > + if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq)) > > > + max_freq = 19200000; > > > > I'd set the default to 50MHz as that is the max supported by hardware. I'd just > > set max_freq declaration to 50MHz and then check the value if it is changed via > > DT. > > 50MHz doesn't seems to be supported on all chip sets. Currently common > denominator on all chip sets, that I can see, is 19.2MHz. I have tried > to test it with more than 19.2MHz on APQ8074 and it fails. > I guess my stance is to set it to the hardware max supported frequency if it is not specified. If that needs to be lower on a board because of whatever reason, they override it. > > > > > + > > > + if (!max_freq) { > > > + dev_err(dev, "invalid clock frequency %d\n", max_freq); > > > + return -ENXIO; > > > + } > > > > This is buggy. Remove this and collapse into the of_property_read_u32 if > > statement. On non-zero, check the range for validity. > > True. Will fix. > > > > > > + > > > + ret = clk_set_rate(cclk, max_freq); > > > + if (ret) > > > + dev_warn(dev, "fail to set SPI frequency %d\n", max_freq); > > > > Bail here? > > I don't know. What will be the consequences if controller continue to > operate on its default rate? > It is unclear. But if you can't set the rate that is configured or if there is a misconfiguration, it's probably better to exit the probe and catch it here. > > > + > > > + ret = clk_prepare_enable(cclk); > > > + if (ret) { > > > + dev_err(dev, "cannot enable core clock\n"); > > > + return ret; > > > + } > > > + > > > + ret = clk_prepare_enable(iclk); > > > + if (ret) { > > > + clk_disable_unprepare(cclk); > > > + dev_err(dev, "cannot enable iface clock\n"); > > > + return ret; > > > + } > > > + > > > + data = readl_relaxed(base + QUP_HW_VERSION); > > > + > > > + if (data < QUP_HW_VERSION_2_1_1) { > > > + clk_disable_unprepare(cclk); > > > + clk_disable_unprepare(iclk); > > > + dev_err(dev, "v.%08x is not supported\n", data); > > > + return -ENXIO; > > > + } > > > + > > > + master = spi_alloc_master(dev, sizeof(struct spi_qup)); > > > + if (!master) { > > > + clk_disable_unprepare(cclk); > > > + clk_disable_unprepare(iclk); > > > + dev_err(dev, "cannot allocate master\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + master->bus_num = pdev->id; > > > + master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP; > > > + master->num_chipselect = SPI_NUM_CHIPSELECTS; > > > + master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32); > > > + master->setup = spi_qup_setup; > > > + master->cleanup = spi_qup_cleanup; > > > + master->transfer_one_message = spi_qup_transfer_one; > > > + master->dev.of_node = pdev->dev.of_node; > > > + master->auto_runtime_pm = true; > > > > Remove this. No runtime support > > > > > + > > > + platform_set_drvdata(pdev, master); > > > + > > > + controller = spi_master_get_devdata(master); > > > + > > > + controller->dev = dev; > > > + controller->base = base; > > > + controller->iclk = iclk; > > > + controller->cclk = cclk; > > > + controller->irq = irq; > > > + controller->max_speed_hz = clk_get_rate(cclk); > > > + controller->speed_hz = controller->max_speed_hz; > > > + > > > + init_completion(&controller->done); > > > + > > > + iomode = readl_relaxed(base + QUP_IO_M_MODES); > > > + > > > + size = QUP_IO_M_OUTPUT_BLOCK_SIZE(iomode); > > > + if (size) > > > + controller->out_blk_sz = size * 16; > > > + else > > > + controller->out_blk_sz = 4; > > > + > > > + size = QUP_IO_M_INPUT_BLOCK_SIZE(iomode); > > > + if (size) > > > + controller->in_blk_sz = size * 16; > > > + else > > > + controller->in_blk_sz = 4; > > > + > > > + size = QUP_IO_M_OUTPUT_FIFO_SIZE(iomode); > > > + controller->out_fifo_sz = controller->out_blk_sz * (2 << size); > > > + > > > + size = QUP_IO_M_INPUT_FIFO_SIZE(iomode); > > > + controller->in_fifo_sz = controller->in_blk_sz * (2 << size); > > > + > > > + dev_info(dev, "v.%08x IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n", > > > + data, controller->in_blk_sz, controller->in_fifo_sz, > > > + controller->out_blk_sz, controller->out_fifo_sz); > > > + > > > + writel_relaxed(1, base + QUP_SW_RESET); > > > + > > > + ret = spi_qup_set_state(controller, QUP_STATE_RESET); > > > + if (ret) { > > > + dev_err(dev, "cannot set RESET state\n"); > > > + goto error; > > > + } > > > + > > > + writel_relaxed(0, base + QUP_OPERATIONAL); > > > + writel_relaxed(0, base + QUP_IO_M_MODES); > > > + writel_relaxed(0, base + QUP_OPERATIONAL_MASK); > > > + writel_relaxed(SPI_ERROR_CLK_UNDER_RUN | SPI_ERROR_CLK_OVER_RUN, > > > + base + SPI_ERROR_FLAGS_EN); > > > + > > > + writel_relaxed(0, base + SPI_CONFIG); > > > + writel_relaxed(SPI_IO_C_NO_TRI_STATE, base + SPI_IO_CONTROL); > > > + > > > + ret = devm_request_irq(dev, irq, spi_qup_qup_irq, > > > + IRQF_TRIGGER_HIGH, pdev->name, controller); > > > + if (ret) { > > > + dev_err(dev, "cannot request IRQ %d\n", irq); > > > > unnecessary print > > Will remove. > > > > > > + goto error; > > > + } > > > + > > > + ret = devm_spi_register_master(dev, master); > > > + if (!ret) { > > > + pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC); > > > + pm_runtime_use_autosuspend(dev); > > > + pm_runtime_set_active(dev); > > > + pm_runtime_enable(dev); > > > > Remove all the runtime stuff. not supported right now. > > > > > + return ret; > > > + } > > > +error: > > > + clk_disable_unprepare(cclk); > > > + clk_disable_unprepare(iclk); > > > + spi_master_put(master); > > > + return ret; > > > +} > > > + > > > > > > > > + > > > +static int spi_qup_remove(struct platform_device *pdev) > > > +{ > > > + struct spi_master *master = dev_get_drvdata(&pdev->dev); > > > + struct spi_qup *controller = spi_master_get_devdata(master); > > > + > > > + pm_runtime_get_sync(&pdev->dev); > > > + > > > > Do we need to wait for any current transactions to complete > > and do a devm_free_irq()? > > > > > + clk_disable_unprepare(controller->cclk); > > > + clk_disable_unprepare(controller->iclk); > > My understanding is: > > Disabling clocks will timeout transaction, if any. Core Device driver > will call: devm_spi_unregister(), which will wait pending transactions > to complete and then remove the SPI master. Disabling clocks will confuse the hardware. We cannot disable clocks while the spi core is active and transferring data. > > > > + > > > + pm_runtime_put_noidle(&pdev->dev); > > > + pm_runtime_disable(&pdev->dev); > > > + return 0; > > > +} > > > + > > > +static struct of_device_id spi_qup_dt_match[] = { > > > + { .compatible = "qcom,spi-qup-v2", }, > > > > Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1 > > (msm8974 v2) > > I am not aware of the difference. My board report v.20020000. > Is there difference of handling these controllers? There were some bug fixes between versions. None of those affect SPI (that I can tell), but it's better to be more descriptive and use the full versions in the compatible tags. > > > Thanks, > Ivan > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/