Re: [flashrom] [PATCH] Add Suport for Dr.Kaiser PC-Waechter

[Carl-Daniel Hailfinger]

Hi,

On 03.09.2009 01:12, Uwe Hermann wrote:
> On Wed, Sep 02, 2009 at 09:01:01PM +0200, TURBO J wrote:
> > this patch adds support for a PCI "flash memory card" named "Dr. Kaiser
> > PC-Waechter".
>
> Great, thanks a lot for the patch!
>
> I committed it in r712 with some smaller changes.
> Small whitespace and coding style fixes.
> Made PCI_BASE_ADDRESS_2 support a bit more generic, see below.
> Added programmer to the manpage.

Thanks!
> > // Map 128KB Flash Memory Window
> > drkaiser_mem=physmap("Dr.Kaiser PC-Waechter FLASH Memory",
> > pci_read_long(pcidev_dev,PCI_BASE_ADDRESS_2), 128*1024);
>
> Are you sure 128 KB is the maximum supported size here? The chip on your
> board is 128KB, but maybe if bigger chips were soldered on instead they
> would still work (depends on the FPGA, hardware wiring etc, I guess)?

Interesting question. The peculiar flash chip mapping has a few other
side effects, though:
> +void drkaiser_chip_writeb(uint8_t val, chipaddr addr)
> +{
> mmio_writeb(val, drkaiser_bar + addr);
>
> +}
>
> +uint8_t drkaiser_chip_readb(const chipaddr addr)
> +{
> return mmio_readb(drkaiser_bar + addr);
>
> +}

Please note that addr is _not_ relative to chip start. For any given
chip, the addr of the lowest byte of the chip is 0x100000000-chipsize.
This means your accessor functions have to implement bounds checking and
either error out or wrap around. Example for wrapping around:

void drkaiser_chip_writeb(uint8_t val, chipaddr addr)
{
    mmio_writeb(val, (drkaiser_bar + addr) & ((1 << 17) - 1);
}

Example for aborting:

void drkaiser_chip_writeb(uint8_t val, chipaddr addr)
{
    /* chipaddr is unsigned */
    if (addr > (1 << 17)) {
    	/* Bitch and complain and abort. */
    	fprintf(stderr, "Waah! Accessing outside the BAR, addr=%08lx.\n",
    		chipaddr);
    	/* Maybe exit(1) when we're sure this will not trigger. */
    	return;
    }
    mmio_writeb(val, (drkaiser_bar + addr) & ((1 << 17) - 1);
}

Otherwise we risk writing to and reading from arbitrary RAM/MMIO regions...

There is another problem in the generic PCI code, but that's not your
fault: We don't check if the BAR is 32bit or 64bit. That may cause some
problems on systems with 64bit address space which place device
resources above 4G.

Regards,
Carl-Daniel