diff mbox series

[01/16] media: i2c: rdacm20: Enable noise immunity

Message ID 20210216174146.106639-2-jacopo+renesas@jmondi.org
State Superseded
Headers show
Series [01/16] media: i2c: rdacm20: Enable noise immunity | expand

Commit Message

Jacopo Mondi Feb. 16, 2021, 5:41 p.m. UTC
Enable the noise immunity threshold at the end of the rdacm20
initialization routine.

The rdcam20 camera module has been so far tested with a startup
delay that allowed the embedded MCU to program the serializer. If
the initialization routine is run before the MCU programs the
serializer and the image sensor and their addresses gets changed
by the rdacm20 driver it is required to manually enable the noise
immunity threshold to make the communication on the control channel
more reliable.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/rdacm20.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Feb. 17, 2021, 12:55 p.m. UTC | #1
Hi Jacopo,

On 16/02/2021 17:41, Jacopo Mondi wrote:
> Enable the noise immunity threshold at the end of the rdacm20

> initialization routine.

> 

> The rdcam20 camera module has been so far tested with a startup

> delay that allowed the embedded MCU to program the serializer. If

> the initialization routine is run before the MCU programs the

> serializer and the image sensor and their addresses gets changed

> by the rdacm20 driver it is required to manually enable the noise

> immunity threshold to make the communication on the control channel

> more reliable.

> 


Oh, this is interesting, ... booting up without the delays would be ...
much nicer.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> ---

>  drivers/media/i2c/rdacm20.c | 8 +++++++-

>  1 file changed, 7 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c

> index 90eb73f0e6e9..f7fd5ae955d0 100644

> --- a/drivers/media/i2c/rdacm20.c

> +++ b/drivers/media/i2c/rdacm20.c

> @@ -541,7 +541,13 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

>  

>  	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");

>  

> -	return 0;

> +	/*

> +	 * Set reverse channel high threshold to increase noise immunity.

> +	 *

> +	 * This should be compensated by increasing the reverse channel

> +	 * amplitude on the remote deserializer side.

> +	 */

> +	return max9271_set_high_threshold(&dev->serializer, true);


Does this work 'out of the box' ? I.e. if this patch is applied, I
assume it is required to remove the regulator delays that I/we have in DT?

Likewise, does that note mean this patch must also be accompanied by the
update in max9286 somehow?

I guess we can't keep 'test bisectability' with this very easily so it
probably doesn't matter too much, the end result will be the interesting
part.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>




>  }

>  

>  static int rdacm20_probe(struct i2c_client *client)

>
Laurent Pinchart Feb. 22, 2021, 12:49 a.m. UTC | #2
Hi Jacopo,

Thank you for the patch.

On Wed, Feb 17, 2021 at 12:55:19PM +0000, Kieran Bingham wrote:
> On 16/02/2021 17:41, Jacopo Mondi wrote:

> > Enable the noise immunity threshold at the end of the rdacm20

> > initialization routine.

> > 

> > The rdcam20 camera module has been so far tested with a startup


s/rdcam20/rdacm20/

> > delay that allowed the embedded MCU to program the serializer. If

> > the initialization routine is run before the MCU programs the

> > serializer and the image sensor and their addresses gets changed

> > by the rdacm20 driver it is required to manually enable the noise

> > immunity threshold to make the communication on the control channel

> > more reliable.

> 

> Oh, this is interesting, ... booting up without the delays would be ...

> much nicer.


I second that, but I'm a bit worried. The MCU has caused us more pain
than gain, the best way to fix it may be with a desoldering station ;-)
Jokes aside, if we want to start initializing with the serializer before
the MCU completes its initialization, then we'll have a racy process,
with two I2C masters configuring the same device. I don't think anything
good can come out of that :-S

Taking into account the fact that on some platforms we'll want to
implement power management for the cameras, disabling power (possibly
individually) when the cameras are not in use, we'll have to handle the
race carefully, and I'm not sure there any other way than waiting for
the camera to be initialized with an initialization delay after power
up.

Based on this, I'm not concerned about this patch in particular, but
potentially about the series as a whole. I'll comment on individual
patches as applicable.

Regarding this patch, doies the MCU enable high threshold for the
reverse channel as part of its initialization procedure ? Do we have a
full list of what it configures in the MAX9271 ? If so, could we capture
it in a comment in the driver ? That would be very helpful as a
reference.

> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> > ---

