[RFC 11/18] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[RFC 11/18] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[Gabor Juhos]

The Atheros AR71XX/AR724X/AR913X SoCs have a built-in SPI controller. This
patch implements a driver for that.

Signed-off-by: Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
---
 .../include/asm/mach-ath79/ath79_spi_platform.h    |   19 ++
 drivers/spi/Kconfig                                |    8 +
 drivers/spi/Makefile                               |    1 +
 drivers/spi/ath79_spi.c                            |  291 ++++++++++++++++++++
 4 files changed, 319 insertions(+), 0 deletions(-)
 create mode 100644 arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
 create mode 100644 drivers/spi/ath79_spi.c

diff --git a/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h b/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
new file mode 100644
index 0000000..aa71216
--- /dev/null
+++ b/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
@@ -0,0 +1,19 @@
+/*
+ *  Platform data definition for Atheros AR71XX/AR724X/AR913X SPI controller
+ *
+ *  Copyright (C) 2008-2010 Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms of the GNU General Public License version 2 as published
+ *  by the Free Software Foundation.
+ */
+
+#ifndef _ATH79_SPI_PLATFORM_H
+#define _ATH79_SPI_PLATFORM_H
+
+struct ath79_spi_platform_data {
+	unsigned	bus_num;
+	unsigned	num_chipselect;
+};
+
+#endif /* _ATH79_SPI_PLATFORM_H */
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 78f9fd0..f2093e1 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -53,6 +53,14 @@ if SPI_MASTER
 
 comment "SPI Master Controller Drivers"
 
+config SPI_ATH79
+	tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver"
+	depends on ATH79 && GENERIC_GPIO
+	select SPI_BITBANG
+	help
+	  This enables support for the SPI controller present on the
+	  Atheros AR71XX/AR724X/AR913X SoCs.
+
 config SPI_ATMEL
 	tristate "Atmel SPI Controller"
 	depends on (ARCH_AT91 || AVR32)
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 8bc1a5a..875bc3d 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_SPI_MASTER)		+= spi.o
 
 # SPI master controller drivers (bus)
 obj-$(CONFIG_SPI_ATMEL)			+= atmel_spi.o
+obj-$(CONFIG_SPI_ATH79)			+= ath79_spi.o
 obj-$(CONFIG_SPI_BFIN)			+= spi_bfin5xx.o
 obj-$(CONFIG_SPI_BITBANG)		+= spi_bitbang.o
 obj-$(CONFIG_SPI_AU1550)		+= au1550_spi.o
diff --git a/drivers/spi/ath79_spi.c b/drivers/spi/ath79_spi.c
new file mode 100644
index 0000000..9b9f9cf
--- /dev/null
+++ b/drivers/spi/ath79_spi.c
@@ -0,0 +1,291 @@
+/*
+ * SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs
+ *
+ * Copyright (C) 2009-2010 Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
+ *
+ * This driver has been based on the spi-gpio.c:
+ *	Copyright (C) 2006,2008 David Brownell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_bitbang.h>
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+
+#include <asm/mach-ath79/ar71xx_regs.h>
+#include <asm/mach-ath79/ath79_spi_platform.h>
+
+#define DRV_DESC	"SPI controller driver for Atheros AR71XX/AR724X/AR91X"
+#define DRV_NAME	"ath79-spi"
+
+struct ath79_spi {
+	struct	spi_bitbang	bitbang;
+	u32			ioc_base;
+	u32			reg_ctrl;
+
+	void __iomem		*base;
+
+	struct platform_device	*pdev;
+};
+
+static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
+{
+	return __raw_readl(sp->base + reg);
+}
+
+static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
+{
+	__raw_writel(val, sp->base + reg);
+}
+
+static inline struct ath79_spi *spidev_to_sp(struct spi_device *spi)
+{
+	return spi_master_get_devdata(spi->master);
+}
+
+static void ath79_spi_chipselect(struct spi_device *spi, int is_active)
+{
+	struct ath79_spi *sp = spidev_to_sp(spi);
+	int cs_high = (spi->mode & SPI_CS_HIGH) ? is_active : !is_active;
+
+	if (is_active) {
+		/* set initial clock polarity */
+		if (spi->mode & SPI_CPOL)
+			sp->ioc_base |= AR71XX_SPI_IOC_CLK;
+		else
+			sp->ioc_base &= ~AR71XX_SPI_IOC_CLK;
+
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
+	}
+
+	if (spi->chip_select) {
+		unsigned long gpio = (unsigned long) spi->controller_data;
+
+		/* SPI is normally active-low */
+		gpio_set_value(gpio, cs_high);
+	} else {
+		if (cs_high)
+			sp->ioc_base |= AR71XX_SPI_IOC_CS0;
+		else
+			sp->ioc_base &= ~AR71XX_SPI_IOC_CS0;
+
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
+	}
+
+}
+
+static int ath79_spi_setup_cs(struct spi_device *spi)
+{
+	struct ath79_spi *sp = spidev_to_sp(spi);
+
+	/* enable GPIO mode */
+	ath79_spi_wr(sp, AR71XX_SPI_REG_FS, AR71XX_SPI_FS_GPIO);
+
+	/* save CTRL register */
+	sp->reg_ctrl = ath79_spi_rr(sp, AR71XX_SPI_REG_CTRL);
+	sp->ioc_base = ath79_spi_rr(sp, AR71XX_SPI_REG_IOC);
+
+	/* TODO: setup speed? */
+	ath79_spi_wr(sp, AR71XX_SPI_REG_CTRL, 0x43);
+
+	if (spi->chip_select) {
+		unsigned long gpio = (unsigned long) spi->controller_data;
+		int status = 0;
+
+		status = gpio_request(gpio, dev_name(&spi->dev));
+		if (status)
+			return status;
+
+		status = gpio_direction_output(gpio, spi->mode & SPI_CS_HIGH);
+		if (status) {
+			gpio_free(gpio);
+			return status;
+		}
+	} else {
+		if (spi->mode & SPI_CS_HIGH)
+			sp->ioc_base |= AR71XX_SPI_IOC_CS0;
+		else
+			sp->ioc_base &= ~AR71XX_SPI_IOC_CS0;
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
+	}
+
+	return 0;
+}
+
+static void ath79_spi_cleanup_cs(struct spi_device *spi)
+{
+	struct ath79_spi *sp = spidev_to_sp(spi);
+
+	if (spi->chip_select) {
+		unsigned long gpio = (unsigned long) spi->controller_data;
+		gpio_free(gpio);
+	}
+
+	/* restore CTRL register */
+	ath79_spi_wr(sp, AR71XX_SPI_REG_CTRL, sp->reg_ctrl);
+	/* disable GPIO mode */
+	ath79_spi_wr(sp, AR71XX_SPI_REG_FS, 0);
+}
+
+static int ath79_spi_setup(struct spi_device *spi)
+{
+	int status = 0;
+
+	if (spi->bits_per_word > 32)
+		return -EINVAL;
+
+	if (!spi->controller_state) {
+		status = ath79_spi_setup_cs(spi);
+		if (status)
+			return status;
+	}
+
+	status = spi_bitbang_setup(spi);
+	if (status && !spi->controller_state)
+		ath79_spi_cleanup_cs(spi);
+
+	return status;
+}
+
+static void ath79_spi_cleanup(struct spi_device *spi)
+{
+	ath79_spi_cleanup_cs(spi);
+	spi_bitbang_cleanup(spi);
+}
+
+static u32 ath79_spi_txrx_mode0(struct spi_device *spi, unsigned nsecs,
+			       u32 word, u8 bits)
+{
+	struct ath79_spi *sp = spidev_to_sp(spi);
+	u32 ioc = sp->ioc_base;
+
+	/* clock starts at inactive polarity */
+	for (word <<= (32 - bits); likely(bits); bits--) {
+		u32 out;
+
+		if (word & (1 << 31))
+			out = ioc | AR71XX_SPI_IOC_DO;
+		else
+			out = ioc & ~AR71XX_SPI_IOC_DO;
+
+		/* setup MSB (to slave) on trailing edge */
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, out);
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, out | AR71XX_SPI_IOC_CLK);
+
+		word <<= 1;
+	}
+
+	return ath79_spi_rr(sp, AR71XX_SPI_REG_RDS);
+}
+
+static int ath79_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct ath79_spi *sp;
+	struct ath79_spi_platform_data *pdata;
+	struct resource	*r;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*sp));
+	if (master == NULL) {
+		dev_err(&pdev->dev, "failed to allocate spi master\n");
+		return -ENOMEM;
+	}
+
+	sp = spi_master_get_devdata(master);
+	platform_set_drvdata(pdev, sp);
+
+	pdata = pdev->dev.platform_data;
+
+	master->setup = ath79_spi_setup;
+	master->cleanup = ath79_spi_cleanup;
+	if (pdata) {
+		master->bus_num = pdata->bus_num;
+		master->num_chipselect = pdata->num_chipselect;
+	} else {
+		master->bus_num = 0;
+		master->num_chipselect = 1;
+	}
+
+	sp->bitbang.master = spi_master_get(master);
+	sp->bitbang.chipselect = ath79_spi_chipselect;
+	sp->bitbang.txrx_word[SPI_MODE_0] = ath79_spi_txrx_mode0;
+	sp->bitbang.setup_transfer = spi_bitbang_setup_transfer;
+	sp->bitbang.flags = SPI_CS_HIGH;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (r == NULL) {
+		ret = -ENOENT;
+		goto err_put_master;
+	}
+
+	sp->base = ioremap(r->start, r->end - r->start + 1);
+	if (!sp->base) {
+		ret = -ENXIO;
+		goto err_put_master;
+	}
+
+	ret = spi_bitbang_start(&sp->bitbang);
+	if (ret)
+		goto err_unmap;
+
+	return 0;
+
+err_unmap:
+	iounmap(sp->base);
+err_put_master:
+	platform_set_drvdata(pdev, NULL);
+	spi_master_put(sp->bitbang.master);
+
+	return ret;
+}
+
+static int ath79_spi_remove(struct platform_device *pdev)
+{
+	struct ath79_spi *sp = platform_get_drvdata(pdev);
+
+	spi_bitbang_stop(&sp->bitbang);
+	iounmap(sp->base);
+	platform_set_drvdata(pdev, NULL);
+	spi_master_put(sp->bitbang.master);
+
+	return 0;
+}
+
+static struct platform_driver ath79_spi_drv = {
+	.probe		= ath79_spi_probe,
+	.remove		= ath79_spi_remove,
+	.driver		= {
+		.name	= DRV_NAME,
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init ath79_spi_init(void)
+{
+	return platform_driver_register(&ath79_spi_drv);
+}
+module_init(ath79_spi_init);
+
+static void __exit ath79_spi_exit(void)
+{
+	platform_driver_unregister(&ath79_spi_drv);
+}
+module_exit(ath79_spi_exit);
+
+MODULE_DESCRIPTION(DRV_DESC);
+MODULE_AUTHOR("Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
1.7.2.1


------------------------------------------------------------------------------
Centralized Desktop Delivery: Dell and VMware Reference Architecture
Simplifying enterprise desktop deployment and management using
Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
client virtualization framework. Read more!
http://p.sf.net/sfu/dell-eql-dev2dev

[RFC 11/18] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[Gabor Juhos]

The Atheros AR71XX/AR724X/AR913X SoCs have a built-in SPI controller. This
patch implements a driver for that.

Signed-off-by: Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
---
Sorry for sending this twice, i forgot to add some CCs in the first round.

 .../include/asm/mach-ath79/ath79_spi_platform.h    |   19 ++
 drivers/spi/Kconfig                                |    8 +
 drivers/spi/Makefile                               |    1 +
 drivers/spi/ath79_spi.c                            |  291 ++++++++++++++++++++
 4 files changed, 319 insertions(+), 0 deletions(-)
 create mode 100644 arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
 create mode 100644 drivers/spi/ath79_spi.c

diff --git a/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h b/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
new file mode 100644
index 0000000..aa71216
--- /dev/null
+++ b/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
@@ -0,0 +1,19 @@
+/*
+ *  Platform data definition for Atheros AR71XX/AR724X/AR913X SPI controller
+ *
+ *  Copyright (C) 2008-2010 Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms of the GNU General Public License version 2 as published
+ *  by the Free Software Foundation.
+ */
+
+#ifndef _ATH79_SPI_PLATFORM_H
+#define _ATH79_SPI_PLATFORM_H
+
+struct ath79_spi_platform_data {
+	unsigned	bus_num;
+	unsigned	num_chipselect;
+};
+
+#endif /* _ATH79_SPI_PLATFORM_H */
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 78f9fd0..f2093e1 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -53,6 +53,14 @@ if SPI_MASTER
 
 comment "SPI Master Controller Drivers"
 
+config SPI_ATH79
+	tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver"
+	depends on ATH79 && GENERIC_GPIO
+	select SPI_BITBANG
+	help
+	  This enables support for the SPI controller present on the
+	  Atheros AR71XX/AR724X/AR913X SoCs.
+
 config SPI_ATMEL
 	tristate "Atmel SPI Controller"
 	depends on (ARCH_AT91 || AVR32)
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 8bc1a5a..875bc3d 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_SPI_MASTER)		+= spi.o
 
 # SPI master controller drivers (bus)
 obj-$(CONFIG_SPI_ATMEL)			+= atmel_spi.o
+obj-$(CONFIG_SPI_ATH79)			+= ath79_spi.o
 obj-$(CONFIG_SPI_BFIN)			+= spi_bfin5xx.o
 obj-$(CONFIG_SPI_BITBANG)		+= spi_bitbang.o
 obj-$(CONFIG_SPI_AU1550)		+= au1550_spi.o
diff --git a/drivers/spi/ath79_spi.c b/drivers/spi/ath79_spi.c
new file mode 100644
index 0000000..9b9f9cf
--- /dev/null
+++ b/drivers/spi/ath79_spi.c
@@ -0,0 +1,291 @@
+/*
+ * SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs
+ *
+ * Copyright (C) 2009-2010 Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
+ *
+ * This driver has been based on the spi-gpio.c:
+ *	Copyright (C) 2006,2008 David Brownell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_bitbang.h>
+#include <linux/bitops.h>
+#include <linux/gpio.h>
+
+#include <asm/mach-ath79/ar71xx_regs.h>
+#include <asm/mach-ath79/ath79_spi_platform.h>
+
+#define DRV_DESC	"SPI controller driver for Atheros AR71XX/AR724X/AR91X"
+#define DRV_NAME	"ath79-spi"
+
+struct ath79_spi {
+	struct	spi_bitbang	bitbang;
+	u32			ioc_base;
+	u32			reg_ctrl;
+
+	void __iomem		*base;
+
+	struct platform_device	*pdev;
+};
+
+static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
+{
+	return __raw_readl(sp->base + reg);
+}
+
+static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
+{
+	__raw_writel(val, sp->base + reg);
+}
+
+static inline struct ath79_spi *spidev_to_sp(struct spi_device *spi)
+{
+	return spi_master_get_devdata(spi->master);
+}
+
+static void ath79_spi_chipselect(struct spi_device *spi, int is_active)
+{
+	struct ath79_spi *sp = spidev_to_sp(spi);
+	int cs_high = (spi->mode & SPI_CS_HIGH) ? is_active : !is_active;
+
+	if (is_active) {
+		/* set initial clock polarity */
+		if (spi->mode & SPI_CPOL)
+			sp->ioc_base |= AR71XX_SPI_IOC_CLK;
+		else
+			sp->ioc_base &= ~AR71XX_SPI_IOC_CLK;
+
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
+	}
+
+	if (spi->chip_select) {
+		unsigned long gpio = (unsigned long) spi->controller_data;
+
+		/* SPI is normally active-low */
+		gpio_set_value(gpio, cs_high);
+	} else {
+		if (cs_high)
+			sp->ioc_base |= AR71XX_SPI_IOC_CS0;
+		else
+			sp->ioc_base &= ~AR71XX_SPI_IOC_CS0;
+
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
+	}
+
+}
+
+static int ath79_spi_setup_cs(struct spi_device *spi)
+{
+	struct ath79_spi *sp = spidev_to_sp(spi);
+
+	/* enable GPIO mode */
+	ath79_spi_wr(sp, AR71XX_SPI_REG_FS, AR71XX_SPI_FS_GPIO);
+
+	/* save CTRL register */
+	sp->reg_ctrl = ath79_spi_rr(sp, AR71XX_SPI_REG_CTRL);
+	sp->ioc_base = ath79_spi_rr(sp, AR71XX_SPI_REG_IOC);
+
+	/* TODO: setup speed? */
+	ath79_spi_wr(sp, AR71XX_SPI_REG_CTRL, 0x43);
+
+	if (spi->chip_select) {
+		unsigned long gpio = (unsigned long) spi->controller_data;
+		int status = 0;
+
+		status = gpio_request(gpio, dev_name(&spi->dev));
+		if (status)
+			return status;
+
+		status = gpio_direction_output(gpio, spi->mode & SPI_CS_HIGH);
+		if (status) {
+			gpio_free(gpio);
+			return status;
+		}
+	} else {
+		if (spi->mode & SPI_CS_HIGH)
+			sp->ioc_base |= AR71XX_SPI_IOC_CS0;
+		else
+			sp->ioc_base &= ~AR71XX_SPI_IOC_CS0;
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
+	}
+
+	return 0;
+}
+
+static void ath79_spi_cleanup_cs(struct spi_device *spi)
+{
+	struct ath79_spi *sp = spidev_to_sp(spi);
+
+	if (spi->chip_select) {
+		unsigned long gpio = (unsigned long) spi->controller_data;
+		gpio_free(gpio);
+	}
+
+	/* restore CTRL register */
+	ath79_spi_wr(sp, AR71XX_SPI_REG_CTRL, sp->reg_ctrl);
+	/* disable GPIO mode */
+	ath79_spi_wr(sp, AR71XX_SPI_REG_FS, 0);
+}
+
+static int ath79_spi_setup(struct spi_device *spi)
+{
+	int status = 0;
+
+	if (spi->bits_per_word > 32)
+		return -EINVAL;
+
+	if (!spi->controller_state) {
+		status = ath79_spi_setup_cs(spi);
+		if (status)
+			return status;
+	}
+
+	status = spi_bitbang_setup(spi);
+	if (status && !spi->controller_state)
+		ath79_spi_cleanup_cs(spi);
+
+	return status;
+}
+
+static void ath79_spi_cleanup(struct spi_device *spi)
+{
+	ath79_spi_cleanup_cs(spi);
+	spi_bitbang_cleanup(spi);
+}
+
+static u32 ath79_spi_txrx_mode0(struct spi_device *spi, unsigned nsecs,
+			       u32 word, u8 bits)
+{
+	struct ath79_spi *sp = spidev_to_sp(spi);
+	u32 ioc = sp->ioc_base;
+
+	/* clock starts at inactive polarity */
+	for (word <<= (32 - bits); likely(bits); bits--) {
+		u32 out;
+
+		if (word & (1 << 31))
+			out = ioc | AR71XX_SPI_IOC_DO;
+		else
+			out = ioc & ~AR71XX_SPI_IOC_DO;
+
+		/* setup MSB (to slave) on trailing edge */
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, out);
+		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, out | AR71XX_SPI_IOC_CLK);
+
+		word <<= 1;
+	}
+
+	return ath79_spi_rr(sp, AR71XX_SPI_REG_RDS);
+}
+
+static int ath79_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct ath79_spi *sp;
+	struct ath79_spi_platform_data *pdata;
+	struct resource	*r;
+	int ret;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*sp));
+	if (master == NULL) {
+		dev_err(&pdev->dev, "failed to allocate spi master\n");
+		return -ENOMEM;
+	}
+
+	sp = spi_master_get_devdata(master);
+	platform_set_drvdata(pdev, sp);
+
+	pdata = pdev->dev.platform_data;
+
+	master->setup = ath79_spi_setup;
+	master->cleanup = ath79_spi_cleanup;
+	if (pdata) {
+		master->bus_num = pdata->bus_num;
+		master->num_chipselect = pdata->num_chipselect;
+	} else {
+		master->bus_num = 0;
+		master->num_chipselect = 1;
+	}
+
+	sp->bitbang.master = spi_master_get(master);
+	sp->bitbang.chipselect = ath79_spi_chipselect;
+	sp->bitbang.txrx_word[SPI_MODE_0] = ath79_spi_txrx_mode0;
+	sp->bitbang.setup_transfer = spi_bitbang_setup_transfer;
+	sp->bitbang.flags = SPI_CS_HIGH;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (r == NULL) {
+		ret = -ENOENT;
+		goto err_put_master;
+	}
+
+	sp->base = ioremap(r->start, r->end - r->start + 1);
+	if (!sp->base) {
+		ret = -ENXIO;
+		goto err_put_master;
+	}
+
+	ret = spi_bitbang_start(&sp->bitbang);
+	if (ret)
+		goto err_unmap;
+
+	return 0;
+
+err_unmap:
+	iounmap(sp->base);
+err_put_master:
+	platform_set_drvdata(pdev, NULL);
+	spi_master_put(sp->bitbang.master);
+
+	return ret;
+}
+
+static int ath79_spi_remove(struct platform_device *pdev)
+{
+	struct ath79_spi *sp = platform_get_drvdata(pdev);
+
+	spi_bitbang_stop(&sp->bitbang);
+	iounmap(sp->base);
+	platform_set_drvdata(pdev, NULL);
+	spi_master_put(sp->bitbang.master);
+
+	return 0;
+}
+
+static struct platform_driver ath79_spi_drv = {
+	.probe		= ath79_spi_probe,
+	.remove		= ath79_spi_remove,
+	.driver		= {
+		.name	= DRV_NAME,
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init ath79_spi_init(void)
+{
+	return platform_driver_register(&ath79_spi_drv);
+}
+module_init(ath79_spi_init);
+
+static void __exit ath79_spi_exit(void)
+{
+	platform_driver_unregister(&ath79_spi_drv);
+}
+module_exit(ath79_spi_exit);
+
+MODULE_DESCRIPTION(DRV_DESC);
+MODULE_AUTHOR("Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
1.7.2.1


------------------------------------------------------------------------------
Centralized Desktop Delivery: Dell and VMware Reference Architecture
Simplifying enterprise desktop deployment and management using
Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
client virtualization framework. Read more!
http://p.sf.net/sfu/dell-eql-dev2dev

Re: [RFC 11/18] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[Grant Likely]

On Fri, Nov 12, 2010 at 10:51:17PM +0100, Gabor Juhos wrote:
> The Atheros AR71XX/AR724X/AR913X SoCs have a built-in SPI controller. This
> patch implements a driver for that.
> 
> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> Cc: spi-devel-general@lists.sourceforge.net

Hi Gabor,

Overall looks pretty good, but a few questions below.

g.

> ---
> Sorry for sending this twice, i forgot to add some CCs in the first round.
> 
>  .../include/asm/mach-ath79/ath79_spi_platform.h    |   19 ++
>  drivers/spi/Kconfig                                |    8 +
>  drivers/spi/Makefile                               |    1 +
>  drivers/spi/ath79_spi.c                            |  291 ++++++++++++++++++++
>  4 files changed, 319 insertions(+), 0 deletions(-)
>  create mode 100644 arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
>  create mode 100644 drivers/spi/ath79_spi.c
> 
> diff --git a/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h b/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
> new file mode 100644
> index 0000000..aa71216
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-ath79/ath79_spi_platform.h
> @@ -0,0 +1,19 @@
> +/*
> + *  Platform data definition for Atheros AR71XX/AR724X/AR913X SPI controller
> + *
> + *  Copyright (C) 2008-2010 Gabor Juhos <juhosg@openwrt.org>
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms of the GNU General Public License version 2 as published
> + *  by the Free Software Foundation.
> + */
> +
> +#ifndef _ATH79_SPI_PLATFORM_H
> +#define _ATH79_SPI_PLATFORM_H
> +
> +struct ath79_spi_platform_data {
> +	unsigned	bus_num;
> +	unsigned	num_chipselect;
> +};
> +
> +#endif /* _ATH79_SPI_PLATFORM_H */
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 78f9fd0..f2093e1 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -53,6 +53,14 @@ if SPI_MASTER
>  
>  comment "SPI Master Controller Drivers"
>  
> +config SPI_ATH79
> +	tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver"
> +	depends on ATH79 && GENERIC_GPIO
> +	select SPI_BITBANG
> +	help
> +	  This enables support for the SPI controller present on the
> +	  Atheros AR71XX/AR724X/AR913X SoCs.
> +
>  config SPI_ATMEL
>  	tristate "Atmel SPI Controller"
>  	depends on (ARCH_AT91 || AVR32)
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 8bc1a5a..875bc3d 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_SPI_MASTER)		+= spi.o
>  
>  # SPI master controller drivers (bus)
>  obj-$(CONFIG_SPI_ATMEL)			+= atmel_spi.o
> +obj-$(CONFIG_SPI_ATH79)			+= ath79_spi.o
>  obj-$(CONFIG_SPI_BFIN)			+= spi_bfin5xx.o
>  obj-$(CONFIG_SPI_BITBANG)		+= spi_bitbang.o
>  obj-$(CONFIG_SPI_AU1550)		+= au1550_spi.o
> diff --git a/drivers/spi/ath79_spi.c b/drivers/spi/ath79_spi.c
> new file mode 100644
> index 0000000..9b9f9cf
> --- /dev/null
> +++ b/drivers/spi/ath79_spi.c
> @@ -0,0 +1,291 @@
> +/*
> + * SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs
> + *
> + * Copyright (C) 2009-2010 Gabor Juhos <juhosg@openwrt.org>
> + *
> + * This driver has been based on the spi-gpio.c:
> + *	Copyright (C) 2006,2008 David Brownell
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi_bitbang.h>
> +#include <linux/bitops.h>
> +#include <linux/gpio.h>
> +
> +#include <asm/mach-ath79/ar71xx_regs.h>
> +#include <asm/mach-ath79/ath79_spi_platform.h>
> +
> +#define DRV_DESC	"SPI controller driver for Atheros AR71XX/AR724X/AR91X"

Used exactly once.  Don't bother with a #define

> +#define DRV_NAME	"ath79-spi"
> +
> +struct ath79_spi {
> +	struct	spi_bitbang	bitbang;
> +	u32			ioc_base;
> +	u32			reg_ctrl;
> +
> +	void __iomem		*base;
> +
> +	struct platform_device	*pdev;
> +};
> +
> +static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
> +{
> +	return __raw_readl(sp->base + reg);
> +}
> +
> +static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
> +{
> +	__raw_writel(val, sp->base + reg);
> +}

This is suspect.  Why is __raw_{readl,writel} being used instead of
ioread32/iowrite32?  The __raw versions don't provide any kind of
ordering barriers.

> +
> +static inline struct ath79_spi *spidev_to_sp(struct spi_device *spi)
> +{
> +	return spi_master_get_devdata(spi->master);
> +}
> +
> +static void ath79_spi_chipselect(struct spi_device *spi, int is_active)
> +{
> +	struct ath79_spi *sp = spidev_to_sp(spi);
> +	int cs_high = (spi->mode & SPI_CS_HIGH) ? is_active : !is_active;
> +
> +	if (is_active) {
> +		/* set initial clock polarity */
> +		if (spi->mode & SPI_CPOL)
> +			sp->ioc_base |= AR71XX_SPI_IOC_CLK;
> +		else
> +			sp->ioc_base &= ~AR71XX_SPI_IOC_CLK;
> +
> +		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
> +	}
> +
> +	if (spi->chip_select) {
> +		unsigned long gpio = (unsigned long) spi->controller_data;
> +
> +		/* SPI is normally active-low */
> +		gpio_set_value(gpio, cs_high);
> +	} else {
> +		if (cs_high)
> +			sp->ioc_base |= AR71XX_SPI_IOC_CS0;
> +		else
> +			sp->ioc_base &= ~AR71XX_SPI_IOC_CS0;
> +
> +		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
> +	}
> +
> +}
> +
> +static int ath79_spi_setup_cs(struct spi_device *spi)
> +{
> +	struct ath79_spi *sp = spidev_to_sp(spi);
> +
> +	/* enable GPIO mode */
> +	ath79_spi_wr(sp, AR71XX_SPI_REG_FS, AR71XX_SPI_FS_GPIO);
> +
> +	/* save CTRL register */
> +	sp->reg_ctrl = ath79_spi_rr(sp, AR71XX_SPI_REG_CTRL);
> +	sp->ioc_base = ath79_spi_rr(sp, AR71XX_SPI_REG_IOC);
> +
> +	/* TODO: setup speed? */
> +	ath79_spi_wr(sp, AR71XX_SPI_REG_CTRL, 0x43);
> +
> +	if (spi->chip_select) {
> +		unsigned long gpio = (unsigned long) spi->controller_data;
> +		int status = 0;
> +
> +		status = gpio_request(gpio, dev_name(&spi->dev));
> +		if (status)
> +			return status;
> +
> +		status = gpio_direction_output(gpio, spi->mode & SPI_CS_HIGH);
> +		if (status) {
> +			gpio_free(gpio);
> +			return status;
> +		}
> +	} else {
> +		if (spi->mode & SPI_CS_HIGH)
> +			sp->ioc_base |= AR71XX_SPI_IOC_CS0;
> +		else
> +			sp->ioc_base &= ~AR71XX_SPI_IOC_CS0;
> +		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, sp->ioc_base);
> +	}
> +
> +	return 0;
> +}
> +
> +static void ath79_spi_cleanup_cs(struct spi_device *spi)
> +{
> +	struct ath79_spi *sp = spidev_to_sp(spi);
> +
> +	if (spi->chip_select) {
> +		unsigned long gpio = (unsigned long) spi->controller_data;
> +		gpio_free(gpio);
> +	}
> +
> +	/* restore CTRL register */
> +	ath79_spi_wr(sp, AR71XX_SPI_REG_CTRL, sp->reg_ctrl);
> +	/* disable GPIO mode */
> +	ath79_spi_wr(sp, AR71XX_SPI_REG_FS, 0);
> +}
> +
> +static int ath79_spi_setup(struct spi_device *spi)
> +{
> +	int status = 0;
> +
> +	if (spi->bits_per_word > 32)
> +		return -EINVAL;
> +
> +	if (!spi->controller_state) {
> +		status = ath79_spi_setup_cs(spi);
> +		if (status)
> +			return status;
> +	}
> +
> +	status = spi_bitbang_setup(spi);
> +	if (status && !spi->controller_state)
> +		ath79_spi_cleanup_cs(spi);
> +
> +	return status;
> +}
> +
> +static void ath79_spi_cleanup(struct spi_device *spi)
> +{
> +	ath79_spi_cleanup_cs(spi);
> +	spi_bitbang_cleanup(spi);
> +}
> +
> +static u32 ath79_spi_txrx_mode0(struct spi_device *spi, unsigned nsecs,
> +			       u32 word, u8 bits)
> +{
> +	struct ath79_spi *sp = spidev_to_sp(spi);
> +	u32 ioc = sp->ioc_base;
> +
> +	/* clock starts at inactive polarity */
> +	for (word <<= (32 - bits); likely(bits); bits--) {
> +		u32 out;
> +
> +		if (word & (1 << 31))
> +			out = ioc | AR71XX_SPI_IOC_DO;
> +		else
> +			out = ioc & ~AR71XX_SPI_IOC_DO;
> +
> +		/* setup MSB (to slave) on trailing edge */
> +		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, out);
> +		ath79_spi_wr(sp, AR71XX_SPI_REG_IOC, out | AR71XX_SPI_IOC_CLK);
> +
> +		word <<= 1;
> +	}
> +
> +	return ath79_spi_rr(sp, AR71XX_SPI_REG_RDS);
> +}
> +
> +static int ath79_spi_probe(struct platform_device *pdev)

__devinit

> +{
> +	struct spi_master *master;
> +	struct ath79_spi *sp;
> +	struct ath79_spi_platform_data *pdata;
> +	struct resource	*r;
> +	int ret;
> +
> +	master = spi_alloc_master(&pdev->dev, sizeof(*sp));
> +	if (master == NULL) {
> +		dev_err(&pdev->dev, "failed to allocate spi master\n");
> +		return -ENOMEM;
> +	}
> +
> +	sp = spi_master_get_devdata(master);
> +	platform_set_drvdata(pdev, sp);
> +
> +	pdata = pdev->dev.platform_data;
> +
> +	master->setup = ath79_spi_setup;
> +	master->cleanup = ath79_spi_cleanup;
> +	if (pdata) {
> +		master->bus_num = pdata->bus_num;
> +		master->num_chipselect = pdata->num_chipselect;
> +	} else {
> +		master->bus_num = 0;

Use -1 so that a bus number can be dynamically assigned.

> +		master->num_chipselect = 1;
> +	}
> +
> +	sp->bitbang.master = spi_master_get(master);
> +	sp->bitbang.chipselect = ath79_spi_chipselect;
> +	sp->bitbang.txrx_word[SPI_MODE_0] = ath79_spi_txrx_mode0;
> +	sp->bitbang.setup_transfer = spi_bitbang_setup_transfer;
> +	sp->bitbang.flags = SPI_CS_HIGH;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (r == NULL) {
> +		ret = -ENOENT;
> +		goto err_put_master;
> +	}
> +
> +	sp->base = ioremap(r->start, r->end - r->start + 1);
> +	if (!sp->base) {
> +		ret = -ENXIO;
> +		goto err_put_master;
> +	}
> +
> +	ret = spi_bitbang_start(&sp->bitbang);
> +	if (ret)
> +		goto err_unmap;
> +
> +	return 0;
> +
> +err_unmap:
> +	iounmap(sp->base);
> +err_put_master:
> +	platform_set_drvdata(pdev, NULL);
> +	spi_master_put(sp->bitbang.master);
> +
> +	return ret;
> +}
> +
> +static int ath79_spi_remove(struct platform_device *pdev)

