[U-Boot] [PATCH 3/6] SPL: Port SPL framework to powerpc

[Heiko Schocher <hs@denx.de>]

Hello Stefan

On 24.08.2012 10:17, Stefan Roese wrote:
> Hi Tom,
>
> On 08/23/2012 09:31 PM, Tom Rini wrote:
>>>>> @@ -89,7 +106,11 @@ void spl_parse_image_header(const struct image_header *header)
>>>>>   		spl_image.size = __be32_to_cpu(header->ih_size) + header_size;
>>>>>   		spl_image.entry_point = __be32_to_cpu(header->ih_load);
>>>>>   		/* Load including the header */
>>>>> +#ifdef CONFIG_ARM
>>>>>   		spl_image.load_addr = spl_image.entry_point - header_size;
>>>>> +#else
>>>>> +		spl_image.load_addr = __be32_to_cpu(header->ih_load);
>>>>> +#endif
>>>>
>>>> This isn't an ARM-ism but is instead because spl_nor.c isn't offsetting
>>>> where the header is like mmc/nand/ymodem do, yes?  Would it be possible
>>>> to make spl_nor.c behave like the others?  One of the reasons I ask is
>>>> I'm looking at a NOR chip on my desk...
>>>
>>> I was wondering about this line as well. Please explain: Why can't ARM
>>> just use header->ih_load as load_addr?
>>
>> Off the top of my head, I believe what goes on is that we read things
>> into SDRAM such that the header is taken into account and we don't need
>> to relocate the payload (U-Boot or Linux).
>
> Hmmm. So for example, when ih_load is set to 0x100000, then you load the
> image to (0x100000 - 0x40) = 0xfffc0? Is this correct?
>
> This can't work for powerpc. As here for Linux both load-address and
> entry-point are set to 0. So when loading the image (e.g. from NOR flash)
> can't copy the image header in front of the image.
>
> Another thing I'm wondering about: Why is only ih_load from the mkimage
> header used and not ih_ep (entry-point)?
>
> I suggest that we switch to copying the "real" image (payload) to the load
> address, skipping the header. Then "ih_load" and "ih_ep" can be used
> without modification.

Yep, this seems a good idea to me.

> BTW: There also seems to be a bug in some of the SPL loaders:
>
> For example in drivers/mtd/nand/nand_spl_load.c:
>
> 		...
> 		if (header->ih_os == IH_OS_LINUX) {
> 			/* happy - was a linux */
> 			nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
> 				spl_image.size, (void *)spl_image.load_addr);
>
> The problem here is that the last 64 bytes of the image are not
> copied to SDRAM. Since the header is copied which is not included
> in the spl_image.size variable. With my idea of only copying
> the payload (skipping the header) this would be:
>
> 			nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS +
> 				sizeof(struct image_header),
> 				spl_image.size, (void *)spl_image.load_addr);
>
> What do you think? Should we switch to this way of loading images?
> Seems more logical to me. And we don't run into problems where the
> load address is 0.

Yes, that should be the way to go ... @Simon: Do you see here some
reason for not switching to copy the real payload?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany