| Message ID | 1445515280-21389-4-git-send-email-nikita@compulab.co.il |
|---|---|
| State | Superseded |
| Delegated to: | Pantelis Antoniou |
| Headers | show |
Hi, On 22-10-15 14:01, Nikita Kiryanov wrote: > Simplify spl_mmc_load_image() code by moving the part that finds the mmc device > into its own function spl_mmc_find_device(), available in two flavors: DM and > non-DM. > > This refactor fixes a bug in which an error in the device location sequence > does not necessarily aborts the rest of the code. With this refactor, we fail > the moment there is an error. > > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > Cc: Igor Grinberg <grinberg@compulab.co.il> > Cc: Paul Kocialkowski <contact@paulk.fr> > Cc: Pantelis Antoniou <panto@antoniou-consulting.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> > --- > common/spl/spl_mmc.c | 77 +++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 55 insertions(+), 22 deletions(-) > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c > index e831970..cfbda1a 100644 > --- a/common/spl/spl_mmc.c > +++ b/common/spl/spl_mmc.c > @@ -11,6 +11,7 @@ > #include <spl.h> > #include <linux/compiler.h> > #include <asm/u-boot.h> > +#include <errno.h> > #include <mmc.h> > #include <image.h> > > @@ -59,6 +60,58 @@ end: > return 0; > } > > +#ifdef CONFIG_DM_MMC > +static int spl_mmc_find_device(struct mmc **mmc) > +{ > + struct udevice *dev; > + int err; > + > + err = mmc_initialize(NULL); > + if (err) { > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > + printf("spl: could not initialize mmc. error: %d\n", err); > +#endif > + return err; > + } > + > + err = uclass_get_device(UCLASS_MMC, 0, &dev); > + if (err) { > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > + puts("spl: could not find mmc device. error: %d\n", err); > +#endif > + return err; > + } > + > + *mmc = NULL; > + *mmc = mmc_get_mmc_dev(dev); > + return *mmc != NULL ? 0 : -ENODEV; > +} > +#else > +static int spl_mmc_find_device(struct mmc **mmc) > +{ > + int err; > + > + err = mmc_initialize(gd->bd); > + if (err) { > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > + printf("spl: could not initialize mmc. error: %d\n", err); > +#endif > + return err; > + } > + > + /* We register only one device. So, the dev id is always 0 */ > + *mmc = find_mmc_device(0); This does not work when there are 2 mmc devices, 1 main and one alternate boot device (sunxi has this). spl_boot_device() can return: BOOT_DEVICE_MMC1 and/or BOOT_DEVICE_MMC2 currently the number in this gets completely ignored by the spl mmc code, this patch-set seems like a good moment to fix this. This will allow cleaning up hacks like this one: if (CONFIG_MMC_SUNXI_SLOT_EXTRA == 2) { mmc1 = find_mmc_device(1); if (sunxi_mmc_has_egon_boot_signature(mmc1)) { /* * spl_mmc.c: spl_mmc_load_image() is hard-coded to * use find_mmc_device(0), no matter what we * return. Swap mmc0 and mmc2 to make this work. */ mmc0->block_dev.dev = 1; mmc1->block_dev.dev = 0; return BOOT_DEVICE_MMC2; } } Regards, Hans spl_ > + if (!*mmc) { > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > + puts("spl: mmc device not found\n"); > +#endif > + return -ENODEV; > + } > + > + return 0; > +} > +#endif > + > #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION > static int mmc_load_image_raw_partition(struct mmc *mmc, int partition) > { > @@ -110,30 +163,10 @@ void spl_mmc_load_image(void) > int err = 0; > __maybe_unused int part; > > -#ifdef CONFIG_DM_MMC > - struct udevice *dev; > - > - mmc_initialize(NULL); > - err = uclass_get_device(UCLASS_MMC, 0, &dev); > - mmc = NULL; > - if (!err) > - mmc = mmc_get_mmc_dev(dev); > -#else > - mmc_initialize(gd->bd); > - > - /* We register only one device. So, the dev id is always 0 */ > - mmc = find_mmc_device(0); > - if (!mmc) { > -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > - puts("spl: mmc device not found\n"); > -#endif > + if (spl_mmc_find_device(&mmc)) > hang(); > - } > -#endif > - > - if (!err) > - err = mmc_init(mmc); > > + err = mmc_init(mmc); > if (err) { > #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > printf("spl: mmc init failed with error: %d\n", err); >
Hi, On Thu, Oct 22, 2015 at 02:42:22PM +0200, Hans de Goede wrote: > Hi, > > On 22-10-15 14:01, Nikita Kiryanov wrote: > >Simplify spl_mmc_load_image() code by moving the part that finds the mmc device > >into its own function spl_mmc_find_device(), available in two flavors: DM and > >non-DM. > > > >This refactor fixes a bug in which an error in the device location sequence > >does not necessarily aborts the rest of the code. With this refactor, we fail > >the moment there is an error. > > > >Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > >Cc: Igor Grinberg <grinberg@compulab.co.il> > >Cc: Paul Kocialkowski <contact@paulk.fr> > >Cc: Pantelis Antoniou <panto@antoniou-consulting.com> > >Cc: Tom Rini <trini@konsulko.com> > >Cc: Simon Glass <sjg@chromium.org> > >--- > > common/spl/spl_mmc.c | 77 +++++++++++++++++++++++++++++++++++++--------------- > > 1 file changed, 55 insertions(+), 22 deletions(-) > > > >diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c > >index e831970..cfbda1a 100644 > >--- a/common/spl/spl_mmc.c > >+++ b/common/spl/spl_mmc.c > >@@ -11,6 +11,7 @@ > > #include <spl.h> > > #include <linux/compiler.h> > > #include <asm/u-boot.h> > >+#include <errno.h> > > #include <mmc.h> > > #include <image.h> > > > >@@ -59,6 +60,58 @@ end: > > return 0; > > } > > > >+#ifdef CONFIG_DM_MMC > >+static int spl_mmc_find_device(struct mmc **mmc) > >+{ > >+ struct udevice *dev; > >+ int err; > >+ > >+ err = mmc_initialize(NULL); > >+ if (err) { > >+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > >+ printf("spl: could not initialize mmc. error: %d\n", err); > >+#endif > >+ return err; > >+ } > >+ > >+ err = uclass_get_device(UCLASS_MMC, 0, &dev); > >+ if (err) { > >+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > >+ puts("spl: could not find mmc device. error: %d\n", err); > >+#endif > >+ return err; > >+ } > >+ > >+ *mmc = NULL; > >+ *mmc = mmc_get_mmc_dev(dev); > >+ return *mmc != NULL ? 0 : -ENODEV; > >+} > >+#else > >+static int spl_mmc_find_device(struct mmc **mmc) > >+{ > >+ int err; > >+ > >+ err = mmc_initialize(gd->bd); > >+ if (err) { > >+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > >+ printf("spl: could not initialize mmc. error: %d\n", err); > >+#endif > >+ return err; > >+ } > >+ > >+ /* We register only one device. So, the dev id is always 0 */ > >+ *mmc = find_mmc_device(0); > > This does not work when there are 2 mmc devices, 1 main and one alternate > boot device (sunxi has this). spl_boot_device() can return: > BOOT_DEVICE_MMC1 and/or BOOT_DEVICE_MMC2 currently the number in this gets > completely ignored by the spl mmc code, this patch-set seems like a good > moment to fix this. I'll look into incorporating it into a V2 (though my time constraints may force me to do otherwise). > > This will allow cleaning up hacks like this one: > > if (CONFIG_MMC_SUNXI_SLOT_EXTRA == 2) { > mmc1 = find_mmc_device(1); > if (sunxi_mmc_has_egon_boot_signature(mmc1)) { > /* > * spl_mmc.c: spl_mmc_load_image() is hard-coded to > * use find_mmc_device(0), no matter what we > * return. Swap mmc0 and mmc2 to make this work. > */ > mmc0->block_dev.dev = 1; > mmc1->block_dev.dev = 0; > return BOOT_DEVICE_MMC2; > } > } > > Regards, > > Hans >
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index e831970..cfbda1a 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -11,6 +11,7 @@ #include <spl.h> #include <linux/compiler.h> #include <asm/u-boot.h> +#include <errno.h> #include <mmc.h> #include <image.h> @@ -59,6 +60,58 @@ end: return 0; } +#ifdef CONFIG_DM_MMC +static int spl_mmc_find_device(struct mmc **mmc) +{ + struct udevice *dev; + int err; + + err = mmc_initialize(NULL); + if (err) { +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT + printf("spl: could not initialize mmc. error: %d\n", err); +#endif + return err; + } + + err = uclass_get_device(UCLASS_MMC, 0, &dev); + if (err) { +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT + puts("spl: could not find mmc device. error: %d\n", err); +#endif + return err; + } + + *mmc = NULL; + *mmc = mmc_get_mmc_dev(dev); + return *mmc != NULL ? 0 : -ENODEV; +} +#else +static int spl_mmc_find_device(struct mmc **mmc) +{ + int err; + + err = mmc_initialize(gd->bd); + if (err) { +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT + printf("spl: could not initialize mmc. error: %d\n", err); +#endif + return err; + } + + /* We register only one device. So, the dev id is always 0 */ + *mmc = find_mmc_device(0); + if (!*mmc) { +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT + puts("spl: mmc device not found\n"); +#endif + return -ENODEV; + } + + return 0; +} +#endif + #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION static int mmc_load_image_raw_partition(struct mmc *mmc, int partition) { @@ -110,30 +163,10 @@ void spl_mmc_load_image(void) int err = 0; __maybe_unused int part; -#ifdef CONFIG_DM_MMC - struct udevice *dev; - - mmc_initialize(NULL); - err = uclass_get_device(UCLASS_MMC, 0, &dev); - mmc = NULL; - if (!err) - mmc = mmc_get_mmc_dev(dev); -#else - mmc_initialize(gd->bd); - - /* We register only one device. So, the dev id is always 0 */ - mmc = find_mmc_device(0); - if (!mmc) { -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT - puts("spl: mmc device not found\n"); -#endif + if (spl_mmc_find_device(&mmc)) hang(); - } -#endif - - if (!err) - err = mmc_init(mmc); + err = mmc_init(mmc); if (err) { #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT printf("spl: mmc init failed with error: %d\n", err);
Simplify spl_mmc_load_image() code by moving the part that finds the mmc device into its own function spl_mmc_find_device(), available in two flavors: DM and non-DM. This refactor fixes a bug in which an error in the device location sequence does not necessarily aborts the rest of the code. With this refactor, we fail the moment there is an error. Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> Cc: Igor Grinberg <grinberg@compulab.co.il> Cc: Paul Kocialkowski <contact@paulk.fr> Cc: Pantelis Antoniou <panto@antoniou-consulting.com> Cc: Tom Rini <trini@konsulko.com> Cc: Simon Glass <sjg@chromium.org> --- common/spl/spl_mmc.c | 77 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 22 deletions(-)