[PATCH 0/4] Add MSPI support for Cygnus

[PATCH 0/4] Add MSPI support for Cygnus

[Jonathan Richardson]

This patchset adds support for the MSPI controller on Cygnus. The existing MSPI
driver in the kernel was written for BCMA which is a Broadcom AMBA bus variant
used on certain chips such as the 53xx.

This patch makes BCMA support optional. The current config is being renamed to
make it chip nonspecific supporting BCMA, and a new config is added to support
non-BCMA chips. DT support is now mandatory to allow removal of a hardcoded SPI
device.

Support is also added to set the baud rate. The controller currently runs at the
slowest speed possible.

Jonathan Richardson (4):
  ARM: dts: Add binding for Broadcom MSPI driver.
  spi: bcm53xx: Refactor to make driver nonspecific to 53xx SoCs
  spi: bcm-mspi: Make BCMA optional to support non-BCMA chips
  spi: bcm-mspi: Add support to set serial baud clock rate

 .../devicetree/bindings/spi/brcm,mspi-spi.txt      |   38 ++
 drivers/spi/Kconfig                                |   12 +-
 drivers/spi/Makefile                               |    3 +-
 drivers/spi/spi-bcm-mspi.c                         |  453 ++++++++++++++++++++
 drivers/spi/spi-bcm-mspi.h                         |   84 ++++
 drivers/spi/spi-bcm53xx.c                          |  299 -------------
 drivers/spi/spi-bcm53xx.h                          |   72 ----
 7 files changed, 585 insertions(+), 376 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
 create mode 100644 drivers/spi/spi-bcm-mspi.c
 create mode 100644 drivers/spi/spi-bcm-mspi.h
 delete mode 100644 drivers/spi/spi-bcm53xx.c
 delete mode 100644 drivers/spi/spi-bcm53xx.h

-- 
1.7.9.5

[PATCH 1/4] ARM: dts: Add binding for Broadcom MSPI driver.

[Jonathan Richardson]

Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 .../devicetree/bindings/spi/brcm,mspi-spi.txt      |   38 ++++++++++++++++++++
 1 file changed, 38 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..16164e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
@@ -0,0 +1,38 @@
+Broadcom MSPI controller
+
+Required properties:
+- compatible: Must be either "brcm,mspi" or "brcm,bcma-mspi". Use
+  "brcm,bcma-mspi" for controllers on a bcma bus and "brcm,mspi" otherwise.
+
+- reg:  Physical base address and length of the controller's registers.
+
+- interrupts: The interrupt id for the controller.
+
+- #address-cells: should be 1.
+
+- #size-cells: should be 0.
+
+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@18047000 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "brcm,mspi";
+	reg = <0x18047000 0x1000>;
+	clocks = <&axi41_clk>;
+	clock-names = "mspi_clk";
+	clock-frequency = <12500000>;
+};
-- 
1.7.9.5

Re: [PATCH 1/4] ARM: dts: Add binding for Broadcom MSPI driver.

[Florian Fainelli]

Le 02/04/2015 12:23, Jonathan Richardson a écrit :
> 
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---
>  .../devicetree/bindings/spi/brcm,mspi-spi.txt      |   38 ++++++++++++++++++++
>  1 file changed, 38 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..16164e3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
> @@ -0,0 +1,38 @@
> +Broadcom MSPI controller
> +
> +Required properties:
> +- compatible: Must be either "brcm,mspi" or "brcm,bcma-mspi". Use
> +  "brcm,bcma-mspi" for controllers on a bcma bus and "brcm,mspi" otherwise.

We need a more specific compatible property here since there are at
least 3 known SoCs families within Broadcom (Cygnus, BCM53xx, BCM7xxx)
that use this controller, also older versions of the core did not have a
revision register, yet they had an internal version numbering that we
might want to reflect here. This does not need to be fixed immediately
though, we can add compatible strings as we start adding support for
older cores.

> +
> +- reg:  Physical base address and length of the controller's registers.
> +
> +- interrupts: The interrupt id for the controller.

I think this should be two cells, on BCM7xxx chips there is a MSPI_DONE
and a MSPI_ERROR interrupt bit, we typically only use the first one, but
since we are describing the hardware here, we need to be exhaustive.

> +
> +- #address-cells: should be 1.
> +
> +- #size-cells: should be 0.
> +
> +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.

See my reply to the patch 4, that does not seem to match the
"clock-frequency" vs. clock phandles practices in DT.

> +
> +Example:
> +
> +mspi@18047000 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "brcm,mspi";
> +	reg = <0x18047000 0x1000>;
> +	clocks = <&axi41_clk>;
> +	clock-names = "mspi_clk";
> +	clock-frequency = <12500000>;

Since "interrupts" is a mandatory property you might want the example to
show it for consistency.
--
Florian

Re: [PATCH 1/4] ARM: dts: Add binding for Broadcom MSPI driver.

[Jonathan Richardson]

On 15-04-04 12:17 PM, Florian Fainelli wrote:
> Le 02/04/2015 12:23, Jonathan Richardson a écrit :
>>
>> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
>> ---
>>  .../devicetree/bindings/spi/brcm,mspi-spi.txt      |   38 ++++++++++++++++++++
>>  1 file changed, 38 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..16164e3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/brcm,mspi-spi.txt
>> @@ -0,0 +1,38 @@
>> +Broadcom MSPI controller
>> +
>> +Required properties:
>> +- compatible: Must be either "brcm,mspi" or "brcm,bcma-mspi". Use
>> +  "brcm,bcma-mspi" for controllers on a bcma bus and "brcm,mspi" otherwise.
> 
> We need a more specific compatible property here since there are at
> least 3 known SoCs families within Broadcom (Cygnus, BCM53xx, BCM7xxx)
> that use this controller, also older versions of the core did not have a
> revision register, yet they had an internal version numbering that we
> might want to reflect here. This does not need to be fixed immediately
> though, we can add compatible strings as we start adding support for
> older cores.
> 
>> +
>> +- reg:  Physical base address and length of the controller's registers.
>> +
>> +- interrupts: The interrupt id for the controller.
> 
> I think this should be two cells, on BCM7xxx chips there is a MSPI_DONE
> and a MSPI_ERROR interrupt bit, we typically only use the first one, but
> since we are describing the hardware here, we need to be exhaustive.

My mistake. Interrupts are not supported by this driver yet. I intend on
adding this later. I will remove this from the docs.

> 
>> +
>> +- #address-cells: should be 1.
>> +
>> +- #size-cells: should be 0.
>> +
>> +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.
> 
> See my reply to the patch 4, that does not seem to match the
> "clock-frequency" vs. clock phandles practices in DT.

I could use a different property for it. I agree it's a bit weird since
we're not changing the frequency of the reference clock but the clock
derived from it for the baud rate of the controller. Does this warrant
adding a different property though?

