Re: [PATCH] i2c: designware: use enable on resume instead initialization
Dear Lucas, Lucas De Marchi lucas.de.mar...@gmail.com wrote on 23.06.2015 19:02:03: On Tue, Jun 23, 2015 at 1:45 PM, christian.rupp...@alitech.com wrote: Hello, Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16: [...] The result is not very encouraging: Out of five (identical) designware i2c controllers we have on my test SOC, the first one initialises properly but the second one gets stuck in the famous irq loop right away when the module is enabled in i2c_dw_init. The system never gets around to try Are you using the pci or platform driver? I noticed yesterday the pci version is failing here with a NULL pointer dereference. The test was performed with the platform driver (instantiated through device tree). I just re-checked and the ultimate problem which hangs/kills the system in my case is the IRQ loop. I haven't observed any NULL pointer dereferences on the road. Greetings, Christian -- 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] i2c: designware: use enable on resume instead initialization
Dear Lucas, De Marchi, Lucas lucas.demar...@intel.com wrote on 24.06.2015 14:56:19: On Wed, 2015-06-24 at 14:27 +0300, Mika Westerberg wrote: On Wed, Jun 24, 2015 at 09:36:43AM +0200, christian.rupp...@alitech.com wrot e: Dear Lucas, Lucas De Marchi lucas.de.mar...@gmail.com wrote on 23.06.2015 19:02:03: On Tue, Jun 23, 2015 at 1:45 PM, christian.rupp...@alitech.com wrote: Hello, Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16: [...] The result is not very encouraging: Out of five (identical) designware i2c controllers we have on my test SOC, the first one initialises properly but the second one gets stuck in the famous irq loop right away when the module is enabled in i2c_dw_init. The system never gets around to try Are you using the pci or platform driver? I noticed yesterday the pci version is failing here with a NULL pointer dereference. The test was performed with the platform driver (instantiated through device tree). I just re-checked and the ultimate problem which hangs/kills thesystem in my case is the IRQ loop. I haven't observed any NULL pointer dereferences on the road. Thanks Christian for testing. Since the patch causes problems on your hardware, I don't think it is good idea to merge it. Yeah, but it would be bad to ignore the problem as well. The way it is now kills any possibility of using DW controller for reading sensors like gyroscope, accelerometer, barometer that have higher sampling rate etc. I'll try to come up with a new patch but since I can't reproduce the problem here it'd be good to know if there's any means for me to test. I'll analyse the problem a bit further and send you a description (sda and scl state at enable time) as soon as I can put my hands on an oscilloscope one evening. Greetings, Christian -- 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] i2c: designware: use enable on resume instead initialization
Hello, Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16: Mika Westerberg mika.westerb...@linux.intel.com wrote on 10.06. 2015 09:07:22: On Tue, Jun 09, 2015 at 03:29:01PM -0300, Lucas De Marchi wrote: Hi Mika, On Tue, Jun 9, 2015 at 5:51 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Mon, Jun 08, 2015 at 02:50:28PM -0300, lucas.de.mar...@gmail.com wrote: From: Fabio Mello fabio.me...@intel.com According to documentation and tests, initialization is not necessary on module resume, since the controller keeps its state between disable/enable. Change the target address is also allowed. So, this patch replaces the initialization on module resume with a simple enable, and removes the (non required anymore) enables and disables. Signed-off-by: Fabio Mello fabio.me...@intel.com Signed-off-by: Lucas De Marchi lucas.demar...@intel.com --- These pictures explain a little more the consequence of letting the enable+disable in the code: http://pub.politreco.com/paste/TEK0011-before.jpg http://pub.politreco.com/paste/TEK0007-after.jpg The yellow line is a GPIO toggle in userspace to mark when we start and finish the i2c transactions. The blue line is the SCL in that i2c bus. Take a look on the huge pauses we have between any 2 transactions. These pauses are removed with this patch and we are able to read our sensor's values in 950usec rather than 5.24msec we had before. We are testing this using a Minnowboard Max that has a designware i2c controller. Did you test this on any other platform than Intel Baytrail? No. The only soc we have here with this controller is the Baytrail. My concern is that this patch might break some non-Intel platform. It would be nice if someone (Christian?) could try this out. Ouch, this one brings back painful memories. Take a look at patch 38d7fadef4973bb94e36897fcb6bb6a12fdd10c9 (https://git.kernel.org/ cgit/linux/kernel/git/torvalds/linux.git/commit/? id=38d7fadef4973bb94e36897fcb6bb6a12fdd10c9) for the context. In brief: - Before patch 38d7fade, the driver disabled the hardware after successful transfers. I do not know the reason for this and I cannot judge whether this is necessary or not. I thus decided not to modify this behaviour in patch 38d7fade. - After patch 38d7fade, the driver disabled the dw controller after all transfers, in particular in the case of unsuccessful transfers. This modification was necessary because of a race condition triggered by an i2c slave device which interrupted transfers in the middle. In this case, the dw controller (at least our version) seems to send spurious interrupts later if it is not disabled. The interrupt handler is not designed to be called on already aborted transfers, however, which leads to undesirable behaviour if the interrupt occurs at the wrong moment (system hangs in irq loop). I will try to dig out the test setup we used to validate the patch at the time but given the fact that this was two years ago this might take a little while. In the meantime, do you have any means to stress test the case of unexpected events on the bus (client aborts transfer, timeout etc.)? An alternative might be to only disable the controller in the case of errors and leave it active after successful transfers. We should understand why the controller was disabled after successful transfers in the first place, however. Maybe some quirk with older versions of the hardware? Mika, do you have any memories about this? As promised I tried to dig out the test setup we used to validate patch 38d7fade at the time but without success. (I half expected that after such a long time...) So I said to myself, let's give the patch a try nevertheless and see if it works in our system at least in the nominal case (no i2c bus errors). The result is not very encouraging: Out of five (identical) designware i2c controllers we have on my test SOC, the first one initialises properly but the second one gets stuck in the famous irq loop right away when the module is enabled in i2c_dw_init. The system never gets around to try initialising the remaining three controllers due to the irq loop. I didn't have the time to investigate the details yet but I suspect this is triggered by some nastily behaved device on the bus. For example, some of our external devices are notorious for keeping i2c lines tied to zero before being properly powered on/reset/initialised by their respective drivers (which in turn depend on the i2c master to be initialised first, obviously). I'll grab an oscilloscope and dump the waves to confirm this suspicion on the occasion. However, similar situations might occur in multi-master setups (which we don't have). I suspect the driver/hardware to also end up in the irq loop after loosing
Re: [PATCH v2] i2c: designware: Suppress error message if platform_get_irq() 0
Alexey Brodkin alexey.brod...@synopsys.com wrote on 03/09/2015 10:03:12
AM:
From: Alexey Brodkin alexey.brod...@synopsys.com
To: linux-i2c@vger.kernel.org,
Cc: linux-ker...@vger.kernel.org, Alexey Brodkin
alexey.brod...@synopsys.com, Vineet Gupta
vineet.gup...@synopsys.com, Christian Ruppert
christian.rupp...@abilis.com, Mika Westerberg
mika.westerb...@linux.intel.com, Wolfram Sang w...@the-dreams.de,
Andy Shevchenko andriy.shevche...@linux.intel.com
Date: 03/09/2015 10:03 AM
As discussed here https://lkml.org/lkml/2015/3/3/568 and here
https://lkml.org/lkml/2015/3/3/453 we're looking forward for
implementing warnings and errors outputs right in platform_get_irq()
instead of having all kind of different outputs in each and every driver
that uses platform_get_irq().
Signed-off-by: Alexey Brodkin abrod...@synopsys.com
Cc: Vineet Gupta vgu...@synopsys.com
Cc: Christian Ruppert christian.rupp...@abilis.com
Acked-by: Christian Ruppert christian.rupp...@alitech.com
Please note that my email address has changed and use the alitech one in
future. The abilis address might be switched off soon.
Cc: Mika Westerberg mika.westerb...@linux.intel.com
Cc: Wolfram Sang w...@the-dreams.de
Cc: Andy Shevchenko andriy.shevche...@linux.intel.com
---
drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/
i2c/busses/i2c-designware-platdrv.c
index c270f5f..fa4e2b5 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -143,10 +143,8 @@ static int dw_i2c_probe(struct platform_device
*pdev)
u32 clk_freq, ht = 0;
irq = platform_get_irq(pdev, 0);
- if (irq 0) {
- dev_err(pdev-dev, no irq resource?\n);
- return irq; /* -ENXIO */
- }
+ if (irq 0)
+ return irq;
dev = devm_kzalloc(pdev-dev, sizeof(struct dw_i2c_dev),
GFP_KERNEL);
if (!dev)
--
2.1.0
--
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] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
Hello Alexey,
Alexey Brodkin alexey.brod...@synopsys.com wrote on 03/03/2015 05:37:31
PM:
From:
Alexey Brodkin alexey.brod...@synopsys.com
To:
christian.rupp...@alitech.com christian.rupp...@alitech.com,
Cc:
mika.westerb...@linux.intel.com mika.westerb...@linux.intel.com,
linux-ker...@vger.kernel.org linux-ker...@vger.kernel.org,
vineet.gup...@synopsys.com vineet.gup...@synopsys.com, wsa@the-
dreams.de w...@the-dreams.de, andriy.shevche...@linux.intel.com
andriy.shevche...@linux.intel.com, linux-i2c@vger.kernel.org
linux-i2c@vger.kernel.org, christian.rupp...@abilis.com
christian.rupp...@abilis.com
Date:
03/03/2015 05:38 PM
Subject:
Re: [PATCH] i2c: designware: Suppress error message if
platform_get_irq() returns -EPROBE_DEFER
Hi Christian,
[...]
irq = platform_get_irq(pdev, 0);
if (irq 0) {
- dev_err(pdev-dev, no irq resource?\n);
+ if (irq != -EPROBE_DEFER)
+ dev_err(pdev-dev, no irq resource?\n);
Presented like this I wonder if this merits being a dev_err at all.
Wouldn't dev_dbg be more adequate? This might remove the need for the
condition and also avoid bothering everyone if something in the
platform
device structures or device tree is not right.
return irq; /* -ENXIO */
}
We've just had similar discussion related to DW APB UART with Andy here
https://lkml.org/lkml/2015/3/3/412
So yes probably we may safely remove error message from here completely.
I agree. Although you do have a point (in the other thread) when saying
this
kind of messages can be useful in some situations. The process of
debugging
device tree and platform device setup is definitely more painful for
drivers
which omit this type of messages completely. Andy's proposal of
centralising
this looks like a very good solution here (and on top of that removes many
useless strings from the kernel binary).
Greetings,
Christian
--
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] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
On 2015-03-03 16:27, Alexey Brodkin wrote:
There's no point in printing error message if platform_get_irq()
returns -EPROBE_DEFER because probe deferring subsystem already outputs
message in bootlog like this:
---8---
platform e001d000.i2c: Driver i2c_designware requests probe deferral
---8---
Moreover in case of probe deferral following message may mislead user:
---8---
i2c_designware e001d000.i2c: no irq resource?
---8---
even though it's expected that platform_get_irq() may return
-EPROBE_DEFER.
Signed-off-by: Alexey Brodkin abrod...@synopsys.com
Cc: Vineet Gupta vgu...@synopsys.com
Cc: Christian Ruppert christian.rupp...@abilis.com
Cc: Mika Westerberg mika.westerb...@linux.intel.com
Cc: Wolfram Sang w...@the-dreams.de
---
drivers/i2c/busses/i2c-designware-platdrv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index c270f5f..01c1b17 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -144,7 +144,8 @@ static int dw_i2c_probe(struct platform_device *pdev)
irq = platform_get_irq(pdev, 0);
if (irq 0) {
- dev_err(pdev-dev, no irq resource?\n);
+ if (irq != -EPROBE_DEFER)
+ dev_err(pdev-dev, no irq resource?\n);
Presented like this I wonder if this merits being a dev_err at all.
Wouldn't dev_dbg be more adequate? This might remove the need for the
condition and also avoid bothering everyone if something in the platform
device structures or device tree is not right.
return irq; /* -ENXIO */
}
--
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] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 2015-03-03 18:21, Wolfram Sang wrote: which omit this type of messages completely. Andy's proposal of centralising this looks like a very good solution here (and on top of that removes many useless strings from the kernel binary). I am all for centralizing printouts. I recommended this at my ELCE talk last year, too. However, you need to keep in mind that irqs are sometimes optional and you don't want error messages for those irqs. IMO worthwhile, but not a low hanging fruit... There is a lot of truth in that. Thus the initial dev_dbg() suggestion to go half way. I still think that Andy's proposal (or a variation thereof to catch the optional irqs case) should be the ultimate goal but I agree that this is more than a quick patch and that it's probably way out of scope here. -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.14 (GNU/Linux) iQIcBAEBAgAGBQJU9fONAAoJENOBSR1q+OdQLh0P/3f9NIBGyvpr3ao+UVhiW26f 8ikcd1lHezjGZuILPgsobb4bpePwyV0IYLoqediGtwOSlbe6IlxC5DTzwlcJhUe5 BDtker32yPTohA8uc27PFlnZ3secJ3xngmF43QwsP4waQxdLsufQ85YQ1/P4SXhQ gkxZpE1J1T4jVshtrf4lWlMJ3okdN2n/aFILRkNPCGc5QV9FjAMDwzYJLhBJw8wj 0Xe0X6zflgLlbUGfIeUIPoS7Stn9JtLZt/ltRAPBfFdKwBLUWOvmqGWl5XBcgzRi UCEEefQl7T/H5HXOEdUyLuCFNsa7rMLEGX8HVSpqb1516dMM4L4vQomISKQyW5ED SHLaCwzIsQTLdaY4gfNdKjJQMkuZSktz5zG8k3SRAR1PSqzNn7isUcc12HmoyRUd 2lONuN79r3z2iM591FofG69uC64RAJc5DHBj9gHbyvPwp48SMOQThO2vUunZvDoZ E9rfux3Dj0txpxeh4GqGUjtDXmIhcLm9uXMYQ2UBMH/vaz3adWcX3rH76MLTIZCS a0ilv+7Jr8VDgJcik2hiFIsSURSZqKSub6bXefvUYc8jOscREa0SnBQvGT3xkl+p n57+5d+lWpkT4a5LSVRqkhhAtG+g/Ti09YOfhR2Z3C2xvWxM7VyVbxY69UDPXW5Z sBDHtNkqCZUcx5wdkPec =cvA7 -END PGP SIGNATURE- -- 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: arc platform code updates (was Re: [PATCH 2/2] i2c: designware: Add support for AMD I2C controller)
On Tue, Oct 07, 2014 at 12:35:29PM +, Vineet Gupta wrote: +CC Guenter Roeck [...] However, the kernel-doc comment for init_machine in mach_desc.h is now slightly confusing (still mentioning device tree). A platform of future can still call of_platform_populate() etc to reparse the stuff for say it's platform devices ! So I would think it is still relevant ! OK. With this patch there remains only a single detail we need to manage through platform-specific code: the reset handler. Today we still provide a patch for the machine_restart function in reset.c to our customers so that rebooting from the command line works. Do you have any plans/ideas to fix this one as well? Patches are welcome ;-) ATM, I dont have a specific use-case for my current platforms, so can't write the code - you can propose a patch and then we can work out what's best in general for all ARC platforms. BTW there's a series in flight on related topic from Guenter so please take a look at that too for big picture ! http://www.spinics.net/linux/lists/kernel/msg1840650.html Actually, before sending my previous mail I looked at the current implementation of the halt hook and didn't like it (otherwise I would have proposed something in the lines). So this one is definitely a step forward! I'm wondering about two things concerning reset, however: 1. Is the PM module the right place for a reset handler? On the one hand reset is functionally very similar to power off but on the other hand reset is technically not a power management functionality. If the PM module is not the right place, which would be the right place instead? 2. What would be the desired behaviour/semantics for a reset handler chain equivalent to the power off handler chain. I see two possibilities here: a) Implementation exactly like power off. Every handler is expected to reset the entire system and never returns. b) A more modular implementation where different subsystems are being reset sequentially (e.g. first reset peripherals through GPIO in high priority handlers and finally reset the core in the terminal low priority handler). Greetings, Christian -- 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 2/2] i2c: designware: Add support for AMD I2C controller
Dear Mika, On Mon, Sep 22, 2014 at 12:12:07PM +0300, Mika Westerberg wrote: On Sat, Sep 20, 2014 at 11:36:34AM +0200, Wolfram Sang wrote: On Thu, Sep 18, 2014 at 12:26:07PM +0300, Mika Westerberg wrote: From: Carl Peng carlpeng...@gmail.com Add support for AMD version of the DW I2C host controller. The device is enumerated from ACPI namespace with ACPI ID AMD0010. Because the core driver needs an input source clock, and this is not an Intel LPSS device where clocks are provided through drivers/acpi/acpi_lpss.c, we register the clock ourselves if the clock rate is given in -driver_data. Signed-off-by: Carl Peng carlpeng...@gmail.com Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com --- Applied to for-next, still wondering... Thanks! drivers/i2c/busses/Kconfig | 1 + drivers/i2c/busses/i2c-designware-platdrv.c | 27 +++ 2 files changed, 28 insertions(+) diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 2ac87fa3058d..9384498ef3c1 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -422,6 +422,7 @@ config I2C_DESIGNWARE_CORE config I2C_DESIGNWARE_PLATFORM tristate Synopsys DesignWare Platform + depends on COMMON_CLK ... do all previous users have COMMON_CLK? The driver is being used on x86, ARM and ARC it seems. For x86 and ARM we pretty much have COMMON_CLK nowadays but I'm not sure about ARC. That's why I have copied Christian Ruppert. Christian, Do you see problems on your side if we depend on COMMON_CLK? COMMON_CLK is not selected by the ARC architecture in general. However, we do select COMMON_CLK in the TB10x platform which uses the designware I2C driver so this new dependency is no problem for us. Vineet, Do you see any issues with this on other existing ARC platforms, e.g. arcfpga? Greetings, Christian -- 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 2/2] i2c: designware: Add support for AMD I2C controller
On Mon, Sep 22, 2014 at 02:16:25PM +, Vineet Gupta wrote: On Monday 22 September 2014 07:30 PM, Mika Westerberg wrote: COMMON_CLK is not selected by the ARC architecture in general. However, we do select COMMON_CLK in the TB10x platform which uses the designware I2C driver so this new dependency is no problem for us. Vineet, Do you see any issues with this on other existing ARC platforms, e.g. arcfpga? So what needs to be done, COMMON_CLK needs to be defined in arch/arc/Kconfig ? And if so why ? Without COMMON_CLK, you are not able to select I2C_DESIGNWARE_PLATFORM anymore. So if something on ARC depends on this driver then we either need the COMMON_CLK there or figure out alternative way to fix Carl's problem. I have not seen the orig patch, but it seems COMMON_CLK is already being selected by TB10x, do we still need it in arch/arcKconfig, for all ARC platforms ? Sorry for the confusion, I should have given you some context. Mika has checked that designware i2c is used by some ARC platforms but he didn't say which ones. The orig patch makes COMMON_CLK a requirement for designware i2c. I checked that we're fine for TB10x (we need COMMON_CLK anyway) but since you weren't in copy I just wanted to make sure that none of the other ARC platforms (which don't seem to select COMMON_CLK) use designware i2c and thus run into trouble. If there is no designware i2c in any of your platforms, simply ignore my message. From my point of view there is no need to move select COMMON_CLK up to the ARC architecture level. Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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 1/2] i2c-designware: make *CNT values configurable
On Sat, Aug 24, 2013 at 01:58:47PM +0900, Shinya Kuribayashi wrote: On 8/21/13 11:39 PM, Christian Ruppert wrote: On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: On 8/5/13 6:31 PM, Christian Ruppert wrote: On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: As said before, all t_SCL things should go away. Please forget about 100kbps, 400kbps, and so on. Bus/clock speed is totally pointless concept for the I2C bus systems. For example, as long as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a certain I2C bus, it doesn't matter that the resulting clock speed is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. Nobody in the I2C bus doesn't care about actual bus/clock speed. We don't have to care about the resulting bus speed, or rather we should/must not check to see if it's within the proper range. Actually, the I2C specification clearly defines f_SCL;max (and thus implies t_SCL;min), both in the tables and the timing diagrams. Why can we ignore this constraint while having to meet all the others? If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), f_SCL;max will be met by itself. I'm not sure if I agree with this: Standard mode: t_r;min 0ns t_f;min +0ns t_HIGH;min + 4000ns t_LOW;min + 4700ns 1/f_SCL = 8700ns == f_SCL = 115kHz==violation of f_SCL;max=100kHz Fast mode (let's assume V_DD = 5.5V): t_r;min 20ns t_f;min + 20ns t_HIGH;min + 600ns t_LIW;min + 1300ns 1/f_SCL = 1940ns == f_SCL = 515kHz==violation of f_SCL;max=400kHz It's more realistic to calculate with say 50ns tr,tf 300ns, than with tt = tf = 0ns or 20ns. Even if with such real tf/tr times, there is cases where f_SCL can be greater than 100/400Hz. I agree. I just used these values to illustrate my point. I understand what you mean, but that was not my point. See below. And again, all I2C master and slave devices in the bus don't care about f_SCL; what they do care are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying f_SCL is pointless and has no value for HCNT/LCNT calculations. I partially agree: If I2C devices don't care about f_SCL but only about t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max constraint. If this is the case, I'm wondering why it is part of the specification, though. With t_r;max and t_f;max, Standard mode: t_r;max 1000ns (max time applied) t_f;max + 300ns (max time applied) t_HIGH;min + 4000ns t_LOW;min + 4700ns 1/f_SCL =1ns == f_SCL = 100kHz== f_SCL;max for Standard-mode Fast mode: t_r;max300ns (max time applied) t_f;max + 300ns (max time applied) t_HIGH;min + 600ns t_LIW;min + 1300ns 1/f_SCL = 2500ns == f_SCL = 400kHz== f_SCL;max for Fast-mode f_SCL;max is defined as a resulting clock frequency with the combination of: (1) the max. conditions of t_r and t_f (2) the min. conditions of t_HIGH and t_LOW We can try to meet t_HIGH;min and t_LOW;min, but we can't meet t_r;min nor t_f;min in the actual systems. The t_r and t_f are _minimum requisites_ for the I/O buffer characteristic of the master and the board designs, hence necessarily contain some time margin. I agree. f_SCL is anything more than the resulting speed of (1) + (2), so I don't think we need to adjust HCNT/LCNT values at all. If with t_r t_r;max and t_f t_f;max, and you've got a faster clock speed than f_SCL;max, then it's a bonus and we can accept it gratefully. Here I'm not sure: The I2C specification clearly states f_SCL min/max requirements in table 10. I'm feeling a bit uneasy patching a driver with (to date) rather pessimistic bus timings to something which might be over-optimistic. If I submit a patch for this I would like to be sure not to break existing systems. How do other drivers manage this? I'd make a compromise proposal; it's fine to make sure whether the resulting f_SCL is within a supported range, but should not make a correction of HCNT/LCNT values. Just report warning messages that some parameters/calculations might be mis-configured an/or wrong. Not sure if this is a good idea: If f_SCL is met by design I'm perfectly happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat the kernel with confusing warnings. If f_SCL is not automatically met we must either make sure t_HIGH/t_LOW are adjusted or we must take the decision to ignore that constraint and document the reasons behind that decision accordingly. I tried to write my thought down, not sure well-explained, though. Notes: * As long as tHD;SDA issue remains in the DW I2C core, we need to have t_HIGH with a relatively
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: Hi, On 8/5/13 6:31 PM, Christian Ruppert wrote: On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: As said before, all t_SCL things should go away. Please forget about 100kbps, 400kbps, and so on. Bus/clock speed is totally pointless concept for the I2C bus systems. For example, as long as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a certain I2C bus, it doesn't matter that the resulting clock speed is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. Nobody in the I2C bus doesn't care about actual bus/clock speed. We don't have to care about the resulting bus speed, or rather we should/must not check to see if it's within the proper range. Actually, the I2C specification clearly defines f_SCL;max (and thus implies t_SCL;min), both in the tables and the timing diagrams. Why can we ignore this constraint while having to meet all the others? If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), f_SCL;max will be met by itself. I'm not sure if I agree with this: Standard mode: t_r;min 0ns t_f;min +0ns t_HIGH;min + 4000ns t_LOW;min + 4700ns 1/f_SCL = 8700ns == f_SCL = 115kHz==violation of f_SCL;max=100kHz Fast mode (let's assume V_DD = 5.5V): t_r;min 20ns t_f;min + 20ns t_HIGH;min + 600ns t_LIW;min + 1300ns 1/f_SCL = 1940ns == f_SCL = 515kHz==violation of f_SCL;max=400kHz In my understanding, f_SCL;max condition is only met a) either if t_HIGH = t_HIGH;min and t_LOW = t_LOW;min then t_r must be t_r;max and t_f must be t_f;max b) or if t_r t_r;max and t_f t_f;max then t_HIGH must be t_HIGH;min and T_LOW must be T_LOW;min Given that we cannot easily influence t_r and t_f we must adjust t_HIGH and t_LOW. What am I missing here? And again, all I2C master and slave devices in the bus don't care about f_SCL; what they do care are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying f_SCL is pointless and has no value for HCNT/LCNT calculations. I partially agree: If I2C devices don't care about f_SCL but only about t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max constraint. If this is the case, I'm wondering why it is part of the specification, though. Is that clear? What is the point to make sure whether f_SCL constraint is met or not? Is there any combination where t_f, t_r, t_HIGH, t_LOW, t_HD;SATA are met, but f_SCL is out of range? I don't think there is. See above. I'd make a compromise proposal; it's fine to make sure whether the resulting f_SCL is within a supported range, but should not make a correction of HCNT/LCNT values. Just report warning messages that some parameters/calculations might be mis-configured an/or wrong. Not sure if this is a good idea: If f_SCL is met by design I'm perfectly happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat the kernel with confusing warnings. If f_SCL is not automatically met we must either make sure t_HIGH/t_LOW are adjusted or we must take the decision to ignore that constraint and document the reasons behind that decision accordingly. Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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 1/2] i2c-designware: make *CNT values configurable
On Mon, Aug 05, 2013 at 12:02:26PM +0200, Wolfram Sang wrote: Would it make sense to add generic I2C device tree properties for those parameters? These parameters are independent of the actual bus driver, rather a PCB property... And as such the correct place would be device tree or ACPI or similar. If there are other bus drivers that make use of tr/tf transition times, I think it makes sense. Wolfram, what's your opinion on this? If it is a PCB property, it makes sense to have generic bindings for it. Can they have very-safe defaults and thus be optional? We can definitely have safe defaults that work for a given driver/hardware. I don't think the same defaults would be safe for all drivers/hardware: The timing strategies of different I2C hardware seems to vary widely (which edges of the clock are sampled, how does different hardware deal with hold times etc) and depending on which parameters are available to the driver to control these timings, the safe values would either have to be the minimum or the maximum in the range allowed by the I2C specification. Every driver would thus have to implement its own defaults in case the properties are not defined. Christian pgpIYNPelPudx.pgp Description: PGP signature
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On Wed, Jul 17, 2013 at 11:39:58PM +0900, Shinya Kuribayashi wrote:
On 7/16/13 8:16 PM, Christian Ruppert wrote: On Sat, Jul 13, 2013 at
02:36:43PM +0900, Shinya Kuribayashi wrote:
Basically, DW I2C core provides a good enough (and quite direct) way
to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers.
But from my experience (with a slightly old version of DW I2C core
around 2005, version 1.06a or so), DW I2C core was apparently lacking
in an appropriate hardware mechanism to meet tHD;STA timing spec. We
then found that we could meet tHD;STA by increasing *HCNT values, so
that's what currently we have in i2c-designware.c I know with that
workaround applied, tHIGH is to be configured with a larger value
than necessary, but we have no choice. For I2C bus systems, we must
meet every timing constraint strictly as required. If DW I2C core
cannot meet tHD;STA without the sacrifice of tHIGH, and I would call
it a hardware bug.
I agree. However, tHD;STA [min] is defined to the same value as tHIGH
[min] for all modes in the I2C specification. Do I understand you
correctly that the SCL_HCNT parameters control at the same time tHIGH
and tHD;STA?
*HCNT value does affect both tHIGH and tHD;STA at the same time.
But we have to make sure that composition of tHIGH is different
from the one of tHD;STA.
For tHIGH
--
DW I2C core is aware of the SCL rising transition (tr) and can
ignore it. It starts counting the HIGH period of the SCL signal
(tHIGH) after the SCL input voltage increases at VIH.
This is described in the data book as an ideal configuration,
and the resulting formula would be:
HCNT + (1+4+3) = IC_CLK * tHIGH ... (a)
please refer to the data book for details about (1+4+3) part.
For tLOW
DW I2C core starts counting the SCL CNTs for the LOW period of
the SCL signal (tLOW) as soon as it pulls the SCL line _without_
_confirming_ the SCL input voltage decreases at VIL.
In order to meet the tLOW timing spec, we need to take into
account the fall time of SCL signal (tf), so the resulting
formula should be:
LCNT + 1 = IC_CLK * (tLOW + tf)
please refer to the data book for details about '+1' part.
For tHD;STA
---
There is no explanation about tHD;STA in the data book. We
only have (my) experimental result; tHD;STA turned out to be
proportional to 'HCNT + 3'. The formula is:
HCNT + 3 = IC_CLK * (tHD;STA + tf) ... (b)
DW I2C core seems to start counting the SCL CNTs for the HIGH
period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_
line without confirming the SDA input voltage decreases at VIL,
so we have to take into account the SDA falling transition (tf)
here.
As can be expected from (a) and (b), if tHD;STA [min] is defined
to the same value as tHIGH [min], we need to have larger HCNT
value than necessary for tHIGH, to meet tHD;STA constraint.
[...]
Hrmmm... This makes my head spin. Do you think the following code
snippet represents an accurate method to calculate the register values?
If you agree I'll prepare a patch based on that for the DW I2C. The
question will be, however, how to obtain the transition times.
Would it make sense to add generic I2C device tree properties for those
parameters? These parameters are independent of the actual bus driver,
rather a PCB property... And as such the correct place would be device
tree or ACPI or similar.
Greetings,
Christian
--- SNIP ---
/*
*t_f;SDA
* |-|
* _ _ . . .
* \/
* SDA \ /
*\/ t_r;SCLt_f;SCL
* |-||-|
* __
* \ /\
* SCL\ / \
* \_/\___ . . .
*|--| |-| ||
*t_HD;STAt_LOW t_HIGH
*
* ||---| ||
* ( HCNT LCNTHCNT ) * 1/f_SYS
*
*
* HCNT = f_SYS * max(t_HD;STA + t_f;SDA , t_HIGH)
* = f_SYS * (t_HIGH + t_f;SDA) because T_HD;STA == T_HIGH
* LCNT = f_SYS * (t_f;SCL + t_LOW)
* HCNT + LCNT + t_r;SCL = 1/f_SCL = t_SCL
*/
#define I2C_STD_MODE (0)
#define I2C_FAST_MODE (1)
#define I2C_FAST_MODE_PLUS (2)
static const struct i2c_timing_params {
unsigned int t_SCL; /* t_SCL in (ms * 65536) */
unsigned int t_LOW; /* t_LOW in (ms * 65536) */
unsigned int t_HIGH; /* t_HIGH = t_HD;STA in (ms * 65536) */
} i2c_timing_params[] = {
{ /* I2C_STD_MODE */
.t_SCL = 10.0e-3 * 65536 + 0.5,
.t_LOW = 4.7e-3 * 65536 + 0.5,
.t_HIGH = 4.0e-3 * 65536 + 0.5,
},
{ /* I2C_FAST_MODE */
.t_SCL = 2.5e-3 * 65536 + 0.5,
.t_LOW = 1.3e-3 * 65536 + 0.5
Re: [PATCH 1/2] i2c-designware: make *CNT values configurable
On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: Hi, Now I've had a look at the whole discussion. Basically, DW I2C core provides a good enough (and quite direct) way to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. But from my experience (with a slightly old version of DW I2C core around 2005, version 1.06a or so), DW I2C core was apparently lacking in an appropriate hardware mechanism to meet tHD;STA timing spec. We then found that we could meet tHD;STA by increasing *HCNT values, so that's what currently we have in i2c-designware.c I know with that workaround applied, tHIGH is to be configured with a larger value than necessary, but we have no choice. For I2C bus systems, we must meet every timing constraint strictly as required. If DW I2C core cannot meet tHD;STA without the sacrifice of tHIGH, and I would call it a hardware bug. I agree. However, tHD;STA [min] is defined to the same value as tHIGH [min] for all modes in the I2C specification. Do I understand you correctly that the SCL_HCNT parameters control at the same time tHIGH and tHD;STA? Concerning SDA_HOLD_TIME, we have done a few measurements in our lab and it looks like the delay between the falling edge clock of the start condition and the rising edge of the first data bit of the start byte is controlled by this parameter. I conclude that in the drawing below (1) is controlled by SCL_HCNT whereas (2) is controlled by SDA_HOLD_TIME. _ scl \__ ... ___ __ sda \_/ ... |-|---| (1) (2) On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote: If I understand the above, it leaves Tf and Tr to be PCB specific and then these values are passed to the core driver from platform data, right? That would be the idea: Calculate Th' and Tl' in function of the desired clock frequency and duty cycle and then adapt these values using measured transition times. I think this would be a good solution. On 7/8/13 10:42 PM, Christian Ruppert wrote: Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN methods are measured by our HW guys on a certain board and they have verified that using those we meet all the I2C timing specs. In order to take advantage of those we need some way to pass those values to the i2c designware core. I have two suggestions: - Use the method outlined in this patch and let the interface driver override *CNT values. With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA quite accurately. On the other hand, what we can't control with DW I2C core is tr and tf. I assumed that it could never be longer than 300nsec (it's defined as a max. in the I2C specification), so I used it for calculation. I agree that tr and tf are PCB specific, and it would a good first step toward timing optimization to make them configurable through platform data. Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt calculations don't suit with later DW I2C cores, then it would be nice for someone who can access to the data book to update formulas, or provide alternative formulas and make them selectable depending on DW core versions. I'm not having the impression there is a huge difference between the different generations of DW blocks. Probably we can find one formula that suits all blocks. We just have to be careful (in doubt rather conservative) with the transition times. This seems to be currently the case and if I understand Mika correctly, his objective is to remove some of this conservatism. It always helps us to have a way to calculate *HCNT and *LCNT values automatically. As said above, DW I2C core can control tHIGH, tLOW, tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated with proper formulas. It also helps hardware people as well to provide reference HCNT/LCNT values. And as a third step, if we want to use optimized HCNT/LCNT values which can not be obtained from proper formulas + user-requested tf/tr, or if we want to use HCNT/LCNT settings verified by vendors or provided from hardware team, then I'm fine with having a way to _override_ HCNT/LCNT values. Such direct way might be useful. I agree. Probably it is best to have both, a default method based on formulas and timing parameters (the formulas are quite simple anyway) which works with device tree and such and a second method based on register values which works with mechanisms like ACPI. Christian -- 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 1/2] i2c-designware: make *CNT values configurable
On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote: On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote: What I meant is the following: The clock cycle time Tc is composed of the four components Tc = Th + Tf + Tl + Tr where Th: Time during which the signal is high Tf: Falling edge transition time Tl: Time during which the signal is low Tr: Rising edge transition time The I2C specification specifies a minimum for Tl and Th and a range (or maximum) for Tr and Tf. A maximum frequency is specified as the frequency obtained by adding the minima for Th and Tl to the maxima of Tr ant Tf. Since as you said, transition times are very much PCB dependent, one way to guarantee the max. frequency constraint (and to achieve a constant frequency at its max) is to define the constants Th' = Th + Tf := Th_min + Tf_max Tl' = Tl + Tr := Tl_min + Tr_max and to calculate the variables Th = Th' - Tf Tl = Tl' - Tr in function of Tf and Tr of the given PCB. If I understand the above, it leaves Tf and Tr to be PCB specific and then these values are passed to the core driver from platform data, right? That would be the idea: Calculate Th' and Tl' in function of the desired clock frequency and duty cycle and then adapt these values using measured transition times. What prevented me from implementing this rather academic approach are the following comments in i2c-designware-core.c: /* * DesignWare I2C core doesn't seem to have solid strategy to meet * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec * will result in violation of the tHD;STA spec. */ /* ... * This is just experimental rule; the tHD;STA period * turned out to be proportinal to (_HCNT + 3). With this setting, * we could meet both tHIGH and tHD;STA timing specs. * ... */ If I interpret this right, the slow down of the clock is intentional to meet tHD;STA timing constraints. I'm thinking that passing directly the measured *CNT values from the platform data makes this even more accurate (given that information is available). And if you only have the Tf and Tr for your board, you can have custom calculation done in the platform part of the driver that takes them into account, and then passes these custom *CNT values to the core. Well, as in the previous discussion on SDA hold time, Tf and Tr are physical values whereas the register values are clock dependent and probably not appropriate at least for device tree usage (or cases where the clock speed might change). If I understand you correctly, ACPI is more register oriented and able to cope with this outside of the OS? At least on Intel hardware we have an ACPI method that returns directly the optimum *CNT values. I don't know ACPI very well and if it deals with register values directly your patch is probably the right thing. Our FW people decided to have a custom ACPI method that returns the correct values to be used in the Windows I2C driver. It returns direct *CNT register values that have been measured to be good for a given PCB. The timing calculation in the driver seems a bit strange to me, however (see above), but I never dared touching it because I never had time to dive into the code deep enough and I was just wondering if it was possible to fix it at the same time. It would be good if we can fix this for non-ACPI devices as well. Is it hard to add similar properties to the driver specific Device Tree bindings? I think it would be quite trivial to add properties for either the register values or the transition time values. For SDA hold time we concluded that time values would be more adequate which brings us to the question of better understanding the hack for tHD;SDA. Otherwise we might break something. Do you think your team which determines the tHD;SDA values could give us some guidance on that? Wolfram, do you have any input on this? -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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 1/2] i2c-designware: make *CNT values configurable
On Tue, Jul 09, 2013 at 11:44:02AM +0300, Mika Westerberg wrote: On Mon, Jul 08, 2013 at 03:42:17PM +0200, Christian Ruppert wrote: On Mon, Jul 08, 2013 at 02:45:26PM +0300, Mika Westerberg wrote: The DesignWare I2C controller has high count (HCNT) and low count (LCNT) registers for each of the I2C speed modes (standard and fast). These registers are programmed based on the input clock speed in the driver. However, that is not always the most accurate way. For example on Intel BayTrail we have 100MHz input clock and calculated *CNT values result as measured by one of our team: Standard mode: 100.25kHz Fast mode: 315.41kHZ The fast mode speed is over 20% lower than what is expected. We have observed similar issues and I am wondering if this effect is due to the overly-pessimistic formulas in i2c_dw_scl_lcnt and i2c_dw_scl_hcnt. The comments in these functions are a bit confusing and I don't pretend having fully understood what the intention is. I'm having the impression that defining the arguments tf in function of the hardware would be the correct way to achieve better clock accuracy. Well, that's not the only timing parameter specified in the I2C spec. We also have tr among other things (even though the dw driver doesn't use it). Exactly. Ant T_HD;STA which is mentioned somewhere in the comment and one of the reasons why I never dared touching these functions. The fact that the designware IP doesn't seem to manage all timings makes this function quite difficult to understand. It might be that there are things like strenght of the pull-up resistors, bus capacitance etc. that are very platform specific and have an effect on the clock signal. I believe tf is supposed to model these things. I just haven't clearly understood how. In my understanding, transition times should be subtracted rather than added to cycle times or am I getting something fundamentally wrong here? I'm not sure, honestly :-) I believe tf is the same tf that is in the I2C spec, meaning fall time of both SCL and SDA signals and the spec measures one clock cycle to be from end of the first tf to the end of the second (fig. 27 in the I2C spec). If I'm reading it right then it means adding instead of substracting. Do you have any suggestions how we could use tf here? What I meant is the following: The clock cycle time Tc is composed of the four components Tc = Th + Tf + Tl + Tr where Th: Time during which the signal is high Tf: Falling edge transition time Tl: Time during which the signal is low Tr: Rising edge transition time The I2C specification specifies a minimum for Tl and Th and a range (or maximum) for Tr and Tf. A maximum frequency is specified as the frequency obtained by adding the minima for Th and Tl to the maxima of Tr ant Tf. Since as you said, transition times are very much PCB dependent, one way to guarantee the max. frequency constraint (and to achieve a constant frequency at its max) is to define the constants Th' = Th + Tf := Th_min + Tf_max Tl' = Tl + Tr := Tl_min + Tr_max and to calculate the variables Th = Th' - Tf Tl = Tl' - Tr in function of Tf and Tr of the given PCB. At least on Intel hardware we have an ACPI method that returns directly the optimum *CNT values. I don't know ACPI very well and if it deals with register values directly your patch is probably the right thing. The timing calculation in the driver seems a bit strange to me, however (see above), but I never dared touching it because I never had time to dive into the code deep enough and I was just wondering if it was possible to fix it at the same time. If ACPI bypasses the function alltogether this is probably not applicable. -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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 1/2] i2c-designware: make *CNT values configurable
, hcnt, lcnt);
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index 912aa22..e8a7565 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -61,6 +61,14 @@
* @tx_fifo_depth: depth of the hardware tx fifo
* @rx_fifo_depth: depth of the hardware rx fifo
* @rx_outstanding: current master-rx elements in tx fifo
+ * @ss_hcnt: standard speed HCNT value
+ * @ss_lcnt: standard speed LCNT value
+ * @fs_hcnt: fast speed HCNT value
+ * @fs_lcnt: fast speed LCNT value
+ *
+ * HCNT and LCNT parameters can be used if the platform knows more accurate
+ * values than the one computed based only on the input clock frequency.
+ * Leave them to be %0 if not used.
*/
struct dw_i2c_dev {
struct device *dev;
@@ -91,6 +99,10 @@ struct dw_i2c_dev {
unsigned intrx_fifo_depth;
int rx_outstanding;
u32 sda_hold_time;
+ u16 ss_hcnt;
+ u16 ss_lcnt;
+ u16 fs_hcnt;
+ u16 fs_lcnt;
};
#define ACCESS_SWAP 0x0001
--
1.8.3.2
--
Christian Ruppert , christian.rupp...@abilis.com
/|
Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri
_// | bilis Systems CH-1228 Plan-les-Ouates
--
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 linux-next] i2c-designware: use div_u64 to fix link
On Tue, Jul 02, 2013 at 11:46:54AM +0200, Vincent Stehlll wrote: This fixes the following link error: drivers/built-in.o: In function `dw_i2c_probe': of_iommu.c:(.text+0x18c8f0): undefined reference to `__aeabi_uldivmod' make: *** [vmlinux] Error 1 Looks good. I also tested it on our ARC based platform, no issues with compilation or functionality. Thanks for pointing this out. Reviewed-by: Christian Ruppert christian.rupp...@abilis.com Signed-off-by: Vincent Stehlé vincent.ste...@freescale.com Cc: Wolfram Sang w...@the-dreams.de Cc: Christian Ruppert christian.rupp...@abilis.com Cc: Pierrick Hascoet pierrick.hasc...@abilis.com --- Hi, Linux next-20130702 link broke for ARM config multi_v7_defconfig. This is with gcc 4.7.2 but I am not sure it matters much here. This patch repairs the link. It did not break anything for me on i.MX6 sabre sd, but it does'nt have a designware i2c, so more reviewing/testing is welcome. Best regards, V. drivers/i2c/busses/i2c-designware-platdrv.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index def79b5..4c5fada 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -122,7 +122,8 @@ static int dw_i2c_probe(struct platform_device *pdev) of_property_read_u32(pdev-dev.of_node, i2c-sda-hold-time-ns, ht); - dev-sda_hold_time = ((u64)ic_clk * ht + 50) / 100; + dev-sda_hold_time = div_u64((u64)ic_clk * ht + 50, + 100); } dev-functionality = -- 1.7.10.4 -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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
[PATCH v10] i2c-designware: make SDA hold time configurable
This patch makes the SDA hold time configurable through device tree.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
.../devicetree/bindings/i2c/i2c-designware.txt | 15 +++
arch/arc/boot/dts/abilis_tb100_dvk.dts | 10 +-
arch/arc/boot/dts/abilis_tb101_dvk.dts | 10 +-
drivers/i2c/busses/i2c-designware-core.c | 13 +
drivers/i2c/busses/i2c-designware-core.h |1 +
drivers/i2c/busses/i2c-designware-platdrv.c| 10 ++
6 files changed, 49 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..7fd7fa2 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,10 @@ Recommended properties :
- clock-frequency : desired I2C bus clock frequency in Hz.
+Optional properties :
+ - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
+ This option is only supported in hardware blocks version 1.11a or newer.
+
Example :
i2c@f {
@@ -20,3 +24,14 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+ i2c@112 {
+ #address-cells = 1;
+ #size-cells = 0;
+ compatible = snps,designware-i2c;
+ reg = 0x112 0x1000;
+ interrupt-parent = ictl;
+ interrupts = 12 1;
+ clock-frequency = 40;
+ i2c-sda-hold-time-ns = 300;
+ };
diff --git a/arch/arc/boot/dts/abilis_tb100_dvk.dts
b/arch/arc/boot/dts/abilis_tb100_dvk.dts
index 0fa0d4a..ebc313a 100644
--- a/arch/arc/boot/dts/abilis_tb100_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb100_dvk.dts
@@ -45,19 +45,19 @@
};
i2c0: i2c@FF12 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c1: i2c@FF121000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c2: i2c@FF122000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c3: i2c@FF123000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c4: i2c@FF124000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
leds {
diff --git a/arch/arc/boot/dts/abilis_tb101_dvk.dts
b/arch/arc/boot/dts/abilis_tb101_dvk.dts
index a4d80ce..b204657 100644
--- a/arch/arc/boot/dts/abilis_tb101_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb101_dvk.dts
@@ -45,19 +45,19 @@
};
i2c0: i2c@FF12 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c1: i2c@FF121000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c2: i2c@FF122000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c3: i2c@FF123000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c4: i2c@FF124000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
leds {
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index c41ca63..d18d4b5 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,9 +67,12 @@
#define DW_IC_STATUS 0x70
#define DW_IC_TXFLR0x74
#define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_ENABLE_STATUS0x9c
#define DW_IC_COMP_PARAM_1 0xf4
+#define DW_IC_COMP_VERSION 0xf8
+#define DW_IC_SDA_HOLD_MIN_VERS0x3131312A
#define DW_IC_COMP_TYPE0xfc
#define DW_IC_COMP_TYPE_VALUE 0x44570140
@@ -332,6 +335,16 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
+ /* Configure SDA Hold Time if required */
+ if (dev-sda_hold_time) {
+ reg = dw_readl(dev, DW_IC_COMP_VERSION);
+ if (reg = DW_IC_SDA_HOLD_MIN_VERS)
+ dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
+ else
[PATCH v9] i2c-designware: make SDA hold time configurable
This patch makes the SDA hold time configurable through device tree.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
.../devicetree/bindings/i2c/i2c-designware.txt | 14 ++
arch/arc/boot/dts/abilis_tb100_dvk.dts | 10 +-
arch/arc/boot/dts/abilis_tb101_dvk.dts | 10 +-
drivers/i2c/busses/i2c-designware-core.c | 13 +
drivers/i2c/busses/i2c-designware-core.h |1 +
drivers/i2c/busses/i2c-designware-platdrv.c| 10 ++
6 files changed, 48 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..3266cc7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,9 @@ Recommended properties :
- clock-frequency : desired I2C bus clock frequency in Hz.
+Optional properties :
+ - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
+
Example :
i2c@f {
@@ -20,3 +23,14 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+ i2c@112 {
+ #address-cells = 1;
+ #size-cells = 0;
+ compatible = snps,designware-i2c;
+ reg = 0x112 0x1000;
+ interrupt-parent = ictl;
+ interrupts = 12 1;
+ clock-frequency = 40;
+ i2c-sda-hold-time-ns = 300;
+ };
diff --git a/arch/arc/boot/dts/abilis_tb100_dvk.dts
b/arch/arc/boot/dts/abilis_tb100_dvk.dts
index 0fa0d4a..ebc313a 100644
--- a/arch/arc/boot/dts/abilis_tb100_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb100_dvk.dts
@@ -45,19 +45,19 @@
};
i2c0: i2c@FF12 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c1: i2c@FF121000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c2: i2c@FF122000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c3: i2c@FF123000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c4: i2c@FF124000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
leds {
diff --git a/arch/arc/boot/dts/abilis_tb101_dvk.dts
b/arch/arc/boot/dts/abilis_tb101_dvk.dts
index a4d80ce..b204657 100644
--- a/arch/arc/boot/dts/abilis_tb101_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb101_dvk.dts
@@ -45,19 +45,19 @@
};
i2c0: i2c@FF12 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c1: i2c@FF121000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c2: i2c@FF122000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c3: i2c@FF123000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c4: i2c@FF124000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
leds {
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index c41ca63..66119e2 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,9 +67,12 @@
#define DW_IC_STATUS 0x70
#define DW_IC_TXFLR0x74
#define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_ENABLE_STATUS0x9c
#define DW_IC_COMP_PARAM_1 0xf4
+#define DW_IC_COMP_VERSION 0xf8
+#define DW_IC_SDA_HOLD_MIN_VERS0x3131312A
#define DW_IC_COMP_TYPE0xfc
#define DW_IC_COMP_TYPE_VALUE 0x44570140
@@ -332,6 +335,16 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
+ /* Configure SDA Hold Time if required */
+ reg = dw_readl(dev, DW_IC_COMP_VERSION);
+ if (dev-sda_hold_time) {
+ if (reg = DW_IC_SDA_HOLD_MIN_VERS)
+ dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
+ else
+ dev_warn(dev-dev,
+ Hardware too old to adjust SDA
Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable
Hello Wolfram, On Wed, Jun 19, 2013 at 05:20:59PM +0200, Wolfram Sang wrote: [...] - It should not be encoded in the devicetree, since the flaw is implicit to the device, so only the driver needs to know about it. I wonder about something like this in the i2c slave driver: ret = i2c_request_sda_hold_time(client); The core then can collect the requests and forward them to the host driver. This driver then can set up the hardware or return -EOPNOTSUPP and we can even warn the user that there might be problems ahead. This might be a solution but given that many I2C drivers are written as an afterthought by device manufacturers and are released under more or less open terms of licensing into the wild I doubt this would work very well in practise. Hrmgl, the design looks much better to me, though. Once a driver is identified to need this, the core is able to report this requirement to a user who might even be unaware of the issue. The dt property has a bit of try this if things don't work, you may be lucky taste to it. Need to think about it. If PCB design also has an influence, then my idea won't work, though. I agree with that in theory. We would need to define more constraints in this case, however: Both min and max setup and hold times would be required for data and start/stop conditions. We have seen cases in which the hold time we configured was too big for some devices on the bus which in turn seemed to get confused between data and start/stop conditions. I don't remember the exact values causing this issue but it was definitely less than the max of 650ns I have grepped out yesterday. Addressing this issue properly would mean implementing some simple form of STA. Not sure if an operating system kernel is the right place for this, though (and pretty sure that individual device drivers aren't). These questions are probably better addressed at the PCB design stage. [...] In the case of the Designware block, the parameter both changes SDA and START hold times, however, and you'll find lots of data sheets for hardware with START hold time requirements on the net, e.g. http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf What I couldn't find is a reference manual for a designware IP that supports sda hold time? I found some spear SoC which do not have that register, so that should surely be reflected in the patchset, too. If you have access to DesignWare documentation, check out the DesignWare DW_apb_i2c Databook Version 1.17a from March 2012. I don't have, and I do have a hard time finding information about it otherwise :( Unluckily, I clearly don't have the right to share this document with you. Do you know the version of the blocks in the spear SoC which do not support this register? ST Spear300 and 600 have 0x3130352a in the version reg according to the refman. Hrmmm... Going through the release notes it looks like this feature was added with module version 1.11a, released in 2010 (the initial version of the IP was released in 2003). The empirical solution in the function i2c_dw_scl_hcnt does not seem to work in all cases: Our lab guys confirmed that we have several PCB designs which do not work without adjusting the sda-hold-time parameter to an appropriate value. The value seems to be different for different PCBs. I'd hope that 300ns is a safe value for all PCBs? Not according to our PCB guys. The highest value I have found in a quick check of our device trees is 650ns with others being just slightly above 300ns. Thanks for sharing your results \o/ This helps me a lot. Can you find out/guess if this value is solely dependend on a slave or is it also dependend on PCB layout? In the simplest form of STA, timings are generally considered a function of three factors: . Drive strength . Bus load (Fan out) . Internal timings of all devices on the bus (setup/hold times etc.) This applies to ASIC internal timing. My PCB experience is quite limited and I'm not sure if this can be applied as such. That said, the I2C specification does define all three of these factors and I deduce they all have their importance. We know that the specified internal device timings are sometimes buggy, I don't know about the other two. Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates pgpyGsxOFvhEv.pgp Description: PGP signature
Re: [PATCH REBASE] i2c-designware: make SDA hold time configurable
On Wed, Jun 19, 2013 at 11:45:40AM +0200, Wolfram Sang wrote: Hi, On Wed, Jun 12, 2013 at 04:47:45PM +0200, Christian Ruppert wrote: On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote: On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote: This patch makes the SDA hold time configurable through device tree. [rebased to i2c-current/i2c-next/mainline-3.10-rc1] Signed-off-by: Christian Ruppert christian.rupp...@abilis.com Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Hmm, I really have problems adding a generic property. I need to better understand why this is needed? What is the usecase? Can't a safe value be calculated depending on the bus-speed? Is there a public datasheet available somewhere? I checked with our PCB/Applications team and the data sheets for the peripherals in question (DVB demodulator front ends) are under NDA. Mika, you seem to be interested in this patch as well. Do you know of any publicly available data sheets for hardware requiring this adjustment? So, I looked around and found: http://www.maximintegrated.com/app-notes/index.mvp/id/3268 which after thinking further about it gives me the following conclusions: - sda-hold-time is a property/requirement of a device not following the I2C spec. It is not a property of the master! Actually, in a protocol like I2C, every device on the bus must respect timing constraints like hold time etc. These parameters apply at the same time to the master and to all slaves. - It should not be encoded in the devicetree, since the flaw is implicit to the device, so only the driver needs to know about it. I wonder about something like this in the i2c slave driver: ret = i2c_request_sda_hold_time(client); The core then can collect the requests and forward them to the host driver. This driver then can set up the hardware or return -EOPNOTSUPP and we can even warn the user that there might be problems ahead. This might be a solution but given that many I2C drivers are written as an afterthought by device manufacturers and are released under more or less open terms of licensing into the wild I doubt this would work very well in practise. - I wonder if we really need to have a parameter time-in-ns? The specs cleary say 300ns, so I'd think this is the value we should always use. This is from a theorhetical pov though, maybe your practical experience is different. What values do you need? In reality, the I2C specification is more subtle than that: The data hold time is specified as 0ns with a footnote [3] stating that devices must internally provide a hold time of at least 300ns for the SDA signal Revision 5 contains a relatively understandable explanation about how to interpret this but earlier versions are less helpful. I think this confusion is at the root of many timing issues encountered with I2C (and the reason why Synopsys made this configurable). In fact, especially earlier specs are _all but_ clear in this point and we cannot assume that all peripherals were designed after Revision 5 was released in October 2012. In the case of the Designware block, the parameter both changes SDA and START hold times, however, and you'll find lots of data sheets for hardware with START hold time requirements on the net, e.g. http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf What I couldn't find is a reference manual for a designware IP that supports sda hold time? I found some spear SoC which do not have that register, so that should surely be reflected in the patchset, too. If you have access to DesignWare documentation, check out the DesignWare DW_apb_i2c Databook Version 1.17a from March 2012. Unluckily, I clearly don't have the right to share this document with you. Do you know the version of the blocks in the spear SoC which do not support this register? The empirical solution in the function i2c_dw_scl_hcnt does not seem to work in all cases: Our lab guys confirmed that we have several PCB designs which do not work without adjusting the sda-hold-time parameter to an appropriate value. The value seems to be different for different PCBs. I'd hope that 300ns is a safe value for all PCBs? Not according to our PCB guys. The highest value I have found in a quick check of our device trees is 650ns with others being just slightly above 300ns. I suspect that this kind of configurability is not the same for all i2c bus master hardware. Yeah, maybe some do sda-holding by default? Dunno, never checked for that detail. Regards, Wolfram -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- To unsubscribe from this list: send the line unsubscribe linux
[PATCH v8] i2c-designware: make SDA hold time configurable
This patch makes the SDA hold time configurable through device tree.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
.../devicetree/bindings/i2c/i2c-designware.txt | 14 ++
arch/arc/boot/dts/abilis_tb100_dvk.dts | 10 +-
arch/arc/boot/dts/abilis_tb101_dvk.dts | 10 +-
drivers/i2c/busses/i2c-designware-core.c |5 +
drivers/i2c/busses/i2c-designware-core.h |1 +
drivers/i2c/busses/i2c-designware-platdrv.c| 10 ++
6 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..3266cc7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,9 @@ Recommended properties :
- clock-frequency : desired I2C bus clock frequency in Hz.
+Optional properties :
+ - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
+
Example :
i2c@f {
@@ -20,3 +23,14 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+ i2c@112 {
+ #address-cells = 1;
+ #size-cells = 0;
+ compatible = snps,designware-i2c;
+ reg = 0x112 0x1000;
+ interrupt-parent = ictl;
+ interrupts = 12 1;
+ clock-frequency = 40;
+ i2c-sda-hold-time-ns = 300;
+ };
diff --git a/arch/arc/boot/dts/abilis_tb100_dvk.dts
b/arch/arc/boot/dts/abilis_tb100_dvk.dts
index 0fa0d4a..ebc313a 100644
--- a/arch/arc/boot/dts/abilis_tb100_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb100_dvk.dts
@@ -45,19 +45,19 @@
};
i2c0: i2c@FF12 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c1: i2c@FF121000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c2: i2c@FF122000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c3: i2c@FF123000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c4: i2c@FF124000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
leds {
diff --git a/arch/arc/boot/dts/abilis_tb101_dvk.dts
b/arch/arc/boot/dts/abilis_tb101_dvk.dts
index a4d80ce..b204657 100644
--- a/arch/arc/boot/dts/abilis_tb101_dvk.dts
+++ b/arch/arc/boot/dts/abilis_tb101_dvk.dts
@@ -45,19 +45,19 @@
};
i2c0: i2c@FF12 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c1: i2c@FF121000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c2: i2c@FF122000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c3: i2c@FF123000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
i2c4: i2c@FF124000 {
- sda-hold-time = 432;
+ i2c-sda-hold-time-ns = 432;
};
leds {
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index c41ca63..6c0e776 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
#define DW_IC_STATUS 0x70
#define DW_IC_TXFLR0x74
#define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_ENABLE_STATUS0x9c
#define DW_IC_COMP_PARAM_1 0xf4
@@ -332,6 +333,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
+ /* Configure SDA Hold Time if required */
+ if (dev-sda_hold_time)
+ dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
+
/* Configure Tx/Rx FIFO threshold levels */
dw_writel(dev, dev-tx_fifo_depth - 1, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index e761ad1..912aa22 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -90,6 +90,7 @@ struct dw_i2c_dev
Re: [PATCH V2] i2c: designware: fix race between subsequent xfers
On Fri, Jun 14, 2013 at 04:37:41PM +0200, Wolfram Sang wrote: On Fri, Jun 07, 2013 at 10:51:23AM +0200, Christian Ruppert wrote: [...] + /* +* We must disable the adapter before unlocking the dev-lock mutex +* below. Otherwise the hardware might continue generating interrupts +* which in turn causes a race condition with the following transfer. I added Needs some more investigation if the additional interrupts are a hardware bug or this driver doesn't handle them correctly yet. to the comment and Good idea, thanks. Applied to for-next, thanks! BTW since I am currently here: i2c-designware-core should be in the 'algos' directory, no? At the risk of passing for a complete moron: What exactly is the difference between I2C algos and I2C bus drivers? Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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] i2c: designware: fix race between subsequent xfers
On Mon, Jun 17, 2013 at 10:33:36AM +0200, Jean Delvare wrote: On Mon, 17 Jun 2013 10:19:33 +0200, Christian Ruppert wrote: On Fri, Jun 14, 2013 at 04:37:41PM +0200, Wolfram Sang wrote: BTW since I am currently here: i2c-designware-core should be in the 'algos' directory, no? At the risk of passing for a complete moron: What exactly is the difference between I2C algos and I2C bus drivers? The i2c/algos directory contains abstracted code which is common to multiple hardware implementations. The most popular of these is i2c-algo-bit which implements software-only I2C over virtually any pair of controllable pins (parallel port, GPIOs, etc.) As a general rule, i2c/algos should only contain reusable, architecture and platform independent code. All the actual hardware access should be delegated to the bus drivers, through callbacks. If this can't be done easily then i2c/algos is not the right place. In this case, busses is the right place for the i2c-designware-core. This file contains the actual driver implementation (i.e. register access, interrupt handling etc.) for a dedicated I2C bus driver hardware block used in SOCs. Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure
Hello, As promised, I gave this one some over-night stress testing and I can confirm what I said previously: - The patch does _not_ solve the interrupt loop lockups on its own. - The patch works well in conjunction with http://patchwork.ozlabs.org/patch/249622/ (which in turn depends on Mika's patch). Under this condition you can assume Tested-By: Christian Ruppert christian.rupp...@abilis.com I still think the code is more logical with this patch than without it and I am in favour of applying both (if Andy agrees that is). We must keep in mind, however, that http://patchwork.ozlabs.org/patch/249622 does fix a real problem we can observe on our chip and for our TB10x product we do require some form of it for stability reasons. Greetings, Christian On Fri, Jun 07, 2013 at 12:30:01PM +0300, Andy Shevchenko wrote: Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com --- drivers/i2c/busses/i2c-designware-core.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index c41ca63..a1d3d95 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -366,9 +366,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) struct i2c_msg *msgs = dev-msgs; u32 ic_con; - /* Disable the adapter */ - __i2c_dw_enable(dev, false); - /* set the slave (target) address */ dw_writel(dev, msgs[dev-msg_write_idx].addr, DW_IC_TAR); @@ -561,6 +558,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) mutex_lock(dev-lock); pm_runtime_get_sync(dev-dev); + ret = i2c_dw_wait_bus_not_busy(dev); + if (ret 0) + goto done; + + /* Disable the adapter */ + __i2c_dw_enable(dev, false); + INIT_COMPLETION(dev-cmd_complete); dev-msgs = msgs; dev-msgs_num = num; @@ -572,10 +576,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) dev-abort_source = 0; dev-rx_outstanding = 0; - ret = i2c_dw_wait_bus_not_busy(dev); - if (ret 0) - goto done; - /* start the transfers */ i2c_dw_xfer_init(dev); -- 1.8.2.rc0.22.gb3600c3 -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure
On Thu, Jun 13, 2013 at 11:33:56AM +0300, Andy Shevchenko wrote: On Thu, 2013-06-13 at 10:16 +0200, Christian Ruppert wrote: As promised, I gave this one some over-night stress testing and I can confirm what I said previously: - The patch does _not_ solve the interrupt loop lockups on its own. So, it just means my assumptions about what is happening there were wrong. So were my initial ones... Or at least insufficient. - The patch works well in conjunction with http://patchwork.ozlabs.org/patch/249622/ (which in turn depends on Mika's patch). Under this condition you can assume Tested-By: Christian Ruppert christian.rupp...@abilis.com I still think the code is more logical with this patch than without it and I am in favour of applying both (if Andy agrees that is). Since my patch doesn't fix anything, I think we may drop it away. We must keep in mind, however, that http://patchwork.ozlabs.org/patch/249622 does fix a real problem we can observe on our chip and for our TB10x product we do require some form of it for stability reasons. I feel like a real fix is to add a memory barier to make changes in the structure consistent. However, I have no clue where. I'm still not sure about the interrupt behaviour of the dw-i2c block in the case of error (and since our problem is fixed it's difficult to justify spending time to investigate further). I suspect that the thing in some situations sends spurious interrupts which confuse the state machine - in which case memory barriers won't help us either. Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure
On Tue, Jun 11, 2013 at 08:40:37PM +0200, Wolfram Sang wrote: On Fri, Jun 07, 2013 at 03:01:34PM +0200, Christian Ruppert wrote: Hi Andy, I like your patch and I just did some testing on it. Unluckily, it doesn't solve the lock-up problem. I haven't investigated any further but I suspect that on top of the cases I observed when debugging this (interrupts after initialisation of dev, easy to prove) there are more obscure cases in which interrupts are generated in an unexpected manner after errors. The interrupt-driven transfer state machine of the driver implicitly relies on the fact that all updates of dev which are related to the same transfer are performed between the mutex_lock and mutex_unlock calls in i2c_dw_xfer. Thus, I decided it was safer to disable the block before releasing the mutex when I wrote my patch. That said, I think the sequencing at transfer initialisation is more logical with your patch and I wonder if it is still worth applying. Any other opinions on this? Ping. There are: [V2] i2c: designware: fix race between subsequent xfers [RFC] i2c-designware-core: disable adapter before fill dev structure What is the consensus of those two patches? Although I almost like Andy's code better than my own it doesn't seem to fix the issue we're aiming at (system lock ups due to undesired interrupts) in all cases. The [V2] i2c: designware: fix race between subsequent xfers is currently the only patch preventing those lock ups in our tests. That said, IMHO Andy's patch seems to be a valuable code clean up nevertheless and could be applied in addition to the other. I suggest I give the combination of both patches some additional testing on the occasion and tag Andy's RFC with tested-by me in case it's stable. Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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 REBASE] i2c-designware: make SDA hold time configurable
On Mon, Jun 10, 2013 at 05:29:55PM +0200, Wolfram Sang wrote: On Tue, May 14, 2013 at 03:04:02PM +0200, Christian Ruppert wrote: This patch makes the SDA hold time configurable through device tree. [rebased to i2c-current/i2c-next/mainline-3.10-rc1] Signed-off-by: Christian Ruppert christian.rupp...@abilis.com Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Hmm, I really have problems adding a generic property. I need to better understand why this is needed? What is the usecase? Can't a safe value be calculated depending on the bus-speed? Is there a public datasheet available somewhere? I checked with our PCB/Applications team and the data sheets for the peripherals in question (DVB demodulator front ends) are under NDA. Mika, you seem to be interested in this patch as well. Do you know of any publicly available data sheets for hardware requiring this adjustment? In the case of the Designware block, the parameter both changes SDA and START hold times, however, and you'll find lots of data sheets for hardware with START hold time requirements on the net, e.g. http://ww1.microchip.com/downloads/en/DeviceDoc/21805B.pdf The empirical solution in the function i2c_dw_scl_hcnt does not seem to work in all cases: Our lab guys confirmed that we have several PCB designs which do not work without adjusting the sda-hold-time parameter to an appropriate value. The value seems to be different for different PCBs. I suspect that this kind of configurability is not the same for all i2c bus master hardware. If you don't want to add a generic property, would you accept the patch with the property renamed to dwi2c,sda-hold-time? Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates pgpJq0LvhUoGt.pgp Description: PGP signature
Re: [PATCH 1/2] i2c: designware: fix race between subsequent xfers
On Fri, Jun 07, 2013 at 08:23:53AM +0300, Mika Westerberg wrote: Hi Christian, On Thu, Jun 06, 2013 at 03:43:35PM +0200, Christian Ruppert wrote: The designware block is not always properly disabled in the case of transfer errors. Interrupts from aborted transfers might be handled after the data structures for the following transfer are initialised but before the hardware is set up. This might corrupt the data structures to the point that the system is stuck in an infinite interrupt loop (where FIFOs are never emptied). This patch cleanly disables the designware-i2c hardware at the end of every transfer, successful or not. Have you tried with the latest mainline driver? There is a commit that solves similar problem: 2a2d95e9d6d29e7 i2c: designware: always clear interrupts before enabling them Maybe it helps? Hi Mika, Thanks for the hint but I have checked both main line and Wolfram's branch and I saw this patch. I actually hoped it would fix our problem but it didn't. Here some more details: We experienced system lockups (complete lock up, no reaction whatsoever) in long-term tests under heavy system load with lots of scheduling and forking/killing. These lockups could be traced to the I2C driver which after some time ended up in an incoherent state: i2c_dw_isr was being called with DW_IC_INTR_RX_FULL but dev-msg_read_idx == dev-msgs_num. This resulted in the FIFO never being emptied by i2c_dw_read. Since the DW_IC_INTR_RX_FULL interrupt is cleared by emptying the FIFO, this situation results in an IRQ loop locking up the system. We found that the situation systematically occurs just after the originating process is interrupted (premature return of wait_for_completion_interruptible_timeout) and further analysis showed the race condition: Interrupts from the previous transfer are sometimes triggered after the initialisation of dev in the beginning of i2c_dw_xfer, thus corrupting the state. If these interrupts occur before dev is initialised everything works fine. An alternative solution would probably be to make sure the hardware is disabled before initialising the dev structure in i2c_dw_xfer. Greetings, Christian -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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
[PATCH V2] i2c: designware: fix race between subsequent xfers
The designware block is not always properly disabled in the case of
transfer errors. Interrupts from aborted transfers might be handled
after the data structures for the following transfer are initialised but
before the hardware is set up. This can corrupt the data structures to
the point that the system is stuck in an infinite interrupt loop (where
FIFOs are never emptied because dev-msg_read_idx == dev-msgs_num).
This patch cleanly disables the designware-i2c hardware at the end of
every transfer, be it successful or not.
This patch requires https://patchwork.kernel.org/patch/2601241/ to be
applied first.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
---
drivers/i2c/busses/i2c-designware-core.c | 10 --
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index b75d292..55a9991 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -588,11 +588,19 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
ret = wait_for_completion_timeout(dev-cmd_complete, HZ);
if (ret == 0) {
dev_err(dev-dev, controller timed out\n);
+ /* i2c_dw_init implicitly disables the adapter */
i2c_dw_init(dev);
ret = -ETIMEDOUT;
goto done;
}
+ /*
+* We must disable the adapter before unlocking the dev-lock mutex
+* below. Otherwise the hardware might continue generating interrupts
+* which in turn causes a race condition with the following transfer.
+*/
+ __i2c_dw_enable(dev, false);
+
if (dev-msg_err) {
ret = dev-msg_err;
goto done;
@@ -600,8 +608,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
/* no error */
if (likely(!dev-cmd_err)) {
- /* Disable the adapter */
- __i2c_dw_enable(dev, false);
ret = num;
goto done;
}
--
1.7.1
--
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: i2c: designware: prevent signals from aborting I2C transfers
On Wed, May 22, 2013 at 10:03:11AM -, Mika Westerberg wrote:
If a process receives signal while it is waiting for I2C transfer to
complete, an error is returned to the caller and the transfer is aborted.
This can cause the driver to fail subsequent transfers. Also according to
commit d295a86eab2 (i2c: mv64xxx: work around signals causing I2C
transactions to be aborted) I2C drivers aren't supposed to abort
transactions on signals.
To prevent this switch to use wait_for_completion_timeout() instead of
wait_for_completion_interruptible_timeout() in the designware I2C driver.
Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
Reviewed-by: Christian Ruppert christian.rupp...@abilis.com
---
drivers/i2c/busses/i2c-designware-core.c |5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index c41ca63..db20a28 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -580,14 +580,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
i2c_dw_xfer_init(dev);
/* wait for tx to complete */
- ret = wait_for_completion_interruptible_timeout(dev-cmd_complete, HZ);
+ ret = wait_for_completion_timeout(dev-cmd_complete, HZ);
if (ret == 0) {
dev_err(dev-dev, controller timed out\n);
i2c_dw_init(dev);
ret = -ETIMEDOUT;
goto done;
- } else if (ret 0)
- goto done;
+ }
if (dev-msg_err) {
ret = dev-msg_err;
--
Christian Ruppert , christian.rupp...@abilis.com
/|
Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri
_// | bilis Systems CH-1228 Plan-les-Ouates
--
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: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure
Hi Andy, I like your patch and I just did some testing on it. Unluckily, it doesn't solve the lock-up problem. I haven't investigated any further but I suspect that on top of the cases I observed when debugging this (interrupts after initialisation of dev, easy to prove) there are more obscure cases in which interrupts are generated in an unexpected manner after errors. The interrupt-driven transfer state machine of the driver implicitly relies on the fact that all updates of dev which are related to the same transfer are performed between the mutex_lock and mutex_unlock calls in i2c_dw_xfer. Thus, I decided it was safer to disable the block before releasing the mutex when I wrote my patch. That said, I think the sequencing at transfer initialisation is more logical with your patch and I wonder if it is still worth applying. Any other opinions on this? Greetings, Christian On Fri, Jun 07, 2013 at 12:30:01PM +0300, Andy Shevchenko wrote: Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com --- drivers/i2c/busses/i2c-designware-core.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index c41ca63..a1d3d95 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -366,9 +366,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) struct i2c_msg *msgs = dev-msgs; u32 ic_con; - /* Disable the adapter */ - __i2c_dw_enable(dev, false); - /* set the slave (target) address */ dw_writel(dev, msgs[dev-msg_write_idx].addr, DW_IC_TAR); @@ -561,6 +558,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) mutex_lock(dev-lock); pm_runtime_get_sync(dev-dev); + ret = i2c_dw_wait_bus_not_busy(dev); + if (ret 0) + goto done; + + /* Disable the adapter */ + __i2c_dw_enable(dev, false); + INIT_COMPLETION(dev-cmd_complete); dev-msgs = msgs; dev-msgs_num = num; @@ -572,10 +576,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) dev-abort_source = 0; dev-rx_outstanding = 0; - ret = i2c_dw_wait_bus_not_busy(dev); - if (ret 0) - goto done; - /* start the transfers */ i2c_dw_xfer_init(dev); -- 1.8.2.rc0.22.gb3600c3 -- Christian Ruppert , christian.rupp...@abilis.com /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- 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
[PATCH 1/2] i2c: designware: fix race between subsequent xfers
The designware block is not always properly disabled in the case of
transfer errors. Interrupts from aborted transfers might be handled
after the data structures for the following transfer are initialised but
before the hardware is set up. This might corrupt the data structures to
the point that the system is stuck in an infinite interrupt loop (where
FIFOs are never emptied).
This patch cleanly disables the designware-i2c hardware at the end of
every transfer, successful or not.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
---
drivers/i2c/busses/i2c-designware-core.c | 14 +++---
1 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index 6c0e776..65c0c7a 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -588,10 +588,20 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
ret = wait_for_completion_interruptible_timeout(dev-cmd_complete, HZ);
if (ret == 0) {
dev_err(dev-dev, controller timed out\n);
+ /* i2c_dw_init implicitly disables the adapter */
i2c_dw_init(dev);
ret = -ETIMEDOUT;
goto done;
- } else if (ret 0)
+ }
+
+ /*
+* We must disable the adapter before unlocking the dev-lock mutex
+* below. Otherwise the hardware might continue generating interrupts
+* which in turn causes a race condition with the following transfer.
+*/
+ __i2c_dw_enable(dev, false);
+
+ if (ret 0)
goto done;
if (dev-msg_err) {
@@ -601,8 +611,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
/* no error */
if (likely(!dev-cmd_err)) {
- /* Disable the adapter */
- __i2c_dw_enable(dev, false);
ret = num;
goto done;
}
--
1.7.1
--
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
[PATCH 2/2] i2c: designware: make i2c xfers non-interruptible
When the process at the source of an i2c transfer is killed in the
middle of the transfer, the transfer is interrupted. Interrupted
transfers might cause buggy slaves on the bus (or higher level drivers)
to go haywire.
This patch forces ongoing i2c transfers to finish properly, even if the
initiating process is killed.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
---
drivers/i2c/busses/i2c-designware-core.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index 65c0c7a..d903368 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -585,7 +585,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
i2c_dw_xfer_init(dev);
/* wait for tx to complete */
- ret = wait_for_completion_interruptible_timeout(dev-cmd_complete, HZ);
+ ret = wait_for_completion_timeout(dev-cmd_complete, HZ);
if (ret == 0) {
dev_err(dev-dev, controller timed out\n);
/* i2c_dw_init implicitly disables the adapter */
--
1.7.1
--
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
[PATCH REBASE] i2c-designware: make SDA hold time configurable
This patch makes the SDA hold time configurable through device tree.
[rebased to i2c-current/i2c-next/mainline-3.10-rc1]
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
.../devicetree/bindings/i2c/i2c-designware.txt | 14 ++
drivers/i2c/busses/i2c-designware-core.c |5 +
drivers/i2c/busses/i2c-designware-core.h |1 +
drivers/i2c/busses/i2c-designware-platdrv.c|9 +
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..21fabe7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,9 @@ Recommended properties :
- clock-frequency : desired I2C bus clock frequency in Hz.
+Optional properties :
+ - sda-hold-time : should contain the SDA hold time in nanoseconds.
+
Example :
i2c@f {
@@ -20,3 +23,14 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+ i2c@112 {
+ #address-cells = 1;
+ #size-cells = 0;
+ compatible = snps,designware-i2c;
+ reg = 0x112 0x1000;
+ interrupt-parent = ictl;
+ interrupts = 12 1;
+ clock-frequency = 40;
+ sda-hold-time = 300;
+ };
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index 21fbb34..91d422c 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
#define DW_IC_STATUS 0x70
#define DW_IC_TXFLR0x74
#define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_ENABLE_STATUS0x9c
#define DW_IC_COMP_PARAM_1 0xf4
@@ -332,6 +333,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
+ /* Configure SDA Hold Time if required */
+ if (dev-sda_hold_time)
+ dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
+
/* Configure Tx/Rx FIFO threshold levels */
dw_writel(dev, dev-tx_fifo_depth - 1, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..33dfec3 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
u32 master_cfg;
unsigned inttx_fifo_depth;
unsigned intrx_fifo_depth;
+ u32 sda_hold_time;
};
#define ACCESS_SWAP0x0001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 8ec9133..589f414 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
#include linux/sched.h
#include linux/err.h
#include linux/interrupt.h
+#include linux/of.h
#include linux/of_i2c.h
#include linux/platform_device.h
#include linux/pm.h
@@ -120,6 +121,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
return PTR_ERR(dev-clk);
clk_prepare_enable(dev-clk);
+ if (pdev-dev.of_node) {
+ u32 ht;
+ u32 ic_clk = dev-get_clk_rate_khz(dev);
+
+ of_property_read_u32(pdev-dev.of_node, sda-hold-time, ht);
+ dev-sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
+ }
+
dev-functionality =
I2C_FUNC_I2C |
I2C_FUNC_10BIT_ADDR |
--
1.7.1
--
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
[PATCH v7] i2c-designware: make SDA hold time configurable
Looks like this was eaten by the spam filter last time so i'm resending
it to the lists only:
This patch makes the SDA hold time configurable through device tree.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
.../devicetree/bindings/i2c/i2c-designware.txt | 14 ++
drivers/i2c/busses/i2c-designware-core.c |5 +
drivers/i2c/busses/i2c-designware-core.h |1 +
drivers/i2c/busses/i2c-designware-platdrv.c|9 +
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..21fabe7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,9 @@ Recommended properties :
- clock-frequency : desired I2C bus clock frequency in Hz.
+Optional properties :
+ - sda-hold-time : should contain the SDA hold time in nanoseconds.
+
Example :
i2c@f {
@@ -20,3 +23,14 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+ i2c@112 {
+ #address-cells = 1;
+ #size-cells = 0;
+ compatible = snps,designware-i2c;
+ reg = 0x112 0x1000;
+ interrupt-parent = ictl;
+ interrupts = 12 1;
+ clock-frequency = 40;
+ sda-hold-time = 300;
+ };
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index 94fd818..ba40e14 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
#define DW_IC_STATUS 0x70
#define DW_IC_TXFLR0x74
#define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_COMP_PARAM_1 0xf4
#define DW_IC_COMP_TYPE0xfc
@@ -310,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
+ /* Configure SDA Hold Time if required */
+ if (dev-sda_hold_time)
+ dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
+
/* Configure Tx/Rx FIFO threshold levels */
dw_writel(dev, dev-tx_fifo_depth - 1, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..33dfec3 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
u32 master_cfg;
unsigned inttx_fifo_depth;
unsigned intrx_fifo_depth;
+ u32 sda_hold_time;
};
#define ACCESS_SWAP0x0001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0ceb6e1..0a6e29b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
#include linux/sched.h
#include linux/err.h
#include linux/interrupt.h
+#include linux/of.h
#include linux/of_i2c.h
#include linux/platform_device.h
#include linux/pm.h
@@ -136,6 +137,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
}
clk_prepare_enable(dev-clk);
+ if (pdev-dev.of_node) {
+ u32 ht;
+ u32 ic_clk = dev-get_clk_rate_khz(dev);
+
+ of_property_read_u32(pdev-dev.of_node, sda-hold-time, ht);
+ dev-sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
+ }
+
dev-functionality =
I2C_FUNC_I2C |
I2C_FUNC_10BIT_ADDR |
--
1.7.1
--
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
[PATCH v2] i2c-designware: make SDA hold time configurable
This patch makes the SDA hold time configurable through device tree.
It was tested on a hybrid of i2c/next and arc-3.8-baseline (see
https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline)
since our platform is ARC based and this architecture is not yet
supported by i2c/next.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
.../devicetree/bindings/i2c/i2c-designware.txt | 15 +++
drivers/i2c/busses/i2c-designware-core.c |5 +
drivers/i2c/busses/i2c-designware-core.h |1 +
drivers/i2c/busses/i2c-designware-platdrv.c|8
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..fb2eac8 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,10 @@ Recommended properties :
- clock-frequency : desired I2C bus clock frequency in Hz.
+Optional properties :
+ - sda-hold-time : If this property is present, the register SDA_HOLD will
+ be initialised with its value.
+
Example :
i2c@f {
@@ -20,3 +24,14 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+ i2c@112 {
+ #address-cells = 1;
+ #size-cells = 0;
+ compatible = snps,designware-i2c;
+ reg = 0x112 0x1000;
+ interrupt-parent = ictl;
+ interrupts = 12 1;
+ clock-frequency = 40;
+ sda-hold-time = 0x64;
+ };
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index 94fd818..ba40e14 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
#define DW_IC_STATUS 0x70
#define DW_IC_TXFLR0x74
#define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_COMP_PARAM_1 0xf4
#define DW_IC_COMP_TYPE0xfc
@@ -310,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
+ /* Configure SDA Hold Time if required */
+ if (dev-sda_hold_time)
+ dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
+
/* Configure Tx/Rx FIFO threshold levels */
dw_writel(dev, dev-tx_fifo_depth - 1, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..33dfec3 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
u32 master_cfg;
unsigned inttx_fifo_depth;
unsigned intrx_fifo_depth;
+ u32 sda_hold_time;
};
#define ACCESS_SWAP0x0001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index d2a33e9..9cb0100 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
#include linux/sched.h
#include linux/err.h
#include linux/interrupt.h
+#include linux/of.h
#include linux/of_i2c.h
#include linux/platform_device.h
#include linux/pm.h
@@ -136,6 +137,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
}
clk_prepare_enable(dev-clk);
+ if (pdev-dev.of_node) {
+ if (of_property_read_u32(pdev-dev.of_node, sda-hold-time,
+ dev-sda_hold_time))
+ dev-sda_hold_time = 0;
+ } else
+ dev-sda_hold_time = 0;
+
dev-functionality =
I2C_FUNC_I2C |
I2C_FUNC_10BIT_ADDR |
--
1.7.1
--
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
[PATCH v3] i2c-designware: make SDA hold time configurable
This patch makes the SDA hold time configurable through device tree.
It was tested on a hybrid of i2c/next and arc-3.8-baseline (see
https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline)
since our platform is ARC based and this architecture is not yet
supported by i2c/next.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
.../devicetree/bindings/i2c/i2c-designware.txt | 15 +++
drivers/i2c/busses/i2c-designware-core.c |5 +
drivers/i2c/busses/i2c-designware-core.h |1 +
drivers/i2c/busses/i2c-designware-platdrv.c|5 +
4 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..fb2eac8 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,10 @@ Recommended properties :
- clock-frequency : desired I2C bus clock frequency in Hz.
+Optional properties :
+ - sda-hold-time : If this property is present, the register SDA_HOLD will
+ be initialised with its value.
+
Example :
i2c@f {
@@ -20,3 +24,14 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+ i2c@112 {
+ #address-cells = 1;
+ #size-cells = 0;
+ compatible = snps,designware-i2c;
+ reg = 0x112 0x1000;
+ interrupt-parent = ictl;
+ interrupts = 12 1;
+ clock-frequency = 40;
+ sda-hold-time = 0x64;
+ };
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index 94fd818..ba40e14 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
#define DW_IC_STATUS 0x70
#define DW_IC_TXFLR0x74
#define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_COMP_PARAM_1 0xf4
#define DW_IC_COMP_TYPE0xfc
@@ -310,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
+ /* Configure SDA Hold Time if required */
+ if (dev-sda_hold_time)
+ dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
+
/* Configure Tx/Rx FIFO threshold levels */
dw_writel(dev, dev-tx_fifo_depth - 1, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..33dfec3 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
u32 master_cfg;
unsigned inttx_fifo_depth;
unsigned intrx_fifo_depth;
+ u32 sda_hold_time;
};
#define ACCESS_SWAP0x0001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index d2a33e9..cb44bae 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
#include linux/sched.h
#include linux/err.h
#include linux/interrupt.h
+#include linux/of.h
#include linux/of_i2c.h
#include linux/platform_device.h
#include linux/pm.h
@@ -136,6 +137,10 @@ static int dw_i2c_probe(struct platform_device *pdev)
}
clk_prepare_enable(dev-clk);
+ if (pdev-dev.of_node)
+ of_property_read_u32(pdev-dev.of_node, sda-hold-time,
+ dev-sda_hold_time);
+
dev-functionality =
I2C_FUNC_I2C |
I2C_FUNC_10BIT_ADDR |
--
1.7.1
--
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
[PATCH] i2c-designware: Interrupt sharing and SDA hold time
This patch implements interrupt sharing and SDA hold time configuration in
the designware i2c driver. Both functions are enabled through platform data
or device tree. Tested with Linux-3.8rc4 (Synopsys ARC port, see
https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline).
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
.../devicetree/bindings/i2c/i2c-designware.txt | 18 ++
drivers/i2c/busses/i2c-designware-core.c |6 +++
drivers/i2c/busses/i2c-designware-core.h |2 +
drivers/i2c/busses/i2c-designware-platdrv.c| 34 +++-
include/linux/i2c/i2c-designware.h | 13 +++
5 files changed, 72 insertions(+), 1 deletions(-)
create mode 100644 include/linux/i2c/i2c-designware.h
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..474386b 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,12 @@ Recommended properties :
- clock-frequency : desired I2C bus clock frequency in Hz.
+Optional properties :
+ - interrupts-shared : If this property is present, the interrupt line is
+ considered shared with other devices.
+ - sda-hold-time : If this property is present, the register SDA_HOLD will
+ be initialised with its value.
+
Example :
i2c@f {
@@ -20,3 +26,15 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+ i2c@112 {
+ #address-cells = 1;
+ #size-cells = 0;
+ compatible = snps,designware-i2c;
+ reg = 0x112 0x1000;
+ interrupt-parent = ictl;
+ interrupts = 12 1;
+ interrupts-shared;
+ clock-frequency = 40;
+ sda-hold-time = 0x64;
+ };
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index cbba7db..36ba0b5 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -34,6 +34,7 @@
#include linux/io.h
#include linux/pm_runtime.h
#include linux/delay.h
+#include linux/i2c/i2c-designware.h
#include i2c-designware-core.h
/*
@@ -66,6 +67,7 @@
#define DW_IC_STATUS 0x70
#define DW_IC_TXFLR0x74
#define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_COMP_PARAM_1 0xf4
#define DW_IC_COMP_TYPE0xfc
@@ -309,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
+ /* Configure SDA Hold Time if required */
+ if (dev-pdat_flags I2C_DW_PDAT_FLAGS_SET_HOLDTIME)
+ dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
+
/* Configure Tx/Rx FIFO threshold levels */
dw_writel(dev, dev-tx_fifo_depth - 1, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..ef20477 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,8 @@ struct dw_i2c_dev {
u32 master_cfg;
unsigned inttx_fifo_depth;
unsigned intrx_fifo_depth;
+ u32 sda_hold_time;
+ unsigned intpdat_flags;
};
#define ACCESS_SWAP0x0001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 343357a..2ad2da3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,11 +34,13 @@
#include linux/sched.h
#include linux/err.h
#include linux/interrupt.h
+#include linux/of.h
#include linux/of_i2c.h
#include linux/platform_device.h
#include linux/pm.h
#include linux/io.h
#include linux/slab.h
+#include linux/i2c/i2c-designware.h
#include i2c-designware-core.h
static struct i2c_algorithm i2c_dw_algo = {
@@ -56,6 +58,28 @@ static int dw_i2c_probe(struct platform_device *pdev)
struct i2c_adapter *adap;
struct resource *mem, *ioarea;
int irq, r;
+ unsigned long irqf;
+ struct i2c_designware_platform_data *pdat = pdev-dev.platform_data;
+
+ if ((!pdat) pdev-dev.of_node) {
+ struct device_node *node = pdev-dev.of_node;
+
+ pdat = kmalloc(sizeof(struct i2c_designware_platform_data),
+ GFP_KERNEL);
+ if (!pdat)
+ return -ENOMEM;
+
+ pdat-flags = 0
Re: [PATCH] i2c-designware: Interrupt sharing and SDA hold time
On Fri, Feb 15, 2013 at 01:50:37PM +0200, Mika Westerberg wrote:
On Fri, Feb 15, 2013 at 11:11:50AM +0100, Christian Ruppert wrote:
This patch implements interrupt sharing and SDA hold time configuration in
the designware i2c driver. Both functions are enabled through platform data
or device tree. Tested with Linux-3.8rc4 (Synopsys ARC port, see
https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline).
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
---
.../devicetree/bindings/i2c/i2c-designware.txt | 18 ++
drivers/i2c/busses/i2c-designware-core.c |6 +++
drivers/i2c/busses/i2c-designware-core.h |2 +
drivers/i2c/busses/i2c-designware-platdrv.c| 34
+++-
include/linux/i2c/i2c-designware.h | 13 +++
5 files changed, 72 insertions(+), 1 deletions(-)
create mode 100644 include/linux/i2c/i2c-designware.h
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..474386b 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,12 @@ Recommended properties :
- clock-frequency : desired I2C bus clock frequency in Hz.
+Optional properties :
+ - interrupts-shared : If this property is present, the interrupt line is
+ considered shared with other devices.
+ - sda-hold-time : If this property is present, the register SDA_HOLD will
+ be initialised with its value.
+
Example :
i2c@f {
@@ -20,3 +26,15 @@ Example :
interrupts = 11;
clock-frequency = 40;
};
+
+ i2c@112 {
+ #address-cells = 1;
+ #size-cells = 0;
+ compatible = snps,designware-i2c;
+ reg = 0x112 0x1000;
+ interrupt-parent = ictl;
+ interrupts = 12 1;
+ interrupts-shared;
+ clock-frequency = 40;
+ sda-hold-time = 0x64;
+ };
diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index cbba7db..36ba0b5 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -34,6 +34,7 @@
#include linux/io.h
#include linux/pm_runtime.h
#include linux/delay.h
+#include linux/i2c/i2c-designware.h
#include i2c-designware-core.h
/*
@@ -66,6 +67,7 @@
#define DW_IC_STATUS 0x70
#define DW_IC_TXFLR0x74
#define DW_IC_RXFLR0x78
+#define DW_IC_SDA_HOLD 0x7c
#define DW_IC_TX_ABRT_SOURCE 0x80
#define DW_IC_COMP_PARAM_1 0xf4
#define DW_IC_COMP_TYPE0xfc
@@ -309,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
dev_dbg(dev-dev, Fast-mode HCNT:LCNT = %d:%d\n, hcnt, lcnt);
+ /* Configure SDA Hold Time if required */
+ if (dev-pdat_flags I2C_DW_PDAT_FLAGS_SET_HOLDTIME)
+ dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
AFAIK, DW_IC_SDA_HOLD minimum value is 1 so you could just do:
if (dev-sda_hold_time)
dw_writel(dev, dev-sda_hold_time, DW_IC_SDA_HOLD);
This is correct, I'll modify it.
Also should this be configurable for standard and fast modes both?
Something like sm_sda_hold_time and fm_sda_hold_time?
AFAIK the requirements for data hold time do not differ between standard
and fast mode. The hardware seems to have been designed in the same
understanding since it provides two separate register sets for clock
timings in standard and fast modes but only one register for data hold
time. Am I missing something here?
+
/* Configure Tx/Rx FIFO threshold levels */
dw_writel(dev, dev-tx_fifo_depth - 1, DW_IC_TX_TL);
dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h
b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..ef20477 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,8 @@ struct dw_i2c_dev {
u32 master_cfg;
unsigned inttx_fifo_depth;
unsigned intrx_fifo_depth;
+ u32 sda_hold_time;
+ unsigned intpdat_flags;
};
#define ACCESS_SWAP0x0001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index 343357a..2ad2da3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,11 +34,13 @@
#include linux/sched.h
#include linux/err.h
#include linux
