[BUG] ls1046a: eDMA does not transfer data from I2C

Tue Sep 20 15:49:28 PDT 2022

> -----Original Message-----
> From: Sean Anderson <sean.anderson at seco.com>
> Sent: Tuesday, September 20, 2022 11:21 AM
> To: Robin Murphy <robin.murphy at arm.com>; Oleksij Rempel
> <linux at rempel-privat.de>; Pengutronix Kernel Team
> <kernel at pengutronix.de>; linux-i2c at vger.kernel.org; linux-arm-kernel
> <linux-arm-kernel at lists.infradead.org>; Vinod Koul <vkoul at kernel.org>;
> dmaengine at vger.kernel.org; Leo Li <leoyang.li at nxp.com>; Laurentiu Tudor
> <laurentiu.tudor at nxp.com>
> Cc: Linux Kernel Mailing List <linux-kernel at vger.kernel.org>; dri-
> devel at lists.freedesktop.org; Christian König <christian.koenig at amd.com>;
> linaro-mm-sig at lists.linaro.org; Shawn Guo <shawnguo at kernel.org>; Sumit
> Semwal <sumit.semwal at linaro.org>; Joy Zou <joy.zou at nxp.com>; linux-
> media at vger.kernel.org
> Subject: Re: [BUG] ls1046a: eDMA does not transfer data from I2C
> 
> 
> 
> On 9/20/22 11:44 AM, Sean Anderson wrote:
> >
> >
> > On 9/20/22 11:24 AM, Sean Anderson wrote:
> >>
> >>
> >> On 9/20/22 6:07 AM, Robin Murphy wrote:
> >>> On 2022-09-19 23:24, Sean Anderson wrote:
> >>>> Hi all,
> >>>>
> >>>> I discovered a bug in either imx_i2c or fsl-edma on the LS1046A
> >>>> where no data is read in i2c_imx_dma_read except for the last two
> >>>> bytes (which are not read using DMA). This is perhaps best
> >>>> illustrated with the following example:
> >>>>
> >>>> # hexdump -C /sys/bus/nvmem/devices/0-00540/nvmem
> >>>> [  308.914884] i2c i2c-0: ffff000809380000 0x0000000889380000
> 0x00000000f5401000 ffff000075401000
> >>>> [  308.923529] src= 2180004 dst=f5401000 attr=   0 soff=   0 nbytes=1
> slast=       0
> >>>> [  308.923529] citer=  7e biter=  7e doff=   1 dlast_sga=       0
> >>>> [  308.923529] major_int=1 disable_req=1 enable_sg=0 [  308.942113]
> >>>> fsl-edma 2c00000.edma: vchan 000000001b4371fc: txd
> >>>> 00000000d9dd26c5[4]: submitted [  308.974049] fsl-edma
> >>>> 2c00000.edma: txd 00000000d9dd26c5[4]: marked complete [
> >>>> 308.981339] i2c i2c-0: ffff000809380000 = [2e 2e 2f 2e 2e 2f 2e 2e
> >>>> 2f 64 65 76 69 63 65 73 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31 38 30
> 30 30 30 2e 69 32 63 2f 69 32 63 2d 30 2f 30 2d 30 30 35 34 2f 30 2d 30 30 35 34
> 30 00 00] [  309.002226] i2c i2c-0: ffff000075401000 = [2e 2e 2f 2e 2e 2f 2e 2e 2f
> 64 65 76 69 63 65 73 2f 70 6c 61 74 66 6f 72 6d 2f 73 6f 63 2f 32 31 38 30 30 30 30
> 2e 69 32 63 2f 69 32 63 2d 30 2f 30 2d 30 30 35 34 2f 30 2d 30 30 35 34 30 00 00]
> [  309.024649] i2c i2c-0: ffff000809380080 0x0000000889380080
> 0x00000000f5401800 ffff000075401800
> >>>> [  309.033270] src= 2180004 dst=f5401800 attr=   0 soff=   0 nbytes=1
> slast=       0
> >>>> [  309.033270] citer=  7e biter=  7e doff=   1 dlast_sga=       0
> >>>> [  309.033270] major_int=1 disable_req=1 enable_sg=0 [  309.051633]
> >>>> fsl-edma 2c00000.edma: vchan 000000001b4371fc: txd
> >>>> 00000000d9dd26c5[5]: submitted [  309.083526] fsl-edma
> >>>> 2c00000.edma: txd 00000000d9dd26c5[5]: marked complete [
> >>>> 309.090807] i2c i2c-0: ffff000809380080 = [00 00 00 00 00 00 00 00
> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>> 00 00 00 00 00 00 00 00 00 00 00 00] [  309.111694] i2c i2c-0:
> >>>> ffff000075401800 = [00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>>> 00 00 00 00]
> >>>> 00000000  2e 2e 2f 2e 2e 2f 2e 2e  2f 64 65 76 69 63 65 73
> >>>> |../../../devices|
> >>>> 00000010  2f 70 6c 61 74 66 6f 72  6d 2f 73 6f 63 2f 32 31
> >>>> |/platform/soc/21|
> >>>> 00000020  38 30 30 30 30 2e 69 32  63 2f 69 32 63 2d 30 2f
> >>>> |80000.i2c/i2c-0/|
> >>>> 00000030  30 2d 30 30 35 34 2f 30  2d 30 30 35 34 30 00 00
> >>>> |0-0054/0-00540..|
> >>>> 00000040  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
> >>>> |................|
> >>>> *
> >>>> 00000070  00 00 00 00 00 00 00 00  00 00 00 00 00 00 ff ff
> >>>> |................|
> >>>> 00000080  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
> >>>> |................|
> >>>> *
> >>>> 000000f0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 ff 5b
> >>>> |...............[|
> >>>> 00000100
> >>>>
> >>>> (patch with my debug prints appended below)
> >>>>
> >>>> Despite the DMA completing successfully, no data was copied into
> >>>> the buffer, leaving the original (now junk) contents. I probed the
> >>>> I2C bus with an oscilloscope, and I verified that the transfer did indeed
> occur.
> >>>> The timing between submission and completion seems reasonable for
> >>>> the bus speed (50 kHz for whatever reason).
> >>>>
> >>>> I had a look over the I2C driver, and nothing looked obviously
> >>>> incorrect. If anyone has ideas on what to try, I'm more than willing.
> >>>
> >>> Is the DMA controller cache-coherent? I see the mainline LS1046A DT
> doesn't have a "dma-coherent" property for it, but the behaviour is entirely
> consistent with that being wrong - dma_map_single() cleans the cache,
> coherent DMA write hits the still-present cache lines, dma_unmap_single()
> invalidates the cache, and boom, the data is gone and you read back the
> previous content of the buffer that was cleaned out to DRAM beforehand.
> >>
> >> I've tried both with and without [1] applied. I also tried removing
> >> the call to dma_unmap_single, but to no effect.
> >
> > Actually, I wasn't updating my device tree like I thought...
> >
> > Turns out I2C works only *without* this patch.
> >
> > So maybe the eDMA is not coherent?
> 
> It seems like this might be the case. From the reference manual:
> 
> > All transactions from eDMA are tagged as snoop configuration if the
> > SCFG_SNPCNFGCR[eDMASNP] bit is set. Refer Snoop Configuration
> Register
> > (SCFG_SNPCNFGCR) for details.
> 
> But there is no such bit in this register on the LS1046A. On the LS1043A, this
> bit does exist, but on the LS1046A it is reserved. I read the register, and
> found the bit was 0. Perhaps eDMA was intended to be coherent, but there
> was a hardware bug?

Thanks for the findings.  I don't know the reason why this bit is reserved on LS1046a either.  It should have a similar design as LS1043a.

> 
> If this is the case, then I think dma-coherent should be left out of the top-
> level /soc node. Instead, the qdma, sata, usb, and emmc nodes should have
> dma-coherent added.
> 
> Li/Laurentiu, what are your thoughts?

Then looks like it is not correct to say all devices on the bus is coherent.  But as we have the new "dma-noncoherent" property now and most of the devices are actually coherent, we probably can keep the bus as dma-coherent and mark exceptions with dma-noncoherent?

Regards,
Leo