On 2/4/19 10:08 pm, Hesham Almatary wrote: > On Sun, 31 Mar 2019 at 22:50, Chris Johns <chr...@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 _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel