Hi Neil, Thanks for your review!
On Thu, Dec 11, 2025 at 10:36 AM Neil Armstrong <[email protected]> wrote: > > Hi, > > On 12/10/25 21:10, 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 | 16 +++++++++++++++ > > common/spl/Makefile | 1 + > > common/spl/spl_ufs.c | 49 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > drivers/Makefile | 1 + > > drivers/scsi/Makefile | 3 +++ > > lib/Makefile | 1 + > > 8 files changed, 73 insertions(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index c9853f409920..c58805a724b9 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -1860,6 +1860,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 a43271671648..aa198ee44a3f 100644 > > --- a/common/spl/Kconfig > > +++ b/common/spl/Kconfig > > @@ -1608,6 +1608,22 @@ 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 > > + 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_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， > > + Units: UFS sectors (1 sector = 4096 bytes). > > The UFS are usually configured with multiple LUNs, so I would expect to at > least > have the LUN parameter in case we use the sector address to load the payload. Indeed. Rockchip's tools don't offer a direct way to access non-default LUNs, which makes it a bit tricky to test booting from anything other than LUN 0 (apart from using a fully booted system with its full-blown drivers to write to other LUNs). I'll test and add the extra Kconfig selector for the LUN. > > 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..3a5fcd4ed7a8 > > --- /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 h_spl_load_read(struct spl_load_info *load, ulong off, > > + ulong size, void *buf) > > What a weird function name, please rename to spl_ufs_load_read Ack. I grabbed it wholesale from spl_mmc.c - but I agree that it's not exactly descriptive :) Will rename in v2. Best regards, Alexey
