diff mbox

[3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

Message ID 1417514749-24319-4-git-send-email-harinik@xilinx.com
State New, archived
Headers show

Commit Message

Harini Katakam Dec. 2, 2014, 10:05 a.m. UTC
From: Vishnu Motghare <vishnum@xilinx.com>

This patch adds "defeature-repeated-start" property in i2c-cadence.txt.

Signed-off-by: Vishnu Motghare <vishnum@xilinx.com>
Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
 .../devicetree/bindings/i2c/i2c-cadence.txt        |   11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Mark Rutland Dec. 2, 2014, 11:19 a.m. UTC | #1
On Tue, Dec 02, 2014 at 10:05:48AM +0000, Harini Katakam wrote:
> From: Vishnu Motghare <vishnum@xilinx.com>
> 
> This patch adds "defeature-repeated-start" property in i2c-cadence.txt.
> 
> Signed-off-by: Vishnu Motghare <vishnum@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
>  .../devicetree/bindings/i2c/i2c-cadence.txt        |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
> index 7cb0b56..9d417a7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
> @@ -11,6 +11,17 @@ Required properties:
>  Optional properties:
>    - clock-frequency: Desired operating frequency, in Hz, of the bus.
>    - clock-names: Input clock name, should be 'pclk'.
> +  - defeature-repeated-start: Include this property to defeature repeated start
> +			      This defeature is due to a few bugs in the
> +			      I2C controller.
> +			      Completion interrupt after a read/receive
> +			      operation is NOT obtained if HOLD bit is set
> +			      at that time. Because of this bug, repeated start
> +			      will only work if there are no transfers following
> +			      a read/receive transfer.
> +			      If HOLD is held for long without a transfer,
> +			      invalid read transactions are generated by the
> +			      controller due to a HW timeout related bug.

I'm not keen on the name; it sounds like we're disabling a feature
rather than describing the problem (and "defeature" is not a common
term in this sense, "disable" would be better).

It sounds like there are two issues with staying in the HOLD state? Lost
completion IRQs and a separate HW timeout bug? Or are the two related?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Harini Katakam Dec. 2, 2014, 12:13 p.m. UTC | #2
Hi Mark,

On Tue, Dec 2, 2014 at 4:49 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Dec 02, 2014 at 10:05:48AM +0000, Harini Katakam wrote:
>> From: Vishnu Motghare <vishnum@xilinx.com>
>>
>> This patch adds "defeature-repeated-start" property in i2c-cadence.txt.
>>
>> Signed-off-by: Vishnu Motghare <vishnum@xilinx.com>
>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>> ---
>>  .../devicetree/bindings/i2c/i2c-cadence.txt        |   11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
>> index 7cb0b56..9d417a7 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
>> @@ -11,6 +11,17 @@ Required properties:
>>  Optional properties:
>>    - clock-frequency: Desired operating frequency, in Hz, of the bus.
>>    - clock-names: Input clock name, should be 'pclk'.
>> +  - defeature-repeated-start: Include this property to defeature repeated start
>> +                           This defeature is due to a few bugs in the
>> +                           I2C controller.
>> +                           Completion interrupt after a read/receive
>> +                           operation is NOT obtained if HOLD bit is set
>> +                           at that time. Because of this bug, repeated start
>> +                           will only work if there are no transfers following
>> +                           a read/receive transfer.
>> +                           If HOLD is held for long without a transfer,
>> +                           invalid read transactions are generated by the
>> +                           controller due to a HW timeout related bug.
>
> I'm not keen on the name; it sounds like we're disabling a feature
> rather than describing the problem (and "defeature" is not a common
> term in this sense, "disable" would be better).
>
> It sounds like there are two issues with staying in the HOLD state? Lost
> completion IRQs and a separate HW timeout bug? Or are the two related?
>

Yes, there are two issues here and they are not related.
But a combination of both is leading to not using repeated start.
The intention was to defeature except that it works in some scenarios
(such as a typical write+read in that order with repeated start)
and there are people who already use the driver with slaves that need this.

Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Dec. 2, 2014, 12:52 p.m. UTC | #3
> >> +  - defeature-repeated-start: Include this property to defeature repeated start
> >> +                           This defeature is due to a few bugs in the
> >> +                           I2C controller.
> >> +                           Completion interrupt after a read/receive
> >> +                           operation is NOT obtained if HOLD bit is set
> >> +                           at that time. Because of this bug, repeated start
> >> +                           will only work if there are no transfers following
> >> +                           a read/receive transfer.
> >> +                           If HOLD is held for long without a transfer,
> >> +                           invalid read transactions are generated by the
> >> +                           controller due to a HW timeout related bug.
> >
> > I'm not keen on the name; it sounds like we're disabling a feature
> > rather than describing the problem (and "defeature" is not a common
> > term in this sense, "disable" would be better).
> >
> > It sounds like there are two issues with staying in the HOLD state? Lost
> > completion IRQs and a separate HW timeout bug? Or are the two related?
> >
> 
> Yes, there are two issues here and they are not related.
> But a combination of both is leading to not using repeated start.
> The intention was to defeature except that it works in some scenarios
> (such as a typical write+read in that order with repeated start)
> and there are people who already use the driver with slaves that need this.

That should not be handled using a binding. If you get a transfer (at
runtime) with criteria you don't support, return with -EOPNOTSUPP from
the master xfer routine.

That being said, the number of broken/not-fully-compliant I2C
controllers has increased a lot recent times (why can't we just use the
established old ones?). Maybe we will have core support for a subset of
I2C (wr+rd) in the future, but that's still ahead...
Harini Katakam Dec. 2, 2014, 1:10 p.m. UTC | #4
Hi,

On Tue, Dec 2, 2014 at 6:22 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> >> +  - defeature-repeated-start: Include this property to defeature repeated start
>> >> +                           This defeature is due to a few bugs in the
>> >> +                           I2C controller.
>> >> +                           Completion interrupt after a read/receive
>> >> +                           operation is NOT obtained if HOLD bit is set
>> >> +                           at that time. Because of this bug, repeated start
>> >> +                           will only work if there are no transfers following
>> >> +                           a read/receive transfer.
>> >> +                           If HOLD is held for long without a transfer,
>> >> +                           invalid read transactions are generated by the
>> >> +                           controller due to a HW timeout related bug.
>> >
>> > I'm not keen on the name; it sounds like we're disabling a feature
>> > rather than describing the problem (and "defeature" is not a common
>> > term in this sense, "disable" would be better).
>> >
>> > It sounds like there are two issues with staying in the HOLD state? Lost
>> > completion IRQs and a separate HW timeout bug? Or are the two related?
>> >
>>
>> Yes, there are two issues here and they are not related.
>> But a combination of both is leading to not using repeated start.
>> The intention was to defeature except that it works in some scenarios
>> (such as a typical write+read in that order with repeated start)
>> and there are people who already use the driver with slaves that need this.
>
> That should not be handled using a binding. If you get a transfer (at
> runtime) with criteria you don't support, return with -EOPNOTSUPP from
> the master xfer routine.
>

I put a check in place for one failure condition that we know (will
change the error code returned).
But given the bugs, it will be useful to just disable it if the system doesn't
require repeated start.
If you think DT entry is not the way to go, do you think a CONFIG option or
something better will work?
We chose a DT property because there is a good chance the user has multiple
cadence I2C controllers - one connected to a slave that needs repeated start
(like a power controller) and another that doesn't care.

Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Dec. 2, 2014, 1:16 p.m. UTC | #5
> But given the bugs, it will be useful to just disable it if the system doesn't
> require repeated start.

What do you do when disable repeated start? Sending STOP and START? If
so, this is really something different than repeated start. By using
I2C_FUNC_I2C a user expects repeated start, so if the HW does not
support it, we should say so and don't try to emulate it with something
different.

> If you think DT entry is not the way to go, do you think a CONFIG option or
> something better will work?

No, check at runtime if the transfer is possible on this HW. Bail out if
not.

> We chose a DT property because there is a good chance the user has multiple
> cadence I2C controllers - one connected to a slave that needs repeated start
> (like a power controller) and another that doesn't care.

The user should not need to know with this level of detail if we can
avoid it IMO.
Harini Katakam Dec. 2, 2014, 1:30 p.m. UTC | #6
Hi,

On Tue, Dec 2, 2014 at 6:46 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> But given the bugs, it will be useful to just disable it if the system doesn't
>> require repeated start.
>
> What do you do when disable repeated start? Sending STOP and START? If
> so, this is really something different than repeated start. By using
> I2C_FUNC_I2C a user expects repeated start, so if the HW does not
> support it, we should say so and don't try to emulate it with something
> different.
>

Yes, we send stop.
Using repeated start, when number of messages passed > 1, HOLD bit is set
by the driver. This is an indication to the controller not to send a STOP.
If we disable repeated start, the driver will not set HOLD bit.
All the messages are sent but with START and a STOP for each of them.

Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Dec. 2, 2014, 2:15 p.m. UTC | #7
> > What do you do when disable repeated start? Sending STOP and START? If
> > so, this is really something different than repeated start. By using
> > I2C_FUNC_I2C a user expects repeated start, so if the HW does not
> > support it, we should say so and don't try to emulate it with something
> > different.
> 
> Yes, we send stop.

As said before, this is wrong. Another master could interfere between
the messages when using stop+start. This is no replacement for repeated
start.
Lars-Peter Clausen Dec. 2, 2014, 3:12 p.m. UTC | #8
On 12/02/2014 03:15 PM, Wolfram Sang wrote:
>>> What do you do when disable repeated start? Sending STOP and START? If
>>> so, this is really something different than repeated start. By using
>>> I2C_FUNC_I2C a user expects repeated start, so if the HW does not
>>> support it, we should say so and don't try to emulate it with something
>>> different.
>>
>> Yes, we send stop.
>
> As said before, this is wrong. Another master could interfere between
> the messages when using stop+start. This is no replacement for repeated
> start.

More importantly a lot of I2C slaves also reset their internal state machine 
on a stop. So e.g. if reading a register is implemented by doing 
start,write,repeated start,read,stop and you replace that with 
start,write,stop,start,read,stop you'll always read register zero instead of 
the register you wanted to read.

- Lars

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

Patch

diff --git a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
index 7cb0b56..9d417a7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
@@ -11,6 +11,17 @@  Required properties:
 Optional properties:
   - clock-frequency: Desired operating frequency, in Hz, of the bus.
   - clock-names: Input clock name, should be 'pclk'.
+  - defeature-repeated-start: Include this property to defeature repeated start
+			      This defeature is due to a few bugs in the
+			      I2C controller.
+			      Completion interrupt after a read/receive
+			      operation is NOT obtained if HOLD bit is set
+			      at that time. Because of this bug, repeated start
+			      will only work if there are no transfers following
+			      a read/receive transfer.
+			      If HOLD is held for long without a transfer,
+			      invalid read transactions are generated by the
+			      controller due to a HW timeout related bug.
 
 Example:
 	i2c@e0004000 {