On 18/07/2023 18:08, Nikunj Kela wrote:
> Add a new transport channel to the SCMI firmware interface driver for
> SCMI message exchange on Qualcomm virtual platforms.
>
> The hypervisor associates an object-id also known as capability-id
> with each hvc doorbell object. The capability-id is used to identify the
> doorbell from the VM's capability namespace, similar to a file-descriptor.
>
...
> +
> +static bool qcom_hvc_chan_available(struct device_node *of_node, int idx)
> +{
> + struct device_node *np = of_parse_phandle(of_node, "shmem", 0);
> +
> + if (!np)
> + return false;
> +
> + of_node_put(np);
> + return true;
> +}
> +
> +static inline void qcom_hvc_channel_lock_init(struct scmi_qcom_hvc *scmi_info)
> +{
> + mutex_init(&scmi_info->shmem_lock);
> +}
> +
> +static inline void
> +qcom_hvc_channel_lock_acquire(struct scmi_qcom_hvc *scmi_info,
> + struct scmi_xfer *xfer __maybe_unused)
Why do you pass unused arguments?
> +{
> + mutex_lock(&scmi_info->shmem_lock);
Why do you need these wrappers? Why not using mutexes directly?
If they are needed, then add lock __acquires annotation.
> +}
> +
> +static inline void qcom_hvc_channel_lock_release(struct scmi_qcom_hvc
> + *scmi_info)
Same comments.
> +{
> + mutex_unlock(&scmi_info->shmem_lock);
> +}
> +
> +static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo,
> + struct device *dev, bool tx)
> +{
> + struct device *cdev = cinfo->dev;
> + struct scmi_qcom_hvc *scmi_info;
> + resource_size_t size;
> + struct resource res;
> + struct device_node *np;
> + unsigned long cap_id;
> + u32 func_id;
> + int ret, irq;
> +
> + if (!tx)
> + return -ENODEV;
> +
> + scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL);
> + if (!scmi_info)
> + return -ENOMEM;
> +
> + np = of_parse_phandle(cdev->of_node, "shmem", 0);
> + if (!of_device_is_compatible(np, "arm,scmi-shmem"))
You leak here reference.
> + return -ENXIO;
> +
> + ret = of_address_to_resource(np, 0, &res);
> + of_node_put(np);
> + if (ret) {
> + dev_err(cdev, "failed to get SCMI Tx shared memory\n");
> + return ret;
> + }
> +
> + size = resource_size(&res);
> +
> + /* let's map 2 additional ulong since
> + * func-id & capability-id are kept after shmem.
> + * +-------+
> + * | |
> + * | shmem |
> + * | |
> + * | |
> + * +-------+ <-- size
> + * | funcId|
> + * +-------+ <-- size + sizeof(ulong)
> + * | capId |
> + * +-------+ <-- size + 2*sizeof(ulong)
> + */
> +
> + scmi_info->shmem = devm_ioremap(dev, res.start,
> + size + 2 * sizeof(unsigned long));
> + if (!scmi_info->shmem) {
> + dev_err(dev, "failed to ioremap SCMI Tx shared memory\n");
> + return -EADDRNOTAVAIL;
> + }
> +
> + func_id = readl((void *)(scmi_info->shmem) + size);
> +
> +#ifdef CONFIG_ARM64
> + cap_id = readq((void *)(scmi_info->shmem) + size +
> + sizeof(unsigned long));
> +#else
> + cap_id = readl((void *)(scmi_info->shmem) + size +
> + sizeof(unsigned long));
> +#endif
> +
> + /*
> + * If there is an interrupt named "a2p", then the service and
> + * completion of a message is signaled by an interrupt rather than by
> + * the return of the hvc call.
> + */
> + irq = of_irq_get_byname(cdev->of_node, "a2p");
> + if (irq > 0) {
> + ret = devm_request_irq(dev, irq, qcom_hvc_msg_done_isr,
> + IRQF_NO_SUSPEND,
> + dev_name(dev), scmi_info);
> + if (ret) {
> + dev_err(dev, "failed to setup SCMI completion irq\n");
return dev_err_probe, unless this is not called in probe... but then
using devm-interface raises questions.
Best regards,
Krzysztof
