Re: [RFC 4/4] m68k: coldfire: fix non-standard readX()/writeX() functions

[Angelo Dureghello]

Hi Greg and all,

so i posted a patch this morning to have edma driver back working.
If approved, i can proceed testing this RFC (adjusting it as needed)
with edma and dspi driver.

Regards,
angelo

On Sun, May 24, 2026 at 04:34:03PM -0500, Angelo Dureghello wrote:
> On Sun, May 24, 2026 at 02:17:07PM -0700, Angelo Dureghello wrote:
> > Hi All,
> >
> > On Sun, May 17, 2026 at 03:41:31PM -0700, Angelo Dureghello wrote:
> > > Hi,
> > >
> > > On Sun, May 17, 2026 at 03:04:23PM -0700, Angelo Dureghello wrote:
> > > > Hi Arnd,
> > > >
> > > > On Sun, May 17, 2026 at 10:08:22PM +0200, Arnd Bergmann wrote:
> > > > > On Sun, May 17, 2026, at 21:43, Angelo Dureghello wrote:
> > > > > > On Thu, May 07, 2026 at 10:43:01PM +1000, Greg Ungerer wrote:
> > > > > >> On 7/5/26 05:12, Arnd Bergmann wrote:
> > > > > >> > On Wed, May 6, 2026, at 16:26, Greg Ungerer wrote:
> > > > > >
> > > > > > [    2.270000] fsl-dspi fsl-dspi.0: Not able to get desc for DMA xfer
> > > > > > [    2.280000] fsl-dspi fsl-dspi.0: DMA transfer failed
> > > > > > [    2.280000] spi_master spi0: failed to transfer one message from queue
> > > > > > [    2.290000] spi_master spi0: noqueue transfer failed
> > > > > > [    2.290000] spi-nor spi0.1: probe with driver spi-nor failed with error -5
> > > > > >
> > >
> > > About this issue, it fails on dma_pool_alloc(), so tomorrow will check,
> > > i probably lost some dma config option.
> > >
> >
> > so i worked on this open issue above:
> >
> > - moved to master and rebased,
> > - crated a wip/edma branch,
> > - bisected and found the offending commit, before this, mcf-edma driver
> >   and connected spi-fsl-dspi (using edma) was both working correctly.
> >
> > 7a360df941a4bd60847208de59f1ac8b166265a2 is the first bad commit
> > commit 7a360df941a4bd60847208de59f1ac8b166265a2 (HEAD)
> > Author: Christoph Hellwig <hch@xxxxxx>
> > Date:   Thu Oct 12 09:52:27 2023 +0200
> >
> >     m68k: don't provide arch_dma_alloc for nommu/coldfire
> >
> >     Coldfire cores configured with a data cache can't provide coherent
> >     DMA allocations at all.
> >
> >     Instead of returning non-coherent kernel memory in this case,
> >     return NULL and fail the allocation.
> >
> >     The only driver that used to rely on the previous behavior (fec) has
> >     been switched to use non-coherent allocations for this case recently.
> >
> >     Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> >     Reviewed-by: Greg Ungerer <gerg@xxxxxxxxxxxxxx>
> >     Tested-by: Greg Ungerer <gerg@xxxxxxxxxxxxxx>
> >
> >  arch/m68k/Kconfig      |  1 -
> >  arch/m68k/kernel/dma.c | 23 -----------------------
> >  2 files changed, 24 deletions(-)
> >
> > So i can try next week a patch for edma looking what has been done
> > in fec, and since i am probably the only with mcf54415, will test it
> > here.
> >
>
> Looking into this better, looks like the above commit was meant for the
> majority on non-mmu ColdFire. I think mcf5441x and some other with mmu
> enabled can flag pages as "page cache disabled".
>
> So i would re-enabled that code only for such mmu families.
>
> Please let me know if i am correct.
> Thanks.
>
> > > > > > DSPI is using edma, i will try to understand where the issue is asap.
> > > > > >
> > > > > > About how it works:
> > > > > > - for accesses to edma module (IP) mmio registers, must be native
> > > > > > big_endian, so using the "be" suffix in "mcf"_edma looks ok for me.
> > > > >
> > > > > The twist here is that with the way that readl() is defined on
> > > > > coldfire as a non-swapping operation, and the generic
> > > > > definition assuming the opposite in
> > > > >
> > > > > static inline u32 ioread32be(const void __iomem *addr)
> > > > > {
> > > > >         return swab32(readl(addr));
> > > > > }
> > > > >
> > > > > the function called ioread32be() actually tries to access
> > > > > the registers as little-endian. I can see two possible ways
> > > > > we got here, but don't know which one is currect:
> > > > >
> > > > > a) the device actually has little-endian registers (like it
> > > > >    does on i.MX, but unlike all other coldfire devices), and
> > > > >    you just never noticed because using ioread32be() worked
> > > > >    as you expected.
> > > > >
> > > > > b) you tested the driver using an ioread32be() definition that
> > > > >    did not have a byteswap and it correctly accessed big-endian
> > > > >    registers at the time, but the version in mainline today does
> > > > >    not.
> > > >
> > > > Ok. The ioread32be now works properly since i had applied Greg patches.
> > > > I generated an error in _probe on edma channel 2, reading status reg.
> > > > looks consistent:
> > > >
> > > > 	iowrite16(2121, regs->erqh);
> > > > 	iowrite8(0x77, regs->serq);
> > > > 	iowrite8(0x12, regs->ssrt);
> > > > 	
> > > > 	u32 status = ioread32be(regs->es);
> > > > 	printk("%s() status: %04x\n", __func__, status);
> > > >
> > > > [    0.140000] mcf_edma_probe() entering
> > > > [    0.140000] mcf_edma_probe(): allocating data
> > > > [    0.140000] mcf_edma_probe() status: 800012f8
> > > >
> > > > If i am not loosing myself in this r/w labyrinth, the path should be:
> > > >
> > > > 1) Greg removed coldfire readl/writel, leaving now the standard LE r/w,
> > > > 2) So the ioread32be swaps the standard LE read giving BE.
> > > >
> > > > Am i correct ?
> > > >
> > > >
> > > > >
> > > > > > - for accessing the "tcd" memory structure, that must be, from what i
> > > > > > remember, anyway in little endian, independently from the cpu core
> > > > > > endiannes, this is the reason that big_endian flag is needed, it is
> > > > > > used for tcd area accesses, so the IP module was built.
> > > > > > The tcd area may be similar to pci accesses (see mcf54415 RM 19.4.16).
> > > > >
> > > > > edma_read_tcdreg() calls into edma_readl(), which is the same function
> > > > > that is used for normal register access, so from what I can tell,
> > > > > they always use the same endianess here.
> > > > >
> > > >
> > > > If edma_readl was using
> > > >
> > > >         if (edma->big_endian)
> > > >                 val = ioread32be(addr);
> > > >
> > > > and never changed, without Greg patch, it was likely returning little
> > > > endian for coldfire and correct LE for other arch ? :)
> > > >
> > > > I remember something about tcd area was coded LE, but will investigate
> > > > better, now i am over midnight.
> > > >
> > > > Regards,
> > > > angelo
> > > >
> > > > >       Arnd
> > >
> > > Regards,
> > > angelo
> >
> > Regards,
> > angelo
>
> Regards,
> angelo