Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46140 )
Change subject: s25f.c: implement probing and block erasers for Spansion ......................................................................
Patch Set 15:
(4 comments)
https://review.coreboot.org/c/flashrom/+/46140/14/chipdrivers.h File chipdrivers.h:
https://review.coreboot.org/c/flashrom/+/46140/14/chipdrivers.h@23 PS14, Line 23: /* for chipaddr and flashctx */
unrelated change, why click Done?
This is just an extra tab that got left behind, I clicked done because I had deleted the write protect code that comment was referring to. I've reverted the tab now.
https://review.coreboot.org/c/flashrom/+/46140/14/chipdrivers.h@173 PS14, Line 173: /* s25fl.c */
I don't see a s25fl.c. I guess it was merged into s25f.c at some point.
Done
https://review.coreboot.org/c/flashrom/+/46140/14/s25f.c File s25f.c:
https://review.coreboot.org/c/flashrom/+/46140/14/s25f.c@1 PS14, Line 1: /* : * This file is part of the flashrom project. : * : * Copyright (C) 2014 Google Inc. : * : * Redistribution and use in source and binary forms, with or without : * modification, are permitted provided that the following conditions : * are met: : * : * Redistributions of source code must retain the above copyright : * notice, this list of conditions and the following disclaimer. : * : * Redistributions in binary form must reproduce the above copyright : * notice, this list of conditions and the following disclaimer in the : * documentation and/or other materials provided with the distribution. : * : * Neither the name of Google or the names of contributors or : * licensors may be used to endorse or promote products derived from this : * software without specific prior written permission. : * : * This software is provided "AS IS," without a warranty of any kind. : * ALL EXPRESS OR IMPLIED CONDITIONS, REPRESENTATIONS AND WARRANTIES, : * INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, FITNESS FOR A : * PARTICULAR PURPOSE OR NON-INFRINGEMENT, ARE HEREBY EXCLUDED. : * GOOGLE INC AND ITS LICENSORS SHALL NOT BE LIABLE : * FOR ANY DAMAGES SUFFERED BY LICENSEE AS A RESULT OF USING, MODIFYING : * OR DISTRIBUTING THIS SOFTWARE OR ITS DERIVATIVES. IN NO EVENT WILL : * GOOGLE OR ITS LICENSORS BE LIABLE FOR ANY LOST REVENUE, PROFIT OR DATA, : * OR FOR DIRECT, INDIRECT, SPECIAL, CONSEQUENTIAL, INCIDENTAL OR : * PUNITIVE DAMAGES, HOWEVER CAUSED AND REGARDLESS OF THE THEORY OF : * LIABILITY, ARISING OUT OF THE USE OF OR INABILITY TO USE THIS SOFTWARE, : * EVEN IF GOOGLE HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH DAMAGES. : */
Use, […]
Done - except I've kept the original copyright year & name.
https://review.coreboot.org/c/flashrom/+/46140/14/s25f.c@370 PS14, Line 370: int probe_spi_big_spansion(struct flashctx *flash) : { : uint8_t cmd = JEDEC_RDID; : uint8_t dev_id[6]; /* We care only about 6 first bytes */ : : if (spi_send_command(flash, sizeof(cmd), sizeof(dev_id), &cmd, dev_id)) : return 0; : : msg_gdbg("Read id bytes: "); : for (size_t i = 0; i < sizeof(dev_id); i++) : msg_gdbg(" 0x%02x", dev_id[i]); : msg_gdbg(".\n"); : : /* : * The structure of the RDID output is as follows: : * : * offset value meaning : * 00h 01h Manufacturer ID for Spansion : * 01h 20h 128 Mb capacity : * 01h 02h 256 Mb capacity : * 02h 18h 128 Mb capacity : * 02h 19h 256 Mb capacity : * 03h 4Dh Full size of the RDID output (ignored) : * 04h 00h FS: 256-kB physical sectors : * 04h 01h FS: 64-kB physical sectors : * 04h 00h FL: 256-kB physical sectors : * 04h 01h FL: Mix of 64-kB and 4KB overlayed sectors : * 05h 80h FL family : * 05h 81h FS family : * : * Need to use bytes 1, 2, 4, and 5 to properly identify one of eight : * possible chips: : * : * 2 types * 2 possible sizes * 2 possible sector layouts : * : */ : : uint32_t model_id = : dev_id[1] << 24 | : dev_id[2] << 16 | : dev_id[4] << 8 | : dev_id[5] << 0; : : if (dev_id[0] == flash->chip->manufacture_id && model_id == flash->chip->model_id) : return 1; : : return 0; : } :
I think this duplicates logic that is in spi25.c […]
I've looked into rdid_get_ids() and I don't think it is the same as what's going on here - they examine different bytes in the RDID response. Maybe we could combine them somehow, but as you say that can be done later.