[U-Boot] [PATCH v4 6/6] common: Generic loader for file system

[Chee, Tien Fong]

On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
> On 25.7.2018 18:03, Tom Rini wrote:
> > 
> > On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
> > > 
> > > Hi,
> > > 
> > > On 25 July 2018 at 03:48, Michal Simek <michal.simek at xilinx.com>
> > > wrote:
> > > > 
> > > > 
> > > > On 25.7.2018 08:31, Chee, Tien Fong wrote:
> > > > > 
> > > > > On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
> > > > > > 
> > > > > > On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > From: Tien Fong Chee <tien.fong.chee at intel.com>
> > > > > > > 
> > > [...]
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > Also that DT binding is quite weird and I don't think you
> > > > > > will get
> > > > > > ACK
> > > > > > for this from device tree community at all. I think that
> > > > > > calling via
> > > > > > platdata and avoid DT nodes would be better way to go.
> > > > > Why do you think DT binding is weird? The DT is designed
> > > > > based on Simon
> > > > > proposal, and i believe following the rules in DTS spec.
> > > > > There are some DT benefits with current design, i think
> > > > > someone may be
> > > > > maintainer need to made the final decision on the design.
> > > > It is software configuration in file which should mostly
> > > > describe
> > > > hardware and state for hardware configuration.
> > > > 
> > > > Your fs_loader node is purely describe sw configuration which
> > > > shouldn't
> > > > be here.
> > > > You have there run time configuration via variables. I think
> > > > using only
> > > > this way is enough. Default variables will match what you would
> > > > want to
> > > > add to DT.
> > > I think DT makes sense in the U-Boot context.
> > > 
> > > We don't have a user space to handle policy decisions, and the
> > > 'chosen' node is a good place to configure these common features.
> > > 
> > > While you can argue that the partition or filesystem where an
> > > image
> > > comes from is a software config, it is something that has to be
> > > configured. It has impact on hardware too, since the FPGA has to
> > > get
> > > its firmware from somewhere. We use the chosen node to specify
> > > the
> > > UART to use, and this is no different. Again, we don't have user-
> > > space
> > > config files in U-Boot.
> > > 
> > > This argument comes up from time to time and I'd really like to
> > > put it
> > > to bed for U-Boot. I understand that Linux has its own approach
> > > and
> > > rules, but in some cases they serve U-Boot poorly.
> > I want to second this as well.  So long as we're using our prefix
> > and
> > we've thought through and discussed what we're trying to do here,
> > it's
> > OK to do things that might not be accepted for Linux.
> > 
> I have not a problem with using chosen node with u-boot prefix
> properties and my colleague hopefully with finish work about moving
> u-boot,dm-pre-reloc; to chosen node where it should be (because
> current
> solution has also problem with ordering).
> 
> In this loader case doc is saying that you can rewrite it with
> variables
> on the prompt (or via script).
> For cases that you want to autodetect platform and pass/load correct
> dtb
> which setup u-boot this can be problematic and using DT is could be
> considered as easier for use.
> 
> In this case this is what was proposed:
> 
> +	fs_loader0: fs-loader at 0 {
> +		u-boot,dm-pre-reloc;
> +		compatible = "u-boot,fs-loader";
> +		phandlepart = <&mmc 1>;
> +	};
> 
> +	fs_loader1: fs-loader at 1 {
> +		u-boot,dm-pre-reloc;
> +		compatible = "u-boot,fs-loader";
> +		mtdpart = "UBI",
> +		ubivol = "ubi0";
> +	};
> 
> u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup
> for
> this driver - it means this should be here.
You are right, i missed this one. The intention of design enables user
to call any loader with default storage through the sequence number  if
fs loader is not defined in chosen. For example, there is a case where
system loading the file from SDMMC, NAND and QSPI.
> 
> compatible = "u-boot,fs-loader"; - bind and probe are empty that's
> why
> this is only used for filling platdata but driver has no user that's
> why
> this is unused till someone calls that functions.
> 
> phandlepart/mtdpart/ubivol is just for setup.
There are some benefits with driver model:
1. Saving space, calling when need.
2. Handle memory allocation and deallocation automatically.
> 
> For the first case you can just use in chosen node:
> u-boot,fs-loader = <&mmc 1>;
> 
> And for UBIfs. I have never played with that but I expect it
> shouldn't
> be big problem to describe it differently too (something like)
> u-boot,fs-loader = <0 ubi0>;
Need consider description for UBIFS, using fs-loader seems not working
for UBIFS, since more arguments such as mtdpartition and mtd volume
need passing into driver. In order to avoid messing, fs_loader can act
the pointer to the chosen. 

Anyway, i have no strong opinion with driver designed via platdata or
driver model if we can resolve the problem for UBIFS and maintainers
agree with it.
> 
> Then this driver/interface can stay in DT where it should stay. The
> only
> thing is how this should be initialized because there is no
> compatible
> string. But you can do that via platdata for platforms which want to
> use
> this.
> 
> Thanks,
> Michal
>