Re: [U-Boot] [PATCH v10 4/4] common: Generic firmware loader for file system

[Tom Rini]

On Thu, Mar 15, 2018 at 07:18:20AM +, Chee, Tien Fong wrote:
> On Thu, 2018-03-08 at 14:04 -0700, Simon Glass wrote:
> > Hi,
> > 
> > On 7 March 2018 at 22:03, Chee, Tien Fong 
> > wrote:
> > > 
> > > On Tue, 2018-03-06 at 10:51 -0700, Simon Glass wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On 5 March 2018 at 02:43,   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 
> > > > > Reviewed-by: Lothar Waßmann 
> > > > > ---
> > > > >  common/Kconfig |  10 ++
> > > > >  common/Makefile|   1 +
> > > > >  common/fs_loader.c | 353
> > > > > +
> > > > >  doc/README.firmware_loader |  86 +++
> > > > >  include/fs_loader.h|  28 
> > > > >  5 files changed, 478 insertions(+)
> > > > >  create mode 100644 common/fs_loader.c
> > > > >  create mode 100644 doc/README.firmware_loader
> > > > >  create mode 100644 include/fs_loader.h
> > > > This looks fine as a concept but I am not keen on the
> > > > implementation.
> > > > 
> > > This patchset has been going through many rounds and a lot of time
> > > spending in review, and it is already working and being tested. I
> > > still
> > > have a lot subsequent patches pending on this patchset. I would
> > > suggest
> > > to accept this patchset, then we can enhance it to driver model in
> > > later.
> > > 
> > > > 
> > > > 1. It should use driver model (only) in U-Boot proper. If there
> > > > is
> > > > some SPL problem then add a specific function or feature for SPL.
> > > We can doing this in later since it is require sometime to figure
> > > out
> > > and testing.
> > > 
> > > > 
> > > > 2. It should not be necessary ti manually init subsystems -
> > > > driver
> > > > model does this for you
> > > This is for initializing storage driver in very early SPL, where
> > > loading from storage to configure some critical HW need to done
> > > first.
> > > 
> > > > 
> > > > 3. You can use the uclass name to find things
> > > Yeah, once it is converted to driver model, we can use the uclass
> > > for
> > > searching HW info in DTS.
> > > > 
> > > > 
> > > > Please let me know if you need more info.
> > Well I'm still not keen on this. Are you planning to do the
> > conversion? It seems like a lot of work for someone else to do.
> > 
> Okay, i will try to convert it into driver model.

Thanks!

-- 
Tom


signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v10 4/4] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Thu, 2018-03-08 at 14:04 -0700, Simon Glass wrote:
> Hi,
> 
> On 7 March 2018 at 22:03, Chee, Tien Fong 
> wrote:
> > 
> > On Tue, 2018-03-06 at 10:51 -0700, Simon Glass wrote:
> > > 
> > > Hi,
> > > 
> > > On 5 March 2018 at 02:43,   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 
> > > > Reviewed-by: Lothar Waßmann 
> > > > ---
> > > >  common/Kconfig |  10 ++
> > > >  common/Makefile|   1 +
> > > >  common/fs_loader.c | 353
> > > > +
> > > >  doc/README.firmware_loader |  86 +++
> > > >  include/fs_loader.h|  28 
> > > >  5 files changed, 478 insertions(+)
> > > >  create mode 100644 common/fs_loader.c
> > > >  create mode 100644 doc/README.firmware_loader
> > > >  create mode 100644 include/fs_loader.h
> > > This looks fine as a concept but I am not keen on the
> > > implementation.
> > > 
> > This patchset has been going through many rounds and a lot of time
> > spending in review, and it is already working and being tested. I
> > still
> > have a lot subsequent patches pending on this patchset. I would
> > suggest
> > to accept this patchset, then we can enhance it to driver model in
> > later.
> > 
> > > 
> > > 1. It should use driver model (only) in U-Boot proper. If there
> > > is
> > > some SPL problem then add a specific function or feature for SPL.
> > We can doing this in later since it is require sometime to figure
> > out
> > and testing.
> > 
> > > 
> > > 2. It should not be necessary ti manually init subsystems -
> > > driver
> > > model does this for you
> > This is for initializing storage driver in very early SPL, where
> > loading from storage to configure some critical HW need to done
> > first.
> > 
> > > 
> > > 3. You can use the uclass name to find things
> > Yeah, once it is converted to driver model, we can use the uclass
> > for
> > searching HW info in DTS.
> > > 
> > > 
> > > Please let me know if you need more info.
> Well I'm still not keen on this. Are you planning to do the
> conversion? It seems like a lot of work for someone else to do.
> 
Okay, i will try to convert it into driver model.

> Regards,
> Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v10 4/4] common: Generic firmware loader for file system

[Wolfgang Denk]

Dear Tien Fong,

In message <1520485397.10181.12.ca...@intel.com> you wrote:
>
> > This looks fine as a concept but I am not keen on the implementation.
> > 
> This patchset has been going through many rounds and a lot of time
> spending in review, and it is already working and being tested. I still

That's life

> have a lot subsequent patches pending on this patchset. I would suggest
> to accept this patchset, then we can enhance it to driver model in
> later.

No.  The usual approach is to _first_ clean up the code, _before_ it
gets added.  there have been just too many cases that interest in
cleaning up disappeared quickly once the unclean version was pulled
into mainline.

> > 1. It should use driver model (only) in U-Boot proper. If there is
> > some SPL problem then add a specific function or feature for SPL.
> We can doing this in later since it is require sometime to figure out
> and testing.

We can also do it now, and that is better.

> > 2. It should not be necessary ti manually init subsystems - driver
> > model does this for you
> This is for initializing storage driver in very early SPL, where
> loading from storage to configure some critical HW need to done first.

The key problem is that your approach does not scale.  We had the
same discussion elsewhere.

> > 3. You can use the uclass name to find things
> Yeah, once it is converted to driver model, we can use the uclass for
> searching HW info in DTS.

Then please convert it.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
I read part of it all the way through.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v10 4/4] common: Generic firmware loader for file system

[Simon Glass]

Hi,

On 7 March 2018 at 22:03, Chee, Tien Fong  wrote:
> On Tue, 2018-03-06 at 10:51 -0700, Simon Glass wrote:
>> Hi,
>>
>> On 5 March 2018 at 02:43,   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 
>> > Reviewed-by: Lothar Waßmann 
>> > ---
>> >  common/Kconfig |  10 ++
>> >  common/Makefile|   1 +
>> >  common/fs_loader.c | 353
>> > +
>> >  doc/README.firmware_loader |  86 +++
>> >  include/fs_loader.h|  28 
>> >  5 files changed, 478 insertions(+)
>> >  create mode 100644 common/fs_loader.c
>> >  create mode 100644 doc/README.firmware_loader
>> >  create mode 100644 include/fs_loader.h
>> This looks fine as a concept but I am not keen on the implementation.
>>
> This patchset has been going through many rounds and a lot of time
> spending in review, and it is already working and being tested. I still
> have a lot subsequent patches pending on this patchset. I would suggest
> to accept this patchset, then we can enhance it to driver model in
> later.
>
>> 1. It should use driver model (only) in U-Boot proper. If there is
>> some SPL problem then add a specific function or feature for SPL.
> We can doing this in later since it is require sometime to figure out
> and testing.
>
>> 2. It should not be necessary ti manually init subsystems - driver
>> model does this for you
> This is for initializing storage driver in very early SPL, where
> loading from storage to configure some critical HW need to done first.
>
>> 3. You can use the uclass name to find things
> Yeah, once it is converted to driver model, we can use the uclass for
> searching HW info in DTS.
>>
>> Please let me know if you need more info.

Well I'm still not keen on this. Are you planning to do the
conversion? It seems like a lot of work for someone else to do.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v10 4/4] common: Generic firmware loader for file system

[Chee, Tien Fong]

On Tue, 2018-03-06 at 10:51 -0700, Simon Glass wrote:
> Hi,
> 
> On 5 March 2018 at 02:43,   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 
> > Reviewed-by: Lothar Waßmann 
> > ---
> >  common/Kconfig |  10 ++
> >  common/Makefile|   1 +
> >  common/fs_loader.c | 353
> > +
> >  doc/README.firmware_loader |  86 +++
> >  include/fs_loader.h|  28 
> >  5 files changed, 478 insertions(+)
> >  create mode 100644 common/fs_loader.c
> >  create mode 100644 doc/README.firmware_loader
> >  create mode 100644 include/fs_loader.h
> This looks fine as a concept but I am not keen on the implementation.
> 
This patchset has been going through many rounds and a lot of time
spending in review, and it is already working and being tested. I still
have a lot subsequent patches pending on this patchset. I would suggest
to accept this patchset, then we can enhance it to driver model in
later.

> 1. It should use driver model (only) in U-Boot proper. If there is
> some SPL problem then add a specific function or feature for SPL.
We can doing this in later since it is require sometime to figure out
and testing.

> 2. It should not be necessary ti manually init subsystems - driver
> model does this for you
This is for initializing storage driver in very early SPL, where
loading from storage to configure some critical HW need to done first.

> 3. You can use the uclass name to find things
Yeah, once it is converted to driver model, we can use the uclass for
searching HW info in DTS.
> 
> Please let me know if you need more info.
> 
> Regards,
> Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v10 4/4] common: Generic firmware loader for file system

[Simon Glass]

Hi,

On 5 March 2018 at 02:43,   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 
> Reviewed-by: Lothar Waßmann 
> ---
>  common/Kconfig |  10 ++
>  common/Makefile|   1 +
>  common/fs_loader.c | 353 
> +
>  doc/README.firmware_loader |  86 +++
>  include/fs_loader.h|  28 
>  5 files changed, 478 insertions(+)
>  create mode 100644 common/fs_loader.c
>  create mode 100644 doc/README.firmware_loader
>  create mode 100644 include/fs_loader.h

This looks fine as a concept but I am not keen on the implementation.

1. It should use driver model (only) in U-Boot proper. If there is
some SPL problem then add a specific function or feature for SPL.
2. It should not be necessary ti manually init subsystems - driver
model does this for you
3. You can use the uclass name to find things

Please let me know if you need more info.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v10 4/4] common: Generic firmware loader for file system

[tien . fong . chee]

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 
Reviewed-by: Lothar Waßmann 
---
 common/Kconfig |  10 ++
 common/Makefile|   1 +
 common/fs_loader.c | 353 +
 doc/README.firmware_loader |  86 +++
 include/fs_loader.h|  28 
 5 files changed, 478 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/Kconfig b/common/Kconfig
index d5bc37e..ff88c6e 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -560,6 +560,16 @@ config DISPLAY_BOARDINFO
  when U-Boot starts up. The board function checkboard() is called
  to do this.
 
+config GENERIC_FIRMWARE_LOADER
+   bool "Enable generic firmware loader driver for file system"
+   depends on SPL
+   help
+ 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.
+
 menu "Start-up hooks"
 
 config ARCH_EARLY_INIT_R
diff --git a/common/Makefile b/common/Makefile
index c7bde23..92d8fb3 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -134,3 +134,4 @@ obj-$(CONFIG_$(SPL_)LOG) += log.o
 obj-$(CONFIG_$(SPL_)LOG_CONSOLE) += log_console.o
 obj-y += s_record.o
 obj-y += xyzModem.o
+obj-$(CONFIG_GENERIC_FIRMWARE_LOADER) += fs_loader.o
diff --git a/common/fs_loader.c b/common/fs_loader.c
new file mode 100644
index 000..44c8ee0
--- /dev/null
+++ b/common/fs_loader.c
@@ -0,0 +1,353 @@
+/*
+ * Copyright (C) 2018 Intel Corporation 
+ *
+ * SPDX-License-Identifier:GPL-2.0
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#ifdef CONFIG_CMD_UBIFS
+#include 
+#include 
+#endif
+#include 
+
+struct firmware_priv {
+   const char *name;   /* Filename */
+   u32 offset; /* Offset of reading a file */
+};
+
+static struct device_location default_locations[] = {
+   {
+   .name = "mmc",
+   .devpart = "0:1",
+   },
+   {
+   .name = "usb",
+   .devpart = "0:1",
+   },
+   {
+   .name = "sata",
+   .devpart = "0:1",
+   },
+   {
+   .name = "ubi",
+   .mtdpart = "UBI",
+   .ubivol = "ubi0",
+   },
+};
+
+/* 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(0);
+   if (err)
+   return err;
+#endif
+
+   return err;
+}
+#else
+static int init_usb(void)
+{
+   debug("Error: Cannot load flash image: no USB support\n");
+   return -ENOSYS;
+}
+#endif
+#endif
+
+#ifdef CONFIG_SATA
+static int init_storage_sata(struct device_location *location)
+{
+   int dev;
+
+   /* Get device ID */
+   dev = (int)simple_strtoul(location->devpart, NULL, 10);
+
+   return sata_probe(dev);
+}
+#else
+static int init_storage_sata(struct device_location *location)
+{
+   debug("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 = ubi_part(location->mtdpart, NULL);
+
+   if (ret) {
+   debug("Cannot find mtd partition %s\n", location->mtdpart);
+   return ret;
+   }
+
+   return cmd_ubifs_mount(location->ubivol);
+}
+
+static int umount_ubifs(void)
+{
+   return cmd_ubifs_umount();
+}
+#else
+static int mount_ubifs(struct device_location *location)
+{
+   debug("Error: Cannot load image: no UBIFS support\n");
+   return -ENOSYS;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT) && defined(CONFIG_SPL_BUILD)
+__weak int init_mmc(void)
+{
+   /* Just for the case MMC is not yet initialized */
+   struct mmc *mmc = NULL;
+   int err;
+
+#ifdef CONFIG_SPL
+   spl_mmc_find_device(, spl_boot_device());
+#else
+   return -ENOENT;
+#endif
+
+   err = mmc_init(mmc);
+   if (err) {
+   debug("spl: mmc init failed with error: %d\n", err);
+   return err;
+   }
+
+   return err;
+}
+#else
+__weak int init_mmc(void)
+{
+   /* Expect somewhere already initialize MMC */
+   return 0;
+}
+#endif
+
+static int select_fs_dev(struct device_location *location)
+{
+   int ret;
+
+