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

Fri Sep 21 04:42:54 UTC 2018

On Tue, 2018-07-31 at 08:22 +0200, Michal Simek wrote:
> On 30.7.2018 18:05, Simon Glass wrote:
> > 
> > Hi Michal,
> > 
> > On 30 July 2018 at 07:30, Michal Simek <michal.simek at xilinx.com>
> > wrote:
> > > 
> > > On 30.7.2018 15:26, Simon Glass wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On 27 July 2018 at 02:40, Chee, Tien Fong <tien.fong.chee at intel
> > > > .com> wrote:
> > > > > 
> > > > > 
> > > > > 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 xi
> > > > > > > > linx.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.
> > > > We should add a compatible string then :-)
> > > Isn't driver name used in case of platdata initialization?
> > If the node is in /chosen and has a compatible string, it will be
> > bound automatically. Manually binding a device is really just a
> > fallback for particular situations (e.g. buses like PCI where we
> > often
> > rely on probing to find out what is on the bus).
> up2you guys. I just have different opinion how this should be done.
> 
> Thanks,
> Michal

Sorry for late reply, because rushing the project.
So, are you guys OK with current implementation because we have no
solution for the UBIFS setting with other solution than using DT?
> 
>