| Message ID | 20171130143007.30258-3-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi, On Thu, Nov 30, 2017 at 03:30:06PM +0100, Andrzej Hajda wrote: > In some circumstances I2C clients can be in abnormal state, to allow > recover them from it I2C master can pulse SCL line until SDA line > goes up and then generate STOP condition. Exynos driver does not have > possibility to manipulate I2C lines directly, but it can provide similar > functionality using manual commands. Replacing I2C adapter reset with Really? READ_DATA looks like 8 clock pulses, but you need 9. > bus recovery significantly incereases I2C bus robustness in case of timeouts. > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > @@ -642,7 +666,7 @@ static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c, > ret = exynos5_i2c_wait_bus_idle(i2c); > > if (ret < 0) { > - exynos5_i2c_reset(i2c); > + exynos5_i2c_recovery(i2c); Bus recovery is only defined for stalled busses, i.e. when SDA is stuck low. It is not a suitable method to deal with general timeouts. > +int exynos5_i2c_recover_bus(struct i2c_adapter *adap) > +{ > + struct exynos5_i2c *i2c = adap->algo_data; > + > + i2c_lock_adapter(adap); > + clk_enable(i2c->clk); > + exynos5_i2c_recovery(i2c); > + clk_disable(i2c->clk); > + i2c_unlock_adapter(adap); Why do you need to lock? Aren't you protected by the call to master_xfer which then detected a problem? Regards, Wolfram
On 15.01.2018 21:52, Wolfram Sang wrote: > Hi, > > On Thu, Nov 30, 2017 at 03:30:06PM +0100, Andrzej Hajda wrote: >> In some circumstances I2C clients can be in abnormal state, to allow >> recover them from it I2C master can pulse SCL line until SDA line >> goes up and then generate STOP condition. Exynos driver does not have >> possibility to manipulate I2C lines directly, but it can provide similar >> functionality using manual commands. Replacing I2C adapter reset with > Really? READ_DATA looks like 8 clock pulses, but you need 9. This is why I have used word 'similar' :) Unfortunately chip does not provide method to send exactly 9 pulses. But according to [1] it is the worst case scenario, usually less than 9 pulses is enough. Another solution I see is to READ_DATA twice, this way we will have 16 pulses. [1]: http://www.analog.com/media/en/technical-documentation/application-notes/54305147357414AN686_0.pdf > >> bus recovery significantly incereases I2C bus robustness in case of timeouts. >> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > > >> @@ -642,7 +666,7 @@ static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c, >> ret = exynos5_i2c_wait_bus_idle(i2c); >> >> if (ret < 0) { >> - exynos5_i2c_reset(i2c); >> + exynos5_i2c_recovery(i2c); > Bus recovery is only defined for stalled busses, i.e. when SDA is stuck > low. It is not a suitable method to deal with general timeouts. You are right, I though I do not have precise info that SDA is stuck low, but I guess status HSI2C_MASTER_ST_LOSE indicates is, I will check it. > > >> +int exynos5_i2c_recover_bus(struct i2c_adapter *adap) >> +{ >> + struct exynos5_i2c *i2c = adap->algo_data; >> + >> + i2c_lock_adapter(adap); >> + clk_enable(i2c->clk); >> + exynos5_i2c_recovery(i2c); >> + clk_disable(i2c->clk); >> + i2c_unlock_adapter(adap); > Why do you need to lock? Aren't you protected by the call to master_xfer > which then detected a problem? If the problem is detected in master_xfer driver uses internal function exynos5_i2c_recovery, but function exynos5_i2c_recover_bus is called by i2c_recover_bus, and the latter can be called from any context, this is why I have added locking. Regards Andrzej > > Regards, > > Wolfram > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Really? READ_DATA looks like 8 clock pulses, but you need 9. > > This is why I have used word 'similar' :) Unfortunately chip does not > provide method to send exactly 9 pulses. Pity :( > But according to [1] it is the worst case scenario, usually less than 9 > pulses is enough. The I2C specification says "should send nine clock pulses". A recovery mechanism which "usually works" doesn't sound very attractive. > Another solution I see is to READ_DATA twice, this way we will have 16 > pulses. Can't you clock a single pulse when requesting an ACK? Or does READ_DATE clock an ACK so we might end up with 9 pulses after all? > > Bus recovery is only defined for stalled busses, i.e. when SDA is stuck > > low. It is not a suitable method to deal with general timeouts. > > You are right, I though I do not have precise info that SDA is stuck > low, but I guess status HSI2C_MASTER_ST_LOSE indicates is, I will check it. Thank you! > >> + i2c_lock_adapter(adap); > >> + clk_enable(i2c->clk); > >> + exynos5_i2c_recovery(i2c); > >> + clk_disable(i2c->clk); > >> + i2c_unlock_adapter(adap); > > Why do you need to lock? Aren't you protected by the call to master_xfer > > which then detected a problem? > > If the problem is detected in master_xfer driver uses internal function > exynos5_i2c_recovery, > but function exynos5_i2c_recover_bus is called by i2c_recover_bus, and > the latter can be called from any context, > this is why I have added locking. But the wrong one? The adapter lock is for serializing access to the adapter using the I2C API. If you need to protect register access within your driver, then you need the lock within struct exynos5_i2c.
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c index b02428498f6d..4ca43980e2ed 100644 --- a/drivers/i2c/busses/i2c-exynos5.c +++ b/drivers/i2c/busses/i2c-exynos5.c @@ -128,6 +128,10 @@ #define HSI2C_TIMEOUT_EN (1u << 31) #define HSI2C_TIMEOUT_MASK 0xff +/* I2C_MANUAL_CMD register bits */ +#define HSI2C_CMD_READ_DATA (1u << 4) +#define HSI2C_CMD_SEND_STOP (1u << 2) + /* I2C_TRANS_STATUS register bits */ #define HSI2C_MASTER_BUSY (1u << 17) #define HSI2C_SLAVE_BUSY (1u << 16) @@ -613,6 +617,26 @@ static void exynos5_i2c_message_start(struct exynos5_i2c *i2c, int stop) spin_unlock_irqrestore(&i2c->lock, flags); } +void exynos5_i2c_recovery(struct exynos5_i2c *i2c) +{ + u32 val; + + val = readl(i2c->regs + HSI2C_CTL) | HSI2C_RXCHON; + writel(val, i2c->regs + HSI2C_CTL); + val = readl(i2c->regs + HSI2C_CONF) & ~HSI2C_AUTO_MODE; + writel(val, i2c->regs + HSI2C_CONF); + + writel(HSI2C_CMD_READ_DATA, i2c->regs + HSI2C_MANUAL_CMD); + exynos5_i2c_wait_bus_idle(i2c); + writel(HSI2C_CMD_SEND_STOP, i2c->regs + HSI2C_MANUAL_CMD); + exynos5_i2c_wait_bus_idle(i2c); + + val = readl(i2c->regs + HSI2C_CTL) & ~HSI2C_RXCHON; + writel(val, i2c->regs + HSI2C_CTL); + val = readl(i2c->regs + HSI2C_CONF) | HSI2C_AUTO_MODE; + writel(val, i2c->regs + HSI2C_CONF); +} + static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c, struct i2c_msg *msgs, int stop) { @@ -642,7 +666,7 @@ static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c, ret = exynos5_i2c_wait_bus_idle(i2c); if (ret < 0) { - exynos5_i2c_reset(i2c); + exynos5_i2c_recovery(i2c); if (ret == -ETIMEDOUT) dev_warn(i2c->dev, "%s timeout\n", (msgs->flags & I2C_M_RD) ? "rx" : "tx"); @@ -703,6 +727,23 @@ static const struct i2c_algorithm exynos5_i2c_algorithm = { .functionality = exynos5_i2c_func, }; +int exynos5_i2c_recover_bus(struct i2c_adapter *adap) +{ + struct exynos5_i2c *i2c = adap->algo_data; + + i2c_lock_adapter(adap); + clk_enable(i2c->clk); + exynos5_i2c_recovery(i2c); + clk_disable(i2c->clk); + i2c_unlock_adapter(adap); + + return 0; +} + +static struct i2c_bus_recovery_info exynos5_i2c_recovery_info = { + .recover_bus = exynos5_i2c_recover_bus +}; + static int exynos5_i2c_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -773,6 +814,7 @@ static int exynos5_i2c_probe(struct platform_device *pdev) goto err_clk; exynos5_i2c_reset(i2c); + i2c->adap.bus_recovery_info = &exynos5_i2c_recovery_info; ret = i2c_add_adapter(&i2c->adap); if (ret < 0)
In some circumstances I2C clients can be in abnormal state, to allow recover them from it I2C master can pulse SCL line until SDA line goes up and then generate STOP condition. Exynos driver does not have possibility to manipulate I2C lines directly, but it can provide similar functionality using manual commands. Replacing I2C adapter reset with bus recovery significantly incereases I2C bus robustness in case of timeouts. Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> --- drivers/i2c/busses/i2c-exynos5.c | 44 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-)