On Thu, 30 Jun 2011 20:24:30 +0200 Carl-Daniel Hailfinger <[email protected]> wrote:
> Am 24.06.2011 16:53 schrieb Stefan Tauner: > > Similar to ICH Hardware Sequencing this uses a generic struct flashchip > > element in flashrom.c with dummy values and a special probe function that > > fills the > > obtained values into that generic struct. > > Documentation used: > > http://www.jedec.org/standards-documents/docs/jesd216 (2011-04) > > W25Q32BV data sheet Revision F (2011-04-01) > > EN25QH16 data sheet Revision F (2011-06-01) > > > > Signed-off-by: Stefan Tauner <[email protected]> > > > > Nice feature. we will have to wait to see if that is true. :) > > --- > > chipdrivers.h | 1 + > > flashchips.c | 24 +++++ > > flashchips.h | 1 + > > flashrom.c | 28 ++++++- > > spi.h | 5 + > > spi25.c | 273 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 6 files changed, 331 insertions(+), 1 deletions(-) > > > > diff --git a/chipdrivers.h b/chipdrivers.h > > index 92ddbea..0a3df50 100644 > > --- a/chipdrivers.h > > +++ b/chipdrivers.h > > @@ -26,6 +26,7 @@ > > #define __CHIPDRIVERS_H__ 1 > > > > /* spi.c, should probably be in spi_chip.c */ > > +int probe_spi_sfdp(struct flashchip *flash); > > int probe_spi_rdid(struct flashchip *flash); > > int probe_spi_rdid4(struct flashchip *flash); > > int probe_spi_rems(struct flashchip *flash); > > diff --git a/flashchips.c b/flashchips.c > > index 865ba2f..6fbbd2c 100644 > > --- a/flashchips.c > > +++ b/flashchips.c > > @@ -8507,6 +8507,30 @@ const struct flashchip flashchips[] = { > > .write = write_jedec_1, > > .read = read_memmapped, > > }, > > + > > + { > > + .vendor = "Unknown", > > + .name = "SFDP device", > > + .bustype = CHIP_BUSTYPE_SPI, > > + .manufacture_id = GENERIC_MANUF_ID, > > + .model_id = SFDP_DEVICE_ID, > > + /* We present our own "report this" text hence we do not > > + * want the default "This flash part has status UNTESTED..." > > + * text to be printed. */ > > + .tested = TEST_OK_PREW, > > + .probe = probe_spi_sfdp, > > + .page_size = 256, > > + .write = spi_chip_write_256, > > > > Does SFDP really specify 256-byte write capability? ah right. i forgot about that. there is a field in the first dword which is the only thing mentioned regarding this subject: "Write Granularity 0: 1 Byte – Use this setting for single byte programmable devices or buffer programmable devices when the buffer is less than 64 bytes (32 Words). 1: Use this setting for buffer programmable devices when the buffer size is 64 bytes (32 Words) or larger. 0 is clear and i should set .write = spi_chip_write_1, but what about the other case? > > + .read = spi_chip_read, > > + /* FIXME: some vendor extensions define this */ > > + .voltage = {}, > > + /* Everything below will be set by the probing function. */ > > + .total_size = 0, > > + .feature_bits = 0, > > + .block_erasers = {}, > > + .unlock = NULL, > > + .printlock = NULL, > > + }, > > > > { > > .vendor = "AMIC", > > diff --git a/flashchips.h b/flashchips.h > > index 3b2b94f..82333c8 100644 > > --- a/flashchips.h > > +++ b/flashchips.h > > @@ -36,6 +36,7 @@ > > > > #define GENERIC_MANUF_ID 0xffff /* Check if there is a vendor ID */ > > #define GENERIC_DEVICE_ID 0xffff /* Only match the vendor ID */ > > +#define SFDP_DEVICE_ID 0xfffe > > > > Just a comment, not something that needs to be changed: We probably need > CFI_DEVICE_ID as well once we support CFI (which is SFDP for non-SPI flash). and there is the id for hwseq, which is currently also 0xfffe. please advice. hint: .manufacture_id and .model_id are uint32_t. > > > > > #define ALLIANCE_ID 0x52 /* Alliance Semiconductor */ > > #define ALLIANCE_AS29F002B 0x34 > > diff --git a/flashrom.c b/flashrom.c > > index aed10aa..56af373 100644 > > --- a/flashrom.c > > +++ b/flashrom.c > > @@ -1206,7 +1206,33 @@ int probe_flash(int startchip, struct flashchip > > *fill_flash, int force) > > * probe_flash() is the first one and thus no chip has been > > * found before. > > */ > > - if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID) > > + if (startchip == 0 && fill_flash->model_id == SFDP_DEVICE_ID) { > > + msg_cinfo("===\n" > > + "SFDP has autodetected a flash chip which is " > > + "not natively supported by flashrom yet.\n"); > > + if (!check_block_erasers(flash, 0)) > > + msg_cinfo("The standard operations read and " > > + "verify should work, but to support " > > + "erase, write and all other " > > + "possible features"); > > + else > > + msg_cinfo("All standard operations (read, " > > + "verify, erase and write) should " > > + "work, but to support all possible " > > + "features"); > > + > > + msg_cinfo(" we need to add them manually.\nYou " > > + "can help us by mailing us the output of " > > + "the following command to flashrom@flashrom." > > + "org: \n'flashrom -VV [plus the " > > + "-p/--programmer parameter (if needed)]" > > + "'\nThanks for your help!\n" > > + "===\n"); > > + } > > > > Should we refactor those SFDP messages into a separate function and do > that for the untested chip messages as well? It would probably improve > code readability here. THAT is not the problem with probe_flash imho. i have refactoring that function (and its caller) on my agenda. that does not contradict refactoring those messages out, but it wont solve the readability issue that exists imho. > > > + > > + if (startchip == 0 || > > + ((fill_flash->model_id != GENERIC_DEVICE_ID) && > > + (fill_flash->model_id != SFDP_DEVICE_ID))) > > break; > > > > notfound: > > diff --git a/spi.h b/spi.h > > index b908603..5f07eae 100644 > > --- a/spi.h > > +++ b/spi.h > > @@ -40,6 +40,11 @@ > > #define JEDEC_REMS_OUTSIZE 0x04 > > #define JEDEC_REMS_INSIZE 0x02 > > > > +/* Read Serial Flash Discoverable Parameters (SFDP) */ > > +#define JEDEC_SFDP 0x5a > > +#define JEDEC_SFDP_OUTSIZE 0x05 /* 8b op, 24b addr, 8b dummy */ > > > > Ouch. A few SPI masters will have pretty explosions with that outsize. > This reminds me... I should resend my patch which can deal with dummy > bytes between write and read in a SPI command. can you point me to one, so that i can look at that code please? > > > +/* JEDEC_SFDP_INSIZE : any length */ > > + > > /* Read Electronic Signature */ > > #define JEDEC_RES 0xab > > #define JEDEC_RES_OUTSIZE 0x04 > > diff --git a/spi25.c b/spi25.c > > index d3680fb..af81f19 100644 > > --- a/spi25.c > > +++ b/spi25.c > > @@ -23,6 +23,7 @@ > > */ > > > > #include <string.h> > > +#include <stdlib.h> > > #include "flash.h" > > #include "flashchips.h" > > #include "chipdrivers.h" > > @@ -113,6 +114,278 @@ int spi_write_disable(void) > > return spi_send_command(sizeof(cmd), 0, cmd, NULL); > > } > > > > > > The code below is huge, and I'd argue it is for a chip family and should > live in sfdp25.c (or something with a similar name). sfdp.c? or spi_sfdp.c? why 25? probably some jedec thing, but i dont know exactly what that indicates (and if it is appropriate here). >> […] > > Could you resend this with the comments addressed and with the squash > patch merged in? I didn't review the SFDP code yet, but the right > presentation makes reviewing easier. sure. do you always prefer the squashes/fixes already merged? i wonder what is better for reviewers in general. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
