RE: [PATCH V10 2/2] mailbox: introduce ARM SMC based mailbox

[Peng Fan <peng.fan@nxp.com>]

> Subject: Re: [PATCH V10 2/2] mailbox: introduce ARM SMC based mailbox
> 
> On Fri, 8 Nov 2019 09:32:43 -0800
> Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> Hi Florian,
> 
> > 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 = link->con_priv;
> > > +	struct arm_smccc_mbox_cmd *cmd = data;
> > > +	unsigned long ret;
> > > +
> > > +	if (ARM_SMCCC_IS_64(chan_data->function_id)) {
> > > +		ret =
> 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 =
> 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.
> 
> 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 would 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 calling
> conventions, regardless of the kernel "bitness", as advertised by the upper 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-bit
> 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?

> 
> >
> > [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.
> 
> I wouldn't call it a "library", but indeed we expose the transport protocol to
> the mailbox client. It seems that the mailbox framework is not really clear
> here, it just states that (at least in many cases) the mailbox client knows about
> the transport protocol, even though the separation between the two suggests
> 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 registers. So
> we make this available, so any mailbox client knows what to expect. At the
> 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.


> 
> > 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.
> 
> If you want to use the mailbox just as a doorbell (as in our case), it doesn't
> matter, so we can as well expose the underlying transport protocol.
> 
> Cheers,
> Andre.