diff mbox

[6/6] powerpc/fsl_soc: Isolate legacy fsl_spi support to mpc832x_rdb boards

Message ID 20090123195041.GF21237@oksana.dev.rtsoft.ru (mailing list archive)
State Accepted, archived
Delegated to: Kumar Gala
Headers show

Commit Message

Anton Vorontsov Jan. 23, 2009, 7:50 p.m. UTC 
The advantages of this:
- Don't encourage legacy support;
- Less external symbols, less code to compile-in for !MPC832x_RDB
  platforms.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/platforms/83xx/mpc832x_rdb.c |  107 +++++++++++++++++++++++++++++
 arch/powerpc/sysdev/fsl_soc.c             |  107 -----------------------------
 arch/powerpc/sysdev/fsl_soc.h             |    4 -
 3 files changed, 107 insertions(+), 111 deletions(-)

Comments

Peter Korsgaard April 8, 2009, 9:18 a.m. UTC | #1 
>>>>> "Anton" == Anton Vorontsov <avorontsov@ru.mvista.com> writes:

Hi,

 Anton> The advantages of this:
 Anton> - Don't encourage legacy support;
 Anton> - Less external symbols, less code to compile-in for !MPC832x_RDB
 Anton>   platforms.

It's nice with your cleanups, but I wonder how to handle more
complicated chip select handling than simply toggling a single gpio.

I have a board (or 2 actually, but they are similar in this regard)
with a mpc8347 using SPI to a number of addon boards. For signal
integrity reasons the SPI signals are routed to a MUX, so the chip
select logic has to set the MUX in addition to controlling the CS line
of the device.

I've been using code like this since late 2007, but this patch
ofcourse breaks it:

static void thinx_spi_activate_cs(u8 cs, u8 polarity)
{
	static u8 old_cs = 255;

	if (cs != old_cs) {
		/* mux setup (cs 2:1)*/
		gpio_set_value(gpio1 + GPIO_SPI_MUX_NOE, 1);
		gpio_set_value(gpio1 + GPIO_SPI_MUX_SEL0, cs&2);
		gpio_set_value(gpio1 + GPIO_SPI_MUX_SEL1, cs&4);
		gpio_set_value(gpio1 + GPIO_SPI_MUX_NOE, 0);
		old_cs = cs;
	}

	switch (cs) {
	case 0: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL1, polarity); break;
	case 1: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL2, polarity); break;
	case 2: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT1, polarity); break;
	case 3: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT2, polarity); break;
	}
}

static void thinx_spi_deactivate_cs(u8 cs, u8 polarity)
{
	switch (cs) {
	case 0: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL1, !polarity); break;
	case 1: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL2, !polarity); break;
	case 2: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT1, !polarity); break;
	case 3: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT2, !polarity); break;
	}
}

static __init int thinx_spi_init(void)
{
	struct device_node *np;
	struct of_gpio_chip *gc;
	static const int gpios[] = {
		GPIO_SPI_CS_BKL1,
		GPIO_SPI_CS_BKL2,
		GPIO_SPI_CS_OPT1,
		GPIO_SPI_CS_OPT2,
		GPIO_SPI_MUX_NOE,
		GPIO_SPI_MUX_SEL0,
		GPIO_SPI_MUX_SEL1
	};
	int i;

	np = of_find_node_by_name(NULL, "gpio-controller");
	if (!np || !np->data) {
		printk(KERN_ERR
		       "gpio1 node not found or controller not registerred\n");
		return -ENODEV;
	}
	gc = np->data;
	gpio1 = gc->gc.base;

	for (i=0; i<ARRAY_SIZE(gpios); i++) {
		gpio_request(gpio1 + gpios[i], "spi");
		gpio_direction_output(gpio1 + gpios[i], 1);
	}

	fsl_spi_init(thinx_spi_boardinfo, ARRAY_SIZE(thinx_spi_boardinfo),
		     thinx_spi_activate_cs, thinx_spi_deactivate_cs);

	return 0;
}

Now, I don't quite see how to handle this with the new OF bindings -
Any ideas?
Peter Korsgaard April 17, 2009, 5:13 a.m. UTC | #2 
>>>>> "Peter" == Peter Korsgaard <jacmet@sunsite.dk> writes:

Anyone? I've locally reverted the commit, but most likely I'm not the
only one using the spi_mpc83xx driver without direct gpio controlled
chip select handling.

 Anton> The advantages of this:
 Anton> - Don't encourage legacy support;
 Anton> - Less external symbols, less code to compile-in for !MPC832x_RDB
 Anton> platforms.

 Peter> It's nice with your cleanups, but I wonder how to handle more
 Peter> complicated chip select handling than simply toggling a single gpio.

 Peter> I have a board (or 2 actually, but they are similar in this regard)
 Peter> with a mpc8347 using SPI to a number of addon boards. For signal
 Peter> integrity reasons the SPI signals are routed to a MUX, so the chip
 Peter> select logic has to set the MUX in addition to controlling the CS line
 Peter> of the device.

 Peter> I've been using code like this since late 2007, but this patch
 Peter> ofcourse breaks it:

 Peter> static void thinx_spi_activate_cs(u8 cs, u8 polarity)
 Peter> {
 Peter> 	static u8 old_cs = 255;

 Peter> 	if (cs != old_cs) {
 Peter> 		/* mux setup (cs 2:1)*/
 Peter> 		gpio_set_value(gpio1 + GPIO_SPI_MUX_NOE, 1);
 Peter> 		gpio_set_value(gpio1 + GPIO_SPI_MUX_SEL0, cs&2);
 Peter> 		gpio_set_value(gpio1 + GPIO_SPI_MUX_SEL1, cs&4);
 Peter> 		gpio_set_value(gpio1 + GPIO_SPI_MUX_NOE, 0);
 Peter> 		old_cs = cs;
 Peter> 	}

 Peter> 	switch (cs) {
 Peter> 	case 0: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL1, polarity); break;
 Peter> 	case 1: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL2, polarity); break;
 Peter> 	case 2: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT1, polarity); break;
 Peter> 	case 3: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT2, polarity); break;
 Peter> 	}
 Peter> }

 Peter> static void thinx_spi_deactivate_cs(u8 cs, u8 polarity)
 Peter> {
 Peter> 	switch (cs) {
 Peter> 	case 0: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL1, !polarity); break;
 Peter> 	case 1: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL2, !polarity); break;
 Peter> 	case 2: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT1, !polarity); break;
 Peter> 	case 3: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT2, !polarity); break;
 Peter> 	}
 Peter> }

 Peter> static __init int thinx_spi_init(void)
 Peter> {
 Peter> 	struct device_node *np;
 Peter> 	struct of_gpio_chip *gc;
 Peter> 	static const int gpios[] = {
 Peter> 		GPIO_SPI_CS_BKL1,
 Peter> 		GPIO_SPI_CS_BKL2,
 Peter> 		GPIO_SPI_CS_OPT1,
 Peter> 		GPIO_SPI_CS_OPT2,
 Peter> 		GPIO_SPI_MUX_NOE,
 Peter> 		GPIO_SPI_MUX_SEL0,
 Peter> 		GPIO_SPI_MUX_SEL1
 Peter> 	};
 Peter> 	int i;

 Peter> 	np = of_find_node_by_name(NULL, "gpio-controller");
 Peter> 	if (!np || !np->data) {
 Peter> 		printk(KERN_ERR
 Peter> 		       "gpio1 node not found or controller not registerred\n");
 Peter> 		return -ENODEV;
 Peter> 	}
 Peter> 	gc = np->data;
 Peter> 	gpio1 = gc->gc.base;

 Peter> 	for (i=0; i<ARRAY_SIZE(gpios); i++) {
 Peter> 		gpio_request(gpio1 + gpios[i], "spi");
 Peter> 		gpio_direction_output(gpio1 + gpios[i], 1);
 Peter> 	}

 Peter> 	fsl_spi_init(thinx_spi_boardinfo, ARRAY_SIZE(thinx_spi_boardinfo),
 Peter> 		     thinx_spi_activate_cs, thinx_spi_deactivate_cs);

 Peter> 	return 0;
 Peter> }

 Peter> Now, I don't quite see how to handle this with the new OF bindings -
 Peter> Any ideas?
Anton Vorontsov April 17, 2009, 12:23 p.m. UTC | #3 
Hi Peter,

Sorry for the late response (and don't hesitate to ping me if I
don't answer, some things get lost in my inbox traffic, sorry).

On Wed, Apr 08, 2009 at 11:18:43AM +0200, Peter Korsgaard wrote:
> >>>>> "Anton" == Anton Vorontsov <avorontsov@ru.mvista.com> writes:
> 
> Hi,
> 
>  Anton> The advantages of this:
>  Anton> - Don't encourage legacy support;
>  Anton> - Less external symbols, less code to compile-in for !MPC832x_RDB
>  Anton>   platforms.
> 
> It's nice with your cleanups, but I wonder how to handle more
> complicated chip select handling than simply toggling a single gpio.
> 
> I have a board (or 2 actually, but they are similar in this regard)
> with a mpc8347 using SPI to a number of addon boards. For signal
> integrity reasons the SPI signals are routed to a MUX, so the chip
> select logic has to set the MUX in addition to controlling the CS line
> of the device.

So that's just a bit complicated GPIO controller. I believe you
should describe it in the device tree and use it's muxed lines as
usual GPIOs.

Something ling

muxed_pio: gpio-controller@.. {
	#gpio-cells = <2>;
	compatible = "..,<board>-muxed-gpios";
	reg = <...>;
	gpios = <...>; <- specify pure BLK1, BLK2, OPT1, OPT2 GPIOs
	gpio-controller;
};

And then,

> I've been using code like this since late 2007, but this patch
> ofcourse breaks it:
> 
> static void thinx_spi_activate_cs(u8 cs, u8 polarity)
> {
> 	static u8 old_cs = 255;
> 
> 	if (cs != old_cs) {
> 		/* mux setup (cs 2:1)*/
> 		gpio_set_value(gpio1 + GPIO_SPI_MUX_NOE, 1);
> 		gpio_set_value(gpio1 + GPIO_SPI_MUX_SEL0, cs&2);
> 		gpio_set_value(gpio1 + GPIO_SPI_MUX_SEL1, cs&4);
> 		gpio_set_value(gpio1 + GPIO_SPI_MUX_NOE, 0);
> 		old_cs = cs;
> 	}
> 
> 	switch (cs) {
> 	case 0: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL1, polarity); break;
> 	case 1: gpio_set_value(gpio1 + GPIO_SPI_CS_BKL2, polarity); break;
> 	case 2: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT1, polarity); break;
> 	case 3: gpio_set_value(gpio1 + GPIO_SPI_CS_OPT2, polarity); break;
> 	}
> }

^^^ Put this logic into the "<board>-muxed-gpios" GPIO controller
driver.

Then spi node would look like this:

spi-controller@.. {
	...
	gpios = <&muxed_pio cs1 0   /* muxed CS_BLK1 */
		 &muxed_pio cs2 0   /* muxed CS_BLK2 */
		 &muxed_pio cs3 0   /* muxed CS_OPT1 */
		 &muxed_pio cs4 0>; /* muxed CS_OPT2 */
};

Hope this helps,
diff mbox

Patch

diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
index e1e7eeb..567ded7 100644
--- a/arch/powerpc/platforms/83xx/mpc832x_rdb.c
+++ b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
@@ -20,6 +20,7 @@ 
 #include <linux/spi/mmc_spi.h>
 #include <linux/mmc/host.h>
 #include <linux/of_platform.h>
+#include <linux/fsl_devices.h>
 
 #include <asm/time.h>
 #include <asm/ipic.h>
@@ -39,6 +40,112 @@ 
 #endif
 
 #ifdef CONFIG_QUICC_ENGINE
+static int __init of_fsl_spi_probe(char *type, char *compatible, u32 sysclk,
+				   struct spi_board_info *board_infos,
+				   unsigned int num_board_infos,
+				   void (*cs_control)(struct spi_device *dev,
+						      bool on))
+{
+	struct device_node *np;
+	unsigned int i = 0;
+
+	for_each_compatible_node(np, type, compatible) {
+		int ret;
+		unsigned int j;
+		const void *prop;
+		struct resource res[2];
+		struct platform_device *pdev;
+		struct fsl_spi_platform_data pdata = {
+			.cs_control = cs_control,
+		};
+
+		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, "cell-index", NULL);
+		if (prop)
+			i = *(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)
+			continue;
+
+		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_add(pdev);
+		if (ret)
+			goto unreg;
+
+		goto next;
+unreg:
+		platform_device_del(pdev);
+err:
+		pr_err("%s: registration failed\n", np->full_name);
+next:
+		i++;
+	}
+
+	return i;
+}
+
+static int __init fsl_spi_init(struct spi_board_info *board_infos,
+			       unsigned int num_board_infos,
+			       void (*cs_control)(struct spi_device *spi,
+						  bool on))
+{
+	u32 sysclk = -1;
+	int ret;
+
+	/* SPI controller is either clocked from QE or SoC clock */
+	sysclk = get_brgfreq();
+	if (sysclk == -1) {
+		sysclk = fsl_get_sys_freq();
+		if (sysclk == -1)
+			return -ENODEV;
+	}
+
+	ret = of_fsl_spi_probe(NULL, "fsl,spi", sysclk, board_infos,
+			       num_board_infos, cs_control);
+	if (!ret)
+		of_fsl_spi_probe("spi", "fsl_spi", sysclk, board_infos,
+				 num_board_infos, cs_control);
+
+	return spi_register_board_info(board_infos, num_board_infos);
+}
+
 static void mpc83xx_spi_cs_control(struct spi_device *spi, bool on)
 {
 	pr_debug("%s %d %d\n", __func__, spi->chip_select, on);
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 0a545a9..18e49ef 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -413,113 +413,6 @@  err:
 
 arch_initcall(fsl_usb_of_init);
 
-static int __init of_fsl_spi_probe(char *type, char *compatible, u32 sysclk,
-				   struct spi_board_info *board_infos,
-				   unsigned int num_board_infos,
-				   void (*cs_control)(struct spi_device *dev,
-						      bool on))
-{
-	struct device_node *np;
-	unsigned int i = 0;
-
-	for_each_compatible_node(np, type, compatible) {
-		int ret;
-		unsigned int j;
-		const void *prop;
-		struct resource res[2];
-		struct platform_device *pdev;
-		struct fsl_spi_platform_data pdata = {
-			.cs_control = cs_control,
-		};
-
-		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, "cell-index", NULL);
-		if (prop)
-			i = *(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)
-			continue;
-
-		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_add(pdev);
-		if (ret)
-			goto unreg;
-
-		goto next;
-unreg:
-		platform_device_del(pdev);
-err:
-		pr_err("%s: registration failed\n", np->full_name);
-next:
-		i++;
-	}
-
-	return i;
-}
-
-int __init fsl_spi_init(struct spi_board_info *board_infos,
-			unsigned int num_board_infos,
-			void (*cs_control)(struct spi_device *spi, bool on))
-{
-	u32 sysclk = -1;
-	int ret;
-
-#ifdef CONFIG_QUICC_ENGINE
-	/* SPI controller is either clocked from QE or SoC clock */
-	sysclk = get_brgfreq();
-#endif
-	if (sysclk == -1) {
-		sysclk = fsl_get_sys_freq();
-		if (sysclk == -1)
-			return -ENODEV;
-	}
-
-	ret = of_fsl_spi_probe(NULL, "fsl,spi", sysclk, board_infos,
-			       num_board_infos, cs_control);
-	if (!ret)
-		of_fsl_spi_probe("spi", "fsl_spi", sysclk, board_infos,
-				 num_board_infos, cs_control);
-
-	return spi_register_board_info(board_infos, num_board_infos);
-}
-
 #if defined(CONFIG_PPC_85xx) || defined(CONFIG_PPC_86xx)
 static __be32 __iomem *rstcr;
 
diff --git a/arch/powerpc/sysdev/fsl_soc.h b/arch/powerpc/sysdev/fsl_soc.h
index b5f3456..42381bb 100644
--- a/arch/powerpc/sysdev/fsl_soc.h
+++ b/arch/powerpc/sysdev/fsl_soc.h
@@ -19,10 +19,6 @@  extern u32 fsl_get_sys_freq(void);
 struct spi_board_info;
 struct device_node;
 
-extern int fsl_spi_init(struct spi_board_info *board_infos,
-			unsigned int num_board_infos,
-			void (*cs_control)(struct spi_device *spi, bool on));
-
 extern void fsl_rstcr_restart(char *cmd);
 
 #if defined(CONFIG_FB_FSL_DIU) || defined(CONFIG_FB_FSL_DIU_MODULE)