spi: driver for Cirrus EP93xx SPI controller

Hello,

This is second, updated revision of the series. I tried to address all the review comments given by H Hartley Sweeten, Ryan Mallon and Martin Guy.

I also tested these again with my TS-7260 board (it has EP9302 chip) using at25 and mmc_spi drivers.

Please review.

Thanks,
MW

Mika Westerberg (3):
spi: implemented driver for Cirrus EP93xx SPI controller
ep93xx: added chip revision reading function
ep93xx: SPI driver platform support code

 arch/arm/mach-ep93xx/clock.c                    |   14 +
 arch/arm/mach-ep93xx/core.c                     |   56 ++
 arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h |    1 +
 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h  |   34 +
 arch/arm/mach-ep93xx/include/mach/platform.h    |   10 +
 drivers/spi/Kconfig                             |   11 +
 drivers/spi/Makefile                            |    1 +
 drivers/spi/ep93xx_spi.c                        | 1063 +++++++++++++++++++++++
 8 files changed, 1190 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
 create mode 100644 drivers/spi/ep93xx_spi.c

This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).

Driver currently supports only interrupt driven mode but in future we may add polling mode support as well.

---
 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h |   34 +
 drivers/spi/Kconfig                            |   11 +
 drivers/spi/Makefile                           |    1 +
 drivers/spi/ep93xx_spi.c                       | 1063 ++++++++++++++++++++++++
 4 files changed, 1109 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-ep93xx/include/mach/ep93xx_spi.h
 create mode 100644 drivers/spi/ep93xx_spi.c

new file mode 100644
index 0000000..fe93d2e
@@ -0,0 +1,34 @@
+#ifndef __ASM_MACH_EP93XX_SPI_H
+#define __ASM_MACH_EP93XX_SPI_H
+
+/**
+ * struct ep93xx_spi_info - EP93xx specific SPI descriptor
+ * @num_chipselect: number of chip select GPIOs on this board
+ * @cs_control: chip select control function
+ * @data: pointer to data that is passed to @cs_control function.
+ *        Can be %NULL if not needed.
+ *
+ * This structure is passed from board support files to EP93xx SPI controller
+ * driver. It provides callback hook to control chip select GPIOs that are
+ * allocated in board support files during board initialization.
+ */
+struct ep93xx_spi_info {
+	int	num_chipselect;
+	/*
+	 * cs_control() - control board chipselect GPIO lines
+	 * @cs: chip select to control
+	 * @value: value to set the chip select line to
+	 * @data: optional data
+	 *
+	 * This function is used to control board specific GPIO lines that
+	 * are allocated as SPI chipselects. @value is either %0 or %1. It
+	 * is possibled to pass additional information using @data.
+	 *
+	 * This function is called from thread context and can sleep if
+	 * necessary.
+	 */
+	void	(*cs_control)(unsigned cs, unsigned value, void *data);
+	void	*data;
+};
+
+#endif /* __ASM_MACH_EP93XX_SPI_H */

index a191fa2..5852340 100644
@@ -117,6 +117,17 @@ config SPI_DAVINCI
 	help
 	  SPI master controller for DaVinci and DA8xx SPI modules.
 
+config SPI_EP93XX
+	tristate "Cirrus Logic EP93xx SPI controller"
+	depends on ARCH_EP93XX
+	help
+	  This enables using the Cirrus EP93xx SPI controller in master
+	  mode. This driver supports EP9301, EP9302, EP9307, EP9312 and EP9315
+	  chips.
+
+	  To compile this driver as a module, choose M here. The module will be
+	  called ep93xx_spi.
+
 config SPI_GPIO
 	tristate "GPIO-based bitbanging SPI Master"
 	depends on GENERIC_GPIO

index d7d0f89..377f845 100644
@@ -21,6 +21,7 @@ obj-$(CONFIG_SPI_DAVINCI)		+= davinci_spi.o
 obj-$(CONFIG_SPI_DESIGNWARE)		+= dw_spi.o
 obj-$(CONFIG_SPI_DW_PCI)		+= dw_spi_pci.o
 obj-$(CONFIG_SPI_DW_MMIO)		+= dw_spi_mmio.o
+obj-$(CONFIG_SPI_EP93XX)		+= ep93xx_spi.o
 obj-$(CONFIG_SPI_GPIO)			+= spi_gpio.o
 obj-$(CONFIG_SPI_IMX)			+= spi_imx.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi_lm70llp.o

1.5.6.5

Added a new function: ep93xx_chip_revision() which reads chip revision from the sysconfig register.

