[PATCH] flashrom: Rewrite SB600 chipset enable function

[Carl-Daniel Hailfinger]

Rewrite the SB600 chipset enable function:
- Check for read/write protected regions first.
- Region protection is write-once according to the data sheets. Check if
the write succeeded.
- Verbose region protection dumping.
- Improve readability of BAR mapping code.
- Align BAR mapping to a page boundary (4k) instead of a 16k boundary.

This patch prepares the code for a SPI detection heuristic.

Tests are appreciated.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-sb600/chipset_enable.c
===================================================================
--- flashrom-sb600/chipset_enable.c	(Revision 459)
+++ flashrom-sb600/chipset_enable.c	(Arbeitskopie)
@@ -658,33 +658,47 @@

static int enable_flash_sb600(struct pci_dev *dev, const char *name)
 {
-	uint32_t tmp, low_bits, num;
+	uint32_t tmp, prot;
    uint8_t reg;

-	low_bits = tmp = pci_read_long(dev, 0xa0);
-	low_bits &= ~0xffffc000; /* for mmap aligning requirements */
-	low_bits &= 0xfffffff0;	/* remove low 4 bits */
-	tmp &= 0xffffc000;
-	printf_debug("SPI base address is at 0x%x\n", tmp + low_bits);
-
-	sb600_spibar = physmap("SB600 SPI registers", tmp, 0x4000);
-	sb600_spibar += low_bits;
-
    /* Clear ROM protect 0-3. */
    for (reg = 0x50; reg < 0x60; reg += 4) {
-		num = pci_read_long(dev, reg);
-		num &= 0xfffffffc;
-		pci_write_byte(dev, reg, num);
+		prot = pci_read_long(dev, reg);
+		if (prot & 0x3)
+			printf("SB600 %s %s protected from %u to %u\n",
+				(prot & 0x1) ? "write" : "",
+				(prot & 0x2) ? "read" : "",
+				(prot & 0xfffffc00),
+				(prot & 0xfffffc00) + ((prot & 0x3ff) << 8));
+		prot &= 0xfffffffc;
+		pci_write_byte(dev, reg, prot);
+		prot = pci_read_long(dev, reg);
+		if (prot & 0x3)
+			printf("SB600 still %s %s protected from %u to %u\n",
+				(prot & 0x1) ? "write" : "",
+				(prot & 0x2) ? "read" : "",
+				(prot & 0xfffffc00),
+				(prot & 0xfffffc00) + ((prot & 0x3ff) << 8));
    }

+	/* Read SPI_BaseAddr */
+	tmp = pci_read_long(dev, 0xa0);
+	tmp &= 0xfffffff0;	/* remove low 4 bits (reserved) */
+	printf_debug("SPI base address is at 0x%x\n", tmp);
+
+	/* Physical memory can only be mapped at page (4k) boundaries */
+	sb600_spibar = physmap("SB600 SPI registers", tmp & 0xfffff000, 0x1000);
+	/* The low bits of the SPI base address are used as offset into the mapped page */
+	sb600_spibar += tmp & 0xfff;
+
    flashbus = BUS_TYPE_SB600_SPI;

-	/* Enable SPI ROM in SB600 PM register. */
-	/* If we enable SPI ROM here, we have to disable it after we leave.
+	/* Force enable SPI ROM in SB600 PM register.
+	 * If we enable SPI ROM here, we have to disable it after we leave.
     * But how can we know which ROM we are going to handle? So we have
     * to trade off. We only access LPC ROM if we boot via LPC ROM. And
-	 * only SPI ROM if we boot via SPI ROM. If you want to do it crossly,
-	 * you have to use the code below.
+	 * only SPI ROM if we boot via SPI ROM. If you want to access SPI on
+	 * boards with LPC straps, you have to use the code below.
     */
    /*
    OUTB(0x8f, 0xcd6);

[PATCH] flashrom: Rewrite SB600 chipset enable function

[Peter Stuge]

Carl-Daniel Hailfinger wrote:
> prot = pci_read_long(dev, reg);
> if (prot & 0x3)
> 	printf("SB600 %s %s protected from %u to %u\n",
> 		(prot & 0x1) ? "write" : "",
> 		(prot & 0x2) ? "read" : "",
> 		(prot & 0xfffffc00),
> 		(prot & 0xfffffc00) + ((prot & 0x3ff) << 8));
> prot &= 0xfffffffc;
> pci_write_byte(dev, reg, prot);

I'd like to avoid writing at all if the region is unprotected. Please
just if (0 == (prot & 0x3)) continue; at the top. Maybe add a message
saying that the region is unprotected.

And I think these should be printf_debug() or such.
> 	printf("SB600 still %s %s protected from %u to %u\n",
> 		(prot & 0x1) ? "write" : "",
> 		(prot & 0x2) ? "read" : "",

Output looks a little nicer if the space is moved down to the
trigraphs.

..still%s%s protected ..
" write" : "",
" read" : ""

Fix this and it's

Acked-by: Peter Stuge

[PATCH] flashrom: Rewrite SB600 chipset enable function

[Carl-Daniel Hailfinger]

On 05.05.2009 18:17, Peter Stuge wrote:
> Carl-Daniel Hailfinger wrote:
> > prot = pci_read_long(dev, reg);
> > if (prot & 0x3)
> > 	printf("SB600 %s %s protected from %u to %u\n",
> > 		(prot & 0x1) ? "write" : "",
> > 		(prot & 0x2) ? "read" : "",
> > 		(prot & 0xfffffc00),
> > 		(prot & 0xfffffc00) + ((prot & 0x3ff) << 8));
> > prot &= 0xfffffffc;
> > pci_write_byte(dev, reg, prot);
>
> I'd like to avoid writing at all if the region is unprotected. Please
> just if (0 == (prot & 0x3)) continue; at the top.

Done.
> Maybe add a message
> saying that the region is unprotected.

An empty region with zero address and no protection flags probably does
not need printing IMHO.
> And I think these should be printf_debug() or such.

I made the first printf printf_debug and the second one (failure)
unconditional printf. Not telling the user about a failed unprotect is bad.
> > 	printf("SB600 still %s %s protected from %u to %u\n",
> > 		(prot & 0x1) ? "write" : "",
> > 		(prot & 0x2) ? "read" : "",
>
> Output looks a little nicer if the space is moved down to the
> trigraphs.
>
> ..still%s%s protected ..
> " write" : "",
> " read" : ""
>
> Fix this and it's

Done.
> Acked-by: Peter Stuge

Thanks, committed in r463.

Regards,
Carl-Daniel