On Wed, Jan 14, 2026 at 10:16 PM Quentin Schulz <[email protected]> wrote: > > Hi Alexey, > > On 1/8/26 6:49 PM, Alexey Charkov wrote: > > Add minimal infrastructure to build SPL images with support for UFS > > storage devices. This also pulls in SCSI support and charset functions, > > which are dependencies of the UFS code. > > > > With this, only a fixed offset is supported for loading the next image, > > which should be specified in CONFIG_SPL_UFS_RAW_U_BOOT_SECTOR as the > > number of 4096-byte sectors into the UFS block device. > > > > Signed-off-by: Alexey Charkov <[email protected]> > > --- > > MAINTAINERS | 1 + > > arch/arm/include/asm/spl.h | 1 + > > common/spl/Kconfig | 29 +++++++++++++++++++++++++++ > > common/spl/Makefile | 1 + > > common/spl/spl_ufs.c | 49 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > drivers/Makefile | 1 + > > drivers/scsi/Makefile | 3 +++ > > lib/Makefile | 1 + > > 8 files changed, 86 insertions(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 27ce73d83f48..ca31e57f018a 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -1861,6 +1861,7 @@ M: Neil Armstrong <[email protected]> > > M: Bhupesh Sharma <[email protected]> > > M: Neha Malcom Francis <[email protected]> > > S: Maintained > > +F: common/spl/spl_ufs.c > > F: drivers/ufs/ > > > > UPL > > diff --git a/arch/arm/include/asm/spl.h b/arch/arm/include/asm/spl.h > > index ee79a19c05c9..dd462ea6ad82 100644 > > --- a/arch/arm/include/asm/spl.h > > +++ b/arch/arm/include/asm/spl.h > > @@ -30,6 +30,7 @@ enum { > > BOOT_DEVICE_XIP, > > BOOT_DEVICE_BOOTROM, > > BOOT_DEVICE_SMH, > > + BOOT_DEVICE_UFS, > > BOOT_DEVICE_NONE > > }; > > #endif > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig > > index 4f4119f5806c..6a82260dd010 100644 > > --- a/common/spl/Kconfig > > +++ b/common/spl/Kconfig > > @@ -1612,6 +1612,35 @@ config SPL_THERMAL > > automatic power-off when the temperature gets too high or low. Other > > devices may be discrete but connected on a suitable bus. > > > > +config SPL_UFS_SUPPORT > > + bool "Support loading from UFS" > > + select SPL_LOAD_BLOCK > > Doesn;t this depend on CONFIG_SCSI as well? Like it does nothing without > it (does it even compile?), see changes you made to drivers/scsi/Makefile.
You're right. It's currently possible to disable CONFIG_UFS and CONFIG_SCSI while keeping this enabled, and then this fails to compile. I wonder how e.g. CONFIG_SPL_SATA solves this, as it doesn't pull in SCSI either... Might be something to look into later. > > + help > > + Enable support for UFS in SPL. This allows > > + use of UFS devices such as hard drives and flash drivers for > > + loading U-Boot. > > + > > +config SPL_UFS_RAW_U_BOOT_DEVNUM > > + int "SCSI device number of the UFS device to load U-Boot from" > > + depends on SPL_UFS_SUPPORT > > + default 0 > > + help > > + UFS devices are usually configured with multiple LUNs, which present > > + themselves as sequentially numbered SCSI devices. Usually one would > > + get a default LUN 0 taking up most of the space on the device, with > > + a number of smaller LUNs following it. This option controls which of > > + them the SPL will attempt to load U-Boot from. Note that this is the > > + SCSI device number, which might differ from the UFS LUN if you have > > + multiple SCSI devices attached and recognized by the SPL. > > + > > I'm not verse in SCSI... Is the device number guaranteed to be stable > across reboots? Is it probe order dependent? If that's the case, this is > not a useful symbol and we should be trying to figure out a way to make > it stable (via aliases for example, or using another mechanism). It is stable across reboots and it follows the same sequence as LUNs, but I suspect that if multiple SCSI drives are present (e.g. a UFS module and some SATA drive) then their numbering might change if e.g. one plugs/unplugs the SATA drive. Can't check it unfortunately, as I don't have a device which would support more than one. There is a dedicated attribute for the LUN within SCSI structures (and also blk_desc for that matter), but to get to it one still needs to provide a device number first. So the alternative seems to be to iterate through all available device numbers, looking for a particular SCSI target and LUN (and belonging to UCLASS_UFS). That felt a bit over engineered for the use case at hand, and even then one can still think of a case where the logic breaks - e.g. if more than one UFS device is present in the system, and their SCSI target numbers change based on probe order. spl_sata.c for instance hardcodes devnum = 0, meaning that a system with two drives would likely boot unpredictably > > +config SPL_UFS_RAW_U_BOOT_SECTOR > > + hex "Address on the UFS to load U-Boot from" > > + depends on SPL_UFS_SUPPORT > > + default 0x800 if ARCH_ROCKCHIP > > + help > > + Address on the block device to load U-Boot from, > > s/,/./ Ack > > + Units: UFS sectors (1 sector = 4096 bytes). > > + > > config SPL_WATCHDOG > > bool "Support watchdog drivers" > > imply SPL_WDT if !HW_WATCHDOG > > diff --git a/common/spl/Makefile b/common/spl/Makefile > > index 4c9482bd3096..e18f3cf09484 100644 > > --- a/common/spl/Makefile > > +++ b/common/spl/Makefile > > @@ -37,6 +37,7 @@ obj-$(CONFIG_$(PHASE_)DFU) += spl_dfu.o > > obj-$(CONFIG_$(PHASE_)SPI_LOAD) += spl_spi.o > > obj-$(CONFIG_$(PHASE_)RAM_SUPPORT) += spl_ram.o > > obj-$(CONFIG_$(PHASE_)USB_SDP_SUPPORT) += spl_sdp.o > > +obj-$(CONFIG_$(PHASE_)UFS_SUPPORT) += spl_ufs.o > > endif > > > > obj-$(CONFIG_$(PHASE_)UPL) += spl_upl.o > > diff --git a/common/spl/spl_ufs.c b/common/spl/spl_ufs.c > > new file mode 100644 > > index 000000000000..6b035db13c41 > > --- /dev/null > > +++ b/common/spl/spl_ufs.c > > @@ -0,0 +1,49 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * (C) Copyright 2025 Alexey Charkov <[email protected]> > > + */ > > + > > +#include <spl.h> > > +#include <spl_load.h> > > +#include <scsi.h> > > +#include <errno.h> > > +#include <image.h> > > +#include <linux/compiler.h> > > +#include <log.h> > > + > > +static ulong spl_ufs_load_read(struct spl_load_info *load, ulong off, > > ulong size, void *buf) > > +{ > > + struct blk_desc *bd = load->priv; > > + lbaint_t sector = off >> bd->log2blksz; > > + lbaint_t count = size >> bd->log2blksz; > > + > > + return blk_dread(bd, sector, count, buf) << bd->log2blksz; > > +} > > + > > +static int spl_ufs_load_image(struct spl_image_info *spl_image, > > + struct spl_boot_device *bootdev) > > +{ > > + int err = -ENOSYS; > > + struct blk_desc *bd; > > + struct spl_load_info load; > > + int devnum = CONFIG_SPL_UFS_RAW_U_BOOT_DEVNUM; > > + unsigned long sector = CONFIG_SPL_UFS_RAW_U_BOOT_SECTOR; > > Reverse christmas tree here usually (so longest line at the top). Ack > > + > > + /* try to recognize storage devices immediately */ > > + scsi_scan(false); > > + bd = blk_get_devnum_by_uclass_id(UCLASS_SCSI, devnum); > > + if (!bd) > > + return -ENODEV; > > + > > + spl_load_init(&load, spl_ufs_load_read, bd, bd->blksz); > > + err = spl_load(spl_image, bootdev, &load, 0, sector << bd->log2blksz); > > + if (err) { > > + puts("spl_ufs_load_image: ufs block read error\n"); > > + log_debug("(error=%d)\n", err); > > Why both a print and a log_debug? If debug is compiled out along with all the string formatting stuff, the puts still provides a useful error message with reduced functionality. I see much of the code under common/spl uses puts for error strings, and log_* for everything else. Thanks a lot, Alexey
