From: David Heidelberg <david@ixit.cz>
To: Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: Tianshu Qiu <tian.shu.qiu@intel.com>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Richard Acayan <mailingradian@gmail.com>,
linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
phone-devel@vger.kernel.org
Subject: Re: [PATCH] media: i2c: imx355: Add support for 24 MHz external clock
Date: Thu, 18 Jun 2026 14:10:59 +0200 [thread overview]
Message-ID: <effcf731-4325-4e27-9330-afa1ea709c9f@ixit.cz> (raw)
In-Reply-To: <CAPY8ntA=1EK20GmqWJzzW7Tp8Mh8aXNad8+ZBG2xVcxrAkwVSA@mail.gmail.com>
On 17/06/2026 19:40, Dave Stevenson wrote:
> Hi David
>
> On Thu, 16 Apr 2026 at 12:26, Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
>>
>> Hi David
>>
>> On Tue, 14 Apr 2026 at 11:17, David Heidelberg via B4 Relay
>> <devnull+david.ixit.cz@kernel.org> wrote:
>>>
>>> From: David Heidelberg <david@ixit.cz>
>>>
>>> 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?
> 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).
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@ixit.cz>
>>
>> Whilst useful comments could be added, it does what it says and works:
>>
>> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>> Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>>> ---
>>> 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@ixit.cz>
>>>
>>>
>>>
--
David Heidelberg
next prev parent reply other threads:[~2026-06-18 12:11 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-14 10:12 [PATCH] media: i2c: imx355: Add support for 24 MHz external clock David Heidelberg via B4 Relay
2026-04-16 2:08 ` Richard Acayan
2026-04-16 11:26 ` Dave Stevenson
2026-06-17 17:40 ` Dave Stevenson
2026-06-18 12:10 ` David Heidelberg [this message]
2026-06-18 14:33 ` Dave Stevenson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=effcf731-4325-4e27-9330-afa1ea709c9f@ixit.cz \
--to=david@ixit.cz \
--cc=dave.stevenson@raspberrypi.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mailingradian@gmail.com \
--cc=mchehab@kernel.org \
--cc=phone-devel@vger.kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=tian.shu.qiu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox