[PATCH 0/4] Fix rockchip I2C bus

[PATCH 0/4] Fix rockchip I2C bus

[Gerald Loacker]

The rockchip I2C driver does not send a stop condition, this violates the
timing on the SCL line. iThe first patch fixes this and puts all related
functions on the same level.

Furthermore, we have seen nested calls to the rockchip_i2c_xfer function.
The second patch adds a check of pending interrupts and avoids
interrupting an ongoing transfer.

The last two patches propagate I2C errors from the KSZ9477 driver to the
DSA subsystem to react accordingly.

To: barebox@lists.infradead.org

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
Gerald Loacker (4):
      i2c: rockchip: fix i2c stop condition
      i2c: rockchip: ignore i2c transfers when another transfer is running
      net: ksz9477: propagate phy read error
      net: ksz9477: propagate phy write error

 drivers/i2c/busses/i2c-rockchip.c | 35 +++++++++++++++++++++--------------
 drivers/net/ksz9477.c             | 15 +++++++--------
 2 files changed, 28 insertions(+), 22 deletions(-)
---
base-commit: 4411b931680e4fb15d6f80e5543ef9f81aef092b
change-id: 20230908-bugfix-i2c-rockchip-f8874bd640e5

Best regards,
-- 
Gerald Loacker <gerald.loacker@wolfvision.net>

[PATCH 1/4] i2c: rockchip: fix i2c stop condition

[Gerald Loacker]

The i2c bus gets disabled without sending a stop condition. This violates
i2c timing on the clock line. Fix this and include all related functions
(rk_i2c_send_start_bit, rk_i2c_send_stop_bit, rk_i2c_disable) onto the same
level.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 drivers/i2c/busses/i2c-rockchip.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rockchip.c b/drivers/i2c/busses/i2c-rockchip.c
index 23bf4a55d7..1bca3e9913 100644
--- a/drivers/i2c/busses/i2c-rockchip.c
+++ b/drivers/i2c/busses/i2c-rockchip.c
@@ -222,10 +222,6 @@ static int rk_i2c_read(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
 	dev_dbg(dev, "rk_i2c_read: chip = %d, reg = %d, r_len = %d, b_len = %d\n",
 	      chip, reg, r_len, b_len);
 
-	err = rk_i2c_send_start_bit(i2c);
-	if (err)
-		return err;
-
 	writel(I2C_MRXADDR_SET(1, chip << 1 | 1), &regs->mrxaddr);
 	if (r_len == 0) {
 		writel(0, &regs->mrxraddr);
@@ -294,8 +290,6 @@ static int rk_i2c_read(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
 i2c_exit:
 	if (err)
 		rk_i2c_show_regs(i2c);
-	rk_i2c_disable(i2c);
-
 	return err;
 }
 
@@ -315,9 +309,6 @@ static int rk_i2c_write(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
 
 	dev_dbg(dev, "rk_i2c_write: chip = %d, reg = %d, r_len = %d, b_len = %d\n",
 	      chip, reg, r_len, b_len);
-	err = rk_i2c_send_start_bit(i2c);
-	if (err)
-		return err;
 
 	while (bytes_remain_len) {
 		if (bytes_remain_len > RK_I2C_FIFO_SIZE)
@@ -370,8 +361,6 @@ static int rk_i2c_write(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
 i2c_exit:
 	if (err)
 		rk_i2c_show_regs(i2c);
-	rk_i2c_disable(i2c);
-
 	return err;
 }
 
@@ -387,6 +376,11 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 		struct i2c_msg *msg = &msgs[i];
 
 		dev_dbg(dev, "i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
+
+		ret = rk_i2c_send_start_bit(i2c);
+		if (ret)
+			break;
+
 		if (msg->flags & I2C_M_RD) {
 			ret = rk_i2c_read(i2c, msg->addr, 0, 0, msg->buf,
 					  msg->len);
@@ -394,6 +388,9 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 			ret = rk_i2c_write(i2c, msg->addr, 0, 0, msg->buf,
 					   msg->len);
 		}
+
+		rk_i2c_send_stop_bit(i2c);
+
 		if (ret) {
 			dev_dbg(dev, "i2c_write: error sending: %pe\n",
 				ERR_PTR(ret));
@@ -402,9 +399,7 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 		}
 	}
 
-	rk_i2c_send_stop_bit(i2c);
 	rk_i2c_disable(i2c);
-
 	return ret < 0 ? ret : nmsgs;
 }
 

-- 
2.37.2

[PATCH 2/4] i2c: rockchip: ignore i2c transfers when another transfer is running

[Gerald Loacker]

It may happen that an i2c transfer is requested by a callback although
there is an other i2c transfer running. In this case do not interrupt the
transfer and return with an error.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 drivers/i2c/busses/i2c-rockchip.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rockchip.c b/drivers/i2c/busses/i2c-rockchip.c
index 1bca3e9913..a869b9d0b7 100644
--- a/drivers/i2c/busses/i2c-rockchip.c
+++ b/drivers/i2c/busses/i2c-rockchip.c
@@ -369,7 +369,19 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 {
 	struct rk_i2c *i2c = to_rk_i2c(adapter);
 	struct device *dev = &adapter->dev;
-	int i, ret = 0;
+	struct i2c_regs *regs = i2c->regs;
+	int i, ret = 0, val;
+
+	val = readl(&regs->con);
+	if (val & I2C_CON_EN) {
+		val = readl(&regs->con);
+		if (val & I2C_IPD_ALL_CLEAN) {
+			dev_dbg(dev,
+				"i2c_xfer: %d messages dropped due to pending interrupts\n",
+				nmsgs);
+			return -EAGAIN;
+		}
+	}
 
 	dev_dbg(dev, "i2c_xfer: %d messages\n", nmsgs);
 	for (i = 0; i < nmsgs; i++) {

-- 
2.37.2

[PATCH 3/4] net: ksz9477: propagate phy read error

[Gerald Loacker]

In case of an error we should not return an arbitrary value, propagate the
error code instead.
Fix return value in case of address error.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 drivers/net/ksz9477.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ksz9477.c b/drivers/net/ksz9477.c
index 9665e0f723..d664b7cf01 100644
--- a/drivers/net/ksz9477.c
+++ b/drivers/net/ksz9477.c
@@ -29,14 +29,15 @@ static int ksz9477_phy_read16(struct dsa_switch *ds, int addr, int reg)
 {
 	struct device *dev = ds->dev;
 	struct ksz_switch *priv = dev_get_priv(dev);
-	u16 val = 0xffff;
+	u16 val;
+	int ret;
 
 	if (addr >= priv->phy_port_cnt)
-		return val;
+		return -EINVAL;
 
-	ksz_pread16(priv, addr, 0x100 + (reg << 1), &val);
+	ret = ksz_pread16(priv, addr, 0x100 + (reg << 1), &val);
 
-	return val;
+	return (ret < 0) ? ret : val;
 }
 
 static int ksz9477_phy_write16(struct dsa_switch *ds, int addr, int reg,

-- 
2.37.2

[PATCH 4/4] net: ksz9477: propagate phy write error

[Gerald Loacker]

Return -EINVAL in case of an address error, otherwise propagate error code.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 drivers/net/ksz9477.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ksz9477.c b/drivers/net/ksz9477.c
index d664b7cf01..0310e0260a 100644
--- a/drivers/net/ksz9477.c
+++ b/drivers/net/ksz9477.c
@@ -48,14 +48,12 @@ static int ksz9477_phy_write16(struct dsa_switch *ds, int addr, int reg,
 
 	/* No real PHY after this. */
 	if (addr >= priv->phy_port_cnt)
-		return 0;
+		return -EINVAL;
 
 	/* No gigabit support.  Do not write to this register. */
 	if (!(priv->features & GBIT_SUPPORT) && reg == MII_CTRL1000)
 		return 0;
-	ksz_pwrite16(priv, addr, 0x100 + (reg << 1), val);
-
-	return 0;
+	return ksz_pwrite16(priv, addr, 0x100 + (reg << 1), val);
 }
 
 static int ksz9477_switch_detect(struct ksz_switch *priv)

-- 
2.37.2

Re: [PATCH 2/4] i2c: rockchip: ignore i2c transfers when another transfer is running

[Sascha Hauer]

On Fri, Sep 08, 2023 at 12:16:47PM +0200, Gerald Loacker wrote:
> It may happen that an i2c transfer is requested by a callback although
> there is an other i2c transfer running. In this case do not interrupt the
> transfer and return with an error.
> 
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> ---
>  drivers/i2c/busses/i2c-rockchip.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rockchip.c b/drivers/i2c/busses/i2c-rockchip.c
> index 1bca3e9913..a869b9d0b7 100644
> --- a/drivers/i2c/busses/i2c-rockchip.c
> +++ b/drivers/i2c/busses/i2c-rockchip.c
> @@ -369,7 +369,19 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  {
>  	struct rk_i2c *i2c = to_rk_i2c(adapter);
>  	struct device *dev = &adapter->dev;
> -	int i, ret = 0;
> +	struct i2c_regs *regs = i2c->regs;
> +	int i, ret = 0, val;
> +
> +	val = readl(&regs->con);
> +	if (val & I2C_CON_EN) {
> +		val = readl(&regs->con);
> +		if (val & I2C_IPD_ALL_CLEAN) {
> +			dev_dbg(dev,
> +				"i2c_xfer: %d messages dropped due to pending interrupts\n",
> +				nmsgs);
> +			return -EAGAIN;
> +		}
> +	}

Can you have a look how this can happen? Normally this should only
happen if you have a heartbeat LED behind a I2C GPIO expander or some
other unusual setup. Adding a dump_stack() next to the dev_dbg() call
might give a clue.

Let's see how this really happens, but generally I think we should find
another solution for this, either on I2C core level or by replacing
the readl_poll_timeout() with a variant that doesn't allow running in
the background, i.e. one which uses is_timeout_non_interruptible()
rather than is_timeout().

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/4] i2c: rockchip: ignore i2c transfers when another transfer is running

[Sascha Hauer]

On Fri, Sep 08, 2023 at 01:51:32PM +0200, Sascha Hauer wrote:
> On Fri, Sep 08, 2023 at 12:16:47PM +0200, Gerald Loacker wrote:
> > It may happen that an i2c transfer is requested by a callback although
> > there is an other i2c transfer running. In this case do not interrupt the
> > transfer and return with an error.
> > 
> > Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> > ---
> >  drivers/i2c/busses/i2c-rockchip.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-rockchip.c b/drivers/i2c/busses/i2c-rockchip.c
> > index 1bca3e9913..a869b9d0b7 100644
> > --- a/drivers/i2c/busses/i2c-rockchip.c
> > +++ b/drivers/i2c/busses/i2c-rockchip.c
> > @@ -369,7 +369,19 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> >  {
> >  	struct rk_i2c *i2c = to_rk_i2c(adapter);
> >  	struct device *dev = &adapter->dev;
> > -	int i, ret = 0;
> > +	struct i2c_regs *regs = i2c->regs;
> > +	int i, ret = 0, val;
> > +
> > +	val = readl(&regs->con);
> > +	if (val & I2C_CON_EN) {
> > +		val = readl(&regs->con);
> > +		if (val & I2C_IPD_ALL_CLEAN) {
> > +			dev_dbg(dev,
> > +				"i2c_xfer: %d messages dropped due to pending interrupts\n",
> > +				nmsgs);
> > +			return -EAGAIN;
> > +		}
> > +	}
> 
> Can you have a look how this can happen? Normally this should only
> happen if you have a heartbeat LED behind a I2C GPIO expander or some
> other unusual setup. Adding a dump_stack() next to the dev_dbg() call
> might give a clue.

I just realized you sent some ksz9477 along with this series. Are you
using that in I2C mode? In that case it could be the periodic link check
that does I2C accesses in the background.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] net: ksz9477: propagate phy read error

[Ahmad Fatoum]

On 08.09.23 12:16, Gerald Loacker wrote:
> In case of an error we should not return an arbitrary value, propagate the
> error code instead.
> Fix return value in case of address error.
> 
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> ---
>  drivers/net/ksz9477.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ksz9477.c b/drivers/net/ksz9477.c
> index 9665e0f723..d664b7cf01 100644
> --- a/drivers/net/ksz9477.c
> +++ b/drivers/net/ksz9477.c
> @@ -29,14 +29,15 @@ static int ksz9477_phy_read16(struct dsa_switch *ds, int addr, int reg)
>  {
>  	struct device *dev = ds->dev;
>  	struct ksz_switch *priv = dev_get_priv(dev);
> -	u16 val = 0xffff;
> +	u16 val;
> +	int ret;
>  
>  	if (addr >= priv->phy_port_cnt)
> -		return val;
> +		return -EINVAL;

Looks sensible IMO, but shouldn't we do the same in dsa_slave_phy_read
if no callback is defined?

@Oleksij, why did you decide for 0xffff instead of a negative error code?


>  
> -	ksz_pread16(priv, addr, 0x100 + (reg << 1), &val);
> +	ret = ksz_pread16(priv, addr, 0x100 + (reg << 1), &val);
>  
> -	return val;
> +	return (ret < 0) ? ret : val;
>  }
>  
>  static int ksz9477_phy_write16(struct dsa_switch *ds, int addr, int reg,
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] net: ksz9477: propagate phy read error

[Oleksij Rempel]

Hi,

On Fri, Sep 08, 2023 at 01:59:05PM +0200, Ahmad Fatoum wrote:
> On 08.09.23 12:16, Gerald Loacker wrote:
> > In case of an error we should not return an arbitrary value, propagate the
> > error code instead.
> > Fix return value in case of address error.
> > 
> > Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> > ---
> >  drivers/net/ksz9477.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ksz9477.c b/drivers/net/ksz9477.c
> > index 9665e0f723..d664b7cf01 100644
> > --- a/drivers/net/ksz9477.c
> > +++ b/drivers/net/ksz9477.c
> > @@ -29,14 +29,15 @@ static int ksz9477_phy_read16(struct dsa_switch *ds, int addr, int reg)
> >  {
> >  	struct device *dev = ds->dev;
> >  	struct ksz_switch *priv = dev_get_priv(dev);
> > -	u16 val = 0xffff;
> > +	u16 val;
> > +	int ret;
> >  
> >  	if (addr >= priv->phy_port_cnt)
> > -		return val;
> > +		return -EINVAL;
> 
> Looks sensible IMO, but shouldn't we do the same in dsa_slave_phy_read
> if no callback is defined?
> 
> @Oleksij, why did you decide for 0xffff instead of a negative error code?

This code imitate real MDIO bus access. If you trying to read at not
existing PHY address, you will get 0xffff instead of negative error.
May be it is better to add a comment :)

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/4] i2c: rockchip: ignore i2c transfers when another transfer is running

[Sascha Hauer]

On Fri, Sep 08, 2023 at 01:55:40PM +0200, Sascha Hauer wrote:
> On Fri, Sep 08, 2023 at 01:51:32PM +0200, Sascha Hauer wrote:
> > On Fri, Sep 08, 2023 at 12:16:47PM +0200, Gerald Loacker wrote:
> > > It may happen that an i2c transfer is requested by a callback although
> > > there is an other i2c transfer running. In this case do not interrupt the
> > > transfer and return with an error.
> > > 
> > > Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> > > ---
> > >  drivers/i2c/busses/i2c-rockchip.c | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-rockchip.c b/drivers/i2c/busses/i2c-rockchip.c
> > > index 1bca3e9913..a869b9d0b7 100644
> > > --- a/drivers/i2c/busses/i2c-rockchip.c
> > > +++ b/drivers/i2c/busses/i2c-rockchip.c
> > > @@ -369,7 +369,19 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> > >  {
> > >  	struct rk_i2c *i2c = to_rk_i2c(adapter);
> > >  	struct device *dev = &adapter->dev;
> > > -	int i, ret = 0;
> > > +	struct i2c_regs *regs = i2c->regs;
> > > +	int i, ret = 0, val;
> > > +
> > > +	val = readl(&regs->con);
> > > +	if (val & I2C_CON_EN) {
> > > +		val = readl(&regs->con);
> > > +		if (val & I2C_IPD_ALL_CLEAN) {
> > > +			dev_dbg(dev,
> > > +				"i2c_xfer: %d messages dropped due to pending interrupts\n",
> > > +				nmsgs);
> > > +			return -EAGAIN;
> > > +		}
> > > +	}
> > 
> > Can you have a look how this can happen? Normally this should only
> > happen if you have a heartbeat LED behind a I2C GPIO expander or some
> > other unusual setup. Adding a dump_stack() next to the dev_dbg() call
> > might give a clue.
> 
> I just realized you sent some ksz9477 along with this series. Are you
> using that in I2C mode? In that case it could be the periodic link check
> that does I2C accesses in the background.

Assuming that this is indeed your problem I have just sent a series that
could fix this issue. It's untested yet, please give it a try.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/4] i2c: rockchip: fix i2c stop condition

[Sascha Hauer]

On Fri, Sep 08, 2023 at 12:16:46PM +0200, Gerald Loacker wrote:
> The i2c bus gets disabled without sending a stop condition. This violates
> i2c timing on the clock line. Fix this and include all related functions
> (rk_i2c_send_start_bit, rk_i2c_send_stop_bit, rk_i2c_disable) onto the same
> level.
> 
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> ---
>  drivers/i2c/busses/i2c-rockchip.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> @@ -387,6 +376,11 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  		struct i2c_msg *msg = &msgs[i];
>  
>  		dev_dbg(dev, "i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
> +
> +		ret = rk_i2c_send_start_bit(i2c);
> +		if (ret)
> +			break;
> +
>  		if (msg->flags & I2C_M_RD) {
>  			ret = rk_i2c_read(i2c, msg->addr, 0, 0, msg->buf,
>  					  msg->len);
> @@ -394,6 +388,9 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  			ret = rk_i2c_write(i2c, msg->addr, 0, 0, msg->buf,
>  					   msg->len);
>  		}
> +
> +		rk_i2c_send_stop_bit(i2c);
> +
>  		if (ret) {
>  			dev_dbg(dev, "i2c_write: error sending: %pe\n",
>  				ERR_PTR(ret));
> @@ -402,9 +399,7 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  		}
>  	}
>  
> -	rk_i2c_send_stop_bit(i2c);

I believe this is wrong. A stop bit should only be sent after the last
message, not after all messages.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/4] i2c: rockchip: ignore i2c transfers when another transfer is running

[Gerald Loacker]

Hi Sascha,

Am 08.09.2023 um 15:13 schrieb Sascha Hauer:
> On Fri, Sep 08, 2023 at 01:55:40PM +0200, Sascha Hauer wrote:
>> On Fri, Sep 08, 2023 at 01:51:32PM +0200, Sascha Hauer wrote:
>>> On Fri, Sep 08, 2023 at 12:16:47PM +0200, Gerald Loacker wrote:
>>>> It may happen that an i2c transfer is requested by a callback although
>>>> there is an other i2c transfer running. In this case do not interrupt the
>>>> transfer and return with an error.
>>>>
>>>> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
>>>> ---
>>>>  drivers/i2c/busses/i2c-rockchip.c | 14 +++++++++++++-
>>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-rockchip.c b/drivers/i2c/busses/i2c-rockchip.c
>>>> index 1bca3e9913..a869b9d0b7 100644
>>>> --- a/drivers/i2c/busses/i2c-rockchip.c
>>>> +++ b/drivers/i2c/busses/i2c-rockchip.c
>>>> @@ -369,7 +369,19 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>>>>  {
>>>>  	struct rk_i2c *i2c = to_rk_i2c(adapter);
>>>>  	struct device *dev = &adapter->dev;
>>>> -	int i, ret = 0;
>>>> +	struct i2c_regs *regs = i2c->regs;
>>>> +	int i, ret = 0, val;
>>>> +
>>>> +	val = readl(&regs->con);
>>>> +	if (val & I2C_CON_EN) {
>>>> +		val = readl(&regs->con);
>>>> +		if (val & I2C_IPD_ALL_CLEAN) {
>>>> +			dev_dbg(dev,
>>>> +				"i2c_xfer: %d messages dropped due to pending interrupts\n",
>>>> +				nmsgs);
>>>> +			return -EAGAIN;
>>>> +		}
>>>> +	}
>>>
>>> Can you have a look how this can happen? Normally this should only
>>> happen if you have a heartbeat LED behind a I2C GPIO expander or some
>>> other unusual setup. Adding a dump_stack() next to the dev_dbg() call
>>> might give a clue.
>>
>> I just realized you sent some ksz9477 along with this series. Are you
>> using that in I2C mode? In that case it could be the periodic link check
>> that does I2C accesses in the background.
> 
> Assuming that this is indeed your problem I have just sent a series that
> could fix this issue. It's untested yet, please give it a try.
> 

Yes, this solves the problem of interrupted I2C transfers from the
periodic link check and also makes the ksz9477 patches obsolete. Thanks!

Gerald

> Sascha
> 

Re: [PATCH 1/4] i2c: rockchip: fix i2c stop condition

[Gerald Loacker]

Am 08.09.2023 um 15:53 schrieb Sascha Hauer:
> On Fri, Sep 08, 2023 at 12:16:46PM +0200, Gerald Loacker wrote:
>> The i2c bus gets disabled without sending a stop condition. This violates
>> i2c timing on the clock line. Fix this and include all related functions
>> (rk_i2c_send_start_bit, rk_i2c_send_stop_bit, rk_i2c_disable) onto the same
>> level.
>>
>> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
>> ---
>>  drivers/i2c/busses/i2c-rockchip.c | 21 ++++++++-------------
>>  1 file changed, 8 insertions(+), 13 deletions(-)
>>
>> @@ -387,6 +376,11 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>>  		struct i2c_msg *msg = &msgs[i];
>>  
>>  		dev_dbg(dev, "i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
>> +
>> +		ret = rk_i2c_send_start_bit(i2c);
>> +		if (ret)
>> +			break;
>> +
>>  		if (msg->flags & I2C_M_RD) {
>>  			ret = rk_i2c_read(i2c, msg->addr, 0, 0, msg->buf,
>>  					  msg->len);
>> @@ -394,6 +388,9 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>>  			ret = rk_i2c_write(i2c, msg->addr, 0, 0, msg->buf,
>>  					   msg->len);
>>  		}
>> +
>> +		rk_i2c_send_stop_bit(i2c);
>> +
>>  		if (ret) {
>>  			dev_dbg(dev, "i2c_write: error sending: %pe\n",
>>  				ERR_PTR(ret));
>> @@ -402,9 +399,7 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>>  		}
>>  	}
>>  
>> -	rk_i2c_send_stop_bit(i2c);
> 
> I believe this is wrong. A stop bit should only be sent after the last
> message, not after all messages.
> 

Your're right. This was just because the repeated start does not work
consitently in our case. I'll come up with another approach.

Gerald

> Sascha
> 

Re: [PATCH 1/4] i2c: rockchip: fix i2c stop condition

[Sascha Hauer]

On Tue, Sep 12, 2023 at 07:45:36AM +0200, Gerald Loacker wrote:
> 
> 
> Am 08.09.2023 um 15:53 schrieb Sascha Hauer:
> > On Fri, Sep 08, 2023 at 12:16:46PM +0200, Gerald Loacker wrote:
> >> The i2c bus gets disabled without sending a stop condition. This violates
> >> i2c timing on the clock line. Fix this and include all related functions
> >> (rk_i2c_send_start_bit, rk_i2c_send_stop_bit, rk_i2c_disable) onto the same
> >> level.
> >>
> >> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> >> ---
> >>  drivers/i2c/busses/i2c-rockchip.c | 21 ++++++++-------------
> >>  1 file changed, 8 insertions(+), 13 deletions(-)
> >>
> >> @@ -387,6 +376,11 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> >>  		struct i2c_msg *msg = &msgs[i];
> >>  
> >>  		dev_dbg(dev, "i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
> >> +
> >> +		ret = rk_i2c_send_start_bit(i2c);
> >> +		if (ret)
> >> +			break;
> >> +
> >>  		if (msg->flags & I2C_M_RD) {
> >>  			ret = rk_i2c_read(i2c, msg->addr, 0, 0, msg->buf,
> >>  					  msg->len);
> >> @@ -394,6 +388,9 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> >>  			ret = rk_i2c_write(i2c, msg->addr, 0, 0, msg->buf,
> >>  					   msg->len);
> >>  		}
> >> +
> >> +		rk_i2c_send_stop_bit(i2c);
> >> +
> >>  		if (ret) {
> >>  			dev_dbg(dev, "i2c_write: error sending: %pe\n",
> >>  				ERR_PTR(ret));
> >> @@ -402,9 +399,7 @@ static int rockchip_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
> >>  		}
> >>  	}
> >>  
> >> -	rk_i2c_send_stop_bit(i2c);
> > 
> > I believe this is wrong. A stop bit should only be sent after the last
> > message, not after all messages.
> > 
> 
> Your're right. This was just because the repeated start does not work
> consitently in our case. I'll come up with another approach.

Might be worth to look at the kernel driver. It has a comment about
this:

	/*
	 * The HW is actually not capable of REPEATED START. But we can
	 * get the intended effect by resetting its internal state
	 * and issuing an ordinary START.
	 */
	ctrl = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
	i2c_writel(i2c, ctrl, REG_CON);

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |