[PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[Vaibhav Hiremath]

This patch series fixes bugs/warnings, cleans up the code and adds
support for PXA910 family of devices to PXA I2C bus driver.

There has been one attempt made sometime back in 2012 to upstream
some of the patches from below list, but did not get follow up later.
I have consolidated all the patches, cleaned them up, splited into
logical changes, added new patches and submitting it now.

I tried to maintain authorship & Signoff except where I did some
significant changes to the code/logic.


Link to previous post:
http://permalink.gmane.org/gmane.linux.drivers.i2c/13557

Testing:
  - Basic testing on PMIC device on I2C-0 interface
  - Boot tested on platform based on PXA1928
  - Probe is successfully passing
  - Read few registers of PMIC (RTC, ID, etc...) during boot

V3 => V4
=======
Link to V3: http://www.spinics.net/lists/devicetree/msg85904.html

  - [PATCH 06/11] Removed unnecessary dev_err on devm_kzalloc() check
  - [PATCH 06/11] Removed return check on platform_get_resource(), as 
    devm_ioremap_resource() does it for us.
    Also, brought up the devm_ioremap_resource() function call in the execution
    sequence, as no point in delaying it if we do not have resource.
    It make sense, after this change.
  - [PATCH 04/11] Typecast changed to 'enum pxa_i2c_types'
    Also updated the subject line "Removed ==> Fix"

V2 => V3
=======
Link to V2: http://www.spinics.net/lists/linux-i2c/msg20059.html

  - Removed PATCH [4/12] related to reset of I2C module.
    Suggested by "Robert Jarzmik"
  - Updated commit description for,
      PATCH [11/12]: Mentioned reasoning about moment of clk_get code.
      PATCH [12/12]: for DT property node.
  - Added Acked by "Robert Jarzmik" to patched which he acked.

V1 => V2:
========
Link to V1 - http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347012.html

  - Fixed all comments from "Robert Jarzmik" and "Wolfram Sang"
  - Dropped Patch
	05/12: using core bus reset implementation - under work.
               Will submit shortly.
       	08/12: NAKed and dropped
  - Separated DT binding patch from driver changes, for easy merge


Leilei Shang (1):
  i2c: pxa: keep i2c irq ON in suspend

Shouming Wang (1):
  i2c: pxa: Return I2C_RETRY when timeout in pio mode

Vaibhav Hiremath (7):
  i2c: pxa: No need to set slave addr for i2c master mode reset
  i2c: pxa: Update debug function to dump more info on error
  i2c:pxa: Use devm_ variants in probe function
  Documentation: binding: add new property 'disable_after_xfer' to
    i2c-pxa
  i2c: pxa: Add support for pxa910/988 & new configuration features
  i2c: pxa: Add ILCR (tLow & tHigh) configuration support
  Documentation: binding: add sclk adjustment properties to i2c-pxa

Yi Zhang (1):
  i2c: pxa: enable/disable i2c module across msg xfer

Yipeng Yao (1):
  i2c: pxa: Fix compile warning in 64bit mode

 Documentation/devicetree/bindings/i2c/i2c-pxa.txt |  18 ++
 drivers/i2c/busses/i2c-pxa.c                      | 261 ++++++++++++++++------
 2 files changed, 211 insertions(+), 68 deletions(-)

-- 
1.9.1

[PATCH-v4 01/11] i2c: pxa: keep i2c irq ON in suspend

[Vaibhav Hiremath]

From: Leilei Shang <shangll@marvell.com>

During suspend there may still be some i2c access happening, as the
interrupt is shared between multiple drivers.
And if we don't keep i2c irq ON, there may be i2c access timeout if
i2c is in irq mode of operation.

Signed-off-by: Raul Xiong <xjian@marvell.com>
Signed-off-by: Xiaofan Tian <tianxf@marvell.com>
[vaibhav.hiremath@linaro.org: updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-pxa.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index d9c0d6a..f4ac8c5 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1232,8 +1232,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
 		i2c->adap.algo = &i2c_pxa_pio_algorithm;
 	} else {
 		i2c->adap.algo = &i2c_pxa_algorithm;
-		ret = request_irq(irq, i2c_pxa_handler, IRQF_SHARED,
-				  dev_name(&dev->dev), i2c);
+		ret = request_irq(irq, i2c_pxa_handler,
+				IRQF_SHARED | IRQF_NO_SUSPEND,
+				dev_name(&dev->dev), i2c);
 		if (ret)
 			goto ereqirq;
 	}
-- 
1.9.1

[PATCH-v4 02/11] i2c: pxa: No need to set slave addr for i2c master mode reset

[Vaibhav Hiremath]

Normally i2c controller works as master, so slave addr is not needed, or
it will impact some slave device (eg. ST NFC chip) i2c accesses, because
it has the same i2c address with controller.

For example,
On the pxa1928 based platform, where PMIC (88pm860) is present @0x30
address on TWSI0 interface, and if we set 0x30 as a slave address in
pxa1928 TWSI0 module, all the transactions towards PMIC would go for toss.

Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/i2c/busses/i2c-pxa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index f4ac8c5..023e59f 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -459,7 +459,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
 	writel(I2C_ISR_INIT, _ISR(i2c));
 	writel(readl(_ICR(i2c)) & ~ICR_UR, _ICR(i2c));
 
-	if (i2c->reg_isar)
+	if (i2c->reg_isar && IS_ENABLED(CONFIG_I2C_PXA_SLAVE))
 		writel(i2c->slave_addr, _ISAR(i2c));
 
 	/* set control register values */
-- 
1.9.1

[PATCH-v4 03/11] i2c: pxa: Return I2C_RETRY when timeout in pio mode

[Vaibhav Hiremath]

From: Shouming Wang <wangshm@marvell.com>

In case of timeout in pio mode of operation return I2C_RETRY.
This behavior will be same as interrupt mode of operation.

Signed-off-by: Shouming Wang <wangshm@marvell.com>
[vaibhav.hiremath@linaro.org: Updated changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/i2c/busses/i2c-pxa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 023e59f..632008f 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -745,8 +745,10 @@ static int i2c_pxa_do_pio_xfer(struct pxa_i2c *i2c,
 	ret = i2c->msg_idx;
 
 out:
-	if (timeout == 0)
+	if (timeout == 0) {
 		i2c_pxa_scream_blue_murder(i2c, "timeout");
+		ret = I2C_RETRY;
+	}
 
 	return ret;
 }
-- 
1.9.1

[PATCH-v4 04/11] i2c: pxa: Fix compile warning in 64bit mode

[Vaibhav Hiremath]

From: Yipeng Yao <ypyao@marvell.com>

Fix below warning message, coming from 64 bit toolchain.

drivers/i2c/busses/i2c-pxa.c:1237:15:
warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]

Signed-off-by: Yipeng Yao <ypyao@marvell.com>
[vaibhav.hiremath@linaro.org: Updated Changelog]
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/i2c/busses/i2c-pxa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 632008f..d0cc058 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1116,7 +1116,9 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 		i2c->use_pio = 1;
 	if (of_get_property(np, "mrvl,i2c-fast-mode", NULL))
 		i2c->fast_mode = 1;
-	*i2c_types = (u32)(of_id->data);
+
+	*i2c_types = (enum pxa_i2c_types)(of_id->data);
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH-v4 05/11] i2c: pxa: Update debug function to dump more info on error

