[RFC 3/3] Re: [PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE

[Domen Puncer]

On 16/05/07 10:19 +0200, Sylvain Munaut wrote:
> 
> 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

[ trimming spi-devel from cc: ]

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

Comments?


Use previous two patches (port_config, clk.h).


Signed-off-by: Domen Puncer <domen.puncer at telargo.com>

---
 drivers/spi/mpc52xx_psc_spi.c |   49 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

Index: work-powerpc.git/drivers/spi/mpc52xx_psc_spi.c
===================================================================
--- work-powerpc.git.orig/drivers/spi/mpc52xx_psc_spi.c
+++ work-powerpc.git/drivers/spi/mpc52xx_psc_spi.c
@@ -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);
+
+		clk = clk_get(&spi->dev, clk_name);
+		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;
+	}
+#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;
 
@@ -328,13 +368,9 @@ static int mpc52xx_psc_spi_port_config(i
 	u32 mclken_div;
 	int ret = 0;
 
-#if defined(CONFIG_PPC_MERGE)
-	cdm = mpc52xx_find_and_map("mpc5200-cdm");
-	gpio = mpc52xx_find_and_map("mpc5200-gpio");
-#else
+#if !defined(CONFIG_PPC_MERGE)
 	cdm = ioremap(MPC52xx_PA(MPC52xx_CDM_OFFSET), MPC52xx_CDM_SIZE);
 	gpio = ioremap(MPC52xx_PA(MPC52xx_GPIO_OFFSET), MPC52xx_GPIO_SIZE);
-#endif
 	if (!cdm || !gpio) {
 		printk(KERN_ERR "Error mapping CDM/GPIO\n");
 		ret = -EFAULT;
@@ -390,6 +426,7 @@ static int mpc52xx_psc_spi_port_config(i
 		ret = -EINVAL;
 		goto unmap_regs;
 	}
+#endif
 
 	/* Reset the PSC into a known state */
 	out_8(&psc->command, MPC52xx_PSC_RST_RX);
@@ -413,11 +450,13 @@ static int mpc52xx_psc_spi_port_config(i
 
 	mps->bits_per_word = 8;
 
+#if !defined(CONFIG_PPC_MERGE)
 unmap_regs:
 	if (cdm)
 		iounmap(cdm);
 	if (gpio)
 		iounmap(gpio);
+#endif
 
 	return ret;
 }