---
 arch/arm/mach-ep93xx/core.c                  |   14 ++++++++++++++
 arch/arm/mach-ep93xx/include/mach/platform.h |    8 ++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

index 90fb591..07572bb 100644
@@ -222,6 +222,20 @@ void ep93xx_devcfg_set_clear(unsigned int set_bits, unsigned int clear_bits)
 }
 EXPORT_SYMBOL(ep93xx_devcfg_set_clear);
 
+/**
+ * ep93xx_chip_revision() - returns the EP93xx chip revision
+ *
+ * See <mach/platform.h> for more information.
+ */
+unsigned int ep93xx_chip_revision(void)
+{
+	unsigned int v;
+
+	v = __raw_readl(EP93XX_SYSCON_SYSCFG);
+	v &= EP93XX_SYSCON_SYSCFG_REV_MASK;
+	v >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
+	return v;
+}
 
 /*************************************************************************
  * EP93xx peripheral handling

index c6dc14d..b663390 100644
@@ -33,6 +33,14 @@ static inline void ep93xx_devcfg_clear_bits(unsigned int bits)
 	ep93xx_devcfg_set_clear(0x00, bits);
 }
 
+#define EP93XX_CHIP_REV_D0	3
+#define EP93XX_CHIP_REV_D1	4
+#define EP93XX_CHIP_REV_E0	5
+#define EP93XX_CHIP_REV_E1	6
+#define EP93XX_CHIP_REV_E2	7
+
+unsigned int ep93xx_chip_revision(void);
+
 void ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr);
 void ep93xx_register_i2c(struct i2c_gpio_platform_data *data,
 			 struct i2c_board_info *devices, int num);

1.5.6.5

This patch adds platform side support code for EP93xx SPI driver. This includes clock, resources and muxing. There is a new function: ep93xx_register_spi() that can be used by board support code to register new SPI devices for the board.

Patch depends on the previous patch which adds function ep93xx_chip_revision().

---
 arch/arm/mach-ep93xx/clock.c                    |   14 +++++++
 arch/arm/mach-ep93xx/core.c                     |   42 +++++++++++++++++++++++
 arch/arm/mach-ep93xx/include/mach/ep93xx-regs.h |    1 +
 arch/arm/mach-ep93xx/include/mach/platform.h    |    2 +
 4 files changed, 59 insertions(+), 0 deletions(-)

index 5f80092..daaa799 100644
@@ -96,6 +96,10 @@ static struct clk clk_keypad = {
 	.enable_mask	= EP93XX_SYSCON_KEYTCHCLKDIV_KEN,
 	.set_rate	= set_keytchclk_rate,
 };
+static struct clk clk_spi = {
+	.parent		= &clk_xtali,
+	.rate		= EP93XX_EXT_CLK_RATE / 2,
+};
 static struct clk clk_pwm = {
 	.parent		= &clk_xtali,
 	.rate		= EP93XX_EXT_CLK_RATE,

@@ -186,6 +190,7 @@ static struct clk_lookup clocks[] = {
 	INIT_CK("ep93xx-ohci",		NULL,		&clk_usb_host),
 	INIT_CK("ep93xx-keypad",	NULL,		&clk_keypad),
 	INIT_CK("ep93xx-fb",		NULL,		&clk_video),
+	INIT_CK("ep93xx-spi",		NULL,		&clk_spi),
 	INIT_CK(NULL,			"pwm_clk",	&clk_pwm),
 	INIT_CK(NULL,			"m2p0",		&clk_m2p0),
 	INIT_CK(NULL,			"m2p1",		&clk_m2p1),

@@ -473,6 +478,14 @@ static int __init ep93xx_clock_init(void)
 	/* Initialize the pll2 derived clocks */
 	clk_usb_host.rate = clk_pll2.rate / (((value >> 28) & 0xf) + 1);
 
+	/*
+	 * EP93xx SSP clock rate was doubled in version E2. For more information
+	 * see:
+	 *     http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf
+	 */
+	if (ep93xx_chip_revision() >= EP93XX_CHIP_REV_E2)
+		clk_spi.rate *= 2;
+
 	pr_info("PLL1 running at %ld MHz, PLL2 at %ld MHz\n",
 		clk_pll1.rate / 1000000, clk_pll2.rate / 1000000);
 	pr_info("FCLK %ld MHz, HCLK %ld MHz, PCLK %ld MHz\n",

@@ -480,6 +493,7 @@ static int __init ep93xx_clock_init(void)
 		clk_p.rate / 1000000);
 
 	clkdev_add_table(clocks, ARRAY_SIZE(clocks));
+
 	return 0;
 }
 arch_initcall(ep93xx_clock_init);

index 93e2ecc..b1e096f 100644
@@ -106,6 +106,7 @@
 
 #define EP93XX_AAC_BASE			EP93XX_APB_IOMEM(0x00080000)
 
+#define EP93XX_SPI_PHYS_BASE		EP93XX_APB_PHYS(0x000a0000)
 #define EP93XX_SPI_BASE			EP93XX_APB_IOMEM(0x000a0000)
 
 #define EP93XX_IRDA_BASE		EP93XX_APB_IOMEM(0x000b0000)

index b663390..d3fb715 100644
@@ -9,6 +9,7 @@ struct i2c_board_info;
 struct platform_device;
 struct ep93xxfb_mach_info;
 struct ep93xx_keypad_platform_data;
+struct ep93xx_spi_info;
 
 struct ep93xx_eth_data
 {

@@ -42,6 +43,7 @@ static inline void ep93xx_devcfg_clear_bits(unsigned int bits)
 unsigned int ep93xx_chip_revision(void);
 
 void ep93xx_register_eth(struct ep93xx_eth_data *data, int copy_addr);
+int ep93xx_register_spi(struct ep93xx_spi_info *info);
 void ep93xx_register_i2c(struct i2c_gpio_platform_data *data,
 			 struct i2c_board_info *devices, int num);
 void ep93xx_register_fb(struct ep93xxfb_mach_info *data);

1.5.6.5

Hello Mika,

I'm ok with this part of your patch series.

Hello,

Any comments on the v2 series of the driver? If it is OK could it be considered for .35?

Thanks,
MW

Hello Mika,

This part (2/3) I am fine with.

If you want, you can add this as a separated patch to Russell's patch tracker with my Acked-by above.

For parts 1 and 3.

I tried testing them in my setup with the mmc_spi.c and sst25l.c drivers. Both did not work. For some reason the data read is always either 0x00 or 0xff.

I will try to get some time this week/weekend to look into this.

Regards,
Hartley

I got these working with the Sim.One board without modification. The data rate when dd'ing from the block device is slightly slower than the Cirrus Logic patches (222kB/s instead of 235kB/s), and I got the same kind of write errors at the same frequency as we did with the Cirrus code, but this seems to be some problem with our board. Unlike the CIrrus code, it successfully retries failed multi-block read requests using single-block reads.

In case it's useful to you, I attach the first hack of the modifications I made to the board description file
I'm not sure it's right, and I haven't coded in the use of the MMC-card-present GPIO interrupt line yet, but it "seems to work" ^TM

M
-------------- next part --------------
A non-text attachment was scrubbed...
Name: simone-add-mmc_spi-2.6.34.patch
Type: text/x-diff
Size: 2072 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100326/1ffed33d/attachment.bin>

The chip revision is normally placed in the global unsigned int
"system_rev", which is also reported by /proc/cpuinfo.
It normally seems to be set from the atags passed by the bootloader
but uboot on this platform doesn't provide that, so it remains 0.
The same applies to the very similar variables system_serial_low and
system_serial_high (which also remain 0 on this u-boot platform). Shall we also set these from the hardware on ep93xx during thus giving them non-zero values in /proc/cpuinfo too?
As to where...? The other platforms that set these variables from
hardware queries do it in the board-specific files' MACHINE_START function, but we'd have to do this in every board file, or have some comon routine and modify every mach-ep93xx board file. The only common routine all ep93xx board inits seem to have is ep93xx_init_devices. This seems the least-resistance place to do this, but has nothing to do with initlalizing devices of course. Is making another function in toe core file and modifying every board's init function preferable?

Also, I have the simpler

    system_rev = *((unsigned int *)EP93XX_SYSCON_CHIP_ID) >> 28;
    system_serial_low = *((unsigned int *)EP93XX_SECURITY_UNIQID);

but maybe explicit readl and use of long defines are preferable.

Sorry, I'm not familiar with linux kernel best practice...

M

No, that's the hardware revision number, which is different from the chip revision number.

system_rev is also what is exported via /proc/cpuinfo because userland software may need this information to adjust their runtime behaviour.

And this is exactly why it should be passed down from the bootloader. Either the bootloader is special to each hardware revision (which is command practice), or it is able to read the revision number from a bunch of pull-up/down resistors.

If the ep93xx provides a unique serial number, then this could be an idea. I would still only set it in case it has not been given by the bootloader though.

Daniel

The board revision? Right, thanks. I don't see a place to report this in cpuinfo, since it is the SoC revision, which is neither the ARM CPU it contains nor the board that conatins the SoC.

It does, world-unique, burned into the chip. Thanks again.

M

Hello all,

Just to clarify things.

'system_rev' is an unsigned int exported from arch/arm/kernel/setup.c It is filled in with the ATAG_REVISION data passed from the bootloader. What this data actually means is specific to the bootloader.

'system_serial_low' and 'system_serial_high' are also unsigned int's exported from arch/arm/kernel/setup.c. These are filled in with the ATAG_SERIAL data passed from the bootloader. Again, what this data actually means is specific to the bootloader.

These three values are exposed to the user via /proc/cpuinfo. The seq_operations that handle this are also in arch/arm/kernel/setup.c.

Mika's patch has nothing to do with these. As already noted, the chip revision needs to be determined in order to figure out the correct internal frequency of the SSP peripheral clock.

This chip revision is currently not exposed to the user in any fashion.

The ep93xx unique serial number, also called the MaverickCrunch ID, can be found in the Sercurity registers. There are actually two of them. One is a 32-bit unique ID the other is a 128-bit random ID. Random meaning random per chip not per boot since it is created by means of fuseable links on the chip that are set during manufacture. These also are not currently exposed to the user in any fashion.

I had submitted a patch previously that adds an architecture specific extension to /proc/cpuinfo that would allow exposing this information on the ep93xx. The same extension could be used on platforms like the TS-7200 to show the jumper settings on the board.

If there is any interest I will re-post that patch series.

Regards,
Hartley

Thanks.

Yes, being able to see the unique ID is certainly useful. I use this in userspace to set a different MAC address on each board for DHCP purposes in a classroom of otherwise identical diskless X terminals based on this chip.

M

Is system_rev same as cpu chip revision? I think it should contain board revision number or something similar.

Neither my platform (uses redboot):

Hardware : Technologic Systems TS-72xx SBC
Revision : 0000
Serial : 0000000000000000

I'm not the best guy to answer these. Maybe Hartley Sweeten or Ryan Mallon have some ideas about this. I didn't even know that such variable exists :)

Regards,
MW

The system_rev != the chip revision.

This is not a good idea.

system_rev, system_serial_high, and system_serial_low are all passed to the kernel by the bootloader as ATAGs. If your bootloader doesn't pass the ATAGs it should be fixed. Overriding the variables breaks what they are intended for.

I have some patches for Redboot but Cirrus still has not sent in the FSF paperwork to transfer the ep93xx port to eCos. With those patches and the /proc/cpuinfo extension patches I mentioned previously I get the following on my system:

/ # cat /proc/cpuinfo
Processor : ARM920T rev 0 (v4l)
BogoMIPS : 99.53
Features : swp half thumb crunch
CPU implementer : 0x41
CPU architecture: 4T
CPU variant : 0x1
CPU part : 0x920
CPU revision : 0

Hardware : Vision Engraving Systems EP9307
Revision : 0001
Serial : 4943410027260017

System Type : iMARC

Unique ID : 921040f8
Maverick Key : 921040f85b6f0e7b76eb23d24309b3 OK
Silicon Rev : E2
Watchdog : active
Reset duration : active
Boot mode : normal async internal 16-bit
/ #

Regards,
Hartley

Ok, thanks for clarification. It seems that in my platform, the bootloader doesn't handle these correctly.

Cool! You should definitely repost your patches.

Regards,
MW

Isn't that

...

Shouldn't that be

since max_rate is the maximum usable rate, which is sspclk / 2, so the existing code seems like it would set the divisors to give twice the requested frequency.

Conversely, if an exact divisor of the master clock rate is requested (such as 1843200), the "<" would give the next lower rate instead of the requested rate so for the highest rates the doubling and this halving would cancel out, while for lower rates you could get up to twice the requested clock frequecy.

...

Isn't that

since the output SPI clock frequency is sspclk / (cpsr * (1 + scr)) where cpsr = 2..254 step 2, scr = 1..255

I also notice that the code recalculates the clock divisors, programs them ad resotres them for every transfer, even if the frequency etc is always the same.
I don't know if this makes much difference to speed in normal operation but it does make the debug output incredibly verbose, halving transfer speeds when SPI debug is enabled.

Is there a suitable place to be lazy about this, so as to avoid doing this if the transfer parameters are the same on successive operations?

Thanks

M

Yes.

Yes, you are right. Very good observation :)

Yes.

It shouldn't. This is checked in ep93xx_spi_setup() and clocks are only calculated when speed is changed. Same thing is with ep93xx_spi_process_transfer() where clocks are only calculated when different transfer speed is requested (t->speed_hz != 0).

Thank you for your comments. I will fix these in the next revision of the patch series.

Regards,
MW

I've been staring more at this again and it looks (2 clock strangenesses and extensive control reg setting apart) like good code. I have another question: like the Cirrus driver, this takes 100% CPU doing busy wait for the current transfer to complete.
Given that this driver is interrupt-based, is there any reason why it can't do something else in the meanwhile?
Not that that's a reason not to include it in 2.6.35 - it works well and we can think whether to make it more efficient in N+1...

M

I can't see anything outstanding in the driver which would make it do that. I haven't had time to test the driver out yet, but it would be nice to fix this before it gets committed. Hopefully it shouldn't be too hard to fix. Any idea where the bulk of the time is being spent?

~Ryan

Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan at bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

Thanks :)

Well it sleeps in ep93xx_spi_process_transfer() while current transfer is complete. It shouldn't busy wait. At least that was not my intention. If you have lot of data to transfer (for example while using mmc_spi), the driver will be very busy.

But I can take a look on this and see if there is something wrong.

I had an idea that the driver could keep SPI controller interrupts enabled and start next transfer in interrupt handler but I didn't implement that yet, because first I wanted a driver that works.

Yeah, maybe we can optimize that later.

Thank you very much. It is good to hear that someone (else than me) has been using this :)

Regards,
MW

Hi,

I tried to find out whether the driver did something wrong to get into busylooping but couldn't find anything. In case of MMC/SD cards, through mmc_spi, transfer sizes are >= 512 bytes at a time and FIFO size is only 8 bytes (or words) so the CPU is expected to be pretty busy serving interrupts from the SSP.

I'm about to post updated series with your comments addressed. I also added support for polling transfers.

Right. that explains it.
The board I'm testing it with just has a single mmc/sd card on the ssp port.

The docs say "separate transmit and receive FIFO memory buffers, 16 bits wide, 8 locations deep".
Watching vmstat output with this driver, it gets 226 kB/sec reading /dev/mmcblk0 at 100% system cpu usage with 2354 interrupts and 2724 context switches per second. I don't see how that relates to 8 bytes or 8 halfwords per interrupt; it suggests 100 bytes transferred per interrupt once you remove the 100/sec of clock tick irq. Writing instead gets 228kB/sec incurring 1916 (read: 1815) irqs and 934 context switches.
Similarly, checking /proc/interrupts, writing 10MB to the raw device incurs 83221 ep93xx-spi interrupts: one per 125 bytes transferred. Is it maybe busy-waiting filling the output FIFO as fast as it is emptying?

Here's the code I'm talking about:

static int ep93xx_spi_read_write(struct ep93xx_spi *espi) {
...

        /*
         * Write as long as TX FIFO is not full. After every write we check
         * whether RX FIFO has any new data in it (and in that case we read what
         * ever was in the RX FIFO).
         */
        while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_TNF) &&
                espi->tx < t->len) {
                ep93xx_do_write(espi, t);

                if (ep93xx_spi_wait_busy(espi, SPI_TIMEOUT)) {
                        msg->status = -ETIMEDOUT;
                        return msg->status;
                }

                while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
                        espi->rx < t->len) {
                        ep93xx_do_read(espi, t);
                }
        }

...

and ep93xx_spi_wait_busy() reads a device register in an busy loop with a timeout:

static int ep93xx_spi_wait_busy(const struct ep93xx_spi *espi,
unsigned long msecs)

{
        unsigned long timeout = jiffies + msecs_to_jiffies(msecs);

        while (time_before(jiffies, timeout)) {
                if ((ep93xx_spi_read_u16(espi, SSPSR) & SSPSR_BSY) == 0)
                        return 0;
                cpu_relax();
        }

        return -ETIMEDOUT;
}

The manual says that the BSY status bit means "SSP is currently transmitting and / or receiving a frame or the transmit FIFO is non-empty."
so this would make the code
- output one byte/halfword
- wait busily until the FIFO is empty of that one datum
- maybe read one received word/byte
- loop

This seems bizarre compared to what the hardware suggests:
We only have one interrupt, which is triggered when
- the TX buffer is less than half empty (ie there is room for 4 or more words)
- the RX buffer is more than half full (ie there are four or more words to read from it)
- RX overrun
and there are status bits to tell us whether the TX and RX buffers are full, empty or half-full (the half-full bits triggering the interrupts).

My understanding ATM is that this version enables interrupts to start a transfer, immediately gets interrupted because the transmit buffer is less than half empty, and if it is supposed to be reading from the card, tramsmits a 0 byte/halfword for every byte/halfword it is supposed to be receiving.

If it's supposed to be sending, it pokes one byte/halfword into the write FIFO, waits for that to have been completely transmitted and for the FIFO to be empty, then checks if it should read anything, then writes another byte/halfword etc. and so on.

This seems completely at odds with what the hardware suggests, which would be
WRITE: fill the TX buffer, then wait for half-empty IRQ.
READ: react to half-full IRQ by emptying the RX buffer.
with some hackery because if you receive a final 1, 2 or 3 byte/halfwords, you don't get an RX interrupt.

I've tried removing the busy-wait from the read-write loop and the kernel hangs forever as soon as it looks at the SSP device. Worse, my serial console is dead so I can't be more expicit.

Am I missing something here, or is the current strategy forced by undocumented brokenness of the hardware?

Lastly, what is the meaning of conditional operator(s) in

        return t->bits_per_word ?: msg->spi->bits_per_word;

It compiles, but it's the first time I've seen this construct in 27 years of C programming. What is the "normal" syntax for this?

M

If you check the sizes of the transfers for the MMC case it is something like:

[ 1796.080000] ep93xx-spi ep93xx-spi: setup: mode 0, cpsr 2, scr 0, dss 7
[ 1796.080000] ep93xx-spi ep93xx-spi: setup: cr0 0x7
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_process_transfer: transferring 512 bytes
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_interrupt: handling interrupt
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_read_write: tx=512, rx=512
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_process_transfer: transferring 2 bytes
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_interrupt: handling interrupt
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_read_write: tx=2, rx=2
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_process_transfer: transferring 1 bytes
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_interrupt: handling interrupt
[ 1796.080000] ep93xx-spi ep93xx-spi: ep93xx_spi_read_write: tx=1, rx=1

So size of the transfer varies, depending on what kind of message is passed from mmc_spi to the driver. From the above, we can see that ep93xx_spi_read_write() transfers 512 bytes before passing control back to the calling interrupt handler.

I got the same results with polling enabled (this is with v3 which is not yet posted).

I think that when the controller is enabled (SSE is set), it is transmitting/receiving once there is data (TX). It even enables the SPI output clock only when TX data is put into TX FIFO (I checked this with oscilloscope).

I added debug print to this function (ep93xx_spi_wait_busy()) and it loops only once. If I don't wait while the bit is cleared, the controller hangs.

Since SPI is full-duplex, I think that it is enough to just perform another transfer once we get RX/TX interrupt. This also makes the driver simpler (yet not the most efficient).

At least if you see my comments above, the driver seems to do the right thing (for example it transmitted one spi_transfer at a time without busy looping).

Yup, I noticed same thing. Once BSY bit is cleared, RX FIFO can be read without hangs (or something like that).

I'm not sure.

For me, current strategy seems clear:
1. Fetch next message
1.1 Enable the SPI controller
1.2 Assert chip-select
1.3 Fetch next transfer
1.3.1 Enable interrupts (if not polling) 1.3.2 Perform the transfer
write/read to/from the FIFO
1.3.2 Disable interrupts (if not polling) ... (for all spi_transfers)
1.4 De-assert chip-select
1.5 Disable the SPI controller
1.6 call msg->complete
...

It isn't the most optimized but my purpose was to get it working first. Since I only have one ep9302 board and 2 SPI devices, it limits my testing a bit. Thanks to you I've got very valuable input related to the driver :)

If you have time, I suggest that add some printk()s to the ep93xx_spi_wait_busy() and see whether it busy loops in your device (or does some other weird things).

It is an GCC extension:

http://gcc.gnu.org/onlinedocs/gcc-4.4.3/gcc/Conditionals.html#Conditionals

I've been using it once I saw it somewhere in the kernel source.

Thanks again,
MW

If you'd like one of the MMC-only boards to test on, I can ship it to you - please provide smail mail address by private mail if that would be welcome

OK, I suggest we don't use "GNU extensions" if they can be avoided, and program in C. The GCC monoculture is ending, thanks to llvm. and those gnu-heads are not the most wise people on the planet. (OK, the ANSI committe are worse, but.... :)

M

Hi again

I've looked at this again, and v2 does send every complete block (up to 512 bytes) before it returns from an interrupt. If you factor out all the conditions that always have constant values, and those that can never happen, from the central loop:

        /*
         * Write as long as TX FIFO is not full. After every write we check
         * whether RX FIFO has any new data in it (and in that case we read what
         * ever was in the RX FIFO).
         */
        while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_TNF) &&
                espi->tx < t->len) {
                ep93xx_do_write(espi, t);

                if (ep93xx_spi_wait_busy(espi, SPI_TIMEOUT)) {
                        msg->status = -ETIMEDOUT;
                        return msg->status;
                }

                while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
                        espi->rx < t->len) {
                        ep93xx_do_read(espi, t);
                }
        }

you end up with:

        while (espi->tx < t->len) {
                ep93xx_do_write(espi, t);

                while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_BSY);

                ep93xx_do_read(espi, t);
        }

This makes each interrupt routine take about 2ms to complete. I don't know if that's unacceptably long or not.

I've coded up a version that works differently, by filling the TX FIFO and returning from the IRQ routine - it then get interrupted again when the TX FIFO is half empty (and the RX FIFO is half full), at which point it empties and fills them
and waits for another interrupt.
The end of the transfer has to be handled specially because the interrupt is held high as long as the TX FIFO is half empty or less, so we have to busy-wait while the final four words are transmitted and received, concluding the transfer.

The result is an increase in MMC card read or write speed from 232kB/s to 315kB/s and a decrease in CPU usage from 100% to 55-60%, tested with plain 1GB SD card.
A 2GB MLC card instead gets 323kB/sec in read and 319kB/sec in write (!).

SDHC cards don't work at all - they are rejected by the MMC-SPI driver (eror -38, ENOSYS: Function not implemented), the same as happens with the old CIrrus driver.

I've only changed the ep93xx_spi_read_write() function in your v2 interrupting driver: the new version is below. Would you see if it works with your other SPI hardware?

I've tried also to handle transfers with word size > 8 bits but have not been able to test these.

Cheers

M

/**
* ep93xx_spi_read_write() - perform next RX/TX transfer
* @espi: ep93xx SPI controller struct
*
* This function transfers next bytes (or words) to/from RX/TX FIFOs. If called
* several times, the whole transfer will be completed. Returns %0 when current
* transfer was not yet completed otherwise length of the transfer (>%0). In
* case of error negative errno is returned.
*
* Although this function is currently only used by interrupt handler it is
* possible that in future we support polling transfers as well: hence calling
* context can be thread or atomic.
*
* Both RX and TX sides have an 8-item-deep FIFO, and interrupts are caused by
* - the transmit FIFO being half empty or less (ie room for four more words)
* - the receive FIFO being half full or more (ie four or more words in it)
* - receive overrun
* This driver works by queuing the SPI transfer list and enabling interrupts
* in the device. This immediately causes a first interrupt because the TX FIFO
* is empty. We fill the TX FIFO and return. We are then interrupted again when
* four words have been transmitted (and received), so we empty the RX and fill
* the TX FIFOs until there are less than 4 words remaining to be transmitted.
* At that point interrupts become useless because the TX IRQ remains set while
* the last 4 words are being transmitted, and the RX IRQ won't trigger unless
* there are 4 or more words in the RX FIFO. So at the end of the transfer we
* simply busy-wait for the last words to be transmitted and received. *
* It's tempting to use the TNF (Transmit buffer not full) status bit to know
* when we have filled the TX FIFO but that makes for RX overruns because
* it queues 8 words in the TC FIFO plus the word already being transmitted
* for a total of 9, which is enough to overflow the RX FIFO.
*/
static int ep93xx_spi_read_write(struct ep93xx_spi *espi)

