| Message ID | 20170424020211.20690-2-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | e7017a3c7d2f9ff845516675205c99df4ad6eacc |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Simon, On 24/04/2017 04:02, Simon Glass wrote: > With driver model MMC is probed automatically when needed. We should not > re-init MMC each time. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > common/env_mmc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/common/env_mmc.c b/common/env_mmc.c > index a5d14d448c..1611886e22 100644 > --- a/common/env_mmc.c > +++ b/common/env_mmc.c > @@ -98,9 +98,10 @@ static const char *init_mmc_for_env(struct mmc *mmc) > if (!mmc) > return "!No MMC card found"; > > +#ifndef CONFIG_BLK > if (mmc_init(mmc)) > return "!MMC init failed"; > - > +#endif I'm not convinced by this. mmc_init() is the starting point of the MMC device initialization process and it must be called somehow before accessing the device and most probe() functions do not call mmc_init(). The sandbox driver does it, but I'm not sure it's the right way because the MMC device initialization process takes a long time. I'd rather have the device initialized only when it's accessed for the first time not when it's probed (especially in the SPL) Jean-Jacques > if (mmc_set_env_part(mmc)) > return "!MMC partition switch failed"; >
Hi Jean-Jacques, On 24 April 2017 at 02:55, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: > Hi Simon, > > On 24/04/2017 04:02, Simon Glass wrote: >> >> With driver model MMC is probed automatically when needed. We should not >> re-init MMC each time. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> common/env_mmc.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/common/env_mmc.c b/common/env_mmc.c >> index a5d14d448c..1611886e22 100644 >> --- a/common/env_mmc.c >> +++ b/common/env_mmc.c >> @@ -98,9 +98,10 @@ static const char *init_mmc_for_env(struct mmc *mmc) >> if (!mmc) >> return "!No MMC card found"; >> +#ifndef CONFIG_BLK >> if (mmc_init(mmc)) >> return "!MMC init failed"; >> - >> +#endif > > I'm not convinced by this. mmc_init() is the starting point of the MMC > device initialization process and it must be called somehow before accessing > the device and most probe() functions do not call mmc_init(). > The sandbox driver does it, but I'm not sure it's the right way because the > MMC device initialization process takes a long time. I'd rather have the > device initialized only when it's accessed for the first time not when it's > probed (especially in the SPL) Yes I would like that too. One option is to add an init() method to mmc and call that when the block device is probed. > > Jean-Jacques > > >> if (mmc_set_env_part(mmc)) >> return "!MMC partition switch failed"; >> > > Regards, Simon
Hi, On 28 April 2017 at 18:27, Simon Glass <sjg@chromium.org> wrote: > Hi Jean-Jacques, > > On 24 April 2017 at 02:55, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: >> Hi Simon, >> >> On 24/04/2017 04:02, Simon Glass wrote: >>> >>> With driver model MMC is probed automatically when needed. We should not >>> re-init MMC each time. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> common/env_mmc.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/common/env_mmc.c b/common/env_mmc.c >>> index a5d14d448c..1611886e22 100644 >>> --- a/common/env_mmc.c >>> +++ b/common/env_mmc.c >>> @@ -98,9 +98,10 @@ static const char *init_mmc_for_env(struct mmc *mmc) >>> if (!mmc) >>> return "!No MMC card found"; >>> +#ifndef CONFIG_BLK >>> if (mmc_init(mmc)) >>> return "!MMC init failed"; >>> - >>> +#endif >> >> I'm not convinced by this. mmc_init() is the starting point of the MMC >> device initialization process and it must be called somehow before accessing >> the device and most probe() functions do not call mmc_init(). >> The sandbox driver does it, but I'm not sure it's the right way because the >> MMC device initialization process takes a long time. I'd rather have the >> device initialized only when it's accessed for the first time not when it's >> probed (especially in the SPL) > > Yes I would like that too. > > One option is to add an init() method to mmc and call that when the > block device is probed. Do you think that would work? I can try it out in the next version perhaps. Then we have: mmc device probe - very fast, just sets up clocks, etc. blk device probe - slow, tries to read card, partition table, etc. > >> >> Jean-Jacques >> >> >>> if (mmc_set_env_part(mmc)) >>> return "!MMC partition switch failed"; Regards, Simon
Hi Simon, On 03/05/2017 04:35, Simon Glass wrote: > Hi, > > On 28 April 2017 at 18:27, Simon Glass <sjg@chromium.org> wrote: >> Hi Jean-Jacques, >> >> On 24 April 2017 at 02:55, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote: >>> Hi Simon, >>> >>> On 24/04/2017 04:02, Simon Glass wrote: >>>> With driver model MMC is probed automatically when needed. We should not >>>> re-init MMC each time. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> --- >>>> >>>> common/env_mmc.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/common/env_mmc.c b/common/env_mmc.c >>>> index a5d14d448c..1611886e22 100644 >>>> --- a/common/env_mmc.c >>>> +++ b/common/env_mmc.c >>>> @@ -98,9 +98,10 @@ static const char *init_mmc_for_env(struct mmc *mmc) >>>> if (!mmc) >>>> return "!No MMC card found"; >>>> +#ifndef CONFIG_BLK >>>> if (mmc_init(mmc)) >>>> return "!MMC init failed"; >>>> - >>>> +#endif >>> I'm not convinced by this. mmc_init() is the starting point of the MMC >>> device initialization process and it must be called somehow before accessing >>> the device and most probe() functions do not call mmc_init(). >>> The sandbox driver does it, but I'm not sure it's the right way because the >>> MMC device initialization process takes a long time. I'd rather have the >>> device initialized only when it's accessed for the first time not when it's >>> probed (especially in the SPL) >> Yes I would like that too. >> >> One option is to add an init() method to mmc and call that when the >> block device is probed. I've just discovered that in mmc_init() is now called from the 'blk' probe of the mc device (it wasn't on the code base I had been looking at). So it should be fine to remove the call to mmc_init() from init_mmc_for_env(). I did the test on DRA7 and it's working. So you can ignore my previous comment. However I tried to go further by removing it from spl_mmc.c as well because mmc_init() is called from there first and then I ran into problems with the detection of the partitions. I don't really have time to look at it right now. It may be related to the fact that my env is not located on the boot device. Jean-Jacques > Do you think that would work? I can try it out in the next version perhaps. > > Then we have: > > mmc device probe - very fast, just sets up clocks, etc. > blk device probe - slow, tries to read card, partition table, etc. > >>> Jean-Jacques >>> >>> >>>> if (mmc_set_env_part(mmc)) >>>> return "!MMC partition switch failed"; > Regards, > Simon >
diff --git a/common/env_mmc.c b/common/env_mmc.c index a5d14d448c..1611886e22 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -98,9 +98,10 @@ static const char *init_mmc_for_env(struct mmc *mmc) if (!mmc) return "!No MMC card found"; +#ifndef CONFIG_BLK if (mmc_init(mmc)) return "!MMC init failed"; - +#endif if (mmc_set_env_part(mmc)) return "!MMC partition switch failed";
With driver model MMC is probed automatically when needed. We should not re-init MMC each time. Signed-off-by: Simon Glass <sjg@chromium.org> --- common/env_mmc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)