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

[Greg Ungerer <gerg@kernel.org>]

Hi Angelo,

(Adding Christoph to CC list)

On 25/5/26 07:34, 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@lst.de>
>> 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@lst.de>
>>      Reviewed-by: Greg Ungerer <gerg@linux-m68k.org>
>>      Tested-by: Greg Ungerer <gerg@linux-m68k.org>
>>
>>   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".

I don't think that is right. The way the underlying data cache is setup for
MMU ColdFire (via the ACR/CACR registers) means that individual pages cannot
be marked as non-cached. So coherent memory allocations are not possible -
at least the way things are today.

It would be possible to set aside a chunk of RAM at kernel startup time
to use as a pool for coherent allocations (since it could be marked as
non-cached via the ACR/CACR registers), but there is no code to support doing
that today.

Regards
Greg


> 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