xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts
Subsystems (2)
PATCH 1/1 xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts
The number of xilinx ps uart should be set by a kernel parameter instead of using a #define. This allows the user to set the number of xilinx ps uart using only kconfig and not modifying kernel source.
The ps uart is used in Xilnx Zynq chips usually in quantities maxing at two, but there may be other chips that use more in the future or that I don't know about.
--- drivers/tty/serial/Kconfig | 9 +++++++++ drivers/tty/serial/xilinx_uartps.c | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-)
drivers/tty/serial/Kconfig +9 -0
@@ -1461,6 +1461,15 @@ config SERIAL_XILINX_PS_UART_CONSOLE help Enable a Cadence UART port to be the system console. +config SERIAL_XILINX_PS_UART_NR_UARTS + int "Maximum number of Cadence UART ports" + depends on CONFIG_SERIAL_XILINX_NR_UARTS + range 1 64 + default 2 + help + Set this to the number of Cadence UARTS in your system, or the number + you think you might implement. + config SERIAL_AR933X tristate "AR933X serial port support" depends on HAVE_CLK && SOC_AR933X
drivers/tty/serial/xilinx_uartps.c +1 -1
@@ -36,7 +36,7 @@#define CDNS_UART_NAME "xuartps"#define CDNS_UART_MAJOR 0/* use dynamic node allocation */ #define CDNS_UART_MINOR 0/* works best with devtmpfs */ -#define CDNS_UART_NR_PORTS 2 +#define CDNS_UART_NR_PORTS CONFIG_SERIAL_XILINX_NR_UARTS#define CDNS_UART_FIFO_SIZE 64/* FIFO size */ #define CDNS_UART_REGISTER_SPACE 0x1000
Signature · 1 line
2.11.0
Quoted 7 lines · On Fri, 19 May 2017 20:21:54 -0600 Sam Povilus <[email protected]>…
The number of xilinx ps uart should be set by a kernel parameter instead of using a #define. This allows the user to set the number of xilinx ps uart using only kconfig and not modifying kernel source.
The ps uart is used in Xilnx Zynq chips usually in quantities maxing at two, but there may be other chips that use more in the future or that I don't know about.
If it maxes at two then just set it to two. If in in future it maxes at lots then when it's going to happen change the code to do dynamic allocation and test it versus actual hardware.
Otherwise you break some of the basic ideas of having one kernel for many systems.
Alan
Hi Alan,
Quoted 11 lines · Alan Cox
Quoted 7 lines · On Fri, 19 May 2017 20:21:54 -0600 Sam Povilus <[email protected]>…
The number of xilinx ps uart should be set by a kernel parameter instead of using a #define. This allows the user to set the number of xilinx ps uart using only kconfig and not modifying kernel source.
The ps uart is used in Xilnx Zynq chips usually in quantities maxing at two, but there may be other chips that use more in the future or that I don't know about.
Quoted 6 lines
If it maxes at two then just set it to two. If in in future it maxes at lots then when it's going to happen change the code to do dynamic allocation and test it versus actual hardware.
Otherwise you break some of the basic ideas of having one kernel for many systems.
We have in soc vendor tree similar patch but the reason is different.
tty: serial: Added a CONFIG_SERIAL_XILINX_NR_UARTS option.
This patch Adds CONFIG_SERIAL_XILINX_NR_UARTS option to allow the user to provide the Max number of uart ports information. If multiple cards (or) PL UARTS are present, the default limit of 2 ports should be increased.
I haven't checked all drivers but in our case we have added this as quick fix for scenarios where you use serial aliases where alias is pointed to serial2 or more.
In cdns_uart_init() cdns_uart_uart_driver is passed which contains .nr which is required to be passed.
What's the best driver to look at dynamic allocation?
Thanks,
Michal
Quoted 9 lines
We have in soc vendor tree similar patch but the reason is different.
tty: serial: Added a
CONFIG_SERIAL_XILINX_NR_UARTSoption.This patch Adds
CONFIG_SERIAL_XILINX_NR_UARTSoption to allow the user to provide the Max number of uart ports information. If multiple cards (or) PL UARTS are present, the default limit of 2 ports should be increased.
Quoted 7 lines
I haven't checked all drivers but in our case we have added this as quick fix for scenarios where you use serial aliases where alias is pointed to serial2 or more.
Incdns_uart_init()cdns_uart_uart_driveris passed which contains .nr which is required to be passed.What's the best driver to look at dynamic allocation?
So there are quite a few that dynamically allocate the objects as they are enumerated (eg max3100), but have a maximum set that is just pointers (so for the max number of ports cheaper than the dynamic code)
The other question is why is it a CONFIG_ option. I'm assuming these platforms are all ARM and in that case you could just pass the value in the device tree, or hard code a safe maximum number of pointers to a value which is the worst case and then install them as they are enumerated.
There are lots of options better than breaking the "one kernel many platforms" model.
Alan
Quoted 15 lines · Alan Cox
Quoted 15 lines
We have in soc vendor tree similar patch but the reason is different.
tty: serial: Added a
CONFIG_SERIAL_XILINX_NR_UARTSoption.This patch Adds
CONFIG_SERIAL_XILINX_NR_UARTSoption to allow the user to provide the Max number of uart ports information. If multiple cards (or) PL UARTS are present, the default limit of 2 ports should be increased.I haven't checked all drivers but in our case we have added this as quick fix for scenarios where you use serial aliases where alias is pointed to serial2 or more.
Incdns_uart_init()cdns_uart_uart_driveris passed which contains .nr which is required to be passed.
Quoted 5 lines
Quoted 1 line
What's the best driver to look at dynamic allocation?
So there are quite a few that dynamically allocate the objects as they are enumerated (eg max3100), but have a maximum set that is just pointers (so for the max number of ports cheaper than the dynamic code)
yep hardcoded max 4 where in probe first free space is found and used (range 0-3) but still max3100s statically allocated. Shouldn't be this also dynamically allocated?
Quoted 5 lines
The other question is why is it a
CONFIG_option. I'm assuming these platforms are all ARM and in that case you could just pass the value in the device tree, or hard code a safe maximum number of pointers to a value which is the worst case and then install them as they are enumerated.
I am not quite sure how exactly you want to do this via DT.
Also what do you think is a safe maximum number? This is fpga - hundreds of pins which can do just uart.
Quoted 2 lines
There are lots of options better than breaking the "one kernel many platforms" model.
Another options is also module parameter and dynamically allocated array in cdns_uart_init.
Thanks,
Michal
Quoted 3 lines
yep hardcoded max 4 where in probe first free space is found and used (range 0-3) but still max3100s statically allocated. Shouldn't be this also dynamically allocated?
The code to do the dynamic allocation would be larger than the array of pointers for the sane worst case.
Quoted 1 line
I am not quite sure how exactly you want to do this via DT.
Count the number of DT entries for this kind of port and allocate that many ?
Quoted 9 lines
Also what do you think is a safe maximum number? This is fpga - hundreds of pins which can do just uart.
Quoted 2 lines
There are lots of options better than breaking the "one kernel many platforms" model.
Another options is also module parameter and dynamically allocated array in
cdns_uart_init.
For a given platform the number is constant and they need to be described, so it seems to make no sense to put it anywhere other than the DT for that platform.
Why should users have to pass magic config options not use DT as intended ?
Alan
Quoted 11 lines · Alan Cox
Quoted 3 lines
yep hardcoded max 4 where in probe first free space is found and used (range 0-3) but still max3100s statically allocated. Shouldn't be this also dynamically allocated?
The code to do the dynamic allocation would be larger than the array of pointers for the sane worst case.
Quoted 1 line
I am not quite sure how exactly you want to do this via DT.
Count the number of DT entries for this kind of port and allocate that many ?
:-) I think there is no doubt how to calculate that number for static/fixed system. But where do you want to put it? And who should read it?
A lot of drivers are calling uart_register_driver in module_init. Others are calling it in probe(pl011 for example).
In module init you probably don't have a pointer to DT to read this and this should be in generic location not in node itself.
If all drivers should call it from probe then we should change that - then reading property is easy and only location should be cleared.
That's why I am curious how exactly you would do it because I haven't seen this before.
Quoted 8 lines
Quoted 8 lines
Also what do you think is a safe maximum number? This is fpga - hundreds of pins which can do just uart.
Quoted 2 lines
There are lots of options better than breaking the "one kernel many platforms" model.
Quoted 6 lines
Quoted 2 lines
Another options is also module parameter and dynamically allocated array in
cdns_uart_init.For a given platform the number is constant and they need to be described, so it seems to make no sense to put it anywhere other than the DT for that platform.
Also I don't think this is really constant in all cases. Especially in connection to fpga where you can put others IPs to PL. You never know what it is present in partial region and how many of these are there. It means having truly dynamic behavior would be welcome.
Can you call uart_register_driver() from probe for every instance? It means nr is 1 all the time.
Quoted 3 lines
Why should users have to pass magic config options not use DT as intended ?
I am not saying that config option is perfect solution. It is at least aligned with 5 others serial drivers in the tree.
Thanks,
Michal
Quoted 2 lines
I am not saying that config option is perfect solution. It is at least aligned with 5 others serial drivers in the tree.
And the fact people keep doing hacks jutifies continuing to make a mess. Especialy as in this case it's entirely theoretical. Nobody has produced actual hardware that hits the limit. Nobody has filed a bug, nobody is impacted.
Creating extra CONFIG_ entries for junk like this is ridiculous, most of the others at least have the excuse of being old code.
Generally it is better to call uart_register_driver from the probe method because that way you don't waste time and memory registering drivers for stuff that isn't even present however if you have no idea how many devices there might be then you still really need to pass a suitable limit and handle it internally dynamically allocating as needed.
If someone was hitting this in the real world and you posted a patch that just changed the constant to 8 or 16 or whatever was needed I wouldn't care too much, but adding CONFIG_ entries just makes stuff harder and harder to config and more and more impossible to keep generic.
I keep hearing that the ARM folks are trying to get one unified kernel. CONFIG_ options is not how to do that.
Alan
Quoted 7 lines · Alan Cox
Quoted 2 lines
I am not saying that config option is perfect solution. It is at least aligned with 5 others serial drivers in the tree.
And the fact people keep doing hacks jutifies continuing to make a mess. Especialy as in this case it's entirely theoretical. Nobody has produced actual hardware that hits the limit. Nobody has filed a bug, nobody is impacted.
This is the reason why we are talking about it how to do it right.
With this ps uart it is not that easy because this is cadence RTL which is not in public IP database but the same think is with uartlite. Limit there is 16. If you really want that I can create that HW design which will require more than 16 uartlites in one design.
Quoted 2 lines
Creating extra
CONFIG_entries for junk like this is ridiculous, most of the others at least have the excuse of being old code.
No doubt about it. I am just trying to find out what's the way you are suggesting.
Quoted 5 lines
Generally it is better to call
uart_register_driverfrom the probe method because that way you don't waste time and memory registering drivers for stuff that isn't even present however if you have no idea how many devices there might be then you still really need to pass a suitable limit and handle it internally dynamically allocating as needed.
Ok. Is there any problem if uart_register_driver is called for every instance separately with nr=1? This driver has major 0, minor 0. Based on comment major 0 is for dynamic node allocation. Not sure about minor but it is easy to figured out if this should be 0 or 1, 2, 3, etc.
Quoted 7 lines
If someone was hitting this in the real world and you posted a patch that just changed the constant to 8 or 16 or whatever was needed I wouldn't care too much, but adding
CONFIG_entries just makes stuff harder and harder to config and more and more impossible to keep generic.I keep hearing that the ARM folks are trying to get one unified kernel.
CONFIG_options is not how to do that.
I have really not a problem with all of this. Just trying to understand how to do it properly and cleanup the second driver which we use on fpga. Last driver used by Xilinx is uart16550 where that old config macro already exists.
Because at least now there is an issue in driver if you use serial aliases (serial2 and up) which needs to be fixed.
Thanks,
Michal
Quoted 20 lines · Michal Simek
Quoted 10 lines · Alan Cox
Quoted 3 lines
I am not saying that config option is perfect solution. It is at least
aligned with 5 others serial drivers in the tree.And the fact people keep doing hacks jutifies continuing to make a mess.
Especialy as in this case it's entirely theoretical. Nobody has produced
actual hardware that hits the limit. Nobody has filed a bug, nobody is impacted.This is the reason why we are talking about it how to do it right.
With this ps uart it is not that easy because this is cadence RTL which is not in public IP database but the same think is with uartlite. Limit there is 16. If you really want that I can create that HW design which will require more than 16 uartlites in one design.
Quoted 6 lines
Quoted 3 lines
Creating extra
CONFIG_entries for junk like this is ridiculous, most of
the others at least have the excuse of being old code.No doubt about it. I am just trying to find out what's the way you are suggesting.
A patch was already recently sent to this mailing list to add a CONFIG_ entry for the maximum number of uartlite devices. It is, as you say, quite
easy to create a device with more than 16 uartlite devices. (It is probably harder to create one with many Cadence RTL PS uarts since that would require a license.)
IMHO it is quite normal for anyone using the uartlite to build his/her own
kernel. and thus make config settings. But it is cumbersome to have to modify the kernel sources. If the number of uartlites could be retrieved from the device tree that would probably be even better if the price in complexity and code size is reasonable.
Maarten
Quoted 2 lines
Ok. Is there any problem if
uart_register_driveris called for every instance separately with nr=1? This driver has major 0, minor 0. Based
If you are doing that level of code restructuring none that I am aware of, and if you find one I'm happy to work on fixing it.
Alan
Quoted 5 lines · Alan Cox
Quoted 2 lines
Ok. Is there any problem if
uart_register_driveris called for every instance separately with nr=1? This driver has major 0, minor 0. BasedIf you are doing that level of code restructuring none that I am aware of, and if you find one I'm happy to work on fixing it.
ok. Let me try to play with it and see if this is working.
M
Quoted 27 lines · Alan Cox
Quoted 9 lines
We have in soc vendor tree similar patch but the reason is different.
tty: serial: Added a
CONFIG_SERIAL_XILINX_NR_UARTSoption.This patch Adds
CONFIG_SERIAL_XILINX_NR_UARTSoption to allow the user to provide the Max number of uart ports information. If multiple cards (or) PL UARTS are present, the default limit of 2 ports should be increased.Quoted 7 lines
I haven't checked all drivers but in our case we have added this as quick fix for scenarios where you use serial aliases where alias is pointed to serial2 or more.
Incdns_uart_init()cdns_uart_uart_driveris passed which contains .nr which is required to be passed.What's the best driver to look at dynamic allocation?
So there are quite a few that dynamically allocate the objects as they are enumerated (eg max3100), but have a maximum set that is just pointers (so for the max number of ports cheaper than the dynamic code)
The other question is why is it a
CONFIG_option. I'm assuming these platforms are all ARM and in that case you could just pass the value in the device tree, or hard code a safe maximum number of pointers to a value which is the worst case and then install them as they are enumerated.
Quoted 4 lines
There are lots of options better than breaking the "one kernel many platforms" model.
Alan
I guess I'm confused how this isn't a better solution than what we have now, or how it breaks the "one kernel many platforms model".
I agree that it is not the best solution, certainly this driver should be re-written to use the device tree and dynamic allocation, but that is not the patch being offered at this time.
This is a very minor module buried deep in the drivers tree. I guess I don't understand how allowing users to choose how many UARTS they might want to implement breaks the "one kernel" model. The users of this module and those like it do not use a pre-compiled kernel and customize their kernels extensively.
Is there some documentation online I could read that explains this "one kernel many platforms" model? Specifically how it pertains to FPGAs? I am but a humble embedded developer trying to make the kernel more useful to myself (and hopefully others), and I think I might have a limited worldview that restricts my ability to see the full picture and I would like to learn.