linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] earlycon: initialise baud field of earlycon device structure
@ 2017-08-15 17:21 Eugeniy Paltsev
  2017-08-15 19:26 ` Rob Herring
  0 siblings, 1 reply; 5+ messages in thread
From: Eugeniy Paltsev @ 2017-08-15 17:21 UTC (permalink / raw)
  To: linux-serial
  Cc: linux-snps-arc, linux-kernel, Greg Kroah-Hartman, Jiri Slaby,
	Eugeniy Paltsev

For now baud field of earlycon structure device is't initialised at all
in of_setup_earlycon (in oppositе to register_earlycon).

So when I use stdout-path to point earlycon device
(like stdout-path = &serial or stdout-path = "serial:115200n8")
baud field of earlycon device structure remains uninitialised and
earlycon initialization is not performed correctly as
of_setup_earlycon is used.
When pass all arguments via bootargs
(like bootargs = "earlycon=uart8250,mmio32,0xf0005000,115200n8")
initialization is performed correctly as register_earlycon is used.

So initialise baud field of earlycon device structure by baud value
from device tree or from options (if they exist) when we use
of_setup_earlycon

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/tty/serial/earlycon.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index c365154..6c2e2b6 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -240,6 +240,7 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
 {
 	int err;
 	struct uart_port *port = &early_console_dev.port;
+	unsigned long baud;
 	const __be32 *val;
 	bool big_endian;
 	u64 addr;
@@ -282,7 +283,15 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
 		}
 	}
 
+	val = of_get_flat_dt_prop(node, "baud", NULL);
+	if (val)
+		early_console_dev.baud = be32_to_cpu(*val);
+
 	if (options) {
+		err = kstrtoul(options, 10, &baud);
+		if (!err)
+			early_console_dev.baud = baud;
+
 		strlcpy(early_console_dev.options, options,
 			sizeof(early_console_dev.options));
 	}
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] earlycon: initialise baud field of earlycon device structure
  2017-08-15 17:21 [PATCH] earlycon: initialise baud field of earlycon device structure Eugeniy Paltsev
@ 2017-08-15 19:26 ` Rob Herring
  2017-08-16 11:52   ` Eugeniy Paltsev
  0 siblings, 1 reply; 5+ messages in thread
From: Rob Herring @ 2017-08-15 19:26 UTC (permalink / raw)
  To: Eugeniy Paltsev
  Cc: linux-serial, arcml, linux-kernel, Greg Kroah-Hartman, Jiri Slaby

On Tue, Aug 15, 2017 at 12:21 PM, Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
> For now baud field of earlycon structure device is't initialised at all
> in of_setup_earlycon (in oppositе to register_earlycon).
>
> So when I use stdout-path to point earlycon device
> (like stdout-path = &serial or stdout-path = "serial:115200n8")
> baud field of earlycon device structure remains uninitialised and
> earlycon initialization is not performed correctly as
> of_setup_earlycon is used.

Because the console driver is supposed to parse the option string.
That allows in theory for the options to be specific for each console.
Maybe your earlycon driver is failing to do that.

> When pass all arguments via bootargs
> (like bootargs = "earlycon=uart8250,mmio32,0xf0005000,115200n8")
> initialization is performed correctly as register_earlycon is used.
>
> So initialise baud field of earlycon device structure by baud value
> from device tree or from options (if they exist) when we use
> of_setup_earlycon
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  drivers/tty/serial/earlycon.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index c365154..6c2e2b6 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -240,6 +240,7 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
>  {
>         int err;
>         struct uart_port *port = &early_console_dev.port;
> +       unsigned long baud;
>         const __be32 *val;
>         bool big_endian;
>         u64 addr;
> @@ -282,7 +283,15 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
>                 }
>         }
>
> +       val = of_get_flat_dt_prop(node, "baud", NULL);

No, we already have a defined way to set the baud, we don't need a
property in addition. Plus you didn't document it.

> +       if (val)
> +               early_console_dev.baud = be32_to_cpu(*val);
> +
>         if (options) {
> +               err = kstrtoul(options, 10, &baud);
> +               if (!err)
> +                       early_console_dev.baud = baud;

This seems fine to do here, but then we should also parse the other
standard options here too. And we should make sure we're not doing it
twice.

> +
>                 strlcpy(early_console_dev.options, options,
>                         sizeof(early_console_dev.options));
>         }
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] earlycon: initialise baud field of earlycon device structure
  2017-08-15 19:26 ` Rob Herring
