drivers/qcom: add Command DB support

drivers/soc/qcom/Kconfig +4 -0

+	help
+	  Command DB queries shared memory by key string for shared system
+	  resources
+
 endmenu

drivers/soc/qcom/Makefile +1 -0

index 40c56f67e94a..7b64135b22eb 100644
@@ -10,3 +10,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o

drivers/soc/qcom/cmd-db.c new file +4 -0

new file mode 100644
index 000000000000..eb10ea8cf963
@@ -0,0 +1,306 @@
+/* Copyright (c) 2016-2018, 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

drivers/soc/qcom/cmd-db.c new file +20 -0

+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+#include <soc/qcom/cmd-db.h>
+
+#define RESOURCE_ID_LEN		8
+#define NUM_PRIORITY		2
+#define MAX_SLV_ID		8
+#define CMD_DB_MAGIC		0x0C0330DBUL
+#define SLAVE_ID_MASK		0x7
+#define SLAVE_ID_SHIFT		16
+
+/**
+ * entry_header: header for each entry in cmddb
+ *
+ * @res_id: resource's identifier
+ * @priority: unused

drivers/soc/qcom/cmd-db.c new file +8 -0

+};
+
+/**
+ * rsc_hdr: resource header information
+ *
+ * @slv_id: id for the resource
+ * @header_offset: Entry header offset from data
+ * @data_offset: Entry offset for datda location

drivers/soc/qcom/cmd-db.c new file +6 -0

+};
+
+/**
+ * cmd_db_header: The DB header information
+ *
+ * @version: The cmd db version

drivers/soc/qcom/cmd-db.c new file +6 -0

+};
+
+/**
+ * cmd_db_entry: Inforamtion for each line in the cmd db
+ *
+ * @resource_id: unique identifier for each entry

drivers/soc/qcom/cmd-db.c new file +15 -0

+
+	return rsc_id;
+}
+
+static int cmd_db_get_header(u64 query, struct entry_header *eh,
+		struct rsc_hdr *rh, bool use_addr)
+{
+	struct rsc_hdr *rsc_hdr;
+	int i, j;
+
+	if (!cmd_db_header)
+		return -EPROBE_DEFER;
+
+	if (!eh || !rh)
+		return -EINVAL;

drivers/soc/qcom/cmd-db.c new file +53 -0

+				+ sizeof(*cmd_db_header)
+				+ rsc_hdr->header_offset);
+
+		for (j = 0; j < rsc_hdr->cnt; j++, ent++) {
+			if (use_addr) {
+				if (ent->addr == (u32)(query))
+					break;
+			} else if (ent->res_id == query)
+				break;
+		}
+
+		if (j < rsc_hdr->cnt) {
+			memcpy(eh, ent, sizeof(*ent));
+			memcpy(rh, &cmd_db_header->header[i], sizeof(*rh));
+			return 0;
+		}
+	}
+	return -ENODEV;
+}
+
+static int cmd_db_get_header_by_rsc_id(const char *resource_id,
+		struct entry_header *ent_hdr,
+		struct rsc_hdr *rsc_hdr)
+{
+	u64 rsc_id = cmd_db_get_u64_id(resource_id);
+
+	return cmd_db_get_header(rsc_id, ent_hdr, rsc_hdr, false);
+}
+
+u32 cmd_db_get_addr(const char *resource_id)
+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+
+	return ret < 0 ? 0 : ent.addr;
+}
+EXPORT_SYMBOL(cmd_db_get_addr);
+
+int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)
+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	if (!data)
+		return -EINVAL;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+
+	if (ret)

drivers/soc/qcom/cmd-db.c new file +56 -0

+			len);
+	return len;
+}
+EXPORT_SYMBOL(cmd_db_get_aux_data);
+
+int cmd_db_get_aux_data_len(const char *resource_id)
+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+
+	return ret < 0 ? 0 : ent.len;
+}
+EXPORT_SYMBOL(cmd_db_get_aux_data_len);
+
+u16 cmd_db_get_version(const char *resource_id)
+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+	return ret < 0 ? 0 : rsc_hdr.version;
+}
+EXPORT_SYMBOL(cmd_db_get_version);
+
+int cmd_db_ready(void)
+{
+	return cmd_db_status;
+}
+EXPORT_SYMBOL(cmd_db_ready);
+
+int cmd_db_get_slave_id(const char *resource_id)
+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+	return ret < 0 ? 0 : (ent.addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;
+}
+EXPORT_SYMBOL(cmd_db_get_slave_id);
+
+static int cmd_db_dev_probe(struct platform_device *pdev)
+{
+	struct resource res;
+	void __iomem *dict;
+
+	dict = of_iomap(pdev->dev.of_node, 0);
+	if (!dict) {
+		cmd_db_status = -ENOMEM;
+		goto failed;
+	}
+

drivers/soc/qcom/cmd-db.c new file +5 -0

+
+failed:
+	return cmd_db_status;
+}
+

drivers/soc/qcom/cmd-db.c new file +4 -0

+{
+	return platform_driver_register(&cmd_db_dev_driver);
+}
+arch_initcall(cmd_db_device_init);

include/soc/qcom/cmd-db.h new file +83 -0

new file mode 100644
index 000000000000..1e42f7509cf9
@@ -0,0 +1,119 @@
+/* Copyright (c) 2016-2018, 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.
+ */
+
+#ifndef __QCOM_COMMAND_DB_H__
+#define __QCOM_COMMAND_DB_H__
+
+
+enum cmd_db_hw_type {
+	CMD_DB_HW_MIN = 3,
+	CMD_DB_HW_ARC = CMD_DB_HW_MIN,
+	CMD_DB_HW_VRM = 4,
+	CMD_DB_HW_BCM = 5,
+	CMD_DB_HW_MAX = CMD_DB_HW_BCM,
+	CMD_DB_HW_ALL = 0xff,
+};
+
+#ifdef CONFIG_QCOM_COMMAND_DB
+/**
+ * cmd_db_get_addr() - Query command db for resource id address.
+ *
+ *  This is used to retrieve resource address based on resource
+ *  id.
+ *
+ *  @resource_id : resource id to query for address
+ *
+ *  returns resource address on success or 0 on error otherwise
+ */
+u32 cmd_db_get_addr(const char *resource_id);
+
+/**
+ * cmd_db_get_aux_data() - Query command db for aux data. This is used to
+ * retrieve a command DB entry based resource address.
+ *
+ *  @resource_id : Resource to retrieve AUX Data on.
+ *  @data : Data buffer to copy returned aux data to. Returns size on NULL
+ *  @len : Caller provides size of data buffer passed in.
+ *
+ *  returns size of data on success, errno on error
+ */
+int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len);
+
+/**
+ * cmd_db_get_aux_data_len - Get the length of the auxllary data stored in DB.
+ *
+ * @resource_id: Resource to retrieve AUX Data.
+ *
+ * returns size on success, errno on error
+ */
+int cmd_db_get_aux_data_len(const char *resource_id);
+
+/**
+ * cmd_db_get_version - Get the version of the command DB data
+ *
+ * @resource_id: Resource id to query the DB for version
+ *
+ * returns version on success, 0 on error.
+ *	Major number in MSB, minor number in LSB
+ */
+u16 cmd_db_get_version(const char *resource_id);
+
+/**
+ * cmd_db_ready - Indicates if command DB is probed
+ *
+ * returns  0 on success , errno otherwise
+ */
+int cmd_db_ready(void);
+
+/**
+ * cmd_db_get_slave_id - Get the slave ID for a given resource address
+ *
+ * @resource_id: Resource id to query the DB for version
+ *
+ * return  cmd_db_hw_type enum  on success, errno on error
+ */

include/soc/qcom/cmd-db.h new file +26 -0

+{
+	return -ENODEV;
+}
+
+int cmd_db_get_aux_data_len(const char *resource_id)
+{
+	return -ENODEV;
+}
+
+u16 cmd_db_get_version(const char *resource_id)
+{
+	return 0;
+}
+
+int cmd_db_ready(void)
+{
+	return -ENODEV;
+}
+
+int cmd_db_get_slave_id(const char *resource_id)
+{
+	return -ENODEV;
+}
+
+#endif /* CONFIG_QCOM_COMMAND_DB */
+#endif /* __QCOM_COMMAND_DB_H__ */

Quoted 8 lines · Lina Iyer

drivers/soc/qcom/Kconfig +0 -0

index b81374bb6713..f21c5d53e721 100644
@@ -97,4 +97,11 @@ config QCOM_WCNSS_CTRL
 	  Client driver for the WCNSS_CTRL SMD channel, used to download nv
 	  firmware to a newly booted WCNSS chip.
 

drivers/soc/qcom/Kconfig config QCOM_WCNSS_CTRL · line 102

Quoted 3 lines

drivers/soc/qcom/Kconfig +3 -0

+config QCOM_COMMAND_DB
+	bool "Qualcomm Command DB"
+	depends on ARCH_QCOM

Add || COMPILE_TEST?

drivers/soc/qcom/Kconfig config QCOM_WCNSS_CTRL · line 104

Quoted 2 lines

drivers/soc/qcom/Kconfig +2 -0

+#include <linux/kernel.h>
+#include <linux/module.h>

It's not a module.

Quoted 9 lines

drivers/soc/qcom/Kconfig +9 -0

+/**
+ * entry_header: header for each entry in cmddb
+ *
+ * @res_id: resource's identifier
+ * @priority: unused
+ * @addr: the address of the resource
+ * @len: length of the data
+ * @offset: offset at which dats starts
+ */

drivers/soc/qcom/Kconfig

Quoted 6 lines

drivers/soc/qcom/Kconfig +6 -0

+struct entry_header {
+	uint64_t res_id;
+	u32 priority[NUM_PRIORITY];
+	u32 addr;
+	u16 len;
+	u16 offset;

Are these little endian? Needs to be __le16 and __le32 then.

Quoted 8 lines

drivers/soc/qcom/Kconfig +8 -0

+};
+
+/**
+ * rsc_hdr: resource header information
+ *
+ * @slv_id: id for the resource
+ * @header_offset: Entry header offset from data
+ * @data_offset: Entry offset for datda location

drivers/soc/qcom/Kconfig

Quoted 10 lines

drivers/soc/qcom/Kconfig +10 -0

+ * @cnt: number of enteries for HW type
+ * @version: MSB is major, LSB is minor
+ */
+struct rsc_hdr {
+	u16  slv_id;
+	u16  header_offset;
+	u16  data_offset;
+	u16  cnt;
+	u16  version;
+	u16 reserved[3];

Same comment.

Quoted 11 lines

drivers/soc/qcom/Kconfig +11 -0

+};
+
+/**
+ * cmd_db_header: The DB header information
+ *
+ * @version: The cmd db version
+ * @magic_number: constant expected in the database
+ * @header: array of resources
+ */
+struct cmd_db_header {
+	u32 version;

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+	u32 magic_num;
+	struct rsc_hdr header[MAX_SLV_ID];
+	u32 check_sum;
+	u32 reserved;
+	u8 data[];

I hope struct randomization doesn't strike back. Same comment about endianness.

Quoted 6 lines

drivers/soc/qcom/Kconfig +6 -0

+};
+
+/**
+ * cmd_db_entry: Inforamtion for each line in the cmd db
+ *
+ * @resource_id: unique identifier for each entry

drivers/soc/qcom/Kconfig

Quoted 10 lines

drivers/soc/qcom/Kconfig +10 -0

+ * @addr: slave id + offset address
+ * @priority: Bitmask for DRV ids
+ * @len: aux data len
+ * @data: data assocaited with the resource
+ */
+struct cmd_db_entry {
+	const char resource_id[RESOURCE_ID_LEN + 1];
+	const u32 addr;
+	const u32 priority[NUM_PRIORITY];
+	u32 len;

But this isn't const?

drivers/soc/qcom/Kconfig

Quoted 1 line

drivers/soc/qcom/Kconfig +1 -0

+	u16 version;

Endian.

drivers/soc/qcom/Kconfig

Quoted 2 lines

drivers/soc/qcom/Kconfig +2 -0

+	u8  data[];
+};

drivers/soc/qcom/Kconfig

Quoted 4 lines

drivers/soc/qcom/Kconfig +4 -0

+
+static void __iomem *start_addr;
+static struct cmd_db_header *cmd_db_header;
+static int cmd_db_status = -EPROBE_DEFER;

Hopefully we never have more than one commmand db? Do consumers "just know" to use this code? I haven't looked at the DT binding, but perhaps consumers need to point to command db via DT phandles so we can identify the consumers. That may make probe defer easier too.

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+
+static u64 cmd_db_get_u64_id(const char *id)
+{
+	uint64_t rsc_id = 0;
+	uint8_t *ch  = (uint8_t *)&rsc_id;

Use u* instead of uint*_t please.

drivers/soc/qcom/Kconfig

Quoted 3 lines

drivers/soc/qcom/Kconfig +3 -0

+	int i;
+
+	for (i = 0; ((i < sizeof(rsc_id)) && id[i]); i++)

Drop useless parenthesis.

drivers/soc/qcom/Kconfig

Quoted 1 line

drivers/soc/qcom/Kconfig +1 -0

+		ch[i] = id[i];

Is this a strcpy?

drivers/soc/qcom/Kconfig

Quoted 3 lines

drivers/soc/qcom/Kconfig +3 -0

+	memcpy_fromio(data,
+			start_addr + sizeof(*cmd_db_header)
+			+ rsc_hdr.data_offset + ent.offset,

Maybe make a macro for rsc_offset() or ent_offset() or something like that?

drivers/soc/qcom/Kconfig

Quoted 1 line

drivers/soc/qcom/Kconfig +1 -0

+	start_addr = devm_ioremap_resource(&pdev->dev, &res);

And if this mapping fails? Is it in DDR? We would need to not use ioremap then.

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+
+	cmd_db_header = devm_kzalloc(&pdev->dev,
+			sizeof(*cmd_db_header), GFP_KERNEL);
+
+	if (!cmd_db_header) {

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+		cmd_db_status = -ENOMEM;
+		goto failed;
+	}
+
+	memcpy(cmd_db_header, start_addr, sizeof(*cmd_db_header));

Why are we copying this? We can't just read from the remmapped ram directly?

drivers/soc/qcom/Kconfig

Quoted 2 lines

drivers/soc/qcom/Kconfig +2 -0

+
+	if (cmd_db_header->magic_num != CMD_DB_MAGIC) {

memcmp?

drivers/soc/qcom/Kconfig

Quoted 6 lines

drivers/soc/qcom/Kconfig +6 -0

+		pr_err("%s(): Invalid Magic\n", __func__);
+		cmd_db_status = -EINVAL;
+		goto failed;
+	}
+	cmd_db_status = 0;
+	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);

What are we populating?

Quoted 9 lines

drivers/soc/qcom/Kconfig +9 -0

+
+failed:
+	return cmd_db_status;
+}
+
+static const struct of_device_id cmd_db_match_table[] = {
+	{ .compatible = "qcom,cmd-db" },
+	{ },
+};

drivers/soc/qcom/Kconfig

Quoted 6 lines

drivers/soc/qcom/Kconfig +6 -0

+
+static struct platform_driver cmd_db_dev_driver = {
+	.probe = cmd_db_dev_probe,
+	.driver = {
+		.name = "cmd-db",
+		.owner = THIS_MODULE,

Drop this.

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+		.of_match_table = cmd_db_match_table,
+	},
+};
+
+int __init cmd_db_device_init(void)

static? Please run sparse.

Quoted 1 line

+int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)

All these below are missing static inline?

Quoted 19 lines

drivers/soc/qcom/Kconfig +19 -0

+{
+	return -ENODEV;
+}
+
+int cmd_db_get_aux_data_len(const char *resource_id)
+{
+	return -ENODEV;
+}
+
+u16 cmd_db_get_version(const char *resource_id)
+{
+	return 0;
+}
+
+int cmd_db_ready(void)
+{
+	return -ENODEV;
+}
+

drivers/soc/qcom/Kconfig

Quoted 8 lines

drivers/soc/qcom/Kconfig +7 -0

+int cmd_db_get_slave_id(const char *resource_id)
+{
+	return -ENODEV;
+}
+
+#endif /* CONFIG_QCOM_COMMAND_DB */
+#endif /* __QCOM_COMMAND_DB_H__ */

Quoted 2 lines · Stephen Boyd

Quoted 1 line · Lina Iyer

drivers/soc/qcom/Kconfig +0 -0

Quoted 9 lines

drivers/soc/qcom/Kconfig

Quoted 6 lines

drivers/soc/qcom/Kconfig +6 -0

+struct entry_header {
+	uint64_t res_id;
+	u32 priority[NUM_PRIORITY];
+	u32 addr;
+	u16 len;
+	u16 offset;

Are these little endian? Needs to be __le16 and __le32 then.

Not a device memory.

Do we need to worry about endianness?

Quoted 6 lines

Hopefully we never have more than one commmand db? Do consumers "just know" to use this code? I haven't looked at the DT binding, but perhaps consumers need to point to command db via DT phandles so we can identify the consumers. That may make probe defer easier too.

There would be just one command DB for an SoC. Currently, none of the the clients need to probe at the time of command DB.

The producer-consumer model might help, but it is probably not needed here.

Quoted 4 lines

drivers/soc/qcom/Kconfig

Quoted 1 line

drivers/soc/qcom/Kconfig +1 -0

+		ch[i] = id[i];

Is this a strcpy?

In a way yes.

drivers/soc/qcom/Kconfig +0 -0

index b81374bb6713..f21c5d53e721 100644

drivers/soc/qcom/Kconfig +0 -0

 endmenu

drivers/soc/qcom/Makefile +1 -0

index 40c56f67e94a..7b64135b22eb 100644
@@ -10,3 +10,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o

drivers/soc/qcom/cmd-db.c +0 -0

+/**
+ * entry_header: header for each entry in cmddb
+ *
+ * @res_id: resource's identifier

+ * @priority: unused
+ * @addr: the address of the resource
+ * @len: length of the data
+ * @offset: offset at which dats starts

+ */

drivers/soc/qcom/cmd-db.c +22 -0

+struct entry_header {
+	uint64_t res_id;
+	u32 priority[NUM_PRIORITY];
+	u32 addr;
+	u16 len;
+	u16 offset;
+};
+
+/**
+ * rsc_hdr: resource header information
+ *
+ * @slv_id: id for the resource
+ * @header_offset: Entry header offset from data
+ * @data_offset: Entry offset for datda location
+ * @cnt: number of enteries for HW type
+ * @version: MSB is major, LSB is minor
+ */
+struct rsc_hdr {
+	u16  slv_id;
+	u16  header_offset;
+	u16  data_offset;
+	u16  cnt;

+ */

drivers/soc/qcom/cmd-db.c +9 -0

+struct cmd_db_header {
+	u32 version;
+	u32 magic_num;
+	struct rsc_hdr header[MAX_SLV_ID];
+	u32 check_sum;
+	u32 reserved;
+	u8 data[];
+};
+

+static int cmd_db_status = -EPROBE_DEFER;

drivers/soc/qcom/cmd-db.c +15 -0

+		}
+	}
+	return -ENODEV;
+}
+
+static int cmd_db_get_header_by_rsc_id(const char *resource_id,
+		struct entry_header *ent_hdr,
+		struct rsc_hdr *rsc_hdr)
+{
+	u64 rsc_id = cmd_db_get_u64_id(resource_id);
+
+	return cmd_db_get_header(rsc_id, ent_hdr, rsc_hdr, false);
+}
+
+u32 cmd_db_get_addr(const char *resource_id)

drivers/soc/qcom/cmd-db.c +5 -0

+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+

drivers/soc/qcom/cmd-db.c +17 -0

+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+
+	return ret < 0 ? 0 : ent.len;
+}
+EXPORT_SYMBOL(cmd_db_get_aux_data_len);
+
+u16 cmd_db_get_version(const char *resource_id)
+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+

drivers/soc/qcom/cmd-db.c +22 -0

+	return cmd_db_status;
+}
+
+static const struct of_device_id cmd_db_match_table[] = {
+	{ .compatible = "qcom,cmd-db" },
+	{ },
+};
+
+static struct platform_driver cmd_db_dev_driver = {
+	.probe = cmd_db_dev_probe,
+	.driver = {
+		.name = "cmd-db",
+		.owner = THIS_MODULE,
+		.of_match_table = cmd_db_match_table,
+	},
+};
+
+int __init cmd_db_device_init(void)
+{
+	return platform_driver_register(&cmd_db_dev_driver);
+}
+arch_initcall(cmd_db_device_init);

include/soc/qcom/cmd-db.h +0 -0

+#ifdef CONFIG_QCOM_COMMAND_DB

+/**
+ * cmd_db_get_addr() - Query command db for resource id address.

+ *
+ * This is used to retrieve resource address based on resource
+ * id.

+ *
+ * @resource_id : resource id to query for address

+ *
+ * returns resource address on success or 0 on error otherwise

+ */

include/soc/qcom/cmd-db.h +10 -0

+u32 cmd_db_get_addr(const char *resource_id);
+
+/**
+ * cmd_db_get_aux_data() - Query command db for aux data. This is used to
+ * retrieve a command DB entry based resource address.
+ *
+ *  @resource_id : Resource to retrieve AUX Data on.
+ *  @data : Data buffer to copy returned aux data to. Returns size on NULL
+ *  @len : Caller provides size of data buffer passed in.
+ *

include/soc/qcom/cmd-db.h +4 -0

+
+/**
+ * cmd_db_ready - Indicates if command DB is probed
+ *

+ */
+int cmd_db_get_slave_id(const char *resource_id);

Quoted 8 lines · Lina Iyer

drivers/soc/qcom/Kconfig +0 -0

index b81374bb6713..f21c5d53e721 100644
@@ -97,4 +97,11 @@ config QCOM_WCNSS_CTRL
 	  Client driver for the WCNSS_CTRL SMD channel, used to download nv
 	  firmware to a newly booted WCNSS chip.
 

drivers/soc/qcom/Kconfig config QCOM_WCNSS_CTRL · line 106

Quoted 7 lines

drivers/soc/qcom/Kconfig +7 -0

+config QCOM_COMMAND_DB
+	bool "Qualcomm Command DB"
+	depends on ARCH_QCOM
+	help
+	  Command DB queries shared memory by key string for shared system
+	  resources
+

Please try to keep these alphabetically sorted.

drivers/soc/qcom/Kconfig config QCOM_WCNSS_CTRL · line 107

Quoted 1 line

drivers/soc/qcom/Kconfig +1 -0

+#define RESOURCE_ID_LEN		8

Only used in unused structure below?

drivers/soc/qcom/Kconfig config QCOM_WCNSS_CTRL

Quoted 6 lines

drivers/soc/qcom/Kconfig +6 -0

+#define NUM_PRIORITY		2
+#define MAX_SLV_ID		8
+#define CMD_DB_MAGIC		0x0C0330DBUL
+#define SLAVE_ID_MASK		0x7
+#define SLAVE_ID_SHIFT		16
+

It would be nice with a kernel-doc "DOC:" here that describes the in-memory format that we're operating on.

Quoted 4 lines

+/**
+ * entry_header: header for each entry in cmddb
+ *
+ * @res_id: resource's identifier

"id" is shorter and just as descriptive.

Quoted 4 lines

+ * @priority: unused
+ * @addr: the address of the resource
+ * @len: length of the data
+ * @offset: offset at which dats starts

"data"

Quoted 27 lines

+ */

drivers/soc/qcom/Kconfig +26 -0

+struct entry_header {
+	uint64_t res_id;
+	u32 priority[NUM_PRIORITY];
+	u32 addr;
+	u16 len;
+	u16 offset;
+};
+
+/**
+ * rsc_hdr: resource header information
+ *
+ * @slv_id: id for the resource
+ * @header_offset: Entry header offset from data
+ * @data_offset: Entry offset for datda location
+ * @cnt: number of enteries for HW type
+ * @version: MSB is major, LSB is minor
+ */
+struct rsc_hdr {
+	u16  slv_id;
+	u16  header_offset;
+	u16  data_offset;
+	u16  cnt;
+	u16  version;
+	u16 reserved[3];
+};
+

Quoted 6 lines

+/**
+ * cmd_db_header: The DB header information
+ *
+ * @version: The cmd db version
+ * @magic_number: constant expected in the database
+ * @header: array of resources

Missing @check_sum and @data.

Quoted 14 lines

+ */

drivers/soc/qcom/Kconfig +13 -0

+struct cmd_db_header {
+	u32 version;
+	u32 magic_num;
+	struct rsc_hdr header[MAX_SLV_ID];
+	u32 check_sum;
+	u32 reserved;
+	u8 data[];
+};
+
+/**
+ * cmd_db_entry: Inforamtion for each line in the cmd db
+ *
+ * @resource_id: unique identifier for each entry

Quoted 6 lines

+ * @addr: slave id + offset address
+ * @priority: Bitmask for DRV ids
+ * @len: aux data len
+ * @data: data assocaited with the resource
+ */
+struct cmd_db_entry {

Why isn't this used anywhere? What am I missing?

drivers/soc/qcom/Kconfig

Quoted 10 lines

drivers/soc/qcom/Kconfig +10 -0

+	const char resource_id[RESOURCE_ID_LEN + 1];
+	const u32 addr;
+	const u32 priority[NUM_PRIORITY];
+	u32 len;
+	u16 version;
+	u8  data[];
+};
+
+static void __iomem *start_addr;
+static struct cmd_db_header *cmd_db_header;

Rather than keeping a copy of this around you can verify the magic in probe and then just carry an object that's:

struct cmd_db {
	struct rsc_hdr *headers;
	void *entries;
};

That way you can drop the start_addr variable as well...

Quoted 1 line

+static int cmd_db_status = -EPROBE_DEFER;

You don't really need a separate variable to track that probe() isn't done. cmd_db_header can be used for this...

Quoted 7 lines

drivers/soc/qcom/Kconfig +7 -0

+
+static u64 cmd_db_get_u64_id(const char *id)
+{
+	uint64_t rsc_id = 0;
+	uint8_t *ch  = (uint8_t *)&rsc_id;
+	int i;
+

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+	for (i = 0; ((i < sizeof(rsc_id)) && id[i]); i++)
+		ch[i] = id[i];
+
+	return rsc_id;
+}

So this "casts" a 8 byte string to a u64. Why not just use u64 constants to represent the resources, as we've done in the past?

drivers/soc/qcom/Kconfig

Quoted 3 lines

drivers/soc/qcom/Kconfig +3 -0

+
+static int cmd_db_get_header(u64 query, struct entry_header *eh,
+		struct rsc_hdr *rh, bool use_addr)

This function is static and there's only one caller, which has use_addr as false. So please omit this parameter for now.

drivers/soc/qcom/Kconfig

Quoted 11 lines

drivers/soc/qcom/Kconfig +11 -0

+{
+	struct rsc_hdr *rsc_hdr;
+	int i, j;
+
+	if (!cmd_db_header)
+		return -EPROBE_DEFER;
+
+	if (!eh || !rh)
+		return -EINVAL;
+
+	rsc_hdr = &cmd_db_header->header[0];

Rather than bumping the pointer in the loop, just move this inside the loop as:

	rsc_hdr = &cmd_db_header->header[i];

drivers/soc/qcom/Kconfig

Quoted 10 lines

drivers/soc/qcom/Kconfig +10 -0

+
+	for (i = 0; i < MAX_SLV_ID ; i++, rsc_hdr++) {
+		struct entry_header *ent;
+
+		if (!rsc_hdr->slv_id)
+			break;
+
+		ent = (struct entry_header *)(start_addr
+				+ sizeof(*cmd_db_header)
+				+ rsc_hdr->header_offset);

If you have start_addr expressed as a struct cmd_db_header then this would be:

	ent = (struct entry_header *)db->data + rsc_hdr->header_offset;

drivers/soc/qcom/Kconfig

Quoted 13 lines

drivers/soc/qcom/Kconfig +13 -0

+
+		for (j = 0; j < rsc_hdr->cnt; j++, ent++) {
+			if (use_addr) {
+				if (ent->addr == (u32)(query))
+					break;
+			} else if (ent->res_id == query)
+				break;
+		}
+
+		if (j < rsc_hdr->cnt) {
+			memcpy(eh, ent, sizeof(*ent));
+			memcpy(rh, &cmd_db_header->header[i], sizeof(*rh));
+			return 0;

This function really returns reference to data, length of the data and version. How about just making this explicit, rather than copying two sets of headers to the caller and having them extract this information?

I think you should turn this function into:

static void *cmd_db_find(u64 id, size_t *len, u16 *version)

Quoted 15 lines

drivers/soc/qcom/Kconfig +15 -0

+		}
+	}
+	return -ENODEV;
+}
+
+static int cmd_db_get_header_by_rsc_id(const char *resource_id,
+		struct entry_header *ent_hdr,
+		struct rsc_hdr *rsc_hdr)
+{
+	u64 rsc_id = cmd_db_get_u64_id(resource_id);
+
+	return cmd_db_get_header(rsc_id, ent_hdr, rsc_hdr, false);
+}
+
+u32 cmd_db_get_addr(const char *resource_id)

drivers/soc/qcom/Kconfig

Quoted 10 lines

drivers/soc/qcom/Kconfig +10 -0

+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+
+	return ret < 0 ? 0 : ent.addr;
+}
+EXPORT_SYMBOL(cmd_db_get_addr);

Please kernel-doc any publicly visible functions.

I presume the returned value of this function is going to be used to reference the data directly, as such it should return a void *.

drivers/soc/qcom/Kconfig

Quoted 2 lines

drivers/soc/qcom/Kconfig +2 -0

+
+int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)

"cmd_db_read_data()" would better represent that we're copying the data into the buffer.

(And the "aux" part in these function names doesn't seem to add any value)

Quoted 10 lines

drivers/soc/qcom/Kconfig +10 -0

+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	if (!data)
+		return -EINVAL;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+	if (ret)
+		return ret;
+
+	if (ent.len < len)
+		return -EINVAL;

The purpose of this check is very likely "will the data fit in @data". In which case the comparison should be len < ent.len

drivers/soc/qcom/Kconfig

Quoted 2 lines

drivers/soc/qcom/Kconfig +2 -0

+
+	len = (ent.len < len) ? ent.len : len;

In particular as we here will make len = MIN(ent.len, len);

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+
+	memcpy_fromio(data,
+			start_addr + sizeof(*cmd_db_header)
+			+ rsc_hdr.data_offset + ent.offset,
+			len);

Why is this fromio when there is a vanilla memcpy() below?

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+	return len;
+}
+EXPORT_SYMBOL(cmd_db_get_aux_data);
+
+int cmd_db_get_aux_data_len(const char *resource_id)

data_len is a typical size_t, in particular as you're clamping the value to a positive number.

Quoted 17 lines

drivers/soc/qcom/Kconfig +17 -0

+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+
+	return ret < 0 ? 0 : ent.len;
+}
+EXPORT_SYMBOL(cmd_db_get_aux_data_len);
+
+u16 cmd_db_get_version(const char *resource_id)
+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+

drivers/soc/qcom/Kconfig

Quoted 10 lines

drivers/soc/qcom/Kconfig +10 -0

+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+	return ret < 0 ? 0 : rsc_hdr.version;
+}
+EXPORT_SYMBOL(cmd_db_get_version);
+
+int cmd_db_ready(void)
+{
+	return cmd_db_status;
+}
+EXPORT_SYMBOL(cmd_db_ready);

Move this function one step down, to keep the getters together.

Based on the function name this function should return a bool.

Quoted 4 lines

drivers/soc/qcom/Kconfig +4 -0

+
+int cmd_db_get_slave_id(const char *resource_id)
+{
+	int ret;

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+	return ret < 0 ? 0 : (ent.addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;

Why is this bit 16-19 of the address? What is ent.addr?! Why isn't this rsc_hdr.slv_id?

drivers/soc/qcom/Kconfig

Quoted 13 lines

drivers/soc/qcom/Kconfig +13 -0

+}
+EXPORT_SYMBOL(cmd_db_get_slave_id);
+
+static int cmd_db_dev_probe(struct platform_device *pdev)
+{
+	struct resource res;
+	void __iomem *dict;
+
+	dict = of_iomap(pdev->dev.of_node, 0);
+	if (!dict) {
+		cmd_db_status = -ENOMEM;
+		goto failed;
+	}

Please use the idiomatic way of remapping memory in a platform driver:

res = platform_get_resource(pdev, IORESOURCE_MEM, 0); base = ioremap_resource(&pdev->dev, res);
if (IS_ERR(base))

	...

And I think you should just return the error code directly, rather than updating cmd_db_status. There isn't much benefit of letting the client know that command db is in "ENOMEM" state vs "EPROBE_DEFER"; neither case will provide a working device and it invites the client implementor to complicate their drivers by trying to do clever things depending on the result.

drivers/soc/qcom/Kconfig

Quoted 9 lines

drivers/soc/qcom/Kconfig +9 -0

+
+	/*
+	 * Read start address and size of the command DB address from
+	 * shared dictionary location
+	 */
+	res.start = readl_relaxed(dict);
+	res.end = res.start + readl_relaxed(dict + 0x4);
+	res.flags = IORESOURCE_MEM;
+	iounmap(dict);

Why can't we just describe the memory directly?

drivers/soc/qcom/Kconfig

Quoted 2 lines

drivers/soc/qcom/Kconfig +2 -0

+
+	start_addr = devm_ioremap_resource(&pdev->dev, &res);

No error handling?

Quoted 7 lines

drivers/soc/qcom/Kconfig +7 -0

+
+	cmd_db_header = devm_kzalloc(&pdev->dev,
+			sizeof(*cmd_db_header), GFP_KERNEL);
+
+	if (!cmd_db_header) {
+		cmd_db_status = -ENOMEM;
+		goto failed;

drivers/soc/qcom/Kconfig

Quoted 6 lines

drivers/soc/qcom/Kconfig +6 -0

+	}
+
+	memcpy(cmd_db_header, start_addr, sizeof(*cmd_db_header));
+
+	if (cmd_db_header->magic_num != CMD_DB_MAGIC) {
+		pr_err("%s(): Invalid Magic\n", __func__);

dev_err() and drop the __func__

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+		cmd_db_status = -EINVAL;
+		goto failed;
+	}
+	cmd_db_status = 0;
+	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);

Why? This isn't described in the binding document.

drivers/soc/qcom/Kconfig

Quoted 2 lines

drivers/soc/qcom/Kconfig +2 -0

+
+failed:

This is not a good name for a label that is used for successful returns as well.

Quoted 23 lines

drivers/soc/qcom/Kconfig +22 -0

+	return cmd_db_status;
+}
+
+static const struct of_device_id cmd_db_match_table[] = {
+	{ .compatible = "qcom,cmd-db" },
+	{ },
+};
+
+static struct platform_driver cmd_db_dev_driver = {
+	.probe = cmd_db_dev_probe,
+	.driver = {
+		.name = "cmd-db",
+		.owner = THIS_MODULE,
+		.of_match_table = cmd_db_match_table,
+	},
+};
+
+int __init cmd_db_device_init(void)
+{
+	return platform_driver_register(&cmd_db_dev_driver);
+}
+arch_initcall(cmd_db_device_init);

include/soc/qcom/cmd-db.h +0 -0

[..]

Quoted 1 line

+#ifdef CONFIG_QCOM_COMMAND_DB

#if IS_ENABLED(CONFIG_QCOM_COMMAND_DB)

Quoted 2 lines

+/**
+ * cmd_db_get_addr() - Query command db for resource id address.

Move the kerneldoc to the implementation.

Quoted 3 lines

+ *
+ * This is used to retrieve resource address based on resource
+ * id.

The description should come after the list of parameters.

Quoted 2 lines

+ *
+ * @resource_id : resource id to query for address

Drop the extra spaces before and after @resource_id.

Quoted 2 lines

+ *
+ * returns resource address on success or 0 on error otherwise

Return:

In what address space does the return value live?

Quoted 15 lines

+ */

include/soc/qcom/cmd-db.h +14 -0

+u32 cmd_db_get_addr(const char *resource_id);
+
+/**
+ * cmd_db_get_aux_data() - Query command db for aux data. This is used to
+ * retrieve a command DB entry based resource address.
+ *
+ *  @resource_id : Resource to retrieve AUX Data on.
+ *  @data : Data buffer to copy returned aux data to. Returns size on NULL
+ *  @len : Caller provides size of data buffer passed in.
+ *
+ *  returns size of data on success, errno on error
+ */
+int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len);
+

Quoted 6 lines

+/**
+ * cmd_db_get_aux_data_len - Get the length of the auxllary data stored in DB.
+ *
+ * @resource_id: Resource to retrieve AUX Data.
+ *
+ * returns size on success, errno on error

No, this returns 0 on error.

Quoted 7 lines

+ */

include/soc/qcom/cmd-db.h +6 -0

+int cmd_db_get_aux_data_len(const char *resource_id);
+
+/**
+ * cmd_db_get_version - Get the version of the command DB data
+ *
+ * @resource_id: Resource id to query the DB for version

Quoted 5 lines

+ *
+ * returns version on success, 0 on error.
+ * Major number in MSB, minor number in LSB
+ */
+u16 cmd_db_get_version(const char *resource_id);

If the two pieces is to be used separately then pass u8 *msg, u8 *lsb to the function and fill these out.

Quoted 9 lines

include/soc/qcom/cmd-db.h +9 -0

+
+/**
+ * cmd_db_ready - Indicates if command DB is probed
+ *
+ * returns  0 on success , errno otherwise
+ */
+int cmd_db_ready(void);
+
+/**

Quoted 5 lines

+ * cmd_db_get_slave_id - Get the slave ID for a given resource address
+ *
+ * @resource_id: Resource id to query the DB for version
+ *
+ * return cmd_db_hw_type enum on success, errno on error

This returns 0 on error, so you could make the return type enum cmd_db_hw_type to clarify the interface.

Quoted 2 lines

+ */
+int cmd_db_get_slave_id(const char *resource_id);

Regards,
Bjorn

Quoted 1 line · Bjorn Andersson

On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:

Quoted 6 lines

Quoted 6 lines

drivers/soc/qcom/cmd-db.c new file +6 -0

+static u64 cmd_db_get_u64_id(const char *id)
+{
+	uint64_t rsc_id = 0;
+	uint8_t *ch  = (uint8_t *)&rsc_id;
+	int i;
+

Quoted 9 lines

drivers/soc/qcom/cmd-db.c line 113

Quoted 5 lines

drivers/soc/qcom/cmd-db.c new file +5 -0

+	for (i = 0; ((i < sizeof(rsc_id)) && id[i]); i++)
+		ch[i] = id[i];
+
+	return rsc_id;
+}

So this "casts" a 8 byte string to a u64. Why not just use u64 constants to represent the resources, as we've done in the past?

This matches the resource specification on the remote end for this new architecture.

Quoted 7 lines

drivers/soc/qcom/cmd-db.c line 116

Quoted 3 lines

drivers/soc/qcom/cmd-db.c new file +3 -0

+
+static int cmd_db_get_header(u64 query, struct entry_header *eh,
+		struct rsc_hdr *rh, bool use_addr)

This function is static and there's only one caller, which has use_addr as false. So please omit this parameter for now.

Ok.

Quoted 14 lines

drivers/soc/qcom/cmd-db.c line 127

Quoted 11 lines

drivers/soc/qcom/cmd-db.c new file +11 -0

+{
+	struct rsc_hdr *rsc_hdr;
+	int i, j;
+
+	if (!cmd_db_header)
+		return -EPROBE_DEFER;
+
+	if (!eh || !rh)
+		return -EINVAL;
+
+	rsc_hdr = &cmd_db_header->header[0];

Rather than bumping the pointer in the loop, just move this inside the loop as:

Quoted 2 lines

	rsc_hdr = &cmd_db_header->header[i];

Sure.

Quoted 34 lines

drivers/soc/qcom/cmd-db.c line 137

Quoted 10 lines

drivers/soc/qcom/cmd-db.c new file +10 -0

+
+	for (i = 0; i < MAX_SLV_ID ; i++, rsc_hdr++) {
+		struct entry_header *ent;
+
+		if (!rsc_hdr->slv_id)
+			break;
+
+		ent = (struct entry_header *)(start_addr
+				+ sizeof(*cmd_db_header)
+				+ rsc_hdr->header_offset);

If you have start_addr expressed as a struct cmd_db_header then this would be:

	ent = (struct entry_header *)db->data + rsc_hdr->header_offset;

drivers/soc/qcom/cmd-db.c line 150

Quoted 13 lines

drivers/soc/qcom/cmd-db.c new file +13 -0

+
+		for (j = 0; j < rsc_hdr->cnt; j++, ent++) {
+			if (use_addr) {
+				if (ent->addr == (u32)(query))
+					break;
+			} else if (ent->res_id == query)
+				break;
+		}
+
+		if (j < rsc_hdr->cnt) {
+			memcpy(eh, ent, sizeof(*ent));
+			memcpy(rh, &cmd_db_header->header[i], sizeof(*rh));
+			return 0;

This function really returns reference to data, length of the data and version. How about just making this explicit, rather than copying two sets of headers to the caller and having them extract this information?

Quoted 4 lines

I think you should turn this function into:

static void *cmd_db_find(u64 id, size_t *len, u16 *version)

I think this is much cleaner, to return the entry that we searched for, rather than return a void *.

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)

"cmd_db_read_data()" would better represent that we're copying the data into the buffer.

Quoted 3 lines

(And the "aux" part in these function names doesn't seem to add any value)

That is how it is represented in the architecture document. It is more helpful for the driver author to maintain relation with the database that is being read.

Quoted 8 lines

drivers/soc/qcom/cmd-db.c line 231

Quoted 5 lines

drivers/soc/qcom/cmd-db.c new file +5 -0

+int cmd_db_ready(void)
+{
+	return cmd_db_status;
+}
+EXPORT_SYMBOL(cmd_db_ready);

Move this function one step down, to keep the getters together.

Quoted 2 lines

Based on the function name this function should return a bool.

Moved it up as I plan to reuse this function in
cmd_db_get_header_by_rsc_id().

Quoted 4 lines

Quoted 4 lines

drivers/soc/qcom/cmd-db.c new file +4 -0

+
+int cmd_db_get_slave_id(const char *resource_id)
+{
+	int ret;

Quoted 9 lines

drivers/soc/qcom/cmd-db.c line 240

Quoted 5 lines

drivers/soc/qcom/cmd-db.c new file +5 -0

+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+	return ret < 0 ? 0 : (ent.addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;

Why is this bit 16-19 of the address? What is ent.addr?! Why isn't this rsc_hdr.slv_id?

BITS 16:18 indicate what type of slave it is - ARC, BCM, VRM. This information is part of each entry.

Quoted 7 lines

drivers/soc/qcom/cmd-db.c line 262

Quoted 4 lines

drivers/soc/qcom/cmd-db.c new file +4 -0

+	res.start = readl_relaxed(dict);
+	res.end = res.start + readl_relaxed(dict + 0x4);
+	res.flags = IORESOURCE_MEM;
+	iounmap(dict);

Why can't we just describe the memory directly?

We could. Just that it is how the memory is defined in the remote end.

Quoted 5 lines

drivers/soc/qcom/cmd-db.c line 264

Quoted 2 lines

drivers/soc/qcom/cmd-db.c new file +2 -0

+
+	start_addr = devm_ioremap_resource(&pdev->dev, &res);

No error handling?

Will fix.

Quoted 2 lines · Lina Iyer

Quoted 1 line · Bjorn Andersson

On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:

[..]

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

drivers/soc/qcom/cmd-db.c new file +6 -0

+static u64 cmd_db_get_u64_id(const char *id)
+{
+	uint64_t rsc_id = 0;
+	uint8_t *ch  = (uint8_t *)&rsc_id;
+	int i;
+

Quoted 9 lines

drivers/soc/qcom/cmd-db.c line 113

Quoted 5 lines

drivers/soc/qcom/cmd-db.c new file +5 -0

+	for (i = 0; ((i < sizeof(rsc_id)) && id[i]); i++)
+		ch[i] = id[i];
+
+	return rsc_id;
+}

So this "casts" a 8 byte string to a u64. Why not just use u64 constants to represent the resources, as we've done in the past?

Quoted 3 lines

This matches the resource specification on the remote end for this new architecture.

But are these resource constants going to be used in the Linux kernel?

In previous variants of the RPM we had these too and in some places they where described in their string variant and some in integer constants. Upstream they all became integer constants, typically with a comment.

Unless there's some code sharing here I think we should do the translation offline and encode them as hexadecimal constants.

[..]

Quoted 10 lines

Quoted 10 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)

"cmd_db_read_data()" would better represent that we're copying the data into the buffer.

(And the "aux" part in these function names doesn't seem to add any value)

Quoted 4 lines

That is how it is represented in the architecture document. It is more helpful for the driver author to maintain relation with the database that is being read.

Okay so it's not "the data" that being read but "the auxiliary data" according to the command db specification. I guess that's fine then, but think it's a "read" and not a "get" in Linux.

[..]

Quoted 8 lines

Quoted 8 lines

drivers/soc/qcom/cmd-db.c line 231

Quoted 5 lines

drivers/soc/qcom/cmd-db.c new file +5 -0

+int cmd_db_ready(void)
+{
+	return cmd_db_status;
+}
+EXPORT_SYMBOL(cmd_db_ready);

Move this function one step down, to keep the getters together.

Quoted 5 lines

Quoted 2 lines

Based on the function name this function should return a bool.

Moved it up as I plan to reuse this function in
cmd_db_get_header_by_rsc_id().

Sounds good, but I presume we still need this function exported in order for client drivers to be able to probe defer.

Quoted 13 lines

Quoted 13 lines

drivers/soc/qcom/cmd-db.c line 240

Quoted 9 lines

drivers/soc/qcom/cmd-db.c new file +9 -0

+
+int cmd_db_get_slave_id(const char *resource_id)
+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+	return ret < 0 ? 0 : (ent.addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;

Why is this bit 16-19 of the address? What is ent.addr?! Why isn't this rsc_hdr.slv_id?

Quoted 3 lines

BITS 16:18 indicate what type of slave it is - ARC, BCM, VRM. This information is part of each entry.

Okay, that sounds reasonable.

But what's the value in rsc_hdr.slv_id representing? The "id" of the resource? Why is that a "slave id"?

[..]

Quoted 9 lines

Quoted 7 lines

drivers/soc/qcom/cmd-db.c line 262

Quoted 4 lines

drivers/soc/qcom/cmd-db.c new file +4 -0

+	res.start = readl_relaxed(dict);
+	res.end = res.start + readl_relaxed(dict + 0x4);
+	res.flags = IORESOURCE_MEM;
+	iounmap(dict);

Why can't we just describe the memory directly?

We could. Just that it is how the memory is defined in the remote end.

I think it's fine that the apps system as a whole implements this functionality, but I think Linux should just have the real memory region described in the reg.

And as the reserved-memory region seems to be the only resource for this thing we should be able to describe it similar to the qcom,rmtfs-mem - where the reserved-memory node has a compatible and is implemented by the qcom_rmtfs_mem driver, without additional nodes referencing it.

Quoted 7 lines

Quoted 5 lines

drivers/soc/qcom/cmd-db.c line 264

Quoted 2 lines

drivers/soc/qcom/cmd-db.c new file +2 -0

+
+	start_addr = devm_ioremap_resource(&pdev->dev, &res);

No error handling?

Will fix.

As I said in my other reply (sorry for spreading this out), as this is system ram, this should be a memremap()

Quoted 23 lines · Bjorn Andersson

Quoted 2 lines · Lina Iyer

Quoted 1 line · Bjorn Andersson

On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:

[..]

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

drivers/soc/qcom/cmd-db.c new file +6 -0

+static u64 cmd_db_get_u64_id(const char *id)
+{
+	uint64_t rsc_id = 0;
+	uint8_t *ch  = (uint8_t *)&rsc_id;
+	int i;
+

Quoted 9 lines

drivers/soc/qcom/cmd-db.c line 113

Quoted 5 lines

drivers/soc/qcom/cmd-db.c new file +5 -0

+	for (i = 0; ((i < sizeof(rsc_id)) && id[i]); i++)
+		ch[i] = id[i];
+
+	return rsc_id;
+}

So this "casts" a 8 byte string to a u64. Why not just use u64 constants to represent the resources, as we've done in the past?

Quoted 3 lines

This matches the resource specification on the remote end for this new architecture.

Quoted 9 lines

But are these resource constants going to be used in the Linux kernel?

In previous variants of the RPM we had these too and in some places they where described in their string variant and some in integer constants. Upstream they all became integer constants, typically with a comment.

Unless there's some code sharing here I think we should do the translation offline and encode them as hexadecimal constants.

While I agree with the suggestion. I doubt this is making it any easier. The general identifier of a resource is a human readadble stringified name. It makes understanding the information regarding the resource - like the rail name, clock etc much more user friendly. I would like to keep it that way. I am quite sure, the driver authors would prefer to, as well. This makes it easier when reading through DT.

Ex: gfx.lvl, ddr.lvl, ddr.tmr as opposed to - 0x6c766c2e786667, 0x6c766c2e726464, 0x726d742e726464 respectively.

Quoted 16 lines

Quoted 15 lines

Quoted 12 lines

drivers/soc/qcom/cmd-db.c line 240

Quoted 8 lines

drivers/soc/qcom/cmd-db.c new file +8 -0

+int cmd_db_get_slave_id(const char *resource_id)
+{
+	int ret;
+	struct entry_header ent;
+	struct rsc_hdr rsc_hdr;
+
+	ret = cmd_db_get_header_by_rsc_id(resource_id, &ent, &rsc_hdr);
+	return ret < 0 ? 0 : (ent.addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;

Why is this bit 16-19 of the address? What is ent.addr?! Why isn't this rsc_hdr.slv_id?

BITS 16:18 indicate what type of slave it is - ARC, BCM, VRM. This information is part of each entry.

Quoted 5 lines

Okay, that sounds reasonable.

But what's the value in rsc_hdr.slv_id representing? The "id" of the resource? Why is that a "slave id"?

The master is the client that runs on Linux and other processors like GPU, Modem etc. The slave is the actual resource like the clock, regulator, etc. Hence the name 'slave id'.

From: Mahesh Sivasubramanian <[email protected]>

Command DB provides information on shared resources like clocks, regulators etc., probed at boot by the remote subsytem and made available in shared memory.

Cc [email protected]

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 .../devicetree/bindings/arm/msm/cmd-db.txt         | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/cmd-db.txt

Documentation/devicetree/bindings/arm/msm/cmd-db.txt new file +0 -0

new file mode 100644
index 000000000000..b56e3e3604d3

Documentation/devicetree/bindings/arm/msm/cmd-db.txt new file +26 -0

+
+Command DB allows drivers to query resource parameters based on pre-determined
+key strings.
+
+The devicetree representation of the command DB driver should be:
+
+PROPERTIES:
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Should be "qcom,cmd-db"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: First element is the base address of shared memory
+		Second element is the size of the shared memory region
+		Points to the dictionary address that houses the command DB
+		start address and the size of the command DB region
+
+Example:
+
+	qcom,cmd-db@c3f000c {
+		compatible = "qcom,cmd-db";
+		reg = <0xc3f000c 0x8>;
+	};

Signature · 7 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 14 lines · Lina Iyer

From: Mahesh Sivasubramanian <[email protected]>

Command DB provides information on shared resources like clocks, regulators etc., probed at boot by the remote subsytem and made available in shared memory.

Cc [email protected]

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 .../devicetree/bindings/arm/msm/cmd-db.txt         | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/cmd-db.txt

Documentation/devicetree/bindings/arm/msm/cmd-db.txt line 2

Quoted 8 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt new file +2 -0

new file mode 100644
index 000000000000..b56e3e3604d3
@@ -0,0 +1,37 @@
+Command DB
+---------

Another strange QCom binding...

Documentation/devicetree/bindings/arm/msm/cmd-db.txt line 6

Quoted 4 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt new file +4 -0

+
+Command DB is a database that provides a mapping between resource key and the
+resource address for a system resource managed by a remote processor. The data
+is stored in a shared memory region and is loaded by the remote processor.

Is said shared memory described in DT. If so, this should be a child node. Only 8 bytes seems kind of fine grained for putting in DT when it could be implied by the parent shared memory node.

Documentation/devicetree/bindings/arm/msm/cmd-db.txt line 11

Quoted 5 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt new file +5 -0

+
+Some of the Qualcomm Technologies Inc SoC's have hardware accelerators for
+controlling shared resources. Depending on the board configuration the shared
+resource properties may change. These properties are dynamically probed by the
+remote processor and made available in the shared memory.

The table may change, but does the presence of it or shared memory location (of the pointer) change?

Quoted 34 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt new file +26 -1

+
+Command DB allows drivers to query resource parameters based on pre-determined
+key strings.
+
+The devicetree representation of the command DB driver should be:
+
+PROPERTIES:
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Should be "qcom,cmd-db"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: First element is the base address of shared memory
+		Second element is the size of the shared memory region
+		Points to the dictionary address that houses the command DB
+		start address and the size of the command DB region
+
+Example:
+
+	qcom,cmd-db@c3f000c {
+		compatible = "qcom,cmd-db";
+		reg = <0xc3f000c 0x8>;
+	};
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 1 line · Rob Herring

On Thu, Jan 18, 2018 at 03:28:02PM -0700, Lina Iyer wrote:

Quoted 1 line

Quoted 1 line

Documentation/devicetree/bindings/arm/msm/cmd-db.txt +0 -0

Quoted 8 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt line 6

Quoted 3 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt +3 -0

+Command DB is a database that provides a mapping between resource key and the
+resource address for a system resource managed by a remote processor. The data
+is stored in a shared memory region and is loaded by the remote processor.

Is said shared memory described in DT. If so, this should be a child node. Only 8 bytes seems kind of fine grained for putting in DT when it could be implied by the parent shared memory node.

I dont believe this memory will be described in DT for this chipset. Will ask internally.

Quoted 9 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt line 11

Quoted 5 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt +5 -0

+
+Some of the Qualcomm Technologies Inc SoC's have hardware accelerators for
+controlling shared resources. Depending on the board configuration the shared
+resource properties may change. These properties are dynamically probed by the
+remote processor and made available in the shared memory.

The table may change, but does the presence of it or shared memory location (of the pointer) change?

The location may change between different SoCs, but will be present in all chipsets of this architecture.

Quoted 2 lines · Lina Iyer

Quoted 1 line · Rob Herring

On Thu, Jan 18, 2018 at 03:28:02PM -0700, Lina Iyer wrote:

[..]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Documentation/devicetree/bindings/arm/msm/cmd-db.txt +0 -0

[..]

Quoted 11 lines

Quoted 8 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt line 6

Quoted 3 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt +3 -0

+Command DB is a database that provides a mapping between resource key and the
+resource address for a system resource managed by a remote processor. The data
+is stored in a shared memory region and is loaded by the remote processor.

Is said shared memory described in DT. If so, this should be a child node. Only 8 bytes seems kind of fine grained for putting in DT when it could be implied by the parent shared memory node.

I dont believe this memory will be described in DT for this chipset. Will ask internally.

Well, these things goes two ways...

Quoted 9 lines

Quoted 9 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt line 11

Quoted 5 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt +5 -0

+
+Some of the Qualcomm Technologies Inc SoC's have hardware accelerators for
+controlling shared resources. Depending on the board configuration the shared
+resource properties may change. These properties are dynamically probed by the
+remote processor and made available in the shared memory.

The table may change, but does the presence of it or shared memory location (of the pointer) change?

Quoted 3 lines

The location may change between different SoCs, but will be present in all chipsets of this architecture.

Where is the actual DB located? System RAM or is it some special device-memory?

Quoted 33 lines · Bjorn Andersson

Quoted 2 lines · Lina Iyer

Quoted 1 line · Rob Herring

On Thu, Jan 18, 2018 at 03:28:02PM -0700, Lina Iyer wrote:

[..]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Documentation/devicetree/bindings/arm/msm/cmd-db.txt +0 -0

[..]

Quoted 11 lines

Quoted 8 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt line 6

Quoted 3 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt +3 -0

+Command DB is a database that provides a mapping between resource key and the
+resource address for a system resource managed by a remote processor. The data
+is stored in a shared memory region and is loaded by the remote processor.

Is said shared memory described in DT. If so, this should be a child node. Only 8 bytes seems kind of fine grained for putting in DT when it could be implied by the parent shared memory node.

I dont believe this memory will be described in DT for this chipset. Will ask internally.

Well, these things goes two ways...

Quoted 12 lines

Quoted 9 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt line 11

Quoted 5 lines

Documentation/devicetree/bindings/arm/msm/cmd-db.txt +5 -0

+
+Some of the Qualcomm Technologies Inc SoC's have hardware accelerators for
+controlling shared resources. Depending on the board configuration the shared
+resource properties may change. These properties are dynamically probed by the
+remote processor and made available in the shared memory.

The table may change, but does the presence of it or shared memory location (of the pointer) change?

The location may change between different SoCs, but will be present in all chipsets of this architecture.

Quoted 3 lines

Where is the actual DB located? System RAM or is it some special device-memory?

It's carved out of system memory and is access protected. Not a device memory.