Re: [PATCH v3 2/3] firmware: add exynos ACPM protocol driver

On Thu, Dec 5, 2024, at 18:53, Tudor Ambarus wrote:

+#define exynos_acpm_set_bulk(data, i)					\
+	(((data) & ACPM_BULK_MASK) << (ACPM_BULK_SHIFT * (i)))
+#define exynos_acpm_read_bulk(data, i)					\
+	(((data) >> (ACPM_BULK_SHIFT * (i))) & ACPM_BULK_MASK)

Could these be inline functions for readability?

+	cmd[3] = (u32)(sched_clock() / 1000000); /*record ktime ms*/

The comment does not match the implementation, sched_clock()
is probably not what you want here because of its limitiations.

Maybe ktime_to_ms(ktime_get())?

+/**
+ * acpm_get_rx() - get response from RX queue.
+ * @achan:	ACPM channel info.
+ * @xfer:	reference to the transfer to get response for.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int acpm_get_rx(struct acpm_chan *achan, struct acpm_xfer *xfer)
+{
+	struct acpm_msg *tx = &xfer->tx;
+	struct acpm_msg *rx = &xfer->rx;
+	struct acpm_rx_data *rx_data;
+	const void __iomem *base, *addr;
+	u32 rx_front, rx_seqnum, tx_seqnum, seqnum;
+	u32 i, val, mlen;
+	bool rx_set = false;
+
+	rx_front = readl_relaxed(achan->rx.front);
+	i = readl_relaxed(achan->rx.rear);

If you have to use readl_relaxed(), please annotate why,
otherwise just use the normal readl().  Is this access to
the SRAM?

+	spin_lock_irqsave(&achan->tx_lock, flags);
+
+	tx_front = readl_relaxed(achan->tx.front);
+	idx = (tx_front + 1) % achan->qlen;
+
+	ret = acpm_wait_for_queue_slots(achan, idx);
+	if (ret) {
+		spin_unlock_irqrestore(&achan->tx_lock, flags);
+		return ret;
+	}

It looks like you are calling a busy loop function inside
of a hardirq handler here, with a 500ms timeout. This is
not ok.

If you may need to wait for a long timeout, I would suggest
changing the interface so that this function is not allowed
to be called from irq-disabled context, change the spinlock
to a mutex and polling read to a sleeping version.

+	/* Advance TX front. */
+	writel_relaxed(idx, achan->tx.front);
+
+	/* Flush SRAM posted writes. */
+	readl_relaxed(achan->tx.front);
+
+	spin_unlock_irqrestore(&achan->tx_lock, flags);

I don't think this sequence guarantees the serialization
you want. By making the access _relaxed() you explicitly
say you don't want serialization, so the store does
not have to complete before the unlock.

+static const struct of_device_id acpm_match[] = {
+	{ .compatible = "google,gs101-acpm-ipc" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, acpm_match);
+
+static struct platform_driver acpm_driver = {
+	.probe	= acpm_probe,
+	.driver	= {
+		.name = "exynos-acpm-protocol",
+		.of_match_table	= of_match_ptr(acpm_match),

Remove the stray of_match_ptr() here.

Why is this in include/linux/soc, and not in the firmware
header?

      Arnd