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;
Date:   Tue, 18 Jul 2023 12:07:52 -0700
From:   Bjorn Andersson <andersson@kernel.org>
To:     Nikunj Kela <quic_nkela@quicinc.com>
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
Message-ID: <6uelwa7fpzbw757rbb2j3u4ogagypdzyqj5mykyhlkkkvrnfin@ttonjpfn7nxc>
References: <20230718160833.36397-1-quic_nkela@quicinc.com>
 <20230718160833.36397-3-quic_nkela@quicinc.com>
 <xkw36iwjb47q7spf6qle5x3kcmx6fv7or6cwhlpkuvihn3xmt2@qhrhtug4htp2>
 <5c76250b-4415-950e-6aab-7ccbdc6ca83a@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5c76250b-4415-950e-6aab-7ccbdc6ca83a@quicinc.com>
Precedence: bulk

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.

> 
> > 
> > > +	 */
> > > +
> > > +	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.

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