| 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
| ||
|
Message-ID: <20250423180937.GB2675@pendragon.ideasonboard.com>
Date: Wed, 23 Apr 2025 21:09:37 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Mathis Foerst <mathis.foerst@...com>
Cc: linux-kernel@...r.kernel.org,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
linux-media@...r.kernel.org, devicetree@...r.kernel.org,
manuel.traut@...com, mathis.foerst@...hlke.com
Subject: Re: [PATCH v4 2/6] media: mt9m114: Bypass PLL if required
Hi Mathis,
Thank you for the patch.
On Fri, Mar 07, 2025 at 10:31:36AM +0100, Mathis Foerst wrote:
> The MT9M114 sensor has an internal PLL that generates the required SYSCLK
> from EXTCLK. It also has the option to bypass the PLL and use EXTCLK
> directly as SYSCLK.
> The current driver implementation uses a hardcoded PLL configuration that
> requires a specific EXTCLK frequency. Depending on the available clocks,
> it can be desirable to use a different PLL configuration or to bypass it.
>
> The link-frequency of the output bus (Parallel or MIPI-CSI) is configured
> in the device tree.
>
> Check if EXTCLK can be used as SYSCLK to achieve this link-frequency. If
> yes, bypass the PLL.
> Otherwise, (as before) check if EXTCLK and the default PLL configuration
> provide the required SYSCLK to achieve the link-frequency. If yes, use the
> PLL. If no, throw an error.
>
> Signed-off-by: Mathis Foerst <mathis.foerst@...com>
> ---
> drivers/media/i2c/mt9m114.c | 62 ++++++++++++++++++++++++++-----------
> 1 file changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 5f0b0ad8f885..b06003b69f6f 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -261,6 +261,7 @@
> #define MT9M114_CAM_PGA_PGA_CONTROL CCI_REG16(0xc95e)
> #define MT9M114_CAM_SYSCTL_PLL_ENABLE CCI_REG8(0xc97e)
> #define MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE BIT(0)
> +#define MT9M114_CAM_SYSCTL_PLL_DISABLE_VALUE 0x00
> #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_P CCI_REG16(0xc982)
> @@ -377,6 +378,7 @@ struct mt9m114 {
> struct gpio_desc *reset;
> struct regulator_bulk_data supplies[3];
> struct v4l2_fwnode_endpoint bus_cfg;
> + bool bypass_pll;
>
> struct {
> unsigned int m;
> @@ -743,14 +745,20 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
> }
>
> /* Configure the PLL. */
> - cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
> - MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE, &ret);
> - cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N,
> - MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(sensor->pll.m,
> - 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);
> + if (sensor->bypass_pll) {
> + cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
> + MT9M114_CAM_SYSCTL_PLL_DISABLE_VALUE, &ret);
> + } else {
> + cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
> + MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE, &ret);
> + cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N,
> + MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(sensor->pll.m,
> + 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);
> + }
You can add a blank line here.
> cci_write(sensor->regmap, MT9M114_CAM_SENSOR_CFG_PIXCLK,
> sensor->pixrate, &ret);
>
> @@ -2235,9 +2243,19 @@ static const struct dev_pm_ops mt9m114_pm_ops = {
> * Probe & Remove
> */
>
> +static int mt9m114_verify_link_frequency(struct mt9m114 *sensor)
> +{
> + unsigned int link_freq = sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY
> + ? sensor->pixrate * 8 : sensor->pixrate * 2;
? sensor->pixrate * 8 : sensor->pixrate * 2;
And missing blank line.
> + if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> + sensor->bus_cfg.link_frequencies[0] != link_freq)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int mt9m114_clk_init(struct mt9m114 *sensor)
> {
> - unsigned int link_freq;
>
> /* Hardcode the PLL multiplier and dividers to default settings. */
> sensor->pll.m = 32;
> @@ -2249,19 +2267,27 @@ static int mt9m114_clk_init(struct mt9m114 *sensor)
> * for 16-bit per pixel, transmitted in DDR over a single lane. For
> * parallel mode, the sensor ouputs one pixel in two PIXCLK cycles.
> */
> - sensor->pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
> - / ((sensor->pll.n + 1) * (sensor->pll.p + 1));
>
> - link_freq = sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY
> - ? sensor->pixrate * 8 : sensor->pixrate * 2;
> + /*
> + * Check if EXTCLK fits the configured link frequency. Bypass the PLL
> + * in this case.
> + */
> + sensor->pixrate = clk_get_rate(sensor->clk) / 2;
> + if (mt9m114_verify_link_frequency(sensor) == 0) {
I would be cleaner to pass the pixel rate as a parameter to the
function:
unsigned int pixrate;
...
pixrate = clk_get_rate(sensor->clk) / 2;
if (mt9m114_verify_link_frequency(sensor, pixrate) == 0) {
sensor->pixrate = pixrate;
sensor->bypass_pll = true;
return 0;
}
> + sensor->bypass_pll = true;
> + return 0;
> + }
>
> - if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> - sensor->bus_cfg.link_frequencies[0] != link_freq) {
> - dev_err(&sensor->client->dev, "Unsupported DT link-frequencies\n");
> - return -EINVAL;
> + /* Check if the PLL configuration fits the configured link frequency */
s/frequency/frequency./
With those small issues addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> + sensor->pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
> + / ((sensor->pll.n + 1) * (sensor->pll.p + 1));
> + if (mt9m114_verify_link_frequency(sensor) == 0) {
> + sensor->bypass_pll = false;
> + return 0;
> }
>
> - return 0;
> + dev_err(&sensor->client->dev, "Unsupported DT link-frequencies\n");
> + return -EINVAL;
> }
>
> static int mt9m114_identify(struct mt9m114 *sensor)
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists