Re: [PATCH v2 1/3] imx8mp-evk: Add support for booting via USB

[Lucas Stach <l.stach@pengutronix.de>]

Am Freitag, dem 13.08.2021 um 21:26 +0200 schrieb Ahmad Fatoum:
> On 13.08.21 17:22, Uwe Kleine-König wrote:
> 
> S-o-b missing.
> 
> > ---
> >  arch/arm/boards/nxp-imx8mp-evk/lowlevel.c | 27 +++++++++++++++++++++++
> >  arch/arm/mach-imx/boot.c                  |  4 +++-
> >  include/asm-generic/sections.h            |  1 +
> >  3 files changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> > index 3298ded5866d..1fb7899198d6 100644
> > --- a/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> > +++ b/arch/arm/boards/nxp-imx8mp-evk/lowlevel.c
> > @@ -99,6 +99,30 @@ static int power_init_board(void)
> >  	return 0;
> >  }
> >  
> > +/* read piggydata via a bootrom callback and place it behind our copy in SDRAM */
> > +static int imx8m_bootrom_load_image(void)
> > +{
> > +	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor) = *(void **)0x988;
> 
> Would be nice to have this in a header, e.g.
> 
> extern struct imx8mp_bootrom_ops {
> 	/* ... */
> 	int (*download_image)(u8 *dest, u32 offset, u32 size, u32 xor);
> } *imx8mp_bootrom_ops = (void *)0x988;
> 
> > +	size_t count = __piggydata_end - __piggydata_start;
> > +	char *p = (char *)MX8M_ATF_BL33_BASE_ADDR + (__piggydata_start - __image_start);
> > +
> > +	while (count) {
> > +		size_t chunksize = min(count, (size_t)1024);
> > +		int ret;
> > +
> > +		ret = download_image(p, 0, chunksize, (uintptr_t)p ^ chunksize);
> > +		if (ret != 0xf0) {
> > +			pr_err("Failed to load piggy data (ret = %x)\n", ret);
> > +			return -EIO;
> > +		}
> > +
> > +		p += chunksize;
> > +		count -= chunksize;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
This shouldn't be in board code, but in some location where it is
reusable by other i.MX8MP boards.

> >  extern struct dram_timing_info imx8mp_evk_dram_timing;
> >  
> >  static void start_atf(void)
> > @@ -125,6 +149,9 @@ static void start_atf(void)
> >  	case BOOTSOURCE_MMC:
> >  		imx8mp_esdhc_load_image(instance, false);
> >  		break;
> > +	case BOOTSOURCE_SERIAL:
> > +		imx8m_bootrom_load_image();
> > +		break;
> >  	default:
> >  		printf("Unhandled bootsource BOOTSOURCE_%d\n", src);
> >  		hang();
> > diff --git a/arch/arm/mach-imx/boot.c b/arch/arm/mach-imx/boot.c
> > index 2b66bbf71eb1..7c1d49291045 100644
> > --- a/arch/arm/mach-imx/boot.c
> > +++ b/arch/arm/mach-imx/boot.c
> > @@ -495,10 +495,12 @@ static void __imx7_get_boot_source(enum bootsource *src, int *instance,
> >  	case 5:
> >  		*src = BOOTSOURCE_NOR;
> >  		break;
> > -	case 15:
> > +	case 14: /* observed on i.MX8MP for USB "serial" booting */
> > +	case 15: /* observed on i.MX8MM for USB "serial" booting */
> >  		*src = BOOTSOURCE_SERIAL;
> >  		break;
> >  	default:
> > +		*src = BOOTSOURCE_UNKNOWN;
> >  		break;
> >  	}
> >  }
> > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> > index 870bff21f668..597c4951ea5e 100644
> > --- a/include/asm-generic/sections.h
> > +++ b/include/asm-generic/sections.h
> > @@ -9,6 +9,7 @@ extern char _end[];
> >  extern char __image_start[];
> >  extern char __image_end[];
> >  extern char __piggydata_start[];
> > +extern char __piggydata_end[];
> 
> Other code normally uses __image_end, but it seems __piggydata_end should be
> equal to it. I'd prefer __image_end, because conceptually, you want to copy
> off the rest of the image (which happens to be just the piggy data)
> 
> @Rouven, The sha sum is at the end of the image, right?
> Would this be included this way?

The checksum is built into the PBL, as it is used to authenticate the
piggydata, as the BootROM will only authenticate the initial loaded
part of the image, i.e. the PBL.

Regards,
Lucas


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox