[LinuxBIOS] [PATCH] flashrom: board specific enables based on matching pci-ids.

[Luc Verhaegen]

Hi,

This patch adds initial flashrom support for matching mainboards based 
on pci-ids and pci subsystem/card ids.

I've been using subsystem/card ids as part of my unichrome driver for 
years (xf86-video-unichrome/src/via_id.c). It is a very easy and 
painless system for identifying boards.

It is relatively reliable, as it's always the same vendors that bugger 
them up (like VIA and ECS). Most vendors do play nice. But, in order to 
not mess up on devices like the EPIA-M, 2 sets of ids can be provided 
here. This gives pretty absolute reliability. Boards that don't need 2 
matches can just null the second set.

This code works correctly on:
* EPIA-M with original BIOS. Matched and enabled correctly:
  $ Found "VIA EPIA M/MII/...": Enabling flash write... OK.
* EPIA-M with linuxbios: not matched - pci subsystem ids not set 
  correctly (yet!) - but not a problem as linuxbios never disabled write 
  in the first place.
* Asus A7V400-MX now works correctly (the CrashFree BIOS). This involves
  poking the Winbond LPC superio:
  $ Found "Asus A7V8-MX SE": Enabling flash write... OK.
  (This different name is correct. The difference is the northbridge 
   revision and a sticker over "A7V8-MX SE")
* Mitac 8999 laptop. Here GPIO15 enables the panel backlight, while on
  EPIA-M it enables rom write. Hence this being highly incorrect here.
  Requires nothing board specific and can flash np.
* Shuttle SK43G requires nothing board specific.

There is one more VT823x based device in this house that might get 
tested, and possibly implemented, this weekend.

The Island Aruma support is currently kept as is.

When no match has been made, or an error occured, then the name based 
matching is still attempted.

I'm not a big fan of doing board specific things on the name alone. A
user also shouldn't be allowed to specify a board name as that would 
lead to very randomly poking bits with potentially not-so-fun 
consequences. Testing here should imho involve editing flash_enable.c 
directly.

Island aruma code can be converted with little effort, all it takes is 
the output of lspci -vn.

Commit message, signed-off, etc are done git-style.

Luc Verhaegen.

[Peter Stuge]

On Fri, Mar 23, 2007 at 06:47:43AM +0100, Luc Verhaegen wrote:
> This patch adds initial flashrom support for matching mainboards
> based on pci-ids and pci subsystem/card ids.

Acked-by: Peter Stuge

but I won't commit just yet, holding off for more comments.

//Peter

[Luc Verhaegen]

On Fri, Mar 23, 2007 at 07:46:45AM +0100, Peter Stuge wrote:
> On Fri, Mar 23, 2007 at 06:47:43AM +0100, Luc Verhaegen wrote:
> > This patch adds initial flashrom support for matching mainboards
> > based on pci-ids and pci subsystem/card ids.
>
> Acked-by: Peter Stuge
>
> but I won't commit just yet, holding off for more comments.
>
> //Peter

Yeah, don't.

I've given it some further work, and will look at it again in a few 
hours time.

Luc Verhaegen.

[Stefan Reinauer]

* Luc Verhaegen  [070323 06:47]:
> This patch adds initial flashrom support for matching mainboards based 
> on pci-ids and pci subsystem/card ids.
> This code works correctly on:
> EPIA-M with original BIOS. Matched and enabled correctly:
> $ Found "VIA EPIA M/MII/...": Enabling flash write... OK.
> EPIA-M with linuxbios: not matched - pci subsystem ids not set 
> correctly (yet!) - but not a problem as linuxbios never disabled write 
> in the first place.

Ok, so do I get this right: all boards below might be supported by
LinuxBIOS as well, if we create ports for them based on the Epia-M
and start setting correct subsystem vendor ids?

[..]

do you have all these devices available for testing?
> There is one more VT823x based device in this house that might get 
> tested, and possibly implemented, this weekend.
>
> The Island Aruma support is currently kept as is.

fine.
> When no match has been made, or an error occured, then the name based 
> matching is still attempted.

so no regressions. Good.
> I'm not a big fan of doing board specific things on the name alone. A
> user also shouldn't be allowed to specify a board name as that would 
> lead to very randomly poking bits with potentially not-so-fun 
> consequences.

I trust the user more on knowing what he bought than the board vendors
on always creating a sane design for something they would never see the
need for.

Also: This is nothing that the user would manually choose, except during
migration process. Later these strings are read from the linuxbios
table, which is "the one correct answer" rather than a solution based on
heuristics..
> Testing here should imho involve editing flash_enable.c 
> directly.

Testing?
> Island aruma code can be converted with little effort, all it takes is 
> the output of lspci -vn.

No need to do so. There is no more legacy bios for the island aruma.
It's a LinuxBIOS only machine.

One issue with subsystem IDs: In LinuxBIOS they are board wide.
All devices on a board get the same subsystem id. I would assume this is
a wrong approach, from reading your code. Is it?
> Index: util/flashrom/flash_enable.c
>
> --- util/flashrom/flash_enable.c	(revision 2573)
> +++ util/flashrom/flash_enable.c	(working copy)
> @@ -158,10 +158,13 @@
>    return enable_flash_ich(dev, name, 0xdc);
>  }
>
> -static int enable_flash_vt823x(struct pci_dev *dev, char *name)
> +/*
> */
>
> +static int
> +enable_flash_vt823x(struct pci_dev *dev, char *name)
>  {
>    uint8_t val;
>    int ret = 0;
>
>    /* ROM Write enable */
>
> val = pci_read_byte(dev, 0x40);
>
> @@ -170,37 +173,10 @@
>
> if (pci_read_byte(dev, 0x40) != val) {
>    	printf("Warning: Failed to enable ROM Write on %s\n", name);
> ret = -1;
> return -1;
> }
>    if (dev->device_id == 0x3177) { /* VT8235 */
>            if (!iopl(3)) { /* enable full IO access */
>                    unsigned int base;
>                    /* GPIO12-15 -> output */
>                    val = pci_read_byte(dev, 0xE4);
>                    val |= 0x38;
>                    pci_write_byte(dev, 0xE4, val);
>                    /* Get Power Management IO address. */
>                    base = pci_read_word(dev, 0x88) & 0xFF80;
>                    /* enable GPIO15 which is connected to write protect. */
>                    val = inb(base + 0x4d);
>                    val |= 0xFF;
>                    outb(val, base + 0x4d);
>                    val = inb(base + 0x4E);
>                    val |= 0x0F;
>                    outb(val, base + 0x4E);
>            } else {
>                    printf("Warning; Failed to disable Write Protect"
>                           " on %s (iopl failed)\n", name);
>                    return -1;
>            }
>    }
> return ret;
>    return 0;
>
> }
>
> static int enable_flash_cs5530(struct pci_dev *dev, char *name)
> @@ -531,9 +507,194 @@
>    { "ISLAND", "ARUMA", mbenable_island_aruma },
>  };
>
> +/*
> Board specific handling, matched by up to two full sets of pci-ids.
> */
>
> +/*
> Suited for VIAs EPIA M and MII, and maybe other CLE266 based EPIAs.
> First pci_dev is used and a VT8235 ISA Bridge (0x1106:0x3177)
> Second pci_dev is VIA CLE266 Host Bridge and not used.
> This line doesn't really need to be raised when using linuxbios,
> but it doesn't hurt.
> */
>
> +static int
> +enable_flash_via_epia_m(struct pci_dev *first, struct pci_dev *second,
>                    char *name)
>
> +{
>    unsigned int base;
>    uint8_t val;
>    /* GPIO12-15 -> output */
>    val = pci_read_byte(first, 0xE4);
>    val |= 0x10;
>    pci_write_byte(first, 0xE4, val);
>    /* Get Power Management IO address. */
>    base = pci_read_word(first, 0x88) & 0xFF80;
>    /* enable GPIO15 which is connected to write protect. */
>    val = inb(base + 0x4D);
>    val |= 0x80;
>    outb(val, base + 0x4D);
>    return 0;
>
> +}
>
> +/*
> Winbond LPC super IO.
> Raises the ROM MEMW# line.
> */
>
> +static void
> +w83697_rom_memw_enable(void)
> +{
>    uint8_t val;
>    outb(0x87, 0x2E); /* enable extended functions */
>    outb(0x87, 0x2E);
>    outb(0x24, 0x2E); /* rom bits live here */
>    val = inb(0x2F);
>    if (!(val & 0x02)) /* flash rom enabled? */
>            outb(val | 0x08, 0x2F); /* enable MEMW# */
>    outb(0xAA, 0x2E); /* disable extended functions */
>
> +}
>
> +/*
> Suited for Asus A7V8X-MX SE and A7V400-MX.
> First pci_dev is used and a VT8235 ISA Bridge (0x1106:0x3177)
> Second pci_dev is VIA KM400 Host Bridge and not used.
> */
>
> +static int
> +enable_flash_asus_a7v8x_mx(struct pci_dev *first, struct pci_dev *second,
>                       char *name)
>
> +{
>    uint8_t val;
>    /* This bit is marked reserved actually */
>    val = pci_read_byte(first, 0x59);
>    val &= 0x7F;
>    pci_write_byte(first, 0x59, val);
>    w83697_rom_memw_enable();
>    return 0;
>
> +}
>
> +/*
> We use 2 sets of ids here, you're free to choose which is which. This to
> provide a very high degree of certainty when matching a board. Not every
> vendor handles subsystem/card ids in a sane manner.
> Keep the second set nulled if it should be ignored.
> This does require that linuxbios sets up the subsystem ids properly.
> */
>
> +struct board_pciid_enable {
>    /* any device, but make it sensible, like the isa bridge*/
>    uint16_t  first_vendor;
>    uint16_t  first_device;
>    uint16_t  first_card_vendor;
>    uint16_t  first_card_device;
>    /* any device, but make it sensible, like the host bridge */
>    uint16_t  second_vendor;
>    uint16_t  second_device;
>    uint16_t  second_card_vendor;
>    uint16_t  second_card_device;
>    char  *name;
>    int  (*enable)(struct pci_dev *first, struct pci_dev *second, char *name);
>
> +};
>
> +struct board_pciid_enable board_pciid_enables[] = {
>    { 0x1106, 0x3177, 0x1106, 0xAA01,  0x1106, 0x3123, 0x1106, 0xAA01, "VIA EPIA M/MII/...", enable_flash_via_epia_m },
>    { 0x1106, 0x3177, 0x1043, 0x80A1,  0x1106, 0x3205, 0x1043, 0x8118, "Asus A7V8-MX SE", enable_flash_asus_a7v8x_mx },
>    { 0, 0, 0, 0,  0, 0, 0, 0, NULL, NULL } /* Keep this */
>
> +};
>
> +/*
> */
>
> +static struct pci_dev *
> +device_pciids_match(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, 0x2C)) &&
>                        (card_device == pci_read_word(temp, 0x2E)))
>                            return temp;
>            }
>    return NULL;
>
> +}
>
> +/*
> */
>
> +static int
> +board_flash_enable_from_pciids(void)
> +{
>    struct board_pciid_enable *board = board_pciid_enables;
>    struct pci_dev *first, *second;
>    int ret;
>    for (; board->name; board++) {
>            first = device_pciids_match(board->first_vendor,
>                                        board->first_device,
>                                        board->first_card_vendor,
>                                        board->first_card_device);
>            if (!first)
>                    continue;
>            if (board->second_vendor) {
>                    second = device_pciids_match(board->second_vendor,
>                                                 board->second_device,
>                                                 board->second_card_vendor,
>                                                 board->second_card_device);
>                    if (!second)
>                            continue;
>            } else
>                    second = NULL;
>            printf("Found \"%s\": Enabling flash write...",
>                   board->name);
>            ret = board->enable(first, second, board->name);
>            if (!ret) {
>                    printf(" OK.\n");
>                    return 1; /* Found and success */
>            } else {
>                    printf(" Failed!\n");
>                    return ret; /* error */
>            }
>    }
>    return 0; /* nothing found */
>
> +}
>
> int enable_flash_write()
>  {
> int i;
> int i, ret;
> struct pci_dev *dev = 0;
> FLASH_ENABLE *enable = 0;
>
> @@ -558,13 +719,19 @@
>    /* First look whether we have to do something for this
>     * motherboard.
>     */
> for (i = 0; i < sizeof(mbenables) / sizeof(mbenables[0]); i++) {
> if(lb_vendor && !strcmp(mbenables[i].vendor, lb_vendor) &&
>    lb_part && !strcmp(mbenables[i].part, lb_part)) {
> 	mbenables[i].doit();
> 	break;
> }
> }
>    ret = board_flash_enable_from_pciids();
>    if (ret < 0)
>            printf("Attempting to continue regardless.\n");
>    if (ret != 1) { /* Try to match on name instead. */
>            for (i = 0; i < sizeof(mbenables) / sizeof(mbenables[0]); i++) {
>                    if(lb_vendor && !strcmp(mbenables[i].vendor, lb_vendor) &&
>                       lb_part && !strcmp(mbenables[i].part, lb_part)) {
>                            mbenables[i].doit();
>                            break;
>                    }
>            }
>    }
>
> /* now let's try to find the chipset we have ... */
>  for (i = 0; i < sizeof(enables) / sizeof(enables[0]) && (!dev);

[Luc Verhaegen]

On Fri, Mar 23, 2007 at 06:42:46PM +0100, Stefan Reinauer wrote:
> Luc Verhaegen  [070323 06:47]:
> > This patch adds initial flashrom support for matching mainboards based 
> > on pci-ids and pci subsystem/card ids.
> > This code works correctly on:
> > EPIA-M with original BIOS. Matched and enabled correctly:
> > $ Found "VIA EPIA M/MII/...": Enabling flash write... OK.
> > EPIA-M with linuxbios: not matched - pci subsystem ids not set 
> > correctly (yet!) - but not a problem as linuxbios never disabled write 
> > in the first place.
>
> Ok, so do I get this right: all boards below might be supported by
> LinuxBIOS as well, if we create ports for them based on the Epia-M
> and start setting correct subsystem vendor ids?

No, this is just flashrom working on them. If users want to read/write 
prorietary bioses from linux directly, then they can. It's only the 
various flash enables that need to be written for flashrom to become a 
general tool.

With what you pointed out about how the flash utilities of different
bios makers work, it's easy, no searching involved.

The code for the Asus board is based on what i've seen in the bios. This 
is all for interoperability, so allowed under most copyright regimes 
afaik. There's enough difference between what the assembler looks like 
and what the C code looks like. It's just clearing one bit on the isa 
bridge and setting one bit on the super io, with the superio 
documentation freely available anyway.

As for the other devices: The Shuttle and the Asus have VIAs KM400 
northbridge, if time permits, i will try to implement them fully.

The K8M800 might be fun after that, but i doubt that i will have time 
for that, besides, i currently have just the laptop. The other board in 
this house is a KT600 (not mine), i'm not sure what brand any more. I'm 
just going to implement flash_enable for that too. That's at most an 
hour or so now if it's also an award/phoenix.
> do you have all these devices available for testing?

Yes, what kind of X driver developer would i be if i didn't have the 
hardware :) -ENOHW is my mantra when asked why i don't support a given 
unichrome version. The only thing you do when developing drivers without 
the hardware is shoot yourself in the foot all the time.
> > There is one more VT823x based device in this house that might get 
> > tested, and possibly implemented, this weekend.
> >
> > The Island Aruma support is currently kept as is.
>
> fine.
> > When no match has been made, or an error occured, then the name based 
> > matching is still attempted.
>
> so no regressions. Good.

Well, it's poking the winbond superio through 0x2E/0x2F. It doesn't 
depend on anything. It's all about correctly identifying the board here.
> I trust the user more on knowing what he bought than the board vendors
> on always creating a sane design for something they would never see the
> need for.

Well, you know how i feel about both chip and board vendors :) I still 
have a lot of trust in pci-ids. Two full sets are a very good match 
indeed.
> Also: This is nothing that the user would manually choose, except during
> migration process. Later these strings are read from the linuxbios
> table, which is "the one correct answer" rather than a solution based on
> heuristics..

Right, depend on the linuxbios table. On the other hand, it's too easy 
to copy over a mainboards Options.lb and forget about adjusting the name 
:)

Maybe the linuxbios name is added to the pci-id table as well, NULL if 
unknown.

If there's a linuxbios table present, then just try to match main ids 
and the name. Otherwise, try to match both main and card ids.
> > Testing here should imho involve editing flash_enable.c directly.
>
> Testing?

Testing of flashrom, when it doesn't work straight away. Enabling random 
io bits, as you too pointed out, is not without hazard and should not be 
done lightly. I was lucky that lid events work ok on this laptop when 
i lowered GPIO15 :)
> > Island aruma code can be converted with little effort, all it takes is 
> > the output of lspci -vn.
>
> No need to do so. There is no more legacy bios for the island aruma.
> It's a LinuxBIOS only machine.
>
> One issue with subsystem IDs: In LinuxBIOS they are board wide.
> All devices on a board get the same subsystem id. I would assume this is
> a wrong approach, from reading your code. Is it?

They don't always do that for some reason. It is either a vendor messing 
up or it is done deliberately. But this is exactly the issue that the 
dual id check is supposed to catch. 2 matches give a very very high 
degree of certainty.

Board wide is what makes sense. Not that sense and vendors always go 
together :)

