[1/6] DaVinci: SPI Driver for DaVinci and DA8xx SOC's

Commit Message

David Brownell July 16, 2009, 10:28 p.m. UTC

On Wednesday 15 July 2009, [e-mail] wrote:
>  arch/arm/mach-davinci/include/mach/spi.h |   45 ++
>  drivers/spi/Kconfig                      |    7 +
>  drivers/spi/Makefile                     |    1 +
>  drivers/spi/davinci_spi.c                |  751 ++++++++++++++++++++++++++++++
>  drivers/spi/davinci_spi.h                |  163 +++++++
>  5 files changed, 967 insertions(+), 0 deletions(-)

It's getting a lot closer.  See the appended ... the issues I
see remaining are partly performance (if you only set the FMT
register in the setup code the per-message overhead will be
a lot less) and partly functional (those per-device settings
that are treated as global, or wrongly held back on v1 parts).

Let me know if you plan to address either of those.

- Dave

Comments

[e-mail] July 16, 2009, 11:24 p.m. UTC | #1

> -----Original Message-----
> From: David Brownell [mailto:[e-mail]]
> Sent: Thursday, July 16, 2009 6:29 PM
> To: Paulraj, Sandeep
> Cc: [e-mail]
> Subject: Re: [PATCH 1/6] DaVinci: SPI Driver for DaVinci and DA8xx SOC's
> 
> On Wednesday 15 July 2009, [e-mail] wrote:
> >  arch/arm/mach-davinci/include/mach/spi.h |   45 ++
> >  drivers/spi/Kconfig                      |    7 +
> >  drivers/spi/Makefile                     |    1 +
> >  drivers/spi/davinci_spi.c                |  751
> ++++++++++++++++++++++++++++++
> >  drivers/spi/davinci_spi.h                |  163 +++++++
> >  5 files changed, 967 insertions(+), 0 deletions(-)
> 
> It's getting a lot closer.  See the appended ... the issues I
> see remaining are partly performance (if you only set the FMT
> register in the setup code the per-message overhead will be
> a lot less) and partly functional (those per-device settings
> that are treated as global, or wrongly held back on v1 parts).
> 
> Let me know if you plan to address either of those.
[Sandeep] why not, I'll make the changes and get back hopefully sometime tomorrow
> 
> - Dave
> 
> 
> ==================== CUT HERE
> Minor fixes:
>   * get rid of global "sdev" ... driver should work with
>     more than one controller
>   * rename set_bits() as set_io_bits(), minimizing confusion
>     with standard set_bit() call; likewise clear_bits()
>   * Version 1 chips can also handle SPI_NO_CS
>   * Remove duplicate or otherwise unneeded #includes
> 
> Less-minor ones:
>   * Remove confusion between buses (MOSI/MISO/SCK; one per SPI
>     controller) and the chip-selects used to share them.
>   * Fix test for SPI_CPHA ...
> 
> Didn't touch:
>   * All the stuff in davinci_spi_bufs_prep() which really belongs
>     in davinci_spi_setup() ... it's just a slowdown, since the FMT
>     register could be written just once.
[Sandeep] I'll make change and test it across DM355, DM365 and DM6467.

>   * The WDELAY and parity stuff in davinci_spi_bufs_prep() which
>     doesn't kick in for version 1 controllers (even though they
>     support it) and which is handled as *global* options instead
>     of per-device ones (controls are in per-device FMT registers):
>        (a) set it with other updates to FMT register
>        (b) make those per-device policies
[Sandeep] I'll tell you what the real issue is. IN reality SPI module on DM355, DM6467, DM355 and DA8xx are all different.

IF you a take a close look at the Dm355 SPI module guide
http://focus.ti.com/lit/ug/sprued4b/sprued4b.pdf

you will notice that it does not have the WDELAY and parity stuff.

The DM365 IP itself is similar to DM355 so it was decided to use the version 1 string for DM365 as well.

There are differences also in the diff pin modes(3,4, 5 pins) between all the different IPs.


> 
> ---
>  drivers/spi/davinci_spi.c |  115 ++++++++++++++++++++++------------------
> ----
>  1 file changed, 59 insertions(+), 56 deletions(-)
> 
> --- a/drivers/spi/davinci_spi.c
> +++ b/drivers/spi/davinci_spi.c
> @@ -16,20 +16,14 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> USA
>   */
> 
> -#include <linux/types.h>
> -#include <linux/io.h>
> -#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/gpio.h>
> -#include <linux/interrupt.h>
>  #include <linux/module.h>
> -#include <linux/device.h>
>  #include <linux/delay.h>
>  #include <linux/platform_device.h>
> -#include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/clk.h>
> -#include <linux/gpio.h>
> 
>  #include <linux/spi/spi.h>
>  #include <linux/spi/spi_bitbang.h>
> @@ -38,7 +32,6 @@
> 
>  #include "davinci_spi.h"
> 
> -struct device *sdev;
> 
>  static void davinci_spi_rx_buf_u8(u32 data, struct davinci_spi
> *davinci_spi)
>  {
> @@ -76,7 +69,7 @@ static u32 davinci_spi_tx_buf_u16(struct
>  	return data;
>  }
> 
> -static inline void set_bits(void __iomem *addr, u32 bits)
> +static inline void set_io_bits(void __iomem *addr, u32 bits)
>  {
>  	u32 v = ioread32(addr);
> 
> @@ -84,7 +77,7 @@ static inline void set_bits(void __iomem
>  	iowrite32(v, addr);
>  }
> 
> -static inline void clear_bits(void __iomem *addr, u32 bits)
> +static inline void clear_io_bits(void __iomem *addr, u32 bits)
>  {
>  	u32 v = ioread32(addr);
> 
> @@ -92,14 +85,14 @@ static inline void clear_bits(void __iom
>  	iowrite32(v, addr);
>  }
> 
> -static inline void set_fmt_bits(void __iomem *addr, u32 bits, int
> bus_num)
> +static inline void set_fmt_bits(void __iomem *addr, u32 bits, int cs_num)
>  {
> -	set_bits(addr + SPIFMT0 + (0x4 * bus_num), bits);
> +	set_io_bits(addr + SPIFMT0 + (0x4 * cs_num), bits);
>  }
> 
> -static inline void clear_fmt_bits(void __iomem *addr, u32 bits, int
> bus_num)
> +static inline void clear_fmt_bits(void __iomem *addr, u32 bits, int
> cs_num)
>  {
> -	clear_bits(addr + SPIFMT0 + (0x4 * bus_num), bits);
> +	clear_io_bits(addr + SPIFMT0 + (0x4 * cs_num), bits);
>  }
> 
>  /*
> @@ -119,7 +112,7 @@ static void davinci_spi_chipselect(struc
>  	 * line for the controller
>  	 */
>  	if (value == BITBANG_CS_INACTIVE) {
> -		set_bits(davinci_spi->base + SPIDEF, CS_DEFAULT);
> +		set_io_bits(davinci_spi->base + SPIDEF, CS_DEFAULT);
> 
>  		data1_reg_val |= CS_DEFAULT << SPIDAT1_CSNR_SHIFT;
>  		iowrite32(data1_reg_val, davinci_spi->base + SPIDAT1);
> @@ -179,14 +172,14 @@ static int davinci_spi_setup_transfer(st
>  		hz = spi->max_speed_hz;
> 
>  	clear_fmt_bits(davinci_spi->base, SPIFMT_CHARLEN_MASK,
> -			spi->master->bus_num);
> +			spi->chip_select);
>  	set_fmt_bits(davinci_spi->base, bits_per_word & 0x1f,
> -			spi->master->bus_num);
> +			spi->chip_select);
> 
>  	prescale = ((clk_get_rate(davinci_spi->clk) / hz) - 1) & 0xff;
> 
> -	clear_fmt_bits(davinci_spi->base, 0x0000ff00, spi->master->bus_num);
> -	set_fmt_bits(davinci_spi->base, prescale << 8, spi->master-
> >bus_num);
> +	clear_fmt_bits(davinci_spi->base, 0x0000ff00, spi->chip_select);
> +	set_fmt_bits(davinci_spi->base, prescale << 8, spi->chip_select);
> 
>  	return 0;
>  }
> @@ -226,84 +219,90 @@ static int davinci_spi_bufs_prep(struct
>  {
>  	int op_mode = 0;
> 
> -	/* configuraton parameter for SPI */
> +	/*
> +	 * Set up device-specific SPI configuration parameters.
> +	 * Most of these would normally be handled in spi_setup(),
> +	 * updating the per-chipselect FMT registers, but some of
> +	 * them use global controls like SPI_LOOP and SPI_READY.
> +	 */
> +
>  	if (spi->mode & SPI_LSB_FIRST)
>  		set_fmt_bits(davinci_spi->base, SPIFMT_SHIFTDIR_MASK,
> -				spi->master->bus_num);
> +				spi->chip_select);
>  	else
>  		clear_fmt_bits(davinci_spi->base, SPIFMT_SHIFTDIR_MASK,
> -				spi->master->bus_num);
> +				spi->chip_select);
> 
>  	if (spi->mode & SPI_CPOL)
>  		set_fmt_bits(davinci_spi->base, SPIFMT_POLARITY_MASK,
> -				spi->master->bus_num);
> +				spi->chip_select);
>  	else
>  		clear_fmt_bits(davinci_spi->base, SPIFMT_POLARITY_MASK,
> -				spi->master->bus_num);
> +				spi->chip_select);
> 
> -	if (!(spi->mode & SPI_CPOL))
> +	if (!(spi->mode & SPI_CPHA))
>  		set_fmt_bits(davinci_spi->base, SPIFMT_PHASE_MASK,
> -				spi->master->bus_num);
> +				spi->chip_select);
>  	else
>  		clear_fmt_bits(davinci_spi->base, SPIFMT_PHASE_MASK,
> -				spi->master->bus_num);
> +				spi->chip_select);
> 
>  	if (davinci_spi->version == SPI_VERSION_2) {
>  		clear_fmt_bits(davinci_spi->base, SPIFMT_WDELAY_MASK,
> -				spi->master->bus_num);
> +				spi->chip_select);
>  		set_fmt_bits(davinci_spi->base,
> -			    ((davinci_spi->pdata->wdelay <<
> -				SPIFMT_WDELAY_SHIFT) &
> -				SPIFMT_WDELAY_MASK),
> -				spi->master->bus_num);
> +				(davinci_spi->pdata->wdelay
> +						<< SPIFMT_WDELAY_SHIFT)
> +					& SPIFMT_WDELAY_MASK,
> +				spi->chip_select);
> 
>  		if (davinci_spi->pdata->odd_parity)
>  			set_fmt_bits(davinci_spi->base,
>  					SPIFMT_ODD_PARITY_MASK,
> -					spi->master->bus_num);
> +					spi->chip_select);
>  		else
>  			clear_fmt_bits(davinci_spi->base,
>  					SPIFMT_ODD_PARITY_MASK,
> -					spi->master->bus_num);
> +					spi->chip_select);
> 
>  		if (davinci_spi->pdata->parity_enable)
>  			set_fmt_bits(davinci_spi->base,
>  					SPIFMT_PARITYENA_MASK,
> -					spi->master->bus_num);
> +					spi->chip_select);
>  		else
>  			clear_fmt_bits(davinci_spi->base,
>  					SPIFMT_PARITYENA_MASK,
> -					spi->master->bus_num);
> +					spi->chip_select);
> 
>  		if (davinci_spi->pdata->wait_enable)
>  			set_fmt_bits(davinci_spi->base,
>  					SPIFMT_WAITENA_MASK,
> -					spi->master->bus_num);
> +					spi->chip_select);
>  		else
>  			clear_fmt_bits(davinci_spi->base,
>  					SPIFMT_WAITENA_MASK,
> -					spi->master->bus_num);
> +					spi->chip_select);
> 
>  		if (davinci_spi->pdata->timer_disable)
>  			set_fmt_bits(davinci_spi->base,
>  					SPIFMT_DISTIMER_MASK,
> -					spi->master->bus_num);
> +					spi->chip_select);
>  		else
>  			clear_fmt_bits(davinci_spi->base,
>  					SPIFMT_DISTIMER_MASK,
> -					spi->master->bus_num);
> +					spi->chip_select);
>  	}
> 
>  	/* Clock internal */
>  	if (davinci_spi->pdata->clk_internal)
> -		set_bits(davinci_spi->base + SPIGCR1,
> +		set_io_bits(davinci_spi->base + SPIGCR1,
>  				SPIGCR1_CLKMOD_MASK);
>  	else
> -		clear_bits(davinci_spi->base + SPIGCR1,
> +		clear_io_bits(davinci_spi->base + SPIGCR1,
>  				SPIGCR1_CLKMOD_MASK);
> 
>  	/* master mode default */
> -	set_bits(davinci_spi->base + SPIGCR1, SPIGCR1_MASTER_MASK);
> +	set_io_bits(davinci_spi->base + SPIGCR1, SPIGCR1_MASTER_MASK);
> 
>  	if (davinci_spi->pdata->intr_level)
>  		iowrite32(SPI_INTLVL_1, davinci_spi->base + SPILVL);
> @@ -311,12 +310,16 @@ static int davinci_spi_bufs_prep(struct
>  		iowrite32(SPI_INTLVL_0, davinci_spi->base + SPILVL);
> 
>  	/*
> -	 * Standard SPI mode uses 4 pins, with chipselect
> -	 * 3 pin SPI is a 4 pin variant without CS (SPI_NO_CS)
> +	 * Version 1 hardware supports two basic SPI modes:
> +	 *  - Standard SPI mode uses 4 pins, with chipselect
> +	 *  - 3 pin SPI is a 4 pin variant without CS (SPI_NO_CS)
>  	 *	(distinct from SPI_3WIRE, with just one data wire;
>  	 *	or similar variants without MOSI or without MISO)
> -	 * 4 pin with enable is (SPI_READY | SPI_NO_CS)
> -	 * 5 pin SPI variant is standard SPI plus SPI_READY
> +	 *
> +	 * Version 2 hardware supports an optional handshaking signal,
> +	 * so it can support two more modes:
> +	 *  - 5 pin SPI variant is standard SPI plus SPI_READY
> +	 *  - 4 pin with enable is (SPI_READY | SPI_NO_CS)
>  	 */
> 
>  	op_mode = SPIPC0_DIFUN_MASK
> @@ -330,10 +333,10 @@ static int davinci_spi_bufs_prep(struct
>  	iowrite32(op_mode, davinci_spi->base + SPIPC0);
> 
>  	if (spi->mode & SPI_LOOP)
> -		set_bits(davinci_spi->base + SPIGCR1,
> +		set_io_bits(davinci_spi->base + SPIGCR1,
>  				SPIGCR1_LOOPBACK_MASK);
>  	else
> -		clear_bits(davinci_spi->base + SPIGCR1,
> +		clear_io_bits(davinci_spi->base + SPIGCR1,
>  				SPIGCR1_LOOPBACK_MASK);
> 
>  	return 0;
> @@ -342,6 +345,7 @@ static int davinci_spi_bufs_prep(struct
>  static int davinci_spi_check_error(struct davinci_spi *davinci_spi,
>  				   int int_status)
>  {
> +	struct device *sdev = davinci_spi->bitbang.master->dev.parent;
> 
>  	if (int_status & SPIFLG_TIMEOUT_MASK) {
>  		dev_dbg(sdev, "SPI Time-out Error\n");
> @@ -417,7 +421,7 @@ static int davinci_spi_bufs_pio(struct s
>  		return ret;
> 
>  	/* Enable SPI */
> -	set_bits(davinci_spi->base + SPIGCR1, SPIGCR1_SPIENA_MASK);
> +	set_io_bits(davinci_spi->base + SPIGCR1, SPIGCR1_SPIENA_MASK);
> 
>  	iowrite32(0 | (pdata->c2tdelay << SPI_C2TDELAY_SHIFT) |
>  			(pdata->t2cdelay << SPI_T2CDELAY_SHIFT),
> @@ -427,7 +431,7 @@ static int davinci_spi_bufs_pio(struct s
>  	data1_reg_val = pdata->cs_hold << SPIDAT1_CSHOLD_SHIFT;
>  	tmp = ~(0x1 << spi->chip_select);
> 
> -	clear_bits(davinci_spi->base + SPIDEF, ~tmp);
> +	clear_io_bits(davinci_spi->base + SPIDEF, ~tmp);
> 
>  	data1_reg_val |= tmp << SPIDAT1_CSNR_SHIFT;
> 
> @@ -437,7 +441,7 @@ static int davinci_spi_bufs_pio(struct s
> 
>  	/* Determine the command to execute READ or WRITE */
>  	if (t->tx_buf) {
> -		clear_bits(davinci_spi->base + SPIINT, SPIINT_MASKALL);
> +		clear_io_bits(davinci_spi->base + SPIINT, SPIINT_MASKALL);
> 
>  		while (1) {
>  			tx_data = davinci_spi->get_tx(davinci_spi);
> @@ -490,7 +494,7 @@ static int davinci_spi_bufs_pio(struct s
>  			int i;
> 
>  			for (i = 0; i < davinci_spi->count; i++) {
> -				set_bits(davinci_spi->base + SPIINT,
> +				set_io_bits(davinci_spi->base + SPIINT,
>  						SPIINT_BITERR_INTR
>  						| SPIINT_OVRRUN_INTR
>  						| SPIINT_RX_INTR);
> @@ -592,7 +596,6 @@ static int davinci_spi_probe(struct plat
>  	}
> 
>  	dev_set_drvdata(&pdev->dev, master);
> -	sdev = &pdev->dev;
> 
>  	davinci_spi = spi_master_get_devdata(master);
>  	if (davinci_spi == NULL) {
> @@ -659,9 +662,9 @@ static int davinci_spi_probe(struct plat
>  	davinci_spi->bitbang.setup_transfer = davinci_spi_setup_transfer;
>  	davinci_spi->bitbang.txrx_bufs = davinci_spi_bufs_pio;
> 
> -	davinci_spi->bitbang.flags = SPI_LSB_FIRST | SPI_LOOP;
> +	davinci_spi->bitbang.flags = SPI_NO_CS | SPI_LSB_FIRST | SPI_LOOP;
>  	if (davinci_spi->version == SPI_VERSION_2)
> -		davinci_spi->bitbang.flags |= SPI_NO_CS | SPI_READY;
> +		davinci_spi->bitbang.flags |= SPI_READY;
> 
>  	davinci_spi->version = pdata->version;
>  	davinci_spi->get_rx = davinci_spi_rx_buf_u8;
> 
>

David Brownell July 16, 2009, 11:54 p.m. UTC | #2

On Thursday 16 July 2009, Paulraj, Sandeep wrote:
> 
> > Didn't touch:
> >   * All the stuff in davinci_spi_bufs_prep() which really belongs
> >     in davinci_spi_setup() ... it's just a slowdown, since the FMT
> >     register could be written just once.
>
> [Sandeep] I'll make change and test it across DM355, DM365 and DM6467.

OK.  That will matter more when DMA support gets added ... but this
is the sort of structural issue that's best fixed early (IMNSHO).

 
> >   * The WDELAY and parity stuff in davinci_spi_bufs_prep() which
> >     doesn't kick in for version 1 controllers (even though they
> >     support it) and which is handled as *global* options instead
> >     of per-device ones (controls are in per-device FMT registers):
> >        (a) set it with other updates to FMT register
> >        (b) make those per-device policies
>
> [Sandeep] I'll tell you what the real issue is. IN reality SPI module
> on DM355, DM6467, DM355 and DA8xx are all different. 

You can maybe tell I was looking at dm365 docs for that.  ;)

Errata are a different story.  DM355 seemed to have the worst
story there.


> IF you a take a close look at the Dm355 SPI module guide
> http://focus.ti.com/lit/ug/sprued4b/sprued4b.pdf
> 
> you will notice that it does not have the WDELAY and parity stuff.
> 
> The DM365 IP itself is similar to DM355 so it was decided to use
> the version 1 string for DM365 as well. 
> 
> There are differences also in the diff pin modes(3,4, 5 pins)
> between all the different IPs. 

I see, then.  Comments missing.  :)

This is the sort of thing which makes me want to put cpu_is_XXX()
logic into drivers ... it's a good way to handle "lots of little
differences".

- Dave

[e-mail] July 17, 2009, 12:39 a.m. UTC | #3

> -----Original Message-----
> From: David Brownell [mailto:[e-mail]]
> Sent: Thursday, July 16, 2009 7:55 PM
> To: Paulraj, Sandeep
> Cc: [e-mail]
> Subject: Re: [PATCH 1/6] DaVinci: SPI Driver for DaVinci and DA8xx SOC's
> 
> On Thursday 16 July 2009, Paulraj, Sandeep wrote:
> >
> > > Didn't touch:
> > >   * All the stuff in davinci_spi_bufs_prep() which really belongs
> > >     in davinci_spi_setup() ... it's just a slowdown, since the FMT
> > >     register could be written just once.
> >
> > [Sandeep] I'll make change and test it across DM355, DM365 and DM6467.
> 
> OK.  That will matter more when DMA support gets added ... but this
> is the sort of structural issue that's best fixed early (IMNSHO).

[Sandeep] I've made the change and tested on DM355 and it worked without any issues.

> 
> 
> > >   * The WDELAY and parity stuff in davinci_spi_bufs_prep() which
> > >     doesn't kick in for version 1 controllers (even though they
> > >     support it) and which is handled as *global* options instead
> > >     of per-device ones (controls are in per-device FMT registers):
> > >        (a) set it with other updates to FMT register
> > >        (b) make those per-device policies
> >
> > [Sandeep] I'll tell you what the real issue is. IN reality SPI module
> > on DM355, DM6467, DM355 and DA8xx are all different.
> 
> You can maybe tell I was looking at dm365 docs for that.  ;)
> 
> Errata are a different story.  DM355 seemed to have the worst
> story there.
> 
> 
> > IF you a take a close look at the Dm355 SPI module guide
> > http://focus.ti.com/lit/ug/sprued4b/sprued4b.pdf
> >
> > you will notice that it does not have the WDELAY and parity stuff.
> >
> > The DM365 IP itself is similar to DM355 so it was decided to use
> > the version 1 string for DM365 as well.
> >
> > There are differences also in the diff pin modes(3,4, 5 pins)
> > between all the different IPs.
> 
> I see, then.  Comments missing.  :)
> 
> This is the sort of thing which makes me want to put cpu_is_XXX()
> logic into drivers ... it's a good way to handle "lots of little
> differences".
[Sandeep] What would be your suggestion? Should I leave it for the time being? 

> 
> - Dave
>

David Brownell July 17, 2009, 1:55 a.m. UTC | #4

On Thursday 16 July 2009, Paulraj, Sandeep wrote:
> > >
> > > There are differences also in the diff pin modes(3,4, 5 pins)
> > > between all the different IPs.
> > 
> > I see, then.  Comments missing.  :)
> > 
> > This is the sort of thing which makes me want to put cpu_is_XXX()
> > logic into drivers ... it's a good way to handle "lots of little
> > differences".
>
> [Sandeep] What would be your suggestion? Should I leave it for
> the time being?  

Yes.

Steve Chen July 17, 2009, 10:38 a.m. UTC | #5

On Thu, 2009-07-16 at 18:55 -0700, David Brownell wrote:
> On Thursday 16 July 2009, Paulraj, Sandeep wrote:
> > > >
> > > > There are differences also in the diff pin modes(3,4, 5 pins)
> > > > between all the different IPs.
> > > 
> > > I see, then.  Comments missing.  :)
> > > 
> > > This is the sort of thing which makes me want to put cpu_is_XXX()
> > > logic into drivers ... it's a good way to handle "lots of little
> > > differences".
> >
> > [Sandeep] What would be your suggestion? Should I leave it for
> > the time being?  
> 
> Yes. 

There used to be cpu_is_xxx logic in this and other DaVinci drivers.
The code became very messy as new SoCs were added.  It took a great
amount of effort to clean up and remove all instances of cpu_is_xxx from
drivers.  Lets stay away from reintroducing the cpu_is_xxx macros back
into the driver if all possible.

Regards,

Steve

> 
> 
> 
> _______________________________________________
> Davinci-linux-open-source mailing list
> [e-mail]
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

David Brownell July 17, 2009, 1:58 p.m. UTC | #6

On Friday 17 July 2009, Steve Chen wrote:
> On Thu, 2009-07-16 at 18:55 -0700, David Brownell wrote:
> > On Thursday 16 July 2009, Paulraj, Sandeep wrote:
> > > > >
> > > > > There are differences also in the diff pin modes(3,4, 5 pins)
> > > > > between all the different IPs.
> > > > 
> > > > I see, then.  Comments missing.  :)
> > > > 
> > > > This is the sort of thing which makes me want to put cpu_is_XXX()
> > > > logic into drivers ... it's a good way to handle "lots of little
> > > > differences".
> > >
> > > [Sandeep] What would be your suggestion? Should I leave it for
> > > the time being?  
> > 
> > Yes. 
> 
> There used to be cpu_is_xxx logic in this and other DaVinci drivers.

"This" is a new driver.  :)


> The code became very messy as new SoCs were added.  It took a great
> amount of effort to clean up and remove all instances of cpu_is_xxx from
> drivers.  Lets stay away from reintroducing the cpu_is_xxx macros back
> into the driver if all possible.

I know there's some dislike of those macros, which is why I was
pointing out that this is the kind of problem that's fairly
awkward to solve *without* using them.  And why I'm not pushing
to switch to them, at least for now.

- Dave

Steve Chen July 17, 2009, 3:49 p.m. UTC | #7

On Fri, 2009-07-17 at 06:58 -0700, David Brownell wrote:
> On Friday 17 July 2009, Steve Chen wrote:
> > On Thu, 2009-07-16 at 18:55 -0700, David Brownell wrote:
> > > On Thursday 16 July 2009, Paulraj, Sandeep wrote:
> > > > > >
> > > > > > There are differences also in the diff pin modes(3,4, 5 pins)
> > > > > > between all the different IPs.
> > > > > 
> > > > > I see, then.  Comments missing.  :)
> > > > > 
> > > > > This is the sort of thing which makes me want to put cpu_is_XXX()
> > > > > logic into drivers ... it's a good way to handle "lots of little
> > > > > differences".
> > > >
> > > > [Sandeep] What would be your suggestion? Should I leave it for
> > > > the time being?  
> > > 
> > > Yes. 
> > 
> > There used to be cpu_is_xxx logic in this and other DaVinci drivers.
> 
> "This" is a new driver.  :)

New in the community kernel yes :)  The DaVinci SPI driver has been in
TI/MV tree for over two years.  It wasn't until a few months ago that
the DaVinci SPI driver got in good enough shape to be submitted.  I know
of 4 people in MV (perhaps just as many people in TI) touched this
driver.

Steve

Patch

==================== CUT HERE
Minor fixes:
  * get rid of global "sdev" ... driver should work with
    more than one controller
  * rename set_bits() as set_io_bits(), minimizing confusion
    with standard set_bit() call; likewise clear_bits()
  * Version 1 chips can also handle SPI_NO_CS
  * Remove duplicate or otherwise unneeded #includes

Less-minor ones:
  * Remove confusion between buses (MOSI/MISO/SCK; one per SPI
    controller) and the chip-selects used to share them.  
  * Fix test for SPI_CPHA ...

Didn't touch:
  * All the stuff in davinci_spi_bufs_prep() which really belongs
    in davinci_spi_setup() ... it's just a slowdown, since the FMT
    register could be written just once.
  * The WDELAY and parity stuff in davinci_spi_bufs_prep() which
    doesn't kick in for version 1 controllers (even though they
    support it) and which is handled as *global* options instead
    of per-device ones (controls are in per-device FMT registers):
       (a) set it with other updates to FMT register
       (b) make those per-device policies

---
 drivers/spi/davinci_spi.c |  115 ++++++++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 56 deletions(-)

--- a/drivers/spi/davinci_spi.c
+++ b/drivers/spi/davinci_spi.c
@@ -16,20 +16,14 @@ 
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include <linux/types.h>
-#include <linux/io.h>
-#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
-#include <linux/interrupt.h>
 #include <linux/module.h>
-#include <linux/device.h>
 #include <linux/delay.h>
 #include <linux/platform_device.h>
-#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/clk.h>
-#include <linux/gpio.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_bitbang.h>
@@ -38,7 +32,6 @@ 
 
 #include "davinci_spi.h"
 
-struct device *sdev;
 
 static void davinci_spi_rx_buf_u8(u32 data, struct davinci_spi *davinci_spi)
 {
@@ -76,7 +69,7 @@  static u32 davinci_spi_tx_buf_u16(struct
 	return data;
 }
 
-static inline void set_bits(void __iomem *addr, u32 bits)
+static inline void set_io_bits(void __iomem *addr, u32 bits)
 {
 	u32 v = ioread32(addr);
 
@@ -84,7 +77,7 @@  static inline void set_bits(void __iomem
 	iowrite32(v, addr);
 }
 
-static inline void clear_bits(void __iomem *addr, u32 bits)
+static inline void clear_io_bits(void __iomem *addr, u32 bits)
 {
 	u32 v = ioread32(addr);
 
@@ -92,14 +85,14 @@  static inline void clear_bits(void __iom
 	iowrite32(v, addr);
 }
 
-static inline void set_fmt_bits(void __iomem *addr, u32 bits, int bus_num)
+static inline void set_fmt_bits(void __iomem *addr, u32 bits, int cs_num)
 {
-	set_bits(addr + SPIFMT0 + (0x4 * bus_num), bits);
+	set_io_bits(addr + SPIFMT0 + (0x4 * cs_num), bits);
 }
 
-static inline void clear_fmt_bits(void __iomem *addr, u32 bits, int bus_num)
+static inline void clear_fmt_bits(void __iomem *addr, u32 bits, int cs_num)
 {
-	clear_bits(addr + SPIFMT0 + (0x4 * bus_num), bits);
+	clear_io_bits(addr + SPIFMT0 + (0x4 * cs_num), bits);
 }
 
 /*
@@ -119,7 +112,7 @@  static void davinci_spi_chipselect(struc
 	 * line for the controller
 	 */
 	if (value == BITBANG_CS_INACTIVE) {
-		set_bits(davinci_spi->base + SPIDEF, CS_DEFAULT);
+		set_io_bits(davinci_spi->base + SPIDEF, CS_DEFAULT);
 
 		data1_reg_val |= CS_DEFAULT << SPIDAT1_CSNR_SHIFT;
 		iowrite32(data1_reg_val, davinci_spi->base + SPIDAT1);
@@ -179,14 +172,14 @@  static int davinci_spi_setup_transfer(st
 		hz = spi->max_speed_hz;
 
 	clear_fmt_bits(davinci_spi->base, SPIFMT_CHARLEN_MASK,
-			spi->master->bus_num);
+			spi->chip_select);
 	set_fmt_bits(davinci_spi->base, bits_per_word & 0x1f,
-			spi->master->bus_num);
+			spi->chip_select);
 
 	prescale = ((clk_get_rate(davinci_spi->clk) / hz) - 1) & 0xff;
 
-	clear_fmt_bits(davinci_spi->base, 0x0000ff00, spi->master->bus_num);
-	set_fmt_bits(davinci_spi->base, prescale << 8, spi->master->bus_num);
+	clear_fmt_bits(davinci_spi->base, 0x0000ff00, spi->chip_select);
+	set_fmt_bits(davinci_spi->base, prescale << 8, spi->chip_select);
 
 	return 0;
 }
@@ -226,84 +219,90 @@  static int davinci_spi_bufs_prep(struct 
 {
 	int op_mode = 0;
 
-	/* configuraton parameter for SPI */
+	/*
+	 * Set up device-specific SPI configuration parameters.
+	 * Most of these would normally be handled in spi_setup(),
+	 * updating the per-chipselect FMT registers, but some of
+	 * them use global controls like SPI_LOOP and SPI_READY.
+	 */
+
 	if (spi->mode & SPI_LSB_FIRST)
 		set_fmt_bits(davinci_spi->base, SPIFMT_SHIFTDIR_MASK,
-				spi->master->bus_num);
+				spi->chip_select);
 	else
 		clear_fmt_bits(davinci_spi->base, SPIFMT_SHIFTDIR_MASK,
-				spi->master->bus_num);
+				spi->chip_select);
 
 	if (spi->mode & SPI_CPOL)
 		set_fmt_bits(davinci_spi->base, SPIFMT_POLARITY_MASK,
-				spi->master->bus_num);
+				spi->chip_select);
 	else
 		clear_fmt_bits(davinci_spi->base, SPIFMT_POLARITY_MASK,
-				spi->master->bus_num);
+				spi->chip_select);
 
-	if (!(spi->mode & SPI_CPOL))
+	if (!(spi->mode & SPI_CPHA))
 		set_fmt_bits(davinci_spi->base, SPIFMT_PHASE_MASK,
-				spi->master->bus_num);
+				spi->chip_select);
 	else
 		clear_fmt_bits(davinci_spi->base, SPIFMT_PHASE_MASK,
-				spi->master->bus_num);
+				spi->chip_select);
 
 	if (davinci_spi->version == SPI_VERSION_2) {
 		clear_fmt_bits(davinci_spi->base, SPIFMT_WDELAY_MASK,
-				spi->master->bus_num);
+				spi->chip_select);
 		set_fmt_bits(davinci_spi->base,
-			    ((davinci_spi->pdata->wdelay <<
-				SPIFMT_WDELAY_SHIFT) &
-				SPIFMT_WDELAY_MASK),
-				spi->master->bus_num);
+				(davinci_spi->pdata->wdelay
+						<< SPIFMT_WDELAY_SHIFT)
+					& SPIFMT_WDELAY_MASK,
+				spi->chip_select);
 
 		if (davinci_spi->pdata->odd_parity)
 			set_fmt_bits(davinci_spi->base,
 					SPIFMT_ODD_PARITY_MASK,
-					spi->master->bus_num);
+					spi->chip_select);
 		else
 			clear_fmt_bits(davinci_spi->base,
 					SPIFMT_ODD_PARITY_MASK,
-					spi->master->bus_num);
+					spi->chip_select);
 
 		if (davinci_spi->pdata->parity_enable)
 			set_fmt_bits(davinci_spi->base,
 					SPIFMT_PARITYENA_MASK,
-					spi->master->bus_num);
+					spi->chip_select);
 		else
 			clear_fmt_bits(davinci_spi->base,
 					SPIFMT_PARITYENA_MASK,
-					spi->master->bus_num);
+					spi->chip_select);
 
 		if (davinci_spi->pdata->wait_enable)
 			set_fmt_bits(davinci_spi->base,
 					SPIFMT_WAITENA_MASK,
-					spi->master->bus_num);
+					spi->chip_select);
 		else
 			clear_fmt_bits(davinci_spi->base,
 					SPIFMT_WAITENA_MASK,
-					spi->master->bus_num);
+					spi->chip_select);
 
 		if (davinci_spi->pdata->timer_disable)
 			set_fmt_bits(davinci_spi->base,
 					SPIFMT_DISTIMER_MASK,
-					spi->master->bus_num);
+					spi->chip_select);
 		else
 			clear_fmt_bits(davinci_spi->base,
 					SPIFMT_DISTIMER_MASK,
-					spi->master->bus_num);
+					spi->chip_select);
 	}
 
 	/* Clock internal */
 	if (davinci_spi->pdata->clk_internal)
