RE: [PATH v7 1/2] mailbox: ZynqMP IPI mailbox controller

[Jiaying Liang]

> -----Original Message-----
> From: Jassi Brar [mailto:jassisinghbrar@xxxxxxxxx]
> Sent: Friday, December 21, 2018 6:00 PM
> To: Jiaying Liang <jliang@xxxxxxxxxx>
> Cc: Michal Simek <michals@xxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> Mark Rutland <mark.rutland@xxxxxxx>; Linux Kernel Mailing List <linux-
> kernel@xxxxxxxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Devicetree
> List <devicetree@xxxxxxxxxxxxxxx>
> Subject: Re: [PATH v7 1/2] mailbox: ZynqMP IPI mailbox controller
> 
> On Thu, Dec 20, 2018 at 11:32 AM Wendy Liang <wendy.liang@xxxxxxxxxx>
> wrote:
> 
> 
> > diff --git a/drivers/mailbox/zynqmp-ipi-mailbox.c
> > b/drivers/mailbox/zynqmp-ipi-mailbox.c
> > new file mode 100644
> > index 0000000..bbddfd5
> > --- /dev/null
> > +++ b/drivers/mailbox/zynqmp-ipi-mailbox.c
> > @@ -0,0 +1,764 @@
> 
> ......
> > +
> > +/* IPI SMC Macros */
> > +#define IPI_SMC_OPEN_IRQ_MASK          0x00000001UL /* IRQ enable bit
> in IPI
> > +                                                     * open SMC call
> > +                                                     */
> > +#define IPI_SMC_NOTIFY_BLOCK_MASK      0x00000001UL /* Flag to
> indicate if
> > +                                                     * IPI notification needs
> > +                                                     * to be blocking.
> > +                                                     */
> > +#define IPI_SMC_ENQUIRY_DIRQ_MASK      0x00000001UL /* Flag to
> indicate if
> > +                                                     * notification interrupt
> > +                                                     * to be disabled.
> > +                                                     */
> > +#define IPI_SMC_ACK_EIRQ_MASK          0x00000001UL /* Flag to indicate
> if
> > +                                                     * notification interrupt
> > +                                                     * to be enabled.
> > +                                                     */
> > +
> The first two are unused.
[Wendy] Will remove the unused macros

> 
> 
> > +struct zynqmp_ipi_pdata {
> > +       struct device *dev;
> > +       int irq;
> > +       unsigned int method;
> >
> 'method' doesn't track the HVC option in the driver. Please have a look.
[Wendy] I will add one more checking in the function implementation
to check HVC and error if it is neither SMC nor HVC.
> 
> ......
> > +
> > +static void zynqmp_ipi_fw_call(struct zynqmp_ipi_mbox *ipi_mbox,
> > +                              unsigned long a0, unsigned long a3,
> > +                              unsigned long a4, unsigned long a5,
> > +                              unsigned long a6, unsigned long a7,
> > +                              struct arm_smccc_res *res)
> >
> [a4,a7] are always 0, so maybe just drop them?
[Wendy] Will drop them from the API, and set them to 0.
> 
> 
> > +static bool zynqmp_ipi_last_tx_done(struct mbox_chan *chan) {
> > +       struct device *dev = chan->mbox->dev;
> > +       struct zynqmp_ipi_mbox *ipi_mbox = dev_get_drvdata(dev);
> > +       struct zynqmp_ipi_mchan *mchan = chan->con_priv;
> > +       int ret;
> > +       u64 arg0;
> > +       struct arm_smccc_res res;
> > +       struct zynqmp_ipi_message *msg;
> > +
> > +       if (WARN_ON(!ipi_mbox)) {
> > +               dev_err(dev, "no platform drv data??\n");
> > +               return false;
> > +       }
> > +
> > +       if (mchan->chan_type == IPI_MB_CHNL_TX) {
> > +               /* We only need to check if the message been taken
> > +                * by the remote in the TX channel
> > +                */
> > +               arg0 = SMC_IPI_MAILBOX_STATUS_ENQUIRY;
> > +               zynqmp_ipi_fw_call(ipi_mbox, arg0, 0, 0, 0, 0, 0, &res);
> > +               /* Check the SMC call status, a0 of the result */
> > +               ret = (int)(res.a0 & 0xFFFFFFFF);
> > +               if (ret < 0 || ret & IPI_MB_STATUS_SEND_PENDING)
> > +                       return false;
> > +
> OK, but ...
> 
> > +               msg = mchan->rx_buf;
> > +               msg->len = mchan->resp_buf_size;
> > +               memcpy_fromio(msg->data, mchan->resp_buf, msg->len);
> > +               mbox_chan_received_data(chan, (void *)msg);
> >
> .... wouldn't this be done from zynqmp_ipi_interrupt()?
[Wendy] The IPI hardware supports both the synchronous mode and
Asynchronous mode.
The rx interrupt used for remote to notify host, or for responding asynchronous
Request. In synchronous mode, the IPI hardware allow remote to write response
To the tx response buffer, and the host side can poll the observation register
And get the response if the remote has acked.
The last_tx_done is implemented for this purpose.
> 
> 
> 
> > +static int zynqmp_ipi_send_data(struct mbox_chan *chan, void *data) {
>   .........
> > +               /* Enquire if the mailbox is free to send message */
> > +               arg0 = SMC_IPI_MAILBOX_STATUS_ENQUIRY;
> > +               timeout = 10;
> > +               if (msg && msg->len) {
> > +                       timeout = 10;
> > +                       do {
> > +                               zynqmp_ipi_fw_call(ipi_mbox, arg0,
> > +                                                  0, 0, 0, 0, 0, &res);
> > +                               ret = res.a0 & 0xFFFFFFFF;
> > +                               if (ret >= 0 &&
> > +                                   !(ret & IPI_MB_STATUS_SEND_PENDING))
> > +                                       break;
> > +                               usleep_range(1, 2);
> > +                               timeout--;
> > +                       } while (timeout);
> > +                       if (!timeout) {
> > +                               dev_warn(dev, "chan %d sending msg timeout.\n",
> > +                                        ipi_mbox->remote_id);
> > +                               return -ETIME;
> > +                       }
> > +                       memcpy_toio(mchan->req_buf, msg->data, msg->len);
> > +               }
> >
> This check should be done in last_tx_done, and not here.
> send_data is never called unless last_tx_done returns true.
[Wendy] will remove it.
> 
> > +               /* Kick IPI mailbox to send message */
> > +               arg0 = SMC_IPI_MAILBOX_NOTIFY;
> > +               zynqmp_ipi_fw_call(ipi_mbox, arg0, 0, 0, 0, 0, 0, &res);
> > +       } else {
> > +               /* Send response message */
> > +               if (msg && msg->len > mchan->resp_buf_size) {
> > +                       dev_err(dev, "channel %d message length %u > max %lu\n",
> > +                               mchan->chan_type, (unsigned int)msg->len,
> > +                               mchan->resp_buf_size);
> > +                       return -EINVAL;
> > +               }
> > +               if (msg && msg->len)
> > +                       memcpy_toio(mchan->resp_buf, msg->data,
> > + msg->len);
> >
> 
> > +               arg0 = SMC_IPI_MAILBOX_NOTIFY;
> > +               arg0 = SMC_IPI_MAILBOX_ACK;
> >
> This is obviously wrong.
[Wendy] will fix
> 
> ....
> > +       mbox->chans = chans;
> > +       chans[IPI_MB_CHNL_TX].con_priv = &ipi_mbox-
> >mchans[IPI_MB_CHNL_TX];
> > +       chans[IPI_MB_CHNL_RX].con_priv = &ipi_mbox-
> >mchans[IPI_MB_CHNL_RX];
> > +       ipi_mbox->mchans[IPI_MB_CHNL_TX].chan_type = IPI_MB_CHNL_TX;
> > +       ipi_mbox->mchans[IPI_MB_CHNL_RX].chan_type = IPI_MB_CHNL_RX;
> > +       ret = mbox_controller_register(mbox);
> >
> while at it, you may want to start using
> devm_mbox_controller_register() recently added by Thierry.
[Wendy] will take a look at this function call
> 
> > +       if (ret)
> > +               dev_err(mdev,
> > +                       "Failed to register mbox_controller(%d)\n", ret);
> > +       else
> > +               dev_info(mdev, "Probed ZynqMP IPI Mailbox driver.\n");
> >
> You may want to also print something instance specific here.
[Wendy] will add more information such as the number of channels.
> 
> 
> > +static int zynqmp_ipi_probe(struct platform_device *pdev) {
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *nc, *np = pdev->dev.of_node;
> > +       struct zynqmp_ipi_pdata *pdata;
> > +       struct zynqmp_ipi_mbox *mbox;
> > +       int i, ret = -EINVAL;
> > +
> > +       i = 0;
> > +       for_each_available_child_of_node(np, nc)
> > +               i++;
> >
> of_get_child_count() ?
[Wendy] Will change.

Thanks,
Wendy
> 
> 
> Thnx