Re: [PATCH v2] uartlite: Update the default for the SERIAL_UARTLITE_NR_UARTS

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 19.11.2021 um 11:21 schrieb Michal Simek:


On 11/18/21 22:21, Ruediger Willenberg wrote:
Am 18.11.2021 um 09:45 schrieb Michal Simek:
On 11/17/21 10:53, Ruediger Willenberg wrote:
Am 17.11.2021 um 06:16 schrieb Shubhrajyoti Datta:
"The uartlite is used by FPGAs that support a basically unlimited number
of uarts so limiting it at 16 dosn't make sense as users might need more
than that."

So change the default number of uarts back to 16.

The DTG should number devices for each driver separately from 0;
serial_core.c checks for (0 <= uart_port->line < NR_UART_PORTS). As a
consequence, when a Zynq system has both a PS-UART and a Uartlite,
setting SERIAL_UARTLITE_NR_UARTS explicitly to 1 in Kconfig means
probing the uartlite fails, which is confusing to the unsuspecting
KConfig user. Setting the default to 16 just kicks the can down the
road because it will fail for more than 15 Uartlites (or less, if there
are more PS-UARTs or AXI 16550A UARTs).

I have no problem with your patch and it is correct (I would prefer a message to show for this option). But it doesn't mean that default 1 was correct value to setup especially when range was setup by Sam from 1-256. And originally number 16 was used.

That's why I think this patch is still valid. If you don't like description it can be changed because definitely people are using systems with more then one uartlite and 1 is not reasonable default.
We can also take a look at default for others serial drivers
and none of them is using 1 as default. IIRC discussion about UARTPS it was said if number is not


Clarification: I support changing the default up to 16. It will avoid problems for most cases where
a) there are only a few Uartlites in the system
AND
b) the Kconfig user doesn't explictly specify SERIAL_UARTLITE_NR_UARTS

