[U-Boot,v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it

Message ID 1518443671-11417-1-git-send-email-faiz_abbas@ti.com
State Accepted
Commit 26862b4a40c31b59618f7776ca06f0ed453cc380
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot,v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it
Related show

Commit Message

Faiz Abbas Feb. 12, 2018, 1:54 p.m.
When booting from a non-MMC device, the MMC sub-system may not be
initialized when the environment is first accessed.
We need to make sure that the MMC sub-system is ready in even a non-MMC
boot case.

Therefore, initialize mmc before loading environment from it.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
Dropped Lukasz's Reviewed-by because patch has
changed.

I have tested this with ENV_IS_IN_FAT and ENV_IS_IN_MMC.

 env/ext4.c | 3 +++
 env/fat.c  | 3 +++
 env/mmc.c  | 2 ++
 3 files changed, 8 insertions(+)

Comments

Faiz Abbas Feb. 19, 2018, 5:32 a.m. | #1
Hi,

On Monday 12 February 2018 07:24 PM, Faiz Abbas wrote:
> When booting from a non-MMC device, the MMC sub-system may not be
> initialized when the environment is first accessed.
> We need to make sure that the MMC sub-system is ready in even a non-MMC
> boot case.
> 
> Therefore, initialize mmc before loading environment from it.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
> Dropped Lukasz's Reviewed-by because patch has
> changed.
> 
> I have tested this with ENV_IS_IN_FAT and ENV_IS_IN_MMC.
> 
>  env/ext4.c | 3 +++
>  env/fat.c  | 3 +++
>  env/mmc.c  | 2 ++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/env/ext4.c b/env/ext4.c
> index 3f3aac5..6c69a0a 100644
> --- a/env/ext4.c
> +++ b/env/ext4.c
> @@ -87,6 +87,9 @@ static int env_ext4_load(void)
>  	int err;
>  	loff_t off;
>  
> +	if (!strcmp(CONFIG_ENV_EXT4_INTERFACE, "mmc"))
> +		mmc_initialize(NULL);
> +
>  	part = blk_get_device_part_str(CONFIG_ENV_EXT4_INTERFACE,
>  				       CONFIG_ENV_EXT4_DEVICE_AND_PART,
>  				       &dev_desc, &info, 1);
> diff --git a/env/fat.c b/env/fat.c
> index 35f7ab5..fdf4b7a 100644
> --- a/env/fat.c
> +++ b/env/fat.c
> @@ -89,6 +89,9 @@ static int env_fat_load(void)
>  	int dev, part;
>  	int err;
>  
> +	if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc"))
> +		mmc_initialize(NULL);
> +
>  	part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
>  					CONFIG_ENV_FAT_DEVICE_AND_PART,
>  					&dev_desc, &info, 1);
> diff --git a/env/mmc.c b/env/mmc.c
> index 1058b8c..6f11dec 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -273,6 +273,8 @@ static int env_mmc_load(void)
>  	ALLOC_CACHE_ALIGN_BUFFER(env_t, tmp_env1, 1);
>  	ALLOC_CACHE_ALIGN_BUFFER(env_t, tmp_env2, 1);
>  
> +	mmc_initialize(NULL);
> +
>  	mmc = find_mmc_device(dev);
>  
>  	errmsg = init_mmc_for_env(mmc);
> 

Gentle ping.

Thanks,
Faiz
Tom Rini Feb. 20, 2018, 10:03 p.m. | #2
On Mon, Feb 12, 2018 at 07:24:31PM +0530, Faiz Abbas wrote:

> When booting from a non-MMC device, the MMC sub-system may not be
> initialized when the environment is first accessed.
> We need to make sure that the MMC sub-system is ready in even a non-MMC
> boot case.
> 
> Therefore, initialize mmc before loading environment from it.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>

Applied to u-boot/master, thanks!
Wolfgang Denk Feb. 24, 2018, 8:58 p.m. | #3
Dear Tom,

In message <20180220220328.GC4311@bill-the-cat> you wrote:
> 
> > We need to make sure that the MMC sub-system is ready in even a non-MMC
> > boot case.
> > 
> > Therefore, initialize mmc before loading environment from it.
> > 
> > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>
> Applied to u-boot/master, thanks!

I had sent a (gentle) NAK for this patch before (Message-id:
<20180207085212.387EB2406CC@gemini.denx.de> Date: Wed, 07 Feb 2018
09:52:12 +0100).

It is wrong to initialize any sub-system just in case it might be
needed. Quote from my message:

In this case I hereby NAK this patch.

U-Boot shall always use lazy initialization and never ever initialize
hardware which it does not really use - see especially item 2 at
[1].

[1] http://www.denx.de/wiki/U-Boot/DesignPrinciples



Why do you ignore this NAK, and why do you add this patch so late in
the release cycle anyway?

Best regards,

Wolfgang Denk
Tom Rini Feb. 24, 2018, 9:53 p.m. | #4
On Sat, Feb 24, 2018 at 09:58:45PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20180220220328.GC4311@bill-the-cat> you wrote:
> > 
> > > We need to make sure that the MMC sub-system is ready in even a non-MMC
> > > boot case.
> > > 
> > > Therefore, initialize mmc before loading environment from it.
> > > 
> > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> >
> > Applied to u-boot/master, thanks!
> 
> I had sent a (gentle) NAK for this patch before (Message-id:
> <20180207085212.387EB2406CC@gemini.denx.de> Date: Wed, 07 Feb 2018
> 09:52:12 +0100).
> 
> It is wrong to initialize any sub-system just in case it might be
> needed. Quote from my message:
> 
> In this case I hereby NAK this patch.
> 
> U-Boot shall always use lazy initialization and never ever initialize
> hardware which it does not really use - see especially item 2 at
> [1].
> 
> [1] http://www.denx.de/wiki/U-Boot/DesignPrinciples
> 
> 
> 
> Why do you ignore this NAK, and why do you add this patch so late in
> the release cycle anyway?

Sorry, didn't v2 address your concerns?  We don't initialize the device
because maybe we'll have env there, we initalize mmc because we need to
check that it is there.  Thanks!
Wolfgang Denk Feb. 25, 2018, 8:53 a.m. | #5
Dear Tom Rini,

In message <20180224215325.GQ4311@bill-the-cat> you wrote:
> 
> > Why do you ignore this NAK, and why do you add this patch so late in
> > the release cycle anyway?
> 
> Sorry, didn't v2 address your concerns?  We don't initialize the device
> because maybe we'll have env there, we initalize mmc because we need to
> check that it is there.  Thanks!

No, it does not.  As is, we initialize MMC in the EXT4 code (in
env_ext4_load()). If we go that route, we would have to add similar
init calls to all other file systemn load routines as well.

This does not make sense to me.  This pollutes the code with
unrelated things - file system code should not depend on any
underlaying driver suport.  It increases code size, makes the code
harder to maintain (if you need to change this, there is good
chances to forget the same change in other file systems), and there
is a good chance that in the end you will initialzie MMC even if you
don't use it.

We should keep the code clean and orthogonal.  Driver init code has
no place in file system code.

If needed, the drivers have to make sure to auto--initialize on
first access.

I hold my NAK on this patch.  It is the wrong thing to do.

Best regards,

Wolfgang Denk
Tom Rini Feb. 25, 2018, 1:48 p.m. | #6
On Sun, Feb 25, 2018 at 09:53:10AM +0100, Wolfgang Denk wrote:
> Dear Tom Rini,
> 
> In message <20180224215325.GQ4311@bill-the-cat> you wrote:
> > 
> > > Why do you ignore this NAK, and why do you add this patch so late in
> > > the release cycle anyway?
> > 
> > Sorry, didn't v2 address your concerns?  We don't initialize the device
> > because maybe we'll have env there, we initalize mmc because we need to
> > check that it is there.  Thanks!
> 
> No, it does not.  As is, we initialize MMC in the EXT4 code (in
> env_ext4_load()). If we go that route, we would have to add similar
> init calls to all other file systemn load routines as well.
> 
> This does not make sense to me.  This pollutes the code with
> unrelated things - file system code should not depend on any
> underlaying driver suport.  It increases code size, makes the code
> harder to maintain (if you need to change this, there is good
> chances to forget the same change in other file systems), and there
> is a good chance that in the end you will initialzie MMC even if you
> don't use it.
> 
> We should keep the code clean and orthogonal.  Driver init code has
> no place in file system code.
> 
> If needed, the drivers have to make sure to auto--initialize on
> first access.
> 
> I hold my NAK on this patch.  It is the wrong thing to do.

I think you have this a little bit wrong.  But, it's also where we are
with the DM conversion.  This _is_ the first place we're trying to
access the MMC.  And it's not in the filesystem code, it's in the
environment code.

