[U-Boot] [PATCH v2 3/3] common: Generic loader for file system

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Thu, 2018-06-07 at 12:29 +0200, Marek Vasut wrote:
> On 06/07/2018 12:11 PM, Chee, Tien Fong wrote:
> > 
> > On Thu, 2018-06-07 at 11:50 +0200, Marek Vasut wrote:
> > > 
> > > On 06/07/2018 11:45 AM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Thu, 2018-06-07 at 10:41 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 06/07/2018 10:36 AM, Chee, Tien Fong wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Thu, 2018-06-07 at 08:51 +0200, Marek Vasut wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 06/07/2018 06:04 AM, Chee, Tien Fong wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Wed, 2018-06-06 at 10:39 +0200, Marek Vasut wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 05/24/2018 07:04 AM, tien.fong.chee at intel.com
> > > > > > > > > wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > > > > > 
> > > > > > > > > > This is file system generic loader which can be
> > > > > > > > > > used to
> > > > > > > > > > load
> > > > > > > > > > the file image from the storage into target such as
> > > > > > > > > > memory.
> > > > > > > > > > The consumer driver would then use this loader to
> > > > > > > > > > program
> > > > > > > > > > whatever,
> > > > > > > > > > ie. the FPGA device.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel
> > > > > > > > > > .com
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > [...]
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > +static int fs_loader_probe(struct udevice *dev)
> > > > > > > > > > +{
> > > > > > > > > > +	return 0;
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > > +static const struct udevice_id fs_loader_ids[] = {
> > > > > > > > > > +	{ .compatible = "fs_loader"},
> > > > > > > > > Why exactly is there a DT compatible for a firmware
> > > > > > > > > loader?
> > > > > > > > > 
> > > > > > > > Correct me if i'm wrong, this is required to look the
> > > > > > > > platform
> > > > > > > > data
> > > > > > > > from DTS, right? Details of DTS in patch 2.
> > > > > > > How so ? The FW loader should behave as a library for
> > > > > > > other
> > > > > > > drivers
> > > > > > > to
> > > > > > > use, not like a driver.
> > > > > > > 
> > > > > > The fs_loader node in DTS just provide a way for user to
> > > > > > tell
> > > > > > the
> > > > > > firmware loader what storage device and default partition
> > > > > > to
> > > > > > load
> > > > > > data
> > > > > > from. Default partition can be overriden through the
> > > > > > variable
> > > > > > environment.
> > > > > So that's sitting in the chosen node ? But why do you need to
> > > > > match
> > > > > on it ?
> > > > > 
> > > > This is new device tree binding for firmware loader called
> > > > fs_loader
> > > > node. firmware loader need to know where is the storage device
> > > > it
> > > > need
> > > > to load from. Those storage device would be defined in
> > > > fs_loader
> > > > node.
> > > This is a configuration. You do not need (new) a DT compatible
> > > for
> > > that.
> > > So why is the DT compatible even needed in the FW loader at all ?
> > > 
> > I thought DT compatible is used by driver to find the fs_loader
> > node in
> > DTS. May be i am wrong.
> There should be no FW loader in the DTS. Why would there be one ?
> 
 I added DTS support for user to define storage type and default
partition. So you want me to remove the DTS?
Removing the DTS, then user can only set storage type and partition
through dev instance. So, this design OK for you?
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > Caller/other drivers can create different firmware loader
> > > > > > instance
> > > > > > based on different fs_loader node(different storage device)
> > > > > > for
> > > > > > their
> > > > > > loading purpose.
> > > > > They can, but that could be done even without the DT
> > > > > compatible.
> > > > > 
> > > > Yes, once you get dev, you can set the storage type and
> > > > partiiton
> > > > attributes through accessing dev->platdata.
> > > Can you show me the DT patch needed for this to work ?
> > > 
> > I can send you the patches separately, but this driver is designed
> > to
> > work with/without DT.
> This is not a driver though, it should be a library (or so) for
> drivers
> to use.
> 
Yes, this is designed for other drivers to use.
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > Why this cannot be used by other driver?
> > > > > I don't understand the question.
> > > > > 
> > > > I means why u say this firmware loader cannot be used by other
> > > > drivers.
> > > > I have tested with FPGA Manager driver(as caller).
> > > It must be used by other drivers, that is the point.
> > > 
> > yes, it can be used by FPGA manager driver.
> > 
>