On 9/20/22 7:05 PM, Sean Anderson wrote:
On 9/20/22 6:49 PM, Leo Li wrote:
-----Original Message----- From: Sean Anderson sean.anderson@seco.com Sent: Tuesday, September 20, 2022 11:21 AM To: Robin Murphy robin.murphy@arm.com; Oleksij Rempel linux@rempel-privat.de; Pengutronix Kernel Team kernel@pengutronix.de; linux-i2c@vger.kernel.org; linux-arm-kernel linux-arm-kernel@lists.infradead.org; Vinod Koul vkoul@kernel.org; dmaengine@vger.kernel.org; Leo Li leoyang.li@nxp.com; Laurentiu Tudor laurentiu.tudor@nxp.com Cc: Linux Kernel Mailing List linux-kernel@vger.kernel.org; dri- devel@lists.freedesktop.org; Christian König christian.koenig@amd.com; linaro-mm-sig@lists.linaro.org; Shawn Guo shawnguo@kernel.org; Sumit Semwal sumit.semwal@linaro.org; Joy Zou joy.zou@nxp.com; linux- media@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.
Funnily enough, this bit is not set for the LS1043A either [1]. So maybe this is a U-Boot bug? I'll test this tomorrow.
OK, this looks like it fixes things. I'll submit a patch to U-Boot. But this bit should really be documented in the LS1046A manual as well.
--Sean
[1] https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/cpu/armv8/fsl-la...
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?
Neat, I didn't know about that.
Yes, that sounds good. For the moment, only i2c0 uses DMA, so we will only need it there. At some point, someone should send a patch enabling it for the rest of the I2C devices, as well as the LPUARTs and SPIs.