Hi Carl-Daniel,
I've updated the patch to address some of the issues you pointed out, namely:
*) Remove the allow32k param *) Renaming fixup_chip to limit_chip (and also make it work when NOT using -c <flashchip>) *) Remove the bridge fixup/break stuff because it's not actually neccessary (turns out the bridge was misconfigured because I had to hot-plug the controller, after a failed flash prevented my machine from booting).
Regarding your other questions, inline answers follow.
On 2016-01-13 11:48, Carl-Daniel Hailfinger wrote:
Hi Joseph,
a few followup questions:
Could you please supply the output of "lspci -nnvvvxxx" (as root) for the device you tested?
04:01.0 Mass storage controller [0180]: Promise Technology, Inc. PDC20267 (FastTrak100/Ultra100) [105a:4d30] (rev 02) Subsystem: Promise Technology, Inc. Ultra100 [105a:4d33] Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort-
SERR- <PERR- INTx-
Latency: 32 Interrupt: pin A routed to IRQ 15 Region 0: I/O ports at d0f0 [size=8] Region 1: I/O ports at d0e0 [size=4] Region 2: I/O ports at d0d0 [size=8] Region 3: I/O ports at d0c0 [size=4] Region 4: I/O ports at d080 [size=64] Region 5: Memory at df300000 (32-bit, non-prefetchable) [size=128K] Expansion ROM at df340000 [disabled] [size=64K] Capabilities: [58] Power Management version 1 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- 00: 5a 10 30 4d 07 00 10 02 02 00 80 01 00 20 00 00 10: f1 d0 00 00 e1 d0 00 00 d1 d0 00 00 c1 d0 00 00 20: 81 d0 00 00 00 00 30 df 00 00 00 00 5a 10 33 4d 30: 00 00 34 df 58 00 00 00 00 00 00 00 0f 01 00 00 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50: ce 3b 00 00 00 00 00 00 01 00 01 00 00 00 00 00 60: 04 f3 4f 00 04 f3 4f 00 04 f3 4f 00 04 f3 4f 00 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Note that BAR5 is listed as having a size of 128 kB, but when reading 128 kB from this physical address, the data repeats every 32 kB. The same thing happens when writing images larger than 32 kB: Writing to address 0x8000 actually writes to 0x0000 (i.e. an address mask of 0x7fff).
Do you know if all address lines (A0-A16) of the flash chip are connected? If not, which are? This would help us determine how much of the flash chip the hardware should be able to address.
I don't, but I'll try to find out.
Oh, and a few more comments below...
On 13.01.2016 02:44, Carl-Daniel Hailfinger wrote:
thanks for your patch!
On 13.01.2016 01:17, Joseph Lehner wrote:
On 2016-01-13 00:29, Joseph Lehner wrote:
This patch adds a new programmer that supports (admittedly ancient) Promise ATA controllers. I've successfully tested this on an Ultra100 controller I had lying around. Mine uses a 128 kB MX29F001T flash chip, but I was not able to write anything past the first page (32 kB),
My suspicion is that you may have been successful writing to bigger addresses, but the read method discarded anything above the 32 kB limit so you could not see what you had written. I can't prove my suspicion, though.
See my comment regarding the lspci output.
which is not that much of a problem, since the option ROM files for these controllers are only 16 kB).
Oh wow. That's just ugly.
It uses a (dirty?) hack to limit the chip size to 16 kB (so you can flash unmodified images) and provides an "allow32k=y" option, allowing you to flash larger images.
IMHO it would make more sense to always limit the size to 32k, and tell people upon programmer init that they can just cat download.rom download.rom > toflash.rom or fill the first 16kiB with 0xff for any firmware downloaded from the manufacturer. This would reduce the complexity of the driver at the cost of a bit more work for the user.
Removed in the attached patch.
Signed-off-by: Joseph C. Lehner joseph.c.lehner@gmail.com
Send patch as text this time.
Nice patch. Review follows inline.
--- /dev/null +++ b/atapromise.c @@ -0,0 +1,259 @@ +/*
- This file is part of the flashrom project.
- Copyright (C) 2015 Joseph C. Lehner joseph.c.lehner@gmail.com
- 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
- */
+#if defined(__i386__) || defined(__x86_64__)
+#include <string.h> +#include <stdlib.h> +#include "flash.h" +#include "programmer.h" +#include "hwaccess.h"
+#define MAX_ROM_DECODE (32 * 1024) +#define ADDR_MASK (MAX_ROM_DECODE - 1)
+/*
- In the absence of any public docs on the PDC2026x family, this programmer
- was created through a mix of reverse-engineering and trial and error.
- The only device tested is an Ultra100 controller, but the logic for
- programming the other 2026x controllers is the same, so it should,
- in theory, work for those as well.
- This programmer is limited to the first 32 kB, which should be sufficient,
Are you sure it's the lowest 32 kiB, not the highest 32 kiB?
- given that the ROM files for these controllers are 16 kB. Since flashrom
- does not support flashing images smaller than the detected flash chip
- (the tested Ultra100 uses a 128 kB MX29F001T chip), the chip size
- is hackishly adjusted in atapromise_fixup_chip.
As mentioned above, 16 kiB support could be delegated to the user in some way. That said, we also need the chip-size fixup during probing. Might make sense to handle this in the generic flashrom core because someone abusing the atapromise programmer as a generic flash programmer would probably like to get a warning instead of missing data...
Done.
- To flash 32 kB files, use "allow32k=y".
- */
+static uint32_t io_base_addr = 0; +static uint32_t rom_base_addr = 0;
+static uint8_t *atapromise_bar = NULL; +static size_t rom_size = 0;
+const struct dev_entry ata_promise[] = {
- {0x105a, 0x4d38, NT, "Promise", "PDC20262 (FastTrak66/Ultra66)"},
- {0x105a, 0x0d30, NT, "Promise", "PDC20265 (FastTrak100 Lite/Ultra100)"},
- {0x105a, 0x4d30, OK, "Promise", "PDC20267 (FastTrak100/Ultra100)"},
- {0},
+};
+static void atapromise_chip_writeb(const struct flashctx *flash, uint8_t val,
chipaddr addr);+static uint8_t atapromise_chip_readb(const struct flashctx *flash,
const chipaddr addr);+static const struct par_master par_master_atapromise = {
.chip_readb = atapromise_chip_readb,.chip_readw = fallback_chip_readw,.chip_readl = fallback_chip_readl,.chip_readn = fallback_chip_readn,.chip_writeb = atapromise_chip_writeb,.chip_writew = fallback_chip_writew,.chip_writel = fallback_chip_writel,.chip_writen = fallback_chip_writen,+};
+void *atapromise_map(const char *descr, uintptr_t phys_addr, size_t len) +{
- /* In case fallback_map ever returns something other than NULL. */
- return NULL;
+}
+static struct pci_dev *atapromise_find_bridge(struct pci_dev *dev) +{
- struct pci_dev *br;
- uint8_t bus_sec, bus_sub, htype;
- for (br = dev->access->devices; br; br = br->next) {
/* Don't use br->hdrtype here! */htype = pci_read_byte(br, PCI_HEADER_TYPE) & 0x7f;if (htype != PCI_HEADER_TYPE_BRIDGE)continue;bus_sec = pci_read_byte(br, PCI_SECONDARY_BUS);bus_sub = pci_read_byte(br, PCI_SUBORDINATE_BUS);if (dev->bus >= bus_sec && dev->bus <= bus_sub) {msg_pdbg("Device is behind bridge %04x:%04x, BDF %02x:%02x.%x.\n",br->vendor_id, br->device_id, br->bus, br->dev, br->func);return br;}- }
- return NULL;
+}
This function is generic and could be useful in pcidev.c. That said, so far we only need it for this driver.
+static int atapromise_fixup_bridge(struct pci_dev *dev)
Does it fix the bridge config, or does it possibly break the bridge config? AFAICS if two PCI devices are behind the bridge and if the non-Promise device has a memory region far away from the Promise memory region, the whole range including both ranges and the unclaimed space in between will be used as memory window of the bridge. AFAICS that may cause havoc (crash, corruption) if some other PCI device behind another bridge uses memory regions in between. I don't see a way to handle this cleanly without a lot more code (and even then), though.
Removed.
+{
- struct pci_dev *br;
- uint16_t reg16;
- /* TODO: What about chained bridges? */
- br = atapromise_find_bridge(dev);
- if (br) {
/* Make sure that the bridge memory windows are set correctly. */reg16 = pci_read_word(dev, PCI_BASE_ADDRESS_5 + 2) & 0xfff0;if (reg16 < pci_read_word(br, PCI_MEMORY_BASE)) {msg_pdbg("Adjusting memory base of bridge to %04x.\n", reg16);rpci_write_word(br, PCI_MEMORY_BASE, reg16);}reg16 += (MAX_ROM_DECODE / 1024);if (reg16 < pci_read_word(br, PCI_MEMORY_LIMIT)) {Inverted logic here?
msg_pdbg("Adjusting memory limit of bridge to %04x.\n", reg16);rpci_write_word(br, PCI_MEMORY_LIMIT, reg16);}- }
- return 0;
+}
+static void atapromise_fixup_chip(struct flashchip *chip)
I wouldn't call it fixup. Mangle or limit might be better words. That said, given the hardware limitations, there's not much you can do unless you figure out a way to perform indirect access.
Done.
+{
- static bool once = false;
- unsigned int i, size;
- if (once)
return;- size = chip->total_size * 1024;
- if (size > rom_size)
- {
/* Undefine all block_erasers that don't operate on the whole chip,* and adjust the eraseblock size of the one that does.*/for (i = 0; i < NUM_ERASEFUNCTIONS; ++i) {if (chip->block_erasers[i].eraseblocks[0].size != size) {chip->block_erasers[i].eraseblocks[0].count = 0;chip->block_erasers[i].block_erase = NULL;} else {chip->block_erasers[i].eraseblocks[0].size = rom_size;break;}}if (i != NUM_ERASEFUNCTIONS) {chip->total_size = rom_size / 1024;if (chip->page_size > rom_size)chip->page_size = rom_size;} else {msg_pwarn("Failed to adjust size of chip \"%s\" (%d kB).\n",chip->name, chip->total_size);}- }
- once = true;
+}
+static bool atapromise_allow32k() +{
- bool ret;
- char *p;
- p = extract_programmer_param("allow32k");
- if (!p) {
return false;- }
- ret = p[0] == '1' || p[0] == 'y' || p[0] == 'Y';
- free(p);
- return ret;
+}
+int atapromise_init(void) +{
- struct pci_dev *dev = NULL;
- char *param_32k = NULL;
- if (rget_io_perms())
return 1;- dev = pcidev_init(ata_promise, PCI_BASE_ADDRESS_4);
- if (!dev)
return 1;- if (atapromise_fixup_bridge(dev))
return 1;- io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_4) & 0xfffe;
- if (!io_base_addr) {
return 1;- }
- /* Not exactly sure what this does, because flashing seems to work
* well without it. However, PTIFLASH does it, so we do it too.*/- OUTB(1, io_base_addr + 0x10);
- rom_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_5);
- if (!rom_base_addr) {
msg_pdbg("Failed to read BAR5\n");return 1;- }
- if (atapromise_allow32k()) {
if (dev->rom_size < (32 * 1024)) {msg_perr("ROM size is reported as %zu kB. Cannot flash 32 kB ""files.\n", dev->rom_size);return 1;}rom_size = 32 * 1024;- } else {
/* Default to 16 kB, so we can flash unpadded images */rom_size = 16 * 1024;- }
- atapromise_bar = (uint8_t*)rphysmap("Promise", rom_base_addr, rom_size);
- if (atapromise_bar == ERROR_PTR) {
return 1;- }
- max_rom_decode.parallel = rom_size;
- register_par_master(&par_master_atapromise, BUS_PARALLEL);
- return 0;
+}
+static void atapromise_chip_writeb(const struct flashctx *flash, uint8_t val,
chipaddr addr)+{
- uint32_t data;
- atapromise_fixup_chip(flash->chip);
Calling atapromise_fixup_chip once for every chip before beginning probe instead of once per access would speed up things, but flashrom currently has no infrastructure to accommodate this. That said, it should be easy to add the infrastructure.
- data = (rom_base_addr + (addr & ADDR_MASK)) << 8 | val;
- OUTL(data, io_base_addr + 0x14);
+}
+static uint8_t atapromise_chip_readb(const struct flashctx *flash,
const chipaddr addr)+{
- atapromise_fixup_chip(flash->chip);
- return pci_mmio_readb(atapromise_bar + (addr & ADDR_MASK));
You're using an asymmetric interface here. You write to the flash chip with OUTL, but you read from the flash chip with readb from another BAR. My hope is that there is another way to read flash contents via OUT*/IN* which doesn't suffer from the address limitation.
As mentioned in my first mail, there is no documentation at all on these chips apart from the kernel drivers in Linux and FreeBSD, which obviously don't handle flash-related stuff. This OUTL/BAR5 combination is also used by PTIFLASH (Promise's DOS-only flash utility).
+}
+#else +#error PCI port I/O access is not supported on this architecture yet. +#endif diff --git a/flashrom.c b/flashrom.c index 113a05b..5503c05 100644 --- a/flashrom.c +++ b/flashrom.c @@ -182,6 +182,18 @@ const struct programmer_entry programmer_table[] = { }, #endif
+#if CONFIG_ATAPROMISE == 1
- {
.name = "atapromise",.type = PCI,.devs.dev = ata_promise,.init = atapromise_init,.map_flash_region = atapromise_map,.unmap_flash_region = fallback_unmap,.delay = internal_delay,- },
+#endif
#if CONFIG_IT8212 == 1 { .name = "it8212",
Overall, I'd say it's really good quality code.
Thanks!
I'd like to have Stefan's input on the chip size mangling and how to solve the problem of flashrom changing chip contents blindly for mangled chips.
I'm wondering if flashrom should maybe provide an option to flash images smaller than the flash chip's capacity (something along the lines of -P/--partial-flash). IMHO this would be a much more elegant solution than hacking the chip size.
Regards, Carl-Daniel
Regards, Joseph