I just wanted to point out that trouble ensues when
a) somebody wants 16 Uartlites in a system when there is also a PS UART (basically any Zynq)
OR
b) somebody explicitly sets the SERIAL_UARTLITE_NR_UARTS to the number she built into a Zynq system
(when the Uartlite driver is activated in menuconfig, the NR parameters pops right up, so it's mighty tempting to actually set it to the accurate number)

Moving the default up should just not be a band-aid for the cases where a single Uartlite can not be assigned, when really there is a fundamental inconsistency between how serial_core.c has limited line numbering since Kernel 2.6 (0 <= line_id < NR_UARTs) and what Xilinx DTG does with UARTs.


 > And using aliases for serial id identification is around for a while and Xilinx DTG is aligned with  > it because via current dt you can't describe cases where you will have stable numbers for different
 > tty names like
 > ttyS0, ttyUL0, ttyPS0, ttyAMA0, etc.
 > That's why DTG generates it per all serial IDs.

Sincere apologies, I do not quite understand this explanation/argument. Would you be so kind to rephrase?

(As I'm a newbie to this list, I'm not sure how much back-and-forth is appropriate about a topic. Since my issue here is about the Xilinx DTG, not Linux kernel code per se, I'd also be fine to continue emailing off-list. There does not seem to be a similar list for device-tree-xlnx)

A lot of drivers are calling of_alias_get_id() which returns you ID from aliases list. If you don't use it or ID is more then your amount of allocates ports you are choosing different logic how to find proper number.

In for example arch/arm/boot/dts/at91-kizbox3_common.dtsi
you can see

  24         aliases {
  25                 serial0 = &uart0;
  26                 serial1 = &uart1;
  27                 serial2 = &uart2;
  28                 serial3 = &uart3;
  29                 serial4 = &uart4;
  30                 serial5 = &uart5;
  31                 serial6 = &uart8;
  32         };

It means uart0 should become ttyX0 uart1 should become ttyX1, etc
If you have all uarts the same type then everything is fine.

But on systems like FPGAs you have different types of uart and via DT you can't really describe things like this
     aliases {
             serial0 = &ps_uart; // to become ttyPS0
             serial0 = &uartlite; // to become ttyUL0
             serial0 = &uart16550; // to become ttyS0
             serial0 = &uartAMA; // to become ttyAMA0
         ...
     }

This is not allowed in DT. You can have only one alias with the same ID.
And if you don't use aliases order depends on probe order which doesn't need to be stable.
If you have only one instance of IP then you will get just id 0 but if you have multiple instances of the same IP type then order is unpredictable. And you really don't want to assigned console to data uart channel and vice-versa or mix channels, etc.

That's why Xilinx DTG assigns number to every IP to be stable and predictable. A lot of people think that what you get from DTG doesn't need to be changed but that's not intention of DTG at all. DTG just does the best to describe the system. It is up to user to do whatever changes are required for their system. If you don't like it feel free to change/fix it.

And in past port-number property was in spec but it not supported by all drivers.
I think we have also generated it for 16550 xilinx uart in past.

Thanks,
Michal

Thank you Michal for kindly taking the time to explain this in detail, that was very helpful.

I've now looked at this more closely and experimented with different combinations of ps_uart, AXI Uartlite, mdm_uart and AXI UART16550A. Relevant observations (Michal you'll know most of this):

1. The Xilinx DTG adds the "port-number" property to all Zynq/Zynq-US Cadence UARTs, AXI Uartlite, mdm_uart and AXI UART16550A (it's only documented so far in the DT-binding file for uartlite)

2. The current "port-number" mechanism in the Xilinx DTG enumerates in the order in which they are listed in the MSS file generated by proprietary Xilinx binaries, be it Vitis/XSCT or petalinux-config. The only exception is when the MSS file declares a UART the OS serial_console, then that one gets 0 and everyone else follows (this is required because the DTG also always sets the kernel bootarg for console to ttyPS_0_ , ttyUL_0_ , etc.)

3. For one type of driver (uartps, uartns, uartlite), the Xilinx binaries list the UARTs in the MSS file strictly alphabetically by HW_INSTANCE name (that's even true if I mix AXI Uartlites and mdm_uarts, which both use the uartlite driver). So the port-numbers follow strictly (with the console exception) and reproducibly.

4. The Xilinx DTG gives "serial" aliases to _all_ said UARTs, however the order is different than the port-numbers/the MSS order:

4.1. ps_uarts (and I suppose other cadence uarts) come first; if ps_uart1 is declared console in the MSS, it gets "serial0", even if there is ps_uart0, which then gets "serial1".

4.2. other UARTs get higher serial aliases, however the order can mix UARTs of different drivers, like Uartlite and AXI16550. This order is not based on MSS, memory addresses, instance names or even order added or interconnect ports in the Vivado Block Design. It is intransparent to me even after looking at devicetree.tcl and seems to change with different combinations of PL UARTs

4.3. even if I declare a Uartlite as the boot console for both u-boot and the Linux kernel in the PetaLinux menuconfig, "serial0" still goes to the ps_uart (that uartlite becomes the first other serial*, though). This is unfortunate because the early bootarg is "serial0:115200n8", so all the early kernel output still goes out over the ps_uart against the configuration.


Lastly I would like to observe that the patch I sent in to git@xxxxxxxxxx for the Xilinx DTG
a) preserves the port-numbering in MSS order for each driver type (uartps, uartns, uartlite)
b) preserves the special port-number=<0> treatment for the UART declared the OS console
but
c) enumerates up consecutively from 0 per driver (uartps,uartns,uartlite), therefore fixing the allocation issue brought into focus by the introduction of the SERIAL_UARTLITE_NR_UARTS parameter to the kernel. The order is reliable and reproducible because it is dependent on the MSS structure. The same is not true for the "serial" alias.

Michal, as you have kindly explained above, there cannot be serial aliases with the same number, and in combination with the unpredictable numbering of non-cadence uarts, the "serial" alias mechanism is unsuitable right now to properly deal with multiple UART types which will pop up in FPGA systems.


I therefore respectfully urge Xilinx to adopt my DTG patch. It does not break anything and keeps numbering as reliable as it is right now, but it fixes the inconsistency with kernel numbering policy.


Since Xilinx tool and Linux kernel development are not synced-up, changes on either side should not break the other; the DTG patch does not break the Linux side. The /tty/serial/uartlite.c patch that we are discussing in the separate thread
 "[PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range"
is a fix for what the DTG does right now, but does not break with either version.

Best wishes and thanks for your time,
Ruediger



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux