[PATCH 1/3] tty/serial: of_serial: add DT alias ID handling

[PATCH 1/3] tty/serial: of_serial: add DT alias ID handling

[Rob Herring]

Add support for alias parsing from the DT. This allows for consistent
tty numbering.

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/of_serial.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
index 64f1bab..3194b42 100644
--- a/drivers/tty/serial/of_serial.c
+++ b/drivers/tty/serial/of_serial.c
@@ -128,6 +128,10 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 	if (of_find_property(np, "no-loopback-test", NULL))
 		port->flags |= UPF_SKIP_TEST;
 
+	ret = of_alias_get_id(np, "serial");
+	if (ret >= 0)
+		port->line = ret;
+
 	port->dev = &ofdev->dev;
 
 	switch (type) {
-- 
2.1.0

[PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Rob Herring]

Add mrvl,pxa-uart and mrvl,mmp-uart compatible strings for the of_serial
driver. These are 8250 variants which have a port type of PORT_XSCALE.

There is also the serial driver pxa.c with these compatible strings
already. However, it can be replaced with the common 8250 driver. It has
some issues like it cannot coexist with the 8250 driver due to tty name
collision. That also means adding these compatible strings here should
not case a problem.

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/of_serial.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
index 3194b42..fbb719c 100644
--- a/drivers/tty/serial/of_serial.c
+++ b/drivers/tty/serial/of_serial.c
@@ -335,6 +335,10 @@ static struct of_device_id of_platform_serial_table[] = {
 		.data = (void *)PORT_ALTR_16550_F64, },
 	{ .compatible = "altr,16550-FIFO128",
 		.data = (void *)PORT_ALTR_16550_F128, },
+	{ .compatible = "mrvl,mmp-uart",
+		.data = (void *)PORT_XSCALE, },
+	{ .compatible = "mrvl,pxa-uart",
+		.data = (void *)PORT_XSCALE, },
 #ifdef CONFIG_SERIAL_OF_PLATFORM_NWPSERIAL
 	{ .compatible = "ibm,qpace-nwp-serial",
 		.data = (void *)PORT_NWPSERIAL, },
-- 
2.1.0

[PATCH 3/3] tty/serial: 8250_early: Add support for PXA UARTs

[Rob Herring]

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(-)

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 */
 
-- 
2.1.0

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Peter Hurley]

Hi Rob,

On 01/26/2015 11:50 PM, Rob Herring wrote:
> Add mrvl,pxa-uart and mrvl,mmp-uart compatible strings for the of_serial
> driver. These are 8250 variants which have a port type of PORT_XSCALE.
> 
> There is also the serial driver pxa.c with these compatible strings
> already. However, it can be replaced with the common 8250 driver. It has
> some issues like it cannot coexist with the 8250 driver due to tty name
> collision. That also means adding these compatible strings here should
> not case a problem.

So what determines which driver is controlling the port if both
drivers are built-in?

Regards,
Peter Hurley

> 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/of_serial.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
> index 3194b42..fbb719c 100644
> --- a/drivers/tty/serial/of_serial.c
> +++ b/drivers/tty/serial/of_serial.c
> @@ -335,6 +335,10 @@ static struct of_device_id of_platform_serial_table[] = {
>  		.data = (void *)PORT_ALTR_16550_F64, },
>  	{ .compatible = "altr,16550-FIFO128",
>  		.data = (void *)PORT_ALTR_16550_F128, },
> +	{ .compatible = "mrvl,mmp-uart",
> +		.data = (void *)PORT_XSCALE, },
> +	{ .compatible = "mrvl,pxa-uart",
> +		.data = (void *)PORT_XSCALE, },
>  #ifdef CONFIG_SERIAL_OF_PLATFORM_NWPSERIAL
>  	{ .compatible = "ibm,qpace-nwp-serial",
>  		.data = (void *)PORT_NWPSERIAL, },
> 

Re: [PATCH 3/3] tty/serial: 8250_early: Add support for PXA UARTs

[Peter Hurley]

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 */
>  
> 

Re: [PATCH 3/3] tty/serial: 8250_early: Add support for PXA UARTs

[Rob Herring]

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

Re: [PATCH 3/3] tty/serial: 8250_early: Add support for PXA UARTs

[Peter Hurley]

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

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Rob Herring]

On Tue, Jan 27, 2015 at 6:44 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> Hi Rob,
>
> On 01/26/2015 11:50 PM, Rob Herring wrote:
>> Add mrvl,pxa-uart and mrvl,mmp-uart compatible strings for the of_serial
>> driver. These are 8250 variants which have a port type of PORT_XSCALE.
>>
>> There is also the serial driver pxa.c with these compatible strings
>> already. However, it can be replaced with the common 8250 driver. It has
>> some issues like it cannot coexist with the 8250 driver due to tty name
>> collision. That also means adding these compatible strings here should
>> not case a problem.
>
> So what determines which driver is controlling the port if both
> drivers are built-in?

If both are built-in, whoever registers ttyS ports first wins. This
will be the 8250 driver due to link order. So I guess PXA systems have
avoided this by never building in 8250 driver.

Rob

>
> Regards,
> Peter Hurley
>
>> 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/of_serial.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
>> index 3194b42..fbb719c 100644
>> --- a/drivers/tty/serial/of_serial.c
>> +++ b/drivers/tty/serial/of_serial.c
>> @@ -335,6 +335,10 @@ static struct of_device_id of_platform_serial_table[] = {
>>               .data = (void *)PORT_ALTR_16550_F64, },
>>       { .compatible = "altr,16550-FIFO128",
>>               .data = (void *)PORT_ALTR_16550_F128, },
>> +     { .compatible = "mrvl,mmp-uart",
>> +             .data = (void *)PORT_XSCALE, },
>> +     { .compatible = "mrvl,pxa-uart",
>> +             .data = (void *)PORT_XSCALE, },
>>  #ifdef CONFIG_SERIAL_OF_PLATFORM_NWPSERIAL
>>       { .compatible = "ibm,qpace-nwp-serial",
>>               .data = (void *)PORT_NWPSERIAL, },
>>
>

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Peter Hurley]

On 01/27/2015 09:30 AM, Rob Herring wrote:
> On Tue, Jan 27, 2015 at 6:44 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> Hi Rob,
>>
>> On 01/26/2015 11:50 PM, Rob Herring wrote:
>>> Add mrvl,pxa-uart and mrvl,mmp-uart compatible strings for the of_serial
>>> driver. These are 8250 variants which have a port type of PORT_XSCALE.
>>>
>>> There is also the serial driver pxa.c with these compatible strings
>>> already. However, it can be replaced with the common 8250 driver. It has
>>> some issues like it cannot coexist with the 8250 driver due to tty name
>>> collision. That also means adding these compatible strings here should
>>> not case a problem.
>>
>> So what determines which driver is controlling the port if both
>> drivers are built-in?
> 
> If both are built-in, whoever registers ttyS ports first wins. This
> will be the 8250 driver due to link order.

Ok, but then I think the commit log should reflect that this patch
effectively replaces pxa2xx-uart driver with the 8250 driver for PXA/MMP uarts
if both drivers are built-in or both drivers are modules.

> So I guess PXA systems have avoided this by never building in 8250 driver.

Platform devices are initialized first, so before this patch the pxa2xx-uart
driver would have claimed the platform ports before the 8250 driver, if
both were built-in (or both modules).

Maybe Kconfig should warn if they're both built-in or both modules?

Regards,
Peter Hurley

>>> 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/of_serial.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/tty/serial/of_serial.c b/drivers/tty/serial/of_serial.c
>>> index 3194b42..fbb719c 100644
>>> --- a/drivers/tty/serial/of_serial.c
>>> +++ b/drivers/tty/serial/of_serial.c
>>> @@ -335,6 +335,10 @@ static struct of_device_id of_platform_serial_table[] = {
>>>               .data = (void *)PORT_ALTR_16550_F64, },
>>>       { .compatible = "altr,16550-FIFO128",
>>>               .data = (void *)PORT_ALTR_16550_F128, },
>>> +     { .compatible = "mrvl,mmp-uart",
>>> +             .data = (void *)PORT_XSCALE, },
>>> +     { .compatible = "mrvl,pxa-uart",
>>> +             .data = (void *)PORT_XSCALE, },
>>>  #ifdef CONFIG_SERIAL_OF_PLATFORM_NWPSERIAL
>>>       { .compatible = "ibm,qpace-nwp-serial",
>>>               .data = (void *)PORT_NWPSERIAL, },
>>>
>>

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Rob Herring]

On Tue, Jan 27, 2015 at 9:09 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 01/27/2015 09:30 AM, Rob Herring wrote:
>> On Tue, Jan 27, 2015 at 6:44 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> Hi Rob,
>>>
>>> On 01/26/2015 11:50 PM, Rob Herring wrote:
>>>> Add mrvl,pxa-uart and mrvl,mmp-uart compatible strings for the of_serial
>>>> driver. These are 8250 variants which have a port type of PORT_XSCALE.
>>>>
>>>> There is also the serial driver pxa.c with these compatible strings
>>>> already. However, it can be replaced with the common 8250 driver. It has
>>>> some issues like it cannot coexist with the 8250 driver due to tty name
>>>> collision. That also means adding these compatible strings here should
>>>> not case a problem.
>>>
>>> So what determines which driver is controlling the port if both
>>> drivers are built-in?
>>
>> If both are built-in, whoever registers ttyS ports first wins. This
>> will be the 8250 driver due to link order.
>
> Ok, but then I think the commit log should reflect that this patch
> effectively replaces pxa2xx-uart driver with the 8250 driver for PXA/MMP uarts
> if both drivers are built-in or both drivers are modules.

It doesn't always. It is only on DT enabled platforms. But I can be
more explicit about that.

There is another patch to remove the PXA driver entirely which I'm
inquiring about why it was never merged [1]. This is why I went down
the path of getting the 8250 driver to work rather than trying to fix
the PXA driver.

>> So I guess PXA systems have avoided this by never building in 8250 driver.
>
> Platform devices are initialized first, so before this patch the pxa2xx-uart
> driver would have claimed the platform ports before the 8250 driver, if
> both were built-in (or both modules).

That's not exactly what I observed. The console is killed when the
real driver initializes. I have no other debug output capability after
that, so I'm not completely sure what was happening. Part of the
problem is you need to move uart_register_driver to probe from the
initcall as this is what other serial drivers sharing ttyS have done.
Using 8250 earlycon with the PXA driver seems to cause problems, so
I'd have to duplicate the earlycon support in the PXA driver.

> Maybe Kconfig should warn if they're both built-in or both modules?

Is there a way to do that?

Rob

[1] https://lkml.org/lkml/2015/1/19/471

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Peter Hurley]

On 01/27/2015 11:44 AM, Rob Herring wrote:
> On Tue, Jan 27, 2015 at 9:09 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 01/27/2015 09:30 AM, Rob Herring wrote:
>>> On Tue, Jan 27, 2015 at 6:44 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>> Hi Rob,
>>>>
>>>> On 01/26/2015 11:50 PM, Rob Herring wrote:
>>>>> Add mrvl,pxa-uart and mrvl,mmp-uart compatible strings for the of_serial
>>>>> driver. These are 8250 variants which have a port type of PORT_XSCALE.
>>>>>
>>>>> There is also the serial driver pxa.c with these compatible strings
>>>>> already. However, it can be replaced with the common 8250 driver. It has
>>>>> some issues like it cannot coexist with the 8250 driver due to tty name
>>>>> collision. That also means adding these compatible strings here should
>>>>> not case a problem.
>>>>
>>>> So what determines which driver is controlling the port if both
>>>> drivers are built-in?
>>>
>>> If both are built-in, whoever registers ttyS ports first wins. This
>>> will be the 8250 driver due to link order.
>>
>> Ok, but then I think the commit log should reflect that this patch
>> effectively replaces pxa2xx-uart driver with the 8250 driver for PXA/MMP uarts
>> if both drivers are built-in or both drivers are modules.
> 
> It doesn't always. It is only on DT enabled platforms. But I can be
> more explicit about that.
> 
> There is another patch to remove the PXA driver entirely which I'm
> inquiring about why it was never merged [1]. This is why I went down
> the path of getting the 8250 driver to work rather than trying to fix
> the PXA driver.

Ah. I had seen your inquiry but didn't grasp the significance.
Greg acked that patch over 8 mos ago; it's still stuck somewhere?

OTOH, I didn't realize that patch removed the PXA driver. That driver has
errata workarounds which didn't get integrated into the 8250 core; it seems
a shame to toss that away.

[ Sidebar: I did a trial split of 8250_core into a separate 8250 base module
  and an 8250 universal driver. I keep having to rework the split but it's
  pretty close now; the 8250 base module might be just the thing for simplifying
  the pxa driver and getting console/earlycon/dma for free.

  Let me see what I can do about getting that done.
]

>>> So I guess PXA systems have avoided this by never building in 8250 driver.
>>
>> Platform devices are initialized first, so before this patch the pxa2xx-uart
>> driver would have claimed the platform ports before the 8250 driver, if
>> both were built-in (or both modules).
> 
> That's not exactly what I observed. The console is killed when the
> real driver initializes. I have no other debug output capability after
> that, so I'm not completely sure what was happening. Part of the
> problem is you need to move uart_register_driver to probe from the
> initcall as this is what other serial drivers sharing ttyS have done.

Ah, yep. I missed that subtlety.

> Using 8250 earlycon with the PXA driver seems to cause problems, so
> I'd have to duplicate the earlycon support in the PXA driver.
> 
>> Maybe Kconfig should warn if they're both built-in or both modules?
> 
> Is there a way to do that?

I think so; conf warns if one setting overrides another, so I think there's
probably a way to make that happen in the Kconfig. I'll mess with that later
tonight or tomorrow, and get back to you.

Regards,
Peter Hurley

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Rob Herring]

On Tue, Jan 27, 2015 at 1:43 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 01/27/2015 11:44 AM, Rob Herring wrote:
>> On Tue, Jan 27, 2015 at 9:09 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> On 01/27/2015 09:30 AM, Rob Herring wrote:
>>>> On Tue, Jan 27, 2015 at 6:44 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>>> Hi Rob,
>>>>>
>>>>> On 01/26/2015 11:50 PM, Rob Herring wrote:
>>>>>> Add mrvl,pxa-uart and mrvl,mmp-uart compatible strings for the of_serial
>>>>>> driver. These are 8250 variants which have a port type of PORT_XSCALE.
>>>>>>
>>>>>> There is also the serial driver pxa.c with these compatible strings
>>>>>> already. However, it can be replaced with the common 8250 driver. It has
>>>>>> some issues like it cannot coexist with the 8250 driver due to tty name
>>>>>> collision. That also means adding these compatible strings here should
>>>>>> not case a problem.
>>>>>
>>>>> So what determines which driver is controlling the port if both
>>>>> drivers are built-in?
>>>>
>>>> If both are built-in, whoever registers ttyS ports first wins. This
>>>> will be the 8250 driver due to link order.
>>>
>>> Ok, but then I think the commit log should reflect that this patch
>>> effectively replaces pxa2xx-uart driver with the 8250 driver for PXA/MMP uarts
>>> if both drivers are built-in or both drivers are modules.
>>
>> It doesn't always. It is only on DT enabled platforms. But I can be
>> more explicit about that.
>>
>> There is another patch to remove the PXA driver entirely which I'm
>> inquiring about why it was never merged [1]. This is why I went down
>> the path of getting the 8250 driver to work rather than trying to fix
>> the PXA driver.
>
> Ah. I had seen your inquiry but didn't grasp the significance.
> Greg acked that patch over 8 mos ago; it's still stuck somewhere?

It appears to me that this patch got added as a dependency to the rest
of the series which never got merged. Guess they lost interest.

> OTOH, I didn't realize that patch removed the PXA driver. That driver has
> errata workarounds which didn't get integrated into the 8250 core; it seems
> a shame to toss that away.

There was some discussion on errata in the prior versions. One of them
was deemed not needed. Another one is needed, but that's on certain
PXA2xx. So we'll need to pickup that work-around at some point, but it
is not yet needed here because PXA2xx is not enabled for DT.

> [ Sidebar: I did a trial split of 8250_core into a separate 8250 base module
>   and an 8250 universal driver. I keep having to rework the split but it's
>   pretty close now; the 8250 base module might be just the thing for simplifying
>   the pxa driver and getting console/earlycon/dma for free.
>
>   Let me see what I can do about getting that done.
> ]

Yes, it would be nice to split the legacy ISA parts from the proper
platform driver. There's way too many kconfig options for the 8250 as
well IMO.

BTW, It appears that the main motivator for a separate driver was to
add DMA support and that's what the out of tree driver adds. But now,
we would never add the PXA DMA API support and the 8250 driver has
learned DMA support.

Rob

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Peter Hurley]

On 01/28/2015 09:21 AM, Rob Herring wrote:
> On Tue, Jan 27, 2015 at 1:43 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 01/27/2015 11:44 AM, Rob Herring wrote:
>>> On Tue, Jan 27, 2015 at 9:09 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>> On 01/27/2015 09:30 AM, Rob Herring wrote:
>>>>> On Tue, Jan 27, 2015 at 6:44 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>>>> On 01/26/2015 11:50 PM, Rob Herring wrote:

[...]

>> OTOH, I didn't realize that patch removed the PXA driver. That driver has
>> errata workarounds which didn't get integrated into the 8250 core; it seems
>> a shame to toss that away.
> 
> There was some discussion on errata in the prior versions. One of them
> was deemed not needed. Another one is needed, but that's on certain
> PXA2xx. So we'll need to pickup that work-around at some point, but it
> is not yet needed here because PXA2xx is not enabled for DT.

Do you know offhand which errata? If not, nbd; I can do a little digging.


>> [ Sidebar: I did a trial split of 8250_core into a separate 8250 base module
>>   and an 8250 universal driver. I keep having to rework the split but it's
>>   pretty close now; the 8250 base module might be just the thing for simplifying
>>   the pxa driver and getting console/earlycon/dma for free.
>>
>>   Let me see what I can do about getting that done.
>> ]
> 
> Yes, it would be nice to split the legacy ISA parts from the proper
> platform driver. There's way too many kconfig options for the 8250 as
> well IMO.

After I finish the split, I can look at making most/all of those options
dependent on the universal 8250 driver only.

> BTW, It appears that the main motivator for a separate driver was to
> add DMA support and that's what the out of tree driver adds. But now,
> we would never add the PXA DMA API support and the 8250 driver has
> learned DMA support.

Yeah, PXA should not have been forked. But I say that with the benefit
of hindsight.

Regards,
Peter Hurley

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Peter Hurley]

On 01/27/2015 11:44 AM, Rob Herring wrote:
> On Tue, Jan 27, 2015 at 9:09 AM, Peter Hurley <peter@hurleysoftware.com> wrote:

[...]

>> Maybe Kconfig should warn if they're both built-in or both modules?
> 
> Is there a way to do that?

Well, it's not what I had in mind originally, but the diff below
generates a broken dependencies warning without causing build problems:
	$ scripts/kconfig/mconf Kconfig
	warning: (SERIAL_PXA) selects TTYS_DRIVER_PXA which has unmet direct dependencies (TTY && HAS_IOMEM && !TTYS_DRIVER)

(My original idea was thwarted by the requirement that choice/endchoice
requires prompts).

That said, for PXA, I think we agree splitting out a standalone 8250 platform
driver is the solution.

Regards,
Peter Hurley

--
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 0fcbcd2..c367542 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -6,6 +6,7 @@
 config SERIAL_8250
 	tristate "8250/16550 and compatible serial support"
 	select SERIAL_CORE
+	select TTYS_DRIVER
 	---help---
 	  This selects whether you want to include the driver for the standard
 	  serial ports.  The standard answer is Y.  People who might say N
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index c79b43c..10c3e27 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -7,6 +7,13 @@ if TTY
 menu "Serial drivers"
 	depends on HAS_IOMEM
 
+config TTYS_DRIVER
+       tristate
+
+config TTYS_DRIVER_PXA
+       tristate
+       depends on !TTYS_DRIVER
+
 config SERIAL_EARLYCON
 	bool
 	help
@@ -438,6 +445,7 @@ config SERIAL_PXA
 	bool "PXA serial port support"
 	depends on ARCH_PXA || ARCH_MMP
 	select SERIAL_CORE
+	select TTYS_DRIVER_PXA
 	help
 	  If you have a machine based on an Intel XScale PXA2xx CPU you
 	  can enable its onboard serial ports by enabling this option.

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Rob Herring]

On Wed, Jan 28, 2015 at 11:37 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 01/27/2015 11:44 AM, Rob Herring wrote:
>> On Tue, Jan 27, 2015 at 9:09 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>
> [...]
>
>>> Maybe Kconfig should warn if they're both built-in or both modules?
>>
>> Is there a way to do that?
>
> Well, it's not what I had in mind originally, but the diff below
> generates a broken dependencies warning without causing build problems:
>         $ scripts/kconfig/mconf Kconfig
>         warning: (SERIAL_PXA) selects TTYS_DRIVER_PXA which has unmet direct dependencies (TTY && HAS_IOMEM && !TTYS_DRIVER)
>
> (My original idea was thwarted by the requirement that choice/endchoice
> requires prompts).

Okay, but this should be a separate patch. The problem exists with or
without my patch.

> That said, for PXA, I think we agree splitting out a standalone 8250 platform
> driver is the solution.

So after all the discussion, you are okay with the original patch?

Rob

>
> Regards,
> Peter Hurley
>
> --
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 0fcbcd2..c367542 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -6,6 +6,7 @@
>  config SERIAL_8250
>         tristate "8250/16550 and compatible serial support"
>         select SERIAL_CORE
> +       select TTYS_DRIVER
>         ---help---
>           This selects whether you want to include the driver for the standard
>           serial ports.  The standard answer is Y.  People who might say N
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index c79b43c..10c3e27 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -7,6 +7,13 @@ if TTY
>  menu "Serial drivers"
>         depends on HAS_IOMEM
>
> +config TTYS_DRIVER
> +       tristate
> +
> +config TTYS_DRIVER_PXA
> +       tristate
> +       depends on !TTYS_DRIVER
> +
>  config SERIAL_EARLYCON
>         bool
>         help
> @@ -438,6 +445,7 @@ config SERIAL_PXA
>         bool "PXA serial port support"
>         depends on ARCH_PXA || ARCH_MMP
>         select SERIAL_CORE
> +       select TTYS_DRIVER_PXA
>         help
>           If you have a machine based on an Intel XScale PXA2xx CPU you
>           can enable its onboard serial ports by enabling this option.
>
>

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Peter Hurley]

On 01/30/2015 02:51 PM, Rob Herring wrote:
> On Wed, Jan 28, 2015 at 11:37 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 01/27/2015 11:44 AM, Rob Herring wrote:
>>> On Tue, Jan 27, 2015 at 9:09 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>
>> [...]
>>
>>>> Maybe Kconfig should warn if they're both built-in or both modules?
>>>
>>> Is there a way to do that?
>>
>> Well, it's not what I had in mind originally, but the diff below
>> generates a broken dependencies warning without causing build problems:
>>         $ scripts/kconfig/mconf Kconfig
>>         warning: (SERIAL_PXA) selects TTYS_DRIVER_PXA which has unmet direct dependencies (TTY && HAS_IOMEM && !TTYS_DRIVER)
>>
>> (My original idea was thwarted by the requirement that choice/endchoice
>> requires prompts).
> 
> Okay, but this should be a separate patch. The problem exists with or
> without my patch.

Yeah, don't worry about this. The solution of how to deal with multiple
ttyS drivers needs to address this problem.

>> That said, for PXA, I think we agree splitting out a standalone 8250 platform
>> driver is the solution.
> 
> So after all the discussion, you are okay with the original patch?

With a more specific commit log, yes. At a minimum, noting under what
circumstances the 8250 driver replaces the pxa2xx-uart driver. (And maybe
noting that this doesn't break hardware that needs those workarounds in
pxa.c).

Regards,
Peter Hurley

Re: [PATCH 2/3] tty/serial: of_serial: add support for PXA/MMP uarts

[Peter Hurley]

On 01/30/2015 03:24 PM, Peter Hurley wrote:
> On 01/30/2015 02:51 PM, Rob Herring wrote:
>> On Wed, Jan 28, 2015 at 11:37 AM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> On 01/27/2015 11:44 AM, Rob Herring wrote:
>> So after all the discussion, you are okay with the original patch?
> 
> With a more specific commit log, yes. At a minimum, noting under what
> circumstances the 8250 driver replaces the pxa2xx-uart driver. (And maybe
> noting that this doesn't break hardware that needs those workarounds in
> pxa.c).

nvm; upstreamed.