> -----Original Message----- > From: Sean Anderson <sean.anderson@xxxxxxxx> > Sent: Tuesday, September 20, 2022 11:21 AM > To: Robin Murphy <robin.murphy@xxxxxxx>; Oleksij Rempel > <linux@xxxxxxxxxxxxxxxx>; Pengutronix Kernel Team > <kernel@xxxxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx; linux-arm-kernel > <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; Vinod Koul <vkoul@xxxxxxxxxx>; > dmaengine@xxxxxxxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; Laurentiu Tudor > <laurentiu.tudor@xxxxxxx> > Cc: Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; dri- > devel@xxxxxxxxxxxxxxxxxxxxx; Christian König <christian.koenig@xxxxxxx>; > linaro-mm-sig@xxxxxxxxxxxxxxxx; Shawn Guo <shawnguo@xxxxxxxxxx>; Sumit > Semwal <sumit.semwal@xxxxxxxxxx>; Joy Zou <joy.zou@xxxxxxx>; linux- > media@xxxxxxxxxxxxxxx > 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
