[PATCH] i2c: core: fix wakeup irq parsing

[Grygorii Strashko]

This patch fixes obvious copy-past error in wake up irq parsing
code which leads to the fact that dev_pm_set_wake_irq() will
be called with wrong IRQ number when "wakeup" IRQ is not
defined in DT.

Cc: Dmitry Torokhov <dmitry.torok...@gmail.com>
Cc: Felipe Balbi <ba...@ti.com>
Cc: <sta...@vger.kernel.org> # v4.3
Fixes: 3fffd1283927 ("i2c: allow specifying separate wakeup interrupt in device 
tree")
Signed-off-by: Grygorii Strashko <grygorii.stras...@ti.com>
---
 drivers/i2c/i2c-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 040af5c..ba8eb08 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -715,7 +715,7 @@ static int i2c_device_probe(struct device *dev)
if (wakeirq > 0 && wakeirq != client->irq)
status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
else if (client->irq > 0)
-   status = dev_pm_set_wake_irq(dev, wakeirq);
+   status = dev_pm_set_wake_irq(dev, client->irq);
else
status = 0;
 
-- 
2.6.3

--
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: Alternative approach to solve the deferred probe

[Grygorii Strashko]

Hi Russell,

On 10/21/2015 09:28 PM, Russell King - ARM Linux wrote:
> On Wed, Oct 21, 2015 at 09:13:48PM +0300, Grygorii Strashko wrote:
>> But I worry a bit (and that my main point) about these few additional
>> rounds of deferred device probing which I have right now and which allows
>> some of drivers to finish, finally, their probes successfully.
>> With proposed change I'll get more messages in boot log, but some of
>> them will belong to drivers which have been probed successfully and so,
>> they will be not really useful.
> 
> Then you haven't properly understood my proposal.
> 
> I want to get rid of all the "X deferred its probing" messages up until
> the point that we set the "please report deferred probes" flag.
> 
> That _should_ mean that all the deferred probing that goes on becomes
> _totally_ silent and becomes hidden (unless you really want to see it,
> in which case we can make a debug option which turns it on) up until
> we're at the point where we want to enter userspace.
> 
> At that point, we then report into the kernel log which devices are
> still deferring and, via appropriately placed dev_warn_deferred(),
> the reasons why the devices are being deferred.
> 
> So, gone will be all the messages earlier in the log about device X
> not having a GPIO/clock/whatever because the device providing the
> GPIO/clock/whatever hasn't been probed.
> 
> If everything is satisfied by the time we run this last round (again,
> I'm not using a three line sentence to describe exactly what I mean,
> I'm sure you know by now... oops, I just did) then the kernel will
> report nothing about any deferrals.  That's _got_ to be an improvement.

Sorry Master, but you really don't need to spend so much time typing the
same things three times  - I understand what are you trying to do :(

I did my comments with assumption that it's not officially prohibited/deprecated
to register drivers (and execute probes) from late_initcall() layer
(just recommended) and there are still bunch of drivers which are doing this.
Now I see that it's not a recommendation any more, and deferred_probe_initcall()
might be a good place to activate driver_deferred_probe_report if goal is to
identify and fix such drivers.

Sorry for your time.

> 
>>
>> As result, I think, the most important thing is to identify (or create)
>> some point during kernel boot when it will be possible to say that all
>> built-in drivers (at least) finish their probes 100% (done or defer).
>>
>> Might be do_initcalls() can be updated (smth like this):
>> static void __init do_initcalls(void)
>> {
>>  int level;
>>
>>  for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++)
>>  do_initcall_level(level);
>>
>> +wait_for_device_probe();
>> +/* Now one final round, reporting any devices that remain deferred */
>> +driver_deferred_probe_report = true;
>> +driver_deferred_probe_trigger();
>> +wait_for_device_probe();
>> }
>>
>> Also, in my opinion, it will be useful if this debugging feature will be
>> optional.
> 
> I wonder why you want it optional... so I'm going to guess and cover
> both cases I can think of below to head off another round of reply on
> this point (sorry if this sucks eggs.)
> 
> I don't see it as being optional, because it's going to be cheap to run
> in the case of a system which has very few or no errors - which is what
> you should have for production systems, right?
> 

Also, I've spend some time today testing your proposal - hope you'll find 
results
useful.

I've applied truncated version of your patch (diff below) on TI's 4.1 kernel and
run few tests (log is below) on dra7-evm/am43xx-gpevm - K4.1 is not far away 
from LKML,
so I assume this test is valid. Overall boot process consists from two stages:
kernel boot and modules loading. 

My Changes:
 - only really_probe() modified to show deferred device/drivers 

>From the log I can see additional messages in log when modules are loading,
because driver_deferred_probe_report is still true - dwc3 probes were deferred,
but then finally succeeded.

So, as you've mentioned, it seems a good thing to deactivate 
driver_deferred_probe_report and
provide user with ability to turn it on again (and probably re-trigger deferred
device probing).

I've found no issues during Kernel boot (built-in) time, new messages are 
displayed only
if probe is failed for some drivers:
[3.219700] == deferred_probe_initcalll
[3.226820] platform omapdrm.0: Driver omapdrm requests probe deferral
[3.233378] platform omapdrm.0: deferring probe:  Driver omapdrm 
requests probe deferral
[3.242084] dra7evm-tpd12s015 encoder@

Re: Alternative approach to solve the deferred probe

[Grygorii Strashko]

On 10/21/2015 06:36 PM, Frank Rowand wrote:
> On 10/21/2015 1:18 AM, Russell King - ARM Linux wrote:
>> On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
>>> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
> 
> < snip >
> 
 +
   static bool driver_deferred_probe_enable = false;
 +
   /**
* driver_deferred_probe_trigger() - Kick off re-probing deferred devices
*
 @@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
driver_deferred_probe_trigger();
>>>
>>> Couldn't you put the "driver_deferred_probe_report = true" here?  And then
>>> not add another round of probes.
>>
>> The idea is not to report anything for drivers that were deferred
>> during the normal bootup.  The above is part of the normal bootup,
>> and the deferred activity should not be warned about.
> 
> The above is currently the last point for probe to succeed or defer
> (until possibly, as you mentioned, module loading resolves the defer).
> If a probe defers above, it will defer again below.  The set of defers
> should be exactly the same above and below.
> 

Unfortunately this is not "the last point for probe to succeed or defer".
There are still a bunch of drivers in Kernel which will be probed at 
late_initcall() level.
(like ./drivers/net/ethernet/ti/cpsw.c => late_initcall(cpsw_init);
Yes - they probably need to be updated to use module_init(), but that's what
we have now). Those drivers will re-trigger deferred device probing if their
probe succeeded.

As result, it is impossible to say when will it happen the 
"final round of deferred device probing" :( and final list of drivers which
was "deferred forever" will be know only when kernel exits to User space 
("deferred forever" - before loading modules).

May be, we also can consider adding debug_fs entry which can be used to display
actual state of deferred_probe_pending_list? 

>>
>> If we have any devices still deferring after _this_ round, that must
>> indicate that some resource they want is not available, and that
>> should be warned about.
>>
>> Of course, modules can defer too - and I made some suggestions in my
>> waffle above the patch about that.
>>
> 
> < adding back trimmed, for fuller context >
> 
/* Sort as many dependencies as possible before exiting initcalls */
flush_workqueue(deferred_wq);
 +
 +  /* Now one final round, reporting any devices that remain deferred */
 +  driver_deferred_probe_report = true;
 +  driver_deferred_probe_trigger();
 +  /* Sort as many dependencies as possible before exiting initcalls */
 +  flush_workqueue(deferred_wq);
 +
return 0;
   }


-- 
regards,
-grygorii
--
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: Alternative approach to solve the deferred probe

[Grygorii Strashko]

On 10/21/2015 09:02 PM, Frank Rowand wrote:
> On 10/21/2015 9:55 AM, Grygorii Strashko wrote:
>> On 10/21/2015 06:36 PM, Frank Rowand wrote:
>>> On 10/21/2015 1:18 AM, Russell King - ARM Linux wrote:
>>>> On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
>>>>> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
>>>
>>> < snip >
>>>
>>>>>> +
>>>>>>static bool driver_deferred_probe_enable = false;
>>>>>> +
>>>>>>/**
>>>>>> * driver_deferred_probe_trigger() - Kick off re-probing deferred 
>>>>>> devices
>>>>>> *
>>>>>> @@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
>>>>>>  driver_deferred_probe_trigger();
>>>>>
>>>>> Couldn't you put the "driver_deferred_probe_report = true" here?  And then
>>>>> not add another round of probes.
>>>>
>>>> The idea is not to report anything for drivers that were deferred
>>>> during the normal bootup.  The above is part of the normal bootup,
>>>> and the deferred activity should not be warned about.
>>>
>>> The above is currently the last point for probe to succeed or defer
>>> (until possibly, as you mentioned, module loading resolves the defer).
>>> If a probe defers above, it will defer again below.  The set of defers
>>> should be exactly the same above and below.
>>>
>>
>> Unfortunately this is not "the last point for probe to succeed or defer".
>> There are still a bunch of drivers in Kernel which will be probed at 
>> late_initcall() level.
>> (like ./drivers/net/ethernet/ti/cpsw.c => late_initcall(cpsw_init);
> 
> Yes, cpsw_init() should _not_ be a late_initcall.  This is yet another
> example of playing games with ordering probes that we have been trying
> to eliminate.

yes, we're trying to solve such issues and have all TI's drivers initialized
from module_init() level, but as usual this process is not so fast. 

You know, some times ago there was no other way to solve boot ordering issues,
but only to play with init levels :) And, as result, right now in drivers/
and sound/ folders there are >77 occurrences of late_initcall(). 

> 
> Thanks for pointing out one of the resulting problems this causes for the
> deferred probe mechanism.
> 
>> Yes - they probably need to be updated to use module_init(), but that's what
>> we have now). Those drivers will re-trigger deferred device probing if their
>> probe succeeded.
> 
> Yes, if cpsw_init() leads to a successful probe, then deferred device probing
> will be re-triggered.  I do not know if cpsw_init() will be called before or
> after deferred_probe_initcall().  The general initcall mechanism does not
> provide any ordering guarantees between the two functions because they are
> at the same initcall level.

It will be called after and it will re-triggered deferred device probing.
Now ordering of init calls will be specified by drivers/Makefile
which itself is funny thing.

> 
>>
>> As result, it is impossible to say when will it happen the
>> "final round of deferred device probing" :( and final list of drivers which
>> was "deferred forever" will be know only when kernel exits to User space
>> ("deferred forever" - before loading modules).
>>
>> May be, we also can consider adding debug_fs entry which can be used to 
>> display
>> actual state of deferred_probe_pending_list?
>>
>>>>
>>>> If we have any devices still deferring after _this_ round, that must
>>>> indicate that some resource they want is not available, and that
>>>> should be warned about.
>>>>
>>>> Of course, modules can defer too - and I made some suggestions in my
>>>> waffle above the patch about that.
>>>>
>>>
>>> < adding back trimmed, for fuller context >
>>>
>>>>>>  /* Sort as many dependencies as possible before exiting 
>>>>>> initcalls */
>>>>>>  flush_workqueue(deferred_wq);
>>>>>> +
>>>>>> +/* Now one final round, reporting any devices that remain 
>>>>>> deferred */
>>>>>> +driver_deferred_probe_report = true;
>>>>>> +driver_deferred_probe_trigger();
>>>>>> +/* Sort as many dependencies as possible before exiting 
>>>>>> initcalls */
>>>>>> +flush_workqueue(deferred_wq);
>>>>>> +
>>>>>>  return 0;
>>>>>>}
>>
>>
> 


-- 
regards,
-grygorii
--
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: Alternative approach to solve the deferred probe

[Grygorii Strashko]

Hi Russell,

On 10/21/2015 08:20 PM, Russell King - ARM Linux wrote:
> On Wed, Oct 21, 2015 at 07:55:29PM +0300, Grygorii Strashko wrote:
>> On 10/21/2015 06:36 PM, Frank Rowand wrote:
>>> The above is currently the last point for probe to succeed or defer
>>> (until possibly, as you mentioned, module loading resolves the defer).
>>> If a probe defers above, it will defer again below.  The set of defers
>>> should be exactly the same above and below.
>>>
>>
>> Unfortunately this is not "the last point for probe to succeed or defer".
> 
> Of course it isn't.  Being pedantic, there's actually no such thing,
> because the point that the kernel as finished booting can never actually
> be determined with things like modules being present.  That's something
> I've acknowledged from the start of this.
> 
>> There are still a bunch of drivers in Kernel which will be probed at 
>> late_initcall() level.
>> (like ./drivers/net/ethernet/ti/cpsw.c => late_initcall(cpsw_init);
>> Yes - they probably need to be updated to use module_init(), but that's what
>> we have now). Those drivers will re-trigger deferred device probing if their
>> probe succeeded.
> 
> Maybe this particular late_initcall() which triggers off the deferred
> probing should be moved to its own really_late_initcall() which happens
> as the very last thing - I think this is intended to run after everything
> else has had a chance to probe once.
> 
>> As result, it is impossible to say when will it happen the
>> "final round of deferred device probing" :( and final list of drivers
>> which was "deferred forever" will be know only when kernel exits to
>> User space  ("deferred forever" - before loading modules).
>>
>> May be, we also can consider adding debug_fs entry which can be used to
>> display actual state of deferred_probe_pending_list?
> 
> There are complaints in this thread about the existing deferred probing
> implementation being hard to debug - where it's known that a device
> has deferred, but it's not known why that happened.
> 
> That would be solved by my proposal, as this final round of probing
> before entering userspace after _all_ normal device probes have been
> attempted once and then we've tried to satisfy the deferred probe
> (okay, that's what it's _supposed_ to be - and as it takes three lines
> to write it, you'll excuse me if I just use the abbreviated "final
> round of deferred probe" which is much shorter - but remember that
> the long version is what I actually mean) would produce a list of
> not only the devices that failed to probe, but also the cause of the
> deferred probes.
> 
> My proposal would ensure that subsystems are happier to add these
> prints, because in the normal scenario where we have deferred probing,
> we're not littering the console log with lots of useless failure
> messages which make people stop and think "now did device X probe?"
> It also means scripts in our boot farms can more effectively analyse
> the log and determine whether the boot was actually successful and
> contained no errors.
> 
> Merely printing the list of devices which have been deferred is next
> to useless.  The next question will always be "why did device X defer?"
> and if that can't be answered, it means people having to spend a long
> time adding lots of printks to the kernel at lots of -EPROBE_DEFER
> returning sites or in the relevant drivers, tracing through the code
> back towards the -EPROBE_DEFER sites to try and track it down.
> 

I perfectly understand your proposal and spent a lot of time trying to
debug such kind issues also (and using printks).  

But I worry a bit (and that my main point) about these few additional
rounds of deferred device probing which I have right now and which allows
some of drivers to finish, finally, their probes successfully.
With proposed change I'll get more messages in boot log, but some of
them will belong to drivers which have been probed successfully and so,
they will be not really useful.

As result, I think, the most important thing is to identify (or create)
some point during kernel boot when it will be possible to say that all
built-in drivers (at least) finish their probes 100% (done or defer).

Might be do_initcalls() can be updated (smth like this):
static void __init do_initcalls(void)
{
int level;

for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++)
do_initcall_level(level);

+   wait_for_device_probe();
+   /* Now one final round, reporting any devices that remain deferred */
+   driver_deferred_probe_report = true;
+   driver_deferred_probe_trigger();
+   wait_for_device_probe();
}

Also, in my opinion, it will be useful if this debugging feature will be 
optional.

Thanks.
-- 
regards,
-grygorii
S/ILKP
--
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] dts: keystone: Use new "ti,keystone-i2c" compatible

[Grygorii Strashko]

On 09/14/2015 12:07 PM, Alexander Sverdlin wrote:

Now as "i2c-davinci" driver has special handling for Keystone it's time to 
switch
the device tree to use new "compatible" property. Old one is left for backwards-
compatibility.

Signed-off-by: Alexander Sverdlin 
---


To: Santosh
Cc: Murali, Sekhar

Seems this patch is Keystone 2 specific.



Changes in v2:
- As suggested by Mark Rutland, kept the old "compatible" for backwards-
   compatibility

  arch/arm/boot/dts/keystone.dtsi |6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
index 72816d6..abd7455 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -106,7 +106,7 @@
};

i2c0: i2c@253 {
-   compatible = "ti,davinci-i2c";
+   compatible = "ti,keystone-i2c", "ti,davinci-i2c";
reg = <0x0253 0x400>;
clock-frequency = <10>;
clocks = <>;
@@ -116,7 +116,7 @@
};

i2c1: i2c@2530400 {
-   compatible = "ti,davinci-i2c";
+   compatible = "ti,keystone-i2c", "ti,davinci-i2c";
reg = <0x02530400 0x400>;
clock-frequency = <10>;
clocks = <>;
@@ -126,7 +126,7 @@
};

i2c2: i2c@2530800 {
-   compatible = "ti,davinci-i2c";
+   compatible = "ti,keystone-i2c", "ti,davinci-i2c";
reg = <0x02530800 0x400>;
clock-frequency = <10>;
clocks = <>;




--
regards,
-grygorii
--
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] dts: keystone: Use new "ti,keystone-i2c" compatible

[Grygorii Strashko]

On 10/20/2015 06:06 PM, Wolfram Sang wrote:

On Mon, Sep 14, 2015 at 11:07:02AM +0200, Alexander Sverdlin wrote:

Now as "i2c-davinci" driver has special handling for Keystone it's time to 
switch
the device tree to use new "compatible" property. Old one is left for backwards-
compatibility.

Signed-off-by: Alexander Sverdlin 


Shall I take this one or shall it go via the davinci tree?



To: Sekhar

--
regards,
-grygorii
--
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 1/2] i2c: davinci: Optimize clock generation on Keystone SoC

[Grygorii Strashko]

On 08/21/2015 12:46 PM, Alexander Sverdlin wrote:

According to "KeyStone Architecture Inter-IC Control Bus User Guide", fixed
additive part of frequency divisors (referred as "d" in the code and datasheet)
always equals to 6, independent of module clock prescaler.

  module clock frequency
master clock frequency = --
  (ICCL + 6) + (ICCH + 6)

It was not the case with original Davinci IP. Introduce new compatible property
"ti,keystone-i2c", which triggers special handling in the driver.

Without this change Keystone-based systems (having 204.8MHz input clock) choose
prescaler 29 (PSC=28). Using d=5 in this case leads to bus bitrate ~353kHz
instead of requested 400kHz. After correction, assuming d=6 bus rate is ~392kHz.
This gives ~11% transfer rate increase.


Thanks Alexander.
Reviewed-by: Grygorii Strashko <grygorii.stras...@ti.com>



Signed-off-by: Alexander Sverdlin <alexander.sverd...@nokia.com>
Tested-by: Hemanth Guruva Reddy <hemanth.guruva_re...@nokia.com>
Tested-by: Lukasz Gemborowski <lukasz.gemborow...@nokia.com>
---
  .../devicetree/bindings/i2c/i2c-davinci.txt|6 +++---
  drivers/i2c/busses/i2c-davinci.c   |8 
  2 files changed, 11 insertions(+), 3 deletions(-)

Changes in v2:
- Introducing new "compatible" property "ti,keystone-i2c" instead of guessing
   silicon revision from ID registers

diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt 
b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
index a4e1cbc..5b123e0 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
@@ -1,10 +1,10 @@
-* Texas Instruments Davinci I2C
+* Texas Instruments Davinci/Keystone I2C

  This file provides information, what the device node for the
-davinci i2c interface contain.
+davinci/keystone i2c interface contains.

  Required properties:
-- compatible: "ti,davinci-i2c";
+- compatible: "ti,davinci-i2c" or "ti,keystone-i2c";
  - reg : Offset and length of the register set for the device

  Recommended properties :
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 3fbb9a0..c5628a4 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -181,6 +181,7 @@ static void i2c_davinci_calc_clk_dividers(struct 
davinci_i2c_dev *dev)
u32 clkh;
u32 clkl;
u32 input_clock = clk_get_rate(dev->clk);
+   struct device_node *of_node = dev->dev->of_node;

/* NOTE: I2C Clock divider programming info
 * As per I2C specs the following formulas provide prescaler
@@ -196,6 +197,9 @@ static void i2c_davinci_calc_clk_dividers(struct 
davinci_i2c_dev *dev)
 * where if PSC == 0, d = 7,
 *   if PSC == 1, d = 6
 *   if PSC > 1 , d = 5
+*
+* Note:
+* d is always 6 on Keystone I2C controller
 */

/* get minimum of 7 MHz clock, but max of 12 MHz */
@@ -204,6 +208,9 @@ static void i2c_davinci_calc_clk_dividers(struct 
davinci_i2c_dev *dev)
psc++;  /* better to run under spec than over */
d = (psc >= 2) ? 5 : 7 - psc;

+   if (of_node && of_device_is_compatible(of_node, "ti,keystone-i2c"))
+   d = 6;
+
clk = ((input_clock / (psc + 1)) / (pdata->bus_freq * 1000));
/* Avoid driving the bus too fast because of rounding errors above */
if (input_clock / (psc + 1) / clk > pdata->bus_freq * 1000)
@@ -726,6 +733,7 @@ static struct i2c_algorithm i2c_davinci_algo = {

  static const struct of_device_id davinci_i2c_of_match[] = {
{.compatible = "ti,davinci-i2c", },
+   {.compatible = "ti,keystone-i2c", },
{},
  };
  MODULE_DEVICE_TABLE(of, davinci_i2c_of_match);




--
regards,
-grygorii
--
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: omap: fix bus recovery setup

[Grygorii Strashko]

Hi Wolfram,

On 07/14/2015 02:10 PM, Wolfram Sang wrote:
 On Wed, Jul 08, 2015 at 04:35:27PM +0200, Jan Luebbe wrote:
 At least on the AM335x, enabling OMAP_I2C_SYSTEST_ST_EN is not enough to
 
 Felipe: it seems you did not need this; is this AM335x specific?

We need it (Felipe's reply can be delayed).

It's not AM335x specific. TMODE has to be configured for all OMAP4+ SoCs too.

 
 allow direct access to the SCL and SDA pins. In addition to ST_EN, we
 need to set the TMODE to 0b11 (Loop back  SDA/SCL IO mode select).
 Also, as the reset values of SCL_O and SDA_O are 0 (which means drive
 low level), we need to set them to 1 (which means high-impedance) to
 avoid unwanted changes on the pins.

 As a precaution, reset all these bits to their default values after
 recovery is complete.

Reviewed-by: Grygorii Strashko grygorii.stras...@ti.com 


 Signed-off-by: Jan Luebbe j...@pengutronix.de
 ---
   drivers/i2c/busses/i2c-omap.c | 11 +++
   1 file changed, 11 insertions(+)

 diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
 index d1c22e3fdd14..fc9bf7f30e35 100644
 --- a/drivers/i2c/busses/i2c-omap.c
 +++ b/drivers/i2c/busses/i2c-omap.c
 @@ -1247,7 +1247,14 @@ static void omap_i2c_prepare_recovery(struct 
 i2c_adapter *adap)
  u32 reg;
   
  reg = omap_i2c_read_reg(dev, OMAP_I2C_SYSTEST_REG);
 +/* enable test mode */
  reg |= OMAP_I2C_SYSTEST_ST_EN;
 +/* select SDA/SCL IO mode */
 +reg |= 3  OMAP_I2C_SYSTEST_TMODE_SHIFT;
 +/* set SCL to high-impedance state (reset value is 0) */
 +reg |= OMAP_I2C_SYSTEST_SCL_O;
 +/* set SDA to high-impedance state (reset value is 0) */
 +reg |= OMAP_I2C_SYSTEST_SDA_O;
  omap_i2c_write_reg(dev, OMAP_I2C_SYSTEST_REG, reg);
   }
   
 @@ -1257,7 +1264,11 @@ static void omap_i2c_unprepare_recovery(struct 
 i2c_adapter *adap)
  u32 reg;
   
  reg = omap_i2c_read_reg(dev, OMAP_I2C_SYSTEST_REG);
 +/* restore reset values */
  reg = ~OMAP_I2C_SYSTEST_ST_EN;
 +reg = ~OMAP_I2C_SYSTEST_TMODE_MASK;
 +reg = ~OMAP_I2C_SYSTEST_SCL_O;
 +reg = ~OMAP_I2C_SYSTEST_SDA_O;
  omap_i2c_write_reg(dev, OMAP_I2C_SYSTEST_REG, reg);
   }
   
 -- 
 2.1.4



-- 
regards,
-grygorii
--
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: busses: i2c-omap: Increase timeout for i2c interrupt

[Grygorii Strashko]

Hi Wolfram,

On 07/10/2015 12:09 PM, Wolfram Sang wrote:
 
 60 s sounds way too much and actually I simply don't believe this is
 the root cause. If I take a look into the driver, then I see, that
 
 I agree, this is just a workaround.
 
 the design is not really the best. The whole IRQ handling could be
 actually performed in hard IRQ handler, without threading overhead.
 Putting even 2 bytes in the controller FIFO should not be too heavy
 for the hard IRQ handler. Then these ridiculous spin_lock()s. What
 is the reason behind? The IRQ is flagged with ONESHOT, so thread and
 hardirq handler are anyway mutually excluded. But if this thread
 ever runs longer than it's allowed in IRQ context, then it anyway
 produces this IRQ latency because it locks spin_lock_irqsave() for
 the whole time! So the whole point of threaded interrupt is missing.
 
 Furthermore, this combination of threaded_irq and struct completion seems
 bogus to me. If you just want to ensure the irq happened before timeout,
 you just complete when the irq happened and do the bottom half after the
 completion returned?
 

I'd very appreciated if You would be able to clarify your point a bit, pls?
completion is used to indicate end of one message transfer (+check for msg 
timeout),
so .master_xfer()-omap_i2c_xfer could switch to next msg.
And there could be more than on IRQ triggered depending on msg length
while one message is being transfered.

-- 
regards,
-grygorii
--
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: busses: i2c-omap: Increase timeout for i2c interrupt

[Grygorii Strashko]

On 07/10/2015 04:26 PM, Alexander Sverdlin wrote:
 Hi!
 
 On 10/07/15 15:17, ext Vignesh R wrote:
 I would propose you to throw away spinlocks. Convert threaded IRQ to

Agree. Looks like spinlock is not needed.

 just one hardirq handler. And continue debugging. You will reduce the
 load of the system with the above measures, maybe it will not happen
 any more, maybe you'll figure out that problem is somewhere else.

 Or this.
 I am not convinced with moving entire code at hardirq context. I believe
 its better to keep hardirq as small as possible.
 
 How deep is the controller's FIFO? 1 byte? 2 bytes? Other drivers can 
 perfectly fill
 next byte in hardirq handler. If you need to do 10 opcodes more in hardirq 
 handler,
 it's much better for the whole system than to trigger scheduler and thread 
 and and and
 just because of these 10 opcodes.
 

1) TRM: Built-in configurable FIFOs (8, 16, 32, 64 bytes) for buffered read or 
write.
2) We trying to be as much compatible with RT kernel as possible where all IRQ
are threaded.

And yes. This patch is u.. WA which tries to fix symptom and not an issue.

-- 
regards,
-grygorii
--
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: davinci: Fix bus rate calculation on Keystone SoC

[Grygorii Strashko]

Hi,
On 07/10/2015 07:02 PM, Sekhar Nori wrote:
 On Friday 10 July 2015 01:23 AM, Wolfram Sang wrote:
 On Thu, Jun 18, 2015 at 12:22:33PM -0400, Murali Karicheri wrote:
 On 06/18/2015 05:00 AM, Sekhar Nori wrote:
 On Thursday 18 June 2015 02:23 PM, Alexander Sverdlin wrote:
 According to KeyStone Architecture I2C User Guide,

   module clock frequency
 master clock frequency = --
   (ICCL + 6) + (ICCH + 6)

 i.e. d in i2c_davinci_calc_clk_dividers() should be fixed and
 not dependent from module clock prescaler PSC on these SoCs.

 Signed-off-by: Alexander Sverdlin alexander.sverd...@nokia.com
 ---

 RFC: If someone from TI has an idea how to improve the coverage of future 
 Keystone
 revisions -- hints/patches are welcome. The current ID check is based on
 Davinci/Keystone datasheets and is at least working on real Keystone II.

 + Murali who works on Keystone devices in TI.

 + Grygorii

 + Grygorii has been involved in the Keystone related enhancement and
 reviewing prior patches. Need to have his ack for this change

 Any news?
 
 Fixing Grygorii's e-mail id.
 
 Grygorii, let me know if you don't have the thread. I can forward.

Thanks Sekhar.

My opinion - it's time for compatible string :) ti,keystone-i2c. 
Especially taking int account two things:
1) In Datasheet SPRS893B TCI6630K2L Multicore DSP+ARM KeyStone II 
System-on-Chip (SoC) (Rev. E) 
   values for those registers specified as:
   0x0034 ICPID1 I2C Peripheral Identification Register 1 [value: 0x 0105]
   0x0038 ICPID2 I2C Peripheral Identification Register 2 [value: 0x 0005]
   (actually the same is in k2h, k2e Datasheets).

2) This is not the first time such discussion has been raised.


  This is not really critical fix. Currently bus rate is lower than expected 
  because of these
  calculation errors. The fix maximizes the bus rate. So newer SoCs will run 
  little bit slower
  until support is added to this part of the code. Not really critical. 

Regarding the patch itself:
- Seems the d value is fixed to 6 as per User Guide SPRUGV3
  KeyStone Architecture 2 Inter-IC Control Bus (I2C) and this change is 
correct. 
  It would be nice to have ref on document in commit message as above.

- I think, it will be very useful to have same real digits/calculation 
mentioned in
  commit message which can show how valuable is the improvement. 

-- 
regards,
-grygorii
--
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 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Grygorii Strashko]

On 07/07/2015 05:13 PM, Wolfram Sang wrote:
 On Tue, Jul 07, 2015 at 03:48:52PM +0200, Uwe Kleine-König wrote:

 On Tue, Jul 07, 2015 at 03:37:49PM +0200, Jan Lübbe wrote:
 On Mi, 2014-11-26 at 19:05 +0200, Grygorii Strashko wrote:
 On 11/26/2014 06:04 PM, Uwe Kleine-König wrote:
 On Wed, Nov 26, 2014 at 03:59:53PM +0200, Grygorii Strashko wrote:
 Having a board where the I2C bus locks up occasionally made it clear
 that the bus recovery in the i2c-davinci driver will only work on
 some boards, because on regular boards, this will only toggle GPIO
 lines that aren't muxed to the actual pins.

 The I2C controller on SoCs like da850 (and da830), Keystone 2 has the
 built-in capability to bit-bang its lines by using the ICPFUNC registers
 of the i2c controller.
 Implement the suggested procedure by toggling SCL and checking SDA using
 the ICPFUNC registers of the I2C controller when present. Allow platforms
 to indicate the presence of the ICPFUNC registers with a has_pfunc 
 platform
 data flag and add optional DT property ti,has-pfunc to indicate
 the same in DT.
 On what does it depend if this pfunc stuff works or not? Only the SoC,
 or also on some board specific properties?

 SoC / set of SoCs. Also, similar feature is supported by OMAP and 
 AM335x/AM437x SoCs
 using I2C_SYSTEST register.

 Given the former using the
 compatible string to detect its availability would be better. (In this
 case also sorry, didn't consider this case when requesting the property
 in the last round.)

 I only stumbled across this after it was merged, with the additional
 I also wonder how it came to the Reviewed-by tag for me. The last thing
 that I said about the patch was On what does it depend if this pfunc
 stuff works or not? Only the SoC, or also on some board specific
 properties? (see above) and the patch looks ok. IMHO this hardly
 justifies to add the Reviewed-by tag for the next round. :-(
 
 That needs to be discussed with Grygorii. I can't verify the correctness
 of tags for every patch, although I do try to keep an eye on it...
 

Regarding the patch looks ok - sincerely sorry!
This is not the first time I've treated looks good.. as Reviewed-by and I got 
no complaints before :(
Will take it into account.


Regarding technical comments:
OK. Seems I missed smth. or understood wrongly.
So, I'll say what's people usually saying here - Sorry for that :(
But, to be honest I don't feel guilty, because:
- v4 of these patches was merged finally
- that v4 missed 2 kernel releases
- you were added in TO: for all versions of these patches.

-- 
regards,
-grygorii
--
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/3] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()

[Grygorii Strashko]

On 12 March 2015 at 10:50, Alexander Sverdlin
alexander.sverd...@nokia.com wrote:
 On 11/03/15 19:35, ext grygorii.stras...@linaro.org wrote:
 +if (i2c_davinci_wait_status_change(dev, DAVINCI_I2C_STR_BB, 0)) {
 +dev_warn(dev-dev, timeout waiting for bus ready\n);
 +davinci_i2c_recover_bus(dev);
 +i2c_davinci_init(dev);
 +/*
 + * the bus should not be busy after init, otherwise something
 + * is badly broken
 + */

 I think you should recheck BB and return error if it's still detected.

 But after re-reading the datasheet, I must conclude, this is almost not 
 possible.
 It will be a dead code. Reset will clear BB anyway and the only possibility 
 to see
 BB set to 1 here is when another master transmits right after our reset. We 
 cannot
 guarantee that we will always catch this here, so this must be any way 
 handled over
 AL interrupt.

The question here what is worse:
- silently continue execution with undefined behavior
- or recheck and return error (That's how it was before).

I like option 2 :) - don't believe HW.

-- 
Best regards,
-grygorii
--
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: omap: implement bus recovery

[Grygorii Strashko]

Hi Felipe,

On 03/11/2015 03:50 AM, Felipe Balbi wrote:

On Mon, Mar 09, 2015 at 11:39:17AM -0500, Felipe Balbi wrote:

On Thu, Feb 19, 2015 at 12:06:49PM -0600, Felipe Balbi wrote:

If either SCL or SDA are stuck low, we need to
recover the bus using the procedure described
on section 3.1.16 of the I2C specification.

Note that we're trying to implement the procedure
exactly as described by that section. First we
check which line is stuck low, then implement
one or the other procedure. If SDA recovery procedure
fails, we reset our IP in an attempt to make it work.

Signed-off-by: Felipe Balbi ba...@ti.com
---

Tested with AM437x IDK, AM437x SK, BeagleBoneBlack and Beagle X15 with
1000 iterations of i2cdetect on all available buses.

That said, I couldn't get any device to hold the bus busy so I could
see this working. If anybody has any good way of forcing a condition
so that we need bus recovery, I'd be glad to look at.


ping


any comments here ?? Anybody at all 



I think the I2C bus recovery infrastructure should be used here ;)
As I did there https://lkml.org/lkml/2014/12/1/397, but
there are no comments too :(

regards,
-grygorii

--
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 v3 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery

[Grygorii Strashko]

Hi Wolfram,

On 12/04/2014 08:29 PM, Wolfram Sang wrote:

On Mon, Dec 01, 2014 at 05:34:05PM +0200, Grygorii Strashko wrote:

This patch changes type of input parameter for .prepare/unprepare_recovery()
callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
This allows to simplify implementation of these callbacks and avoid
type conversations from i2c_bus_recovery_info to i2c_adapter.
The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
which contains pointer on it.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Acked-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com


Ah, bus recovery, I need to have a closer look on this one. Scheduled
for 3.20.



Unfortunately, it looks like you've missed these patches :(

I'm going to resend them if you agree.

regards,
-grygorii
--
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 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Grygorii Strashko]

Having a board where the I2C bus locks up occasionally made it clear
that the bus recovery in the i2c-davinci driver will only work on
some boards, because on regular boards, this will only toggle GPIO
lines that aren't muxed to the actual pins.

The I2C controller on SoCs like da850 (and da830), Keystone 2 has the
built-in capability to bit-bang its lines by using the ICPFUNC registers
of the i2c controller.
Implement the suggested procedure by toggling SCL and checking SDA using
the ICPFUNC registers of the I2C controller when present. Allow platforms
to indicate the presence of the ICPFUNC registers with a has_pfunc platform
data flag and add optional DT property ti,has-pfunc to indicate
the same in DT.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
CC: Mike Looijmans i...@milosoftware.com
CC: devicet...@vger.kernel.org
Reviewed-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de
Signed-off-by: Ben Gardiner bengardi...@nanometrics.ca
Signed-off-by: Mike Looijmans milo-softw...@users.sourceforge.net
[grygorii.stras...@ti.com: combined patches from Ben Gardiner and
Mike Looijmans and reimplemented ICPFUNC bus recovery using I2C
bus recovery infrastructure]
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 .../devicetree/bindings/i2c/i2c-davinci.txt|   3 +
 drivers/i2c/busses/i2c-davinci.c   | 102 -
 include/linux/platform_data/i2c-davinci.h  |   1 +
 3 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt 
b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
index 2dc935b..a4e1cbc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
@@ -10,6 +10,9 @@ Required properties:
 Recommended properties :
 - interrupts : standard interrupt property.
 - clock-frequency : desired I2C bus clock frequency in Hz.
+- ti,has-pfunc: boolean; if defined, it indicates that SoC supports PFUNC
+   registers. PFUNC registers allow to switch I2C pins to function as
+   GPIOs, so they can by toggled manually.
 
 Example (enbw_cmc board):
i2c@1c22000 {
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 00aed63..a1bb587 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -64,6 +64,12 @@
 #define DAVINCI_I2C_IVR_REG0x28
 #define DAVINCI_I2C_EMDR_REG   0x2c
 #define DAVINCI_I2C_PSC_REG0x30
+#define DAVINCI_I2C_FUNC_REG   0x48
+#define DAVINCI_I2C_DIR_REG0x4c
+#define DAVINCI_I2C_DIN_REG0x50
+#define DAVINCI_I2C_DOUT_REG   0x54
+#define DAVINCI_I2C_DSET_REG   0x58
+#define DAVINCI_I2C_DCLR_REG   0x5c
 
 #define DAVINCI_I2C_IVR_AAS0x07
 #define DAVINCI_I2C_IVR_SCD0x06
@@ -97,6 +103,29 @@
 #define DAVINCI_I2C_IMR_NACK   BIT(1)
 #define DAVINCI_I2C_IMR_AL BIT(0)
 
+/* set SDA and SCL as GPIO */
+#define DAVINCI_I2C_FUNC_PFUNC0BIT(0)
+
+/* set SCL as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR0  BIT(0)
+/* set SDA as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR1  BIT(1)
+
+/* read SCL GPIO level */
+#define DAVINCI_I2C_DIN_PDIN0 BIT(0)
+/* read SDA GPIO level */
+#define DAVINCI_I2C_DIN_PDIN1 BIT(1)
+
+/*set the SCL GPIO high */
+#define DAVINCI_I2C_DSET_PDSET0BIT(0)
+/*set the SDA GPIO high */
+#define DAVINCI_I2C_DSET_PDSET1BIT(1)
+
+/* set the SCL GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR0BIT(0)
+/* set the SDA GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+
 struct davinci_i2c_dev {
struct device   *dev;
void __iomem*base;
@@ -257,6 +286,71 @@ static struct i2c_bus_recovery_info 
davinci_i2c_gpio_recovery_info = {
.unprepare_recovery = davinci_i2c_unprepare_recovery,
 };
 
+static void davinci_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   if (val)
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_DSET_REG,
+ DAVINCI_I2C_DSET_PDSET0);
+   else
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_DCLR_REG,
+ DAVINCI_I2C_DCLR_PDCLR0);
+}
+
+static int davinci_i2c_get_scl(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+   int val;
+
+   /* read the state of SCL */
+   val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+   return val  DAVINCI_I2C_DIN_PDIN0;
+}
+
+static int davinci_i2c_get_sda(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+   int val;
+
+   /* read the state of SDA */
+   val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+   return val  DAVINCI_I2C_DIN_PDIN1;
+}
+
+static void

[PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure

[Grygorii Strashko]

This patch converts Davinci I2C driver to use I2C bus recovery
infrastructure, introduced by commit 5f9296ba21b3 (i2c: Add
bus recovery infrastructure).

The i2c_bus_recovery_info is configured for Davinci I2C adapter
only in case scl_pin is provided in platform data.

As the controller must be held in reset while doing so, the
recovery routine must re-init the controller. Since this was already
being done after each call to i2c_recover_bus, move those calls into
the recovery_prepare/unprepare routines and as well.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Acked-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c | 77 +++-
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 17e1203..00aed63 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct 
davinci_i2c_dev *i2c_dev, int reg)
return readw_relaxed(i2c_dev-base + reg);
 }
 
-/* Generate a pulse on the i2c clock pin. */
-static void davinci_i2c_clock_pulse(unsigned int scl_pin)
-{
-   u16 i;
-
-   if (scl_pin) {
-   /* Send high and low on the SCL line */
-   for (i = 0; i  9; i++) {
-   gpio_set_value(scl_pin, 0);
-   udelay(20);
-   gpio_set_value(scl_pin, 1);
-   udelay(20);
-   }
-   }
-}
-
-/* This routine does i2c bus recovery as specified in the
- * i2c protocol Rev. 03 section 3.16 titled Bus clear
- */
-static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
-{
-   u32 flag = 0;
-   struct davinci_i2c_platform_data *pdata = dev-pdata;
-
-   dev_err(dev-dev, initiating i2c bus recovery\n);
-   /* Send NACK to the slave */
-   flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-   flag |=  DAVINCI_I2C_MDR_NACK;
-   /* write the data into mode register */
-   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-   davinci_i2c_clock_pulse(pdata-scl_pin);
-   /* Send STOP */
-   flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-   flag |= DAVINCI_I2C_MDR_STP;
-   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-}
-
 static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
int val)
 {
@@ -267,6 +230,34 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 }
 
 /*
+ * This routine does i2c bus recovery by using i2c_generic_gpio_recovery
+ * which is provided by I2C Bus recovery infrastructure.
+ */
+static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   /* Disable interrupts */
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
+
+   /* put I2C into reset */
+   davinci_i2c_reset_ctrl(dev, 0);
+}
+
+static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   i2c_davinci_init(dev);
+}
+
+static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
+   .recover_bus = i2c_generic_gpio_recovery,
+   .prepare_recovery = davinci_i2c_prepare_recovery,
+   .unprepare_recovery = davinci_i2c_unprepare_recovery,
+};
+
+/*
  * Waiting for bus not busy
  */
 static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
@@ -286,8 +277,7 @@ static int i2c_davinci_wait_bus_not_busy(struct 
davinci_i2c_dev *dev,
return -ETIMEDOUT;
} else {
to_cnt = 0;
-   davinci_i2c_recover_bus(dev);
-   i2c_davinci_init(dev);
+   i2c_recover_bus(dev-adapter);
}
}
if (allow_sleep)
@@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct 
i2c_msg *msg, int stop)
  dev-adapter.timeout);
if (r == 0) {
dev_err(dev-dev, controller timed out\n);
-   davinci_i2c_recover_bus(dev);
-   i2c_davinci_init(dev);
+   i2c_recover_bus(adap);
dev-buf_len = 0;
return -ETIMEDOUT;
}
@@ -721,6 +710,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
adap-timeout = DAVINCI_I2C_TIMEOUT;
adap-dev.of_node = pdev-dev.of_node;
 
