| Message ID | 1520852023-27083-7-git-send-email-pierre-yves.mordret@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 12, 2018 at 11:53:43AM +0100, Pierre-Yves MORDRET wrote: > Feature prevents I2C lock-ups. Mechanism resets I2C state machine > and releases SCL/SDA signals but preserves I2C registers. Does it release SDA when held down by the slave? Because that is what the recovery mechanism is for.
Yes. Recovery mechanism is triggered whenever a busy line is detected. This busy state can be checked by sw when either SDA or SCL is low: a bit is set by Hw for this purpose. On busy state I2C is reconfigured and lines are released the time of recovery. Regards On 03/17/2018 09:47 PM, Wolfram Sang wrote: > On Mon, Mar 12, 2018 at 11:53:43AM +0100, Pierre-Yves MORDRET wrote: >> Feature prevents I2C lock-ups. Mechanism resets I2C state machine >> and releases SCL/SDA signals but preserves I2C registers. > > Does it release SDA when held down by the slave? Because that is what > the recovery mechanism is for. >
On 12/03/2018 18:53, Pierre-Yves MORDRET wrote: > Feature prevents I2C lock-ups. Mechanism resets I2C state machine > and releases SCL/SDA signals but preserves I2C registers. > > Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com> > --- > Version history: > v1: > * Initial > --- > --- > drivers/i2c/busses/i2c-stm32f7.c | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c > index 69a2e5e..3808bc9 100644 > --- a/drivers/i2c/busses/i2c-stm32f7.c > +++ b/drivers/i2c/busses/i2c-stm32f7.c > @@ -718,6 +718,20 @@ static void stm32f7_i2c_smbus_reload(struct stm32f7_i2c_dev *i2c_dev) > writel_relaxed(cr2, i2c_dev->base + STM32F7_I2C_CR2); > } > > +static int stm32f7_i2c_recover_bus(struct i2c_adapter *i2c_adap) > +{ > + struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap); > + > + dev_info(i2c_dev->dev, "Trying to recover bus\n"); > + > + stm32f7_i2c_clr_bits(i2c_dev->base + STM32F7_I2C_CR1, > + STM32F7_I2C_CR1_PE); This only "releases" the scl / sda on the stm32f7 end (outputs are hiz I guess). It doesn't trigger the scl clocking needed to recover a stuck device via i2c recovery from what I can see in the data sheet. > + > + stm32f7_i2c_hw_config(i2c_dev); Nothing in here either I think. > + > + return 0; > +} > + > static int stm32f7_i2c_wait_free_bus(struct stm32f7_i2c_dev *i2c_dev) > { > u32 status; > @@ -727,12 +741,18 @@ static int stm32f7_i2c_wait_free_bus(struct stm32f7_i2c_dev *i2c_dev) > status, > !(status & STM32F7_I2C_ISR_BUSY), > 10, 1000); > + if (!ret) > + return 0; > + > + dev_info(i2c_dev->dev, "bus busy\n"); > + > + ret = i2c_recover_bus(&i2c_dev->adap); > if (ret) { > - dev_dbg(i2c_dev->dev, "bus busy\n"); > - ret = -EBUSY; > + dev_err(i2c_dev->dev, "Failed to recover the bus (%d)\n", ret); > + return ret; > } > > - return ret; > + return -EBUSY; > } > > static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev, > @@ -1476,6 +1496,7 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data) > if (status & STM32F7_I2C_ISR_BERR) { > dev_err(dev, "<%s>: Bus error\n", __func__); > writel_relaxed(STM32F7_I2C_ICR_BERRCF, base + STM32F7_I2C_ICR); > + i2c_recover_bus(&i2c_dev->adap); > f7_msg->result = -EIO; > } > > @@ -1760,6 +1781,10 @@ static struct i2c_algorithm stm32f7_i2c_algo = { > .unreg_slave = stm32f7_i2c_unreg_slave, > }; > > +static struct i2c_bus_recovery_info stm32f7_i2c_recovery_info = { > + .recover_bus = stm32f7_i2c_recover_bus, > +}; > + > static int stm32f7_i2c_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > @@ -1879,6 +1904,7 @@ static int stm32f7_i2c_probe(struct platform_device *pdev) > adap->algo = &stm32f7_i2c_algo; > adap->dev.parent = &pdev->dev; > adap->dev.of_node = pdev->dev.of_node; > + adap->bus_recovery_info = &stm32f7_i2c_recovery_info; > > init_completion(&i2c_dev->complete); > >
On 03/19/2018 10:36 AM, Phil Reid wrote: > On 12/03/2018 18:53, Pierre-Yves MORDRET wrote: >> Feature prevents I2C lock-ups. Mechanism resets I2C state machine >> and releases SCL/SDA signals but preserves I2C registers. >> >> Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com> >> --- >> Version history: >> v1: >> * Initial >> --- >> --- >> drivers/i2c/busses/i2c-stm32f7.c | 32 +++++++++++++++++++++++++++++--- >> 1 file changed, 29 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c >> index 69a2e5e..3808bc9 100644 >> --- a/drivers/i2c/busses/i2c-stm32f7.c >> +++ b/drivers/i2c/busses/i2c-stm32f7.c >> @@ -718,6 +718,20 @@ static void stm32f7_i2c_smbus_reload(struct stm32f7_i2c_dev *i2c_dev) >> writel_relaxed(cr2, i2c_dev->base + STM32F7_I2C_CR2); >> } >> >> +static int stm32f7_i2c_recover_bus(struct i2c_adapter *i2c_adap) >> +{ >> + struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap); >> + >> + dev_info(i2c_dev->dev, "Trying to recover bus\n"); >> + >> + stm32f7_i2c_clr_bits(i2c_dev->base + STM32F7_I2C_CR1, >> + STM32F7_I2C_CR1_PE); > > This only "releases" the scl / sda on the stm32f7 end (outputs are hiz I guess). > It doesn't trigger the scl clocking needed to recover a stuck device via i2c recovery > from what I can see in the data sheet. > Correct. This mechanism resets the core IP and not the slave. > >> + >> + stm32f7_i2c_hw_config(i2c_dev); > > Nothing in here either I think. > > >> + >> + return 0; >> +} >> + >> static int stm32f7_i2c_wait_free_bus(struct stm32f7_i2c_dev *i2c_dev) >> { >> u32 status; >> @@ -727,12 +741,18 @@ static int stm32f7_i2c_wait_free_bus(struct stm32f7_i2c_dev *i2c_dev) >> status, >> !(status & STM32F7_I2C_ISR_BUSY), >> 10, 1000); >> + if (!ret) >> + return 0; >> + >> + dev_info(i2c_dev->dev, "bus busy\n"); >> + >> + ret = i2c_recover_bus(&i2c_dev->adap); >> if (ret) { >> - dev_dbg(i2c_dev->dev, "bus busy\n"); >> - ret = -EBUSY; >> + dev_err(i2c_dev->dev, "Failed to recover the bus (%d)\n", ret); >> + return ret; >> } >> >> - return ret; >> + return -EBUSY; >> } >> >> static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev, >> @@ -1476,6 +1496,7 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data) >> if (status & STM32F7_I2C_ISR_BERR) { >> dev_err(dev, "<%s>: Bus error\n", __func__); >> writel_relaxed(STM32F7_I2C_ICR_BERRCF, base + STM32F7_I2C_ICR); >> + i2c_recover_bus(&i2c_dev->adap); >> f7_msg->result = -EIO; >> } >> >> @@ -1760,6 +1781,10 @@ static struct i2c_algorithm stm32f7_i2c_algo = { >> .unreg_slave = stm32f7_i2c_unreg_slave, >> }; >> >> +static struct i2c_bus_recovery_info stm32f7_i2c_recovery_info = { >> + .recover_bus = stm32f7_i2c_recover_bus, >> +}; >> + >> static int stm32f7_i2c_probe(struct platform_device *pdev) >> { >> struct device_node *np = pdev->dev.of_node; >> @@ -1879,6 +1904,7 @@ static int stm32f7_i2c_probe(struct platform_device *pdev) >> adap->algo = &stm32f7_i2c_algo; >> adap->dev.parent = &pdev->dev; >> adap->dev.of_node = pdev->dev.of_node; >> + adap->bus_recovery_info = &stm32f7_i2c_recovery_info; >> >> init_completion(&i2c_dev->complete); >> >> > >
Hi Wolfram, I do believe there is a misunderstanding of this recovery mechanism at my end. I intends to solve a problem where my I2C was stall but not the slave. Thus if bus is busy I reset the IP. Looking at the recovery API, this recovery is for slave and nothing else with my case. Therefore I think I have to get move this reset out of recovery API. Please correct me whether I'm wrong Slave Recovery mechanism is another story to implement in our platform since we have to deal with GPIOs. Let me know Regards On 03/19/2018 09:51 AM, Pierre Yves MORDRET wrote: > Yes. Recovery mechanism is triggered whenever a busy line is detected. > This busy state can be checked by sw when either SDA or SCL is low: a bit is set > by Hw for this purpose. > On busy state I2C is reconfigured and lines are released the time of recovery. > > Regards > On 03/17/2018 09:47 PM, Wolfram Sang wrote: >> On Mon, Mar 12, 2018 at 11:53:43AM +0100, Pierre-Yves MORDRET wrote: >>> Feature prevents I2C lock-ups. Mechanism resets I2C state machine >>> and releases SCL/SDA signals but preserves I2C registers. >> >> Does it release SDA when held down by the slave? Because that is what >> the recovery mechanism is for. >>
Hi, > Looking at the recovery API, this recovery is for slave and nothing else with my > case. Therefore I think I have to get move this reset out of recovery API. Yes, you are correct. You need a custom function, this is totally driver specific. > Slave Recovery mechanism is another story to implement in our platform since we > have to deal with GPIOs. For that, the recovery infrastructure in the core has lots of helpers for you. Regards, Wolfram
Thanks for your quick answer On 03/20/2018 10:42 AM, Wolfram Sang wrote: > Hi, > >> Looking at the recovery API, this recovery is for slave and nothing else with my >> case. Therefore I think I have to get move this reset out of recovery API. > > Yes, you are correct. You need a custom function, this is totally driver > specific. OK ! > >> Slave Recovery mechanism is another story to implement in our platform since we >> have to deal with GPIOs. > > For that, the recovery infrastructure in the core has lots of helpers > for you. > Yeah. This is what I saw nonetheless the main trouble is to setup SCL/SDA into GPIO. Not that easy. but feasible of course. BTW I will rework my driver. Thanks. > Regards, > > Wolfram >
diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c index 69a2e5e..3808bc9 100644 --- a/drivers/i2c/busses/i2c-stm32f7.c +++ b/drivers/i2c/busses/i2c-stm32f7.c @@ -718,6 +718,20 @@ static void stm32f7_i2c_smbus_reload(struct stm32f7_i2c_dev *i2c_dev) writel_relaxed(cr2, i2c_dev->base + STM32F7_I2C_CR2); } +static int stm32f7_i2c_recover_bus(struct i2c_adapter *i2c_adap) +{ + struct stm32f7_i2c_dev *i2c_dev = i2c_get_adapdata(i2c_adap); + + dev_info(i2c_dev->dev, "Trying to recover bus\n"); + + stm32f7_i2c_clr_bits(i2c_dev->base + STM32F7_I2C_CR1, + STM32F7_I2C_CR1_PE); + + stm32f7_i2c_hw_config(i2c_dev); + + return 0; +} + static int stm32f7_i2c_wait_free_bus(struct stm32f7_i2c_dev *i2c_dev) { u32 status; @@ -727,12 +741,18 @@ static int stm32f7_i2c_wait_free_bus(struct stm32f7_i2c_dev *i2c_dev) status, !(status & STM32F7_I2C_ISR_BUSY), 10, 1000); + if (!ret) + return 0; + + dev_info(i2c_dev->dev, "bus busy\n"); + + ret = i2c_recover_bus(&i2c_dev->adap); if (ret) { - dev_dbg(i2c_dev->dev, "bus busy\n"); - ret = -EBUSY; + dev_err(i2c_dev->dev, "Failed to recover the bus (%d)\n", ret); + return ret; } - return ret; + return -EBUSY; } static void stm32f7_i2c_xfer_msg(struct stm32f7_i2c_dev *i2c_dev, @@ -1476,6 +1496,7 @@ static irqreturn_t stm32f7_i2c_isr_error(int irq, void *data) if (status & STM32F7_I2C_ISR_BERR) { dev_err(dev, "<%s>: Bus error\n", __func__); writel_relaxed(STM32F7_I2C_ICR_BERRCF, base + STM32F7_I2C_ICR); + i2c_recover_bus(&i2c_dev->adap); f7_msg->result = -EIO; } @@ -1760,6 +1781,10 @@ static struct i2c_algorithm stm32f7_i2c_algo = { .unreg_slave = stm32f7_i2c_unreg_slave, }; +static struct i2c_bus_recovery_info stm32f7_i2c_recovery_info = { + .recover_bus = stm32f7_i2c_recover_bus, +}; + static int stm32f7_i2c_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -1879,6 +1904,7 @@ static int stm32f7_i2c_probe(struct platform_device *pdev) adap->algo = &stm32f7_i2c_algo; adap->dev.parent = &pdev->dev; adap->dev.of_node = pdev->dev.of_node; + adap->bus_recovery_info = &stm32f7_i2c_recovery_info; init_completion(&i2c_dev->complete);
Feature prevents I2C lock-ups. Mechanism resets I2C state machine and releases SCL/SDA signals but preserves I2C registers. Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mordret@st.com> --- Version history: v1: * Initial --- --- drivers/i2c/busses/i2c-stm32f7.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-)