Skip to contentSkip to conversation

spi: Add SPI driver for Mikrotik RB4xx series boards

PATCH Superseded Bert Vermeulen 3 participants 7 messages Apr 6, 2015 – Apr 10, 2015

Subsystems (2)

v6 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

PATCH v6 1/1 spi: Add SPI driver for Mikrotik RB4xx series boards

This driver mediates access between the connected CPLD and other devices on the bus.

The m25p80-compatible boot flash and (some models) MMC use regular SPI, bitbanged as required by the SoC. However the SPI-connected CPLD has a "fast write" mode, in which two bits are transferred by SPI clock cycle. The second bit is transmitted with the SoC's CS2 pin.

Protocol drivers using this fast write facility signal this by setting the cs_change flag on transfers.

---
Changes in v6:
- removed unnecessary SPI mode check
- whitespace fixes

 drivers/spi/Kconfig     |   6 ++
 drivers/spi/Makefile    |   1 +
 drivers/spi/spi-rb4xx.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/spi/spi-rb4xx.c
drivers/spi/Kconfig +6 -0
index ab8dfbe..8b1beaf6 100644
@@ -429,6 +429,12 @@ config SPI_ROCKCHIP
 	  The main usecase of this controller is to use spi flash as boot
 	  device.
 
+config SPI_RB4XX
+	tristate "Mikrotik RB4XX SPI master"
+	depends on SPI_MASTER && ATH79
+	help
+	  SPI controller driver for the Mikrotik RB4xx series boards.
+
 config SPI_RSPI
 	tristate "Renesas RSPI/QSPI controller"
 	depends on SUPERH || ARCH_SHMOBILE || COMPILE_TEST
drivers/spi/Makefile +1 -0
index d8cbf65..0218f39 100644
@@ -66,6 +66,7 @@ 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_RB4XX)			+= spi-rb4xx.o
 obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
 spi-s3c24xx-hw-y			:= spi-s3c24xx.o
drivers/spi/spi-rb4xx.c new file +239 -0

239 lines not on this page · raw message

Signature · 6 lines
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reviewed-by on v6 1/1
#1 lore raw
Quoted 11 lines · Bert Vermeulen

This driver mediates access between the connected CPLD and other devices on the bus.

The m25p80-compatible boot flash and (some models) MMC use regular SPI, bitbanged as required by the SoC. However the SPI-connected CPLD has a "fast write" mode, in which two bits are transferred by SPI clock cycle. The second bit is transmitted with the SoC's CS2 pin.

Protocol drivers using this fast write facility signal this by setting the cs_change flag on transfers.

(If someone gives you a tag, please, keep it in the sequential version(s) if there are only cosmetic changes)

Quoted 290 lines

---
Changes in v6:
- removed unnecessary SPI mode check
- whitespace fixes

 drivers/spi/Kconfig     |   6 ++
 drivers/spi/Makefile    |   1 +
 drivers/spi/spi-rb4xx.c | 239 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/spi/spi-rb4xx.c
drivers/spi/Kconfig +6 -0
index ab8dfbe..8b1beaf6 100644
@@ -429,6 +429,12 @@ config SPI_ROCKCHIP
          The main usecase of this controller is to use spi flash as boot
          device.
 
+config SPI_RB4XX
+       tristate "Mikrotik RB4XX SPI master"
+       depends on SPI_MASTER && ATH79
+       help
+         SPI controller driver for the Mikrotik RB4xx series boards.
+
 config SPI_RSPI
        tristate "Renesas RSPI/QSPI controller"
        depends on SUPERH || ARCH_SHMOBILE || COMPILE_TEST
drivers/spi/Makefile +1 -0
index d8cbf65..0218f39 100644
@@ -66,6 +66,7 @@ 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_RB4XX)                        += spi-rb4xx.o
 obj-$(CONFIG_SPI_RSPI)                 += spi-rspi.o
 obj-$(CONFIG_SPI_S3C24XX)              += spi-s3c24xx-hw.o
 spi-s3c24xx-hw-y                       := spi-s3c24xx.o
drivers/spi/spi-rb4xx.c new file +239 -1
new file mode 100644
index 0000000..0cead2e
@@ -0,0 +1,239 @@
+/*
+ * SPI controller driver for the Mikrotik RB4xx boards
+ *
+ * Copyright (C) 2010 Gabor Juhos <[email protected]>
+ * Copyright (C) 2015 Bert Vermeulen <[email protected]>
+ *
+ * This file was based on the patches for Linux 2.6.27.39 published by
+ * MikroTik for their RouterBoard 4xx series devices.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/spi/spi.h>
+
+#include <asm/mach-ath79/ar71xx_regs.h>
+
+#define SPI_CLOCK_FASTEST      0x40
+#define SPI_HZ                 33333334
+#define CPLD_CMD_WRITE_NAND    0x08 /* bulk write mode */
+
+struct rb4xx_spi {
+       void __iomem            *base;
+       struct spi_master       *master;
+};
+
+static inline u32 rb4xx_read(struct rb4xx_spi *rbspi, u32 reg)
+{
+       return __raw_readl(rbspi->base + reg);
+}
+
+static inline void rb4xx_write(struct rb4xx_spi *rbspi, u32 reg, u32 value)
+{
+       __raw_writel(value, rbspi->base + reg);
+}
+
+static inline void do_spi_clk(struct rb4xx_spi *rbspi, u32 spi_ioc, int value)
+{
+       u32 regval;
+
+       regval = spi_ioc;
+       if (value & BIT(0))
+               regval |= AR71XX_SPI_IOC_DO;
+
+       rb4xx_write(rbspi, AR71XX_SPI_REG_IOC, regval);
+       rb4xx_write(rbspi, AR71XX_SPI_REG_IOC, regval | AR71XX_SPI_IOC_CLK);
+}
+
+static void do_spi_byte(struct rb4xx_spi *rbspi, u32 spi_ioc, u8 byte)
+{
+       int i;
+
+       for (i = 7; i >= 0; i--)
+               do_spi_clk(rbspi, spi_ioc, byte >> i);
+}
+
+static inline void do_spi_clk_fast(struct rb4xx_spi *rbspi, u32 spi_ioc,
+                                  u8 value)
+{
+       u32 regval;
+
+       regval = spi_ioc;
+       if (value & BIT(1))
+               regval |= AR71XX_SPI_IOC_DO;
+       if (value & BIT(0))
+               regval |= AR71XX_SPI_IOC_CS2;
+
+       rb4xx_write(rbspi, AR71XX_SPI_REG_IOC, regval);
+       rb4xx_write(rbspi, AR71XX_SPI_REG_IOC, regval | AR71XX_SPI_IOC_CLK);
+}
+
+/* Two bits at a time, msb first */
+static void do_spi_byte_fast(struct rb4xx_spi *rbspi, u32 spi_ioc, u8 byte)
+{
+       do_spi_clk_fast(rbspi, spi_ioc, byte >> 6);
+       do_spi_clk_fast(rbspi, spi_ioc, byte >> 4);
+       do_spi_clk_fast(rbspi, spi_ioc, byte >> 2);
+       do_spi_clk_fast(rbspi, spi_ioc, byte >> 0);
+}
+
+static void rb4xx_set_cs(struct spi_device *spi, bool enable)
+{
+       struct rb4xx_spi *rbspi = spi_master_get_devdata(spi->master);
+
+       /*
+        * Setting CS is done along with bitbanging the actual values,
+        * since it's all on the same hardware register. However the
+        * CPLD needs CS deselected after every command.
+        */
+       if (!enable)
+               rb4xx_write(rbspi, AR71XX_SPI_REG_IOC,
+                           AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1);
+}
+
+/* Deselect CS0 and CS1. */
+static int rb4xx_unprepare_transfer_hardware(struct spi_master *master)
+{
+       struct rb4xx_spi *rbspi = spi_master_get_devdata(master);
+
+       rb4xx_write(rbspi, AR71XX_SPI_REG_IOC,
+                   AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1);
+
+       return 0;
+}
+
+static int rb4xx_transfer_one(struct spi_master *master,
+                             struct spi_device *spi, struct spi_transfer *t)
+{
+       struct rb4xx_spi *rbspi = spi_master_get_devdata(master);
+       int i;
+       u32 spi_ioc;
+       u8 *rx_buf;
+       const u8 *tx_buf;
+       unsigned char out;
+
+       /*
+        * Prime the SPI register with the SPI device selected. The m25p80 boot
+        * flash and CPLD share the CS0 pin. This works because the CPLD's
+        * command set was designed to almost not clash with that of the
+        * boot flash.
+        */
+       if (spi->chip_select == 2)
+               spi_ioc = AR71XX_SPI_IOC_CS0;
+       else
+               spi_ioc = AR71XX_SPI_IOC_CS1;
+
+       tx_buf = t->tx_buf;
+       rx_buf = t->rx_buf;
+       for (i = 0; i < t->len; ++i) {
+               out = tx_buf ? tx_buf[i] : 0x00;
+               if (spi->chip_select == 1 && t->cs_change) {
+                       /* CPLD in bulk write mode gets two bits per clock */
+                       do_spi_byte_fast(rbspi, spi_ioc, out);
+                       /* Don't want the real CS toggled */
+                       t->cs_change = 0;
+               } else {
+                       do_spi_byte(rbspi, spi_ioc, out);
+               }
+               if (!rx_buf)
+                       continue;
+               rx_buf[i] = rb4xx_read(rbspi, AR71XX_SPI_REG_RDS);
+       }
+       spi_finalize_current_transfer(master);
+
+       return 0;
+}
+
+static int rb4xx_spi_setup(struct spi_device *spi)
+{
+       if (spi->bits_per_word != 8 && spi->bits_per_word != 0) {
+               dev_err(&spi->dev, "bits_per_word %u not supported\n",
+                       spi->bits_per_word);
+               return -EINVAL;
+       }
+
+       return 0;
+}
+
+static int rb4xx_spi_probe(struct platform_device *pdev)
+{
+       struct spi_master *master;
+       struct clk *ahb_clk;
+       struct rb4xx_spi *rbspi;
+       struct resource *r;
+       int err;
+       void __iomem *spi_base;
+
+       ahb_clk = devm_clk_get(&pdev->dev, "ahb");
+       if (IS_ERR(ahb_clk))
+               return PTR_ERR(ahb_clk);
+
+       err = clk_prepare_enable(ahb_clk);
+       if (err)
+               return err;
+
+       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       spi_base = devm_ioremap_resource(&pdev->dev, r);
+       if (!spi_base)
+               return PTR_ERR(spi_base);
+
+       master = spi_alloc_master(&pdev->dev, sizeof(*rbspi));
+       if (!master)
+               return -ENOMEM;
+
+       master->bus_num = 0;
+       master->num_chipselect = 3;
+       master->setup = rb4xx_spi_setup;
+       master->transfer_one = rb4xx_transfer_one;
+       master->unprepare_transfer_hardware = rb4xx_unprepare_transfer_hardware;
+       master->set_cs = rb4xx_set_cs;
+
+       rbspi = spi_master_get_devdata(master);
+       rbspi->master = master;
+       rbspi->base = spi_base;
+
+       platform_set_drvdata(pdev, rbspi);
+
+       err = spi_register_master(master);
+       if (err) {
+               dev_err(&pdev->dev, "failed to register SPI master\n");
+               spi_master_put(master);
+               return err;
+       }
+
+       /* Enable SPI */
+       rb4xx_write(rbspi, AR71XX_SPI_REG_FS, AR71XX_SPI_FS_GPIO);
+
+       return 0;
+}
+
+static int rb4xx_spi_remove(struct platform_device *pdev)
+{
+       struct rb4xx_spi *rbspi = platform_get_drvdata(pdev);
+
+       spi_master_put(rbspi->master);
+
+       return 0;
+}
+
+static struct platform_driver rb4xx_spi_drv = {
+       .probe = rb4xx_spi_probe,
+       .remove = rb4xx_spi_remove,
+       .driver = {
+               .name = "rb4xx-spi",
+       },
+};
+
+module_platform_driver(rb4xx_spi_drv);
+
+MODULE_DESCRIPTION("Mikrotik RB4xx SPI controller driver");
+MODULE_AUTHOR("Gabor Juhos <[email protected]>");
+MODULE_AUTHOR("Bert Vermeulen <[email protected]>");
+MODULE_LICENSE("GPL v2");
--

1.9.1

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#2 lore raw
In reply to Bert Vermeulen

drivers/spi/spi-rb4xx.c line 136

Quoted 2 lines · Bert Vermeulen
drivers/spi/spi-rb4xx.c new file +2 -0
+	for (i = 0; i < t->len; ++i) {
+		out = tx_buf ? tx_buf[i] : 0x00;

This looks like the driver needs to set SPI_MASTER_MUST_TX.

drivers/spi/spi-rb4xx.c line 110

Quoted 10 lines
drivers/spi/spi-rb4xx.c new file +10 -0
+/* Deselect CS0 and CS1. */
+static int rb4xx_unprepare_transfer_hardware(struct spi_master *master)
+{
+       struct rb4xx_spi *rbspi = spi_master_get_devdata(master);
+
+       rb4xx_write(rbspi, AR71XX_SPI_REG_IOC,
+                   AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1);
+
+       return 0;
+}

This seems broken - if chip select needs to be deasserted it should be be deasserted before we get to unprepare, otherwise if more than one SPI message is queued at once then presumably chip select won't be deasserted between them which would break things. This is what the set_cs() operation is for...

drivers/spi/spi-rb4xx.c line 144

Quoted 8 lines
drivers/spi/spi-rb4xx.c new file +8 -0
+		if (spi->chip_select == 1 && t->cs_change) {
+			/* CPLD in bulk write mode gets two bits per clock */
+			do_spi_byte_fast(rbspi, spi_ioc, out);
+			/* Don't want the real CS toggled */
+			t->cs_change = 0;
+		} else {
+			do_spi_byte(rbspi, spi_ioc, out);
+		}

This is making very little sense to me and the fact that the driver is messing with cs_change is definitely buggy, it'll mean that repeated use of the same transfer will be broken. What is the above code supposed to do, both with regard to selecting "fast" mode (why would you want slow mode?) and with regard to the chip select?

I queried this on a previous version and asked for the code to be better documented...

drivers/spi/spi-rb4xx.c line 163

Quoted 10 lines
drivers/spi/spi-rb4xx.c new file +10 -0
+static int rb4xx_spi_setup(struct spi_device *spi)
+{
+	if (spi->bits_per_word != 8 && spi->bits_per_word != 0) {
+		dev_err(&spi->dev, "bits_per_word %u not supported\n",
+			spi->bits_per_word);
+		return -EINVAL;
+	}
+
+	return 0;
+}

This should be removed, the driver should be setting bits_per_word_mask.

drivers/spi/spi-rb4xx.c line 180

Quoted 7 lines
drivers/spi/spi-rb4xx.c new file +7 -0
+	ahb_clk = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(ahb_clk))
+		return PTR_ERR(ahb_clk);
+
+	err = clk_prepare_enable(ahb_clk);
+	if (err)
+		return err;

There's no error handling (or indeed any other code) disabling the clock, probably this enable should happen later and those disables definitely need adding. I'd also expect to see runtime PM to keep the clock disabled when the controller isn't in use in order to save power.

drivers/spi/spi-rb4xx.c line 221

Quoted 5 lines
drivers/spi/spi-rb4xx.c new file +5 -0
+static int rb4xx_spi_remove(struct platform_device *pdev)
+{
+	struct rb4xx_spi *rbspi = platform_get_drvdata(pdev);
+
+	spi_master_put(rbspi->master);

devm_spi_register_master().

#3 lore raw
Quoted 15 lines · Bert Vermeulen

drivers/spi/spi-rb4xx.c line 144

Quoted 8 lines
drivers/spi/spi-rb4xx.c new file +8 -0
+		if (spi->chip_select == 1 && t->cs_change) {
+			/* CPLD in bulk write mode gets two bits per clock */
+			do_spi_byte_fast(rbspi, spi_ioc, out);
+			/* Don't want the real CS toggled */
+			t->cs_change = 0;
+		} else {
+			do_spi_byte(rbspi, spi_ioc, out);
+		}

This is making very little sense to me and the fact that the driver is messing with cs_change is definitely buggy, it'll mean that repeated use of the same transfer will be broken. What is the above code supposed to do, both with regard to selecting "fast" mode (why would you want slow mode?) and with regard to the chip select?

Quoted 2 lines

I queried this on a previous version and asked for the code to be better documented...

I documented it in the commit message:

The m25p80-compatible boot flash and (some models) MMC use regular SPI, bitbanged as required by the SoC. However the SPI-connected CPLD has a "fast write" mode, in which two bits are transferred by SPI clock cycle. The second bit is transmitted with the SoC's CS2 pin.

Protocol drivers using this fast write facility signal this by setting the cs_change flag on transfers.

The cs_change flag is used here instead of the openwrt version's spi_transfer.fast_write flag. The CPLD driver sets this flag on a per-transfer basis.

I wish I could tell you more about it, but I only know what I found in this code.

Quoted 4 lines
Quoted 4 lines
drivers/spi/spi-rb4xx.c new file +4 -0
+	ahb_clk = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(ahb_clk))
+		return PTR_ERR(ahb_clk);
+
Quoted 8 lines

drivers/spi/spi-rb4xx.c line 180

Quoted 3 lines
drivers/spi/spi-rb4xx.c new file +3 -0
+	err = clk_prepare_enable(ahb_clk);
+	if (err)
+		return err;

There's no error handling (or indeed any other code) disabling the clock, probably this enable should happen later and those disables definitely need adding. I'd also expect to see runtime PM to keep the clock disabled when the controller isn't in use in order to save power.

No problem disabling the clock on error/remove, but PM doesn't seem worth doing in this case -- the ath79 platform's clock enable/disable are just dummy functions. The ar7100 series SoCs have no power management, it seems.

I have a patch addressing all your other comments.

Signature · 5 lines
Bert Vermeulen        [email protected]          email/xmpp
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#4 lore raw
Quoted 6 lines · Bert Vermeulen

On 04/06/2015 06:39 PM, Mark Brown wrote:> On Mon, Apr 06, 2015 at

Quoted 2 lines

I queried this on a previous version and asked for the code to be better documented...

I documented it in the commit message:

I'm asking for the *code* to be better documented. Right now it's just raising obvious questions which are at best going to cost people time digging for the reasons.

Quoted 11 lines

The m25p80-compatible boot flash and (some models) MMC use regular SPI, bitbanged as required by the SoC. However the SPI-connected CPLD has a "fast write" mode, in which two bits are transferred by SPI clock cycle. The second bit is transmitted with the SoC's CS2 pin.

Protocol drivers using this fast write facility signal this by setting the cs_change flag on transfers.

The cs_change flag is used here instead of the openwrt version's spi_transfer.fast_write flag. The CPLD driver sets this flag on a per-transfer basis.

No, this is broken - it's abusing a standard API in a way that's completly incompatible with the meaning of that API which is obviously a very bad idea, especially since good practice is to offload the implementation of that standard API to the core. It *sounds* like you're just trying to implement two wire mode which does have a standard API, please use that.

#5 lore raw
Quoted 13 lines · Mark Brown
Quoted 11 lines · Bert Vermeulen

The m25p80-compatible boot flash and (some models) MMC use regular SPI, bitbanged as required by the SoC. However the SPI-connected CPLD has a "fast write" mode, in which two bits are transferred by SPI clock cycle. The second bit is transmitted with the SoC's CS2 pin.

Protocol drivers using this fast write facility signal this by setting the cs_change flag on transfers.

The cs_change flag is used here instead of the openwrt version's spi_transfer.fast_write flag. The CPLD driver sets this flag on a per-transfer basis.

Quoted 6 lines

No, this is broken - it's abusing a standard API in a way that's completly incompatible with the meaning of that API which is obviously a very bad idea, especially since good practice is to offload the implementation of that standard API to the core. It *sounds* like you're just trying to implement two wire mode which does have a standard API, please use that.

Can you please advise what kind of solution would be acceptable then? I need to signal from an SPI protocol driver to an SPI master on a per-transfer basis.

Adding a flag to struct spi_transfer for this one driver, as openwrt does, seems stupid -- I agree. And sure, using spi_transfer.cs_change is a little dodgy. But I don't see a standard way to do it otherwise, and I don't really want to keep trying things until you approve of one. So tell me how you would do it, and I'll implement it that way. I just want to get this code in.

Also, I have no idea what you mean by two-wire mode. This "fast mode" is SPI
+ one extra pin.

Signature · 5 lines
Bert Vermeulen        [email protected]          email/xmpp
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#6 lore raw
Quoted 9 lines · Bert Vermeulen
Quoted 5 lines · Mark Brown

On Thu, Apr 09, 2015 at 11:31:12PM +0200, Bert Vermeulen wrote:

implementation of that standard API to the core. It *sounds* like you're just trying to implement two wire mode which does have a standard API, please use that.

Can you please advise what kind of solution would be acceptable then? I need to signal from an SPI protocol driver to an SPI master on a per-transfer basis.

Please refer to my reply above...

Quoted 2 lines

Also, I have no idea what you mean by two-wire mode. This "fast mode" is SPI
+ one extra pin.

SPI_TX_DUAL.