Hi Marek, Sorry for rate reply.
On 2021/05/25 16:35, Marek Vasut wrote:
On 5/12/21 4:09 PM, Kunihiko Hayashi wrote:
When CONFIG_ENV_IS_NOWHERE is enabled, env_nowhere_init() sets ENV_INVALID to gd->env_valid, and sets default_environment before relocation to gd->env_addr. After that, env_init() switches gd->env_valid to ENV_VALID by the previous fix.
If gd->env_valid is ENV_INVALID, env_get_char() returns relocated default_environment, however, env_get_char() returns gd->env_addr before relocation since gd->env_valid is ENV_VALID, and access to gd->env_addr will cause a fault.
This leaves gd->env_valid as ENV_INVALID for "nowhere" location.
So do I understand this correctly that _after_ relocation, env_init() is called and env_init() does not update gd->env_addr to the relocated one?
In my understandings, env_init() belongs to init_sequence_f[] and env_init() is called before relocation.
I would expect that after relocation, if all you have is env_nowhere driver, the env_nowhere_init() is called again from the first for() loop of env_init() [1], which would set gd->env_valid to ENV_INVALID [1], and that for() loop would exit with ret = -ENOENT [2], so then the last part of env_init() would check for ret == -ENOENT and update gd->env_addr to relocated default_environment [3].
324 int env_init(void) 325 { 326 struct env_driver *drv; 327 int ret = -ENOENT; 328 int prio; 329 330 for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) { /* Part [1] */ 331 if (!drv->init || !(ret = drv->init())) 332 env_set_inited(drv->location); 333 if (ret == -ENOENT) 334 env_set_inited(drv->location); 335 336 debug("%s: Environment %s init done (ret=%d)\n", __func__, 337 drv->name, ret); 338 /* Part [2] */ 339 if (gd->env_valid == ENV_INVALID) 340 ret = -ENOENT; 341 } 342 343 if (!prio) 344 return -ENODEV; 345 /* Part [3] */ 346 if (ret == -ENOENT) { /* This should be relocated default_environment address */ 347 gd->env_addr = (ulong)&default_environment[0]; 348 gd->env_valid = ENV_VALID; 349 350 return 0; 351 } 352 353 return ret; 354 }
Or am I missing something obvious ?
These are called before relocation, and update gd->env_addr to non-relocated default_environment by [3].
After that, gd->env_addr is relocated in initr_reloc_global_data() if CONFIG_SYS_RELOC_GD_ENV_ADDR is defined.
| #ifdef CONFIG_SYS_RELOC_GD_ENV_ADDR | /* | * Relocate the early env_addr pointer unless we know it is not inside | * the binary. Some systems need this and for the rest, it doesn't hurt. | */ | gd->env_addr += gd->reloc_off; | #endif
However, I misunderstood my situation. gd->reloc_off doesn't have the proper value because CONFIG_SYS_TEXT_BASE is zero due to CONFIG_POSITION_INDENENDENT=y.
gd->reloc_off is calculated with CONFIG_SYS_TEXT_BASE in setup_reloc().
| gd->reloc_off = gd->relocaddr - CONFIG_SYS_TEXT_BASE;
gd->env_addr is added with gd->reloc_off (== gd->relocaddr - 0), as a result, gd->env_addr has wrong address.
In this case, I think the proper solution is to undefine CONFIG_SYS_RELOC_GD_ENV_ADDR.
My patch isn't necessary no longer and your patch also works with "nowhere".
Thank you,
--- Best Regards Kunihiko Hayashi