Re: [PATCH v2 00/28] arm: semihosting: Cleanups and new features

[Sean Anderson <sean.anderson@seco.com>]

Hi Andre,

On 3/11/22 1:22 PM, Andre Przywara wrote:
> On Thu, 10 Mar 2022 15:50:30 -0500
> Sean Anderson <sean.anderson@seco.com> wrote:
> 
> Hi Sean,
> 
> many thanks for doing this, I like that clean up.
> 
>> This cleans up the semihosting code and adds the following new features:
>> 
>> - hostfs support (like sandbox)
>> - support for being used as a SPL boot device
>> - serial device support
>> - falling back to normal drivers if semihosting is disabled
>> 
>> The main device affected by these changes is vexpress64, so I'd appreciate
>> if Andre (or anyone else) could try booting.
> 
> So I did this. Apart from the missing return in 26/28, as you figured
> yourself, the bootcmd_smh definition is missing the (dummy) device number,
> so it should read:
> 	"if load hostfs 0 ${boot_addr_r} ${boot_name}; then" ....
> 
> With that (and the other three occasions) fixed the automatic boot worked
> on the v8A model. The device number seems to be missing from some
> documentation patches as well.

Ah, looks like I forgot about this parameter. Will update.

> Also: I couldn't get the semihosting serial to work, it didn't show up in
> coninfo, so I couldn't switch with "setenv stdout". I enabled
> SEMIHOSTING_FALLBACK and SEMIHOSTING_SERIAL, I am surely missing something?

The problem is that U-Boot only probes /chosen/stdout-path by default.
You can change this by enabling CONFIG_SERIAL_PROBE_ALL. Then, U-Boot
will use CONFIG_CONS_INDEX, which defaults to 1 (aka seq 0). Since we
use U_BOOT_DRVINFO to create the semihosting serial, it will tend to be
before any other serials, so we will be seq 0 (and get selected).

However, it doesn't seem like SYS_READC works in QEMU for whatever
reason. You can apply the below patch to use open/read instead:

---
diff --git a/drivers/serial/serial_semihosting.c b/drivers/serial/serial_semihosting.c
index 049dca5428..b182f5ab93 100644
--- a/drivers/serial/serial_semihosting.c
+++ b/drivers/serial/serial_semihosting.c
@@ -14,13 +14,18 @@
  * @outfd: stdout file descriptor (or error)
  */
 struct smh_serial_priv {
+       int infd;
        int outfd;
 };
 
 #if CONFIG_IS_ENABLED(DM_SERIAL)
 static int smh_serial_getc(struct udevice *dev)
 {
-       return smh_getc();
+       char ch = 0;
+       struct smh_serial_priv *priv = dev_get_priv(dev);
+
+       smh_read(priv->infd, &ch, sizeof(ch));
+       return ch;
 }
 
 static int smh_serial_putc(struct udevice *dev, const char ch)
@@ -77,6 +82,7 @@ static int smh_serial_probe(struct udevice *dev)
 {
        struct smh_serial_priv *priv = dev_get_priv(dev);
 
+       priv->infd = smh_open(":tt", MODE_READ);
        priv->outfd = smh_open(":tt", MODE_WRITE);
        return 0;
 }
---

> And btw.: QEMU supports semihosting, and I have used that in the past to
> easily load files from my host into U-Boot:
> 
> $ make qemu_arm_defconfig
> $ /src/linux/scripts/config --enable CONFIG_SEMIHOSTING
> $ make
> $ qemu-system-arm -M virt -cpu cortex-a15 -m 256M -semihosting -bios
>   u-boot.bin -nographic			OR
> $ qemu-system-aarch64 -M virt -cpu cortex-a57 -m 256M -semihosting -bios
>   u-boot.bin -nographic

Thanks for suggesting the command lines. Though arm32 will not support
fallback (which should have been added to Kconfig).

> did the trick for me.
> Please note that QEMU does not seem to implement ISERROR:
> =======
> qemu: Unsupported SemiHosting SWI 0x08
> =======
> Might be relatively easy to fix there, haven't checked, but can you use
> something else for the detection?

Looks like it was added in commit 767ba049b8 ("semihosting: Implement
SYS_ISERROR"). I think SYS_ERRNO will work fine instead.

> I am in the process of going through the patches more thoroughly, and will
> reply next week.

Thanks.

--Sean

> 
> Thanks,
> Andre
> 
>> These changes are motivated by bringup for ls1046a. When forcing JTAG
>> boot, this device disables most communication peripherals, including
>> serial and ethernet devices. This appears to be fixed in later
>> generation devices, but we are stuck with it for now. Semihosting
>> provides an easy way to run a few console commands.
>> 
>> The patches in this series are organized as follows:
>> 
>> 0-4: rST conversions and other documentation updates
>> 5-9: Semihosting cleanups
>> 10-14: Filesystem support (including SPL boot device)
>> 15-16: Serial support
>> 16: Documentation update
>> 17: JTAG boot support for LS1046A
>> 19-25: Semihosting fallback
>> 26-28: DM puts support
>> 
>> The last two groups of patches are "bonus;" the first 17 patches stand
>> on their own. The last two groups could be broken out as separate
>> series, but I have kept them in this one to help with my sanity (and not
>> have to deal with too many outstanding series).
>> 
>> This series depends on [1]. Patch 17 depends on [2].
>> 
>> [1] https://lore.kernel.org/u-boot/CACRpkdZ+9fmNjC_mvrbPa9-iuTQVd8UkJ7Zpe7cL0c5vZygsVw@mail.gmail.com/T/
>> [2] https://lore.kernel.org/u-boot/20220222183840.1355337-2-sean.anderson@seco.com/
>> 
>> Changes in v2:
>> - Document debug uart
>> - Make CONFIG_SPL_SEMIHOSTING depend on SPL
>> - Compile arch/arm/lib/semihosting.o in SPL
>> - Rebase on Andre's series
>> - Fix typos in commit message
>> - Fix baud numbers being off by 10
>> - Rename non-DM driver struct to match format of other drivers
>> - Add migration instructions for smhload
>> - Add semihosting fallback implementation
>> - Add implementation of puts for DM
>> 
>> Sean Anderson (28):
>>   doc: Convert semihosting readme to rST
>>   nxp: ls1046ardb: Convert README to rST
>>   doc: ls1046ardb: Expand boot mode section
>>   doc: ls1046ardb: Document debug uart
>>   arm: smh: Add semihosting entry to MAINTAINERS
>>   arm: smh: Export semihosting functions
>>   arm: smh: Use numeric modes for smh_open
>>   arm: smh: Return errno on error
>>   arm: smh: Document functions in header
>>   arm: smh: Add some file manipulation commands
>>   spl: Add semihosting boot method
>>   fs: Add semihosting filesystem
>>   cmd: fdt: Use start/size for chosen instead of start/end
>>   arm: smh: Remove smhload command
>>   arm: smh: Add some functions for working with the host console
>>   serial: Add semihosting driver
>>   doc: smh: Update semihosting documentation
>>   ls1046ardb: Add support for JTAG boot
>>   arm64: Save esr in pt_regs
>>   arm64: Save spsr in pt_regs
>>   arm64: Import some ESR and SPSR defines from Linux
>>   arm: smh: Add option to detect semihosting
>>   arm: Catch non-emulated semihosting calls
>>   serial: smh: Initialize serial only if semihosting is enabled
>>   arm64: ls1046a: Support semihosting fallback
>>   serial: dm: Add support for puts
>>   serial: sandbox: Implement puts
>>   serial: smh: Implement puts for DM
>> 
>>  MAINTAINERS                             |   5 +
>>  arch/arm/Kconfig                        |  47 +++-
>>  arch/arm/cpu/armv8/exceptions.S         |   9 +-
>>  arch/arm/cpu/armv8/fsl-layerscape/spl.c |   3 +
>>  arch/arm/include/asm/esr.h              | 343 ++++++++++++++++++++++++
>>  arch/arm/include/asm/proc-armv/ptrace.h |  75 ++++++
>>  arch/arm/include/asm/spl.h              |   1 +
>>  arch/arm/include/asm/u-boot-arm.h       |   7 +-
>>  arch/arm/lib/Makefile                   |   2 +-
>>  arch/arm/lib/interrupts_64.c            |  80 ++++--
>>  arch/arm/lib/semihosting.c              | 230 ++++++++--------
>>  arch/arm/mach-imx/imx8m/soc.c           |   4 +-
>>  board/freescale/ls1046ardb/MAINTAINERS  |   1 +
>>  board/freescale/ls1046ardb/README       |  76 ------
>>  board/freescale/ls1046ardb/ls1046ardb.c |  11 +
>>  cmd/fdt.c                               |   6 +-
>>  common/spl/Makefile                     |   1 +
>>  common/spl/spl_semihosting.c            |  71 +++++
>>  disk/part.c                             |   4 +-
>>  doc/README.semihosting                  |  38 ---
>>  doc/board/nxp/index.rst                 |   1 +
>>  doc/board/nxp/ls1046ardb.rst            | 191 +++++++++++++
>>  doc/usage/index.rst                     |   1 +
>>  doc/usage/semihosting.rst               |  73 +++++
>>  drivers/serial/Kconfig                  |  22 ++
>>  drivers/serial/Makefile                 |   1 +
>>  drivers/serial/sandbox.c                |  21 +-
>>  drivers/serial/serial-uclass.c          |  27 +-
>>  drivers/serial/serial.c                 |   2 +
>>  drivers/serial/serial_semihosting.c     | 168 ++++++++++++
>>  fs/Makefile                             |   1 +
>>  fs/fs.c                                 |  20 ++
>>  fs/semihostingfs.c                      | 115 ++++++++
>>  include/configs/ls1046ardb.h            |   2 +
>>  include/configs/vexpress_aemv8.h        |  10 +-
>>  include/fs.h                            |   1 +
>>  include/semihosting.h                   | 149 ++++++++++
>>  include/semihostingfs.h                 |  21 ++
>>  include/serial.h                        |  19 ++
>>  39 files changed, 1581 insertions(+), 278 deletions(-)
>>  create mode 100644 arch/arm/include/asm/esr.h
>>  delete mode 100644 board/freescale/ls1046ardb/README
>>  create mode 100644 common/spl/spl_semihosting.c
>>  delete mode 100644 doc/README.semihosting
>>  create mode 100644 doc/board/nxp/ls1046ardb.rst
>>  create mode 100644 doc/usage/semihosting.rst
>>  create mode 100644 drivers/serial/serial_semihosting.c
>>  create mode 100644 fs/semihostingfs.c
>>  create mode 100644 include/semihosting.h
>>  create mode 100644 include/semihostingfs.h
>> 
>