-		set_bits(davinci_spi->base + SPIGCR1,
+		set_io_bits(davinci_spi->base + SPIGCR1,
 				SPIGCR1_CLKMOD_MASK);
 	else
-		clear_bits(davinci_spi->base + SPIGCR1,
+		clear_io_bits(davinci_spi->base + SPIGCR1,
 				SPIGCR1_CLKMOD_MASK);
 
 	/* master mode default */
-	set_bits(davinci_spi->base + SPIGCR1, SPIGCR1_MASTER_MASK);
+	set_io_bits(davinci_spi->base + SPIGCR1, SPIGCR1_MASTER_MASK);
 
 	if (davinci_spi->pdata->intr_level)
 		iowrite32(SPI_INTLVL_1, davinci_spi->base + SPILVL);
@@ -311,12 +310,16 @@  static int davinci_spi_bufs_prep(struct 
 		iowrite32(SPI_INTLVL_0, davinci_spi->base + SPILVL);
 
 	/*
-	 * Standard SPI mode uses 4 pins, with chipselect
-	 * 3 pin SPI is a 4 pin variant without CS (SPI_NO_CS)
+	 * Version 1 hardware supports two basic SPI modes:
+	 *  - Standard SPI mode uses 4 pins, with chipselect
+	 *  - 3 pin SPI is a 4 pin variant without CS (SPI_NO_CS)
 	 *	(distinct from SPI_3WIRE, with just one data wire;
 	 *	or similar variants without MOSI or without MISO)
-	 * 4 pin with enable is (SPI_READY | SPI_NO_CS)
-	 * 5 pin SPI variant is standard SPI plus SPI_READY
+	 *
+	 * Version 2 hardware supports an optional handshaking signal,
+	 * so it can support two more modes:
+	 *  - 5 pin SPI variant is standard SPI plus SPI_READY
+	 *  - 4 pin with enable is (SPI_READY | SPI_NO_CS)
 	 */
 
 	op_mode = SPIPC0_DIFUN_MASK
@@ -330,10 +333,10 @@  static int davinci_spi_bufs_prep(struct 
 	iowrite32(op_mode, davinci_spi->base + SPIPC0);
 
 	if (spi->mode & SPI_LOOP)
-		set_bits(davinci_spi->base + SPIGCR1,
+		set_io_bits(davinci_spi->base + SPIGCR1,
 				SPIGCR1_LOOPBACK_MASK);
 	else
-		clear_bits(davinci_spi->base + SPIGCR1,
+		clear_io_bits(davinci_spi->base + SPIGCR1,
 				SPIGCR1_LOOPBACK_MASK);
 
 	return 0;
@@ -342,6 +345,7 @@  static int davinci_spi_bufs_prep(struct 
 static int davinci_spi_check_error(struct davinci_spi *davinci_spi,
 				   int int_status)
 {
+	struct device *sdev = davinci_spi->bitbang.master->dev.parent;
 
 	if (int_status & SPIFLG_TIMEOUT_MASK) {
 		dev_dbg(sdev, "SPI Time-out Error\n");
@@ -417,7 +421,7 @@  static int davinci_spi_bufs_pio(struct s
 		return ret;
 
 	/* Enable SPI */
-	set_bits(davinci_spi->base + SPIGCR1, SPIGCR1_SPIENA_MASK);
+	set_io_bits(davinci_spi->base + SPIGCR1, SPIGCR1_SPIENA_MASK);
 
 	iowrite32(0 | (pdata->c2tdelay << SPI_C2TDELAY_SHIFT) |
 			(pdata->t2cdelay << SPI_T2CDELAY_SHIFT),
@@ -427,7 +431,7 @@  static int davinci_spi_bufs_pio(struct s
 	data1_reg_val = pdata->cs_hold << SPIDAT1_CSHOLD_SHIFT;
 	tmp = ~(0x1 << spi->chip_select);
 
-	clear_bits(davinci_spi->base + SPIDEF, ~tmp);
+	clear_io_bits(davinci_spi->base + SPIDEF, ~tmp);
 
 	data1_reg_val |= tmp << SPIDAT1_CSNR_SHIFT;
 
@@ -437,7 +441,7 @@  static int davinci_spi_bufs_pio(struct s
 
 	/* Determine the command to execute READ or WRITE */
 	if (t->tx_buf) {
-		clear_bits(davinci_spi->base + SPIINT, SPIINT_MASKALL);
+		clear_io_bits(davinci_spi->base + SPIINT, SPIINT_MASKALL);
 
 		while (1) {
 			tx_data = davinci_spi->get_tx(davinci_spi);
@@ -490,7 +494,7 @@  static int davinci_spi_bufs_pio(struct s
 			int i;
 
 			for (i = 0; i < davinci_spi->count; i++) {
-				set_bits(davinci_spi->base + SPIINT,
+				set_io_bits(davinci_spi->base + SPIINT,
 						SPIINT_BITERR_INTR
 						| SPIINT_OVRRUN_INTR
 						| SPIINT_RX_INTR);
@@ -592,7 +596,6 @@  static int davinci_spi_probe(struct plat
 	}
 
 	dev_set_drvdata(&pdev->dev, master);
-	sdev = &pdev->dev;
 
 	davinci_spi = spi_master_get_devdata(master);
 	if (davinci_spi == NULL) {
@@ -659,9 +662,9 @@  static int davinci_spi_probe(struct plat
 	davinci_spi->bitbang.setup_transfer = davinci_spi_setup_transfer;
 	davinci_spi->bitbang.txrx_bufs = davinci_spi_bufs_pio;
 
-	davinci_spi->bitbang.flags = SPI_LSB_FIRST | SPI_LOOP;
+	davinci_spi->bitbang.flags = SPI_NO_CS | SPI_LSB_FIRST | SPI_LOOP;
 	if (davinci_spi->version == SPI_VERSION_2)
-		davinci_spi->bitbang.flags |= SPI_NO_CS | SPI_READY;
+		davinci_spi->bitbang.flags |= SPI_READY;
 
 	davinci_spi->version = pdata->version;
 	davinci_spi->get_rx = davinci_spi_rx_buf_u8;