Hi Dave,
On Mon, Jan 20, 2025 at 11:35:40AM +0000, Dave Stevenson wrote:
> Commit ceddfd4493b3 ("media: i2c: imx219: Support four-lane operation")
> added support for device tree to allow configuration of the sensor to
> use 4 lanes with a link frequency of 363MHz, and amended the advertised
> pixel rate to 280.8MPix/s.
>
> However it didn't change any of the PLL settings, so actually it would
> have been running overclocked in the MIPI block, and with the frame
> rate and exposure calculations being wrong as the pixel rate was
> unchanged.
>
> The pixel rate and link frequency advertised were taken from the "Clock
> Setting Example" section of the datasheet. However those are based on an
> external clock of 12MHz, and are unachievable with a clock of 24MHz - it
> seems PREPLLCLK_VT_DIV and PREPLLCK_OP_DIV can ONLY be set via the
> automatic configuration documented in "9-1-2 EXCK_FREQ setting depend on
> INCK frequency", not by writing the registers.
> The closest we can get with a 24MHz clock is 281.6MPix/s and 364MHz.
>
> Dropping all support for the 363MHz link frequency would cause problems
> for existing users, so allow it, but log a warning that the requested
> value is being changed to the supported one.
>
> Fixes: ceddfd4493b3 ("media: i2c: imx219: Support four-lane operation")
> Co-developed-by: Peyton Howe <peyton.howe@xxxxxxxxxxxxx>
> Signed-off-by: Peyton Howe <peyton.howe@xxxxxxxxxxxxx>
> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> ---
> This was fed back to us by Peyton Howe as giving image corruption
> on a Raspberry Pi with DF Robot imx219 module, and confirmed with
> a Soho Enterprises module.
> Effectively the module was being overclocked and misbehaving.
>
> With this patch and the Soho Enterprises module no image corruption
> is observed, and the frame rates are spot on. I haven't checked
> exposure times, but those should follow frame rate as they are
> based on the same clock.
> ---
> drivers/media/i2c/imx219.c | 78 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 61 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 2d54cea113e1..562b3eb0cb1e 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -133,10 +133,11 @@
>
> /* Pixel rate is fixed for all the modes */
> #define IMX219_PIXEL_RATE 182400000
> -#define IMX219_PIXEL_RATE_4LANE 280800000
> +#define IMX219_PIXEL_RATE_4LANE 281600000
>
> #define IMX219_DEFAULT_LINK_FREQ 456000000
> -#define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000
> +#define IMX219_DEFAULT_LINK_FREQ_4LANE_UNSUPPORTED 363000000
> +#define IMX219_DEFAULT_LINK_FREQ_4LANE 364000000
This shows again the ill effects of register list based drivers. :-(
>
> /* IMX219 native and active pixel array size. */
> #define IMX219_NATIVE_WIDTH 3296U
> @@ -168,15 +169,6 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
> { CCI_REG8(0x30eb), 0x05 },
> { CCI_REG8(0x30eb), 0x09 },
>
> - /* PLL Clock Table */
> - { IMX219_REG_VTPXCK_DIV, 5 },
> - { IMX219_REG_VTSYCK_DIV, 1 },
> - { IMX219_REG_PREPLLCK_VT_DIV, 3 }, /* 0x03 = AUTO set */
> - { IMX219_REG_PREPLLCK_OP_DIV, 3 }, /* 0x03 = AUTO set */
> - { IMX219_REG_PLL_VT_MPY, 57 },
> - { IMX219_REG_OPSYCK_DIV, 1 },
> - { IMX219_REG_PLL_OP_MPY, 114 },
> -
> /* Undocumented registers */
> { CCI_REG8(0x455e), 0x00 },
> { CCI_REG8(0x471e), 0x4b },
> @@ -201,6 +193,34 @@ static const struct cci_reg_sequence imx219_common_regs[] = {
> { IMX219_REG_EXCK_FREQ, IMX219_EXCK_FREQ(IMX219_XCLK_FREQ / 1000000) },
> };
>
> +static const struct cci_reg_sequence imx219_2lane_regs[] = {
> + /* PLL Clock Table */
> + { IMX219_REG_VTPXCK_DIV, 5 },
> + { IMX219_REG_VTSYCK_DIV, 1 },
> + { IMX219_REG_PREPLLCK_VT_DIV, 3 }, /* 0x03 = AUTO set */
> + { IMX219_REG_PREPLLCK_OP_DIV, 3 }, /* 0x03 = AUTO set */
> + { IMX219_REG_PLL_VT_MPY, 57 },
> + { IMX219_REG_OPSYCK_DIV, 1 },
> + { IMX219_REG_PLL_OP_MPY, 114 },
> +
> + /* 2-Lane CSI Mode */
> + { IMX219_REG_CSI_LANE_MODE, IMX219_CSI_2_LANE_MODE },
> +};
> +
> +static const struct cci_reg_sequence imx219_4lane_regs[] = {
> + /* PLL Clock Table */
> + { IMX219_REG_VTPXCK_DIV, 5 },
> + { IMX219_REG_VTSYCK_DIV, 1 },
> + { IMX219_REG_PREPLLCK_VT_DIV, 3 }, /* 0x03 = AUTO set */
> + { IMX219_REG_PREPLLCK_OP_DIV, 3 }, /* 0x03 = AUTO set */
> + { IMX219_REG_PLL_VT_MPY, 88 },
> + { IMX219_REG_OPSYCK_DIV, 1 },
> + { IMX219_REG_PLL_OP_MPY, 91 },
> +
> + /* 4-Lane CSI Mode */
> + { IMX219_REG_CSI_LANE_MODE, IMX219_CSI_4_LANE_MODE },
> +};
> +
> static const s64 imx219_link_freq_menu[] = {
> IMX219_DEFAULT_LINK_FREQ,
> };
> @@ -662,9 +682,11 @@ static int imx219_set_framefmt(struct imx219 *imx219,
>
> static int imx219_configure_lanes(struct imx219 *imx219)
> {
> - return cci_write(imx219->regmap, IMX219_REG_CSI_LANE_MODE,
> - imx219->lanes == 2 ? IMX219_CSI_2_LANE_MODE :
> - IMX219_CSI_4_LANE_MODE, NULL);
> + /* Write the appropriate PLL settings for the number of MIPI lanes */
> + return cci_multi_reg_write(imx219->regmap,
> + imx219->lanes == 2 ? imx219_2lane_regs : imx219_4lane_regs,
> + imx219->lanes == 2 ? ARRAY_SIZE(imx219_2lane_regs) :
> + ARRAY_SIZE(imx219_4lane_regs), NULL);
> };
>
> static int imx219_start_streaming(struct imx219 *imx219,
> @@ -1036,6 +1058,7 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> .bus_type = V4L2_MBUS_CSI2_DPHY
> };
> int ret = -EINVAL;
> + bool link_frequency_valid = false;
>
> endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> if (!endpoint)
> @@ -1062,9 +1085,30 @@ static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> goto error_out;
> }
>
> - if (ep_cfg.nr_of_link_frequencies != 1 ||
> - (ep_cfg.link_frequencies[0] != ((imx219->lanes == 2) ?
> - IMX219_DEFAULT_LINK_FREQ : IMX219_DEFAULT_LINK_FREQ_4LANE))) {
> + if (ep_cfg.nr_of_link_frequencies == 1) {
> + switch (imx219->lanes) {
> + case 2:
> + if (ep_cfg.link_frequencies[0] ==
> + IMX219_DEFAULT_LINK_FREQ)
> + link_frequency_valid = true;
> + break;
> + case 4:
> + if (ep_cfg.link_frequencies[0] ==
> + IMX219_DEFAULT_LINK_FREQ_4LANE)
> + link_frequency_valid = true;
> + else if (ep_cfg.link_frequencies[0] ==
> + IMX219_DEFAULT_LINK_FREQ_4LANE_UNSUPPORTED) {
> + dev_warn(dev, "Link frequency of %d not supported, but has been incorrectly advertised previously\n",
> + IMX219_DEFAULT_LINK_FREQ_4LANE_UNSUPPORTED);
> + dev_warn(dev, "Using link frequency of %d\n",
> + IMX219_DEFAULT_LINK_FREQ_4LANE);
Would it be helpful to use v4l2_link_freq_to_bitmap() here? The old
frequency requires separate handling but I guess you'll still want to
expose the correct frequency to the user space so it should be just one
condition.
> + link_frequency_valid = true;
> + }
> + break;
> + }
> + }
> +
> + if (!link_frequency_valid) {
> dev_err_probe(dev, -EINVAL,
> "Link frequency not supported: %lld\n",
> ep_cfg.link_frequencies[0]);
>
--
Lomd regards,
Sakari Ailus
