SPI: add support for PNX/SPI controller
PATCH 1/1 SPI: add support for PNX/SPI controller
Hi,
inlined is a pretty much _development_ version of the Philips SPI controller support we're working with on some Philips ARM targets (PNX0106 and PNX4008, namely) based on spi_bitbang library from David Brownell. It's quite a notable thing that this buggy and in some way complicated SPI controller fits well in this bitbang framework! :) This patch also contains dumb EEPROM driver for the onboard EEPROM behind the SPI bus. This driver was used by us for controller driver testing, mainly.
drivers/spi/Kconfig | 8 drivers/spi/Makefile | 2 drivers/spi/eeprom.c | 127 ++++++++++++ drivers/spi/spi_pnx.c | 365 ++++++++++++++++++++++++++++++++++++ drivers/spi/spi_pnx.h | 60 ++++++ include/asm-arm/arch-pnx4008/spi.h | 370 +++++++++++++++++++++++++++++++++++++ 6 files changed, 932 insertions(+)
drivers/spi/Kconfig +8 -0
@@ -95,6 +95,14 @@ config SPI_BUTTERFLY # Add new SPI master controllers in alphabetical order above this line # +config SPI_PNX + tristate "PNX SPI bus support" + depends on ARCH_PNX4008 && SPI_BITBANG + +config SPI_EEPROM + tristate "SPI EEPROM" + depends on SPI + # # There are lots of SPI device types, with sensors and memory
drivers/spi/Makefile +2 -0
@@ -13,9 +13,11 @@ obj-$(CONFIG_SPI_MASTER) += spi.o # SPI master controller drivers (bus) obj-$(CONFIG_SPI_BITBANG) += spi_bitbang.o obj-$(CONFIG_SPI_BUTTERFLY) += spi_butterfly.o +obj-$(CONFIG_SPI_PNX) += spi_pnx.o # ... add above this line ... # SPI protocol drivers (device/link on bus) +obj-$(CONFIG_SPI_EEPROM) += eeprom.o # ... add above this line ... # SPI slave controller drivers (upstream link)
drivers/spi/eeprom.c new file +127 -0
127 lines not on this page · raw message
drivers/spi/spi_pnx.c new file +365 -0
365 lines not on this page · raw message
include/asm-arm/arch-pnx4008/spi.h new file +370 -0
370 lines not on this page · raw message
drivers/spi/spi_pnx.h new file +60 -0
60 lines not on this page · raw message
Quoted 1 line · Vitaly Wool
(PNX0106 and PNX4008, namely) based on
spi_bitbanglibrary
Good! Does need work yet though...
Quoted 2 lines
It's quite a notable thing that this buggy and in some way complicated SPI controller fits well in this bitbang framework! :)
So the transfer-level hooks worked for you ... I'm glad.
Quoted 2 lines
This patch also contains dumb EEPROM driver for the onboard EEPROM behind the SPI bus. This driver was used by us for controller driver testing, mainly.
It should still be a separate patch though.
Quoted 1 line
# Add new SPI master controllers in alphabetical order above this line
Note how it says "above", yet you've added yours ... below! :(
drivers/spi/Kconfig config SPI_BUTTERFLY · line 100
Quoted 5 lines
#
drivers/spi/Kconfig +3
-0+config SPI_PNX + tristate "PNX SPI bus support" + depends on ARCH_PNX4008 && SPI_BITBANG
... also added the eeprom so it shows in the list of controller drivers!!
drivers/spi/Kconfig config SPI_BUTTERFLY
Quoted 8 lines
drivers/spi/Kconfig +6
-0+config SPI_EEPROM + tristate "SPI EEPROM" + depends on SPI + +#define lock_device( dev ) /* down( &dev->sem ); */ +#define unlock_device( dev ) /* up( &dev->sem ); */
Not clear why you'd want such stuff ... better to just remove NOPs like that.
drivers/spi/spi_pnx.c line 124
Quoted 5 lines
drivers/spi/spi_pnx.c new file +5
-0+static void +spi_pnx_chipselect (struct spi_device *spi,int is_on) +{ +spipnx_arch_cs (spi, is_on); +}
I see that "arch" hook is actually just toggling a DMA line. Did you notice how Stephen's PXA code handled that? The "controller_data" was a board-specific GPIO toggle function, as provided in the board_info that implicitly defines the relevant device.
Plus, activating the chip does more than just toggling some arch-specific GPIO. It has to set the right SPI mode, of which CPOL must be set _before_ chipselect goes active. And it has to set the transfer clock, for chips like yours which are multiplexing devices using software.
Quoted 2 lines
drivers/spi/spi_pnx.c new file +2
-0+static int spi_pnx_xfer (struct spi_device *spidev,struct spi_transfer *t) +{
...
drivers/spi/spi_pnx.c line 155
Quoted 2 lines
drivers/spi/spi_pnx.c new file +2
-0+if (spidev->max_speed_hz) +spi_pnx_set_clock_rate (spidev->max_speed_hz /1000 , regs);
Just do it the one time, when activating that chip's selection.
...
drivers/spi/spi_pnx.c line 170
Quoted 10 lines
drivers/spi/spi_pnx.c new file +10
-0+if (t->tx_buf) { + dat = (u8 *)t->tx_buf; + regs->con |=SPIPNX_CON_RXTX ; + regs->ier |=SPIPNX_IER_EOT ; + regs->con &= ~SPIPNX_CON_SHIFT_OFF ; + regs->frm = len; + +if (dd->dma_mode && len >=FIFO_CHUNK_SIZE ) { +void *dmasafe =NULL ; +if (t->tx_dma)
That's not actually correct, since zero might be a legal DMA address. Unfortunately we have no DMA_ADDR_INVALID to test against, so this will need to suffice for now. (And I updated the spi_bitbang code to ensure that address _is_ zeroed when the message didn't set up the DMA addresses already, matching the default behavior.)
drivers/spi/spi_pnx.c line 179
Quoted 9 lines
drivers/spi/spi_pnx.c new file +9
-0+ params.dma_buffer = t->tx_dma; + else { + dmasafe =kmalloc (len,SLAB_KERNEL ); +if (!dmasafe) { + len =0 ; + goto out; + } + params.dma_buffer =dma_map_single (dev->parent, dmasafe, len,DMA_TO_DEVICE ); +memcpy (dmasafe, dat, len);
You do know this is incorrect don't you? Call dma_map_single() if you need a dma address. The caller already guaranteed that the input address is dma-safe.
...
Quoted 3 lines
drivers/spi/spi_pnx.c new file +3
-0+ } + +if (t->rx_buf) {
...
Review
Quoted 1 line
(quoted diff) +1
-0+ }
That RX bit sure looks wrong to me ... as if, given an spi_transfer with both TX and RX buffers, it will execute them in sequence (half duplex) rather than concurrently (full duplex). SPI is full duplex, and this code should be too. If it can't implement that, then it should be reporting an error if both rx and tx are requested, rather than doing the wrong thing.
Quoted 1 line
+static int
spi_pnx_probe(struct device *device)
That should probably be marked __init ..
Quoted 10 lines
drivers/spi/spi_pnx.c new file +10
-0+{ +struct spi_master *master; +struct spi_pnx_data *data; +struct spi_bitbang *pnx; +int rc =0 ; + +printk ("spi probe called\n" ); + master =spi_alloc_master (device,sizeof *data); +if (!master) { + rc = -ENOMEM ;
drivers/spi/spi_pnx.c line 303
Quoted 10 lines
drivers/spi/spi_pnx.c new file +10
-0+ goto out; + } + master->setup = &spi_pnx_setup; + master->bus_num =to_platform_device (device)->id; + + pnx =spi_master_get_devdata (master); + pnx->master = master; + pnx->chipselect = &spi_pnx_chipselect; + pnx->txrx_bufs = &spi_pnx_xfer; + rc =spi_bitbang_start (pnx);
You shouldn't start() it until it's fully set up, including hardware being ready to go.
drivers/spi/spi_pnx.c line 307
Quoted 4 lines
drivers/spi/spi_pnx.c new file +4
-0+if (rc <0 ) + goto out; + + data =kzalloc (sizeof *data,GFP_KERNEL );
You don't need that kzalloc, it was already done for you by virtue of your telling spi_alloc_master() to do so. But then you saved that memory into "pnx" not "data" (bug! wrong size!). What you should be doing is nesting your bitbang structure inside the "data" thing.
Quoted 1 line
+static int
spi_pnx_remove(struct device *device)
Likewise, mark this __exit. And I suspect you're using some kernel older than 2.6.15-rc6 ... ;)
I notice your header file was mostly inline function declarations, even for basic parts like clocking and DMA. That's best as part of the driver itself, if it's not following some framework like the <asm/hardware/clock.h> API on ARM.
- Dave
Quoted 10 lines · David Brownell
Quoted 3 lines · Vitaly Wool
(PNX0106 and PNX4008, namely) based on
spi_bitbanglibraryGood! Does need work yet though...
Yup...
Quoted 11 lines
Quoted 3 lines
# Add new SPI master controllers in alphabetical order above this line
Note how it says "above", yet you've added yours ... below! :(
Quoted 2 lines
#
Quoted 10 lines
drivers/spi/Kconfig config SPI_BUTTERFLY · line 100
Quoted 5 lines
drivers/spi/Kconfig +3
-0+config SPI_PNX + tristate "PNX SPI bus support" + depends on ARCH_PNX4008 && SPI_BITBANG... also added the eeprom so it shows in the list of controller drivers!!
Oh sorry. Stupid me... Not that it matters really since this driver's not yet mature enough anyway.
Quoted 12 lines
Quoted 9 lines
drivers/spi/Kconfig +4
-0+config SPI_EEPROM + tristate "SPI EEPROM" + depends on SPI +
Quoted 10 lines
drivers/spi/spi_pnx.c line 30
Quoted 4 lines
drivers/spi/spi_pnx.c new file +2
-0+#define lock_device ( dev )/* down( &dev->sem ); */ +#define unlock_device ( dev )/* up( &dev->sem ); */ Not clear why you'd want such stuff ... better to just remove NOPs like that.
They caused problems on 2.6.10... I might have uncommented that but I saw no reason. This driver is now is in FYI state :)
Quoted 17 lines
drivers/spi/spi_pnx.c line 124
Quoted 10 lines
drivers/spi/spi_pnx.c new file +5
-0+static void +spi_pnx_chipselect (struct spi_device *spi,int is_on) +{ +spipnx_arch_cs (spi, is_on); +}I see that "arch" hook is actually just toggling a DMA line. Did you notice how Stephen's PXA code handled that? The "
controller_data" was a board-specific GPIO toggle function, as provided in theboard_infothat implicitly defines the relevant device.
Quoted 7 lines
Plus, activating the chip does more than just toggling some arch-specific GPIO. It has to set the right SPI mode, of which CPOL must be set _before_ chipselect goes active. And it has to set the transfer clock, for chips like yours which are multiplexing devices using software.
Yep, the current way of doing stuff is a sort of abuse to the driver's model.
It's just "put #ifdefs out of the C dode" thing. We're gonna rewrite that using controller_data but in our case that's gonna be a structure with several functions and data needed. We have more arch-specific stuff to care about than Stephen's PXA code, so we just didn't bother at the moment; but anyway you're right.
Quoted 10 lines
Quoted 7 lines
drivers/spi/spi_pnx.c new file +2
-0+static int spi_pnx_xfer (struct spi_device *spidev,struct spi_transfer *t) +{...
Quoted 8 lines
drivers/spi/spi_pnx.c line 155
Quoted 4 lines
drivers/spi/spi_pnx.c new file +2
-0+if (spidev->max_speed_hz) +spi_pnx_set_clock_rate (spidev->max_speed_hz /1000 , regs);Just do it the one time, when activating that chip's selection.
Nm really in our case, we have to reset the chip at each direction change due to hardware bug.
Quoted 14 lines
...
Quoted 10 lines
drivers/spi/spi_pnx.c new file +10
-0+if (t->tx_buf) { + dat = (u8 *)t->tx_buf; + regs->con |=SPIPNX_CON_RXTX ; + regs->ier |=SPIPNX_IER_EOT ; + regs->con &= ~SPIPNX_CON_SHIFT_OFF ; + regs->frm = len; + +if (dd->dma_mode && len >=FIFO_CHUNK_SIZE ) { +void *dmasafe =NULL ; +if (t->tx_dma)
Quoted 10 lines
Quoted 2 lines
That's not actually correct, since zero might be a legal DMA address. Unfortunately we have no
DMA_ADDR_INVALIDto test against, so this will need to suffice for now. (And I updated thespi_bitbangcode to ensure that address _is_ zeroed when the message didn't set up the DMA addresses already, matching the default behavior.)
Hmm, DMA_ADDR_INVALID... (u_long)-1 ?
Quoted 20 lines
Quoted 5 lines
drivers/spi/spi_pnx.c line 179
Quoted 9 lines
drivers/spi/spi_pnx.c new file +9
-0+ params.dma_buffer = t->tx_dma; + else { + dmasafe =kmalloc (len,SLAB_KERNEL ); +if (!dmasafe) { + len =0 ; + goto out; + } + params.dma_buffer =dma_map_single (dev->parent, dmasafe, len,DMA_TO_DEVICE ); +memcpy (dmasafe, dat, len);You do know this is incorrect don't you? Call
dma_map_single()if you need a dma address. The caller already guaranteed that the input address is dma-safe.
Well it didn't :) See eeprom.c.
It's not the way to go however, but it's nice to have ability to add DMA safety in some controller drivers working with weird device drivers :/
Quoted 11 lines
...
Quoted 5 lines
drivers/spi/spi_pnx.c new file +3
-0+ } + +if (t->rx_buf) {...
Quoted 12 lines
Review
Quoted 3 lines
(quoted diff) +1
-0+ }That RX bit sure looks wrong to me ... as if, given an
spi_transferwith both TX and RX buffers, it will execute them in sequence (half duplex) rather than concurrently (full duplex). SPI is full duplex, and this code should be too. If it can't implement that, then it should be reporting an error if both rx and tx are requested, rather than doing the wrong thing.
This particular controller is full duplex but it has some hw problems working in FD mode. This is a workaround for FD which looks ok to me.
Quoted 16 lines
drivers/spi/spi_pnx.c line 307
Quoted 8 lines
drivers/spi/spi_pnx.c new file +4
-0+if (rc <0 ) + goto out; + + data =kzalloc (sizeof *data,GFP_KERNEL );You don't need that kzalloc, it was already done for you by virtue of your telling
spi_alloc_master()to do so. But then you saved that memory into "pnx" not "data" (bug! wrong size!). What you should be doing is nesting your bitbang structure inside the "data" thing.
Agreed.
Quoted 4 lines
Likewise, mark this
__exit. And I suspect you're using some kernel older than 2.6.15-rc6 ... ;)
Definitely, it was quickly ported from 2.6.10.
Quoted 6 lines
I notice your header file was mostly inline function declarations, even for basic parts like clocking and DMA. That's best as part of the driver itself, if it's not following some framework like the <
asm/hardware/clock.h> API on ARM.
Well, these all platform-specific functions are gonna be in controller_data some day... hopefully soon.
Vitaly