| Message ID | 20210216174146.106639-10-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: i2c: GMSL reliability improvements | expand |
Hi Jacopo, On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > The OV10640 image sensor reset and powerdown on signals are controlled Drop the "on"? > by the embedded OV490 ISP. The current reset procedure does not respect > the 1 millisecond power-up delay and releases the reset signal before > the powerdown one. > > Fix the OV10640 power up sequence by releasing the powerdown signal, > waiting the mandatory 1 millisecond power up delay and then releasing "powerdown" vs. "power-up" vs. "power up"? > the reset signal. The reset delay is not characterized in the chip > manual if not as "255 XVCLK + initialization". Wait for at least 3 > milliseconds to guarantee the SCCB bus is available. Gr{oetje,eeting}s, Geert
Hi Jacopo, On 16/02/2021 17:41, Jacopo Mondi wrote: > The OV10640 image sensor reset and powerdown on signals are controlled > by the embedded OV490 ISP. The current reset procedure does not respect > the 1 millisecond power-up delay and releases the reset signal before > the powerdown one. > > Fix the OV10640 power up sequence by releasing the powerdown signal, > waiting the mandatory 1 millisecond power up delay and then releasing > the reset signal. The reset delay is not characterized in the chip > manual if not as "255 XVCLK + initialization". Wait for at least 3 > milliseconds to guarantee the SCCB bus is available. > > This commit fixes a sporadic start-up error triggered by a failure to > read the OV10640 chip ID: > rdacm21 8-0054: OV10640 ID mismatch: (0x01) Have you done a similar initialisation rework for the RDACM21 as you do in [03/16] of this series for the RDACM20 (or was it already better perhaps?) Only interested because of noting that I think the 'mismatch' check is now gone from the RDAMC20. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/rdacm21.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index b22a2ca5340b..c420a6b96879 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev) > { > u8 val; > > - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */ > + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */ > ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); > ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); > ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0); > ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0); > - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); > + > ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0); > + usleep_range(1500, 3000); > + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); > usleep_range(3000, 5000); > > /* Read OV10640 ID to test communications. */ >
Hi Jacopo, Thank you for the patch. On Tue, Feb 16, 2021 at 06:41:39PM +0100, Jacopo Mondi wrote: > The OV10640 image sensor reset and powerdown on signals are controlled s/ on// > by the embedded OV490 ISP. The current reset procedure does not respect > the 1 millisecond power-up delay and releases the reset signal before > the powerdown one. > > Fix the OV10640 power up sequence by releasing the powerdown signal, > waiting the mandatory 1 millisecond power up delay and then releasing > the reset signal. The reset delay is not characterized in the chip > manual if not as "255 XVCLK + initialization". Wait for at least 3 > milliseconds to guarantee the SCCB bus is available. > > This commit fixes a sporadic start-up error triggered by a failure to > read the OV10640 chip ID: > rdacm21 8-0054: OV10640 ID mismatch: (0x01) > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/rdacm21.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index b22a2ca5340b..c420a6b96879 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev) > { > u8 val; > > - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */ > + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */ > ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); > ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); > ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0); > ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0); > - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); > + > ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0); Shouldn't this be OV490_GPIO_OUTPUT_VALUE1 ? > + usleep_range(1500, 3000); > + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); I'm a bit puzzled by why this patch would improve the ID read issue, given that it sets GPIO0 to 1, then sets GPIO0 to 1, compared to previously setting GPIO0 to 1 following by setting GPIO0 to 1 :-) Maybe it's the additional delay ? In any case, it would probably be a good idea to perform additional tests after fixing this. > usleep_range(3000, 5000); > > /* Read OV10640 ID to test communications. */
Hi On Mon, Feb 22, 2021 at 03:27:25AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Feb 16, 2021 at 06:41:39PM +0100, Jacopo Mondi wrote: > > The OV10640 image sensor reset and powerdown on signals are controlled > > s/ on// > > > by the embedded OV490 ISP. The current reset procedure does not respect > > the 1 millisecond power-up delay and releases the reset signal before > > the powerdown one. > > > > Fix the OV10640 power up sequence by releasing the powerdown signal, > > waiting the mandatory 1 millisecond power up delay and then releasing > > the reset signal. The reset delay is not characterized in the chip > > manual if not as "255 XVCLK + initialization". Wait for at least 3 > > milliseconds to guarantee the SCCB bus is available. > > > > This commit fixes a sporadic start-up error triggered by a failure to > > read the OV10640 chip ID: > > rdacm21 8-0054: OV10640 ID mismatch: (0x01) > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/rdacm21.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > index b22a2ca5340b..c420a6b96879 100644 > > --- a/drivers/media/i2c/rdacm21.c > > +++ b/drivers/media/i2c/rdacm21.c > > @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev) > > { > > u8 val; > > > > - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */ > > + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */ > > ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); > > ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); > > ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0); > > ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0); > > - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); > > + > > ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0); > > Shouldn't this be OV490_GPIO_OUTPUT_VALUE1 ? > Ouch, yes it should > > + usleep_range(1500, 3000); > > + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); > > I'm a bit puzzled by why this patch would improve the ID read issue, > given that it sets GPIO0 to 1, then sets GPIO0 to 1, compared to > previously setting GPIO0 to 1 following by setting GPIO0 to 1 :-) Maybe :) it doesn't make things worse at least! > it's the additional delay ? In any case, it would probably be a good > idea to perform additional tests after fixing this. I think the additional delay plays indeed a role, as it's a requirement in the datasheet that was not respected, but now I'm dead scared to fix this and find out I've opened another can of worms.. > > > usleep_range(3000, 5000); > > > > /* Read OV10640 ID to test communications. */ > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Feb 22, 2021 at 04:19:13PM +0100, Jacopo Mondi wrote: > On Mon, Feb 22, 2021 at 03:27:25AM +0200, Laurent Pinchart wrote: > > On Tue, Feb 16, 2021 at 06:41:39PM +0100, Jacopo Mondi wrote: > > > The OV10640 image sensor reset and powerdown on signals are controlled > > > > s/ on// > > > > > by the embedded OV490 ISP. The current reset procedure does not respect > > > the 1 millisecond power-up delay and releases the reset signal before > > > the powerdown one. > > > > > > Fix the OV10640 power up sequence by releasing the powerdown signal, > > > waiting the mandatory 1 millisecond power up delay and then releasing > > > the reset signal. The reset delay is not characterized in the chip > > > manual if not as "255 XVCLK + initialization". Wait for at least 3 > > > milliseconds to guarantee the SCCB bus is available. > > > > > > This commit fixes a sporadic start-up error triggered by a failure to > > > read the OV10640 chip ID: > > > rdacm21 8-0054: OV10640 ID mismatch: (0x01) > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > --- > > > drivers/media/i2c/rdacm21.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > > index b22a2ca5340b..c420a6b96879 100644 > > > --- a/drivers/media/i2c/rdacm21.c > > > +++ b/drivers/media/i2c/rdacm21.c > > > @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev) > > > { > > > u8 val; > > > > > > - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */ > > > + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */ > > > ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); > > > ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); > > > ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0); > > > ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0); > > > - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); > > > + > > > ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0); > > > > Shouldn't this be OV490_GPIO_OUTPUT_VALUE1 ? > > Ouch, yes it should > > > > + usleep_range(1500, 3000); > > > + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); > > > > I'm a bit puzzled by why this patch would improve the ID read issue, > > given that it sets GPIO0 to 1, then sets GPIO0 to 1, compared to > > previously setting GPIO0 to 1 following by setting GPIO0 to 1 :-) Maybe > > :) it doesn't make things worse at least! > > > it's the additional delay ? In any case, it would probably be a good > > idea to perform additional tests after fixing this. > > I think the additional delay plays indeed a role, as it's a > requirement in the datasheet that was not respected, but now I'm dead > scared to fix this and find out I've opened another can of worms.. With some luck it will be the opposite, and fixing this will make the startup sequence much more reliable. Of course luck has been quite scarce so far for anything even remotely related to GMSL, so I won't hold my breath. > > > usleep_range(3000, 5000); > > > > > > /* Read OV10640 ID to test communications. */
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index b22a2ca5340b..c420a6b96879 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device *dev) { u8 val; - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */ + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */ ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0); ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0); ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0); ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0); - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0); + usleep_range(1500, 3000); + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0); usleep_range(3000, 5000); /* Read OV10640 ID to test communications. */
The OV10640 image sensor reset and powerdown on signals are controlled by the embedded OV490 ISP. The current reset procedure does not respect the 1 millisecond power-up delay and releases the reset signal before the powerdown one. Fix the OV10640 power up sequence by releasing the powerdown signal, waiting the mandatory 1 millisecond power up delay and then releasing the reset signal. The reset delay is not characterized in the chip manual if not as "255 XVCLK + initialization". Wait for at least 3 milliseconds to guarantee the SCCB bus is available. This commit fixes a sporadic start-up error triggered by a failure to read the OV10640 chip ID: rdacm21 8-0054: OV10640 ID mismatch: (0x01) Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/rdacm21.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)