Re: [PATCH 4/6] media: i2c: imx334: Support 4 or 8 lane operation modes

[Dave Stevenson]

Hi Tarang & Sakari

On Thu, 27 Mar 2025 at 10:55, Tarang Raval
<tarang.raval@xxxxxxxxxxxxxxxxx> wrote:
>
> Hi Sakari,
>
> Thanks for the review.
>
> > On Mon, Mar 10, 2025 at 12:47:46PM +0530, Tarang Raval wrote:
> > > imx334 can support both 4 and 8 lane configurations.
> > > Extend the driver to configure the lane mode accordingly.
> > >
> > > Signed-off-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/media/i2c/imx334.c | 22 +++++++++++++++++++---
> > >  1 file changed, 19 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index 24ccfd1d0986..23bfc64969cc 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -47,6 +47,8 @@
> > >  #define IMX334_EXPOSURE_DEFAULT      0x0648
> > >
> > >  #define IMX334_REG_LANEMODE            CCI_REG8(0x3a01)
> > > +#define IMX334_CSI_4_LANE_MODE         3
> > > +#define IMX334_CSI_8_LANE_MODE         7
> > >
> > >  /* Window cropping Settings */
> > >  #define IMX334_REG_AREA3_ST_ADR_1      CCI_REG16_LE(0x3074)
> > > @@ -107,7 +109,6 @@
> > >  /* CSI2 HW configuration */
> > >  #define IMX334_LINK_FREQ_891M        891000000
> > >  #define IMX334_LINK_FREQ_445M        445500000
> > > -#define IMX334_NUM_DATA_LANES        4
> > >
> > >  #define IMX334_REG_MIN               0x00
> > >  #define IMX334_REG_MAX               0xfffff
> > > @@ -181,6 +182,7 @@ struct imx334_mode {
> > >   * @exp_ctrl: Pointer to exposure control
> > >   * @again_ctrl: Pointer to analog gain control
> > >   * @vblank: Vertical blanking in lines
> > > + * @lane_mode: Mode for number of connected data lanes
> > >   * @cur_mode: Pointer to current selected sensor mode
> > >   * @mutex: Mutex for serializing sensor controls
> > >   * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
> > > @@ -204,6 +206,7 @@ struct imx334 {
> > >               struct v4l2_ctrl *again_ctrl;
> > >       };
> > >       u32 vblank;
> > > +     u32 lane_mode;
> > >       const struct imx334_mode *cur_mode;
> > >       struct mutex mutex;
> > >       unsigned long link_freq_bitmap;
> > > @@ -240,7 +243,6 @@ static const struct cci_reg_sequence common_mode_regs[] = {
> > >       { IMX334_REG_HADD_VADD, 0x00},
> > >       { IMX334_REG_VALID_EXPAND, 0x03},
> > >       { IMX334_REG_TCYCLE, 0x00},
> > > -     { IMX334_REG_LANEMODE, 0x03},
> >
> > Not a fault of this patch but also the closing brace should have a space
> > before it. Could you address it in the earlier patches?
>
> Okay, I will correct it.
>
> > >       { IMX334_REG_TCLKPOST, 0x007f},
> > >       { IMX334_REG_TCLKPREPARE, 0x0037},
> > >       { IMX334_REG_TCLKTRAIL, 0x0037},
> > > @@ -876,6 +878,13 @@ static int imx334_start_streaming(struct imx334 *imx334)
> > >               return ret;
> > >       }
> > >
> > > +     ret = cci_write(imx334->cci, IMX334_REG_LANEMODE,
> > > +                     imx334->lane_mode, NULL);
> > > +     if (ret) {
> > > +             dev_err(imx334->dev, "failed to configure lanes\n");
> > > +             return ret;
> > > +     }
> > > +
> > >       ret = imx334_set_framefmt(imx334);
> > >       if (ret) {
> > >               dev_err(imx334->dev, "%s failed to set frame format: %d\n",
> > > @@ -1022,7 +1031,14 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
> > >       if (ret)
> > >               return ret;
> > >
> > > -     if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX334_NUM_DATA_LANES) {
> > > +     switch (bus_cfg.bus.mipi_csi2.num_data_lanes) {
> > > +     case 4:
> > > +             imx334->lane_mode = IMX334_CSI_4_LANE_MODE;
> > > +             break;
> > > +     case 8:
> > > +             imx334->lane_mode = IMX334_CSI_8_LANE_MODE;
> >
> > Doesn't this affect the PLL configuration? Presumably higher frame rates
> > could be achieved at least.
>
> Sorry, my commit message is misleading. The intention of this patch is to
> configure the lane mode dynamically from the streaming function instead
> of using a hardcoded value.
>
> You are correct that supporting an 8-lane mode requires changes to the PLL
> configuration. This patch does not address that aspect yet.

Is it actually required, or just a nicety?
The datasheet [1] says:
"Maximum frame rate in All-pixel scan mode 3840(H)×2160(V) AD12bit: 60
frame / s"
The current driver configuration for the 3840x2160 mode is a pixel
clock 594MHz with total timings of (3840+560) x (2160+90), which gives
a framerate of 60fps. So you already have the maximum capabilities of
the sensor exposed.

Adding the 8 lane mode gives you the option to run at half the link
frequency of the 4 lane, but Sony Starvis sensors have a FIFO between
pixel array and MIPI block. All the other Starvis sensors I've
encountered are quite happy at any of the link frequencies as long as
the horizontal blanking makes the line period sufficient to send each
line.

The datasheet does say "The bit rate maximum value are 1782 Mbps /
Lane in 4 Lane mode and 1188 Mbps / Lane in 8 Lane mode. " (page 78
"CSI-2 output"), but then also "The maximum bit rate of each Lane are
1782 Mbps / Lane." (page 81 "MIPI transmitter"). Surely all lanes can
either do 1782Mbps, or they can't. They won't have downrated just
lanes 5-8.
Presumably it works at 1782Mbps/lane in 8 lane mode or you wouldn't
have submitted the patch,

We've been here before with the imx290 and imx415 drivers and what can
be supported with each combination of lanes and link frequency.

Cheers
  Dave

[1] https://en.sunnywale.com/uploadfile/2022/1205/IMX334LQR-C%20full%20datasheet_Awin.pdf

> Best Regards,
> Tarang
> > > +             break;
> > > +     default:
> > >               dev_err(imx334->dev,
> > >                       "number of CSI2 data lanes %d is not supported\n",
> > >                       bus_cfg.bus.mipi_csi2.num_data_lanes);
> >
> > --
> > Regards,
> >
> > Sakari Ailus