[U-Boot] MMC boot broken on OMAP-L138 LCDK

[U-Boot] MMC boot broken on OMAP-L138 LCDK

[Sekhar Nori]

Hi Adam,

MMC/SD boot is broken on OMAP-L138 LCDK. This is since v2018.11. 
v2018.09 is fine.

The first breakage occurred with commit 21af33ed0319 ("ARM: davinci: 
omapl138_lcdk: Enable DM_MMC"). This commit moved to DM_MMC for U-Boot 
while keeping legacy mode for SPL.

The "#ifndef CONFIG_DM_MMC" introduced by this commit is incorrect I 
think because CONFIG_DM_MMC is defined in SPL build too and because of 
this board_mmc_init() never gets defined. I think the intent was to 
define board_mmc_init() for SPL case, and for that the following diff 
should do:

diff --git a/board/davinci/da8xxevm/omapl138_lcdk.c b/board/davinci/da8xxevm/omapl138_lcdk.c
index 2c2f885d43e4..fe1bf4410145 100644
--- a/board/davinci/da8xxevm/omapl138_lcdk.c
+++ b/board/davinci/da8xxevm/omapl138_lcdk.c
@@ -353,7 +353,7 @@ int misc_init_r(void)
 	return 0;
 }
 
-#ifndef CONFIG_DM_MMC
+#if !CONFIG_IS_ENABLED(DM_MMC)
 #ifdef CONFIG_MMC_DAVINCI
 static struct davinci_mmc mmc_sd0 = {
 	.reg_base = (struct davinci_mmc_regs *)DAVINCI_MMC_SD0_BASE,


With this fixed, MMC/SD boot is still broken after commit 15b8c7505819 
("davinci: da850evm/omapl138-lcdk: Move BSS to SDRAM because SRAM is 
full").

After this commit, mmc_initialize() in drivers/mmc/mmc.c fails because 
the static variable 'initialized' does not get initialized to 0 as 
intended in code and gets set to a random value because of which no 
MMC/SD device gets registered. I did notice that 'initialized' is in 
BSS, so I believe after this commit BSS is not getting set to 0.

Thanks,
Sekhar

[U-Boot] MMC boot broken on OMAP-L138 LCDK

[Adam Ford]

On Fri, May 17, 2019 at 7:36 AM Sekhar Nori <nsekhar@ti.com> wrote:
>
> Hi Adam,
>
> MMC/SD boot is broken on OMAP-L138 LCDK. This is since v2018.11.
> v2018.09 is fine.
>
> The first breakage occurred with commit 21af33ed0319 ("ARM: davinci:
> omapl138_lcdk: Enable DM_MMC"). This commit moved to DM_MMC for U-Boot
> while keeping legacy mode for SPL.

I am going to preface this by stating I don't have the LCDK.  Peter
Howard (added in CC) asked me to submit this on his behalf, so I
reviewed his patch and submitted it which is why both our names are on
the s-o-b.

>
> The "#ifndef CONFIG_DM_MMC" introduced by this commit is incorrect I
> think because CONFIG_DM_MMC is defined in SPL build too and because of
> this board_mmc_init() never gets defined. I think the intent was to
> define board_mmc_init() for SPL case, and for that the following diff
> should do:
>
> diff --git a/board/davinci/da8xxevm/omapl138_lcdk.c b/board/davinci/da8xxevm/omapl138_lcdk.c
> index 2c2f885d43e4..fe1bf4410145 100644
> --- a/board/davinci/da8xxevm/omapl138_lcdk.c
> +++ b/board/davinci/da8xxevm/omapl138_lcdk.c
> @@ -353,7 +353,7 @@ int misc_init_r(void)
>         return 0;
>  }
>
> -#ifndef CONFIG_DM_MMC
> +#if !CONFIG_IS_ENABLED(DM_MMC)
>  #ifdef CONFIG_MMC_DAVINCI
>  static struct davinci_mmc mmc_sd0 = {
>         .reg_base = (struct davinci_mmc_regs *)DAVINCI_MMC_SD0_BASE,
>

I would agree that the #ifndef should be replaced with if
!CONFIG_IS_ENABLED(DM_MMC), but I would recomend checking into
migrating the LCDK to DM. and DM_MMC.
>
> With this fixed, MMC/SD boot is still broken after commit 15b8c7505819
> ("davinci: da850evm/omapl138-lcdk: Move BSS to SDRAM because SRAM is
> full").
>
> After this commit, mmc_initialize() in drivers/mmc/mmc.c fails because
> the static variable 'initialized' does not get initialized to 0 as
> intended in code and gets set to a random value because of which no
> MMC/SD device gets registered. I did notice that 'initialized' is in
> BSS, so I believe after this commit BSS is not getting set to 0.

I sent Peter H a list of patches to consider because I spent a lot of
time trying to modernize the da850_evm using DM, and DM_SPL and
eventually SPL_OF_CONTROL which ultimately eliminated a lot of the
manual initializing of the various drivers.

See:
commit a4670f8ebb5b4df6afeb5155fb5b44c1d1d154b9 Enable DM and device
tree support for da850-evm
commit c4fa049a121457ff38b74daac32e18d7afbd4538 shrunk SPL by 6.5k
commit 391328dc30b78be6f0d1181088eda0fd7febd5f3 removed DM_I2C_COMPAT
commit cb19c29398cb84e72236ab6bae3763028fce5d44 board: da8xxevm: Add
SPL DM for serial, spi (with platdata)
commit 591353d011b5ff6dbc8511b7a839cddf5b610495 ARM: da850evm:
Increase Malloc Size
commit 15b8c7505819fa48dd99fb51e91b9536f341fde1 moved BSS to SDRAM
because SRAM is full
commit f7c1d53605d9ec528abacda9ba1763c67221fc88 ARM: davinci:
da850evm: Enable SPL_OF_CONTROL without PLATDATA
commit fd3c26f3f921ad7addf95857cdb99a883c6e497a ARM: da850evm: Fix
broken SPI Flash

Having said that, the hardware I have for the da850 EVM doesn't
support MMC booting, so I cannot verify whether or not MMC initializes
properly in SPL.  I know the MMC initializes in U-Boot once it's
booted from SPI flash.

If someone wants to send me an LCDK, I'd be happy to look at
attempting to modernize the U-Boot for it.  Otherwise, we may want to
consider splitting the linker scripts between  LCDK and da850 evm so
da850evm can use SPL_OF_CONTROL with enough memory and LCDK can keep
it's legacy code status in SPL.

adam
>
> Thanks,
> Sekhar

[U-Boot] MMC boot broken on OMAP-L138 LCDK

[Sekhar Nori]

On 17/05/19 6:34 PM, Adam Ford wrote:
> On Fri, May 17, 2019 at 7:36 AM Sekhar Nori <nsekhar@ti.com> wrote:
>>
>> Hi Adam,
>>
>> MMC/SD boot is broken on OMAP-L138 LCDK. This is since v2018.11.
>> v2018.09 is fine.
>>
>> The first breakage occurred with commit 21af33ed0319 ("ARM: davinci:
>> omapl138_lcdk: Enable DM_MMC"). This commit moved to DM_MMC for U-Boot
>> while keeping legacy mode for SPL.
> 
> I am going to preface this by stating I don't have the LCDK.  Peter
> Howard (added in CC) asked me to submit this on his behalf, so I
> reviewed his patch and submitted it which is why both our names are on
> the s-o-b.
> 
>>
>> The "#ifndef CONFIG_DM_MMC" introduced by this commit is incorrect I
>> think because CONFIG_DM_MMC is defined in SPL build too and because of
>> this board_mmc_init() never gets defined. I think the intent was to
>> define board_mmc_init() for SPL case, and for that the following diff
>> should do:
>>
>> diff --git a/board/davinci/da8xxevm/omapl138_lcdk.c b/board/davinci/da8xxevm/omapl138_lcdk.c
>> index 2c2f885d43e4..fe1bf4410145 100644
>> --- a/board/davinci/da8xxevm/omapl138_lcdk.c
>> +++ b/board/davinci/da8xxevm/omapl138_lcdk.c
>> @@ -353,7 +353,7 @@ int misc_init_r(void)
>>         return 0;
>>  }
>>
>> -#ifndef CONFIG_DM_MMC
>> +#if !CONFIG_IS_ENABLED(DM_MMC)
>>  #ifdef CONFIG_MMC_DAVINCI
>>  static struct davinci_mmc mmc_sd0 = {
>>         .reg_base = (struct davinci_mmc_regs *)DAVINCI_MMC_SD0_BASE,
>>
> 
> I would agree that the #ifndef should be replaced with if
> !CONFIG_IS_ENABLED(DM_MMC), but I would recomend checking into
> migrating the LCDK to DM. and DM_MMC.

Okay. For now, I will submit a fix changing the #ifndef.

>> With this fixed, MMC/SD boot is still broken after commit 15b8c7505819
>> ("davinci: da850evm/omapl138-lcdk: Move BSS to SDRAM because SRAM is
>> full").
>>
>> After this commit, mmc_initialize() in drivers/mmc/mmc.c fails because
>> the static variable 'initialized' does not get initialized to 0 as
>> intended in code and gets set to a random value because of which no
>> MMC/SD device gets registered. I did notice that 'initialized' is in
>> BSS, so I believe after this commit BSS is not getting set to 0.
> 
> I sent Peter H a list of patches to consider because I spent a lot of
> time trying to modernize the da850_evm using DM, and DM_SPL and
> eventually SPL_OF_CONTROL which ultimately eliminated a lot of the
> manual initializing of the various drivers.
> 
> See:
> commit a4670f8ebb5b4df6afeb5155fb5b44c1d1d154b9 Enable DM and device
> tree support for da850-evm
> commit c4fa049a121457ff38b74daac32e18d7afbd4538 shrunk SPL by 6.5k
> commit 391328dc30b78be6f0d1181088eda0fd7febd5f3 removed DM_I2C_COMPAT
> commit cb19c29398cb84e72236ab6bae3763028fce5d44 board: da8xxevm: Add
> SPL DM for serial, spi (with platdata)
> commit 591353d011b5ff6dbc8511b7a839cddf5b610495 ARM: da850evm:
> Increase Malloc Size
> commit 15b8c7505819fa48dd99fb51e91b9536f341fde1 moved BSS to SDRAM
> because SRAM is full
> commit f7c1d53605d9ec528abacda9ba1763c67221fc88 ARM: davinci:
> da850evm: Enable SPL_OF_CONTROL without PLATDATA
> commit fd3c26f3f921ad7addf95857cdb99a883c6e497a ARM: da850evm: Fix
> broken SPI Flash
> 
> Having said that, the hardware I have for the da850 EVM doesn't
> support MMC booting, so I cannot verify whether or not MMC initializes
> properly in SPL.  I know the MMC initializes in U-Boot once it's
> booted from SPI flash.

MMC/SD boot can be simulated on any board by hacking spl_boot_device()
in arch/arm/mach-davinci/spl.c

> 
> If someone wants to send me an LCDK, I'd be happy to look at
> attempting to modernize the U-Boot for it.  Otherwise, we may want to

I will try to get you a board.

> consider splitting the linker scripts between  LCDK and da850 evm so
> da850evm can use SPL_OF_CONTROL with enough memory and LCDK can keep
> it's legacy code status in SPL.

I think this is not an LCDK specific problem. Looking at
arch/arm/lib/crt0.S, BSS is cleared before board_init_r() is called.

In case of DA850, DDR is initialized in
board_init_r()->spl_board_init()->arch_cpu_init()->da850_ddr_setup()

So, after you moved BSS to use SDRAM, the BSS clearing code was working
on uninitialized DDR. I think the right fix may be to call
arch_cpu_init() in board_init_f()?

Thanks,
Sekhar

[U-Boot] MMC boot broken on OMAP-L138 LCDK

[Adam Ford]

On Mon, May 20, 2019 at 4:09 AM Sekhar Nori <nsekhar@ti.com> wrote:
>
> On 17/05/19 6:34 PM, Adam Ford wrote:
> > On Fri, May 17, 2019 at 7:36 AM Sekhar Nori <nsekhar@ti.com> wrote:
> >>
> >> Hi Adam,
> >>
> >> MMC/SD boot is broken on OMAP-L138 LCDK. This is since v2018.11.
> >> v2018.09 is fine.
> >>
> >> The first breakage occurred with commit 21af33ed0319 ("ARM: davinci:
> >> omapl138_lcdk: Enable DM_MMC"). This commit moved to DM_MMC for U-Boot
> >> while keeping legacy mode for SPL.
> >
> > I am going to preface this by stating I don't have the LCDK.  Peter
> > Howard (added in CC) asked me to submit this on his behalf, so I
> > reviewed his patch and submitted it which is why both our names are on
> > the s-o-b.
> >
> >>
> >> The "#ifndef CONFIG_DM_MMC" introduced by this commit is incorrect I
> >> think because CONFIG_DM_MMC is defined in SPL build too and because of
> >> this board_mmc_init() never gets defined. I think the intent was to
> >> define board_mmc_init() for SPL case, and for that the following diff
> >> should do:
> >>
> >> diff --git a/board/davinci/da8xxevm/omapl138_lcdk.c b/board/davinci/da8xxevm/omapl138_lcdk.c
> >> index 2c2f885d43e4..fe1bf4410145 100644
> >> --- a/board/davinci/da8xxevm/omapl138_lcdk.c
> >> +++ b/board/davinci/da8xxevm/omapl138_lcdk.c
> >> @@ -353,7 +353,7 @@ int misc_init_r(void)
> >>         return 0;
> >>  }
> >>
> >> -#ifndef CONFIG_DM_MMC
> >> +#if !CONFIG_IS_ENABLED(DM_MMC)
> >>  #ifdef CONFIG_MMC_DAVINCI
> >>  static struct davinci_mmc mmc_sd0 = {
> >>         .reg_base = (struct davinci_mmc_regs *)DAVINCI_MMC_SD0_BASE,
> >>
> >
> > I would agree that the #ifndef should be replaced with if
> > !CONFIG_IS_ENABLED(DM_MMC), but I would recomend checking into
> > migrating the LCDK to DM. and DM_MMC.
>
> Okay. For now, I will submit a fix changing the #ifndef.
>
> >> With this fixed, MMC/SD boot is still broken after commit 15b8c7505819
> >> ("davinci: da850evm/omapl138-lcdk: Move BSS to SDRAM because SRAM is
> >> full").
> >>
> >> After this commit, mmc_initialize() in drivers/mmc/mmc.c fails because
> >> the static variable 'initialized' does not get initialized to 0 as
> >> intended in code and gets set to a random value because of which no
> >> MMC/SD device gets registered. I did notice that 'initialized' is in
> >> BSS, so I believe after this commit BSS is not getting set to 0.
> >
> > I sent Peter H a list of patches to consider because I spent a lot of
> > time trying to modernize the da850_evm using DM, and DM_SPL and
> > eventually SPL_OF_CONTROL which ultimately eliminated a lot of the
> > manual initializing of the various drivers.
> >
> > See:
> > commit a4670f8ebb5b4df6afeb5155fb5b44c1d1d154b9 Enable DM and device
> > tree support for da850-evm
> > commit c4fa049a121457ff38b74daac32e18d7afbd4538 shrunk SPL by 6.5k
> > commit 391328dc30b78be6f0d1181088eda0fd7febd5f3 removed DM_I2C_COMPAT
> > commit cb19c29398cb84e72236ab6bae3763028fce5d44 board: da8xxevm: Add
> > SPL DM for serial, spi (with platdata)
> > commit 591353d011b5ff6dbc8511b7a839cddf5b610495 ARM: da850evm:
> > Increase Malloc Size
> > commit 15b8c7505819fa48dd99fb51e91b9536f341fde1 moved BSS to SDRAM
> > because SRAM is full
> > commit f7c1d53605d9ec528abacda9ba1763c67221fc88 ARM: davinci:
> > da850evm: Enable SPL_OF_CONTROL without PLATDATA
> > commit fd3c26f3f921ad7addf95857cdb99a883c6e497a ARM: da850evm: Fix
> > broken SPI Flash
> >
> > Having said that, the hardware I have for the da850 EVM doesn't
> > support MMC booting, so I cannot verify whether or not MMC initializes
> > properly in SPL.  I know the MMC initializes in U-Boot once it's
> > booted from SPI flash.
>
> MMC/SD boot can be simulated on any board by hacking spl_boot_device()
> in arch/arm/mach-davinci/spl.c

That's a good idea.  I never thought of that.  I'll have to add the
mmc entry to the device tree and enable MMC in SPL, but I'll try to
run some tests this week.  I'm out of town Friday-Tuesday, so if I'll
try to get to it by Wednesday.

>
> >
> > If someone wants to send me an LCDK, I'd be happy to look at
> > attempting to modernize the U-Boot for it.  Otherwise, we may want to
>
> I will try to get you a board.

thanks.

>
> > consider splitting the linker scripts between  LCDK and da850 evm so
> > da850evm can use SPL_OF_CONTROL with enough memory and LCDK can keep
> > it's legacy code status in SPL.
>
> I think this is not an LCDK specific problem. Looking at
> arch/arm/lib/crt0.S, BSS is cleared before board_init_r() is called.
>
> In case of DA850, DDR is initialized in
> board_init_r()->spl_board_init()->arch_cpu_init()->da850_ddr_setup()
>
> So, after you moved BSS to use SDRAM, the BSS clearing code was working
> on uninitialized DDR. I think the right fix may be to call
> arch_cpu_init() in board_init_f()?

What about adding a memset function to the end of spl_board_init() so
after arch_cpu_init() is called,and after preloader_consol_init() is
called, we clear BSS?  I ran some preliminary tests on da850evm and it
didn't see to break anything.  There are a few boards that appear to
do things in this order.

adam
>
> Thanks,
> Sekhar

[U-Boot] MMC boot broken on OMAP-L138 LCDK

[Sekhar Nori]

On 20/05/19 5:55 PM, Adam Ford wrote:
> What about adding a memset function to the end of spl_board_init() so
> after arch_cpu_init() is called,and after preloader_consol_init() is
> called, we clear BSS?  I ran some preliminary tests on da850evm and it
> didn't see to break anything.  There are a few boards that appear to
> do things in this order.

This will work, but, looks like there is a desire to maintain some
"standard" around this. Ie board_init_r() can assume BSS is available
(see README -> Board Initialisation Flow:).

Doing BSS init in in spl_board_init() called from board_init_r() will
violate that.

Thanks,
Sekhar

[U-Boot] MMC boot broken on OMAP-L138 LCDK

[Adam Ford]

On Mon, May 20, 2019 at 7:38 AM Sekhar Nori <nsekhar@ti.com> wrote:
>
> On 20/05/19 5:55 PM, Adam Ford wrote:
> > What about adding a memset function to the end of spl_board_init() so
> > after arch_cpu_init() is called,and after preloader_consol_init() is
> > called, we clear BSS?  I ran some preliminary tests on da850evm and it
> > didn't see to break anything.  There are a few boards that appear to
> > do things in this order.
>
> This will work, but, looks like there is a desire to maintain some
> "standard" around this. Ie board_init_r() can assume BSS is available
> (see README -> Board Initialisation Flow:).
>
> Doing BSS init in in spl_board_init() called from board_init_r() will
> violate that.

Based on your previous comment about board_init_f and your comment
above, I read through the readme and I agree with your assessment that
the current setup appears to already violate the startup flow.

Looking at board_f.c, there is a board_init_f which calls
arch_cpu_init(), so I thought I might try to remove the call from
spl_board_init.  This didn't work.  I then I attempted to rename
spl_board_init to board_init_f to move these function calls earlier in
the startup sequence. I also undefined SPL_BOARD_INIT from menuconfig,
so the call to spl_board_init() wouldn't happen. Unfortunately, now
the da850 doesn't start with that either.  If you have any
suggestions, I'm willing to try them.

adam
>
> Thanks,
> Sekhar
>
>

[U-Boot] MMC boot broken on OMAP-L138 LCDK

[Adam Ford]

On Mon, May 20, 2019 at 10:00 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, May 20, 2019 at 7:38 AM Sekhar Nori <nsekhar@ti.com> wrote:
> >
> > On 20/05/19 5:55 PM, Adam Ford wrote:
> > > What about adding a memset function to the end of spl_board_init() so
> > > after arch_cpu_init() is called,and after preloader_consol_init() is
> > > called, we clear BSS?  I ran some preliminary tests on da850evm and it
> > > didn't see to break anything.  There are a few boards that appear to
> > > do things in this order.
> >
> > This will work, but, looks like there is a desire to maintain some
> > "standard" around this. Ie board_init_r() can assume BSS is available
> > (see README -> Board Initialisation Flow:).
> >
> > Doing BSS init in in spl_board_init() called from board_init_r() will
> > violate that.
>
> Based on your previous comment about board_init_f and your comment
> above, I read through the readme and I agree with your assessment that
> the current setup appears to already violate the startup flow.
>
> Looking at board_f.c, there is a board_init_f which calls
> arch_cpu_init(), so I thought I might try to remove the call from
> spl_board_init.  This didn't work.  I then I attempted to rename
> spl_board_init to board_init_f to move these function calls earlier in
> the startup sequence. I also undefined SPL_BOARD_INIT from menuconfig,
> so the call to spl_board_init() wouldn't happen. Unfortunately, now
> the da850 doesn't start with that either.  If you have any
> suggestions, I'm willing to try them.

I just submitted a patch which moves the initialization to
board_init_f and clears BSS inside there.  I was only able to test it
on the da850evm.  I am trying to get the da850 to boot from MMC, but
it's not something I've done before, so it's taking a bit of time.  It
does appear to be recognizing whether or not the card initializes, but
I am not sure why it's not run u-boot.img yet.

With card installed:

U-Boot SPL 2019.07-rc2-00579-gc17c82f15a-dirty (May 20 2019 - 14:48:11 -0500)
>>SPL: board_init_r()
Trying to boot from MMC1
SPL: failed to boot from all boot devices
### ERROR ### Please RESET the board ###

Without card installed:

U-Boot SPL 2019.07-rc2-00579-gc17c82f15a-dirty (May 20 2019 - 14:48:11 -0500)
>>SPL: board_init_r()
Trying to boot from MMC1
Card did not respond to voltage select!
spl: mmc init failed with error: -95
SPL: failed to boot from all boot devices
### ERROR ### Please RESET the board ###

I am at least making some progress.

adam
>
> adam
> >
> > Thanks,
> > Sekhar
> >
> >