[flashrom] [PATCH 3/3] add support for SFDP (JESD216)
Stefan Tauner
stefan.tauner at student.tuwien.ac.at
Thu Jun 30 20:58:34 CEST 2011
On Thu, 30 Jun 2011 20:24:30 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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 <stefan.tauner at student.tuwien.ac.at>
> >
>
> 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 at 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
More information about the flashrom
mailing list