Nouveau - Mar 2010 - [PATCH] drm/nouveau: fix vbios load and check functions on PowerPC

From: Andrea Tacconi <tacconet at libero.it>

Subject: [PATCH] drm/nouveau: fix vbios load and check functions on PowerPC

On PowerPC the Bios signature reports a wrong lenght of video rom.
Fix this by reading the correct size from Open Firmware.

Set Pramin as primary vbios searching method, because it's the only working
method on PowerPC.

The nv_cksum function always fails.
Fix this by Calculating and adding checksum byte at the end of vbios.

Signed-off-by: Andrea Tacconi <tacconet at libero.it>
---
diff --git a/drivers/gpu/drm/nouveau/nouveau_bios.c
b/drivers/gpu/drm/nouveau/nouveau_bios.c
index 6b6c303..c234b45 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bios.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bios.c
@@ -69,12 +69,29 @@ static bool nv_cksum(const uint8_t *data, unsigned int
length)
 static int
 score_vbios(struct drm_device *dev, const uint8_t *data, const bool writeable)
 {
+	int bios_size = 0;
+#if defined(__powerpc__)
+	struct device_node *dn = NULL;
+#endif
+
 	if (!(data[0] == 0x55 && data[1] == 0xAA)) {
 		NV_TRACEWARN(dev, "... BIOS signature not found\n");
 		return 0;
 	}
+#if defined(__powerpc__)
+	/*
+	 * The Bios signature reports a wrong lenght of rom.
+	 * The correct size is read from Open Firmware.
+	 */
+	dn = pci_device_to_OF_node(dev->pdev);
+	of_find_property(dn, "NVDA,BMP", &bios_size);
+	/* added checksum byte */
+	bios_size++;
+#else
+	bios_size = (data[2] * 512);
+#endif
 
-	if (nv_cksum(data, data[2] * 512)) {
+	if (nv_cksum(data, bios_size)) {
 		NV_TRACEWARN(dev, "... BIOS checksum invalid\n");
 		/* if a ro image is somewhat bad, it's probably all rubbish */
 		return writeable ? 2 : 1;
@@ -183,11 +200,22 @@ struct methods {
 	const bool rw;
 };
 
+#if defined(__powerpc__)
+	/*
+	 * For now PRAMIN is the only working method on PowerPC
+	 */
+static struct methods nv04_methods[] = {
+	{ "PRAMIN", load_vbios_pramin, true },
+	{ "PROM", load_vbios_prom, false },
+	{ "PCIROM", load_vbios_pci, true },
+};
+#else
 static struct methods nv04_methods[] = {
 	{ "PROM", load_vbios_prom, false },
 	{ "PRAMIN", load_vbios_pramin, true },
 	{ "PCIROM", load_vbios_pci, true },
 };
+#endif
 
 static struct methods nv50_methods[] = {
 	{ "PRAMIN", load_vbios_pramin, true },
diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c
b/drivers/gpu/drm/nouveau/nouveau_state.c
index 58b4680..35d35cc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_state.c
+++ b/drivers/gpu/drm/nouveau/nouveau_state.c
@@ -607,19 +607,49 @@ int nouveau_firstopen(struct drm_device *dev)
 static void nouveau_OF_copy_vbios_to_ramin(struct drm_device *dev)
 {
 #if defined(__powerpc__)
-	int size, i;
-	const uint32_t *bios;
+	/*
+	 * Copy BMP Bios to RAMIN, calculate its checksum and append it to Bios.
+	 */
+	int size, i, j, unread_bytes, size_int;
+	uint8_t sum = 0;
+	uint8_t checksum = 0;
+	uint32_t last_bytes = 0;
+	const uint32_t *bios = NULL;
 	struct device_node *dn = pci_device_to_OF_node(dev->pdev);
-	if (!dn) {
-		NV_INFO(dev, "Unable to get the OF node\n");
-		return;
-	}
 
+	size_int = sizeof(uint32_t);
 	bios = of_get_property(dn, "NVDA,BMP", &size);
+	/* write bios data and sum all bytes */
 	if (bios) {
-		for (i = 0; i < size; i += 4)
-			nv_wi32(dev, i, bios[i/4]);
-		NV_INFO(dev, "OF bios successfully copied (%d bytes)\n", size);
+		for (j = 0, i = 0; j < (size / size_int); j++, i += 4) {
+			nv_wi32(dev, i, bios[j]);
+			sum += (bios[j] & 0xFF000000) >> 24;
+			sum += (bios[j] & 0xFF0000) >> 16;
+			sum += (bios[j] & 0xFF00) >> 8;
+			sum += (bios[j] & 0xFF);
+		}
+		unread_bytes = size % size_int;
+		switch (unread_bytes) {
+		case 0:
+			/* all bytes were read, nothing to do */
+			break;
+		case 3:
+			sum += (bios[j] & 0xFF00) >> 8;
+			last_bytes |= (bios[j] & 0xFF00);
+		case 2:
+			sum += (bios[j] & 0xFF0000) >> 16;
+			last_bytes |= (bios[j] & 0xFF0000);
+		case 1:
+			sum += (bios[j] & 0xFF000000) >> 24;
+			last_bytes |= (bios[j] & 0xFF000000);
+			break;
+		}
+		/* the checksum is the two's complement of the sum */
+		checksum = ~sum + 1;
+		/* add checksum (1 byte) in the correct position */
+		last_bytes |= (checksum << (24 - unread_bytes * 8));
+		nv_wi32(dev, i, last_bytes);
+		NV_INFO(dev, "OF bios successfully copied (%d bytes) checksum
0x%x\n", size, checksum);
 	} else {
 		NV_INFO(dev, "Unable to get the OF bios\n");
 	}

On Tue, Mar 23, 2010 at 06:09:07PM +0100, tacconet at libero.it
wrote:
> From: Andrea Tacconi <tacconet at libero.it>
> 
> Subject: [PATCH] drm/nouveau: fix vbios load and check functions on PowerPC
> 
> On PowerPC the Bios signature reports a wrong lenght of video rom.
> Fix this by reading the correct size from Open Firmware.
> 
> Set Pramin as primary vbios searching method, because it's the only
working method on PowerPC.
> 
> The nv_cksum function always fails.
> Fix this by Calculating and adding checksum byte at the end of vbios.
> 
> Signed-off-by: Andrea Tacconi <tacconet at libero.it>
> ---
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bios.c
b/drivers/gpu/drm/nouveau/nouveau_bios.c
> index 6b6c303..c234b45 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bios.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bios.c
> @@ -69,12 +69,29 @@ static bool nv_cksum(const uint8_t *data, unsigned int
length)
>  static int
>  score_vbios(struct drm_device *dev, const uint8_t *data, const bool
writeable)
>  {
> +	int bios_size = 0;
> +#if defined(__powerpc__)
> +	struct device_node *dn = NULL;
> +#endif
> +
>  	if (!(data[0] == 0x55 && data[1] == 0xAA)) {
>  		NV_TRACEWARN(dev, "... BIOS signature not found\n");
>  		return 0;
>  	}
> +#if defined(__powerpc__)
> +	/*
> +	 * The Bios signature reports a wrong lenght of rom.
> +	 * The correct size is read from Open Firmware.
> +	 */
> +	dn = pci_device_to_OF_node(dev->pdev);
> +	of_find_property(dn, "NVDA,BMP", &bios_size);
> +	/* added checksum byte */
> +	bios_size++;
> +#else
> +	bios_size = (data[2] * 512);
> +#endif
>  
> -	if (nv_cksum(data, data[2] * 512)) {
> +	if (nv_cksum(data, bios_size)) {
how about factoring bios_size calculation to another function?

#if defined(__powerpc__)
int fun()
{
}
#else
int fun()
{
}
#endif
>  		NV_TRACEWARN(dev, "... BIOS checksum invalid\n");
>  		/* if a ro image is somewhat bad, it's probably all rubbish */
>  		return writeable ? 2 : 1;
> @@ -183,11 +200,22 @@ struct methods {
>  	const bool rw;
>  };
>  
> +#if defined(__powerpc__)
> +	/*
> +	 * For now PRAMIN is the only working method on PowerPC
> +	 */
> +static struct methods nv04_methods[] = {
> +	{ "PRAMIN", load_vbios_pramin, true },
> +	{ "PROM", load_vbios_prom, false },
> +	{ "PCIROM", load_vbios_pci, true },
> +};
> +#else
>  static struct methods nv04_methods[] = {
>  	{ "PROM", load_vbios_prom, false },
>  	{ "PRAMIN", load_vbios_pramin, true },
>  	{ "PCIROM", load_vbios_pci, true },
>  };
> +#endif
it pramin is the only method why do you need to redefine this array with
different order?
doesn't it fallback to next method when earlier fails?
>  
>  static struct methods nv50_methods[] = {
>  	{ "PRAMIN", load_vbios_pramin, true },
> diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c
b/drivers/gpu/drm/nouveau/nouveau_state.c
> index 58b4680..35d35cc 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_state.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_state.c
> @@ -607,19 +607,49 @@ int nouveau_firstopen(struct drm_device *dev)
>  static void nouveau_OF_copy_vbios_to_ramin(struct drm_device *dev)
>  {
>  #if defined(__powerpc__)
> -	int size, i;
> -	const uint32_t *bios;
> +	/*
> +	 * Copy BMP Bios to RAMIN, calculate its checksum and append it to Bios.
> +	 */
> +	int size, i, j, unread_bytes, size_int;
> +	uint8_t sum = 0;
> +	uint8_t checksum = 0;
> +	uint32_t last_bytes = 0;
> +	const uint32_t *bios = NULL;
>  	struct device_node *dn = pci_device_to_OF_node(dev->pdev);
> -	if (!dn) {
> -		NV_INFO(dev, "Unable to get the OF node\n");
> -		return;
> -	}
>  
> +	size_int = sizeof(uint32_t);
>  	bios = of_get_property(dn, "NVDA,BMP", &size);
> +	/* write bios data and sum all bytes */
>  	if (bios) {
> -		for (i = 0; i < size; i += 4)
> -			nv_wi32(dev, i, bios[i/4]);
> -		NV_INFO(dev, "OF bios successfully copied (%d bytes)\n",
size);
> +		for (j = 0, i = 0; j < (size / size_int); j++, i += 4) {
you are using uint32_t, nv_wi32, incrementing by 4, so why do you need size_int?
> +			nv_wi32(dev, i, bios[j]);
> +			sum += (bios[j] & 0xFF000000) >> 24;
> +			sum += (bios[j] & 0xFF0000) >> 16;
> +			sum += (bios[j] & 0xFF00) >> 8;
> +			sum += (bios[j] & 0xFF);
> +		}
> +		unread_bytes = size % size_int;
unwritten_bytes?
> +		switch (unread_bytes) {
> +		case 0:
> +			/* all bytes were read, nothing to do */
> +			break;
> +		case 3:
> +			sum += (bios[j] & 0xFF00) >> 8;
> +			last_bytes |= (bios[j] & 0xFF00);
> +		case 2:
> +			sum += (bios[j] & 0xFF0000) >> 16;
> +			last_bytes |= (bios[j] & 0xFF0000);
> +		case 1:
> +			sum += (bios[j] & 0xFF000000) >> 24;
> +			last_bytes |= (bios[j] & 0xFF000000);
> +			break;
> +		}
comment about "fall through" to the next case would make this code
easier to read.
you could move case 0 as the last one
> +		/* the checksum is the two's complement of the sum */
> +		checksum = ~sum + 1;
> +		/* add checksum (1 byte) in the correct position */
> +		last_bytes |= (checksum << (24 - unread_bytes * 8));
> +		nv_wi32(dev, i, last_bytes);
> +		NV_INFO(dev, "OF bios successfully copied (%d bytes) checksum
0x%x\n", size, checksum);
>  	} else {
>  		NV_INFO(dev, "Unable to get the OF bios\n");
>  	}