Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox

[Jassi Brar]

On Wed, Sep 18, 2019 at 5:00 AM Andre Przywara <andre.przywara@xxxxxxx> wrote:

> >
> > >
> > > > + };
> > > > +};
> > >
> > > If this is the data structure that this mailbox controller uses, I would expect
> > > this to be documented somewhere, or even exported.
> >
> > Export this structure in include/linux/mailbox/smc-mailbox.h?
>
> For instance, even though I am not sure this is really desired or helpful, since we expect a generic mailbox client (the SCPI or SCMI driver) to deal with that mailbox.
>
> But at least the expected format should be documented, which could just be in writing, not necessarily in code.
>
Yes, the packet format is specified by the controller and defined in
some header for clients to include. Other platforms do that already.



> > > > +
> > > > +typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
> > > > +                             unsigned long, unsigned long,
> > > > +                             unsigned long, unsigned long,
> > > > +                             unsigned long);
> > > > +static smc_mbox_fn *invoke_smc_mbox_fn;
> > > > +
> > > > +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;
> > > > + u32 function_id;
> > > > +
> > > > + function_id = chan_data->function_id;
> > > > + if (!function_id)
> > > > +         function_id = cmd->function_id;
> > > > +
> > > > + if (function_id & BIT(30)) {
> > >
> > >     if (ARM_SMCCC_IS_64(function_id)) {
> >
> > ok
> >
> > >
> > > > +         ret = invoke_smc_mbox_fn(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 = invoke_smc_mbox_fn(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]);
> > > > + }
> > > > +
> > > > + mbox_chan_received_data(link, (void *)ret);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static unsigned long __invoke_fn_hvc(unsigned int function_id,
> > > > +                              unsigned long arg0, unsigned long arg1,
> > > > +                              unsigned long arg2, unsigned long arg3,
> > > > +                              unsigned long arg4, unsigned long arg5) {
> > > > + struct arm_smccc_res res;
> > > > +
> > > > + arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4,
> > > > +               arg5, 0, &res);
> > > > + return res.a0;
> > > > +}
> > > > +
> > > > +static unsigned long __invoke_fn_smc(unsigned int function_id,
> > > > +                              unsigned long arg0, unsigned long arg1,
> > > > +                              unsigned long arg2, unsigned long arg3,
> > > > +                              unsigned long arg4, unsigned long arg5) {
> > > > + struct arm_smccc_res res;
> > > > +
> > > > + arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4,
> > > > +               arg5, 0, &res);
> > > > + return res.a0;
> > > > +}
> > > > +
> > > > +static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
> > > > + .send_data      = arm_smc_send_data,
> > > > +};
> > > > +
> > > > +static int arm_smc_mbox_probe(struct platform_device *pdev) {
> > > > + struct device *dev = &pdev->dev;
> > > > + struct mbox_controller *mbox;
> > > > + struct arm_smc_chan_data *chan_data;
> > > > + int ret;
> > > > + u32 function_id = 0;
> > > > +
> > > > + if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
> > > > +         invoke_smc_mbox_fn = __invoke_fn_smc;
> > > > + else
> > > > +         invoke_smc_mbox_fn = __invoke_fn_hvc;
> > > > +
> > > > + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > > > + if (!mbox)
> > > > +         return -ENOMEM;
> > > > +
> > > > + mbox->num_chans = 1;
> > > > + mbox->chans = devm_kzalloc(dev, sizeof(*mbox->chans), GFP_KERNEL);
> > > > + if (!mbox->chans)
> > > > +         return -ENOMEM;
> > > > +
> > > > + chan_data = devm_kzalloc(dev, sizeof(*chan_data), GFP_KERNEL);
> > > > + if (!chan_data)
> > > > +         return -ENOMEM;
> > > > +
> > > > + of_property_read_u32(dev->of_node, "arm,func-id", &function_id);
> > > > + chan_data->function_id = function_id;
> > > > +
> > > > + mbox->chans->con_priv = chan_data;
> > > > +
> > > > + mbox->txdone_poll = false;
> > > > + mbox->txdone_irq = false;
> > >
> > > Don't we need to provide something to confirm reception to the client? In our
> > > case we can set this as soon as the smc/hvc returns.
> >
> > As smc/hvc returns, it means the transfer is over and receive is done.
>
> I understand that, but was wondering if the Linux mailbox framework knows about that? In my older version I was calling mbox_chan_received_data() after the smc call returned.
>
The code already does that at the end of  send_data

> Also there is mbox_chan_txdone() with which a controller driver can signal TX completion explicitly.
>
No. Controller can use that only if it has specified txdone_irq, which
is not the case here.

Thanks