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
