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
