Skip to content

IMX708: Add support for 4-lane operation #6775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: rpi-6.12.y
Choose a base branch
from
+376 −125
Open

IMX708: Add support for 4-lane operation #6775

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2d38887
imx708: intial support for 4-lane operation
peyton-howe Apr 2, 2025
0a886f7
Update pixel rate for 4lane full resolution and make HBLANK controllable
peyton-howe Apr 4, 2025
2e4be19
Rework imx708 driver to modify pixel rate and line length based on th…
peyton-howe Apr 5, 2025
9e77b99
Some minor cleanup to better match IMX415 structure for dynamic setti…
peyton-howe Apr 9, 2025
1fd4408
Add missing vblank back to set_framing_limits and minor cleanup of na…
peyton-howe Apr 9, 2025
f31864a
Back to unsigned int for line_length_pix
peyton-howe Apr 9, 2025
55c7b20
Restructure WIP
peyton-howe Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
230 changes: 161 additions & 69 deletions drivers/media/i2c/imx708.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ MODULE_PARM_DESC(qbc_adjust, "Quad Bayer broken line correction strength [0,2-5]
#define IMX708_CSI_2_LANE_MODE 0x01
#define IMX708_CSI_4_LANE_MODE 0x03

#define IMX708_EXCLK_FREQ 0x18
#define IMX708_IOP_PREPLLCK_DIV 0x04

#define IMX708_REG_ORIENTATION 0x101

#define IMX708_INCLK_FREQ 24000000
Expand Down Expand Up @@ -208,75 +211,32 @@ enum {
IMX708_LINK_FREQ_450MHZ,
IMX708_LINK_FREQ_447MHZ,
IMX708_LINK_FREQ_453MHZ,
IMX708_LINK_FREQ_524MHZ,
IMX708_LINK_FREQ_547MHZ,
IMX708_LINK_FREQ_640MHZ,
IMX708_LINK_FREQ_750MHZ,
IMX708_LINK_FREQ_900MHZ,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tangential suggestion: Do we even need discrete values for link frequency: Couldn't we just support all frequencies from 447MHz to 750MHz, in 3MHz steps, if that is what the PLL is capable of?

Of course the default values in DeviceTree will have to be carefully chosen for compatibility and EMC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PLL should be capable of even more than that as both IVT_PREPLLCK_DIV and IVT_PLL_MPY could be modified. Potentially the ccs-pll helper functions could compute PLL settings, but it adds quite a lot of complexity.

You actually get more fun in having to create the array of s64's for the V4L2_CID_LINK_FREQUENCY control - probably stick it in struct imx708 so that every instance of imx708 could choose a preferred link frequency.

Default values would remain as is for 2 lane.
4 lane we probably declare as experimental and not EMC tested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need that flexibility -- multiples of 3MHz seem fine -- I wondered if we could get rid of this apparently random-looking list of values, and replace it by a stepped range?

Actually some of these magic-looking numbers are not multiples of 3MHz (547MHz and 640MHz). Are those implemented accurately right now?

I am experimenting at the moment with other clock frequencies like 480MHz and it feels silly to have to add them to the enum just to try them out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic should be:
(24MHz / (reg 0x030d)) * ((reg 0x030e<<8) + (reg 0x030f)) / 2
0x030d is 4 in all modes, giving us 3MHz * (030e/f)

I'd agree that imx708_configure_link_frequency is computing these values, but there is no point in having the enum. Read the raw link frequency from DT, validate it against selected clock frequency and reg 0x030d, and store that value in struct imx708.
Actually the LINK_FREQUENCY control can point to that as the single array value, and imx708_configure_link_frequency can use it to compute the register settings.

};

/* 450MHz is the nominal "default" link frequency */
static const s64 link_freqs[] = {
[IMX708_LINK_FREQ_450MHZ] = 450000000,
[IMX708_LINK_FREQ_447MHZ] = 447000000,
[IMX708_LINK_FREQ_453MHZ] = 453000000,
[IMX708_LINK_FREQ_524MHZ] = 524000000,
[IMX708_LINK_FREQ_547MHZ] = 547000000,
};

/* 450MHz is the nominal "default" link frequency */
static const struct imx708_reg link_450Mhz_regs[] = {
{0x030E, 0x01},
{0x030F, 0x2c},
};

static const struct imx708_reg link_447Mhz_regs[] = {
{0x030E, 0x01},
{0x030F, 0x2a},
};

static const struct imx708_reg link_453Mhz_regs[] = {
{0x030E, 0x01},
{0x030F, 0x2e},
};

static const struct imx708_reg link_524Mhz_regs[] = {
{0x030E, 0x01},
{0x030F, 0x5D},
};

static const struct imx708_reg link_547Mhz_regs[] = {
{0x030E, 0x01},
{0x030F, 0x6D},
};

static const struct imx708_reg_list link_freq_regs[] = {
[IMX708_LINK_FREQ_450MHZ] = {
.regs = link_450Mhz_regs,
.num_of_regs = ARRAY_SIZE(link_450Mhz_regs)
},
[IMX708_LINK_FREQ_447MHZ] = {
.regs = link_447Mhz_regs,
.num_of_regs = ARRAY_SIZE(link_447Mhz_regs)
},
[IMX708_LINK_FREQ_453MHZ] = {
.regs = link_453Mhz_regs,
.num_of_regs = ARRAY_SIZE(link_453Mhz_regs)
},
[IMX708_LINK_FREQ_524MHZ] = {
.regs = link_524Mhz_regs,
.num_of_regs = ARRAY_SIZE(link_524Mhz_regs)
},
[IMX708_LINK_FREQ_547MHZ] = {
.regs = link_547Mhz_regs,
.num_of_regs = ARRAY_SIZE(link_547Mhz_regs)
},
[IMX708_LINK_FREQ_640MHZ] = 640000000,
[IMX708_LINK_FREQ_750MHZ] = 750000000,
[IMX708_LINK_FREQ_900MHZ] = 900000000,
};

static const struct imx708_reg mode_common_regs[] = {
{0x0100, 0x00},
{0x0136, 0x18}, //REG_EXCK_FREQ_MSB
{0x0137, 0x00}, //REG_EXCK_FREQ_LSB
{0x33F0, 0x02}, //REG_IOPSYCK_DIV 0x01, 0x02
{0x33F1, 0x05}, //REG_IOPPXCK_DIV 0x01, 0x05
{0x0136, IMX708_EXCLK_FREQ}, //REG_EXCK_FREQ_MSB
{0x0137, 0x00}, //REG_EXCK_FREQ_LSB
{0x030D, IMX708_IOP_PREPLLCK_DIV},
{0x33F0, 0x02}, //REG_IOPSYCK_DIV
{0x33F1, 0x05}, //REG_IOPPXCK_DIV
{0x3062, 0x00},
{0x3063, 0x12}, //0x30, 0x12
{0x3063, 0x12},
{0x3068, 0x00},
{0x3069, 0x12},
{0x306A, 0x00},
Expand Down Expand Up @@ -321,25 +281,66 @@ static const struct imx708_reg mode_common_regs[] = {

/* Pixel rate setup */
enum {
IMX708_PIX_RATE_198Mhz,
IMX708_PIX_RATE_304Mhz,
IMX708_PIX_RATE_416Mhz,
IMX708_PIX_RATE_499Mhz,
IMX708_PIX_RATE_566Mhz,
IMX708_PIX_RATE_585Mhz,
IMX708_PIX_RATE_595Mhz,
IMX708_PIX_RATE_608Mhz,
IMX708_PIX_RATE_620Mhz,
IMX708_PIX_RATE_777Mhz,
IMX708_PIX_RATE_801Mhz,
IMX708_PIX_RATE_854Mhz,
IMX708_PIX_RATE_950Mhz,
};

static const s64 pixel_rates[] = {
[IMX708_PIX_RATE_198Mhz] = 198400000,
[IMX708_PIX_RATE_304Mhz] = 304000000,
[IMX708_PIX_RATE_416Mhz] = 416000000,
[IMX708_PIX_RATE_499Mhz] = 499200000,
[IMX708_PIX_RATE_566Mhz] = 566400000,
[IMX708_PIX_RATE_585Mhz] = 585600000,
[IMX708_PIX_RATE_595Mhz] = 595200000,
[IMX708_PIX_RATE_608Mhz] = 608000000,
[IMX708_PIX_RATE_620Mhz] = 620800000,
[IMX708_PIX_RATE_777Mhz] = 777600000,
[IMX708_PIX_RATE_801Mhz] = 801600000,
[IMX708_PIX_RATE_854Mhz] = 854400000,
[IMX708_PIX_RATE_950Mhz] = 950400000,
};


static const struct imx708_reg pixel_rate_198Mhz_regs[] = {
{0x0305, 0x03},
{0x0306, 0x00},
{0x0307, 0x3E},
};

static const struct imx708_reg pixel_rate_304Mhz_regs[] = {
{0x0305, 0x03},
{0x0306, 0x00},
{0x0307, 0x5F},
};

static const struct imx708_reg pixel_rate_416Mhz_regs[] = {
{0x0305, 0x03},
{0x0306, 0x00},
{0x0307, 0x82},
};

static const struct imx708_reg pixel_rate_499Mhz_regs[] = {
{0x0305, 0x02},
{0x0306, 0x00},
{0x0307, 0x68},
};

static const struct imx708_reg pixel_rate_566Mhz_regs[] = {
{0x0305, 0x02},
{0x0306, 0x00},
{0x0307, 0x76},
{0x0307, 0x50},
};

static const struct imx708_reg pixel_rate_585Mhz_regs[] = {
Expand All @@ -354,19 +355,59 @@ static const struct imx708_reg pixel_rate_595Mhz_regs[] = {
{0x0307, 0x7C},
};

static const struct imx708_reg pixel_rate_608Mhz_regs[] = {
{0x0305, 0x03},
{0x0306, 0x00},
{0x0307, 0xBE},
};

static const struct imx708_reg pixel_rate_620Mhz_regs[] = {
{0x0305, 0x03},
{0x0306, 0x00},
{0x0307, 0xC2},
};

static const struct imx708_reg pixel_rate_777Mhz_regs[] = {
{0x0305, 0x02},
{0x0306, 0x00},
{0x0307, 0xA2},
};

static const struct imx708_reg pixel_rate_801Mhz_regs[] = {
{0x0305, 0x02},
{0x0306, 0x00},
{0x0307, 0xA7},
};

static const struct imx708_reg pixel_rate_854Mhz_regs[] = {
{0x0305, 0x03},
{0x0306, 0x01},
{0x0307, 0x0B},
};

static const struct imx708_reg pixel_rate_950Mhz_regs[] = {
{0x0305, 0x03},
{0x0306, 0x01},
{0x0307, 0x29},
};

static const struct imx708_reg_list pixel_rate_regs[] = {
[IMX708_PIX_RATE_198Mhz] = {
.regs = pixel_rate_198Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_198Mhz_regs)
},
[IMX708_PIX_RATE_304Mhz] = {
.regs = pixel_rate_304Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_304Mhz_regs)
},
[IMX708_PIX_RATE_416Mhz] = {
.regs = pixel_rate_416Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_416Mhz_regs)
},
[IMX708_PIX_RATE_499Mhz] = {
.regs = pixel_rate_499Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_499Mhz_regs)
},
[IMX708_PIX_RATE_566Mhz] = {
.regs = pixel_rate_566Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_566Mhz_regs)
Expand All @@ -379,14 +420,30 @@ static const struct imx708_reg_list pixel_rate_regs[] = {
.regs = pixel_rate_595Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_595Mhz_regs)
},
[IMX708_PIX_RATE_608Mhz] = {
.regs = pixel_rate_608Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_608Mhz_regs)
},
[IMX708_PIX_RATE_620Mhz] = {
.regs = pixel_rate_620Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_620Mhz_regs)
},
[IMX708_PIX_RATE_777Mhz] = {
.regs = pixel_rate_777Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_777Mhz_regs)
},
[IMX708_PIX_RATE_801Mhz] = {
.regs = pixel_rate_801Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_801Mhz_regs)
},
[IMX708_PIX_RATE_854Mhz] = {
.regs = pixel_rate_854Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_854Mhz_regs)
},
[IMX708_PIX_RATE_950Mhz] = {
.regs = pixel_rate_950Mhz_regs,
.num_of_regs = ARRAY_SIZE(pixel_rate_950Mhz_regs)
},
};

/* Line Length setup */
Expand Down Expand Up @@ -448,7 +505,6 @@ static const struct imx708_reg mode_4608x2592_regs[] = {
{0x0301, 0x05}, //REG_IVTPXCK_DIV
{0x0303, 0x02}, //REG_IVTSYCK_DIV
{0x030B, 0x02},
{0x030D, 0x04}, //REG_IOP_PREPLLCK_DIV
{0x0310, 0x01}, //REG_PLL_MULTI_DRV
{0x3CA0, 0x00},
{0x3CA1, 0x64}, //0x32, 0x64
Expand Down Expand Up @@ -539,7 +595,6 @@ static const struct imx708_reg mode_2x2binned_regs[] = {
{0x0301, 0x05},
{0x0303, 0x02},
{0x030B, 0x02},
{0x030D, 0x04},
{0x0310, 0x01},
{0x3CA0, 0x00},
{0x3CA1, 0x3C},
Expand Down Expand Up @@ -630,7 +685,6 @@ static const struct imx708_reg mode_2x2binned_720p_regs[] = {
{0x0301, 0x05},
{0x0303, 0x02},
{0x030B, 0x02},
{0x030D, 0x04},
{0x0310, 0x01},
{0x3CA0, 0x00},
{0x3CA1, 0x3C},
Expand Down Expand Up @@ -721,7 +775,6 @@ static const struct imx708_reg mode_hdr_regs[] = {
{0x0301, 0x05},
{0x0303, 0x02},
{0x030B, 0x02},
{0x030D, 0x04},
{0x0310, 0x01},
{0x3CA0, 0x00},
{0x3CA1, 0x00},
Expand Down Expand Up @@ -774,12 +827,13 @@ static const struct imx708_reg mode_hdr_regs[] = {
};

/* Mode configs. Keep separate lists for when HDR is enabled or not. */
// 304000000 for 22fps, 416000000 for 30fps, 499200000 for 36fps, 620800000 for 45 fps
static const struct imx708_mode supported_modes_10bit_no_hdr[] = {
{
/* Full resolution. */
.width = 4608,
.height = 2592,
.line_length_pix = {15648, 10432},
.line_length_pix = {5216, 5216},
.crop = {
.left = IMX708_PIXEL_ARRAY_LEFT,
.top = IMX708_PIXEL_ARRAY_TOP,
Expand All @@ -792,7 +846,7 @@ static const struct imx708_mode supported_modes_10bit_no_hdr[] = {
.num_of_regs = ARRAY_SIZE(mode_4608x2592_regs),
.regs = mode_4608x2592_regs,
},
.pixel_rate = {595200000, 854400000},
.pixel_rate = {198400000, 304000000},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about this factor-of-3 change of pixel rate and line length (though I never understood the rationale of our original settings either). Is it a real, functional change or just a change in the way things are represented? Does it improve image quality?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't change image quality at all, only readout rate.

I suspect that we just copied the values provided by Sony in their big spreadsheet as supposedly the optimised values, but they often wrote the register set based on a requested resolution/frame rate combination whereas we actually want the max that can be achieved by the hardware.

Max pixel rate is going to depend on the link frequency to ensure we can output all the pixel data in the appropriate period of time. I can see a case where the driver ought to verify that the selected link frequency can do that, because otherwise DT can give us unworkable configurations. This is going to need a fair amount more analysis.

Copy link
Contributor

@njhollinghurst njhollinghurst Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Specifically for full-res mode and with 2 lanes, the old rate (595200000) was the Sony recommended value. It has suddenly been divided by 3 (and the line width changed to match). I just wondered why this was changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped the line length down to their minimum possible values for the given resolution, as outlined here. I plan to clean this up so there's only a single entry for line_length_pix, like before. This will ensure the user is always able to set the maximum framerate based on the number of MIPI lanes and the data rate of those lanes.

I then adjusted the pixel rate accordingly for the 2-lane full-res mode to match the original settings with the lower line length.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I don't know why Sony recommended the faster rate (and they might not be willing to explain). I thought it might be extra cycles needed for re-Mosaic but if the image is unaffected then perhaps it's not needed.

Copy link
Contributor

@njhollinghurst njhollinghurst Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested with 4 lanes your suggested pixel rate of 416000000 for 30fps, and it seems to work fine -- not only at a MIPI link frequency of "547MHz" (which I suspect is actually 546MHz) with no need to overclock RP1; but also at the perhaps more Pi-friendly 492MHz and as low as 480MHz (though not, alas, at 450MHz).

So I would be happy to see 30fps supported here. Of course it depends on the more general question of default link frequency.

.exposure_lines_min = 8,
.exposure_lines_step = 1,
.hdr = false,
Expand All @@ -802,7 +856,7 @@ static const struct imx708_mode supported_modes_10bit_no_hdr[] = {
/* regular 2x2 binned. */
.width = 2304,
.height = 1296,
.line_length_pix = {7824, 5216},
.line_length_pix = {7824, 5216}, //2608?
.crop = {
.left = IMX708_PIXEL_ARRAY_LEFT,
.top = IMX708_PIXEL_ARRAY_TOP,
Expand All @@ -825,7 +879,7 @@ static const struct imx708_mode supported_modes_10bit_no_hdr[] = {
/* 2x2 binned and cropped for 720p. */
.width = 1536,
.height = 864,
.line_length_pix = {5216, 5216},
.line_length_pix = {5216, 5216}, //2608?
.crop = {
.left = IMX708_PIXEL_ARRAY_LEFT + 768,
.top = IMX708_PIXEL_ARRAY_TOP + 432,
Expand Down Expand Up @@ -1586,11 +1640,51 @@ static int imx708_configure_lanes(struct imx708 *imx708)
IMX708_CSI_4_LANE_MODE);
};

static int imx708_configure_link_frequency(struct imx708 *imx708)
{
u16 link_freq_reg_value;
link_freq_reg_value = 4 * link_freqs[imx708->link_freq_idx] * IMX708_IOP_PREPLLCK_DIV / (1000000 * IMX708_EXCLK_FREQ);

struct i2c_client *client = v4l2_get_subdevdata(&imx708->sd);

const struct imx708_reg freq_regs[] = {
{0x030E, (link_freq_reg_value>>8)},
{0x030F, link_freq_reg_value&0xFF},
};

dev_info(&client->dev, "Set link freq MSB %hhu\n", freq_regs[0].val);
dev_info(&client->dev, "Set link freq LSB %hhu\n", freq_regs[1].val);

return imx708_write_regs(imx708, freq_regs, ARRAY_SIZE(freq_regs));
};

// static int imx708_configure_pixel_rate(struct imx708 *imx708)
// {
// // 304000000 for 22fps, 416000000 for 30fps, 499200000 for 36fps, 620800000 for 45 fps
// struct i2c_client *client = v4l2_get_subdevdata(&imx708->sd);

// u64 pix_rate;
// u8 div;
// // pix_rate = link_freqs[imx708->link_freq_idx] * 2 * imx708->lanes / 8;
// pix_rate = imx708->mode->line_length_pix * (imx708->mode->crop.height + imx708->mode->vlbank_min) * 10;

// const struct imx708_reg freq_regs[] = {
// {0x0305, 0x02},
// {0x0306, (pix_rate>>8)},
// {0x0307, (pix_rate&0xFF)},
// };

// dev_info(&client->dev, "Set link freq MSB %hhu\n", freq_regs[0].val);
// dev_info(&client->dev, "Set link freq LSB %hhu\n", freq_regs[1].val);

// return imx708_write_regs(imx708, freq_regs, ARRAY_SIZE(freq_regs));
// };

/* Start streaming */
static int imx708_start_streaming(struct imx708 *imx708)
{
struct i2c_client *client = v4l2_get_subdevdata(&imx708->sd);
const struct imx708_reg_list *reg_list, *freq_regs, *pix_rate_regs;
const struct imx708_reg_list *reg_list, *pix_rate_regs;
int i, ret;
u32 val;

Expand Down Expand Up @@ -1663,9 +1757,7 @@ static int imx708_start_streaming(struct imx708 *imx708)
}

/* Update the link frequency registers */
freq_regs = &link_freq_regs[imx708->link_freq_idx];
ret = imx708_write_regs(imx708, freq_regs->regs,
freq_regs->num_of_regs);
ret = imx708_configure_link_frequency(imx708);
if (ret) {
dev_err(&client->dev, "%s failed to set link frequency registers\n",
__func__);
Expand Down