Skip to contentSkip to conversation

ARM: shmobile: Koelsch DT serial integration prototype

PATCH/RFC Magnus Damm 5 participants 17 messages Apr 21, 2014 – Apr 24, 2014

Subsystems (5)
Device tree bindings Arm (32-bit) Arm sub-architectures SuperH +1 more subsystems
lore
Share & tools
lore

Magnus Damm

author maintainer
#0 lore raw

From: Magnus Damm <[email protected]>

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

This patch handles all 3 cases of C-code less DT board support and the older board support with C code which includes both legacy and DT reference. The "enabling of serial DT devices in the board DTS" and "requiring serial DT devices at boot" are disconnected in this patch - only the former is implemented. The latter is best handled when C board code is phased out in my opinion.

To handle serial ports described in DT this patch does two things:

1) It enables serial ports in the board specific DTS
-> Nothing special here, except perhaps the preserved device order

2) It extends the existing platform device registration code -> When the existing C board code calls to add serial platform devices then we check if DT platform devices exist in the DTB. If so then we disable the serial DT devices and stick with platform devices.

The combination of 1) and 2) allows us to use the same DTB regardless if C board code is compiled in or not. Also migration becomes easy and forgiving, if for instance the user only updates the kernel and uses the old DTB then the board code will keep on working even though serial devices may not be present in the DTS.

The patch simply postpones the entire "which serial port minor device shall we use" issue and simply uses the same device as the existing board code. If we want to reorder the serial ports then we can do that as an incremental step afterwards - after board code written in C has been removed. Not reordering has the benefit that it reduces complexity and allows us to keep the kernel command line in the board DTS as-is.

Needs to be beaten into better shape and made generic enough to support all mach-shmobile board code.

Not signed-off-by: Magnus Damm <[email protected]>
---

Written on top of renesas-devel-v3.15-rc1-20140414v2

 arch/arm/boot/dts/r8a7791-koelsch.dts  |   21 ++++++++++++++++++++-
 arch/arm/mach-shmobile/setup-r8a7791.c |   27 ++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 2 deletions(-)
arch/arm/boot/dts/r8a7791-koelsch.dts +20 -1
@@ -19,6 +19,11 @@
 	model = "Koelsch";
 	compatible = "renesas,koelsch", "renesas,r8a7791";
 
+	aliases {
+		serial6 = &scif0;
+		serial7 = &scif1;
+	};
+
 	chosen {
 		bootargs = "console=ttySC6,115200 ignore_loglevel rw root=/dev/nfs ip=dhcp";
 	};

@@ -230,7 +235,7 @@
 };
 
 &pfc {
-	pinctrl-0 = <&du_pins &scif0_pins &scif1_pins>;
+	pinctrl-0 = <&du_pins>;
 	pinctrl-names = "default";
 
 	i2c2_pins: i2c2 {

@@ -342,6 +347,20 @@
 	status = "okay";
 };
 
+&scif0 {
+       pinctrl-0 = <&scif0_pins>;
+       pinctrl-names = "default";
+
+       status = "okay";
+};
+
+&scif1 {
+       pinctrl-0 = <&scif1_pins>;
+       pinctrl-names = "default";
+
+       status = "okay";
+};
+
 &qspi {
 	pinctrl-0 = <&qspi_pins>;
 	pinctrl-names = "default";
arch/arm/mach-shmobile/setup-r8a7791.c +26 -1
@@ -94,7 +94,9 @@ static struct plat_sci_port scif##index#
 static struct resource scif##index##_resources[] = {			\
 	DEFINE_RES_MEM(baseaddr, 0x100),				\
 	DEFINE_RES_IRQ(irq),						\
-}
+};									\
+									\
+static struct property scif##index##_property
 
 #define R8A7791_SCIF(index, baseaddr, irq)				\
 	__R8A7791_SCIF(PORT_SCIF, index, baseaddr, irq)

@@ -121,7 +123,30 @@ R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29
 R8A7791_SCIFA(13, 0xe6c78000, gic_spi(30)); /* SCIFA4 */
 R8A7791_SCIFA(14, 0xe6c80000, gic_spi(31)); /* SCIFA5 */
 
+static void serial_disable_dt(struct resource *res, struct property *prop)
+{
+	struct device_node *np;
+	char str[17];
+	char *disabled = "disabled";
+
+	snprintf(str, ARRAY_SIZE(str), "/serial@%08x",
+		 (unsigned int)res->start);
+
+	np = of_find_node_by_path(str);
+	if (np) {
+		if (of_device_is_available(np)) {
+			prop->name = "status";
+			prop->length = strlen(disabled) + 1;
+			prop->value = disabled;
+			of_update_property(np, prop);
+			printk("xxx disabled %s\n", str);
+		}
+		of_node_put(np);
+	}
+}
+
 #define r8a7791_register_scif(index)					       \
