diff mbox

[v2,1/4] tty: serial: Add 8250 earlycon to support noinit option

Message ID 54CE6FD9.5030606@hurleysoftware.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Hurley Feb. 1, 2015, 6:26 p.m. UTC
On 02/01/2015 11:27 AM, Peter Hurley wrote:
> Hi Eddie,
> 
> On 01/12/2015 08:08 AM, Eddie Huang wrote:
>> Add earlycon support not only baudrate option, but also add noinit option.
>> If use noinit option, 8250 earlycon will not init serial hardware and use
>> loader setting.
> 
> I see this went into Greg's tty-testing branch.
> 
> The only point of this is to not program the divisor, right?
> 
> I ask because early_serial8250_setup() could already handle this without
> extra options by simply not doing divisor programming if no baud option is
> present.

Does the patch below work for your use-case?

[ Note: the patch applies to 3.19-rcX. To test, your noinit patches need to be
  reverted first.
]

--- >% ---
From: Peter Hurley <peter@hurleysoftware.com>
Subject: [PATCH] serial: 8250_early: Assume uart already initialized if no
 baud option

The <baud><parity><bit> option string is not supplied if the earlycon
is started via devicetree and OF_EARLYCON_DECLARE(). The option string
is also not required if started via kernel command line parameters of
the form:
  earlycon=uart,mmio,<addr>
  console=uart,mmio,<addr>

If earlycon_device->baud is 0, then an option string was not supplied.
In this case, assume the uart has already been initialized by the
bootloader or firmware.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/8250/8250_early.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Eddie Huang (黃智傑) Feb. 2, 2015, 3:16 a.m. UTC | #1
Hi Peter,

On Sun, 2015-02-01 at 13:26 -0500, Peter Hurley wrote:
> On 02/01/2015 11:27 AM, Peter Hurley wrote:
> > Hi Eddie,
> > 
> > On 01/12/2015 08:08 AM, Eddie Huang wrote:
> >> Add earlycon support not only baudrate option, but also add noinit option.
> >> If use noinit option, 8250 earlycon will not init serial hardware and use
> >> loader setting.
> > 
> > I see this went into Greg's tty-testing branch.
> > 
> > The only point of this is to not program the divisor, right?
> > 
> > I ask because early_serial8250_setup() could already handle this without
> > extra options by simply not doing divisor programming if no baud option is
> > present.
> 
> Does the patch below work for your use-case?
> 
> [ Note: the patch applies to 3.19-rcX. To test, your noinit patches need to be
>   reverted first.
> ]
> 
> --- >% ---
> From: Peter Hurley <peter@hurleysoftware.com>
> Subject: [PATCH] serial: 8250_early: Assume uart already initialized if no
>  baud option
> 
> The <baud><parity><bit> option string is not supplied if the earlycon
> is started via devicetree and OF_EARLYCON_DECLARE(). The option string
> is also not required if started via kernel command line parameters of
> the form:
>   earlycon=uart,mmio,<addr>
>   console=uart,mmio,<addr>
> 
> If earlycon_device->baud is 0, then an option string was not supplied.
> In this case, assume the uart has already been initialized by the
> bootloader or firmware.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/serial/8250/8250_early.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
> index d7b831b..1701d00 100644
> --- a/drivers/tty/serial/8250/8250_early.c
> +++ b/drivers/tty/serial/8250/8250_early.c
> @@ -149,12 +149,18 @@ static int __init early_serial8250_setup(struct earlycon_device *device,
>  		return 0;
>  
>  	if (!device->baud) {
> +		struct uart_port *port = &device->port;
> +		unsigned int ier;
> +
>  		device->baud = probe_baud(&device->port);
>  		snprintf(device->options, sizeof(device->options), "%u",
>  			 device->baud);
> -	}
>  
> -	init_port(device);
> +		/* assume the device was initialized, only mask interrupts */
> +		ier = serial8250_early_in(port, UART_IER);
> +		serial8250_early_out(port, UART_IER, ier & UART_IER_UUE);
> +	} else
> +		init_port(device);
Should add brace in else.

Where is original line here.
          early_device = device;
>  	device->con->write = early_serial8250_write;
>  	return 0;

With above comment, I test ok on my platform.

Eddie
Best Regards.
Peter Hurley Feb. 2, 2015, 3:45 a.m. UTC | #2
On 02/01/2015 10:16 PM, Eddie Huang wrote:
> Hi Peter,
> 
> On Sun, 2015-02-01 at 13:26 -0500, Peter Hurley wrote:
>> On 02/01/2015 11:27 AM, Peter Hurley wrote:
>>> Hi Eddie,
>>>
>>> On 01/12/2015 08:08 AM, Eddie Huang wrote:
>>>> Add earlycon support not only baudrate option, but also add noinit option.
>>>> If use noinit option, 8250 earlycon will not init serial hardware and use
>>>> loader setting.
>>>
>>> I see this went into Greg's tty-testing branch.
>>>
>>> The only point of this is to not program the divisor, right?
>>>
>>> I ask because early_serial8250_setup() could already handle this without
>>> extra options by simply not doing divisor programming if no baud option is
>>> present.
>>
>> Does the patch below work for your use-case?
>>
>> [ Note: the patch applies to 3.19-rcX. To test, your noinit patches need to be
>>   reverted first.
>> ]
>>
>> --- >% ---
>> From: Peter Hurley <peter@hurleysoftware.com>
>> Subject: [PATCH] serial: 8250_early: Assume uart already initialized if no
>>  baud option
>>
>> The <baud><parity><bit> option string is not supplied if the earlycon
>> is started via devicetree and OF_EARLYCON_DECLARE(). The option string
>> is also not required if started via kernel command line parameters of
>> the form:
>>   earlycon=uart,mmio,<addr>
>>   console=uart,mmio,<addr>
>>
>> If earlycon_device->baud is 0, then an option string was not supplied.
>> In this case, assume the uart has already been initialized by the
>> bootloader or firmware.
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>  drivers/tty/serial/8250/8250_early.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
>> index d7b831b..1701d00 100644
>> --- a/drivers/tty/serial/8250/8250_early.c
>> +++ b/drivers/tty/serial/8250/8250_early.c
>> @@ -149,12 +149,18 @@ static int __init early_serial8250_setup(struct earlycon_device *device,
>>  		return 0;
>>  
>>  	if (!device->baud) {
>> +		struct uart_port *port = &device->port;
>> +		unsigned int ier;
>> +
>>  		device->baud = probe_baud(&device->port);
>>  		snprintf(device->options, sizeof(device->options), "%u",
>>  			 device->baud);
>> -	}
>>  
>> -	init_port(device);
>> +		/* assume the device was initialized, only mask interrupts */
>> +		ier = serial8250_early_in(port, UART_IER);
>> +		serial8250_early_out(port, UART_IER, ier & UART_IER_UUE);
>> +	} else
>> +		init_port(device);
> Should add brace in else.

I don't do that unless I have to.

> Where is original line here.
>           early_device = device;

Whoops :)

I wrote the patch from a private branch which implements extensible console
matching (so a console can define its own match function) and a bunch of
other console cleanup and code removal. In that series, early_device becomes
unnecessary and is removed.

I'll respin proper patches on top of Greg's tty-testing branch with reverts
for the noinit options. I noticed that one of the noinit patches actually
has the linkage for the mtk earlycon, so I'll be sure to preserve that.

