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

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

[Anton Vorontsov]

Hi all,

Thanks for the previous reviews, this is v4 against Linus' tree,
as of today.

Changelog:

v3 -> v4
  - removed fsl,device-id property from SPI nodes;
  - instead of fsl_spi_info structure, now fsl_spi_init() accepting
    four arguments;
  - machine_is(mpc832x_rdb) added in the beginning of
    mpc832x_spi_init().

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

[PATCH v4 1/2] [POWERPC] fsl_soc: add support for fsl_spi

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/sysdev/fsl_soc.c |   85 +++++++++++++++++++++++++++++++++++++++++
 arch/powerpc/sysdev/fsl_soc.h |    7 +++
 2 files changed, 92 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..25a96b5 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/of_platform.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>
@@ -1187,3 +1188,87 @@ err:
 arch_initcall(cpm_smc_uart_of_init);
 
 #endif /* CONFIG_8xx */
+
+int fsl_spi_init(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))
+{
+	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 *mode;
+		struct resource res[2];
+		struct platform_device *pdev = NULL;
+		struct fsl_spi_platform_data pdata = {
+			.activate_cs = activate_cs,
+			.deactivate_cs = deactivate_cs,
+		};
+
+		memset(res, 0, sizeof(res));
+
+		mode = of_get_property(np, "mode", NULL);
+		pdata.sysclk = sysclk;
+		pdata.bus_num = *(u32 *)of_get_property(np, "reg", NULL);
+
+		for (j = 0; j < num_board_infos; j++) {
+			if (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(board_infos, 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..618d91d 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -8,5 +8,12 @@ extern phys_addr_t get_immrbase(void);
 extern u32 get_brgfreq(void);
 extern u32 get_baudrate(void);
 
+struct spi_board_info;
+
+extern int fsl_spi_init(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));
+
 #endif
 #endif
-- 
1.5.0.6

[PATCH v4 2/2] [POWERPC] MPC832x_RDB: update dts to use SPI1 in QE, register mmc_spi stub

[Anton Vorontsov]

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

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/boot/dts/mpc832x_rdb.dts     |    2 +-
 arch/powerpc/platforms/83xx/mpc832x_rdb.c |   50 +++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc832x_rdb.dts b/arch/powerpc/boot/dts/mpc832x_rdb.dts
index e9c332f..1ac0025 100644
--- a/arch/powerpc/boot/dts/mpc832x_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc832x_rdb.dts
@@ -211,7 +211,7 @@
 			reg = <4c0 40>;
 			interrupts = <2>;
 			interrupt-parent = <&qeic>;
-			mode = "cpu";
+			mode = "cpu-qe";
 		};
 
 		spi@500 {
diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
index e021b08..cf58d06 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,54 @@
 #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 = 0x4c0,
+	.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 int __init mpc832x_spi_init(void)
+{
+	if (!machine_is(mpc832x_rdb))
+		return 0;
+
+	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(&mpc832x_spi_boardinfo, 1,
+			    mpc83xx_spi_activate_cs,
+			    mpc83xx_spi_deactivate_cs);
+}
+
+device_initcall(mpc832x_spi_init);
+
 /* ************************************************************************
  *
  * Setup the architecture
-- 
1.5.0.6

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

[Anton Vorontsov]

On Tue, Aug 21, 2007 at 05:45:37PM +0400, Anton Vorontsov wrote:
> Hi all,
> 
> Thanks for the previous reviews, this is v4 against Linus' tree,
> as of today.

Okay, here is brand-new, shiny v5. Today and only today it comes
without section mismatch warnings, don't miss your chance. Get it
free of charge! ;-)

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

[PATCH v5 1/2] [POWERPC] fsl_soc: add support for fsl_spi

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/sysdev/fsl_soc.c |   84 +++++++++++++++++++++++++++++++++++++++++
 arch/powerpc/sysdev/fsl_soc.h |    7 +++
 2 files changed, 91 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..2bdaeb2 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/of_platform.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>
@@ -1187,3 +1188,86 @@ err:
 arch_initcall(cpm_smc_uart_of_init);
 
 #endif /* CONFIG_8xx */
+
+int __init fsl_spi_init(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))
+{
+	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 *mode;
+		struct resource res[2];
+		struct platform_device *pdev = NULL;
+		struct fsl_spi_platform_data pdata = {
+			.activate_cs = activate_cs,
+			.deactivate_cs = deactivate_cs,
+		};
+
+		memset(res, 0, sizeof(res));
+
+		mode = of_get_property(np, "mode", NULL);
+		pdata.sysclk = sysclk;
+		pdata.bus_num = *(u32 *)of_get_property(np, "reg", NULL);
+
+		for (j = 0; j < num_board_infos; j++) {
+			if (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(board_infos, num_board_infos);
+}
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index 04e145b..618d91d 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -8,5 +8,12 @@ extern phys_addr_t get_immrbase(void);
 extern u32 get_brgfreq(void);
 extern u32 get_baudrate(void);
 
+struct spi_board_info;
+
+extern int fsl_spi_init(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));
+
 #endif
 #endif
-- 
1.5.0.6

[PATCH v5 2/2] [POWERPC] MPC832x_RDB: update dts to use SPI1 in QE, register mmc_spi stub

[Anton Vorontsov]

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

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/boot/dts/mpc832x_rdb.dts     |    2 +-
 arch/powerpc/platforms/83xx/mpc832x_rdb.c |   50 +++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc832x_rdb.dts b/arch/powerpc/boot/dts/mpc832x_rdb.dts
index e9c332f..1ac0025 100644
--- a/arch/powerpc/boot/dts/mpc832x_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc832x_rdb.dts
@@ -211,7 +211,7 @@
 			reg = <4c0 40>;
 			interrupts = <2>;
 			interrupt-parent = <&qeic>;
-			mode = "cpu";
+			mode = "cpu-qe";
 		};
 
 		spi@500 {
diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
index e021b08..cf58d06 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,54 @@
 #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 = 0x4c0,
+	.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 int __init mpc832x_spi_init(void)
+{
+	if (!machine_is(mpc832x_rdb))
+		return 0;
+
+	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(&mpc832x_spi_boardinfo, 1,
+			    mpc83xx_spi_activate_cs,
+			    mpc83xx_spi_deactivate_cs);
+}
+
+device_initcall(mpc832x_spi_init);
+
 /* ************************************************************************
  *
  * Setup the architecture
-- 
1.5.0.6

Re: [PATCH v5 0/2] SPI support for fsl_soc and mpc832x_rdb

[Kumar Gala]

On Aug 21, 2007, at 10:27 AM, Anton Vorontsov wrote:

> On Tue, Aug 21, 2007 at 05:45:37PM +0400, Anton Vorontsov wrote:
>> Hi all,
>>
>> Thanks for the previous reviews, this is v4 against Linus' tree,
>> as of today.
>
> Okay, here is brand-new, shiny v5. Today and only today it comes
> without section mismatch warnings, don't miss your chance. Get it
> free of charge! ;-)

This looks good, have some minor cleanup nits.

- k

Re: [PATCH v5 1/2] [POWERPC] fsl_soc: add support for fsl_spi

[Kumar Gala]

On Aug 21, 2007, at 10:29 AM, Anton Vorontsov wrote:

> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  arch/powerpc/sysdev/fsl_soc.c |   84 ++++++++++++++++++++++++++++++ 
> +++++++++++
>  arch/powerpc/sysdev/fsl_soc.h |    7 +++
>  2 files changed, 91 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/ 
> fsl_soc.c
> index 1cf29c9..2bdaeb2 100644
> --- a/arch/powerpc/sysdev/fsl_soc.c
> +++ b/arch/powerpc/sysdev/fsl_soc.c
> @@ -24,6 +24,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/of_platform.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>
> @@ -1187,3 +1188,86 @@ err:
>  arch_initcall(cpm_smc_uart_of_init);
>
>  #endif /* CONFIG_8xx */
> +
> +int __init fsl_spi_init(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))
> +{
> +	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 *mode;
> +		struct resource res[2];
> +		struct platform_device *pdev = NULL;
> +		struct fsl_spi_platform_data pdata = {
> +			.activate_cs = activate_cs,
> +			.deactivate_cs = deactivate_cs,
> +		};
> +
> +		memset(res, 0, sizeof(res));
> +
> +		mode = of_get_property(np, "mode", NULL);
> +		pdata.sysclk = sysclk;
> +		pdata.bus_num = *(u32 *)of_get_property(np, "reg", NULL);
> +
> +		for (j = 0; j < num_board_infos; j++) {
> +			if (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;;

double ;;

can we not use of_irq_to_resource() ?

> +
> +		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(board_infos, num_board_infos);
> +}
> diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/ 
> fsl_soc.h
> index 04e145b..618d91d 100644
> --- a/arch/powerpc/sysdev/fsl_soc.h
> +++ b/arch/powerpc/sysdev/fsl_soc.h
> @@ -8,5 +8,12 @@ extern phys_addr_t get_immrbase(void);
>  extern u32 get_brgfreq(void);
>  extern u32 get_baudrate(void);
>
> +struct spi_board_info;
> +
> +extern int fsl_spi_init(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));
> +
>  #endif
>  #endif
> -- 
> 1.5.0.6
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH v5 2/2] [POWERPC] MPC832x_RDB: update dts to use SPI1 in QE, register mmc_spi stub

[Kumar Gala]

On Aug 21, 2007, at 10:29 AM, Anton Vorontsov wrote:

> mmc_spi already tested to work. When it will hit mainline
> the only change that will be needed is replacing "spidev"
> with "mmc_spi".
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  arch/powerpc/boot/dts/mpc832x_rdb.dts     |    2 +-
>  arch/powerpc/platforms/83xx/mpc832x_rdb.c |   50 ++++++++++++++++++ 
> +++++++++++
>  2 files changed, 51 insertions(+), 1 deletions(-)
>
> diff --git a/arch/powerpc/boot/dts/mpc832x_rdb.dts b/arch/powerpc/ 
> boot/dts/mpc832x_rdb.dts
> index e9c332f..1ac0025 100644
> --- a/arch/powerpc/boot/dts/mpc832x_rdb.dts
> +++ b/arch/powerpc/boot/dts/mpc832x_rdb.dts
> @@ -211,7 +211,7 @@
>  			reg = <4c0 40>;
>  			interrupts = <2>;
>  			interrupt-parent = <&qeic>;
> -			mode = "cpu";
> +			mode = "cpu-qe";
>  		};
>
>  		spi@500 {
> diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c b/arch/ 
> powerpc/platforms/83xx/mpc832x_rdb.c
> index e021b08..cf58d06 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"

we usually do #include <sysdev/fsl_soc.h>

>
>  #undef DEBUG
>  #ifdef DEBUG
> @@ -32,6 +34,54 @@
>  #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 = 0x4c0,
> +	.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 int __init mpc832x_spi_init(void)
> +{
> +	if (!machine_is(mpc832x_rdb))
> +		return 0;
> +
> +	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(&mpc832x_spi_boardinfo, 1,
> +			    mpc83xx_spi_activate_cs,
> +			    mpc83xx_spi_deactivate_cs);
> +}
> +
> +device_initcall(mpc832x_spi_init);
> +
>  /*  
> ********************************************************************** 
> **
>   *
>   * Setup the architecture
> -- 
> 1.5.0.6
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

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

[Anton Vorontsov]

On Wed, Aug 22, 2007 at 09:22:47AM -0500, Kumar Gala wrote:
>
> On Aug 21, 2007, at 10:27 AM, Anton Vorontsov wrote:
>
>> On Tue, Aug 21, 2007 at 05:45:37PM +0400, Anton Vorontsov wrote:
>>> Hi all,
>>>
>>> Thanks for the previous reviews, this is v4 against Linus' tree,
>>> as of today.
>>
>> Okay, here is brand-new, shiny v5. Today and only today it comes
>> without section mismatch warnings, don't miss your chance. Get it
>> free of charge! ;-)
>
> This looks good, have some minor cleanup nits.

Much thanks, fixed.

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

[PATCH v6 1/2] [POWERPC] fsl_soc: add support for fsl_spi

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/sysdev/fsl_soc.c |   81 +++++++++++++++++++++++++++++++++++++++++
 arch/powerpc/sysdev/fsl_soc.h |    7 ++++
 2 files changed, 88 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..473b45b 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/of_platform.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>
@@ -1187,3 +1188,83 @@ err:
 arch_initcall(cpm_smc_uart_of_init);
 
 #endif /* CONFIG_8xx */
+
+int __init fsl_spi_init(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))
+{
+	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 *mode;
+		struct resource res[2];
+		struct platform_device *pdev;
+		struct fsl_spi_platform_data pdata = {
+			.activate_cs = activate_cs,
+			.deactivate_cs = deactivate_cs,
+		};
+
+		memset(res, 0, sizeof(res));
+
+		mode = of_get_property(np, "mode", NULL);
+		pdata.sysclk = sysclk;
+		pdata.bus_num = *(u32 *)of_get_property(np, "reg", NULL);
+
+		for (j = 0; j < num_board_infos; j++) {
+			if (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;
+
+		ret = of_irq_to_resource(np, 0, &res[1]);
+		if (ret == NO_IRQ)
+			goto err;
+
+		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(board_infos, num_board_infos);
+}
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index 04e145b..618d91d 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -8,5 +8,12 @@ extern phys_addr_t get_immrbase(void);
 extern u32 get_brgfreq(void);
 extern u32 get_baudrate(void);
 
+struct spi_board_info;
+
+extern int fsl_spi_init(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));
+
 #endif
 #endif
-- 
1.5.0.6

[PATCH v6 2/2] [POWERPC] MPC832x_RDB: update dts to use SPI1 in QE, register mmc_spi stub

[Anton Vorontsov]

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

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/boot/dts/mpc832x_rdb.dts     |    2 +-
 arch/powerpc/platforms/83xx/mpc832x_rdb.c |   50 +++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc832x_rdb.dts b/arch/powerpc/boot/dts/mpc832x_rdb.dts
index e9c332f..1ac0025 100644
--- a/arch/powerpc/boot/dts/mpc832x_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc832x_rdb.dts
@@ -211,7 +211,7 @@
 			reg = <4c0 40>;
 			interrupts = <2>;
 			interrupt-parent = <&qeic>;
-			mode = "cpu";
+			mode = "cpu-qe";
 		};
 
 		spi@500 {
diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
index e021b08..305ec7d 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>
@@ -22,6 +23,7 @@
 #include <asm/udbg.h>
 #include <asm/qe.h>
 #include <asm/qe_ic.h>
+#include <sysdev/fsl_soc.h>
 
 #include "mpc83xx.h"
 
@@ -32,6 +34,54 @@
 #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 = 0x4c0,
+	.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 int __init mpc832x_spi_init(void)
+{
+	if (!machine_is(mpc832x_rdb))
+		return 0;
+
+	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(&mpc832x_spi_boardinfo, 1,
+			    mpc83xx_spi_activate_cs,
+			    mpc83xx_spi_deactivate_cs);
+}
+
+device_initcall(mpc832x_spi_init);
+
 /* ************************************************************************
  *
  * Setup the architecture
-- 
1.5.0.6

Re: [PATCH v6 0/2] SPI support for fsl_soc and mpc832x_rdb

[Kumar Gala]

On Aug 22, 2007, at 9:54 AM, Anton Vorontsov wrote:

> On Wed, Aug 22, 2007 at 09:22:47AM -0500, Kumar Gala wrote:
>>
>> On Aug 21, 2007, at 10:27 AM, Anton Vorontsov wrote:
>>
>>> On Tue, Aug 21, 2007 at 05:45:37PM +0400, Anton Vorontsov wrote:
>>>> Hi all,
>>>>
>>>> Thanks for the previous reviews, this is v4 against Linus' tree,
>>>> as of today.
>>>
>>> Okay, here is brand-new, shiny v5. Today and only today it comes
>>> without section mismatch warnings, don't miss your chance. Get it
>>> free of charge! ;-)
>>
>> This looks good, have some minor cleanup nits.
>
> Much thanks, fixed.

np.  Do you know what effect the modalias being wrong has on current  
functionality?  and when mmc_spi gets added?

- k

Re: [PATCH v6 0/2] SPI support for fsl_soc and mpc832x_rdb

[Anton Vorontsov]

On Wed, Aug 22, 2007 at 10:01:13AM -0500, Kumar Gala wrote:
>
> On Aug 22, 2007, at 9:54 AM, Anton Vorontsov wrote:
>
>> On Wed, Aug 22, 2007 at 09:22:47AM -0500, Kumar Gala wrote:
>>>
>>> On Aug 21, 2007, at 10:27 AM, Anton Vorontsov wrote:
>>>
>>>> On Tue, Aug 21, 2007 at 05:45:37PM +0400, Anton Vorontsov wrote:
>>>>> Hi all,
>>>>>
>>>>> Thanks for the previous reviews, this is v4 against Linus' tree,
>>>>> as of today.
>>>>
>>>> Okay, here is brand-new, shiny v5. Today and only today it comes
>>>> without section mismatch warnings, don't miss your chance. Get it
>>>> free of charge! ;-)
>>>
>>> This looks good, have some minor cleanup nits.
>>
>> Much thanks, fixed.
>
> np.  Do you know what effect the modalias being wrong has on current 
> functionality?

No effect in the `kernel oops' meanings, nothing too bad will happen.

But personally I vote for spidev stub to stay. This way I (and anybody
else) can actually test SPI (and MMC) even without mmc_spi driver, i.e.
using Documentation/spi/spidev_test.c (already in Linus' tree).

> and when mmc_spi gets added?

Let's hope in v2.6.24. IIRC it's already in -mm for some time, and it's
pretty mature already, thanks to David Brownell.

> - k

Thanks,

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

Re: [PATCH v6 1/2] [POWERPC] fsl_soc: add support for fsl_spi

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

Hi Anton,

On Wed, 22 Aug 2007 18:57:32 +0400 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
>
> +	sysclk = *(u32 *)of_get_property(np, "bus-frequency", NULL);

I just cringe everytime I see someone dereference a pointer they got from
somewhere (effectively) external without checking for NULL.  I realise
that sometimes "that can't happen" ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH v7 0/3] [POWERPC] fsl_soc: add support for fsl_spi

[Anton Vorontsov]

On Thu, Aug 23, 2007 at 01:24:21PM +1000, Stephen Rothwell wrote:
> Hi Anton,
> 
> On Wed, 22 Aug 2007 18:57:32 +0400 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> >
> > +	sysclk = *(u32 *)of_get_property(np, "bus-frequency", NULL);
> 
> I just cringe everytime I see someone dereference a pointer they got from
> somewhere (effectively) external without checking for NULL.

Actually, sitting at the editor I thought twice before _removing_ NULL
checks, and obviously was wrong with final decision. ;-)

I just knew that there is already many places in fsl_soc that don't
check for NULLs, especially when:

> I realise
> that sometimes "that can't happen" ...

Probably to save some code space. But now I seem to comprehend it: not
checking for NULL properties is not a some kind of rule for fsl_soc,
but exceptions which probably should be fixed someday.

Thanks.


[Not related to this particular answer]

Heh.. honestly speaking, I myself don't like externs I introducing in
the board file. Okay, let's risk, and do whole thing correctly:
placing externs into asm/qe.h, trivial patch that could end up with
long discussion. ;-)

Lucky numbered v7 is following.

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

[PATCH v7 1/3] [POWERPC] QE lib: extern par_io_config_pin and par_io_data_set funcs

[Anton Vorontsov]

This is needed to configure and control QE pario pins from the kernel.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 include/asm-powerpc/qe.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/asm-powerpc/qe.h b/include/asm-powerpc/qe.h
index 9d304b1..ad23c58 100644
--- a/include/asm-powerpc/qe.h
+++ b/include/asm-powerpc/qe.h
@@ -32,6 +32,9 @@
 extern void qe_reset(void);
 extern int par_io_init(struct device_node *np);
 extern int par_io_of_config(struct device_node *np);
+extern int par_io_config_pin(u8 port, u8 pin, int dir, int open_drain,
+			     int assignment, int has_irq);
+extern int par_io_data_set(u8 port, u8 pin, u8 val);
 
 /* QE internal API */
 int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol, u32 cmd_input);
-- 
1.5.0.6

[PATCH v7 2/3] [POWERPC] fsl_soc: add support for fsl_spi

[Anton Vorontsov]

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/sysdev/fsl_soc.c |   87 +++++++++++++++++++++++++++++++++++++++++
 arch/powerpc/sysdev/fsl_soc.h |    7 +++
 2 files changed, 94 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..2f11323 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/of_platform.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>
@@ -1187,3 +1188,89 @@ err:
 arch_initcall(cpm_smc_uart_of_init);
 
 #endif /* CONFIG_8xx */
+
+int __init fsl_spi_init(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))
+{
+	struct device_node *np;
+	unsigned int i;
+	const u32 *sysclk;
+
+	np = of_find_node_by_type(NULL, "qe");
+	if (!np)
+		return -ENODEV;
+
+	sysclk = of_get_property(np, "bus-frequency", NULL);
+	if (!sysclk)
+		return -ENODEV;
+
+	for (np = NULL, i = 1;
+	     (np = of_find_compatible_node(np, "spi", "fsl_spi")) != NULL;
+	     i++) {
+		int ret = 0;
+		unsigned int j;
+		const void *prop;
+		struct resource res[2];
+		struct platform_device *pdev;
+		struct fsl_spi_platform_data pdata = {
+			.activate_cs = activate_cs,
+			.deactivate_cs = deactivate_cs,
+		};
+
+		memset(res, 0, sizeof(res));
+
+		pdata.sysclk = *sysclk;
+
+		prop = of_get_property(np, "reg", NULL);
+		if (!prop)
+			goto err;
+		pdata.bus_num = *(u32 *)prop;
+
+		prop = of_get_property(np, "mode", NULL);
+		if (prop && !strcmp(prop, "cpu-qe"))
+			pdata.qe_mode = 1;
+
+		for (j = 0; j < num_board_infos; j++) {
+			if (board_infos[j].bus_num == pdata.bus_num)
+				pdata.max_chipselect++;
+		}
+
+		if (!pdata.max_chipselect)
+			goto err;
+
+		ret = of_address_to_resource(np, 0, &res[0]);
+		if (ret)
+			goto err;
+
+		ret = of_irq_to_resource(np, 0, &res[1]);
+		if (ret == NO_IRQ)
+			goto err;
+
+		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(board_infos, num_board_infos);
+}
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index 04e145b..618d91d 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -8,5 +8,12 @@ extern phys_addr_t get_immrbase(void);
 extern u32 get_brgfreq(void);
 extern u32 get_baudrate(void);
 
+struct spi_board_info;
+
+extern int fsl_spi_init(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));
+
 #endif
 #endif
-- 
1.5.0.6

[PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1 in QE, register mmc_spi stub

[Anton Vorontsov]

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

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/boot/dts/mpc832x_rdb.dts     |    2 +-
 arch/powerpc/platforms/83xx/mpc832x_rdb.c |   46 +++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/dts/mpc832x_rdb.dts b/arch/powerpc/boot/dts/mpc832x_rdb.dts
index e9c332f..1ac0025 100644
--- a/arch/powerpc/boot/dts/mpc832x_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc832x_rdb.dts
@@ -211,7 +211,7 @@
 			reg = <4c0 40>;
 			interrupts = <2>;
 			interrupt-parent = <&qeic>;
-			mode = "cpu";
+			mode = "cpu-qe";
 		};
 
 		spi@500 {
diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
index e021b08..7ddb527 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>
@@ -22,6 +23,7 @@
 #include <asm/udbg.h>
 #include <asm/qe.h>
 #include <asm/qe_ic.h>
+#include <sysdev/fsl_soc.h>
 
 #include "mpc83xx.h"
 
@@ -32,6 +34,50 @@
 #define DBG(fmt...)
 #endif
 
+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 = 0x4c0,
+	.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 int __init mpc832x_spi_init(void)
+{
+	if (!machine_is(mpc832x_rdb))
+		return 0;
+
+	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(&mpc832x_spi_boardinfo, 1,
+			    mpc83xx_spi_activate_cs,
+			    mpc83xx_spi_deactivate_cs);
+}
+
+device_initcall(mpc832x_spi_init);
+
 /* ************************************************************************
  *
  * Setup the architecture
-- 
1.5.0.6

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1 in QE, register mmc_spi stub

[Timur Tabi]

Anton Vorontsov wrote:

> +static int __init mpc832x_spi_init(void)
> +{
> +	if (!machine_is(mpc832x_rdb))
> +		return 0;
> +
> +	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 */

Why are you doing this here, and not in the device tree?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

RE: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Li Yang-r58472]

> -----Original Message-----
> From: linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org=20
> [mailto:linuxppc-dev-bounces+leoli=3Dfreescale.com@ozlabs.org]=20
> On Behalf Of Tabi Timur-B04825
> Sent: Friday, August 31, 2007 5:07 AM
> To: Anton Vorontsov
> Cc: linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts=20
> to use SPI1in QE, register mmc_spi stub
>=20
> Anton Vorontsov wrote:
>=20
> > +static int __init mpc832x_spi_init(void) {
> > +	if (!machine_is(mpc832x_rdb))
> > +		return 0;
> > +
> > +	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 */
>=20
> Why are you doing this here, and not in the device tree?

I saw you used pio node, but later changed to make it hardcoded.  What's
the reason?  IMO, device tree is used to describe the hardware settings,
pio node is a perfect match.  Moreover, changing the device tree is much
easier than changing the code.

- Leo

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Segher Boessenkool]

>>> +	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 */
>>
>> Why are you doing this here, and not in the device tree?
>
> I saw you used pio node, but later changed to make it hardcoded.  
> What's
> the reason?  IMO, device tree is used to describe the hardware 
> settings,
> pio node is a perfect match.

Not at all.  The device tree describe how the hardware _is_
set up (after firmware, bootloader etc.); now how it _should
be_ set up by the kernel.

It would make a lot of sense to do this work in the firmware
instead, but it doesn't make sense at all to put this stuff
into the device tree.

> Moreover, changing the device tree is much
> easier than changing the code.

How so?  You don't like C code?  :-)


Segher

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Timur Tabi]

Segher Boessenkool wrote:

> Not at all.  The device tree describe how the hardware _is_
> set up (after firmware, bootloader etc.); now how it _should
> be_ set up by the kernel.

I agree with this general sentiment, but in the case of QE pin configuration, 
then device tree, in a sense, does contain how the hardware is set up.  The 
par_io section in the device tree describes they layout of the wiring between 
the SOC and peripherals.  If the par_io registers are not programmed 
correctly, the SOC won't be able to communicate with the peripheral.

Sure, the kernel currently reads the device tree and programs the par_io 
registers accordingly, but that doesn't mean the information *shouldn't* be in 
the device tree.

> It would make a lot of sense to do this work in the firmware
> instead, but it doesn't make sense at all to put this stuff
> into the device tree.

1) If the firmware does configure the pins, then the device tree *will* 
describe how the hardware is set up.

2) How would the firmware know how to do board configuration if it doesn't 
have the instructions in the device tree?

Besides, every other board does it's par_io configuration based on the device 
tree.  So if Anton is going to break that pattern, we should be talking about 
moving all that code into U-boot, instead of just putting in a one-time 
exception (especially since the patch contains no explanation as to why these 
par_io pins are being configured differently than every other board).

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Anton Vorontsov]

On Mon, Sep 03, 2007 at 08:55:56AM -0500, Timur Tabi wrote:
[...]
> Besides, every other board does it's par_io configuration based on the 
> device tree.

My words. ;-) See
http://ozlabs.org/pipermail/linuxppc-dev/2007-August/040685.html

Also, see how it was made initially:
http://ozlabs.org/pipermail/linuxppc-dev/2007-August/040440.html

par_io was in the device tree. But Kumar and Segher convinced me to
not do that.

> So if Anton is going to break that pattern, we should be 
> talking about moving all that code into U-boot,
[...]

To my personal taste that's bad idea. In that scheme, I need to upgrade
bootloader to use such a little thing as SPI from the kernel.

I expect very few things from the bootloader: setup memory controller,
copy kernel from the source (flash, ethernet, rs232, ide, sata, ...)
into the RAM and boot it. Everything else from the bootloader side is
profusion.

p.s. Cc'ing Kumar.

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

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Segher Boessenkool]

>> Not at all.  The device tree describe how the hardware _is_
>> set up (after firmware, bootloader etc.); now how it _should
>> be_ set up by the kernel.
>
> I agree with this general sentiment, but in the case of QE pin 
> configuration, then device tree, in a sense, does contain how the 
> hardware is set up.  The par_io section in the device tree describes 
> they layout of the wiring between the SOC and peripherals.  If the 
> par_io registers are not programmed correctly, the SOC won't be able 
> to communicate with the peripheral.
>
> Sure, the kernel currently reads the device tree and programs the 
> par_io registers accordingly, but that doesn't mean the information 
> *shouldn't* be in the device tree.

It is fine if the device tree describes how things are wired together;
that's part of the device tree's purpose, after all.

It is not fine if the device tree says _how to_ configure the hardware;
you are supposed to put your device drivers elsewhere, not in the
device tree!

>> It would make a lot of sense to do this work in the firmware
>> instead, but it doesn't make sense at all to put this stuff
>> into the device tree.
>
> 1) If the firmware does configure the pins, then the device tree 
> *will* describe how the hardware is set up.

Yes.  It won't show pario register contents though.

> 2) How would the firmware know how to do board configuration if it 
> doesn't have the instructions in the device tree?

This kind of thing is typically hardcoded into the firmware (just like
the device tree is) -- just put it somewhere _next to_ the device tree,
not _in_ it.  The device tree is not a config file for the firmware; it
is an interface between the firmware and its client (an OS, typically).

> Besides, every other board does it's par_io configuration based on the 
> device tree.  So if Anton is going to break that pattern, we should be 
> talking about moving all that code into U-boot, instead of just 
> putting in a one-time exception (especially since the patch contains 
> no explanation as to why these par_io pins are being configured 
> differently than every other board).

Yes, I definitely didn't mean to single out Anton's patch, sorry if it
sounded that way.


Segher

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Segher Boessenkool]

> I expect very few things from the bootloader: setup memory controller,

Setup CPUs, clocks and voltages, system busses, system controllers,
memory controllers, memory itself, _and system GPIOs_ :-)

You know, all the basic and/or board-specific stuff that is needed to
run a system.  This kind of GPIOs definitely belongs to that category.


Segher

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Timur Tabi]

Segher Boessenkool wrote:

> This kind of thing is typically hardcoded into the firmware (just like
> the device tree is) -- just put it somewhere _next to_ the device tree,
> not _in_ it. 

What is next to the device tree?  AFAIK, there is no other standard data 
structure that could take place of the par_io nodes in the DTS.

I agree with your points, Segher, but replacing the par_io node with a bunch 
of par_io_config_pin() calls in the kernel is not really an improvement, I 
think.  Until we migrate the QE pin configuration code to U-boot, I suggest 
that we keep the device tree structure as-is and continue to use it for new 
code.  That way, it will all stay in one place.


-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Anton Vorontsov]

On Tue, Sep 04, 2007 at 01:17:51AM +0200, Segher Boessenkool wrote:
>> I expect very few things from the bootloader: setup memory controller,
>
> Setup CPUs, clocks and voltages, system busses, system controllers,
> memory controllers, memory itself,

Yup-yup, these little things. ;-) But clocks and voltages are set
up by the firmware to initially boot the kernel. After kernel
booted, it can, and should, and do manage this stuff. Power
management (includingturning on/off clocks, voltages), cpu freq
(this leads into whole other management issues, like changing
prescalers for companion chips and/or on-board CPLDs, if they're
getting clocks from the CPU-dependant source).

That's for clocks and voltages. Ok, let's back to GPIOs.

> _and system GPIOs_ :-)

Yup, firmware should set up gpios, to make initial kernel boot.
After that, kernel can and should manage GPIOs. Few examples.

Say units (like USB or SPI) can work in very different modes. And
it's okay if user want to change device mode in the runtime. Often
that means that we must also change GPIOs' dedicated functions or
even directions. Say, SPI slave or master, USB host/device/otg,
and so on.

Yes, probably there is no single powerpc driver actually doing this
today, because of many reasons. Some features unneeded today, some
are hard to implement. And maybe there is no single PowerPC device
yet that needs this, I don't know, but it's easy to imagine. ;-)

Another example, power management - if some unit temporarily unused,
to save power GPIOs should be set up differently. Say if SPI unit
turned off (unused just now), it's wise to bring their dedicated
GPIOs to "power saving" state (be it output0/1 or input, it
depends).

"Unneeded" GPIO pin drains less than 1 mA, but if there are bunch of
such GPIOs, this could be a real issue, especially on battery
powered devices.

And yet another GPIOs use from the kernel side I can bring, and it's
real use case: software-managed chip-selects, just like I have to
use for MMC-over-SPI.

To sum up: kernel anyway must be aware of GPIOs, and I'd prefer
smart kernel wrt GPIOs, instead of dumb kernel blindly relying on
the firmware setup.


At the same time I agree: doing gpio setup in the board file isn't a
great solution, just like doing it in the device tree. But hard-code
gpio setup in the firmware is much worse and short-sighted approach.

Thanks,

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

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Scott Wood]

On Tue, Sep 04, 2007 at 02:47:50PM +0400, Anton Vorontsov wrote:
> > _and system GPIOs_ :-)
> 
> Yup, firmware should set up gpios, to make initial kernel boot.

No, it should set all pins and similar setup-once type initialization
that is board-specific rather than device-specific -- there's no reason
for the kernel to need to care about that sort of thing.

> After that, kernel can and should manage GPIOs. Few examples.
> 
> Say units (like USB or SPI) can work in very different modes. And
> it's okay if user want to change device mode in the runtime.

This is a special case that warrants kernel involvement.  It should not
be used as justification for the bootloader leaving pins unconfigured in
the majority of cases where it does not make sense to change them at
runtime.

> Another example, power management - if some unit temporarily unused,
> to save power GPIOs should be set up differently. Say if SPI unit
> turned off (unused just now), it's wise to bring their dedicated
> GPIOs to "power saving" state (be it output0/1 or input, it
> depends).

The kernel is of course welcome to do so -- and this may be a valid
reason to attach pin information to specific device nodes, if it actually
saves a non-negligible amount of power -- but it's not a reason to force
the kernel to have to care by not setting things up in the firmware.

-Scott

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Vitaly Bordug]

On Tue, 4 Sep 2007 13:20:28 -0500
Scott Wood wrote:

> > > _and system GPIOs_ :-)  
> > 
> > Yup, firmware should set up gpios, to make initial kernel boot.  
> 
> No, it should set all pins and similar setup-once type initialization
> that is board-specific rather than device-specific -- there's no
> reason for the kernel to need to care about that sort of thing.
> 
> > After that, kernel can and should manage GPIOs. Few examples.
> > 
> > Say units (like USB or SPI) can work in very different modes. And
> > it's okay if user want to change device mode in the runtime.  
> 
> This is a special case that warrants kernel involvement.  It should
> not be used as justification for the bootloader leaving pins
> unconfigured in the majority of cases where it does not make sense to
> change them at runtime.
> 

This argue is useless: both opinions have same weight. Firmware is able to provide meaningful default gpio setup, and nobody
will object. The point is, that blindly assume all the low-level  stuff is fine, you are able only in case you've developed respective bits in the firmware.

More usual case is additional overhead of checking if the firmware not attempting to do something not really wanted, so we'll
always have a tradeoff. 


-- 
Sincerely, Vitaly

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Anton Vorontsov]

On Tue, Sep 04, 2007 at 01:20:28PM -0500, Scott Wood wrote:
> On Tue, Sep 04, 2007 at 02:47:50PM +0400, Anton Vorontsov wrote:
> > > _and system GPIOs_ :-)
> > 
> > Yup, firmware should set up gpios, to make initial kernel boot.
> 
> No, it should set all pins and similar setup-once type initialization
> that is board-specific rather than device-specific -- there's no reason
> for the kernel to need to care about that sort of thing.
> 
> > After that, kernel can and should manage GPIOs. Few examples.
> > 
> > Say units (like USB or SPI) can work in very different modes. And
> > it's okay if user want to change device mode in the runtime.
> 
> This is a special case that warrants kernel involvement.  It should not
> be used as justification for the bootloader leaving pins unconfigured in
> the majority of cases where it does not make sense to change them at
> runtime.
> 
> > Another example, power management - if some unit temporarily unused,
> > to save power GPIOs should be set up differently. Say if SPI unit
> > turned off (unused just now), it's wise to bring their dedicated
> > GPIOs to "power saving" state (be it output0/1 or input, it
> > depends).
> 
> The kernel is of course welcome to do so -- and this may be a valid
> reason to attach pin information to specific device nodes, if it actually
> saves a non-negligible amount of power -- but it's not a reason to force
> the kernel to have to care by not setting things up in the firmware.

Well, I might agree here. But to me it seems unnatural that I have to
upgrade bootloader to use SPI -- I can already boot the kernel.

Bootloader's duties are finished when kernel booted. And if already
running kernel is unable to do something, it's not bootloader's fault
anymore, but kernel's itself. At least I like to think so. Maybe I'm
wrong, yet not sure. ;-)

And from the practical point of view, upgrading bootloader is much
more error-prone and risky for the users without proper rescue tools
and knowledge. Kernel is easier to deploy after bug-fixing (and
wrongly set up GPIO pin is a bug). That's why I tend to like "dumb
and simple" bootloaders and do not hang up too much duties on it.

Anyhow, all above is just my own preference, I'm not arguing at
all, because personally I'm fine with any option, be it dts, board
file, device driver's file, the bootloader, or all at once. ;-)

> -Scott

Thanks,

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

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Scott Wood]

On Wed, Sep 05, 2007 at 03:40:23PM +0400, Anton Vorontsov wrote:
> On Tue, Sep 04, 2007 at 01:20:28PM -0500, Scott Wood wrote:
> > The kernel is of course welcome to do so -- and this may be a valid
> > reason to attach pin information to specific device nodes, if it actually
> > saves a non-negligible amount of power -- but it's not a reason to force
> > the kernel to have to care by not setting things up in the firmware.
> 
> Well, I might agree here. But to me it seems unnatural that I have to
> upgrade bootloader to use SPI -- I can already boot the kernel.

Sure -- the firmware should have been done right the first time.

Unfortunately, it very often doesn't, and thus fixups in the kernel's
platform code are warranted, but the firmware is still the preferred
place to do it.

> Bootloader's duties are finished when kernel booted.

And this line of thinking is what causes boards to be shipped with
half-assed firmware. :-)

-Scott

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Segher Boessenkool]

>> This kind of thing is typically hardcoded into the firmware (just like
>> the device tree is) -- just put it somewhere _next to_ the device 
>> tree,
>> not _in_ it.
>
> What is next to the device tree?

"Anywhere else in the firmware".

> AFAIK, there is no other standard data structure that could take place 
> of the par_io nodes in the DTS.

The device tree is not a dumping ground for all your "I need some
standard data structure" stuff.  Use an XML file if you have to ;-)

> I agree with your points, Segher, but replacing the par_io node with a 
> bunch of par_io_config_pin() calls in the kernel is not really an 
> improvement, I think.  Until we migrate the QE pin configuration code 
> to U-boot, I suggest that we keep the device tree structure as-is and 
> continue to use it for new code.  That way, it will all stay in one 
> place.

Sure, some migration plan is in order, things won't change overnight.
That doesn't mean you don't need to start planning _now_ ;-)


Segher

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Scott Wood]

On Thu, Sep 06, 2007 at 04:13:56PM +0200, Segher Boessenkool wrote:
> > AFAIK, there is no other standard data structure that could take place 
> > of the par_io nodes in the DTS.
> 
> The device tree is not a dumping ground for all your "I need some
> standard data structure" stuff.  Use an XML file if you have to ;-)

Bah.  How about we just remove the nodes you don't want to see before
passing it on to the kernel? :-)

-Scott

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Segher Boessenkool]

>> _and system GPIOs_ :-)
>
> Yup, firmware should set up gpios, to make initial kernel boot.
> After that, kernel can and should manage GPIOs.

Sure.  But only the GPIOs it _does_ need to toggle, not the ones
that have to be fixed to a certain value (like everything that is
described in the par_io nodes now).

Devices that really _use_ some GPIO, should use some generic GPIO
binding in the device tree, and the generic GPIO subsystem in the
kernel.

> Few examples.

[some good examples of why GPIOs can be useful at runtime snipped]

> At the same time I agree: doing gpio setup in the board file isn't a
> great solution, just like doing it in the device tree. But hard-code
> gpio setup in the firmware is much worse and short-sighted approach.

It is the correct solution for 99.99% of GPIOs: every GPIO needs to
either be set to some fixed configuration (and value), dictated by
the board design; or at least it needs to be initialised to something
that results in a stable system ;-)


Segher

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Segher Boessenkool]

>> The kernel is of course welcome to do so -- and this may be a valid
>> reason to attach pin information to specific device nodes, if it 
>> actually
>> saves a non-negligible amount of power -- but it's not a reason to 
>> force
>> the kernel to have to care by not setting things up in the firmware.
>
> Well, I might agree here. But to me it seems unnatural that I have to
> upgrade bootloader to use SPI -- I can already boot the kernel.
>
> Bootloader's duties are finished when kernel booted. And if already
> running kernel is unable to do something, it's not bootloader's fault
> anymore, but kernel's itself.

If the firmware failed to properly initialise the system into some
stable state, then yeah, it _is_ the firmware's fault.

> And from the practical point of view, upgrading bootloader is much
> more error-prone and risky for the users without proper rescue tools
> and knowledge.

Yeah well -- it's hardly rocket science to make this a very
reliable and safe procedure.

>  Kernel is easier to deploy after bug-fixing (and
> wrongly set up GPIO pin is a bug). That's why I tend to like "dumb
> and simple" bootloaders and do not hang up too much duties on it.

That's one of the ways it is done -- have a "dumb and simple"
recovery firmware that can install new versions of the "main"
firmware.

Anyway, we're getting very far off-topic now :-)


Segher

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Segher Boessenkool]

