[U-Boot] [PATCH v1] env_mmc: configure environment offsets via device tree
This introduces the ability to override the environment offets from the device tree by setting the following nodes in '/config': 'u-boot,mmc-env-offset' - overrides CONFIG_ENV_OFFSET 'u-boot,mmc-env-offset-redundant' - overrides CONFIG_ENV_OFFSET_REDUND
To keep with the previous logic, the CONFIG_* defines still need to be available and the statically defined values become the defaults, when the corresponding properties are not set in the device-tree.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com --- common/env_mmc.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/common/env_mmc.c b/common/env_mmc.c index 16f6a17..ef3dbd1 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -1,24 +1,25 @@ /* * (C) Copyright 2008-2011 Freescale Semiconductor, Inc. * * SPDX-License-Identifier: GPL-2.0+ */
/* #define DEBUG */
#include <common.h>
#include <command.h> #include <environment.h> +#include <fdtdec.h> #include <linux/stddef.h> #include <malloc.h> #include <memalign.h> #include <mmc.h> #include <search.h> #include <errno.h>
#if defined(CONFIG_ENV_SIZE_REDUND) && \ (CONFIG_ENV_SIZE_REDUND != CONFIG_ENV_SIZE) #error CONFIG_ENV_SIZE_REDUND should be the same as CONFIG_ENV_SIZE #endif
@@ -36,21 +37,43 @@ DECLARE_GLOBAL_DATA_PTR; #define CONFIG_ENV_OFFSET 0 #endif
-__weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr) +#ifdef CONFIG_OF_LIBFDT +static inline s64 mmc_offset(int copy) { - s64 offset; + const char *propname = "u-boot,mmc-env-offset"; + s64 defvalue = CONFIG_ENV_OFFSET;
- offset = CONFIG_ENV_OFFSET; -#ifdef CONFIG_ENV_OFFSET_REDUND +#if defined(CONFIG_ENV_OFFSET_REDUND) + if (copy) { + propname = "u-boot,mmc-env-offset-redundant"; + defvalue = CONFIG_ENV_OFFSET_REDUND; + } +#endif + + return fdtdec_get_config_int(gd->fdt_blob, propname, defvalue); +} +#else +static inline s64 mmc_offset(int copy) +{ + s64 offset = CONFIG_ENV_OFFSET; + +#if defined(CONFIG_ENV_OFFSET_REDUND) if (copy) offset = CONFIG_ENV_OFFSET_REDUND; #endif + return offset; +} +#endif + +__weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr) +{ + s64 offset = mmc_offset(copy);
if (offset < 0) offset += mmc->capacity;
*env_addr = offset;
return 0; }
On 02/17/17 19:28, Philipp Tomsich wrote:
This introduces the ability to override the environment offets from the device tree by setting the following nodes in '/config': 'u-boot,mmc-env-offset' - overrides CONFIG_ENV_OFFSET 'u-boot,mmc-env-offset-redundant' - overrides CONFIG_ENV_OFFSET_REDUND
To keep with the previous logic, the CONFIG_* defines still need to be available and the statically defined values become the defaults, when the corresponding properties are not set in the device-tree.
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem... I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
common/env_mmc.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/common/env_mmc.c b/common/env_mmc.c index 16f6a17..ef3dbd1 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -1,24 +1,25 @@ /*
- (C) Copyright 2008-2011 Freescale Semiconductor, Inc.
- SPDX-License-Identifier: GPL-2.0+
*/
/* #define DEBUG */
#include <common.h>
#include <command.h> #include <environment.h> +#include <fdtdec.h> #include <linux/stddef.h> #include <malloc.h> #include <memalign.h> #include <mmc.h> #include <search.h> #include <errno.h>
#if defined(CONFIG_ENV_SIZE_REDUND) && \ (CONFIG_ENV_SIZE_REDUND != CONFIG_ENV_SIZE) #error CONFIG_ENV_SIZE_REDUND should be the same as CONFIG_ENV_SIZE #endif
@@ -36,21 +37,43 @@ DECLARE_GLOBAL_DATA_PTR; #define CONFIG_ENV_OFFSET 0 #endif
-__weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr) +#ifdef CONFIG_OF_LIBFDT +static inline s64 mmc_offset(int copy) {
- s64 offset;
- const char *propname = "u-boot,mmc-env-offset";
- s64 defvalue = CONFIG_ENV_OFFSET;
- offset = CONFIG_ENV_OFFSET;
-#ifdef CONFIG_ENV_OFFSET_REDUND +#if defined(CONFIG_ENV_OFFSET_REDUND)
- if (copy) {
propname = "u-boot,mmc-env-offset-redundant";defvalue = CONFIG_ENV_OFFSET_REDUND;- }
+#endif
- return fdtdec_get_config_int(gd->fdt_blob, propname, defvalue);
+} +#else +static inline s64 mmc_offset(int copy) +{
- s64 offset = CONFIG_ENV_OFFSET;
+#if defined(CONFIG_ENV_OFFSET_REDUND) if (copy) offset = CONFIG_ENV_OFFSET_REDUND; #endif
- return offset;
+} +#endif
+__weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr) +{
s64 offset = mmc_offset(copy);
if (offset < 0) offset += mmc->capacity;
*env_addr = offset;
return 0;
}
Igor,
On 19 Feb 2017, at 12:52, Igor Grinberg grinberg@compulab.co.il wrote:
On 02/17/17 19:28, Philipp Tomsich wrote:
This introduces the ability to override the environment offets from the device tree by setting the following nodes in '/config': 'u-boot,mmc-env-offset' - overrides CONFIG_ENV_OFFSET 'u-boot,mmc-env-offset-redundant' - overrides CONFIG_ENV_OFFSET_REDUND
To keep with the previous logic, the CONFIG_* defines still need to be available and the statically defined values become the defaults, when the corresponding properties are not set in the device-tree.
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
U-Boot usually locates the FDT to use at a preconfigured address (or one configured in the early environment). This will not be the environment we’d load from a MMC device configured via DM_MMC.
So there shouldn’t be a chicken & egg here.
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
So putting this in the DT is the best (and least intrusive) option available.
Regards, Philipp.
Hi Philipp,
On 02/19/17 19:59, Dr. Philipp Tomsich wrote:
Igor,
On 19 Feb 2017, at 12:52, Igor Grinberg grinberg@compulab.co.il wrote:
On 02/17/17 19:28, Philipp Tomsich wrote:
This introduces the ability to override the environment offets from the device tree by setting the following nodes in '/config': 'u-boot,mmc-env-offset' - overrides CONFIG_ENV_OFFSET 'u-boot,mmc-env-offset-redundant' - overrides CONFIG_ENV_OFFSET_REDUND
To keep with the previous logic, the CONFIG_* defines still need to be available and the statically defined values become the defaults, when the corresponding properties are not set in the device-tree.
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
Why? U-Boot can live pretty well w/o DT.
U-Boot usually locates the FDT to use at a preconfigured address (or one configured in the early environment).
Or just by providing the address to bootz command...
This will not be the environment we’d load from a MMC device configured via DM_MMC.
So there shouldn’t be a chicken & egg here.
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. And that will solve the problem.
So putting this in the DT is the best (and least intrusive) option available.
Ok. I see your point now. Yet I think we should keep the DT to its purpose - which is to describe the h/w (and not the s/w placement layout).
On 20 Feb 2017, at 08:22, Igor Grinberg grinberg@compulab.co.il wrote:
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
Why? U-Boot can live pretty well w/o DT.
If U-Boot runs without DT, then nothing will/should change about how the setting is retrieved from CONFIG_ENV_OFFSET.
The platform that motivated this change is ARCH_SUNXI, which does not use per-board defines but aims to have one generic bootloader per-SoC.
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. And that will solve the problem.
Doing this would still get into the way of architectures that want to build a single ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same across all board and vendor configurations. This can easily be handled with the (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
If we decide to this, I’d at least like to introduce the function call to (the weak function) mmc_get_env_addr(…), so we can override this in the board code.
So putting this in the DT is the best (and least intrusive) option available.
Ok. I see your point now. Yet I think we should keep the DT to its purpose - which is to describe the h/w (and not the s/w placement layout).
-- Regards, Igor.
Hi,
On 20 February 2017 at 02:18, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
On 20 Feb 2017, at 08:22, Igor Grinberg grinberg@compulab.co.il wrote:
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
Why? U-Boot can live pretty well w/o DT.
If U-Boot runs without DT, then nothing will/should change about how the setting is retrieved from CONFIG_ENV_OFFSET.
The platform that motivated this change is ARCH_SUNXI, which does not use per-board defines but aims to have one generic bootloader per-SoC.
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. And that will solve the problem.
Doing this would still get into the way of architectures that want to build a single ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same across all board and vendor configurations. This can easily be handled with the (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
If we decide to this, I’d at least like to introduce the function call to (the weak function) mmc_get_env_addr(…), so we can override this in the board code.
So putting this in the DT is the best (and least intrusive) option available.
Ok. I see your point now. Yet I think we should keep the DT to its purpose - which is to describe the h/w (and not the s/w placement layout).
Well actually we put things like flash map in there and the environment position seems like a very good thing to have there. So I like this patch. There is too compile-time configuration in U-Boot still...
In fact I wish we could support this for all environment types. Overall the environment drivers need some work to make them more similar...
Regards, Simon
Hi Philipp, Simon,
On 02/22/17 05:59, Simon Glass wrote:
Hi,
On 20 February 2017 at 02:18, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
On 20 Feb 2017, at 08:22, Igor Grinberg grinberg@compulab.co.il wrote:
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
Why? U-Boot can live pretty well w/o DT.
If U-Boot runs without DT, then nothing will/should change about how the setting is retrieved from CONFIG_ENV_OFFSET.
ok.
The platform that motivated this change is ARCH_SUNXI, which does not use per-board defines but aims to have one generic bootloader per-SoC.
Well, after a ten year experience with boars and different SoC vendors, I don't think it is possible... Unless all boards are copies of their respective reference design...
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. And that will solve the problem.
Doing this would still get into the way of architectures that want to build a single ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same across all board and vendor configurations. This can easily be handled with the (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
I think Kconfig would be enough for this, but please try your approach. The 'universal' thing will probably work if you don't have too many boards and they don't differ too much from each other...
If we decide to this, I’d at least like to introduce the function call to (the weak function) mmc_get_env_addr(…), so we can override this in the board code.
That is how it works today: include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
So putting this in the DT is the best (and least intrusive) option available.
Ok. I see your point now. Yet I think we should keep the DT to its purpose - which is to describe the h/w (and not the s/w placement layout).
Well actually we put things like flash map in there and the environment position seems like a very good thing to have there.
Well, I thought so too... But I had a discussion with kernel people some time ago and they discourage this approach and would not like to have the flash mapping in the DT.
So I like this patch. There is too compile-time configuration in U-Boot still...
Yeah, I know you like it ;-) but DT is not the place for it, unless DT specs. guys decide to change its purpose and place s/w configuration straps inside...
Although, U-Boot build process is not a pain at all, you might want to design another abstraction layer for s/w configuration straps. That way you will be able to keep the U-Boot core binary generic... Is it worse the effort? I don't know. Currently, having the board infrastructure, provides that layer and works fine.
In fact I wish we could support this for all environment types. Overall the environment drivers need some work to make them more similar...
Indeed, but not just that... It would be great to add a DM to env. drivers. This will make it possible to use more than one environment driver in runtime and make many people who struggle with it happy...
Hi all,
On 02/22/2017 04:35 PM, Igor Grinberg wrote:
Hi Philipp, Simon,
This patch is delegated to me..but i didn't receive any email in my mail box. So i didn't know this...i don't read yet fully..
After reading mails, i will reply again.
Thanks.
Best Regards, Jaehoon Chung
On 02/22/17 05:59, Simon Glass wrote:
Hi,
On 20 February 2017 at 02:18, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
On 20 Feb 2017, at 08:22, Igor Grinberg grinberg@compulab.co.il wrote:
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
Why? U-Boot can live pretty well w/o DT.
If U-Boot runs without DT, then nothing will/should change about how the setting is retrieved from CONFIG_ENV_OFFSET.
ok.
The platform that motivated this change is ARCH_SUNXI, which does not use per-board defines but aims to have one generic bootloader per-SoC.
Well, after a ten year experience with boars and different SoC vendors, I don't think it is possible... Unless all boards are copies of their respective reference design...
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. And that will solve the problem.
Doing this would still get into the way of architectures that want to build a single ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same across all board and vendor configurations. This can easily be handled with the (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
I think Kconfig would be enough for this, but please try your approach. The 'universal' thing will probably work if you don't have too many boards and they don't differ too much from each other...
If we decide to this, I’d at least like to introduce the function call to (the weak function) mmc_get_env_addr(…), so we can override this in the board code.
That is how it works today: include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
So putting this in the DT is the best (and least intrusive) option available.
Ok. I see your point now. Yet I think we should keep the DT to its purpose - which is to describe the h/w (and not the s/w placement layout).
Well actually we put things like flash map in there and the environment position seems like a very good thing to have there.
Well, I thought so too... But I had a discussion with kernel people some time ago and they discourage this approach and would not like to have the flash mapping in the DT.
So I like this patch. There is too compile-time configuration in U-Boot still...
Yeah, I know you like it ;-) but DT is not the place for it, unless DT specs. guys decide to change its purpose and place s/w configuration straps inside...
Although, U-Boot build process is not a pain at all, you might want to design another abstraction layer for s/w configuration straps. That way you will be able to keep the U-Boot core binary generic... Is it worse the effort? I don't know. Currently, having the board infrastructure, provides that layer and works fine.
In fact I wish we could support this for all environment types. Overall the environment drivers need some work to make them more similar...
Indeed, but not just that... It would be great to add a DM to env. drivers. This will make it possible to use more than one environment driver in runtime and make many people who struggle with it happy...
Hi Igor,
On 22 February 2017 at 00:35, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Philipp, Simon,
On 02/22/17 05:59, Simon Glass wrote:
Hi,
On 20 February 2017 at 02:18, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
On 20 Feb 2017, at 08:22, Igor Grinberg grinberg@compulab.co.il wrote:
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
Why? U-Boot can live pretty well w/o DT.
If U-Boot runs without DT, then nothing will/should change about how the setting is retrieved from CONFIG_ENV_OFFSET.
ok.
The platform that motivated this change is ARCH_SUNXI, which does not use per-board defines but aims to have one generic bootloader per-SoC.
Well, after a ten year experience with boars and different SoC vendors, I don't think it is possible... Unless all boards are copies of their respective reference design...
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. And that will solve the problem.
Doing this would still get into the way of architectures that want to build a single ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same across all board and vendor configurations. This can easily be handled with the (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
I think Kconfig would be enough for this, but please try your approach. The 'universal' thing will probably work if you don't have too many boards and they don't differ too much from each other...
If we decide to this, I’d at least like to introduce the function call to (the weak function) mmc_get_env_addr(…), so we can override this in the board code.
That is how it works today: include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
So putting this in the DT is the best (and least intrusive) option available.
Ok. I see your point now. Yet I think we should keep the DT to its purpose - which is to describe the h/w (and not the s/w placement layout).
Well actually we put things like flash map in there and the environment position seems like a very good thing to have there.
Well, I thought so too... But I had a discussion with kernel people some time ago and they discourage this approach and would not like to have the flash mapping in the DT.
I'm sorry to hear that, but it doesn't change the fact that it is very useful for dealing with hardware-specific differences between models.
Building the same software multiple times with different #ifdefs is often painful. Using a DT to handle these minor differences reduces the number of build combinations and simplifies testing.
So I like this patch. There is too compile-time configuration in U-Boot still...
Yeah, I know you like it ;-) but DT is not the place for it, unless DT specs. guys decide to change its purpose and place s/w configuration straps inside...
Although, U-Boot build process is not a pain at all, you might want to design another abstraction layer for s/w configuration straps. That way you will be able to keep the U-Boot core binary generic... Is it worse the effort? I don't know. Currently, having the board infrastructure, provides that layer and works fine.
At present in U-Boot DT is that layer. We don't need to ban useful additions like this and I really am not keen on adding another config mechanism.
In fact I wish we could support this for all environment types. Overall the environment drivers need some work to make them more similar...
Indeed, but not just that... It would be great to add a DM to env. drivers. This will make it possible to use more than one environment driver in runtime and make many people who struggle with it happy...
Yes.
Regards, Simon
Hi Simon,
On 03/03/17 06:53, Simon Glass wrote:
Hi Igor,
On 22 February 2017 at 00:35, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Philipp, Simon,
On 02/22/17 05:59, Simon Glass wrote:
Hi,
On 20 February 2017 at 02:18, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
On 20 Feb 2017, at 08:22, Igor Grinberg grinberg@compulab.co.il wrote:
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
Why? U-Boot can live pretty well w/o DT.
If U-Boot runs without DT, then nothing will/should change about how the setting is retrieved from CONFIG_ENV_OFFSET.
ok.
The platform that motivated this change is ARCH_SUNXI, which does not use per-board defines but aims to have one generic bootloader per-SoC.
Well, after a ten year experience with boars and different SoC vendors, I don't think it is possible... Unless all boards are copies of their respective reference design...
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. And that will solve the problem.
Doing this would still get into the way of architectures that want to build a single ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same across all board and vendor configurations. This can easily be handled with the (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
I think Kconfig would be enough for this, but please try your approach. The 'universal' thing will probably work if you don't have too many boards and they don't differ too much from each other...
If we decide to this, I’d at least like to introduce the function call to (the weak function) mmc_get_env_addr(…), so we can override this in the board code.
That is how it works today: include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
So putting this in the DT is the best (and least intrusive) option available.
Ok. I see your point now. Yet I think we should keep the DT to its purpose - which is to describe the h/w (and not the s/w placement layout).
Well actually we put things like flash map in there and the environment position seems like a very good thing to have there.
Well, I thought so too... But I had a discussion with kernel people some time ago and they discourage this approach and would not like to have the flash mapping in the DT.
I'm sorry to hear that, but it doesn't change the fact that it is very useful for dealing with hardware-specific differences between models.
According to kernel guys, these are not h/w specific, but rather device specific... and it actually makes sense - on the same h/w design different applications can use different flash mapping - just like the block devices.
Building the same software multiple times with different #ifdefs is often painful. Using a DT to handle these minor differences reduces the number of build combinations and simplifies testing.
Well, partially... I agree that building different binaries with #ifdefs is very painful and I highly discourage this approach. Using a DT to handle these differences, IMO, is better but also not that good, as it requires building multiple DT binaries (which I consider the same as multiple U-Boot binaries) and therefore does not reduce the number of build combinations... you just abstract the problem to a separate binary.
I'm more in favor of run time detection and adjustment of static data along with dynamic code run.
Regarding the environment, I think it would be great to have U-Boot detect where environment location is and proceed with it. Just like the boot sequence stuff...
So I like this patch. There is too compile-time configuration in U-Boot still...
Yeah, I know you like it ;-) but DT is not the place for it, unless DT specs. guys decide to change its purpose and place s/w configuration straps inside...
Although, U-Boot build process is not a pain at all, you might want to design another abstraction layer for s/w configuration straps. That way you will be able to keep the U-Boot core binary generic... Is it worse the effort? I don't know. Currently, having the board infrastructure, provides that layer and works fine.
At present in U-Boot DT is that layer. We don't need to ban useful additions like this and I really am not keen on adding another config mechanism.
So, how would you sync between the DTs in U-Boot and kernel?
Hi Igor,
On 5 March 2017 at 01:39, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Simon,
On 03/03/17 06:53, Simon Glass wrote:
Hi Igor,
On 22 February 2017 at 00:35, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Philipp, Simon,
On 02/22/17 05:59, Simon Glass wrote:
Hi,
On 20 February 2017 at 02:18, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
On 20 Feb 2017, at 08:22, Igor Grinberg grinberg@compulab.co.il wrote:
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
Why? U-Boot can live pretty well w/o DT.
If U-Boot runs without DT, then nothing will/should change about how the setting is retrieved from CONFIG_ENV_OFFSET.
ok.
The platform that motivated this change is ARCH_SUNXI, which does not use per-board defines but aims to have one generic bootloader per-SoC.
Well, after a ten year experience with boars and different SoC vendors, I don't think it is possible... Unless all boards are copies of their respective reference design...
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. And that will solve the problem.
Doing this would still get into the way of architectures that want to build a single ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same across all board and vendor configurations. This can easily be handled with the (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
I think Kconfig would be enough for this, but please try your approach. The 'universal' thing will probably work if you don't have too many boards and they don't differ too much from each other...
If we decide to this, I’d at least like to introduce the function call to (the weak function) mmc_get_env_addr(…), so we can override this in the board code.
That is how it works today: include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
So putting this in the DT is the best (and least intrusive) option available.
Ok. I see your point now. Yet I think we should keep the DT to its purpose - which is to describe the h/w (and not the s/w placement layout).
Well actually we put things like flash map in there and the environment position seems like a very good thing to have there.
Well, I thought so too... But I had a discussion with kernel people some time ago and they discourage this approach and would not like to have the flash mapping in the DT.
I'm sorry to hear that, but it doesn't change the fact that it is very useful for dealing with hardware-specific differences between models.
According to kernel guys, these are not h/w specific, but rather device specific... and it actually makes sense - on the same h/w design different applications can use different flash mapping - just like the block devices.
Really? That sounds like a pretty esoteric case to me.
In my experience the flash mapping is normally fixed at production time and there may be parts that are write-protected, e.g. with a hardware switch. The flash map generally needs to be understood by both firmware and kernel. It is as much a feature of the hardware as which serial console to use.
Building the same software multiple times with different #ifdefs is often painful. Using a DT to handle these minor differences reduces the number of build combinations and simplifies testing.
Well, partially... I agree that building different binaries with #ifdefs is very painful and I highly discourage this approach. Using a DT to handle these differences, IMO, is better but also not that good, as it requires building multiple DT binaries (which I consider the same as multiple U-Boot binaries) and therefore does not reduce the number of build combinations... you just abstract the problem to a separate binary.
It has a big positive impact on complexity though. You build U-Boot once and attach the appropriate device tree at the end, perhaps during production. This is similar to how Linux boots on different hardware times, with just a different DT.
I'm more in favor of run time detection and adjustment of static data along with dynamic code run.
Well that is certainly possible. I think the trade-off here depends on your production syste,.
Regarding the environment, I think it would be great to have U-Boot detect where environment location is and proceed with it. Just like the boot sequence stuff...
Well that is a separate feature from what is being discussed here. But IMO a solid flash map available to everything is better. You can define where the environment is, and anything else you find.
So I like this patch. There is too compile-time configuration in U-Boot still...
Yeah, I know you like it ;-) but DT is not the place for it, unless DT specs. guys decide to change its purpose and place s/w configuration straps inside...
Although, U-Boot build process is not a pain at all, you might want to design another abstraction layer for s/w configuration straps. That way you will be able to keep the U-Boot core binary generic... Is it worse the effort? I don't know. Currently, having the board infrastructure, provides that layer and works fine.
At present in U-Boot DT is that layer. We don't need to ban useful additions like this and I really am not keen on adding another config mechanism.
So, how would you sync between the DTs in U-Boot and kernel?
Do you mean how to sync the flashmap? It can be passed down to Linux in the DT, if that is what you are asking?
Regards, Simon
Hi Simon,
On 03/12/17 22:21, Simon Glass wrote:
Hi Igor,
On 5 March 2017 at 01:39, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Simon,
On 03/03/17 06:53, Simon Glass wrote:
Hi Igor,
On 22 February 2017 at 00:35, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Philipp, Simon,
On 02/22/17 05:59, Simon Glass wrote:
Hi,
On 20 February 2017 at 02:18, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
On 20 Feb 2017, at 08:22, Igor Grinberg grinberg@compulab.co.il wrote:
That sounds too odd... DT's purpose is to describe the h/w... and that does not look so... We also, have a dt file name in the environment, so this creates will create a chicken and an egg problem…
I don’t really follow… as far as I knew the DT name would have to come from some other source anyway, as the device containing the env might only be described through the device tree (e.g. mmc0).
Why? U-Boot can live pretty well w/o DT.
If U-Boot runs without DT, then nothing will/should change about how the setting is retrieved from CONFIG_ENV_OFFSET.
ok.
The platform that motivated this change is ARCH_SUNXI, which does not use per-board defines but aims to have one generic bootloader per-SoC.
Well, after a ten year experience with boars and different SoC vendors, I don't think it is possible... Unless all boards are copies of their respective reference design...
I really don't think we should go that direction. DT is not meant to provide a solution to all your problems...
I don’t see how this is different from other entries in chosen and config as of today: common/autoboot.c allows an override through /config/bootdelay common/board_r.c uses /config/load-environment common/cli.c can pull in /config/bootcmd drivers/serial/serial-uclass.c uses /chosen/stdout-path
In fact, it is the absence of this mechanism that is causing problems today: CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and matching #ifdef primitives in a shared header (sunxi-common.h in our case).
Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. And that will solve the problem.
Doing this would still get into the way of architectures that want to build a single ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same across all board and vendor configurations. This can easily be handled with the (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
I think Kconfig would be enough for this, but please try your approach. The 'universal' thing will probably work if you don't have too many boards and they don't differ too much from each other...
If we decide to this, I’d at least like to introduce the function call to (the weak function) mmc_get_env_addr(…), so we can override this in the board code.
That is how it works today: include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
So putting this in the DT is the best (and least intrusive) option available.
Ok. I see your point now. Yet I think we should keep the DT to its purpose - which is to describe the h/w (and not the s/w placement layout).
Well actually we put things like flash map in there and the environment position seems like a very good thing to have there.
Well, I thought so too... But I had a discussion with kernel people some time ago and they discourage this approach and would not like to have the flash mapping in the DT.
I'm sorry to hear that, but it doesn't change the fact that it is very useful for dealing with hardware-specific differences between models.
According to kernel guys, these are not h/w specific, but rather device specific... and it actually makes sense - on the same h/w design different applications can use different flash mapping - just like the block devices.
Really? That sounds like a pretty esoteric case to me.
Well, yes.. It might be esoteric, but strong enough to push back on some patches we have sent to the DT in kernel...
In my experience the flash mapping is normally fixed at production time and there may be parts that are write-protected, e.g. with a hardware switch.
Well, I think this is completely true in x86 world... where things are much more strict.
The flash map generally needs to be understood by both firmware and kernel.
True, but that does not have to come from DT... They prefer the kernel command line over DT.
It is as much a feature of the hardware as which serial console to use.
This one I cannot agree to... We have had several cases where customers changed the flash partitioning for their application needs etc. So, that's why I've said it makes some sense.
Building the same software multiple times with different #ifdefs is often painful. Using a DT to handle these minor differences reduces the number of build combinations and simplifies testing.
Well, partially... I agree that building different binaries with #ifdefs is very painful and I highly discourage this approach. Using a DT to handle these differences, IMO, is better but also not that good, as it requires building multiple DT binaries (which I consider the same as multiple U-Boot binaries) and therefore does not reduce the number of build combinations... you just abstract the problem to a separate binary.
It has a big positive impact on complexity though. You build U-Boot once and attach the appropriate device tree at the end, perhaps during production. This is similar to how Linux boots on different hardware times, with just a different DT.
Right! The thing is U-Boot is not Linux - it is a SoC/board specific bootloader.
I'm more in favor of run time detection and adjustment of static data along with dynamic code run.
Well that is certainly possible. I think the trade-off here depends on your production syste,.
Yep. We have lots of board configuration combinations and we certainly cannot build a DT for each combination... What we can do (and we are currently really good at this) is detect things at runtime and update the DT with the correct data for Linux to boot.
Regarding the environment, I think it would be great to have U-Boot detect where environment location is and proceed with it. Just like the boot sequence stuff...
Well that is a separate feature from what is being discussed here. But IMO a solid flash map available to everything is better. You can define where the environment is, and anything else you find.
So I like this patch. There is too compile-time configuration in U-Boot still...
Yeah, I know you like it ;-) but DT is not the place for it, unless DT specs. guys decide to change its purpose and place s/w configuration straps inside...
Although, U-Boot build process is not a pain at all, you might want to design another abstraction layer for s/w configuration straps. That way you will be able to keep the U-Boot core binary generic... Is it worse the effort? I don't know. Currently, having the board infrastructure, provides that layer and works fine.
At present in U-Boot DT is that layer. We don't need to ban useful additions like this and I really am not keen on adding another config mechanism.
So, how would you sync between the DTs in U-Boot and kernel?
Do you mean how to sync the flashmap? It can be passed down to Linux in the DT, if that is what you are asking?
No, I mean the DT source code.
Hi Igor,
On 14 March 2017 at 07:11, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Simon,
On 03/12/17 22:21, Simon Glass wrote:
Hi Igor,
On 5 March 2017 at 01:39, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Simon,
On 03/03/17 06:53, Simon Glass wrote:
Hi Igor,
On 22 February 2017 at 00:35, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Philipp, Simon,
On 02/22/17 05:59, Simon Glass wrote:
Hi,
On 20 February 2017 at 02:18, Dr. Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote: > > On 20 Feb 2017, at 08:22, Igor Grinberg grinberg@compulab.co.il wrote: > > That sounds too odd... > DT's purpose is to describe the h/w... and that does not look so... > We also, have a dt file name in the environment, so this creates will create > a chicken and an egg problem… > > > I don’t really follow… as far as I knew the DT name would have to come > from some other source anyway, as the device containing the env might > only be described through the device tree (e.g. mmc0). > > > Why? U-Boot can live pretty well w/o DT. > > > If U-Boot runs without DT, then nothing will/should change about how the > setting > is retrieved from CONFIG_ENV_OFFSET.
ok.
> > The platform that motivated this change is ARCH_SUNXI, which does not use > per-board defines but aims to have one generic bootloader per-SoC.
Well, after a ten year experience with boars and different SoC vendors, I don't think it is possible... Unless all boards are copies of their respective reference design...
> > I really don't think we should go that direction. DT is not meant to provide > a solution to all your problems... > > > I don’t see how this is different from other entries in chosen and config as > of today: > common/autoboot.c allows an override through /config/bootdelay > common/board_r.c uses /config/load-environment > common/cli.c can pull in /config/bootcmd > drivers/serial/serial-uclass.c uses /chosen/stdout-path > > In fact, it is the absence of this mechanism that is causing problems today: > CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would > need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and > matching #ifdef primitives in a shared header (sunxi-common.h in our case). > > > Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig. > And that will solve the problem. > > > Doing this would still get into the way of architectures that want to build > a single > ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same > across all board and vendor configurations. This can easily be handled with > the > (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
I think Kconfig would be enough for this, but please try your approach. The 'universal' thing will probably work if you don't have too many boards and they don't differ too much from each other...
> > If we decide to this, I’d at least like to introduce the function call to > (the weak > function) mmc_get_env_addr(…), so we can override this in the board code.
That is how it works today: include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
> > So putting this in the DT is the best (and least intrusive) option > available. > > > Ok. I see your point now. Yet I think we should keep the DT to its purpose - > which > is to describe the h/w (and not the s/w placement layout).
Well actually we put things like flash map in there and the environment position seems like a very good thing to have there.
Well, I thought so too... But I had a discussion with kernel people some time ago and they discourage this approach and would not like to have the flash mapping in the DT.
I'm sorry to hear that, but it doesn't change the fact that it is very useful for dealing with hardware-specific differences between models.
According to kernel guys, these are not h/w specific, but rather device specific... and it actually makes sense - on the same h/w design different applications can use different flash mapping - just like the block devices.
Really? That sounds like a pretty esoteric case to me.
Well, yes.. It might be esoteric, but strong enough to push back on some patches we have sent to the DT in kernel...
In my experience the flash mapping is normally fixed at production time and there may be parts that are write-protected, e.g. with a hardware switch.
Well, I think this is completely true in x86 world... where things are much more strict.
It depends on the board, but all ARM Chromebooks have the same restriction, for example.
The flash map generally needs to be understood by both firmware and kernel.
True, but that does not have to come from DT... They prefer the kernel command line over DT.
It is as much a feature of the hardware as which serial console to use.
This one I cannot agree to... We have had several cases where customers changed the flash partitioning for their application needs etc. So, that's why I've said it makes some sense.
If you don't need a fixed partition then you have some options:
- U-Boot determines the partition layout and passes it to Linux (e.g. via command line) - U-Boot doesn't touch the flash and Linux can work it out
But if you don't want a fixed layout, why are you talking about putting it in DT in U-Boot? That would not make sense. The case for putting it in U-Boot's DT is when it is fixed.
Building the same software multiple times with different #ifdefs is often painful. Using a DT to handle these minor differences reduces the number of build combinations and simplifies testing.
Well, partially... I agree that building different binaries with #ifdefs is very painful and I highly discourage this approach. Using a DT to handle these differences, IMO, is better but also not that good, as it requires building multiple DT binaries (which I consider the same as multiple U-Boot binaries) and therefore does not reduce the number of build combinations... you just abstract the problem to a separate binary.
It has a big positive impact on complexity though. You build U-Boot once and attach the appropriate device tree at the end, perhaps during production. This is similar to how Linux boots on different hardware times, with just a different DT.
Right! The thing is U-Boot is not Linux - it is a SoC/board specific bootloader.
The complexity of SoCs is growing all the time, and if you have to support a lot of variants of one SoC it often makes sense to describe the variations in a device tree, just as with Linux.
I'm more in favor of run time detection and adjustment of static data along with dynamic code run.
Well that is certainly possible. I think the trade-off here depends on your production syste,.
Yep. We have lots of board configuration combinations and we certainly cannot build a DT for each combination... What we can do (and we are currently really good at this) is detect things at runtime and update the DT with the correct data for Linux to boot.
Sounds fine to me. There are many ways to do this.
Regarding the environment, I think it would be great to have U-Boot detect where environment location is and proceed with it. Just like the boot sequence stuff...
Well that is a separate feature from what is being discussed here. But IMO a solid flash map available to everything is better. You can define where the environment is, and anything else you find.
So I like this patch. There is too compile-time configuration in U-Boot still...
Yeah, I know you like it ;-) but DT is not the place for it, unless DT specs. guys decide to change its purpose and place s/w configuration straps inside...
Although, U-Boot build process is not a pain at all, you might want to design another abstraction layer for s/w configuration straps. That way you will be able to keep the U-Boot core binary generic... Is it worse the effort? I don't know. Currently, having the board infrastructure, provides that layer and works fine.
At present in U-Boot DT is that layer. We don't need to ban useful additions like this and I really am not keen on adding another config mechanism.
So, how would you sync between the DTs in U-Boot and kernel?
Do you mean how to sync the flashmap? It can be passed down to Linux in the DT, if that is what you are asking?
No, I mean the DT source code.
If Linux wants the flashmap in its command line (which is of course inside the DT passed to Linux :-) then that's fine. Whatever it wants. My point is that with U-Boot we should support having the flash map in U-Boot's DT for its own use, for situations where the flashmap is fixed. While I understand that Linux makes its own decisions on how it wants to receive the flashmap from the boot loader, it is better for U-Boot to make its own decisions on that matter for its internal use too.
Regards. Simon
Hi Philipp,
On 17 February 2017 at 10:28, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:
This introduces the ability to override the environment offets from the device tree by setting the following nodes in '/config': 'u-boot,mmc-env-offset' - overrides CONFIG_ENV_OFFSET 'u-boot,mmc-env-offset-redundant' - overrides CONFIG_ENV_OFFSET_REDUND
To keep with the previous logic, the CONFIG_* defines still need to be available and the statically defined values become the defaults, when the corresponding properties are not set in the device-tree.
Signed-off-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
common/env_mmc.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)
Can you please resend this with an update to config.txt?
I think longer term we should support this for all environment types, but this is a good start.
Regards, Simon
participants (5)
-
Dr. Philipp Tomsich -
Igor Grinberg -
Jaehoon Chung -
Philipp Tomsich
-
Simon Glass