On Fri, Jan 25, 2019 at 04:36:13PM +0000, Alexandru Elisei wrote:
> On 1/24/19 12:37 PM, Andrew Jones wrote:
> > On Thu, Jan 24, 2019 at 11:59:43AM +0000, Andre Przywara wrote:
> >> On Thu, 24 Jan 2019 11:16:29 +0000
> >> Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
> >>
> >>> A warning is displayed if uart0_base is different from what the code
> >>> expects qemu to generate for the pl011 UART in the device tree.
> >>> However, now we support the ns16550a UART emulated by kvmtool, which
> >>> has a different address. This leads to the warning being displayed
> >>> even though the UART is configured and working as expected.
> >>>
> >>> Now that we support multiple UARTs, the warning serves no purpose, so
> >>> remove it.
> >> Mmh, but we use that address before, right? So for anything not
> >> emulating an UART at this QEMU specific address we write to some random
> >> (device) memory?
> >>
> >> Drew, how important is this early print feature for kvm-unit-tests?
> > The setup code passes through quite a few asserts before getting through
> > io_init() (including in uart0_init), so I think there's still value in
> > having a guessed UART address. Maybe we can provide guesses for both
> > QEMU and kvmtool, and some selection method, that would be used until
> > we've properly assigned uart0_base from DT?
> >
> >> Cheers,
> >> Andre.
> >>
> >>> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> >>> ---
> >>> lib/arm/io.c | 6 ------
> >>> 1 file changed, 6 deletions(-)
> >>>
> >>> diff --git a/lib/arm/io.c b/lib/arm/io.c
> >>> index 35fc05aeb4db..87435150f73e 100644
> >>> --- a/lib/arm/io.c
> >>> +++ b/lib/arm/io.c
> >>> @@ -61,12 +61,6 @@ static void uart0_init(void)
> >>> }
> >>>
> >>> uart0_base = ioremap(base.addr, base.size);
> >>> -
> >>> - if (uart0_base != (u8 *)UART_EARLY_BASE) {
> >>> - printf("WARNING: early print support may not work. "
> >>> - "Found uart at %p, but early base is %p.\n",
> >>> - uart0_base, (u8 *)UART_EARLY_BASE);
> >>> - }
> >>> }
> > This warning is doing what it should, which is pointing out that the
> > UART_EARLY_BASE guess appears to be wrong. If we can provide a way
> > to support more than one guess, then we should keep this warning but
> > adjust it to match one of any of the guesses.
> >
> > Thanks,
> > drew
>
> I'm not really sure how to implement a selection method. I've looked at
> splitting io_init() into uart0_init() and chr_testdev_init() and calling
> uart0_init() very early in the setup process, but uart0_init() itself uses
> printf() and assert().
>
> I've also thought about adding another function, something like
> uart0_early_init(), that is called very early in setup() and gets the base
> address from the dtb bootargs. But that means calling dt_init() and
> dt_get_bootargs(), which can fail.
>
> One other option that could work is to make it a compile-time configuration.
>
> What do you think?
>
Compile-time is fine, which I guess will result in a new configure script
option as well. I wonder if we shouldn't consider generating a config.h
file with stuff like this rather than adding another -D to the compile
line.
drew
