| 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 | expand |
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
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!
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
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!
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
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!
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
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
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
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.
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
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
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
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);
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(+)