diff mbox series

[3/3] cmd: add acmconsole command

Message ID 1629371586-9349-3-git-send-email-loic.poulain@linaro.org
State New
Headers show
Series [1/3] lib/circbuf: Make circbuf selectable symbol | expand

Commit Message

Loic Poulain Aug. 19, 2021, 11:13 a.m. UTC 
This command allows to start CDC ACM function and redirect console
(stdin, stdout, stderr) to USB (acmconsole start). The console can
then be accessed through the USB host for debugging purpose. The
host can stop the session (acmconsole stop) to revert back console
to serial and unregister CDC ACM USB function.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>

---
 cmd/Kconfig      |  8 ++++++++
 cmd/Makefile     |  2 ++
 cmd/acmconsole.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+)
 create mode 100644 cmd/acmconsole.c

-- 
2.7.4

Comments

Pali Rohár Sept. 27, 2021, 8:04 p.m. UTC | #1 
On Thursday 19 August 2021 13:13:06 Loic Poulain wrote:
> This command allows to start CDC ACM function and redirect console

> (stdin, stdout, stderr) to USB (acmconsole start). The console can

> then be accessed through the USB host for debugging purpose. The

> host can stop the session (acmconsole stop) to revert back console

> to serial and unregister CDC ACM USB function.


Hello!

Could you rather implement this CDC ACM gadget in a way that calling
'setenv stdout cdc_acm' (or 'setenv stdout usbtty') would do this
console redirect? Like it is supported for current usbtty or vga output
code.

Then this acmconsole command would not be needed at all.

Nokia N900 uses usbtty and has in its default env defined:

   "stdin=usbtty,serial,vga\0"
   "stdout=usbtty,serial,vga\0"
   "stderr=usbtty,serial,vga\0"

With this configuration, U-Boot console is automatically activated on
usbtty, serial console and also on vga display. And no additional
command is needed. U-Boot console works on all "outputs" at the same
time.
Loic Poulain Nov. 16, 2021, 7:05 p.m. UTC | #2 
Hi Pali,

Sorry for the late reply,

On Mon, 27 Sept 2021 at 22:04, Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 19 August 2021 13:13:06 Loic Poulain wrote:
> > This command allows to start CDC ACM function and redirect console
> > (stdin, stdout, stderr) to USB (acmconsole start). The console can
> > then be accessed through the USB host for debugging purpose. The
> > host can stop the session (acmconsole stop) to revert back console
> > to serial and unregister CDC ACM USB function.
>
> Hello!
>
> Could you rather implement this CDC ACM gadget in a way that calling
> 'setenv stdout cdc_acm' (or 'setenv stdout usbtty') would do this
> console redirect? Like it is supported for current usbtty or vga output
> code.
>
> Then this acmconsole command would not be needed at all.

Yes, that would be good, but AFAIR I restricted this cdc_acm usage to
this command because we can't have fine grained control like e.g.
using cdc_acm as stdout only, when used, it should necessarily be
STDIN (at least). The reason is because of the single-tasking nature
of u-boot, and the fact that we need to poll the USB controller for
events via the 'usb_gadget_handle_interrupts()' function. In our case
we simply do that in the getc() function, which is only called for
input consoles in the u-boot mainloop.

An alternative would be to have a u-boot API to register generic
callbacks to be executed in the mainloop, but this is probably a
different thread.

Also we could possibly live with this 'bug' and simply accept that
stdout-only configuration will be broken.

Regards,
Loic
Pali Rohár Nov. 16, 2021, 7:36 p.m. UTC | #3 
Hello!

On Tuesday 16 November 2021 20:05:07 Loic Poulain wrote:
> Hi Pali,
> 
> Sorry for the late reply,
> 
> On Mon, 27 Sept 2021 at 22:04, Pali Rohár <pali@kernel.org> wrote:
> >
> > On Thursday 19 August 2021 13:13:06 Loic Poulain wrote:
> > > This command allows to start CDC ACM function and redirect console
> > > (stdin, stdout, stderr) to USB (acmconsole start). The console can
> > > then be accessed through the USB host for debugging purpose. The
> > > host can stop the session (acmconsole stop) to revert back console
> > > to serial and unregister CDC ACM USB function.
> >
> > Hello!
> >
> > Could you rather implement this CDC ACM gadget in a way that calling
> > 'setenv stdout cdc_acm' (or 'setenv stdout usbtty') would do this
> > console redirect? Like it is supported for current usbtty or vga output
> > code.
> >
> > Then this acmconsole command would not be needed at all.
> 
> Yes, that would be good, but AFAIR I restricted this cdc_acm usage to
> this command because we can't have fine grained control like e.g.
> using cdc_acm as stdout only, when used, it should necessarily be
> STDIN (at least). The reason is because of the single-tasking nature
> of u-boot, and the fact that we need to poll the USB controller for
> events via the 'usb_gadget_handle_interrupts()' function.

There was already discussion that custom commands for activating drivers
are against driver model design. See:
https://lore.kernel.org/u-boot/20211026151742.42b0fcfa@thinkpad/

So I think that there should not be any driver specific command which
just activates device (in this case console).

> In our case
> we simply do that in the getc() function, which is only called for
> input consoles in the u-boot mainloop.

This looks like a hack in the current implementation which seems that
also misuse driver model design. At least this should be documented as
this is limitation of functionality.

> An alternative would be to have a u-boot API to register generic
> callbacks to be executed in the mainloop, but this is probably a
> different thread.

I guess that this should be discussed, so some optimal solution for
issue is chosen. Otherwise we will have there second implementation of
"usbtty", which even would not provide all functionality as original
"usbtty" code.

> Also we could possibly live with this 'bug' and simply accept that
> stdout-only configuration will be broken.
> 
> Regards,
> Loic
Loic Poulain Nov. 22, 2021, 10:33 a.m. UTC | #4 
Hi Pali,

On Tue, 16 Nov 2021 at 20:36, Pali Rohár <pali@kernel.org> wrote:
>
> Hello!
>
> On Tuesday 16 November 2021 20:05:07 Loic Poulain wrote:
> > Hi Pali,
> >
> > Sorry for the late reply,
> >
> > On Mon, 27 Sept 2021 at 22:04, Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Thursday 19 August 2021 13:13:06 Loic Poulain wrote:
> > > > This command allows to start CDC ACM function and redirect console
> > > > (stdin, stdout, stderr) to USB (acmconsole start). The console can
> > > > then be accessed through the USB host for debugging purpose. The
> > > > host can stop the session (acmconsole stop) to revert back console
> > > > to serial and unregister CDC ACM USB function.
> > >
> > > Hello!
> > >
> > > Could you rather implement this CDC ACM gadget in a way that calling
> > > 'setenv stdout cdc_acm' (or 'setenv stdout usbtty') would do this
> > > console redirect? Like it is supported for current usbtty or vga output
> > > code.
> > >
> > > Then this acmconsole command would not be needed at all.
> >
> > Yes, that would be good, but AFAIR I restricted this cdc_acm usage to
> > this command because we can't have fine grained control like e.g.
> > using cdc_acm as stdout only, when used, it should necessarily be
> > STDIN (at least). The reason is because of the single-tasking nature
> > of u-boot, and the fact that we need to poll the USB controller for
> > events via the 'usb_gadget_handle_interrupts()' function.
>
> There was already discussion that custom commands for activating drivers
> are against driver model design. See:
> https://lore.kernel.org/u-boot/20211026151742.42b0fcfa@thinkpad/
>
> So I think that there should not be any driver specific command which
> just activates device (in this case console).

That's a bit different here since it's not a standard
device/peripheral driver but a USB function, which is something the
'user' can decide to enable or not, all the other USB functions have
dedicated commands AFAIK. It's not clear otherwise at which place we
should register the acm_console? should it be unconditionally
registered at init?

Regards,
Loic
Pali Rohár Nov. 22, 2021, 11:54 a.m. UTC | #5 
On Monday 22 November 2021 11:33:15 Loic Poulain wrote:
> Hi Pali,
> 
> On Tue, 16 Nov 2021 at 20:36, Pali Rohár <pali@kernel.org> wrote:
> >
> > Hello!
> >
> > On Tuesday 16 November 2021 20:05:07 Loic Poulain wrote:
> > > Hi Pali,
> > >
> > > Sorry for the late reply,
> > >
> > > On Mon, 27 Sept 2021 at 22:04, Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Thursday 19 August 2021 13:13:06 Loic Poulain wrote:
> > > > > This command allows to start CDC ACM function and redirect console
> > > > > (stdin, stdout, stderr) to USB (acmconsole start). The console can
> > > > > then be accessed through the USB host for debugging purpose. The
> > > > > host can stop the session (acmconsole stop) to revert back console
> > > > > to serial and unregister CDC ACM USB function.
> > > >
> > > > Hello!
> > > >
> > > > Could you rather implement this CDC ACM gadget in a way that calling
> > > > 'setenv stdout cdc_acm' (or 'setenv stdout usbtty') would do this
> > > > console redirect? Like it is supported for current usbtty or vga output
> > > > code.
> > > >
> > > > Then this acmconsole command would not be needed at all.
> > >
> > > Yes, that would be good, but AFAIR I restricted this cdc_acm usage to
> > > this command because we can't have fine grained control like e.g.
> > > using cdc_acm as stdout only, when used, it should necessarily be
> > > STDIN (at least). The reason is because of the single-tasking nature
> > > of u-boot, and the fact that we need to poll the USB controller for
> > > events via the 'usb_gadget_handle_interrupts()' function.
> >
> > There was already discussion that custom commands for activating drivers
> > are against driver model design. See:
> > https://lore.kernel.org/u-boot/20211026151742.42b0fcfa@thinkpad/
> >
> > So I think that there should not be any driver specific command which
> > just activates device (in this case console).
> 
> That's a bit different here since it's not a standard
> device/peripheral driver but a USB function, which is something the
> 'user' can decide to enable or not, all the other USB functions have
> dedicated commands AFAIK. It's not clear otherwise at which place we
> should register the acm_console? should it be unconditionally
> registered at init?

I really do not know. But for me it could make sense that activation
happens when calling standard "setenv std* cdc_acm" commands...
diff mbox series

Patch

diff --git a/cmd/Kconfig b/cmd/Kconfig
index ffef3cc..7cc9b1c 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1382,6 +1382,14 @@  config CMD_AXI
 	  Interface) busses, a on-chip interconnect specification for managing
 	  functional blocks in SoC designs, which is also often used in designs
 	  involving FPGAs (e.g.  communication with IP cores in Xilinx FPGAs).
+
+config CMD_USB_ACM_CONSOLE
+	bool "ACM USB console"
+	select USB_FUNCTION_ACM
+	help
+	  Enable the command "acmconsole" for accessing u-boot console from a
+	  USB host through CDC ACM protocol.
+
 endmenu
 
 
diff --git a/cmd/Makefile b/cmd/Makefile
index ed36694..c116091 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -34,6 +34,7 @@  obj-$(CONFIG_CMD_BOOTMENU) += bootmenu.o
 obj-$(CONFIG_CMD_BOOTSTAGE) += bootstage.o
 obj-$(CONFIG_CMD_BOOTZ) += bootz.o
 obj-$(CONFIG_CMD_BOOTI) += booti.o
+ob-y += acmconsole.o
 obj-$(CONFIG_CMD_BTRFS) += btrfs.o
 obj-$(CONFIG_CMD_BUTTON) += button.o
 obj-$(CONFIG_CMD_CACHE) += cache.o
@@ -174,6 +175,7 @@  obj-$(CONFIG_CMD_FS_UUID) += fs_uuid.o
 obj-$(CONFIG_CMD_USB_MASS_STORAGE) += usb_mass_storage.o
 obj-$(CONFIG_CMD_USB_SDP) += usb_gadget_sdp.o
 obj-$(CONFIG_CMD_THOR_DOWNLOAD) += thordown.o
+obj-$(CONFIG_CMD_USB_ACM_CONSOLE) += acmconsole.o
 obj-$(CONFIG_CMD_XIMG) += ximg.o
 obj-$(CONFIG_CMD_YAFFS2) += yaffs2.o
 obj-$(CONFIG_CMD_SPL) += spl.o
diff --git a/cmd/acmconsole.c b/cmd/acmconsole.c
new file mode 100644
index 0000000..dac8136
--- /dev/null
+++ b/cmd/acmconsole.c
@@ -0,0 +1,50 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * acmconsole.c -- Console over USB CDC serial (ACM) function gadget function
+ *
+ * Copyright (c) 2021, Linaro Ltd <loic.poulain@linaro.org>
+ */
+
+#include <common.h>
+#include <console.h>
+#include <dfu.h>
+#include <g_dnl.h>
+#include <stdio_dev.h>
+#include <usb.h>
+#include <watchdog.h>
+
+int do_usb_acmconsole(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	int ret;
+
+	if (argc != 2)
+		return CMD_RET_USAGE;
+
+	if (!strcmp(argv[1], "start")) {
+		ret = g_dnl_register("usb_serial_acm");
+		if (ret)
+			return ret;
+
+		/* ACM function creates a stdio device name 'stdio_acm' */
+		console_assign(stdin, "stdio_acm");
+		console_assign(stdout, "stdio_acm");
+		console_assign(stderr, "stdio_acm");
+	} else if (!strcmp(argv[1], "stop")) {
+		/* default to serial (TODO: restore initial devices) */
+		console_assign(stdin, "serial");
+		console_assign(stdout, "serial");
+		console_assign(stderr, "serial");
+
+		g_dnl_unregister();
+		g_dnl_clear_detach();
+	} else {
+		return CMD_RET_USAGE;
+	}
+
+	return 0;
+}
+
+U_BOOT_CMD(acmconsole, CONFIG_SYS_MAXARGS, 1, do_usb_acmconsole,
+	   "Console over USB CDC ACM",
+	   "start - start console over USB CDC ACM gadget function\n" \
+	   "acmconsole stop - stop USB console");