mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE

Well, this comment is not about the patch but about the driver it self, I didn't see it before today.

So here's a few things I see from a quick glance at the code :

- Chaning port_config in the driver is just wrong ... you should _not_
do that. That should have been setup by the bootloader or at worse in the platform setup code.

- MPC52xx_PA(MPC52xx_PSCx_OFFSET(...)) ??? You should get that from the
resource of the platform_device. This macro is just there for early console stuff.

- You do read/write/modify operation on CDM shared register
(clk_enables) from a driver, you should have added something in common 52xx code to do theses with proper locking.
- You can get f_system from the device tree instead of just assuming
it's 512 MHz. It probably need to be done the same way it's done to find ipb_freq.
- Would have been nice to be able to somehow configure MCLK rather than
#define it

- I hope to remove all arch/ppc stuff by 2.6.23 if I can make the
cuimage stuff work in arch/powerpc so just including the platform code stuff for 1 kernel version ...

Quoted 3 lines · Sylvain Munaut

Well, this comment is not about the patch but about the driver it self, I didn't see it before today.

It merged earlier in the 2.6.22 cycle. If you don't have criticisms about the patch itself, I'll forward it for merging after I get at least an ack from Dragos.

Quoted 4 lines

- MPC52xx_PA(MPC52xx_PSCx_OFFSET(...)) ??? You should get that from the
resource of the platform_device. This macro is just there for early console stuff.

That PPC_MERGE stuff does look messy.

Quoted 9 lines

- You do read/write/modify operation on CDM shared register
(clk_enables) from a driver, you should have added something in common 52xx code to do theses with proper locking.
- You can get f_system from the device tree instead of just assuming
it's 512 MHz. It probably need to be done the same way it's done to find ipb_freq.
- Would have been nice to be able to somehow configure MCLK rather than
#define it

Best to use <linux/clk.h> for all of those, but it seems powerpc/ppc don't support those interfaces yet ... is there maybe a plan for resolving that issue?

Quoted 7 lines · David Brownell

Quoted 4 lines · Sylvain Munaut

Well, this comment is not about the patch but about the driver it self, I didn't see it before today.

Quoted 5 lines

It merged earlier in the 2.6.22 cycle. If you don't have criticisms about the patch itself, I'll forward it for merging after I get at least an ack from Dragos.

Yes, I saw when looking at the spi-devl archive. Would have been nice if the author though of cc-ing the ppc-embedded list ;)

The patch looks ok to me (and needed actually since as Domen pointed out, 52xx
has been replaced by 5200 in the device tree).
And cell-index has been added to know the psc id without dirty tricks.

Quoted 5 lines

Quoted 5 lines

- MPC52xx_PA(MPC52xx_PSCx_OFFSET(...)) ??? You should get that from the
resource of the platform_device. This macro is just there for early console stuff.

Quoted 3 lines

That PPC_MERGE stuff does look messy.

Yes, trying to support both in a driver is really not pretty. Once we can finally get rid of it I'll submit a patch to clear that out.

Fix mpc5200 PSC SPI driver to actually work for CONFIG_PPC_MERGE
- s/mpc52xx/mpc5200/, as this was changed in device tree some time ago
- fix spi id detection

Signed-off-by Domen Puncer <[email protected]>

---
 drivers/spi/mpc52xx_psc_spi.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Index: work-powerpc.git/drivers/spi/mpc52xx_psc_spi.c
===================================================================

drivers/spi/mpc52xx_psc_spi.c +11 -14

@@ -329,8 +329,8 @@ static int mpc52xx_psc_spi_port_config(i
 	int ret = 0;
 
 #if defined(CONFIG_PPC_MERGE)
-	cdm = mpc52xx_find_and_map("mpc52xx-cdm");
-	gpio = mpc52xx_find_and_map("mpc52xx-gpio");
+	cdm = mpc52xx_find_and_map("mpc5200-cdm");
+	gpio = mpc52xx_find_and_map("mpc5200-gpio");
 #else
 	cdm = ioremap(MPC52xx_PA(MPC52xx_CDM_OFFSET), MPC52xx_CDM_SIZE);
 	gpio = ioremap(MPC52xx_PA(MPC52xx_GPIO_OFFSET), MPC52xx_GPIO_SIZE);

@@ -445,9 +445,6 @@ static int __init mpc52xx_psc_spi_do_pro
 	struct spi_master *master;
 	int ret;
 
-	if (pdata == NULL)
-		return -ENODEV;
-
 	master = spi_alloc_master(dev, sizeof *mps);
 	if (master == NULL)
 		return -ENOMEM;

@@ -594,17 +591,17 @@ static int __init mpc52xx_psc_spi_of_pro
 	}
 	regaddr64 = of_translate_address(op->node, regaddr_p);
 
