From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lina Iyer Subject: Re: [PATCH 1/2] drivers: qcom: add command DB driver Date: Wed, 7 Feb 2018 18:29:42 +0000 Message-ID: <20180207182942.GD16153@codeaurora.org> References: <20180118220833.16616-1-ilina@codeaurora.org> <20180118220833.16616-2-ilina@codeaurora.org> <20180205231835.GF9465@builder> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:53840 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753794AbeBGS3o (ORCPT ); Wed, 7 Feb 2018 13:29:44 -0500 Content-Disposition: inline In-Reply-To: <20180205231835.GF9465@builder> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Bjorn Andersson Cc: andy.gross@linaro.org, david.brown@linaro.org, sboyd@codeaurora.org, rnayak@codeaurora.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, msivasub@codeaurora.org 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