Date: Thu, 29 Sep 2005 18:02:08 -0700
From: David Brownell <david-b@pacbell.net>
To: linux-kernel@vger.kernel.org, basicmark@yahoo.com
Subject: Re: [RFC][PATCH] SPI subsystem
Cc: dpervushin@ru.mvista.com
References: <20050918144520.36950.qmail@web30305.mail.mud.yahoo.com>
In-Reply-To: <20050918144520.36950.qmail@web30305.mail.mud.yahoo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Message-Id: <20050930010208.F410EE9E6B@adsl-69-107-32-110.dsl.pltn13.pacbell.net>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 16659
Lines: 466

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


> Date: Sun, 18 Sep 2005 15:45:20 +0100 (BST)
> From: Mark Underwood <basicmark@yahoo.com>

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.


> > +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".


> > +
> > + /* 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.

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!)


> > + /* 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.


> > +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.


> > +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.


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.


> > +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.

  * 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? 


> > +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) ...

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).


> > +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.


> > +/**
> > + * 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.

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().


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

- Dave


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