[PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

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 <damm+renesas@opensource.se>
---

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

--- 0001/arch/arm/boot/dts/r8a7791-koelsch.dts
+++ work/arch/arm/boot/dts/r8a7791-koelsch.dts	2014-04-17 17:02:42.000000000 +0900
@@ -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";
--- 0001/arch/arm/mach-shmobile/setup-r8a7791.c
+++ work/arch/arm/mach-shmobile/setup-r8a7791.c	2014-04-17 16:32:32.000000000 +0900
@@ -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), \

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Geert Uytterhoeven]

Hi Magnus,

On Mon, Apr 21, 2014 at 3:31 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> 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.

[...]

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

Good, thanks!

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

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

> --- 0001/arch/arm/mach-shmobile/setup-r8a7791.c
> +++ work/arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900
> @@ -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...

>  #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";

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

> +                       prop->value = disabled;
> +                       of_update_property(np, prop);
> +                       printk("xxx disabled %s\n", str);

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

> +               }
> +               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 -- geert@linux-m68k.org

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

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Monday 21 April 2014 10:31:14 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> 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.

> 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 <damm+renesas@opensource.se>
> ---
> 
>  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(-)
> 
> --- 0001/arch/arm/boot/dts/r8a7791-koelsch.dts
> +++ work/arch/arm/boot/dts/r8a7791-koelsch.dts	2014-04-17 
17:02:42.000000000
> +0900 @@ -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";
> --- 0001/arch/arm/mach-shmobile/setup-r8a7791.c
> +++ work/arch/arm/mach-shmobile/setup-r8a7791.c	2014-04-17
> 16:32:32.000000000 +0900 @@ -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), \

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Apr 21, 2014 at 11:50 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 21 April 2014 10:31:14 Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> 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 -- geert@linux-m68k.org

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

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Laurent Pinchart]

Hi Geert,

On Monday 21 April 2014 12:22:02 Geert Uytterhoeven wrote:
> On Mon, Apr 21, 2014 at 11:50 AM, Laurent Pinchart wrote:
> > On Monday 21 April 2014 10:31:14 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> 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.

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.

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

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Apr 21, 2014 at 11:09 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> 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.

> 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 -- geert@linux-m68k.org

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

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Simon Horman]

On Mon, Apr 21, 2014 at 10:56:41AM +0200, Geert Uytterhoeven wrote:
> Hi Magnus,
> 
> On Mon, Apr 21, 2014 at 3:31 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> > 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.
> 
> [...]
> 
> > 1) It enables serial ports in the board specific DTS
> >   -> Nothing special here, except perhaps the preserved device order
> 
> Good, thanks!
> 
> > 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.

> > 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!
> 
> > --- 0001/arch/arm/mach-shmobile/setup-r8a7791.c
> > +++ work/arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900
> > @@ -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...
> 
> >  #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";
> 
> > +       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.
> 
> > +                       prop->value = disabled;
> > +                       of_update_property(np, prop);
> > +                       printk("xxx disabled %s\n", str);
> 
> pr_info(), drop the "xxx ", capitalize "Disabled".
> 
> > +               }
> > +               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 -- geert@linux-m68k.org
> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Magnus Damm]

Hi Geert,

On Mon, Apr 21, 2014 at 5:56 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Mon, Apr 21, 2014 at 3:31 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> 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.
>
> [...]
>
>> 1) It enables serial ports in the board specific DTS
>>   -> Nothing special here, except perhaps the preserved device order
>
> Good, thanks!

Thanks! =)

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

>> --- 0001/arch/arm/mach-shmobile/setup-r8a7791.c
>> +++ work/arch/arm/mach-shmobile/setup-r8a7791.c 2014-04-17 16:32:32.000000000 +0900
>> @@ -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...
>
>>  #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";
>
>> +       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.
>
>> +                       prop->value = disabled;
>> +                       of_update_property(np, prop);
>> +                       printk("xxx disabled %s\n", str);
>
> pr_info(), drop the "xxx ", capitalize "Disabled".
>
>> +               }
>> +               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

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Magnus Damm]

Hi Laurent,

On Mon, Apr 21, 2014 at 6:50 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Monday 21 April 2014 10:31:14 Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> 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

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Laurent Pinchart]

Hi Magnus,

On Wednesday 23 April 2014 11:07:25 Magnus Damm wrote:
> On Mon, Apr 21, 2014 at 6:50 PM, Laurent Pinchart wrote:
> > On Monday 21 April 2014 10:31:14 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> 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

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.

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

> Let us discuss more face to face at ELC!

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Magnus Damm]

Hi Laurent,

On Wed, Apr 23, 2014 at 9:00 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Wednesday 23 April 2014 11:07:25 Magnus Damm wrote:
>> On Mon, Apr 21, 2014 at 6:50 PM, Laurent Pinchart wrote:
>> > On Monday 21 April 2014 10:31:14 Magnus Damm wrote:
>> >> From: Magnus Damm <damm@opensource.se>
>> >>
>> >> 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
>
> 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.

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

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

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Magnus Damm]

Hi Laurent, everyone,

On Wed, Apr 23, 2014 at 9:22 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> Hi Laurent,
>
> On Wed, Apr 23, 2014 at 9:00 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi Magnus,
>>
>> On Wednesday 23 April 2014 11:07:25 Magnus Damm wrote:
>>> On Mon, Apr 21, 2014 at 6:50 PM, Laurent Pinchart wrote:
>>> > On Monday 21 April 2014 10:31:14 Magnus Damm wrote:
>>> >> From: Magnus Damm <damm@opensource.se>
>>> >>
>>> >> 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
>>
>> 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.
>
>> 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... =)
>
>>> 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?

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

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Sergei Shtylyov]

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

[...]

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

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

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

> / magnus

WBR, Sergei

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Simon Horman]

On Thu, Apr 24, 2014 at 02:18:30AM +0400, Sergei Shtylyov wrote:
> On 04/23/2014 04:22 PM, Magnus Damm wrote:
> 
> [...]
> 
> >>>>>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
> 
> >>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.
> 
>    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.

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Magnus Damm]

On Thu, Apr 24, 2014 at 7:18 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 04/23/2014 04:22 PM, Magnus Damm wrote:
>
> [...]
>
>
>>>>>> 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
>
>
>>> 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.
>
>
>    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

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Geert Uytterhoeven]

On Thu, Apr 24, 2014 at 12:18 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
>> 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 -- geert@linux-m68k.org

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

Re: [PATCH/RFC] ARM: shmobile: Koelsch DT serial integration prototype

[Sergei Shtylyov]

Hello.

On 04/24/2014 09:45 AM, Magnus Damm wrote:

>> [...]

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

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

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

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

> / magnus

WBR, Sergei