Regards,
Peter Hurley
Greg KH Feb. 2, 2015, 4:28 a.m. UTC | #3
On Sun, Feb 01, 2015 at 10:45:12PM -0500, Peter Hurley wrote:
> On 02/01/2015 10:16 PM, Eddie Huang wrote:
> > Hi Peter,
> > 
> > On Sun, 2015-02-01 at 13:26 -0500, Peter Hurley wrote:
> >> On 02/01/2015 11:27 AM, Peter Hurley wrote:
> >>> Hi Eddie,
> >>>
> >>> On 01/12/2015 08:08 AM, Eddie Huang wrote:
> >>>> Add earlycon support not only baudrate option, but also add noinit option.
> >>>> If use noinit option, 8250 earlycon will not init serial hardware and use
> >>>> loader setting.
> >>>
> >>> I see this went into Greg's tty-testing branch.
> >>>
> >>> The only point of this is to not program the divisor, right?
> >>>
> >>> I ask because early_serial8250_setup() could already handle this without
> >>> extra options by simply not doing divisor programming if no baud option is
> >>> present.
> >>
> >> Does the patch below work for your use-case?
> >>
> >> [ Note: the patch applies to 3.19-rcX. To test, your noinit patches need to be
> >>   reverted first.
> >> ]
> >>
> >> --- >% ---
> >> From: Peter Hurley <peter@hurleysoftware.com>
> >> Subject: [PATCH] serial: 8250_early: Assume uart already initialized if no
> >>  baud option
> >>
> >> The <baud><parity><bit> option string is not supplied if the earlycon
> >> is started via devicetree and OF_EARLYCON_DECLARE(). The option string
> >> is also not required if started via kernel command line parameters of
> >> the form:
> >>   earlycon=uart,mmio,<addr>
> >>   console=uart,mmio,<addr>
> >>
> >> If earlycon_device->baud is 0, then an option string was not supplied.
> >> In this case, assume the uart has already been initialized by the
> >> bootloader or firmware.
> >>
> >> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> >> ---
> >>  drivers/tty/serial/8250/8250_early.c | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
> >> index d7b831b..1701d00 100644
> >> --- a/drivers/tty/serial/8250/8250_early.c
> >> +++ b/drivers/tty/serial/8250/8250_early.c
> >> @@ -149,12 +149,18 @@ static int __init early_serial8250_setup(struct earlycon_device *device,
> >>  		return 0;
> >>  
> >>  	if (!device->baud) {
> >> +		struct uart_port *port = &device->port;
> >> +		unsigned int ier;
> >> +
> >>  		device->baud = probe_baud(&device->port);
> >>  		snprintf(device->options, sizeof(device->options), "%u",
> >>  			 device->baud);
> >> -	}
> >>  
> >> -	init_port(device);
> >> +		/* assume the device was initialized, only mask interrupts */
> >> +		ier = serial8250_early_in(port, UART_IER);
> >> +		serial8250_early_out(port, UART_IER, ier & UART_IER_UUE);
> >> +	} else
> >> +		init_port(device);
> > Should add brace in else.
> 
> I don't do that unless I have to.
> 
> > Where is original line here.
> >           early_device = device;
> 
> Whoops :)
> 
> I wrote the patch from a private branch which implements extensible console
> matching (so a console can define its own match function) and a bunch of
> other console cleanup and code removal. In that series, early_device becomes
> unnecessary and is removed.
> 
> I'll respin proper patches on top of Greg's tty-testing branch with reverts
> for the noinit options. I noticed that one of the noinit patches actually
> has the linkage for the mtk earlycon, so I'll be sure to preserve that.

I can just drop the patches in the tty-testing branch, that's what it is
there for :)

Just let me know the specific patches and I will do so, thanks.

greg k-h
Peter Hurley Feb. 2, 2015, 5:15 a.m. UTC | #4
On 02/01/2015 11:28 PM, Greg Kroah-Hartman wrote:
> On Sun, Feb 01, 2015 at 10:45:12PM -0500, Peter Hurley wrote:
>> On 02/01/2015 10:16 PM, Eddie Huang wrote:

>> I'll respin proper patches on top of Greg's tty-testing branch with reverts
>> for the noinit options. I noticed that one of the noinit patches actually
>> has the linkage for the mtk earlycon, so I'll be sure to preserve that.
> 
> I can just drop the patches in the tty-testing branch, that's what it is
> there for :)
> 
> Just let me know the specific patches and I will do so, thanks.

Well that pretty much means dropping the 3 patches that add earlycon to
8250_mtk and then applying my patch (needs fixed to apply cleanly, which
I can do) and then applying a fixed-up replacement patch to add earlycon
to 8250_mtk (which I can also supply).

Is that the way you want to go?
Greg KH Feb. 2, 2015, 5:24 a.m. UTC | #5
On Mon, Feb 02, 2015 at 12:15:31AM -0500, Peter Hurley wrote:
> On 02/01/2015 11:28 PM, Greg Kroah-Hartman wrote:
> > On Sun, Feb 01, 2015 at 10:45:12PM -0500, Peter Hurley wrote:
> >> On 02/01/2015 10:16 PM, Eddie Huang wrote:
> 
> >> I'll respin proper patches on top of Greg's tty-testing branch with reverts
> >> for the noinit options. I noticed that one of the noinit patches actually
> >> has the linkage for the mtk earlycon, so I'll be sure to preserve that.
> > 
> > I can just drop the patches in the tty-testing branch, that's what it is
> > there for :)
> > 
> > Just let me know the specific patches and I will do so, thanks.
> 
> Well that pretty much means dropping the 3 patches that add earlycon to
> 8250_mtk and then applying my patch (needs fixed to apply cleanly, which
> I can do) and then applying a fixed-up replacement patch to add earlycon
> to 8250_mtk (which I can also supply).
> 
> Is that the way you want to go?

Sounds good to me, send your patch, and I'll fix it all up tomorrow.

thanks,

greg k-h
Eddie Huang (黃智傑) Feb. 2, 2015, 5:33 a.m. UTC | #6
Hi Peter,

On Sun, 2015-02-01 at 21:24 -0800, Greg Kroah-Hartman wrote:
> On Mon, Feb 02, 2015 at 12:15:31AM -0500, Peter Hurley wrote:
> > On 02/01/2015 11:28 PM, Greg Kroah-Hartman wrote:
> > > On Sun, Feb 01, 2015 at 10:45:12PM -0500, Peter Hurley wrote:
> > >> On 02/01/2015 10:16 PM, Eddie Huang wrote:
> > 
> > >> I'll respin proper patches on top of Greg's tty-testing branch with reverts
> > >> for the noinit options. I noticed that one of the noinit patches actually
> > >> has the linkage for the mtk earlycon, so I'll be sure to preserve that.
> > > 
> > > I can just drop the patches in the tty-testing branch, that's what it is
> > > there for :)
> > > 
> > > Just let me know the specific patches and I will do so, thanks.
> > 
> > Well that pretty much means dropping the 3 patches that add earlycon to
> > 8250_mtk and then applying my patch (needs fixed to apply cleanly, which
> > I can do) and then applying a fixed-up replacement patch to add earlycon
> > to 8250_mtk (which I can also supply).
> > 
> > Is that the way you want to go?
> 
> Sounds good to me, send your patch, and I'll fix it all up tomorrow.
> 
> thanks,
> 
> greg k-h

Actually, your patch is a little different from my original idea.
Although my use case only care about divisor now, but other hardware
setting is still hard-code, not from parameter. In init_port()
function:   
	serial8250_early_out(port, UART_LCR, 0x3); /* 8n1 */
	serial8250_early_out(port, UART_IER, 0);/* no interrupt */
	serial8250_early_out(port, UART_FCR, 0); /* no fifo */
	serial8250_early_out(port, UART_MCR, 0x3); /* DTR + RTS */

This is why I propose a new option "noinit".

After checking further, in my case, I found that your patch should be
unnecessary because if skip baudrate, probe_baud() read DLL/DLM register
and init_port() write the same DLL/DLM value back, no touch any high
speed register, which means keep uart divisor setting as loader

Since I don't take "console=uart,mmio32,<addr>,noinit" into
consideration, it is good to drop my patches in the tty-testing branch.
For my case, I can send another series without noinit, just 8250_mtk.c
and its linkage modification in 8250_early.c

Eddie
Peter Hurley Feb. 2, 2015, 5:43 a.m. UTC | #7
On 02/02/2015 12:33 AM, Eddie Huang wrote:
> Hi Peter,
> 
> On Sun, 2015-02-01 at 21:24 -0800, Greg Kroah-Hartman wrote:
>> On Mon, Feb 02, 2015 at 12:15:31AM -0500, Peter Hurley wrote:
>>> On 02/01/2015 11:28 PM, Greg Kroah-Hartman wrote:
>>>> On Sun, Feb 01, 2015 at 10:45:12PM -0500, Peter Hurley wrote:
>>>>> On 02/01/2015 10:16 PM, Eddie Huang wrote:
>>>
>>>>> I'll respin proper patches on top of Greg's tty-testing branch with reverts
>>>>> for the noinit options. I noticed that one of the noinit patches actually
>>>>> has the linkage for the mtk earlycon, so I'll be sure to preserve that.
>>>>
>>>> I can just drop the patches in the tty-testing branch, that's what it is
>>>> there for :)
>>>>
>>>> Just let me know the specific patches and I will do so, thanks.
>>>
>>> Well that pretty much means dropping the 3 patches that add earlycon to
>>> 8250_mtk and then applying my patch (needs fixed to apply cleanly, which
>>> I can do) and then applying a fixed-up replacement patch to add earlycon
>>> to 8250_mtk (which I can also supply).
>>>
>>> Is that the way you want to go?
>>
>> Sounds good to me, send your patch, and I'll fix it all up tomorrow.
>>
>> thanks,
>>
>> greg k-h
> 
> Actually, your patch is a little different from my original idea.
> Although my use case only care about divisor now, but other hardware
> setting is still hard-code, not from parameter. In init_port()
> function:   
> 	serial8250_early_out(port, UART_LCR, 0x3); /* 8n1 */
> 	serial8250_early_out(port, UART_IER, 0);/* no interrupt */
> 	serial8250_early_out(port, UART_FCR, 0); /* no fifo */
> 	serial8250_early_out(port, UART_MCR, 0x3); /* DTR + RTS */
> 
> This is why I propose a new option "noinit".
> 
> After checking further, in my case, I found that your patch should be
> unnecessary because if skip baudrate, probe_baud() read DLL/DLM register
> and init_port() write the same DLL/DLM value back, no touch any high
> speed register, which means keep uart divisor setting as loader
> 
> Since I don't take "console=uart,mmio32,<addr>,noinit" into
> consideration, it is good to drop my patches in the tty-testing branch.
> For my case, I can send another series without noinit, just 8250_mtk.c
> and its linkage modification in 8250_early.c

