| Message ID | 1417514749-24319-4-git-send-email-harinik@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
> >> + - 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...
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
> 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.
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
> > 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.
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
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 {