Skip to contentSkip to conversation

env_mmc: support env partition setup in runtime

PATCH Igor Grinberg 1 participant 2 messages Jul 3, 2014 – Jul 15, 2014 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Igor Grinberg

author
#1 lore raw

Hi Pantelis, Tom,

Apparently, Dmitry has sent the message in html format...

Resending now...
Sorry for that...

-------- Original Message --------
Subject: Re: [U-Boot] [PATCH 2/3] env_mmc: support env partition setup in runtime
Date: Wed, 25 Jun 2014 10:42:11 +0300
From: Dmitry Lifshitz <[email protected]>
To: Pantelis Antoniou <[email protected]>
CC: u-boot at lists.denx.de, Tom Rini <[email protected]>, Igor Grinberg <[email protected]>

Hi Pantelis,

Quoted 59 lines · Pantelis Antoniou

Hi Dmitry,

I took a good look at the patch and there's a problem.

Quoted 45 lines · Dmitry Lifshitz

Add callback with __weak annotation to allow setup of environment partition number in runtime from a board file.

Signed-off-by Dmitry Lifshitz <[email protected]>
Signed-off-by Igor Grinberg <[email protected]>

---
common/env_mmc.c | 35 ++++++++++++++++++++++++++---------

1 files changed, 26 insertions(+), 9 deletions(-)
common/env_mmc.c +24 -0
index 045428c..5d4b5f4 100644
@@ -62,6 +62,30 @@ int env_init(void)
 	return 0;
 }
 
+
+#ifdef CONFIG_SYS_MMC_ENV_PART
+__weak uint mmc_get_env_part(struct mmc *mmc)
+{
+	return CONFIG_SYS_MMC_ENV_PART;
+}
+
+static int mmc_set_env_part(struct mmc *mmc)
+{
+	uint part = mmc_get_env_part(mmc);
+
+	if (part != mmc->part_num) {
+		if (mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV, part)) {
+			puts("MMC partition switch failed\n");
+			return -1;
+		}
+	}
+
+	return 0;
+}
+#else
+static inline int mmc_set_env_part(struct mmc *mmc) {return 0; };
+#endif
+
 static int init_mmc_for_env(struct mmc *mmc)
 {
 	if (!mmc) {

common/env_mmc.c static int init_mmc_for_env(struct mmc *mmc) · line 100

Quoted 3 lines
common/env_mmc.c +0 -0
@@ -74,15 +98,8 @@ static int init_mmc_for_env(struct mmc *mmc)
 		return -1;
 	}

Just before this hunk, we have this:

Quoted 3 lines
common/env_mmc.c +0 -0
 #ifdef CONFIG_SYS_MMC_ENV_PART
        int dev = CONFIG_SYS_MMC_ENV_DEV;
Quoted 6 lines
Quoted 5 lines
common/env_mmc.c +0 -0
 #ifdef CONFIG_SPL_BUILD
        dev = 0;

#endif
#endif

This appears to be broken for SPL in case that CONFIG_SYS_MMC_ENV_DEV is not 0.

Exactly as it was broken before the patch, right?

Quoted 5 lines

SPL hardcoded dev to 0, while mmc_switch_part is implicitly operating on CONFIG_SYS_MMC_ENV_DEV.

Please rework it so that the SPL case is unaffected.

This patch does not change the behavior and its purpose is not fixing the current code.

The bug describe can be fixed by later patch and possibly will require additional review and testing.

Given that this patch does not change the behavior, can it be accepted please ?

Regards,

Dmitry

Quoted 18 lines
Quoted 16 lines
common/env_mmc.c +2 -9
-#ifdef CONFIG_SYS_MMC_ENV_PART
-	if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) {
-		if (mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV,
-				    CONFIG_SYS_MMC_ENV_PART)) {
-			puts("MMC partition switch failed\n");
-			return -1;
-		}
-	}
-#endif
+	if (mmc_set_env_part(mmc))
+		return -1;

	return 0;
}
Signature · 1 line
1.7.5.4
author
#2 lore raw

