Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3ae2d618-490a-06da-b4b6-b5a24b0a9747@quicinc.com>
Date:   Tue, 18 Jul 2023 11:25:48 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <sudeep.holla@arm.com>
CC:     <cristian.marussi@arm.com>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <linux-arm-kernel@lists.infradead.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>
References: <20230718160833.36397-1-quic_nkela@quicinc.com>
 <20230718160833.36397-3-quic_nkela@quicinc.com>
 <d9d5ffd5-6f85-f091-5d69-12cdd8d04c99@linaro.org>
From:   Nikunj Kela <quic_nkela@quicinc.com>
In-Reply-To: <d9d5ffd5-6f85-f091-5d69-12cdd8d04c99@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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
>