[PATCH 2.6-git] SPI: add set_clock() to bitbang

[PATCH 2.6-git] SPI: add set_clock() to bitbang

[Vitaly Wool]

Hi David,

inlined is the small patch that adds set_clock function to the spi_bitbang structure.
Currently SPI bus clock can be configured either in chipselect() (which is _wrong_) or in txrx_buf (also doesn't encourage me much). Making it a separate function adds readability for the code.
Also, it seems to be redundant to set clock on each transfer, so it's proposed to do per-message clock setting. If SPI bus clock setting involves some PLL reconfiguration it's definitely gonna save some time.

Vitaly

Signed-off-by: Vitaly Wool <vwool@ru.mvista.com>

 drivers/spi/spi_bitbang.c       |    3 +++
 include/linux/spi/spi_bitbang.h |    1 +
 2 files changed, 4 insertions(+)

Index: linux-2.6.orig/drivers/spi/spi_bitbang.c
===================================================================
--- linux-2.6.orig.orig/drivers/spi/spi_bitbang.c
+++ linux-2.6.orig/drivers/spi/spi_bitbang.c
@@ -263,6 +263,9 @@ nsecs = 100;
 		chipselect = 0;
 		status = 0;
 
+		if (bitbang->set_clock)
+			bitbang->set_clock(spi);
+
 		for (;;t++) {
 			if (bitbang->shutdown) {
 				status = -ESHUTDOWN;
Index: linux-2.6.orig/include/linux/spi/spi_bitbang.h
===================================================================
--- linux-2.6.orig.orig/include/linux/spi/spi_bitbang.h
+++ linux-2.6.orig/include/linux/spi/spi_bitbang.h
@@ -31,6 +31,7 @@ struct spi_bitbang {
 	struct spi_master	*master;
 
 	void	(*chipselect)(struct spi_device *spi, int is_on);
+	void	(*set_clock)(struct spi_device *spi);
 
 	int	(*txrx_bufs)(struct spi_device *spi, struct spi_transfer *t);
 	u32	(*txrx_word[4])(struct spi_device *spi,

Re: [PATCH 2.6-git] SPI: add set_clock() to bitbang

[David Brownell]

On Thursday 22 December 2005 7:04 am, Vitaly Wool wrote:
> Hi David,
> 
> inlined is the small patch that adds set_clock function to the spi_bitbang structure.

This is actually not needed.  Clocks are set through the setup() method
in the spi_master, and controller drivers are (courtesy of the library
approach) free to provide their own.  Drivers for word-at-a-time hardware
would still need to call spi_bitbang_setup() in their own setup() code,
to set up the per-device controller_state, and spi_bitbang_cleanup() in
their own cleanup() code, to deallocate it.


> Currently SPI bus clock can be configured either in chipselect() (which is _wrong_)
> or in txrx_buf (also doesn't encourage me much). Making it a separate function adds
> readability for the code.   Also, it seems to be redundant to set clock on each
> transfer, so it's proposed to do per-message clock setting. If SPI bus clock
> setting involves some PLL reconfiguration it's definitely gonna save some time.  

Exactly why there's already spi_setup() in the common infrastruture,
and why the spi_bitbang_{setup,cleanup}() routines are exported for
simple drivers to shift-register level hardware.  The real "bang four
bits into protocol" style drivers won't need that since the bitbang
setup() calls calculate the delays used to satisfy those timings.
But hardware drivers -- for word-at-a-time or buffer-at-a-time style
usage -- would need that to set the clock dividers.

- Dave


/**
 * spi_setup -- setup SPI mode and clock rate
 * @spi: the device whose settings are being modified
 *
 * SPI protocol drivers may need to update the transfer mode if the
 * device doesn't work with the mode 0 default.  They may likewise need
 * to update clock rates or word sizes from initial values.  This function
 * changes those settings, and must be called from a context that can sleep.
 */
static inline int
spi_setup(struct spi_device *spi)
{
        return spi->master->setup(spi);
}

Re: [PATCH 2.6-git] SPI: add set_clock() to bitbang

[Vitaly Wool]

David Brownell wrote:

>On Thursday 22 December 2005 7:04 am, Vitaly Wool wrote:
>  
>
>>Hi David,
>>
>>inlined is the small patch that adds set_clock function to the spi_bitbang structure.
>>    
>>
>
>This is actually not needed.  Clocks are set through the setup() method
>in the spi_master, and controller drivers are (courtesy of the library
>approach) free to provide their own.  Drivers for word-at-a-time hardware
>would still need to call spi_bitbang_setup() in their own setup() code,
>to set up the per-device controller_state, and spi_bitbang_cleanup() in
>their own cleanup() code, to deallocate it.
>  
>
Where is it supposed to call setup? I guess it's anyway gonna be 
per-transfer, right?
Or am I missing something?

Vitaly

Re: [PATCH 2.6-git] SPI: add set_clock() to bitbang

[David Brownell]

> >This is actually not needed.  Clocks are set through the setup() method
> ...
>
> Where is it supposed to call setup? I guess it's anyway gonna be 
> per-transfer, right?
> Or am I missing something?

When the device is created, the core calls setup() to get things like
chipselect polarity sorted out and put into the inactive state.   That
matches the board-specific defaults associated with that device, which
would be a function of voltage, routing, and more.

And from then on, it'd be rare to ever call setup() again ... though
drivers certainly could do that between spi_message interactions with
a given device.

- Dave

Re: [PATCH 2.6-git] SPI: add set_clock() to bitbang

[Vitaly Wool]

David Brownell wrote:

>>>This is actually not needed.  Clocks are set through the setup() method
>>>      
>>>
>>...
>>
>>Where is it supposed to call setup? I guess it's anyway gonna be 
>>per-transfer, right?
>>Or am I missing something?
>>    
>>
>
>When the device is created, the core calls setup() to get things like
>chipselect polarity sorted out and put into the inactive state.   That
>matches the board-specific defaults associated with that device, which
>would be a function of voltage, routing, and more.
>
>And from then on, it'd be rare to ever call setup() again ... though
>drivers certainly could do that between spi_message interactions with
>a given device.
>  
>
No, suppose there're two devices behind the same SPI bus that have 
different clock constraints. As active SPI device change may well happen 
when each new message is processed, we'll need to set up clocks again 
for each message. Right?

Vitaly

Re: [PATCH 2.6-git] SPI: add set_clock() to bitbang

[David Brownell]

> No, suppose there're two devices behind the same SPI bus that have 
> different clock constraints. As active SPI device change may well happen 
> when each new message is processed, we'll need to set up clocks again 
> for each message. Right?

Clock is coupled to chipselect/device.  When the bus controller
switches to the other device, it updates the clock accordingly.

How exactly that's done is system-specific.  Many controllers
just have a register per chipselect, listing stuff like SPI mode,
clock divisor, and word size.  So switching to that chipselect
kicks those in automatically ... devices ignore the clock unless
they've been selected.

So it's like I said earlier.  And going to a new message will
normally involve a new chipselect, yes ... but maybe not, there's
that hint available to avoid the switching.

Re: [PATCH 2.6-git] SPI: add set_clock() to bitbang

[Vitaly Wool]

David Brownell wrote:

>>No, suppose there're two devices behind the same SPI bus that have 
>>different clock constraints. As active SPI device change may well happen 
>>when each new message is processed, we'll need to set up clocks again 
>>for each message. Right?
>>    
>>
>
>Clock is coupled to chipselect/device.  When the bus controller
>switches to the other device, it updates the clock accordingly.
>  
>
Yeah, but chipselect is called on per-transfer basis what is likely to 
be redundant for clock setting.
Per-message clock configuration is enough AFAIS.

>How exactly that's done is system-specific.  Many controllers
>just have a register per chipselect, listing stuff like SPI mode,
>clock divisor, and word size.  So switching to that chipselect
>kicks those in automatically ... devices ignore the clock unless
>they've been selected.
>  
>
Hmm, usually clocks are configured for the bus not device.
So, summarizing, you haven't convinced me yet. :)

Vitaly

Re: [PATCH 2.6-git] SPI: add set_clock() to bitbang

[David Brownell]

> >>No, suppose there're two devices behind the same SPI bus that have 
> >>different clock constraints. As active SPI device change may well happen 
> >>when each new message is processed, we'll need to set up clocks again 
> >>for each message. Right?
> >
> >Clock is coupled to chipselect/device.  When the bus controller
> >switches to the other device, it updates the clock accordingly.
> 
> Yeah, but chipselect is called on per-transfer basis what is likely to 
> be redundant for clock setting.

This is basic SPI protocol stuff ... chipselect activates once at
the start of a series ("message") of transfers, and deactivates
when the series completes.  It is NOT "per transfer" ... normally
it's active between transfers, as a basic protocol requirement.

There are exceptions related to cs_change, mostly so chipselect can
be dropped temporarily ... e.g. to terminate a protocol operation,
so the next one in that group can start; EEPROMs work that way.
But most devices keep chipselect active during the whole series
of transfers that make up a message.


> Per-message clock configuration is enough AFAIS.

It doesn't need to be reconfigured even that often; it's basically
once-per-device, except in rather exceptional case.

The controller does need to update its clock rate whenever it
starts talking to a different device, though.  Some devices max
out at 1 MHz, while others are happy at 40 MHz ... so when the
chipselect to one of those goes active, SCK has to match.


> >How exactly that's done is system-specific.  Many controllers
> >just have a register per chipselect, listing stuff like SPI mode,
> >clock divisor, and word size.  So switching to that chipselect
> >kicks those in automatically ... devices ignore the clock unless
> >they've been selected.
> 
> Hmm, usually clocks are configured for the bus not device.

Not a chance.  The clock is activated to talk to a given device;
and there's no requirement that all devices on the bus use the
same clock rate.  (If one chipselect gives access to a linked series
of devices, clearly they'll all need to be clocked alike.  But
that's not a bus, it's just a compound device ... like a big shift
register.)

I did my homework when putting that API together, and looked at
quite a few SPI controllers.  **Not one** of them forces all
their chipselets to use the same clock rate.

The closest thing to your description is the SSP hardware on PXA,
which doesn't _have_ the notion of device selection.  Chipselects
for SPI are layered on top of SSP, using GPIOs ... and, you may
have noticed in Stephen's PXA driver, updating the clock (and SPI
mode, etc) whenever the software starts talking to a new device.

- Dave

Re: [PATCH 2.6-git] SPI: add set_clock() to bitbang

[Vitaly Wool]

David Brownell wrote:

>>>How exactly that's done is system-specific.  Many controllers
>>>just have a register per chipselect, listing stuff like SPI mode,
>>>clock divisor, and word size.  So switching to that chipselect
>>>kicks those in automatically ... devices ignore the clock unless
>>>they've been selected.
>>>      
>>>
>>Hmm, usually clocks are configured for the bus not device.
>>    
>>
>
>Not a chance.  The clock is activated to talk to a given device;
>and there's no requirement that all devices on the bus use the
>same clock rate.  (If one chipselect gives access to a linked series
>of devices, clearly they'll all need to be clocked alike.  But
>that's not a bus, it's just a compound device ... like a big shift
>register.)
>
>I did my homework when putting that API together, and looked at
>quite a few SPI controllers.  **Not one** of them forces all
>their chipselets to use the same clock rate.
>  
>
I admit that thw word 'usually' is incorrect here, but still we have two 
Philips ARM boards where the SPI clock is configured _only_ on the bus, 
by setting the bus clock divisor on per-message basis. I was also 
keeping in mind PXA, so it wasn't just bare words...

Vitaly