Skip to contentSkip to conversation

82xx: Parse SMC serial device node in DTS

PATCH In review Mark Zhan 4 participants 12 messages Jul 16, 2007 – Jul 17, 2007 PowerPC

lore
Share & tools
lore

Mark Zhan

author
#0 lore raw

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

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

Signed-off-by Mark Zhan <[email protected]>
---
 arch/powerpc/sysdev/fsl_soc.c |   37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)
arch/powerpc/sysdev/fsl_soc.c +20 -17
Index: linux-powerpc-2.6.x/arch/powerpc/sysdev/fsl_soc.c
===================================================================
@@ -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;
 	}
#1 lore raw

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

author
#2 lore raw

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

Quoted 15 lines · Arnd Bergmann

arch/powerpc/sysdev/fsl_soc.c line 807

Quoted 9 lines · Mark Zhan
arch/powerpc/sysdev/fsl_soc.c +7 -1
-               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 <><
#3 lore raw
Quoted 3 lines · Mark Zhan
Quoted 2 lines · Arnd Bergmann
Quoted 1 line · Mark Zhan

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 cpm_uart_data.uart_clk =3D ppc_proc_fre=

q;

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

+ =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",

Quoted 1 line
Quoted 1 line
Quoted 1 line

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

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

+ =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",

Quoted 1 line
Quoted 1 line
Quoted 1 line

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

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

+ =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 <><
author
#4 lore raw
Quoted 26 lines · Arnd Bergmann
Quoted 11 lines · Mark Zhan
Quoted 10 lines · Arnd Bergmann
Quoted 8 lines · Mark Zhan
arch/powerpc/sysdev/fsl_soc.c +7 -1
-               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);
+               }
Quoted 6 lines
Quoted 2 lines

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.

Quoted 3 lines

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

#5 lore raw
Quoted 8 lines · Mark Zhan

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 <><
author
#6 lore raw
Quoted 7 lines · Arnd Bergmann
Quoted 6 lines · Mark Zhan

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"

Quoted 7 lines
Quoted 2 lines

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

#7 lore raw
Quoted 10 lines · Mark Zhan
Quoted 5 lines

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 <><
#8 lore raw
Quoted 7 lines · Arnd Bergmann
Quoted 5 lines · Mark Zhan
Quoted 4 lines

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.

Quoted 8 lines
Quoted 4 lines

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

#9 lore raw
In reply to Mark Zhan
Quoted 5 lines

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

#10 lore raw
In reply to Arnd Bergmann
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
(quoted diff) +2 -0
+               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.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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

#11 lore raw
Quoted 7 lines · Segher Boessenkool
Quoted 5 lines

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