diff mbox series

[v4,3/8] i2c: mediatek: Reset the handshake signal between i2c and dma

Message ID 1626517079-9057-4-git-send-email-kewei.xu@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Introduce an attribute to choose timing setting | expand

Commit Message

Kewei Xu July 17, 2021, 10:17 a.m. UTC
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.

Signed-off-by: Kewei Xu <kewei.xu@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Chen-Yu Tsai Aug. 11, 2021, 8:41 a.m. UTC | #1
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
Kewei Xu Aug. 21, 2021, 7:40 a.m. UTC | #2
On Wed, 2021-08-11 at 16:41 +0800, Chen-Yu Tsai wrote:
> 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?

Yes, It has been tested on MT8192.

> 
> > 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?
> 

Thank you for your suggestion. We will appropriately add the delay in
the next version of patch according to the hardware behavior
characteristics. 

Regards
Kewei

> > +               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
diff mbox series

Patch

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);
+		mtk_i2c_writew(i2c, I2C_HANDSHAKE_RST, OFFSET_SOFTRESET);
+		mtk_i2c_writew(i2c, I2C_CHN_CLR_FLAG, OFFSET_SOFTRESET);
+		mtk_i2c_writew(i2c, I2C_RELIABILITY | I2C_DMAACK_ENABLE,
+			       OFFSET_DEBUGCTRL);
+	}
+
 	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))