[U-Boot,v3,06/20] common: Generic firmware loader for file system

Message ID 1507882137-27841-7-git-send-email-tien.fong.chee@intel.com
State Superseded
Delegated to: Marek Vasut
Headers show
Series
  • Add FPGA, SDRAM, SPL loadfs U-boot & booting to console
Related show

Commit Message

Chee, Tien Fong Oct. 13, 2017, 8:08 a.m.
From: Tien Fong Chee <tien.fong.chee@intel.com>

Generic firmware loader framework contains some common functionality
which is reusable by any specific file system firmware loader.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 common/Makefile   |   2 +
 common/load_fs.c  | 163 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/load_fs.h |  40 ++++++++++++++
 3 files changed, 205 insertions(+)
 create mode 100644 common/load_fs.c
 create mode 100644 include/load_fs.h

Comments

Dinh Nguyen Oct. 16, 2017, 2:08 p.m. | #1
On 10/13/2017 03:08 AM, tien.fong.chee@intel.com wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
> 
> Generic firmware loader framework contains some common functionality
> which is reusable by any specific file system firmware loader.
> 
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> ---
>  common/Makefile   |   2 +
>  common/load_fs.c  | 163 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/load_fs.h |  40 ++++++++++++++
>  3 files changed, 205 insertions(+)
>  create mode 100644 common/load_fs.c
>  create mode 100644 include/load_fs.h

There is alot of change here and the commit message doesn't tell me
anything! Please describe, in detail, what your patch is doing.

Also you need to include more people in the review path for this patch.

Dinh
Marek Vasut Oct. 16, 2017, 2:41 p.m. | #2
On 10/16/2017 04:08 PM, Dinh Nguyen wrote:
> 
> 
> On 10/13/2017 03:08 AM, tien.fong.chee@intel.com wrote:
>> From: Tien Fong Chee <tien.fong.chee@intel.com>
>>
>> Generic firmware loader framework contains some common functionality
>> which is reusable by any specific file system firmware loader.
>>
>> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
>> ---
>>  common/Makefile   |   2 +
>>  common/load_fs.c  | 163 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/load_fs.h |  40 ++++++++++++++
>>  3 files changed, 205 insertions(+)
>>  create mode 100644 common/load_fs.c
>>  create mode 100644 include/load_fs.h
> 
> There is alot of change here and the commit message doesn't tell me
> anything! Please describe, in detail, what your patch is doing.
> 
> Also you need to include more people in the review path for this patch.

And pull it out of the series and convert the splash loader to use this.
It seems my comment about factoring out code from the splash loader was
ignored, sigh ...

> Dinh
>
Chee, Tien Fong Oct. 23, 2017, 6:37 a.m. | #3
On Isn, 2017-10-16 at 16:41 +0200, Marek Vasut wrote:
> On 10/16/2017 04:08 PM, Dinh Nguyen wrote:

> > 

> > 

> > 

> > On 10/13/2017 03:08 AM, tien.fong.chee@intel.com wrote:

> > > 

> > > From: Tien Fong Chee <tien.fong.chee@intel.com>

> > > 

> > > Generic firmware loader framework contains some common

> > > functionality

> > > which is reusable by any specific file system firmware loader.

> > > 

> > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

> > > ---

> > >  common/Makefile   |   2 +

> > >  common/load_fs.c  | 163

> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++

> > >  include/load_fs.h |  40 ++++++++++++++

> > >  3 files changed, 205 insertions(+)

> > >  create mode 100644 common/load_fs.c

> > >  create mode 100644 include/load_fs.h

> > There is alot of change here and the commit message doesn't tell me

> > anything! Please describe, in detail, what your patch is doing.

> > 

> > Also you need to include more people in the review path for this

> > patch.

These are the code factored out from splash loader, contains some
common functions which can be used by other file system loader such as
fpga loadfs.
Okay, i will include more peoples.
> And pull it out of the series and convert the splash loader to use

> this.

> It seems my comment about factoring out code from the splash loader

> was

> ignored, sigh ...

> 

I didn't ignore your comment, i factored out the common code from
splash loader and put in more generic function name and file name.
Then, i will replace these generic fs loader with splash fs loader at
different patchset.
> > 

> > Dinh

> > 

>
Lukasz Majewski Oct. 26, 2017, 12:51 p.m. | #4
Tien Fong,

> On Isn, 2017-10-16 at 16:41 +0200, Marek Vasut wrote:
> > On 10/16/2017 04:08 PM, Dinh Nguyen wrote:
> > > 
> > > 
> > > 
> > > On 10/13/2017 03:08 AM, tien.fong.chee@intel.com wrote:
> > > > 
> > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > 
> > > > Generic firmware loader framework contains some common
> > > > functionality
> > > > which is reusable by any specific file system firmware loader.
> > > > 
> > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > ---
> > > >  common/Makefile   |   2 +
> > > >  common/load_fs.c  | 163
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/load_fs.h |  40 ++++++++++++++
> > > >  3 files changed, 205 insertions(+)
> > > >  create mode 100644 common/load_fs.c
> > > >  create mode 100644 include/load_fs.h
> > > There is alot of change here and the commit message doesn't tell
> > > me anything! Please describe, in detail, what your patch is doing.
> > > 
> > > Also you need to include more people in the review path for this
> > > patch.
> These are the code factored out from splash loader, contains some
> common functions which can be used by other file system loader such as
> fpga loadfs.

Would it be possible to provide ./doc entry to explain how one can use
this set of tools (splash/loadfs loaders) ?

> Okay, i will include more peoples.
> > And pull it out of the series and convert the splash loader to use
> > this.
> > It seems my comment about factoring out code from the splash loader
> > was
> > ignored, sigh ...
> > 
> I didn't ignore your comment, i factored out the common code from
> splash loader and put in more generic function name and file name.
> Then, i will replace these generic fs loader with splash fs loader at
> different patchset.
> > > 
> > > Dinh
> > > 
> > 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot




Best regards,

Lukasz Majewski

--

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: wd@denx.de
Chee, Tien Fong Oct. 27, 2017, 9:23 a.m. | #5
On Kha, 2017-10-26 at 14:51 +0200, Lukasz Majewski wrote:
> Tien Fong,

> 

> > 

> > On Isn, 2017-10-16 at 16:41 +0200, Marek Vasut wrote:

> > > 

> > > On 10/16/2017 04:08 PM, Dinh Nguyen wrote:

> > > > 

> > > > 

> > > > 

> > > > 

> > > > On 10/13/2017 03:08 AM, tien.fong.chee@intel.com wrote:

> > > > > 

> > > > > 

> > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>

> > > > > 

> > > > > Generic firmware loader framework contains some common

> > > > > functionality

> > > > > which is reusable by any specific file system firmware

> > > > > loader.

> > > > > 

> > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>

> > > > > ---

> > > > >  common/Makefile   |   2 +

> > > > >  common/load_fs.c  | 163

> > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++

> > > > >  include/load_fs.h |  40 ++++++++++++++

> > > > >  3 files changed, 205 insertions(+)

> > > > >  create mode 100644 common/load_fs.c

> > > > >  create mode 100644 include/load_fs.h

> > > > There is alot of change here and the commit message doesn't

> > > > tell

> > > > me anything! Please describe, in detail, what your patch is

> > > > doing.

> > > > 

> > > > Also you need to include more people in the review path for

> > > > this

> > > > patch.

> > These are the code factored out from splash loader, contains some

> > common functions which can be used by other file system loader such

> > as

> > fpga loadfs.

> Would it be possible to provide ./doc entry to explain how one can

> use

> this set of tools (splash/loadfs loaders) ?

> 

Sure. I will provide a./doc or comment in next version. Basically, the
idea is factoring out the common code which specific handlle image in
file format loading from flash to target(memory/device) between splash
loader and fpga loadfs. So, you will see i have declared a few weak
functions, which is used for defined speficic handling algorithm such
as get_file, and fs_loading.

Initially, my plan is creating a more generic function name and geneirc
file name, then replacing those splash_loader fs at separate patch set.

Now, i am working directly on splash loader. Anyway, i also like more
discussion and good comments while i am working on it.

Thanks.
 
> > 

> > Okay, i will include more peoples.

> > > 

> > > And pull it out of the series and convert the splash loader to

> > > use

> > > this.

> > > It seems my comment about factoring out code from the splash

> > > loader

> > > was

> > > ignored, sigh ...

> > > 

> > I didn't ignore your comment, i factored out the common code from

> > splash loader and put in more generic function name and file name.

> > Then, i will replace these generic fs loader with splash fs loader

> > at

> > different patchset.

> > > 

> > > > 

> > > > 

> > > > Dinh

> > > > 

> > _______________________________________________

> > U-Boot mailing list

> > U-Boot@lists.denx.de

> > https://lists.denx.de/listinfo/u-boot

> 

> 

> 

> Best regards,

> 

> Lukasz Majewski

> 

> --

> 

> 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: wd@denx.de
Lukasz Majewski Oct. 27, 2017, 10:35 a.m. | #6
Hi Tien Fong,

> On Kha, 2017-10-26 at 14:51 +0200, Lukasz Majewski wrote:
> > Tien Fong,
> > 
> > > 
> > > On Isn, 2017-10-16 at 16:41 +0200, Marek Vasut wrote:
> > > > 
> > > > On 10/16/2017 04:08 PM, Dinh Nguyen wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > On 10/13/2017 03:08 AM, tien.fong.chee@intel.com wrote:
> > > > > > 
> > > > > > 
> > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > 
> > > > > > Generic firmware loader framework contains some common
> > > > > > functionality
> > > > > > which is reusable by any specific file system firmware
> > > > > > loader.
> > > > > > 
> > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > ---
> > > > > >  common/Makefile   |   2 +
> > > > > >  common/load_fs.c  | 163
> > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/load_fs.h |  40 ++++++++++++++
> > > > > >  3 files changed, 205 insertions(+)
> > > > > >  create mode 100644 common/load_fs.c
> > > > > >  create mode 100644 include/load_fs.h
> > > > > There is alot of change here and the commit message doesn't
> > > > > tell
> > > > > me anything! Please describe, in detail, what your patch is
> > > > > doing.
> > > > > 
> > > > > Also you need to include more people in the review path for
> > > > > this
> > > > > patch.
> > > These are the code factored out from splash loader, contains some
> > > common functions which can be used by other file system loader
> > > such as
> > > fpga loadfs.
> > Would it be possible to provide ./doc entry to explain how one can
> > use
> > this set of tools (splash/loadfs loaders) ?
> > 
> Sure. I will provide a./doc or comment in next version. Basically, the
> idea is factoring out the common code which specific handlle image in
> file format loading from flash to target(memory/device) between splash
> loader and fpga loadfs. So, you will see i have declared a few weak
> functions, which is used for defined speficic handling algorithm such
> as get_file, and fs_loading.
> 
> Initially, my plan is creating a more generic function name and
> geneirc file name, then replacing those splash_loader fs at separate
> patch set.
> 
> Now, i am working directly on splash loader. Anyway, i also like more
> discussion and good comments while i am working on it.

I've asked for a documentation, since I do have one idea in the back of
my head.

I'm wondering if other SoCs could benefit from this solution? For
example when we treat the FPGA as a DSP processor which needs to have
bitstream ( or better firmware ) loaded to some physical address. I'm
also wondering if your work would allow for start/stop of the code
execution?

It would be best to have some kind of common code and extensions per
soc/architecture.

I cannot help much with review/design phase since I know very little on
this particular Altera (up.. sorry Intel) solution.

Please add me to CC for your further work.

> 
> Thanks.
>  
> > > 
> > > Okay, i will include more peoples.
> > > > 
> > > > And pull it out of the series and convert the splash loader to
> > > > use
> > > > this.
> > > > It seems my comment about factoring out code from the splash
> > > > loader
> > > > was
> > > > ignored, sigh ...
> > > > 
> > > I didn't ignore your comment, i factored out the common code from
> > > splash loader and put in more generic function name and file name.
> > > Then, i will replace these generic fs loader with splash fs loader
> > > at
> > > different patchset.
> > > > 
> > > > > 
> > > > > 
> > > > > Dinh
> > > > > 
> > > _______________________________________________
> > > U-Boot mailing list
> > > U-Boot@lists.denx.de
> > > https://lists.denx.de/listinfo/u-boot
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > 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:
> > wd@denx.de



Best regards,

Lukasz Majewski

--

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: wd@denx.de
Marek Vasut Oct. 28, 2017, 11:32 a.m. | #7
On 10/27/2017 12:35 PM, Lukasz Majewski wrote:
[...]
>>>>>>>  common/Makefile   |   2 +
>>>>>>>  common/load_fs.c  | 163
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  include/load_fs.h |  40 ++++++++++++++
>>>>>>>  3 files changed, 205 insertions(+)
>>>>>>>  create mode 100644 common/load_fs.c
>>>>>>>  create mode 100644 include/load_fs.h
>>>>>> There is alot of change here and the commit message doesn't
>>>>>> tell
>>>>>> me anything! Please describe, in detail, what your patch is
>>>>>> doing.
>>>>>>
>>>>>> Also you need to include more people in the review path for
>>>>>> this
>>>>>> patch.
>>>> These are the code factored out from splash loader, contains some
>>>> common functions which can be used by other file system loader
>>>> such as
>>>> fpga loadfs.
>>> Would it be possible to provide ./doc entry to explain how one can
>>> use
>>> this set of tools (splash/loadfs loaders) ?
>>>
>> Sure. I will provide a./doc or comment in next version. Basically, the
>> idea is factoring out the common code which specific handlle image in
>> file format loading from flash to target(memory/device) between splash
>> loader and fpga loadfs. So, you will see i have declared a few weak
>> functions, which is used for defined speficic handling algorithm such
>> as get_file, and fs_loading.
>>
>> Initially, my plan is creating a more generic function name and
>> geneirc file name, then replacing those splash_loader fs at separate
>> patch set.
>>
>> Now, i am working directly on splash loader. Anyway, i also like more
>> discussion and good comments while i am working on it.
> 
> I've asked for a documentation, since I do have one idea in the back of
> my head.
> 
> I'm wondering if other SoCs could benefit from this solution? For
> example when we treat the FPGA as a DSP processor which needs to have
> bitstream ( or better firmware ) loaded to some physical address. I'm
> also wondering if your work would allow for start/stop of the code
> execution?

This is supposed to be a firmware loader (kind-of like the firmware
loader in Linux), so I have no idea what you mean by "start/stop" execution.

> It would be best to have some kind of common code and extensions per
> soc/architecture.

I can't see a usecase for that.

> I cannot help much with review/design phase since I know very little on
> this particular Altera (up.. sorry Intel) solution.[...]
Lukasz Majewski Oct. 28, 2017, 9:43 p.m. | #8
Hi Marek,

> On 10/27/2017 12:35 PM, Lukasz Majewski wrote:
> [...]
> >>>>>>>  common/Makefile   |   2 +
> >>>>>>>  common/load_fs.c  | 163
> >>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>  include/load_fs.h |  40 ++++++++++++++
> >>>>>>>  3 files changed, 205 insertions(+)
> >>>>>>>  create mode 100644 common/load_fs.c
> >>>>>>>  create mode 100644 include/load_fs.h
> >>>>>> There is alot of change here and the commit message doesn't
> >>>>>> tell
> >>>>>> me anything! Please describe, in detail, what your patch is
> >>>>>> doing.
> >>>>>>
> >>>>>> Also you need to include more people in the review path for
> >>>>>> this
> >>>>>> patch.
> >>>> These are the code factored out from splash loader, contains some
> >>>> common functions which can be used by other file system loader
> >>>> such as
> >>>> fpga loadfs.
> >>> Would it be possible to provide ./doc entry to explain how one can
> >>> use
> >>> this set of tools (splash/loadfs loaders) ?
> >>>
> >> Sure. I will provide a./doc or comment in next version. Basically,
> >> the idea is factoring out the common code which specific handlle
> >> image in file format loading from flash to target(memory/device)
> >> between splash loader and fpga loadfs. So, you will see i have
> >> declared a few weak functions, which is used for defined speficic
> >> handling algorithm such as get_file, and fs_loading.
> >>
> >> Initially, my plan is creating a more generic function name and
> >> geneirc file name, then replacing those splash_loader fs at
> >> separate patch set.
> >>
> >> Now, i am working directly on splash loader. Anyway, i also like
> >> more discussion and good comments while i am working on it.
> > 
> > I've asked for a documentation, since I do have one idea in the
> > back of my head.
> > 
> > I'm wondering if other SoCs could benefit from this solution? For
> > example when we treat the FPGA as a DSP processor which needs to
> > have bitstream ( or better firmware ) loaded to some physical
> > address. I'm also wondering if your work would allow for start/stop
> > of the code execution?
> 
> This is supposed to be a firmware loader (kind-of like the firmware
> loader in Linux),

So in principle I should be able to load any bitstream (firmware) to
any FPGA/SoC/whatever.

I do have in mind the ADI's SHARC DSP cores...

> so I have no idea what you mean by "start/stop"
> execution.

Ok. This can be done in a separate driver. No issues with that.

> 
> > It would be best to have some kind of common code and extensions per
> > soc/architecture.
> 
> I can't see a usecase for that.

The use case would be to reuse/tweak this code to be able to load
firmware for the SHARC DSP processors.

> 
> > I cannot help much with review/design phase since I know very
> > little on this particular Altera (up.. sorry Intel) solution.[...]




Best regards,

Lukasz Majewski

--

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: wd@denx.de
Marek Vasut Oct. 29, 2017, 9:35 a.m. | #9
On 10/28/2017 11:43 PM, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 10/27/2017 12:35 PM, Lukasz Majewski wrote:
>> [...]
>>>>>>>>>  common/Makefile   |   2 +
>>>>>>>>>  common/load_fs.c  | 163
>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>  include/load_fs.h |  40 ++++++++++++++
>>>>>>>>>  3 files changed, 205 insertions(+)
>>>>>>>>>  create mode 100644 common/load_fs.c
>>>>>>>>>  create mode 100644 include/load_fs.h
>>>>>>>> There is alot of change here and the commit message doesn't
>>>>>>>> tell
>>>>>>>> me anything! Please describe, in detail, what your patch is
>>>>>>>> doing.
>>>>>>>>
>>>>>>>> Also you need to include more people in the review path for
>>>>>>>> this
>>>>>>>> patch.
>>>>>> These are the code factored out from splash loader, contains some
>>>>>> common functions which can be used by other file system loader
>>>>>> such as
>>>>>> fpga loadfs.
>>>>> Would it be possible to provide ./doc entry to explain how one can
>>>>> use
>>>>> this set of tools (splash/loadfs loaders) ?
>>>>>
>>>> Sure. I will provide a./doc or comment in next version. Basically,
>>>> the idea is factoring out the common code which specific handlle
>>>> image in file format loading from flash to target(memory/device)
>>>> between splash loader and fpga loadfs. So, you will see i have
>>>> declared a few weak functions, which is used for defined speficic
>>>> handling algorithm such as get_file, and fs_loading.
>>>>
>>>> Initially, my plan is creating a more generic function name and
>>>> geneirc file name, then replacing those splash_loader fs at
>>>> separate patch set.
>>>>
>>>> Now, i am working directly on splash loader. Anyway, i also like
>>>> more discussion and good comments while i am working on it.
>>>
>>> I've asked for a documentation, since I do have one idea in the
>>> back of my head.
>>>
>>> I'm wondering if other SoCs could benefit from this solution? For
>>> example when we treat the FPGA as a DSP processor which needs to
>>> have bitstream ( or better firmware ) loaded to some physical
>>> address. I'm also wondering if your work would allow for start/stop
>>> of the code execution?
>>
>> This is supposed to be a firmware loader (kind-of like the firmware
>> loader in Linux),
> 
> So in principle I should be able to load any bitstream (firmware)

Yes, using this framework.

> to
> any FPGA/SoC/whatever.

No, this part is up to you to implement.

> I do have in mind the ADI's SHARC DSP cores...

The part which calls the firmware loader API and pushes it into those
cores is up to you to implement.

>> so I have no idea what you mean by "start/stop"
>> execution.
> 
> Ok. This can be done in a separate driver. No issues with that.

IMO it indeed should.

>>> It would be best to have some kind of common code and extensions per
>>> soc/architecture.
>>
>> I can't see a usecase for that.
> 
> The use case would be to reuse/tweak this code to be able to load
> firmware for the SHARC DSP processors.

I think your driver will use the firmware loader indeed, but this
functionality you described would be part of some driver for that
hardware, not the FW loader.
Lukasz Majewski Oct. 29, 2017, 10:57 p.m. | #10
Hi Marek,

> On 10/28/2017 11:43 PM, Lukasz Majewski wrote:
> > Hi Marek,
> >   
> >> On 10/27/2017 12:35 PM, Lukasz Majewski wrote:
> >> [...]  
> >>>>>>>>>  common/Makefile   |   2 +
> >>>>>>>>>  common/load_fs.c  | 163
> >>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>  include/load_fs.h |  40 ++++++++++++++
> >>>>>>>>>  3 files changed, 205 insertions(+)
> >>>>>>>>>  create mode 100644 common/load_fs.c
> >>>>>>>>>  create mode 100644 include/load_fs.h  
> >>>>>>>> There is alot of change here and the commit message doesn't
> >>>>>>>> tell
> >>>>>>>> me anything! Please describe, in detail, what your patch is
> >>>>>>>> doing.
> >>>>>>>>
> >>>>>>>> Also you need to include more people in the review path for
> >>>>>>>> this
> >>>>>>>> patch.  
> >>>>>> These are the code factored out from splash loader, contains
> >>>>>> some common functions which can be used by other file system
> >>>>>> loader such as
> >>>>>> fpga loadfs.  
> >>>>> Would it be possible to provide ./doc entry to explain how one
> >>>>> can use
> >>>>> this set of tools (splash/loadfs loaders) ?
> >>>>>  
> >>>> Sure. I will provide a./doc or comment in next version.
> >>>> Basically, the idea is factoring out the common code which
> >>>> specific handlle image in file format loading from flash to
> >>>> target(memory/device) between splash loader and fpga loadfs. So,
> >>>> you will see i have declared a few weak functions, which is used
> >>>> for defined speficic handling algorithm such as get_file, and
> >>>> fs_loading.
> >>>>
> >>>> Initially, my plan is creating a more generic function name and
> >>>> geneirc file name, then replacing those splash_loader fs at
> >>>> separate patch set.
> >>>>
> >>>> Now, i am working directly on splash loader. Anyway, i also like
> >>>> more discussion and good comments while i am working on it.  
> >>>
> >>> I've asked for a documentation, since I do have one idea in the
> >>> back of my head.
> >>>
> >>> I'm wondering if other SoCs could benefit from this solution? For
> >>> example when we treat the FPGA as a DSP processor which needs to
> >>> have bitstream ( or better firmware ) loaded to some physical
> >>> address. I'm also wondering if your work would allow for
> >>> start/stop of the code execution?  
> >>
> >> This is supposed to be a firmware loader (kind-of like the firmware
> >> loader in Linux),  
> > 
> > So in principle I should be able to load any bitstream (firmware)  
> 
> Yes, using this framework.
> 
> > to
> > any FPGA/SoC/whatever.  
> 
> No, this part is up to you to implement.
> 
> > I do have in mind the ADI's SHARC DSP cores...  
> 
> The part which calls the firmware loader API and pushes it into those
> cores is up to you to implement.
> 
> >> so I have no idea what you mean by "start/stop"
> >> execution.  
> > 
> > Ok. This can be done in a separate driver. No issues with that.  
> 
> IMO it indeed should.

