From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Thu, 12 Jul 2018 07:19:02 +0000 Subject: [U-Boot] [PATCH v4 6/6] common: Generic loader for file system In-Reply-To: References: <1530865683-23207-1-git-send-email-tien.fong.chee@intel.com> Message-ID: <1531379941.9615.16.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Wed, 2018-07-11 at 14:13 -0600, Simon Glass wrote: > Hi Tien, > > On 6 July 2018 at 02:28,   wrote: > > > > From: Tien Fong Chee > > > > This is file system generic loader which can be used to load > > the file image from the storage into target such as memory. > > The consumer driver would then use this loader to program whatever, > > ie. the FPGA device. > > > > Signed-off-by: Tien Fong Chee > > --- > >  drivers/misc/Kconfig     |  10 ++ > >  drivers/misc/Makefile    |   1 + > >  drivers/misc/fs_loader.c | 295 > > +++++++++++++++++++++++++++++++++++++++++++++++ > >  include/dm/uclass-id.h   |   1 + > >  include/fs_loader.h      |  79 +++++++++++++ > >  5 files changed, 386 insertions(+) > >  create mode 100644 drivers/misc/fs_loader.c > >  create mode 100644 include/fs_loader.h > > > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > index 17b3a80..4163b4f 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -277,4 +277,14 @@ config GDSYS_RXAUI_CTRL > >         depends on MISC > >         help > >           Support gdsys FPGA's RXAUI control. > > + > > +config FS_LOADER > > +       bool "Enable loader driver for file system" > > +       help > > +         This is file system generic loader which can be used to > > load > > +         the file image from the storage into target such as > > memory. > *a* file image? Okay. > > > > > + > > +         The consumer driver would then use this loader to program > > whatever, > > +         ie. the FPGA device. > e.g. an FPGA device Okay. > > > > > + > >  endmenu > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > > index 4ce9d21..67a36f8 100644 > > --- a/drivers/misc/Makefile > > +++ b/drivers/misc/Makefile > > @@ -54,3 +54,4 @@ obj-$(CONFIG_STM32_RCC) += stm32_rcc.o > >  obj-$(CONFIG_STM32MP_FUSE) += stm32mp_fuse.o > >  obj-$(CONFIG_SYS_DPAA_QBMAN) += fsl_portals.o > >  obj-$(CONFIG_GDSYS_RXAUI_CTRL) += gdsys_rxaui_ctrl.o > > +obj-$(CONFIG_FS_LOADER) += fs_loader.o > > diff --git a/drivers/misc/fs_loader.c b/drivers/misc/fs_loader.c > > new file mode 100644 > > index 0000000..5fe642b > > --- /dev/null > > +++ b/drivers/misc/fs_loader.c > > @@ -0,0 +1,295 @@ > > +/* > > + * Copyright (C) 2018 Intel Corporation > > + * > > + * SPDX-License-Identifier:    GPL-2.0 > > + */ > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +DECLARE_GLOBAL_DATA_PTR; > > + > > +struct firmware_priv { > > +       const char *name;       /* Filename */ > > +       u32 offset;             /* Offset of reading a file */ > I don't understand that comment. Is it the offset within the file to > read from? Please can you expand it a bit? Sure. This is offset within the file, can be used in limited memory buffer, chunk by chunk transfer from device storage to memory. > > > > > +}; > > + > > +#ifdef CONFIG_CMD_UBIFS > > +static int mount_ubifs(char *mtdpart, char *ubivol) > > +{ > > +       int ret = ubi_part(mtdpart, NULL); > > + > > +       if (ret) { > > +               debug("Cannot find mtd partition %s\n", mtdpart); > > +               return ret; > > +       } > > + > > +       return cmd_ubifs_mount(ubivol); > > +} > > + > > +static int umount_ubifs(void) > > +{ > > +       return cmd_ubifs_umount(); > > +} > > +#else > > +static int mount_ubifs(char *mtdpart, char *ubivol) > > +{ > > +       debug("Error: Cannot load image: no UBIFS support\n"); > blank line here Okay. > > > > > +       return -ENOSYS; > > +} > > +#endif > > + > > +static int select_fs_dev(struct device_platdata *plat) > > +{ > > +       int ret; > > + > > +       if (plat->phandlepart.phandle) { > > +               ofnode node; > > + > > +               node = ofnode_get_by_phandle(plat- > > >phandlepart.phandle); > > + > > +               int of_offset = ofnode_to_offset(node); > > + > > +               struct udevice *dev; > > + > > +               ret = device_get_global_by_of_offset(of_offset, > > &dev); > Can you please create device_get_global_by_ofnode() and use that > instead? We should not use offsets in new code. okay. > > > > > +               if (!ret) { > > +                       struct blk_desc *desc = > > blk_get_by_device(dev); > > +                       if (desc) { > > +                               ret = > > fs_set_blk_dev_with_part(desc, > > +                                       plat- > > >phandlepart.partition); > > +                       } else { > > +                               debug("%s: No device found\n", > > __func__); > > +                               return -ENODEV; > > +                       } > > +               } > > +       } else if (plat->mtdpart && plat->ubivol) { > > +               ret = mount_ubifs(plat->mtdpart, plat->ubivol); > > +               if (ret) > > +                       return ret; > > + > > +               ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS); > > +       } else { > > +               debug("Error: unsupported storage device.\n"); > > +               return -ENODEV; > > +       } > > + > > +       if (ret) > > +               debug("Error: could not access storage.\n"); > > + > > +       return ret; > > +} > > + > > +/** > > + * _request_firmware_prepare - Prepare firmware struct. > > + * > > + * @name: Name of firmware file. > > + * @dbuf: Address of buffer to load firmware into. > > + * @size: Size of buffer. > > + * @offset: Offset of a file for start reading into buffer. > > + * @firmwarep: Pointer to pointer to firmware image. > > + * > > + * Return: Negative value if fail, 0 for successful. > > + */ > > +static int _request_firmware_prepare(const char *name, void *dbuf, > > +                                   size_t size, u32 offset, > > +                                   struct firmware **firmwarep) > > +{ > struct firmware *firmware = *firmwarep; > > and use that instead of the clumsy *firmwarep in this function Okay. > > > > > +       if (!name || name[0] == '\0') > > +               return -EINVAL; > > + > > +       /* No memory allocation is required if *firmwarep is > > allocated */ > > +       if (!(*firmwarep)) { > Can this not be allocated automatically by the uclass or driver? Okay, i will try it out. > > > > > +               (*firmwarep) = calloc(1, sizeof(struct firmware)); > > +               if (!(*firmwarep)) > > +                       return -ENOMEM; > > + > > +               (*firmwarep)->priv = calloc(1, sizeof(struct > > firmware_priv)); > > +               if (!(*firmwarep)->priv) { > > +                       free(*firmwarep); > > +                       return -ENOMEM; > > +               } > > +       } else if (!(*firmwarep)->priv) { > Can you not use the driver's priv_auto_alloc_size to allocate this? > Or > the uclass? Okay, i will try it out. > > > > > +               (*firmwarep)->priv = calloc(1, sizeof(struct > > firmware_priv)); > > +               if (!(*firmwarep)->priv) { > > +                       free(*firmwarep); > > +                       return -ENOMEM; > > +               } > > +       } > > + > > +       ((struct firmware_priv *)((*firmwarep)->priv))->name = > > name; > Again this needs a local variable. Why? > > > > > +       ((struct firmware_priv *)((*firmwarep)->priv))->offset = > > offset; > > +       (*firmwarep)->data = dbuf; > > +       (*firmwarep)->size = size; > > + > > +       return 0; > > +} > > + > > +/** > > + * release_firmware - Release the resource associated with a > > firmware image > > + * @firmware: Firmware resource to release > > + */ > > +void release_firmware(struct firmware *firmware) > > +{ > > +       if (firmware) { > > +               if (firmware->priv) { > > +                       free(firmware->priv); > > +                       firmware->priv = NULL; > > +               } > > +               free(firmware); > > +       } > > +} > > + > > +/** > > + * fw_get_filesystem_firmware - load firmware into an allocated > > buffer. > > + * @plat: Platform data such as storage and partition firmware > > loading from. > > + * @firmware: pointer to firmware image. > > + * > > + * Return: Size of total read, negative value when error. > > + */ > > +static int fw_get_filesystem_firmware(struct device_platdata > > *plat, > > +                                    struct firmware *firmware) > > +{ > > +       struct firmware_priv *fw_priv = NULL; > > +       loff_t actread; > > +       char *storage_interface, *dev_part, *ubi_mtdpart, > > *ubi_volume; > > +       int ret; > > + > > +       storage_interface = env_get("storage_interface"); > > +       dev_part = env_get("fw_dev_part"); > > +       ubi_mtdpart = env_get("fw_ubi_mtdpart"); > > +       ubi_volume = env_get("fw_ubi_volume"); > > + > > +       if (storage_interface && dev_part) { > > +               ret = fs_set_blk_dev(storage_interface, dev_part, > > FS_TYPE_ANY); > > +       } else if (storage_interface && ubi_mtdpart && ubi_volume) > > { > > +               ret = mount_ubifs(ubi_mtdpart, ubi_volume); > > +               if (ret) > > +                       return ret; > > + > > +               if (!strcmp("ubi", storage_interface)) > > +                       ret = fs_set_blk_dev(storage_interface, > > NULL, > > +                               FS_TYPE_UBIFS); > > +               else > > +                       ret = -ENODEV; > > +       } else { > > +               ret = select_fs_dev(plat); > > +       } > > + > > +       if (ret) > > +               goto out; > > + > > +       fw_priv = firmware->priv; > > + > > +       ret = fs_read(fw_priv->name, (ulong)map_to_sysmem(firmware- > > >data), > > +                       fw_priv->offset, firmware->size, &actread); > > +       if (ret) { > > +               debug("Error: %d Failed to read %s from flash %lld > > != %d.\n", > > +                     ret, fw_priv->name, actread, firmware->size); > > +       } else { > > +               ret = actread; > > +       } > > + > > +out: > > +#ifdef CONFIG_CMD_UBIFS > > +       umount_ubifs(); > > +#endif > debug() here to report failure Okay. > > > > > +       return ret; > > +} > > + > > +/** > > + * request_firmware_into_buf - Load firmware into a previously > > allocated buffer. > > + * @plat: Platform data such as storage and partition firmware > > loading from. > > + * @name: Name of firmware file. > > + * @buf: Address of buffer to load firmware into. > > + * @size: Size of buffer. > > + * @offset: Offset of a file for start reading into buffer. > > + * @firmwarep: Pointer to firmware image. > > + * > > + * The firmware is loaded directly into the buffer pointed to by > > @buf and > > + * the @firmwarep data member is pointed at @buf. > > + * > > + * Return: Size of total read, negative value when error. > > + */ > > +int request_firmware_into_buf(struct device_platdata *plat, > > +                             const char *name, > > +                             void *buf, size_t size, u32 offset, > > +                             struct firmware **firmwarep) > > +{ > > +       int ret; > > + > > +       if (!plat) > > +               return -EINVAL; > > + > > +       ret = _request_firmware_prepare(name, buf, size, offset, > > firmwarep); > > +       if (ret < 0) /* error */ > > +               return ret; > > + > > +       ret = fw_get_filesystem_firmware(plat, *firmwarep); > > + > > +       return ret; > > +} > > + > > +static int fs_loader_ofdata_to_platdata(struct udevice *dev) > > +{ > > +       const char *fs_loader_path; > > +       u32 phandlepart[2]; > > + > > +       fs_loader_path = ofnode_get_chosen_prop("firmware-loader"); > > + > > +       if (fs_loader_path) { > > +               ofnode fs_loader_node; > > + > > +               fs_loader_node = ofnode_path(fs_loader_path); > > +               if (ofnode_valid(fs_loader_node)) { > > +                       struct device_platdata *plat; > > +                       plat = dev->platdata; > > + > > +                       if (!ofnode_read_u32_array(fs_loader_node, > > +                                                 "phandlepart", > > +                                                 phandlepart, 2)) > > { > > +                               plat->phandlepart.phandle = > > phandlepart[0]; > > +                               plat->phandlepart.partition = > > phandlepart[1]; > > +                       } > > + > > +                       plat->mtdpart = (char *)ofnode_read_string( > > +                                        fs_loader_node, > > "mtdpart"); > > + > > +                       plat->ubivol = (char *)ofnode_read_string( > > +                                        fs_loader_node, "ubivol"); > > +               } > > +       } > > + > > +       return 0; > > +} > > + > > +static int fs_loader_probe(struct udevice *dev) > > +{ > > +       return 0; > > +}; > > + > > +static const struct udevice_id fs_loader_ids[] = { > > +       { .compatible = "u-boot,fs-loader"}, > > +       { } > > +}; > > + > > +U_BOOT_DRIVER(fs_loader) = { > > +       .name                   = "fs-loader", > > +       .id                     = UCLASS_FS_FIRMWARE_LOADER, > > +       .of_match               = fs_loader_ids, > > +       .probe                  = fs_loader_probe, > > +       .ofdata_to_platdata     = fs_loader_ofdata_to_platdata, > > +       .platdata_auto_alloc_size       = sizeof(struct > > device_platdata), > > +}; > > + > > +UCLASS_DRIVER(fs_loader) = { > > +       .id             = UCLASS_FS_FIRMWARE_LOADER, > > +       .name           = "fs-loader", > > +}; > > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h > > index d7f9df3..39e88ac 100644 > > --- a/include/dm/uclass-id.h > > +++ b/include/dm/uclass-id.h > > @@ -36,6 +36,7 @@ enum uclass_id { > >         UCLASS_DMA,             /* Direct Memory Access */ > >         UCLASS_EFI,             /* EFI managed devices */ > >         UCLASS_ETH,             /* Ethernet device */ > > +       UCLASS_FS_FIRMWARE_LOADER,              /* Generic loader > > */ > >         UCLASS_GPIO,            /* Bank of general-purpose I/O pins > > */ > >         UCLASS_FIRMWARE,        /* Firmware */ > >         UCLASS_I2C,             /* I2C bus */ > > diff --git a/include/fs_loader.h b/include/fs_loader.h > > new file mode 100644 > > index 0000000..0be4f17 > > --- /dev/null > > +++ b/include/fs_loader.h > > @@ -0,0 +1,79 @@ > > +/* > > + * Copyright (C) 2018 Intel Corporation > > + * > > + * SPDX-License-Identifier:    GPL-2.0 > > + */ > > +#ifndef _FS_LOADER_H_ > > +#define _FS_LOADER_H_ > > + > > +#include > > + > > +/** > > + * struct firmware - A place for storing firmware and its > > attribute data. > > + * > > + * This holds information about a firmware and its content. > > + * > > + * @size: Size of a file > > + * @data: Buffer for file > > + * @priv: Firmware loader private fields > > + */ > > +struct firmware { > > +       size_t size; > > +       const u8 *data; > > +       void *priv; > > +}; > Let's try to get rid of priv by using driver model. Okay. > > > > > + > > +/** > > + * struct phandle_part - A place for storing phandle of node and > > its partition > > + * > > + * This holds information about a phandle of the block device, and > > its > > + * partition where the firmware would be loaded from. > > + * > > + * @phandle: Phandle of storage device node > > + * @partition: Partition of block device > > + */ > > +struct phandle_part { > struct device_part ? > > A phandle is a device-tree concept. Okay. > > > > > +       u32 phandle; > > +       u32 partition; > > +}; > > + > > +/** > > + * struct phandle_part - A place for storing all supported storage > > devices > > + * > > + * This holds information about all supported storage devices for > > driver use. > > + * > > + * @phandlepart: Attribute data for block device. > nit: please drop your periods at the end of these lines - they are > not > sentences. > Okay. > > > > + * @mtdpart: MTD partition for ubi partition. > > + * @ubivol: UBI volume-name for ubifsmount. > > + */ > > +struct device_platdata { > > +       struct phandle_part phandlepart; > > +       char *mtdpart; > > +       char *ubivol; > > +}; > > + > > +/** > > + * release_firmware - Release the resource associated with a > > firmware image > > + * @firmware: Firmware resource to release > > + */ > > +void release_firmware(struct firmware *firmware); > > + > > +/** > > + * request_firmware_into_buf - Load firmware into a previously > > allocated buffer. > > + * @plat: Platform data such as storage and partition firmware > > loading from. > > + * @name: Name of firmware file. > > + * @buf: Address of buffer to load firmware into. > > + * @size: Size of buffer. > > + * @offset: Offset of a file for start reading into buffer. > > + * @firmwarep: Pointer to firmware image. > > + * > > + * The firmware is loaded directly into the buffer pointed to by > > @buf and > > + * the @firmwarep data member is pointed at @buf. > > + * > > + * Return: Size of total read, negative value when error. > > + */ > > +int request_firmware_into_buf(struct device_platdata *plat, > > +                             const char *name, > > +                             void *buf, size_t size, u32 offset, > > +                             struct firmware **firmwarep); > Without a test it's hard to see how this is used, but I'm not keen on > the struct firmware ** here. > > Can you just use struct firmware * instead? > > Or maybe just return the fields individually since you only have > start > address and size, I think. I can try to remove struct firmware, let driver model handles all memory allocation, and then struct device_platdata *plat will change to struct udevice *dev. So, it would become like this int request_firmware_into_buf(struct udevice *dev,       const char *name,       void *buf, size_t size, u32 offset); I will remove release_firmware() after letting driver model to handle all memory deallocation. So, the API interface would looks a bit different compare to Linux firmware API interface. Does this change OK for you? > > > > > +#endif > > -- > > 2.2.0 > > > Regards, > Simon