[PATCH v2] i2c: cadence: Add standard bus recovery support

[PATCH v2] i2c: cadence: Add standard bus recovery support

[Shubhrajyoti Datta]

From: Robert Hancock <robert.hancock@calian.com>

Hook up the standard GPIO/pinctrl-based recovery support for this
driver.

Based on a patch "i2c: cadence: Recover bus after controller reset" by
Chirag Parekh in the Xilinx kernel Git tree, but simplified to make use
of more common code.

Cc: Chirag Parekh <chiragp@xilinx.com>
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
v2:
Rebased.

 drivers/i2c/busses/i2c-cadence.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 805c77143a0f..22ca4ca2a1c1 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -179,6 +179,7 @@ enum cdns_i2c_slave_state {
  * @clk_rate_change_nb:	Notifier block for clock rate changes
  * @quirks:		flag for broken hold bit usage in r1p10
  * @ctrl_reg:		Cached value of the control register.
+ * @rinfo:		Structure holding recovery information.
  * @ctrl_reg_diva_divb: value of fields DIV_A and DIV_B from CR register
  * @slave:		Registered slave instance.
  * @dev_mode:		I2C operating role(master/slave).
@@ -204,6 +205,7 @@ struct cdns_i2c {
 	struct notifier_block clk_rate_change_nb;
 	u32 quirks;
 	u32 ctrl_reg;
+	struct i2c_bus_recovery_info rinfo;
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	u16 ctrl_reg_diva_divb;
 	struct i2c_client *slave;
@@ -788,6 +790,7 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
 	/* Wait for the signal of completion */
 	time_left = wait_for_completion_timeout(&id->xfer_done, adap->timeout);
 	if (time_left == 0) {
+		i2c_recover_bus(adap);
 		cdns_i2c_master_reset(adap);
 		dev_err(id->adap.dev.parent,
 				"timeout waiting on completion\n");
@@ -1270,6 +1273,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 	id->adap.retries = 3;		/* Default retry value. */
 	id->adap.algo_data = id;
 	id->adap.dev.parent = &pdev->dev;
+	id->adap.bus_recovery_info = &id->rinfo;
 	init_completion(&id->xfer_done);
 	snprintf(id->adap.name, sizeof(id->adap.name),
 		 "Cadence I2C at %08lx", (unsigned long)r_mem->start);
-- 
2.17.1

Re: [PATCH v2] i2c: cadence: Add standard bus recovery support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

On Mon, Nov 29, 2021 at 02:31:16PM +0530, Shubhrajyoti Datta wrote:
> From: Robert Hancock <robert.hancock@calian.com>
> 
> Hook up the standard GPIO/pinctrl-based recovery support for this
> driver.
> 
> Based on a patch "i2c: cadence: Recover bus after controller reset" by
> Chirag Parekh in the Xilinx kernel Git tree, but simplified to make use
> of more common code.

Guys, sorry for the long delay.

>  	if (time_left == 0) {
> +		i2c_recover_bus(adap);

According to I2C specs, recovery should be done at the beginning of a
transfer when SDA is detected low. I think this makes sense because
other issues may have stalled the bus as well (e.g. broken bootloader).
Makes sense?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] i2c: cadence: Add standard bus recovery support

[Robert Hancock]

On Mon, 2021-11-29 at 13:00 +0100, Wolfram Sang wrote:
> On Mon, Nov 29, 2021 at 02:31:16PM +0530, Shubhrajyoti Datta wrote:
> > From: Robert Hancock <robert.hancock@calian.com>
> > 
> > Hook up the standard GPIO/pinctrl-based recovery support for this
> > driver.
> > 
> > Based on a patch "i2c: cadence: Recover bus after controller reset" by
> > Chirag Parekh in the Xilinx kernel Git tree, but simplified to make use
> > of more common code.
> 
> Guys, sorry for the long delay.
> 
> >  	if (time_left == 0) {
> > +		i2c_recover_bus(adap);
> 
> According to I2C specs, recovery should be done at the beginning of a
> transfer when SDA is detected low. I think this makes sense because
> other issues may have stalled the bus as well (e.g. broken bootloader).
> Makes sense?

It looks like on the start of a transfer in cdns_i2c_master_xfer, if the
controller reports "bus active" it just bails out with EAGAIN:

        /* Check if the bus is free */
        if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
                ret = -EAGAIN;
                goto out;
        }

I'm not sure exactly what the BA flag indicates (the Xilinx documentation just
says "ongoing transfer on the I2C bus"), so we'd have to distinguish between
the case of another master doing a transfer and the bus actually being hung up.
I'm not sure it's clear from the public documentation how to do this?

That might be another improvement that could be added in once the bus recovery
functionality is in place?

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

RE: [PATCH v2] i2c: cadence: Add standard bus recovery support

[Shubhrajyoti Datta]

> -----Original Message-----
> From: Robert Hancock <robert.hancock@calian.com>
> Sent: Saturday, January 8, 2022 3:55 AM
> To: Shubhrajyoti Datta <shubhraj@xilinx.com>; wsa@kernel.org
> Cc: Michal Simek <michals@xilinx.com>; chiragp@xilinx.com; git
> <git@xilinx.com>; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v2] i2c: cadence: Add standard bus recovery support
> 
> On Mon, 2021-11-29 at 13:00 +0100, Wolfram Sang wrote:
> > On Mon, Nov 29, 2021 at 02:31:16PM +0530, Shubhrajyoti Datta wrote:
> > > From: Robert Hancock <robert.hancock@calian.com>
> > >
> > > Hook up the standard GPIO/pinctrl-based recovery support for this
> > > driver.
> > >
> > > Based on a patch "i2c: cadence: Recover bus after controller reset"
> > > by Chirag Parekh in the Xilinx kernel Git tree, but simplified to
> > > make use of more common code.
> >
> > Guys, sorry for the long delay.
> >
> > >  	if (time_left == 0) {
> > > +		i2c_recover_bus(adap);
> >
> > According to I2C specs, recovery should be done at the beginning of a
> > transfer when SDA is detected low. I think this makes sense because
> > other issues may have stalled the bus as well (e.g. broken bootloader).
> > Makes sense?
> 
> It looks like on the start of a transfer in cdns_i2c_master_xfer, if the
> controller reports "bus active" it just bails out with EAGAIN:
> 
>         /* Check if the bus is free */
>         if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
>                 ret = -EAGAIN;
>                 goto out;
>         }
> 
> I'm not sure exactly what the BA flag indicates (the Xilinx documentation
> just says "ongoing transfer on the I2C bus"), so we'd have to distinguish
> between the case of another master doing a transfer and the bus actually
> being hung up.
> I'm not sure it's clear from the public documentation how to do this?
> 
> That might be another improvement that could be added in once the bus
> recovery functionality is in place?

