| Message ID | 20200904201835.5958-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ov5640 feature enhancement and fixes | expand |
Hi Prabhakar, As discussed separately I would prefer to better understand issue before going to this patch. Nevertheless I have some remarks in code in case we'll need it at the end. On 9/4/20 10:18 PM, Lad Prabhakar wrote: > Keep the sensor in software power down mode and wake up only in > ov5640_set_stream_dvp() callback. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > Tested-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/media/i2c/ov5640.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 2fe4a7ac0592..880fde73a030 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -34,6 +34,8 @@ > #define OV5640_REG_SYS_RESET02 0x3002 > #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 > #define OV5640_REG_SYS_CTRL0 0x3008 > +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 > +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 For the time being this section was only referring to registers addresses and bit details was explained into a comment right before affectation, see OV5640_REG_IO_MIPI_CTRL00 for example. > #define OV5640_REG_CHIP_ID 0x300a > #define OV5640_REG_IO_MIPI_CTRL00 0x300e > #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 > @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, > val = regs->val; > mask = regs->mask; > > + /* remain in power down mode for DVP */ > + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && > + val == OV5640_REG_SYS_CTRL0_SW_PWUP && > + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) > + continue; > + I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) has been partially removed from big init sequence: for power-up part, but power-dwn remains at very beginning of sequence. We should completely remove 0x3008 from init sequence, including power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that should be called at the right place instead. > if (mask) > ret = ov5640_mod_reg(sensor, reg_addr, mask, val); > else > @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) > * PAD OUTPUT ENABLE 02 > * - [7:2]: D[5:0] output enable > */ > - return ov5640_write_reg(sensor, > - OV5640_REG_PAD_OUTPUT_ENABLE02, > - on ? 0xfc : 0); > + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, > + on ? 0xfc : 0); > + if (ret) > + return ret; > + > + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? > + OV5640_REG_SYS_CTRL0_SW_PWUP : > + OV5640_REG_SYS_CTRL0_SW_PWDN); > } > > static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > BR, Hugues.
On 9/9/20 7:16 PM, Hugues FRUCHET wrote: > Hi Prabhakar, > > As discussed separately I would prefer to better understand issue before > going to this patch. > Nevertheless I have some remarks in code in case we'll need it at the end. > > On 9/4/20 10:18 PM, Lad Prabhakar wrote: >> Keep the sensor in software power down mode and wake up only in >> ov5640_set_stream_dvp() callback. >> >> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> >> Tested-by: Jacopo Mondi <jacopo@jmondi.org> >> --- >> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++--- >> 1 file changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index 2fe4a7ac0592..880fde73a030 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -34,6 +34,8 @@ >> #define OV5640_REG_SYS_RESET02 0x3002 >> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 >> #define OV5640_REG_SYS_CTRL0 0x3008 >> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 >> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 > > For the time being this section was only referring to registers > addresses and bit details was explained into a comment right before > affectation, see OV5640_REG_IO_MIPI_CTRL00 for example. > >> #define OV5640_REG_CHIP_ID 0x300a >> #define OV5640_REG_IO_MIPI_CTRL00 0x300e >> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 >> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, >> val = regs->val; >> mask = regs->mask; >> >> + /* remain in power down mode for DVP */ >> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && >> + val == OV5640_REG_SYS_CTRL0_SW_PWUP && >> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) >> + continue; >> + > > I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) > has been partially removed from big init sequence: for power-up part, > but power-dwn remains at very beginning of sequence. > We should completely remove 0x3008 from init sequence, including > power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that > should be called at the right place instead. > > >> if (mask) >> ret = ov5640_mod_reg(sensor, reg_addr, mask, val); >> else >> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) >> * PAD OUTPUT ENABLE 02 >> * - [7:2]: D[5:0] output enable >> */ >> - return ov5640_write_reg(sensor, >> - OV5640_REG_PAD_OUTPUT_ENABLE02, >> - on ? 0xfc : 0); >> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, >> + on ? 0xfc : 0); >> + if (ret) >> + return ret; >> + >> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? >> + OV5640_REG_SYS_CTRL0_SW_PWUP : >> + OV5640_REG_SYS_CTRL0_SW_PWDN); >> } >> >> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) >> > > > BR, > Hugues. > Hello everyone, When I updated driver in my tree with this patch, I noticed that my setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped working. It looks like the culprit is this patch. I am not sure which is the best way to fix it. It looks like initially things work fine when probing the driver, but when we want to start streaming at a later point, the register SYS_CTRL0 cannot be written anymore. Here is an output of the log: --- Opening /dev/video0... Trying source module v4l2... /dev/video0 opened. No input was specified, using the first. ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121 Error starting stream. VIDIOC_STREAMON: Remote I/O error Unable to use mmap. Using read instead. Unable to use read. I am using a simple application to read from /dev/video0 (which is registered by the atmel-isc once the sensor probed) I have a feeling that something is wrong related to power, but I cannot tell for sure. Reverting this patch makes it work fine again. Help is appreciated, Thanks, Eugen
On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote: > On 9/9/20 7:16 PM, Hugues FRUCHET wrote: >> Hi Prabhakar, >> >> As discussed separately I would prefer to better understand issue before >> going to this patch. >> Nevertheless I have some remarks in code in case we'll need it at the end. >> >> On 9/4/20 10:18 PM, Lad Prabhakar wrote: >>> Keep the sensor in software power down mode and wake up only in >>> ov5640_set_stream_dvp() callback. >>> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> >>> Tested-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++--- >>> 1 file changed, 16 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >>> index 2fe4a7ac0592..880fde73a030 100644 >>> --- a/drivers/media/i2c/ov5640.c >>> +++ b/drivers/media/i2c/ov5640.c >>> @@ -34,6 +34,8 @@ >>> #define OV5640_REG_SYS_RESET02 0x3002 >>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 >>> #define OV5640_REG_SYS_CTRL0 0x3008 >>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 >>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 >> >> For the time being this section was only referring to registers >> addresses and bit details was explained into a comment right before >> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example. >> >>> #define OV5640_REG_CHIP_ID 0x300a >>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e >>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 >>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, >>> val = regs->val; >>> mask = regs->mask; >>> >>> + /* remain in power down mode for DVP */ >>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && >>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP && >>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) >>> + continue; >>> + >> >> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) >> has been partially removed from big init sequence: for power-up part, >> but power-dwn remains at very beginning of sequence. >> We should completely remove 0x3008 from init sequence, including >> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that >> should be called at the right place instead. >> >> >>> if (mask) >>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val); >>> else >>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) >>> * PAD OUTPUT ENABLE 02 >>> * - [7:2]: D[5:0] output enable >>> */ >>> - return ov5640_write_reg(sensor, >>> - OV5640_REG_PAD_OUTPUT_ENABLE02, >>> - on ? 0xfc : 0); >>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, >>> + on ? 0xfc : 0); >>> + if (ret) >>> + return ret; >>> + >>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? >>> + OV5640_REG_SYS_CTRL0_SW_PWUP : >>> + OV5640_REG_SYS_CTRL0_SW_PWDN); >>> } >>> >>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) >>> >> >> >> BR, >> Hugues. >> > > > Hello everyone, > > When I updated driver in my tree with this patch, I noticed that my > setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped > working. > > It looks like the culprit is this patch. > I am not sure which is the best way to fix it. > It looks like initially things work fine when probing the driver, but > when we want to start streaming at a later point, the register SYS_CTRL0 > cannot be written anymore. > Here is an output of the log: > > --- Opening /dev/video0... > Trying source module v4l2... > /dev/video0 opened. > No input was specified, using the first. > ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 > atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121 > Error starting stream. > VIDIOC_STREAMON: Remote I/O error > Unable to use mmap. Using read instead. > Unable to use read. > > I am using a simple application to read from /dev/video0 (which is > registered by the atmel-isc once the sensor probed) > > I have a feeling that something is wrong related to power, but I cannot > tell for sure. > > Reverting this patch makes it work fine again. > > Help is appreciated, > Thanks, > Eugen > Gentle ping. I would like to send a revert patch if nobody cares about this driver/patch except me. Thanks, Eugen
Hi Hugues, Eugen, On Tue, Dec 21, 2021 at 07:37:47AM +0000, Eugen.Hristev@microchip.com wrote: > On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote: > > On 9/9/20 7:16 PM, Hugues FRUCHET wrote: > >> Hi Prabhakar, > >> > >> As discussed separately I would prefer to better understand issue before > >> going to this patch. > >> Nevertheless I have some remarks in code in case we'll need it at the end. > >> > >> On 9/4/20 10:18 PM, Lad Prabhakar wrote: > >>> Keep the sensor in software power down mode and wake up only in > >>> ov5640_set_stream_dvp() callback. > >>> > >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> Tested-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++--- > >>> 1 file changed, 16 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > >>> index 2fe4a7ac0592..880fde73a030 100644 > >>> --- a/drivers/media/i2c/ov5640.c > >>> +++ b/drivers/media/i2c/ov5640.c > >>> @@ -34,6 +34,8 @@ > >>> #define OV5640_REG_SYS_RESET02 0x3002 > >>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 > >>> #define OV5640_REG_SYS_CTRL0 0x3008 > >>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 > >>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 > >> > >> For the time being this section was only referring to registers > >> addresses and bit details was explained into a comment right before > >> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example. > >> > >>> #define OV5640_REG_CHIP_ID 0x300a > >>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e > >>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 > >>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, > >>> val = regs->val; > >>> mask = regs->mask; > >>> > >>> + /* remain in power down mode for DVP */ > >>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && > >>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP && > >>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) > >>> + continue; > >>> + > >> > >> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) > >> has been partially removed from big init sequence: for power-up part, > >> but power-dwn remains at very beginning of sequence. > >> We should completely remove 0x3008 from init sequence, including > >> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that > >> should be called at the right place instead. > >> > >> > >>> if (mask) > >>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val); > >>> else > >>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) > >>> * PAD OUTPUT ENABLE 02 > >>> * - [7:2]: D[5:0] output enable > >>> */ > >>> - return ov5640_write_reg(sensor, > >>> - OV5640_REG_PAD_OUTPUT_ENABLE02, > >>> - on ? 0xfc : 0); > >>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, > >>> + on ? 0xfc : 0); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? > >>> + OV5640_REG_SYS_CTRL0_SW_PWUP : > >>> + OV5640_REG_SYS_CTRL0_SW_PWDN); > >>> } > >>> > >>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > >>> > >> > >> > >> BR, > >> Hugues. > >> > > > > > > Hello everyone, > > > > When I updated driver in my tree with this patch, I noticed that my > > setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped > > working. > > > > It looks like the culprit is this patch. > > I am not sure which is the best way to fix it. > > It looks like initially things work fine when probing the driver, but > > when we want to start streaming at a later point, the register SYS_CTRL0 > > cannot be written anymore. > > Here is an output of the log: > > > > --- Opening /dev/video0... > > Trying source module v4l2... > > /dev/video0 opened. > > No input was specified, using the first. > > ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 > > atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121 > > Error starting stream. > > VIDIOC_STREAMON: Remote I/O error > > Unable to use mmap. Using read instead. > > Unable to use read. > > > > I am using a simple application to read from /dev/video0 (which is > > registered by the atmel-isc once the sensor probed) > > > > I have a feeling that something is wrong related to power, but I cannot > > tell for sure. > > > > Reverting this patch makes it work fine again. > > > > Help is appreciated, > > Thanks, > > Eugen > > > > > Gentle ping. > > I would like to send a revert patch if nobody cares about this > driver/patch except me. Hugues: any thoughts on this? Without knowing much about the sensor, the stated intent of the patch seems reasonable. But if it breaks things, then reverting it sounds like a good idea until a proper fix is available.
Hi, Sorry for the delay. On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote: > > On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote: > > On 9/9/20 7:16 PM, Hugues FRUCHET wrote: > >> Hi Prabhakar, > >> > >> As discussed separately I would prefer to better understand issue before > >> going to this patch. > >> Nevertheless I have some remarks in code in case we'll need it at the end. > >> > >> On 9/4/20 10:18 PM, Lad Prabhakar wrote: > >>> Keep the sensor in software power down mode and wake up only in > >>> ov5640_set_stream_dvp() callback. > >>> > >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> Tested-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++--- > >>> 1 file changed, 16 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > >>> index 2fe4a7ac0592..880fde73a030 100644 > >>> --- a/drivers/media/i2c/ov5640.c > >>> +++ b/drivers/media/i2c/ov5640.c > >>> @@ -34,6 +34,8 @@ > >>> #define OV5640_REG_SYS_RESET02 0x3002 > >>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 > >>> #define OV5640_REG_SYS_CTRL0 0x3008 > >>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 > >>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 > >> > >> For the time being this section was only referring to registers > >> addresses and bit details was explained into a comment right before > >> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example. > >> > >>> #define OV5640_REG_CHIP_ID 0x300a > >>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e > >>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 > >>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, > >>> val = regs->val; > >>> mask = regs->mask; > >>> > >>> + /* remain in power down mode for DVP */ > >>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && > >>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP && > >>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) > >>> + continue; > >>> + > >> > >> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) > >> has been partially removed from big init sequence: for power-up part, > >> but power-dwn remains at very beginning of sequence. > >> We should completely remove 0x3008 from init sequence, including > >> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that > >> should be called at the right place instead. > >> > >> > >>> if (mask) > >>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val); > >>> else > >>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) > >>> * PAD OUTPUT ENABLE 02 > >>> * - [7:2]: D[5:0] output enable > >>> */ > >>> - return ov5640_write_reg(sensor, > >>> - OV5640_REG_PAD_OUTPUT_ENABLE02, > >>> - on ? 0xfc : 0); > >>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, > >>> + on ? 0xfc : 0); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? > >>> + OV5640_REG_SYS_CTRL0_SW_PWUP : > >>> + OV5640_REG_SYS_CTRL0_SW_PWDN); > >>> } > >>> > >>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > >>> > >> > >> > >> BR, > >> Hugues. > >> > > > > > > Hello everyone, > > > > When I updated driver in my tree with this patch, I noticed that my > > setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped > > working. > > > > It looks like the culprit is this patch. > > I am not sure which is the best way to fix it. > > It looks like initially things work fine when probing the driver, but > > when we want to start streaming at a later point, the register SYS_CTRL0 > > cannot be written anymore. > > Here is an output of the log: > > > > --- Opening /dev/video0... > > Trying source module v4l2... > > /dev/video0 opened. > > No input was specified, using the first. > > ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 > > atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121 > > Error starting stream. > > VIDIOC_STREAMON: Remote I/O error > > Unable to use mmap. Using read instead. > > Unable to use read. > > > > I am using a simple application to read from /dev/video0 (which is > > registered by the atmel-isc once the sensor probed) > > > > I have a feeling that something is wrong related to power, but I cannot > > tell for sure. > > > > Reverting this patch makes it work fine again. > > > > Help is appreciated, > > Thanks, > > Eugen > > > > > Gentle ping. > > I would like to send a revert patch if nobody cares about this > driver/patch except me. > I powered up the Renesas RZ/G1H connected with an ov5640 sensor and was able to capture the video data [0] using the yavta application. I'm fine with reverting the patch too as this works fine as well. Just some suggestions: - What is the application you are trying to use for capturing? - Have you tried reducing the i2c clock speed? - Can you try and do a dummy write for some other register in ov5640_set_stream_dvp() to see if i2c writes are failing for all regs? - Can you add ov5640_read_reg() in ov5640_write_reg() when i2c_transfer() fails to check if read too is failing. [0] https://paste.debian.net/1224317/ Cheers, Prabhakar
On 12/21/21 4:47 PM, Lad, Prabhakar wrote: > Hi, > > Sorry for the delay. > > On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote: >> >> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote: >>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote: >>>> Hi Prabhakar, >>>> >>>> As discussed separately I would prefer to better understand issue before >>>> going to this patch. >>>> Nevertheless I have some remarks in code in case we'll need it at the end. >>>> >>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote: >>>>> Keep the sensor in software power down mode and wake up only in >>>>> ov5640_set_stream_dvp() callback. >>>>> >>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> >>>>> Tested-by: Jacopo Mondi <jacopo@jmondi.org> >>>>> --- >>>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++--- >>>>> 1 file changed, 16 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >>>>> index 2fe4a7ac0592..880fde73a030 100644 >>>>> --- a/drivers/media/i2c/ov5640.c >>>>> +++ b/drivers/media/i2c/ov5640.c >>>>> @@ -34,6 +34,8 @@ >>>>> #define OV5640_REG_SYS_RESET02 0x3002 >>>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 >>>>> #define OV5640_REG_SYS_CTRL0 0x3008 >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 >>>> >>>> For the time being this section was only referring to registers >>>> addresses and bit details was explained into a comment right before >>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example. >>>> >>>>> #define OV5640_REG_CHIP_ID 0x300a >>>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e >>>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 >>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, >>>>> val = regs->val; >>>>> mask = regs->mask; >>>>> >>>>> + /* remain in power down mode for DVP */ >>>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && >>>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP && >>>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) >>>>> + continue; >>>>> + >>>> >>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) >>>> has been partially removed from big init sequence: for power-up part, >>>> but power-dwn remains at very beginning of sequence. >>>> We should completely remove 0x3008 from init sequence, including >>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that >>>> should be called at the right place instead. >>>> >>>> >>>>> if (mask) >>>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val); >>>>> else >>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) >>>>> * PAD OUTPUT ENABLE 02 >>>>> * - [7:2]: D[5:0] output enable >>>>> */ >>>>> - return ov5640_write_reg(sensor, >>>>> - OV5640_REG_PAD_OUTPUT_ENABLE02, >>>>> - on ? 0xfc : 0); >>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, >>>>> + on ? 0xfc : 0); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? >>>>> + OV5640_REG_SYS_CTRL0_SW_PWUP : >>>>> + OV5640_REG_SYS_CTRL0_SW_PWDN); >>>>> } >>>>> >>>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) >>>>> >>>> >>>> >>>> BR, >>>> Hugues. >>>> >>> >>> >>> Hello everyone, >>> >>> When I updated driver in my tree with this patch, I noticed that my >>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped >>> working. >>> >>> It looks like the culprit is this patch. >>> I am not sure which is the best way to fix it. >>> It looks like initially things work fine when probing the driver, but >>> when we want to start streaming at a later point, the register SYS_CTRL0 >>> cannot be written anymore. >>> Here is an output of the log: >>> >>> --- Opening /dev/video0... >>> Trying source module v4l2... >>> /dev/video0 opened. >>> No input was specified, using the first. >>> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 >>> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121 >>> Error starting stream. >>> VIDIOC_STREAMON: Remote I/O error >>> Unable to use mmap. Using read instead. >>> Unable to use read. >>> >>> I am using a simple application to read from /dev/video0 (which is >>> registered by the atmel-isc once the sensor probed) >>> >>> I have a feeling that something is wrong related to power, but I cannot >>> tell for sure. >>> >>> Reverting this patch makes it work fine again. >>> >>> Help is appreciated, >>> Thanks, >>> Eugen >>> >> >> >> Gentle ping. >> >> I would like to send a revert patch if nobody cares about this >> driver/patch except me. >> > I powered up the Renesas RZ/G1H connected with an ov5640 sensor and > was able to capture the video data [0] using the yavta application. > I'm fine with reverting the patch too as this works fine as well. Hi Prabhakar, Thanks for trying this out. ov5640 works in both parallel or CSI2 mode. Looking at the board, it looks a parallel connection but I am not 100%, you tested using parallel interface ? > > Just some suggestions: > - What is the application you are trying to use for capturing? I was using a simple app that reads from /dev/video0, it's called fswebcam. I can try more apps if it's needed. > - Have you tried reducing the i2c clock speed? I did not, but without this patch, there is no problem whatsoever, and I have been using this sensor since around 4.9 kernel version. > - Can you try and do a dummy write for some other register in > ov5640_set_stream_dvp() to see if i2c writes are failing for all regs? I can try > - Can you add ov5640_read_reg() in ov5640_write_reg() when > i2c_transfer() fails to check if read too is failing. > > [0] https://paste.debian.net/1224317/ You seem to be able to capture successfully, I have a feeling that in my case the sensor is somehow not powered up when the capture is being requested. Could you share a snippet of your device tree node for the sensor so I can have a look ? Thanks, Eugen > > Cheers, > Prabhakar >
Hi Eugen, On Tue, Dec 21, 2021 at 3:02 PM <Eugen.Hristev@microchip.com> wrote: > > On 12/21/21 4:47 PM, Lad, Prabhakar wrote: > > Hi, > > > > Sorry for the delay. > > > > On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote: > >> > >> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote: > >>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote: > >>>> Hi Prabhakar, > >>>> > >>>> As discussed separately I would prefer to better understand issue before > >>>> going to this patch. > >>>> Nevertheless I have some remarks in code in case we'll need it at the end. > >>>> > >>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote: > >>>>> Keep the sensor in software power down mode and wake up only in > >>>>> ov5640_set_stream_dvp() callback. > >>>>> > >>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > >>>>> Tested-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>> --- > >>>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++--- > >>>>> 1 file changed, 16 insertions(+), 3 deletions(-) > >>>>> > >>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > >>>>> index 2fe4a7ac0592..880fde73a030 100644 > >>>>> --- a/drivers/media/i2c/ov5640.c > >>>>> +++ b/drivers/media/i2c/ov5640.c > >>>>> @@ -34,6 +34,8 @@ > >>>>> #define OV5640_REG_SYS_RESET02 0x3002 > >>>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 > >>>>> #define OV5640_REG_SYS_CTRL0 0x3008 > >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 > >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 > >>>> > >>>> For the time being this section was only referring to registers > >>>> addresses and bit details was explained into a comment right before > >>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example. > >>>> > >>>>> #define OV5640_REG_CHIP_ID 0x300a > >>>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e > >>>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 > >>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, > >>>>> val = regs->val; > >>>>> mask = regs->mask; > >>>>> > >>>>> + /* remain in power down mode for DVP */ > >>>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && > >>>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP && > >>>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) > >>>>> + continue; > >>>>> + > >>>> > >>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) > >>>> has been partially removed from big init sequence: for power-up part, > >>>> but power-dwn remains at very beginning of sequence. > >>>> We should completely remove 0x3008 from init sequence, including > >>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that > >>>> should be called at the right place instead. > >>>> > >>>> > >>>>> if (mask) > >>>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val); > >>>>> else > >>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) > >>>>> * PAD OUTPUT ENABLE 02 > >>>>> * - [7:2]: D[5:0] output enable > >>>>> */ > >>>>> - return ov5640_write_reg(sensor, > >>>>> - OV5640_REG_PAD_OUTPUT_ENABLE02, > >>>>> - on ? 0xfc : 0); > >>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, > >>>>> + on ? 0xfc : 0); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? > >>>>> + OV5640_REG_SYS_CTRL0_SW_PWUP : > >>>>> + OV5640_REG_SYS_CTRL0_SW_PWDN); > >>>>> } > >>>>> > >>>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > >>>>> > >>>> > >>>> > >>>> BR, > >>>> Hugues. > >>>> > >>> > >>> > >>> Hello everyone, > >>> > >>> When I updated driver in my tree with this patch, I noticed that my > >>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped > >>> working. > >>> > >>> It looks like the culprit is this patch. > >>> I am not sure which is the best way to fix it. > >>> It looks like initially things work fine when probing the driver, but > >>> when we want to start streaming at a later point, the register SYS_CTRL0 > >>> cannot be written anymore. > >>> Here is an output of the log: > >>> > >>> --- Opening /dev/video0... > >>> Trying source module v4l2... > >>> /dev/video0 opened. > >>> No input was specified, using the first. > >>> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 > >>> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121 > >>> Error starting stream. > >>> VIDIOC_STREAMON: Remote I/O error > >>> Unable to use mmap. Using read instead. > >>> Unable to use read. > >>> > >>> I am using a simple application to read from /dev/video0 (which is > >>> registered by the atmel-isc once the sensor probed) > >>> > >>> I have a feeling that something is wrong related to power, but I cannot > >>> tell for sure. > >>> > >>> Reverting this patch makes it work fine again. > >>> > >>> Help is appreciated, > >>> Thanks, > >>> Eugen > >>> > >> > >> > >> Gentle ping. > >> > >> I would like to send a revert patch if nobody cares about this > >> driver/patch except me. > >> > > I powered up the Renesas RZ/G1H connected with an ov5640 sensor and > > was able to capture the video data [0] using the yavta application. > > I'm fine with reverting the patch too as this works fine as well. > > Hi Prabhakar, > > Thanks for trying this out. > ov5640 works in both parallel or CSI2 mode. Looking at the board, it > looks a parallel connection but I am not 100%, you tested using parallel > interface ? > I have tested it in parallel interface mode as RZ/G1H supports parallel capture > > > > Just some suggestions: > > - What is the application you are trying to use for capturing? > > I was using a simple app that reads from /dev/video0, it's called > fswebcam. I can try more apps if it's needed. > could you give it a shot with yavta please. > > - Have you tried reducing the i2c clock speed? > > I did not, but without this patch, there is no problem whatsoever, and I > have been using this sensor since around 4.9 kernel version. > Agreed, I'm thinking aloud just to narrow things. > > - Can you try and do a dummy write for some other register in > > ov5640_set_stream_dvp() to see if i2c writes are failing for all regs? > > I can try > Thank you. > > - Can you add ov5640_read_reg() in ov5640_write_reg() when > > i2c_transfer() fails to check if read too is failing. > > > > [0] https://paste.debian.net/1224317/ > > You seem to be able to capture successfully, I have a feeling that in my > case the sensor is somehow not powered up when the capture is being > requested. > Could you share a snippet of your device tree node for the sensor so I > can have a look ? > [0] contains the bridge node and [1] contains the sensor node. [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309 [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6 Cheers, Prabhakar
On 12/21/21 5:11 PM, Lad, Prabhakar wrote: > Hi Eugen, > > On Tue, Dec 21, 2021 at 3:02 PM <Eugen.Hristev@microchip.com> wrote: >> >> On 12/21/21 4:47 PM, Lad, Prabhakar wrote: >>> Hi, >>> >>> Sorry for the delay. >>> >>> On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote: >>>> >>>> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote: >>>>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote: >>>>>> Hi Prabhakar, >>>>>> >>>>>> As discussed separately I would prefer to better understand issue before >>>>>> going to this patch. >>>>>> Nevertheless I have some remarks in code in case we'll need it at the end. >>>>>> >>>>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote: >>>>>>> Keep the sensor in software power down mode and wake up only in >>>>>>> ov5640_set_stream_dvp() callback. >>>>>>> >>>>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>>>>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> >>>>>>> Tested-by: Jacopo Mondi <jacopo@jmondi.org> >>>>>>> --- >>>>>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++--- >>>>>>> 1 file changed, 16 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >>>>>>> index 2fe4a7ac0592..880fde73a030 100644 >>>>>>> --- a/drivers/media/i2c/ov5640.c >>>>>>> +++ b/drivers/media/i2c/ov5640.c >>>>>>> @@ -34,6 +34,8 @@ >>>>>>> #define OV5640_REG_SYS_RESET02 0x3002 >>>>>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 >>>>>>> #define OV5640_REG_SYS_CTRL0 0x3008 >>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 >>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 >>>>>> >>>>>> For the time being this section was only referring to registers >>>>>> addresses and bit details was explained into a comment right before >>>>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example. >>>>>> >>>>>>> #define OV5640_REG_CHIP_ID 0x300a >>>>>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e >>>>>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 >>>>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, >>>>>>> val = regs->val; >>>>>>> mask = regs->mask; >>>>>>> >>>>>>> + /* remain in power down mode for DVP */ >>>>>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && >>>>>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP && >>>>>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) >>>>>>> + continue; >>>>>>> + >>>>>> >>>>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) >>>>>> has been partially removed from big init sequence: for power-up part, >>>>>> but power-dwn remains at very beginning of sequence. >>>>>> We should completely remove 0x3008 from init sequence, including >>>>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that >>>>>> should be called at the right place instead. >>>>>> >>>>>> >>>>>>> if (mask) >>>>>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val); >>>>>>> else >>>>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) >>>>>>> * PAD OUTPUT ENABLE 02 >>>>>>> * - [7:2]: D[5:0] output enable >>>>>>> */ >>>>>>> - return ov5640_write_reg(sensor, >>>>>>> - OV5640_REG_PAD_OUTPUT_ENABLE02, >>>>>>> - on ? 0xfc : 0); >>>>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, >>>>>>> + on ? 0xfc : 0); >>>>>>> + if (ret) >>>>>>> + return ret; >>>>>>> + >>>>>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? >>>>>>> + OV5640_REG_SYS_CTRL0_SW_PWUP : >>>>>>> + OV5640_REG_SYS_CTRL0_SW_PWDN); >>>>>>> } >>>>>>> >>>>>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) >>>>>>> >>>>>> >>>>>> >>>>>> BR, >>>>>> Hugues. >>>>>> >>>>> >>>>> >>>>> Hello everyone, >>>>> >>>>> When I updated driver in my tree with this patch, I noticed that my >>>>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped >>>>> working. >>>>> >>>>> It looks like the culprit is this patch. >>>>> I am not sure which is the best way to fix it. >>>>> It looks like initially things work fine when probing the driver, but >>>>> when we want to start streaming at a later point, the register SYS_CTRL0 >>>>> cannot be written anymore. >>>>> Here is an output of the log: >>>>> >>>>> --- Opening /dev/video0... >>>>> Trying source module v4l2... >>>>> /dev/video0 opened. >>>>> No input was specified, using the first. >>>>> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 >>>>> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121 >>>>> Error starting stream. >>>>> VIDIOC_STREAMON: Remote I/O error >>>>> Unable to use mmap. Using read instead. >>>>> Unable to use read. >>>>> >>>>> I am using a simple application to read from /dev/video0 (which is >>>>> registered by the atmel-isc once the sensor probed) >>>>> >>>>> I have a feeling that something is wrong related to power, but I cannot >>>>> tell for sure. >>>>> >>>>> Reverting this patch makes it work fine again. >>>>> >>>>> Help is appreciated, >>>>> Thanks, >>>>> Eugen >>>>> >>>> >>>> >>>> Gentle ping. >>>> >>>> I would like to send a revert patch if nobody cares about this >>>> driver/patch except me. >>>> >>> I powered up the Renesas RZ/G1H connected with an ov5640 sensor and >>> was able to capture the video data [0] using the yavta application. >>> I'm fine with reverting the patch too as this works fine as well. >> >> Hi Prabhakar, >> >> Thanks for trying this out. >> ov5640 works in both parallel or CSI2 mode. Looking at the board, it >> looks a parallel connection but I am not 100%, you tested using parallel >> interface ? >> > I have tested it in parallel interface mode as RZ/G1H supports parallel capture > >>> >>> Just some suggestions: >>> - What is the application you are trying to use for capturing? >> >> I was using a simple app that reads from /dev/video0, it's called >> fswebcam. I can try more apps if it's needed. >> > could you give it a shot with yavta please. Hello Lad, I debugged this further, and I have some news: It looks like the 'write 0x2 to SYS_CLTR0' does not fail itself, rather the sensor refuses to accept a power up. I tried to read the register before the write, and it reads 0x42. Then, I tried to write 0x42 back, and it works fine. So, I do not think there is a problem with i2c communication. The only problem is that the sensor refuses to power up (accept the 0x2 into the SYS_CTRL_0 ), due to an unknown (to me) reason. If the power up is performed at the initialization phase, it works. I also tried to capture with v4l2-ctl, and the result is the same. Which of the init configuration set of registers your test is using? It may be that it does not work in a specific config . The datasheet which I have does not claim that the 'power up' might fail in some circumstances. Thanks for helping, Eugen > >>> - Have you tried reducing the i2c clock speed? >> >> I did not, but without this patch, there is no problem whatsoever, and I >> have been using this sensor since around 4.9 kernel version. >> > Agreed, I'm thinking aloud just to narrow things. > >>> - Can you try and do a dummy write for some other register in >>> ov5640_set_stream_dvp() to see if i2c writes are failing for all regs? >> >> I can try >> > Thank you. > >>> - Can you add ov5640_read_reg() in ov5640_write_reg() when >>> i2c_transfer() fails to check if read too is failing. >>> >>> [0] https://paste.debian.net/1224317/ >> >> You seem to be able to capture successfully, I have a feeling that in my >> case the sensor is somehow not powered up when the capture is being >> requested. >> Could you share a snippet of your device tree node for the sensor so I >> can have a look ? >> > [0] contains the bridge node and [1] contains the sensor node. > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309 > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6 > > Cheers, > Prabhakar >
Hi Eugen, On Mon, Jan 3, 2022 at 11:29 AM <Eugen.Hristev@microchip.com> wrote: > > On 12/21/21 5:11 PM, Lad, Prabhakar wrote: > > Hi Eugen, > > > > On Tue, Dec 21, 2021 at 3:02 PM <Eugen.Hristev@microchip.com> wrote: > >> > >> On 12/21/21 4:47 PM, Lad, Prabhakar wrote: > >>> Hi, > >>> > >>> Sorry for the delay. > >>> > >>> On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote: > >>>> > >>>> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote: > >>>>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote: > >>>>>> Hi Prabhakar, > >>>>>> > >>>>>> As discussed separately I would prefer to better understand issue before > >>>>>> going to this patch. > >>>>>> Nevertheless I have some remarks in code in case we'll need it at the end. > >>>>>> > >>>>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote: > >>>>>>> Keep the sensor in software power down mode and wake up only in > >>>>>>> ov5640_set_stream_dvp() callback. > >>>>>>> > >>>>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>>>>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > >>>>>>> Tested-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>>>> --- > >>>>>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++--- > >>>>>>> 1 file changed, 16 insertions(+), 3 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > >>>>>>> index 2fe4a7ac0592..880fde73a030 100644 > >>>>>>> --- a/drivers/media/i2c/ov5640.c > >>>>>>> +++ b/drivers/media/i2c/ov5640.c > >>>>>>> @@ -34,6 +34,8 @@ > >>>>>>> #define OV5640_REG_SYS_RESET02 0x3002 > >>>>>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 > >>>>>>> #define OV5640_REG_SYS_CTRL0 0x3008 > >>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 > >>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 > >>>>>> > >>>>>> For the time being this section was only referring to registers > >>>>>> addresses and bit details was explained into a comment right before > >>>>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example. > >>>>>> > >>>>>>> #define OV5640_REG_CHIP_ID 0x300a > >>>>>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e > >>>>>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 > >>>>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, > >>>>>>> val = regs->val; > >>>>>>> mask = regs->mask; > >>>>>>> > >>>>>>> + /* remain in power down mode for DVP */ > >>>>>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && > >>>>>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP && > >>>>>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) > >>>>>>> + continue; > >>>>>>> + > >>>>>> > >>>>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) > >>>>>> has been partially removed from big init sequence: for power-up part, > >>>>>> but power-dwn remains at very beginning of sequence. > >>>>>> We should completely remove 0x3008 from init sequence, including > >>>>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that > >>>>>> should be called at the right place instead. > >>>>>> > >>>>>> > >>>>>>> if (mask) > >>>>>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val); > >>>>>>> else > >>>>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) > >>>>>>> * PAD OUTPUT ENABLE 02 > >>>>>>> * - [7:2]: D[5:0] output enable > >>>>>>> */ > >>>>>>> - return ov5640_write_reg(sensor, > >>>>>>> - OV5640_REG_PAD_OUTPUT_ENABLE02, > >>>>>>> - on ? 0xfc : 0); > >>>>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, > >>>>>>> + on ? 0xfc : 0); > >>>>>>> + if (ret) > >>>>>>> + return ret; > >>>>>>> + > >>>>>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? > >>>>>>> + OV5640_REG_SYS_CTRL0_SW_PWUP : > >>>>>>> + OV5640_REG_SYS_CTRL0_SW_PWDN); > >>>>>>> } > >>>>>>> > >>>>>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > >>>>>>> > >>>>>> > >>>>>> > >>>>>> BR, > >>>>>> Hugues. > >>>>>> > >>>>> > >>>>> > >>>>> Hello everyone, > >>>>> > >>>>> When I updated driver in my tree with this patch, I noticed that my > >>>>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped > >>>>> working. > >>>>> > >>>>> It looks like the culprit is this patch. > >>>>> I am not sure which is the best way to fix it. > >>>>> It looks like initially things work fine when probing the driver, but > >>>>> when we want to start streaming at a later point, the register SYS_CTRL0 > >>>>> cannot be written anymore. > >>>>> Here is an output of the log: > >>>>> > >>>>> --- Opening /dev/video0... > >>>>> Trying source module v4l2... > >>>>> /dev/video0 opened. > >>>>> No input was specified, using the first. > >>>>> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2 > >>>>> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121 > >>>>> Error starting stream. > >>>>> VIDIOC_STREAMON: Remote I/O error > >>>>> Unable to use mmap. Using read instead. > >>>>> Unable to use read. > >>>>> > >>>>> I am using a simple application to read from /dev/video0 (which is > >>>>> registered by the atmel-isc once the sensor probed) > >>>>> > >>>>> I have a feeling that something is wrong related to power, but I cannot > >>>>> tell for sure. > >>>>> > >>>>> Reverting this patch makes it work fine again. > >>>>> > >>>>> Help is appreciated, > >>>>> Thanks, > >>>>> Eugen > >>>>> > >>>> > >>>> > >>>> Gentle ping. > >>>> > >>>> I would like to send a revert patch if nobody cares about this > >>>> driver/patch except me. > >>>> > >>> I powered up the Renesas RZ/G1H connected with an ov5640 sensor and > >>> was able to capture the video data [0] using the yavta application. > >>> I'm fine with reverting the patch too as this works fine as well. > >> > >> Hi Prabhakar, > >> > >> Thanks for trying this out. > >> ov5640 works in both parallel or CSI2 mode. Looking at the board, it > >> looks a parallel connection but I am not 100%, you tested using parallel > >> interface ? > >> > > I have tested it in parallel interface mode as RZ/G1H supports parallel capture > > > >>> > >>> Just some suggestions: > >>> - What is the application you are trying to use for capturing? > >> > >> I was using a simple app that reads from /dev/video0, it's called > >> fswebcam. I can try more apps if it's needed. > >> > > could you give it a shot with yavta please. > > Hello Lad, > > I debugged this further, and I have some news: > > It looks like the 'write 0x2 to SYS_CLTR0' does not fail itself, rather > the sensor refuses to accept a power up. > > I tried to read the register before the write, and it reads 0x42. > Then, I tried to write 0x42 back, and it works fine. > So, I do not think there is a problem with i2c communication. > The only problem is that the sensor refuses to power up (accept the 0x2 > into the SYS_CTRL_0 ), due to an unknown (to me) reason. > That's strange. > If the power up is performed at the initialization phase, it works. > > I also tried to capture with v4l2-ctl, and the result is the same. > you mean yavta ? > Which of the init configuration set of registers your test is using? I have been testing 320x240 and 640x480. Could you give that a try please? > It may be that it does not work in a specific config . > > The datasheet which I have does not claim that the 'power up' might fail > in some circumstances. > Let me check if I can ping OmniVision FAE. > Thanks for helping, > > Eugen > > > > > >>> - Have you tried reducing the i2c clock speed? > >> > >> I did not, but without this patch, there is no problem whatsoever, and I > >> have been using this sensor since around 4.9 kernel version. > >> > > Agreed, I'm thinking aloud just to narrow things. > > > >>> - Can you try and do a dummy write for some other register in > >>> ov5640_set_stream_dvp() to see if i2c writes are failing for all regs? > >> > >> I can try > >> > > Thank you. > > > >>> - Can you add ov5640_read_reg() in ov5640_write_reg() when > >>> i2c_transfer() fails to check if read too is failing. > >>> > >>> [0] https://paste.debian.net/1224317/ > >> > >> You seem to be able to capture successfully, I have a feeling that in my > >> case the sensor is somehow not powered up when the capture is being > >> requested. > >> Could you share a snippet of your device tree node for the sensor so I > >> can have a look ? > >> > > [0] contains the bridge node and [1] contains the sensor node. > > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309 > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6 > > Cheers, Prabhakar
On Tue, Jan 4, 2022 at 9:57 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > > Hi Eugen, > > On Mon, Jan 3, 2022 at 11:29 AM <Eugen.Hristev@microchip.com> wrote: > > > > On 12/21/21 5:11 PM, Lad, Prabhakar wrote: > > > Hi Eugen, > > > <snip> > > > could you give it a shot with yavta please. > > > > Hello Lad, > > > > I debugged this further, and I have some news: > > > > It looks like the 'write 0x2 to SYS_CLTR0' does not fail itself, rather > > the sensor refuses to accept a power up. > > > > I tried to read the register before the write, and it reads 0x42. > > Then, I tried to write 0x42 back, and it works fine. > > So, I do not think there is a problem with i2c communication. > > The only problem is that the sensor refuses to power up (accept the 0x2 > > into the SYS_CTRL_0 ), due to an unknown (to me) reason. > > > That's strange. > > > If the power up is performed at the initialization phase, it works. > > > > I also tried to capture with v4l2-ctl, and the result is the same. > > > you mean yavta ? > > > Which of the init configuration set of registers your test is using? > I have been testing 320x240 and 640x480. Could you give that a try please? > > > It may be that it does not work in a specific config . > > > > The datasheet which I have does not claim that the 'power up' might fail > > in some circumstances. > > > Let me check if I can ping OmniVision FAE. > Fyi.. I got the below feedback from OmniVision FAE. SW standby bit is working as expected from my side. As far as the sensor initialization is concerned we use HW power up sequence defined in the datasheet followed by SW initialization. SW initialization consist of the following :- 78 3103 11 ; I2C timing ( do not modify) 78 3008 82 ; SW reset 78 3008 42 ; Stop streaming 78 ……. Sensor settings for required mode 78 3008 02 ; Start streaming Note:- 0x3008[7] SW reset bit is volatile, as soon as a reset is applied the all the bits are cleared and 0x3008[7:0] set to default value hence should read 0x2 Cheers, Prabhakar
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 2fe4a7ac0592..880fde73a030 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -34,6 +34,8 @@ #define OV5640_REG_SYS_RESET02 0x3002 #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006 #define OV5640_REG_SYS_CTRL0 0x3008 +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 #define OV5640_REG_CHIP_ID 0x300a #define OV5640_REG_IO_MIPI_CTRL00 0x300e #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017 @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, val = regs->val; mask = regs->mask; + /* remain in power down mode for DVP */ + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && + val == OV5640_REG_SYS_CTRL0_SW_PWUP && + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY) + continue; + if (mask) ret = ov5640_mod_reg(sensor, reg_addr, mask, val); else @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) * PAD OUTPUT ENABLE 02 * - [7:2]: D[5:0] output enable */ - return ov5640_write_reg(sensor, - OV5640_REG_PAD_OUTPUT_ENABLE02, - on ? 0xfc : 0); + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, + on ? 0xfc : 0); + if (ret) + return ret; + + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? + OV5640_REG_SYS_CTRL0_SW_PWUP : + OV5640_REG_SYS_CTRL0_SW_PWDN); } static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)