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

[Stephen Boyd <sboyd@codeaurora.org>]

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