[PATCH] Revert "tty: xilinx_uartps: Fix missing id assignment to the console"

[PATCH] Revert "tty: xilinx_uartps: Fix missing id assignment to the console"

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This reverts commit 2ae11c46d5fdc46cb396e35911c713d271056d35.

It turned out to break the ultra96-rev1, e.g., which uses uart1 as
serial0 (and stdout-path = "serial0:115200n8").

Cc: stable <stable@vger.kernel.org>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/tty/serial/xilinx_uartps.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 35e9e8faf8de..ac137b6a1dc1 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1459,7 +1459,6 @@ static int cdns_uart_probe(struct platform_device *pdev)
 		cdns_uart_uart_driver.nr = CDNS_UART_NR_PORTS;
 #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
 		cdns_uart_uart_driver.cons = &cdns_uart_console;
-		cdns_uart_console.index = id;
 #endif
 
 		rc = uart_register_driver(&cdns_uart_uart_driver);
-- 
2.26.2

Re: [PATCH] Revert "tty: xilinx_uartps: Fix missing id assignment to the console"

[Michal Simek]

On 18. 06. 20 10:11, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This reverts commit 2ae11c46d5fdc46cb396e35911c713d271056d35.
> 
> It turned out to break the ultra96-rev1, e.g., which uses uart1 as
> serial0 (and stdout-path = "serial0:115200n8").
> 
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 35e9e8faf8de..ac137b6a1dc1 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -1459,7 +1459,6 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  		cdns_uart_uart_driver.nr = CDNS_UART_NR_PORTS;
>  #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
>  		cdns_uart_uart_driver.cons = &cdns_uart_console;
> -		cdns_uart_console.index = id;
>  #endif
>  
>  		rc = uart_register_driver(&cdns_uart_uart_driver);
> 

Reviewed-by: Michal Simek <michal.simek@xilinx.com>
Tested-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

[PATCH] tty: xilinx_uartps: Really fix id assignment

[Helmut Grohne]

The problems started with the revert (18cc7ac8a28e28). The
cdns_uart_console.index is statically assigned -1. When the port is
registered, Linux assigns consecutive numbers to it. It turned out that
when using ttyPS1 as console, the index is not updated as we are reusing
the same cdns_uart_console instance for multiple ports. When registering
ttyPS0, it gets updated from -1 to 0, but when registering ttyPS1, it
already is 0 and not updated.

That led to 2ae11c46d5fdc4. It assigns the index prior to registering
the uart_driver once. Unfortunately, that ended up breaking the
situation where the probe order does not match the id order. When using
the same device tree for both uboot and linux, it is important that the
serial0 alias points to the console. So some boards reverse those
aliases. This was reported by Jan Kiszka. The proposed fix was reverting
the index assignment and going back to the previous iteration.

However such a reversed assignement (serial0 -> uart1, serial1 -> uart0)
was already partially broken by the revert (18cc7ac8a28e28). While the
ttyPS device works, the kmsg connection is already broken and kernel
messages go missing. Reverting the id assignment does not fix this.

From the xilinx_uartps driver pov (after reverting the refactoring
commits), there can be only one console. This manifests in static
variables console_pprt and cdns_uart_console. These variables are not
properly linked and can go out of sync. The cdns_uart_console.index is
important for uart_add_one_port. We call that function for each port -
one of which hopefully is the console. If it isn't, the CON_ENABLED flag
is not set and console_port is cleared. The next cdns_uart_probe call
then tries to register the next port using that same cdns_uart_console.

It is important that console_port and cdns_uart_console (and its index
in particular) stay in sync. The index assignment implemented by
Shubhrajyoti Datta is correct in principle. It just may have to happen a
second time if the first cdns_uart_probe call didn't encounter the
console device. And we shouldn't change the index once the console uart
is registered.

Reported-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Reported-by: Jan Kiszka <jan.kiszka@web.de>
Link: https://lore.kernel.org/linux-serial/f4092727-d8f5-5f91-2c9f-76643aace993@siemens.com/
Fixes: 18cc7ac8a28e28 ("Revert "serial: uartps: Register own uart console and driver structures"")
Fixes: 2ae11c46d5fdc4 ("tty: xilinx_uartps: Fix missing id assignment to the console")
Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
---
 drivers/tty/serial/xilinx_uartps.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index b9d672af8b65..2833f1418d6d 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1465,7 +1465,6 @@ static int cdns_uart_probe(struct platform_device *pdev)
 		cdns_uart_uart_driver.nr = CDNS_UART_NR_PORTS;
 #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
 		cdns_uart_uart_driver.cons = &cdns_uart_console;
-		cdns_uart_console.index = id;
 #endif
 
 		rc = uart_register_driver(&cdns_uart_uart_driver);
@@ -1581,8 +1580,10 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	 * If register_console() don't assign value, then console_port pointer
 	 * is cleanup.
 	 */
-	if (!console_port)
+	if (!console_port) {
+		cdns_uart_console.index = id;
 		console_port = port;
+	}
 #endif
 
 	rc = uart_add_one_port(&cdns_uart_uart_driver, port);
@@ -1595,8 +1596,10 @@ static int cdns_uart_probe(struct platform_device *pdev)
 #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
 	/* This is not port which is used for console that's why clean it up */
 	if (console_port == port &&
-	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED))
+	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED)) {
 		console_port = NULL;
+		cdns_uart_console.index = -1;
+	}
 #endif
 
 	cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
-- 
2.20.1

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Michal Simek]

Hi,

On 09. 07. 20 9:48, Helmut Grohne wrote:
> The problems started with the revert (18cc7ac8a28e28). The
> cdns_uart_console.index is statically assigned -1. When the port is
> registered, Linux assigns consecutive numbers to it. It turned out that
> when using ttyPS1 as console, the index is not updated as we are reusing
> the same cdns_uart_console instance for multiple ports. When registering
> ttyPS0, it gets updated from -1 to 0, but when registering ttyPS1, it
> already is 0 and not updated.
> 
> That led to 2ae11c46d5fdc4. It assigns the index prior to registering
> the uart_driver once. Unfortunately, that ended up breaking the
> situation where the probe order does not match the id order. When using
> the same device tree for both uboot and linux, it is important that the
> serial0 alias points to the console. So some boards reverse those
> aliases. This was reported by Jan Kiszka. The proposed fix was reverting
> the index assignment and going back to the previous iteration.
> 
> However such a reversed assignement (serial0 -> uart1, serial1 -> uart0)
> was already partially broken by the revert (18cc7ac8a28e28). While the
> ttyPS device works, the kmsg connection is already broken and kernel
> messages go missing. Reverting the id assignment does not fix this.
> 
> From the xilinx_uartps driver pov (after reverting the refactoring
> commits), there can be only one console. This manifests in static
> variables console_pprt and cdns_uart_console. These variables are not
> properly linked and can go out of sync. The cdns_uart_console.index is
> important for uart_add_one_port. We call that function for each port -
> one of which hopefully is the console. If it isn't, the CON_ENABLED flag
> is not set and console_port is cleared. The next cdns_uart_probe call
> then tries to register the next port using that same cdns_uart_console.
> 
> It is important that console_port and cdns_uart_console (and its index
> in particular) stay in sync. The index assignment implemented by
> Shubhrajyoti Datta is correct in principle. It just may have to happen a
> second time if the first cdns_uart_probe call didn't encounter the
> console device. And we shouldn't change the index once the console uart
> is registered.
> 
> Reported-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Link: https://lore.kernel.org/linux-serial/f4092727-d8f5-5f91-2c9f-76643aace993@siemens.com/
> Fixes: 18cc7ac8a28e28 ("Revert "serial: uartps: Register own uart console and driver structures"")
> Fixes: 2ae11c46d5fdc4 ("tty: xilinx_uartps: Fix missing id assignment to the console")
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index b9d672af8b65..2833f1418d6d 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -1465,7 +1465,6 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  		cdns_uart_uart_driver.nr = CDNS_UART_NR_PORTS;
>  #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
>  		cdns_uart_uart_driver.cons = &cdns_uart_console;
> -		cdns_uart_console.index = id;
>  #endif
>  
>  		rc = uart_register_driver(&cdns_uart_uart_driver);
> @@ -1581,8 +1580,10 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	 * If register_console() don't assign value, then console_port pointer
>  	 * is cleanup.
>  	 */
> -	if (!console_port)
> +	if (!console_port) {
> +		cdns_uart_console.index = id;
>  		console_port = port;
> +	}
>  #endif
>  
>  	rc = uart_add_one_port(&cdns_uart_uart_driver, port);
> @@ -1595,8 +1596,10 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
>  	/* This is not port which is used for console that's why clean it up */
>  	if (console_port == port &&
> -	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED))
> +	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED)) {
>  		console_port = NULL;
> +		cdns_uart_console.index = -1;
> +	}
>  #endif
>  
>  	cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
> 

Thanks for the patch. I think that it will be very useful to list out
all testcases to make sure that we test all that scenarios and finally
get over it.
We are testing scenarios and likely we are missing some that we are not
able catch all issues before we submit a patch.

Thanks,
Michal

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Helmut Grohne]

Hi Michal,

On Fri, Jul 10, 2020 at 01:45:21PM +0200, Michal Simek wrote:
> Thanks for the patch. I think that it will be very useful to list out
> all testcases to make sure that we test all that scenarios and finally
> get over it.
> We are testing scenarios and likely we are missing some that we are not
> able catch all issues before we submit a patch.

Let me try to enumerate those I know:

uart0    | uart1    | console | remark
---------+----------+---------+----------
serial0  | serial1  | ttyPS0  | regular case
serial0  | serial1  | ttyPS1  | normal assignment, second console
serial1  | serial0  | ttyPS0  | -> Jan Kiszka, broken since revert
disabled | serial0  | ttyPS0  | use only uart1 as serial0
serial0  | disabled | ttyPS0  | regular case with uart1 disabled

Out of these, I'm actively using configurations 3 and 4.

Which of these scenarios do you test already?

Helmut

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Michal Simek]

On 13. 07. 20 9:11, Helmut Grohne wrote:
> Hi Michal,
> 
> On Fri, Jul 10, 2020 at 01:45:21PM +0200, Michal Simek wrote:
>> Thanks for the patch. I think that it will be very useful to list out
>> all testcases to make sure that we test all that scenarios and finally
>> get over it.
>> We are testing scenarios and likely we are missing some that we are not
>> able catch all issues before we submit a patch.
> 
> Let me try to enumerate those I know:
> 
> uart0    | uart1    | console | remark
> ---------+----------+---------+----------
> serial0  | serial1  | ttyPS0  | regular case
> serial0  | serial1  | ttyPS1  | normal assignment, second console
> serial1  | serial0  | ttyPS0  | -> Jan Kiszka, broken since revert
> disabled | serial0  | ttyPS0  | use only uart1 as serial0
> serial0  | disabled | ttyPS0  | regular case with uart1 disabled
> 
> Out of these, I'm actively using configurations 3 and 4.
> 
> Which of these scenarios do you test already?

For above we are missing also others
serial1 | serial0 | ttyPS1
disabled| serial1 | ttyPS1
serial1 | disables | ttyPS0

All of these above are just not setting any console= on bootargs.
All of these should be tested with earlycon in bootargs to also test
earlyconsole.
All combinations with both uarts should be also tested with
console=ttyPS0,115200 and console=ttyPS1,115200.

Driver supports up to 16 ports. It means also some ttyPS2 and more
should be also tested.

What I found recently in connection to Jan's issue was that testing was
done with bootargs setup which prevent seeing his issue on ultra96.

It means mix of these combinations is tested regularly but not all of
them. Do you see any other combination which is not supported?
When we have list I will ask testing team to test all combinations.

Thanks,
Michal

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Helmut Grohne]

Hi Michal,

On Mon, Jul 13, 2020 at 01:49:38PM +0200, Michal Simek wrote:
> On 13. 07. 20 9:11, Helmut Grohne wrote:
> > Let me try to enumerate those I know:
> > 
> > uart0    | uart1    | console | remark
> > ---------+----------+---------+----------
> > serial0  | serial1  | ttyPS0  | regular case
> > serial0  | serial1  | ttyPS1  | normal assignment, second console
> > serial1  | serial0  | ttyPS0  | -> Jan Kiszka, broken since revert
> > disabled | serial0  | ttyPS0  | use only uart1 as serial0
> > serial0  | disabled | ttyPS0  | regular case with uart1 disabled
> > 
> > Out of these, I'm actively using configurations 3 and 4.
> > 
> > Which of these scenarios do you test already?
> 
> For above we are missing also others
> serial1 | serial0 | ttyPS1
> disabled| serial1 | ttyPS1

Is it actually possible to have ttyPS1, but no ttyPS0? I think I tried
doing that earlier and it resulted in there being ttyPS0, but no ttyPS1.

> serial1 | disables | ttyPS0

I'm not sure what this is supposed to mean. When there is no serial0
alias, I'd expect ttyPS0 to be missing. However as indicated above that
is not what happens in practice. So either of these two configurations
seems invalid to me.

> All of these above are just not setting any console= on bootargs.

We usually set the console= assignment on bootargs.

> It means mix of these combinations is tested regularly but not all of
> them. Do you see any other combination which is not supported?

I'm not aware of further relevant combinations.

Can we maybe trim down the matrix somehow? In my context, the need for
swapping the serial aliases arises from a limitation in u-boot-xlnx and
the desire to use one dtb for both linux and u-boot. It requires that
the serial0 alias is the console. Are there other reasons to swap them?
If not, maybe fixing u-boot would be an option?

Helmut

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Maarten Brock]

On 2020-07-13 14:10, Helmut Grohne wrote:
> Hi Michal,
> 
> On Mon, Jul 13, 2020 at 01:49:38PM +0200, Michal Simek wrote:
>> On 13. 07. 20 9:11, Helmut Grohne wrote:
>> > Let me try to enumerate those I know:
>> >
>> > uart0    | uart1    | console | remark
>> > ---------+----------+---------+----------
>> > serial0  | serial1  | ttyPS0  | regular case
>> > serial0  | serial1  | ttyPS1  | normal assignment, second console
>> > serial1  | serial0  | ttyPS0  | -> Jan Kiszka, broken since revert
>> > disabled | serial0  | ttyPS0  | use only uart1 as serial0
>> > serial0  | disabled | ttyPS0  | regular case with uart1 disabled
>> >
>> > Out of these, I'm actively using configurations 3 and 4.
>> >
>> > Which of these scenarios do you test already?
>> 
>> For above we are missing also others
>> serial1 | serial0 | ttyPS1
>> disabled| serial1 | ttyPS1
> 
> Is it actually possible to have ttyPS1, but no ttyPS0? I think I tried
> doing that earlier and it resulted in there being ttyPS0, but no 
> ttyPS1.

What if you also have a 16550 (in the PL) and give it the serial0 alias?
Or a UARTlite? The serialN alias is inappropriate to set the number for
ttyPSn. How are you supposed to create all of ttyPS0, ttyS0 and ttyUL0
using a single serial0 alias?

>> serial1 | disables | ttyPS0
> 
> I'm not sure what this is supposed to mean. When there is no serial0
> alias, I'd expect ttyPS0 to be missing. However as indicated above that
> is not what happens in practice. So either of these two configurations
> seems invalid to me.
> 
>> All of these above are just not setting any console= on bootargs.
> 
> We usually set the console= assignment on bootargs.
> 
>> It means mix of these combinations is tested regularly but not all of
>> them. Do you see any other combination which is not supported?
> 
> I'm not aware of further relevant combinations.
> 
> Can we maybe trim down the matrix somehow? In my context, the need for
> swapping the serial aliases arises from a limitation in u-boot-xlnx and
> the desire to use one dtb for both linux and u-boot. It requires that
> the serial0 alias is the console. Are there other reasons to swap them?
> If not, maybe fixing u-boot would be an option?
> 
> Helmut

I think that it would be better if u-boot used a "console" alias.

Maarten

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Michal Simek]

On 13. 07. 20 18:08, Maarten Brock wrote:
> On 2020-07-13 14:10, Helmut Grohne wrote:
>> Hi Michal,
>>
>> On Mon, Jul 13, 2020 at 01:49:38PM +0200, Michal Simek wrote:
>>> On 13. 07. 20 9:11, Helmut Grohne wrote:
>>> > Let me try to enumerate those I know:
>>> >
>>> > uart0    | uart1    | console | remark
>>> > ---------+----------+---------+----------
>>> > serial0  | serial1  | ttyPS0  | regular case
>>> > serial0  | serial1  | ttyPS1  | normal assignment, second console
>>> > serial1  | serial0  | ttyPS0  | -> Jan Kiszka, broken since revert
>>> > disabled | serial0  | ttyPS0  | use only uart1 as serial0
>>> > serial0  | disabled | ttyPS0  | regular case with uart1 disabled
>>> >
>>> > Out of these, I'm actively using configurations 3 and 4.
>>> >
>>> > Which of these scenarios do you test already?
>>>
>>> For above we are missing also others
>>> serial1 | serial0 | ttyPS1
>>> disabled| serial1 | ttyPS1
>>
>> Is it actually possible to have ttyPS1, but no ttyPS0? I think I tried
>> doing that earlier and it resulted in there being ttyPS0, but no ttyPS1.
> 
> What if you also have a 16550 (in the PL) and give it the serial0 alias?
> Or a UARTlite? The serialN alias is inappropriate to set the number for
> ttyPSn. How are you supposed to create all of ttyPS0, ttyS0 and ttyUL0
> using a single serial0 alias?

yes this combination is not possible and I don't think this is xilinx
specific issue.
I expect the same problem you have with ttyAMA, ttyS and others.


>>> serial1 | disables | ttyPS0
>>
>> I'm not sure what this is supposed to mean. When there is no serial0
>> alias, I'd expect ttyPS0 to be missing. However as indicated above that
>> is not what happens in practice. So either of these two configurations
>> seems invalid to me.
>>
>>> All of these above are just not setting any console= on bootargs.
>>
>> We usually set the console= assignment on bootargs.
>>
>>> It means mix of these combinations is tested regularly but not all of
>>> them. Do you see any other combination which is not supported?
>>
>> I'm not aware of further relevant combinations.
>>
>> Can we maybe trim down the matrix somehow? In my context, the need for
>> swapping the serial aliases arises from a limitation in u-boot-xlnx and
>> the desire to use one dtb for both linux and u-boot. It requires that
>> the serial0 alias is the console. Are there other reasons to swap them?
>> If not, maybe fixing u-boot would be an option?
>>
>> Helmut
> 
> I think that it would be better if u-boot used a "console" alias.

console is defined in bootargs which is OS specific feature. U-Boot has
no idea what ttyPS, ttyS, etc means. That's why I don't think there is
something wrong in this in u-boot. But please elaborate more on this
because I am not aware about any issue on u-boot configuration.

Thanks,
Michal

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Maarten Brock]

On 2020-07-22 09:18, Michal Simek wrote:
> On 13. 07. 20 18:08, Maarten Brock wrote:
>> On 2020-07-13 14:10, Helmut Grohne wrote:
>>> Hi Michal,
>>> 
>>> On Mon, Jul 13, 2020 at 01:49:38PM +0200, Michal Simek wrote:
>>>> On 13. 07. 20 9:11, Helmut Grohne wrote:
>>>> > Let me try to enumerate those I know:
>>>> >
>>>> > uart0    | uart1    | console | remark
>>>> > ---------+----------+---------+----------
>>>> > serial0  | serial1  | ttyPS0  | regular case
>>>> > serial0  | serial1  | ttyPS1  | normal assignment, second console
>>>> > serial1  | serial0  | ttyPS0  | -> Jan Kiszka, broken since revert
>>>> > disabled | serial0  | ttyPS0  | use only uart1 as serial0
>>>> > serial0  | disabled | ttyPS0  | regular case with uart1 disabled
>>>> >
>>>> > Out of these, I'm actively using configurations 3 and 4.
>>>> >
>>>> > Which of these scenarios do you test already?
>>>> 
>>>> For above we are missing also others
>>>> serial1 | serial0 | ttyPS1
>>>> disabled| serial1 | ttyPS1
>>> 
>>> Is it actually possible to have ttyPS1, but no ttyPS0? I think I 
>>> tried
>>> doing that earlier and it resulted in there being ttyPS0, but no 
>>> ttyPS1.
>> 
>> What if you also have a 16550 (in the PL) and give it the serial0 
>> alias?
>> Or a UARTlite? The serialN alias is inappropriate to set the number 
>> for
>> ttyPSn. How are you supposed to create all of ttyPS0, ttyS0 and ttyUL0
>> using a single serial0 alias?
> 
> yes this combination is not possible and I don't think this is xilinx
> specific issue.
> I expect the same problem you have with ttyAMA, ttyS and others.

Well, it is very easy to add a 16550 in the programmable logic of a 
Zynq.
Worse, it's impossible to only add uartps devices as the IP for it is
not available to the public.
It is less easy to add a 16550 to a CPU with ttyAMA but no external bus.
But if you add e.g. an I2C/SPI based SC16IS7xx which generates ttySCx 
you
might have the same problems.

But the problem is worse. What happens if you give the serial0 alias to
a xilinx_uartps and the 16550 driver has already taken ttyS0? (Or vice
versa?) Will the uartps still use ttyPS0 or will it ignore the serial0
alias? I predict the latter.

I see only two ways out.
* Let uartps generate ttySx device names, or
* Do not use serialN alias to set the number.

It was already stated that it is impossible to have ttyPS1 and no 
ttyPS0.
That would mean we cannot give serial0 to ttyS0 and serial1 to ttyPS1.
This makes me wonder if the opposite is valid: to give serial0 to ttyPS0
and serial1 to ttyS1. Probably not either.

There really needs to be a way to create deterministic names for the
devices!

>>>> serial1 | disables | ttyPS0
>>> 
>>> I'm not sure what this is supposed to mean. When there is no serial0
>>> alias, I'd expect ttyPS0 to be missing. However as indicated above 
>>> that
>>> is not what happens in practice. So either of these two 
>>> configurations
>>> seems invalid to me.
>>> 
>>>> All of these above are just not setting any console= on bootargs.
>>> 
>>> We usually set the console= assignment on bootargs.
>>> 
>>>> It means mix of these combinations is tested regularly but not all 
>>>> of
>>>> them. Do you see any other combination which is not supported?
>>> 
>>> I'm not aware of further relevant combinations.
>>> 
>>> Can we maybe trim down the matrix somehow? In my context, the need 
>>> for
>>> swapping the serial aliases arises from a limitation in u-boot-xlnx 
>>> and
>>> the desire to use one dtb for both linux and u-boot. It requires that
>>> the serial0 alias is the console. Are there other reasons to swap 
>>> them?
>>> If not, maybe fixing u-boot would be an option?
>>> 
>>> Helmut
>> 
>> I think that it would be better if u-boot used a "console" alias.
> 
> console is defined in bootargs which is OS specific feature. U-Boot has
> no idea what ttyPS, ttyS, etc means. That's why I don't think there is
> something wrong in this in u-boot. But please elaborate more on this
> because I am not aware about any issue on u-boot configuration.
> 
> Thanks,
> Michal

What I meant to say is that apparently U-boot requires serial0 to point 
to
the user-interface. This limits your options when assigning aliases. If
U-boot would use a different entry (e.g. "console" or better yet
"earlycon") things might be easier. serial0 should not be special IMHO.

But let's not diverge too much here.

Maarten

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Michal Simek]

On 22. 07. 20 18:50, Maarten Brock wrote:
> On 2020-07-22 09:18, Michal Simek wrote:
>> On 13. 07. 20 18:08, Maarten Brock wrote:
>>> On 2020-07-13 14:10, Helmut Grohne wrote:
>>>> Hi Michal,
>>>>
>>>> On Mon, Jul 13, 2020 at 01:49:38PM +0200, Michal Simek wrote:
>>>>> On 13. 07. 20 9:11, Helmut Grohne wrote:
>>>>> > Let me try to enumerate those I know:
>>>>> >
>>>>> > uart0    | uart1    | console | remark
>>>>> > ---------+----------+---------+----------
>>>>> > serial0  | serial1  | ttyPS0  | regular case
>>>>> > serial0  | serial1  | ttyPS1  | normal assignment, second console
>>>>> > serial1  | serial0  | ttyPS0  | -> Jan Kiszka, broken since revert
>>>>> > disabled | serial0  | ttyPS0  | use only uart1 as serial0
>>>>> > serial0  | disabled | ttyPS0  | regular case with uart1 disabled
>>>>> >
>>>>> > Out of these, I'm actively using configurations 3 and 4.
>>>>> >
>>>>> > Which of these scenarios do you test already?
>>>>>
>>>>> For above we are missing also others
>>>>> serial1 | serial0 | ttyPS1
>>>>> disabled| serial1 | ttyPS1
>>>>
>>>> Is it actually possible to have ttyPS1, but no ttyPS0? I think I tried
>>>> doing that earlier and it resulted in there being ttyPS0, but no
>>>> ttyPS1.
>>>
>>> What if you also have a 16550 (in the PL) and give it the serial0 alias?
>>> Or a UARTlite? The serialN alias is inappropriate to set the number for
>>> ttyPSn. How are you supposed to create all of ttyPS0, ttyS0 and ttyUL0
>>> using a single serial0 alias?
>>
>> yes this combination is not possible and I don't think this is xilinx
>> specific issue.
>> I expect the same problem you have with ttyAMA, ttyS and others.
> 
> Well, it is very easy to add a 16550 in the programmable logic of a Zynq.

agree.

> Worse, it's impossible to only add uartps devices as the IP for it is
> not available to the public.

I expect you mean to PL. It is cadence IP and Xilinx is not the right
company who should releasing this paid IP to public.

> It is less easy to add a 16550 to a CPU with ttyAMA but no external bus.

Nope. Xilinx latest SoC called versal is using pl011/sbsa uart instead
of cadence one that's why it is as simple as was on zynq/zynqmp.

On Xilinx devices you can regularly see mixing devices with ttyPS, ttyS,
ttyAMA and ttyUL.
There could be others but they are more rare cases.

> But if you add e.g. an I2C/SPI based SC16IS7xx which generates ttySCx you
> might have the same problems.

yes and a lot of other examples are also present.

> 
> But the problem is worse. What happens if you give the serial0 alias to
> a xilinx_uartps and the 16550 driver has already taken ttyS0? (Or vice
> versa?) Will the uartps still use ttyPS0 or will it ignore the serial0
> alias? I predict the latter.