[Vaibhav Hiremath]

Update i2c_pxa_scream_blue_murder() fn to print more information
in case of error.
Also, use dev_err variants instead of printk.

Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-pxa.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index d0cc058..beeed7c 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -132,6 +132,7 @@ struct pxa_i2c {
 	unsigned int		msg_idx;
 	unsigned int		msg_ptr;
 	unsigned int		slave_addr;
+	unsigned int		req_slave_addr;
 
 	struct i2c_adapter	adap;
 	struct clk		*clk;
@@ -253,15 +254,20 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int lno, const char *fname)
 static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
 {
 	unsigned int i;
-	printk(KERN_ERR "i2c: error: %s\n", why);
-	printk(KERN_ERR "i2c: msg_num: %d msg_idx: %d msg_ptr: %d\n",
+	struct device *dev = &i2c->adap.dev;
+
+	dev_err(dev, "slave_0x%x error: %s\n",
+		i2c->req_slave_addr >> 1, why);
+	dev_err(dev, "msg_num: %d msg_idx: %d msg_ptr: %d\n",
 		i2c->msg_num, i2c->msg_idx, i2c->msg_ptr);
-	printk(KERN_ERR "i2c: ICR: %08x ISR: %08x\n",
-	       readl(_ICR(i2c)), readl(_ISR(i2c)));
-	printk(KERN_DEBUG "i2c: log: ");
+	dev_err(dev, "IBMR: %08x IDBR: %08x ICR: %08x ISR: %08x\n",
+		readl(_IBMR(i2c)), readl(_IDBR(i2c)), readl(_ICR(i2c)),
+		readl(_ISR(i2c)));
+	dev_dbg(dev, "log: ");
 	for (i = 0; i < i2c->irqlogidx; i++)
-		printk("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
-	printk("\n");
+		pr_debug("[%08x:%08x] ", i2c->isrlog[i], i2c->icrlog[i]);
+
+	pr_debug("\n");
 }
 
 #else /* ifdef DEBUG */
@@ -638,6 +644,7 @@ static inline void i2c_pxa_start_message(struct pxa_i2c *i2c)
 	 * Step 1: target slave address into IDBR
 	 */
 	writel(i2c_pxa_addr_byte(i2c->msg), _IDBR(i2c));
+	i2c->req_slave_addr = i2c_pxa_addr_byte(i2c->msg);
 
 	/*
 	 * Step 2: initiate the write.
@@ -951,6 +958,7 @@ static void i2c_pxa_irq_txempty(struct pxa_i2c *i2c, u32 isr)
 		 * Write the next address.
 		 */
 		writel(i2c_pxa_addr_byte(i2c->msg), _IDBR(i2c));
+		i2c->req_slave_addr = i2c_pxa_addr_byte(i2c->msg);
 
 		/*
 		 * And trigger a repeated start, and send the byte.
-- 
1.9.1

[PATCH-v4 06/11] i2c:pxa: Use devm_ variants in probe function

[Vaibhav Hiremath]

This patch cleans up i2c_pxa_probe() function,

 - Use devm_ variants wherever
   This will clean both probe exit and i2c_pxa_remove() functions

 - Check platform resource before parsing any other data from DT/platform

 - Use dev_err on failure from i2c_add_numbered_adapter()

 - Use pr_info instead of printk for KERN_INFO

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/i2c/busses/i2c-pxa.c | 81 ++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 51 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index beeed7c..66cf437 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1158,10 +1158,23 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	struct resource *res = NULL;
 	int ret, irq;
 
-	i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
-	if (!i2c) {
-		ret = -ENOMEM;
-		goto emalloc;
+	i2c = devm_kzalloc(&dev->dev, sizeof(struct pxa_i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+
+	i2c->reg_base = devm_ioremap_resource(&dev->dev, res);
+	if (IS_ERR(i2c->reg_base)) {
+		dev_err(&dev->dev, "failed to map resource: %ld\n",
+			PTR_ERR(i2c->reg_base));
+		return PTR_ERR(i2c->reg_base);
+	}
+
+	irq = platform_get_irq(dev, 0);
+	if (irq < 0) {
+		dev_err(&dev->dev, "no irq resource: %d\n", irq);
+		return irq;
 	}
 
 	/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
@@ -1171,19 +1184,7 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	if (ret > 0)
 		ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
 	if (ret < 0)
-		goto eclk;
-
-	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
-	irq = platform_get_irq(dev, 0);
-	if (res == NULL || irq < 0) {
-		ret = -ENODEV;
-		goto eclk;
-	}
-
-	if (!request_mem_region(res->start, resource_size(res), res->name)) {
-		ret = -ENOMEM;
-		goto eclk;
-	}
+		return ret;
 
 	i2c->adap.owner   = THIS_MODULE;
 	i2c->adap.retries = 5;
@@ -1193,16 +1194,10 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 	strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
 
-	i2c->clk = clk_get(&dev->dev, NULL);
+	i2c->clk = devm_clk_get(&dev->dev, NULL);
 	if (IS_ERR(i2c->clk)) {
-		ret = PTR_ERR(i2c->clk);
-		goto eclk;
-	}
-
-	i2c->reg_base = ioremap(res->start, resource_size(res));
-	if (!i2c->reg_base) {
-		ret = -EIO;
-		goto eremap;
+		dev_err(&dev->dev, "failed to get the clk: %ld\n", PTR_ERR(i2c->clk));
+		return PTR_ERR(i2c->clk);
 	}
 
 	i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
@@ -1244,11 +1239,13 @@ static int i2c_pxa_probe(struct platform_device *dev)
 		i2c->adap.algo = &i2c_pxa_pio_algorithm;
 	} else {
 		i2c->adap.algo = &i2c_pxa_algorithm;
-		ret = request_irq(irq, i2c_pxa_handler,
+		ret = devm_request_irq(&dev->dev, irq, i2c_pxa_handler,
 				IRQF_SHARED | IRQF_NO_SUSPEND,
 				dev_name(&dev->dev), i2c);
-		if (ret)
+		if (ret) {
+			dev_err(&dev->dev, "failed to request irq: %d\n", ret);
 			goto ereqirq;
+		}
 	}
 
 	i2c_pxa_reset(i2c);
@@ -1261,33 +1258,22 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 	ret = i2c_add_numbered_adapter(&i2c->adap);
 	if (ret < 0) {
-		printk(KERN_INFO "I2C: Failed to add bus\n");
-		goto eadapt;
+		dev_err(&dev->dev, "failed to add bus: %d\n", ret);
+		goto ereqirq;
 	}
 
 	platform_set_drvdata(dev, i2c);
 
 #ifdef CONFIG_I2C_PXA_SLAVE
-	printk(KERN_INFO "I2C: %s: PXA I2C adapter, slave address %d\n",
-	       dev_name(&i2c->adap.dev), i2c->slave_addr);
+	dev_info(&i2c->adap.dev, " PXA I2C adapter, slave address %d\n",
+		i2c->slave_addr);
 #else
-	printk(KERN_INFO "I2C: %s: PXA I2C adapter\n",
-	       dev_name(&i2c->adap.dev));
+	dev_info(&i2c->adap.dev, " PXA I2C adapter\n");
 #endif
 	return 0;
 
-eadapt:
-	if (!i2c->use_pio)
-		free_irq(irq, i2c);
 ereqirq:
 	clk_disable_unprepare(i2c->clk);
-	iounmap(i2c->reg_base);
-eremap:
-	clk_put(i2c->clk);
-eclk:
-	kfree(i2c);
-emalloc:
-	release_mem_region(res->start, resource_size(res));
 	return ret;
 }
 
@@ -1296,15 +1282,8 @@ static int i2c_pxa_remove(struct platform_device *dev)
 	struct pxa_i2c *i2c = platform_get_drvdata(dev);
 
 	i2c_del_adapter(&i2c->adap);
-	if (!i2c->use_pio)
-		free_irq(i2c->irq, i2c);
 
 	clk_disable_unprepare(i2c->clk);
-	clk_put(i2c->clk);
-
-	iounmap(i2c->reg_base);
-	release_mem_region(i2c->iobase, i2c->iosize);
-	kfree(i2c);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH-v4 06/11] i2c:pxa: Use devm_ variants in probe function

[Wolfram Sang]

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

> +	i2c->reg_base = devm_ioremap_resource(&dev->dev, res);
> +	if (IS_ERR(i2c->reg_base)) {
> +		dev_err(&dev->dev, "failed to map resource: %ld\n",
> +			PTR_ERR(i2c->reg_base));
> +		return PTR_ERR(i2c->reg_base);
> +	}

One change I did when applying: removed this error message.
devm_ioremap_resource prints out the errors it finds.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH-v4 06/11] i2c:pxa: Use devm_ variants in probe function

[Vaibhav Hiremath]

On Tuesday 14 July 2015 05:05 PM, Wolfram Sang wrote:
>> +	i2c->reg_base = devm_ioremap_resource(&dev->dev, res);
>> +	if (IS_ERR(i2c->reg_base)) {
>> +		dev_err(&dev->dev, "failed to map resource: %ld\n",
>> +			PTR_ERR(i2c->reg_base));
>> +		return PTR_ERR(i2c->reg_base);
>> +	}
>
> One change I did when applying: removed this error message.
> devm_ioremap_resource prints out the errors it finds.
>

devm_ioremap_resource doesn't print return value.

So this additional error message would print one of, -EINVAL, -EBUSY
or -ENOMEM.

That was the reason I kept it.

If you feel it is not required, I am OK to remove it.

Thanks for the update, it certainly saved one more version :) .

Thanks,
Vaibhav

Re: [PATCH-v4 06/11] i2c:pxa: Use devm_ variants in probe function

[Wolfram Sang]

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


> So this additional error message would print one of, -EINVAL, -EBUSY
> or -ENOMEM.

The driver core can report this; dunno from head if it is verbose output
or not.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH-v4 07/11] i2c: pxa: enable/disable i2c module across msg xfer

[Vaibhav Hiremath]

From: Yi Zhang <yizhang@marvell.com>

Enable i2c module/unit before transmission and disable when it
finishes.

why?
It's because the i2c bus may be disturbed if the slave device,
typically a touch, powers on.

As we do not want to break slave mode support, this patch introduces
DT property to control disable of the I2C module after xfer in master
mode of operation.

i2c-disable-after-xfer : If set, driver will disable I2C module after
msg xfer

Signed-off-by: Yi Zhang <yizhang@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
Note that, in order _NOT_ to break existing slave support, we can not
enable this property by default. The only option is to use DT property
to control this feature.

 drivers/i2c/busses/i2c-pxa.c | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 66cf437..abf04f2 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -161,6 +161,7 @@ struct pxa_i2c {
 	unsigned char		master_code;
 	unsigned long		rate;
 	bool			highmode_enter;
+	bool			disable_after_xfer;
 };
 
 #define _IBMR(i2c)	((i2c)->reg_ibmr)
@@ -284,6 +285,24 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
 static void i2c_pxa_master_complete(struct pxa_i2c *i2c, int ret);
 static irqreturn_t i2c_pxa_handler(int this_irq, void *dev_id);
 
+/* enable/disable i2c unit */
+static inline int i2c_pxa_is_enabled(struct pxa_i2c *i2c)
+{
+	return (readl(_ICR(i2c)) & ICR_IUE);
+}
+
+static inline void i2c_pxa_enable(struct pxa_i2c *i2c, bool enable)
+{
+	if (enable) {
+		if (!i2c_pxa_is_enabled(i2c)) {
+			writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c));
+			udelay(100);
+		}
+	} else {
+		writel(readl(_ICR(i2c)) & ~ICR_IUE, _ICR(i2c));
+	}
+}
+
 static inline int i2c_pxa_is_slavemode(struct pxa_i2c *i2c)
 {
 	return !(readl(_ICR(i2c)) & ICR_SCLE);
@@ -480,8 +499,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
 	i2c_pxa_set_slave(i2c, 0);
 
 	/* enable unit */
-	writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c));
-	udelay(100);
+	i2c_pxa_enable(i2c, true);
 }
 
 
@@ -832,6 +850,9 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap,
 	struct pxa_i2c *i2c = adap->algo_data;
 	int ret, i;
 
+	/* Enable i2c unit */
+	i2c_pxa_enable(i2c, true);
+
 	/* If the I2C controller is disabled we need to reset it
 	  (probably due to a suspend/resume destroying state). We do
 	  this here as we can then avoid worrying about resuming the
@@ -852,6 +873,11 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap,
 	ret = -EREMOTEIO;
  out:
 	i2c_pxa_set_slave(i2c, ret);
+
+	/* disable i2c unit */
+	if (i2c->disable_after_xfer)
+		i2c_pxa_enable(i2c, false);
+
 	return ret;
 }
 
@@ -1067,6 +1093,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num
 	struct pxa_i2c *i2c = adap->algo_data;
 	int ret, i;
 
+	/* Enable i2c unit */
+	i2c_pxa_enable(i2c, true);
+
 	for (i = adap->retries; i >= 0; i--) {
 		ret = i2c_pxa_do_xfer(i2c, msgs, num);
 		if (ret != I2C_RETRY)
@@ -1080,6 +1109,10 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num
 	ret = -EREMOTEIO;
  out:
 	i2c_pxa_set_slave(i2c, ret);
+	/* disable i2c unit */
+	if (i2c->disable_after_xfer)
+		i2c_pxa_enable(i2c, false);
+
 	return ret;
 }
 
@@ -1120,6 +1153,9 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 	/* For device tree we always use the dynamic or alias-assigned ID */
 	i2c->adap.nr = -1;
 
+	i2c->disable_after_xfer = of_property_read_bool(np,
+				"i2c-disable-after-xfer");
+
 	if (of_get_property(np, "mrvl,i2c-polling", NULL))
 		i2c->use_pio = 1;
 	if (of_get_property(np, "mrvl,i2c-fast-mode", NULL))
@@ -1264,6 +1300,9 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 	platform_set_drvdata(dev, i2c);
 
+	if (i2c->disable_after_xfer)
+		i2c_pxa_enable(i2c, false);
+
 #ifdef CONFIG_I2C_PXA_SLAVE
 	dev_info(&i2c->adap.dev, " PXA I2C adapter, slave address %d\n",
 		i2c->slave_addr);
-- 
1.9.1

[PATCH-v4 08/11] Documentation: binding: add new property 'disable_after_xfer' to i2c-pxa

[Vaibhav Hiremath]

Driver now supports enable/disable across msg xfer, which user
can control it by new DT property -

i2c-disable-after-xfer : If set, driver will disable I2C module after msg
 xfer and enable it back before xfer.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/i2c/i2c-pxa.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
index 12b78ac..9657db5 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
@@ -18,6 +18,11 @@ Recommended properties :
    status register of i2c controller instead.
  - mrvl,i2c-fast-mode : Enable fast mode of i2c controller.
 
+Optional properties :
+
+ - i2c-disable-after-xfer : If set, driver will disable I2C module
+   after msg xfer and enable it again before xfer.
+
 Examples:
 	twsi1: i2c@d4011000 {
 		compatible = "mrvl,mmp-twsi";
-- 
1.9.1

[PATCH-v4 09/11] i2c: pxa: Add support for pxa910/988 & new configuration features

[Vaibhav Hiremath]

TWSI_ILCR & TWSI_IWCR registers are used to adjust clock rate
of standard & fast mode in pxa910/988; so this patch adds these two new
entries to "struct pxa_reg_layout" and "struct pxa_i2c".

As discussed in the previous patch-series, the idea here is to add standard
DT properties for ilcr and iwcr configuration fields.
In case of Master ilcr is used for low/high time and in case of slave mode
of operation iwcr is used for setup/hold time.

Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
Signed-off-by: Yi Zhang <yizhang@marvell.com>
Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/i2c/busses/i2c-pxa.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index abf04f2..8d76197 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -46,12 +46,15 @@ struct pxa_reg_layout {
 	u32 icr;
 	u32 isr;
 	u32 isar;
+	u32 ilcr;
+	u32 iwcr;
 };
 
 enum pxa_i2c_types {
 	REGS_PXA2XX,
 	REGS_PXA3XX,
 	REGS_CE4100,
+	REGS_PXA910,
 };
 
 /*
@@ -79,12 +82,22 @@ static struct pxa_reg_layout pxa_reg_layout[] = {
 		.isr =	0x04,
 		/* no isar register */
 	},
+	[REGS_PXA910] = {
+		.ibmr = 0x00,
+		.idbr = 0x08,
+		.icr =	0x10,
+		.isr =	0x18,
+		.isar = 0x20,
+		.ilcr = 0x28,
+		.iwcr = 0x30,
+	},
 };
 
 static const struct platform_device_id i2c_pxa_id_table[] = {
 	{ "pxa2xx-i2c",		REGS_PXA2XX },
 	{ "pxa3xx-pwri2c",	REGS_PXA3XX },
 	{ "ce4100-i2c",		REGS_CE4100 },
+	{ "pxa910-i2c",		REGS_PXA910 },
 	{ },
 };
 MODULE_DEVICE_TABLE(platform, i2c_pxa_id_table);
@@ -124,6 +137,24 @@ MODULE_DEVICE_TABLE(platform, i2c_pxa_id_table);
 #define ISR_SAD		(1 << 9)	   /* slave address detected */
 #define ISR_BED		(1 << 10)	   /* bus error no ACK/NAK */
 
+/* bit field shift & mask */
+#define ILCR_SLV_SHIFT		0
+#define ILCR_SLV_MASK		(0x1FF << ILCR_SLV_SHIFT)
+#define ILCR_FLV_SHIFT		9
+#define ILCR_FLV_MASK		(0x1FF << ILCR_FLV_SHIFT)
+#define ILCR_HLVL_SHIFT		18
+#define ILCR_HLVL_MASK		(0x1FF << ILCR_HLVL_SHIFT)
+#define ILCR_HLVH_SHIFT		27
+#define ILCR_HLVH_MASK		(0x1F << ILCR_HLVH_SHIFT)
+
+#define IWCR_CNT_SHIFT		0
+#define IWCR_CNT_MASK		(0x1F << IWCR_CNT_SHIFT)
+#define IWCR_HS_CNT1_SHIFT	5
+#define IWCR_HS_CNT1_MASK	(0x1F << IWCR_HS_CNT1_SHIFT)
+#define IWCR_HS_CNT2_SHIFT	10
+#define IWCR_HS_CNT2_MASK	(0x1F << IWCR_HS_CNT2_SHIFT)
+
+
 struct pxa_i2c {
 	spinlock_t		lock;
 	wait_queue_head_t	wait;
@@ -150,6 +181,8 @@ struct pxa_i2c {
 	void __iomem		*reg_icr;
 	void __iomem		*reg_isr;
 	void __iomem		*reg_isar;
+	void __iomem		*reg_ilcr;
+	void __iomem		*reg_iwcr;
 
 	unsigned long		iobase;
 	unsigned long		iosize;
@@ -169,6 +202,8 @@ struct pxa_i2c {
 #define _ICR(i2c)	((i2c)->reg_icr)
 #define _ISR(i2c)	((i2c)->reg_isr)
 #define _ISAR(i2c)	((i2c)->reg_isar)
+#define _ILCR(i2c)	((i2c)->reg_ilcr)
+#define _IWCR(i2c)	((i2c)->reg_iwcr)
 
 /*
  * I2C Slave mode address
@@ -1135,7 +1170,7 @@ static const struct i2c_algorithm i2c_pxa_pio_algorithm = {
 static const struct of_device_id i2c_pxa_dt_ids[] = {
 	{ .compatible = "mrvl,pxa-i2c", .data = (void *)REGS_PXA2XX },
 	{ .compatible = "mrvl,pwri2c", .data = (void *)REGS_PXA3XX },
-	{ .compatible = "mrvl,mmp-twsi", .data = (void *)REGS_PXA2XX },
+	{ .compatible = "mrvl,mmp-twsi", .data = (void *)REGS_PXA910 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, i2c_pxa_dt_ids);
@@ -1243,6 +1278,11 @@ static int i2c_pxa_probe(struct platform_device *dev)
 	if (i2c_type != REGS_CE4100)
 		i2c->reg_isar = i2c->reg_base + pxa_reg_layout[i2c_type].isar;
 
+	if (i2c_type == REGS_PXA910) {
+		i2c->reg_ilcr = i2c->reg_base + pxa_reg_layout[i2c_type].ilcr;
+		i2c->reg_iwcr = i2c->reg_base + pxa_reg_layout[i2c_type].iwcr;
+	}
+
 	i2c->iobase = res->start;
 	i2c->iosize = resource_size(res);
 
-- 
1.9.1

[PATCH-v4 10/11] i2c: pxa: Add ILCR (tLow & tHigh) configuration support

[Vaibhav Hiremath]

With addition of PXA910 family of devices, the TWSI module supports
SCL clock adjustment using ILCR register.

This patch enables the control and configuration of ICLR through DT
properties,

i2c-sclk-high-time-ns:
  SCLK high time (tHigh), for standard/fast/high speed mode
i2c-sclk-low-time-ns:
  SCLK low time (tLow), for standard/fast/high speed mode

Note that in case of standard and fast mod, the tLow and tHigh counters
are same, and software will use tLow value.

Also, brought up devm_clk_get() fn above i2c_pxa_probe_dt(), as it
uses clk rate for timing calculations.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
Signed-off-by: Jett.Zhou <jtzhou@marvell.com>
Signed-off-by: Yi Zhang <yizhang@marvell.com>
---
 drivers/i2c/busses/i2c-pxa.c | 66 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 8d76197..ee79599 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -195,6 +195,9 @@ struct pxa_i2c {
 	unsigned long		rate;
 	bool			highmode_enter;
 	bool			disable_after_xfer;
+
+	unsigned int		sclk_thigh_load_cnt;
+	unsigned int		sclk_tlow_load_cnt;
 };
 
 #define _IBMR(i2c)	((i2c)->reg_ibmr)
@@ -507,6 +510,33 @@ static void i2c_pxa_set_slave(struct pxa_i2c *i2c, int errcode)
 #define i2c_pxa_set_slave(i2c, err)	do { } while (0)
 #endif
 
+static void i2c_pxa_do_sclk_adj(struct pxa_i2c *i2c)
+{
+	unsigned int reg_ilcr;
+
+	reg_ilcr = readl(_ILCR(i2c));
+
+	/* For standard/fast mode tlow and thigh counters are same */
+	if (i2c->sclk_tlow_load_cnt) {
+		unsigned int mask, shift;
+
+		mask = i2c->high_mode ? ILCR_HLVL_MASK :
+			i2c->fast_mode ? ILCR_FLV_MASK : ILCR_SLV_MASK;
+		shift = i2c->high_mode ? ILCR_HLVL_SHIFT :
+			i2c->fast_mode ? ILCR_FLV_SHIFT : ILCR_SLV_SHIFT;
+
+		reg_ilcr &= ~mask;
+		reg_ilcr |= i2c->sclk_tlow_load_cnt << shift;
+	}
+
+	if (i2c->high_mode && i2c->sclk_thigh_load_cnt) {
+		reg_ilcr &= ~ILCR_HLVH_MASK;
+		reg_ilcr |= i2c->sclk_thigh_load_cnt << ILCR_HLVH_SHIFT;
+	}
+
+	writel(reg_ilcr, _ILCR(i2c));
+}
+
 static void i2c_pxa_reset(struct pxa_i2c *i2c)
 {
 	pr_debug("Resetting I2C Controller Unit\n");
@@ -526,6 +556,8 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
 	writel(I2C_ICR_INIT | (i2c->fast_mode ? ICR_FM : 0), _ICR(i2c));
 	writel(readl(_ICR(i2c)) | (i2c->high_mode ? ICR_HS : 0), _ICR(i2c));
 
+	i2c_pxa_do_sclk_adj(i2c);
+
 #ifdef CONFIG_I2C_PXA_SLAVE
 	dev_info(&i2c->adap.dev, "Enabling slave mode\n");
 	writel(readl(_ICR(i2c)) | ICR_SADIE | ICR_ALDIE | ICR_SSDIE, _ICR(i2c));
@@ -1198,6 +1230,26 @@ static int i2c_pxa_probe_dt(struct platform_device *pdev, struct pxa_i2c *i2c,
 
 	*i2c_types = (enum pxa_i2c_types)(of_id->data);
 
+	/* optional properties */
+	if (of_device_is_compatible(np, "mrvl,mmp-twsi")) {
+		unsigned int tlow = 0, thigh = 0;
+		unsigned int clk_ns;
+
+		/* clock time in nsec */
+		clk_ns = 1000000 / (i2c->rate / 1000);
+
+		of_property_read_u32(np, "i2c-sclk-high-time-ns", &thigh);
+		i2c->sclk_thigh_load_cnt = thigh / clk_ns;
+
+		of_property_read_u32(np, "i2c-sclk-low-time-ns", &tlow);
+		i2c->sclk_tlow_load_cnt = tlow / clk_ns;
+
+		/* For std/fast mode tlow & thigh have same bit-fields */
+		if (!i2c->high_mode &&
+			(i2c->sclk_tlow_load_cnt != i2c->sclk_thigh_load_cnt))
+			dev_warn(&i2c->adap.dev,
+				"mismatch of tLow & tHigh values, using tLow\n");
+	}
 	return 0;
 }
 
@@ -1248,6 +1300,14 @@ static int i2c_pxa_probe(struct platform_device *dev)
 		return irq;
 	}
 
+	i2c->clk = devm_clk_get(&dev->dev, NULL);
+	if (IS_ERR(i2c->clk)) {
+		dev_err(&dev->dev, "failed to get the clk: %ld\n", PTR_ERR(i2c->clk));
+		return PTR_ERR(i2c->clk);
+	}
+
+	i2c->rate = clk_get_rate(i2c->clk);
+
 	/* Default adapter num to device id; i2c_pxa_probe_dt can override. */
 	i2c->adap.nr = dev->id;
 
@@ -1265,12 +1325,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
 
 	strlcpy(i2c->adap.name, "pxa_i2c-i2c", sizeof(i2c->adap.name));
 
-	i2c->clk = devm_clk_get(&dev->dev, NULL);
-	if (IS_ERR(i2c->clk)) {
-		dev_err(&dev->dev, "failed to get the clk: %ld\n", PTR_ERR(i2c->clk));
-		return PTR_ERR(i2c->clk);
-	}
-
 	i2c->reg_ibmr = i2c->reg_base + pxa_reg_layout[i2c_type].ibmr;
 	i2c->reg_idbr = i2c->reg_base + pxa_reg_layout[i2c_type].idbr;
 	i2c->reg_icr = i2c->reg_base + pxa_reg_layout[i2c_type].icr;
-- 
1.9.1

[PATCH-v4 11/11] Documentation: binding: add sclk adjustment properties to i2c-pxa

[Vaibhav Hiremath]

With addition of PXA910 family of devices, the TWSI module supports
new feature which allows us to adjust SCLK. i2c-pxa driver takes input
configuration in nsec and converts it to respective bit-fields,

 - i2c-sclk-low-time-ns : SCLK low time (tlow)
   This property is used along with mode selection.
 - i2c-sclk-high-time-ns : SCLK high time (thigh)
 - i2c-start-hold-time-ns : Used in case of high speed mode for start bit
   hold/setup wait counter.
 - i2c-stop-hold-time-ns : Used in case of high speed mode for stop bit
   hold/setup wait counter.
 - i2c-sda-hold-time-ns : Used to calculate hold/setup wait counter for
   standard and fast mode.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/i2c/i2c-pxa.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
index 9657db5..bb4df60 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
@@ -23,12 +23,25 @@ Optional properties :
  - i2c-disable-after-xfer : If set, driver will disable I2C module
    after msg xfer and enable it again before xfer.
 
+   (Applicable to PXA910 family):
+
+ - i2c-sclk-low-time-ns : SCLK low time (tlow), for standard/fast/high
+   speed mode.
+   This property is used along with mode selection. Driver uses this property
+   to set low/high time for standard and fast speed mode, as HW counter
+   bit-field is same for both.
+ - i2c-sclk-high-time-ns : SCLK high time (thigh), Used in case of high speed
+   mode only.
+
 Examples:
 	twsi1: i2c@d4011000 {
 		compatible = "mrvl,mmp-twsi";
 		reg = <0xd4011000 0x1000>;
 		interrupts = <7>;
 		mrvl,i2c-fast-mode;
+
+		i2c-sclk-low-time-ns = <988>;
+		i2c-sclk-high-time-ns = <988>;
 	};
 	
 	twsi2: i2c@d4025000 {
-- 
1.9.1

Re: [PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[Wolfram Sang]

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

On Tue, Jul 14, 2015 at 01:06:39PM +0530, Vaibhav Hiremath wrote:
> This patch series fixes bugs/warnings, cleans up the code and adds
> support for PXA910 family of devices to PXA I2C bus driver.
> 
> There has been one attempt made sometime back in 2012 to upstream
> some of the patches from below list, but did not get follow up later.
> I have consolidated all the patches, cleaned them up, splited into
> logical changes, added new patches and submitting it now.
> 
> I tried to maintain authorship & Signoff except where I did some
> significant changes to the code/logic.

So, I applied patches 1-6 to for-next to make some progress.

The others need more thought because of the bindings which shall be
discussed replying to the patches in question.

Thanks for the updated work with lots of proper references.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[Vaibhav Hiremath]

On Tuesday 14 July 2015 05:04 PM, Wolfram Sang wrote:
> On Tue, Jul 14, 2015 at 01:06:39PM +0530, Vaibhav Hiremath wrote:
>> This patch series fixes bugs/warnings, cleans up the code and adds
>> support for PXA910 family of devices to PXA I2C bus driver.
>>
>> There has been one attempt made sometime back in 2012 to upstream
>> some of the patches from below list, but did not get follow up later.
>> I have consolidated all the patches, cleaned them up, splited into
>> logical changes, added new patches and submitting it now.
>>
>> I tried to maintain authorship & Signoff except where I did some
>> significant changes to the code/logic.
>
> So, I applied patches 1-6 to for-next to make some progress.
>
> The others need more thought because of the bindings which shall be
> discussed replying to the patches in question.
>
> Thanks for the updated work with lots of proper references.
>

OK, Thanks and no issues.

Lets discuss more on the bindings.

Thanks,
Vaibhav

Re: [PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

>> So, I applied patches 1-6 to for-next to make some progress.
>>
>> The others need more thought because of the bindings which shall be
>> discussed replying to the patches in question.
>>
>> Thanks for the updated work with lots of proper references.
>>
>
> OK, Thanks and no issues.
>
> Lets discuss more on the bindings.

I made a simple try on my reference platform with the whole patchset.
It oopses on a NULL dereference.

The stack is in [1].
I think it boils down to :
 - i2c_pxa_do_sclk_adj()
   - reg_ilcr = readl(_ILCR(i2c));

I also think the faulty patch is :
 - i2c: pxa: Add ILCR (tLow & tHigh) configuration support

My case, an I2C master case, I'd like you to find the issue and fix it.

Cheers.

-- 
Robert

[1] Stack
&Unable to handle kernel NULL pointer dereference at virtual address 00000000
&pgd = c0004000
"[00000000] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.2.0-rc2-next-20150713+ #705
Hardware name: MIO A701
task: c3856bc0 ti: c3858000 task.ti: c3858000
PC is at i2c_pxa_reset+0x114/0x1cc
LR is at i2c_pxa_probe+0x35c/0x428
pc : [<c02ee4b0>]    lr : [<c02ef080>]    psr: 68000053
sp : c3859d50  ip : c3859d70  fp : c3859d6c
r10: c38f75ac  r9 : c06c7668  r8 : 00000000
r7 : c394a300  r6 : c06c42f0  r5 : c38f7410  r4 : 00000002
r3 : 000087a0  r2 : f2301690  r1 : 00000000  r0 : 00000000
Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: 0000397f  Table: a0004000  DAC: 00000017
Process swapper (pid: 1, stack limit = 0xc3858198)
Stack: (0xc3859d50 to 0xc385a000)
9d40:                                     00004080 c38f7410 c06c42e0 c06c42f0
9d60: c3859dbc c3859d70 c02ef080 c02ee3a8 00004080 c38d7b40 c38f7410 c3859d88
9d80: c014eb08 c04d1878 00000001 00000000 00000000 c06c42f0 c06c42f0 c06f10c8
9da0: fffffdfb 00000000 c0678550 00000000 c3859ddc c3859dc0 c0280b18 c02eed30
9dc0: c06c42f0 c0731330 c0705098 c06f10c8 c3859e0c c3859de0 c027e850 c0280ad0
9de0: c3859e0c c3859df0 c06c42f0 c06f10c8 c06c4324 00000000 c0704fc0 c0678550
9e00: c3859e2c c3859e10 c027eb64 c027e6c8 00000000 00000000 c06f10c8 c027eaec
9e20: c3859e54 c3859e30 c027c5c4 c027eaf8 c389b96c c38dc6d0 c3940c78 c06f10c8
9e40: c39408e0 c06ec778 c3859e64 c3859e58 c027e228 c027c56c c3859e94 c3859e68
9e60: c027dbc8 c027e20c c05803d0 c394a3e0 c3859e94 c06f10c8 c06c2380 c394a3e0
9e80: c069a888 00000000 c3859eac c3859e98 c027f6d4 c027dac0 c06c2380 c06c2380
9ea0: c3859ebc c3859eb0 c0280a38 c027f630 c3859ecc c3859ec0 c069a8a0 c02809ec
9ec0: c3859f4c c3859ed0 c000975c c069a894 c3859efc c3859ee0 c3859efc c3859ee8
9ee0: c00389e0 c0201e98 00000000 c3ffcbe1 c3859f4c c3859f00 c0038c00 c00389cc
9f00: c004710c c003eea8 00000000 00000004 00000004 c3ffcbfd c0615b48 c057cd28
9f20: 00000000 00000004 c070f7a0 00000004 c070f7a0 c06bd020 c070f7a0 c06a64b0
9f40: c3859f94 c3859f50 c0678e3c c000963c 00000004 00000004 00000000 c0678550
9f60: c0497ffc 00000085 00000000 00000000 c0497ffc 00000000 00000000 00000000
9f80: 00000000 00000000 c3859fac c3859f98 c0498014 c0678d44 c3858000 00000000
9fa0: 00000000 c3859fb0 c000a5f0 c0498008 00000000 00000000 00000000 00000000
9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 b3ff8c5d 4cc3d76f
[<c02ee4b0>] (i2c_pxa_reset) from [<c02ef080>] (i2c_pxa_probe+0x35c/0x428)
[<c02ef080>] (i2c_pxa_probe) from [<c0280b18>] (platform_drv_probe+0x54/0xb0)
[<c0280b18>] (platform_drv_probe) from [<c027e850>] (driver_probe_device+0x194/0x430)
[<c027e850>] (driver_probe_device) from [<c027eb64>] (__driver_attach+0x78/0x9c)
[<c027eb64>] (__driver_attach) from [<c027c5c4>] (bus_for_each_dev+0x64/0xb4)
[<c027c5c4>] (bus_for_each_dev) from [<c027e228>] (driver_attach+0x28/0x30)
[<c027e228>] (driver_attach) from [<c027dbc8>] (bus_add_driver+0x114/0x274)
[<c027dbc8>] (bus_add_driver) from [<c027f6d4>] (driver_register+0xb0/0xf8)
[<c027f6d4>] (driver_register) from [<c0280a38>] (__platform_driver_register+0x58/0x6c)
[<c0280a38>] (__platform_driver_register) from [<c069a8a0>] (i2c_adap_pxa_init+0x18/0x20)
[<c069a8a0>] (i2c_adap_pxa_init) from [<c000975c>] (do_one_initcall+0x12c/0x220)
[<c000975c>] (do_one_initcall) from [<c0678e3c>] (kernel_init_freeable+0x104/0x1d8)
[<c0678e3c>] (kernel_init_freeable) from [<c0498014>] (kernel_init+0x18/0xfc)
[<c0498014>] (kernel_init) from [<c000a5f0>] (ret_from_fork+0x14/0x24)
Code: 03a00000 e1803003 e5823000 e5950318 (e5903000) 
---[ end trace 76138ae455db32c0 ]---

[2] Disassembly of the i2c_pxa_reset() and its inlined i2c_pxa_do_sclk_adj()
512	
513	static void i2c_pxa_do_sclk_adj(struct pxa_i2c *i2c)
514	{
515		unsigned int reg_ilcr;
516	
517		reg_ilcr = readl(_ILCR(i2c));
   0xc02ee4ac <+272>:	ldr	r0, [r5, #792]	; 0x318

518	
519		/* For standard/fast mode tlow and thigh counters are same */
520		if (i2c->sclk_tlow_load_cnt) {
   0xc02ee4b4 <+280>:	ldr	r12, [r5, #828]	; 0x33c
   0xc02ee4b8 <+284>:	cmp	r12, #0
   0xc02ee4bc <+288>:	beq	0xc02ee4fc <i2c_pxa_reset+352>

Re: [PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[Vaibhav Hiremath]

On Saturday 18 July 2015 01:19 AM, Robert Jarzmik wrote:
> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>
>>> So, I applied patches 1-6 to for-next to make some progress.
>>>
>>> The others need more thought because of the bindings which shall be
>>> discussed replying to the patches in question.
>>>
>>> Thanks for the updated work with lots of proper references.
>>>
>>
>> OK, Thanks and no issues.
>>
>> Lets discuss more on the bindings.
>
> I made a simple try on my reference platform with the whole patchset.
> It oopses on a NULL dereference.
>
> The stack is in [1].
> I think it boils down to :
>   - i2c_pxa_do_sclk_adj()
>     - reg_ilcr = readl(_ILCR(i2c));
>
> I also think the faulty patch is :
>   - i2c: pxa: Add ILCR (tLow & tHigh) configuration support
>
> My case, an I2C master case, I'd like you to find the issue and fix it.
>

Which is this reference platform?
Can you share few details -

  - reference Platform?
  - DT file if you could
  - Boot log (if you could)


I am using pxa1928 based platform, and I do not see any issues.

Thanks,
Vaibhav

Re: [PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[Vaibhav Hiremath]

On Monday 20 July 2015 12:36 PM, Vaibhav Hiremath wrote:
>
>
> On Saturday 18 July 2015 01:19 AM, Robert Jarzmik wrote:
>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>>
>>>> So, I applied patches 1-6 to for-next to make some progress.
>>>>
>>>> The others need more thought because of the bindings which shall be
>>>> discussed replying to the patches in question.
>>>>
>>>> Thanks for the updated work with lots of proper references.
>>>>
>>>
>>> OK, Thanks and no issues.
>>>
>>> Lets discuss more on the bindings.
>>
>> I made a simple try on my reference platform with the whole patchset.
>> It oopses on a NULL dereference.
>>
>> The stack is in [1].
>> I think it boils down to :
>>   - i2c_pxa_do_sclk_adj()
>>     - reg_ilcr = readl(_ILCR(i2c));
>>
>> I also think the faulty patch is :
>>   - i2c: pxa: Add ILCR (tLow & tHigh) configuration support
>>
>> My case, an I2C master case, I'd like you to find the issue and fix it.
>>
>
> Which is this reference platform?
> Can you share few details -
>
>   - reference Platform?
>   - DT file if you could
>   - Boot log (if you could)
>
>
> I am using pxa1928 based platform, and I do not see any issues.
>

Having said that,
I see issues in the patch for non PXA910 platform, where
i2c_pxa_do_sclk_adj() will be called unconditionally and obviously
reg_ilcr and reg_wcr are not set.

I will fix this and send the patch.

Thanks,
Vaibhav

Re: [PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[Vaibhav Hiremath]

On Monday 20 July 2015 12:39 PM, Vaibhav Hiremath wrote:
>
>
> On Monday 20 July 2015 12:36 PM, Vaibhav Hiremath wrote:
>>
>>
>> On Saturday 18 July 2015 01:19 AM, Robert Jarzmik wrote:
>>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>>>
>>>>> So, I applied patches 1-6 to for-next to make some progress.
>>>>>
>>>>> The others need more thought because of the bindings which shall be
>>>>> discussed replying to the patches in question.
>>>>>
>>>>> Thanks for the updated work with lots of proper references.
>>>>>
>>>>
>>>> OK, Thanks and no issues.
>>>>
>>>> Lets discuss more on the bindings.
>>>
>>> I made a simple try on my reference platform with the whole patchset.
>>> It oopses on a NULL dereference.
>>>
>>> The stack is in [1].
>>> I think it boils down to :
>>>   - i2c_pxa_do_sclk_adj()
>>>     - reg_ilcr = readl(_ILCR(i2c));
>>>
>>> I also think the faulty patch is :
>>>   - i2c: pxa: Add ILCR (tLow & tHigh) configuration support
>>>
>>> My case, an I2C master case, I'd like you to find the issue and fix it.
>>>
>>
>> Which is this reference platform?
>> Can you share few details -
>>
>>   - reference Platform?
>>   - DT file if you could
>>   - Boot log (if you could)
>>
>>
>> I am using pxa1928 based platform, and I do not see any issues.
>>
>
> Having said that,
> I see issues in the patch for non PXA910 platform, where
> i2c_pxa_do_sclk_adj() will be called unconditionally and obviously
> reg_ilcr and reg_wcr are not set.
>
> I will fix this and send the patch.
>

This should fix the issue -


hvaibhav@hvaibhav-ThinkPad-T440p:~/projects/mainline/linux$ git diff 
drivers/i2c/busses/i2c-pxa.c
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index e0aa087..9e372fc 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -590,6 +590,9 @@ static void i2c_pxa_do_sclk_adj(struct pxa_i2c *i2c)
  {
         unsigned int reg_ilcr;

+       if (!i2c->reg_ilcr)
+               return;
+
         reg_ilcr = readl(_ILCR(i2c));

         /* For standard/fast mode tlow and thigh counters are same */



If you are ok, I will re-spin the patch and submit.

Thanks,
Vaibhav

Re: [PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family

[Robert Jarzmik]

Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:

> On Monday 20 July 2015 12:39 PM, Vaibhav Hiremath wrote:
>>
>>
>> On Monday 20 July 2015 12:36 PM, Vaibhav Hiremath wrote:
>>>
>>>
>>> On Saturday 18 July 2015 01:19 AM, Robert Jarzmik wrote:
>>>> Vaibhav Hiremath <vaibhav.hiremath@linaro.org> writes:
>>>>
>>>>>> So, I applied patches 1-6 to for-next to make some progress.
>>>>>>
>>>>>> The others need more thought because of the bindings which shall be
>>>>>> discussed replying to the patches in question.
>>>>>>
>>>>>> Thanks for the updated work with lots of proper references.
>>>>>>
>>>>>
>>>>> OK, Thanks and no issues.
>>>>>
>>>>> Lets discuss more on the bindings.
>>>>
>>>> I made a simple try on my reference platform with the whole patchset.
>>>> It oopses on a NULL dereference.
>>>>
>>>> The stack is in [1].
>>>> I think it boils down to :
>>>>   - i2c_pxa_do_sclk_adj()
>>>>     - reg_ilcr = readl(_ILCR(i2c));
>>>>
>>>> I also think the faulty patch is :
>>>>   - i2c: pxa: Add ILCR (tLow & tHigh) configuration support
>>>>
>>>> My case, an I2C master case, I'd like you to find the issue and fix it.
>>>>
>>>
>>> Which is this reference platform?
>>> Can you share few details -
>>>
>>>   - reference Platform?
Mitac Mio A701, pxa270.

>>>   - DT file if you could
None, ran in platform_data mode.

>>>   - Boot log (if you could)
You already found the issue, no need for that anymore.

>>> I am using pxa1928 based platform, and I do not see any issues.

>> Having said that,
>> I see issues in the patch for non PXA910 platform, where
>> i2c_pxa_do_sclk_adj() will be called unconditionally and obviously
>> reg_ilcr and reg_wcr are not set.
Yep.

>> I will fix this and send the patch.
>
> This should fix the issue -
Most probably yes.

> If you are ok, I will re-spin the patch and submit.
Yes, I'm ok, let's try the next iteration.

Cheers.

-- 
Robert