This patchset adds support for Broadcoms MSPI SPI controller. It can be found on
various SoC's such as Cygnus.
Jonathan Richardson (2):
ARM: dts: Add binding for Broadcom MSPI driver.
spi: bcm-mspi: Add support for Broadcom MSPI driver.
.../devicetree/bindings/spi/brcm,mspi-spi.txt | 39 ++
drivers/spi/Kconfig | 5 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-bcm-mspi.c | 505 ++++++++++++++++++++
4 files changed, 550 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
create mode 100644 drivers/spi/spi-bcm-mspi.c
--
1.7.9.5
Signed-off-by: Jonathan Richardson <[email protected]>
---
.../devicetree/bindings/spi/brcm,mspi-spi.txt | 39 ++++++++++++++++++++
1 file changed, 39 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
diff --git a/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
new file mode 100644
index 0000000..e86a7a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
@@ -0,0 +1,39 @@
+Broadcom MSPI controller
+
+The Broadcom MSPI controller is a SPI controller found on various chips such
+as Cygnus.
+
+Required properties:
+- compatible: Must be "brcm,mspi-v0".
+
+- reg: The first register is the physical base address of the MSPI controller.
+ The second register is the address of the MSPI interrupt control registers. It
+ is only required for chips that have a separate register set for extended
+ interrupt control. This is required for Cygnus.
+
+- interrupts: Interrupt ID of the controller.
+
+Optional properties:
+- clocks: The MSPI reference clock. If not provided then it is assumed a clock
+ is enabled by default and no control of clock-frequency (see below) is
+ possible.
+
+- clock-names: The name of the reference clock.
+
+- clock-frequency: Desired frequency of the clock. This will set the serial
+ clock baud rate (SPBR) based on the reference clock frequency. The frequency
+ of the SPBR is mspi_clk / (2 * SPBR) where SPBR is a value between 1-255
+ determined by the desired 'clock-frequency'. If not provided then the default
+ baud rate of the controller is used.
+
+Example:
+
+mspi: mspi@18047200 {
+ compatible = "brcm,mspi-v0";
+ reg = <0x18047200 0x188>,
+ <0x180473a0 0x1c>;
+ interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&axi41_clk>;
+ clock-names = "mspi_clk";
+ clock-frequency = <12500000>;
+};
--
1.7.9.5
The MSPI controller is a SPI controller found on various Broadcom
SoC's such as Cygnus.
Signed-off-by: Jonathan Richardson <[email protected]>
---
drivers/spi/Kconfig | 5 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-bcm-mspi.c | 505 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 511 insertions(+)
create mode 100644 drivers/spi/spi-bcm-mspi.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ab8dfbe..23622c2 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -121,6 +121,11 @@ config SPI_BCM53XX
help
Enable support for the SPI controller on Broadcom BCM53xx ARM SoCs.
+config SPI_BCM_MSPI
+ tristate "Broadcom MSPI controller"
+ help
+ Enable support for the Broadcom MSPI controller.
+
config SPI_BCM63XX
tristate "Broadcom BCM63xx SPI controller"
depends on BCM63XX
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d8cbf65..5ebecba 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_SPI_BCM2835) += spi-bcm2835.o
obj-$(CONFIG_SPI_BCM53XX) += spi-bcm53xx.o
obj-$(CONFIG_SPI_BCM63XX) += spi-bcm63xx.o
obj-$(CONFIG_SPI_BCM63XX_HSSPI) += spi-bcm63xx-hsspi.o
+obj-$(CONFIG_SPI_BCM_MSPI) += spi-bcm-mspi.o
obj-$(CONFIG_SPI_BFIN5XX) += spi-bfin5xx.o
obj-$(CONFIG_SPI_ADI_V3) += spi-adi-v3.o
obj-$(CONFIG_SPI_BFIN_SPORT) += spi-bfin-sport.o
diff --git a/drivers/spi/spi-bcm-mspi.c b/drivers/spi/spi-bcm-mspi.c
new file mode 100644
index 0000000..9f6cc03
--- /dev/null
+++ b/drivers/spi/spi-bcm-mspi.c
@@ -0,0 +1,505 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * 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 version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; 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/platform_device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/spi/spi.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+
+#define MSPI_SPCR0_LSB_OFFSET 0x0
+#define MSPI_SPCR0_LSB_SHIFT 0
+#define SPBR_MIN 8U
+#define SPBR_MAX 255U
+#define MSPI_NEWQP_OFFSET 0x10
+#define MSPI_ENDQP_OFFSET 0x14
+#define MSPI_SPCR2_OFFSET 0x18
+#define MSPI_SPCR2_SPE_SHIFT 6
+#define MSPI_SPCR2_SPIFIE_SHIFT 5
+#define MSPI_SPCR2_CONT_AFTER_CMD_SHIFT 7
+#define MSPI_STATUS_OFFSET 0x20
+#define MSPI_TXRAM_OFFSET 0x40
+#define MSPI_RXRAM_OFFSET 0xc0
+#define MSPI_CDRAM_OFFSET 0x140
+#define MSPI_CDRAM_CONT_SHIFT 7
+#define MSPI_CDRAM_BITSE_SHIFT 6
+#define MSPI_CDRAM_PCS_SHIFT 0
+#define MSPI_WRITE_LOCK_OFFSET 0x180
+#define INTERRUPT_MSPI_DONE_OFFSET 0x14
+
+#define NUM_SLOTS 16
+
+struct bcm_mspi {
+ struct platform_device *pdev;
+ void __iomem *base;
+ /* Base of interrupt control regs. */
+ void __iomem *int_base;
+ struct spi_master *master;
+ struct clk *clk;
+ u32 spbr;
+ struct completion xfer_done;
+ int rx_slot;
+};
+
+/*
+ * Calculate TXRAM offset for the requested slot.
+ */
+static inline u32 tx_offset(int slot)
+{
+ BUG_ON(slot > NUM_SLOTS);
+ return 4 * (slot * 2);
+}
+
+/*
+ * Calculate RXRAM offset for the requested slot.
+ */
+static inline u32 rx_offset(int slot)
+{
+ BUG_ON(slot > NUM_SLOTS);
+ return 4 * (1 + slot * 2);
+}
+
+/*
+ * Calculate CDRAM offset for the requested slot.
+ */
+static inline u32 cdram_offset(int slot)
+{
+ BUG_ON(slot > NUM_SLOTS);
+ return MSPI_CDRAM_OFFSET + (4 * slot);
+}
+
+/*
+ * Start tx or rx SPI transfer and wait until complete. The queue start pointer
+ * is always 0. The end queue pointer is passed in to end_slot.
+ *
+ * @set_cont Set the CONT bit if true, clear if false. The CONT bit must be set
+ * if all slots are filled and there is more data to be transferred.
+ * @end_slot The end queue pointer.
+ * Returns 0 if successful, -EIO if transfer timed out.
+ */
+static int bcm_mspi_start_transfer(struct bcm_mspi *mspi, bool set_cont,
+ int end_slot)
+{
+ int val;
+ int err = 0;
+
+ /* Set queue pointers for transmit. */
+ writel(0, mspi->base + MSPI_NEWQP_OFFSET);
+ writel(end_slot, mspi->base + MSPI_ENDQP_OFFSET);
+ dev_dbg(&mspi->pdev->dev, "NEWQP: %d ENDQP: %d\n", 0, end_slot);
+
+ reinit_completion(&mspi->xfer_done);
+
+ writel(1, mspi->base + MSPI_WRITE_LOCK_OFFSET);
+
+ /*
+ * Start the transfer. CONT bit is set if another tx SPI transfer
+ * is required.
+ */
+ val = (1 << MSPI_SPCR2_SPE_SHIFT) | (1 << MSPI_SPCR2_SPIFIE_SHIFT);
+ if (set_cont)
+ val |= (1 << MSPI_SPCR2_CONT_AFTER_CMD_SHIFT);
+
+ writel(val, mspi->base + MSPI_SPCR2_OFFSET);
+ dev_dbg(&mspi->pdev->dev, "SPCR2: %d\n", val);
+
+ /* Wait for interrupt indicating transfer is complete. */
+ if (!wait_for_completion_timeout(&mspi->xfer_done,
+ msecs_to_jiffies(10))) {
+ dev_err(&mspi->pdev->dev,
+ "timeout waiting for tx MSPI interrupt\n");
+ err = -ETIMEDOUT;
+ }
+
+ writel(0, mspi->base + MSPI_WRITE_LOCK_OFFSET);
+
+ return err;
+}
+
+/*
+ * Copies data from tx buffer to the h/w tx buffer (TXRAM). When all tx slots
+ * have been filled, a SPI transfer is initiated to send the data to the device.
+ * Continues until all data has been sent.
+ *
+ * Data is copied into the h/w transmit buffer starting at TXRAM0. The CONT bit
+ * is set in the corresponding command register (CDRAMx) if there is another
+ * byte to send. It is cleared on the last byte.
+ *
+ * The number of bytes sent is saved and is the start of the receive buffer for
+ * the next receive transfer. See bcm_mspi_rx_data().
+ */
+static int bcm_mspi_tx_data(struct spi_master *master,
+ struct spi_device *spi_dev, struct spi_transfer *transfer)
+{
+ struct bcm_mspi *mspi = spi_master_get_devdata(master);
+ int slot = 0;
+ const u8 *buf = transfer->tx_buf;
+ u32 val;
+ int bytes_processed = 0;
+ int err;
+
+ if (!transfer->tx_buf)
+ return 0;
+
+ dev_dbg(&mspi->pdev->dev, "tx %d bytes\n", transfer->len);
+
+ while (bytes_processed < transfer->len) {
+ bool last_slot;
+
+ /*
+ * Write data to slots until all are filled or all
+ * bytes written.
+ */
+ for (slot = 0; slot < NUM_SLOTS; slot++) {
+ u32 txram_offset = MSPI_TXRAM_OFFSET + tx_offset(slot);
+ u32 msb = *buf++;
+
+ val = (spi_dev->chip_select << MSPI_CDRAM_PCS_SHIFT) |
+ (1 << MSPI_CDRAM_CONT_SHIFT);
+
+ writel(msb, mspi->base + txram_offset);
+ dev_dbg(&mspi->pdev->dev, "TXRAM: write 0x%x to 0x%x\n",
+ msb, txram_offset);
+
+ bytes_processed++;
+
+ if (bytes_processed >= transfer->len)
+ last_slot = true;
+ else
+ last_slot = false;
+
+ /*
+ * Update command register. CONT cleared
+ * on last slot.
+ */
+ if (last_slot)
+ val &= ~(1 << MSPI_CDRAM_CONT_SHIFT);
+
+ writel(val, mspi->base + cdram_offset(slot));
+ dev_dbg(&mspi->pdev->dev, "CDRAM: write 0x%x to 0x%x\n",
+ val, cdram_offset(slot));
+
+ /* Stop filling slots if all data written. */
+ if (last_slot)
+ break;
+ }
+
+ /* Start transfer and wait until complete. */
+ err = bcm_mspi_start_transfer(mspi, !last_slot, slot);
+ if (err)
+ return err;
+
+ /* Delay requested amount before next transfer. */
+ udelay(transfer->delay_usecs);
+ }
+
+ /* The rx data will go into RXRAM0/1 + last tx length. */
+ if (slot + 1 >= NUM_SLOTS)
+ mspi->rx_slot = 0;
+ else
+ mspi->rx_slot = slot + 1;
+
+ return 0;
+}
+
+/*
+ * Receives data from the device by configuring the command registers (CDRAMx)
+ * and then initiating a receive transfer. When the transfer is complete, the
+ * data is copied to the receive buffer. Continues until all all data has
+ * been received.
+ *
+ * The received data is copied into the RXRAM buffer starting at RXRAM0 + the
+ * last tx length + 1. After the transfer is complete, the data is received
+ * starting at RXRAM0 again until another tx transfer is done. The CONT bit
+ * is always set on the command register (CDRAMx) corresponding to the RXRAM
+ * slot. The next unused slot is also configured except the CONT bit is cleared.
+ * This quirk applies to rx transfers only.
+ */
+static int bcm_mspi_rx_data(struct spi_master *master,
+ struct spi_device *spi_dev, struct spi_transfer *transfer)
+{
+ struct bcm_mspi *mspi = spi_master_get_devdata(master);
+ int slot, rx_slot, end_slot;
+ int bytes_processed = 0;
+ u8 *buf = transfer->rx_buf;
+ u32 val;
+ int err;
+
+ if (!transfer->rx_buf)
+ return 0;
+
+ dev_dbg(&mspi->pdev->dev, "rx %d bytes\n", transfer->len);
+
+ /* Receive all rx data. */
+ while (bytes_processed < transfer->len) {
+ bool last_slot;
+
+ /* Set command register for each slot. */
+ for (slot = 0; slot < NUM_SLOTS; slot++) {
+ last_slot = (slot + 1 >= transfer->len);
+
+ val = (spi_dev->chip_select << MSPI_CDRAM_PCS_SHIFT) |
+ (1 << MSPI_CDRAM_CONT_SHIFT);
+
+ /* Update command register. */
+ writel(val, mspi->base + cdram_offset(slot));
+ dev_dbg(&mspi->pdev->dev, "CDRAM: write 0x%x to 0x%x\n",
+ val, cdram_offset(slot));
+
+ /* Stop filling slots if all data written. */
+ if (last_slot)
+ break;
+ }
+
+ /* CONT bit is cleared on the next unused slot. */
+ end_slot = slot;
+ if (slot + 1 < NUM_SLOTS) {
+ end_slot += 1;
+ val &= ~(1 << MSPI_CDRAM_CONT_SHIFT);
+ writel(val, mspi->base + cdram_offset(end_slot));
+ dev_dbg(&mspi->pdev->dev, "CDRAM: write 0x%x to 0x%x\n",
+ val, cdram_offset(end_slot));
+ }
+
+ /* Start transfer and wait until complete. */
+ err = bcm_mspi_start_transfer(mspi, !last_slot, end_slot);
+ if (err)
+ return err;
+
+ /* Copy data from rx registers to rx buffer. */
+ for (rx_slot = mspi->rx_slot; rx_slot < NUM_SLOTS; rx_slot++) {
+ u32 rxram_offset = MSPI_RXRAM_OFFSET +
+ rx_offset(rx_slot);
+ u32 msb = readl(mspi->base + rxram_offset);
+
+ dev_dbg(&mspi->pdev->dev, "rxram offset: %x. data = 0x%x\n",
+ rxram_offset, msb);
+
+ *buf++ = (u8)msb;
+ bytes_processed++;
+
+ if (bytes_processed >= transfer->len)
+ break;
+ }
+
+ /*
+ * The read pointer always starts at RXRAM0 after an rx transfer
+ * of any length.
+ */
+ mspi->rx_slot = 0;
+
+ /* Delay requested amount before next transfer. */
+ udelay(transfer->delay_usecs);
+ }
+
+ return 0;
+}
+
+static int bcm_mspi_transfer_one(struct spi_master *master,
+ struct spi_device *spidev, struct spi_transfer *transfer)
+{
+ int err;
+
+ /* 8 bit transfers only are currently supported. */
+ if (transfer->bits_per_word > 8)
+ return -ENOTSUPP;
+
+ err = bcm_mspi_tx_data(master, spidev, transfer);
+ if (err)
+ return err;
+
+ err = bcm_mspi_rx_data(master, spidev, transfer);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+/*
+ * The ISR is called when a SPI transfer has completed. SPIF (MSPI finished)
+ * will be set in the status register.
+ */
+static irqreturn_t bcm_mspi_isr(int irq, void *data)
+{
+ struct platform_device *pdev = data;
+ struct bcm_mspi *mspi = platform_get_drvdata(pdev);
+ u32 val;
+
+ val = readl(mspi->base + MSPI_STATUS_OFFSET);
+ if (val & 1) {
+ /* Clear interrupt then signal completion of transfer. */
+ val &= ~1;
+ writel(val, mspi->base + MSPI_STATUS_OFFSET);
+ if (mspi->int_base)
+ writel(1, mspi->int_base + INTERRUPT_MSPI_DONE_OFFSET);
+
+ complete(&mspi->xfer_done);
+
+ return IRQ_HANDLED;
+ }
+
+ return IRQ_NONE;
+}
+
+static void bcm_mspi_hw_init(struct bcm_mspi *mspi)
+{
+ /* Set SPBR (serial clock baud rate). */
+ if (mspi->spbr)
+ writel(mspi->spbr << MSPI_SPCR0_LSB_SHIFT,
+ mspi->base + MSPI_SPCR0_LSB_OFFSET);
+}
+
+static const struct of_device_id bcm_mspi_dt[] = {
+ { .compatible = "brcm,mspi" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, bcm_mspi_dt);
+
+static int bcm_mspi_probe(struct platform_device *pdev)
+{
+ struct bcm_mspi *data;
+ struct spi_master *master;
+ struct device *dev = &pdev->dev;
+ int err;
+ struct resource *res;
+ unsigned int irq;
+
+ dev_info(dev, "Initializing BCM MSPI\n");
+
+ master = spi_alloc_master(dev, sizeof(*data));
+ if (!master) {
+ dev_err(dev, "error allocating spi_master\n");
+ return -ENOMEM;
+ }
+
+ platform_set_drvdata(pdev, master);
+
+ data = spi_master_get_devdata(master);
+ data->master = master;
+ data->pdev = pdev;
+ platform_set_drvdata(pdev, data);
+ init_completion(&data->xfer_done);
+
+ /* SPI master will always use the SPI device(s) from DT. */
+ master->dev.of_node = dev->of_node;
+ master->transfer_one = bcm_mspi_transfer_one;
+
+ /*
+ * Enable clock if provided. The frequency can be changed by setting
+ * SPBR (serial clock baud rate) based on the desired 'clock-frequency'.
+ *
+ * Baud rate is calculated as: mspi_clk / (2 * SPBR) where SPBR is a
+ * value between 1-255. If not set then it is left at the h/w default.
+ */
+ data->clk = devm_clk_get(dev, "mspi_clk");
+ if (!IS_ERR(data->clk)) {
+ u32 desired_rate = 0;
+
+ err = clk_prepare_enable(data->clk);
+ if (err < 0) {
+ dev_err(dev, "failed to enable clock: %d\n", err);
+ goto out;
+ }
+
+ /* Calculate SPBR if clock-frequency provided. */
+ of_property_read_u32(dev->of_node, "clock-frequency",
+ &desired_rate);
+ if (desired_rate > 0) {
+ u32 spbr = clk_get_rate(data->clk) / (2 * desired_rate);
+
+ if (spbr > 0) {
+ data->spbr = clamp_val(spbr, SPBR_MIN,
+ SPBR_MAX);
+ } else {
+ dev_err(dev, "failed to get clock rate: %d\n",
+ spbr);
+ err = spbr;
+ goto out;
+ }
+ }
+ } else {
+ /* Don't report error if clock not specified - it's optional. */
+ if (PTR_ERR(data->clk) != -ENOENT) {
+ err = PTR_ERR(data->clk);
+ dev_err(dev, "failed to get clock: %d\n", err);
+ goto out;
+ }
+ }
+
+ /* Map base memory address. */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ data->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(data->base)) {
+ dev_err(&pdev->dev, "unable to map base address\n");
+ err = PTR_ERR(data->base);
+ goto out;
+ }
+
+ /* Map interrupt control base memory address. Not used on all chips. */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (res) {
+ data->int_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(data->int_base)) {
+ dev_err(&pdev->dev, "unable to map base address\n");
+ err = PTR_ERR(data->base);
+ goto out;
+ }
+ }
+
+ /* Get IRQ. */
+ irq = platform_get_irq(pdev, 0);
+ if (irq == 0) {
+ dev_err(&pdev->dev, "could not get IRQ\n");
+ err = -EIO;
+ goto out;
+ }
+
+ /* Initialize SPI controller. */
+ bcm_mspi_hw_init(data);
+
+ err = devm_request_irq(&pdev->dev, irq, bcm_mspi_isr,
+ IRQF_SHARED, "bcm-mspi", pdev);
+ if (err)
+ goto out;
+
+ err = devm_spi_register_master(dev, data->master);
+ if (err)
+ goto out;
+
+ dev_info(dev, "BCM MSPI initialized successfully\n");
+
+ return 0;
+
+out:
+ spi_master_put(data->master);
+ return err;
+}
+
+static struct platform_driver bcm_mspi_driver = {
+ .driver = {
+ .name = "brcm,mspi-v0",
+ .of_match_table = bcm_mspi_dt,
+ },
+ .probe = bcm_mspi_probe,
+};
+
+module_platform_driver(bcm_mspi_driver);
+
+MODULE_DESCRIPTION("Broadcom MSPI SPI Controller driver");
+MODULE_AUTHOR("Broadcom");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5
On 12/05/15 10:38, Jonathan Richardson wrote:
>
> Signed-off-by: Jonathan Richardson <[email protected]>
> ---
> .../devicetree/bindings/spi/brcm,mspi-spi.txt | 39 ++++++++++++++++++++
> 1 file changed, 39 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
>
> diff --git a/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
> new file mode 100644
> index 0000000..e86a7a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
> @@ -0,0 +1,39 @@
> +Broadcom MSPI controller
> +
> +The Broadcom MSPI controller is a SPI controller found on various chips such
> +as Cygnus.
> +
> +Required properties:
> +- compatible: Must be "brcm,mspi-v0".
> +
> +- reg: The first register is the physical base address of the MSPI controller.
> + The second register is the address of the MSPI interrupt control registers. It
> + is only required for chips that have a separate register set for extended
> + interrupt control. This is required for Cygnus.
In which case we would want to mandate the use of a "reg-names"
property, because there could be other kinds of controllers, such as the
one found on BCM7xxx where we have the 3 "reg" units, the first one is
the MSPI controller register range, while the two others are used for
the Boot SPI portion of the controller to accelerate flash reads, so
completely unrelated here.
Right now, using your driver on this Device Tree/configuration makes
bcm_mspi request the second register range and mistakenly assuming this
is the extended interrupt control register.
You could argue that the BSPI portion should be a separate node, but
even so, better be safe than sorry.
--
Florian
On 12/05/15 10:38, Jonathan Richardson wrote:
> The MSPI controller is a SPI controller found on various Broadcom
> SoC's such as Cygnus.
After adding an include/linux/io.h to fix the build error on the
implicit readl/write declaration, plus some additional tweaking, this
seems to detect a SPI flash correctly on BCM7xxx, however read transfers
don't return correct data, I think I have an idea why though.
Thanks!
--
Florian
On Tue, May 12, 2015 at 10:38:13AM -0700, Jonathan Richardson wrote:
> + /* Wait for interrupt indicating transfer is complete. */
> + if (!wait_for_completion_timeout(&mspi->xfer_done,
> + msecs_to_jiffies(10))) {
What if we have a large transfer on a slow bus?
> + while (bytes_processed < transfer->len) {
> + /* Start transfer and wait until complete. */
> + err = bcm_mspi_start_transfer(mspi, !last_slot, slot);
> + if (err)
> + return err;
This isn't really start_transfer() is it? It's doing the entire
operation.
> + /* Delay requested amount before next transfer. */
> + udelay(transfer->delay_usecs);
> + }
This is buggy, it's applying the per-transfer delay every timme we fill
the FIFO.
> + /* The rx data will go into RXRAM0/1 + last tx length. */
> + if (slot + 1 >= NUM_SLOTS)
> + mspi->rx_slot = 0;
> + else
> + mspi->rx_slot = slot + 1;
How is this going to work for full duplex transfers if we had to fill
the FIFO more than once?
> +static int bcm_mspi_transfer_one(struct spi_master *master,
> + struct spi_device *spidev, struct spi_transfer *transfer)
> +{
> + int err;
> +
> + /* 8 bit transfers only are currently supported. */
> + if (transfer->bits_per_word > 8)
> + return -ENOTSUPP;
Tell the core what the device supports and it will check for you.
> +
> + err = bcm_mspi_tx_data(master, spidev, transfer);
> + if (err)
> + return err;
> +
> + err = bcm_mspi_rx_data(master, spidev, transfer);
> + if (err)
> + return err;
> +
> + return 0;
> +}
I would expect the recieve and transmit operations to be running in
parallel, not executed one after another, given the need to keep
manually filling and draining the FIFOs.
> + struct spi_master *master;
> + struct device *dev = &pdev->dev;
> + int err;
> + struct resource *res;
> + unsigned int irq;
> +
> + dev_info(dev, "Initializing BCM MSPI\n");
Don't spam the logs like this, there's no content in this message.
> + master = spi_alloc_master(dev, sizeof(*data));
> + if (!master) {
devm_spi_alloc_master().
> + /* Map base memory address. */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(data->base)) {
> + dev_err(&pdev->dev, "unable to map base address\n");
devm_ioremap_resource() will complain for you.
On 15-05-12 12:17 PM, Mark Brown wrote:
> On Tue, May 12, 2015 at 10:38:13AM -0700, Jonathan Richardson wrote:
>
>> + /* Wait for interrupt indicating transfer is complete. */
>> + if (!wait_for_completion_timeout(&mspi->xfer_done,
>> + msecs_to_jiffies(10))) {
>
> What if we have a large transfer on a slow bus?
I guess it depends on the reference clock frequency (as well as spbr)
which we may not know because I made it optional. I could possibly
calculate it. I'll look into this. I used the slowest possible spbr
based on our ref clock frequency and tested large transfers which never
timed out.
>
>> + while (bytes_processed < transfer->len) {
>
>> + /* Start transfer and wait until complete. */
>> + err = bcm_mspi_start_transfer(mspi, !last_slot, slot);
>> + if (err)
>> + return err;
>
> This isn't really start_transfer() is it? It's doing the entire
> operation.
Yes, the entire operation.
>
>> + /* Delay requested amount before next transfer. */
>> + udelay(transfer->delay_usecs);
>> + }
>
> This is buggy, it's applying the per-transfer delay every timme we fill
> the FIFO.
Thanks, I'll fix.
>
>> + /* The rx data will go into RXRAM0/1 + last tx length. */
>> + if (slot + 1 >= NUM_SLOTS)
>> + mspi->rx_slot = 0;
>> + else
>> + mspi->rx_slot = slot + 1;
>
> How is this going to work for full duplex transfers if we had to fill
> the FIFO more than once?
See below, it's not.
>
>> +static int bcm_mspi_transfer_one(struct spi_master *master,
>> + struct spi_device *spidev, struct spi_transfer *transfer)
>> +{
>> + int err;
>> +
>> + /* 8 bit transfers only are currently supported. */
>> + if (transfer->bits_per_word > 8)
>> + return -ENOTSUPP;
>
> Tell the core what the device supports and it will check for you.
>
>> +
>> + err = bcm_mspi_tx_data(master, spidev, transfer);
>> + if (err)
>> + return err;
>> +
>> + err = bcm_mspi_rx_data(master, spidev, transfer);
>> + if (err)
>> + return err;
>> +
>> + return 0;
>> +}
>
> I would expect the recieve and transmit operations to be running in
> parallel, not executed one after another, given the need to keep
> manually filling and draining the FIFOs.
The driver was only written with NOR flash in mind as the slave. Since
this is really just half duplex, it works, though it won't with a real
full duplex slave. m25p80 never calls transfer_one with both tx and rx
buffers. The rx bytes that we don't care about from a tx request are
dropped.
We don't have any full duplex slaves so it's a hard to test. I could
possibly re-write this so that tx/rx is interleaved and that should be
good for full duplex or close enough that it won't have to be completely
overhauled should we ever connect a full duplex slave to it. Maybe I
should do that. This came from a pre-existing driver that had the same
limitation.
>
>> + struct spi_master *master;
>> + struct device *dev = &pdev->dev;
>> + int err;
>> + struct resource *res;
>> + unsigned int irq;
>> +
>> + dev_info(dev, "Initializing BCM MSPI\n");
>
> Don't spam the logs like this, there's no content in this message.
>
>> + master = spi_alloc_master(dev, sizeof(*data));
>> + if (!master) {
>
> devm_spi_alloc_master().
This doesn't exist but devm_spi_register_master() does which I used.
>
>> + /* Map base memory address. */
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + data->base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(data->base)) {
>> + dev_err(&pdev->dev, "unable to map base address\n");
>
> devm_ioremap_resource() will complain for you.
>
Thanks,
Jon
Hi Jon,
comment below for full-duplex operation.
On 15-05-13 04:49 PM, Jonathan Richardson wrote:
> On 15-05-12 12:17 PM, Mark Brown wrote:
>> On Tue, May 12, 2015 at 10:38:13AM -0700, Jonathan Richardson wrote:
>>
>>
>>> + /* The rx data will go into RXRAM0/1 + last tx length. */
>>> + if (slot + 1 >= NUM_SLOTS)
>>> + mspi->rx_slot = 0;
>>> + else
>>> + mspi->rx_slot = slot + 1;
>>
>> How is this going to work for full duplex transfers if we had to fill
>> the FIFO more than once?
>
> See below, it's not.
>
>>
>>> +static int bcm_mspi_transfer_one(struct spi_master *master,
>>> + struct spi_device *spidev, struct spi_transfer *transfer)
>>> +{
>>> + int err;
>>> +
>>> + /* 8 bit transfers only are currently supported. */
>>> + if (transfer->bits_per_word > 8)
>>> + return -ENOTSUPP;
>>
>> Tell the core what the device supports and it will check for you.
>>
>>> +
>>> + err = bcm_mspi_tx_data(master, spidev, transfer);
>>> + if (err)
>>> + return err;
>>> +
>>> + err = bcm_mspi_rx_data(master, spidev, transfer);
>>> + if (err)
>>> + return err;
>>> +
>>> + return 0;
>>> +}
>>
>> I would expect the recieve and transmit operations to be running in
>> parallel, not executed one after another, given the need to keep
>> manually filling and draining the FIFOs.
>
> The driver was only written with NOR flash in mind as the slave. Since
> this is really just half duplex, it works, though it won't with a real
> full duplex slave. m25p80 never calls transfer_one with both tx and rx
> buffers. The rx bytes that we don't care about from a tx request are
> dropped.
>
> We don't have any full duplex slaves so it's a hard to test. I could
> possibly re-write this so that tx/rx is interleaved and that should be
> good for full duplex or close enough that it won't have to be completely
> overhauled should we ever connect a full duplex slave to it. Maybe I
> should do that. This came from a pre-existing driver that had the same
> limitation.
>
>>
The purpose of this mspi interface is to connect to NOR flash. There
are other SPI interfaces on the devices used to connect to other SPI
devies. We don't have any need to support full duplex slaves on this
port (NOR have any hardware wired this way - pun realized after typing
this).
On Wed, May 13, 2015 at 05:19:06PM -0700, Scott Branden wrote:
> The purpose of this mspi interface is to connect to NOR flash. There are
> other SPI interfaces on the devices used to connect to other SPI devies. We
> don't have any need to support full duplex slaves on this port (NOR have any
> hardware wired this way - pun realized after typing this).
Chip vendors often say this sort of thing and then get surprised by what
their users choose to do, and even if it only ever gets used with flash
all it would take is some new flash command which can use full duplex
for something. Please write the code so it at least tries to handle
full duplex operation, if you can't test it fully that's not the end of
the world. It doesn't look like it should be particularly difficult.
Hi Mark,
On 15-05-14 03:31 AM, Mark Brown wrote:
> On Wed, May 13, 2015 at 05:19:06PM -0700, Scott Branden wrote:
>
>> The purpose of this mspi interface is to connect to NOR flash. There are
>> other SPI interfaces on the devices used to connect to other SPI devies. We
>> don't have any need to support full duplex slaves on this port (NOR have any
>> hardware wired this way - pun realized after typing this).
>
> Chip vendors often say this sort of thing and then get surprised by what
> their users choose to do, and even if it only ever gets used with flash
> all it would take is some new flash command which can use full duplex
> for something. Please write the code so it at least tries to handle
> full duplex operation, if you can't test it fully that's not the end of
> the world. It doesn't look like it should be particularly difficult.
>
Yes, there is always room for improvements in code. In this case - it
really is not worth our time to add code we can't test. We try to
deliver code that we can test and actually works. Yes, if anyone needs
to use the mspi for full duplex operation code can be added in the
future - it is software. This block has gone through many generations
of our SoCs and has only been used for this purpose - the bootROM boots
from this SPI only. It is dedicated for this purpose.
Also, there are other SPI blocks on the SoC that are used for connect
other SPI devices. These SPI blocks are properly designed for
full-duplex operation. This mspi block is designed for connecting to
NOR flash.
Regards,
Scott
On 14/05/15 11:19, Scott Branden wrote:
> Hi Mark,
>
> On 15-05-14 03:31 AM, Mark Brown wrote:
>> On Wed, May 13, 2015 at 05:19:06PM -0700, Scott Branden wrote:
>>
>>> The purpose of this mspi interface is to connect to NOR flash. There
>>> are
>>> other SPI interfaces on the devices used to connect to other SPI
>>> devies. We
>>> don't have any need to support full duplex slaves on this port (NOR
>>> have any
>>> hardware wired this way - pun realized after typing this).
>>
>> Chip vendors often say this sort of thing and then get surprised by what
>> their users choose to do, and even if it only ever gets used with flash
>> all it would take is some new flash command which can use full duplex
>> for something. Please write the code so it at least tries to handle
>> full duplex operation, if you can't test it fully that's not the end of
>> the world. It doesn't look like it should be particularly difficult.
>>
> Yes, there is always room for improvements in code. In this case - it
> really is not worth our time to add code we can't test. We try to
> deliver code that we can test and actually works. Yes, if anyone needs
> to use the mspi for full duplex operation code can be added in the
> future - it is software. This block has gone through many generations
> of our SoCs and has only been used for this purpose - the bootROM boots
> from this SPI only. It is dedicated for this purpose.
>
> Also, there are other SPI blocks on the SoC that are used for connect
> other SPI devices. These SPI blocks are properly designed for
> full-duplex operation. This mspi block is designed for connecting to
> NOR flash.
This is an implementation detail and architectural choice that is
specific to the Cygnus SoCs where a single MSPI is present, right?
This same MSPI block is instantiated multiple times in BCM7xxx chips to
interface to general purpose SPI slaves, ranging from Ethernet switches
to bluetooth chips etc... those will definitively want full-duplex to be
working.
I would greatly appreciate if some thought to supporting full duplex
transfers was given in this driver drop. If you cannot test that, then
maybe just flag the driver has been half-duplex only for now...
Thank you.
--
Florian
On 15-05-14 11:28 AM, Florian Fainelli wrote:
> On 14/05/15 11:19, Scott Branden wrote:
>> Hi Mark,
>>
>> On 15-05-14 03:31 AM, Mark Brown wrote:
>>> On Wed, May 13, 2015 at 05:19:06PM -0700, Scott Branden wrote:
>>>
>>>> The purpose of this mspi interface is to connect to NOR flash. There
>>>> are
>>>> other SPI interfaces on the devices used to connect to other SPI
>>>> devies. We
>>>> don't have any need to support full duplex slaves on this port (NOR
>>>> have any
>>>> hardware wired this way - pun realized after typing this).
>>>
>>> Chip vendors often say this sort of thing and then get surprised by what
>>> their users choose to do, and even if it only ever gets used with flash
>>> all it would take is some new flash command which can use full duplex
>>> for something. Please write the code so it at least tries to handle
>>> full duplex operation, if you can't test it fully that's not the end of
>>> the world. It doesn't look like it should be particularly difficult.
>>>
>> Yes, there is always room for improvements in code. In this case - it
>> really is not worth our time to add code we can't test. We try to
>> deliver code that we can test and actually works. Yes, if anyone needs
>> to use the mspi for full duplex operation code can be added in the
>> future - it is software. This block has gone through many generations
>> of our SoCs and has only been used for this purpose - the bootROM boots
>> from this SPI only. It is dedicated for this purpose.
>>
>> Also, there are other SPI blocks on the SoC that are used for connect
>> other SPI devices. These SPI blocks are properly designed for
>> full-duplex operation. This mspi block is designed for connecting to
>> NOR flash.
>
> This is an implementation detail and architectural choice that is
> specific to the Cygnus SoCs where a single MSPI is present, right?
>
> This same MSPI block is instantiated multiple times in BCM7xxx chips to
> interface to general purpose SPI slaves, ranging from Ethernet switches
> to bluetooth chips etc... those will definitively want full-duplex to be
> working.
>
> I would greatly appreciate if some thought to supporting full duplex
> transfers was given in this driver drop. If you cannot test that, then
> maybe just flag the driver has been half-duplex only for now...
>
> Thank you.
>
Hi Florian,
Yes, we can flag that - but we have no understanding of how the block
operates in this mode. It would be great if your team could add the
necessary full duplex support in a patch enhancement to the driver?
On 15-05-14 11:36 AM, Scott Branden wrote:
> On 15-05-14 11:28 AM, Florian Fainelli wrote:
>> On 14/05/15 11:19, Scott Branden wrote:
>>> Hi Mark,
>>>
>>> On 15-05-14 03:31 AM, Mark Brown wrote:
>>>> On Wed, May 13, 2015 at 05:19:06PM -0700, Scott Branden wrote:
>>>>
>>>>> The purpose of this mspi interface is to connect to NOR flash. There
>>>>> are
>>>>> other SPI interfaces on the devices used to connect to other SPI
>>>>> devies. We
>>>>> don't have any need to support full duplex slaves on this port (NOR
>>>>> have any
>>>>> hardware wired this way - pun realized after typing this).
>>>>
>>>> Chip vendors often say this sort of thing and then get surprised by
>>>> what
>>>> their users choose to do, and even if it only ever gets used with flash
>>>> all it would take is some new flash command which can use full duplex
>>>> for something. Please write the code so it at least tries to handle
>>>> full duplex operation, if you can't test it fully that's not the end of
>>>> the world. It doesn't look like it should be particularly difficult.
>>>>
>>> Yes, there is always room for improvements in code. In this case - it
>>> really is not worth our time to add code we can't test. We try to
>>> deliver code that we can test and actually works. Yes, if anyone needs
>>> to use the mspi for full duplex operation code can be added in the
>>> future - it is software. This block has gone through many generations
>>> of our SoCs and has only been used for this purpose - the bootROM boots
>>> from this SPI only. It is dedicated for this purpose.
>>>
>>> Also, there are other SPI blocks on the SoC that are used for connect
>>> other SPI devices. These SPI blocks are properly designed for
>>> full-duplex operation. This mspi block is designed for connecting to
>>> NOR flash.
>>
>> This is an implementation detail and architectural choice that is
>> specific to the Cygnus SoCs where a single MSPI is present, right?
>>
>> This same MSPI block is instantiated multiple times in BCM7xxx chips to
>> interface to general purpose SPI slaves, ranging from Ethernet switches
>> to bluetooth chips etc... those will definitively want full-duplex to be
>> working.
>>
>> I would greatly appreciate if some thought to supporting full duplex
>> transfers was given in this driver drop. If you cannot test that, then
>> maybe just flag the driver has been half-duplex only for now...
>>
>> Thank you.
>>
> Hi Florian,
> Yes, we can flag that - but we have no understanding of how the block
> operates in this mode. It would be great if your team could add the
> necessary full duplex support in a patch enhancement to the driver?
>
Going blind on this one isn't the way to go. If I had a real full duplex
slave I could experiment with it to get it working. But with the docs I
have on this and considering how finicky the block is I think we're
wasting our time.
Jon
On Thu, May 14, 2015 at 11:19:01AM -0700, Scott Branden wrote:
> On 15-05-14 03:31 AM, Mark Brown wrote:
> >Chip vendors often say this sort of thing and then get surprised by what
> >their users choose to do, and even if it only ever gets used with flash
> >all it would take is some new flash command which can use full duplex
> >for something. Please write the code so it at least tries to handle
> >full duplex operation, if you can't test it fully that's not the end of
> >the world. It doesn't look like it should be particularly difficult.
> Yes, there is always room for improvements in code. In this case - it
> really is not worth our time to add code we can't test. We try to deliver
> code that we can test and actually works. Yes, if anyone needs to use the
While I try to not apply code that has obvious problems with silent data
corruption in it which is what we have just now.
> mspi for full duplex operation code can be added in the future - it is
> software. This block has gone through many generations of our SoCs and has
> only been used for this purpose - the bootROM boots from this SPI only. It
> is dedicated for this purpose.
All it takes is one hardware engineer who sees a SPI controller and a
GPIO they can use for chip select; I wouldn't be so sure that nobody
ever fixed this up locally (or happened to use a device that only needed
single duplex).
On 15-05-14 12:08 PM, Mark Brown wrote:
> On Thu, May 14, 2015 at 11:19:01AM -0700, Scott Branden wrote:
>> On 15-05-14 03:31 AM, Mark Brown wrote:
>
>>> Chip vendors often say this sort of thing and then get surprised by what
>>> their users choose to do, and even if it only ever gets used with flash
>>> all it would take is some new flash command which can use full duplex
>>> for something. Please write the code so it at least tries to handle
>>> full duplex operation, if you can't test it fully that's not the end of
>>> the world. It doesn't look like it should be particularly difficult.
>
>> Yes, there is always room for improvements in code. In this case - it
>> really is not worth our time to add code we can't test. We try to deliver
>> code that we can test and actually works. Yes, if anyone needs to use the
>
> While I try to not apply code that has obvious problems with silent data
> corruption in it which is what we have just now.
>
>> mspi for full duplex operation code can be added in the future - it is
>> software. This block has gone through many generations of our SoCs and has
>> only been used for this purpose - the bootROM boots from this SPI only. It
>> is dedicated for this purpose.
>
> All it takes is one hardware engineer who sees a SPI controller and a
> GPIO they can use for chip select; I wouldn't be so sure that nobody
> ever fixed this up locally (or happened to use a device that only needed
> single duplex).
>
Hi Mark. Would it help if we just set the flags to half duplex using
SPI_MASTER_HALF_DUPLEX when we register the master?
On Thu, May 14, 2015 at 12:55:32PM -0700, Jonathan Richardson wrote:
> On 15-05-14 12:08 PM, Mark Brown wrote:
> > All it takes is one hardware engineer who sees a SPI controller and a
> > GPIO they can use for chip select; I wouldn't be so sure that nobody
> > ever fixed this up locally (or happened to use a device that only needed
> > single duplex).
> Hi Mark. Would it help if we just set the flags to half duplex using
> SPI_MASTER_HALF_DUPLEX when we register the master?
That would deal with the data corruption issues, yes. Though it seems
like there are some people who want full duplex support here - I don't
know if any of them can point you at suitable hardware to test?
On 14/05/15 13:12, Mark Brown wrote:
> On Thu, May 14, 2015 at 12:55:32PM -0700, Jonathan Richardson wrote:
>> On 15-05-14 12:08 PM, Mark Brown wrote:
>
>>> All it takes is one hardware engineer who sees a SPI controller and a
>>> GPIO they can use for chip select; I wouldn't be so sure that nobody
>>> ever fixed this up locally (or happened to use a device that only needed
>>> single duplex).
>
>> Hi Mark. Would it help if we just set the flags to half duplex using
>> SPI_MASTER_HALF_DUPLEX when we register the master?
>
> That would deal with the data corruption issues, yes. Though it seems
> like there are some people who want full duplex support here - I don't
> know if any of them can point you at suitable hardware to test?
I am fine with having the driver supporting only half-duplex for now,
considering that Jonathan and Scott have no peripheral to test
full-duplex with.
Getting you platforms and test setups could take more time than needed
so, let's proceed with the shortest path.
--
Florian