* [U-Boot] [U-Boot, v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it
2018-02-25 13:48 ` Tom Rini
@ 2018-02-25 14:50 ` Wolfgang Denk
2018-02-25 18:35 ` Tom Rini
2018-02-25 15:18 ` Lukasz Majewski
2018-02-26 14:29 ` Faiz Abbas
2 siblings, 1 reply; 14+ messages in thread
From: Wolfgang Denk @ 2018-02-25 14:50 UTC (permalink / raw)
To: u-boot
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
--
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 at denx.de
I had the rare misfortune of being one of the first people to try and
implement a PL/1 compiler. -- T. Cheatham
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [U-Boot, v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it
2018-02-25 14:50 ` Wolfgang Denk
@ 2018-02-25 18:35 ` Tom Rini
0 siblings, 0 replies; 14+ messages in thread
From: Tom Rini @ 2018-02-25 18:35 UTC (permalink / raw)
To: u-boot
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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180225/b7506fad/attachment.sig>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [U-Boot, v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it
2018-02-25 13:48 ` Tom Rini
2018-02-25 14:50 ` Wolfgang Denk
@ 2018-02-25 15:18 ` Lukasz Majewski
2018-02-25 17:38 ` Wolfgang Denk
2018-02-26 14:29 ` Faiz Abbas
2 siblings, 1 reply; 14+ messages in thread
From: Lukasz Majewski @ 2018-02-25 15:18 UTC (permalink / raw)
To: u-boot
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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180225/31133e6c/attachment.sig>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [U-Boot, v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it
2018-02-25 15:18 ` Lukasz Majewski
@ 2018-02-25 17:38 ` Wolfgang Denk
0 siblings, 0 replies; 14+ messages in thread
From: Wolfgang Denk @ 2018-02-25 17:38 UTC (permalink / raw)
To: u-boot
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
--
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 at denx.de
"If you can, help others. If you can't, at least don't hurt others."
- the Dalai Lama
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [U-Boot, v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it
2018-02-25 13:48 ` Tom Rini
2018-02-25 14:50 ` Wolfgang Denk
2018-02-25 15:18 ` Lukasz Majewski
@ 2018-02-26 14:29 ` Faiz Abbas
2018-02-28 9:08 ` Lukasz Majewski
2 siblings, 1 reply; 14+ messages in thread
From: Faiz Abbas @ 2018-02-26 14:29 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [U-Boot, v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it
2018-02-26 14:29 ` Faiz Abbas
@ 2018-02-28 9:08 ` Lukasz Majewski
2018-03-05 9:59 ` Faiz Abbas
0 siblings, 1 reply; 14+ messages in thread
From: Lukasz Majewski @ 2018-02-28 9:08 UTC (permalink / raw)
To: u-boot
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 at 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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180228/3324f292/attachment.sig>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [U-Boot] [U-Boot, v2] env: mmc/fat/ext4: make sure that the MMC sub-system is initialized before using it
2018-02-28 9:08 ` Lukasz Majewski
@ 2018-03-05 9:59 ` Faiz Abbas
0 siblings, 0 replies; 14+ messages in thread
From: Faiz Abbas @ 2018-03-05 9:59 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 14+ messages in thread