+	serial_disable_dt(scif##index##_resources, &scif##index##_property);   \
 	platform_device_register_resndata(&platform_bus, "sh-sci", index,      \
 					  scif##index##_resources,	       \
 					  ARRAY_SIZE(scif##index##_resources), \
#1 lore raw

Hi Magnus,

Quoted 3 lines · Magnus Damm

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

[...]

Quoted 2 lines

1) It enables serial ports in the board specific DTS
-> Nothing special here, except perhaps the preserved device order

Good, thanks!

Quoted 4 lines

2) It extends the existing platform device registration code -> When the existing C board code calls to add serial platform devices then we check if DT platform devices exist in the DTB. If so then we disable the serial DT devices and stick with platform devices.

Is there any specific reason why you chose this solution, over the alternative approach of not registering the platform devices if the corresponding serial devices are present in DT?

Quoted 5 lines

The combination of 1) and 2) allows us to use the same DTB regardless if C board code is compiled in or not. Also migration becomes easy and forgiving, if for instance the user only updates the kernel and uses the old DTB then the board code will keep on working even though serial devices may not be present in the DTS.

Very well!

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 static struct plat_sci_port scif##index# · line 99

Quoted 10 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +3 -1
@@ -94,7 +94,9 @@ static struct plat_sci_port scif##index#
 static struct resource scif##index##_resources[] = {                   \
        DEFINE_RES_MEM(baseaddr, 0x100),                                \
        DEFINE_RES_IRQ(irq),                                            \
-}
+};                                                                     \
+                                                                       \
+static struct property scif##index##_property

This is static, as the property's lifecycle dictates it to stay alive...

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 130

Quoted 11 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +5 -0
 #define R8A7791_SCIF(index, baseaddr, irq)                             \
        __R8A7791_SCIF(PORT_SCIF, index, baseaddr, irq)

@@ -121,7 +123,30 @@ R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29
 R8A7791_SCIFA(13, 0xe6c78000, gic_spi(30)); /* SCIFA4 */
 R8A7791_SCIFA(14, 0xe6c80000, gic_spi(31)); /* SCIFA5 */
 
+static void serial_disable_dt(struct resource *res, struct property *prop)
+{
+       struct device_node *np;
+       char str[17];
+       char *disabled = "disabled";

... while this is a local variable on the stack, with limited lifetime. So that should be:

        static char disabled[] = "disabled";

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 138

Quoted 8 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +8 -0
+       snprintf(str, ARRAY_SIZE(str), "/serial@%08x",
+                (unsigned int)res->start);
+
+       np = of_find_node_by_path(str);
+       if (np) {
+               if (of_device_is_available(np)) {
+                       prop->name = "status";
+                       prop->length = strlen(disabled) + 1;

Or "sizeof(disabled)", after changing to a static array.

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 141

Quoted 3 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +3 -0
+                       prop->value = disabled;
+                       of_update_property(np, prop);
+                       printk("xxx disabled %s\n", str);

pr_info(), drop the "xxx ", capitalize "Disabled".

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 151

Quoted 10 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +6 -0
+               }
+               of_node_put(np);
+       }
+}
+
 #define r8a7791_register_scif(index)                                          \
+       serial_disable_dt(scif##index##_resources, &scif##index##_property);   \
        platform_device_register_resndata(&platform_bus, "sh-sci", index,      \
                                          scif##index##_resources,             \
                                          ARRAY_SIZE(scif##index##_resources), \

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

#2 lore raw
In reply to Magnus Damm

Hi Magnus,

Thank you for the patch.

Quoted 5 lines · On Monday 21 April 2014 10:31:14 Magnus Damm wrote:

From: Magnus Damm <[email protected]>

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to DT in one go, like we do for all other devices.

arch/arm/boot/dts/r8a7791-koelsch.dts

Quoted 45 lines

This patch handles all 3 cases of C-code less DT board support and the older board support with C code which includes both legacy and DT reference. The "enabling of serial DT devices in the board DTS" and "requiring serial DT devices at boot" are disconnected in this patch - only the former is implemented. The latter is best handled when C board code is phased out in my opinion.

To handle serial ports described in DT this patch does two things:

1) It enables serial ports in the board specific DTS
-> Nothing special here, except perhaps the preserved device order

2) It extends the existing platform device registration code -> When the existing C board code calls to add serial platform devices then we check if DT platform devices exist in the DTB. If so then we disable the serial DT devices and stick with platform devices.

The combination of 1) and 2) allows us to use the same DTB regardless if C board code is compiled in or not. Also migration becomes easy and forgiving, if for instance the user only updates the kernel and uses the old DTB then the board code will keep on working even though serial devices may not be present in the DTS.

The patch simply postpones the entire "which serial port minor device shall we use" issue and simply uses the same device as the existing board code. If we want to reorder the serial ports then we can do that as an incremental step afterwards - after board code written in C has been removed. Not reordering has the benefit that it reduces complexity and allows us to keep the kernel command line in the board DTS as-is.

Needs to be beaten into better shape and made generic enough to support all mach-shmobile board code.

Not signed-off-by: Magnus Damm <[email protected]>
---

Written on top of renesas-devel-v3.15-rc1-20140414v2

 arch/arm/boot/dts/r8a7791-koelsch.dts  |   21 ++++++++++++++++++++-
 arch/arm/mach-shmobile/setup-r8a7791.c |   27 ++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 2 deletions(-)
arch/arm/boot/dts/r8a7791-koelsch.dts +0 -0

17:02:42.000000000

Quoted 87 lines

+0900 @@ -19,6 +19,11 @@

arch/arm/boot/dts/r8a7791-koelsch.dts +5 -0
 	model = "Koelsch";
 	compatible = "renesas,koelsch", "renesas,r8a7791";
 
+	aliases {
+		serial6 = &scif0;
+		serial7 = &scif1;
+	};
+
 	chosen {
 		bootargs = "console=ttySC6,115200 ignore_loglevel rw root=/dev/nfs

ip=dhcp"; };

arch/arm/boot/dts/r8a7791-koelsch.dts +15 -1
@@ -230,7 +235,7 @@
 };
 
 &pfc {
-	pinctrl-0 = <&du_pins &scif0_pins &scif1_pins>;
+	pinctrl-0 = <&du_pins>;
 	pinctrl-names = "default";
 
 	i2c2_pins: i2c2 {

@@ -342,6 +347,20 @@
 	status = "okay";
 };
 
+&scif0 {
+       pinctrl-0 = <&scif0_pins>;
+       pinctrl-names = "default";
+
+       status = "okay";
+};
+
+&scif1 {
+       pinctrl-0 = <&scif1_pins>;
+       pinctrl-names = "default";
+
+       status = "okay";
+};
+
 &qspi {
 	pinctrl-0 = <&qspi_pins>;
 	pinctrl-names = "default";
arch/arm/mach-shmobile/setup-r8a7791.c +0 -0

16:32:32.000000000 +0900 @@ -94,7 +94,9 @@ static struct plat_sci_port scif##index#
static struct resource scif##index##_resources[] = { \

arch/arm/mach-shmobile/setup-r8a7791.c +26 -1
 	DEFINE_RES_MEM(baseaddr, 0x100),				\
 	DEFINE_RES_IRQ(irq),						\
-}
+};									\
+									\
+static struct property scif##index##_property
 
 #define R8A7791_SCIF(index, baseaddr, irq)				\
 	__R8A7791_SCIF(PORT_SCIF, index, baseaddr, irq)

@@ -121,7 +123,30 @@ R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29
 R8A7791_SCIFA(13, 0xe6c78000, gic_spi(30)); /* SCIFA4 */
 R8A7791_SCIFA(14, 0xe6c80000, gic_spi(31)); /* SCIFA5 */
 
+static void serial_disable_dt(struct resource *res, struct property *prop)
+{
+	struct device_node *np;
+	char str[17];
+	char *disabled = "disabled";
+
+	snprintf(str, ARRAY_SIZE(str), "/serial@%08x",
+		 (unsigned int)res->start);
+
+	np = of_find_node_by_path(str);
+	if (np) {
+		if (of_device_is_available(np)) {
+			prop->name = "status";
+			prop->length = strlen(disabled) + 1;
+			prop->value = disabled;
+			of_update_property(np, prop);
+			printk("xxx disabled %s\n", str);
+		}
+		of_node_put(np);
+	}
+}
+
 #define r8a7791_register_scif(index)					       \
+	serial_disable_dt(scif##index##_resources, &scif##index##_property);   \
 	platform_device_register_resndata(&platform_bus, "sh-sci", index,      \
 					  scif##index##_resources,	       \
 					  ARRAY_SIZE(scif##index##_resources), \
Signature · 3 lines
Regards,

Laurent Pinchart
#3 lore raw
In reply to Magnus Damm

Hi Laurent,

Quoted 9 lines · Laurent Pinchart
Quoted 5 lines · On Monday 21 April 2014 10:31:14 Magnus Damm wrote:

From: Magnus Damm <[email protected]>

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to DT in one go, like we do for all other devices.

Having the backwards-compatibility mode for DTSes without the main serial console is nice, though. But I agree that we shouldn't introduce more steps than necessary.

FWIW, I've been using your "ARM: shmobile: koelsch: Enable SCIF0 and SCIF1 serial ports in DT" on and off.

Last time I tried together with "of: Enable console on serial ports specified by /chosen/stdout-path", which allows to boot without any "console=" parameters at all.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

#4 lore raw
In reply to Magnus Damm

Hi Geert,

Quoted 8 lines · On Monday 21 April 2014 12:22:02 Geert Uytterhoeven wrote:
Quoted 7 lines · Laurent Pinchart
Quoted 5 lines · On Monday 21 April 2014 10:31:14 Magnus Damm wrote:

From: Magnus Damm <[email protected]>

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

Quoted 6 lines
Quoted 2 lines

I think you've over-engineering it. I would just switch serial devices to DT in one go, like we do for all other devices.

Having the backwards-compatibility mode for DTSes without the main serial console is nice, though. But I agree that we shouldn't introduce more steps than necessary.

Sure, that could be useful during development when you regularly have to switch between different DTs files. However I don't think we should add too many of such hacks to the mainline kernel. Given that we can easily switch in one go, I believe the patch can be kept out-of-tree.

Quoted 2 lines

FWIW, I've been using your "ARM: shmobile: koelsch: Enable SCIF0 and SCIF1 serial ports in DT" on and off.

I keep them in all my working branch and haven't run into any issue so far, so I think they should be pretty stable now. Thank you for testing them.

Quoted 3 lines

Last time I tried together with "of: Enable console on serial ports specified by /chosen/stdout-path", which allows to boot without any "console=" parameters at all.

That's a nice patch, I wasn't aware of it. How is the baud rate selected in that case ?

Signature · 3 lines
Regards,

Laurent Pinchart
#5 lore raw
In reply to Magnus Damm

Hi Laurent,

Quoted 5 lines · Laurent Pinchart
Quoted 3 lines

Last time I tried together with "of: Enable console on serial ports specified by /chosen/stdout-path", which allows to boot without any "console=" parameters at all.

That's a nice patch, I wasn't aware of it.

You probably want to apply the other parts from the series "[RFC 0/5] of: Automatic console registration cleanups" as well.

Quoted 1 line

How is the baud rate selected in that case ?

Funny, I was also wondering when I first saw the series, and it motivated me to give it a try ;-)

drivers/tty/serial/sh-sci.c:serial_console_setup():

static int serial_console_setup(struct console *co, char *options)

{
        struct sci_port *sci_port;
        struct uart_port *port;
        int baud = 115200;
        int bits = 8;
        int parity = 'n';
        int flow = 'n';

In the absence of any user-specified parameters, it uses the defaults above.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

#6 lore raw
In reply to Magnus Damm
Quoted 14 lines · Geert Uytterhoeven

Hi Magnus,

Quoted 3 lines · Magnus Damm

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

[...]

Quoted 2 lines

1) It enables serial ports in the board specific DTS
-> Nothing special here, except perhaps the preserved device order

Good, thanks!

Quoted 8 lines
Quoted 4 lines

2) It extends the existing platform device registration code -> When the existing C board code calls to add serial platform devices then we check if DT platform devices exist in the DTB. If so then we disable the serial DT devices and stick with platform devices.

Is there any specific reason why you chose this solution, over the alternative approach of not registering the platform devices if the corresponding serial devices are present in DT?

My understanding is that the motivation is to avoid changing the run-time behaviour due to internal infrastructure changes: specifically to avoid changing the names of serial port devices.

Quoted 81 lines
Quoted 5 lines

The combination of 1) and 2) allows us to use the same DTB regardless if C board code is compiled in or not. Also migration becomes easy and forgiving, if for instance the user only updates the kernel and uses the old DTB then the board code will keep on working even though serial devices may not be present in the DTS.

Very well!

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 static struct plat_sci_port scif##index# · line 99

Quoted 10 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +3 -1
@@ -94,7 +94,9 @@ static struct plat_sci_port scif##index#
 static struct resource scif##index##_resources[] = {                   \
        DEFINE_RES_MEM(baseaddr, 0x100),                                \
        DEFINE_RES_IRQ(irq),                                            \
-}
+};                                                                     \
+                                                                       \
+static struct property scif##index##_property

This is static, as the property's lifecycle dictates it to stay alive...

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 130

Quoted 11 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +5 -0
 #define R8A7791_SCIF(index, baseaddr, irq)                             \
        __R8A7791_SCIF(PORT_SCIF, index, baseaddr, irq)

@@ -121,7 +123,30 @@ R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29
 R8A7791_SCIFA(13, 0xe6c78000, gic_spi(30)); /* SCIFA4 */
 R8A7791_SCIFA(14, 0xe6c80000, gic_spi(31)); /* SCIFA5 */
 
+static void serial_disable_dt(struct resource *res, struct property *prop)
+{
+       struct device_node *np;
+       char str[17];
+       char *disabled = "disabled";

... while this is a local variable on the stack, with limited lifetime. So that should be:

        static char disabled[] = "disabled";

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 138

Quoted 8 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +8 -0
+       snprintf(str, ARRAY_SIZE(str), "/serial@%08x",
+                (unsigned int)res->start);
+
+       np = of_find_node_by_path(str);
+       if (np) {
+               if (of_device_is_available(np)) {
+                       prop->name = "status";
+                       prop->length = strlen(disabled) + 1;

Or "sizeof(disabled)", after changing to a static array.

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 141

Quoted 3 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +3 -0
+                       prop->value = disabled;
+                       of_update_property(np, prop);
+                       printk("xxx disabled %s\n", str);

pr_info(), drop the "xxx ", capitalize "Disabled".

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 151

Quoted 10 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +6 -0
+               }
+               of_node_put(np);
+       }
+}
+
 #define r8a7791_register_scif(index)                                          \
+       serial_disable_dt(scif##index##_resources, &scif##index##_property);   \
        platform_device_register_resndata(&platform_bus, "sh-sci", index,      \
                                          scif##index##_resources,             \
                                          ARRAY_SIZE(scif##index##_resources), \

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#7 lore raw
In reply to Magnus Damm

Hi Geert,

Quoted 7 lines · Geert Uytterhoeven

Hi Magnus,

Quoted 3 lines · Magnus Damm

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

Quoted 6 lines

[...]

Quoted 2 lines

1) It enables serial ports in the board specific DTS
-> Nothing special here, except perhaps the preserved device order

Good, thanks!

Thanks! =)

Quoted 8 lines
Quoted 4 lines

2) It extends the existing platform device registration code -> When the existing C board code calls to add serial platform devices then we check if DT platform devices exist in the DTB. If so then we disable the serial DT devices and stick with platform devices.

Is there any specific reason why you chose this solution, over the alternative approach of not registering the platform devices if the corresponding serial devices are present in DT?

At first it seemed more logical to disable the platform devices when serial DT devices existed, however after considering how the rest of the C board code operates I came to think that it would be most sane to keep the devices the same in case board code written in C while still allowing the DT-only case to use serial DT as expected. This way Legacy board code and DT reference board code written in C keep on behaving the same. Basically we end up with two cases:

A) DT-only board support (Only sane way forward for upstream development)

-> These use serial ports described in DTB but allow easy use together with same DTB as in B) by keeping the serial order as before. This is where we develop and integrate upstream support. This is what we should care about.

B) Board code written in C (Legacy + DT reference)

-> I propose that these will keep on using platform devices written in C until we phase out the actual C board code. Changing this around does not make any sense IMO (just migration pain, no real upside), focus for upstream development should be put on A) to allow us removing B) as soon as possible.

The real questions are IMO:

What needs to be done so we can phase out B)? Can we pour effort from changing around B) into improving A)?

Quoted 47 lines

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 static struct plat_sci_port scif##index# · line 99

Quoted 10 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +3 -1
@@ -94,7 +94,9 @@ static struct plat_sci_port scif##index#
 static struct resource scif##index##_resources[] = {                   \
        DEFINE_RES_MEM(baseaddr, 0x100),                                \
        DEFINE_RES_IRQ(irq),                                            \
-}
+};                                                                     \
+                                                                       \
+static struct property scif##index##_property

This is static, as the property's lifecycle dictates it to stay alive...

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 130

Quoted 11 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +5 -0
 #define R8A7791_SCIF(index, baseaddr, irq)                             \
        __R8A7791_SCIF(PORT_SCIF, index, baseaddr, irq)

@@ -121,7 +123,30 @@ R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29
 R8A7791_SCIFA(13, 0xe6c78000, gic_spi(30)); /* SCIFA4 */
 R8A7791_SCIFA(14, 0xe6c80000, gic_spi(31)); /* SCIFA5 */
 