> 
>> +
>> +Example:
>> +
>> +mspi@18047000 {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	compatible = "brcm,mspi";
>> +	reg = <0x18047000 0x1000>;
>> +	clocks = <&axi41_clk>;
>> +	clock-names = "mspi_clk";
>> +	clock-frequency = <12500000>;
> 
> Since "interrupts" is a mandatory property you might want the example to
> show it for consistency.
> --
> Florian
> 

[PATCH 2/4] spi: bcm53xx: Refactor to make driver nonspecific to 53xx SoCs

[Jonathan Richardson]

The Broadcom MSPI controller is used on various SoCs. It is being
renamed so that it can be extended and reused on other chips. It is
renamed to bcm-mspi.

Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/spi/Kconfig        |    7 +-
 drivers/spi/Makefile       |    2 +-
 drivers/spi/spi-bcm-mspi.c |  307 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi-bcm-mspi.h |   84 ++++++++++++
 drivers/spi/spi-bcm53xx.c  |  299 ------------------------------------------
 drivers/spi/spi-bcm53xx.h  |   72 -----------
 6 files changed, 395 insertions(+), 376 deletions(-)
 create mode 100644 drivers/spi/spi-bcm-mspi.c
 create mode 100644 drivers/spi/spi-bcm-mspi.h
 delete mode 100644 drivers/spi/spi-bcm53xx.c
 delete mode 100644 drivers/spi/spi-bcm53xx.h

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ab8dfbe..766e08d 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -113,13 +113,12 @@ config SPI_AU1550
 	  If you say yes to this option, support will be included for the
 	  PSC SPI controller found on Au1550, Au1200 and Au1300 series.
 
-config SPI_BCM53XX
-	tristate "Broadcom BCM53xx SPI controller"
-	depends on ARCH_BCM_5301X
+config SPI_BCMA_MSPI
+	tristate "Broadcom BCMA MSPI controller"
 	depends on BCMA_POSSIBLE
 	select BCMA
 	help
-          Enable support for the SPI controller on Broadcom BCM53xx ARM SoCs.
+	  Enable support for the Broadcom BCMA MSPI controller.
 
 config SPI_BCM63XX
 	tristate "Broadcom BCM63xx SPI controller"
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d8cbf65..6b58100 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -15,7 +15,7 @@ obj-$(CONFIG_SPI_ATMEL)			+= spi-atmel.o
 obj-$(CONFIG_SPI_ATH79)			+= spi-ath79.o
 obj-$(CONFIG_SPI_AU1550)		+= spi-au1550.o
 obj-$(CONFIG_SPI_BCM2835)		+= spi-bcm2835.o
-obj-$(CONFIG_SPI_BCM53XX)		+= spi-bcm53xx.o
+obj-$(CONFIG_SPI_BCMA_MSPI)		+= spi-bcm-mspi.o
 obj-$(CONFIG_SPI_BCM63XX)		+= spi-bcm63xx.o
 obj-$(CONFIG_SPI_BCM63XX_HSSPI)		+= spi-bcm63xx-hsspi.o
 obj-$(CONFIG_SPI_BFIN5XX)		+= spi-bfin5xx.o
diff --git a/drivers/spi/spi-bcm-mspi.c b/drivers/spi/spi-bcm-mspi.c
new file mode 100644
index 0000000..67ea246
--- /dev/null
+++ b/drivers/spi/spi-bcm-mspi.c
@@ -0,0 +1,307 @@
+/*
+ * Portions 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/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/bcma/bcma.h>
+#include <linux/spi/spi.h>
+
+#include "spi-bcm-mspi.h"
+
+#define BCM_MSPI_MAX_SPI_BAUD   13500000	/* 216 MHz? */
+
+/* The longest observed required wait was 19 ms */
+#define BCM_MSPI_SPE_TIMEOUT_MS 80
+
+struct bcm_mspi {
+	struct bcma_device *core;
+	struct spi_master *master;
+
+	size_t read_offset;
+};
+
+static inline u32 bcm_mspi_read(struct bcm_mspi *mspi, u16 offset)
+{
+	return bcma_read32(mspi->core, offset);
+}
+
+static inline void bcm_mspi_write(struct bcm_mspi *mspi, u16 offset,
+				    u32 value)
+{
+	bcma_write32(mspi->core, offset, value);
+}
+
+static inline unsigned int bcm_mspi_calc_timeout(size_t len)
+{
+	/* Do some magic calculation based on length and buad. Add 10% and 1. */
+	return (len * 9000 / BCM_MSPI_MAX_SPI_BAUD * 110 / 100) + 1;
+}
+
+static int bcm_mspi_wait(struct bcm_mspi *mspi, unsigned int timeout_ms)
+{
+	unsigned long deadline;
+	u32 tmp;
+
+	/* SPE bit has to be 0 before we read MSPI STATUS */
+	deadline = jiffies + BCM_MSPI_SPE_TIMEOUT_MS * HZ / 1000;
+	do {
+		tmp = bcm_mspi_read(mspi, MSPI_SPCR2);
+		if (!(tmp & MSPI_SPCR2_SPE))
+			break;
+		udelay(5);
+	} while (!time_after_eq(jiffies, deadline));
+
+	if (tmp & MSPI_SPCR2_SPE)
+		goto spi_timeout;
+
+	/* Check status */
+	deadline = jiffies + timeout_ms * HZ / 1000;
+	do {
+		tmp = bcm_mspi_read(mspi, MSPI_MSPI_STATUS);
+		if (tmp & MSPI_MSPI_STATUS_SPIF) {
+			bcm_mspi_write(mspi, MSPI_MSPI_STATUS, 0);
+			return 0;
+		}
+
+		cpu_relax();
+		udelay(100);
+	} while (!time_after_eq(jiffies, deadline));
+
+spi_timeout:
+	bcm_mspi_write(mspi, MSPI_MSPI_STATUS, 0);
+
+	pr_err("Timeout waiting for SPI to be ready!\n");
+
+	return -EBUSY;
+}
+
+static void bcm_mspi_buf_write(struct bcm_mspi *mspi, u8 *w_buf,
+				 size_t len, bool cont)
+{
+	u32 tmp;
+	int i;
+
+	for (i = 0; i < len; i++) {
+		/* Transmit Register File MSB */
+		bcm_mspi_write(mspi, MSPI_TXRAM + 4 * (i * 2),
+				 (unsigned int)w_buf[i]);
+	}
+
+	for (i = 0; i < len; i++) {
+		tmp = CDRAM_CONT | CDRAM_PCS_DISABLE_ALL | CDRAM_PCS_DSCK;
+		if (!cont && i == len - 1)
+			tmp &= ~CDRAM_CONT;
+		tmp &= ~0x1;
+		/* Command Register File */
+		bcm_mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
+	}
+
+	/* Set queue pointers */
+	bcm_mspi_write(mspi, MSPI_NEWQP, 0);
+	bcm_mspi_write(mspi, MSPI_ENDQP, len - 1);
+
+	if (cont)
+		bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 1);
+
+	/* Start SPI transfer */
+	tmp = bcm_mspi_read(mspi, MSPI_SPCR2);
+	tmp |= MSPI_SPCR2_SPE;
+	if (cont)
+		tmp |= MSPI_SPCR2_CONT_AFTER_CMD;
+	bcm_mspi_write(mspi, MSPI_SPCR2, tmp);
+
+	/* Wait for SPI to finish */
+	bcm_mspi_wait(mspi, bcm_mspi_calc_timeout(len));
+
+	if (!cont)
+		bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 0);
+
+	mspi->read_offset = len;
+}
+
+static void bcm_mspi_buf_read(struct bcm_mspi *mspi, u8 *r_buf,
+				size_t len, bool cont)
+{
+	u32 tmp;
+	int i;
+
+	for (i = 0; i < mspi->read_offset + len; i++) {
+		tmp = CDRAM_CONT | CDRAM_PCS_DISABLE_ALL |
+		      CDRAM_PCS_DSCK;
+		if (!cont && i == mspi->read_offset + len - 1)
+			tmp &= ~CDRAM_CONT;
+		tmp &= ~0x1;
+		/* Command Register File */
+		bcm_mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
+	}
+
+	/* Set queue pointers */
+	bcm_mspi_write(mspi, MSPI_NEWQP, 0);
+	bcm_mspi_write(mspi, MSPI_ENDQP,
+			 mspi->read_offset + len - 1);
+
+	if (cont)
+		bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 1);
+
+	/* Start SPI transfer */
+	tmp = bcm_mspi_read(mspi, MSPI_SPCR2);
+	tmp |= MSPI_SPCR2_SPE;
+	if (cont)
+		tmp |= MSPI_SPCR2_CONT_AFTER_CMD;
+	bcm_mspi_write(mspi, MSPI_SPCR2, tmp);
+
+	/* Wait for SPI to finish */
+	bcm_mspi_wait(mspi, bcm_mspi_calc_timeout(len));
+
+	if (!cont)
+		bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 0);
+
+	for (i = 0; i < len; ++i) {
+		int offset = mspi->read_offset + i;
+
+		/* Data stored in the transmit register file LSB */
+		r_buf[i] = (u8)bcm_mspi_read(mspi,
+			MSPI_RXRAM + 4 * (1 + offset * 2));
+	}
+
+	mspi->read_offset = 0;
+}
+
+static int bcm_mspi_transfer_one(struct spi_master *master,
+				   struct spi_device *spi,
+				   struct spi_transfer *t)
+{
+	struct bcm_mspi *mspi = spi_master_get_devdata(master);
+	u8 *buf;
+	size_t left;
+
+	if (t->tx_buf) {
+		buf = (u8 *)t->tx_buf;
+		left = t->len;
+		while (left) {
+			size_t to_write = min_t(size_t, 16, left);
+			bool cont = left - to_write > 0;
+
+			bcm_mspi_buf_write(mspi, buf, to_write, cont);
+			left -= to_write;
+			buf += to_write;
+		}
+	}
+
+	if (t->rx_buf) {
+		buf = (u8 *)t->rx_buf;
+		left = t->len;
+		while (left) {
+			size_t to_read = min_t(size_t, 16 - mspi->read_offset,
+					       left);
+			bool cont = left - to_read > 0;
+
+			bcm_mspi_buf_read(mspi, buf, to_read, cont);
+			left -= to_read;
+			buf += to_read;
+		}
+	}
+
+	return 0;
+}
+
+static struct spi_board_info bcm53xx_info = {
+	.modalias	= "bcm53xxspiflash",
+};
+
+static const struct bcma_device_id bcm_mspi_bcma_tbl[] = {
+	BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_NS_QSPI, BCMA_ANY_REV,
+		BCMA_ANY_CLASS),
+	{},
+};
+MODULE_DEVICE_TABLE(bcma, bcm_mspi_bcma_tbl);
+
+static inline u32 bcm_bcma_mspi_read(struct bcm_mspi *mspi, u16 offset)
+{
+	return bcma_read32(mspi->core, offset);
+}
+
+static inline void bcm_bcma_mspi_write(struct bcm_mspi *mspi, u16 offset,
+	u32 value)
+{
+	bcma_write32(mspi->core, offset, value);
+}
+
+static int bcm_mspi_bcma_probe(struct bcma_device *core)
+{
+	struct bcm_mspi *data;
+	struct spi_master *master;
+	int err;
+
+	dev_info(&core->dev, "BCM MSPI BCMA probe\n");
+
+	if (core->bus->drv_cc.core->id.rev != 42) {
+		pr_err("SPI on SoC with unsupported ChipCommon rev\n");
+		return -ENOTSUPP;
+	}
+
+	master = spi_alloc_master(&core->dev, sizeof(*data));
+	if (!master)
+		return -ENOMEM;
+
+	data = spi_master_get_devdata(master);
+	data->master = master;
+	data->core = core;
+
+	master->transfer_one = bcm_mspi_transfer_one;
+	bcma_set_drvdata(core, data);
+
+	err = devm_spi_register_master(&core->dev, data->master);
+	if (err) {
+		spi_master_put(master);
+		bcma_set_drvdata(core, NULL);
+		goto out;
+	}
+
+	/* Broadcom SoCs (at least with the CC rev 42) use SPI for flash only */
+	spi_new_device(master, &bcm53xx_info);
+
+out:
+	return err;
+}
+
+static struct bcma_driver bcm_mspi_bcma_driver = {
+	.name		= KBUILD_MODNAME,
+	.id_table	= bcm_mspi_bcma_tbl,
+	.probe		= bcm_mspi_bcma_probe,
+};
+
+static int __init bcm_mspi_bcma_module_init(void)
+{
+	int err = 0;
+
+	err = bcma_driver_register(&bcm_mspi_bcma_driver);
+	if (err)
+		pr_err("Failed to register bcma driver: %d\n", err);
+
+	return err;
+}
+
+static void __exit bcm_mspi_bcma_module_exit(void)
+{
+	bcma_driver_unregister(&bcm_mspi_bcma_driver);
+}
+
+module_init(bcm_mspi_bcma_module_init);
+module_exit(bcm_mspi_bcma_module_exit);
+
+MODULE_DESCRIPTION("Broadcom MSPI SPI Controller driver");
+MODULE_AUTHOR("Rafał Miłecki <zajec5@gmail.com>");
+MODULE_AUTHOR("Broadcom");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/spi/spi-bcm-mspi.h b/drivers/spi/spi-bcm-mspi.h
new file mode 100644
index 0000000..d633d5b
--- /dev/null
+++ b/drivers/spi/spi-bcm-mspi.h
@@ -0,0 +1,84 @@
+/*
+ * Portions 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.
+ */
+#ifndef SPI_BCM_MSPI_H
+#define SPI_BCM_MSPI_H
+
+#define BSPI_REVISION_ID                0x000
+#define BSPI_SCRATCH                    0x004
+#define BSPI_MAST_N_BOOT_CTRL           0x008
+#define BSPI_BUSY_STATUS                0x00c
+#define BSPI_INTR_STATUS                0x010
+#define BSPI_B0_STATUS                  0x014
+#define BSPI_B0_CTRL                    0x018
+#define BSPI_B1_STATUS                  0x01c
+#define BSPI_B1_CTRL                    0x020
+#define BSPI_STRAP_OVERRIDE_CTRL        0x024
+#define BSPI_FLEX_MODE_ENABLE           0x028
+#define BSPI_BITS_PER_CYCLE             0x02c
+#define BSPI_BITS_PER_PHASE             0x030
+#define BSPI_CMD_AND_MODE_BYTE          0x034
+#define BSPI_BSPI_FLASH_UPPER_ADDR_BYTE 0x038
+#define BSPI_BSPI_XOR_VALUE             0x03c
+#define BSPI_BSPI_XOR_ENABLE            0x040
+#define BSPI_BSPI_PIO_MODE_ENABLE       0x044
+#define BSPI_BSPI_PIO_IODIR             0x048
+#define BSPI_BSPI_PIO_DATA              0x04c
+
+/* RAF */
+#define RAF_START_ADDR                  0x100
+#define RAF_NUM_WORDS                   0x104
+#define RAF_CTRL                        0x108
+#define RAF_FULLNESS                    0x10c
+#define RAF_WATERMARK                   0x110
+#define RAF_STATUS                      0x114
+#define RAF_READ_DATA                   0x118
+#define RAF_WORD_CNT                    0x11c
+#define RAF_CURR_ADDR                   0x120
+
+/* MSPI */
+#define MSPI_SPCR0_LSB                  0x200
+#define MSPI_SPCR0_MSB                  0x204
+#define MSPI_SPCR1_LSB                  0x208
+#define MSPI_SPCR1_MSB                  0x20c
+#define MSPI_NEWQP                      0x210
+#define MSPI_ENDQP                      0x214
+#define MSPI_SPCR2                      0x218
+#define MSPI_SPCR2_SPE                  0x00000040
+#define MSPI_SPCR2_CONT_AFTER_CMD       0x00000080
+#define MSPI_MSPI_STATUS                0x220
+#define MSPI_MSPI_STATUS_SPIF           0x00000001
+#define MSPI_CPTQP                      0x224
+#define MSPI_TXRAM                      0x240 /* 32 registers, up to 0x2b8 */
+#define MSPI_RXRAM                      0x2c0 /* 32 registers, up to 0x33c */
+#define MSPI_CDRAM                      0x340 /* 16 registers, up to 0x37c */
+#define CDRAM_PCS_PCS0                  0x00000001
+#define CDRAM_PCS_PCS1                  0x00000002
+#define CDRAM_PCS_PCS2                  0x00000004
+#define CDRAM_PCS_PCS3                  0x00000008
+#define CDRAM_PCS_DISABLE_ALL           0x0000000f
+#define CDRAM_PCS_DSCK                  0x00000010
+#define CDRAM_BITSE                     0x00000040
+#define CDRAM_CONT                      0x00000080
+#define MSPI_WRITE_LOCK                 0x380
+#define MSPI_DISABLE_FLUSH_GEN          0x384
+
+/* Interrupt */
+#define INTR_RAF_LR_FULLNESS_REACHED    0x3a0
+#define INTR_RAF_LR_TRUNCATED           0x3a4
+#define INTR_RAF_LR_IMPATIENT           0x3a8
+#define INTR_RAF_LR_SESSION_DONE        0x3ac
+#define INTR_RAF_LR_OVERREAD            0x3b0
+#define INTR_MSPI_DONE                  0x3b4
+#define INTR_MSPI_HALT_SET_TRANSACTION_DONE  0x3b8
+
+#endif
diff --git a/drivers/spi/spi-bcm53xx.c b/drivers/spi/spi-bcm53xx.c
deleted file mode 100644
index 3fb91c8..0000000
--- a/drivers/spi/spi-bcm53xx.c
+++ /dev/null
@@ -1,299 +0,0 @@
-#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/slab.h>
-#include <linux/delay.h>
-#include <linux/bcma/bcma.h>
-#include <linux/spi/spi.h>
-
-#include "spi-bcm53xx.h"
-
-#define BCM53XXSPI_MAX_SPI_BAUD	13500000	/* 216 MHz? */
-
-/* The longest observed required wait was 19 ms */
-#define BCM53XXSPI_SPE_TIMEOUT_MS	80
-
-struct bcm53xxspi {
-	struct bcma_device *core;
-	struct spi_master *master;
-
-	size_t read_offset;
-};
-
-static inline u32 bcm53xxspi_read(struct bcm53xxspi *b53spi, u16 offset)
-{
-	return bcma_read32(b53spi->core, offset);
-}
-
-static inline void bcm53xxspi_write(struct bcm53xxspi *b53spi, u16 offset,
-				    u32 value)
-{
-	bcma_write32(b53spi->core, offset, value);
-}
-
-static inline unsigned int bcm53xxspi_calc_timeout(size_t len)
-{
-	/* Do some magic calculation based on length and buad. Add 10% and 1. */
-	return (len * 9000 / BCM53XXSPI_MAX_SPI_BAUD * 110 / 100) + 1;
-}
-
-static int bcm53xxspi_wait(struct bcm53xxspi *b53spi, unsigned int timeout_ms)
-{
-	unsigned long deadline;
-	u32 tmp;
-
-	/* SPE bit has to be 0 before we read MSPI STATUS */
-	deadline = jiffies + BCM53XXSPI_SPE_TIMEOUT_MS * HZ / 1000;
-	do {
-		tmp = bcm53xxspi_read(b53spi, B53SPI_MSPI_SPCR2);
-		if (!(tmp & B53SPI_MSPI_SPCR2_SPE))
-			break;
-		udelay(5);
-	} while (!time_after_eq(jiffies, deadline));
-
-	if (tmp & B53SPI_MSPI_SPCR2_SPE)
-		goto spi_timeout;
-
-	/* Check status */
-	deadline = jiffies + timeout_ms * HZ / 1000;
-	do {
-		tmp = bcm53xxspi_read(b53spi, B53SPI_MSPI_MSPI_STATUS);
-		if (tmp & B53SPI_MSPI_MSPI_STATUS_SPIF) {
-			bcm53xxspi_write(b53spi, B53SPI_MSPI_MSPI_STATUS, 0);
-			return 0;
-		}
-
-		cpu_relax();
-		udelay(100);
-	} while (!time_after_eq(jiffies, deadline));
-
-spi_timeout:
-	bcm53xxspi_write(b53spi, B53SPI_MSPI_MSPI_STATUS, 0);
-
-	pr_err("Timeout waiting for SPI to be ready!\n");
-
-	return -EBUSY;
-}
-
-static void bcm53xxspi_buf_write(struct bcm53xxspi *b53spi, u8 *w_buf,
-				 size_t len, bool cont)
-{
-	u32 tmp;
-	int i;
-
-	for (i = 0; i < len; i++) {
-		/* Transmit Register File MSB */
-		bcm53xxspi_write(b53spi, B53SPI_MSPI_TXRAM + 4 * (i * 2),
-				 (unsigned int)w_buf[i]);
-	}
-
-	for (i = 0; i < len; i++) {
-		tmp = B53SPI_CDRAM_CONT | B53SPI_CDRAM_PCS_DISABLE_ALL |
-		      B53SPI_CDRAM_PCS_DSCK;
-		if (!cont && i == len - 1)
-			tmp &= ~B53SPI_CDRAM_CONT;
-		tmp &= ~0x1;
-		/* Command Register File */
-		bcm53xxspi_write(b53spi, B53SPI_MSPI_CDRAM + 4 * i, tmp);
-	}
-
-	/* Set queue pointers */
-	bcm53xxspi_write(b53spi, B53SPI_MSPI_NEWQP, 0);
-	bcm53xxspi_write(b53spi, B53SPI_MSPI_ENDQP, len - 1);
-
-	if (cont)
-		bcm53xxspi_write(b53spi, B53SPI_MSPI_WRITE_LOCK, 1);
-
-	/* Start SPI transfer */
-	tmp = bcm53xxspi_read(b53spi, B53SPI_MSPI_SPCR2);
-	tmp |= B53SPI_MSPI_SPCR2_SPE;
-	if (cont)
-		tmp |= B53SPI_MSPI_SPCR2_CONT_AFTER_CMD;
-	bcm53xxspi_write(b53spi, B53SPI_MSPI_SPCR2, tmp);
-
-	/* Wait for SPI to finish */
-	bcm53xxspi_wait(b53spi, bcm53xxspi_calc_timeout(len));
-
-	if (!cont)
-		bcm53xxspi_write(b53spi, B53SPI_MSPI_WRITE_LOCK, 0);
-
-	b53spi->read_offset = len;
-}
-
-static void bcm53xxspi_buf_read(struct bcm53xxspi *b53spi, u8 *r_buf,
-				size_t len, bool cont)
-{
-	u32 tmp;
-	int i;
-
-	for (i = 0; i < b53spi->read_offset + len; i++) {
-		tmp = B53SPI_CDRAM_CONT | B53SPI_CDRAM_PCS_DISABLE_ALL |
-		      B53SPI_CDRAM_PCS_DSCK;
-		if (!cont && i == b53spi->read_offset + len - 1)
-			tmp &= ~B53SPI_CDRAM_CONT;
-		tmp &= ~0x1;
-		/* Command Register File */
-		bcm53xxspi_write(b53spi, B53SPI_MSPI_CDRAM + 4 * i, tmp);
-	}
-
-	/* Set queue pointers */
-	bcm53xxspi_write(b53spi, B53SPI_MSPI_NEWQP, 0);
-	bcm53xxspi_write(b53spi, B53SPI_MSPI_ENDQP,
-			 b53spi->read_offset + len - 1);
-
-	if (cont)
-		bcm53xxspi_write(b53spi, B53SPI_MSPI_WRITE_LOCK, 1);
-
-	/* Start SPI transfer */
-	tmp = bcm53xxspi_read(b53spi, B53SPI_MSPI_SPCR2);
-	tmp |= B53SPI_MSPI_SPCR2_SPE;
-	if (cont)
-		tmp |= B53SPI_MSPI_SPCR2_CONT_AFTER_CMD;
-	bcm53xxspi_write(b53spi, B53SPI_MSPI_SPCR2, tmp);
-
-	/* Wait for SPI to finish */
-	bcm53xxspi_wait(b53spi, bcm53xxspi_calc_timeout(len));
-
-	if (!cont)
-		bcm53xxspi_write(b53spi, B53SPI_MSPI_WRITE_LOCK, 0);
-
-	for (i = 0; i < len; ++i) {
-		int offset = b53spi->read_offset + i;
-
-		/* Data stored in the transmit register file LSB */
-		r_buf[i] = (u8)bcm53xxspi_read(b53spi, B53SPI_MSPI_RXRAM + 4 * (1 + offset * 2));
-	}
-
-	b53spi->read_offset = 0;
-}
-
-static int bcm53xxspi_transfer_one(struct spi_master *master,
-				   struct spi_device *spi,
-				   struct spi_transfer *t)
-{
-	struct bcm53xxspi *b53spi = spi_master_get_devdata(master);
-	u8 *buf;
-	size_t left;
-
-	if (t->tx_buf) {
-		buf = (u8 *)t->tx_buf;
-		left = t->len;
-		while (left) {
-			size_t to_write = min_t(size_t, 16, left);
-			bool cont = left - to_write > 0;
-
-			bcm53xxspi_buf_write(b53spi, buf, to_write, cont);
-			left -= to_write;
-			buf += to_write;
-		}
-	}
-
-	if (t->rx_buf) {
-		buf = (u8 *)t->rx_buf;
-		left = t->len;
-		while (left) {
-			size_t to_read = min_t(size_t, 16 - b53spi->read_offset,
-					       left);
-			bool cont = left - to_read > 0;
-
-			bcm53xxspi_buf_read(b53spi, buf, to_read, cont);
-			left -= to_read;
-			buf += to_read;
-		}
-	}
-
-	return 0;
-}
-
-/**************************************************
- * BCMA
- **************************************************/
-
-static struct spi_board_info bcm53xx_info = {
-	.modalias	= "bcm53xxspiflash",
-};
-
-static const struct bcma_device_id bcm53xxspi_bcma_tbl[] = {
-	BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_NS_QSPI, BCMA_ANY_REV, BCMA_ANY_CLASS),
-	{},
-};
-MODULE_DEVICE_TABLE(bcma, bcm53xxspi_bcma_tbl);
-
-static int bcm53xxspi_bcma_probe(struct bcma_device *core)
-{
-	struct bcm53xxspi *b53spi;
-	struct spi_master *master;
-	int err;
-
-	if (core->bus->drv_cc.core->id.rev != 42) {
-		pr_err("SPI on SoC with unsupported ChipCommon rev\n");
-		return -ENOTSUPP;
-	}
-
-	master = spi_alloc_master(&core->dev, sizeof(*b53spi));
-	if (!master)
-		return -ENOMEM;
-
-	b53spi = spi_master_get_devdata(master);
-	b53spi->master = master;
-	b53spi->core = core;
-
-	master->transfer_one = bcm53xxspi_transfer_one;
-
-	bcma_set_drvdata(core, b53spi);
-
-	err = devm_spi_register_master(&core->dev, master);
-	if (err) {
-		spi_master_put(master);
-		bcma_set_drvdata(core, NULL);
-		goto out;
-	}
-
-	/* Broadcom SoCs (at least with the CC rev 42) use SPI for flash only */
-	spi_new_device(master, &bcm53xx_info);
-
-out:
-	return err;
-}
-
-static void bcm53xxspi_bcma_remove(struct bcma_device *core)
-{
-	struct bcm53xxspi *b53spi = bcma_get_drvdata(core);
-
-	spi_unregister_master(b53spi->master);
-}
-
-static struct bcma_driver bcm53xxspi_bcma_driver = {
-	.name		= KBUILD_MODNAME,
-	.id_table	= bcm53xxspi_bcma_tbl,
-	.probe		= bcm53xxspi_bcma_probe,
-	.remove		= bcm53xxspi_bcma_remove,
-};
-
-/**************************************************
- * Init & exit
- **************************************************/
-
-static int __init bcm53xxspi_module_init(void)
-{
-	int err = 0;
-
-	err = bcma_driver_register(&bcm53xxspi_bcma_driver);
-	if (err)
-		pr_err("Failed to register bcma driver: %d\n", err);
-
-	return err;
-}
-
-static void __exit bcm53xxspi_module_exit(void)
-{
-	bcma_driver_unregister(&bcm53xxspi_bcma_driver);
-}
-
-module_init(bcm53xxspi_module_init);
-module_exit(bcm53xxspi_module_exit);
-
-MODULE_DESCRIPTION("Broadcom BCM53xx SPI Controller driver");
-MODULE_AUTHOR("Rafał Miłecki <zajec5@gmail.com>");
-MODULE_LICENSE("GPL");
diff --git a/drivers/spi/spi-bcm53xx.h b/drivers/spi/spi-bcm53xx.h
deleted file mode 100644
index 73575df..0000000
--- a/drivers/spi/spi-bcm53xx.h
+++ /dev/null
@@ -1,72 +0,0 @@
-#ifndef SPI_BCM53XX_H
-#define SPI_BCM53XX_H
-
-#define B53SPI_BSPI_REVISION_ID			0x000
-#define B53SPI_BSPI_SCRATCH			0x004
-#define B53SPI_BSPI_MAST_N_BOOT_CTRL		0x008
-#define B53SPI_BSPI_BUSY_STATUS			0x00c
-#define B53SPI_BSPI_INTR_STATUS			0x010
-#define B53SPI_BSPI_B0_STATUS			0x014
-#define B53SPI_BSPI_B0_CTRL			0x018
-#define B53SPI_BSPI_B1_STATUS			0x01c
-#define B53SPI_BSPI_B1_CTRL			0x020
-#define B53SPI_BSPI_STRAP_OVERRIDE_CTRL		0x024
-#define B53SPI_BSPI_FLEX_MODE_ENABLE		0x028
-#define B53SPI_BSPI_BITS_PER_CYCLE		0x02c
-#define B53SPI_BSPI_BITS_PER_PHASE		0x030
-#define B53SPI_BSPI_CMD_AND_MODE_BYTE		0x034
-#define B53SPI_BSPI_BSPI_FLASH_UPPER_ADDR_BYTE	0x038
-#define B53SPI_BSPI_BSPI_XOR_VALUE		0x03c
-#define B53SPI_BSPI_BSPI_XOR_ENABLE		0x040
-#define B53SPI_BSPI_BSPI_PIO_MODE_ENABLE	0x044
-#define B53SPI_BSPI_BSPI_PIO_IODIR		0x048
-#define B53SPI_BSPI_BSPI_PIO_DATA		0x04c
-
-/* RAF */
-#define B53SPI_RAF_START_ADDR			0x100
-#define B53SPI_RAF_NUM_WORDS			0x104
-#define B53SPI_RAF_CTRL				0x108
-#define B53SPI_RAF_FULLNESS			0x10c
-#define B53SPI_RAF_WATERMARK			0x110
-#define B53SPI_RAF_STATUS			0x114
-#define B53SPI_RAF_READ_DATA			0x118
-#define B53SPI_RAF_WORD_CNT			0x11c
-#define B53SPI_RAF_CURR_ADDR			0x120
-
-/* MSPI */
-#define B53SPI_MSPI_SPCR0_LSB			0x200
-#define B53SPI_MSPI_SPCR0_MSB			0x204
-#define B53SPI_MSPI_SPCR1_LSB			0x208
-#define B53SPI_MSPI_SPCR1_MSB			0x20c
-#define B53SPI_MSPI_NEWQP			0x210
-#define B53SPI_MSPI_ENDQP			0x214
-#define B53SPI_MSPI_SPCR2			0x218
-#define  B53SPI_MSPI_SPCR2_SPE			0x00000040
-#define  B53SPI_MSPI_SPCR2_CONT_AFTER_CMD	0x00000080
-#define B53SPI_MSPI_MSPI_STATUS			0x220
-#define  B53SPI_MSPI_MSPI_STATUS_SPIF		0x00000001
-#define B53SPI_MSPI_CPTQP			0x224
-#define B53SPI_MSPI_TXRAM			0x240 /* 32 registers, up to 0x2b8 */
-#define B53SPI_MSPI_RXRAM			0x2c0 /* 32 registers, up to 0x33c */
-#define B53SPI_MSPI_CDRAM			0x340 /* 16 registers, up to 0x37c */
-#define  B53SPI_CDRAM_PCS_PCS0			0x00000001
-#define  B53SPI_CDRAM_PCS_PCS1			0x00000002
-#define  B53SPI_CDRAM_PCS_PCS2			0x00000004
-#define  B53SPI_CDRAM_PCS_PCS3			0x00000008
-#define  B53SPI_CDRAM_PCS_DISABLE_ALL		0x0000000f
-#define  B53SPI_CDRAM_PCS_DSCK			0x00000010
-#define  B53SPI_CDRAM_BITSE			0x00000040
-#define  B53SPI_CDRAM_CONT			0x00000080
-#define B53SPI_MSPI_WRITE_LOCK			0x380
-#define B53SPI_MSPI_DISABLE_FLUSH_GEN		0x384
-
-/* Interrupt */
-#define B53SPI_INTR_RAF_LR_FULLNESS_REACHED	0x3a0
-#define B53SPI_INTR_RAF_LR_TRUNCATED		0x3a4
-#define B53SPI_INTR_RAF_LR_IMPATIENT		0x3a8
-#define B53SPI_INTR_RAF_LR_SESSION_DONE		0x3ac
-#define B53SPI_INTR_RAF_LR_OVERREAD		0x3b0
-#define B53SPI_INTR_MSPI_DONE			0x3b4
-#define B53SPI_INTR_MSPI_HALT_SET_TRANSACTION_DONE	0x3b8
-
-#endif /* SPI_BCM53XX_H */
-- 
1.7.9.5

