[PATCH v3 0/2] SPI support for fsl_soc and mpc832x_rdb

Hi all,

This is v3. The only objection I can imagine is about "fsl,device-id".
Though in the v2 nobody complained, thus it's stayed intact.

If you want to, complain now. I'll give up and will remove it. ;-)

Changelog:

v2 -> v3
o Device tree:
  - completely removed mmc node;
  - removed pio-handles and pio-maps.

o board file:
  - Instead of par_io_of_config(), now par_io_config_pin() used to
    configure GPIO pins, which does not require device tree node.

v1 -> v2
o Device tree:
  - cosmetic cleanups (@01 -> @1);
  - device-id renamed to fsl,device-id;
  - removed max-chipselect and sysclk properties from spi node;
  - removed chipselect property from mmc node, now reg property
    used for this purpose, thereby address-cells and size-cells
    added to the spi node;
  - other non-mandatory (device-id, device_type, compatible, ...)
    properties removed from mmc node, today board file is using
    of_find_node_by_name(), instead of of_find_compatible_node();
  - "qe" mode renamed to "cpu-qe".

o board file <-> fsl_soc interaction
  - fsl_soc no longer scans for SPI nodes in the arch initcall.
    Also it's no longer exports any global variables. Instead, it's
    export fsl_spi_init function now, which accepts pointer to the
    fsl_spi_board_info structure;
  - board file fills fsl_spi_board_info structure and issues
    fsl_spi_init(), which register SPI devices and SPI board infos.
    Various sanity checks also perfromed.

    I'd want to note that if spi_mpc83xx will be converted to
    of_platform_driver then the scheme described above will not
    work anymore, and I'll have to revert back ugly hacks:
    global variables for activate/deactivate_cs functions. I see
    no other options.

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

mmc_spi already tested to work. When it will hit mainline
the only change that will be needed is replacing "spidev"
by "mmc_spi".

Signed-off-by: Anton Vorontsov <redacted>
---
 arch/powerpc/boot/dts/mpc832x_rdb.dts     |    4 ++-
 arch/powerpc/platforms/83xx/mpc832x_rdb.c |   52 +++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc832x_rdb.dts b/arch/powerpc/boot/dts/mpc832x_rdb.dts
index e9c332f..664dd16 100644
--- a/arch/powerpc/boot/dts/mpc832x_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc832x_rdb.dts

@@ -207,15 +207,17 @@
 
 		spi@4c0 {
 			device_type = "spi";
+			fsl,device-id = <1>;
 			compatible = "fsl_spi";
 			reg = <4c0 40>;
 			interrupts = <2>;
 			interrupt-parent = <&qeic>;
-			mode = "cpu";
+			mode = "cpu-qe";
 		};
 
 		spi@500 {
 			device_type = "spi";
+			fsl,device-id = <2>;
 			compatible = "fsl_spi";
 			reg = <500 40>;
 			interrupts = <1>;

diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
index 93ba957..89cc039 100644
--- a/arch/powerpc/platforms/83xx/mpc832x_rdb.c
+++ b/arch/powerpc/platforms/83xx/mpc832x_rdb.c

@@ -15,6 +15,7 @@
  */
 
 #include <linux/pci.h>
+#include <linux/spi/spi.h>
 
 #include <asm/of_platform.h>
 #include <asm/time.h>

@@ -24,6 +25,7 @@
 #include <asm/qe_ic.h>
 
 #include "mpc83xx.h"
+#include "../../sysdev/fsl_soc.h"
 
 #undef DEBUG
 #ifdef DEBUG

@@ -32,6 +34,56 @@
 #define DBG(fmt...)
 #endif
 
+extern int par_io_data_set(u8 port, u8 pin, u8 val);
+extern int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
+			     int assignment, int has_irq);
+
+static void mpc83xx_spi_activate_cs(u8 cs, u8 polarity)
+{
+	pr_debug("%s %d %d\n", __func__, cs, polarity);
+	par_io_data_set(3, 13, polarity);
+}
+
+static void mpc83xx_spi_deactivate_cs(u8 cs, u8 polarity)
+{
+	pr_debug("%s %d %d\n", __func__, cs, polarity);
+	par_io_data_set(3, 13, !polarity);
+}
+
+static struct spi_board_info mpc832x_spi_boardinfo = {
+	.bus_num = 1,
+	.chip_select = 0,
+	.max_speed_hz = 50000000,
+	/*
+	 * XXX: This is spidev (spi in userspace) stub, should
+	 * be replaced by "mmc_spi" when mmc_spi will hit mainline.
+	 */
+	.modalias = "spidev",
+};
+
+static struct fsl_spi_board_info fsl_spi_board_info = {
+	.activate_cs = mpc83xx_spi_activate_cs,
+	.deactivate_cs = mpc83xx_spi_deactivate_cs,
+	.board_infos = &mpc832x_spi_boardinfo,
+	.num_board_infos = 1,
+};
+
+static int __init mpc832x_spi_init(void)
+{
+	par_io_config_pin(3,  0, 3, 0, 1, 0); /* SPI1 MOSI, I/O */
+	par_io_config_pin(3,  1, 3, 0, 1, 0); /* SPI1 MISO, I/O */
+	par_io_config_pin(3,  2, 3, 0, 1, 0); /* SPI1 CLK,  I/O */
+	par_io_config_pin(3,  3, 2, 0, 1, 0); /* SPI1 SEL,  I   */
+
+	par_io_config_pin(3, 13, 1, 0, 0, 0); /* !SD_CS,    O */
+	par_io_config_pin(3, 14, 2, 0, 0, 0); /* SD_INSERT, I */
+	par_io_config_pin(3, 15, 2, 0, 0, 0); /* SD_PROTECT,I */
+
+	return fsl_spi_init(&fsl_spi_board_info);
+}
+
+device_initcall(mpc832x_spi_init);
+
 /* ************************************************************************
  *
  * Setup the architecture

-- 
1.5.0.6

Signed-off-by: Anton Vorontsov <redacted>
---
 arch/powerpc/sysdev/fsl_soc.c |   88 +++++++++++++++++++++++++++++++++++++++++
 arch/powerpc/sysdev/fsl_soc.h |   12 ++++++
 2 files changed, 100 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 727453d..0771700 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c

@@ -23,6 +23,7 @@
 #include <linux/device.h>
 #include <linux/platform_device.h>
 #include <linux/phy.h>
+#include <linux/spi/spi.h>
 #include <linux/fsl_devices.h>
 #include <linux/fs_enet_pd.h>
 #include <linux/fs_uart_pd.h>

@@ -1186,3 +1187,90 @@ err:
 arch_initcall(cpm_smc_uart_of_init);
 
 #endif /* CONFIG_8xx */
+
+int fsl_spi_init(struct fsl_spi_board_info *binfo)
+{
+	struct device_node *np;
+	unsigned int i;
+	u32 sysclk;
+
+	np = of_find_node_by_type(NULL, "qe");
+	if (!np)
+		return -ENODEV;
+
+	sysclk = *(u32 *)of_get_property(np, "bus-frequency", NULL);
+
+	for (np = NULL, i = 1;
+	     (np = of_find_compatible_node(np, "spi", "fsl_spi")) != NULL;
+	     i++) {
+		int ret = 0;
+		unsigned int j;
+		const char *devid;
+		const char *mode;
+		struct resource res[2];
+		struct platform_device *pdev = NULL;
+		struct fsl_spi_platform_data pdata = {
+			.activate_cs = binfo->activate_cs,
+			.deactivate_cs = binfo->deactivate_cs,
+		};
+
+		memset(res, 0, sizeof(res));
+
+		devid = of_get_property(np, "fsl,device-id", NULL);
+		mode = of_get_property(np, "mode", NULL);
+		if (!devid || !mode)
+			goto err;
+
+		pdata.sysclk = sysclk;
+		pdata.bus_num = *(u32 *)devid;
+
+		for (j = 0; j < binfo->num_board_infos; j++) {
+			if (binfo->board_infos[j].bus_num == pdata.bus_num)
+				pdata.max_chipselect++;
+		}
+
+		if (!pdata.max_chipselect)
+			goto err;
+
+		if (!strcmp(mode, "cpu-qe"))
+			pdata.qe_mode = 1;
+
+		ret = of_address_to_resource(np, 0, &res[0]);
+		if (ret)
+			goto err;
+
+		res[1].start = res[2].end = irq_of_parse_and_map(np, 0);
+		if (res[1].start == NO_IRQ)
+			goto err;
+
+		res[1].name = "mpc83xx_spi";
+		res[1].flags = IORESOURCE_IRQ;;
+
+		pdev = platform_device_alloc("mpc83xx_spi", i);
+		if (!pdev)
+			goto err;
+
+		ret = platform_device_add_data(pdev, &pdata, sizeof(pdata));
+		if (ret)
+			goto unreg;
+
+		ret = platform_device_add_resources(pdev, res,
+						    ARRAY_SIZE(res));
+		if (ret)
+			goto unreg;
+
+		ret = platform_device_register(pdev);
+		if (ret)
+			goto unreg;
+
+		continue;
+unreg:
+		platform_device_del(pdev);
+err:
+		continue;
+	}
+
+	return spi_register_board_info(binfo->board_infos,
+				       binfo->num_board_infos);
+}
+EXPORT_SYMBOL(fsl_spi_init);

diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index 04e145b..a83998d 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h

@@ -8,5 +8,17 @@ extern phys_addr_t get_immrbase(void);
 extern u32 get_brgfreq(void);
 extern u32 get_baudrate(void);
 
+struct spi_board_info;
+
+struct fsl_spi_board_info {
+	struct spi_board_info *board_infos;
+	unsigned int num_board_infos;
+
+	void (*activate_cs)(u8 cs, u8 polarity);
+	void (*deactivate_cs)(u8 cs, u8 polarity);
+};
+
+extern int fsl_spi_init(struct fsl_spi_board_info *info);
+
 #endif
 #endif

-- 
1.5.0.6

On Wed, Aug 08, 2007 at 09:09:21PM +0400, Anton Vorontsov wrote:

Hrm... Freescale is using these 'fsl,device-id' properties, I'm
guessing they're basically the same thing as the 'cell-index' used in
the EMAC binding.

We should co-ordinate better on this, if it's to become a
convention...

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

On Aug 8, 2007, at 12:09 PM, Anton Vorontsov wrote:

Do we really need a struct, can we not just pass the 4 params as args  
to the function?

May need to a typedef to linux/fsl_device.h for the chipselect call  
back.

- k

On Aug 8, 2007, at 12:07 PM, Anton Vorontsov wrote:

Hi all,

This is v3. The only objection I can imagine is about "fsl,device-id".
Though in the v2 nobody complained, thus it's stayed intact.

If you want to, complain now. I'll give up and will remove it. ;-)

:), here's my complaint for it.

We should just use the physical address of the bus register base as  
the id.  We do the same thing for the MII phy bus driver for gianfar.

- k

Hrm... Freescale is using these 'fsl,device-id' properties, I'm
guessing they're basically the same thing as the 'cell-index' used in
the EMAC binding.

We should co-ordinate better on this, if it's to become a
convention...

That means we shouldn't coordinate on this, right?


Segher

  - cosmetic cleanups (@01 -> @1);

That's a correctness fix, not just cosmetics.

  - other non-mandatory (device-id, device_type, compatible, ...)
    properties removed from mmc node, today board file is using
    of_find_node_by_name(), instead of of_find_compatible_node();

That's the wrong way around -- a driver (or platform code, same
thing) isn't using "compatible", while it should, so you remove
"compatible"?


Segher

On Thu, Aug 09, 2007 at 10:15:44PM +0200, Segher Boessenkool wrote:

quoted

Hrm... Freescale is using these 'fsl,device-id' properties, I'm
guessing they're basically the same thing as the 'cell-index' used in
the EMAC binding.

We should co-ordinate better on this, if it's to become a
convention...

That means we shouldn't coordinate on this, right?

Heh.  Either one is kind of ugly, I'll grant you.

But, many SoCs do have a notion of device "number", which is relevant
for programming other general control registers in places.  We need to
encode it somehow, and it would be good to have a consistent way of
doing it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

quoted

quoted

We should co-ordinate better on this, if it's to become a
convention...

That means we shouldn't coordinate on this, right?

Heh.  Either one is kind of ugly, I'll grant you.

But, many SoCs do have a notion of device "number", which is relevant
for programming other general control registers in places.  We need to
encode it somehow, and it would be good to have a consistent way of
doing it.

I'm not convinced there isn't a more direct way to represent
the relevant relationships.

Either way, we don't have enough experience with this stuff
yet to know what works well and what doesn't (at least, I
don't, and I haven't seen any evidence that others do); so
I'd prefer to keep this in per-device bindings for now; it
should be there anyhow, but once we do have experience with
it we could do some recommendation.


Segher

On Fri, Aug 10, 2007 at 10:42:51PM +0200, Segher Boessenkool wrote:

quoted

quoted

quoted

We should co-ordinate better on this, if it's to become a
convention...

That means we shouldn't coordinate on this, right?

Heh.  Either one is kind of ugly, I'll grant you.

But, many SoCs do have a notion of device "number", which is relevant
for programming other general control registers in places.  We need to
encode it somehow, and it would be good to have a consistent way of
doing it.

I'm not convinced there isn't a more direct way to represent
the relevant relationships.

Either way, we don't have enough experience with this stuff
yet to know what works well and what doesn't (at least, I
don't, and I haven't seen any evidence that others do); so
I'd prefer to keep this in per-device bindings for now; it
should be there anyhow, but once we do have experience with
it we could do some recommendation.

Well of course it will remain in the per-device bindings.  But just
because these are in different per-device bindings doesn't mean we
can't *try* to use consistent property names for similar things...

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

quoted

quoted

But, many SoCs do have a notion of device "number", which is relevant
for programming other general control registers in places.  We need 
to
encode it somehow, and it would be good to have a consistent way of
doing it.

I'm not convinced there isn't a more direct way to represent
the relevant relationships.

Either way, we don't have enough experience with this stuff
yet to know what works well and what doesn't (at least, I
don't, and I haven't seen any evidence that others do); so
I'd prefer to keep this in per-device bindings for now; it
should be there anyhow, but once we do have experience with
it we could do some recommendation.

Well of course it will remain in the per-device bindings.  But just
because these are in different per-device bindings doesn't mean we
can't *try* to use consistent property names for similar things...

Sure, if those properties do exactly the same thing, identical
names are a good thing.  Until we're reasonably certain that this
is a good way to do things I'd rather people experiment a bit
instead of all cluster around a random thing that would be labeled
"de facto standard".

:-)


Segher