>>> AFAIK, there is no other standard data structure that could take 
>>> place
>>> of the par_io nodes in the DTS.
>>
>> The device tree is not a dumping ground for all your "I need some
>> standard data structure" stuff.  Use an XML file if you have to ;-)
>
> Bah.  How about we just remove the nodes you don't want to see before
> passing it on to the kernel? :-)

That is fine.  In fact, LinuxBIOS will probably do similar things.

Since this node will have one user only, feel free to create a
format for it that is as simple as possible for what you need
it for, there's no need to follow any conventions (as long as
you can create a _real_ tree from it later ;-) )


Segher

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Timur Tabi]

Segher Boessenkool wrote:
>>> _and system GPIOs_ :-)
>>
>> Yup, firmware should set up gpios, to make initial kernel boot.
>> After that, kernel can and should manage GPIOs.
> 
> Sure.  But only the GPIOs it _does_ need to toggle, not the ones
> that have to be fixed to a certain value (like everything that is
> described in the par_io nodes now).

Some history:

The par_io nodes are statically defined because the we don't support the 
concept of moving devices from one UCC to another.  When the boards are 
designed, typically the external PHYs (UART, Ethernet, whatever) are 
hard-wired to a particular UCC, and so its par_io configuration is static.

This works only because there are usually more than enough UCCs for every 
device.  The 8360, for instance, has 8 UCCs, but the MPC8360EMDS only uses two 
of them, for Ethernet.

So having the par_io nodes in the device tree is very convenient, because it

A) Is simple and compact
B) Doesn't require full-blown GPIO support
C) The device drivers can ignore par_io programming - they can just assume 
that the pins are configured correctly.
D) It makes it easy for U-Boot and/or Linux to configure the pins.

There are some drawbacks.  For instance on UART, in loopback mode, the par_io 
pins should be reconfigured.  Since loopback mode is a configurable option for 
serial drivers, that means that a UCC UART driver (which I'm working on) needs 
to be aware of the par_io pins.  It would be nice if there were a way I could 
tag a par_io node as being for loopback.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[David Gibson]

On Wed, Sep 05, 2007 at 08:21:06AM -0500, Scott Wood wrote:
> On Wed, Sep 05, 2007 at 03:40:23PM +0400, Anton Vorontsov wrote:
> > On Tue, Sep 04, 2007 at 01:20:28PM -0500, Scott Wood wrote:
> > > The kernel is of course welcome to do so -- and this may be a valid
> > > reason to attach pin information to specific device nodes, if it actually
> > > saves a non-negligible amount of power -- but it's not a reason to force
> > > the kernel to have to care by not setting things up in the firmware.
> > 
> > Well, I might agree here. But to me it seems unnatural that I have to
> > upgrade bootloader to use SPI -- I can already boot the kernel.
> 
> Sure -- the firmware should have been done right the first time.
> 
> Unfortunately, it very often doesn't, and thus fixups in the kernel's
> platform code are warranted, but the firmware is still the preferred
> place to do it.

Appealing though it is, I think the whole "firmware is the preferred
place to do it" approach is a lost cause (for nearly every value of
"it").

Firmwares are, more often than not, crap, and that's unlikely to
change.  For a lot of things, having the kernel or bootwrapper cope as
a special case with a handful of crap firmwares which don't set things
up properly isn't actually any easier than having it set them up
itself, always.

Which is why I err strongly in favour of having the kernel set things
up rather than rely on firmware setup, unless there's a very strong
reason why we *have* to respect the firmware's setup.

(Incidentally, this reasoning is why although the approach is very
neat-looking, I'm actually quite uncomfortable with firmwares directly
supplying flattened device trees to the kernel, rather than having the
tree in the bootwrapper.)

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

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[Timur Tabi]

David Gibson wrote:

> Firmwares are, more often than not, crap, and that's unlikely to
> change.  For a lot of things, having the kernel or bootwrapper cope as
> a special case with a handful of crap firmwares which don't set things
> up properly isn't actually any easier than having it set them up
> itself, always.

Similarly, firmware is very often unlikely to be updated.  So we need to 
support situations where we use old firmware with new kernels.  The cuImage 
support is a classic example of this.  Moving code from kernel to firmware 
just because some people think it belongs in the firmware is usually impractical.

If we added code to U-Boot today to configure the par_io pins, it would 
probably still be years before we could safely remove that code from the kernel.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH v7 3/3] [POWERPC] MPC832x_RDB: update dts to use SPI1in QE, register mmc_spi stub

[David Gibson]

On Thu, Sep 06, 2007 at 04:13:56PM +0200, Segher Boessenkool wrote:
> >> This kind of thing is typically hardcoded into the firmware (just like
> >> the device tree is) -- just put it somewhere _next to_ the device 
> >> tree,
> >> not _in_ it.
> >
> > What is next to the device tree?
> 
> "Anywhere else in the firmware".
> 
> > AFAIK, there is no other standard data structure that could take place 
> > of the par_io nodes in the DTS.
> 
> The device tree is not a dumping ground for all your "I need some
> standard data structure" stuff.  Use an XML file if you have to ;-)

Actually, I think "I need a standard data structure" is well within
the purview of what we hoped the flat device tree could do.  We don't
have to follow IEEE1275 slavishly.

To isolate this sort of thing out from the device tree "proper", we
may wish to define some new "virtual" device nodes to go at the root
level (in the same way that /chosen and /aliases already have defined
purposes that don't involve describing real devices).

I can't speak specifically to the par_io nodes, because their function
was not obvious to me from my glance at the examples we have, and I
don't recall whatever previous discussions they were described in.

We do want, where possible, though, to make it unambiguous what things
are describing immutable facts about the hardware and what are hints
or startup configuration only.

> > I agree with your points, Segher, but replacing the par_io node with a 
> > bunch of par_io_config_pin() calls in the kernel is not really an 
> > improvement, I think.  Until we migrate the QE pin configuration code 
> > to U-boot, I suggest that we keep the device tree structure as-is and 
> > continue to use it for new code.  That way, it will all stay in one 
> > place.
> 
> Sure, some migration plan is in order, things won't change overnight.
> That doesn't mean you don't need to start planning _now_ ;-)

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