Luc Verhaegen.

[Stefan Reinauer]

* Luc Verhaegen  [070323 21:18]:
> > Ok, so do I get this right: all boards below might be supported by
> > LinuxBIOS as well, if we create ports for them based on the Epia-M
> > and start setting correct subsystem vendor ids?
>
> No, this is just flashrom working on them. If users want to read/write 
> prorietary bioses from linux directly, then they can. It's only the 
> various flash enables that need to be written for flashrom to become a 
> general tool.

Sorry, I asked the wrong way. Do all systems have the same north and
southbridge? Because then we could easily get all of them up in LinuxBIOS
itself in a few days, if there's testhardware. But it looks like it is
not that easy:
> As for the other devices: The Shuttle and the Asus have VIAs KM400 
> northbridge, if time permits, i will try to implement them fully.

Is this very different from the vt8623?
> > Also: This is nothing that the user would manually choose, except during
> > migration process. Later these strings are read from the linuxbios
> > table, which is "the one correct answer" rather than a solution based on
> > heuristics..
>
> Right, depend on the linuxbios table. On the other hand, it's too easy 
> to copy over a mainboards Options.lb and forget about adjusting the name 
> :)

Well, if you do that on an already working port, you might be into
trouble, but then again, if you mess up other files like that, you will
end up with a not bootable system very easily.
> Maybe the linuxbios name is added to the pci-id table as well, NULL if 
> unknown.
>
> If there's a linuxbios table present, then just try to match main ids 
> and the name. Otherwise, try to match both main and card ids.

sounds reasonable.

[LinuxBIOS] [PATCH] flashrom: board specific enables based on matching pci-ids.

[Corey Osgood]

Luc Verhaegen wrote:
> Hi,
>
> This patch adds initial flashrom support for matching mainboards based 
> on pci-ids and pci subsystem/card ids.
>
> I've been using subsystem/card ids as part of my unichrome driver for 
> years (xf86-video-unichrome/src/via_id.c). It is a very easy and 
> painless system for identifying boards.
>
> It is relatively reliable, as it's always the same vendors that bugger 
> them up (like VIA and ECS). Most vendors do play nice. But, in order to 
> not mess up on devices like the EPIA-M, 2 sets of ids can be provided 
> here. This gives pretty absolute reliability. Boards that don't need 2 
> matches can just null the second set.
>
> This code works correctly on:
> EPIA-M with original BIOS. Matched and enabled correctly:
> $ Found "VIA EPIA M/MII/...": Enabling flash write... OK.
> EPIA-M with linuxbios: not matched - pci subsystem ids not set 
> correctly (yet!) - but not a problem as linuxbios never disabled write 
> in the first place.
> Asus A7V400-MX now works correctly (the CrashFree BIOS). This involves
> poking the Winbond LPC superio:
> $ Found "Asus A7V8-MX SE": Enabling flash write... OK.
> (This different name is correct. The difference is the northbridge 
>  revision and a sticker over "A7V8-MX SE")
> Mitac 8999 laptop. Here GPIO15 enables the panel backlight, while on
> EPIA-M it enables rom write. Hence this being highly incorrect here.
> Requires nothing board specific and can flash np.
> Shuttle SK43G requires nothing board specific.
>
> There is one more VT823x based device in this house that might get 
> tested, and possibly implemented, this weekend.
>
> The Island Aruma support is currently kept as is.
>
> When no match has been made, or an error occured, then the name based 
> matching is still attempted.
>
> I'm not a big fan of doing board specific things on the name alone. A
> user also shouldn't be allowed to specify a board name as that would 
> lead to very randomly poking bits with potentially not-so-fun 
> consequences. Testing here should imho involve editing flash_enable.c 
> directly.
>
> Island aruma code can be converted with little effort, all it takes is 
> the output of lspci -vn.
>
> Commit message, signed-off, etc are done git-style.
>
> Luc Verhaegen.

Luc, could you check out the PCChips M789CG for me and see if this can
support it? The stock bios is an AMIBIOS (if it matters), with a VT8235M
southbridge and ITE IT8705f Super I/O (docs here:
http://www.iteusa.com/product_info/file/pc/IT8705F_V.0.4.1.pdf). I've
tried both the epia-m and asus fixups that are currently in your patch,
but neither of them worked, and I know very little about the workings of
flashrom. Here's the line with my vendor ids:

{ 0x1106, 0x3177, 0x1106, 0x0000,  0x1106, 0x3123, 0x1106, 0x0000,
"PCChips M789CG(v3.0)", enable_flash_xxx },

Also, should we be moving these board-specific flash fixups off into
another file? I get the feeling we're going to have a lot of them in the
future.

Thanks,
Corey

[LinuxBIOS] [PATCH] flashrom: board specific enables based on matching pci-ids.

[Corey Osgood]

Whoops, sorry to bother you, seems that the it8705 is currently not
supported in flashrom and the chip is connected to it. The main problem
is that flashrom find the vt8235 and tries to use it, but there's no rom
on it. This could get ugly, but I'm looking at it.

-Corey

[Stefan Reinauer]

* Corey Osgood  [070325 09:06]:
> Whoops, sorry to bother you, seems that the it8705 is currently not
> supported in flashrom and the chip is connected to it. The main problem
> is that flashrom find the vt8235 and tries to use it, but there's no rom
> on it. This could get ugly, but I'm looking at it.

No worries, the vt8235 part is definitely correct and needed.
> From a sw point of view the it8705 has some io lines which are probably

be used to lock the chip write or chip select line.

Stefan

[Stefan Reinauer]

* Corey Osgood  [070325 07:40]:
> Also, should we be moving these board-specific flash fixups off into
> another file? I get the feeling we're going to have a lot of them in the
> future.

Yes, indeed. Luc, could you make this an extra file? Then I'll commit
it.

[Luc Verhaegen]

On Sun, Mar 25, 2007 at 03:04:07PM +0200, Stefan Reinauer wrote:
> Corey Osgood  [070325 07:40]:
> > Also, should we be moving these board-specific flash fixups off into
> > another file? I get the feeling we're going to have a lot of them in the
> > future.
>
> Yes, indeed. Luc, could you make this an extra file? Then I'll commit
> it.

Yeah, sure. Any idea about sensible naming? chip_enable.c and 
board_enable.c?

Luc Verhaegen.

[Stefan Reinauer]

* Luc Verhaegen  [070325 20:00]:
> On Sun, Mar 25, 2007 at 03:04:07PM +0200, Stefan Reinauer wrote:
> > Corey Osgood  [070325 07:40]:
> > > Also, should we be moving these board-specific flash fixups off into
> > > another file? I get the feeling we're going to have a lot of them in the
> > > future.
> >
> > Yes, indeed. Luc, could you make this an extra file? Then I'll commit
> > it.
>
> Yeah, sure. Any idea about sensible naming? chip_enable.c and

maybe chipset_enable?
> board_enable.c?

sounds good.
> Luc Verhaegen.

[Luc Verhaegen]

On Sun, Mar 25, 2007 at 08:09:22PM +0200, Stefan Reinauer wrote:
> maybe chipset_enable?
>
> sounds good.

Done.

The island aruma got renamed to agami aruma at some point, the board 
specific match has been adjusted to that now. I hope that the pci ids i 
gathered from its src/mainboards/ directory are ok. I of course have no 
way of testing this.

The rest is pretty much last patch. Except that board enable functions 
only get passed the name now. The board enable function needs to claim 
its own pci device, this is safer as the ordering of both found pci 
devices (and the need for not having to match this and thus handing the 
callbacks a NULL) was going to get us into trouble.

The difference for chipset enable and flash_enable is the much cleaner 
chipset_flash_enable routine. iopl and preparing of the pci_access has 
been relegated to flash_rom.c. Then there's only some extraneous
whitespace removal and replacing // with /* */. I'm not sure how svn 
handles moving of files, but that's usually a good point to do such 
function-less changes.

Has been tested on all the same machines again. Needs a quick test on 
the agami.

Luc Verhaegen.

[Luc Verhaegen]

On Mon, Mar 26, 2007 at 04:59:18PM +0200, Luc Verhaegen wrote:
> Done.
>
> The island aruma got renamed to agami aruma at some point, the board 
> specific match has been adjusted to that now. I hope that the pci ids i 
> gathered from its src/mainboards/ directory are ok. I of course have no 
> way of testing this.
>
> The rest is pretty much last patch. Except that board enable functions 
> only get passed the name now. The board enable function needs to claim 
> its own pci device, this is safer as the ordering of both found pci 
> devices (and the need for not having to match this and thus handing the 
> callbacks a NULL) was going to get us into trouble.
>
> The difference for chipset enable and flash_enable is the much cleaner 
> chipset_flash_enable routine. iopl and preparing of the pci_access has 
> been relegated to flash_rom.c. Then there's only some extraneous
> whitespace removal and replacing // with /* */. I'm not sure how svn 
> handles moving of files, but that's usually a good point to do such 
> function-less changes.
>
> Has been tested on all the same machines again. Needs a quick test on 
> the agami.
>
> Luc Verhaegen.

ping :)

Anything in this patch that isn't acceptable?

About the island aruma, if that's what's keeping this code back.

It was renamed to the agami aruma in linuxbios already, so only the ones 
running the older linuxbios would've been able to run this anyway. So 
aruma was already broken, if it breaks further with this patch, no 
further harm will be done.

Luc Verhaegen.

[Uwe Hermann]

Here's a quick review:

On Mon, Mar 26, 2007 at 04:59:18PM +0200, Luc Verhaegen wrote:
> been relegated to flash_rom.c. Then there's only some extraneous
> whitespace removal and replacing // with /* */. I'm not sure how svn 
> handles moving of files, but that's usually a good point to do such 
> function-less changes.

Nope, please don't. I suggest to split this up in multiple
patches/steps. First, split flash_enable.c into board_enable.c and
chipset_enable.c without _any_ changes in content (only Makefile fixes
and related adaptions).

Then do the other code changes (with no random cosmetics such as changing
comment style in the patch), then as a last step change any cosmetics
you don't like...

This way all patches only contain related changes and are easily
readable an reviewable.
> Fixes GPIO15 being raised on every VT8235 southbridge, regardless of what
> that line actually controls; rom on EPIA-M, backlight on mitac 8999 laptop.

Please sumbit an extra patch for this please, it's not related to the
rest AFAICS.
> Adds flashrom support for Asus A7V400-MX (KM400 + VT8235)

Ditto. In general, please send rather multiple smaller patches than one
huge patch, that's a lot easier to review and merge. Don't mix unrelated
changes into one huge patch.
> signed-off-by: Luc Verhaegen

Capital 'S'.
> rm -f *.o *~
> rm -f *.o *~ flashrom

