diff mbox

[v4,16/22,media] em28xx: use a better value for I2C timeouts

Message ID 1388832951-11195-17-git-send-email-m.chehab@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mauro Carvalho Chehab Jan. 4, 2014, 10:55 a.m. UTC 
In the lack of a better spec, let's assume the timeout
values compatible with SMBus spec:
	http://smbus.org/specs/smbus110.pdf

at chapter 8 - Electrical Characteristics of SMBus devices

Ok, SMBus is a subset of I2C, and not all devices will be
following it, but the timeout value before this patch was not
even following the spec.

So, while we don't have a better guess for it, use 35 + 1
ms as the timeout.

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/usb/em28xx/em28xx.h | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Frank Schäfer Jan. 5, 2014, 8:38 p.m. UTC | #1 
Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab:
> In the lack of a better spec, let's assume the timeout
> values compatible with SMBus spec:
> 	http://smbus.org/specs/smbus110.pdf
>
> at chapter 8 - Electrical Characteristics of SMBus devices
>
> Ok, SMBus is a subset of I2C, and not all devices will be
> following it, but the timeout value before this patch was not
> even following the spec.
>
> So, while we don't have a better guess for it, use 35 + 1
> ms as the timeout.
>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> ---
>  drivers/media/usb/em28xx/em28xx.h | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index db47c2236ca4..9af19332b0f1 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -183,8 +183,21 @@
>  
>  #define EM28XX_INTERLACED_DEFAULT 1
>  
> -/* time in msecs to wait for i2c xfers to finish */
> -#define EM2800_I2C_XFER_TIMEOUT		20
> +/*
> + * Time in msecs to wait for i2c xfers to finish.
> + * 35ms is the maximum time a SMBUS device could wait when
> + * clock stretching is used. As the transfer itself will take
> + * some time to happen, set it to 35 ms.
> + *
> + * Ok, I2C doesn't specify any limit. So, eventually, we may need
> + * to increase this timeout.
> + *
> + * FIXME: this assumes that an I2C message is not longer than 1ms.
> + * This is actually dependent on the I2C bus speed, although most
> + * devices use a 100kHz clock. So, this assumtion is true most of
> + * the time.
> + */
> +#define EM2800_I2C_XFER_TIMEOUT		36
>  
>  /* time in msecs to wait for AC97 xfers to finish */
>  #define EM2800_AC97_XFER_TIMEOUT	100
Mauro...
What exactly are you fixing with this patch ?
Which devices are not working with the current timeout value ?

You really shouldn't increase the timout to 172% for all devices based
on such a fragile pure theory.

NACK.



--
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
Mauro Carvalho Chehab Jan. 5, 2014, 8:57 p.m. UTC | #2 
Em Sun, 05 Jan 2014 21:38:31 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab:
> > In the lack of a better spec, let's assume the timeout
> > values compatible with SMBus spec:
> > 	http://smbus.org/specs/smbus110.pdf
> >
> > at chapter 8 - Electrical Characteristics of SMBus devices
> >
> > Ok, SMBus is a subset of I2C, and not all devices will be
> > following it, but the timeout value before this patch was not
> > even following the spec.
> >
> > So, while we don't have a better guess for it, use 35 + 1
> > ms as the timeout.
> >
> > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > ---
> >  drivers/media/usb/em28xx/em28xx.h | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> > index db47c2236ca4..9af19332b0f1 100644
> > --- a/drivers/media/usb/em28xx/em28xx.h
> > +++ b/drivers/media/usb/em28xx/em28xx.h
> > @@ -183,8 +183,21 @@
> >  
> >  #define EM28XX_INTERLACED_DEFAULT 1
> >  
> > -/* time in msecs to wait for i2c xfers to finish */
> > -#define EM2800_I2C_XFER_TIMEOUT		20
> > +/*
> > + * Time in msecs to wait for i2c xfers to finish.
> > + * 35ms is the maximum time a SMBUS device could wait when
> > + * clock stretching is used. As the transfer itself will take
> > + * some time to happen, set it to 35 ms.
> > + *
> > + * Ok, I2C doesn't specify any limit. So, eventually, we may need
> > + * to increase this timeout.
> > + *
> > + * FIXME: this assumes that an I2C message is not longer than 1ms.
> > + * This is actually dependent on the I2C bus speed, although most
> > + * devices use a 100kHz clock. So, this assumtion is true most of
> > + * the time.
> > + */
> > +#define EM2800_I2C_XFER_TIMEOUT		36
> >  
> >  /* time in msecs to wait for AC97 xfers to finish */
> >  #define EM2800_AC97_XFER_TIMEOUT	100
> Mauro...
> What exactly are you fixing with this patch ?

