Re: [U-Boot] [PATCH v10 4/4] common: Generic firmware loader for file system
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
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
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
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
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
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
