[PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[Ruediger Willenberg]

Find free uart_port struct in range 0 <= id < ULITE_NR_UARTS when
the device tree port-number property is outside that range. This
happens when there are other UART types in the system because the
Xilinx device tree generator numbers all UARTs consecutively;
as a result, not as many Uartlites as specified by the
SERIAL_UARTLITE_NR_UARTS parameter could be successfully added.

Signed-off-by: Ruediger Willenberg <r.willenberg@hs-mannheim.de>
---
Changes in v2:
 - give KERN_NOTICE when changing the id,
   with reference to the requested port-number 

 drivers/tty/serial/uartlite.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index d3d9566e5dbd..27c513c7350e 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -631,15 +631,17 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
 {
 	struct uart_port *port;
 	int rc;
+	int oor_id = -1;
 
-	/* if id = -1; then scan for a free id and use that */
-	if (id < 0) {
+	/* if id -1 or out of range; then scan for a free id and use that */
+	if (id < 0 || id >= ULITE_NR_UARTS) {
+		oor_id = id;
 		for (id = 0; id < ULITE_NR_UARTS; id++)
 			if (ulite_ports[id].mapbase == 0)
 				break;
 	}
-	if (id < 0 || id >= ULITE_NR_UARTS) {
-		dev_err(dev, "%s%i too large\n", ULITE_NAME, id);
+	if (id == ULITE_NR_UARTS) {
+		dev_err(dev, "maximum number of %s assigned\n", ULITE_NAME);
 		return -EINVAL;
 	}
 
@@ -676,7 +678,11 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
 		dev_set_drvdata(dev, NULL);
 		return rc;
 	}
-
+	if (oor_id >= 0)
+		dev_notice(dev,
+			"assigned uartlite with device tree port-number=<%i> to %s%i\n",
+			oor_id, ULITE_NAME, id);
+
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[Michal Simek]

On 11/18/21 22:17, Ruediger Willenberg wrote:
> Find free uart_port struct in range 0 <= id < ULITE_NR_UARTS when
> the device tree port-number property is outside that range. This
> happens when there are other UART types in the system because the
> Xilinx device tree generator numbers all UARTs consecutively;
> as a result, not as many Uartlites as specified by the
> SERIAL_UARTLITE_NR_UARTS parameter could be successfully added.
> 
> Signed-off-by: Ruediger Willenberg <r.willenberg@hs-mannheim.de>
> ---
> Changes in v2:
>   - give KERN_NOTICE when changing the id,
>     with reference to the requested port-number
> 
>   drivers/tty/serial/uartlite.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index d3d9566e5dbd..27c513c7350e 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -631,15 +631,17 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
>   {
>   	struct uart_port *port;
>   	int rc;
> +	int oor_id = -1;
>   
> -	/* if id = -1; then scan for a free id and use that */
> -	if (id < 0) {
> +	/* if id -1 or out of range; then scan for a free id and use that */
> +	if (id < 0 || id >= ULITE_NR_UARTS) {
> +		oor_id = id;
>   		for (id = 0; id < ULITE_NR_UARTS; id++)
>   			if (ulite_ports[id].mapbase == 0)
>   				break;
>   	}
> -	if (id < 0 || id >= ULITE_NR_UARTS) {
> -		dev_err(dev, "%s%i too large\n", ULITE_NAME, id);
> +	if (id == ULITE_NR_UARTS) {
> +		dev_err(dev, "maximum number of %s assigned\n", ULITE_NAME);
>   		return -EINVAL;
>   	}
>   
> @@ -676,7 +678,11 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
>   		dev_set_drvdata(dev, NULL);
>   		return rc;
>   	}

nit: please keep this newline here.

> -
> +	if (oor_id >= 0)
> +		dev_notice(dev,
> +			"assigned uartlite with device tree port-number=<%i> to %s%i\n",
> +			oor_id, ULITE_NAME, id);
> +



[linux](master)$ ./scripts/checkpatch.pl --strict 
0001-serial-uartlite-Move-out-of-range-port-numbers-into-.patch
CHECK: Alignment should match open parenthesis
#54: FILE: drivers/tty/serial/uartlite.c:669:
+		dev_notice(dev,
+			"assigned uartlite with device tree port-number=<%i> to %s%i\n",



>   	return 0;
>   }
>   

And there is one more issue with this. If you start to mix serial IPs 
which are partially recorded in aliases. For example like this.

aliases {
	serial0 = &uart1;
};

uart0 {
...
};

uart1 {
...
};

Uart0 is probed first. It is without alias and id 0 will be assigned to 
it. Then uart1 is probed. It looks at aliases you get id 0 but it is 
already taken and you end up with "cannot assign to %s%i; it is already 
in use"

It would be IMHO better to use of_alias_get_highest_id().
In xilinx_uartps I used in past of_alias_get_alias_list() to get 
bitfield of aliases which are taken and use only that one which are free 
but up2you. Having the same behavior across drivers would be good.

Thanks,
Michal

Re: [PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[Ruediger Willenberg]

Am 19.11.2021 um 13:21 schrieb Michal Simek:
> On 11/18/21 22:17, Ruediger Willenberg wrote:
>> Find free uart_port struct in range 0 <= id < ULITE_NR_UARTS when
>> the device tree port-number property is outside that range. This
>> happens when there are other UART types in the system because the
>> Xilinx device tree generator numbers all UARTs consecutively;
>> as a result, not as many Uartlites as specified by the
>> SERIAL_UARTLITE_NR_UARTS parameter could be successfully added.
>>
>> Signed-off-by: Ruediger Willenberg <r.willenberg@hs-mannheim.de>
>> ---
>> Changes in v2:
>>   - give KERN_NOTICE when changing the id,
>>     with reference to the requested port-number
>>
>> -    if (id < 0 || id >= ULITE_NR_UARTS) {
>> -        dev_err(dev, "%s%i too large\n", ULITE_NAME, id);
>> +    if (id == ULITE_NR_UARTS) {
>> +        dev_err(dev, "maximum number of %s assigned\n", ULITE_NAME);
>>           return -EINVAL;
>>       }
>> @@ -676,7 +678,11 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
>>           dev_set_drvdata(dev, NULL);
>>           return rc;
>>       }
> 
> nit: please keep this newline here.
> 
>> -
>> +    if (oor_id >= 0)
>> +        dev_notice(dev,
>> +            "assigned uartlite with device tree port-number=<%i> to %s%i\n",
>> +            oor_id, ULITE_NAME, id);
>> +
> 
> 
> 
> [linux](master)$ ./scripts/checkpatch.pl --strict 
> 0001-serial-uartlite-Move-out-of-range-port-numbers-into-.patch
> CHECK: Alignment should match open parenthesis

Thanks, I apparently didn't do "--strict" checking, sorry. Will fix both in PATCH v3


> And there is one more issue with this. If you start to mix serial IPs which are partially recorded 
> in aliases. For example like this.
> 
> aliases {
>      serial0 = &uart1;
> };
> 
> uart0 {
> ...
> };
> Uart0 is probed first. It is without alias and id 0 will be assigned to it. Then uart1 is probed. It 
> looks at aliases you get id 0 but it is already taken and you end up with "cannot assign to %s%i; it 
> is already in use"

As I point out in the other thread
  "[PATCH v2] uartlite: Update the default for the SERIAL_UARTLITE_NR_UARTS"
(I don't feel crossposting all of it here would be good practice)

a) the Xilinx DTG gives "serial" aliases to _all_ Xilinx UARTs
b) the Xilinx DTG gives "port-number" properties to _all_ Xilinx UARTs
c) these two are not assigned in the same order by the Xilinx DTG, which is problematic

uartlite.c currently looks at port-number properties, not aliases. So unassigned aliases are not the 
issue. A similar-looking but more complicated issue that COULD happen, to be transparent, is this:

* I have 1 ps_uart ("psuart1") and 2 uartlites ("uartlite0" and "uartlite1")
* The second uartlite is configured in MSS to be the OS console_device
* SERIAL_UARTLITE_NR_UARTS = 2
* The current Xilinx DTG could assign like this:
   uartlite1: port-number=<0> (because console)
   psuart1:   port-number=<1>
   uartlite0: port-number=<2>

* uartlite0 gets probed first, gets reassigned line 0, uartlite1 is probed, gets reassigned line 1, 
which means it's not the console

Except MSS seems to list uartps after uartlite, so even that does not happen. So yeah, this 
convoluted scenario could only be fixed by a much bigger change (see your next point below and my 
reply) OR by fixing the Xilinx DTG. "serial" alias order as currently generated by the Xilinx DTG is 
also a bit of a mess, as I explain in the other thread.

> It would be IMHO better to use of_alias_get_highest_id().
> In xilinx_uartps I used in past of_alias_get_alias_list() to get bitfield of aliases which are taken 
> and use only that one which are free but up2you. Having the same behavior across drivers would be good.

Right now it seems xilinx_uartps.c is only using
   id = of_alias_get_id(pdev->dev.of_node, "serial");
and then checking
   if (id >= CDNS_UART_NR_PORTS) {

Which means it's relying on
a) CDNS_UART_NR_PORTS being high enough and
b) the Xilinx DTG giving ps_uarts preferential treatment (i.e. always starting alias enumeration 
from serial0 with ps_uarts first).
I can envision in principle changing uartlite.c to the mechanism you suggest but it is a much bigger 
change, with the potential to break more stuff.

This is currently a minimal fix to the issue of the DTG not being compatible with the proper use of 
SERIAL_UARTLITE_NR_UARTS; it won't even be required anymore if Xilinx accepts my DTG patch. As the 
current DTG does not evenly hold "serial" aliases and "port-numbers" the same, I really don't see 
why they should not.

Thank you,
Ruediger

Re: [PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[Michal Simek]

On 11/22/21 23:08, Ruediger Willenberg wrote:
> Am 19.11.2021 um 13:21 schrieb Michal Simek:
>> On 11/18/21 22:17, Ruediger Willenberg wrote:
>>> Find free uart_port struct in range 0 <= id < ULITE_NR_UARTS when
>>> the device tree port-number property is outside that range. This
>>> happens when there are other UART types in the system because the
>>> Xilinx device tree generator numbers all UARTs consecutively;
>>> as a result, not as many Uartlites as specified by the
>>> SERIAL_UARTLITE_NR_UARTS parameter could be successfully added.
>>>
>>> Signed-off-by: Ruediger Willenberg <r.willenberg@hs-mannheim.de>
>>> ---
>>> Changes in v2:
>>>   - give KERN_NOTICE when changing the id,
>>>     with reference to the requested port-number
>>>
>>> -    if (id < 0 || id >= ULITE_NR_UARTS) {
>>> -        dev_err(dev, "%s%i too large\n", ULITE_NAME, id);
>>> +    if (id == ULITE_NR_UARTS) {
>>> +        dev_err(dev, "maximum number of %s assigned\n", ULITE_NAME);
>>>           return -EINVAL;
>>>       }
>>> @@ -676,7 +678,11 @@ static int ulite_assign(struct device *dev, int 
>>> id, u32 base, int irq,
>>>           dev_set_drvdata(dev, NULL);
>>>           return rc;
>>>       }
>>
>> nit: please keep this newline here.
>>
>>> -
>>> +    if (oor_id >= 0)
>>> +        dev_notice(dev,
>>> +            "assigned uartlite with device tree port-number=<%i> to 
>>> %s%i\n",
>>> +            oor_id, ULITE_NAME, id);
>>> +
>>
>>
>>
>> [linux](master)$ ./scripts/checkpatch.pl --strict 
>> 0001-serial-uartlite-Move-out-of-range-port-numbers-into-.patch
>> CHECK: Alignment should match open parenthesis
> 
> Thanks, I apparently didn't do "--strict" checking, sorry. Will fix both 
> in PATCH v3
> 
> 
>> And there is one more issue with this. If you start to mix serial IPs 
>> which are partially recorded in aliases. For example like this.
>>
>> aliases {
>>      serial0 = &uart1;
>> };
>>
>> uart0 {
>> ...
>> };
>> Uart0 is probed first. It is without alias and id 0 will be assigned 
>> to it. Then uart1 is probed. It looks at aliases you get id 0 but it 
>> is already taken and you end up with "cannot assign to %s%i; it is 
>> already in use"
> 
> As I point out in the other thread
>   "[PATCH v2] uartlite: Update the default for the 
> SERIAL_UARTLITE_NR_UARTS"
> (I don't feel crossposting all of it here would be good practice)
> 
> a) the Xilinx DTG gives "serial" aliases to _all_ Xilinx UARTs
> b) the Xilinx DTG gives "port-number" properties to _all_ Xilinx UARTs
> c) these two are not assigned in the same order by the Xilinx DTG, which 
> is problematic

DTG/MSS logic should be solved in different mailing list that's why 
please move it there.

> 
> uartlite.c currently looks at port-number properties, not aliases. So 
> unassigned aliases are not the issue. A similar-looking but more 
> complicated issue that COULD happen, to be transparent, is this:

port-number is optional and as I said in the second thread I think that 
this should be deprecated and removed. Most of DT drivers take care 
about serial alias that's why we should go that route too.

And I also remember that drivers shouldn't really use different names as 
ttyUL, ttyPS, ttyAMA, etc but all of them should use the same name as 
ttyUSB is doing.

Thanks,
Michal

Re: [PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[Rüdiger Willenberg]

Am 23.11.2021 um 13:19 schrieb Michal Simek:
> 
> 
> On 11/22/21 23:08, Ruediger Willenberg wrote:
>> Am 19.11.2021 um 13:21 schrieb Michal Simek:
>>> On 11/18/21 22:17, Ruediger Willenberg wrote:
>>>> Find free uart_port struct in range 0 <= id < ULITE_NR_UARTS when
>>>> the device tree port-number property is outside that range. This
>>>> happens when there are other UART types in the system because the
>>>> Xilinx device tree generator numbers all UARTs consecutively;
>>>> as a result, not as many Uartlites as specified by the
>>>> SERIAL_UARTLITE_NR_UARTS parameter could be successfully added.
>>>>
>>>> Signed-off-by: Ruediger Willenberg <r.willenberg@hs-mannheim.de>
>>>> ---
>>>> Changes in v2:
>>>>   - give KERN_NOTICE when changing the id,
>>>>     with reference to the requested port-number
>>>>
>>>> -    if (id < 0 || id >= ULITE_NR_UARTS) {
>>>> -        dev_err(dev, "%s%i too large\n", ULITE_NAME, id);
>>>> +    if (id == ULITE_NR_UARTS) {
>>>> +        dev_err(dev, "maximum number of %s assigned\n", ULITE_NAME);
>>>>           return -EINVAL;
>>>>       }
>>>> @@ -676,7 +678,11 @@ static int ulite_assign(struct device *dev, int 
>>>> id, u32 base, int irq,
>>>>           dev_set_drvdata(dev, NULL);
>>>>           return rc;
>>>>       }
>>>
>>> nit: please keep this newline here.
>>>
>>>> -
>>>> +    if (oor_id >= 0)
>>>> +        dev_notice(dev,
>>>> +            "assigned uartlite with device tree port-number=<%i> to 
>>>> %s%i\n",
>>>> +            oor_id, ULITE_NAME, id);
>>>> +
>>>
>>>
>>>
>>> [linux](master)$ ./scripts/checkpatch.pl --strict 
>>> 0001-serial-uartlite-Move-out-of-range-port-numbers-into-.patch
>>> CHECK: Alignment should match open parenthesis
>>
>> Thanks, I apparently didn't do "--strict" checking, sorry. Will fix 
>> both in PATCH v3
>>
>>
>>> And there is one more issue with this. If you start to mix serial IPs 
>>> which are partially recorded in aliases. For example like this.
>>>
>>> aliases {
>>>      serial0 = &uart1;
>>> };
>>>
>>> uart0 {
>>> ...
>>> };
>>> Uart0 is probed first. It is without alias and id 0 will be assigned 
>>> to it. Then uart1 is probed. It looks at aliases you get id 0 but it 
>>> is already taken and you end up with "cannot assign to %s%i; it is 
>>> already in use"
>>
>> As I point out in the other thread
>>   "[PATCH v2] uartlite: Update the default for the 
>> SERIAL_UARTLITE_NR_UARTS"
>> (I don't feel crossposting all of it here would be good practice)
>>
>> a) the Xilinx DTG gives "serial" aliases to _all_ Xilinx UARTs
>> b) the Xilinx DTG gives "port-number" properties to _all_ Xilinx UARTs
>> c) these two are not assigned in the same order by the Xilinx DTG, 
>> which is problematic
> 
> DTG/MSS logic should be solved in different mailing list that's why 
> please move it there.

I agree in principle. Michal, since you have an @xilinx.com domain could 
you please share where that place to discuss is?

The Linux "devicetree" list doesn't not seem the right place either for 
a company-managed, separate repo. If this goes your suggested way 
(completely changing the uartlite driver to use the "serial" alias, the 
Xilinx DTG _needs_ to be improved, including respecting the serial0 
alias for the designated console. I'd be glad to contribute to the 
discussion there. Right not there's a mess that's not solved by throwing 
out the port-number property alone.

>>
>> uartlite.c currently looks at port-number properties, not aliases. So 
>> unassigned aliases are not the issue. A similar-looking but more 
>> complicated issue that COULD happen, to be transparent, is this:
> 
> port-number is optional and as I said in the second thread I think that 
> this should be deprecated and removed. Most of DT drivers take care 
> about serial alias that's why we should go that route too.
> 
> And I also remember that drivers shouldn't really use different names as 
> ttyUL, ttyPS, ttyAMA, etc but all of them should use the same name as 
> ttyUSB is doing.

Big improvements are good and well, but they tend to break things and 
again it does mean the Xilinx DTG needs to move with that.
I'd be immensely grateful if you could indicate _where_ to discuss those 
changes, and who has the authority to speak for Xilinx. Off-list, if you 
think that is more appropriate.

Thank you,
Ruediger

Re: [PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[Michal Simek]

On 11/23/21 15:59, Rüdiger Willenberg wrote:
> Am 23.11.2021 um 13:19 schrieb Michal Simek:
>>
>>
>> On 11/22/21 23:08, Ruediger Willenberg wrote:
>>> Am 19.11.2021 um 13:21 schrieb Michal Simek:
>>>> On 11/18/21 22:17, Ruediger Willenberg wrote:
>>>>> Find free uart_port struct in range 0 <= id < ULITE_NR_UARTS when
>>>>> the device tree port-number property is outside that range. This
>>>>> happens when there are other UART types in the system because the
>>>>> Xilinx device tree generator numbers all UARTs consecutively;
>>>>> as a result, not as many Uartlites as specified by the
>>>>> SERIAL_UARTLITE_NR_UARTS parameter could be successfully added.
>>>>>
>>>>> Signed-off-by: Ruediger Willenberg <r.willenberg@hs-mannheim.de>
>>>>> ---
>>>>> Changes in v2:
>>>>>   - give KERN_NOTICE when changing the id,
>>>>>     with reference to the requested port-number
>>>>>
>>>>> -    if (id < 0 || id >= ULITE_NR_UARTS) {
>>>>> -        dev_err(dev, "%s%i too large\n", ULITE_NAME, id);
>>>>> +    if (id == ULITE_NR_UARTS) {
>>>>> +        dev_err(dev, "maximum number of %s assigned\n", ULITE_NAME);
>>>>>           return -EINVAL;
>>>>>       }
>>>>> @@ -676,7 +678,11 @@ static int ulite_assign(struct device *dev, 
>>>>> int id, u32 base, int irq,
>>>>>           dev_set_drvdata(dev, NULL);
>>>>>           return rc;
>>>>>       }
>>>>
>>>> nit: please keep this newline here.
>>>>
>>>>> -
>>>>> +    if (oor_id >= 0)
>>>>> +        dev_notice(dev,
>>>>> +            "assigned uartlite with device tree port-number=<%i> 
>>>>> to %s%i\n",
>>>>> +            oor_id, ULITE_NAME, id);
>>>>> +
>>>>
>>>>
>>>>
>>>> [linux](master)$ ./scripts/checkpatch.pl --strict 
>>>> 0001-serial-uartlite-Move-out-of-range-port-numbers-into-.patch
>>>> CHECK: Alignment should match open parenthesis
>>>
>>> Thanks, I apparently didn't do "--strict" checking, sorry. Will fix 
>>> both in PATCH v3
>>>
>>>
>>>> And there is one more issue with this. If you start to mix serial 
>>>> IPs which are partially recorded in aliases. For example like this.
>>>>
>>>> aliases {
>>>>      serial0 = &uart1;
>>>> };
>>>>
>>>> uart0 {
>>>> ...
>>>> };
>>>> Uart0 is probed first. It is without alias and id 0 will be assigned 
>>>> to it. Then uart1 is probed. It looks at aliases you get id 0 but it 
>>>> is already taken and you end up with "cannot assign to %s%i; it is 
>>>> already in use"
>>>
>>> As I point out in the other thread
>>>   "[PATCH v2] uartlite: Update the default for the 
>>> SERIAL_UARTLITE_NR_UARTS"
>>> (I don't feel crossposting all of it here would be good practice)
>>>
>>> a) the Xilinx DTG gives "serial" aliases to _all_ Xilinx UARTs
>>> b) the Xilinx DTG gives "port-number" properties to _all_ Xilinx UARTs
>>> c) these two are not assigned in the same order by the Xilinx DTG, 
>>> which is problematic
>>
>> DTG/MSS logic should be solved in different mailing list that's why 
>> please move it there.
> 
> I agree in principle. Michal, since you have an @xilinx.com domain could 
> you please share where that place to discuss is?
> 
> The Linux "devicetree" list doesn't not seem the right place either for 
> a company-managed, separate repo. If this goes your suggested way 
> (completely changing the uartlite driver to use the "serial" alias, the 
> Xilinx DTG _needs_ to be improved, including respecting the serial0 
> alias for the designated console. I'd be glad to contribute to the 
> discussion there. Right not there's a mess that's not solved by throwing 
> out the port-number property alone.
> 
>>>
>>> uartlite.c currently looks at port-number properties, not aliases. So 
>>> unassigned aliases are not the issue. A similar-looking but more 
>>> complicated issue that COULD happen, to be transparent, is this:
>>
>> port-number is optional and as I said in the second thread I think 
>> that this should be deprecated and removed. Most of DT drivers take 
>> care about serial alias that's why we should go that route too.
>>
>> And I also remember that drivers shouldn't really use different names 
>> as ttyUL, ttyPS, ttyAMA, etc but all of them should use the same name 
>> as ttyUSB is doing.
> 
> Big improvements are good and well, but they tend to break things and 
> again it does mean the Xilinx DTG needs to move with that.
> I'd be immensely grateful if you could indicate _where_ to discuss those 
> changes, and who has the authority to speak for Xilinx. Off-list, if you 
> think that is more appropriate.

Let's just figure it out how it should be done properly and then there 
is no problem to update DTG based on it.

Thanks,
Michal

Re: [PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[Maarten Brock]

On 2021-11-23 13:19, Michal Simek wrote:
> On 11/22/21 23:08, Ruediger Willenberg wrote:
>> Am 19.11.2021 um 13:21 schrieb Michal Simek:
>>> On 11/18/21 22:17, Ruediger Willenberg wrote:
>>>> Find free uart_port struct in range 0 <= id < ULITE_NR_UARTS when
>>>> the device tree port-number property is outside that range. This
>>>> happens when there are other UART types in the system because the
>>>> Xilinx device tree generator numbers all UARTs consecutively;
>>>> as a result, not as many Uartlites as specified by the
>>>> SERIAL_UARTLITE_NR_UARTS parameter could be successfully added.
>>>> 
>>>> Signed-off-by: Ruediger Willenberg <r.willenberg@hs-mannheim.de>
>>>> ---
>>>> Changes in v2:
>>>>   - give KERN_NOTICE when changing the id,
>>>>     with reference to the requested port-number
>>>> 
>>>> -    if (id < 0 || id >= ULITE_NR_UARTS) {
>>>> -        dev_err(dev, "%s%i too large\n", ULITE_NAME, id);
>>>> +    if (id == ULITE_NR_UARTS) {
>>>> +        dev_err(dev, "maximum number of %s assigned\n", 
>>>> ULITE_NAME);
>>>>           return -EINVAL;
>>>>       }
>>>> @@ -676,7 +678,11 @@ static int ulite_assign(struct device *dev, int 
>>>> id, u32 base, int irq,
>>>>           dev_set_drvdata(dev, NULL);
>>>>           return rc;
>>>>       }
>>> 
>>> nit: please keep this newline here.
>>> 
>>>> -
>>>> +    if (oor_id >= 0)
>>>> +        dev_notice(dev,
>>>> +            "assigned uartlite with device tree port-number=<%i> to 
>>>> %s%i\n",
>>>> +            oor_id, ULITE_NAME, id);
>>>> +
>>> 
>>> 
>>> 
>>> [linux](master)$ ./scripts/checkpatch.pl --strict 
>>> 0001-serial-uartlite-Move-out-of-range-port-numbers-into-.patch
>>> CHECK: Alignment should match open parenthesis
>> 
>> Thanks, I apparently didn't do "--strict" checking, sorry. Will fix 
>> both in PATCH v3
>> 
>> 
>>> And there is one more issue with this. If you start to mix serial IPs 
>>> which are partially recorded in aliases. For example like this.
>>> 
>>> aliases {
>>>      serial0 = &uart1;
>>> };
>>> 
>>> uart0 {
>>> ...
>>> };
>>> Uart0 is probed first. It is without alias and id 0 will be assigned 
>>> to it. Then uart1 is probed. It looks at aliases you get id 0 but it 
>>> is already taken and you end up with "cannot assign to %s%i; it is 
>>> already in use"
>> 
>> As I point out in the other thread
>>   "[PATCH v2] uartlite: Update the default for the 
>> SERIAL_UARTLITE_NR_UARTS"
>> (I don't feel crossposting all of it here would be good practice)
>> 
>> a) the Xilinx DTG gives "serial" aliases to _all_ Xilinx UARTs
>> b) the Xilinx DTG gives "port-number" properties to _all_ Xilinx UARTs
>> c) these two are not assigned in the same order by the Xilinx DTG, 
>> which is problematic
> 
> DTG/MSS logic should be solved in different mailing list that's why
> please move it there.

What mailing list? I'd like to be in on this.

>> 
>> uartlite.c currently looks at port-number properties, not aliases. So 
>> unassigned aliases are not the issue. A similar-looking but more 
>> complicated issue that COULD happen, to be transparent, is this:
> 
> port-number is optional and as I said in the second thread I think
> that this should be deprecated and removed. Most of DT drivers take
> care about serial alias that's why we should go that route too.
> 
> And I also remember that drivers shouldn't really use different names
> as ttyUL, ttyPS, ttyAMA, etc but all of them should use the same name
> as ttyUSB is doing.
> 
> Thanks,
> Michal

The port-number is optional, but currently automatically generated. The 
same goes
for the aliases. How can one remove them from the device-tree? Is 
overwriting the
only option? If so, what value would you recommend?

If, and only if, devices would start to use the same name would the 
serial alias
become a decent solution. But I'm sure that the 8250 driver currently 
does not
supports e.g. ttyS25 when SERIAL_8250_NR_UARTS is configured for only 4 
devices.

As a workaround I always patch the xilinx_uartps.c driver to also 
support
port-number. This way I can overwrite the port-number values in the 
device-tree
to get them numbered ttyPS0 and ttyPS1. And then I can use the serial 
alias for
my 16550's.

&uart0 {
         port-number = <0>;                      // this is a 
xilinx_uartps
};

&uart1 {
         port-number = <1>;                      // this is a 
xilinx_uartps
};

&liteuarts_axi_uartlite_0 {
         port-number = <0>;                      // this is a uartlite
};

&liteuarts_axi_uartlite_1 {
         port-number = <1>;                      // this is a uartlite
};
[...]
&liteuarts_axi_uartlite_10 {
         port-number = <10>;                     // this is a uartlite
};

The 16550's also received a port-number but the driver ignores that.

aliases {
         serial0 = &uart0;
         serial1 = &fulluarts_axi_uart16550_1;
         serial2 = &fulluarts_axi_uart16550_2;
         serial3 = &fulluarts_axi_uart16550_3;
         serial4 = &fulluarts_axi_uart16550_4;
         serial5 = &fulluarts_axi_uart16550_5;
         serial6 = &fulluarts_axi_uart16550_6;
         serial7 = &fulluarts_axi_uart16550_7;
         serial8 = &fulluarts_axi_uart16550_0;
};

Note how I had to abuse serial8 to get the 16550_0 at ttyS0 with
SERIAL_8250_NR_UARTS=8. This does leave me with several dangling serial 
aliases
in the final device tree.

The fact that I have to use this workaround to me means that the serial 
naming
and numbering system is broken.

If you ask me, the solution is not to start using the serial alias, but 
abandon it
and go for port-number entirely, at least for uartlite and 
xilinx_uartps. But
eventually also for the 8250 driver and all others.

Btw. the port-number and the MSS also don't sort natural, but purely 
alphabetical.
axi_uartlite_10 comes before axi_uartlite_2. Maybe I should have renamed 
them
with a leading zero myself.

Maarten

Re: [PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[Michal Simek]

On 11/26/21 16:35, Maarten Brock wrote:
> On 2021-11-23 13:19, Michal Simek wrote:
>> On 11/22/21 23:08, Ruediger Willenberg wrote:
>>> Am 19.11.2021 um 13:21 schrieb Michal Simek:
>>>> On 11/18/21 22:17, Ruediger Willenberg wrote:
>>>>> Find free uart_port struct in range 0 <= id < ULITE_NR_UARTS when
>>>>> the device tree port-number property is outside that range. This
>>>>> happens when there are other UART types in the system because the
>>>>> Xilinx device tree generator numbers all UARTs consecutively;
>>>>> as a result, not as many Uartlites as specified by the
>>>>> SERIAL_UARTLITE_NR_UARTS parameter could be successfully added.
>>>>>
>>>>> Signed-off-by: Ruediger Willenberg <r.willenberg@hs-mannheim.de>
>>>>> ---
>>>>> Changes in v2:
>>>>>   - give KERN_NOTICE when changing the id,
>>>>>     with reference to the requested port-number
>>>>>
>>>>> -    if (id < 0 || id >= ULITE_NR_UARTS) {
>>>>> -        dev_err(dev, "%s%i too large\n", ULITE_NAME, id);
>>>>> +    if (id == ULITE_NR_UARTS) {
>>>>> +        dev_err(dev, "maximum number of %s assigned\n", ULITE_NAME);
>>>>>           return -EINVAL;
>>>>>       }
>>>>> @@ -676,7 +678,11 @@ static int ulite_assign(struct device *dev, int id, 
>>>>> u32 base, int irq,
>>>>>           dev_set_drvdata(dev, NULL);
>>>>>           return rc;
>>>>>       }
>>>>
>>>> nit: please keep this newline here.
>>>>
>>>>> -
>>>>> +    if (oor_id >= 0)
>>>>> +        dev_notice(dev,
>>>>> +            "assigned uartlite with device tree port-number=<%i> to %s%i\n",
>>>>> +            oor_id, ULITE_NAME, id);
>>>>> +
>>>>
>>>>
>>>>
>>>> [linux](master)$ ./scripts/checkpatch.pl --strict 
>>>> 0001-serial-uartlite-Move-out-of-range-port-numbers-into-.patch
>>>> CHECK: Alignment should match open parenthesis
>>>
>>> Thanks, I apparently didn't do "--strict" checking, sorry. Will fix both in 
>>> PATCH v3
>>>
>>>
>>>> And there is one more issue with this. If you start to mix serial IPs which 
>>>> are partially recorded in aliases. For example like this.
>>>>
>>>> aliases {
>>>>      serial0 = &uart1;
>>>> };
>>>>
>>>> uart0 {
>>>> ...
>>>> };
>>>> Uart0 is probed first. It is without alias and id 0 will be assigned to it. 
>>>> Then uart1 is probed. It looks at aliases you get id 0 but it is already 
>>>> taken and you end up with "cannot assign to %s%i; it is already in use"
>>>
>>> As I point out in the other thread
>>>   "[PATCH v2] uartlite: Update the default for the SERIAL_UARTLITE_NR_UARTS"
>>> (I don't feel crossposting all of it here would be good practice)
>>>
>>> a) the Xilinx DTG gives "serial" aliases to _all_ Xilinx UARTs
>>> b) the Xilinx DTG gives "port-number" properties to _all_ Xilinx UARTs
>>> c) these two are not assigned in the same order by the Xilinx DTG, which is 
>>> problematic
>>
>> DTG/MSS logic should be solved in different mailing list that's why
>> please move it there.
> 
> What mailing list? I'd like to be in on this.

xilinx forums.

> 
>>>
>>> uartlite.c currently looks at port-number properties, not aliases. So 
>>> unassigned aliases are not the issue. A similar-looking but more complicated 
>>> issue that COULD happen, to be transparent, is this:
>>
>> port-number is optional and as I said in the second thread I think
>> that this should be deprecated and removed. Most of DT drivers take
>> care about serial alias that's why we should go that route too.
>>
>> And I also remember that drivers shouldn't really use different names
>> as ttyUL, ttyPS, ttyAMA, etc but all of them should use the same name
>> as ttyUSB is doing.
>>
>> Thanks,
>> Michal
> 
> The port-number is optional, but currently automatically generated. The same goes
> for the aliases. How can one remove them from the device-tree? Is overwriting the
> only option? If so, what value would you recommend?

As was said DTG does the best to help you to describe system. You should take 
output of DTG and fix your design by hand to match your requirements.

> 
> If, and only if, devices would start to use the same name would the serial alias
> become a decent solution. But I'm sure that the 8250 driver currently does not
> supports e.g. ttyS25 when SERIAL_8250_NR_UARTS is configured for only 4 devices.
> 
> As a workaround I always patch the xilinx_uartps.c driver to also support
> port-number. This way I can overwrite the port-number values in the device-tree
> to get them numbered ttyPS0 and ttyPS1. And then I can use the serial alias for
> my 16550's.
> 
> &uart0 {
>          port-number = <0>;                      // this is a xilinx_uartps
> };
> 
> &uart1 {
>          port-number = <1>;                      // this is a xilinx_uartps
> };
> 
> &liteuarts_axi_uartlite_0 {
>          port-number = <0>;                      // this is a uartlite
> };
> 
> &liteuarts_axi_uartlite_1 {
>          port-number = <1>;                      // this is a uartlite
> };
> [...]
> &liteuarts_axi_uartlite_10 {
>          port-number = <10>;                     // this is a uartlite
> };
> 
> The 16550's also received a port-number but the driver ignores that.
> 
> aliases {
>          serial0 = &uart0;
>          serial1 = &fulluarts_axi_uart16550_1;
>          serial2 = &fulluarts_axi_uart16550_2;
>          serial3 = &fulluarts_axi_uart16550_3;
>          serial4 = &fulluarts_axi_uart16550_4;
>          serial5 = &fulluarts_axi_uart16550_5;
>          serial6 = &fulluarts_axi_uart16550_6;
>          serial7 = &fulluarts_axi_uart16550_7;
>          serial8 = &fulluarts_axi_uart16550_0;
> };
> 
> Note how I had to abuse serial8 to get the 16550_0 at ttyS0 with
> SERIAL_8250_NR_UARTS=8. This does leave me with several dangling serial aliases
> in the final device tree.
> 
> The fact that I have to use this workaround to me means that the serial naming
> and numbering system is broken.

I am not the right person to judge this. IIRC last time we discussed about it it 
was said that someone should take a look at USB tty behavior where this is solved.

> If you ask me, the solution is not to start using the serial alias, but abandon it
> and go for port-number entirely, at least for uartlite and xilinx_uartps. But
> eventually also for the 8250 driver and all others.
> 
> Btw. the port-number and the MSS also don't sort natural, but purely alphabetical.
> axi_uartlite_10 comes before axi_uartlite_2. Maybe I should have renamed them
> with a leading zero myself.

I choose the way that I am normally using serial aliases and don't using port 
numbers and systems have ttyPS0, PS1, S2, S3, UL4, UL5 etc. And I have stable 
number and not all ttyX starts from 0. Which is the same solution which Xilinx 
is using because I think I have introduced this behavior in DTG (or at least 
reviewed it).

Thanks,
Michal