On Mon, Dec 14, 2015 at 07:24:19PM +0000, Trent Piepho wrote:
> On Mon, 2015-12-14 at 11:01 +0100, Sascha Hauer wrote:
> > On Fri, Dec 11, 2015 at 08:05:00PM +0000, Trent Piepho wrote:
> > > On Fri, 2015-12-11 at 10:30 +0100, Sascha Hauer wrote:
> > > > On Fri, Dec 11, 2015 at 02:13:21AM +0000, Trent Piepho wrote:
> > > > The "barebox,environment" compatible describes a binding which takes
> > > > partition described in device-path as a raw file containing a barebox
> > > > envfs.
> > >
> > > Well, I redefined it to have the device-path be the device containing
> > > the environment as raw data or the one containing a filesystem with the
> > > environment as a file if the architecture expects that.
> > >
> > > That doesn't seem like a bad definition.
> >
> > No, indeed not. It's more the implementation I have a problem with. The
> > implementation should be done in the environment driver.
>
> Ok, my second attempt moved it to the barebox env OF driver. This seems
> better as it unifies the boards that have an env on a device and those
> that put it in a file.
>
> > > > What you have here is a partition which contains a FAT which has a
> > > > barebox envfs in a file. I think it's fine to extend the binding (and
> > > > the barebox env driver) to handle that case. What you are doing here is
> > > > to smuggle code behind the driver and bend the environment path to
> > > > somewhere else. That is not ok. I think what we can do is to add some
> > > > file-path property which contains the path and tells the barebox
> > > > environment driver to not use the whole partition but instead to mount
> > > > it and use the file specified in the file-path property.
> > > >
> > > > As an alternative which I might even like better is to extend the
> > > > partition binding to be able to describe a file in a partition, like:
> > >
> > > This won't work. mmc devices, which all boards that use a file for the
> > > env are using, don't have partitions in the device tree, since they are
> > > created dynamically from the partition table. There is nowhere to put
> > > the file node.
> >
> > Just because they are dynamically created from information found on the
> > MMC card doesn't mean we can't describe them in the device tree. We can
> > for example specify "The second partition" or "The partition with UUID
> > xy".
>
> That would be hard to do in practice, as the UUID of a partition is
> normally generated at random when the partition is made. It would be
> hard to keep the UUID in the device table matching the partition table.
> I suppose one could go by partition number or by name too. Which is
> really no different than the partname: syntax but in a different
> location.
Yes, the UUID may be a bad example, replace that with anything else that
is able to identify the partition.
>
> > > It is also problematic for my board. I need to change the env path
> > > based on data in an eeprom that selects which partition to use. Other
> > > boards that have multiple environments select them in a
> > > postcore_initcall using boot strapping pins that aren't part of a
> > > device. I'm using an eeprom so I need to do the selection later in the
> > > init process.
> >
> > Ok, so you couldn't you do the same?
>
> Yes, I made it work with my second attempt. The env driver is a
> late_initcall so I was able to have board code modify the OF tree so
> that it points to the correct partition.
>
> > > Maybe extend load_environment to do the mount? const char
> > > *default_environment_path would have to change to something that could
> > > support the concept of a device + optional filename. This way one could
> > > change the env path upto environment_initcall() time.
> >
> > My original plan was to extend this:
> >
> > device-path = &mmc, "partname:1";
> >
> > with a filename, like this:
> >
> > device-path = &mmc, "partname:1", "/bla/env";
> >
> > I found out that this is not how the device tree should be like. It
> > looks and feels better when the phandle directly points to a node and
> > the node describes a partition (or file in this case).
>
> I thought the env code was intentionally not using phandles so that it
> could be in a different DT than the DT describing the hardware. Thus
> the path instead of a phandle.
Sorry, mixed that up. &mmc is a path, not a phandle.
>
> I didn't want to extend device-path with an optional 3rd string since
> device-path is already IMHO too complex supporting both a path to a
> partition or a path to a device and then a partition description.
I tend to agree. When I created the binding it seemed like a good idea
to be able to chain together strings like describing the device, the
partition and then the filename. The problems I had implementing it
should have shown me that idea wasn't the best.
> So
> one would also want to support device-path = &eeprom, "/bla/env" and now
> you've got to guess if the 2nd string is a file or partition name.
>
> So I've guess we have:
>
> {
> compatible = "barebox,environment";
> device-path = &foo, "partname:bar";
> file-path = "baz";
> }
>
> OR
>
> {
> compatible = "barebox,environment";
> env-path = &environment;
> }
>
> &foo {
> partition@0 {
> label = "bar";
> environment: file@0 {
> path = "baz";
> };
> };
> }
>
> So the same information but in a different layout. The latter layout is
> more complex and I'm not sure how the code would properly tell if the
> node path refers to a device or a partition or a file.
Your current "Support env from file in a file-system via device tree"
patch indeed has a straight forward implementation. Maybe we should go
that way. I'll have a closer look at it.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
