| Message ID | 1512460690-3454-8-git-send-email-tien.fong.chee@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | Add FPGA driver, SDRAM driver, generic firmware loader and booting U-Boot. | expand |
Hi, On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.com wrote: > From: Tien Fong Chee <tien.fong.chee@intel.com> > > 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 <tien.fong.chee@intel.com> > --- > common/Makefile | 1 + > common/fs_loader.c | 304 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/fs_loader.h | 30 ++++++ > 3 files changed, 335 insertions(+) > create mode 100644 common/fs_loader.c > create mode 100644 include/fs_loader.h > > diff --git a/common/Makefile b/common/Makefile > index 801ea31..419e915 100644 > --- a/common/Makefile > +++ b/common/Makefile > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > obj-y += command.o > obj-y += s_record.o > obj-y += xyzModem.o > +obj-y += fs_loader.o > diff --git a/common/fs_loader.c b/common/fs_loader.c > new file mode 100644 > index 0000000..04f682b > --- /dev/null > +++ b/common/fs_loader.c > @@ -0,0 +1,304 @@ > +/* > + * Copyright (C) 2017 Intel Corporation <www.intel.com> > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ > + > +#include <common.h> > +#include <errno.h> > +#include <fs.h> > +#include <fs_loader.h> > +#include <nand.h> > +#include <sata.h> > +#include <spi.h> > +#include <spi_flash.h> > +#include <spl.h> > +#include <linux/string.h> > +#include <usb.h> > + > +static struct device_location default_locations[] = { > + { > + .name = "mmc", > + .devpart = "0:1", > + }, > + { > + .name = "usb", > + .devpart = "0:1", > + }, > + { > + .name = "sata", > + .devpart = "0:1", > + }, > +}; > + > +/* USB build is not supported yet in SPL */ > +#ifndef CONFIG_SPL_BUILD > +#ifdef CONFIG_USB_STORAGE > +static int init_usb(void) > +{ > + int err = 0; > Useless initialization. > + err = usb_init(); > + > + if (err) > Unnecessary blank line. > + return err; > + > +#ifndef CONFIG_DM_USB > + err = usb_stor_scan(1) < 0 ? -ENODEV : 0; > +#endif > + > + return err; > +} > +#else > +static int init_usb(void) > +{ > + printf("Error: Cannot load flash image: no USB support\n"); > + return -ENOSYS; > +} > +#endif > +#endif > + > +#ifdef CONFIG_SATA > +static int init_storage_sata(void) > +{ > + return sata_probe(0); > +} > +#else > +static int init_storage_sata(void) > +{ > + printf("Error: Cannot load image: no SATA support\n"); > + return -ENOSYS; > +} > +#endif > + > +#ifdef CONFIG_CMD_UBIFS > +static int mount_ubifs(struct device_location *location) > +{ > + int ret = 0; > Useless initialization. > + char cmd[32]; > + > + sprintf(cmd, "ubi part %s", location->mtdpart); > + > + ret = run_command(cmd, 0); > + > + if (ret) Unnecessary blank line. > + return ret; > + > + sprintf(cmd, "ubifsmount %s", location->ubivol); > + > + ret = run_command(cmd, 0); > + > + return res; > s/res/ret/ You obviously never compiled this code with CONFIG_CMD_UBIFS enabled. > +} > + > +static int umount_ubifs(void) > +{ > + return run_command("ubifsumount", 0); > +} > +#else > +static int mount_ubifs(struct device_location *location) > +{ > + printf("Error: Cannot load image: no UBIFS support\n"); > + return -ENOSYS; > +} > + > +static int umount_ubifs(void) > +{ > + printf("Error: Cannot unmount UBIFS: no UBIFS support\n"); > This error message is useless, since umount_ubifs() would probably never be called when mount_ubifs() failed. > +} > +#endif > + > +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD) > +static int init_mmc(void) > +{ > + /* Just for the case MMC is not yet initialized */ > + struct mmc *mmc = NULL; > + int err = 0; > Useless initialization. > + > + spl_mmc_find_device(&mmc, spl_boot_device()); > + > + err = mmc_init(mmc); > + > + if (err) { > + printf("spl: mmc init failed with error: %d\n", err); > + return err; > + } > + > + return err; > +} > +#else > +static int init_mmc(void) > +{ > + /* Expect somewhere already initialize MMC */ > + return 0; > +} > +#endif > + > +static int select_fs_dev(struct device_location *location) > +{ > + int ret = 0; > Useless initialization. Omitting the initialization here makes sure, that 'ret' is properly set in each of the below if/else clauses. Otherwise the compiler will complain about an unitialized use of 'ret'. > + if (!strcmp("mmc", location->name)) > + ret = fs_set_blk_dev("mmc", location->devpart, FS_TYPE_ANY); > + else if (!strcmp("usb", location->name)) > + ret = fs_set_blk_dev("usb", location->devpart, FS_TYPE_ANY); > + else if (!strcmp("sata", location->name)) > + ret = fs_set_blk_dev("sata", location->devpart, FS_TYPE_ANY); > + else if (!strcmp("ubi", location->name)) > + if (location->ubivol != NULL) > + ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS); > + else > + ret = -ENODEV; > + else { > + printf("Error: unsupported location storage.\n"); > + return -ENODEV; > + } > + All if/else clauses should be enclosed in {}. Refer to Documentation/CodingStyle in the Linux source which also applies to U-Boot. > + if (ret) > + printf("Error: could not access storage.\n"); > + > + return ret; > +} > + > +static int init_storage_device(struct device_location *location) > +{ > + int ret = 0; > +#ifndef CONFIG_SPL_BUILD > + /* USB build is not supported yet in SPL */ > + if (!strcmp("usb", location->name)) > + ret = init_usb(); > +#endif > + > + if (!strcmp("mmc", location->name)) > + ret = init_mmc(); > + > + if (!strcmp("sata", location->name)) > + ret = init_storage_sata(); > + > + if (location->ubivol != NULL) > + ret = mount_ubifs(location); > + > + return ret; > +} > + > +static void set_storage_devpart(char *name, char *devpart) > +{ > + int i; > + u32 size; > + > + size = ARRAY_SIZE(default_locations); > + > + for (i = 0; i < size; i++) { > The compiler should complain about a comparison of signed and unsigned integers. I would do: size_t i; for (i = 0; i < ARRAY_SIZE(default_locations); i++) { [...] > + if (!strcmp(default_locations[i].name, name)) > + default_locations[i].devpart = devpart; > + } > + > + return; > +} > + > +/* > + * Prepare firmware struct; > + * return -ve if fail. > + */ > +static int _request_firmware_prepare(struct firmware **firmware_p, > + const char *name, void *dbuf, > + size_t size, u32 offset) > +{ > + struct firmware *firmware = NULL; > + int ret = 0; > + > + if (!name || name[0] == '\0') > + ret = -EINVAL; > + Is it really useful to continue here initializing the 'firmware' struct and returning an error at the end? > + *firmware_p = firmware = calloc(1, sizeof(*firmware)); > + > + if (!firmware) { > + printf("%s: calloc(struct firmware) failed\n", __func__); > + return -ENOMEM; > + } > + > + firmware->name = name; > + firmware->data = dbuf; > + firmware->size = size; > + firmware->offset = offset; > + > + return ret; > +} > + > +/* > + * fw_get_filesystem_firmware - load firmware into a allocated buffer > + * @firmware_p: pointer to firmware image > + * @location: An array of supported firmware location > + * > + * @return: size of total read > + * -ve when error > + */ > +static int fw_get_filesystem_firmware(struct device_location *location, > + struct firmware *firmware_p) > +{ > + loff_t actread; > + char *dev_part; > + int ret = 0; > + Useless initialization. > + dev_part = env_get("FW_DEV_PART"); > + > + if (dev_part) > Unnecessary blank line. > + set_storage_devpart(location->name, dev_part); > + > + ret = init_storage_device(location); > + > + if (ret) Unnecessary blank line. > + goto out; > + > + select_fs_dev(location); > + > + ret = fs_read(firmware_p->name, (u32)firmware_p->data, > The second parameter of fs_read is of type 'ulong' NOT 'u32'! This will blow up when compiling for 64bit machines. > + firmware_p->offset, firmware_p->size, &actread); > + > + if (ret || (actread != firmware_p->size)) { > + printf("Error: %d Failed to read %s from flash %lld ", > + ret, > + firmware_p->name, > + actread); > + printf("!= %d.\n", firmware_p->size); > + return -EPERM; > + } else > + ret = actread; > + broken indentation. All clauses of the if/else statement should be enclosed in {}. > +out: > + if (location->ubivol != NULL) > + umount_ubifs(); > + > + return ret; > +} > +/* > + * request_firmware_into_buf - load firmware into a previously allocated buffer > + * @firmware_p: pointer to firmware image > + * @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 > + * > + * This firmware is loaded directly into the buffer pointed to by @buf and > + * the @firmware_p data member is pointed at @buf. > + * > + * @return: size of total read > + * -ve when error > + */ > +int request_firmware_into_buf(struct firmware **firmware_p, > + const char *name, > + struct device_location *location, > + void *buf, size_t size, u32 offset) > +{ > + int ret = 0; > Useless initialization. > + ret = _request_firmware_prepare(firmware_p, name, buf, size, offset); > + > + if (ret < 0) /* error */ > Unnecessary blank line. > + return ret; > + > + ret = fw_get_filesystem_firmware(location, *firmware_p); > + > + return ret; > +} > diff --git a/include/fs_loader.h b/include/fs_loader.h > new file mode 100644 > index 0000000..dacc1f3 > --- /dev/null > +++ b/include/fs_loader.h > @@ -0,0 +1,30 @@ > +/* > + * Copyright (C) 2017 Intel Corporation <www.intel.com> > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ > +#ifndef _FS_LOADER_H_ > +#define _FS_LOADER_H_ > + > +#include <linux/types.h> > + > +struct firmware { > + size_t size; /* Size of a file */ > + const u8 *data; /* Buffer for file */ > const void *data? > + const char *name; /* Filename */ > + u32 offset; /* Offset of reading a file */ > + void *priv; /* Firmware loader private fields */ > +}; > + > +struct device_location { > + char *name; /* Such as mmc, usb,and sata. */ > + char *devpart; /* Use the load command dev:part conventions */ > + char *mtdpart; /* MTD partition for ubi part */ > + char *ubivol; /* UBI volume-name for ubifsmount */ > +}; > + > +int request_firmware_into_buf(struct firmware **firmware_p, > + const char *name, > + struct device_location *location, > + void *buf, size_t size, u32 offset); > +#endif Lothar Waßmann
On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote: > Hi, > > On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.com wrote: > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > 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 <tien.fong.chee@intel.com> > > --- > > common/Makefile | 1 + > > common/fs_loader.c | 304 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > include/fs_loader.h | 30 ++++++ > > 3 files changed, 335 insertions(+) > > create mode 100644 common/fs_loader.c > > create mode 100644 include/fs_loader.h > > > > diff --git a/common/Makefile b/common/Makefile > > index 801ea31..419e915 100644 > > --- a/common/Makefile > > +++ b/common/Makefile > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > > obj-y += command.o > > obj-y += s_record.o > > obj-y += xyzModem.o > > +obj-y += fs_loader.o > > diff --git a/common/fs_loader.c b/common/fs_loader.c > > new file mode 100644 > > index 0000000..04f682b > > --- /dev/null > > +++ b/common/fs_loader.c > > @@ -0,0 +1,304 @@ > > +/* > > + * Copyright (C) 2017 Intel Corporation <www.intel.com> > > + * > > + * SPDX-License-Identifier: GPL-2.0 > > + */ > > + > > +#include <common.h> > > +#include <errno.h> > > +#include <fs.h> > > +#include <fs_loader.h> > > +#include <nand.h> > > +#include <sata.h> > > +#include <spi.h> > > +#include <spi_flash.h> > > +#include <spl.h> > > +#include <linux/string.h> > > +#include <usb.h> > > + > > +static struct device_location default_locations[] = { > > + { > > + .name = "mmc", > > + .devpart = "0:1", > > + }, > > + { > > + .name = "usb", > > + .devpart = "0:1", > > + }, > > + { > > + .name = "sata", > > + .devpart = "0:1", > > + }, > > +}; > > + > > +/* USB build is not supported yet in SPL */ > > +#ifndef CONFIG_SPL_BUILD > > +#ifdef CONFIG_USB_STORAGE > > +static int init_usb(void) > > +{ > > + int err = 0; > > > Useless initialization. > noted. > > > > + err = usb_init(); > > + > > + if (err) > > > Unnecessary blank line. > Sorry, i'm not catching you because there is no blank line between "if" and "return" > > > > + return err; > > + > > +#ifndef CONFIG_DM_USB > > + err = usb_stor_scan(1) < 0 ? -ENODEV : 0; > > +#endif > > + > > + return err; > > +} > > +#else > > +static int init_usb(void) > > +{ > > + printf("Error: Cannot load flash image: no USB > > support\n"); > > + return -ENOSYS; > > +} > > +#endif > > +#endif > > + > > +#ifdef CONFIG_SATA > > +static int init_storage_sata(void) > > +{ > > + return sata_probe(0); > > +} > > +#else > > +static int init_storage_sata(void) > > +{ > > + printf("Error: Cannot load image: no SATA support\n"); > > + return -ENOSYS; > > +} > > +#endif > > + > > +#ifdef CONFIG_CMD_UBIFS > > +static int mount_ubifs(struct device_location *location) > > +{ > > + int ret = 0; > > > Useless initialization. > noted. > > > > + char cmd[32]; > > + > > + sprintf(cmd, "ubi part %s", location->mtdpart); > > + > > + ret = run_command(cmd, 0); > > + > > + if (ret) > Unnecessary blank line. > > > > > + return ret; > > + > > + sprintf(cmd, "ubifsmount %s", location->ubivol); > > + > > + ret = run_command(cmd, 0); > > + > > + return res; > > > s/res/ret/ > You obviously never compiled this code with CONFIG_CMD_UBIFS enabled. > Good catch!! I will rectify. > > > > +} > > + > > +static int umount_ubifs(void) > > +{ > > + return run_command("ubifsumount", 0); > > +} > > +#else > > +static int mount_ubifs(struct device_location *location) > > +{ > > + printf("Error: Cannot load image: no UBIFS support\n"); > > + return -ENOSYS; > > +} > > + > > +static int umount_ubifs(void) > > +{ > > + printf("Error: Cannot unmount UBIFS: no UBIFS support\n"); > > > This error message is useless, since umount_ubifs() would > probably never be called when mount_ubifs() failed. > Yeah, you are right, i will remove it. > > > > +} > > +#endif > > + > > +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD) > > +static int init_mmc(void) > > +{ > > + /* Just for the case MMC is not yet initialized */ > > + struct mmc *mmc = NULL; > > + int err = 0; > > > Useless initialization. > noted. > > > > + > > + spl_mmc_find_device(&mmc, spl_boot_device()); > > + > > + err = mmc_init(mmc); > > + > > + if (err) { > > + printf("spl: mmc init failed with error: %d\n", > > err); > > + return err; > > + } > > + > > + return err; > > +} > > +#else > > +static int init_mmc(void) > > +{ > > + /* Expect somewhere already initialize MMC */ > > + return 0; > > +} > > +#endif > > + > > +static int select_fs_dev(struct device_location *location) > > +{ > > + int ret = 0; > > > Useless initialization. Omitting the initialization here makes sure, > that 'ret' is properly set in each of the below if/else clauses. > Otherwise the compiler will complain about an unitialized use of > 'ret'. > Okay. sure. > > > > + if (!strcmp("mmc", location->name)) > > + ret = fs_set_blk_dev("mmc", location->devpart, > > FS_TYPE_ANY); > > + else if (!strcmp("usb", location->name)) > > + ret = fs_set_blk_dev("usb", location->devpart, > > FS_TYPE_ANY); > > + else if (!strcmp("sata", location->name)) > > + ret = fs_set_blk_dev("sata", location->devpart, > > FS_TYPE_ANY); > > + else if (!strcmp("ubi", location->name)) > > + if (location->ubivol != NULL) > > + ret = fs_set_blk_dev("ubi", NULL, > > FS_TYPE_UBIFS); > > + else > > + ret = -ENODEV; > > + else { > > + printf("Error: unsupported location storage.\n"); > > + return -ENODEV; > > + } > > + > All if/else clauses should be enclosed in {}. > Refer to Documentation/CodingStyle in the Linux source which also > applies to U-Boot. > Okay. > > > > + if (ret) > > + printf("Error: could not access storage.\n"); > > + > > + return ret; > > +} > > + > > +static int init_storage_device(struct device_location *location) > > +{ > > + int ret = 0; > > +#ifndef CONFIG_SPL_BUILD > > + /* USB build is not supported yet in SPL */ > > + if (!strcmp("usb", location->name)) > > + ret = init_usb(); > > +#endif > > + > > + if (!strcmp("mmc", location->name)) > > + ret = init_mmc(); > > + > > + if (!strcmp("sata", location->name)) > > + ret = init_storage_sata(); > > + > > + if (location->ubivol != NULL) > > + ret = mount_ubifs(location); > > + > > + return ret; > > +} > > + > > +static void set_storage_devpart(char *name, char *devpart) > > +{ > > + int i; > > + u32 size; > > + > > + size = ARRAY_SIZE(default_locations); > > + > > + for (i = 0; i < size; i++) { > > > The compiler should complain about a comparison of signed and > unsigned > integers. > I would do: > size_t i; > > for (i = 0; i < ARRAY_SIZE(default_locations); i++) { > [...] > Okay. noted. > > > > + if (!strcmp(default_locations[i].name, name)) > > + default_locations[i].devpart = devpart; > > + } > > + > > + return; > > +} > > + > > +/* > > + * Prepare firmware struct; > > + * return -ve if fail. > > + */ > > +static int _request_firmware_prepare(struct firmware **firmware_p, > > + const char *name, void *dbuf, > > + size_t size, u32 offset) > > +{ > > + struct firmware *firmware = NULL; > > + int ret = 0; > > + > > + if (!name || name[0] == '\0') > > + ret = -EINVAL; > > + > Is it really useful to continue here initializing the 'firmware' > struct and returning an error at the end? > I try to keep it very close to Linux firmware loader. When more API ported in from Linux in future, this can be helper function. Anyway, i have no strong opinion, i can move to caller if you guys think that is better. > > > > + *firmware_p = firmware = calloc(1, sizeof(*firmware)); > > + > > + if (!firmware) { > > + printf("%s: calloc(struct firmware) failed\n", > > __func__); > > + return -ENOMEM; > > + } > > + > > + firmware->name = name; > > + firmware->data = dbuf; > > + firmware->size = size; > > + firmware->offset = offset; > > + > > + return ret; > > +} > > + > > +/* > > + * fw_get_filesystem_firmware - load firmware into a allocated > > buffer > > + * @firmware_p: pointer to firmware image > > + * @location: An array of supported firmware location > > + * > > + * @return: size of total read > > + * -ve when error > > + */ > > +static int fw_get_filesystem_firmware(struct device_location > > *location, > > + struct firmware *firmware_p) > > +{ > > + loff_t actread; > > + char *dev_part; > > + int ret = 0; > > + > Useless initialization. > noted. > > > > + dev_part = env_get("FW_DEV_PART"); > > + > > + if (dev_part) > > > Unnecessary blank line. > > > > > + set_storage_devpart(location->name, dev_part); > > + > > + ret = init_storage_device(location); > > + > > + if (ret) > Unnecessary blank line. > > > > > + goto out; > > + > > + select_fs_dev(location); > > + > > + ret = fs_read(firmware_p->name, (u32)firmware_p->data, > > > The second parameter of fs_read is of type 'ulong' NOT 'u32'! > This will blow up when compiling for 64bit machines. > Okay. > > > > + firmware_p->offset, firmware_p->size, > > &actread); > > + > > + if (ret || (actread != firmware_p->size)) { > > + printf("Error: %d Failed to read %s from flash > > %lld ", > > + ret, > > + firmware_p->name, > > + actread); > > + printf("!= %d.\n", firmware_p->size); > > + return -EPERM; > > + } else > > + ret = actread; > > + > broken indentation. > All clauses of the if/else statement should be enclosed in {}. > Okay. > > > > +out: > > + if (location->ubivol != NULL) > > + umount_ubifs(); > > + > > + return ret; > > +} > > +/* > > + * request_firmware_into_buf - load firmware into a previously > > allocated buffer > > + * @firmware_p: pointer to firmware image > > + * @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 > > + * > > + * This firmware is loaded directly into the buffer pointed to by > > @buf and > > + * the @firmware_p data member is pointed at @buf. > > + * > > + * @return: size of total read > > + * -ve when error > > + */ > > +int request_firmware_into_buf(struct firmware **firmware_p, > > + const char *name, > > + struct device_location *location, > > + void *buf, size_t size, u32 offset) > > +{ > > + int ret = 0; > > > Useless initialization. > noted. > > > > + ret = _request_firmware_prepare(firmware_p, name, buf, > > size, offset); > > + > > + if (ret < 0) /* error */ > > > Unnecessary blank line. > > > > > + return ret; > > + > > + ret = fw_get_filesystem_firmware(location, *firmware_p); > > + > > + return ret; > > +} > > diff --git a/include/fs_loader.h b/include/fs_loader.h > > new file mode 100644 > > index 0000000..dacc1f3 > > --- /dev/null > > +++ b/include/fs_loader.h > > @@ -0,0 +1,30 @@ > > +/* > > + * Copyright (C) 2017 Intel Corporation <www.intel.com> > > + * > > + * SPDX-License-Identifier: GPL-2.0 > > + */ > > +#ifndef _FS_LOADER_H_ > > +#define _FS_LOADER_H_ > > + > > +#include <linux/types.h> > > + > > +struct firmware { > > + size_t size; /* Size of a file */ > > + const u8 *data; /* Buffer for file */ > > > const void *data? > okay. > > > > + const char *name; /* Filename */ > > + u32 offset; /* Offset of reading a file */ > > + void *priv; /* Firmware loader private > > fields */ > > +}; > > + > > +struct device_location { > > + char *name; /* Such as mmc, usb,and sata. */ > > + char *devpart; /* Use the load command dev:part > > conventions */ > > + char *mtdpart; /* MTD partition for ubi part */ > > + char *ubivol; /* UBI volume-name for ubifsmount */ > > +}; > > + > > +int request_firmware_into_buf(struct firmware **firmware_p, > > + const char *name, > > + struct device_location *location, > > + void *buf, size_t size, u32 offset); > > +#endif > > Lothar Waßmann
Hi, On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote: > On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote: > > Hi, > > > > On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.com wrote: > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > > 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 <tien.fong.chee@intel.com> > > > --- > > > common/Makefile | 1 + > > > common/fs_loader.c | 304 > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > include/fs_loader.h | 30 ++++++ > > > 3 files changed, 335 insertions(+) > > > create mode 100644 common/fs_loader.c > > > create mode 100644 include/fs_loader.h > > > > > > diff --git a/common/Makefile b/common/Makefile > > > index 801ea31..419e915 100644 > > > --- a/common/Makefile > > > +++ b/common/Makefile > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > > > obj-y += command.o > > > obj-y += s_record.o > > > obj-y += xyzModem.o > > > +obj-y += fs_loader.o > > > diff --git a/common/fs_loader.c b/common/fs_loader.c > > > new file mode 100644 > > > index 0000000..04f682b > > > --- /dev/null > > > +++ b/common/fs_loader.c > > > @@ -0,0 +1,304 @@ > > > +/* > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com> > > > + * > > > + * SPDX-License-Identifier: GPL-2.0 > > > + */ > > > + > > > +#include <common.h> > > > +#include <errno.h> > > > +#include <fs.h> > > > +#include <fs_loader.h> > > > +#include <nand.h> > > > +#include <sata.h> > > > +#include <spi.h> > > > +#include <spi_flash.h> > > > +#include <spl.h> > > > +#include <linux/string.h> > > > +#include <usb.h> > > > + > > > +static struct device_location default_locations[] = { > > > + { > > > + .name = "mmc", > > > + .devpart = "0:1", > > > + }, > > > + { > > > + .name = "usb", > > > + .devpart = "0:1", > > > + }, > > > + { > > > + .name = "sata", > > > + .devpart = "0:1", > > > + }, > > > +}; > > > + > > > +/* USB build is not supported yet in SPL */ > > > +#ifndef CONFIG_SPL_BUILD > > > +#ifdef CONFIG_USB_STORAGE > > > +static int init_usb(void) > > > +{ > > > + int err = 0; > > > > > Useless initialization. > > > noted. > > > > > > + err = usb_init(); > > > + > > > + if (err) > > > > > Unnecessary blank line. > > > Sorry, i'm not catching you because there is no blank line between "if" > and "return" > I meant the blank line between 'err = ...' and 'if (err)' > > > > > > + if (!strcmp(default_locations[i].name, name)) > > > + default_locations[i].devpart = devpart; > > > + } > > > + > > > + return; > > > +} > > > + > > > +/* > > > + * Prepare firmware struct; > > > + * return -ve if fail. > > > + */ > > > +static int _request_firmware_prepare(struct firmware **firmware_p, > > > + const char *name, void *dbuf, > > > + size_t size, u32 offset) > > > +{ > > > + struct firmware *firmware = NULL; > > > + int ret = 0; > > > + > > > + if (!name || name[0] == '\0') > > > + ret = -EINVAL; > > > + > > Is it really useful to continue here initializing the 'firmware' > > struct and returning an error at the end? > > > I try to keep it very close to Linux firmware loader. When more API > ported in from Linux in future, this can be helper function. Anyway, i > have no strong opinion, i can move to caller if you guys think that is > better. The Linux firmware loader has this: | if (!name || name[0] == '\0') { | ret = -EINVAL; | goto out; | } Note the 'goto out' which is missing in your code. If following the Linux code closely, you would have to set *firmware_p to NULL and exit in this case. Lothar Waßmann
On Rab, 2017-12-06 at 12:00 +0100, Lothar Waßmann wrote: > Hi, > > On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote: > > > > On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote: > > > > > > Hi, > > > > > > On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.com > > > wrote: > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > > > > 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 <tien.fong.chee@intel.com> > > > > --- > > > > common/Makefile | 1 + > > > > common/fs_loader.c | 304 > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > include/fs_loader.h | 30 ++++++ > > > > 3 files changed, 335 insertions(+) > > > > create mode 100644 common/fs_loader.c > > > > create mode 100644 include/fs_loader.h > > > > > > > > diff --git a/common/Makefile b/common/Makefile > > > > index 801ea31..419e915 100644 > > > > --- a/common/Makefile > > > > +++ b/common/Makefile > > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > > > > obj-y += command.o > > > > obj-y += s_record.o > > > > obj-y += xyzModem.o > > > > +obj-y += fs_loader.o > > > > diff --git a/common/fs_loader.c b/common/fs_loader.c > > > > new file mode 100644 > > > > index 0000000..04f682b > > > > --- /dev/null > > > > +++ b/common/fs_loader.c > > > > @@ -0,0 +1,304 @@ > > > > +/* > > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com> > > > > + * > > > > + * SPDX-License-Identifier: GPL-2.0 > > > > + */ > > > > + > > > > +#include <common.h> > > > > +#include <errno.h> > > > > +#include <fs.h> > > > > +#include <fs_loader.h> > > > > +#include <nand.h> > > > > +#include <sata.h> > > > > +#include <spi.h> > > > > +#include <spi_flash.h> > > > > +#include <spl.h> > > > > +#include <linux/string.h> > > > > +#include <usb.h> > > > > + > > > > +static struct device_location default_locations[] = { > > > > + { > > > > + .name = "mmc", > > > > + .devpart = "0:1", > > > > + }, > > > > + { > > > > + .name = "usb", > > > > + .devpart = "0:1", > > > > + }, > > > > + { > > > > + .name = "sata", > > > > + .devpart = "0:1", > > > > + }, > > > > +}; > > > > + > > > > +/* USB build is not supported yet in SPL */ > > > > +#ifndef CONFIG_SPL_BUILD > > > > +#ifdef CONFIG_USB_STORAGE > > > > +static int init_usb(void) > > > > +{ > > > > + int err = 0; > > > > > > > Useless initialization. > > > > > noted. > > > > > > > > > > > > > > > + err = usb_init(); > > > > + > > > > + if (err) > > > > > > > Unnecessary blank line. > > > > > Sorry, i'm not catching you because there is no blank line between > > "if" > > and "return" > > > I meant the blank line between 'err = ...' and 'if (err)' > Okay, i can change that. > > > > > > > > > > > > > > > > > + if (!strcmp(default_locations[i].name, name)) > > > > + default_locations[i].devpart = > > > > devpart; > > > > + } > > > > + > > > > + return; > > > > +} > > > > + > > > > +/* > > > > + * Prepare firmware struct; > > > > + * return -ve if fail. > > > > + */ > > > > +static int _request_firmware_prepare(struct firmware > > > > **firmware_p, > > > > + const char *name, void > > > > *dbuf, > > > > + size_t size, u32 offset) > > > > +{ > > > > + struct firmware *firmware = NULL; > > > > + int ret = 0; > > > > + > > > > + if (!name || name[0] == '\0') > > > > + ret = -EINVAL; > > > > + > > > Is it really useful to continue here initializing the 'firmware' > > > struct and returning an error at the end? > > > > > I try to keep it very close to Linux firmware loader. When more API > > ported in from Linux in future, this can be helper function. > > Anyway, i > > have no strong opinion, i can move to caller if you guys think that > > is > > better. > The Linux firmware loader has this: > > > > if (!name || name[0] == '\0') { > > ret = -EINVAL; > > goto out; > > } > Note the 'goto out' which is missing in your code. > If following the Linux code closely, you would have to set > *firmware_p > to NULL and exit in this case. > I can set the *firmware_p to NUll in failing case. But, i checked the static int _request_firmware_prepare in Linux, there is no goto out error handling method in the function. Fyi, there is no allocated memory release in U-Boot. https://elixir.free-electrons.com/linux/v4.13.15/source/drivers/base/fi rmware_class.c#L1191 > > Lothar Waßmann
Hi, On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote: > On Rab, 2017-12-06 at 12:00 +0100, Lothar Waßmann wrote: > > Hi, > > > > On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote: > > > > > > On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote: > > > > > > > > Hi, > > > > > > > > On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.com > > > > wrote: > > > > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > > > > > > 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 <tien.fong.chee@intel.com> > > > > > --- > > > > > common/Makefile | 1 + > > > > > common/fs_loader.c | 304 > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > include/fs_loader.h | 30 ++++++ > > > > > 3 files changed, 335 insertions(+) > > > > > create mode 100644 common/fs_loader.c > > > > > create mode 100644 include/fs_loader.h > > > > > > > > > > diff --git a/common/Makefile b/common/Makefile > > > > > index 801ea31..419e915 100644 > > > > > --- a/common/Makefile > > > > > +++ b/common/Makefile > > > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > > > > > obj-y += command.o > > > > > obj-y += s_record.o > > > > > obj-y += xyzModem.o > > > > > +obj-y += fs_loader.o > > > > > diff --git a/common/fs_loader.c b/common/fs_loader.c > > > > > new file mode 100644 > > > > > index 0000000..04f682b > > > > > --- /dev/null > > > > > +++ b/common/fs_loader.c > > > > > @@ -0,0 +1,304 @@ > > > > > +/* > > > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com> > > > > > + * > > > > > + * SPDX-License-Identifier: GPL-2.0 > > > > > + */ > > > > > + > > > > > +#include <common.h> > > > > > +#include <errno.h> > > > > > +#include <fs.h> > > > > > +#include <fs_loader.h> > > > > > +#include <nand.h> > > > > > +#include <sata.h> > > > > > +#include <spi.h> > > > > > +#include <spi_flash.h> > > > > > +#include <spl.h> > > > > > +#include <linux/string.h> > > > > > +#include <usb.h> > > > > > + > > > > > +static struct device_location default_locations[] = { > > > > > + { > > > > > + .name = "mmc", > > > > > + .devpart = "0:1", > > > > > + }, > > > > > + { > > > > > + .name = "usb", > > > > > + .devpart = "0:1", > > > > > + }, > > > > > + { > > > > > + .name = "sata", > > > > > + .devpart = "0:1", > > > > > + }, > > > > > +}; > > > > > + > > > > > +/* USB build is not supported yet in SPL */ > > > > > +#ifndef CONFIG_SPL_BUILD > > > > > +#ifdef CONFIG_USB_STORAGE > > > > > +static int init_usb(void) > > > > > +{ > > > > > + int err = 0; > > > > > > > > > Useless initialization. > > > > > > > noted. > > > > > > > > > > > > > > > > > > > + err = usb_init(); > > > > > + > > > > > + if (err) > > > > > > > > > Unnecessary blank line. > > > > > > > Sorry, i'm not catching you because there is no blank line between > > > "if" > > > and "return" > > > > > I meant the blank line between 'err = ...' and 'if (err)' > > > Okay, i can change that. > > > > > > > > > > > > > > > > > > > > > > + if (!strcmp(default_locations[i].name, name)) > > > > > + default_locations[i].devpart = > > > > > devpart; > > > > > + } > > > > > + > > > > > + return; > > > > > +} > > > > > + > > > > > +/* > > > > > + * Prepare firmware struct; > > > > > + * return -ve if fail. > > > > > + */ > > > > > +static int _request_firmware_prepare(struct firmware > > > > > **firmware_p, > > > > > + const char *name, void > > > > > *dbuf, > > > > > + size_t size, u32 offset) > > > > > +{ > > > > > + struct firmware *firmware = NULL; > > > > > + int ret = 0; > > > > > + > > > > > + if (!name || name[0] == '\0') > > > > > + ret = -EINVAL; > > > > > + > > > > Is it really useful to continue here initializing the 'firmware' > > > > struct and returning an error at the end? > > > > > > > I try to keep it very close to Linux firmware loader. When more API > > > ported in from Linux in future, this can be helper function. > > > Anyway, i > > > have no strong opinion, i can move to caller if you guys think that > > > is > > > better. > > The Linux firmware loader has this: > > > > > > if (!name || name[0] == '\0') { > > > ret = -EINVAL; > > > goto out; > > > } > > Note the 'goto out' which is missing in your code. > > If following the Linux code closely, you would have to set > > *firmware_p > > to NULL and exit in this case. > > > I can set the *firmware_p to NUll in failing case. But, i checked the > static int _request_firmware_prepare in Linux, there is no goto out > error handling method in the function. Fyi, there is no allocated > memory release in U-Boot. > https://elixir.free-electrons.com/linux/v4.13.15/source/drivers/base/fi > rmware_class.c#L1191 > > I was referring to _request_firmware() which calls request_firmware_prepare() and does the checking of 'name' as your code does. Lothar Waßmann
On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote: > Hi, > > On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote: > > > > On Rab, 2017-12-06 at 12:00 +0100, Lothar Waßmann wrote: > > > > > > Hi, > > > > > > On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote: > > > > > > > > > > > > On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote: > > > > > > > > > > > > > > > Hi, > > > > > > > > > > On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.com > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > > > > > > > > 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 <tien.fong.chee@intel.com> > > > > > > --- > > > > > > common/Makefile | 1 + > > > > > > common/fs_loader.c | 304 > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > include/fs_loader.h | 30 ++++++ > > > > > > 3 files changed, 335 insertions(+) > > > > > > create mode 100644 common/fs_loader.c > > > > > > create mode 100644 include/fs_loader.h > > > > > > > > > > > > diff --git a/common/Makefile b/common/Makefile > > > > > > index 801ea31..419e915 100644 > > > > > > --- a/common/Makefile > > > > > > +++ b/common/Makefile > > > > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > > > > > > obj-y += command.o > > > > > > obj-y += s_record.o > > > > > > obj-y += xyzModem.o > > > > > > +obj-y += fs_loader.o > > > > > > diff --git a/common/fs_loader.c b/common/fs_loader.c > > > > > > new file mode 100644 > > > > > > index 0000000..04f682b > > > > > > --- /dev/null > > > > > > +++ b/common/fs_loader.c > > > > > > @@ -0,0 +1,304 @@ > > > > > > +/* > > > > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com> > > > > > > + * > > > > > > + * SPDX-License-Identifier: GPL-2.0 > > > > > > + */ > > > > > > + > > > > > > +#include <common.h> > > > > > > +#include <errno.h> > > > > > > +#include <fs.h> > > > > > > +#include <fs_loader.h> > > > > > > +#include <nand.h> > > > > > > +#include <sata.h> > > > > > > +#include <spi.h> > > > > > > +#include <spi_flash.h> > > > > > > +#include <spl.h> > > > > > > +#include <linux/string.h> > > > > > > +#include <usb.h> > > > > > > + > > > > > > +static struct device_location default_locations[] = { > > > > > > + { > > > > > > + .name = "mmc", > > > > > > + .devpart = "0:1", > > > > > > + }, > > > > > > + { > > > > > > + .name = "usb", > > > > > > + .devpart = "0:1", > > > > > > + }, > > > > > > + { > > > > > > + .name = "sata", > > > > > > + .devpart = "0:1", > > > > > > + }, > > > > > > +}; > > > > > > + > > > > > > +/* USB build is not supported yet in SPL */ > > > > > > +#ifndef CONFIG_SPL_BUILD > > > > > > +#ifdef CONFIG_USB_STORAGE > > > > > > +static int init_usb(void) > > > > > > +{ > > > > > > + int err = 0; > > > > > > > > > > > Useless initialization. > > > > > > > > > noted. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + err = usb_init(); > > > > > > + > > > > > > + if (err) > > > > > > > > > > > Unnecessary blank line. > > > > > > > > > Sorry, i'm not catching you because there is no blank line > > > > between > > > > "if" > > > > and "return" > > > > > > > I meant the blank line between 'err = ...' and 'if (err)' > > > > > Okay, i can change that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + if (!strcmp(default_locations[i].name, > > > > > > name)) > > > > > > + default_locations[i].devpart = > > > > > > devpart; > > > > > > + } > > > > > > + > > > > > > + return; > > > > > > +} > > > > > > + > > > > > > +/* > > > > > > + * Prepare firmware struct; > > > > > > + * return -ve if fail. > > > > > > + */ > > > > > > +static int _request_firmware_prepare(struct firmware > > > > > > **firmware_p, > > > > > > + const char *name, > > > > > > void > > > > > > *dbuf, > > > > > > + size_t size, u32 > > > > > > offset) > > > > > > +{ > > > > > > + struct firmware *firmware = NULL; > > > > > > + int ret = 0; > > > > > > + > > > > > > + if (!name || name[0] == '\0') > > > > > > + ret = -EINVAL; > > > > > > + > > > > > Is it really useful to continue here initializing the > > > > > 'firmware' > > > > > struct and returning an error at the end? > > > > > > > > > I try to keep it very close to Linux firmware loader. When more > > > > API > > > > ported in from Linux in future, this can be helper function. > > > > Anyway, i > > > > have no strong opinion, i can move to caller if you guys think > > > > that > > > > is > > > > better. > > > The Linux firmware loader has this: > > > > > > > > > > > > if (!name || name[0] == '\0') { > > > > ret = -EINVAL; > > > > goto out; > > > > } > > > Note the 'goto out' which is missing in your code. > > > If following the Linux code closely, you would have to set > > > *firmware_p > > > to NULL and exit in this case. > > > > > I can set the *firmware_p to NUll in failing case. But, i checked > > the > > static int _request_firmware_prepare in Linux, there is no goto out > > error handling method in the function. Fyi, there is no allocated > > memory release in U-Boot. > > https://elixir.free-electrons.com/linux/v4.13.15/source/drivers/bas > > e/fi > > rmware_class.c#L1191 > > > > > > > I was referring to _request_firmware() which calls > request_firmware_prepare() and does the checking of 'name' as your > code does. > Ohh.....i skipped _request_firmware() because there is no error handling required in U-Boot such as memory release. For the checking at both firmware_p and name, i would rather to merge them into request_firware_prepare() because they are related to each other instead of creating one function just for checking purpose. What do you think? > > Lothar Waßmann
Hi, On Thu, 7 Dec 2017 08:10:06 +0000 Chee, Tien Fong wrote: > On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote: > > Hi, > > > > On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote: > > > > > > On Rab, 2017-12-06 at 12:00 +0100, Lothar Waßmann wrote: > > > > > > > > Hi, > > > > > > > > On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote: > > > > > > > > > > > > > > > On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote: > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.com > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > > > > > > > > > > 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 <tien.fong.chee@intel.com> > > > > > > > --- > > > > > > > common/Makefile | 1 + > > > > > > > common/fs_loader.c | 304 > > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > include/fs_loader.h | 30 ++++++ > > > > > > > 3 files changed, 335 insertions(+) > > > > > > > create mode 100644 common/fs_loader.c > > > > > > > create mode 100644 include/fs_loader.h > > > > > > > > > > > > > > diff --git a/common/Makefile b/common/Makefile > > > > > > > index 801ea31..419e915 100644 > > > > > > > --- a/common/Makefile > > > > > > > +++ b/common/Makefile > > > > > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > > > > > > > obj-y += command.o > > > > > > > obj-y += s_record.o > > > > > > > obj-y += xyzModem.o > > > > > > > +obj-y += fs_loader.o > > > > > > > diff --git a/common/fs_loader.c b/common/fs_loader.c > > > > > > > new file mode 100644 > > > > > > > index 0000000..04f682b > > > > > > > --- /dev/null > > > > > > > +++ b/common/fs_loader.c > > > > > > > @@ -0,0 +1,304 @@ > > > > > > > +/* > > > > > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com> > > > > > > > + * > > > > > > > + * SPDX-License-Identifier: GPL-2.0 > > > > > > > + */ > > > > > > > + > > > > > > > +#include <common.h> > > > > > > > +#include <errno.h> > > > > > > > +#include <fs.h> > > > > > > > +#include <fs_loader.h> > > > > > > > +#include <nand.h> > > > > > > > +#include <sata.h> > > > > > > > +#include <spi.h> > > > > > > > +#include <spi_flash.h> > > > > > > > +#include <spl.h> > > > > > > > +#include <linux/string.h> > > > > > > > +#include <usb.h> > > > > > > > + > > > > > > > +static struct device_location default_locations[] = { > > > > > > > + { > > > > > > > + .name = "mmc", > > > > > > > + .devpart = "0:1", > > > > > > > + }, > > > > > > > + { > > > > > > > + .name = "usb", > > > > > > > + .devpart = "0:1", > > > > > > > + }, > > > > > > > + { > > > > > > > + .name = "sata", > > > > > > > + .devpart = "0:1", > > > > > > > + }, > > > > > > > +}; > > > > > > > + > > > > > > > +/* USB build is not supported yet in SPL */ > > > > > > > +#ifndef CONFIG_SPL_BUILD > > > > > > > +#ifdef CONFIG_USB_STORAGE > > > > > > > +static int init_usb(void) > > > > > > > +{ > > > > > > > + int err = 0; > > > > > > > > > > > > > Useless initialization. > > > > > > > > > > > noted. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + err = usb_init(); > > > > > > > + > > > > > > > + if (err) > > > > > > > > > > > > > Unnecessary blank line. > > > > > > > > > > > Sorry, i'm not catching you because there is no blank line > > > > > between > > > > > "if" > > > > > and "return" > > > > > > > > > I meant the blank line between 'err = ...' and 'if (err)' > > > > > > > Okay, i can change that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + if (!strcmp(default_locations[i].name, > > > > > > > name)) > > > > > > > + default_locations[i].devpart = > > > > > > > devpart; > > > > > > > + } > > > > > > > + > > > > > > > + return; > > > > > > > +} > > > > > > > + > > > > > > > +/* > > > > > > > + * Prepare firmware struct; > > > > > > > + * return -ve if fail. > > > > > > > + */ > > > > > > > +static int _request_firmware_prepare(struct firmware > > > > > > > **firmware_p, > > > > > > > + const char *name, > > > > > > > void > > > > > > > *dbuf, > > > > > > > + size_t size, u32 > > > > > > > offset) > > > > > > > +{ > > > > > > > + struct firmware *firmware = NULL; > > > > > > > + int ret = 0; > > > > > > > + > > > > > > > + if (!name || name[0] == '\0') > > > > > > > + ret = -EINVAL; > > > > > > > + > > > > > > Is it really useful to continue here initializing the > > > > > > 'firmware' > > > > > > struct and returning an error at the end? > > > > > > > > > > > I try to keep it very close to Linux firmware loader. When more > > > > > API > > > > > ported in from Linux in future, this can be helper function. > > > > > Anyway, i > > > > > have no strong opinion, i can move to caller if you guys think > > > > > that > > > > > is > > > > > better. > > > > The Linux firmware loader has this: > > > > > > > > > > > > > > > if (!name || name[0] == '\0') { > > > > > ret = -EINVAL; > > > > > goto out; > > > > > } > > > > Note the 'goto out' which is missing in your code. > > > > If following the Linux code closely, you would have to set > > > > *firmware_p > > > > to NULL and exit in this case. > > > > > > > I can set the *firmware_p to NUll in failing case. But, i checked > > > the > > > static int _request_firmware_prepare in Linux, there is no goto out > > > error handling method in the function. Fyi, there is no allocated > > > memory release in U-Boot. > > > https://elixir.free-electrons.com/linux/v4.13.15/source/drivers/bas > > > e/fi > > > rmware_class.c#L1191 > > > > > > > > > > I was referring to _request_firmware() which calls > > request_firmware_prepare() and does the checking of 'name' as your > > code does. > > > Ohh.....i skipped _request_firmware() because there is no error > handling required in U-Boot such as memory release. For the checking at > both firmware_p and name, i would rather to merge them into > request_firware_prepare() because they are related to each other > instead of creating one function just for checking purpose. What do you > think? > I don't mind whether you combine those functions or not as long as you reproduce the same functionality which your patch currently does not do. Lothar Waßmann
On Kha, 2017-12-07 at 10:00 +0100, Lothar Waßmann wrote: > Hi, > > On Thu, 7 Dec 2017 08:10:06 +0000 Chee, Tien Fong wrote: > > > > On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote: > > > > > > Hi, > > > > > > On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote: > > > > > > > > > > > > On Rab, 2017-12-06 at 12:00 +0100, Lothar Waßmann wrote: > > > > > > > > > > > > > > > Hi, > > > > > > > > > > On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.c > > > > > > > om > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > > > > > > > > > > > > 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 <tien.fong.chee@intel.com > > > > > > > > > > > > > > > > > --- > > > > > > > > common/Makefile | 1 + > > > > > > > > common/fs_loader.c | 304 > > > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > include/fs_loader.h | 30 ++++++ > > > > > > > > 3 files changed, 335 insertions(+) > > > > > > > > create mode 100644 common/fs_loader.c > > > > > > > > create mode 100644 include/fs_loader.h > > > > > > > > > > > > > > > > diff --git a/common/Makefile b/common/Makefile > > > > > > > > index 801ea31..419e915 100644 > > > > > > > > --- a/common/Makefile > > > > > > > > +++ b/common/Makefile > > > > > > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > > > > > > > > obj-y += command.o > > > > > > > > obj-y += s_record.o > > > > > > > > obj-y += xyzModem.o > > > > > > > > +obj-y += fs_loader.o > > > > > > > > diff --git a/common/fs_loader.c b/common/fs_loader.c > > > > > > > > new file mode 100644 > > > > > > > > index 0000000..04f682b > > > > > > > > --- /dev/null > > > > > > > > +++ b/common/fs_loader.c > > > > > > > > @@ -0,0 +1,304 @@ > > > > > > > > +/* > > > > > > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com > > > > > > > > > > > > > > > > > + * > > > > > > > > + * SPDX-License-Identifier: GPL-2.0 > > > > > > > > + */ > > > > > > > > + > > > > > > > > +#include <common.h> > > > > > > > > +#include <errno.h> > > > > > > > > +#include <fs.h> > > > > > > > > +#include <fs_loader.h> > > > > > > > > +#include <nand.h> > > > > > > > > +#include <sata.h> > > > > > > > > +#include <spi.h> > > > > > > > > +#include <spi_flash.h> > > > > > > > > +#include <spl.h> > > > > > > > > +#include <linux/string.h> > > > > > > > > +#include <usb.h> > > > > > > > > + > > > > > > > > +static struct device_location default_locations[] = { > > > > > > > > + { > > > > > > > > + .name = "mmc", > > > > > > > > + .devpart = "0:1", > > > > > > > > + }, > > > > > > > > + { > > > > > > > > + .name = "usb", > > > > > > > > + .devpart = "0:1", > > > > > > > > + }, > > > > > > > > + { > > > > > > > > + .name = "sata", > > > > > > > > + .devpart = "0:1", > > > > > > > > + }, > > > > > > > > +}; > > > > > > > > + > > > > > > > > +/* USB build is not supported yet in SPL */ > > > > > > > > +#ifndef CONFIG_SPL_BUILD > > > > > > > > +#ifdef CONFIG_USB_STORAGE > > > > > > > > +static int init_usb(void) > > > > > > > > +{ > > > > > > > > + int err = 0; > > > > > > > > > > > > > > > Useless initialization. > > > > > > > > > > > > > noted. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + err = usb_init(); > > > > > > > > + > > > > > > > > + if (err) > > > > > > > > > > > > > > > Unnecessary blank line. > > > > > > > > > > > > > Sorry, i'm not catching you because there is no blank line > > > > > > between > > > > > > "if" > > > > > > and "return" > > > > > > > > > > > I meant the blank line between 'err = ...' and 'if (err)' > > > > > > > > > Okay, i can change that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + if (!strcmp(default_locations[i].name, > > > > > > > > name)) > > > > > > > > + default_locations[i].devpart = > > > > > > > > devpart; > > > > > > > > + } > > > > > > > > + > > > > > > > > + return; > > > > > > > > +} > > > > > > > > + > > > > > > > > +/* > > > > > > > > + * Prepare firmware struct; > > > > > > > > + * return -ve if fail. > > > > > > > > + */ > > > > > > > > +static int _request_firmware_prepare(struct firmware > > > > > > > > **firmware_p, > > > > > > > > + const char *name, > > > > > > > > void > > > > > > > > *dbuf, > > > > > > > > + size_t size, u32 > > > > > > > > offset) > > > > > > > > +{ > > > > > > > > + struct firmware *firmware = NULL; > > > > > > > > + int ret = 0; > > > > > > > > + > > > > > > > > + if (!name || name[0] == '\0') > > > > > > > > + ret = -EINVAL; > > > > > > > > + > > > > > > > Is it really useful to continue here initializing the > > > > > > > 'firmware' > > > > > > > struct and returning an error at the end? > > > > > > > > > > > > > I try to keep it very close to Linux firmware loader. When > > > > > > more > > > > > > API > > > > > > ported in from Linux in future, this can be helper > > > > > > function. > > > > > > Anyway, i > > > > > > have no strong opinion, i can move to caller if you guys > > > > > > think > > > > > > that > > > > > > is > > > > > > better. > > > > > The Linux firmware loader has this: > > > > > > > > > > > > > > > > > > > > > > > > if (!name || name[0] == '\0') { > > > > > > ret = -EINVAL; > > > > > > goto out; > > > > > > } > > > > > Note the 'goto out' which is missing in your code. > > > > > If following the Linux code closely, you would have to set > > > > > *firmware_p > > > > > to NULL and exit in this case. > > > > > > > > > I can set the *firmware_p to NUll in failing case. But, i > > > > checked > > > > the > > > > static int _request_firmware_prepare in Linux, there is no goto > > > > out > > > > error handling method in the function. Fyi, there is no > > > > allocated > > > > memory release in U-Boot. > > > > https://elixir.free-electrons.com/linux/v4.13.15/source/drivers > > > > /bas > > > > e/fi > > > > rmware_class.c#L1191 > > > > > > > > > > > > > > > > > > I was referring to _request_firmware() which calls > > > request_firmware_prepare() and does the checking of 'name' as > > > your > > > code does. > > > > > Ohh.....i skipped _request_firmware() because there is no error > > handling required in U-Boot such as memory release. For the > > checking at > > both firmware_p and name, i would rather to merge them into > > request_firware_prepare() because they are related to each other > > instead of creating one function just for checking purpose. What do > > you > > think? > > > I don't mind whether you combine those functions or not as long as > you > reproduce the same functionality which your patch currently does not > do. > Okay. SO i will add the checking on firmware_p whether is NULL, and assign NULL to *firmware_P if calloc is failed. There is no goout since there is no error handling required. > > Lothar Waßmann
Hi, On Fri, 8 Dec 2017 05:25:58 +0000 Chee, Tien Fong wrote: > On Kha, 2017-12-07 at 10:00 +0100, Lothar Waßmann wrote: > > Hi, > > > > On Thu, 7 Dec 2017 08:10:06 +0000 Chee, Tien Fong wrote: > > > > > > On Kha, 2017-12-07 at 08:49 +0100, Lothar Waßmann wrote: > > > > > > > > Hi, > > > > > > > > On Thu, 7 Dec 2017 05:29:24 +0000 Chee, Tien Fong wrote: > > > > > > > > > > > > > > > On Rab, 2017-12-06 at 12:00 +0100, Lothar Waßmann wrote: > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Wed, 6 Dec 2017 10:06:21 +0000 Chee, Tien Fong wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sel, 2017-12-05 at 09:53 +0100, Lothar Waßmann wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Tue, 5 Dec 2017 15:57:57 +0800 tien.fong.chee@intel.c > > > > > > > > om > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com> > > > > > > > > > > > > > > > > > > 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 <tien.fong.chee@intel.com > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > common/Makefile | 1 + > > > > > > > > > common/fs_loader.c | 304 > > > > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > > include/fs_loader.h | 30 ++++++ > > > > > > > > > 3 files changed, 335 insertions(+) > > > > > > > > > create mode 100644 common/fs_loader.c > > > > > > > > > create mode 100644 include/fs_loader.h > > > > > > > > > > > > > > > > > > diff --git a/common/Makefile b/common/Makefile > > > > > > > > > index 801ea31..419e915 100644 > > > > > > > > > --- a/common/Makefile > > > > > > > > > +++ b/common/Makefile > > > > > > > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o > > > > > > > > > obj-y += command.o > > > > > > > > > obj-y += s_record.o > > > > > > > > > obj-y += xyzModem.o > > > > > > > > > +obj-y += fs_loader.o > > > > > > > > > diff --git a/common/fs_loader.c b/common/fs_loader.c > > > > > > > > > new file mode 100644 > > > > > > > > > index 0000000..04f682b > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/common/fs_loader.c > > > > > > > > > @@ -0,0 +1,304 @@ > > > > > > > > > +/* > > > > > > > > > + * Copyright (C) 2017 Intel Corporation <www.intel.com > > > > > > > > > > > > > > > > > > > + * > > > > > > > > > + * SPDX-License-Identifier: GPL-2.0 > > > > > > > > > + */ > > > > > > > > > + > > > > > > > > > +#include <common.h> > > > > > > > > > +#include <errno.h> > > > > > > > > > +#include <fs.h> > > > > > > > > > +#include <fs_loader.h> > > > > > > > > > +#include <nand.h> > > > > > > > > > +#include <sata.h> > > > > > > > > > +#include <spi.h> > > > > > > > > > +#include <spi_flash.h> > > > > > > > > > +#include <spl.h> > > > > > > > > > +#include <linux/string.h> > > > > > > > > > +#include <usb.h> > > > > > > > > > + > > > > > > > > > +static struct device_location default_locations[] = { > > > > > > > > > + { > > > > > > > > > + .name = "mmc", > > > > > > > > > + .devpart = "0:1", > > > > > > > > > + }, > > > > > > > > > + { > > > > > > > > > + .name = "usb", > > > > > > > > > + .devpart = "0:1", > > > > > > > > > + }, > > > > > > > > > + { > > > > > > > > > + .name = "sata", > > > > > > > > > + .devpart = "0:1", > > > > > > > > > + }, > > > > > > > > > +}; > > > > > > > > > + > > > > > > > > > +/* USB build is not supported yet in SPL */ > > > > > > > > > +#ifndef CONFIG_SPL_BUILD > > > > > > > > > +#ifdef CONFIG_USB_STORAGE > > > > > > > > > +static int init_usb(void) > > > > > > > > > +{ > > > > > > > > > + int err = 0; > > > > > > > > > > > > > > > > > Useless initialization. > > > > > > > > > > > > > > > noted. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + err = usb_init(); > > > > > > > > > + > > > > > > > > > + if (err) > > > > > > > > > > > > > > > > > Unnecessary blank line. > > > > > > > > > > > > > > > Sorry, i'm not catching you because there is no blank line > > > > > > > between > > > > > > > "if" > > > > > > > and "return" > > > > > > > > > > > > > I meant the blank line between 'err = ...' and 'if (err)' > > > > > > > > > > > Okay, i can change that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + if (!strcmp(default_locations[i].name, > > > > > > > > > name)) > > > > > > > > > + default_locations[i].devpart = > > > > > > > > > devpart; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + return; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > +/* > > > > > > > > > + * Prepare firmware struct; > > > > > > > > > + * return -ve if fail. > > > > > > > > > + */ > > > > > > > > > +static int _request_firmware_prepare(struct firmware > > > > > > > > > **firmware_p, > > > > > > > > > + const char *name, > > > > > > > > > void > > > > > > > > > *dbuf, > > > > > > > > > + size_t size, u32 > > > > > > > > > offset) > > > > > > > > > +{ > > > > > > > > > + struct firmware *firmware = NULL; > > > > > > > > > + int ret = 0; > > > > > > > > > + > > > > > > > > > + if (!name || name[0] == '\0') > > > > > > > > > + ret = -EINVAL; > > > > > > > > > + > > > > > > > > Is it really useful to continue here initializing the > > > > > > > > 'firmware' > > > > > > > > struct and returning an error at the end? > > > > > > > > > > > > > > > I try to keep it very close to Linux firmware loader. When > > > > > > > more > > > > > > > API > > > > > > > ported in from Linux in future, this can be helper > > > > > > > function. > > > > > > > Anyway, i > > > > > > > have no strong opinion, i can move to caller if you guys > > > > > > > think > > > > > > > that > > > > > > > is > > > > > > > better. > > > > > > The Linux firmware loader has this: > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (!name || name[0] == '\0') { > > > > > > > ret = -EINVAL; > > > > > > > goto out; > > > > > > > } > > > > > > Note the 'goto out' which is missing in your code. > > > > > > If following the Linux code closely, you would have to set > > > > > > *firmware_p > > > > > > to NULL and exit in this case. > > > > > > > > > > > I can set the *firmware_p to NUll in failing case. But, i > > > > > checked > > > > > the > > > > > static int _request_firmware_prepare in Linux, there is no goto > > > > > out > > > > > error handling method in the function. Fyi, there is no > > > > > allocated > > > > > memory release in U-Boot. > > > > > https://elixir.free-electrons.com/linux/v4.13.15/source/drivers > > > > > /bas > > > > > e/fi > > > > > rmware_class.c#L1191 > > > > > > > > > > > > > > > > > > > > > > I was referring to _request_firmware() which calls > > > > request_firmware_prepare() and does the checking of 'name' as > > > > your > > > > code does. > > > > > > > Ohh.....i skipped _request_firmware() because there is no error > > > handling required in U-Boot such as memory release. For the > > > checking at > > > both firmware_p and name, i would rather to merge them into > > > request_firware_prepare() because they are related to each other > > > instead of creating one function just for checking purpose. What do > > > you > > > think? > > > > > I don't mind whether you combine those functions or not as long as > > you > > reproduce the same functionality which your patch currently does not > > do. > > > Okay. SO i will add the checking on firmware_p whether is NULL, and > assign NULL to *firmware_P if calloc is failed. There is no goout since > there is no error handling required. > > You should also exit early setting *firmware_p to NULL when the 'name' check fails, like the Linux code does! I.e.: + struct firmware *firmware = NULL; + + *firmware_p = NULL; + if (!name || name[0] == '\0') + return -EINVAL; + + *firmware_p = firmware = calloc(1, sizeof(*firmware)); + if (!firmware) { + printf("%s: calloc(struct firmware) failed\n", __func__); + return -ENOMEM; + } + + firmware->name = name; + firmware->data = dbuf; + firmware->size = size; + firmware->offset = offset; + + return 0; Lothar Waßmann
diff --git a/common/Makefile b/common/Makefile index 801ea31..419e915 100644 --- a/common/Makefile +++ b/common/Makefile @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o obj-y += command.o obj-y += s_record.o obj-y += xyzModem.o +obj-y += fs_loader.o diff --git a/common/fs_loader.c b/common/fs_loader.c new file mode 100644 index 0000000..04f682b --- /dev/null +++ b/common/fs_loader.c @@ -0,0 +1,304 @@ +/* + * Copyright (C) 2017 Intel Corporation <www.intel.com> + * + * SPDX-License-Identifier: GPL-2.0 + */ + +#include <common.h> +#include <errno.h> +#include <fs.h> +#include <fs_loader.h> +#include <nand.h> +#include <sata.h> +#include <spi.h> +#include <spi_flash.h> +#include <spl.h> +#include <linux/string.h> +#include <usb.h> + +static struct device_location default_locations[] = { + { + .name = "mmc", + .devpart = "0:1", + }, + { + .name = "usb", + .devpart = "0:1", + }, + { + .name = "sata", + .devpart = "0:1", + }, +}; + +/* USB build is not supported yet in SPL */ +#ifndef CONFIG_SPL_BUILD +#ifdef CONFIG_USB_STORAGE +static int init_usb(void) +{ + int err = 0; + + err = usb_init(); + + if (err) + return err; + +#ifndef CONFIG_DM_USB + err = usb_stor_scan(1) < 0 ? -ENODEV : 0; +#endif + + return err; +} +#else +static int init_usb(void) +{ + printf("Error: Cannot load flash image: no USB support\n"); + return -ENOSYS; +} +#endif +#endif + +#ifdef CONFIG_SATA +static int init_storage_sata(void) +{ + return sata_probe(0); +} +#else +static int init_storage_sata(void) +{ + printf("Error: Cannot load image: no SATA support\n"); + return -ENOSYS; +} +#endif + +#ifdef CONFIG_CMD_UBIFS +static int mount_ubifs(struct device_location *location) +{ + int ret = 0; + char cmd[32]; + + sprintf(cmd, "ubi part %s", location->mtdpart); + + ret = run_command(cmd, 0); + + if (ret) + return ret; + + sprintf(cmd, "ubifsmount %s", location->ubivol); + + ret = run_command(cmd, 0); + + return res; +} + +static int umount_ubifs(void) +{ + return run_command("ubifsumount", 0); +} +#else +static int mount_ubifs(struct device_location *location) +{ + printf("Error: Cannot load image: no UBIFS support\n"); + return -ENOSYS; +} + +static int umount_ubifs(void) +{ + printf("Error: Cannot unmount UBIFS: no UBIFS support\n"); + return -ENOSYS; +} +#endif + +#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD) +static int init_mmc(void) +{ + /* Just for the case MMC is not yet initialized */ + struct mmc *mmc = NULL; + int err = 0; + + spl_mmc_find_device(&mmc, spl_boot_device()); + + err = mmc_init(mmc); + + if (err) { + printf("spl: mmc init failed with error: %d\n", err); + return err; + } + + return err; +} +#else +static int init_mmc(void) +{ + /* Expect somewhere already initialize MMC */ + return 0; +} +#endif + +static int select_fs_dev(struct device_location *location) +{ + int ret = 0; + + if (!strcmp("mmc", location->name)) + ret = fs_set_blk_dev("mmc", location->devpart, FS_TYPE_ANY); + else if (!strcmp("usb", location->name)) + ret = fs_set_blk_dev("usb", location->devpart, FS_TYPE_ANY); + else if (!strcmp("sata", location->name)) + ret = fs_set_blk_dev("sata", location->devpart, FS_TYPE_ANY); + else if (!strcmp("ubi", location->name)) + if (location->ubivol != NULL) + ret = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS); + else + ret = -ENODEV; + else { + printf("Error: unsupported location storage.\n"); + return -ENODEV; + } + + if (ret) + printf("Error: could not access storage.\n"); + + return ret; +} + +static int init_storage_device(struct device_location *location) +{ + int ret = 0; +#ifndef CONFIG_SPL_BUILD + /* USB build is not supported yet in SPL */ + if (!strcmp("usb", location->name)) + ret = init_usb(); +#endif + + if (!strcmp("mmc", location->name)) + ret = init_mmc(); + + if (!strcmp("sata", location->name)) + ret = init_storage_sata(); + + if (location->ubivol != NULL) + ret = mount_ubifs(location); + + return ret; +} + +static void set_storage_devpart(char *name, char *devpart) +{ + int i; + u32 size; + + size = ARRAY_SIZE(default_locations); + + for (i = 0; i < size; i++) { + if (!strcmp(default_locations[i].name, name)) + default_locations[i].devpart = devpart; + } + + return; +} + +/* + * Prepare firmware struct; + * return -ve if fail. + */ +static int _request_firmware_prepare(struct firmware **firmware_p, + const char *name, void *dbuf, + size_t size, u32 offset) +{ + struct firmware *firmware = NULL; + int ret = 0; + + if (!name || name[0] == '\0') + ret = -EINVAL; + + *firmware_p = firmware = calloc(1, sizeof(*firmware)); + + if (!firmware) { + printf("%s: calloc(struct firmware) failed\n", __func__); + return -ENOMEM; + } + + firmware->name = name; + firmware->data = dbuf; + firmware->size = size; + firmware->offset = offset; + + return ret; +} + +/* + * fw_get_filesystem_firmware - load firmware into a allocated buffer + * @firmware_p: pointer to firmware image + * @location: An array of supported firmware location + * + * @return: size of total read + * -ve when error + */ +static int fw_get_filesystem_firmware(struct device_location *location, + struct firmware *firmware_p) +{ + loff_t actread; + char *dev_part; + int ret = 0; + + dev_part = env_get("FW_DEV_PART"); + + if (dev_part) + set_storage_devpart(location->name, dev_part); + + ret = init_storage_device(location); + + if (ret) + goto out; + + select_fs_dev(location); + + ret = fs_read(firmware_p->name, (u32)firmware_p->data, + firmware_p->offset, firmware_p->size, &actread); + + if (ret || (actread != firmware_p->size)) { + printf("Error: %d Failed to read %s from flash %lld ", + ret, + firmware_p->name, + actread); + printf("!= %d.\n", firmware_p->size); + return -EPERM; + } else + ret = actread; + +out: + if (location->ubivol != NULL) + umount_ubifs(); + + return ret; +} +/* + * request_firmware_into_buf - load firmware into a previously allocated buffer + * @firmware_p: pointer to firmware image + * @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 + * + * This firmware is loaded directly into the buffer pointed to by @buf and + * the @firmware_p data member is pointed at @buf. + * + * @return: size of total read + * -ve when error + */ +int request_firmware_into_buf(struct firmware **firmware_p, + const char *name, + struct device_location *location, + void *buf, size_t size, u32 offset) +{ + int ret = 0; + + ret = _request_firmware_prepare(firmware_p, name, buf, size, offset); + + if (ret < 0) /* error */ + return ret; + + ret = fw_get_filesystem_firmware(location, *firmware_p); + + return ret; +} diff --git a/include/fs_loader.h b/include/fs_loader.h new file mode 100644 index 0000000..dacc1f3 --- /dev/null +++ b/include/fs_loader.h @@ -0,0 +1,30 @@ +/* + * Copyright (C) 2017 Intel Corporation <www.intel.com> + * + * SPDX-License-Identifier: GPL-2.0 + */ +#ifndef _FS_LOADER_H_ +#define _FS_LOADER_H_ + +#include <linux/types.h> + +struct firmware { + size_t size; /* Size of a file */ + const u8 *data; /* Buffer for file */ + const char *name; /* Filename */ + u32 offset; /* Offset of reading a file */ + void *priv; /* Firmware loader private fields */ +}; + +struct device_location { + char *name; /* Such as mmc, usb,and sata. */ + char *devpart; /* Use the load command dev:part conventions */ + char *mtdpart; /* MTD partition for ubi part */ + char *ubivol; /* UBI volume-name for ubifsmount */ +}; + +int request_firmware_into_buf(struct firmware **firmware_p, + const char *name, + struct device_location *location, + void *buf, size_t size, u32 offset); +#endif