On 28.09.2007 16:55, Uwe Hermann wrote: > On Fri, Sep 28, 2007 at 03:35:54PM +0200, Carl-Daniel Hailfinger wrote: >> On 27.09.2007 16:30, Ward Vandewege wrote: >>> On Thu, Sep 27, 2007 at 11:31:14AM +0200, Carl-Daniel Hailfinger wrote: >>>> No problem. Can you commit with your changes? Changelog follows: >>>> >>>> Add preliminary SPI flash identification support for SPI chips attached >>>> to ITE IT8716F Super I/O. Right now this is hardcoded to the Gigabyte >>>> M57SLI board. It works only with rev 2.0 of the board, but it will bail >>>> out on earlier versions, so no damage can occur. >>>> >>>> Signed-off-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]> >>> Acked-by: Ward Vandewege <[EMAIL PROTECTED]> >>> >>> and committed in r2811 >> Thanks for committing! >> >> Here is a patch against current svn which aims to restructure SPI flash >> support in a more reasonable way. Please note that the patch is not >> ready to be committed, but it should work much better than the existing > > Yep, Signed-off-by missing for this one (yeah, there's one for the older > version, but still)...
Intentional. Was not ready to be applied. >> one, namely without the dirty "do everything in board_enable functions" >> hack. Functions still have to be moved to another file, but for initial >> testing, this should be fine. >> >> Regards, >> Carl-Daniel >> >> Index: util/flashrom/flash.h >> =================================================================== >> --- util/flashrom/flash.h (Revision 2811) >> +++ util/flashrom/flash.h (Arbeitskopie) >> @@ -68,8 +68,31 @@ >> #define AT_29C040A 0xA4 >> #define AT_29C020 0xDA >> >> +#define EON_ID 0x1C >> +/* EN25 chips are SPI, first byte of device id is memory type, >> + second byte of device id is log(bitsize)-9 */ >> +#define EN_25B05 0x2010 /* 2^19 kbit or 2^16 kByte */ >> +#define EN_25B10 0x2011 >> +#define EN_25B20 0x2012 >> +#define EN_25B40 0x2013 >> +#define EN_25B80 0x2014 >> +#define EN_25B16 0x2015 >> +#define EN_25B32 0x2016 >> + >> #define MX_ID 0xC2 /* Macronix (MX) */ >> #define MX_29F002 0xB0 >> +/* MX25L chips are SPI, first byte of device id is memory type, >> + second byte of device id is log(bitsize)-9 */ >> +#define MX_25L512 0x2010 /* 2^19 kbit or 2^16 kByte */ >> +#define MX_25L1005 0x2011 >> +#define MX_25L2005 0x2012 >> +#define MX_25L4005 0x2013 /* MX25L4005{,A} */ >> +#define MX_25L8005 0x2014 >> +#define MX_25L1605 0x2015 /* MX25L1605{,A,D} */ >> +#define MX_25L3205 0x2016 /* MX25L3205{,A} */ >> +#define MX_25L6405 0x2017 /* MX25L3205{,D} */ >> +#define MX_25L1635D 0x2415 >> +#define MX_25L3235D 0x2416 > > Maybe we should separate out the IDs for SPI chips in a different > section of the header file (to not confuse people into assuing they're > LPC parts or so)? All SPI parts have 16-bit device IDs, I'll add a comment about that. >> #define SHARP_ID 0xB0 /* Sharp */ >> #define SHARP_LHF00L04 0xCF >> @@ -182,6 +205,8 @@ >> int linuxbios_init(void); >> extern char *lb_part, *lb_vendor; >> >> +int probe_spi(struct flashchip *flash); > > Can be > int probe_spi(const struct flashchip *flash); > I think. Maybe, but that's a separate cleanup. This function prototype matches the others. >> Index: util/flashrom/board_enable.c >> =================================================================== >> --- util/flashrom/board_enable.c (Revision 2811) >> +++ util/flashrom/board_enable.c (Arbeitskopie) >> @@ -96,35 +96,100 @@ >> return flashport; >> } >> >> -static void it8716_serial_rdid(uint16_t port) >> +/* The IT8716 only supports commands with length 1,2,4,5 bytes including > > IT8716F please, as per datasheet. AFAICS all ITEs should have the F at the > end of the name. Applies to several places in the code and comments. OK. >> + command byte and can not read more than 3 bytes from the device. >> + This function expects writearr[0] to be the first byte sent to the >> device, >> + whereas the IT8716 splits commands internally into address and >> non-address >> + commands with the address in inverse wire order. That's why the register >> + ordering in case 4 and 5 may seem strange. */ >> +static int it8716_spi_command(uint16_t port, unsigned char writecnt, >> unsigned char readcnt, const unsigned char *writearr, unsigned char *readarr) > > If the size of writecnt/readcnt/writearr/readarr etc. is fixed/specified in > the datasheet then please make it uint16_t etc. instead of the "generic" > unsigned char and friends. {read,write}cnt are between 0 and 5, so they could as well be int. No datasheet specification for their variable size. {read,write}arr are just helper constructs to abstract the horrible datasheet variable types. Or do you honestly want a 24-bit type? >> +static int it8716_serial_rdid(uint16_t port, unsigned char *readarr) >> +{ >> + /* RDID is 0x9f */ >> + const unsigned char cmd[] = {0x9f}; > > #define RDID 0x9f > > at the top of the file? Can do, but it will get messy very soon because there is no way to sensibly specify command value, input size, output size in one #define. Furthermore, depending on the state of the chip, the commands change values/input/output size. So I'd prefer to keep it that way right now until we figure out a better abstraction which doesn't complicate stuff. >> + >> + if (it8716_spi_command(port, 1, 3, cmd, readarr)) >> + return 1; >> + printf("RDID returned %02x %02x %02x\n", readarr[0], readarr[1], >> readarr[2]); >> + return 0; >> +} >> + >> +static uint16_t it8716_flashport = 0; > > Move this to the top of the file, please. OK. >> static int it87xx_probe_serial_flash(const char *name) >> { >> - uint16_t flashport; >> - flashport = find_ite_serial_flash_port(0x2e); >> - if (flashport) >> - it8716_serial_rdid(flashport); >> - flashport = find_ite_serial_flash_port(0x4e); >> - if (flashport) >> - it8716_serial_rdid(flashport); >> + it8716_flashport = find_ite_serial_flash_port(0x2e); >> + if (!it8716_flashport) >> + it8716_flashport = find_ite_serial_flash_port(0x4e); > > #defines for 0x2e and 0x4e. OK. >> + return (!it8716_flashport); >> +} >> + >> +int probe_spi(struct flashchip *flash) >> +{ >> + unsigned char readarr[3]; >> + uint8_t manuf_id; >> + uint16_t model_id; >> + if (it8716_flashport) { >> + it8716_serial_rdid(it8716_flashport, readarr); >> + manuf_id = readarr[0]; >> + model_id = (readarr[1] << 8) | readarr[2]; >> + printf_debug("%s: id1 0x%x, id2 0x%x\n", __FUNCTION__, >> manuf_id, model_id); >> + if (manuf_id == flash->manufacture_id && model_id == >> flash->model_id) >> + return 1; >> + } >> + >> return 0; >> } > > > Looks good otherwise, but should be tested of course. Yes, looking for testers. Carl-Daniel -- linuxbios mailing list linuxbios@linuxbios.org http://www.linuxbios.org/mailman/listinfo/linuxbios