[PATCH 0/3] qcom: Add SMEM MTD parser

[PATCH 0/3] qcom: Add SMEM MTD parser

[Mathieu Olivari]

QCOM platforms such as IPQ806x are using SMEM to store their flash
layout. This patch set adds the DT nodes required to instanciate SMEM
on IPQ806x and add an MTD parser using it.

This change is based on the SMEM driver posted here:
*https://lkml.org/lkml/2015/7/27/1125

v2:
*Release the SPI device reference after looking it up (put_device())
*Represent SMEM data as __le32 rather than u32
*Move new DT nodes in their proper respective location
*Address readability concerns in MTD parser

Mathieu Olivari (3):
  ARM: qcom: add SFPB nodes to IPQ806x dts
  ARM: qcom: add SMEM device node to IPQ806x dts
  mtd: add SMEM parser for QCOM platforms

 arch/arm/boot/dts/qcom-ipq8064.dtsi |  23 +++-
 drivers/mtd/Kconfig                 |   7 ++
 drivers/mtd/Makefile                |   1 +
 drivers/mtd/qcom_smem_part.c        | 224 ++++++++++++++++++++++++++++++++++++
 4 files changed, 254 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/qcom_smem_part.c

-- 
2.1.4

[PATCH 0/3] qcom: Add SMEM MTD parser

[Mathieu Olivari]

QCOM platforms such as IPQ806x are using SMEM to store their flash
layout. This patch set adds the DT nodes required to instanciate SMEM
on IPQ806x and add an MTD parser using it.

This change is based on the SMEM driver posted here:
*https://lkml.org/lkml/2015/7/27/1125

v2:
*Release the SPI device reference after looking it up (put_device())
*Represent SMEM data as __le32 rather than u32
*Move new DT nodes in their proper respective location
*Address readability concerns in MTD parser

Mathieu Olivari (3):
  ARM: qcom: add SFPB nodes to IPQ806x dts
  ARM: qcom: add SMEM device node to IPQ806x dts
  mtd: add SMEM parser for QCOM platforms

 arch/arm/boot/dts/qcom-ipq8064.dtsi |  23 +++-
 drivers/mtd/Kconfig                 |   7 ++
 drivers/mtd/Makefile                |   1 +
 drivers/mtd/qcom_smem_part.c        | 224 ++++++++++++++++++++++++++++++++++++
 4 files changed, 254 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/qcom_smem_part.c

-- 
2.1.4

[PATCH 1/3] ARM: qcom: add SFPB nodes to IPQ806x dts

[Mathieu Olivari]

Add one new node to the ipq806x.dtsi file to declare & register the
hardware spinlock devices. This mechanism is required to be used by
other drivers such as SMEM.

Signed-off-by: Mathieu Olivari <mathieu@codeaurora.org>
---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 9f727d8..8d366ae 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -329,5 +329,16 @@
 			#reset-cells = <1>;
 		};
 
+		sfpb_mutex_block: syscon@1200600 {
+			compatible = "syscon";
+			reg = <0x01200600 0x100>;
+		};
+	};
+
+	sfpb_mutex: sfpb-mutex {
+		compatible = "qcom,sfpb-mutex";
+		syscon = <&sfpb_mutex_block 4 4>;
+
+		#hwlock-cells = <1>;
 	};
 };
-- 
2.1.4

[PATCH 1/3] ARM: qcom: add SFPB nodes to IPQ806x dts

[Mathieu Olivari]

Add one new node to the ipq806x.dtsi file to declare & register the
hardware spinlock devices. This mechanism is required to be used by
other drivers such as SMEM.

Signed-off-by: Mathieu Olivari <mathieu-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 9f727d8..8d366ae 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -329,5 +329,16 @@
 			#reset-cells = <1>;
 		};
 
+		sfpb_mutex_block: syscon@1200600 {
+			compatible = "syscon";
+			reg = <0x01200600 0x100>;
+		};
+	};
+
+	sfpb_mutex: sfpb-mutex {
+		compatible = "qcom,sfpb-mutex";
+		syscon = <&sfpb_mutex_block 4 4>;
+
+		#hwlock-cells = <1>;
 	};
 };
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] ARM: qcom: add SFPB nodes to IPQ806x dts

[Mathieu Olivari]

Add one new node to the ipq806x.dtsi file to declare & register the
hardware spinlock devices. This mechanism is required to be used by
other drivers such as SMEM.

Signed-off-by: Mathieu Olivari <mathieu@codeaurora.org>
---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 9f727d8..8d366ae 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -329,5 +329,16 @@
 			#reset-cells = <1>;
 		};
 
+		sfpb_mutex_block: syscon at 1200600 {
+			compatible = "syscon";
+			reg = <0x01200600 0x100>;
+		};
+	};
+
+	sfpb_mutex: sfpb-mutex {
+		compatible = "qcom,sfpb-mutex";
+		syscon = <&sfpb_mutex_block 4 4>;
+
+		#hwlock-cells = <1>;
 	};
 };
-- 
2.1.4

[PATCH 2/3] ARM: qcom: add SMEM device node to IPQ806x dts

[Mathieu Olivari]

SMEM is used on IPQ806x to store various board related information such
as boot device and flash partition layout. We'll declare it as a device
so we can make use of it thanks to the new SMEM soc driver.

Signed-off-by: Mathieu Olivari <mathieu@codeaurora.org>
---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 8d366ae..85dbccf 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -55,7 +55,7 @@
 			no-map;
 		};
 
-		smem@41000000 {
+		smem: smem@41000000 {
 			reg = <0x41000000 0x200000>;
 			no-map;
 		};
@@ -341,4 +341,10 @@
 
 		#hwlock-cells = <1>;
 	};
+
+	smem {
+		compatible = "qcom,smem";
+		memory-region = <&smem>;
+		hwlocks = <&sfpb_mutex 3>;
+	};
 };
-- 
2.1.4

[PATCH 2/3] ARM: qcom: add SMEM device node to IPQ806x dts

[Mathieu Olivari]

SMEM is used on IPQ806x to store various board related information such
as boot device and flash partition layout. We'll declare it as a device
so we can make use of it thanks to the new SMEM soc driver.

Signed-off-by: Mathieu Olivari <mathieu@codeaurora.org>
---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 8d366ae..85dbccf 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -55,7 +55,7 @@
 			no-map;
 		};
 
-		smem at 41000000 {
+		smem: smem at 41000000 {
 			reg = <0x41000000 0x200000>;
 			no-map;
 		};
@@ -341,4 +341,10 @@
 
 		#hwlock-cells = <1>;
 	};
+
+	smem {
+		compatible = "qcom,smem";
+		memory-region = <&smem>;
+		hwlocks = <&sfpb_mutex 3>;
+	};
 };
-- 
2.1.4

[PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Mathieu Olivari]

On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
used to store partition layout. This new parser can now be used to read
SMEM and use it to register an MTD layout according to its content.

Signed-off-by: Mathieu Olivari <mathieu@codeaurora.org>
---
 drivers/mtd/Kconfig          |   7 ++
 drivers/mtd/Makefile         |   1 +
 drivers/mtd/qcom_smem_part.c | 231 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 239 insertions(+)
 create mode 100644 drivers/mtd/qcom_smem_part.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index a03ad29..debc887 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -155,6 +155,13 @@ config MTD_BCM47XX_PARTS
 	  This provides partitions parser for devices based on BCM47xx
 	  boards.
 
+config MTD_QCOM_SMEM_PARTS
+	tristate "QCOM SMEM partitioning support"
+	depends on QCOM_SMEM
+	help
+	  This provides partitions parser for QCOM devices using SMEM
+	  such as IPQ806x.
+
 comment "User Modules And Translation Layers"
 
 #
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 99bb9a1..b3c7de4 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_MTD_AFS_PARTS)	+= afs.o
 obj-$(CONFIG_MTD_AR7_PARTS)	+= ar7part.o
 obj-$(CONFIG_MTD_BCM63XX_PARTS)	+= bcm63xxpart.o
 obj-$(CONFIG_MTD_BCM47XX_PARTS)	+= bcm47xxpart.o
+obj-$(CONFIG_MTD_QCOM_SMEM_PARTS) += qcom_smem_part.o
 
 # 'Users' - code which presents functionality to userspace.
 obj-$(CONFIG_MTD_BLKDEVS)	+= mtd_blkdevs.o
diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
new file mode 100644
index 0000000..20fef5c
--- /dev/null
+++ b/drivers/mtd/qcom_smem_part.c
@@ -0,0 +1,231 @@
+/*
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+
+#include <linux/soc/qcom/smem.h>
+
+/* Processor/host identifier for the application processor */
+#define SMEM_HOST_APPS			0
+
+/* SMEM items index */
+#define SMEM_AARM_PARTITION_TABLE	9
+#define SMEM_BOOT_FLASH_TYPE		421
+#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
+
+/* SMEM Flash types */
+#define SMEM_FLASH_NAND			2
+#define SMEM_FLASH_SPI			6
+
+#define SMEM_PART_NAME_SZ		16
+#define SMEM_PARTS_MAX			32
+
+struct smem_partition {
+	char name[SMEM_PART_NAME_SZ];
+	__le32 start;
+	__le32 size;
+	__le32 attr;
+};
+
+struct smem_partition_table {
+	u8 magic[8];
+	__le32 version;
+	__le32 len;
+	struct smem_partition parts[SMEM_PARTS_MAX];
+};
+
+/* SMEM Magic values in partition table */
+static const u8 SMEM_PTABLE_MAGIC[] = {
+	0xaa, 0x73, 0xee, 0x55,
+	0xdb, 0xbd, 0x5e, 0xe3,
+};
+
+static int qcom_smem_get_flash_blksz(u64 **smem_blksz)
+{
+	int ret;
+	size_t size;
+
+	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
+			    (void **) smem_blksz, &size);
+
+	if (ret < 0) {
+		pr_err("Unable to read flash blksz from SMEM\n");
+		return -ENOENT;
+	}
+
+	if (size != sizeof(**smem_blksz)) {
+		pr_err("Invalid flash blksz size in SMEM\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_smem_get_flash_type(u64 **smem_flash_type)
+{
+	int ret;
+	size_t size;
+
+	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
+			    (void **) smem_flash_type, &size);
+
+	if (ret < 0) {
+		pr_err("Unable to read flash type from SMEM\n");
+		return -ENOENT;
+	}
+
+	if (size != sizeof(**smem_flash_type)) {
+		pr_err("Invalid flash type size in SMEM\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
+{
+	int ret;
+	size_t size;
+
+	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
+			    (void **) pparts, &size);
+
+	if (ret < 0) {
+		pr_err("Unable to read partition table from SMEM\n");
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
+static int of_dev_node_match(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static bool is_spi_device(struct device_node *np)
+{
+	struct device *dev;
+
+	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
+	if (!dev)
+		return false;
+
+	put_device(dev);
+	return true;
+}
+
+static int parse_qcom_smem_partitions(struct mtd_info *master,
+				      struct mtd_partition **pparts,
+				      struct mtd_part_parser_data *data)
+{
+	struct smem_partition_table *smem_parts;
+	u64 *smem_flash_type, *smem_blksz;
+	struct mtd_partition *mtd_parts;
+	struct device_node *of_node = data->of_node;
+	int i, ret;
+
+	/*
+	 * SMEM will only store the partition table of the boot device.
+	 * If this is not the boot device, do not return any partition.
+	 */
+	ret = qcom_smem_get_flash_type(&smem_flash_type);
+	if (ret < 0)
+		return ret;
+
+	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
+	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
+		return 0;
+
+	/*
+	 * Just for sanity purpose, make sure the block size in SMEM matches the
+	 * block size of the MTD device
+	 */
+	ret = qcom_smem_get_flash_blksz(&smem_blksz);
+	if (ret < 0)
+		return ret;
+
+	if (*smem_blksz != master->erasesize) {
+		pr_err("SMEM block size differs from MTD block size\n");
+		return -EINVAL;
+	}
+
+	/* Get partition pointer from SMEM */
+	ret = qcom_smem_get_flash_partitions(&smem_parts);
+	if (ret < 0)
+		return ret;
+
+	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
+		   sizeof(SMEM_PTABLE_MAGIC))) {
+		pr_err("SMEM partition magic invalid\n");
+		return -EINVAL;
+	}
+
+	/* Allocate and populate the mtd structures */
+	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
+			    GFP_KERNEL);
+	if (!mtd_parts)
+		return -ENOMEM;
+
+	for (i = 0; i < smem_parts->len; i++) {
+		struct smem_partition *s_part = &smem_parts->parts[i];
+		struct mtd_partition *m_part = &mtd_parts[i];
+
+		m_part->name = s_part->name;
+		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
+		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
+
+		/*
+		 * The last SMEM partition may have its size marked as
+		 * something like 0xffffffff, which means "until the end of the
+		 * flash device". In this case, truncate it.
+		 */
+		if (m_part->offset + m_part->size > master->size)
+			m_part->size = master->size - m_part->offset;
+	}
+
+	*pparts = mtd_parts;
+
+	return smem_parts->len;
+}
+
+static struct mtd_part_parser qcom_smem_parser = {
+	.owner = THIS_MODULE,
+	.parse_fn = parse_qcom_smem_partitions,
+	.name = "qcom-smem",
+};
+
+static int __init qcom_smem_parser_init(void)
+{
+	register_mtd_parser(&qcom_smem_parser);
+	return 0;
+}
+
+static void __exit qcom_smem_parser_exit(void)
+{
+	deregister_mtd_parser(&qcom_smem_parser);
+}
+
+module_init(qcom_smem_parser_init);
+module_exit(qcom_smem_parser_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Mathieu Olivari <mathieu@codeaurora.org>");
+MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");
-- 
2.1.4

[PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Mathieu Olivari]

On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
used to store partition layout. This new parser can now be used to read
SMEM and use it to register an MTD layout according to its content.

Signed-off-by: Mathieu Olivari <mathieu-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/mtd/Kconfig          |   7 ++
 drivers/mtd/Makefile         |   1 +
 drivers/mtd/qcom_smem_part.c | 231 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 239 insertions(+)
 create mode 100644 drivers/mtd/qcom_smem_part.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index a03ad29..debc887 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -155,6 +155,13 @@ config MTD_BCM47XX_PARTS
 	  This provides partitions parser for devices based on BCM47xx
 	  boards.
 
+config MTD_QCOM_SMEM_PARTS
+	tristate "QCOM SMEM partitioning support"
+	depends on QCOM_SMEM
+	help
+	  This provides partitions parser for QCOM devices using SMEM
+	  such as IPQ806x.
+
 comment "User Modules And Translation Layers"
 
 #
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 99bb9a1..b3c7de4 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_MTD_AFS_PARTS)	+= afs.o
 obj-$(CONFIG_MTD_AR7_PARTS)	+= ar7part.o
 obj-$(CONFIG_MTD_BCM63XX_PARTS)	+= bcm63xxpart.o
 obj-$(CONFIG_MTD_BCM47XX_PARTS)	+= bcm47xxpart.o
+obj-$(CONFIG_MTD_QCOM_SMEM_PARTS) += qcom_smem_part.o
 
 # 'Users' - code which presents functionality to userspace.
 obj-$(CONFIG_MTD_BLKDEVS)	+= mtd_blkdevs.o
diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
new file mode 100644
index 0000000..20fef5c
--- /dev/null
+++ b/drivers/mtd/qcom_smem_part.c
@@ -0,0 +1,231 @@
+/*
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+
+#include <linux/soc/qcom/smem.h>
+
+/* Processor/host identifier for the application processor */
+#define SMEM_HOST_APPS			0
+
+/* SMEM items index */
+#define SMEM_AARM_PARTITION_TABLE	9
+#define SMEM_BOOT_FLASH_TYPE		421
+#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
+
+/* SMEM Flash types */
+#define SMEM_FLASH_NAND			2
+#define SMEM_FLASH_SPI			6
+
+#define SMEM_PART_NAME_SZ		16
+#define SMEM_PARTS_MAX			32
+
+struct smem_partition {
+	char name[SMEM_PART_NAME_SZ];
+	__le32 start;
+	__le32 size;
+	__le32 attr;
+};
+
+struct smem_partition_table {
+	u8 magic[8];
+	__le32 version;
+	__le32 len;
+	struct smem_partition parts[SMEM_PARTS_MAX];
+};
+
+/* SMEM Magic values in partition table */
+static const u8 SMEM_PTABLE_MAGIC[] = {
+	0xaa, 0x73, 0xee, 0x55,
+	0xdb, 0xbd, 0x5e, 0xe3,
+};
+
+static int qcom_smem_get_flash_blksz(u64 **smem_blksz)
+{
+	int ret;
+	size_t size;
+
+	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
+			    (void **) smem_blksz, &size);
+
+	if (ret < 0) {
+		pr_err("Unable to read flash blksz from SMEM\n");
+		return -ENOENT;
+	}
+
+	if (size != sizeof(**smem_blksz)) {
+		pr_err("Invalid flash blksz size in SMEM\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_smem_get_flash_type(u64 **smem_flash_type)
+{
+	int ret;
+	size_t size;
+
+	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
+			    (void **) smem_flash_type, &size);
+
+	if (ret < 0) {
+		pr_err("Unable to read flash type from SMEM\n");
+		return -ENOENT;
+	}
+
+	if (size != sizeof(**smem_flash_type)) {
+		pr_err("Invalid flash type size in SMEM\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
+{
+	int ret;
+	size_t size;
+
+	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
+			    (void **) pparts, &size);
+
+	if (ret < 0) {
+		pr_err("Unable to read partition table from SMEM\n");
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
+static int of_dev_node_match(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static bool is_spi_device(struct device_node *np)
+{
+	struct device *dev;
+
+	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
+	if (!dev)
+		return false;
+
+	put_device(dev);
+	return true;
+}
+
+static int parse_qcom_smem_partitions(struct mtd_info *master,
+				      struct mtd_partition **pparts,
+				      struct mtd_part_parser_data *data)
+{
+	struct smem_partition_table *smem_parts;
+	u64 *smem_flash_type, *smem_blksz;
+	struct mtd_partition *mtd_parts;
+	struct device_node *of_node = data->of_node;
+	int i, ret;
+
+	/*
+	 * SMEM will only store the partition table of the boot device.
+	 * If this is not the boot device, do not return any partition.
+	 */
+	ret = qcom_smem_get_flash_type(&smem_flash_type);
+	if (ret < 0)
+		return ret;
+
+	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
+	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
+		return 0;
+
+	/*
+	 * Just for sanity purpose, make sure the block size in SMEM matches the
+	 * block size of the MTD device
+	 */
+	ret = qcom_smem_get_flash_blksz(&smem_blksz);
+	if (ret < 0)
+		return ret;
+
+	if (*smem_blksz != master->erasesize) {
+		pr_err("SMEM block size differs from MTD block size\n");
+		return -EINVAL;
+	}
+
+	/* Get partition pointer from SMEM */
+	ret = qcom_smem_get_flash_partitions(&smem_parts);
+	if (ret < 0)
+		return ret;
+
+	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
+		   sizeof(SMEM_PTABLE_MAGIC))) {
+		pr_err("SMEM partition magic invalid\n");
+		return -EINVAL;
+	}
+
+	/* Allocate and populate the mtd structures */
+	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
+			    GFP_KERNEL);
+	if (!mtd_parts)
+		return -ENOMEM;
+
+	for (i = 0; i < smem_parts->len; i++) {
+		struct smem_partition *s_part = &smem_parts->parts[i];
+		struct mtd_partition *m_part = &mtd_parts[i];
+
+		m_part->name = s_part->name;
+		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
+		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
+
+		/*
+		 * The last SMEM partition may have its size marked as
+		 * something like 0xffffffff, which means "until the end of the
+		 * flash device". In this case, truncate it.
+		 */
+		if (m_part->offset + m_part->size > master->size)
+			m_part->size = master->size - m_part->offset;
+	}
+
+	*pparts = mtd_parts;
+
+	return smem_parts->len;
+}
+
+static struct mtd_part_parser qcom_smem_parser = {
+	.owner = THIS_MODULE,
+	.parse_fn = parse_qcom_smem_partitions,
+	.name = "qcom-smem",
+};
+
+static int __init qcom_smem_parser_init(void)
+{
+	register_mtd_parser(&qcom_smem_parser);
+	return 0;
+}
+
+static void __exit qcom_smem_parser_exit(void)
+{
+	deregister_mtd_parser(&qcom_smem_parser);
+}
+
+module_init(qcom_smem_parser_init);
+module_exit(qcom_smem_parser_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Mathieu Olivari <mathieu-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>");
+MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Mathieu Olivari]

On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
used to store partition layout. This new parser can now be used to read
SMEM and use it to register an MTD layout according to its content.

Signed-off-by: Mathieu Olivari <mathieu@codeaurora.org>
---
 drivers/mtd/Kconfig          |   7 ++
 drivers/mtd/Makefile         |   1 +
 drivers/mtd/qcom_smem_part.c | 231 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 239 insertions(+)
 create mode 100644 drivers/mtd/qcom_smem_part.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index a03ad29..debc887 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -155,6 +155,13 @@ config MTD_BCM47XX_PARTS
 	  This provides partitions parser for devices based on BCM47xx
 	  boards.
 
+config MTD_QCOM_SMEM_PARTS
+	tristate "QCOM SMEM partitioning support"
+	depends on QCOM_SMEM
+	help
+	  This provides partitions parser for QCOM devices using SMEM
+	  such as IPQ806x.
+
 comment "User Modules And Translation Layers"
 
 #
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 99bb9a1..b3c7de4 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_MTD_AFS_PARTS)	+= afs.o
 obj-$(CONFIG_MTD_AR7_PARTS)	+= ar7part.o
 obj-$(CONFIG_MTD_BCM63XX_PARTS)	+= bcm63xxpart.o
 obj-$(CONFIG_MTD_BCM47XX_PARTS)	+= bcm47xxpart.o
+obj-$(CONFIG_MTD_QCOM_SMEM_PARTS) += qcom_smem_part.o
 
 # 'Users' - code which presents functionality to userspace.
 obj-$(CONFIG_MTD_BLKDEVS)	+= mtd_blkdevs.o
diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
new file mode 100644
index 0000000..20fef5c
--- /dev/null
+++ b/drivers/mtd/qcom_smem_part.c
@@ -0,0 +1,231 @@
+/*
+ * Copyright (c) 2015, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+
+#include <linux/soc/qcom/smem.h>
+
+/* Processor/host identifier for the application processor */
+#define SMEM_HOST_APPS			0
+
+/* SMEM items index */
+#define SMEM_AARM_PARTITION_TABLE	9
+#define SMEM_BOOT_FLASH_TYPE		421
+#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
+
+/* SMEM Flash types */
+#define SMEM_FLASH_NAND			2
+#define SMEM_FLASH_SPI			6
+
+#define SMEM_PART_NAME_SZ		16
+#define SMEM_PARTS_MAX			32
+
+struct smem_partition {
+	char name[SMEM_PART_NAME_SZ];
+	__le32 start;
+	__le32 size;
+	__le32 attr;
+};
+
+struct smem_partition_table {
+	u8 magic[8];
+	__le32 version;
+	__le32 len;
+	struct smem_partition parts[SMEM_PARTS_MAX];
+};
+
+/* SMEM Magic values in partition table */
+static const u8 SMEM_PTABLE_MAGIC[] = {
+	0xaa, 0x73, 0xee, 0x55,
+	0xdb, 0xbd, 0x5e, 0xe3,
+};
+
+static int qcom_smem_get_flash_blksz(u64 **smem_blksz)
+{
+	int ret;
+	size_t size;
+
+	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
+			    (void **) smem_blksz, &size);
+
+	if (ret < 0) {
+		pr_err("Unable to read flash blksz from SMEM\n");
+		return -ENOENT;
+	}
+
+	if (size != sizeof(**smem_blksz)) {
+		pr_err("Invalid flash blksz size in SMEM\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_smem_get_flash_type(u64 **smem_flash_type)
+{
+	int ret;
+	size_t size;
+
+	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
+			    (void **) smem_flash_type, &size);
+
+	if (ret < 0) {
+		pr_err("Unable to read flash type from SMEM\n");
+		return -ENOENT;
+	}
+
+	if (size != sizeof(**smem_flash_type)) {
+		pr_err("Invalid flash type size in SMEM\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
+{
+	int ret;
+	size_t size;
+
+	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
+			    (void **) pparts, &size);
+
+	if (ret < 0) {
+		pr_err("Unable to read partition table from SMEM\n");
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
+static int of_dev_node_match(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static bool is_spi_device(struct device_node *np)
+{
+	struct device *dev;
+
+	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
+	if (!dev)
+		return false;
+
+	put_device(dev);
+	return true;
+}
+
+static int parse_qcom_smem_partitions(struct mtd_info *master,
+				      struct mtd_partition **pparts,
+				      struct mtd_part_parser_data *data)
+{
+	struct smem_partition_table *smem_parts;
+	u64 *smem_flash_type, *smem_blksz;
+	struct mtd_partition *mtd_parts;
+	struct device_node *of_node = data->of_node;
+	int i, ret;
+
+	/*
+	 * SMEM will only store the partition table of the boot device.
+	 * If this is not the boot device, do not return any partition.
+	 */
+	ret = qcom_smem_get_flash_type(&smem_flash_type);
+	if (ret < 0)
+		return ret;
+
+	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
+	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
+		return 0;
+
+	/*
+	 * Just for sanity purpose, make sure the block size in SMEM matches the
+	 * block size of the MTD device
+	 */
+	ret = qcom_smem_get_flash_blksz(&smem_blksz);
+	if (ret < 0)
+		return ret;
+
+	if (*smem_blksz != master->erasesize) {
+		pr_err("SMEM block size differs from MTD block size\n");
+		return -EINVAL;
+	}
+
+	/* Get partition pointer from SMEM */
+	ret = qcom_smem_get_flash_partitions(&smem_parts);
+	if (ret < 0)
+		return ret;
+
+	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
+		   sizeof(SMEM_PTABLE_MAGIC))) {
+		pr_err("SMEM partition magic invalid\n");
+		return -EINVAL;
+	}
+
+	/* Allocate and populate the mtd structures */
+	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
+			    GFP_KERNEL);
+	if (!mtd_parts)
+		return -ENOMEM;
+
+	for (i = 0; i < smem_parts->len; i++) {
+		struct smem_partition *s_part = &smem_parts->parts[i];
+		struct mtd_partition *m_part = &mtd_parts[i];
+
+		m_part->name = s_part->name;
+		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
+		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
+
+		/*
+		 * The last SMEM partition may have its size marked as
+		 * something like 0xffffffff, which means "until the end of the
+		 * flash device". In this case, truncate it.
+		 */
+		if (m_part->offset + m_part->size > master->size)
+			m_part->size = master->size - m_part->offset;
+	}
+
+	*pparts = mtd_parts;
+
+	return smem_parts->len;
+}
+
+static struct mtd_part_parser qcom_smem_parser = {
+	.owner = THIS_MODULE,
+	.parse_fn = parse_qcom_smem_partitions,
+	.name = "qcom-smem",
+};
+
+static int __init qcom_smem_parser_init(void)
+{
+	register_mtd_parser(&qcom_smem_parser);
+	return 0;
+}
+
+static void __exit qcom_smem_parser_exit(void)
+{
+	deregister_mtd_parser(&qcom_smem_parser);
+}
+
+module_init(qcom_smem_parser_init);
+module_exit(qcom_smem_parser_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Mathieu Olivari <mathieu@codeaurora.org>");
+MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");
-- 
2.1.4

Re: [PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Bjorn Andersson]

On Fri 14 Aug 17:46 PDT 2015, Mathieu Olivari wrote:

> On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
> used to store partition layout. This new parser can now be used to read
> SMEM and use it to register an MTD layout according to its content.
> 

Nice to see another user of the smem code :)

> diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
[..]
> +
> +/* Processor/host identifier for the application processor */
> +#define SMEM_HOST_APPS			0

This makes me wonder if there will ever be an apps partition. I would
have picked QCOM_SMEM_HOST_ANY, but I don't know what future platforms
might hold for us.


And as a side note, I think I will propose that we rename that define
QCOM_SMEM_GLOBAL.

> +
> +/* SMEM items index */
> +#define SMEM_AARM_PARTITION_TABLE	9
> +#define SMEM_BOOT_FLASH_TYPE		421
> +#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
> +
> +/* SMEM Flash types */
> +#define SMEM_FLASH_NAND			2
> +#define SMEM_FLASH_SPI			6
> +
> +#define SMEM_PART_NAME_SZ		16
> +#define SMEM_PARTS_MAX			32
> +
> +struct smem_partition {
> +	char name[SMEM_PART_NAME_SZ];
> +	__le32 start;
> +	__le32 size;
> +	__le32 attr;
> +};
> +
> +struct smem_partition_table {
> +	u8 magic[8];
> +	__le32 version;
> +	__le32 len;
> +	struct smem_partition parts[SMEM_PARTS_MAX];
> +};
> +
> +/* SMEM Magic values in partition table */
> +static const u8 SMEM_PTABLE_MAGIC[] = {
> +	0xaa, 0x73, 0xee, 0x55,
> +	0xdb, 0xbd, 0x5e, 0xe3,
> +};
> +
> +static int qcom_smem_get_flash_blksz(u64 **smem_blksz)

Instead of passing pointers around I think you should just make this
function return < 0 for errors and >= 0 for the requested data. You can
probably still have it as an int, as the current values are well below
the size of the int.

> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
> +			    (void **) smem_blksz, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash blksz from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_blksz)) {
> +		pr_err("Invalid flash blksz size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_type(u64 **smem_flash_type)

Same as above.

> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
> +			    (void **) smem_flash_type, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash type from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_flash_type)) {
> +		pr_err("Invalid flash type size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
> +			    (void **) pparts, &size);
> +

If you don't care about the size just pass NULL as the last argument.

> +	if (ret < 0) {
> +		pr_err("Unable to read partition table from SMEM\n");
> +		return -ENOENT;
> +	}

I think it would be cleaner if you had this function check the magic and
version and then return the partition pointer. That is:

static smem_partition *qcom_smem_get_flash_partitions(size_t *count)


And make *count = le32_to_cpu(table->len); so you don't have to care
about that below.

(And use NULL or ERR_PTR for the error paths)

> +
> +	return 0;
> +}
> +
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +static bool is_spi_device(struct device_node *np)
> +{
> +	struct device *dev;
> +
> +	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
> +	if (!dev)
> +		return false;
> +
> +	put_device(dev);
> +	return true;
> +}
> +
> +static int parse_qcom_smem_partitions(struct mtd_info *master,
> +				      struct mtd_partition **pparts,
> +				      struct mtd_part_parser_data *data)
> +{
> +	struct smem_partition_table *smem_parts;
> +	u64 *smem_flash_type, *smem_blksz;
> +	struct mtd_partition *mtd_parts;
> +	struct device_node *of_node = data->of_node;
> +	int i, ret;
> +
> +	/*
> +	 * SMEM will only store the partition table of the boot device.
> +	 * If this is not the boot device, do not return any partition.
> +	 */

I guess this is the result of below checks, but it's not describing
what's actually done here. I think it should say "only parse
partitions on the specified memory type" or something like that.

> +	ret = qcom_smem_get_flash_type(&smem_flash_type);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
> +	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
> +		return 0;
> +
> +	/*
> +	 * Just for sanity purpose, make sure the block size in SMEM matches the
> +	 * block size of the MTD device
> +	 */

Well, this is not only for sanity purpose, as you multiply all sizes
with this number below...

> +	ret = qcom_smem_get_flash_blksz(&smem_blksz);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (*smem_blksz != master->erasesize) {
> +		pr_err("SMEM block size differs from MTD block size\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Get partition pointer from SMEM */
> +	ret = qcom_smem_get_flash_partitions(&smem_parts);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
> +		   sizeof(SMEM_PTABLE_MAGIC))) {
> +		pr_err("SMEM partition magic invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Allocate and populate the mtd structures */
> +	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
> +			    GFP_KERNEL);
> +	if (!mtd_parts)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < smem_parts->len; i++) {

This might have the wrong endian...

> +		struct smem_partition *s_part = &smem_parts->parts[i];
> +		struct mtd_partition *m_part = &mtd_parts[i];

I think Stephens suggestion was that you assign *pparts before looping
and then you simply do:

*pparts = mtd_parts;

while (len--) {
	..

	s_part++;
	mtd_parts++;
}

> +
> +		m_part->name = s_part->name;

I tried to follow the mtd code, and I believe that this pointer might
be freed in free_partition(). If so you must kstrdup it here.

> +		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
> +		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
> +
> +		/*
> +		 * The last SMEM partition may have its size marked as
> +		 * something like 0xffffffff, which means "until the end of the
> +		 * flash device". In this case, truncate it.
> +		 */
> +		if (m_part->offset + m_part->size > master->size)
> +			m_part->size = master->size - m_part->offset;
> +	}
> +
> +	*pparts = mtd_parts;
> +
> +	return smem_parts->len;

Again, this might be in the wrong endian.

> +}
> +
> +static struct mtd_part_parser qcom_smem_parser = {
> +	.owner = THIS_MODULE,
> +	.parse_fn = parse_qcom_smem_partitions,
> +	.name = "qcom-smem",
> +};
> +
> +static int __init qcom_smem_parser_init(void)
> +{
> +	register_mtd_parser(&qcom_smem_parser);
> +	return 0;
> +}
> +
> +static void __exit qcom_smem_parser_exit(void)
> +{
> +	deregister_mtd_parser(&qcom_smem_parser);
> +}
> +
> +module_init(qcom_smem_parser_init);
> +module_exit(qcom_smem_parser_exit);

Mostly unrelated to this patch; This makes me appreciate the
module_platform_driver() macro, would be nice to see a patch that
introduces something similar for partition parsers.

> +
> +MODULE_LICENSE("GPL");

Are you sure you don't mean "GPL v2" here?

> +MODULE_AUTHOR("Mathieu Olivari <mathieu@codeaurora.org>");
> +MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");

Regards,
Bjorn

Re: [PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Bjorn Andersson]

On Fri 14 Aug 17:46 PDT 2015, Mathieu Olivari wrote:

> On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
> used to store partition layout. This new parser can now be used to read
> SMEM and use it to register an MTD layout according to its content.
> 

Nice to see another user of the smem code :)

> diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
[..]
> +
> +/* Processor/host identifier for the application processor */
> +#define SMEM_HOST_APPS			0

This makes me wonder if there will ever be an apps partition. I would
have picked QCOM_SMEM_HOST_ANY, but I don't know what future platforms
might hold for us.


And as a side note, I think I will propose that we rename that define
QCOM_SMEM_GLOBAL.

> +
> +/* SMEM items index */
> +#define SMEM_AARM_PARTITION_TABLE	9
> +#define SMEM_BOOT_FLASH_TYPE		421
> +#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
> +
> +/* SMEM Flash types */
> +#define SMEM_FLASH_NAND			2
> +#define SMEM_FLASH_SPI			6
> +
> +#define SMEM_PART_NAME_SZ		16
> +#define SMEM_PARTS_MAX			32
> +
> +struct smem_partition {
> +	char name[SMEM_PART_NAME_SZ];
> +	__le32 start;
> +	__le32 size;
> +	__le32 attr;
> +};
> +
> +struct smem_partition_table {
> +	u8 magic[8];
> +	__le32 version;
> +	__le32 len;
> +	struct smem_partition parts[SMEM_PARTS_MAX];
> +};
> +
> +/* SMEM Magic values in partition table */
> +static const u8 SMEM_PTABLE_MAGIC[] = {
> +	0xaa, 0x73, 0xee, 0x55,
> +	0xdb, 0xbd, 0x5e, 0xe3,
> +};
> +
> +static int qcom_smem_get_flash_blksz(u64 **smem_blksz)

Instead of passing pointers around I think you should just make this
function return < 0 for errors and >= 0 for the requested data. You can
probably still have it as an int, as the current values are well below
the size of the int.

> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
> +			    (void **) smem_blksz, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash blksz from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_blksz)) {
> +		pr_err("Invalid flash blksz size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_type(u64 **smem_flash_type)

Same as above.

> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
> +			    (void **) smem_flash_type, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash type from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_flash_type)) {
> +		pr_err("Invalid flash type size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
> +			    (void **) pparts, &size);
> +

If you don't care about the size just pass NULL as the last argument.

> +	if (ret < 0) {
> +		pr_err("Unable to read partition table from SMEM\n");
> +		return -ENOENT;
> +	}

I think it would be cleaner if you had this function check the magic and
version and then return the partition pointer. That is:

static smem_partition *qcom_smem_get_flash_partitions(size_t *count)


And make *count = le32_to_cpu(table->len); so you don't have to care
about that below.

(And use NULL or ERR_PTR for the error paths)

> +
> +	return 0;
> +}
> +
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +static bool is_spi_device(struct device_node *np)
> +{
> +	struct device *dev;
> +
> +	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
> +	if (!dev)
> +		return false;
> +
> +	put_device(dev);
> +	return true;
> +}
> +
> +static int parse_qcom_smem_partitions(struct mtd_info *master,
> +				      struct mtd_partition **pparts,
> +				      struct mtd_part_parser_data *data)
> +{
> +	struct smem_partition_table *smem_parts;
> +	u64 *smem_flash_type, *smem_blksz;
> +	struct mtd_partition *mtd_parts;
> +	struct device_node *of_node = data->of_node;
> +	int i, ret;
> +
> +	/*
> +	 * SMEM will only store the partition table of the boot device.
> +	 * If this is not the boot device, do not return any partition.
> +	 */

I guess this is the result of below checks, but it's not describing
what's actually done here. I think it should say "only parse
partitions on the specified memory type" or something like that.

> +	ret = qcom_smem_get_flash_type(&smem_flash_type);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
> +	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
> +		return 0;
> +
> +	/*
> +	 * Just for sanity purpose, make sure the block size in SMEM matches the
> +	 * block size of the MTD device
> +	 */

Well, this is not only for sanity purpose, as you multiply all sizes
with this number below...

> +	ret = qcom_smem_get_flash_blksz(&smem_blksz);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (*smem_blksz != master->erasesize) {
> +		pr_err("SMEM block size differs from MTD block size\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Get partition pointer from SMEM */
> +	ret = qcom_smem_get_flash_partitions(&smem_parts);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
> +		   sizeof(SMEM_PTABLE_MAGIC))) {
> +		pr_err("SMEM partition magic invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Allocate and populate the mtd structures */
> +	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
> +			    GFP_KERNEL);
> +	if (!mtd_parts)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < smem_parts->len; i++) {

This might have the wrong endian...

> +		struct smem_partition *s_part = &smem_parts->parts[i];
> +		struct mtd_partition *m_part = &mtd_parts[i];

I think Stephens suggestion was that you assign *pparts before looping
and then you simply do:

*pparts = mtd_parts;

while (len--) {
	..

	s_part++;
	mtd_parts++;
}

> +
> +		m_part->name = s_part->name;

I tried to follow the mtd code, and I believe that this pointer might
be freed in free_partition(). If so you must kstrdup it here.

> +		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
> +		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
> +
> +		/*
> +		 * The last SMEM partition may have its size marked as
> +		 * something like 0xffffffff, which means "until the end of the
> +		 * flash device". In this case, truncate it.
> +		 */
> +		if (m_part->offset + m_part->size > master->size)
> +			m_part->size = master->size - m_part->offset;
> +	}
> +
> +	*pparts = mtd_parts;
> +
> +	return smem_parts->len;

Again, this might be in the wrong endian.

> +}
> +
> +static struct mtd_part_parser qcom_smem_parser = {
> +	.owner = THIS_MODULE,
> +	.parse_fn = parse_qcom_smem_partitions,
> +	.name = "qcom-smem",
> +};
> +
> +static int __init qcom_smem_parser_init(void)
> +{
> +	register_mtd_parser(&qcom_smem_parser);
> +	return 0;
> +}
> +
> +static void __exit qcom_smem_parser_exit(void)
> +{
> +	deregister_mtd_parser(&qcom_smem_parser);
> +}
> +
> +module_init(qcom_smem_parser_init);
> +module_exit(qcom_smem_parser_exit);

Mostly unrelated to this patch; This makes me appreciate the
module_platform_driver() macro, would be nice to see a patch that
introduces something similar for partition parsers.

> +
> +MODULE_LICENSE("GPL");

Are you sure you don't mean "GPL v2" here?

> +MODULE_AUTHOR("Mathieu Olivari <mathieu-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>");
> +MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Bjorn Andersson]

On Fri 14 Aug 17:46 PDT 2015, Mathieu Olivari wrote:

> On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
> used to store partition layout. This new parser can now be used to read
> SMEM and use it to register an MTD layout according to its content.
> 

Nice to see another user of the smem code :)

> diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
[..]
> +
> +/* Processor/host identifier for the application processor */
> +#define SMEM_HOST_APPS			0

This makes me wonder if there will ever be an apps partition. I would
have picked QCOM_SMEM_HOST_ANY, but I don't know what future platforms
might hold for us.


And as a side note, I think I will propose that we rename that define
QCOM_SMEM_GLOBAL.

> +
> +/* SMEM items index */
> +#define SMEM_AARM_PARTITION_TABLE	9
> +#define SMEM_BOOT_FLASH_TYPE		421
> +#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
> +
> +/* SMEM Flash types */
> +#define SMEM_FLASH_NAND			2
> +#define SMEM_FLASH_SPI			6
> +
> +#define SMEM_PART_NAME_SZ		16
> +#define SMEM_PARTS_MAX			32
> +
> +struct smem_partition {
> +	char name[SMEM_PART_NAME_SZ];
> +	__le32 start;
> +	__le32 size;
> +	__le32 attr;
> +};
> +
> +struct smem_partition_table {
> +	u8 magic[8];
> +	__le32 version;
> +	__le32 len;
> +	struct smem_partition parts[SMEM_PARTS_MAX];
> +};
> +
> +/* SMEM Magic values in partition table */
> +static const u8 SMEM_PTABLE_MAGIC[] = {
> +	0xaa, 0x73, 0xee, 0x55,
> +	0xdb, 0xbd, 0x5e, 0xe3,
> +};
> +
> +static int qcom_smem_get_flash_blksz(u64 **smem_blksz)

Instead of passing pointers around I think you should just make this
function return < 0 for errors and >= 0 for the requested data. You can
probably still have it as an int, as the current values are well below
the size of the int.

> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
> +			    (void **) smem_blksz, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash blksz from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_blksz)) {
> +		pr_err("Invalid flash blksz size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_type(u64 **smem_flash_type)

Same as above.

> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
> +			    (void **) smem_flash_type, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash type from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_flash_type)) {
> +		pr_err("Invalid flash type size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
> +			    (void **) pparts, &size);
> +

If you don't care about the size just pass NULL as the last argument.

> +	if (ret < 0) {
> +		pr_err("Unable to read partition table from SMEM\n");
> +		return -ENOENT;
> +	}

I think it would be cleaner if you had this function check the magic and
version and then return the partition pointer. That is:

static smem_partition *qcom_smem_get_flash_partitions(size_t *count)


And make *count = le32_to_cpu(table->len); so you don't have to care
about that below.

(And use NULL or ERR_PTR for the error paths)

> +
> +	return 0;
> +}
> +
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +static bool is_spi_device(struct device_node *np)
> +{
> +	struct device *dev;
> +
> +	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
> +	if (!dev)
> +		return false;
> +
> +	put_device(dev);
> +	return true;
> +}
> +
> +static int parse_qcom_smem_partitions(struct mtd_info *master,
> +				      struct mtd_partition **pparts,
> +				      struct mtd_part_parser_data *data)
> +{
> +	struct smem_partition_table *smem_parts;
> +	u64 *smem_flash_type, *smem_blksz;
> +	struct mtd_partition *mtd_parts;
> +	struct device_node *of_node = data->of_node;
> +	int i, ret;
> +
> +	/*
> +	 * SMEM will only store the partition table of the boot device.
> +	 * If this is not the boot device, do not return any partition.
> +	 */

I guess this is the result of below checks, but it's not describing
what's actually done here. I think it should say "only parse
partitions on the specified memory type" or something like that.

> +	ret = qcom_smem_get_flash_type(&smem_flash_type);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
> +	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
> +		return 0;
> +
> +	/*
> +	 * Just for sanity purpose, make sure the block size in SMEM matches the
> +	 * block size of the MTD device
> +	 */

Well, this is not only for sanity purpose, as you multiply all sizes
with this number below...

> +	ret = qcom_smem_get_flash_blksz(&smem_blksz);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (*smem_blksz != master->erasesize) {
> +		pr_err("SMEM block size differs from MTD block size\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Get partition pointer from SMEM */
> +	ret = qcom_smem_get_flash_partitions(&smem_parts);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
> +		   sizeof(SMEM_PTABLE_MAGIC))) {
> +		pr_err("SMEM partition magic invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Allocate and populate the mtd structures */
> +	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
> +			    GFP_KERNEL);
> +	if (!mtd_parts)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < smem_parts->len; i++) {

This might have the wrong endian...

> +		struct smem_partition *s_part = &smem_parts->parts[i];
> +		struct mtd_partition *m_part = &mtd_parts[i];

I think Stephens suggestion was that you assign *pparts before looping
and then you simply do:

*pparts = mtd_parts;

while (len--) {
	..

	s_part++;
	mtd_parts++;
}

> +
> +		m_part->name = s_part->name;

I tried to follow the mtd code, and I believe that this pointer might
be freed in free_partition(). If so you must kstrdup it here.

> +		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
> +		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
> +
> +		/*
> +		 * The last SMEM partition may have its size marked as
> +		 * something like 0xffffffff, which means "until the end of the
> +		 * flash device". In this case, truncate it.
> +		 */
> +		if (m_part->offset + m_part->size > master->size)
> +			m_part->size = master->size - m_part->offset;
> +	}
> +
> +	*pparts = mtd_parts;
> +
> +	return smem_parts->len;

Again, this might be in the wrong endian.

> +}
> +
> +static struct mtd_part_parser qcom_smem_parser = {
> +	.owner = THIS_MODULE,
> +	.parse_fn = parse_qcom_smem_partitions,
> +	.name = "qcom-smem",
> +};
> +
> +static int __init qcom_smem_parser_init(void)
> +{
> +	register_mtd_parser(&qcom_smem_parser);
> +	return 0;
> +}
> +
> +static void __exit qcom_smem_parser_exit(void)
> +{
> +	deregister_mtd_parser(&qcom_smem_parser);
> +}
> +
> +module_init(qcom_smem_parser_init);
> +module_exit(qcom_smem_parser_exit);

Mostly unrelated to this patch; This makes me appreciate the
module_platform_driver() macro, would be nice to see a patch that
introduces something similar for partition parsers.

> +
> +MODULE_LICENSE("GPL");

Are you sure you don't mean "GPL v2" here?

> +MODULE_AUTHOR("Mathieu Olivari <mathieu@codeaurora.org>");
> +MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");

Regards,
Bjorn

[PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Bjorn Andersson]

On Fri 14 Aug 17:46 PDT 2015, Mathieu Olivari wrote:

> On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
> used to store partition layout. This new parser can now be used to read
> SMEM and use it to register an MTD layout according to its content.
> 

Nice to see another user of the smem code :)

> diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
[..]
> +
> +/* Processor/host identifier for the application processor */
> +#define SMEM_HOST_APPS			0

This makes me wonder if there will ever be an apps partition. I would
have picked QCOM_SMEM_HOST_ANY, but I don't know what future platforms
might hold for us.


And as a side note, I think I will propose that we rename that define
QCOM_SMEM_GLOBAL.

> +
> +/* SMEM items index */
> +#define SMEM_AARM_PARTITION_TABLE	9
> +#define SMEM_BOOT_FLASH_TYPE		421
> +#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
> +
> +/* SMEM Flash types */
> +#define SMEM_FLASH_NAND			2
> +#define SMEM_FLASH_SPI			6
> +
> +#define SMEM_PART_NAME_SZ		16
> +#define SMEM_PARTS_MAX			32
> +
> +struct smem_partition {
> +	char name[SMEM_PART_NAME_SZ];
> +	__le32 start;
> +	__le32 size;
> +	__le32 attr;
> +};
> +
> +struct smem_partition_table {
> +	u8 magic[8];
> +	__le32 version;
> +	__le32 len;
> +	struct smem_partition parts[SMEM_PARTS_MAX];
> +};
> +
> +/* SMEM Magic values in partition table */
> +static const u8 SMEM_PTABLE_MAGIC[] = {
> +	0xaa, 0x73, 0xee, 0x55,
> +	0xdb, 0xbd, 0x5e, 0xe3,
> +};
> +
> +static int qcom_smem_get_flash_blksz(u64 **smem_blksz)

Instead of passing pointers around I think you should just make this
function return < 0 for errors and >= 0 for the requested data. You can
probably still have it as an int, as the current values are well below
the size of the int.

> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
> +			    (void **) smem_blksz, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash blksz from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_blksz)) {
> +		pr_err("Invalid flash blksz size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_type(u64 **smem_flash_type)

Same as above.

> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
> +			    (void **) smem_flash_type, &size);
> +
> +	if (ret < 0) {
> +		pr_err("Unable to read flash type from SMEM\n");
> +		return -ENOENT;
> +	}
> +
> +	if (size != sizeof(**smem_flash_type)) {
> +		pr_err("Invalid flash type size in SMEM\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
> +{
> +	int ret;
> +	size_t size;
> +
> +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
> +			    (void **) pparts, &size);
> +

If you don't care about the size just pass NULL as the last argument.

> +	if (ret < 0) {
> +		pr_err("Unable to read partition table from SMEM\n");
> +		return -ENOENT;
> +	}

I think it would be cleaner if you had this function check the magic and
version and then return the partition pointer. That is:

static smem_partition *qcom_smem_get_flash_partitions(size_t *count)


And make *count = le32_to_cpu(table->len); so you don't have to care
about that below.

(And use NULL or ERR_PTR for the error paths)

> +
> +	return 0;
> +}
> +
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> +	return dev->of_node == data;
> +}
> +
> +static bool is_spi_device(struct device_node *np)
> +{
> +	struct device *dev;
> +
> +	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
> +	if (!dev)
> +		return false;
> +
> +	put_device(dev);
> +	return true;
> +}
> +
> +static int parse_qcom_smem_partitions(struct mtd_info *master,
> +				      struct mtd_partition **pparts,
> +				      struct mtd_part_parser_data *data)
> +{
> +	struct smem_partition_table *smem_parts;
> +	u64 *smem_flash_type, *smem_blksz;
> +	struct mtd_partition *mtd_parts;
> +	struct device_node *of_node = data->of_node;
> +	int i, ret;
> +
> +	/*
> +	 * SMEM will only store the partition table of the boot device.
> +	 * If this is not the boot device, do not return any partition.
> +	 */

I guess this is the result of below checks, but it's not describing
what's actually done here. I think it should say "only parse
partitions on the specified memory type" or something like that.

> +	ret = qcom_smem_get_flash_type(&smem_flash_type);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
> +	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
> +		return 0;
> +
> +	/*
> +	 * Just for sanity purpose, make sure the block size in SMEM matches the
> +	 * block size of the MTD device
> +	 */

Well, this is not only for sanity purpose, as you multiply all sizes
with this number below...

> +	ret = qcom_smem_get_flash_blksz(&smem_blksz);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (*smem_blksz != master->erasesize) {
> +		pr_err("SMEM block size differs from MTD block size\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Get partition pointer from SMEM */
> +	ret = qcom_smem_get_flash_partitions(&smem_parts);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
> +		   sizeof(SMEM_PTABLE_MAGIC))) {
> +		pr_err("SMEM partition magic invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Allocate and populate the mtd structures */
> +	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
> +			    GFP_KERNEL);
> +	if (!mtd_parts)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < smem_parts->len; i++) {

This might have the wrong endian...

> +		struct smem_partition *s_part = &smem_parts->parts[i];
> +		struct mtd_partition *m_part = &mtd_parts[i];

I think Stephens suggestion was that you assign *pparts before looping
and then you simply do:

*pparts = mtd_parts;

while (len--) {
	..

	s_part++;
	mtd_parts++;
}

> +
> +		m_part->name = s_part->name;

I tried to follow the mtd code, and I believe that this pointer might
be freed in free_partition(). If so you must kstrdup it here.

> +		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
> +		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
> +
> +		/*
> +		 * The last SMEM partition may have its size marked as
> +		 * something like 0xffffffff, which means "until the end of the
> +		 * flash device". In this case, truncate it.
> +		 */
> +		if (m_part->offset + m_part->size > master->size)
> +			m_part->size = master->size - m_part->offset;
> +	}
> +
> +	*pparts = mtd_parts;
> +
> +	return smem_parts->len;

Again, this might be in the wrong endian.

> +}
> +
> +static struct mtd_part_parser qcom_smem_parser = {
> +	.owner = THIS_MODULE,
> +	.parse_fn = parse_qcom_smem_partitions,
> +	.name = "qcom-smem",
> +};
> +
> +static int __init qcom_smem_parser_init(void)
> +{
> +	register_mtd_parser(&qcom_smem_parser);
> +	return 0;
> +}
> +
> +static void __exit qcom_smem_parser_exit(void)
> +{
> +	deregister_mtd_parser(&qcom_smem_parser);
> +}
> +
> +module_init(qcom_smem_parser_init);
> +module_exit(qcom_smem_parser_exit);

Mostly unrelated to this patch; This makes me appreciate the
module_platform_driver() macro, would be nice to see a patch that
introduces something similar for partition parsers.

> +
> +MODULE_LICENSE("GPL");

Are you sure you don't mean "GPL v2" here?

> +MODULE_AUTHOR("Mathieu Olivari <mathieu@codeaurora.org>");
> +MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");

Regards,
Bjorn

Re: [PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Mathieu Olivari]

On Fri, Aug 14, 2015 at 10:48:17PM -0700, Bjorn Andersson wrote:
> On Fri 14 Aug 17:46 PDT 2015, Mathieu Olivari wrote:
> 
> > On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
> > used to store partition layout. This new parser can now be used to read
> > SMEM and use it to register an MTD layout according to its content.
> > 
> 
> Nice to see another user of the smem code :)
> 
> > diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
> [..]
> > +
> > +/* Processor/host identifier for the application processor */
> > +#define SMEM_HOST_APPS			0
> 
> This makes me wonder if there will ever be an apps partition. I would
> have picked QCOM_SMEM_HOST_ANY, but I don't know what future platforms
> might hold for us.
> 
> 
> And as a side note, I think I will propose that we rename that define
> QCOM_SMEM_GLOBAL.
OK; I'll rename it in QCOM_SMEM_GLOBAL then to be in sync.
> 
> > +
> > +/* SMEM items index */
> > +#define SMEM_AARM_PARTITION_TABLE	9
> > +#define SMEM_BOOT_FLASH_TYPE		421
> > +#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
> > +
> > +/* SMEM Flash types */
> > +#define SMEM_FLASH_NAND			2
> > +#define SMEM_FLASH_SPI			6
> > +
> > +#define SMEM_PART_NAME_SZ		16
> > +#define SMEM_PARTS_MAX			32
> > +
> > +struct smem_partition {
> > +	char name[SMEM_PART_NAME_SZ];
> > +	__le32 start;
> > +	__le32 size;
> > +	__le32 attr;
> > +};
> > +
> > +struct smem_partition_table {
> > +	u8 magic[8];
> > +	__le32 version;
> > +	__le32 len;
> > +	struct smem_partition parts[SMEM_PARTS_MAX];
> > +};
> > +
> > +/* SMEM Magic values in partition table */
> > +static const u8 SMEM_PTABLE_MAGIC[] = {
> > +	0xaa, 0x73, 0xee, 0x55,
> > +	0xdb, 0xbd, 0x5e, 0xe3,
> > +};
> > +
> > +static int qcom_smem_get_flash_blksz(u64 **smem_blksz)
> 
> Instead of passing pointers around I think you should just make this
> function return < 0 for errors and >= 0 for the requested data. You can
> probably still have it as an int, as the current values are well below
> the size of the int.
Good point. Thanks. I'll report it.
> 
> > +{
> > +	int ret;
> > +	size_t size;
> > +
> > +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
> > +			    (void **) smem_blksz, &size);
> > +
> > +	if (ret < 0) {
> > +		pr_err("Unable to read flash blksz from SMEM\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	if (size != sizeof(**smem_blksz)) {
> > +		pr_err("Invalid flash blksz size in SMEM\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_smem_get_flash_type(u64 **smem_flash_type)
> 
> Same as above.
OK.
> 
> > +{
> > +	int ret;
> > +	size_t size;
> > +
> > +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
> > +			    (void **) smem_flash_type, &size);
> > +
> > +	if (ret < 0) {
> > +		pr_err("Unable to read flash type from SMEM\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	if (size != sizeof(**smem_flash_type)) {
> > +		pr_err("Invalid flash type size in SMEM\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
> > +{
> > +	int ret;
> > +	size_t size;
> > +
> > +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
> > +			    (void **) pparts, &size);
> > +
> 
> If you don't care about the size just pass NULL as the last argument.
OK.
> 
> > +	if (ret < 0) {
> > +		pr_err("Unable to read partition table from SMEM\n");
> > +		return -ENOENT;
> > +	}
> 
> I think it would be cleaner if you had this function check the magic and
> version and then return the partition pointer. That is:
> 
> static smem_partition *qcom_smem_get_flash_partitions(size_t *count)
> 
> 
> And make *count = le32_to_cpu(table->len); so you don't have to care
> about that below.
> 
> (And use NULL or ERR_PTR for the error paths)
Same; good idea. I'll make the mod.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int of_dev_node_match(struct device *dev, void *data)
> > +{
> > +	return dev->of_node == data;
> > +}
> > +
> > +static bool is_spi_device(struct device_node *np)
> > +{
> > +	struct device *dev;
> > +
> > +	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
> > +	if (!dev)
> > +		return false;
> > +
> > +	put_device(dev);
> > +	return true;
> > +}
> > +
> > +static int parse_qcom_smem_partitions(struct mtd_info *master,
> > +				      struct mtd_partition **pparts,
> > +				      struct mtd_part_parser_data *data)
> > +{
> > +	struct smem_partition_table *smem_parts;
> > +	u64 *smem_flash_type, *smem_blksz;
> > +	struct mtd_partition *mtd_parts;
> > +	struct device_node *of_node = data->of_node;
> > +	int i, ret;
> > +
> > +	/*
> > +	 * SMEM will only store the partition table of the boot device.
> > +	 * If this is not the boot device, do not return any partition.
> > +	 */
> 
> I guess this is the result of below checks, but it's not describing
> what's actually done here. I think it should say "only parse
> partitions on the specified memory type" or something like that.
OK.
> 
> > +	ret = qcom_smem_get_flash_type(&smem_flash_type);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
> > +	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
> > +		return 0;
> > +
> > +	/*
> > +	 * Just for sanity purpose, make sure the block size in SMEM matches the
> > +	 * block size of the MTD device
> > +	 */
> 
> Well, this is not only for sanity purpose, as you multiply all sizes
> with this number below...
I'll update the comment.
> 
> > +	ret = qcom_smem_get_flash_blksz(&smem_blksz);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (*smem_blksz != master->erasesize) {
> > +		pr_err("SMEM block size differs from MTD block size\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Get partition pointer from SMEM */
> > +	ret = qcom_smem_get_flash_partitions(&smem_parts);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
> > +		   sizeof(SMEM_PTABLE_MAGIC))) {
> > +		pr_err("SMEM partition magic invalid\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Allocate and populate the mtd structures */
> > +	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
> > +			    GFP_KERNEL);
> > +	if (!mtd_parts)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < smem_parts->len; i++) {
> 
> This might have the wrong endian...
Right. I'll do a le32_to_cpu()
> 
> > +		struct smem_partition *s_part = &smem_parts->parts[i];
> > +		struct mtd_partition *m_part = &mtd_parts[i];
> 
> I think Stephens suggestion was that you assign *pparts before looping
> and then you simply do:
> 
> *pparts = mtd_parts;
> 
> while (len--) {
> 	..
> 
> 	s_part++;
> 	mtd_parts++;
> }
> 
> > +
> > +		m_part->name = s_part->name;
> 
> I tried to follow the mtd code, and I believe that this pointer might
> be freed in free_partition(). If so you must kstrdup it here.
OK.
> 
> > +		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
> > +		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
> > +
> > +		/*
> > +		 * The last SMEM partition may have its size marked as
> > +		 * something like 0xffffffff, which means "until the end of the
> > +		 * flash device". In this case, truncate it.
> > +		 */
> > +		if (m_part->offset + m_part->size > master->size)
> > +			m_part->size = master->size - m_part->offset;
> > +	}
> > +
> > +	*pparts = mtd_parts;
> > +
> > +	return smem_parts->len;
> 
> Again, this might be in the wrong endian.
Yes.
> 
> > +}
> > +
> > +static struct mtd_part_parser qcom_smem_parser = {
> > +	.owner = THIS_MODULE,
> > +	.parse_fn = parse_qcom_smem_partitions,
> > +	.name = "qcom-smem",
> > +};
> > +
> > +static int __init qcom_smem_parser_init(void)
> > +{
> > +	register_mtd_parser(&qcom_smem_parser);
> > +	return 0;
> > +}
> > +
> > +static void __exit qcom_smem_parser_exit(void)
> > +{
> > +	deregister_mtd_parser(&qcom_smem_parser);
> > +}
> > +
> > +module_init(qcom_smem_parser_init);
> > +module_exit(qcom_smem_parser_exit);
> 
> Mostly unrelated to this patch; This makes me appreciate the
> module_platform_driver() macro, would be nice to see a patch that
> introduces something similar for partition parsers.
> 
> > +
> > +MODULE_LICENSE("GPL");
> 
> Are you sure you don't mean "GPL v2" here?
I'll update it.
> 
> > +MODULE_AUTHOR("Mathieu Olivari <mathieu@codeaurora.org>");
> > +MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");
> 
> Regards,
> Bjorn

Re: [PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Mathieu Olivari]

On Fri, Aug 14, 2015 at 10:48:17PM -0700, Bjorn Andersson wrote:
> On Fri 14 Aug 17:46 PDT 2015, Mathieu Olivari wrote:
> 
> > On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
> > used to store partition layout. This new parser can now be used to read
> > SMEM and use it to register an MTD layout according to its content.
> > 
> 
> Nice to see another user of the smem code :)
> 
> > diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
> [..]
> > +
> > +/* Processor/host identifier for the application processor */
> > +#define SMEM_HOST_APPS			0
> 
> This makes me wonder if there will ever be an apps partition. I would
> have picked QCOM_SMEM_HOST_ANY, but I don't know what future platforms
> might hold for us.
> 
> 
> And as a side note, I think I will propose that we rename that define
> QCOM_SMEM_GLOBAL.
OK; I'll rename it in QCOM_SMEM_GLOBAL then to be in sync.
> 
> > +
> > +/* SMEM items index */
> > +#define SMEM_AARM_PARTITION_TABLE	9
> > +#define SMEM_BOOT_FLASH_TYPE		421
> > +#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
> > +
> > +/* SMEM Flash types */
> > +#define SMEM_FLASH_NAND			2
> > +#define SMEM_FLASH_SPI			6
> > +
> > +#define SMEM_PART_NAME_SZ		16
> > +#define SMEM_PARTS_MAX			32
> > +
> > +struct smem_partition {
> > +	char name[SMEM_PART_NAME_SZ];
> > +	__le32 start;
> > +	__le32 size;
> > +	__le32 attr;
> > +};
> > +
> > +struct smem_partition_table {
> > +	u8 magic[8];
> > +	__le32 version;
> > +	__le32 len;
> > +	struct smem_partition parts[SMEM_PARTS_MAX];
> > +};
> > +
> > +/* SMEM Magic values in partition table */
> > +static const u8 SMEM_PTABLE_MAGIC[] = {
> > +	0xaa, 0x73, 0xee, 0x55,
> > +	0xdb, 0xbd, 0x5e, 0xe3,
> > +};
> > +
> > +static int qcom_smem_get_flash_blksz(u64 **smem_blksz)
> 
> Instead of passing pointers around I think you should just make this
> function return < 0 for errors and >= 0 for the requested data. You can
> probably still have it as an int, as the current values are well below
> the size of the int.
Good point. Thanks. I'll report it.
> 
> > +{
> > +	int ret;
> > +	size_t size;
> > +
> > +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
> > +			    (void **) smem_blksz, &size);
> > +
> > +	if (ret < 0) {
> > +		pr_err("Unable to read flash blksz from SMEM\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	if (size != sizeof(**smem_blksz)) {
> > +		pr_err("Invalid flash blksz size in SMEM\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_smem_get_flash_type(u64 **smem_flash_type)
> 
> Same as above.
OK.
> 
> > +{
> > +	int ret;
> > +	size_t size;
> > +
> > +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
> > +			    (void **) smem_flash_type, &size);
> > +
> > +	if (ret < 0) {
> > +		pr_err("Unable to read flash type from SMEM\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	if (size != sizeof(**smem_flash_type)) {
> > +		pr_err("Invalid flash type size in SMEM\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
> > +{
> > +	int ret;
> > +	size_t size;
> > +
> > +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
> > +			    (void **) pparts, &size);
> > +
> 
> If you don't care about the size just pass NULL as the last argument.
OK.
> 
> > +	if (ret < 0) {
> > +		pr_err("Unable to read partition table from SMEM\n");
> > +		return -ENOENT;
> > +	}
> 
> I think it would be cleaner if you had this function check the magic and
> version and then return the partition pointer. That is:
> 
> static smem_partition *qcom_smem_get_flash_partitions(size_t *count)
> 
> 
> And make *count = le32_to_cpu(table->len); so you don't have to care
> about that below.
> 
> (And use NULL or ERR_PTR for the error paths)
Same; good idea. I'll make the mod.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int of_dev_node_match(struct device *dev, void *data)
> > +{
> > +	return dev->of_node == data;
> > +}
> > +
> > +static bool is_spi_device(struct device_node *np)
> > +{
> > +	struct device *dev;
> > +
> > +	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
> > +	if (!dev)
> > +		return false;
> > +
> > +	put_device(dev);
> > +	return true;
> > +}
> > +
> > +static int parse_qcom_smem_partitions(struct mtd_info *master,
> > +				      struct mtd_partition **pparts,
> > +				      struct mtd_part_parser_data *data)
> > +{
> > +	struct smem_partition_table *smem_parts;
> > +	u64 *smem_flash_type, *smem_blksz;
> > +	struct mtd_partition *mtd_parts;
> > +	struct device_node *of_node = data->of_node;
> > +	int i, ret;
> > +
> > +	/*
> > +	 * SMEM will only store the partition table of the boot device.
> > +	 * If this is not the boot device, do not return any partition.
> > +	 */
> 
> I guess this is the result of below checks, but it's not describing
> what's actually done here. I think it should say "only parse
> partitions on the specified memory type" or something like that.
OK.
> 
> > +	ret = qcom_smem_get_flash_type(&smem_flash_type);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
> > +	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
> > +		return 0;
> > +
> > +	/*
> > +	 * Just for sanity purpose, make sure the block size in SMEM matches the
> > +	 * block size of the MTD device
> > +	 */
> 
> Well, this is not only for sanity purpose, as you multiply all sizes
> with this number below...
I'll update the comment.
> 
> > +	ret = qcom_smem_get_flash_blksz(&smem_blksz);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (*smem_blksz != master->erasesize) {
> > +		pr_err("SMEM block size differs from MTD block size\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Get partition pointer from SMEM */
> > +	ret = qcom_smem_get_flash_partitions(&smem_parts);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
> > +		   sizeof(SMEM_PTABLE_MAGIC))) {
> > +		pr_err("SMEM partition magic invalid\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Allocate and populate the mtd structures */
> > +	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
> > +			    GFP_KERNEL);
> > +	if (!mtd_parts)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < smem_parts->len; i++) {
> 
> This might have the wrong endian...
Right. I'll do a le32_to_cpu()
> 
> > +		struct smem_partition *s_part = &smem_parts->parts[i];
> > +		struct mtd_partition *m_part = &mtd_parts[i];
> 
> I think Stephens suggestion was that you assign *pparts before looping
> and then you simply do:
> 
> *pparts = mtd_parts;
> 
> while (len--) {
> 	..
> 
> 	s_part++;
> 	mtd_parts++;
> }
> 
> > +
> > +		m_part->name = s_part->name;
> 
> I tried to follow the mtd code, and I believe that this pointer might
> be freed in free_partition(). If so you must kstrdup it here.
OK.
> 
> > +		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
> > +		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
> > +
> > +		/*
> > +		 * The last SMEM partition may have its size marked as
> > +		 * something like 0xffffffff, which means "until the end of the
> > +		 * flash device". In this case, truncate it.
> > +		 */
> > +		if (m_part->offset + m_part->size > master->size)
> > +			m_part->size = master->size - m_part->offset;
> > +	}
> > +
> > +	*pparts = mtd_parts;
> > +
> > +	return smem_parts->len;
> 
> Again, this might be in the wrong endian.
Yes.
> 
> > +}
> > +
> > +static struct mtd_part_parser qcom_smem_parser = {
> > +	.owner = THIS_MODULE,
> > +	.parse_fn = parse_qcom_smem_partitions,
> > +	.name = "qcom-smem",
> > +};
> > +
> > +static int __init qcom_smem_parser_init(void)
> > +{
> > +	register_mtd_parser(&qcom_smem_parser);
> > +	return 0;
> > +}
> > +
> > +static void __exit qcom_smem_parser_exit(void)
> > +{
> > +	deregister_mtd_parser(&qcom_smem_parser);
> > +}
> > +
> > +module_init(qcom_smem_parser_init);
> > +module_exit(qcom_smem_parser_exit);
> 
> Mostly unrelated to this patch; This makes me appreciate the
> module_platform_driver() macro, would be nice to see a patch that
> introduces something similar for partition parsers.
> 
> > +
> > +MODULE_LICENSE("GPL");
> 
> Are you sure you don't mean "GPL v2" here?
I'll update it.
> 
> > +MODULE_AUTHOR("Mathieu Olivari <mathieu@codeaurora.org>");
> > +MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");
> 
> Regards,
> Bjorn

[PATCH 3/3] mtd: add SMEM parser for QCOM platforms

[Mathieu Olivari]

On Fri, Aug 14, 2015 at 10:48:17PM -0700, Bjorn Andersson wrote:
> On Fri 14 Aug 17:46 PDT 2015, Mathieu Olivari wrote:
> 
> > On QCOM platforms using MTD devices storage (such as IPQ806x), SMEM is
> > used to store partition layout. This new parser can now be used to read
> > SMEM and use it to register an MTD layout according to its content.
> > 
> 
> Nice to see another user of the smem code :)
> 
> > diff --git a/drivers/mtd/qcom_smem_part.c b/drivers/mtd/qcom_smem_part.c
> [..]
> > +
> > +/* Processor/host identifier for the application processor */
> > +#define SMEM_HOST_APPS			0
> 
> This makes me wonder if there will ever be an apps partition. I would
> have picked QCOM_SMEM_HOST_ANY, but I don't know what future platforms
> might hold for us.
> 
> 
> And as a side note, I think I will propose that we rename that define
> QCOM_SMEM_GLOBAL.
OK; I'll rename it in QCOM_SMEM_GLOBAL then to be in sync.
> 
> > +
> > +/* SMEM items index */
> > +#define SMEM_AARM_PARTITION_TABLE	9
> > +#define SMEM_BOOT_FLASH_TYPE		421
> > +#define SMEM_BOOT_FLASH_BLOCK_SIZE	424
> > +
> > +/* SMEM Flash types */
> > +#define SMEM_FLASH_NAND			2
> > +#define SMEM_FLASH_SPI			6
> > +
> > +#define SMEM_PART_NAME_SZ		16
> > +#define SMEM_PARTS_MAX			32
> > +
> > +struct smem_partition {
> > +	char name[SMEM_PART_NAME_SZ];
> > +	__le32 start;
> > +	__le32 size;
> > +	__le32 attr;
> > +};
> > +
> > +struct smem_partition_table {
> > +	u8 magic[8];
> > +	__le32 version;
> > +	__le32 len;
> > +	struct smem_partition parts[SMEM_PARTS_MAX];
> > +};
> > +
> > +/* SMEM Magic values in partition table */
> > +static const u8 SMEM_PTABLE_MAGIC[] = {
> > +	0xaa, 0x73, 0xee, 0x55,
> > +	0xdb, 0xbd, 0x5e, 0xe3,
> > +};
> > +
> > +static int qcom_smem_get_flash_blksz(u64 **smem_blksz)
> 
> Instead of passing pointers around I think you should just make this
> function return < 0 for errors and >= 0 for the requested data. You can
> probably still have it as an int, as the current values are well below
> the size of the int.
Good point. Thanks. I'll report it.
> 
> > +{
> > +	int ret;
> > +	size_t size;
> > +
> > +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_BLOCK_SIZE,
> > +			    (void **) smem_blksz, &size);
> > +
> > +	if (ret < 0) {
> > +		pr_err("Unable to read flash blksz from SMEM\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	if (size != sizeof(**smem_blksz)) {
> > +		pr_err("Invalid flash blksz size in SMEM\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_smem_get_flash_type(u64 **smem_flash_type)
> 
> Same as above.
OK.
> 
> > +{
> > +	int ret;
> > +	size_t size;
> > +
> > +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_BOOT_FLASH_TYPE,
> > +			    (void **) smem_flash_type, &size);
> > +
> > +	if (ret < 0) {
> > +		pr_err("Unable to read flash type from SMEM\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	if (size != sizeof(**smem_flash_type)) {
> > +		pr_err("Invalid flash type size in SMEM\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int qcom_smem_get_flash_partitions(struct smem_partition_table **pparts)
> > +{
> > +	int ret;
> > +	size_t size;
> > +
> > +	ret = qcom_smem_get(SMEM_HOST_APPS, SMEM_AARM_PARTITION_TABLE,
> > +			    (void **) pparts, &size);
> > +
> 
> If you don't care about the size just pass NULL as the last argument.
OK.
> 
> > +	if (ret < 0) {
> > +		pr_err("Unable to read partition table from SMEM\n");
> > +		return -ENOENT;
> > +	}
> 
> I think it would be cleaner if you had this function check the magic and
> version and then return the partition pointer. That is:
> 
> static smem_partition *qcom_smem_get_flash_partitions(size_t *count)
> 
> 
> And make *count = le32_to_cpu(table->len); so you don't have to care
> about that below.
> 
> (And use NULL or ERR_PTR for the error paths)
Same; good idea. I'll make the mod.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int of_dev_node_match(struct device *dev, void *data)
> > +{
> > +	return dev->of_node == data;
> > +}
> > +
> > +static bool is_spi_device(struct device_node *np)
> > +{
> > +	struct device *dev;
> > +
> > +	dev = bus_find_device(&spi_bus_type, NULL, np, of_dev_node_match);
> > +	if (!dev)
> > +		return false;
> > +
> > +	put_device(dev);
> > +	return true;
> > +}
> > +
> > +static int parse_qcom_smem_partitions(struct mtd_info *master,
> > +				      struct mtd_partition **pparts,
> > +				      struct mtd_part_parser_data *data)
> > +{
> > +	struct smem_partition_table *smem_parts;
> > +	u64 *smem_flash_type, *smem_blksz;
> > +	struct mtd_partition *mtd_parts;
> > +	struct device_node *of_node = data->of_node;
> > +	int i, ret;
> > +
> > +	/*
> > +	 * SMEM will only store the partition table of the boot device.
> > +	 * If this is not the boot device, do not return any partition.
> > +	 */
> 
> I guess this is the result of below checks, but it's not describing
> what's actually done here. I think it should say "only parse
> partitions on the specified memory type" or something like that.
OK.
> 
> > +	ret = qcom_smem_get_flash_type(&smem_flash_type);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if ((*smem_flash_type == SMEM_FLASH_NAND && !mtd_type_is_nand(master))
> > +	    || (*smem_flash_type == SMEM_FLASH_SPI && !is_spi_device(of_node)))
> > +		return 0;
> > +
> > +	/*
> > +	 * Just for sanity purpose, make sure the block size in SMEM matches the
> > +	 * block size of the MTD device
> > +	 */
> 
> Well, this is not only for sanity purpose, as you multiply all sizes
> with this number below...
I'll update the comment.
> 
> > +	ret = qcom_smem_get_flash_blksz(&smem_blksz);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (*smem_blksz != master->erasesize) {
> > +		pr_err("SMEM block size differs from MTD block size\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Get partition pointer from SMEM */
> > +	ret = qcom_smem_get_flash_partitions(&smem_parts);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (memcmp(SMEM_PTABLE_MAGIC, smem_parts->magic,
> > +		   sizeof(SMEM_PTABLE_MAGIC))) {
> > +		pr_err("SMEM partition magic invalid\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Allocate and populate the mtd structures */
> > +	mtd_parts = kcalloc(le32_to_cpu(smem_parts->len), sizeof(*mtd_parts),
> > +			    GFP_KERNEL);
> > +	if (!mtd_parts)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < smem_parts->len; i++) {
> 
> This might have the wrong endian...
Right. I'll do a le32_to_cpu()
> 
> > +		struct smem_partition *s_part = &smem_parts->parts[i];
> > +		struct mtd_partition *m_part = &mtd_parts[i];
> 
> I think Stephens suggestion was that you assign *pparts before looping
> and then you simply do:
> 
> *pparts = mtd_parts;
> 
> while (len--) {
> 	..
> 
> 	s_part++;
> 	mtd_parts++;
> }
> 
> > +
> > +		m_part->name = s_part->name;
> 
> I tried to follow the mtd code, and I believe that this pointer might
> be freed in free_partition(). If so you must kstrdup it here.
OK.
> 
> > +		m_part->size = le32_to_cpu(s_part->size) * (*smem_blksz);
> > +		m_part->offset = le32_to_cpu(s_part->start) * (*smem_blksz);
> > +
> > +		/*
> > +		 * The last SMEM partition may have its size marked as
> > +		 * something like 0xffffffff, which means "until the end of the
> > +		 * flash device". In this case, truncate it.
> > +		 */
> > +		if (m_part->offset + m_part->size > master->size)
> > +			m_part->size = master->size - m_part->offset;
> > +	}
> > +
> > +	*pparts = mtd_parts;
> > +
> > +	return smem_parts->len;
> 
> Again, this might be in the wrong endian.
Yes.
> 
> > +}
> > +
> > +static struct mtd_part_parser qcom_smem_parser = {
> > +	.owner = THIS_MODULE,
> > +	.parse_fn = parse_qcom_smem_partitions,
> > +	.name = "qcom-smem",
> > +};
> > +
> > +static int __init qcom_smem_parser_init(void)
> > +{
> > +	register_mtd_parser(&qcom_smem_parser);
> > +	return 0;
> > +}
> > +
> > +static void __exit qcom_smem_parser_exit(void)
> > +{
> > +	deregister_mtd_parser(&qcom_smem_parser);
> > +}
> > +
> > +module_init(qcom_smem_parser_init);
> > +module_exit(qcom_smem_parser_exit);
> 
> Mostly unrelated to this patch; This makes me appreciate the
> module_platform_driver() macro, would be nice to see a patch that
> introduces something similar for partition parsers.
> 
> > +
> > +MODULE_LICENSE("GPL");
> 
> Are you sure you don't mean "GPL v2" here?
I'll update it.
> 
> > +MODULE_AUTHOR("Mathieu Olivari <mathieu@codeaurora.org>");
> > +MODULE_DESCRIPTION("Parsing code for SMEM based partition tables");
> 
> Regards,
> Bjorn

[PATCH 0/3] qcom: Add SMEM MTD parser

[Mathieu Olivari]

QCOM platforms such as IPQ806x are using SMEM to store their flash
layout. This patch set adds the DT nodes required to instanciate SMEM
on IPQ806x and add an MTD parser using it.

This change is based on the SMEM driver posted here:
*https://lkml.org/lkml/2015/7/27/1125

Mathieu Olivari (3):
  ARM: qcom: add SFPB nodes to IPQ806x dts
  ARM: qcom: add SMEM device node to IPQ806x dts
  mtd: add SMEM parser for QCOM platforms

 arch/arm/boot/dts/qcom-ipq8064.dtsi |  23 +++-
 drivers/mtd/Kconfig                 |   7 ++
 drivers/mtd/Makefile                |   1 +
 drivers/mtd/qcom_smem_part.c        | 224 ++++++++++++++++++++++++++++++++++++
 4 files changed, 254 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/qcom_smem_part.c

-- 
2.1.4

[PATCH 0/3] qcom: Add SMEM MTD parser

[Mathieu Olivari]

QCOM platforms such as IPQ806x are using SMEM to store their flash
layout. This patch set adds the DT nodes required to instanciate SMEM
on IPQ806x and add an MTD parser using it.

This change is based on the SMEM driver posted here:
*https://lkml.org/lkml/2015/7/27/1125

Mathieu Olivari (3):
  ARM: qcom: add SFPB nodes to IPQ806x dts
  ARM: qcom: add SMEM device node to IPQ806x dts
  mtd: add SMEM parser for QCOM platforms

 arch/arm/boot/dts/qcom-ipq8064.dtsi |  23 +++-
 drivers/mtd/Kconfig                 |   7 ++
 drivers/mtd/Makefile                |   1 +
 drivers/mtd/qcom_smem_part.c        | 224 ++++++++++++++++++++++++++++++++++++
 4 files changed, 254 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/qcom_smem_part.c

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] qcom: Add SMEM MTD parser

[Mathieu Olivari]

QCOM platforms such as IPQ806x are using SMEM to store their flash
layout. This patch set adds the DT nodes required to instanciate SMEM
on IPQ806x and add an MTD parser using it.

This change is based on the SMEM driver posted here:
*https://lkml.org/lkml/2015/7/27/1125

Mathieu Olivari (3):
  ARM: qcom: add SFPB nodes to IPQ806x dts
  ARM: qcom: add SMEM device node to IPQ806x dts
  mtd: add SMEM parser for QCOM platforms

 arch/arm/boot/dts/qcom-ipq8064.dtsi |  23 +++-
 drivers/mtd/Kconfig                 |   7 ++
 drivers/mtd/Makefile                |   1 +
 drivers/mtd/qcom_smem_part.c        | 224 ++++++++++++++++++++++++++++++++++++
 4 files changed, 254 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/qcom_smem_part.c

-- 
2.1.4