Re: [PATCH 2/7] mailbox: keembay-scmi-mailbox: Add support for Keem Bay mailbox

From: Sudeep Holla <sudeep.holla@arm.com>
To: Paul Murphy <paul.j.murphy@linux.intel.com>
Cc: Daniele Alessandrelli <daniele.alessandrelli@linux.intel.com>,
	linux-arm-kernel@lists.infradead.org,
	Rob Herring <robh+dt@kernel.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Jassi Brar <jassisinghbrar@gmail.com>,
	Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>,
	Paul Murphy <paul.j.murphy@intel.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Dinh Nguyen <dinguyen@kernel.org>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 2/7] mailbox: keembay-scmi-mailbox: Add support for Keem Bay mailbox
Date: Fri, 10 Jul 2020 09:01:16 +0100	[thread overview]
Message-ID: <20200710075931.GB1189@bogus> (raw)
In-Reply-To: <537a4c$cobr4j@orsmga004-auth.jf.intel.com>

On Thu, Jul 09, 2020 at 02:28:10PM +0100, Paul Murphy wrote:
> 
> On 7/9/20 13:23, Daniele Alessandrelli wrote:
> > Hi Sudeep,
> > 
> > Thanks for your review.
> > 
> > On Wed, 2020-07-08 at 21:34 +0100, Sudeep Holla wrote:
> > > On Tue, Jun 16, 2020 at 04:56:08PM +0100, Daniele Alessandrelli
> > > wrote:
> > > > From: Paul Murphy <paul.j.murphy@intel.com>
> > > > 
> > > > Keem Bay SoC has a ARM trusted firmware-based secure monitor which
> > > > acts
> > > > as the SCP for the purposes of power management over SCMI.
> > > > 
> > > > This driver implements the transport layer for SCMI to function.
> > > > 
> > > Please use the smc transport support in
> > > driver/firmware/arm_scmi/smc.c
> > > for this. You don't need mailbox support for SMC/HVC. Basically you
> > > don't need this driver at all and you have everything you need to
> > > support
> > > what you want.
> > > 
> > > Let me know if you face issues.
> > > 
> > Sorry, we didn't know about the SMC transport support for SCMI. Looks
> > like it was added only recently, while our driver was already developed
> > and waiting to be upstreamed.
> > 
> > I agree that we can drop this driver and switch to the SMC transport as
> > you suggested, but I think we'll have to modify our bootloader SiP
> > service slightly. Paul, can you elaborate?
> > 
> 
> Just one question.
> 
> In our patch, we pass the shared memory address as the second argument of
> the SiP service, as it means we don't have to hardcode that in our firmware.
> Sudeep, do you know if it was intentional in smc_send_message() to leave
> that out? If we leave it out, we are requiring the secure monitor to
> hardcode the shared memory address.
> 

Please post a patch adding the address as 2nd parameter. Cc Peng Fan from
NXP who is the original author of the file. I was also wondering why have
I not added the address when I extended support for multiple channel/shmem
with smc/hvc. One key point here is that firmware *must not* return
INVALID PARAMETER and *must* ignore it.

May be we can add a note while adding that the firmware can ignore that
parameter if it supports only one channel && hardcoded in the firmware.
It needs to be PA as obtained from the DT. Since it is custom SIP id, it
needs to be well documented.

-- 
Regards,
Sudeep