Re: [flashrom] [PATCH] Add atapromise programmer

[Carl-Daniel Hailfinger]

Hi Joseph,

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), 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.
> > Signed-off-by: Joseph C. Lehner
>
> 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
> 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...
> 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.
> +{
> 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.
> +{
> 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));
>
> +}
>
> +#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.

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.

Regards,
Carl-Daniel