From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chee, Tien Fong Date: Tue, 31 Jul 2018 05:12:09 +0000 Subject: [U-Boot] [PATCH v4 6/6] common: Generic loader for file system In-Reply-To: References: <1530865683-23207-1-git-send-email-tien.fong.chee@intel.com> <30e0c183-12d9-667d-3e51-caa986b399ff@xilinx.com> <1532500298.9671.24.camel@intel.com> <037207e4-bff7-8b97-7f06-74ddabe7c237@xilinx.com> <20180725160316.GW11755@bill-the-cat> <780b5bee-4448-793b-aaa9-fc5fe2ba1d3d@xilinx.com> <1532680844.10087.21.camel@intel.com> <02ac3ca3-ccf7-3ae6-ed37-78614064a743@xilinx.com> Message-ID: <1533013928.9940.1.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Mon, 2018-07-30 at 10:05 -0600, Simon Glass wrote: > Hi Michal, > > On 30 July 2018 at 07:30, Michal Simek > wrote: > > > > On 30.7.2018 15:26, Simon Glass wrote: > > > > > > Hi, > > > > > > On 27 July 2018 at 02:40, Chee, Tien Fong > > om> 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 > > > > > > nx.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 > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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). So, is this still the same with current implementation? / { chosen { firmware-loader = &fs_loader0; }; fs_loader0: fs-loader at 0 { u-boot,dm-pre-reloc; compatible = "u-boot,fs-loader"; source-partition = <&mmc 1>; }; }; > > Regards, > Simon