On 16.01.2010 13:54, Michael Karcher wrote:
> This mainboard uses external pins for banking the ROM chip, which seems
> to be impossible to do with the "internal" programmer, yet I had to clone
> most of the init stuff from internal.c . Hints for a clean implementation
> will be appreciated.
>
It would be possible to make a copy of the selected struct programmer
array element before running programmer_init, and have the
programmer-specific programmer_init change the function …
[View More]pointers on the
fly. In your case, we'd change all chip_{read,write}[bwln] and
{un,}map_flash_region pointers inside a board enable function. With
register_shutdown it should be possible to have your shutdown function
run in the intended place.
> ---
> Makefile | 2 +-
> flash.h | 11 ++++++
> flashrom.c | 19 ++++++++++
> t20.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 139 insertions(+), 1 deletions(-)
> create mode 100644 t20.c
>
> diff --git a/Makefile b/Makefile
> index 501f65a..cdd45d1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -101,7 +101,7 @@ CONFIG_PRINT_WIKI ?= no
>
> ifeq ($(CONFIG_INTERNAL), yes)
> FEATURE_CFLAGS += -D'INTERNAL_SUPPORT=1'
> -PROGRAMMER_OBJS += chipset_enable.o board_enable.o cbtable.o it87spi.o ichspi.o sb600spi.o wbsio_spi.o
> +PROGRAMMER_OBJS += chipset_enable.o board_enable.o cbtable.o it87spi.o ichspi.o sb600spi.o t20.o wbsio_spi.o
> NEED_PCI := yes
> endif
>
> diff --git a/flash.h b/flash.h
> index 9a25c70..77b0d48 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -58,6 +58,9 @@ enum programmer {
> #if INTERNAL_SUPPORT == 1
> PROGRAMMER_IT87SPI,
> #endif
> +#if INTERNAL_SUPPORT == 1
> + PROGRAMMER_TPT20,
> +#endif
> #if FT2232_SPI_SUPPORT == 1
> PROGRAMMER_FT2232SPI,
> #endif
> @@ -435,6 +438,14 @@ uint8_t drkaiser_chip_readb(const chipaddr addr);
> extern struct pcidev_status drkaiser_pcidev[];
> #endif
>
> +/* t20.c */
> +#if INTERNAL_SUPPORT == 1
> +int t20_init(void);
> +int t20_shutdown(void);
> +void t20_chip_writeb(uint8_t val, chipaddr addr);
> +uint8_t t20_chip_readb(const chipaddr addr);
> +#endif
> +
> /* satasii.c */
> #if SATASII_SUPPORT == 1
> int satasii_init(void);
> diff --git a/flashrom.c b/flashrom.c
> index db44c2f..7f05eeb 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -226,6 +226,25 @@ const struct programmer_entry programmer_table[] = {
> },
> #endif
>
> +#if INTERNAL_SUPPORT == 1
> + {
> + .name = "t20",
> + .init = t20_init,
> + .shutdown = t20_shutdown,
> + .map_flash_region = fallback_map,
> + .unmap_flash_region = fallback_unmap,
> + .chip_readb = t20_chip_readb,
> + .chip_readw = fallback_chip_readw,
> + .chip_readl = fallback_chip_readl,
> + .chip_readn = fallback_chip_readn,
> + .chip_writeb = t20_chip_writeb,
> + .chip_writew = fallback_chip_writew,
> + .chip_writel = fallback_chip_writel,
> + .chip_writen = fallback_chip_writen,
> + .delay = internal_delay,
> + },
> +#endif
> +
> #if FT2232_SPI_SUPPORT == 1
> {
> .name = "ft2232spi",
> diff --git a/t20.c b/t20.c
> new file mode 100644
> index 0000000..d6841cc
> --- /dev/null
> +++ b/t20.c
> @@ -0,0 +1,108 @@
> +/*
> + * This file is part of the flashrom project.
> + *
> + * Copyright (C) 2010 Michael Karcher <flashrom(a)mkarcher.dialup.fu-berlin.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/* Base address of register IND1 in the Field IMGA in the DSDT */
> +#define IMGA1_BASE 0x15EE
>
It would be great if we could get this info from a live system, and not
have to rely on hardcoding it (may depend on BIOS version, installed
hardware etc.). No idea if it is possible to ask ACPI about that value
(at least without including a big ACPI parser in flashrom).
> +
> +#include "flash.h"
> +
> +static uint8_t *t20_base;
> +
> +#define MAX_COUNT 20
> +
> +static void imga1_mask(uint8_t index, uint8_t mask, uint8_t value)
>
This function can fail. Should it return int instead of void?
> +{
> + uint16_t temp;
> + int count = 0;
> + /* Be careful, as ACPI might use these registers too, so check
> + that the index is still OK. */
>
Comment style nitpick: Please start the second and following lines of a
multiline comment with * and terminate such comments on a separate line.
I don't see the "Be careful" motto mentioned above in the code below. If
ACPI accesses IMGA1_BASE, we will pretty sure mess up ACPI accesses
there. It's a race condition, and since we are not in control of ACPI,
there is nothing we can do to reduce impact on the ACPI code. Besides
that, there is a time window between the last INW and the final OUTW
where anything can happen.
> + do {
> + OUTB(index, IMGA1_BASE);
> + temp = INW(IMGA1_BASE);
>
OUTB, but INW. Is there a reason for the size mismatch?
> + }
> + while ((temp & 0xff) != index && ++count < MAX_COUNT);
>
Minor coding style nitpick: The while statement should be on the same
line as the closing brace.
> + if(count == MAX_COUNT)
> + {
> + printf("error: IMGA1 index stuck at %02x\n", temp & 0xff);
> + return;
> + }
> + temp &= ~(mask << 8);
>
OK, maybe I've stared at such code too often, but given that mask is
uint8_t and shifting happens inside brackets, isn't the compiler free to
treat the calculation inside brackets as yielding an uint8_t as well,
causing the contents of the bracket to be always zero, or to wrap around
the bitshift at an 8-bit boundary, causing the contents of the bracket
to be unshifted? I would have expected a cast of mask to uint16_t.
> + temp |= (value & mask) << 8;
> + OUTW(temp, IMGA1_BASE);
>
To be honest, the code flow in this function seems obvious at first, and
on a second look it becomes an example of clever (maybe even too clever)
coding. A few comments about the behaviour of IMGA1_BASE would be very
appreciated. So far it seems that IMGA1_BASE is used to refer to two
registers, namely an index/value pair. The OUTW is a trick to get an
atomic write of index+value (is that guaranteed to be atomic by the x86
ISA?), a similar explanation probably applies to the INW.
> +}
> +
> +int t20_init(void)
> +{
> + pacc = pci_alloc();
> + pci_init(pacc);
> + pci_scan_bus(pacc); /* We want to get the list of devices */
> + get_io_perms();
>
I'm not 100% sure if the ordering above is correct. On some fringe
platform, we might have to get IO permission before scanning the PCI
bus. This is only a minor nitpick, not a hard requirement.
> + /* ID of the graphics chip in T20/T21/T22. The mainboards are
> + identical in these models */
> + if (!pci_card_find(0x5333, 0x8c12, 0x1014, 0x017f))
> + {
> + msg_perr("This is not a ThinkPad T20/T21/T22\n");
> + return 1;
> + }
>
IMHO the matching is too loose here. I'd like to see an additional DMI
match and/or a second PCI device match.
> + if (chipset_flash_enable() == -2) {
> + printf("WARNING: No chipset found. Flash detection "
> + "will most likely fail.\n");
> + }
>
What if the chipset enable returns -1?
> +
> + imga1_mask(0, 4, 4); /* Enable flash writes */
> + t20_base = physmap("IBM T20 mainboard flash", 0xFFFE0000, 0x20000);
>
A comment about the address used for physmap would be very nice, even if
it is just "The EC maps a window of the flash chip to 4G-0x20000".
> + buses_supported = CHIP_BUSTYPE_PARALLEL;
> + return 0;
> +}
> +
> +int t20_shutdown(void)
> +{
> + physunmap(t20_base, 0x20000);
> + imga1_mask(0, 4, 4); /* Disable flash writes */
> + release_io_perms();
> + pci_cleanup(pacc);
> + return 0;
> +}
> +
> +static unsigned int currentbank = -1U;
>
You could move currentbank inside switchtobank. Not sure if that is a
good idea, though.
> +
> +static void switchtobank(unsigned int banknum)
> +{
> + if (banknum > 3)
> + {
> + msg_perr("Bad bank number %d\n", banknum);
> + return;
> + }
> + if(banknum != currentbank)
> + imga1_mask(0, 3, banknum);
> + currentbank = banknum;
> +}
> +
> +void t20_chip_writeb(uint8_t val, chipaddr addr)
> +{
> + switchtobank(addr >> 17);
> + mmio_writeb(val, t20_base + (addr & 0x1FFFF));
> +}
> +
> +uint8_t t20_chip_readb(chipaddr addr)
> +{
> + switchtobank(addr >> 17);
> + return mmio_readb(t20_base + (addr & 0x1FFFF));
> +}
>
Overall, I like the patch and will probably ack it next time.
Regards,
Carl-Daniel
--
Developer quote of the year:
"We are juggling too many chainsaws and flaming arrows and tigers."
[View Less]
SPI RES is the most unreliable way to identify chips because it only
returns a 1-byte ID for most chips. For every given ID out there,
probably a dozen incompatible flash chips match it.
We already refuse to identify a chip with RES if that chip responds to
RDID (3 bytes, good match), and with this patch we additionally refuse
RES if the chip responds to REMS (2 bytes, still a good match). This
increases matching accuracy a lot.
Besides that, the RDID/REMS response checking has been cleaned up …
[View More]for
better readability.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Index: flashrom-spi_res_avoidance/spi.c
===================================================================
--- flashrom-spi_res_avoidance/spi.c (Revision 896)
+++ flashrom-spi_res_avoidance/spi.c (Arbeitskopie)
@@ -385,17 +385,30 @@
{
unsigned char readarr[3];
uint32_t id2;
+ const unsigned char allff[] = {0xff, 0xff, 0xff};
+ const unsigned char all00[] = {0x00, 0x00, 0x00};
- /* Check if RDID was successful and did not return 0xff 0xff 0xff.
- * In that case, RES is pointless.
+ /* Check if RDID is usable and does not return 0xff 0xff 0xff or
+ * 0x00 0x00 0x00. In that case, RES is pointless.
*/
- if (!spi_rdid(readarr, 3) && ((readarr[0] != 0xff) ||
- (readarr[1] != 0xff) || (readarr[2] != 0xff)))
+ if (!spi_rdid(readarr, 3) && memcmp(readarr, allff, 3) &&
+ memcmp(readarr, all00, 3)) {
+ msg_cdbg("Ignoring RES in favour of RDID.\n");
return 0;
+ }
+ /* Check if REMS is usable and does not return 0xff 0xff or
+ * 0x00 0x00. In that case, RES is pointless.
+ */
+ if (!spi_rems(readarr) && memcmp(readarr, allff, JEDEC_REMS_INSIZE) &&
+ memcmp(readarr, all00, JEDEC_REMS_INSIZE)) {
+ msg_cdbg("Ignoring RES in favour of REMS.\n");
+ return 0;
+ }
if (spi_res(readarr))
return 0;
+ /* FIXME: Handle the case where RES gives a 2-byte response. */
id2 = readarr[0];
printf_debug("%s: id 0x%x\n", __func__, id2);
if (id2 != flash->model_id)
--
Developer quote of the year:
"We are juggling too many chainsaws and flaming arrows and tigers."
[View Less]
Author: hailfinger
Date: Fri Feb 12 20:37:25 2010
New Revision: 899
URL: http://flashrom.org/trac/coreboot/changeset/899
Log:
SPI RES is the most unreliable way to identify chips because it only
returns a 1-byte ID for most chips. For every given ID out there,
probably a dozen incompatible flash chips match it.
We already refuse to identify a chip with RES if that chip responds to
RDID (3 bytes, good match), and with this patch we additionally refuse
RES if the chip responds to REMS (2 bytes, …
[View More]still a good match). This
increases matching accuracy a lot.
Besides that, the RDID/REMS response checking has been cleaned up for
better readability.
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Acked-by: Sean Nelson <audiohacked(a)gmail.com>
Modified:
trunk/spi.c
Modified: trunk/spi.c
==============================================================================
--- trunk/spi.c Fri Feb 12 20:35:25 2010 (r898)
+++ trunk/spi.c Fri Feb 12 20:37:25 2010 (r899)
@@ -385,17 +385,30 @@
{
unsigned char readarr[3];
uint32_t id2;
+ const unsigned char allff[] = {0xff, 0xff, 0xff};
+ const unsigned char all00[] = {0x00, 0x00, 0x00};
- /* Check if RDID was successful and did not return 0xff 0xff 0xff.
- * In that case, RES is pointless.
+ /* Check if RDID is usable and does not return 0xff 0xff 0xff or
+ * 0x00 0x00 0x00. In that case, RES is pointless.
*/
- if (!spi_rdid(readarr, 3) && ((readarr[0] != 0xff) ||
- (readarr[1] != 0xff) || (readarr[2] != 0xff)))
+ if (!spi_rdid(readarr, 3) && memcmp(readarr, allff, 3) &&
+ memcmp(readarr, all00, 3)) {
+ msg_cdbg("Ignoring RES in favour of RDID.\n");
return 0;
+ }
+ /* Check if REMS is usable and does not return 0xff 0xff or
+ * 0x00 0x00. In that case, RES is pointless.
+ */
+ if (!spi_rems(readarr) && memcmp(readarr, allff, JEDEC_REMS_INSIZE) &&
+ memcmp(readarr, all00, JEDEC_REMS_INSIZE)) {
+ msg_cdbg("Ignoring RES in favour of REMS.\n");
+ return 0;
+ }
if (spi_res(readarr))
return 0;
+ /* FIXME: Handle the case where RES gives a 2-byte response. */
id2 = readarr[0];
printf_debug("%s: id 0x%x\n", __func__, id2);
if (id2 != flash->model_id)
[View Less]
Fix erase blocks for Winbond W25X* SPI chips. The Winbond W25X10 and
related chips only have 4k and 64k blocks and only accept erase
commands: 20h, d8h, and c7h.
Signed-off-by: Sean Nelson <audiohacked(a)gmail.com>
Adds support to board_enable.c for ASUS A8JM Notebooks (and possibly
all A8J series)
Signed-off-by: James Lancaster <deathstalker(a)gmail.com>
---
I have now gotten it working. I would like to thank some of the people
in IRC for trying to help. It came down to me seeing a few similar 945
+ ICH7 and just trying (first randomly, then systematically) all the
possible GPIO pins. Finding that one needs to raise GPIO pin 34 on the
ich7. The rest was already supported.
Of note:
It does not …
[View More]include DMI matching. I think the PCI IDs will identify it
(and possibly other A8J series) uniquely.
It should work for any A8J_ laptop, but as I don't have any others I'm
not absolutely certain. (I think they simply differ by video card.
except possibly the A8JS.)
An additional warning:
Either 0.91 or r3844 (Currently on Jaunty) partially flashed the bios
initially. I'm not sure how, and frankly, given the hours with a
corrupted flash rom, I'm not going to experiment, unless someone has a
specific idea.
[View Less]
Hi,
I'm trying to flash the latest BIOS-version on a HP Vectra VL420 SFF
but it fails with latest flashrom (r811).
This computer has a motherboard similiar to an ASUS P4B-MX, the HP
board is even labeled "P4B-MX" on the board itself. Some differences
though: The HP board uses an AMI BIOS while the ASUS seems to have an
Award BIOS.
Latest VL420-BIOS (version "JA.01.07US"):
ftp://ftp.hp.com/pub/softpaq/sp30001-30500/sp30221.exe
Info attached.
Thanks!
-mattias