+static void serial_disable_dt(struct resource *res, struct property *prop)
+{
+       struct device_node *np;
+       char str[17];
+       char *disabled = "disabled";

... while this is a local variable on the stack, with limited lifetime. So that should be:

        static char disabled[] = "disabled";

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 138

Quoted 8 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +8 -0
+       snprintf(str, ARRAY_SIZE(str), "/serial@%08x",
+                (unsigned int)res->start);
+
+       np = of_find_node_by_path(str);
+       if (np) {
+               if (of_device_is_available(np)) {
+                       prop->name = "status";
+                       prop->length = strlen(disabled) + 1;

Or "sizeof(disabled)", after changing to a static array.

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 141

Quoted 3 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +3 -0
+                       prop->value = disabled;
+                       of_update_property(np, prop);
+                       printk("xxx disabled %s\n", str);

pr_info(), drop the "xxx ", capitalize "Disabled".

Quoted 10 lines

arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 R8A7791_SCIFA(12, 0xe6c70000, gic_spi(29 · line 151

Quoted 10 lines
arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900 renamed from arch/arm/mach-shmobile/setup-r8a7791.c +6 -0
+               }
+               of_node_put(np);
+       }
+}
+
 #define r8a7791_register_scif(index)                                          \
+       serial_disable_dt(scif##index##_resources, &scif##index##_property);   \
        platform_device_register_resndata(&platform_bus, "sh-sci", index,      \
                                          scif##index##_resources,             \
                                          ARRAY_SIZE(scif##index##_resources), \

Thanks for the feedback. As you probably can tell, this ended up as a pretty quick unpolished hack! =)

I don't intend to update this prototype. After discussion and agreement about how to integrate the DT serial devices I think Simon should continue the actual integration - perhaps using code from here if that's what we agree on.

Cheers,

/ magnus

author maintainer
#8 lore raw
In reply to Magnus Damm

Hi Laurent,

Quoted 5 lines · Laurent Pinchart

Hi Magnus,

Thank you for the patch.

On Monday 21 April 2014 10:31:14 Magnus Damm wrote:

Quoted 8 lines
Quoted 5 lines

From: Magnus Damm <[email protected]>

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to DT in one go, like we do for all other devices.

I agree about the risk of over-engineering! =)

At the same time I would like to integrate your serial DT support as soon as possible. Order wise I'd like to integrate your serial DT support before phasing out legacy board code. We can of course discuss the order, but if we first integrate serial DT then we end up with 3 different serial cases since legacy does not use DT for "regular devices" at all. This seems quite messy in my mind, so with this proposal I hope to let all us upstream developers focus on the "DT-only board code" case that should move forward unrestricted, and at the same time keep the C board code as-is serial port wise but also start phasing out more aggressively.

I'm also considering handling timers the same. The point is to allow unrestricted development of "DT-only board code" and still limit the amount of churn for legacy to a bare minimum.

Let us discuss more face to face at ELC!

Thanks,

/ magnus

#9 lore raw
In reply to Magnus Damm

Hi Magnus,

Quoted 11 lines · On Wednesday 23 April 2014 11:07:25 Magnus Damm wrote:
Quoted 9 lines · Laurent Pinchart
Quoted 5 lines · On Monday 21 April 2014 10:31:14 Magnus Damm wrote:

From: Magnus Damm <[email protected]>

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to DT in one go, like we do for all other devices.

Quoted 6 lines

I agree about the risk of over-engineering! =)

At the same time I would like to integrate your serial DT support as soon as possible. Order wise I'd like to integrate your serial DT support before phasing out legacy board code. We can of course discuss the order, but if we first integrate serial DT then we end up with 3 different serial cases

The fact that we even have 3 different cases (DT only, C DT reference and C legacy) shows that there's a problem. We started with C code that we forked into C DT reference code instead of porting the C code over to DT, and now we want to keep using platform devices in the C DT reference code ? Come on... What's the next step, introducing new C DT reference-but-better board files ? :-) The C DT reference board files need to be ported to DT or dropped altogether right now. Any intermediate solution is just asking for trouble.

Quoted 9 lines

since legacy does not use DT for "regular devices" at all. This seems quite messy in my mind, so with this proposal I hope to let all us upstream developers focus on the "DT-only board code" case that should move forward unrestricted, and at the same time keep the C board code as-is serial port wise but also start phasing out more aggressively.

I'm also considering handling timers the same. The point is to allow unrestricted development of "DT-only board code" and still limit the amount of churn for legacy to a bare minimum.

Seriously ? Count me out on that...

Quoted 1 line

Let us discuss more face to face at ELC!

Signature · 3 lines
Regards,

Laurent Pinchart
author maintainer
#10 lore raw
In reply to Magnus Damm

Hi Laurent,

Quoted 21 lines · Laurent Pinchart

Hi Magnus,

Quoted 11 lines · On Wednesday 23 April 2014 11:07:25 Magnus Damm wrote:
Quoted 9 lines · Laurent Pinchart
Quoted 5 lines · On Monday 21 April 2014 10:31:14 Magnus Damm wrote:

From: Magnus Damm <[email protected]>

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to DT in one go, like we do for all other devices.

Quoted 6 lines

I agree about the risk of over-engineering! =)

At the same time I would like to integrate your serial DT support as soon as possible. Order wise I'd like to integrate your serial DT support before phasing out legacy board code. We can of course discuss the order, but if we first integrate serial DT then we end up with 3 different serial cases

Quoted 4 lines

The fact that we even have 3 different cases (DT only, C DT reference and C legacy) shows that there's a problem. We started with C code that we forked into C DT reference code instead of porting the C code over to DT, and now we want to keep using platform devices in the C DT reference code ? Come on...

What is the problem? We need to encourage development and also provide a stable base. And from my point of view, you're not even supposed to use the C DT reference code as a developer - you should use DT-only board support for development.

Historically we implemented DT bindings for as many devices as possible, but since SCIF and timers lacked DT bindings we used platform devices for them to allow early DT development of other devices even though DT bindings for some devices were missing.

Quoted 3 lines

What's the next step, introducing new C DT reference-but-better board files ? :-) The C DT reference board files need to be ported to DT or dropped altogether right now. Any intermediate solution is just asking for trouble.

Please tell me how we are supposed to use the devices that are included in the C DT reference board code but lack DT bindings?

I have no problem switching over to DT, but unless bindings exist it is difficult to move over... =)

Quoted 11 lines
Quoted 9 lines

since legacy does not use DT for "regular devices" at all. This seems quite messy in my mind, so with this proposal I hope to let all us upstream developers focus on the "DT-only board code" case that should move forward unrestricted, and at the same time keep the C board code as-is serial port wise but also start phasing out more aggressively.

I'm also considering handling timers the same. The point is to allow unrestricted development of "DT-only board code" and still limit the amount of churn for legacy to a bare minimum.

Seriously ? Count me out on that...

From my point of view it makes most sense to
- add DT bindings for devices that lack them
- get rid of C board code

But until we have DT bindings
- keep using what we have without breakage

Which portion is it that you dislike?

/ magnus

author maintainer
#11 lore raw
In reply to Magnus Damm

Hi Laurent, everyone,

Quoted 62 lines · Magnus Damm

Hi Laurent,

Quoted 21 lines · Laurent Pinchart

Hi Magnus,

Quoted 11 lines · On Wednesday 23 April 2014 11:07:25 Magnus Damm wrote:
Quoted 9 lines · Laurent Pinchart
Quoted 5 lines · On Monday 21 April 2014 10:31:14 Magnus Damm wrote:

From: Magnus Damm <[email protected]>

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to DT in one go, like we do for all other devices.

Quoted 6 lines

I agree about the risk of over-engineering! =)

At the same time I would like to integrate your serial DT support as soon as possible. Order wise I'd like to integrate your serial DT support before phasing out legacy board code. We can of course discuss the order, but if we first integrate serial DT then we end up with 3 different serial cases

Quoted 4 lines

The fact that we even have 3 different cases (DT only, C DT reference and C legacy) shows that there's a problem. We started with C code that we forked into C DT reference code instead of porting the C code over to DT, and now we want to keep using platform devices in the C DT reference code ? Come on...

What is the problem? We need to encourage development and also provide a stable base. And from my point of view, you're not even supposed to use the C DT reference code as a developer - you should use DT-only board support for development.

Historically we implemented DT bindings for as many devices as possible, but since SCIF and timers lacked DT bindings we used platform devices for them to allow early DT development of other devices even though DT bindings for some devices were missing.

Quoted 3 lines

What's the next step, introducing new C DT reference-but-better board files ? :-) The C DT reference board files need to be ported to DT or dropped altogether right now. Any intermediate solution is just asking for trouble.

Please tell me how we are supposed to use the devices that are included in the C DT reference board code but lack DT bindings?

I have no problem switching over to DT, but unless bindings exist it is difficult to move over... =)

Quoted 11 lines
Quoted 9 lines

since legacy does not use DT for "regular devices" at all. This seems quite messy in my mind, so with this proposal I hope to let all us upstream developers focus on the "DT-only board code" case that should move forward unrestricted, and at the same time keep the C board code as-is serial port wise but also start phasing out more aggressively.

I'm also considering handling timers the same. The point is to allow unrestricted development of "DT-only board code" and still limit the amount of churn for legacy to a bare minimum.

Seriously ? Count me out on that...

Quoted 8 lines

From my point of view it makes most sense to
- add DT bindings for devices that lack them
- get rid of C board code

But until we have DT bindings
- keep using what we have without breakage

Which portion is it that you dislike?

Now after some negotiation over chat Laurent and I have come to the following conclusion:

Our proposed way forward for the upcoming SCIF DT integration is:
- keeping the existing serial port order
- requiring a DTB upgrade for DT reference C code
- keeping legacy C board code as-is

Laurent said he will update his DT integration patches and submit them to the mailing list.

Regarding serial port order, it is of course important to use a standard serial port order. To be able to share same DTB with already existing board code we plan on adjusting the serial port order after phasing out the legacy board code.

The following schedule has been proposed:
1. DT SCIF integration
2. DT timer integration
3. legacy board code removal
4. SCIF reordering

If there are any concerns about this please let me know. Otherwise I propose we follow the plan above.

Cheers,

/ magnus

#12 lore raw
In reply to Magnus Damm

On 04/23/2014 04:22 PM, Magnus Damm wrote:

[...]

Quoted 19 lines
Quoted 14 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to DT in one go, like we do for all other devices.

Quoted 6 lines

I agree about the risk of over-engineering! =)

At the same time I would like to integrate your serial DT support as soon as possible. Order wise I'd like to integrate your serial DT support before phasing out legacy board code. We can of course discuss the order, but if we first integrate serial DT then we end up with 3 different serial cases

Quoted 4 lines

The fact that we even have 3 different cases (DT only, C DT reference and C legacy) shows that there's a problem. We started with C code that we forked into C DT reference code instead of porting the C code over to DT, and now we want to keep using platform devices in the C DT reference code ? Come on...

Quoted 4 lines

What is the problem? We need to encourage development and also provide a stable base. And from my point of view, you're not even supposed to use the C DT reference code as a developer - you should use DT-only board support for development.

I wonder how DT-only support (without C reference code) is possible with
the current devel branch: only board-{lager|koelsch}-reference.c include call to of_platform_populate() without which DT probing drivers won't see their devices. If we had that call in the setup-r8a779[01].c then I would have understood what you mean by DT-only...

Quoted 1 line

/ magnus

WBR, Sergei

#13 lore raw
In reply to Magnus Damm
Quoted 28 lines · Sergei Shtylyov

On 04/23/2014 04:22 PM, Magnus Damm wrote:

[...]

Quoted 19 lines
Quoted 14 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to DT in one go, like we do for all other devices.

Quoted 6 lines

I agree about the risk of over-engineering! =)

At the same time I would like to integrate your serial DT support as soon as possible. Order wise I'd like to integrate your serial DT support before phasing out legacy board code. We can of course discuss the order, but if we first integrate serial DT then we end up with 3 different serial cases

Quoted 4 lines

The fact that we even have 3 different cases (DT only, C DT reference and C legacy) shows that there's a problem. We started with C code that we forked into C DT reference code instead of porting the C code over to DT, and now we want to keep using platform devices in the C DT reference code ? Come on...

Quoted 4 lines

What is the problem? We need to encourage development and also provide a stable base. And from my point of view, you're not even supposed to use the C DT reference code as a developer - you should use DT-only board support for development.

Quoted 6 lines

I wonder how DT-only support (without C reference code) is
possible with the current devel branch: only board-{lager|koelsch}-reference.c include call to of_platform_populate() without which DT probing drivers won't see their devices. If we had that call in the setup-r8a779[01].c then I would have understood what you mean by DT-only...

I believe Magnus is referring to the case where one boots without a board file at all. At this stage that case may require some tweaking to boot.

author maintainer
#14 lore raw
In reply to Magnus Damm
Quoted 42 lines · Sergei Shtylyov

On 04/23/2014 04:22 PM, Magnus Damm wrote:

[...]

Quoted 31 lines
Quoted 21 lines
Quoted 13 lines
Quoted 8 lines
Quoted 3 lines

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to
DT in one go, like we do for all other devices.

I agree about the risk of over-engineering! =)

Quoted 6 lines

At the same time I would like to integrate your serial DT support as soon as
possible. Order wise I'd like to integrate your serial DT support before phasing out legacy board code. We can of course discuss the order, but if we
first integrate serial DT then we end up with 3 different serial cases

Quoted 8 lines

The fact that we even have 3 different cases (DT only, C DT reference and C
legacy) shows that there's a problem. We started with C code that we forked
into C DT reference code instead of porting the C code over to DT, and now we
want to keep using platform devices in the C DT reference code ? Come on...

Quoted 4 lines

What is the problem? We need to encourage development and also provide a stable base. And from my point of view, you're not even supposed to use the C DT reference code as a developer - you should use DT-only board support for development.

Quoted 5 lines

I wonder how DT-only support (without C reference code) is possible with
the current devel branch: only board-{lager|koelsch}-reference.c include call to of_platform_populate() without which DT probing drivers won't see their devices. If we had that call in the setup-r8a779[01].c then I would have understood what you mean by DT-only...

I believe the machine vector in setup-r8a779x.c is enough at this point, perhaps you don't are missing the default case when certain callbacks are left as NULL?

/ magnus

#15 lore raw
In reply to Magnus Damm
Quoted 10 lines · Sergei Shtylyov
Quoted 4 lines

What is the problem? We need to encourage development and also provide a stable base. And from my point of view, you're not even supposed to use the C DT reference code as a developer - you should use DT-only board support for development.

I wonder how DT-only support (without C reference code) is possible with
the current devel branch: only board-{lager|koelsch}-reference.c include call to of_platform_populate() without which DT probing drivers won't see their devices. If we had that call in the setup-r8a779[01].c then I would have understood what you mean by DT-only...

Koelsch boots without C board code, up to the point where NFS root is to be mounted. It seems Ethernet is not working, although I have DT-friendly support for enabling its MSTP clock.

You need "ARM: shmobile: r8a7790: Add early debugging support" and boot with "keep_bootcon", or "ARM: shmobile: koelsch: Enable SCIF0 and SCIF1 serial ports in DT" to keep an eye on what the kernel is doing.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

#16 lore raw
In reply to Magnus Damm

Hello.

Quoted 39 lines · Magnus Damm
Quoted 33 lines

[...]

Quoted 26 lines
Quoted 17 lines
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines

A simple serial port integration prototype that happens to target Koelsch. Written to propose how to move one step closer to integrate DT support for serial ports on mach-shmobile.

I think you've over-engineering it. I would just switch serial devices to
DT in one go, like we do for all other devices.

Quoted 8 lines

I agree about the risk of over-engineering! =)

At the same time I would like to integrate your serial DT support as soon as
possible. Order wise I'd like to integrate your serial DT support before phasing out legacy board code. We can of course discuss the order, but if we
first integrate serial DT then we end up with 3 different serial cases

Quoted 8 lines

The fact that we even have 3 different cases (DT only, C DT reference and C
legacy) shows that there's a problem. We started with C code that we forked
into C DT reference code instead of porting the C code over to DT, and now we
want to keep using platform devices in the C DT reference code ? Come on...

Quoted 4 lines

What is the problem? We need to encourage development and also provide a stable base. And from my point of view, you're not even supposed to use the C DT reference code as a developer - you should use DT-only board support for development.

Quoted 5 lines

I wonder how DT-only support (without C reference code) is possible with
the current devel branch: only board-{lager|koelsch}-reference.c include call to of_platform_populate() without which DT probing drivers won't see their devices. If we had that call in the setup-r8a779[01].c then I would have understood what you mean by DT-only...

Quoted 3 lines

I believe the machine vector in setup-r8a779x.c is enough at this point, perhaps you don't are missing the default case when certain callbacks are left as NULL?

Ah, indeed, init_machine() is missing and in this case
of_platform_populate() is called! Thank you, it wasn't obvious to me.

Quoted 1 line

/ magnus

WBR, Sergei