Message-ID: <5c76250b-4415-950e-6aab-7ccbdc6ca83a@quicinc.com>
Date:   Tue, 18 Jul 2023 11:53:24 -0700
From:   Nikunj Kela <quic_nkela@...cinc.com>
To:     Bjorn Andersson <andersson@...nel.org>
CC:     <sudeep.holla@....com>, <cristian.marussi@....com>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <conor+dt@...nel.org>, <agross@...nel.org>,
        <konrad.dybcio@...aro.org>, <linux-arm-kernel@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport


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

Powered by blists - more mailing lists