[PATCH mips/linux.git 0/5] firmware: bcm47xx_nvram: refactor finding & reading NVRAM

[PATCH mips/linux.git 0/5] firmware: bcm47xx_nvram: refactor finding & reading NVRAM

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This patchset refactors driver part finding and reading NVRAM.

It been tested on BCM4706. Updated code checks the same offsets as
before. Driver still finds & copies NVRAM content.

It's a new patchset replacing previous single-patch attempt:
[PATCH V2 mips/linux.git] firmware: bcm47xx_nvram: refactor finding & reading NVRAM

Rafał Miłecki (5):
  firmware: bcm47xx_nvram: rename finding function and its variables
  firmware: bcm47xx_nvram: add helper checking for NVRAM
  firmware: bcm47xx_nvram: extract code copying NVRAM
  firmware: bcm47xx_nvram: look for NVRAM with for instead of while
  firmware: bcm47xx_nvram: inline code checking NVRAM size

 drivers/firmware/broadcom/bcm47xx_nvram.c | 92 ++++++++++++-----------
 1 file changed, 47 insertions(+), 45 deletions(-)

-- 
2.26.2

[PATCH mips/linux.git 1/5] firmware: bcm47xx_nvram: rename finding function and its variables

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

1. Use "bcm47xx_" function name prefix for consistency
2. It takes flash start as argument so s/iobase/flash_start/
3. "off" was used for finding flash end so just call it "flash_size"

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/firmware/broadcom/bcm47xx_nvram.c | 24 ++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c
index 835ece9c00f1..b04007adc79f 100644
--- a/drivers/firmware/broadcom/bcm47xx_nvram.c
+++ b/drivers/firmware/broadcom/bcm47xx_nvram.c
@@ -48,11 +48,13 @@ static u32 find_nvram_size(void __iomem *end)
 	return 0;
 }
 
