Skip to contentSkip to conversation

Simple driver for Xilinx SPI controler.

PATCH In review Andrei Konovalov 7 participants 16 messages Jun 6, 2007 – Jun 13, 2007 6 messages not in the archive

Subsystems (3)
Network drivers SPI Arm/Zynq architecture
lore
Share & tools
lore

Andrei Konovalov

author
#0 lore raw

PATCH 1/1 Simple driver for Xilinx SPI controler.

Only master mode is supported.
No support for multiple masters. Slave mode is not supported either. Not using level 1 drivers from EDK.

The reason not to use EDK is basically this driver being a simple one:
- SPI level 1 driver from EDK supports much more then needed for this simple version of the SPI driver (slave mode, multiple masters etc). And leaving just single master case alone is not possible without changing the level 1 driver.
- SPI level 1 driver from EDK manipulates chip selects on its own. This makes it impossible to use the spi_bitbang.

The driver has been tested with Xilinx ML300 reference design.

The patch to enable SPI for Xilinx ML300 board (EDK reference design) will be sent to linuxppc-embedded list shortly.

Would be nice to get this driver into mainline.
Reviews and comments are welcome.

Thanks,
Andrei

Signed-off-by Yuri Frolov <[email protected]>
Signed-off-by Andrei Konovalov <[email protected]>
---
 arch/ppc/syslib/virtex_devices.h |    6 +
 drivers/spi/Kconfig              |    9 +
 drivers/spi/Makefile             |    1 +
 drivers/spi/xilinx_spi.c         |  447 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 463 insertions(+), 0 deletions(-)
 create mode 100644 drivers/spi/xilinx_spi.c
arch/ppc/syslib/virtex_devices.h +6 -0
index b49dc61..9ab3152 100644
@@ -45,4 +45,10 @@ struct xtemac_platform_data {
 	u8 mac_addr[6];
 };
 
+/* SPI Controller IP */
+struct xspi_platform_data {
+	s16 bus_num;
+	u16 num_chipselect;
+};
+
 #endif  /* __ASM_VIRTEX_DEVICES_H__ */
drivers/spi/Kconfig +9 -0
index 5e3f748..851d651 100644
@@ -156,6 +156,15 @@ config SPI_S3C24XX_GPIO
 	  GPIO lines to provide the SPI bus. This can be used where
 	  the inbuilt hardware cannot provide the transfer mode, or
 	  where the board is using non hardware connected pins.
+
+config SPI_XILINX
+	tristate "Xilinx SPI controller"
+	depends on SPI_MASTER && XILINX_VIRTEX && EXPERIMENTAL
+	select SPI_BITBANG
+	help
+	  This enables using the SPI controller IP from Xilinx EDK in master
+	  mode. See the DS464, "OPB Serial Peripheral Interface (SPI) (v1.00e)"
+	  Product Specification document for the hardware details.
 #
 # Add new SPI master controllers in alphabetical order above this line
 #
drivers/spi/Makefile +1 -0
index 5788d86..a2412bd 100644
@@ -23,6 +23,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_spi.o
 obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
 obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
+obj-$(CONFIG_SPI_XILINX)		+= xilinx_spi.o
 # 	... add above this line ...
 
 # SPI protocol drivers (device/link on bus)
drivers/spi/xilinx_spi.c new file +447 -0

447 lines not on this page · raw message

Signature · 1 line
1.5.1.1
#1 lore raw

In order to anticipate future Microblaze support with most of these drivers,
we've been moving towards having all of the Kconfig dependencies on XILINX_EDK,
rather than XILINX_VIRTEX...

Steve Neuendorffer

Quoted 5 lines

-----Original Message-----
From:
[email protected]

[mailto:linuxppc-embedded-bounces+stephen=neuendorffer.name@oz

labs.org] On Behalf Of Andrei Konovalov

Quoted 538 lines

Sent: Wednesday, June 06, 2007 7:06 AM
To: [email protected]
Cc: [email protected]
Subject: [PATCH] Simple driver for Xilinx SPI controler.

drivers/spi/Kconfig +6 -0
+config SPI_XILINX
+	tristate "Xilinx SPI controller"
+	depends on SPI_MASTER && XILINX_VIRTEX && EXPERIMENTAL
+	select SPI_BITBANG
+	help
+	  This enables using the SPI controller IP from Xilinx

EDK in master

(quoted diff) +1 -0
+	  mode. See the DS464, "OPB Serial Peripheral Interface

(SPI) (v1.00e)"

drivers/spi/Kconfig +1 -0
+	  Product Specification document for the hardware details.
  #
  # Add new SPI master controllers in alphabetical order

above this line
#

drivers/spi/Makefile +2 -0
index 5788d86..a2412bd 100644
@@ -23,6 +23,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC)		
+= mpc52xx_psc_spi.o
  obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
  obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
  obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
+obj-$(CONFIG_SPI_XILINX)		+= xilinx_spi.o
  # 	... add above this line ...
 
  # SPI protocol drivers (device/link on bus) diff --git

a/drivers/spi/xilinx_spi.c b/drivers/spi/xilinx_spi.c new file mode 100644 index 0000000..e5f149f

drivers/spi/xilinx_spi.c new file +9 -0
@@ -0,0 +1,447 @@
+/*
+ * xilinx_spi.c
+ *
+ * Xilinx SPI controler driver (master mode only)
+ *
+ * Author: MontaVista Software, Inc.
+ *         [email protected]
+ *
+ * 2002-2007 (c) MontaVista Software, Inc.  This file is

licensed under
+the
+ * terms of the GNU General Public License version 2. This program is
+licensed
+ * "as is" without any warranty of any kind, whether express or implied.
+ */

drivers/spi/xilinx_spi.c new file +14 -0
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_bitbang.h>
+
+#include <asm/io.h>
+#include <syslib/virtex_devices.h>
+
+#define XILINX_SPI_NAME "xspi"
+
+/* Register definitions as per "OPB Serial Peripheral

Interface (SPI)

drivers/spi/xilinx_spi.c new file +4 -0
+(v1.00e)
+ * Product Specification", DS464
+ */
+#define XSPI_CR_OFFSET		0x62	/* 16-bit

Control Register */

drivers/spi/xilinx_spi.c new file +12 -0
+
+#define XSPI_CR_ENABLE		0x02
+#define XSPI_CR_MASTER_MODE	0x04
+#define XSPI_CR_CPOL		0x08
+#define XSPI_CR_CPHA		0x10
+#define XSPI_CR_MODE_MASK	(XSPI_CR_CPHA | XSPI_CR_CPOL)
+#define XSPI_CR_TXFIFO_RESET	0x20
+#define XSPI_CR_RXFIFO_RESET	0x40
+#define XSPI_CR_MANUAL_SSELECT	0x80
+#define XSPI_CR_TRANS_INHIBIT	0x100
+
+#define XSPI_SR_OFFSET		0x67	/* 8-bit Status

Register */

drivers/spi/xilinx_spi.c new file +2 -0
+
+#define XSPI_SR_RX_EMPTY_MASK	0x01	/* Receive FIFO

is empty */

drivers/spi/xilinx_spi.c new file +2 -0
+#define XSPI_SR_RX_FULL_MASK	0x02	/* Receive FIFO is full */
+#define XSPI_SR_TX_EMPTY_MASK	0x04	/* Transmit

FIFO is empty */

drivers/spi/xilinx_spi.c new file +4 -0
+#define XSPI_SR_TX_FULL_MASK	0x08	/* Transmit FIFO is full */
+#define XSPI_SR_MODE_FAULT_MASK	0x10	/* Mode fault error */
+
+#define XSPI_TXD_OFFSET		0x6b	/* 8-bit Data

Transmit Register */
+#define XSPI_RXD_OFFSET 0x6f /* 8-bit Data
Receive Register */

drivers/spi/xilinx_spi.c new file +2 -0
+
+#define XSPI_SSR_OFFSET		0x70	/* 32-bit Slave

Select Register */

drivers/spi/xilinx_spi.c new file +6 -0
+
+/* Register definitions as per "OPB IPIF (v3.01c) Product 
+Specification", DS414
+ * IPIF registers are 32 bit
+ */
+#define XIPIF_V123B_DGIER_OFFSET	0x1c	/* IPIF global

int enable reg */

drivers/spi/xilinx_spi.c new file +3 -0
+#define XIPIF_V123B_GINTR_ENABLE	0x80000000
+
+#define XIPIF_V123B_IISR_OFFSET		0x20	/* IPIF

interrupt status reg */
+#define XIPIF_V123B_IIER_OFFSET 0x28 /* IPIF
interrupt enable reg */

drivers/spi/xilinx_spi.c new file +3 -0
+
+#define XSPI_INTR_MODE_FAULT		0x01	/* Mode fault error */
+#define XSPI_INTR_SLAVE_MODE_FAULT	0x02	/* Selected as

slave while

drivers/spi/xilinx_spi.c new file +3 -0
+                                                * disabled */
+#define XSPI_INTR_TX_EMPTY		0x04	/* TxFIFO is empty */
+#define XSPI_INTR_TX_UNDERRUN		0x08	/*

TxFIFO was underrun */

drivers/spi/xilinx_spi.c new file +4 -0
+#define XSPI_INTR_RX_FULL		0x10	/* RxFIFO is full */
+#define XSPI_INTR_RX_OVERRUN		0x20	/* RxFIFO was overrun */
+
+#define XIPIF_V123B_RESETR_OFFSET	0x40	/* IPIF reset

register */
+#define XIPIF_V123B_RESET_MASK 0x0a /* the
value to write */

drivers/spi/xilinx_spi.c new file +5 -0
+
+/* Simple macros to get the code more readable */
+#define xspi_in16(addr)		in_be16((u16 __iomem *)(addr))
+#define xspi_in32(addr)		in_be32((u32 __iomem *)(addr))
+#define xspi_out16(addr, value)	out_be16((u16 __iomem

*)(addr), (value))
+#define xspi_out32(addr, value) out_be32((u32 __iomem
*)(addr), (value))

drivers/spi/xilinx_spi.c new file +7 -0
+
+struct xilinx_spi {
+	/* bitbang has to be first */
+	struct spi_bitbang bitbang;
+	struct completion done;
+
+	u32		regs_phys;	/* phys. address of the

control registers */

drivers/spi/xilinx_spi.c new file +1 -0
+	void __iomem	*regs;		/* virt. address of the

control registers */

drivers/spi/xilinx_spi.c new file +6 -0
+
+	u32 irq;
+
+	u8 *rx_ptr;		/* pointer in the Tx buffer */
+	const u8 *tx_ptr;	/* pointer in the Rx buffer */
+	int remaining_bytes;	/* the number of bytes left to

transfer */

drivers/spi/xilinx_spi.c new file +7 -0
+};
+
+static void xspi_abort_transfer(u8 __iomem *regs_base) {
+	/* Deselect the slave on the SPI bus */
+	xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);
+
+	/* Terminate transmit in progress (if any) and Reset

the FIFOs */

drivers/spi/xilinx_spi.c new file +18 -0
+	xspi_out16(regs_base + XSPI_CR_OFFSET,
+		   xspi_in16(regs_base + XSPI_CR_OFFSET)
+		   | XSPI_CR_TRANS_INHIBIT | XSPI_CR_TXFIFO_RESET
+		   | XSPI_CR_RXFIFO_RESET);
+}
+
+static void xspi_init_hw(u8 __iomem *regs_base) {
+	/* Reset the SPI device */
+	xspi_out32(regs_base + XIPIF_V123B_RESETR_OFFSET,
+		   XIPIF_V123B_RESET_MASK);
+	/* Disable all the interrupts just in case */
+	xspi_out32(regs_base + XIPIF_V123B_IIER_OFFSET, 0);
+	/* Enable the global IPIF interrupt */
+	xspi_out32(regs_base + XIPIF_V123B_DGIER_OFFSET,
+		   XIPIF_V123B_GINTR_ENABLE);
+	/* Deselect the slave on the SPI bus */
+	xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);
+	/* Disable the transmitter, enable Manual Slave Select

Assertion,

drivers/spi/xilinx_spi.c new file +6 -0
+	 * put SPI controller into master mode, and enable it */
+	xspi_out16(regs_base + XSPI_CR_OFFSET,
+		   XSPI_CR_TRANS_INHIBIT | XSPI_CR_MANUAL_SSELECT
+		   | XSPI_CR_MASTER_MODE | XSPI_CR_ENABLE); }
+
+static void xilinx_spi_chipselect(struct spi_device *spi,

int is_on) {

drivers/spi/xilinx_spi.c new file +20 -0
+	struct xilinx_spi *xspi;
+	u8 __iomem *regs_base;
+
+	xspi = spi_master_get_devdata(spi->master);
+	regs_base = xspi->regs;
+
+	if (is_on == BITBANG_CS_INACTIVE) {
+		/* Deselect the slave on the SPI bus */
+		xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);
+	} else if (is_on == BITBANG_CS_ACTIVE) {
+		/* Set the SPI clock phase and polarity */
+		u16 cr = xspi_in16(regs_base + XSPI_CR_OFFSET)
+			 & ~XSPI_CR_MODE_MASK;
+		if (spi->mode & SPI_CPHA)
+			cr |= XSPI_CR_CPHA;
+		if (spi->mode & SPI_CPOL)
+			cr |= XSPI_CR_CPOL;
+		xspi_out16(regs_base + XSPI_CR_OFFSET, cr);
+
+		/* We do not check spi->max_speed_hz here as

the SPI clock

drivers/spi/xilinx_spi.c new file +1 -0
+		 * frequency is not software programmable (the

IP block design

drivers/spi/xilinx_spi.c new file +10 -0
+		 * parameter)
+		 */
+
+		/* Activate the chip select */
+		xspi_out32(regs_base + XSPI_SSR_OFFSET,
+			   ~(0x0001 << spi->chip_select));
+	}
+}
+
+/* spi_bitbang requires custom setup_transfer() to be

defined if there

drivers/spi/xilinx_spi.c new file +4 -0
+is a
+ * custom txrx_bufs(). We have nothing to setup here as the SPI IP 
+block
+ * supports just 8 bits per word, and SPI clock can't be

changed in software.
+ * Check for 8 bits per word; speed_hz checking could be added if the
+SPI
+ * clock information is available. Chip select delay calculations could
+be
+ * added here as soon as bitbang_work() can be made aware of the delay value.
+ */

drivers/spi/xilinx_spi.c new file +56 -0
+static int xilinx_spi_setup_transfer(struct spi_device *spi,
+				     struct spi_transfer *t)
+{
+	u8 bits_per_word;
+
+	bits_per_word = (t) ? t->bits_per_word : spi->bits_per_word;
+	if (bits_per_word != 8)
+		return -EINVAL;
+
+	return 0;
+}
+
+
+static int xilinx_spi_setup(struct spi_device *spi) {
+	struct spi_bitbang *bitbang;
+	struct xilinx_spi *xspi;
+	int retval;
+
+	xspi = spi_master_get_devdata(spi->master);
+	bitbang = &xspi->bitbang;
+
+	if (!spi->bits_per_word)
+		spi->bits_per_word = 8;
+
+	retval = xilinx_spi_setup_transfer(spi, NULL);
+	if (retval < 0)
+		return retval;
+
+	dev_dbg(&spi->dev, "%s, mode %d, %u bits/w, %u nsec/bit\n",
+		__FUNCTION__, spi->mode & (SPI_CPOL | SPI_CPHA),
+		spi->bits_per_word, 0);
+
+	return 0;
+}
+
+static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct 
+spi_transfer *t) {
+	struct xilinx_spi *xspi;
+	u8 __iomem *regs_base;
+	u32 ipif_ier;
+	u16 cr;
+	u8 sr;
+
+	/* We get here with transmitter inhibited */
+
+	xspi = spi_master_get_devdata(spi->master);
+	regs_base = xspi->regs;
+
+	xspi->tx_ptr = t->tx_buf;
+	xspi->rx_ptr = t->rx_buf;
+	xspi->remaining_bytes = t->len;
+	INIT_COMPLETION(xspi->done);
+
+	/* Fill the Tx FIFO with as many bytes as possible */
+	sr = in_8(regs_base + XSPI_SR_OFFSET);
+	while ((sr & XSPI_SR_TX_FULL_MASK) == 0 &&

xspi->remaining_bytes > 0) {

drivers/spi/xilinx_spi.c new file +2 -0
+		if (xspi->tx_ptr) {
+			out_8(regs_base + XSPI_TXD_OFFSET,

*xspi->tx_ptr++);

drivers/spi/xilinx_spi.c new file +8 -0
+		} else {
+			out_8(regs_base + XSPI_TXD_OFFSET, 0);
+		}
+		xspi->remaining_bytes--;
+		sr = in_8(regs_base + XSPI_SR_OFFSET);
+	}
+
+	/* Enable the transmit empty interrupt, which we use to

determine

drivers/spi/xilinx_spi.c new file +7 -0
+	 * progress on the transmission.
+	 */
+	ipif_ier = xspi_in32(regs_base + XIPIF_V123B_IIER_OFFSET);
+	xspi_out32(regs_base + XIPIF_V123B_IIER_OFFSET,
+		   ipif_ier | XSPI_INTR_TX_EMPTY);
+
+	/* Start the transfer by not inhibiting the transmitter

any longer */

drivers/spi/xilinx_spi.c new file +1 -0
+	cr = xspi_in16(regs_base + XSPI_CR_OFFSET) &

~XSPI_CR_TRANS_INHIBIT;

drivers/spi/xilinx_spi.c new file +13 -0
+	xspi_out16(regs_base + XSPI_CR_OFFSET, cr);
+
+	wait_for_completion(&xspi->done);
+
+	/* Disable the transmit empty interrupt */
+	xspi_out32(regs_base + XIPIF_V123B_IIER_OFFSET, ipif_ier);
+
+	return t->len - xspi->remaining_bytes; }
+
+
+/* This driver supports single master mode only. Hence Tx FIFO Empty
+ * is the only interrupt we care about.
+ * Receive FIFO Overrun, Transmit FIFO Underrun, Mode Fault,

and Slave

drivers/spi/xilinx_spi.c new file +12 -0
+Mode
+ * Fault are not to happen.
+ */
+static irqreturn_t xilinx_spi_irq(int irq, void *dev_id) {
+	struct xilinx_spi *xspi;
+	u8 __iomem *regs_base;
+	u32 ipif_isr;
+
+	xspi = (struct xilinx_spi *) dev_id;
+	regs_base = xspi->regs;
+
+     	/* Get the IPIF inetrrupts, and clear them

immediately */

drivers/spi/xilinx_spi.c new file +4 -0
+	ipif_isr = xspi_in32(regs_base + XIPIF_V123B_IISR_OFFSET);
+	xspi_out32(regs_base + XIPIF_V123B_IISR_OFFSET, ipif_isr);
+
+	if (ipif_isr & XSPI_INTR_TX_EMPTY) {	/* Transmission

completed */

drivers/spi/xilinx_spi.c new file +4 -0
+		u16 cr;
+		u8 sr;
+
+		/* A transmit has just completed. Process

received data and

drivers/spi/xilinx_spi.c new file +2 -0
+		 * check for more data to transmit. Always inhibit the
+		 * transmitter while the Isr refills the

transmit register/FIFO,

drivers/spi/xilinx_spi.c new file +21 -0
+		 * or make sure it is stopped if we're done.
+	         */
+		cr = xspi_in16(regs_base + XSPI_CR_OFFSET);
+		xspi_out16(regs_base + XSPI_CR_OFFSET,
+			   cr | XSPI_CR_TRANS_INHIBIT);
+
+		/* Read out all the data from the Rx FIFO */
+		sr = in_8(regs_base + XSPI_SR_OFFSET);
+		while ((sr & XSPI_SR_RX_EMPTY_MASK) == 0) {
+			u8 data;
+
+			data = in_8(regs_base + XSPI_RXD_OFFSET);
+			if (xspi->rx_ptr) {
+				*xspi->rx_ptr++ = data;
+			}
+			sr = in_8(regs_base + XSPI_SR_OFFSET);
+		}
+
+	        /* See if there is more data to send */
+		if (xspi->remaining_bytes > 0) {
+			/* sr content is valid here; no need

for io_8() */

drivers/spi/xilinx_spi.c new file +4 -0
+			while ((sr & XSPI_SR_TX_FULL_MASK) == 0
+				&& xspi->remaining_bytes > 0) {
+				if (xspi->tx_ptr) {
+					out_8(regs_base +

XSPI_TXD_OFFSET,

drivers/spi/xilinx_spi.c new file +3 -0
+					      *xspi->tx_ptr++);
+				} else {
+					out_8(regs_base +

XSPI_TXD_OFFSET, 0);

drivers/spi/xilinx_spi.c new file +77 -0
+				}
+				xspi->remaining_bytes--;
+				sr = in_8(regs_base + XSPI_SR_OFFSET);
+			}
+			/* Start the transfer by not inhibiting the
+			 * transmitter any longer
+			 */
+			xspi_out16(regs_base + XSPI_CR_OFFSET, cr);
+		} else {
+			/* No more data to send.
+			 * Indicate the transfer is completed.
+			 */
+			complete(&xspi->done);
+		}
+	} else {
+		/* spurious interrupt */
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int __init xilinx_spi_probe(struct platform_device *dev) {
+	int ret = 0;
+	struct spi_master *master;
+	struct xilinx_spi *xspi;
+	struct xspi_platform_data *pdata;
+	struct resource *r;
+
+	/* Get resources(memory, IRQ) associated with the device */
+	master = spi_alloc_master(&dev->dev, sizeof(struct xilinx_spi));
+
+	if (master == NULL) {
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(dev, master);
+	pdata = dev->dev.platform_data;
+
+	if (pdata == NULL) {
+		ret = -ENODEV;
+		goto put_master;
+	}
+
+	r = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (r == NULL) {
+		ret = -ENODEV;
+		goto put_master;
+	}
+
+	xspi = spi_master_get_devdata(master);
+	xspi->bitbang.master = spi_master_get(master);
+	xspi->bitbang.chipselect = xilinx_spi_chipselect;
+	xspi->bitbang.setup_transfer = xilinx_spi_setup_transfer;
+	xspi->bitbang.txrx_bufs = xilinx_spi_txrx_bufs;
+	xspi->bitbang.master->setup = xilinx_spi_setup;
+	init_completion(&xspi->done);
+
+	xspi->regs = ioremap(r->start, r->end - r->start + 1);
+	if (xspi->regs == NULL) {
+		ret = -ENOMEM;
+		goto put_master;
+	}
+
+	xspi->irq = platform_get_irq(dev, 0);
+	if (xspi->irq < 0) {
+		ret = -ENXIO;
+		goto unmap_io;
+	}
+
+	master->bus_num = pdata->bus_num;
+	master->num_chipselect = pdata->num_chipselect;
+
+	/* SPI controller initializations */
+	xspi_init_hw(xspi->regs);
+
+	/* Register for SPI Interrupt */
+	ret = request_irq(xspi->irq, xilinx_spi_irq, 0,

XILINX_SPI_NAME, xspi);

drivers/spi/xilinx_spi.c new file +62 -1
+	if (ret != 0)
+		goto unmap_io;
+
+	ret = spi_bitbang_start(&xspi->bitbang);
+	if (ret != 0) {
+		printk(KERN_ALERT "spi_btbang_start FAILED\n");
+		goto free_irq;
+	}
+
+	printk(KERN_INFO "%s: at 0x%08X mapped to 0x%08X, irq=%d\n",
+	       dev->dev.bus_id, r->start, (u32)xspi->regs, xspi->irq);
+
+	return ret;
+
+free_irq:
+        free_irq(xspi->irq, xspi);
+unmap_io:
+	iounmap(xspi->regs);
+put_master:
+        spi_master_put(master);
+	return ret;
+}
+
+static int __devexit xilinx_spi_remove(struct platform_device *dev) {
+	struct xilinx_spi *xspi;
+	struct spi_master *master;
+
+	master = platform_get_drvdata(dev);
+	xspi = spi_master_get_devdata(master);
+
+	spi_bitbang_stop(&xspi->bitbang);
+	xspi_abort_transfer(xspi->regs);
+	free_irq(xspi->irq, xspi);
+	iounmap(xspi->regs);
+	platform_set_drvdata(dev, 0);
+	spi_master_put(xspi->bitbang.master);
+
+	return 0;
+}
+
+static struct platform_driver xilinx_spi_driver = {
+	.probe	= xilinx_spi_probe,
+	.remove	= __devexit_p(xilinx_spi_remove),
+	.driver = {
+		   .name = XILINX_SPI_NAME,
+		   .owner = THIS_MODULE,
+	},
+};
+
+static int __init xilinx_spi_init(void) {
+	return platform_driver_register(&xilinx_spi_driver);
+}
+
+static void __exit xilinx_spi_exit(void) {
+	platform_driver_unregister(&xilinx_spi_driver);
+}
+
+module_init(xilinx_spi_init);
+module_exit(xilinx_spi_exit);
+
+MODULE_AUTHOR("MontaVista Software, Inc. <[email protected]>"); 
+MODULE_DESCRIPTION("Xilinx SPI driver"); MODULE_LICENSE("GPL");
--

1.5.1.1

_______________________________________________
Linuxppc-embedded mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-embedded

#2 lore raw

Hi Steve,

Quoted 4 lines · Stephen Neuendorffer

In order to anticipate future Microblaze support with most of these drivers,
we've been moving towards having all of the Kconfig dependencies on XILINX_EDK,

There is no XILINX_EDK in the ko tree.

Quoted 1 line

rather than XILINX_VIRTEX...

Using XILINX_VIRTEX seems to be a common thing:

Quoted 8 lines

-------- Original Message --------
Subject: [RFC] Xilinx SystemACE device driver
Date: Sat, 14 Apr 2007 19:23:14 -0600
From: Grant Likely <[email protected]>
To: [email protected], Andrei Konovalov <[email protected]>, Peter Korsgaard <[email protected]>, Rick Moleres <[email protected]>, Stefan Roese <[email protected]>

Add support for block device access to the Xilinx SystemACE Compact flash interface

<snip>

Quoted 5 lines
drivers/block/Kconfig +0 -0
index 17ee97f..08ad23c 100644

drivers/block/Kconfig config ATA_OVER_ETH · line 461

Quoted 9 lines
drivers/block/Kconfig +6 -0
@@ -453,6 +453,12 @@ config ATA_OVER_ETH
 	This driver provides Support for ATA over Ethernet block
 	devices like the Coraid EtherDrive (R) Storage Blade.
 
+config XILINX_SYSACE
+	tristate "Xilinx SystemACE support"
+	depends on XILINX_VIRTEX
+	help
+	  Include support for the Xilinx SystemACE CompactFlash interface
+

BTW the SystemACE has been successfully tested with ppc440 based board. So XILINX_VIRTEX doesn't imply ppc405.

On the other side,
UART Lite driver (in the ko tree) doesn't depend on both XILINX_EDK and XILINX_VIRTEX

Couldn't Microblaze port use XILINX_VIRTEX too?
IMHO XILINX_VIRTEX is more general name for something inside the Virtex chip (ppc405 core, Microblaze softcore, IP blocks from EDK or elsewhere).

I've recently added XILINX_EDK to linux-xilinx-26.git repository at source.mvista.com, but to include the level 1 drivers (aka OS independent drivers) from EDK. Haven't planned to use XILINX_EDK for drivers not using EDK code.
Here is the relevant snippet from arch/ppc/platforms/4xx/Kconfig
(from linux-xilinx-26.git @ source.mvista.com, ko doesn't have XILINX_EDK and the rest below):

===8<=====================================
config XILINX_VIRTEX_II_PRO
bool
select XILINX_VIRTEX

config XILINX_VIRTEX_4_FX
bool
select XILINX_VIRTEX

config XILINX_VIRTEX
bool

# The options selected by EDK based drivers. Not visible from [menu]config. config XILINX_EDK
bool
config XILINX_IPIF_V123B
bool
config XILINX_FIFO_V200A
bool
config XILINX_DMA_V300A
bool
===8<=====================================

If we do something like
config XILINX_MICROBLAZE
bool
select XILINX_VIRTEX
in arch/microblaze/Kconfig
would it work?

Thanks,
Andrei

#3 lore raw

Yes, except that microblazes systems run in non-virtex FPGAs...

It may be that XILINX_EDK is not the right thing either, but it seemed like the easiest way to have a superset of powerpc and microblaze systems

What I'm trying to do is (in concert with Grant's recent configuration changes)
having an easy way for a new board configuration to select the Kconfig options for
all of the Xilinx drivers. Namely:

config XILINX_ML300

	bool "Xilinx-ML300"
	select XILINX_VIRTEX_II_PRO
	select EMBEDDEDBOOT
	help
	  This option enables support for the Xilinx ML300 evaluation

board.

config XILINX_VIRTEX_II_PRO
bool
select XILINX_VIRTEX

config XILINX_EDK

	bool
	depends on XILINX_VIRTEX || XILINX_MICROBLAZE
	default y

config XILINX_GPIO
tristate "Xilinx OPB GPIO Support" depends on XILINX_EDK
help
This option enables support for Xilinx GPIO.

It seems like I'm using XILINX_EDK to mean something different than you are.
Perhaps it sould be instead:

config XILINX_DRIVERS

	bool
	depends on XILINX_VIRTEX || XILINX_MICROBLAZE
	default y

config XILINX_GPIO_EDK
tristate "Xilinx EDK-based OPB GPIO Support" depends on XILINX_DRIVERS
select XILINX_EDK
help
This option enables support for Xilinx GPIO using EDK OS
independent driver.

config XILINX_GPIO
tristate "Xilinx OPB GPIO Support" depends on XILINX_DRIVERS
help
This option enables support for Xilinx GPIO.

Would this work for you?

Steve

Quoted 106 lines

-----Original Message-----
From: Andrei Konovalov [mailto:[email protected]]
Sent: Wednesday, June 06, 2007 10:58 AM
To: Stephen Neuendorffer
Cc: [email protected]; [email protected]; Wolfgang Reissnegger
Subject: Re: [PATCH] Simple driver for Xilinx SPI controler.

Hi Steve,

Quoted 3 lines · Stephen Neuendorffer

In order to anticipate future Microblaze support with most of these drivers, we've been moving towards having all of the Kconfig dependencies on XILINX_EDK,

There is no XILINX_EDK in the ko tree.

Quoted 1 line

rather than XILINX_VIRTEX...

Using XILINX_VIRTEX seems to be a common thing:

Quoted 5 lines

-------- Original Message --------
Subject: [RFC] Xilinx SystemACE device driver
Date: Sat, 14 Apr 2007 19:23:14 -0600
From: Grant Likely <[email protected]>
To: [email protected], Andrei Konovalov

<[email protected]>, Peter Korsgaard
<[email protected]>, Rick Moleres <[email protected]>,
Stefan Roese <[email protected]>

Quoted 3 lines

Add support for block device access to the Xilinx SystemACE Compact flash interface

<snip>

drivers/block/Kconfig config ATA_OVER_ETH · line 460

Quoted 13 lines
drivers/block/Kconfig index renamed from drivers/block/Kconfig +0 -0

17ee97f..08ad23c 100644

drivers/block/Kconfig +5 -0
@@ -453,6 +453,12 @@ config ATA_OVER_ETH
 	This driver provides Support for ATA over Ethernet block
 	devices like the Coraid EtherDrive (R) Storage Blade.
 
+config XILINX_SYSACE
+	tristate "Xilinx SystemACE support"
+	depends on XILINX_VIRTEX
+	help
+	  Include support for the Xilinx SystemACE CompactFlash

interface

drivers/block/Kconfig config ATA_OVER_ETH · line 461

Quoted 1 line
drivers/block/Kconfig +1 -0
+

BTW the SystemACE has been successfully tested with ppc440 based board. So XILINX_VIRTEX doesn't imply ppc405.

On the other side,
UART Lite driver (in the ko tree) doesn't depend on both XILINX_EDK and XILINX_VIRTEX

Couldn't Microblaze port use XILINX_VIRTEX too? IMHO XILINX_VIRTEX is more general name for something inside the Virtex chip (ppc405 core, Microblaze softcore, IP blocks from EDK or elsewhere).

I've recently added XILINX_EDK to linux-xilinx-26.git repository at source.mvista.com, but to include the level 1 drivers (aka OS independent drivers) from EDK. Haven't planned to use XILINX_EDK for drivers not using EDK code. Here is the relevant snippet from arch/ppc/platforms/4xx/Kconfig (from linux-xilinx-26.git @ source.mvista.com, ko doesn't have XILINX_EDK and the rest below):

===8<=====================================
config XILINX_VIRTEX_II_PRO
bool
select XILINX_VIRTEX

config XILINX_VIRTEX_4_FX
bool
select XILINX_VIRTEX

config XILINX_VIRTEX
bool

# The options selected by EDK based drivers. Not visible from [menu]config.
config XILINX_EDK
bool
config XILINX_IPIF_V123B
bool
config XILINX_FIFO_V200A
bool
config XILINX_DMA_V300A
bool
===8<=====================================

If we do something like
config XILINX_MICROBLAZE
bool
select XILINX_VIRTEX
in arch/microblaze/Kconfig
would it work?

Thanks,
Andrei

#4 lore raw
Quoted 21 lines · Stephen Neuendorffer

Yes, except that microblazes systems run in non-virtex FPGAs...

It may be that XILINX_EDK is not the right thing either, but it seemed like the easiest way to have a superset of powerpc and microblaze systems

What I'm trying to do is (in concert with Grant's recent configuration changes)
having an easy way for a new board configuration to select the Kconfig options for
all of the Xilinx drivers. Namely:

config XILINX_ML300
bool "Xilinx-ML300"
select XILINX_VIRTEX_II_PRO
select EMBEDDEDBOOT
help
This option enables support for the Xilinx ML300 evaluation
board.

Quoted 8 lines

config XILINX_VIRTEX_II_PRO
bool
select XILINX_VIRTEX

config XILINX_EDK

        bool
        depends on XILINX_VIRTEX || XILINX_MICROBLAZE
        default y

I don't have any problem with adding XILINX_EDK (or whatever), but think particular layout is back-assward. Rather than XILINX_EDK depending on XILINX_VIRTEX/MICROBLAZE, XILINX_VIRTEX/MICROBLAZE should 'select' XILINX_EDK and you need to drop the 'default y' line. Otherwise the XILINX_EDK option shows up in non-edk config files.

Quoted 8 lines

config XILINX_GPIO
tristate "Xilinx OPB GPIO Support" depends on XILINX_EDK
help
This option enables support for Xilinx GPIO.

Quoted 8 lines

It seems like I'm using XILINX_EDK to mean something different than you are.
Perhaps it sould be instead:

config XILINX_DRIVERS

        bool
        depends on XILINX_VIRTEX || XILINX_MICROBLAZE
        default y

Hmmm, yes. XILINX_DRIVERS is probably better.

Quoted 8 lines

config XILINX_GPIO_EDK
tristate "Xilinx EDK-based OPB GPIO Support" depends on XILINX_DRIVERS
select XILINX_EDK
help
This option enables support for Xilinx GPIO using EDK OS
independent driver.

Why still have XILINX_EDK? I thought you meant replace XILINX_EDK with XILINX_DRIVERS.

Cheers,
g.

Signature · 4 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195

A message that is not in the archive (sent off-list or not yet received): fa686aa40706061200u1141fcc9k73b7cc80c07240d1-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#6 lore raw
Quoted 6 lines
Quoted 5 lines

config XILINX_EDK

        bool
        depends on XILINX_VIRTEX || XILINX_MICROBLAZE
        default y
Quoted 6 lines

I don't have any problem with adding XILINX_EDK (or whatever), but think particular layout is back-assward. Rather than XILINX_EDK depending on XILINX_VIRTEX/MICROBLAZE, XILINX_VIRTEX/MICROBLAZE should 'select' XILINX_EDK and you need to drop the 'default y' line.
Otherwise the XILINX_EDK option shows up in non-edk config files.

Good point...

Quoted 2 lines

Why still have XILINX_EDK? I thought you meant replace XILINX_EDK with XILINX_DRIVERS.

Yes, that's what I was suggesting.
But I think that Andrei still wants to use XILINX_EDK to pull in the stuff in drivers/xilinx_common.

# The Xilinx OS common code

obj-$(CONFIG_XILINX_EDK) += xbasic_types.o xpacket_fifo_l_v2_00_a.o \
			    xpacket_fifo_v2_00_a.o xversion.o \
			    xdma_channel.o xdma_channel_sg.o

Which you wouldn't want if you were able to build a kernel completely from 'non-edk' drivers.

Steve

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

#7 lore raw
Quoted 7 lines · Stephen Neuendorffer
Quoted 2 lines

Why still have XILINX_EDK? I thought you meant replace XILINX_EDK with XILINX_DRIVERS.

Yes, that's what I was suggesting.
But I think that Andrei still wants to use XILINX_EDK to pull in the stuff in drivers/xilinx_common.

Quoted 7 lines

# The Xilinx OS common code

obj-$(CONFIG_XILINX_EDK) += xbasic_types.o xpacket_fifo_l_v2_00_a.o \
                            xpacket_fifo_v2_00_a.o xversion.o \
                            xdma_channel.o xdma_channel_sg.o

Which you wouldn't want if you were able to build a kernel completely from 'non-edk' drivers.

Fair enough, but that's only an issue for vendor trees. None of this code will be going into mainline. Any of it that does get into good shape should probably go under drivers/misc, and have the drivers select only the pieces that are needed (with finer-grain CONFIG_ items). A blanked CONFIG_XILINX_EDK for random stuff is not a good idea.

Cheers,
g.

Quoted 5 lines

Steve

Signature · 4 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195
#8 lore raw

IMHO the Kconfig stuff is off topic on spi-devel-general list, so I've removed them from the distribution list. OK?

Quoted 7 lines · Grant Likely
Quoted 6 lines · Stephen Neuendorffer
Quoted 2 lines

Why still have XILINX_EDK? I thought you meant replace XILINX_EDK with XILINX_DRIVERS.

Yes, that's what I was suggesting.
But I think that Andrei still wants to use XILINX_EDK to pull in the stuff in drivers/xilinx_common.

Correct,

Quoted 5 lines
Quoted 5 lines

# The Xilinx OS common code

obj-$(CONFIG_XILINX_EDK) += xbasic_types.o xpacket_fifo_l_v2_00_a.o \
                            xpacket_fifo_v2_00_a.o xversion.o \
                            xdma_channel.o xdma_channel_sg.o

.. but not exactly this way.
See
http://source.mvista.com/git/gitweb.cgi?p=linux-xilinx-26.git;a=commitdiff;h=e5b35cc7fdef280b7dd7e592a57c6dd823c62ea1
http://source.mvista.com/git/gitweb.cgi?p=linux-xilinx-26.git;a=commitdiff;h=b03eb158a0c1b4a8dd32940df02431bdd5ffcef9

Quoted 6 lines
Quoted 2 lines

Which you wouldn't want if you were able to build a kernel completely from 'non-edk' drivers.

Fair enough, but that's only an issue for vendor trees. None of this code will be going into mainline. Any of it that does get into good shape should probably go under drivers/misc, and have the drivers

At the moment I have this stuff in arch/ppc/syslib/xilinx. drivers/misc seems a better place indeed.
Anyone objects I move arch/ppc/syslib/xilinx/* to drivers/misc/xilinx/* ?

Quoted 2 lines

select only the pieces that are needed (with finer-grain CONFIG_ items).

Yes, my current implementation is:

arch/ppc/platforms/4xx/Kconfig +0 -0
@@ -229,6 +229,16 @@ config XILINX_VIRTEX_4_FX

config XILINX_VIRTEX
bool
+# The options selected by EDK based drivers. Not visible from [menu]config. +config XILINX_EDK
+ bool
+config XILINX_IPIF_V123B
+ bool
+config XILINX_FIFO_V200A
+ bool
+config XILINX_DMA_V300A
+ bool
+
config STB03xxx

arch/ppc/syslib/xilinx/Makefile new file +10 -0
@@ -0,0 +1,10 @@
+#
+# Makefile for generic hardware blocks that could be incorporated into
+# various IP blocks: IPIF, FIFO, SGDMA.
+#
+
+obj-$(CONFIG_XILINX_EDK) += xbasic_types.o
+obj-$(CONFIG_XILINX_IPIF_V123B) += xipif_v1_23_b.o
+obj-$(CONFIG_XILINX_FIFO_V200A) += xpacket_fifo_v2_00_a.o \
+ xpacket_fifo_l_v2_00_a.o
+obj-$(CONFIG_XILINX_DMA_V300A) += xdmav3.o xdmav3_intr.o xdmav3_sg.o
drivers/net/Kconfig +0 -0
@@ -2330,6 +2330,18 @@ config ATL1

To compile this driver as a module, choose M here. The module will be called atl1.
+config XILINX_TEMAC
+ tristate "Xilinx 10/100/1000 Mbit TEMAC support"
+ depends on XILINX_VIRTEX
+ select PHYLIB
+ select XILINX_EDK
+# select XILINX_IPIF_V123B
+ select XILINX_FIFO_V200A
+ select XILINX_DMA_V300A
+ help
+ This driver supports Tri-Mode, 10/100/1000 Mbit EMAC IP
+ from Xilinx EDK.
+
endif # NETDEV_1000

Is it better?
(Except for CONFIG_XILINX_EDK which should be better renamed to CONFIG_XILINX_COMMON or CONFIG_XILINX_COMMON_V100A as per the level 1 driver name in the EDK:
sw/XilinxProcessrIPLib/drivers/common_v1_00_a/src)

Quoted 2 lines

A blanked CONFIG_XILINX_EDK for random stuff is not a good idea.

Agreed

Thanks,
Andrei

Quoted 10 lines

Cheers,
g.

Quoted 5 lines

Steve

#9 lore raw
In reply to Grant Likely

Grant,

I'm in the process of merging your virtex-dev tree into our linux-2.6-xlnx tree. I'll be taking care of the necessary changes. They will show up on the Xilinx git repository soon.

Thanks,
Wolfgang

Quoted 71 lines · Grant Likely
Quoted 21 lines · Stephen Neuendorffer

Yes, except that microblazes systems run in non-virtex FPGAs...

It may be that XILINX_EDK is not the right thing either, but it seemed like the easiest way to have a superset of powerpc and microblaze systems

What I'm trying to do is (in concert with Grant's recent configuration changes)
having an easy way for a new board configuration to select the Kconfig options for
all of the Xilinx drivers. Namely:

config XILINX_ML300
bool "Xilinx-ML300"
select XILINX_VIRTEX_II_PRO
select EMBEDDEDBOOT
help
This option enables support for the Xilinx ML300 evaluation
board.

Quoted 8 lines

config XILINX_VIRTEX_II_PRO
bool
select XILINX_VIRTEX

config XILINX_EDK

        bool
        depends on XILINX_VIRTEX || XILINX_MICROBLAZE
        default y

I don't have any problem with adding XILINX_EDK (or whatever), but think particular layout is back-assward. Rather than XILINX_EDK depending on XILINX_VIRTEX/MICROBLAZE, XILINX_VIRTEX/MICROBLAZE should 'select' XILINX_EDK and you need to drop the 'default y' line. Otherwise the XILINX_EDK option shows up in non-edk config files.

Quoted 8 lines

config XILINX_GPIO
tristate "Xilinx OPB GPIO Support" depends on XILINX_EDK
help
This option enables support for Xilinx GPIO.

Quoted 8 lines

It seems like I'm using XILINX_EDK to mean something different than you are.
Perhaps it sould be instead:

config XILINX_DRIVERS

        bool
        depends on XILINX_VIRTEX || XILINX_MICROBLAZE
        default y

Hmmm, yes. XILINX_DRIVERS is probably better.

Quoted 8 lines

config XILINX_GPIO_EDK
tristate "Xilinx EDK-based OPB GPIO Support" depends on XILINX_DRIVERS
select XILINX_EDK
help
This option enables support for Xilinx GPIO using EDK OS
independent driver.

Why still have XILINX_EDK? I thought you meant replace XILINX_EDK with XILINX_DRIVERS.

Cheers,
g.

#10 lore raw
Quoted 5 lines · Wolfgang Reissnegger

Grant,

I'm in the process of merging your virtex-dev tree into our linux-2.6-xlnx tree. I'll be taking care of the necessary changes. They will show up on the Xilinx git repository soon.

Don't forget to send patches to the list for review so we can get this stuff into mainline.

g.

Signature · 4 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195

A message that is not in the archive (sent off-list or not yet received): [email protected]

#12 lore raw
Quoted 3 lines · Andrei Konovalov

Would be nice to get this driver into mainline.
Reviews and comments are welcome.

I'll ignore the Kconfig flamage ... ;)

Quoted 12 lines
drivers/spi/xilinx_spi.c new file +9 -0
@@ -0,0 +1,447 @@
+/*
+ * xilinx_spi.c
+ *
+ * Xilinx SPI controler driver (master mode only)
+ *
+ * Author: MontaVista Software, Inc.
+ *         [email protected]
+ *
+ * 2002-2007 (c) MontaVista Software, Inc.  This file is licensed under the

drivers/spi/xilinx_spi.c line 19

Quoted 10 lines
drivers/spi/xilinx_spi.c new file +10 -0
+ * terms of the GNU General Public License version 2.  This program is licensed
+ * "as is" without any warranty of any kind, whether express or implied.
+ */
+
+
+/* Simple macros to get the code more readable */
+#define xspi_in16(addr)		in_be16((u16 __iomem *)(addr))
+#define xspi_in32(addr)		in_be32((u32 __iomem *)(addr))
+#define xspi_out16(addr, value)	out_be16((u16 __iomem *)(addr), (value))
+#define xspi_out32(addr, value)	out_be32((u32 __iomem *)(addr), (value))

I'm rather used to seeing I/O addressses passed around as "void __iomem *" so those sorts of cast are not needed... :)

drivers/spi/xilinx_spi.c line 96

Quoted 3 lines
drivers/spi/xilinx_spi.c new file +3 -0
+
+static void xspi_abort_transfer(u8 __iomem *regs_base)
+{

You should not need an abort primitive. This is called only in the remove-controller path. By the time it's called, every child spi_device on this bus segment should have been removed ... which means any spi_driver attached to that device has already returned from its remove() method, which in turn means that there will be no spi_message objects in flight from any of those drivers.

drivers/spi/xilinx_spi.c line 136

Quoted 11 lines
drivers/spi/xilinx_spi.c new file +11 -0
+static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
+{
+	struct xilinx_spi *xspi;
+	u8 __iomem *regs_base;
+
+	xspi = spi_master_get_devdata(spi->master);
+	regs_base = xspi->regs;
+
+	if (is_on == BITBANG_CS_INACTIVE) {
+		/* Deselect the slave on the SPI bus */
+		xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);

I take it you can't support SPI_CS_HIGH??

Quoted 5 lines
drivers/spi/xilinx_spi.c new file +5 -0
+/* spi_bitbang requires custom setup_transfer() to be defined if there is a
+ * custom txrx_bufs(). We have nothing to setup here as the SPI IP block
+ * supports just 8 bits per word, and SPI clock can't be changed in software.
+ * Check for 8 bits per word; speed_hz checking could be added if the SPI
+ * clock information is available. Chip select delay calculations could be

drivers/spi/xilinx_spi.c line 172

Quoted 10 lines
drivers/spi/xilinx_spi.c new file +10 -0
+ * added here as soon as bitbang_work() can be made aware of the delay value.
+ */
+static int xilinx_spi_setup_transfer(struct spi_device *spi,
+				     struct spi_transfer *t)
+{
+	u8 bits_per_word;
+
+	bits_per_word = (t) ? t->bits_per_word : spi->bits_per_word;
+	if (bits_per_word != 8)
+		return -EINVAL;

Speed checking *SHOULD* be added; the clock info can be platform data.

It would make trouble if a driver needed to turn the clock down to, say, 400 KHz for some operation, and the controller said "yes" but kept it at 25 MHz or whatever. It's OK if the driver is told that 400 KHz can't happen though -- it's possible to recover from that.

(Although in practice it's best to have the transfer method do the error checking, so that messages that will fail do so before they are allowed to enter the I/O queue.)

ISTR you may need to delegate to the default method here too, but it's been a while since I poked at that level and the issue might not apply to this particular driver config.

drivers/spi/xilinx_spi.c line 185

Quoted 13 lines
drivers/spi/xilinx_spi.c new file +13 -0
+
+	return 0;
+}
+
+
+static int xilinx_spi_setup(struct spi_device *spi)
+{
+	struct spi_bitbang *bitbang;
+	struct xilinx_spi *xspi;
+	int retval;
+
+	xspi = spi_master_get_devdata(spi->master);
+	bitbang = &xspi->bitbang;

You need to verify ALL the input parameters. In particular, mask spi->mode against all the values this driver recognizes and supports. If you don't support SPI_LSB_FIRST it's a bug if setup() succeeds after setting that. Same thing with all other bits defined today (SPI_3WIRE, SPI_CS_HIGH) and in the future...

For an example, see the atmel_spi.c driver and MODEBITS. There's a patch in MM that makes all the SPI controller drivers have analagous tests ... and for bitbang there's a patch adding spi_bitbang.mode flags to declare any extra spi->mode flags that should be accepted.

drivers/spi/xilinx_spi.c line 265

Quoted 12 lines
drivers/spi/xilinx_spi.c new file +12 -0
+	...
+	
+static irqreturn_t xilinx_spi_irq(int irq, void *dev_id)
+{
+	struct xilinx_spi *xspi;
+	u8 __iomem *regs_base;
+	u32 ipif_isr;
+
+	xspi = (struct xilinx_spi *) dev_id;
+	regs_base = xspi->regs;
+
+     	/* Get the IPIF inetrrupts, and clear them immediately */

Spell checkers will tell you this is "interrupts" ... ;)

Quoted 29 lines
drivers/spi/xilinx_spi.c new file +29 -0
+	ipif_isr = xspi_in32(regs_base + XIPIF_V123B_IISR_OFFSET);
+	xspi_out32(regs_base + XIPIF_V123B_IISR_OFFSET, ipif_isr);
+
+	if (ipif_isr & XSPI_INTR_TX_EMPTY) {	/* Transmission completed */
+		u16 cr;
+		u8 sr;
+
+		/* A transmit has just completed. Process received data and
+		 * check for more data to transmit. Always inhibit the
+		 * transmitter while the Isr refills the transmit register/FIFO,
+		 * or make sure it is stopped if we're done.
+	         */
+		cr = xspi_in16(regs_base + XSPI_CR_OFFSET);
+		xspi_out16(regs_base + XSPI_CR_OFFSET,
+			   cr | XSPI_CR_TRANS_INHIBIT);
+
+		/* Read out all the data from the Rx FIFO */
+		sr = in_8(regs_base + XSPI_SR_OFFSET);
+		while ((sr & XSPI_SR_RX_EMPTY_MASK) == 0) {
+			u8 data;
+
+			data = in_8(regs_base + XSPI_RXD_OFFSET);
+			if (xspi->rx_ptr) {
+				*xspi->rx_ptr++ = data;
+			}
+			sr = in_8(regs_base + XSPI_SR_OFFSET);
+		}
+
+	        /* See if there is more data to send */

drivers/spi/xilinx_spi.c line 304

Quoted 10 lines
drivers/spi/xilinx_spi.c new file +10 -0
+		if (xspi->remaining_bytes > 0) {
+			/* sr content is valid here; no need for io_8() */
+			while ((sr & XSPI_SR_TX_FULL_MASK) == 0
+				&& xspi->remaining_bytes > 0) {
+				if (xspi->tx_ptr) {
+					out_8(regs_base + XSPI_TXD_OFFSET,
+					      *xspi->tx_ptr++);
+				} else {
+					out_8(regs_base + XSPI_TXD_OFFSET, 0);
+				}

This duplicates the loop in txrx_bufs(); that's bad style. Have one routine holding the shared code.

Quoted 28 lines
drivers/spi/xilinx_spi.c new file +28 -0
+static int __init xilinx_spi_probe(struct platform_device *dev)
+{
+	int ret = 0;
+	struct spi_master *master;
+	struct xilinx_spi *xspi;
+	struct xspi_platform_data *pdata;
+	struct resource *r;
+
+	/* Get resources(memory, IRQ) associated with the device */
+	master = spi_alloc_master(&dev->dev, sizeof(struct xilinx_spi));
+
+	if (master == NULL) {
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(dev, master);
+	pdata = dev->dev.platform_data;
+
+	if (pdata == NULL) {
+		ret = -ENODEV;
+		goto put_master;
+	}
+
+	r = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (r == NULL) {
+		ret = -ENODEV;
+		goto put_master;
+	}

drivers/spi/xilinx_spi.c line 362

Quoted 10 lines
drivers/spi/xilinx_spi.c new file +10 -0
+
+	xspi = spi_master_get_devdata(master);
+	xspi->bitbang.master = spi_master_get(master);
+	xspi->bitbang.chipselect = xilinx_spi_chipselect;
+	xspi->bitbang.setup_transfer = xilinx_spi_setup_transfer;
+	xspi->bitbang.txrx_bufs = xilinx_spi_txrx_bufs;
+	xspi->bitbang.master->setup = xilinx_spi_setup;
+	init_completion(&xspi->done);
+
+	xspi->regs = ioremap(r->start, r->end - r->start + 1);

Strictly speaking a request_region() should precede the ioremap, but a lot of folk don't bother. However, lacking that I'd put the request_irq() earlier, since that will be the only resource providing any guard against another driver sharing the hardware.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

#13 lore raw

Hi David,

Thanks for reviewing this stuff!

Quoted 24 lines · David Brownell
Quoted 2 lines · Andrei Konovalov

Would be nice to get this driver into mainline.
Reviews and comments are welcome.

I'll ignore the Kconfig flamage ... ;)

Quoted 17 lines
drivers/spi/xilinx_spi.c new file +14 -0
@@ -0,0 +1,447 @@
+/*
+ * xilinx_spi.c
+ *
+ * Xilinx SPI controler driver (master mode only)
+ *
+ * Author: MontaVista Software, Inc.
+ *         [email protected]
+ *
+ * 2002-2007 (c) MontaVista Software, Inc.  This file is licensed under the
+ * terms of the GNU General Public License version 2.  This program is licensed
+ * "as is" without any warranty of any kind, whether express or implied.
+ */
+
+
Quoted 8 lines

drivers/spi/xilinx_spi.c line 78

Quoted 5 lines
drivers/spi/xilinx_spi.c new file +5 -0
+/* Simple macros to get the code more readable */
+#define xspi_in16(addr)		in_be16((u16 __iomem *)(addr))
+#define xspi_in32(addr)		in_be32((u32 __iomem *)(addr))
+#define xspi_out16(addr, value)	out_be16((u16 __iomem *)(addr), (value))
+#define xspi_out32(addr, value)	out_be32((u32 __iomem *)(addr), (value))

I'm rather used to seeing I/O addressses passed around as "void __iomem *" so those sorts of cast are not needed... :)

I've decided to make the base_address (u8 __iomem *) to make it easier to add the register offsets to the base. Like that:

static void xspi_init_hw(u8 __iomem *regs_base)

{
	/* Reset the SPI device */
	xspi_out32(regs_base + XIPIF_V123B_RESETR_OFFSET,
		   XIPIF_V123B_RESET_MASK);

...

Without the cast, out_be32(regs_base + XIPIF_V123B_RESETR_OFFSET, XIPIF_V123B_RESET_MASK) produces a warning.

Another solution could be

#define XSPI_REG(base, offset) (void *)((base) + (offset)) and

	/* Reset the SPI device */
	out_be32(XSPI_REG(regs_base, XIPIF_V123B_RESETR_OFFSET),
	         XIPIF_V123B_RESET_MASK);

Is it better?

Quoted 11 lines

drivers/spi/xilinx_spi.c line 96

Quoted 3 lines
drivers/spi/xilinx_spi.c new file +3 -0
+
+static void xspi_abort_transfer(u8 __iomem *regs_base)
+{

You should not need an abort primitive. This is called only in the remove-controller path. By the time it's called, every child spi_device on this bus segment should have been removed ... which means any spi_driver attached to that device has already returned from its remove() method, which in turn means that there will be no spi_message objects in flight from any of those drivers.

I am paranoid enough not to rely 100% on the spi_device's and the spi_bitbang doing all that :)
Agreed, xspi_abort_transfer is bad name here. But I still would like to leave these two writes:

+ /* Deselect the slave on the SPI bus */
+ xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);
+ /* Disable the transmitter */
+ xspi_out16(regs_base + XSPI_CR_OFFSET,
+ XSPI_CR_TRANS_INHIBIT | XSPI_CR_MANUAL_SSELECT);

in xilinx_spi_remove(). Just in case :)

Quoted 5 lines
Quoted 5 lines
drivers/spi/xilinx_spi.c new file +5 -0
+static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
+{
+	struct xilinx_spi *xspi;
+	u8 __iomem *regs_base;
+
Quoted 8 lines

drivers/spi/xilinx_spi.c line 136

Quoted 6 lines
drivers/spi/xilinx_spi.c new file +6 -0
+	xspi = spi_master_get_devdata(spi->master);
+	regs_base = xspi->regs;
+
+	if (is_on == BITBANG_CS_INACTIVE) {
+		/* Deselect the slave on the SPI bus */
+		xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);

I take it you can't support SPI_CS_HIGH??

There is no clear indication in the SPI controller docs that SPI_CS_HIGH would work. I suspect it would as we don't use the ability of the controller to generate the chip selects automatically (the auto mode doesn't support SPI_CS_HIGH). But it is more safe to advertise SPI_CS_HIGH is not supported.

Quoted 18 lines
Quoted 5 lines
drivers/spi/xilinx_spi.c new file +5 -0
+/* spi_bitbang requires custom setup_transfer() to be defined if there is a
+ * custom txrx_bufs(). We have nothing to setup here as the SPI IP block
+ * supports just 8 bits per word, and SPI clock can't be changed in software.
+ * Check for 8 bits per word; speed_hz checking could be added if the SPI
+ * clock information is available. Chip select delay calculations could be

drivers/spi/xilinx_spi.c line 172

Quoted 10 lines
drivers/spi/xilinx_spi.c new file +10 -0
+ * added here as soon as bitbang_work() can be made aware of the delay value.
+ */
+static int xilinx_spi_setup_transfer(struct spi_device *spi,
+				     struct spi_transfer *t)
+{
+	u8 bits_per_word;
+
+	bits_per_word = (t) ? t->bits_per_word : spi->bits_per_word;
+	if (bits_per_word != 8)
+		return -EINVAL;

Speed checking *SHOULD* be added; the clock info can be platform data.

Quoted 4 lines

It would make trouble if a driver needed to turn the clock down to, say, 400 KHz for some operation, and the controller said "yes" but kept it at 25 MHz or whatever. It's OK if the driver is told that 400 KHz can't happen though -- it's possible to recover from that.

OK

Quoted 31 lines

(Although in practice it's best to have the transfer method do the error checking, so that messages that will fail do so before they are allowed to enter the I/O queue.)

ISTR you may need to delegate to the default method here too, but it's been a while since I poked at that level and the issue might not apply to this particular driver config.

drivers/spi/xilinx_spi.c line 185

Quoted 13 lines
drivers/spi/xilinx_spi.c new file +13 -0
+
+	return 0;
+}
+
+
+static int xilinx_spi_setup(struct spi_device *spi)
+{
+	struct spi_bitbang *bitbang;
+	struct xilinx_spi *xspi;
+	int retval;
+
+	xspi = spi_master_get_devdata(spi->master);
+	bitbang = &xspi->bitbang;

You need to verify ALL the input parameters. In particular, mask spi->mode against all the values this driver recognizes and supports. If you don't support SPI_LSB_FIRST it's a bug if setup() succeeds after setting that. Same thing with all other bits defined today (SPI_3WIRE, SPI_CS_HIGH) and in the future...

Quoted 5 lines

For an example, see the atmel_spi.c driver and MODEBITS. There's a patch in MM that makes all the SPI controller drivers have analagous tests ... and for bitbang there's a patch adding spi_bitbang.mode flags to declare any extra spi->mode flags that should be accepted.

OK. Thanks.

Quoted 6 lines
Quoted 6 lines
drivers/spi/xilinx_spi.c new file +6 -0
+	...
+	
+static irqreturn_t xilinx_spi_irq(int irq, void *dev_id)
+{
+	struct xilinx_spi *xspi;
+	u8 __iomem *regs_base;
Quoted 8 lines

drivers/spi/xilinx_spi.c line 265

Quoted 6 lines
drivers/spi/xilinx_spi.c new file +6 -0
+	u32 ipif_isr;
+
+	xspi = (struct xilinx_spi *) dev_id;
+	regs_base = xspi->regs;
+
+     	/* Get the IPIF inetrrupts, and clear them immediately */

Spell checkers will tell you this is "interrupts" ... ;)

Oops...

Quoted 34 lines
Quoted 34 lines
drivers/spi/xilinx_spi.c new file +34 -0
+	ipif_isr = xspi_in32(regs_base + XIPIF_V123B_IISR_OFFSET);
+	xspi_out32(regs_base + XIPIF_V123B_IISR_OFFSET, ipif_isr);
+
+	if (ipif_isr & XSPI_INTR_TX_EMPTY) {	/* Transmission completed */
+		u16 cr;
+		u8 sr;
+
+		/* A transmit has just completed. Process received data and
+		 * check for more data to transmit. Always inhibit the
+		 * transmitter while the Isr refills the transmit register/FIFO,
+		 * or make sure it is stopped if we're done.
+	         */
+		cr = xspi_in16(regs_base + XSPI_CR_OFFSET);
+		xspi_out16(regs_base + XSPI_CR_OFFSET,
+			   cr | XSPI_CR_TRANS_INHIBIT);
+
+		/* Read out all the data from the Rx FIFO */
+		sr = in_8(regs_base + XSPI_SR_OFFSET);
+		while ((sr & XSPI_SR_RX_EMPTY_MASK) == 0) {
+			u8 data;
+
+			data = in_8(regs_base + XSPI_RXD_OFFSET);
+			if (xspi->rx_ptr) {
+				*xspi->rx_ptr++ = data;
+			}
+			sr = in_8(regs_base + XSPI_SR_OFFSET);
+		}
+
+	        /* See if there is more data to send */
+		if (xspi->remaining_bytes > 0) {
+			/* sr content is valid here; no need for io_8() */
+			while ((sr & XSPI_SR_TX_FULL_MASK) == 0
+				&& xspi->remaining_bytes > 0) {
+				if (xspi->tx_ptr) {
Quoted 8 lines

drivers/spi/xilinx_spi.c line 304

Quoted 5 lines
drivers/spi/xilinx_spi.c new file +5 -0
+					out_8(regs_base + XSPI_TXD_OFFSET,
+					      *xspi->tx_ptr++);
+				} else {
+					out_8(regs_base + XSPI_TXD_OFFSET, 0);
+				}

This duplicates the loop in txrx_bufs(); that's bad style. Have one routine holding the shared code.

OK

Quoted 35 lines
Quoted 35 lines
drivers/spi/xilinx_spi.c new file +35 -0
+static int __init xilinx_spi_probe(struct platform_device *dev)
+{
+	int ret = 0;
+	struct spi_master *master;
+	struct xilinx_spi *xspi;
+	struct xspi_platform_data *pdata;
+	struct resource *r;
+
+	/* Get resources(memory, IRQ) associated with the device */
+	master = spi_alloc_master(&dev->dev, sizeof(struct xilinx_spi));
+
+	if (master == NULL) {
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(dev, master);
+	pdata = dev->dev.platform_data;
+
+	if (pdata == NULL) {
+		ret = -ENODEV;
+		goto put_master;
+	}
+
+	r = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (r == NULL) {
+		ret = -ENODEV;
+		goto put_master;
+	}
+
+	xspi = spi_master_get_devdata(master);
+	xspi->bitbang.master = spi_master_get(master);
+	xspi->bitbang.chipselect = xilinx_spi_chipselect;
+	xspi->bitbang.setup_transfer = xilinx_spi_setup_transfer;
+	xspi->bitbang.txrx_bufs = xilinx_spi_txrx_bufs;
+	xspi->bitbang.master->setup = xilinx_spi_setup;
Quoted 8 lines

drivers/spi/xilinx_spi.c line 362

Quoted 3 lines
drivers/spi/xilinx_spi.c new file +3 -0
+	init_completion(&xspi->done);
+
+	xspi->regs = ioremap(r->start, r->end - r->start + 1);

Strictly speaking a request_region() should precede the ioremap, but a lot of folk don't bother. However, lacking that I'd put the request_irq() earlier, since that will be the only resource providing any guard against another driver sharing the hardware.

Will add request_region(). I was confused by platform_device_register() doing something with the resources (contrary to the "plain" device_register()).

Thanks,
Andrei

A message that is not in the archive (sent off-list or not yet received): [email protected]

#15 lore raw
Quoted 11 lines · Andrei Konovalov
Quoted 8 lines

drivers/spi/xilinx_spi.c line 78

Quoted 5 lines
drivers/spi/xilinx_spi.c new file +5 -0
+/* Simple macros to get the code more readable */
+#define xspi_in16(addr)		in_be16((u16 __iomem *)(addr))
+#define xspi_in32(addr)		in_be32((u32 __iomem *)(addr))
+#define xspi_out16(addr, value)	out_be16((u16 __iomem *)(addr), (value))
+#define xspi_out32(addr, value)	out_be32((u32 __iomem *)(addr), (value))

I'm rather used to seeing I/O addressses passed around as "void __iomem *" so those sorts of cast are not needed... :)

I've decided to make the base_address (u8 __iomem *) to make it easier to add the register offsets to the base. Like that:

Adding register offsets works with "void __iomem *" too ...

Quoted 13 lines
Quoted 1 line

You should not need an abort primitive. ...

I am paranoid enough not to rely 100% on the spi_device's and the spi_bitbang doing all that :)
Agreed, xspi_abort_transfer is bad name here. But I still would like to leave these two writes:

drivers/spi/xilinx_spi.c new file +5 -0
+	/* Deselect the slave on the SPI bus */
+	xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);
+	/* Disable the transmitter */
+	xspi_out16(regs_base + XSPI_CR_OFFSET,
+		   XSPI_CR_TRANS_INHIBIT | XSPI_CR_MANUAL_SSELECT);
Quoted 1 line

in xilinx_spi_remove(). Just in case :)

What happens when you add a BUG_ON to catch the device still being active?

Really, you need to rely on the rest of the system working correctly.

Quoted 6 lines
Quoted 1 line

I take it you can't support SPI_CS_HIGH??

There is no clear indication in the SPI controller docs that SPI_CS_HIGH would work. I suspect it would as we don't use the ability of the controller to generate the chip selects automatically (the auto mode doesn't support SPI_CS_HIGH). But it is more safe to advertise SPI_CS_HIGH is not supported.

If you don't explicitly support it, it's unlikely to work. :)

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

A message that is not in the archive (sent off-list or not yet received): 200706062209.09731.david-b-yBeKhBN/[email protected]

author
#17 lore raw

Hi David,

In the bottom there is the interdiff against the previous version of the patch. The new patch is attached to this message.

Thanks,
Andrei

Quoted 5 lines · David Brownell
Quoted 2 lines · Andrei Konovalov

Would be nice to get this driver into mainline.
Reviews and comments are welcome.

I'll ignore the Kconfig flamage ... ;)

I've left untouched XILINX_VIRTEX in

+config SPI_XILINX
+ tristate "Xilinx SPI controller"
+ depends on SPI_MASTER && XILINX_VIRTEX && EXPERIMENTAL

as in the current kernel there is no other symbol to "depend on". If / when someone comes with a separate patch to add XILINX_DRIVERS or whatever suitable this "depends on" line can be revisited.

Quoted 17 lines
Quoted 17 lines
drivers/spi/xilinx_spi.c new file +14 -0
@@ -0,0 +1,447 @@
+/*
+ * xilinx_spi.c
+ *
+ * Xilinx SPI controler driver (master mode only)
+ *
+ * Author: MontaVista Software, Inc.
+ *         [email protected]
+ *
+ * 2002-2007 (c) MontaVista Software, Inc.  This file is licensed under the
+ * terms of the GNU General Public License version 2.  This program is licensed
+ * "as is" without any warranty of any kind, whether express or implied.
+ */
+
+
Quoted 8 lines

drivers/spi/xilinx_spi.c line 19

Quoted 5 lines
drivers/spi/xilinx_spi.c new file +5 -0
+/* Simple macros to get the code more readable */
+#define xspi_in16(addr)		in_be16((u16 __iomem *)(addr))
+#define xspi_in32(addr)		in_be32((u32 __iomem *)(addr))
+#define xspi_out16(addr, value)	out_be16((u16 __iomem *)(addr), (value))
+#define xspi_out32(addr, value)	out_be32((u32 __iomem *)(addr), (value))

I'm rather used to seeing I/O addressses passed around as "void __iomem *" so those sorts of cast are not needed... :)

The macros has been removed, using "void __iomem *" now.

Quoted 11 lines

drivers/spi/xilinx_spi.c line 22

Quoted 3 lines
drivers/spi/xilinx_spi.c new file +3 -0
+
+static void xspi_abort_transfer(u8 __iomem *regs_base)
+{

You should not need an abort primitive. This is called only in the remove-controller path. By the time it's called, every child spi_device on this bus segment should have been removed ... which means any spi_driver attached to that device has already returned from its remove() method, which in turn means that there will be no spi_message objects in flight from any of those drivers.

xspi_abort_transfer() removed

Quoted 5 lines
Quoted 5 lines
drivers/spi/xilinx_spi.c new file +5 -0
+static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
+{
+	struct xilinx_spi *xspi;
+	u8 __iomem *regs_base;
+
Quoted 8 lines

drivers/spi/xilinx_spi.c line 33

Quoted 6 lines
drivers/spi/xilinx_spi.c new file +6 -0
+	xspi = spi_master_get_devdata(spi->master);
+	regs_base = xspi->regs;
+
+	if (is_on == BITBANG_CS_INACTIVE) {
+		/* Deselect the slave on the SPI bus */
+		xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);

I take it you can't support SPI_CS_HIGH??

Yes.

Quoted 9 lines
Quoted 9 lines
drivers/spi/xilinx_spi.c new file +9 -0
+/* spi_bitbang requires custom setup_transfer() to be defined if there is a
+ * custom txrx_bufs(). We have nothing to setup here as the SPI IP block
+ * supports just 8 bits per word, and SPI clock can't be changed in software.
+ * Check for 8 bits per word; speed_hz checking could be added if the SPI
+ * clock information is available. Chip select delay calculations could be
+ * added here as soon as bitbang_work() can be made aware of the delay value.
+ */
+static int xilinx_spi_setup_transfer(struct spi_device *spi,
+				     struct spi_transfer *t)
Quoted 8 lines

drivers/spi/xilinx_spi.c line 48

Quoted 6 lines
drivers/spi/xilinx_spi.c new file +6 -0
+{
+	u8 bits_per_word;
+
+	bits_per_word = (t) ? t->bits_per_word : spi->bits_per_word;
+	if (bits_per_word != 8)
+		return -EINVAL;

Speed checking *SHOULD* be added; the clock info can be platform data.

Speed checking has be added.

The current problem is that EDK (the tool to create the FPGA "image" thus configuring what devices are in it) doesn't put the SPI clock information into any *.c or *.h file it generates. I've tried to address that in the "enable SPI driver for ML300" patch, but this doesn't affect the driver patch.

Quoted 7 lines

(Although in practice it's best to have the transfer method do the error checking, so that messages that will fail do so before they are allowed to enter the I/O queue.)

ISTR you may need to delegate to the default method here too, but it's been a while since I poked at that level and the issue might not apply to this particular driver config.

As far as I can tell, for the drivers relying on spi_bitbabg the setup_transfer() method is the right place for the speed checking.

Quoted 12 lines
Quoted 12 lines
drivers/spi/xilinx_spi.c new file +12 -0
+
+	return 0;
+}
+
+
+static int xilinx_spi_setup(struct spi_device *spi)
+{
+	struct spi_bitbang *bitbang;
+	struct xilinx_spi *xspi;
+	int retval;
+
+	xspi = spi_master_get_devdata(spi->master);
Quoted 8 lines

drivers/spi/xilinx_spi.c line 61

Quoted 1 line
drivers/spi/xilinx_spi.c new file +1 -0
+	bitbang = &xspi->bitbang;

You need to verify ALL the input parameters. In particular, mask spi->mode against all the values this driver recognizes and supports. If you don't support SPI_LSB_FIRST it's a bug if setup() succeeds after setting that. Same thing with all other bits defined today (SPI_3WIRE, SPI_CS_HIGH) and in the future...

Done

Quoted 6 lines
Quoted 6 lines
drivers/spi/xilinx_spi.c new file +6 -0
+	...
+	
+static irqreturn_t xilinx_spi_irq(int irq, void *dev_id)
+{
+	struct xilinx_spi *xspi;
+	u8 __iomem *regs_base;
Quoted 8 lines

drivers/spi/xilinx_spi.c line 73

Quoted 6 lines
drivers/spi/xilinx_spi.c new file +6 -0
+	u32 ipif_isr;
+
+	xspi = (struct xilinx_spi *) dev_id;
+	regs_base = xspi->regs;
+
+     	/* Get the IPIF inetrrupts, and clear them immediately */

Spell checkers will tell you this is "interrupts" ... ;)

Fixed

Quoted 34 lines
Quoted 34 lines
drivers/spi/xilinx_spi.c new file +34 -0
+	ipif_isr = xspi_in32(regs_base + XIPIF_V123B_IISR_OFFSET);
+	xspi_out32(regs_base + XIPIF_V123B_IISR_OFFSET, ipif_isr);
+
+	if (ipif_isr & XSPI_INTR_TX_EMPTY) {	/* Transmission completed */
+		u16 cr;
+		u8 sr;
+
+		/* A transmit has just completed. Process received data and
+		 * check for more data to transmit. Always inhibit the
+		 * transmitter while the Isr refills the transmit register/FIFO,
+		 * or make sure it is stopped if we're done.
+	         */
+		cr = xspi_in16(regs_base + XSPI_CR_OFFSET);
+		xspi_out16(regs_base + XSPI_CR_OFFSET,
+			   cr | XSPI_CR_TRANS_INHIBIT);
+
+		/* Read out all the data from the Rx FIFO */
+		sr = in_8(regs_base + XSPI_SR_OFFSET);
+		while ((sr & XSPI_SR_RX_EMPTY_MASK) == 0) {
+			u8 data;
+
+			data = in_8(regs_base + XSPI_RXD_OFFSET);
+			if (xspi->rx_ptr) {
+				*xspi->rx_ptr++ = data;
+			}
+			sr = in_8(regs_base + XSPI_SR_OFFSET);
+		}
+
+	        /* See if there is more data to send */
+		if (xspi->remaining_bytes > 0) {
+			/* sr content is valid here; no need for io_8() */
+			while ((sr & XSPI_SR_TX_FULL_MASK) == 0
+				&& xspi->remaining_bytes > 0) {
+				if (xspi->tx_ptr) {
Quoted 8 lines

drivers/spi/xilinx_spi.c line 112

Quoted 5 lines
drivers/spi/xilinx_spi.c new file +5 -0
+					out_8(regs_base + XSPI_TXD_OFFSET,
+					      *xspi->tx_ptr++);
+				} else {
+					out_8(regs_base + XSPI_TXD_OFFSET, 0);
+				}

This duplicates the loop in txrx_bufs(); that's bad style. Have one routine holding the shared code.

Moved into separate function.

Quoted 35 lines
Quoted 35 lines
drivers/spi/xilinx_spi.c new file +35 -0
+static int __init xilinx_spi_probe(struct platform_device *dev)
+{
+	int ret = 0;
+	struct spi_master *master;
+	struct xilinx_spi *xspi;
+	struct xspi_platform_data *pdata;
+	struct resource *r;
+
+	/* Get resources(memory, IRQ) associated with the device */
+	master = spi_alloc_master(&dev->dev, sizeof(struct xilinx_spi));
+
+	if (master == NULL) {
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(dev, master);
+	pdata = dev->dev.platform_data;
+
+	if (pdata == NULL) {
+		ret = -ENODEV;
+		goto put_master;
+	}
+
+	r = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (r == NULL) {
+		ret = -ENODEV;
+		goto put_master;
+	}
+
+	xspi = spi_master_get_devdata(master);
+	xspi->bitbang.master = spi_master_get(master);
+	xspi->bitbang.chipselect = xilinx_spi_chipselect;
+	xspi->bitbang.setup_transfer = xilinx_spi_setup_transfer;
+	xspi->bitbang.txrx_bufs = xilinx_spi_txrx_bufs;
+	xspi->bitbang.master->setup = xilinx_spi_setup;
Quoted 8 lines

drivers/spi/xilinx_spi.c line 150

Quoted 3 lines
drivers/spi/xilinx_spi.c new file +3 -0
+	init_completion(&xspi->done);
+
+	xspi->regs = ioremap(r->start, r->end - r->start + 1);

Strictly speaking a request_region() should precede the ioremap, but a lot of folk don't bother. However, lacking that I'd put the request_irq() earlier, since that will be the only resource providing any guard against another driver sharing the hardware.

request_region() added

Here is the interdiff against the previous version of the patch:

arch/ppc/syslib/virtex_devices.h +1 -0
diff -u b/arch/ppc/syslib/virtex_devices.h b/arch/ppc/syslib/virtex_devices.h
@@ -49,6 +49,7 @@
 struct xspi_platform_data {
 	s16 bus_num;
 	u16 num_chipselect;
+	u32 speed_hz;
 };
 
 #endif  /* __ASM_VIRTEX_DEVICES_H__ */
drivers/spi/xilinx_spi.c +85 -94

327 lines not on this page · raw message

Simple driver for Xilinx SPI controler.

From: Andrei Konovalov <[email protected]>

Only master mode is supported. No support for multiple masters. Slave mode is not supported either.
Not using level 1 drivers from EDK.

Signed-off-by Yuri Frolov <[email protected]>
Signed-off-by Andrei Konovalov <[email protected]>

---
---

 arch/ppc/syslib/virtex_devices.h |    7 +
 drivers/spi/Kconfig              |    9 +
 drivers/spi/Makefile             |    1 
 drivers/spi/xilinx_spi.c         |  438 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 455 insertions(+), 0 deletions(-)
arch/ppc/syslib/virtex_devices.h +7 -0
index b49dc61..420a634 100644
@@ -31,4 +31,11 @@ struct xtemac_platform_data {
  */
 int virtex_device_fixup(struct platform_device *dev);
 
+/* SPI Controller IP */
+struct xspi_platform_data {
+	s16 bus_num;
+	u16 num_chipselect;
+	u32 speed_hz;
+};
+
 #endif  /* __ASM_VIRTEX_DEVICES_H__ */
drivers/spi/Kconfig +9 -0
index 5e3f748..851d651 100644
@@ -156,6 +156,15 @@ config SPI_S3C24XX_GPIO
 	  GPIO lines to provide the SPI bus. This can be used where
 	  the inbuilt hardware cannot provide the transfer mode, or
 	  where the board is using non hardware connected pins.
+
+config SPI_XILINX
+	tristate "Xilinx SPI controller"
+	depends on SPI_MASTER && XILINX_VIRTEX && EXPERIMENTAL
+	select SPI_BITBANG
+	help
+	  This enables using the SPI controller IP from Xilinx EDK in master
+	  mode. See the DS464, "OPB Serial Peripheral Interface (SPI) (v1.00e)"
+	  Product Specification document for the hardware details.
 #
 # Add new SPI master controllers in alphabetical order above this line
 #
drivers/spi/Makefile +1 -0
index 5788d86..a2412bd 100644
@@ -23,6 +23,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_spi.o
 obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
 obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
+obj-$(CONFIG_SPI_XILINX)		+= xilinx_spi.o
 # 	... add above this line ...
 
 # SPI protocol drivers (device/link on bus)
drivers/spi/xilinx_spi.c new file +438 -0

438 lines not on this page · raw message

A message that is not in the archive (sent off-list or not yet received): [email protected]

#19 lore raw
Quoted 1 line · Andrei Konovalov

The new patch is attached to this message.

Would you send a version that doesn't produce so many warnings when run against scripts/checkpatch.pl ??

The issues seem to be mostly whitespace and linelength, so that should be easy.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

A message that is not in the archive (sent off-list or not yet received): 200706120928.09176.david-b-yBeKhBN/[email protected]

author
#21 lore raw
Quoted 10 lines · David Brownell
Quoted 1 line · Andrei Konovalov

The new patch is attached to this message.

Would you send a version that doesn't produce so many warnings when run against scripts/checkpatch.pl ??

The issues seem to be mostly whitespace and linelength, so that should be easy.

- Dave

Here is the updated patch (attached).
The changes are just
- replace "#include <asm/io.h>" with "#include <linux/io.h>"
- whitespace and linelength mods

Thanks again,
Andrei

Simple driver for Xilinx SPI controler.

From: Andrei Konovalov <[email protected]>

Only master mode is supported. No support for multiple masters. Slave mode is not supported either.
Not using level 1 drivers from EDK.

Signed-off-by Yuri Frolov <[email protected]>
Signed-off-by Andrei Konovalov <[email protected]>

---

 arch/ppc/syslib/virtex_devices.h |    7 +
 drivers/spi/Kconfig              |    9 +
 drivers/spi/Makefile             |    1 
 drivers/spi/xilinx_spi.c         |  438 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 455 insertions(+), 0 deletions(-)
arch/ppc/syslib/virtex_devices.h +7 -0
index 3d4be14..9f38d92 100644
@@ -31,4 +31,11 @@ void __init virtex_early_serial_map(void);
  */
 int virtex_device_fixup(struct platform_device *dev);
 
+/* SPI Controller IP */
+struct xspi_platform_data {
+	s16 bus_num;
+	u16 num_chipselect;
+	u32 speed_hz;
+};
+
 #endif  /* __ASM_VIRTEX_DEVICES_H__ */
drivers/spi/Kconfig +9 -0
index 5e3f748..851d651 100644
@@ -156,6 +156,15 @@ config SPI_S3C24XX_GPIO
 	  GPIO lines to provide the SPI bus. This can be used where
 	  the inbuilt hardware cannot provide the transfer mode, or
 	  where the board is using non hardware connected pins.
+
+config SPI_XILINX
+	tristate "Xilinx SPI controller"
+	depends on SPI_MASTER && XILINX_VIRTEX && EXPERIMENTAL
+	select SPI_BITBANG
+	help
+	  This enables using the SPI controller IP from Xilinx EDK in master
+	  mode. See the DS464, "OPB Serial Peripheral Interface (SPI) (v1.00e)"
+	  Product Specification document for the hardware details.
 #
 # Add new SPI master controllers in alphabetical order above this line
 #
drivers/spi/Makefile +1 -0
index 5788d86..a2412bd 100644
@@ -23,6 +23,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_spi.o
 obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
 obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
+obj-$(CONFIG_SPI_XILINX)		+= xilinx_spi.o
 # 	... add above this line ...
 
 # SPI protocol drivers (device/link on bus)
drivers/spi/xilinx_spi.c new file +438 -0

438 lines not on this page · raw message