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

[Arnd Bergmann]

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