@ 2017-08-16 11:52   ` Eugeniy Paltsev
  2017-08-16 13:46     ` Rob Herring
  0 siblings, 1 reply; 5+ messages in thread
From: Eugeniy Paltsev @ 2017-08-16 11:52 UTC (permalink / raw)
  To: robherring2
  Cc: linux-serial, linux-kernel, jslaby, Eugeniy.Paltsev, gregkh,
	linux-snps-arc

Hi Rob,

On Tue, 2017-08-15 at 14:26 -0500, Rob Herring wrote:
> On Tue, Aug 15, 2017 at 12:21 PM, Eugeniy Paltsev
> <Eugeniy.Paltsev@synopsys.com> wrote:
> > [snip]
> > @@ -282,7 +283,15 @@ int __init of_setup_earlycon(const struct
> > earlycon_id *match,
> >                 }
> >         }
> > 
> > +       val = of_get_flat_dt_prop(node, "baud", NULL);
> 
> No, we already have a defined way to set the baud, we don't need a
> property in addition. Plus you didn't document it.

I guess by defined way to set the baud you mean setting baud after
device alias
in stdout-path property (like stdout-path = "serial:115200n8"), right?

The idea was to reuse "baud" property from serial node to set the
earlycon baud:

chosen {
    ...
    stdout-path = &serial;
};

serial: uart@... {
    ...
    baud = <115200>;   /* Get baud from here */
};

> > +       if (val)
> > +               early_console_dev.baud = be32_to_cpu(*val);
> > +
> >         if (options) {
> > +               err = kstrtoul(options, 10, &baud);
> > +               if (!err)
> > +                       early_console_dev.baud = baud;
> 
> This seems fine to do here, but then we should also parse the other
> standard options here too. And we should make sure we're not doing it
> twice.
I added only baud parsing here because we parse only baud from standard
options
when register_earlycon is used. (see parse_options function which is
called
from register_earlycon)

But I can add other standard options parsing here (probably using
uart_parse_options + uart_set_options). 
What do you think?

> > +
> >                 strlcpy(early_console_dev.options, options,
> >                         sizeof(early_console_dev.options));
> >         }
> > --
> > 2.9.3
-- 
 Eugeniy Paltsev

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] earlycon: initialise baud field of earlycon device structure
  2017-08-16 11:52   ` Eugeniy Paltsev
@ 2017-08-16 13:46     ` Rob Herring
  2017-08-16 15:26       ` Eugeniy Paltsev
  0 siblings, 1 reply; 5+ messages in thread
From: Rob Herring @ 2017-08-16 13:46 UTC (permalink / raw)
  To: Eugeniy Paltsev
  Cc: linux-serial, linux-kernel, jslaby, gregkh, linux-snps-arc

On Wed, Aug 16, 2017 at 6:52 AM, Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
> Hi Rob,
>
> On Tue, 2017-08-15 at 14:26 -0500, Rob Herring wrote:
>> On Tue, Aug 15, 2017 at 12:21 PM, Eugeniy Paltsev
>> <Eugeniy.Paltsev@synopsys.com> wrote:
>> > [snip]
>> > @@ -282,7 +283,15 @@ int __init of_setup_earlycon(const struct
>> > earlycon_id *match,
>> >                 }
>> >         }
>> >
>> > +       val = of_get_flat_dt_prop(node, "baud", NULL);
>>
>> No, we already have a defined way to set the baud, we don't need a
>> property in addition. Plus you didn't document it.
>
> I guess by defined way to set the baud you mean setting baud after
> device alias
> in stdout-path property (like stdout-path = "serial:115200n8"), right?
>
> The idea was to reuse "baud" property from serial node to set the
> earlycon baud:
>
> chosen {
>     ...
>     stdout-path = &serial;
> };
>
> serial: uart@... {
>     ...
>     baud = <115200>;   /* Get baud from here */

"current-speed" is already defined for this purpose. If you want to
add that, that's fine.

> };
>
>> > +       if (val)
>> > +               early_console_dev.baud = be32_to_cpu(*val);
>> > +
>> >         if (options) {
>> > +               err = kstrtoul(options, 10, &baud);
>> > +               if (!err)
>> > +                       early_console_dev.baud = baud;
>>
>> This seems fine to do here, but then we should also parse the other
>> standard options here too. And we should make sure we're not doing it
>> twice.
> I added only baud parsing here because we parse only baud from standard
> options
> when register_earlycon is used. (see parse_options function which is
> called
> from register_earlycon)
>
> But I can add other standard options parsing here (probably using
> uart_parse_options + uart_set_options).
> What do you think?

That seems fine as long as consoles can still have their own options.

Rob

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] earlycon: initialise baud field of earlycon device structure
  2017-08-16 13:46     ` Rob Herring
@ 2017-08-16 15:26       ` Eugeniy Paltsev
  0 siblings, 0 replies; 5+ messages in thread
From: Eugeniy Paltsev @ 2017-08-16 15:26 UTC (permalink / raw)
  To: robherring2
  Cc: linux-serial, linux-kernel, jslaby, Eugeniy.Paltsev, gregkh,
	linux-snps-arc

On Wed, 2017-08-16 at 08:46 -0500, Rob Herring wrote:
> On Wed, Aug 16, 2017 at 6:52 AM, Eugeniy Paltsev
> <Eugeniy.Paltsev@synopsys.com> wrote:
> > Hi Rob,
> > 
> > On Tue, 2017-08-15 at 14:26 -0500, Rob Herring wrote:
> > > On Tue, Aug 15, 2017 at 12:21 PM, Eugeniy Paltsev
> > > <Eugeniy.Paltsev@synopsys.com> wrote:
> > > > [snip]
> > > > @@ -282,7 +283,15 @@ int __init of_setup_earlycon(const struct
> > > > earlycon_id *match,
> > > >                 }
> > > >         }
> > > > 
> > > > +       val = of_get_flat_dt_prop(node, "baud", NULL);
> > > 
> > > No, we already have a defined way to set the baud, we don't need
> > > a
> > > property in addition. Plus you didn't document it.
> > 
> > I guess by defined way to set the baud you mean setting baud after
> > device alias
> > in stdout-path property (like stdout-path = "serial:115200n8"),
> > right?
> > 
> > The idea was to reuse "baud" property from serial node to set the
> > earlycon baud:
> > 
> > chosen {
> >     ...
> >     stdout-path = &serial;
> > };
> > 
> > serial: uart@... {
> >     ...
> >     baud = <115200>;   /* Get baud from here */
> 
> "current-speed" is already defined for this purpose. If you want to
> add that, that's fine.
Ok, I'll add "current-speed".

> > };
> > 
> > > > +       if (val)
> > > > +               early_console_dev.baud = be32_to_cpu(*val);
> > > > +
> > > >         if (options) {
> > > > +               err = kstrtoul(options, 10, &baud);
> > > > +               if (!err)
> > > > +                       early_console_dev.baud = baud;
> > > 
> > > This seems fine to do here, but then we should also parse the
> > > other
> > > standard options here too. And we should make sure we're not
> > > doing it
> > > twice.
> > 
> > I added only baud parsing here because we parse only baud from
> > standard
> > options
> > when register_earlycon is used. (see parse_options function which
> > is
> > called
> > from register_earlycon)
> > 
> > But I can add other standard options parsing here (probably using
> > uart_parse_options + uart_set_options).
> > What do you think?
> 
> That seems fine as long as consoles can still have their own options.
Ok.

Should I also add standard options parsing to register_earlycon?
(as we parse only baud from standard options in register_earlycon)

> Rob
-- 
 Eugeniy Paltsev

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-08-16 15:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-15 17:21 [PATCH] earlycon: initialise baud field of earlycon device structure Eugeniy Paltsev
2017-08-15 19:26 ` Rob Herring
2017-08-16 11:52   ` Eugeniy Paltsev
2017-08-16 13:46     ` Rob Herring
2017-08-16 15:26       ` Eugeniy Paltsev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).