| Message ID | 38974aab-06d0-f5ff-d359-5eedd2f3bb3e@seco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [BUG] ls1046a: eDMA does not transfer data from I2C | expand |
> -----Original Message----- > From: Sean Anderson <sean.anderson@seco.com> > Sent: Monday, September 19, 2022 5:24 PM > To: 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 > Cc: Sumit Semwal <sumit.semwal@linaro.org>; Christian König > <christian.koenig@amd.com>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; linux-media@vger.kernel.org; dri- > devel@lists.freedesktop.org; linaro-mm-sig@lists.linaro.org; Joy Zou > <joy.zou@nxp.com>; Peng Ma <peng.ma@nxp.com>; Robin Gong > <yibin.gong@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Leo Li > <leoyang.li@nxp.com> > Subject: [BUG] ls1046a: eDMA does not transfer data from I2C > > 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: What is the kernel tree/tag that you are testing with? Regards, Leo
On 9/19/22 6:40 PM, Leo Li wrote: > > >> -----Original Message----- >> From: Sean Anderson <sean.anderson@seco.com> >> Sent: Monday, September 19, 2022 5:24 PM >> To: 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 >> Cc: Sumit Semwal <sumit.semwal@linaro.org>; Christian König >> <christian.koenig@amd.com>; Linux Kernel Mailing List <linux- >> kernel@vger.kernel.org>; linux-media@vger.kernel.org; dri- >> devel@lists.freedesktop.org; linaro-mm-sig@lists.linaro.org; Joy Zou >> <joy.zou@nxp.com>; Peng Ma <peng.ma@nxp.com>; Robin Gong >> <yibin.gong@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Leo Li >> <leoyang.li@nxp.com> >> Subject: [BUG] ls1046a: eDMA does not transfer data from I2C >> >> 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: > > What is the kernel tree/tag that you are testing with? 5.15.40 Sorry, I forgot to put that in the first email. I can test with linux/master, but I reviewed the different commits and I did not think there would be any difference. I also cherry-picked the eDMA memcpy commit, but the dma test passed. --Sean
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. Robin. > --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;
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@nxp.com/ >> --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;
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? --Sean > --Sean > > [1] https://lore.kernel.org/linux-arm-kernel/20220915233432.31660-6-leoyang.li@nxp.com/ > >>> --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; >
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? 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? --Sean
> -----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. > > 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
> > > > 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, So the coherent DMA write only gets data into the cache not also the DRAM? Otherwise a read back would get the updated data too. - Leo > 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. > > Robin. > > > --Sean
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. [1] https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/cpu/armv8/fsl-layerscape/soc.c#L680 >> >> 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. --Sean
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-layerscape/soc.c#L680 > >>> >>> 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.
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;