Hi Peng,
On 9/29/19 11:20 PM, Peng Fan wrote:
> From: Peng Fan <peng.fan@xxxxxxx>
>
> 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.
[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. 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.
--
Florian
