[U-Boot] [PATCH 0/3] cm_t54: add MAC address and env partiton handling

[U-Boot] [PATCH 0/3] cm_t54: add MAC address and env partiton handling

[Dmitry Lifshitz]

Add Eth MAC address handling, stored in onboard EEPROM.

cm-t54 config defines eMMC as env storage device.
cm-t54 U-Boot environment is stored in the same partition as boot loader.
It can be both - eMMC boot or user data partition.
Add support for setting environment partition number in runtime.

Dmitry Lifshitz (3):
  cm-t54: add EEPROM support and MAC address handling
  env_mmc: support env partition setup in runtime
  cm-t54: add environment partition runtime detection

 board/compulab/cm_t54/cm_t54.c |   85 ++++++++++++++++++++++++++++++++++++++++
 common/env_mmc.c               |   35 ++++++++++++----
 include/configs/cm_t54.h       |   10 +++++
 3 files changed, 121 insertions(+), 9 deletions(-)

-- 
1.7.5.4

[U-Boot] [PATCH 1/3] cm-t54: add EEPROM support and MAC address handling

[Dmitry Lifshitz]

cm-t54 Eth MAC address is stored in onboard EEPROM.
Add EEPROM support and setup stored Eth MAC address.

If EEPROM does not contain a valid MAC, then generate it from the
processor ID code (reference code is taken from OMAP5 uEvm board file).

Modify Device Tree blob MAC address field with retrieved data.

Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
Acked-by: Igor Grinberg <grinberg@compulab.co.il>
---
 board/compulab/cm_t54/cm_t54.c |   63 ++++++++++++++++++++++++++++++++++++++++
 include/configs/cm_t54.h       |    9 ++++++
 2 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/board/compulab/cm_t54/cm_t54.c b/board/compulab/cm_t54/cm_t54.c
index 1a4be72..574ced2 100644
--- a/board/compulab/cm_t54/cm_t54.c
+++ b/board/compulab/cm_t54/cm_t54.c
@@ -9,6 +9,7 @@
  */
 
 #include <common.h>
+#include <fdt_support.h>
 #include <usb.h>
 #include <mmc.h>
 #include <palmas.h>
@@ -20,6 +21,8 @@
 #include <asm/arch/ehci.h>
 #include <asm/ehci-omap.h>
 
+#include "../common/eeprom.h"
+
 #define DIE_ID_REG_BASE		(OMAP54XX_L4_CORE_BASE + 0x2000)
 #define DIE_ID_REG_OFFSET	0x200
 
@@ -99,6 +102,66 @@ int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#ifdef CONFIG_USB_HOST_ETHER
+
+void ft_board_setup(void *blob, bd_t *bd)
+{
+	uint8_t enetaddr[6];
+
+	/* MAC addr */
+	if (eth_getenv_enetaddr("usbethaddr", enetaddr)) {
+		fdt_find_and_setprop(blob, "/smsc95xx at 0", "mac-address",
+				     enetaddr, 6, 1);
+	}
+}
+
+static void generate_mac_addr(uint8_t *enetaddr)
+{
+	int reg;
+
+	reg = DIE_ID_REG_BASE + DIE_ID_REG_OFFSET;
+
+	/*
+	 * create a fake MAC address from the processor ID code.
+	 * first byte is 0x02 to signify locally administered.
+	 */
+	enetaddr[0] = 0x02;
+	enetaddr[1] = readl(reg + 0x10) & 0xff;
+	enetaddr[2] = readl(reg + 0xC) & 0xff;
+	enetaddr[3] = readl(reg + 0x8) & 0xff;
+	enetaddr[4] = readl(reg) & 0xff;
+	enetaddr[5] = (readl(reg) >> 8) & 0xff;
+}
+
+/*
+ * Routine: handle_mac_address
+ * Description: prepare MAC address for on-board Ethernet.
+ */
+static int handle_mac_address(void)
+{
+	uint8_t enetaddr[6];
+	int ret;
+
+	ret = eth_getenv_enetaddr("usbethaddr", enetaddr);
+	if (ret)
+		return 0;
+
+	ret = cl_eeprom_read_mac_addr(enetaddr);
+	if (!ret || !is_valid_ether_addr(enetaddr))
+		generate_mac_addr(enetaddr);
+
+	if (!is_valid_ether_addr(enetaddr))
+		return -1;
+
+	return eth_setenv_enetaddr("usbethaddr", enetaddr);
+}
+
+int board_eth_init(bd_t *bis)
+{
+	return handle_mac_address();
+}
+#endif
+
 #ifdef CONFIG_USB_EHCI
 static struct omap_usbhs_board_data usbhs_bdata = {
 	.port_mode[0] = OMAP_USBHS_PORT_MODE_UNUSED,
diff --git a/include/configs/cm_t54.h b/include/configs/cm_t54.h
index 3ca229b..8d474f0 100644
--- a/include/configs/cm_t54.h
+++ b/include/configs/cm_t54.h
@@ -19,6 +19,15 @@
 #undef CONFIG_MISC_INIT_R
 #undef CONFIG_SPL_OS_BOOT
 
+/* Device Tree defines */
+#define CONFIG_OF_LIBFDT
+#define CONFIG_OF_BOARD_SETUP
+
+/* EEPROM related defines */
+#define CONFIG_SYS_I2C_OMAP34XX
+#define CONFIG_SYS_I2C_EEPROM_ADDR	0x50
+#define CONFIG_SYS_I2C_EEPROM_ADDR_LEN	1
+
 /* Enable SD/MMC CD and WP GPIOs */
 #define OMAP_HSMMC_USE_GPIO
 
-- 
1.7.5.4

[U-Boot] [PATCH 2/3] env_mmc: support env partition setup in runtime

[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 <lifshitz@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 common/env_mmc.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/common/env_mmc.c b/common/env_mmc.c
index 045428c..5d4b5f4 100644
--- a/common/env_mmc.c
+++ b/common/env_mmc.c
@@ -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) {
@@ -74,15 +98,8 @@ static int init_mmc_for_env(struct mmc *mmc)
 		return -1;
 	}
 
-#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;
 }
