Re: [RFC][PATCH] SPI subsystem

[Mark Underwood]

--- David Brownell <david-b@xxxxxxxxxxx> wrote:

> Sorry for the delay getting back to these comments; I wanted to
> give them proper attention, which kept not arriving.
> 

I'm glad I'm not the only one :)

> 
> > Date: Sun, 18 Sep 2005 15:45:20 +0100 (BST)
> > From: Mark Underwood <basicmark@xxxxxxxxx>
> 
> First comments for <linux/spi.h>:
> 
> > > +struct spi_device { /* this proxies the device through a master */
> > > + struct device  dev;
> > > + struct spi_master *master;
> > > + u32   max_speed_hz;
> > > + u8   chip_select;
> > > + u8   mode;
> > > +#define SPI_CPHA 0x01  /* clock phase */
> > > +#define SPI_CPOL 0x02  /* clock polarity */
> > > +#define SPI_MODE_0 (0|0)
> > > +#define SPI_MODE_1 (0|SPI_CPHA)
> > > +#define SPI_MODE_2 (SPI_CPOL|0)
> > > +#define SPI_MODE_3 (SPI_CPOL|SPI_CPHA)
> >
> > Would be more flexable to have this in the message or even
> > the spi_transfer structure. Although I
> > don't know who would need this flexability.
> 
> In this case, I don't see a benefit.  The chips support only one
> signaling method at a time.  It can be changed between requests,
> by calling spi_setup(...), but even that will be rare.  I don't
> think there's any point to encouraging finer grained changes.
> 
> 
> > > +struct spi_master {
> > > + ...
> > > +};
> >
> > I notice that there is no bus lock. Are you expecting the adapter
> > driver to handle the fact that its transfer routine could be called
> > before a previous call returns?
> 
> Yes.  The transfer routine is purely async, and its responsibility
> is to append that spi_message to the current queue.  (Assuming
> the driver isn't a simple pure-PIO driver without a queue...)
> 
> That's a simple matter of a spin_lock_irqsave/list_add_tail/unlock.
> 

OK. Thought so. I think that in the documentation (when it gets written ;) we need to warn people
that they can only do quick work (adding message to a queue or waking up a kthread) in the
transfer routine as it would not be fair for a PIO driver to transfer several KB in what might be
interrupt context.

> 
> > > +struct spi_transfer {
> > > + /* it's ok if tx_buf == rx_buf (right?)
> > > +  * for MicroWire, one buffer must be null
> > > +  * buffers must work with dma_*map_single() calls
> > > +  */
> > > + void  *tx_buf, *rx_buf;
> > > + unsigned len;
> > > +};
> >
> > I would like more flexability. I might want to toggle the CS line within
> > a message or another CS line which is really a GPO pin used for register
> > select. For example a char LCD with SPI interface
> > would require this and yes, they do exist! I've used one :).
> 
> I've been persuaded that at least the "toggle chipselect" thing
> is needed, because of chips like the CS8415A (or ISTR some EEPROMs)
> that read by starting a write (to set a data pointer), dropping
> chipselect temporarily, then issuing the read.  Those all need
> to be treated as single "spi_message".
> 

More convergence, good :)

> 
> > > +
> > > + /* Optionally leave this chipselect active afterward */
> > > + unsigned  csrel_disable:1;
> >
> > This would be a disaster as anther SPI device driver might have
> > put a transfer straight after this one, in which case that message
> > would be sent to both devices :(, or has the driver that did this
> > take a lock on the bus? If so what lock?.
> 
> That's not how it works.  No spi_message starts unless _only_ that
> device's chipselect is active.  If some other chipselect is still
> active, it must first be turned off.  No lock needed, beyond the fact
> that the controller has only one queue and driver ... that driver
> ensures many correctness issues, not just this one.

I see. Sorry I'm mixing up our subsytems :(.

> 
> The point of that option is to minimize the overhead of starting a
> new transaction to a "favored" device.  I understand that's needed
> with some of the CORGI (Zaurus) touchscreen support.  (Along with
> some other funky stuff like vertical retrace synchronization!)
> 

Yes that is something that I have started to think about with respect to adding messages in
callback context (e.g. a network device which you have to to a write/read combination to get the
amount of data in the buffer and then just contine reading to get the data). But I want to get
what we have at the moment sorted before moving onto things like that.

> 
> > > + /* completion is reported this way */
> > > + void    (*complete)(void *context);
> > > + void   *context;
> > > + unsigned  actual_length;
> > > + int   status;
> > > +
> > > + /* for optional controller driver use */
> > > + struct list_head queue;
> >
> > If your putting this here wouldn't it make sense to also add
> > a list_head to the adapter structure?
> 
> That's only the first of many chunks of driver-private data
> they'll need.  And as I've commented before, there's no business
> any other software has touching that queue ... assuming the
> controller driver is even written to use a queue.
> 
> (Many current SPI drivers just spin using PIO to complete requests,
> and they could be pretty easily converted to this framework without
> forcing that character to change right away ...)
> 
> 
> > > +};
> >
> > Clock speed should also be in this structure as a SPI device might
> > want to change the speed it's clocked at for each message.
> > For example MMC cards are probed at 400KHz but can be read/written to
> > at up to 25MHz.
> 
> The way I see that being done is by just calling spi_setup() to
> update the device speed.  That's a direct mirror of how it's
> done in the MMC (or PCMCIA) stacks:  a separate call to set
> the I/O mode parameters.
> 
> 
> > A priv pointer would be very usefull as I could allocate enough
> > memory for my message structure plus the transfer items and any
> > other thing(s) that I need to store and then set priv to point to
> > my area of memory (like you can for skb's).
> 
> Yes, the latest version has spi_message.state, a void *pointer
> for use by whoever currently owns that message.
> 

Thank you :)

> 
> > > +static inline int
> > > +spi_setup(struct spi_device *spi)
> > > +{
> > > + return spi->master->setup(spi);
> > > +}
> > > +
> >
> > Where would this be used? Surely only the adapter could do this
> > as the SPI device driver and core only knows when it sends the
> > request for a transfer, not when the transfer actually happens.
> 
> See above ... that's how the clock speed would be changed, or
> how various other long-lasting SPI protocol tweaks would kick in.
> 
> This doesn't _need_ to touch chip registers, though it can.
> It just changes i/o characterics for that specific device.
> 

So your asking the adapter to keep a 'personality' for each device on that bus (clock speed, cs &
clock mode etc) and then just before the transfer to/from a device is started the adapter takes
the 'personality' of that device (i.e. sets clock speed registers if needed etc)?

> 
> > > +static inline int
> > > +spi_async(struct spi_device *spi, struct spi_message *message)
> > > +{
> > > + message->dev = spi;
> > > + return spi->master->transfer(spi, message);
> > > +}
> >
> >
> > Couldn't/shouldn't this be in the core, otherwise it looks like
> > you can only do sync transfers (or
> > maybe some comment to say that it's in the header file).
> 
> The header file IS part of the core, and that's where that little
> routine is declared (so no need for a comment saying that).  The
> headerdefines the interface to the core.  Code running in an IRQ
> (or BH) will use spi_async(), while code running in a sleeping
> context could use spi_sync() if it likes.
> 
> And spi_sync() is sort-of-core; really it's just a veneer over
> that core async I/O primitive, but one that's so small (and easy
> to use) that it's worth paying the price to have it "everywhere".
> 
> (Remember, we're still talking about 2 KBytes ARM object code...)
> 
> 
> > > +static inline void
> > > +spi_unregister_device(struct spi_device *spi)
> > > +{
> > > + if (spi)
> > > +  device_unregister(&spi->dev);
> > > +}
> > > +
> >
> > Couldn't/shouldn't this be in the core, otherwise it looks like
> > you can only register a device and
> > not unregister (or maybe some comment to say that it's in the header file).
> 
> That immediately follows the spi_new_device() declaration;
> the device would have been registered using that call.
> 
> Really, I don't see much need for either function except to
> handle the sort of "hotplug an SPI adapter" code you were
> talking about.  The reason they're inlined there is not
> because they're "not core"; it's because they'll be used
> so infrequenty that nobody else should pay the cost for
> them to exist.
> 

OK.

> 
> And now stuff from "spi.c":
> 
> > > +static int spi_suspend(struct device *dev, pm_message_t message)
> > > +{
> > > + if (dev->driver && dev->driver->suspend)
> > > +  return dev->driver->suspend(dev, message, SUSPEND_POWER_DOWN);
> > > + else
> > > +  return 0;
> > > +}
> 
> Actually those aren't quite right; the dev->power.power_state fields
> need to be updated.  Otherwise only sysfs will be doing it.
> 
> > > +static int spi_resume(struct device *dev)
> > > +{
> > > + if (dev->driver && dev->driver->resume)
> > > +  return dev->driver->resume(dev, RESUME_POWER_ON);
> > > + else
> > > +  return 0;
> > > +}
> >
> > What happens about all the devices sitting on the adapter?
> 
> That's the suspend routine for those devices.  The adapter
> would have a separate suspend routine ...
> 
> > Does the driver core suspend them for you? If so could you
> > show me where because I missed it.
> 
> Good point.  It's arguably a weakness in the driver core.
> Meanwhile, what I've done elsewhere is basically
> 
>  device_for_each_driver(... fail_if_not_suspended);
> 
> The invariant for the spi_master would be that it needs to
> ensure that its children (the spi_device objects) are all
> suspended -- if they have a driver, that is.
> 

OK. Thats what I did. I guess the reason that the driver core can't do it is because some busses
may have to do it differently from others.

> 
> > > +struct spi_device *__init_or_module
> > > +spi_new_device(struct spi_master *master, struct spi_board_info *chip)
> > > +{
> > > + ...
> > > +
> > > + /* drivers may modify this default i/o setup */
> > > + status = master->setup(proxy);
> >
> > How would this work if two devices work in a different mode?
> >
> > Example:
> > SPI device A works in mode 0 and so the adapter is setup to mode 0.
> > SPI device B works in mode 1 and so the adapter is setup to mode 1.
> 
> That's the wrong starting point.  It's not the adapter that's set
> to a given mode ... it's the interactions with a given device.
> 
> > Device A does a transfer, which it should be done in mode 0, but
> > the transfer is actually done in
> > mode 1 as the last call to setup was for mode 1.
> 
> No, device A would never be used in the wrong mode.  That's
> a constraint that the spi_master must implement.
> 
> 
> > Setting up of the mode and clock should only be done in the context
> > of a message (and I mean when a message is transfered, not when it's
> > queued) as then and only then are the settings relevant and
> > you can guaranty that your not interfering with the settings for
> > other devices on the bus.
> 
> Not exactly.  Think of different kinds of SPI controller:
> 
>   * Like the PXA SSP.  An spi_master for that controller will either
>     implement its own chipselects using GPIOs, manually bank-switching
>     the registers ... or it won't use chipselects, so it'll never
>     need to change the registers.  Either way, the register settings
>     will be associated with the device, not the controller.
> 
>     So master->setup() can just update the copy of the registers
>     used for that device, and they'll be used to set up the controller
>     the next time a transfer to that device is started.

Right, so the answer to one of my questions above is yes, the adapter is expected to store a
'personality' for each device on the bus for adapters that don't support it in hardware.
This does mean that if a SPI driver wants to send messages at different speeds in the callback of
the current message it would have to change the speed for the next message.

> 
>   * Like the AT91rm9200 SPI.  Each chipselect has a dedicated
>     register covering mode, clock, and some delays.
> 
>     So master->setup() can just update the registers directly,
>     it won't need to copy them when it starts a transfer, and
>     starting a transfer involves fewer register writes.
> 
> If the driver for an SPI controller gets the settings wrong, that'd
> be a bug just like reading or writing the wrong data.
> 
> 
> > > +EXPORT_SYMBOL_GPL(spi_new_device);
> >
> > I think we should have a bus lock (in the adapter structure) for
> > safety, and in the remove routine as well.
> 
> Why?  I don't see any need for one, at least in the "all drivers
> must use this one" category.  Persuade me; what problems would such
> a lock solve? 
> 

Problems with parallel calls to register spi device/unregister spi device/transfer?

> 
> > > +int __init_or_module  // would be __init except for SPI_EXAMPLE
> > > +spi_register_board_info(struct spi_board_info const *info, unsigned n)
> > > +...
> >
> > This function should call scan_boardinfo as there may be devices in this
> > list that sit on adapters that have been registered already.
> 
> Not easily.  Remember, this is called from the board init code,
> normally in arch_initcall() which is before drivers are expected
> to start registering...
> 
> 
> > Please can we have a 'undo' version (the general rule being you
> > should be able to undo what you have done ;), i.e.
> 
> That rule isn't really followed for board init code though.  There's
> no point, since it's not like the board could transmogrify itself!
> The parts registered there can't physically vanish.
> 
> 
> > spi_unregister_board_info as I might have two different parallel port
> > boards (one with EEPROM and one with Ethernet for example) and I
> > don't want to have to reset my PC to switch between the two.
> 
> The parallel port adapter wouldn't use that interface.  It would
> instead be using spi_new_device() with board_info matching the
> device (Ethernet, EEPROM, USB controller, etc) ...

OK. So if I had an array of devices then I have to go though that array and call  spi_new_device()
for each one?
Where do I get spi_master from? I need a function to which I can pass the name/bus number to and
get a spi_master pointer in return.

> 
> Then those devices would automatically vanish (in the latest code)
> when when the adapter calls the spi_unregister_master() routine.  
> 
> 
> > > +int __init_or_module
> > > +spi_register_master(struct device *dev, struct spi_master *master)
> > > +{
> > > + static atomic_t  dyn_bus_id = ATOMIC_INIT(0);
> > > + int   status = -ENODEV;
> > > +
> > > + if (list_empty(&board_list)) {
> > > +  dev_dbg(dev, "spi board info is missing\n");
> > > +  goto done;
> > > + }
> >
> > Why is the fact the there is no board information registered at the moment
> > a reason to fail?
> > I thought I could register adapters and board/platform information in any
> > order I wanted.
> 
> It's not; I recenty ripped that code out.  For your case of a
> parallel port adapter, there would never be one.  Only for
> "normal" situations would "nothing declared" be fishy, and
> it's not really worth even a warning.
> 
> 
> > > +void spi_unregister_master(struct spi_master *master)
> > > +{
> > > +/* REVISIT when do children get deleted? */
> > > + class_device_unregister(&master->cdev);
> > > +
> > > + put_device(master->cdev.dev);
> > > + master->cdev.dev = NULL;
> > > +
> > > +}
> > > +EXPORT_SYMBOL_GPL(spi_unregister_master);
> > > +
> >
> > Does this work? Adding a child device will cause the parent devices
> > ref count to be incremented so
> > surely you have to release all the children first.
> 
> I finally revisited that and added the code to unregister the
> children (right there).
> 

OK. There is an example of how to do this in my code :)

> 
> > > +int spi_sync(struct spi_device *spi, struct spi_message *message)
> > > +{
> > > + DECLARE_COMPLETION(done);
> > > + int status;
> > > +
> > > + message->complete = spi_sync_complete;
> > > + message->context = &done;
> > > + status = spi_async(spi, message);
> > > + if (status == 0)
> > > +  wait_for_completion(&done);
> > > + message->context = NULL;
> > > + return status;
> > > +}
> > > +EXPORT_SYMBOL(spi_sync);
> >
> > Why not combine spi_sync and spi_async and just check for a NULL pointer
> > in callback? If the callback/complete pointer is NULL then it's a sync
> > transfer else it's an async transfer.
> 
> No, there is only ** one ** way to report completion and that's
> through the callback.  All transfers are async at the low level.
> This small wrapper just uses the async notification callback to
> wake up a thread, so that thread has a synchronous model.
> 

Sorry I didn't make myself clear. I mean check the complete element in the spi_message structure
when spi_transfer is called. So:

int spi_transfer(struct spi_device *spi, struct spi_message *message)
{
        if (message->complete)
                /* We have callback so transfer is async */
        else
                /* We have no callback so transfer is sync */
}

Although thinking about it this is probably a bad idea as it could be prone to errors as people
who want an async transfer might forget/not need to set the complete element and would get a sync
transfer instead :(.

> 
> > > +/**
> > > + * spi_w8r8 - SPI synchronous 8 bit write followed by 8 bit read
> > > + * @spi: device with which data will be exchanged
> > > + * @cmd: command to be written before data is read back
> > > + *
> > > + * This returns the (unsigned) eight bit number returned by the
> > > + * device, or else a negative error code.
> > > + */
> 
> > > +/**
> > > + * spi_w8r16 - SPI synchronous 8 bit write followed by 16 bit read
> > > + * @spi: device with which data will be exchanged
> > > + * @cmd: command to be written before data is read back
> > > + *
> > > + * This returns the (unsigned) sixteen bit number returned by the
> > > + * device, or else a negative error code.
> > > + */
> >
> > Should these live in the core? I know they don't take up much space
> > but if I don't need them why should I have to have them?
> > What about putting these as inline functions in spi.h?
> 
> Agreed.  The latest version does just that ... but it also has
> a new helper function to call (write X bytes, read Y bytes back)
> to help keep the nonsharable/inlined parts small.
> 
> 
> > Hmm, using local variables for messages, so DMA adapter drivers have
> > to check if this is non-kmalloc'ed space (how?)
> 
> They can't check that.  It turns out that most current Linuxes
> have no issues DMAing a few bytes from the stack.

Will the DMA remapping calls work with data from the stack?

> 
> But if we ever get a version where that's an issue -- or someone
> feels compelled to clean up that little issue, despite the fact that
> doing that creates a performance hit! -- the write_then_read() call
> could get some minor tweaks.
> 
> 
> > and either do a non DMA transfer or copy the data into a kmalloc'ed
> > area of memory to do the DMA from/to. It would make the adapter drivers
> > life easier if we stipulated that all messages must be kmalloc'ed.
> 
> The true requirement is already documented:  that all buffers must
> work with dma_{map,unmap}_single().  That's less restrictive than
> saying they've got to come from kmalloc().
> 

OK. That's what I meant :)

> 
> The "maybe-nice" thing that's not supported there is letting
> drivers provide their own DMA addresses, already mapped.  If we
> ever need such a thing, it can be done; but IMO there's not a
> lot of point to it quite yet.  Wait until we have the block
> layer handing scatterlists down to some SPI device; then the
> dma_map_sg() stuff will make us want that

I agree.

Mark

> 
> - Dave
> 
> 
> > Mark
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 



		
___________________________________________________________ 
To help you stay safe and secure online, we've developed the all new Yahoo! Security Centre. http://uk.security.yahoo.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/