| Message ID | 20210216174146.106639-17-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: i2c: GMSL reliability improvements | expand |
On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > With the camera modules initialization routines now running with > the noise immunity threshold enabled, it is possible to restore > the bit rate of the I2C transactions transported on the GMSL control > channel to 339 Kbps. > > The 339 Kbps bit rate represents the default setting for the serializer > and the deserializer chips, and the setup/hold time and slave timeout > time in use are calibrate to support that rate. calibrated Gr{oetje,eeting}s, Geert
On 16/02/2021 17:41, Jacopo Mondi wrote: > With the camera modules initialization routines now running with > the noise immunity threshold enabled, it is possible to restore > the bit rate of the I2C transactions transported on the GMSL control > channel to 339 Kbps. > > The 339 Kbps bit rate represents the default setting for the serializer > and the deserializer chips, and the setup/hold time and slave timeout > time in use are calibrate to support that rate. s/calibrate/calibrated/ Does that mean the setup/hold time and timeouts should be adjusted based on the i2c speed? (which we have not been doing?) With all of your other reliability improvements, does *this* change alone have any difference or impact on reliability? I.e. if we go slower (stay at current speed) - would we be more reliable? Is there a reliability improvement by making this speed faster? I presume we don't have a way to convey the i2c bus speed between the max9286 and the max9271 currently? Seems a bit like a bus parameter.... > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/max9286.c | 2 +- > drivers/media/i2c/rdacm20.c | 2 +- > drivers/media/i2c/rdacm21.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index aa01d5bb79ef..0b620f2f8c41 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -330,7 +330,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv) > static void max9286_configure_i2c(struct max9286_priv *priv, bool localack) > { > u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US | > - MAX9286_I2CMSTBT_105KBPS; > + MAX9286_I2CMSTBT_339KBPS; > > if (localack) > config |= MAX9286_I2CLOCACK; > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index 0632ef98eea7..d45e8b0e52a0 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -450,7 +450,7 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val) > ret = max9271_configure_i2c(&dev->serializer, > MAX9271_I2CSLVSH_469NS_234NS | > MAX9271_I2CSLVTO_1024US | > - MAX9271_I2CMSTBT_105KBPS); > + MAX9271_I2CMSTBT_339KBPS); > if (ret) > return ret; > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 80b6f16f87a8..552985026458 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -442,7 +442,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val) > ret = max9271_configure_i2c(&dev->serializer, > MAX9271_I2CSLVSH_469NS_234NS | > MAX9271_I2CSLVTO_1024US | > - MAX9271_I2CMSTBT_105KBPS); > + MAX9271_I2CMSTBT_339KBPS); > if (ret) > return ret; > >
Hi Jacopo, Thank you for the patch. On Thu, Feb 18, 2021 at 04:07:30PM +0000, Kieran Bingham wrote: > On 16/02/2021 17:41, Jacopo Mondi wrote: > > With the camera modules initialization routines now running with > > the noise immunity threshold enabled, it is possible to restore > > the bit rate of the I2C transactions transported on the GMSL control > > channel to 339 Kbps. > > > > The 339 Kbps bit rate represents the default setting for the serializer > > and the deserializer chips, and the setup/hold time and slave timeout > > time in use are calibrate to support that rate. > > s/calibrate/calibrated/ > > Does that mean the setup/hold time and timeouts should be adjusted based > on the i2c speed? (which we have not been doing?) > > With all of your other reliability improvements, does *this* change > alone have any difference or impact on reliability? > > I.e. if we go slower (stay at current speed) - would we be more reliable? > > Is there a reliability improvement by making this speed faster? > > I presume we don't have a way to convey the i2c bus speed between the > max9286 and the max9271 currently? Seems a bit like a bus parameter.... There's a max bus speed DT parameter that we can use. And I think we should, as, in theory at least, there's no guarantee that all the devices behind the serializer can operate at 400kbps. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/max9286.c | 2 +- > > drivers/media/i2c/rdacm20.c | 2 +- > > drivers/media/i2c/rdacm21.c | 2 +- > > 3 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index aa01d5bb79ef..0b620f2f8c41 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -330,7 +330,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv) > > static void max9286_configure_i2c(struct max9286_priv *priv, bool localack) > > { > > u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US | > > - MAX9286_I2CMSTBT_105KBPS; > > + MAX9286_I2CMSTBT_339KBPS; > > > > if (localack) > > config |= MAX9286_I2CLOCACK; > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > index 0632ef98eea7..d45e8b0e52a0 100644 > > --- a/drivers/media/i2c/rdacm20.c > > +++ b/drivers/media/i2c/rdacm20.c > > @@ -450,7 +450,7 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val) > > ret = max9271_configure_i2c(&dev->serializer, > > MAX9271_I2CSLVSH_469NS_234NS | > > MAX9271_I2CSLVTO_1024US | > > - MAX9271_I2CMSTBT_105KBPS); > > + MAX9271_I2CMSTBT_339KBPS); > > if (ret) > > return ret; > > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > index 80b6f16f87a8..552985026458 100644 > > --- a/drivers/media/i2c/rdacm21.c > > +++ b/drivers/media/i2c/rdacm21.c > > @@ -442,7 +442,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val) > > ret = max9271_configure_i2c(&dev->serializer, > > MAX9271_I2CSLVSH_469NS_234NS | > > MAX9271_I2CSLVTO_1024US | > > - MAX9271_I2CMSTBT_105KBPS); > > + MAX9271_I2CMSTBT_339KBPS); > > if (ret) > > return ret; > >
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index aa01d5bb79ef..0b620f2f8c41 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -330,7 +330,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv) static void max9286_configure_i2c(struct max9286_priv *priv, bool localack) { u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US | - MAX9286_I2CMSTBT_105KBPS; + MAX9286_I2CMSTBT_339KBPS; if (localack) config |= MAX9286_I2CLOCACK; diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index 0632ef98eea7..d45e8b0e52a0 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -450,7 +450,7 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val) ret = max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS | MAX9271_I2CSLVTO_1024US | - MAX9271_I2CMSTBT_105KBPS); + MAX9271_I2CMSTBT_339KBPS); if (ret) return ret; diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 80b6f16f87a8..552985026458 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -442,7 +442,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val) ret = max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS | MAX9271_I2CSLVTO_1024US | - MAX9271_I2CMSTBT_105KBPS); + MAX9271_I2CMSTBT_339KBPS); if (ret) return ret;
With the camera modules initialization routines now running with the noise immunity threshold enabled, it is possible to restore the bit rate of the I2C transactions transported on the GMSL control channel to 339 Kbps. The 339 Kbps bit rate represents the default setting for the serializer and the deserializer chips, and the setup/hold time and slave timeout time in use are calibrate to support that rate. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/max9286.c | 2 +- drivers/media/i2c/rdacm20.c | 2 +- drivers/media/i2c/rdacm21.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)