* [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-05-11 21:34 ` David Daney
0 siblings, 0 replies; 28+ messages in thread
From: David Daney @ 2012-05-11 21:34 UTC (permalink / raw)
To: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Grant Likely,
Rob Herring, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, David Daney,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
Add the driver, link it into the kbuild system and provide device tree
binding documentation.
Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
---
.../devicetree/bindings/spi/spi-octeon.txt | 33 ++
drivers/spi/Kconfig | 7 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-octeon.c | 369 ++++++++++++++++++++
4 files changed, 410 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/spi/spi-octeon.txt
create mode 100644 drivers/spi/spi-octeon.c
diff --git a/Documentation/devicetree/bindings/spi/spi-octeon.txt b/Documentation/devicetree/bindings/spi/spi-octeon.txt
new file mode 100644
index 0000000..431add1
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-octeon.txt
@@ -0,0 +1,33 @@
+Cavium, Inc. OCTEON SOC SPI master controller.
+
+Required properties:
+- compatible : "cavium,octeon-3010-spi"
+- reg : The register base for the controller.
+- interrupts : One interrupt, used by the controller.
+- #address-cells : <1>, as required by generic SPI binding.
+- #size-cells : <0>, also as required by generic SPI binding.
+
+Child nodes as per the generic SPI binding.
+
+Example:
+
+ spi@1070000001000 {
+ compatible = "cavium,octeon-3010-spi";
+ reg = <0x10700 0x00001000 0x0 0x100>;
+ interrupts = <0 58>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ eeprom@0 {
+ compatible = "st,m95256", "atmel,at25";
+ reg = <0>;
+ spi-max-frequency = <5000000>;
+ spi-cpha;
+ spi-cpol;
+
+ pagesize = <64>;
+ size = <32768>;
+ address-width = <16>;
+ };
+ };
+
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 00c0240..e1dd0d0 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -228,6 +228,13 @@ config SPI_OC_TINY
help
This is the driver for OpenCores tiny SPI master controller.
+config SPI_OCTEON
+ tristate "Cavium OCTEON SPI controller"
+ depends on CPU_CAVIUM_OCTEON
+ help
+ SPI host driver for the hardware found on some Cavium OCTEON
+ SOCs.
+
config SPI_OMAP_UWIRE
tristate "OMAP1 MicroWire"
depends on ARCH_OMAP1
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 9d75d21..c7f8b71 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC) += spi-mpc52xx-psc.o
obj-$(CONFIG_SPI_MPC52xx) += spi-mpc52xx.o
obj-$(CONFIG_SPI_NUC900) += spi-nuc900.o
obj-$(CONFIG_SPI_OC_TINY) += spi-oc-tiny.o
+obj-$(CONFIG_SPI_OCTEON) += spi-octeon.o
obj-$(CONFIG_SPI_OMAP_UWIRE) += spi-omap-uwire.o
obj-$(CONFIG_SPI_OMAP_100K) += spi-omap-100k.o
obj-$(CONFIG_SPI_OMAP24XX) += spi-omap2-mcspi.o
diff --git a/drivers/spi/spi-octeon.c b/drivers/spi/spi-octeon.c
new file mode 100644
index 0000000..7207aaf
--- /dev/null
+++ b/drivers/spi/spi-octeon.c
@@ -0,0 +1,369 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2011, 2012 Cavium, Inc.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/of_spi.h>
+#include <linux/io.h>
+#include <linux/of.h>
+
+#include <asm/octeon/octeon.h>
+#include <asm/octeon/cvmx-mpi-defs.h>
+
+#define DRV_VERSION "2.0" /* Version 1 was the out-of-tree driver */
+#define DRV_DESCRIPTION "Cavium, Inc. OCTEON SPI bus driver"
+
+
+#define OCTEON_SPI_CFG 0
+#define OCTEON_SPI_STS 0x08
+#define OCTEON_SPI_TX 0x10
+#define OCTEON_SPI_DAT0 0x80
+
+#define OCTEON_SPI_MAX_BYTES 9
+
+#define OCTEON_SPI_MAX_CLOCK_HZ 16000000
+
+struct octeon_spi {
+ struct spi_master *my_master;
+ u64 register_base;
+ u64 last_cfg;
+ u64 cs_enax;
+};
+
+struct octeon_spi_setup {
+ u32 max_speed_hz;
+ u8 chip_select;
+ u8 mode;
+ u8 bits_per_word;
+};
+
+static void octeon_spi_wait_ready(struct octeon_spi *p)
+{
+ union cvmx_mpi_sts mpi_sts;
+ unsigned int loops = 0;
+
+ do {
+ if (loops++)
+ __delay(500);
+ mpi_sts.u64 = cvmx_read_csr(p->register_base + OCTEON_SPI_STS);
+ } while (mpi_sts.s.busy);
+}
+
+static int octeon_spi_do_transfer(struct octeon_spi *p,
+ struct spi_message *msg,
+ struct spi_transfer *xfer,
+ bool last_xfer)
+{
+ union cvmx_mpi_cfg mpi_cfg;
+ union cvmx_mpi_tx mpi_tx;
+ unsigned int clkdiv;
+ unsigned int speed_hz;
+ int mode;
+ bool cpha, cpol;
+ int bits_per_word;
+ const u8 *tx_buf;
+ u8 *rx_buf;
+ int len;
+ int i;
+
+ struct octeon_spi_setup *msg_setup = spi_get_ctldata(msg->spi);
+
+ speed_hz = msg_setup->max_speed_hz;
+ mode = msg_setup->mode;
+ cpha = mode & SPI_CPHA;
+ cpol = mode & SPI_CPOL;
+ bits_per_word = msg_setup->bits_per_word;
+
+ if (xfer->speed_hz)
+ speed_hz = xfer->speed_hz;
+ if (xfer->bits_per_word)
+ bits_per_word = xfer->bits_per_word;
+
+ if (speed_hz > OCTEON_SPI_MAX_CLOCK_HZ)
+ speed_hz = OCTEON_SPI_MAX_CLOCK_HZ;
+
+ clkdiv = octeon_get_io_clock_rate() / (2 * speed_hz);
+
+ mpi_cfg.u64 = 0;
+
+ mpi_cfg.s.clkdiv = clkdiv;
+ mpi_cfg.s.cshi = (mode & SPI_CS_HIGH) ? 1 : 0;
+ mpi_cfg.s.lsbfirst = (mode & SPI_LSB_FIRST) ? 1 : 0;
+ mpi_cfg.s.wireor = (mode & SPI_3WIRE) ? 1 : 0;
+ mpi_cfg.s.idlelo = cpha != cpol;
+ mpi_cfg.s.cslate = cpha ? 1 : 0;
+ mpi_cfg.s.enable = 1;
+
+ if (msg_setup->chip_select < 4)
+ p->cs_enax |= 1ull << (12 + msg_setup->chip_select);
+ mpi_cfg.u64 |= p->cs_enax;
+
+ if (mpi_cfg.u64 != p->last_cfg) {
+ p->last_cfg = mpi_cfg.u64;
+ cvmx_write_csr(p->register_base + OCTEON_SPI_CFG, mpi_cfg.u64);
+ }
+ tx_buf = xfer->tx_buf;
+ rx_buf = xfer->rx_buf;
+ len = xfer->len;
+ while (len > OCTEON_SPI_MAX_BYTES) {
+ for (i = 0; i < OCTEON_SPI_MAX_BYTES; i++) {
+ u8 d;
+ if (tx_buf)
+ d = *tx_buf++;
+ else
+ d = 0;
+ cvmx_write_csr(p->register_base + OCTEON_SPI_DAT0 + (8 * i), d);
+ }
+ mpi_tx.u64 = 0;
+ mpi_tx.s.csid = msg_setup->chip_select;
+ mpi_tx.s.leavecs = 1;
+ mpi_tx.s.txnum = tx_buf ? OCTEON_SPI_MAX_BYTES : 0;
+ mpi_tx.s.totnum = OCTEON_SPI_MAX_BYTES;
+ cvmx_write_csr(p->register_base + OCTEON_SPI_TX, mpi_tx.u64);
+
+ octeon_spi_wait_ready(p);
+ if (rx_buf)
+ for (i = 0; i < OCTEON_SPI_MAX_BYTES; i++) {
+ u64 v = cvmx_read_csr(p->register_base + OCTEON_SPI_DAT0 + (8 * i));
+ *rx_buf++ = (u8)v;
+ }
+ len -= OCTEON_SPI_MAX_BYTES;
+ }
+
+ for (i = 0; i < len; i++) {
+ u8 d;
+ if (tx_buf)
+ d = *tx_buf++;
+ else
+ d = 0;
+ cvmx_write_csr(p->register_base + OCTEON_SPI_DAT0 + (8 * i), d);
+ }
+
+ mpi_tx.u64 = 0;
+ mpi_tx.s.csid = msg_setup->chip_select;
+ if (last_xfer)
+ mpi_tx.s.leavecs = xfer->cs_change;
+ else
+ mpi_tx.s.leavecs = !xfer->cs_change;
+ mpi_tx.s.txnum = tx_buf ? len : 0;
+ mpi_tx.s.totnum = len;
+ cvmx_write_csr(p->register_base + OCTEON_SPI_TX, mpi_tx.u64);
+
+ octeon_spi_wait_ready(p);
+ if (rx_buf)
+ for (i = 0; i < len; i++) {
+ u64 v = cvmx_read_csr(p->register_base + OCTEON_SPI_DAT0 + (8 * i));
+ *rx_buf++ = (u8)v;
+ }
+
+ if (xfer->delay_usecs)
+ udelay(xfer->delay_usecs);
+
+ return xfer->len;
+}
+
+static int octeon_spi_validate_bpw(struct spi_device *spi, u32 speed)
+{
+ switch (speed) {
+ case 8:
+ break;
+ default:
+ dev_err(&spi->dev, "Error: %d bits per word not supported\n",
+ speed);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int octeon_spi_transfer_one_message(struct spi_master *master,
+ struct spi_message *msg)
+{
+ struct octeon_spi *p = spi_master_get_devdata(master);
+ unsigned int total_len = 0;
+ int status = 0;
+ struct spi_transfer *xfer;
+
+ /*
+ * We better have set the configuration via a call to .setup
+ * before we get here.
+ */
+ if (spi_get_ctldata(msg->spi) == NULL) {
+ status = -EINVAL;
+ goto err;
+ }
+
+ list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+ if (xfer->bits_per_word) {
+ status = octeon_spi_validate_bpw(msg->spi,
+ xfer->bits_per_word);
+ if (status)
+ goto err;
+ }
+ }
+
+ list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+ bool last_xfer = &xfer->transfer_list == msg->transfers.prev;
+ int r = octeon_spi_do_transfer(p, msg, xfer, last_xfer);
+ if (r < 0) {
+ status = r;
+ goto err;
+ }
+ total_len += r;
+ }
+err:
+ msg->status = status;
+ msg->actual_length = total_len;
+ spi_finalize_current_message(master);
+ return status;
+}
+
+static struct octeon_spi_setup *octeon_spi_new_setup(struct spi_device *spi)
+{
+ struct octeon_spi_setup *setup = kzalloc(sizeof(*setup), GFP_KERNEL);
+ if (!setup)
+ return NULL;
+
+ setup->max_speed_hz = spi->max_speed_hz;
+ setup->chip_select = spi->chip_select;
+ setup->mode = spi->mode;
+ setup->bits_per_word = spi->bits_per_word;
+ return setup;
+}
+
+static int octeon_spi_setup(struct spi_device *spi)
+{
+ int r;
+ struct octeon_spi_setup *new_setup;
+ struct octeon_spi_setup *old_setup = spi_get_ctldata(spi);
+
+ r = octeon_spi_validate_bpw(spi, spi->bits_per_word);
+ if (r)
+ return r;
+
+ new_setup = octeon_spi_new_setup(spi);
+ if (!new_setup)
+ return -ENOMEM;
+
+ spi_set_ctldata(spi, new_setup);
+ kfree(old_setup);
+
+ return 0;
+}
+
+static void octeon_spi_cleanup(struct spi_device *spi)
+{
+ struct octeon_spi_setup *old_setup = spi_get_ctldata(spi);
+ spi_set_ctldata(spi, NULL);
+ kfree(old_setup);
+}
+
+static int octeon_spi_nop_transfer_hardware(struct spi_master *master)
+{
+ return 0;
+}
+
+static int __devinit octeon_spi_probe(struct platform_device *pdev)
+{
+
+ struct resource *res_mem;
+ struct spi_master *master;
+ struct octeon_spi *p;
+ int err = -ENOENT;
+
+ master = spi_alloc_master(&pdev->dev, sizeof(struct octeon_spi));
+ if (!master)
+ return -ENOMEM;
+ p = spi_master_get_devdata(master);
+ platform_set_drvdata(pdev, p);
+ p->my_master = master;
+
+ res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ if (res_mem == NULL) {
+ dev_err(&pdev->dev, "found no memory resource\n");
+ err = -ENXIO;
+ goto fail;
+ }
+ if (!devm_request_mem_region(&pdev->dev, res_mem->start,
+ resource_size(res_mem), res_mem->name)) {
+ dev_err(&pdev->dev, "request_mem_region failed\n");
+ goto fail;
+ }
+ p->register_base = (u64)devm_ioremap(&pdev->dev, res_mem->start,
+ resource_size(res_mem));
+
+ /* Dynamic bus numbering */
+ master->bus_num = -1;
+ master->num_chipselect = 4;
+ master->mode_bits = SPI_CPHA |
+ SPI_CPOL |
+ SPI_CS_HIGH |
+ SPI_LSB_FIRST |
+ SPI_3WIRE;
+
+ master->setup = octeon_spi_setup;
+ master->cleanup = octeon_spi_cleanup;
+ master->prepare_transfer_hardware = octeon_spi_nop_transfer_hardware;
+ master->transfer_one_message = octeon_spi_transfer_one_message;
+ master->unprepare_transfer_hardware = octeon_spi_nop_transfer_hardware;
+
+ master->dev.of_node = pdev->dev.of_node;
+ err = spi_register_master(master);
+ if (err) {
+ dev_err(&pdev->dev, "register master failed: %d\n", err);
+ goto fail;
+ }
+
+ dev_info(&pdev->dev, "Version " DRV_VERSION "\n");
+
+ return 0;
+fail:
+ spi_master_put(master);
+ return err;
+}
+
+static int __devexit octeon_spi_remove(struct platform_device *pdev)
+{
+ struct octeon_spi *p = platform_get_drvdata(pdev);
+ struct spi_master *master = p->my_master;
+
+ spi_unregister_master(master);
+
+ /* Clear the CSENA* and put everything in a known state. */
+ cvmx_write_csr(p->register_base + OCTEON_SPI_CFG, 0);
+ spi_master_put(master);
+ return 0;
+}
+
+static struct of_device_id octeon_spi_match[] = {
+ {
+ .compatible = "cavium,octeon-3010-spi",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, octeon_spi_match);
+
+static struct platform_driver octeon_spi_driver = {
+ .driver = {
+ .name = "spi-octeon",
+ .owner = THIS_MODULE,
+ .of_match_table = octeon_spi_match,
+ },
+ .probe = octeon_spi_probe,
+ .remove = __exit_p(octeon_spi_remove),
+};
+
+module_platform_driver(octeon_spi_driver);
+
+MODULE_DESCRIPTION(DRV_DESCRIPTION);
+MODULE_VERSION(DRV_VERSION);
+MODULE_AUTHOR("David Daney");
+MODULE_LICENSE("GPL");
--
1.7.2.3
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
2012-05-11 21:34 ` David Daney
(?)
@ 2012-05-14 5:46 ` Shubhrajyoti Datta
2012-05-14 18:13 ` David Daney
-1 siblings, 1 reply; 28+ messages in thread
From: Shubhrajyoti Datta @ 2012-05-14 5:46 UTC (permalink / raw)
To: David Daney
Cc: devicetree-discuss, Grant Likely, Rob Herring, spi-devel-general,
linux-mips, David Daney, linux-kernel, linux-doc
Hi David,
A few comments.
On Sat, May 12, 2012 at 3:04 AM, David Daney <ddaney.cavm@gmail.com> wrote:
> From: David Daney <david.daney@cavium.com>
>
> Add the driver, link it into the kbuild system and provide device tree
> binding documentation.
>
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
> .../devicetree/bindings/spi/spi-octeon.txt | 33 ++
> drivers/spi/Kconfig | 7 +
> drivers/spi/Makefile | 1 +
> drivers/spi/spi-octeon.c | 369 ++++++++++++++++++++
> 4 files changed, 410 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/spi/spi-octeon.txt
> create mode 100644 drivers/spi/spi-octeon.c
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-octeon.txt b/Documentation/devicetree/bindings/spi/spi-octeon.txt
> new file mode 100644
> index 0000000..431add1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-octeon.txt
> @@ -0,0 +1,33 @@
> +Cavium, Inc. OCTEON SOC SPI master controller.
> +
> +Required properties:
> +- compatible : "cavium,octeon-3010-spi"
> +- reg : The register base for the controller.
> +- interrupts : One interrupt, used by the controller.
> +- #address-cells : <1>, as required by generic SPI binding.
> +- #size-cells : <0>, also as required by generic SPI binding.
> +
> +Child nodes as per the generic SPI binding.
> +
> +Example:
> +
> + spi@1070000001000 {
> + compatible = "cavium,octeon-3010-spi";
> + reg = <0x10700 0x00001000 0x0 0x100>;
> + interrupts = <0 58>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + eeprom@0 {
> + compatible = "st,m95256", "atmel,at25";
> + reg = <0>;
> + spi-max-frequency = <5000000>;
> + spi-cpha;
> + spi-cpol;
> +
> + pagesize = <64>;
> + size = <32768>;
> + address-width = <16>;
> + };
> + };
> +
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 00c0240..e1dd0d0 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -228,6 +228,13 @@ config SPI_OC_TINY
> help
> This is the driver for OpenCores tiny SPI master controller.
>
> +config SPI_OCTEON
> + tristate "Cavium OCTEON SPI controller"
> + depends on CPU_CAVIUM_OCTEON
> + help
> + SPI host driver for the hardware found on some Cavium OCTEON
> + SOCs.
> +
> config SPI_OMAP_UWIRE
> tristate "OMAP1 MicroWire"
> depends on ARCH_OMAP1
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 9d75d21..c7f8b71 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC) += spi-mpc52xx-psc.o
> obj-$(CONFIG_SPI_MPC52xx) += spi-mpc52xx.o
> obj-$(CONFIG_SPI_NUC900) += spi-nuc900.o
> obj-$(CONFIG_SPI_OC_TINY) += spi-oc-tiny.o
> +obj-$(CONFIG_SPI_OCTEON) += spi-octeon.o
> obj-$(CONFIG_SPI_OMAP_UWIRE) += spi-omap-uwire.o
> obj-$(CONFIG_SPI_OMAP_100K) += spi-omap-100k.o
> obj-$(CONFIG_SPI_OMAP24XX) += spi-omap2-mcspi.o
> diff --git a/drivers/spi/spi-octeon.c b/drivers/spi/spi-octeon.c
> new file mode 100644
> index 0000000..7207aaf
> --- /dev/null
> +++ b/drivers/spi/spi-octeon.c
> @@ -0,0 +1,369 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2011, 2012 Cavium, Inc.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/spi/spi.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/of_spi.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +
> +#include <asm/octeon/octeon.h>
> +#include <asm/octeon/cvmx-mpi-defs.h>
> +
> +#define DRV_VERSION "2.0" /* Version 1 was the out-of-tree driver */
This could be given a miss. As it is less meaningful once accepted.
> +#define DRV_DESCRIPTION "Cavium, Inc. OCTEON SPI bus driver"
> +
> +
> +#define OCTEON_SPI_CFG 0
> +#define OCTEON_SPI_STS 0x08
> +#define OCTEON_SPI_TX 0x10
> +#define OCTEON_SPI_DAT0 0x80
> +
> +#define OCTEON_SPI_MAX_BYTES 9
> +
> +#define OCTEON_SPI_MAX_CLOCK_HZ 16000000
> +
> +struct octeon_spi {
> + struct spi_master *my_master;
> + u64 register_base;
> + u64 last_cfg;
> + u64 cs_enax;
> +};
> +
> +struct octeon_spi_setup {
> + u32 max_speed_hz;
> + u8 chip_select;
> + u8 mode;
> + u8 bits_per_word;
> +};
> +
> +static void octeon_spi_wait_ready(struct octeon_spi *p)
> +{
> + union cvmx_mpi_sts mpi_sts;
> + unsigned int loops = 0;
> +
> + do {
> + if (loops++)
> + __delay(500);
Could we allow have a non-busy loop here?
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
2012-05-14 5:46 ` Shubhrajyoti Datta
@ 2012-05-14 18:13 ` David Daney
2012-05-20 5:26 ` Grant Likely
0 siblings, 1 reply; 28+ messages in thread
From: David Daney @ 2012-05-14 18:13 UTC (permalink / raw)
To: Shubhrajyoti Datta
Cc: David Daney, devicetree-discuss, Grant Likely, Rob Herring,
spi-devel-general, linux-mips, linux-kernel, linux-doc
On 05/13/2012 10:46 PM, Shubhrajyoti Datta wrote:
> Hi David,
> A few comments.
>
> On Sat, May 12, 2012 at 3:04 AM, David Daney<ddaney.cavm@gmail.com> wrote:
[...]
>> +
>> +#define DRV_VERSION "2.0" /* Version 1 was the out-of-tree driver */
> This could be given a miss. As it is less meaningful once accepted.
Well, this leads to the question, what is the purpose of the
'MODULE_VERSION()' macro? If I use that, I need to populate it with a
value.
>
[...]
>> +static void octeon_spi_wait_ready(struct octeon_spi *p)
>> +{
>> + union cvmx_mpi_sts mpi_sts;
>> + unsigned int loops = 0;
>> +
>> + do {
>> + if (loops++)
>> + __delay(500);
> Could we allow have a non-busy loop here?
>
We could, but I thought about it and chose not to.
The SPI hardware can queue a maximum of 9 bytes (72 bits) before
software has to take action. That works out to 3.6 uS at a 20MHz clock
rate. Sleeping, scheduling to a different task, taking an interrupt and
then switching back to this task will likely not be faster than that.
At lower clock rates, it would make more sense, but it adds complexity
to the driver. We can always revisit this decision if it proves to be a
problem.
David Daney
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-05-20 5:26 ` Grant Likely
0 siblings, 0 replies; 28+ messages in thread
From: Grant Likely @ 2012-05-20 5:26 UTC (permalink / raw)
To: David Daney, Shubhrajyoti Datta
Cc: David Daney, devicetree-discuss, Rob Herring, spi-devel-general,
linux-mips, linux-kernel, linux-doc
On Mon, 14 May 2012 11:13:41 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> On 05/13/2012 10:46 PM, Shubhrajyoti Datta wrote:
> > Hi David,
> > A few comments.
> >
> > On Sat, May 12, 2012 at 3:04 AM, David Daney<ddaney.cavm@gmail.com> wrote:
> [...]
>
> >> +
> >> +#define DRV_VERSION "2.0" /* Version 1 was the out-of-tree driver */
> > This could be given a miss. As it is less meaningful once accepted.
>
>
> Well, this leads to the question, what is the purpose of the
> 'MODULE_VERSION()' macro? If I use that, I need to populate it with a
> value.
Don't use MODULE_VERSION. Just because it is there doesn't mean you
need to use it. :-) You'll notice that none of the other spi drivers
have it.
g.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-05-20 5:26 ` Grant Likely
0 siblings, 0 replies; 28+ messages in thread
From: Grant Likely @ 2012-05-20 5:26 UTC (permalink / raw)
To: Shubhrajyoti Datta
Cc: David Daney, devicetree-discuss, Rob Herring, spi-devel-general,
linux-mips, linux-kernel, linux-doc
On Mon, 14 May 2012 11:13:41 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> On 05/13/2012 10:46 PM, Shubhrajyoti Datta wrote:
> > Hi David,
> > A few comments.
> >
> > On Sat, May 12, 2012 at 3:04 AM, David Daney<ddaney.cavm@gmail.com> wrote:
> [...]
>
> >> +
> >> +#define DRV_VERSION "2.0" /* Version 1 was the out-of-tree driver */
> > This could be given a miss. As it is less meaningful once accepted.
>
>
> Well, this leads to the question, what is the purpose of the
> 'MODULE_VERSION()' macro? If I use that, I need to populate it with a
> value.
Don't use MODULE_VERSION. Just because it is there doesn't mean you
need to use it. :-) You'll notice that none of the other spi drivers
have it.
g.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-05-20 5:26 ` Grant Likely
0 siblings, 0 replies; 28+ messages in thread
From: Grant Likely @ 2012-05-20 5:26 UTC (permalink / raw)
To: David Daney, Shubhrajyoti Datta
Cc: devicetree-discuss, Rob Herring, spi-devel-general, linux-mips,
linux-kernel, linux-doc
On Mon, 14 May 2012 11:13:41 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> On 05/13/2012 10:46 PM, Shubhrajyoti Datta wrote:
> > Hi David,
> > A few comments.
> >
> > On Sat, May 12, 2012 at 3:04 AM, David Daney<ddaney.cavm@gmail.com> wrote:
> [...]
>
> >> +
> >> +#define DRV_VERSION "2.0" /* Version 1 was the out-of-tree driver */
> > This could be given a miss. As it is less meaningful once accepted.
>
>
> Well, this leads to the question, what is the purpose of the
> 'MODULE_VERSION()' macro? If I use that, I need to populate it with a
> value.
Don't use MODULE_VERSION. Just because it is there doesn't mean you
need to use it. :-) You'll notice that none of the other spi drivers
have it.
g.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-05-14 20:07 ` Linus Walleij
0 siblings, 0 replies; 28+ messages in thread
From: Linus Walleij @ 2012-05-14 20:07 UTC (permalink / raw)
To: David Daney
Cc: devicetree-discuss, Grant Likely, Rob Herring, spi-devel-general,
linux-mips, David Daney, linux-kernel, linux-doc
On Fri, May 11, 2012 at 11:34 PM, David Daney <ddaney.cavm@gmail.com> wrote:
> + mpi_cfg.u64 = 0;
> + mpi_cfg.u64 |= p->cs_enax;
> + if (mpi_cfg.u64 != p->last_cfg) {
But now I see why this 64bit is so clever. Forget the comment on 1/2!
This has a certain elegance to it that I just learned to appreciate.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-05-14 20:07 ` Linus Walleij
0 siblings, 0 replies; 28+ messages in thread
From: Linus Walleij @ 2012-05-14 20:07 UTC (permalink / raw)
To: David Daney
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, David Daney,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
On Fri, May 11, 2012 at 11:34 PM, David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> + mpi_cfg.u64 = 0;
> + mpi_cfg.u64 |= p->cs_enax;
> + if (mpi_cfg.u64 != p->last_cfg) {
But now I see why this 64bit is so clever. Forget the comment on 1/2!
This has a certain elegance to it that I just learned to appreciate.
Yours,
Linus Walleij
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
2012-05-11 21:34 ` David Daney
` (2 preceding siblings ...)
(?)
@ 2012-05-20 5:46 ` Grant Likely
2012-08-21 19:30 ` David Daney
-1 siblings, 1 reply; 28+ messages in thread
From: Grant Likely @ 2012-05-20 5:46 UTC (permalink / raw)
To: David Daney, devicetree-discuss, Rob Herring, spi-devel-general
Cc: linux-kernel, linux-mips, linux-doc, David Daney
On Fri, 11 May 2012 14:34:46 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> From: David Daney <david.daney@cavium.com>
>
> Add the driver, link it into the kbuild system and provide device tree
> binding documentation.
>
> Signed-off-by: David Daney <david.daney@cavium.com>
Some comments below, but you can add my a-b:
Acked-by: Grant Likely <grant.likely@secretlab.ca>
> +#include <asm/octeon/octeon.h>
> +#include <asm/octeon/cvmx-mpi-defs.h>
> +
> +#define DRV_VERSION "2.0" /* Version 1 was the out-of-tree driver */
As already discussed, drop this line.
> +#define DRV_DESCRIPTION "Cavium, Inc. OCTEON SPI bus driver"
Used exactly once. Drop this line and move string to the
MODULE_DESCRIPTION().
> +static int __devinit octeon_spi_probe(struct platform_device *pdev)
> +{
> +
> + struct resource *res_mem;
> + struct spi_master *master;
> + struct octeon_spi *p;
> + int err = -ENOENT;
> +
> + master = spi_alloc_master(&pdev->dev, sizeof(struct octeon_spi));
> + if (!master)
> + return -ENOMEM;
> + p = spi_master_get_devdata(master);
> + platform_set_drvdata(pdev, p);
> + p->my_master = master;
> +
> + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + if (res_mem == NULL) {
> + dev_err(&pdev->dev, "found no memory resource\n");
> + err = -ENXIO;
> + goto fail;
> + }
> + if (!devm_request_mem_region(&pdev->dev, res_mem->start,
> + resource_size(res_mem), res_mem->name)) {
> + dev_err(&pdev->dev, "request_mem_region failed\n");
> + goto fail;
> + }
> + p->register_base = (u64)devm_ioremap(&pdev->dev, res_mem->start,
> + resource_size(res_mem));
Nasty cast. p->register_base needs to be an __iomem pointer
variable. The fact taht cvmx_read_csr accepts a uint64_t instead of
an __iomem pointer looks really wrong. Why is it written that way?
> +
> + /* Dynamic bus numbering */
> + master->bus_num = -1;
> + master->num_chipselect = 4;
> + master->mode_bits = SPI_CPHA |
> + SPI_CPOL |
> + SPI_CS_HIGH |
> + SPI_LSB_FIRST |
> + SPI_3WIRE;
> +
> + master->setup = octeon_spi_setup;
> + master->cleanup = octeon_spi_cleanup;
> + master->prepare_transfer_hardware = octeon_spi_nop_transfer_hardware;
> + master->transfer_one_message = octeon_spi_transfer_one_message;
> + master->unprepare_transfer_hardware = octeon_spi_nop_transfer_hardware;
> +
> + master->dev.of_node = pdev->dev.of_node;
> + err = spi_register_master(master);
> + if (err) {
> + dev_err(&pdev->dev, "register master failed: %d\n", err);
> + goto fail;
> + }
> +
> + dev_info(&pdev->dev, "Version " DRV_VERSION "\n");
> +
> + return 0;
> +fail:
> + spi_master_put(master);
> + return err;
> +}
> +
> +static int __devexit octeon_spi_remove(struct platform_device *pdev)
> +{
> + struct octeon_spi *p = platform_get_drvdata(pdev);
> + struct spi_master *master = p->my_master;
> +
> + spi_unregister_master(master);
> +
> + /* Clear the CSENA* and put everything in a known state. */
> + cvmx_write_csr(p->register_base + OCTEON_SPI_CFG, 0);
> + spi_master_put(master);
> + return 0;
> +}
> +
> +static struct of_device_id octeon_spi_match[] = {
> + {
> + .compatible = "cavium,octeon-3010-spi",
> + },
> + {},
Nitpick:
{ .compatible = "cavium,octeon-3010-spi", },
{},
No need for the extra lines when it is so short.
> +};
> +MODULE_DEVICE_TABLE(of, octeon_spi_match);
> +
> +static struct platform_driver octeon_spi_driver = {
> + .driver = {
> + .name = "spi-octeon",
> + .owner = THIS_MODULE,
> + .of_match_table = octeon_spi_match,
> + },
> + .probe = octeon_spi_probe,
> + .remove = __exit_p(octeon_spi_remove),
__devexit_p
> +};
> +
> +module_platform_driver(octeon_spi_driver);
> +
> +MODULE_DESCRIPTION(DRV_DESCRIPTION);
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_AUTHOR("David Daney");
> +MODULE_LICENSE("GPL");
> --
> 1.7.2.3
>
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
2012-05-20 5:46 ` Grant Likely
@ 2012-08-21 19:30 ` David Daney
0 siblings, 0 replies; 28+ messages in thread
From: David Daney @ 2012-08-21 19:30 UTC (permalink / raw)
To: Grant Likely
Cc: devicetree-discuss, Rob Herring, spi-devel-general, linux-kernel,
linux-mips, David Daney
On 05/19/2012 10:46 PM, Grant Likely wrote:
> On Fri, 11 May 2012 14:34:46 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> Add the driver, link it into the kbuild system and provide device tree
>> binding documentation.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>
> Some comments below, but you can add my a-b:
>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>
[...]
>> + p->register_base = (u64)devm_ioremap(&pdev->dev, res_mem->start,
>> + resource_size(res_mem));
>
> Nasty cast. p->register_base needs to be an __iomem pointer
> variable.
No, it is only ever used as an argument to cvmx_{read,write}_csr(),
which want the u64 type.
> The fact taht cvmx_read_csr accepts a uint64_t instead of
> an __iomem pointer looks really wrong. Why is it written that way?
Register addresses on OCTEON are 64-bits wide. In a 32-bit kernel,
pointers are only 32-bits wide. Thus was born the cvmx_read_csr()
function that takes a u64 address.
We no longer support 32-bit kernels, but the legacy of the interface
lives on.
David Daney
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-08-21 19:30 ` David Daney
0 siblings, 0 replies; 28+ messages in thread
From: David Daney @ 2012-08-21 19:30 UTC (permalink / raw)
To: Grant Likely
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, David Daney,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
On 05/19/2012 10:46 PM, Grant Likely wrote:
> On Fri, 11 May 2012 14:34:46 -0700, David Daney <ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>
>> Add the driver, link it into the kbuild system and provide device tree
>> binding documentation.
>>
>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>
> Some comments below, but you can add my a-b:
>
> Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>
[...]
>> + p->register_base = (u64)devm_ioremap(&pdev->dev, res_mem->start,
>> + resource_size(res_mem));
>
> Nasty cast. p->register_base needs to be an __iomem pointer
> variable.
No, it is only ever used as an argument to cvmx_{read,write}_csr(),
which want the u64 type.
> The fact taht cvmx_read_csr accepts a uint64_t instead of
> an __iomem pointer looks really wrong. Why is it written that way?
Register addresses on OCTEON are 64-bits wide. In a 32-bit kernel,
pointers are only 32-bits wide. Thus was born the cvmx_read_csr()
function that takes a u64 address.
We no longer support 32-bit kernels, but the legacy of the interface
lives on.
David Daney
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
2012-05-11 21:34 ` David Daney
` (3 preceding siblings ...)
(?)
@ 2012-08-21 18:23 ` John Crispin
2012-08-21 18:33 ` David Daney
-1 siblings, 1 reply; 28+ messages in thread
From: John Crispin @ 2012-08-21 18:23 UTC (permalink / raw)
To: David Daney; +Cc: linux-mips
On 11/05/12 23:34, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
>
> Add the driver, link it into the kbuild system and provide device tree
> binding documentation.
>
> Signed-off-by: David Daney <david.daney@cavium.com>
Thanks, queued for 3.7.
John
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
2012-08-21 18:23 ` John Crispin
@ 2012-08-21 18:33 ` David Daney
2012-08-21 18:35 ` John Crispin
0 siblings, 1 reply; 28+ messages in thread
From: David Daney @ 2012-08-21 18:33 UTC (permalink / raw)
To: John Crispin, Ralf Baechle; +Cc: linux-mips
On 08/21/2012 11:23 AM, John Crispin wrote:
> On 11/05/12 23:34, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> Add the driver, link it into the kbuild system and provide device tree
>> binding documentation.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>
> Thanks, queued for 3.7.
>
This cannot go in like this.
There were a bunch of requests by other maintainers that were never
done. Also since it is in a foreign subsystem, it at a minimum needs
some additional Acked-bys
David Daney
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
2012-08-21 18:33 ` David Daney
@ 2012-08-21 18:35 ` John Crispin
2012-08-21 18:40 ` David Daney
0 siblings, 1 reply; 28+ messages in thread
From: John Crispin @ 2012-08-21 18:35 UTC (permalink / raw)
To: David Daney; +Cc: Ralf Baechle, linux-mips
On 21/08/12 20:33, David Daney wrote:
> On 08/21/2012 11:23 AM, John Crispin wrote:
>> On 11/05/12 23:34, David Daney wrote:
>>> From: David Daney <david.daney@cavium.com>
>>>
>>> Add the driver, link it into the kbuild system and provide device tree
>>> binding documentation.
>>>
>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>
>> Thanks, queued for 3.7.
>>
>
> This cannot go in like this.
>
> There were a bunch of requests by other maintainers that were never
> done. Also since it is in a foreign subsystem, it at a minimum needs
> some additional Acked-bys
hmmm ... you are right about the Acked-by ... i forgot to merge add it ...
i did ask you about this on IRC and you said the driver was ok to be
merged like this. inside the thread linus wllej at some point says that
his comment was bogus if i am not mistaken ?
John
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
2012-08-21 18:35 ` John Crispin
@ 2012-08-21 18:40 ` David Daney
2012-08-21 18:40 ` John Crispin
0 siblings, 1 reply; 28+ messages in thread
From: David Daney @ 2012-08-21 18:40 UTC (permalink / raw)
To: John Crispin; +Cc: Ralf Baechle, linux-mips
On 08/21/2012 11:35 AM, John Crispin wrote:
> On 21/08/12 20:33, David Daney wrote:
>> On 08/21/2012 11:23 AM, John Crispin wrote:
>>> On 11/05/12 23:34, David Daney wrote:
>>>> From: David Daney <david.daney@cavium.com>
>>>>
>>>> Add the driver, link it into the kbuild system and provide device tree
>>>> binding documentation.
>>>>
>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>
>>> Thanks, queued for 3.7.
>>>
>>
>> This cannot go in like this.
>>
>> There were a bunch of requests by other maintainers that were never
>> done. Also since it is in a foreign subsystem, it at a minimum needs
>> some additional Acked-bys
>
> hmmm ... you are right about the Acked-by ... i forgot to merge add it ...
>
> i did ask you about this on IRC and you said the driver was ok to be
> merged like this. inside the thread linus wllej at some point says that
> his comment was bogus if i am not mistaken ?
OK, I think I won't answer this type of question on IRC in the future.
I really should have looked at it in more detail. But at the end of the
day, I need to fix it up before it can go in.
David Daney
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/2] spi: Add SPI master controller for OCTEON SOCs.
2012-08-21 18:40 ` David Daney
@ 2012-08-21 18:40 ` John Crispin
0 siblings, 0 replies; 28+ messages in thread
From: John Crispin @ 2012-08-21 18:40 UTC (permalink / raw)
To: linux-mips
On 21/08/12 20:40, David Daney wrote:
> On 08/21/2012 11:35 AM, John Crispin wrote:
>> On 21/08/12 20:33, David Daney wrote:
>>> On 08/21/2012 11:23 AM, John Crispin wrote:
>>>> On 11/05/12 23:34, David Daney wrote:
>>>>> From: David Daney <david.daney@cavium.com>
>>>>>
>>>>> Add the driver, link it into the kbuild system and provide device tree
>>>>> binding documentation.
>>>>>
>>>>> Signed-off-by: David Daney <david.daney@cavium.com>
>>>>
>>>> Thanks, queued for 3.7.
>>>>
>>>
>>> This cannot go in like this.
>>>
>>> There were a bunch of requests by other maintainers that were never
>>> done. Also since it is in a foreign subsystem, it at a minimum needs
>>> some additional Acked-bys
>>
>> hmmm ... you are right about the Acked-by ... i forgot to merge add it
>> ...
>>
>> i did ask you about this on IRC and you said the driver was ok to be
>> merged like this. inside the thread linus wllej at some point says that
>> his comment was bogus if i am not mistaken ?
>
> OK, I think I won't answer this type of question on IRC in the future. I
> really should have looked at it in more detail. But at the end of the
> day, I need to fix it up before it can go in.
>
> David Daney
i agree, i just removed it from the queue ...
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-08-21 19:49 ` Guenter Roeck
0 siblings, 0 replies; 28+ messages in thread
From: Guenter Roeck @ 2012-08-21 19:49 UTC (permalink / raw)
To: David Daney
Cc: devicetree-discuss, Grant Likely, Rob Herring, spi-devel-general,
linux-kernel, linux-mips, linux-doc, David Daney
On Fri, May 11, 2012 at 08:34:46PM -0000, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
>
> Add the driver, link it into the kbuild system and provide device tree
> binding documentation.
>
> Signed-off-by: David Daney <david.daney@cavium.com>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>
[ ... ]
> +
> +static int __devexit octeon_spi_remove(struct platform_device *pdev)
> +{
> + struct octeon_spi *p = platform_get_drvdata(pdev);
> + struct spi_master *master = p->my_master;
> +
> + spi_unregister_master(master);
> +
I know it is a bit late, but ...
The call to spi_unregister_master() frees the memory associated with master,
ie 'p', and the spi_master_put() below without matching spi_master_get() is
unnecessary/wrong. One possible fix would be to use
struct spi_master *master = spi_master_get(p->my_master);
above. That protects master and p while it is still being used, and makes use
of the call to spi_master_put() below. Another option might be to move
cvmx_write_csr() ahead of the call to spi_unregister_master() and drop the
call to spi_master_put().
Guenter
> + /* Clear the CSENA* and put everything in a known state. */
> + cvmx_write_csr(p->register_base + OCTEON_SPI_CFG, 0);
> + spi_master_put(master);
> + return 0;
> +}
> +
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-08-21 19:49 ` Guenter Roeck
0 siblings, 0 replies; 28+ messages in thread
From: Guenter Roeck @ 2012-08-21 19:49 UTC (permalink / raw)
To: David Daney
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, David Daney,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
On Fri, May 11, 2012 at 08:34:46PM -0000, David Daney wrote:
> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>
> Add the driver, link it into the kbuild system and provide device tree
> binding documentation.
>
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>
[ ... ]
> +
> +static int __devexit octeon_spi_remove(struct platform_device *pdev)
> +{
> + struct octeon_spi *p = platform_get_drvdata(pdev);
> + struct spi_master *master = p->my_master;
> +
> + spi_unregister_master(master);
> +
I know it is a bit late, but ...
The call to spi_unregister_master() frees the memory associated with master,
ie 'p', and the spi_master_put() below without matching spi_master_get() is
unnecessary/wrong. One possible fix would be to use
struct spi_master *master = spi_master_get(p->my_master);
above. That protects master and p while it is still being used, and makes use
of the call to spi_master_put() below. Another option might be to move
cvmx_write_csr() ahead of the call to spi_unregister_master() and drop the
call to spi_master_put().
Guenter
> + /* Clear the CSENA* and put everything in a known state. */
> + cvmx_write_csr(p->register_base + OCTEON_SPI_CFG, 0);
> + spi_master_put(master);
> + return 0;
> +}
> +
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-08-21 20:38 ` David Daney
0 siblings, 0 replies; 28+ messages in thread
From: David Daney @ 2012-08-21 20:38 UTC (permalink / raw)
To: Guenter Roeck
Cc: David Daney, devicetree-discuss, Grant Likely, Rob Herring,
spi-devel-general, linux-kernel, linux-mips, linux-doc,
David Daney
On 08/21/2012 12:49 PM, Guenter Roeck wrote:
> On Fri, May 11, 2012 at 08:34:46PM -0000, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> Add the driver, link it into the kbuild system and provide device tree
>> binding documentation.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>>
> [ ... ]
>
>> +
>> +static int __devexit octeon_spi_remove(struct platform_device *pdev)
>> +{
>> + struct octeon_spi *p = platform_get_drvdata(pdev);
>> + struct spi_master *master = p->my_master;
>> +
>> + spi_unregister_master(master);
>> +
>
> I know it is a bit late, but ...
In this case, just in time. I am now finally getting back to fixing the
issues with this driver, and looking to merging it in the near future.
David Daney
>
> The call to spi_unregister_master() frees the memory associated with master,
> ie 'p', and the spi_master_put() below without matching spi_master_get() is
> unnecessary/wrong. One possible fix would be to use
>
> struct spi_master *master = spi_master_get(p->my_master);
>
> above. That protects master and p while it is still being used, and makes use
> of the call to spi_master_put() below. Another option might be to move
> cvmx_write_csr() ahead of the call to spi_unregister_master() and drop the
> call to spi_master_put().
>
> Guenter
>
>> + /* Clear the CSENA* and put everything in a known state. */
>> + cvmx_write_csr(p->register_base + OCTEON_SPI_CFG, 0);
>> + spi_master_put(master);
>> + return 0;
>> +}
>> +
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [2/2] spi: Add SPI master controller for OCTEON SOCs.
@ 2012-08-21 20:38 ` David Daney
0 siblings, 0 replies; 28+ messages in thread
From: David Daney @ 2012-08-21 20:38 UTC (permalink / raw)
To: Guenter Roeck
Cc: David Daney, devicetree-discuss, Grant Likely, Rob Herring,
spi-devel-general, linux-kernel, linux-mips, linux-doc,
David Daney
On 08/21/2012 12:49 PM, Guenter Roeck wrote:
> On Fri, May 11, 2012 at 08:34:46PM -0000, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> Add the driver, link it into the kbuild system and provide device tree
>> binding documentation.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> Acked-by: Grant Likely <grant.likely@secretlab.ca>
>>
> [ ... ]
>
>> +
>> +static int __devexit octeon_spi_remove(struct platform_device *pdev)
>> +{
>> + struct octeon_spi *p = platform_get_drvdata(pdev);
>> + struct spi_master *master = p->my_master;
>> +
>> + spi_unregister_master(master);
>> +
>
> I know it is a bit late, but ...
In this case, just in time. I am now finally getting back to fixing the
issues with this driver, and looking to merging it in the near future.
David Daney
>
> The call to spi_unregister_master() frees the memory associated with master,
> ie 'p', and the spi_master_put() below without matching spi_master_get() is
> unnecessary/wrong. One possible fix would be to use
>
> struct spi_master *master = spi_master_get(p->my_master);
>
> above. That protects master and p while it is still being used, and makes use
> of the call to spi_master_put() below. Another option might be to move
> cvmx_write_csr() ahead of the call to spi_unregister_master() and drop the
> call to spi_master_put().
>
> Guenter
>
>> + /* Clear the CSENA* and put everything in a known state. */
>> + cvmx_write_csr(p->register_base + OCTEON_SPI_CFG, 0);
>> + spi_master_put(master);
>> + return 0;
>> +}
>> +
>
^ permalink raw reply [flat|nested] 28+ messages in thread