I think this can be moved to a wait till the bus is not active with a sufficient timeout.
And on timeout  we assume that the bus is stuck.

If you all agree I will send implement and send the next version.
 
> 
> --
> Robert Hancock
> Senior Hardware Designer, Calian Advanced Technologies www.calian.com

RE: [PATCH v2] i2c: cadence: Add standard bus recovery support

[Shubhrajyoti Datta]

> 
> > -----Original Message-----
> > From: Robert Hancock <robert.hancock@calian.com>
> > Sent: Saturday, January 8, 2022 3:55 AM
> > To: Shubhrajyoti Datta <shubhraj@xilinx.com>; wsa@kernel.org
> > Cc: Michal Simek <michals@xilinx.com>; chiragp@xilinx.com; git
> > <git@xilinx.com>; linux-i2c@vger.kernel.org
> > Subject: Re: [PATCH v2] i2c: cadence: Add standard bus recovery
> > support
> >
> > On Mon, 2021-11-29 at 13:00 +0100, Wolfram Sang wrote:
> > > On Mon, Nov 29, 2021 at 02:31:16PM +0530, Shubhrajyoti Datta wrote:
> > > > From: Robert Hancock <robert.hancock@calian.com>
> > > >
> > > > Hook up the standard GPIO/pinctrl-based recovery support for this
> > > > driver.
> > > >
> > > > Based on a patch "i2c: cadence: Recover bus after controller reset"
> > > > by Chirag Parekh in the Xilinx kernel Git tree, but simplified to
> > > > make use of more common code.
> > >
> > > Guys, sorry for the long delay.
> > >
> > > >  	if (time_left == 0) {
> > > > +		i2c_recover_bus(adap);
> > >
> > > According to I2C specs, recovery should be done at the beginning of
> > > a transfer when SDA is detected low. I think this makes sense
> > > because other issues may have stalled the bus as well (e.g. broken
> bootloader).
> > > Makes sense?
> >
> > It looks like on the start of a transfer in cdns_i2c_master_xfer, if
> > the controller reports "bus active" it just bails out with EAGAIN:
> >
> >         /* Check if the bus is free */
> >         if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
> >                 ret = -EAGAIN;
> >                 goto out;
> >         }
> >
> > I'm not sure exactly what the BA flag indicates (the Xilinx
> > documentation just says "ongoing transfer on the I2C bus"), so we'd
> > have to distinguish between the case of another master doing a
> > transfer and the bus actually being hung up.
> > I'm not sure it's clear from the public documentation how to do this?
> >
> > That might be another improvement that could be added in once the bus
> > recovery functionality is in place?
> 
> I think this can be moved to a wait till the bus is not active with a sufficient
> timeout.
> And on timeout  we assume that the bus is stuck.
> 
> If you all agree I will send implement and send the next version.

