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

From: Nikunj Kela <quic_nkela@quicinc.com>
To: Bjorn Andersson <andersson@kernel.org>
Cc: <sudeep.holla@arm.com>, <cristian.marussi@arm.com>,
	<robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
	<conor+dt@kernel.org>, <agross@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>
Subject: Re: [PATCH 2/2] firmware: arm_scmi: Add qcom hvc/shmem transport
Date: Tue, 18 Jul 2023 12:10:16 -0700	[thread overview]
Message-ID: <d2d01794-ef13-07f9-7695-b56b58cc01ac@quicinc.com> (raw)
In-Reply-To: <6uelwa7fpzbw757rbb2j3u4ogagypdzyqj5mykyhlkkkvrnfin@ttonjpfn7nxc>

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