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 16:47:19 +0000 Message-ID: <20180207164719.GA16153@codeaurora.org> References: <20180118220833.16616-1-ilina@codeaurora.org> <20180118220833.16616-2-ilina@codeaurora.org> <20180125204613.GW28313@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:50204 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754463AbeBGQrW (ORCPT ); Wed, 7 Feb 2018 11:47:22 -0500 Content-Disposition: inline In-Reply-To: <20180125204613.GW28313@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Stephen Boyd Cc: andy.gross@linaro.org, david.brown@linaro.org, rnayak@codeaurora.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, msivasub@codeaurora.org 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 >> +#include > >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