SPI OF bindings, MMC-over-SPI, chip-selects and so on

This patch converts the driver to speak OF directly. FSL SPI controllers do not use internal chip-select machines, so boards must use GPIOs for these purposes.

Signed-off-by Anton Vorontsov <[email protected]>

---
 Documentation/powerpc/booting-without-of.txt |    1 +
 drivers/spi/Kconfig                          |    3 +-
 drivers/spi/spi_mpc83xx.c                    |  279 +++++++++++++++++---------
 3 files changed, 184 insertions(+), 99 deletions(-)

drivers/spi/spi_mpc83xx.c +0 -1

@@ -536,92 +539,152 @@ static void mpc83xx_spi_cleanup(struct spi_device *spi)
       kfree(spi->controller_state);
 }
 
-static int __init mpc83xx_spi_probe(struct platform_device *dev)

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+       master->num_chipselect = of_num_children(np);

This assumes that there are no gaps in the assigned CS numbers of child nodes, or that the child nodes are an exhaustive list of attached devices, neither of which may be true. num_chipselect should be calculated from the number of GPIOs specified instead.

On Wed, May 21, 2008 at 10:50:02AM -0600, Grant Likely wrote:
[..]

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+       master->num_chipselect = of_num_children(np);

This assumes that there are no gaps in the assigned CS numbers of child nodes, or that the child nodes are an exhaustive list of attached devices, neither of which may be true. num_chipselect should be calculated from the number of GPIOs specified instead.

[I'm not arguing just a thought.]

- every SPI device must have its own chip-select, otherwise SPI device node should not be a part of a SPI controller node;

- or there is just once device on the SPI bus with chip-select always asserted, no gpios = <> is specified (this case is implemented);
- or the SPI is bridged, gpios = <> should list behind-the-bridge devices' chip-selects, and driver should understand that there is a "special" (bridge) device somewhere on the bus (not implemented).

Dedicated (usually the ones that need to fill platform data) constructors will create board info, so SPI core will probe them as normal SPI devices.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/spi/spi_of.c       |   67 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi_of.h |    5 +++
 2 files changed, 72 insertions(+), 0 deletions(-)

drivers/spi/spi_of.c +0 -0

index b5ae434..2e1a11f 100644
@@ -11,6 +11,66 @@
 #include <linux/spi/spi.h>

Quoted 17 lines · Anton Vorontsov

Dedicated (usually the ones that need to fill platform data) constructors will create board info, so SPI core will probe them as normal SPI devices.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/spi/spi_of.c       |   67 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi_of.h |    5 +++
 2 files changed, 72 insertions(+), 0 deletions(-)

drivers/spi/spi_of.c +0 -0

index b5ae434..2e1a11f 100644
@@ -11,6 +11,66 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_of.h>
 

drivers/spi/spi_of.c line 21

Quoted 8 lines

drivers/spi/spi_of.c +8 -0

+/*
+ * Caller have no idea who is master, i.e. this function does not
+ * accept pointer to the master, instead we use board infos.
+ */
+int of_spi_device_probe_common(struct device_node *np,
+			       struct spi_board_info *spi_binfo,
+			       const char *modalias)
+{

Hm, I might well misunderstand something here, but it looks to me like you are again trying to use both OF _and_ platform (spi_board_info) bindings for your SPI setup?

And this is exactly what we are trying to avoid in Grant's series of patches...

Quoted 4 lines · Guennadi Liakhovetski

Hm, I might well misunderstand something here, but it looks to me like you are again trying to use both OF _and_ platform (spi_board_info) bindings for your SPI setup?

Yes, you didn't misunderstand. ;-)

Quoted 2 lines

And this is exactly what we are trying to avoid in Grant's series of patches...

I didn't find other way... The show stopper is "master" argument, drivers don't know about masters (and should not, since if they should, then this implies that masters should be registered prior to devices, and that complicates everything).

What is the problem with board infos, btw? I missed that part. Board

Quoted 16 lines · Anton Vorontsov

Quoted 4 lines · Guennadi Liakhovetski

Hm, I might well misunderstand something here, but it looks to me like you are again trying to use both OF _and_ platform (spi_board_info) bindings for your SPI setup?

Yes, you didn't misunderstand. ;-)

Quoted 2 lines

And this is exactly what we are trying to avoid in Grant's series of patches...

I didn't find other way... The show stopper is "master" argument, drivers don't know about masters (and should not, since if they should, then this implies that masters should be registered prior to devices, and that complicates everything).

Quoted 1 line

What is the problem with board infos, btw? I missed that part. Board

In short: board infos are not bad as such. I find it bad if you have to use both OF and platform bindings to describe _one_ piece of hardware.

Quoted 18 lines · Guennadi Liakhovetski

Quoted 16 lines · Anton Vorontsov

Quoted 4 lines · Guennadi Liakhovetski

Hm, I might well misunderstand something here, but it looks to me like you are again trying to use both OF _and_ platform (spi_board_info) bindings for your SPI setup?

Yes, you didn't misunderstand. ;-)

Quoted 2 lines

And this is exactly what we are trying to avoid in Grant's series of patches...

I didn't find other way... The show stopper is "master" argument, drivers don't know about masters (and should not, since if they should, then this implies that masters should be registered prior to devices, and that complicates everything).

Quoted 4 lines

Quoted 1 line

What is the problem with board infos, btw? I missed that part. Board

In short: board infos are not bad as such. I find it bad if you have to use both OF and platform bindings to describe _one_ piece of hardware.

This particular discussion isn't about describing hardware (since we're describing it via device tree), but about implementation details, such as:

1. Passing platform_data to the drivers;
2. Creating "SPI Linux devices" from the OF description.

I see there ways:

1. Grant Likely's approach (works great for simple drivers which don't need SPI platform_data).
2. Old board infos approach, there we can do whatever we want.
3. Implementing OF bindings for the every SPI driver that needs platform_data.

I concede that sometimes platform code just has to pass data to the driver that cannot be described in the device tree. callback pointers being the most significant example and we do need a sane way to do so.

Quoted 3 lines · Grant Likely

I concede that sometimes platform code just has to pass data to the driver that cannot be described in the device tree. callback pointers being the most significant example and we do need a sane way to do so.

Sorry, I just cannot understand. So far my understanding was, that OF bindings were there to _completely_ describe hardware, that cannot be autoconfigured or probed.

So a _generic_ driver gets all information it
needs from the device tree and needs no additional information to handle the hardware.

Now if a driver needs additional information from "platform code" what's the purpose of partial data in the device tree at all? Just use board information as usual.

What good is an OF-binding if it doesn't provide complete information?

If some behaviour is entirely board specific and doesn't have any possibility of being duplicated on other boards, then it makes sense for the code implementing that behaviour to live with the platform code.

Quoted 1 line

What good is an OF-binding if it doesn't provide complete information?

It still increases the amount of common code even if there are rare corner cases that require board specific code.

Quoted 4 lines · Grant Likely

If some behaviour is entirely board specific and doesn't have any possibility of being duplicated on other boards, then it makes sense for the code implementing that behaviour to live with the platform code.

Exactly - "entirely board specific" and "doesn't have any possibility of being duplicated on other boards."

Quoted 2 lines

Quoted 1 line

What good is an OF-binding if it doesn't provide complete information?

Quoted 2 lines

It still increases the amount of common code even if there are rare corner cases that require board specific code.

Right again - _rare_ corner cases. Whereas we are talking about _all_ SPI busses, maybe apart from those, where the controller itself switches CSs in a well-defined way, and the driver doesn't need any additional information to handle this.

Quoted 5 lines · Guennadi Liakhovetski

Right again - _rare_ corner cases. Whereas we are talking about _all_ SPI busses, maybe apart from those, where the controller itself switches CSs in a well-defined way, and the driver doesn't need any additional information to handle this.

Ah, I see where we are crossing our wires. I was talking about the case of registering spi devices. I agree that the spi bus should not need any additional information.

Quoted 8 lines · Grant Likely

Quoted 5 lines · Guennadi Liakhovetski