+	/* get PSC id (1..6, used by port_config) */
 	if (op->dev.platform_data == NULL) {
-		struct device_node *np;
-		int i = 0;
+		const u32 *psc_nump;
 
-		for_each_node_by_type(np, "spi") {
-			if (of_find_device_by_node(np) == op) {
-				id = i;
-				break;
-			}
-			i++;
+		psc_nump = of_get_property(op->node, "cell-index", NULL);
+		if (!psc_nump || *psc_nump > 5) {
+			printk(KERN_ERR "mpc52xx_psc_spi: Device node %s has invalid "
+					"cell-index property\n", op->node->full_name);
+			return -EINVAL;
 		}
+		id = *psc_nump + 1;
 	}
 
 	return mpc52xx_psc_spi_do_probe(&op->dev, (u32)regaddr64, (u32)size64,

@@ -617,7 +614,7 @@ static int __exit mpc52xx_psc_spi_of_rem
 }
 
 static struct of_device_id mpc52xx_psc_spi_of_match[] = {
-	{ .type = "spi", .compatible = "mpc52xx-psc-spi", },
+	{ .type = "spi", .compatible = "mpc5200-psc-spi", },
 	{},
 };
 

- Chaning port_config in the driver is just wrong ... you should _not_
do that. That should have been setup by the bootloader or at worse in the platform setup code.

- You do read/write/modify operation on CDM shared register
(clk_enables) from a driver, you should have added something in common 52xx code to do theses with proper locking.
- MPC52xx_PA(MPC52xx_PSCx_OFFSET(...)) ??? You should get that from the
resource of the platform_device. This macro is just there for early console stuff.
- You can get f_system from the device tree instead of just assuming
it's 512 MHz. It probably need to be done the same way it's done to find ipb_freq.
- Would have been nice to be able to somehow configure MCLK rather than
#define it

Well, this comment is not about the patch but about the driver it self, I didn't see it before today.
So here's a few things I see from a quick glance at the code :

- Chaning port_config in the driver is just wrong ... you should _not_
do that. That should have been setup by the bootloader or at worse in the platform setup code.
- You do read/write/modify operation on CDM shared register
(clk_enables) from a driver, you should have added something in common 52xx code to do theses with proper locking.
- MPC52xx_PA(MPC52xx_PSCx_OFFSET(...)) ??? You should get that from the
resource of the platform_device. This macro is just there for early console stuff.
- You can get f_system from the device tree instead of just assuming
it's 512 MHz. It probably need to be done the same way it's done to find ipb_freq.
- Would have been nice to be able to somehow configure MCLK rather than
#define it
- I hope to remove all arch/ppc stuff by 2.6.23 if I can make the
cuimage stuff work in arch/powerpc so just including the platform code stuff for 1 kernel version ...

Sylvain

Hi!

I tried to fix these issues, like suggested.

Quoted 4 lines · Sylvain Munaut

- Chaning port_config in the driver is just wrong ... you should _not_
do that. That should have been setup by the bootloader or at worse in the platform setup code.

[ trimming spi-devel and dbrownell from cc: ]

Setup gpio->port_config to match PSC's set in device tree.

Signed-off-by Domen Puncer <[email protected]>

---
 arch/powerpc/platforms/52xx/mpc52xx_common.c |   64 +++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

Index: work-powerpc.git/arch/powerpc/platforms/52xx/mpc52xx_common.c
===================================================================

arch/powerpc/platforms/52xx/mpc52xx_common.c +64 -0

@@ -75,6 +75,68 @@ mpc52xx_find_ipb_freq(struct device_node
 }
 EXPORT_SYMBOL(mpc52xx_find_ipb_freq);
 
+void __init
+mpc52xx_setup_port_config(void)
+{
+	struct mpc52xx_gpio __iomem *gpio;
+	struct device_node *np, *child = NULL;
+	u32 port_config;
+
+	/* Map zones */
+	gpio = mpc52xx_find_and_map("mpc5200-gpio");
+	if (!gpio) {
+		printk(KERN_ERR __FILE__ ": "
+			"Error while mapping GPIO register for port config. "
+			"Expect some abnormal behavior\n");
+		return;
+	}
+
+	/* Set port config */
+	port_config = in_be32(&gpio->port_config);
+
+	np = of_find_node_by_type(NULL, "soc");
+	if (!np) {
+		printk(KERN_ERR "%s: %i can't find node with type 'soc'\n",
+				__func__, __LINE__);
+		iounmap(gpio);
+		return;
+	}
+
+	while ((child = of_get_next_child(np, child))) {
+		if (of_device_is_compatible(child, "mpc5200-psc")) {
+			int ci = -1;
+			const int *pci;
+
+			pci = of_get_property(child, "cell-index", NULL);
+			if (pci)
+				ci = *pci;
+			if (ci < 0 || ci > 5 || ci == 3 || ci == 4) {
+				printk(KERN_ALERT "%s: %i psc node '%s' has invalid "
+						"cell-index: %i\n", __func__, __LINE__,
+						child->name, ci);
+				continue;
+			}
+
+			port_config &= ~(0x7 << ci*4);
+			if (strcmp(child->name, "ac97") == 0)
+				port_config |= 0x2 << ci*4;	/* AC97 functionality */
+			else if (strcmp(child->name, "serial") == 0)
+				port_config |= 0x5 << ci*4;	/* UARTe with CD */
+			else if (strcmp(child->name, "spi") == 0)
+				port_config |= 0x7 << ci*4;	/* CODEC with MCLK */
+			else
+				printk(KERN_ALERT "%s: %i psc node '%s' not handled\n",
+						__func__, __LINE__, child->name);
+		}
+	}
+	of_node_put(np);
+
+	pr_debug("port_config: old:%x new:%x\n",
+	         in_be32(&gpio->port_config), port_config);
+	out_be32(&gpio->port_config, port_config);
+
+	iounmap(gpio);
+}
 
 void __init
 mpc52xx_setup_cpu(void)

@@ -114,6 +176,8 @@ mpc52xx_setup_cpu(void)
 unmap_regs:
 	if (cdm) iounmap(cdm);
 	if (xlb) iounmap(xlb);
+
+	mpc52xx_setup_port_config();
 }
 
 void __init

For clk.h, it does seem quite some code, compared what's there currently.

drivers/spi/mpc52xx_psc_spi.c +10 -0

@@ -18,6 +18,7 @@
 
 #if defined(CONFIG_PPC_MERGE)
 #include <asm/of_platform.h>
+#include <linux/clk.h>
 #else
 #include <linux/platform_device.h>
 #endif

@@ -106,13 +107,52 @@ static void mpc52xx_psc_spi_activate_cs(
 	/* Set clock frequency and bits per word
 	 * Because psc->ccr is defined as 16bit register instead of 32bit
 	 * just set the lower byte of BitClkDiv
+	 * Because BitClkDiv is 8-bit on mpc5200. Lets stay compatible.
 	 */
+#if defined(CONFIG_PPC_MERGE)
 	ccr = in_be16(&psc->ccr);
 	ccr &= 0xFF00;
+	{
+		u8 bitclkdiv = 2;	/* minimum bitclkdiv */
+		int speed = cs->speed_hz ? cs->speed_hz : 1000000;
+		char clk_name[10];
+		struct clk *clk;	// TODO into mps, clk_get, clk_put
+		int mclk;
+		int system;

drivers/spi/mpc52xx_psc_spi.c +17 -0

+		if (!clk)
+			dev_err(&spi->dev, "couldn't get %s clock\n", clk_name);
+
+		system = clk_get_rate(clk_get_parent(clk));	// TODO, checking, refcounting
+		mclk = speed * (bitclkdiv+1);
+		if (system/mclk > 512) { /* bigger than mclkdiv */
+			bitclkdiv = system/512/speed;
+			mclk = speed * (bitclkdiv+1);
+		}
+
+		if (clk_set_rate(clk, mclk))
+			dev_err(&spi->dev, "couldn't set %s's rate\n", clk_name);
+
+		dev_info(&spi->dev, "clock: wanted: %i, got: %li\n", speed,
+				clk_round_rate(clk, mclk) / (bitclkdiv+1));
+
+		ccr |= bitclkdiv;

NACK

Not at all what I meant ...

We should not modify port config in any automatic way, just let the boot loader set it up.
If it known not to do it propertly, then in the platform file (lite5200b, efika, ...)
adjust port config according to what's really on the board.

Quoted 1 line · Domen Puncer

For clk.h, it does seem quite some code, compared what's there currently.

You're using it wrong then ...

Quoted 37 lines

drivers/spi/mpc52xx_psc_spi.c +19 -0

@@ -18,6 +18,7 @@
 
 #if defined(CONFIG_PPC_MERGE)
 #include <asm/of_platform.h>
+#include <linux/clk.h>
 #else
 #include <linux/platform_device.h>
 #endif

@@ -106,13 +107,52 @@ static void mpc52xx_psc_spi_activate_cs(
 	/* Set clock frequency and bits per word
 	 * Because psc->ccr is defined as 16bit register instead of 32bit
 	 * just set the lower byte of BitClkDiv
+	 * Because BitClkDiv is 8-bit on mpc5200. Lets stay compatible.
 	 */
+#if defined(CONFIG_PPC_MERGE)
 	ccr = in_be16(&psc->ccr);
 	ccr &= 0xFF00;
+	{
+		u8 bitclkdiv = 2;	/* minimum bitclkdiv */
+		int speed = cs->speed_hz ? cs->speed_hz : 1000000;
+		char clk_name[10];
+		struct clk *clk;	// TODO into mps, clk_get, clk_put
+		int mclk;
+		int system;
+		/*
+		 * pscclk = mclk/(bitclkdiv+1))		bitclkdiv is 8-bit, >= 2
+		 * mclk = fsys/(mclkdiv+1)		mclkdiv is 9-bit, >= 1
+		 * as mclkdiv has higher precision, we want is as big as possible
+		 * => we get < 0.002*clock error
+		 */
+
+		snprintf(clk_name, 10, "psc%i_mclk", spi->master->bus_num);
+

drivers/spi/mpc52xx_psc_spi.c static void mpc52xx_psc_spi_activate_cs( · line 131

Quoted 1 line

drivers/spi/mpc52xx_psc_spi.c +1 -0

+		clk = clk_get(&spi->dev, clk_name);

Don't expect spi->dev to have a clock. Instead, the host controller has a clock, which would be looked up the probe() routine, then released in remove(): clock = clk_get(dev, "clockname"), with the clock framework using "dev" to identify which clock it returns. (That is, the function clock for all SPI controllers would have the same name ... not for example "spi1_fclk", "spi2_fclk", etc. but instead "spi_fclk".)

Quoted 19 lines

drivers/spi/mpc52xx_psc_spi.c +17 -0

+		if (!clk)
+			dev_err(&spi->dev, "couldn't get %s clock\n", clk_name);
+
+		system = clk_get_rate(clk_get_parent(clk));	// TODO, checking, refcounting
+		mclk = speed * (bitclkdiv+1);
+		if (system/mclk > 512) { /* bigger than mclkdiv */
+			bitclkdiv = system/512/speed;
+			mclk = speed * (bitclkdiv+1);
+		}
+
+		if (clk_set_rate(clk, mclk))
+			dev_err(&spi->dev, "couldn't set %s's rate\n", clk_name);
+
+		dev_info(&spi->dev, "clock: wanted: %i, got: %li\n", speed,
+				clk_round_rate(clk, mclk) / (bitclkdiv+1));
+
+		ccr |= bitclkdiv;

drivers/spi/mpc52xx_psc_spi.c static void mpc52xx_psc_spi_activate_cs( · line 158

Quoted 10 lines

drivers/spi/mpc52xx_psc_spi.c +3 -0

+	}
+#else
 	if (cs->speed_hz)
 		ccr |= (MCLK / cs->speed_hz - 1) & 0xFF;
 	else /* by default SPI Clk 1MHz */
 		ccr |= (MCLK / 1000000 - 1) & 0xFF;
+#endif
 	out_be16(&psc->ccr, ccr);
 	mps->bits_per_word = cs->bits_per_word;
 

\