+   if (dev-pdata-scl_pin) {
+   adap-bus_recovery_info = davinci_i2c_gpio_recovery_info;
+   adap

[PATCH v3 1/5] i2c: i2c-davinci: switch to use platform_get_irq

[Grygorii Strashko]

Switch Davinci I2C driver to use platform_get_irq(), because
it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..)
for requesting IRQ resources any more, as they can be not ready yet
in case of DT-boot.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Acked-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..25e8e25 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,17 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 {
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
-   struct resource *mem, *irq;
-   int r;
-
-   irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-   if (!irq) {
-   dev_err(pdev-dev, no irq resource?\n);
-   return -ENODEV;
+   struct resource *mem;
+   int r, irq;
+
+   irq = platform_get_irq(pdev, 0);
+   if (irq = 0) {
+   if (!irq)
+   irq = -ENXIO;
+   if (irq != -EPROBE_DEFER)
+   dev_err(pdev-dev,
+   can't get irq resource ret=%d\n, irq);
+   return irq;
}
 
dev = devm_kzalloc(pdev-dev, sizeof(struct davinci_i2c_dev),
@@ -661,7 +665,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
init_completion(dev-xfr_complete);
 #endif
dev-dev = pdev-dev;
-   dev-irq = irq-start;
+   dev-irq = irq;
dev-pdata = dev_get_platdata(pdev-dev);
platform_set_drvdata(pdev, dev);
 
-- 
1.9.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 2/5] i2c: davinci: generate STP always when NACK is received

[Grygorii Strashko]

According to I2C specification the NACK should be handled as follows:
When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then generate either a STOP condition to
abort the transfer, or a repeated START condition to start a new transfer.
[I2C spec Rev. 6, 3.1.6: http://www.nxp.com/documents/user_manual/UM10204.pdf]

Currently the Davinci i2c driver interrupts the transfer on receipt of a
NACK but fails to send a STOP in some situations and so makes the bus
stuck until next I2C IP reset (idle/enable).

For example, the issue will happen during SMBus read transfer which
consists from two i2c messages write command/address and read data:

S Slave Address Wr A Command Code A Sr Slave Address Rd A D1..Dn A P
--- write --- --- read -

The I2C client device will send NACK if it can't recognize Command Code
and it's expected from I2C master to generate STP in this case.
But now, Davinci i2C driver will just exit with -EREMOTEIO and STP will
not be generated.

Hence, fix it by generating Stop condition (STP) always when NACK is received.

This patch fixes Davinci I2C in the same way it was done for OMAP I2C
commit cda2109a26eb (i2c: omap: query STP always when NACK is received).

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Reviewed-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de
Reported-by: Hein Tibosch hein_tibo...@yahoo.es
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 25e8e25..17e1203 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct 
i2c_msg *msg, int stop)
if (dev-cmd_err  DAVINCI_I2C_STR_NACK) {
if (msg-flags  I2C_M_IGNORE_NAK)
return msg-len;
-   if (stop) {
-   w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-   w |= DAVINCI_I2C_MDR_STP;
-   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
-   }
+   w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+   w |= DAVINCI_I2C_MDR_STP;
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
return -EREMOTEIO;
}
return -EIO;
-- 
1.9.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 0/5] i2c: davinci improvements and fixes

[Grygorii Strashko]

This series contains some fixes and improvements for Davinci I2C:
patch 1 - small improvement
patch 2 - fixes Bus busy state for some case (was reported long time ago:()

patch 3-5 - converts driver to use I2C bus recovery infrastructure and
adds Davinci I2C bus recovery mechanizm based on using ICPFUNC registers.
These patches are combination of two patches from Ben Gardiner [1] and
Mike Looijmans [2] which i've reworked to use I2C bus recovery infrastructure

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/047305.html
[PATCH 0/6] i2c-davinci gpio pulsed SCL recovery with ICPFUNC
[2] https://lkml.org/lkml/2014/2/28/133
[PATCH] i2c-davinci: Implement a bus recovery that actually works

Changes in v3:
- minor comments applied

Changes in v2:
- patch 1: error handling improved
- patch 2: commit message updated
- patch 4: minor comments applied
- patch 5: added new optional DT property ti,has-pfunc

Link on v2:
 https://lkml.org/lkml/2014/11/26/252

Link on v1:
 http://www.spinics.net/lists/linux-i2c/msg17601.html

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com

Grygorii Strashko (5):
  i2c: i2c-davinci: switch to use platform_get_irq
  i2c: davinci: generate STP always when NACK is received
  i2c: recovery: change input parameter to i2c_adapter for
prepare/unprepare_recovery
  i2c: davinci: use bus recovery infrastructure
  i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

 .../devicetree/bindings/i2c/i2c-davinci.txt|   3 +
 drivers/i2c/busses/i2c-davinci.c   | 205 +++--
 drivers/i2c/i2c-core.c |   4 +-
 include/linux/i2c.h|   4 +-
 include/linux/platform_data/i2c-davinci.h  |   1 +
 5 files changed, 159 insertions(+), 58 deletions(-)

-- 
1.9.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 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery

[Grygorii Strashko]

This patch changes type of input parameter for .prepare/unprepare_recovery()
callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
This allows to simplify implementation of these callbacks and avoid
type conversations from i2c_bus_recovery_info to i2c_adapter.
The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
which contains pointer on it.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Acked-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/i2c-core.c | 4 ++--
 include/linux/i2c.h| 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6..72b6e34 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -563,7 +563,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
int i = 0, val = 1, ret = 0;
 
if (bri-prepare_recovery)
-   bri-prepare_recovery(bri);
+   bri-prepare_recovery(adap);
 
/*
 * By this time SCL is high, as we need to give 9 falling-rising edges
@@ -588,7 +588,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
}
 
if (bri-unprepare_recovery)
-   bri-unprepare_recovery(bri);
+   bri-unprepare_recovery(adap);
 
return ret;
 }
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..cf9380f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -404,8 +404,8 @@ struct i2c_bus_recovery_info {
void (*set_scl)(struct i2c_adapter *, int val);
int (*get_sda)(struct i2c_adapter *);
 
-   void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
-   void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
+   void (*prepare_recovery)(struct i2c_adapter *);
+   void (*unprepare_recovery)(struct i2c_adapter *);
 
/* gpio recovery */
int scl_gpio;
-- 
1.9.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 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Grygorii Strashko]

Having a board where the I2C bus locks up occasionally made it clear
that the bus recovery in the i2c-davinci driver will only work on
some boards, because on regular boards, this will only toggle GPIO
lines that aren't muxed to the actual pins.

The I2C controller on SoCs like da850 (and da830), Keystone 2 has the
built-in capability to bit-bang its lines by using the ICPFUNC registers
of the i2c controller.
Implement the suggested procedure by toggling SCL and checking SDA using
the ICPFUNC registers of the I2C controller when present. Allow platforms
to indicate the presence of the ICPFUNC registers with a has_pfunc platform
data flag and add optional DT property ti,has-pfunc to indicate
the same in DT.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
CC: devicet...@vger.kernel.org
Signed-off-by: Ben Gardiner bengardi...@nanometrics.ca
Signed-off-by: Mike Looijmans milo-softw...@users.sourceforge.net
[grygorii.stras...@ti.com: combined patches from Ben Gardiner and
Mike Looijmans and reimplemented ICPFUNC bus recovery using I2C
bus recovery infrastructure]
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 .../devicetree/bindings/i2c/i2c-davinci.txt|   3 +
 drivers/i2c/busses/i2c-davinci.c   | 102 -
 include/linux/platform_data/i2c-davinci.h  |   1 +
 3 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt 
b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
index 2dc935b..a4e1cbc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
@@ -10,6 +10,9 @@ Required properties:
 Recommended properties :
 - interrupts : standard interrupt property.
 - clock-frequency : desired I2C bus clock frequency in Hz.
+- ti,has-pfunc: boolean; if defined, it indicates that SoC supports PFUNC
+   registers. PFUNC registers allow to switch I2C pins to function as
+   GPIOs, so they can by toggled manually.
 
 Example (enbw_cmc board):
i2c@1c22000 {
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8605b4..f7dae10f 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -64,6 +64,12 @@
 #define DAVINCI_I2C_IVR_REG0x28
 #define DAVINCI_I2C_EMDR_REG   0x2c
 #define DAVINCI_I2C_PSC_REG0x30
+#define DAVINCI_I2C_FUNC_REG   0x48
+#define DAVINCI_I2C_DIR_REG0x4c
+#define DAVINCI_I2C_DIN_REG0x50
+#define DAVINCI_I2C_DOUT_REG   0x54
+#define DAVINCI_I2C_DSET_REG   0x58
+#define DAVINCI_I2C_DCLR_REG   0x5c
 
 #define DAVINCI_I2C_IVR_AAS0x07
 #define DAVINCI_I2C_IVR_SCD0x06
@@ -97,6 +103,29 @@
 #define DAVINCI_I2C_IMR_NACK   BIT(1)
 #define DAVINCI_I2C_IMR_AL BIT(0)
 
+/* set SDA and SCL as GPIO */
+#define DAVINCI_I2C_FUNC_PFUNC0BIT(0)
+
+/* set SCL as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR0  BIT(0)
+/* set SDA as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR1  BIT(1)
+
+/* read SCL GPIO level */
+#define DAVINCI_I2C_DIN_PDIN0 BIT(0)
+/* read SDA GPIO level */
+#define DAVINCI_I2C_DIN_PDIN1 BIT(1)
+
+/*set the SCL GPIO high */
+#define DAVINCI_I2C_DSET_PDSET0BIT(0)
+/*set the SDA GPIO high */
+#define DAVINCI_I2C_DSET_PDSET1BIT(1)
+
+/* set the SCL GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR0BIT(0)
+/* set the SDA GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+
 struct davinci_i2c_dev {
struct device   *dev;
void __iomem*base;
@@ -257,6 +286,71 @@ static struct i2c_bus_recovery_info 
davinci_i2c_gpio_recovery_info = {
.unprepare_recovery = davinci_i2c_unprepare_recovery,
 };
 
+static void davinci_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   if (val)
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_DSET_REG,
+ DAVINCI_I2C_DSET_PDSET0);
+   else
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_DCLR_REG,
+ DAVINCI_I2C_DCLR_PDCLR0);
+}
+
+static int davinci_i2c_get_scl(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+   int val;
+
+   /* read the state of SCL */
+   val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+   return val  DAVINCI_I2C_DIN_PDIN0;
+}
+
+static int davinci_i2c_get_sda(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+   int val;
+
+   /* read the state of SDA */
+   val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+   return val  DAVINCI_I2C_DIN_PDIN1;
+}
+
+static void davinci_i2c_scl_prepare_recovery(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap

[PATCH v2 1/5] i2c: i2c-davinci: switch to use platform_get_irq

[Grygorii Strashko]

Switch Davinci I2C driver to use platform_get_irq(), because
it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..)
for requesting IRQ resources any more, as they can be not ready yet
in case of DT-boot.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..7f54903 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,17 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 {
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
-   struct resource *mem, *irq;
-   int r;
-
-   irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-   if (!irq) {
-   dev_err(pdev-dev, no irq resource?\n);
-   return -ENODEV;
+   struct resource *mem;
+   int r, irq;
+
+   irq = platform_get_irq(pdev, 0);
+   if (irq = 0) {
+   if (irq == -EPROBE_DEFER)
+   return irq;
+   if (!irq)
+   irq = -ENXIO;
+   dev_err(pdev-dev, can't get irq resource ret=%d\n, irq);
+   return irq;
}
 
dev = devm_kzalloc(pdev-dev, sizeof(struct davinci_i2c_dev),
@@ -661,7 +665,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
init_completion(dev-xfr_complete);
 #endif
dev-dev = pdev-dev;
-   dev-irq = irq-start;
+   dev-irq = irq;
dev-pdata = dev_get_platdata(pdev-dev);
platform_set_drvdata(pdev, dev);
 
-- 
1.9.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 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery

[Grygorii Strashko]

This patch changes type of input parameter for .prepare/unprepare_recovery()
callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
This allows to simplify implementation of these callbacks and avoid
type conversations from i2c_bus_recovery_info to i2c_adapter.
The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
which contains pointer on it.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Acked-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/i2c-core.c | 4 ++--
 include/linux/i2c.h| 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6..72b6e34 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -563,7 +563,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
int i = 0, val = 1, ret = 0;
 
if (bri-prepare_recovery)
-   bri-prepare_recovery(bri);
+   bri-prepare_recovery(adap);
 
/*
 * By this time SCL is high, as we need to give 9 falling-rising edges
@@ -588,7 +588,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
}
 
if (bri-unprepare_recovery)
-   bri-unprepare_recovery(bri);
+   bri-unprepare_recovery(adap);
 
return ret;
 }
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..cf9380f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -404,8 +404,8 @@ struct i2c_bus_recovery_info {
void (*set_scl)(struct i2c_adapter *, int val);
int (*get_sda)(struct i2c_adapter *);
 
-   void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
-   void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
+   void (*prepare_recovery)(struct i2c_adapter *);
+   void (*unprepare_recovery)(struct i2c_adapter *);
 
/* gpio recovery */
int scl_gpio;
-- 
1.9.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 4/5] i2c: davinci: use bus recovery infrastructure

[Grygorii Strashko]

This patch converts Davinci I2C driver to use I2C bus recovery
infrastructure, introduced by commit 5f9296ba21b3 (i2c: Add
bus recovery infrastructure).

The i2c_bus_recovery_info is configured for Davinci I2C adapter
only in case if scl_pin is provided in platform data at least.

As the controller must be held in reset while doing so, the
recovery routine must re-init the controller. Since this was already
being done after each call to i2c_recover_bus, move those calls into
the recovery_prepare/unprepare routines and as well.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c | 77 +++-
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 1990ae8..b8605b4 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct 
davinci_i2c_dev *i2c_dev, int reg)
return readw_relaxed(i2c_dev-base + reg);
 }
 
-/* Generate a pulse on the i2c clock pin. */
-static void davinci_i2c_clock_pulse(unsigned int scl_pin)
-{
-   u16 i;
-
-   if (scl_pin) {
-   /* Send high and low on the SCL line */
-   for (i = 0; i  9; i++) {
-   gpio_set_value(scl_pin, 0);
-   udelay(20);
-   gpio_set_value(scl_pin, 1);
-   udelay(20);
-   }
-   }
-}
-
-/* This routine does i2c bus recovery as specified in the
- * i2c protocol Rev. 03 section 3.16 titled Bus clear
- */
-static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
-{
-   u32 flag = 0;
-   struct davinci_i2c_platform_data *pdata = dev-pdata;
-
-   dev_err(dev-dev, initiating i2c bus recovery\n);
-   /* Send NACK to the slave */
-   flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-   flag |=  DAVINCI_I2C_MDR_NACK;
-   /* write the data into mode register */
-   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-   davinci_i2c_clock_pulse(pdata-scl_pin);
-   /* Send STOP */
-   flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-   flag |= DAVINCI_I2C_MDR_STP;
-   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-}
-
 static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
int val)
 {
@@ -267,6 +230,34 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 }
 
 /*
+ * This routine does i2c bus recovery by using i2c_generic_gpio_recovery
+ * which is provided by I2C Bus recovery infrastructure.
+ */
+static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   /* Disable interrupts */
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
+
+   /* put I2C into reset */
+   davinci_i2c_reset_ctrl(dev, 0);
+}
+
+static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   i2c_davinci_init(dev);
+}
+
+static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
+   .recover_bus = i2c_generic_gpio_recovery,
+   .prepare_recovery = davinci_i2c_prepare_recovery,
+   .unprepare_recovery = davinci_i2c_unprepare_recovery,
+};
+
+/*
  * Waiting for bus not busy
  */
 static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
@@ -286,8 +277,7 @@ static int i2c_davinci_wait_bus_not_busy(struct 
davinci_i2c_dev *dev,
return -ETIMEDOUT;
} else {
to_cnt = 0;
-   davinci_i2c_recover_bus(dev);
-   i2c_davinci_init(dev);
+   i2c_recover_bus(dev-adapter);
}
}
if (allow_sleep)
@@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct 
i2c_msg *msg, int stop)
  dev-adapter.timeout);
if (r == 0) {
dev_err(dev-dev, controller timed out\n);
-   davinci_i2c_recover_bus(dev);
-   i2c_davinci_init(dev);
+   i2c_recover_bus(adap);
dev-buf_len = 0;
return -ETIMEDOUT;
}
@@ -721,6 +710,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
adap-timeout = DAVINCI_I2C_TIMEOUT;
adap-dev.of_node = pdev-dev.of_node;
 
+   if (dev-pdata-scl_pin) {
+   adap-bus_recovery_info = davinci_i2c_gpio_recovery_info;
+   adap-bus_recovery_info-scl_gpio = dev-pdata-scl_pin

[PATCH v2 2/5] i2c: davinci: query STP always when NACK is received

[Grygorii Strashko]

According to I2C specification the NACK should be handled as folows:
When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then generate either a STOP condition to
abort the transfer, or a repeated START condition to start a new transfer.
[I2C spec Rev. 6, 3.1.6: http://www.nxp.com/documents/user_manual/UM10204.pdf]

Currently the Davinci i2c driver interrupts the transfer on receipt of a
NACK but fails to send a STOP in some situations and so makes the bus
stuck until next I2C IP reset (idle/enable).

For example, the issue will happen during SMBus read transfer which
consists from two i2c messages write command/address and read data:

S Slave Address Wr A Command Code A Sr Slave Address Rd A D1..Dn A P
--- write --- --- read -

The I2C client device will send NACK if it can't recognize Command Code
and it's expected from I2C master to query STP in this case.
But now, Davinci i2C driver will just exit with -EREMOTEIO and STP will
not be queried.

Hence, fix it by querying Stop condition (STP) always when NACK is received.

This patch fixes Davinci I2C in the same way it was done for OMAP I2C
commit cda2109a26eb (i2c: omap: query STP always when NACK is received).

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Reported-by: Hein Tibosch hein_tibo...@yahoo.es
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 7f54903..1990ae8 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct 
i2c_msg *msg, int stop)
if (dev-cmd_err  DAVINCI_I2C_STR_NACK) {
if (msg-flags  I2C_M_IGNORE_NAK)
return msg-len;
-   if (stop) {
-   w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-   w |= DAVINCI_I2C_MDR_STP;
-   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
-   }
+   w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+   w |= DAVINCI_I2C_MDR_STP;
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
return -EREMOTEIO;
}
return -EIO;
-- 
1.9.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 v2 1/5] i2c: i2c-davinci: switch to use platform_get_irq

[Grygorii Strashko]

On 11/26/2014 05:54 PM, Uwe Kleine-König wrote:

Hello Grygorii,

On Wed, Nov 26, 2014 at 03:59:49PM +0200, Grygorii Strashko wrote:

Switch Davinci I2C driver to use platform_get_irq(), because
it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..)
for requesting IRQ resources any more, as they can be not ready yet
in case of DT-boot.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
  drivers/i2c/busses/i2c-davinci.c | 20 
  1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..7f54903 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,17 @@ static int davinci_i2c_probe(struct platform_device *pdev)
  {
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
-   struct resource *mem, *irq;
-   int r;
-
-   irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-   if (!irq) {
-   dev_err(pdev-dev, no irq resource?\n);
-   return -ENODEV;
+   struct resource *mem;
+   int r, irq;
+
+   irq = platform_get_irq(pdev, 0);
+   if (irq = 0) {
+   if (irq == -EPROBE_DEFER)
+   return irq;
+   if (!irq)
+   irq = -ENXIO;
+   dev_err(pdev-dev, can't get irq resource ret=%d\n, irq);
+   return irq;

The simpler and I think also more usual logic is:

if (!irq)
irq = -ENXIO;
if (irq != -EPROBE_DEFER)
dev_err(...);
return irq;

Other than that the patch looks fine.


Ok. will change and add your ack.

regards,
-grygorii

--
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 2/5] i2c: davinci: query STP always when NACK is received

[Grygorii Strashko]

On 11/26/2014 05:57 PM, Uwe Kleine-König wrote:

Hello,

I don't understand your use of query in the subject and later in the
commit log. Do you mean send?


will change to generate. Ok?



On Wed, Nov 26, 2014 at 03:59:50PM +0200, Grygorii Strashko wrote:

According to I2C specification the NACK should be handled as folows:

s/folows/follows/

ok.

Can I assume that you are ok with this patch in general?

regards,
-grygorii


--
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 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Grygorii Strashko]

On 11/26/2014 06:04 PM, Uwe Kleine-König wrote:
 On Wed, Nov 26, 2014 at 03:59:53PM +0200, Grygorii Strashko wrote:
 Having a board where the I2C bus locks up occasionally made it clear
 that the bus recovery in the i2c-davinci driver will only work on
 some boards, because on regular boards, this will only toggle GPIO
 lines that aren't muxed to the actual pins.

 The I2C controller on SoCs like da850 (and da830), Keystone 2 has the
 built-in capability to bit-bang its lines by using the ICPFUNC registers
 of the i2c controller.
 Implement the suggested procedure by toggling SCL and checking SDA using
 the ICPFUNC registers of the I2C controller when present. Allow platforms
 to indicate the presence of the ICPFUNC registers with a has_pfunc platform
 data flag and add optional DT property ti,has-pfunc to indicate
 the same in DT.
 On what does it depend if this pfunc stuff works or not? Only the SoC,
 or also on some board specific properties?

SoC / set of SoCs. Also, similar feature is supported by OMAP and AM335x/AM437x 
SoCs
using I2C_SYSTEST register.

 Given the former using the
 compatible string to detect its availability would be better. (In this
 case also sorry, didn't consider this case when requesting the property
 in the last round.)
 
 The patch looks ok.

regards,
-grygorii
--
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: [5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Grygorii Strashko]

Hi Uwe,
On 11/24/2014 09:45 PM, Uwe Kleine-König wrote:

On Mon, Nov 24, 2014 at 03:15:58PM +0200, Grygorii Strashko wrote:

On 11/23/2014 07:04 PM, Uwe Kleine-König wrote:

On Thu, Nov 20, 2014 at 12:03:08PM +0200, Grygorii Strashko wrote:

@@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (!of_property_read_u32(pdev-dev.of_node, clock-frequency,
prop))
dev-pdata-bus_freq = prop / 1000;
+   dev-pdata-has_pfunc = true;

I don't understand this. Why does this ICPFUNC recovery work if the bus
is probed by oftree, but doesn't if not?


I've mentioned this in commit message:
  Allow platforms to indicate the presence of the ICPFUNC registers with a 
has_pfunc
  platform data flag and enable this mode for platforms which supports DT 
(da850 and
  Keystone 2 are two SoCs which support DT now and they also support ICPFUNC 
registers).

Ah, so you assume that in the dt case the pfunc functionality is
available. I didn't understand if it's bad or not if this assumption is
wrong, but I suggest to only use the pfunc stuff if the device node has
a given property (e.g. ti,has_pfunc).


Agree. I'll add it.

regards,
-grygorii
--
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: [5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Grygorii Strashko]

Hi Uwe,
On 11/23/2014 07:04 PM, Uwe Kleine-König wrote:
 On Thu, Nov 20, 2014 at 12:03:08PM +0200, Grygorii Strashko wrote:
 @@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct platform_device 
 *pdev)
  if (!of_property_read_u32(pdev-dev.of_node, clock-frequency,
  prop))
  dev-pdata-bus_freq = prop / 1000;
 +dev-pdata-has_pfunc = true;
 I don't understand this. Why does this ICPFUNC recovery work if the bus
 is probed by oftree, but doesn't if not?

I've mentioned this in commit message:
 Allow platforms to indicate the presence of the ICPFUNC registers with a 
has_pfunc
 platform data flag and enable this mode for platforms which supports DT (da850 
and
 Keystone 2 are two SoCs which support DT now and they also support ICPFUNC 
registers).

I'll add proper comment here.

 
  } else if (!dev-pdata) {
  dev-pdata = davinci_i2c_platform_data_default;
  }
 @@ -705,7 +801,9 @@ static int davinci_i2c_probe(struct platform_device 
 *pdev)
  adap-timeout = DAVINCI_I2C_TIMEOUT;
  adap-dev.of_node = pdev-dev.of_node;
   
 -if (dev-pdata-scl_pin) {
 +if (dev-pdata-has_pfunc)
 +adap-bus_recovery_info = davinci_i2c_scl_recovery_info;
 +else if (dev-pdata-scl_pin) {
  adap-bus_recovery_info = davinci_i2c_gpio_recovery_info;
  adap-bus_recovery_info-scl_gpio = dev-pdata-scl_pin;
  adap-bus_recovery_info-sda_gpio = dev-pdata-sda_pin;
 diff --git a/include/linux/platform_data/i2c-davinci.h 
 b/include/linux/platform_data/i2c-davinci.h
 index 2312d19..89fd347 100644
 --- a/include/linux/platform_data/i2c-davinci.h
 +++ b/include/linux/platform_data/i2c-davinci.h
 @@ -18,6 +18,7 @@ struct davinci_i2c_platform_data {
  unsigned intbus_delay;  /* post-transaction delay (usec) */
  unsigned intsda_pin;/* GPIO pin ID to use for SDA */
  unsigned intscl_pin;/* GPIO pin ID to use for SCL */
 +boolhas_pfunc;  /*chip has a ICPFUNC register */
 Space after /* please
 

regards,
-grygorii

--
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: [4/5] i2c: davinci: use bus recovery infrastructure

[Grygorii Strashko]

Hi Uwe,

On 11/23/2014 10:36 PM, Uwe Kleine-König wrote:
 On Fri, Nov 21, 2014 at 09:33:22PM +0200, Grygorii Strashko wrote:
 On 11/21/2014 09:07 PM, Uwe Kleine-König wrote:
 On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
 Just another general comment about the driver that doesn't influence the
 correctness of this patch: The i2c-davinci driver is quite quick to
 reset the bus. I wonder how often this reset triggers. Is the bus in
 question less stable than others?

 In comparison to ..? :)
 In comparison to other bus drivers in other SoCs. I know this might be
 hard to answer. I just wonder where the reason for this has to be
 located. Strange hardware? Software bug? Or is this SoC just operating
 with strange slaves more often than others?

Davinci driver does reset in two cases:
- when I2C transaction isn't completed due to timeout (no irq received)
- when BB is detected
both cases are reasonable, because in 1st case HW state is undefined
in 2d case - Davinci I2C supports only master mode and if BB detected
we need perform some recovery procedure.

Also, this patch doesn't introduce functional changes - it's just code
reworking intended to reuse I2C bus recovery infrastructure

i2c-omap.c - OMAP I2C driver does mostly the same now.
i2c-tegra.c - seems, It will do reset even frequently.
i2c-imx.c - if understand right, it will reinitialize I2C controller 
before each transfer, because it enables/disables I2C clocks.
... 

So, what i can say here is just In comparison to ..? :)

regards,
-grygorii
--
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: [2/5] i2c: davinci: query STP always when NACK is received

[Grygorii Strashko]

Hi Uwe,
On 11/23/2014 10:33 PM, Uwe Kleine-König wrote:
 On Fri, Nov 21, 2014 at 05:33:37PM +0200, Grygorii Strashko wrote:
 On 11/21/2014 03:10 PM, Uwe Kleine-König wrote:
 On Fri, Nov 21, 2014 at 02:48:57PM +0200, Grygorii Strashko wrote:
 On 11/21/2014 12:19 AM, Uwe Kleine-König wrote:
 diff --git a/drivers/i2c/busses/i2c-davinci.c 
 b/drivers/i2c/busses/i2c-davinci.c
 index 9bbfb8f..2cef115 100644
 --- a/drivers/i2c/busses/i2c-davinci.c
 +++ b/drivers/i2c/busses/i2c-davinci.c
 @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, 
 struct i2c_msg *msg, int stop)
  if (dev-cmd_err  DAVINCI_I2C_STR_NACK) {
  if (msg-flags  I2C_M_IGNORE_NAK)
  return msg-len;
 -if (stop) {
 -w = davinci_i2c_read_reg(dev, 
 DAVINCI_I2C_MDR_REG);
 -w |= DAVINCI_I2C_MDR_STP;
 -davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 
 w);
 -}
 +w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
 +w |= DAVINCI_I2C_MDR_STP;
 +davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 I think this is a good change, but I wonder if the handling of
 I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
 for the 2nd byte of a 5-byte-message, the transfer supposed to
 continue, right? (Hmm, maybe the framework handle this and restarts the
 transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
 handle this flag?)

 Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
 change current behavior - davinci driver will interrupt transfer of 
 i2c_msg always
in case of NACK and start transfer of the next i2c_msg (if exist).
 In my opinion, Above question is out of scope of this patch.
 Yeah right, that's exactly what I thought.

 Thinking again I wonder if with your change handling is correct when the
 sender wants to do a repeated start. That would need a more detailed
 look into the driver.

 Davinci driver will always abort transfer with error -EREMOTEIO in case if
 NACK received from I2C slave device. And the next omap_i2c_xfer() call may
 be *not* targeted to the same I2C slave device.
 ^ if !I2C_M_IGNORE_NAK
 Does this resolve my concern? I think it doesn't. Also a Sr might well
 address another device, doesn't it?
 
 A call to .master_xfer with a message sequence implicitly expects ACKs
 from the slave and doesn't tell anything about what should be done on a
 NAK. So IMHO you must not send a P when the slave responds with a NAK,
 but error out and let the sender decide if it wants to reply with P or
 Sr.

Sry, but what should be done is defined by I2C/SMbus specs? Does it?
For SMBus devices, the specification states (http://smbus.org/specs/)
4.2.Acknowledge (ACK) and not acknowledge (NACK):
- The slave device detects an invalid command or invalid data. In this 
  case the slave device must not acknowledge the received byte. The master
  upon detection of this condition must generate a STOP condition and
  retry the transaction
For I2C devices, the specification states 
[http://www.nxp.com/documents/user_manual/UM10204.pdf]:
3.1.6 Acknowledge (ACK) and Not Acknowledge (NACK)
When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then generate either a STOP condition to
abort the transfer, or a repeated START condition to start a new transfer.

Let take a look on i2c/smbus xfer:
i2c_lock_adapter(adap)
 adap-algo-master_xfer/smbus_xfer()
i2c_unlock_adapter(adap);
 |- rt_mutex_unlock(adapter-bus_lock);
|- task switch
 
So, there is no guarantee that next xfer will address the same I2C client 
device,
which, in turn, may lead to BB detection (will lead to BB detection if previous
transfer has been not acknowledged by SMbus client device).

Small summary, I2C core + Davinci I2C driver provide ability to use repeated
start (Sr) only within one I2C transaction - which is a number of write/read
operations specified by i2c_msg array. NACK always interrupts transaction
with -EREMOTEIO.

Also, the I2C core doesn't provide ability to manually send P.

regards,
-grygorii
--
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: [5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Grygorii Strashko]

On 11/24/2014 08:13 PM, Mike Looijmans wrote:
 On 24-11-2014 14:15, Grygorii Strashko wrote:
 Hi Uwe,
 On 11/23/2014 07:04 PM, Uwe Kleine-König wrote:
 On Thu, Nov 20, 2014 at 12:03:08PM +0200, Grygorii Strashko wrote:
 @@ -664,6 +759,7 @@ static int davinci_i2c_probe(struct 
 platform_device *pdev)
if (!of_property_read_u32(pdev-dev.of_node, 
 clock-frequency,
prop))
dev-pdata-bus_freq = prop / 1000;
 +dev-pdata-has_pfunc = true;
 I don't understand this. Why does this ICPFUNC recovery work if the bus
 is probed by oftree, but doesn't if not?
 I've mentioned this in commit message:
   Allow platforms to indicate the presence of the ICPFUNC registers 
 with a has_pfunc
   platform data flag and enable this mode for platforms which supports 
 DT (da850 and
   Keystone 2 are two SoCs which support DT now and they also support 
 ICPFUNC registers).

 I'll add proper comment here.
 
 Just thinking: What happens if you try to use the ICPFUNC registers on 
 platforms that don't support it? If the answer is nothing bad, then 
 you might as well assume that if the platform doesn't specify its own 
 GPIOs, you can always try using the ICPFUNC registers to shake the I2C 
 bus. Better to try and fail than to never try at all...
 

I think the right answer is !nothing bad.

My intention was to enable this feature by default, because current 
DT-compatible
SoCs support it and the possibility that older SoCs will migrate to DT is low.
But now I think that the right way will be to add proper compatible strings
and use them to detect if ICPFUNC registers are supported or not.

[...]

regards,
-grygorii
--
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: [1/5] i2c: i2c-davinci: switch to use platform_get_irq

[Grygorii Strashko]

On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:
 Hello Grygorii,
 
 On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
 Switch Davinci I2C driver to use platform_get_irq(), because
 - it is not recommened to use
platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
resources any more, as they can be not ready yet in case of DT-booting.
 - it makes code simpler

 CC: Sekhar Nori nsek...@ti.com
 CC: Kevin Hilman khil...@deeprootsystems.com
 CC: Santosh Shilimkar ssant...@kernel.org
 CC: Murali Karicheri m-kariche...@ti.com
 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
 ---
   drivers/i2c/busses/i2c-davinci.c | 14 +++---
   1 file changed, 7 insertions(+), 7 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-davinci.c 
 b/drivers/i2c/busses/i2c-davinci.c
 index 4d96147..9bbfb8f 100644
 --- a/drivers/i2c/busses/i2c-davinci.c
 +++ b/drivers/i2c/busses/i2c-davinci.c
 @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device 
 *pdev)
   {
  struct davinci_i2c_dev *dev;
  struct i2c_adapter *adap;
 -struct resource *mem, *irq;
 -int r;
 +struct resource *mem;
 +int r, irq;
   
 -irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 -if (!irq) {
 -dev_err(pdev-dev, no irq resource?\n);
 -return -ENODEV;
 +irq = platform_get_irq(pdev, 0);
 One bad thing about platform_get_irq is its unusual handling of irq=0.
 I'm pretty sure you don't want to use this value, so adding something
 like:
 
   if (!irq)
   irq = -ENXIO
 
 would be welcome because the usual value for invalid irq is 0 and not
 -ESOMETHING. platform_get_irq is one of the very few functions that
 don't adhere to this convention. With handling = 0 as error your code
 is immune to changes in this area. Although I notice that
 platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
 
 Apart from your change I wonder if platform_get_irq should handle
 of_irq_get returning 0 as an error.

I think you are right and It seems like, the check for !irq should
be added/restored for OF case in platform_get_irq() too.

Also, I've simulated irq == 0 case - the .probe() failed with error -EINVAL
which is returned by request_threaded_irq() because of 
!irq_settings_can_request(desc).
 i2c_davinci 253.i2c: failure requesting irq 0
 i2c_davinci: probe of 253.i2c failed with error -22

I'm not sure that above will work for everyone because it depends on 
ARCH_IRQ_INIT_FLAGS
and ARCH_IRQ_INIT_FLAGS = (IRQ_NOREQUEST | IRQ_NOPROBE) for ARM.

 
 +if (irq  0) {
 +dev_err(pdev-dev, can't get irq resource ret=%d\n, irq);
 Please don't print an error if irq=-EPROBE_DEFER.
ok.

regards,
-grygorii

--
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: [2/5] i2c: davinci: query STP always when NACK is received

[Grygorii Strashko]

Hi Uwe,

On 11/21/2014 12:19 AM, Uwe Kleine-König wrote:
 On Thu, Nov 20, 2014 at 12:03:05PM +0200, Grygorii Strashko wrote:
 According to I2C specification the NACK should be handled as folowing:
 s/folowing/follows/
 
 When SDA remains HIGH during this ninth clock pulse, this is defined as the 
 Not
 Acknowledge signal. The master can then gene rate either a STOP condition to
 s/gene rate/generate/
 
 abort the transfer, or a repeated START condition to start a new transfer.
 [http://www.nxp.com/documents/user_manual/UM10204.pdf]
 The link is nice, but pointing out that this is the i2c spec would be
 nice.
 
 The same is recomened by TI I2C wiki:
 s/recomened/recommended/
 
   http://processors.wiki.ti.com/index.php/I2C_Tips
 If the specification tells what to do, there is no need to further
 support your claim.
 
 Currently, the Davinci I2C driver interrupts I2C trunsfer in case of NACK, 
 but
 s/trunsfer/transfer/
 
 It queries Stop condition DAVINCI_I2C_MDR_REG.STP=1 only if NACK has been 
 received
 s/It/it/
 
 during the last message transmitting/recieving.
 s/transmitting/transmitted/; s/recieving/received/
 
 I think I don't understand this sentence even with the typos corrected.
 Do you want to say:
 
 Currently the Davinci i2c driver interrupts the transfer on receipt of a
 NACK but fails to send a STOP in some situations and so makes the bus
 stuck.
 
 This may lead to Bus stuck in Bus Busy until I2C IP reset (idle/enable) if
 during SMBus reading transaction the first I2C message is NACKed.
 Did you hit this problem, or is this a theoretical issue?

I've hit this issue on OMAP board and the Davinci I2C driver is implemented in 
a similar way.
Now I've no HW which would allow me to reproduce it on Davinci.
quotes from https://lkml.org/lkml/2013/7/16/235:

The problem is, that lm75 device is SmBus compatible and its driver has
.detect() function implemented. During detection it tries to scan some
registers which might be not present in current device - in my case
it's tmp105.

For example to read regA in tmp105 following is done:
1) do write in Index register (val RegA index) (I2C 1st message)
2) do read (I2C 2d message)
the message 1 is Nacked by lm75 type of device in case if register index is 
wrong, 
but i2c-omap don't send NACK (or STP). As result, bus stack in Bus Busy 
state.

For SMBus devices the specification states (http://smbus.org/specs/)
4.2.Acknowledge (ACK) and not acknowledge (NACK):
- The slave device detects an invalid command or invalid data. In this
case the slave device must not acknowledge the received byte. The
master upon detection of this condition must generate a STOP condition
and retry the transaction


 
 Assuming this is a candidate for stable, adding a Fixes:-footer would be
 nice.
 
 Hence, fix it by querying Stop condition (STP) always when NACK is received.

 This patch fixes Davinci I2C in the same way it was done for OMAP I2C
 commit cda2109a26eb (i2c: omap: query STP always when NACK is received).

 More info can be found at:
 https://lkml.org/lkml/2013/7/16/159
 http://patchwork.ozlabs.org/patch/249790/
 I'd drop this more info paragraph.
 
 CC: Sekhar Nori nsek...@ti.com
 CC: Kevin Hilman khil...@deeprootsystems.com
 CC: Santosh Shilimkar ssant...@kernel.org
 CC: Murali Karicheri m-kariche...@ti.com
 Reported-by: Hein Tibosch hein_tibo...@yahoo.es
 Is this Reported-by tag reused from the omap issue?
 
 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
 ---
   drivers/i2c/busses/i2c-davinci.c | 8 +++-
   1 file changed, 3 insertions(+), 5 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-davinci.c 
 b/drivers/i2c/busses/i2c-davinci.c
 index 9bbfb8f..2cef115 100644
 --- a/drivers/i2c/busses/i2c-davinci.c
 +++ b/drivers/i2c/busses/i2c-davinci.c
 @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct 
 i2c_msg *msg, int stop)
  if (dev-cmd_err  DAVINCI_I2C_STR_NACK) {
  if (msg-flags  I2C_M_IGNORE_NAK)
  return msg-len;
 -if (stop) {
 -w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
 -w |= DAVINCI_I2C_MDR_STP;
 -davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 -}
 +w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
 +w |= DAVINCI_I2C_MDR_STP;
 +davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 I think this is a good change, but I wonder if the handling of
 I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
 for the 2nd byte of a 5-byte-message, the transfer supposed to
 continue, right? (Hmm, maybe the framework handle this and restarts the
 transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
 handle this flag?)

Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
change current behavior - davinci driver will interrupt transfer of i2c_msg 
always
 in case of NACK and start transfer of the next i2c_msg

Re: [1/5] i2c: i2c-davinci: switch to use platform_get_irq

[Grygorii Strashko]

On 11/21/2014 04:03 PM, Rob Herring wrote:

On Fri, Nov 21, 2014 at 5:01 AM, Grygorii Strashko
grygorii.stras...@ti.com wrote:

On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:

Hello Grygorii,

On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:

Switch Davinci I2C driver to use platform_get_irq(), because
- it is not recommened to use
platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
resources any more, as they can be not ready yet in case of DT-booting.
- it makes code simpler

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
   drivers/i2c/busses/i2c-davinci.c | 14 +++---
   1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..9bbfb8f 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
   {
  struct davinci_i2c_dev *dev;
  struct i2c_adapter *adap;
-struct resource *mem, *irq;
-int r;
+struct resource *mem;
+int r, irq;

-irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-if (!irq) {
-dev_err(pdev-dev, no irq resource?\n);
-return -ENODEV;
+irq = platform_get_irq(pdev, 0);

One bad thing about platform_get_irq is its unusual handling of irq=0.
I'm pretty sure you don't want to use this value, so adding something
like:

   if (!irq)
   irq = -ENXIO


I'll add this check in driver.



would be welcome because the usual value for invalid irq is 0 and not
-ESOMETHING. platform_get_irq is one of the very few functions that
don't adhere to this convention. With handling = 0 as error your code
is immune to changes in this area. Although I notice that
platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.

Apart from your change I wonder if platform_get_irq should handle
of_irq_get returning 0 as an error.


I think you are right and It seems like, the check for !irq should
be added/restored for OF case in platform_get_irq() too.


Changing the return values of platform_get_irq is tricky as it would
potentially break drivers because NO_IRQ can be 0 or -1 depending on
the arch. Drivers checking against specific values of NO_IRQ would
break. We've done some clean-up in this area, but I suspect more is
needed.


Thanks for your comment.

regards,
-grygorii
--
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: [2/5] i2c: davinci: query STP always when NACK is received

[Grygorii Strashko]

Hi Uwe,

On 11/21/2014 03:10 PM, Uwe Kleine-König wrote:
 On Fri, Nov 21, 2014 at 02:48:57PM +0200, Grygorii Strashko wrote:
 On 11/21/2014 12:19 AM, Uwe Kleine-König wrote:
 diff --git a/drivers/i2c/busses/i2c-davinci.c 
 b/drivers/i2c/busses/i2c-davinci.c
 index 9bbfb8f..2cef115 100644
 --- a/drivers/i2c/busses/i2c-davinci.c
 +++ b/drivers/i2c/busses/i2c-davinci.c
 @@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct 
 i2c_msg *msg, int stop)
if (dev-cmd_err  DAVINCI_I2C_STR_NACK) {
if (msg-flags  I2C_M_IGNORE_NAK)
return msg-len;
 -  if (stop) {
 -  w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
 -  w |= DAVINCI_I2C_MDR_STP;
 -  davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 -  }
 +  w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
 +  w |= DAVINCI_I2C_MDR_STP;
 +  davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 I think this is a good change, but I wonder if the handling of
 I2C_M_IGNORE_NAK is correct here. If the controller reports a NACK say
 for the 2nd byte of a 5-byte-message, the transfer supposed to
 continue, right? (Hmm, maybe the framework handle this and restarts the
 transfer with I2C_M_NOSTART but the davinci driver doesn't seem to
 handle this flag?)

 Have nothing to say about handling of I2C_M_IGNORE_NAK. I'm not going to
 change current behavior - davinci driver will interrupt transfer of i2c_msg 
 always
   in case of NACK and start transfer of the next i2c_msg (if exist).
 In my opinion, Above question is out of scope of this patch.
 Yeah right, that's exactly what I thought.
 
 Thinking again I wonder if with your change handling is correct when the
 sender wants to do a repeated start. That would need a more detailed
 look into the driver.

Davinci driver will always abort transfer with error -EREMOTEIO in case if
NACK received from I2C slave device. And the next omap_i2c_xfer() call may
be *not* targeted to the same I2C slave device.
^ if !I2C_M_IGNORE_NAK

This discussion is absolutely similar to https://lkml.org/lkml/2013/7/16/235
So, I'm just copy-pasting my answers from there ;)

regards,
-grygorii
--
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: [4/5] i2c: davinci: use bus recovery infrastructure

[Grygorii Strashko]

Hi Uwe,

On 11/21/2014 09:07 PM, Uwe Kleine-König wrote:
 On Thu, Nov 20, 2014 at 12:03:07PM +0200, Grygorii Strashko wrote:
 This patch converts Davinci I2C driver to use I2C bus recovery
 infrastructure, introduced by commit 5f9296ba21b3 (i2c: Add
 bus recovery infrastructure).

 The i2c_bus_recovery_info is configured for Davinci I2C adapter
 only in case if scl_pin is provided in Platform data at least.
 s/Platform/platform/
 

 Because the controller must be held in reset while doing so, the
 s/Because/As/
 
 recovery routine must re-init the controller. Since this was already
 being done after each call to i2c_recover_bus, move those calls into
 the recovery_prepare/unprepare routines and as well.

 CC: Sekhar Nori nsek...@ti.com
 CC: Kevin Hilman khil...@deeprootsystems.com
 CC: Santosh Shilimkar ssant...@kernel.org
 CC: Murali Karicheri m-kariche...@ti.com
 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
 ---
   drivers/i2c/busses/i2c-davinci.c | 76 
 ++--
   1 file changed, 35 insertions(+), 41 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-davinci.c 
 b/drivers/i2c/busses/i2c-davinci.c
 index 2cef115..db2a2cd 100644
 --- a/drivers/i2c/busses/i2c-davinci.c
 +++ b/drivers/i2c/busses/i2c-davinci.c
 @@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct 
 davinci_i2c_dev *i2c_dev, int reg)
  return readw_relaxed(i2c_dev-base + reg);
   }
   
 -/* Generate a pulse on the i2c clock pin. */
 -static void davinci_i2c_clock_pulse(unsigned int scl_pin)
 -{
 -u16 i;
 -
 -if (scl_pin) {
 -/* Send high and low on the SCL line */
 -for (i = 0; i  9; i++) {
 -gpio_set_value(scl_pin, 0);
 -udelay(20);
 -gpio_set_value(scl_pin, 1);
 -udelay(20);
 -}
 -}
 -}
 -
 -/* This routine does i2c bus recovery as specified in the
 - * i2c protocol Rev. 03 section 3.16 titled Bus clear
 - */
 -static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
 -{
 -u32 flag = 0;
 -struct davinci_i2c_platform_data *pdata = dev-pdata;
 -
 -dev_err(dev-dev, initiating i2c bus recovery\n);
 -/* Send NACK to the slave */
 -flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
 -flag |=  DAVINCI_I2C_MDR_NACK;
 -/* write the data into mode register */
 -davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
 -davinci_i2c_clock_pulse(pdata-scl_pin);
 -/* Send STOP */
 -flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
 -flag |= DAVINCI_I2C_MDR_STP;
 -davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
 -}
 -
   static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
  int val)
   {
 @@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
  return 0;
   }
   
 +/* This routine does i2c bus recovery as specified in the
 + * i2c protocol Rev. 03 section 3.16 titled Bus clear
 + */
 This comment is wrong. The actual bus clear is implemented by
 i2c_generic_gpio_recovery. Also while touching this comment, convert it
 to the usual format with /* on its own line. (The file in question has
 already both types of comment, so consistency is not a reason to keep it
 as is.)

It has been just copy-pasted, but ok.
I'll change this comment as following:
/* 
 * This routine does i2c bus recovery by using i2c_generic_gpio_recovery
 * which is provided by I2C Bus recovery infrastructure.
 */
Is it ok?

 
 Even though I remember that I reviewed this bus recovery patch (that
 resulted in 5f9296ba21b3) back then, I don't remember why it was split
 in prepare + recover + unprepare. But that is unrelated to this patch.
 
 +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
 +{
 +struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
 +
 +dev_err(dev-dev, initiating i2c bus recovery\n);
 +/* Disable interrupts */
 +davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
 +
 +/* put I2C into reset */
 +davinci_i2c_reset_ctrl(dev, 0);
 +}
 +
 +static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
 +{
 +struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
 +
 +i2c_davinci_init(dev);
 +}
 +
 +static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
 I'd call this only davinci_i2c_recovery_info.

No. Pls, see next patch.

 
 +.recover_bus = i2c_generic_gpio_recovery,
 +.prepare_recovery = davinci_i2c_prepare_recovery,
 +.unprepare_recovery = davinci_i2c_unprepare_recovery,
 +};
 new line here please

:) Ok.
Fixed in next patch.

 
   /*
* Waiting for bus not busy
*/
 @@ -286,8 +276,7 @@ static int i2c_davinci_wait_bus_not_busy(struct 
 davinci_i2c_dev *dev,
  return -ETIMEDOUT;
  } else {
  to_cnt = 0

[PATCH 2/5] i2c: davinci: query STP always when NACK is received

[Grygorii Strashko]

According to I2C specification the NACK should be handled as folowing:
When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then gene rate either a STOP condition to
abort the transfer, or a repeated START condition to start a new transfer.
[http://www.nxp.com/documents/user_manual/UM10204.pdf]

The same is recomened by TI I2C wiki:
 http://processors.wiki.ti.com/index.php/I2C_Tips

Currently, the Davinci I2C driver interrupts I2C trunsfer in case of NACK, but
It queries Stop condition DAVINCI_I2C_MDR_REG.STP=1 only if NACK has been 
received
during the last message transmitting/recieving.
This may lead to Bus stuck in Bus Busy until I2C IP reset (idle/enable) if
during SMBus reading transaction the first I2C message is NACKed.

Hence, fix it by querying Stop condition (STP) always when NACK is received.

This patch fixes Davinci I2C in the same way it was done for OMAP I2C
commit cda2109a26eb (i2c: omap: query STP always when NACK is received).

More info can be found at:
https://lkml.org/lkml/2013/7/16/159
http://patchwork.ozlabs.org/patch/249790/

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Reported-by: Hein Tibosch hein_tibo...@yahoo.es
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 9bbfb8f..2cef115 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct 
i2c_msg *msg, int stop)
if (dev-cmd_err  DAVINCI_I2C_STR_NACK) {
if (msg-flags  I2C_M_IGNORE_NAK)
return msg-len;
-   if (stop) {
-   w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-   w |= DAVINCI_I2C_MDR_STP;
-   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
-   }
+   w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+   w |= DAVINCI_I2C_MDR_STP;
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
return -EREMOTEIO;
}
return -EIO;
-- 
1.9.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 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Grygorii Strashko]

