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

[Sebastian Huber]

On 03/04/2019 00:07, Chris Johns wrote:
> 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.

The driver uses interrupts, so the patch as is is not all right from my 
point of view, see my comment in another e-mail to this patch.

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

https://docs.rtems.org/branches/master/user/bsps/bsps-riscv.html#build-configuration-options

The device tree support itself should not use BSP options.

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.