Re: [PATCH] spi: imx: correct handling of MXC_CSPIRXDATA value endianness

[Simon Horman <horms@xxxxxxxxxx>]

On Fri, May 26, 2023 at 04:09:48PM +0200, Ahmad Fatoum wrote:
> Hello Simon,
> 
> On 26.05.23 16:03, Simon Horman wrote:
> > The existing code seems be intended to handle MXC_CSPIRXDATA values
> > which are in big endian. However, it seems that this is only
> > handled correctly in the case where the host is little endian.
> > 
> > First, consider the read case.
> > 
> > 	u32 val = be32_to_cpu(readl(...))
> > 
> > readl() will read a 32bit value and return it after applying le32_to_cpu().
> > On a little endian host le32_to_cpu() is a noop. So the raw value is
> > returned. This is then converted from big endian to host byte-order -
> > the value is byte-swapped - using be32_to_cpu(). Assuming the raw value
> > is big endian a host byte-order value is obtained. This seems correct.
> > 
> > However, on a big endian system, le32_to_cpu() will perform a byte-swap,
> > while be32_to_cpu() is a noop. Assuming the underlying value is big
> > endian this is incorrect, because it should not be byte-swapped to
> > obtain the value in host byte-order - big endian.
> > 
> > Surveying other kernel code it seems that a correct approach is:
> > 
> > 	 be32_to_cpu((__force __be32)__raw_readl(...))
> 
> How about using ioread32be?

...

On Fri, May 26, 2023 at 04:12:46PM +0200, Ahmad Fatoum wrote:
> On 26.05.23 16:09, Ahmad Fatoum wrote:
> >> -	writel(val, spi_imx->base + MXC_CSPITXDATA);
> >> +	__raw_writel((__force u32)cpu_to_be32(val),
> >> +		     spi_imx->base + MXC_CSPITXDATA);
> >>  }
> 
> On more thing: __raw_writel doesn't involve a write barrier (at least
> on ARM). That means above code introduces a bug as the CPU may now reorder
> writes that were sequential before. Both iowrite32be() and readl()
> have a __iowmb(); on ARM before doing the write itself.

Thanks Ahmad,

I agree that ioread32be() and iowrite32be() look like a better solution.
I'll plan to spin a v2 accordingly.