Kevin, Could you please take this patch in? Currently i2c is not working very well with cpuidle and off mode, and the patch is needed. I would go with V4 of the patch. - Kalle >-----Original Message----- >From: linux-omap-owner@xxxxxxxxxxxxxxx >[mailto:linux-omap-owner@xxxxxxxxxxxxxxx] On Behalf Of ext >Kalle Jokiniemi >Sent: 29. lokakuuta 2009 10:55 >To: Sonasath, Moiz >Cc: khilman@xxxxxxxxxxxxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx >Subject: RE: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency >constraint in i2c > >OK, let's try this once more, since my mail did not seem to go >to linux-omap. > >Sorry for the spam. > >See my comments below: > >On Fri, 2009-10-23 at 18:53 +0300, Sonasath, Moiz wrote: >> Hello Jokiniemi! >> >> > -----Original Message----- >> > From: Kalle Jokiniemi [mailto:kalle.jokiniemi@xxxxxxxxx] >> > Sent: Wednesday, October 21, 2009 6:51 AM >> > To: khilman@xxxxxxxxxxxxxxxxxxx >> > Cc: linux-omap@xxxxxxxxxxxxxxx; Kalle Jokiniemi; Sonasath, Moiz; >> Jarkko >> > Nikula; Paul Walmsley; Menon, Nishanth >> > Subject: [PATCH V4 3/3] OMAP: I2C: Add mpu wake up latency >> constraint in >> > i2c >> > >> > While waiting for completion of the i2c transfer, the MPU >could hit >> > OFF mode and cause several msecs of delay that made i2c transfers >> > fail more often. The extra delays and subsequent re-trys cause i2c >> > clocks to be active more often. This has also an negative >effect on >> > power consumption. >> > >> > Created a mechanism for passing and using the constraint setting >> > function in driver code. The used mpu wake up latency constraints >> > are now set individually per bus, and they are calculated based on >> > clock rate and fifo size. >> > >> > Thanks to Jarkko Nikula, Moiz Sonasath, Paul Walmsley, and >Nishanth >> > Menon for tuning out the details of this patch. >> > >> > Cc: Moiz Sonasath <m-sonasath@xxxxxx> >> > Cc: Jarkko Nikula <jhnikula@xxxxxxxxx> >> > Cc: Paul Walmsley <paul@xxxxxxxxx> >> > Cc: Nishanth Menon <nm@xxxxxx> >> > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@xxxxxxxxx> >> > --- >> > >> >> >> > dev->speed = speed; >> > dev->idle = 1; >> > @@ -911,6 +923,11 @@ omap_i2c_probe(struct platform_device *pdev) >> > */ >> > dev->fifo_size = (dev->fifo_size / 2); >> > dev->b_hw = 1; /* Enable hardware fixes */ >> > + >> > + /* calculate wakeup latency constraint for MPU */ >> > + if (dev->set_mpu_wkup_lat != NULL) >> > + dev->latency = (1000000 * dev->fifo_size) / >> > + (1000 * speed / 8); >> > } >> >> IMHO, here instead of using 'dev->fifo_size' for calculating the >> 'dev->latency', we need to use: >> >> 1. For RX case, to avoid Reciver overrun: >> if (msg->flags & I2C_M_RD) >> Use [(FIFO Depth)bytes - (FIFO THRSH)bytes] in >calculating >> dev->latency >> >> Because conceptually, RDR/RRDY interrupts are generated when >RTRSH is >> reached, so we want the MPU to be wake up within the time it >takes to >> fill the FIFO from RTRSH to FIFO Depth (FIFO full). >> >> 2. For TX case, to avoid Transmitter Underflow: >> if (!(msg->flags & I2C_M_RD)) >> Use (FIFO THRSH)bytes in calculating dev->latency >> >> Because conceptually, XDR/XRDY interrupts are generated when >XTRSH is >> reached, so we want the MPU to be wake up within the time it >takes to >> drain the FIFO from XTRSH to Zero (FIFO empty). >> >> Using, dev->fifo_size instead, works in the present code because we >> have a RTRSH/XTRSH = dev->fifo_size/2 = 4 bytes And therefore, (FIFO >> Depth)bytes - (FIFO THRSH)bytes = 8 - 4 = 4 bytes >> >> But, to make it more generic in future and to make it independent of >> any changes in the RTRSH/XTRSH values or FIFO depths in future, we >> should use a generic code here. > >Well, I don't completely agree with the necessity of preparing >for different rx/tx thresholds. For this to make sense, the >i2c-omap driver should first separate in it's code the use of >rx and tx thresholds. If someone is planning to do that, >he/she should anyway update the usage of fifo_size throughout >the code, including the wake up latency setting. > >Anyways, attached a patch that separates the mpu wake up >latencies for rx and tx. In case that is needed. Though I'm >not for it, since it adds unneeded complexity. > >- Kalle > >> >> > >> > /* reset ASAP, clearing any IRQs */ diff --git >> > a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h new >file mode >> > 100644 index 0000000..1362fba >> > --- /dev/null >> > +++ b/include/linux/i2c-omap.h >> > @@ -0,0 +1,9 @@ >> > +#ifndef __I2C_OMAP_H__ >> > +#define __I2C_OMAP_H__ >> > + >> > +struct omap_i2c_bus_platform_data { >> > + u32 clkrate; >> > + void (*set_mpu_wkup_lat)(struct device *dev, >int set); >> > +}; >> > + >> > +#endif >> > -- >> > 1.5.4.3 >> >> Regards >> Moiz Sonasath >> >-- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
