Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Peng Fan <peng.fan@nxp.com>
To:     Andre Przywara <andre.przywara@arm.com>,
        Florian Fainelli <f.fainelli@gmail.com>
CC:     "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "jassisinghbrar@gmail.com" <jassisinghbrar@gmail.com>,
        "sudeep.holla@arm.com" <sudeep.holla@arm.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        dl-linux-imx <linux-imx@nxp.com>
Subject: RE: [PATCH V10 2/2] mailbox: introduce ARM SMC based mailbox
Thread-Topic: [PATCH V10 2/2] mailbox: introduce ARM SMC based mailbox
Thread-Index: AQHVd1cfT8QFyxAbm0eWywLWz3Kfz6eBxl+AgAXiXwCAD5uG0A==
Date:   Fri, 22 Nov 2019 09:47:17 +0000
Message-ID: <AM0PR04MB4481463F6F6A57E564CB025A88490@AM0PR04MB4481.eurprd04.prod.outlook.com>
References: <1569824287-4263-1-git-send-email-peng.fan@nxp.com>
 <1569824287-4263-3-git-send-email-peng.fan@nxp.com>
 <2c8fa412-33c2-57c7-20b7-37b3b70ce524@gmail.com>
 <20191112112414.10f3f88e@donnerap.cambridge.arm.com>
In-Reply-To: <20191112112414.10f3f88e@donnerap.cambridge.arm.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 7750a7b5-0efc-4c8b-1b04-08d76f30f642
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Nov 2019 09:47:17.6055
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: vvoSGRxP29pGdnwYzIYlvfXW04TirHfRVVL0qTSuhbzBdDjScaWTF7/BfR9NZJnIhnGW3d1DOggl3dmizCbvAA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB6577
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> Subject: Re: [PATCH V10 2/2] mailbox: introduce ARM SMC based mailbox
>=20
> On Fri, 8 Nov 2019 09:32:43 -0800
> Florian Fainelli <f.fainelli@gmail.com> wrote:
>=20
> Hi Florian,
>=20
> > On 9/29/19 11:20 PM, Peng Fan wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > This mailbox driver implements a mailbox which signals transmitted
> > > data via an ARM smc (secure monitor call) instruction. The mailbox
> > > receiver is implemented in firmware and can synchronously return
> > > data when it returns execution to the non-secure world again.
> > > An asynchronous receive path is not implemented.
> > > This allows the usage of a mailbox to trigger firmware actions on
> > > SoCs which either don't have a separate management processor or on
> > > which such a core is not available. A user of this mailbox could be
> > > the SCP interface.
> >
> > Sorry for not spotting this, or rather asking this earlier, but I do
> > have one question below.
> >
> > [snip]
> >
> > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > +	struct arm_smc_chan_data *chan_data =3D link->con_priv;
> > > +	struct arm_smccc_mbox_cmd *cmd =3D data;
> > > +	unsigned long ret;
> > > +
> > > +	if (ARM_SMCCC_IS_64(chan_data->function_id)) {
> > > +		ret =3D
> chan_data->invoke_smc_mbox_fn(chan_data->function_id,
> > > +						    cmd->args_smccc64[0],
> > > +						    cmd->args_smccc64[1],
> > > +						    cmd->args_smccc64[2],
> > > +						    cmd->args_smccc64[3],
> > > +						    cmd->args_smccc64[4],
> > > +						    cmd->args_smccc64[5]);
> > > +	} else {
> > > +		ret =3D
> chan_data->invoke_smc_mbox_fn(chan_data->function_id,
> > > +						    cmd->args_smccc32[0],
> > > +						    cmd->args_smccc32[1],
> > > +						    cmd->args_smccc32[2],
> > > +						    cmd->args_smccc32[3],
> > > +						    cmd->args_smccc32[4],
> > > +						    cmd->args_smccc32[5]);
> > > +	}
> >
> > Why did not we use unsigned long for the args_smccc[] array to be bit
> > width independent, this is what the PSCI infrastructure does and it
> > looks a lot nicer IMHO. More question below.
>=20
> Huh, interestingly I think this comes from the combination of the two
> problems you point out, which evolved separately:
> Earlier we had no exported interface between the transport driver and the
> mailbox client, just a void pointer. So using "long" in the structure wou=
ld not
> work, because it would behave differently between arm32 and arm64 kernels=
.
> But the firmware interface would always be fixed to one of the two callin=
g
> conventions, regardless of the kernel "bitness", as advertised by the upp=
er bits
> of the function ID.
> So we introduced explicit types that are used depending on the
> firmware-advertised calling convention. The idea was that any packed data
> any client would provide would always end up in consecutive registers in =
the
> firmware.
> Now we explicitly advertise the expected message structure in the new
> header file, so we could go back to unsigned long here, indeed. A 32-bit =
kernel
> could never use the 64-bit calling convention, so long would fit. In a 64=
-bit
> kernel the compiler would either downgrade the long argument to the 32-bi=
t
> arguments the firmware expects, or keep it long.
> So it might be worth a short to go back to long.

I'll drop the check ARM_SMCCC_IS_64(chan_data->function_id) and
Directly
 chan_data->invoke_smc_mbox_fn(chan_data->function_id,
						    cmd->args_smccc[0],
						    cmd->args_smccc[1],
						    cmd->args_smccc[2],
						    cmd->args_smccc[3],
						    cmd->args_smccc[4],
						    cmd->args_smccc[5]);

Is this ok for you?

>=20
> >
> > [snip]
> >
> > > +
> > > +#ifndef _LINUX_ARM_SMCCC_MBOX_H_
> > > +#define _LINUX_ARM_SMCCC_MBOX_H_
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +/**
> > > + * struct arm_smccc_mbox_cmd - ARM SMCCC message structure
> > > + * @args_smccc32/64:	actual usage of registers is up to the protocol
> > > + *			(within the SMCCC limits)
> > > + */
> > > +struct arm_smccc_mbox_cmd {
> > > +	union {
> > > +		u32 args_smccc32[6];
> > > +		u64 args_smccc64[6];
> > > +	};
> > > +};
> >
> > Why is this being moved to a separate header file and not within the
> > driver's main file? It is not like we offer the ability for a driver
> > to embed this ARM SMC mailbox driver as a library, and customize the
> > values of the SMC arguments (maybe we should do that, as a later
> > patch) except for the function_id.
>=20
> I wouldn't call it a "library", but indeed we expose the transport protoc=
ol to
> the mailbox client. It seems that the mailbox framework is not really cle=
ar
> here, it just states that (at least in many cases) the mailbox client kno=
ws about
> the transport protocol, even though the separation between the two sugges=
ts
> otherwise. This probably stems back from the days, where mailboxes were
> directly used by their users, without providing any kind of abstraction.
> So going with this, the SMC mailbox transport driver enforces a specific
> transport protocol for the payload, namely the six SMCCC defined register=
s. So
> we make this available, so any mailbox client knows what to expect. At th=
e
> end of the day on the other end there will be some firmware probably
> expecting specific data in specific registers - or no data at all, as in =
the simple
> doorbell case we intend to use for SCPI/SCMI.

struct arm_smccc_mbox_cmd {
	unsigned long args_smccc[6];
};

Is this ok for you?

Thanks,
Peng.


>=20
> > If you have a "public" header, there is usually a service or some
> > configuration that your driver would offer, which is not the case
> > here.
>=20
> If you want to use the mailbox just as a doorbell (as in our case), it do=
esn't
> matter, so we can as well expose the underlying transport protocol.
>=20
> Cheers,
> Andre.