Re: [PATCH 2/2] spi: add driver for Rockchip RK3xxx SoCs integrated SPI
On Tue, Jun 24, 2014 at 12:07:32PM +0800, addy ke wrote:
> In order to facilitate understanding,rockchip SPI controller IP design looks
> similar in its registers to designware. But IC implementation is different
> from
> designware, such as dma request line, register offset, register configuration,
> and so on.So we need a dedicated driver for Rockchip RK3XXX SoCs integrated
> SPI.
Can you be more specific about the differences please?
> +struct rockchip_spi {
> + struct device *dev;
> + struct spi_master *master;
> +
> + struct clk *spiclk;
> + struct clk *apb_pclk;
> +
> + void __iomem*regs;
> + int irq;
> + /*depth of the FIFO buffer */
> + u32 fifo_len;
> + /* max bus freq supported */
> + u32 max_freq;
> + u16 bus_num;
> + /* supported slave numbers */
> + u16 num_cs;
> + enum rockchip_ssi_type type;
The indentation in this struct appears to be all over the place, in
general it's simpler and clearer to not try to align the variable names.
> +static inline void spi_chip_sel(struct rockchip_spi *rs, u16 cs)
> +{
> + writel_relaxed(1 << cs, rs->regs + ROCKCHIP_SPI_SER);
> +}
There's no clear operation I can see and I'm not seeing a set_cs()
operation provided to the core as I'd expect, this means that chip
select handling doesn't work properly.
> +static void rockchip_spi_hw_init(struct rockchip_spi *rs)
> +{
> + u32 fifo;
> +
> + spi_enable_chip(rs, 0);
> + spi_mask_intr(rs, INT_MASK);
> +
> + for (fifo = 2; fifo < 32; fifo++) {
> + writel_relaxed(fifo, rs->regs + ROCKCHIP_SPI_TXFTLR);
> + if (fifo != readl_relaxed(rs->regs + ROCKCHIP_SPI_TXFTLR))
> + break;
> +
> + }
> + rs->fifo_len = (fifo == 31) ? 0 : fifo;
> + writel_relaxed(0, rs->regs + ROCKCHIP_SPI_TXFTLR);
> +}
Some more commenting would make it much clearer what this does.
> +static int rockchip_spi_setup(struct spi_device *spi)
> +{
> + struct rockchip_spi *rs;
> +
> + rs = spi_master_get_devdata(spi->master);
> +
> + pm_runtime_get_sync(rs->dev);
> +
> + if (spi->max_speed_hz > rs->max_freq)
> + spi->max_speed_hz = rs->max_freq;
> +
> + pm_runtime_put(rs->dev);
I can't see any reason to runtime enable the hardware here - there's no
interaction with it.
> +static int rockchip_spi_prepare_message(struct spi_master *master,
> + struct spi_message *msg)
> +{
> + struct rockchip_spi *rs = spi_master_get_devdata(master);
> + struct spi_device *spi = msg->spi;
> +
> + if (spi->chip_select >= rs->num_cs) {
> + dev_err(rs->dev, "chip_select %d is invalide, max is %d\n",
invalid.
> +static void wait_for_not_busy(struct rockchip_spi *rs)
> +{
> + u32 status;
> + unsigned long tmo;
> + int ms;
> +
> + ms = rs->len * 8 * 1000 / rs->speed;
> + ms += 10;
> +
> + tmo = msecs_to_loops(ms);
> + do {
> + status = readl_relaxed(rs->regs + ROCKCHIP_SPI_SR);
> + } while ((status & SR_BUSY) && tmo--);
This is a potentially long busy wait and there's not much headroom if
the calculations are wrong.
> +
> + BUG_ON(!tmo);
I'd expect a WARN_ON() at most here.
> +static int wait_for_dma(struct rockchip_spi *rs)
> +{
> + unsigned long tmo;
> + int ms;
> +
> + ms = rs->len * 8 * 1000 / rs->speed;
> + ms += 10;
10ms probably isn't enough headroom on a loaded system - the scheduler
may take longer than that to run the thread. It looks like you could
also be using the core timeout code here, return a positive value from
transfer_one() and then call spi_finalize_current_transfer() when done.
> +static int rockchip_spi_transfer_one(struct spi_master *master,
> + struct spi_device *spi,
> + struct spi_transfer *xfer)
> +{
> + int ret = 0;
> + struct rockchip_spi *rs = spi_master_get_devdata(master);
> +
> + if (!xfer->tx_buf && !xfer->rx_buf) {
> + dev_err(rs->dev, "No buffer for transfer\n");
> + return -EINVAL;
> + }
Let the core worry about things like this.
> + rs->speed = xfer->speed_hz?:spi->max_speed_hz;
The core will ensure that the transfer will have the correct speed set.
In general there's a lot of copy values from the transfer into the
driver data, it seems like it'd be simpler to just refer to the original
source of the data.
> + rs->tx = (void *)xfer->tx_buf;
Why is the driver casting away const here?
> +static irqreturn_t rockchip_spi_irq(int irq, void *data)
> +{
> + struct rockchip_spi *rs = data;
> + u32 isr = readl_relaxed(rs->regs + ROCKCHIP_SPI_ISR);
> +
> +
Re: [PATCH 2/2] spi: add driver for Rockchip RK3xxx SoCs integrated SPI
On Tue, Jun 24, 2014 at 12:07:32PM +0800, addy ke wrote:
In order to facilitate understanding,rockchip SPI controller IP design looks
similar in its registers to designware. But IC implementation is different
from
designware, such as dma request line, register offset, register configuration,
and so on.So we need a dedicated driver for Rockchip RK3XXX SoCs integrated
SPI.
Can you be more specific about the differences please?
+struct rockchip_spi {
+ struct device *dev;
+ struct spi_master *master;
+
+ struct clk *spiclk;
+ struct clk *apb_pclk;
+
+ void __iomem*regs;
+ int irq;
+ /*depth of the FIFO buffer */
+ u32 fifo_len;
+ /* max bus freq supported */
+ u32 max_freq;
+ u16 bus_num;
+ /* supported slave numbers */
+ u16 num_cs;
+ enum rockchip_ssi_type type;
The indentation in this struct appears to be all over the place, in
general it's simpler and clearer to not try to align the variable names.
+static inline void spi_chip_sel(struct rockchip_spi *rs, u16 cs)
+{
+ writel_relaxed(1 cs, rs-regs + ROCKCHIP_SPI_SER);
+}
There's no clear operation I can see and I'm not seeing a set_cs()
operation provided to the core as I'd expect, this means that chip
select handling doesn't work properly.
+static void rockchip_spi_hw_init(struct rockchip_spi *rs)
+{
+ u32 fifo;
+
+ spi_enable_chip(rs, 0);
+ spi_mask_intr(rs, INT_MASK);
+
+ for (fifo = 2; fifo 32; fifo++) {
+ writel_relaxed(fifo, rs-regs + ROCKCHIP_SPI_TXFTLR);
+ if (fifo != readl_relaxed(rs-regs + ROCKCHIP_SPI_TXFTLR))
+ break;
+
+ }
+ rs-fifo_len = (fifo == 31) ? 0 : fifo;
+ writel_relaxed(0, rs-regs + ROCKCHIP_SPI_TXFTLR);
+}
Some more commenting would make it much clearer what this does.
+static int rockchip_spi_setup(struct spi_device *spi)
+{
+ struct rockchip_spi *rs;
+
+ rs = spi_master_get_devdata(spi-master);
+
+ pm_runtime_get_sync(rs-dev);
+
+ if (spi-max_speed_hz rs-max_freq)
+ spi-max_speed_hz = rs-max_freq;
+
+ pm_runtime_put(rs-dev);
I can't see any reason to runtime enable the hardware here - there's no
interaction with it.
+static int rockchip_spi_prepare_message(struct spi_master *master,
+ struct spi_message *msg)
+{
+ struct rockchip_spi *rs = spi_master_get_devdata(master);
+ struct spi_device *spi = msg-spi;
+
+ if (spi-chip_select = rs-num_cs) {
+ dev_err(rs-dev, chip_select %d is invalide, max is %d\n,
invalid.
+static void wait_for_not_busy(struct rockchip_spi *rs)
+{
+ u32 status;
+ unsigned long tmo;
+ int ms;
+
+ ms = rs-len * 8 * 1000 / rs-speed;
+ ms += 10;
+
+ tmo = msecs_to_loops(ms);
+ do {
+ status = readl_relaxed(rs-regs + ROCKCHIP_SPI_SR);
+ } while ((status SR_BUSY) tmo--);
This is a potentially long busy wait and there's not much headroom if
the calculations are wrong.
+
+ BUG_ON(!tmo);
I'd expect a WARN_ON() at most here.
+static int wait_for_dma(struct rockchip_spi *rs)
+{
+ unsigned long tmo;
+ int ms;
+
+ ms = rs-len * 8 * 1000 / rs-speed;
+ ms += 10;
10ms probably isn't enough headroom on a loaded system - the scheduler
may take longer than that to run the thread. It looks like you could
also be using the core timeout code here, return a positive value from
transfer_one() and then call spi_finalize_current_transfer() when done.
+static int rockchip_spi_transfer_one(struct spi_master *master,
+ struct spi_device *spi,
+ struct spi_transfer *xfer)
+{
+ int ret = 0;
+ struct rockchip_spi *rs = spi_master_get_devdata(master);
+
+ if (!xfer-tx_buf !xfer-rx_buf) {
+ dev_err(rs-dev, No buffer for transfer\n);
+ return -EINVAL;
+ }
Let the core worry about things like this.
+ rs-speed = xfer-speed_hz?:spi-max_speed_hz;
The core will ensure that the transfer will have the correct speed set.
In general there's a lot of copy values from the transfer into the
driver data, it seems like it'd be simpler to just refer to the original
source of the data.
+ rs-tx = (void *)xfer-tx_buf;
Why is the driver casting away const here?
+static irqreturn_t rockchip_spi_irq(int irq, void *data)
+{
+ struct rockchip_spi *rs = data;
+ u32 isr = readl_relaxed(rs-regs + ROCKCHIP_SPI_ISR);
+
+ dev_dbg(rs-dev, isr 0x%x\n, isr);
+
+ return IRQ_HANDLED;
+}
This is completely ignoring the interrupt except for logging it - it'd
be simpler
[PATCH 2/2] spi: add driver for Rockchip RK3xxx SoCs integrated SPI
In order to facilitate understanding,rockchip SPI controller IP design looks similar in its registers to designware. But IC implementation is different from designware, such as dma request line, register offset, register configuration, and so on.So we need a dedicated driver for Rockchip RK3XXX SoCs integrated SPI. Signed-off-by: addy ke --- drivers/spi/Kconfig| 11 + drivers/spi/Makefile | 1 + drivers/spi/spi-rockchip.c | 894 + 3 files changed, 906 insertions(+) create mode 100644 drivers/spi/spi-rockchip.c diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 60f2b41..5b51ab2 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -375,6 +375,17 @@ config SPI_PXA2XX config SPI_PXA2XX_PCI def_tristate SPI_PXA2XX && PCI +config SPI_ROCKCHIP + tristate "Rockchip SPI controller driver" + help + This selects a driver for Rockchip SPI controller. + + If you say yes to this option, support will be included for + RK3066, RK3188 and RK3288 families of SPI controller. + Rockchip SPI controller support DMA transport and PIO mode. + The main usecase of this controller is to use spi flash as boot + device. + config SPI_RSPI tristate "Renesas RSPI/QSPI controller" depends on (SUPERH && SH_DMAE_BASE) || ARCH_SHMOBILE diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index bd79266..361fbf3 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -60,6 +60,7 @@ 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_ROCKCHIP) += spi-rockchip.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-rockchip.c b/drivers/spi/spi-rockchip.c new file mode 100644 index 000..bd7c218 --- /dev/null +++ b/drivers/spi/spi-rockchip.c @@ -0,0 +1,894 @@ +/* + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd + * Author: addy ke + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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 +#include +#include +#include +#include +#include + +#define DRIVER_NAME "rockchip-spi" + +/* SPI register offsets */ +#define ROCKCHIP_SPI_CTRLR00x +#define ROCKCHIP_SPI_CTRLR10x0004 +#define ROCKCHIP_SPI_SSIENR0x0008 +#define ROCKCHIP_SPI_SER 0x000c +#define ROCKCHIP_SPI_BAUDR 0x0010 +#define ROCKCHIP_SPI_TXFTLR0x0014 +#define ROCKCHIP_SPI_RXFTLR0x0018 +#define ROCKCHIP_SPI_TXFLR 0x001c +#define ROCKCHIP_SPI_RXFLR 0x0020 +#define ROCKCHIP_SPI_SR0x0024 +#define ROCKCHIP_SPI_IPR 0x0028 +#define ROCKCHIP_SPI_IMR 0x002c +#define ROCKCHIP_SPI_ISR 0x0030 +#define ROCKCHIP_SPI_RISR 0x0034 +#define ROCKCHIP_SPI_ICR 0x0038 +#define ROCKCHIP_SPI_DMACR 0x003c +#define ROCKCHIP_SPI_DMATDLR 0x0040 +#define ROCKCHIP_SPI_DMARDLR 0x0044 +#define ROCKCHIP_SPI_TXDR 0x0400 +#define ROCKCHIP_SPI_RXDR 0x0800 + +/* Bit fields in CTRLR0 */ +#define CR0_DFS_OFFSET 0 + +#define CR0_CFS_OFFSET 2 + +#define CR0_SCPH_OFFSET6 + +#define CR0_SCPOL_OFFSET 7 + +#define CR0_CSM_OFFSET 8 +#define CR0_CSM_KEEP 0x0 +/* ss_n be high for half sclk_out cycles */ +#define CR0_CSM_HALF 0X1 +/* ss_n be high for one sclk_out cycle */ +#define CR0_CSM_ONE0x2 + +/* ss_n to sclk_out delay */ +#define CR0_SSD_OFFSET 10 +/* + * The period between ss_n active and + * sclk_out active is half sclk_out cycles + */ +#define CR0_SSD_HALF 0x0 +/* + * The period between ss_n active and + * sclk_out active is one sclk_out cycle + */ +#define CR0_SSD_ONE
[PATCH 2/2] spi: add driver for Rockchip RK3xxx SoCs integrated SPI
In order to facilitate understanding,rockchip SPI controller IP design looks similar in its registers to designware. But IC implementation is different from designware, such as dma request line, register offset, register configuration, and so on.So we need a dedicated driver for Rockchip RK3XXX SoCs integrated SPI. Signed-off-by: addy ke addy...@rock-chips.com --- drivers/spi/Kconfig| 11 + drivers/spi/Makefile | 1 + drivers/spi/spi-rockchip.c | 894 + 3 files changed, 906 insertions(+) create mode 100644 drivers/spi/spi-rockchip.c diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 60f2b41..5b51ab2 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -375,6 +375,17 @@ config SPI_PXA2XX config SPI_PXA2XX_PCI def_tristate SPI_PXA2XX PCI +config SPI_ROCKCHIP + tristate Rockchip SPI controller driver + help + This selects a driver for Rockchip SPI controller. + + If you say yes to this option, support will be included for + RK3066, RK3188 and RK3288 families of SPI controller. + Rockchip SPI controller support DMA transport and PIO mode. + The main usecase of this controller is to use spi flash as boot + device. + config SPI_RSPI tristate Renesas RSPI/QSPI controller depends on (SUPERH SH_DMAE_BASE) || ARCH_SHMOBILE diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index bd79266..361fbf3 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -60,6 +60,7 @@ 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_ROCKCHIP) += spi-rockchip.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-rockchip.c b/drivers/spi/spi-rockchip.c new file mode 100644 index 000..bd7c218 --- /dev/null +++ b/drivers/spi/spi-rockchip.c @@ -0,0 +1,894 @@ +/* + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd + * Author: addy ke addy...@rock-chips.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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/init.h +#include linux/module.h +#include linux/clk.h +#include linux/err.h +#include linux/delay.h +#include linux/interrupt.h +#include linux/platform_device.h +#include linux/slab.h +#include linux/spi/spi.h +#include linux/scatterlist.h +#include linux/of.h +#include linux/pm_runtime.h +#include linux/io.h +#include linux/scatterlist.h +#include linux/dmaengine.h + +#define DRIVER_NAME rockchip-spi + +/* SPI register offsets */ +#define ROCKCHIP_SPI_CTRLR00x +#define ROCKCHIP_SPI_CTRLR10x0004 +#define ROCKCHIP_SPI_SSIENR0x0008 +#define ROCKCHIP_SPI_SER 0x000c +#define ROCKCHIP_SPI_BAUDR 0x0010 +#define ROCKCHIP_SPI_TXFTLR0x0014 +#define ROCKCHIP_SPI_RXFTLR0x0018 +#define ROCKCHIP_SPI_TXFLR 0x001c +#define ROCKCHIP_SPI_RXFLR 0x0020 +#define ROCKCHIP_SPI_SR0x0024 +#define ROCKCHIP_SPI_IPR 0x0028 +#define ROCKCHIP_SPI_IMR 0x002c +#define ROCKCHIP_SPI_ISR 0x0030 +#define ROCKCHIP_SPI_RISR 0x0034 +#define ROCKCHIP_SPI_ICR 0x0038 +#define ROCKCHIP_SPI_DMACR 0x003c +#define ROCKCHIP_SPI_DMATDLR 0x0040 +#define ROCKCHIP_SPI_DMARDLR 0x0044 +#define ROCKCHIP_SPI_TXDR 0x0400 +#define ROCKCHIP_SPI_RXDR 0x0800 + +/* Bit fields in CTRLR0 */ +#define CR0_DFS_OFFSET 0 + +#define CR0_CFS_OFFSET 2 + +#define CR0_SCPH_OFFSET6 + +#define CR0_SCPOL_OFFSET 7 + +#define CR0_CSM_OFFSET 8 +#define CR0_CSM_KEEP 0x0 +/* ss_n be high for half sclk_out cycles */ +#define CR0_CSM_HALF 0X1 +/* ss_n be high for one sclk_out cycle */ +#define CR0_CSM_ONE0x2 + +/* ss_n to sclk_out delay */ +#define CR0_SSD_OFFSET 10 +/* + * The period between
