[PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Antti Palosaari]

New drxk firmware download does not work with tda18271. Actual
reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
will work as it does not do as much I/O during attach than tda18271.

Root of cause is tuner I/O during drx-k asynchronous firmware
download. request_firmware_nowait()... :-/

Cc: Michael Krufky <mkrufky@linuxtv.org>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/usb/em28xx/em28xx-dvb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
index 770a5af..fd750d4 100644
--- a/drivers/media/usb/em28xx/em28xx-dvb.c
+++ b/drivers/media/usb/em28xx/em28xx-dvb.c
@@ -1122,9 +1122,8 @@ static int em28xx_dvb_init(struct em28xx *dev)
 
 		if (dvb->fe[0]) {
 			/* attach tuner */
-			if (!dvb_attach(tda18271_attach, dvb->fe[0], 0x60,
-					&dev->i2c_adap,
-					&em28xx_cxd2820r_tda18271_config)) {
+			if (!dvb_attach(tda18271c2dd_attach, dvb->fe[0],
+					&dev->i2c_adap, 0x60)) {
 				dvb_frontend_detach(dvb->fe[0]);
 				result = -EINVAL;
 				goto out_free;
-- 
1.7.11.4

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Michael Krufky]

On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari <crope@iki.fi> wrote:
> New drxk firmware download does not work with tda18271. Actual
> reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
> will work as it does not do as much I/O during attach than tda18271.
>
> Root of cause is tuner I/O during drx-k asynchronous firmware
> download. request_firmware_nowait()... :-/
>
> Cc: Michael Krufky <mkrufky@linuxtv.org>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  drivers/media/usb/em28xx/em28xx-dvb.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> index 770a5af..fd750d4 100644
> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> @@ -1122,9 +1122,8 @@ static int em28xx_dvb_init(struct em28xx *dev)
>
>                 if (dvb->fe[0]) {
>                         /* attach tuner */
> -                       if (!dvb_attach(tda18271_attach, dvb->fe[0], 0x60,
> -                                       &dev->i2c_adap,
> -                                       &em28xx_cxd2820r_tda18271_config)) {
> +                       if (!dvb_attach(tda18271c2dd_attach, dvb->fe[0],
> +                                       &dev->i2c_adap, 0x60)) {
>                                 dvb_frontend_detach(dvb->fe[0]);
>                                 result = -EINVAL;
>                                 goto out_free;
> --
> 1.7.11.4
>


utterly ridiculous.  I understand why Antti is making this patch, so I
cannot blame him for it, but this whole idea of asynchronous firmware
load instead of allowing the bridge driver to orchestrate things is a
major problem -- THAT is what needs fixing.  let's fix the ACTUAL
problem.

(if we have to merge this for the short-term, i understand... i just
reiterate - we set a horrible president by merging a second tda18271
driver)

-Mike

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Mauro Carvalho Chehab]

Em Mon, 1 Oct 2012 21:43:51 -0400
Michael Krufky <mkrufky@linuxtv.org> escreveu:

> On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari <crope@iki.fi> wrote:
> > New drxk firmware download does not work with tda18271. Actual
> > reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
> > will work as it does not do as much I/O during attach than tda18271.
> >
> > Root of cause is tuner I/O during drx-k asynchronous firmware
> > download. request_firmware_nowait()... :-/
> >
> > Cc: Michael Krufky <mkrufky@linuxtv.org>
> > Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> > Signed-off-by: Antti Palosaari <crope@iki.fi>
> > ---
> >  drivers/media/usb/em28xx/em28xx-dvb.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
> > index 770a5af..fd750d4 100644
> > --- a/drivers/media/usb/em28xx/em28xx-dvb.c
> > +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
> > @@ -1122,9 +1122,8 @@ static int em28xx_dvb_init(struct em28xx *dev)
> >
> >                 if (dvb->fe[0]) {
> >                         /* attach tuner */
> > -                       if (!dvb_attach(tda18271_attach, dvb->fe[0], 0x60,
> > -                                       &dev->i2c_adap,
> > -                                       &em28xx_cxd2820r_tda18271_config)) {
> > +                       if (!dvb_attach(tda18271c2dd_attach, dvb->fe[0],
> > +                                       &dev->i2c_adap, 0x60)) {
> >                                 dvb_frontend_detach(dvb->fe[0]);
> >                                 result = -EINVAL;
> >                                 goto out_free;
> > --
> > 1.7.11.4
> >
> 
> 
> utterly ridiculous.  I understand why Antti is making this patch, so I
> cannot blame him for it, but this whole idea of asynchronous firmware
> load instead of allowing the bridge driver to orchestrate things is a
> major problem -- THAT is what needs fixing.  let's fix the ACTUAL
> problem.
> 
> (if we have to merge this for the short-term, i understand... i just
> reiterate - we set a horrible president by merging a second tda18271
> driver)

It is not ridiculous: this bug affects Kernel 3.6, so it needs a quick fix.
This is very likely the quickest clean fix for this issue, and changes
only one statement.

So, it can be applied without any hash upstream.

After having this fix applied, a better solution can be developed for Kernel
3.8 (as it is likely too late for 3.7).

Regards,
Mauro

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Devin Heitmueller]

On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari <crope@iki.fi> wrote:
> New drxk firmware download does not work with tda18271. Actual
> reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
> will work as it does not do as much I/O during attach than tda18271.
>
> Root of cause is tuner I/O during drx-k asynchronous firmware
> download. request_firmware_nowait()... :-/

This seems like it's just changing the timing of the initialization
process, which isn't really any better than the "msleep(2000)".  It's
just dumb luck that it happens to work on the developer's system.

Don't get me wrong, I agree with Michael that this whole situation is
ridiculous, but I don't see why swapping out the entire driver is a
reasonable fix.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[PATCH] tda18271: prevent register access during attach() if delay_cal is set

[Michael Krufky]

if the configuration option 'delay_cal' is set, prevent any register
access during attach(), delaying both IR & RF calibration until init()

both module option 'cal' or configuration option 'rf_cal_on_startup'
will override this delay. it makes no sense to mix 'delay_cal' with
'rf_cal_on_startup' as these options conflict with each other.

Signed-off-by: Michael Krufky <mkrufky@linuxtv.org>
---
 drivers/media/tuners/tda18271-fe.c |    5 +++++
 drivers/media/tuners/tda18271.h    |    5 +++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/media/tuners/tda18271-fe.c b/drivers/media/tuners/tda18271-fe.c
index de21197..ca202da 100644
--- a/drivers/media/tuners/tda18271-fe.c
+++ b/drivers/media/tuners/tda18271-fe.c
@@ -1278,6 +1278,11 @@ struct dvb_frontend *tda18271_attach(struct dvb_frontend *fe, u8 addr,
 		if (tda_fail(ret))
 			goto fail;
 
+		/* if delay_cal is set, delay IR & RF calibration until init()
+		 * module option 'cal' overrides this delay */
+		if ((cfg->delay_cal) && (!tda18271_need_cal_on_startup(cfg)))
+			break;
+
 		mutex_lock(&priv->lock);
 		tda18271_init_regs(fe);
 
diff --git a/drivers/media/tuners/tda18271.h b/drivers/media/tuners/tda18271.h
index 640bae4..89b6c6d 100644
--- a/drivers/media/tuners/tda18271.h
+++ b/drivers/media/tuners/tda18271.h
@@ -105,6 +105,11 @@ struct tda18271_config {
 	/* force rf tracking filter calibration on startup */
 	unsigned int rf_cal_on_startup:1;
 
+	/* prevent any register access during attach(),
+	 * delaying both IR & RF calibration until init()
+	 * module option 'cal' overrides this delay */
+	unsigned int delay_cal:1;
+
 	/* interface to saa713x / tda829x */
 	unsigned int config;
 };
-- 
1.7.9.5

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Michael Krufky]

On Mon, Oct 1, 2012 at 9:58 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari <crope@iki.fi> wrote:
>> New drxk firmware download does not work with tda18271. Actual
>> reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
>> will work as it does not do as much I/O during attach than tda18271.
>>
>> Root of cause is tuner I/O during drx-k asynchronous firmware
>> download. request_firmware_nowait()... :-/
>
> This seems like it's just changing the timing of the initialization
> process, which isn't really any better than the "msleep(2000)".  It's
> just dumb luck that it happens to work on the developer's system.
>
> Don't get me wrong, I agree with Michael that this whole situation is
> ridiculous, but I don't see why swapping out the entire driver is a
> reasonable fix.

I just send out a patch entitled, "tda18271: prevent register access
during attach() if delay_cal is set"   Antti, could you set
tda18271_config.delay_cal = 1 with this patch applied and see if it
solves your problem?

Again, although this may solve the problem for this particular device,
the *real* problem is this asynchronous firmware download in the demod
driver.

Nonetheless, Antti has been asking for this feature, to not allow
register access during attach, I was against it and I have my reasons,
but I believe that this patch is a fair compromise.

After somebody can test it, I think we should merge this -- any comments?

http://patchwork.linuxtv.org/patch/14799/

Regards,

Mike Krufky

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Antti Palosaari]

On 10/02/2012 06:17 AM, Michael Krufky wrote:
> On Mon, Oct 1, 2012 at 9:58 PM, Devin Heitmueller
> <dheitmueller@kernellabs.com> wrote:
>> On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari <crope@iki.fi> wrote:
>>> New drxk firmware download does not work with tda18271. Actual
>>> reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
>>> will work as it does not do as much I/O during attach than tda18271.
>>>
>>> Root of cause is tuner I/O during drx-k asynchronous firmware
>>> download. request_firmware_nowait()... :-/
>>
>> This seems like it's just changing the timing of the initialization
>> process, which isn't really any better than the "msleep(2000)".  It's
>> just dumb luck that it happens to work on the developer's system.
>>
>> Don't get me wrong, I agree with Michael that this whole situation is
>> ridiculous, but I don't see why swapping out the entire driver is a
>> reasonable fix.
>
> I just send out a patch entitled, "tda18271: prevent register access
> during attach() if delay_cal is set"   Antti, could you set
> tda18271_config.delay_cal = 1 with this patch applied and see if it
> solves your problem?
>
> Again, although this may solve the problem for this particular device,
> the *real* problem is this asynchronous firmware download in the demod
> driver.
>
> Nonetheless, Antti has been asking for this feature, to not allow
> register access during attach, I was against it and I have my reasons,
> but I believe that this patch is a fair compromise.
>
> After somebody can test it, I think we should merge this -- any comments?
>
> http://patchwork.linuxtv.org/patch/14799/

I tested. It does not help. I also looked it more and it really bails 
out with error much earlier, in function where it reads chip ID. That 
makes me look the tda18271c2dd driver. I found that for some reason 
these drivers uses different method for register read. tda18271 uses I2C 
transaction with 2 messages, write and read with REPEATED START 
condition. tda18271c2dd driver is just simple I2C read. So which one is 
correct?

Also other note. tda18271c2dd does not have almost any error logging. 
Only error log is failed I2C write. So it could be even possible 
tda18271c2dd fails too, but as it keeps silence and discards all the 
error I don't see it and it even works :S

And 3rd issue. It crashes. Very often. I didn't take picture anymore as 
I have taken earlier. I am so f***ing pissed off all the long time 
problems with that em28xx driver! It has crashed more than any other 
driver I have ever seen. It is really, really, problematic. The amount 
of time what I have loosen em28xx driver problems when hacking with 
relatively small amount of devices is huge. It is surely more time that 
it will take me to write whole driver from the scratch using DVB USB. As 
I cannot trust em28xx correctness it is very hard to debug these 
crashes. That one seems to come from drxk, but is it really?

http://palosaari.fi/linux/v4l-dvb/em28xx_drxk_crash/

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Mauro Carvalho Chehab]

Em Tue, 02 Oct 2012 12:55:55 +0300
Antti Palosaari <crope@iki.fi> escreveu:

> On 10/02/2012 06:17 AM, Michael Krufky wrote:
> > On Mon, Oct 1, 2012 at 9:58 PM, Devin Heitmueller
> > <dheitmueller@kernellabs.com> wrote:
> >> On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari <crope@iki.fi> wrote:
> >>> New drxk firmware download does not work with tda18271. Actual
> >>> reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
> >>> will work as it does not do as much I/O during attach than tda18271.
> >>>
> >>> Root of cause is tuner I/O during drx-k asynchronous firmware
> >>> download. request_firmware_nowait()... :-/
> >>
> >> This seems like it's just changing the timing of the initialization
> >> process, which isn't really any better than the "msleep(2000)".  It's
> >> just dumb luck that it happens to work on the developer's system.
> >>
> >> Don't get me wrong, I agree with Michael that this whole situation is
> >> ridiculous, but I don't see why swapping out the entire driver is a
> >> reasonable fix.
> >
> > I just send out a patch entitled, "tda18271: prevent register access
> > during attach() if delay_cal is set"   Antti, could you set
> > tda18271_config.delay_cal = 1 with this patch applied and see if it
> > solves your problem?
> >
> > Again, although this may solve the problem for this particular device,
> > the *real* problem is this asynchronous firmware download in the demod
> > driver.
> >
> > Nonetheless, Antti has been asking for this feature, to not allow
> > register access during attach, I was against it and I have my reasons,
> > but I believe that this patch is a fair compromise.
> >
> > After somebody can test it, I think we should merge this -- any comments?
> >
> > http://patchwork.linuxtv.org/patch/14799/
> 
> I tested. It does not help. I also looked it more and it really bails 
> out with error much earlier, in function where it reads chip ID. That 
> makes me look the tda18271c2dd driver. 

I saw Antti's logs: basically, tda18271_get_id() reads all registers at the
chip during attach(), returning -EINVAL if tda18271_read_regs(fe) can't
read the value for R_ID register.

Btw, why do you need to read 16 registers at once, instead of just reading
the needed register? read_extended and write operations are even more evil:
they read/write the full set of 39 registers on each operation. That seems
to be overkill, especially on places like tda18271_get_id(), where
all the driver is doing is to check for the ID register.

Worse than that, tda18271_get_id() doesn't even check if the read()
operation failed: it assumes that it will always work, letting the
switch(regs[R_ID]) to print a wrong message (device unknown) when
what actually failed where the 16 registers dump.

> I found that for some reason 
> these drivers uses different method for register read. tda18271 uses I2C 
> transaction with 2 messages, write and read with REPEATED START 
> condition. tda18271c2dd driver is just simple I2C read. So which one is 
> correct?

That's due to the I2C locking schema: if you do two separate I2C
transfers, the I2C core will allow an event to happen between the
two operations. That typically causes troubles on read operations.
So, it is recommended to use just one i2c_transfer() call for read
operations that are mapped via a write and a read.

> 
> Also other note. tda18271c2dd does not have almost any error logging. 
> Only error log is failed I2C write. So it could be even possible 
> tda18271c2dd fails too, but as it keeps silence and discards all the 
> error I don't see it and it even works :S

I don't think this is the case. The tda18271c2dd driver is just for version
v2 of the silicon, while tda18271 supports both v1 and v2. As there are
differences between them, tda18271 needs to read the chip version, in
order to adjust a few internal settings.

Of course, before making the driver available, tda18271_get_id() needs
to be called and to successfully return the chip ID.

Whenever it should be at attach() or later is a good point for discussions.

> And 3rd issue. It crashes. Very often. I didn't take picture anymore as 
> I have taken earlier. I am so f***ing pissed off all the long time 
> problems with that em28xx driver! It has crashed more than any other 
> driver I have ever seen. It is really, really, problematic. The amount 
> of time what I have loosen em28xx driver problems when hacking with 
> relatively small amount of devices is huge. It is surely more time that 
> it will take me to write whole driver from the scratch using DVB USB. 

For sure rewriting it into dvb-usb will take a lot more time: these
chipsets are lot more complex than pure DVB ones, due to the analog part
and audio part. Also, there are lots of supported devices on it, each with
their own particular configuration, and there are a few different options
on how remote controllers and audio work there.

It is weird that you're experiencing so many issues on this driver.
One thing that may help you during development is to not compile the
remote controller and the alsa drivers. Removing an alsa driver during
development can be a pain, if you have pulseaudio running, as it will
keep the audio opened forever, preventing driver removal. So, you need
first to tell pulseaudio to unload the driver module before being able
to remove it, by using a proper pacmd/pactl enchant.

> As 
> I cannot trust em28xx correctness it is very hard to debug these 
> crashes. That one seems to come from drxk, but is it really?
> 
> http://palosaari.fi/linux/v4l-dvb/em28xx_drxk_crash/

Not sure how you work, but I suspect you're not using a serial console.
The best thing you can do for yourself, when developing drivers, is to buy
or wire a cross serial cable and use it. That saves you a lot time when
you get crashes.

It is hard to analyze that log without actually knowing what you were
doing, but the above log could even be caused by your 2 seconds delay ;)
I suspect that request_firmware callback call got delayed for a long time
and maybe you asked to remove the driver before the callback is called.
So, at the time i2c_lock_adapter() is called, there's no more I2C bus
there. That's my initial guess. Ok, if I'm right, there's a real bug at
the driver (or at I2C core), as it shouldn't be allowed to unregister
the I2C bus while there are some deferred work there.

Regards,
Mauro

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Michael Krufky]

On Tue, Oct 2, 2012 at 7:05 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Btw, why do you need to read 16 registers at once, instead of just reading
> the needed register? read_extended and write operations are even more evil:
> they read/write the full set of 39 registers on each operation. That seems
> to be overkill, especially on places like tda18271_get_id(), where
> all the driver is doing is to check for the ID register.

TDA18271 does not support subaddressing for read operations.  The only
way to read a register is by dumping full register contents.  16
registers in simple mode, 39 registers in extended mode.

> Worse than that, tda18271_get_id() doesn't even check if the read()
> operation failed: it assumes that it will always work, letting the
> switch(regs[R_ID]) to print a wrong message (device unknown) when
> what actually failed where the 16 registers dump.

That's a pretty standard operation to be able to read a chip's ID in
its driver attach function.  You even have some drivers that continue
trying to attach frontends and tuners as long as they continue to get
an error in the attach() function.  If we dont read the chip's ID
during attach() then how do we know we're attaching to the correct
chip?

I'll look at the fact that it doesn't check for a read error -- that
can be easily fixed.

> Whenever it should be at attach() or later is a good point for discussions.

The tda18271 driver supports running multiple tda18271 devices in
tandem with one another, including the ability to share xtal input and
rf loop thru.  In some cases, the order in which we initialize the
different tda18271's (when there are multiples) must be carefully
controlled, and we do this by attaching them to the bridge driver in
the order needed, such as in the saa7164 driver -- we need to be ABLE
to initialize the tuner during the attach, but being able to defer it
*as an option* is OK with me.

Regards,

Mike Krufky

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Mauro Carvalho Chehab]

Em Tue, 2 Oct 2012 08:22:28 -0400
Michael Krufky <mkrufky@linuxtv.org> escreveu:

> On Tue, Oct 2, 2012 at 7:05 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
> > Btw, why do you need to read 16 registers at once, instead of just reading
> > the needed register? read_extended and write operations are even more evil:
> > they read/write the full set of 39 registers on each operation. That seems
> > to be overkill, especially on places like tda18271_get_id(), where
> > all the driver is doing is to check for the ID register.
> 
> TDA18271 does not support subaddressing for read operations.  The only
> way to read a register is by dumping full register contents.  16
> registers in simple mode, 39 registers in extended mode.

Well, at least at get_id() I think you should just read the ID register
and not the full set.

> > Worse than that, tda18271_get_id() doesn't even check if the read()
> > operation failed: it assumes that it will always work, letting the
> > switch(regs[R_ID]) to print a wrong message (device unknown) when
> > what actually failed where the 16 registers dump.
> 
> That's a pretty standard operation to be able to read a chip's ID in
> its driver attach function.  You even have some drivers that continue
> trying to attach frontends and tuners as long as they continue to get
> an error in the attach() function.  If we dont read the chip's ID
> during attach() then how do we know we're attaching to the correct
> chip?

Yes, reading the chip ID there seems ok.

Btw, I think we should re-visit the I2C gate control logic where implemented.

Antti pasted me yesterday the logs from the driver:

By looking on those messages:

Sep 28 01:35:57 localhost kernel: [44798.782787] drxk: i2c_read: read from 63 42 c0 00, value =  00 00
Sep 28 01:35:57 localhost kernel: [44798.782804] tda18271_read_regs: [5-0060|M] ERROR: i2c_transfer returned: -19
...
Sep 28 01:35:57 localhost kernel: [44798.782980] Unknown device (16) detected @ 5-0060, device not supported.
Sep 28 01:35:57 localhost kernel: [44798.782985] tda18271_attach: [5-0060|M] error -22 on line 1274
Sep 28 01:35:57 localhost kernel: [44798.782989] tda18271 5-0060: destroying instance
Sep 28 01:35:57 localhost kernel: [44798.783003] drxk: drxk_release

I'm almost sure that the I2C gate control is at the wrong state there.

Very likely, the tda code is trying to access the I2C bus before DRX-K to
restore the I2C switch back to its original way.

In other words, I think that drivers with an I2C switch should be doing:
	- take I2C lock;
	- switch I2C gate;
	- do writes and/or read ops;
	- switch I2C gate back;
	- release I2C lock.

What's implemented, however, is:

	- switch I2C gate;
	- take I2C lock;
	- do writes and/or read ops;
	- release I2C lock.
	- switch I2C gate back;

So, there is a chance of a race condition where a pending I2C
operation will be handled with the I2C gate switch at the wrong
state.

Such change is not trivial, as it requires reviewing all drivers. Also,
the I2C switching may also require access to the I2C bus, making it
harder to do.

I know khali coded something for I2C switch at I2C core. We should likely
visit it and see if it could improve things there.

> I'll look at the fact that it doesn't check for a read error -- that
> can be easily fixed.

Please do so.

> > Whenever it should be at attach() or later is a good point for discussions.
> 
> The tda18271 driver supports running multiple tda18271 devices in
> tandem with one another, including the ability to share xtal input and
> rf loop thru.  In some cases, the order in which we initialize the
> different tda18271's (when there are multiples) must be carefully
> controlled, and we do this by attaching them to the bridge driver in
> the order needed, such as in the saa7164 driver -- we need to be ABLE
> to initialize the tuner during the attach, but being able to defer it
> *as an option* is OK with me.

Just wrote an email to Greg, c/c the involved parts, with regards to it.

I think the better, in the short term, is to apply the change for tda18271dd.

For the long term, to revert the drx-k asynchronous load, as I suspect
that, while delaying tda18271 init would fix for this device, we'll end
by getting problems on other parts.

That OOPS pointed by Antti shows that, by using an async load there, we'll
need to add some task to kill the deferred firmware loads if the I2C
bus got removed. I'm sure we'll also find other regressions by deferring
initialization task.

Regards,
Mauro

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Antti Palosaari]

On 10/02/2012 04:17 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 2 Oct 2012 08:22:28 -0400
> Michael Krufky <mkrufky@linuxtv.org> escreveu:
>
>> On Tue, Oct 2, 2012 at 7:05 AM, Mauro Carvalho Chehab
>> <mchehab@redhat.com> wrote:
>>> Btw, why do you need to read 16 registers at once, instead of just reading
>>> the needed register? read_extended and write operations are even more evil:
>>> they read/write the full set of 39 registers on each operation. That seems
>>> to be overkill, especially on places like tda18271_get_id(), where
>>> all the driver is doing is to check for the ID register.
>>
>> TDA18271 does not support subaddressing for read operations.  The only
>> way to read a register is by dumping full register contents.  16
>> registers in simple mode, 39 registers in extended mode.
>
> Well, at least at get_id() I think you should just read the ID register
> and not the full set.
>
>>> Worse than that, tda18271_get_id() doesn't even check if the read()
>>> operation failed: it assumes that it will always work, letting the
>>> switch(regs[R_ID]) to print a wrong message (device unknown) when
>>> what actually failed where the 16 registers dump.
>>
>> That's a pretty standard operation to be able to read a chip's ID in
>> its driver attach function.  You even have some drivers that continue
>> trying to attach frontends and tuners as long as they continue to get
>> an error in the attach() function.  If we dont read the chip's ID
>> during attach() then how do we know we're attaching to the correct
>> chip?
>
> Yes, reading the chip ID there seems ok.
>
> Btw, I think we should re-visit the I2C gate control logic where implemented.
>
> Antti pasted me yesterday the logs from the driver:
>
> By looking on those messages:
>
> Sep 28 01:35:57 localhost kernel: [44798.782787] drxk: i2c_read: read from 63 42 c0 00, value =  00 00
> Sep 28 01:35:57 localhost kernel: [44798.782804] tda18271_read_regs: [5-0060|M] ERROR: i2c_transfer returned: -19
> ...
> Sep 28 01:35:57 localhost kernel: [44798.782980] Unknown device (16) detected @ 5-0060, device not supported.
> Sep 28 01:35:57 localhost kernel: [44798.782985] tda18271_attach: [5-0060|M] error -22 on line 1274
> Sep 28 01:35:57 localhost kernel: [44798.782989] tda18271 5-0060: destroying instance
> Sep 28 01:35:57 localhost kernel: [44798.783003] drxk: drxk_release
>
> I'm almost sure that the I2C gate control is at the wrong state there.
>
> Very likely, the tda code is trying to access the I2C bus before DRX-K to
> restore the I2C switch back to its original way.
>
> In other words, I think that drivers with an I2C switch should be doing:
> 	- take I2C lock;
> 	- switch I2C gate;
> 	- do writes and/or read ops;
> 	- switch I2C gate back;
> 	- release I2C lock.
>
> What's implemented, however, is:
>
> 	- switch I2C gate;
> 	- take I2C lock;
> 	- do writes and/or read ops;
> 	- release I2C lock.
> 	- switch I2C gate back;

I am not sure at all about that I2C-gate. tda18271cc does not implement 
gate control and it is working. If I disable DRX-K gate control (by 
setting callback to NULL) tda18271 still fails similarly. No matter if 
gate control is used or not. So it is not behind gate or drx-k is broken 
driver is broken what goes to gate control.

I took two new logs with em28xx I2C debugs enabled.
http://palosaari.fi/linux/v4l-dvb/em28xx_drxk_tda18271_gate_disabled.txt
http://palosaari.fi/linux/v4l-dvb/em28xx_drxk_tda18271_gate_enabled.txt

Do you see reason here?

> So, there is a chance of a race condition where a pending I2C
> operation will be handled with the I2C gate switch at the wrong
> state.
>
> Such change is not trivial, as it requires reviewing all drivers. Also,
> the I2C switching may also require access to the I2C bus, making it
> harder to do.
>
> I know khali coded something for I2C switch at I2C core. We should likely
> visit it and see if it could improve things there.
>
>> I'll look at the fact that it doesn't check for a read error -- that
>> can be easily fixed.
>
> Please do so.
>
>>> Whenever it should be at attach() or later is a good point for discussions.
>>
>> The tda18271 driver supports running multiple tda18271 devices in
>> tandem with one another, including the ability to share xtal input and
>> rf loop thru.  In some cases, the order in which we initialize the
>> different tda18271's (when there are multiples) must be carefully
>> controlled, and we do this by attaching them to the bridge driver in
>> the order needed, such as in the saa7164 driver -- we need to be ABLE
>> to initialize the tuner during the attach, but being able to defer it
>> *as an option* is OK with me.
>
> Just wrote an email to Greg, c/c the involved parts, with regards to it.
>
> I think the better, in the short term, is to apply the change for tda18271dd.
>
> For the long term, to revert the drx-k asynchronous load, as I suspect
> that, while delaying tda18271 init would fix for this device, we'll end
> by getting problems on other parts.
>
> That OOPS pointed by Antti shows that, by using an async load there, we'll
> need to add some task to kill the deferred firmware loads if the I2C
> bus got removed. I'm sure we'll also find other regressions by deferring
> initialization task.
>
> Regards,
> Mauro
>


Antti

-- 
http://palosaari.fi/

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Antti Palosaari]

On 10/02/2012 02:05 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 02 Oct 2012 12:55:55 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> On 10/02/2012 06:17 AM, Michael Krufky wrote:
>>> On Mon, Oct 1, 2012 at 9:58 PM, Devin Heitmueller
>>> <dheitmueller@kernellabs.com> wrote:
>>>> On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>> New drxk firmware download does not work with tda18271. Actual
>>>>> reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
>>>>> will work as it does not do as much I/O during attach than tda18271.
>>>>>
>>>>> Root of cause is tuner I/O during drx-k asynchronous firmware
>>>>> download. request_firmware_nowait()... :-/
>>>>
>>>> This seems like it's just changing the timing of the initialization
>>>> process, which isn't really any better than the "msleep(2000)".  It's
>>>> just dumb luck that it happens to work on the developer's system.
>>>>
>>>> Don't get me wrong, I agree with Michael that this whole situation is
>>>> ridiculous, but I don't see why swapping out the entire driver is a
>>>> reasonable fix.
>>>
>>> I just send out a patch entitled, "tda18271: prevent register access
>>> during attach() if delay_cal is set"   Antti, could you set
>>> tda18271_config.delay_cal = 1 with this patch applied and see if it
>>> solves your problem?
>>>
>>> Again, although this may solve the problem for this particular device,
>>> the *real* problem is this asynchronous firmware download in the demod
>>> driver.
>>>
>>> Nonetheless, Antti has been asking for this feature, to not allow
>>> register access during attach, I was against it and I have my reasons,
>>> but I believe that this patch is a fair compromise.
>>>
>>> After somebody can test it, I think we should merge this -- any comments?
>>>
>>> http://patchwork.linuxtv.org/patch/14799/
>>
>> I tested. It does not help. I also looked it more and it really bails
>> out with error much earlier, in function where it reads chip ID. That
>> makes me look the tda18271c2dd driver.
>
> I saw Antti's logs: basically, tda18271_get_id() reads all registers at the
> chip during attach(), returning -EINVAL if tda18271_read_regs(fe) can't
> read the value for R_ID register.
>
> Btw, why do you need to read 16 registers at once, instead of just reading
> the needed register? read_extended and write operations are even more evil:
> they read/write the full set of 39 registers on each operation. That seems
> to be overkill, especially on places like tda18271_get_id(), where
> all the driver is doing is to check for the ID register.
>
> Worse than that, tda18271_get_id() doesn't even check if the read()
> operation failed: it assumes that it will always work, letting the
> switch(regs[R_ID]) to print a wrong message (device unknown) when
> what actually failed where the 16 registers dump.
>
>> I found that for some reason
>> these drivers uses different method for register read. tda18271 uses I2C
>> transaction with 2 messages, write and read with REPEATED START
>> condition. tda18271c2dd driver is just simple I2C read. So which one is
>> correct?
>
> That's due to the I2C locking schema: if you do two separate I2C
> transfers, the I2C core will allow an event to happen between the
> two operations. That typically causes troubles on read operations.
> So, it is recommended to use just one i2c_transfer() call for read
> operations that are mapped via a write and a read.

