Re: [spi-devel-general] [PATCH] Simple driver for Xilinx SPI controler.

[Andrei Konovalov <akonovalov@ru.mvista.com>]

Hi David,

Thanks for reviewing this stuff!

David Brownell wrote:
> On Wednesday 06 June 2007, Andrei Konovalov wrote:
>> Would be nice to get this driver into mainline.
>> Reviews and comments are welcome.
> 
> I'll ignore the Kconfig flamage ... ;)
> 
> 
>> --- /dev/null
>> +++ b/drivers/spi/xilinx_spi.c
>> @@ -0,0 +1,447 @@
>> +/*
>> + * xilinx_spi.c
>> + *
>> + * Xilinx SPI controler driver (master mode only)
>> + *
>> + * Author: MontaVista Software, Inc.
>> + *         source@mvista.com
>> + *
>> + * 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.
>> + */
>> +
>> +
>> +/* 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?

>> +
>> +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 :)

>> +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??

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.

>> +/* 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)
>> +{
>> +	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.

OK

> (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.
> 
> 
> 
>> +
>> +	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.

OK. Thanks.

>> +	...
>> +	
>> +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" ... ;)

Oops...

>> +	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) {
>> +					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

>> +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);
> 
> 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