On 7/23/23 06:40, Bin Meng wrote:

In preparation to future changes, refactor this routine a little bit.

Signed-off-by: Bin Meng bmeng@tinylab.org

common/console.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/common/console.c b/common/console.c index d0640ba05a..af52897ec3 100644 --- a/common/console.c +++ b/common/console.c @@ -1012,27 +1012,27 @@ int console_init_f(void)

static void stdio_print_current_devices(void) {

• char *stdinname, *stdoutname, *stderrname;
• stdinname = stdio_devices[stdin] ?

•     stdio_devices[stdin]->name :
  

•     "No input devices available!";
  

• stdoutname = stdio_devices[stdout] ?

•      stdio_devices[stdout]->name :
  

•      "No output devices available!";
  

This string will never be printed as you have no output device.

Best regards

Heinrich

• stderrname = stdio_devices[stderr] ?

•      stdio_devices[stderr]->name :
  

•      "No error devices available!";
  

• /* Print information */ puts("In: ");

• if (stdio_devices[stdin] == NULL) {

• puts("No input devices available!\n");
  

• } else {

• printf ("%s\n", stdio_devices[stdin]->name);
  

• }

• printf("%s\n", stdinname);

  puts("Out: ");

• if (stdio_devices[stdout] == NULL) {

• puts("No output devices available!\n");
  

• } else {

• printf ("%s\n", stdio_devices[stdout]->name);
  

• }

• printf("%s\n", stdoutname);

  puts("Err: ");

• if (stdio_devices[stderr] == NULL) {

• puts("No error devices available!\n");
  

• } else {

• printf ("%s\n", stdio_devices[stderr]->name);
  

• }

• printf("%s\n", stderrname); }

  #if CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)

On 7/23/23 06:40, Bin Meng wrote:

At present if both CONSOLE_MUX and SYS_CONSOLE_IS_IN_ENV are on, during boot, the printed out stdio devices are incomplete, e.g.: with "stdout=serial,vidconsole", only "vidconsole" is printed.

For such case, we can print out the stdio device name from the environment variables.

Signed-off-by: Bin Meng bmeng@tinylab.org

common/console.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/common/console.c b/common/console.c index af52897ec3..98c3ee6ca6 100644 --- a/common/console.c +++ b/common/console.c @@ -1014,15 +1014,27 @@ static void stdio_print_current_devices(void) { char *stdinname, *stdoutname, *stderrname;

• stdinname = stdio_devices[stdin] ?

•     stdio_devices[stdin]->name :
  

•     "No input devices available!";
  

• stdoutname = stdio_devices[stdout] ?

•      stdio_devices[stdout]->name :
  

•      "No output devices available!";
  

• stderrname = stdio_devices[stderr] ?

•      stdio_devices[stderr]->name :
  

•      "No error devices available!";
  

• if (CONFIG_IS_ENABLED(CONSOLE_MUX) &&

•    CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)) {
  

• /* stdin stdout and stderr are in environment */
  

• stdinname  = env_get("stdin");
  

• stdoutname = env_get("stdout");
  

• stderrname = env_get("stderr");
  

• stdinname = stdinname ? : "No input devices available!";
  

• stdoutname = stdoutname ? : "No output devices available!";
  

This string will never be printed as you have no output device.

• stderrname = stderrname ? : "No error devices available!";
  

• } else {

• stdinname = stdio_devices[stdin] ?
  

• 	stdio_devices[stdin]->name :
  

• 	"No input devices available!";
  

• stdoutname = stdio_devices[stdout] ?
  

• 	stdio_devices[stdout]->name :
  

• 	"No output devices available!";
  

ditto

• stderrname = stdio_devices[stderr] ?
  

• 	stdio_devices[stderr]->name :
  

• 	"No error devices available!";
  

• }

do_coninfo() shows how to print all muxed devices irrespective of SYS_CONSOLE_IS_IN_ENV.

Best regards

Heinrich

/* Print information */ puts("In: ");

Am 23. Juli 2023 14:04:39 MESZ schrieb Bin Meng bmeng.cn@gmail.com:

Hi Heinrich,

On Sun, Jul 23, 2023 at 2:38 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 7/23/23 06:40, Bin Meng wrote:

...

At present if both CONSOLE_MUX and SYS_CONSOLE_IS_IN_ENV are on, during boot, the printed out stdio devices are incomplete, e.g.: with "stdout=serial,vidconsole", only "vidconsole" is printed.

For such case, we can print out the stdio device name from the environment variables.

Signed-off-by: Bin Meng bmeng@tinylab.org

common/console.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/common/console.c b/common/console.c index af52897ec3..98c3ee6ca6 100644 --- a/common/console.c +++ b/common/console.c @@ -1014,15 +1014,27 @@ static void stdio_print_current_devices(void) { char *stdinname, *stdoutname, *stderrname;

• stdinname = stdio_devices[stdin] ?
  

•             stdio_devices[stdin]->name :
  

•             "No input devices available!";
  

• stdoutname = stdio_devices[stdout] ?
  

•              stdio_devices[stdout]->name :
  

•              "No output devices available!";
  

• stderrname = stdio_devices[stderr] ?
  

•              stdio_devices[stderr]->name :
  

•              "No error devices available!";
  

• if (CONFIG_IS_ENABLED(CONSOLE_MUX) &&
  

•     CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)) {
  

•         /* stdin stdout and stderr are in environment */
  

•         stdinname  = env_get("stdin");
  

•         stdoutname = env_get("stdout");
  

•         stderrname = env_get("stderr");
  

•         stdinname = stdinname ? : "No input devices available!";
  

•         stdoutname = stdoutname ? : "No output devices available!";
  

This string will never be printed as you have no output device.

This logic follows what it was before. Yes, without a stdout device, puts() or printf() is useless.

...

...

•         stderrname = stderrname ? : "No error devices available!";
  

• } else {
  

•         stdinname = stdio_devices[stdin] ?
  

•                 stdio_devices[stdin]->name :
  

•                 "No input devices available!";
  

•         stdoutname = stdio_devices[stdout] ?
  

•                 stdio_devices[stdout]->name :
  

•                 "No output devices available!";
  

ditto

...

•         stderrname = stdio_devices[stderr] ?
  

•                 stdio_devices[stderr]->name :
  

•                 "No error devices available!";
  

• }
  

do_coninfo() shows how to print all muxed devices irrespective of SYS_CONSOLE_IS_IN_ENV.

Are you suggesting we refactor the codes to follow the same logic used in do_coninfo()?

As the coninfo command is enabled on most devices, we could use a common output function to reduce code size.

Do we need this output at all if we hsve coninfo?

Regards

Heinrich

Regards, Bin

On Sun, 23 Jul 2023 at 08:17, Bin Meng bmeng.cn@gmail.com wrote:

Hi Heinrich,

On Sun, Jul 23, 2023 at 8:48 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

Am 23. Juli 2023 14:04:39 MESZ schrieb Bin Meng bmeng.cn@gmail.com:

...

Hi Heinrich,

On Sun, Jul 23, 2023 at 2:38 PM Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 7/23/23 06:40, Bin Meng wrote:

...

At present if both CONSOLE_MUX and SYS_CONSOLE_IS_IN_ENV are on, during boot, the printed out stdio devices are incomplete, e.g.: with "stdout=serial,vidconsole", only "vidconsole" is printed.

For such case, we can print out the stdio device name from the environment variables.

Signed-off-by: Bin Meng bmeng@tinylab.org

common/console.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/common/console.c b/common/console.c index af52897ec3..98c3ee6ca6 100644 --- a/common/console.c +++ b/common/console.c @@ -1014,15 +1014,27 @@ static void stdio_print_current_devices(void) { char *stdinname, *stdoutname, *stderrname;

• stdinname = stdio_devices[stdin] ?
  

•             stdio_devices[stdin]->name :
  

•             "No input devices available!";
  

• stdoutname = stdio_devices[stdout] ?
  

•              stdio_devices[stdout]->name :
  

•              "No output devices available!";
  

• stderrname = stdio_devices[stderr] ?
  

•              stdio_devices[stderr]->name :
  

•              "No error devices available!";
  

• if (CONFIG_IS_ENABLED(CONSOLE_MUX) &&
  

•     CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)) {
  

•         /* stdin stdout and stderr are in environment */
  

•         stdinname  = env_get("stdin");
  

•         stdoutname = env_get("stdout");
  

•         stderrname = env_get("stderr");
  

•         stdinname = stdinname ? : "No input devices available!";
  

•         stdoutname = stdoutname ? : "No output devices available!";
  

This string will never be printed as you have no output device.

This logic follows what it was before. Yes, without a stdout device, puts() or printf() is useless.

...

...

•         stderrname = stderrname ? : "No error devices available!";
  

• } else {
  

•         stdinname = stdio_devices[stdin] ?
  

•                 stdio_devices[stdin]->name :
  

•                 "No input devices available!";
  

•         stdoutname = stdio_devices[stdout] ?
  

•                 stdio_devices[stdout]->name :
  

•                 "No output devices available!";
  

ditto

...

•         stderrname = stdio_devices[stderr] ?
  

•                 stdio_devices[stderr]->name :
  

•                 "No error devices available!";
  

• }
  

do_coninfo() shows how to print all muxed devices irrespective of SYS_CONSOLE_IS_IN_ENV.

Are you suggesting we refactor the codes to follow the same logic used in do_coninfo()?

As the coninfo command is enabled on most devices, we could use a common output function to reduce code size.

Do we need this output at all if we hsve coninfo?

I think so. U-Boot has been displaying the stdio devices for quite a long time. The config option SYS_CONSOLE_INFO_QUIET was introduced for any board who does not want this.

config SYS_CONSOLE_INFO_QUIET bool "Don't display the console devices on boot" help Normally U-Boot displays the current settings for stdout, stdin and stderr on boot when the post-relocation console is set up. Enable this option to suppress this output. It can be obtained by calling stdio_print_current_devices() from board code.

Tom, what's your idea?

I agree that we should show it, since otherwise it is misleading.

Reviewed-by: Simon Glass sjg@chromium.org

Am 23. Juli 2023 06:40:41 MESZ schrieb Bin Meng bmeng@tinylab.org:

This brings PCI xHCI support to QEMU RISC-V and uses a usb keyboard as one of the input devices.

Signed-off-by: Bin Meng bmeng@tinylab.org

---

board/emulation/qemu-riscv/Kconfig | 5 +++++ board/emulation/qemu-riscv/qemu-riscv.c | 5 +++++ doc/board/emulation/qemu-riscv.rst | 5 +++++ include/configs/qemu-riscv.h | 2 +- 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/board/emulation/qemu-riscv/Kconfig b/board/emulation/qemu-riscv/Kconfig index 7220c55350..b503578d27 100644 --- a/board/emulation/qemu-riscv/Kconfig +++ b/board/emulation/qemu-riscv/Kconfig @@ -76,5 +76,10 @@ config BOARD_SPECIFIC_OPTIONS # dummy imply VIDEO_BOCHS imply SYS_WHITE_ON_BLACK imply PRE_CONSOLE_BUFFER

• imply USB
• imply USB_XHCI_HCD
• imply USB_XHCI_PCI

QEMU could alternatively use EHCI or OHCI for the keyboard. Does enabling XHCI add support for these too?

Best regards

Heinrich

• imply USB_KEYBOARD
• imply CMD_USB

endif diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c index f39f3be366..181abbbf97 100644 --- a/board/emulation/qemu-riscv/qemu-riscv.c +++ b/board/emulation/qemu-riscv/qemu-riscv.c @@ -12,6 +12,7 @@ #include <log.h> #include <spl.h> #include <init.h> +#include <usb.h> #include <virtio_types.h> #include <virtio.h>

@@ -41,6 +42,10 @@ int board_init(void)

int board_late_init(void) {

• /* start usb so that usb keyboard can be used as input device */
• if (CONFIG_IS_ENABLED(USB_KEYBOARD))

• usb_init();
  

• return 0;

}

diff --git a/doc/board/emulation/qemu-riscv.rst b/doc/board/emulation/qemu-riscv.rst index 9d21f3270c..61137bcbf1 100644 --- a/doc/board/emulation/qemu-riscv.rst +++ b/doc/board/emulation/qemu-riscv.rst @@ -138,6 +138,11 @@ and adding::

-serial stdio -device VGA

+In addition, a usb keyboard can be attached to an emulated xHCI controller in +RISC-V virt machine as an option of input devices by adding::

• -device qemu-xhci,id=xhci -device usb-kbd,bus=xhci.0

Running with KVM

diff --git a/include/configs/qemu-riscv.h b/include/configs/qemu-riscv.h index d5146e70f7..584559cfa3 100644 --- a/include/configs/qemu-riscv.h +++ b/include/configs/qemu-riscv.h @@ -17,7 +17,7 @@

/* Environment options */

-#define CFG_STD_DEVICES_SETTINGS "stdin=serial\0" \ +#define CFG_STD_DEVICES_SETTINGS "stdin=serial,usbkbd\0" \ "stdout=serial,vidconsole\0" \ "stderr=serial,vidconsole\0"

From: Bin Meng bmeng@tinylab.org Date: Sun, 23 Jul 2023 12:40:41 +0800

This brings PCI xHCI support to QEMU RISC-V and uses a usb keyboard as one of the input devices.

Signed-off-by: Bin Meng bmeng@tinylab.org

---

board/emulation/qemu-riscv/Kconfig | 5 +++++ board/emulation/qemu-riscv/qemu-riscv.c | 5 +++++ doc/board/emulation/qemu-riscv.rst | 5 +++++ include/configs/qemu-riscv.h | 2 +- 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/board/emulation/qemu-riscv/Kconfig b/board/emulation/qemu-riscv/Kconfig index 7220c55350..b503578d27 100644 --- a/board/emulation/qemu-riscv/Kconfig +++ b/board/emulation/qemu-riscv/Kconfig @@ -76,5 +76,10 @@ config BOARD_SPECIFIC_OPTIONS # dummy imply VIDEO_BOCHS imply SYS_WHITE_ON_BLACK imply PRE_CONSOLE_BUFFER

• imply USB
• imply USB_XHCI_HCD
• imply USB_XHCI_PCI
• imply USB_KEYBOARD
• imply CMD_USB

endif diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c index f39f3be366..181abbbf97 100644 --- a/board/emulation/qemu-riscv/qemu-riscv.c +++ b/board/emulation/qemu-riscv/qemu-riscv.c @@ -12,6 +12,7 @@ #include <log.h> #include <spl.h> #include <init.h> +#include <usb.h> #include <virtio_types.h> #include <virtio.h>

@@ -41,6 +42,10 @@ int board_init(void)

int board_late_init(void) {

• /* start usb so that usb keyboard can be used as input device */
• if (CONFIG_IS_ENABLED(USB_KEYBOARD))

• usb_init();
  

This is typically handled by including "usb start" in CONFIG_PREBOOT, which is done by boot/Kconfig. Any reason why that doesn't work for you?

return 0; }

diff --git a/doc/board/emulation/qemu-riscv.rst b/doc/board/emulation/qemu-riscv.rst index 9d21f3270c..61137bcbf1 100644 --- a/doc/board/emulation/qemu-riscv.rst +++ b/doc/board/emulation/qemu-riscv.rst @@ -138,6 +138,11 @@ and adding::

 -serial stdio -device VGA

+In addition, a usb keyboard can be attached to an emulated xHCI controller in +RISC-V virt machine as an option of input devices by adding::

• -device qemu-xhci,id=xhci -device usb-kbd,bus=xhci.0

Running with KVM

diff --git a/include/configs/qemu-riscv.h b/include/configs/qemu-riscv.h index d5146e70f7..584559cfa3 100644 --- a/include/configs/qemu-riscv.h +++ b/include/configs/qemu-riscv.h @@ -17,7 +17,7 @@

/* Environment options */

-#define CFG_STD_DEVICES_SETTINGS "stdin=serial\0" \ +#define CFG_STD_DEVICES_SETTINGS "stdin=serial,usbkbd\0" \ "stdout=serial,vidconsole\0" \ "stderr=serial,vidconsole\0"

-- 2.34.1

Date: Sun, 23 Jul 2023 11:30:31 +0200 From: Heinrich Schuchardt xypron.glpk@gmx.de

Am 23. Juli 2023 10:38:00 MESZ schrieb Mark Kettenis mark.kettenis@xs4all.nl:

...

...

From: Bin Meng bmeng@tinylab.org Date: Sun, 23 Jul 2023 12:40:41 +0800

This brings PCI xHCI support to QEMU RISC-V and uses a usb keyboard as one of the input devices.

Signed-off-by: Bin Meng bmeng@tinylab.org

---

board/emulation/qemu-riscv/Kconfig | 5 +++++ board/emulation/qemu-riscv/qemu-riscv.c | 5 +++++ doc/board/emulation/qemu-riscv.rst | 5 +++++ include/configs/qemu-riscv.h | 2 +- 4 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/board/emulation/qemu-riscv/Kconfig b/board/emulation/qemu-riscv/Kconfig index 7220c55350..b503578d27 100644 --- a/board/emulation/qemu-riscv/Kconfig +++ b/board/emulation/qemu-riscv/Kconfig @@ -76,5 +76,10 @@ config BOARD_SPECIFIC_OPTIONS # dummy imply VIDEO_BOCHS imply SYS_WHITE_ON_BLACK imply PRE_CONSOLE_BUFFER

• imply USB
• imply USB_XHCI_HCD
• imply USB_XHCI_PCI
• imply USB_KEYBOARD
• imply CMD_USB

endif diff --git a/board/emulation/qemu-riscv/qemu-riscv.c b/board/emulation/qemu-riscv/qemu-riscv.c index f39f3be366..181abbbf97 100644 --- a/board/emulation/qemu-riscv/qemu-riscv.c +++ b/board/emulation/qemu-riscv/qemu-riscv.c @@ -12,6 +12,7 @@ #include <log.h> #include <spl.h> #include <init.h> +#include <usb.h> #include <virtio_types.h> #include <virtio.h>

@@ -41,6 +42,10 @@ int board_init(void)

int board_late_init(void) {

• /* start usb so that usb keyboard can be used as input device */
• if (CONFIG_IS_ENABLED(USB_KEYBOARD))

• usb_init();
  

This is typically handled by including "usb start" in CONFIG_PREBOOT, which is done by boot/Kconfig. Any reason why that doesn't work for you?

We run pci_init() in board_r.c. Why don't do the same for USB instead of the PREBOOT quirk?

Well, yes, that was going to be my next question. If using PREBOOT is for some reason no longer the preferred way to do this, should there be some other generic method to make sure the USB controllers are started when a USB keyboard is configured as an input method. Doing something generic like this in board-specific code seems wrong to me.

Cheers,

Mark

...

...

return 0; }

diff --git a/doc/board/emulation/qemu-riscv.rst b/doc/board/emulation/qemu-riscv.rst index 9d21f3270c..61137bcbf1 100644 --- a/doc/board/emulation/qemu-riscv.rst +++ b/doc/board/emulation/qemu-riscv.rst @@ -138,6 +138,11 @@ and adding::

 -serial stdio -device VGA

+In addition, a usb keyboard can be attached to an emulated xHCI controller in +RISC-V virt machine as an option of input devices by adding::

• -device qemu-xhci,id=xhci -device usb-kbd,bus=xhci.0

Running with KVM

diff --git a/include/configs/qemu-riscv.h b/include/configs/qemu-riscv.h index d5146e70f7..584559cfa3 100644 --- a/include/configs/qemu-riscv.h +++ b/include/configs/qemu-riscv.h @@ -17,7 +17,7 @@

/* Environment options */

-#define CFG_STD_DEVICES_SETTINGS "stdin=serial\0" \ +#define CFG_STD_DEVICES_SETTINGS "stdin=serial,usbkbd\0" \ "stdout=serial,vidconsole\0" \ "stderr=serial,vidconsole\0"

-- 2.34.1