Please let me know if this approach is fine.
> 
> >
> > --
> > Robert Hancock
> > Senior Hardware Designer, Calian Advanced Technologies
> www.calian.com

[PATCH v2] i2c: cadence: Add standard bus recovery support

[Shubhrajyoti Datta]

From: Robert Hancock <robert.hancock@calian.com>

Hook up the standard GPIO/pinctrl-based recovery support..
In the discurssion
https://patchwork.ozlabs.org/project/linux-i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xilinx.com/

recovery should be done at the beginning of the transaction.
Here we are doing the recovery at the beginning on a timeout.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---
v2:
Updated the busbusy check on a timeout

 drivers/i2c/busses/i2c-cadence.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index b4c1ad19cdae..cf15eca1f9e4 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -10,6 +10,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/of.h>
@@ -125,6 +126,8 @@
 #define CDNS_I2C_DIVB_MAX	64
 
 #define CDNS_I2C_TIMEOUT_MAX	0xFF
+#define CDNS_I2C_POLL_US	100000
+#define CDNS_I2C_TIMEOUT_US	500000
 
 #define CDNS_I2C_BROKEN_HOLD_BIT	BIT(0)
 
@@ -179,6 +182,7 @@ enum cdns_i2c_slave_state {
  * @clk_rate_change_nb:	Notifier block for clock rate changes
  * @quirks:		flag for broken hold bit usage in r1p10
  * @ctrl_reg:		Cached value of the control register.
+ * @rinfo:		Structure holding recovery information.
  * @ctrl_reg_diva_divb: value of fields DIV_A and DIV_B from CR register
  * @slave:		Registered slave instance.
  * @dev_mode:		I2C operating role(master/slave).
@@ -204,6 +208,7 @@ struct cdns_i2c {
 	struct notifier_block clk_rate_change_nb;
 	u32 quirks;
 	u32 ctrl_reg;
+	struct i2c_bus_recovery_info rinfo;
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	u16 ctrl_reg_diva_divb;
 	struct i2c_client *slave;
@@ -796,6 +801,7 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
 	/* Wait for the signal of completion */
 	time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout);
 	if (time_left == 0) {
+		i2c_recover_bus(adap);
 		cdns_i2c_master_reset(adap);
 		dev_err(id->adap.dev.parent,
 				"timeout waiting on completion\n");
@@ -852,8 +858,12 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 #endif
 
 	/* Check if the bus is free */
-	if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
+	ret = readl_poll_timeout(id->membase + CDNS_I2C_SR_OFFSET, reg,
+				 !(reg & CDNS_I2C_SR_BA),
+				 CDNS_I2C_POLL_US, CDNS_I2C_TIMEOUT_US);
+	if (ret) {
 		ret = -EAGAIN;
+		i2c_recover_bus(adap);
 		goto out;
 	}
 
@@ -1278,6 +1288,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 	id->adap.retries = 3;		/* Default retry value. */
 	id->adap.algo_data = id;
 	id->adap.dev.parent = &pdev->dev;
+	id->adap.bus_recovery_info = &id->rinfo;
 	init_completion(&id->xfer_done);
 	snprintf(id->adap.name, sizeof(id->adap.name),
 		 "Cadence I2C at %08lx", (unsigned long)r_mem->start);
-- 
2.17.1

Re: [PATCH v2] i2c: cadence: Add standard bus recovery support

[Michal Simek]

On 7/4/22 07:54, Shubhrajyoti Datta wrote:
> From: Robert Hancock <robert.hancock@calian.com>
> 
> Hook up the standard GPIO/pinctrl-based recovery support..
> In the discurssion

typo

> https://patchwork.ozlabs.org/project/linux-i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xilinx.com/
> 
> recovery should be done at the beginning of the transaction.
> Here we are doing the recovery at the beginning on a timeout.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> v2:
> Updated the busbusy check on a timeout
> 
>   drivers/i2c/busses/i2c-cadence.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index b4c1ad19cdae..cf15eca1f9e4 100644
> --- a/drivers/i2c/busses/i2c-cadence.c
> +++ b/drivers/i2c/busses/i2c-cadence.c
> @@ -10,6 +10,7 @@
>   #include <linux/i2c.h>
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
> +#include <linux/iopoll.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/of.h>
> @@ -125,6 +126,8 @@
>   #define CDNS_I2C_DIVB_MAX	64
>   
>   #define CDNS_I2C_TIMEOUT_MAX	0xFF
> +#define CDNS_I2C_POLL_US	100000
> +#define CDNS_I2C_TIMEOUT_US	500000
>   
>   #define CDNS_I2C_BROKEN_HOLD_BIT	BIT(0)
>   
> @@ -179,6 +182,7 @@ enum cdns_i2c_slave_state {
>    * @clk_rate_change_nb:	Notifier block for clock rate changes
>    * @quirks:		flag for broken hold bit usage in r1p10
>    * @ctrl_reg:		Cached value of the control register.
> + * @rinfo:		Structure holding recovery information.
>    * @ctrl_reg_diva_divb: value of fields DIV_A and DIV_B from CR register
>    * @slave:		Registered slave instance.
>    * @dev_mode:		I2C operating role(master/slave).
> @@ -204,6 +208,7 @@ struct cdns_i2c {
>   	struct notifier_block clk_rate_change_nb;
>   	u32 quirks;
>   	u32 ctrl_reg;
> +	struct i2c_bus_recovery_info rinfo;
>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>   	u16 ctrl_reg_diva_divb;
>   	struct i2c_client *slave;
> @@ -796,6 +801,7 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
>   	/* Wait for the signal of completion */
>   	time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout);
>   	if (time_left == 0) {
> +		i2c_recover_bus(adap);
>   		cdns_i2c_master_reset(adap);
>   		dev_err(id->adap.dev.parent,
>   				"timeout waiting on completion\n");
> @@ -852,8 +858,12 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>   #endif
>   
>   	/* Check if the bus is free */
> -	if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
> +	ret = readl_poll_timeout(id->membase + CDNS_I2C_SR_OFFSET, reg,
> +				 !(reg & CDNS_I2C_SR_BA),
> +				 CDNS_I2C_POLL_US, CDNS_I2C_TIMEOUT_US);
> +	if (ret) {
>   		ret = -EAGAIN;
> +		i2c_recover_bus(adap);
>   		goto out;
>   	}
>   
> @@ -1278,6 +1288,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>   	id->adap.retries = 3;		/* Default retry value. */
>   	id->adap.algo_data = id;
>   	id->adap.dev.parent = &pdev->dev;
> +	id->adap.bus_recovery_info = &id->rinfo;
>   	init_completion(&id->xfer_done);
>   	snprintf(id->adap.name, sizeof(id->adap.name),
>   		 "Cadence I2C at %08lx", (unsigned long)r_mem->start);

I have no problem with it. I expect you have tested it on the real HW.

Acked-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH v2] i2c: cadence: Add standard bus recovery support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

Hi,

On Mon, Jul 04, 2022 at 11:24:20AM +0530, Shubhrajyoti Datta wrote:
> From: Robert Hancock <robert.hancock@calian.com>
> 
> Hook up the standard GPIO/pinctrl-based recovery support..
> In the discurssion
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xilinx.com/
> 
> recovery should be done at the beginning of the transaction.
> Here we are doing the recovery at the beginning on a timeout.

Which is still wrong.

> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

This is an AMD address, but the one you sent from is from Xilinx?

>  	if (time_left == 0) {
> +		i2c_recover_bus(adap);

This is the wrong part.

>  		cdns_i2c_master_reset(adap);
>  		dev_err(id->adap.dev.parent,
>  				"timeout waiting on completion\n");
> @@ -852,8 +858,12 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  #endif
>  
>  	/* Check if the bus is free */
> -	if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA) {
> +	ret = readl_poll_timeout(id->membase + CDNS_I2C_SR_OFFSET, reg,
> +				 !(reg & CDNS_I2C_SR_BA),
> +				 CDNS_I2C_POLL_US, CDNS_I2C_TIMEOUT_US);
> +	if (ret) {
>  		ret = -EAGAIN;
> +		i2c_recover_bus(adap);
>  		goto out;

This is proper.

>  	}
>  
> @@ -1278,6 +1288,7 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>  	id->adap.retries = 3;		/* Default retry value. */
>  	id->adap.algo_data = id;
>  	id->adap.dev.parent = &pdev->dev;
> +	id->adap.bus_recovery_info = &id->rinfo;

Since 'rinfo' is never populated with actual data, I am quite sure this
patch has never been tested.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] i2c: cadence: Add standard bus recovery support

[Robert Hancock]

On Thu, 2022-07-07 at 23:03 +0200, Wolfram Sang wrote:
> Hi,
> 
> On Mon, Jul 04, 2022 at 11:24:20AM +0530, Shubhrajyoti Datta wrote:
> > From: Robert Hancock <robert.hancock@calian.com>
> > 
> > Hook up the standard GPIO/pinctrl-based recovery support..
> > In the discurssion
> > https://patchwork.ozlabs.org/project/linux-i2c/patch/20211129090116.16628-1-shubhrajyoti.datta@xilinx.com/
> > 
> > recovery should be done at the beginning of the transaction.
> > Here we are doing the recovery at the beginning on a timeout.
> 
> Which is still wrong.
> 
> > 
> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> 
> This is an AMD address, but the one you sent from is from Xilinx?
> 
> >         if (time_left == 0) {
> > +               i2c_recover_bus(adap);
> 
> This is the wrong part.
> 
> >                 cdns_i2c_master_reset(adap);
> >                 dev_err(id->adap.dev.parent,
> >                                 "timeout waiting on completion\n");
> > @@ -852,8 +858,12 @@ static int cdns_i2c_master_xfer(struct
> > i2c_adapter *adap, struct i2c_msg *msgs,
> >  #endif
> >  
> >         /* Check if the bus is free */
> > -       if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA)
> > {
> > +       ret = readl_poll_timeout(id->membase + CDNS_I2C_SR_OFFSET,
> > reg,
> > +                                !(reg & CDNS_I2C_SR_BA),
> > +                                CDNS_I2C_POLL_US,
> > CDNS_I2C_TIMEOUT_US);
> > +       if (ret) {
> >                 ret = -EAGAIN;
> > +               i2c_recover_bus(adap);
> >                 goto out;
> 
> This is proper.
> 
> >         }
> >  
> > @@ -1278,6 +1288,7 @@ static int cdns_i2c_probe(struct
> > platform_device *pdev)
> >         id->adap.retries = 3;           /* Default retry value. */
> >         id->adap.algo_data = id;
> >         id->adap.dev.parent = &pdev->dev;
> > +       id->adap.bus_recovery_info = &id->rinfo;
> 
> Since 'rinfo' is never populated with actual data, I am quite sure
> this
> patch has never been tested.

I think this (setting bus_recovery_info to point to a zeroed structure)
is sufficient to allow the generic recovery initialization in i2c-core-
base.c to work. i2c_gpio_init_recovery should fill in the required info
based on the available pinctrl and gpio configuration in this case.

> 
> Regards,
> 
>    Wolfram
> 

RE: [PATCH v2] i2c: cadence: Add standard bus recovery support

[Datta, Shubhrajyoti]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Robert Hancock <robert.hancock@calian.com>
> Sent: Friday, July 8, 2022 3:45 AM
> To: shubhrajyoti.datta@xilinx.com; wsa@kernel.org
> Cc: michal.simek@xilinx.com; linux-i2c@vger.kernel.org; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>
> Subject: Re: [PATCH v2] i2c: cadence: Add standard bus recovery support
> 
> [CAUTION: External Email]
> 
> On Thu, 2022-07-07 at 23:03 +0200, Wolfram Sang wrote:
> > Hi,
> >
> > On Mon, Jul 04, 2022 at 11:24:20AM +0530, Shubhrajyoti Datta wrote:
> > > From: Robert Hancock <robert.hancock@calian.com>
> > >
> > > Hook up the standard GPIO/pinctrl-based recovery support..
> > > In the discurssion
> > >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > tchwork.ozlabs.org%2Fproject%2Flinux-
> i2c%2Fpatch%2F20211129090116.16
> > > 628-1-
> shubhrajyoti.datta%40xilinx.com%2F&amp;data=05%7C01%7Cshubhraj
> > >
> yoti.datta%40amd.com%7C4e1a91e059a8495756a208da60661551%7C3dd89
> 61fe4
> > >
> 884e608e11a82d994e183d%7C0%7C0%7C637928288864536085%7CUnknow
> n%7CTWFp
> > >
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6
> > >
> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=xhjvSNSf1%2FO5ihd%2B92O%2B
> LCOci265Q
> > > R8HiNh%2B8TBWXw8%3D&amp;reserved=0
> > >
> > > recovery should be done at the beginning of the transaction.
> > > Here we are doing the recovery at the beginning on a timeout.
> >
> > Which is still wrong.
Will fix this.

> >
> > >
> > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> >
> > This is an AMD address, but the one you sent from is from Xilinx?
> >
> > >         if (time_left == 0) {
> > > +               i2c_recover_bus(adap);
> >
> > This is the wrong part.
Will fix

> >
> > >                 cdns_i2c_master_reset(adap);
> > >                 dev_err(id->adap.dev.parent,
> > >                                 "timeout waiting on completion\n");
> > > @@ -852,8 +858,12 @@ static int cdns_i2c_master_xfer(struct
> > > i2c_adapter *adap, struct i2c_msg *msgs,  #endif
> > >
> > >         /* Check if the bus is free */
> > > -       if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_BA)
> > > {
> > > +       ret = readl_poll_timeout(id->membase + CDNS_I2C_SR_OFFSET,
> > > reg,
> > > +                                !(reg & CDNS_I2C_SR_BA),
> > > +                                CDNS_I2C_POLL_US,
> > > CDNS_I2C_TIMEOUT_US);
> > > +       if (ret) {
> > >                 ret = -EAGAIN;
> > > +               i2c_recover_bus(adap);
> > >                 goto out;
> >
> > This is proper.
> >
> > >         }
> > >
> > > @@ -1278,6 +1288,7 @@ static int cdns_i2c_probe(struct
> > > platform_device *pdev)
> > >         id->adap.retries = 3;           /* Default retry value. */
> > >         id->adap.algo_data = id;
> > >         id->adap.dev.parent = &pdev->dev;
> > > +       id->adap.bus_recovery_info = &id->rinfo;
> >
> > Since 'rinfo' is never populated with actual data, I am quite sure
> > this patch has never been tested.
> 
> I think this (setting bus_recovery_info to point to a zeroed structure) is
> sufficient to allow the generic recovery initialization in i2c-core- base.c to
> work. i2c_gpio_init_recovery should fill in the required info based on the
> available pinctrl and gpio configuration in this case.

I  checked the handler calls however I will try to check with a setup where I can probe and
Verify the clock cycles.
> 
> >
> > Regards,
> >
> >    Wolfram
> >