It fixes some of the timeouts I noticed here with HVR-950.

> Which devices are not working with the current timeout value ?
> 
> You really shouldn't increase the timout to 172% for all devices based
> on such a fragile pure theory.

It is not fragile. It is the SMBUS spec. It should _at_least_ wait up to
the timeout specified there.

Btw, it is not increasing the timeout. It is actually reducing it.

See, this is the code before the patch:

for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0;
	     read_timeout -= 5) {
		ret = dev->em28xx_read_reg(dev, 0x05);
		if (ret == 0x84 + len - 1) {
			break;
		} else if (ret == 0x94 + len - 1) {
			return -ENODEV;
		} else if (ret < 0) {
			em28xx_warn("failed to get i2c transfer status from bridge register (error=%i)\n",
				    ret);
			return ret;
		}
		msleep(5);
	}

msleep(5) actually sleeps up to 20 ms, as the minimal time is the
schedule() time - being 10 ms a typical value (CONFIG_HZ equal to 100). 

So, the current code has a timeout of up to 100 ms.

This patch is actually reducing from 100 ms to 36 ms.

Regards,
Mauro
--
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
Frank Schäfer Jan. 7, 2014, 5:15 p.m. UTC | #3 
Am 05.01.2014 21:57, schrieb Mauro Carvalho Chehab:
> Em Sun, 05 Jan 2014 21:38:31 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab:
>>> In the lack of a better spec, let's assume the timeout
>>> values compatible with SMBus spec:
>>> 	http://smbus.org/specs/smbus110.pdf
>>>
>>> at chapter 8 - Electrical Characteristics of SMBus devices
>>>
>>> Ok, SMBus is a subset of I2C, and not all devices will be
>>> following it, but the timeout value before this patch was not
>>> even following the spec.
>>>
>>> So, while we don't have a better guess for it, use 35 + 1
>>> ms as the timeout.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
>>> ---
>>>  drivers/media/usb/em28xx/em28xx.h | 17 +++++++++++++++--
>>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>>> index db47c2236ca4..9af19332b0f1 100644
>>> --- a/drivers/media/usb/em28xx/em28xx.h
>>> +++ b/drivers/media/usb/em28xx/em28xx.h
>>> @@ -183,8 +183,21 @@
>>>  
>>>  #define EM28XX_INTERLACED_DEFAULT 1
>>>  
>>> -/* time in msecs to wait for i2c xfers to finish */
>>> -#define EM2800_I2C_XFER_TIMEOUT		20
>>> +/*
>>> + * Time in msecs to wait for i2c xfers to finish.
>>> + * 35ms is the maximum time a SMBUS device could wait when
>>> + * clock stretching is used. As the transfer itself will take
>>> + * some time to happen, set it to 35 ms.
>>> + *
>>> + * Ok, I2C doesn't specify any limit. So, eventually, we may need
>>> + * to increase this timeout.
>>> + *
>>> + * FIXME: this assumes that an I2C message is not longer than 1ms.
>>> + * This is actually dependent on the I2C bus speed, although most
>>> + * devices use a 100kHz clock. So, this assumtion is true most of
>>> + * the time.
>>> + */
>>> +#define EM2800_I2C_XFER_TIMEOUT		36
>>>  
>>>  /* time in msecs to wait for AC97 xfers to finish */
>>>  #define EM2800_AC97_XFER_TIMEOUT	100
>> Mauro...
>> What exactly are you fixing with this patch ?
> It fixes some of the timeouts I noticed here with HVR-950.
>
>> Which devices are not working with the current timeout value ?
>>
>> You really shouldn't increase the timout to 172% for all devices based
>> on such a fragile pure theory.
> It is not fragile. It is the SMBUS spec. It should _at_least_ wait up to
> the timeout specified there.
>
> Btw, it is not increasing the timeout. It is actually reducing it.
>
> See, this is the code before the patch:
>
> for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0;
> 	     read_timeout -= 5) {
> 		ret = dev->em28xx_read_reg(dev, 0x05);
> 		if (ret == 0x84 + len - 1) {
> 			break;
> 		} else if (ret == 0x94 + len - 1) {
> 			return -ENODEV;
> 		} else if (ret < 0) {
> 			em28xx_warn("failed to get i2c transfer status from bridge register (error=%i)\n",
> 				    ret);
> 			return ret;
> 		}
> 		msleep(5);
> 	}
>
> msleep(5) actually sleeps up to 20 ms, as the minimal time is the
> schedule() time - being 10 ms a typical value (CONFIG_HZ equal to 100). 
>
> So, the current code has a timeout of up to 100 ms.
20ms / 5ms = 4.
4 * 10ms = 40ms ?

> This patch is actually reducing from 100 ms to 36 ms.
Ok, it's the same as with AC97 reads/writes.
I would accept any value from 20ms to 40ms.

> Regards,
> Mauro

--
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
Mauro Carvalho Chehab Jan. 8, 2014, 2:39 p.m. UTC | #4 
Em Tue, 07 Jan 2014 18:15:38 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 05.01.2014 21:57, schrieb Mauro Carvalho Chehab:
> > Em Sun, 05 Jan 2014 21:38:31 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab:
> >>> In the lack of a better spec, let's assume the timeout
> >>> values compatible with SMBus spec:
> >>> 	http://smbus.org/specs/smbus110.pdf
> >>>
> >>> at chapter 8 - Electrical Characteristics of SMBus devices
> >>>
> >>> Ok, SMBus is a subset of I2C, and not all devices will be
> >>> following it, but the timeout value before this patch was not
> >>> even following the spec.
> >>>
> >>> So, while we don't have a better guess for it, use 35 + 1
> >>> ms as the timeout.
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> >>> ---
> >>>  drivers/media/usb/em28xx/em28xx.h | 17 +++++++++++++++--
> >>>  1 file changed, 15 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> >>> index db47c2236ca4..9af19332b0f1 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx.h
> >>> +++ b/drivers/media/usb/em28xx/em28xx.h
> >>> @@ -183,8 +183,21 @@
> >>>  
> >>>  #define EM28XX_INTERLACED_DEFAULT 1
> >>>  
> >>> -/* time in msecs to wait for i2c xfers to finish */
> >>> -#define EM2800_I2C_XFER_TIMEOUT		20
> >>> +/*
> >>> + * Time in msecs to wait for i2c xfers to finish.
> >>> + * 35ms is the maximum time a SMBUS device could wait when
> >>> + * clock stretching is used. As the transfer itself will take
> >>> + * some time to happen, set it to 35 ms.
> >>> + *
> >>> + * Ok, I2C doesn't specify any limit. So, eventually, we may need
> >>> + * to increase this timeout.
> >>> + *
> >>> + * FIXME: this assumes that an I2C message is not longer than 1ms.
> >>> + * This is actually dependent on the I2C bus speed, although most
> >>> + * devices use a 100kHz clock. So, this assumtion is true most of
> >>> + * the time.
> >>> + */
> >>> +#define EM2800_I2C_XFER_TIMEOUT		36
> >>>  
> >>>  /* time in msecs to wait for AC97 xfers to finish */
> >>>  #define EM2800_AC97_XFER_TIMEOUT	100
> >> Mauro...
> >> What exactly are you fixing with this patch ?
> > It fixes some of the timeouts I noticed here with HVR-950.
> >
> >> Which devices are not working with the current timeout value ?
> >>
> >> You really shouldn't increase the timout to 172% for all devices based
> >> on such a fragile pure theory.
> > It is not fragile. It is the SMBUS spec. It should _at_least_ wait up to
> > the timeout specified there.
> >
> > Btw, it is not increasing the timeout. It is actually reducing it.
> >
> > See, this is the code before the patch:
> >
> > for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0;
> > 	     read_timeout -= 5) {
> > 		ret = dev->em28xx_read_reg(dev, 0x05);
> > 		if (ret == 0x84 + len - 1) {
> > 			break;
> > 		} else if (ret == 0x94 + len - 1) {
> > 			return -ENODEV;
> > 		} else if (ret < 0) {
> > 			em28xx_warn("failed to get i2c transfer status from bridge register (error=%i)\n",
> > 				    ret);
> > 			return ret;
> > 		}
> > 		msleep(5);
> > 	}
> >
> > msleep(5) actually sleeps up to 20 ms, as the minimal time is the
> > schedule() time - being 10 ms a typical value (CONFIG_HZ equal to 100). 
> >
> > So, the current code has a timeout of up to 100 ms.
> 20ms / 5ms = 4.
> 4 * 10ms = 40ms ?

Yes. Actually the worse case is 20ms, as one could have manually 50 for
HZ, but, as distros either use 100 or 1000 for CONFIG_HZ, it seems safe
enough to assume that 10ms is the real worse case scenario for the
sleep time given by msleep(5).

> 
> > This patch is actually reducing from 100 ms to 36 ms.
> Ok, it's the same as with AC97 reads/writes.

Yes.

> I would accept any value from 20ms to 40ms.

36ms seems good enough, at least as an start point, as it is the maximum
time given by SMBUS spec.

It should be noticed that any I2C device that doesn't follow SMBUS
spec will likely have a much higher timeout, as said at
Documentation/i2c/fault-codes:

ETIMEDOUT
	This is returned by drivers when an operation took too much
	time, and was aborted before it completed.

	SMBus adapters may return it when an operation took more
	time than allowed by the SMBus specification; for example,
	when a slave stretches clocks too far.  I2C has no such
	timeouts, but it's normal for I2C adapters to impose some
	arbitrary limits (much longer than SMBus!) too.

That means that we might need to increase the timeout in order to
support I2C adapters that use stretching and are not SMBUS compliant.

Regards,
Mauro
--
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 mbox

Patch

diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index db47c2236ca4..9af19332b0f1 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -183,8 +183,21 @@ 
 
 #define EM28XX_INTERLACED_DEFAULT 1
 
-/* time in msecs to wait for i2c xfers to finish */
-#define EM2800_I2C_XFER_TIMEOUT		20
+/*
+ * Time in msecs to wait for i2c xfers to finish.
+ * 35ms is the maximum time a SMBUS device could wait when
+ * clock stretching is used. As the transfer itself will take
+ * some time to happen, set it to 35 ms.
+ *
+ * Ok, I2C doesn't specify any limit. So, eventually, we may need
+ * to increase this timeout.
+ *
+ * FIXME: this assumes that an I2C message is not longer than 1ms.
+ * This is actually dependent on the I2C bus speed, although most
+ * devices use a 100kHz clock. So, this assumtion is true most of
+ * the time.
+ */
+#define EM2800_I2C_XFER_TIMEOUT		36
 
 /* time in msecs to wait for AC97 xfers to finish */
 #define EM2800_AC97_XFER_TIMEOUT	100