[coreboot] [patch] SST25VF016B (2MB) flash on m57sli (IT8716F).

[Ronald Hoogenboom]

On Tue, 2008-03-11 at 01:12 +0100, Carl-Daniel Hailfinger wrote:
> On 10.03.2008 09:05, Ronald Hoogenboom wrote:
> > This patch allows direct out-of-SPI-flash boot of a Linux kernel. It
> > circumvents the 512KB limitation in the IT8716f superio of memory
> > mapping an SPI flash device by using a PIO method for reading the data.
> > Limitation: no nrv2b support.
> >
> > Signed-off-by: Ronald Hoogenboom <hoogenboom30 at zonnet.nl>
> >   
> 
> src/stream/piorom_stream.c:
> A missing license header.

I just imitated rom_stream.c, which hasn't got it either...

> 
> src/mainboard/gigabyte/m57sli/Options.lb:
> Changed to be SPI-only (rev 1.x boards will have problems).

Rev. 1.x boards would choose 512K rom, which changes the piorom_stream
back to rom_stream (see the very first #if in piorom_stream). I agree
this is ugly, but I couldn't make it work in the Options.lb with an 'if
ROM_SIZE > 512*1024' - like construct.

> CONFIG_VGA_ROM_RUN settings are added (does not belong into this patch).

True.

> 
> src/lib/lzma_cb.c:
> 
> This copies the incorrect copyright header from src/lib/lzma.c. I sent a
> patch to correct the copyright header (GPL v2 and later) more than half
> a year ago, IIRC Uwe vetoed the change because it did not introduce the
> long copyright+disclaimer header.

Where is a correct example of the header you would like to see?

> It would be great if you used sizeof() instead of sizeof because it
> helps readability.

That's a matter of taste. I never use brackets on sizeof (except for
types), because it makes it look like a function, which it isn't. But
conforming is easy...

> 
> static unsigned char scratchpad[15980];
> static CLzmaDecoderState state;
> static UInt32 outSize;
> 
> All three allocations were moved from the stack to rwdata. Is this
> intentional?

This was done, because I wanted to use the out-read mode of the lzma
decoder too, but that became too complex... I think they can be moved
back to the stack now, if that is prefered.

> 
> src/lib/lzmadecode.c:
> 
> You change the definition of RC_TEST for the normal case (different
> error code). Any reason for that?

No, this was just done while debugging and it remained there. I will
remove that.

> 
> I think the big buffer for streaming LZMA decompression is not allocated
> anywhere, but it's already late at night, so I may be mistaken.

rom_stream also just dumps the decompressed result at address 16*2^20,
so I figured that it would be OK if I did it too... (I must say, it made
me frown when I saw it, but hey, it's only a short-lived intermediate
and nobody else is using the memory, but coreboot...)

> Regards,
> Carl-Daniel
> 

Can you comment back, then I'll change things and recreate a patch.

Thanks,
Ronald.