| Message ID | 1422334209-23125-3-git-send-email-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/26/2015 11:50 PM, Rob Herring wrote: > The PXA variant of the 8250 UART adds a UART enable bit which must not > be cleared. Make the earlycon support maintain this bit if it is set. > This implies some initialization of the UART, but we cannot > unconditionally set the bit as some other variants require this bit to > be clear for other functions. So if the bootloader doesn't enable the uart, then earlycon doesn't work? Or is there some other piece that is enabling the uart after earlycon setup (besides the driver, of course)? > Signed-off-by: Rob Herring <robh@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Jiri Slaby <jslaby@suse.cz> > Cc: linux-serial@vger.kernel.org > --- > drivers/tty/serial/8250/8250_early.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c > index ce2a8ab..c31a22b 100644 > --- a/drivers/tty/serial/8250/8250_early.c > +++ b/drivers/tty/serial/8250/8250_early.c > @@ -93,10 +93,10 @@ static void __init early_serial8250_write(struct console *console, > struct uart_port *port = &early_device->port; > unsigned int ier; > > - /* Save the IER and disable interrupts */ > + /* Save the IER and disable interrupts preserving the UUE bit */ > ier = serial8250_early_in(port, UART_IER); > if (ier) > - serial8250_early_out(port, UART_IER, 0); > + serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); > > uart_console_write(port, s, count, serial_putc); > > @@ -127,9 +127,11 @@ static void __init init_port(struct earlycon_device *device) > struct uart_port *port = &device->port; > unsigned int divisor; > unsigned char c; > + unsigned int ier; > > serial8250_early_out(port, UART_LCR, 0x3); /* 8n1 */ > - serial8250_early_out(port, UART_IER, 0); /* no interrupt */ > + ier = serial8250_early_in(port, UART_IER); > + serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); /* no interrupt */ The trailing comment seems no longer relevant; how about below? /* Mask interrupts (preserve XScale's uart enable bit) */ serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); OTOH, if the uart is disabled, shouldn't earlycon be disabled? Regards, Peter Hurley > serial8250_early_out(port, UART_FCR, 0); /* no fifo */ > serial8250_early_out(port, UART_MCR, 0x3); /* DTR + RTS */ > >
On Tue, Jan 27, 2015 at 7:10 AM, Peter Hurley <peter@hurleysoftware.com> wrote: > On 01/26/2015 11:50 PM, Rob Herring wrote: >> The PXA variant of the 8250 UART adds a UART enable bit which must not >> be cleared. Make the earlycon support maintain this bit if it is set. >> This implies some initialization of the UART, but we cannot >> unconditionally set the bit as some other variants require this bit to >> be clear for other functions. > > So if the bootloader doesn't enable the uart, then earlycon doesn't work? Correct. Some earlycon drivers rely on this fact and don't do any initialization. There is a patch posted also to skip init for the 8250 earlycon. We could rely on that instead, but the 1st chunk is still needed. The UART already being initialized should be pretty common for PXA based systems. More so than x86 UARTs. I think it is better to work under some conditions than none. > Or is there some other piece that is enabling the uart after earlycon setup > (besides the driver, of course)? > >> Signed-off-by: Rob Herring <robh@kernel.org> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Jiri Slaby <jslaby@suse.cz> >> Cc: linux-serial@vger.kernel.org >> --- >> drivers/tty/serial/8250/8250_early.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c >> index ce2a8ab..c31a22b 100644 >> --- a/drivers/tty/serial/8250/8250_early.c >> +++ b/drivers/tty/serial/8250/8250_early.c >> @@ -93,10 +93,10 @@ static void __init early_serial8250_write(struct console *console, >> struct uart_port *port = &early_device->port; >> unsigned int ier; >> >> - /* Save the IER and disable interrupts */ >> + /* Save the IER and disable interrupts preserving the UUE bit */ >> ier = serial8250_early_in(port, UART_IER); >> if (ier) >> - serial8250_early_out(port, UART_IER, 0); >> + serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); >> >> uart_console_write(port, s, count, serial_putc); >> >> @@ -127,9 +127,11 @@ static void __init init_port(struct earlycon_device *device) >> struct uart_port *port = &device->port; >> unsigned int divisor; >> unsigned char c; >> + unsigned int ier; >> >> serial8250_early_out(port, UART_LCR, 0x3); /* 8n1 */ >> - serial8250_early_out(port, UART_IER, 0); /* no interrupt */ >> + ier = serial8250_early_in(port, UART_IER); >> + serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); /* no interrupt */ > > The trailing comment seems no longer relevant; how about below? It is because we are still disabling interrupts. > /* Mask interrupts (preserve XScale's uart enable bit) */ > serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); > > OTOH, if the uart is disabled, shouldn't earlycon be disabled? There's no way I can detect that. Rob
On 01/27/2015 09:05 AM, Rob Herring wrote: > On Tue, Jan 27, 2015 at 7:10 AM, Peter Hurley <peter@hurleysoftware.com> wrote: >> On 01/26/2015 11:50 PM, Rob Herring wrote: >>> The PXA variant of the 8250 UART adds a UART enable bit which must not >>> be cleared. Make the earlycon support maintain this bit if it is set. >>> This implies some initialization of the UART, but we cannot >>> unconditionally set the bit as some other variants require this bit to >>> be clear for other functions. >> >> So if the bootloader doesn't enable the uart, then earlycon doesn't work? > > Correct. Some earlycon drivers rely on this fact and don't do any > initialization. There is a patch posted also to skip init for the 8250 > earlycon. We could rely on that instead, but the 1st chunk is still > needed. > > The UART already being initialized should be pretty common for PXA > based systems. More so than x86 UARTs. I think it is better to work > under some conditions than none. Ok. >> Or is there some other piece that is enabling the uart after earlycon setup >> (besides the driver, of course)? >> >>> Signed-off-by: Rob Herring <robh@kernel.org> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Cc: Jiri Slaby <jslaby@suse.cz> >>> Cc: linux-serial@vger.kernel.org >>> --- >>> drivers/tty/serial/8250/8250_early.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c >>> index ce2a8ab..c31a22b 100644 >>> --- a/drivers/tty/serial/8250/8250_early.c >>> +++ b/drivers/tty/serial/8250/8250_early.c >>> @@ -93,10 +93,10 @@ static void __init early_serial8250_write(struct console *console, >>> struct uart_port *port = &early_device->port; >>> unsigned int ier; >>> >>> - /* Save the IER and disable interrupts */ >>> + /* Save the IER and disable interrupts preserving the UUE bit */ >>> ier = serial8250_early_in(port, UART_IER); >>> if (ier) >>> - serial8250_early_out(port, UART_IER, 0); >>> + serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); >>> >>> uart_console_write(port, s, count, serial_putc); >>> >>> @@ -127,9 +127,11 @@ static void __init init_port(struct earlycon_device *device) >>> struct uart_port *port = &device->port; >>> unsigned int divisor; >>> unsigned char c; >>> + unsigned int ier; >>> >>> serial8250_early_out(port, UART_LCR, 0x3); /* 8n1 */ >>> - serial8250_early_out(port, UART_IER, 0); /* no interrupt */ >>> + ier = serial8250_early_in(port, UART_IER); >>> + serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); /* no interrupt */ >> >> The trailing comment seems no longer relevant; how about below? > > It is because we are still disabling interrupts. I realize that, which is why my suggested replacement comment includes that information. It's not obvious what effect UART_IER_UUE has from just looking at the code. >> /* Mask interrupts (preserve XScale's uart enable bit) */ >> serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); >> >> OTOH, if the uart is disabled, shouldn't earlycon be disabled? > > There's no way I can detect that. Right, sorry. [ I was about to write, "If UART_IER_UEE is not set, ..." which of course is true for every other non-PXA 8250-base uart :) ] Regards, Peter Hurley
diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c index ce2a8ab..c31a22b 100644 --- a/drivers/tty/serial/8250/8250_early.c +++ b/drivers/tty/serial/8250/8250_early.c @@ -93,10 +93,10 @@ static void __init early_serial8250_write(struct console *console, struct uart_port *port = &early_device->port; unsigned int ier; - /* Save the IER and disable interrupts */ + /* Save the IER and disable interrupts preserving the UUE bit */ ier = serial8250_early_in(port, UART_IER); if (ier) - serial8250_early_out(port, UART_IER, 0); + serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); uart_console_write(port, s, count, serial_putc); @@ -127,9 +127,11 @@ static void __init init_port(struct earlycon_device *device) struct uart_port *port = &device->port; unsigned int divisor; unsigned char c; + unsigned int ier; serial8250_early_out(port, UART_LCR, 0x3); /* 8n1 */ - serial8250_early_out(port, UART_IER, 0); /* no interrupt */ + ier = serial8250_early_in(port, UART_IER); + serial8250_early_out(port, UART_IER, ier & UART_IER_UUE); /* no interrupt */ serial8250_early_out(port, UART_FCR, 0); /* no fifo */ serial8250_early_out(port, UART_MCR, 0x3); /* DTR + RTS */
The PXA variant of the 8250 UART adds a UART enable bit which must not be cleared. Make the earlycon support maintain this bit if it is set. This implies some initialization of the UART, but we cannot unconditionally set the bit as some other variants require this bit to be clear for other functions. Signed-off-by: Rob Herring <robh@kernel.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jiri Slaby <jslaby@suse.cz> Cc: linux-serial@vger.kernel.org --- drivers/tty/serial/8250/8250_early.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)