[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Guennadi Liakhovetski <lg at denx.de> wrote:
> On Thu, 8 May 2008, Haavard Skinnemoen wrote:
> > I think a claim/release interface is the best way to ensure that a
> > device driver can do multiple transfers more or less atomically (i.e.
> > without releasing the chip select and without having any traffic on the
> > bus in between) in a flexible way.
> 
> Is it compulsory then to claim / release a bus or not? Looks like it is 
> not from your description above. But - even if you don't claim a bus for 
> exclusive access, you still have to release it, right? Otherwise, if 
> someone has setup a slave without claiming the bus, noone will ever be 
> able to claim it afterwards? Because you shouldn't be able to claim it as 
> long as there are even non-exclusive users on the bus? Actually, what's 
> the mode variable in the setup method for? Maybe we could just use it to 
> configure exclusive / non-exclusive access? Then you get a simple API like

I'd say we should either make it compulsory or forget about it. You
always have exclusive access to the bus while doing a transfer, so
distinguishing between "exclusive" and "non-exclusive" users just
complicates things.

So we should just define spi_claim_bus() as a way of letting the
controller driver know that "I'm going to start talking to this
particular slave now", and let it handle it any way it chooses. We
could also use the interface to add debugging checks to verify that
we're not trying to access a different slaves while some driver thinks
it has exclusive access to the bus.

The "mode" variable in the setup function means exactly the same as it
does in your spi_select() function -- a bitwise combination of various
communication parameters like clock polarity, clock phase, etc.

Not all drivers may actually _need_ to do anything in the claim/release
functions though. I just think the interface makes sense conceptually,
and I also think it might improve performance with some drivers.

Another reason to have the claim/release interface is to allow
controller drivers to aggressively turn the controller on/off in order
to save power, or to ensure that everything is properly switched off
when booting the OS.

> slave = spi_setup_slave(host, dev, hz, 0 /*| SPI_ACCESS_EXCLUSIVE */);
> if (!slave)
> 	return;
> /* Now you MUST release the slave / host */
> spi_xfer();
> ...
> spi_release();

I think you should set up a slave exactly once before starting to
communicate with it. Once you've done that, you just execute sequences
of

spi_claim_bus(slave);
spi_xfer(slave, ...);
/* possibly more transfers */
spi_release_bus(slave);

We could also define commonly-used "canned" sequences, like spi_w8r8()
(write 8 bits, read 8 bits).

> And you get access to the bus if there are no exclusive users, and you get 
> exclusive access, only if there are no users currently at all.

I don't see how having multiple active users on the bus at the same
time can possibly make any sense...once you start a transfer, you
better make sure nobody else is doing a transfer at the same time since
you're using the same MOSI/MISO/SCK lines...

> What do we do, if there are several non-exclusive users on a bus, and one 
> of them doesn't release its cs between transfers? Return an error to 
> others if they try to communicate at this time?

That's a bug, but there are several ways the controller driver can try
to recover. The best one is probably to just deselect the CS that was
left active, possibly complaining a bit about it to the console.

spi_release_bus() could deactivate CS implicitly, making sure this
situation never happens in the first place.

> Looks like most of this API implementation is hardware independent, and 
> should go into an spi.c?

Not really...what "claiming" the bus really means is highly hardware
dependent. And SPI slave setup is mostly about decoding
hardware-independent parameters like SCK rate and mode bits into
hardware register values. But any convenience wrappers like spi_w8r8()
probably belongs somewhere hardware-independent.

Haavard