diff mbox

[v2,2/2] of: support passing console options with stdout-path

Message ID 1417023640-23464-3-git-send-email-leif.lindholm@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Leif Lindholm Nov. 26, 2014, 5:40 p.m. UTC
Support specifying console options (like with console=ttyXN,<options>)
by appending them to the stdout-path property after a separating ':'.

Example:
        stdout-path = "uart0:115200";

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 drivers/of/base.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Andrew Lunn Nov. 26, 2014, 6:30 p.m. UTC | #1
On Wed, Nov 26, 2014 at 05:40:40PM +0000, Leif Lindholm wrote:
> Support specifying console options (like with console=ttyXN,<options>)
> by appending them to the stdout-path property after a separating ':'.
> 
> Example:
>         stdout-path = "uart0:115200";

Hi Leif

This should be documented somewhere under
Documentation/devicetree/bindings/

Not sure where thought. Maybe a top level chosen.txt?

    Andrew
Grant Likely Nov. 26, 2014, 9:07 p.m. UTC | #2
On Wed, Nov 26, 2014 at 6:30 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Nov 26, 2014 at 05:40:40PM +0000, Leif Lindholm wrote:
>> Support specifying console options (like with console=ttyXN,<options>)
>> by appending them to the stdout-path property after a separating ':'.
>>
>> Example:
>>         stdout-path = "uart0:115200";
>
> Hi Leif
>
> This should be documented somewhere under
> Documentation/devicetree/bindings/
>
> Not sure where thought. Maybe a top level chosen.txt?

Actually, this one doesn't. It is already documented in ePAPR

g.
Andrew Lunn Nov. 26, 2014, 9:48 p.m. UTC | #3
On Wed, Nov 26, 2014 at 09:07:33PM +0000, Grant Likely wrote:
> On Wed, Nov 26, 2014 at 6:30 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Wed, Nov 26, 2014 at 05:40:40PM +0000, Leif Lindholm wrote:
> >> Support specifying console options (like with console=ttyXN,<options>)
> >> by appending them to the stdout-path property after a separating ':'.
> >>
> >> Example:
> >>         stdout-path = "uart0:115200";
> >
> > Hi Leif
> >
> > This should be documented somewhere under
> > Documentation/devicetree/bindings/
> >
> > Not sure where thought. Maybe a top level chosen.txt?
> 
> Actually, this one doesn't. It is already documented in ePAPR

Hi Grant

Humm, do i have an old version of ePAPR?

All i see is that in Table 3-4 It says:

stdout-path O <string> A string that specifies the full path to the
	      	       node representing the device to be used for
	      	       boot console output. If the character ":" is
	      	       present in the value it terminates the
	      	       path. The value may be an alias.

		       If the stdin-path property is not specified,
		       stdout-path should be assumed to define the input device.

So what is before the : is defined. What comes afterwards,
baudrate/parity/bits/flow control does not appear to the defined in
ePAPR. Should we not document the extension being added here?

       Andrew
Mark Rutland Nov. 27, 2014, 12:15 p.m. UTC | #4
On Wed, Nov 26, 2014 at 09:48:47PM +0000, Andrew Lunn wrote:
> On Wed, Nov 26, 2014 at 09:07:33PM +0000, Grant Likely wrote:
> > On Wed, Nov 26, 2014 at 6:30 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Wed, Nov 26, 2014 at 05:40:40PM +0000, Leif Lindholm wrote:
> > >> Support specifying console options (like with console=ttyXN,<options>)
> > >> by appending them to the stdout-path property after a separating ':'.
> > >>
> > >> Example:
> > >>         stdout-path = "uart0:115200";
> > >
> > > Hi Leif
> > >
> > > This should be documented somewhere under
> > > Documentation/devicetree/bindings/
> > >
> > > Not sure where thought. Maybe a top level chosen.txt?
> > 
> > Actually, this one doesn't. It is already documented in ePAPR
> 
> Hi Grant
> 
> Humm, do i have an old version of ePAPR?
> 
> All i see is that in Table 3-4 It says:
> 
> stdout-path O <string> A string that specifies the full path to the
> 	      	       node representing the device to be used for
> 	      	       boot console output. If the character ":" is
> 	      	       present in the value it terminates the
> 	      	       path. The value may be an alias.
> 
> 		       If the stdin-path property is not specified,
> 		       stdout-path should be assumed to define the input device.
> 
> So what is before the : is defined. What comes afterwards,
> baudrate/parity/bits/flow control does not appear to the defined in
> ePAPR. Should we not document the extension being added here?

I believe that we should, and it should be relatively trivial to add a
document stating that the format and meaning of the parts after the ':'
are device-specific. 

So how about Documentation/devicetree/bindings/serial/stdout-path.txt,
with something like the following:

---->8----
Device trees may specify the device to be used for boot console output
with a stdout-path property under /chosen, as described in ePAPR, e.g.

/ {
	chosen {
		stdout-path = "/serial@f00:115200";
	};

	serial@f00 {
		compatible = "vendor,some-uart";
		reg = <0xf00 0x10>;
	};
};

If the character ":" is present in the value, this terminates the path.
The meaning of any cahracters following the ":" is device-specific, and
must be specified in the relevant binding documentation.
---->8----

The more difficult part is documenting those (and I'm still uneasy about
conflating the Linux driver command line options with the DT binding for
that reason).

Thanks,
Mark.
Leif Lindholm Nov. 27, 2014, 1:16 p.m. UTC | #5
On Thu, Nov 27, 2014 at 12:15:43PM +0000, Mark Rutland wrote:
> On Wed, Nov 26, 2014 at 09:48:47PM +0000, Andrew Lunn wrote:
> > On Wed, Nov 26, 2014 at 09:07:33PM +0000, Grant Likely wrote:
> > > On Wed, Nov 26, 2014 at 6:30 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > > > On Wed, Nov 26, 2014 at 05:40:40PM +0000, Leif Lindholm wrote:
> > > >> Support specifying console options (like with console=ttyXN,<options>)
> > > >> by appending them to the stdout-path property after a separating ':'.
> > > >>
> > > >> Example:
> > > >>         stdout-path = "uart0:115200";
> > > >
> > > > Hi Leif
> > > >
> > > > This should be documented somewhere under
> > > > Documentation/devicetree/bindings/
> > > >
> > > > Not sure where thought. Maybe a top level chosen.txt?
> > > 
> > > Actually, this one doesn't. It is already documented in ePAPR
> > 
> > Hi Grant
> > 
> > Humm, do i have an old version of ePAPR?
> > 
> > All i see is that in Table 3-4 It says:
> > 
> > stdout-path O <string> A string that specifies the full path to the
> > 	      	       node representing the device to be used for
> > 	      	       boot console output. If the character ":" is
> > 	      	       present in the value it terminates the
> > 	      	       path. The value may be an alias.
> > 
> > 		       If the stdin-path property is not specified,
> > 		       stdout-path should be assumed to define the input device.
> > 
> > So what is before the : is defined. What comes afterwards,
> > baudrate/parity/bits/flow control does not appear to the defined in
> > ePAPR. Should we not document the extension being added here?
> 
> I believe that we should, and it should be relatively trivial to add a
> document stating that the format and meaning of the parts after the ':'
> are device-specific. 

Device-specific is a bit broad though?

> So how about Documentation/devicetree/bindings/serial/stdout-path.txt,
> with something like the following:

There is, however, nothing serial-specific about this functionality -
it console-specific.

Could it be bindings/console/stdout-path.txt?
 
> ---->8----
> Device trees may specify the device to be used for boot console output
> with a stdout-path property under /chosen, as described in ePAPR, e.g.
> 
> / {
> 	chosen {
> 		stdout-path = "/serial@f00:115200";
> 	};
> 
> 	serial@f00 {
> 		compatible = "vendor,some-uart";
> 		reg = <0xf00 0x10>;
> 	};
> };
> 
> If the character ":" is present in the value, this terminates the path.
> The meaning of any cahracters following the ":" is device-specific, and
> must be specified in the relevant binding documentation.
> ---->8----
> 
> The more difficult part is documenting those (and I'm still uneasy about
> conflating the Linux driver command line options with the DT binding for
> that reason).

For the current situation, which _is_ serial-specific, could we then
add a serial/stdout-path.txt describing the mapping to
uart_parse_options - making that interface an implicit requirement for
the use of stdout-path?

/
    Leif
Grant Likely Nov. 27, 2014, 1:39 p.m. UTC | #6
On Wed, Nov 26, 2014 at 9:48 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Nov 26, 2014 at 09:07:33PM +0000, Grant Likely wrote:
>> On Wed, Nov 26, 2014 at 6:30 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > On Wed, Nov 26, 2014 at 05:40:40PM +0000, Leif Lindholm wrote:
>> >> Support specifying console options (like with console=ttyXN,<options>)
>> >> by appending them to the stdout-path property after a separating ':'.
>> >>
>> >> Example:
>> >>         stdout-path = "uart0:115200";
>> >
>> > Hi Leif
>> >
>> > This should be documented somewhere under
>> > Documentation/devicetree/bindings/
>> >
>> > Not sure where thought. Maybe a top level chosen.txt?
>>
>> Actually, this one doesn't. It is already documented in ePAPR
>
> Hi Grant
>
> Humm, do i have an old version of ePAPR?
>
> All i see is that in Table 3-4 It says:
>
> stdout-path O <string> A string that specifies the full path to the
>                        node representing the device to be used for
>                        boot console output. If the character ":" is
>                        present in the value it terminates the
>                        path. The value may be an alias.
>
>                        If the stdin-path property is not specified,
>                        stdout-path should be assumed to define the input device.
>
> So what is before the : is defined. What comes afterwards,
> baudrate/parity/bits/flow control does not appear to the defined in
> ePAPR. Should we not document the extension being added here?

Ah, good point. I had thought you were referring to the argument separator.

Yes, the extension should be documented. The binding can actually be
driver specific, but there is little to no reason for each uart driver
to be unique in this regard and the b/p/b/f format is very well
established, so I agree. Other drivers, say video out or netcon, could
use different arguments.

g.
Mark Rutland Nov. 27, 2014, 1:41 p.m. UTC | #7
On Thu, Nov 27, 2014 at 01:16:36PM +0000, Leif Lindholm wrote:
> On Thu, Nov 27, 2014 at 12:15:43PM +0000, Mark Rutland wrote:
> > On Wed, Nov 26, 2014 at 09:48:47PM +0000, Andrew Lunn wrote:
> > > On Wed, Nov 26, 2014 at 09:07:33PM +0000, Grant Likely wrote:
> > > > On Wed, Nov 26, 2014 at 6:30 PM, Andrew Lunn <andrew@lunn.ch> wrote:
> > > > > On Wed, Nov 26, 2014 at 05:40:40PM +0000, Leif Lindholm wrote:
> > > > >> Support specifying console options (like with console=ttyXN,<options>)
> > > > >> by appending them to the stdout-path property after a separating ':'.
> > > > >>
> > > > >> Example:
> > > > >>         stdout-path = "uart0:115200";
> > > > >
> > > > > Hi Leif
> > > > >
> > > > > This should be documented somewhere under
> > > > > Documentation/devicetree/bindings/
> > > > >
> > > > > Not sure where thought. Maybe a top level chosen.txt?
> > > > 
> > > > Actually, this one doesn't. It is already documented in ePAPR
> > > 
> > > Hi Grant
> > > 
> > > Humm, do i have an old version of ePAPR?
> > > 
> > > All i see is that in Table 3-4 It says:
> > > 
> > > stdout-path O <string> A string that specifies the full path to the
> > > 	      	       node representing the device to be used for
> > > 	      	       boot console output. If the character ":" is
> > > 	      	       present in the value it terminates the
> > > 	      	       path. The value may be an alias.
> > > 
> > > 		       If the stdin-path property is not specified,
> > > 		       stdout-path should be assumed to define the input device.
> > > 
> > > So what is before the : is defined. What comes afterwards,
> > > baudrate/parity/bits/flow control does not appear to the defined in
> > > ePAPR. Should we not document the extension being added here?
> > 
> > I believe that we should, and it should be relatively trivial to add a
> > document stating that the format and meaning of the parts after the ':'
> > are device-specific. 
> 
> Device-specific is a bit broad though?

If we have a generic set of properties, then I'm happy to have that.
Otherwise I assumed that the portion after the ':' would be interpreted
w.r.t. the binding for the device being pointed to.

> > So how about Documentation/devicetree/bindings/serial/stdout-path.txt,
> > with something like the following:
> 
> There is, however, nothing serial-specific about this functionality -
> it console-specific.
> 
> Could it be bindings/console/stdout-path.txt?

I am in no way attached to the path. But to bikeshed a little, I would
expect (or at least I would hope) not to have an awful lot under
bindings/console, given that consoles are a SW construct. So we could
just drop this right under bindings/stdout-path.txt for now until we
find a better taxonomy.

>  
> > ---->8----
> > Device trees may specify the device to be used for boot console output
> > with a stdout-path property under /chosen, as described in ePAPR, e.g.
> > 
> > / {
> > 	chosen {
> > 		stdout-path = "/serial@f00:115200";
> > 	};
> > 
> > 	serial@f00 {
> > 		compatible = "vendor,some-uart";
> > 		reg = <0xf00 0x10>;
> > 	};
> > };
> > 
> > If the character ":" is present in the value, this terminates the path.
> > The meaning of any cahracters following the ":" is device-specific, and
> > must be specified in the relevant binding documentation.
> > ---->8----
> > 
> > The more difficult part is documenting those (and I'm still uneasy about
> > conflating the Linux driver command line options with the DT binding for
> > that reason).
> 
> For the current situation, which _is_ serial-specific, could we then
> add a serial/stdout-path.txt describing the mapping to
> uart_parse_options - making that interface an implicit requirement for
> the use of stdout-path?

That would depend on what you mean by "the mapping to
uart_parse_options". The binding should _not_ refer to Linux internals,
just the parts visible to an arbitrary DT author/parseer.

So we'd need to document which strings you can have after the ':' and
what they mean -- I don't want to see that defined by reference to
Linux-specific command line options because people will change that
without thinking.

Mark.
Grant Likely Nov. 27, 2014, 1:45 p.m. UTC | #8
On Thu, Nov 27, 2014 at 1:16 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Nov 27, 2014 at 12:15:43PM +0000, Mark Rutland wrote:
>> On Wed, Nov 26, 2014 at 09:48:47PM +0000, Andrew Lunn wrote:
>> > On Wed, Nov 26, 2014 at 09:07:33PM +0000, Grant Likely wrote:
>> > > On Wed, Nov 26, 2014 at 6:30 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>> > > > On Wed, Nov 26, 2014 at 05:40:40PM +0000, Leif Lindholm wrote:
>> > > >> Support specifying console options (like with console=ttyXN,<options>)
>> > > >> by appending them to the stdout-path property after a separating ':'.
>> > > >>
>> > > >> Example:
>> > > >>         stdout-path = "uart0:115200";
>> > > >
>> > > > Hi Leif
>> > > >
>> > > > This should be documented somewhere under
>> > > > Documentation/devicetree/bindings/
>> > > >
>> > > > Not sure where thought. Maybe a top level chosen.txt?
>> > >
>> > > Actually, this one doesn't. It is already documented in ePAPR
>> >
>> > Hi Grant
>> >
>> > Humm, do i have an old version of ePAPR?
>> >
>> > All i see is that in Table 3-4 It says:
>> >
>> > stdout-path O <string> A string that specifies the full path to the
>> >                    node representing the device to be used for
>> >                    boot console output. If the character ":" is
>> >                    present in the value it terminates the
>> >                    path. The value may be an alias.
>> >
>> >                    If the stdin-path property is not specified,
>> >                    stdout-path should be assumed to define the input device.
>> >
>> > So what is before the : is defined. What comes afterwards,
>> > baudrate/parity/bits/flow control does not appear to the defined in
>> > ePAPR. Should we not document the extension being added here?
>>
>> I believe that we should, and it should be relatively trivial to add a
>> document stating that the format and meaning of the parts after the ':'
>> are device-specific.
>
> Device-specific is a bit broad though?

Not really, but the expectation should be quite strong that for UARTs,
the first argument after the ':' is the b/p/b/f string. We could also
specify that ',' is used as a separator when multiple arguments are to
be passed to a device.

>
>> So how about Documentation/devicetree/bindings/serial/stdout-path.txt,
>> with something like the following:
>
> There is, however, nothing serial-specific about this functionality -
> it console-specific.
>
> Could it be bindings/console/stdout-path.txt?
>
>> ---->8----
>> Device trees may specify the device to be used for boot console output
>> with a stdout-path property under /chosen, as described in ePAPR, e.g.
>>
>> / {
>>       chosen {
>>               stdout-path = "/serial@f00:115200";
>>       };
>>
>>       serial@f00 {
>>               compatible = "vendor,some-uart";
>>               reg = <0xf00 0x10>;
>>       };
>> };
>>
>> If the character ":" is present in the value, this terminates the path.
>> The meaning of any cahracters following the ":" is device-specific, and
>> must be specified in the relevant binding documentation.
>> ---->8----
>>
>> The more difficult part is documenting those (and I'm still uneasy about
>> conflating the Linux driver command line options with the DT binding for
>> that reason).
>
> For the current situation, which _is_ serial-specific, could we then
> add a serial/stdout-path.txt describing the mapping to
> uart_parse_options - making that interface an implicit requirement for
> the use of stdout-path?

Put it in Documentation/devicetree/bindings/chosen.txt

We may as well have one file for all the common stuff that goes into chosen.

g.
diff mbox

Patch

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 3e764bd..d265514 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -37,6 +37,7 @@  EXPORT_SYMBOL(of_allnodes);
 struct device_node *of_chosen;
 struct device_node *of_aliases;
 struct device_node *of_stdout;
+static char *of_stdout_options;
 
 struct kset *of_kset;
 
@@ -1844,7 +1845,7 @@  void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align))
 		if (IS_ENABLED(CONFIG_PPC) && !name)
 			name = of_get_property(of_aliases, "stdout", NULL);
 		if (name)
-			of_stdout = of_find_node_by_path(name, NULL);
+			of_stdout = of_find_node_by_path(name, &of_stdout_options);
 	}
 
 	if (!of_aliases)
@@ -1970,7 +1971,7 @@  bool of_console_check(struct device_node *dn, char *name, int index)
 {
 	if (!dn || dn != of_stdout || console_set_on_cmdline)
 		return false;
-	return !add_preferred_console(name, index, NULL);
+	return !add_preferred_console(name, index, of_stdout_options);
 }
 EXPORT_SYMBOL_GPL(of_console_check);