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

On 7/18/2023 12:30 PM, Bjorn Andersson wrote:

On Tue, Jul 18, 2023 at 12:10:16PM -0700, Nikunj Kela wrote:

quoted

On 7/18/2023 12:07 PM, Bjorn Andersson wrote:

quoted

On Tue, Jul 18, 2023 at 11:53:24AM -0700, Nikunj Kela wrote:

quoted

On 7/18/2023 11:29 AM, Bjorn Andersson wrote:

quoted

On Tue, Jul 18, 2023 at 09:08:33AM -0700, Nikunj Kela wrote:

quoted

diff --git a/drivers/firmware/arm_scmi/qcom_hvc.c b/drivers/firmware/arm_scmi/qcom_hvc.c

[..]

[..]

quoted

quoted

quoted

quoted

quoted

+#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.