{
	struct spi_message *msg;
	struct spi_transfer *t;
	unsigned long flags;
	unsigned n;	/* Number of bytes to send to the TX FIFO */

	spin_lock_irqsave(&espi->lock, flags);
	msg = espi->current_msg;
	spin_unlock_irqrestore(&espi->lock, flags);

	t = msg->state;

	/* Read all received data */
	while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)
		ep93xx_do_read(espi, t);

	n = t->len - espi->tx;	/* Total number of bytes waiting to be
				 * sent to the TX FIFO for this transfer */

	switch (n) {
	default:
		/* This is the start or middle of a multi-word transfer:
		 * fill the TX FIFO as much as it is safe to do without
		 * risking RX overruns. To do this we ensure that the RX FIFO
		 * can end up with no more than 8 words in it by calculating
		 * how many words are in the RX/TX FIFOs and the TX/RX shift
		 * registers from the difference between the number of bytes
		 * we have already sent and received.
		 */
		{ unsigned words_in_flight =
			(espi->tx - espi->rx) / (bits_per_word(espi) > 8 ? 2 : 1);
		  unsigned space = 8 - words_in_flight;
		  if (n > space) n = space;
		}

		/* space and n are always >= 1 */
		do {
			ep93xx_do_write(espi, t);
		} while (--n > 0);
		break;

	case 1:
		/* Single-word transfers are common and not worth returning and
		 * causing another interrupt for. This also handles a single
		 * final word of a longer tranfer, which again is not worth
		 * causing another interrupt for.
		 * Just transmit it and wait for the reply.
		 */
		ep93xx_do_write(espi, t);
		/* Fall through to wait for and read the received word */
	case 0:
		/* All of this transfer's data has been sent to the TX FIFO.
		 * The TX and RX IRQs are now useless to tell us when the last
		 * few words have finished, so simply wait for the last 0-4
		 * words to be transmitted and for the corresponding RX data
		 * to come back in.
		 * We empty the RX FIFO while we are waiting, which is faster
		 * than first waiting and then emptying.
		 */
		{
			unsigned ssp_status;

			do {
				ssp_status = ep93xx_spi_read_u8(espi, SSPSR);
				if (ssp_status & SSPSR_RNE)
					ep93xx_do_read(espi, t);
			} while (ssp_status & SSPSR_BSY);
		}

		/* Finish emptying the RX FIFO. Even though we emptied it while
		 * waiting on BUSY, there can still be several (up to 3) words
		 * still waiting in it.  Not sure why!
		 */
		while (ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)
			ep93xx_do_read(espi, t);
		break;
	}

	/* is transfer finished? */
	if (espi->rx == t->len) {
		msg->actual_length += t->len;
		return t->len;
	}

	return 0;
}

Oops, I mean

- unsigned n; /* Number of bytes to send to the TX FIFO */
+ unsigned n; /* Number of words to send to the TX FIFO */

and

- n = t->len - espi->tx; /* Total number of bytes waiting to be
- * sent to the TX FIFO for this transfer */
+ /* Number of words still to be sent to the TX FIFO */
+ n = (t->len - espi->tx) / (bits_per_word(espi) > 8 ? 2 : 1);

sorry about that.

M

Yeah. It's probably bad thing to spend that much time in interrupt handler.

I tested your version with few SD cards (one of them is SDHC) and works without any problems (and performance is better). Also tested with SPI EEPROM through at25 driver and it works well.

However, I would really like to have this code to be simpler and to be suitable for polling as well. I wrote myself version that is based on yours and also borrowed this fifo_level thingy from amba-pl022 driver. I tested it with the same devices and performance seems to be in par with your version.

Can you try following with your devices?

struct ep93xx_spi {
	...
	size_t	fifo_level;
};

static int ep93xx_spi_read_write(struct ep93xx_spi *espi)

{
        struct spi_message *msg;
        struct spi_transfer *t;
        unsigned long flags;

        spin_lock_irqsave(&espi->lock, flags);
        msg = espi->current_msg;
        spin_unlock_irqrestore(&espi->lock, flags);

        t = msg->state;

        if (espi->tx == 0 && espi->rx == 0)
                espi->fifo_level = 0;

        while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE) &&
                espi->rx < t->len) {
                ep93xx_do_read(espi, t);
                espi->fifo_level--;
        }

        while (espi->fifo_level < 8 && espi->tx < t->len) {
                ep93xx_do_write(espi, t);
                espi->fifo_level++;
        }

        /* is transfer finished? */
        if (espi->tx == t->len && espi->rx == t->len) {
                msg->actual_length += t->len;
                return t->len;
        }

        return 0;
}

Thanks,
MW

Woo, nice one!
Same CPU usage (~57%) same throughput (315kb/s) and it's much more elegant. :) Just using repeated interrupts to handle the last 4 words instead of the nasty busy-wait loops seems fairer to other interrupts too!

You can make it even smaller be removing some redundant checks deriving from the fact that it's synchronous, so N writes always cause N reads:

can be

         if (espi->tx == 0)
                 espi->fifo_level = 0;

,

         while ((ep93xx_spi_read_u8(espi, SSPSR) & SSPSR_RNE)) {

and

         if (espi->rx == t->len) {

Cheers!

M

and

- if (espi->tx == 0 && espi->rx == 0)
- espi->fifo_level = 0;

since it's always created as 0 and is always reset to 0 by the end of a transfer

and if you like

          /* is transfer finished? */

- if (espi->tx == t->len && espi->rx == t->len) {
+ if (espi->fifo_level == 0) {

to save a couple of instructions

M