| Message ID | 54CE6FD9.5030606@hurleysoftware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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?
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
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
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
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 --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;