[PATCH v2 5/7] arm: qemu: Enable ramfb by default

[Simon Glass]

Hi Alper,

On Mon, 28 Aug 2023 at 12:29, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> On 2023-08-28 20:54 +03:00, Simon Glass wrote:
> > Hi Alper,
> > On Mon, 28 Aug 2023 at 09:46, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
> >>
> >> On 2023-08-22 21:56 +03:00, Simon Glass wrote:
> >>> Hi Alper,
> >>>
> >>> On Tue, 22 Aug 2023 at 06:10, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
> >>>>
> >>>> From: Alexander Graf <agraf at csgraf.de>
> >>>>
> >>>> Now that we have everything in place to support ramfb, let's wire it up
> >>>> by default in the ARM QEMU targets. That way, you can easily use a
> >>>> graphical console by just passing -device ramfb to the QEMU command line.
> >>>>
> >>>> Signed-off-by: Alexander Graf <agraf at csgraf.de>
> >>>> [Alper: Rebase on bochs changes, add pre-reloc init, error handling]
> >>>> Co-developed-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> >>>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> - Rebase on "qemu: arm: Enable Bochs, console buffering, USB keyboard"
> >>>> - Drop env changes from ARM (necessary but in prerequisite series)
> >>>> - Drop imply VIDEO, SYS_CONSOLE_IN_ENV changes from ARM (in prereq.)
> >>>> - Probe QFW in ARM QEMU board_early_init_f to bind ramfb pre-reloc
> >>>> - Add IS_ENABLED(CONFIG_QFW) check and error handling to ARM QEMU
> >>>>
> >>>>  arch/arm/Kconfig                    |  3 +++
> >>>>  board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++++++++++++
> >>>>  2 files changed, 44 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >>>> index 1fd3ccd1607f..7afe26ac804f 100644
> >>>> --- a/arch/arm/Kconfig
> >>>> +++ b/arch/arm/Kconfig
> >>>> @@ -1046,6 +1046,9 @@ config ARCH_QEMU
> >>>>         imply USB_XHCI_PCI
> >>>>         imply USB_KEYBOARD
> >>>>         imply CMD_USB
> >>>> +       imply VIDEO_RAMFB
> >>>> +       imply BOARD_EARLY_INIT_F
> >>>> +       imply BOARD_EARLY_INIT_R
> >>>>
> >>>>  config ARCH_RMOBILE
> >>>>         bool "Renesas ARM SoCs"
> >>>> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> >>>> index 942f1fff5717..23ef31cb7feb 100644
> >>>> --- a/board/emulation/qemu-arm/qemu-arm.c
> >>>> +++ b/board/emulation/qemu-arm/qemu-arm.c
> >>>> @@ -11,6 +11,7 @@
> >>>>  #include <fdtdec.h>
> >>>>  #include <init.h>
> >>>>  #include <log.h>
> >>>> +#include <qfw.h>
> >>>>  #include <usb.h>
> >>>>  #include <virtio_types.h>
> >>>>  #include <virtio.h>
> >>>> @@ -102,6 +103,46 @@ static struct mm_region qemu_arm64_mem_map[] = {
> >>>>  struct mm_region *mem_map = qemu_arm64_mem_map;
> >>>>  #endif
> >>>>
> >>>> +int board_early_init_f(void)
> >>>> +{
> >>>> +       struct udevice *dev;
> >>>> +       int ret;
> >>>> +
> >>>> +       /*
> >>>> +        * Make sure we enumerate the QEMU Firmware device to bind ramfb
> >>>> +        * so video_reserve() can reserve memory for it.
> >>>> +        */
> >>>> +       if (IS_ENABLED(CONFIG_QFW)) {
> >>>> +               ret = qfw_get_dev(&dev);
> >>>> +               if (ret) {
> >>>> +                       log_err("Failed to get QEMU FW device: %d\n", ret);
> >>>
> >>> We should only present an error if the device is present but
> >>> failed...so if the user doesn't provide the flag, all should be well.
> >>
> >> I don't understand what you mean by "user doesn't provide the flag". But
> >> I assume this should ignore the error (unless what?) so that we can
> >> continue to boot. Would that apply for board_early_init_r below as well?
> >
> > I thought you said there was a qemu flag to enable ramfb? So add it to
> > the DT and then the device can (in its bind routine) decide not to be
> > bound by returning -ENODEV
>
> Ah. I'm not used to calling them flags, especially those that have
> values as in "-device ramfb". My mind jumped to DM_FLAG_PRE_RELOC /
> bootph-some-ram.
>
> But note that what this code tries to probe here is not the ramfb
> device, it's the bus/parent of that. If this fails, we can't know if the
> user specified "-device ramfb" or not.
>
> Is the parent node's device probed by the time .bind() is called? Or,
> can I have it probed by calling qfw_get_dev() (which is just
> uclass_first_device_err(UCLASS_QFW, ...))?

I suppose your problem might be that there are up to three possible
video devices (vesa, bochs, ramfb) and one of them needs to return 0
from bind() and the others need to return -ENODEV?

Perhaps another way would be to update the logic in
stdio_init_tables() to find them all (which I believe it already
does?) and then select one?

Regards,
Simon