Re: [PATCH 2/3] soc: qcom: Add AOSS QMP communication driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 12/27/2018 1:58 AM, Bjorn Andersson wrote:
On Tue 20 Nov 04:22 PST 2018, Arun Kumar Neelakantam wrote:

Thanks for the review Arun.

On 11/12/2018 1:35 PM, Bjorn Andersson wrote:
[..]
+int qmp_send(struct qmp *qmp, const void *data, size_t len)
+{
+	int ret;
+
+	if (WARN_ON(len + sizeof(u32) > qmp->size)) {
+		dev_err(qmp->dev, "message too long\n");
+		return -EINVAL;
+	}
+
+	if (WARN_ON(len % sizeof(u32))) {
+		dev_err(qmp->dev, "message not 32-bit aligned\n");
+		return -EINVAL;
+	}
+
+	mutex_lock(&qmp->tx_lock);
+
+	if (!qmp_message_empty(qmp)) {
+		dev_err(qmp->dev, "mailbox left busy\n");
+		ret = -EINVAL;
should it be -EBUSY ?
That makes more sense.

And qmp_messge_empty will be done either by remote if it process the data
else by this driver in TIMEOUT case, so does we need this check for every TX
? I think we can just reset to Zero once in open time.
Didn't think about that, should we really make the QMP link ready again
when we get a timeout? Can we expect that the firmware of the remote
side is ready to serve future messages?


Should we keep this check and remove the writel() below?
I prefer we can just remove this check and keep writel() below same as down stream.

+		goto out_unlock;
+	}
+
+	/* The message RAM only implements 32-bit accesses */
+	__iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32),
+			 data, len / sizeof(u32));
+	writel(len, qmp->msgram + qmp->offset);
+	qmp_kick(qmp);
+
+	ret = wait_event_interruptible_timeout(qmp->event,
+					       qmp_message_empty(qmp), HZ);
+	if (!ret) {
+		dev_err(qmp->dev, "ucore did not ack channel\n");
+		ret = -ETIMEDOUT;
+
+		writel(0, qmp->msgram + qmp->offset);
+	} else {
+		ret = 0;
+	}
+
+out_unlock:
+	mutex_unlock(&qmp->tx_lock);
+
+	return ret;
+}
Regards,
Bjorn



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux