Re: [PATCH 3/4] spi: Add OF binding support for SPI busses

Previous thread: none

Next thread: tip build broken on sparc64... by David Miller on Friday, May 16, 2008 - 1:07 pm. (4 messages)
From: Grant Likely
Date: Friday, May 16, 2008 - 12:35 pm

This series is a set of changes to allow the slaves on an SPI bus to be
described in the OF device tree (useful in arch/powerpc) and adds a driver
that uses it (the Freescale MPC5200 SoC's SPI device).

Please review and comment.  David, I've included in this series my earlier
patch to change modalias from a pointer to a string as one of the later
patches depends on it.

Cheers,
g.

--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
--

From: Grant Likely
Date: Friday, May 16, 2008 - 12:36 pm

From: Grant Likely <grant.likely@secretlab.ca>

Currently, 'modalias' in the spi_device structure is a 'const char *'.
The spi_new_device() function fills in the modalias value from a passed
in spi_board_info data block.  Since it is a pointer copy, the new
spi_device remains dependent on the spi_board_info structure after the
new spi_device is registered (no other fields in spi_device directly
depend on the spi_board_info structure; all of the other data is copied).

This causes a problem when dynamically propulating the list of attached
SPI devices.  For example, in arch/powerpc, the list of SPI devices can
be populated from data in the device tree.  With the current code, the
device tree adapter must kmalloc() a new spi_board_info structure for
each new SPI device it finds in the device tree, and there is no simple
mechanism in place for keeping track of these allocations.

This patch changes modalias from a 'const char *' to a fixed char array.
By copying the modalias string instead of referencing it, the dependency
on the spi_board_info structure is eliminated and an outside caller does
not need to maintain a separate spi_board_info allocation for each device.

If searched through the code to the best of my ability for any references
to modalias which may be affected by this change and haven't found anything.
It has been tested with the lite5200b platform in arch/powerpc.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/spi/spi.c       |    2 +-
 include/linux/spi/spi.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 1ad12af..bdf1b70 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -229,7 +229,7 @@ struct spi_device *spi_new_device(struct spi_master *master,
 	proxy->max_speed_hz = chip->max_speed_hz;
 	proxy->mode = chip->mode;
 	proxy->irq = chip->irq;
-	proxy->modalias = chip->modalias;
+	strncpy(proxy->modalias, chip->modalias, KOBJ_NAME_LEN);
 
 ...
From: Grant Likely
Date: Friday, May 16, 2008 - 12:36 pm

From: Grant Likely <grant.likely@secretlab.ca>

spi_new_device() allocates and registers an spi device all in one swoop.
If the driver needs to add extra data to the spi_device before it is
registered, then this causes problems.

This patch splits the allocation and registration portions of code out
of spi_new_device() and creates three new functions; spi_alloc_device(),
spi_register_device(), and spi_device_release().  spi_new_device() is
modified to use the new functions for allocation and registration.
None of the existing users of spi_new_device() should be affected by
this change.

Drivers using the new API can forego the use of an spi_board_info
structure to describe the device layout and populate data into the
spi_device structure directly.

This change is in preparation for adding an OF device tree parser to
generate spi_devices based on data in the device tree.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/spi/spi.c       |  150 +++++++++++++++++++++++++++++++++--------------
 include/linux/spi/spi.h |   13 ++++
 2 files changed, 119 insertions(+), 44 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index bdf1b70..9c7a84d 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -178,6 +178,107 @@ struct boardinfo {
 static LIST_HEAD(board_list);
 static DEFINE_MUTEX(board_lock);
 
+/**
+ * spi_alloc_device - Allocate a new SPI device
+ * @master: Controller to which device is connected
+ * Context: can sleep
+ *
+ * Allows a driver to allocate and initialize and spi_device without
+ * registering it immediately.  This allows a driver to directly
+ * fill the spi_device with device parameters before calling
+ * spi_register_device() on it.
+ *
+ * Caller is responsible to call spi_register_device on the returned
+ * spi_device structure.
+ *
+ * Returns a pointer to the new device, or NULL.
+ */
+struct spi_device *spi_alloc_device(struct spi_master *master)
+{
+	struct spi_device	*spi;
+	struct device		*dev ...
From: David Brownell
Date: Wednesday, May 21, 2008 - 5:17 pm

I have no problem with the first two, but why the last?

If the devices are always allocated by spi_alloc_device() as
they should be -- probably through an intermediary -- the
only public function necessary for that cleanup should be
the existing spi_dev_put().

- Dave
--

From: Grant Likely
Date: Friday, May 23, 2008 - 11:43 pm

Ah, okay.  I'm still a bit fuzzy on the device model conventions.
I'll remove that then.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Grant Likely
Date: Friday, May 23, 2008 - 11:54 pm

On Sat, May 24, 2008 at 12:43 AM, Grant Likely

Question:  spi_alloc_device() (and the original code) does a
spi_master_get() on the spi_master device.  Doesn't spi_master_put()
need to be called when the device is discarded?  spi_dev_put() doesn't
do that explicitly; is it an implicit operation after a device has
been deregistered from the spi_master?

Thanks,



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: David Brownell
Date: Sunday, June 29, 2008 - 9:08 pm

Depends whether or not the add() has been done to hook things into
the driver model tree, as I recall.  The add() presumes things are
properly refcounted.  When you make a driver model tree node vanish,
its associated refcounts get updated too.

- Dave

--

From: Grant Likely
Date: Tuesday, June 17, 2008 - 12:28 am

On Sat, May 24, 2008 at 12:43 AM, Grant Likely

I've dug into this some more.  spi_alloc_device only allocates the
memory.  It doesn't call device_initialize() to initialize the kref.
All of that behaviour is handled within device_register().  Therefore
if a driver uses spi_alloc_device() and then if a later part of the
initialization fails before spi_register_device() is called, then the
alloc'd memory needs to be freed, but spi_dev_put() won't work because
the kobj isn't set up so I need another function to handle freeing it
in on a failure path.

Should I switch things around to do device_initialize() in the alloc
function and call device_add() instead of device_register() in the
spi_register_device() function?  Is that sufficient to make
put_device() work?

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: David Brownell
Date: Sunday, June 29, 2008 - 9:10 pm

Well, the driver model idiom is initialize() then add(), with
register() calls combining the two.  An alloc() is just a bit
outside those core idioms ...

But one alloc() example is platform_device_alloc(), which does
the device_initialize() call ... followed by platform_device_add().

The spi_new_device() call does a bunch of stuff beyond a register(),



You should also rename it to spi_add_device(), since register()
calls always do the initialize() rather than having it done for
them in advance.  People rely on those names supporting that

Looks like it to me.  Calling device_initialize() will
do a kobject_init(), which is documented as requiring
a kobject_put() to clean up ... that's all put_device()
will ever do.

- Dave


--

From: Grant Likely
Date: Friday, May 16, 2008 - 12:36 pm

From: Grant Likely <grant.likely@secretlab.ca>

This patch adds support for populating an SPI bus based on data in the
OF device tree.  This is useful for powerpc platforms which use the
device tree instead of discrete code for describing platform layout.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 Documentation/powerpc/booting-without-of.txt |   61 ++++++++++++++++++
 drivers/spi/Kconfig                          |    4 +
 drivers/spi/Makefile                         |    1 
 drivers/spi/spi_of.c                         |   86 ++++++++++++++++++++++++++
 include/linux/spi/spi_of.h                   |   18 +++++
 5 files changed, 170 insertions(+), 0 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 1d2a772..452c242 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -58,6 +58,7 @@ Table of Contents
       o) Xilinx IP cores
       p) Freescale Synchronous Serial Interface
 	  q) USB EHCI controllers
+      s) SPI busses
 
   VII - Marvell Discovery mv64[345]6x System Controller chips
     1) The /system-controller node
@@ -2870,6 +2871,66 @@ platforms are moved over to use the flattened-device-tree model.
 		reg = <0xe8000000 32>;
 	};
 
+    s) SPI (Serial Peripheral Interface) busses
+
+    SPI busses can be described with a node for the SPI master device
+    and a set of child nodes for each SPI slave on the bus.  For this
+    discussion, it is assumed that the system's SPI controller is in
+    SPI master mode.  This binding does not describe SPI controllers
+    in slave mode.
+
+    The SPI master node requires the following properties:
+    - #address-cells  - number of cells required to define a chip select
+			address on the SPI bus.
+    - #size-cells     - should be zero.
+    - compatible      - name of SPI bus controller following generic names
+			recommended practice.
+    No other properties are ...
From: Randy Dunlap
Date: Friday, May 16, 2008 - 1:47 pm

~~~

Hi,
You mostly capitalize "SPI" in sentences (i.e., when it's not part of
a function name or OF data), so could the 3 underlined instances of it
also be all caps?

Thanks,
---
~Randy
--

From: Grant Likely
Date: Friday, May 16, 2008 - 1:51 pm

No problem.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Anton Vorontsov
Date: Friday, May 16, 2008 - 3:03 pm

No way to pass platform data... can you suggest any idea to use
this for things like
"[POWERPC] 86xx: mpc8610_hpcd: add support for SPI and MMC-over-SPI"
I've sent just recently...?

Maybe this code could do something like
spi->dev.platform_data = nc->data;
and board code would fill nc->data at early stages? This needs to be a
convention, not just random use though.. Maybe we can expand the struct
device_node to explicitly include .platform_data for such cases?

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
--

From: Grant Likely
Date: Friday, May 16, 2008 - 3:14 pm

That's right.  platform_data being a very driver specific thing there
is no way to generically extract a pdata structure from the device
tree.  Instead, I'm storing the device node in archdata.of_node (line
immediately above spi_register_device) so that drivers can read the
device node themselves to populate a platform_device structure.

Hmmm, as you say, this could end up being rather messy.  However, by
passing the device node pointer, the driver could extract that data on
a per case basis.  (ie. it would be decided on a per driver basis
where to get the platform data).  I'm not sure; this bears more
thought...

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Anton Vorontsov
Date: Friday, May 16, 2008 - 3:49 pm

Sometimes it's not worth powder and shot adding OF functionality to
the drivers, I2C and SPI are major examples. Another [not mmc_spi]
example is drivers/input/touchscreen/ads7846.c, which is SPI driver
and needs platform data. There is a board that needs this (touchscreen
controller on a MPC8360E-RDK).

Also there is no way to pass functions via device tree, we're
always end up doing board-specific hooks in the generic drivers...

Finally, let's call this platform_data and be done with it. Then we
can use this for things like drivers/video/fsl-diu-fb.c (see diu_ops,
which is global struct, filled by
arch/powerpc/platforms/86xx/mpc8610_hpcd.c).

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
--

From: Grant Likely
Date: Friday, May 16, 2008 - 10:02 pm

In my mind; platform_data and the device tree are all about the same
thing: representation.  In other words, how to describe the
configuration of the hardware independent of the driver itself.  The
device tree and platform data structures both solve the same problem.
In both cases, the representation data must be
translated/decoded/interpreted and stored in the drivers own private
data structure so it can be of use.

One of the things I find rather interesting is just how frequently
drivers using platform data structures have a big block of code which
simply copy pdata fields into identically named fields in the device
private data... specifically: copied discretely instead of being a
verbatim block that can be memcopied, or instead of maintaining a
pointer and using the pdata itself.  It highlights for me just how
much pdata structures are decoupled from the driver itself (just like
how the device tree data is decoupled from the driver)... but I
digress.

The point is that the translation of data from the device tree (and
from pdata for that matter) to a form usable by the driver has to live
*somewhere*.  Does it belong in the platform code?  Or should it live
with the driver itself?  I argue that it really belongs as much as
feasibly possible with the driver code.  Even if a pdata structure is
chosen to be used as an intermediary representation, the code is only
relevant to the driver and therefore shouldn't appear anywhere else in
the kernel tree.  Putting it with the driver also has the added
advantage that it can be lumped in with the driver module and
therefore will only get compiled into the kernel if the driver is
present.  Putting driver specific (not platform specific) translation
code anywhere other than with the device driver it is intended for is
just wrong.

In addition, I'd really like to avoid a situation where the same block
of translation code (or at least calls to it) is duplicated all over
the platform code directories.  It's that sort of duplication ...
From: David Brownell
Date: Wednesday, May 21, 2008 - 6:16 pm

Platform_data isn't what I'd call independent of drivers.

The reason the data is there in the first place is that

... because platform data was designed as a partial template
for that driver, letting it do that.  (Sometimes without even
doing scale conversions.)  As drivers grow functionally, they
sometimes end up needing more platform data fields, to expose
data that previously didn't matter.

Whether that data can usefully be stored in flash (or ROM)
and handed out through the bootloader is something of a
manufacturing issue.

- Dave
--

From: Grant Likely
Date: Friday, May 23, 2008 - 11:24 pm

Oh, of course the driver needs it!  I'm not claiming otherwise.  More
what I mean is that the driver doesn't need to be loaded or even

I do not dispute any of that.  My point, however, is that pdata is
typically used simply as a representation that is convenient for
platform code to pass that data into the driver and that often drivers
don't use that representation directly.  Instead, the data is
explicitly copied explicitly field by field into the driver at probe
time and is not touched again.  That says to me that driver developers
view pdata as somewhat decoupled from the internal workings of the
driver and in the case of many powerpc devices a different
representation is more convenient; namely a device tree node.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Anton Vorontsov
Date: Wednesday, May 21, 2008 - 8:19 am

[...]

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
--

From: David Brownell
Date: Thursday, May 22, 2008 - 7:05 pm

Yes please.

--

From: Grant Likely
Date: Friday, May 23, 2008 - 11:26 pm

Okay, I wasn't sure.  Will do.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Grant Likely
Date: Saturday, May 24, 2008 - 9:50 am

On Sat, May 24, 2008 at 12:26 AM, Grant Likely

I'm having second thoughts about this.  I think this code is more SPI
centric than it is OF centric.  ie. it is usable by all spi masters in
an OF enabled system, but it is not usable by all OF devices in an SPI
enabled system.  Or, in other words; it adds OF support to SPI, not
the other way around.  I think drivers/spi is the right place for this
to live.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Jochen Friedrich
Date: Saturday, May 24, 2008 - 10:14 am

Isn't the same true for drivers/of/gpio.c or drivers/of/of_i2c.c, as well?

Thanks,
Jochen
--

From: Grant Likely
Date: Saturday, May 24, 2008 - 10:33 am

I would argue 'yes!'

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: David Brownell
Date: Saturday, May 24, 2008 - 10:45 am

... all the more reason to have the SPI glue go there too,
matching the ACPI/PCI precedent as well as those others!

--

From: Grant Likely
Date: Saturday, May 24, 2008 - 9:56 pm

Alright; I give!  I'll put it in drivers/of.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: David Brownell
Date: Saturday, May 24, 2008 - 10:43 am

It's not usable by *any* SPI master on a non-OF system though.

I'd still rather see such translations in the OF-specific part of
the source tree.  Like drivers/acpi/pci_*.c code, this has more to
do with the firmware interface than with bus (SPI) interface.

Arguments could be made both ways here, but for the moment it makes
more sense to me to keep this type of platform glue (be it OF, ACPI,
arch-specific setup code, or whatever) together in the source tree
and apart from the bus-specific code.

Where do the proposed patches gluing OF to I2C live, or has that
been settled yet?

- Dave


--

From: Guennadi Liakhovetski
Date: Monday, May 19, 2008 - 6:17 am

Yes, this looks like a problem to me. This means, SPI devices will need 
two bindings - OF and platform?... Maybe define an spi_chipselect 
OF-binding?

Thanks
Guennadi
---
Guennadi Liakhovetski
--

From: Grant Likely
Date: Monday, May 19, 2008 - 8:57 am

On Mon, May 19, 2008 at 7:17 AM, Guennadi Liakhovetski

Actually, spi devices have *neither*.  :-)  They bind to the SPI bus.
Not the platform bus or of_platform bus.  But that is Linux internal
details; this discussion is about device tree bindings.

Note that I did say that drivers can define additional properties for
supporting chip select changes as needed.  I'm just not attempting to
encode them into the formal binding.  There is simply just too many
different ways to manipulate chip select signals and so I don't feel
confident trying to define a *common* binding at this moment in time.
At some point in the future when we have a number of examples to
choose from then we can extend this binding with chip select related
properties.

As for the Linux internals, the 5200 SPI bus driver that I posted
exports a function that allows another driver to call in and
manipulated the CS lines before the transfer.  It isn't the prettiest
solution, but I'm not locked into the approach and that gives some
time to consider cleaner interfaces.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Gary Jennejohn
Date: Monday, May 19, 2008 - 10:09 am

On Mon, 19 May 2008 09:57:21 -0600

I sort of hesitate to hijack this thread, but since we're discussing SPI
and chip selects...

I have a driver for the SPI controller in the 440EPx.  This controller
is very simple and does not have any internal support for a chip select.
The controller seems to also be in the 440GR and 440EP, and may be in
other AMCC CPUs too.

All chip selects must be done using GPIO.  In fact, the board for which
I developed this driver, a modified sequoia, actually uses 2 chip selects.

My problem was, and is, that there's no generic GPIO support for powerpc.
At least, not that I'm aware of.  Please tell me if I'm wrong.

So the driver has great gobs of GPIO code in it, most of which I took
from u-boot.  The code is pretty generic, but some 440EPx-specific
stuff may have crept in without my being aware of it.

My real question is - should this code be in a platform-specific file
such as sequoia.c, which could result in lots of duplicated code, or is
it better to leave it in the driver for now until some day we hopefully
get generic GPIO support for powerpc?

I want to get this driver upstream ASAP.

---
Gary Jennejohn
*********************************************************************
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
*********************************************************************
--

From: Anton Vorontsov
Date: Monday, May 19, 2008 - 10:19 am

Documentation/powerpc/booting-without-of.txt
VIII - Specifying GPIO information for devices.

And include/linux/of_gpio.h + drivers/of/gpio.c.

Soon I'll post some patches for mpc83xx_spi showing how to use GPIOs
for the SPI chip selects.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
--

From: Guennadi Liakhovetski
Date: Monday, May 19, 2008 - 9:30 am

Right, sorry, your SPI bus driver scans the bus device bindings and 

Yes, I understand, that physically there can be many ways SPI chipselects 
can be controlled. But I thought there could be a generic way to cover 
them all by defining a separate entry on your SPI bus. Like

+    SPI example for an MPC5200 SPI bus:
+		spi@f00 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			compatible = "fsl,mpc5200b-spi","fsl,mpc5200-spi";
+			reg = <0xf00 0x20>;
+			interrupts = <2 13 0 2 14 0>;
+			interrupt-parent = <&mpc5200_pic>;
+			eth-switch-cs@0 {
+				compatible = "oem,cs-type";
+			};
+
+			ethernet-switch@0 {
+				compatible = "micrel,ks8995m";
+				linux,modalias = "ks8995";
+				max-speed = <1000000>;
+				reg = <0>;
+				cs-parent = <&/.../spi@f00/eth-switch-cs@0>;
+			};
...
+		};

Then whatever method is used to actually switch the CS, a driver should be 
registered to handle eth-switch-cs@0, providing the required calls. 
Without such a driver ethernet-switch@0 will not probe successfully. 
Wouldn't this cover all possible cases? One could even consider actually 
putting SPI devices on SPI chipselect busses, but that won't look very 
elegant:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--

From: Grant Likely
Date: Monday, May 19, 2008 - 10:13 pm

On Mon, May 19, 2008 at 10:30 AM, Guennadi Liakhovetski

Hurrmmmm...

I'm not so fond of this approach.  cs-parent doesn't seem to make much
sense to me.  It might be better to have a cs-handler property on the
SPI bus node instead of on the SPI slave nodes, but even then it
leaves a number of questions about what it really means.  In some
cases it would be overkill.  For example, if the SPI node simply had
multiple GPIO lines then an extra cs-parent node wouldn't be needed at
all.  Then there are the complex arrangements.  When setting CS
requires inserting a special 'set cs' SPI message at the right time.
Or worse; when setting CS requires /modifying/ the sent SPI message.
Essentially, the binding would need to describe the ability to
completely intercept and rewrite all SPI messages going through the CS
scheme.

I'm not saying it's not possible to do, but I am saying that I'd like
to have a better feel for all the use cases before it is defined.  I'm
not convinced that adding a cs-parent phandle will do that
appropriately.  That being said, my gut feel is that the solution will
be to support spi-bridge nodes that handle the complex CS
configuration settings; the spi-bridge would be a child of the
spi-master and the parent of the spi devices; and simple CS settings
being handled with regular old GPIO bindings.  (Much like the last
suggestion you make; except that I think that it *does* looks
elegant.)  :-)

example; here's an SPI bus that has 2 GPIOs for two bus CS lines and
an SPI bridge that uses both CSes; one address for accessing the
bridge's CS register and one CS to access the downstream devices.

+    SPI example for an MPC5200 SPI bus:
+               spi@f00 {
+                   #address-cells = <1>;
+                   #size-cells = <0>;
+                   compatible = "fsl,mpc5200b-spi","fsl,mpc5200-spi";
+                   reg = <0xf00 0x20>;
+                   interrupts = <2 13 0 2 14 0>;
+                   interrupt-parent = <&mpc5200_pic>;
+      ...
From: Guennadi Liakhovetski
Date: Tuesday, May 20, 2008 - 8:26 am

Hm, are there actually such SPI _controllers_ that use SPI data to toggle 
chipselects? I.e., you would have to send your SPI client data (for the 
RTC or whatever) plus some extra bytes or with some modifications, and 
this extra information would then be intercepted by the SPI _controller_ 
itself and only client data would be sent out? Isn't what you're 
describing really a case of an SPI bridge, as you also call it below? In 
which case, I think, it might make sense to cleanly differentiate these 
two cases:

1. SPI chipselect. Either controlled by an external (typically a GPIO) 
signal or by the controller itself, in the latter case the controller has 
to be configured with the required address

2. SPI bridge. I don't know such configurations, so, I can only guess: the 
SPI controller has a single SPI client, which acts like a bridge. It 
receives data from the primary host, and in this data the target client 
data and its address are encoded.

Now, I can also imagine case 2 where the bridge is actually a part of the 

Ok, elegance apart:-) You can use the SPI-bridge construct to also 
describe simple SPI-chipselect configurations. But is it really a good 
idea? Wouldn't it be better to handle these two cases separately? Using 
"bridge" to describe CS's seems also confusing - imagine someone 
implementing a new DTS, having to describe a bridge not having one doesn't 

Well, in this case - yes, because addressing clients "behind" the expander 
and the expander itself is done differently.

On the whole, I think, it begins to look good - I think, it is better to 
implement an imperfect but complete and consistent scheme and modify or 
extend it in the future, than a perfect, but incomplete, and have to use 
auxiliary means (platform bindings) to fill in the gaps.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--

From: Segher Boessenkool
Date: Wednesday, May 21, 2008 - 12:11 pm

It would be best to handle all these things that are specific to
a certain SPI controller (like how CSs work) in the binding for
that SPI controller, and not try to shoehorn all of this into some
artificial generic framework.

If you can have identical addresses on the SPI bus going to different
devices based on which CS is asserted, you'll have to make the CS part
of the "reg".  Example:

spi-controller {
	#address-cells = 2;
	#size-cells = 0;
	some-device@0,f000 { reg = < 0 f000 >; } // CS 0, SPI address f000
	some-device@1,f000 { reg = < 1 f000 >; } // CS 1, SPI address f000
	some-device@1,ff00 { reg = < 1 ff00 >; } // CS 1, SPI address ff00
}

SPI-to-SPI bridges can (and should!) be handled the same way as
anything-to-anything-else bridges are handled as well: either there
is a nice simple one-to-one matching (and you can use "ranges") or
you need a driver for that bridge that knows how to make it work (or
both, "ranges" isn't always enough, the bridge might require some
specific handling for some special situations -- error handling,
suspend, whatever).


Segher

--

From: Grant Likely
Date: Wednesday, May 21, 2008 - 12:33 pm

On Wed, May 21, 2008 at 1:11 PM, Segher Boessenkool

For SPI the CS # *is* the address.  :-)

Unlike I2C, SPI doesn't impose any protocol on the data.  It is all
anonymous data out, anonymous data in, a clock and a chip select.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: David Brownell
Date: Thursday, May 22, 2008 - 7:26 pm

Very true ... but then there are SPI chips which embed addressing.

I have in mind the mcp23s08 (and mcp23s17) GPIO expanders, which
support up to four chips wired in parallel on a given chipselect.
The devices are distinguished by how two address pins are wired;
and two bits in the command byte must match them.  (I think they
just recycled an I2C design into the SPI world.)

- Dave
--

From: Grant Likely
Date: Friday, May 23, 2008 - 11:25 pm

Very good point.  Okay, so we cannot assume any correlation between
the number of CS lines and the number of child nodes to the SPI bus.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: David Brownell
Date: Saturday, May 24, 2008 - 12:13 am

That wasn't what I was implying ... all the devices hooked
up to a given chipselect should be viewed as a single (albeit
composite) child node.

Now, the driver for that child node may want to expose lots
of substructure.  But that's no different from any other
complex device, whose protocol happens to embed some notion
of component addressing.

It's just that in the case I mentioned, that addressing is
a bit more externally visible than it is in some other cases.

- Dave

--

From: Grant Likely
Date: Tuesday, May 20, 2008 - 8:48 am

On Tue, May 20, 2008 at 9:26 AM, Guennadi Liakhovetski

Yes!  There really are!!!  One case I've been told of is an SPI bridge


Hmmm, yeah, I suppose it is possible; but if it is internal to the bus
controller then it can also be handled internally by the bus
controller driver and probably won't need to be reflected in the

It also needs to describe layouts which require SPI transfers in a
particular order.  For example, if you're doing 2 SPI messages (M1 and
M2) to 2 different SPI devices (S1 and S2), and the CS lines are on a
GPIO expander which is a third SPI device (S3).  In which case 5 or 6
SPI messages need to be transmitted:

ctrl msg -> S3    // To set the CS to S1
M1 -> S1
ctrl msg -> S3    // To clear the CS to S1
ctrl msg -> S3    // To set the CS to S2
M2 -> S2
ctrl msg -> S3    // To clear the CS to S2

An important subtlety to note here is that the GPIO device (S3) cannot
simply enqueue the control message to the SPI device when it is time
to send M1 or M2.  Normal enqueuing would add the messages to the end
of the queue; too late to actually activate the relevant CS line.
Granted, the is mostly a driver issue; not a device tree issue; but it
has some impact on the layout.  It could be handled with the
spi_bridge construct, but S1 and S2 aren't really children of S3.  On
the other hand; the spi_bridge is really more of a board level
construct.  The spi_bridge could be considered the board wireup and
S1, S2 and S3 are all children of the spi_bridge.  The spi_bridge
would have to be knowledgeable enough to handle control messages to S3

... and it must be said that I got rather lazy in my example below.  I
really covered both layouts (simple GPIO and an SPI bridge in the same
example without documenting it sufficiently.  I'll hash out my
thoughts some more and post a series of better examples this

I'm not too opposed to using non-standard properties to describe stuff
in the short term; but I agree that avoiding hacky platform data stuff
is not ...
From: Grant Likely
Date: Friday, May 16, 2008 - 12:36 pm

From: Grant Likely <grant.likely@secretlab.ca>

Unlike the old CSB driver, this driver uses the SPI infrastructure.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/spi/Kconfig             |    8 +
 drivers/spi/Makefile            |    1 
 drivers/spi/mpc52xx_spi.c       |  561 +++++++++++++++++++++++++++++++++++++++
 include/linux/spi/mpc52xx_spi.h |   10 +
 4 files changed, 580 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 12c35da..bd07429 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -120,6 +120,14 @@ config SPI_LM70_LLP
 	  which interfaces to an LM70 temperature sensor using
 	  a parallel port.
 
+config SPI_MPC52xx
+	tristate "Freescale MPC52xx SPI (non-PSC) controller support"
+	depends on PPC_MPC52xx && SPI
+	select SPI_MASTER_OF
+	help
+	  This drivers supports the MPC52xx SPI controller in master SPI
+	  mode.
+
 config SPI_MPC52xx_PSC
 	tristate "Freescale MPC52xx PSC SPI controller"
 	depends on SPI_MASTER && PPC_MPC52xx && EXPERIMENTAL
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 29c592f..805bef1 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_SPI_PXA2XX)		+= pxa2xx_spi.o
 obj-$(CONFIG_SPI_OMAP_UWIRE)		+= omap_uwire.o
 obj-$(CONFIG_SPI_OMAP24XX)		+= omap2_mcspi.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_spi.o
+obj-$(CONFIG_SPI_MPC52xx)		+= mpc52xx_spi.o
 obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
 obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
new file mode 100644
index 0000000..39dc6a2
--- /dev/null
+++ b/drivers/spi/mpc52xx_spi.c
@@ -0,0 +1,561 @@
+/*
+ * MPC52xx SPI master driver.
+ * Copyright (C) 2008 Secret Lab Technologies Ltd.
+ *
+ * This is the driver for the MPC5200's dedicated SPI device (not for a
+ * PSC in SPI mode)
+ */
+
+#include ...
From: Grant Likely
Date: Friday, May 16, 2008 - 12:42 pm

er, this comment is *obviously* wrong.  I'll fix it in the next version.

Cheers,
g.


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Jon Smirl
Date: Friday, May 16, 2008 - 1:27 pm

Right now we have SPI hooked up to PSC3. Hardware engineer is gone but
I'll see if I can get him to alter things to use the SPI controller. I
have an old mail from him where he thinks the Phytec board is missing
a signal needed to use the SPI controller.

Is the current SPI driver working on PSC3? I have a MMC card wired up
to it but I've never tried using it.

I have the MPC5200 PSC SPI driver enabled and "MMC/SD over SPI"
enabled in my kernel. The MMC bus gets created but there aren't any
devices on it. Do we need something in the SPI driver so that the MMC
layer can find it?

Are you going to keeps this as two drivers or merge them? If it is two
drivers there should be one entry in Kconfig and two sub choices for


-- 
Jon Smirl
jonsmirl@gmail.com
--

From: Grant Likely
Date: Friday, May 16, 2008 - 1:48 pm

While I'd appreciate the testing, I suspect that you really don't want
to do that.  The dedicated SPI controller isn't very good.  It only
does a byte at a time and so is rather slow.  A PSC is SPI mode should


Yes, see patch 3 in my series.  the PSC SPI driver needs to do
something like this.  SPI busses don't really do autodetection (but

It will remain as two drivers.  The devices are entirely separate.  I
don't think putting them under a single Kconfig is needed or a good
idea.  They both depend on PPC_MPC52xx anyway so they only show up if
you're building for a 5200 platform.

Thanks for the comments.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Jon Smirl
Date: Friday, May 16, 2008 - 2:25 pm

What is the device tree node for PSC3 supposed to look like when it
has both serial and spi enabled?

-- 
Jon Smirl
jonsmirl@gmail.com
--

From: Grant Likely
Date: Friday, May 16, 2008 - 2:32 pm

The *PSC3 device* cannot support both serial and SPI at the same time.
 Only one mode works at a time...

However, *PSC3 pin group* has can be configured to route both the
*PSC3 device* and the *SPI device* signal out to the board at the same
time.

Pin routing is not something that is described by the device tree.
It's viewed as a board level initialization thing, similar to how DDR
RAM initialization is viewed.  Ideally, the bootloader will write the
correct value into port_config for pin routing and Linux will never
need to touch it.  If the bootloader cannot be changed, then
board-specific platform code can be added to fixup the port_config
setting.  However, the drivers should never touch or care about pin
routing.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--

From: Jon Smirl
Date: Friday, May 16, 2008 - 2:42 pm

I need to talk to my hardware guy.  He is using PSC3 for the boot
console with the assumption that once booted it is ok to retask it to
SPI. Serial console is only needed for software debugging.  SSH works
after boot and can replace the serial console.

I'll trying changing my device tree entry from UART to SPI and boot.


-- 
Jon Smirl
jonsmirl@gmail.com
--

Previous thread: none

Next thread: tip build broken on sparc64... by David Miller on Friday, May 16, 2008 - 1:07 pm. (4 messages)