spi: add spi controller master driver for Blackfin 6xx processor
PATCH 1/1 spi: add spi controller master driver for Blackfin 6xx processor
New spi controller is integrated into Blackfin 6xx processor. Comparing to bf5xx spi controller, we support 32 bits word size and independent receive and transmit DMA channels now. Also mode 0 and 2 (CPHA = 0) can get fully supported becasue cs line may be controlled by the software.
--- drivers/spi/Kconfig | 8 +- drivers/spi/Makefile | 1 + drivers/spi/spi-bfin6xx.c | 1026 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 1034 insertions(+), 1 deletions(-) create mode 100644 drivers/spi/spi-bfin6xx.c
drivers/spi/Kconfig +7 -1
@@ -87,10 +87,16 @@ config SPI_BCM2835 config SPI_BFIN5XX tristate "SPI controller driver for ADI Blackfin5xx" - depends on BLACKFIN + depends on BLACKFIN && !BF60x help This is the SPI controller master driver for Blackfin 5xx processor. +config SPI_BFIN6XX + tristate "SPI controller driver for ADI Blackfin6xx" + depends on BF60x + help + This is the SPI controller master driver for Blackfin 6xx processor. + config SPI_BFIN_SPORT tristate "SPI bus via Blackfin SPORT" depends on BLACKFIN
drivers/spi/Makefile +1 -0
@@ -17,6 +17,7 @@ obj-$(CONFIG_SPI_AU1550) += spi-au1550.o obj-$(CONFIG_SPI_BCM2835) += spi-bcm2835.o obj-$(CONFIG_SPI_BCM63XX) += spi-bcm63xx.o obj-$(CONFIG_SPI_BFIN5XX) += spi-bfin5xx.o +obj-$(CONFIG_SPI_BFIN6XX) += spi-bfin6xx.o obj-$(CONFIG_SPI_BFIN_SPORT) += spi-bfin-sport.o obj-$(CONFIG_SPI_BITBANG) += spi-bitbang.o obj-$(CONFIG_SPI_BUTTERFLY) += spi-butterfly.o
drivers/spi/spi-bfin6xx.c new file +1026 -0
1,026 lines not on this page · raw message
Signature · 1 line
1.7.0.4
A message that is not in the archive (sent off-list or not yet received): 1370295814-28723-1-git-send-email-scott.jiang.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
drivers/spi/spi-bfin6xx.c
Quoted 3 lines · Scott Jiang
drivers/spi/spi-bfin6xx.c +1
-0+#include <linux/workqueue.h>
Why do you need this?
------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite It's a free troubleshooting tool designed for production Get down to code-level detail for bottlenecks, with <2% overhead. Download for free and get started troubleshooting in minutes.
http://p.sf.net/sfu/appdyn_d2d_ap2
A message that is not in the archive (sent off-list or not yet received): CAOh2x=mFXh9wW7E7JpKjEbZ_Dpi3v25mBG=p+STuQ=Pv=E6WoQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
2013/6/3 Viresh Kumar <[email protected]>:
Quoted 6 lines · Scott Jiang
drivers/spi/spi-bfin6xx.c
Quoted 3 lines
drivers/spi/spi-bfin6xx.c +1
-0+#include <linux/workqueue.h> Why do you need this?
Sorry, I forgot to remove this line when I converted to core queuing.
------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. A cloud service to automate IT design, transition and operations
2. Dashboards that offer high-level views of enterprise services
3. A single system of record for all IT processes
http://p.sf.net/sfu/servicenow-d2d-j
drivers/spi/spi-bfin6xx.c line 40
Quoted 4 lines · Scott Jiang
drivers/spi/spi-bfin6xx.c new file +4
-0+#define START_STATE ((void *)0) +#define RUNNING_STATE ((void *)1) +#define DONE_STATE ((void *)2) +#define ERROR_STATE ((void *)-1)
This looks icky due to the casts. Why not just store the state in the driver data for the device, it's not possible to have multiple messages active at once anyway?
drivers/spi/spi-bfin6xx.c line 127
Quoted 10 lines
drivers/spi/spi-bfin6xx.c new file +10
-0+static unsigned long sclk; +/* Caculate the SPI_CLOCK register value based on input HZ */ +static u32 hz_to_spi_clock (u32 speed_hz) +{ +u32 spi_clock = sclk / speed_hz; + +if (spi_clock) + spi_clock--; +return spi_clock; +}
Just from a style poin of view the global variable here isn't great, even if it's always global for the system currently it'd be better to have it in the data structure.
drivers/spi/spi-bfin6xx.c line 371
Quoted 2 lines
drivers/spi/spi-bfin6xx.c new file +2
-0+static void bfin_spi_pump_transfers (unsigned long data) +{
Looking at this I can't help but think there must be more common code we can factor out but that's a separate thing to adding a new driver.
drivers/spi/spi-bfin6xx.c line 446
Quoted 7 lines
drivers/spi/spi-bfin6xx.c new file +7
-0+ + drv_data->transfer_len = transfer->len; + drv_data->cs_change = transfer->cs_change; + +/* Bits per word setup */ + bits_per_word = transfer->bits_per_word ? : + message->spi->bits_per_word ? :8 ;
This isn't a triumph of legibility and really ought to be factored out into the core as the same will be true for all SPI transfers.
drivers/spi/spi-bfin6xx.c line 537
Quoted 4 lines
drivers/spi/spi-bfin6xx.c new file +3
-0+if (chip->enable_dma) { +return ; + }
The pump function is a pretty big function and this if in the middle of it makes the flow a bit odd. It would probably be better to split out DMA and PIO transfer functions and then make this just call whichever is appropriate.
drivers/spi/spi-bfin6xx.c line 606
Quoted 10 lines
drivers/spi/spi-bfin6xx.c new file +10
-0+ }else { +/* Update total byte transferred */ + message->actual_length += drv_data->transfer_len; +/* Move to next transfer of this msg */ + message->state =bfin_spi_next_transfer (drv_data); +if (drv_data->cs_change && message->state !=DONE_STATE ) { +bfin_spi_flush (drv_data); +bfin_spi_cs_deactive (drv_data, chip); + } + }
It seems like we'd want to deassert /CS even on error but we didn't do that...
drivers/spi/spi-bfin6xx.c line 658
Quoted 7 lines
drivers/spi/spi-bfin6xx.c new file +7
-0+if (spi->bits_per_word !=8 + && spi->bits_per_word !=16 + && spi->bits_per_word !=32 ) { +dev_err (&spi->dev,"%d bits_per_word is not supported\n" , + spi->bits_per_word); +return -EINVAL ; + }
Write this as a switch statement.
drivers/spi/spi-bfin6xx.c line 672
Quoted 4 lines
drivers/spi/spi-bfin6xx.c new file +4
-0+if (chip_info->control & ~bfin_ctl_reg) { +dev_err (&spi->dev,"do not set bits " +"that the SPI framework manages\n" ); + goto error;
Don't split error messages over lines.
Quoted 8 lines
drivers/spi/spi-bfin6xx.c new file +8
-0+if (chip->cs <MAX_CTRL_CS ) { + chip->ssel = (1 << chip->cs) <<8 ; + ret =peripheral_request (ssel[spi->master->bus_num] + [chip->cs-1 ],dev_name (&spi->dev)); +if (ret) { +dev_err (&spi->dev,"peripheral_request() error\n" ); + goto error; + }
drivers/spi/spi-bfin6xx.c line 697
Quoted 10 lines
drivers/spi/spi-bfin6xx.c new file +10
-0+ }else { + chip->cs_gpio = chip->cs -MAX_CTRL_CS ; + ret =gpio_request_one (chip->cs_gpio,GPIOF_OUT_INIT_HIGH , +dev_name (&spi->dev)); +if (ret) { +dev_err (&spi->dev,"gpio_request_one() error\n" ); + goto error; + } + } +spi_set_ctldata (spi, chip);
Why is all this resource allocation not being done in the probe() function?
Quoted 4 lines
drivers/spi/spi-bfin6xx.c new file +4
-0+static irqreturn_t bfin_spi_tx_dma_isr (int irq,void *dev_id) +{ +struct bfin_spi_master *drv_data = dev_id; +u32 dma_stat =get_dma_curr_irqstat (drv_data->tx_dma);
drivers/spi/spi-bfin6xx.c line 770
Quoted 10 lines
drivers/spi/spi-bfin6xx.c new file +10
-0+ +clear_dma_irqstat (drv_data->tx_dma); +if (dma_stat &DMA_DONE ) + drv_data->tx_num++; +if (dma_stat &DMA_ERR ) +dev_err (&drv_data->master->dev, +"spi tx dma error: %d\n" , dma_stat); +bfin_write_and (&drv_data->regs->tx_control, ~SPI_TXCTL_TDR_NF ); +return IRQ_HANDLED ; +}
This doesn't check that it got a sane state from the hardware - what happens if neither DMA_DONE nor DMA_ERR is true? I'd also expect us to fail the transfer if we get an error state.
drivers/spi/spi-bfin6xx.c line 909
Quoted 2 lines
drivers/spi/spi-bfin6xx.c new file +2
-0+ +dev_info (dev,"bfin-spi probe success\n" );
Remove this, it's just noisy.
Quoted 5 lines
drivers/spi/spi-bfin6xx.c new file +5
-0+#ifdef CONFIG_PM +static int bfin_spi_suspend (struct device *dev) +{ +struct spi_master *master =dev_get_drvdata (dev); +struct bfin_spi_master *drv_data =spi_master_get_devdata (master);
drivers/spi/spi-bfin6xx.c line 955
Quoted 10 lines
drivers/spi/spi-bfin6xx.c new file +10
-0+ +spi_master_suspend (master); + + drv_data->control =bfin_read (&drv_data->regs->control); + drv_data->ssel =bfin_read (&drv_data->regs->ssel); + +bfin_write (&drv_data->regs->control,SPI_CTL_MSTR |SPI_CTL_CPHA ); +bfin_write (&drv_data->regs->ssel,0x0000FE00 ); +free_dma (drv_data->rx_dma); +free_dma (drv_data->tx_dma);
Why free and reallocate the DMA over suspend?
_______________________________________________
Uclinux-dist-devel mailing list Uclinux-dist-devel-ZG0+EudsQA8dtHy/[email protected]
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi Mark,
2013/6/3 Mark Brown <[email protected]>:
Quoted 11 lines · Scott Jiang
drivers/spi/spi-bfin6xx.c line 40
Quoted 5 lines
drivers/spi/spi-bfin6xx.c new file +4
-0+#define START_STATE ((void *)0) +#define RUNNING_STATE ((void *)1) +#define DONE_STATE ((void *)2) +#define ERROR_STATE ((void *)-1)This looks icky due to the casts. Why not just store the state in the driver data for the device, it's not possible to have multiple messages active at once anyway?
Yes, I can move it to driver data. But struct spi_message has state field for this message, I just want to make use of it.
Quoted 8 lines
drivers/spi/spi-bfin6xx.c line 672
Quoted 5 lines
drivers/spi/spi-bfin6xx.c new file +4
-0+if (chip_info->control & ~bfin_ctl_reg) { +dev_err (&spi->dev,"do not set bits " +"that the SPI framework manages\n" ); + goto error;Don't split error messages over lines.
It outputs in one line.
Quoted 19 lines
Quoted 8 lines
drivers/spi/spi-bfin6xx.c new file +8
-0+if (chip->cs <MAX_CTRL_CS ) { + chip->ssel = (1 << chip->cs) <<8 ; + ret =peripheral_request (ssel[spi->master->bus_num] + [chip->cs-1 ],dev_name (&spi->dev)); +if (ret) { +dev_err (&spi->dev,"peripheral_request() error\n" ); + goto error; + }drivers/spi/spi-bfin6xx.c line 697
Quoted 10 lines
drivers/spi/spi-bfin6xx.c new file +10
-0+ }else { + chip->cs_gpio = chip->cs -MAX_CTRL_CS ; + ret =gpio_request_one (chip->cs_gpio,GPIOF_OUT_INIT_HIGH , +dev_name (&spi->dev)); +if (ret) { +dev_err (&spi->dev,"gpio_request_one() error\n" ); + goto error; + } + } +spi_set_ctldata (spi, chip);
Quoted 3 lines
Why is all this resource allocation not being done in the
probe()function?
You can't know the cs line that the spi device uses in probe()
Quoted 10 lines
Quoted 10 lines
drivers/spi/spi-bfin6xx.c new file +10
-0+static irqreturn_t bfin_spi_tx_dma_isr (int irq,void *dev_id) +{ +struct bfin_spi_master *drv_data = dev_id; +u32 dma_stat =get_dma_curr_irqstat (drv_data->tx_dma); + +clear_dma_irqstat (drv_data->tx_dma); +if (dma_stat &DMA_DONE ) + drv_data->tx_num++; +if (dma_stat &DMA_ERR ) + dev_err(&drv_data->master->dev,
Quoted 9 lines
drivers/spi/spi-bfin6xx.c line 770
Quoted 4 lines
drivers/spi/spi-bfin6xx.c new file +4
-0+ "spi tx dma error: %d\n", dma_stat); +bfin_write_and (&drv_data->regs->tx_control, ~SPI_TXCTL_TDR_NF ); +return IRQ_HANDLED ; +}This doesn't check that it got a sane state from the hardware - what happens if neither
DMA_DONEnorDMA_ERRis true? I'd also expect us to fail the transfer if we get an error state.
DMA_DONE means correct, while DMA_ERR means error. Others only tell you current status, so when DMA_ERR I output the whole register value to check what error is.
Quoted 8 lines
Quoted 8 lines
drivers/spi/spi-bfin6xx.c new file +8
-0+static int bfin_spi_suspend (struct device *dev) +{ +struct spi_master *master =dev_get_drvdata (dev); +struct bfin_spi_master *drv_data =spi_master_get_devdata (master); + +spi_master_suspend (master); + + drv_data->control =bfin_read (&drv_data->regs->control);
Quoted 8 lines
drivers/spi/spi-bfin6xx.c line 955
Quoted 6 lines
drivers/spi/spi-bfin6xx.c new file +6
-0+ drv_data->ssel =bfin_read (&drv_data->regs->ssel); + +bfin_write (&drv_data->regs->control,SPI_CTL_MSTR |SPI_CTL_CPHA ); +bfin_write (&drv_data->regs->ssel,0x0000FE00 ); +free_dma (drv_data->rx_dma); +free_dma (drv_data->tx_dma);Why free and reallocate the DMA over suspend?
It's a boot rom bug. If it starts from spi flash, it will polute spi and dma status.
------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. A cloud service to automate IT design, transition and operations
2. Dashboards that offer high-level views of enterprise services
3. A single system of record for all IT processes
http://p.sf.net/sfu/servicenow-d2d-j
A message that is not in the archive (sent off-list or not yet received): CAHG8p1D8mXq_=3DOBsRMCyRQ9UpWOLbU4y0VYO7PzERxknO7ow-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 10 lines · Scott Jiang
2013/6/3 Mark Brown <[email protected]>:
Quoted 6 lines
drivers/spi/spi-bfin6xx.c line 672
Quoted 4 lines
drivers/spi/spi-bfin6xx.c new file +4
-0+if (chip_info->control & ~bfin_ctl_reg) { +dev_err (&spi->dev,"do not set bits " +"that the SPI framework manages\n" ); + goto error;Don't split error messages over lines.
It outputs in one line.
Yes but when I grep for "do not set bits that the SPI framework manages" after I see it in my kernel output I won't be able to find it because you split it over multiple lines in the source file.
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/spi/spi-bfin6xx.c new file +10
-0+static irqreturn_t bfin_spi_tx_dma_isr (int irq,void *dev_id) +{ +struct bfin_spi_master *drv_data = dev_id; +u32 dma_stat =get_dma_curr_irqstat (drv_data->tx_dma); + +clear_dma_irqstat (drv_data->tx_dma); +if (dma_stat &DMA_DONE ) + drv_data->tx_num++; +if (dma_stat &DMA_ERR ) + dev_err(&drv_data->master->dev,Quoted 8 lines
drivers/spi/spi-bfin6xx.c line 770
Quoted 4 lines
drivers/spi/spi-bfin6xx.c new file +4
-0+ "spi tx dma error: %d\n", dma_stat); +bfin_write_and (&drv_data->regs->tx_control, ~SPI_TXCTL_TDR_NF ); +return IRQ_HANDLED ; +}This doesn't check that it got a sane state from the hardware - what happens if neither
DMA_DONEnorDMA_ERRis true? I'd also expect us to fail the transfer if we get an error state.
Quoted 3 lines
DMA_DONEmeans correct, whileDMA_ERRmeans error. Others only tell you current status, so whenDMA_ERRI output the whole register value to check what error is.
That doesn't address my concern - what happens if the hardware messed up and didn't set either bit?
Quoted 4 lines
Quoted 1 line
Why free and reallocate the DMA over suspend?
It's a boot rom bug. If it starts from spi flash, it will polute spi and dma status.
OK, document this then - otherwise someone will just come along and delete the code since it's not obviously sensible.
_______________________________________________
Uclinux-dist-devel mailing list Uclinux-dist-devel-ZG0+EudsQA8dtHy/[email protected]
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel