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

[Chris Johns]

On 2/4/19 10:08 pm, Hesham Almatary wrote:
> 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.

And what is this BSP developer to do when they see this and need it?

What if I need the commented code and I post a change that uncomments this code
and comments the other call. I suspect the patch would be voted down.

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

Sure, that is fine ... but can we have some User Manual documentation of the
options this BSP has if that is OK? :)

To be openly honest I dislike configure options and BSP_OPTS. The BSP
configurations are hardly documented, sporadic in what they cover and naming,
are never tested and so equally confusing. We have a large amount of BSP code in
RTEMS controlled by these options and we do not know if the code works let alone
builds. The options are difficult to validate and the interactions between
conflicting options is often not clear, are difficult to manage, and often never
checked in the configure code. And there is no way we could ever build a small
subset of these options with a buildbot type test tool.

I prefer the weak function approach as used in the Zynq BSP because the build of
RTEMS is standard, complex operations and code can be handled with C constructs
with the support of the compiler, and it is easier for a user to configuration
manage in their code base rather than the way they build RTEMS. We also have
only a base level of functionality in RTEMS and it is always built when the BSP
is built.

Chris