diff mbox

[U-Boot,1/2] spl: spl_mmc: Clearer structure in spl_mmc_load_image and cosmetics

Message ID 1429471809-17069-1-git-send-email-contact@paulk.fr
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Paul Kocialkowski April 19, 2015, 7:30 p.m. UTC 
This refactors spl_mmc_load_image to use a switch/case structure and easier
to understand spl_start_uboot checks. It also drops fallbacks on boot devices
that were not selected in the first place.

Lines that go beyond 80 chars are also reduced by reducing the number of tabs.
Debug and error strings are refctored to match a common style.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 common/spl/spl_mmc.c | 112 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 61 insertions(+), 51 deletions(-)

Comments

Tom Rini April 21, 2015, 1:32 p.m. UTC | #1 
On Sun, Apr 19, 2015 at 09:30:08PM +0200, Paul Kocialkowski wrote:

> This refactors spl_mmc_load_image to use a switch/case structure and easier
> to understand spl_start_uboot checks. It also drops fallbacks on boot devices
> that were not selected in the first place.

I don't like the dropping fallback on boot devices part and this is
going to break existing setups.  What some people do is on platforms
where the ROM doesn't grok FAT they still have u-boot.img on FAT and
just keep SPL written to the raw device.  Then booting from both raw or
RAW+FAT works.

> Lines that go beyond 80 chars are also reduced by reducing the number of tabs.
> Debug and error strings are refctored to match a common style.

I like the strings having a common style.  Please make sure that
checkpatch is happy about how you re-indent the code too, thanks.
Paul Kocialkowski April 27, 2015, 8:21 a.m. UTC | #2 
Le mardi 21 avril 2015 à 09:32 -0400, Tom Rini a écrit :
> On Sun, Apr 19, 2015 at 09:30:08PM +0200, Paul Kocialkowski wrote:
> 
> > This refactors spl_mmc_load_image to use a switch/case structure and easier
> > to understand spl_start_uboot checks. It also drops fallbacks on boot devices
> > that were not selected in the first place.
> 
> I don't like the dropping fallback on boot devices part and this is
> going to break existing setups.  What some people do is on platforms
> where the ROM doesn't grok FAT they still have u-boot.img on FAT and
> just keep SPL written to the raw device.  Then booting from both raw or
> RAW+FAT works.

Just sent out v2 addressing these concerns, thanks for the review!

> > Lines that go beyond 80 chars are also reduced by reducing the number of tabs.
> > Debug and error strings are refctored to match a common style.
> 
> I like the strings having a common style.  Please make sure that
> checkpatch is happy about how you re-indent the code too, thanks.

Checkpatch is happy indeed.
diff mbox

Patch

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index e580f22..dd150a7 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -21,7 +21,7 @@  static int mmc_load_image_raw_sector(struct mmc *mmc, unsigned long sector)
 	struct image_header *header;
 
 	header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
-						sizeof(struct image_header));
+					 sizeof(struct image_header));
 
 	/* read image header to find the image size & load address */
 	err = mmc->block_dev.block_read(0, sector, 1, header);
@@ -35,7 +35,7 @@  static int mmc_load_image_raw_sector(struct mmc *mmc, unsigned long sector)
 
 	/* convert size to sectors - round up */
 	image_size_sectors = (spl_image.size + mmc->read_bl_len - 1) /
-				mmc->read_bl_len;
+			     mmc->read_bl_len;
 
 	/* Read the header too to avoid extra memcpy */
 	err = mmc->block_dev.block_read(0, sector, image_size_sectors,
@@ -44,7 +44,7 @@  static int mmc_load_image_raw_sector(struct mmc *mmc, unsigned long sector)
 end:
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
 	if (err == 0)
-		printf("spl: mmc blk read err - %lu\n", err);
+		printf("spl: mmc block read error\n");
 #endif
 
 	return (err == 0);
@@ -69,33 +69,37 @@  static int mmc_load_image_raw_partition(struct mmc *mmc, int partition)
 #ifdef CONFIG_SPL_OS_BOOT
 static int mmc_load_image_raw_os(struct mmc *mmc)
 {
-	if (!mmc->block_dev.block_read(0,
-				       CONFIG_SYS_MMCSD_RAW_MODE_ARGS_SECTOR,
-				       CONFIG_SYS_MMCSD_RAW_MODE_ARGS_SECTORS,
-				       (void *)CONFIG_SYS_SPL_ARGS_ADDR)) {
+	unsigned long err;
+
+	err = mmc->block_dev.block_read(0,
+					CONFIG_SYS_MMCSD_RAW_MODE_ARGS_SECTOR,
+					CONFIG_SYS_MMCSD_RAW_MODE_ARGS_SECTORS,
+					(void *)CONFIG_SYS_SPL_ARGS_ADDR);
+	if (err) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		printf("mmc args blk read error\n");
+		printf("spl: mmc block read error\n");
 #endif
 		return -1;
 	}
 
 	return mmc_load_image_raw_sector(mmc,
-						CONFIG_SYS_MMCSD_RAW_MODE_KERNEL_SECTOR);
+		CONFIG_SYS_MMCSD_RAW_MODE_KERNEL_SECTOR);
 }
 #endif
 
 void spl_mmc_load_image(void)
 {
 	struct mmc *mmc;
-	int err;
 	u32 boot_mode;
+	int err;
 
 	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");
+		puts("spl: mmc device not found\n");
 #endif
 		hang();
 	}
@@ -103,16 +107,20 @@  void spl_mmc_load_image(void)
 	err = mmc_init(mmc);
 	if (err) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		printf("spl: mmc init failed: err - %d\n", err);
+		printf("spl: mmc init failed with error: %d\n", err);
 #endif
 		hang();
 	}
 
 	boot_mode = spl_boot_mode();
-	if (boot_mode == MMCSD_MODE_RAW) {
-		debug("boot mode - RAW\n");
+	switch (boot_mode) {
+	case MMCSD_MODE_RAW:
+		debug("spl: mmc boot mode: raw\n");
+
 #ifdef CONFIG_SPL_OS_BOOT
-		if (spl_start_uboot() || mmc_load_image_raw_os(mmc))
+		if (!spl_start_uboot())
+			err = mmc_load_image_raw_os(mmc);
+		else
 #endif
 #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION
 		err = mmc_load_image_raw_partition(mmc,
@@ -121,34 +129,44 @@  void spl_mmc_load_image(void)
 		err = mmc_load_image_raw_sector(mmc,
 			CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR);
 #endif
+		break;
 #if defined(CONFIG_SPL_FAT_SUPPORT) || defined(CONFIG_SPL_EXT_SUPPORT)
-	}
-	if (err || boot_mode == MMCSD_MODE_FS) {
-		debug("boot mode - FS\n");
+	case MMCSD_MODE_FS:
+		debug("spl: mmc boot mode: fs\n");
+
 #ifdef CONFIG_SPL_FAT_SUPPORT
 #ifdef CONFIG_SPL_OS_BOOT
-		if (spl_start_uboot() || spl_load_image_fat_os(&mmc->block_dev,
-								CONFIG_SYS_MMCSD_FS_BOOT_PARTITION))
+		if (!spl_start_uboot()) {
+			err = spl_load_image_fat_os(&mmc->block_dev,
+				CONFIG_SYS_MMCSD_FS_BOOT_PARTITION);
+			if (!err)
+				break;
+		} else {
 #endif
 		err = spl_load_image_fat(&mmc->block_dev,
-					CONFIG_SYS_MMCSD_FS_BOOT_PARTITION,
-					CONFIG_SPL_FS_LOAD_PAYLOAD_NAME);
-		if(err)
-#endif /* CONFIG_SPL_FAT_SUPPORT */
-		{
+					 CONFIG_SYS_MMCSD_FS_BOOT_PARTITION,
+					 CONFIG_SPL_FS_LOAD_PAYLOAD_NAME);
+		if (!err)
+			break;
+#ifdef CONFIG_SPL_OS_BOOT
+		}
+#endif
+#endif
 #ifdef CONFIG_SPL_EXT_SUPPORT
 #ifdef CONFIG_SPL_OS_BOOT
-		if (spl_start_uboot() || spl_load_image_ext_os(&mmc->block_dev,
-								CONFIG_SYS_MMCSD_FS_BOOT_PARTITION))
+		if (!spl_start_uboot())
+			err = spl_load_image_ext_os(&mmc->block_dev,
+				CONFIG_SYS_MMCSD_FS_BOOT_PARTITION))
+		else
 #endif
 		err = spl_load_image_ext(&mmc->block_dev,
-					CONFIG_SYS_MMCSD_FS_BOOT_PARTITION,
-					CONFIG_SPL_FS_LOAD_PAYLOAD_NAME);
-#endif /* CONFIG_SPL_EXT_SUPPORT */
-		}
-#endif /* defined(CONFIG_SPL_FAT_SUPPORT) || defined(CONFIG_SPL_EXT_SUPPORT) */
+					 CONFIG_SYS_MMCSD_FS_BOOT_PARTITION,
+					 CONFIG_SPL_FS_LOAD_PAYLOAD_NAME);
+#endif
+		break;
+#endif
 #ifdef CONFIG_SUPPORT_EMMC_BOOT
-	} else if (boot_mode == MMCSD_MODE_EMMCBOOT) {
+	case MMCSD_MODE_EMMCBOOT:
 		/*
 		 * We need to check what the partition is configured to.
 		 * 1 and 2 match up to boot0 / boot1 and 7 is user data
@@ -161,34 +179,26 @@  void spl_mmc_load_image(void)
 
 		if (mmc_switch_part(0, part)) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-			puts("MMC partition switch failed\n");
+			puts("spl: mmc partition switch failed\n");
 #endif
 			hang();
 		}
+
 #ifdef CONFIG_SPL_OS_BOOT
-		if (spl_start_uboot() || mmc_load_image_raw_os(mmc))
+		if (!spl_start_uboot())
+			err = mmc_load_image_raw_os(mmc);
+		else
 #endif
 		err = mmc_load_image_raw_sector(mmc,
 			CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR);
+		break;
 #endif
-	}
-
-	switch(boot_mode){
-		case MMCSD_MODE_RAW:
-#if defined(CONFIG_SPL_FAT_SUPPORT) || defined(CONFIG_SPL_EXT_SUPPORT)
-		case MMCSD_MODE_FS:
-#endif
-#ifdef CONFIG_SUPPORT_EMMC_BOOT
-		case MMCSD_MODE_EMMCBOOT:
-#endif
-			/* Boot mode is ok. Nothing to do. */
-			break;
-		case MMCSD_MODE_UNDEFINED:
-		default:
+	case MMCSD_MODE_UNDEFINED:
+	default:
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-			puts("spl: wrong MMC boot mode\n");
+		puts("spl: mmc: wrong boot mode\n");
 #endif
-			hang();
+		hang();
 	}
 
 	if (err)