Am 30.06.2011 20:58 schrieb Stefan Tauner:
> 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?
>
spi_chip_write_256, and page_size=64. This reminds me... page_size must die!
>>> + .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.
>
Right, that is something I wanted to comment on once hwseq works. I'll
send my response in that thread.
>>>
>>> #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.
>
probe_flash will see another refactoring soon once the programmer
registration stuff is completely done and merged. By then, probing will
be per-busmaster, not per-chip. Don't invest too much (or any) time in a
probe_flash refactoring, it might get rewritten again.
>>> +
>>> + 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?
>
I think it87 can't deal with this. Some SPI masters (At least SB600,
ICH/VIA, Dediprog, maybe even others) have explicit ways to handle one
dummy byte between write and read, but we currently ignore that feature
everywhere.
>>> +/* 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).
>
The 25 comes from the *25* in most SPI flash chip model names.
spi_sfdp.c might be an option, but it does not fit with the names of the
other chip driver files.
>>> […]
>>>
>> 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.
>
If a patch had a full review, it makes sense to post a separate fixup
(if that fixup needs review), but I prefer a merged patch pretty much
for everything. You could post a fixup as reply to the original patch if
you want quick feedback for the fixup, but if you repost a whole series,
please use merged patches always.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
