[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28
Marek Vasut
marex at denx.de
Tue Jun 26 08:22:54 EDT 2012
Dear Shawn Guo,
> On Sat, Jun 23, 2012 at 08:43:53PM +0200, Marek Vasut wrote:
> > This is slightly reworked version of the SPI driver.
> > Support for DT has been added and it's been converted
> > to queued API.
> >
> > Based on previous attempt by:
> > Fabio Estevam <fabio.estevam at freescale.com>
> >
> > Signed-off-by: Fabio Estevam <fabio.estevam at freescale.com>
> > Signed-off-by: Marek Vasut <marex at denx.de>
> > Cc: Chris Ball <cjb at laptop.org>
> > Cc: Detlev Zundel <dzu at denx.de>
> > CC: Dong Aisheng <b29396 at freescale.com>
> > Cc: Grant Likely <grant.likely at secretlab.ca>
> > Cc: Linux ARM kernel <linux-arm-kernel at lists.infradead.org>
> > Cc: Rob Herring <rob.herring at calxeda.com>
> > CC: Shawn Guo <shawn.guo at linaro.org>
> > Cc: Stefano Babic <sbabic at denx.de>
> > Cc: Wolfgang Denk <wd at denx.de>
> > ---
> >
> > drivers/spi/Kconfig | 6 +
> > drivers/spi/Makefile | 1 +
> > drivers/spi/spi-mxs.c | 407
> > +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 414
> > insertions(+)
> > create mode 100644 drivers/spi/spi-mxs.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index cd2fe35..85ca6a7 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -355,6 +355,12 @@ config SPI_STMP3XXX
> >
> > help
> >
> > SPI driver for Freescale STMP37xx/378x SoC SSP interface
> >
> > +config SPI_MXS
> > + tristate "Freescale MXS SPI controller"
> > + depends on ARCH_MXS
> > + help
> > + SPI driver for Freescale MXS devices.
> > +
> >
> > config SPI_TEGRA
> >
> > tristate "Nvidia Tegra SPI controller"
> > depends on ARCH_TEGRA && TEGRA_SYSTEM_DMA
> >
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 9d75d21..a684d1e 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -35,6 +35,7 @@ obj-$(CONFIG_SPI_LM70_LLP) += spi-lm70llp.o
> >
> > obj-$(CONFIG_SPI_MPC512x_PSC) += spi-mpc512x-psc.o
> > obj-$(CONFIG_SPI_MPC52xx_PSC) += spi-mpc52xx-psc.o
> > obj-$(CONFIG_SPI_MPC52xx) += spi-mpc52xx.o
> >
> > +obj-$(CONFIG_SPI_MXS) += spi-mxs.o
> >
> > obj-$(CONFIG_SPI_NUC900) += spi-nuc900.o
> > obj-$(CONFIG_SPI_OC_TINY) += spi-oc-tiny.o
> > obj-$(CONFIG_SPI_OMAP_UWIRE) += spi-omap-uwire.o
> >
> > diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> > new file mode 100644
> > index 0000000..81a2643
> > --- /dev/null
> > +++ b/drivers/spi/spi-mxs.c
> > @@ -0,0 +1,407 @@
> > +/*
> > + * Freescale MXS SPI master driver
> > + *
> > + * Copyright 2012 DENX Software Engineering, GmbH.
> > + * Copyright 2012 Freescale Semiconductor, Inc.
> > + * Copyright 2008 Embedded Alley Solutions, Inc All Rights Reserved.
> > + *
> > + * Rework and transition to new API by:
> > + * Marek Vasut <marex at denx.de>
> > + *
> > + * Based on previous attempt by:
> > + * Fabio Estevam <fabio.estevam at freescale.com>
> > + *
> > + * Based on code from U-Boot bootloader by:
> > + * Marek Vasut <marex at denx.de>
> > + *
> > + * Based on spi-stmp.c, which is:
> > + * Author: Dmitry Pervushin <dimka at embeddedalley.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * 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 <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/ioport.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/delay.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/highmem.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/completion.h>
> > +#include <linux/gpio.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/fsl/mxs-dma.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/stmp_device.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/mxs-spi.h>
>
> All these headers are needed? dma-mapping.h, dmaengine.h, highmem.h,
> mxs-dma.h etc? Or you will need them later?
Not later, I already do need them. It's just that I didn't submit the last patch
yet, since I developed it only after these were submitted.
> > +
> > +#define SSP_TIMEOUT 200 /* 200 ms */
> > +
> > +static int mxs_spi_setup_transfer(struct spi_device *spi,
> > + struct spi_transfer *t)
> > +{
> > + struct mxs_ssp *ssp = spi_master_get_devdata(spi->master);
> > + uint8_t bits_per_word;
> > + uint32_t hz = 0;
> > +
> > + bits_per_word = spi->bits_per_word;
> > + if (t && t->bits_per_word)
> > + bits_per_word = t->bits_per_word;
> > +
> > + if (bits_per_word != 8) {
> > + dev_err(&spi->dev, "%s, unsupported bits_per_word=%d\n",
> > + __func__, bits_per_word);
> > + return -EINVAL;
> > + }
> > +
> > + if (spi->max_speed_hz)
> > + hz = spi->max_speed_hz;
> > + if (t && t->speed_hz)
> > + hz = t->speed_hz;
> > + if (hz == 0) {
> > + dev_err(&spi->dev, "Cannot continue with zero clock\n");
> > + return -EINVAL;
> > + }
> > +
> > + mxs_ssp_set_clk_rate(ssp, hz);
> > +
> > + writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
> > + BF_SSP_CTRL1_WORD_LENGTH
> > + (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
> > + ((spi->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
> > + ((spi->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
> > + ssp->base + HW_SSP_CTRL1(ssp));
> > +
> > + writel(0x0, ssp->base + HW_SSP_CMD0 + STMP_OFFSET_REG_SET);
> > +
> > + return 0;
> > +}
> > +
> > +static void mxs_spi_cleanup(struct spi_device *spi)
> > +{
> > + return;
> > +}
> > +
> > +/* the spi->mode bits understood by this driver: */
>
> Incomplete comment? Remove it or make it complete?
Good catch.
> > +static int mxs_spi_setup(struct spi_device *spi)
> > +{
> > + int err = 0;
> > +
> > + if (!spi->bits_per_word)
> > + spi->bits_per_word = 8;
> > +
> > + if (spi->mode & ~(SPI_CPOL | SPI_CPHA))
> > + return -EINVAL;
> > +
> > + err = mxs_spi_setup_transfer(spi, NULL);
> > + if (err) {
> > + dev_err(&spi->dev,
> > + "Failed to setup transfer, error = %d\n", err);
> > + }
>
> Unnecessary braces.
It's much more readable for multiline code.
> > +
> > + return err;
> > +}
> > +
> > +static void mxs_spi_set_cs(struct mxs_ssp *ssp, unsigned cs)
> > +{
> > + const uint32_t mask =
> > + BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ;
> > + uint32_t select = 0;
> > +
> > + writel(mask, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > +
> > + if (cs & 1)
> > + select |= BM_SSP_CTRL0_WAIT_FOR_CMD;
> > + if (cs & 2)
> > + select |= BM_SSP_CTRL0_WAIT_FOR_IRQ;
> > +
> > + writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +}
> > +
> > +static inline void mxs_spi_enable(struct mxs_ssp *ssp)
> > +{
> > + writel(BM_SSP_CTRL0_LOCK_CS,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > + writel(BM_SSP_CTRL0_IGNORE_CRC,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > +}
> > +
> > +static inline void mxs_spi_disable(struct mxs_ssp *ssp)
> > +{
> > + writel(BM_SSP_CTRL0_LOCK_CS,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > + writel(BM_SSP_CTRL0_IGNORE_CRC,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +}
> > +
> > +static int mxs_ssp_wait_set(struct mxs_ssp *ssp, int offset, int mask)
> > +{
> > + unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
> > +
> > + while (!(readl_relaxed(ssp->base + offset) & mask)) {
> > + udelay(1);
> > + if (time_after(jiffies, timeout))
> > + return -ETIMEDOUT;
> > + }
> > + return 0;
> > +}
> > +
> > +static int mxs_ssp_wait_clr(struct mxs_ssp *ssp, int offset, int mask)
> > +{
> > + unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
> > +
> > + while ((readl_relaxed(ssp->base + offset) & mask)) {
> > + udelay(1);
> > + if (time_after(jiffies, timeout))
> > + return -ETIMEDOUT;
> > + }
> > + return 0;
> > +}
> > +
>
> Merge these two functions into mxs_ssp_wait with one more argument?
>
> > +static int mxs_spi_txrx_pio(struct mxs_ssp *ssp, int cs,
> > + unsigned char *buf, int len,
> > + int *first, int *last, int write)
> > +{
> > + if (*first) {
> > + mxs_spi_enable(ssp);
> > + *first = 0;
> > + }
> > +
> > + mxs_spi_set_cs(ssp, cs);
> > +
> > + while (len--) {
> > + if (*last && len == 0) {
> > + mxs_spi_disable(ssp);
> > + *last = 0;
> > + }
> > +
> > + if (ssp->devid == IMX23_SSP) {
> > + writel(BM_SSP_CTRL0_XFER_COUNT,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > + writel(1,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > + } else {
> > + writel(1, ssp->base + HW_SSP_XFER_SIZE);
> > + }
> > +
> > + if (write)
> > + writel(BM_SSP_CTRL0_READ,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > + else
> > + writel(BM_SSP_CTRL0_READ,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +
> > + writel(BM_SSP_CTRL0_RUN,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +
> > + if (mxs_ssp_wait_set(ssp, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN))
> > + return -ETIMEDOUT;
> > +
> > + if (write)
> > + writel(*buf, ssp->base + HW_SSP_DATA);
> > +
> > + writel(BM_SSP_CTRL0_DATA_XFER,
> > + ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +
> > + if (!write) {
> > + if (mxs_ssp_wait_clr(ssp, HW_SSP_STATUS(ssp),
> > + BM_SSP_STATUS_FIFO_EMPTY))
> > + return -ETIMEDOUT;
> > +
> > + *buf = (readl(ssp->base + HW_SSP_DATA) & 0xff);
> > + }
> > +
> > + if (mxs_ssp_wait_clr(ssp, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN))
> > + return -ETIMEDOUT;
> > +
> > + buf++;
> > + }
> > +
> > + if (len <= 0)
> > + return 0;
> > +
> > + return -ETIMEDOUT;
> > +}
> > +
> > +static int mxs_spi_transfer_one(struct spi_master *spi, struct
> > spi_message *m) +{
> > + struct mxs_ssp *ssp = spi_master_get_devdata(spi);
> > + int first, last;
> > + struct spi_transfer *t, *tmp_t;
> > + int status = 0;
> > + int cs;
> > +
> > + first = last = 0;
> > +
> > + cs = m->spi->chip_select;
> > +
> > + list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
> > +
> > + mxs_spi_setup_transfer(m->spi, t);
> > +
> > + if (&t->transfer_list == m->transfers.next)
> > + first = !0;
>
> Why not simply "first = 1;"?
>
> > + if (&t->transfer_list == m->transfers.prev)
> > + last = !0;
>
> Ditto
>
> > + if (t->rx_buf && t->tx_buf) {
> > + pr_debug("%s: cannot send and receive simultaneously\n",
> > + __func__);
>
> dev_dbg? Then __func__ is not important to be there.
Correct.
> > + return -EINVAL;
> > + }
> > +
> > + if (t->tx_buf)
> > + status = mxs_spi_txrx_pio(ssp, cs, (void *)t->tx_buf,
>
> Is the cast really needed? The t->rx_buf below seems not having it.
Yes, it is. Since t->tx_buf is const void *, we need to cast it so the compiler
won't complain.
> > + t->len, &first, &last, 1);
> > + if (t->rx_buf)
> > + status = mxs_spi_txrx_pio(ssp, cs, t->rx_buf,
> > + t->len, &first, &last, 0);
> > + m->actual_length += t->len;
> > + if (status)
> > + break;
> > +
> > + first = last = 0;
> > + }
> > +
> > + m->status = 0;
> > + spi_finalize_current_message(spi);
> > +
> > + return status;
> > +}
> > +
> > +static const struct of_device_id mxs_spi_dt_ids[] = {
> > + { .compatible = "fsl,imx23-spi", .data = (void *) IMX23_SSP, },
> > + { .compatible = "fsl,imx28-spi", .data = (void *) IMX28_SSP, },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mxs_spi_dt_ids);
> > +
> > +static int __devinit mxs_spi_probe(struct platform_device *pdev)
> > +{
> > + const struct of_device_id *of_id =
> > + of_match_device(mxs_spi_dt_ids, &pdev->dev);
> > + struct device_node *np = pdev->dev.of_node;
> > + struct spi_master *host;
> > + struct mxs_ssp *ssp;
> > + struct resource *iores;
> > + struct pinctrl *pinctrl;
> > + int ret = 0;
> > +
> > + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!iores)
> > + return -EINVAL;
>
> Moving the call right before devm_request_and_ioremap seems reasonable?
> Also the error check could be saved, since devm_request_and_ioremap
> will take care of it.
Actually I tried to follow the code path of mxs_mmc here as much as possible,
hoping we might eventually be able to converge them and make some code shared.
> > +
> > + host = spi_alloc_master(&pdev->dev, sizeof(struct mxs_ssp));
>
> sizeof(*ssp)
>
> > + if (!host)
> > + return -ENOMEM;
> > +
> > + ssp = spi_master_get_devdata(host);
> > + ssp->dev = &pdev->dev;
> > + ssp->base = devm_request_and_ioremap(&pdev->dev, iores);
> > + if (!ssp->base) {
> > + ret = -EADDRNOTAVAIL;
> > + goto out_spi_free;
> > + }
> > +
> > + if (np)
> > + ssp->devid = (enum mxs_ssp_id) of_id->data;
> > + else
> > + ssp->devid = pdev->id_entry->driver_data;
> > +
> > + host->transfer_one_message = mxs_spi_transfer_one;
> > + host->setup = mxs_spi_setup;
> > + host->cleanup = mxs_spi_cleanup;
> > + host->mode_bits = SPI_CPOL | SPI_CPHA;
> > + host->num_chipselect = 3;
> > + host->dev.of_node = np;
> > +
> > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> > + if (IS_ERR(pinctrl)) {
> > + ret = PTR_ERR(pinctrl);
> > + goto out_spi_free;
> > + }
> > +
> > + ssp->clk = clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(ssp->clk)) {
> > + ret = PTR_ERR(ssp->clk);
> > + goto out_spi_free;
> > + }
> > +
> > + clk_prepare_enable(ssp->clk);
> > + ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
> > +
> > + stmp_reset_block(ssp->base);
> > +
> > + platform_set_drvdata(pdev, host);
> > +
> > + ret = spi_register_master(host);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Cannot register SPI master, %d\n", ret);
> > + goto out_clk_put;
> > + }
> > +
> > + return 0;
> > +
> > +out_clk_put:
> > + clk_disable_unprepare(ssp->clk);
> > + clk_put(ssp->clk);
> > +out_spi_free:
> > + spi_master_put(host);
>
> spi_master_put will not free the memory, so we have to call kfree to
> do it.
>
> > + return ret;
> > +}
> > +
> > +static int __devexit mxs_spi_remove(struct platform_device *pdev)
> > +{
> > + struct spi_master *host;
> > + struct mxs_ssp *ssp;
> > +
> > + host = platform_get_drvdata(pdev);
> > + if (host)
> > + return 0;
>
> Hmm, why the check and return here?
If it's not set, things went _very_ wrong somewhere. Maybe BUG_ON() might be
better?
> > +
> > + ssp = spi_master_get_devdata(host);
> > +
> > + spi_unregister_master(host);
> > +
> > + platform_set_drvdata(pdev, NULL);
> > +
> > + clk_disable_unprepare(ssp->clk);
> > + clk_put(ssp->clk);
> > +
> > + spi_master_put(host);
>
> kfree
>
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver mxs_spi_driver = {
> > + .probe = mxs_spi_probe,
> > + .remove = __devexit_p(mxs_spi_remove),
> > + .driver = {
> > + .name = "mxs-spi",
> > + .owner = THIS_MODULE,
> > + .of_match_table = mxs_spi_dt_ids,
> > + },
> > +};
> > +
> > +module_platform_driver(mxs_spi_driver);
> > +
> > +MODULE_AUTHOR("Dmitry Pervushin <dimka at embeddedalley.com>");
> > +MODULE_DESCRIPTION("MXS SPI master driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:mxs-spi");
More information about the linux-arm-kernel
mailing list