[PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE

[PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE

[Domen Puncer]

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 <domen.puncer@telargo.com>

---
 drivers/spi/mpc52xx_psc_spi.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 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
@@ -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", },
 	{},
 };
 

Re: [PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE

[Sylvain Munaut]

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

Re: [PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE

[David Brownell]

On Wednesday 16 May 2007, Sylvain Munaut wrote:
> 
> 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.


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

And that's where the worst of the magic numerology lives too.
If that's pinmux style setup, agreed -- it doesn't belong there.

As a rule, I tell folk that Linux shouldn't rely on the boot
loader to have done much more than get Linux loaded.  The way
board development works, bootloaders usually freeze well before
all the setup nuances Linux cares about have been resolved.


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


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


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

This originally started out as ppc then moved to powerpc ... agreed,
it would be nicer to need only one kind of bus glue.

- Dave

Re: [PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE

[Sylvain Munaut]

David Brownell wrote:
> On Wednesday 16 May 2007, Sylvain Munaut wrote:
>   
>> 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.
>   
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.

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

>>  - 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?
>   
Mmm, I wasn't aware of that interface, I'll look into that. Thanks.


    Sylvain

Re: [PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE

[Dragos Carp]

Sylvain Munaut wrote:
> David Brownell wrote:
>   
>> On Wednesday 16 May 2007, Sylvain Munaut wrote:
>>   
>>     
>>> 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.
>>   
>>     
> 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 ;)
>
>   
Sorry about not cc-ing ppc-embedded, this was my first kernel patch, I'm 
not very familiar with the submitting process.
> 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.
>
>   
The patch looks ok. I'm currently still using 2.6.20 which still has 
52xx. I compiled it for 2.6.22-rc2 and of course this problem was not 
caught by the compiler. Unfortunately for the board that I'm using 
(MPC5200-tiny from Pythec),  there is no adaptation yet for  2.6.21.
>>>  - 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.
>>   
>>     
> 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.
>
>   
A clean up for PPC_MERGE will be nice. There are still some magic 
numbers used in the driver (SICR_SIM_CODEC8, SICR_GENCLK, etc..) that 
should be defined in mpc52xx_psc.h.

Dragos

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

[Domen Puncer]

Hi!

I tried to fix these issues, like suggested.

On 16/05/07 10:19 +0200, Sylvain Munaut wrote:
>  - 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 <domen.puncer@telargo.com>

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

Index: work-powerpc.git/arch/powerpc/platforms/52xx/mpc52xx_common.c
===================================================================
--- work-powerpc.git.orig/arch/powerpc/platforms/52xx/mpc52xx_common.c
+++ work-powerpc.git/arch/powerpc/platforms/52xx/mpc52xx_common.c
@@ -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

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

[Sylvain Munaut]

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.

    Sylvain



Domen Puncer wrote:
> Hi!
>
> I tried to fix these issues, like suggested.
>
> On 16/05/07 10:19 +0200, Sylvain Munaut wrote:
>   
>>  - 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 <domen.puncer@telargo.com>
>
> ---
>  arch/powerpc/platforms/52xx/mpc52xx_common.c |   64 +++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> Index: work-powerpc.git/arch/powerpc/platforms/52xx/mpc52xx_common.c
> ===================================================================
> --- work-powerpc.git.orig/arch/powerpc/platforms/52xx/mpc52xx_common.c
> +++ work-powerpc.git/arch/powerpc/platforms/52xx/mpc52xx_common.c
> @@ -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
>
>   

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

[Grant Likely]

On 5/25/07, Sylvain Munaut <tnt@246tnt.com> wrote:
> 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.

I second that NACK.  Drivers must *never* touch port config.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

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

[Domen Puncer]

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

[ trimming spi-devel from cc: ]


Use clk.h interface for mpc52xx.
Parse device tree for Fsystem, or leave it default 528 (yes, not 512).

Currently only psc_mclks are defined.


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

---
 arch/powerpc/platforms/52xx/Makefile |    2 
 arch/powerpc/platforms/52xx/clock.c  |  311 +++++++++++++++++++++++++++++++++++
 2 files changed, 312 insertions(+), 1 deletion(-)

Index: work-powerpc.git/arch/powerpc/platforms/52xx/Makefile
===================================================================
--- work-powerpc.git.orig/arch/powerpc/platforms/52xx/Makefile
+++ work-powerpc.git/arch/powerpc/platforms/52xx/Makefile
@@ -2,7 +2,7 @@
 # Makefile for 52xx based boards
 #
 ifeq ($(CONFIG_PPC_MERGE),y)
-obj-y				+= mpc52xx_pic.o mpc52xx_common.o
+obj-y				+= mpc52xx_pic.o mpc52xx_common.o clock.o
 obj-$(CONFIG_PCI)		+= mpc52xx_pci.o
 obj-$(CONFIG_PPC_BESTCOMM) 	+= bestcomm.o
 obj-$(CONFIG_FEC_MPC52xx)	+= sdma_fec_rx_task.o sdma_fec_tx_task.o fec.o
Index: work-powerpc.git/arch/powerpc/platforms/52xx/clock.c
===================================================================
--- /dev/null
+++ work-powerpc.git/arch/powerpc/platforms/52xx/clock.c
@@ -0,0 +1,311 @@
+/*
+ * arch/powerpc/platforms/52xx/clock.c
+ * based on linux/arch/arm/mach-at91/clock.c
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/errno.h>
+#include <linux/spinlock.h>
+#include <linux/clk.h>
+
+#include <asm/io.h>
+#include <asm/mpc52xx.h>
+
+
+struct clk {
+	struct list_head node;
+	const char	*name;		/* unique clock name */
+	unsigned long	rate_hz;
+	struct clk	*parent;
+	void		(*mode)(struct clk *, int);
+	u16		users;
+	int		cdm_id;		/* == bit number in datasheet, or 0 */
+	long		(*set_rate)(struct clk *, unsigned long rate, int round_only);
+};
+
+#define CDM_ID_PSC3 24
+#define CDM_ID_PSC2 25
+#define CDM_ID_PSC1 26
+#define CDM_ID_PSC6 27
+
+long psc_set_rate(struct clk *clk, unsigned long rate, int round_only);
+
+static struct clk clk_system = {
+	.name		= "system",
+	.rate_hz	= 528000000,	/* default if there's no system-frequency in dts */
+	.users		= 1,		/* always on */
+};
+static struct clk clk_psc1 = {
+	.name		= "psc1_mclk",
+	.cdm_id		= CDM_ID_PSC1,
+	.parent		= &clk_system,
+	.set_rate	= psc_set_rate,
+};
+static struct clk clk_psc2 = {
+	.name		= "psc2_mclk",
+	.cdm_id		= CDM_ID_PSC2,
+	.parent		= &clk_system,
+	.set_rate	= psc_set_rate,
+};
+static struct clk clk_psc3 = {
+	.name		= "psc3_mclk",
+	.cdm_id		= CDM_ID_PSC3,
+	.parent		= &clk_system,
+	.set_rate	= psc_set_rate,
+};
+static struct clk clk_psc6 = {
+	.name		= "psc6_mclk",
+	.cdm_id		= CDM_ID_PSC6,
+	.parent		= &clk_system,
+	.set_rate	= psc_set_rate,
+};
+
+
+
+static LIST_HEAD(clocks);
+static DEFINE_SPINLOCK(clk_lock);
+
+static struct mpc52xx_cdm __iomem *cdm;
+static DEFINE_SPINLOCK(cdm_lock);
+
+
+long psc_set_rate(struct clk *clk, unsigned long rate, int round_only)
+{
+	u16 mclkdiv;
+	u16 __iomem *divreg;
+
+	/* pick a divider that will get the closest clock */
+	mclkdiv = (clk->parent->rate_hz + rate/2) / rate - 1;
+
+	/* trim to closest possible. or should we return an error? */
+	mclkdiv = min(mclkdiv, (u16)0x1ff);
+	mclkdiv = max(mclkdiv, (u16)1);
+
+	rate = clk->parent->rate_hz / (mclkdiv + 1);
+	mclkdiv |= 0x8000;	/* enable (this is not clk_enable!) */
+
+	if (round_only)
+		return rate;
+
+	if (clk->cdm_id == CDM_ID_PSC1)
+		divreg = &cdm->mclken_div_psc1;
+	else if (clk->cdm_id == CDM_ID_PSC2)
+		divreg = &cdm->mclken_div_psc2;
+	else if (clk->cdm_id == CDM_ID_PSC3)
+		divreg = &cdm->mclken_div_psc3;
+	else if (clk->cdm_id == CDM_ID_PSC6)
+		divreg = &cdm->mclken_div_psc6;
+	else
+		return -ENODEV;
+
+	out_be16(divreg, mclkdiv);
+
+	return 0;
+}
+
+/* clocks cannot be de-registered no refcounting necessary */
+struct clk *clk_get(struct device *dev, const char *id)
+{
+	struct clk *clk;
+
+	list_for_each_entry(clk, &clocks, node) {
+		if (strcmp(id, clk->name) == 0)
+			return clk;
+	}
+
+	return ERR_PTR(-ENOENT);
+}
+EXPORT_SYMBOL(clk_get);
+
+void clk_put(struct clk *clk)
+{
+}
+EXPORT_SYMBOL(clk_put);
+
+
+static void clk_mode_cdm(int cdm_id, int enabled)
+{
+	unsigned long flags;
+	u32 clk_enables;
+
+	if (cdm_id < 12 || cdm_id > 31) {
+		printk(KERN_ERR "%s: %i invalid cdm_id: %i\n", __func__, __LINE__, cdm_id);
+		return;
+	}
+
+	spin_lock_irqsave(&cdm_lock, flags);
+	clk_enables = in_be32(&cdm->clk_enables);
+	if (enabled)
+		clk_enables |= 1 << (31-cdm_id);
+	else
+		clk_enables &= ~(1 << (31-cdm_id));
+
+	out_be32(&cdm->clk_enables, clk_enables);
+	spin_unlock_irqrestore(&cdm_lock, flags);
+}
+
+static void __clk_enable(struct clk *clk)
+{
+	if (clk->parent)
+		__clk_enable(clk->parent);
+	if (clk->users++ == 0 && clk->mode) {
+		if (clk->cdm_id)
+			clk_mode_cdm(clk->cdm_id, 1);
+		else
+			clk->mode(clk, 1);
+	}
+}
+
+int clk_enable(struct clk *clk)
+{
+	unsigned long	flags;
+
+	spin_lock_irqsave(&clk_lock, flags);
+	__clk_enable(clk);
+	spin_unlock_irqrestore(&clk_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL(clk_enable);
+
+static void __clk_disable(struct clk *clk)
+{
+	BUG_ON(clk->users == 0);
+	if (--clk->users == 0 && clk->mode) {
+		if (clk->cdm_id)
+			clk_mode_cdm(clk->cdm_id, 0);
+		else
+			clk->mode(clk, 0);
+	}
+	if (clk->parent)
+		__clk_disable(clk->parent);
+}
+
+void clk_disable(struct clk *clk)
+{
+	unsigned long	flags;
+
+	spin_lock_irqsave(&clk_lock, flags);
+	__clk_disable(clk);
+	spin_unlock_irqrestore(&clk_lock, flags);
+}
+EXPORT_SYMBOL(clk_disable);
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	unsigned long	flags;
+	unsigned long	rate;
+
+	spin_lock_irqsave(&clk_lock, flags);
+	for (;;) {
+		rate = clk->rate_hz;
+		if (rate || !clk->parent)
+			break;
+		clk = clk->parent;
+	}
+	spin_unlock_irqrestore(&clk_lock, flags);
+	return rate;
+}
+EXPORT_SYMBOL(clk_get_rate);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	unsigned long flags;
+	long ret;
+
+	if (!clk->set_rate)
+		return -EINVAL;
+
+	spin_lock_irqsave(&clk_lock, flags);
+	ret = clk->set_rate(clk, rate, 1);
+	spin_unlock_irqrestore(&clk_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	unsigned long flags;
+	int ret;
+
+	if (!clk->set_rate)
+		return -EINVAL;
+	if (clk->users)
+		return -EBUSY;
+
+	spin_lock_irqsave(&clk_lock, flags);
+	clk->rate_hz = clk->set_rate(clk, rate, 1);
+	ret = clk->set_rate(clk, rate, 0);
+	spin_unlock_irqrestore(&clk_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	return clk->parent;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	unsigned long	flags;
+
+	if (clk->users)
+		return -EBUSY;
+	spin_lock_irqsave(&clk_lock, flags);
+
+	clk->rate_hz = parent->rate_hz;
+	clk->parent = parent;
+
+	spin_unlock_irqrestore(&clk_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+
+
+static struct clk *const mpc5200_clocks[] __initdata = {
+	&clk_system,
+	&clk_psc1,
+	&clk_psc2,
+	&clk_psc3,
+	&clk_psc6,
+};
+
+static int __init mpc5200_clock_init(void)
+{
+	int i;
+	unsigned int fsystem = 0;
+	struct device_node *np;
+
+	/* get clk_system rate from device tree */
+	np = of_find_node_by_type(NULL, "soc");
+	if (np) {
+		const unsigned int *fp =
+			of_get_property(np, "system-frequency", NULL);
+		if (fp)
+			fsystem = *fp;
+		of_node_put(np);
+	}
+	if (fsystem)
+		clk_system.rate_hz = fsystem;
+
+	cdm = mpc52xx_find_and_map("mpc5200-cdm");
+	if (!cdm) {
+		printk(KERN_ERR "%s: %i couldn't map mpc5200-cdm\n", __func__, __LINE__);
+		return -EFAULT;
+	}
+
+	/* Register the PMC's standard clocks */
+	for (i = 0; i < ARRAY_SIZE(mpc5200_clocks); i++)
+		list_add_tail(&mpc5200_clocks[i]->node, &clocks);
+
+	return 0;
+}
+
+arch_initcall(mpc5200_clock_init);

[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@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;
 }

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

[David Brownell]

On Friday 25 May 2007, Domen Puncer wrote:

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

You're using it wrong then ...


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

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".)


> +		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;
>  
\

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

[Domen Puncer]

On 25/05/07 09:34 -0700, David Brownell wrote:
> On Friday 25 May 2007, Domen Puncer wrote:
> 
> > For clk.h, it does seem quite some code, compared what's there currently.
> 
> You're using it wrong then ...

I meant this + the clock.c part.

> 
> 
> > --- 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);
> 
> 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".)


Aha, so that's what struct dev* is for. :-)
Now I see two options:
- create something like at91_clock_associate(), and call it from
  _probe (of_devices are created automatically from device tree,
  so we can't call it the way at91_clock_associate is).
- in clk_get() call dev_get_drvdata(dev), cast it to spi_master *,
  _hope_ it really is that, use bus_id.

Sure there must be a third, right one?


	Domen
> 
> 
> > +		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;
> >  
> \

Re: [PATCH] mpc52xx_psc_spi: fix it for CONFIG_PPC_MERGE

[Dragos Carp]

Acked-by: Dragos Carp <dragos.carp@toptica.com>

Domen Puncer wrote:
> 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 <domen.puncer@telargo.com>
>
> ---
>  drivers/spi/mpc52xx_psc_spi.c |   25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 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
> @@ -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", },
>  	{},
>  };
>  
>
>
>   


-- 

Dragos Carp
Research & Development

Phone: +49 89 85837-159, Fax: +49 89 85837-200
Address: TOPTICA Photonics AG, Lochhamer Schlag 19, D-82166 Graefelfing, Germany

TOPTICA – A Passion for Precision
TOPTICA Photonics AG, Registered Office: 82166 Graefelfing, Germany
Companies' Register: Amtsgericht München, HRB 137368
Executive Directors: Dr. Wilhelm Kaenders, Dr. Thomas Weber
Chairman of the Supervisory Board: Dr. Kristian Hohla
www.toptica.com