| Message ID | 20250504101336.18748-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | media: mt9m114: Changes to make it work with atomisp devices | expand |
Hi Hans, Thank you for the patch. On Sun, May 04, 2025 at 12:13:22PM +0200, Hans de Goede wrote: > Make aptina_pll_calculate() debug log the calculated p1 min and max values, > this makes it easier to see how the m, n and p1 values were chosen. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/aptina-pll.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/i2c/aptina-pll.c b/drivers/media/i2c/aptina-pll.c > index b1f89bbf9d47..cd2ed4583c97 100644 > --- a/drivers/media/i2c/aptina-pll.c > +++ b/drivers/media/i2c/aptina-pll.c > @@ -129,6 +129,8 @@ int aptina_pll_calculate(struct device *dev, > p1_max = min(limits->p1_max, limits->out_clock_max * div / > (pll->ext_clock * pll->m)); > > + dev_dbg(dev, "pll: p1 min %u max %u\n", p1_min, p1_max); > + > for (p1 = p1_max & ~1; p1 >= p1_min; p1 -= 2) { > unsigned int mf_inc = p1 / gcd(div, p1); > unsigned int mf_high;
Hi Hans, Thank you for the patch. On Sun, May 04, 2025 at 12:13:24PM +0200, Hans de Goede wrote: > Before this change the driver used hardcoded PLL m, n and p values to > achieve a 48MHz pixclock when used with an external clock with a frequency > of 24 MHz. > > Use aptina_pll_calculate() to allow the driver to work with different > external clock frequencies. The m, n, and p values will be unchanged > with a 24 MHz extclk and this has also been tested with a 19.2 MHz > clock where m gets increased from 32 to 40. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/mt9m114.c | 53 +++++++++++++++++++++++++++---------- > 1 file changed, 39 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index be1d2ec64b89..9fdfd90afc22 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -31,6 +31,8 @@ > #include <media/v4l2-mediabus.h> > #include <media/v4l2-subdev.h> > > +#include "aptina-pll.h" You need to select CONFIG_VIDEO_APTINA_PLL. > + > /* Sysctl registers */ > #define MT9M114_CHIP_ID CCI_REG16(0x0000) > #define MT9M114_COMMAND_REGISTER CCI_REG16(0x0080) > @@ -262,9 +264,9 @@ > #define MT9M114_CAM_SYSCTL_PLL_ENABLE CCI_REG8(0xc97e) > #define MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE BIT(0) > #define MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N CCI_REG16(0xc980) > -#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(m, n) (((n) << 8) | (m)) > +#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(m, n) ((((n) - 1) << 8) | (m)) > #define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P CCI_REG16(0xc982) > -#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(p) ((p) << 8) > +#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(p) (((p) - 1) << 8) > #define MT9M114_CAM_PORT_OUTPUT_CONTROL CCI_REG16(0xc984) > #define MT9M114_CAM_PORT_PORT_SELECT_PARALLEL (0 << 0) > #define MT9M114_CAM_PORT_PORT_SELECT_MIPI (1 << 0) > @@ -325,7 +327,7 @@ > * minimum values that have been seen in register lists are 303 and 38, use > * them. > * > - * Set the default to achieve 1280x960 at 30fps. > + * Set the default to achieve 1280x960 at 30fps with a 48 MHz pixclock. > */ > #define MT9M114_MIN_HBLANK 303 > #define MT9M114_MIN_VBLANK 38 > @@ -335,6 +337,8 @@ > #define MT9M114_DEF_FRAME_RATE 30 > #define MT9M114_MAX_FRAME_RATE 120 > > +#define MT9M114_DEF_PIXCLOCK 48000000 > + > #define MT9M114_PIXEL_ARRAY_WIDTH 1296U > #define MT9M114_PIXEL_ARRAY_HEIGHT 976U > > @@ -379,11 +383,7 @@ struct mt9m114 { > struct v4l2_fwnode_endpoint bus_cfg; > u32 clk_freq; > > - struct { > - unsigned int m; > - unsigned int n; > - unsigned int p; > - } pll; > + struct aptina_pll pll; > > unsigned int pixrate; > bool streaming; > @@ -751,7 +751,7 @@ static int mt9m114_initialize(struct mt9m114 *sensor) > sensor->pll.n), > &ret); > cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_P, > - MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p), &ret); > + MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p1), &ret); > cci_write(sensor->regmap, MT9M114_CAM_SENSOR_CFG_PIXCLK, > sensor->pixrate, &ret); > > @@ -2238,11 +2238,36 @@ static const struct dev_pm_ops mt9m114_pm_ops = { > static int mt9m114_clk_init(struct mt9m114 *sensor) > { > unsigned int link_freq; > + /* > + * TODO these limits have been copied from mt9p031.c, with out_clock_max > + * increased from 360000000 to 400000000 to get the same PLL settings as > + * in the static register lists for 24 MHz and 19.2 MHz ext clock freq. > + * These should be verified / adjusted by someone with access to > + * the full MT9M114 datasheet. Not all constraints are documented, unfortunately :-( > + */ > + static const struct aptina_pll_limits limits = { > + .ext_clock_min = 6000000, > + .ext_clock_max = 27000000, The external clock range is 6 MHz to 54 MHz. > + .int_clock_min = 2000000, > + .int_clock_max = 13500000, There's no documented values for the internal frequency. > + .out_clock_min = 180000000, > + .out_clock_max = 400000000, The maximum is 768 MHz. There's no documented minimum value. > + .pix_clock_max = 96000000, I think the maximum pixel clock frequency is 48 MPix/s. > + .n_min = 1, > + .n_max = 64, > + .m_min = 16, > + .m_max = 255, I found a screenshot that specifies the N range as 0-63 (so 1-64 after adding 1) and the M range as 16-192. > + .p1_min = 1, > + .p1_max = 128, I don't know what the maximum value is, but the field is documented as 6 bits, so it can't be above 64. > + }; > + int ret; > > - /* Hardcode the PLL multiplier and dividers to default settings. */ > - sensor->pll.m = 32; > - sensor->pll.n = 1; > - sensor->pll.p = 7; > + sensor->pll.ext_clock = sensor->clk_freq; > + sensor->pll.pix_clock = MT9M114_DEF_PIXCLOCK; > + > + ret = aptina_pll_calculate(&sensor->client->dev, &limits, &sensor->pll); > + if (ret) > + return ret; > > /* > * Calculate the pixel rate and link frequency. The CSI-2 bus is clocked > @@ -2250,7 +2275,7 @@ static int mt9m114_clk_init(struct mt9m114 *sensor) > * parallel mode, the sensor ouputs one pixel in two PIXCLK cycles. > */ > sensor->pixrate = sensor->clk_freq * sensor->pll.m > - / ((sensor->pll.n + 1) * (sensor->pll.p + 1)); > + / (sensor->pll.n * sensor->pll.p1); > > link_freq = sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY > ? sensor->pixrate * 8 : sensor->pixrate * 2;
Hi Hans, On Sun, May 04, 2025 at 12:13:26PM +0200, Hans de Goede wrote: > The old default hblank and vblank values were based on reaching 30 fps s/old/current/ and s/were/are/ > with the pixel-array outputting 1280x960, but the default format for > the pixel-array source pad and the isp sink pad is 1296x976, correct > the default hblank and vblank values to take this into account. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/mt9m114.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index b40142aed3e8..52337c719e22 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -327,12 +327,12 @@ > * minimum values that have been seen in register lists are 303 and 21, use > * them. > * > - * Set the default to achieve 1280x960 at 30fps with a 48 MHz pixclock. > + * Set the default to achieve 1296x976 at 30fps with a 48 MHz pixclock. 1280x960 is referring to the output resolution. I agree about the new blanking values, but maybe we could be more precise in the comment: * Set the default to achieve full resolution (1296x976 analog crop * rectangle, 1280x960 output size) at 30fps with a 48 MHz pixclock. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > */ > #define MT9M114_MIN_HBLANK 303 > #define MT9M114_MIN_VBLANK 21 > -#define MT9M114_DEF_HBLANK 323 > -#define MT9M114_DEF_VBLANK 39 > +#define MT9M114_DEF_HBLANK 307 > +#define MT9M114_DEF_VBLANK 23 > > #define MT9M114_DEF_FRAME_RATE 30 > #define MT9M114_MAX_FRAME_RATE 120
Hi Hans, Thank you for the patch. On Sun, May 04, 2025 at 12:13:34PM +0200, Hans de Goede wrote: > Add support for the mt9m114 sensor being enumerated through ACPI > using the INT33F0 HID as found on the Asus T100TA. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/mt9m114.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index eba8232444c9..08e4d7ebcbd1 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -2556,11 +2556,18 @@ static const struct of_device_id mt9m114_of_ids[] = { > }; > MODULE_DEVICE_TABLE(of, mt9m114_of_ids); > > +static const struct acpi_device_id mt9m114_acpi_ids[] = { > + { "INT33F0" }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(acpi, mt9m114_acpi_ids); > + > static struct i2c_driver mt9m114_driver = { > .driver = { > .name = "mt9m114", > .pm = &mt9m114_pm_ops, > .of_match_table = mt9m114_of_ids, > + .acpi_match_table = mt9m114_acpi_ids, > }, > .probe = mt9m114_probe, > .remove = mt9m114_remove,
Hi Hans, Thank you for the patch. On Sun, May 04, 2025 at 12:13:28PM +0200, Hans de Goede wrote: > Update hblank and vblank defaults when the pixel-array format changes, > to maintain 30 fps on format changes. I don't think this should be the kernel's responsibility to do so. Sakari, any opinion ? > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/mt9m114.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index ba8b7660f88a..be3e7bb44ad8 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -333,6 +333,10 @@ > #define MT9M114_MIN_VBLANK 21 > #define MT9M114_DEF_HBLANK 308 > #define MT9M114_DEF_VBLANK 21 > +#define MT9M114_DEF_HTS \ > + (MT9M114_PIXEL_ARRAY_WIDTH + MT9M114_DEF_HBLANK) > +#define MT9M114_DEF_VTS \ > + (MT9M114_PIXEL_ARRAY_HEIGHT + MT9M114_DEF_VBLANK) > > #define MT9M114_DEF_FRAME_RATE 30 > #define MT9M114_MAX_FRAME_RATE 120 > @@ -1130,18 +1134,22 @@ static void mt9m114_pa_ctrl_update_exposure(struct mt9m114 *sensor, bool manual) > static void mt9m114_pa_ctrl_update_blanking(struct mt9m114 *sensor, > const struct v4l2_mbus_framefmt *format) > { > - unsigned int max_blank; > + unsigned int def_blank, max_blank; > > /* Update the blanking controls ranges based on the output size. */ > max_blank = MT9M114_CAM_SENSOR_CFG_LINE_LENGTH_PCK_MAX > - format->width; > + def_blank = MT9M114_DEF_HTS - format->width; > __v4l2_ctrl_modify_range(sensor->pa.hblank, MT9M114_MIN_HBLANK, > - max_blank, 1, MT9M114_DEF_HBLANK); > + max_blank, 1, def_blank); > + __v4l2_ctrl_s_ctrl(sensor->pa.hblank, def_blank); > > max_blank = MT9M114_CAM_SENSOR_CFG_FRAME_LENGTH_LINES_MAX > - format->height; > + def_blank = MT9M114_DEF_VTS - format->height; > __v4l2_ctrl_modify_range(sensor->pa.vblank, MT9M114_MIN_VBLANK, > - max_blank, 1, MT9M114_DEF_VBLANK); > + max_blank, 1, def_blank); > + __v4l2_ctrl_s_ctrl(sensor->pa.vblank, def_blank); > } > > /* -----------------------------------------------------------------------------
Hi Hans, On Sun, May 04, 2025 at 12:13:30PM +0200, Hans de Goede wrote: > Put sensor back in reset on power-down. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/mt9m114.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index 43efcbdf614e..fa7c2137c6ba 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -2202,6 +2202,7 @@ static int mt9m114_power_on(struct mt9m114 *sensor) > > static void mt9m114_power_off(struct mt9m114 *sensor) > { > + gpiod_set_value(sensor->reset, 1); The sensor requires a delay of at least 10 EXTCLK cycles here. > clk_disable_unprepare(sensor->clk); > regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies); > }
Hi Hans, On Sun, May 04, 2025 at 12:13:28PM +0200, Hans de Goede wrote: > Update hblank and vblank defaults when the pixel-array format changes, > to maintain 30 fps on format changes. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/mt9m114.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c > index ba8b7660f88a..be3e7bb44ad8 100644 > --- a/drivers/media/i2c/mt9m114.c > +++ b/drivers/media/i2c/mt9m114.c > @@ -333,6 +333,10 @@ > #define MT9M114_MIN_VBLANK 21 > #define MT9M114_DEF_HBLANK 308 > #define MT9M114_DEF_VBLANK 21 > +#define MT9M114_DEF_HTS \ > + (MT9M114_PIXEL_ARRAY_WIDTH + MT9M114_DEF_HBLANK) > +#define MT9M114_DEF_VTS \ > + (MT9M114_PIXEL_ARRAY_HEIGHT + MT9M114_DEF_VBLANK) > > #define MT9M114_DEF_FRAME_RATE 30 > #define MT9M114_MAX_FRAME_RATE 120 > @@ -1130,18 +1134,22 @@ static void mt9m114_pa_ctrl_update_exposure(struct mt9m114 *sensor, bool manual) > static void mt9m114_pa_ctrl_update_blanking(struct mt9m114 *sensor, > const struct v4l2_mbus_framefmt *format) > { > - unsigned int max_blank; > + unsigned int def_blank, max_blank; > > /* Update the blanking controls ranges based on the output size. */ > max_blank = MT9M114_CAM_SENSOR_CFG_LINE_LENGTH_PCK_MAX > - format->width; > + def_blank = MT9M114_DEF_HTS - format->width; > __v4l2_ctrl_modify_range(sensor->pa.hblank, MT9M114_MIN_HBLANK, > - max_blank, 1, MT9M114_DEF_HBLANK); > + max_blank, 1, def_blank); __v4l2_ctrl_modify_range() may fail. The problem isn't introduced by this patch but it'd be nice to fix it (separately). > + __v4l2_ctrl_s_ctrl(sensor->pa.hblank, def_blank); > > max_blank = MT9M114_CAM_SENSOR_CFG_FRAME_LENGTH_LINES_MAX > - format->height; > + def_blank = MT9M114_DEF_VTS - format->height; > __v4l2_ctrl_modify_range(sensor->pa.vblank, MT9M114_MIN_VBLANK, > - max_blank, 1, MT9M114_DEF_VBLANK); > + max_blank, 1, def_blank); > + __v4l2_ctrl_s_ctrl(sensor->pa.vblank, def_blank); > } > > /* -----------------------------------------------------------------------------
Hi Laurent, On Sat, May 10, 2025 at 12:38:08AM +0200, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Sun, May 04, 2025 at 12:13:28PM +0200, Hans de Goede wrote: > > Update hblank and vblank defaults when the pixel-array format changes, > > to maintain 30 fps on format changes. > > I don't think this should be the kernel's responsibility to do so. > Sakari, any opinion ? Generally drivers don't do this indeed. It's the user space's resposibility to configure the frame rate. Drivers only guarantee hardware limits are respected.
Hi, On 10-May-25 2:48 PM, Sakari Ailus wrote: > Hi Laurent, > > On Sat, May 10, 2025 at 12:38:08AM +0200, Laurent Pinchart wrote: >> Hi Hans, >> >> Thank you for the patch. >> >> On Sun, May 04, 2025 at 12:13:28PM +0200, Hans de Goede wrote: >>> Update hblank and vblank defaults when the pixel-array format changes, >>> to maintain 30 fps on format changes. >> >> I don't think this should be the kernel's responsibility to do so. >> Sakari, any opinion ? > > Generally drivers don't do this indeed. It's the user space's resposibility > to configure the frame rate. Drivers only guarantee hardware limits are > respected. Ok, I'll drop this patch for v2. Laurent, thank you for all the reviews so far. I agree with all your remarks and I'll address them for v2. I notice that you did not review 10/13 - 12/13 yet, I guess you ran out of time ? Also I guess 10/13 is possibly a bit controversial patch, all I can say about that is that this one is necessary to make things work in raw-bayer mode with the atomisp. Regards, Hans
On Sat, May 10, 2025 at 04:04:04PM +0200, Hans de Goede wrote: > Hi, > > On 10-May-25 2:48 PM, Sakari Ailus wrote: > > Hi Laurent, > > > > On Sat, May 10, 2025 at 12:38:08AM +0200, Laurent Pinchart wrote: > >> Hi Hans, > >> > >> Thank you for the patch. > >> > >> On Sun, May 04, 2025 at 12:13:28PM +0200, Hans de Goede wrote: > >>> Update hblank and vblank defaults when the pixel-array format changes, > >>> to maintain 30 fps on format changes. > >> > >> I don't think this should be the kernel's responsibility to do so. > >> Sakari, any opinion ? > > > > Generally drivers don't do this indeed. It's the user space's resposibility > > to configure the frame rate. Drivers only guarantee hardware limits are > > respected. > > Ok, I'll drop this patch for v2. > > Laurent, thank you for all the reviews so far. I agree with > all your remarks and I'll address them for v2. > > I notice that you did not review 10/13 - 12/13 yet, I guess > you ran out of time ? Yes I ran out of time, and next week will likely be busy. Feel free to send a v2 addressing the review comments so far, and I can review patchs 10/13to 12/13 from there. > Also I guess 10/13 is possibly a bit > controversial patch, all I can say about that is that this one > is necessary to make things work in raw-bayer mode with the atomisp. > > Regards, > > Hans
Hi All, Here is a series of patches to make the "mainline" mt9m114 driver work on devices with an atomisp CSI2 receiver / ISP. This has been tested on an Asus T100TA. This allows dropping the atomisp specific atomisp-mt9m114 driver from drivers/staging. Chances are there are conflicts here with Mathis': "[PATCH v4 0/6] MT9M114 driver bugfix and improvements" series. I'll expect there be some review comments on this series anyways, so merging Mathis' series first is fine, then I'll rebase this series on top for v2. Regards, Hans Hans de Goede (13): media: aptina-pll: Debug log p1 min and max values media: mt9m114: Add support for clock-frequency property media: mt9m114: Use aptina-PLL helper to get PLL values media: mt9m114: Lower minimum vblank value media: mt9m114: Fix default hblank and vblank values media: mt9m114: Tweak default hblank and vblank for more accurate fps media: mt9m114: Update hblank and vblank defaults on pixel-array format changes media: mt9m114: Avoid a reset low spike during probe() media: mt9m114: Put sensor in reset on power-down media: mt9m114: Fix scaler bypass mode media: mt9m114: Drop start-, stop-streaming sequence from initialize media: mt9m114: Return -EPROBE_DEFER if no endpoint is found media: mt9m114: Add ACPI enumeration support drivers/media/i2c/aptina-pll.c | 2 + drivers/media/i2c/mt9m114.c | 167 ++++++++++++++++++++++++--------- 2 files changed, 125 insertions(+), 44 deletions(-)