Message-ID: <20250623114658.GB32376@pendragon.ideasonboard.com>
Date: Mon, 23 Jun 2025 14:46:58 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda Delgado <ribalda@...nel.org>
Cc: André Apitzsch <git@...tzsch.eu>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org,
	Ricardo Ribalda <ribalda@...omium.org>
Subject: Re: [PATCH RESEND v4 3/5] media: i2c: imx214: Make use of CCS PLL
 calculator

Hi Ricardo,

On Mon, Jun 23, 2025 at 11:31:17AM +0200, Ricardo Ribalda Delgado wrote:
> On Sun, Jun 22, 2025 at 7:13 PM Laurent Pinchart wrote:
> > On Sun, Jun 22, 2025 at 05:34:56PM +0200, André Apitzsch wrote:
> > > Am Samstag, dem 21.06.2025 um 21:17 +0300 schrieb Laurent Pinchart:
> > > > On Sat, Jun 21, 2025 at 11:37:27AM +0200, André Apitzsch via B4 Relay wrote:
> > > > > From: André Apitzsch <git@...tzsch.eu>
> > > > >
> > > > > Calculate PLL parameters based on clock frequency and link
> > > > > frequency.
> > > > >
> > > > > Acked-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > > Signed-off-by: André Apitzsch <git@...tzsch.eu>
> > > > > ---
> > > > >  drivers/media/i2c/Kconfig  |   1 +
> > > > >  drivers/media/i2c/imx214.c | 213 ++++++++++++++++++++++++++++++++++++---------
> > > > >  2 files changed, 175 insertions(+), 39 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > index
> > > > > e68202954a8fd4711d108cf295d5771246fbc406..08db8abeea218080b0bf5bfe6
> > > > > cf82f1c0b100c4a 100644
> > > > > --- a/drivers/media/i2c/Kconfig
> > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > [..]
> > > > > @@ -1224,42 +1336,52 @@ static int imx214_parse_fwnode(struct
> > > > > device *dev)
> > > > >   if (!endpoint)
> > > > >   return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
> > > > >
> > > > > - ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> > > > > + bus_cfg->bus_type = V4L2_MBUS_CSI2_DPHY;
> > > > > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, bus_cfg);
> > > > > + fwnode_handle_put(endpoint);
> > > >
> > > > ... drop this. Up to you.
> > > >
> > > > >   if (ret) {
> > > > >   dev_err_probe(dev, ret, "parsing endpoint node failed\n");
> > > > > - goto done;
> > > > > + goto error;
> > > >
> > > > You can return ret here.
> > > >
> > > > >   }
> > > > >
> > > > >   /* Check the number of MIPI CSI2 data lanes */
> > > > > - if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > > > > + if (bus_cfg->bus.mipi_csi2.num_data_lanes != 4) {
> > > > >   ret = dev_err_probe(dev, -EINVAL,
> > > > >       "only 4 data lanes are currently supported\n");
> > > > > - goto done;
> > > > > + goto error;
> > > > >   }
> > > > >
> > > > > - if (bus_cfg.nr_of_link_frequencies != 1)
> > > > > + if (bus_cfg->nr_of_link_frequencies != 1)
> > > > >   dev_warn(dev, "Only one link-frequency supported, please review
> > > > > your DT. Continuing anyway\n");
> > > >
> > > > Now that the driver can calculate PLL parameters dynamically, it
> > > > would be nice to lift this restriction and make the link frequency
> > > > control writable, in a separate patch on top of this series.
> > >
> > > Maybe this could be postponed, as I don't have any use for it at the
> > > moment and I don't want to further delay this series.
> >
> > When I said "on top", I didn't mean in a new version of this series. We
> > can merge this first, and then lift this restriction. I don't have an
> > imx214-based device so I can't do it myself and test it :-/
> >
> > > > > - for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > > > - if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
> > > > > + for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
> > > > > + u64 freq = bus_cfg->link_frequencies[i];
> > > > > + struct ccs_pll pll;
> > > > > +
> > > > > + if (!imx214_pll_calculate(imx214, &pll, freq))
> > > > >   break;
> > > > > - if (bus_cfg.link_frequencies[i] ==
> > > > > -     IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> > > > > + if (freq == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> > > > >   dev_warn(dev,
> > > > >   "link-frequencies %d not supported, please review your DT.
> > > > > Continuing anyway\n",
> > > > >   IMX214_DEFAULT_LINK_FREQ);
> > > > > + freq = IMX214_DEFAULT_LINK_FREQ;
> > > > > + if (imx214_pll_calculate(imx214, &pll, freq))
> > > > > + continue;
> > > > > + bus_cfg->link_frequencies[i] = freq;
> > > > >   break;
> > > > >   }
> > > >
> > > > How about separating the IMX214_DEFAULT_LINK_FREQ_LEGACY check from
> > > > the PLL calculation ? Something like
> > > >
> > > >  u64 freq = bus_cfg->link_frequencies[i];
> > > >  struct ccs_pll pll;
> > > >
> > > >  if (freq == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> > > >  dev_warn(dev,
> > > >  "link-frequencies %d not supported, please review your DT.
> > > > Continuing anyway\n",
> > > >  IMX214_DEFAULT_LINK_FREQ);
> > > >  freq = IMX214_DEFAULT_LINK_FREQ;
> > > >  bus_cfg->link_frequencies[i] = freq;
> > > >  }
> > >
> > > With PLL calculation, 480000000 (=IMX214_DEFAULT_LINK_FREQ_LEGACY)
> > > might be a valid link frequency explicitly set by the user. I'm not
> > > sure whether it is a good idea to overwrite the link frequency, before
> > > trying the PLL calculation. That's why I would keep the code the way it
> > > is.
> >
> > The current code accepts both IMX214_DEFAULT_LINK_FREQ (600 MHz) and
> > IMX214_DEFAULT_LINK_FREQ_LEGACY (400 MHz), and programs the PLL with (as
> > far as I understand) a 600 MHz clock frequency in either case. To avoid
> > a change in behaviour, I think overriding the 400 MHz frequency with 600
> > MHz in this patch would be best. We could then drop that in a later
> > patch, possibly by patching the clock frequency in a platform-specific
> > driver instead of the imx214 driver.
> >
> > > >  if (!imx214_pll_calculate(imx214, &pll, freq))
> > > >  break;
> > > >
> > > > It will then become easier to drop this legacy support from the
> > > > driver. What platform(s) are know to specify an incorrect link
> > > > frequency ?
> > >
> > > I don't know.
> >
> > Ricardo, do you have any information about this ?
> 
> This was for a development platform for Qualcomm, think of a pizero
> like, but with a Snapdragon device.
> 
> There was a Qtechnology product based on that platform. I asked them
> if they could provide me a device for testing, but it has been
> discontinued and  replaced with something better.
> 
> We can start to deprecate the clock quirk if you want.

That would be great. Thanks for the confirmation.

By deprecate, do you mean first printing a message for a few kernel
releases, or can we just drop it in the next version of this patch ?

> > > > >   }
> > > > >
> > > > > - if (i == bus_cfg.nr_of_link_frequencies)
> > > > > + if (i == bus_cfg->nr_of_link_frequencies)
> > > > >   ret = dev_err_probe(dev, -EINVAL,
> > > > > -     "link-frequencies %d not supported, please review your DT\n",
> > > > > -     IMX214_DEFAULT_LINK_FREQ);
> > > > > +     "link-frequencies %lld not supported, please review your
> > > > > DT\n",
> > > > > +     bus_cfg->nr_of_link_frequencies ?
> > > > > +     bus_cfg->link_frequencies[0] : 0);
> > > > >
> > > > > -done:
> > > > > - v4l2_fwnode_endpoint_free(&bus_cfg);
> > > > > - fwnode_handle_put(endpoint);
> > > > > + return 0;
> > > > > +
> > > > > +error:
> > > > > + v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> > > > >   return ret;
> > > > >  }
> > > > >
> > > > > @@ -1299,7 +1421,7 @@ static int imx214_probe(struct i2c_client
> > > > > *client)
> > > > >   return dev_err_probe(dev, PTR_ERR(imx214->regmap),
> > > > >        "failed to initialize CCI\n");
> > > > >
> > > > > - ret = imx214_parse_fwnode(dev);
> > > > > + ret = imx214_parse_fwnode(dev, imx214);
> > > > >   if (ret)
> > > > >   return ret;
> > > > >
> > > > > @@ -1310,7 +1432,9 @@ static int imx214_probe(struct i2c_client
> > > > > *client)
> > > > >   * Enable power initially, to avoid warnings
> > > > >   * from clk_disable on power_off
> > > > >   */
> > > > > - imx214_power_on(imx214->dev);
> > > > > + ret = imx214_power_on(imx214->dev);
> > > > > + if (ret < 0)
> > > > > + goto error_fwnode;
> > > >
> > > > This change seems to belong to a separate patch.
> > > >
> > > > >
> > > > >   ret = imx214_identify_module(imx214);
> > > > >   if (ret)
> > > > > @@ -1341,6 +1465,12 @@ static int imx214_probe(struct i2c_client
> > > > > *client)
> > > > >   pm_runtime_set_active(imx214->dev);
> > > > >   pm_runtime_enable(imx214->dev);
> > > > >
> > > > > + ret = imx214_pll_update(imx214);
> > > > > + if (ret < 0) {
> > > > > + dev_err_probe(dev, ret, "failed to update PLL\n");
> > > > > + goto error_subdev_cleanup;
> > > > > + }
> > > >
> > > > I would move this to imx214_ctrls_init().
> > > >
> > > > > +
> > > > >   ret = v4l2_async_register_subdev_sensor(&imx214->sd);
> > > > >   if (ret < 0) {
> > > > >   dev_err_probe(dev, ret,
> > > > > @@ -1366,6 +1496,9 @@ static int imx214_probe(struct i2c_client
> > > > > *client)
> > > > >  error_power_off:
> > > > >   imx214_power_off(imx214->dev);
> > > > >
> > > > > +error_fwnode:
> > > > > + v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> > > > > +
> > > > >   return ret;
> > > > >  }
> > > > >
> > > > > @@ -1378,6 +1511,8 @@ static void imx214_remove(struct i2c_client
> > > > > *client)
> > > > >   v4l2_subdev_cleanup(sd);
> > > > >   media_entity_cleanup(&imx214->sd.entity);
> > > > >   v4l2_ctrl_handler_free(&imx214->ctrls);
> > > > > + v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> > > > > +
> > > > >   pm_runtime_disable(&client->dev);
> > > > >   if (!pm_runtime_status_suspended(&client->dev)) {
> > > > >   imx214_power_off(imx214->dev);

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists