[PATCH 0/3] i2c-cht-wc: Various fixes

[PATCH 0/3] i2c-cht-wc: Various fixes

[Hans de Goede]

Hi Wolfram,

Sorry for sending this out so shortly after you merged the
adapter driver itself, but while testing some code which
sits on top of the adapter + charger combo I noticed some
subtle issues, which took me most of my weekend to get
to the bottom too.

I guess in a way this way is better as it will lead
to a git log explaining all the bug-fixes.

Regards,

Hans

[PATCH 1/3] i2c-cht-wc: Add locking to interrupt / smbus_xfer functions

[Hans de Goede]

Although unlikely without locking the smbux_xfer function may miss
the nack flag and further fixes in this patch-set add some more
complex constructions which need protection.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-cht-wc.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
index fe5caf70c7fe..09e0df49df6b 100644
--- a/drivers/i2c/busses/i2c-cht-wc.c
+++ b/drivers/i2c/busses/i2c-cht-wc.c
@@ -47,6 +47,7 @@ struct cht_wc_i2c_adap {
 	struct i2c_adapter adapter;
 	wait_queue_head_t wait;
 	struct irq_chip irqchip;
+	struct mutex adap_lock;
 	struct mutex irqchip_lock;
 	struct regmap *regmap;
 	struct irq_domain *irq_domain;
@@ -63,10 +64,13 @@ static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
 	struct cht_wc_i2c_adap *adap = data;
 	int ret, reg;
 
+	mutex_lock(&adap->adap_lock);
+
 	/* Read IRQs */
 	ret = regmap_read(adap->regmap, CHT_WC_EXTCHGRIRQ, &reg);
 	if (ret) {
 		dev_err(&adap->adapter.dev, "Error reading extchgrirq reg\n");
+		mutex_unlock(&adap->adap_lock);
 		return IRQ_NONE;
 	}
 
@@ -80,6 +84,16 @@ static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
 	if (ret)
 		dev_err(&adap->adapter.dev, "Error writing extchgrirq reg\n");
 
+	if (reg & CHT_WC_EXTCHGRIRQ_ADAP_IRQMASK) {
+		adap->nack = !!(reg & CHT_WC_EXTCHGRIRQ_NACK_IRQ);
+		adap->done = true;
+	}
+
+	mutex_unlock(&adap->adap_lock);
+
+	if (reg & CHT_WC_EXTCHGRIRQ_ADAP_IRQMASK)
+		wake_up(&adap->wait);
+
 	/*
 	 * Do NOT use handle_nested_irq here, the client irq handler will
 	 * likely want to do i2c transfers and the i2c controller uses this
@@ -96,12 +110,6 @@ static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
 		local_irq_enable();
 	}
 
-	if (reg & CHT_WC_EXTCHGRIRQ_ADAP_IRQMASK) {
-		adap->nack = !!(reg & CHT_WC_EXTCHGRIRQ_NACK_IRQ);
-		adap->done = true;
-		wake_up(&adap->wait);
-	}
-
 	return IRQ_HANDLED;
 }
 
@@ -119,8 +127,10 @@ static int cht_wc_i2c_adap_smbus_xfer(struct i2c_adapter *_adap, u16 addr,
 	struct cht_wc_i2c_adap *adap = i2c_get_adapdata(_adap);
 	int ret, reg;
 
+	mutex_lock(&adap->adap_lock);
 	adap->nack = false;
 	adap->done = false;
+	mutex_unlock(&adap->adap_lock);
 
 	ret = regmap_write(adap->regmap, CHT_WC_I2C_CLIENT_ADDR, addr);
 	if (ret)
@@ -146,18 +156,18 @@ static int cht_wc_i2c_adap_smbus_xfer(struct i2c_adapter *_adap, u16 addr,
 	ret = wait_event_timeout(adap->wait, adap->done, 3 * HZ);
 	if (ret == 0)
 		return -ETIMEDOUT;
-	if (adap->nack)
-		return -EIO;
 
-	if (read_write == I2C_SMBUS_READ) {
+	ret = 0;
+	mutex_lock(&adap->adap_lock);
+	if (adap->nack)
+		ret = -EIO;
+	else if (read_write == I2C_SMBUS_READ) {
 		ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA, &reg);
-		if (ret)
-			return ret;
-
 		data->byte = reg;
 	}
+	mutex_unlock(&adap->adap_lock);
 
-	return 0;
+	return ret;
 }
 
 static const struct i2c_algorithm cht_wc_i2c_adap_algo = {
@@ -241,6 +251,7 @@ static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	init_waitqueue_head(&adap->wait);
+	mutex_init(&adap->adap_lock);
 	mutex_init(&adap->irqchip_lock);
 	adap->irqchip = cht_wc_i2c_irq_chip;
 	adap->regmap = pmic->regmap;
-- 
2.13.4

Re: [PATCH 1/3] i2c-cht-wc: Add locking to interrupt / smbus_xfer functions

[Andy Shevchenko]

On Mon, 2017-08-14 at 22:17 +0200, Hans de Goede wrote:
> Although unlikely without locking the smbux_xfer function may miss
> the nack flag and further fixes in this patch-set add some more
> complex constructions which need protection.
 
> -	if (read_write == I2C_SMBUS_READ) {
> +	ret = 0;
> +	mutex_lock(&adap->adap_lock);
> +	if (adap->nack)
> +		ret = -EIO;
> +	else if (read_write == I2C_SMBUS_READ) {
>  		ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA,
> &reg);
> -		if (ret)
> -			return ret;
> -

>  		data->byte = reg;

Can this be moved out to keep logic the same (don't dirt data on error)?

>  	}
> +	mutex_unlock(&adap->adap_lock);
>  
> -	return 0;
> +	return ret;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/3] i2c-cht-wc: Add locking to interrupt / smbus_xfer functions

[Hans de Goede]

Hi,

On 14-08-17 22:35, Andy Shevchenko wrote:
> On Mon, 2017-08-14 at 22:17 +0200, Hans de Goede wrote:
>> Although unlikely without locking the smbux_xfer function may miss
>> the nack flag and further fixes in this patch-set add some more
>> complex constructions which need protection.
>   
>> -	if (read_write == I2C_SMBUS_READ) {
>> +	ret = 0;
>> +	mutex_lock(&adap->adap_lock);
>> +	if (adap->nack)
>> +		ret = -EIO;
>> +	else if (read_write == I2C_SMBUS_READ) {
>>   		ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA,
>> &reg);
>> -		if (ret)
>> -			return ret;
>> -
> 
>>   		data->byte = reg;
> 
> Can this be moved out to keep logic the same (don't dirt data on error)?

Setting data on error is not really a problem and this gets
fixed in the next patch in the series where the reading
of the data moves to the irq-handler.

Regards,

Hans

Re: [PATCH 1/3] i2c-cht-wc: Add locking to interrupt / smbus_xfer functions

[Wolfram Sang]

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

On Mon, Aug 14, 2017 at 10:17:24PM +0200, Hans de Goede wrote:
> Although unlikely without locking the smbux_xfer function may miss
> the nack flag and further fixes in this patch-set add some more
> complex constructions which need protection.

Could be reworded, but after reading it 3 times, I got it.

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-next, thanks! Please don't forget the "i2c: " prefix in the
subject.


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

[PATCH 2/3] i2c-cht-wc: Ack read irqs after reading the data register

[Hans de Goede]

Testing has shown that writing 1 to clear the read-complete irq does
not work until the data register has been read first.

This commit fixes the driver to read the data register first, halving the
amount of interrupts in most cases since we mostly read on this i2c bus.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-cht-wc.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
index 09e0df49df6b..11f7e516f1b1 100644
--- a/drivers/i2c/busses/i2c-cht-wc.c
+++ b/drivers/i2c/busses/i2c-cht-wc.c
@@ -55,7 +55,8 @@ struct cht_wc_i2c_adap {
 	int client_irq;
 	u8 irq_mask;
 	u8 old_irq_mask;
-	bool nack;
+	int read_data;
+	bool io_error;
 	bool done;
 };
 
@@ -76,6 +77,11 @@ static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
 
 	reg &= ~adap->irq_mask;
 
+	/* Reads must be acked after reading the received data. */
+	ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA, &adap->read_data);
+	if (ret)
+		adap->io_error = true;
+
 	/*
 	 * Immediately ack IRQs, so that if new IRQs arrives while we're
 	 * handling the previous ones our irq will re-trigger when we're done.
@@ -85,7 +91,7 @@ static irqreturn_t cht_wc_i2c_adap_thread_handler(int id, void *data)
 		dev_err(&adap->adapter.dev, "Error writing extchgrirq reg\n");
 
 	if (reg & CHT_WC_EXTCHGRIRQ_ADAP_IRQMASK) {
-		adap->nack = !!(reg & CHT_WC_EXTCHGRIRQ_NACK_IRQ);
+		adap->io_error |= !!(reg & CHT_WC_EXTCHGRIRQ_NACK_IRQ);
 		adap->done = true;
 	}
 
@@ -125,10 +131,10 @@ static int cht_wc_i2c_adap_smbus_xfer(struct i2c_adapter *_adap, u16 addr,
 				      union i2c_smbus_data *data)
 {
 	struct cht_wc_i2c_adap *adap = i2c_get_adapdata(_adap);
-	int ret, reg;
+	int ret;
 
 	mutex_lock(&adap->adap_lock);
-	adap->nack = false;
+	adap->io_error = false;
 	adap->done = false;
 	mutex_unlock(&adap->adap_lock);
 
@@ -159,12 +165,10 @@ static int cht_wc_i2c_adap_smbus_xfer(struct i2c_adapter *_adap, u16 addr,
 
 	ret = 0;
 	mutex_lock(&adap->adap_lock);
-	if (adap->nack)
+	if (adap->io_error)
 		ret = -EIO;
-	else if (read_write == I2C_SMBUS_READ) {
-		ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA, &reg);
-		data->byte = reg;
-	}
+	else if (read_write == I2C_SMBUS_READ)
+		data->byte = adap->read_data;
 	mutex_unlock(&adap->adap_lock);
 
 	return ret;
@@ -238,7 +242,7 @@ static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
 		.addr = 0x6b,
 		.properties = bq24190_props,
 	};
-	int ret, irq;
+	int ret, reg, irq;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
@@ -264,6 +268,11 @@ static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
 
 	/* Clear and activate i2c-adapter interrupts, disable client IRQ */
 	adap->old_irq_mask = adap->irq_mask = ~CHT_WC_EXTCHGRIRQ_ADAP_IRQMASK;
+
+	ret = regmap_read(adap->regmap, CHT_WC_I2C_RDDATA, &reg);
+	if (ret)
+		return ret;
+
 	ret = regmap_write(adap->regmap, CHT_WC_EXTCHGRIRQ, ~adap->irq_mask);
 	if (ret)
 		return ret;
-- 
2.13.4

Re: [PATCH 2/3] i2c-cht-wc: Ack read irqs after reading the data register

[Wolfram Sang]

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

On Mon, Aug 14, 2017 at 10:17:25PM +0200, Hans de Goede wrote:
> Testing has shown that writing 1 to clear the read-complete irq does
> not work until the data register has been read first.
> 
> This commit fixes the driver to read the data register first, halving the
> amount of interrupts in most cases since we mostly read on this i2c bus.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-next, thanks!


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

[PATCH 3/3] i2c-cht-wc: Workaround CHT GPIO controller IRQ issues

[Hans de Goede]

The Cherry Trail Whiskey Cove PMIC's IRQ line is attached to one of
the GPIOs of the Cherry Trail SoC. The CHT GPIO controller sometimes
fails to deliver IRQs (seen when there is an IRQ storm on another pin).

This commit works around this by reducing the long timeout which was
a poor attempt to workaround this from 3s to 30ms and after that
manually checking the status register for transfer completion by
calling the threaded IRQ handler directly.

This is safe todo as the entire threaded IRQ handler is protected
by a mutex.

Note 30ms should be more then long enough, at 100KHz any smbus single
byte transaction should be finished in 4ms.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-cht-wc.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
index 11f7e516f1b1..21312eed09e4 100644
--- a/drivers/i2c/busses/i2c-cht-wc.c
+++ b/drivers/i2c/busses/i2c-cht-wc.c
@@ -158,10 +158,16 @@ static int cht_wc_i2c_adap_smbus_xfer(struct i2c_adapter *_adap, u16 addr,
 	if (ret)
 		return ret;
 
-	/* 3 second timeout, during cable plug the PMIC responds quite slow */
-	ret = wait_event_timeout(adap->wait, adap->done, 3 * HZ);
-	if (ret == 0)
-		return -ETIMEDOUT;
+	ret = wait_event_timeout(adap->wait, adap->done, msecs_to_jiffies(30));
+	if (ret == 0) {
+		/*
+		 * The CHT GPIO controller serializes all IRQs, sometimes
+		 * causing significant delays, check status manually.
+		 */
+		cht_wc_i2c_adap_thread_handler(0, adap);
+		if (!adap->done)
+			return -ETIMEDOUT;
+	}
 
 	ret = 0;
 	mutex_lock(&adap->adap_lock);
-- 
2.13.4

Re: [PATCH 3/3] i2c-cht-wc: Workaround CHT GPIO controller IRQ issues

[Wolfram Sang]

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

On Mon, Aug 14, 2017 at 10:17:26PM +0200, Hans de Goede wrote:
> The Cherry Trail Whiskey Cove PMIC's IRQ line is attached to one of
> the GPIOs of the Cherry Trail SoC. The CHT GPIO controller sometimes
> fails to deliver IRQs (seen when there is an IRQ storm on another pin).
> 
> This commit works around this by reducing the long timeout which was
> a poor attempt to workaround this from 3s to 30ms and after that
> manually checking the status register for transfer completion by
> calling the threaded IRQ handler directly.
> 
> This is safe todo as the entire threaded IRQ handler is protected
> by a mutex.
> 
> Note 30ms should be more then long enough, at 100KHz any smbus single
> byte transaction should be finished in 4ms.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-next, thanks!


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