[U-Boot] __mmc_get_env_addr() is not being called?

[U-Boot] __mmc_get_env_addr() is not being called?

[Shawn Guo]

Hi,

I'm running v2011.12-rc1 (with one missing usdhc patch applied) on
mx6qarm2 board, and seeing a problem.  When I install u-boot on a
blank SD card, it boots fine with messge "*** Warning - bad CRC,
using default environment" seen as expected.  But once I run command
'save' to write envs to the card, the card stop booting.

I tracked the issue a little bit, and found __mmc_get_env_addr() does
not get called at all.  Therefore when I run 'save' for the first time,
the envs are written to offset 0 of the card, which in turn overwrites
the boot image.

I have not figured out why it does.  But the following change fixes
the problem for me.

diff --git a/common/env_mmc.c b/common/env_mmc.c
index 8441c77..3832fe4 100644
--- a/common/env_mmc.c
+++ b/common/env_mmc.c
@@ -46,13 +46,11 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_ENV_OFFSET 0
 #endif

-static int __mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
+static int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
 {
        *env_addr = CONFIG_ENV_OFFSET;
        return 0;
 }
-int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
-       __attribute__((weak, alias("__mmc_get_env_addr")));

Any comment on why the change fixes the problem is appreciated.

-- 
Regards,
Shawn

[U-Boot] __mmc_get_env_addr() is not being called?

[Stefano Babic]

On 16/12/2011 06:54, Shawn Guo wrote:
> Hi,
> 

Hi Shawn,

> I'm running v2011.12-rc1 (with one missing usdhc patch applied) on
> mx6qarm2 board, and seeing a problem.  When I install u-boot on a
> blank SD card, it boots fine with messge "*** Warning - bad CRC,
> using default environment" seen as expected.  But once I run command
> 'save' to write envs to the card, the card stop booting.
> 
> I tracked the issue a little bit, and found __mmc_get_env_addr() does
> not get called at all.  Therefore when I run 'save' for the first time,
> the envs are written to offset 0 of the card, which in turn overwrites
> the boot image.
> 
> I have not figured out why it does.  But the following change fixes
> the problem for me.
> 
> diff --git a/common/env_mmc.c b/common/env_mmc.c
> index 8441c77..3832fe4 100644
> --- a/common/env_mmc.c
> +++ b/common/env_mmc.c
> @@ -46,13 +46,11 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define CONFIG_ENV_OFFSET 0
>  #endif
> 
> -static int __mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
> +static int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
>  {
>         *env_addr = CONFIG_ENV_OFFSET;
>         return 0;
>  }
> -int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
> -       __attribute__((weak, alias("__mmc_get_env_addr")));
> 
> Any comment on why the change fixes the problem is appreciated.

The function is declared weak, this means that a specific architecture
could implement its own version. The reason why it does not work is that
there is a specific implementation in board/freescale/sdhc_boot.c, and
the mmc_get_env_addr() in this file is taken instead of the default when
CONFIG_ENV_IS_IN_MMC (as in our case) is set. Instead of your proposal,
we should surround mmc_get_env_addr() in sdhc_boot.c with something like
#ifndef CONFIG_MX6Q, or check if we can reuse this function, that really
does not use CONFIG_ENV_OFFSET at all.

I put Jason (board maintainer) in CC - Jason, what do mind ? Do you get
the same issue when you tested the board ?

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] __mmc_get_env_addr() is not being called?

[Fabio Estevam]

On Fri, Dec 16, 2011 at 3:54 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> Hi,
>
> I'm running v2011.12-rc1 (with one missing usdhc patch applied) on
> mx6qarm2 board, and seeing a problem. ?When I install u-boot on a
> blank SD card, it boots fine with messge "*** Warning - bad CRC,
> using default environment" seen as expected. ?But once I run command
> 'save' to write envs to the card, the card stop booting.

This is the same behavior I saw on mx28evk as well and I sent this
patch to fix it:
http://lists.denx.de/pipermail/u-boot/2011-November/111448.html

>
> I tracked the issue a little bit, and found __mmc_get_env_addr() does
> not get called at all. ?Therefore when I run 'save' for the first time,
> the envs are written to offset 0 of the card, which in turn overwrites
> the boot image.
>
> I have not figured out why it does. ?But the following change fixes
> the problem for me.
>
> diff --git a/common/env_mmc.c b/common/env_mmc.c
> index 8441c77..3832fe4 100644
> --- a/common/env_mmc.c
> +++ b/common/env_mmc.c
> @@ -46,13 +46,11 @@ DECLARE_GLOBAL_DATA_PTR;
> ?#define CONFIG_ENV_OFFSET 0
> ?#endif
>
> -static int __mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
> +static int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
> ?{
> ? ? ? ?*env_addr = CONFIG_ENV_OFFSET;
> ? ? ? ?return 0;
> ?}
> -int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
> - ? ? ? __attribute__((weak, alias("__mmc_get_env_addr")));
>
> Any comment on why the change fixes the problem is appreciated.

The weak function was introduced by this commit:
http://git.denx.de/?p=u-boot.git;a=commitdiff;h=97039ab98c551c7860bc0977d684ef686159e0d7

which breaks non CONFIG_FSL_ESDHC users.

Regards,

Fabio Estevam

[U-Boot] __mmc_get_env_addr() is not being called?

[Stefano Babic]

On 16/12/2011 10:33, Fabio Estevam wrote:

> 
> The weak function was introduced by this commit:
> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=97039ab98c551c7860bc0977d684ef686159e0d7
> 
> which breaks non CONFIG_FSL_ESDHC users.

Ok, I see - the patch is more related to Freescale SOCs as to the MMC,
and it is sometimes unknown who whould pick it up.

Andy, not CONFIG_FSL_ESDHC means IMX users - if you do not complain, I
merge it into u-boot-imx.

Thanks,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] __mmc_get_env_addr() is not being called?

[Kumar Gala]

On Dec 16, 2011, at 3:42 AM, Stefano Babic wrote:

> On 16/12/2011 10:33, Fabio Estevam wrote:
> 
>> 
>> The weak function was introduced by this commit:
>> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=97039ab98c551c7860bc0977d684ef686159e0d7
>> 
>> which breaks non CONFIG_FSL_ESDHC users.
> 
> Ok, I see - the patch is more related to Freescale SOCs as to the MMC,
> and it is sometimes unknown who whould pick it up.
> 
> Andy, not CONFIG_FSL_ESDHC means IMX users - if you do not complain, I
> merge it into u-boot-imx.
> 
> Thanks,
> Stefano

If I'm reading the patch correct it will break things (or I'm not sure which patch you're referring to).  How would the PPC specific version ever get called?

- k

[U-Boot] __mmc_get_env_addr() is not being called?

[Jason Liu]

2011/12/16 Stefano Babic <sbabic@denx.de>:
> On 16/12/2011 06:54, Shawn Guo wrote:
>> Hi,
>>
>
> Hi Shawn,
>
>> I'm running v2011.12-rc1 (with one missing usdhc patch applied) on
>> mx6qarm2 board, and seeing a problem. ?When I install u-boot on a
>> blank SD card, it boots fine with messge "*** Warning - bad CRC,
>> using default environment" seen as expected. ?But once I run command
>> 'save' to write envs to the card, the card stop booting.
>>
>> I tracked the issue a little bit, and found __mmc_get_env_addr() does
>> not get called at all. ?Therefore when I run 'save' for the first time,
>> the envs are written to offset 0 of the card, which in turn overwrites
>> the boot image.
>>
>> I have not figured out why it does. ?But the following change fixes
>> the problem for me.
>>
>> diff --git a/common/env_mmc.c b/common/env_mmc.c
>> index 8441c77..3832fe4 100644
>> --- a/common/env_mmc.c
>> +++ b/common/env_mmc.c
>> @@ -46,13 +46,11 @@ DECLARE_GLOBAL_DATA_PTR;
>> ?#define CONFIG_ENV_OFFSET 0
>> ?#endif
>>
>> -static int __mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
>> +static int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
>> ?{
>> ? ? ? ? *env_addr = CONFIG_ENV_OFFSET;
>> ? ? ? ? return 0;
>> ?}
>> -int mmc_get_env_addr(struct mmc *mmc, u32 *env_addr)
>> - ? ? ? __attribute__((weak, alias("__mmc_get_env_addr")));
>>
>> Any comment on why the change fixes the problem is appreciated.
>
> The function is declared weak, this means that a specific architecture
> could implement its own version. The reason why it does not work is that
> there is a specific implementation in board/freescale/sdhc_boot.c, and
> the mmc_get_env_addr() in this file is taken instead of the default when
> CONFIG_ENV_IS_IN_MMC (as in our case) is set. Instead of your proposal,
> we should surround mmc_get_env_addr() in sdhc_boot.c with something like
> #ifndef CONFIG_MX6Q, or check if we can reuse this function, that really
> does not use CONFIG_ENV_OFFSET at all.
>
> I put Jason (board maintainer) in CC - Jason, what do mind ? Do you get
> the same issue when you tested the board ?

Stefano, I saw the issue.
Fabio's patch can fix the i.mx28evk issue, but can't fix the following
platforms:

11    144  include/configs/efikamx.h <<CONFIG_FSL_ESDHC>>
             #define CONFIG_FSL_ESDHC
  12     86  include/configs/mx51evk.h <<CONFIG_FSL_ESDHC>>
             #define CONFIG_FSL_ESDHC
  13     58  include/configs/mx53ard.h <<CONFIG_FSL_ESDHC>>
             #define CONFIG_FSL_ESDHC
  14     68  include/configs/mx53evk.h <<CONFIG_FSL_ESDHC>>
             #define CONFIG_FSL_ESDHC
  15     51  include/configs/mx53loco.h <<CONFIG_FSL_ESDHC>>
             #define CONFIG_FSL_ESDHC
  16     58  include/configs/mx53smd.h <<CONFIG_FSL_ESDHC>>
             #define CONFIG_FSL_ESDHC
  17     48  include/configs/mx6qarm2.h <<CONFIG_FSL_ESDHC>>
             #define CONFIG_FSL_ESDHC
  18     48  include/configs/mx6qsabrelite.h <<CONFIG_FSL_ESDHC>>
             #define CONFIG_FSL_ESDHC
  20    105  include/configs/vision2.h <<CONFIG_FSL_ESDHC>>
             #define CONFIG_FSL_ESDHC

The commit  97039ab98 (env_mmc: Allow board code to override the
environment address)
break the i.mx5/6 boards support.

 BR,
Jason

>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 ?Email: office at denx.de
> =====================================================================
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] __mmc_get_env_addr() is not being called?

[Stefano Babic]

On 18/12/2011 18:56, Kumar Gala wrote:
> 
> On Dec 16, 2011, at 3:42 AM, Stefano Babic wrote:
> 
>> On 16/12/2011 10:33, Fabio Estevam wrote:
>>
>>>
>>> The weak function was introduced by this commit:
>>> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=97039ab98c551c7860bc0977d684ef686159e0d7
>>>
>>> which breaks non CONFIG_FSL_ESDHC users.
>>
>> Ok, I see - the patch is more related to Freescale SOCs as to the MMC,
>> and it is sometimes unknown who whould pick it up.
>>
>> Andy, not CONFIG_FSL_ESDHC means IMX users - if you do not complain, I
>> merge it into u-boot-imx.
>>
>> Thanks,
>> Stefano
> 
> If I'm reading the patch correct it will break things (or I'm not sure which patch you're referring to).  How would the PPC specific version ever get called?

Yes, I was too optimistic with the patch, I have myself seen that the
patch then break PowerPC boards.

Reading sdhc_boot.c, this file is not strictly related to PowerPC or IMX
hardware, but where u-boot image is stored on PowerPC code.

This is like a specific feature, but there is not a specific CONFIG_ for
it. In fact, in board/freescale/common/Makefile the file is always
compiled if CONFIG_RAMBOOT_PBL (not significant for imx) is not set if
the environment is in MMC:

ifndef CONFIG_RAMBOOT_PBL
COBJS-$(CONFIG_ENV_IS_IN_MMC)   += sdhc_boot.o
endif

What about to have a specific CONFIG_FSL_* for this file ? It can be
defined for PowerPC boards, and it will not for IMX. Any thoughts ?

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] __mmc_get_env_addr() is not being called?

[Jason Hui]

On Mon, Dec 19, 2011 at 2:37 PM, Stefano Babic <sbabic@denx.de> wrote:
> On 18/12/2011 18:56, Kumar Gala wrote:
>>
>> On Dec 16, 2011, at 3:42 AM, Stefano Babic wrote:
>>
>>> On 16/12/2011 10:33, Fabio Estevam wrote:
>>>
>>>>
>>>> The weak function was introduced by this commit:
>>>> http://git.denx.de/?p=u-boot.git;a=commitdiff;h=97039ab98c551c7860bc0977d684ef686159e0d7
>>>>
>>>> which breaks non CONFIG_FSL_ESDHC users.
>>>
>>> Ok, I see - the patch is more related to Freescale SOCs as to the MMC,
>>> and it is sometimes unknown who whould pick it up.
>>>
>>> Andy, not CONFIG_FSL_ESDHC means IMX users - if you do not complain, I
>>> merge it into u-boot-imx.
>>>
>>> Thanks,
>>> Stefano
>>
>> If I'm reading the patch correct it will break things (or I'm not sure which patch you're referring to). ?How would the PPC specific version ever get called?
>
> Yes, I was too optimistic with the patch, I have myself seen that the
> patch then break PowerPC boards.
>
> Reading sdhc_boot.c, this file is not strictly related to PowerPC or IMX
> hardware, but where u-boot image is stored on PowerPC code.
>
> This is like a specific feature, but there is not a specific CONFIG_ for
> it. In fact, in board/freescale/common/Makefile the file is always
> compiled if CONFIG_RAMBOOT_PBL (not significant for imx) is not set if
> the environment is in MMC:
>
> ifndef CONFIG_RAMBOOT_PBL
> COBJS-$(CONFIG_ENV_IS_IN_MMC) ? += sdhc_boot.o
> endif
>
> What about to have a specific CONFIG_FSL_* for this file ? It can be
> defined for PowerPC boards, and it will not for IMX. Any thoughts ?

I agree with this option. Also Cced: Chang-Ming.Huang at freescale.com

>
> Stefano
>
> --
> =====================================================================
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 ?Email: office at denx.de
> =====================================================================