Re: [PATCH 2/4] spi: axi-spi-engine: don't repeat mode config for offload

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2025-04-29 at 10:24 -0500, David Lechner wrote:
> On 4/29/25 4:08 AM, Nuno Sá wrote:
> > Hi David,
> > 
> > The whole series LGTM but I do have a small concern (maybe an hypothetical
> > one)
> > in this one...
> > 
> > 
> > On Mon, 2025-04-28 at 15:58 -0500, David Lechner wrote:
> 
> 
> ...
> 
> > > +
> > > +	writel_relaxed(SPI_ENGINE_CMD_SYNC(0),
> > > +		spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> > > +
> > > +	writel_relaxed(SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_CONFIG,
> > > +					    priv->spi_mode_config),
> > > +		       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> > > +
> > > +	writel_relaxed(SPI_ENGINE_CMD_SYNC(1),
> > > +		spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> > > +
> > 
> > I would assume that SPI_ENGINE_CMD_SYNC(0) + SPI_ENGINE_CMD_SYNC(1) should
> > be
> > executed in order by the core? I think all this relaxed API's don't give any
> > guarantee about store completion so, in theory, we could have SYNC(0) after
> > SYNC(1). Even the full barrier variants don't guarantee this I believe [1].
> > There's ioremap_np() variant but likely not supported in many platforms.
> > Doing a
> > read() before SYNC(1) should be all we need to guarantee proper ordering
> > here.
> > 
> > Or maybe this is all theoretical and not an issue on the platforms this
> > driver
> > is supported but meh, just raising the possibility.
> > 
> > 
> > [1]:
> > https://elixir.bootlin.com/linux/v6.12-rc6/source/Documentation/driver-api/device-io.rst#L299
> > 
> 
> The way I am reading this, relaxed isn't "no barriers", just "weaker
> barriers".

Yes, sometimes just compiler ones. Bad phrasing from me...

> Quoting from the linked doc:
> 
> 	On architectures that require an expensive barrier for serializing
> 	against DMA, these “relaxed” versions of the MMIO accessors only
> 	serialize against each other, but contain a less expensive barrier
> 	operation.
> 
> So that sounds like we should not have a problem to me. (And if there is a
> problem, then it would affect other parts of the driver, like when we load
> the fifos with tx data in a loop.)

Well that just ensures that they are issued by the order you wrote them but it
does not guarantee that they end up in the same order on the peripheral itself.

Anyways, likely not an issue on the arches we care and as you mentioned it,
there are other places of the driver where this could matter. So, I guess not
material for this series.

- Nuno Sá