[PATCH] socfpga: Find partition with environment via device tree

[PATCH] socfpga: Find partition with environment via device tree

[Trent Piepho]

socfpga loads the environment from a file named "barebox.env" located
on the device "/dev/mmc0.1".  Both those names are hard-coded in the
socfpga code and can't be changed.

Barebox supports selecting the location of the environment using a
"barebox,environment" node in device tree's "chosen" node.

Change socfpga to use this mechanism by looking for barebox.env in the
device or partition specified in the device tree.

The filename is still hard-coded.  Someone who wants to change it
could put that into the device tree too (new file-name property?  3rd
string in device-path?).

Remove the stat() call in socfpga_env_init(), as mount() already
checks if the device exists.  Also remove device_detect_by_name() call
as the barebox env device code now takes care of that for us.

Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---

OMAP could do the same thing...

 arch/arm/dts/socfpga.dtsi       |  7 +++++++
 arch/arm/mach-socfpga/generic.c | 37 +++++++++++++------------------------
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
index d4d498b..1d0d6ff 100644
--- a/arch/arm/dts/socfpga.dtsi
+++ b/arch/arm/dts/socfpga.dtsi
@@ -1,4 +1,11 @@
 / {
+	chosen {
+                environment@0 {
+                        compatible = "barebox,environment";
+                        device-path = &mmc, "partname:1";
+                };
+	};
+
 	aliases {
 		mmc0 = &mmc;
 	};
diff --git a/arch/arm/mach-socfpga/generic.c b/arch/arm/mach-socfpga/generic.c
index 2d4afd0..46f9415 100644
--- a/arch/arm/mach-socfpga/generic.c
+++ b/arch/arm/mach-socfpga/generic.c
@@ -105,38 +105,27 @@ static int socfpga_init(void)
 core_initcall(socfpga_init);
 
 #if defined(CONFIG_ENV_HANDLING)
-#define ENV_PATH "/boot/barebox.env"
+#define ENV_PATH "/boot"		/* Where to mount env device */
+#define ENV_FILE "barebox.env"		/* File on device to use */
+
 static int socfpga_env_init(void)
 {
-	struct stat s;
-	char *diskdev, *partname;
+	const char *device;
 	int ret;
 
-	diskdev = "mmc0";
-
-	device_detect_by_name(diskdev);
-
-	partname = asprintf("/dev/%s.1", diskdev);
-
-	ret = stat(partname, &s);
-
-	if (ret) {
-		pr_err("Failed to load environment: no device '%s'\n", diskdev);
-		goto out_free;
-	}
-
-	mkdir("/boot", 0666);
-	ret = mount(partname, "fat", "/boot", NULL);
+	/* Get device env is on and mount it */
+	device = default_environment_path_get();
+	mkdir(ENV_PATH, 0777);
+	ret = mount(device, "fat", ENV_PATH, NULL);
 	if (ret) {
-		pr_err("Failed to load environment: mount %s failed (%d)\n", partname, ret);
-		goto out_free;
+		pr_err("Failed to load environment: mount %s failed (%d)\n", device, ret);
+		return ret;
 	}
 
-	pr_debug("Loading default env from %s on device %s\n", ENV_PATH, diskdev);
-	default_environment_path_set(ENV_PATH);
+	/* Change env to be in a file on the now mounted device */
+	pr_debug("Loading default env from %s on device %s\n", ENV_FILE, device);
+	default_environment_path_set(ENV_PATH "/" ENV_FILE);
 
-out_free:
-	free(partname);
 	return 0;
 }
 late_initcall(socfpga_env_init);
-- 
1.8.3.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] socfpga: Find partition with environment via device tree

[Sascha Hauer]

On Fri, Dec 11, 2015 at 02:13:21AM +0000, Trent Piepho wrote:
> socfpga loads the environment from a file named "barebox.env" located
> on the device "/dev/mmc0.1".  Both those names are hard-coded in the
> socfpga code and can't be changed.
> 
> Barebox supports selecting the location of the environment using a
> "barebox,environment" node in device tree's "chosen" node.
> 
> Change socfpga to use this mechanism by looking for barebox.env in the
> device or partition specified in the device tree.
> 
> The filename is still hard-coded.  Someone who wants to change it
> could put that into the device tree too (new file-name property?  3rd
> string in device-path?).
> 
> Remove the stat() call in socfpga_env_init(), as mount() already
> checks if the device exists.  Also remove device_detect_by_name() call
> as the barebox env device code now takes care of that for us.
> 
> Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
> ---
> 
> OMAP could do the same thing...
> 
>  arch/arm/dts/socfpga.dtsi       |  7 +++++++
>  arch/arm/mach-socfpga/generic.c | 37 +++++++++++++------------------------
>  2 files changed, 20 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
> index d4d498b..1d0d6ff 100644
> --- a/arch/arm/dts/socfpga.dtsi
> +++ b/arch/arm/dts/socfpga.dtsi
> @@ -1,4 +1,11 @@
>  / {
> +	chosen {
> +                environment@0 {
> +                        compatible = "barebox,environment";
> +                        device-path = &mmc, "partname:1";
> +                };
> +	};

The "barebox,environment" compatible describes a binding which takes
partition described in device-path as a raw file containing a barebox
envfs.
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:

environment@0 {
	compatible = "barebox,environment";
	device-path = &env;
};

eeprom@foo {
	partition@0 {
		reg = <0, 0x8000>;
		label = "foo";
		env: file@0 {
			path = "barebox.env";
		};
	};
};

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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] socfpga: Find partition with environment via device tree

[Trent Piepho]

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.

> 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.

I did think about extending the env driver to support files and put the
mounting code in there.  It would bloat it for all those platforms that
don't use files (but it would mean all platforms could use files and
socfpga could use a raw device...)

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.

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.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] socfpga: Find partition with environment via device tree

[Sascha Hauer]

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.

> 
> > 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".
We have the same problem sometimes with USB or PCI devices aswell. The
busses are autoprobable, so the devices are usually not described in the
device tree. Nevertheless sometimes a USB device has an additional reset
pin or regulator which is not autodetectable (worse even, the device may
not show up on the bus without handling the regulator first). For these
reasons having a descripion of devices on autoprobable busses is
desirable. I think partitions fall into the same category.

> 
> I did think about extending the env driver to support files and put the
> mounting code in there.  It would bloat it for all those platforms that
> don't use files (but it would mean all platforms could use files and
> socfpga could use a raw device...)

If it bloats the code too much we could make it configurable, that
shouldn't be a problem.

> 
> 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?

> 
> 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).

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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] socfpga: Find partition with environment via device tree

[Trent Piepho]

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.

> > 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.

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.  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.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] socfpga: Find partition with environment via device tree

[Sascha Hauer]

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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox