Eric Brower napsal(a): > Attached is a revised patch to provide enhanced multi-master > arbitration support for i2c-algo-pcf. The initial patch was > commented-on about 10 months ago, so I've included the prior mail > below for reference and interspersed my comments. I am not a member > of this list, so please copy me directly on replies. The attached > patch has been tested against a fairly recent Linus 2.6 GIT tree. > > Ref: http://lists.lm-sensors.org/pipermail/i2c/2007-August/001690.html > > Commit text: > Improve lost-arbitration handling of PCF8584. This is necessary for > support of a currently out-of-kernel driver for Sun Microsystems E250 > environmental management; perhaps others. > Signed-off-by: Eric Brower <ebrower at gmail.com> > > Daniel, please ACK if you are comfortable doing so.
I confirm this patch it works well on my E250. Acked by: Dan Smolik <[EMAIL PROTECTED]> Dan > > >> Hi Eric, >> >> When sending a patch, please set the attachment type to something more >> friendly than "application/octet-stream". > > Will do. I've tried to fool gmail with a .txt extension, rather than .patch. > >> On Thu, 19 Jul 2007 22:26:08 -0700, Eric Brower wrote: >>> Attached is a patch that improves multi-master support for PCF8584. >>> The current implementation of multi-master support (mine) in >>> i2c-algo-pcf does not properly handle a collision detected during >>> wait_for_bb() >>> and thus causes an error that is only recoverable by reloading the >>> relevant device drivers. I'd appreciate your consideration of this >>> patch. >>> >>> The only affected driver is i2c-envctrl.c-- currently out-of-kernel, >>> in development; you >>> can find it here if you are interested: >>> >>> http://www.david-web.co.uk/index.php?p=envctrl >>> >>> This change has been tested by a few individuals (on Sun Microsystems >>> E250 servers) over the course of several months, and rectifies >>> occasional errors seen without the patch applied. >>> >>> I'll be happy to provide further clarification if you are interested. >>> Please cc me on responses-- I am not a member of this list. >> Note that I can't test your patch, as I don't have supported hardware. >> >>> Commit text: >>> >>> Improve lost-arbitration handling of PCF8584 >>> Signed-off-by: Eric Brower <ebrower at gmail.com> >> Review: >> >>> diff --git a/drivers/i2c/algos/i2c-algo-pcf.c >>> b/drivers/i2c/algos/i2c-algo-pcf.c >>> index ecb2c2d..2cc0302 100644 >>> --- a/drivers/i2c/algos/i2c-algo-pcf.c >>> +++ b/drivers/i2c/algos/i2c-algo-pcf.c >>> @@ -78,6 +78,31 @@ static void i2c_stop(struct i2c_algo_pcf >>> set_pcf(adap, 1, I2C_PCF_STOP); >>> } >>> >>> +static inline void handle_lab(struct i2c_algo_pcf_data *adap, const int >>> *status) >>> +{ >>> + DEB2(printk(KERN_INFO >>> + "i2c-algo-pcf.o: lost arbitration (CSR 0x%02x)\n", >>> + *status)); >>> + /* Cleanup from LAB-- reset and enable ESO. >>> + * This resets the PCF8584; since we've lost the bus, no >>> + * further attempts should be made by callers to clean up >>> + * (no i2c_stop() etc.) >>> + */ >>> + set_pcf(adap, 1, I2C_PCF_PIN); >>> + set_pcf(adap, 1, I2C_PCF_ESO); >>> + /* We pause for a time period sufficient for any running >>> + * I2C transaction to complete-- the arbitration logic won't >>> + * work properly until the next START is seen. >>> + * It is assumed the bus driver or client has set a proper value. >>> + */ >>> + if (adap->lab_mdelay) { >>> + mdelay(adap->lab_mdelay); >>> + } >> Out of curiosity: on what basis will the value of lab_mdelay be chosen >> by the driver author? This seems to be pretty arbitrary, as you don't >> know what kind of transactions the other master is doing. The >> underlying question being: is it worth having a per-adapter parameter >> for this, as opposed to a hard-coded value? > > The driver (bus adapter) author shall select a value that is longer > than the longest start/stop bus transaction possible by either master. > This is not entirely arbitrary, and can be determined by snooping the > i2c bus. Unfortunately, for proper lost-arbitration functionality > with PCF8584 you must determine a sufficient value. This is described > in the Philips/NXP AN96040 document: > > "Attention ! In both cases, the PCF8584 is not able to recognise that > there is still an ongoing > transmission. In order to prevent another arbitration problem the > controlling software should > wait for a time, longer than the longest message which can be sent > over the bus, before trying > to access the bus again." > > The parameter should be per-adapter, not hard-coded, for the above reason. > > Here's an example of a bus driver (i2c-envctrl) setting a desired value: > > static struct i2c_algo_pcf_data pcf_envctrl_data = { > .setpcf = pcf_envctrl_setbyte, > .getpcf = pcf_envctrl_getbyte, > .getown = pcf_envctrl_getown, > .getclock = pcf_envctrl_getclock, > .waitforpin = pcf_envctrl_waitforpin, > .udelay = 10, > .timeout = 100, > /* > * This could be made dynamic based upon get_clock(), but > * we know clock does not change within this implementation, so: > * 10,000 uSec per bit (@100kHz) * 9 bits/byte * 3 trans > */ > .lab_mdelay = 270, > }; > > >> Also, it seems to me that you could sleep here. If you sleep a bit >> longer than expected, it shouldn't be a problem, right? > > Not a problem. The pause is only intended to ensure at least > "lab_mdelay" msecs prior to the next attempt to use the bus. This > gives the internal state machine of PCF8584 sufficient time to see a > stop or start sequence on the bus. > >>> + DEB2(printk(KERN_INFO >>> + "i2c-algo-pcf.o: reset LAB condition (CSR 0x%02x)\n", >>> + get_pcf(adap,1))); >> Coding style: space after comma. > > Done. Thanks. > >>> +} >> This function is too big to be inlined, especially given that it should >> rarely be called, so speeding it up isn't very interesting. > > Understood-- inline removed. > >>> + >>> static int wait_for_bb(struct i2c_algo_pcf_data *adap) { >>> >>> int timeout = DEF_TIMEOUT; >>> @@ -86,6 +111,10 @@ static int wait_for_bb(struct i2c_algo_p >>> status = get_pcf(adap, 1); >>> #ifndef STUB_I2C >>> while (timeout-- && !(status & I2C_PCF_BB)) { >>> + if (status & I2C_PCF_LAB) { >>> + handle_lab(adap, &status); >>> + return -EINTR; >>> + } >> How can this happen? We check for busy bus before we start >> transmitting, and arbitration can't be lost until we at least try to >> transmit. > > This is a side-effect that occurs when the lab_mdelay value is not > long enough to ensure a stop or start sequence is seen by the master. > The internal state machine of the PCF8584 can trigger LAB at an > unexpected time. In fact, you are correct that we should not see it > here; given the current structure of the driver, however, we do see it > here and the driver otherwise makes no attempt to resolve the issue. > This state requires a driver reload to rectify. Future work on this > driver could perhaps resolve the issue of this (and other unexpected > conditions) via a reset. > > However, if we assume the bus driver author has selected a proper > value, we should not expect this to occur. So, I've removed this > section and it could be added back in the future as "insurance" > against unexpected behavior if necessary. > >>> udelay(100); /* wait for 100 us */ >>> status = get_pcf(adap, 1); >>> } >>> @@ -109,23 +139,7 @@ static int wait_for_pin(struct i2c_algo_ >>> *status = get_pcf(adap, 1); >>> } >>> if (*status & I2C_PCF_LAB) { >>> - DEB2(printk(KERN_INFO >>> - "i2c-algo-pcf.o: lost arbitration (CSR 0x%02x)\n", >>> - *status)); >>> - /* Cleanup from LAB-- reset and enable ESO. >>> - * This resets the PCF8584; since we've lost the bus, no >>> - * further attempts should be made by callers to clean up >>> - * (no i2c_stop() etc.) >>> - */ >>> - set_pcf(adap, 1, I2C_PCF_PIN); >>> - set_pcf(adap, 1, I2C_PCF_ESO); >>> - /* TODO: we should pause for a time period sufficient for any >>> - * running I2C transaction to complete-- the arbitration >>> - * logic won't work properly until the next START is seen. >>> - */ >>> - DEB2(printk(KERN_INFO >>> - "i2c-algo-pcf.o: reset LAB condition (CSR 0x%02x)\n", >>> - get_pcf(adap,1))); >>> + handle_lab(adap, status); >>> return(-EINTR); >>> } >>> #endif >>> @@ -218,9 +232,8 @@ static inline int try_address(struct i2c >>> break; /* success! */ >>> } >>> } >>> - if (wfp == -EINTR) { >>> + else if (wfp == -EINTR) { >>> /* arbitration lost */ >>> - udelay(adap->udelay); >>> return -EINTR; >>> } >>> i2c_stop(adap); >>> @@ -378,6 +393,10 @@ static int pcf_xfer(struct i2c_adapter * >>> /* Check for bus busy */ >>> timeout = wait_for_bb(adap); >>> if (timeout) { >>> + if (timeout == -EINTR) { >>> + /* arbitration lost */ >>> + return -EINTR; >>> + } >>> DEB2(printk(KERN_ERR "i2c-algo-pcf.o: " >>> "Timeout waiting for BB in pcf_xfer\n");) >>> return -EIO; >>> diff --git a/include/linux/i2c-algo-pcf.h b/include/linux/i2c-algo-pcf.h >>> index 994eb86..d11c141 100644 >>> --- a/include/linux/i2c-algo-pcf.h >>> +++ b/include/linux/i2c-algo-pcf.h >>> @@ -36,6 +36,12 @@ struct i2c_algo_pcf_data { >>> /* local settings */ >>> int udelay; >>> int timeout; >>> + >>> + /* Multi-master lost arbitration back-off delay (msecs) >>> + * This should be set by the bus adapter or knowledgable client >>> + * if bus is multi-mastered, else zero >>> + */ >>> + unsigned long lab_mdelay; >>> }; >>> >>> int i2c_pcf_add_bus(struct i2c_adapter *); >> As a side question, shouldn't i2c-algo-pcf retry, rather than fail, on >> arbitration lost? > > It is not a safe assumption that a command (or series of commands) can > be re-tried after arbitration is lost. A convenience function or > setting in the driver may be worthwhile for cases where the consumer > knows the action to be safe, but I would divorce such an enhancement > from this proposed patch. > >> -- >> Jean Delvare > > Thank you again for your consideration. Apologies for the tardiness of a > reply. > > > ------------------------------------------------------------------------ > > diff --git a/drivers/i2c/algos/i2c-algo-pcf.c > b/drivers/i2c/algos/i2c-algo-pcf.c > index ecb2c2d..5b8e686 100644 > --- a/drivers/i2c/algos/i2c-algo-pcf.c > +++ b/drivers/i2c/algos/i2c-algo-pcf.c > @@ -78,6 +78,31 @@ static void i2c_stop(struct i2c_algo_pcf > set_pcf(adap, 1, I2C_PCF_STOP); > } > > +static void handle_lab(struct i2c_algo_pcf_data *adap, const int *status) > +{ > + DEB2(printk(KERN_INFO > + "i2c-algo-pcf.o: lost arbitration (CSR 0x%02x)\n", > + *status)); > + /* Cleanup from LAB-- reset and enable ESO. > + * This resets the PCF8584; since we've lost the bus, no > + * further attempts should be made by callers to clean up > + * (no i2c_stop() etc.) > + */ > + set_pcf(adap, 1, I2C_PCF_PIN); > + set_pcf(adap, 1, I2C_PCF_ESO); > + /* We pause for a time period sufficient for any running > + * I2C transaction to complete-- the arbitration logic won't > + * work properly until the next START is seen. > + * It is assumed the bus driver or client has set a proper value. > + */ > + if (adap->lab_mdelay) { > + mdelay(adap->lab_mdelay); > + } > + DEB2(printk(KERN_INFO > + "i2c-algo-pcf.o: reset LAB condition (CSR 0x%02x)\n", > + get_pcf(adap, 1))); > +} > + > static int wait_for_bb(struct i2c_algo_pcf_data *adap) { > > int timeout = DEF_TIMEOUT; > @@ -109,23 +134,7 @@ static int wait_for_pin(struct i2c_algo_ > *status = get_pcf(adap, 1); > } > if (*status & I2C_PCF_LAB) { > - DEB2(printk(KERN_INFO > - "i2c-algo-pcf.o: lost arbitration (CSR 0x%02x)\n", > - *status)); > - /* Cleanup from LAB-- reset and enable ESO. > - * This resets the PCF8584; since we've lost the bus, no > - * further attempts should be made by callers to clean up > - * (no i2c_stop() etc.) > - */ > - set_pcf(adap, 1, I2C_PCF_PIN); > - set_pcf(adap, 1, I2C_PCF_ESO); > - /* TODO: we should pause for a time period sufficient for any > - * running I2C transaction to complete-- the arbitration > - * logic won't work properly until the next START is seen. > - */ > - DEB2(printk(KERN_INFO > - "i2c-algo-pcf.o: reset LAB condition (CSR 0x%02x)\n", > - get_pcf(adap,1))); > + handle_lab(adap, status); > return(-EINTR); > } > #endif > diff --git a/include/linux/i2c-algo-pcf.h b/include/linux/i2c-algo-pcf.h > index 994eb86..d11c141 100644 > --- a/include/linux/i2c-algo-pcf.h > +++ b/include/linux/i2c-algo-pcf.h > @@ -36,6 +36,12 @@ struct i2c_algo_pcf_data { > /* local settings */ > int udelay; > int timeout; > + > + /* Multi-master lost arbitration back-off delay (msecs) > + * This should be set by the bus adapter or knowledgable client > + * if bus is multi-mastered, else zero > + */ > + unsigned long lab_mdelay; > }; > > int i2c_pcf_add_bus(struct i2c_adapter *); -- Mydatex s r.o. http://www.mydatex.cz email: [EMAIL PROTECTED] mob: 604200362 tel: 226210085 _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
