On 08.05.2009 13:41, Uwe Hermann wrote:
On Fri, May 08, 2009 at 02:57:27AM +0200, Carl-Daniel Hailfinger wrote:
To prepare flashrom for Paraflasher (or other external flasher) support, each chip read/write access is either handled as memory mapped access or external flasher cycle.
This part is ok. These functions certainly don't belong in flash.h.
read_memmapped definitely does not belong in flash.h, true.
External flashers can set the flasher variable to their own ID/number during startup and handle accesses in the switch statement inside chip_{read,write}[bwl].
I'd like to solve this part differently, however. A switch statement becomes very big and ugly very soon if we suport more than one flasher. In addition all flasher code for all of them must be in one single file.
The attached draft (non-compiling patch) uses a struct and functions to solve the issue, then each programmer can have it's own file where it provides the required API functions (read, write, init, shutdown, etc). E.g. we then have paraflasher.c, usbprog.c, etc.
Agreed. I really like your function pointer approach. I had used it myself, but I somehow lost the code and started again from scratch.
Index: flashrom-external_infrastructure/flash.h
--- flashrom-external_infrastructure/flash.h (Revision 471) +++ flashrom-external_infrastructure/flash.h (Arbeitskopie)
[...]
+/* external.c */
I'd name this rather "internal.c" (for internal flasher) or "board.c" (for flashing in the mainboard, vs. using an external flasher).
With your function pointer approach, we have to do it differently anyway.
Index: flashrom-external_infrastructure/external.c
--- flashrom-external_infrastructure/external.c (Revision 0) +++ flashrom-external_infrastructure/external.c (Revision 0) @@ -0,0 +1,111 @@
[...]
+void chip_writew(uint16_t b, volatile void *addr) +void chip_writel(uint32_t b, volatile void *addr)
Will we ever need u16 and u32 accessor functions btw? I think they're unused right now, not sure if future plans require them(?)
I think they can be used to speed up reading (less overhead for some protocols).
Draft for external flasher support.
Signed-off-by: Uwe Hermann uwe@hermann-uwe.de
Index: flash.h
--- flash.h (Revision 472) +++ flash.h (Arbeitskopie)
+struct programmer {
Maybe call it struct programmer_entry to avoid confusion with int programmer.
Anyway, I took your code as inspiration and source and created an alternative patch which should work fine.
Compile tested.
Add external flasher support: - Read/write accesses through function pointers - Command line parameter for internal/external flasher - Board and chipset setup moved to internal init function - Shutdown stuff moved to internal shutdown function
As a side benefit, this will allow us to undo chipset write enable during shutdown.
Runtime tests appreciated.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-external_functionpointer/flash.h =================================================================== --- flashrom-external_functionpointer/flash.h (Revision 473) +++ flashrom-external_functionpointer/flash.h (Arbeitskopie) @@ -76,34 +76,54 @@ #endif #endif
+extern int programmer; +#define PROGRAMMER_INTERNAL 0x00 + +struct programmer_entry { + const char *vendor; + const char *name; + + int (*init) (void); + int (*shutdown) (void); + + void (*chip_writeb) (uint8_t b, volatile void *addr); + void (*chip_writew) (uint16_t b, volatile void *addr); + void (*chip_writel) (uint32_t b, volatile void *addr); + uint8_t (*chip_readb) (const volatile void *addr); + uint16_t (*chip_readw) (const volatile void *addr); + uint32_t (*chip_readl) (const volatile void *addr); +}; + +extern const struct programmer_entry programmer_table[]; + static inline void chip_writeb(uint8_t b, volatile void *addr) { - *(volatile uint8_t *) addr = b; + programmer_table[programmer].chip_writeb(b, addr); }
static inline void chip_writew(uint16_t b, volatile void *addr) { - *(volatile uint16_t *) addr = b; + programmer_table[programmer].chip_writew(b, addr); }
static inline void chip_writel(uint32_t b, volatile void *addr) { - *(volatile uint32_t *) addr = b; + programmer_table[programmer].chip_writel(b, addr); }
static inline uint8_t chip_readb(const volatile void *addr) { - return *(volatile uint8_t *) addr; + return programmer_table[programmer].chip_readb(addr); }
static inline uint16_t chip_readw(const volatile void *addr) { - return *(volatile uint16_t *) addr; + return programmer_table[programmer].chip_readw(addr); }
static inline uint32_t chip_readl(const volatile void *addr) { - return *(volatile uint32_t *) addr; + return programmer_table[programmer].chip_readl(addr); }
#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) @@ -535,6 +555,16 @@ void *physmap(const char *descr, unsigned long phys_addr, size_t len); void physunmap(void *virt_addr, size_t len);
+/* intext.c */ +int internal_init(void); +int internal_shutdown(void); +void internal_chip_writeb(uint8_t b, volatile void *addr); +void internal_chip_writew(uint16_t b, volatile void *addr); +void internal_chip_writel(uint32_t b, volatile void *addr); +uint8_t internal_chip_readb(const volatile void *addr); +uint16_t internal_chip_readw(const volatile void *addr); +uint32_t internal_chip_readl(const volatile void *addr); + /* flashrom.c */ extern int verbose; #define printf_debug(x...) { if (verbose) printf(x); } Index: flashrom-external_functionpointer/intext.c =================================================================== --- flashrom-external_functionpointer/intext.c (Revision 0) +++ flashrom-external_functionpointer/intext.c (Revision 0) @@ -0,0 +1,166 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2009 Carl-Daniel Hailfinger + * + * 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 + */ + +#include <stdint.h> +#include <string.h> +#include <stdlib.h> +#include <errno.h> +#include <pci/pci.h> +#include "flash.h" + +#if defined(__FreeBSD__) || defined(__DragonFly__) +int io_fd; +#endif + +const struct programmer_entry programmer_table[] = { + { + .init = internal_init, + .shutdown = internal_shutdown, + .chip_readb = internal_chip_readb, + .chip_readw = internal_chip_readw, + .chip_readl = internal_chip_readl, + .chip_writeb = internal_chip_writeb, + .chip_writew = internal_chip_writew, + .chip_writel = internal_chip_writel, + }, + + {}, +}; + +struct pci_access *pacc; /* For board and chipset_enable */ + +struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device) +{ + struct pci_dev *temp; + struct pci_filter filter; + + pci_filter_init(NULL, &filter); + filter.vendor = vendor; + filter.device = device; + + for (temp = pacc->devices; temp; temp = temp->next) + if (pci_filter_match(&filter, temp)) + return temp; + + return NULL; +} + +struct pci_dev *pci_card_find(uint16_t vendor, uint16_t device, + uint16_t card_vendor, uint16_t card_device) +{ + struct pci_dev *temp; + struct pci_filter filter; + + pci_filter_init(NULL, &filter); + filter.vendor = vendor; + filter.device = device; + + for (temp = pacc->devices; temp; temp = temp->next) + if (pci_filter_match(&filter, temp)) { + if ((card_vendor == + pci_read_word(temp, PCI_SUBSYSTEM_VENDOR_ID)) + && (card_device == + pci_read_word(temp, PCI_SUBSYSTEM_ID))) + return temp; + } + + return NULL; +} + +int internal_init(void) +{ + int ret = 0; + + /* First get full io access */ +#if defined (__sun) && (defined(__i386) || defined(__amd64)) + if (sysi86(SI86V86, V86SC_IOPL, PS_IOPL) != 0) { +#elif defined(__FreeBSD__) || defined (__DragonFly__) + if ((io_fd = open("/dev/io", O_RDWR)) < 0) { +#else + if (iopl(3) != 0) { +#endif + fprintf(stderr, "ERROR: Could not get IO privileges (%s).\nYou need to be root.\n", strerror(errno)); + exit(1); + } + + /* Initialize PCI access for flash enables */ + pacc = pci_alloc(); /* Get the pci_access structure */ + /* Set all options you want -- here we stick with the defaults */ + pci_init(pacc); /* Initialize the PCI library */ + pci_scan_bus(pacc); /* We want to get the list of devices */ + + /* We look at the lbtable first to see if we need a + * mainboard specific flash enable sequence. + */ + coreboot_init(); + + /* try to enable it. Failure IS an option, since not all motherboards + * really need this to be done, etc., etc. + */ + ret = chipset_flash_enable(); + if (ret == -2) { + printf("WARNING: No chipset found. Flash detection " + "will most likely fail.\n"); + } + + board_flash_enable(lb_vendor, lb_part); + + return ret; +} + +int internal_shutdown(void) +{ +#if defined(__FreeBSD__) || defined(__DragonFly__) + close(io_fd); +#endif + + return 0; +} + +void internal_chip_writeb(uint8_t b, volatile void *addr) +{ + *(volatile uint8_t *) addr = b; +} + +void internal_chip_writew(uint16_t b, volatile void *addr) +{ + *(volatile uint16_t *) addr = b; +} + +void internal_chip_writel(uint32_t b, volatile void *addr) +{ + *(volatile uint32_t *) addr = b; +} + +uint8_t internal_chip_readb(const volatile void *addr) +{ + return *(volatile uint8_t *) addr; +} + +uint16_t internal_chip_readw(const volatile void *addr) +{ + return *(volatile uint16_t *) addr; +} + +uint32_t internal_chip_readl(const volatile void *addr) +{ + return *(volatile uint32_t *) addr; +} + Index: flashrom-external_functionpointer/Makefile =================================================================== --- flashrom-external_functionpointer/Makefile (Revision 473) +++ flashrom-external_functionpointer/Makefile (Arbeitskopie) @@ -34,7 +34,7 @@ w49f002u.o 82802ab.o pm49fl00x.o sst49lf040.o en29f002a.o \ sst49lfxxxc.o sst_fwhub.o layout.o cbtable.o flashchips.o physmap.o \ flashrom.o w39v080fa.o sharplhf00l04.o w29ee011.o spi.o it87spi.o \ - ichspi.o w39v040c.o sb600spi.o wbsio_spi.o m29f002.o + ichspi.o w39v040c.o sb600spi.o wbsio_spi.o m29f002.o intext.o
all: pciutils dep $(PROGRAM)
Index: flashrom-external_functionpointer/flashrom.c =================================================================== --- flashrom-external_functionpointer/flashrom.c (Revision 473) +++ flashrom-external_functionpointer/flashrom.c (Arbeitskopie) @@ -21,7 +21,6 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */
-#include <errno.h> #include <fcntl.h> #include <sys/types.h> #include <sys/stat.h> @@ -30,52 +29,13 @@ #include <string.h> #include <stdlib.h> #include <getopt.h> -#include <pci/pci.h> #include "flash.h"
char *chip_to_probe = NULL; -struct pci_access *pacc; /* For board and chipset_enable */ int exclude_start_page, exclude_end_page; int verbose = 0; +int programmer = PROGRAMMER_INTERNAL;
-struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device) -{ - struct pci_dev *temp; - struct pci_filter filter; - - pci_filter_init(NULL, &filter); - filter.vendor = vendor; - filter.device = device; - - for (temp = pacc->devices; temp; temp = temp->next) - if (pci_filter_match(&filter, temp)) - return temp; - - return NULL; -} - -struct pci_dev *pci_card_find(uint16_t vendor, uint16_t device, - uint16_t card_vendor, uint16_t card_device) -{ - struct pci_dev *temp; - struct pci_filter filter; - - pci_filter_init(NULL, &filter); - filter.vendor = vendor; - filter.device = device; - - for (temp = pacc->devices; temp; temp = temp->next) - if (pci_filter_match(&filter, temp)) { - if ((card_vendor == - pci_read_word(temp, PCI_SUBSYSTEM_VENDOR_ID)) - && (card_device == - pci_read_word(temp, PCI_SUBSYSTEM_ID))) - return temp; - } - - return NULL; -} - void map_flash_registers(struct flashchip *flash) { size_t size = flash->total_size * 1024; @@ -360,9 +320,6 @@ int force = 0; int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; int ret = 0, i; -#if defined(__FreeBSD__) || defined(__DragonFly__) - int io_fd; -#endif
static struct option long_options[] = { {"read", 0, 0, 'r'}, @@ -378,6 +335,7 @@ {"layout", 1, 0, 'l'}, {"image", 1, 0, 'i'}, {"list-supported", 0, 0, 'L'}, + {"programmer", 1, 0, 'p'}, {"help", 0, 0, 'h'}, {"version", 0, 0, 'R'}, {0, 0, 0, 0} @@ -399,7 +357,7 @@ }
setbuf(stdout, NULL); - while ((opt = getopt_long(argc, argv, "rRwvVEfc:s:e:m:l:i:Lh", + while ((opt = getopt_long(argc, argv, "rRwvVEfc:s:e:m:l:i:p:Lh", long_options, &option_index)) != EOF) { switch (opt) { case 'r': @@ -458,6 +416,14 @@ print_supported_boards(); exit(0); break; + case 'p': + if (strncmp(optarg, "internal", 8) == 0) { + programmer = PROGRAMMER_INTERNAL; + } else { + printf("Error: Unknown programmer.\n"); + exit(1); + } + break; case 'R': /* print_version() is always called during startup. */ exit(0); @@ -477,42 +443,10 @@ if (optind < argc) filename = argv[optind++];
- /* First get full io access */ -#if defined (__sun) && (defined(__i386) || defined(__amd64)) - if (sysi86(SI86V86, V86SC_IOPL, PS_IOPL) != 0) { -#elif defined(__FreeBSD__) || defined (__DragonFly__) - if ((io_fd = open("/dev/io", O_RDWR)) < 0) { -#else - if (iopl(3) != 0) { -#endif - fprintf(stderr, "ERROR: Could not get IO privileges (%s).\nYou need to be root.\n", strerror(errno)); - exit(1); - } + ret = programmer_table[programmer].init();
- /* Initialize PCI access for flash enables */ - pacc = pci_alloc(); /* Get the pci_access structure */ - /* Set all options you want -- here we stick with the defaults */ - pci_init(pacc); /* Initialize the PCI library */ - pci_scan_bus(pacc); /* We want to get the list of devices */ - myusec_calibrate_delay();
- /* We look at the lbtable first to see if we need a - * mainboard specific flash enable sequence. - */ - coreboot_init(); - - /* try to enable it. Failure IS an option, since not all motherboards - * really need this to be done, etc., etc. - */ - ret = chipset_flash_enable(); - if (ret == -2) { - printf("WARNING: No chipset found. Flash detection " - "will most likely fail.\n"); - } - - board_flash_enable(lb_vendor, lb_part); - for (i = 0; i < ARRAY_SIZE(flashes); i++) { flashes[i] = probe_flash(i ? flashes[i - 1] + 1 : flashchips, 0); @@ -702,8 +636,7 @@ if (verify_it) ret |= verify_flash(flash, buf);
-#ifdef __FreeBSD__ - close(io_fd); -#endif + programmer_table[programmer].shutdown(); + return ret; }