-/* Probe for NVRAM header */
-static int nvram_find_and_copy(void __iomem *iobase, u32 lim)
+/**
+ * bcm47xx_nvram_find_and_copy - find NVRAM on flash mapping & copy it
+ */
+static int bcm47xx_nvram_find_and_copy(void __iomem *flash_start, size_t res_size)
 {
 	struct nvram_header __iomem *header;
-	u32 off;
+	size_t flash_size;
 	u32 size;
 
 	if (nvram_len) {
@@ -61,25 +63,25 @@ static int nvram_find_and_copy(void __iomem *iobase, u32 lim)
 	}
 
 	/* TODO: when nvram is on nand flash check for bad blocks first. */
-	off = FLASH_MIN;
-	while (off <= lim) {
+	flash_size = FLASH_MIN;
+	while (flash_size <= res_size) {
 		/* Windowed flash access */
-		size = find_nvram_size(iobase + off);
+		size = find_nvram_size(flash_start + flash_size);
 		if (size) {
-			header = (struct nvram_header *)(iobase + off - size);
+			header = (struct nvram_header *)(flash_start + flash_size - size);
 			goto found;
 		}
-		off <<= 1;
+		flash_size <<= 1;
 	}
 
 	/* Try embedded NVRAM at 4 KB and 1 KB as last resorts */
-	header = (struct nvram_header *)(iobase + 4096);
+	header = (struct nvram_header *)(flash_start + 4096);
 	if (header->magic == NVRAM_MAGIC) {
 		size = NVRAM_SPACE;
 		goto found;
 	}
 
-	header = (struct nvram_header *)(iobase + 1024);
+	header = (struct nvram_header *)(flash_start + 1024);
 	if (header->magic == NVRAM_MAGIC) {
 		size = NVRAM_SPACE;
 		goto found;
@@ -124,7 +126,7 @@ int bcm47xx_nvram_init_from_mem(u32 base, u32 lim)
 	if (!iobase)
 		return -ENOMEM;
 
-	err = nvram_find_and_copy(iobase, lim);
+	err = bcm47xx_nvram_find_and_copy(iobase, lim);
 
 	iounmap(iobase);
 
-- 
2.26.2

[PATCH mips/linux.git 2/5] firmware: bcm47xx_nvram: add helper checking for NVRAM

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This avoids duplicating code doing casting and checking for NVRAM magic.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/firmware/broadcom/bcm47xx_nvram.c | 30 ++++++++++++++---------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c
index b04007adc79f..99f3ec180be6 100644
--- a/drivers/firmware/broadcom/bcm47xx_nvram.c
+++ b/drivers/firmware/broadcom/bcm47xx_nvram.c
@@ -34,14 +34,20 @@ static char nvram_buf[NVRAM_SPACE];
 static size_t nvram_len;
 static const u32 nvram_sizes[] = {0x6000, 0x8000, 0xF000, 0x10000};
 
+/**
+ * bcm47xx_nvram_is_valid - check for a valid NVRAM at specified memory
+ */
+static bool bcm47xx_nvram_is_valid(void __iomem *nvram)
+{
+	return ((struct nvram_header *)nvram)->magic == NVRAM_MAGIC;
+}
+
 static u32 find_nvram_size(void __iomem *end)
 {
-	struct nvram_header __iomem *header;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(nvram_sizes); i++) {
-		header = (struct nvram_header *)(end - nvram_sizes[i]);
-		if (header->magic == NVRAM_MAGIC)
+		if (bcm47xx_nvram_is_valid(end - nvram_sizes[i]))
 			return nvram_sizes[i];
 	}
 
@@ -55,6 +61,7 @@ static int bcm47xx_nvram_find_and_copy(void __iomem *flash_start, size_t res_siz
 {
 	struct nvram_header __iomem *header;
 	size_t flash_size;
+	size_t offset;
 	u32 size;
 
 	if (nvram_len) {
@@ -68,31 +75,30 @@ static int bcm47xx_nvram_find_and_copy(void __iomem *flash_start, size_t res_siz
 		/* Windowed flash access */
 		size = find_nvram_size(flash_start + flash_size);
 		if (size) {
-			header = (struct nvram_header *)(flash_start + flash_size - size);
+			offset = flash_size - size;
 			goto found;
 		}
 		flash_size <<= 1;
 	}
 
 	/* Try embedded NVRAM at 4 KB and 1 KB as last resorts */
-	header = (struct nvram_header *)(flash_start + 4096);
-	if (header->magic == NVRAM_MAGIC) {
-		size = NVRAM_SPACE;
+
+	offset = 4096;
+	if (bcm47xx_nvram_is_valid(flash_start + offset))
 		goto found;
-	}
 
-	header = (struct nvram_header *)(flash_start + 1024);
-	if (header->magic == NVRAM_MAGIC) {
-		size = NVRAM_SPACE;
+	offset = 1024;
+	if (bcm47xx_nvram_is_valid(flash_start + offset))
 		goto found;
-	}
 
 	pr_err("no nvram found\n");
 	return -ENXIO;
 
 found:
+	header = (struct nvram_header *)(flash_start + offset);
 	__ioread32_copy(nvram_buf, header, sizeof(*header) / 4);
 	nvram_len = ((struct nvram_header *)(nvram_buf))->len;
+	size = res_size - offset;
 	if (nvram_len > size) {
 		pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n");
 		nvram_len = size;
-- 
2.26.2

[PATCH mips/linux.git 3/5] firmware: bcm47xx_nvram: extract code copying NVRAM

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This simplifies function finding NVRAM. It doesn't directly deal with
NVRAM structure anymore and is a bit smaller.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/firmware/broadcom/bcm47xx_nvram.c | 43 +++++++++++++----------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c
index 99f3ec180be6..09f51b95849e 100644
--- a/drivers/firmware/broadcom/bcm47xx_nvram.c
+++ b/drivers/firmware/broadcom/bcm47xx_nvram.c
@@ -54,12 +54,35 @@ static u32 find_nvram_size(void __iomem *end)
 	return 0;
 }
 
+/**
+ * bcm47xx_nvram_copy - copy NVRAM to internal buffer
+ */
+static void bcm47xx_nvram_copy(void __iomem *nvram_start, size_t res_size)
+{
+	struct nvram_header __iomem *header = nvram_start;
+	size_t copy_size;
+
+	copy_size = header->len;
+	if (copy_size > res_size) {
+		pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n");
+		copy_size = res_size;
+	}
+	if (copy_size >= NVRAM_SPACE) {
+		pr_err("nvram on flash (%zu bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
+		       copy_size, NVRAM_SPACE - 1);
+		copy_size = NVRAM_SPACE - 1;
+	}
+
+	__ioread32_copy(nvram_buf, nvram_start, DIV_ROUND_UP(copy_size, 4));
+	nvram_buf[NVRAM_SPACE - 1] = '\0';
+	nvram_len = copy_size;
+}
+
 /**
  * bcm47xx_nvram_find_and_copy - find NVRAM on flash mapping & copy it
  */
 static int bcm47xx_nvram_find_and_copy(void __iomem *flash_start, size_t res_size)
 {
-	struct nvram_header __iomem *header;
 	size_t flash_size;
 	size_t offset;
 	u32 size;
@@ -95,23 +118,7 @@ static int bcm47xx_nvram_find_and_copy(void __iomem *flash_start, size_t res_siz
 	return -ENXIO;
 
 found:
-	header = (struct nvram_header *)(flash_start + offset);
-	__ioread32_copy(nvram_buf, header, sizeof(*header) / 4);
-	nvram_len = ((struct nvram_header *)(nvram_buf))->len;
-	size = res_size - offset;
-	if (nvram_len > size) {
-		pr_err("The nvram size according to the header seems to be bigger than the partition on flash\n");
-		nvram_len = size;
-	}
-	if (nvram_len >= NVRAM_SPACE) {
-		pr_err("nvram on flash (%zu bytes) is bigger than the reserved space in memory, will just copy the first %i bytes\n",
-		       nvram_len, NVRAM_SPACE - 1);
-		nvram_len = NVRAM_SPACE - 1;
-	}
-	/* proceed reading data after header */
-	__ioread32_copy(nvram_buf + sizeof(*header), header + 1,
-			DIV_ROUND_UP(nvram_len, 4));
-	nvram_buf[NVRAM_SPACE - 1] = '\0';
+	bcm47xx_nvram_copy(flash_start + offset, res_size - offset);
 
 	return 0;
 }
-- 
2.26.2

[PATCH mips/linux.git 4/5] firmware: bcm47xx_nvram: look for NVRAM with for instead of while

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This loop requires variable initialization, stop condition and post
iteration increment. It's pretty much a for loop definition.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/firmware/broadcom/bcm47xx_nvram.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c
index 09f51b95849e..1d2271b1e07a 100644
--- a/drivers/firmware/broadcom/bcm47xx_nvram.c
+++ b/drivers/firmware/broadcom/bcm47xx_nvram.c
@@ -93,15 +93,13 @@ static int bcm47xx_nvram_find_and_copy(void __iomem *flash_start, size_t res_siz
 	}
 
 	/* TODO: when nvram is on nand flash check for bad blocks first. */
-	flash_size = FLASH_MIN;
-	while (flash_size <= res_size) {
+	for (flash_size = FLASH_MIN; flash_size <= res_size; flash_size <<= 1) {
 		/* Windowed flash access */
 		size = find_nvram_size(flash_start + flash_size);
 		if (size) {
 			offset = flash_size - size;
 			goto found;
 		}
-		flash_size <<= 1;
 	}
 
 	/* Try embedded NVRAM at 4 KB and 1 KB as last resorts */
-- 
2.26.2

[PATCH mips/linux.git 5/5] firmware: bcm47xx_nvram: inline code checking NVRAM size

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Separated function was not improving code quality much (or at all).
Moreover it expected possible flash end address as argument and it was
returning NVRAM size.

The new code always operates on offsets which means less logic and less
calculations.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/firmware/broadcom/bcm47xx_nvram.c | 25 +++++++----------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c
index 1d2271b1e07a..bd235833b687 100644
--- a/drivers/firmware/broadcom/bcm47xx_nvram.c
+++ b/drivers/firmware/broadcom/bcm47xx_nvram.c
@@ -42,18 +42,6 @@ static bool bcm47xx_nvram_is_valid(void __iomem *nvram)
 	return ((struct nvram_header *)nvram)->magic == NVRAM_MAGIC;
 }
 
-static u32 find_nvram_size(void __iomem *end)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(nvram_sizes); i++) {
-		if (bcm47xx_nvram_is_valid(end - nvram_sizes[i]))
-			return nvram_sizes[i];
-	}
-
-	return 0;
-}
-
 /**
  * bcm47xx_nvram_copy - copy NVRAM to internal buffer
  */
@@ -85,7 +73,7 @@ static int bcm47xx_nvram_find_and_copy(void __iomem *flash_start, size_t res_siz
 {
 	size_t flash_size;
 	size_t offset;
-	u32 size;
+	int i;
 
 	if (nvram_len) {
 		pr_warn("nvram already initialized\n");
@@ -93,12 +81,13 @@ static int bcm47xx_nvram_find_and_copy(void __iomem *flash_start, size_t res_siz
 	}
 
 	/* TODO: when nvram is on nand flash check for bad blocks first. */
+
+	/* Try every possible flash size and check for NVRAM at its end */
 	for (flash_size = FLASH_MIN; flash_size <= res_size; flash_size <<= 1) {
-		/* Windowed flash access */
-		size = find_nvram_size(flash_start + flash_size);
-		if (size) {
-			offset = flash_size - size;
-			goto found;
+		for (i = 0; i < ARRAY_SIZE(nvram_sizes); i++) {
+			offset = flash_size - nvram_sizes[i];
+			if (bcm47xx_nvram_is_valid(flash_start + offset))
+				goto found;
 		}
 	}
 
-- 
2.26.2

Re: [PATCH mips/linux.git 0/5] firmware: bcm47xx_nvram: refactor finding & reading NVRAM

[Thomas Bogendoerfer]

On Mon, Mar 08, 2021 at 10:03:15AM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This patchset refactors driver part finding and reading NVRAM.
> 
> It been tested on BCM4706. Updated code checks the same offsets as
> before. Driver still finds & copies NVRAM content.
> 
> It's a new patchset replacing previous single-patch attempt:
> [PATCH V2 mips/linux.git] firmware: bcm47xx_nvram: refactor finding & reading NVRAM
> 
> Rafał Miłecki (5):
>   firmware: bcm47xx_nvram: rename finding function and its variables
>   firmware: bcm47xx_nvram: add helper checking for NVRAM
>   firmware: bcm47xx_nvram: extract code copying NVRAM
>   firmware: bcm47xx_nvram: look for NVRAM with for instead of while
>   firmware: bcm47xx_nvram: inline code checking NVRAM size
> 
>  drivers/firmware/broadcom/bcm47xx_nvram.c | 92 ++++++++++++-----------
>  1 file changed, 47 insertions(+), 45 deletions(-)

series applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]