| 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: <CAPY8ntAwe24LQCNWHbChAAGPo+UUiMXBeUrLU=XnoW00pHXdhQ@mail.gmail.com>
Date: Mon, 25 Apr 2022 18:57:32 +0100
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Adam Ford <aford173@...il.com>
Cc: Linux Media Mailing List <linux-media@...r.kernel.org>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
Tim Harvey <tharvey@...eworks.com>,
cstevens@...conembedded.com,
Adam Ford-BE <aford@...conembedded.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] media: i2c: imx219: Enable variable XCLK
On Mon, 25 Apr 2022 at 18:28, Adam Ford <aford173@...il.com> wrote:
>
> On Mon, Apr 25, 2022 at 11:13 AM Dave Stevenson
> <dave.stevenson@...pberrypi.com> wrote:
> >
> > Hi again
> >
> > On Mon, 25 Apr 2022 at 16:58, Dave Stevenson
> > <dave.stevenson@...pberrypi.com> wrote:
> > >
> > > Hi Adam
> > >
> > > I have no way of testing with an alternate XCLK value, so I'm working
> > > based on the datasheet only.
> > >
> > > On Tue, 12 Apr 2022 at 14:55, Adam Ford <aford173@...il.com> wrote:
> > > >
> > > > The datasheet shows the external clock can be anything
> > > > from 6MHz to 27MHz, but EXCK, PREPLLCK_VT_DIV and
> > > > PREPLLCK_OP_DIV need to change based on the clock, so
> > > > create helper functions to set these registers based on
> > > > the rate of xclk.
> > > >
> > > > Move the validation of the clock rate into imx219_check_hwcfg
> > > > which means delaying the call to it until after xclk has been
> > > > determined.
> > > >
> > > > Signed-off-by: Adam Ford <aford173@...il.com>
> > > > ---
> > > > drivers/media/i2c/imx219.c | 79 ++++++++++++++++++++++++++++++--------
> > > > 1 file changed, 63 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > index d13ce5c1ece6..08e7d0e72430 100644
> > > > --- a/drivers/media/i2c/imx219.c
> > > > +++ b/drivers/media/i2c/imx219.c
> > > > @@ -39,8 +39,12 @@
> > > > #define IMX219_REG_CHIP_ID 0x0000
> > > > #define IMX219_CHIP_ID 0x0219
> > > >
> > > > -/* External clock frequency is 24.0M */
> > > > -#define IMX219_XCLK_FREQ 24000000
> > > > +/* Default external clock frequency is 24.0M */
> > > > +#define IMX219_XCLK_MIN_FREQ 6000000
> > > > +#define IMX219_XCLK_MAX_FREQ 27000000
> > > > +#define IMX219_REG_EXCK 0x012a
> > > > +#define IMX219_REG_PREPLLCK_VT_DIV 0x0304
> > > > +#define IMX219_REG_PREPLLCK_OP_DIV 0x0305
> > > >
> > > > /* Pixel rate is fixed for all the modes */
> > > > #define IMX219_PIXEL_RATE 182400000
> > > > @@ -166,8 +170,6 @@ static const struct imx219_reg pll_clk_table[] = {
> > > >
> > > > {0x0301, 0x05}, /* VTPXCK_DIV */
> > > > {0x0303, 0x01}, /* VTSYSCK_DIV */
> > > > - {0x0304, 0x03}, /* PREPLLCK_VT_DIV 0x03 = AUTO set */
> > > > - {0x0305, 0x03}, /* PREPLLCK_OP_DIV 0x03 = AUTO set */
> > > > {0x0306, 0x00}, /* PLL_VT_MPY */
> > > > {0x0307, 0x39},
> > > > {0x030b, 0x01}, /* OP_SYS_CLK_DIV */
> > > > @@ -182,7 +184,6 @@ static const struct imx219_reg pll_clk_table[] = {
> > > > */
> > > > static const struct imx219_reg mode_3280x2464_regs[] = {
> > > > {0x0128, 0x00},
> > > > - {0x012a, 0x18},
> > > > {0x012b, 0x00},
> > > > {0x0164, 0x00},
> > > > {0x0165, 0x00},
> > > > @@ -222,7 +223,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = {
> > > >
> > > > static const struct imx219_reg mode_1920_1080_regs[] = {
> > > > {0x0128, 0x00},
> > > > - {0x012a, 0x18},
> > > > {0x012b, 0x00},
> > > > {0x0162, 0x0d},
> > > > {0x0163, 0x78},
> > > > @@ -262,7 +262,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = {
> > > >
> > > > static const struct imx219_reg mode_1640_1232_regs[] = {
> > > > {0x0128, 0x00},
> > > > - {0x012a, 0x18},
> > > > {0x012b, 0x00},
> > > > {0x0164, 0x00},
> > > > {0x0165, 0x00},
> > > > @@ -302,7 +301,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = {
> > > >
> > > > static const struct imx219_reg mode_640_480_regs[] = {
> > > > {0x0128, 0x00},
> > > > - {0x012a, 0x18},
> > > > {0x012b, 0x00},
> > > > {0x0162, 0x0d},
> > > > {0x0163, 0x78},
> > > > @@ -1015,6 +1013,50 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> > > > return ret;
> > > > };
> > > >
> > > > +static int imx219_set_exck_freq(struct imx219 *imx219)
> > > > +{
> > > > + int ret;
> > > > + /* The imx219 registers need MHz not Hz */
> > > > + u8 clk = (u8) (imx219->xclk_freq/1000000U);
> > > > +
> > > > + /* Set the clock frequency in MHz */
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_EXCK,
> > > > + IMX219_REG_VALUE_08BIT, clk);
> >
> > In reviewing your other patch I noticed that the EXCK register is
> > actually a 16 bit value. The integer part is in 0x012a, and the
> > fractional part (1/256) in 0x012b, which is currently initialised from
> > the mode tables.
> > Your division discards the fractional part totally, so if the
> > configured frequency was 19.2MHz, then it would be programmed
> > incorrectly.
> >
> > The value for register 0x012b needs to be computed and set here.
>
> That makes sense.
> I am understanding your comment about register 0x12b, would the
> example frequency of 19.2MHz translate to 51 for register 12b?
Yes, AIUI 19.2MHz would be 0x13 0x33 (decimal 19 and 51 / 256).
The datasheet lists the default as being a 7.6MHz clock with register
values 0x07 0x99 (7 and 153/256, or 7.5976MHz)
> Part of me thinks I should drop this patch because I have no real way
> to test it, and I don't like writing 'theoretical' code.
I have some reservations over it for the same reasons.
If you haven't got an actual use case for it, then drop it for now.
Dave
> adam
> >
> > > > +
> > > > + /* Configure the PREPLLCK_VT_DIV and PREPLLCK_OP_DIV for automatic */
> > > > + switch (clk) {
> > > > + case 6 ... 11:
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x01);
> > > > + if (ret)
> > > > + return ret;
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x01);
> > > > + return ret;
> > > > + case 12 ... 23:
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x02);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x02);
> > > > +
> > > > + return ret;
> > > > + case 24 ... 27:
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_VT_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x03);
> > > > + if (ret)
> > > > + return ret;
> > > > + ret = imx219_write_reg(imx219, IMX219_REG_PREPLLCK_OP_DIV,
> > > > + IMX219_REG_VALUE_08BIT, 0x03);
> > > > + return ret;
> > > > + default:
> > > > + /* Should never get here */
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > > > +
> > > > static int imx219_start_streaming(struct imx219 *imx219)
> > > > {
> > > > struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > > > @@ -1039,6 +1081,9 @@ static int imx219_start_streaming(struct imx219 *imx219)
> > > > goto err_rpm_put;
> > > > }
> > > >
> > > > + /* Configure clock based on reference clock frequency */
> > > > + imx219_set_exck_freq(imx219);
> > >
> > > You're not checking the return value from this function, so any I2C
> > > failures will be ignored.
> > >
> > > > +
> > > > /* Apply default values of current mode */
> > > > reg_list = &imx219->mode->reg_list;
> > > > ret = imx219_write_regs(imx219, reg_list->regs, reg_list->num_of_regs);
> > > > @@ -1428,6 +1473,13 @@ static int imx219_check_hwcfg(struct imx219 *imx219)
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + if ((imx219->xclk_freq < IMX219_XCLK_MIN_FREQ) ||
> > > > + imx219->xclk_freq > IMX219_XCLK_MAX_FREQ) {
> > > > + dev_err(&client->dev, "xclk frequency not supported: %d Hz\n",
> > >
> > > imx219->xclk_freq is unsigned, so %u
> > >
> > > > + imx219->xclk_freq);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -1478,10 +1530,6 @@ static int imx219_probe(struct i2c_client *client)
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - /* Check the hardware configuration in device tree */
> > > > - if (imx219_check_hwcfg(imx219))
> > > > - return -EINVAL;
> > > > -
> > > > /* Get system clock (xclk) */
> > > > imx219->xclk = devm_clk_get(dev, NULL);
> > > > if (IS_ERR(imx219->xclk)) {
> > > > @@ -1490,11 +1538,10 @@ static int imx219_probe(struct i2c_client *client)
> > > > }
> > > >
> > > > imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > >
> > > My bug admittedly, but clk_get_rate returns an unsigned long, but
> > > imx219->xclk_freq is u32.
> > > Ideally imx219->xclk_freq should be unsigned long to match, and the
> > > dev_err I commented on earlier should be %lu.
> > >
> > > Cheers.
> > > Dave
> > >
> > > > - if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> > > > - dev_err(dev, "xclk frequency not supported: %d Hz\n",
> > > > - imx219->xclk_freq);
> > > > +
> > > > + /* Check the hardware configuration in device tree */
> > > > + if (imx219_check_hwcfg(imx219))
> > > > return -EINVAL;
> > > > - }
> > > >
> > > > ret = imx219_get_regulators(imx219);
> > > > if (ret) {
> > > > --
> > > > 2.34.1
> > > >
Powered by blists - more mailing lists