Wolfgang, Albert, Sandeep,
Luca Ceresoli wrote:
+#ifndef __ASSEMBLY__ +extern unsigned int boot_flash_base; +extern unsigned int boot_flash_off; +extern unsigned int boot_flash_sec; +extern unsigned int boot_flash_type; +#endif
Can we please get rid of this?
Hopefully. This mostly depends on how able I will be in understanding the meaning of this stuff and how it should be done instead. I might need support.
I knew this would have been painful.
Here are my considerations about "those variables". They are a bit lengthy, but I think they deserve a little time to understand the issue and choose the best way out.
Observations:
a. arch/arm/cpu/armv7/omap3/mem.c computes a value for those variables based on config-values in the config file (see gpmc_init(), bottom half):
#if defined(CONFIG_CMD_NAND) /* CS 0 */ ... base = PISMO1_NAND_BASE; size = PISMO1_NAND_SIZE; ... #if defined(CONFIG_ENV_IS_IN_NAND) f_off = SMNAND_ENV_OFFSET; f_sec = (128<< 10); /* 128 KiB */ /* env setup */ boot_flash_base = base; boot_flash_off = f_off; boot_flash_sec = f_sec; boot_flash_env_addr = f_off; #endif #endif
#if defined(CONFIG_CMD_ONENAND) ... base = PISMO1_ONEN_BASE; size = PISMO1_ONEN_SIZE; ... #if defined(CONFIG_ENV_IS_IN_ONENAND) f_off = ONENAND_ENV_OFFSET; f_sec = (128<< 10); /* 128 KiB */ /* env setup */ boot_flash_base = base; boot_flash_off = f_off; boot_flash_sec = f_sec; boot_flash_env_addr = f_off; #endif #endif
b. this computation is trivial and based only on constants; c. omap3 config files define some config-values based on those variables; e.g. omap3_beagle.h does: #define CONFIG_ENV_OFFSET boot_flash_off d. common/env_nand.c (and maybe other common files?) use the config-values such as CONFIG_ENV_OFFSET to do their work.
Analysis:
If those variable declarations were moved to some common place, I think it should be mem.h (as they are computed in mem.c), or some other omap3-specific include file.
Problem 1: how can a file in common/ (such as env_nand.c) see variables declared in an omap3-specific include file? The whole thing is currently working because they are declared (extern) in the config file, which is included by common/ files.
If those variables were declared in mem.h, it would be necessary to include <asm/arch/mem.h> in env_nand.c, which would break compilation for architectures where mem.h does not exist (which are the majority).
Problem 2: points a. and c. clearly show a depencency loop. mem.c computes values needed for the config, but it needs itself the config. In fact the computation itself is a function of config-values, and produces config-values as a result.
This definitely has to be moved out of mem.c, which has to live on top of the config file and not mess with it.
The point is, where to place these computations?
Solution(s):
I propose 2 solutions to solve both problems. They are both based on the fact that these computations are based only on constants, and thus they can live entirely in the *_config.h file without ever declaring any variable.
Solution 1: Since they are based on constants only, if there were properly folded the computations could sit *duplicated* in all the config files taking only a few lines, like this:
#if defined(CONFIG_CMD_NAND)&& defined(CONFIG_ENV_IS_IN_NAND) #define CONFIG_SYS_FLASH_BASE PISMO1_NAND_BASE #define CONFIG_SYS_ENV_SECT_SIZE (128<< 10) #define CONFIG_ENV_OFFSET SMNAND_ENV_OFFSET #endif
#if defined(CONFIG_CMD_ONENAND)&& defined(CONFIG_ENV_IS_IN_ONENAND) ... similar ... #endif
Solution 2: If, on the other hand, it is desirable to have the computation clearly visible and not duplicated, it could be done in an auxiliary .h file, to be included *in the middle* of config files:
#define CONFIG_CMD_NAND #define CONFIG_ENV_IS_IN_NAND ... #include<omap3_compute_flash_values.h> /* any better name? */ /* now CONFIG_SYS_FLASH_BASE and friends are defined
Wolfgang, Albert, Sandeep, which solution would best fit the U-Boot style?
I personally slightly prefer the former, as most boards do actually support only NAND or ONENAND but not both, so the lines to add would not be so many (probably 4 after alla the cleanups). Moreover, the .h file to be included in the middle is a trick that makes code less readable.
Anyway, whichever you suggest to be the best will be in the v4 patch series.
Luca