I know rather well how I2C works. As many messages you put to single 
transfer those are aimed to do with REPEATED START condition without a 
STOP. And naturally, when you split to multiple transactions then there 
is STOP. STOP is send after the last I2C message in one transaction.

But what I tried to say there was a different communication schema used 
between the drivers. Other must (quite likely) be wrong. There is no any 
mention about hacks. I don't see how that I2C locking you mention is 
related, as it is *one* I2C transfer in question for both cases.

Here is difference what I mean:
tda18271: START c0|Ack|00|Ack|START c1|data read|NAck|STOP
tda18271cc: START c1|data read|NAck|STOP

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Michael Krufky]

On Tue, Oct 2, 2012 at 8:29 AM, Antti Palosaari <crope@iki.fi> wrote:
> On 10/02/2012 02:05 PM, Mauro Carvalho Chehab wrote:
>>
>> Em Tue, 02 Oct 2012 12:55:55 +0300
>> Antti Palosaari <crope@iki.fi> escreveu:
>>
>>> On 10/02/2012 06:17 AM, Michael Krufky wrote:
>>>>
>>>> On Mon, Oct 1, 2012 at 9:58 PM, Devin Heitmueller
>>>> <dheitmueller@kernellabs.com> wrote:
>>>>>
>>>>> On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>
>>>>>> New drxk firmware download does not work with tda18271. Actual
>>>>>> reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
>>>>>> will work as it does not do as much I/O during attach than tda18271.
>>>>>>
>>>>>> Root of cause is tuner I/O during drx-k asynchronous firmware
>>>>>> download. request_firmware_nowait()... :-/
>>>>>
>>>>>
>>>>> This seems like it's just changing the timing of the initialization
>>>>> process, which isn't really any better than the "msleep(2000)".  It's
>>>>> just dumb luck that it happens to work on the developer's system.
>>>>>
>>>>> Don't get me wrong, I agree with Michael that this whole situation is
>>>>> ridiculous, but I don't see why swapping out the entire driver is a
>>>>> reasonable fix.
>>>>
>>>>
>>>> I just send out a patch entitled, "tda18271: prevent register access
>>>> during attach() if delay_cal is set"   Antti, could you set
>>>> tda18271_config.delay_cal = 1 with this patch applied and see if it
>>>> solves your problem?
>>>>
>>>> Again, although this may solve the problem for this particular device,
>>>> the *real* problem is this asynchronous firmware download in the demod
>>>> driver.
>>>>
>>>> Nonetheless, Antti has been asking for this feature, to not allow
>>>> register access during attach, I was against it and I have my reasons,
>>>> but I believe that this patch is a fair compromise.
>>>>
>>>> After somebody can test it, I think we should merge this -- any
>>>> comments?
>>>>
>>>> http://patchwork.linuxtv.org/patch/14799/
>>>
>>>
>>> I tested. It does not help. I also looked it more and it really bails
>>> out with error much earlier, in function where it reads chip ID. That
>>> makes me look the tda18271c2dd driver.
>>
>>
>> I saw Antti's logs: basically, tda18271_get_id() reads all registers at
>> the
>> chip during attach(), returning -EINVAL if tda18271_read_regs(fe) can't
>> read the value for R_ID register.
>>
>> Btw, why do you need to read 16 registers at once, instead of just reading
>> the needed register? read_extended and write operations are even more
>> evil:
>> they read/write the full set of 39 registers on each operation. That seems
>> to be overkill, especially on places like tda18271_get_id(), where
>> all the driver is doing is to check for the ID register.
>>
>> Worse than that, tda18271_get_id() doesn't even check if the read()
>> operation failed: it assumes that it will always work, letting the
>> switch(regs[R_ID]) to print a wrong message (device unknown) when
>> what actually failed where the 16 registers dump.
>>
>>> I found that for some reason
>>> these drivers uses different method for register read. tda18271 uses I2C
>>> transaction with 2 messages, write and read with REPEATED START
>>> condition. tda18271c2dd driver is just simple I2C read. So which one is
>>> correct?
>>
>>
>> That's due to the I2C locking schema: if you do two separate I2C
>> transfers, the I2C core will allow an event to happen between the
>> two operations. That typically causes troubles on read operations.
>> So, it is recommended to use just one i2c_transfer() call for read
>> operations that are mapped via a write and a read.
>
>
> I know rather well how I2C works. As many messages you put to single
> transfer those are aimed to do with REPEATED START condition without a STOP.
> And naturally, when you split to multiple transactions then there is STOP.
> STOP is send after the last I2C message in one transaction.
>
> But what I tried to say there was a different communication schema used
> between the drivers. Other must (quite likely) be wrong. There is no any
> mention about hacks. I don't see how that I2C locking you mention is
> related, as it is *one* I2C transfer in question for both cases.
>
> Here is difference what I mean:
> tda18271: START c0|Ack|00|Ack|START c1|data read|NAck|STOP
> tda18271cc: START c1|data read|NAck|STOP

As per section 9.1 of the NXP TDA18271 datasheet (page 8) (download
from http://www.nxp.com/documents/data_sheet/TDA18271HD.pdf)

9.1 I2C-bus format, Write/Read mode
Remark: The I2C-bus read in the TDA18271HD must read the entire
I2C-bus map, with
required subaddress 00h. The number of bytes read is 16, or 39 in
extended register
mode; see Table 7. Reading write-only bits can return values that are
different from the
programmed values

tda18271 is doing the right thing, tda18271cc is not.

-Mike Krufky

Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd

[Antti Palosaari]

On 10/02/2012 03:42 PM, Michael Krufky wrote:
> On Tue, Oct 2, 2012 at 8:29 AM, Antti Palosaari <crope@iki.fi> wrote:
>> On 10/02/2012 02:05 PM, Mauro Carvalho Chehab wrote:
>>>
>>> Em Tue, 02 Oct 2012 12:55:55 +0300
>>> Antti Palosaari <crope@iki.fi> escreveu:
>>>
>>>> On 10/02/2012 06:17 AM, Michael Krufky wrote:
>>>>>
>>>>> On Mon, Oct 1, 2012 at 9:58 PM, Devin Heitmueller
>>>>> <dheitmueller@kernellabs.com> wrote:
>>>>>>
>>>>>> On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari <crope@iki.fi> wrote:
>>>>>>>
>>>>>>> New drxk firmware download does not work with tda18271. Actual
>>>>>>> reason is more drxk driver than tda18271. Anyhow, tda18271c2dd
>>>>>>> will work as it does not do as much I/O during attach than tda18271.
>>>>>>>
>>>>>>> Root of cause is tuner I/O during drx-k asynchronous firmware
>>>>>>> download. request_firmware_nowait()... :-/
>>>>>>
>>>>>>
>>>>>> This seems like it's just changing the timing of the initialization
>>>>>> process, which isn't really any better than the "msleep(2000)".  It's
>>>>>> just dumb luck that it happens to work on the developer's system.
>>>>>>
>>>>>> Don't get me wrong, I agree with Michael that this whole situation is
>>>>>> ridiculous, but I don't see why swapping out the entire driver is a
>>>>>> reasonable fix.
>>>>>
>>>>>
>>>>> I just send out a patch entitled, "tda18271: prevent register access
>>>>> during attach() if delay_cal is set"   Antti, could you set
>>>>> tda18271_config.delay_cal = 1 with this patch applied and see if it
>>>>> solves your problem?
>>>>>
>>>>> Again, although this may solve the problem for this particular device,
>>>>> the *real* problem is this asynchronous firmware download in the demod
>>>>> driver.
>>>>>
>>>>> Nonetheless, Antti has been asking for this feature, to not allow
>>>>> register access during attach, I was against it and I have my reasons,
>>>>> but I believe that this patch is a fair compromise.
>>>>>
>>>>> After somebody can test it, I think we should merge this -- any
>>>>> comments?
>>>>>
>>>>> http://patchwork.linuxtv.org/patch/14799/
>>>>
>>>>
>>>> I tested. It does not help. I also looked it more and it really bails
>>>> out with error much earlier, in function where it reads chip ID. That
>>>> makes me look the tda18271c2dd driver.
>>>
>>>
>>> I saw Antti's logs: basically, tda18271_get_id() reads all registers at
>>> the
>>> chip during attach(), returning -EINVAL if tda18271_read_regs(fe) can't
>>> read the value for R_ID register.
>>>
>>> Btw, why do you need to read 16 registers at once, instead of just reading
>>> the needed register? read_extended and write operations are even more
>>> evil:
>>> they read/write the full set of 39 registers on each operation. That seems
>>> to be overkill, especially on places like tda18271_get_id(), where
>>> all the driver is doing is to check for the ID register.
>>>
>>> Worse than that, tda18271_get_id() doesn't even check if the read()
>>> operation failed: it assumes that it will always work, letting the
>>> switch(regs[R_ID]) to print a wrong message (device unknown) when
>>> what actually failed where the 16 registers dump.
>>>
>>>> I found that for some reason
>>>> these drivers uses different method for register read. tda18271 uses I2C
>>>> transaction with 2 messages, write and read with REPEATED START
>>>> condition. tda18271c2dd driver is just simple I2C read. So which one is
>>>> correct?
>>>
>>>
>>> That's due to the I2C locking schema: if you do two separate I2C
>>> transfers, the I2C core will allow an event to happen between the
>>> two operations. That typically causes troubles on read operations.
>>> So, it is recommended to use just one i2c_transfer() call for read
>>> operations that are mapped via a write and a read.
>>
>>
>> I know rather well how I2C works. As many messages you put to single
>> transfer those are aimed to do with REPEATED START condition without a STOP.
>> And naturally, when you split to multiple transactions then there is STOP.
>> STOP is send after the last I2C message in one transaction.
>>
>> But what I tried to say there was a different communication schema used
>> between the drivers. Other must (quite likely) be wrong. There is no any
>> mention about hacks. I don't see how that I2C locking you mention is
>> related, as it is *one* I2C transfer in question for both cases.
>>
>> Here is difference what I mean:
>> tda18271: START c0|Ack|00|Ack|START c1|data read|NAck|STOP
>> tda18271cc: START c1|data read|NAck|STOP
>
> As per section 9.1 of the NXP TDA18271 datasheet (page 8) (download
> from http://www.nxp.com/documents/data_sheet/TDA18271HD.pdf)
>
> 9.1 I2C-bus format, Write/Read mode
> Remark: The I2C-bus read in the TDA18271HD must read the entire
> I2C-bus map, with
> required subaddress 00h. The number of bytes read is 16, or 39 in
> extended register
> mode; see Table 7. Reading write-only bits can return values that are
> different from the
> programmed values
>
> tda18271 is doing the right thing, tda18271cc is not.
>
> -Mike Krufky

Yes, you are correct! Thanks.

I will still test if it makes any difference.

regards
Antti


-- 
http://palosaari.fi/