From: Jassi Brar <jassisinghbrar@gmail.com>
To: Wendy Liang <wendy.liang@xilinx.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
Devicetree List <devicetree@vger.kernel.org>,
Michal Simek <michal.simek@xilinx.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATH v7 1/2] mailbox: ZynqMP IPI mailbox controller
Date: Fri, 21 Dec 2018 19:59:48 -0600 [thread overview]
Message-ID: <CABb+yY27K8V-Mg_0kvQFvvAGhzdXG+7Zcz9tGDe_1FTStvra3Q@mail.gmail.com> (raw)
In-Reply-To: <1545327149-22331-2-git-send-email-wendy.liang@xilinx.com>
On Thu, Dec 20, 2018 at 11:32 AM Wendy Liang <wendy.liang@xilinx.com> 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.
> +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.
......
> +
> +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?
> +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()?
> +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.
> + /* 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.
....
> + 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.
> + 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.
> +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() ?
Thnx
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2018-12-22 2:00 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-20 17:32 [PATH v7 0/2] Xilinx ZynqMP IPI Mailbox Controller Driver Wendy Liang
2018-12-20 17:32 ` [PATH v7 1/2] mailbox: ZynqMP IPI mailbox controller Wendy Liang
2018-12-22 1:59 ` Jassi Brar [this message]
2019-01-11 0:58 ` Jiaying Liang
2018-12-20 17:32 ` [PATH v7 2/2] dt-bindings: mailbox: Add Xilinx IPI Mailbox Wendy Liang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABb+yY27K8V-Mg_0kvQFvvAGhzdXG+7Zcz9tGDe_1FTStvra3Q@mail.gmail.com \
--to=jassisinghbrar@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=michal.simek@xilinx.com \
--cc=robh+dt@kernel.org \
--cc=wendy.liang@xilinx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).