Ok.

Greg,

The patches to drop from tty-testing are:
* 405017d Document: Modify 8250 earlycon kernel parameters
* 0dff3a4 tty: serial: 8250_mtk: Add earlycon
* b829735 tty: serial: Add 8250 earlycon to support noinit option

Regards,
Peter Hurley
Greg KH Feb. 2, 2015, 6:46 p.m. UTC | #8
On Mon, Feb 02, 2015 at 12:43:50AM -0500, Peter Hurley wrote:
> On 02/02/2015 12:33 AM, Eddie Huang wrote:
> > Hi Peter,
> > 
> > On Sun, 2015-02-01 at 21:24 -0800, Greg Kroah-Hartman wrote:
> >> On Mon, Feb 02, 2015 at 12:15:31AM -0500, Peter Hurley wrote:
> >>> On 02/01/2015 11:28 PM, Greg Kroah-Hartman wrote:
> >>>> On Sun, Feb 01, 2015 at 10:45:12PM -0500, Peter Hurley wrote:
> >>>>> On 02/01/2015 10:16 PM, Eddie Huang wrote:
> >>>
> >>>>> I'll respin proper patches on top of Greg's tty-testing branch with reverts
> >>>>> for the noinit options. I noticed that one of the noinit patches actually
> >>>>> has the linkage for the mtk earlycon, so I'll be sure to preserve that.
> >>>>
> >>>> I can just drop the patches in the tty-testing branch, that's what it is
> >>>> there for :)
> >>>>
> >>>> Just let me know the specific patches and I will do so, thanks.
> >>>
> >>> Well that pretty much means dropping the 3 patches that add earlycon to
> >>> 8250_mtk and then applying my patch (needs fixed to apply cleanly, which
> >>> I can do) and then applying a fixed-up replacement patch to add earlycon
> >>> to 8250_mtk (which I can also supply).
> >>>
> >>> Is that the way you want to go?
> >>
> >> Sounds good to me, send your patch, and I'll fix it all up tomorrow.
> >>
> >> thanks,
> >>
> >> greg k-h
> > 
> > Actually, your patch is a little different from my original idea.
> > Although my use case only care about divisor now, but other hardware
> > setting is still hard-code, not from parameter. In init_port()
> > function:   
> > 	serial8250_early_out(port, UART_LCR, 0x3); /* 8n1 */
> > 	serial8250_early_out(port, UART_IER, 0);/* no interrupt */
> > 	serial8250_early_out(port, UART_FCR, 0); /* no fifo */
> > 	serial8250_early_out(port, UART_MCR, 0x3); /* DTR + RTS */
> > 
> > This is why I propose a new option "noinit".
> > 
> > After checking further, in my case, I found that your patch should be
> > unnecessary because if skip baudrate, probe_baud() read DLL/DLM register
> > and init_port() write the same DLL/DLM value back, no touch any high
> > speed register, which means keep uart divisor setting as loader
> > 
> > Since I don't take "console=uart,mmio32,<addr>,noinit" into
> > consideration, it is good to drop my patches in the tty-testing branch.
> > For my case, I can send another series without noinit, just 8250_mtk.c
> > and its linkage modification in 8250_early.c
> 
> Ok.
> 
> Greg,
> 
> The patches to drop from tty-testing are:
> * 405017d Document: Modify 8250 earlycon kernel parameters
> * 0dff3a4 tty: serial: 8250_mtk: Add earlycon
> * b829735 tty: serial: Add 8250 earlycon to support noinit option

Ok, now dropped, thanks.

greg k-h
diff mbox

Patch

diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index d7b831b..1701d00 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -149,12 +149,18 @@  static int __init early_serial8250_setup(struct earlycon_device *device,
 		return 0;
 
 	if (!device->baud) {
+		struct uart_port *port = &device->port;
+		unsigned int ier;
+
 		device->baud = probe_baud(&device->port);
 		snprintf(device->options, sizeof(device->options), "%u",
 			 device->baud);
-	}
 
-	init_port(device);
+		/* assume the device was initialized, only mask interrupts */
+		ier = serial8250_early_in(port, UART_IER);
+		serial8250_early_out(port, UART_IER, ier & UART_IER_UUE);
+	} else
+		init_port(device);
 
 	device->con->write = early_serial8250_write;
 	return 0;