Re: [RESEND PATCH v2 1/3] misc: fs_loader: Add function to get the chosen loader

[Ramon Fried <rfried.dev@gmail.com>]

On Fri, Apr 22, 2022 at 8:30 PM Sean Anderson <sean.anderson@seco.com> wrote:
>
> The fs_loader device is used to pull in settings via the chosen node.
> However, there was no library function for this, so arria10 was doing it
> explicitly. This function subsumes that, and uses ofnode_get_chosen_node
> instead of navigating the device tree directly. Because fs_loader pulls
> its config from the environment by default, it's fine to create a device
> with nothing backing it at all. Doing this allows enabling
> CONFIG_FS_LOADER without needing to modify the device tree.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
>  arch/arm/mach-k3/common.c         |  2 +-
>  arch/arm/mach-omap2/boot-common.c |  2 +-
>  drivers/fpga/socfpga_arria10.c    | 24 ++----------------------
>  drivers/misc/fs_loader.c          | 27 +++++++++++++++++++++++++++
>  include/fs_loader.h               | 12 ++++++++++++
>  5 files changed, 43 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> index b4b75f4e6c..ec236d5a2e 100644
> --- a/arch/arm/mach-k3/common.c
> +++ b/arch/arm/mach-k3/common.c
> @@ -181,7 +181,7 @@ int load_firmware(char *name_fw, char *name_loadaddr, u32 *loadaddr)
>         if (!*loadaddr)
>                 return 0;
>
> -       if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
> +       if (!get_fs_loader(&fsdev)) {
>                 size = request_firmware_into_buf(fsdev, name, (void *)*loadaddr,
>                                                  0, 0);
>         }
> diff --git a/arch/arm/mach-omap2/boot-common.c b/arch/arm/mach-omap2/boot-common.c
> index c463c96c74..88132b6572 100644
> --- a/arch/arm/mach-omap2/boot-common.c
> +++ b/arch/arm/mach-omap2/boot-common.c
> @@ -212,7 +212,7 @@ int load_firmware(char *name_fw, u32 *loadaddr)
>         if (!*loadaddr)
>                 return 0;
>
> -       if (!uclass_get_device(UCLASS_FS_FIRMWARE_LOADER, 0, &fsdev)) {
> +       if (!get_fs_loader(&fsdev)) {
>                 size = request_firmware_into_buf(fsdev, name_fw,
>                                                  (void *)*loadaddr, 0, 0);
>         }
> diff --git a/drivers/fpga/socfpga_arria10.c b/drivers/fpga/socfpga_arria10.c
> index 798e3a3f90..65bebd8997 100644
> --- a/drivers/fpga/socfpga_arria10.c
> +++ b/drivers/fpga/socfpga_arria10.c
> @@ -765,32 +765,12 @@ int socfpga_loadfs(fpga_fs_info *fpga_fsinfo, const void *buf, size_t bsize,
>         u32 phandle;
>
>         node = get_fpga_mgr_ofnode(ofnode_null());
> -
> -       if (ofnode_valid(node)) {
> -               phandle_p = ofnode_get_property(node, "firmware-loader", &size);
> -               if (!phandle_p) {
> -                       node = ofnode_path("/chosen");
> -                       if (!ofnode_valid(node)) {
> -                               debug("FPGA: /chosen node was not found.\n");
> -                               return -ENOENT;
> -                       }
> -
> -                       phandle_p = ofnode_get_property(node, "firmware-loader",
> -                                                      &size);
> -                       if (!phandle_p) {
> -                               debug("FPGA: firmware-loader property was not");
> -                               debug(" found.\n");
> -                               return -ENOENT;
> -                       }
> -               }
> -       } else {
> +       if (!ofnode_valid(node)) {
>                 debug("FPGA: FPGA manager node was not found.\n");
>                 return -ENOENT;
>         }
>
> -       phandle = fdt32_to_cpu(*phandle_p);
> -       ret = uclass_get_device_by_phandle_id(UCLASS_FS_FIRMWARE_LOADER,
> -                                            phandle, &dev);
> +       ret = get_fs_loader(&dev);
>         if (ret)
>                 return ret;
>
> diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c
> index 0139bd66ba..0018c930ec 100644
> --- a/drivers/misc/fs_loader.c
> +++ b/drivers/misc/fs_loader.c
> @@ -15,6 +15,8 @@
>  #include <fs_loader.h>
>  #include <log.h>
>  #include <asm/global_data.h>
> +#include <dm/device-internal.h>
> +#include <dm/root.h>
>  #include <linux/string.h>
>  #include <mapmem.h>
>  #include <malloc.h>
> @@ -293,6 +295,31 @@ U_BOOT_DRIVER(fs_loader) = {
>         .priv_auto      = sizeof(struct firmware),
>  };
>
> +static struct device_plat default_plat = { 0 };
> +
> +int get_fs_loader(struct udevice **dev)
> +{
> +       int ret;
> +       ofnode node = ofnode_get_chosen_node("firmware-loader");
> +
> +       if (ofnode_valid(node))
> +               return uclass_get_device_by_ofnode(UCLASS_FS_FIRMWARE_LOADER,
> +                                                  node, dev);
> +
> +       /* Try the first device if none was chosen */
> +       ret = uclass_first_device_err(UCLASS_FS_FIRMWARE_LOADER, dev);
> +       if (ret != -ENODEV)
> +               return ret;
> +
> +       /* Just create a new device */
> +       ret = device_bind(dm_root(), DM_DRIVER_GET(fs_loader), "default-loader",
> +                         &default_plat, ofnode_null(), dev);
> +       if (ret)
> +               return ret;
> +
> +       return device_probe(*dev);
> +}
> +
>  UCLASS_DRIVER(fs_loader) = {
>         .id             = UCLASS_FS_FIRMWARE_LOADER,
>         .name           = "fs-loader",
> diff --git a/include/fs_loader.h b/include/fs_loader.h
> index 8de7cb18dc..5eb5b7ab4a 100644
> --- a/include/fs_loader.h
> +++ b/include/fs_loader.h
> @@ -52,4 +52,16 @@ struct device_plat {
>  int request_firmware_into_buf(struct udevice *dev,
>                               const char *name,
>                               void *buf, size_t size, u32 offset);
> +
> +/**
> + * get_fs_loader() - Get the chosen filesystem loader
> + * @dev: Where to store the device
> + *
> + * This gets a filesystem loader device based on the value of
> + * /chosen/firmware-loader. If no such property exists, it returns a
> + * firmware loader which is configured by environmental variables.
> + *
> + * Return: 0 on success, negative value on error
> + */
> +int get_fs_loader(struct udevice **dev);
>  #endif
> --
> 2.35.1.1320.gc452695387.dirty
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>