linux-kernel - Re: [PATCH v3 12/12] media: i2c: imx214: Fix link frequency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPybu_2hEzpDbQ-RHTOEcEm4BqL7ctZ0EzvnTMW6xbJBjsoTSg@mail.gmail.com>
Date: Sun, 8 Dec 2024 22:05:52 +0100
From: Ricardo Ribalda Delgado <ribalda@...nel.org>
To: git@...tzsch.eu
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, ~postmarketos/upstreaming@...ts.sr.ht, 
	phone-devel@...r.kernel.org, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Vincent Knecht <vincent.knecht@...loo.org>
Subject: Re: [PATCH v3 12/12] media: i2c: imx214: Fix link frequency

Acked-by: Ricardo Ribalda <ribalda@...omium.org>

nit:  media: i2c: imx214: Fix link frequency validation



On Sat, Dec 7, 2024 at 9:49 PM André Apitzsch via B4 Relay
<devnull+git.apitzsch.eu@...nel.org> wrote:
>
> From: André Apitzsch <git@...tzsch.eu>
>
> The driver defines IMX214_DEFAULT_LINK_FREQ 480000000, and then
> IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10),
> which works out as 384MPix/s. (The 8 is 4 lanes and DDR.)
>
> Parsing the PLL registers with the defined 24MHz input. We're in single
> PLL mode, so MIPI frequency is directly linked to pixel rate.  VTCK ends
> up being 1200MHz, and VTPXCK and OPPXCK both are 120MHz.  Section 5.3
> "Frame rate calculation formula" says "Pixel rate
> [pixels/s] = VTPXCK [MHz] * 4", so 120 * 4 = 480MPix/s, which basically
> agrees with my number above.
>
> 3.1.4. MIPI global timing setting says "Output bitrate = OPPXCK * reg
> 0x113[7:0]", so 120MHz * 10, or 1200Mbit/s. That would be a link
> frequency of 600MHz due to DDR.
> That also matches to 480MPix/s * 10bpp / 4 lanes / 2 for DDR.
>
> Keep the previous link frequency for backward compatibility.
>
> Signed-off-by: André Apitzsch <git@...tzsch.eu>
> ---
>  drivers/media/i2c/imx214.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 1330f13207beec0960c384681bf0b49e99fe860f..910ad03cda23345d3d10d13cd30f007954534e80 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -31,7 +31,9 @@
>  #define IMX214_REG_FAST_STANDBY_CTRL   CCI_REG8(0x0106)
>
>  #define IMX214_DEFAULT_CLK_FREQ        24000000
> -#define IMX214_DEFAULT_LINK_FREQ 480000000
> +#define IMX214_DEFAULT_LINK_FREQ       600000000
> +/* Keep wrong link frequency for backward compatibility */
> +#define IMX214_DEFAULT_LINK_FREQ_LEGACY        480000000
>  #define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
>  #define IMX214_FPS 30
>
> @@ -1216,18 +1218,22 @@ static int imx214_parse_fwnode(struct device *dev)
>                 goto done;
>         }
>

Now that I think about this... We only support
buf_cfg.no_of_link_frequencies ==1.
Maybe you can add a check before the loop
if (bus_cfg.nr_of_link_frequencies != 1)
                       dev_warn(dev, "Only one link-frequency
supported, please review your DT. Continuing anyway\n ")


> -       for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> +       for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
>                 if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
>                         break;
> -
> -       if (i == bus_cfg.nr_of_link_frequencies) {
> -               dev_err_probe(dev, -EINVAL,
> -                             "link-frequencies %d not supported, Please review your DT\n",
> -                             IMX214_DEFAULT_LINK_FREQ);
> -               ret = -EINVAL;
> -               goto done;
> +               if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> +                       dev_warn(dev,
> +                                "link-frequencies %d not supported, please review your DT. Continuing anyway\n",
> +                                IMX214_DEFAULT_LINK_FREQ);
> +                       break;
> +               }
>         }
>
> +       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);
> +
>  done:
>         v4l2_fwnode_endpoint_free(&bus_cfg);
>         fwnode_handle_put(endpoint);
>
> --
> 2.47.1
>
>
>