Re: [RFC PATCH v1 2/8] misc/arm-cla: Add launch operation helpers

On Fri, Jul 17, 2026 at 02:16:36PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 17, 2026, at 12:47, Ryan Roberts wrote:
> > From: Jean-Philippe Brucker <[email protected]>
> >
> > CLA commands are issued by writing optional payload registers,
> > programming the LAUNCH register and polling LRESP until the hardware
> > accepts or rejects the operation.
> >
> > Add a common launch helper that performs this sequence on the CLA's
> > local CPU, waits for LRESP completion and translates launch response
> > codes into Linux errors.
> >
> > Build accelerator reset and register read and write support on top of
> > it. The register read and write helpers split larger accesses into
> > multiple launch operations when an access crosses an eight-register
> > window.
> 
> I'm a bit confused by the MMIO register access ordering, if this is
> not a normal AXI attached device with a DMA master, I think it
> would make sense to better document what it is.
> 
> > +static inline u64 cla_reg_read(struct cla_dev *dev, off_t reg)
> > +{
> > +	return readq_relaxed(dev->regs + reg);
> > +}
> > +
> > +static inline void cla_reg_write(struct cla_dev *dev, off_t reg, u64 
> > val)
> > +{
> > +	return writeq_relaxed(val, dev->regs + reg);
> > +}
> 
> For regular devices that have a DMA master, you cannot use
> the relaxed operations by default since they do not serialize
> against DMA transfers.
> 
> To do this properly, you'd have to define separate cla_reg_read()
> and cla_reg_read_relaxed() helpers and then use them as needed,
> ideally with a comment for each relaxed instance to explain why
> that one is both performance critical and safe.
> 
> If for some reason this accelerator is not a DMA master (e.g.
> because it is implemented through CPU microcode and accesses
> the memory through the CPU's own load/store unit), that should
> be documented here to explain that you are relying on
> implementation defined behavior outside of the normal driver
> and memory model.

Good point, this needs better comments. The kernel driver doesn't use any
DMA so the accessors can all be _relaxed. The userspace library, which
does use DMA and MMIO, needs to insert memory barriers when programming
the device.

It's possible we are missing barriers in the context switching path, but
I don't think we need any. The driver forces accelerators to idle which,
in this hardware, guarantees that all DMA initiated by previous user is
complete, and no DMA will be issued until we return to user after
assigning a new context. We don't touch the user buffers but for the page
table walker, the barriers in the arch mm code should be sufficient.

> 
> > +	/*
> > +	 * No barrier needed because accesses use Device-nGnRE, within the 
> > same
> > +	 * memory-mapped peripheral, so accesses arrive at the endpoint in
> > +	 * program order.
> > +	 */
> 
> This comment in turn looks completely useless, as that is true
> for any MMIO device. The only barriers that you'd normally need
> here on sane architectures (not Alpha) are to serialize MMIO
> against DMA.
> 
> > +
> > +	if (launch->data_mode == CLA_DATA_OUT)
> > +		for (i = 0; i < launch->ndata_m1 + 1; i++)
> > +			launch->data[i] = cla_reg_read(dev, CLA_REG_DATA(i));
> 
> Instead of the open-coded loop, maybe this can be built
> on top of __iowrite32_copy()

Ah yes, __iowrite64_copy() should work here (reg accesses must be 64-bit).

> 
> > +/**
> > + * cla_op_wait_lresp - Wait for any LAUNCH op to complete.
> > +int cla_op_wait_lresp(struct cla_dev *dev, u64 *lresp)
> > +{
> > +	return readq_relaxed_poll_timeout_atomic(dev->regs + CLA_REG_LRESP,
> > +			*lresp, FIELD_GET(CLA_LRESP_PENDING, *lresp) == 0,
> > +			CLA_LRESP_DELAY_US, CLA_LRESP_TIMEOUT_US);
> 
> Similarly, the readq_relaxed_poll_timeout_atomic() specifically
> does not wait for DMA, so you may need separate helpers for
> devices that can do DMA and readq_poll_timeout_atomic() vs devices
> that never access memory and can use the relaxed version.

This does not wait for DMA, only for the LAUNCH command issued with MMIO
to complete.

> 
> You may also need a non-atomic version, as blocking the CPU
> for 100µs is not great for realtime workloads.

Agreed

Thanks,
Jean