> >  drivers/media/i2c/rdacm20.c | 8 +++++++-

> >  1 file changed, 7 insertions(+), 1 deletion(-)

> > 

> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c

> > index 90eb73f0e6e9..f7fd5ae955d0 100644

> > --- a/drivers/media/i2c/rdacm20.c

> > +++ b/drivers/media/i2c/rdacm20.c

> > @@ -541,7 +541,13 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

> >  

> >  	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");

> >  

> > -	return 0;

> > +	/*

> > +	 * Set reverse channel high threshold to increase noise immunity.

> > +	 *

> > +	 * This should be compensated by increasing the reverse channel

> > +	 * amplitude on the remote deserializer side.

> > +	 */

> > +	return max9271_set_high_threshold(&dev->serializer, true);

> 

> Does this work 'out of the box' ? I.e. if this patch is applied, I

> assume it is required to remove the regulator delays that I/we have in DT?

> 

> Likewise, does that note mean this patch must also be accompanied by the

> update in max9286 somehow?

> 

> I guess we can't keep 'test bisectability' with this very easily so it

> probably doesn't matter too much, the end result will be the interesting

> part.

> 

> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 

> >  }

> >  

> >  static int rdacm20_probe(struct i2c_client *client)


-- 
Regards,

Laurent Pinchart
Jacopo Mondi Feb. 22, 2021, 2:59 p.m. UTC | #3
Hi Kieran, Laurent,

On Mon, Feb 22, 2021 at 02:49:34AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,

>

> Thank you for the patch.

>

> On Wed, Feb 17, 2021 at 12:55:19PM +0000, Kieran Bingham wrote:

> > On 16/02/2021 17:41, Jacopo Mondi wrote:

> > > Enable the noise immunity threshold at the end of the rdacm20

> > > initialization routine.

> > >

> > > The rdcam20 camera module has been so far tested with a startup

>

> s/rdcam20/rdacm20/

>

> > > delay that allowed the embedded MCU to program the serializer. If

> > > the initialization routine is run before the MCU programs the

> > > serializer and the image sensor and their addresses gets changed

> > > by the rdacm20 driver it is required to manually enable the noise

> > > immunity threshold to make the communication on the control channel

> > > more reliable.

> >

> > Oh, this is interesting, ... booting up without the delays would be ...

> > much nicer.

>

> I second that, but I'm a bit worried. The MCU has caused us more pain

> than gain, the best way to fix it may be with a desoldering station ;-)


I wish we could

> Jokes aside, if we want to start initializing with the serializer before

> the MCU completes its initialization, then we'll have a racy process,

> with two I2C masters configuring the same device. I don't think anything

> good can come out of that :-S

>

> Taking into account the fact that on some platforms we'll want to

> implement power management for the cameras, disabling power (possibly

> individually) when the cameras are not in use, we'll have to handle the

> race carefully, and I'm not sure there any other way than waiting for

> the camera to be initialized with an initialization delay after power

> up.


Currently I really cannot tell how long the intialization takes, and
we downstream inserted a 'long enough' 8 seconds delay to accommodate
that, which seems one of those solution that work as long as they
don't work anymore.

One thing to notices is that I tried to interface with the MCU to read
its most basic parameters, like the number of i2c messages sent to the
serializer before programming the image sensor and that's just a mere 3
messages. What I noticed was also that I was not able to talk to the MCU
for 1.5 seconds, which I'm not sure it's because of a startup delay of
because of cross-talks. If that was a startup delay, it would really be
convenient, as we could change the chip addresses immediately and have
the MCU programming being sent to a non-existing id.

>

> Based on this, I'm not concerned about this patch in particular, but

> potentially about the series as a whole. I'll comment on individual

> patches as applicable.

>

> Regarding this patch, doies the MCU enable high threshold for the

> reverse channel as part of its initialization procedure ? Do we have a


we always assumed so, as it was not required for the RDACM20 to start
with a de-serializer low power amplitude and increase it after the
camera has probed like we have to do for the un-programmed RDACM21
with the intiial startup delay (the custom maxim,reverse-channel-microvolt
property serves this purpose)

As a confirmation, if I remove the delay and probe the camera before
the MCU gets to program it, I need to enable the threshold manually
as otherwise I lose the ability to stream from cameras.

All in all, my best bet is that without the delay we get to probe and
re-program the serializer address before the MCU starts it programming
phase. All of this is without synchronization, without any known delay
being described in the camera manual, all based on empirical
deduction and repeated testing. I'll keep the opinion on GMSL as a
techology for myself here, but I think this solution is in-line with
the global quality level of the systems we're working with.

> full list of what it configures in the MAX9271 ? If so, could we capture


Not at the moment but I can investigate dumping that from the MCU as
I've been able to get its 'status' and a command to get its content
should be available

> it in a comment in the driver ? That would be very helpful as a

> reference.

>

> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> > > ---

> > >  drivers/media/i2c/rdacm20.c | 8 +++++++-

> > >  1 file changed, 7 insertions(+), 1 deletion(-)

> > >

> > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c

> > > index 90eb73f0e6e9..f7fd5ae955d0 100644

> > > --- a/drivers/media/i2c/rdacm20.c

> > > +++ b/drivers/media/i2c/rdacm20.c

> > > @@ -541,7 +541,13 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

> > >

> > >  	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");

> > >

> > > -	return 0;

> > > +	/*

> > > +	 * Set reverse channel high threshold to increase noise immunity.

> > > +	 *

> > > +	 * This should be compensated by increasing the reverse channel

> > > +	 * amplitude on the remote deserializer side.

> > > +	 */

> > > +	return max9271_set_high_threshold(&dev->serializer, true);

> >

> > Does this work 'out of the box' ? I.e. if this patch is applied, I

> > assume it is required to remove the regulator delays that I/we have in DT?


It doesn't hurt, as if this happen -after- the MCU has programmed the
chip, we're just re-enabling something that was enabled (remember
RDACM20 goes with maxim,reverse-channel-microvol=170 when the dealy
was inserted).

Without the dealy it could be operated as the RDACM21 (start low,
probe+enable threshold, set high).

> >

> > Likewise, does that note mean this patch must also be accompanied by the

> > update in max9286 somehow?

> >


Ee have a DT property to control this already, and the delay+channel
amplitude can be controlled from DTS entirely.

> > I guess we can't keep 'test bisectability' with this very easily so it

> > probably doesn't matter too much, the end result will be the interesting

> > part.

> >

> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> >

> > >  }

> > >

> > >  static int rdacm20_probe(struct i2c_client *client)

>

> --

> Regards,

>

> Laurent Pinchart
Laurent Pinchart Feb. 24, 2021, 8:35 p.m. UTC | #4
Hi Jacopo,

On Mon, Feb 22, 2021 at 03:59:05PM +0100, Jacopo Mondi wrote:
> On Mon, Feb 22, 2021 at 02:49:34AM +0200, Laurent Pinchart wrote:

> > On Wed, Feb 17, 2021 at 12:55:19PM +0000, Kieran Bingham wrote:

> > > On 16/02/2021 17:41, Jacopo Mondi wrote:

> > > > Enable the noise immunity threshold at the end of the rdacm20

> > > > initialization routine.

> > > >

> > > > The rdcam20 camera module has been so far tested with a startup

> >

> > s/rdcam20/rdacm20/

> >

> > > > delay that allowed the embedded MCU to program the serializer. If

> > > > the initialization routine is run before the MCU programs the

> > > > serializer and the image sensor and their addresses gets changed

> > > > by the rdacm20 driver it is required to manually enable the noise

> > > > immunity threshold to make the communication on the control channel

> > > > more reliable.

> > >

> > > Oh, this is interesting, ... booting up without the delays would be ...

> > > much nicer.

> >

> > I second that, but I'm a bit worried. The MCU has caused us more pain

> > than gain, the best way to fix it may be with a desoldering station ;-)

> 

> I wish we could

> 

> > Jokes aside, if we want to start initializing with the serializer before

> > the MCU completes its initialization, then we'll have a racy process,

> > with two I2C masters configuring the same device. I don't think anything

> > good can come out of that :-S

> >

> > Taking into account the fact that on some platforms we'll want to

> > implement power management for the cameras, disabling power (possibly

> > individually) when the cameras are not in use, we'll have to handle the

> > race carefully, and I'm not sure there any other way than waiting for

> > the camera to be initialized with an initialization delay after power

> > up.

> 

> Currently I really cannot tell how long the intialization takes, and

> we downstream inserted a 'long enough' 8 seconds delay to accommodate

> that, which seems one of those solution that work as long as they

> don't work anymore.

> 

> One thing to notices is that I tried to interface with the MCU to read

