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

[Lina Iyer <ilina@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 <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