On 7/18/2023 11:17 AM, Krzysztof Kozlowski wrote:
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?
I mimic the same behavior as in smc.c. At the moment, we don't have a
use case for atomicity but was trying to keep the same skeleton in case
we need to add the atomic support in future. But you are right, I don't
need wrapper at the moment.
+{
+ 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.
Will remove the wrappers.
+}
+
+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.
Wouldn't the devm_* API take care of that implicitly? It is same in
smc.c as well.
+ 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.
This is copied as is from existing smc.c
Best regards,
Krzysztof
