diff mbox series

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

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

Commit Message

Sean Anderson Sept. 19, 2022, 10:24 p.m. UTC
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.

--Sean

Comments

Leo Li Sept. 19, 2022, 10:40 p.m. UTC | #1
> -----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
Sean Anderson Sept. 19, 2022, 10:45 p.m. UTC | #2
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
Robin Murphy Sept. 20, 2022, 10:07 a.m. UTC | #3
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;
Sean Anderson Sept. 20, 2022, 3:24 p.m. UTC | #4
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;
Sean Anderson Sept. 20, 2022, 3:44 p.m. UTC | #5
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;
>
Sean Anderson Sept. 20, 2022, 4:21 p.m. UTC | #6
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
Leo Li Sept. 20, 2022, 10:49 p.m. UTC | #7
> -----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
Leo Li Sept. 20, 2022, 10:58 p.m. UTC | #8
> >
> > 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
Sean Anderson Sept. 20, 2022, 11:05 p.m. UTC | #9
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
Sean Anderson Sept. 22, 2022, 11:10 p.m. UTC | #10
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 mbox series

Patch

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;