Re: [PATCH-v5 5/5] i2c: pxa: Add ILCR (tLow tHigh) configuration support
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes: Robert, It would be helpful if you can test this patch-series and confirm that it now fixes the NULL pointer deference issue. Tested, it works on pxa27x in master mode, in non-DT mode. For all non-DT patches, you can add my : Tested-by: Robert Jarzmik robert.jarz...@free.fr Cheers. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v5 5/5] i2c: pxa: Add ILCR (tLow tHigh) configuration support
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes: Robert, It would be helpful if you can test this patch-series and confirm that it now fixes the NULL pointer deference issue. Thanks, Vaibhav Hi Vaibhav, My next slot is probably this comming Sunday. I'll do the test and report. Cheers. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v4 00/11] i2c: pxa: Fixes, cleanup and support for pxa910 family
Vaibhav Hiremath vaibhav.hirem...@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
pgd = c0004000
[] *pgd=
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: 6853
sp : c3859d50 ip : c3859d70 fp : c3859d6c
r10: c38f75ac r9 : c06c7668 r8 :
r7 : c394a300 r6 : c06c42f0 r5 : c38f7410 r4 : 0002
r3 : 87a0 r2 : f2301690 r1 : r0 :
Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment kernel
Control: 397f Table: a0004000 DAC: 0017
Process swapper (pid: 1, stack limit = 0xc3858198)
Stack: (0xc3859d50 to 0xc385a000)
9d40: 4080 c38f7410 c06c42e0 c06c42f0
9d60: c3859dbc c3859d70 c02ef080 c02ee3a8 4080 c38d7b40 c38f7410 c3859d88
9d80: c014eb08 c04d1878 0001 c06c42f0 c06c42f0 c06f10c8
9da0: fdfb c0678550 c3859ddc c3859dc0 c0280b18 c02eed30
9dc0: c06c42f0 c0731330 c0705098 c06f10c8 c3859e0c c3859de0 c027e850 c0280ad0
9de0: c3859e0c c3859df0 c06c42f0 c06f10c8 c06c4324 c0704fc0 c0678550
9e00: c3859e2c c3859e10 c027eb64 c027e6c8 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 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 c3ffcbe1 c3859f4c c3859f00 c0038c00 c00389cc
9f00: c004710c c003eea8 0004 0004 c3ffcbfd c0615b48 c057cd28
9f20: 0004 c070f7a0 0004 c070f7a0 c06bd020 c070f7a0 c06a64b0
9f40: c3859f94 c3859f50 c0678e3c c000963c 0004 0004 c0678550
9f60: c0497ffc 0085 c0497ffc
9f80: c3859fac c3859f98 c0498014 c0678d44 c3858000
9fa0: c3859fb0 c000a5f0 c0498008
9fc0:
9fe0: 0013 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: 03a0 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
Re: [PATCH-V2 07/12] i2c:pxa: Use devm_ variants in probe function
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes:
@@ -1201,16 +1201,17 @@ 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;
+ dev_err(dev-dev, failed to get the clk\n);
For consistency's sake, please use :
dev_err(dev-dev, failed to get the clk: %ld\n,
PTR_ERR(i2c-clk));
Once that is done, you can add my :
Acked-by: Robert Jarzmik robert.jarz...@free.fr
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-V2 08/12] i2c: pxa: enable/disable i2c module across msg xfer
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes:
#define _IBMR(i2c) ((i2c)-reg_ibmr)
@@ -286,6 +287,22 @@ 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 !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);
@@ -482,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);
}
@@ -840,6 +856,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
@@ -860,6 +879,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;
}
@@ -1075,6 +1099,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);
Okay, what happens in master mode when we get there on the 2nd xfer :
- i2c is enabled AFAIU.
- as a consequence i2c_pxa_is_enabled() returns true
- as a consequence, i2c_pxa_enable() _disables_ the i2c, right ?
- as a consequence i2c is broken
Is this correct, because if it is the patch needs a rework ?
Cheers.
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-V2 11/12] i2c: pxa: Add ILCR (tLow tHigh) configuration support
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes:
+ i2c-clk = devm_clk_get(dev-dev, NULL);
+ if (IS_ERR(i2c-clk)) {
+ dev_err(dev-dev, failed to get the clk\n);
+ dev_err(dev-dev, failed to get the clk: %ld\n,
PTR_ERR(i2c-clk));
+ return PTR_ERR(i2c-clk);
+ }
Why is this block moving up ? I can't find the reason in the commit message or
in the code.
Cheers.
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-V2 04/12] i2c: pxa: Reset i2c controller on timeout in interrupt and pio mode
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes: In case of timeout during msg xfer assert reset to i2c controller for both interrupt and PIO mode of operation. Signed-off-by: Jett.Zhou jtz...@marvell.com [vaibhav.hirem...@linaro.org: Split merge patches into logical changes and update the Changelog] Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org I already said it previously, I'm against an unconditional reset in a timeout path. Make it a quirk or whatever, but in the current status, I'm against. The previous behavior looks correct to me : upon timeout, retry, no need to reset. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-V2 08/12] i2c: pxa: enable/disable i2c module across msg xfer
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes: I have taken care of all comments on the 3 patches. Just in case if you have any comments on other patches in series, I will wait for a day before pushing next version. That would be great, yeah. I'll make another pass tomorrow. If that works for you, and if you release your next version before Sunday morning (french local time), I'll make another review for Sunday evening of all the patches. Cheers. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-V2 02/12] i2c: pxa: No need to set slave addr for i2c master mode reset
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes: 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. Signed-off-by: Jett.Zhou jtz...@marvell.com Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org Cc: Wolfram Sang w...@the-dreams.de Acked-by: Robert Jarzmik robert.jarz...@free.fr Cheers. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-V2 03/12] i2c: pxa: Return I2C_RETRY when timeout in pio mode
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes: From: Shouming Wang wang...@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 wang...@marvell.com [vaibhav.hirem...@linaro.org: Updated changelog] Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org Acked-by: Robert Jarzmik robert.jarz...@free.fr Cheers. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/12] i2c:pxa: Use devm_ variants in probe function
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes:
@@ -1270,10 +1270,17 @@ static int i2c_pxa_probe(struct platform_device *dev)
struct resource *res = NULL;
int ret, irq;
- i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
+ i2c = devm_kzalloc(dev-dev, sizeof(struct pxa_i2c), GFP_KERNEL);
if (!i2c) {
- ret = -ENOMEM;
- goto emalloc;
+ dev_err(dev-dev, memory allocation failed\n);
+ return -ENOMEM;
+ }
+
+ res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+ irq = platform_get_irq(dev, 0);
+ if (res == NULL || irq 0) {
+ dev_err(dev-dev, no mem/irq resource\n);
+ return -ENODEV;
I'd like to have the error code here, as in other part of the driver.
Ie. I'd want :
dev_err(dev-dev, no mem/irq resource: %d\n, irq);
Moreover, return -ENODEV if res == NULL, but return irq if irq 0 (think of
-EPROBE_DEFER).
+ i2c-reg_base = devm_ioremap_resource(dev-dev, res);
if (!i2c-reg_base) {
if (IS_ERR(i2c-reg_base)) instead.
- ret = -EIO;
- goto eremap;
+ dev_err(dev-dev, failed to map resource\n);
+ return -EIO;
Ditto, ie. include PTR_ERR(i2c-reg_base) in the dev_err(), and return
PTR_ERR(i2c-reg_base);
@@ -1361,11 +1356,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\n);
Ditto for the dev_err() and error code reporting.
@@ -1380,8 +1377,8 @@ 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\n);
Ditto.
@@ -1411,26 +1408,15 @@ static int i2c_pxa_probe(struct platform_device *dev)
}
}
#ifdef CONFIG_I2C_PXA_SLAVE
- printk(KERN_INFO I2C: %s: PXA I2C adapter, slave address %d\n,
+ pr_info(I2C: %s: PXA I2C adapter, slave address %d\n,
dev_name(i2c-adap.dev), i2c-slave_addr);
dev_info() maybe ?
#else
- printk(KERN_INFO I2C: %s: PXA I2C adapter\n,
-dev_name(i2c-adap.dev));
+ pr_info(I2C: %s: PXA I2C adapter\n, dev_name(i2c-adap.dev));
Ditto.
Cheers.
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/12] i2c: pxa: Add reset operation when i2c bus busy
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes:
From: Jett.Zhou jtz...@marvell.com
According to some test in emei_dkb, we found some i2c slave device
(eg. camera sensor ov2659 power up) introduce noise on sda, so detect
i2c controller busy, and assert reset to i2c controller to recover as
early as possible to avoid more latency on the entire i2c transaction.
Signed-off-by: Jett.Zhou jtz...@marvell.com
[vaibhav.hirem...@linaro.org: Removed reduction in timeout value, as I
do not have goot explanation for it. Logically it is not required.
And also Updated changelog]
Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org
Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org
---
drivers/i2c/busses/i2c-pxa.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index d4c798a..a76c901 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -314,6 +314,10 @@ static int i2c_pxa_wait_bus_not_busy(struct pxa_i2c *i2c)
{
int timeout = DEF_TIMEOUT;
+ if (readl(_ISR(i2c)) (ISR_IBB | ISR_UB))
+ i2c_pxa_reset(i2c);
The pxa27x manual states in the Developer Manual, chapter 9.4.13 Reset
Conditions :
Software must ensure that (1) the I 2 C unit is not busy before it
asserts a reset
Given that, I don't agree with this patch. Moreover, reseting unconditionaly the
i2c bus on each busy state on a write transaction for one single corner case is
not something that has my agreement. A quirk might overcome my reluctance.
Cheers.
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/12] i2c: pxa: Return I2C_RETRY when timeout in pio mode
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes:
From: Shouming Wang wang...@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 wang...@marvell.com
[vaibhav.hirem...@linaro.org: Updated changelog]
Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org
Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org
---
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 eb09071..2777d5c 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -841,8 +841,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;
+ }
Ok, looks good to me.
As it changes the dynamic behavior of i2c_pxa_pio_xfer(), I'd like to know how
it was tested (on which platform, and what was on the I2C bus).
Cheers.
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] i2c: pxa: Add support for pxa910/988 new configuration features
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes:
@@ -167,6 +184,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
@@ -467,11 +486,16 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c)
if (i2c-reg_isar)
writel(i2c-slave_addr, _ISAR(i2c));
#endif
-
Not in this patch.
/* set control register values */
writel(I2C_ICR_INIT | (i2c-fast_mode ? ICR_FM : 0), _ICR(i2c));
writel(readl(_ICR(i2c)) | (i2c-high_mode ? ICR_HS : 0), _ICR(i2c));
+ if (i2c-ilcr)
+ writel(i2c-ilcr, _ILCR(i2c));
+ if (i2c-iwcr)
+ writel(i2c-iwcr, _IWCR(i2c));
+ udelay(2);
This is a magical 2us. Where does it come from ?
Cheers.
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/12] i2c: pxa: Add bus reset functionality
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes:
From: Rob Herring r...@kernel.org
Since there is some problematic i2c slave devices on some
platforms such as dkb (sometimes), it will drop down sda
and make i2c bus hang, at that time, it need to config
scl/sda into gpio to simulate stop sequence to recover
i2c bus, so add this interface.
Signed-off-by: Leilei Shang shan...@marvell.com
Signed-off-by: Rob Herring r...@kernel.org
[vaibhav.hirem...@linaro.org: Updated Changelog]
Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org
Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org
I'm not enthusiastic about this patch.
Linus, would you please place a comment on this patch ?
My personal feeling :
- all the udelays are just ugly
- the pinctrl binding into i2c-pxa doesn't look good to me
Linus will probably comment on that
- all of this patch looks like a workaround for a single platform poor
support. If this is dkb specific (what is dkb BTW ?), can't it be hooked
into this driver, but placed in dkb platform code ?
Cheers.
--
Robert
---
drivers/i2c/busses/i2c-pxa.c | 90
1 file changed, 90 insertions(+)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 8ca5552..eb09071 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -37,6 +37,8 @@
#include linux/slab.h
#include linux/io.h
#include linux/i2c/pxa-i2c.h
+#include linux/of_gpio.h
+#include linux/pinctrl/consumer.h
#include asm/irq.h
@@ -177,6 +179,9 @@ struct pxa_i2c {
boolhighmode_enter;
unsigned intilcr;
unsigned intiwcr;
+ struct pinctrl *pinctrl;
+ struct pinctrl_state*pin_i2c;
+ struct pinctrl_state*pin_gpio;
};
#define _IBMR(i2c) ((i2c)-reg_ibmr)
@@ -269,6 +274,62 @@ static void i2c_pxa_show_state(struct pxa_i2c *i2c, int
lno, const char *fname)
#define show_state(i2c) i2c_pxa_show_state(i2c, __LINE__, __func__)
+static void i2c_bus_reset(struct pxa_i2c *i2c)
+{
+ int ret, ccnt, pins_scl, pins_sda;
+ struct device *dev = i2c-adap.dev.parent;
+ struct device_node *np = dev-of_node;
+
+ if (!i2c-pinctrl) {
+ dev_warn(dev, could not do i2c bus reset\n);
+ return;
+ }
+
+ ret = pinctrl_select_state(i2c-pinctrl, i2c-pin_gpio);
+ if (ret) {
+ dev_err(dev, could not set gpio pins\n);
+ return;
+ }
+
+ pins_scl = of_get_named_gpio(np, i2c-gpio, 0);
+ if (!gpio_is_valid(pins_scl)) {
+ dev_err(dev, i2c scl gpio not set\n);
+ goto err_out;
+ }
+ pins_sda = of_get_named_gpio(np, i2c-gpio, 1);
+ if (!gpio_is_valid(pins_sda)) {
+ dev_err(dev, i2c sda gpio not set\n);
+ goto err_out;
+ }
+
+ gpio_request(pins_scl, NULL);
+ gpio_request(pins_sda, NULL);
+
+ gpio_direction_input(pins_sda);
+ for (ccnt = 20; ccnt; ccnt--) {
+ gpio_direction_output(pins_scl, ccnt 0x01);
+ udelay(5);
+ }
+ gpio_direction_output(pins_scl, 0);
+ udelay(5);
+ gpio_direction_output(pins_sda, 0);
+ udelay(5);
+ /* stop signal */
+ gpio_direction_output(pins_scl, 1);
+ udelay(5);
+ gpio_direction_output(pins_sda, 1);
+ udelay(5);
+
+ gpio_free(pins_scl);
+ gpio_free(pins_sda);
+
+err_out:
+ ret = pinctrl_select_state(i2c-pinctrl, i2c-pin_i2c);
+ if (ret)
+ dev_err(dev, could not set default(i2c) pins\n);
+ return;
+}
+
static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why)
{
unsigned int i;
@@ -281,6 +342,11 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c
*i2c, const char *why)
for (i = 0; i i2c-irqlogidx; i++)
printk([%08x:%08x] , i2c-isrlog[i], i2c-icrlog[i]);
printk(\n);
+ if (strcmp(why, exhausted retries) != 0) {
+ i2c_bus_reset(i2c);
+ /* reset i2c contorler when it's fail */
+ i2c_pxa_reset(i2c);
+ }
}
#else /* ifdef DEBUG */
@@ -1301,6 +1367,30 @@ static int i2c_pxa_probe(struct platform_device *dev)
platform_set_drvdata(dev, i2c);
+ i2c-pinctrl = devm_pinctrl_get(dev-dev);
+ if (IS_ERR(i2c-pinctrl)) {
+ i2c-pinctrl = NULL;
+ dev_warn(dev-dev, could not get pinctrl\n);
+ } else {
+ i2c-pin_i2c = pinctrl_lookup_state(i2c-pinctrl, default);
+ if (IS_ERR(i2c-pin_i2c)) {
+ dev_err(dev-dev, could not get default(i2c)
pinstate\n);
+ ret = IS_ERR(i2c-pin_i2c);
+ }
+
+ i2c-pin_gpio = pinctrl_lookup_state(i2c-pinctrl, gpio);
+ if (IS_ERR(i2c-pin_gpio)) {
+ dev_err(dev-dev, could
Re: [PATCH 02/12] i2c: pxa: No need to set slave addr for i2c master mode reset
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes: diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c index f4ac8c5..d4c798a 100644 --- a/drivers/i2c/busses/i2c-pxa.c +++ b/drivers/i2c/busses/i2c-pxa.c @@ -459,8 +459,10 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c) writel(I2C_ISR_INIT, _ISR(i2c)); writel(readl(_ICR(i2c)) ~ICR_UR, _ICR(i2c)); +#ifdef CONFIG_I2C_PXA_SLAVE if (i2c-reg_isar) writel(i2c-slave_addr, _ISAR(i2c)); +#endif I'd rather have : if (i2c-reg_isar IS_ENABLED(CONFIG_I2C_PXA_SLAVE)) writel(i2c-slave_addr, _ISAR(i2c)); Cheers. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/12] i2c: pxa: Reset i2c controller on timeout in interrupt and pio mode
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes: In case of timeout during msg xfer assert reset to i2c controller for both interrupt and PIO mode of operation. Signed-off-by: Jett.Zhou jtz...@marvell.com [vaibhav.hirem...@linaro.org: Split merge patches into logical changes and update the Changelog] Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org I have the same comment as before. I don't like a reset in the transfer path, especially in normal busy phases for slow I2C devices. A quirk as before. Cheers. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/12] i2c: pxa: Remove compile warnning in 64bit mode
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes: From: Yipeng Yao yp...@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 yp...@marvell.com [vaibhav.hirem...@linaro.org: Updated Changelog] Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org Cc: Wolfram Sang w...@the-dreams.de Acked-by: Robert Jarzmik robert.jarz...@free.fr Cheers. -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/12] i2c: pxa: Update debug function to dump more info on error
Vaibhav Hiremath vaibhav.hirem...@linaro.org writes:
Update i2c_pxa_scream_blue_murder() fn to print more information
in case of error.
Signed-off-by: Jett.Zhou jtz...@marvell.com
[vaibhav.hirem...@linaro.org: Split patches into logical changes
and update the Changelog]
Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org
Cc: Wolfram Sang w...@the-dreams.de
Signed-off-by: Vaibhav Hiremath vaibhav.hirem...@linaro.org
---
drivers/i2c/busses/i2c-pxa.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index cf6c383..065e647 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -147,6 +147,7 @@ struct pxa_i2c {
unsigned intmsg_idx;
unsigned intmsg_ptr;
unsigned intslave_addr;
+ unsigned intreq_slave_addr;
struct i2c_adapter adap;
struct clk *clk;
@@ -335,11 +336,13 @@ err_out:
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_ERRi2c: %s slave_0x%x error: %s\n, i2c-adap.name,
+ i2c-req_slave_addr 1, why);
Why not simply use dev_err() instead of adding manually i2c-adap.name ?
Cheers.
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 15/17] ARM: pxa: poodle: don't preallocate IRQ space for locomo
Dmitry Eremin-Solenikov dbarysh...@gmail.com writes: As new locomo driver supports SPARSE_IRQ, don't preallocate NR_IRQS space for it on poodle. Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com Acked-by: Robert Jarzmik robert.jarz...@free.fr -- Robert -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp
Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:
Change clk_enable/disable() calls to clk_prepare_enable() and
clk_disable_unprepare().
Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com
---
drivers/usb/gadget/udc/pxa25x_udc.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c
b/drivers/usb/gadget/udc/pxa25x_udc.c
index 42f7eeb..e4964ee 100644
--- a/drivers/usb/gadget/udc/pxa25x_udc.c
+++ b/drivers/usb/gadget/udc/pxa25x_udc.c
@@ -103,8 +103,8 @@ static const char ep0name [] = ep0;
/* IXP doesn't yet support linux/clk.h */
#define clk_get(dev,name)NULL
-#define clk_enable(clk) do { } while (0)
-#define clk_disable(clk) do { } while (0)
+#define clk_prepare_enable(clk) do { } while (0)
+#define clk_disable_unprepare(clk) do { } while (0)
#define clk_put(clk) do { } while (0)
#endif
@@ -932,7 +932,7 @@ static int pullup(struct pxa25x_udc *udc)
if (!udc-active) {
udc-active = 1;
/* Enable clock for USB device */
- clk_enable(udc-clk);
+ clk_prepare_enable(udc-clk);
Guess what, Russell's remark on i2c and serial made me cross-check. And there
is a case where this will be called in irq context too ...
See :
- do_IRQ
- lubbock_vbus_irq()
- pxa25x_udc_vbus_session()
- pullup()
- clk_prepare_enable()
- CRACK
Note that your patch is not really the faulty one, I think a threaded irq
instead of the raw irq should do the trick. And it is granted on UDC api that
vbus function is called in a sleeping context (Felipe correct me if I'm
wrong), so a patch to fix this before your current code would be fine.
Cheers.
--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