__devexit

> +{
> +	struct ath79_spi *sp = platform_get_drvdata(pdev);
> +
> +	spi_bitbang_stop(&sp->bitbang);
> +	iounmap(sp->base);
> +	platform_set_drvdata(pdev, NULL);
> +	spi_master_put(sp->bitbang.master);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver ath79_spi_drv = {
> +	.probe		= ath79_spi_probe,
> +	.remove		= ath79_spi_remove,

__devexit_p(ath79_spi_remove),

> +	.driver		= {
> +		.name	= DRV_NAME,
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init ath79_spi_init(void)
> +{
> +	return platform_driver_register(&ath79_spi_drv);
> +}
> +module_init(ath79_spi_init);
> +
> +static void __exit ath79_spi_exit(void)
> +{
> +	platform_driver_unregister(&ath79_spi_drv);
> +}
> +module_exit(ath79_spi_exit);
> +
> +MODULE_DESCRIPTION(DRV_DESC);
> +MODULE_AUTHOR("Gabor Juhos <juhosg@openwrt.org>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" DRV_NAME);
> -- 
> 1.7.2.1
> 
> 
> ------------------------------------------------------------------------------
> Centralized Desktop Delivery: Dell and VMware Reference Architecture
> Simplifying enterprise desktop deployment and management using
> Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
> client virtualization framework. Read more!
> http://p.sf.net/sfu/dell-eql-dev2dev
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [RFC 11/18] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[Gabor Juhos]

Hi Grant,

>> <...>
>> +#include <asm/mach-ath79/ath79_spi_platform.h>
>> +
>> +#define DRV_DESC	"SPI controller driver for Atheros AR71XX/AR724X/AR91X"
> 
> Used exactly once.  Don't bother with a #define

Ok.

>> +#define DRV_NAME	"ath79-spi"
>> +
>> +struct ath79_spi {
>> +	struct	spi_bitbang	bitbang;
>> +	u32			ioc_base;
>> +	u32			reg_ctrl;
>> +
>> +	void __iomem		*base;
>> +
>> +	struct platform_device	*pdev;
>> +};
>> +
>> +static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
>> +{
>> +	return __raw_readl(sp->base + reg);
>> +}
>> +
>> +static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
>> +{
>> +	__raw_writel(val, sp->base + reg);
>> +}
> 
> This is suspect.  Why is __raw_{readl,writel} being used instead of
> ioread32/iowrite32?  The __raw versions don't provide any kind of
> ordering barriers.

Mainly because the resulting code is smaller, and the performance is a bit
better with the use of the __raw versions. The controller is embedded into the
SoC and the registers are memory mapped, so i think it is safe to access them
with __raw_{readl,writel}. However I can change it if that is the preferred method.

>> <...>
>> +static int ath79_spi_probe(struct platform_device *pdev)
> 
> __devinit

I will add this.

> 
>> +{
>> +	struct spi_master *master;
>> +	struct ath79_spi *sp;
>> +	struct ath79_spi_platform_data *pdata;
>> +	struct resource	*r;
>> +	int ret;
>> +
>> +	master = spi_alloc_master(&pdev->dev, sizeof(*sp));
>> +	if (master == NULL) {
>> +		dev_err(&pdev->dev, "failed to allocate spi master\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	sp = spi_master_get_devdata(master);
>> +	platform_set_drvdata(pdev, sp);
>> +
>> +	pdata = pdev->dev.platform_data;
>> +
>> +	master->setup = ath79_spi_setup;
>> +	master->cleanup = ath79_spi_cleanup;
>> +	if (pdata) {
>> +		master->bus_num = pdata->bus_num;
>> +		master->num_chipselect = pdata->num_chipselect;
>> +	} else {
>> +		master->bus_num = 0;
> 
> Use -1 so that a bus number can be dynamically assigned

All right.

>> <...>
>> +static int ath79_spi_remove(struct platform_device *pdev)
> 
> __devexit
> 
>> +{
>> +	struct ath79_spi *sp = platform_get_drvdata(pdev);
>> +
>> +	spi_bitbang_stop(&sp->bitbang);
>> +	iounmap(sp->base);
>> +	platform_set_drvdata(pdev, NULL);
>> +	spi_master_put(sp->bitbang.master);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver ath79_spi_drv = {
>> +	.probe		= ath79_spi_probe,
>> +	.remove		= ath79_spi_remove,
> 
> __devexit_p(ath79_spi_remove),
> 

I will add these annotations as well.

Thank you,
Gabor

Re: [RFC 11/18] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[Grant Likely]

On Sun, Nov 14, 2010 at 10:03:56PM +0100, Gabor Juhos wrote:
> >> +static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
> >> +{
> >> +	return __raw_readl(sp->base + reg);
> >> +}
> >> +
> >> +static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
> >> +{
> >> +	__raw_writel(val, sp->base + reg);
> >> +}
> > 
> > This is suspect.  Why is __raw_{readl,writel} being used instead of
> > ioread32/iowrite32?  The __raw versions don't provide any kind of
> > ordering barriers.
> 
> Mainly because the resulting code is smaller, and the performance is a bit
> better with the use of the __raw versions. The controller is embedded into the
> SoC and the registers are memory mapped, so i think it is safe to access them
> with __raw_{readl,writel}. However I can change it if that is the preferred method.
> 

Smaller, yes, because it doesn't have any io barriers; but is it safe?
Do you know whether or not the CPU will reorder the instructions on
you?  Being embedded into the SoC doesn't really mean anything in this
regard.  Unless you really understand all the behaviour of the CPU and
bus, then the safe versions must be used.

If you *do* really understand all the behaviour and decide it is safe
to use the __raw versions, then the driver needs to be well documented
as to the reasons why the __raw versions are safe to use.

g.


------------------------------------------------------------------------------
Centralized Desktop Delivery: Dell and VMware Reference Architecture
Simplifying enterprise desktop deployment and management using
Dell EqualLogic storage and VMware View: A highly scalable, end-to-end
client virtualization framework. Read more!
http://p.sf.net/sfu/dell-eql-dev2dev

Re: [RFC 11/18] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[Gabor Juhos]

2010.11.15. 5:04 keltezéssel, Grant Likely írta:
> On Sun, Nov 14, 2010 at 10:03:56PM +0100, Gabor Juhos wrote:
>>>> +static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
>>>> +{
>>>> +	return __raw_readl(sp->base + reg);
>>>> +}
>>>> +
>>>> +static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
>>>> +{
>>>> +	__raw_writel(val, sp->base + reg);
>>>> +}
>>>
>>> This is suspect.  Why is __raw_{readl,writel} being used instead of
>>> ioread32/iowrite32?  The __raw versions don't provide any kind of
>>> ordering barriers.
>>
>> Mainly because the resulting code is smaller, and the performance is a bit
>> better with the use of the __raw versions. The controller is embedded into the
>> SoC and the registers are memory mapped, so i think it is safe to access them
>> with __raw_{readl,writel}. However I can change it if that is the preferred method.
>>
> 
> Smaller, yes, because it doesn't have any io barriers; but is it safe?
> Do you know whether or not the CPU will reorder the instructions on
> you?  Being embedded into the SoC doesn't really mean anything in this
> regard.  Unless you really understand all the behaviour of the CPU and
> bus, then the safe versions must be used.
> 
> If you *do* really understand all the behaviour and decide it is safe
> to use the __raw versions, then the driver needs to be well documented
> as to the reasons why the __raw versions are safe to use.

These SoCs are using the MIPS 24K core. This core is based on an in-order
architecture, so it is safe to use the __raw versions from the CPU's side.

To be honest, I have no informations about that the completion of the request is
always in order that the request are received on the AHB bus between the CPU and
the SPI controller. However the Atheros' reference code uses the __raw versions
everywhere to access the registers of the built-in devices, so I assume that no
out-of-order completion is allowed on that bus.

Regards,
Gabor

Re: [RFC 11/18] spi: add SPI controller driver for the Atheros AR71XX/AR724X/AR913X SoCs

[Grant Likely]

On Mon, Nov 15, 2010 at 2:09 AM, Gabor Juhos <juhosg@openwrt.org> wrote:
> 2010.11.15. 5:04 keltezéssel, Grant Likely írta:
>> On Sun, Nov 14, 2010 at 10:03:56PM +0100, Gabor Juhos wrote:
>>>>> +static inline u32 ath79_spi_rr(struct ath79_spi *sp, unsigned reg)
>>>>> +{
>>>>> +  return __raw_readl(sp->base + reg);
>>>>> +}
>>>>> +
>>>>> +static inline void ath79_spi_wr(struct ath79_spi *sp, unsigned reg, u32 val)
>>>>> +{
>>>>> +  __raw_writel(val, sp->base + reg);
>>>>> +}
>>>>
>>>> This is suspect.  Why is __raw_{readl,writel} being used instead of
>>>> ioread32/iowrite32?  The __raw versions don't provide any kind of
>>>> ordering barriers.
>>>
>>> Mainly because the resulting code is smaller, and the performance is a bit
>>> better with the use of the __raw versions. The controller is embedded into the
>>> SoC and the registers are memory mapped, so i think it is safe to access them
>>> with __raw_{readl,writel}. However I can change it if that is the preferred method.
>>>
>>
>> Smaller, yes, because it doesn't have any io barriers; but is it safe?
>> Do you know whether or not the CPU will reorder the instructions on
>> you?  Being embedded into the SoC doesn't really mean anything in this
>> regard.  Unless you really understand all the behaviour of the CPU and
>> bus, then the safe versions must be used.
>>
>> If you *do* really understand all the behaviour and decide it is safe
>> to use the __raw versions, then the driver needs to be well documented
>> as to the reasons why the __raw versions are safe to use.
>
> These SoCs are using the MIPS 24K core. This core is based on an in-order
> architecture, so it is safe to use the __raw versions from the CPU's side.
>
> To be honest, I have no informations about that the completion of the request is
> always in order that the request are received on the AHB bus between the CPU and
> the SPI controller. However the Atheros' reference code uses the __raw versions
> everywhere to access the registers of the built-in devices, so I assume that no
> out-of-order completion is allowed on that bus.

Ralf, what say you?  I personally don't like this, and it makes for a
bad example of driver code, but I'll accept it if you say that it is
the right thing to do for MIPS device drivers.  (Although I retain my
requirement that the use of __raw accessors needs to be well
documented).

g.