diff mbox series

[U-Boot] imx: imx8: fix loading container image from eMMC boot partitions

Message ID 20191021152154.15385-1-agust@denx.de
State Superseded
Delegated to: Stefano Babic
Headers show
Series [U-Boot] imx: imx8: fix loading container image from eMMC boot partitions | expand

Commit Message

Anatolij Gustschin Oct. 21, 2019, 3:21 p.m. UTC 
Booting with images in eMMC hardware boot partition doesn't work
because the container header is loaded from user partition, thus
the loaded data doesn't have a valid header. Add partition switching
to support booting from eMMC boot partitions.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
 arch/arm/mach-imx/imx8/image.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Peng Fan Oct. 22, 2019, 2:43 a.m. UTC | #1 
Hi Anatolij,

> -----Original Message-----
> From: Anatolij Gustschin <agust@denx.de>
> Sent: 2019年10月21日 23:22
> To: u-boot@lists.denx.de
> Cc: sbabic@denx.de; festevam@gmail.com; dl-uboot-imx
> <uboot-imx@nxp.com>; Ye Li <ye.li@nxp.com>
> Subject: [PATCH] imx: imx8: fix loading container image from eMMC boot
> partitions
> 
> Booting with images in eMMC hardware boot partition doesn't work because
> the container header is loaded from user partition, thus the loaded data
> doesn't have a valid header. Add partition switching to support booting from
> eMMC boot partitions.

get_boot_device_offset already switched partition, there is no need to add
partition switch logic in get_container_size.

Regards,
Peng.

> 
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
>  arch/arm/mach-imx/imx8/image.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-imx/imx8/image.c
> b/arch/arm/mach-imx/imx8/image.c index 58a29e8..b0e9494 100644
> --- a/arch/arm/mach-imx/imx8/image.c
> +++ b/arch/arm/mach-imx/imx8/image.c
> @@ -72,7 +72,20 @@ static int get_container_size(void *dev, int dev_type,
> unsigned long offset)
>  	if (dev_type == MMC_DEV) {
>  		unsigned long count = 0;
>  		struct mmc *mmc = (struct mmc *)dev;
> -
> +		u8 part;
> +
> +		part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> +		if (part == 7)
> +			part = 0;
> +		if (part == 1 || part == 2) {
> +			if (CONFIG_IS_ENABLED(MMC_TINY))
> +				ret = mmc_switch_part(mmc, part);
> +			else
> +				ret = blk_dselect_hwpart(mmc_get_blk_desc(mmc),
> +							 part);
> +			if (ret)
> +				return ret;
> +		}
>  		count = blk_dread(mmc_get_blk_desc(mmc),
>  				  offset / mmc->read_bl_len,
>  				  CONTAINER_HDR_ALIGNMENT / mmc->read_bl_len,
> --
> 2.7.4
Anatolij Gustschin Oct. 22, 2019, 7:34 a.m. UTC | #2 
Hi Peng,

On Tue, 22 Oct 2019 02:43:31 +0000
Peng Fan peng.fan@nxp.com wrote:
...
> get_boot_device_offset already switched partition, there is no need to add
> partition switch logic in get_container_size.

I'd be very happy if it did, but it didn't, at least not in current mainline
tree. I can't find eMMC hw partition switching code in u-boot-imx tree either.
I've checked in current master and in next branches:

 https://github.com/sbabic/u-boot-imx/blob/master/arch/arm/mach-imx/imx8/image.c#L123
 https://github.com/sbabic/u-boot-imx/blob/next/arch/arm/mach-imx/imx8/image.c#L123

I'll re-spin the patch to add it in spl_mmc_get_uboot_raw_sector(), this
seems to be a better place for it.

--
Anatolij
Peng Fan Oct. 22, 2019, 8:13 a.m. UTC | #3 
Hi Anatolij,

> Subject: Re: [PATCH] imx: imx8: fix loading container image from eMMC boot
> partitions
> 
> Hi Peng,
> 
> On Tue, 22 Oct 2019 02:43:31 +0000
> Peng Fan peng.fan@nxp.com wrote:
> ...
> > get_boot_device_offset already switched partition, there is no need to
> > add partition switch logic in get_container_size.
> 
> I'd be very happy if it did, but it didn't, at least not in current mainline tree. I
> can't find eMMC hw partition switching code in u-boot-imx tree either.
> I've checked in current master and in next branches:

My bad. I looked wrong. There is no such code.

But I do not understand well. When burn flash.bin into boot partition,
SPL will be loaded from boot partition, SPL will load container images
from boot partition, but in your case, I think SPL will load container images
from user partition, so you switch back to boot partition.
From common/spl/spl_mmc.c, there is boot partition switch there,
Why need to add such logic in imx8/image.c?

Thanks,
Peng.
> 
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Fsbabic%2Fu-boot-imx%2Fblob%2Fmaster%2Farch%2Farm%2Fmach-i
> mx%2Fimx8%2Fimage.c%23L123&amp;data=02%7C01%7Cpeng.fan%40nxp.c
> om%7Cf5d635a085054823ffe008d756c24582%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C637073264694636512&amp;sdata=Td3sdxz67eX
> EFuEMXzM2A3Dtm1OXRsR8m9cKKxo52UE%3D&amp;reserved=0
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Fsbabic%2Fu-boot-imx%2Fblob%2Fnext%2Farch%2Farm%2Fmach-im
> x%2Fimx8%2Fimage.c%23L123&amp;data=02%7C01%7Cpeng.fan%40nxp.co
> m%7Cf5d635a085054823ffe008d756c24582%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C637073264694636512&amp;sdata=TjcYIyo%2BO6J
> B4IHv5%2F5p89BOWh2EPnSu%2Fe%2BKEGg0brY%3D&amp;reserved=0
> 
> I'll re-spin the patch to add it in spl_mmc_get_uboot_raw_sector(), this seems
> to be a better place for it.
> 
> --
> Anatolij
Anatolij Gustschin Oct. 22, 2019, 9:49 a.m. UTC | #4 
Hi Peng,

On Tue, 22 Oct 2019 08:13:31 +0000
Peng Fan peng.fan@nxp.com wrote:
...
> But I do not understand well. When burn flash.bin into boot partition,
> SPL will be loaded from boot partition,

yes, correct. On my test board flash.bin is in the first boot partition
and SPL is loaded as expected.

> SPL will load container images
> from boot partition,

SPL switches to user partition during the inif of mmc driver:
  spl_mmc_load() -> mmc_init()

Then it tries to load the container image in spl_mmc_get_uboot_raw_sector(),
but here it reads the data from user partition and the header is not
recognized as a valid header.

> but in your case, I think SPL will load container images
> from user partition, so you switch back to boot partition.

yes, because the mmc init switches to user partition.

> From common/spl/spl_mmc.c, there is boot partition switch there,
> Why need to add such logic in imx8/image.c?

spl_mmc_load() reads the container header before partition
switching.

--
Anatolij
Peng Fan Oct. 22, 2019, 10:08 a.m. UTC | #5 
> Subject: Re: [PATCH] imx: imx8: fix loading container image from eMMC boot
> partitions
> 
> Hi Peng,
> 
> On Tue, 22 Oct 2019 08:13:31 +0000
> Peng Fan peng.fan@nxp.com wrote:
> ...
> > But I do not understand well. When burn flash.bin into boot partition,
> > SPL will be loaded from boot partition,
> 
> yes, correct. On my test board flash.bin is in the first boot partition and SPL is
> loaded as expected.
> 
> > SPL will load container images
> > from boot partition,
> 
> SPL switches to user partition during the inif of mmc driver:
>   spl_mmc_load() -> mmc_init()
> 
> Then it tries to load the container image in spl_mmc_get_uboot_raw_sector(),
> but here it reads the data from user partition and the header is not recognized
> as a valid header.
> 
> > but in your case, I think SPL will load container images from user
> > partition, so you switch back to boot partition.
> 
> yes, because the mmc init switches to user partition.
> 
> > From common/spl/spl_mmc.c, there is boot partition switch there, Why
> > need to add such logic in imx8/image.c?
> 
> spl_mmc_load() reads the container header before partition switching.

Oh. Indeeded. Not took care of emmc when adding spl_mmc_get_uboot_raw_sector (:
Thanks.

Thanks,
Peng.

> 
> --
> Anatolij
Anatolij Gustschin Oct. 22, 2019, 10:16 a.m. UTC | #6 
On Tue, 22 Oct 2019 10:08:42 +0000
Peng Fan peng.fan@nxp.com wrote:
... 
> > > From common/spl/spl_mmc.c, there is boot partition switch there, Why
> > > need to add such logic in imx8/image.c?  
> > 
> > spl_mmc_load() reads the container header before partition switching.  
> 
> Oh. Indeeded. Not took care of emmc when adding spl_mmc_get_uboot_raw_sector (:

I think we should avoid adding partition switching in get_container_size(),
it looks like container header is read several times which is suboptimal.
I'll have to rethink this patch, don't have time for this now.

--
Anatolij
Peng Fan Oct. 22, 2019, 10:18 a.m. UTC | #7 
> Subject: Re: [PATCH] imx: imx8: fix loading container image from eMMC boot
> partitions
> 
> On Tue, 22 Oct 2019 10:08:42 +0000
> Peng Fan peng.fan@nxp.com wrote:
> ...
> > > > From common/spl/spl_mmc.c, there is boot partition switch there,
> > > > Why need to add such logic in imx8/image.c?
> > >
> > > spl_mmc_load() reads the container header before partition switching.
> >
> > Oh. Indeeded. Not took care of emmc when adding
> spl_mmc_get_uboot_raw_sector (:
> 
> I think we should avoid adding partition switching in get_container_size(), it
> looks like container header is read several times which is suboptimal.
> I'll have to rethink this patch, don't have time for this now.

ok. I'll give a look, if you not mind.

Thanks,
Peng.

> 
> --
> Anatolij
Anatolij Gustschin Oct. 22, 2019, 10:19 a.m. UTC | #8 
On Tue, 22 Oct 2019 10:18:01 +0000
Peng Fan peng.fan@nxp.com wrote:
...
> ok. I'll give a look, if you not mind.

Sure, no problem. Thanks!

--
Anatolij
diff mbox series

Patch

diff --git a/arch/arm/mach-imx/imx8/image.c b/arch/arm/mach-imx/imx8/image.c
index 58a29e8..b0e9494 100644
--- a/arch/arm/mach-imx/imx8/image.c
+++ b/arch/arm/mach-imx/imx8/image.c
@@ -72,7 +72,20 @@  static int get_container_size(void *dev, int dev_type, unsigned long offset)
 	if (dev_type == MMC_DEV) {
 		unsigned long count = 0;
 		struct mmc *mmc = (struct mmc *)dev;
-
+		u8 part;
+
+		part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
+		if (part == 7)
+			part = 0;
+		if (part == 1 || part == 2) {
+			if (CONFIG_IS_ENABLED(MMC_TINY))
+				ret = mmc_switch_part(mmc, part);
+			else
+				ret = blk_dselect_hwpart(mmc_get_blk_desc(mmc),
+							 part);
+			if (ret)
+				return ret;
+		}
 		count = blk_dread(mmc_get_blk_desc(mmc),
 				  offset / mmc->read_bl_len,
 				  CONTAINER_HDR_ALIGNMENT / mmc->read_bl_len,