Re: [PATCH 1/2] firmware: samsung: acpm: add fire-and-forget xfer support

[Krzysztof Kozlowski]

On 12/06/2026 06:34, Alexey Klimov wrote:
>  
> +int acpm_dvfs_set_rate_fast(struct acpm_handle *handle,
> +			    unsigned int acpm_chan_id, unsigned int clk_id,
> +			    unsigned long rate)
> +{
> +	struct acpm_xfer xfer = {0};
> +	u32 cmd[4];
> +
> +	acpm_dvfs_init_set_rate_cmd(cmd, clk_id, rate);
> +	acpm_set_xfer(&xfer, cmd, ARRAY_SIZE(cmd), acpm_chan_id, false);
> +
> +	return acpm_do_xfer_fast(handle, &xfer);
> +}
> +
> +

Just one blank line

>  static void acpm_dvfs_init_get_rate_cmd(u32 cmd[4], unsigned int clk_id)
>  {
>  	cmd[0] = FIELD_PREP(ACPM_DVFS_ID, clk_id);
> diff --git a/drivers/firmware/samsung/exynos-acpm-dvfs.h b/drivers/firmware/samsung/exynos-acpm-dvfs.h
> index b37b15426102..107d9aa27690 100644
> --- a/drivers/firmware/samsung/exynos-acpm-dvfs.h
> +++ b/drivers/firmware/samsung/exynos-acpm-dvfs.h
> @@ -14,6 +14,9 @@ struct acpm_handle;
>  int acpm_dvfs_set_rate(struct acpm_handle *handle,
>  		       unsigned int acpm_chan_id, unsigned int id,
>  		       unsigned long rate);
> +int acpm_dvfs_set_rate_fast(struct acpm_handle *handle,
> +			    unsigned int acpm_chan_id, unsigned int id,
> +			    unsigned long rate);
>  unsigned long acpm_dvfs_get_rate(struct acpm_handle *handle,
>  				 unsigned int acpm_chan_id,


...

> +	do {
> +		val = readl(base + mlen * i);
> +		rx_seqnum = FIELD_GET(ACPM_PROTOCOL_SEQNUM, val);
> +
> +		if (rx_seqnum) {
> +			seqnum = rx_seqnum - 1;
> +			rx_data = &achan->rx_data[seqnum];
> +
> +			if (rx_data->rxcnt)
> +				__ioread32_copy(rx_data->cmd, base + mlen * i, rx_data->rxcnt);
> +
> +			/* Signal the waiting thread (if any). If it hasn't started

Please use Linux style comments.

> +			 * spinning yet, it will see this instantly when it does. */
> +			smp_store_release(&rx_data->completed, true);
> +
> +			/* Only free the sequence number if it belongs to an
> +			 * async request. Senders who use regular acpm_do_xfer()
> +			 * will free their own sequence numbers in
> +			 * acpm_dequeue_by_polling().
> +			 */
> +			if (rx_data->is_async)
> +				clear_bit_unlock(seqnum, achan->bitmap_seqnum);
> +		}
> +
> +		i = (i + 1) % achan->qlen;
> +	} while (i != rx_front);
> +
> +	writel(rx_front, achan->rx.rear);
> +
> +	spin_unlock(&achan->rx_lock);
> +}
> +
>  /**
>   * acpm_dequeue_by_polling() - RX dequeue by polling.
>   * @achan:	ACPM channel info.
> @@ -388,15 +446,15 @@ static int acpm_wait_for_queue_slots(struct acpm_chan *achan, u32 next_tx_front)
>  }
>  


...

> +
>  /**
>   * acpm_do_xfer() - do one transfer.
>   * @handle:	pointer to the acpm handle.
> @@ -485,7 +606,7 @@ int acpm_do_xfer(struct acpm_handle *handle, const struct acpm_xfer *xfer)
>  	msg.chan_id = xfer->acpm_chan_id;
>  	msg.chan_type = EXYNOS_MBOX_CHAN_TYPE_DOORBELL;
>  
> -	scoped_guard(mutex, &achan->tx_lock) {
> +	scoped_guard(spinlock, &achan->tx_lock) {

This blind replacement just feels completely wrong. Not only you made
huge part of code non-sleepable but actually put spinlock over
readl_poll_timeout with 2*500 ms timeout.



>  		tx_front = readl(achan->tx.front);
>  		idx = (tx_front + 1) % achan->qlen;
>  
> @@ -654,8 +775,8 @@ static int acpm_channels_init(struct acpm_info *acpm)
>  		if (ret)
>  			return ret;
>  
Best regards,
Krzysztof