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. > diff --git a/drivers/firmware/samsung/exynos-acpm.h > b/drivers/firmware/samsung/exynos-acpm.h > new file mode 100644 > index 000000000000..a03adcd260f5 > --- /dev/null > +++ b/drivers/firmware/samsung/exynos-acpm.h > @@ -0,0 +1,15 @@ > +#ifndef __EXYNOS_ACPM_H__ > +#define __EXYNOS_ACPM_H__ > + > +struct acpm_handle; > +struct acpm_xfer; > + > +int acpm_do_xfer(const struct acpm_handle *handle, struct acpm_xfer > *xfer); > + > +#endif /* __EXYNOS_ACPM_H__ */ > diff --git a/include/linux/soc/samsung/exynos-acpm-protocol.h > b/include/linux/soc/samsung/exynos-acpm-protocol.h > new file mode 100644 > index 000000000000..762783af7617 > --- /dev/null > +++ b/include/linux/soc/samsung/exynos-acpm-protocol.h Why is this in include/linux/soc, and not in the firmware header? Arnd