| Message ID | CD8CC2B65FEE304DA95744A5472698F202957C7C3A@dlee06.ent.ti.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following: >> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG. >> Otherwise some data bytes can be lost while transferring them from the >> memory to the I2C interface. >> >> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting >> if there is NACK | AL, set the appropriate error flags, ack the pending >> + } >> + cpu_relax(); >> + stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); >> + } > this is an infinite while loop + it tries to handle error cases - > essentially do another isr routine inside itself. > [Moiz] > Yes, it is a busy wait loop. But AFAIK while we come to this part of > the code the only thing that can go wrong in the transfer is either > the device would go off suddenly (creating a NACK) or there is an > arbitration lost(AL). This loop takes care of these two error conditions. > Apart from these, if the hardware is functional, the XUDF bit should > be set as soon as the FIFO gets empty. > > Correct me if I am missing something. That is exactly what I was complaining about -> why not use the existing logic above in the code to take care of it as I indicated below? do you see a problem with the logic I send? it is lesser code and should IMHO take care of the same issue without the additional 3rd nested while loop > > How about: > Apply [1] and the following? > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index ad8d201..e3f21af 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id) > } > if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) { > u8 num_bytes = 1; > + if (cpu_is_omap34xx() && > + !(stat & OMAP_I2C_STAT_XUDF)){ > + cpu_relax(); > + continue; > + } > + > if (dev->fifo_size) { > if (stat & OMAP_I2C_STAT_XRDY) > num_bytes = dev->fifo_size; > > > Regards, > Nishanth Menon > Ref: > [1] http://patchwork.kernel.org/patch/32332/
Hello Nishant, Comments inlined, Regards Moiz Sonasath -----Original Message----- From: Menon, Nishanth Sent: Wednesday, July 15, 2009 4:43 PM To: Sonasath, Moiz Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following: >> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG. >> Otherwise some data bytes can be lost while transferring them from the >> memory to the I2C interface. >> >> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting >> if there is NACK | AL, set the appropriate error flags, ack the pending >> + } >> + cpu_relax(); >> + stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); >> + } > this is an infinite while loop + it tries to handle error cases - > essentially do another isr routine inside itself. > [Moiz] > Yes, it is a busy wait loop. But AFAIK while we come to this part of > the code the only thing that can go wrong in the transfer is either > the device would go off suddenly (creating a NACK) or there is an > arbitration lost(AL). This loop takes care of these two error conditions. > Apart from these, if the hardware is functional, the XUDF bit should > be set as soon as the FIFO gets empty. > > Correct me if I am missing something. That is exactly what I was complaining about -> why not use the existing logic above in the code to take care of it as I indicated below? do you see a problem with the logic I send? it is lesser code and should IMHO take care of the same issue without the additional 3rd nested while loop > > How about: > Apply [1] and the following? > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index ad8d201..e3f21af 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id) > } > if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) { > u8 num_bytes = 1; > + if (cpu_is_omap34xx() && > + !(stat & OMAP_I2C_STAT_XUDF)){ > + cpu_relax(); > + continue; > + } > + [Moiz] In this alternate implementation if a NACK|AL is produced while waiting on the XUDF, it returns from the ISR (as per my patch [2/3]) without ACKING the XRDY/XDR interrupts which would make it return to the ISR again and again which looks like a problem. To accommodate this implementation, we will have to ACK XRDY/XDR before returning from the ISR. > if (dev->fifo_size) { > if (stat & OMAP_I2C_STAT_XRDY) > num_bytes = dev->fifo_size; > > > Regards, > Nishanth Menon > Ref: > [1] http://patchwork.kernel.org/patch/32332/
Sonasath, Moiz had written, on 07/15/2009 05:23 PM, the following: > Hello Nishant, > > Comments inlined, > > Regards > Moiz Sonasath > > > -----Original Message----- > From: Menon, Nishanth > Sent: Wednesday, July 15, 2009 4:43 PM > To: Sonasath, Moiz > Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley > Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 > > Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following: >>> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG. >>> Otherwise some data bytes can be lost while transferring them from the >>> memory to the I2C interface. >>> >>> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting >>> if there is NACK | AL, set the appropriate error flags, ack the pending >>> + } >>> + cpu_relax(); >>> + stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); >>> + } >> this is an infinite while loop + it tries to handle error cases - >> essentially do another isr routine inside itself. >> [Moiz] >> Yes, it is a busy wait loop. But AFAIK while we come to this part of > > the code the only thing that can go wrong in the transfer is either > > the device would go off suddenly (creating a NACK) or there is an > > arbitration lost(AL). This loop takes care of these two error conditions. > > Apart from these, if the hardware is functional, the XUDF bit should > > be set as soon as the FIFO gets empty. >> Correct me if I am missing something. > That is exactly what I was complaining about -> why not use the existing > logic above in the code to take care of it as I indicated below? do you > see a problem with the logic I send? it is lesser code and should IMHO > take care of the same issue without the additional 3rd nested while loop >> How about: >> Apply [1] and the following? >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index ad8d201..e3f21af 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id) >> } >> if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) { >> u8 num_bytes = 1; >> + if (cpu_is_omap34xx() && >> + !(stat & OMAP_I2C_STAT_XUDF)){ >> + cpu_relax(); >> + continue; >> + } >> + > > [Moiz] > > In this alternate implementation if a NACK|AL is produced while waiting > on the XUDF, it returns from the ISR (as per my patch [2/3]) > without ACKING the XRDY/XDR interrupts which would make it return > to the ISR again and again which looks like a problem. To accommodate >this implementation, we will have to ACK XRDY/XDR before returning from the ISR. ok, this is due to my [1] patch which should have handled this condition in the first place. I will rework my original [1] patch and resend it. Regards, Nishanth Menon Ref: [1] http://patchwork.kernel.org/patch/32332/ -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Nishant! I am also not sure, if the count=100; value will be enough time for the XUDF to be set. If not then it will keep running into timeout errors. Regards Moiz Sonasath -----Original Message----- From: Menon, Nishanth Sent: Wednesday, July 15, 2009 5:27 PM To: Sonasath, Moiz Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley; Pandita, Vikram Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 Sonasath, Moiz had written, on 07/15/2009 05:23 PM, the following: > Hello Nishant, > > Comments inlined, > > Regards > Moiz Sonasath > > > -----Original Message----- > From: Menon, Nishanth > Sent: Wednesday, July 15, 2009 4:43 PM > To: Sonasath, Moiz > Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley > Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 > > Sonasath, Moiz had written, on 07/15/2009 10:40 AM, the following: >>> When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG. >>> Otherwise some data bytes can be lost while transferring them from the >>> memory to the I2C interface. >>> >>> Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting >>> if there is NACK | AL, set the appropriate error flags, ack the pending >>> + } >>> + cpu_relax(); >>> + stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); >>> + } >> this is an infinite while loop + it tries to handle error cases - >> essentially do another isr routine inside itself. >> [Moiz] >> Yes, it is a busy wait loop. But AFAIK while we come to this part of > > the code the only thing that can go wrong in the transfer is either > > the device would go off suddenly (creating a NACK) or there is an > > arbitration lost(AL). This loop takes care of these two error conditions. > > Apart from these, if the hardware is functional, the XUDF bit should > > be set as soon as the FIFO gets empty. >> Correct me if I am missing something. > That is exactly what I was complaining about -> why not use the existing > logic above in the code to take care of it as I indicated below? do you > see a problem with the logic I send? it is lesser code and should IMHO > take care of the same issue without the additional 3rd nested while loop >> How about: >> Apply [1] and the following? >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index ad8d201..e3f21af 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id) >> } >> if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) { >> u8 num_bytes = 1; >> + if (cpu_is_omap34xx() && >> + !(stat & OMAP_I2C_STAT_XUDF)){ >> + cpu_relax(); >> + continue; >> + } >> + > > [Moiz] > > In this alternate implementation if a NACK|AL is produced while waiting > on the XUDF, it returns from the ISR (as per my patch [2/3]) > without ACKING the XRDY/XDR interrupts which would make it return > to the ISR again and again which looks like a problem. To accommodate >this implementation, we will have to ACK XRDY/XDR before returning from the ISR. ok, this is due to my [1] patch which should have handled this condition in the first place. I will rework my original [1] patch and resend it. Regards, Nishanth Menon Ref: [1] http://patchwork.kernel.org/patch/32332/ -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following: > > I am also not sure, if the count=100; value will be enough time for the XUDF > to be set. If not then it will keep running into timeout errors. > Do you mean we need a delay for checking again? that should be easy to incorporate - what kind of delay are we speaking of here? do you have a count requirement for handling this? it is essentially the time b/w XRDY to XUNDF.. this should be deterministic rt? Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Nishant, AFAIK, the time between XRDY/XDR and setting of XUDF bit is not deterministic, it depends on the I2C bus speed but I am not sure if we can translate the speed into a fixed count number which we can use as a timeout limit. In that case we need to make a balanced assumption of the count value so that we don't fall in the timeout case under normal operation. May be someone can give a pointer here. Regards Moiz Sonasath -----Original Message----- From: Menon, Nishanth Sent: Wednesday, July 15, 2009 5:32 PM To: Sonasath, Moiz Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley; Pandita, Vikram Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following: > > I am also not sure, if the count=100; value will be enough time for the XUDF > to be set. If not then it will keep running into timeout errors. > Do you mean we need a delay for checking again? that should be easy to incorporate - what kind of delay are we speaking of here? do you have a count requirement for handling this? it is essentially the time b/w XRDY to XUNDF.. this should be deterministic rt? Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sonasath, Moiz had written, on 07/15/2009 05:37 PM, the following: > > -----Original Message----- > From: Menon, Nishanth > Sent: Wednesday, July 15, 2009 5:32 PM please stop top posting.. >> Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following: >> I am also not sure, if the count=100; value will be enough time for the XUDF >> > to be set. If not then it will keep running into timeout errors. >> Do you mean we need a delay for checking again? that should be easy to >> incorporate - what kind of delay are we speaking of here? do you have a >> count requirement for handling this? it is essentially the time b/w XRDY >> to XUNDF.. this should be deterministic rt? >> > > AFAIK, the time between XRDY/XDR and setting of XUDF bit is not >deterministic, it depends on the I2C bus speed but I am not sure if we >can translate the speed into a fixed count number which we can use as a > timeout limit. In that case we need to make a balanced assumption of >the count value so that we don't fall in the timeout case under normal >operation. > > May be someone can give a pointer here. > Here is my attempt at this: XRDY -> in this case the FIFO is completely empty - fill it up for XTRSH XDR -> FIFO is not completely empty, fill as per TXSTAT reg. if you look at the sequences of events that should happen -> a) no previous data transmitted: XRDY, XUNDF b) previous data available, XDR, XRDY, XUNDF The variables are: i) bus speed ii) num bytes to empty (if XRDY then 0, else TXSTAT) Time = (num_bytes+c) * (1/bus_speed) where c = some constant time interval for XRDY->XUNDF.. I think.. One possible strategy: a)Worst case is XTRSH, so, Compute time prior to entry into transmit loop. Set the count as this+ add a constant for additional events b) Add a constant delay when you decide to continue back - not sure if cpu_relax is predictable delay.. -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Nishant, Comments inlined, Regards Moiz Sonasath -----Original Message----- From: Menon, Nishanth Sent: Wednesday, July 15, 2009 6:03 PM To: Sonasath, Moiz Cc: linux-omap@vger.kernel.org; Kamat, Nishant; Paul Walmsley; Pandita, Vikram Subject: Re: [PATCH 3/3] [OMAP:I2C]OMAP3430 Silicon Errata 1.153 Sonasath, Moiz had written, on 07/15/2009 05:37 PM, the following: > > -----Original Message----- > From: Menon, Nishanth > Sent: Wednesday, July 15, 2009 5:32 PM please stop top posting.. >> Sonasath, Moiz had written, on 07/15/2009 05:29 PM, the following: >> I am also not sure, if the count=100; value will be enough time for the XUDF >> > to be set. If not then it will keep running into timeout errors. >> Do you mean we need a delay for checking again? that should be easy to >> incorporate - what kind of delay are we speaking of here? do you have a >> count requirement for handling this? it is essentially the time b/w XRDY >> to XUNDF.. this should be deterministic rt? >> > > AFAIK, the time between XRDY/XDR and setting of XUDF bit is not >deterministic, it depends on the I2C bus speed but I am not sure if we >can translate the speed into a fixed count number which we can use as a > timeout limit. In that case we need to make a balanced assumption of >the count value so that we don't fall in the timeout case under normal >operation. > > May be someone can give a pointer here. > Here is my attempt at this: XRDY -> in this case the FIFO is completely empty - fill it up for XTRSH XDR -> FIFO is not completely empty, fill as per TXSTAT reg. if you look at the sequences of events that should happen -> a) no previous data transmitted: XRDY, XUNDF b) previous data available, XDR, XRDY, XUNDF The variables are: i) bus speed ii) num bytes to empty (if XRDY then 0, else TXSTAT) Time = (num_bytes+c) * (1/bus_speed) where c = some constant time interval for XRDY->XUNDF.. I think.. One possible strategy: a)Worst case is XTRSH, so, Compute time prior to entry into transmit loop. Set the count as this+ add a constant for additional events b) Add a constant delay when you decide to continue back - not sure if cpu_relax is predictable delay.. [Moiz] Let me just rephrase what you said and from what I see in the TRM, XRDY-> when TX FIFO level is equal/below XTRSH and TXSTAT is equal/greater than XTRSH, LH will write number of data bytes specified by XTRSH XDR-> when TX FIFO level is equal/below the XTRSH and TXSTAT is less than XTRSH, LH will write number of data bytes specified by TXSTAT XUDF-> when shift register and the TX FIFO are empty and there are still some bytes to transmit (DCOUNT not 0) Here we are looking at the wait time between XRDY/XDR and XUDF. Considering worst case scenarios, Wait_time = (time to transmit out XTRSH bytes from the FIFO at the slowest bus speed) + (safeguard delay) The slowest bus speed appears in standard mode (100K bits/sec). Therefore, Wait_time = [(XTRSH*8)*(1/100K)] seconds + constant; Does this look correct? -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Sonasath, Moiz wrote: > > When an XRDY/XDR is hit, wait for XUDF before writing data to DATA_REG. > > Otherwise some data bytes can be lost while transferring them from the > > memory to the I2C interface. > > > > Do a Busy-wait for XUDF, before writing data to DATA_REG. While waiting > > if there is NACK | AL, set the appropriate error flags, ack the pending > > interrupts and return from the ISR. > > > > Signed-off-by: Moiz Sonasath<m-sonasath@ti.com> > > Signed-off-by: Vikram Pandita<vikram.pandita@ti.com> > > --- > > drivers/i2c/busses/i2c-omap.c | 24 +++++++++++++++++++++++- > > 1 files changed, 23 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c- > omap.c > > index 05b5e4c..8deaf87 100644 > > --- a/drivers/i2c/busses/i2c-omap.c > > +++ b/drivers/i2c/busses/i2c-omap.c > > @@ -672,9 +672,10 @@ omap_i2c_isr(int this_irq, void *dev_id) > > break; > > } > > > > + err = 0; > > +complete: > cant we rename this label? > [Moiz] > This path actually completes the IRQ service routine and therefore the > name. > > > omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat); > > > > - err = 0; > > if (stat & OMAP_I2C_STAT_NACK) { > > err |= OMAP_I2C_STAT_NACK; > > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, > > @@ -764,6 +765,27 @@ omap_i2c_isr(int this_irq, void *dev_id) > > "data to send\n"); > > break; > > } > > + > > + /* > > + * OMAP3430 Errata 1.153: When an XRDY/XDR > > + * is hit, wait for XUDF before writing data > > + * to DATA_REG. Otherwise some data bytes can > > + * be lost while transferring them from the > > + * memory to the I2C interface. > > + */ > > + > > + if (cpu_is_omap34xx()) { > > + while (!(stat & OMAP_I2C_STAT_XUDF)) { > > + if (stat & (OMAP_I2C_STAT_NACK | > OMAP_I2C_STAT_AL)) { > > + omap_i2c_ack_stat(dev, > stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)); > generic comment - code nesting is getting overwhelming - we may like to > refactor the isr function at a later date I think.. > > + err |= OMAP_I2C_STAT_XUDF; > why set the err if we wantedly force this to happen? > [Moiz] > The idea here is to let the upper application to know that something went > wrong while waiting for the XUDF bit to be set. This is just for debugging > purpose. > > + goto complete; > > > + } > > + cpu_relax(); > > + stat = omap_i2c_read_reg(dev, > OMAP_I2C_STAT_REG); > > + } > this is an infinite while loop + it tries to handle error cases - > essentially do another isr routine inside itself. > [Moiz] > Yes, it is a busy wait loop. But AFAIK while we come to this part of the > code the only thing that can go wrong in the transfer is either the device > would go off suddenly (creating a NACK) or there is an arbitration > lost(AL). This loop takes care of these two error conditions. Apart from > these, if the hardware is functional, the XUDF bit should be set as soon > as the FIFO gets empty. > > Correct me if I am missing something. As of now I am posting my patch without a timeout, later as per the need I will optimize it with a timeout approach. > > How about: > Apply [1] and the following? > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index ad8d201..e3f21af 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id) > } > if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) { > u8 num_bytes = 1; > + if (cpu_is_omap34xx() && > + !(stat & OMAP_I2C_STAT_XUDF)){ > + cpu_relax(); > + continue; > + } > + > if (dev->fifo_size) { > if (stat & OMAP_I2C_STAT_XRDY) > num_bytes = dev->fifo_size; > > > Regards, > Nishanth Menon > Ref: > [1] http://patchwork.kernel.org/patch/32332/ > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index ad8d201..e3f21af 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -728,6 +728,12 @@ omap_i2c_isr(int this_irq, void *dev_id) } if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) { u8 num_bytes = 1; + if (cpu_is_omap34xx() && + !(stat & OMAP_I2C_STAT_XUDF)){ + cpu_relax(); + continue; + } + if (dev->fifo_size) { if (stat & OMAP_I2C_STAT_XRDY) num_bytes = dev->fifo_size;