Right again - _rare_ corner cases. Whereas we are talking about _all_ SPI busses, maybe apart from those, where the controller itself switches CSs in a well-defined way, and the driver doesn't need any additional information to handle this.

Quoted 3 lines

Ah, I see where we are crossing our wires. I was talking about the case of registering spi devices. I agree that the spi bus should not need any additional information.

No, sorry for not making it clear. I wrote "busses" because on those controllers, that control CS themselves _devices_ don't need any additional info. But I meant, that describing SPI _devices_ should be done in only one way or another - either using fdt, or platform data, not both.

Dedicated (usually the ones that need to fill platform data) constructors will create board info, so SPI core will probe them as normal SPI devices.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/spi/spi_of.c       |   67 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/spi_of.h |    5 +++
 2 files changed, 72 insertions(+), 0 deletions(-)

drivers/spi/spi_of.c +43 -0

index b5ae434..2e1a11f 100644
@@ -11,6 +11,66 @@
 #include <linux/spi/spi.h>
 #include <linux/spi/spi_of.h>
 
+/*
+ * Caller have no idea who is master, i.e. this function does not
+ * accept pointer to the master, instead we use board infos.
+ */
+int of_spi_device_probe_common(struct device_node *np,
+                              struct spi_board_info *spi_binfo,
+                              const char *modalias)
+{
+       struct device_node *parent;
+       const u32 *bus_num;
+       const u32 *chip_select;
+       const u32 *max_speed;
+       int size;
+
+       parent = of_get_parent(np);
+       if (!parent) {
+               pr_err("%s: no parent\n", np->full_name);
+               return -EINVAL;
+       }
+
+       bus_num = of_get_property(parent, "reg", &size);
+       if (!bus_num || size < sizeof(*bus_num)) {
+               pr_err("%s: no reg specified for parent\n", np->full_name);
+               of_node_put(parent);
+               return -EINVAL;
+       }
+       spi_binfo->bus_num = *bus_num;
+       of_node_put(parent);
+
+       chip_select = of_get_property(np, "reg", &size);
+       if (!chip_select || size < sizeof(*chip_select)) {
+               pr_err("%s: no reg (chip-select) specified\n", np->full_name);
+               return -EINVAL;
+       }
+       spi_binfo->chip_select = *chip_select;
+
+       max_speed = of_get_property(np, "max-speed", &size);
+       if (!max_speed || size < sizeof(*max_speed))
+               spi_binfo->max_speed_hz = 100000;
+       else
+               spi_binfo->max_speed_hz = *max_speed;
+
+       strcpy(spi_binfo->modalias, modalias);

This is just a bait for further discussion of OF/SPI, chip-selects, e.t.c.

I've converted the spi_mpc83xx to the OF driver (using Grant's SPI_MASTER_OF work + some additions), and implemented MMC-over-SPI bindings. This stuff extensively using GPIOs, and I think this will work for the "bridged SPI" too, since the SPI bridge could be represented as GPIO controller (inside the SPI controller node).

Quoted 3 lines · Anton Vorontsov

This is just a bait for further discussion of OF/SPI, chip-selects, e.t.c.

Quoted 5 lines

I've converted the spi_mpc83xx to the OF driver (using Grant's SPI_MASTER_OF work + some additions), and implemented MMC-over-SPI bindings. This stuff extensively using GPIOs, and I think this will work for the "bridged SPI" too, since the SPI bridge could be represented as GPIO controller (inside the SPI controller node).

Yes, but do you really _want_ to represent them as GPIOs? Firstly they are not really "G" P, secondly, they are not even "I", rather only "O".

And do
you really want to register your SPI CS lines to the whole system for everyone's (ab)use?...

Hi all,

This is just a bait for further discussion of OF/SPI, chip-selects, e.t.c.

I've converted the spi_mpc83xx to the OF driver (using Grant's SPI_MASTER_OF work + some additions), and implemented MMC-over-SPI bindings. This stuff extensively using GPIOs, and I think this will work for the "bridged SPI" too, since the SPI bridge could be represented as GPIO controller (inside the SPI controller node).

Hi all,

This is just a bait for further discussion of OF/SPI, chip-selects, e.t.c.