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@xxxxxxxxxx> > > > > --- > > > > 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@xxxxxxxxxxxxxxxx> > > > > > > > } > > > > > > > > static int rdacm20_probe(struct i2c_client *client) -- Regards, Laurent Pinchart
