* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
@ 2018-02-07 20:45 Goldschmidt Simon
2018-02-07 21:18 ` York Sun
0 siblings, 1 reply; 21+ messages in thread
From: Goldschmidt Simon @ 2018-02-07 20:45 UTC (permalink / raw)
To: u-boot
On 02/07/2018 21:18, York Sun wrote:
> On 02/07/2018 12:43 AM, Maxime Ripard wrote:
>> Hi,
>>
>> On Tue, Jan 30, 2018 at 11:02:49PM +0000, York Sun wrote:
>>> On 01/30/2018 12:16 PM, York Sun wrote:
>>>> On 01/30/2018 11:40 AM, York Sun wrote:
>>>>> On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
>>>>>> On 23.01.2018 21:16, Maxime Ripard wrote:
>>>>>>> Now that we have everything in place to support multiple environment, let's
>>>>>>> make sure the current code can use it.
>>>>>>>
>>>>>>> The priority used between the various environment is the same one that was
>>>>>>> used in the code previously.
>>>>>>>
>>>>>>> At read / init times, the highest priority environment is going to be
>>>>>>> detected,
>>>>>>
>>>>>> Does priority handling really work here? Most env drivers seem to ignore
>>>>>> the return value of env_import and may thus return success although
>>>>>> importing the environment failed (only reading the data from the device
>>>>>> succeeded).
>>>>>>
>>>>>> This is from reading the code, I haven't had a chance to test this, yet.
>>>>>
>>>>> It is broken on my LS1043ARDB with simply NOR flash. I am trying to
>>>>> determine what went wrong.
>>>>>
>>>>
>>>> I found the problem. The variable "env_load_location" is static. It is
>>>> probably not write-able during booting from flash. It is expected to be
>>>> set during ENVOP_INIT. But if I print this variable, it has
>>>> ENVL_UNKNOWN. I can make it work by moving env_load_location to global
>>>> data structure.
>>
>> That would work for me.
>>
>>>> That being said, this addition of multiple environments really slows
>>>> down the booting process for me. I see every time env_get_char() is
>>>> called, env_driver_lookup() runs. A simple call of env_get_f() gets
>>>> slowed down dramatically. I didn't find out where the most time is spent
>>>> yet.
>>>>
>>>> Does anyone else experience this unbearable slowness?
>>>>
>>>
>>> I found the problem. In patch #3 in this series, the default get_char()
>>> was dropped so there is no driver for a plain NOR flash. A quick (and
>>> maybe dirty) fix is this
>>>
>>>
>>> diff --git a/env/env.c b/env/env.c
>>> index edfb575..210bae2 100644
>>> --- a/env/env.c
>>> +++ b/env/env.c
>>> @@ -159,7 +159,7 @@ int env_get_char(int index)
>>> int ret;
>>>
>>> if (!drv->get_char)
>>> - continue;
>>> + return *(uchar *)(gd->env_addr + index);
>>>
>>> if (!env_has_inited(drv->location))
>>> continue;
>>
>> And this too.
>
> If you agree with this fix (actually revert your change earlier), I can
> send out a patch.
I still think we should get rid of the 'get_char' callback for
the env drivers. While that could have made sense for some boards
before the conversion to multiple environments (although I
doubt that, as the environment is *not* checked for
validity in this call), its meaning is totally lost when having
multiple env drivers active.
The whole purpose of multiple env drivers is that we select a
valid driver in the 'load' callback. How could we possibly know
that the 'get_char' callback of the highest prio env driver is
what we want?
I'd rather make 'env_get_char' weak and let boards decide if they
really want this behaviour.
A quick search through the current code base shows me *no* usage
of 'get_char' in nvram.c (CONFIG_SYS_NVRAM_ACCESS_ROUTINE is never
defined) and only 7 defconfigs that use ENV_IS_IN_EEPROM:
imx31_phycore_defconfig
imx31_phycore_eet_defconfig
km_kirkwood_128m16_defconfig
km_kirkwood_defconfig
km_kirkwood_pci_defconfig
mgcoge3un_defconfig
portl2_defconfig
Are these seven boards worth keeping this feature?
Simon
>>
>>> With this temporary fix, my flash chip works again and I can boot all
>>> the way up in a timely manner. I still don't like to call
>>> env_driver_lookup() thousands of times to get a simple env variable.
>>>
>>> Can Maxime post a quick post soon?
>>
>> Given that you already made all the debugging, and the patches, and I
>> have no way to test, I guess it would make more sense if you did it :)
>
> Yes, I have tested on my boards. I will send out this patch.
>
> York
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-07 20:45 [U-Boot] [PATCH v3 09/15] env: Support multiple environments Goldschmidt Simon
@ 2018-02-07 21:18 ` York Sun
2018-02-08 5:30 ` Simon Goldschmidt
0 siblings, 1 reply; 21+ messages in thread
From: York Sun @ 2018-02-07 21:18 UTC (permalink / raw)
To: u-boot
On 02/07/2018 12:45 PM, Goldschmidt Simon wrote:
> On 02/07/2018 21:18, York Sun wrote:
>> On 02/07/2018 12:43 AM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Tue, Jan 30, 2018 at 11:02:49PM +0000, York Sun wrote:
>>>> On 01/30/2018 12:16 PM, York Sun wrote:
>>>>> On 01/30/2018 11:40 AM, York Sun wrote:
>>>>>> On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
>>>>>>> On 23.01.2018 21:16, Maxime Ripard wrote:
>>>>>>>> Now that we have everything in place to support multiple environment, let's
>>>>>>>> make sure the current code can use it.
>>>>>>>>
>>>>>>>> The priority used between the various environment is the same one that was
>>>>>>>> used in the code previously.
>>>>>>>>
>>>>>>>> At read / init times, the highest priority environment is going to be
>>>>>>>> detected,
>>>>>>>
>>>>>>> Does priority handling really work here? Most env drivers seem to ignore
>>>>>>> the return value of env_import and may thus return success although
>>>>>>> importing the environment failed (only reading the data from the device
>>>>>>> succeeded).
>>>>>>>
>>>>>>> This is from reading the code, I haven't had a chance to test this, yet.
>>>>>>
>>>>>> It is broken on my LS1043ARDB with simply NOR flash. I am trying to
>>>>>> determine what went wrong.
>>>>>>
>>>>>
>>>>> I found the problem. The variable "env_load_location" is static. It is
>>>>> probably not write-able during booting from flash. It is expected to be
>>>>> set during ENVOP_INIT. But if I print this variable, it has
>>>>> ENVL_UNKNOWN. I can make it work by moving env_load_location to global
>>>>> data structure.
>>>
>>> That would work for me.
Actually I am not a big fun to using global data. It increases size for
everybody. But I don't see a way you can save the variable before
relocation.
>>>
>>>>> That being said, this addition of multiple environments really slows
>>>>> down the booting process for me. I see every time env_get_char() is
>>>>> called, env_driver_lookup() runs. A simple call of env_get_f() gets
>>>>> slowed down dramatically. I didn't find out where the most time is spent
>>>>> yet.
>>>>>
>>>>> Does anyone else experience this unbearable slowness?
>>>>>
>>>>
>>>> I found the problem. In patch #3 in this series, the default get_char()
>>>> was dropped so there is no driver for a plain NOR flash. A quick (and
>>>> maybe dirty) fix is this
>>>>
>>>>
>>>> diff --git a/env/env.c b/env/env.c
>>>> index edfb575..210bae2 100644
>>>> --- a/env/env.c
>>>> +++ b/env/env.c
>>>> @@ -159,7 +159,7 @@ int env_get_char(int index)
>>>> int ret;
>>>>
>>>> if (!drv->get_char)
>>>> - continue;
>>>> + return *(uchar *)(gd->env_addr + index);
>>>>
>>>> if (!env_has_inited(drv->location))
>>>> continue;
>>>
>>> And this too.
>>
>> If you agree with this fix (actually revert your change earlier), I can
>> send out a patch.
>
> I still think we should get rid of the 'get_char' callback for
> the env drivers. While that could have made sense for some boards
> before the conversion to multiple environments (although I
> doubt that, as the environment is *not* checked for
> validity in this call), its meaning is totally lost when having
> multiple env drivers active.
>
> The whole purpose of multiple env drivers is that we select a
> valid driver in the 'load' callback. How could we possibly know
> that the 'get_char' callback of the highest prio env driver is
> what we want?
>
> I'd rather make 'env_get_char' weak and let boards decide if they
> really want this behaviour.
>
> A quick search through the current code base shows me *no* usage
> of 'get_char' in nvram.c (CONFIG_SYS_NVRAM_ACCESS_ROUTINE is never
> defined) and only 7 defconfigs that use ENV_IS_IN_EEPROM:
>
> imx31_phycore_defconfig
> imx31_phycore_eet_defconfig
> km_kirkwood_128m16_defconfig
> km_kirkwood_defconfig
> km_kirkwood_pci_defconfig
> mgcoge3un_defconfig
> portl2_defconfig
>
> Are these seven boards worth keeping this feature?
>
Simon,
Adding multiple environments seems to be an improvement. But this fell
through the cracks. I don't know if other boards also read env before
relocation. All my boards reads hwconfig before relocation. Having to
create a new function for all of them doesn't look appealing to me.
York
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-07 21:18 ` York Sun
@ 2018-02-08 5:30 ` Simon Goldschmidt
0 siblings, 0 replies; 21+ messages in thread
From: Simon Goldschmidt @ 2018-02-08 5:30 UTC (permalink / raw)
To: u-boot
On 07.02.2018 22:18, York Sun wrote:
> On 02/07/2018 12:45 PM, Goldschmidt Simon wrote:
>> On 02/07/2018 21:18, York Sun wrote:
>>> On 02/07/2018 12:43 AM, Maxime Ripard wrote:
>>>> Hi,
>>>>
>>>> On Tue, Jan 30, 2018 at 11:02:49PM +0000, York Sun wrote:
>>>>> On 01/30/2018 12:16 PM, York Sun wrote:
>>>>>> On 01/30/2018 11:40 AM, York Sun wrote:
>>>>>>> On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
>>>>>>>> On 23.01.2018 21:16, Maxime Ripard wrote:
>>>>>>>>> Now that we have everything in place to support multiple environment, let's
>>>>>>>>> make sure the current code can use it.
>>>>>>>>>
>>>>>>>>> The priority used between the various environment is the same one that was
>>>>>>>>> used in the code previously.
>>>>>>>>>
>>>>>>>>> At read / init times, the highest priority environment is going to be
>>>>>>>>> detected,
>>>>>>>> Does priority handling really work here? Most env drivers seem to ignore
>>>>>>>> the return value of env_import and may thus return success although
>>>>>>>> importing the environment failed (only reading the data from the device
>>>>>>>> succeeded).
>>>>>>>>
>>>>>>>> This is from reading the code, I haven't had a chance to test this, yet.
>>>>>>> It is broken on my LS1043ARDB with simply NOR flash. I am trying to
>>>>>>> determine what went wrong.
>>>>>>>
>>>>>> I found the problem. The variable "env_load_location" is static. It is
>>>>>> probably not write-able during booting from flash. It is expected to be
>>>>>> set during ENVOP_INIT. But if I print this variable, it has
>>>>>> ENVL_UNKNOWN. I can make it work by moving env_load_location to global
>>>>>> data structure.
>>>> That would work for me.
> Actually I am not a big fun to using global data. It increases size for
> everybody. But I don't see a way you can save the variable before
> relocation.
>
>>>>>> That being said, this addition of multiple environments really slows
>>>>>> down the booting process for me. I see every time env_get_char() is
>>>>>> called, env_driver_lookup() runs. A simple call of env_get_f() gets
>>>>>> slowed down dramatically. I didn't find out where the most time is spent
>>>>>> yet.
>>>>>>
>>>>>> Does anyone else experience this unbearable slowness?
>>>>>>
>>>>> I found the problem. In patch #3 in this series, the default get_char()
>>>>> was dropped so there is no driver for a plain NOR flash. A quick (and
>>>>> maybe dirty) fix is this
>>>>>
>>>>>
>>>>> diff --git a/env/env.c b/env/env.c
>>>>> index edfb575..210bae2 100644
>>>>> --- a/env/env.c
>>>>> +++ b/env/env.c
>>>>> @@ -159,7 +159,7 @@ int env_get_char(int index)
>>>>> int ret;
>>>>>
>>>>> if (!drv->get_char)
>>>>> - continue;
>>>>> + return *(uchar *)(gd->env_addr + index);
>>>>>
>>>>> if (!env_has_inited(drv->location))
>>>>> continue;
>>>> And this too.
>>> If you agree with this fix (actually revert your change earlier), I can
>>> send out a patch.
>> I still think we should get rid of the 'get_char' callback for
>> the env drivers. While that could have made sense for some boards
>> before the conversion to multiple environments (although I
>> doubt that, as the environment is *not* checked for
>> validity in this call), its meaning is totally lost when having
>> multiple env drivers active.
>>
>> The whole purpose of multiple env drivers is that we select a
>> valid driver in the 'load' callback. How could we possibly know
>> that the 'get_char' callback of the highest prio env driver is
>> what we want?
>>
>> I'd rather make 'env_get_char' weak and let boards decide if they
>> really want this behaviour.
>>
>> A quick search through the current code base shows me *no* usage
>> of 'get_char' in nvram.c (CONFIG_SYS_NVRAM_ACCESS_ROUTINE is never
>> defined) and only 7 defconfigs that use ENV_IS_IN_EEPROM:
>>
>> imx31_phycore_defconfig
>> imx31_phycore_eet_defconfig
>> km_kirkwood_128m16_defconfig
>> km_kirkwood_defconfig
>> km_kirkwood_pci_defconfig
>> mgcoge3un_defconfig
>> portl2_defconfig
>>
>> Are these seven boards worth keeping this feature?
>>
> Simon,
>
> Adding multiple environments seems to be an improvement. But this fell
> through the cracks. I don't know if other boards also read env before
> relocation. All my boards reads hwconfig before relocation. Having to
> create a new function for all of them doesn't look appealing to me.
The change I proposed would be to restore the old behavior but kick out
the byte-by-byte reading from eeprom and nvram. My understanding was you
are using flash and were reading from environment in ram, not in nvram
or eeprom.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 00/15] env: Multiple env support and env transition for sunxi
@ 2018-01-23 20:16 Maxime Ripard
2018-01-23 20:16 ` [U-Boot] [PATCH v3 09/15] env: Support multiple environments Maxime Ripard
0 siblings, 1 reply; 21+ messages in thread
From: Maxime Ripard @ 2018-01-23 20:16 UTC (permalink / raw)
To: u-boot
Hi,
Here is a second attempt at transitioning away from the MMC raw environment
to a FAT-based one for Allwinner SoCs. Since the RFC was quite well
received, I actually tested it and fixed a few rough edges.
You'll find the first RFC here for reference:
https://lists.denx.de/pipermail/u-boot/2017-October/310111.html
And the second that originated in this series:
https://lists.denx.de/pipermail/u-boot/2017-November/311608.html
I gave it a travis run, and it passes all tests:
https://travis-ci.org/mripard/u-boot
Thanks!
Maxime
Original description of the patches:
The fundamental issue I'm trying to adress is that we've had for a
very long time the assumption that the main U-Boot binary wouldn't
exceed around 500 bytes.
However, we're starting to get real close to that limit, and are
running out of silver bullets to deal with the consequences of having
a bigger U-Boot binary, the main consequence being that we would
have some overlap between the environment and U-Boot.
One way to address this that has been suggested by Tom is to move away
from the raw MMC environment to a FAT-based one. This would allow us
to slowly migrate away, and eventually remove the MMC-raw option
entirely to reclaim that space for the binary.
That cannot be done in a single release however, since we might have
environments in the wild already that people rely on. And since we
always encouraged people to use the raw MMC environment, noone would
expect that.
This is even worse since some platforms are using the U-Boot
environment to deal with implement their upgrade mechanism, such as
mender.io, and force moving the environment would break any further
upgrade.
The suggested implementation is to allow U-Boot to compile multiple
environments backend at once, based on the work done by Simon. The
default behaviour shouldn't change obviously. We can then piggy-back
on this to tweak on a per-board basis the environment lookup algorithm
to always favour the FAT-based environment and then fallback to the
MMC. It will allow us to migrate a raw-MMC user to a FAT based
solution as soon as they update their environment (assuming that there
is a bootable FAT partition in the system).
This has just been compile tested on sunxi so far, and I'd like
general comments on the approach taken. Obviously, this will need to
work properly before being merged.
Changes from v2:
- Fix a bug showing up on travis that resulted in the vexpress_ca_15 DHCP
unit test failing. This was due to an improper return of ENVL_UNKNOWN.
- Added Simon reviewed-by tag
Changes from v1:
- Collect tags
- Rebased on v2018.01
- Fixed build failures on a couple of boards
- Added back the message and the error code when an environment is
failing
- Added some comments about the printf in environments
- Added a build time check about the number of our locations array to see
if we're overflowing the location variable
- Fixed the drv->init return code being ignored
- Added helpers to manage the init status
- Changed the ENVO prefix for the operations to ENVOP
- Added some comments and documentation
Changes from the RFC:
- Added more useful messages to see where we're loading / saving
- Init all the environments no matter what, and the deal with whatever
env we want to pick at load time
- Added the various tags collected
Maxime Ripard (15):
cmd: nvedit: Get rid of the env lookup
env: Rename env_driver_lookup_default and env_get_default_location
env: Pass additional parameters to the env lookup function
env: Make the env save message a bit more explicit
env: Make it explicit where we're loading our environment from
env: fat: Make the debug messages play a little nicer
env: mmc: Make the debug messages play a little nicer
env: common: Make the debug messages play a little nicer
env: Support multiple environments
env: Initialise all the environments
env: mmc: depends on the MMC framework
env: Allow to build multiple environments in Kconfig
env: Mark env_get_location as weak
sunxi: Transition from the MMC to a FAT-based environment
env: sunxi: Enable FAT-based environment support by default
board/sunxi/board.c | 16 ++-
cmd/nvedit.c | 4 +-
configs/MPC8313ERDB_NAND_33_defconfig | 1 +-
configs/MPC8313ERDB_NAND_66_defconfig | 1 +-
configs/cl-som-imx7_defconfig | 1 +-
configs/microblaze-generic_defconfig | 1 +-
env/Kconfig | 70 ++++----
env/common.c | 2 +-
env/env.c | 251 +++++++++++++++++++--------
env/fat.c | 25 ++-
env/mmc.c | 1 +-
include/asm-generic/global_data.h | 1 +-
include/environment.h | 15 +-
13 files changed, 266 insertions(+), 123 deletions(-)
base-commit: f3dd87e0b98999a78e500e8c6d2b063ebadf535a
--
git-series 0.9.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-01-23 20:16 [U-Boot] [PATCH v3 00/15] env: Multiple env support and env transition for sunxi Maxime Ripard
@ 2018-01-23 20:16 ` Maxime Ripard
2018-01-30 8:19 ` Simon Goldschmidt
2018-02-06 8:09 ` Simon Goldschmidt
0 siblings, 2 replies; 21+ messages in thread
From: Maxime Ripard @ 2018-01-23 20:16 UTC (permalink / raw)
To: u-boot
Now that we have everything in place to support multiple environment, let's
make sure the current code can use it.
The priority used between the various environment is the same one that was
used in the code previously.
At read / init times, the highest priority environment is going to be
detected, and we'll use the same one without lookup during writes. This
should implement the same behaviour than we currently have.
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
env/env.c | 80 +++++++++++++++++++++++++++++++++++---------------------
1 file changed, 50 insertions(+), 30 deletions(-)
diff --git a/env/env.c b/env/env.c
index 906f28ee50a1..1182fdb545db 100644
--- a/env/env.c
+++ b/env/env.c
@@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
return NULL;
}
+static enum env_location env_locations[] = {
+#ifdef CONFIG_ENV_IS_IN_EEPROM
+ ENVL_EEPROM,
+#endif
+#ifdef CONFIG_ENV_IS_IN_FAT
+ ENVL_FAT,
+#endif
+#ifdef CONFIG_ENV_IS_IN_FLASH
+ ENVL_FLASH,
+#endif
+#ifdef CONFIG_ENV_IS_IN_MMC
+ ENVL_MMC,
+#endif
+#ifdef CONFIG_ENV_IS_IN_NAND
+ ENVL_NAND,
+#endif
+#ifdef CONFIG_ENV_IS_IN_NVRAM
+ ENVL_NVRAM,
+#endif
+#ifdef CONFIG_ENV_IS_IN_REMOTE
+ ENVL_REMOTE,
+#endif
+#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
+ ENVL_SPI_FLASH,
+#endif
+#ifdef CONFIG_ENV_IS_IN_UBI
+ ENVL_UBI,
+#endif
+#ifdef CONFIG_ENV_IS_NOWHERE
+ ENVL_NOWHERE,
+#endif
+};
+
+static enum env_location env_load_location = ENVL_UNKNOWN;
+
/**
* env_get_location() - Returns the best env location for a board
* @op: operations performed on the environment
@@ -45,36 +80,21 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
*/
static enum env_location env_get_location(enum env_operation op, int prio)
{
- /*
- * We support a single environment, so any environment asked
- * with a priority that is not zero is out of our supported
- * bounds.
- */
- if (prio >= 1)
- return ENVL_UNKNOWN;
-
- if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
- return ENVL_EEPROM;
- else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
- return ENVL_FAT;
- else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
- return ENVL_FLASH;
- else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
- return ENVL_MMC;
- else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
- return ENVL_NAND;
- else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
- return ENVL_NVRAM;
- else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
- return ENVL_REMOTE;
- else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
- return ENVL_SPI_FLASH;
- else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
- return ENVL_UBI;
- else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
- return ENVL_NOWHERE;
- else
- return ENVL_UNKNOWN;
+ switch (op) {
+ case ENVOP_GET_CHAR:
+ case ENVOP_INIT:
+ case ENVOP_LOAD:
+ if (prio >= ARRAY_SIZE(env_locations))
+ return ENVL_UNKNOWN;
+
+ env_load_location = env_locations[prio];
+ return env_load_location;
+
+ case ENVOP_SAVE:
+ return env_load_location;
+ }
+
+ return ENVL_UNKNOWN;
}
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-01-23 20:16 ` [U-Boot] [PATCH v3 09/15] env: Support multiple environments Maxime Ripard
@ 2018-01-30 8:19 ` Simon Goldschmidt
2018-01-30 19:39 ` York Sun
2018-02-06 8:09 ` Simon Goldschmidt
1 sibling, 1 reply; 21+ messages in thread
From: Simon Goldschmidt @ 2018-01-30 8:19 UTC (permalink / raw)
To: u-boot
On 23.01.2018 21:16, Maxime Ripard wrote:
> Now that we have everything in place to support multiple environment, let's
> make sure the current code can use it.
>
> The priority used between the various environment is the same one that was
> used in the code previously.
>
> At read / init times, the highest priority environment is going to be
> detected,
Does priority handling really work here? Most env drivers seem to ignore
the return value of env_import and may thus return success although
importing the environment failed (only reading the data from the device
succeeded).
This is from reading the code, I haven't had a chance to test this, yet.
> and we'll use the same one without lookup during writes. This
> should implement the same behaviour than we currently have.
Is there a way to save the environment to drivers other than the one
selected at init time?
Regards,
Simon
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> env/env.c | 80 +++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 50 insertions(+), 30 deletions(-)
>
> diff --git a/env/env.c b/env/env.c
> index 906f28ee50a1..1182fdb545db 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> return NULL;
> }
>
> +static enum env_location env_locations[] = {
> +#ifdef CONFIG_ENV_IS_IN_EEPROM
> + ENVL_EEPROM,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_FAT
> + ENVL_FAT,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_FLASH
> + ENVL_FLASH,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_MMC
> + ENVL_MMC,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_NAND
> + ENVL_NAND,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_NVRAM
> + ENVL_NVRAM,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_REMOTE
> + ENVL_REMOTE,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> + ENVL_SPI_FLASH,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_UBI
> + ENVL_UBI,
> +#endif
> +#ifdef CONFIG_ENV_IS_NOWHERE
> + ENVL_NOWHERE,
> +#endif
> +};
> +
> +static enum env_location env_load_location = ENVL_UNKNOWN;
> +
> /**
> * env_get_location() - Returns the best env location for a board
> * @op: operations performed on the environment
> @@ -45,36 +80,21 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> */
> static enum env_location env_get_location(enum env_operation op, int prio)
> {
> - /*
> - * We support a single environment, so any environment asked
> - * with a priority that is not zero is out of our supported
> - * bounds.
> - */
> - if (prio >= 1)
> - return ENVL_UNKNOWN;
> -
> - if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
> - return ENVL_EEPROM;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> - return ENVL_FAT;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
> - return ENVL_FLASH;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
> - return ENVL_MMC;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
> - return ENVL_NAND;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
> - return ENVL_NVRAM;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
> - return ENVL_REMOTE;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
> - return ENVL_SPI_FLASH;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
> - return ENVL_UBI;
> - else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
> - return ENVL_NOWHERE;
> - else
> - return ENVL_UNKNOWN;
> + switch (op) {
> + case ENVOP_GET_CHAR:
> + case ENVOP_INIT:
> + case ENVOP_LOAD:
> + if (prio >= ARRAY_SIZE(env_locations))
> + return ENVL_UNKNOWN;
> +
> + env_load_location = env_locations[prio];
> + return env_load_location;
> +
> + case ENVOP_SAVE:
> + return env_load_location;
> + }
> +
> + return ENVL_UNKNOWN;
> }
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-01-30 8:19 ` Simon Goldschmidt
@ 2018-01-30 19:39 ` York Sun
2018-01-30 20:16 ` York Sun
0 siblings, 1 reply; 21+ messages in thread
From: York Sun @ 2018-01-30 19:39 UTC (permalink / raw)
To: u-boot
On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
> On 23.01.2018 21:16, Maxime Ripard wrote:
>> Now that we have everything in place to support multiple environment, let's
>> make sure the current code can use it.
>>
>> The priority used between the various environment is the same one that was
>> used in the code previously.
>>
>> At read / init times, the highest priority environment is going to be
>> detected,
>
> Does priority handling really work here? Most env drivers seem to ignore
> the return value of env_import and may thus return success although
> importing the environment failed (only reading the data from the device
> succeeded).
>
> This is from reading the code, I haven't had a chance to test this, yet.
It is broken on my LS1043ARDB with simply NOR flash. I am trying to
determine what went wrong.
York
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-01-30 19:39 ` York Sun
@ 2018-01-30 20:16 ` York Sun
2018-01-30 23:02 ` York Sun
0 siblings, 1 reply; 21+ messages in thread
From: York Sun @ 2018-01-30 20:16 UTC (permalink / raw)
To: u-boot
On 01/30/2018 11:40 AM, York Sun wrote:
> On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
>> On 23.01.2018 21:16, Maxime Ripard wrote:
>>> Now that we have everything in place to support multiple environment, let's
>>> make sure the current code can use it.
>>>
>>> The priority used between the various environment is the same one that was
>>> used in the code previously.
>>>
>>> At read / init times, the highest priority environment is going to be
>>> detected,
>>
>> Does priority handling really work here? Most env drivers seem to ignore
>> the return value of env_import and may thus return success although
>> importing the environment failed (only reading the data from the device
>> succeeded).
>>
>> This is from reading the code, I haven't had a chance to test this, yet.
>
> It is broken on my LS1043ARDB with simply NOR flash. I am trying to
> determine what went wrong.
>
I found the problem. The variable "env_load_location" is static. It is
probably not write-able during booting from flash. It is expected to be
set during ENVOP_INIT. But if I print this variable, it has
ENVL_UNKNOWN. I can make it work by moving env_load_location to global
data structure.
That being said, this addition of multiple environments really slows
down the booting process for me. I see every time env_get_char() is
called, env_driver_lookup() runs. A simple call of env_get_f() gets
slowed down dramatically. I didn't find out where the most time is spent
yet.
Does anyone else experience this unbearable slowness?
York
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-01-30 20:16 ` York Sun
@ 2018-01-30 23:02 ` York Sun
2018-01-31 6:41 ` Simon Goldschmidt
2018-02-07 8:43 ` Maxime Ripard
0 siblings, 2 replies; 21+ messages in thread
From: York Sun @ 2018-01-30 23:02 UTC (permalink / raw)
To: u-boot
On 01/30/2018 12:16 PM, York Sun wrote:
> On 01/30/2018 11:40 AM, York Sun wrote:
>> On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
>>> On 23.01.2018 21:16, Maxime Ripard wrote:
>>>> Now that we have everything in place to support multiple environment, let's
>>>> make sure the current code can use it.
>>>>
>>>> The priority used between the various environment is the same one that was
>>>> used in the code previously.
>>>>
>>>> At read / init times, the highest priority environment is going to be
>>>> detected,
>>>
>>> Does priority handling really work here? Most env drivers seem to ignore
>>> the return value of env_import and may thus return success although
>>> importing the environment failed (only reading the data from the device
>>> succeeded).
>>>
>>> This is from reading the code, I haven't had a chance to test this, yet.
>>
>> It is broken on my LS1043ARDB with simply NOR flash. I am trying to
>> determine what went wrong.
>>
>
> I found the problem. The variable "env_load_location" is static. It is
> probably not write-able during booting from flash. It is expected to be
> set during ENVOP_INIT. But if I print this variable, it has
> ENVL_UNKNOWN. I can make it work by moving env_load_location to global
> data structure.
>
> That being said, this addition of multiple environments really slows
> down the booting process for me. I see every time env_get_char() is
> called, env_driver_lookup() runs. A simple call of env_get_f() gets
> slowed down dramatically. I didn't find out where the most time is spent
> yet.
>
> Does anyone else experience this unbearable slowness?
>
I found the problem. In patch #3 in this series, the default get_char()
was dropped so there is no driver for a plain NOR flash. A quick (and
maybe dirty) fix is this
diff --git a/env/env.c b/env/env.c
index edfb575..210bae2 100644
--- a/env/env.c
+++ b/env/env.c
@@ -159,7 +159,7 @@ int env_get_char(int index)
int ret;
if (!drv->get_char)
- continue;
+ return *(uchar *)(gd->env_addr + index);
if (!env_has_inited(drv->location))
continue;
With this temporary fix, my flash chip works again and I can boot all
the way up in a timely manner. I still don't like to call
env_driver_lookup() thousands of times to get a simple env variable.
Can Maxime post a quick post soon?
York
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-01-30 23:02 ` York Sun
@ 2018-01-31 6:41 ` Simon Goldschmidt
2018-02-07 8:43 ` Maxime Ripard
1 sibling, 0 replies; 21+ messages in thread
From: Simon Goldschmidt @ 2018-01-31 6:41 UTC (permalink / raw)
To: u-boot
On 31.01.2018 00:02, York Sun wrote:
> On 01/30/2018 12:16 PM, York Sun wrote:
>> On 01/30/2018 11:40 AM, York Sun wrote:
>>> On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
>>>> On 23.01.2018 21:16, Maxime Ripard wrote:
>>>>> Now that we have everything in place to support multiple environment, let's
>>>>> make sure the current code can use it.
>>>>>
>>>>> The priority used between the various environment is the same one that was
>>>>> used in the code previously.
>>>>>
>>>>> At read / init times, the highest priority environment is going to be
>>>>> detected,
>>>> Does priority handling really work here? Most env drivers seem to ignore
>>>> the return value of env_import and may thus return success although
>>>> importing the environment failed (only reading the data from the device
>>>> succeeded).
>>>>
>>>> This is from reading the code, I haven't had a chance to test this, yet.
>>> It is broken on my LS1043ARDB with simply NOR flash. I am trying to
>>> determine what went wrong.
>>>
>> I found the problem. The variable "env_load_location" is static. It is
>> probably not write-able during booting from flash. It is expected to be
>> set during ENVOP_INIT. But if I print this variable, it has
>> ENVL_UNKNOWN. I can make it work by moving env_load_location to global
>> data structure.
>>
>> That being said, this addition of multiple environments really slows
>> down the booting process for me. I see every time env_get_char() is
>> called, env_driver_lookup() runs. A simple call of env_get_f() gets
>> slowed down dramatically. I didn't find out where the most time is spent
>> yet.
>>
>> Does anyone else experience this unbearable slowness?
Yes. Depending on CPU speed... :-)
On my board, that slowdown comes from the fact that env_get_f does not
check the return value of env_get_char for an error. That leads to
trying for CONFIG_ENV_SIZE times, which is of course slow.
I'll post a fix for that.
> I found the problem. In patch #3 in this series, the default get_char()
> was dropped so there is no driver for a plain NOR flash. A quick (and
> maybe dirty) fix is this
That patch #3 actually changed the behavior for all env drivers not
providing .get_char (so all drivers except for eeprom and nvram) from
returning what's behind gd->env_addr. Your patch below restores the old
behaviour.
I can't tell what's the correct behaviour though: in my tests, env_get_f
got called even after importing the environment, which is not what I
would have expected...
> diff --git a/env/env.c b/env/env.c
> index edfb575..210bae2 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -159,7 +159,7 @@ int env_get_char(int index)
> int ret;
>
> if (!drv->get_char)
> - continue;
> + return *(uchar *)(gd->env_addr + index);
>
> if (!env_has_inited(drv->location))
> continue;
>
> With this temporary fix, my flash chip works again and I can boot all
> the way up in a timely manner. I still don't like to call
> env_driver_lookup() thousands of times to get a simple env variable.
That's not a thing Maxime has changed but a change that came in when
adding environment drivers.
Simon
>
> Can Maxime post a quick post soon?
>
> York
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-01-30 23:02 ` York Sun
2018-01-31 6:41 ` Simon Goldschmidt
@ 2018-02-07 8:43 ` Maxime Ripard
2018-02-07 20:18 ` York Sun
1 sibling, 1 reply; 21+ messages in thread
From: Maxime Ripard @ 2018-02-07 8:43 UTC (permalink / raw)
To: u-boot
Hi,
On Tue, Jan 30, 2018 at 11:02:49PM +0000, York Sun wrote:
> On 01/30/2018 12:16 PM, York Sun wrote:
> > On 01/30/2018 11:40 AM, York Sun wrote:
> >> On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
> >>> On 23.01.2018 21:16, Maxime Ripard wrote:
> >>>> Now that we have everything in place to support multiple environment, let's
> >>>> make sure the current code can use it.
> >>>>
> >>>> The priority used between the various environment is the same one that was
> >>>> used in the code previously.
> >>>>
> >>>> At read / init times, the highest priority environment is going to be
> >>>> detected,
> >>>
> >>> Does priority handling really work here? Most env drivers seem to ignore
> >>> the return value of env_import and may thus return success although
> >>> importing the environment failed (only reading the data from the device
> >>> succeeded).
> >>>
> >>> This is from reading the code, I haven't had a chance to test this, yet.
> >>
> >> It is broken on my LS1043ARDB with simply NOR flash. I am trying to
> >> determine what went wrong.
> >>
> >
> > I found the problem. The variable "env_load_location" is static. It is
> > probably not write-able during booting from flash. It is expected to be
> > set during ENVOP_INIT. But if I print this variable, it has
> > ENVL_UNKNOWN. I can make it work by moving env_load_location to global
> > data structure.
That would work for me.
> > That being said, this addition of multiple environments really slows
> > down the booting process for me. I see every time env_get_char() is
> > called, env_driver_lookup() runs. A simple call of env_get_f() gets
> > slowed down dramatically. I didn't find out where the most time is spent
> > yet.
> >
> > Does anyone else experience this unbearable slowness?
> >
>
> I found the problem. In patch #3 in this series, the default get_char()
> was dropped so there is no driver for a plain NOR flash. A quick (and
> maybe dirty) fix is this
>
>
> diff --git a/env/env.c b/env/env.c
> index edfb575..210bae2 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -159,7 +159,7 @@ int env_get_char(int index)
> int ret;
>
> if (!drv->get_char)
> - continue;
> + return *(uchar *)(gd->env_addr + index);
>
> if (!env_has_inited(drv->location))
> continue;
And this too.
> With this temporary fix, my flash chip works again and I can boot all
> the way up in a timely manner. I still don't like to call
> env_driver_lookup() thousands of times to get a simple env variable.
>
> Can Maxime post a quick post soon?
Given that you already made all the debugging, and the patches, and I
have no way to test, I guess it would make more sense if you did it :)
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/bcecb269/attachment.sig>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-07 8:43 ` Maxime Ripard
@ 2018-02-07 20:18 ` York Sun
0 siblings, 0 replies; 21+ messages in thread
From: York Sun @ 2018-02-07 20:18 UTC (permalink / raw)
To: u-boot
On 02/07/2018 12:43 AM, Maxime Ripard wrote:
> Hi,
>
> On Tue, Jan 30, 2018 at 11:02:49PM +0000, York Sun wrote:
>> On 01/30/2018 12:16 PM, York Sun wrote:
>>> On 01/30/2018 11:40 AM, York Sun wrote:
>>>> On 01/30/2018 12:19 AM, Simon Goldschmidt wrote:
>>>>> On 23.01.2018 21:16, Maxime Ripard wrote:
>>>>>> Now that we have everything in place to support multiple environment, let's
>>>>>> make sure the current code can use it.
>>>>>>
>>>>>> The priority used between the various environment is the same one that was
>>>>>> used in the code previously.
>>>>>>
>>>>>> At read / init times, the highest priority environment is going to be
>>>>>> detected,
>>>>>
>>>>> Does priority handling really work here? Most env drivers seem to ignore
>>>>> the return value of env_import and may thus return success although
>>>>> importing the environment failed (only reading the data from the device
>>>>> succeeded).
>>>>>
>>>>> This is from reading the code, I haven't had a chance to test this, yet.
>>>>
>>>> It is broken on my LS1043ARDB with simply NOR flash. I am trying to
>>>> determine what went wrong.
>>>>
>>>
>>> I found the problem. The variable "env_load_location" is static. It is
>>> probably not write-able during booting from flash. It is expected to be
>>> set during ENVOP_INIT. But if I print this variable, it has
>>> ENVL_UNKNOWN. I can make it work by moving env_load_location to global
>>> data structure.
>
> That would work for me.
>
>>> That being said, this addition of multiple environments really slows
>>> down the booting process for me. I see every time env_get_char() is
>>> called, env_driver_lookup() runs. A simple call of env_get_f() gets
>>> slowed down dramatically. I didn't find out where the most time is spent
>>> yet.
>>>
>>> Does anyone else experience this unbearable slowness?
>>>
>>
>> I found the problem. In patch #3 in this series, the default get_char()
>> was dropped so there is no driver for a plain NOR flash. A quick (and
>> maybe dirty) fix is this
>>
>>
>> diff --git a/env/env.c b/env/env.c
>> index edfb575..210bae2 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -159,7 +159,7 @@ int env_get_char(int index)
>> int ret;
>>
>> if (!drv->get_char)
>> - continue;
>> + return *(uchar *)(gd->env_addr + index);
>>
>> if (!env_has_inited(drv->location))
>> continue;
>
> And this too.
If you agree with this fix (actually revert your change earlier), I can
send out a patch.
>
>> With this temporary fix, my flash chip works again and I can boot all
>> the way up in a timely manner. I still don't like to call
>> env_driver_lookup() thousands of times to get a simple env variable.
>>
>> Can Maxime post a quick post soon?
>
> Given that you already made all the debugging, and the patches, and I
> have no way to test, I guess it would make more sense if you did it :)
Yes, I have tested on my boards. I will send out this patch.
York
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-01-23 20:16 ` [U-Boot] [PATCH v3 09/15] env: Support multiple environments Maxime Ripard
2018-01-30 8:19 ` Simon Goldschmidt
@ 2018-02-06 8:09 ` Simon Goldschmidt
2018-02-06 8:20 ` Joakim Tjernlund
2018-02-07 8:19 ` Maxime Ripard
1 sibling, 2 replies; 21+ messages in thread
From: Simon Goldschmidt @ 2018-02-06 8:09 UTC (permalink / raw)
To: u-boot
On 23.01.2018 21:16, Maxime Ripard wrote:
> Now that we have everything in place to support multiple environment, let's
> make sure the current code can use it.
I get more build errors testing this feature: there's a global variable
'env_ptr' declared in flash, nand, nvram and remote env drivers
(declared as extern in envrionment.h). From reading the code, it seems
like these could just be changed to static, since 'env_ptr' is not used
outside these drivers?
Simon
>
> The priority used between the various environment is the same one that was
> used in the code previously.
>
> At read / init times, the highest priority environment is going to be
> detected, and we'll use the same one without lookup during writes. This
> should implement the same behaviour than we currently have.
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> env/env.c | 80 +++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 50 insertions(+), 30 deletions(-)
>
> diff --git a/env/env.c b/env/env.c
> index 906f28ee50a1..1182fdb545db 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> return NULL;
> }
>
> +static enum env_location env_locations[] = {
> +#ifdef CONFIG_ENV_IS_IN_EEPROM
> + ENVL_EEPROM,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_FAT
> + ENVL_FAT,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_FLASH
> + ENVL_FLASH,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_MMC
> + ENVL_MMC,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_NAND
> + ENVL_NAND,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_NVRAM
> + ENVL_NVRAM,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_REMOTE
> + ENVL_REMOTE,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> + ENVL_SPI_FLASH,
> +#endif
> +#ifdef CONFIG_ENV_IS_IN_UBI
> + ENVL_UBI,
> +#endif
> +#ifdef CONFIG_ENV_IS_NOWHERE
> + ENVL_NOWHERE,
> +#endif
> +};
> +
> +static enum env_location env_load_location = ENVL_UNKNOWN;
> +
> /**
> * env_get_location() - Returns the best env location for a board
> * @op: operations performed on the environment
> @@ -45,36 +80,21 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> */
> static enum env_location env_get_location(enum env_operation op, int prio)
> {
> - /*
> - * We support a single environment, so any environment asked
> - * with a priority that is not zero is out of our supported
> - * bounds.
> - */
> - if (prio >= 1)
> - return ENVL_UNKNOWN;
> -
> - if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
> - return ENVL_EEPROM;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> - return ENVL_FAT;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
> - return ENVL_FLASH;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
> - return ENVL_MMC;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
> - return ENVL_NAND;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
> - return ENVL_NVRAM;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
> - return ENVL_REMOTE;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
> - return ENVL_SPI_FLASH;
> - else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
> - return ENVL_UBI;
> - else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
> - return ENVL_NOWHERE;
> - else
> - return ENVL_UNKNOWN;
> + switch (op) {
> + case ENVOP_GET_CHAR:
> + case ENVOP_INIT:
> + case ENVOP_LOAD:
> + if (prio >= ARRAY_SIZE(env_locations))
> + return ENVL_UNKNOWN;
> +
> + env_load_location = env_locations[prio];
> + return env_load_location;
> +
> + case ENVOP_SAVE:
> + return env_load_location;
> + }
> +
> + return ENVL_UNKNOWN;
> }
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-06 8:09 ` Simon Goldschmidt
@ 2018-02-06 8:20 ` Joakim Tjernlund
2018-02-07 8:18 ` Maxime Ripard
2018-02-07 8:32 ` Simon Goldschmidt
2018-02-07 8:19 ` Maxime Ripard
1 sibling, 2 replies; 21+ messages in thread
From: Joakim Tjernlund @ 2018-02-06 8:20 UTC (permalink / raw)
To: u-boot
On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:
.....
> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> > env/env.c | 80 +++++++++++++++++++++++++++++++++++---------------------
> > 1 file changed, 50 insertions(+), 30 deletions(-)
> >
> > diff --git a/env/env.c b/env/env.c
> > index 906f28ee50a1..1182fdb545db 100644
> > --- a/env/env.c
> > +++ b/env/env.c
> > @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> > return NULL;
> > }
> >
> > +static enum env_location env_locations[] = {
Don't use static/global variables. They cause a lot of relocation work/size
and is less flexible. There is no way to #define ENVL_EEPROM to a function
when a variable.
Jocke
> > +#ifdef CONFIG_ENV_IS_IN_EEPROM
> > + ENVL_EEPROM,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_FAT
> > + ENVL_FAT,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_FLASH
> > + ENVL_FLASH,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_MMC
> > + ENVL_MMC,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_NAND
> > + ENVL_NAND,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_NVRAM
> > + ENVL_NVRAM,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_REMOTE
> > + ENVL_REMOTE,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
> > + ENVL_SPI_FLASH,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_IN_UBI
> > + ENVL_UBI,
> > +#endif
> > +#ifdef CONFIG_ENV_IS_NOWHERE
> > + ENVL_NOWHERE,
> > +#endif
> > +};
> > +
> > +static enum env_location env_load_location = ENVL_UNKNOWN;
> > +
> > /**
> > * env_get_location() - Returns the best env location for a board
> > * @op: operations performed on the environment
> > @@ -45,36 +80,21 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> > */
> > static enum env_location env_get_location(enum env_operation op, int prio)
> > {
> > - /*
> > - * We support a single environment, so any environment asked
> > - * with a priority that is not zero is out of our supported
> > - * bounds.
> > - */
> > - if (prio >= 1)
> > - return ENVL_UNKNOWN;
> > -
> > - if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
> > - return ENVL_EEPROM;
> > - else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
> > - return ENVL_FAT;
> > - else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
> > - return ENVL_FLASH;
> > - else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
> > - return ENVL_MMC;
> > - else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
> > - return ENVL_NAND;
> > - else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
> > - return ENVL_NVRAM;
> > - else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
> > - return ENVL_REMOTE;
> > - else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
> > - return ENVL_SPI_FLASH;
> > - else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
> > - return ENVL_UBI;
> > - else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
> > - return ENVL_NOWHERE;
> > - else
> > - return ENVL_UNKNOWN;
> > + switch (op) {
> > + case ENVOP_GET_CHAR:
> > + case ENVOP_INIT:
> > + case ENVOP_LOAD:
> > + if (prio >= ARRAY_SIZE(env_locations))
> > + return ENVL_UNKNOWN;
> > +
> > + env_load_location = env_locations[prio];
> > + return env_load_location;
> > +
> > + case ENVOP_SAVE:
> > + return env_load_location;
> > + }
> > +
> > + return ENVL_UNKNOWN;
> > }
> >
> >
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-06 8:20 ` Joakim Tjernlund
@ 2018-02-07 8:18 ` Maxime Ripard
2018-02-07 8:34 ` Joakim Tjernlund
2018-02-07 8:32 ` Simon Goldschmidt
1 sibling, 1 reply; 21+ messages in thread
From: Maxime Ripard @ 2018-02-07 8:18 UTC (permalink / raw)
To: u-boot
Hi,
On Tue, Feb 06, 2018 at 08:20:49AM +0000, Joakim Tjernlund wrote:
> On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:
>
> .....
> > > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > ---
> > > env/env.c | 80 +++++++++++++++++++++++++++++++++++---------------------
> > > 1 file changed, 50 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/env/env.c b/env/env.c
> > > index 906f28ee50a1..1182fdb545db 100644
> > > --- a/env/env.c
> > > +++ b/env/env.c
> > > @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> > > return NULL;
> > > }
> > >
> > > +static enum env_location env_locations[] = {
>
> Don't use static/global variables. They cause a lot of relocation work/size
> and is less flexible. There is no way to #define ENVL_EEPROM to a function
> when a variable.
Is that last sentence truncated?
Can you elaborate a bit more on what is the source of the relocation
issues you're mentionning? Is that because of the section it ends up
in?
Thanks!
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/6f9f7ff2/attachment.sig>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-07 8:18 ` Maxime Ripard
@ 2018-02-07 8:34 ` Joakim Tjernlund
0 siblings, 0 replies; 21+ messages in thread
From: Joakim Tjernlund @ 2018-02-07 8:34 UTC (permalink / raw)
To: u-boot
On Thu, 1970-01-01 at 00:00 +0000, Maxime Ripard wrote:
> Hi,
>
> On Tue, Feb 06, 2018 at 08:20:49AM +0000, Joakim Tjernlund wrote:
> > On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:
> >
> > .....
> > > > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > ---
> > > > env/env.c | 80 +++++++++++++++++++++++++++++++++++---------------------
> > > > 1 file changed, 50 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/env/env.c b/env/env.c
> > > > index 906f28ee50a1..1182fdb545db 100644
> > > > --- a/env/env.c
> > > > +++ b/env/env.c
> > > > @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> > > > return NULL;
> > > > }
> > > >
> > > > +static enum env_location env_locations[] = {
> >
> > Don't use static/global variables. They cause a lot of relocation work/size
> > and is less flexible. There is no way to #define ENVL_EEPROM to a function
> > when a variable.
>
> Is that last sentence truncated?
>
> Can you elaborate a bit more on what is the source of the relocation
> issues you're mentionning? Is that because of the section it ends up
> in?
Mainly that its adds relocation entries that take up space, more space than doing
a simple assignment directly in code.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-06 8:20 ` Joakim Tjernlund
2018-02-07 8:18 ` Maxime Ripard
@ 2018-02-07 8:32 ` Simon Goldschmidt
2018-02-07 8:45 ` Joakim Tjernlund
1 sibling, 1 reply; 21+ messages in thread
From: Simon Goldschmidt @ 2018-02-07 8:32 UTC (permalink / raw)
To: u-boot
On 06.02.2018 09:20, Joakim Tjernlund wrote:
> On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:
>
> .....
>>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>> ---
>>> env/env.c | 80 +++++++++++++++++++++++++++++++++++---------------------
>>> 1 file changed, 50 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/env/env.c b/env/env.c
>>> index 906f28ee50a1..1182fdb545db 100644
>>> --- a/env/env.c
>>> +++ b/env/env.c
>>> @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
>>> return NULL;
>>> }
>>>
>>> +static enum env_location env_locations[] = {
> Don't use static/global variables. They cause a lot of relocation work/size
> and is less flexible.
In this specific case, I think this array should be const anyway, would
that prevent the relocation problems you see?
> There is no way to #define ENVL_EEPROM to a function
> when a variable.
ENVL_EEPROM is an enum value, why would you define it to a function?
Simon
>
> Jocke
>
>>> +#ifdef CONFIG_ENV_IS_IN_EEPROM
>>> + ENVL_EEPROM,
>>> +#endif
>>> +#ifdef CONFIG_ENV_IS_IN_FAT
>>> + ENVL_FAT,
>>> +#endif
>>> +#ifdef CONFIG_ENV_IS_IN_FLASH
>>> + ENVL_FLASH,
>>> +#endif
>>> +#ifdef CONFIG_ENV_IS_IN_MMC
>>> + ENVL_MMC,
>>> +#endif
>>> +#ifdef CONFIG_ENV_IS_IN_NAND
>>> + ENVL_NAND,
>>> +#endif
>>> +#ifdef CONFIG_ENV_IS_IN_NVRAM
>>> + ENVL_NVRAM,
>>> +#endif
>>> +#ifdef CONFIG_ENV_IS_IN_REMOTE
>>> + ENVL_REMOTE,
>>> +#endif
>>> +#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
>>> + ENVL_SPI_FLASH,
>>> +#endif
>>> +#ifdef CONFIG_ENV_IS_IN_UBI
>>> + ENVL_UBI,
>>> +#endif
>>> +#ifdef CONFIG_ENV_IS_NOWHERE
>>> + ENVL_NOWHERE,
>>> +#endif
>>> +};
>>> +
>>> +static enum env_location env_load_location = ENVL_UNKNOWN;
>>> +
>>> /**
>>> * env_get_location() - Returns the best env location for a board
>>> * @op: operations performed on the environment
>>> @@ -45,36 +80,21 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
>>> */
>>> static enum env_location env_get_location(enum env_operation op, int prio)
>>> {
>>> - /*
>>> - * We support a single environment, so any environment asked
>>> - * with a priority that is not zero is out of our supported
>>> - * bounds.
>>> - */
>>> - if (prio >= 1)
>>> - return ENVL_UNKNOWN;
>>> -
>>> - if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
>>> - return ENVL_EEPROM;
>>> - else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
>>> - return ENVL_FAT;
>>> - else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
>>> - return ENVL_FLASH;
>>> - else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
>>> - return ENVL_MMC;
>>> - else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
>>> - return ENVL_NAND;
>>> - else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
>>> - return ENVL_NVRAM;
>>> - else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
>>> - return ENVL_REMOTE;
>>> - else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
>>> - return ENVL_SPI_FLASH;
>>> - else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
>>> - return ENVL_UBI;
>>> - else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
>>> - return ENVL_NOWHERE;
>>> - else
>>> - return ENVL_UNKNOWN;
>>> + switch (op) {
>>> + case ENVOP_GET_CHAR:
>>> + case ENVOP_INIT:
>>> + case ENVOP_LOAD:
>>> + if (prio >= ARRAY_SIZE(env_locations))
>>> + return ENVL_UNKNOWN;
>>> +
>>> + env_load_location = env_locations[prio];
>>> + return env_load_location;
>>> +
>>> + case ENVOP_SAVE:
>>> + return env_load_location;
>>> + }
>>> +
>>> + return ENVL_UNKNOWN;
>>> }
>>>
>>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-07 8:32 ` Simon Goldschmidt
@ 2018-02-07 8:45 ` Joakim Tjernlund
2018-02-07 18:23 ` Maxime Ripard
0 siblings, 1 reply; 21+ messages in thread
From: Joakim Tjernlund @ 2018-02-07 8:45 UTC (permalink / raw)
To: u-boot
On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> On 06.02.2018 09:20, Joakim Tjernlund wrote:
> > On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:
> >
> > .....
> > > > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > ---
> > > > env/env.c | 80 +++++++++++++++++++++++++++++++++++---------------------
> > > > 1 file changed, 50 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/env/env.c b/env/env.c
> > > > index 906f28ee50a1..1182fdb545db 100644
> > > > --- a/env/env.c
> > > > +++ b/env/env.c
> > > > @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> > > > return NULL;
> > > > }
> > > >
> > > > +static enum env_location env_locations[] = {
> >
> > Don't use static/global variables. They cause a lot of relocation work/size
> > and is less flexible.
>
> In this specific case, I think this array should be const anyway, would
> that prevent the relocation problems you see?
>
> > There is no way to #define ENVL_EEPROM to a function
> > when a variable.
>
> ENVL_EEPROM is an enum value, why would you define it to a function?
I got boards that very similar but differ in where/how env. is stored, like different
flash so I need to be able to select at runtime how get my env., I haven't
looked if this particular area is affected but ideally I would like if all
env. related "constants" could be impl. with a function instead.
Also, using static/global vars takes more space than simple assignments in code, ideally
everything needed early (before reloc/ in SPL) should avoid relocations to save space.
Jocke
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-07 8:45 ` Joakim Tjernlund
@ 2018-02-07 18:23 ` Maxime Ripard
0 siblings, 0 replies; 21+ messages in thread
From: Maxime Ripard @ 2018-02-07 18:23 UTC (permalink / raw)
To: u-boot
On Wed, Feb 07, 2018 at 08:45:46AM +0000, Joakim Tjernlund wrote:
> On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> >
> >
> > On 06.02.2018 09:20, Joakim Tjernlund wrote:
> > > On Thu, 1970-01-01 at 00:00 +0000, Simon Goldschmidt wrote:
> > >
> > > .....
> > > > > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > > ---
> > > > > env/env.c | 80 +++++++++++++++++++++++++++++++++++---------------------
> > > > > 1 file changed, 50 insertions(+), 30 deletions(-)
> > > > >
> > > > > diff --git a/env/env.c b/env/env.c
> > > > > index 906f28ee50a1..1182fdb545db 100644
> > > > > --- a/env/env.c
> > > > > +++ b/env/env.c
> > > > > @@ -26,6 +26,41 @@ static struct env_driver *_env_driver_lookup(enum env_location loc)
> > > > > return NULL;
> > > > > }
> > > > >
> > > > > +static enum env_location env_locations[] = {
> > >
> > > Don't use static/global variables. They cause a lot of relocation work/size
> > > and is less flexible.
> >
> > In this specific case, I think this array should be const anyway, would
> > that prevent the relocation problems you see?
>
> >
> > > There is no way to #define ENVL_EEPROM to a function
> > > when a variable.
> >
> > ENVL_EEPROM is an enum value, why would you define it to a function?
>
> I got boards that very similar but differ in where/how env. is
> stored, like different flash so I need to be able to select at
> runtime how get my env., I haven't looked if this particular area is
> affected but ideally I would like if all env. related "constants"
> could be impl. with a function instead.
It's exactly the point of this entire serie though, and why we merged
it.
You just need to override env_get_location in your board, and return
the preferred location.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/ab92d14d/attachment.sig>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-06 8:09 ` Simon Goldschmidt
2018-02-06 8:20 ` Joakim Tjernlund
@ 2018-02-07 8:19 ` Maxime Ripard
2018-02-07 8:25 ` Simon Goldschmidt
1 sibling, 1 reply; 21+ messages in thread
From: Maxime Ripard @ 2018-02-07 8:19 UTC (permalink / raw)
To: u-boot
On Tue, Feb 06, 2018 at 09:09:07AM +0100, Simon Goldschmidt wrote:
> On 23.01.2018 21:16, Maxime Ripard wrote:
> > Now that we have everything in place to support multiple environment, let's
> > make sure the current code can use it.
>
> I get more build errors testing this feature: there's a global variable
> 'env_ptr' declared in flash, nand, nvram and remote env drivers (declared as
> extern in envrionment.h). From reading the code, it seems like these could
> just be changed to static, since 'env_ptr' is not used outside these
> drivers?
Given Joakim's comment, I guess we should keep them !static, rename
them to $env_env_ptr, and remove the definition in the
include/environment that doesn't seem used anywhere.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/e071e98b/attachment.sig>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-07 8:19 ` Maxime Ripard
@ 2018-02-07 8:25 ` Simon Goldschmidt
2018-02-07 18:48 ` Maxime Ripard
0 siblings, 1 reply; 21+ messages in thread
From: Simon Goldschmidt @ 2018-02-07 8:25 UTC (permalink / raw)
To: u-boot
On 07.02.2018 09:19, Maxime Ripard wrote:
> On Tue, Feb 06, 2018 at 09:09:07AM +0100, Simon Goldschmidt wrote:
>> On 23.01.2018 21:16, Maxime Ripard wrote:
>>> Now that we have everything in place to support multiple environment, let's
>>> make sure the current code can use it.
>> I get more build errors testing this feature: there's a global variable
>> 'env_ptr' declared in flash, nand, nvram and remote env drivers (declared as
>> extern in envrionment.h). From reading the code, it seems like these could
>> just be changed to static, since 'env_ptr' is not used outside these
>> drivers?
> Given Joakim's comment, I guess we should keep them !static, rename
> them to $env_env_ptr, and remove the definition in the
> include/environment that doesn't seem used anywhere.
That's OK for me, I just wanted to point out the build error.
However, I do think that having unnecessary non-static global variables
is not really good coding style as you risk name clashes. I'd really be
interested in a reason for this.
Simon
^ permalink raw reply [flat|nested] 21+ messages in thread
* [U-Boot] [PATCH v3 09/15] env: Support multiple environments
2018-02-07 8:25 ` Simon Goldschmidt
@ 2018-02-07 18:48 ` Maxime Ripard
0 siblings, 0 replies; 21+ messages in thread
From: Maxime Ripard @ 2018-02-07 18:48 UTC (permalink / raw)
To: u-boot
1;5002;0c
On Wed, Feb 07, 2018 at 09:25:55AM +0100, Simon Goldschmidt wrote:
> On 07.02.2018 09:19, Maxime Ripard wrote:
> > On Tue, Feb 06, 2018 at 09:09:07AM +0100, Simon Goldschmidt wrote:
> > > On 23.01.2018 21:16, Maxime Ripard wrote:
> > > > Now that we have everything in place to support multiple environment, let's
> > > > make sure the current code can use it.
> > > I get more build errors testing this feature: there's a global variable
> > > 'env_ptr' declared in flash, nand, nvram and remote env drivers (declared as
> > > extern in envrionment.h). From reading the code, it seems like these could
> > > just be changed to static, since 'env_ptr' is not used outside these
> > > drivers?
> > Given Joakim's comment, I guess we should keep them !static, rename
> > them to $env_env_ptr, and remove the definition in the
> > include/environment that doesn't seem used anywhere.
>
> That's OK for me, I just wanted to point out the build error.
>
> However, I do think that having unnecessary non-static global variables is
> not really good coding style as you risk name clashes. I'd really be
> interested in a reason for this.
If the relocation works with a static variable, I'm all for it.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180207/a44d255e/attachment.sig>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2018-02-08 5:30 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-07 20:45 [U-Boot] [PATCH v3 09/15] env: Support multiple environments Goldschmidt Simon
2018-02-07 21:18 ` York Sun
2018-02-08 5:30 ` Simon Goldschmidt
-- strict thread matches above, loose matches on Subject: below --
2018-01-23 20:16 [U-Boot] [PATCH v3 00/15] env: Multiple env support and env transition for sunxi Maxime Ripard
2018-01-23 20:16 ` [U-Boot] [PATCH v3 09/15] env: Support multiple environments Maxime Ripard
2018-01-30 8:19 ` Simon Goldschmidt
2018-01-30 19:39 ` York Sun
2018-01-30 20:16 ` York Sun
2018-01-30 23:02 ` York Sun
2018-01-31 6:41 ` Simon Goldschmidt
2018-02-07 8:43 ` Maxime Ripard
2018-02-07 20:18 ` York Sun
2018-02-06 8:09 ` Simon Goldschmidt
2018-02-06 8:20 ` Joakim Tjernlund
2018-02-07 8:18 ` Maxime Ripard
2018-02-07 8:34 ` Joakim Tjernlund
2018-02-07 8:32 ` Simon Goldschmidt
2018-02-07 8:45 ` Joakim Tjernlund
2018-02-07 18:23 ` Maxime Ripard
2018-02-07 8:19 ` Maxime Ripard
2018-02-07 8:25 ` Simon Goldschmidt
2018-02-07 18:48 ` Maxime Ripard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.