| Message ID | 20230627131830.54601-29-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov2680: Bugfixes + ACPI + selection(crop-tgt) API support | expand |
Morning Hans On 27/06/2023 15:18, Hans de Goede wrote: > Add read-only link-freq and pixel-rate controls. This is necessary for > the sensor to work with the ipu3-cio2 driver and for libcamera. > > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/ov2680.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index 8bc542df1890..95d3152ddd22 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -75,6 +75,12 @@ > #define OV2680_MIN_CROP_WIDTH 2 > #define OV2680_MIN_CROP_HEIGHT 2 > > +/* Fixed pre-div of 1/2 */ > +#define OV2680_PLL_PREDIV0 2 > + > +/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */ > +#define OV2680_PLL_PREDIV 2 > + > /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */ > #define OV2680_PIXELS_PER_LINE 1704 > #define OV2680_LINES_PER_FRAME 1294 > @@ -118,6 +124,8 @@ struct ov2680_ctrls { > struct v4l2_ctrl *hflip; > struct v4l2_ctrl *vflip; > struct v4l2_ctrl *test_pattern; > + struct v4l2_ctrl *link_freq; > + struct v4l2_ctrl *pixel_rate; > }; > > struct ov2680_mode { > @@ -145,6 +153,8 @@ struct ov2680_dev { > struct clk *xvclk; > u32 xvclk_freq; > u8 pll_mult; > + s64 link_freq[1]; > + s64 pixel_rate; > struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES]; > > struct gpio_desc *pwdn_gpio; > @@ -906,6 +916,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, > 0, 1023, 1, 250); > > + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, > + 0, 0, sensor->link_freq); > + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, > + 0, sensor->pixel_rate, > + 1, sensor->pixel_rate); > + > if (hdl->error) { > ret = hdl->error; > goto cleanup_entity; > @@ -913,6 +929,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > > ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; > ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; > + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > sensor->sd.ctrl_handler = hdl; > > @@ -1030,6 +1047,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) > > sensor->pll_mult = ov2680_pll_multipliers[i]; > > + sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 / > + OV2680_PLL_PREDIV * sensor->pll_mult; > + > + /* CSI-2 is double data rate, bus-format is 10 bpp */ > + sensor->pixel_rate = sensor->link_freq[0] * 2 / 10; I'm a little unsure on this one. My understanding is that the link frequency really ought to come from the endpoint properties (which in our case would be set by the ipu-bridge; though it doesn't for this sensor at the moment because I didn't understand it properly back then) because it's a platform specific thing. What the value should be, I have been determining by reading the PLL settings for the sensor whilst the laptop's running Windows. So whilst this is probably technically fine in supporting the link frequency that the driver already expects to configure for whatever platform this was originally designed for, my guess would be that the Miix expects a different link frequency and ideally we'd support that instead. For example see these commits for the ov7251: ed9566ce1946 media: i2c: Add support for new frequencies to ov7251 df057b0dd99b media: i2c: Add ov7251_pll_configure() 1757b44eb6bb media: i2c: Remove per-mode frequencies from ov7251 cc125aaa5a78 media: i2c: Provide ov7251_check_hwcfg() Which keeps support for the link frequency that the driver already configured and adds support for the new one, for both 19.2 and 24MHz input clocks. What would be your thoughts on moving #15 and this patch to something like that? > + > return 0; > } >
Hi Dan, On 7/4/23 11:22, Dan Scally wrote: > Morning Hans > > On 27/06/2023 15:18, Hans de Goede wrote: >> Add read-only link-freq and pixel-rate controls. This is necessary for >> the sensor to work with the ipu3-cio2 driver and for libcamera. >> >> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/media/i2c/ov2680.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c >> index 8bc542df1890..95d3152ddd22 100644 >> --- a/drivers/media/i2c/ov2680.c >> +++ b/drivers/media/i2c/ov2680.c >> @@ -75,6 +75,12 @@ >> #define OV2680_MIN_CROP_WIDTH 2 >> #define OV2680_MIN_CROP_HEIGHT 2 >> +/* Fixed pre-div of 1/2 */ >> +#define OV2680_PLL_PREDIV0 2 >> + >> +/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */ >> +#define OV2680_PLL_PREDIV 2 >> + >> /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */ >> #define OV2680_PIXELS_PER_LINE 1704 >> #define OV2680_LINES_PER_FRAME 1294 >> @@ -118,6 +124,8 @@ struct ov2680_ctrls { >> struct v4l2_ctrl *hflip; >> struct v4l2_ctrl *vflip; >> struct v4l2_ctrl *test_pattern; >> + struct v4l2_ctrl *link_freq; >> + struct v4l2_ctrl *pixel_rate; >> }; >> struct ov2680_mode { >> @@ -145,6 +153,8 @@ struct ov2680_dev { >> struct clk *xvclk; >> u32 xvclk_freq; >> u8 pll_mult; >> + s64 link_freq[1]; >> + s64 pixel_rate; >> struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES]; >> struct gpio_desc *pwdn_gpio; >> @@ -906,6 +916,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >> ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, >> 0, 1023, 1, 250); >> + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, >> + 0, 0, sensor->link_freq); >> + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, >> + 0, sensor->pixel_rate, >> + 1, sensor->pixel_rate); >> + >> if (hdl->error) { >> ret = hdl->error; >> goto cleanup_entity; >> @@ -913,6 +929,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >> ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; >> ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; >> + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; >> sensor->sd.ctrl_handler = hdl; >> @@ -1030,6 +1047,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) >> sensor->pll_mult = ov2680_pll_multipliers[i]; >> + sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 / >> + OV2680_PLL_PREDIV * sensor->pll_mult; >> + >> + /* CSI-2 is double data rate, bus-format is 10 bpp */ >> + sensor->pixel_rate = sensor->link_freq[0] * 2 / 10; > > > I'm a little unsure on this one. My understanding is that the link frequency really ought to come from the endpoint properties (which in our case would be set by the ipu-bridge; though it doesn't for this sensor at the moment because I didn't understand it properly back then) because it's a platform specific thing. What the value should be, I have been determining by reading the PLL settings for the sensor whilst the laptop's running Windows. So whilst this is probably technically fine in supporting the link frequency that the driver already expects to configure for whatever platform this was originally designed for, my guess would be that the Miix expects a different link frequency and ideally we'd support that instead. For example see these commits for the ov7251: The datasheet is clear that the ov2680 is intended to be used with a fixed pixelclock of 66 MHz: "2.2 architecture The OV2680 sensor core generates streaming pixel data at a constant frame rate to a pixel clock of 66 MHz." and the ov2680 always operates in single lane mode. So there really is not much to configure here. Also the datasheet only contains a rudimentary description of the PLL, which is not really enough to write a function to configure the PLL for arbitrary link-frequencies. The adjustment to make the sensor work with a 19.2MHz xvclk instead of the default 24 MHz comes from the atomisp code. How to make other adjustments would pretty much be guess work. I guess we could add code to check the link-frequencies and check that there is only 1 and it matches the expected 330 MHz then the driver still honors the link-frequencies property while at the same time sticking with the fixed setup the sensor is intended to be used with. Would adding a link-frequency check like that work for you ? And if yes what should the link-frequency control return then, the actual achieved frequency (this would be better IMHO) or the one from the property ? Regards, Hans
Hi Hans and Dan On Tue, 4 Jul 2023 at 10:45, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Dan, > > On 7/4/23 11:22, Dan Scally wrote: > > Morning Hans > > > > On 27/06/2023 15:18, Hans de Goede wrote: > >> Add read-only link-freq and pixel-rate controls. This is necessary for > >> the sensor to work with the ipu3-cio2 driver and for libcamera. > >> > >> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/media/i2c/ov2680.c | 23 +++++++++++++++++++++++ > >> 1 file changed, 23 insertions(+) > >> > >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > >> index 8bc542df1890..95d3152ddd22 100644 > >> --- a/drivers/media/i2c/ov2680.c > >> +++ b/drivers/media/i2c/ov2680.c > >> @@ -75,6 +75,12 @@ > >> #define OV2680_MIN_CROP_WIDTH 2 > >> #define OV2680_MIN_CROP_HEIGHT 2 > >> +/* Fixed pre-div of 1/2 */ > >> +#define OV2680_PLL_PREDIV0 2 > >> + > >> +/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */ > >> +#define OV2680_PLL_PREDIV 2 > >> + > >> /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */ > >> #define OV2680_PIXELS_PER_LINE 1704 > >> #define OV2680_LINES_PER_FRAME 1294 > >> @@ -118,6 +124,8 @@ struct ov2680_ctrls { > >> struct v4l2_ctrl *hflip; > >> struct v4l2_ctrl *vflip; > >> struct v4l2_ctrl *test_pattern; > >> + struct v4l2_ctrl *link_freq; > >> + struct v4l2_ctrl *pixel_rate; > >> }; > >> struct ov2680_mode { > >> @@ -145,6 +153,8 @@ struct ov2680_dev { > >> struct clk *xvclk; > >> u32 xvclk_freq; > >> u8 pll_mult; > >> + s64 link_freq[1]; > >> + s64 pixel_rate; > >> struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES]; > >> struct gpio_desc *pwdn_gpio; > >> @@ -906,6 +916,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > >> ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, > >> 0, 1023, 1, 250); > >> + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, > >> + 0, 0, sensor->link_freq); > >> + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, > >> + 0, sensor->pixel_rate, > >> + 1, sensor->pixel_rate); > >> + > >> if (hdl->error) { > >> ret = hdl->error; > >> goto cleanup_entity; > >> @@ -913,6 +929,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > >> ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; > >> ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; > >> + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >> sensor->sd.ctrl_handler = hdl; > >> @@ -1030,6 +1047,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) > >> sensor->pll_mult = ov2680_pll_multipliers[i]; > >> + sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 / > >> + OV2680_PLL_PREDIV * sensor->pll_mult; > >> + > >> + /* CSI-2 is double data rate, bus-format is 10 bpp */ > >> + sensor->pixel_rate = sensor->link_freq[0] * 2 / 10; > > > > > > I'm a little unsure on this one. My understanding is that the link frequency really ought to come from the endpoint properties (which in our case would be set by the ipu-bridge; though it doesn't for this sensor at the moment because I didn't understand it properly back then) because it's a platform specific thing. What the value should be, I have been determining by reading the PLL settings for the sensor whilst the laptop's running Windows. So whilst this is probably technically fine in supporting the link frequency that the driver already expects to configure for whatever platform this was originally designed for, my guess would be that the Miix expects a different link frequency and ideally we'd support that instead. For example see these commits for the ov7251: > > The datasheet is clear that the ov2680 is intended to be used with > a fixed pixelclock of 66 MHz: > > "2.2 architecture > > The OV2680 sensor core generates streaming pixel data at a constant > frame rate to a pixel clock of 66 MHz." > > and the ov2680 always operates in single lane mode. So there really is > not much to configure here. > > Also the datasheet only contains a rudimentary description of the PLL, > which is not really enough to write a function to configure the PLL for > arbitrary link-frequencies. Working from an OV2680 datasheet I've found on the internet [1] I'll agree with you that the pixel clock is intended only ever to be 66MHz. With the FIFO at the front of the "image output interface" in fig 2-1, and the dividers shown in fig 2-8, the link frequencies could differ from one fixed ratio from the pixel clock. Not being able to produce fully arbitrary link frequencies is totally acceptable in a driver, so there's no obligation to understand or control those dividers. > The adjustment to make the sensor work with a 19.2MHz xvclk instead > of the default 24 MHz comes from the atomisp code. How to make other > adjustments would pretty much be guess work. > > I guess we could add code to check the link-frequencies and check > that there is only 1 and it matches the expected 330 MHz then the driver > still honors the link-frequencies property while at the same time > sticking with the fixed setup the sensor is intended to be used with. > > Would adding a link-frequency check like that work for you ? This is the approach taken in other drivers - see imx214 and imx219 for 2 examples (I've not gone looking for more). > And if yes what should the link-frequency control return then, > the actual achieved frequency (this would be better IMHO) or > the one from the property ? You've checked that the link-frequency matches the expected 330MHz, so isn't the actual achieved frequency the same as the one configured? I'm confused by your statement. I'm assuming that it's the ipu3-cio that needs link-frequency, as AFAIK libcamera doesn't. Dave [1] http://www.datasheet.es/PDF/1045403/OV2680-pdf.html > Regards, > > Hans > >
Hi Dave, On 7/4/23 13:23, Dave Stevenson wrote: > Hi Hans and Dan > > On Tue, 4 Jul 2023 at 10:45, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Dan, >> >> On 7/4/23 11:22, Dan Scally wrote: >>> Morning Hans >>> >>> On 27/06/2023 15:18, Hans de Goede wrote: >>>> Add read-only link-freq and pixel-rate controls. This is necessary for >>>> the sensor to work with the ipu3-cio2 driver and for libcamera. >>>> >>>> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/media/i2c/ov2680.c | 23 +++++++++++++++++++++++ >>>> 1 file changed, 23 insertions(+) >>>> >>>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c >>>> index 8bc542df1890..95d3152ddd22 100644 >>>> --- a/drivers/media/i2c/ov2680.c >>>> +++ b/drivers/media/i2c/ov2680.c >>>> @@ -75,6 +75,12 @@ >>>> #define OV2680_MIN_CROP_WIDTH 2 >>>> #define OV2680_MIN_CROP_HEIGHT 2 >>>> +/* Fixed pre-div of 1/2 */ >>>> +#define OV2680_PLL_PREDIV0 2 >>>> + >>>> +/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */ >>>> +#define OV2680_PLL_PREDIV 2 >>>> + >>>> /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */ >>>> #define OV2680_PIXELS_PER_LINE 1704 >>>> #define OV2680_LINES_PER_FRAME 1294 >>>> @@ -118,6 +124,8 @@ struct ov2680_ctrls { >>>> struct v4l2_ctrl *hflip; >>>> struct v4l2_ctrl *vflip; >>>> struct v4l2_ctrl *test_pattern; >>>> + struct v4l2_ctrl *link_freq; >>>> + struct v4l2_ctrl *pixel_rate; >>>> }; >>>> struct ov2680_mode { >>>> @@ -145,6 +153,8 @@ struct ov2680_dev { >>>> struct clk *xvclk; >>>> u32 xvclk_freq; >>>> u8 pll_mult; >>>> + s64 link_freq[1]; >>>> + s64 pixel_rate; >>>> struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES]; >>>> struct gpio_desc *pwdn_gpio; >>>> @@ -906,6 +916,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >>>> ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, >>>> 0, 1023, 1, 250); >>>> + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, >>>> + 0, 0, sensor->link_freq); >>>> + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, >>>> + 0, sensor->pixel_rate, >>>> + 1, sensor->pixel_rate); >>>> + >>>> if (hdl->error) { >>>> ret = hdl->error; >>>> goto cleanup_entity; >>>> @@ -913,6 +929,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >>>> ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; >>>> ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; >>>> + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>> sensor->sd.ctrl_handler = hdl; >>>> @@ -1030,6 +1047,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) >>>> sensor->pll_mult = ov2680_pll_multipliers[i]; >>>> + sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 / >>>> + OV2680_PLL_PREDIV * sensor->pll_mult; >>>> + >>>> + /* CSI-2 is double data rate, bus-format is 10 bpp */ >>>> + sensor->pixel_rate = sensor->link_freq[0] * 2 / 10; >>> >>> >>> I'm a little unsure on this one. My understanding is that the link frequency really ought to come from the endpoint properties (which in our case would be set by the ipu-bridge; though it doesn't for this sensor at the moment because I didn't understand it properly back then) because it's a platform specific thing. What the value should be, I have been determining by reading the PLL settings for the sensor whilst the laptop's running Windows. So whilst this is probably technically fine in supporting the link frequency that the driver already expects to configure for whatever platform this was originally designed for, my guess would be that the Miix expects a different link frequency and ideally we'd support that instead. For example see these commits for the ov7251: >> >> The datasheet is clear that the ov2680 is intended to be used with >> a fixed pixelclock of 66 MHz: >> >> "2.2 architecture >> >> The OV2680 sensor core generates streaming pixel data at a constant >> frame rate to a pixel clock of 66 MHz." >> >> and the ov2680 always operates in single lane mode. So there really is >> not much to configure here. >> >> Also the datasheet only contains a rudimentary description of the PLL, >> which is not really enough to write a function to configure the PLL for >> arbitrary link-frequencies. > > Working from an OV2680 datasheet I've found on the internet [1] I'll > agree with you that the pixel clock is intended only ever to be 66MHz. > > With the FIFO at the front of the "image output interface" in fig 2-1, > and the dividers shown in fig 2-8, the link frequencies could differ > from one fixed ratio from the pixel clock. > Not being able to produce fully arbitrary link frequencies is totally > acceptable in a driver, so there's no obligation to understand or > control those dividers. > >> The adjustment to make the sensor work with a 19.2MHz xvclk instead >> of the default 24 MHz comes from the atomisp code. How to make other >> adjustments would pretty much be guess work. >> >> I guess we could add code to check the link-frequencies and check >> that there is only 1 and it matches the expected 330 MHz then the driver >> still honors the link-frequencies property while at the same time >> sticking with the fixed setup the sensor is intended to be used with. >> >> Would adding a link-frequency check like that work for you ? > > This is the approach taken in other drivers - see imx214 and imx219 > for 2 examples (I've not gone looking for more). > >> And if yes what should the link-frequency control return then, >> the actual achieved frequency (this would be better IMHO) or >> the one from the property ? > > You've checked that the link-frequency matches the expected 330MHz, so > isn't the actual achieved frequency the same as the one configured? > I'm confused by your statement. On x86 devices the ov2680 is used with a 19.2 MHz clock from the PMC rather then with a 24 MHz clk. This results in a link-frequency of: 331.2 MHz instead of 330 MHz (and a matching pixel-clk of 66.24 MHz instead of 66 MHz). So my question is do we make the link-frequency control return 331.2 MHz or 330 MHz when a 19.2 MHz xvclk is used ? 330 MHz is what will be requested in the link-frequency device property. 331.2 MHz will be what is actually used. (the 24MHz xvclk scenario results in an exact link-freq of 330MHz) Regards, Hans > > I'm assuming that it's the ipu3-cio that needs link-frequency, as > AFAIK libcamera doesn't. > > Dave > > [1] http://www.datasheet.es/PDF/1045403/OV2680-pdf.html > >> Regards, >> >> Hans >> >> >
p.s. On 7/4/23 13:29, Hans de Goede wrote: > Hi Dave, > > On 7/4/23 13:23, Dave Stevenson wrote: >> Hi Hans and Dan >> >> On Tue, 4 Jul 2023 at 10:45, Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Hi Dan, >>> >>> On 7/4/23 11:22, Dan Scally wrote: >>>> Morning Hans >>>> >>>> On 27/06/2023 15:18, Hans de Goede wrote: >>>>> Add read-only link-freq and pixel-rate controls. This is necessary for >>>>> the sensor to work with the ipu3-cio2 driver and for libcamera. >>>>> >>>>> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> --- >>>>> drivers/media/i2c/ov2680.c | 23 +++++++++++++++++++++++ >>>>> 1 file changed, 23 insertions(+) >>>>> >>>>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c >>>>> index 8bc542df1890..95d3152ddd22 100644 >>>>> --- a/drivers/media/i2c/ov2680.c >>>>> +++ b/drivers/media/i2c/ov2680.c >>>>> @@ -75,6 +75,12 @@ >>>>> #define OV2680_MIN_CROP_WIDTH 2 >>>>> #define OV2680_MIN_CROP_HEIGHT 2 >>>>> +/* Fixed pre-div of 1/2 */ >>>>> +#define OV2680_PLL_PREDIV0 2 >>>>> + >>>>> +/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */ >>>>> +#define OV2680_PLL_PREDIV 2 >>>>> + >>>>> /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */ >>>>> #define OV2680_PIXELS_PER_LINE 1704 >>>>> #define OV2680_LINES_PER_FRAME 1294 >>>>> @@ -118,6 +124,8 @@ struct ov2680_ctrls { >>>>> struct v4l2_ctrl *hflip; >>>>> struct v4l2_ctrl *vflip; >>>>> struct v4l2_ctrl *test_pattern; >>>>> + struct v4l2_ctrl *link_freq; >>>>> + struct v4l2_ctrl *pixel_rate; >>>>> }; >>>>> struct ov2680_mode { >>>>> @@ -145,6 +153,8 @@ struct ov2680_dev { >>>>> struct clk *xvclk; >>>>> u32 xvclk_freq; >>>>> u8 pll_mult; >>>>> + s64 link_freq[1]; >>>>> + s64 pixel_rate; >>>>> struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES]; >>>>> struct gpio_desc *pwdn_gpio; >>>>> @@ -906,6 +916,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >>>>> ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, >>>>> 0, 1023, 1, 250); >>>>> + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, >>>>> + 0, 0, sensor->link_freq); >>>>> + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, >>>>> + 0, sensor->pixel_rate, >>>>> + 1, sensor->pixel_rate); >>>>> + >>>>> if (hdl->error) { >>>>> ret = hdl->error; >>>>> goto cleanup_entity; >>>>> @@ -913,6 +929,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >>>>> ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; >>>>> ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; >>>>> + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>>> sensor->sd.ctrl_handler = hdl; >>>>> @@ -1030,6 +1047,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) >>>>> sensor->pll_mult = ov2680_pll_multipliers[i]; >>>>> + sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 / >>>>> + OV2680_PLL_PREDIV * sensor->pll_mult; >>>>> + >>>>> + /* CSI-2 is double data rate, bus-format is 10 bpp */ >>>>> + sensor->pixel_rate = sensor->link_freq[0] * 2 / 10; >>>> >>>> >>>> I'm a little unsure on this one. My understanding is that the link frequency really ought to come from the endpoint properties (which in our case would be set by the ipu-bridge; though it doesn't for this sensor at the moment because I didn't understand it properly back then) because it's a platform specific thing. What the value should be, I have been determining by reading the PLL settings for the sensor whilst the laptop's running Windows. So whilst this is probably technically fine in supporting the link frequency that the driver already expects to configure for whatever platform this was originally designed for, my guess would be that the Miix expects a different link frequency and ideally we'd support that instead. For example see these commits for the ov7251: >>> >>> The datasheet is clear that the ov2680 is intended to be used with >>> a fixed pixelclock of 66 MHz: >>> >>> "2.2 architecture >>> >>> The OV2680 sensor core generates streaming pixel data at a constant >>> frame rate to a pixel clock of 66 MHz." >>> >>> and the ov2680 always operates in single lane mode. So there really is >>> not much to configure here. >>> >>> Also the datasheet only contains a rudimentary description of the PLL, >>> which is not really enough to write a function to configure the PLL for >>> arbitrary link-frequencies. >> >> Working from an OV2680 datasheet I've found on the internet [1] I'll >> agree with you that the pixel clock is intended only ever to be 66MHz. >> >> With the FIFO at the front of the "image output interface" in fig 2-1, >> and the dividers shown in fig 2-8, the link frequencies could differ >> from one fixed ratio from the pixel clock. >> Not being able to produce fully arbitrary link frequencies is totally >> acceptable in a driver, so there's no obligation to understand or >> control those dividers. >> >>> The adjustment to make the sensor work with a 19.2MHz xvclk instead >>> of the default 24 MHz comes from the atomisp code. How to make other >>> adjustments would pretty much be guess work. >>> >>> I guess we could add code to check the link-frequencies and check >>> that there is only 1 and it matches the expected 330 MHz then the driver >>> still honors the link-frequencies property while at the same time >>> sticking with the fixed setup the sensor is intended to be used with. >>> >>> Would adding a link-frequency check like that work for you ? >> >> This is the approach taken in other drivers - see imx214 and imx219 >> for 2 examples (I've not gone looking for more). >> >>> And if yes what should the link-frequency control return then, >>> the actual achieved frequency (this would be better IMHO) or >>> the one from the property ? >> >> You've checked that the link-frequency matches the expected 330MHz, so >> isn't the actual achieved frequency the same as the one configured? >> I'm confused by your statement. > > On x86 devices the ov2680 is used with a 19.2 MHz clock from > the PMC rather then with a 24 MHz clk. > > This results in a link-frequency of: 331.2 MHz instead of 330 > MHz (and a matching pixel-clk of 66.24 MHz instead of 66 MHz). > > So my question is do we make the link-frequency control > return 331.2 MHz or 330 MHz when a 19.2 MHz xvclk is used ? > > 330 MHz is what will be requested in the link-frequency > device property. 331.2 MHz will be what is actually used. > > (the 24MHz xvclk scenario results in an exact link-freq of > 330MHz) I just realized that since both the xvclk frequency and the link-frequency property will both come from the same ACPI glue we could fix this be actually putting 331.2 MHz in the property and making the driver accept this as valid when a 19.2 MHz xvclk is used. That would make the property and the actually achieved link-frequency be in sync again. So this is probably the best way to handle this. Does this solution work for everyone ? Regards, Hans >> I'm assuming that it's the ipu3-cio that needs link-frequency, as >> AFAIK libcamera doesn't. >> >> Dave >> >> [1] http://www.datasheet.es/PDF/1045403/OV2680-pdf.html >> >>> Regards, >>> >>> Hans >>> >>> >> >
Hello On 04/07/2023 13:32, Hans de Goede wrote: > p.s. > > On 7/4/23 13:29, Hans de Goede wrote: >> Hi Dave, >> >> On 7/4/23 13:23, Dave Stevenson wrote: >>> Hi Hans and Dan >>> >>> On Tue, 4 Jul 2023 at 10:45, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi Dan, >>>> >>>> On 7/4/23 11:22, Dan Scally wrote: >>>>> Morning Hans >>>>> >>>>> On 27/06/2023 15:18, Hans de Goede wrote: >>>>>> Add read-only link-freq and pixel-rate controls. This is necessary for >>>>>> the sensor to work with the ipu3-cio2 driver and for libcamera. >>>>>> >>>>>> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>> --- >>>>>> drivers/media/i2c/ov2680.c | 23 +++++++++++++++++++++++ >>>>>> 1 file changed, 23 insertions(+) >>>>>> >>>>>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c >>>>>> index 8bc542df1890..95d3152ddd22 100644 >>>>>> --- a/drivers/media/i2c/ov2680.c >>>>>> +++ b/drivers/media/i2c/ov2680.c >>>>>> @@ -75,6 +75,12 @@ >>>>>> #define OV2680_MIN_CROP_WIDTH 2 >>>>>> #define OV2680_MIN_CROP_HEIGHT 2 >>>>>> +/* Fixed pre-div of 1/2 */ >>>>>> +#define OV2680_PLL_PREDIV0 2 >>>>>> + >>>>>> +/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */ >>>>>> +#define OV2680_PLL_PREDIV 2 >>>>>> + >>>>>> /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */ >>>>>> #define OV2680_PIXELS_PER_LINE 1704 >>>>>> #define OV2680_LINES_PER_FRAME 1294 >>>>>> @@ -118,6 +124,8 @@ struct ov2680_ctrls { >>>>>> struct v4l2_ctrl *hflip; >>>>>> struct v4l2_ctrl *vflip; >>>>>> struct v4l2_ctrl *test_pattern; >>>>>> + struct v4l2_ctrl *link_freq; >>>>>> + struct v4l2_ctrl *pixel_rate; >>>>>> }; >>>>>> struct ov2680_mode { >>>>>> @@ -145,6 +153,8 @@ struct ov2680_dev { >>>>>> struct clk *xvclk; >>>>>> u32 xvclk_freq; >>>>>> u8 pll_mult; >>>>>> + s64 link_freq[1]; >>>>>> + s64 pixel_rate; >>>>>> struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES]; >>>>>> struct gpio_desc *pwdn_gpio; >>>>>> @@ -906,6 +916,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >>>>>> ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, >>>>>> 0, 1023, 1, 250); >>>>>> + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, >>>>>> + 0, 0, sensor->link_freq); >>>>>> + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, >>>>>> + 0, sensor->pixel_rate, >>>>>> + 1, sensor->pixel_rate); >>>>>> + >>>>>> if (hdl->error) { >>>>>> ret = hdl->error; >>>>>> goto cleanup_entity; >>>>>> @@ -913,6 +929,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) >>>>>> ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; >>>>>> ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; >>>>>> + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; >>>>>> sensor->sd.ctrl_handler = hdl; >>>>>> @@ -1030,6 +1047,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) >>>>>> sensor->pll_mult = ov2680_pll_multipliers[i]; >>>>>> + sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 / >>>>>> + OV2680_PLL_PREDIV * sensor->pll_mult; >>>>>> + >>>>>> + /* CSI-2 is double data rate, bus-format is 10 bpp */ >>>>>> + sensor->pixel_rate = sensor->link_freq[0] * 2 / 10; >>>>> >>>>> I'm a little unsure on this one. My understanding is that the link frequency really ought to come from the endpoint properties (which in our case would be set by the ipu-bridge; though it doesn't for this sensor at the moment because I didn't understand it properly back then) because it's a platform specific thing. What the value should be, I have been determining by reading the PLL settings for the sensor whilst the laptop's running Windows. So whilst this is probably technically fine in supporting the link frequency that the driver already expects to configure for whatever platform this was originally designed for, my guess would be that the Miix expects a different link frequency and ideally we'd support that instead. For example see these commits for the ov7251: >>>> The datasheet is clear that the ov2680 is intended to be used with >>>> a fixed pixelclock of 66 MHz: >>>> >>>> "2.2 architecture >>>> >>>> The OV2680 sensor core generates streaming pixel data at a constant >>>> frame rate to a pixel clock of 66 MHz." >>>> >>>> and the ov2680 always operates in single lane mode. So there really is >>>> not much to configure here. >>>> >>>> Also the datasheet only contains a rudimentary description of the PLL, >>>> which is not really enough to write a function to configure the PLL for >>>> arbitrary link-frequencies. >>> Working from an OV2680 datasheet I've found on the internet [1] I'll >>> agree with you that the pixel clock is intended only ever to be 66MHz. >>> >>> With the FIFO at the front of the "image output interface" in fig 2-1, >>> and the dividers shown in fig 2-8, the link frequencies could differ >>> from one fixed ratio from the pixel clock. >>> Not being able to produce fully arbitrary link frequencies is totally >>> acceptable in a driver, so there's no obligation to understand or >>> control those dividers. >>> >>>> The adjustment to make the sensor work with a 19.2MHz xvclk instead >>>> of the default 24 MHz comes from the atomisp code. How to make other >>>> adjustments would pretty much be guess work. >>>> >>>> I guess we could add code to check the link-frequencies and check >>>> that there is only 1 and it matches the expected 330 MHz then the driver >>>> still honors the link-frequencies property while at the same time >>>> sticking with the fixed setup the sensor is intended to be used with. >>>> >>>> Would adding a link-frequency check like that work for you ? >>> This is the approach taken in other drivers - see imx214 and imx219 >>> for 2 examples (I've not gone looking for more). >>> >>>> And if yes what should the link-frequency control return then, >>>> the actual achieved frequency (this would be better IMHO) or >>>> the one from the property ? >>> You've checked that the link-frequency matches the expected 330MHz, so >>> isn't the actual achieved frequency the same as the one configured? >>> I'm confused by your statement. >> On x86 devices the ov2680 is used with a 19.2 MHz clock from >> the PMC rather then with a 24 MHz clk. >> >> This results in a link-frequency of: 331.2 MHz instead of 330 >> MHz (and a matching pixel-clk of 66.24 MHz instead of 66 MHz). >> >> So my question is do we make the link-frequency control >> return 331.2 MHz or 330 MHz when a 19.2 MHz xvclk is used ? >> >> 330 MHz is what will be requested in the link-frequency >> device property. 331.2 MHz will be what is actually used. >> >> (the 24MHz xvclk scenario results in an exact link-freq of >> 330MHz) > I just realized that since both the xvclk frequency and the > link-frequency property will both come from the same ACPI > glue we could fix this be actually putting 331.2 MHz > in the property and making the driver accept this as valid > when a 19.2 MHz xvclk is used. > > That would make the property and the actually achieved > link-frequency be in sync again. So this is probably the > best way to handle this. > > Does this solution work for everyone ? Yep that sounds good to me then. > > Regards, > > Hans > > > > > >>> I'm assuming that it's the ipu3-cio that needs link-frequency, as >>> AFAIK libcamera doesn't. >>> >>> Dave >>> >>> [1] http://www.datasheet.es/PDF/1045403/OV2680-pdf.html >>> >>>> Regards, >>>> >>>> Hans >>>> >>>>
On Tue, 4 Jul 2023 at 12:32, Hans de Goede <hdegoede@redhat.com> wrote: > > p.s. > > On 7/4/23 13:29, Hans de Goede wrote: > > Hi Dave, > > > > On 7/4/23 13:23, Dave Stevenson wrote: > >> Hi Hans and Dan > >> > >> On Tue, 4 Jul 2023 at 10:45, Hans de Goede <hdegoede@redhat.com> wrote: > >>> > >>> Hi Dan, > >>> > >>> On 7/4/23 11:22, Dan Scally wrote: > >>>> Morning Hans > >>>> > >>>> On 27/06/2023 15:18, Hans de Goede wrote: > >>>>> Add read-only link-freq and pixel-rate controls. This is necessary for > >>>>> the sensor to work with the ipu3-cio2 driver and for libcamera. > >>>>> > >>>>> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>>> --- > >>>>> drivers/media/i2c/ov2680.c | 23 +++++++++++++++++++++++ > >>>>> 1 file changed, 23 insertions(+) > >>>>> > >>>>> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > >>>>> index 8bc542df1890..95d3152ddd22 100644 > >>>>> --- a/drivers/media/i2c/ov2680.c > >>>>> +++ b/drivers/media/i2c/ov2680.c > >>>>> @@ -75,6 +75,12 @@ > >>>>> #define OV2680_MIN_CROP_WIDTH 2 > >>>>> #define OV2680_MIN_CROP_HEIGHT 2 > >>>>> +/* Fixed pre-div of 1/2 */ > >>>>> +#define OV2680_PLL_PREDIV0 2 > >>>>> + > >>>>> +/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */ > >>>>> +#define OV2680_PLL_PREDIV 2 > >>>>> + > >>>>> /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */ > >>>>> #define OV2680_PIXELS_PER_LINE 1704 > >>>>> #define OV2680_LINES_PER_FRAME 1294 > >>>>> @@ -118,6 +124,8 @@ struct ov2680_ctrls { > >>>>> struct v4l2_ctrl *hflip; > >>>>> struct v4l2_ctrl *vflip; > >>>>> struct v4l2_ctrl *test_pattern; > >>>>> + struct v4l2_ctrl *link_freq; > >>>>> + struct v4l2_ctrl *pixel_rate; > >>>>> }; > >>>>> struct ov2680_mode { > >>>>> @@ -145,6 +153,8 @@ struct ov2680_dev { > >>>>> struct clk *xvclk; > >>>>> u32 xvclk_freq; > >>>>> u8 pll_mult; > >>>>> + s64 link_freq[1]; > >>>>> + s64 pixel_rate; > >>>>> struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES]; > >>>>> struct gpio_desc *pwdn_gpio; > >>>>> @@ -906,6 +916,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > >>>>> ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, > >>>>> 0, 1023, 1, 250); > >>>>> + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, > >>>>> + 0, 0, sensor->link_freq); > >>>>> + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, > >>>>> + 0, sensor->pixel_rate, > >>>>> + 1, sensor->pixel_rate); > >>>>> + > >>>>> if (hdl->error) { > >>>>> ret = hdl->error; > >>>>> goto cleanup_entity; > >>>>> @@ -913,6 +929,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) > >>>>> ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; > >>>>> ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; > >>>>> + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > >>>>> sensor->sd.ctrl_handler = hdl; > >>>>> @@ -1030,6 +1047,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) > >>>>> sensor->pll_mult = ov2680_pll_multipliers[i]; > >>>>> + sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 / > >>>>> + OV2680_PLL_PREDIV * sensor->pll_mult; > >>>>> + > >>>>> + /* CSI-2 is double data rate, bus-format is 10 bpp */ > >>>>> + sensor->pixel_rate = sensor->link_freq[0] * 2 / 10; > >>>> > >>>> > >>>> I'm a little unsure on this one. My understanding is that the link frequency really ought to come from the endpoint properties (which in our case would be set by the ipu-bridge; though it doesn't for this sensor at the moment because I didn't understand it properly back then) because it's a platform specific thing. What the value should be, I have been determining by reading the PLL settings for the sensor whilst the laptop's running Windows. So whilst this is probably technically fine in supporting the link frequency that the driver already expects to configure for whatever platform this was originally designed for, my guess would be that the Miix expects a different link frequency and ideally we'd support that instead. For example see these commits for the ov7251: > >>> > >>> The datasheet is clear that the ov2680 is intended to be used with > >>> a fixed pixelclock of 66 MHz: > >>> > >>> "2.2 architecture > >>> > >>> The OV2680 sensor core generates streaming pixel data at a constant > >>> frame rate to a pixel clock of 66 MHz." > >>> > >>> and the ov2680 always operates in single lane mode. So there really is > >>> not much to configure here. > >>> > >>> Also the datasheet only contains a rudimentary description of the PLL, > >>> which is not really enough to write a function to configure the PLL for > >>> arbitrary link-frequencies. > >> > >> Working from an OV2680 datasheet I've found on the internet [1] I'll > >> agree with you that the pixel clock is intended only ever to be 66MHz. > >> > >> With the FIFO at the front of the "image output interface" in fig 2-1, > >> and the dividers shown in fig 2-8, the link frequencies could differ > >> from one fixed ratio from the pixel clock. > >> Not being able to produce fully arbitrary link frequencies is totally > >> acceptable in a driver, so there's no obligation to understand or > >> control those dividers. > >> > >>> The adjustment to make the sensor work with a 19.2MHz xvclk instead > >>> of the default 24 MHz comes from the atomisp code. How to make other > >>> adjustments would pretty much be guess work. > >>> > >>> I guess we could add code to check the link-frequencies and check > >>> that there is only 1 and it matches the expected 330 MHz then the driver > >>> still honors the link-frequencies property while at the same time > >>> sticking with the fixed setup the sensor is intended to be used with. > >>> > >>> Would adding a link-frequency check like that work for you ? > >> > >> This is the approach taken in other drivers - see imx214 and imx219 > >> for 2 examples (I've not gone looking for more). > >> > >>> And if yes what should the link-frequency control return then, > >>> the actual achieved frequency (this would be better IMHO) or > >>> the one from the property ? > >> > >> You've checked that the link-frequency matches the expected 330MHz, so > >> isn't the actual achieved frequency the same as the one configured? > >> I'm confused by your statement. > > > > On x86 devices the ov2680 is used with a 19.2 MHz clock from > > the PMC rather then with a 24 MHz clk. > > > > This results in a link-frequency of: 331.2 MHz instead of 330 > > MHz (and a matching pixel-clk of 66.24 MHz instead of 66 MHz). > > > > So my question is do we make the link-frequency control > > return 331.2 MHz or 330 MHz when a 19.2 MHz xvclk is used ? > > > > 330 MHz is what will be requested in the link-frequency > > device property. 331.2 MHz will be what is actually used. > > > > (the 24MHz xvclk scenario results in an exact link-freq of > > 330MHz) OK, that wasn't clear from the earlier discussion. > I just realized that since both the xvclk frequency and the > link-frequency property will both come from the same ACPI > glue we could fix this be actually putting 331.2 MHz > in the property and making the driver accept this as valid > when a 19.2 MHz xvclk is used. > > That would make the property and the actually achieved > link-frequency be in sync again. So this is probably the > best way to handle this. > > Does this solution work for everyone ? Yes, asking for the right thing sounds like a good solution. It's the same route as I went for with imx258 in [1]. There 24MHz results in a slightly different link freq to the existing 19.2MHz configuration. Dave [1] https://patchwork.linuxtv.org/project/linux-media/patch/20230530173000.3060865-9-dave.stevenson@raspberrypi.com/ > Regards, > > Hans > > > > > > >> I'm assuming that it's the ipu3-cio that needs link-frequency, as > >> AFAIK libcamera doesn't. > >> > >> Dave > >> > >> [1] http://www.datasheet.es/PDF/1045403/OV2680-pdf.html > >> > >>> Regards, > >>> > >>> Hans > >>> > >>> > >> > > >
On Tue, Jul 04, 2023 at 01:19:28PM +0100, Dave Stevenson wrote: > On Tue, 4 Jul 2023 at 12:32, Hans de Goede <hdegoede@redhat.com> wrote: > > On 7/4/23 13:29, Hans de Goede wrote: > > > On 7/4/23 13:23, Dave Stevenson wrote: ... > > > So my question is do we make the link-frequency control > > > return 331.2 MHz or 330 MHz when a 19.2 MHz xvclk is used ? > > > > > > 330 MHz is what will be requested in the link-frequency > > > device property. 331.2 MHz will be what is actually used. > > > > > > (the 24MHz xvclk scenario results in an exact link-freq of > > > 330MHz) > > OK, that wasn't clear from the earlier discussion. > > > I just realized that since both the xvclk frequency and the > > link-frequency property will both come from the same ACPI > > glue we could fix this be actually putting 331.2 MHz > > in the property and making the driver accept this as valid > > when a 19.2 MHz xvclk is used. > > > > That would make the property and the actually achieved > > link-frequency be in sync again. So this is probably the > > best way to handle this. > > > > Does this solution work for everyone ? > > Yes, asking for the right thing sounds like a good solution. +1 here. > It's the same route as I went for with imx258 in [1]. There 24MHz > results in a slightly different link freq to the existing 19.2MHz > configuration.
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c index 8bc542df1890..95d3152ddd22 100644 --- a/drivers/media/i2c/ov2680.c +++ b/drivers/media/i2c/ov2680.c @@ -75,6 +75,12 @@ #define OV2680_MIN_CROP_WIDTH 2 #define OV2680_MIN_CROP_HEIGHT 2 +/* Fixed pre-div of 1/2 */ +#define OV2680_PLL_PREDIV0 2 + +/* Pre-div configurable through reg 0x3080, left at its default of 0x02 : 1/2 */ +#define OV2680_PLL_PREDIV 2 + /* 66MHz pixel clock: 66MHz / 1704 * 1294 = 30fps */ #define OV2680_PIXELS_PER_LINE 1704 #define OV2680_LINES_PER_FRAME 1294 @@ -118,6 +124,8 @@ struct ov2680_ctrls { struct v4l2_ctrl *hflip; struct v4l2_ctrl *vflip; struct v4l2_ctrl *test_pattern; + struct v4l2_ctrl *link_freq; + struct v4l2_ctrl *pixel_rate; }; struct ov2680_mode { @@ -145,6 +153,8 @@ struct ov2680_dev { struct clk *xvclk; u32 xvclk_freq; u8 pll_mult; + s64 link_freq[1]; + s64 pixel_rate; struct regulator_bulk_data supplies[OV2680_NUM_SUPPLIES]; struct gpio_desc *pwdn_gpio; @@ -906,6 +916,12 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 1023, 1, 250); + ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ, + 0, 0, sensor->link_freq); + ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, + 0, sensor->pixel_rate, + 1, sensor->pixel_rate); + if (hdl->error) { ret = hdl->error; goto cleanup_entity; @@ -913,6 +929,7 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor) ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT; + ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; sensor->sd.ctrl_handler = hdl; @@ -1030,6 +1047,12 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) sensor->pll_mult = ov2680_pll_multipliers[i]; + sensor->link_freq[0] = sensor->xvclk_freq / OV2680_PLL_PREDIV0 / + OV2680_PLL_PREDIV * sensor->pll_mult; + + /* CSI-2 is double data rate, bus-format is 10 bpp */ + sensor->pixel_rate = sensor->link_freq[0] * 2 / 10; + return 0; }