On Tue, Jan 16, 2024 at 7:09 PM Heinrich Schuchardt <xypron.g...@gmx.de>
wrote:
> On 16.01.24 13:36, Mayuresh Chitale wrote:
> > Some platforms as described by EBBR specification may store images in
> > the FIRMWARE directory of the UEFI system partition(ESP). Add support
> > to boot from the EFI system partition if it is enabled for a platform.
> >
> > Signed-off-by: Mayuresh Chitale <mchit...@ventanamicro.com>
>
> Thank you for respinning this series.
>
> For future submissions it would be preferable to have the changes
> between the versions also in the individual patches.
>
Ok.
>
> > ---
> > common/spl/Kconfig | 8 +++++++
> > common/spl/spl_blk_fs.c | 51 ++++++++++++++++++++++++++++++-----------
> > common/spl/spl_fat.c | 34 +++++++++++++++++++++++----
> > 3 files changed, 75 insertions(+), 18 deletions(-)
> >
> > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > index cf7ffc9b112..48e4e43196a 100644
> > --- a/common/spl/Kconfig
> > +++ b/common/spl/Kconfig
> > @@ -1292,6 +1292,14 @@ config SPL_SATA_RAW_U_BOOT_SECTOR
> > Sector on the SATA disk to load U-Boot from, when the SATA disk
> is being
> > used in raw mode. Units: SATA disk sectors (1 sector = 512
> bytes).
> >
> > +config SPL_ESP_BOOT
> > + bool "Load next stage boot image from the UEFI system partition"
> > + default y if BOOT_DEFAULTS
> > + select SPL_PARTITION_TYPE_GUID
> > + help
> > + When enabled, first try to boot from the UEFI system partition as
> > + described in the Ch.4 of the EBBR specification.
> > +
> > config SPL_NVME
> > bool "NVM Express device support"
> > depends on BLK
> > diff --git a/common/spl/spl_blk_fs.c b/common/spl/spl_blk_fs.c
> > index 04eac6f306b..a2e8c2ce910 100644
> > --- a/common/spl/spl_blk_fs.c
> > +++ b/common/spl/spl_blk_fs.c
> > @@ -10,12 +10,15 @@
> > #include <spl_load.h>
> > #include <image.h>
> > #include <fs.h>
> > +#include <part.h>
> > #include <asm/cache.h>
> > #include <asm/io.h>
> >
> > struct blk_dev {
> > const char *ifname;
> > const char *filename;
> > + int devnum;
> > + int partnum;
> > char dev_part_str[8];
> > };
> >
> > @@ -44,6 +47,29 @@ static ulong spl_fit_read(struct spl_load_info *load,
> ulong file_offset,
> > return actlen;
> > }
> >
> > +static int spl_blk_file_size(struct blk_dev *dev, const char *filename,
> > + loff_t *filesize)
> > +{
> > + int ret;
> > +
> > + snprintf(dev->dev_part_str, sizeof(dev->dev_part_str) - 1, "%x:%x",
> > + dev->devnum, dev->partnum);
>
> Using log functions allows to provide more information if
> CONFIG_SPL_LOG=y otherwise they fall back to printf() and debug().
>
> > + debug("Loading file %s from %s %s\n", filename, dev->ifname,
> > + dev->dev_part_str);
>
> log_debug(
>
> > + ret = fs_set_blk_dev(dev->ifname, dev->dev_part_str, FS_TYPE_ANY);
> > + if (ret) {
> > + printf("spl: unable to set blk_dev %s %s. Err - %d\n",
>
> SPL binary size is limited on many systems. There is already a message
> telling that we are in SPL. I would suggest to abbreviate the message.
>
> log_err("Can't access %s %s\n", dev->ifname, dev->dev_part_str);
>
> > + dev->ifname, dev->dev_part_str, ret);
> > + return ret;
> > + }
> > +
> > + ret = fs_size(filename, filesize);
> > + if (ret)
> > + printf("spl: unable to get size, file: %s. Err - %d\n",
> > + filename, ret);
>
> log_err("File not found %s\n", filename);
>
> Moving to log functions and adjusting the messages can be done in a
> follow up patch. I don't want to stop this series.
>
> Ok.
> Reviewed-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
>
>
> > + return ret;
> > +}
> > +
> > int spl_blk_load_image(struct spl_image_info *spl_image,
> > struct spl_boot_device *bootdev,
> > enum uclass_id uclass_id, int devnum, int partnum)
> > @@ -53,7 +79,7 @@ int spl_blk_load_image(struct spl_image_info
> *spl_image,
> > loff_t filesize;
> > struct blk_dev dev;
> > struct spl_load_info load;
> > - int ret;
> > + int ret, part;
> >
> > blk_desc = blk_get_devnum_by_uclass_id(uclass_id, devnum);
> > if (!blk_desc) {
> > @@ -65,21 +91,18 @@ int spl_blk_load_image(struct spl_image_info
> *spl_image,
> >
> > dev.filename = filename;
> > dev.ifname = blk_get_uclass_name(uclass_id);
> > - snprintf(dev.dev_part_str, sizeof(dev.dev_part_str) - 1, "%x:%x",
> > - devnum, partnum);
> > - ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
> > - if (ret) {
> > - printf("spl: unable to set blk_dev %s %s. Err - %d\n",
> > - dev.ifname, dev.dev_part_str, ret);
> > - return ret;
> > + dev.devnum = devnum;
> > + dev.partnum = partnum;
> > + if (IS_ENABLED(CONFIG_SPL_ESP_BOOT)) {
> > + part = part_get_esp(blk_desc);
> > + if (part)
> > + dev.partnum = part;
> > + else
> > + return -ENODEV;
> > }
> > -
> > - ret = fs_size(filename, &filesize);
> > - if (ret) {
> > - printf("spl: unable to get file size: %s. Err - %d\n",
> > - filename, ret);
> > + ret = spl_blk_file_size(&dev, filename, &filesize);
> > + if (ret)
> > return ret;
> > - }
> >
> > load.read = spl_fit_read;
> > if (IS_ENABLED(CONFIG_SPL_FS_FAT_DMA_ALIGN))
> > diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> > index a52f9e178e6..8c426a3f3e7 100644
> > --- a/common/spl/spl_fat.c
> > +++ b/common/spl/spl_fat.c
> > @@ -60,10 +60,10 @@ static ulong spl_fit_read(struct spl_load_info
> *load, ulong file_offset,
> > return actread;
> > }
> >
> > -int spl_load_image_fat(struct spl_image_info *spl_image,
> > - struct spl_boot_device *bootdev,
> > - struct blk_desc *block_dev, int partition,
> > - const char *filename)
> > +int spl_load_image_fat_one(struct spl_image_info *spl_image,
> > + struct spl_boot_device *bootdev,
> > + struct blk_desc *block_dev, int partition,
> > + const char *filename)
> > {
> > int err;
> > loff_t size;
> > @@ -103,6 +103,32 @@ end:
> > return err;
> > }
> >
> > +int spl_load_image_fat(struct spl_image_info *spl_image,
> > + struct spl_boot_device *bootdev,
> > + struct blk_desc *block_dev, int partition,
> > + const char *filename)
> > +{
> > + int err, part;
> > +
> > + /*
> > + * First try to boot from EFI System partition. In case of failure,
> > + * fall back to the configured partition.
> > + */
> > + if (IS_ENABLED(CONFIG_SPL_ESP_BOOT)) {
> > + part = part_get_esp(block_dev);
> > + if (part) {
> > + err = spl_load_image_fat_one(spl_image, bootdev,
> > + block_dev, part,
> > + filename);
> > + if (!err)
> > + return err;
> > + }
> > + }
> > +
> > + return spl_load_image_fat_one(spl_image, bootdev, block_dev,
> > + partition, filename);
> > +}
> > +
> > #if CONFIG_IS_ENABLED(OS_BOOT)
> > int spl_load_image_fat_os(struct spl_image_info *spl_image,
> > struct spl_boot_device *bootdev,
>
>
