Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1626517079-9057-1-git-send-email-kewei.xu@mediatek.com> <1626517079-9057-4-git-send-email-kewei.xu@mediatek.com>
In-Reply-To: <1626517079-9057-4-git-send-email-kewei.xu@mediatek.com>
From:   Chen-Yu Tsai <wenst@chromium.org>
Date:   Wed, 11 Aug 2021 16:41:04 +0800
Message-ID: <CAGXv+5EGpUV0eaQ6duZ8y8u2dvc1y3uc2YwWP=07D+ZZpzMCvQ@mail.gmail.com>
Subject: Re: [PATCH v4 3/8] i2c: mediatek: Reset the handshake signal between
 i2c and dma
To:     Kewei Xu <kewei.xu@mediatek.com>
Cc:     wsa@the-dreams.de, Matthias Brugger <matthias.bgg@gmail.com>,
        Rob Herring <robh+dt@kernel.org>, linux-i2c@vger.kernel.org,
        Devicetree List <devicetree@vger.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>,
        srv_heupstream <srv_heupstream@mediatek.com>,
        leilk.liu@mediatek.com, qii.wang@mediatek.com,
        yuhan.wei@mediatek.com, ot_daolong.zhu@mediatek.com,
        liguo.zhang@mediatek.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Sat, Jul 17, 2021 at 6:29 PM Kewei Xu <kewei.xu@mediatek.com> wrote:
>
> Due to changes in the hardware design of the handshaking signal
> between i2c and dma, it is necessary to reset the handshaking
> signal before each transfer to ensure that the multi-msgs can
> be transferred correctly.

This also affects MT8192. Has this been tested on that SoC as well?

> Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index 222ff765e55d..c0108387f34b 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -47,6 +47,9 @@
>  #define I2C_RD_TRANAC_VALUE            0x0001
>  #define I2C_SCL_MIS_COMP_VALUE         0x0000
>  #define I2C_CHN_CLR_FLAG               0x0000
> +#define I2C_CLR_DEBUGCTR               0x0000
> +#define I2C_RELIABILITY                        0x0010
> +#define I2C_DMAACK_ENABLE              0x0008
>
>  #define I2C_DMA_CON_TX                 0x0000
>  #define I2C_DMA_CON_RX                 0x0001
> @@ -850,6 +853,17 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
>
>         reinit_completion(&i2c->msg_complete);
>
> +       if (i2c->dev_comp->apdma_sync) {
> +               mtk_i2c_writew(i2c, I2C_CLR_DEBUGCTR, OFFSET_DEBUGCTRL);
> +               writel(I2C_DMA_HANDSHAKE_RST | I2C_DMA_WARM_RST,
> +                      i2c->pdmabase + OFFSET_RST);
> +               writel(I2C_DMA_CLR_FLAG, i2c->pdmabase + OFFSET_RST);

I2C_DMA_WARM_RST is self-clearing. Is I2C_DMA_HANDSHAKE_RST not
self-clearing? If both are self-clearing, don't you need to wait and
check for them to cleared? If they aren't self-clearing, do you need
to delay some time for them to complete?

> +               mtk_i2c_writew(i2c, I2C_HANDSHAKE_RST, OFFSET_SOFTRESET);
> +               mtk_i2c_writew(i2c, I2C_CHN_CLR_FLAG, OFFSET_SOFTRESET);

Same here. No time delay needed?

> +               mtk_i2c_writew(i2c, I2C_RELIABILITY | I2C_DMAACK_ENABLE,
> +                              OFFSET_DEBUGCTRL);

A comment explaining what the section above does would be nice. AFAICU
this is force resetting the DMA handling.


Regards
ChenYu

> +       }
> +
>         control_reg = mtk_i2c_readw(i2c, OFFSET_CONTROL) &
>                         ~(I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS);
>         if ((i2c->speed_hz > I2C_MAX_FAST_MODE_PLUS_FREQ) || (left_num >= 1))
> --
> 2.18.0
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek