Message-ID: <5ac65f22-0ea8-b6b0-f1d4-57ba3b5ba259@quicinc.com>
Date:   Wed, 19 Jul 2023 07:02:24 -0700
From:   Nikunj Kela <quic_nkela@...cinc.com>
To:     Cristian Marussi <cristian.marussi@....com>
CC:     Bjorn Andersson <andersson@...nel.org>, <sudeep.holla@....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/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

Powered by blists - more mailing lists