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
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