-- 
1.7.5.4

[U-Boot] [PATCH 3/3] cm-t54: add environment partition runtime detection

[Dmitry Lifshitz]

Add environment partition runtime detection callback.

Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
Acked-by: Igor Grinberg <grinberg@compulab.co.il>
---
 board/compulab/cm_t54/cm_t54.c |   22 ++++++++++++++++++++++
 include/configs/cm_t54.h       |    1 +
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/board/compulab/cm_t54/cm_t54.c b/board/compulab/cm_t54/cm_t54.c
index 574ced2..11e77f9 100644
--- a/board/compulab/cm_t54/cm_t54.c
+++ b/board/compulab/cm_t54/cm_t54.c
@@ -13,6 +13,7 @@
 #include <usb.h>
 #include <mmc.h>
 #include <palmas.h>
+#include <spl.h>
 
 #include <asm/gpio.h>
 #include <asm/arch/sys_proto.h>
@@ -74,6 +75,27 @@ static int cm_t54_palmas_regulator_set(u8 vreg, u8 vval, u8 creg, u8 cval)
 	return 0;
 }
 
+/*
+ * Routine: mmc_get_env_part
+ * Description:  setup environment storage device partition.
+ */
+#ifdef CONFIG_SYS_MMC_ENV_PART
+uint mmc_get_env_part(struct mmc *mmc)
+{
+	u32 bootmode = gd->arch.omap_boot_params.omap_bootmode;
+	uint bootpart = CONFIG_SYS_MMC_ENV_PART;
+
+	/*
+	 * If booted from eMMC boot partition then force eMMC
+	 * FIRST boot partition to be env storage
+	 */
+	if (bootmode == BOOT_DEVICE_MMC2_2)
+		bootpart = 1;
+
+	return bootpart;
+}
+#endif
+
 #if defined(CONFIG_GENERIC_MMC) && !defined(CONFIG_SPL_BUILD)
 #define SB_T54_CD_GPIO 228
 #define SB_T54_WP_GPIO 229
diff --git a/include/configs/cm_t54.h b/include/configs/cm_t54.h
index 8d474f0..680aac3 100644
--- a/include/configs/cm_t54.h
+++ b/include/configs/cm_t54.h
@@ -49,6 +49,7 @@
 
 #define CONFIG_ENV_IS_IN_MMC
 #define CONFIG_SYS_MMC_ENV_DEV		1		/* SLOT2: eMMC(1) */
+#define CONFIG_SYS_MMC_ENV_PART		0
 #define CONFIG_ENV_OFFSET		0xc0000		/* (in bytes) 768 KB */
 #define CONFIG_ENV_SIZE			(16 << 10)	/* 16 KB */
 #define CONFIG_ENV_OFFSET_REDUND	(CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE)
-- 
1.7.5.4

[U-Boot] [U-Boot, 1/3] cm-t54: add EEPROM support and MAC address handling

[Tom Rini]

On Sun, Apr 27, 2014 at 01:18:46PM +0300, Dmitry Lifshitz wrote:

> cm-t54 Eth MAC address is stored in onboard EEPROM.
> Add EEPROM support and setup stored Eth MAC address.
> 
> If EEPROM does not contain a valid MAC, then generate it from the
> processor ID code (reference code is taken from OMAP5 uEvm board file).
> 
> Modify Device Tree blob MAC address field with retrieved data.
> 
> Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>

Applied to u-boot-ti/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140523/fcd06a30/attachment.pgp>

[U-Boot] [U-Boot, 3/3] cm-t54: add environment partition runtime detection

[Tom Rini]

On Sun, Apr 27, 2014 at 01:18:48PM +0300, Dmitry Lifshitz wrote:

> Add environment partition runtime detection callback.
> 
> Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>

Applied to u-boot-ti/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140523/871aeb41/attachment.pgp>

[U-Boot] [U-Boot, 3/3] cm-t54: add environment partition runtime detection

[Igor Grinberg]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Tom,

On 05/24/14 02:49, Tom Rini wrote:
> On Sun, Apr 27, 2014 at 01:18:48PM +0300, Dmitry Lifshitz wrote:
> 
>> Add environment partition runtime detection callback.
>>
>> Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
>> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> 
> Applied to u-boot-ti/master, thanks!

What about patch 2/3 (env_mmc: support env partition setup in runtime)?


- -- 
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJTgZptAAoJEBDE8YO64EfaFZMP/1ImzvDWXRihV5AvAF2wlLHD
vagDIcDyMq4r2OyvYNroAz55bGav4drztlXdXeSCFVx8P8ASjxP7V/3pfPWbXMxx
t1d/3Wam+u9h0YTWTT4DWcYC31IKXTNFzt1wh/Rya/QUHxxW//ZGp9nG7VAoG5sU
7YSTu37sI2zayzW7QtCJKWfsUyCrzkvMNvhApTYnLAAD0kpSGXd6oLNwZ5U37508
7myR9vY+8eDTJcYPOtjHXfCfecp2kQjPY/X9U8X5WDayXxj/ItMw+GiJVqbN2LJF
K98F9SU6lGleSC7zkpQ8jb3q9rqtZp4EVYS6kP7nIRwsXUAjTtNgXH3uBAVxWKkN
Z3+/zjuL0OWtURPAHzKLE6wkYnYVQAsnA/2rDisonr5nZHTi25NucvYLpzr2EzBI
/yXrmsxuVZTqmaV50F0Tjm2Ba8dFd9FFXxDltTuTo4lj+VMJ7QifJikklwf4ejgV
eQnHuMhi5kaImYbSTFswwNzL3206dkKNiUYlHJWEkW5UMEwlQzcPWzQ+H6QoxW0e
Cn1b0Tb1spH3f3rO/4rYHyu2ovKtSH1CSOuPckIeR38WyNZq9kMQgkZif4sd2ZTP
V7dd68u1F81k6pPKgAfsdRN5lNoY59UZqsAmIGFfU78iEQTsX/oSiBaWxe8umDc8
JKJnkexMI5cmcJU1I3uT
=p3sC
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 2/3] env_mmc: support env partition setup in runtime

[Igor Grinberg]

ping..

On 04/27/14 13:18, Dmitry Lifshitz wrote:
> Add callback with __weak annotation to allow setup of environment
> partition number in runtime from a board file.
> 
> Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>  common/env_mmc.c |   35 ++++++++++++++++++++++++++---------
>  1 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/common/env_mmc.c b/common/env_mmc.c
> index 045428c..5d4b5f4 100644
> --- a/common/env_mmc.c
> +++ b/common/env_mmc.c
> @@ -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) {
> @@ -74,15 +98,8 @@ static int init_mmc_for_env(struct mmc *mmc)
>  		return -1;
>  	}
>  
> -#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;
>  }
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 2/3] env_mmc: support env partition setup in runtime

[Tom Rini]

On Sun, May 25, 2014 at 10:23:46AM +0300, Igor Grinberg wrote:

> ping..
> 
> On 04/27/14 13:18, Dmitry Lifshitz wrote:
> > Add callback with __weak annotation to allow setup of environment
> > partition number in runtime from a board file.
> > 
> > Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
> > Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> > ---
> >  common/env_mmc.c |   35 ++++++++++++++++++++++++++---------
> >  1 files changed, 26 insertions(+), 9 deletions(-)
> > 
> > diff --git a/common/env_mmc.c b/common/env_mmc.c
> > index 045428c..5d4b5f4 100644
> > --- a/common/env_mmc.c
> > +++ b/common/env_mmc.c
> > @@ -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) {
> > @@ -74,15 +98,8 @@ static int init_mmc_for_env(struct mmc *mmc)
> >  		return -1;
> >  	}
> >  
> > -#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;
> >  }
> > 

Pantelis, weren't you going to pick this up?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140530/0a40b835/attachment.pgp>

[U-Boot] [PATCH 2/3] env_mmc: support env partition setup in runtime

[Igor Grinberg]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Tom,

rc3 is out...

On 05/30/14 21:56, Tom Rini wrote:
> On Sun, May 25, 2014 at 10:23:46AM +0300, Igor Grinberg wrote:
> 
>> ping..
>>
>> On 04/27/14 13:18, Dmitry Lifshitz wrote:
>>> Add callback with __weak annotation to allow setup of environment
>>> partition number in runtime from a board file.
>>>
>>> Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
>>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>

[...]

> 
> Pantelis, weren't you going to pick this up?  Thanks!

Can we please have this in for 2014.07?

Thanks!

- -- 
Regards,
Igor.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJTl/xzAAoJEBDE8YO64EfayCkQAJ0UFo/F6DebROHb3fU7u5/J
iW1zRT7i1quZU5vnSa5E1I8vJ7O7IdWErrvgD6wweFjy03ueaE/m3uHXhQnNW4NC
Kgmfr28wCnlHVEJX0RA7Ei0tTbziZTCgZ5+D7xyya/CH+wpDBXw57609XG/cVwH+
JXhKlDPLOyp74kLxwPWT4jJ8ZA/GEZWL97VGWA62OQI5KEh2lASNCjHm9U5bqLPf
6Jl6F57VEsEWyCzz0DrrNZFHeyxIM45o6aUnwz4BrZAZZ+UwXnRkuVPSwKVGlGYc
+TaMjsu3XNV14MtPh5M6tbSs6mpOGHAW20ik0SwT72v4hh7SV4RyWdCY9JbKdKRG
2mEocMs7EFucrNjp0G8YD6U93b9BtTPSpiSnzaBnpECk3GTZQymsSVBn8J4qqoqK
Ucp59mrZc5rFJXU+hCWFcGAdGqfUOuIhX6xPLd0AQlfHgswrEqJ7GBEvZEDLOnnk
wU68HpNs0tW863BYfdabZnsj5BK51NlqIOAI86zCfywDmdIgtZtse7AtlJD5J9Lt
uWLGfxScppTErM8vXRBN/p2YaoCbaLiV/ur/JO4gQNrw5PSPNUUKiooABfHn9HCg
2aABhEiuloslrkgKQKx5TEKT7skVqkbRnzkBD81E/1kasx0o/dEGiADxWiIXdTZw
G1PXeefGklZlCjp8/h2I
=YCwX
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 2/3] env_mmc: support env partition setup in runtime

[Pantelis Antoniou]

Hi Dmitry,

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

On Apr 27, 2014, at 1:18 PM, Dmitry Lifshitz wrote:

> Add callback with __weak annotation to allow setup of environment
> partition number in runtime from a board file.
> 
> Signed-off-by: Dmitry Lifshitz <lifshitz@compulab.co.il>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
> common/env_mmc.c |   35 ++++++++++++++++++++++++++---------
> 1 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/common/env_mmc.c b/common/env_mmc.c
> index 045428c..5d4b5f4 100644
> --- a/common/env_mmc.c
> +++ b/common/env_mmc.c
> @@ -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) {
> @@ -74,15 +98,8 @@ static int init_mmc_for_env(struct mmc *mmc)
> 		return -1;
> 	}
> 

Just before this hunk, we have this:

> #ifdef CONFIG_SYS_MMC_ENV_PART
>         int dev = CONFIG_SYS_MMC_ENV_DEV;
> 
> #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.

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.

> -#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;
> }
> -- 
> 1.7.5.4

Regards

-- Pantelis

[U-Boot] [PATCH 2/3] env_mmc: support env partition setup in runtime

[Dmitry Lifshitz]

Hi Pantelis,

On 06/12/2014 06:10 PM, Pantelis Antoniou wrote:
> Hi Dmitry,
>
> I took a good look at the patch and there's a problem.
>
> On Apr 27, 2014, at 1:18 PM, Dmitry Lifshitz wrote:
>
>> Add callback with __weak annotation to allow setup of environment
>> partition number in runtime from a board file.
>>
>> Signed-off-by: Dmitry Lifshitz<lifshitz@compulab.co.il>
>> Signed-off-by: Igor Grinberg<grinberg@compulab.co.il>
>> ---
>> common/env_mmc.c |   35 ++++++++++++++++++++++++++---------
>> 1 files changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/common/env_mmc.c b/common/env_mmc.c
>> index 045428c..5d4b5f4 100644
>> --- a/common/env_mmc.c
>> +++ b/common/env_mmc.c
>> @@ -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) {
>> @@ -74,15 +98,8 @@ static int init_mmc_for_env(struct mmc *mmc)
>> 		return -1;
>> 	}
>>
> Just before this hunk, we have this:
>
>> #ifdef CONFIG_SYS_MMC_ENV_PART
>>          int dev = CONFIG_SYS_MMC_ENV_DEV;
>>
>> #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?

> 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

>> -#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;
>> }
>> -- 
>> 1.7.5.4
>