[PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Mark Zhan]

This patch makes 'cpm_uart_of_init' be able to parses SMC serial device node in DTS

Signed-off-by: Mark Zhan <rongkai.zhan@windriver.com>
---
  arch/powerpc/sysdev/fsl_soc.c |   37 ++++++++++++++++++++-----------------
  1 file changed, 20 insertions(+), 17 deletions(-)

Index: linux-powerpc-2.6.x/arch/powerpc/sysdev/fsl_soc.c
===================================================================
--- linux-powerpc-2.6.x.orig/arch/powerpc/sysdev/fsl_soc.c	2007-07-16 15:52:17.000000000 +0800
+++ linux-powerpc-2.6.x/arch/powerpc/sysdev/fsl_soc.c	2007-07-16 16:59:31.000000000 +0800
@@ -757,8 +757,8 @@

  arch_initcall(fs_enet_of_init);

-static const char scc_regs[] = "regs";
-static const char scc_pram[] = "pram";
+static const char cpm_uart_regs[] = "regs";
+static const char cpm_uart_pram[] = "pram";

  static int __init cpm_uart_of_init(void)
  {
@@ -775,6 +775,7 @@
  		const int *id;
  		const char *model;

+		cpm_uart_dev = NULL;
  		memset(r, 0, sizeof(r));
  		memset(&cpm_uart_data, 0, sizeof(cpm_uart_data));

@@ -782,31 +783,35 @@
  		if (ret)
  			goto err;

-		r[0].name = scc_regs;
+		r[0].name = cpm_uart_regs;

  		ret = of_address_to_resource(np, 1, &r[1]);
  		if (ret)
  			goto err;
-		r[1].name = scc_pram;
+		r[1].name = cpm_uart_pram;

  		of_irq_to_resource(np, 0, &r[2]);

-		cpm_uart_dev =
-		    platform_device_register_simple("fsl-cpm-scc:uart", i, &r[0], 3);
-
-		if (IS_ERR(cpm_uart_dev)) {
-			ret = PTR_ERR(cpm_uart_dev);
-			goto err;
-		}
-
  		id = of_get_property(np, "device-id", NULL);
  		cpm_uart_data.fs_no = *id;

  		model = of_get_property(np, "model", NULL);
  		strcpy(cpm_uart_data.fs_type, model);

-		cpm_uart_data.uart_clk = ppc_proc_freq;
+		if (strstr(model, "SMC")) {
+			cpm_uart_dev = platform_device_register_simple("fsl-cpm-smc:uart",
+							i, &r[0], 3);
+		} else if (strstr(model, "SCC")) {
+			cpm_uart_dev = platform_device_register_simple("fsl-cpm-scc:uart",
+							i, &r[0], 3);
+		}

+		if (IS_ERR(cpm_uart_dev)) {
+			ret = PTR_ERR(cpm_uart_dev);
+			goto err;
+		}
+
+		cpm_uart_data.uart_clk = ppc_proc_freq;
  		cpm_uart_data.tx_num_fifo = 4;
  		cpm_uart_data.tx_buf_size = 32;
  		cpm_uart_data.rx_num_fifo = 4;
@@ -816,10 +821,8 @@
  		cpm_uart_data.clk_tx = *((u32 *)of_get_property(np,
  						"tx-clock", NULL));

-		ret =
-		    platform_device_add_data(cpm_uart_dev, &cpm_uart_data,
-					     sizeof(struct
-						    fs_uart_platform_info));
+		ret = platform_device_add_data(cpm_uart_dev, &cpm_uart_data,
+					sizeof(struct fs_uart_platform_info));
  		if (ret)
  			goto unreg;
  	}

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Arnd Bergmann]

T24gTW9uZGF5IDE2IEp1bHkgMjAwNywgTWFyayBaaGFuIHdyb3RlOgo+IC2goKCgoKCgoKCgoKCg
oKBjcG1fdWFydF9kYXRhLnVhcnRfY2xrID0gcHBjX3Byb2NfZnJlcTsKPiAroKCgoKCgoKCgoKCg
oKCgaWYgKHN0cnN0cihtb2RlbCwgIlNNQyIpKSB7Cj4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
Y3BtX3VhcnRfZGV2ID0gcGxhdGZvcm1fZGV2aWNlX3JlZ2lzdGVyX3NpbXBsZSgiZnNsLWNwbS1z
bWM6dWFydCIsCj4gK6CgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKCgoKCgoKCgoKBpLCAmclswXSwgMyk7Cj4gK6CgoKCgoKCgoKCgoKCgoH0gZWxzZSBpZiAoc3Ry
c3RyKG1vZGVsLCAiU0NDIikpIHsKPiAroKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBjcG1fdWFydF9k
ZXYgPSBwbGF0Zm9ybV9kZXZpY2VfcmVnaXN0ZXJfc2ltcGxlKCJmc2wtY3BtLXNjYzp1YXJ0IiwK
PiAroKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oGksICZyWzBdLCAzKTsKPiAroKCgoKCgoKCgoKCgoKCgfQo+IAoKWW91IHNob3VsZCBwcm9iYWJs
eSB1c2Ugb2ZfZGV2aWNlX2lzX2NvbXBhdGlibGUoKSB0byBjaGVjawppZiBhIGdpdmVuIGRldmlj
ZSBjYW4gYmUgdXNlZCBieSBhIHBhcnRpY3VsYXIgZHJpdmVyLgoKCUFybmQgPD48Cg==

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Mark Zhan]

Hello Arnd,

I think, the function of_find_compatible_node(), which is called in the
for loop, has already done that. So definitely, no need to call
of_device_is_compatible() any more.

Thanks
Mark Zhan
On Tue, 2007-07-17 at 03:02 +0200, Arnd Bergmann wrote:
> On Monday 16 July 2007, Mark Zhan wrote:
> > -               cpm_uart_data.uart_clk = ppc_proc_freq;
> > +               if (strstr(model, "SMC")) {
> > +                       cpm_uart_dev = platform_device_register_simple("fsl-cpm-smc:uart",
> > +                                                       i, &r[0], 3);
> > +               } else if (strstr(model, "SCC")) {
> > +                       cpm_uart_dev = platform_device_register_simple("fsl-cpm-scc:uart",
> > +                                                       i, &r[0], 3);
> > +               }
> > 
> 
> You should probably use of_device_is_compatible() to check
> if a given device can be used by a particular driver.
> 
> 	Arnd <><

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Arnd Bergmann]

On Tuesday 17 July 2007, Mark Zhan wrote:
> On Tue, 2007-07-17 at 03:02 +0200, Arnd Bergmann wrote:
> > On Monday 16 July 2007, Mark Zhan wrote:
> > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 cpm_uart_data.uart_clk =3D ppc_proc_fre=
q;
> > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (strstr(model, "SMC")) {
> > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cpm_uart_dev =3D platfo=
rm_device_register_simple("fsl-cpm-smc:uart",
> > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 i, &r[0], 3);
> > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else if (strstr(model, "SCC")) {
> > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cpm_uart_dev =3D platfo=
rm_device_register_simple("fsl-cpm-scc:uart",
> > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 i, &r[0], 3);
> > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> >
> > You should probably use of_device_is_compatible() to check
> > if a given device can be used by a particular driver.
>
> I think, the function of_find_compatible_node(), which is called in the
> for loop, has already done that. So definitely, no need to call
> of_device_is_compatible() any more.

It's a little more complicated than that. The "compatible" property should
indicate the exact interface of that device, so the "model" does not really
matter here. I don't know the difference between smc and scc, but if you
need to register them as different pplatform devices, they should
normally also have different names in "compatible", possibly in addition
to the existing one.

If the compatible property contains "fsl,cpm-smc\0cpm_uart", you can scan f=
or
either of them. The loop will iterate over all cpm_uart compatible devices,
while the later test will look for an fsl,cpm-smc compatible device.

	Arnd <><

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Arnd Bergmann]

On Tuesday 17 July 2007, Mark Zhan wrote:
> Yes, basically I agree what you say. That should be the right way. 
> 
> but, the current situation is that: in all DTS files that are using smc
> or scc as uart device, all device node definitions have the same
> "compatible" property -- "cpm_uart"
> 
> So what I do here is just following the upstream source tree.
> 

Well, AFAICS, all of them currently use scc. The 8xx platforms don't
even build correctly in the mainline kernel, so I guess it would
be good to change them to also list fsl,cpm-smc in the compatible
property.

	Arnd <><

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Mark Zhan]

On Tue, 2007-07-17 at 14:06 +0200, Arnd Bergmann wrote:
> On Tuesday 17 July 2007, Mark Zhan wrote:
> > On Tue, 2007-07-17 at 03:02 +0200, Arnd Bergmann wrote:
> > > On Monday 16 July 2007, Mark Zhan wrote:
> > > > -               cpm_uart_data.uart_clk = ppc_proc_freq;
> > > > +               if (strstr(model, "SMC")) {
> > > > +                       cpm_uart_dev = platform_device_register_simple("fsl-cpm-smc:uart",
> > > > +                                                       i, &r[0], 3);
> > > > +               } else if (strstr(model, "SCC")) {
> > > > +                       cpm_uart_dev = platform_device_register_simple("fsl-cpm-scc:uart",
> > > > +                                                       i, &r[0], 3);
> > > > +               }
> > >
> > > You should probably use of_device_is_compatible() to check
> > > if a given device can be used by a particular driver.
> >
> > I think, the function of_find_compatible_node(), which is called in the
> > for loop, has already done that. So definitely, no need to call
> > of_device_is_compatible() any more.
> 
> It's a little more complicated than that. The "compatible" property should
> indicate the exact interface of that device, so the "model" does not really
> matter here. I don't know the difference between smc and scc, but if you
> need to register them as different pplatform devices, they should
> normally also have different names in "compatible", possibly in addition
> to the existing one.
> 
> If the compatible property contains "fsl,cpm-smc\0cpm_uart", you can scan for
> either of them. The loop will iterate over all cpm_uart compatible devices,
> while the later test will look for an fsl,cpm-smc compatible device.

Arnd,

Yes, basically I agree what you say. That should be the right way. 

but, the current situation is that: in all DTS files that are using smc
or scc as uart device, all device node definitions have the same
"compatible" property -- "cpm_uart"

So what I do here is just following the upstream source tree.

Thanks
Mark Zhan

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Arnd Bergmann]

On Tuesday 17 July 2007, Mark Zhan wrote:
> 
> > Well, AFAICS, all of them currently use scc. The 8xx platforms don't
> > even build correctly in the mainline kernel, so I guess it would
> > be good to change them to also list fsl,cpm-smc in the compatible
> > property.
> 
> That probably could be done by another series of patches, and I really
> don't want to introduce those kinds of patches in my BSP patches. I
> don't know if the 8xx/82xx series patches from Scott Wood will cover
> this issue or not.

I guess he can best answer that himself. Scott, what are your plans
to deal with the different types of cpm ports?

	Arnd <><

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Arnd Bergmann]

On Tuesday 17 July 2007, Segher Boessenkool wrote:
> > If the compatible property contains "fsl,cpm-smc\0cpm_uart", you =A0
> > can scan for
> > either of them. The loop will iterate over all cpm_uart compatible =A0
> > devices,
> > while the later test will look for an fsl,cpm-smc compatible device.
>=20
> Well, "fsl,cpm-uart" (note the two differences), but yes.

My point was about the 'smc' part that needs to be added, so I guess
if we want to have that in there, it should be "fsl,cpm-smc-uart"
really, and "fsl,cpm-scc-uart", respectively.

	Arnd <><

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Mark Zhan]

On Tue, 2007-07-17 at 15:06 +0200, Arnd Bergmann wrote:
> On Tuesday 17 July 2007, Mark Zhan wrote:
> > Yes, basically I agree what you say. That should be the right way. 
> > 
> > but, the current situation is that: in all DTS files that are using smc
> > or scc as uart device, all device node definitions have the same
> > "compatible" property -- "cpm_uart"
> > 
> > So what I do here is just following the upstream source tree.
> > 
> 
> Well, AFAICS, all of them currently use scc. The 8xx platforms don't
> even build correctly in the mainline kernel, so I guess it would
> be good to change them to also list fsl,cpm-smc in the compatible
> property.

That probably could be done by another series of patches, and I really
don't want to introduce those kinds of patches in my BSP patches. I
don't know if the 8xx/82xx series patches from Scott Wood will cover
this issue or not. We can wait for them to see where thing will go.

Thanks
mark zhan

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Segher Boessenkool]

>>>> +               if (strstr(model, "SMC")) {
>>>> +                       cpm_uart_dev =  
>>>> platform_device_register_simple("fsl-cpm-smc:uart",
>>>> +                                                       i, &r 
>>>> [0], 3);
>>>> +               } else if (strstr(model, "SCC")) {

>>> You should probably use of_device_is_compatible() to check
>>> if a given device can be used by a particular driver.
>>
>> I think, the function of_find_compatible_node(), which is called  
>> in the
>> for loop, has already done that. So definitely, no need to call
>> of_device_is_compatible() any more.
>
> It's a little more complicated than that. The "compatible" property  
> should
> indicate the exact interface of that device, so the "model" does  
> not really
> matter here.

"model" is meant to be an exact manufacturer model number.
This can be used for workaround code or such if the "compatible"
entry isn't specific enough.  You should try to make sure your
"compatible" entries are, though.

Oh, and strstr() is asking for trouble.  Just do a full
compare.

> I don't know the difference between smc and scc, but if you
> need to register them as different pplatform devices, they should
> normally also have different names in "compatible", possibly in  
> addition
> to the existing one.

Either that, or if they are identical device, just used in
different ways / connected differently, show that via some
other properties or such.

> If the compatible property contains "fsl,cpm-smc\0cpm_uart", you  
> can scan for
> either of them. The loop will iterate over all cpm_uart compatible  
> devices,
> while the later test will look for an fsl,cpm-smc compatible device.

Well, "fsl,cpm-uart" (note the two differences), but yes.


Segher

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Segher Boessenkool]

> but, the current situation is that: in all DTS files that are using  
> smc
> or scc as uart device, all device node definitions have the same
> "compatible" property -- "cpm_uart"
>
> So what I do here is just following the upstream source tree.

True enough.  OTOH, it doesn't help to continue supporting
the "bad old stuff", esp. if new extra workarounds are
needed for it.


Segher

Re: [PATCH 2/3] 82xx: Parse SMC serial device node in DTS

[Scott Wood]

Arnd Bergmann wrote:
> On Tuesday 17 July 2007, Mark Zhan wrote:
> 
>>>Well, AFAICS, all of them currently use scc. The 8xx platforms don't
>>>even build correctly in the mainline kernel, so I guess it would
>>>be good to change them to also list fsl,cpm-smc in the compatible
>>>property.
>>
>>That probably could be done by another series of patches, and I really
>>don't want to introduce those kinds of patches in my BSP patches. I
>>don't know if the 8xx/82xx series patches from Scott Wood will cover
>>this issue or not.
> 
> 
> I guess he can best answer that himself. Scott, what are your plans
> to deal with the different types of cpm ports?

My patchset converts the CPM enet and serial drivers to of platform 
devices when CONFIG_PPC_MERGE is defined, and signficantly overhauls the 
device tree binding.  A 885 CPM UART would have a compatible like this:

compatible = "fsl,mpc885-smc-uart", "fsl,cpm1-smc-uart",
              "fsl,cpm1-uart", "fsl,cpm-smc-uart", "fsl,cpm-uart";

-Scott