Ok.

> 
> >>> It would be best to have some kind of common code and extensions
> >>> per soc/architecture.  
> >>
> >> I can't see a usecase for that.  
> > 
> > The use case would be to reuse/tweak this code to be able to load
> > firmware for the SHARC DSP processors.  
> 
> I think your driver will use the firmware loader indeed, but this
> functionality you described would be part of some driver for that
> hardware, not the FW loader.

Unfortunately, there is no such "driver" available from the vendor -
hence I'm poking around to see what can be "reused".



Best regards,

Lukasz Majewski

--

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: wd@denx.de
Marek Vasut Oct. 29, 2017, 10:59 p.m. | #11
On 10/29/2017 11:57 PM, Lukasz Majewski wrote:
> Hi Marek,
> 
[...]

>>>>> It would be best to have some kind of common code and extensions
>>>>> per soc/architecture.  
>>>>
>>>> I can't see a usecase for that.  
>>>
>>> The use case would be to reuse/tweak this code to be able to load
>>> firmware for the SHARC DSP processors.  
>>
>> I think your driver will use the firmware loader indeed, but this
>> functionality you described would be part of some driver for that
>> hardware, not the FW loader.
> 
> Unfortunately, there is no such "driver" available from the vendor -
> hence I'm poking around to see what can be "reused".
Generic FW loader to load whatever blob + whatever driver to install it
into the HW. You likely need to write the driver part then.

Patch

diff --git a/common/Makefile b/common/Makefile
index 801ea31..0e591c0 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -130,3 +130,5 @@  obj-$(CONFIG_CMD_DFU) += dfu.o
 obj-y += command.o
 obj-y += s_record.o
 obj-y += xyzModem.o
+
+obj-y += load_fs.o
diff --git a/common/load_fs.c b/common/load_fs.c
new file mode 100644
index 0000000..9f9ca88
--- /dev/null
+++ b/common/load_fs.c
@@ -0,0 +1,163 @@ 
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <fs.h>
+#include <fdt_support.h>
+#include <load_fs.h>
+#include <nand.h>
+#include <sata.h>
+#include <spi.h>
+#include <spi_flash.h>
+#include <usb.h>
+
+int flash_select_fs_dev(struct flash_location *location)
+{
+	int res;
+
+	switch (location->storage) {
+	case FLASH_STORAGE_MMC:
+		res = fs_set_blk_dev("mmc", location->devpart, FS_TYPE_ANY);
+		break;
+	case FLASH_STORAGE_USB:
+		res = fs_set_blk_dev("usb", location->devpart, FS_TYPE_ANY);
+		break;
+	case FLASH_STORAGE_SATA:
+		res = fs_set_blk_dev("sata", location->devpart, FS_TYPE_ANY);
+		break;
+	case FLASH_STORAGE_NAND:
+		if (location->ubivol != NULL)
+			res = fs_set_blk_dev("ubi", NULL, FS_TYPE_UBIFS);
+		else
+			res = -ENODEV;
+		break;
+	default:
+		printf("Error: unsupported location storage.\n");
+		return -ENODEV;
+	}
+
+	if (res)
+		printf("Error: could not access storage.\n");
+
+	return res;
+}
+#ifndef CONFIG_SPL_BUILD
+#ifdef CONFIG_USB_STORAGE
+static int flash_init_usb(void)
+{
+	int err;
+
+	err = usb_init();
+	if (err)
+		return err;
+
+#ifndef CONFIG_DM_USB
+	err = usb_stor_scan(1) < 0 ? -ENODEV : 0;
+#endif
+
+	return err;
+}
+#else
+static inline int flash_init_usb(void)
+{
+	printf("Cannot load flash image: no USB support\n");
+	return -ENOSYS;
+}
+#endif
+#endif
+
+#ifdef CONFIG_SATA
+static int flash_init_sata(void)
+{
+	return sata_probe(0);
+}
+#else
+static inline int flash_init_sata(void)
+{
+	printf("Cannot load flash image: no SATA support\n");
+	return -ENOSYS;
+}
+#endif
+
+#ifdef CONFIG_CMD_UBIFS
+static int flash_mount_ubifs(struct flash_location *location)
+{
+	int res;
+	char cmd[32];
+
+	sprintf(cmd, "ubi part %s", location->mtdpart);
+	res = run_command(cmd, 0);
+	if (res)
+		return res;
+
+	sprintf(cmd, "ubifsmount %s", location->ubivol);
+	res = run_command(cmd, 0);
+
+	return res;
+}
+
+static inline int flash_umount_ubifs(void)
+{
+	return run_command("ubifsumount", 0);
+}
+#else
+static inline int flash_mount_ubifs(struct flash_location *location)
+{
+	printf("Cannot load flash image: no UBIFS support\n");
+	return -ENOSYS;
+}
+
+static inline int flash_umount_ubifs(void)
+{
+	printf("Cannot unmount UBIFS: no UBIFS support\n");
+	return -ENOSYS;
+}
+#endif
+
+__weak char *get_file(void *file_info)
+{
+	return NULL;
+}
+
+__weak int fs_loading(void *file_info, const void *load_addr, size_t bsize)
+{
+	return 0;
+}
+
+int load_fs(struct flash_location *location, void *file_info,
+		const void *load_addr, size_t bsize)
+{
+	int res = 0;
+	char *flash_file;
+
+	flash_file = get_file(file_info);
+	if (!flash_file) {
+		printf("no filename specified.\n");
+		return -EINVAL;
+	}
+
+#ifndef CONFIG_SPL_BUILD
+	if (location->storage == FLASH_STORAGE_USB)
+		res = flash_init_usb();
+#endif
+
+	if (location->storage == FLASH_STORAGE_SATA)
+		res = flash_init_sata();
+
+	if (location->ubivol != NULL)
+		res = flash_mount_ubifs(location);
+
+	if (res)
+		return res;
+
+	res = fs_loading(file_info, load_addr, bsize);
+
+	if (location->ubivol != NULL)
+		flash_umount_ubifs();
+
+	return res;
+}
diff --git a/include/load_fs.h b/include/load_fs.h
new file mode 100644
index 0000000..a26d630
--- /dev/null
+++ b/include/load_fs.h
@@ -0,0 +1,40 @@ 
+/*
+ * Copyright (C) 2017 Intel Corporation <www.intel.com>
+ *
+ * SPDX-License-Identifier:    GPL-2.0
+ */
+
+
+#ifndef _LOAD_FS_H_
+#define _LOAD_FS_H_
+
+#include <errno.h>
+
+enum flash_storage {
+	FLASH_STORAGE_NAND,
+	FLASH_STORAGE_SF,
+	FLASH_STORAGE_MMC,
+	FLASH_STORAGE_USB,
+	FLASH_STORAGE_SATA,
+};
+
+enum flash_flags {
+	FLASH_STORAGE_RAW, /* Stored in raw memory */
+	FLASH_STORAGE_FS,  /* Stored within a file system */
+	FLASH_STORAGE_FIT, /* Stored inside a FIT image */
+};
+
+struct flash_location {
+	char *name;
+	enum flash_storage storage;
+	enum flash_flags flags;
+	u32 offset;	/* offset from start of storage */
+	char *devpart;  /* Use the load command dev:part conventions */
+	char *mtdpart;	/* MTD partition for ubi part */
+	char *ubivol;	/* UBI volume-name for ubifsmount */
+};
+
+int load_fs(struct flash_location *location, void *file_info,
+				const void *load_addr, size_t bsize);
+int flash_select_fs_dev(struct flash_location *location);
+#endif