Why? 'make distclean' deleteѕ the binary already.
> distclean: clean
>    rm -f $(PROGRAM) .dependencies
> +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;
>
> +}

Please use tabs for indenting (yes, much of the current LinuxBIOS code
needs fixing too, but we should properly indent _new_ code at least).

See also:
http://linuxbios.org/Development_Guidelines#Coding_Style

Uwe.

[Luc Verhaegen]

On Mon, Apr 02, 2007 at 12:36:28AM +0200, Uwe Hermann wrote:
> Here's a quick review:
>
> On Mon, Mar 26, 2007 at 04:59:18PM +0200, Luc Verhaegen wrote:
> > been relegated to flash_rom.c. Then there's only some extraneous
> > whitespace removal and replacing // with /* */. I'm not sure how svn 
> > handles moving of files, but that's usually a good point to do such 
> > function-less changes.
>
> Nope, please don't. I suggest to split this up in multiple
> patches/steps. First, split flash_enable.c into board_enable.c and
> chipset_enable.c without _any_ changes in content (only Makefile fixes
> and related adaptions).
>
> Then do the other code changes (with no random cosmetics such as changing
> comment style in the patch), then as a last step change any cosmetics
> you don't like...
>
> This way all patches only contain related changes and are easily
> readable an reviewable.
> > Fixes GPIO15 being raised on every VT8235 southbridge, regardless of what
> > that line actually controls; rom on EPIA-M, backlight on mitac 8999 laptop.
>
> Please sumbit an extra patch for this please, it's not related to the
> rest AFAICS.
> > Adds flashrom support for Asus A7V400-MX (KM400 + VT8235)
>
> Ditto. In general, please send rather multiple smaller patches than one
> huge patch, that's a lot easier to review and merge. Don't mix unrelated
> changes into one huge patch.
> > signed-off-by: Luc Verhaegen
>
> Capital 'S'.
> > rm -f *.o *~
> > rm -f *.o *~ flashrom
>
> Why? 'make distclean' delete?? the binary already.
> > distclean: clean
> >    rm -f $(PROGRAM) .dependencies
> > +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;
> >
> > +}
>
> Please use tabs for indenting (yes, much of the current LinuxBIOS code
> needs fixing too, but we should properly indent _new_ code at least).
>
> See also:
> http://linuxbios.org/Development_Guidelines#Coding_Style
>
> Uwe.

Ok, will do when i have some time again.

Luc Verhaegen.

[Peter Stuge]

On Sun, Mar 25, 2007 at 08:00:39PM +0200, Luc Verhaegen wrote:
> Any idea about sensible naming? chip_enable.c and board_enable.c?

Two files?

I'd say write_enable, since that's the usual name for the signal.

//Peter

[Luc Verhaegen]

On Sun, Mar 25, 2007 at 01:40:23AM -0400, Corey Osgood wrote:
> Luc, could you check out the PCChips M789CG for me and see if this can
> support it? The stock bios is an AMIBIOS (if it matters), with a VT8235M
> southbridge and ITE IT8705f Super I/O (docs here:
> http://www.iteusa.com/product_info/file/pc/IT8705F_V.0.4.1.pdf). I've
> tried both the epia-m and asus fixups that are currently in your patch,
> but neither of them worked, and I know very little about the workings of
> flashrom. Here's the line with my vendor ids:
>
> { 0x1106, 0x3177, 0x1106, 0x0000,  0x1106, 0x3123, 0x1106, 0x0000,
> "PCChips M789CG(v3.0)", enable_flash_xxx },
>
> Also, should we be moving these board-specific flash fixups off into
> another file? I get the feeling we're going to have a lot of them in the
> future.
>
> Thanks,
> Corey

I'll hand you a bit of python in a few hours that gathers the following 
information.
* Int16 vector for AMI
* Dump of 0xE0000-0xFFFFF. The int16 vector should live here, and the 
AWDFLASH structure exists here too.

So a tiny bit of python will provide for AMI and AWD.

Luc Verhaegen.

[Luc Verhaegen]

On Sun, Mar 25, 2007 at 06:05:51PM +0200, Luc Verhaegen wrote:
> On Sun, Mar 25, 2007 at 01:40:23AM -0400, Corey Osgood wrote:
> > Luc, could you check out the PCChips M789CG for me and see if this can
> > support it? The stock bios is an AMIBIOS (if it matters), with a VT8235M
> > southbridge and ITE IT8705f Super I/O (docs here:
> > http://www.iteusa.com/product_info/file/pc/IT8705F_V.0.4.1.pdf). I've
> > tried both the epia-m and asus fixups that are currently in your patch,
> > but neither of them worked, and I know very little about the workings of
> > flashrom. Here's the line with my vendor ids:
> >
> > { 0x1106, 0x3177, 0x1106, 0x0000,  0x1106, 0x3123, 0x1106, 0x0000,
> > "PCChips M789CG(v3.0)", enable_flash_xxx },

Oh, PCChips, isn't that another name for Asrock isn't it? In any case, 
that's yet another known pci-id offender.
> > Also, should we be moving these board-specific flash fixups off into
> > another file? I get the feeling we're going to have a lot of them in the
> > future.
> >
> > Thanks,
> > Corey
>
> I'll hand you a bit of python in a few hours that gathers the following 
> information.
> Int16 vector for AMI
> Dump of 0xE0000-0xFFFFF. The int16 vector should live here, and the
>
> AWDFLASH structure exists here too.
>
> So a tiny bit of python will provide for AMI and AWD.

Attached.

Do send the rom to me personally, i don't think everyone on the 
list wants to receive a 128kB file :)

Luc Verhaegen.

[Luc Verhaegen]

On Mon, Mar 26, 2007 at 08:52:16AM +0200, Luc Verhaegen wrote:
> On Sun, Mar 25, 2007 at 06:05:51PM +0200, Luc Verhaegen wrote:
> > On Sun, Mar 25, 2007 at 01:40:23AM -0400, Corey Osgood wrote:
> > > Luc, could you check out the PCChips M789CG for me and see if this can
> > > support it? The stock bios is an AMIBIOS (if it matters), with a VT8235M
> > > southbridge and ITE IT8705f Super I/O (docs here:
> > > http://www.iteusa.com/product_info/file/pc/IT8705F_V.0.4.1.pdf). I've
> > > tried both the epia-m and asus fixups that are currently in your patch,
> > > but neither of them worked, and I know very little about the workings of
> > > flashrom. Here's the line with my vendor ids:
> > >
> > > { 0x1106, 0x3177, 0x1106, 0x0000,  0x1106, 0x3123, 0x1106, 0x0000,
> > > "PCChips M789CG(v3.0)", enable_flash_xxx },
>
> Oh, PCChips, isn't that another name for Asrock isn't it? In any case, 
> that's yet another known pci-id offender.
> > > Also, should we be moving these board-specific flash fixups off into
> > > another file? I get the feeling we're going to have a lot of them in the
> > > future.
> > >
> > > Thanks,
> > > Corey
> >
> > I'll hand you a bit of python in a few hours that gathers the following 
> > information.
> > Int16 vector for AMI
> > Dump of 0xE0000-0xFFFFF. The int16 vector should live here, and the
> >
> > AWDFLASH structure exists here too.
> >
> > So a tiny bit of python will provide for AMI and AWD.
>
> Attached.
>
> Do send the rom to me personally, i don't think everyone on the 
> list wants to receive a 128kB file :)
>
> Luc Verhaegen.

This dump is fully non-intrusive, and it would allow me to:
* Fix this PCChips motherboard.
* Find out whether it really is as easy under AMI as i think it is :)
  I've already seen that it's highly trivial under award/phoenix with 
  the asus board.

So the sooner you send this on, the sooner your motherboard will be 
happy and the sooner some general conclusion is reached about the flash 
enables. :)

Imho an lrmi extension is not a good option, the knowledge of flash 
enable disappears as soon as something other than the manufacturers rom 
is installed.

flashrom needs native support for each motherboard it wishes to support.

Luc Verhaegen.

[LinuxBIOS] [PATCH] flashrom: board specific enables based on matching pci-ids.

[Corey Osgood]

Luc Verhaegen wrote:
> This dump is fully non-intrusive, and it would allow me to:
> Fix this PCChips motherboard.
> Find out whether it really is as easy under AMI as i think it is :)
> I've already seen that it's highly trivial under award/phoenix with 
> the asus board.
>
> So the sooner you send this on, the sooner your motherboard will be 
> happy and the sooner some general conclusion is reached about the flash 
> enables. :)
>
> Imho an lrmi extension is not a good option, the knowledge of flash 
> enable disappears as soon as something other than the manufacturers rom 
> is installed.
>
> flashrom needs native support for each motherboard it wishes to support.
>
> Luc Verhaegen.

Sorry to keep you waiting, the board rejected Gentoo because I forgot to
get the i586 stage 3 tarball instead of the i686 one, and I haven't had
time to mess around with it again. I'm about to start the Ubuntu install
again, I'll have that dump for you in a couple hours.

-Corey

[David H. Barr]

On 3/25/07, Corey Osgood  wrote:
> Luc, could you check out the PCChips M789CG for me

Just a warning: like many of PC Chips' products, the M789CG comes in
several revisions.  To the best of my knowledge, the 2.x series have
an unsocketed PLCC-32 part, where the 3.x series have a socketed
PLCC-32 part (counterintuitive, to say the least).  I have no idea
what other differences may be present.

-dhbarr.