Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1646046 > unrolled thread

[PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts Sam Povilus <kernel.development@povil.us> - 2017-05-20 04:30 +0200
    Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the  number of xilinx ps uarts Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-05-20 18:30 +0200
      Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the  number of xilinx ps uarts Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-05-22 20:30 +0200
        Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the  number of xilinx ps uarts Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-05-23 22:10 +0200
          Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the  number of xilinx ps uarts Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-05-24 15:40 +0200
        Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the  number of xilinx ps uarts Sam Povilus <kernel.development@povil.us> - 2017-05-24 05:30 +0200

#1646046 — [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. 

Signed-off-by: Sam Povilus <kernel.development@povil.us>
---
 drivers/tty/serial/Kconfig         | 9 +++++++++
 drivers/tty/serial/xilinx_uartps.c | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 5c8850f7a2a0..fef25f17a4cc 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -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
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index c0539950f8d7..a2c51c35da65 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -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
 
-- 
2.11.0

[toc] | [next] | [standalone]

#1646146 — Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts

On Fri, 19 May 2017 20:21:54 -0600
Sam Povilus <kernel.development@povil.us> wrote:

> 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

[toc] | [prev] | [next] | [standalone]

#1647246 — Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts

> 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?

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

[toc] | [prev] | [next] | [standalone]

#1648386 — Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts

> 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.

> 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 ?

> 
> Also what do you think is a safe maximum number? This is fpga - hundreds
> of pins which can do just uart.
> 
> > 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

[toc] | [prev] | [next] | [standalone]

#1649618 — Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts

> 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

[toc] | [prev] | [next] | [standalone]

#1649074 — Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts

On Mon, May 22, 2017 at 07:26:36PM +0100, Alan Cox wrote:
> > 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?
> 
> 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


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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web