[PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[Simon Arlott]

Broadcom BCM963xx boards have multiple nvram variants across different
SoCs with additional checksum fields added whenever the size of the
nvram was extended.

Add this structure as a header file so that multiple drivers and userspace
can use it.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
v3: Fix includes/type names, add comments explaining the nvram struct.

v2: Use external struct bcm963xx_nvram definition for bcm963268part.

 MAINTAINERS                         |  1 +
 include/uapi/linux/bcm963xx_nvram.h | 53 +++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)
 create mode 100644 include/uapi/linux/bcm963xx_nvram.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6b6d4e2e..abf18b4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2393,6 +2393,7 @@ F:	drivers/irqchip/irq-bcm63*
 F:	drivers/irqchip/irq-bcm7*
 F:	drivers/irqchip/irq-brcmstb*
 F:	include/linux/bcm63xx_wdt.h
+F:	include/uapi/linux/bcm963xx_nvram.h
 
 BROADCOM TG3 GIGABIT ETHERNET DRIVER
 M:	Prashant Sreedharan <prashant@broadcom.com>
diff --git a/include/uapi/linux/bcm963xx_nvram.h b/include/uapi/linux/bcm963xx_nvram.h
new file mode 100644
index 0000000..2dcb307
--- /dev/null
+++ b/include/uapi/linux/bcm963xx_nvram.h
@@ -0,0 +1,53 @@
+#ifndef _UAPI__LINUX_BCM963XX_NVRAM_H__
+#define _UAPI__LINUX_BCM963XX_NVRAM_H__
+
+#include <linux/types.h>
+#include <linux/if_ether.h>
+
+/*
+ * Broadcom BCM963xx SoC board nvram data structure.
+ *
+ * The nvram structure varies in size depending on the SoC board version. Use
+ * the appropriate minimum BCM963XX_NVRAM_*_SIZE define for the information
+ * you need instead of sizeof(struct bcm963xx_nvram) as this may change.
+ *
+ * The "version" field value maps directly to the size and checksum names, e.g.
+ * version 4 uses "checksum_v4" and the data is BCM963XX_NVRAM_V4_SIZE bytes.
+ *
+ * Do not use the __reserved fields, especially not as an offset for CRC
+ * calculations (use BCM963XX_NVRAM_*_SIZE instead). These may be removed or
+ * repositioned.
+ */
+
+#define BCM963XX_NVRAM_V4_SIZE		300
+#define BCM963XX_NVRAM_V5_SIZE		1024
+#define BCM963XX_NVRAM_V6_SIZE		BCM963XX_NVRAM_V5_SIZE
+#define BCM963XX_NVRAM_V7_SIZE		3072
+
+#define BCM963XX_NVRAM_NR_PARTS		5
+
+struct bcm963xx_nvram {
+	__u32	version;
+	char	bootline[256];
+	char	name[16];
+	__u32	main_tp_number;
+	__u32	psi_size;
+	__u32	mac_addr_count;
+	__u8	mac_addr_base[ETH_ALEN];
+	__u8	__reserved1[2];
+	__u32	checksum_v4;
+
+	__u8	__reserved2[292];
+	__u32	nand_part_offset[BCM963XX_NVRAM_NR_PARTS];
+	__u32	nand_part_size[BCM963XX_NVRAM_NR_PARTS];
+	__u8	__reserved3[388];
+	union {
+		__u32	checksum_v5;
+		__u32	checksum_v6;
+	};
+
+	__u8	__reserved4[2044];
+	__u32	checksum_v7;
+} __packed;
+
+#endif /* _UAPI__LINUX_BCM963XX_NVRAM_H__ */
-- 
2.1.4

-- 
Simon Arlott

[PATCH linux-next (v3) 2/3] MIPS: bcm63xx: nvram: Use nvram structure definition from header file

[Simon Arlott]

Use the common definition of the nvram structure from the header file
include/uapi/linux/bcm963xx_nvram.h instead of maintaining a separate
copy.

Read the version 5 size of nvram data from memory and then support the
version 4 and version 5 variants of nvram data checksums.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
Compile tested only (there is no support for brcmnand on mach-bcm63xx).

v3: No changes (reworded commit message).

v2: Use external struct bcm963xx_nvram definition for bcm963268part.

 arch/mips/bcm63xx/nvram.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/arch/mips/bcm63xx/nvram.c b/arch/mips/bcm63xx/nvram.c
index 4b50d40..36e74f9 100644
--- a/arch/mips/bcm63xx/nvram.c
+++ b/arch/mips/bcm63xx/nvram.c
@@ -16,25 +16,9 @@
 #include <linux/kernel.h>
 #include <linux/if_ether.h>
 
+#include <uapi/linux/bcm963xx_nvram.h>
 #include <bcm63xx_nvram.h>
 
-/*
- * nvram structure
- */
-struct bcm963xx_nvram {
-	u32	version;
-	u8	reserved1[256];
-	u8	name[16];
-	u32	main_tp_number;
-	u32	psi_size;
-	u32	mac_addr_count;
-	u8	mac_addr_base[ETH_ALEN];
-	u8	reserved2[2];
-	u32	checksum_old;
-	u8	reserved3[720];
-	u32	checksum_high;
-};
-
 #define BCM63XX_DEFAULT_PSI_SIZE	64
 
 static struct bcm963xx_nvram nvram;
@@ -47,17 +31,17 @@ void __init bcm63xx_nvram_init(void *addr)
 	u8 hcs_mac_addr[ETH_ALEN] = { 0x00, 0x10, 0x18, 0xff, 0xff, 0xff };
 
 	/* extract nvram data */
-	memcpy(&nvram, addr, sizeof(nvram));
+	memcpy(&nvram, addr, BCM963XX_NVRAM_V5_SIZE);
 
 	/* check checksum before using data */
 	if (nvram.version <= 4) {
-		check_len = offsetof(struct bcm963xx_nvram, reserved3);
-		expected_crc = nvram.checksum_old;
-		nvram.checksum_old = 0;
+		check_len = BCM963XX_NVRAM_V4_SIZE;
+		expected_crc = nvram.checksum_v4;
+		nvram.checksum_v4 = 0;
 	} else {
-		check_len = sizeof(nvram);
-		expected_crc = nvram.checksum_high;
-		nvram.checksum_high = 0;
+		check_len = BCM963XX_NVRAM_V5_SIZE;
+		expected_crc = nvram.checksum_v5;
+		nvram.checksum_v5 = 0;
 	}
 
 	crc = crc32_le(~0, (u8 *)&nvram, check_len);
-- 
2.1.4

-- 
Simon Arlott

[PATCH linux-next (v3) 3/3] mtd: part: Add BCM962368 CFE partitioning support

[Simon Arlott]

Add partitioning support for BCM963268 boards with CFE bootloaders.
The following partitions are defined:
  "boot":           CFE and nvram data
  "rootfs":         Currently selected rootfs
  "data":           Configuration data
  "rootfs1_update": Container for the whole flash area used
                    for the first rootfs to allow it to be
                    updated.
  "rootfs2_update": Container for the whole flash area used
                    for the second rootfs to allow it to be
                    updated.
  "rootfs_other":   The other (not currently selected) rootfs

Example:
[    1.904302] nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
[    1.911000] nand: Macronix NAND 128MiB 3,3V 8-bit
[    1.915855] nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
[    1.923797] bcm6368_nand 10000200.nand: detected 128MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, Hamming ECC
[    1.936994] Bad block table found at page 65472, version 0x01
[    1.944121] Bad block table found at page 65408, version 0x01
[    1.951166] nand_read_bbt: bad block at 0x000007480000
[    1.969377] bcm963268part: rootfs1: CFE boot tag found at 0x20000 with version 6, board type 963168VX
[    1.980690] bcm963268part: rootfs2: CFE boot tag found at 0x4000000 with version 6, board type 963168VX
[    1.990801] bcm963268part: CFE bootline selected latest image rootfs1 (rootfs1_seq=2, rootfs2_seq=1)
[    2.022080] 6 bcm963268part partitions found on MTD device brcmnand.0
[    2.042659] Creating 6 MTD partitions on "brcmnand.0":
[    2.048025] 0x000000000000-0x000000020000 : "boot"
[    2.062134] 0x000000040000-0x000001120000 : "rootfs"
[    2.077632] 0x000007b00000-0x000007f00000 : "data"
[    2.091363] 0x000000020000-0x000003ac0000 : "rootfs1_update"
[    2.106228] 0x000004000000-0x000007ac0000 : "rootfs2_update"
[    2.121093] 0x000004020000-0x000005060000 : "rootfs_other"

The nvram contains the offset and size of the boot, rootfs1, rootfs2
and data partitions. The presence of CFE and nvram is verified by
reading from the boot partition which is assumed to be at offset 0
and the process aborts if the nvram read indicates that this is not
the case.

There is bcm_tag information at the start of each rootfs that is used
to determine which rootfs is newer and what its real offset/size is.

The CFE bootline is used to select a rootfs.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
v3: Use COMPILE_TEST.

    Ensure that strings read from flash are null terminated and validate
    bcm_tag integer values (this also moves reporting of rootfs sequence
    numbers to later on).

v2: Use external struct bcm963xx_nvram definition for bcm963268part.

    Removed support for the nand partition number field, it's not a
    standard Broadcom field (was added by MitraStar Technology Corp.).

 drivers/mtd/Kconfig         |  21 +++
 drivers/mtd/Makefile        |   1 +
 drivers/mtd/bcm963268part.c | 350 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 372 insertions(+)
 create mode 100644 drivers/mtd/bcm963268part.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 42cc953..8209730 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -148,6 +148,27 @@ config MTD_BCM63XX_PARTS
 	  This provides partions parsing for BCM63xx devices with CFE
 	  bootloaders.
 
+config MTD_BCM963268_PARTS
+	tristate "BCM963268 CFE partitioning support"
+	depends on BMIPS_GENERIC || COMPILE_TEST
+	select CRC32
+	help
+	  This provides partitions parsing for BCM963268 boards with CFE
+	  bootloaders. The following partitions are defined:
+	    "boot":           CFE and nvram data
+	    "rootfs":         Currently selected rootfs
+	    "data":           Configuration data
+	    "rootfs1_update": Container for the whole flash area used
+	                      for the first rootfs to allow it to be
+	                      updated.
+	    "rootfs2_update": Container for the whole flash area used
+	                      for the second rootfs to allow it to be
+	                      updated.
+	    "rootfs_other":   The other (not currently selected) rootfs
+
+	  A decision is made regarding which of the two rootfs is to be
+	  used based on the nvram data.
+
 config MTD_BCM47XX_PARTS
 	tristate "BCM47XX partitioning support"
 	depends on BCM47XX || ARCH_BCM_5301X
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 99bb9a1..f0f4140 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_MTD_CMDLINE_PARTS) += cmdlinepart.o
 obj-$(CONFIG_MTD_AFS_PARTS)	+= afs.o
 obj-$(CONFIG_MTD_AR7_PARTS)	+= ar7part.o
 obj-$(CONFIG_MTD_BCM63XX_PARTS)	+= bcm63xxpart.o
+obj-$(CONFIG_MTD_BCM963268_PARTS) += bcm963268part.o
 obj-$(CONFIG_MTD_BCM47XX_PARTS)	+= bcm47xxpart.o
 
 # 'Users' - code which presents functionality to userspace.
diff --git a/drivers/mtd/bcm963268part.c b/drivers/mtd/bcm963268part.c
new file mode 100644
index 0000000..2740a48
--- /dev/null
+++ b/drivers/mtd/bcm963268part.c
@@ -0,0 +1,350 @@
+/*
+ * BCM963268 CFE image tag parser
+ * Copyright 2015 Simon Arlott
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Derived from drivers/mtd/bcm63xxpart.c:
+ * Copyright © 2006-2008  Florian Fainelli <florian@openwrt.org>
+ *			  Mike Albon <malbon@openwrt.org>
+ * Copyright © 2009-2010  Daniel Dickinson <openwrt@cshore.neomailbox.net>
+ * Copyright © 2011-2013  Jonas Gorski <jonas.gorski@gmail.com>
+ *
+ * Derived from bcm963xx_4.12L.06B_consumer/bcmdrivers/opensource/char/board/bcm963xx/impl1/board.c,
+ *	bcm963xx_4.12L.06B_consumer/shared/opensource/include/bcm963xx/bcm_hwdefs.h:
+ * Copyright (c) 2002 Broadcom Corporation
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/crc32.h>
+#include <linux/if_ether.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/sizes.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+
+#include <asm/mach-bcm63xx/bcm963xx_tag.h>
+#include <uapi/linux/bcm963xx_nvram.h>
+
+/* Extended flash address, needs to be subtracted from bcm_tag flash offsets */
+#define BCM63XX_EXTENDED_SIZE		0xBFC00000
+
+#define BCM63XX_CFE_MAGIC_OFFSET	0x4e0
+#define BCM963XX_CFE_VERSION_OFFSET	0x570
+#define BCM963XX_NVRAM_OFFSET		0x580
+
+enum bcm963268_nvram_part {
+	PART_BOOT = 0,
+	PART_ROOTFS_1,
+	PART_ROOTFS_2,
+	PART_DATA,
+	PART_BBT,
+};
+
+/* Ensure strings read from flash structs are null terminated */
+#define STR_NULL_TERMINATE(x) \
+	do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0)
+
+static int bcm963268_detect_cfe(struct mtd_info *master)
+{
+	char buf[9];
+	int ret;
+	size_t retlen;
+
+	ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen,
+		       (void *)buf);
+	buf[retlen] = 0;
+
+	if (ret < 0)
+		return ret;
+
+	if (strncmp("cfe-v", buf, 5) == 0)
+		return 0;
+
+	return -EINVAL;
+}
+
+static int bcm963268_read_nvram(struct mtd_info *master,
+	struct bcm963xx_nvram *nvram)
+{
+	u32 crc, expected_crc;
+	size_t retlen;
+	int ret;
+
+	/* extract nvram data */
+	ret = mtd_read(master, BCM963XX_NVRAM_OFFSET, BCM963XX_NVRAM_V6_SIZE,
+			&retlen, (void *)nvram);
+
+	if (ret < 0)
+		return ret;
+
+	if (nvram->version < 6) {
+		pr_warn("nvram version %d not supported\n", nvram->version);
+		return -EINVAL;
+	}
+
+	/* check checksum before using data */
+	expected_crc = nvram->checksum_v6;
+	nvram->checksum_v6 = 0;
+
+	crc = crc32_le(~0, (u8 *)nvram, BCM963XX_NVRAM_V6_SIZE);
+
+	if (crc != expected_crc)
+		pr_warn("nvram checksum failed, contents may be invalid (expected %08x, got %08x)\n",
+			expected_crc, crc);
+
+	return 0;
+}
+
+static bool bcm963268_boot_latest(struct bcm963xx_nvram *nvram)
+{
+	char *p;
+
+	STR_NULL_TERMINATE(nvram->bootline);
+
+	/* Find previous image parameter "p" */
+	if (!strncmp(nvram->bootline, "p=", 2))
+		p = nvram->bootline;
+	else
+		p = strstr(nvram->bootline, " p=");
+
+	if (p == NULL)
+		return true;
+
+	p += 2;
+	if (*p == '\0')
+		return true;
+
+	return *p != '0';
+}
+
+static bool bcm963268_parse_rootfs_tag(struct mtd_info *master,
+	const char *name, loff_t rootfs_part, u64 *rootfs_offset,
+	u64 *rootfs_size, unsigned int *rootfs_sequence)
+{
+	struct bcm_tag *buf;
+	int ret;
+	size_t retlen;
+	u32 computed_crc;
+	bool rootfs_ok = false;
+
+	*rootfs_offset = 0;
+	*rootfs_size = 0;
+	*rootfs_sequence = 0;
+
+	buf = vmalloc(sizeof(struct bcm_tag));
+	if (!buf)
+		goto out;
+
+	ret = mtd_read(master, rootfs_part, sizeof(*buf), &retlen, (void *)buf);
+	if (ret < 0)
+		goto out;
+
+	if (retlen != sizeof(*buf))
+		goto out;
+
+	computed_crc = crc32_le(IMAGETAG_CRC_START, (u8 *)buf,
+				offsetof(struct bcm_tag, header_crc));
+	if (computed_crc == buf->header_crc) {
+		STR_NULL_TERMINATE(buf->board_id);
+		STR_NULL_TERMINATE(buf->tag_version);
+
+		pr_info("%s: CFE boot tag found at 0x%llx with version %s, board type %s\n",
+			name, rootfs_part, buf->tag_version, buf->board_id);
+
+		/* Get rootfs offset and size from tag data */
+		STR_NULL_TERMINATE(buf->flash_image_start);
+		if (kstrtou64(buf->flash_image_start, 10, rootfs_offset) ||
+				*rootfs_offset < BCM63XX_EXTENDED_SIZE) {
+			pr_err("%s: invalid rootfs offset: %*ph\n", name,
+				sizeof(buf->flash_image_start),
+				buf->flash_image_start);
+			goto out;
+		}
+
+		STR_NULL_TERMINATE(buf->root_length);
+		if (kstrtou64(buf->root_length, 10, rootfs_size) ||
+				rootfs_size == 0) {
+			pr_err("%s: invalid rootfs size: %*ph\n", name,
+				sizeof(buf->root_length), buf->root_length);
+			goto out;
+		}
+
+		/* Adjust for flash offset */
+		*rootfs_offset -= BCM63XX_EXTENDED_SIZE;
+
+		/* Remove bcm_tag data from length */
+		*rootfs_size -= *rootfs_offset - rootfs_part;
+
+		/* Get image sequence number to determine which one is newer */
+		STR_NULL_TERMINATE(buf->dual_image);
+		if (kstrtouint(buf->dual_image, 10, rootfs_sequence)) {
+			pr_err("%s: invalid rootfs sequence: %*ph\n", name,
+				sizeof(buf->dual_image), buf->dual_image);
+			goto out;
+		}
+
+		rootfs_ok = true;
+	} else {
+		pr_warn("%s: CFE boot tag at 0x%llx CRC invalid (expected %08x, actual %08x)\n",
+			name, rootfs_part, buf->header_crc, computed_crc);
+		goto out;
+	}
+
+out:
+	vfree(buf);
+	return rootfs_ok;
+}
+
+static int bcm963268_parse_cfe_partitions(struct mtd_info *master,
+	const struct mtd_partition **pparts, struct mtd_part_parser_data *data)
+{
+	int nrparts, curpart;
+	struct bcm963xx_nvram *nvram = NULL;
+	struct mtd_partition *parts;
+	u64 rootfs1_off, rootfs1_size;
+	unsigned int rootfs1_seq;
+	u64 rootfs2_off, rootfs2_size;
+	unsigned int rootfs2_seq;
+	bool rootfs1, rootfs2;
+	bool use_first;
+	int ret;
+
+	if (bcm963268_detect_cfe(master)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	nvram = vzalloc(sizeof(*nvram));
+	if (!nvram) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	if (bcm963268_read_nvram(master, nvram)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* We've just read the nvram from offset 0,
+	 * so it must be located there.
+	 */
+	if (nvram->nand_part_offset[PART_BOOT] != 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Get the rootfs partition locations */
+	rootfs1 = bcm963268_parse_rootfs_tag(master, "rootfs1",
+		nvram->nand_part_offset[PART_ROOTFS_1] * SZ_1K,
+		&rootfs1_off, &rootfs1_size, &rootfs1_seq);
+	rootfs2 = bcm963268_parse_rootfs_tag(master, "rootfs2",
+		nvram->nand_part_offset[PART_ROOTFS_2] * SZ_1K,
+		&rootfs2_off, &rootfs2_size, &rootfs2_seq);
+	if (!rootfs1 && !rootfs2) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Determine primary rootfs partition */
+	if (rootfs1 && rootfs2) {
+		bool use_latest = bcm963268_boot_latest(nvram);
+
+		/* Compare sequence numbers */
+		if (use_latest)
+			use_first = rootfs1_seq > rootfs2_seq;
+		else
+			use_first = rootfs1_seq < rootfs2_seq;
+
+		pr_info("CFE bootline selected %s image rootfs%u (rootfs1_seq=%u, rootfs2_seq=%u)\n",
+			use_latest ? "latest" : "previous",
+			use_first ? 1 : 2,
+			rootfs1_seq, rootfs2_seq);
+	} else {
+		use_first = rootfs1;
+	}
+
+	/* Partitions:
+	 * 1 boot
+	 * 2 rootfs
+	 * 3 data
+	 * 4 rootfs1_update
+	 * 5 rootfs2_update
+	 * 6 rootfs_other
+	 */
+	nrparts = 6;
+	curpart = 0;
+
+	parts = kcalloc(nrparts, sizeof(*parts), GFP_KERNEL);
+	if (!parts) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	parts[curpart].name = "boot";
+	parts[curpart].offset = nvram->nand_part_offset[PART_BOOT] * SZ_1K;
+	parts[curpart].size = nvram->nand_part_size[PART_BOOT] * SZ_1K;
+	curpart++;
+
+	parts[curpart].name = "rootfs";
+	parts[curpart].offset = use_first ? rootfs1_off : rootfs2_off;
+	parts[curpart].size = use_first ? rootfs1_size : rootfs2_size;
+	curpart++;
+
+	parts[curpart].name = "data";
+	parts[curpart].offset = nvram->nand_part_offset[PART_DATA] * SZ_1K;
+	parts[curpart].size = nvram->nand_part_size[PART_DATA] * SZ_1K;
+	curpart++;
+
+	/* Full rootfs partitions for updates */
+	parts[curpart].name = "rootfs1_update";
+	parts[curpart].offset = nvram->nand_part_offset[PART_ROOTFS_1] * SZ_1K;
+	parts[curpart].size = nvram->nand_part_size[PART_ROOTFS_1] * SZ_1K;
+	curpart++;
+
+	parts[curpart].name = "rootfs2_update";
+	parts[curpart].offset = nvram->nand_part_offset[PART_ROOTFS_2] * SZ_1K;
+	parts[curpart].size = nvram->nand_part_size[PART_ROOTFS_2] * SZ_1K;
+	curpart++;
+
+	/* Other rootfs if both are available */
+	if (rootfs1 && rootfs2) {
+		parts[curpart].name = "rootfs_other";
+		parts[curpart].offset = use_first ? rootfs2_off : rootfs1_off;
+		parts[curpart].size = use_first ? rootfs2_size : rootfs1_size;
+		curpart++;
+	}
+
+	*pparts = parts;
+	ret = 0;
+
+out:
+	vfree(nvram);
+
+	if (ret < 0)
+		return ret;
+
+	return nrparts;
+};
+
+static struct mtd_part_parser bcm963268_cfe_parser = {
+	.name = "bcm963268part",
+	.parse_fn = bcm963268_parse_cfe_partitions,
+};
+module_mtd_part_parser(bcm963268_cfe_parser);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Simon Arlott");
+MODULE_DESCRIPTION("MTD partitioning for BCM963268 CFE bootloaders");
-- 
2.1.4

-- 
Simon Arlott

Re: [PATCH linux-next (v3) 3/3] mtd: part: Add BCM962368 CFE partitioning support

[Jonas Gorski]

Hi,

On Fri, Dec 11, 2015 at 11:02 PM, Simon Arlott <simon@fire.lp0.eu> wrote:
> Add partitioning support for BCM963268 boards with CFE bootloaders.
> The following partitions are defined:
>   "boot":           CFE and nvram data
>   "rootfs":         Currently selected rootfs
>   "data":           Configuration data
>   "rootfs1_update": Container for the whole flash area used
>                     for the first rootfs to allow it to be
>                     updated.
>   "rootfs2_update": Container for the whole flash area used
>                     for the second rootfs to allow it to be
>                     updated.
>   "rootfs_other":   The other (not currently selected) rootfs

This isn't BCM963268 specific, this is BCM63XX NAND specific and used
since the first SoC with a NAND controller (BCM6368).

>
> Example:
> [    1.904302] nand: device found, Manufacturer ID: 0xc2, Chip ID: 0xf1
> [    1.911000] nand: Macronix NAND 128MiB 3,3V 8-bit
> [    1.915855] nand: 128 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
> [    1.923797] bcm6368_nand 10000200.nand: detected 128MiB total, 128KiB blocks, 2KiB pages, 16B OOB, 8-bit, Hamming ECC
> [    1.936994] Bad block table found at page 65472, version 0x01
> [    1.944121] Bad block table found at page 65408, version 0x01
> [    1.951166] nand_read_bbt: bad block at 0x000007480000
> [    1.969377] bcm963268part: rootfs1: CFE boot tag found at 0x20000 with version 6, board type 963168VX
> [    1.980690] bcm963268part: rootfs2: CFE boot tag found at 0x4000000 with version 6, board type 963168VX
> [    1.990801] bcm963268part: CFE bootline selected latest image rootfs1 (rootfs1_seq=2, rootfs2_seq=1)
> [    2.022080] 6 bcm963268part partitions found on MTD device brcmnand.0
> [    2.042659] Creating 6 MTD partitions on "brcmnand.0":
> [    2.048025] 0x000000000000-0x000000020000 : "boot"
> [    2.062134] 0x000000040000-0x000001120000 : "rootfs"
> [    2.077632] 0x000007b00000-0x000007f00000 : "data"
> [    2.091363] 0x000000020000-0x000003ac0000 : "rootfs1_update"
> [    2.106228] 0x000004000000-0x000007ac0000 : "rootfs2_update"
> [    2.121093] 0x000004020000-0x000005060000 : "rootfs_other"
>
> The nvram contains the offset and size of the boot, rootfs1, rootfs2
> and data partitions. The presence of CFE and nvram is verified by
> reading from the boot partition which is assumed to be at offset 0
> and the process aborts if the nvram read indicates that this is not
> the case.
>
> There is bcm_tag information at the start of each rootfs that is used
> to determine which rootfs is newer and what its real offset/size is.
>
> The CFE bootline is used to select a rootfs.
>
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> ---
> v3: Use COMPILE_TEST.
>
>     Ensure that strings read from flash are null terminated and validate
>     bcm_tag integer values (this also moves reporting of rootfs sequence
>     numbers to later on).
>
> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
>
>     Removed support for the nand partition number field, it's not a
>     standard Broadcom field (was added by MitraStar Technology Corp.).
>
>  drivers/mtd/Kconfig         |  21 +++
>  drivers/mtd/Makefile        |   1 +
>  drivers/mtd/bcm963268part.c | 350 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 372 insertions(+)
>  create mode 100644 drivers/mtd/bcm963268part.c
>
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 42cc953..8209730 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -148,6 +148,27 @@ config MTD_BCM63XX_PARTS
>           This provides partions parsing for BCM63xx devices with CFE
>           bootloaders.
>
> +config MTD_BCM963268_PARTS
> +       tristate "BCM963268 CFE partitioning support"
> +       depends on BMIPS_GENERIC || COMPILE_TEST

Since you include from arch/mips/include, this will fail to compile on
everything != MIPS

> +       select CRC32
> +       help
> +         This provides partitions parsing for BCM963268 boards with CFE
> +         bootloaders. The following partitions are defined:
> +           "boot":           CFE and nvram data
> +           "rootfs":         Currently selected rootfs
> +           "data":           Configuration data
> +           "rootfs1_update": Container for the whole flash area used
> +                             for the first rootfs to allow it to be
> +                             updated.
> +           "rootfs2_update": Container for the whole flash area used
> +                             for the second rootfs to allow it to be
> +                             updated.
> +           "rootfs_other":   The other (not currently selected) rootfs
> +
> +         A decision is made regarding which of the two rootfs is to be
> +         used based on the nvram data.
> +
>  config MTD_BCM47XX_PARTS
>         tristate "BCM47XX partitioning support"
>         depends on BCM47XX || ARCH_BCM_5301X
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..f0f4140 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_MTD_CMDLINE_PARTS) += cmdlinepart.o
>  obj-$(CONFIG_MTD_AFS_PARTS)    += afs.o
>  obj-$(CONFIG_MTD_AR7_PARTS)    += ar7part.o
>  obj-$(CONFIG_MTD_BCM63XX_PARTS)        += bcm63xxpart.o
> +obj-$(CONFIG_MTD_BCM963268_PARTS) += bcm963268part.o
>  obj-$(CONFIG_MTD_BCM47XX_PARTS)        += bcm47xxpart.o
>
>  # 'Users' - code which presents functionality to userspace.
> diff --git a/drivers/mtd/bcm963268part.c b/drivers/mtd/bcm963268part.c
> new file mode 100644
> index 0000000..2740a48
> --- /dev/null
> +++ b/drivers/mtd/bcm963268part.c
> @@ -0,0 +1,350 @@
> +/*
> + * BCM963268 CFE image tag parser
> + * Copyright 2015 Simon Arlott
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Derived from drivers/mtd/bcm63xxpart.c:
> + * Copyright © 2006-2008  Florian Fainelli <florian@openwrt.org>
> + *                       Mike Albon <malbon@openwrt.org>
> + * Copyright © 2009-2010  Daniel Dickinson <openwrt@cshore.neomailbox.net>
> + * Copyright © 2011-2013  Jonas Gorski <jonas.gorski@gmail.com>
> + *
> + * Derived from bcm963xx_4.12L.06B_consumer/bcmdrivers/opensource/char/board/bcm963xx/impl1/board.c,
> + *     bcm963xx_4.12L.06B_consumer/shared/opensource/include/bcm963xx/bcm_hwdefs.h:
> + * Copyright (c) 2002 Broadcom Corporation
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/crc32.h>
> +#include <linux/if_ether.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +
> +#include <asm/mach-bcm63xx/bcm963xx_tag.h>

Why not move that one as well? Then you don't need to depend on MIPS
for COMPILE_TEST.

> +#include <uapi/linux/bcm963xx_nvram.h>
> +
> +/* Extended flash address, needs to be subtracted from bcm_tag flash offsets */
> +#define BCM63XX_EXTENDED_SIZE          0xBFC00000

This should be in bcm963xx_tag.h.
> +
> +#define BCM63XX_CFE_MAGIC_OFFSET       0x4e0
> +#define BCM963XX_CFE_VERSION_OFFSET    0x570
> +#define BCM963XX_NVRAM_OFFSET          0x580

You should decide whether you want to call it BCM63XX (the SoC) or
BCM963XX (the Board using the SoC). Also probably better served in
bcm963xx_nvram.h


> +
> +enum bcm963268_nvram_part {
> +       PART_BOOT = 0,
> +       PART_ROOTFS_1,
> +       PART_ROOTFS_2,
> +       PART_DATA,
> +       PART_BBT,
> +};

This should be in bcm963xx_nvram.h.

> +
> +/* Ensure strings read from flash structs are null terminated */
> +#define STR_NULL_TERMINATE(x) \
> +       do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0)
> +
> +static int bcm963268_detect_cfe(struct mtd_info *master)
> +{
> +       char buf[9];
> +       int ret;
> +       size_t retlen;
> +
> +       ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, &retlen,
> +                      (void *)buf);
> +       buf[retlen] = 0;
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       if (strncmp("cfe-v", buf, 5) == 0)
> +               return 0;
> +
> +       return -EINVAL;
> +}
> +
> +static int bcm963268_read_nvram(struct mtd_info *master,
> +       struct bcm963xx_nvram *nvram)
> +{
> +       u32 crc, expected_crc;
> +       size_t retlen;
> +       int ret;
> +
> +       /* extract nvram data */
> +       ret = mtd_read(master, BCM963XX_NVRAM_OFFSET, BCM963XX_NVRAM_V6_SIZE,
> +                       &retlen, (void *)nvram);
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       if (nvram->version < 6) {
> +               pr_warn("nvram version %d not supported\n", nvram->version);
> +               return -EINVAL;
> +       }
> +
> +       /* check checksum before using data */
> +       expected_crc = nvram->checksum_v6;
> +       nvram->checksum_v6 = 0;
> +
> +       crc = crc32_le(~0, (u8 *)nvram, BCM963XX_NVRAM_V6_SIZE);
> +
> +       if (crc != expected_crc)
> +               pr_warn("nvram checksum failed, contents may be invalid (expected %08x, got %08x)\n",
> +                       expected_crc, crc);
> +
> +       return 0;
> +}
> +
> +static bool bcm963268_boot_latest(struct bcm963xx_nvram *nvram)
> +{
> +       char *p;
> +
> +       STR_NULL_TERMINATE(nvram->bootline);
> +
> +       /* Find previous image parameter "p" */
> +       if (!strncmp(nvram->bootline, "p=", 2))
> +               p = nvram->bootline;
> +       else
> +               p = strstr(nvram->bootline, " p=");
> +
> +       if (p == NULL)
> +               return true;
> +
> +       p += 2;
> +       if (*p == '\0')
> +               return true;

 '\0' is also != '0', so this check is also covered by the following comparison.

> +
> +       return *p != '0';
> +}
> +
> +static bool bcm963268_parse_rootfs_tag(struct mtd_info *master,
> +       const char *name, loff_t rootfs_part, u64 *rootfs_offset,
> +       u64 *rootfs_size, unsigned int *rootfs_sequence)
> +{
> +       struct bcm_tag *buf;
> +       int ret;
> +       size_t retlen;
> +       u32 computed_crc;
> +       bool rootfs_ok = false;
> +
> +       *rootfs_offset = 0;
> +       *rootfs_size = 0;
> +       *rootfs_sequence = 0;
> +
> +       buf = vmalloc(sizeof(struct bcm_tag));
> +       if (!buf)
> +               goto out;
> +
> +       ret = mtd_read(master, rootfs_part, sizeof(*buf), &retlen, (void *)buf);
> +       if (ret < 0)
> +               goto out;
> +
> +       if (retlen != sizeof(*buf))
> +               goto out;
> +
> +       computed_crc = crc32_le(IMAGETAG_CRC_START, (u8 *)buf,
> +                               offsetof(struct bcm_tag, header_crc));
> +       if (computed_crc == buf->header_crc) {
> +               STR_NULL_TERMINATE(buf->board_id);
> +               STR_NULL_TERMINATE(buf->tag_version);
> +
> +               pr_info("%s: CFE boot tag found at 0x%llx with version %s, board type %s\n",
> +                       name, rootfs_part, buf->tag_version, buf->board_id);
> +
> +               /* Get rootfs offset and size from tag data */
> +               STR_NULL_TERMINATE(buf->flash_image_start);
> +               if (kstrtou64(buf->flash_image_start, 10, rootfs_offset) ||
> +                               *rootfs_offset < BCM63XX_EXTENDED_SIZE) {
> +                       pr_err("%s: invalid rootfs offset: %*ph\n", name,
> +                               sizeof(buf->flash_image_start),
> +                               buf->flash_image_start);
> +                       goto out;
> +               }

what are the values for kernel(len)? if these are zero, it might make
sense to merge the two parsers, or at least have them in a single
compile unit to have them share the tag parsing code.

> +
> +               STR_NULL_TERMINATE(buf->root_length);
> +               if (kstrtou64(buf->root_length, 10, rootfs_size) ||
> +                               rootfs_size == 0) {
> +                       pr_err("%s: invalid rootfs size: %*ph\n", name,
> +                               sizeof(buf->root_length), buf->root_length);
> +                       goto out;
> +               }
> +
> +               /* Adjust for flash offset */
> +               *rootfs_offset -= BCM63XX_EXTENDED_SIZE;
> +
> +               /* Remove bcm_tag data from length */
> +               *rootfs_size -= *rootfs_offset - rootfs_part;
> +
> +               /* Get image sequence number to determine which one is newer */
> +               STR_NULL_TERMINATE(buf->dual_image);
> +               if (kstrtouint(buf->dual_image, 10, rootfs_sequence)) {

You should probably just rename and fixup the dual_image part to
image_sequence, length 4.

> +                       pr_err("%s: invalid rootfs sequence: %*ph\n", name,
> +                               sizeof(buf->dual_image), buf->dual_image);
> +                       goto out;
> +               }
> +
> +               rootfs_ok = true;
> +       } else {
> +               pr_warn("%s: CFE boot tag at 0x%llx CRC invalid (expected %08x, actual %08x)\n",
> +                       name, rootfs_part, buf->header_crc, computed_crc);
> +               goto out;
> +       }
> +
> +out:
> +       vfree(buf);
> +       return rootfs_ok;
> +}
> +
> +static int bcm963268_parse_cfe_partitions(struct mtd_info *master,
> +       const struct mtd_partition **pparts, struct mtd_part_parser_data *data)
> +{
> +       int nrparts, curpart;
> +       struct bcm963xx_nvram *nvram = NULL;
> +       struct mtd_partition *parts;
> +       u64 rootfs1_off, rootfs1_size;
> +       unsigned int rootfs1_seq;
> +       u64 rootfs2_off, rootfs2_size;
> +       unsigned int rootfs2_seq;
> +       bool rootfs1, rootfs2;
> +       bool use_first;
> +       int ret;
> +
> +       if (bcm963268_detect_cfe(master)) {
> +               ret = -EINVAL;
> +               goto out;

You haven't allocated nvram yet, just directly return -EINVAL.

> +       }
> +
> +       nvram = vzalloc(sizeof(*nvram));
> +       if (!nvram) {
> +               ret = -ENOMEM;
> +               goto out;

Similar here.

> +       }
> +
> +       if (bcm963268_read_nvram(master, nvram)) {
> +               ret = -EINVAL;

Why not pass on the return value of _read_nvram?

> +               goto out;
> +       }
> +
> +       /* We've just read the nvram from offset 0,
> +        * so it must be located there.
> +        */
> +       if (nvram->nand_part_offset[PART_BOOT] != 0) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       /* Get the rootfs partition locations */
> +       rootfs1 = bcm963268_parse_rootfs_tag(master, "rootfs1",
> +               nvram->nand_part_offset[PART_ROOTFS_1] * SZ_1K,
> +               &rootfs1_off, &rootfs1_size, &rootfs1_seq);
> +       rootfs2 = bcm963268_parse_rootfs_tag(master, "rootfs2",
> +               nvram->nand_part_offset[PART_ROOTFS_2] * SZ_1K,
> +               &rootfs2_off, &rootfs2_size, &rootfs2_seq);
> +       if (!rootfs1 && !rootfs2) {
> +               ret = -EINVAL;

While it might be nice in theory to still have the _update partitions
for flashing on an "uninitialized" flash, in practice this case is
unreachable, as CFE *needs* a working rootfs to function, and the
device is basically bricked if both are invalid.

> (snip rest)


Jonas

Re: [PATCH linux-next (v3) 3/3] mtd: part: Add BCM962368 CFE partitioning support

[Simon Arlott]

On 11/12/15 23:12, Jonas Gorski wrote:
> On Fri, Dec 11, 2015 at 11:02 PM, Simon Arlott <simon@fire.lp0.eu> wrote:
>> +#define BCM63XX_CFE_MAGIC_OFFSET       0x4e0
>> +#define BCM963XX_CFE_VERSION_OFFSET    0x570
>> +#define BCM963XX_NVRAM_OFFSET          0x580
> 
> You should decide whether you want to call it BCM63XX (the SoC) or
> BCM963XX (the Board using the SoC). Also probably better served in
> bcm963xx_nvram.h

I'm going to name the CFE locations, nvram and anything that is a
property of the flash layout BCM963XX_*, as a theoretical board using
BCM63XX with no flash wouldn't use these. The parser is still called
bcm63xxpart so that's currently an anomaly.

I'm working on making it support both NOR and NAND flash layouts.

-- 
Simon Arlott

Re: [PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[Jonas Gorski]

Hi,

On Fri, Dec 11, 2015 at 10:54 PM, Simon Arlott <simon@fire.lp0.eu> wrote:
> Broadcom BCM963xx boards have multiple nvram variants across different
> SoCs with additional checksum fields added whenever the size of the
> nvram was extended.
>
> Add this structure as a header file so that multiple drivers and userspace
> can use it.
>
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> ---
> v3: Fix includes/type names, add comments explaining the nvram struct.
>
> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
>
>  MAINTAINERS                         |  1 +
>  include/uapi/linux/bcm963xx_nvram.h | 53 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
>  create mode 100644 include/uapi/linux/bcm963xx_nvram.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6b6d4e2e..abf18b4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2393,6 +2393,7 @@ F:        drivers/irqchip/irq-bcm63*
>  F:     drivers/irqchip/irq-bcm7*
>  F:     drivers/irqchip/irq-brcmstb*
>  F:     include/linux/bcm63xx_wdt.h
> +F:     include/uapi/linux/bcm963xx_nvram.h
>
>  BROADCOM TG3 GIGABIT ETHERNET DRIVER
>  M:     Prashant Sreedharan <prashant@broadcom.com>
> diff --git a/include/uapi/linux/bcm963xx_nvram.h b/include/uapi/linux/bcm963xx_nvram.h
> new file mode 100644
> index 0000000..2dcb307
> --- /dev/null
> +++ b/include/uapi/linux/bcm963xx_nvram.h

Why uapi? The nvram layout isn't really enforced to be that way, and
at least Huawei uses a modified one on some devices (in case you
wondered why bcm63xx doesn't fail a crc32-"broken" one), so IMHO it
should be kept for in-kernel use only.

> @@ -0,0 +1,53 @@
> +#ifndef _UAPI__LINUX_BCM963XX_NVRAM_H__
> +#define _UAPI__LINUX_BCM963XX_NVRAM_H__
> +
> +#include <linux/types.h>
> +#include <linux/if_ether.h>
> +
> +/*
> + * Broadcom BCM963xx SoC board nvram data structure.
> + *
> + * The nvram structure varies in size depending on the SoC board version. Use
> + * the appropriate minimum BCM963XX_NVRAM_*_SIZE define for the information
> + * you need instead of sizeof(struct bcm963xx_nvram) as this may change.
> + *
> + * The "version" field value maps directly to the size and checksum names, e.g.
> + * version 4 uses "checksum_v4" and the data is BCM963XX_NVRAM_V4_SIZE bytes.
> + *
> + * Do not use the __reserved fields, especially not as an offset for CRC
> + * calculations (use BCM963XX_NVRAM_*_SIZE instead). These may be removed or
> + * repositioned.
> + */
> +
> +#define BCM963XX_NVRAM_V4_SIZE         300
> +#define BCM963XX_NVRAM_V5_SIZE         1024
> +#define BCM963XX_NVRAM_V6_SIZE         BCM963XX_NVRAM_V5_SIZE
> +#define BCM963XX_NVRAM_V7_SIZE         3072
> +
> +#define BCM963XX_NVRAM_NR_PARTS                5
> +
> +struct bcm963xx_nvram {
> +       __u32   version;
> +       char    bootline[256];
> +       char    name[16];
> +       __u32   main_tp_number;
> +       __u32   psi_size;
> +       __u32   mac_addr_count;
> +       __u8    mac_addr_base[ETH_ALEN];
> +       __u8    __reserved1[2];
> +       __u32   checksum_v4;
> +
> +       __u8    __reserved2[292];
> +       __u32   nand_part_offset[BCM963XX_NVRAM_NR_PARTS];
> +       __u32   nand_part_size[BCM963XX_NVRAM_NR_PARTS];
> +       __u8    __reserved3[388];
> +       union {
> +               __u32   checksum_v5;
> +               __u32   checksum_v6;
> +       };

what's the point of this union? Both are the same size and have the
same function.

> +
> +       __u8    __reserved4[2044];
> +       __u32   checksum_v7;
> +} __packed;

Why is it __packed? there are no unaligned members, so it should work
fine without this (and it did for bcm63xx).


Jonas

Re: [PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[Simon Arlott]

On 11/12/15 22:02, Jonas Gorski wrote:
> Hi,
> 
> On Fri, Dec 11, 2015 at 10:54 PM, Simon Arlott <simon@fire.lp0.eu> wrote:
>> Broadcom BCM963xx boards have multiple nvram variants across different
>> SoCs with additional checksum fields added whenever the size of the
>> nvram was extended.
>>
>> Add this structure as a header file so that multiple drivers and userspace
>> can use it.
>>
>> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
>> ---
>> v3: Fix includes/type names, add comments explaining the nvram struct.
>>
>> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
>>
>>  MAINTAINERS                         |  1 +
>>  include/uapi/linux/bcm963xx_nvram.h | 53 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 54 insertions(+)
>>  create mode 100644 include/uapi/linux/bcm963xx_nvram.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6b6d4e2e..abf18b4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2393,6 +2393,7 @@ F:        drivers/irqchip/irq-bcm63*
>>  F:     drivers/irqchip/irq-bcm7*
>>  F:     drivers/irqchip/irq-brcmstb*
>>  F:     include/linux/bcm63xx_wdt.h
>> +F:     include/uapi/linux/bcm963xx_nvram.h
>>
>>  BROADCOM TG3 GIGABIT ETHERNET DRIVER
>>  M:     Prashant Sreedharan <prashant@broadcom.com>
>> diff --git a/include/uapi/linux/bcm963xx_nvram.h b/include/uapi/linux/bcm963xx_nvram.h
>> new file mode 100644
>> index 0000000..2dcb307
>> --- /dev/null
>> +++ b/include/uapi/linux/bcm963xx_nvram.h
> 
> Why uapi? The nvram layout isn't really enforced to be that way, and
> at least Huawei uses a modified one on some devices (in case you
> wondered why bcm63xx doesn't fail a crc32-"broken" one), so IMHO it
> should be kept for in-kernel use only.

Because Florian suggested include/uapi/linux/bcm963xx_nvram.h; I could
move it to include/linux/ instead if this is preferred.

>> @@ -0,0 +1,53 @@
>> +#ifndef _UAPI__LINUX_BCM963XX_NVRAM_H__
>> +#define _UAPI__LINUX_BCM963XX_NVRAM_H__
>> +
>> +#include <linux/types.h>
>> +#include <linux/if_ether.h>
>> +
>> +/*
>> + * Broadcom BCM963xx SoC board nvram data structure.
>> + *
>> + * The nvram structure varies in size depending on the SoC board version. Use
>> + * the appropriate minimum BCM963XX_NVRAM_*_SIZE define for the information
>> + * you need instead of sizeof(struct bcm963xx_nvram) as this may change.
>> + *
>> + * The "version" field value maps directly to the size and checksum names, e.g.
>> + * version 4 uses "checksum_v4" and the data is BCM963XX_NVRAM_V4_SIZE bytes.
>> + *
>> + * Do not use the __reserved fields, especially not as an offset for CRC
>> + * calculations (use BCM963XX_NVRAM_*_SIZE instead). These may be removed or
>> + * repositioned.
>> + */
>> +
>> +#define BCM963XX_NVRAM_V4_SIZE         300
>> +#define BCM963XX_NVRAM_V5_SIZE         1024
>> +#define BCM963XX_NVRAM_V6_SIZE         BCM963XX_NVRAM_V5_SIZE
>> +#define BCM963XX_NVRAM_V7_SIZE         3072
>> +
>> +#define BCM963XX_NVRAM_NR_PARTS                5
>> +
>> +struct bcm963xx_nvram {
>> +       __u32   version;
>> +       char    bootline[256];
>> +       char    name[16];
>> +       __u32   main_tp_number;
>> +       __u32   psi_size;
>> +       __u32   mac_addr_count;
>> +       __u8    mac_addr_base[ETH_ALEN];
>> +       __u8    __reserved1[2];
>> +       __u32   checksum_v4;
>> +
>> +       __u8    __reserved2[292];
>> +       __u32   nand_part_offset[BCM963XX_NVRAM_NR_PARTS];
>> +       __u32   nand_part_size[BCM963XX_NVRAM_NR_PARTS];
>> +       __u8    __reserved3[388];
>> +       union {
>> +               __u32   checksum_v5;
>> +               __u32   checksum_v6;
>> +       };
> 
> what's the point of this union? Both are the same size and have the
> same function.

For convenience when deciding which size of nvram to use.

The mach-bcm63xx code uses the V5 definitions because it supports
checksums at the v4 and v5 sizes.

The bcm963xxpart code uses the V6 definitions because that's what my
board has and I can't tell if the nand_part values are valid in version
5 or if they were only added in version 6.
 
>> +
>> +       __u8    __reserved4[2044];
>> +       __u32   checksum_v7;
>> +} __packed;
> 
> Why is it __packed? there are no unaligned members, so it should work
> fine without this (and it did for bcm63xx).

I could remove it, but as soon as someone adds an unaligned member but
forgets to add __packed it's going to break.

There are unaligned members in some of the __reserved areas, like this
one:

#define NVRAM_GPON_SERIAL_NUMBER_LEN    13
#define NVRAM_GPON_PASSWORD_LEN         11

    char gponSerialNumber[NVRAM_GPON_SERIAL_NUMBER_LEN];
    char gponPassword[NVRAM_GPON_PASSWORD_LEN];

-- 
Simon Arlott

Re: [PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[Jonas Gorski]

On Fri, Dec 11, 2015 at 11:24 PM, Simon Arlott <simon@fire.lp0.eu> wrote:
> On 11/12/15 22:02, Jonas Gorski wrote:
>> Hi,
>>
>> On Fri, Dec 11, 2015 at 10:54 PM, Simon Arlott <simon@fire.lp0.eu> wrote:
>>> Broadcom BCM963xx boards have multiple nvram variants across different
>>> SoCs with additional checksum fields added whenever the size of the
>>> nvram was extended.
>>>
>>> Add this structure as a header file so that multiple drivers and userspace
>>> can use it.
>>>
>>> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
>>> ---
>>> v3: Fix includes/type names, add comments explaining the nvram struct.
>>>
>>> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
>>>
>>>  MAINTAINERS                         |  1 +
>>>  include/uapi/linux/bcm963xx_nvram.h | 53 +++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 54 insertions(+)
>>>  create mode 100644 include/uapi/linux/bcm963xx_nvram.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 6b6d4e2e..abf18b4 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2393,6 +2393,7 @@ F:        drivers/irqchip/irq-bcm63*
>>>  F:     drivers/irqchip/irq-bcm7*
>>>  F:     drivers/irqchip/irq-brcmstb*
>>>  F:     include/linux/bcm63xx_wdt.h
>>> +F:     include/uapi/linux/bcm963xx_nvram.h
>>>
>>>  BROADCOM TG3 GIGABIT ETHERNET DRIVER
>>>  M:     Prashant Sreedharan <prashant@broadcom.com>
>>> diff --git a/include/uapi/linux/bcm963xx_nvram.h b/include/uapi/linux/bcm963xx_nvram.h
>>> new file mode 100644
>>> index 0000000..2dcb307
>>> --- /dev/null
>>> +++ b/include/uapi/linux/bcm963xx_nvram.h
>>
>> Why uapi? The nvram layout isn't really enforced to be that way, and
>> at least Huawei uses a modified one on some devices (in case you
>> wondered why bcm63xx doesn't fail a crc32-"broken" one), so IMHO it
>> should be kept for in-kernel use only.
>
> Because Florian suggested include/uapi/linux/bcm963xx_nvram.h; I could
> move it to include/linux/ instead if this is preferred.
>
>>> @@ -0,0 +1,53 @@
>>> +#ifndef _UAPI__LINUX_BCM963XX_NVRAM_H__
>>> +#define _UAPI__LINUX_BCM963XX_NVRAM_H__
>>> +
>>> +#include <linux/types.h>
>>> +#include <linux/if_ether.h>
>>> +
>>> +/*
>>> + * Broadcom BCM963xx SoC board nvram data structure.
>>> + *
>>> + * The nvram structure varies in size depending on the SoC board version. Use
>>> + * the appropriate minimum BCM963XX_NVRAM_*_SIZE define for the information
>>> + * you need instead of sizeof(struct bcm963xx_nvram) as this may change.
>>> + *
>>> + * The "version" field value maps directly to the size and checksum names, e.g.
>>> + * version 4 uses "checksum_v4" and the data is BCM963XX_NVRAM_V4_SIZE bytes.
>>> + *
>>> + * Do not use the __reserved fields, especially not as an offset for CRC
>>> + * calculations (use BCM963XX_NVRAM_*_SIZE instead). These may be removed or
>>> + * repositioned.

Because I just saw that: Nobody will read that. ;p

>>> + */
>>> +
>>> +#define BCM963XX_NVRAM_V4_SIZE         300
>>> +#define BCM963XX_NVRAM_V5_SIZE         1024
>>> +#define BCM963XX_NVRAM_V6_SIZE         BCM963XX_NVRAM_V5_SIZE
>>> +#define BCM963XX_NVRAM_V7_SIZE         3072
>>> +
>>> +#define BCM963XX_NVRAM_NR_PARTS                5
>>> +
>>> +struct bcm963xx_nvram {
>>> +       __u32   version;
>>> +       char    bootline[256];
>>> +       char    name[16];
>>> +       __u32   main_tp_number;
>>> +       __u32   psi_size;
>>> +       __u32   mac_addr_count;
>>> +       __u8    mac_addr_base[ETH_ALEN];
>>> +       __u8    __reserved1[2];
>>> +       __u32   checksum_v4;
>>> +
>>> +       __u8    __reserved2[292];
>>> +       __u32   nand_part_offset[BCM963XX_NVRAM_NR_PARTS];
>>> +       __u32   nand_part_size[BCM963XX_NVRAM_NR_PARTS];
>>> +       __u8    __reserved3[388];
>>> +       union {
>>> +               __u32   checksum_v5;
>>> +               __u32   checksum_v6;
>>> +       };
>>
>> what's the point of this union? Both are the same size and have the
>> same function.
>
> For convenience when deciding which size of nvram to use.
>
> The mach-bcm63xx code uses the V5 definitions because it supports
> checksums at the v4 and v5 sizes.
>
> The bcm963xxpart code uses the V6 definitions because that's what my
> board has and I can't tell if the nand_part values are valid in version
> 5 or if they were only added in version 6.

But you don't have a

union {
        __u32   checksum_v1;
        __u32   checksum_v2;
        __u32   checksum_v3;
        __u32   checksum_v4;
};

so this seems inconsistent. Maybe just call these checksum_v4 /
checksum_1k / checksum_3k?

>
>>> +
>>> +       __u8    __reserved4[2044];
>>> +       __u32   checksum_v7;

FWIW, this seems to be only present on secure boot enabled BCM963268
boards or so, so isn't really a v7 checksum, but a signature present
nvram extended checksum. on != 63268 boards with a v7 nvram this won't
exist.

>>> +} __packed;
>>
>> Why is it __packed? there are no unaligned members, so it should work
>> fine without this (and it did for bcm63xx).
>
> I could remove it, but as soon as someone adds an unaligned member but
> forgets to add __packed it's going to break.

Broadcom doesn't use __packed  either, so one can assume anything with
alignment requirements will be aligned.

>
> There are unaligned members in some of the __reserved areas, like this
> one:
>
> #define NVRAM_GPON_SERIAL_NUMBER_LEN    13
> #define NVRAM_GPON_PASSWORD_LEN         11
>
>     char gponSerialNumber[NVRAM_GPON_SERIAL_NUMBER_LEN];
>     char gponPassword[NVRAM_GPON_PASSWORD_LEN];

char is size 1, it can never be unaligned (as relevant for __packed).
And together they are a multiple of 4, so anything following will be
correctly aligned again.


Jonas

Re: [PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[Simon Arlott]

On 11/12/15 23:29, Jonas Gorski wrote:
> On Fri, Dec 11, 2015 at 11:24 PM, Simon Arlott <simon@fire.lp0.eu> wrote:
>> On 11/12/15 22:02, Jonas Gorski wrote:
>>> On Fri, Dec 11, 2015 at 10:54 PM, Simon Arlott <simon@fire.lp0.eu> wrote:
>>>> Broadcom BCM963xx boards have multiple nvram variants across different
>>>> SoCs with additional checksum fields added whenever the size of the
>>>> nvram was extended.
>>>>
>>>> Add this structure as a header file so that multiple drivers and userspace
>>>> can use it.
>>>>
>>>> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
>>>> ---
>>>> v3: Fix includes/type names, add comments explaining the nvram struct.
>>>>
>>>> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
...
>>>> diff --git a/include/uapi/linux/bcm963xx_nvram.h b/include/uapi/linux/bcm963xx_nvram.h
>>>> new file mode 100644
>>>> index 0000000..2dcb307
>>>> --- /dev/null
>>>> +++ b/include/uapi/linux/bcm963xx_nvram.h
>>>
>>> Why uapi? The nvram layout isn't really enforced to be that way, and
>>> at least Huawei uses a modified one on some devices (in case you
>>> wondered why bcm63xx doesn't fail a crc32-"broken" one), so IMHO it
>>> should be kept for in-kernel use only.
>>
>> Because Florian suggested include/uapi/linux/bcm963xx_nvram.h; I could
>> move it to include/linux/ instead if this is preferred.
...
>>>> + * Do not use the __reserved fields, especially not as an offset for CRC
>>>> + * calculations (use BCM963XX_NVRAM_*_SIZE instead). These may be removed or
>>>> + * repositioned.
> 
> Because I just saw that: Nobody will read that. ;p

I'll move this to include/linux/bcm963xx_nvram.h and omit the linux-api
mailing list when I next send the patch series.

-- 
Simon Arlott