Re: [PATCH] media: i2c: imx355: Add support for 24 MHz external clock

[Dave Stevenson]

On Thu, 18 Jun 2026 at 13:11, David Heidelberg <david@xxxxxxx> wrote:
>
> On 17/06/2026 19:40, Dave Stevenson wrote:
> > Hi David
> >
> > On Thu, 16 Apr 2026 at 12:26, Dave Stevenson
> > <dave.stevenson@xxxxxxxxxxxxxxx> wrote:
> >>
> >> Hi David
> >>
> >> On Tue, 14 Apr 2026 at 11:17, David Heidelberg via B4 Relay
> >> <devnull+david.ixit.cz@xxxxxxxxxx> wrote:
> >>>
> >>> From: David Heidelberg <david@xxxxxxx>
> >>>
> >>> The IMX355 sensor supports multiple external clock frequencies,
> >>> including 19.2 MHz and 24 MHz. The driver currently supports only
> >>> fixed 19.2 MHz input clock.
> >>>
> >>> Refactor the clock handling to make the PLL configuration dependent
> >>> on the external clock frequency and add support for 24 MHz. Introduce
> >>> a table of clock parameter sets and program the corresponding EXTCLK
> >>> frequency and PLL multipliers to maintain consistent internal VCO
> >>> frequencies across supported inputs.
> >>>
> >>> The PLL settings are adjusted so that:
> >>>    - VT VCO remains at 1152 MHz
> >>>    - OP VCO remains at 720 MHz
> >>>
> >>> This preserves existing timing characteristics while allowing systems
> >>> using a 24 MHz clock to operate correctly.
> >>
> >> I happened to have someone asking for this same requirement, so tried
> >> out your patch.
> >>
> >> I don't have a datasheet for IMX355, but the patterns very closely
> >> follow IMX477 and IMX708 for which I do.
> >> Those both have a single PLL and a dual PLL mode selected via register
> >> PLL_MULT_DRIV (0x0310). The imx355 driver is setting that to 0 for
> >> single PLL mode, which means that the PREDIV_IVT (0x0305) and MPY_IVT
> >> PLL (0x0306/7) settings do nothing as the IVT block is driven from
> >> IOPCK.
> >
> > I now have the datasheet and software reference manual for IMX355.
> >
> > It says that dual PLL mode is not available.
> > "In PLL single mode, IOP_PREPLLCK_DIV and IOP_PLL_MPY are applied to
> > IOPCK PLL, and IVT_PREPLLCK_DIV and IVT_PLL_MPY are ignored".
> > So there is no need to have alternate pll_vt_mpy values for 24MHz.
> >
> > I have no idea why Sony say that dual PLL mode isn't available. When I
> > was adding the 2 lane support I used it without any issues. I'm now
> > reworking those patches to avoid dual PLL mode.
>
> Maybe there is some ERRATA which makes it unstable or problematic?

Not mentioned in the information I've got, so it must have been
deleted as a feature very early in development.
C'est la vie.

> > Then again the datasheet also says that LINE_LENGTH_PCK is constrained
> > to 3672 in full res and 4x4 binning mode, and 1836 in 2x2 binned modes
> > "to avoid sensor internal interference (FPN)", so they obviously hit
> > some odd behaviours and just added constraints.
> >
> > Do you wish to send a V2 dropping pll_vt_mpy, or shall I pull it into my series?
>
> What works for you better (since u sending bigger changes than I do).

I'm happy to merge it into my series as that avoids having to declare
dependencies.
I had hoped to get my v2 out this week, but it just hasn't happened.
It won't be next week, but hopefully within the fortnight.

  Dave

> David
>
> >
> > Thanks
> >    Dave
> >
> >> Your patch therefore works, but does more than is necessary - you
> >> really can set pll_vt_mpy to any value and it works exactly the same.
> >> I guess changing the unconnected IVTCK clock within the sensor could
> >> feasibly change EMC emissions, but that feels pretty unlikely.
> >> Possibly add a comment to your existing comment of "VT VCO = 1152 MHz"
> >> to say that it's unused to avoid others going down the rabbit hole I
> >> encountered.
> >>
> >> (For those referencing the other datasheets, the description for
> >> single PLL mode lists configuring IVT_PREPLLCK_DIV and IVT_PLL_MPY,
> >> but the diagram shows that IOP is always driven from IOPCK, and IVT is
> >> muxed between IOPCK and IVTCK)
> >>
> >>> No functional change for existing 19.2 MHz users.
> >>>
> >>> Assisted-by: Claude:claude-opus-4-6
> >>> Signed-off-by: David Heidelberg <david@xxxxxxx>
> >>
> >> Whilst useful comments could be added, it does what it says and works:
> >>
> >> Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> >> Tested-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> >>
> >>> ---
> >>> Known users: Pixel 3 and 3a.
> >>> ---
> >>>   drivers/media/i2c/imx355.c | 114 +++++++++++++++++++++------------------------
> >>>   1 file changed, 54 insertions(+), 60 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> >>> index 27a5c212a527f..f9ec13bb27d10 100644
> >>> --- a/drivers/media/i2c/imx355.c
> >>> +++ b/drivers/media/i2c/imx355.c
> >>> @@ -25,6 +25,11 @@
> >>>   #define IMX355_REG_CHIP_ID             0x0016
> >>>   #define IMX355_CHIP_ID                 0x0355
> >>>
> >>> +/* PLL registers that depend on the external clock frequency */
> >>> +#define IMX355_REG_EXTCLK_FREQ         0x0136
> >>> +#define IMX355_REG_PLL_VT_MUL          0x0306
> >>> +#define IMX355_REG_PLL_OP_MUL          0x030e
> >>> +
> >>>   /* V_TIMING internal */
> >>>   #define IMX355_REG_FLL                 0x0340
> >>>   #define IMX355_FLL_MAX                 0xffff
> >>> @@ -63,7 +68,6 @@
> >>>
> >>>   /* default link frequency and external clock */
> >>>   #define IMX355_LINK_FREQ_DEFAULT       360000000LL
> >>> -#define IMX355_EXT_CLK                 19200000
> >>>   #define IMX355_LINK_FREQ_INDEX         0
> >>>
> >>>   /* number of data lanes */
> >>> @@ -100,6 +104,33 @@ struct imx355_mode {
> >>>          struct imx355_reg_list reg_list;
> >>>   };
> >>>
> >>> +struct imx355_clk_params {
> >>> +       u32 ext_clk;
> >>> +       u16 extclk_freq; /* External clock (MHz) in 8.8 fixed point) */
> >>> +       u16 pll_vt_mpy; /* VT system PLL multiplier */
> >>> +       u16 pll_op_mpy; /* OP system PLL multiplier */
> >>> +};
> >>> +
> >>> +/*
> >>> + * All modes use the same PLL dividers (PREPLLCK_VT_DIV=2, PREPLLCK_OP_DIV=2),
> >>> + * so the multipliers are adjusted to produce the same VCO frequencies:
> >>> + *   VT VCO = 1152 MHz, OP VCO = 720 MHz
> >>> + */
> >>> +static const struct imx355_clk_params imx355_clk_params[] = {
> >>> +       {
> >>> +               .ext_clk = 19200000,
> >>> +               .extclk_freq = 0x1333,  /* 19.2 MHz */
> >>> +               .pll_vt_mpy = 120,      /* 19.2 / 2 * 120 = 1152 MHz */
> >>> +               .pll_op_mpy = 75,       /* 19.2 / 2 * 75  = 720 MHz */
> >>> +       },
> >>> +       {
> >>> +               .ext_clk = 24000000,
> >>> +               .extclk_freq = 0x1800,  /* 24.0 MHz */
> >>> +               .pll_vt_mpy = 96,       /* 24.0 / 2 * 96  = 1152 MHz */
> >>> +               .pll_op_mpy = 60,       /* 24.0 / 2 * 60  = 720 MHz */
> >>> +       },
> >>> +};
> >>> +
> >>>   struct imx355_hwcfg {
> >>>          unsigned long link_freq_bitmap;
> >>>   };
> >>> @@ -125,6 +156,7 @@ struct imx355 {
> >>>          const struct imx355_mode *cur_mode;
> >>>
> >>>          struct imx355_hwcfg *hwcfg;
> >>> +       const struct imx355_clk_params *clk_params;
> >>>
> >>>          /*
> >>>           * Mutex for serialized access:
> >>> @@ -144,8 +176,6 @@ static const struct regulator_bulk_data imx355_supplies[] = {
> >>>   };
> >>>
> >>>   static const struct imx355_reg imx355_global_regs[] = {
> >>> -       { 0x0136, 0x13 },
> >>> -       { 0x0137, 0x33 },
> >>>          { 0x304e, 0x03 },
> >>>          { 0x4348, 0x16 },
> >>>          { 0x4350, 0x19 },
> >>> @@ -231,12 +261,8 @@ static const struct imx355_reg mode_3268x2448_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -280,12 +306,8 @@ static const struct imx355_reg mode_3264x2448_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -329,12 +351,8 @@ static const struct imx355_reg mode_3280x2464_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -378,12 +396,8 @@ static const struct imx355_reg mode_1940x1096_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -427,12 +441,8 @@ static const struct imx355_reg mode_1936x1096_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -476,12 +486,8 @@ static const struct imx355_reg mode_1924x1080_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -525,12 +531,8 @@ static const struct imx355_reg mode_1920x1080_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -574,12 +576,8 @@ static const struct imx355_reg mode_1640x1232_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -623,12 +621,8 @@ static const struct imx355_reg mode_1640x922_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -672,12 +666,8 @@ static const struct imx355_reg mode_1300x736_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -721,12 +711,8 @@ static const struct imx355_reg mode_1296x736_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -770,12 +756,8 @@ static const struct imx355_reg mode_1284x720_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -819,12 +801,8 @@ static const struct imx355_reg mode_1280x720_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x00 },
> >>>          { 0x0701, 0x10 },
> >>> @@ -868,12 +846,8 @@ static const struct imx355_reg mode_820x616_regs[] = {
> >>>          { 0x0301, 0x05 },
> >>>          { 0x0303, 0x01 },
> >>>          { 0x0305, 0x02 },
> >>> -       { 0x0306, 0x00 },
> >>> -       { 0x0307, 0x78 },
> >>>          { 0x030b, 0x01 },
> >>>          { 0x030d, 0x02 },
> >>> -       { 0x030e, 0x00 },
> >>> -       { 0x030f, 0x4b },
> >>>          { 0x0310, 0x00 },
> >>>          { 0x0700, 0x02 },
> >>>          { 0x0701, 0x78 },
> >>> @@ -1422,6 +1396,20 @@ static int imx355_start_streaming(struct imx355 *imx355)
> >>>                  return ret;
> >>>          }
> >>>
> >>> +       /* Set PLL registers for the external clock frequency */
> >>> +       ret = imx355_write_reg(imx355, IMX355_REG_EXTCLK_FREQ, 2,
> >>> +                              imx355->clk_params->extclk_freq);
> >>> +       if (ret)
> >>> +               return ret;
> >>> +       ret = imx355_write_reg(imx355, IMX355_REG_PLL_VT_MUL, 2,
> >>> +                              imx355->clk_params->pll_vt_mpy);
> >>> +       if (ret)
> >>> +               return ret;
> >>> +       ret = imx355_write_reg(imx355, IMX355_REG_PLL_OP_MUL, 2,
> >>> +                              imx355->clk_params->pll_op_mpy);
> >>> +       if (ret)
> >>> +               return ret;
> >>> +
> >>>          /* set digital gain control to all color mode */
> >>>          ret = imx355_write_reg(imx355, IMX355_REG_DPGA_USE_GLOBAL_GAIN, 1, 1);
> >>>          if (ret)
> >>> @@ -1749,7 +1737,13 @@ static int imx355_probe(struct i2c_client *client)
> >>>                                       "failed to get clock\n");
> >>>
> >>>          freq = clk_get_rate(imx355->clk);
> >>> -       if (freq != IMX355_EXT_CLK)
> >>> +       for (unsigned int i = 0; i < ARRAY_SIZE(imx355_clk_params); i++) {
> >>> +               if (freq == imx355_clk_params[i].ext_clk) {
> >>> +                       imx355->clk_params = &imx355_clk_params[i];
> >>> +                       break;
> >>> +               }
> >>> +       }
> >>> +       if (!imx355->clk_params)
> >>>                  return dev_err_probe(imx355->dev, -EINVAL,
> >>>                                       "external clock %lu is not supported\n",
> >>>                                       freq);
> >>>
> >>> ---
> >>> base-commit: 66672af7a095d89f082c5327f3b15bc2f93d558e
> >>> change-id: 20260414-imx355-24mhz-b8ccfab3adfb
> >>>
> >>> Best regards,
> >>> --
> >>> David Heidelberg <david@xxxxxxx>
> >>>
> >>>
> >>>
>
> --
> David Heidelberg
>