That said, Faiz, what exactly is the failure sequence (and hardware)?
Thanks!
Wolfgang Denk Feb. 25, 2018, 2:50 p.m. | #7
Dear Tom,

In message <20180225134810.GU4311@bill-the-cat> you wrote:
> 
> > We should keep the code clean and orthogonal.  Driver init code has
> > no place in file system code.
> >
> > If needed, the drivers have to make sure to auto--initialize on
> > first access.
> >
> > I hold my NAK on this patch.  It is the wrong thing to do.
> 
> I think you have this a little bit wrong.  But, it's also where we are
> with the DM conversion.  This _is_ the first place we're trying to
> access the MMC.  And it's not in the filesystem code, it's in the
> environment code.

Maybe I was not really clear.  You are right as this is not generic
file system code.  Instead, it is the EXT4 support code for the
environment handling.

The patch adds mmc_initialize() to env_ext4_load(), so whenever we
try to load the environment from an EXT4 file system, it will
initialize the MMC subsystem.

However - what if we want to load the environment from an EXT4 file
system on any other storage device - say, USB, or SATA, or flash?

In all such cases the initialization of MMC would be plain wrong.


And what if we want to load the environment from any other type of
file system - say, cramfs, zfs, etc. - on SDCard or eMMC?  These do
not initialize MMC, so it would fail?

Yes, we have the same crappy code in env/fat.c, but this is the
wrong thing to do, and must be cleaned up there as well.


This is what I meant when I wrote that the file system (interface)
code and the storage device support code are independent of each
other, and code should be kept orthogonal - storage support like MMC
etc. has no place in code dealing with a specific file system type.

I still think my NAK is reasonable.

Best regards,

Wolfgang Denk
Lukasz Majewski Feb. 25, 2018, 3:18 p.m. | #8
Hi Tom, Wolfgang,

> On Sun, Feb 25, 2018 at 09:53:10AM +0100, Wolfgang Denk wrote:
> > Dear Tom Rini,
> > 
> > In message <20180224215325.GQ4311@bill-the-cat> you wrote:  
> > >   
> > > > Why do you ignore this NAK, and why do you add this patch so
> > > > late in the release cycle anyway?  
> > > 
> > > Sorry, didn't v2 address your concerns?  We don't initialize the
> > > device because maybe we'll have env there, we initalize mmc
> > > because we need to check that it is there.  Thanks!  
> > 
> > No, it does not.  As is, we initialize MMC in the EXT4 code (in
> > env_ext4_load()). If we go that route, we would have to add similar
> > init calls to all other file systemn load routines as well.
> > 
> > This does not make sense to me.  This pollutes the code with
> > unrelated things - file system code should not depend on any
> > underlaying driver suport.  It increases code size, makes the code
> > harder to maintain (if you need to change this, there is good
> > chances to forget the same change in other file systems), and there
> > is a good chance that in the end you will initialzie MMC even if you
> > don't use it.
> > 
> > We should keep the code clean and orthogonal.  Driver init code has
> > no place in file system code.
> > 
> > If needed, the drivers have to make sure to auto--initialize on
> > first access.
> > 
> > I hold my NAK on this patch.  It is the wrong thing to do.  
> 
> I think you have this a little bit wrong.  But, it's also where we are
> with the DM conversion.  This _is_ the first place we're trying to
> access the MMC.  And it's not in the filesystem code, it's in the
> environment code.
> 
> That said, Faiz, what exactly is the failure sequence (and hardware)?

As I've read the discussion between Tom and Wolfgang - I'm wondering if
this initialization could be done in the driver model? 

I think that DM is a right place to put such code (ecluding the case
of env in eMMC readed in SPL).

I've added Simon to CC, so maybe he can give us some insights.

> Thanks!
> 


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Wolfgang Denk Feb. 25, 2018, 5:38 p.m. | #9
Dear Lukasz,

In message <20180225161813.10554012@jawa> you wrote:
>
> As I've read the discussion between Tom and Wolfgang - I'm wondering if
> this initialization could be done in the driver model?

Indeed DM would be a good place for such lazy initialization as
would be useful here.
> 
> I think that DM is a right place to put such code (ecluding the case
> of env in eMMC readed in SPL).

But even there we cannot add code to initialize all kinds of
potential storage devices to all kinds of supported file systems.
This may work with a single fixed configuration (which probably is
what the OP had in mind), but it does not scale, and it is the wrong
thing to do.

> I've added Simon to CC, so maybe he can give us some insights.

Thanks.  Hopefully Simon has a clever idea...

Best regards,

Wolfgang Denk
Tom Rini Feb. 25, 2018, 6:35 p.m. | #10
On Sun, Feb 25, 2018 at 03:50:41PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <20180225134810.GU4311@bill-the-cat> you wrote:
> > 
> > > We should keep the code clean and orthogonal.  Driver init code has
> > > no place in file system code.
> > >
> > > If needed, the drivers have to make sure to auto--initialize on
> > > first access.
> > >
> > > I hold my NAK on this patch.  It is the wrong thing to do.
> > 
> > I think you have this a little bit wrong.  But, it's also where we are
> > with the DM conversion.  This _is_ the first place we're trying to
> > access the MMC.  And it's not in the filesystem code, it's in the
> > environment code.
> 
> Maybe I was not really clear.  You are right as this is not generic
> file system code.  Instead, it is the EXT4 support code for the
> environment handling.
> 
> The patch adds mmc_initialize() to env_ext4_load(), so whenever we
> try to load the environment from an EXT4 file system, it will
> initialize the MMC subsystem.
> 
> However - what if we want to load the environment from an EXT4 file
> system on any other storage device - say, USB, or SATA, or flash?

Good question, and part of why after re-reading the code, I want to see
just what the combination of hardware Faiz is trying is.  MMC should
already have been initialized.  Unless I'm missing the specific init
path he has.  This does also highlight that env on fat/ext4 as only
likely been tested on MMC devices, even prior to the recent changes.

> In all such cases the initialization of MMC would be plain wrong.

Correct, and we don't try and initialize MMC if we aren't told that the
environment resides on mmc.

> And what if we want to load the environment from any other type of
> file system - say, cramfs, zfs, etc. - on SDCard or eMMC?  These do
> not initialize MMC, so it would fail?
> 
> Yes, we have the same crappy code in env/fat.c, but this is the
> wrong thing to do, and must be cleaned up there as well.

Yes, one of the things I've suggested before is that ENV_IS_IN_FAT and
ENV_IS_IN_EXT should be able to be rewritten to use the generic FS
operation API we have so that zfs, etc, could be automatically
supported.

> This is what I meant when I wrote that the file system (interface)
> code and the storage device support code are independent of each
> other, and code should be kept orthogonal - storage support like MMC
> etc. has no place in code dealing with a specific file system type.

The problem we have today, and I hope we can more cleverly resolve once
more/everything has been migrated to DM is that don't yet have a good
way to say "retry $X later" or "retry $X after $Y".  Because setting
aside the specific issue Faiz ran into, FS+SATA has never worked because
initr_scsi is well after initr_env, and that's why the sata env code
(which uses blocks and not fs) has always had to sata_initialize().

> I still think my NAK is reasonable.

Conceptually, I disagree because we don't yet have a more nicer solution
available yet.  With this specific patch, it might be a problem with the
board code, as mmc_initialize() should already have been called.  So
maybe this needs to come out.
Faiz Abbas Feb. 26, 2018, 2:29 p.m. | #11
Hi,

On Sunday 25 February 2018 07:18 PM, Tom Rini wrote:
> On Sun, Feb 25, 2018 at 09:53:10AM +0100, Wolfgang Denk wrote:
>> Dear Tom Rini,
>>
>> In message <20180224215325.GQ4311@bill-the-cat> you wrote:
>>>
>>>> Why do you ignore this NAK, and why do you add this patch so late in
>>>> the release cycle anyway?
>>>
>>> Sorry, didn't v2 address your concerns?  We don't initialize the device
>>> because maybe we'll have env there, we initalize mmc because we need to
>>> check that it is there.  Thanks!
>>
>> No, it does not.  As is, we initialize MMC in the EXT4 code (in
>> env_ext4_load()). If we go that route, we would have to add similar
>> init calls to all other file systemn load routines as well.
>>
>> This does not make sense to me.  This pollutes the code with
>> unrelated things - file system code should not depend on any
>> underlaying driver suport.  It increases code size, makes the code
>> harder to maintain (if you need to change this, there is good
>> chances to forget the same change in other file systems), and there
>> is a good chance that in the end you will initialzie MMC even if you
>> don't use it.
>>
>> We should keep the code clean and orthogonal.  Driver init code has
>> no place in file system code.
>>
>> If needed, the drivers have to make sure to auto--initialize on
>> first access.
>>
>> I hold my NAK on this patch.  It is the wrong thing to do.
> 
> I think you have this a little bit wrong.  But, it's also where we are
> with the DM conversion.  This _is_ the first place we're trying to
> access the MMC.  And it's not in the filesystem code, it's in the
> environment code.
> 
> That said, Faiz, what exactly is the failure sequence (and hardware)?
> Thanks!
>


The failure happens in SPL when booting from a non-MMC device (say NAND)
and environment is in MMC. I have seen it in am335x_evm (with NAND and
ethernet boot mode) and in dra7xx_evm (with qspi boot mode).

The failure sequence is as follows:

1. spl_start_uboot() in the appropriate board file calls env_load().

2. Since ENV_IS_IN_FAT and CONFIG_ENV_FAT_INTERFACE=mmc, env_fat_load()
eventually leads to a call to find_mmc_device() in
drivers/mmc/mmc_legacy.c or mmc-uclass.c

3. Since the mmc devices have not been probed (by a call to
mmc_initialize()), SPL is not able to get the environment and I get this
warning message:

*** Warning - No MMC card found, using default environment


Note: In case of legacy mode (CONFIG_BLK is not enabled), SPL crashes
because of dereferencing a NULL pointer (mmc_devices) in find_mmc_device().


Thanks,
Faiz
Lukasz Majewski Feb. 28, 2018, 9:08 a.m. | #12
On Mon, 26 Feb 2018 19:59:46 +0530
Faiz Abbas <faiz_abbas@ti.com> wrote:

> Hi,
> 
> On Sunday 25 February 2018 07:18 PM, Tom Rini wrote:
> > On Sun, Feb 25, 2018 at 09:53:10AM +0100, Wolfgang Denk wrote:  
> >> Dear Tom Rini,
> >>
> >> In message <20180224215325.GQ4311@bill-the-cat> you wrote:  
> >>>  
> >>>> Why do you ignore this NAK, and why do you add this patch so
> >>>> late in the release cycle anyway?  
> >>>
> >>> Sorry, didn't v2 address your concerns?  We don't initialize the
> >>> device because maybe we'll have env there, we initalize mmc
> >>> because we need to check that it is there.  Thanks!  
> >>
> >> No, it does not.  As is, we initialize MMC in the EXT4 code (in
> >> env_ext4_load()). If we go that route, we would have to add similar
> >> init calls to all other file systemn load routines as well.
> >>
> >> This does not make sense to me.  This pollutes the code with
> >> unrelated things - file system code should not depend on any
> >> underlaying driver suport.  It increases code size, makes the code
> >> harder to maintain (if you need to change this, there is good
> >> chances to forget the same change in other file systems), and there
> >> is a good chance that in the end you will initialzie MMC even if
> >> you don't use it.
> >>
> >> We should keep the code clean and orthogonal.  Driver init code has
> >> no place in file system code.
> >>
> >> If needed, the drivers have to make sure to auto--initialize on
> >> first access.
> >>
> >> I hold my NAK on this patch.  It is the wrong thing to do.  
> > 
> > I think you have this a little bit wrong.  But, it's also where we
> > are with the DM conversion.  This _is_ the first place we're trying
> > to access the MMC.  And it's not in the filesystem code, it's in the
> > environment code.
> > 
> > That said, Faiz, what exactly is the failure sequence (and
> > hardware)? Thanks!
> >  
> 
> 
> The failure happens in SPL when booting from a non-MMC device (say
> NAND) and environment is in MMC. I have seen it in am335x_evm (with
> NAND and ethernet boot mode) and in dra7xx_evm (with qspi boot mode).
> 
> The failure sequence is as follows:
> 
> 1. spl_start_uboot() in the appropriate board file calls env_load().

Just out of curiosity - is the env_load() preceded with env_init() ?

Maybe env_init() is the place to resolve the issue with eMMC
init (to call board_mmc_init() for SPL) ?



> 
> 2. Since ENV_IS_IN_FAT and CONFIG_ENV_FAT_INTERFACE=mmc,
> env_fat_load() eventually leads to a call to find_mmc_device() in
> drivers/mmc/mmc_legacy.c or mmc-uclass.c
> 
> 3. Since the mmc devices have not been probed (by a call to
> mmc_initialize()), SPL is not able to get the environment and I get
> this warning message:
> 
> *** Warning - No MMC card found, using default environment
> 
> 
> Note: In case of legacy mode (CONFIG_BLK is not enabled), SPL crashes
> because of dereferencing a NULL pointer (mmc_devices) in
> find_mmc_device().
> 
> 
> Thanks,
> Faiz
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Faiz Abbas March 5, 2018, 9:59 a.m. | #13
Hi,

On Wednesday 28 February 2018 02:38 PM, Lukasz Majewski wrote:
> On Mon, 26 Feb 2018 19:59:46 +0530
> Faiz Abbas <faiz_abbas@ti.com> wrote:
> 
>> Hi,
>>
>> On Sunday 25 February 2018 07:18 PM, Tom Rini wrote:
>>> On Sun, Feb 25, 2018 at 09:53:10AM +0100, Wolfgang Denk wrote:  
>>>> Dear Tom Rini,
>>>>
>>>> In message <20180224215325.GQ4311@bill-the-cat> you wrote:  
>>>>>  
>>>>>> Why do you ignore this NAK, and why do you add this patch so
>>>>>> late in the release cycle anyway?  
>>>>>
>>>>> Sorry, didn't v2 address your concerns?  We don't initialize the
>>>>> device because maybe we'll have env there, we initalize mmc
>>>>> because we need to check that it is there.  Thanks!  
>>>>
>>>> No, it does not.  As is, we initialize MMC in the EXT4 code (in
>>>> env_ext4_load()). If we go that route, we would have to add similar
>>>> init calls to all other file systemn load routines as well.
>>>>
>>>> This does not make sense to me.  This pollutes the code with
>>>> unrelated things - file system code should not depend on any
>>>> underlaying driver suport.  It increases code size, makes the code
>>>> harder to maintain (if you need to change this, there is good
>>>> chances to forget the same change in other file systems), and there
>>>> is a good chance that in the end you will initialzie MMC even if
>>>> you don't use it.
>>>>
>>>> We should keep the code clean and orthogonal.  Driver init code has
>>>> no place in file system code.
>>>>
>>>> If needed, the drivers have to make sure to auto--initialize on
>>>> first access.
>>>>
>>>> I hold my NAK on this patch.  It is the wrong thing to do.  
>>>
>>> I think you have this a little bit wrong.  But, it's also where we
>>> are with the DM conversion.  This _is_ the first place we're trying
>>> to access the MMC.  And it's not in the filesystem code, it's in the
>>> environment code.
>>>
>>> That said, Faiz, what exactly is the failure sequence (and
>>> hardware)? Thanks!
>>>  
>>
>>
>> The failure happens in SPL when booting from a non-MMC device (say
>> NAND) and environment is in MMC. I have seen it in am335x_evm (with
>> NAND and ethernet boot mode) and in dra7xx_evm (with qspi boot mode).
>>
>> The failure sequence is as follows:
>>
>> 1. spl_start_uboot() in the appropriate board file calls env_load().
> 
> Just out of curiosity - is the env_load() preceded with env_init() ?
> 
> Maybe env_init() is the place to resolve the issue with eMMC
> init (to call board_mmc_init() for SPL) ?
> 

That was the case in v1.

Check Wolfgang's comments there.

Thanks,
Faiz

Patch

diff --git a/env/ext4.c b/env/ext4.c
index 3f3aac5..6c69a0a 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -87,6 +87,9 @@  static int env_ext4_load(void)
 	int err;
 	loff_t off;
 
+	if (!strcmp(CONFIG_ENV_EXT4_INTERFACE, "mmc"))
+		mmc_initialize(NULL);
+
 	part = blk_get_device_part_str(CONFIG_ENV_EXT4_INTERFACE,
 				       CONFIG_ENV_EXT4_DEVICE_AND_PART,
 				       &dev_desc, &info, 1);
diff --git a/env/fat.c b/env/fat.c
index 35f7ab5..fdf4b7a 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -89,6 +89,9 @@  static int env_fat_load(void)
 	int dev, part;
 	int err;
 
+	if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc"))
+		mmc_initialize(NULL);
+
 	part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
 					CONFIG_ENV_FAT_DEVICE_AND_PART,
 					&dev_desc, &info, 1);
diff --git a/env/mmc.c b/env/mmc.c
index 1058b8c..6f11dec 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -273,6 +273,8 @@  static int env_mmc_load(void)
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, tmp_env1, 1);
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, tmp_env2, 1);
 
+	mmc_initialize(NULL);
+
 	mmc = find_mmc_device(dev);
 
 	errmsg = init_mmc_for_env(mmc);