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

[Ronald Hoogenboom]

On Wed, 2008-02-20 at 23:21 +0100, Ronald Hoogenboom wrote:
> On Tue, 2008-02-19 at 23:34 -0500, Ward Vandewege wrote:
<...>
> > > -default MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID=0x1022
> > > -default MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID=0x2b80
> > > +default MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID=0x1458
> > > +default MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID=0xe000
> > 
> > Also unrelated to the rest of the patch? This does not seem to have any
> > effect on my box. Also; the proprietary bios uses 0x1022; why are you
> > changing this?
> > 
> My prop. BIOS does put the gigabyte subvendor ID in. When I changed it,
> I was only looking at MCP55 only (which gets the e000 DID from the prop.
> BIOS), I didn't realize the setting inpacted EVERY PCI card that can set
> subsystem ID's. Indeed it didn't have any effect with me either, but I
> do think that the 1458 vendor is more appropriate (for what that's
> worth...)
> 
I was totally bullshitting here... I changed it to make
'kudzu' (RedHat/Fedora's new-hardware detector) NOT re-configure my
network interface when booted coreboot. These are the values put in by
the proprietary bios for the PCI 00:08.0 (MCP55 Ethernet).

Ronald.



-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

Hi Ward,

Thanks for reviewing my patch. I'm glad to hear that it got you a
booting mainboard too.

Most of the seemingly unrelated changes are a result of my quest to get
my nvidia VGA to work. Also I'm quite new submitting patches and I'm not
yet very familiar with what stuff is allowed to be in a patch like this
and what not. Thanks for the advise in this.

 Please find my comments below.


On Tue, 2008-02-19 at 23:34 -0500, Ward Vandewege wrote:
> Hi Ronald,

> > Not so nice: hardcoded 0x820 SPI-IO port.
> 
> Is that superio specific? Maybe some sort of lookup table based on the
> superio in use would be a solution? At this point there would be just the one
> line I presume.
> 

I was thinking this port number should be snarfed from the
mainboard/gigabyte/m57sli/Config.lb file (see line 293: io 0x64 =
0x820), but I couldn't figure out how, so I hardcoded it to get going.

> > -   unsigned long index;
> > +   //unsigned long index;
> 
> What's this change for? Unrelated to the rest of the patch? If it's general
> cleanup, please do this in a separate patch.
> 
This was only to stop vim from going to that file every compile ;-).
Just to get rid of an 'unused variable' warning.


> > +uses CONFIG_VGA_ROM_RUN

> > -default CONFIG_PCI_ROM_RUN=1
> > +default CONFIG_VGA_ROM_RUN=1
> > +default CONFIG_PCI_ROM_RUN=0
> 
> Unrelated to the rest of the patch? I'm not sure I want this default to
> change - it's nice to have any PCI roms run by default.
> 
You are right. But it might be good to at least give the user the option
to choose to run only VGA ROM (get display going) and leave the rest to
the OS.

> > -default MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID=0x1022
> > -default MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID=0x2b80
> > +default MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID=0x1458
> > +default MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID=0xe000
> 
> Also unrelated to the rest of the patch? This does not seem to have any
> effect on my box. Also; the proprietary bios uses 0x1022; why are you
> changing this?
> 
My prop. BIOS does put the gigabyte subvendor ID in. When I changed it,
I was only looking at MCP55 only (which gets the e000 DID from the prop.
BIOS), I didn't realize the setting inpacted EVERY PCI card that can set
subsystem ID's. Indeed it didn't have any effect with me either, but I
do think that the 1458 vendor is more appropriate (for what that's
worth...)

> This is great work; if you can respond to the comments above I think I can
> ack this soon.
> 
Thanks, please let me know what to leave out definitively or what to put
in a separate patch and I'll send (a) new patch(es).

> Thanks,
> Ward.
> 
Regards,
Ronald.


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ward Vandewege]

Hi Ronald,

On Mon, Feb 18, 2008 at 10:45:23PM +0100, Ronald Hoogenboom wrote:
> Finally, now I know it's working (at least the part that I'm patching
> here...), here is the patch that uses PIO mode read from SPI rom with
> lzma decompression.
> 
> 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.

I've tested this on an m57sli-s4 (soic/spi revision) with a 2MB SPI chip, and
it boots. Great work!

> Not so nice: hardcoded 0x820 SPI-IO port.

Is that superio specific? Maybe some sort of lookup table based on the
superio in use would be a solution? At this point there would be just the one
line I presume.

> Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>

I won't pretend to understand everything, but I have a few comments, see
below.

> Index: src/southbridge/nvidia/mcp55/mcp55_lpc.c
> ===
> --- src/southbridge/nvidia/mcp55/mcp55_lpc.c  (revision 3103)
> +++ src/southbridge/nvidia/mcp55/mcp55_lpc.c  (working copy)
> @@ -243,7 +243,7 @@
>  static void mcp55_lpc_read_resources(device_t dev)
>  {
>   struct resource *res;
> - unsigned long index;
> + //unsigned long index;

What's this change for? Unrelated to the rest of the patch? If it's general
cleanup, please do this in a separate patch.



> Index: src/mainboard/gigabyte/m57sli/Options.lb
> ===
> --- src/mainboard/gigabyte/m57sli/Options.lb  (revision 3103)
> +++ src/mainboard/gigabyte/m57sli/Options.lb  (working copy)
> @@ -42,7 +42,7 @@
>  uses ROM_IMAGE_SIZE
>  uses ROM_SECTION_SIZE
>  uses ROM_SECTION_OFFSET
> -uses CONFIG_ROM_PAYLOAD
> +uses CONFIG_PIOROM_PAYLOAD
>  uses CONFIG_ROM_PAYLOAD_START
>  uses CONFIG_COMPRESSED_PAYLOAD_NRV2B
>  uses CONFIG_COMPRESSED_PAYLOAD_LZMA
> @@ -80,6 +80,7 @@
>  uses OBJCOPY
>  uses CONFIG_CHIP_NAME
>  uses CONFIG_CONSOLE_VGA
> +uses CONFIG_VGA_ROM_RUN
>  uses CONFIG_USBDEBUG_DIRECT
>  uses CONFIG_PCI_ROM_RUN
>  uses HW_MEM_HOLE_SIZEK
> @@ -211,7 +212,8 @@
>  
>  #VGA Console
>  default CONFIG_CONSOLE_VGA=1
> -default CONFIG_PCI_ROM_RUN=1
> +default CONFIG_VGA_ROM_RUN=1
> +default CONFIG_PCI_ROM_RUN=0

Unrelated to the rest of the patch? I'm not sure I want this default to
change - it's nice to have any PCI roms run by default.

>  #default CONFIG_USBDEBUG_DIRECT=1
>  
> @@ -253,8 +255,8 @@
>  ##
>  default MAINBOARD_PART_NUMBER="m57sli"
>  default MAINBOARD_VENDOR="GIGABYTE"
> -default MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID=0x1022
> -default MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID=0x2b80
> +default MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID=0x1458
> +default MAINBOARD_PCI_SUBSYSTEM_DEVICE_ID=0xe000

Also unrelated to the rest of the patch? This does not seem to have any
effect on my box. Also; the proprietary bios uses 0x1022; why are you
changing this?

This is great work; if you can respond to the comments above I think I can
ack this soon.

Thanks,
Ward.

-- 
Ward Vandewege <[EMAIL PROTECTED]>
Free Software Foundation - Senior System Administrator

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

Finally, now I know it's working (at least the part that I'm patching
here...), here is the patch that uses PIO mode read from SPI rom with
lzma decompression.

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.
Not so nice: hardcoded 0x820 SPI-IO port.

Best regards,
Ronald.

Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>
Index: src/southbridge/nvidia/mcp55/mcp55_lpc.c
===
--- src/southbridge/nvidia/mcp55/mcp55_lpc.c	(revision 3103)
+++ src/southbridge/nvidia/mcp55/mcp55_lpc.c	(working copy)
@@ -243,7 +243,7 @@
 static void mcp55_lpc_read_resources(device_t dev)
 {
 	struct resource *res;
-	unsigned long index;
+	//unsigned long index;
 
 	/* Get the normal pci resources of this device */
 	pci_dev_read_resources(dev); // We got one for APIC, or one more for TRAP
Index: src/stream/Config.lb
===
--- src/stream/Config.lb	(revision 3103)
+++ src/stream/Config.lb	(working copy)
@@ -1,9 +1,18 @@
 uses CONFIG_ROM_PAYLOAD
+uses CONFIG_PIOROM_PAYLOAD
 uses CONFIG_IDE_PAYLOAD
 uses CONFIG_FS_PAYLOAD
 uses CONFIG_IDE
 uses CONFIG_SERIAL_PAYLOAD
 
+if CONFIG_PIOROM_PAYLOAD
+#  if (ROM_SIZE > 512*1024)
+object piorom_stream.o
+#  else
+#object rom_stream.o
+#  end
+end
+
 if CONFIG_ROM_PAYLOAD
   object rom_stream.o
 end
Index: src/stream/piorom_stream.c
===
--- src/stream/piorom_stream.c	(revision 0)
+++ src/stream/piorom_stream.c	(revision 0)
@@ -0,0 +1,197 @@
+#if ROM_SIZE <= 512*1024
+#include "rom_stream.c"
+#else
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* if they set the precompressed rom stream, they better have set a type */
+#if CONFIG_PRECOMPRESSED_PAYLOAD && ((!CONFIG_COMPRESSED_PAYLOAD_NRV2B) && (!CONFIG_COMPRESSED_PAYLOAD_LZMA))
+#error "You set CONFIG_PRECOMPRESSED_PAYLOAD but need to set CONFIG_COMPRESSED_PAYLOAD_NRV2B or CONFIG_COMPRESSED_PAYLOAD_LZMA"
+#endif
+
+/* If they set ANY of these, then we're compressed */
+#if ((CONFIG_COMPRESSED_PAYLOAD_NRV2B) || (CONFIG_COMPRESSED_PAYLOAD_LZMA))
+#define UNCOMPRESSER 1
+extern unsigned char _heap, _eheap;
+
+/* this here is it8716 specific */
+#include 
+
+uint16_t port = 0x820;
+uint32_t address;
+#define IN_CHUNK_SIZE 3
+#define JEDEC_READ	0x03
+static void init_pioread(void *src)
+{
+	uint8_t busy, writeenc;
+	int size;
+
+	address=(uint32_t)src;
+	do {
+		busy = inb(port) & 0x80;
+	} while (busy);
+
+	outb(JEDEC_READ, port + 1);
+	outb((address >> 0) & 0xff, port + 2);
+	outb((address >> 8) & 0xff, port + 3);
+	outb((address >> 16) & 0xff, port + 4);
+	size=3;
+	writeenc = 0x2;
+	outb(((0x4) << 4) | (size << 2) | (writeenc), port);
+}
+
+static void get_chunk(uint8_t *dest, int size)
+{
+	uint8_t busy, writeenc;
+	int i;
+
+	size&=3;
+	do {
+		busy = inb(port) & 0x80;
+	} while (busy);
+
+	for (i = 0; i < size; i++) {
+		dest[i] = inb(port + 5 + i);
+	}
+	address+=size;
+
+	outb(JEDEC_READ, port + 1);
+	outb((address >> 0) & 0xff, port + 2);
+	outb((address >> 8) & 0xff, port + 3);
+	outb((address >> 16) & 0xff, port + 4);
+	size=3;
+	writeenc = 0x2;
+	outb(((0x4) << 4) | (size << 2) | (writeenc), port);
+}
+#endif
+
+#if (CONFIG_COMPRESSED_PAYLOAD_NRV2B) 
+#define HAVE_UNCOMPRESSER 1
+// include generic nrv2b
+#error "nrv2b is not supported for piorom (yet...?)"
+#include "../lib/nrv2b.c"
+#endif
+
+#if (CONFIG_COMPRESSED_PAYLOAD_LZMA)
+#if HAVE_UNCOMPRESSER
+#error "You're defining more than one compression type, which is not allowed (of course)"
+#endif
+#define HAVE_UNCOMPRESSER 1
+// include generic lzma with callback
+#include "../lib/lzma_cb.c"
+#endif
+
+#ifndef CONFIG_ROM_PAYLOAD_START
+#define CONFIG_ROM_PAYLOAD_START 0xUL
+#endif
+
+/* well, this is a mess, and it will get fixed, but not right away.
+ * until we stop using 'ld' for building the rom image, that is.
+ * problem is, that on the sc520, ROM_PAYLOAD_START has to be at 0x200.
+ * but if you set CONFIG_ROM_PAYLOAD_START to that, then ld will try to
+ * build a giant image: 0x0-0x200, i.e. almost 4 GB.
+ * so make this non-static, non-const for now.
+ */
+
+/*XX */
+/*static const */unsigned char *rom_start = (unsigned char *)CONFIG_ROM_PAYLOAD_START;
+/*static const */unsigned char *rom_end   = (unsigned char *)(CONFIG_ROM_PAYLOAD_START + PAYLOAD_SIZE - 1);
+/*XX */
+
+static const unsigned char *rom;
+
+#if UNCOMPRESSER
+static inline unsigned long 
+uncompress(uint8_t * rom_start, uint8_t *dest )
+{
+	init_pioread(rom_start);
+#if (CONFIG_COMPRESSED_PAYLOAD_NRV2B) 
+	unsigned long ilen; // used compressed stream length
+	return unrv2b(rom_start, dest, &ilen);
+#endif
+#if (CONFIG_COMPRESSED_PAYLOAD_LZMA)
+	return ulzma(dest);
+#endif
+}
+

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

[Ronald Hoogenboom]

Carl-Daniel Hailfinger wrote:

>Harald: This patch should fix your problems writing to the chip. Use
>either "patch -l" or remove the // before
>//while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>
>Ronald: I need an ack to commit this.
>
>On 20.01.2008 11:59, Ronald Hoogenboom wrote:
>  
>
>>Carl-Daniel Hailfinger wrote:
>>
>>  
>>
>>
>>>Did you have time to test with the while
>>>(generic_spi_read_status_register() & .._WIP) enabled? It would be more
>>>correct to have it enabled.
>>>
>>>
>>>  
>>>
>>I had that in initially (that's why it was there, but commented out) but 
>>all it got me was slower programming times and the datasheet also 
>>suggests it is ok to just do a timed wait. But in the face of 
>>genericness for other chips, it is indeed more correct to do the check 
>>for the busy bit.
>>
>>
>
>Ronald/Harald, can you please ack the change? It is reproduced below
>(whichspace-damaged).
>
>Regards,
>Carl-Daniel
>
>Make sure we delay writing the next byte long enough in SPI byte
>programming.
>Minor formatting changes.
>
>Signed-off-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]>
>
>  
>
Acked-by: Ronald Hoogenboom <[EMAIL PROTECTED]>


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 22.01.2008 03:53, ron minnich wrote:
> Just FYI, some luck writing this chip on the sis board ...
>
> The write had no errors. The -v failed. But on using the -r to read to
> a file, I find
> no differences between the file used for -w and the file used for -v!
>   

Yes, very old lingering bug in the verify code. Fixed with the patch I
just sent to the list.

> Next is to try to actually put the vga bios onto the image and see if,
> this time, I get something.
>   

Should work.

> I brought a post card home so I'm hoping to see something.
>   

Good luck!


Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 22.01.2008 12:08, Harald Gutmann wrote:
> Am Dienstag, 22. Januar 2008 01:09:09 schrieben Sie:
>   
>> Harald: This patch should fix your problems writing to the chip. Use
>> either "patch -l" or remove the // before
>> //while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>> 
> I've updated my flashrom tree to revision 3068 and commented that line out.
> Now i can flash the chip, like Ronald says. I wrote the chip, and read the 
> file again, and compared the start and the end of the file with 
> hexdump/head/tail and diff. On the first/last 5 lines there is no 
> difference between the file which was written to the chip, and the file i've 
> read with flashrom from the chip.
>
> Any idea why verifying fails?
>   

Yes. Will send a patch.

>> Ronald: I need an ack to commit this.
>>
>> On 20.01.2008 11:59, Ronald Hoogenboom wrote:
>> 
>>> Carl-Daniel Hailfinger wrote:
>>>   
 Did you have time to test with the while
 (generic_spi_read_status_register() & .._WIP) enabled? It would be more
 correct to have it enabled.
 
>>> I had that in initially (that's why it was there, but commented out) but
>>> all it got me was slower programming times and the datasheet also
>>> suggests it is ok to just do a timed wait. But in the face of
>>> genericness for other chips, it is indeed more correct to do the check
>>> for the busy bit.
>>>   
>> Ronald/Harald, can you please ack the change? It is reproduced below
>> (whichspace-damaged).
>>
>> Regards,
>> Carl-Daniel
>>
>> Make sure we delay writing the next byte long enough in SPI byte
>> programming.
>> Minor formatting changes.
>>
>> Signed-off-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]>
>> 
> Acked-by: Harald Gutmann <[EMAIL PROTECTED]>
>   

Thanks, r3069.

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Harald Gutmann]

Am Dienstag, 22. Januar 2008 01:09:09 schrieben Sie:
> Harald: This patch should fix your problems writing to the chip. Use
> either "patch -l" or remove the // before
> //while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
I've updated my flashrom tree to revision 3068 and commented that line out.
Now i can flash the chip, like Ronald says. I wrote the chip, and read the 
file again, and compared the start and the end of the file with 
hexdump/head/tail and diff. On the first/last 5 lines there is no 
difference between the file which was written to the chip, and the file i've 
read with flashrom from the chip.

Any idea why verifying fails?


> Ronald: I need an ack to commit this.
>
> On 20.01.2008 11:59, Ronald Hoogenboom wrote:
> > Carl-Daniel Hailfinger wrote:
> >> Did you have time to test with the while
> >> (generic_spi_read_status_register() & .._WIP) enabled? It would be more
> >> correct to have it enabled.
> >
> > I had that in initially (that's why it was there, but commented out) but
> > all it got me was slower programming times and the datasheet also
> > suggests it is ok to just do a timed wait. But in the face of
> > genericness for other chips, it is indeed more correct to do the check
> > for the busy bit.
>
> Ronald/Harald, can you please ack the change? It is reproduced below
> (whichspace-damaged).
>
> Regards,
> Carl-Daniel
>
> Make sure we delay writing the next byte long enough in SPI byte
> programming.
> Minor formatting changes.
>
> Signed-off-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]>
Acked-by: Harald Gutmann <[EMAIL PROTECTED]>

>
>
> Index: spi.c
> ===
> --- spi.c (Revision 3068)
> +++ spi.c (Arbeitskopie)
> @@ -519,12 +519,8 @@
>   for (i=0; i   generic_spi_write_enable();
>   spi_byte_program(i,buf[i]);
> - /* FIXME: We really should read the status register and delay
> -  * accordingly.
> -  */
> - //while (generic_spi_read_status_register() & 
> JEDEC_RDSR_BIT_WIP)
> + while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>   myusec_delay(10);
>   //if (i%1024==0) fputc('b',stderr);
>   }
>
>
> Regards,
> Carl-Daniel
Regards, Harald


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[ron minnich]

Just FYI, some luck writing this chip on the sis board ...

The write had no errors. The -v failed. But on using the -r to read to
a file, I find
no differences between the file used for -w and the file used for -v!

Next is to try to actually put the vga bios onto the image and see if,
this time, I get something.

I brought a post card home so I'm hoping to see something.

ron

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

Harald: This patch should fix your problems writing to the chip. Use
either "patch -l" or remove the // before
//while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)

Ronald: I need an ack to commit this.

On 20.01.2008 11:59, Ronald Hoogenboom wrote:
> Carl-Daniel Hailfinger wrote:
>
>   
>> Did you have time to test with the while
>> (generic_spi_read_status_register() & .._WIP) enabled? It would be more
>> correct to have it enabled.
>>
>> 
> I had that in initially (that's why it was there, but commented out) but 
> all it got me was slower programming times and the datasheet also 
> suggests it is ok to just do a timed wait. But in the face of 
> genericness for other chips, it is indeed more correct to do the check 
> for the busy bit.

Ronald/Harald, can you please ack the change? It is reproduced below
(whichspace-damaged).

Regards,
Carl-Daniel

Make sure we delay writing the next byte long enough in SPI byte
programming.
Minor formatting changes.

Signed-off-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]>


Index: spi.c
===
--- spi.c   (Revision 3068)
+++ spi.c   (Arbeitskopie)
@@ -519,12 +519,8 @@
for (i=0; ihttp://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 21.01.2008 00:47, Ronald Hoogenboom wrote:
> On Mon, 2008-01-21 at 00:24 +0100, Ronald Hoogenboom wrote:
>   
>> (LPC is the only datapath to the SB and CPU, so it has to!). We could
>> optimize by omitting the wait for SPI ready when there is no data to be
>> read, eg. readcnt==0. I'll have a look at what can be gained by that.
>>
>> 
> Omitting it saves 10 seconds with the unconditional 10us delay, reducing
> to 40~45 secs. So I think it is worth putting it in. With the
> conditional delay, it is not so easily measurable, because of the
> natural variance much larger than 10 us...
> Patch follows...
>
> Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>
>   

Acked-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]>

Committed in r3068.

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

On Mon, 2008-01-21 at 00:24 +0100, Ronald Hoogenboom wrote:
> (LPC is the only datapath to the SB and CPU, so it has to!). We could
> optimize by omitting the wait for SPI ready when there is no data to be
> read, eg. readcnt==0. I'll have a look at what can be gained by that.
> 
Omitting it saves 10 seconds with the unconditional 10us delay, reducing
to 40~45 secs. So I think it is worth putting it in. With the
conditional delay, it is not so easily measurable, because of the
natural variance much larger than 10 us...
Patch follows...

Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>

---

Index: spi.c
===
--- spi.c	(revision 3066)
+++ spi.c	(working copy)
@@ -227,12 +227,14 @@
 	 * We can't use writecnt directly, but have to use a strange encoding.
 	 */ 
 	outb(((0x4 + (fast_spi ? 1 : 0)) << 4) | ((readcnt & 0x3) << 2) | (writeenc), port);
-	do {
-		busy = inb(port) & 0x80;
-	} while (busy);
-
-	for (i = 0; i < readcnt; i++) {
-		readarr[i] = inb(port + 5 + i);
+	if (readcnt > 0) {
+		do {
+			busy = inb(port) & 0x80;
+		} while (busy);
+		i=0;
+		do {
+			readarr[i] = inb(port + 5 + i);
+		} while (++i < readcnt);
 	}
 
 	return 0;
-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

On Sun, 2008-01-20 at 22:28 +0100, Carl-Daniel Hailfinger wrote:

> I think we can shorten the delay in the loop to about 5 us. However,
> even with 10 us delay we still can't explain the time needed to program
> the chip.
> In the case without status reading, we have exactly 5 bytes on the SPI
> bus for every byte written. At 16 MHz, 5 bytes take 2.5 us transfer

I think you are forgetting the enable-write command, needed before each
byte-program command. So that will be another 1 byte = 3.0 us total.
But if you look at the it8716f_spi_command function, that will also
before and after the command wait for the busy bit of the 8716's SPI
controller. These actions will generate extra traffic on the LPC bus
(LPC is the only datapath to the SB and CPU, so it has to!). We could
optimize by omitting the wait for SPI ready when there is no data to be
read, eg. readcnt==0. I'll have a look at what can be gained by that.

> time. Adding an unconditional 10 us delay (max. byte program time
> according to the datasheets) gives us a total of 12.5 us per byte
> written to flash. Looking at the time you needed to program the while
> flash chip, things don't add up. With 12.5 us per byte and 2 MByte chip
> size, the total programming time should be close to 25 seconds.
> That is half the time it takes in reality and tells us we have ~12.5 us
> overhead per transaction. This overhead can NOT be attributed to LPC
> cycles because we avoid LPC altogether in byte programming. Either there
> is some concurrent access to the ROM (SMM or other stuff) or the
> accumulated reads/writes to the I/O ports of the IT8716F take that long.
> 

IO MUST go through LPC, as that is the ONLY path to Southbridge.

> Can you use oprofile against the current code to find out where we spend
> all of that overhead?
> 
> > Maybe it would be nice to have some kind of progress bar, like in 'rpm
> > -Uhv', 'scp' or 'cdparanoia', at least while programming the flash,
> > especially when the programming times get over a few minutes, as can be
> > expected with these bigger and bigger serial flash devices.
> >   
> 
> Yes, this would be nice to have. Some chips already implement a per-page
> progress indicator.
> 
> > Maybe there should also be support for partial erasing and programming
> > (depending on what the device allows).
> >   
> 
> The functions are there for most chips, but we currently have no way to
> tell flashrom about sector sizes. Since quite a few SPI chips have
> non-uniform sector sizes, you have to store an array or sector sizes for
> each chip. That gets tedious quickly.
> 
> 
> Regards,
> Carl-Daniel


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 20.01.2008 12:48, Ronald Hoogenboom wrote:
> On Sun, 2008-01-20 at 11:59 +0100, Ronald Hoogenboom wrote:
>   
>> The read status register will take at least:
>> 16*(2/33) us = about 1 us (excluding the LPC latency, which is?), so 
>> assuming that the first read status will show busy and the second will 
>> show ready, it is only 2 us slower: ~20%, so that should be acceptable. 
>> I'll do some checks on the hardware shortly.
>>
>> 
> I've done the tests and the programming result (eg. the data read back
> >from the flash) is correct with and without the status reading.
> With status reading, programming takes:
> 1'15 ~ 1'50
> Without status reading, programming takes:
> 0'50 ~ 0'55
>   

Hm.

> There is quite some variance in the time taken extra by reading the
> status, the lower bound is about the expected penalty (as calculated
> above), but the higher bound is not so easily explainable without better
> understanding. If you think this performance hit is acceptable, then go
> for it, put the status reading back in, I would say.
>   

I think we can shorten the delay in the loop to about 5 us. However,
even with 10 us delay we still can't explain the time needed to program
the chip.
In the case without status reading, we have exactly 5 bytes on the SPI
bus for every byte written. At 16 MHz, 5 bytes take 2.5 us transfer
time. Adding an unconditional 10 us delay (max. byte program time
according to the datasheets) gives us a total of 12.5 us per byte
written to flash. Looking at the time you needed to program the while
flash chip, things don't add up. With 12.5 us per byte and 2 MByte chip
size, the total programming time should be close to 25 seconds.
That is half the time it takes in reality and tells us we have ~12.5 us
overhead per transaction. This overhead can NOT be attributed to LPC
cycles because we avoid LPC altogether in byte programming. Either there
is some concurrent access to the ROM (SMM or other stuff) or the
accumulated reads/writes to the I/O ports of the IT8716F take that long.

Can you use oprofile against the current code to find out where we spend
all of that overhead?

> Maybe it would be nice to have some kind of progress bar, like in 'rpm
> -Uhv', 'scp' or 'cdparanoia', at least while programming the flash,
> especially when the programming times get over a few minutes, as can be
> expected with these bigger and bigger serial flash devices.
>   

Yes, this would be nice to have. Some chips already implement a per-page
progress indicator.

> Maybe there should also be support for partial erasing and programming
> (depending on what the device allows).
>   

The functions are there for most chips, but we currently have no way to
tell flashrom about sector sizes. Since quite a few SPI chips have
non-uniform sector sizes, you have to store an array or sector sizes for
each chip. That gets tedious quickly.


Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

On Sun, 2008-01-20 at 11:59 +0100, Ronald Hoogenboom wrote:
> The read status register will take at least:
> 16*(2/33) us = about 1 us (excluding the LPC latency, which is?), so 
> assuming that the first read status will show busy and the second will 
> show ready, it is only 2 us slower: ~20%, so that should be acceptable. 
> I'll do some checks on the hardware shortly.
> 
I've done the tests and the programming result (eg. the data read back
from the flash) is correct with and without the status reading.
With status reading, programming takes:
1'15 ~ 1'50
Without status reading, programming takes:
0'50 ~ 0'55

There is quite some variance in the time taken extra by reading the
status, the lower bound is about the expected penalty (as calculated
above), but the higher bound is not so easily explainable without better
understanding. If you think this performance hit is acceptable, then go
for it, put the status reading back in, I would say.

Maybe it would be nice to have some kind of progress bar, like in 'rpm
-Uhv', 'scp' or 'cdparanoia', at least while programming the flash,
especially when the programming times get over a few minutes, as can be
expected with these bigger and bigger serial flash devices.

Maybe there should also be support for partial erasing and programming
(depending on what the device allows).

Ronald.


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

Carl-Daniel Hailfinger wrote:

>Did you have time to test with the while
>(generic_spi_read_status_register() & .._WIP) enabled? It would be more
>correct to have it enabled.
>
>  
>
I had that in initially (that's why it was there, but commented out) but 
all it got me was slower programming times and the datasheet also 
suggests it is ok to just do a timed wait. But in the face of 
genericness for other chips, it is indeed more correct to do the check 
for the busy bit.

The read status register will take at least:
16*(2/33) us = about 1 us (excluding the LPC latency, which is?), so 
assuming that the first read status will show busy and the second will 
show ready, it is only 2 us slower: ~20%, so that should be acceptable. 
I'll do some checks on the hardware shortly.

>Regards,
>Carl-Daniel
>  
>
Regards back,
Ronald.

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 18.01.2008 23:10, Ward Vandewege wrote:
> On Wed, Jan 16, 2008 at 10:34:29PM -0500, Ward Vandewege wrote:
>   
>>> Winbond W25X16VSSI
>>> Winbond W25X32VSSI
>>>
>>> Problem with Winbond is they really only want to sell large
>>> quantities. I haven't even gotten a quote and delivery time
>>> back from my sales rep on a request I made in November.
>>>   
>
> What about 
>
>   Winbond W25X80VSSIG
>
> ? That's an 8Mbit chip which is avaible in small quantities.
>   

Ronald Hoogenboom wrote that IT8716F startup clocks SPI at 16 MHz, so
almost any SPI chip should work. We just have to make sure we don't
accidentially change the speed of the IT8716F (which will happen if you
simply run flashrom).

So it is a case of "should work" for slower flash chips right now.
Changing that to "will work even if flashrom is invoked" requires some
flashrom surgery.

For the Winbond W25X80VSSIG everything should work fine. It is fast enough.

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 19.01.2008 19:34, Ronald Hoogenboom wrote:
> Carl-Daniel Hailfinger wrote:
>
>> */
>> -//while (generic_spi_read_status_register() &
>> JEDEC_RDSR_BIT_WIP)
>> +while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>> myusec_delay(10);
>> -//if (i%1024==0) fputc('b',stderr);
>> +if (i % 1024 == 0)
>> +printf("b");
>> }
>>  
>>
> There is a problem with using printf like this and that is: stdout is
> line buffered. The 'b''s won't show until a newline is output or
> fflush is called. That's why I used fputc initially.
> If you do this with the 'pio mode' writing, you should do this for all
> methods an all chips for constistent behaviour, so that is why I
> commented it out initially.

Ah OK. I'll drop that part.
Did you have time to test with the while
(generic_spi_read_status_register() & .._WIP) enabled? It would be more
correct to have it enabled.

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

Carl-Daniel Hailfinger wrote:

> */
>-  //while (generic_spi_read_status_register() & 
>JEDEC_RDSR_BIT_WIP)
>+  while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>   myusec_delay(10);
>-  //if (i%1024==0) fputc('b',stderr);
>+  if (i % 1024 == 0)
>+  printf("b");
>   }
>  
>
There is a problem with using printf like this and that is: stdout is 
line buffered. The 'b''s won't show until a newline is output or fflush 
is called. That's why I used fputc initially.
If you do this with the 'pio mode' writing, you should do this for all 
methods an all chips for constistent behaviour, so that is why I 
commented it out initially.

Ronald.

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 19.01.2008 01:23, Peter Stuge wrote:
> On Sat, Jan 19, 2008 at 01:18:11AM +0100, Carl-Daniel Hailfinger wrote:
>   
>> +while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>>  myusec_delay(10);
>> 
>
> Indent this properly, please.
>   

Will be done in the commit. The patch I posted was excluding whitespace
changes to facilitate review.

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Peter Stuge]

On Sat, Jan 19, 2008 at 01:18:11AM +0100, Carl-Daniel Hailfinger wrote:
> + while (generic_spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>   myusec_delay(10);

Indent this properly, please.


//Peter

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 19.01.2008 01:08, Carl-Daniel Hailfinger wrote:
> On 19.01.2008 00:17, Ronald Hoogenboom wrote:
>   
>> On Fri, 2008-01-18 at 23:31 +0100, Carl-Daniel Hailfinger wrote:
>>   
>> 
>>> Thanks for reworking the code! I have factored out some common status
>>> registers to duplicate less code and hope the code still works. Could
>>> you please review it and tell me what you think?
>>>
>>> 
>>>   
>> Fine by me, works like a charm, the way I meant it to work ;-).
>>   
>> 
>
> Thanks, checked in and added you as copyright holder. Revision 3061.
>   

What about the following patch on top of it?

Make sure we delay writing the next byte long enough in SPI byte
programming. This could even result in a speedup.
Print a "b" for every kByte written to flash.
Minor formatting changes.

Signed-off-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]>

Index: spi.c
===
--- spi.c   (Revision 3061)
+++ spi.c   (Arbeitskopie)
@@ -516,12 +516,10 @@
for (i=0; i 512 * 1024) {
for (i = 0; i < total_size; i+=3) {
int toread=3;
-   if (total_size-i < toread) toread=total_size-i;
+   if (total_size - i < toread)
+   toread = total_size - i;
spi_3byte_read(i, buf+i, toread);
}
} else {


Regards,
Carl-Daniel


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 19.01.2008 00:17, Ronald Hoogenboom wrote:
> On Fri, 2008-01-18 at 23:31 +0100, Carl-Daniel Hailfinger wrote:
>   
>> Thanks for reworking the code! I have factored out some common status
>> registers to duplicate less code and hope the code still works. Could
>> you please review it and tell me what you think?
>>
>> 
> Fine by me, works like a charm, the way I meant it to work ;-).
>   

Thanks, checked in and added you as copyright holder. Revision 3061.

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

On Fri, 2008-01-18 at 23:31 +0100, Carl-Daniel Hailfinger wrote:
> Thanks for reworking the code! I have factored out some common status
> registers to duplicate less code and hope the code still works. Could
> you please review it and tell me what you think?
> 
Fine by me, works like a charm, the way I meant it to work ;-).

Ronald.


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 18.01.2008 21:48, Ronald Hoogenboom wrote:
> I think this is a matter of taste... But if the style is like this, I
> will comply. So I changed my patch again to do exactly the same, but
> with a few JEDEC_... defines.
>
>   
> Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>
>   

Thanks for reworking the code! I have factored out some common status
registers to duplicate less code and hope the code still works. Could
you please review it and tell me what you think?

Ron (Minnich), can you test with your GA-2761GXDK?

Regards,
Carl-Daniel
---
Support SPI flash chips bigger than 512 kByte sitting behind IT8716F
Super I/O performing LPC-to-SPI flash translation.

Signed-off-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]>
Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>