yes and this issue is around I expect from time where the first !ttyS
was created.
I my series to this driver (which is reverted now as incorrect design) I
was using bitmaps to find the highest free alias at least for this driver.
But for quite a long time there is commit 351d224f64af ("of: base: add
function to get highest id of an alias stem") which return the highest
ID for certain device type which could be used.

DT alias list is stable it means if driver supports it all the time you
should get order based on it.
It means you shouldn't never reach situation that you will have ttyS0
and ttyPS0 because only one device have serial0 alias.

If uart IP is not listed in alias list you should get highest id and
starts to count from there.
But there is issue related to this too which is that every driver has
NR_PORTS defined. In this case we have 16 in the driver.
When you have serial16 = &uart0 then you can't register it as ttyPS16.

My series was trying to solve it but it was reverted and I haven't had a
time to take a look at it again.

> 
> I see only two ways out.
> * Let uartps generate ttySx device names, or

It is not just about uartps but about all drivers and just use ttySx.
(not sure if this should be also for ttyUSBx devices)

> * Do not use serialN alias to set the number.

If this support is removed I expect it will caused a lot of issues for
others SOCs.


> It was already stated that it is impossible to have ttyPS1 and no ttyPS0.
> That would mean we cannot give serial0 to ttyS0 and serial1 to ttyPS1.
> This makes me wonder if the opposite is valid: to give serial0 to ttyPS0
> and serial1 to ttyS1. Probably not either.

Please take a look at Helmut's reply where he retest this scenario and
having ttyPS1 without ttyPS0 is possible.

> 
> There really needs to be a way to create deterministic names for the
> devices!

If you have alias list described for all devices you have deterministic
names already.
But the highest ID in name is limited by every driver which seems to
limitation which shouldn't be there.
If I write for example serial1000 = &whatever_uart; then I would expect
that I will get ttyX1000 which points to this device.

And as was also confirmed alias list is stable for base dts file. When
you start to work with device tree overlays alias list can't be extended
with newly added devices which has to use dynamic ID assignment.


>>>>> serial1 | disables | ttyPS0
>>>>
>>>> I'm not sure what this is supposed to mean. When there is no serial0
>>>> alias, I'd expect ttyPS0 to be missing. However as indicated above that
>>>> is not what happens in practice. So either of these two configurations
>>>> seems invalid to me.
>>>>
>>>>> All of these above are just not setting any console= on bootargs.
>>>>
>>>> We usually set the console= assignment on bootargs.
>>>>
>>>>> It means mix of these combinations is tested regularly but not all of
>>>>> them. Do you see any other combination which is not supported?
>>>>
>>>> I'm not aware of further relevant combinations.
>>>>
>>>> Can we maybe trim down the matrix somehow? In my context, the need for
>>>> swapping the serial aliases arises from a limitation in u-boot-xlnx and
>>>> the desire to use one dtb for both linux and u-boot. It requires that
>>>> the serial0 alias is the console. Are there other reasons to swap them?
>>>> If not, maybe fixing u-boot would be an option?
>>>>
>>>> Helmut
>>>
>>> I think that it would be better if u-boot used a "console" alias.
>>
>> console is defined in bootargs which is OS specific feature. U-Boot has
>> no idea what ttyPS, ttyS, etc means. That's why I don't think there is
>> something wrong in this in u-boot. But please elaborate more on this
>> because I am not aware about any issue on u-boot configuration.
>>
>> Thanks,
>> Michal
> 
> What I meant to say is that apparently U-boot requires serial0 to point to
> the user-interface. This limits your options when assigning aliases. If
> U-boot would use a different entry (e.g. "console" or better yet
> "earlycon") things might be easier. serial0 should not be special IMHO.
> 
> But let's not diverge too much here.

U-Boot is using stdout path and I am not aware about limitation to have
serial0 alias filled. If stdout is pointing to serial444 then u-boot
should be using it as console without any issue.
If you see any issue in u-boot please send email to u-boot mailing list
and CC me and we can take a look at it but as of today I am not aware
about any issue related to this.

Even serial uclass records serial alias (req) for drivers properly.

	aliases {
		serial0 = &uart0;
		serial1 = &uart1;
		serial2 = &dcc;
		serial1000 = &uartlite;
	};

uclass 80: serial
0     dcc @ 7deb8230, seq -1, (req 2)
1   * serial@ff000000 @ 7deb9db0, seq 0, (req 0)
2     serial@ff010000 @ 7deb9e80, seq -1, (req 1)
3     serial@800c0000 @ 7deba960, seq -1, (req 1000)


Thanks,
Michal

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Michal Simek]

On 13. 07. 20 14:10, Helmut Grohne wrote:
> Hi Michal,
> 
> On Mon, Jul 13, 2020 at 01:49:38PM +0200, Michal Simek wrote:
>> On 13. 07. 20 9:11, Helmut Grohne wrote:
>>> Let me try to enumerate those I know:
>>>
>>> uart0    | uart1    | console | remark
>>> ---------+----------+---------+----------
>>> serial0  | serial1  | ttyPS0  | regular case
>>> serial0  | serial1  | ttyPS1  | normal assignment, second console
>>> serial1  | serial0  | ttyPS0  | -> Jan Kiszka, broken since revert
>>> disabled | serial0  | ttyPS0  | use only uart1 as serial0
>>> serial0  | disabled | ttyPS0  | regular case with uart1 disabled
>>>
>>> Out of these, I'm actively using configurations 3 and 4.
>>>
>>> Which of these scenarios do you test already?
>>
>> For above we are missing also others
>> serial1 | serial0 | ttyPS1
>> disabled| serial1 | ttyPS1
> 
> Is it actually possible to have ttyPS1, but no ttyPS0? I think I tried
> doing that earlier and it resulted in there being ttyPS0, but no ttyPS1.

