i2c: bcm-iproc: Add i2c recovery support

Message ID 20210530225659.17138-1-chris.packham@alliedtelesis.co.nz
State New
Headers show
Series
  • i2c: bcm-iproc: Add i2c recovery support
Related show

Commit Message

Chris Packham May 30, 2021, 10:56 p.m.
From: Richard Laing <richard.laing@alliedtelesis.co.nz>

The bcm-iproc controller can put the SDA/SCL lines into bit-bang mode,
make use of this to support i2c bus recovery.

Signed-off-by: Richard Laing <richard.laing@alliedtelesis.co.nz>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Richard did most of the work on this. I'm just cleaning it up to get it
    upstream.

 drivers/i2c/busses/i2c-bcm-iproc.c | 115 +++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

Comments

Ray Jui June 1, 2021, 9:03 p.m. | #1
On 5/30/2021 3:56 PM, Chris Packham wrote:
> From: Richard Laing <richard.laing@alliedtelesis.co.nz>

> 

> The bcm-iproc controller can put the SDA/SCL lines into bit-bang mode,

> make use of this to support i2c bus recovery.

> 

> Signed-off-by: Richard Laing <richard.laing@alliedtelesis.co.nz>

> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

> ---

> 

> Notes:

>     Richard did most of the work on this. I'm just cleaning it up to get it

>     upstream.

> 

>  drivers/i2c/busses/i2c-bcm-iproc.c | 115 +++++++++++++++++++++++++++++

>  1 file changed, 115 insertions(+)

> 

> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c

> index cceaf69279a9..d63a286c1660 100644

> --- a/drivers/i2c/busses/i2c-bcm-iproc.c

> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c

> @@ -26,6 +26,7 @@

>  #define CFG_RESET_SHIFT              31

>  #define CFG_EN_SHIFT                 30

>  #define CFG_SLAVE_ADDR_0_SHIFT       28

> +#define CFG_BIT_BANG_SHIFT           29


move this up one line (to be consistent with existing bit order)

>  #define CFG_M_RETRY_CNT_SHIFT        16

>  #define CFG_M_RETRY_CNT_MASK         0x0f

>  

> @@ -66,6 +67,12 @@

>  #define S_FIFO_RX_THLD_SHIFT         8

>  #define S_FIFO_RX_THLD_MASK          0x3f

>  

> +#define M_BB_CTRL_OFFSET             0x14

> +#define M_BB_SMBCLK_IN               31


M_BB_CTRL_CLK_IN_SHIFT, ket is to have '_SHIFT' to be consistent with
existing code

> +#define M_BB_SMBCLK_OUT_EN           30


M_BB_CTRL_CLK_OUT_SHIFT

> +#define M_BB_SMBDAT_IN               29


M_BB_CTRL_DATA_IN_SHIFT

> +#define M_BB_SMBDAT_OUT_EN           28


M_BB_CTRL_DATA_OUT_SHIFT

> +

>  #define M_CMD_OFFSET                 0x30

>  #define M_CMD_START_BUSY_SHIFT       31

>  #define M_CMD_STATUS_SHIFT           25

> @@ -713,6 +720,112 @@ static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,

>  	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);

>  }

>  

> +static void bcm_iproc_i2c_reset(struct bcm_iproc_i2c_dev *iproc_i2c)

> +{

> +	u32 tmp;

> +

> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);

> +	tmp |= BIT(CFG_RESET_SHIFT);

> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);

> +	udelay(100);


This puts the controller in reset and hold it there, but never brings
the controller out of reset (bcm_iproc_i2c_init called in unprepare
brings the controller out of reset)

Calling it a "reset" function is a bit misleading to me. My expectation
of a reset function is that you generate a reset pulse, ie.g., reset ->
delay -> out of reset.

Why don't you simply put this seuquence of code in the prepare_recovery
function below, instead of calling this a reset function?

> +

> +}

> +

> +static void bcm_iproc_i2c_prepare_recovery(struct i2c_adapter *adap)

> +{

> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

> +	u32 tmp;

> +

> +	dev_dbg(iproc_i2c->device, "Prepare recovery\n");

> +

> +	/* Disable interrupts */

> +	writel(0, iproc_i2c->base + IE_OFFSET);

> +	readl(iproc_i2c->base + IE_OFFSET);

> +	synchronize_irq(iproc_i2c->irq);

> +

> +	bcm_iproc_i2c_reset(iproc_i2c);

> +

> +	/* Switch to bit-bang mode */

> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);

> +	tmp |= BIT(CFG_BIT_BANG_SHIFT);

> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);


add usleep_range(100, 200) here, required delay after switching to bit
bang based on spec.

> +}

> +

> +static void bcm_iproc_i2c_unprepare_recovery(struct i2c_adapter *adap)

> +{

> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

> +	u32 tmp;

> +

> +	/* Switch to normal mode */

> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);

> +	tmp &= ~BIT(CFG_BIT_BANG_SHIFT);

> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);

> +	udelay(100);

> +

> +	bcm_iproc_i2c_init(iproc_i2c);


Add sequence to re-configure to desired bus speed here after the reset
sequence (someone else in our team tested this is required to resume to
proper bus speed).

> +	bcm_iproc_i2c_enable_disable(iproc_i2c, true);

> +

> +	dev_dbg(iproc_i2c->device, "Recovery complete\n");

> +}

> +

> +static int bcm_iproc_i2c_get_scl(struct i2c_adapter *adap)

> +{

> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

> +	u32 tmp;

> +

> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

> +

> +	return !!(tmp & BIT(M_BB_SMBCLK_IN));

> +}

> +

> +static void bcm_iproc_i2c_set_scl(struct i2c_adapter *adap, int val)

> +{

> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

> +	u32 tmp;

> +

> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

> +	if (val)

> +		tmp |= BIT(M_BB_SMBCLK_OUT_EN);

> +	else

> +		tmp &= ~BIT(M_BB_SMBCLK_OUT_EN);

> +

> +	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);

> +}

> +

> +static void bcm_iproc_i2c_set_sda(struct i2c_adapter *adap, int val)

> +{

> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

> +	u32 tmp;

> +

> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

> +	if (val)

> +		tmp |= BIT(M_BB_SMBDAT_OUT_EN);

> +	else

> +		tmp &= ~BIT(M_BB_SMBDAT_OUT_EN);

> +

> +	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);

> +}

> +

> +static int bcm_iproc_i2c_get_sda(struct i2c_adapter *adap)

> +{

> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

> +	u32 tmp;

> +

> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

> +

> +	return !!(tmp & BIT(M_BB_SMBDAT_IN));

> +}

> +

> +static struct i2c_bus_recovery_info bcm_iproc_recovery_info = {


static const struct ...

> +	.recover_bus = i2c_generic_scl_recovery,

> +	.prepare_recovery = bcm_iproc_i2c_prepare_recovery,

> +	.unprepare_recovery = bcm_iproc_i2c_unprepare_recovery,

> +	.set_scl = bcm_iproc_i2c_set_scl,

> +	.get_scl = bcm_iproc_i2c_get_scl,

> +	.set_sda = bcm_iproc_i2c_set_sda,

> +	.get_sda = bcm_iproc_i2c_get_sda,

> +};

> +

>  static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,

>  				      struct i2c_msg *msg)

>  {

> @@ -839,6 +952,7 @@ static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,

>  	if (!!(iproc_i2c_rd_reg(iproc_i2c,

>  				M_CMD_OFFSET) & BIT(M_CMD_START_BUSY_SHIFT))) {

>  		dev_warn(iproc_i2c->device, "bus is busy\n");

> +		i2c_recover_bus(&iproc_i2c->adapter);


'i2c_recover_bus' should not be ALWAYS called here. You don't know if
bus is actually locked up or it's other issues that caused this.

We need a logic to detect and confirm the lock up condition before
committing to recover operation:

/* Check if bus lockup occurred, and invoke recovery if so. */
static void iproc_i2c_lockup_recover(struct bcm_iproc_i2c_dev *iproc_i2c)
{
	/*
	 * assume bus lockup if SDA line is low;
	 * note that there is no need to switch to
	 * bit-bang mode for this check.
	 */
	if (!bcm_iproc_i2c_get_sda(&iproc_i2c->adapter)) {
		/* locked up - invoke i2c bus recovery. */
		int ret = i2c_recover_bus(&iproc_i2c->adapter);
			if (ret)
				dev_err(iproc_i2c->device,
					"bus recovery: error %d\n",
					ret);
	}
}

'iproc_i2c_lockup_recover' should be called in two locations in the driver:

1. After 'transaction timed out' (and after flush both TX/RX FIFOS)
2. After 'bcm_iproc_i2c_check_status' failures (and after flush both
TX/RX FIFOs).

>  		return -EBUSY;

>  	}

>  

> @@ -1111,6 +1225,7 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)

>  		of_node_full_name(iproc_i2c->device->of_node));

>  	adap->algo = &bcm_iproc_algo;

>  	adap->quirks = &bcm_iproc_i2c_quirks;

> +	adap->bus_recovery_info = &bcm_iproc_recovery_info;

>  	adap->dev.parent = &pdev->dev;

>  	adap->dev.of_node = pdev->dev.of_node;

>  

> 


Thanks,

Ray
Chris Packham June 1, 2021, 9:44 p.m. | #2
On 2/06/21 9:03 am, Ray Jui wrote:
>

> On 5/30/2021 3:56 PM, Chris Packham wrote:

>> From: Richard Laing <richard.laing@alliedtelesis.co.nz>

>>

>> The bcm-iproc controller can put the SDA/SCL lines into bit-bang mode,

>> make use of this to support i2c bus recovery.

>>

>> Signed-off-by: Richard Laing <richard.laing@alliedtelesis.co.nz>

>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

>> ---

>>

>> Notes:

>>      Richard did most of the work on this. I'm just cleaning it up to get it

>>      upstream.

>>

>>   drivers/i2c/busses/i2c-bcm-iproc.c | 115 +++++++++++++++++++++++++++++

>>   1 file changed, 115 insertions(+)

>>

>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c

>> index cceaf69279a9..d63a286c1660 100644

>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c

>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c

>> @@ -26,6 +26,7 @@

>>   #define CFG_RESET_SHIFT              31

>>   #define CFG_EN_SHIFT                 30

>>   #define CFG_SLAVE_ADDR_0_SHIFT       28

>> +#define CFG_BIT_BANG_SHIFT           29

> move this up one line (to be consistent with existing bit order)


Ack.

>>   #define CFG_M_RETRY_CNT_SHIFT        16

>>   #define CFG_M_RETRY_CNT_MASK         0x0f

>>   

>> @@ -66,6 +67,12 @@

>>   #define S_FIFO_RX_THLD_SHIFT         8

>>   #define S_FIFO_RX_THLD_MASK          0x3f

>>   

>> +#define M_BB_CTRL_OFFSET             0x14

>> +#define M_BB_SMBCLK_IN               31

> M_BB_CTRL_CLK_IN_SHIFT, ket is to have '_SHIFT' to be consistent with

> existing code


Ack.

>> +#define M_BB_SMBCLK_OUT_EN           30

> M_BB_CTRL_CLK_OUT_SHIFT

>

>> +#define M_BB_SMBDAT_IN               29

> M_BB_CTRL_DATA_IN_SHIFT

>

>> +#define M_BB_SMBDAT_OUT_EN           28

> M_BB_CTRL_DATA_OUT_SHIFT

>

>> +

>>   #define M_CMD_OFFSET                 0x30

>>   #define M_CMD_START_BUSY_SHIFT       31

>>   #define M_CMD_STATUS_SHIFT           25

>> @@ -713,6 +720,112 @@ static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,

>>   	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);

>>   }

>>   

>> +static void bcm_iproc_i2c_reset(struct bcm_iproc_i2c_dev *iproc_i2c)

>> +{

>> +	u32 tmp;

>> +

>> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);

>> +	tmp |= BIT(CFG_RESET_SHIFT);

>> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);

>> +	udelay(100);

> This puts the controller in reset and hold it there, but never brings

> the controller out of reset (bcm_iproc_i2c_init called in unprepare

> brings the controller out of reset)

>

> Calling it a "reset" function is a bit misleading to me. My expectation

> of a reset function is that you generate a reset pulse, ie.g., reset ->

> delay -> out of reset.

>

> Why don't you simply put this seuquence of code in the prepare_recovery

> function below, instead of calling this a reset function?


Will do. Should I also change the udelay(100) to a usleep_range(100, 
200)? Same question for the other uses of udelay().

>

>> +

>> +}

>> +

>> +static void bcm_iproc_i2c_prepare_recovery(struct i2c_adapter *adap)

>> +{

>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>> +	u32 tmp;

>> +

>> +	dev_dbg(iproc_i2c->device, "Prepare recovery\n");

>> +

>> +	/* Disable interrupts */

>> +	writel(0, iproc_i2c->base + IE_OFFSET);

>> +	readl(iproc_i2c->base + IE_OFFSET);

>> +	synchronize_irq(iproc_i2c->irq);

>> +

>> +	bcm_iproc_i2c_reset(iproc_i2c);

>> +

>> +	/* Switch to bit-bang mode */

>> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);

>> +	tmp |= BIT(CFG_BIT_BANG_SHIFT);

>> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);

> add usleep_range(100, 200) here, required delay after switching to bit

> bang based on spec.


Ack.

>

>> +}

>> +

>> +static void bcm_iproc_i2c_unprepare_recovery(struct i2c_adapter *adap)

>> +{

>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>> +	u32 tmp;

>> +

>> +	/* Switch to normal mode */

>> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);

>> +	tmp &= ~BIT(CFG_BIT_BANG_SHIFT);

>> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);

>> +	udelay(100);

>> +

>> +	bcm_iproc_i2c_init(iproc_i2c);

> Add sequence to re-configure to desired bus speed here after the reset

> sequence (someone else in our team tested this is required to resume to

> proper bus speed).

bcm_iproc_i2c_resume() seems to have the exact sequence I need. If I 
move it outside of CONFIG_PM_SLEEP I could just call it here.
>> +	bcm_iproc_i2c_enable_disable(iproc_i2c, true);

>> +

>> +	dev_dbg(iproc_i2c->device, "Recovery complete\n");

>> +}

>> +

>> +static int bcm_iproc_i2c_get_scl(struct i2c_adapter *adap)

>> +{

>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>> +	u32 tmp;

>> +

>> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

>> +

>> +	return !!(tmp & BIT(M_BB_SMBCLK_IN));

>> +}

>> +

>> +static void bcm_iproc_i2c_set_scl(struct i2c_adapter *adap, int val)

>> +{

>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>> +	u32 tmp;

>> +

>> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

>> +	if (val)

>> +		tmp |= BIT(M_BB_SMBCLK_OUT_EN);

>> +	else

>> +		tmp &= ~BIT(M_BB_SMBCLK_OUT_EN);

>> +

>> +	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);

>> +}

>> +

>> +static void bcm_iproc_i2c_set_sda(struct i2c_adapter *adap, int val)

>> +{

>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>> +	u32 tmp;

>> +

>> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

>> +	if (val)

>> +		tmp |= BIT(M_BB_SMBDAT_OUT_EN);

>> +	else

>> +		tmp &= ~BIT(M_BB_SMBDAT_OUT_EN);

>> +

>> +	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);

>> +}

>> +

>> +static int bcm_iproc_i2c_get_sda(struct i2c_adapter *adap)

>> +{

>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>> +	u32 tmp;

>> +

>> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

>> +

>> +	return !!(tmp & BIT(M_BB_SMBDAT_IN));

>> +}

>> +

>> +static struct i2c_bus_recovery_info bcm_iproc_recovery_info = {

> static const struct ...

Ack.
>> +	.recover_bus = i2c_generic_scl_recovery,

>> +	.prepare_recovery = bcm_iproc_i2c_prepare_recovery,

>> +	.unprepare_recovery = bcm_iproc_i2c_unprepare_recovery,

>> +	.set_scl = bcm_iproc_i2c_set_scl,

>> +	.get_scl = bcm_iproc_i2c_get_scl,

>> +	.set_sda = bcm_iproc_i2c_set_sda,

>> +	.get_sda = bcm_iproc_i2c_get_sda,

>> +};

>> +

>>   static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,

>>   				      struct i2c_msg *msg)

>>   {

>> @@ -839,6 +952,7 @@ static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,

>>   	if (!!(iproc_i2c_rd_reg(iproc_i2c,

>>   				M_CMD_OFFSET) & BIT(M_CMD_START_BUSY_SHIFT))) {

>>   		dev_warn(iproc_i2c->device, "bus is busy\n");

>> +		i2c_recover_bus(&iproc_i2c->adapter);

> 'i2c_recover_bus' should not be ALWAYS called here. You don't know if

> bus is actually locked up or it's other issues that caused this.

>

> We need a logic to detect and confirm the lock up condition before

> committing to recover operation:

>

> /* Check if bus lockup occurred, and invoke recovery if so. */

> static void iproc_i2c_lockup_recover(struct bcm_iproc_i2c_dev *iproc_i2c)

> {

> 	/*

> 	 * assume bus lockup if SDA line is low;

> 	 * note that there is no need to switch to

> 	 * bit-bang mode for this check.

> 	 */

> 	if (!bcm_iproc_i2c_get_sda(&iproc_i2c->adapter)) {

> 		/* locked up - invoke i2c bus recovery. */

> 		int ret = i2c_recover_bus(&iproc_i2c->adapter);

> 			if (ret)

> 				dev_err(iproc_i2c->device,

> 					"bus recovery: error %d\n",

> 					ret);

> 	}

> }

>

> 'iproc_i2c_lockup_recover' should be called in two locations in the driver:

>

> 1. After 'transaction timed out' (and after flush both TX/RX FIFOS)

> 2. After 'bcm_iproc_i2c_check_status' failures (and after flush both

> TX/RX FIFOs).


I see bcm_iproc_i2c_check_status() has a "recovery" case on an unknown 
error. Should I remove that if I'm going to call 
iproc_i2c_lockup_recover() or check for ret != -EIO?

>

>>   		return -EBUSY;

>>   	}

>>   

>> @@ -1111,6 +1225,7 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)

>>   		of_node_full_name(iproc_i2c->device->of_node));

>>   	adap->algo = &bcm_iproc_algo;

>>   	adap->quirks = &bcm_iproc_i2c_quirks;

>> +	adap->bus_recovery_info = &bcm_iproc_recovery_info;

>>   	adap->dev.parent = &pdev->dev;

>>   	adap->dev.of_node = pdev->dev.of_node;

>>   

>>

> Thanks,

>

> Ray
Ray Jui June 1, 2021, 9:51 p.m. | #3
On 6/1/2021 2:44 PM, Chris Packham wrote:
> 

> On 2/06/21 9:03 am, Ray Jui wrote:

>>

>> On 5/30/2021 3:56 PM, Chris Packham wrote:

>>> From: Richard Laing <richard.laing@alliedtelesis.co.nz>

>>>

>>> The bcm-iproc controller can put the SDA/SCL lines into bit-bang mode,

>>> make use of this to support i2c bus recovery.

>>>

>>> Signed-off-by: Richard Laing <richard.laing@alliedtelesis.co.nz>

>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

>>> ---

>>>

>>> Notes:

>>>      Richard did most of the work on this. I'm just cleaning it up to get it

>>>      upstream.

>>>

>>>   drivers/i2c/busses/i2c-bcm-iproc.c | 115 +++++++++++++++++++++++++++++

>>>   1 file changed, 115 insertions(+)

>>>

>>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c

>>> index cceaf69279a9..d63a286c1660 100644

>>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c

>>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c

>>> @@ -26,6 +26,7 @@

>>>   #define CFG_RESET_SHIFT              31

>>>   #define CFG_EN_SHIFT                 30

>>>   #define CFG_SLAVE_ADDR_0_SHIFT       28

>>> +#define CFG_BIT_BANG_SHIFT           29

>> move this up one line (to be consistent with existing bit order)

> 

> Ack.

> 

>>>   #define CFG_M_RETRY_CNT_SHIFT        16

>>>   #define CFG_M_RETRY_CNT_MASK         0x0f

>>>   

>>> @@ -66,6 +67,12 @@

>>>   #define S_FIFO_RX_THLD_SHIFT         8

>>>   #define S_FIFO_RX_THLD_MASK          0x3f

>>>   

>>> +#define M_BB_CTRL_OFFSET             0x14

>>> +#define M_BB_SMBCLK_IN               31

>> M_BB_CTRL_CLK_IN_SHIFT, ket is to have '_SHIFT' to be consistent with

>> existing code

> 

> Ack.

> 

>>> +#define M_BB_SMBCLK_OUT_EN           30

>> M_BB_CTRL_CLK_OUT_SHIFT

>>

>>> +#define M_BB_SMBDAT_IN               29

>> M_BB_CTRL_DATA_IN_SHIFT

>>

>>> +#define M_BB_SMBDAT_OUT_EN           28

>> M_BB_CTRL_DATA_OUT_SHIFT

>>

>>> +

>>>   #define M_CMD_OFFSET                 0x30

>>>   #define M_CMD_START_BUSY_SHIFT       31

>>>   #define M_CMD_STATUS_SHIFT           25

>>> @@ -713,6 +720,112 @@ static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,

>>>   	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);

>>>   }

>>>   

>>> +static void bcm_iproc_i2c_reset(struct bcm_iproc_i2c_dev *iproc_i2c)

>>> +{

>>> +	u32 tmp;

>>> +

>>> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);

>>> +	tmp |= BIT(CFG_RESET_SHIFT);

>>> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);

>>> +	udelay(100);

>> This puts the controller in reset and hold it there, but never brings

>> the controller out of reset (bcm_iproc_i2c_init called in unprepare

>> brings the controller out of reset)

>>

>> Calling it a "reset" function is a bit misleading to me. My expectation

>> of a reset function is that you generate a reset pulse, ie.g., reset ->

>> delay -> out of reset.

>>

>> Why don't you simply put this seuquence of code in the prepare_recovery

>> function below, instead of calling this a reset function?

> 

> Will do. Should I also change the udelay(100) to a usleep_range(100, 

> 200)? Same question for the other uses of udelay().

> 


Sure I think that makes sense.

>>

>>> +

>>> +}

>>> +

>>> +static void bcm_iproc_i2c_prepare_recovery(struct i2c_adapter *adap)

>>> +{

>>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>>> +	u32 tmp;

>>> +

>>> +	dev_dbg(iproc_i2c->device, "Prepare recovery\n");

>>> +

>>> +	/* Disable interrupts */

>>> +	writel(0, iproc_i2c->base + IE_OFFSET);

>>> +	readl(iproc_i2c->base + IE_OFFSET);

>>> +	synchronize_irq(iproc_i2c->irq);

>>> +

>>> +	bcm_iproc_i2c_reset(iproc_i2c);

>>> +

>>> +	/* Switch to bit-bang mode */

>>> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);

>>> +	tmp |= BIT(CFG_BIT_BANG_SHIFT);

>>> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);

>> add usleep_range(100, 200) here, required delay after switching to bit

>> bang based on spec.

> 

> Ack.

> 

>>

>>> +}

>>> +

>>> +static void bcm_iproc_i2c_unprepare_recovery(struct i2c_adapter *adap)

>>> +{

>>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>>> +	u32 tmp;

>>> +

>>> +	/* Switch to normal mode */

>>> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);

>>> +	tmp &= ~BIT(CFG_BIT_BANG_SHIFT);

>>> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);

>>> +	udelay(100);

>>> +

>>> +	bcm_iproc_i2c_init(iproc_i2c);

>> Add sequence to re-configure to desired bus speed here after the reset

>> sequence (someone else in our team tested this is required to resume to

>> proper bus speed).

> bcm_iproc_i2c_resume() seems to have the exact sequence I need. If I 

> move it outside of CONFIG_PM_SLEEP I could just call it here.


Yes agree with you that 'bcm_iproc_i2c_resume' can be resued.

>>> +	bcm_iproc_i2c_enable_disable(iproc_i2c, true);

>>> +

>>> +	dev_dbg(iproc_i2c->device, "Recovery complete\n");

>>> +}

>>> +

>>> +static int bcm_iproc_i2c_get_scl(struct i2c_adapter *adap)

>>> +{

>>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>>> +	u32 tmp;

>>> +

>>> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

>>> +

>>> +	return !!(tmp & BIT(M_BB_SMBCLK_IN));

>>> +}

>>> +

>>> +static void bcm_iproc_i2c_set_scl(struct i2c_adapter *adap, int val)

>>> +{

>>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>>> +	u32 tmp;

>>> +

>>> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

>>> +	if (val)

>>> +		tmp |= BIT(M_BB_SMBCLK_OUT_EN);

>>> +	else

>>> +		tmp &= ~BIT(M_BB_SMBCLK_OUT_EN);

>>> +

>>> +	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);

>>> +}

>>> +

>>> +static void bcm_iproc_i2c_set_sda(struct i2c_adapter *adap, int val)

>>> +{

>>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>>> +	u32 tmp;

>>> +

>>> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

>>> +	if (val)

>>> +		tmp |= BIT(M_BB_SMBDAT_OUT_EN);

>>> +	else

>>> +		tmp &= ~BIT(M_BB_SMBDAT_OUT_EN);

>>> +

>>> +	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);

>>> +}

>>> +

>>> +static int bcm_iproc_i2c_get_sda(struct i2c_adapter *adap)

>>> +{

>>> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);

>>> +	u32 tmp;

>>> +

>>> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);

>>> +

>>> +	return !!(tmp & BIT(M_BB_SMBDAT_IN));

>>> +}

>>> +

>>> +static struct i2c_bus_recovery_info bcm_iproc_recovery_info = {

>> static const struct ...

> Ack.

>>> +	.recover_bus = i2c_generic_scl_recovery,

>>> +	.prepare_recovery = bcm_iproc_i2c_prepare_recovery,

>>> +	.unprepare_recovery = bcm_iproc_i2c_unprepare_recovery,

>>> +	.set_scl = bcm_iproc_i2c_set_scl,

>>> +	.get_scl = bcm_iproc_i2c_get_scl,

>>> +	.set_sda = bcm_iproc_i2c_set_sda,

>>> +	.get_sda = bcm_iproc_i2c_get_sda,

>>> +};

>>> +

>>>   static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,

>>>   				      struct i2c_msg *msg)

>>>   {

>>> @@ -839,6 +952,7 @@ static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,

>>>   	if (!!(iproc_i2c_rd_reg(iproc_i2c,

>>>   				M_CMD_OFFSET) & BIT(M_CMD_START_BUSY_SHIFT))) {

>>>   		dev_warn(iproc_i2c->device, "bus is busy\n");

>>> +		i2c_recover_bus(&iproc_i2c->adapter);

>> 'i2c_recover_bus' should not be ALWAYS called here. You don't know if

>> bus is actually locked up or it's other issues that caused this.

>>

>> We need a logic to detect and confirm the lock up condition before

>> committing to recover operation:

>>

>> /* Check if bus lockup occurred, and invoke recovery if so. */

>> static void iproc_i2c_lockup_recover(struct bcm_iproc_i2c_dev *iproc_i2c)

>> {

>> 	/*

>> 	 * assume bus lockup if SDA line is low;

>> 	 * note that there is no need to switch to

>> 	 * bit-bang mode for this check.

>> 	 */

>> 	if (!bcm_iproc_i2c_get_sda(&iproc_i2c->adapter)) {

>> 		/* locked up - invoke i2c bus recovery. */

>> 		int ret = i2c_recover_bus(&iproc_i2c->adapter);

>> 			if (ret)

>> 				dev_err(iproc_i2c->device,

>> 					"bus recovery: error %d\n",

>> 					ret);

>> 	}

>> }

>>

>> 'iproc_i2c_lockup_recover' should be called in two locations in the driver:

>>

>> 1. After 'transaction timed out' (and after flush both TX/RX FIFOS)

>> 2. After 'bcm_iproc_i2c_check_status' failures (and after flush both

>> TX/RX FIFOs).

> 

> I see bcm_iproc_i2c_check_status() has a "recovery" case on an unknown 

> error. Should I remove that if I'm going to call 

> iproc_i2c_lockup_recover() or check for ret != -EIO?

> 


You may leave that (as first level recovery due to some potential
internal ASIC issues).

Let iproc_i2c_lockup_recover handles the bus lockup type of recovery
(after confirming the bus is indeed locked up).

>>

>>>   		return -EBUSY;

>>>   	}

>>>   

>>> @@ -1111,6 +1225,7 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)

>>>   		of_node_full_name(iproc_i2c->device->of_node));

>>>   	adap->algo = &bcm_iproc_algo;

>>>   	adap->quirks = &bcm_iproc_i2c_quirks;

>>> +	adap->bus_recovery_info = &bcm_iproc_recovery_info;

>>>   	adap->dev.parent = &pdev->dev;

>>>   	adap->dev.of_node = pdev->dev.of_node;

>>>   

>>>

>> Thanks,

>>

>> Ray
Chris Packham June 2, 2021, 2:32 a.m. | #4
On 2/06/21 9:03 am, Ray Jui wrote:
>

> On 5/30/2021 3:56 PM, Chris Packham wrote:

>> From: Richard Laing <richard.laing@alliedtelesis.co.nz>

>>

>> The bcm-iproc controller can put the SDA/SCL lines into bit-bang mode,

>> make use of this to support i2c bus recovery.

>>

>> Signed-off-by: Richard Laing <richard.laing@alliedtelesis.co.nz>

>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

>> ---

<snip>
> +

>> +static struct i2c_bus_recovery_info bcm_iproc_recovery_info = {

> static const struct ...


The kernel test bot doesn't like the suggestion.

 >> drivers/i2c/busses/i2c-bcm-iproc.c:1264:26: error: assigning to 

'struct i2c_bus_recovery_info *' from 'const struct 
i2c_bus_recovery_info *' discards qualifiers 
[-Werror,-Wincompatible-pointer-types-discards-qualifiers]

            adap->bus_recovery_info = &bcm_iproc_recovery_info;
                                    ^ ~~~~~~~~~~~~~~~~~~~~~~~~

I can see why. Are you happy for me to drop the const in v3? Or perhaps 
you have a better suggestion.
Ray Jui June 3, 2021, 5:01 p.m. | #5
On 6/1/2021 7:32 PM, Chris Packham wrote:
> 

> On 2/06/21 9:03 am, Ray Jui wrote:

>>

>> On 5/30/2021 3:56 PM, Chris Packham wrote:

>>> From: Richard Laing <richard.laing@alliedtelesis.co.nz>

>>>

>>> The bcm-iproc controller can put the SDA/SCL lines into bit-bang mode,

>>> make use of this to support i2c bus recovery.

>>>

>>> Signed-off-by: Richard Laing <richard.laing@alliedtelesis.co.nz>

>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

>>> ---

> <snip>

>> +

>>> +static struct i2c_bus_recovery_info bcm_iproc_recovery_info = {

>> static const struct ...

> 

> The kernel test bot doesn't like the suggestion.

> 

>  >> drivers/i2c/busses/i2c-bcm-iproc.c:1264:26: error: assigning to 

> 'struct i2c_bus_recovery_info *' from 'const struct 

> i2c_bus_recovery_info *' discards qualifiers 

> [-Werror,-Wincompatible-pointer-types-discards-qualifiers]

> 

>             adap->bus_recovery_info = &bcm_iproc_recovery_info;

>                                     ^ ~~~~~~~~~~~~~~~~~~~~~~~~

> 

> I can see why. Are you happy for me to drop the const in v3? Or perhaps 

> you have a better suggestion.

> 


Yeah of course. Please drop it. Sorry I missed that while making that
suggestion.

Patch

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index cceaf69279a9..d63a286c1660 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -26,6 +26,7 @@ 
 #define CFG_RESET_SHIFT              31
 #define CFG_EN_SHIFT                 30
 #define CFG_SLAVE_ADDR_0_SHIFT       28
+#define CFG_BIT_BANG_SHIFT           29
 #define CFG_M_RETRY_CNT_SHIFT        16
 #define CFG_M_RETRY_CNT_MASK         0x0f
 
@@ -66,6 +67,12 @@ 
 #define S_FIFO_RX_THLD_SHIFT         8
 #define S_FIFO_RX_THLD_MASK          0x3f
 
+#define M_BB_CTRL_OFFSET             0x14
+#define M_BB_SMBCLK_IN               31
+#define M_BB_SMBCLK_OUT_EN           30
+#define M_BB_SMBDAT_IN               29
+#define M_BB_SMBDAT_OUT_EN           28
+
 #define M_CMD_OFFSET                 0x30
 #define M_CMD_START_BUSY_SHIFT       31
 #define M_CMD_STATUS_SHIFT           25
@@ -713,6 +720,112 @@  static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
 	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
 }
 
+static void bcm_iproc_i2c_reset(struct bcm_iproc_i2c_dev *iproc_i2c)
+{
+	u32 tmp;
+
+	tmp = readl(iproc_i2c->base + CFG_OFFSET);
+	tmp |= BIT(CFG_RESET_SHIFT);
+	writel(tmp, iproc_i2c->base + CFG_OFFSET);
+	udelay(100);
+
+}
+
+static void bcm_iproc_i2c_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	dev_dbg(iproc_i2c->device, "Prepare recovery\n");
+
+	/* Disable interrupts */
+	writel(0, iproc_i2c->base + IE_OFFSET);
+	readl(iproc_i2c->base + IE_OFFSET);
+	synchronize_irq(iproc_i2c->irq);
+
+	bcm_iproc_i2c_reset(iproc_i2c);
+
+	/* Switch to bit-bang mode */
+	tmp = readl(iproc_i2c->base + CFG_OFFSET);
+	tmp |= BIT(CFG_BIT_BANG_SHIFT);
+	writel(tmp, iproc_i2c->base + CFG_OFFSET);
+}
+
+static void bcm_iproc_i2c_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	/* Switch to normal mode */
+	tmp = readl(iproc_i2c->base + CFG_OFFSET);
+	tmp &= ~BIT(CFG_BIT_BANG_SHIFT);
+	writel(tmp, iproc_i2c->base + CFG_OFFSET);
+	udelay(100);
+
+	bcm_iproc_i2c_init(iproc_i2c);
+	bcm_iproc_i2c_enable_disable(iproc_i2c, true);
+
+	dev_dbg(iproc_i2c->device, "Recovery complete\n");
+}
+
+static int bcm_iproc_i2c_get_scl(struct i2c_adapter *adap)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
+
+	return !!(tmp & BIT(M_BB_SMBCLK_IN));
+}
+
+static void bcm_iproc_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
+	if (val)
+		tmp |= BIT(M_BB_SMBCLK_OUT_EN);
+	else
+		tmp &= ~BIT(M_BB_SMBCLK_OUT_EN);
+
+	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);
+}
+
+static void bcm_iproc_i2c_set_sda(struct i2c_adapter *adap, int val)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
+	if (val)
+		tmp |= BIT(M_BB_SMBDAT_OUT_EN);
+	else
+		tmp &= ~BIT(M_BB_SMBDAT_OUT_EN);
+
+	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);
+}
+
+static int bcm_iproc_i2c_get_sda(struct i2c_adapter *adap)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
+
+	return !!(tmp & BIT(M_BB_SMBDAT_IN));
+}
+
+static struct i2c_bus_recovery_info bcm_iproc_recovery_info = {
+	.recover_bus = i2c_generic_scl_recovery,
+	.prepare_recovery = bcm_iproc_i2c_prepare_recovery,
+	.unprepare_recovery = bcm_iproc_i2c_unprepare_recovery,
+	.set_scl = bcm_iproc_i2c_set_scl,
+	.get_scl = bcm_iproc_i2c_get_scl,
+	.set_sda = bcm_iproc_i2c_set_sda,
+	.get_sda = bcm_iproc_i2c_get_sda,
+};
+
 static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
 				      struct i2c_msg *msg)
 {
@@ -839,6 +952,7 @@  static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,
 	if (!!(iproc_i2c_rd_reg(iproc_i2c,
 				M_CMD_OFFSET) & BIT(M_CMD_START_BUSY_SHIFT))) {
 		dev_warn(iproc_i2c->device, "bus is busy\n");
+		i2c_recover_bus(&iproc_i2c->adapter);
 		return -EBUSY;
 	}
 
@@ -1111,6 +1225,7 @@  static int bcm_iproc_i2c_probe(struct platform_device *pdev)
 		of_node_full_name(iproc_i2c->device->of_node));
 	adap->algo = &bcm_iproc_algo;
 	adap->quirks = &bcm_iproc_i2c_quirks;
+	adap->bus_recovery_info = &bcm_iproc_recovery_info;
 	adap->dev.parent = &pdev->dev;
 	adap->dev.of_node = pdev->dev.of_node;