[PATCH 0/2] drivers/qcom: add Command DB support

[PATCH 0/2] drivers/qcom: add Command DB support

[Lina Iyer]

Hi,

These patches add support for reading a shared memory database in the newer
QCOM SoCs called Command DB. With the new architecture on SDM845, shared
resources like clocks, regulators etc have dynamic properties. These properties
may change based on external components, board configurations or available
feature set. A remote processor detects these parameters and fills up the
database with the resource and available state information. Platform drivers
that need these shared resources will need to query this database to get the
address and properties and vote for the state. The information in the database
is static.

The database is read-only memory location that is availble for Linux. A
pre-defined string is used as a key into an entry in the database. Generally,
platform drivers query the database only at init to get the information they
need.

Thanks,
Lina

Lina Iyer (2):
  drivers: qcom: add command DB driver
  dt-bindings: introduce Command DB for QCOM SoCs

 .../devicetree/bindings/arm/msm/cmd-db.txt         |  37 +++
 drivers/soc/qcom/Kconfig                           |   7 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/cmd-db.c                          | 306 +++++++++++++++++++++
 include/soc/qcom/cmd-db.h                          | 119 ++++++++
 5 files changed, 470 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/cmd-db.txt
 create mode 100644 drivers/soc/qcom/cmd-db.c
 create mode 100644 include/soc/qcom/cmd-db.h

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

[PATCH 1/2] drivers: qcom: add command DB driver

[Lina Iyer]

From: Mahesh Sivasubramanian <msivasub@codeaurora.org>

Command DB is a simple database in the shared memory of QCOM SoCs, that
provides information regarding shared resources. Some shared resources
in the SoC have properties that are probed dynamically at boot by the
remote processor. The information pertaining to the SoC and the platform
are made available in the shared memory. Drivers can query this
information using predefined strings.

Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/soc/qcom/Kconfig  |   7 ++
 drivers/soc/qcom/Makefile |   1 +
 drivers/soc/qcom/cmd-db.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/cmd-db.h | 119 ++++++++++++++++++
 4 files changed, 433 insertions(+)
 create mode 100644 drivers/soc/qcom/cmd-db.c
 create mode 100644 include/soc/qcom/cmd-db.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index b81374bb6713..f21c5d53e721 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -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.
 
+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
+
 endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 40c56f67e94a..7b64135b22eb 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -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
diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
new file mode 100644
index 000000000000..eb10ea8cf963
--- /dev/null
+++ b/drivers/soc/qcom/cmd-db.c
@@ -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
+ * 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/module.h>
+#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
+ * @addr: the address of the resource
+ * @len: length of the data
+ * @offset: offset at which dats starts
+ */
+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];
+};
+
+/**
+ * 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;
+	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
+ * @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;
+	u16 version;
+	u8  data[];
+};
+
+static void __iomem *start_addr;
+static struct cmd_db_header *cmd_db_header;
+static int cmd_db_status = -EPROBE_DEFER;
+
+static u64 cmd_db_get_u64_id(const char *id)
+{
+	uint64_t rsc_id = 0;
+	uint8_t *ch  = (uint8_t *)&rsc_id;
+	int i;
+
+	for (i = 0; ((i < sizeof(rsc_id)) && id[i]); i++)
+		ch[i] = id[i];
+
+	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;
+
+	rsc_hdr = &cmd_db_header->header[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);
+
+		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)
+		return ret;
+
+	if (ent.len < len)
+		return -EINVAL;
+
+	len = (ent.len < len) ? ent.len : len;
+
+	memcpy_fromio(data,
+			start_addr + sizeof(*cmd_db_header)
+			+ rsc_hdr.data_offset + ent.offset,
+			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;
+	}
+
+	/*
+	 * 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);
+
+	start_addr = devm_ioremap_resource(&pdev->dev, &res);
+
+	cmd_db_header = devm_kzalloc(&pdev->dev,
+			sizeof(*cmd_db_header), GFP_KERNEL);
+
+	if (!cmd_db_header) {
+		cmd_db_status = -ENOMEM;
+		goto failed;
+	}
+
+	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__);
+		cmd_db_status = -EINVAL;
+		goto failed;
+	}
+	cmd_db_status = 0;
+	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+
+failed:
+	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);
diff --git a/include/soc/qcom/cmd-db.h b/include/soc/qcom/cmd-db.h
new file mode 100644
index 000000000000..1e42f7509cf9
--- /dev/null
+++ b/include/soc/qcom/cmd-db.h
@@ -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
+ */
+int cmd_db_get_slave_id(const char *resource_id);
+
+#else
+
+static inline u32 cmd_db_get_addr(const char *resource_id)
+{
+	return 0;
+}
+
+int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)
+{
+	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__ */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] dt-bindings: introduce Command DB for QCOM SoCs

[Lina Iyer]

From: Mahesh Sivasubramanian <msivasub@codeaurora.org>

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: devicetree@vger.kernel.org
Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 .../devicetree/bindings/arm/msm/cmd-db.txt         | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/cmd-db.txt

diff --git a/Documentation/devicetree/bindings/arm/msm/cmd-db.txt b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
new file mode 100644
index 000000000000..b56e3e3604d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
@@ -0,0 +1,37 @@
+Command DB
+---------
+
+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.
+
+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.
+
+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

[PATCH 2/2] dt-bindings: introduce Command DB for QCOM SoCs

[Lina Iyer]

From: Mahesh Sivasubramanian <msivasub@codeaurora.org>

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: devicetree@vger.kernel.org
Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 .../devicetree/bindings/arm/msm/cmd-db.txt         | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/cmd-db.txt

diff --git a/Documentation/devicetree/bindings/arm/msm/cmd-db.txt b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
new file mode 100644
index 000000000000..b56e3e3604d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
@@ -0,0 +1,37 @@
+Command DB
+---------
+
+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.
+
+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.
+
+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

Re: [PATCH 1/2] drivers: qcom: add command DB driver

[Stephen Boyd]

On 01/18, Lina Iyer wrote:
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index b81374bb6713..f21c5d53e721 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -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.
>  
> +config QCOM_COMMAND_DB
> +	bool "Qualcomm Command DB"
> +	depends on ARCH_QCOM

Add || COMPILE_TEST?

> +	help
> +	  Command DB queries shared memory by key string for shared system
> +	  resources
> +
>  endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 40c56f67e94a..7b64135b22eb 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -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
> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
> new file mode 100644
> index 000000000000..eb10ea8cf963
> --- /dev/null
> +++ b/drivers/soc/qcom/cmd-db.c
> @@ -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
> + * 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/module.h>

It's not a module.

> +#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
> + * @addr: the address of the resource
> + * @len: length of the data
> + * @offset: offset at which dats starts
> + */
> +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.

> +};
> +
> +/**
> + * 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];

Same comment.

> +};
> +
> +/**
> + * 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;
> +	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.

> +};
> +
> +/**
> + * cmd_db_entry: Inforamtion for each line in the cmd db
> + *
> + * @resource_id: unique identifier for each entry
> + * @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?

> +	u16 version;

Endian.

> +	u8  data[];
> +};
> +
> +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.

> +
> +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.

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

Drop useless parenthesis.

> +		ch[i] = id[i];

Is this a strcpy?

> +
> +	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;
> +
> +	rsc_hdr = &cmd_db_header->header[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

Useless cast?

> +				+ 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)
> +		return ret;
> +
> +	if (ent.len < len)
> +		return -EINVAL;
> +
> +	len = (ent.len < len) ? ent.len : len;
> +
> +	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?

> +			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;
> +	}
> +
> +	/*
> +	 * 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);
> +
> +	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.

> +
> +	cmd_db_header = devm_kzalloc(&pdev->dev,
> +			sizeof(*cmd_db_header), GFP_KERNEL);
> +
> +	if (!cmd_db_header) {
> +		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?

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

memcmp?

> +		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?

> +
> +failed:
> +	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,

Drop this.

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

static? Please run sparse.

> +{
> +	return platform_driver_register(&cmd_db_dev_driver);
> +}
> +arch_initcall(cmd_db_device_init);
> diff --git a/include/soc/qcom/cmd-db.h b/include/soc/qcom/cmd-db.h
> new file mode 100644
> index 000000000000..1e42f7509cf9
> --- /dev/null
> +++ b/include/soc/qcom/cmd-db.h
> @@ -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
> + */
> +int cmd_db_get_slave_id(const char *resource_id);
> +
> +#else
> +
> +static inline u32 cmd_db_get_addr(const char *resource_id)
> +{
> +	return 0;
> +}
> +
> +int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)

All these below are missing static inline?

> +{
> +	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__ */
> 

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

Re: [PATCH 2/2] dt-bindings: introduce Command DB for QCOM SoCs

[Rob Herring]

On Thu, Jan 18, 2018 at 03:28:02PM -0700, Lina Iyer wrote:
> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> 
> 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: devicetree@vger.kernel.org
> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  .../devicetree/bindings/arm/msm/cmd-db.txt         | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/msm/cmd-db.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/msm/cmd-db.txt b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
> new file mode 100644
> index 000000000000..b56e3e3604d3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
> @@ -0,0 +1,37 @@
> +Command DB
> +---------

Another strange QCom binding...

> +
> +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.

> +
> +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?

> +
> +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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] dt-bindings: introduce Command DB for QCOM SoCs

[Lina Iyer]

Thanks Rob, for taking time to review these bindings.

On Mon, Jan 29 2018 at 19:08 +0000, Rob Herring wrote:
>On Thu, Jan 18, 2018 at 03:28:02PM -0700, Lina Iyer wrote:
>> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>
>> 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: devicetree@vger.kernel.org
>> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  .../devicetree/bindings/arm/msm/cmd-db.txt         | 37 ++++++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/msm/cmd-db.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/msm/cmd-db.txt b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
>> new file mode 100644
>> index 000000000000..b56e3e3604d3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
>> @@ -0,0 +1,37 @@
>> +Command DB
>> +---------
>
>Another strange QCom binding...
>
:)

>> +
>> +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.

>> +
>> +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.

Thanks,
Lina

>> +
>> +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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] dt-bindings: introduce Command DB for QCOM SoCs

[Bjorn Andersson]

On Tue 30 Jan 08:17 PST 2018, Lina Iyer wrote:
> On Mon, Jan 29 2018 at 19:08 +0000, Rob Herring wrote:
> > On Thu, Jan 18, 2018 at 03:28:02PM -0700, Lina Iyer wrote:
[..]
> > > diff --git a/Documentation/devicetree/bindings/arm/msm/cmd-db.txt b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
[..]
> > > +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...

> > > +
> > > +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.
> 

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

Regards,
Bjorn

Re: [PATCH 1/2] drivers: qcom: add command DB driver

[Bjorn Andersson]

On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index b81374bb6713..f21c5d53e721 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -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.
>  
> +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.

>  endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 40c56f67e94a..7b64135b22eb 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -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
> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
[..]
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#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

Only used in unused structure below?

> +#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.

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

"id" is shorter and just as descriptive.

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

"data"

> + */
> +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];
> +};
> +
> +/**
> + * 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.

> + */
> +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
> + * @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?

> +	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...

> +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...

> +
> +static u64 cmd_db_get_u64_id(const char *id)
> +{
> +	uint64_t rsc_id = 0;
> +	uint8_t *ch  = (uint8_t *)&rsc_id;
> +	int i;
> +
> +	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?

> +
> +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.

> +{
> +	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];

> +
> +	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;

> +
> +		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)

> +		}
> +	}
> +	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);

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 *.

> +
> +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)

> +{
> +	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)
> +		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

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

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

> +
> +	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?

> +	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.

> +{
> +	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);

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

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

> +
> +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?

> +}
> +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.

> +
> +	/*
> +	 * 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?

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

No error handling?

> +
> +	cmd_db_header = devm_kzalloc(&pdev->dev,
> +			sizeof(*cmd_db_header), GFP_KERNEL);
> +
> +	if (!cmd_db_header) {
> +		cmd_db_status = -ENOMEM;
> +		goto failed;
> +	}
> +
> +	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__

> +		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.

> +
> +failed:

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

> +	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);
> diff --git a/include/soc/qcom/cmd-db.h b/include/soc/qcom/cmd-db.h
[..]
> +#ifdef CONFIG_QCOM_COMMAND_DB

#if IS_ENABLED(CONFIG_QCOM_COMMAND_DB)

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

Move the kerneldoc to the implementation.

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

The description should come after the list of parameters.

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

Drop the extra spaces before and after @resource_id.

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

Return: 


In what address space does the return value live?

> + */
> +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

No, this returns 0 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);

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

> +
> +/**
> + * 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

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

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

Regards,
Bjorn

Re: [PATCH 2/2] dt-bindings: introduce Command DB for QCOM SoCs

[Lina Iyer]

On Mon, Feb 05 2018 at 22:11 +0000, Bjorn Andersson wrote:
>On Tue 30 Jan 08:17 PST 2018, Lina Iyer wrote:
>> On Mon, Jan 29 2018 at 19:08 +0000, Rob Herring wrote:
>> > On Thu, Jan 18, 2018 at 03:28:02PM -0700, Lina Iyer wrote:
>[..]
>> > > diff --git a/Documentation/devicetree/bindings/arm/msm/cmd-db.txt b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
>[..]
>> > > +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...
>
>> > > +
>> > > +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.
>>
>
>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.

-- Lina

--
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 2/2] dt-bindings: introduce Command DB for QCOM SoCs

[Bjorn Andersson]

On Tue 06 Feb 12:05 PST 2018, Lina Iyer wrote:

> On Mon, Feb 05 2018 at 22:11 +0000, Bjorn Andersson wrote:
> > On Tue 30 Jan 08:17 PST 2018, Lina Iyer wrote:
> > > On Mon, Jan 29 2018 at 19:08 +0000, Rob Herring wrote:
> > > > On Thu, Jan 18, 2018 at 03:28:02PM -0700, Lina Iyer wrote:
> > [..]
> > > > > diff --git a/Documentation/devicetree/bindings/arm/msm/cmd-db.txt b/Documentation/devicetree/bindings/arm/msm/cmd-db.txt
> > [..]
> > > > > +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...
> > 
> > > > > +
> > > > > +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.
> > > 
> > 
> > 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.
> 

It sounds like this mimics the model we have for SMEM then, a chunk of
System RAM with an address and size described in IMEM (iirc). The system
integrator can move or resize SMEM by just modifying the reference in
IMEM and all systems in the SoC will adapt.

Except for the fact that once we reach the point in Linux where we read
out the references Linux has already started using the part of System
RAM that's associated with SMEM - so we must also describe it in a
reserved-memory node; making the indirection useless for Linux.

Note that the boot loader could still read the indirection and adjust
the reserved-memory node accordingly - so it's useful from a systems
perspective.


If this is the case for Command DB then I suggest that we describe it as
an explicit reserved-memory directly and just use that region, rather
than the indirection.

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 1/2] drivers: qcom: add command DB driver

[Lina Iyer]

On Thu, Jan 25 2018 at 20:46 +0000, Stephen Boyd wrote:
>On 01/18, Lina Iyer wrote:
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index b81374bb6713..f21c5d53e721 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -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.
>>
>> +config QCOM_COMMAND_DB
>> +	bool "Qualcomm Command DB"
>> +	depends on ARCH_QCOM
>
>Add || COMPILE_TEST?
>
Sure.

>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>
>It's not a module.
>
Ok. Removed.

>> +/**
>> + * 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
>> + */
>> +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?

>> +};
>> +
>> +/**
>> + * 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];
>
>Same comment.
>
>> +};
>> +
>> +/**
>> + * 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;
>> +	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.
>
I don't believe it will. Any ideas to ensure that it won't?

>> +};
>> +
>> +/**
>> + * cmd_db_entry: Inforamtion for each line in the cmd db
>> + *
>> + * @resource_id: unique identifier for each entry
>> + * @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?
>
>> +	u16 version;
>
>Endian.
>
>> +	u8  data[];
>> +};
Removing this unused structure.

>> +
>> +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.
>
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.

>> +
>> +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.
>
Ok.

>> +	int i;
>> +
>> +	for (i = 0; ((i < sizeof(rsc_id)) && id[i]); i++)
>
>Drop useless parenthesis.
>
Ok.

>> +		ch[i] = id[i];
>
>Is this a strcpy?
>
In a way yes.

>> +	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?
>
Sure.

>> +	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.
>
Ok.

>> +
>> +	cmd_db_header = devm_kzalloc(&pdev->dev,
>> +			sizeof(*cmd_db_header), GFP_KERNEL);
>> +
>> +	if (!cmd_db_header) {
>> +		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?
>
Correct. Will take care of it.

>> +
>> +	if (cmd_db_header->magic_num != CMD_DB_MAGIC) {
>
>memcmp?
>
It's just a number. Simple comparision should do.

>> +		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?
>
We don't need it with the model we are using.

>> +
>> +failed:
>> +	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,
>
>Drop this.
>
OK.

>> +		.of_match_table = cmd_db_match_table,
>> +	},
>> +};
>> +
>> +int __init cmd_db_device_init(void)
>
>static? Please run sparse.
>
Ok.

>> +int cmd_db_get_aux_data(const char *resource_id, u8 *data, int len)
>
>All these below are missing static inline?
>
Yes, they should be static. Will fix.

>> +{
>> +	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__ */
>>
Thanks,
Lina

Re: [PATCH 1/2] drivers: qcom: add command DB driver

[Bjorn Andersson]

On Wed 07 Feb 08:47 PST 2018, Lina Iyer wrote:
> On Thu, Jan 25 2018 at 20:46 +0000, Stephen Boyd wrote:
> > On 01/18, Lina Iyer wrote:
> > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
[..]
> > > +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.

Forgot to mention this based on your other answer, but as this is not
device memory you should remove the __iomem specifier and use memremap()
instead of ioremap().

> Do we need to worry about endianness?
> 

We have done that in many of the other core drivers, I do however
suspect that Stephen was the only one who ever booted the kernel in this
mode.

[..]
> > 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.
> 

I think it's fine to not describe this relationship in DT. We can use
probe deferral without it.

[..]
> > > +		ch[i] = id[i];
> > 
> > Is this a strcpy?
> > 
> In a way yes.
> 

I think we should use 64-bit constants throughout the kernel instead of
using strings that gets converted here...

Regards,
Bjorn

Re: [PATCH 1/2] drivers: qcom: add command DB driver

[Lina Iyer]

On Mon, Feb 05 2018 at 23:18 +0000, Bjorn Andersson wrote:
>On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index b81374bb6713..f21c5d53e721 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -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.
>>
>> +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.
>
Sure.

>> +#define RESOURCE_ID_LEN		8
>
>Only used in unused structure below?
>
Removed along with the unused structure.

>> +#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.
>
I will try to add something. Not sure how much detail I can get into.

>> +/**
>> + * entry_header: header for each entry in cmddb
>> + *
>> + * @res_id: resource's identifier
>
>"id" is shorter and just as descriptive.
>
Ok

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

>> + */
>> +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];
>> +};
>> +
>> +/**
>> + * 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.
>
Will add.

>> + */
>> +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
>> + * @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?
>
Reminiscent of some old code. Will remove.

>> +	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...
>
I will try and reorganize. But I would like to keep the data structure
definition the same as it is here. It helps to be in parity with the
non-linux end that populates the data.

>> +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...
>
Will remove.

>> +
>> +static u64 cmd_db_get_u64_id(const char *id)
>> +{
>> +	uint64_t rsc_id = 0;
>> +	uint8_t *ch  = (uint8_t *)&rsc_id;
>> +	int i;
>> +
>> +	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.

>> +
>> +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.
>> +{
>> +	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];
>
Sure.

>> +
>> +	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;
>
>> +
>> +		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)
>
I think this is much cleaner, to return the entry that we searched for,
rather than return a void *.

>> +		}
>> +	}
>> +	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);
>
>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 *.
>
The original author chose to document the functions in the header file.
Will move to the .c file.

>> +
>> +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)
>
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.

>> +{
>> +	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)
>> +		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
>
Yes. Will fix.

>> +
>> +	len = (ent.len < len) ? ent.len : len;
>
>In particular as we here will make len = MIN(ent.len, len);
>
yes.
>> +
>> +	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?
>
memcpy should do.

>> +	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.
>
Makes sense. Will fix.
>> +{
>> +	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);
>
>Move this function one step down, to keep the getters together.
>
>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().

>> +
>> +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.

>> +}
>> +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.
>
Okay
>> +
>> +	/*
>> +	 * 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?
>
We could. Just that it is how the memory is defined in the remote end.

>> +
>> +	start_addr = devm_ioremap_resource(&pdev->dev, &res);
>
>No error handling?
>
Will fix.

>> +
>> +	cmd_db_header = devm_kzalloc(&pdev->dev,
>> +			sizeof(*cmd_db_header), GFP_KERNEL);
>> +
>> +	if (!cmd_db_header) {
>> +		cmd_db_status = -ENOMEM;
>> +		goto failed;
>> +	}
>> +
>> +	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__
>
OK.
>> +		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.
>
Not needed. Will remove.

>> +
>> +failed:
>
>This is not a good name for a label that is used for successful returns
>as well.
>
:)

>> +	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);
>> diff --git a/include/soc/qcom/cmd-db.h b/include/soc/qcom/cmd-db.h
>[..]
>> +#ifdef CONFIG_QCOM_COMMAND_DB
>
>#if IS_ENABLED(CONFIG_QCOM_COMMAND_DB)
>
>> +/**
>> + * cmd_db_get_addr() - Query command db for resource id address.
>
>Move the kerneldoc to the implementation.
>
>> + *
>> + *  This is used to retrieve resource address based on resource
>> + *  id.
>
>The description should come after the list of parameters.
>
OK
>> + *
>> + *  @resource_id : resource id to query for address
>
>Drop the extra spaces before and after @resource_id.
>
OK
>> + *
>> + *  returns resource address on success or 0 on error otherwise
>
>Return:
>
>
>In what address space does the return value live?
>
The address is a unique identifier for the resource. Its a 32 bit value.

>> + */
>> +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
>
>No, this returns 0 on error.
>
Thanks, good catch.
>> + */
>> +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);
>
>If the two pieces is to be used separately then pass u8 *msg, u8 *lsb to
>the function and fill these out.
>
This function doesnt seem to be used downstream. Will remove.
>> +
>> +/**
>> + * 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
>
>This returns 0 on error, so you could make the return type enum
>cmd_db_hw_type to clarify the interface.
>
OK
>> + */
>> +int cmd_db_get_slave_id(const char *resource_id);
>
>Regards,
>Bjorn
Thanks for the review Bjorn.

--Lina

Re: [PATCH 1/2] drivers: qcom: add command DB driver

[Bjorn Andersson]

On Wed 07 Feb 10:29 PST 2018, Lina Iyer wrote:
> On Mon, Feb 05 2018 at 23:18 +0000, Bjorn Andersson wrote:
> > On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:
[..]
> > > +static u64 cmd_db_get_u64_id(const char *id)
> > > +{
> > > +	uint64_t rsc_id = 0;
> > > +	uint8_t *ch  = (uint8_t *)&rsc_id;
> > > +	int i;
> > > +
> > > +	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.
> 

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.

> > > +
> > > +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.
> > > +{
> > > +	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];
> > 
> Sure.
> 
> > > +
> > > +	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;
> > 
> > > +
> > > +		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)
> > 
> I think this is much cleaner, to return the entry that we searched for,
> rather than return a void *.
> 

It might be that I misunderstand the in-memory data structures. But as
it seems you really need to return both structs and then have the client
do math it seemed easier to just pick the values we know the accessors
are interested in and return those...

[..]
> > > +
> > > +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)
> > 
> 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.

[..]
> > > +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.
> > 
> 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.

> > > +
> > > +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.
> 

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"?

[..]
> > > +	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.

> > > +
> > > +	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()

Regards,
Bjorn

Re: [PATCH 1/2] drivers: qcom: add command DB driver

[Lina Iyer]

On Wed, Feb 07 2018 at 21:06 +0000, Bjorn Andersson wrote:
>On Wed 07 Feb 10:29 PST 2018, Lina Iyer wrote:
>> On Mon, Feb 05 2018 at 23:18 +0000, Bjorn Andersson wrote:
>> > On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:
>[..]
>> > > +static u64 cmd_db_get_u64_id(const char *id)
>> > > +{
>> > > +	uint64_t rsc_id = 0;
>> > > +	uint8_t *ch  = (uint8_t *)&rsc_id;
>> > > +	int i;
>> > > +
>> > > +	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.
>>
>
>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.

>[..]
>> > > +
>> > > +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)
>> >
>> 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.
>
I can change to read_ instead of get_.

>[..]
>> > > +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.
>> >
>> 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.
>
Yes.

>> > > +
>> > > +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.
>>
>
>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'.

>[..]
>> > > +	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.
>
Saw your other email in this regard. Will use reserved memory node.

>> > > +
>> > > +	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()
>
Sure.

Thanks,
Lina

Re: [PATCH 1/2] drivers: qcom: add command DB driver

[Bjorn Andersson]

On Wed 07 Feb 13:43 PST 2018, Lina Iyer wrote:

> On Wed, Feb 07 2018 at 21:06 +0000, Bjorn Andersson wrote:
> > On Wed 07 Feb 10:29 PST 2018, Lina Iyer wrote:
> > > On Mon, Feb 05 2018 at 23:18 +0000, Bjorn Andersson wrote:
> > > > On Thu 18 Jan 14:08 PST 2018, Lina Iyer wrote:
> > [..]
> > > > > +static u64 cmd_db_get_u64_id(const char *id)
> > > > > +{
> > > > > +	uint64_t rsc_id = 0;
> > > > > +	uint8_t *ch  = (uint8_t *)&rsc_id;
> > > > > +	int i;
> > > > > +
> > > > > +	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.
> > > 
> > 
> > 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.
> 

The way that upstream regulator, clock and the current direction
interconnect is taken these constants are not visible in DT.

The way we have dealt with these constants previously (it's pretty much
the same with previous RPM versions) is to used defines in those
drivers.  E.g. RPM_KEY_{SWEN,UV,MA} in qcom_smd-regulator.c


That said, I don't have a strong opinion about this so feel free to
retain this model if you think it helps those drivers.

[..]
> > > > > +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.
> > > 
> > 
> > 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'.
> 

Okay, that does make sense.

But if a "slave" is a "resource" it would be convenient if the driver
picked one of the names and stuck with that.

And it sounds like this function should be named get_slave_type() (or
get_resource_type()).


Thanks,
Bjorn