Re: [PATCH] i2c: designware: use enable on resume instead initialization
Dear Lucas, "De Marchi, Lucas" 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 wrote on 23.06.2015 19:02:03: > > > > On Tue, Jun 23, 2015 at 1:45 PM, 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
Dear Lucas, Lucas De Marchi wrote on 23.06.2015 19:02:03: > On Tue, Jun 23, 2015 at 1:45 PM, 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
Hello, Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16: > Mika Westerberg 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 > > > wrote: > > > > On Mon, Jun 08, 2015 at 02:50:28PM -0300, > lucas.de.mar...@gmail.com wrote: > > > >> From: Fabio Mello > > > >> > > > >> 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 > > > >> Signed-off-by: Lucas De Marchi > > > >> --- > > > >> > > > >> 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 nev
Re: [PATCH v2] i2c: designware: Suppress error message if platform_get_irq() < 0
Alexey Brodkin wrote on 03/09/2015 10:03:12
AM:
> From: Alexey Brodkin
> To: linux-i2c@vger.kernel.org,
> Cc: linux-ker...@vger.kernel.org, Alexey Brodkin
> , Vineet Gupta
> , Christian Ruppert
> , Mika Westerberg
> , Wolfram Sang ,
> Andy Shevchenko
> 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
> Cc: Vineet Gupta
> Cc: Christian Ruppert
Acked-by: Christian Ruppert
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
> Cc: Wolfram Sang
> Cc: Andy Shevchenko
> ---
> 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
-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: [PATCH] i2c: designware: Suppress error message if platform_get_irq() returns -EPROBE_DEFER
Hello Alexey,
Alexey Brodkin wrote on 03/03/2015 05:37:31
PM:
> From:
>
> Alexey Brodkin
>
> To:
>
> "christian.rupp...@alitech.com" ,
>
> Cc:
>
> "mika.westerb...@linux.intel.com" ,
> "linux-ker...@vger.kernel.org" ,
> "vineet.gup...@synopsys.com" , "wsa@the-
> dreams.de" , "andriy.shevche...@linux.intel.com"
> , "linux-i2c@vger.kernel.org"
> , "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
> Cc: Vineet Gupta
> Cc: Christian Ruppert
> Cc: Mika Westerberg
> Cc: Wolfram Sang
> ---
> 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: 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
Hi Vineet, On Fri, Sep 26, 2014 at 09:20:20AM +0530, Vineet Gupta wrote: > Hi Chrisitian, > > On Monday 22 September 2014 10:53 PM, Christian Ruppert wrote: > > 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. > > I still ended up moving COMMON_CLK out of tb10x to arch/arc - due to > consolidation > of .init_machine() callbacks - from platform to arc core. > > Give linux-next a spin (perhaps later today as I just pushed a build error in > that > area). > > You'll like the almost empty platform file :-) This looks very nice indeed. Support for "standard" platforms is simple and elegant. The fact that most platforms won't need any specific support code at all (apart from drivers) goes well with the idea of device tree. However, the kernel-doc comment for init_machine in mach_desc.h is now slightly confusing (still mentioning device tree). 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? 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 , /| 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 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 > > > > > > 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 > > > Signed-off-by: Mika Westerberg > > > --- > > > > 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 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 exist
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 , /| 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 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: > On 7/22/13 10:17 PM, Christian Ruppert wrote:> 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. > > Please find my comments below. > > >/* > > *t_f;SDA > > * |-| > > * _ _ . . . > > * \/ > > * SDA \ / > > *\/ t_r;SCLt_f;SCL > > * |-||-| > > * __ > > * \ /\ > > * SCL\ / \ > > * \___
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; /*
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 , /| 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 , /| 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
lcnt = i2c_dw_scl_lcnt(input_clock_khz,
> + 13, /* tLOW = 1.3 us */
> + 3, /* tf = 0.3 us */
> + 0); /* No offset */
> + }
> dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
> dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
> dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", 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 ,
/|
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 v10] i2c-designware: make SDA hold time configurable
On Wed, Jul 03, 2013 at 04:44:09PM +0200, Arnd Bergmann wrote: > On Wednesday 03 July 2013, Christian Ruppert wrote: > > On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote: > > > On Wednesday 03 July 2013, Christian Ruppert wrote: > > > > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote: > > > > > On Wednesday 26 June 2013, Wolfram Sang wrote: > > > > > > On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote: > > > > > > > This patch makes the SDA hold time configurable through device > > > > > > > tree. > > > > > > > > > > > > > > Signed-off-by: Christian Ruppert > > > > > > > Signed-off-by: Pierrick Hascoet > > > > > > > > > > > > Applied to for-next, thanks for keeping at it and providing lots of > > > > > > useful information. Much appreciated! > > > > > > > > > > Sorry, but I got a regression that I didn't find reported elsewhere > > > > > so far, even though it breaks a lot of the ARM defconfig builds: > > > > > > > > > > drivers/built-in.o: In function `dw_i2c_probe': > > > > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: > > > > > undefined reference to `__udivdi3' > > > > > > > > > > I suspect you want something like the change below. > > > > > > > > This looks similar to a patch Vincent Stehle submitted yesterday, see > > > > https://lkml.org/lkml/2013/7/2/145 > > > > > > Thanks for the link. Actually his patch looks wrong to me, because > > > > > > dev->sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100); > > > > > > assigns the division remainder to sda_hold_time, not the quotient. > > > > Hrmmm... At least when I tested it this morning on an ARC architecture > > it worked as intended and returned the quotient. Does that mean we have > > an issue with this function on ARC? Can anyone who knows these functions > > better than I comment? > > ARC just uses the generic version of div_u64, which is defined in lib/div64.c. > > I suspect that the division remainder just happens to work well enough for > you to not cause any run-time error. You could try adding a printk > in that function to show the values you get on ARC. That's what I did and they were identical to the original values calculated with /. I just looked at include/linux/math64.h and found the following comment: /** * div_u64 - unsigned 64bit divide with 32bit divisor * * This is the most common 64bit divide and should be used if possible, * as many 32bit archs can optimize this variant better than a full 64bit * divide. */ Although this doesn't explicitly state what the function returns to me this sounds more like the quotient is returned rather than the remainder? -- Christian Ruppert , /| 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 v10] i2c-designware: make SDA hold time configurable
On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote: > On Wednesday 03 July 2013, Christian Ruppert wrote: > > On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote: > > > On Wednesday 26 June 2013, Wolfram Sang wrote: > > > > On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote: > > > > > This patch makes the SDA hold time configurable through device tree. > > > > > > > > > > Signed-off-by: Christian Ruppert > > > > > Signed-off-by: Pierrick Hascoet > > > > > > > > Applied to for-next, thanks for keeping at it and providing lots of > > > > useful information. Much appreciated! > > > > > > Sorry, but I got a regression that I didn't find reported elsewhere > > > so far, even though it breaks a lot of the ARM defconfig builds: > > > > > > drivers/built-in.o: In function `dw_i2c_probe': > > > /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined > > > reference to `__udivdi3' > > > > > > I suspect you want something like the change below. > > > > This looks similar to a patch Vincent Stehle submitted yesterday, see > > https://lkml.org/lkml/2013/7/2/145 > > Thanks for the link. Actually his patch looks wrong to me, because > > dev->sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100); > > assigns the division remainder to sda_hold_time, not the quotient. Hrmmm... At least when I tested it this morning on an ARC architecture it worked as intended and returned the quotient. Does that mean we have an issue with this function on ARC? Can anyone who knows these functions better than I comment? Greetings, Christian -- Christian Ruppert , /| 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 v10] i2c-designware: make SDA hold time configurable
On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
> On Wednesday 26 June 2013, Wolfram Sang wrote:
> > On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
> > > This patch makes the SDA hold time configurable through device tree.
> > >
> > > Signed-off-by: Christian Ruppert
> > > Signed-off-by: Pierrick Hascoet
> >
> > Applied to for-next, thanks for keeping at it and providing lots of
> > useful information. Much appreciated!
>
> Sorry, but I got a regression that I didn't find reported elsewhere
> so far, even though it breaks a lot of the ARM defconfig builds:
>
> drivers/built-in.o: In function `dw_i2c_probe':
> /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined
> reference to `__udivdi3'
>
> I suspect you want something like the change below.
This looks similar to a patch Vincent Stehle submitted yesterday, see
https://lkml.org/lkml/2013/7/2/145
> Signed-off-by: Arnd Bergmann
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index def79b5..57e3a07 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -119,10 +119,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
> if (pdev->dev.of_node) {
> u32 ht = 0;
> u32 ic_clk = dev->get_clk_rate_khz(dev);
> + u64 hold_time;
>
> of_property_read_u32(pdev->dev.of_node,
> "i2c-sda-hold-time-ns", &ht);
> - dev->sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
> + hold_time = (u64)ic_clk * ht + 50;
> + do_div(hold_time, 100);
> + dev->sda_hold_time = hold_time;
> }
>
> dev->functionality =
--
Christian Ruppert ,
/|
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 > Signed-off-by: Vincent Stehlé > Cc: Wolfram Sang > Cc: Christian Ruppert > Cc: Pierrick Hascoet > --- > > > 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 , /| 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
Signed-off-by: Pierrick Hascoet
---
.../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) {
+
[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
Signed-off-by: Pierrick Hascoet
---
.../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_S
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 , /| 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 > > > > Signed-off-by: Pierrick Hascoet > > > > > > 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.
[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
Signed-off-by: Pierrick Hascoet
---
.../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-
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 , /| 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 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 , /| 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 > > > > 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 , /| 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 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 > --- > 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 , /| 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 > > Signed-off-by: Pierrick Hascoet > > 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 , /| 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: [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 , /| 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 > --- > 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 , /| 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: 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
Reviewed-by: Christian Ruppert
>
> ---
> 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 ,
/|
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
---
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: [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 , /| 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 2/2] i2c: designware: make i2c xfers non-interruptible
On Fri, Jun 07, 2013 at 08:25:55AM +0300, Mika Westerberg wrote: > On Thu, Jun 06, 2013 at 03:43:36PM +0200, Christian Ruppert wrote: > > 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. > > I already sent a similar patch ;-) > > https://patchwork.kernel.org/patch/2601241/ > > However, it is not yet picked by Wolfram. Oops, this looks like a human race condition :) I checked main line and Wolfram's git but didn't think of checking patchwork. I'll rebase the second patch on yours. -- Christian Ruppert , /| 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 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
---
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 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
---
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 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
Signed-off-by: Pierrick Hascoet
---
.../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
#include
#include
+#include
#include
#include
#include
@@ -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
Signed-off-by: Pierrick Hascoet
---
.../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
#include
#include
+#include
#include
#include
#include
@@ -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 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
Signed-off-by: Pierrick Hascoet
---
.../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
#include
#include
+#include
#include
#include
#include
@@ -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 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
Signed-off-by: Pierrick Hascoet
---
.../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
#include
#include
+#include
#include
#include
#include
@@ -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
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
> > Signed-off-by: Pierrick Hascoet
> > ---
> > .../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
> > #include
> > #include
> > +#include
> > #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
> > -
[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
Signed-off-by: Pierrick Hascoet
---
.../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
#include
#include
+#include
#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
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
#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;
+
+ if (of_get_property(node, "interrupts-shared", NULL))
+ pdat->fla