serial1 alias should all the time create ttyPS1 device.
serial0 ttyPS0.

If there are no serial aliases driver should started numbering them from 0.


>> serial1 | disables | ttyPS0
> 
> I'm not sure what this is supposed to mean. When there is no serial0
> alias, I'd expect ttyPS0 to be missing. However as indicated above that
> is not what happens in practice. So either of these two configurations
> seems invalid to me.

You are right on this one - this is not possible. It should be ttyPS1.

> 
>> All of these above are just not setting any console= on bootargs.
> 
> We usually set the console= assignment on bootargs.

I found some time ago that setting up console in bootargs was covering
issue reported by Jan but didn't have time to look at it in more details.

> 
>> It means mix of these combinations is tested regularly but not all of
>> them. Do you see any other combination which is not supported?
> 
> I'm not aware of further relevant combinations.
> 
> Can we maybe trim down the matrix somehow? In my context, the need for
> swapping the serial aliases arises from a limitation in u-boot-xlnx and
> the desire to use one dtb for both linux and u-boot. It requires that
> the serial0 alias is the console. Are there other reasons to swap them?
> If not, maybe fixing u-boot would be an option?

I am not aware about any limitation in u-boot in this space. U-Boot is
working with aliases and also with stdout-patch selection.
Can you please elaborate more on this?

Thanks,
Michal

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Helmut Grohne]

Hi Michal,

On Wed, Jul 22, 2020 at 09:14:40AM +0200, Michal Simek wrote:
> On 13. 07. 20 14:10, Helmut Grohne wrote:
> > Can we maybe trim down the matrix somehow? In my context, the need for
> > swapping the serial aliases arises from a limitation in u-boot-xlnx and
> > the desire to use one dtb for both linux and u-boot. It requires that
> > the serial0 alias is the console. Are there other reasons to swap them?
> > If not, maybe fixing u-boot would be an option?
> 
> I am not aware about any limitation in u-boot in this space. U-Boot is
> working with aliases and also with stdout-patch selection.
> Can you please elaborate more on this?

I have retested this (with more recent versions than earlier).

I confirm that u-boot correctly deals with aliases and
chosen/stdout-path. Using linear aliases and assigning stdout to serial1
works.

I also confirm that having ttyPS1 without having ttyPS0 works (only
tested with this patch applied). I questioned this earlier.

So it seems that the need for reversing these aliases is less strong
now. If I remember correctly, Jan Kiszka was also using reversed
aliases, but I don't know why.

Helmut

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Michal Simek]

On 23. 07. 20 11:50, Helmut Grohne wrote:
> Hi Michal,
> 
> On Wed, Jul 22, 2020 at 09:14:40AM +0200, Michal Simek wrote:
>> On 13. 07. 20 14:10, Helmut Grohne wrote:
>>> Can we maybe trim down the matrix somehow? In my context, the need for
>>> swapping the serial aliases arises from a limitation in u-boot-xlnx and
>>> the desire to use one dtb for both linux and u-boot. It requires that
>>> the serial0 alias is the console. Are there other reasons to swap them?
>>> If not, maybe fixing u-boot would be an option?
>>
>> I am not aware about any limitation in u-boot in this space. U-Boot is
>> working with aliases and also with stdout-patch selection.
>> Can you please elaborate more on this?
> 
> I have retested this (with more recent versions than earlier).
> 
> I confirm that u-boot correctly deals with aliases and
> chosen/stdout-path. Using linear aliases and assigning stdout to serial1
> works.
> 
> I also confirm that having ttyPS1 without having ttyPS0 works (only
> tested with this patch applied). I questioned this earlier.

Thanks for confirmation.

> 
> So it seems that the need for reversing these aliases is less strong
> now. If I remember correctly, Jan Kiszka was also using reversed
> aliases, but I don't know why.

ultra96 is using both uarts. Where physical uart0 is connected to
bt/wifi TI chip which requires flow control which needs to be done via
PL (really just wires in PL). Physical uart1 is used for console.
That's why I have decided when I was describing this DT to use aliases
that ttyPS0 will be console and ttyPS1 will be connection to bt/wifi chip.

It is more convenient to have console named as ttyPS0.
I think I have seen that some SoCs are simply not changing orders for
IPs and all the time uart0 is serial0, etc. and it would also make sense.
But I have never start to use it and mostly for historical reason Xilinx
SW didn't handle consoles on ttyPS1 properly that's why console were all
the time pointed by ttyPS0 no matter which serial IP was used.

Thanks,
Michal

Re: [PATCH] tty: xilinx_uartps: Really fix id assignment

[Greg Kroah-Hartman]

On Thu, Jul 09, 2020 at 09:48:53AM +0200, Helmut Grohne wrote:
> The problems started with the revert (18cc7ac8a28e28). The
> cdns_uart_console.index is statically assigned -1. When the port is
> registered, Linux assigns consecutive numbers to it. It turned out that
> when using ttyPS1 as console, the index is not updated as we are reusing
> the same cdns_uart_console instance for multiple ports. When registering
> ttyPS0, it gets updated from -1 to 0, but when registering ttyPS1, it
> already is 0 and not updated.
> 
> That led to 2ae11c46d5fdc4. It assigns the index prior to registering
> the uart_driver once. Unfortunately, that ended up breaking the
> situation where the probe order does not match the id order. When using
> the same device tree for both uboot and linux, it is important that the
> serial0 alias points to the console. So some boards reverse those
> aliases. This was reported by Jan Kiszka. The proposed fix was reverting
> the index assignment and going back to the previous iteration.
> 
> However such a reversed assignement (serial0 -> uart1, serial1 -> uart0)
> was already partially broken by the revert (18cc7ac8a28e28). While the
> ttyPS device works, the kmsg connection is already broken and kernel
> messages go missing. Reverting the id assignment does not fix this.
> 
> >From the xilinx_uartps driver pov (after reverting the refactoring
> commits), there can be only one console. This manifests in static
> variables console_pprt and cdns_uart_console. These variables are not
> properly linked and can go out of sync. The cdns_uart_console.index is
> important for uart_add_one_port. We call that function for each port -
> one of which hopefully is the console. If it isn't, the CON_ENABLED flag
> is not set and console_port is cleared. The next cdns_uart_probe call
> then tries to register the next port using that same cdns_uart_console.
> 
> It is important that console_port and cdns_uart_console (and its index
> in particular) stay in sync. The index assignment implemented by
> Shubhrajyoti Datta is correct in principle. It just may have to happen a
> second time if the first cdns_uart_probe call didn't encounter the
> console device. And we shouldn't change the index once the console uart
> is registered.
> 
> Reported-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Reported-by: Jan Kiszka <jan.kiszka@web.de>
> Link: https://lore.kernel.org/linux-serial/f4092727-d8f5-5f91-2c9f-76643aace993@siemens.com/
> Fixes: 18cc7ac8a28e28 ("Revert "serial: uartps: Register own uart console and driver structures"")
> Fixes: 2ae11c46d5fdc4 ("tty: xilinx_uartps: Fix missing id assignment to the console")
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

What tree/branch is this against?  It doesn't seem to apply to my
tty-linus branch which is where I would think it should go to, right?

thanks,

greg k-h

[PATCH v2] tty: xilinx_uartps: Really fix id assignment

[Helmut Grohne]

The problems started with the revert (18cc7ac8a28e28). The
cdns_uart_console.index is statically assigned -1. When the port is
registered, Linux assigns consecutive numbers to it. It turned out that
when using ttyPS1 as console, the index is not updated as we are reusing
the same cdns_uart_console instance for multiple ports. When registering
ttyPS0, it gets updated from -1 to 0, but when registering ttyPS1, it
already is 0 and not updated.

That led to 2ae11c46d5fdc4. It assigns the index prior to registering
the uart_driver once. Unfortunately, that ended up breaking the
situation where the probe order does not match the id order. When using
the same device tree for both uboot and linux, it is important that the
serial0 alias points to the console. So some boards reverse those
aliases. This was reported by Jan Kiszka. The proposed fix was reverting
the index assignment and going back to the previous iteration.

However such a reversed assignement (serial0 -> uart1, serial1 -> uart0)
was already partially broken by the revert (18cc7ac8a28e28). While the
ttyPS device works, the kmsg connection is already broken and kernel
messages go missing. Reverting the id assignment does not fix this.

From the xilinx_uartps driver pov (after reverting the refactoring
commits), there can be only one console. This manifests in static
variables console_pprt and cdns_uart_console. These variables are not
properly linked and can go out of sync. The cdns_uart_console.index is
important for uart_add_one_port. We call that function for each port -
one of which hopefully is the console. If it isn't, the CON_ENABLED flag
is not set and console_port is cleared. The next cdns_uart_probe call
then tries to register the next port using that same cdns_uart_console.

It is important that console_port and cdns_uart_console (and its index
in particular) stay in sync. The index assignment implemented by
Shubhrajyoti Datta is correct in principle. It just may have to happen a
second time if the first cdns_uart_probe call didn't encounter the
console device. And we shouldn't change the index once the console uart
is registered.

Reported-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Reported-by: Jan Kiszka <jan.kiszka@web.de>
Link: https://lore.kernel.org/linux-serial/f4092727-d8f5-5f91-2c9f-76643aace993@siemens.com/
Fixes: 18cc7ac8a28e28 ("Revert "serial: uartps: Register own uart console and driver structures"")
Fixes: 2ae11c46d5fdc4 ("tty: xilinx_uartps: Fix missing id assignment to the console")
Fixes: 76ed2e10579671 ("Revert "tty: xilinx_uartps: Fix missing id assignment to the console"")
Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
---
 drivers/tty/serial/xilinx_uartps.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Changes since v1:
 * This patch was meant to replace 76ed2e10579671. Since it is part of
   gregkh/tty-linus already, rebase this patch onto it.

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 672cfa075e28..2833f1418d6d 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1580,8 +1580,10 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	 * If register_console() don't assign value, then console_port pointer
 	 * is cleanup.
 	 */
-	if (!console_port)
+	if (!console_port) {
+		cdns_uart_console.index = id;
 		console_port = port;
+	}
 #endif
 
 	rc = uart_add_one_port(&cdns_uart_uart_driver, port);
@@ -1594,8 +1596,10 @@ static int cdns_uart_probe(struct platform_device *pdev)
 #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
 	/* This is not port which is used for console that's why clean it up */
 	if (console_port == port &&
-	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED))
+	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED)) {
 		console_port = NULL;
+		cdns_uart_console.index = -1;
+	}
 #endif
 
 	cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
-- 
2.20.1