Re: [flashrom] [PATCH] add support for Pm25LQ032

[Stefan Tauner]

On Sat, 14 Mar 2015 15:00:10 +0000
Steven Honeyman  wrote:
> I found this in a Chinese router, read tested (twice, successful) but
> not yet write/erase tested.
> Based on information from the datasheet at
> http://www.chingistek.com/img/Product_Files/Pm25LQ032C%20datasheet%20v1.6.1....
>
> Signed-off-by: Steven Honeyman

Hi Steven,

thanks for your patch. There are a number of similar chips (Pm25LQ020, 
Pm25LQ040, Pm25LQ080, Pm25LQ016). Do you want to add support for them too?
> Index: flashchips.c
>
> --- flashchips.c    (revision 1888)
> +++ flashchips.c    (working copy)
> @@ -9708,8 +9708,46 @@
>          .voltage    = {2700, 3600}, /* 2.3-3.6V for Pm25LD040 */
>      },
>
> -{
> {
>     .vendor        = "PMC",
>    .name        = "Pm25LQ032",

Have you found any traces of a Pm25LQ032 without a C suffix? I don't
and would rather add the C suffix here (and in the ID macro) until we
have proof they really exist.
>    .bustype    = BUS_SPI,
>    .manufacture_id    = PMC_ID,
>    .model_id    = PMC_PM25LQ032,
>    .total_size    = 4096,
>    .page_size    = 256,
>    .feature_bits    = FEATURE_WRSR_WREN,

Missing OTP flag and information.
>    .tested        = TEST_OK_PR,
>    .probe        = probe_spi_rdid,
>    .probe_timing    = TIMING_ZERO,
>    .block_erasers    =
>    {
>        {
>            .eraseblocks = { {4 * 1024, 1024} },
>            .block_erase = spi_block_erase_20,    /* sector erase */

All these comments are redundant. Experienced flashrom hacker will
instantly deduce that from the erase block size(s).
>        }, {
>            .eraseblocks = { {4 * 1024, 1024} },
>            .block_erase = spi_block_erase_d7,    /* sector erase */
>        }, {
>            .eraseblocks = { {64 * 1024, 64} },
>            .block_erase = spi_block_erase_d8,    /* block erase */
>        }, {
>            .eraseblocks = { {4096 * 1024, 1} },
>            .block_erase = spi_block_erase_60,    /* chip erase */
>        }, {
>            .eraseblocks = { {4096 * 1024, 1} },
>            .block_erase = spi_block_erase_c7,    /* chip erase */
>        }
>    },
>    .printlock    = spi_prettyprint_status_register_bp3_srwd,

bit 6 is quad enable
>    .unlock        = spi_disable_blockprotect_bp3_srwd,
>    .write        = spi_chip_write_256,
>    .read        = spi_chip_read, /* Fast read (0x0B), dual I/O
>
> supported */

Actually it supports even multi I/O
>    .voltage    = {2700, 3600}, /* 2.7-3.6V for Pm25Q032 */

The comment there is redundant (and contains a typo). We only comment
the cases where multiple chips share the same ID but have different
voltage ranges. The smaller range goes into the code, the broader one
gets commented.
> },
> {
>    .vendor        = "PMC",
>    .name        = "Pm25LV512(A)",
>    .bustype    = BUS_SPI,
>    .manufacture_id    = PMC_ID_NOPREFIX,
> Index: flashchips.h
>
> --- flashchips.h    (revision 1888)
> +++ flashchips.h    (working copy)
> @@ -560,6 +560,7 @@
>  #define PMC_PM25LD512        0x20    /* Same as Pm25LD512C, but the
> latter has more locking options. */
>  #define PMC_PM25LD010        0x21    /* Same as Pm25LD010C, but the
> latter has more locking options. */
>  #define PMC_PM25LD020        0x22    /* Same as Pm25LD020C, but the
> latter has more locking options. */
> +#define PMC_PM25LQ032        0x46
>  #define PMC_PM25LV512        0x7B    /* Same as Pm25LV512A */
>  #define PMC_PM25LV010        0x7C    /* Same as Pm25LV010A, but the
> former does not support RDID but RES3 only. */
>  #define PMC_PM25LV020        0x7D

The important parts are all correct and I think erase/write should
just work, thank you. If you plan to add the other members of this
chip's family please supersede this patch with the new one and include
the corrections mentioned above. If you do not want to add them please
say so and I'll correct it myself before committing.