ping.

Quoted 132 lines · Igor Grinberg

Hi Pantelis, Tom,

Apparently, Dmitry has sent the message in html format...

Resending now...
Sorry for that...

-------- Original Message --------
Subject: Re: [U-Boot] [PATCH 2/3] env_mmc: support env partition setup in runtime
Date: Wed, 25 Jun 2014 10:42:11 +0300
From: Dmitry Lifshitz <[email protected]>
To: Pantelis Antoniou <[email protected]>
CC: u-boot at lists.denx.de, Tom Rini <[email protected]>, Igor Grinberg <[email protected]>

Hi Pantelis,

Quoted 59 lines · Pantelis Antoniou

Hi Dmitry,

I took a good look at the patch and there's a problem.

Quoted 45 lines · Dmitry Lifshitz

Add callback with __weak annotation to allow setup of environment partition number in runtime from a board file.

Signed-off-by Dmitry Lifshitz <[email protected]>
Signed-off-by Igor Grinberg <[email protected]>

---
common/env_mmc.c | 35 ++++++++++++++++++++++++++---------

1 files changed, 26 insertions(+), 9 deletions(-)
common/env_mmc.c +24 -0
index 045428c..5d4b5f4 100644
@@ -62,6 +62,30 @@ int env_init(void)
 	return 0;
 }
 
+
+#ifdef CONFIG_SYS_MMC_ENV_PART
+__weak uint mmc_get_env_part(struct mmc *mmc)
+{
+	return CONFIG_SYS_MMC_ENV_PART;
+}
+
+static int mmc_set_env_part(struct mmc *mmc)
+{
+	uint part = mmc_get_env_part(mmc);
+
+	if (part != mmc->part_num) {
+		if (mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV, part)) {
+			puts("MMC partition switch failed\n");
+			return -1;
+		}
+	}
+
+	return 0;
+}
+#else
+static inline int mmc_set_env_part(struct mmc *mmc) {return 0; };
+#endif
+
 static int init_mmc_for_env(struct mmc *mmc)
 {
 	if (!mmc) {

common/env_mmc.c static int init_mmc_for_env(struct mmc *mmc) · line 100

Quoted 3 lines
common/env_mmc.c +0 -0
@@ -74,15 +98,8 @@ static int init_mmc_for_env(struct mmc *mmc)
 		return -1;
 	}

Just before this hunk, we have this:

Quoted 3 lines
common/env_mmc.c +0 -0
 #ifdef CONFIG_SYS_MMC_ENV_PART
        int dev = CONFIG_SYS_MMC_ENV_DEV;
Quoted 6 lines
Quoted 5 lines
common/env_mmc.c +0 -0
 #ifdef CONFIG_SPL_BUILD
        dev = 0;

#endif
#endif

This appears to be broken for SPL in case that CONFIG_SYS_MMC_ENV_DEV is not 0.

Exactly as it was broken before the patch, right?

Quoted 5 lines

SPL hardcoded dev to 0, while mmc_switch_part is implicitly operating on CONFIG_SYS_MMC_ENV_DEV.

Please rework it so that the SPL case is unaffected.

This patch does not change the behavior and its purpose is not fixing the current code.

The bug describe can be fixed by later patch and possibly will require additional review and testing.

Given that this patch does not change the behavior, can it be accepted please ?

Regards,

Dmitry

Quoted 12 lines
Quoted 12 lines
common/env_mmc.c +2 -9
-#ifdef CONFIG_SYS_MMC_ENV_PART
-	if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) {
-		if (mmc_switch_part(CONFIG_SYS_MMC_ENV_DEV,
-				    CONFIG_SYS_MMC_ENV_PART)) {
-			puts("MMC partition switch failed\n");
-			return -1;
-		}
-	}
-#endif
+	if (mmc_set_env_part(mmc))
+		return -1;
Quoted 6 lines
Quoted 4 lines
	return 0;
}
Signature · 1 line
1.7.5.4

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Signature · 2 lines
Regards,
Igor.