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

[Daniel Schwierzeck]

Hi Stefan,

2012/8/24 Stefan Roese <sr at denx.de>:
> Hi Daniel,
>
> On 08/24/2012 01:49 PM, Daniel Schwierzeck wrote:
>>>> I still would like to move to my suggestion to not copy the header and
>>>> use the mkimage header values ih_load and ih_ep directly. Right now I
>>>> don't see any showstopper for doing it this way. I'll send a patch to
>>>> change this shortly (if everything works out).
>>>
>>> Hmmm. As it seems some SPL loading drivers (block like mmc, streaming
>>> like ymodem) are not that easily converted to skipping the header. So
>>> I'm not so sure if we should go this way after all...
>>>
>>> Comments?
>>
>> I did similar work for my upcoming Lantiq MIPS SoC port. In my
>> approach I also support compressed u-boot
>> images as payload (LZO, LZMA).
>
> Yes, I thought about adding this as well. Nice.
>
>> The u-boot image is merged with SPL
>> image without any padding or
>> fixed flash offsets to achieve a maximum reduction of flash footprint.
>
> Interesting. I'm still padding to the fixed offset. Let me look into
> squeezing those two images together as well...
>
>> I ended up with doing something like this:
>>
>> static int spl_parse_image(const image_header_t *hdr, struct spl_image *spl)
>> {
>>        u32 magic;
>>
>>        magic = image_get_magic(hdr);
>>        if (magic != IH_MAGIC)
>>                return -1;
>>
>>        spl->data_addr += image_get_header_size();
>>        spl->entry_addr = image_get_load(hdr);
>>        spl->size = image_get_data_size(hdr);
>>        spl->comp = image_get_comp(hdr);
>>
>>        if (spl->comp == IH_COMP_NONE)
>>                spl->load_addr = spl->entry_addr;
>>        else
>>                spl->load_addr = CONFIG_LOADADDR;
>>
>>        return 0;
>> }
>
> So you introduced a new "struct spl_image" and did not use "struct
> spl_image_info" from the SPL framework? Why is that? We should
> definitely consolidate to one structure here.

I implemented this after the initial SPL infrastructure got merged
into mainline one year ago.
At this time a generic SPL framework did not exist ;)

>
>> spl->data_addr points to the image header of the payload and is
>> initialized by the caller
>> dependent on the used load mechanism.
>
> Good idea. Looks like your spl_parse_image might be a potential
> replacement for spl_parse_image_header() from this common SPL framework.
>
>> If the payload is uncompressed
>> it can be directly copied to
>> its final RAM location. A compressed payload needs an intermediate
>> copy step if it is stored in SPI or NAND flash.
>> I chose CONFIG_LOADADDR. After this the decompression function
>> extracts the image to its final RAM location.
>
> Sounds good. Do you have some patches that you could send for review?
> Did you try to keep as much platform/arch independent as possible?

Not yet. I had not time yet to adopt my work to the new common SPL framework.

My current SPL solution is queued here:
http://dev.phrozen.org/gitweb/?p=uboot-upstream.git;a=commitdiff;h=39165fa145b2d959f1eaa6faa3ab3053823bb985

>
>> I think we should keep an architecture-neutral SPL image context
>> inside the generic SPL framework.
>> The content of this context should be filled by architecture/SoC/board
>> specific code.
>
> Ack. It would be great to review your code here. Please keep us informed.
>

Actually this was my response to your original question.
Quote:
> I still would like to move to my suggestion to not copy the header and
> use the mkimage header values ih_load and ih_ep directly. Right now I
> don't see any showstopper for doing it this way. I'll send a patch to
> change this shortly (if everything works out).

So I suggest to not remove the 'struct spl_image_info'.

-- 
Best regards,
Daniel