[U-Boot-Users] [RFC/PATCH 2/6] SPI API improvements

[Haavard Skinnemoen <haavard.skinnemoen@atmel.com>]

On Fri, 16 May 2008 12:41:04 +0200 (CEST)
Guennadi Liakhovetski <lg@denx.de> wrote:

> On Fri, 16 May 2008, Haavard Skinnemoen wrote:
> 
> > From: Haavard Skinnemoen <hskinnemoen@atmel.com>
> > 
> > This patch gets rid of the spi_chipsel table and adds a handful of new
> > functions that makes the SPI layer cleaner and more flexible.
> 
> Ok, this looks good to me now. And it works too. Just one question:

Great!

> >  	do {
> >  		reg = 0x2c000000;
> > -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day1);
> > +		err = spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day1,
> > +				SPI_XFER_BEGIN | SPI_XFER_END);
> >  
> >  		if (err)
> >  			return err;
> >  
> >  		reg = 0x28000000;
> > -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&time);
> > +		err = spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&time,
> > +				SPI_XFER_BEGIN | SPI_XFER_END);
> >  
> >  		if (err)
> >  			return err;
> >  
> >  		reg = 0x2c000000;
> > -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day2);
> > +		err = spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day2,
> > +				SPI_XFER_BEGIN | SPI_XFER_END);
> 
> Here... We perform 3 transfers on SPI one after another, and every time we 
> do "SPI_XFER_BEGIN | SPI_XFER_EN"... Doesn't this defeat the whole purpose 
> of these flags? Would it be bad, if we did

Well, no, I wouldn't say it defeats the purpose of these flags...

> >  		reg = 0x2c000000;
> > -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day1);
> > +		err = spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day1,
> > +				SPI_XFER_BEGIN);
> > - 
> > - 		if (err)
> > - 			return err;
> >  
> >  		reg = 0x28000000;
> > -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&time);
> > +		err |= spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&time, 0);
> > - 
> > - 		if (err)
> > - 			return err;
> >  
> >  		reg = 0x2c000000;
> > -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day2);
> > +		err |= spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day2,
> > +				SPI_XFER_END);
> >  
> >  		if (err)
> >  			return err;
> 
> ? The worst that can happen with this, is that the first or the second 
> transfer return an error, and we go on with one or two more transfers 
> instead of aborting immediately. Can this have any negative effects?

While I don't know the RTC in question well enough to say for sure,
there's a very real possibility that your suggestion simply won't work.

The purpose of the spi_xfer flags parameter isn't to gang together
multiple transfers -- it's to allow more precise chip select control so
that you can split a transfer into multiple spi_xfer() calls. This is
useful for devices where you typically start with a command, possibly
with a few bytes of parameters, then transfer data without releasing
the chip select in between.

The SPI flash driver posted later in this thread does this -- it first
sends a command along with any address bytes, and then starts the
actual data transfer with the buffer provided by the caller. The chip
select must stay active during the whole sequence, so without this
tweak, it would have to copy everything into a temporary buffer first.

If you try to combine multiple transfers into one like you did above,
the chip select will stay active all the time. And many devices expect
the chip select to go inactive after each command, and may simply
ignore all but the first or last if it stays active.

It could be that your solution works, but I wanted to mimic the
existing behaviour as closely as possible. And I definitely don't want
to "optimize" chip select control without knowing the device in
question very well.

> >  	reg = 0x2c000000 | day | 0x80000000;
> > -	spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day);
> > +	spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day,
> > +			SPI_XFER_BEGIN | SPI_XFER_END);
> >  
> >  	reg = 0x28000000 | time | 0x80000000;
> > -	spi_xfer(0, 32, (uchar *)&reg, (uchar *)&time);
> > +	spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&time,
> > +			SPI_XFER_BEGIN | SPI_XFER_END);
> > +
> > +	spi_release_bus(slave);
> >  }
> >  
> >  void rtc_reset(void)
> 
> Here error is not checked at all... So, it should be no problem doing only 
> SPI_XFER_BEGIN in the first xfer and only SPI_XFER_END in the second one.

No, I don't think that will work, for the reasons mentioned above.
There's no error checking because the existing code doesn't check for
errors...and there's no way to return an error status from the function
anyway.

Btw, there are a few spi_xfer() semantics that I want to be a bit more
clearly defined, but I forgot to document it before sending the patch.
These are:
  * If spi_xfer() fails, it automatically terminates the transfer as if
    the SPI_XFER_END flag was set.
  * If bitlen == 0, spi_xfer() must deactivate CS if the SPI_XFER_END
    flag is set.
  * If dout == NULL, spi_xfer() will transmit unspecified data.
    Alternatively, we could specify that it must send zeroes.
  * If din == NULL, spi_xfer() will ignore any received data.

If people agree that these semantics make sense, we should probably go
through the drivers and make sure it's handled appropriately.

Oh, and I want to do something about the "bitlen" parameter, but I
don't wanna open that particular can of worms in this patchset :-)

Haavard