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

[Andre Przywara]

On Wed, 18 Sep 2019 08:31:57 -0500
Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote:

Hi,

> 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.

Yeah, I saw some examples as well, but not every driver was following this apparently.
I guess since we have a fixed data format we should export the struct then, maybe with a remark that the actual usage of registers is up to the protocol (within the SMCCC limits), so is optional.

> > > > > +
> > > > > +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

True, for some reason I totally missed that line, sorry for that.
 
> > 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.

I see. So does the framework handle the case where both txdone_poll and txdone_irq are false?

Cheers,
Andre.