| Message ID | 1346319391-19015-3-git-send-email-remi.cardona@smartjog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/30/2012 12:36 PM, Rémi Cardona wrote: > ds3000_readreg() returns negative values in case of i2c failures. The > old code would simply return 0 when failing to read the 0xb2 register, > misleading ds3000_initfe() into believing that the firmware had been > correctly loaded. > > Signed-off-by: Rémi Cardona <remi.cardona@smartjog.com> > --- > drivers/media/dvb/frontends/ds3000.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/dvb/frontends/ds3000.c b/drivers/media/dvb/frontends/ds3000.c > index 066870a..4c774c4 100644 > --- a/drivers/media/dvb/frontends/ds3000.c > +++ b/drivers/media/dvb/frontends/ds3000.c > @@ -391,8 +391,14 @@ static int ds3000_firmware_ondemand(struct dvb_frontend *fe) > > dprintk("%s()\n", __func__); > > - if (ds3000_readreg(state, 0xb2) <= 0) > + ret = ds3000_readreg(state, 0xb2); > + if (ret == 0) { > + printk(KERN_INFO "%s: Firmware already uploaded, skipping\n", > + __func__); > return ret; > + } else if (ret < 0) { > + return ret; > + } > > /* Load firmware */ > /* request the firmware, this will block until someone uploads it */ > As I understand firmware downloading failure is coming from the fact that register read fails => fails to detect if firmware is already running or not. Original behavior to expect firmware is loaded and running when register read fails is very stupid and your fix seems much better. So first priority should be try fix that issue with register read. Is it coming from the USB stack (eg. error 110 timeout) or some other error coming from the fact chip answers wrong? Do you see other register I/O failing too? Does adding few usec sleep help? regards Antti
Hi Antti, On 08/30/2012 03:39 PM, Antti Palosaari wrote: > As I understand firmware downloading failure is coming from the fact > that register read fails => fails to detect if firmware is already > running or not. Well we actually see 2 cases: - the register read failure (when ds3000_readreg() returns negative values). This case is fairly rare, and no changes we've done to the driver allowed us to make those cards work. - the register read returning 0. Looking at the current code, it looks like the 0xb2 register is supposed to mean that a firmware is loaded. This case is fairly common: we've had many cards randomly saying that a firmware was loaded when none had been. Often, a simple reboot will do the trick. But sometimes, forcing the firmware upload (ie, bypassing the 0xb2 register check) allows the stubborn cards to function properly. > Original behavior to expect firmware is loaded and running when register > read fails is very stupid and your fix seems much better. Well, this patch should not really change the behavior. It just propagates register read errors to ds3000_initfe(). It'll just fail earlier. > So first priority should be try fix that issue with register read. Is it > coming from the USB stack (eg. error 110 timeout) or some other error > coming from the fact chip answers wrong? The cards we're using are PCIe (and not the ones with an embedded USB controller). > Do you see other register I/O failing too? I'll see if I can get you an answer for that, since the cards are shipped with the appliance we send to our customers. Remote debugging is somewhat tricky. > Does adding few usec sleep help? I'm not quite sure where to add those sleeps. In the register reading/writing functions? 10us? 100us? Many thanks Rémi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/30/2012 06:21 PM, Rémi Cardona wrote: > Hi Antti, > > On 08/30/2012 03:39 PM, Antti Palosaari wrote: >> As I understand firmware downloading failure is coming from the fact >> that register read fails => fails to detect if firmware is already >> running or not. > > Well we actually see 2 cases: > > - the register read failure (when ds3000_readreg() returns negative > values). This case is fairly rare, and no changes we've done to the > driver allowed us to make those cards work. hmm, looks like ds3000_readreg() logic is still a little bit broken. It checks count of sent messages and compares it to 2. But if I2C-adapter sends only 1 message or 3 (which should not be possible) function return that count instead of -EREMOTEIO. OK, quite rare situation, but one point more to fail if I2C-adapter has also bug. But that happens for return value 0 too. Could it be the issue? I2C-adapter returns 0 for some reason? Bug in I2C-adapter with bug in ds3000_readreg() implementation? > - the register read returning 0. Looking at the current code, it looks > like the 0xb2 register is supposed to mean that a firmware is loaded. > This case is fairly common: we've had many cards randomly saying that a > firmware was loaded when none had been. Often, a simple reboot will do > the trick. But sometimes, forcing the firmware upload (ie, bypassing the > 0xb2 register check) allows the stubborn cards to function properly. > >> Original behavior to expect firmware is loaded and running when register >> read fails is very stupid and your fix seems much better. > > Well, this patch should not really change the behavior. It just > propagates register read errors to ds3000_initfe(). It'll just fail earlier. > >> So first priority should be try fix that issue with register read. Is it >> coming from the USB stack (eg. error 110 timeout) or some other error >> coming from the fact chip answers wrong? > > The cards we're using are PCIe (and not the ones with an embedded USB > controller). The idea of my question was to ask where those errors are coming from (I spoke mistakenly about USB because I usually play with USB devices). You basically see two different kind of errors, 1) bus communication fails, eg. usb timeouts. 2) chips returns error status. Later cases the error could come from the this could come from the firmware if chip uses firmware or from the silicon. It could be from the I2C-adapter firmware. >> Do you see other register I/O failing too? > > I'll see if I can get you an answer for that, since the cards are > shipped with the appliance we send to our customers. Remote debugging is > somewhat tricky. > >> Does adding few usec sleep help? > > I'm not quite sure where to add those sleeps. In the register > reading/writing functions? 10us? 100us? Add sleep after the each operation. Good place to add sleep is I2C-adapter. I2C-adapters usually supports two different operations, write and read + write using repeated START condition. Former us used typically for register write and later for register read. 500us is good choice. If it is only that one register read which causes problems, how about repeating it? > > Many thanks > > Rémi >
On 08/30/2012 06:00 PM, Antti Palosaari wrote: > hmm, looks like ds3000_readreg() logic is still a little bit broken. It > checks count of sent messages and compares it to 2. But if I2C-adapter > sends only 1 message or 3 (which should not be possible) function return > that count instead of -EREMOTEIO. OK, quite rare situation, but one > point more to fail if I2C-adapter has also bug. You're absolutely right. The logic is completely awkward. I'm currently working on a new patch series that will clean up all the register reading/writing functions to properly check i2c_transfer()'s return code. > But that happens for return value 0 too. Could it be the issue? > I2C-adapter returns 0 for some reason? Bug in I2C-adapter with bug in > ds3000_readreg() implementation? [...] > You basically see two different kind of errors, 1) bus communication > fails, eg. usb timeouts. 2) chips returns error status. Later cases the > error could come from the this could come from the firmware if chip uses > firmware or from the silicon. It could be from the I2C-adapter firmware. Right now, I just have no idea. Right now, the overwhelming majority of the cards we tested with a recent kernel (about 60 units) are working just fine with the code as-is. My main (if not sole) objective for now is to make it easier for us (but hopefully for others too) to find defective cards/chips. > Add sleep after the each operation. Good place to add sleep is > I2C-adapter. I2C-adapters usually supports two different operations, > write and read + write using repeated START condition. Former us used > typically for register write and later for register read. > > 500us is good choice. If it is only that one register read which causes > problems, how about repeating it? I'll first see how our units are behaving with the upcoming patches but I'll definitely keep this in mind as a next step. Thanks again, Rémi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Max, On 08/31/2012 10:29 AM, nibble.max wrote: > As remember that there is a fault in the tuner register read function in ds3000.c file. > It will cause the read back value wrong. Well, using 0x12 works out for most of the cards we have in the wild. Not knowing what 0x11 / 0x12 means, I'd be wary of suggesting such a change myself. Once I isolate the faulty cards, I'll try using 0x11 on them to see if it changes anything. Thanks for the pointer, Rémi -- To unsubscribe from this list: send the line "unsubscribe linux-media" 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/media/dvb/frontends/ds3000.c b/drivers/media/dvb/frontends/ds3000.c index 066870a..4c774c4 100644 --- a/drivers/media/dvb/frontends/ds3000.c +++ b/drivers/media/dvb/frontends/ds3000.c @@ -391,8 +391,14 @@ static int ds3000_firmware_ondemand(struct dvb_frontend *fe) dprintk("%s()\n", __func__); - if (ds3000_readreg(state, 0xb2) <= 0) + ret = ds3000_readreg(state, 0xb2); + if (ret == 0) { + printk(KERN_INFO "%s: Firmware already uploaded, skipping\n", + __func__); return ret; + } else if (ret < 0) { + return ret; + } /* Load firmware */ /* request the firmware, this will block until someone uploads it */
ds3000_readreg() returns negative values in case of i2c failures. The old code would simply return 0 when failing to read the 0xb2 register, misleading ds3000_initfe() into believing that the firmware had been correctly loaded. Signed-off-by: Rémi Cardona <remi.cardona@smartjog.com> --- drivers/media/dvb/frontends/ds3000.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)