Re: [coreboot] [PATCH] flashrom: Infrastructure for ICH9 merge

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> Add more infrastructure for flashrom ICH9 support.
>
> Signed-off-by: Carl-Daniel Hailfinger

Good direction, but I have to NACK for a few things:
> /* Calculate the Root Complex Register Block address */
> tmp &= 0xffffc000;
> printf_debug("Root Complex Base Address Register = 0x%x\n", tmp);
> rcba = mmap(0, 0x3510, PROT_READ, MAP_SHARED, fd_mem, (off_t)tmp);
> if (rcba == MAP_FAILED) {
> printf_debug("Root Complex Register Block address = 0x%x\n", tmp);
> rcrb = mmap(0, 0x3A00, PROT_READ, MAP_SHARED, fd_mem, (off_t)tmp);

The rcrb has a size of 16K (0x4000). Please map the whole bar. Mapping 
14.5K of a 16K bar seems a bit artificial.
> ich_spibar = mmap(0, 0x200, PROT_READ, MAP_SHARED, fd_mem_uncached,
> 	 (off_t)tmp + (off_t)spibar);

The SPI bar is not a bar itself, but it is part of the RCBA. Please just 
map this once and make ich_spibar point into that.
> Index: flashrom-ich9/flashrom.c
>
> --- flashrom-ich9/flashrom.c	(Revision 3311)
> +++ flashrom-ich9/flashrom.c	(Arbeitskopie)
> @@ -45,6 +45,7 @@
>  int exclude_start_page, exclude_end_page;
>  int force = 0, verbose = 0;
>  int fd_mem;
> +int fd_mem_uncached;
>
> struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device)
>  {
> @@ -387,6 +388,12 @@
>    	       ". You need to be root.");
>    	exit(1);
>    }
> /* Open the memory device UNCACHED. That's important for MMIO. */
> if ((fd_mem_uncached = open(MEM_DEV, O_RDWR|O_SYNC)) < 0) {
> perror("Error: Can not access memory using " MEM_DEV
>        ". You need to be root.");
> exit(1);
> }

Please don't open the memory device twice. Instead just change the 
already existing open on the memory device to be uncached.

With above changes:

Acked-by: Stefan Reinauer

Stefan