Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On 24.01.2018 06:13, Chee, Tien Fong wrote:
On Tue, 2018-01-23 at 10:13 +0100, Simon Goldschmidt wrote:
On 23.01.2018 09:31, Chee, Tien Fong wrote:
On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.c...@intel.com 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
---
common/Makefile| 1 +
common/fs_loader.c | 309
+
doc/README.firmware_loader | 76 +++
include/fs_loader.h| 28
4 files changed, 414 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
diff --git a/common/Makefile b/common/Makefile
index cec506f..2934221 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
diff --git a/include/fs_loader.h b/include/fs_loader.h
new file mode 100644
index 000..83a8b80
--- /dev/null
+++ b/include/fs_loader.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2017 Intel Corporation
+ *
+ * SPDX-License-Identifier:GPL-2.0
+ */
+#ifndef _FS_LOADER_H_
+#define _FS_LOADER_H_
+
+#include
+
+struct firmware {
+ size_t size;/* Size of a file */
+ const u8 *data; /* Buffer for file */
+ void *priv; /* Firmware loader private
fields */
+};
+
+struct device_location {
+ char *name; /* Such as mmc, usb,and sata. */
Would it make sense to use 'enum if_type' from blk.h here instead
of
a
"magic" name? Or are the constants passed here "well-known"
enough
to
hide them?
This structure is declared such way so that it can be compatible
with
common/splash.c. It also much more easy to port fs_loader into
common/splash_source.c in later.
OK, but reading splash_source.c, it seems to me that 'enum
splash_storage' is used to detect the device to load from, not 'char
*name'.
However, since these constant strings already seem to be "common
knowledge" for callers of fs.h functions, it might be OK to use them
in
the firmware loader, too. I just wanted to point this out while this
is
in the reviewing process.
Actually this is common interface name used in any fs related. You can
refer more details regarding @ifname in include/fs.h and include part.h
.
So, why this interface name must be in characters string?
It's actually serving in 2 main purposes, and 1 minor purpose:
1. console environment, most commands using those interface name as
their arguments in characters string format such as FPGA loadfs
command.
2. These command interface name is one of the characters string
argument of fs_set_blk_dev too.
3. As identity of default location and its dev_part configuration.
Right. I see that there does not seem to be a way to provide the
interface type to fs.h without using a string. In most cases, this
string is provided by the environment or via console, so I see where
that comes from. It's probably not up to this patch to change it.
Regards,
Simon
+ 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On Tue, 2018-01-23 at 10:13 +0100, Simon Goldschmidt wrote:
> On 23.01.2018 09:31, Chee, Tien Fong wrote:
> >
> > On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
> > >
> > > On 27.12.2017 06:04, tien.fong.c...@intel.com 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
> > > > ---
> > > > common/Makefile| 1 +
> > > > common/fs_loader.c | 309
> > > > +
> > > > doc/README.firmware_loader | 76 +++
> > > > include/fs_loader.h| 28
> > > > 4 files changed, 414 insertions(+)
> > > > create mode 100644 common/fs_loader.c
> > > > create mode 100644 doc/README.firmware_loader
> > > > create mode 100644 include/fs_loader.h
> > > >
> > > > diff --git a/common/Makefile b/common/Makefile
> > > > index cec506f..2934221 100644
> > > > --- a/common/Makefile
> > > > +++ b/common/Makefile
> > > > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
> > >
> > >
> > > >
> > > > diff --git a/include/fs_loader.h b/include/fs_loader.h
> > > > new file mode 100644
> > > > index 000..83a8b80
> > > > --- /dev/null
> > > > +++ b/include/fs_loader.h
> > > > @@ -0,0 +1,28 @@
> > > > +/*
> > > > + * Copyright (C) 2017 Intel Corporation
> > > > + *
> > > > + * SPDX-License-Identifier:GPL-2.0
> > > > + */
> > > > +#ifndef _FS_LOADER_H_
> > > > +#define _FS_LOADER_H_
> > > > +
> > > > +#include
> > > > +
> > > > +struct firmware {
> > > > + size_t size;/* Size of a file */
> > > > + const u8 *data; /* Buffer for file */
> > > > + void *priv; /* Firmware loader private
> > > > fields */
> > > > +};
> > > > +
> > > > +struct device_location {
> > > > + char *name; /* Such as mmc, usb,and sata. */
> > > Would it make sense to use 'enum if_type' from blk.h here instead
> > > of
> > > a
> > > "magic" name? Or are the constants passed here "well-known"
> > > enough
> > > to
> > > hide them?
> > >
> > This structure is declared such way so that it can be compatible
> > with
> > common/splash.c. It also much more easy to port fs_loader into
> > common/splash_source.c in later.
> OK, but reading splash_source.c, it seems to me that 'enum
> splash_storage' is used to detect the device to load from, not 'char
> *name'.
>
> However, since these constant strings already seem to be "common
> knowledge" for callers of fs.h functions, it might be OK to use them
> in
> the firmware loader, too. I just wanted to point this out while this
> is
> in the reviewing process.
>
Actually this is common interface name used in any fs related. You can
refer more details regarding @ifname in include/fs.h and include part.h
.
So, why this interface name must be in characters string?
It's actually serving in 2 main purposes, and 1 minor purpose:
1. console environment, most commands using those interface name as
their arguments in characters string format such as FPGA loadfs
command.
2. These command interface name is one of the characters string
argument of fs_set_blk_dev too.
3. As identity of default location and its dev_part configuration.
> >
> > >
> > > Regards,
> > > Simon
> > >
> > > >
> > > > + 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On 23.01.2018 09:31, Chee, Tien Fong wrote:
On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.c...@intel.com 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
---
common/Makefile| 1 +
common/fs_loader.c | 309
+
doc/README.firmware_loader | 76 +++
include/fs_loader.h| 28
4 files changed, 414 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
diff --git a/common/Makefile b/common/Makefile
index cec506f..2934221 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
diff --git a/include/fs_loader.h b/include/fs_loader.h
new file mode 100644
index 000..83a8b80
--- /dev/null
+++ b/include/fs_loader.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2017 Intel Corporation
+ *
+ * SPDX-License-Identifier:GPL-2.0
+ */
+#ifndef _FS_LOADER_H_
+#define _FS_LOADER_H_
+
+#include
+
+struct firmware {
+ size_t size;/* Size of a file */
+ const u8 *data; /* Buffer for file */
+ void *priv; /* Firmware loader private
fields */
+};
+
+struct device_location {
+ char *name; /* Such as mmc, usb,and sata. */
Would it make sense to use 'enum if_type' from blk.h here instead of
a
"magic" name? Or are the constants passed here "well-known" enough
to
hide them?
This structure is declared such way so that it can be compatible with
common/splash.c. It also much more easy to port fs_loader into
common/splash_source.c in later.
OK, but reading splash_source.c, it seems to me that 'enum
splash_storage' is used to detect the device to load from, not 'char *name'.
However, since these constant strings already seem to be "common
knowledge" for callers of fs.h functions, it might be OK to use them in
the firmware loader, too. I just wanted to point this out while this is
in the reviewing process.
Regards,
Simon
+ 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On Tue, 2018-01-23 at 08:58 +0100, Simon Goldschmidt wrote:
> On 27.12.2017 06:04, tien.fong.c...@intel.com 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
> > ---
> > common/Makefile| 1 +
> > common/fs_loader.c | 309
> > +
> > doc/README.firmware_loader | 76 +++
> > include/fs_loader.h| 28
> > 4 files changed, 414 insertions(+)
> > create mode 100644 common/fs_loader.c
> > create mode 100644 doc/README.firmware_loader
> > create mode 100644 include/fs_loader.h
> >
> > diff --git a/common/Makefile b/common/Makefile
> > index cec506f..2934221 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
>
>
> >
> > diff --git a/include/fs_loader.h b/include/fs_loader.h
> > new file mode 100644
> > index 000..83a8b80
> > --- /dev/null
> > +++ b/include/fs_loader.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright (C) 2017 Intel Corporation
> > + *
> > + * SPDX-License-Identifier:GPL-2.0
> > + */
> > +#ifndef _FS_LOADER_H_
> > +#define _FS_LOADER_H_
> > +
> > +#include
> > +
> > +struct firmware {
> > + size_t size;/* Size of a file */
> > + const u8 *data; /* Buffer for file */
> > + void *priv; /* Firmware loader private
> > fields */
> > +};
> > +
> > +struct device_location {
> > + char *name; /* Such as mmc, usb,and sata. */
> Would it make sense to use 'enum if_type' from blk.h here instead of
> a
> "magic" name? Or are the constants passed here "well-known" enough
> to
> hide them?
>
This structure is declared such way so that it can be compatible with
common/splash.c. It also much more easy to port fs_loader into
common/splash_source.c in later.
> Regards,
> Simon
>
> >
> > + 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On 27.12.2017 06:04, tien.fong.c...@intel.com 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
---
common/Makefile| 1 +
common/fs_loader.c | 309 +
doc/README.firmware_loader | 76 +++
include/fs_loader.h| 28
4 files changed, 414 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
diff --git a/common/Makefile b/common/Makefile
index cec506f..2934221 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -130,3 +130,4 @@ obj-$(CONFIG_CMD_DFU) += dfu.o
diff --git a/include/fs_loader.h b/include/fs_loader.h
new file mode 100644
index 000..83a8b80
--- /dev/null
+++ b/include/fs_loader.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2017 Intel Corporation
+ *
+ * SPDX-License-Identifier:GPL-2.0
+ */
+#ifndef _FS_LOADER_H_
+#define _FS_LOADER_H_
+
+#include
+
+struct firmware {
+ size_t size;/* Size of a file */
+ const u8 *data; /* Buffer for file */
+ void *priv; /* Firmware loader private fields */
+};
+
+struct device_location {
+ char *name; /* Such as mmc, usb,and sata. */
Would it make sense to use 'enum if_type' from blk.h here instead of a
"magic" name? Or are the constants passed here "well-known" enough to
hide them?
Regards,
Simon
+ 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On 23.01.2018 07:28, Chee, Tien Fong wrote:
On Mon, 2018-01-22 at 12:41 +0100, Simon Goldschmidt wrote:
On 22.01.2018 09:08, Chee, Tien Fong wrote:
On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.c...@intel.com 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
---
common/Makefile| 1 +
common/fs_loader.c | 309
+
doc/README.firmware_loader | 76 +++
include/fs_loader.h| 28
4 files changed, 414 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
+#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;
+
+ 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;
+}
I see two problems here: First, you're ignoring the return value
of
spl_mmc_find_device() and initialize 'mmc' to NULL instead.
Wouldn't
it
be better to let 'mmc' be uninitialized and return the error code
returned by spl_mmc_find_device() if there is one?
Yeah, you are right, i should add the check on the return value. I
think that would better to initialize NULL to mmc, because there is
no
checking on the mmc in spl_mmc_find_device(), so that is possible
memory access violation/abort can be happended if unknown value in
mmc
pointer.
Second, using spl_boot_device() would prevent making the loader
work
on
mach-socfpga when spl is not loaded from mmc, right?
E.g. for the case I'm currently trying to fix (boot from qspi),
this
loader would not work although there's technically no reason
since
the
platform only has one mmc. The call to spl_boot_device() could be
replaced by the exact value here for platforms that only have one
mmc. I
don't know how to fix that, though.
The main purpose here is to initialize the mmc driver. So which
storage
user wants to load the file is totally depend what storage such as
mmc
user defines in location->name. Loader would init the storage based
on
the storage defined in location->name before accessing it. Since
the
loader only support file system at this moment, i would suggest FAT
fs
for mmc and ubi fs for qspi.
What I meant to say is this: at least on mach-socfpga, from reading
the
code, I cannot load a file from mmc when booting from qspi, as
'spl_boot_device' returns 'BOOT_DEVICE_SPI' in that case, although I
need to pass 'BOOT_DEVICE_MMC1' to 'spl_mmc_find_device'. Or am I
wrong
here?
Okay, i got you.
Yeah, you are right for use case if you need to load the file from mmc
during SPL boot and SPL is not loaded from mmc.
Since the spl_boot_device is platform dependent, you may try to add
some state machine so that this function can return correct device
type.
Secondly, you can use this function in U-Boot instead of SPL.
You're right on that. Thinking about it, I would probably use it from
U-Boot, not from SPL...
Lastly, i can declare __Weak to init_mmc, so that user can define their
own implementation.
At least somehow allowing to override which device is passed to
'spl_mmc_find_device' might be good, instead of hardcoding it to
'spl_boot_device'...
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On Mon, 2018-01-22 at 12:41 +0100, Simon Goldschmidt wrote:
> On 22.01.2018 09:08, Chee, Tien Fong wrote:
> >
> > On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
> > >
> > > On 27.12.2017 06:04, tien.fong.c...@intel.com 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
> > > > ---
> > > > common/Makefile| 1 +
> > > > common/fs_loader.c | 309
> > > > +
> > > > doc/README.firmware_loader | 76 +++
> > > > include/fs_loader.h| 28
> > > > 4 files changed, 414 insertions(+)
> > > > create mode 100644 common/fs_loader.c
> > > > create mode 100644 doc/README.firmware_loader
> > > > create mode 100644 include/fs_loader.h
> > >
> > >
> > > >
> > > > +#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;
> > > > +
> > > > + 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;
> > > > +}
> > > I see two problems here: First, you're ignoring the return value
> > > of
> > > spl_mmc_find_device() and initialize 'mmc' to NULL instead.
> > > Wouldn't
> > > it
> > > be better to let 'mmc' be uninitialized and return the error code
> > > returned by spl_mmc_find_device() if there is one?
> > >
> > Yeah, you are right, i should add the check on the return value. I
> > think that would better to initialize NULL to mmc, because there is
> > no
> > checking on the mmc in spl_mmc_find_device(), so that is possible
> > memory access violation/abort can be happended if unknown value in
> > mmc
> > pointer.
> >
> > >
> > > Second, using spl_boot_device() would prevent making the loader
> > > work
> > > on
> > > mach-socfpga when spl is not loaded from mmc, right?
> > >
> > > E.g. for the case I'm currently trying to fix (boot from qspi),
> > > this
> > > loader would not work although there's technically no reason
> > > since
> > > the
> > > platform only has one mmc. The call to spl_boot_device() could be
> > > replaced by the exact value here for platforms that only have one
> > > mmc. I
> > > don't know how to fix that, though.
> > >
> > The main purpose here is to initialize the mmc driver. So which
> > storage
> > user wants to load the file is totally depend what storage such as
> > mmc
> > user defines in location->name. Loader would init the storage based
> > on
> > the storage defined in location->name before accessing it. Since
> > the
> > loader only support file system at this moment, i would suggest FAT
> > fs
> > for mmc and ubi fs for qspi.
> What I meant to say is this: at least on mach-socfpga, from reading
> the
> code, I cannot load a file from mmc when booting from qspi, as
> 'spl_boot_device' returns 'BOOT_DEVICE_SPI' in that case, although I
> need to pass 'BOOT_DEVICE_MMC1' to 'spl_mmc_find_device'. Or am I
> wrong
> here?
>
Okay, i got you.
Yeah, you are right for use case if you need to load the file from mmc
during SPL boot and SPL is not loaded from mmc.
Since the spl_boot_device is platform dependent, you may try to add
some state machine so that this function can return correct device
type.
Secondly, you can use this function in U-Boot instead of SPL.
Lastly, i can declare __Weak to init_mmc, so that user can define their
own implementation.
> Regards,
> Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On Mon, 2018-01-22 at 09:44 +0100, Lothar Waßmann wrote:
> Hi,
>
> On Mon, 22 Jan 2018 07:11:37 + Chee, Tien Fong wrote:
> >
> > On Thu, 2018-01-18 at 12:12 +0100, Marek Vasut wrote:
> > >
> > > On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
> > > >
> > > >
> > > > On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
> > > > >
> > > > >
> > > > > On 12/27/2017 06:04 AM, tien.fong.c...@intel.com wrote:
> > > > >
> > > > > Whoa, this improved substantially since last time I checked.
> > > > > Minor
> > > > > nitpicks below.
> > > > >
> > > > > [...]
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > +/* USB build is not supported yet in SPL */
> > > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > > +#ifdef CONFIG_USB_STORAGE
> > > > > > +static int init_usb(void)
> > > > > > +{
> > > > > > + int err;
> > > > > > +
> > > > > > + err = usb_init();
> > > > > > + if (err)
> > > > > > + return err;
> > > > > > +
> > > > > > +#ifndef CONFIG_DM_USB
> > > > > > + err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
> > > > > if (err)
> > > > > return err;
> > > > > ?
> > > > >
> > > > This is last line code of the function, so it's always return
> > > > the
> > > > result regardless error or not.
> > > You are rewriting the true error code with -ENODEV instead of
> > > propagating it.
> > >
> > Ohhare you saying to change the codes as shown in below:
> >
> > err = usb_stor_scan(1);
> > if (err)
> > return err;
> >
> usb_stor_scan() does not return a sensible error code, but '-1' if no
> device was found. This should be changed to -ENODEV then!
>
Okay, so this should be fixed in usb_stor_scan() function.
>
> Lothar Waßmann
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On 01/22/2018 08:11 AM, Chee, Tien Fong wrote:
[...]
>>> This is last line code of the function, so it's always return the
>>> result regardless error or not.
>> You are rewriting the true error code with -ENODEV instead of
>> propagating it.
>>
> Ohhare you saying to change the codes as shown in below:
>
> err = usb_stor_scan(1);
> if (err)
> return err;
Right
[...]
> +static int umount_ubifs(void)
> +{
> + return run_command("ubifsumount", 0);
Just call the function directly ?
>>> There are some checking like ubifs_initialized in the cmd/ubifs.c.
>>> Direct callng the function would bypass those checking.
>> Then factor those out into a function you can all and call that
>> function.
>>
> Just for curious, is it worth to factor those into a function? Does it
> help to boost the performance or for other purpose?
It just makes no sense to involve the whole command machinery if you can
call a function which does exactly the same.
[...]
--
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On 22.01.2018 09:08, Chee, Tien Fong wrote:
On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
On 27.12.2017 06:04, tien.fong.c...@intel.com 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
---
common/Makefile| 1 +
common/fs_loader.c | 309
+
doc/README.firmware_loader | 76 +++
include/fs_loader.h| 28
4 files changed, 414 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
+#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;
+
+ 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;
+}
I see two problems here: First, you're ignoring the return value of
spl_mmc_find_device() and initialize 'mmc' to NULL instead. Wouldn't
it
be better to let 'mmc' be uninitialized and return the error code
returned by spl_mmc_find_device() if there is one?
Yeah, you are right, i should add the check on the return value. I
think that would better to initialize NULL to mmc, because there is no
checking on the mmc in spl_mmc_find_device(), so that is possible
memory access violation/abort can be happended if unknown value in mmc
pointer.
Second, using spl_boot_device() would prevent making the loader work
on
mach-socfpga when spl is not loaded from mmc, right?
E.g. for the case I'm currently trying to fix (boot from qspi), this
loader would not work although there's technically no reason since
the
platform only has one mmc. The call to spl_boot_device() could be
replaced by the exact value here for platforms that only have one
mmc. I
don't know how to fix that, though.
The main purpose here is to initialize the mmc driver. So which storage
user wants to load the file is totally depend what storage such as mmc
user defines in location->name. Loader would init the storage based on
the storage defined in location->name before accessing it. Since the
loader only support file system at this moment, i would suggest FAT fs
for mmc and ubi fs for qspi.
What I meant to say is this: at least on mach-socfpga, from reading the
code, I cannot load a file from mmc when booting from qspi, as
'spl_boot_device' returns 'BOOT_DEVICE_SPI' in that case, although I
need to pass 'BOOT_DEVICE_MMC1' to 'spl_mmc_find_device'. Or am I wrong
here?
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
Hi,
On Mon, 22 Jan 2018 07:11:37 + Chee, Tien Fong wrote:
> On Thu, 2018-01-18 at 12:12 +0100, Marek Vasut wrote:
> > On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
> > >
> > > On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
> > > >
> > > > On 12/27/2017 06:04 AM, tien.fong.c...@intel.com wrote:
> > > >
> > > > Whoa, this improved substantially since last time I checked.
> > > > Minor
> > > > nitpicks below.
> > > >
> > > > [...]
> > > >
> > > > >
> > > > >
> > > > > +/* USB build is not supported yet in SPL */
> > > > > +#ifndef CONFIG_SPL_BUILD
> > > > > +#ifdef CONFIG_USB_STORAGE
> > > > > +static int init_usb(void)
> > > > > +{
> > > > > + int err;
> > > > > +
> > > > > + err = usb_init();
> > > > > + if (err)
> > > > > + return err;
> > > > > +
> > > > > +#ifndef CONFIG_DM_USB
> > > > > + err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
> > > > if (err)
> > > > return err;
> > > > ?
> > > >
> > > This is last line code of the function, so it's always return the
> > > result regardless error or not.
> > You are rewriting the true error code with -ENODEV instead of
> > propagating it.
> >
> Ohhare you saying to change the codes as shown in below:
>
> err = usb_stor_scan(1);
> if (err)
> return err;
>
usb_stor_scan() does not return a sensible error code, but '-1' if no
device was found. This should be changed to -ENODEV then!
Lothar Waßmann
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On Thu, 2018-01-18 at 06:57 +0100, Simon Goldschmidt wrote:
> On 27.12.2017 06:04, tien.fong.c...@intel.com 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
> > ---
> > common/Makefile| 1 +
> > common/fs_loader.c | 309
> > +
> > doc/README.firmware_loader | 76 +++
> > include/fs_loader.h| 28
> > 4 files changed, 414 insertions(+)
> > create mode 100644 common/fs_loader.c
> > create mode 100644 doc/README.firmware_loader
> > create mode 100644 include/fs_loader.h
>
>
> >
> > +#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;
> > +
> > + 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;
> > +}
> I see two problems here: First, you're ignoring the return value of
> spl_mmc_find_device() and initialize 'mmc' to NULL instead. Wouldn't
> it
> be better to let 'mmc' be uninitialized and return the error code
> returned by spl_mmc_find_device() if there is one?
>
Yeah, you are right, i should add the check on the return value. I
think that would better to initialize NULL to mmc, because there is no
checking on the mmc in spl_mmc_find_device(), so that is possible
memory access violation/abort can be happended if unknown value in mmc
pointer.
> Second, using spl_boot_device() would prevent making the loader work
> on
> mach-socfpga when spl is not loaded from mmc, right?
>
> E.g. for the case I'm currently trying to fix (boot from qspi), this
> loader would not work although there's technically no reason since
> the
> platform only has one mmc. The call to spl_boot_device() could be
> replaced by the exact value here for platforms that only have one
> mmc. I
> don't know how to fix that, though.
>
The main purpose here is to initialize the mmc driver. So which storage
user wants to load the file is totally depend what storage such as mmc
user defines in location->name. Loader would init the storage based on
the storage defined in location->name before accessing it. Since the
loader only support file system at this moment, i would suggest FAT fs
for mmc and ubi fs for qspi.
> Regards,
> Simon
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On Thu, 2018-01-18 at 12:12 +0100, Marek Vasut wrote:
> On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
> >
> > On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
> > >
> > > On 12/27/2017 06:04 AM, tien.fong.c...@intel.com wrote:
> > >
> > > Whoa, this improved substantially since last time I checked.
> > > Minor
> > > nitpicks below.
> > >
> > > [...]
> > >
> > > >
> > > >
> > > > +/* USB build is not supported yet in SPL */
> > > > +#ifndef CONFIG_SPL_BUILD
> > > > +#ifdef CONFIG_USB_STORAGE
> > > > +static int init_usb(void)
> > > > +{
> > > > + int err;
> > > > +
> > > > + err = usb_init();
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > +#ifndef CONFIG_DM_USB
> > > > + err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
> > > if (err)
> > > return err;
> > > ?
> > >
> > This is last line code of the function, so it's always return the
> > result regardless error or not.
> You are rewriting the true error code with -ENODEV instead of
> propagating it.
>
Ohhare you saying to change the codes as shown in below:
err = usb_stor_scan(1);
if (err)
return err;
> > >
> > > >
> > > >
> > > > +#endif
> > > > +
> > > > + return err;
> > > > +}
> > > > +#else
> > > > +static int init_usb(void)
> > > > +{
> > > > + printf("Error: Cannot load flash image: no USB
> > > > support\n");
> > > debug() ? Fix globally ...
> > >
> > okay.
> > >
> > > >
> > > >
> > > > + 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;
> > > > + char cmd[32];
> > > > +
> > > > + sprintf(cmd, "ubi part %s", location->mtdpart);
> > > snprintf() ...
> > >
> > okay.
> > >
> > > >
> > > >
> > > > + ret = run_command(cmd, 0);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + sprintf(cmd, "ubifsmount %s", location->ubivol);
> > > > +
> > > > + ret = run_command(cmd, 0);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int umount_ubifs(void)
> > > > +{
> > > > + return run_command("ubifsumount", 0);
> > > Just call the function directly ?
> > >
> > There are some checking like ubifs_initialized in the cmd/ubifs.c.
> > Direct callng the function would bypass those checking.
> Then factor those out into a function you can all and call that
> function.
>
Just for curious, is it worth to factor those into a function? Does it
help to boost the performance or for other purpose?
> >
> > >
> > > >
> > > >
> > > > +}
> > > > +#else
> > > > +static int mount_ubifs(struct device_location *location)
> > > > +{
> > > > + printf("Error: Cannot load image: 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;
> > > > +
> > > > + 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;
> > > > +
> > > > + 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");
> > > > +
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On 01/18/2018 05:33 AM, Chee, Tien Fong wrote:
> On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
>> On 12/27/2017 06:04 AM, tien.fong.c...@intel.com wrote:
>>
>> Whoa, this improved substantially since last time I checked. Minor
>> nitpicks below.
>>
>> [...]
>>
>>>
>>> +/* USB build is not supported yet in SPL */
>>> +#ifndef CONFIG_SPL_BUILD
>>> +#ifdef CONFIG_USB_STORAGE
>>> +static int init_usb(void)
>>> +{
>>> + int err;
>>> +
>>> + err = usb_init();
>>> + if (err)
>>> + return err;
>>> +
>>> +#ifndef CONFIG_DM_USB
>>> + err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
>> if (err)
>> return err;
>> ?
>>
> This is last line code of the function, so it's always return the
> result regardless error or not.
You are rewriting the true error code with -ENODEV instead of
propagating it.
>>>
>>> +#endif
>>> +
>>> + return err;
>>> +}
>>> +#else
>>> +static int init_usb(void)
>>> +{
>>> + printf("Error: Cannot load flash image: no USB
>>> support\n");
>> debug() ? Fix globally ...
>>
> okay.
>>>
>>> + 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;
>>> + char cmd[32];
>>> +
>>> + sprintf(cmd, "ubi part %s", location->mtdpart);
>> snprintf() ...
>>
> okay.
>>>
>>> + ret = run_command(cmd, 0);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + sprintf(cmd, "ubifsmount %s", location->ubivol);
>>> +
>>> + ret = run_command(cmd, 0);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int umount_ubifs(void)
>>> +{
>>> + return run_command("ubifsumount", 0);
>> Just call the function directly ?
>>
> There are some checking like ubifs_initialized in the cmd/ubifs.c.
> Direct callng the function would bypass those checking.
Then factor those out into a function you can all and call that function.
>>>
>>> +}
>>> +#else
>>> +static int mount_ubifs(struct device_location *location)
>>> +{
>>> + printf("Error: Cannot load image: 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;
>>> +
>>> + 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;
>>> +
>>> + 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;
>>> +
>>> + if (!strcmp("mmc", location->name)) {
>>> + ret = init_mmc();
>>> + } else if (!strcmp("sata", location->name)) {
>>> + ret = init_storage_sata();
>>> + } else if (location->ubivol != NULL) {
>>> + ret = mount_ubifs(location);
>>> +#ifndef CONFIG_SPL_BUILD
>>> + /* USB build is not supported yet in SPL */
>>> + } else if (!strcmp("usb", location->name)) {
>>> + ret = init_usb();
>>> +#endif
>>> + } else {
>>> + printf("Error: no supported storage device is
>>> available.\n");
>>> + ret = -ENODEV;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void set_storage_devpart(char *name, char *devpart)
>>> +{
>>> + size_t i;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
>>> + if (!strcmp(default_locations[i].name, name))
>>> + default_locations[i].devpart = devpart;
>>> + }
>>> +}
>>> +
>>
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On 27.12.2017 06:04, tien.fong.c...@intel.com 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
---
common/Makefile| 1 +
common/fs_loader.c | 309 +
doc/README.firmware_loader | 76 +++
include/fs_loader.h| 28
4 files changed, 414 insertions(+)
create mode 100644 common/fs_loader.c
create mode 100644 doc/README.firmware_loader
create mode 100644 include/fs_loader.h
+#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;
+
+ 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;
+}
I see two problems here: First, you're ignoring the return value of
spl_mmc_find_device() and initialize 'mmc' to NULL instead. Wouldn't it
be better to let 'mmc' be uninitialized and return the error code
returned by spl_mmc_find_device() if there is one?
Second, using spl_boot_device() would prevent making the loader work on
mach-socfpga when spl is not loaded from mmc, right?
E.g. for the case I'm currently trying to fix (boot from qspi), this
loader would not work although there's technically no reason since the
platform only has one mmc. The call to spl_boot_device() could be
replaced by the exact value here for platforms that only have one mmc. I
don't know how to fix that, though.
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On Tue, 2018-01-16 at 15:41 +0100, Marek Vasut wrote:
> On 12/27/2017 06:04 AM, tien.fong.c...@intel.com wrote:
>
> Whoa, this improved substantially since last time I checked. Minor
> nitpicks below.
>
> [...]
>
> >
> > +/* USB build is not supported yet in SPL */
> > +#ifndef CONFIG_SPL_BUILD
> > +#ifdef CONFIG_USB_STORAGE
> > +static int init_usb(void)
> > +{
> > + int err;
> > +
> > + err = usb_init();
> > + if (err)
> > + return err;
> > +
> > +#ifndef CONFIG_DM_USB
> > + err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
> if (err)
> return err;
> ?
>
This is last line code of the function, so it's always return the
result regardless error or not.
> >
> > +#endif
> > +
> > + return err;
> > +}
> > +#else
> > +static int init_usb(void)
> > +{
> > + printf("Error: Cannot load flash image: no USB
> > support\n");
> debug() ? Fix globally ...
>
okay.
> >
> > + 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;
> > + char cmd[32];
> > +
> > + sprintf(cmd, "ubi part %s", location->mtdpart);
> snprintf() ...
>
okay.
> >
> > + ret = run_command(cmd, 0);
> > + if (ret)
> > + return ret;
> > +
> > + sprintf(cmd, "ubifsmount %s", location->ubivol);
> > +
> > + ret = run_command(cmd, 0);
> > +
> > + return ret;
> > +}
> > +
> > +static int umount_ubifs(void)
> > +{
> > + return run_command("ubifsumount", 0);
> Just call the function directly ?
>
There are some checking like ubifs_initialized in the cmd/ubifs.c.
Direct callng the function would bypass those checking.
> >
> > +}
> > +#else
> > +static int mount_ubifs(struct device_location *location)
> > +{
> > + printf("Error: Cannot load image: 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;
> > +
> > + 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;
> > +
> > + 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;
> > +
> > + if (!strcmp("mmc", location->name)) {
> > + ret = init_mmc();
> > + } else if (!strcmp("sata", location->name)) {
> > + ret = init_storage_sata();
> > + } else if (location->ubivol != NULL) {
> > + ret = mount_ubifs(location);
> > +#ifndef CONFIG_SPL_BUILD
> > + /* USB build is not supported yet in SPL */
> > + } else if (!strcmp("usb", location->name)) {
> > + ret = init_usb();
> > +#endif
> > + } else {
> > + printf("Error: no supported storage device is
> > available.\n");
> > + ret = -ENODEV;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void set_storage_devpart(char *name, char *devpart)
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
> > + if (!strcmp(default_locations[i].name, name))
> > + default_locations[i].devpart = devpart;
> > + }
> > +}
> > +
> > +/*
> > + * Prepare firmware struct;
> > + * return -ve if fail.
> Use kerneldoc and keep it consistent.
>
kerneldoc doesn't has explanation for this function, and this function
is not for user. Or you means i shouldn'
Re: [U-Boot] [PATCH v6 2/2] common: Generic firmware loader for file system
On 12/27/2017 06:04 AM, tien.fong.c...@intel.com wrote:
Whoa, this improved substantially since last time I checked. Minor
nitpicks below.
[...]
> +/* USB build is not supported yet in SPL */
> +#ifndef CONFIG_SPL_BUILD
> +#ifdef CONFIG_USB_STORAGE
> +static int init_usb(void)
> +{
> + int err;
> +
> + err = usb_init();
> + if (err)
> + return err;
> +
> +#ifndef CONFIG_DM_USB
> + err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
if (err)
return err;
?
> +#endif
> +
> + return err;
> +}
> +#else
> +static int init_usb(void)
> +{
> + printf("Error: Cannot load flash image: no USB support\n");
debug() ? Fix globally ...
> + 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;
> + char cmd[32];
> +
> + sprintf(cmd, "ubi part %s", location->mtdpart);
snprintf() ...
> + ret = run_command(cmd, 0);
> + if (ret)
> + return ret;
> +
> + sprintf(cmd, "ubifsmount %s", location->ubivol);
> +
> + ret = run_command(cmd, 0);
> +
> + return ret;
> +}
> +
> +static int umount_ubifs(void)
> +{
> + return run_command("ubifsumount", 0);
Just call the function directly ?
> +}
> +#else
> +static int mount_ubifs(struct device_location *location)
> +{
> + printf("Error: Cannot load image: 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;
> +
> + 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;
> +
> + 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;
> +
> + if (!strcmp("mmc", location->name)) {
> + ret = init_mmc();
> + } else if (!strcmp("sata", location->name)) {
> + ret = init_storage_sata();
> + } else if (location->ubivol != NULL) {
> + ret = mount_ubifs(location);
> +#ifndef CONFIG_SPL_BUILD
> + /* USB build is not supported yet in SPL */
> + } else if (!strcmp("usb", location->name)) {
> + ret = init_usb();
> +#endif
> + } else {
> + printf("Error: no supported storage device is available.\n");
> + ret = -ENODEV;
> + }
> +
> + return ret;
> +}
> +
> +static void set_storage_devpart(char *name, char *devpart)
> +{
> + size_t i;
> +
> + for (i = 0; i < ARRAY_SIZE(default_locations); i++) {
> + if (!strcmp(default_locations[i].name, name))
> + default_locations[i].devpart = devpart;
> + }
> +}
> +
> +/*
> + * Prepare firmware struct;
> + * return -ve if fail.
Use kerneldoc and keep it consistent.
> + */
> +static int _request_firmware_prepare(struct firmware **firmware_p,
> + const char *name, void *dbuf,
> + size_t size, u32 offset)
> +{
> + struct firmware *firmware;
> + struct firmware_priv *fw_priv;
> +
> + *firmware_p = NULL;
> +
> + if (!name || name[0] == '\0')
> + return -EINVAL;
> +
> + firmware = calloc(1, sizeof(*firmware));
> + if (!firmware) {
> + printf("%s: calloc(struct firmware) failed\n", __func__);
If malloc fails, you're screwed anyway and printf will likely fail too,
so drop it.
> + r