Having a board where the I2C bus locks up occasionally made it clear
that the bus recovery in the i2c-davinci driver will only work on
some boards, because on regular boards, this will only toggle GPIO
lines that aren't muxed to the actual pins.

The I2C controller on SoCs like da850 (and da830), Keystone 2 has the
built-in capability to bit-bang its lines by using the ICPFUNC registers
of the i2c controller.
Implement the suggested procedure by toggling SCL and checking SDA using
the ICPFUNC registers of the I2C controller when present. Allow platforms
to indicate the presence of the ICPFUNC registers with a has_pfunc platform
data flag and enable this mode for platforms which supports DT
(da850 and Keystone 2 are two SoCs which supports DT now and also
support ICPFUNC registers).

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Signed-off-by: Ben Gardiner bengardi...@nanometrics.ca
Signed-off-by: Mike Looijmans milo-softw...@users.sourceforge.net
[grygorii.stras...@ti.com: combined patches from Ben Gardiner and
Mike Looijmans and reimplemented ICPFUNC bus recovery using I2C
bus recovery infrastructure]
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c  | 100 +-
 include/linux/platform_data/i2c-davinci.h |   1 +
 2 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index db2a2cd..6cf96fb 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -64,6 +64,12 @@
 #define DAVINCI_I2C_IVR_REG0x28
 #define DAVINCI_I2C_EMDR_REG   0x2c
 #define DAVINCI_I2C_PSC_REG0x30
+#define DAVINCI_I2C_FUNC_REG   0x48
+#define DAVINCI_I2C_DIR_REG0x4c
+#define DAVINCI_I2C_DIN_REG0x50
+#define DAVINCI_I2C_DOUT_REG   0x54
+#define DAVINCI_I2C_DSET_REG   0x58
+#define DAVINCI_I2C_DCLR_REG   0x5c
 
 #define DAVINCI_I2C_IVR_AAS0x07
 #define DAVINCI_I2C_IVR_SCD0x06
@@ -97,6 +103,29 @@
 #define DAVINCI_I2C_IMR_NACK   BIT(1)
 #define DAVINCI_I2C_IMR_AL BIT(0)
 
+/* set SDA and SCL as GPIO */
+#define DAVINCI_I2C_FUNC_PFUNC0BIT(0)
+
+/* set SCL as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR0  BIT(0)
+/* set SDA as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR1  BIT(1)
+
+/* read SCL GPIO level */
+#define DAVINCI_I2C_DIN_PDIN0 BIT(0)
+/* read SDA GPIO level */
+#define DAVINCI_I2C_DIN_PDIN1 BIT(1)
+
+/*set the SCL GPIO high */
+#define DAVINCI_I2C_DSET_PDSET0BIT(0)
+/*set the SDA GPIO high */
+#define DAVINCI_I2C_DSET_PDSET1BIT(1)
+
+/* set the SCL GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR0BIT(0)
+/* set the SDA GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR1BIT(1)
+
 struct davinci_i2c_dev {
struct device   *dev;
void __iomem*base;
@@ -256,6 +285,72 @@ static struct i2c_bus_recovery_info 
davinci_i2c_gpio_recovery_info = {
.prepare_recovery = davinci_i2c_prepare_recovery,
.unprepare_recovery = davinci_i2c_unprepare_recovery,
 };
+
+static void davinci_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   if (val)
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_DSET_REG,
+ DAVINCI_I2C_DSET_PDSET0);
+   else
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_DCLR_REG,
+ DAVINCI_I2C_DCLR_PDCLR0);
+}
+
+static int davinci_i2c_get_scl(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+   int val;
+
+   /* read the state of SCL */
+   val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+   return val  DAVINCI_I2C_DIN_PDIN0;
+}
+
+static int davinci_i2c_get_sda(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+   int val;
+
+   /* read the state of SDA */
+   val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+   return val  DAVINCI_I2C_DIN_PDIN1;
+}
+
+static void davinci_i2c_scl_prepare_recovery(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   davinci_i2c_prepare_recovery(adap);
+
+   /* SCL output, SDA input */
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_DIR_REG, DAVINCI_I2C_DIR_PDIR0);
+
+   /* change to GPIO mode */
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_FUNC_REG,
+ DAVINCI_I2C_FUNC_PFUNC0);
+}
+
+static void davinci_i2c_scl_unprepare_recovery(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   /* change back to I2C mode */
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_FUNC_REG, 0);
+
+   davinci_i2c_unprepare_recovery(adap);
+}
+
+static struct i2c_bus_recovery_info

[PATCH 4/5] i2c: davinci: use bus recovery infrastructure

[Grygorii Strashko]

This patch converts Davinci I2C driver to use I2C bus recovery
infrastructure, introduced by commit 5f9296ba21b3 (i2c: Add
bus recovery infrastructure).

The i2c_bus_recovery_info is configured for Davinci I2C adapter
only in case if scl_pin is provided in Platform data at least.

Because the controller must be held in reset while doing so, the
recovery routine must re-init the controller. Since this was already
being done after each call to i2c_recover_bus, move those calls into
the recovery_prepare/unprepare routines and as well.

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c | 76 ++--
 1 file changed, 35 insertions(+), 41 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 2cef115..db2a2cd 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct 
davinci_i2c_dev *i2c_dev, int reg)
return readw_relaxed(i2c_dev-base + reg);
 }
 
-/* Generate a pulse on the i2c clock pin. */
-static void davinci_i2c_clock_pulse(unsigned int scl_pin)
-{
-   u16 i;
-
-   if (scl_pin) {
-   /* Send high and low on the SCL line */
-   for (i = 0; i  9; i++) {
-   gpio_set_value(scl_pin, 0);
-   udelay(20);
-   gpio_set_value(scl_pin, 1);
-   udelay(20);
-   }
-   }
-}
-
-/* This routine does i2c bus recovery as specified in the
- * i2c protocol Rev. 03 section 3.16 titled Bus clear
- */
-static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
-{
-   u32 flag = 0;
-   struct davinci_i2c_platform_data *pdata = dev-pdata;
-
-   dev_err(dev-dev, initiating i2c bus recovery\n);
-   /* Send NACK to the slave */
-   flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-   flag |=  DAVINCI_I2C_MDR_NACK;
-   /* write the data into mode register */
-   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-   davinci_i2c_clock_pulse(pdata-scl_pin);
-   /* Send STOP */
-   flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-   flag |= DAVINCI_I2C_MDR_STP;
-   davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-}
-
 static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
int val)
 {
@@ -266,6 +229,33 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
return 0;
 }
 
+/* This routine does i2c bus recovery as specified in the
+ * i2c protocol Rev. 03 section 3.16 titled Bus clear
+ */
+static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   dev_err(dev-dev, initiating i2c bus recovery\n);
+   /* Disable interrupts */
+   davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
+
+   /* put I2C into reset */
+   davinci_i2c_reset_ctrl(dev, 0);
+}
+
+static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
+{
+   struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+   i2c_davinci_init(dev);
+}
+
+static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
+   .recover_bus = i2c_generic_gpio_recovery,
+   .prepare_recovery = davinci_i2c_prepare_recovery,
+   .unprepare_recovery = davinci_i2c_unprepare_recovery,
+};
 /*
  * Waiting for bus not busy
  */
@@ -286,8 +276,7 @@ static int i2c_davinci_wait_bus_not_busy(struct 
davinci_i2c_dev *dev,
return -ETIMEDOUT;
} else {
to_cnt = 0;
-   davinci_i2c_recover_bus(dev);
-   i2c_davinci_init(dev);
+   i2c_recover_bus(dev-adapter);
}
}
if (allow_sleep)
@@ -376,8 +365,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct 
i2c_msg *msg, int stop)
  dev-adapter.timeout);
if (r == 0) {
dev_err(dev-dev, controller timed out\n);
-   davinci_i2c_recover_bus(dev);
-   i2c_davinci_init(dev);
+   i2c_recover_bus(adap);
dev-buf_len = 0;
return -ETIMEDOUT;
}
@@ -717,6 +705,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
adap-timeout = DAVINCI_I2C_TIMEOUT;
adap-dev.of_node = pdev-dev.of_node;
 
+   if (dev-pdata-scl_pin) {
+   adap-bus_recovery_info = davinci_i2c_gpio_recovery_info;
+   adap-bus_recovery_info-scl_gpio = dev-pdata-scl_pin

[PATCH 0/5] i2c: davinci improvements and fixes

[Grygorii Strashko]

This series contains some fixes and improvements for Davinci I2C:
patch 1 - small improvement
patch 2 - fixes Bus busy state for some case (was reported long time ago:()

patch 3-5 - converts driver to use I2C bus recovery infrastructure and
adds Davinci I2C bus recovery mechanizm based on using ICPFUNC registers.
These patches are combination of two patches from Ben Gardiner [1] and
Mike Looijmans [2] which i've reworked to use I2C bus recovery infrastructure

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/047305.html
[PATCH 0/6] i2c-davinci gpio pulsed SCL recovery with ICPFUNC
[2] https://lkml.org/lkml/2014/2/28/133
[PATCH] i2c-davinci: Implement a bus recovery that actually works

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com 

Grygorii Strashko (5):
  i2c: i2c-davinci: switch to use platform_get_irq
  i2c: davinci: query STP always when NACK is received
  i2c: recovery: change input parameter to i2c_adapter for
prepare/unprepare_recovery
  i2c: davinci: use bus recovery infrastructure
  i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

 drivers/i2c/busses/i2c-davinci.c  | 196 ++
 drivers/i2c/i2c-core.c|   4 +-
 include/linux/i2c.h   |   4 +-
 include/linux/platform_data/i2c-davinci.h |   1 +
 4 files changed, 148 insertions(+), 57 deletions(-)

-- 
1.9.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/5] i2c: i2c-davinci: switch to use platform_get_irq

[Grygorii Strashko]

Switch Davinci I2C driver to use platform_get_irq(), because
- it is not recommened to use
  platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
  resources any more, as they can be not ready yet in case of DT-booting.
- it makes code simpler

CC: Sekhar Nori nsek...@ti.com
CC: Kevin Hilman khil...@deeprootsystems.com
CC: Santosh Shilimkar ssant...@kernel.org
CC: Murali Karicheri m-kariche...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-davinci.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..9bbfb8f 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 {
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
-   struct resource *mem, *irq;
-   int r;
+   struct resource *mem;
+   int r, irq;
 
-   irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-   if (!irq) {
-   dev_err(pdev-dev, no irq resource?\n);
-   return -ENODEV;
+   irq = platform_get_irq(pdev, 0);
+   if (irq  0) {
+   dev_err(pdev-dev, can't get irq resource ret=%d\n, irq);
+   return irq;
}
 
dev = devm_kzalloc(pdev-dev, sizeof(struct davinci_i2c_dev),
@@ -661,7 +661,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
init_completion(dev-xfr_complete);
 #endif
dev-dev = pdev-dev;
-   dev-irq = irq-start;
+   dev-irq = irq;
dev-pdata = dev_get_platdata(pdev-dev);
platform_set_drvdata(pdev, dev);
 
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: i2c: omap: fix NACK and Arbitration Lost irq handling

[Grygorii Strashko]

On 11/15/2014 03:12 AM, Alexander Kochetkov wrote:

commit 1d7afc95946487945cc7f5019b41255b72224b70 (i2c: omap: ack IRQ in parts)
changed the interrupt handler to complete transfers without clearing
XRDY (AL case) and ARDY (NACK case) flags. XRDY or ARDY interrupt will be
fired again (in parallel with omap_i2c_xfer_msg). Interrupt handler will
complete transfers second time. As a result, NACK and AL transfers
terminates with transfer timeout and sometimes client code segfault.

The patch restore original logic of handling NACK and AL interrupts and
fix race between interrupt handler and omap_i2c_xfer_msg (for AL and
NACK case only).

Tested on Beagleboard XM C.


Seems you've got the same issue as I :) long time ago
https://lkml.org/lkml/2013/6/7/530



Signed-off-by: Alexander Kochetkov al.koc...@gmail.com
---
  drivers/i2c/busses/i2c-omap.c |2 --
  1 file changed, 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 90dcc2e..9af7095 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -926,14 +926,12 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
if (stat  OMAP_I2C_STAT_NACK) {
err |= OMAP_I2C_STAT_NACK;
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
-   break;
}

if (stat  OMAP_I2C_STAT_AL) {
dev_err(dev-dev, Arbitration lost\n);
err |= OMAP_I2C_STAT_AL;
omap_i2c_ack_stat(dev, OMAP_I2C_STAT_AL);
-   break;
}

/*



--
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-davinci.c: CPU FREQ causes lock up due to xfr_complete

[Grygorii Strashko]

On 07/30/2014 09:18 AM, Sekhar Nori wrote:

On Tuesday 29 July 2014 11:00 PM, Grygorii Strashko wrote:

Hi Jon,

On 07/29/2014 06:53 PM, Jon Cormier wrote:

A slimmer patch suggested by Grygorii Strashko grygorii.stras...@ti.com



Ok. The problem seems to be deeper than at first look.
You have sequence (in Mainline kernel):
cpufreq:
  |- notify CPUFREQ_PRECHANGE
 |- i2c-davinci will lock  put I2C in reset
  |- cpufreq_driver-target_index
 |- davinci_target()
|- pdata-set_voltage() - It will try to use I2C to set new voltage,
while I2C is in reset or locked! Bug!
  |- notify CPUFREQ_POSTCHANGE
 |- i2c-davinci will re-enable I2C and adjust I2C clock


Good find. I really wonder how this escaped so far. I can swear cpufreq
transitions were tested multiple times. From the description it looks
like this should hit every single time there is a voltage adjustment.

On DA850 which is the only DaVinci implementing cpufreq, I2C0 input
frequency will remain constant across cpufreq transitions since it
derives from PLL0 AUXCLK which is used pre-multipler/divider. It remains
fixed.

The code seems to have been added for I2C1 which does have a fixed ratio
with cpu clock.

PMIC should usually be put on I2C0 to help prevent these kind of issues.


I see few possible ways to solve it:
1) use CLK notifier instead of CPUfreq notifiers


This will require common clock framework, right? We dont have that on
mach-davinci.


:( I've forgotten about that.




2) do smth similar to 61c7cff8 i2c: S3C24XX I2C frequency scaling support
   + 9bcd04bf i2c: s3c2410: grab adapter lock while changing i2c clock


This looks promising. Although I wonder if delta_f will always remain
zero in s3c24xx_i2c_cpufreq_transition() when the CPUFREQ_PRECHANGE call
is made - because clock tree is not updated yet?


That's funny - seems PRE/POST notifiers are called twice for s3c24xx :)
First one from cpufreq core.
Second time from s3c_cpufreq_target() and, looks like, clock
freq will be updated at that time.





3) update I2C clock in CPUFREQ_POSTCHANGE - may be unsafe


Well, even now the I2C clock is only getting updated in POSTCHANGE,
right? Also, resetting I2C in PRECHANGE seems excessive. It is only
required when changing the prescalar. So you can do:

} else if (val == CPUFREQ_POSTCHANGE) {
davinci_i2c_reset_ctrl(dev, 0);
i2c_davinci_calc_clk_dividers(dev);
davinci_i2c_reset_ctrl(dev, 1);
}

So this along with the i2c_lock_adapter() a la
s3c24xx_i2c_cpufreq_transition() should be the right fix, I guess.



Regards,
-grygorii
--
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 1/5] i2c: Don't start transfers when suspended

[Grygorii Strashko]

Hi

On 07/17/2014 05:48 PM, Bastian Hecht wrote:
 i2c transfer requests come in very uncontrolled, like from interrupt routines.
 We might be suspended when this happens. To avoid i2c timeouts caused by
 powered down busses we check for suspension.
 
 Several bus drivers handle this problem on their own. We can clean things up
 by moving the protection mechanism into the core.

 I'm not sure, this optimization is safe  (
Because, in many cases the access to PMIC IC needs to be allowed till late
stages of suspending (at least till suspend_noirq stage or even later).
For example, on some OMAP SoC Voltage management code need to use services
provided by PMIC IC, which is connected to I2C.

As result, it may cause regression and break PM functionality on some SoCs
if i2c-core will block I2c transfers unconditionally at device's
suspending stage.

May be it will be useful to add just suspended field in i2c adapter
structure and provide corresponding accessors.

 
 Signed-off-by: Bastian Hecht hec...@gmail.com
 ---
 changelog v2:
 
 - commit message extended.
 - initialization added for adap-suspended
 - swapped branch for increased performance
 
 
   drivers/i2c/i2c-core.c | 25 -
   include/linux/i2c.h|  1 +
   2 files changed, 25 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
 index 7c7f4b8..b15dc20 100644
 --- a/drivers/i2c/i2c-core.c
 +++ b/drivers/i2c/i2c-core.c
 @@ -343,6 +343,13 @@ static int i2c_legacy_resume(struct device *dev)
   static int i2c_device_pm_suspend(struct device *dev)
   {
   const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL;
 + struct i2c_adapter *adap = i2c_verify_adapter(dev);
 +
 + if (adap) {
 + i2c_lock_adapter(adap);
 + adap-suspended = true;
 + i2c_unlock_adapter(adap);
 + }
   
   if (pm)
   return pm_generic_suspend(dev);
 @@ -353,6 +360,13 @@ static int i2c_device_pm_suspend(struct device *dev)
   static int i2c_device_pm_resume(struct device *dev)
   {
   const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL;
 + struct i2c_adapter *adap = i2c_verify_adapter(dev);
 +
 + if (adap) {
 + i2c_lock_adapter(adap);
 + adap-suspended = false;
 + i2c_unlock_adapter(adap);
 + }
   
   if (pm)
   return pm_generic_resume(dev);
 @@ -1243,6 +1257,7 @@ static int i2c_register_adapter(struct i2c_adapter 
 *adap)
   dev_set_name(adap-dev, i2c-%d, adap-nr);
   adap-dev.bus = i2c_bus_type;
   adap-dev.type = i2c_adapter_type;
 + adap-suspended = false;
   res = device_register(adap-dev);
   if (res)
   goto out_list;
 @@ -1819,7 +1834,10 @@ int i2c_transfer(struct i2c_adapter *adap, struct 
 i2c_msg *msgs, int num)
   i2c_lock_adapter(adap);
   }
   
 - ret = __i2c_transfer(adap, msgs, num);
 + if (!adap-suspended)
 + ret = __i2c_transfer(adap, msgs, num);
 + else
 + ret = -EIO;
   i2c_unlock_adapter(adap);
   
   return ret;
 @@ -2577,6 +2595,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 
 addr, unsigned short flags,
   
   if (adapter-algo-smbus_xfer) {
   i2c_lock_adapter(adapter);
 + if (adapter-suspended) {
 + res = -EIO;
 + goto unlock;
 + }
   
   /* Retry automatically on arbitration loss */
   orig_jiffies = jiffies;
 @@ -2590,6 +2612,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 
 addr, unsigned short flags,
  orig_jiffies + adapter-timeout))
   break;
   }
 +unlock:
   i2c_unlock_adapter(adapter);
   
   if (res != -EOPNOTSUPP || !adapter-algo-master_xfer)
 diff --git a/include/linux/i2c.h b/include/linux/i2c.h
 index b556e0a..af08c75 100644
 --- a/include/linux/i2c.h
 +++ b/include/linux/i2c.h
 @@ -434,6 +434,7 @@ struct i2c_adapter {
   int timeout;/* in jiffies */
   int retries;
   struct device dev;  /* the adapter device */
 + unsigned int suspended:1;
   
   int nr;
   char name[48];
 

Regards,
- grygroii
--
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: davinci: raw read and write endian fix

[Grygorii Strashko]

On 11/20/2013 08:23 PM, Taras Kondratiuk wrote:

I2C IP block expect LE data, but CPU may operate in BE mode.
Need to use endian neutral functions to read/write h/w registers.
I.e instead of __raw_read[lw] and __raw_write[lw] functions code
need to use read[lw]_relaxed and write[lw]_relaxed functions.
If the first simply reads/writes register, the second will byteswap
it if host operates in BE mode.

Changes are trivial sed like replacement of __raw_xxx functions
with xxx_relaxed variant.


Reviewed-by: Grygorii Strashko grygorii.stras...@ti.com



Signed-off-by: Taras Kondratiuk taras.kondrat...@linaro.org
---
Based on Linus' master tip (b4789b8).
Tested on Keystone2 EVM.

  drivers/i2c/busses/i2c-davinci.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 132369f..a629447 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -125,12 +125,12 @@ static struct davinci_i2c_platform_data 
davinci_i2c_platform_data_default = {
  static inline void davinci_i2c_write_reg(struct davinci_i2c_dev *i2c_dev,
 int reg, u16 val)
  {
-   __raw_writew(val, i2c_dev-base + reg);
+   writew_relaxed(val, i2c_dev-base + reg);
  }

  static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int 
reg)
  {
-   return __raw_readw(i2c_dev-base + reg);
+   return readw_relaxed(i2c_dev-base + reg);
  }

  /* Generate a pulse on the i2c clock pin. */



--
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-omap: always send stop after nack

[Grygorii Strashko]

On 08/19/2013 03:11 PM, Wolfram Sang wrote:

Hi,


Which means your original patch starts to make a lot more sense. I
wonder is this is really what we should be doing though - breaking out
of the loop, I mean.


Yup, that is fine. I applied the old patch with Acks from Hein and
Felipe to -next. Thanks!


Thanks.




It looks like TI's i2c-davinci will have the same problem as i2c-omap,
and will need the same change.


Somebody up for this?



Regards,
-grygorii
--
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-omap: always send stop after nack

[Grygorii Strashko]

Hi Hein, Felipe

On 07/16/2013 12:42 PM, Felipe Balbi wrote:

Hi,

On Tue, Jul 16, 2013 at 05:33:47PM +0800, Hein Tibosch wrote:

On 7/16/2013 5:03 PM, Felipe Balbi wrote:

Hi,

On Tue, Jul 16, 2013 at 04:19:35PM +0800, Hein Tibosch wrote:

Hi Vikram,

On a OMAP4460, i2c-bus-3:

A driver (lm75) is causing many 'timeout waiting for bus ready' errors.
SDA remains high (as it should), but SCL remains low after a NACK.
The bus becomes _unusable for other clients_.

While probing, lm75 writes a command, followed by a read + stop,
but the write command is NACK'd. The chip does accept other writes/reads,
it just refuses to ack invalid commands.

Can you tell me if the patch below would make any sense? Or is it the
responsibility of the client to reset the i2c_smbus?

patch below breaks repeated start.

Felipe, I'd very appreciate if you'd be able to provide the use case
which will fail with such solution?


Hi,

No, after the NACK, no more commands are being processed,
including a repeated start. omap_i2c_xfer() returns -EREMOTEIO
without ever freeing the bus.

The bus is left in an impossible state with SCL constantly low
and all next commands (to different chips) will therefore get
a -ETIMEDOUT

With this patch, the bus will become idle again and new commands
can be processed normally

I think, this is valid fix, but it was done here already:)
http://patchwork.ozlabs.org/patch/249790/
i2c: omap: query STP always when NACK is received

And nacked in the same way :(
But! I've back-ported my patch on TI Android product kernel 3.4, did
sanity test and I didn't see any issues with my patch :))



but you mentioned that if you have IGNORE_NAK set, everything is fine,
since lm75 will get a return value of 0 and things will work just fine,
right ?

Also, you also said that the chip 'refuses to ack invalid commands', why
are you sending invalid commands to start with ? This could be a bug in
i2c-omap.c, sure, but let's try to figure out why IGNORE_NAK helps and
why is lm75 driver sending invalid commands.



The problem is, that lm75 device is SmBus compatible and its driver has
.detect() function implemented. During detection it tries to scan some
registers which might be not present in current device - in my case
it's tmp105.

For example to read regA in tmp105 following is done:
1) do write in Index register (val RegA index) (I2C 1st message)
2) do read (I2C 2d message)
the message 1 is Nacked by device in case if register index is wrong, 
but i2c-omap don't send NACK (or STP). As result, bus stack in Bus Busy 
state.


For SMBus devices the specification states (http://smbus.org/specs/)
4.2.Acknowledge (ACK) and not acknowledge (NACK):
- The slave device detects an invalid command or invalid data. In this
case the slave device must not acknowledge the received byte. The
master upon detection of this condition must generate a STOP condition
and retry the transaction

Regards,
-grygorii

--
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-omap: always send stop after nack

[Grygorii Strashko]

Hi Felipe,
On 07/16/2013 02:27 PM, Felipe Balbi wrote:

Hi,

On Tue, Jul 16, 2013 at 02:01:11PM +0300, Grygorii Strashko wrote:

On a OMAP4460, i2c-bus-3:

A driver (lm75) is causing many 'timeout waiting for bus ready' errors.
SDA remains high (as it should), but SCL remains low after a NACK.
The bus becomes _unusable for other clients_.

While probing, lm75 writes a command, followed by a read + stop,
but the write command is NACK'd. The chip does accept other writes/reads,
it just refuses to ack invalid commands.

Can you tell me if the patch below would make any sense? Or is it the
responsibility of the client to reset the i2c_smbus?

patch below breaks repeated start.

Felipe, I'd very appreciate if you'd be able to provide the use case
which will fail with such solution?


can't you see how this would fail ?

assume omap_i2c_xfer() is called with its last argument (num) being
greater than one and you get the NAK before the last transfer.

That's our case - NACK from slave before last transfer


Will you not be breaking a possible repeated start for the following
transfer ?
Sorry, but in this case omap_i2c_xfer() will be aborted and there would 
be no transfers until next call to omap_i2c_xfer().

Which, in turn, may address another device!?





No, after the NACK, no more commands are being processed,
including a repeated start. omap_i2c_xfer() returns -EREMOTEIO
without ever freeing the bus.

The bus is left in an impossible state with SCL constantly low
and all next commands (to different chips) will therefore get
a -ETIMEDOUT

With this patch, the bus will become idle again and new commands
can be processed normally

I think, this is valid fix, but it was done here already:)
http://patchwork.ozlabs.org/patch/249790/
i2c: omap: query STP always when NACK is received

And nacked in the same way :(
But! I've back-ported my patch on TI Android product kernel 3.4, did
sanity test and I didn't see any issues with my patch :))


that's because you don't care about repeated start, but that's a valid
bus signal which needs to be supported.





but you mentioned that if you have IGNORE_NAK set, everything is fine,
since lm75 will get a return value of 0 and things will work just fine,
right ?

Also, you also said that the chip 'refuses to ack invalid commands', why
are you sending invalid commands to start with ? This could be a bug in
i2c-omap.c, sure, but let's try to figure out why IGNORE_NAK helps and
why is lm75 driver sending invalid commands.



The problem is, that lm75 device is SmBus compatible and its driver has
.detect() function implemented. During detection it tries to scan some
registers which might be not present in current device - in my case
it's tmp105.

For example to read regA in tmp105 following is done:
1) do write in Index register (val RegA index) (I2C 1st message)
2) do read (I2C 2d message)
the message 1 is Nacked by device in case if register index is wrong,
but i2c-omap don't send NACK (or STP). As result, bus stack in Bus
Busy state.