Re: [PATCH 2/4] spi: bcm53xx: Refactor to make driver nonspecific to 53xx SoCs

[Andy Shevchenko]

On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
<jonathar@broadcom.com> wrote:
> The Broadcom MSPI controller is used on various SoCs. It is being
> renamed so that it can be extended and reused on other chips. It is
> renamed to bcm-mspi.
>
What if you resend this one with -M -C applied?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] spi: bcm53xx: Refactor to make driver nonspecific to 53xx SoCs

[Rafał Miłecki]

On 3 April 2015 at 15:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
>> The Broadcom MSPI controller is used on various SoCs. It is being
>> renamed so that it can be extended and reused on other chips. It is
>> renamed to bcm-mspi.
>>
> What if you resend this one with -M -C applied?

Definitely, right now I can't really review this patch (and I want to).

-- 
Rafał

Re: [PATCH 2/4] spi: bcm53xx: Refactor to make driver nonspecific to 53xx SoCs

[Jonathan Richardson]

On 15-04-06 03:18 AM, Rafał Miłecki wrote:
> On 3 April 2015 at 15:35, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>> <jonathar@broadcom.com> wrote:
>>> The Broadcom MSPI controller is used on various SoCs. It is being
>>> renamed so that it can be extended and reused on other chips. It is
>>> renamed to bcm-mspi.
>>>
>> What if you resend this one with -M -C applied?
> 
> Definitely, right now I can't really review this patch (and I want to).
> 

Thanks Rafal for the review. Please see previous reply to Andy and let
me know if I'm missing something.

Re: [PATCH 2/4] spi: bcm53xx: Refactor to make driver nonspecific to 53xx SoCs

[Jonathan Richardson]

On 15-04-03 06:35 AM, Andy Shevchenko wrote:
> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
>> The Broadcom MSPI controller is used on various SoCs. It is being
>> renamed so that it can be extended and reused on other chips. It is
>> renamed to bcm-mspi.
>>
> What if you resend this one with -M -C applied?
> 
> 

I'm not seeing any difference in the patches unfortunately. I'll keep
playing with it and re-send if I can find a way to improve it.

The changes are just renaming variables, structures, functions, file
name to get rid of 53xx and replace with mspi. The config is renamed
from SPI_BCM53XX to SPI_BCMA_MSPI.

Re: [PATCH 2/4] spi: bcm53xx: Refactor to make driver nonspecific to 53xx SoCs

[Andy Shevchenko]

On Mon, Apr 6, 2015 at 9:30 PM, Jonathan Richardson
<jonathar@broadcom.com> wrote:
> On 15-04-03 06:35 AM, Andy Shevchenko wrote:
>> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>> <jonathar@broadcom.com> wrote:
>>> The Broadcom MSPI controller is used on various SoCs. It is being
>>> renamed so that it can be extended and reused on other chips. It is
>>> renamed to bcm-mspi.
>>>
>> What if you resend this one with -M -C applied?
>>
>>
>
> I'm not seeing any difference in the patches unfortunately. I'll keep
> playing with it and re-send if I can find a way to improve it.
>
> The changes are just renaming variables, structures, functions, file
> name to get rid of 53xx and replace with mspi. The config is renamed
> from SPI_BCM53XX to SPI_BCMA_MSPI.
>

Better to split: a) moving, b) renaming something inside.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/4] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

[Jonathan Richardson]

The Broadcom MSPI controller is used on various chips. The driver only
supported BCM53xx chips with BCMA (an AMBA bus variant). The driver is
refactored to make BCMA optional and provides a new config for non BCMA
systems.

Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/spi/Kconfig        |    5 ++
 drivers/spi/Makefile       |    1 +
 drivers/spi/spi-bcm-mspi.c |  200 +++++++++++++++++++++++++++++++++-----------
 3 files changed, 155 insertions(+), 51 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 766e08d..23f2357 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -120,6 +120,11 @@ config SPI_BCMA_MSPI
 	help
 	  Enable support for the Broadcom BCMA MSPI controller.
 
+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 6b58100..36872d2 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_SPI_ATH79)			+= spi-ath79.o
 obj-$(CONFIG_SPI_AU1550)		+= spi-au1550.o
 obj-$(CONFIG_SPI_BCM2835)		+= spi-bcm2835.o
 obj-$(CONFIG_SPI_BCMA_MSPI)		+= spi-bcm-mspi.o
+obj-$(CONFIG_SPI_BCM_MSPI)		+= spi-bcm-mspi.o
 obj-$(CONFIG_SPI_BCM63XX)		+= spi-bcm63xx.o
 obj-$(CONFIG_SPI_BCM63XX_HSSPI)		+= spi-bcm63xx-hsspi.o
 obj-$(CONFIG_SPI_BFIN5XX)		+= spi-bfin5xx.o
diff --git a/drivers/spi/spi-bcm-mspi.c b/drivers/spi/spi-bcm-mspi.c
index 67ea246..df27449 100644
--- a/drivers/spi/spi-bcm-mspi.c
+++ b/drivers/spi/spi-bcm-mspi.c
@@ -11,11 +11,13 @@
  * 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/bcma/bcma.h>
 #include <linux/spi/spi.h>
+#include <linux/of.h>
 
 #include "spi-bcm-mspi.h"
 
@@ -25,22 +27,17 @@
 #define BCM_MSPI_SPE_TIMEOUT_MS 80
 
 struct bcm_mspi {
+	#ifdef CONFIG_SPI_BCMA_MSPI
 	struct bcma_device *core;
-	struct spi_master *master;
+	#endif
 
+	void __iomem *base;
+	struct spi_master *master;
 	size_t read_offset;
-};
 
-static inline u32 bcm_mspi_read(struct bcm_mspi *mspi, u16 offset)
-{
-	return bcma_read32(mspi->core, offset);
-}
-
-static inline void bcm_mspi_write(struct bcm_mspi *mspi, u16 offset,
-				    u32 value)
-{
-	bcma_write32(mspi->core, offset, value);
-}
+	void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
+	u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
+};
 
 static inline unsigned int bcm_mspi_calc_timeout(size_t len)
 {
@@ -56,7 +53,7 @@ static int bcm_mspi_wait(struct bcm_mspi *mspi, unsigned int timeout_ms)
 	/* SPE bit has to be 0 before we read MSPI STATUS */
 	deadline = jiffies + BCM_MSPI_SPE_TIMEOUT_MS * HZ / 1000;
 	do {
-		tmp = bcm_mspi_read(mspi, MSPI_SPCR2);
+		tmp = mspi->mspi_read(mspi, MSPI_SPCR2);
 		if (!(tmp & MSPI_SPCR2_SPE))
 			break;
 		udelay(5);
@@ -68,9 +65,9 @@ static int bcm_mspi_wait(struct bcm_mspi *mspi, unsigned int timeout_ms)
 	/* Check status */
 	deadline = jiffies + timeout_ms * HZ / 1000;
 	do {
-		tmp = bcm_mspi_read(mspi, MSPI_MSPI_STATUS);
+		tmp = mspi->mspi_read(mspi, MSPI_MSPI_STATUS);
 		if (tmp & MSPI_MSPI_STATUS_SPIF) {
-			bcm_mspi_write(mspi, MSPI_MSPI_STATUS, 0);
+			mspi->mspi_write(mspi, MSPI_MSPI_STATUS, 0);
 			return 0;
 		}
 
@@ -79,7 +76,7 @@ static int bcm_mspi_wait(struct bcm_mspi *mspi, unsigned int timeout_ms)
 	} while (!time_after_eq(jiffies, deadline));
 
 spi_timeout:
-	bcm_mspi_write(mspi, MSPI_MSPI_STATUS, 0);
+	mspi->mspi_write(mspi, MSPI_MSPI_STATUS, 0);
 
 	pr_err("Timeout waiting for SPI to be ready!\n");
 
@@ -94,7 +91,7 @@ static void bcm_mspi_buf_write(struct bcm_mspi *mspi, u8 *w_buf,
 
 	for (i = 0; i < len; i++) {
 		/* Transmit Register File MSB */
-		bcm_mspi_write(mspi, MSPI_TXRAM + 4 * (i * 2),
+		mspi->mspi_write(mspi, MSPI_TXRAM + 4 * (i * 2),
 				 (unsigned int)w_buf[i]);
 	}
 
@@ -104,28 +101,28 @@ static void bcm_mspi_buf_write(struct bcm_mspi *mspi, u8 *w_buf,
 			tmp &= ~CDRAM_CONT;
 		tmp &= ~0x1;
 		/* Command Register File */
-		bcm_mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
+		mspi->mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
 	}
 
 	/* Set queue pointers */
-	bcm_mspi_write(mspi, MSPI_NEWQP, 0);
-	bcm_mspi_write(mspi, MSPI_ENDQP, len - 1);
+	mspi->mspi_write(mspi, MSPI_NEWQP, 0);
+	mspi->mspi_write(mspi, MSPI_ENDQP, len - 1);
 
 	if (cont)
-		bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 1);
+		mspi->mspi_write(mspi, MSPI_WRITE_LOCK, 1);
 
 	/* Start SPI transfer */
-	tmp = bcm_mspi_read(mspi, MSPI_SPCR2);
+	tmp = mspi->mspi_read(mspi, MSPI_SPCR2);
 	tmp |= MSPI_SPCR2_SPE;
 	if (cont)
 		tmp |= MSPI_SPCR2_CONT_AFTER_CMD;
-	bcm_mspi_write(mspi, MSPI_SPCR2, tmp);
+	mspi->mspi_write(mspi, MSPI_SPCR2, tmp);
 
 	/* Wait for SPI to finish */
 	bcm_mspi_wait(mspi, bcm_mspi_calc_timeout(len));
 
 	if (!cont)
-		bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 0);
+		mspi->mspi_write(mspi, MSPI_WRITE_LOCK, 0);
 
 	mspi->read_offset = len;
 }
@@ -143,35 +140,35 @@ static void bcm_mspi_buf_read(struct bcm_mspi *mspi, u8 *r_buf,
 			tmp &= ~CDRAM_CONT;
 		tmp &= ~0x1;
 		/* Command Register File */
-		bcm_mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
+		mspi->mspi_write(mspi, MSPI_CDRAM + 4 * i, tmp);
 	}
 
 	/* Set queue pointers */
-	bcm_mspi_write(mspi, MSPI_NEWQP, 0);
-	bcm_mspi_write(mspi, MSPI_ENDQP,
+	mspi->mspi_write(mspi, MSPI_NEWQP, 0);
+	mspi->mspi_write(mspi, MSPI_ENDQP,
 			 mspi->read_offset + len - 1);
 
 	if (cont)
-		bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 1);
+		mspi->mspi_write(mspi, MSPI_WRITE_LOCK, 1);
 
 	/* Start SPI transfer */
-	tmp = bcm_mspi_read(mspi, MSPI_SPCR2);
+	tmp = mspi->mspi_read(mspi, MSPI_SPCR2);
 	tmp |= MSPI_SPCR2_SPE;
 	if (cont)
 		tmp |= MSPI_SPCR2_CONT_AFTER_CMD;
-	bcm_mspi_write(mspi, MSPI_SPCR2, tmp);
+	mspi->mspi_write(mspi, MSPI_SPCR2, tmp);
 
 	/* Wait for SPI to finish */
 	bcm_mspi_wait(mspi, bcm_mspi_calc_timeout(len));
 
 	if (!cont)
-		bcm_mspi_write(mspi, MSPI_WRITE_LOCK, 0);
+		mspi->mspi_write(mspi, MSPI_WRITE_LOCK, 0);
 
 	for (i = 0; i < len; ++i) {
 		int offset = mspi->read_offset + i;
 
 		/* Data stored in the transmit register file LSB */
-		r_buf[i] = (u8)bcm_mspi_read(mspi,
+		r_buf[i] = (u8)mspi->mspi_read(mspi,
 			MSPI_RXRAM + 4 * (1 + offset * 2));
 	}
 
@@ -216,9 +213,103 @@ static int bcm_mspi_transfer_one(struct spi_master *master,
 	return 0;
 }
 
-static struct spi_board_info bcm53xx_info = {
-	.modalias	= "bcm53xxspiflash",
+/*
+ * Allocate SPI master for both bcma and non bcma bus. The SPI device must be
+ * configured in DT.
+ */
+static struct bcm_mspi *bcm_mspi_init(struct device *dev)
+{
+	struct bcm_mspi *data;
+	struct spi_master *master;
+
+	master = spi_alloc_master(dev, sizeof(*data));
+	if (!master) {
+		dev_err(dev, "error allocating spi_master\n");
+		return 0;
+	}
+
+	data = spi_master_get_devdata(master);
+	data->master = master;
+
+	/* 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;
+
+	return data;
+}
+
+#ifdef CONFIG_SPI_BCM_MSPI
+
+static const struct of_device_id bcm_mspi_dt[] = {
+	{ .compatible = "brcm,mspi" },
+	{ },
 };
+MODULE_DEVICE_TABLE(of, bcm_mspi_dt);
+
+static inline u32 bcm_mspi_read(struct bcm_mspi *mspi, u16 offset)
+{
+	return readl(mspi->base + offset);
+}
+
+static inline void bcm_mspi_write(struct bcm_mspi *mspi, u16 offset,
+	u32 value)
+{
+	writel(value, mspi->base + offset);
+}
+
+/*
+ * Probe routine for non-bcma devices.
+ */
+static int bcm_mspi_probe(struct platform_device *pdev)
+{
+	struct bcm_mspi *data;
+	struct device *dev = &pdev->dev;
+	int err;
+	struct resource *res;
+
+	dev_info(dev, "BCM MSPI probe\n");
+
+	data = bcm_mspi_init(dev);
+	if (!data)
+		return -ENOMEM;
+
+	/* 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 I/O memory\n");
+		err = PTR_ERR(data->base);
+		goto out;
+	}
+
+	data->mspi_read = bcm_mspi_read;
+	data->mspi_write = bcm_mspi_write;
+	platform_set_drvdata(pdev, data);
+
+	err = devm_spi_register_master(dev, data->master);
+	if (err)
+		goto out;
+
+	return 0;
+
+out:
+	spi_master_put(data->master);
+	return err;
+}
+
+static struct platform_driver bcm_mspi_driver = {
+	.driver = {
+		.name = "bcm-mspi",
+		.of_match_table = bcm_mspi_dt,
+	},
+	.probe = bcm_mspi_probe,
+};
+
+module_platform_driver(bcm_mspi_driver);
+
+#endif
+
+#ifdef CONFIG_SPI_BCMA_MSPI
 
 static const struct bcma_device_id bcm_mspi_bcma_tbl[] = {
 	BCMA_CORE(BCMA_MANUF_BCM, BCMA_CORE_NS_QSPI, BCMA_ANY_REV,
@@ -227,6 +318,12 @@ static const struct bcma_device_id bcm_mspi_bcma_tbl[] = {
 };
 MODULE_DEVICE_TABLE(bcma, bcm_mspi_bcma_tbl);
 
+static const struct of_device_id bcm_bcma_mspi_dt[] = {
+	{ .compatible = "brcm,bcma-mspi" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, bcm_mspi_dt);
+
 static inline u32 bcm_bcma_mspi_read(struct bcm_mspi *mspi, u16 offset)
 {
 	return bcma_read32(mspi->core, offset);
@@ -238,53 +335,52 @@ static inline void bcm_bcma_mspi_write(struct bcm_mspi *mspi, u16 offset,
 	bcma_write32(mspi->core, offset, value);
 }
 
+/*
+ * Probe routine for bcma devices.
+ */
 static int bcm_mspi_bcma_probe(struct bcma_device *core)
 {
 	struct bcm_mspi *data;
-	struct spi_master *master;
 	int err;
 
 	dev_info(&core->dev, "BCM MSPI BCMA probe\n");
 
 	if (core->bus->drv_cc.core->id.rev != 42) {
-		pr_err("SPI on SoC with unsupported ChipCommon rev\n");
+		dev_err(&core->dev,
+			"SPI on SoC with unsupported ChipCommon rev\n");
 		return -ENOTSUPP;
 	}
 
-	master = spi_alloc_master(&core->dev, sizeof(*data));
-	if (!master)
+	data = bcm_mspi_init(&core->dev);
+	if (!data)
 		return -ENOMEM;
 
-	data = spi_master_get_devdata(master);
-	data->master = master;
+	data->mspi_read = bcm_bcma_mspi_read;
+	data->mspi_write = bcm_bcma_mspi_write;
 	data->core = core;
-
-	master->transfer_one = bcm_mspi_transfer_one;
 	bcma_set_drvdata(core, data);
 
 	err = devm_spi_register_master(&core->dev, data->master);
 	if (err) {
-		spi_master_put(master);
-		bcma_set_drvdata(core, NULL);
-		goto out;
+		spi_master_put(data->master);
+		return err;
 	}
 
-	/* Broadcom SoCs (at least with the CC rev 42) use SPI for flash only */
-	spi_new_device(master, &bcm53xx_info);
-
-out:
-	return err;
+	return 0;
 }
 
 static struct bcma_driver bcm_mspi_bcma_driver = {
 	.name		= KBUILD_MODNAME,
+	.drv = {
+		.of_match_table = bcm_bcma_mspi_dt,
+	},
 	.id_table	= bcm_mspi_bcma_tbl,
 	.probe		= bcm_mspi_bcma_probe,
 };
 
 static int __init bcm_mspi_bcma_module_init(void)
 {
-	int err = 0;
+	int err;
 
 	err = bcma_driver_register(&bcm_mspi_bcma_driver);
 	if (err)
@@ -301,6 +397,8 @@ static void __exit bcm_mspi_bcma_module_exit(void)
 module_init(bcm_mspi_bcma_module_init);
 module_exit(bcm_mspi_bcma_module_exit);
 
+#endif
+
 MODULE_DESCRIPTION("Broadcom MSPI SPI Controller driver");
 MODULE_AUTHOR("Rafał Miłecki <zajec5@gmail.com>");
 MODULE_AUTHOR("Broadcom");
-- 
1.7.9.5

Re: [PATCH 3/4] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

[Andy Shevchenko]

On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
<jonathar@broadcom.com> wrote:
> The Broadcom MSPI controller is used on various chips. The driver only
> supported BCM53xx chips with BCMA (an AMBA bus variant). The driver is
> refactored to make BCMA optional and provides a new config for non BCMA
> systems.

>  struct bcm_mspi {
> +       #ifdef CONFIG_SPI_BCMA_MSPI
>         struct bcma_device *core;
> -       struct spi_master *master;
> +       #endif
>
> +       void __iomem *base;
> +       struct spi_master *master;
>         size_t read_offset;

> +       void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
> +       u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
> +};

To avoid ugly ifdefs I think better to split driver to core part and
the actual driver part, at the end you will have something like
mspi-core.c mspi-53xx.c mspi-whatever.c. Check for example spi-dw*.c

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

[Florian Fainelli]

On 03/04/15 06:38, Andy Shevchenko wrote:
> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
>> The Broadcom MSPI controller is used on various chips. The driver only
>> supported BCM53xx chips with BCMA (an AMBA bus variant). The driver is
>> refactored to make BCMA optional and provides a new config for non BCMA
>> systems.
> 
>>  struct bcm_mspi {
>> +       #ifdef CONFIG_SPI_BCMA_MSPI
>>         struct bcma_device *core;
>> -       struct spi_master *master;
>> +       #endif
>>
>> +       void __iomem *base;
>> +       struct spi_master *master;
>>         size_t read_offset;
> 
>> +       void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
>> +       u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
>> +};
> 
> To avoid ugly ifdefs I think better to split driver to core part and
> the actual driver part, at the end you will have something like
> mspi-core.c mspi-53xx.c mspi-whatever.c. Check for example spi-dw*.c
> 

Actually, I am really curious whether we need the special BCMA I/O
accessors in the first place, cannot we just access the MSPI core on
BCM53xx chips using regular MMIO? That would probably solve the
"problem" entirely. Rafal, did you try this before?

As for splitting the driver into a "library" driver which is mostly
independent from the bus and a bus-specific wrapper, I think BCMA is
really the only special case here, which is why I suggested earlier to
Jonathan that we might just prefer ifdefing things out instead of
creating a separate layer just for BCMA.
-- 
Florian

Re: [PATCH 3/4] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

[Rafał Miłecki]

On 3 April 2015 at 19:52, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 03/04/15 06:38, Andy Shevchenko wrote:
>> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>> <jonathar@broadcom.com> wrote:
>>> The Broadcom MSPI controller is used on various chips. The driver only
>>> supported BCM53xx chips with BCMA (an AMBA bus variant). The driver is
>>> refactored to make BCMA optional and provides a new config for non BCMA
>>> systems.
>>
>>>  struct bcm_mspi {
>>> +       #ifdef CONFIG_SPI_BCMA_MSPI
>>>         struct bcma_device *core;
>>> -       struct spi_master *master;
>>> +       #endif
>>>
>>> +       void __iomem *base;
>>> +       struct spi_master *master;
>>>         size_t read_offset;
>>
>>> +       void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
>>> +       u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
>>> +};
>>
>> To avoid ugly ifdefs I think better to split driver to core part and
>> the actual driver part, at the end you will have something like
>> mspi-core.c mspi-53xx.c mspi-whatever.c. Check for example spi-dw*.c
>
> Actually, I am really curious whether we need the special BCMA I/O
> accessors in the first place, cannot we just access the MSPI core on
> BCM53xx chips using regular MMIO? That would probably solve the
> "problem" entirely. Rafal, did you try this before?

It's a matter of choice between:
1) Using one design for all bcma users
2) Using one design for all bcm-mspi users
I believe no matter which one you choose, you'll break another one.

If you take a look at drivers/bcma/host_soc.c, you'll see we've there
core->io_addr. I guess you could use it as the base in bcm-mspi. That
of course will make you a bit less compatible with other bcma drivers
(skipping bcma R/W layer).


> As for splitting the driver into a "library" driver which is mostly
> independent from the bus and a bus-specific wrapper, I think BCMA is
> really the only special case here, which is why I suggested earlier to
> Jonathan that we might just prefer ifdefing things out instead of
> creating a separate layer just for BCMA.

I think you may be right, this #if for bcma shouldn't be that bad and
it shouldn't grow in the future.
Still, I'd like to get this patch split nicely to review independent changes.

-- 
Rafał

Re: [PATCH 3/4] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

[Jonathan Richardson]

On 15-04-06 03:36 AM, Rafał Miłecki wrote:
> On 3 April 2015 at 19:52, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 03/04/15 06:38, Andy Shevchenko wrote:
>>> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>>> <jonathar@broadcom.com> wrote:
>>>> The Broadcom MSPI controller is used on various chips. The driver only
>>>> supported BCM53xx chips with BCMA (an AMBA bus variant). The driver is
>>>> refactored to make BCMA optional and provides a new config for non BCMA
>>>> systems.
>>>
>>>>  struct bcm_mspi {
>>>> +       #ifdef CONFIG_SPI_BCMA_MSPI
>>>>         struct bcma_device *core;
>>>> -       struct spi_master *master;
>>>> +       #endif
>>>>
>>>> +       void __iomem *base;
>>>> +       struct spi_master *master;
>>>>         size_t read_offset;
>>>
>>>> +       void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
>>>> +       u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
>>>> +};
>>>
>>> To avoid ugly ifdefs I think better to split driver to core part and
>>> the actual driver part, at the end you will have something like
>>> mspi-core.c mspi-53xx.c mspi-whatever.c. Check for example spi-dw*.c
>>
>> Actually, I am really curious whether we need the special BCMA I/O
>> accessors in the first place, cannot we just access the MSPI core on
>> BCM53xx chips using regular MMIO? That would probably solve the
>> "problem" entirely. Rafal, did you try this before?
> 
> It's a matter of choice between:
> 1) Using one design for all bcma users
> 2) Using one design for all bcm-mspi users
> I believe no matter which one you choose, you'll break another one.
> 
> If you take a look at drivers/bcma/host_soc.c, you'll see we've there
> core->io_addr. I guess you could use it as the base in bcm-mspi. That
> of course will make you a bit less compatible with other bcma drivers
> (skipping bcma R/W layer).

That would require compiling in BCMA for a driver/chip that doesn't use
BCMA but then I could do DT parsing in init only anyway. I don't think
that's really an option so I'm going to leave as is unless there is
further opinion on it.

> 
> 
>> As for splitting the driver into a "library" driver which is mostly
>> independent from the bus and a bus-specific wrapper, I think BCMA is
>> really the only special case here, which is why I suggested earlier to
>> Jonathan that we might just prefer ifdefing things out instead of
>> creating a separate layer just for BCMA.
> 
> I think you may be right, this #if for bcma shouldn't be that bad and
> it shouldn't grow in the future.
> Still, I'd like to get this patch split nicely to review independent changes.
> 

Making BCMA optional was made possible by using DT. I'm not sure I could
split it into two commits. I would have to add a hard coded SPI device
for non-BMCA as well. I thought the driver was a bit odd in that this
was hard coded. Normally this should be in a separate driver. How would
you use it if you wanted to use m25p80 for example?

Re: [PATCH 3/4] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

[Jonathan Richardson]

On 15-04-03 10:52 AM, Florian Fainelli wrote:
> On 03/04/15 06:38, Andy Shevchenko wrote:
>> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
>> <jonathar@broadcom.com> wrote:
>>> The Broadcom MSPI controller is used on various chips. The driver only
>>> supported BCM53xx chips with BCMA (an AMBA bus variant). The driver is
>>> refactored to make BCMA optional and provides a new config for non BCMA
>>> systems.
>>
>>>  struct bcm_mspi {
>>> +       #ifdef CONFIG_SPI_BCMA_MSPI
>>>         struct bcma_device *core;
>>> -       struct spi_master *master;
>>> +       #endif
>>>
>>> +       void __iomem *base;
>>> +       struct spi_master *master;
>>>         size_t read_offset;
>>
>>> +       void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
>>> +       u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
>>> +};
>>
>> To avoid ugly ifdefs I think better to split driver to core part and
>> the actual driver part, at the end you will have something like
>> mspi-core.c mspi-53xx.c mspi-whatever.c. Check for example spi-dw*.c
>>
> 
> Actually, I am really curious whether we need the special BCMA I/O
> accessors in the first place, cannot we just access the MSPI core on
> BCM53xx chips using regular MMIO? That would probably solve the
> "problem" entirely. Rafal, did you try this before?
> 
> As for splitting the driver into a "library" driver which is mostly
> independent from the bus and a bus-specific wrapper, I think BCMA is
> really the only special case here, which is why I suggested earlier to
> Jonathan that we might just prefer ifdefing things out instead of
> creating a separate layer just for BCMA.
> 

I cringed adding the ifdefs to the driver but didn't think the small
amount of code that wouldn't be used again warranted 3 files. I could
also handle the two different probe routines by doing some DT parsing in
init, but then BCMA would have to be compiled for the non-BCMA MSPI
driver and I didn't want to do that either.

Re: [PATCH 3/4] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

[Rafał Miłecki]

On 3 April 2015 at 15:38, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Apr 2, 2015 at 10:23 PM, Jonathan Richardson
> <jonathar@broadcom.com> wrote:
>> The Broadcom MSPI controller is used on various chips. The driver only
>> supported BCM53xx chips with BCMA (an AMBA bus variant). The driver is
>> refactored to make BCMA optional and provides a new config for non BCMA
>> systems.
>
>>  struct bcm_mspi {
>> +       #ifdef CONFIG_SPI_BCMA_MSPI
>>         struct bcma_device *core;
>> -       struct spi_master *master;
>> +       #endif
>>
>> +       void __iomem *base;
>> +       struct spi_master *master;
>>         size_t read_offset;
>
>> +       void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
>> +       u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
>> +};
>
> To avoid ugly ifdefs I think better to split driver to core part and
> the actual driver part, at the end you will have something like
> mspi-core.c mspi-53xx.c mspi-whatever.c. Check for example spi-dw*.c

I also believe we usually (always?) don't align any #if-s (no indent/tabs).

-- 
Rafał

Re: [PATCH 3/4] spi: bcm-mspi: Make BCMA optional to support non-BCMA chips

[Rafał Miłecki]

On 2 April 2015 at 21:23, Jonathan Richardson <jonathar@broadcom.com> wrote:
> The Broadcom MSPI controller is used on various chips. The driver only
> supported BCM53xx chips with BCMA (an AMBA bus variant). The driver is
> refactored to make BCMA optional and provides a new config for non BCMA
> systems.

I think this patch provides 3 changes instead of just one described
above. You refactored R/W ops (pointers), made bcma optional and added
DT support. What about patch-per-change?

[PATCH 4/4] spi: bcm-mspi: Add support to set serial baud clock rate

[Jonathan Richardson]

The driver wasn't setting the SPBR (serial clock baud rate) which caused
it to run at the slowest speed possible. The driver now calculates the
SPBR based on the reference clock frequency resulting in much faster
SPI transfers.

Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 drivers/spi/spi-bcm-mspi.c |   48 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/spi/spi-bcm-mspi.c b/drivers/spi/spi-bcm-mspi.c
index df27449..5617b5b 100644
--- a/drivers/spi/spi-bcm-mspi.c
+++ b/drivers/spi/spi-bcm-mspi.c
@@ -18,10 +18,15 @@
 #include <linux/bcma/bcma.h>
 #include <linux/spi/spi.h>
 #include <linux/of.h>
+#include <linux/clk.h>
 
 #include "spi-bcm-mspi.h"
 
 #define BCM_MSPI_MAX_SPI_BAUD   13500000	/* 216 MHz? */
+#define SPBR_MIN                8U
+#define SPBR_MAX                255U
+#define MSPI_SPCR0_LSB_OFFSET   0x200
+#define MSPI_SPCR0_LSB_SHIFT    0
 
 /* The longest observed required wait was 19 ms */
 #define BCM_MSPI_SPE_TIMEOUT_MS 80
@@ -33,7 +38,9 @@ struct bcm_mspi {
 
 	void __iomem *base;
 	struct spi_master *master;
+	struct clk *clk;
 	size_t read_offset;
+	u32    spbr;
 
 	void (*mspi_write)(struct bcm_mspi *mspi, u16 offset, u32 value);
 	u32 (*mspi_read)(struct bcm_mspi *mspi, u16 offset);
@@ -45,6 +52,15 @@ static inline unsigned int bcm_mspi_calc_timeout(size_t len)
 	return (len * 9000 / BCM_MSPI_MAX_SPI_BAUD * 110 / 100) + 1;
 }
 
+static void bcm_mspi_hw_init(struct bcm_mspi *mspi)
+{
+	/* Set SPBR (serial clock baud rate). */
+	if (mspi->spbr) {
+		mspi->mspi_write(mspi, MSPI_SPCR0_LSB_OFFSET,
+			mspi->spbr << MSPI_SPCR0_LSB_SHIFT);
+	}
+}
+
 static int bcm_mspi_wait(struct bcm_mspi *mspi, unsigned int timeout_ms)
 {
 	unsigned long deadline;
@@ -221,6 +237,7 @@ static struct bcm_mspi *bcm_mspi_init(struct device *dev)
 {
 	struct bcm_mspi *data;
 	struct spi_master *master;
+	u32 desired_rate;
 
 	master = spi_alloc_master(dev, sizeof(*data));
 	if (!master) {
@@ -235,6 +252,31 @@ static struct bcm_mspi *bcm_mspi_init(struct device *dev)
 	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)) {
+		int ret = clk_prepare_enable(data->clk);
+
+		if (ret < 0) {
+			dev_err(dev, "failed to enable clock: %d\n", ret);
+			return 0;
+		}
+
+		/* Calculate SPBR if clock-frequency provided. */
+		if (of_property_read_u32(dev->of_node, "clock-frequency",
+			&desired_rate) >= 0) {
+			u32 spbr = clk_get_rate(data->clk) / (2 * desired_rate);
+
+			data->spbr = clamp_val(spbr, SPBR_MIN, SPBR_MAX);
+		}
+	}
+
 	return data;
 }
 
@@ -286,6 +328,9 @@ static int bcm_mspi_probe(struct platform_device *pdev)
 	data->mspi_write = bcm_mspi_write;
 	platform_set_drvdata(pdev, data);
 
+	/* Initialize SPI controller. */
+	bcm_mspi_hw_init(data);
+
 	err = devm_spi_register_master(dev, data->master);
 	if (err)
 		goto out;
@@ -360,6 +405,9 @@ static int bcm_mspi_bcma_probe(struct bcma_device *core)
 	data->core = core;
 	bcma_set_drvdata(core, data);
 
+	/* Initialize SPI controller. */
+	bcm_mspi_hw_init(data);
+
 	err = devm_spi_register_master(&core->dev, data->master);
 	if (err) {
 		spi_master_put(data->master);
-- 
1.7.9.5

Re: [PATCH 4/4] spi: bcm-mspi: Add support to set serial baud clock rate

[Florian Fainelli]

Le 02/04/2015 12:23, Jonathan Richardson a écrit :
> The driver wasn't setting the SPBR (serial clock baud rate) which caused
> it to run at the slowest speed possible. The driver now calculates the
> SPBR based on the reference clock frequency resulting in much faster
> SPI transfers.
> 
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---

[snip]

> +	data->clk = devm_clk_get(dev, "mspi_clk");
> +	if (!IS_ERR(data->clk)) {
> +		int ret = clk_prepare_enable(data->clk);
> +
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable clock: %d\n", ret);
> +			return 0;
> +		}
> +
> +		/* Calculate SPBR if clock-frequency provided. */
> +		if (of_property_read_u32(dev->of_node, "clock-frequency",
> +			&desired_rate) >= 0) {
> +			u32 spbr = clk_get_rate(data->clk) / (2 * desired_rate);

Usually, specifying a "clock-frequency" property is done when there is
no clock provider available, yet we take this code path only if we could
find a "mspi_clk" which sounds a litle weird.

Once there is a proper "mspi_clk" clock, I would make it mandatory for
the clock provider to be able to provide the rate as well?
--
Florian

Re: [PATCH 4/4] spi: bcm-mspi: Add support to set serial baud clock rate

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

On Sat, Apr 04, 2015 at 12:12:59PM -0700, Florian Fainelli wrote:
> Le 02/04/2015 12:23, Jonathan Richardson a écrit :

> > +		/* Calculate SPBR if clock-frequency provided. */
> > +		if (of_property_read_u32(dev->of_node, "clock-frequency",
> > +			&desired_rate) >= 0) {
> > +			u32 spbr = clk_get_rate(data->clk) / (2 * desired_rate);

> Usually, specifying a "clock-frequency" property is done when there is
> no clock provider available, yet we take this code path only if we could
> find a "mspi_clk" which sounds a litle weird.

> Once there is a proper "mspi_clk" clock, I would make it mandatory for
> the clock provider to be able to provide the rate as well?

As far as I can tell it's already mandatory if the property is present -
it's taking the clock presented to the block and then using an internal
divider to bring that down to the desired rate.

We are missing error handling though.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/4] spi: bcm-mspi: Add support to set serial baud clock rate

[Jonathan Richardson]

On 15-04-06 02:46 AM, Mark Brown wrote:
> On Sat, Apr 04, 2015 at 12:12:59PM -0700, Florian Fainelli wrote:
>> Le 02/04/2015 12:23, Jonathan Richardson a écrit :
> 
>>> +		/* Calculate SPBR if clock-frequency provided. */
>>> +		if (of_property_read_u32(dev->of_node, "clock-frequency",
>>> +			&desired_rate) >= 0) {
>>> +			u32 spbr = clk_get_rate(data->clk) / (2 * desired_rate);
> 
>> Usually, specifying a "clock-frequency" property is done when there is
>> no clock provider available, yet we take this code path only if we could
>> find a "mspi_clk" which sounds a litle weird.
> 
>> Once there is a proper "mspi_clk" clock, I would make it mandatory for
>> the clock provider to be able to provide the rate as well?
> 
> As far as I can tell it's already mandatory if the property is present -
> it's taking the clock presented to the block and then using an internal
> divider to bring that down to the desired rate.
> 
> We are missing error handling though.
> 

Thanks for the review. Yes that's correct. I also tried to make it
backwards compatible with the current version of the driver where you
can ignore configuring the frequency. The result being ref clock
frequency / 2 * 255. If you provide the clock then it will
enable/prepare it. If you also provide clock-frequency then it will
configure the SPBR. If you don't provide anything then it works as
before - it assumes the clock is already enabled and uses the h/w
default SPBR.