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

[Peng Fan]

Hi Jassi,

> Subject: Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox
> 
> 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.

So you prefer add the structure in include/linux/mailbox/smc-mailbox.h?

Thanks,
Peng.

> 
> 
> 
> > > > > +
> > > > > +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),
> > > > > + mbox->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