> its most basic parameters, like the number of i2c messages sent to the

> serializer before programming the image sensor and that's just a mere 3

> messages. What I noticed was also that I was not able to talk to the MCU

> for 1.5 seconds, which I'm not sure it's because of a startup delay of

> because of cross-talks. If that was a startup delay, it would really be

> convenient, as we could change the chip addresses immediately and have

> the MCU programming being sent to a non-existing id.


Scary :-)

> > Based on this, I'm not concerned about this patch in particular, but

> > potentially about the series as a whole. I'll comment on individual

> > patches as applicable.

> >

> > Regarding this patch, doies the MCU enable high threshold for the

> > reverse channel as part of its initialization procedure ? Do we have a

> 

> we always assumed so, as it was not required for the RDACM20 to start

> with a de-serializer low power amplitude and increase it after the

> camera has probed like we have to do for the un-programmed RDACM21

> with the intiial startup delay (the custom maxim,reverse-channel-microvolt

> property serves this purpose)

> 

> As a confirmation, if I remove the delay and probe the camera before

> the MCU gets to program it, I need to enable the threshold manually

> as otherwise I lose the ability to stream from cameras.

> 

> All in all, my best bet is that without the delay we get to probe and

> re-program the serializer address before the MCU starts it programming

> phase. All of this is without synchronization, without any known delay

> being described in the camera manual, all based on empirical

> deduction and repeated testing. I'll keep the opinion on GMSL as a

> techology for myself here, but I think this solution is in-line with

> the global quality level of the systems we're working with.


Your opinion may have transpired from that last sentence.

> > full list of what it configures in the MAX9271 ? If so, could we capture

> 

> Not at the moment but I can investigate dumping that from the MCU as

> I've been able to get its 'status' and a command to get its content

> should be available


Thanks, that would be very helpful.

> > it in a comment in the driver ? That would be very helpful as a

> > reference.

> >

> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> > > > ---

> > > >  drivers/media/i2c/rdacm20.c | 8 +++++++-

> > > >  1 file changed, 7 insertions(+), 1 deletion(-)

> > > >

> > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c

> > > > index 90eb73f0e6e9..f7fd5ae955d0 100644

> > > > --- a/drivers/media/i2c/rdacm20.c

> > > > +++ b/drivers/media/i2c/rdacm20.c

> > > > @@ -541,7 +541,13 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

> > > >

> > > >  	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");

> > > >

> > > > -	return 0;

> > > > +	/*

> > > > +	 * Set reverse channel high threshold to increase noise immunity.

> > > > +	 *

> > > > +	 * This should be compensated by increasing the reverse channel

> > > > +	 * amplitude on the remote deserializer side.

> > > > +	 */

> > > > +	return max9271_set_high_threshold(&dev->serializer, true);

> > >

> > > Does this work 'out of the box' ? I.e. if this patch is applied, I

> > > assume it is required to remove the regulator delays that I/we have in DT?

> 

> It doesn't hurt, as if this happen -after- the MCU has programmed the

> chip, we're just re-enabling something that was enabled (remember

> RDACM20 goes with maxim,reverse-channel-microvol=170 when the dealy

> was inserted).

> 

> Without the dealy it could be operated as the RDACM21 (start low,

> probe+enable threshold, set high).

> 

> > > Likewise, does that note mean this patch must also be accompanied by the

> > > update in max9286 somehow?

> 

> Ee have a DT property to control this already, and the delay+channel

> amplitude can be controlled from DTS entirely.

> 

> > > I guess we can't keep 'test bisectability' with this very easily so it

> > > probably doesn't matter too much, the end result will be the interesting

> > > part.

> > >

> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> > >

> > > >  }

> > > >

> > > >  static int rdacm20_probe(struct i2c_client *client)


-- 
Regards,

Laurent Pinchart
diff mbox series

Patch

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 90eb73f0e6e9..f7fd5ae955d0 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -541,7 +541,13 @@  static int rdacm20_initialize(struct rdacm20_device *dev)
 
 	dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
 
-	return 0;
+	/*
+	 * Set reverse channel high threshold to increase noise immunity.
+	 *
+	 * This should be compensated by increasing the reverse channel
+	 * amplitude on the remote deserializer side.
+	 */
+	return max9271_set_high_threshold(&dev->serializer, true);
 }
 
 static int rdacm20_probe(struct i2c_client *client)