Re: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport

On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote:

On 7/18/2023 11:29 AM, Bjorn Andersson wrote:

quoted

On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:

quoted

diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c
new file mode 100644
index 000000000000..3b6096d8fe67
--- /dev/null
+++ b/drivers/firmware/arm_scmi/qcom_hvc.c

@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Message
+ * Qualcomm HVC/shmem Transport driver
+ *
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright 2020 NXP
+ *
+ * This is copied from drivers/firmware/arm_scmi/smc.c

s/copied from/based on/

ok.

Hi Nikunj,

quoted

quoted

+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>

[snip]

quoted

quoted

+
+static inline void
+qcom_hvc_channel_lock_acquire(struct scmi_qcom_hvc *scmi_info,
+			      struct scmi_xfer *xfer __maybe_unused)
+{

You claim above that you copied smc.c, but you don't mention that you
dropped the support for transfers from atomic mode. Please capture this
in the commit message, so that someone looking at this in the future
knows why you made this choice.

At the moment, we dont have any requirement to support atomicity. Will add a
comment in the commit message.

As said no atomic support no wrappers needed.

quoted

quoted

+	mutex_lock(&scmi_info->shmem_lock);
+}
+
+static inline void qcom_hvc_channel_lock_release(struct scmi_qcom_hvc
+						 *scmi_info)
+{
+	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;

Please declare one variable per line, and please sort them by length, in
descending order (i.e. reverse Christmas tree).

ok

quoted

quoted

+
+	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"))
+		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)

Relying on an undocumented convention that following the region
specified in DeviceTree are two architecture specifically sized integers
isn't good practice.

This should be covered by the DeviceTree binding, in one way or another.

ok. Usually, DTBs don't allow non-hw properties in the dtb. I can try adding
a property as cap-id-width if its allowed.

This is sort of transport configuration so maybe it could be placed on a
shmem on its own, but it seems difficult that the binding can be accepted
since, as you said, is not an HW description BUT indeed configuration.

quoted

quoted

+	 */
+
+	scmi_info->shmem = devm_ioremap(dev, res.start,
+					size + 2 * sizeof(unsigned long));

I don't find any code that uses the size of the defined shm, so I don't
think you need to do this dance.

Right! I can remove the addition part.

quoted

Mmm...but can you access this trailing config bytes if you dont ioremap it ?

quoted

quoted

+	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

Please don't make the in-memory representation depend on architecture
specific data types. Quite likely you didn't compile test one of these
variants?

Just define the in-memory representation as u32 + u64.

I tested this for ARM64, I didn't test it for 32bit since Hypervisor doesn't
support it currently. In future, it may add 32 bit support too.

quoted

quoted

+
+	/*
+	 * 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 ret;
+		}
+	} else {
+		cinfo->no_completion_irq = true;
+	}
+
+	scmi_info->func_id = func_id;
+	scmi_info->cap_id = cap_id;
+	scmi_info->cinfo = cinfo;
+	qcom_hvc_channel_lock_init(scmi_info);
+	cinfo->transport_info = scmi_info;
+
+	return 0;
+}
+
+static int qcom_hvc_chan_free(int id, void *p, void *data)
+{
+	struct scmi_chan_info *cinfo = p;
+	struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
+
+	cinfo->transport_info = NULL;
+	scmi_info->cinfo = NULL;

Your a2p interrupt is cleaned up using devres, which will happen at a
later point. So just setting cinfo to NULL here would cause an interrupt
to trigger qcom_hvc_msg_done_isr() which will call scmi_rx_callback()
which happily will dereference that NULL.

Ok, will add a check in ISR.

quoted

Other transports here takes care to block/inhibit any further possible
message reception with a transport/subsystem dependent method (like masking
the IRQ calling into mbox subsys or breaking the virtio device); I suppose
here you could also unregister the ISR before clearing to NULL.
(and I'll need to post a similar fix for SMC...)

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel