[U-Boot] [PATCH v3 4/4] ARMV7: OMAP3: Add support for Comelit DIG297 board

[Luca Ceresoli]

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