We've had some changes pushed which breaks the previous patch. So here is an update.
Luc Verhaegen.
On Wed, Dec 16, 2009 at 12:14:02PM +0100, Luc Verhaegen wrote:
We've had some changes pushed which breaks the previous patch. So here is an update.
Luc Verhaegen.
No ACKs?
This code has been tested on several of my boards and works just fine. All this code really needs is an ACK from a conceptual/review pov.
Luc Verhaegen.
On Mon, Dec 21, 2009 at 03:04:01PM +0100, Luc Verhaegen wrote:
On Wed, Dec 16, 2009 at 12:14:02PM +0100, Luc Verhaegen wrote:
We've had some changes pushed which breaks the previous patch. So here is an update.
Luc Verhaegen.
No ACKs?
This code has been tested on several of my boards and works just fine. All this code really needs is an ACK from a conceptual/review pov.
Luc Verhaegen.
With the two boards added to the known good list (Even though i seriously doubt the advantage and definitely doubt the medium and longer term correctness of such a list).
-> r815.
Luc Verhaegen.
On 16.12.2009 12:14, Luc Verhaegen wrote:
Chipset/Board: vt8237: Set All mem cycles to LPC in chipset enable.
I don't have the datasheet, so I can't cross-check. I'm very interested why this would be needed, though. If mem cycles don't go to LPC and we have flash on LPC, the board won't boot anyway. If we force mem cycles to LPC although we have parallel ROM, this will break. Does VT8237R support parallel ROMs?
Only done for VT8237R (possibly needed for VT8237 too), VT8235 does not need this (even if the original bios does so: Asus A7V8X-MX SE, MSI KT4V were verified).
Yes, the VT8235 datasheet says those bits are reserved.
How about VT8237A and VT8237S?
This then opens a floodgate of cleanups in the board enables.
- EPIA SP board enable vanishes, taking EPIA CN match with it.
Those should be moved to the "known good" list in print.c or they will end up not being listed as supported.
- Asus A7V8X-MX/Tyan S2498 board enable then equals w836xx_memw_enable_2e
- AOpen vKM400Am-S board enable then equals it8705_rom_write_enable
- Epia M board enable becomes via_vt823x_gpio15_raise
- Epia N board enable becomes via_vt823x_gpio9_raise
- Asus M2V-MX board enable becomes via_vt823x_gpio5_raise
- vt823x_gpio_set becomes via_vt823x_gpio_set, and now detects ISA bridge itself, in concordance with intel ich and nvidia mcp gpio.
I do like the GPIO cleanups. They make the board enables shorter and more readable.
--- a/chipset_enable.c +++ b/chipset_enable.c @@ -603,6 +603,13 @@ static int enable_flash_vt823x(struct pci_dev *dev, const char *name) return -1; }
- if (dev->device_id == 0x3227) { /* VT8237 */
Should say VT8237R, not VT8237 (at least according to your comments in other places in the code).
/* All memory cycles, not just ROM ones, go to LPC. */val = pci_read_byte(dev, 0x59);val &= ~0x80;pci_write_byte(dev, 0x59, val);- }
- return 0;
}
Side note: Should we read the ROMCS# strap on VT8235 to check whether the ROM is LPC or Parallel? Does this strap exist for other VT823x as well?
I found a bug and two weirdnesses (at least when using VT8235 datasheet as reference) in the original code for vt823x_gpio_set(): GPIO 9 on VT8235 needs val |= 0x08, not val |= 0x20. Not sure whether a fix here would break some board enables. For GPIOs 9,12-13 we have to make sure that PCI config reg 0x53 bit 7 (PCI DMA Pair A and Pair B) is 0. For GPIOs 12-15 we have to make sure that PCI config reg 0x5b bit 1 (INTE#, INTF#, INTG#, INTH#) is 0.
If you have a good explanation for the mem cycle question and if you'll add the removed boards to the known good list either as followup patch or right now, this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
A bugfix for the GPIO function would be appreciated, but that's a separate issue. I don't have VT8237* datasheets, so I can't do that myself.
Regards, Carl-Daniel
On Tue, Dec 22, 2009 at 03:48:38AM +0100, Carl-Daniel Hailfinger wrote:
On 16.12.2009 12:14, Luc Verhaegen wrote:
Chipset/Board: vt8237: Set All mem cycles to LPC in chipset enable.
I don't have the datasheet, so I can't cross-check. I'm very interested why this would be needed, though. If mem cycles don't go to LPC and we have flash on LPC, the board won't boot anyway. If we force mem cycles to LPC although we have parallel ROM, this will break. Does VT8237R support parallel ROMs?
We need this, otherwise we wouldn't be changing this many board enables now.
About parallel roms, no idea, but if we apply this patch, we should soon find out.
Only done for VT8237R (possibly needed for VT8237 too), VT8235 does not need this (even if the original bios does so: Asus A7V8X-MX SE, MSI KT4V were verified).
Yes, the VT8235 datasheet says those bits are reserved.
How about VT8237A and VT8237S?
No idea. Although no boards have needed this so far.
This then opens a floodgate of cleanups in the board enables.
- EPIA SP board enable vanishes, taking EPIA CN match with it.
Those should be moved to the "known good" list in print.c or they will end up not being listed as supported.
Not a big fan of a list which is constantly out-of-date, but will do in a follow up patch.
I do like the GPIO cleanups. They make the board enables shorter and more readable.
Yes, we have enough board enables today to be starting to see a line in them.
Should say VT8237R, not VT8237 (at least according to your comments in other places in the code).
Ok.
/* All memory cycles, not just ROM ones, go to LPC. */val = pci_read_byte(dev, 0x59);val &= ~0x80;pci_write_byte(dev, 0x59, val);- }
- return 0;
}
Side note: Should we read the ROMCS# strap on VT8235 to check whether the ROM is LPC or Parallel? Does this strap exist for other VT823x as well?
I found a bug and two weirdnesses (at least when using VT8235 datasheet as reference) in the original code for vt823x_gpio_set(): GPIO 9 on VT8235 needs val |= 0x08, not val |= 0x20. Not sure whether a fix here would break some board enables. For GPIOs 9,12-13 we have to make sure that PCI config reg 0x53 bit 7 (PCI DMA Pair A and Pair B) is 0. For GPIOs 12-15 we have to make sure that PCI config reg 0x5b bit 1 (INTE#, INTF#, INTG#, INTH#) is 0.
If you have a good explanation for the mem cycle question and if you'll add the removed boards to the known good list either as followup patch or right now, this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
First part: not sure we should look for an explanation first, or just wait for the reports to roll in. The VT8237R is very popular, it will be a matter of wait and see.
Second part, ok.
A bugfix for the GPIO function would be appreciated, but that's a separate issue. I don't have VT8237* datasheets, so I can't do that myself.
Ok, will have a look too.
Luc Verhaegen.
On 22.12.2009 13:52, Luc Verhaegen wrote:
On Tue, Dec 22, 2009 at 03:48:38AM +0100, Carl-Daniel Hailfinger wrote:
On 16.12.2009 12:14, Luc Verhaegen wrote:
Chipset/Board: vt8237: Set All mem cycles to LPC in chipset enable.
I don't have the datasheet, so I can't cross-check. I'm very interested why this would be needed, though. If mem cycles don't go to LPC and we have flash on LPC, the board won't boot anyway. If we force mem cycles to LPC although we have parallel ROM, this will break. Does VT8237R support parallel ROMs?
We need this, otherwise we wouldn't be changing this many board enables now.
Ah hm. I thought most board enables were created by imitating the behaviour of the in-memory board enable code, not by checking for the necessity of each step in there. If there are VT8237R straps which select the ROM type, we should check them and warn very loudly if a board strapped to parallel gets all memory accesses redirected to LPC. That will make sure users know WTF went wrong (if something went wrong).
About parallel roms, no idea, but if we apply this patch, we should soon find out.
Side note: Should we read the ROMCS# strap on VT8235 to check whether the ROM is LPC or Parallel? Does this strap exist for other VT823x as well?
The strap reading is IMHO very desirable because it can reduce the number of probed chips roughly by half.
If you have a good explanation for the mem cycle question and if you'll add the removed boards to the known good list either as followup patch or right now, this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
First part: not sure we should look for an explanation first, or just wait for the reports to roll in. The VT8237R is very popular, it will be a matter of wait and see.
Second part, ok.
A bugfix for the GPIO function would be appreciated, but that's a separate issue. I don't have VT8237* datasheets, so I can't do that myself.
Ok, will have a look too.
Deal. Go ahead and commit.
Regards, Carl-Daniel
On Tue, Dec 22, 2009 at 02:25:41PM +0100, Carl-Daniel Hailfinger wrote:
On 22.12.2009 13:52, Luc Verhaegen wrote:
On Tue, Dec 22, 2009 at 03:48:38AM +0100, Carl-Daniel Hailfinger wrote:
On 16.12.2009 12:14, Luc Verhaegen wrote:
Chipset/Board: vt8237: Set All mem cycles to LPC in chipset enable.
I don't have the datasheet, so I can't cross-check. I'm very interested why this would be needed, though. If mem cycles don't go to LPC and we have flash on LPC, the board won't boot anyway. If we force mem cycles to LPC although we have parallel ROM, this will break. Does VT8237R support parallel ROMs?
We need this, otherwise we wouldn't be changing this many board enables now.
Ah hm. I thought most board enables were created by imitating the behaviour of the in-memory board enable code, not by checking for the necessity of each step in there. If there are VT8237R straps which select the ROM type, we should check them and warn very loudly if a board strapped to parallel gets all memory accesses redirected to LPC. That will make sure users know WTF went wrong (if something went wrong).
About parallel roms, no idea, but if we apply this patch, we should soon find out.
Side note: Should we read the ROMCS# strap on VT8235 to check whether the ROM is LPC or Parallel? Does this strap exist for other VT823x as well?
The strap reading is IMHO very desirable because it can reduce the number of probed chips roughly by half.
The bit we are setting is the following:
ROM Memory Cycles Go To LPC 0 Disable (all memory cycles go to LPC). default. 1 Enable (only ROM memory cycles go to LPC).
What we do is set this bit to 0.
This bit is present in 8235 too, but there it seems to be not necessary.
It seems like a bug in vt8237R, and in 8237R only, as we only need to set it there.
Luc Verhaegen.
On 22.12.2009 18:41, Luc Verhaegen wrote:
On Tue, Dec 22, 2009 at 02:25:41PM +0100, Carl-Daniel Hailfinger wrote:
On 22.12.2009 13:52, Luc Verhaegen wrote:
On Tue, Dec 22, 2009 at 03:48:38AM +0100, Carl-Daniel Hailfinger wrote:
On 16.12.2009 12:14, Luc Verhaegen wrote:
Chipset/Board: vt8237: Set All mem cycles to LPC in chipset enable.
I don't have the datasheet, so I can't cross-check. I'm very interested why this would be needed, though. If mem cycles don't go to LPC and we have flash on LPC, the board won't boot anyway. If we force mem cycles to LPC although we have parallel ROM, this will break. Does VT8237R support parallel ROMs?
We need this, otherwise we wouldn't be changing this many board enables now.
Ah hm. I thought most board enables were created by imitating the behaviour of the in-memory board enable code, not by checking for the necessity of each step in there. If there are VT8237R straps which select the ROM type, we should check them and warn very loudly if a board strapped to parallel gets all memory accesses redirected to LPC. That will make sure users know WTF went wrong (if something went wrong).
About parallel roms, no idea, but if we apply this patch, we should soon find out.
Side note: Should we read the ROMCS# strap on VT8235 to check whether the ROM is LPC or Parallel? Does this strap exist for other VT823x as well?
The strap reading is IMHO very desirable because it can reduce the number of probed chips roughly by half.
The bit we are setting is the following:
ROM Memory Cycles Go To LPC 0 Disable (all memory cycles go to LPC). default. 1 Enable (only ROM memory cycles go to LPC).
What we do is set this bit to 0.
This bit is present in 8235 too, but there it seems to be not necessary.
It seems like a bug in vt8237R, and in 8237R only, as we only need to set it there.
Thanks for the explanation.
A slightly modified patch was committed in r815.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Luc Verhaegen