[PATCH 3/4] bsps/riscv: UART - Comment code that queries UART interrupts

[Hesham Almatary]

On Sun, 31 Mar 2019 at 22:50, Chris Johns <chrisj at rtems.org> wrote:
>
> On 1/4/19 1:33 am, Hesham Almatary wrote:
> > * Different RISC-V DTBs use different names for UART interrupts such as
> > "interrupts" and "interrupts-extended", so it is not portable.
> > * polling functions are currently used, so there is no need to query interrupts.
> > ---
> >  bsps/riscv/riscv/console/console-config.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/bsps/riscv/riscv/console/console-config.c b/bsps/riscv/riscv/console/console-config.c
> > index 04d0b28361..81e46f304f 100644
> > --- a/bsps/riscv/riscv/console/console-config.c
> > +++ b/bsps/riscv/riscv/console/console-config.c
> > @@ -183,13 +183,20 @@ static void riscv_console_probe(void)
> >
> >        ctx->clock = fdt32_to_cpu(val[0]);
> >
> > -      val = (fdt32_t *) fdt_getprop(fdt, node, "interrupts", &len);
> > +      /* XXX Different RISC-V DTBs use different property names for interrupt
> > +      sources. If an interrupt-driven driver is needed, uncomment, replace the
> > +      "interrupts-extended" string below with your target's DTB name for UART
> > +      interrupts, and use interrupt-driven UART functions instead of
> > +      ns16550_polled_* functions */
> > +
> > +      /* val = (fdt32_t *) fdt_getprop(fdt, node, "interrupts-extended", &len);
>
> Asking a user to edit a BSP complicates a user's ability to configuration manage
> and track RTEMS so I prefer we find and use other solutions. For example you
> could replace the commented call with a weak function of:
>
The comment is not really for the user, but rather for RISC-V BSP
developers. I came across this issue when I was trying to get this BSP
(which is supposed to be a generic riscv one, but it only accounts for
QEMU's DTB format) to work on another platform.

>   val = rtems_riscv_get_console_interrupts(fdt, node, &len);
>
> which returns by default "interrupts". A user can then provide a non-weak
> version of function and override the default behaviour and return
> "interrupts-extended"
>
That makes things slightly complicated for the user I think. What
about having a BSP option in the riscv configure.ac that specifies the
DTB UART interrupt node name? For example, similar to how we currently
pass -DRISCV_ENABLE_HTIF_SUPPORT to build RISC-V BSP for Spike, we can
add another BSP_FDT_UART_INTERRUPT option (which defaults to
"interrupts"), but allow the user to override it at configure time.

> A documentation patch for the user manual about this would be nice.
>
> Chris