From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
niklas.soderlund+renesas@ragnatech.se, geert@linux-m68k.org,
Mauro Carvalho Chehab <mchehab@kernel.org>,
linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity
Date: Wed, 24 Feb 2021 22:35:33 +0200 [thread overview]
Message-ID: <YDa4leayx0E/AwyX@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20210222145905.ao4larxjv5tvk3yd@uno.localdomain>
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
next prev parent reply other threads:[~2021-02-24 20:37 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-16 17:41 [PATCH 00/16] media: i2c: GMSL reliability improvements Jacopo Mondi
2021-02-16 17:41 ` [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity Jacopo Mondi
2021-02-17 12:55 ` Kieran Bingham
2021-02-22 0:49 ` Laurent Pinchart
2021-02-22 14:59 ` Jacopo Mondi
2021-02-24 20:35 ` Laurent Pinchart [this message]
2021-02-16 17:41 ` [PATCH 02/16] media: i2c: rdacm20: Embedded 'serializer' field Jacopo Mondi
2021-02-17 12:56 ` Kieran Bingham
2021-02-22 0:58 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop Jacopo Mondi
2021-02-17 13:01 ` Kieran Bingham
2021-02-22 1:05 ` Laurent Pinchart
2021-02-22 15:06 ` Jacopo Mondi
2021-02-24 20:27 ` Laurent Pinchart
2021-02-25 8:56 ` Kieran Bingham
2021-02-16 17:41 ` [PATCH 04/16] media: i2c: rdacm20: Report camera module name Jacopo Mondi
2021-02-17 13:02 ` Kieran Bingham
2021-02-22 1:06 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 05/16] media: i2c: rdacm20: Check return values Jacopo Mondi
2021-02-17 13:08 ` Kieran Bingham
2021-02-22 1:09 ` Laurent Pinchart
2021-02-22 15:08 ` Jacopo Mondi
2021-02-16 17:41 ` [PATCH 06/16] media: i2c: rdacm20: Re-work ov10635 reset Jacopo Mondi
2021-02-17 13:22 ` Kieran Bingham
2021-02-22 1:15 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay Jacopo Mondi
2021-02-17 8:02 ` Geert Uytterhoeven
2021-02-17 13:33 ` Kieran Bingham
2021-02-22 1:18 ` Laurent Pinchart
2021-02-22 15:11 ` Jacopo Mondi
2021-02-24 20:29 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 08/16] media: i2c: max9286: Adjust parameters indent Jacopo Mondi
2021-02-17 13:36 ` Kieran Bingham
2021-02-22 1:20 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization Jacopo Mondi
2021-02-17 8:06 ` Geert Uytterhoeven
2021-02-17 13:55 ` Kieran Bingham
2021-02-22 1:27 ` Laurent Pinchart
2021-02-22 15:19 ` Jacopo Mondi
2021-02-24 20:30 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 10/16] media: i2c: max9286: Rename reverse_channel_mv Jacopo Mondi
2021-02-17 8:07 ` Geert Uytterhoeven
2021-02-18 15:06 ` Kieran Bingham
2021-02-22 1:29 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 11/16] media: i2c: max9286: Cache channel amplitude Jacopo Mondi
2021-02-17 8:08 ` Geert Uytterhoeven
2021-02-18 15:39 ` Kieran Bingham
2021-02-22 1:33 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 12/16] media: i2c: max9286: Define high " Jacopo Mondi
2021-02-18 15:39 ` Kieran Bingham
2021-02-22 1:35 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 13/16] media: i2c: rdacm2x: Implement .init() subdev op Jacopo Mondi
2021-02-17 8:17 ` Geert Uytterhoeven
2021-02-18 16:13 ` Kieran Bingham
2021-02-22 1:52 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 14/16] media: i2c: max9286: Initialize remotes when bound Jacopo Mondi
2021-02-17 8:19 ` Geert Uytterhoeven
2021-02-18 16:00 ` Kieran Bingham
2021-02-22 2:03 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 15/16] media: i2c: max9286: Rework comments in .bound() Jacopo Mondi
2021-02-18 16:03 ` Kieran Bingham
2021-02-16 17:41 ` [PATCH 16/16] media: i2c: gmsl: Use 339Kbps I2C bit-rate Jacopo Mondi
2021-02-17 8:19 ` Geert Uytterhoeven
2021-02-18 16:07 ` Kieran Bingham
2021-02-22 2:06 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YDa4leayx0E/AwyX@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=geert@linux-m68k.org \
--cc=jacopo+renesas@jmondi.org \
--cc=jacopo@jmondi.org \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).