| Message ID | 20230724222210.162785-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: Enable MIPI interface in ov5640_set_power_mipi() | expand |
Hi Marek On Tue, Jul 25, 2023 at 12:22:10AM +0200, Marek Vasut wrote: > Set OV5640_REG_IO_MIPI_CTRL00 bit 2 to 1 instead of 0, since 1 means > MIPI CSI2 interface, while 0 means CPI parallel interface. > > In the ov5640_set_power_mipi() the interface should obviously be set > to MIPI CSI2 since this functions is used to power up the sensor when > operated in MIPI CSI2 mode. The sensor should not be in CPI mode in > that case. Does this actually help fixing your 'first frame corrupted issue' ? I think the logic here was to power up the interface here in ov5640_set_power_mipi() with the CSI-2 interface disabled to enter LP-11 mode (something some receivers like the imx6 one are particularly sensible to). Then at stream time the CSI-2 interface is actually enabled in ov5640_set_stream_mipi() just before streaming is started. Also the register documentation is very confusing and as reported in ov5640_set_stream_mipi() it is also probably wrong (at least in the datasheet version I have). I would be particularly cautious in touching this sequence as it has been validated to work with multiple receivers. Of course if it actually fixes an issue for you it should be taken in, but ideally, as this sensor is still used in a large number of evaluation boards, it should be validated by other consumers of this driver (st, imx, microchip and rensas to name a few). Thanks j > > Fixes: aa4bb8b8838f ("media: ov5640: Re-work MIPI startup sequence") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Steve Longerbeam <slongerbeam@gmail.com> > Cc: linux-media@vger.kernel.org > --- > drivers/media/i2c/ov5640.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 8f21839b08c78..8b7ff2f3bdda7 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -2539,9 +2539,9 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on) > * "ov5640_set_stream_mipi()") > * [4] = 0 : Power up MIPI HS Tx > * [3] = 0 : Power up MIPI LS Rx > - * [2] = 0 : MIPI interface disabled > + * [2] = 1 : MIPI interface enabled > */ > - ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40); > + ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x44); > if (ret) > return ret; > > -- > 2.40.1 >
On 7/25/23 11:04, Jacopo Mondi wrote: > Hi Marek Hi, > On Tue, Jul 25, 2023 at 12:22:10AM +0200, Marek Vasut wrote: >> Set OV5640_REG_IO_MIPI_CTRL00 bit 2 to 1 instead of 0, since 1 means >> MIPI CSI2 interface, while 0 means CPI parallel interface. >> >> In the ov5640_set_power_mipi() the interface should obviously be set >> to MIPI CSI2 since this functions is used to power up the sensor when >> operated in MIPI CSI2 mode. The sensor should not be in CPI mode in >> that case. > > Does this actually help fixing your 'first frame corrupted issue' ? Yes it does. > I think the logic here was to power up the interface here in > ov5640_set_power_mipi() with the CSI-2 interface disabled to enter > LP-11 mode (something some receivers like the imx6 one are > particularly sensible to). Per OV5640 datasheet. Register 0x300e bit 2 selects sensor interface mode between MIPI CSI2 and CPI (parallel), it has nothing to do with LP modes . Register 0x3019 bits [6:4] control LP00/LP11 mode on CSI2 lines. > Then at stream time the CSI-2 interface is actually enabled in > ov5640_set_stream_mipi() just before streaming is started. > > Also the register documentation is very confusing and as reported in > ov5640_set_stream_mipi() it is also probably wrong (at least in the > datasheet version I have). > > I would be particularly cautious in touching this sequence as it has > been validated to work with multiple receivers. Of course if it > actually fixes an issue for you it should be taken in, but ideally, as > this sensor is still used in a large number of evaluation boards, it > should be validated by other consumers of this driver (st, imx, > microchip and rensas to name a few). That's quite a tall order which effectively makes it impossible to change or fix anything in this driver.
Hi Marek On Tue, Jul 25, 2023 at 11:41:18AM +0200, Marek Vasut wrote: > On 7/25/23 11:04, Jacopo Mondi wrote: > > Hi Marek > > Hi, > > > On Tue, Jul 25, 2023 at 12:22:10AM +0200, Marek Vasut wrote: > > > Set OV5640_REG_IO_MIPI_CTRL00 bit 2 to 1 instead of 0, since 1 means > > > MIPI CSI2 interface, while 0 means CPI parallel interface. > > > > > > In the ov5640_set_power_mipi() the interface should obviously be set > > > to MIPI CSI2 since this functions is used to power up the sensor when > > > operated in MIPI CSI2 mode. The sensor should not be in CPI mode in > > > that case. > > > > Does this actually help fixing your 'first frame corrupted issue' ? > > Yes it does. > > > I think the logic here was to power up the interface here in > > ov5640_set_power_mipi() with the CSI-2 interface disabled to enter > > LP-11 mode (something some receivers like the imx6 one are > > particularly sensible to). > > Per OV5640 datasheet. > > Register 0x300e bit 2 selects sensor interface mode between MIPI CSI2 and > CPI (parallel), it has nothing to do with LP modes . > > Register 0x3019 bits [6:4] control LP00/LP11 mode on CSI2 lines. > > > Then at stream time the CSI-2 interface is actually enabled in > > ov5640_set_stream_mipi() just before streaming is started. > > > > Also the register documentation is very confusing and as reported in > > ov5640_set_stream_mipi() it is also probably wrong (at least in the > > datasheet version I have). > > > > I would be particularly cautious in touching this sequence as it has > > been validated to work with multiple receivers. Of course if it > > actually fixes an issue for you it should be taken in, but ideally, as > > this sensor is still used in a large number of evaluation boards, it > > should be validated by other consumers of this driver (st, imx, > > microchip and rensas to name a few). > > That's quite a tall order which effectively makes it impossible to change or > fix anything in this driver. Really? Asking other people which use this driver to test a patch before merging it to make sure it doesn't break their setup makes "impossibile to change of fix anything" ? And it's not an order of any sort, but there are a lot of users of this driver and multiple times fixing a thing on one side breaks things for others, I'm just trying to coordinate between multiple developers.
On 7/25/23 12:04, Jacopo Mondi wrote: > Hi Marek Hi, > On Tue, Jul 25, 2023 at 11:41:18AM +0200, Marek Vasut wrote: >> On 7/25/23 11:04, Jacopo Mondi wrote: >>> Hi Marek >> >> Hi, >> >>> On Tue, Jul 25, 2023 at 12:22:10AM +0200, Marek Vasut wrote: >>>> Set OV5640_REG_IO_MIPI_CTRL00 bit 2 to 1 instead of 0, since 1 means >>>> MIPI CSI2 interface, while 0 means CPI parallel interface. >>>> >>>> In the ov5640_set_power_mipi() the interface should obviously be set >>>> to MIPI CSI2 since this functions is used to power up the sensor when >>>> operated in MIPI CSI2 mode. The sensor should not be in CPI mode in >>>> that case. >>> >>> Does this actually help fixing your 'first frame corrupted issue' ? >> >> Yes it does. >> >>> I think the logic here was to power up the interface here in >>> ov5640_set_power_mipi() with the CSI-2 interface disabled to enter >>> LP-11 mode (something some receivers like the imx6 one are >>> particularly sensible to). >> >> Per OV5640 datasheet. >> >> Register 0x300e bit 2 selects sensor interface mode between MIPI CSI2 and >> CPI (parallel), it has nothing to do with LP modes . >> >> Register 0x3019 bits [6:4] control LP00/LP11 mode on CSI2 lines. >> >>> Then at stream time the CSI-2 interface is actually enabled in >>> ov5640_set_stream_mipi() just before streaming is started. >>> >>> Also the register documentation is very confusing and as reported in >>> ov5640_set_stream_mipi() it is also probably wrong (at least in the >>> datasheet version I have). >>> >>> I would be particularly cautious in touching this sequence as it has >>> been validated to work with multiple receivers. Of course if it >>> actually fixes an issue for you it should be taken in, but ideally, as >>> this sensor is still used in a large number of evaluation boards, it >>> should be validated by other consumers of this driver (st, imx, >>> microchip and rensas to name a few). >> >> That's quite a tall order which effectively makes it impossible to change or >> fix anything in this driver. > > Really? Asking other people which use this driver to test a patch before > merging it to make sure it doesn't break their setup makes "impossibile to > change of fix anything" ? > > And it's not an order of any sort, but there are a lot of users of > this driver and multiple times fixing a thing on one side breaks > things for others, I'm just trying to coordinate between multiple > developers. I'm afraid my expression of skepticism to the expected amount of testing has been misinterpreted, and so was the message you were trying to convey, i.e. communication breakdown. So let me rephrase. If the expectation is that every change to this driver has to be tested by all aforementioned parties, then it will be very hard to get changes in, based on my previous experience. Maybe let's stop this part of the thread here, and focus on the upper part?
Hi Marek On Tue, Jul 25, 2023 at 12:35:00PM +0200, Marek Vasut wrote: > On 7/25/23 12:04, Jacopo Mondi wrote: > > Hi Marek > > Hi, > > > On Tue, Jul 25, 2023 at 11:41:18AM +0200, Marek Vasut wrote: > > > On 7/25/23 11:04, Jacopo Mondi wrote: > > > > Hi Marek > > > > > > Hi, > > > > > > > On Tue, Jul 25, 2023 at 12:22:10AM +0200, Marek Vasut wrote: > > > > > Set OV5640_REG_IO_MIPI_CTRL00 bit 2 to 1 instead of 0, since 1 means > > > > > MIPI CSI2 interface, while 0 means CPI parallel interface. > > > > > > > > > > In the ov5640_set_power_mipi() the interface should obviously be set > > > > > to MIPI CSI2 since this functions is used to power up the sensor when > > > > > operated in MIPI CSI2 mode. The sensor should not be in CPI mode in > > > > > that case. > > > > > > > > Does this actually help fixing your 'first frame corrupted issue' ? > > > > > > Yes it does. > > > Do you think it's worth mentioning it in the commit message ? > > > > I think the logic here was to power up the interface here in > > > > ov5640_set_power_mipi() with the CSI-2 interface disabled to enter > > > > LP-11 mode (something some receivers like the imx6 one are > > > > particularly sensible to). > > > > > > Per OV5640 datasheet. > > > > > > Register 0x300e bit 2 selects sensor interface mode between MIPI CSI2 and > > > CPI (parallel), it has nothing to do with LP modes . > > > > > > Register 0x3019 bits [6:4] control LP00/LP11 mode on CSI2 lines. > > > > > > > Then at stream time the CSI-2 interface is actually enabled in > > > > ov5640_set_stream_mipi() just before streaming is started. > > > > > > > > Also the register documentation is very confusing and as reported in > > > > ov5640_set_stream_mipi() it is also probably wrong (at least in the > > > > datasheet version I have). > > > > > > > > I would be particularly cautious in touching this sequence as it has > > > > been validated to work with multiple receivers. Of course if it > > > > actually fixes an issue for you it should be taken in, but ideally, as > > > > this sensor is still used in a large number of evaluation boards, it > > > > should be validated by other consumers of this driver (st, imx, > > > > microchip and rensas to name a few). Ok, I've been able to test on i.MX6Q which I was concerned for because of its sensitivness to LP-11 detection. Let alone that imx6 with ov5640 is broken on mainline because of unrelated reasons [1] I've been able to confirm that the sensor still works on this platform Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> [Test on imx6q] Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Can you confirm you have tested with iMX8MP as well ? Let me cc a few people that might be interested in testing this and give them a bit of time to do so. After that let's collect this patch! Thanks j [1] My tree looks like: 6205990fa14e media: ov5640: Enable MIPI interface in ov5640_set_power_mipi() 91f44d7815aa media: ov5640: Fix initial RESETB state and annotate timings 015497e35950 media: i2c: ov5640: Implement get_mbus_config cd4415af64e7 Revert "media: video-mux: update driver to active state" d716bd480968 Revert "media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()" > > > > > > That's quite a tall order which effectively makes it impossible to change or > > > fix anything in this driver. > > > > Really? Asking other people which use this driver to test a patch before > > merging it to make sure it doesn't break their setup makes "impossibile to > > change of fix anything" ? > > > > And it's not an order of any sort, but there are a lot of users of > > this driver and multiple times fixing a thing on one side breaks > > things for others, I'm just trying to coordinate between multiple > > developers. > > I'm afraid my expression of skepticism to the expected amount of testing has > been misinterpreted, and so was the message you were trying to convey, i.e. > communication breakdown. > > So let me rephrase. > > If the expectation is that every change to this driver has to be tested by > all aforementioned parties, then it will be very hard to get changes in, > based on my previous experience. > > Maybe let's stop this part of the thread here, and focus on the upper part?
On 8/2/23 15:13, Jacopo Mondi wrote: > Hi Marek Hi, > On Tue, Jul 25, 2023 at 12:35:00PM +0200, Marek Vasut wrote: >> On 7/25/23 12:04, Jacopo Mondi wrote: >>> Hi Marek >> >> Hi, >> >>> On Tue, Jul 25, 2023 at 11:41:18AM +0200, Marek Vasut wrote: >>>> On 7/25/23 11:04, Jacopo Mondi wrote: >>>>> Hi Marek >>>> >>>> Hi, >>>> >>>>> On Tue, Jul 25, 2023 at 12:22:10AM +0200, Marek Vasut wrote: >>>>>> Set OV5640_REG_IO_MIPI_CTRL00 bit 2 to 1 instead of 0, since 1 means >>>>>> MIPI CSI2 interface, while 0 means CPI parallel interface. >>>>>> >>>>>> In the ov5640_set_power_mipi() the interface should obviously be set >>>>>> to MIPI CSI2 since this functions is used to power up the sensor when >>>>>> operated in MIPI CSI2 mode. The sensor should not be in CPI mode in >>>>>> that case. >>>>> >>>>> Does this actually help fixing your 'first frame corrupted issue' ? >>>> >>>> Yes it does. >>>> > > Do you think it's worth mentioning it in the commit message ? Done in V2 >>>>> I think the logic here was to power up the interface here in >>>>> ov5640_set_power_mipi() with the CSI-2 interface disabled to enter >>>>> LP-11 mode (something some receivers like the imx6 one are >>>>> particularly sensible to). >>>> >>>> Per OV5640 datasheet. >>>> >>>> Register 0x300e bit 2 selects sensor interface mode between MIPI CSI2 and >>>> CPI (parallel), it has nothing to do with LP modes . >>>> >>>> Register 0x3019 bits [6:4] control LP00/LP11 mode on CSI2 lines. >>>> >>>>> Then at stream time the CSI-2 interface is actually enabled in >>>>> ov5640_set_stream_mipi() just before streaming is started. >>>>> >>>>> Also the register documentation is very confusing and as reported in >>>>> ov5640_set_stream_mipi() it is also probably wrong (at least in the >>>>> datasheet version I have). >>>>> >>>>> I would be particularly cautious in touching this sequence as it has >>>>> been validated to work with multiple receivers. Of course if it >>>>> actually fixes an issue for you it should be taken in, but ideally, as >>>>> this sensor is still used in a large number of evaluation boards, it >>>>> should be validated by other consumers of this driver (st, imx, >>>>> microchip and rensas to name a few). > > Ok, I've been able to test on i.MX6Q which I was concerned for because > of its sensitivness to LP-11 detection. > > Let alone that imx6 with ov5640 is broken on mainline because of > unrelated reasons [1] I've been able to confirm that the sensor still > works on this platform > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > [Test on imx6q] > Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Can you confirm you have tested with iMX8MP as well ? I tested this great sensor on MN/MP/STM32MP15xx [...]
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 8f21839b08c78..8b7ff2f3bdda7 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -2539,9 +2539,9 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on) * "ov5640_set_stream_mipi()") * [4] = 0 : Power up MIPI HS Tx * [3] = 0 : Power up MIPI LS Rx - * [2] = 0 : MIPI interface disabled + * [2] = 1 : MIPI interface enabled */ - ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40); + ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x44); if (ret) return ret;
Set OV5640_REG_IO_MIPI_CTRL00 bit 2 to 1 instead of 0, since 1 means MIPI CSI2 interface, while 0 means CPI parallel interface. In the ov5640_set_power_mipi() the interface should obviously be set to MIPI CSI2 since this functions is used to power up the sensor when operated in MIPI CSI2 mode. The sensor should not be in CPI mode in that case. Fixes: aa4bb8b8838f ("media: ov5640: Re-work MIPI startup sequence") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl> Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Steve Longerbeam <slongerbeam@gmail.com> Cc: linux-media@vger.kernel.org --- drivers/media/i2c/ov5640.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)