wait a minute, it's not i2c-omap which needs to send NAK, it's LM75,
and it does the NAK. The handling for NAK in the i2c framework is to
return -EREMOTEIO as we do. If our last message got a NAK, we send STOP
because there will be no other transfers following this one, namely, the
for loop in omap_i2c_xfer() will be finished.

Sorry, wrong descr, my bad - slave sends NACK (lm75), master (OMAP i2)
should send STP.
But, you *can* send STT if you wish to continue with next
message to the *same* device - which is not true for OMAP i2c, because 
OMAP I2C driver always interrupts transfer with error -EREMOTEIO!!

And, again:), next call of omap_i2c_xfer() may be *not* to the same
slave I2C device.




For SMBus devices the specification states (http://smbus.org/specs/)
4.2.Acknowledge (ACK) and not acknowledge (NACK):
- The slave device detects an invalid command or invalid data. In this
case the slave device must not acknowledge the received byte. The
master upon detection of this condition must generate a STOP condition
and retry the transaction


hmm, but that's something that the OMAP I2C controller doesn't support
and is emulated by the i2c framework, right ?

If you look into the I2C specification, the one the OMAP controller is
compliant to, you'll see e.g. in Figure 13 that a repeated start is a
valid condition after a NAK.

Also it states that:

This is indicated by the slave generating the not-acknowledge on the
first byte to follow. The slave leaves the data line HIGH and the master
generates a STOP or a repeated START condition.

Because the OMAP I2C controller is compliant to the I2C specification,
not the SMBus specification, we must follow through with the loop and
let the next message try to send a repeated start.

What you need here is a way to discriminate between SMBus message and
normal I2C message, that way you could have something like:
I don't think that is right (my explanation above) - the same can happen 
even with pure

Re: [RFC] i2c: add deprecation warning for class based instantiation

[Grygorii Strashko]

Hi Wolfram,

On 06/19/2013 01:15 PM, Wolfram Sang wrote:

On Fri, Jun 07, 2013 at 11:09:26AM +0200, Wolfram Sang wrote:

Class based instantiation can cause huge delays when booting. This
mechanism was used when it was not possible to describe slaves on I2C
busses. We now have other mechanisms, so most embedded I2C will not need
classes and it was explicitly not recommended to use them. Add a
deprecation warning for drivers who want to disable class based in the
near future to gain boot-up time, so users relying on this technique can
switch to something better. They really should.

Signed-off-by: Wolfram Sang w...@the-dreams.de

No reactions on this, pity. Deferring.


Sorry, for delayed reply - I've had problems with my e-mail.

I've tested this patch on our TI K3.4 product kernel with additional 
change below:

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index c0330a4..442101d 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -186,7 +186,7 @@ static int __devinit i2c_gpio_probe(struct 
platform_device *pdev)

adap-owner = THIS_MODULE;
snprintf(adap-name, sizeof(adap-name), i2c-gpio%d, pdev-id);
adap-algo_data = bit_data;
-   adap-class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
+   adap-class = I2C_CLASS_HWMON | I2C_CLASS_SPD | 
I2C_CLASS_DEPRECATED;

adap-dev.parent = pdev-dev;
adap-dev.of_node = pdev-dev.of_node;

It works fine, in general - see my minor comment inline.
[0.662536] omap_i2c omap_i2c.2: bus 2 rev2.4.0 at 400 kHz
[0.662567]  (null): This adapter will soon drop class based 
instantiation of slaves

^ invalid adapter device name here
[0.662567] Please make sure all its I2C devices are instantiated by 
other means.

[0.662567] Check 'Documentation/i2c/instantiating-devices' for details

Also tested on Linux master - OMAP4 SDP board.

In addition, I've found the following users of *i2c-gpio* (just FYI):
./arch/powerpc/boot/dts/wii.dts:compatible = i2c-gpio;
./arch/mips/alchemy/board-gpr.c:.name= i2c-gpio,
./arch/mips/ath79/mach-pb44.c:.name= i2c-gpio,
./arch/avr32/boards/merisc/setup.c:.name= i2c-gpio,
./arch/avr32/boards/atngw100/setup.c:.name= i2c-gpio,
./arch/avr32/boards/hammerhead/setup.c:.name= i2c-gpio,
./arch/avr32/boards/mimc200/setup.c:.name= i2c-gpio,
./arch/blackfin/mach-bf533/boards/blackstamp.c:.name= 
i2c-gpio,

./arch/blackfin/mach-bf533/boards/ezkit.c:.name= i2c-gpio,
./arch/blackfin/mach-bf533/boards/stamp.c:.name= i2c-gpio,
./arch/blackfin/mach-bf561/boards/ezkit.c:.name= i2c-gpio,
./arch/arm/mach-ep93xx/core.c:.name= i2c-gpio,
./arch/arm/mach-shmobile/board-armadillo800eva.c:.name = i2c-gpio,
./arch/arm/mach-pxa/viper.c:.name= i2c-gpio,
./arch/arm/mach-pxa/viper.c:tpm_device = 
platform_device_alloc(i2c-gpio, 2);

./arch/arm/mach-pxa/palmz72.c:.name= i2c-gpio,
./arch/arm/boot/dts/at91sam9g45.dtsi:compatible = i2c-gpio;
./arch/arm/boot/dts/at91sam9263.dtsi:compatible = i2c-gpio;
./arch/arm/boot/dts/at91sam9x5.dtsi:compatible = i2c-gpio;
./arch/arm/boot/dts/at91sam9x5.dtsi:compatible = i2c-gpio;
./arch/arm/boot/dts/at91sam9x5.dtsi:compatible = i2c-gpio;
./arch/arm/boot/dts/usb_a9g20-dab-mmx.dtsi:i2c-gpio@0 {
./arch/arm/boot/dts/ste-nomadik-stn8815.dtsi:compatible = 
i2c-gpio;
./arch/arm/boot/dts/ste-nomadik-stn8815.dtsi:compatible = 
i2c-gpio;
./arch/arm/boot/dts/ste-nomadik-stn8815.dtsi:compatible = 
i2c-gpio;

./arch/arm/boot/dts/at91sam9260.dtsi:compatible = i2c-gpio;
./arch/arm/boot/dts/at91rm9200.dtsi:compatible = i2c-gpio;
./arch/arm/boot/dts/at91sam9n12.dtsi:compatible = i2c-gpio;
./arch/arm/mach-ks8695/board-acs5k.c:.name= i2c-gpio,
./arch/arm/mach-s5pv210/mach-goni.c:.name= i2c-gpio,
./arch/arm/mach-s5pv210/mach-goni.c:.name= i2c-gpio,
./arch/arm/mach-s5pv210/mach-aquila.c:.name= i2c-gpio,
./arch/arm/mach-s5pv210/mach-aquila.c:.name= i2c-gpio,
./arch/arm/mach-sa1100/simpad.c:.name = i2c-gpio,
./arch/arm/mach-exynos/mach-universal_c210.c:.name= i2c-gpio,
./arch/arm/mach-exynos/mach-nuri.c:.name= i2c-gpio,
./arch/arm/mach-at91/at91sam9263_devices.c:.name= 
i2c-gpio,
./arch/arm/mach-at91/at91sam9261_devices.c:.name= 
i2c-gpio,
./arch/arm/mach-at91/at91sam9g45_devices.c:.name= 
i2c-gpio,
./arch/arm/mach-at91/at91sam9g45_devices.c:.name= 
i2c-gpio,

./arch/arm/mach-at91/at91rm9200_devices.c:.name= i2c-gpio,
./arch/arm/mach-at91/at91sam9260_devices.c:.name= 
i2c-gpio,

./arch/arm/mach-at91/at91sam9rl_devices.c:.name= i2c-gpio,

Re: [RFC] i2c: add deprecation warning for class based instantiation

[Grygorii Strashko]

On 06/07/2013 12:09 PM, Wolfram Sang wrote:

Class based instantiation can cause huge delays when booting. This
mechanism was used when it was not possible to describe slaves on I2C
busses. We now have other mechanisms, so most embedded I2C will not need
classes and it was explicitly not recommended to use them. Add a
deprecation warning for drivers who want to disable class based in the
near future to gain boot-up time, so users relying on this technique can
switch to something better. They really should.

Signed-off-by: Wolfram Sang w...@the-dreams.de
---
  drivers/i2c/busses/i2c-omap.c |2 +-
  drivers/i2c/i2c-core.c|6 ++
  include/linux/i2c.h   |1 +
  3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e02f9e3..f06109f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1231,7 +1231,7 @@ omap_i2c_probe(struct platform_device *pdev)
adap = dev-adapter;
i2c_set_adapdata(adap, dev);
adap-owner = THIS_MODULE;
-   adap-class = I2C_CLASS_HWMON;
+   adap-class = I2C_CLASS_HWMON | I2C_CLASS_DEPRECATED;
strlcpy(adap-name, OMAP I2C adapter, sizeof(adap-name));
adap-algo = omap_i2c_algo;
adap-dev.parent = pdev-dev;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 48e31ed..47ea1f0 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -999,6 +999,12 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
return -EINVAL;
}
  
+	if (adap-class  I2C_CLASS_DEPRECATED)

+   dev_warn(adap-dev, This adapter will soon drop class based 
+   instantiation of slaves\nPlease make sure all its I2C 
+   devices are instantiated by other means.\nCheck 
+   'Documentation/i2c/instantiating-devices' for 
details\n);
+
Seems, this need to be moved down - after res = 
device_register(adap-dev);
- or - right before: bus_for_each_drv(i2c_bus_type, NULL, adap, 
__process_new_adapter);




rt_mutex_init(adap-bus_lock);
mutex_init(adap-userspace_clients_lock);
INIT_LIST_HEAD(adap-userspace_clients);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index e988fa9..ffab838 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -473,6 +473,7 @@ void i2c_unlock_adapter(struct i2c_adapter *);
  #define I2C_CLASS_HWMON   (10)/* lm_sensors, ... */
  #define I2C_CLASS_DDC (13)/* DDC bus on graphics adapters */
  #define I2C_CLASS_SPD (17)/* Memory modules */
+#define I2C_CLASS_DEPRECATED   (18)/* Warn users that adapter will stop 
using classes */
  
  /* Internal numbers to terminate lists */

  #define I2C_CLIENT_END0xfffeU




- grygorii
--
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

[RFC 1/2] i2c: omap: drop class based instantiation of slaves

[Grygorii Strashko]

Class based instantiation mechanism can cause huge delays when booting.
For example: when CONFIG_SENSORS_LM75 option is enabled for omap4-sdp board -
it introduces 5-6 ms boot delay.
It's not recommended to use this mechanism with embedded I2C, so disable
it by leaving I2C adapter class field undefined (remove I2C_CLASS_HWMON).

CC: Tony Lindgren t...@atomide.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-omap.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index f06109f..ae2b27f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1231,7 +1231,7 @@ omap_i2c_probe(struct platform_device *pdev)
adap = dev-adapter;
i2c_set_adapdata(adap, dev);
adap-owner = THIS_MODULE;
-   adap-class = I2C_CLASS_HWMON | I2C_CLASS_DEPRECATED;
+   adap-class = I2C_CLASS_DEPRECATED;
strlcpy(adap-name, OMAP I2C adapter, sizeof(adap-name));
adap-algo = omap_i2c_algo;
adap-dev.parent = pdev-dev;
-- 
1.7.9.5

--
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

[RFC 2/2] i2c: gpio: drop class based instantiation of slaves

[Grygorii Strashko]

Class based instantiation mechanism can cause huge delays when booting.
For example: when CONFIG_SENSORS_LM75 option is enabled for or omap5-uevm board,
where i2c-gpio is used for HDMI edid reading - it introduces up to 5 sec boot
delay.
It's not recommended to use this mechanism with embedded I2C, so disable it by
leaving I2C adapter class field undefined (remove I2C_CLASS_HWMON and
I2C_CLASS_SPD) and add a deprecation warning to allow users, relying on this
mechanism, to switch to something better.

CC: Haavard Skinnemoen hskinnem...@gmail.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-gpio.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index bc6e139..33e3d0e 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -215,7 +215,7 @@ static int i2c_gpio_probe(struct platform_device *pdev)
snprintf(adap-name, sizeof(adap-name), i2c-gpio%d, 
pdev-id);
 
adap-algo_data = bit_data;
-   adap-class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
+   adap-class = I2C_CLASS_DEPRECATED;
adap-dev.parent = pdev-dev;
adap-dev.of_node = pdev-dev.of_node;
 
-- 
1.7.9.5

--
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 3/5] i2c: omap: handle all irqs befor unblocking omap_i2c_xfer_msg()

[Grygorii Strashko]

Hi Felipe,
On 06/07/2013 10:05 PM, Felipe Balbi wrote:

Hi,

On Fri, Jun 07, 2013 at 09:46:06PM +0300, Grygorii Strashko wrote:

ARDY|NACK and ARDY|AL are set together in OMAP_I2C_STAT_REG, which will be

Have you seen that happen ever ? AL is Arbitration Lost, we never put
OMAP in a multi-master environment before.

This is an example from real life:
[0.271942] omap_i2c omap_i2c.1: bus 1 rev2.4.0 at 400 kHz
[1.283416] omap_i2c omap_i2c.1: timeout waiting for bus ready
[1.300109] OMAP_I2C DEBUG: stat=1001
[1.300140] omap_i2c omap_i2c.1: Arbitration lost
[1.300140] OMAP_I2C DEBUG: IE=601F
[1.300140] OMAP_I2C DEBUG: STAT=1000
[1.300170] OMAP_I2C DEBUG: IV=636F
[1.300170] OMAP_I2C DEBUG: WE=636F
[1.300170] OMAP_I2C DEBUG: SYSS=1
[1.300170] OMAP_I2C DEBUG: BUF=707
[1.300201] OMAP_I2C DEBUG: CNT=1
[1.300201] OMAP_I2C DEBUG: DATA=1
[1.300201] OMAP_I2C DEBUG: SYSC=215
[1.300201] OMAP_I2C DEBUG: CON=8200
[1.300231] OMAP_I2C DEBUG: OA=0
[1.300231] OMAP_I2C DEBUG: SA=49
[1.300231] OMAP_I2C DEBUG: PSC=9
[1.300262] OMAP_I2C DEBUG: SCLL=9
[1.300262] OMAP_I2C DEBUG: SCLH=3
[1.300262] OMAP_I2C DEBUG: SYSTEST=1E0
[1.300262] OMAP_I2C DEBUG: BUFSTAT=4000

and my headache now :..(


ARDY | NACK I also find it a bit hard for those two to happen together
since ARDY will be set when you can change controller's register
*again*, mening that a transfer has completed.

There are examples:
[3.544952] omap_i2c 4806.i2c: IRQ (ISR = 0x0006)

[   25.574523] omap_i2c 4835.i2c: IRQ (ISR = 0x0014)
[   25.579925] omap_i2c 4835.i2c: IRQ (ISR = 0x0012)

to see it - enable debug output in omap_i2c_isr_thread:
dev_dbg(dev-dev, IRQ (ISR = 0x%04x)\n, stat);



Also, we need to follow what the programming model says. And, I don't
have docs with me right now, but IIRC it tells us to bail out if any of
the error conditions are met.


yep, but first of all - all IRQs need to be acked before exit.

Sorry, for delayed reply - I've had problems with my e-mail.

- grygorii

--
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/5] i2c: omap: add runtime check in isr to be sure that i2c is enabled

[Grygorii Strashko]

Hi Felipe,
On 06/07/2013 10:02 PM, Felipe Balbi wrote:

Hi,

On Fri, Jun 07, 2013 at 09:46:05PM +0300, Grygorii Strashko wrote:

Add runtime check at the beginning of omap_i2c_isr/omap_i2c_isr_thread
to be sure that i2c is enabled, before performing IRQ handling and accessing
I2C IP registers:
if (pm_runtime_suspended(dev-dev)) {
WARN_ONCE(true, We should never be here!\n);
return IRQ_NONE;
}

Produce warning in case if ISR called when i2c is disabled

CC: Kevin Hilman khil...@linaro.org
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
  drivers/i2c/busses/i2c-omap.c |   10 ++
  1 file changed, 10 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 97844ff..2dac598 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -885,6 +885,11 @@ omap_i2c_isr(int irq, void *dev_id)
u16 stat;
  
  	spin_lock(dev-lock);

+   if (pm_runtime_suspended(dev-dev)) {
+   WARN_ONCE(true, We should never be here!\n);
+   return IRQ_NONE;
+   }

returning IRQ_NONE is not what you want to do in this case. You want to
setup a flag so that your runtime_resume() knows that there are pending
events to be handled and you handle those in runtime_resume time.

I don't want to handle this IRQ - we should never be here.
Will be changed to  IRQ_HANDLED.


But to be frank, I don't see how this can trigger since we're calling
pm_runtime_get_sync() from omap_i2c_xfer() which means by the time
pm_runtime_get_sync() returns, assuming no errors, i2c module should be
fully resumed and ready to go. Perhaps you have found a bug somewhere
else ?

May be it's better to revert this patch:
e3a36b207f76364c281aeecaf14c1b22a7247278
i2c: omap: remove unnecessary pm_runtime_suspended check

which doesn't cover case when transfer is *finished*.
Please, see https://patchwork.kernel.org/patch/2689211/ and
cover-latter.


Also, your 'We should never be here' message isn't helpfull at all.


@@ -905,6 +910,11 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
u16 stat;
int err = 0, count = 0;
  
+	if (pm_runtime_suspended(dev-dev)) {

+   WARN_ONCE(true, We should never be here!\n);
+   return IRQ_NONE;
+   }

because of IRQF_ONESHOT I can't see how this would *ever* be a valid
check.


Please, see https://patchwork.kernel.org/patch/2689211/ and
cover-latter.

Sorry, for delayed reply - I've had problems with my e-mail.

- grygorii
--
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/5] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle

[Grygorii Strashko]

On 06/07/2013 11:51 PM, Kevin Hilman wrote:

Grygorii Strashko grygorii.stras...@ti.com writes:


From: Kevin Hilman khil...@deeprootsystems.com

Currently, runtime PM is used to keep the device enabled only during
active transfers and for a configurable runtime PM autosuspend timout
after an xfer.

In addition to idling the device, driver's -runtime_suspend() method
currently disables device interrupts when idle.  However, on some SoCs
(notably OMAP4+), the I2C hardware may shared with other coprocessors.
This means that the MPU will still recieve interrupts if a coprocessor
is using the I2C device.  To avoid this, also disable interrupts at
the MPU INTC when idling the device in -runtime_suspend() (and
re-enable them in -runtime_resume().)  This part based on an original
patch from Shubhrajyoti Datta.  NOTE: for proper sharing the I2C with
a coprocessor, this driver still needs hwspinlock support added.

More over, this patch fixes the kernel boot failure which happens when
CONFIG_SENSORS_LM75=y:

[2.251220] WARNING: at drivers/bus/omap_l3_noc.c:113 
l3_interrupt_handler+0x140/0x184()
[2.257781] L3 custom error: MASTER:MPU TARGET:L4 PER2
[2.264373] Modules linked in:
[2.268463] CPU: 0 PID: 2 Comm: kthreadd Not tainted 
3.10.0-rc4-00034-g042dd60-dirty #64
[2.270385] [c001a80c] (unwind_backtrace+0x0/0xf0) from [c0017238] 
(show_stack+0x10/0x14)
[2.286102] [c0017238] (show_stack+0x10/0x14) from [c0040fd0] 
(warn_slowpath_common+0x4c/0x68)
[2.295593] [c0040fd0] (warn_slowpath_common+0x4c/0x68) from [c0041080] 
(warn_slowpath_fmt+0x30/0x40)
[2.299987] [c0041080] (warn_slowpath_fmt+0x30/0x40) from [c02c5ed0] 
(l3_interrupt_handler+0x140/0x184)
[2.315582] [c02c5ed0] (l3_interrupt_handler+0x140/0x184) from 
[c009ffb8] (handle_irq_event_percpu+0x58/0x258)
[2.323364] [c009ffb8] (handle_irq_event_percpu+0x58/0x258) from 
[c00a01f4] (handle_irq_event+0x3c/0x5c)
[2.327819] [c00a01f4] (handle_irq_event+0x3c/0x5c) from [c00a2f7c] 
(handle_fasteoi_irq+0xbc/0x164)
[2.337829] [c00a2f7c] (handle_fasteoi_irq+0xbc/0x164) from [c009f978] 
(generic_handle_irq+0x20/0x30)
[2.357513] [c009f978] (generic_handle_irq+0x20/0x30) from [c0014168] 
(handle_IRQ+0x4c/0xac)
[2.366821] [c0014168] (handle_IRQ+0x4c/0xac) from [c00085b4] 
(gic_handle_irq+0x28/0x5c)
[2.375762] [c00085b4] (gic_handle_irq+0x28/0x5c) from [c04f08a4] 
(__irq_svc+0x44/0x5c)
[2.379821] Exception stack(0xdb085ec0 to 0xdb085f08)
[2.389953] 5ec0: 0001 0001  db07ea00 4113 db2317a8 
db084000 02f1
[2.389953] 5ee0: db07ea00    c04fd990 db085f08 
c009170c c04f03e8
[2.405609] 5f00: 2113 
[2.408355] [c04f08a4] (__irq_svc+0x44/0x5c) from [c04f03e8] 
(_raw_spin_unlock_irqrestore+0x34/0x44)
[2.418304] [c04f03e8] (_raw_spin_unlock_irqrestore+0x34/0x44) from 
[c00403c0] (do_fork+0xa4/0x2d4)
[2.427795] [c00403c0] (do_fork+0xa4/0x2d4) from [c0040620] 
(kernel_thread+0x30/0x38)
[2.437805] [c0040620] (kernel_thread+0x30/0x38) from [c0063888] 
(kthreadd+0xd4/0x13c)
[2.448364] [c0063888] (kthreadd+0xd4/0x13c) from [c0013308] 
(ret_from_fork+0x14/0x2c)
[2.448364] ---[ end trace da8cf92c433d1616 ]---

The root casue of crash is race between omap_i2c_runtime_suspend()
  and omap_i2c_isr_thread()

If there's a race here, then it's not the same problem which is
described above, unless the CPU2 IRQ is happening because of a shared
user of I2C, in which case it doesn't need any additional explanation.

no shared users here

CPU1:   CPU2:
|-omap_i2c_xfer |
   |- pm_runtime_put_autosuspend()   |
  |-timeout  |-omap_i2c_isr()
   |-return IRQ_WAKE_THREAD;
  |-omap_i2c_runtime_suspend()   |
 |-omap_i2c_isr_thread()
   |- oops is here - I2C module is in 
idle state

If this is happening, I don't think it's related to $SUBJECT patch (but
is probably hidden by it.)

I can remove fix spurious IRQs:  from $SUBJECT. What do you think?


Instead, what probably needs to happen in this is case is that
omap_i2c_isr() should be doing a mark last busy to reset the
autosuspend timeout.  And, that should be done in a separate patch.
Yes - from one side. From other side, this patch prevents such situation 
to happen.
disable_irq(_dev-irq); - waits for any pending IRQ handlers for this 
interrupt

to complete before returning.

If we are in .runtime_idle() callback - it means I2C transaction has 
been finished
(and It doesn't matter successfully or not) and we don't want to receive 
IRQs any more.


As I mentioned in cover-latter this happens because PM runtime 
auto-suspend isn't

working properly during the boot:

[   23.190002] omap_i2c 4835.i2c:  i2c_add_numbered_adapter
[   23.204681] omap_i2c 4835.i2c: bus 3 rev0.11 at 400 kHz

Re: [PATCH] i2c: Let users disable Probe an I2C bus for certain devices

[Grygorii Strashko]

Hi Wolfram,
On 06/07/2013 12:06 PM, Wolfram Sang wrote:

3) Thinking about Mainline: To reach the same target - no I2C
detection - and taking
into account above assumption No changes in default behavior
the following will need to be done:
- change i2c-omap/i2c-gpio DT bindings and add parameter which will
allow to change
   .class value for adapter. Not sure, it's possible because this parameter
   will be Linux and not HW specific (smth. like i2c_disable_detection)
- update drivers i2c-omap/i2c-gpio to use i2c_disable_detection
- update OMAP4/5 DTS files

So, It seemed a good solution for me to add 6 lines of code in i2c-core.c
instead of doing all that stuff.

Well... I understand the default behaviour issue, yet I still think
that setting class to 0 is the right thing to do. OMAP is an embedded
SoC which always had i2c_board_info or devictree which are the preferred
ways of instantiating. Given that, I would accept a patch setting it to
0. The more user friendly way might be to introduce a new class which
makes users aware of the issue. Proof of concept follows, only compile
tested.


That sounds good to me - I can prepare patch for i2c-omap.c.
But, there is still an open question regarding *i2c-gpio.c* which,
actually, a source of biggest part of delay.

Potentially, it can be tunned using timeout value. But, again, this
is additional work/patches to workaround unneeded system feature:
- timeout should not break interaction with I2C devices on i2c-gpio
bus and, at same time, speed up boot;
- DTS/board files need to be updated.

May be, just may be), we can continue with 
CONFIG_I2C_DISABLE_DEVICE_DETECTION,

and print deprecation warning for each registered adapter
if this config option is defined.

- grygorii
--
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 0/5] v3.10-rc4: fix OMAP4 boot failure if CONFIG_SENSORS_LM75=y

[Grygorii Strashko]

 spurious IRQs: disable/enable IRQ at INTC when idle
- fixes the boot crash, but I2C bus 2 still stuck in Bus Busy state.

[   23.487915] omap_i2c 4806.i2c: timeout waiting for bus ready

In addition, I've created two patches to fix wrong IRQ handling and to prevent 
such kind of crashes in the future:
  i2c: omap: add runtime check in isr to be sure that i2c is enabled
  i2c: omap: handle all irqs befor unblocking omap_i2c_xfer_msg()


2) Q: Why I2C is suspended so fast during the boot?
The issue 1 should be reproducible very rare, in general. I traced it to:
omap_i2c_xfer
|- pm_runtime_put_autosuspend()
   |- __pm_runtime_suspend
  |- rpm_suspend()
 |- mod_timer(dev-power.suspend_timer, expires);

The mod_timer() should schedule next timer event after ~100-200 jiffes,
which is equal to  1 sec (as requested).
 But, at boot time, the timer expires after ~50 ms instead!!

[   23.190002] omap_i2c 4835.i2c:  i2c_add_numbered_adapter
[   23.204681] omap_i2c 4835.i2c: bus 3 rev0.11 at 400 kHz
[   23.211669] omap_i2c 4835.i2c: == rpm_suspend elapsed 0 last_busy 
4294939616
[   23.219879] omap_i2c 4835.i2c: == rpm_suspend expires 4294939716 
last_busy 4294939616 autosuspend_delay 1000
[   23.219879] omap_i2c 4835.i2c: == rpm_suspend expires 4294939700
[   23.237274] omap_i2c 4835.i2c: == rpm_suspend mod_timer expires 
4294939700
--- the timer scheduled to 84 jiffes
[   23.246185] omap_i2c 4835.i2c:  pm_runtime_put_autosuspend
[   23.253448] omap-dma-engine 4a056000.dma-controller: allocating channel for 
62
[   23.261138] omap-dma-engine 4a056000.dma-controller: allocating channel for 
61
[   23.269500] omap_i2c 4807.i2c:  omap_i2c_runtime_resume
[   23.275817] omap_i2c 4835.i2c:  omap_i2c_runtime_suspend
--- and expired after ~40 ms

Unfortunatelly, Im not able to find the root cause of such timers behavior - 
have
not to much knowledge about system clocks code.


3) when lm75_detect() requests to read from  bus 2 addr 0x48 (tmp105 sensor)
registers 0x04 and above (registers don't exist) the bus stuck in Bus busy 
condition
 until next I2C re-initialization (PM runtime suspend/resume). 

[3.300933] omap_i2c 4806.i2c: timeout waiting for bus ready
[4.369262] omap_i2c 4806.i2c: timeout waiting for bus ready
[5.381530] omap_i2c 4806.i2c: timeout waiting for bus ready

I've found that the NACK condition not handled properly:
- when NACK condition is detected the master should generate STT or STP 
condition
- now, the I2C driver generates STP only if NACK has been received during the
  last message transmitting/recieving. As result, the Bus Busy state may occur
  if NACK has been received during any other messages transmission
  /reception, because STP isn't generated.

Patch i2c: omap: query STP always when NACK is received: fixes this issue.


This patch series based on top of:
http://patchwork.ozlabs.org/patch/248188/

Grygorii Strashko (4):
  i2c: omap: add runtime check in isr to be sure that i2c is enabled
  i2c: omap: handle all irqs befor unblocking omap_i2c_xfer_msg()
  i2c: omap: query STP always when NACK is received
  i2c: omap: remove omap_i2c_isr() hw irq handler

Kevin Hilman (1):
  i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle

 drivers/i2c/busses/i2c-omap.c |   52 +
 1 file changed, 16 insertions(+), 36 deletions(-)


CC: Kevin Hilman khil...@linaro.org
CC: Tomi Valkeinen tomi.valkei...@ti.com
CC: Felipe Balbi ba...@ti.com
-- 
1.7.9.5

--
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 5/5] i2c: omap: remove omap_i2c_isr() hw irq handler

[Grygorii Strashko]

The omap_i2c_isr() does the irq check and schedules threaded handler if any of
enabled IRQs is active, but currently the I2C IRQs are enabled just once,
when I2C IP is enabling (transfer started) and they aren't changed after that.
More over, now the I2C INTC IRQ is disabled when I2C IP is idled.
Thus, I2C IRQs will start coming only when we want that, and there is
no sense to have omap_i2c_isr() anymore:
- omap_i2c_isr_thread() does all needed checks
- synchronization is provided IRQ Core.

So, get rid of omap_i2c_isr(), custom locking and
struct omap_i2c_dev-lock variable.

CC: Kevin Hilman khil...@linaro.org
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---

This is clean-up patch.

 drivers/i2c/busses/i2c-omap.c |   33 +
 1 file changed, 1 insertion(+), 32 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index b3daf3f..749f390 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -183,7 +183,6 @@ enum {
 #define OMAP_I2C_IP_V2_INTERRUPTS_MASK 0x6FFF
 
 struct omap_i2c_dev {
-   spinlock_t  lock;   /* IRQ synchronization */
struct device   *dev;
void __iomem*base;  /* virtual */
int irq;
@@ -876,35 +875,10 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev 
*dev, u8 num_bytes,
 }
 
 static irqreturn_t
-omap_i2c_isr(int irq, void *dev_id)
-{
-   struct omap_i2c_dev *dev = dev_id;
-   irqreturn_t ret = IRQ_HANDLED;
-   u16 mask;
-   u16 stat;
-
-   spin_lock(dev-lock);
-   if (pm_runtime_suspended(dev-dev)) {
-   WARN_ONCE(true, We should never be here!\n);
-   return IRQ_NONE;
-   }
 
-   mask = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
-   stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
-
-   if (stat  mask)
-   ret = IRQ_WAKE_THREAD;
-
-   spin_unlock(dev-lock);
-
-   return ret;
-}
-
-static irqreturn_t
 omap_i2c_isr_thread(int this_irq, void *dev_id)
 {
struct omap_i2c_dev *dev = dev_id;
-   unsigned long flags;
u16 bits;
u16 stat;
int err = 0, count = 0;
@@ -914,7 +888,6 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
return IRQ_NONE;
}
 
-   spin_lock_irqsave(dev-lock, flags);
do {
bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
@@ -1035,8 +1008,6 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
omap_i2c_complete_cmd(dev, err);
 
 out:
-   spin_unlock_irqrestore(dev-lock, flags);
-
return IRQ_HANDLED;
 }
 
@@ -1146,8 +1117,6 @@ omap_i2c_probe(struct platform_device *pdev)
dev-dev = pdev-dev;
dev-irq = irq;
 
-   spin_lock_init(dev-lock);
-
platform_set_drvdata(pdev, dev);
init_completion(dev-cmd_complete);
 
@@ -1229,7 +1198,7 @@ omap_i2c_probe(struct platform_device *pdev)
IRQF_NO_SUSPEND, pdev-name, dev);
else
r = devm_request_threaded_irq(pdev-dev, dev-irq,
-   omap_i2c_isr, omap_i2c_isr_thread,
+   NULL, omap_i2c_isr_thread,
IRQF_NO_SUSPEND | IRQF_ONESHOT,
pdev-name, dev);
 
-- 
1.7.9.5

--
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 4/5] i2c: omap: query STP always when NACK is received

[Grygorii Strashko]

According to I2C specification the NACK should be handled as folowing:
When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then gene rate either a STOP condition to
abort the transfer, or a repeated START condition to start a new transfer.
[http://www.nxp.com/documents/user_manual/UM10204.pdf]

The same is recomened by TI I2C wiki:
 http://processors.wiki.ti.com/index.php/I2C_Tips

Currently, the OMAP I2C driver interrupts I2C trunsfer in case of NACK, but
It queries Stop condition OMAP_I2C_CON_REG.STP=1 only if NACK has been received
during the last message transmitting/recieving.
This may lead to stuck Bus in Bus Busy until I2C IP reset (idle/enable).

Hence, fix it by querying Stop condition (STP) always when NACK is received.

CC: Kevin Hilman khil...@linaro.org
CC: Felipe Balbi ba...@ti.com
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-omap.c |9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 46fb8a5..b3daf3f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -618,11 +618,10 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
if (dev-cmd_err  OMAP_I2C_STAT_NACK) {
if (msg-flags  I2C_M_IGNORE_NAK)
return 0;
-   if (stop) {
-   w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
-   w |= OMAP_I2C_CON_STP;
-   omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
-   }
+
+   w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
+   w |= OMAP_I2C_CON_STP;
+   omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
return -EREMOTEIO;
}
return -EIO;
-- 
1.7.9.5

--
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 3/5] i2c: omap: handle all irqs befor unblocking omap_i2c_xfer_msg()

[Grygorii Strashko]

ARDY|NACK and ARDY|AL are set together in OMAP_I2C_STAT_REG, which will be
processed incorrectly now:
iterration 1:
- I2C irq triggered (ARDY|NACK)
- omap_i2c_isr_thread() will ask NACK, fill dev-cmd_err = OMAP_I2C_STAT_NACK
  and trigger cmd_complete completion.
- omap_i2c_xfer_msg() will be unblocked, hande NACK and finish its execution.
- omap_i2c_xfer() will finish

iteration 2:
- I2C irq triggered (ARDY)
- omap_i2c_isr_thread() will ask ARDY, trigger completion
  At this point dev-cmd_err == OMAP_I2C_STAT_NACK, cmd_complete is not
  initialized and omap_i2c_xfer() may be completed at all.

So, I2C driver is not ready for iteration 2.
Hence, fix it by asking NACK, AL and ARDY all togather in omap_i2c_isr_thread()
before unblocking omap_i2c_xfer_msg() execution.

This is the old I2C interrupt handler behavior which was changed by commit:
1d7afc95946487945cc7f5019b41255b72224b70 i2c: omap: ack IRQ in parts.

CC: Kevin Hilman khil...@linaro.org
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-omap.c |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 2dac598..46fb8a5 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -938,17 +938,15 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
break;
}
 
+   omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL);
+
if (stat  OMAP_I2C_STAT_NACK) {
err |= OMAP_I2C_STAT_NACK;
-   omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
-   break;
}
 
if (stat  OMAP_I2C_STAT_AL) {
dev_err(dev-dev, Arbitration lost\n);
err |= OMAP_I2C_STAT_AL;
-   omap_i2c_ack_stat(dev, OMAP_I2C_STAT_AL);
-   break;
}
 
/*
-- 
1.7.9.5

--
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 0/5] v3.10-rc4: fix OMAP4 boot failure if CONFIG_SENSORS_LM75=y

[Grygorii Strashko]

 spurious IRQs: disable/enable IRQ at INTC when idle
- fixes the boot crash, but I2C bus 2 still stuck in Bus Busy state.

[   23.487915] omap_i2c 4806.i2c: timeout waiting for bus ready

In addition, I've created two patches to fix wrong IRQ handling and to prevent 
such kind of crashes in the future:
  i2c: omap: add runtime check in isr to be sure that i2c is enabled
  i2c: omap: handle all irqs befor unblocking omap_i2c_xfer_msg()


2) Q: Why I2C is suspended so fast during the boot?
The issue 1 should be reproducible very rare, in general. I traced it to:
omap_i2c_xfer
|- pm_runtime_put_autosuspend()
   |- __pm_runtime_suspend
  |- rpm_suspend()
 |- mod_timer(dev-power.suspend_timer, expires);

The mod_timer() should schedule next timer event after ~100-200 jiffes,
which is equal to  1 sec (as requested).
 But, at boot time, the timer expires after ~50 ms instead!!

[   23.190002] omap_i2c 4835.i2c:  i2c_add_numbered_adapter
[   23.204681] omap_i2c 4835.i2c: bus 3 rev0.11 at 400 kHz
[   23.211669] omap_i2c 4835.i2c: == rpm_suspend elapsed 0 last_busy 
4294939616
[   23.219879] omap_i2c 4835.i2c: == rpm_suspend expires 4294939716 
last_busy 4294939616 autosuspend_delay 1000
[   23.219879] omap_i2c 4835.i2c: == rpm_suspend expires 4294939700
[   23.237274] omap_i2c 4835.i2c: == rpm_suspend mod_timer expires 
4294939700
--- the timer scheduled to 84 jiffes
[   23.246185] omap_i2c 4835.i2c:  pm_runtime_put_autosuspend
[   23.253448] omap-dma-engine 4a056000.dma-controller: allocating channel for 
62
[   23.261138] omap-dma-engine 4a056000.dma-controller: allocating channel for 
61
[   23.269500] omap_i2c 4807.i2c:  omap_i2c_runtime_resume
[   23.275817] omap_i2c 4835.i2c:  omap_i2c_runtime_suspend
--- and expired after ~40 ms

Unfortunatelly, Im not able to find the root cause of such timers behavior - 
have
not to much knowledge about system clocks code.


3) when lm75_detect() requests to read from  bus 2 addr 0x48 (tmp105 sensor)
registers 0x04 and above (registers don't exist) the bus stuck in Bus busy 
condition
 until next I2C re-initialization (PM runtime suspend/resume). 

[3.300933] omap_i2c 4806.i2c: timeout waiting for bus ready
[4.369262] omap_i2c 4806.i2c: timeout waiting for bus ready
[5.381530] omap_i2c 4806.i2c: timeout waiting for bus ready

I've found that the NACK condition not handled properly:
- when NACK condition is detected the master should generate STT or STP 
condition
- now, the I2C driver generates STP only if NACK has been received during the
  last message transmitting/recieving. As result, the Bus Busy state may occur
  if NACK has been received during any other messages transmission
  /reception, because STP isn't generated.

Patch i2c: omap: query STP always when NACK is received: fixes this issue.


This patch series based on top of:
http://patchwork.ozlabs.org/patch/248188/

Grygorii Strashko (4):
  i2c: omap: add runtime check in isr to be sure that i2c is enabled
  i2c: omap: handle all irqs befor unblocking omap_i2c_xfer_msg()
  i2c: omap: query STP always when NACK is received
  i2c: omap: remove omap_i2c_isr() hw irq handler

Kevin Hilman (1):
  i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle

 drivers/i2c/busses/i2c-omap.c |   52 +
 1 file changed, 16 insertions(+), 36 deletions(-)


CC: Kevin Hilman khil...@linaro.org
CC: Tomi Valkeinen tomi.valkei...@ti.com
CC: Felipe Balbi ba...@ti.com
-- 
1.7.9.5

--
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/5] i2c: omap: add runtime check in isr to be sure that i2c is enabled

[Grygorii Strashko]

Add runtime check at the beginning of omap_i2c_isr/omap_i2c_isr_thread
to be sure that i2c is enabled, before performing IRQ handling and accessing
I2C IP registers:
if (pm_runtime_suspended(dev-dev)) {
WARN_ONCE(true, We should never be here!\n);
return IRQ_NONE;
}

Produce warning in case if ISR called when i2c is disabled

CC: Kevin Hilman khil...@linaro.org
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-omap.c |   10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 97844ff..2dac598 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -885,6 +885,11 @@ omap_i2c_isr(int irq, void *dev_id)
u16 stat;
 
spin_lock(dev-lock);
+   if (pm_runtime_suspended(dev-dev)) {
+   WARN_ONCE(true, We should never be here!\n);
+   return IRQ_NONE;
+   }
+
mask = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 
@@ -905,6 +910,11 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
u16 stat;
int err = 0, count = 0;
 
+   if (pm_runtime_suspended(dev-dev)) {
+   WARN_ONCE(true, We should never be here!\n);
+   return IRQ_NONE;
+   }
+
spin_lock_irqsave(dev-lock, flags);
do {
bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
-- 
1.7.9.5

--
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/5] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle

[Grygorii Strashko]

From: Kevin Hilman khil...@deeprootsystems.com

Currently, runtime PM is used to keep the device enabled only during
active transfers and for a configurable runtime PM autosuspend timout
after an xfer.

In addition to idling the device, driver's -runtime_suspend() method
currently disables device interrupts when idle.  However, on some SoCs
(notably OMAP4+), the I2C hardware may shared with other coprocessors.
This means that the MPU will still recieve interrupts if a coprocessor
is using the I2C device.  To avoid this, also disable interrupts at
the MPU INTC when idling the device in -runtime_suspend() (and
re-enable them in -runtime_resume().)  This part based on an original
patch from Shubhrajyoti Datta.  NOTE: for proper sharing the I2C with
a coprocessor, this driver still needs hwspinlock support added.

More over, this patch fixes the kernel boot failure which happens when
CONFIG_SENSORS_LM75=y:

[2.251220] WARNING: at drivers/bus/omap_l3_noc.c:113 
l3_interrupt_handler+0x140/0x184()
[2.257781] L3 custom error: MASTER:MPU TARGET:L4 PER2
[2.264373] Modules linked in:
[2.268463] CPU: 0 PID: 2 Comm: kthreadd Not tainted 
3.10.0-rc4-00034-g042dd60-dirty #64
[2.270385] [c001a80c] (unwind_backtrace+0x0/0xf0) from [c0017238] 
(show_stack+0x10/0x14)
[2.286102] [c0017238] (show_stack+0x10/0x14) from [c0040fd0] 
(warn_slowpath_common+0x4c/0x68)
[2.295593] [c0040fd0] (warn_slowpath_common+0x4c/0x68) from [c0041080] 
(warn_slowpath_fmt+0x30/0x40)
[2.299987] [c0041080] (warn_slowpath_fmt+0x30/0x40) from [c02c5ed0] 
(l3_interrupt_handler+0x140/0x184)
[2.315582] [c02c5ed0] (l3_interrupt_handler+0x140/0x184) from 
[c009ffb8] (handle_irq_event_percpu+0x58/0x258)
[2.323364] [c009ffb8] (handle_irq_event_percpu+0x58/0x258) from 
[c00a01f4] (handle_irq_event+0x3c/0x5c)
[2.327819] [c00a01f4] (handle_irq_event+0x3c/0x5c) from [c00a2f7c] 
(handle_fasteoi_irq+0xbc/0x164)
[2.337829] [c00a2f7c] (handle_fasteoi_irq+0xbc/0x164) from [c009f978] 
(generic_handle_irq+0x20/0x30)
[2.357513] [c009f978] (generic_handle_irq+0x20/0x30) from [c0014168] 
(handle_IRQ+0x4c/0xac)
[2.366821] [c0014168] (handle_IRQ+0x4c/0xac) from [c00085b4] 
(gic_handle_irq+0x28/0x5c)
[2.375762] [c00085b4] (gic_handle_irq+0x28/0x5c) from [c04f08a4] 
(__irq_svc+0x44/0x5c)
[2.379821] Exception stack(0xdb085ec0 to 0xdb085f08)
[2.389953] 5ec0: 0001 0001  db07ea00 4113 db2317a8 
db084000 02f1
[2.389953] 5ee0: db07ea00    c04fd990 db085f08 
c009170c c04f03e8
[2.405609] 5f00: 2113 
[2.408355] [c04f08a4] (__irq_svc+0x44/0x5c) from [c04f03e8] 
(_raw_spin_unlock_irqrestore+0x34/0x44)
[2.418304] [c04f03e8] (_raw_spin_unlock_irqrestore+0x34/0x44) from 
[c00403c0] (do_fork+0xa4/0x2d4)
[2.427795] [c00403c0] (do_fork+0xa4/0x2d4) from [c0040620] 
(kernel_thread+0x30/0x38)
[2.437805] [c0040620] (kernel_thread+0x30/0x38) from [c0063888] 
(kthreadd+0xd4/0x13c)
[2.448364] [c0063888] (kthreadd+0xd4/0x13c) from [c0013308] 
(ret_from_fork+0x14/0x2c)
[2.448364] ---[ end trace da8cf92c433d1616 ]---

The root casue of crash is race between omap_i2c_runtime_suspend()
 and omap_i2c_isr_thread()

CPU1:   CPU2:
|-omap_i2c_xfer |
  |- pm_runtime_put_autosuspend()   |
 |-timeout  |-omap_i2c_isr()
  |-return IRQ_WAKE_THREAD;
 |-omap_i2c_runtime_suspend()   |
|-omap_i2c_isr_thread()
  |- oops is here - I2C module is in 
idle state


Cc: Nishanth Menon n...@ti.com
Signed-off-by: Kevin Hilman khil...@linaro.org
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-omap.c |4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 64c26f9..97844ff 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1290,6 +1290,8 @@ static int omap_i2c_runtime_suspend(struct device *dev)
struct platform_device *pdev = to_platform_device(dev);
struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
 
+   disable_irq(_dev-irq);
+
_dev-iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG);
 
if (_dev-scheme == OMAP_I2C_SCHEME_0)
@@ -1320,6 +1322,8 @@ static int omap_i2c_runtime_resume(struct device *dev)
 
__omap_i2c_init(_dev);
 
+   enable_irq(_dev-irq);
+
return 0;
 }
 #endif /* CONFIG_PM_RUNTIME */
-- 
1.7.9.5

--
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: omap: convert to module_platform_driver()

[Grygorii Strashko]

On 06/05/2013 04:50 PM, Wolfram Sang wrote:

The similar patch already exists:
https://patchwork.kernel.org/patch/2448251/ - [v2,1/2] RTC: rtc-twl:
Fix rtc_reg_map initialization
from Peter Ujfalusi

So, I think it is best if you resend this patch after all the fixes it
needs are applied or you resend the series with all patches it depends
on. Which should then probably go via arm-soc. Or?


It can wait - I'll track and resend after twl-rtc patches.

-grygorii

--
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: Let users disable Probe an I2C bus for certain devices

[Grygorii Strashko]

Hi
On 06/04/2013 08:49 PM, Wolfram Sang wrote:

On Tue, Jun 04, 2013 at 08:33:42PM +0300, Grygorii Strashko wrote:


Currently the I2C devices instantiation Method 3 Probe an I2C bus for
certain devices (see Documentation/i2c/instantiating-devices) is always
enabled for all platforms (boards) and can't be disabled.

Not true. Set .class = 0 for your adapter. I always ask authors of new
drivers if they really need to set .class to something.


Agree, sorry, my statement is wrong. it would be right to say
..can't be disabled without kernel code modification.

Few notes here:
1) boot delay issue isn't related to new drivers. There are
hwmon/lm75.c, i2c/busses/i2c-gpio.c and i2c/busses/i2c-omap.c

2) Initially, I've fighted with it on TI K3.4 product kernel where
DT isn't supported yet. And first thing, which I've tried to do is to 
correct

.class parameter for adapter, but with assumption:
Default behavior shouldn't be changed as I2C detection might be used by 
some

of OMAP boards.

I've started from i2c-omap.c and OMAP4/5 (my target). As result, I was 
need to make
changes in *7* files to set and pass platform parameter to i2c-omap.c 
driver which

will allow to change .class to 0 on demand.

At this point, I've realized that i still need to deal with i2c-gpio.c - 
which

is generic driver.

3) Thinking about Mainline: To reach the same target - no I2C detection 
- and taking

into account above assumption No changes in default behavior
the following will need to be done:
- change i2c-omap/i2c-gpio DT bindings and add parameter which will 
allow to change

  .class value for adapter. Not sure, it's possible because this parameter
  will be Linux and not HW specific (smth. like i2c_disable_detection)
- update drivers i2c-omap/i2c-gpio to use i2c_disable_detection
- update OMAP4/5 DTS files

So, It seemed a good solution for me to add 6 lines of code in i2c-core.c
instead of doing all that stuff.

Thanks/sorry for your time.

- grygorii


--
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: omap: convert to module_platform_driver()

[Grygorii Strashko]

Hi Kevin,
On 06/03/2013 11:59 PM, Kevin Hilman wrote:

Grygorii Strashko grygorii.stras...@ti.com writes:


The OMAP I2C driver has a relation to pinctrl-single driver. As result,
its probe will be deferred during system boot until late init time,
because the pinctrl-single is initizalized as moudle/device init time.
This, in turn, will delay initialization of all I2C devices (like mfd,
I2C regulators and etc.) and cause boot delay (more over, it can broken
initialization of drivers which are not ready to use deferred probe
mechanism yet, for example DSS).

There are no sense to keep OMAP I2C initialization on subsys init layer
any more, hence shift it to module/device layer where the i2c --
pinctrl-single dependency is resolved in drivers/Makefile now.

Cc: Wolfram Sang w...@the-dreams.de
Cc: Ben Dooks (embedded platforms) ben-li...@fluff.org
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: linux-o...@vger.kernel.org
Cc: linux-i2c@vger.kernel.org
Cc: linux-ker...@vger.kernel.org

Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com

Testing this patch with PATCH 1/2, the twl_rtc driver fails to correctly
initialize on OMAP3:

  twl_rtc rtc.22: hctosys: invalid date/time

instead of the expected result:

   twl_rtc rtc.22: setting system clock to 2000-01-01 00:00:00 UTC 
(946684800)

so something is still not right for the init sequence.

Kevin
I think, the root cause of the problem isn't this patch - it's just yet 
another side effect
of using deferred probes :). I've taken a look on twl-rtc code and found 
possible error place

static int __init twl_rtc_init(void)
{
if (twl_class_is_4030()) -- here,
rtc_reg_map = (u8 *) twl4030_rtc_reg_map;
else
rtc_reg_map = (u8 *) twl6030_rtc_reg_map;

return platform_driver_register(twl4030rtc_driver);
}
In drivers/Makefile:
obj-$(CONFIG_RTC_LIB)+= rtc/  -- RTC is placed before I2C
obj-y+= i2c/ media/ - and only here I2C bus 
instantiates TWL-core device

and configures twl_priv-twl_id

Thats why it's working on my OMAP4/twl6030 board. Could you check if 
below fix will work on OMAP3:

diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c
index 8751a52..aaa5015 100644
--- a/drivers/rtc/rtc-twl.c
+++ b/drivers/rtc/rtc-twl.c
@@ -469,6 +469,11 @@ static int twl_rtc_probe(struct platform_device *pdev)
if (irq = 0)
goto out1;

+   if (twl_class_is_4030())
+   rtc_reg_map = (u8 *) twl4030_rtc_reg_map;
+   else
+   rtc_reg_map = (u8 *) twl6030_rtc_reg_map;
+
ret = twl_rtc_read_u8(rd_reg, REG_RTC_STATUS_REG);
if (ret  0)
goto out1;
@@ -610,11 +615,6 @@ static struct platform_driver twl4030rtc_driver = {

 static int __init twl_rtc_init(void)
 {
-   if (twl_class_is_4030())
-   rtc_reg_map = (u8 *) twl4030_rtc_reg_map;
-   else
-   rtc_reg_map = (u8 *) twl6030_rtc_reg_map;
-
return platform_driver_register(twl4030rtc_driver);
 }
 module_init(twl_rtc_init);

Unfortunately, I have no OMAP3 HW and can't check it.

Regards,
-grygorii


--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] i2c: omap: enhance pinctrl support

[Grygorii Strashko]

Hi Kevin, Gururaja

On 05/31/2013 08:34 PM, Kevin Hilman wrote:

Hebbar Gururaja gururaja.heb...@ti.com writes:


Amend the I2C omap pin controller to optionally take a pin control
handle and set the state of the pins to:

- default on boot, resume and before performing an i2c transfer
- idle after initial default, after resume default, and after each
i2c xfer
- sleep on suspend()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

Note:
A .suspend  .resume callback is added which simply puts the pins to sleep
state upon suspend  are moved to default  idle state upon resume.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

(Changes based on i2c-nomadik.c)

Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Wolfram Sang w...@the-dreams.de
Cc: linux-o...@vger.kernel.org
Cc: linux-i2c@vger.kernel.org

[...]


@@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg 
msgs[], int num)
  
  out:

pm_runtime_mark_last_busy(dev-dev);
+
pm_runtime_put_autosuspend(dev-dev);
+   /* Optionally let pins go into idle state */
+   if (!IS_ERR(dev-pins_idle))
+   if (pinctrl_select_state(dev-pinctrl, dev-pins_idle))
+   dev_err(dev-dev, could not set pins to idle state\n);

This is wrong.  You're changing the muxing before the device actually
goes idle.  Anything you want to happen when the device actually idles
for real has to be in runtime PM callbacks.

Looking below, I see it's already in the callbacks, so why is it here also?


I have two questions regarding this patch  the whole series:

1) PM runtime suspend/resume
Current sequence:
- resume
  |- omap_hwmod_enable()
 |- _enable()
 |- omap_hwmod_mux(oh-mux, _HWMOD_STATE_ENABLED)
|- enable module (syscclocks)
  |- omap_i2c_runtime_resume()
- suspend
  |- omap_i2c_runtime_suspend()
  |- omap_hwmod_idle()
 |- _idle()
 |- disbale module (syscclocks)
 |- omap_hwmod_mux(oh-mux, _HWMOD_STATE_IDLE);

The new order will change this sequence - *Is it safe?*:
- resume
  |- omap_hwmod_enable()
 |- _enable()
|- enable module (syscclocks)  Is it valid to enable 
module before PINs?

  |- omap_i2c_runtime_resume()
 |- PINs state set to DEFAULT
- suspend
  |- omap_i2c_runtime_suspend()
 |- PINs state set to IDLE
  |- omap_hwmod_idle()
 |- _idle()
 |- disbale module (syscclocks)  Is it valid to disable 
module after PINs?


2) System suspend (OFF-mode) with assumption that noirq stage will be 
used for PINs cfg

Current implementation: handled in the same way as PM runtime


The new implementation:   -- total mess is here((:
- suspend_noirq - I2C device can be still active because of PM auto-suspend
  |-_od_suspend_noirq
 |- omap_i2c_suspend_noirq
|- PINs state set to SLEEP
  |- pm_generic_runtime_suspend
 |- omap_i2c_runtime_suspend()
|- PINs state set to IDLE  --- *oops* PINs state is IDLE and 
not SLEEP

  |- omap_device_idle()
 |- omap_hwmod_idle()
|- _idle()
   |- disbale module (syscclocks)

- resume_noirq - I2C was active before suspend
  |-_od_resume_noirq
 |- omap_hwmod_enable()
|- _enable()
   |- enable module (syscclocks)
 |- pm_generic_runtime_resume
|- omap_i2c_runtime_resume()
   |- PINs state set to DEFAULT  --- 
 |- omap_i2c_resume_noirq
|- PINs state set to DEFAULT
|- PINs state set to IDLE--- *big oops* we have active 
module and its

  PINs state is IDLE

All mentioned above, applicable for most of all patches in series.
And It seems, that this functionality can't be implemented in such way (
- OMAP device FW and Driver core might handle PINs states changing and
  drivers (DT) should provide set of available states only.

Am I wrong?

Regards,
-grygorii




[...]


@@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev)
omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
}
  
+	if (!IS_ERR(_dev-pins_idle))

+   if (pinctrl_select_state(_dev-pinctrl, _dev-pins_idle))
+   dev_err(dev, could not set pins to idle state\n);
+
return 0;
  }
  

Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line 

Re: [PATCH 11/11] i2c: omap: enhance pinctrl support

[Grygorii Strashko]

On 05/31/2013 01:13 PM, Hebbar Gururaja wrote:

Amend the I2C omap pin controller to optionally take a pin control
handle and set the state of the pins to:

- default on boot, resume and before performing an i2c transfer
- idle after initial default, after resume default, and after each
i2c xfer
- sleep on suspend()

By optionally putting the pins into sleep state in the suspend callback
we can accomplish two things.
- One is to minimize current leakage from pins and thus save power,
- second, we can prevent the IP from driving pins output in an
uncontrolled manner, which may happen if the power domain drops the
domain regulator.

Note:
A .suspend  .resume callback is added which simply puts the pins to sleep
state upon suspend  are moved to default  idle state upon resume.

If any of the above pin states are missing in dt, a warning message
about the missing state is displayed.
If certain pin-states are not available, to remove this warning message
pass respective state name with null phandler.

(Changes based on i2c-nomadik.c)

Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Wolfram Sang w...@the-dreams.de
Cc: linux-o...@vger.kernel.org
Cc: linux-i2c@vger.kernel.org
---
:100644 100644 e02f9e3... 588ba28... M  drivers/i2c/busses/i2c-omap.c
  drivers/i2c/busses/i2c-omap.c |  112 ++---
  1 file changed, 105 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e02f9e3..588ba28 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -214,7 +214,11 @@ struct omap_i2c_dev {
u16 westate;
u16 errata;
  
-	struct pinctrl		*pins;

+   /* Three pin states - default, idle  sleep */
+   struct pinctrl  *pinctrl;
+   struct pinctrl_state*pins_default;
+   struct pinctrl_state*pins_idle;
+   struct pinctrl_state*pins_sleep;
  };
  
  static const u8 reg_map_ip_v1[] = {

@@ -641,6 +645,11 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg 
msgs[], int num)
if (IS_ERR_VALUE(r))
goto out;
  
The current HWMOD framework configures PINs to enable state before 
enabling the device and
switch PINs to idle state after disabling the device. Why here its done 
in different order?

+   /* Optionaly enable pins to be muxed in and configured */
+   if (!IS_ERR(dev-pins_default))
+   if (pinctrl_select_state(dev-pinctrl, dev-pins_default))
+   dev_err(dev-dev, could not set default pins\n);
+
r = omap_i2c_wait_for_bb(dev);
if (r  0)
goto out;
@@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg 
msgs[], int num)
  
  out:

pm_runtime_mark_last_busy(dev-dev);
+
pm_runtime_put_autosuspend(dev-dev);
+   /* Optionally let pins go into idle state */
+   if (!IS_ERR(dev-pins_idle))
+   if (pinctrl_select_state(dev-pinctrl, dev-pins_idle))
+   dev_err(dev-dev, could not set pins to idle state\n);
+
return r;
  }
  
@@ -1123,14 +1138,47 @@ omap_i2c_probe(struct platform_device *pdev)

dev-set_mpu_wkup_lat = pdata-set_mpu_wkup_lat;
}
  
-	dev-pins = devm_pinctrl_get_select_default(pdev-dev);

-   if (IS_ERR(dev-pins)) {
-   if (PTR_ERR(dev-pins) == -EPROBE_DEFER)
+   dev-pinctrl = devm_pinctrl_get(pdev-dev);

May be struct device -pins-p can be used instead of dev-pinctrl?

+   if (!IS_ERR(dev-pinctrl)) {
+   dev-pins_default = pinctrl_lookup_state(dev-pinctrl,
+PINCTRL_STATE_DEFAULT);
+   if (IS_ERR(dev-pins_default))
+   dev_dbg(pdev-dev, could not get default pinstate\n);
+   else
+   if (pinctrl_select_state(dev-pinctrl,
+dev-pins_default))
+   dev_err(pdev-dev,
+   could not set default pinstate\n);

Don't need to set Default pin state
Default pins state is applied by DD framework automatically before 
device probing

and stored in struct device -pins-default_state

+
+   dev-pins_idle = pinctrl_lookup_state(dev-pinctrl,
+ PINCTRL_STATE_IDLE);
+   if (IS_ERR(dev-pins_idle))
+   dev_dbg(pdev-dev, could not get idle pinstate\n);
+   else
+   /* If possible, let's idle until the first transfer */
+   if (pinctrl_select_state(dev-pinctrl, dev-pins_idle))
+   dev_err(pdev-dev,
+   could not set idle pinstate\n);
+
+   dev-pins_sleep = 

[PATCH 1/2] i2c: omap: convert to module_platform_driver()

[Grygorii Strashko]

The OMAP I2C driver has a relation to pinctrl-single driver. As result,
its probe will be deferred during system boot until late init time,
because the pinctrl-single is initizalized as moudle/device init time.
This, in turn, will delay initialization of all I2C devices (like mfd,
I2C regulators and etc.) and cause boot delay (more over, it can broken
initialization of drivers which are not ready to use deferred probe
mechanism yet, for example DSS).

There are no sense to keep OMAP I2C initialization on subsys init layer
any more, hence shift it to module/device layer where the i2c --
pinctrl-single dependency is resolved in drivers/Makefile now.

Cc: Wolfram Sang w...@the-dreams.de
Cc: Ben Dooks (embedded platforms) ben-li...@fluff.org
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: linux-o...@vger.kernel.org
Cc: linux-i2c@vger.kernel.org
Cc: linux-ker...@vger.kernel.org

Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
---
 drivers/i2c/busses/i2c-omap.c |   14 +-
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 4cc2f05..70d3fed 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1342,19 +1342,7 @@ static struct platform_driver omap_i2c_driver = {
},
 };
 
-/* I2C may be needed to bring up other drivers */
-static int __init
-omap_i2c_init_driver(void)
-{
-   return platform_driver_register(omap_i2c_driver);
-}
-subsys_initcall(omap_i2c_init_driver);
-
-static void __exit omap_i2c_exit_driver(void)
-{
-   platform_driver_unregister(omap_i2c_driver);
-}
-module_exit(omap_i2c_exit_driver);
+module_platform_driver(omap_i2c_driver);
 
 MODULE_AUTHOR(MontaVista Software, Inc. (and others));
 MODULE_DESCRIPTION(TI OMAP I2C bus adapter);
-- 
1.7.9.5

--
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 0/2] OMAP: fix boot sequence

[Grygorii Strashko]

On 04/23/2013 08:34 PM, Tony Lindgren wrote:

* Grygorii Strashko grygorii.stras...@ti.com [130423 06:25]:

Hi

There are two public discussions now related to OMAP boot and drivers
initialization issues:
Multiple issues with omap4 panda es in linux next
   http://www.spinics.net/lists/linux-omap/msg90241.html
[BUG] omap: mfd/regulator: twl/core: init order
   http://www.spinics.net/lists/linux-omap/msg89980.html

In both cases there are pinctrl-single/I2C/MFD/Regulators initailization issue:
- regulators are not initialized because of twl,
- twl is not initialized because of I2C,
- I2C is not initialized because of pinctrl-single,
- pinctrl-single is initialized at mudule/device init time.
So, most everything will be shifted at late_initcall time.

This may cause boot delay (more over, it can broken initialization of drivers
which are not ready to use deferred probe mechanism yet, for example DSS).

Introduced pathes shift I2C and TWL iniialization to module/device init layer
instead of subsys init layer where initialization dependencies resolved
indirectly in drivers/Makefile now.

Grygorii Strashko (2):
   i2c: omap: convert to module_platform_driver()
   mfd: twl-core: convert to module_i2c_driver()

  drivers/i2c/busses/i2c-omap.c |   14 +-
  drivers/mfd/twl-core.c|   12 +---
  2 files changed, 2 insertions(+), 24 deletions(-)

Thanks, I have few questions:

1. It seems that the real fix to the issues we're seeing
is to make the broken drivers to support -EPROBE_DEFER?

yes.


2. If so, can these be merged later on as clean-up?

if i understand Tomi correctly, he has problems with DSS updating to use
 -EPROBE_DEFER, so this is rather fix than clean-up.
But need confirmation that it's true from him.


3. Can these two patches be merged separately without
breaking things?

I think, yes.


Regards,

Tony


--
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