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