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.
The hypervisor, in addition to the function-id, expects the capability-id
to be passed in x1 register when HVC call is invoked.
The qcom hvc doorbell/shared memory transport uses a statically defined
shared memory region that binds with "arm,scmi-shmem" device tree node.
The function-id & capability-id are allocated by the hypervisor on bootup
and are stored in the shmem region by the firmware before starting Linux.
Signed-off-by: Nikunj Kela <[email protected]>
---
drivers/firmware/arm_scmi/Kconfig | 13 ++
drivers/firmware/arm_scmi/Makefile | 1 +
drivers/firmware/arm_scmi/common.h | 3 +
drivers/firmware/arm_scmi/driver.c | 4 +
drivers/firmware/arm_scmi/qcom_hvc.c | 241 +++++++++++++++++++++++++++
5 files changed, 262 insertions(+)
create mode 100644 drivers/firmware/arm_scmi/qcom_hvc.c
diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index ea0f5083ac47..40d07329ebf7 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -99,6 +99,19 @@ config ARM_SCMI_TRANSPORT_OPTEE
If you want the ARM SCMI PROTOCOL stack to include support for a
transport based on OP-TEE SCMI service, answer Y.
+config ARM_SCMI_TRANSPORT_QCOM_HVC
+ bool "SCMI transport based on hvc doorbell & shmem for Qualcomm SoCs"
+ depends on ARCH_QCOM
+ select ARM_SCMI_HAVE_TRANSPORT
+ select ARM_SCMI_HAVE_SHMEM
+ default y
+ help
+ Enable hvc doorbell & shmem based transport for SCMI.
+
+ If you want the ARM SCMI PROTOCOL stack to include support for a
+ hvc doorbell and shmem transport on Qualcomm virtual platforms,
+ answer Y.
+
config ARM_SCMI_TRANSPORT_SMC
bool "SCMI transport based on SMC"
depends on HAVE_ARM_SMCCC_DISCOVERY
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index b31d78fa66cc..ba1ff5893ec0 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -10,6 +10,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
+scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_QCOM_HVC) += qcom_hvc.o
scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index c46dc5215af7..5c98cbb1278b 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -298,6 +298,9 @@ extern const struct scmi_desc scmi_virtio_desc;
#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE
extern const struct scmi_desc scmi_optee_desc;
#endif
+#ifdef CONFIG_ARM_SCMI_TRANSPORT_QCOM_HVC
+extern const struct scmi_desc scmi_qcom_hvc_desc;
+#endif
void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr, void *priv);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index b5957cc12fee..c54519596c29 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2918,6 +2918,10 @@ static const struct of_device_id scmi_of_match[] = {
#endif
#ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO
{ .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc},
+#endif
+#ifdef CONFIG_ARM_SCMI_TRANSPORT_QCOM_HVC
+ { .compatible = "qcom,scmi-hvc-shmem",
+ .data = &scmi_qcom_hvc_desc },
#endif
{ /* Sentinel */ },
};
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
+ */
+
+#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>
+#include <linux/of_irq.h>
+#include <linux/processor.h>
+#include <linux/slab.h>
+
+#include "common.h"
+
+/**
+ * struct scmi_qcom_hvc - Structure representing a SCMI qcom hvc transport
+ *
+ * @cinfo: SCMI channel info
+ * @shmem: Transmit/Receive shared memory area
+ * @shmem_lock: Lock to protect access to Tx/Rx shared memory area.
+ * @func_id: hvc call function-id
+ * @cap_id: hvc doorbell's capability id
+ */
+
+struct scmi_qcom_hvc {
+ struct scmi_chan_info *cinfo;
+ struct scmi_shared_mem __iomem *shmem;
+ /* Protect access to shmem area */
+ struct mutex shmem_lock;
+ u32 func_id;
+ unsigned long cap_id;
+};
+
+static irqreturn_t qcom_hvc_msg_done_isr(int irq, void *data)
+{
+ struct scmi_qcom_hvc *scmi_info = data;
+
+ scmi_rx_callback(scmi_info->cinfo,
+ shmem_read_header(scmi_info->shmem), NULL);
+
+ return IRQ_HANDLED;
+}
+
+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)
+{
+ 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;
+
+ 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)
+ */
+
+ 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 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;
+
+ return 0;
+}
+
+static int qcom_hvc_send_message(struct scmi_chan_info *cinfo,
+ struct scmi_xfer *xfer)
+{
+ struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
+ struct arm_smccc_res res;
+
+ /*
+ * Channel will be released only once response has been
+ * surely fully retrieved, so after .mark_txdone()
+ */
+ qcom_hvc_channel_lock_acquire(scmi_info, xfer);
+
+ shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
+
+ arm_smccc_1_1_hvc(scmi_info->func_id, scmi_info->cap_id,
+ 0, 0, 0, 0, 0, 0, &res);
+
+ /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
+ if (res.a0) {
+ qcom_hvc_channel_lock_release(scmi_info);
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static void qcom_hvc_fetch_response(struct scmi_chan_info *cinfo,
+ struct scmi_xfer *xfer)
+{
+ struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
+
+ shmem_fetch_response(scmi_info->shmem, xfer);
+}
+
+static void qcom_hvc_mark_txdone(struct scmi_chan_info *cinfo, int ret,
+ struct scmi_xfer *__unused)
+{
+ struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
+
+ qcom_hvc_channel_lock_release(scmi_info);
+}
+
+static const struct scmi_transport_ops scmi_qcom_hvc_ops = {
+ .chan_available = qcom_hvc_chan_available,
+ .chan_setup = qcom_hvc_chan_setup,
+ .chan_free = qcom_hvc_chan_free,
+ .send_message = qcom_hvc_send_message,
+ .mark_txdone = qcom_hvc_mark_txdone,
+ .fetch_response = qcom_hvc_fetch_response,
+};
+
+const struct scmi_desc scmi_qcom_hvc_desc = {
+ .ops = &scmi_qcom_hvc_ops,
+ .max_rx_timeout_ms = 30,
+ .max_msg = 20,
+ .max_msg_size = 128,
+ .sync_cmds_completed_on_ret = true,
+};
--
2.17.1
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
On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:
> 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/
> + */
> +
> +#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>
> +#include <linux/of_irq.h>
> +#include <linux/processor.h>
This is here because the original code uses spin_until_cond().
> +#include <linux/slab.h>
> +
> +#include "common.h"
> +
> +/**
> + * struct scmi_qcom_hvc - Structure representing a SCMI qcom hvc transport
> + *
> + * @cinfo: SCMI channel info
> + * @shmem: Transmit/Receive shared memory area
> + * @shmem_lock: Lock to protect access to Tx/Rx shared memory area.
> + * @func_id: hvc call function-id
> + * @cap_id: hvc doorbell's capability id
> + */
> +
> +struct scmi_qcom_hvc {
> + struct scmi_chan_info *cinfo;
> + struct scmi_shared_mem __iomem *shmem;
> + /* Protect access to shmem area */
> + struct mutex shmem_lock;
> + u32 func_id;
> + unsigned long cap_id;
One architecture-independent and one architecture-dependent parameter,
see below.
> +};
> +
> +static irqreturn_t qcom_hvc_msg_done_isr(int irq, void *data)
> +{
> + struct scmi_qcom_hvc *scmi_info = data;
> +
> + scmi_rx_callback(scmi_info->cinfo,
> + shmem_read_header(scmi_info->shmem), NULL);
80-char is a nice guideline, but this would be easier to read if not
line-wrapped.
> +
> + return IRQ_HANDLED;
> +}
> +
> +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);
Just inline these three lock-related methods, saves the reader from
wondering what is actually locked, what a "channel" is (is a "channel"
the same thing as a "shm region"?) etc.
> +}
> +
> +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.
> + 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).
> +
> + 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.
> + */
> +
> + 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.
> + 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.
> +
> + /*
> + * 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.
> +
> + return 0;
> +}
> +
> +static int qcom_hvc_send_message(struct scmi_chan_info *cinfo,
> + struct scmi_xfer *xfer)
> +{
> + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
> + struct arm_smccc_res res;
> +
> + /*
> + * Channel will be released only once response has been
> + * surely fully retrieved, so after .mark_txdone()
> + */
> + qcom_hvc_channel_lock_acquire(scmi_info, xfer);
> +
> + shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
> +
> + arm_smccc_1_1_hvc(scmi_info->func_id, scmi_info->cap_id,
> + 0, 0, 0, 0, 0, 0, &res);
> +
> + /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
Does this hold for your implementation as well?
> + if (res.a0) {
> + qcom_hvc_channel_lock_release(scmi_info);
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static void qcom_hvc_fetch_response(struct scmi_chan_info *cinfo,
> + struct scmi_xfer *xfer)
> +{
> + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
> +
> + shmem_fetch_response(scmi_info->shmem, xfer);
> +}
> +
> +static void qcom_hvc_mark_txdone(struct scmi_chan_info *cinfo, int ret,
> + struct scmi_xfer *__unused)
> +{
> + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
> +
> + qcom_hvc_channel_lock_release(scmi_info);
> +}
> +
> +static const struct scmi_transport_ops scmi_qcom_hvc_ops = {
> + .chan_available = qcom_hvc_chan_available,
> + .chan_setup = qcom_hvc_chan_setup,
> + .chan_free = qcom_hvc_chan_free,
> + .send_message = qcom_hvc_send_message,
> + .mark_txdone = qcom_hvc_mark_txdone,
> + .fetch_response = qcom_hvc_fetch_response,
> +};
> +
> +const struct scmi_desc scmi_qcom_hvc_desc = {
> + .ops = &scmi_qcom_hvc_ops,
> + .max_rx_timeout_ms = 30,
> + .max_msg = 20,
I'm confused about the purpose of this number, it serves both as a limit
of the number of clients that are currently allowed to prepare messages,
and as a limit for how many outstanding transfers waiting for response.
Making the prior limit too low will result in the client running into an
-ENOMEM which it likely won't recover from - the user experience after
getting -ENOMEM on regulator_enable() will be suboptimal...
The latter limit would relate to resource limitations on the firmware
side. Please make sure that you have accounted for 2.5kB of firmware
memory, per agent, in your design.
> + .max_msg_size = 128,
Regards,
Bjorn
On 7/18/2023 11:29 AM, Bjorn Andersson wrote:
> On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:
>> 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.
>
>> + */
>> +
>> +#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>
>> +#include <linux/of_irq.h>
>> +#include <linux/processor.h>
> This is here because the original code uses spin_until_cond().
ok, will remove it.
>
>> +#include <linux/slab.h>
>> +
>> +#include "common.h"
>> +
>> +/**
>> + * struct scmi_qcom_hvc - Structure representing a SCMI qcom hvc transport
>> + *
>> + * @cinfo: SCMI channel info
>> + * @shmem: Transmit/Receive shared memory area
>> + * @shmem_lock: Lock to protect access to Tx/Rx shared memory area.
>> + * @func_id: hvc call function-id
>> + * @cap_id: hvc doorbell's capability id
>> + */
>> +
>> +struct scmi_qcom_hvc {
>> + struct scmi_chan_info *cinfo;
>> + struct scmi_shared_mem __iomem *shmem;
>> + /* Protect access to shmem area */
>> + struct mutex shmem_lock;
>> + u32 func_id;
>> + unsigned long cap_id;
> One architecture-independent and one architecture-dependent parameter,
> see below.
>
>> +};
>> +
>> +static irqreturn_t qcom_hvc_msg_done_isr(int irq, void *data)
>> +{
>> + struct scmi_qcom_hvc *scmi_info = data;
>> +
>> + scmi_rx_callback(scmi_info->cinfo,
>> + shmem_read_header(scmi_info->shmem), NULL);
> 80-char is a nice guideline, but this would be easier to read if not
> line-wrapped.
ok.
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +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);
> Just inline these three lock-related methods, saves the reader from
> wondering what is actually locked, what a "channel" is (is a "channel"
> the same thing as a "shm region"?) etc.
will do.
>
>> +}
>> +
>> +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.
>
>> + 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
>
>> +
>> + 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.
>
>> + */
>> +
>> + 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.
>
>> + 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.
>> +
>> + /*
>> + * 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.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_hvc_send_message(struct scmi_chan_info *cinfo,
>> + struct scmi_xfer *xfer)
>> +{
>> + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
>> + struct arm_smccc_res res;
>> +
>> + /*
>> + * Channel will be released only once response has been
>> + * surely fully retrieved, so after .mark_txdone()
>> + */
>> + qcom_hvc_channel_lock_acquire(scmi_info, xfer);
>> +
>> + shmem_tx_prepare(scmi_info->shmem, xfer, cinfo);
>> +
>> + arm_smccc_1_1_hvc(scmi_info->func_id, scmi_info->cap_id,
>> + 0, 0, 0, 0, 0, 0, &res);
>> +
>> + /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
> Does this hold for your implementation as well?
We will have different error codes based on the cap-id passed. Will
remove the above comment.
>
>> + if (res.a0) {
>> + qcom_hvc_channel_lock_release(scmi_info);
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void qcom_hvc_fetch_response(struct scmi_chan_info *cinfo,
>> + struct scmi_xfer *xfer)
>> +{
>> + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
>> +
>> + shmem_fetch_response(scmi_info->shmem, xfer);
>> +}
>> +
>> +static void qcom_hvc_mark_txdone(struct scmi_chan_info *cinfo, int ret,
>> + struct scmi_xfer *__unused)
>> +{
>> + struct scmi_qcom_hvc *scmi_info = cinfo->transport_info;
>> +
>> + qcom_hvc_channel_lock_release(scmi_info);
>> +}
>> +
>> +static const struct scmi_transport_ops scmi_qcom_hvc_ops = {
>> + .chan_available = qcom_hvc_chan_available,
>> + .chan_setup = qcom_hvc_chan_setup,
>> + .chan_free = qcom_hvc_chan_free,
>> + .send_message = qcom_hvc_send_message,
>> + .mark_txdone = qcom_hvc_mark_txdone,
>> + .fetch_response = qcom_hvc_fetch_response,
>> +};
>> +
>> +const struct scmi_desc scmi_qcom_hvc_desc = {
>> + .ops = &scmi_qcom_hvc_ops,
>> + .max_rx_timeout_ms = 30,
>> + .max_msg = 20,
> I'm confused about the purpose of this number, it serves both as a limit
> of the number of clients that are currently allowed to prepare messages,
> and as a limit for how many outstanding transfers waiting for response.
>
> Making the prior limit too low will result in the client running into an
> -ENOMEM which it likely won't recover from - the user experience after
> getting -ENOMEM on regulator_enable() will be suboptimal...
>
> The latter limit would relate to resource limitations on the firmware
> side. Please make sure that you have accounted for 2.5kB of firmware
> memory, per agent, in your design.
ok!
>> + .max_msg_size = 128,
> Regards,
> Bjorn
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
>
On 7/18/2023 12:07 PM, Bjorn Andersson wrote:
> On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote:
>> On 7/18/2023 11:29 AM, Bjorn Andersson wrote:
>>> On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:
>>>> diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c
> [..]
>>>> +static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo,
>>>> + struct device *dev, bool tx)
>>>> +{
> [..]
>>>> + /* 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.
>>
> If you remove the additional part, per the next comment, DeviceTree
> would be oblivious to these properties. I'll don't know if the
> DeviceTree people have any concerns/suggestions about this.
ok.
>
>>>> + */
>>>> +
>>>> + 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.
>>>> + 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.
> I'd not be surprised if the capability id is 64 bit on a 32-bit machine
> as well, it's not really a property of the architecture.
on 32bit machine, you will have to use SMC32 convention. lt will mean
that the parameters can only be 32 bit long. If you keep cap-id 64 bit
in 32bit machines, then it has to be passed in two parameters. Are you
suggesting, I make this driver dependent on ARM64 and only care about 64
bit for now?
>
> But regardless, always using 64 bits in your memory representation will
> at worst waste a few bytes. But the result is a better defined
> interface, and you can avoid the conditional code.
>
> Regards,
> Bjorn
On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote:
> On 7/18/2023 11:29 AM, Bjorn Andersson wrote:
> > On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:
> > > diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c
[..]
> > > +static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo,
> > > + struct device *dev, bool tx)
> > > +{
[..]
> > > + /* 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.
>
If you remove the additional part, per the next comment, DeviceTree
would be oblivious to these properties. I'll don't know if the
DeviceTree people have any concerns/suggestions about this.
>
> >
> > > + */
> > > +
> > > + 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.
> >
> > > + 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.
I'd not be surprised if the capability id is 64 bit on a 32-bit machine
as well, it's not really a property of the architecture.
But regardless, always using 64 bits in your memory representation will
at worst waste a few bytes. But the result is a better defined
interface, and you can avoid the conditional code.
Regards,
Bjorn
On 18/07/2023 20:25, Nikunj Kela wrote:
>>> +
>>> + 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.
Thanks for bringing my attention to this. I sent a fix for smc.c. Fix
your patch as well, please.
>>> + 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
I understand and I hope you understand the code you copied. If there is
a bug in existing code, please do not copy it to new code (like leaking
OF node reference).
Best regards,
Krzysztof
On Tue, Jul 18, 2023 at 12:10:16PM -0700, Nikunj Kela wrote:
>
> On 7/18/2023 12:07 PM, Bjorn Andersson wrote:
> > On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote:
> > > On 7/18/2023 11:29 AM, Bjorn Andersson wrote:
> > > > On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:
> > > > > diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c
> > [..]
[..]
> > > > > +#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.
> > I'd not be surprised if the capability id is 64 bit on a 32-bit machine
> > as well, it's not really a property of the architecture.
>
> on 32bit machine, you will have to use SMC32 convention. lt will mean that
> the parameters can only be 32 bit long. If you keep cap-id 64 bit in 32bit
> machines, then it has to be passed in two parameters. Are you suggesting, I
> make this driver dependent on ARM64 and only care about 64 bit for now?
>
I'm suggesting that the calling convention is one thing and the
in-memory format for passing the information to the driver is a
different thing.
Keep the arguments passed in memory architecture-independent (i.e. make
it u64).
If you're saying that the calling convention would be different on a
32-bit system, then you are also saying that your driver _is_ 64-bit
specific. Please confirm what the size of your capability id would be in
such a system and make sure the Kconfig and/or the code, reflects
reality.
Thanks,
Bjorn
On 7/18/2023 11:42 AM, Krzysztof Kozlowski wrote:
> On 18/07/2023 20:25, Nikunj Kela wrote:
>>>> +
>>>> + 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.
> Thanks for bringing my attention to this. I sent a fix for smc.c. Fix
> your patch as well, please.
Thanks, I thought you were referring to kzalloc cleanup. Will include
this fix. BTW, you may need to fix mailbox.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
> I understand and I hope you understand the code you copied. If there is
> a bug in existing code, please do not copy it to new code (like leaking
> OF node reference).
>
> Best regards,
> Krzysztof
>
On 7/18/2023 12:30 PM, Bjorn Andersson wrote:
> On Tue, Jul 18, 2023 at 12:10:16PM -0700, Nikunj Kela wrote:
>> On 7/18/2023 12:07 PM, Bjorn Andersson wrote:
>>> On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote:
>>>> On 7/18/2023 11:29 AM, Bjorn Andersson wrote:
>>>>> On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:
>>>>>> diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c
>>> [..]
> [..]
>>>>>> +#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.
>>> I'd not be surprised if the capability id is 64 bit on a 32-bit machine
>>> as well, it's not really a property of the architecture.
>> on 32bit machine, you will have to use SMC32 convention. lt will mean that
>> the parameters can only be 32 bit long. If you keep cap-id 64 bit in 32bit
>> machines, then it has to be passed in two parameters. Are you suggesting, I
>> make this driver dependent on ARM64 and only care about 64 bit for now?
>>
> I'm suggesting that the calling convention is one thing and the
> in-memory format for passing the information to the driver is a
> different thing.
>
> Keep the arguments passed in memory architecture-independent (i.e. make
> it u64).
>
> If you're saying that the calling convention would be different on a
> 32-bit system, then you are also saying that your driver _is_ 64-bit
> specific. Please confirm what the size of your capability id would be in
> such a system and make sure the Kconfig and/or the code, reflects
> reality.
>
> Thanks,
> Bjorn
It is confirmed that cap-id will be 32 bit long on 32bit machines. That
said, I will make changes to use last 16bytes irrespective of the
architecture.
On 18/07/2023 23:16, Nikunj Kela wrote:
>
> On 7/18/2023 11:42 AM, Krzysztof Kozlowski wrote:
>> On 18/07/2023 20:25, Nikunj Kela wrote:
>>>>> +
>>>>> + 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.
>> Thanks for bringing my attention to this. I sent a fix for smc.c. Fix
>> your patch as well, please.
> Thanks, I thought you were referring to kzalloc cleanup. Will include
> this fix. BTW, you may need to fix mailbox.c as well.
Indeed, thanks.
Best regards,
Krzysztof
On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote:
>
> On 7/18/2023 11:29 AM, Bjorn Andersson wrote:
> > On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:
> > > 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,
> >
> > > + */
> > > +
> > > +#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]
> > > +
> > > +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.
>
> >
> > > + 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
> >
> > > +
> > > + 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.
>
> >
> > > + */
> > > +
> > > + 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.
> >
Mmm...but can you access this trailing config bytes if you dont ioremap it ?
> > > + 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.
> > > +
> > > + /*
> > > + * 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.
> >
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
On 7/19/2023 3:55 AM, Cristian Marussi wrote:
> On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote:
>> On 7/18/2023 11:29 AM, Bjorn Andersson wrote:
>>> On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:
>>>> 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,
>
>>>> + */
>>>> +
>>>> +#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]
>
>>>> +
>>>> +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.
ACK!
>
>>>> + 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
>>>> +
>>>> + 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.
Ok.
>
>>>> + */
>>>> +
>>>> + 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.
> Mmm...but can you access this trailing config bytes if you dont ioremap it ?
No, I meant, the last 16 bytes of each channel can be used so we don't
need to remap 2 extra ulong.
>
>>>> + 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.
>>>> +
>>>> + /*
>>>> + * 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.
> 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, will do!
>
> Thanks,
> Cristian
Hi Nikunj,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.5-rc2 next-20230721]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nikunj-Kela/dt-bindings-arm-Add-qcom-specific-hvc-transport-for-SCMI/20230719-001116
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230718160833.36397-3-quic_nkela%40quicinc.com
patch subject: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport
config: arm64-randconfig-r083-20230723 (https://download.01.org/0day-ci/archive/20230723/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230723/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> drivers/firmware/arm_scmi/qcom_hvc.c:136:26: sparse: sparse: cast removes address space '__iomem' of expression
>> drivers/firmware/arm_scmi/qcom_hvc.c:136:52: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile [noderef] __iomem *addr @@ got void * @@
drivers/firmware/arm_scmi/qcom_hvc.c:136:52: sparse: expected void const volatile [noderef] __iomem *addr
drivers/firmware/arm_scmi/qcom_hvc.c:136:52: sparse: got void *
drivers/firmware/arm_scmi/qcom_hvc.c:139:25: sparse: sparse: cast removes address space '__iomem' of expression
drivers/firmware/arm_scmi/qcom_hvc.c:139:58: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const volatile [noderef] __iomem *addr @@ got void * @@
drivers/firmware/arm_scmi/qcom_hvc.c:139:58: sparse: expected void const volatile [noderef] __iomem *addr
drivers/firmware/arm_scmi/qcom_hvc.c:139:58: sparse: got void *
vim +/__iomem +136 drivers/firmware/arm_scmi/qcom_hvc.c
82
83 static int qcom_hvc_chan_setup(struct scmi_chan_info *cinfo,
84 struct device *dev, bool tx)
85 {
86 struct device *cdev = cinfo->dev;
87 struct scmi_qcom_hvc *scmi_info;
88 resource_size_t size;
89 struct resource res;
90 struct device_node *np;
91 unsigned long cap_id;
92 u32 func_id;
93 int ret, irq;
94
95 if (!tx)
96 return -ENODEV;
97
98 scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL);
99 if (!scmi_info)
100 return -ENOMEM;
101
102 np = of_parse_phandle(cdev->of_node, "shmem", 0);
103 if (!of_device_is_compatible(np, "arm,scmi-shmem"))
104 return -ENXIO;
105
106 ret = of_address_to_resource(np, 0, &res);
107 of_node_put(np);
108 if (ret) {
109 dev_err(cdev, "failed to get SCMI Tx shared memory\n");
110 return ret;
111 }
112
113 size = resource_size(&res);
114
115 /* let's map 2 additional ulong since
116 * func-id & capability-id are kept after shmem.
117 * +-------+
118 * | |
119 * | shmem |
120 * | |
121 * | |
122 * +-------+ <-- size
123 * | funcId|
124 * +-------+ <-- size + sizeof(ulong)
125 * | capId |
126 * +-------+ <-- size + 2*sizeof(ulong)
127 */
128
129 scmi_info->shmem = devm_ioremap(dev, res.start,
130 size + 2 * sizeof(unsigned long));
131 if (!scmi_info->shmem) {
132 dev_err(dev, "failed to ioremap SCMI Tx shared memory\n");
133 return -EADDRNOTAVAIL;
134 }
135
> 136 func_id = readl((void *)(scmi_info->shmem) + size);
137
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki