linux-kernel - Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

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 for Android: free password hash cracker in your pocket

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

Message-ID: <3b54ab9b-4ffd-ab32-d495-fad6132ea504@microchip.com>
Date:   Tue, 29 Jun 2021 10:47:56 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <hugues.fruchet@...com>, <prabhakar.mahadev-lad.rj@...renesas.com>,
        <jacopo+renesas@...ndi.org>, <sakari.ailus@...ux.intel.com>,
        <laurent.pinchart@...asonboard.com>, <slongerbeam@...il.com>,
        <paul.kocialkowski@...tlin.com>
CC:     <linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-renesas-soc@...r.kernel.org>, <biju.das.jz@...renesas.com>,
        <prabhakar.csengg@...il.com>, <mchehab@...nel.org>
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP
 mode unless streaming

On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> Hi Prabhakar,
> 
> As discussed separately I would prefer to better understand issue before
> going to this patch.
> Nevertheless I have some remarks in code in case we'll need it at the end.
> 
> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
>> Keep the sensor in software power down mode and wake up only in
>> ov5640_set_stream_dvp() callback.
>>
>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>> Reviewed-by: Biju Das <biju.das.jz@...renesas.com>
>> Tested-by: Jacopo Mondi <jacopo@...ndi.org>
>> ---
>>    drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
>>    1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index 2fe4a7ac0592..880fde73a030 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -34,6 +34,8 @@
>>    #define OV5640_REG_SYS_RESET02              0x3002
>>    #define OV5640_REG_SYS_CLOCK_ENABLE02       0x3006
>>    #define OV5640_REG_SYS_CTRL0                0x3008
>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> 
> For the time being this section was only referring to registers
> addresses and bit details was explained into a comment right before
> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> 
>>    #define OV5640_REG_CHIP_ID          0x300a
>>    #define OV5640_REG_IO_MIPI_CTRL00   0x300e
>>    #define OV5640_REG_PAD_OUTPUT_ENABLE01      0x3017
>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>>                val = regs->val;
>>                mask = regs->mask;
>>
>> +             /* remain in power down mode for DVP */
>> +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
>> +                 val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
>> +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
>> +                     continue;
>> +
> 
> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> has been partially removed from big init sequence: for power-up part,
> but power-dwn remains at very beginning of sequence.
> We should completely remove 0x3008 from init sequence, including
> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> should be called at the right place instead.
> 
> 
>>                if (mask)
>>                        ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>>                else
>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>         * PAD OUTPUT ENABLE 02
>>         * - [7:2]:     D[5:0] output enable
>>         */
>> -     return ov5640_write_reg(sensor,
>> -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
>> -                             on ? 0xfc : 0);
>> +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
>> +                            on ? 0xfc : 0);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>> +                             OV5640_REG_SYS_CTRL0_SW_PWUP :
>> +                             OV5640_REG_SYS_CTRL0_SW_PWDN);
>>    }
>>
>>    static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>
> 
> 
> BR,
> Hugues.
> 


Hello everyone,

When I updated driver in my tree with this patch, I noticed that my 
setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped 
working.

It looks like the culprit is this patch.
I am not sure which is the best way to fix it.
It looks like initially things work fine when probing the driver, but 
when we want to start streaming at a later point, the register SYS_CTRL0 
cannot be written anymore.
Here is an output of the log:

  --- Opening /dev/video0...
     Trying source module v4l2...
     /dev/video0 opened.
     No input was specified, using the first.
     ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
     atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
     Error starting stream.
     VIDIOC_STREAMON: Remote I/O error
     Unable to use mmap. Using read instead.
     Unable to use read.

I am using a simple application to read from /dev/video0 (which is 
registered by the atmel-isc once the sensor probed)

I have a feeling that something is wrong related to power, but I cannot 
tell for sure.

Reverting this patch makes it work fine again.

Help is appreciated,
Thanks,
Eugen