* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
@ 2018-08-14 9:27 Marek Vasut
2018-08-14 9:51 ` Simon Goldschmidt
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Marek Vasut @ 2018-08-14 9:27 UTC (permalink / raw)
To: u-boot
The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
is available and can be corrupted by loading ie. uImage or fitImage
headers there. Sometimes it could be beneficial to load the headers
elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
still want to parse the image headers in some local onchip memory to
ie. extract firmware from that image.
Add the possibility to override the location where the headers get
loaded by introducing new function, spl_get_load_buffer() which takes
two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
data that are to be loaded there -- and returns a valid buffer address
or hangs the system. The default behavior is the same as before, add
the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
override the weak spl_get_load_buffer() function though.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Tom Rini <trini@konsulko.com>
---
V2: Fix build issues on multiple boards due to incorrect pointer casting
---
common/spl/spl.c | 5 +++++
common/spl/spl_ext.c | 3 +--
common/spl/spl_fat.c | 3 +--
common/spl/spl_fit.c | 6 +++---
common/spl/spl_mmc.c | 6 +++---
common/spl/spl_nand.c | 4 ++--
common/spl/spl_onenand.c | 3 +--
common/spl/spl_ram.c | 5 +++--
common/spl/spl_spi.c | 3 +--
common/spl/spl_ubi.c | 3 +--
include/spl.h | 9 +++++++++
11 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/common/spl/spl.c b/common/spl/spl.c
index eda84d0c74..7c22d0f44d 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -127,6 +127,11 @@ __weak void spl_board_prepare_for_boot(void)
/* Nothing to do! */
}
+__weak struct image_header *spl_get_load_buffer(ssize_t offset, size_t size)
+{
+ return (struct image_header *)(CONFIG_SYS_TEXT_BASE + offset);
+}
+
void spl_set_header_raw_uboot(struct spl_image_info *spl_image)
{
ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos);
diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
index fd30a61f9a..fe05223605 100644
--- a/common/spl/spl_ext.c
+++ b/common/spl/spl_ext.c
@@ -16,8 +16,7 @@ int spl_load_image_ext(struct spl_image_info *spl_image,
loff_t filelen, actlen;
disk_partition_t part_info = {};
- header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
- sizeof(struct image_header));
+ header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
if (part_get_info(block_dev, partition, &part_info)) {
printf("spl: no partition table found\n");
diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
index 0403016bb4..163e540622 100644
--- a/common/spl/spl_fat.c
+++ b/common/spl/spl_fat.c
@@ -63,8 +63,7 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
if (err)
goto end;
- header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
- sizeof(struct image_header));
+ header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
err = file_fat_read(filename, header, sizeof(struct image_header));
if (err <= 0)
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index 9eabb1c105..f08e5018c3 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -357,7 +357,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
struct spl_image_info image_info;
int node = -1;
int images, ret;
- int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
+ int base_offset, hsize, align_len = ARCH_DMA_MINALIGN - 1;
int index = 0;
/*
@@ -386,8 +386,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
* For FIT with data embedded, data is loaded as part of FIT image.
* For FIT with external data, data is not loaded in this step.
*/
- fit = (void *)((CONFIG_SYS_TEXT_BASE - size - info->bl_len -
- align_len) & ~align_len);
+ hsize = (size + info->bl_len + align_len) & ~align_len;
+ fit = spl_get_load_buffer(-hsize, hsize);
sectors = get_aligned_image_size(info, size, 0);
count = info->read(info, sector, sectors, fit);
debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu\n",
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index 0b2f059570..75c41598e6 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -55,13 +55,13 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
{
unsigned long count;
struct image_header *header;
+ struct blk_desc *bd = mmc_get_blk_desc(mmc);
int ret = 0;
- header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
- sizeof(struct image_header));
+ header = spl_get_load_buffer(-sizeof(*header), bd->blksz);
/* read image header to find the image size & load address */
- count = blk_dread(mmc_get_blk_desc(mmc), sector, 1, header);
+ count = blk_dread(bd, sector, 1, header);
debug("hdr read sector %lx, count=%lu\n", sector, count);
if (count == 0) {
ret = -EIO;
diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
index 2722fd3860..6eb190f1ea 100644
--- a/common/spl/spl_nand.c
+++ b/common/spl/spl_nand.c
@@ -83,8 +83,8 @@ static int spl_nand_load_image(struct spl_image_info *spl_image,
#endif
nand_init();
- /*use CONFIG_SYS_TEXT_BASE as temporary storage area */
- header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
+ header = spl_get_load_buffer(0, sizeof(*header));
+
#ifdef CONFIG_SPL_OS_BOOT
if (!spl_start_uboot()) {
/*
diff --git a/common/spl/spl_onenand.c b/common/spl/spl_onenand.c
index d32333935a..ee30f328e6 100644
--- a/common/spl/spl_onenand.c
+++ b/common/spl/spl_onenand.c
@@ -21,8 +21,7 @@ static int spl_onenand_load_image(struct spl_image_info *spl_image,
debug("spl: onenand\n");
- /*use CONFIG_SYS_TEXT_BASE as temporary storage area */
- header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
+ header = spl_get_load_buffer(0, CONFIG_SYS_ONENAND_PAGE_SIZE);
/* Load u-boot */
onenand_spl_load_image(CONFIG_SYS_ONENAND_U_BOOT_OFFS,
CONFIG_SYS_ONENAND_PAGE_SIZE, (void *)header);
diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
index e594beaeaa..619b39a537 100644
--- a/common/spl/spl_ram.c
+++ b/common/spl/spl_ram.c
@@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
* No binman support or no information. For now, fix it
* to the address pointed to by U-Boot.
*/
- u_boot_pos = CONFIG_SYS_TEXT_BASE -
- sizeof(struct image_header);
+ header = spl_get_load_buffer(-sizeof(*header),
+ sizeof(*header));
+
}
header = (struct image_header *)map_sysmem(u_boot_pos, 0);
diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
index ba60a3a3c5..e10cf0124f 100644
--- a/common/spl/spl_spi.c
+++ b/common/spl/spl_spi.c
@@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
return -ENODEV;
}
- /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
- header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
+ header = spl_get_load_buffer(-sizeof(*header), 0x40);
#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
payload_offs = fdtdec_get_config_int(gd->fdt_blob,
diff --git a/common/spl/spl_ubi.c b/common/spl/spl_ubi.c
index a7939e9030..67e5fadd7c 100644
--- a/common/spl/spl_ubi.c
+++ b/common/spl/spl_ubi.c
@@ -61,8 +61,7 @@ int spl_ubi_load_image(struct spl_image_info *spl_image,
puts("Loading Linux failed, falling back to U-Boot.\n");
}
#endif
- header = (struct image_header *)
- (CONFIG_SYS_TEXT_BASE - sizeof(struct image_header));
+ header = spl_get_load_buffer(-sizeof(*header), sizeof(header));
volumes[0].vol_id = CONFIG_SPL_UBI_LOAD_MONITOR_ID;
volumes[0].load_addr = (void *)header;
diff --git a/include/spl.h b/include/spl.h
index 7fad62c043..b42683c9e7 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -303,4 +303,13 @@ void board_return_to_bootrom(void);
* the boot-payload
*/
void spl_perform_fixups(struct spl_image_info *spl_image);
+
+/*
+ * spl_get_load_buffer() - get buffer for loading partial image data
+ *
+ * Returns memory area which can be populated by partial image data,
+ * ie. uImage or fitImage header.
+ */
+struct image_header *spl_get_load_buffer(ssize_t offset, size_t size);
+
#endif
--
2.16.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 9:27 [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage Marek Vasut
@ 2018-08-14 9:51 ` Simon Goldschmidt
2018-08-14 10:01 ` Marek Vasut
2018-09-26 12:42 ` [U-Boot] [U-Boot,V2] " Tom Rini
2018-10-03 13:39 ` [U-Boot] [PATCH V2] " Michal Simek
2 siblings, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-08-14 9:51 UTC (permalink / raw)
To: u-boot
On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <marex@denx.de> wrote:
>
> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
> is available and can be corrupted by loading ie. uImage or fitImage
> headers there. Sometimes it could be beneficial to load the headers
> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
> still want to parse the image headers in some local onchip memory to
> ie. extract firmware from that image.
>
> Add the possibility to override the location where the headers get
> loaded by introducing new function, spl_get_load_buffer() which takes
> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
> data that are to be loaded there -- and returns a valid buffer address
> or hangs the system. The default behavior is the same as before, add
> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
> override the weak spl_get_load_buffer() function though.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Tom Rini <trini@konsulko.com>
> ---
> V2: Fix build issues on multiple boards due to incorrect pointer casting
> ---
> common/spl/spl.c | 5 +++++
> common/spl/spl_ext.c | 3 +--
> common/spl/spl_fat.c | 3 +--
> common/spl/spl_fit.c | 6 +++---
> common/spl/spl_mmc.c | 6 +++---
> common/spl/spl_nand.c | 4 ++--
> common/spl/spl_onenand.c | 3 +--
> common/spl/spl_ram.c | 5 +++--
> common/spl/spl_spi.c | 3 +--
> common/spl/spl_ubi.c | 3 +--
> include/spl.h | 9 +++++++++
> 11 files changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index eda84d0c74..7c22d0f44d 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -127,6 +127,11 @@ __weak void spl_board_prepare_for_boot(void)
> /* Nothing to do! */
> }
>
> +__weak struct image_header *spl_get_load_buffer(ssize_t offset, size_t size)
> +{
> + return (struct image_header *)(CONFIG_SYS_TEXT_BASE + offset);
> +}
> +
> void spl_set_header_raw_uboot(struct spl_image_info *spl_image)
> {
> ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos);
> diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
> index fd30a61f9a..fe05223605 100644
> --- a/common/spl/spl_ext.c
> +++ b/common/spl/spl_ext.c
> @@ -16,8 +16,7 @@ int spl_load_image_ext(struct spl_image_info *spl_image,
> loff_t filelen, actlen;
> disk_partition_t part_info = {};
>
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
> - sizeof(struct image_header));
> + header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>
> if (part_get_info(block_dev, partition, &part_info)) {
> printf("spl: no partition table found\n");
> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> index 0403016bb4..163e540622 100644
> --- a/common/spl/spl_fat.c
> +++ b/common/spl/spl_fat.c
> @@ -63,8 +63,7 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
> if (err)
> goto end;
>
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
> - sizeof(struct image_header));
> + header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>
> err = file_fat_read(filename, header, sizeof(struct image_header));
> if (err <= 0)
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 9eabb1c105..f08e5018c3 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -357,7 +357,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
> struct spl_image_info image_info;
> int node = -1;
> int images, ret;
> - int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
> + int base_offset, hsize, align_len = ARCH_DMA_MINALIGN - 1;
> int index = 0;
>
> /*
> @@ -386,8 +386,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
> * For FIT with data embedded, data is loaded as part of FIT image.
> * For FIT with external data, data is not loaded in this step.
> */
> - fit = (void *)((CONFIG_SYS_TEXT_BASE - size - info->bl_len -
> - align_len) & ~align_len);
> + hsize = (size + info->bl_len + align_len) & ~align_len;
> + fit = spl_get_load_buffer(-hsize, hsize);
> sectors = get_aligned_image_size(info, size, 0);
> count = info->read(info, sector, sectors, fit);
> debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu\n",
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index 0b2f059570..75c41598e6 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -55,13 +55,13 @@ int mmc_load_image_raw_sector(struct spl_image_info *spl_image,
> {
> unsigned long count;
> struct image_header *header;
> + struct blk_desc *bd = mmc_get_blk_desc(mmc);
> int ret = 0;
>
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
> - sizeof(struct image_header));
> + header = spl_get_load_buffer(-sizeof(*header), bd->blksz);
>
> /* read image header to find the image size & load address */
> - count = blk_dread(mmc_get_blk_desc(mmc), sector, 1, header);
> + count = blk_dread(bd, sector, 1, header);
> debug("hdr read sector %lx, count=%lu\n", sector, count);
> if (count == 0) {
> ret = -EIO;
> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> index 2722fd3860..6eb190f1ea 100644
> --- a/common/spl/spl_nand.c
> +++ b/common/spl/spl_nand.c
> @@ -83,8 +83,8 @@ static int spl_nand_load_image(struct spl_image_info *spl_image,
> #endif
> nand_init();
>
> - /*use CONFIG_SYS_TEXT_BASE as temporary storage area */
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> + header = spl_get_load_buffer(0, sizeof(*header));
> +
> #ifdef CONFIG_SPL_OS_BOOT
> if (!spl_start_uboot()) {
> /*
> diff --git a/common/spl/spl_onenand.c b/common/spl/spl_onenand.c
> index d32333935a..ee30f328e6 100644
> --- a/common/spl/spl_onenand.c
> +++ b/common/spl/spl_onenand.c
> @@ -21,8 +21,7 @@ static int spl_onenand_load_image(struct spl_image_info *spl_image,
>
> debug("spl: onenand\n");
>
> - /*use CONFIG_SYS_TEXT_BASE as temporary storage area */
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> + header = spl_get_load_buffer(0, CONFIG_SYS_ONENAND_PAGE_SIZE);
> /* Load u-boot */
> onenand_spl_load_image(CONFIG_SYS_ONENAND_U_BOOT_OFFS,
> CONFIG_SYS_ONENAND_PAGE_SIZE, (void *)header);
> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
> index e594beaeaa..619b39a537 100644
> --- a/common/spl/spl_ram.c
> +++ b/common/spl/spl_ram.c
> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
> * No binman support or no information. For now, fix it
> * to the address pointed to by U-Boot.
> */
> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
> - sizeof(struct image_header);
> + header = spl_get_load_buffer(-sizeof(*header),
> + sizeof(*header));
> +
Using "spl_get_load_buffer()" here seems to be a bit misleading, as
the address is used for "execute-in-place", not for loading.
> }
> header = (struct image_header *)map_sysmem(u_boot_pos, 0);
>
> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> index ba60a3a3c5..e10cf0124f 100644
> --- a/common/spl/spl_spi.c
> +++ b/common/spl/spl_spi.c
> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
> return -ENODEV;
> }
>
> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
Shouldn't the first argument be 0 here instead of -sizeof(*header)?
>
> #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
> payload_offs = fdtdec_get_config_int(gd->fdt_blob,
> diff --git a/common/spl/spl_ubi.c b/common/spl/spl_ubi.c
> index a7939e9030..67e5fadd7c 100644
> --- a/common/spl/spl_ubi.c
> +++ b/common/spl/spl_ubi.c
> @@ -61,8 +61,7 @@ int spl_ubi_load_image(struct spl_image_info *spl_image,
> puts("Loading Linux failed, falling back to U-Boot.\n");
> }
> #endif
> - header = (struct image_header *)
> - (CONFIG_SYS_TEXT_BASE - sizeof(struct image_header));
> + header = spl_get_load_buffer(-sizeof(*header), sizeof(header));
> volumes[0].vol_id = CONFIG_SPL_UBI_LOAD_MONITOR_ID;
> volumes[0].load_addr = (void *)header;
>
> diff --git a/include/spl.h b/include/spl.h
> index 7fad62c043..b42683c9e7 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -303,4 +303,13 @@ void board_return_to_bootrom(void);
> * the boot-payload
> */
> void spl_perform_fixups(struct spl_image_info *spl_image);
> +
> +/*
> + * spl_get_load_buffer() - get buffer for loading partial image data
> + *
> + * Returns memory area which can be populated by partial image data,
> + * ie. uImage or fitImage header.
> + */
> +struct image_header *spl_get_load_buffer(ssize_t offset, size_t size);
> +
> #endif
> --
> 2.16.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 9:51 ` Simon Goldschmidt
@ 2018-08-14 10:01 ` Marek Vasut
2018-08-14 10:22 ` Simon Goldschmidt
0 siblings, 1 reply; 16+ messages in thread
From: Marek Vasut @ 2018-08-14 10:01 UTC (permalink / raw)
To: u-boot
On 08/14/2018 11:51 AM, Simon Goldschmidt wrote:
> On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <marex@denx.de> wrote:
>>
>> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
>> is available and can be corrupted by loading ie. uImage or fitImage
>> headers there. Sometimes it could be beneficial to load the headers
>> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
>> still want to parse the image headers in some local onchip memory to
>> ie. extract firmware from that image.
>>
>> Add the possibility to override the location where the headers get
>> loaded by introducing new function, spl_get_load_buffer() which takes
>> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
>> data that are to be loaded there -- and returns a valid buffer address
>> or hangs the system. The default behavior is the same as before, add
>> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
>> override the weak spl_get_load_buffer() function though.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>> V2: Fix build issues on multiple boards due to incorrect pointer casting
[...]
>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
>> index e594beaeaa..619b39a537 100644
>> --- a/common/spl/spl_ram.c
>> +++ b/common/spl/spl_ram.c
>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
>> * No binman support or no information. For now, fix it
>> * to the address pointed to by U-Boot.
>> */
>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
>> - sizeof(struct image_header);
>> + header = spl_get_load_buffer(-sizeof(*header),
>> + sizeof(*header));
>> +
>
> Using "spl_get_load_buffer()" here seems to be a bit misleading, as
> the address is used for "execute-in-place", not for loading.
Do you have a better solution ? Instead of hard-coding the load buffer
address with some macro, this uses function which could be overridden.
Whether it's XIP or not has nothing to do with it.
>> }
>> header = (struct image_header *)map_sysmem(u_boot_pos, 0);
>>
>> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
>> index ba60a3a3c5..e10cf0124f 100644
>> --- a/common/spl/spl_spi.c
>> +++ b/common/spl/spl_spi.c
>> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
>> return -ENODEV;
>> }
>>
>> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
>> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
>> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
>
> Shouldn't the first argument be 0 here instead of -sizeof(*header)?
No, because then the payload doesn't end up at CONFIG_SYS_TEXT_BASE .
[...]
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 10:01 ` Marek Vasut
@ 2018-08-14 10:22 ` Simon Goldschmidt
2018-08-14 10:42 ` Marek Vasut
0 siblings, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-08-14 10:22 UTC (permalink / raw)
To: u-boot
On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <marex@denx.de> wrote:
>
> On 08/14/2018 11:51 AM, Simon Goldschmidt wrote:
> > On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <marex@denx.de> wrote:
> >>
> >> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
> >> is available and can be corrupted by loading ie. uImage or fitImage
> >> headers there. Sometimes it could be beneficial to load the headers
> >> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
> >> still want to parse the image headers in some local onchip memory to
> >> ie. extract firmware from that image.
> >>
> >> Add the possibility to override the location where the headers get
> >> loaded by introducing new function, spl_get_load_buffer() which takes
> >> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
> >> data that are to be loaded there -- and returns a valid buffer address
> >> or hangs the system. The default behavior is the same as before, add
> >> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
> >> override the weak spl_get_load_buffer() function though.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Tom Rini <trini@konsulko.com>
> >> ---
> >> V2: Fix build issues on multiple boards due to incorrect pointer casting
>
> [...]
>
> >> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
> >> index e594beaeaa..619b39a537 100644
> >> --- a/common/spl/spl_ram.c
> >> +++ b/common/spl/spl_ram.c
> >> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
> >> * No binman support or no information. For now, fix it
> >> * to the address pointed to by U-Boot.
> >> */
> >> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
> >> - sizeof(struct image_header);
> >> + header = spl_get_load_buffer(-sizeof(*header),
> >> + sizeof(*header));
> >> +
> >
> > Using "spl_get_load_buffer()" here seems to be a bit misleading, as
> > the address is used for "execute-in-place", not for loading.
>
> Do you have a better solution ? Instead of hard-coding the load buffer
> address with some macro, this uses function which could be overridden.
> Whether it's XIP or not has nothing to do with it.
I meant the name is a bit misleading as it implies loading. But since
the preferred way to do this is using binman, it's probably not worth
adding yet another weak function for RAM boot?
>
> >> }
> >> header = (struct image_header *)map_sysmem(u_boot_pos, 0);
> >>
> >> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> >> index ba60a3a3c5..e10cf0124f 100644
> >> --- a/common/spl/spl_spi.c
> >> +++ b/common/spl/spl_spi.c
> >> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
> >> return -ENODEV;
> >> }
> >>
> >> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
> >> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> >> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
> >
> > Shouldn't the first argument be 0 here instead of -sizeof(*header)?
>
> No, because then the payload doesn't end up at CONFIG_SYS_TEXT_BASE .
Sorry, I haven't studied the code around the patch, only the patch.
And I still think "header" has changed from "CONFIG_SYS_TEXT_BASE" to
"CONFIG_SYS_TEXT_BASE - sizeof(*header)" with your patch. That might
be a change required to get it work, I don't know that. But as this
isn' mentioned in the commit message, to me it seemed like a copy and
paste error or something.
Simon
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 10:22 ` Simon Goldschmidt
@ 2018-08-14 10:42 ` Marek Vasut
2018-08-14 12:56 ` Simon Goldschmidt
0 siblings, 1 reply; 16+ messages in thread
From: Marek Vasut @ 2018-08-14 10:42 UTC (permalink / raw)
To: u-boot
On 08/14/2018 12:22 PM, Simon Goldschmidt wrote:
> On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 08/14/2018 11:51 AM, Simon Goldschmidt wrote:
>>> On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
>>>> is available and can be corrupted by loading ie. uImage or fitImage
>>>> headers there. Sometimes it could be beneficial to load the headers
>>>> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
>>>> still want to parse the image headers in some local onchip memory to
>>>> ie. extract firmware from that image.
>>>>
>>>> Add the possibility to override the location where the headers get
>>>> loaded by introducing new function, spl_get_load_buffer() which takes
>>>> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
>>>> data that are to be loaded there -- and returns a valid buffer address
>>>> or hangs the system. The default behavior is the same as before, add
>>>> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
>>>> override the weak spl_get_load_buffer() function though.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> ---
>>>> V2: Fix build issues on multiple boards due to incorrect pointer casting
>>
>> [...]
>>
>>>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
>>>> index e594beaeaa..619b39a537 100644
>>>> --- a/common/spl/spl_ram.c
>>>> +++ b/common/spl/spl_ram.c
>>>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
>>>> * No binman support or no information. For now, fix it
>>>> * to the address pointed to by U-Boot.
>>>> */
>>>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
>>>> - sizeof(struct image_header);
>>>> + header = spl_get_load_buffer(-sizeof(*header),
>>>> + sizeof(*header));
>>>> +
>>>
>>> Using "spl_get_load_buffer()" here seems to be a bit misleading, as
>>> the address is used for "execute-in-place", not for loading.
>>
>> Do you have a better solution ? Instead of hard-coding the load buffer
>> address with some macro, this uses function which could be overridden.
>> Whether it's XIP or not has nothing to do with it.
>
> I meant the name is a bit misleading as it implies loading. But since
> the preferred way to do this is using binman, it's probably not worth
> adding yet another weak function for RAM boot?
Do you have a better name that fits all the other usecases ? This
function just gets you buffer into which you can load the image.
>>>> }
>>>> header = (struct image_header *)map_sysmem(u_boot_pos, 0);
>>>>
>>>> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
>>>> index ba60a3a3c5..e10cf0124f 100644
>>>> --- a/common/spl/spl_spi.c
>>>> +++ b/common/spl/spl_spi.c
>>>> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
>>>> return -ENODEV;
>>>> }
>>>>
>>>> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
>>>> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
>>>> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
>>>
>>> Shouldn't the first argument be 0 here instead of -sizeof(*header)?
>>
>> No, because then the payload doesn't end up at CONFIG_SYS_TEXT_BASE .
>
> Sorry, I haven't studied the code around the patch, only the patch.
> And I still think "header" has changed from "CONFIG_SYS_TEXT_BASE" to
> "CONFIG_SYS_TEXT_BASE - sizeof(*header)" with your patch. That might
> be a change required to get it work, I don't know that. But as this
> isn' mentioned in the commit message, to me it seemed like a copy and
> paste error or something.
I suspect it's the SPI that's weird. Look at the surrounding code, IMO
this is how it should be.
> Simon
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 10:42 ` Marek Vasut
@ 2018-08-14 12:56 ` Simon Goldschmidt
2018-08-14 18:12 ` Marek Vasut
0 siblings, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-08-14 12:56 UTC (permalink / raw)
To: u-boot
On Tue, Aug 14, 2018 at 2:05 PM Marek Vasut <marex@denx.de> wrote:
>
> On 08/14/2018 12:22 PM, Simon Goldschmidt wrote:
> > On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 08/14/2018 11:51 AM, Simon Goldschmidt wrote:
> >>> On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
> >>>> is available and can be corrupted by loading ie. uImage or fitImage
> >>>> headers there. Sometimes it could be beneficial to load the headers
> >>>> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
> >>>> still want to parse the image headers in some local onchip memory to
> >>>> ie. extract firmware from that image.
> >>>>
> >>>> Add the possibility to override the location where the headers get
> >>>> loaded by introducing new function, spl_get_load_buffer() which takes
> >>>> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
> >>>> data that are to be loaded there -- and returns a valid buffer address
> >>>> or hangs the system. The default behavior is the same as before, add
> >>>> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
> >>>> override the weak spl_get_load_buffer() function though.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> Cc: Tom Rini <trini@konsulko.com>
> >>>> ---
> >>>> V2: Fix build issues on multiple boards due to incorrect pointer casting
> >>
> >> [...]
> >>
> >>>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
> >>>> index e594beaeaa..619b39a537 100644
> >>>> --- a/common/spl/spl_ram.c
> >>>> +++ b/common/spl/spl_ram.c
> >>>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
> >>>> * No binman support or no information. For now, fix it
> >>>> * to the address pointed to by U-Boot.
> >>>> */
> >>>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
> >>>> - sizeof(struct image_header);
> >>>> + header = spl_get_load_buffer(-sizeof(*header),
> >>>> + sizeof(*header));
> >>>> +
> >>>
> >>> Using "spl_get_load_buffer()" here seems to be a bit misleading, as
> >>> the address is used for "execute-in-place", not for loading.
> >>
> >> Do you have a better solution ? Instead of hard-coding the load buffer
> >> address with some macro, this uses function which could be overridden.
> >> Whether it's XIP or not has nothing to do with it.
> >
> > I meant the name is a bit misleading as it implies loading. But since
> > the preferred way to do this is using binman, it's probably not worth
> > adding yet another weak function for RAM boot?
>
> Do you have a better name that fits all the other usecases ? This
> function just gets you buffer into which you can load the image.
Not really. I just wonder if you have to override the location for
some board, RAM booting might not work any more as it relies on a
fixed address, not some generic buffer.
Maybe we could add the boot device to your new weak function? If we
add some comment to the new weak function, that would have made it
much more clear for me how to boot U-Boot from FPGA OnChip RAM when I
tried some months ago :-)
> >>>> }
> >>>> header = (struct image_header *)map_sysmem(u_boot_pos, 0);
> >>>>
> >>>> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> >>>> index ba60a3a3c5..e10cf0124f 100644
> >>>> --- a/common/spl/spl_spi.c
> >>>> +++ b/common/spl/spl_spi.c
> >>>> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
> >>>> return -ENODEV;
> >>>> }
> >>>>
> >>>> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
> >>>> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> >>>> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
> >>>
> >>> Shouldn't the first argument be 0 here instead of -sizeof(*header)?
> >>
> >> No, because then the payload doesn't end up at CONFIG_SYS_TEXT_BASE .
> >
> > Sorry, I haven't studied the code around the patch, only the patch.
> > And I still think "header" has changed from "CONFIG_SYS_TEXT_BASE" to
> > "CONFIG_SYS_TEXT_BASE - sizeof(*header)" with your patch. That might
> > be a change required to get it work, I don't know that. But as this
> > isn' mentioned in the commit message, to me it seemed like a copy and
> > paste error or something.
>
> I suspect it's the SPI that's weird. Look at the surrounding code, IMO
> this is how it should be.
Reading the code, I guess the exact location of 'header' is not
important. So the code should still work after applying your patch,
even if it changes the location of 'header'.
Simon
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 12:56 ` Simon Goldschmidt
@ 2018-08-14 18:12 ` Marek Vasut
2018-08-14 18:25 ` Simon Goldschmidt
0 siblings, 1 reply; 16+ messages in thread
From: Marek Vasut @ 2018-08-14 18:12 UTC (permalink / raw)
To: u-boot
On 08/14/2018 02:56 PM, Simon Goldschmidt wrote:
> On Tue, Aug 14, 2018 at 2:05 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 08/14/2018 12:22 PM, Simon Goldschmidt wrote:
>>> On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 08/14/2018 11:51 AM, Simon Goldschmidt wrote:
>>>>> On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
>>>>>> is available and can be corrupted by loading ie. uImage or fitImage
>>>>>> headers there. Sometimes it could be beneficial to load the headers
>>>>>> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
>>>>>> still want to parse the image headers in some local onchip memory to
>>>>>> ie. extract firmware from that image.
>>>>>>
>>>>>> Add the possibility to override the location where the headers get
>>>>>> loaded by introducing new function, spl_get_load_buffer() which takes
>>>>>> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
>>>>>> data that are to be loaded there -- and returns a valid buffer address
>>>>>> or hangs the system. The default behavior is the same as before, add
>>>>>> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
>>>>>> override the weak spl_get_load_buffer() function though.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>> ---
>>>>>> V2: Fix build issues on multiple boards due to incorrect pointer casting
>>>>
>>>> [...]
>>>>
>>>>>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
>>>>>> index e594beaeaa..619b39a537 100644
>>>>>> --- a/common/spl/spl_ram.c
>>>>>> +++ b/common/spl/spl_ram.c
>>>>>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
>>>>>> * No binman support or no information. For now, fix it
>>>>>> * to the address pointed to by U-Boot.
>>>>>> */
>>>>>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
>>>>>> - sizeof(struct image_header);
>>>>>> + header = spl_get_load_buffer(-sizeof(*header),
>>>>>> + sizeof(*header));
>>>>>> +
>>>>>
>>>>> Using "spl_get_load_buffer()" here seems to be a bit misleading, as
>>>>> the address is used for "execute-in-place", not for loading.
>>>>
>>>> Do you have a better solution ? Instead of hard-coding the load buffer
>>>> address with some macro, this uses function which could be overridden.
>>>> Whether it's XIP or not has nothing to do with it.
>>>
>>> I meant the name is a bit misleading as it implies loading. But since
>>> the preferred way to do this is using binman, it's probably not worth
>>> adding yet another weak function for RAM boot?
>>
>> Do you have a better name that fits all the other usecases ? This
>> function just gets you buffer into which you can load the image.
>
> Not really. I just wonder if you have to override the location for
> some board, RAM booting might not work any more as it relies on a
> fixed address, not some generic buffer.
I do, yeah, the board is not upstream completely yet though, so I am
just sending this as a cleanup.
> Maybe we could add the boot device to your new weak function? If we
> add some comment to the new weak function, that would have made it
> much more clear for me how to boot U-Boot from FPGA OnChip RAM when I
> tried some months ago :-)
This really just gives you a buffer. I don't need to know which boot
media is used. If there is a usecase, sure, it can be added later.
>>>>>> }
>>>>>> header = (struct image_header *)map_sysmem(u_boot_pos, 0);
>>>>>>
>>>>>> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
>>>>>> index ba60a3a3c5..e10cf0124f 100644
>>>>>> --- a/common/spl/spl_spi.c
>>>>>> +++ b/common/spl/spl_spi.c
>>>>>> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
>>>>>> return -ENODEV;
>>>>>> }
>>>>>>
>>>>>> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
>>>>>> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
>>>>>> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
>>>>>
>>>>> Shouldn't the first argument be 0 here instead of -sizeof(*header)?
>>>>
>>>> No, because then the payload doesn't end up at CONFIG_SYS_TEXT_BASE .
>>>
>>> Sorry, I haven't studied the code around the patch, only the patch.
>>> And I still think "header" has changed from "CONFIG_SYS_TEXT_BASE" to
>>> "CONFIG_SYS_TEXT_BASE - sizeof(*header)" with your patch. That might
>>> be a change required to get it work, I don't know that. But as this
>>> isn' mentioned in the commit message, to me it seemed like a copy and
>>> paste error or something.
>>
>> I suspect it's the SPI that's weird. Look at the surrounding code, IMO
>> this is how it should be.
>
> Reading the code, I guess the exact location of 'header' is not
> important. So the code should still work after applying your patch,
> even if it changes the location of 'header'.
The thing is, the payload (ie. uboot) is linked against the TEXT_BASE,
so putting it at TEXT_BASE + offset can cause trouble.
> Simon
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 18:12 ` Marek Vasut
@ 2018-08-14 18:25 ` Simon Goldschmidt
2018-08-15 7:52 ` Marek Vasut
0 siblings, 1 reply; 16+ messages in thread
From: Simon Goldschmidt @ 2018-08-14 18:25 UTC (permalink / raw)
To: u-boot
On Tue, Aug 14, 2018 at 8:17 PM Marek Vasut <marex@denx.de> wrote:
>
> On 08/14/2018 02:56 PM, Simon Goldschmidt wrote:
> > On Tue, Aug 14, 2018 at 2:05 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 08/14/2018 12:22 PM, Simon Goldschmidt wrote:
> >>> On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 08/14/2018 11:51 AM, Simon Goldschmidt wrote:
> >>>>> On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
> >>>>>> is available and can be corrupted by loading ie. uImage or fitImage
> >>>>>> headers there. Sometimes it could be beneficial to load the headers
> >>>>>> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
> >>>>>> still want to parse the image headers in some local onchip memory to
> >>>>>> ie. extract firmware from that image.
> >>>>>>
> >>>>>> Add the possibility to override the location where the headers get
> >>>>>> loaded by introducing new function, spl_get_load_buffer() which takes
> >>>>>> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
> >>>>>> data that are to be loaded there -- and returns a valid buffer address
> >>>>>> or hangs the system. The default behavior is the same as before, add
> >>>>>> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
> >>>>>> override the weak spl_get_load_buffer() function though.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>> ---
> >>>>>> V2: Fix build issues on multiple boards due to incorrect pointer casting
> >>>>
> >>>> [...]
> >>>>
> >>>>>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
> >>>>>> index e594beaeaa..619b39a537 100644
> >>>>>> --- a/common/spl/spl_ram.c
> >>>>>> +++ b/common/spl/spl_ram.c
> >>>>>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
> >>>>>> * No binman support or no information. For now, fix it
> >>>>>> * to the address pointed to by U-Boot.
> >>>>>> */
> >>>>>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
> >>>>>> - sizeof(struct image_header);
> >>>>>> + header = spl_get_load_buffer(-sizeof(*header),
> >>>>>> + sizeof(*header));
> >>>>>> +
> >>>>>
> >>>>> Using "spl_get_load_buffer()" here seems to be a bit misleading, as
> >>>>> the address is used for "execute-in-place", not for loading.
> >>>>
> >>>> Do you have a better solution ? Instead of hard-coding the load buffer
> >>>> address with some macro, this uses function which could be overridden.
> >>>> Whether it's XIP or not has nothing to do with it.
> >>>
> >>> I meant the name is a bit misleading as it implies loading. But since
> >>> the preferred way to do this is using binman, it's probably not worth
> >>> adding yet another weak function for RAM boot?
> >>
> >> Do you have a better name that fits all the other usecases ? This
> >> function just gets you buffer into which you can load the image.
> >
> > Not really. I just wonder if you have to override the location for
> > some board, RAM booting might not work any more as it relies on a
> > fixed address, not some generic buffer.
>
> I do, yeah, the board is not upstream completely yet though, so I am
> just sending this as a cleanup.
>
> > Maybe we could add the boot device to your new weak function? If we
> > add some comment to the new weak function, that would have made it
> > much more clear for me how to boot U-Boot from FPGA OnChip RAM when I
> > tried some months ago :-)
>
> This really just gives you a buffer. I don't need to know which boot
> media is used. If there is a usecase, sure, it can be added later.
I may have a use case for one of our custom boards, but it's probably
not worth doing that now.
>
> >>>>>> }
> >>>>>> header = (struct image_header *)map_sysmem(u_boot_pos, 0);
> >>>>>>
> >>>>>> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> >>>>>> index ba60a3a3c5..e10cf0124f 100644
> >>>>>> --- a/common/spl/spl_spi.c
> >>>>>> +++ b/common/spl/spl_spi.c
> >>>>>> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
> >>>>>> return -ENODEV;
> >>>>>> }
> >>>>>>
> >>>>>> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
> >>>>>> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> >>>>>> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
> >>>>>
> >>>>> Shouldn't the first argument be 0 here instead of -sizeof(*header)?
> >>>>
> >>>> No, because then the payload doesn't end up at CONFIG_SYS_TEXT_BASE .
> >>>
> >>> Sorry, I haven't studied the code around the patch, only the patch.
> >>> And I still think "header" has changed from "CONFIG_SYS_TEXT_BASE" to
> >>> "CONFIG_SYS_TEXT_BASE - sizeof(*header)" with your patch. That might
> >>> be a change required to get it work, I don't know that. But as this
> >>> isn' mentioned in the commit message, to me it seemed like a copy and
> >>> paste error or something.
> >>
> >> I suspect it's the SPI that's weird. Look at the surrounding code, IMO
> >> this is how it should be.
> >
> > Reading the code, I guess the exact location of 'header' is not
> > important. So the code should still work after applying your patch,
> > even if it changes the location of 'header'.
>
> The thing is, the payload (ie. uboot) is linked against the TEXT_BASE,
> so putting it at TEXT_BASE + offset can cause trouble.
Right. All I wanted to say is I saw something that changed but from
the commit message, it didn't seem like it was intended.
The rest seems fine to me right now, so if it's of any use:
Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 18:25 ` Simon Goldschmidt
@ 2018-08-15 7:52 ` Marek Vasut
2018-08-15 9:04 ` Simon Goldschmidt
0 siblings, 1 reply; 16+ messages in thread
From: Marek Vasut @ 2018-08-15 7:52 UTC (permalink / raw)
To: u-boot
On 08/14/2018 08:25 PM, Simon Goldschmidt wrote:
> On Tue, Aug 14, 2018 at 8:17 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 08/14/2018 02:56 PM, Simon Goldschmidt wrote:
>>> On Tue, Aug 14, 2018 at 2:05 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 08/14/2018 12:22 PM, Simon Goldschmidt wrote:
>>>>> On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 08/14/2018 11:51 AM, Simon Goldschmidt wrote:
>>>>>>> On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <marex@denx.de> wrote:
>>>>>>>>
>>>>>>>> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
>>>>>>>> is available and can be corrupted by loading ie. uImage or fitImage
>>>>>>>> headers there. Sometimes it could be beneficial to load the headers
>>>>>>>> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
>>>>>>>> still want to parse the image headers in some local onchip memory to
>>>>>>>> ie. extract firmware from that image.
>>>>>>>>
>>>>>>>> Add the possibility to override the location where the headers get
>>>>>>>> loaded by introducing new function, spl_get_load_buffer() which takes
>>>>>>>> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
>>>>>>>> data that are to be loaded there -- and returns a valid buffer address
>>>>>>>> or hangs the system. The default behavior is the same as before, add
>>>>>>>> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
>>>>>>>> override the weak spl_get_load_buffer() function though.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>> ---
>>>>>>>> V2: Fix build issues on multiple boards due to incorrect pointer casting
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
>>>>>>>> index e594beaeaa..619b39a537 100644
>>>>>>>> --- a/common/spl/spl_ram.c
>>>>>>>> +++ b/common/spl/spl_ram.c
>>>>>>>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info *spl_image,
>>>>>>>> * No binman support or no information. For now, fix it
>>>>>>>> * to the address pointed to by U-Boot.
>>>>>>>> */
>>>>>>>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
>>>>>>>> - sizeof(struct image_header);
>>>>>>>> + header = spl_get_load_buffer(-sizeof(*header),
>>>>>>>> + sizeof(*header));
>>>>>>>> +
>>>>>>>
>>>>>>> Using "spl_get_load_buffer()" here seems to be a bit misleading, as
>>>>>>> the address is used for "execute-in-place", not for loading.
>>>>>>
>>>>>> Do you have a better solution ? Instead of hard-coding the load buffer
>>>>>> address with some macro, this uses function which could be overridden.
>>>>>> Whether it's XIP or not has nothing to do with it.
>>>>>
>>>>> I meant the name is a bit misleading as it implies loading. But since
>>>>> the preferred way to do this is using binman, it's probably not worth
>>>>> adding yet another weak function for RAM boot?
>>>>
>>>> Do you have a better name that fits all the other usecases ? This
>>>> function just gets you buffer into which you can load the image.
>>>
>>> Not really. I just wonder if you have to override the location for
>>> some board, RAM booting might not work any more as it relies on a
>>> fixed address, not some generic buffer.
>>
>> I do, yeah, the board is not upstream completely yet though, so I am
>> just sending this as a cleanup.
>>
>>> Maybe we could add the boot device to your new weak function? If we
>>> add some comment to the new weak function, that would have made it
>>> much more clear for me how to boot U-Boot from FPGA OnChip RAM when I
>>> tried some months ago :-)
>>
>> This really just gives you a buffer. I don't need to know which boot
>> media is used. If there is a usecase, sure, it can be added later.
>
> I may have a use case for one of our custom boards, but it's probably
> not worth doing that now.
Subsequent patch then ?
>>>>>>>> }
>>>>>>>> header = (struct image_header *)map_sysmem(u_boot_pos, 0);
>>>>>>>>
>>>>>>>> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
>>>>>>>> index ba60a3a3c5..e10cf0124f 100644
>>>>>>>> --- a/common/spl/spl_spi.c
>>>>>>>> +++ b/common/spl/spl_spi.c
>>>>>>>> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info *spl_image,
>>>>>>>> return -ENODEV;
>>>>>>>> }
>>>>>>>>
>>>>>>>> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
>>>>>>>> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
>>>>>>>> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
>>>>>>>
>>>>>>> Shouldn't the first argument be 0 here instead of -sizeof(*header)?
>>>>>>
>>>>>> No, because then the payload doesn't end up at CONFIG_SYS_TEXT_BASE .
>>>>>
>>>>> Sorry, I haven't studied the code around the patch, only the patch.
>>>>> And I still think "header" has changed from "CONFIG_SYS_TEXT_BASE" to
>>>>> "CONFIG_SYS_TEXT_BASE - sizeof(*header)" with your patch. That might
>>>>> be a change required to get it work, I don't know that. But as this
>>>>> isn' mentioned in the commit message, to me it seemed like a copy and
>>>>> paste error or something.
>>>>
>>>> I suspect it's the SPI that's weird. Look at the surrounding code, IMO
>>>> this is how it should be.
>>>
>>> Reading the code, I guess the exact location of 'header' is not
>>> important. So the code should still work after applying your patch,
>>> even if it changes the location of 'header'.
>>
>> The thing is, the payload (ie. uboot) is linked against the TEXT_BASE,
>> so putting it at TEXT_BASE + offset can cause trouble.
>
> Right. All I wanted to say is I saw something that changed but from
> the commit message, it didn't seem like it was intended.
> The rest seems fine to me right now, so if it's of any use:
>
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-15 7:52 ` Marek Vasut
@ 2018-08-15 9:04 ` Simon Goldschmidt
0 siblings, 0 replies; 16+ messages in thread
From: Simon Goldschmidt @ 2018-08-15 9:04 UTC (permalink / raw)
To: u-boot
Marek Vasut <marex@denx.de> schrieb am Mi., 15. Aug. 2018, 10:57:
> On 08/14/2018 08:25 PM, Simon Goldschmidt wrote:
> > On Tue, Aug 14, 2018 at 8:17 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 08/14/2018 02:56 PM, Simon Goldschmidt wrote:
> >>> On Tue, Aug 14, 2018 at 2:05 PM Marek Vasut <marex@denx.de> wrote:
> >>>>
> >>>> On 08/14/2018 12:22 PM, Simon Goldschmidt wrote:
> >>>>> On Tue, Aug 14, 2018 at 12:10 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>
> >>>>>> On 08/14/2018 11:51 AM, Simon Goldschmidt wrote:
> >>>>>>> On Tue, Aug 14, 2018 at 11:27 AM Marek Vasut <marex@denx.de>
> wrote:
> >>>>>>>>
> >>>>>>>> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory
> location
> >>>>>>>> is available and can be corrupted by loading ie. uImage or
> fitImage
> >>>>>>>> headers there. Sometimes it could be beneficial to load the
> headers
> >>>>>>>> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while
> we
> >>>>>>>> still want to parse the image headers in some local onchip memory
> to
> >>>>>>>> ie. extract firmware from that image.
> >>>>>>>>
> >>>>>>>> Add the possibility to override the location where the headers get
> >>>>>>>> loaded by introducing new function, spl_get_load_buffer() which
> takes
> >>>>>>>> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of
> the
> >>>>>>>> data that are to be loaded there -- and returns a valid buffer
> address
> >>>>>>>> or hangs the system. The default behavior is the same as before,
> add
> >>>>>>>> the offset to CONFIG_SYS_TEXT_BASE and return that address. User
> can
> >>>>>>>> override the weak spl_get_load_buffer() function though.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>> ---
> >>>>>>>> V2: Fix build issues on multiple boards due to incorrect pointer
> casting
> >>>>>>
> >>>>>> [...]
> >>>>>>
> >>>>>>>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
> >>>>>>>> index e594beaeaa..619b39a537 100644
> >>>>>>>> --- a/common/spl/spl_ram.c
> >>>>>>>> +++ b/common/spl/spl_ram.c
> >>>>>>>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct
> spl_image_info *spl_image,
> >>>>>>>> * No binman support or no information.
> For now, fix it
> >>>>>>>> * to the address pointed to by U-Boot.
> >>>>>>>> */
> >>>>>>>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
> >>>>>>>> - sizeof(struct
> image_header);
> >>>>>>>> + header =
> spl_get_load_buffer(-sizeof(*header),
> >>>>>>>> +
> sizeof(*header));
> >>>>>>>> +
> >>>>>>>
> >>>>>>> Using "spl_get_load_buffer()" here seems to be a bit misleading, as
> >>>>>>> the address is used for "execute-in-place", not for loading.
> >>>>>>
> >>>>>> Do you have a better solution ? Instead of hard-coding the load
> buffer
> >>>>>> address with some macro, this uses function which could be
> overridden.
> >>>>>> Whether it's XIP or not has nothing to do with it.
> >>>>>
> >>>>> I meant the name is a bit misleading as it implies loading. But since
> >>>>> the preferred way to do this is using binman, it's probably not worth
> >>>>> adding yet another weak function for RAM boot?
> >>>>
> >>>> Do you have a better name that fits all the other usecases ? This
> >>>> function just gets you buffer into which you can load the image.
> >>>
> >>> Not really. I just wonder if you have to override the location for
> >>> some board, RAM booting might not work any more as it relies on a
> >>> fixed address, not some generic buffer.
> >>
> >> I do, yeah, the board is not upstream completely yet though, so I am
> >> just sending this as a cleanup.
> >>
> >>> Maybe we could add the boot device to your new weak function? If we
> >>> add some comment to the new weak function, that would have made it
> >>> much more clear for me how to boot U-Boot from FPGA OnChip RAM when I
> >>> tried some months ago :-)
> >>
> >> This really just gives you a buffer. I don't need to know which boot
> >> media is used. If there is a usecase, sure, it can be added later.
> >
> > I may have a use case for one of our custom boards, but it's probably
> > not worth doing that now.
>
> Subsequent patch then ?
>
I'll send a patch if we really need that...
> >>>>>>>> }
> >>>>>>>> header = (struct image_header
> *)map_sysmem(u_boot_pos, 0);
> >>>>>>>>
> >>>>>>>> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> >>>>>>>> index ba60a3a3c5..e10cf0124f 100644
> >>>>>>>> --- a/common/spl/spl_spi.c
> >>>>>>>> +++ b/common/spl/spl_spi.c
> >>>>>>>> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct
> spl_image_info *spl_image,
> >>>>>>>> return -ENODEV;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
> >>>>>>>> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> >>>>>>>> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
> >>>>>>>
> >>>>>>> Shouldn't the first argument be 0 here instead of -sizeof(*header)?
> >>>>>>
> >>>>>> No, because then the payload doesn't end up at CONFIG_SYS_TEXT_BASE
> .
> >>>>>
> >>>>> Sorry, I haven't studied the code around the patch, only the patch.
> >>>>> And I still think "header" has changed from "CONFIG_SYS_TEXT_BASE" to
> >>>>> "CONFIG_SYS_TEXT_BASE - sizeof(*header)" with your patch. That might
> >>>>> be a change required to get it work, I don't know that. But as this
> >>>>> isn' mentioned in the commit message, to me it seemed like a copy and
> >>>>> paste error or something.
> >>>>
> >>>> I suspect it's the SPI that's weird. Look at the surrounding code, IMO
> >>>> this is how it should be.
> >>>
> >>> Reading the code, I guess the exact location of 'header' is not
> >>> important. So the code should still work after applying your patch,
> >>> even if it changes the location of 'header'.
> >>
> >> The thing is, the payload (ie. uboot) is linked against the TEXT_BASE,
> >> so putting it at TEXT_BASE + offset can cause trouble.
> >
> > Right. All I wanted to say is I saw something that changed but from
> > the commit message, it didn't seem like it was intended.
> > The rest seems fine to me right now, so if it's of any use:
> >
> > Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> >
>
>
> --
> Best regards,
> Marek Vasut
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [U-Boot,V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 9:27 [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage Marek Vasut
2018-08-14 9:51 ` Simon Goldschmidt
@ 2018-09-26 12:42 ` Tom Rini
2018-10-03 13:39 ` [U-Boot] [PATCH V2] " Michal Simek
2 siblings, 0 replies; 16+ messages in thread
From: Tom Rini @ 2018-09-26 12:42 UTC (permalink / raw)
To: u-boot
On Tue, Aug 14, 2018 at 11:27:02AM +0200, Marek Vasut wrote:
> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
> is available and can be corrupted by loading ie. uImage or fitImage
> headers there. Sometimes it could be beneficial to load the headers
> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
> still want to parse the image headers in some local onchip memory to
> ie. extract firmware from that image.
>
> Add the possibility to override the location where the headers get
> loaded by introducing new function, spl_get_load_buffer() which takes
> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
> data that are to be loaded there -- and returns a valid buffer address
> or hangs the system. The default behavior is the same as before, add
> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
> override the weak spl_get_load_buffer() function though.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Tom Rini <trini@konsulko.com>
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180926/c60c4946/attachment.sig>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-08-14 9:27 [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage Marek Vasut
2018-08-14 9:51 ` Simon Goldschmidt
2018-09-26 12:42 ` [U-Boot] [U-Boot,V2] " Tom Rini
@ 2018-10-03 13:39 ` Michal Simek
2018-10-03 19:41 ` Marek Vasut
2 siblings, 1 reply; 16+ messages in thread
From: Michal Simek @ 2018-10-03 13:39 UTC (permalink / raw)
To: u-boot
Hi Marek,
út 14. 8. 2018 v 11:27 odesílatel Marek Vasut <marex@denx.de> napsal:
> The SPL loaders assume that the CONFIG_SYS_TEXT_BASE memory location
> is available and can be corrupted by loading ie. uImage or fitImage
> headers there. Sometimes it could be beneficial to load the headers
> elsewhere, ie. if CONFIG_SYS_TEXT_BASE is not yet writable while we
> still want to parse the image headers in some local onchip memory to
> ie. extract firmware from that image.
>
> Add the possibility to override the location where the headers get
> loaded by introducing new function, spl_get_load_buffer() which takes
> two arguments -- offset from the CONFIG_SYS_TEXT_BASE and size of the
> data that are to be loaded there -- and returns a valid buffer address
> or hangs the system. The default behavior is the same as before, add
> the offset to CONFIG_SYS_TEXT_BASE and return that address. User can
> override the weak spl_get_load_buffer() function though.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Tom Rini <trini@konsulko.com>
> ---
> V2: Fix build issues on multiple boards due to incorrect pointer casting
> ---
> common/spl/spl.c | 5 +++++
> common/spl/spl_ext.c | 3 +--
> common/spl/spl_fat.c | 3 +--
> common/spl/spl_fit.c | 6 +++---
> common/spl/spl_mmc.c | 6 +++---
> common/spl/spl_nand.c | 4 ++--
> common/spl/spl_onenand.c | 3 +--
> common/spl/spl_ram.c | 5 +++--
> common/spl/spl_spi.c | 3 +--
> common/spl/spl_ubi.c | 3 +--
> include/spl.h | 9 +++++++++
> 11 files changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index eda84d0c74..7c22d0f44d 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -127,6 +127,11 @@ __weak void spl_board_prepare_for_boot(void)
> /* Nothing to do! */
> }
>
> +__weak struct image_header *spl_get_load_buffer(ssize_t offset, size_t
> size)
> +{
> + return (struct image_header *)(CONFIG_SYS_TEXT_BASE + offset);
> +}
> +
> void spl_set_header_raw_uboot(struct spl_image_info *spl_image)
> {
> ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos);
> diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
> index fd30a61f9a..fe05223605 100644
> --- a/common/spl/spl_ext.c
> +++ b/common/spl/spl_ext.c
> @@ -16,8 +16,7 @@ int spl_load_image_ext(struct spl_image_info *spl_image,
> loff_t filelen, actlen;
> disk_partition_t part_info = {};
>
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
> - sizeof(struct
> image_header));
> + header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>
> if (part_get_info(block_dev, partition, &part_info)) {
> printf("spl: no partition table found\n");
> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> index 0403016bb4..163e540622 100644
> --- a/common/spl/spl_fat.c
> +++ b/common/spl/spl_fat.c
> @@ -63,8 +63,7 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
> if (err)
> goto end;
>
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
> - sizeof(struct
> image_header));
> + header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>
> err = file_fat_read(filename, header, sizeof(struct image_header));
> if (err <= 0)
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 9eabb1c105..f08e5018c3 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -357,7 +357,7 @@ int spl_load_simple_fit(struct spl_image_info
> *spl_image,
> struct spl_image_info image_info;
> int node = -1;
> int images, ret;
> - int base_offset, align_len = ARCH_DMA_MINALIGN - 1;
> + int base_offset, hsize, align_len = ARCH_DMA_MINALIGN - 1;
> int index = 0;
>
> /*
> @@ -386,8 +386,8 @@ int spl_load_simple_fit(struct spl_image_info
> *spl_image,
> * For FIT with data embedded, data is loaded as part of FIT image.
> * For FIT with external data, data is not loaded in this step.
> */
> - fit = (void *)((CONFIG_SYS_TEXT_BASE - size - info->bl_len -
> - align_len) & ~align_len);
> + hsize = (size + info->bl_len + align_len) & ~align_len;
> + fit = spl_get_load_buffer(-hsize, hsize);
> sectors = get_aligned_image_size(info, size, 0);
> count = info->read(info, sector, sectors, fit);
> debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu\n",
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index 0b2f059570..75c41598e6 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -55,13 +55,13 @@ int mmc_load_image_raw_sector(struct spl_image_info
> *spl_image,
> {
> unsigned long count;
> struct image_header *header;
> + struct blk_desc *bd = mmc_get_blk_desc(mmc);
> int ret = 0;
>
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE -
> - sizeof(struct image_header));
> + header = spl_get_load_buffer(-sizeof(*header), bd->blksz);
>
> /* read image header to find the image size & load address */
> - count = blk_dread(mmc_get_blk_desc(mmc), sector, 1, header);
> + count = blk_dread(bd, sector, 1, header);
> debug("hdr read sector %lx, count=%lu\n", sector, count);
> if (count == 0) {
> ret = -EIO;
> diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> index 2722fd3860..6eb190f1ea 100644
> --- a/common/spl/spl_nand.c
> +++ b/common/spl/spl_nand.c
> @@ -83,8 +83,8 @@ static int spl_nand_load_image(struct spl_image_info
> *spl_image,
> #endif
> nand_init();
>
> - /*use CONFIG_SYS_TEXT_BASE as temporary storage area */
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> + header = spl_get_load_buffer(0, sizeof(*header));
> +
> #ifdef CONFIG_SPL_OS_BOOT
> if (!spl_start_uboot()) {
> /*
> diff --git a/common/spl/spl_onenand.c b/common/spl/spl_onenand.c
> index d32333935a..ee30f328e6 100644
> --- a/common/spl/spl_onenand.c
> +++ b/common/spl/spl_onenand.c
> @@ -21,8 +21,7 @@ static int spl_onenand_load_image(struct spl_image_info
> *spl_image,
>
> debug("spl: onenand\n");
>
> - /*use CONFIG_SYS_TEXT_BASE as temporary storage area */
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> + header = spl_get_load_buffer(0, CONFIG_SYS_ONENAND_PAGE_SIZE);
> /* Load u-boot */
> onenand_spl_load_image(CONFIG_SYS_ONENAND_U_BOOT_OFFS,
> CONFIG_SYS_ONENAND_PAGE_SIZE, (void *)header);
> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
> index e594beaeaa..619b39a537 100644
> --- a/common/spl/spl_ram.c
> +++ b/common/spl/spl_ram.c
> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info
> *spl_image,
> * No binman support or no information. For now,
> fix it
> * to the address pointed to by U-Boot.
> */
> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
> - sizeof(struct image_header);
> + header = spl_get_load_buffer(-sizeof(*header),
> + sizeof(*header));
> +
>
I am curious how you have tested this change.
Because I see on my zc706 that this breaks my SPL boot flow.
This should be assigned to u_boot_pos instead of header (which is done 2
line below)
and also here is additional empty line which shouldn't be there.
> }
> header = (struct image_header *)map_sysmem(u_boot_pos, 0);
>
> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> index ba60a3a3c5..e10cf0124f 100644
> --- a/common/spl/spl_spi.c
> +++ b/common/spl/spl_spi.c
> @@ -88,8 +88,7 @@ static int spl_spi_load_image(struct spl_image_info
> *spl_image,
> return -ENODEV;
> }
>
> - /* use CONFIG_SYS_TEXT_BASE as temporary storage area */
> - header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> + header = spl_get_load_buffer(-sizeof(*header), 0x40);
>
And I am also curious why we have 0x40 here.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-10-03 13:39 ` [U-Boot] [PATCH V2] " Michal Simek
@ 2018-10-03 19:41 ` Marek Vasut
2018-10-04 6:00 ` Michal Simek
0 siblings, 1 reply; 16+ messages in thread
From: Marek Vasut @ 2018-10-03 19:41 UTC (permalink / raw)
To: u-boot
On 10/03/2018 03:39 PM, Michal Simek wrote:
> Hi Marek,
Hi,
[...]
>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
>> index e594beaeaa..619b39a537 100644
>> --- a/common/spl/spl_ram.c
>> +++ b/common/spl/spl_ram.c
>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info
>> *spl_image,
>> * No binman support or no information. For now,
>> fix it
>> * to the address pointed to by U-Boot.
>> */
>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
>> - sizeof(struct image_header);
>> + header = spl_get_load_buffer(-sizeof(*header),
>> + sizeof(*header));
>> +
>>
>
> I am curious how you have tested this change.
> Because I see on my zc706 that this breaks my SPL boot flow.
Definitely not on every single board we support ...
> This should be assigned to u_boot_pos instead of header (which is done 2
> line below)
> and also here is additional empty line which shouldn't be there.
Patches welcome. And yes, it should be u_boot_pos .
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-10-03 19:41 ` Marek Vasut
@ 2018-10-04 6:00 ` Michal Simek
2018-10-04 7:02 ` Marek Vasut
0 siblings, 1 reply; 16+ messages in thread
From: Michal Simek @ 2018-10-04 6:00 UTC (permalink / raw)
To: u-boot
On 3.10.2018 21:41, Marek Vasut wrote:
> On 10/03/2018 03:39 PM, Michal Simek wrote:
>> Hi Marek,
>
> Hi,
>
> [...]
>
>>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
>>> index e594beaeaa..619b39a537 100644
>>> --- a/common/spl/spl_ram.c
>>> +++ b/common/spl/spl_ram.c
>>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info
>>> *spl_image,
>>> * No binman support or no information. For now,
>>> fix it
>>> * to the address pointed to by U-Boot.
>>> */
>>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
>>> - sizeof(struct image_header);
>>> + header = spl_get_load_buffer(-sizeof(*header),
>>> + sizeof(*header));
>>> +
>>>
>>
>> I am curious how you have tested this change.
>> Because I see on my zc706 that this breaks my SPL boot flow.
>
> Definitely not on every single board we support ...
non has asked for testing on every single board but you should test or
at least ask for testing modes which you are not able to test yourself.
M
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-10-04 6:00 ` Michal Simek
@ 2018-10-04 7:02 ` Marek Vasut
2018-10-04 7:41 ` Michal Simek
0 siblings, 1 reply; 16+ messages in thread
From: Marek Vasut @ 2018-10-04 7:02 UTC (permalink / raw)
To: u-boot
On 10/04/2018 08:00 AM, Michal Simek wrote:
> On 3.10.2018 21:41, Marek Vasut wrote:
>> On 10/03/2018 03:39 PM, Michal Simek wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>> [...]
>>
>>>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
>>>> index e594beaeaa..619b39a537 100644
>>>> --- a/common/spl/spl_ram.c
>>>> +++ b/common/spl/spl_ram.c
>>>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info
>>>> *spl_image,
>>>> * No binman support or no information. For now,
>>>> fix it
>>>> * to the address pointed to by U-Boot.
>>>> */
>>>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
>>>> - sizeof(struct image_header);
>>>> + header = spl_get_load_buffer(-sizeof(*header),
>>>> + sizeof(*header));
>>>> +
>>>>
>>>
>>> I am curious how you have tested this change.
>>> Because I see on my zc706 that this breaks my SPL boot flow.
>>
>> Definitely not on every single board we support ...
>
>
> non has asked for testing on every single board but you should test or
> at least ask for testing modes which you are not able to test yourself.
You had the chance to review or test the patch while it was on the ML,
it was there for a long time (months?), that's what the review process
is for. Review process didn't catch it, so send a fix ...
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage
2018-10-04 7:02 ` Marek Vasut
@ 2018-10-04 7:41 ` Michal Simek
0 siblings, 0 replies; 16+ messages in thread
From: Michal Simek @ 2018-10-04 7:41 UTC (permalink / raw)
To: u-boot
On 4.10.2018 09:02, Marek Vasut wrote:
> On 10/04/2018 08:00 AM, Michal Simek wrote:
>> On 3.10.2018 21:41, Marek Vasut wrote:
>>> On 10/03/2018 03:39 PM, Michal Simek wrote:
>>>> Hi Marek,
>>>
>>> Hi,
>>>
>>> [...]
>>>
>>>>> diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c
>>>>> index e594beaeaa..619b39a537 100644
>>>>> --- a/common/spl/spl_ram.c
>>>>> +++ b/common/spl/spl_ram.c
>>>>> @@ -63,8 +63,9 @@ static int spl_ram_load_image(struct spl_image_info
>>>>> *spl_image,
>>>>> * No binman support or no information. For now,
>>>>> fix it
>>>>> * to the address pointed to by U-Boot.
>>>>> */
>>>>> - u_boot_pos = CONFIG_SYS_TEXT_BASE -
>>>>> - sizeof(struct image_header);
>>>>> + header = spl_get_load_buffer(-sizeof(*header),
>>>>> + sizeof(*header));
>>>>> +
>>>>>
>>>>
>>>> I am curious how you have tested this change.
>>>> Because I see on my zc706 that this breaks my SPL boot flow.
>>>
>>> Definitely not on every single board we support ...
>>
>>
>> non has asked for testing on every single board but you should test or
>> at least ask for testing modes which you are not able to test yourself.
>
> You had the chance to review or test the patch while it was on the ML,
> it was there for a long time (months?), that's what the review process
> is for. Review process didn't catch it, so send a fix ...
2 patches sent.
M
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-10-04 7:41 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-14 9:27 [U-Boot] [PATCH V2] spl: Weed out CONFIG_SYS_TEXT_BASE usage Marek Vasut
2018-08-14 9:51 ` Simon Goldschmidt
2018-08-14 10:01 ` Marek Vasut
2018-08-14 10:22 ` Simon Goldschmidt
2018-08-14 10:42 ` Marek Vasut
2018-08-14 12:56 ` Simon Goldschmidt
2018-08-14 18:12 ` Marek Vasut
2018-08-14 18:25 ` Simon Goldschmidt
2018-08-15 7:52 ` Marek Vasut
2018-08-15 9:04 ` Simon Goldschmidt
2018-09-26 12:42 ` [U-Boot] [U-Boot,V2] " Tom Rini
2018-10-03 13:39 ` [U-Boot] [PATCH V2] " Michal Simek
2018-10-03 19:41 ` Marek Vasut
2018-10-04 6:00 ` Michal Simek
2018-10-04 7:02 ` Marek Vasut
2018-10-04 7:41 ` Michal Simek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.