Index: flashrom-big/flash.h
===
--- flashrom-big/flash.h(Revision 3051)
+++ flashrom-big/flash.h(Arbeitskopie)
@@ -234,7 +234,16 @@
 
 #define TI_ID  0x97/* Texas Instruments */
 
+/*
+ * W25X chips are SPI, first byte of device ID is memory type, second
+ * byte of device ID is related to log(bitsize).
+ */
 #define WINBOND_ID 0xDA/* Winbond */
+#define WINBOND_NEX_ID 0xEF/* Winbond (ex Nexcom) serial flash 
devices */
+#define W_25X100x3011
+#define W_25X200x3012
+#define W_25X400x3013
+#define W_25X800x3014
 #define W_29C011   0xC1
 #define W_29C020C  0x45
 #define W_29C040P  0x46
@@ -297,6 +306,7 @@
 void generic_spi_write_disable();
 int generic_spi_chip_erase_c7(struct flashchip *flash);
 int generic_spi_chip_write(struct flashchip *flash, uint8_t *buf);
+int generic_spi_chip_read(struct flashchip *flash, uint8_t *buf);
 
 /* 82802ab.c */
 int probe_82802ab(struct flashchip *flash);
Index: flashrom-big/flashchips.c
===
--- flashrom-big/flashchips.c   (Revision 3051)
+++ flashrom-big/flashchips.c   (Arbeitskopie)
@@ -51,13 +51,13 @@
{"MX29F002",MX_ID,  MX_29F002,  256, 64 * 1024,
 probe_29f002,  erase_29f002,   write_29f002},
{"MX25L4005",   MX_ID,  MX_25L4005, 512, 256,
-probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write},
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
{"MX25L8005",   MX_ID,  MX_25L8005, 1024, 256,
-probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write},
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
{"SST25VF040B", SST_ID, SST_25VF040B,   512,256,
-   probe_spi,  generic_spi_chip_erase_c7,  generic_spi_chip_write},
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
{"SST25VF016B", SST_ID, SST_25VF016B,   2048,   256,
-   probe_spi,  generic_spi_chip_erase_c7,  generic_spi_chip_write},
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
{"SST29EE020A", SST_ID, SST_29EE020A,   256, 128,
 probe_jedec,   erase_chip_jedec, write_jedec},
{"SST28SF040A", SST_ID, SST_28SF040,512, 256,
@@ -122,6 +122,14 @@
 probe_jedec,   erase_chip_jedec, write_39sf020},
{"W39V080A",WINBOND_ID, W_39V080A,  1024, 64*1024,
 probe_jedec,   erase_chip_jedec, write_39sf020},
+   {"W25x10",  WINBOND_NEX_ID, W_25X10,128, 256, 
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
+   {"W25x20",  WINBOND_NEX_ID, W_25X20,256, 256, 
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
+   {"W25x40",  WINBOND_NEX_ID, W_25X40,512, 256, 
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
+   {"W25x80",  WINBOND_NEX_ID, W_25X80,1024, 256, 
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
{"M29F002B",ST_ID,  ST_M29F002B,256, 64 * 1024,
 probe_jedec,   erase_chip_jedec, write_jedec},
{"M50FW040",ST_ID,  ST_M50FW040,512, 64 * 1024,
@@ -151,23 +159,23 @@
{"M29F040B",ST_ID,  ST_M29F040B,512, 64 * 1024,
 probe_29f040b, erase_29f040b,  write_29f040b},
{"M25P05-A",ST_ID,  ST_M25P05A, 64, 256,
-probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write},
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic

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

[Ward Vandewege]

On Wed, Jan 16, 2008 at 10:34:29PM -0500, Ward Vandewege wrote:
> > Winbond W25X16VSSI
> > Winbond W25X32VSSI
> > 
> > Problem with Winbond is they really only want to sell large
> > quantities. I haven't even gotten a quote and delivery time
> > back from my sales rep on a request I made in November.

What about 

  Winbond W25X80VSSIG

? That's an 8Mbit chip which is avaible in small quantities.

Thanks,
Ward.
 
-- 
Ward Vandewege <[EMAIL PROTECTED]>
Free Software Foundation - Senior System Administrator

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 18.01.2008 21:09, Ronald Hoogenboom wrote:
> OK, I've checked it again and fixed a few things, see the read function
> and the erase also has the block protect disable, and it works again.
>   

Thanks! It seems I was half asleep while coding the if (size > 512k) part.

> It takes just under a minute to flash the whole chip, which is still
> quite acceptable, I would say. It took WAY longer when the byte write
> was timed by usleep instead of the myusec_delay.
>   

Yes, speed was not the primary concern when developing the code, it was
mostly about getting the code into a working state with maximum
adherence to the datasheets.

> Are there any more chips that need the block protect disable?
> Are these block protect bits always at the same position in the status
> register?
>   

As far as I know, almost all chips have the block protect bits in the
same place in the status register. I can verify that later with some of
the data sheets on my disk.


Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

On Fri, 2008-01-18 at 09:39 -0800, ron minnich wrote:
> On Jan 18, 2008 6:49 AM, Carl-Daniel Hailfinger
> > should work as well and fit a little better into flashrom structure.
> 
> The top 512 flash fine, the lower 1.5M did not flash, but did erase,
> maybe. hard to say if
> readback worked.
> 

That is because the test for 512k+ was the wrong way around in the read
function.
Non-mapped memory also reads as ff (same as erased) using mmap method.
The erase also needed the Block Protection to be disabled, on power-up
the Block protection defaults to on (but, maybe not with the chips on
your board.. What type?).
See my re-worked and re-re-worked patches.

> coreboot appears to not have booted, sadly.
> 
> ron


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

Thanks for the explanations, however vague ;-). I thought I was the only
one wandering in the dark..

On Thu, 2008-01-17 at 23:38 -0800, ron minnich wrote:
> other indexes, there is. Does this mean that the superio.c file fails
> to
> > configure this io port?
> 
> yes, it means that the port is not configured. There is a lot of TODO
> in this file.
> It may be time to fill it in.
> 
> 
Might this also be the cause of the NVIDIA X11 driver not working yet?

Can you just give me a push in the right direction how to 'fill it in'?
How can I configure the SPI io port in LDN 07, index 0x64 and 0x65 and
what io address to configure it to? Can I just pick 0x820 like the
vendor bios does? Or do I ask it to the pnp functions (I guess)? What
pnp function to use? Is there some more complete example superio chip
that does similar things to do the copycat monkey-learns-trick?

Hope the 1/2 day off was worth it...

Ronald.


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

On Fri, 2008-01-18 at 03:13 +0100, Carl-Daniel Hailfinger wrote:

> I was unclear. I mean stuff like this:
> 
> void spi_write_status_register(int status)
> {
> const unsigned char cmd[] = {0x01,(unsigned char)status};

> would probably be more readable if it looked like this:
> 
> void spi_write_status_register(int status)
> {
> const unsigned char cmd[] = {JEDEC_WRSR,(unsigned char)status};
> /* Send WRSR (Write Status Register) */
> generic_spi_command(JEDEC_WRSR_OUTSIZE, JEDEC_WRSR_INSIZE, cmd,
> NULL);
> }
> 

I think this is a matter of taste... But if the style is like this, I
will comply. So I changed my patch again to do exactly the same, but
with a few JEDEC_... defines.

> Regards,
> Carl-Daniel

Regards,
Ronald

Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>

---

Index: flashrom/flash.h
===
--- flashrom/flash.h	(revision 3051)
+++ flashrom/flash.h	(working copy)
@@ -234,7 +234,16 @@
 
 #define TI_ID			0x97	/* Texas Instruments */
 
+/*
+ * W25X chips are SPI, first byte of device ID is memory type, second
+ * byte of device ID is related to log(bitsize).
+ */
 #define WINBOND_ID		0xDA	/* Winbond */
+#define WINBOND_NEX_ID		0xEF	/* Winbond (ex Nexcom) serial flash devices */
+#define W_25X10			0x3011
+#define W_25X20			0x3012
+#define W_25X40			0x3013
+#define W_25X80			0x3014
 #define W_29C011		0xC1
 #define W_29C020C		0x45
 #define W_29C040P		0x46
@@ -297,6 +306,7 @@
 void generic_spi_write_disable();
 int generic_spi_chip_erase_c7(struct flashchip *flash);
 int generic_spi_chip_write(struct flashchip *flash, uint8_t *buf);
+int generic_spi_chip_read(struct flashchip *flash, uint8_t *buf);
 
 /* 82802ab.c */
 int probe_82802ab(struct flashchip *flash);
Index: flashrom/flashchips.c
===
--- flashrom/flashchips.c	(revision 3051)
+++ flashrom/flashchips.c	(working copy)
@@ -51,13 +51,13 @@
 	{"MX29F002",	MX_ID,		MX_29F002,	256, 64 * 1024,
 	 probe_29f002,	erase_29f002, 	write_29f002},
 	{"MX25L4005",	MX_ID,		MX_25L4005,	512, 256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"MX25L8005",	MX_ID,		MX_25L8005,	1024, 256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"SST25VF040B",	SST_ID,		SST_25VF040B,	512,	256,
-	probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"SST25VF016B",	SST_ID,		SST_25VF016B,	2048,	256,
-	probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"SST29EE020A", SST_ID,		SST_29EE020A,	256, 128,
 	 probe_jedec,	erase_chip_jedec, write_jedec},
 	{"SST28SF040A", SST_ID,		SST_28SF040,	512, 256,
@@ -122,6 +122,14 @@
 	 probe_jedec,   erase_chip_jedec, write_39sf020},
 	{"W39V080A", 	WINBOND_ID, 	W_39V080A,	1024, 64*1024,
 	 probe_jedec,	erase_chip_jedec, write_39sf020},
+	{"W25x10",	WINBOND_NEX_ID, W_25X10,	128, 256, 
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
+	{"W25x20",	WINBOND_NEX_ID, W_25X20,	256, 256, 
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
+	{"W25x40",	WINBOND_NEX_ID, W_25X40,	512, 256, 
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
+	{"W25x80",	WINBOND_NEX_ID, W_25X80,	1024, 256, 
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M29F002B",	ST_ID, 		ST_M29F002B,	256, 64 * 1024,
 	 probe_jedec,	erase_chip_jedec, write_jedec},
 	{"M50FW040",	ST_ID, 		ST_M50FW040,	512, 64 * 1024,
@@ -151,23 +159,23 @@
 	{"M29F040B",	ST_ID, 		ST_M29F040B,	512, 64 * 1024,
 	 probe_29f040b, erase_29f040b,	write_29f040b},
 	{"M25P05-A",	ST_ID,		ST_M25P05A,	64,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P10-A",	ST_ID,		ST_M25P10A,	128,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P20",	ST_ID,		ST_M25P20,	256,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P40",	ST_ID,		ST_M25P40,	512,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P80",	ST_ID,		ST_M25P80,	1024,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_

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

[Ronald Hoogenboom]

OK, I've checked it again and fixed a few things, see the read function
and the erase also has the block protect disable, and it works again.

It takes just under a minute to flash the whole chip, which is still
quite acceptable, I would say. It took WAY longer when the byte write
was timed by usleep instead of the myusec_delay.

Are there any more chips that need the block protect disable?
Are these block protect bits always at the same position in the status
register?

Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>

---

Index: flashrom/flash.h
===
--- flashrom/flash.h	(revision 3051)
+++ flashrom/flash.h	(working copy)
@@ -234,7 +234,16 @@
 
 #define TI_ID			0x97	/* Texas Instruments */
 
+/*
+ * W25X chips are SPI, first byte of device ID is memory type, second
+ * byte of device ID is related to log(bitsize).
+ */
 #define WINBOND_ID		0xDA	/* Winbond */
+#define WINBOND_NEX_ID		0xEF	/* Winbond (ex Nexcom) serial flash devices */
+#define W_25X10			0x3011
+#define W_25X20			0x3012
+#define W_25X40			0x3013
+#define W_25X80			0x3014
 #define W_29C011		0xC1
 #define W_29C020C		0x45
 #define W_29C040P		0x46
@@ -297,6 +306,7 @@
 void generic_spi_write_disable();
 int generic_spi_chip_erase_c7(struct flashchip *flash);
 int generic_spi_chip_write(struct flashchip *flash, uint8_t *buf);
+int generic_spi_chip_read(struct flashchip *flash, uint8_t *buf);
 
 /* 82802ab.c */
 int probe_82802ab(struct flashchip *flash);
Index: flashrom/flashchips.c
===
--- flashrom/flashchips.c	(revision 3051)
+++ flashrom/flashchips.c	(working copy)
@@ -51,13 +51,13 @@
 	{"MX29F002",	MX_ID,		MX_29F002,	256, 64 * 1024,
 	 probe_29f002,	erase_29f002, 	write_29f002},
 	{"MX25L4005",	MX_ID,		MX_25L4005,	512, 256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"MX25L8005",	MX_ID,		MX_25L8005,	1024, 256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"SST25VF040B",	SST_ID,		SST_25VF040B,	512,	256,
-	probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"SST25VF016B",	SST_ID,		SST_25VF016B,	2048,	256,
-	probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"SST29EE020A", SST_ID,		SST_29EE020A,	256, 128,
 	 probe_jedec,	erase_chip_jedec, write_jedec},
 	{"SST28SF040A", SST_ID,		SST_28SF040,	512, 256,
@@ -122,6 +122,14 @@
 	 probe_jedec,   erase_chip_jedec, write_39sf020},
 	{"W39V080A", 	WINBOND_ID, 	W_39V080A,	1024, 64*1024,
 	 probe_jedec,	erase_chip_jedec, write_39sf020},
+	{"W25x10",	WINBOND_NEX_ID, W_25X10,	128, 256, 
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
+	{"W25x20",	WINBOND_NEX_ID, W_25X20,	256, 256, 
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
+	{"W25x40",	WINBOND_NEX_ID, W_25X40,	512, 256, 
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
+	{"W25x80",	WINBOND_NEX_ID, W_25X80,	1024, 256, 
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M29F002B",	ST_ID, 		ST_M29F002B,	256, 64 * 1024,
 	 probe_jedec,	erase_chip_jedec, write_jedec},
 	{"M50FW040",	ST_ID, 		ST_M50FW040,	512, 64 * 1024,
@@ -151,23 +159,23 @@
 	{"M29F040B",	ST_ID, 		ST_M29F040B,	512, 64 * 1024,
 	 probe_29f040b, erase_29f040b,	write_29f040b},
 	{"M25P05-A",	ST_ID,		ST_M25P05A,	64,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P10-A",	ST_ID,		ST_M25P10A,	128,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P20",	ST_ID,		ST_M25P20,	256,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P40",	ST_ID,		ST_M25P40,	512,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P80",	ST_ID,		ST_M25P80,	1024,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P16",	ST_ID,		ST_M25P16,	2048,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
 	{"M25P32",	ST_ID,		ST_M25P32,	40

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

[Ronald Hoogenboom]

On Fri, 2008-01-18 at 03:00 +0100, Carl-Daniel Hailfinger wrote:
> On 17.01.2008 22:25, Ronald Hoogenboom wrote:
...
> > the limited io transfer length options of the IT8716). It might be more
> > appropriate to have an 'over512k_page_write' for more speed with chips
> > that do support that. Also the byte program wait time is hard wired to
> > 10 us (following the 25vf016b datasheet). And the block protect
> > enablement might also NOT be so generic.
> >   
> 
> By the way, the lack of block-write in the data sheet does not mean it
> will not work with the real chip.
> 
No, it doesn't work! Way back when the mmapped mode was stil in
flashboot ... then the program only wrote a few bytes here and there in
the area > 0x18, so this doesn't work properly, like the datasheet
sais. Not all datasheets are false...

> 
I'll have a look if it still works after your rework to the patch.

Ronald.


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[ron minnich]

On Jan 18, 2008 6:49 AM, Carl-Daniel Hailfinger
<[EMAIL PROTECTED]> wrote:

> I have a mad scheme which would work for v3 without downsides (except
> for the fact that the boot block and initram have to be in the upper 512
> kB, but that's the case anyway even without my scheme).
> Basically, we create a flat LAR-in-LAR structure. Before you start to
> barf, listen:
> Instead of storing real flash size at the top of ROM, we store a virtual
> size which corresponds to the start of the first LAR member in the top
> 512 kB block. This virtual size is what all LAR parsing uses until CAR
> has been disabled. Then we shadow the entire ROM somewhere in RAM and
> further accesses to the ROM will be directed there, including all LAR
> functions.

That's fine, it's very simliar to what I had to do for the brain-dead
chip that had a 64K hole in FLASH at (0-64K) -- had to shadow it at
0x200. We're going to see this problem again and again, we might
as well figure it out now.

> Can you test the modified variant I posted as a reply to Ronald? It
> should work as well and fit a little better into flashrom structure.

The top 512 flash fine, the lower 1.5M did not flash, but did erase,
maybe. hard to say if
readback worked.

coreboot appears to not have booted, sadly.

ron

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 18.01.2008 08:17, ron minnich wrote:
> OK, SiS sent me this nice board with some 2 MB flash parts and I have
> not gotten much past step 1, "Program the flash", as it is a 2 MB
> flash. I had no idea what a mess the superio/flash interface was going
> to be.
>
> So, Ronald, I am most interested in your patch, hope it is ready soon!
>   

I think one or two more iterations and we should be ready to go.

> And, well, seems we have a problem with 2 MB parts. Ouch. I guess what
> I will do for now is use 512KB of the 2 MB part (i.e. program it with
> 4 512KB images).
>   

For now. We could certainly enable ROM shadowing in RAM if we can make
some RAM appear directly below 4 GB.

> I hate to say this, and I agree with the desire to remain clean in v3,
> but we may have to find a way to deal with this mess. I hope not.  It
> is hard to believe how brain-dead PC hardware can be, given that the
> same mistakes and lessons are learned (and forgotten) over and over
> again. Too bad.
>   

I have a mad scheme which would work for v3 without downsides (except
for the fact that the boot block and initram have to be in the upper 512
kB, but that's the case anyway even without my scheme).
Basically, we create a flat LAR-in-LAR structure. Before you start to
barf, listen:
Instead of storing real flash size at the top of ROM, we store a virtual
size which corresponds to the start of the first LAR member in the top
512 kB block. This virtual size is what all LAR parsing uses until CAR
has been disabled. Then we shadow the entire ROM somewhere in RAM and
further accesses to the ROM will be directed there, including all LAR
functions.

> Anyway I am ready to test the patch tomorrow morning, I have a 1/2
> vacation day and intend to put it to good use ...
>   

Can you test the modified variant I posted as a reply to Ronald? It
should work as well and fit a little better into flashrom structure.

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Corey Osgood]

ron minnich wrote:
> On Jan 17, 2008 2:32 PM, Ronald Hoogenboom <[EMAIL PROTECTED]> wrote:
>   
>> Now I'm looking at how to make the rom_stream read the flash chip like
>> in the over512k_read_chip.
>> But I'm a bit stuck on the overal mechanics of the initialization
>> process and how to get hold of the assigned io port for the SPI serial
>> flash controller in the IT8716.
>> This is supposed to be configured by the GPIO Config registers (LDN 07)
>> at index 0x64 and 0x65, but in the pnp_dev_info array in
>> it8716f/superio.c, there is nothing listed after IT8716F_GPIO, while at
>> other indexes, there is. Does this mean that the superio.c file fails to
>> configure this io port?
>> 
>
> yes, it means that the port is not configured. There is a lot of TODO
> in this file.
> It may be time to fill it in.
>
>   
>> Suppose that this port IS configured correcly, what means should my
>> rom_stream use to retrieve this port address?
>> 
>
> I think you make a new stream type, let's called it bad_stream, that knows
> how to handle the addresses. The bad_stream would know that when the
> address is in a certain range, i.e. below the address (0-512KB), it
> will use I/O, and when it is above, it can use memory access.
>
> So in src/stream/Config.lb
> you add something like this:
>
> uses CONFIG_BADROM_PAYLOAD
>
> if CONFIG_BADROM_PAYLOAD
>   object badrom_stream.o
> end
>
>   
>> Is there some doc about these pnp functions?
>> 
>
> no, sorry. We are doing better in v3 ... we know the lack of docs is a 
> problem.
>
>   
>> Suppose I've figured it all out and created my new rom_stream_over512k
>> (or so) object, how do I mingle it into the Config.lb files, so it will
>> automatically get chosen when a ROM_SIZE of over 512k is selected in the
>> target configuration (AND the superio is an IT8716F).
>> 
>
> I think you make those boards always use bad_stream (or whatever) in all 
> cases,
> and not try to get the config tool to pick different types, as in the
> example above.
>
> But, it might be possible to do this:
>
> if CONFIG_BADROM_PAYLOAD
>   if ROM_SIZE > 512*1024
>object badrom_stream.o
>   else
>   object rom_stream.o
>   end
> end
>
> Something like this might work.
>
> The it8761f is NOT the worst thing we have seen, it's just really, really bad.
>
> So I guess we have to put in another kludge for badly design chipsets.
> It's not the first time :-)
>
>   
>> In the targets/gigabyte/m57sli directory, there is a Config.lb.kernel
>> file, which I used as a base for my build, but it only uses a 'fallback'
>> and a 'failover' image. Why no 'normal' one? What is supposed to be the
>> purpose of each image? (You see, I'm really missing some key parts of
>> the big picture...)
>> 
>
> you're not missing anything. The picture, which was once, years ago,
> pretty clean, has gotten grafitti-tagged by many authors and the
> necessities of strange hardware.
>
> This will make no sense, but I'll try. Fallback is the image you can
> ALWAYS use, even when normal does not work. Normal is an image you can
> use unless something breaks, and you use fallback.
>
> So, what's the one image you must always have? fallback. What's the
> optional image? normal. So, when there is only going to be one image,
> it's called fallback.
>
> What people don't realize is that the images can have any name, so,
> for instance, on single image ROMs, they should really not use the
> name 'fallback' -- there is nothing sacred about the names. 'single'
> would do.
>
> failover -- that snuck in at some point and I think YH Lu understands why.
>
> This is a bit messy as the guys who first put the fallback/normal in
> never intended for bioses to be built WITHOUT fallback/normal. I
> relied heavily on the fallback stuff at LANL but have since hardly
> used it. Not all people think it is needed -- OLPC for example has
> explicitly rejected a need for a fallback bios image. It's an open
> question as to whether fallback bios images have real value for
> people.
>   

Seems like this might be a good time to raise this question: what 
exactly is the difference between normal and fallback? I know part of 
the answer, but I don't think I know all of it.

-Corey

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[ron minnich]

On Jan 17, 2008 2:32 PM, Ronald Hoogenboom <[EMAIL PROTECTED]> wrote:
> Now I'm looking at how to make the rom_stream read the flash chip like
> in the over512k_read_chip.
> But I'm a bit stuck on the overal mechanics of the initialization
> process and how to get hold of the assigned io port for the SPI serial
> flash controller in the IT8716.
> This is supposed to be configured by the GPIO Config registers (LDN 07)
> at index 0x64 and 0x65, but in the pnp_dev_info array in
> it8716f/superio.c, there is nothing listed after IT8716F_GPIO, while at
> other indexes, there is. Does this mean that the superio.c file fails to
> configure this io port?

yes, it means that the port is not configured. There is a lot of TODO
in this file.
It may be time to fill it in.

> Suppose that this port IS configured correcly, what means should my
> rom_stream use to retrieve this port address?

I think you make a new stream type, let's called it bad_stream, that knows
how to handle the addresses. The bad_stream would know that when the
address is in a certain range, i.e. below the address (0-512KB), it
will use I/O, and when it is above, it can use memory access.

So in src/stream/Config.lb
you add something like this:

uses CONFIG_BADROM_PAYLOAD

if CONFIG_BADROM_PAYLOAD
  object badrom_stream.o
end

>
> Is there some doc about these pnp functions?

no, sorry. We are doing better in v3 ... we know the lack of docs is a problem.

>
> Suppose I've figured it all out and created my new rom_stream_over512k
> (or so) object, how do I mingle it into the Config.lb files, so it will
> automatically get chosen when a ROM_SIZE of over 512k is selected in the
> target configuration (AND the superio is an IT8716F).

I think you make those boards always use bad_stream (or whatever) in all cases,
and not try to get the config tool to pick different types, as in the
example above.

But, it might be possible to do this:

if CONFIG_BADROM_PAYLOAD
  if ROM_SIZE > 512*1024
   object badrom_stream.o
  else
  object rom_stream.o
  end
end

Something like this might work.

The it8761f is NOT the worst thing we have seen, it's just really, really bad.

So I guess we have to put in another kludge for badly design chipsets.
It's not the first time :-)

>
> In the targets/gigabyte/m57sli directory, there is a Config.lb.kernel
> file, which I used as a base for my build, but it only uses a 'fallback'
> and a 'failover' image. Why no 'normal' one? What is supposed to be the
> purpose of each image? (You see, I'm really missing some key parts of
> the big picture...)

you're not missing anything. The picture, which was once, years ago,
pretty clean, has gotten grafitti-tagged by many authors and the
necessities of strange hardware.

This will make no sense, but I'll try. Fallback is the image you can
ALWAYS use, even when normal does not work. Normal is an image you can
use unless something breaks, and you use fallback.

So, what's the one image you must always have? fallback. What's the
optional image? normal. So, when there is only going to be one image,
it's called fallback.

What people don't realize is that the images can have any name, so,
for instance, on single image ROMs, they should really not use the
name 'fallback' -- there is nothing sacred about the names. 'single'
would do.

failover -- that snuck in at some point and I think YH Lu understands why.

This is a bit messy as the guys who first put the fallback/normal in
never intended for bioses to be built WITHOUT fallback/normal. I
relied heavily on the fallback stuff at LANL but have since hardly
used it. Not all people think it is needed -- OLPC for example has
explicitly rejected a need for a fallback bios image. It's an open
question as to whether fallback bios images have real value for
people.

Let me know how your new stream code goes as I am going to need it, I think.

ron

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[ron minnich]

OK, SiS sent me this nice board with some 2 MB flash parts and I have
not gotten much past step 1, "Program the flash", as it is a 2 MB
flash. I had no idea what a mess the superio/flash interface was going
to be.

So, Ronald, I am most interested in your patch, hope it is ready soon!

And, well, seems we have a problem with 2 MB parts. Ouch. I guess what
I will do for now is use 512KB of the 2 MB part (i.e. program it with
4 512KB images).

I hate to say this, and I agree with the desire to remain clean in v3,
but we may have to find a way to deal with this mess. I hope not.  It
is hard to believe how brain-dead PC hardware can be, given that the
same mistakes and lessons are learned (and forgotten) over and over
again. Too bad.

Anyway I am ready to test the patch tomorrow morning, I have a 1/2
vacation day and intend to put it to good use ...

ron

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 18.01.2008 03:00, Carl-Daniel Hailfinger wrote:
> Nice. I have restructured your patch a bit, but I'd like to improve even
> further. Could you take a look at all those places where you added
> "const unsigned char cmd[] = XXX" and use #defines for commands like
> those at the top of spi.c? That would improve readability of the code.
>   

I was unclear. I mean stuff like this:

void spi_write_status_register(int status)
{
const unsigned char cmd[] = {0x01,(unsigned char)status};
/* Send WRSR (Write Status Register) */
generic_spi_command(2, 0, cmd, NULL);
}

would probably be more readable if it looked like this:

void spi_write_status_register(int status)
{
const unsigned char cmd[] = {JEDEC_WRSR,(unsigned char)status};
/* Send WRSR (Write Status Register) */
generic_spi_command(JEDEC_WRSR_OUTSIZE, JEDEC_WRSR_INSIZE, cmd,
NULL);
}

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 17.01.2008 22:25, Ronald Hoogenboom wrote:
> On Thu, 2008-01-17 at 00:56 +0100, Carl-Daniel Hailfinger wrote:
>
>   
>> Please see
>> http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure for
>> details on patch submission. There are two things that block acceptance
>> of your patch:
>> - A "Signed-off-by:" line is missing. Please add one.
>> 
>
> I'll try again now... Sorry for my ignorance (or failure to read the
> procedure).
>   

No problem, thanks for fixing this up.

>> The function is probably more specific to IT8716F and chips bigger than
>> 512 kByte. Maybe rename the function and have the IT8716F writing code
>> switch to you method if the chip is bigger than 512 kB?
>> 
>
> I've made it a little more generic, but otoh, the spi.c is sort of less
> generic than the filename would suggest, quite IT8716F specific.
>   

Yes, but I always try to keep infrastructure generic enough to be able
to support other SPI hosts later.

> I've made all spi flash chips over 512k point to the new functions and
> hope that they will all work for them too (I cannot test this). With the
> generic functions, it will definately not work because of the 8716's
> 512k limit, so it won't break anything that is not already broken...
> But the lack of block-write seems to me is quite sst chip specific
> (remember they have an AAI mode instead, which doesn't play well with
> the limited io transfer length options of the IT8716). It might be more
> appropriate to have an 'over512k_page_write' for more speed with chips
> that do support that. Also the byte program wait time is hard wired to
> 10 us (following the 25vf016b datasheet). And the block protect
> enablement might also NOT be so generic.
>   

By the way, the lack of block-write in the data sheet does not mean it
will not work with the real chip.


>> Please try to address the comments above and repost.
>> Note: I suggest that the "maximum frequency" stuff could be handled in
>> another patch because it requires redesign of some structures (needs
>> discussion) and is also is logically another change.
>>
>> 
>
> For now, the frequency is set to 33/2 for the new large flash functions,
> pending a better approach.
>
> The reworked attached patch also includes the added winbond (previously
> known as Nexcom) chips.
>
> Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>
>   

Nice. I have restructured your patch a bit, but I'd like to improve even
further. Could you take a look at all those places where you added
"const unsigned char cmd[] = XXX" and use #defines for commands like
those at the top of spi.c? That would improve readability of the code.
Please verify the code still works for you.

Explicit mentioning of flash functions for chips >512 kByte has been
converted to implicit decisions. Some cosmetic changes were done as well.

Signed-off-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]>


Index: flashrom-big/flash.h
===
--- flashrom-big/flash.h(Revision 3051)
+++ flashrom-big/flash.h(Arbeitskopie)
@@ -234,7 +234,16 @@
 
 #define TI_ID  0x97/* Texas Instruments */
 
+/*
+ * W25X chips are SPI, first byte of device ID is memory type, second
+ * byte of device ID is related to log(bitsize).
+ */
 #define WINBOND_ID 0xDA/* Winbond */
+#define WINBOND_NEX_ID 0xEF/* Winbond (ex Nexcom) serial flash 
devices */
+#define W_25X100x3011
+#define W_25X200x3012
+#define W_25X400x3013
+#define W_25X800x3014
 #define W_29C011   0xC1
 #define W_29C020C  0x45
 #define W_29C040P  0x46
@@ -297,6 +306,7 @@
 void generic_spi_write_disable();
 int generic_spi_chip_erase_c7(struct flashchip *flash);
 int generic_spi_chip_write(struct flashchip *flash, uint8_t *buf);
+int generic_spi_chip_read(struct flashchip *flash, uint8_t *buf);
 
 /* 82802ab.c */
 int probe_82802ab(struct flashchip *flash);
Index: flashrom-big/flashchips.c
===
--- flashrom-big/flashchips.c   (Revision 3051)
+++ flashrom-big/flashchips.c   (Arbeitskopie)
@@ -51,13 +51,13 @@
{"MX29F002",MX_ID,  MX_29F002,  256, 64 * 1024,
 probe_29f002,  erase_29f002,   write_29f002},
{"MX25L4005",   MX_ID,  MX_25L4005, 512, 256,
-probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write},
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
{"MX25L8005",   MX_ID,  MX_25L8005, 1024, 256,
-probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write},
+probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write, 
generic_spi_chip_read},
{"SST25VF040B", SST_ID, SST_25VF040B,   512,256,
-   p

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

[Ronald Hoogenboom]

Now I'm looking at how to make the rom_stream read the flash chip like
in the over512k_read_chip.
But I'm a bit stuck on the overal mechanics of the initialization
process and how to get hold of the assigned io port for the SPI serial
flash controller in the IT8716.
This is supposed to be configured by the GPIO Config registers (LDN 07)
at index 0x64 and 0x65, but in the pnp_dev_info array in
it8716f/superio.c, there is nothing listed after IT8716F_GPIO, while at
other indexes, there is. Does this mean that the superio.c file fails to
configure this io port?

Suppose that this port IS configured correcly, what means should my
rom_stream use to retrieve this port address?

Is there some doc about these pnp functions?

Suppose I've figured it all out and created my new rom_stream_over512k
(or so) object, how do I mingle it into the Config.lb files, so it will
automatically get chosen when a ROM_SIZE of over 512k is selected in the
target configuration (AND the superio is an IT8716F).

In the targets/gigabyte/m57sli directory, there is a Config.lb.kernel
file, which I used as a base for my build, but it only uses a 'fallback'
and a 'failover' image. Why no 'normal' one? What is supposed to be the
purpose of each image? (You see, I'm really missing some key parts of
the big picture...)

Ronald.


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ronald Hoogenboom]

On Thu, 2008-01-17 at 00:56 +0100, Carl-Daniel Hailfinger wrote:

> Please see
> http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure for
> details on patch submission. There are two things that block acceptance
> of your patch:
> - A "Signed-off-by:" line is missing. Please add one.

I'll try again now... Sorry for my ignorance (or failure to read the
procedure).

> The function is probably more specific to IT8716F and chips bigger than
> 512 kByte. Maybe rename the function and have the IT8716F writing code
> switch to you method if the chip is bigger than 512 kB?
> 

I've made it a little more generic, but otoh, the spi.c is sort of less
generic than the filename would suggest, quite IT8716F specific.
I've made all spi flash chips over 512k point to the new functions and
hope that they will all work for them too (I cannot test this). With the
generic functions, it will definately not work because of the 8716's
512k limit, so it won't break anything that is not already broken...
But the lack of block-write seems to me is quite sst chip specific
(remember they have an AAI mode instead, which doesn't play well with
the limited io transfer length options of the IT8716). It might be more
appropriate to have an 'over512k_page_write' for more speed with chips
that do support that. Also the byte program wait time is hard wired to
10 us (following the 25vf016b datasheet). And the block protect
enablement might also NOT be so generic.

> 
> Please try to address the comments above and repost.
> Note: I suggest that the "maximum frequency" stuff could be handled in
> another patch because it requires redesign of some structures (needs
> discussion) and is also is logically another change.
> 

For now, the frequency is set to 33/2 for the new large flash functions,
pending a better approach.

The reworked attached patch also includes the added winbond (previously
known as Nexcom) chips.

Signed-off-by: Ronald Hoogenboom <[EMAIL PROTECTED]>

---
Index: flash.h
===
--- flash.h	(revision 3051)
+++ flash.h	(working copy)
@@ -247,6 +247,12 @@
 #define W_49V002A		0xB0
 #define W_49V002FA		0x32
 
+#define WINBOND_SPI_ID 0xEF/* Winbond serial flash devices */
+#define W_25X100x3011
+#define W_25X200x3012
+#define W_25X400x3013
+#define W_25X800x3014
+
 /* udelay.c */
 void myusec_delay(int time);
 void myusec_calibrate_delay();
@@ -297,6 +303,8 @@
 void generic_spi_write_disable();
 int generic_spi_chip_erase_c7(struct flashchip *flash);
 int generic_spi_chip_write(struct flashchip *flash, uint8_t *buf);
+int over512k_spi_chip_write(struct flashchip *flash, uint8_t *buf);
+int over512k_spi_chip_read(struct flashchip *flash, uint8_t *buf);
 
 /* 82802ab.c */
 int probe_82802ab(struct flashchip *flash);
Index: flashchips.c
===
--- flashchips.c	(revision 3051)
+++ flashchips.c	(working copy)
@@ -53,11 +53,11 @@
 	{"MX25L4005",	MX_ID,		MX_25L4005,	512, 256,
 	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
 	{"MX25L8005",	MX_ID,		MX_25L8005,	1024, 256,
-	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	over512k_spi_chip_write, over512k_spi_chip_read},
 	{"SST25VF040B",	SST_ID,		SST_25VF040B,	512,	256,
 	probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
 	{"SST25VF016B",	SST_ID,		SST_25VF016B,	2048,	256,
-	probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
+	 probe_spi,	generic_spi_chip_erase_c7,	over512k_spi_chip_write, over512k_spi_chip_read},
 	{"SST29EE020A", SST_ID,		SST_29EE020A,	256, 128,
 	 probe_jedec,	erase_chip_jedec, write_jedec},
 	{"SST28SF040A", SST_ID,		SST_28SF040,	512, 256,
@@ -122,6 +122,14 @@
 	 probe_jedec,   erase_chip_jedec, write_39sf020},
 	{"W39V080A", 	WINBOND_ID, 	W_39V080A,	1024, 64*1024,
 	 probe_jedec,	erase_chip_jedec, write_39sf020},
+	{"W25x10",  WINBOND_SPI_ID, W_25X10,128, 256, 
+	 probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write},
+	{"W25x20",  WINBOND_SPI_ID, W_25X20,256, 256, 
+	 probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write},
+	{"W25x40",  WINBOND_SPI_ID, W_25X40,512, 256, 
+	 probe_spi, generic_spi_chip_erase_c7,  generic_spi_chip_write},
+	{"W25x80",  WINBOND_SPI_ID, W_25X80,1024, 256, 
+	 probe_spi, generic_spi_chip_erase_c7,  over512k_spi_chip_write, over512k_spi_chip_read},
 	{"M29F002B",	ST_ID, 		ST_M29F002B,	256, 64 * 1024,
 	 probe_jedec,	erase_chip_jedec, write_jedec},
 	{"M50FW040",	ST_ID, 		ST_M50FW040,	512, 64 * 1024,
@@ -159,15 +167,15 @@
 	{"M25P40",	ST_ID,		ST_M25P40,	512,	256,
 	 probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write},
 	{"M25P80",	ST_ID,		ST_M25P80,	1024,	256,
-	 probe_spi,	generic_spi_chip_erase_c7,	ge

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

[Ronald Hoogenboom]

On Thu, 2008-01-17 at 01:24 +0100, Carl-Daniel Hailfinger wrote:
> On 17.01.2008 00:13, Ronald Hoogenboom wrote:
> > I mounted a SST25VF016B 2MByte flash chip on the second SPI bios
> ...
> > 33MHz) is negligible.
> >   
> 
> Please be aware that the M57SLI may read the reset vector and other
> really early stuff at 33 MHz, thereby causing read errors (sometimes
> single bit shifts) which are really hard to find.
> 
As far as I've seen the default io speed setting is 33/2, see the table
in sect. 9.5.3.1 on page 78. So I hope (if the datasheet is correct)
everything is done at low speed initially, which is GOOD ;-).
> 
> Regards,
> Carl-Daniel

What I've seen is that the memory mapped area from fff8 to 
is the top quarter of the 2MB flash rom, so it should be the intended
contents at the correct location, as the linuxbios code is located there
at the top and the elf payload at the bottom.
Probably the IT8716 doesn't do any address translation at all and the
flash chip just ignores the upper address bits (just like when
block-reading past the top of the chips address space).

brgds,
Ronald.


-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ward Vandewege]

On Thu, Jan 17, 2008 at 03:00:35PM +0100, Peter Stuge wrote:
> I've found what seems to be a good 16Mbit candidate that is readily
> available to me at a fair price without(!) moq. :)
> 
> Spansion S25FL016A0LMFI011
> http://www.spansion.com/products/S25FL016A.html
> http://www.spansion.com/datasheets/s25fl016a_00_c3.pdf

Nice! That one is available for $1.88 (1+) - $1.61 (100+) at jameco.com.

I'll get a couple I think. 16Mbit is plenty big for an LAB payload with a
useful busybox environment.

Thanks for digging this one up.

Thanks,
Ward.

-- 
Ward Vandewege <[EMAIL PROTECTED]>
Free Software Foundation - Senior System Administrator

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Peter Stuge]

On Thu, Jan 17, 2008 at 03:00:35PM +0100, Peter Stuge wrote:
> I've tried to get quotes also for 16 and 32Mbit Macronix but they
> weren't available last time I asked.

16Mbit is now, but 32 and 64 are still not in production, only
sampling.


//Peter

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 17.01.2008 15:00, Peter Stuge wrote:
> I've found what seems to be a good 16Mbit candidate that is readily
> available to me at a fair price without(!) moq. :)
>
> Spansion S25FL016A0LMFI011
> http://www.spansion.com/products/S25FL016A.html
> http://www.spansion.com/datasheets/s25fl016a_00_c3.pdf
>   

Yes, that one should work fine.

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 17.01.2008 14:32, Harald Gutmann wrote:
> Do you think it would be possible to add write support for the MX25L8005 and 
> the MX25L3205D to flashrom based on this patch?
>   

Yes. The code is generic enough. But the order in which you write
sectors to the chip may differ between both flashing methods.


Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Peter Stuge]

On Wed, Jan 16, 2008 at 10:34:29PM -0500, Ward Vandewege wrote:
> > > Any other suggestions for large SPI/SOIC-8 chips?
> > 
> > Macronix MX25L8005M2C-15G
> 
> Mouser/digikeys/jameco don't have this one.

I've tried to get quotes also for 16 and 32Mbit Macronix but they
weren't available last time I asked.


> Weird, yeah, minimum purchase is around $500 for those two - 270
> units of the 16Mbit, or 180 units of the 32Mbit chip (this is
> digikeys, the others don't carry them).
> 
> Maybe a group buy is in order... 

32Mbit is indeed nice.


I've found what seems to be a good 16Mbit candidate that is readily
available to me at a fair price without(!) moq. :)

Spansion S25FL016A0LMFI011
http://www.spansion.com/products/S25FL016A.html
http://www.spansion.com/datasheets/s25fl016a_00_c3.pdf


//Peter

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Harald Gutmann]

Do you think it would be possible to add write support for the MX25L8005 and 
the MX25L3205D to flashrom based on this patch?

I'd be really glad to see write support for those chips.

regards,
Harald



-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Harald Gutmann]

Am Donnerstag, 17. Januar 2008 03:09:43 schrieb Ward Vandewege:
> On Thu, Jan 17, 2008 at 03:03:48AM +0100, Peter Stuge wrote:
> > On Wed, Jan 16, 2008 at 08:57:52PM -0500, Ward Vandewege wrote:
> > > > English translation:
> > > >
> > > > SST25VF016B is not really compatible with the IT8716F superio.
> > >
> > > All right. So how about the Atmel AT45DB321D-SU
> >
> > Unfortunately it has a different pinout than the flash chips we've
> > looked at so far.
>
> Oh, you're right, odd. It would work in a socket I guess, if that socket is
> wired up correctly. Hrm.
>
> Any other suggestions for large SPI/SOIC-8 chips?

I've two Macronix here one is 8mbit with part number: MX25L8005 and the second 
one is an MX25L3205D.

>
> Thanks,
> Ward.
regards,
Harald

> --
> Ward Vandewege <[EMAIL PROTECTED]>
> Free Software Foundation - Senior System Administrator



-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ward Vandewege]

On Thu, Jan 17, 2008 at 03:24:23AM +0100, Peter Stuge wrote:
> On Wed, Jan 16, 2008 at 09:09:43PM -0500, Ward Vandewege wrote:
> > > > All right. So how about the Atmel AT45DB321D-SU
> > > 
> > > Unfortunately it has a different pinout than the flash chips
> > > we've looked at so far.
> > 
> > Oh, you're right, odd. It would work in a socket I guess, if that
> > socket is wired up correctly. Hrm.
> 
> If the SPI commands for reading are the same then yeah, it should
> work.

Hmm, yeah, that's worth investigating I guess.

> > Any other suggestions for large SPI/SOIC-8 chips?
> 
> Macronix MX25L8005M2C-15G

Mouser/digikeys/jameco don't have this one.

> Winbond W25X16VSSI
> Winbond W25X32VSSI
> 
> Problem with Winbond is they really only want to sell large
> quantities. I haven't even gotten a quote and delivery time
> back from my sales rep on a request I made in November.

Weird, yeah, minimum purchase is around $500 for those two - 270 units of the
16Mbit, or 180 units of the 32Mbit chip (this is digikeys, the others don't
carry them).

Maybe a group buy is in order... 

Thanks,
Ward.

-- 
Ward Vandewege <[EMAIL PROTECTED]>
Free Software Foundation - Senior System Administrator

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Peter Stuge]

On Wed, Jan 16, 2008 at 09:09:43PM -0500, Ward Vandewege wrote:
> > > All right. So how about the Atmel AT45DB321D-SU
> > 
> > Unfortunately it has a different pinout than the flash chips
> > we've looked at so far.
> 
> Oh, you're right, odd. It would work in a socket I guess, if that
> socket is wired up correctly. Hrm.

If the SPI commands for reading are the same then yeah, it should
work.


> Any other suggestions for large SPI/SOIC-8 chips?

Macronix MX25L8005M2C-15G
Winbond W25X16VSSI
Winbond W25X32VSSI


Winbond will also make a 64Mbit flash but it didn't have a product
number a few months ago.

Problem with Winbond is they really only want to sell large
quantities. I haven't even gotten a quote and delivery time
back from my sales rep on a request I made in November.


//Peter

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 17.01.2008 02:48, Ward Vandewege wrote:
> Hi Ronald,
>
> On Thu, Jan 17, 2008 at 12:13:48AM +0100, Ronald Hoogenboom wrote:
>   
>> Problem1 (for reading) is solved by NOT using the mmap method for
>> reading the flash contents, but using outb() for sending the flash read
>> commands (using a specific 25vf016 read function). Also the normal read
>> command is only supported up to 25MHz by this chip, so I cannot use the
>> 33MHz speed as used normally by spi.c. There is also a 'high speed' read
>> command (0x0b), which inserts an extra dummy byte between address and
>> data, but as the 8716 only allows max. 3 bytes read per io transfer, the
>> gain (3 bytes per io transfer @ 16MHz versus 2 bytes per io transfer @
>> 33MHz) is negligible.
>> 
>
> Can you elaborate a bit about that - I see the two read speeds in the
> datasheet, but how do you get to 16MHz in that last sentence?
>   

33 MHz /2 (at least that's how the IT8716F datasheet calls it.


Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ward Vandewege]

On Thu, Jan 17, 2008 at 03:03:48AM +0100, Peter Stuge wrote:
> On Wed, Jan 16, 2008 at 08:57:52PM -0500, Ward Vandewege wrote:
> > > English translation:
> > > 
> > > SST25VF016B is not really compatible with the IT8716F superio.
> > 
> > All right. So how about the Atmel AT45DB321D-SU
> 
> Unfortunately it has a different pinout than the flash chips we've
> looked at so far.

Oh, you're right, odd. It would work in a socket I guess, if that socket is
wired up correctly. Hrm.

Any other suggestions for large SPI/SOIC-8 chips?

Thanks,
Ward.

-- 
Ward Vandewege <[EMAIL PROTECTED]>
Free Software Foundation - Senior System Administrator

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Peter Stuge]

On Wed, Jan 16, 2008 at 08:57:52PM -0500, Ward Vandewege wrote:
> > English translation:
> > 
> > SST25VF016B is not really compatible with the IT8716F superio.
> 
> All right. So how about the Atmel AT45DB321D-SU

Unfortunately it has a different pinout than the flash chips we've
looked at so far.


//Peter

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ward Vandewege]

On Thu, Jan 17, 2008 at 02:29:16AM +0100, Peter Stuge wrote:
> On Thu, Jan 17, 2008 at 01:24:38AM +0100, Carl-Daniel Hailfinger wrote:
> > Please be aware that the M57SLI may read the reset vector and other
> > really early stuff at 33 MHz, thereby causing read errors
> > (sometimes single bit shifts) which are really hard to find.
> 
> English translation:
> 
> SST25VF016B is not really compatible with the IT8716F superio.

All right. So how about the Atmel AT45DB321D-SU, which is a 32Mbit (!) chip
that seems to be able to do 33MHz reads in slow mode (and 66MHz in fast
mode). Only about $3.66 at Mouser.com, and it's in stock.

Here's the datasheet:

   http://www.atmel.com/dyn/resources/prod_documents/doc3597.pdf

Good? Bad?

Thanks,
Ward.

-- 
Ward Vandewege <[EMAIL PROTECTED]>
Free Software Foundation - Senior System Administrator

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ward Vandewege]

Hi Ronald,

On Thu, Jan 17, 2008 at 12:13:48AM +0100, Ronald Hoogenboom wrote:
> Problem1 (for reading) is solved by NOT using the mmap method for
> reading the flash contents, but using outb() for sending the flash read
> commands (using a specific 25vf016 read function). Also the normal read
> command is only supported up to 25MHz by this chip, so I cannot use the
> 33MHz speed as used normally by spi.c. There is also a 'high speed' read
> command (0x0b), which inserts an extra dummy byte between address and
> data, but as the 8716 only allows max. 3 bytes read per io transfer, the
> gain (3 bytes per io transfer @ 16MHz versus 2 bytes per io transfer @
> 33MHz) is negligible.

Can you elaborate a bit about that - I see the two read speeds in the
datasheet, but how do you get to 16MHz in that last sentence?

Thanks,
Ward.

-- 
Ward Vandewege <[EMAIL PROTECTED]>
Free Software Foundation - Senior System Administrator

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 17.01.2008 02:29, Peter Stuge wrote:
> On Thu, Jan 17, 2008 at 01:24:38AM +0100, Carl-Daniel Hailfinger wrote:
>   
>> Please be aware that the M57SLI may read the reset vector and other
>> really early stuff at 33 MHz, thereby causing read errors
>> (sometimes single bit shifts) which are really hard to find.
>> 
>
> English translation:
>   

;-)

> SST25VF016B is not really compatible with the IT8716F superio.
>
> About right?
>   

Mostly. If the M57SLI uses the IT8716F in 33 MHz mode at startup, the
SST25VF016B is indeed incompatible. If IT8716F startup happens at 16
MHz, it will work.
I have no idea about the real behaviour of the hardware.

Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Peter Stuge]

On Thu, Jan 17, 2008 at 01:24:38AM +0100, Carl-Daniel Hailfinger wrote:
> Please be aware that the M57SLI may read the reset vector and other
> really early stuff at 33 MHz, thereby causing read errors
> (sometimes single bit shifts) which are really hard to find.

English translation:

SST25VF016B is not really compatible with the IT8716F superio.

About right?


//Peter

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 17.01.2008 00:13, Ronald Hoogenboom wrote:
> I mounted a SST25VF016B 2MByte flash chip on the second SPI bios
> landpattern on the m57sli mobo (as per the m57sli tutorial).
> There as some problems with that[...]
>
> Problem1 (for reading) is solved by NOT using the mmap method for
> reading the flash contents, but using outb() for sending the flash read
> commands (using a specific 25vf016 read function). Also the normal read
> command is only supported up to 25MHz by this chip, so I cannot use the
> 33MHz speed as used normally by spi.c. There is also a 'high speed' read
> command (0x0b), which inserts an extra dummy byte between address and
> data, but as the 8716 only allows max. 3 bytes read per io transfer, the
> gain (3 bytes per io transfer @ 16MHz versus 2 bytes per io transfer @
> 33MHz) is negligible.
>   

Please be aware that the M57SLI may read the reset vector and other
really early stuff at 33 MHz, thereby causing read errors (sometimes
single bit shifts) which are really hard to find.


Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

On 17.01.2008 01:05, Ward Vandewege wrote:
> Hi Ronald and Carl-Daniel,
>
> Great work - I want to use larger SPI chips on this board myself.
>
> On Thu, Jan 17, 2008 at 12:56:08AM +0100, Carl-Daniel Hailfinger wrote:
>   
>> I see you invested a lot of time to work around the deficiencies of the
>> IT8716F SPI translation function. You even did it the way I described
>> the process a few weeks ago (but I had no time to implement it).
>>
>> 
>>> Problem1 and Problem2 have impact on the useability of flashrom, which I
>>> have sort-of solved (patch below). Problem1 also has impact on the
>>> elfboot.c and rom-stream.c in coreboot (linuxbios), which I'm still
>>> investigating (suggestions welcome!).
>>>   
>>>   
>> I'd like to not handle this problem at all in coreboot v3. 
>> 
>
> What do you propose for v3?
>   

Keep v3 clean, simply refuse to support such horrible interfaces in v3.
Especially the lar walk code would explode to about twice the size if we
wanted to support flash parts that can not be memmapped completely.
There are workarounds for some of that stuff, but I'll keep them secret
until someone manages to actually port a board with IT8176F SPI
translation to v3. I hope that will never happen.


Regards,
Carl-Daniel

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Ward Vandewege]

Hi Ronald and Carl-Daniel,

Great work - I want to use larger SPI chips on this board myself.

On Thu, Jan 17, 2008 at 12:56:08AM +0100, Carl-Daniel Hailfinger wrote:
> I see you invested a lot of time to work around the deficiencies of the
> IT8716F SPI translation function. You even did it the way I described
> the process a few weeks ago (but I had no time to implement it).
> 
> > Problem1 and Problem2 have impact on the useability of flashrom, which I
> > have sort-of solved (patch below). Problem1 also has impact on the
> > elfboot.c and rom-stream.c in coreboot (linuxbios), which I'm still
> > investigating (suggestions welcome!).
> >   
> 
> I'd like to not handle this problem at all in coreboot v3. 

What do you propose for v3?

Thanks,
Ward.

-- 
Ward Vandewege <[EMAIL PROTECTED]>
Free Software Foundation - Senior System Administrator

-- 
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

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

[Carl-Daniel Hailfinger]

Hi Ronald,

first of all, welcome to coreboot.

I see you invested a lot of time to work around the deficiencies of the
IT8716F SPI translation function. You even did it the way I described
the process a few weeks ago (but I had no time to implement it).

Please see
http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure for
details on patch submission. There are two things that block acceptance
of your patch:
- A "Signed-off-by:" line is missing. Please add one.
- The patch does not apply because Evolution mangled it on your side.
Either fix the configuration or attach the patch as plain text.

On 17.01.2008 00:13, Ronald Hoogenboom wrote:
> I mounted a SST25VF016B 2MByte flash chip on the second SPI bios
> landpattern on the m57sli mobo (as per the m57sli tutorial).
> There as some problems with that, on the one side from the point of the
> LPC memory mapping options of the IT8716F (max. 512KB contiguous) and on
> the other side with the programming method(s) supported by the SST flash
> chip (only the byte-mode programming is compatible with the IT8716).
> Lets call them problem1 and problem2.
>
> The reason why I want the 2MB is that I would like to try direct Linux
> kernel as the payload.
>   

Agreed.

> Problem1 and Problem2 have impact on the useability of flashrom, which I
> have sort-of solved (patch below). Problem1 also has impact on the
> elfboot.c and rom-stream.c in coreboot (linuxbios), which I'm still
> investigating (suggestions welcome!).
>   

I'd like to not handle this problem at all in coreboot v3. In v2, we can
probably work around this issue by ensuring that all code until RAM is
enabled has to be mapped in the contiguous 512 kByte area (which may
correspond to the first or last 512 kB of the flash chip, nobody really
knows and ITE are not answering my mails.

> Problem1 (for reading) is solved by NOT using the mmap method for
> reading the flash contents, but using outb() for sending the flash read
> commands (using a specific 25vf016 read function). Also the normal read
> command is only supported up to 25MHz by this chip, so I cannot use the
> 33MHz speed as used normally by spi.c. There is also a 'high speed' read
> command (0x0b), which inserts an extra dummy byte between address and
> data, but as the 8716 only allows max. 3 bytes read per io transfer, the
> gain (3 bytes per io transfer @ 16MHz versus 2 bytes per io transfer @
> 33MHz) is negligible.
>   

Yes. We have to specify a maximum speed for each SPI chip in
flashchips.c. Suggestions welcome.

> Problem2 (for writing) is solved similarly: The mmap write method cannot
> be used effectively, because the flash chip needs a Write-enable command
> for each Byte-program (no block program concept in byte-program mode).
> The chip has a so-called AAI (auto address increment) programming mode
> instead, but that cannot be used with the 8716 chip, because it needs 6
> byte and 3 byte io transfers, which it doesn't support (bah!). Not using
> the mmap writes also solves the limited LPC memory address window. So,
> there is also a 25vf016 specific write function.
>   

The function is probably more specific to IT8716F and chips bigger than
512 kByte. Maybe rename the function and have the IT8716F writing code
switch to you method if the chip is bigger than 512 kB?


> NOTE: there may be some offset because of other changes...
>
> Index: flash.h
> ===
> --- flash.h   (revision 3051)
> +++ flash.h   (working copy)
> @@ -297,6 +303,8 @@
>  void generic_spi_write_disable();
>  int generic_spi_chip_erase_c7(struct flashchip *flash);
>  int generic_spi_chip_write(struct flashchip *flash, uint8_t *buf);
> +int sst25vf016_spi_chip_write(struct flashchip *flash, uint8_t *buf);
> +int sst25vf016_spi_chip_read(struct flashchip *flash, uint8_t *buf);
>  
>  /* 82802ab.c */
>  int probe_82802ab(struct flashchip *flash);
> Index: flashchips.c
> ===
> --- flashchips.c  (revision 3051)
> +++ flashchips.c  (working copy)
> @@ -57,7 +57,7 @@
>   {"SST25VF040B", SST_ID, SST_25VF040B,   512,256,
>   probe_spi,  generic_spi_chip_erase_c7,  generic_spi_chip_write},
>   {"SST25VF016B", SST_ID, SST_25VF016B,   2048,   256,
> - probe_spi,  generic_spi_chip_erase_c7,  generic_spi_chip_write},
> +  probe_spi, generic_spi_chip_erase_c7,  
> sst25vf016_spi_chip_write,
> sst25vf016_spi_chip_read},
>   {"SST29EE020A", SST_ID, SST_29EE020A,   256, 128,
>probe_jedec,   erase_chip_jedec, write_jedec},
>   {"SST28SF040A", SST_ID, SST_28SF040,512, 256,
> Index: spi.c
> ===
> --- spi.c (revision 3051)
> +++ spi.c (working copy)
> @@ -77,6 +77,8 @@
>  #define JEDEC_RDSR_BIT_WIP   (0x01 << 0)
>  
>  uint16_t it8716f_flashport = 0;
> +/* use fast 33MHz SP