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. --Sean [1] https://lore.kernel.org/linux-arm-kernel/20220915233432.31660-6-leoyang.li@xxxxxxx/ >> --Sean >> >> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c >> index 15896e2413c4..1d9d4a55d2af 100644 >> --- a/drivers/dma/fsl-edma-common.c >> +++ b/drivers/dma/fsl-edma-common.c >> @@ -391,6 +391,12 @@ void fsl_edma_fill_tcd(struct fsl_edma_hw_tcd *tcd, u32 src, u32 dst, >> { >> u16 csr = 0; >> + pr_info("src=%8x dst=%8x attr=%4x soff=%4x nbytes=%u slast=%8x\n" >> + "citer=%4x biter=%4x doff=%4x dlast_sga=%8x\n" >> + "major_int=%d disable_req=%d enable_sg=%d\n", >> + src, dst, attr, soff, nbytes, slast, citer, biter, doff, >> + dlast_sga, major_int, disable_req, enable_sg); >> + >> /* >> * eDMA hardware SGs require the TCDs to be stored in little >> * endian format irrespective of the register endian model. >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c >> index 3576b63a6c03..0217f0cb1331 100644 >> --- a/drivers/i2c/busses/i2c-imx.c >> +++ b/drivers/i2c/busses/i2c-imx.c >> @@ -402,6 +402,9 @@ static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx, >> dev_err(dev, "DMA mapping failed\n"); >> goto err_map; >> } >> + phys_addr_t bufp = virt_to_phys(msgs->buf); >> + dev_info(dev, "%px %pap %pad %px\n", msgs->buf, &bufp, >> + &dma->dma_buf, phys_to_virt(dma->dma_buf)); >> txdesc = dmaengine_prep_slave_single(dma->chan_using, dma->dma_buf, >> dma->dma_len, dma->dma_transfer_dir, >> @@ -965,6 +968,9 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx, >> } >> schedule(); >> } >> + dev_info(dev, "%px = [%*ph]\n", msgs->buf, msgs->len, msgs->buf); >> + dev_info(dev, "%px = [%*ph]\n", phys_to_virt(dma->dma_buf), msgs->len, >> + phys_to_virt(dma->dma_buf)); >> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); >> temp &= ~I2CR_DMAEN;
