Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi, On Mon, May 07, 2018 at 06:00:55PM -0700, Sam Bobrowicz wrote: > > Hi, > > > > Here is a "small" series that mostly cleans up the ov5640 driver code, > > slowly getting rid of the big data array for more understandable code > > (hopefully). > > > > The biggest addition would be the clock rate computation at runtime, > > instead of relying on those arrays to setup the clock tree > > properly. As a side effect, it fixes the framerate that was off by > > around 10% on the smaller resolutions, and we now support 60fps. > > > > This also introduces a bunch of new features. > > > > Let me know what you think, > > Maxime > > > > Changes from v1: > > - Integrated Hugues' suggestions to fix v4l2-compliance > > - Fixed the bus width with JPEG > > - Dropped the clock rate calculation loops for something simpler as > > suggested by Sakari > > - Cache the exposure value instead of using the control value > > - Rebased on top of 4.17 > > > > Maxime Ripard (10): > > media: ov5640: Don't force the auto exposure state at start time > > media: ov5640: Init properly the SCLK dividers > > media: ov5640: Change horizontal and vertical resolutions name > > media: ov5640: Add horizontal and vertical totals > > media: ov5640: Program the visible resolution > > media: ov5640: Adjust the clock based on the expected rate > > media: ov5640: Compute the clock rate at runtime > > media: ov5640: Enhance FPS handling > > media: ov5640: Add 60 fps support > > media: ov5640: Remove duplicate auto-exposure setup > > > > Mylène Josserand (2): > > media: ov5640: Add auto-focus feature > > media: ov5640: Add light frequency control > > > > drivers/media/i2c/ov5640.c | 752 + > > 1 file changed, 422 insertions(+), 330 deletions(-) > > > > -- > > 2.17.0 > > > > As discussed, MIPI required some additional work. Please see the > patches here which add support for MIPI: > https://www.dropbox.com/s/73epty7808yzq1t/ov5640_mipi_fixes.zip?dl=0 > > The first 3 patches are fixes I believe should be made to earlier > patches prior to submitting v2 of this series. The remaining 4 patches > should probably just be added onto the end of this series as-is (or > with feedback incorporated if needed). > > I will note that this is still not working correctly on my system for > any resolution that requires a 672 Mbps mipi rate. This includes > 1080p@30hz, full@15hz, and 720p@60hz. My CSI2 receiver is reporting > CRC errors though, so this could be an integrity issue on my module. > I'm curious to hear if others have success at these resolutions. > > Please try this out on other MIPI and DVP platforms with as many > different resolutions as possible and let me know if it works. I've took some of your changes to remain as feature stable as possible in my series. Some other changes (like the PLL2 setup), while totally welcome, should be in a separate, subsequent series. DVP works as expected, after looking at the feedback from Loic (and the clock tree especially), some of the comments you made (like the bit divider being meaningless for DVP) are not totally accurate, so I've tried to make the best blend of all the feedback you gave. It still works properly with DVP, but I still don't have a MIPI camera to test, so I'm not sure this works, even though I should have the same setup than the one you reported. Thanks! Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
> Hi, > > Here is a "small" series that mostly cleans up the ov5640 driver code, > slowly getting rid of the big data array for more understandable code > (hopefully). > > The biggest addition would be the clock rate computation at runtime, > instead of relying on those arrays to setup the clock tree > properly. As a side effect, it fixes the framerate that was off by > around 10% on the smaller resolutions, and we now support 60fps. > > This also introduces a bunch of new features. > > Let me know what you think, > Maxime > > Changes from v1: > - Integrated Hugues' suggestions to fix v4l2-compliance > - Fixed the bus width with JPEG > - Dropped the clock rate calculation loops for something simpler as > suggested by Sakari > - Cache the exposure value instead of using the control value > - Rebased on top of 4.17 > > Maxime Ripard (10): > media: ov5640: Don't force the auto exposure state at start time > media: ov5640: Init properly the SCLK dividers > media: ov5640: Change horizontal and vertical resolutions name > media: ov5640: Add horizontal and vertical totals > media: ov5640: Program the visible resolution > media: ov5640: Adjust the clock based on the expected rate > media: ov5640: Compute the clock rate at runtime > media: ov5640: Enhance FPS handling > media: ov5640: Add 60 fps support > media: ov5640: Remove duplicate auto-exposure setup > > Mylène Josserand (2): > media: ov5640: Add auto-focus feature > media: ov5640: Add light frequency control > > drivers/media/i2c/ov5640.c | 752 + > 1 file changed, 422 insertions(+), 330 deletions(-) > > -- > 2.17.0 > As discussed, MIPI required some additional work. Please see the patches here which add support for MIPI: https://www.dropbox.com/s/73epty7808yzq1t/ov5640_mipi_fixes.zip?dl=0 The first 3 patches are fixes I believe should be made to earlier patches prior to submitting v2 of this series. The remaining 4 patches should probably just be added onto the end of this series as-is (or with feedback incorporated if needed). I will note that this is still not working correctly on my system for any resolution that requires a 672 Mbps mipi rate. This includes 1080p@30hz, full@15hz, and 720p@60hz. My CSI2 receiver is reporting CRC errors though, so this could be an integrity issue on my module. I'm curious to hear if others have success at these resolutions. Please try this out on other MIPI and DVP platforms with as many different resolutions as possible and let me know if it works. I'd also recommend that someone add a patch to the series that enables 720p@60hz. I didn't have time to do it yet, but it should be really easy. Sam
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
On Fri, May 04, 2018 at 02:13:16PM -0700, Sam Bobrowicz wrote: > > FYI, I've found the following document: > > https://community.nxp.com/servlet/JiveServlet/downloadImage/105-32914-99951/ov5640_diagram.jpg > > > > So it seems that clock tree is a bit different. > > So that is where the Digilent RnD team got their information. I was > working with an ASCII representation of the same diagram, thanks for > providing that. > > > > > So, from my understanding we need to calculate dividers so that: > > sclk = vtot * htot * fps > > pclk = sclk * byte-per-pixel > > mipisclk = 8 * pclk / num-lanes > > Unfortunately there are more factors at play depending on what > features you are using in the ISP. Easiest way for me to describe them > is to just complete my patches, then we can discuss further. Do we support any of those features at the moment? If not, then we shouldn't worry about it too much for now. Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
> Hi, > >> Good news, MIPI is now working on my platform. I had to make several >> changes to how the mipi clocking is calculated in order to get things >> stable, but I think I got it figured out. Maxime's changes were really >> helpful. > > Great, I also try to make it work with MIPI-CSI2, If you have found > the magic formula to configure the registers, I would be pleased to > test it on my side. > >> >> I will try to get some patches out today or tomorrow that should get >> you up and running. >> >> Maxime, I'd prefer to create the patches myself for the experience. >> I've read all of the submission guidelines and I understand the >> general process, but how should I submit them to the mailing list >> since they are based to your patches, which are still under review? >> Should I send the patch series to the mailing list myself and just >> mention this patch series, maybe with the In-Reply-To header? Or >> should I just post a link to them here so you can review them and add >> them to a new version of your patch series? > > Yes, I think your patch(es) should be integrated in the Maxime's series. > > Regards, > Loic Based on Maxime's reply, I have decided to make the patches based off of that series, post it on dropbox, and provide a link here. Then we can discuss how to integrate them into v2 of the series. The patches are taking longer than expected, my tree is pretty chaotic since I've been running rapid-fire experiments for weeks now. I'm still working on getting the changes organized into an appropriate set of patches. I think they will go out over the weekend. Sam
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
> Hi, > > On 3 May 2018 at 17:16, Maxime Ripard wrote: > > Hi, > > > > On Wed, May 02, 2018 at 11:11:55AM -0700, Sam Bobrowicz wrote: > >> > On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote: > >> >> FYI, still hard at work on this. Did some more experiments last week > >> >> that seemed to corroborate the clock tree in the spreadsheet. It also > >> >> seems that the output of the P divider cell, SCLK cell and MIPI Rate > >> >> cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) > >> >> in order for the sensor to work properly on my platform, and that the > >> >> SCLK value must be close to the "rate" variable that you calculate and > >> >> pass to set_mipi_pclk. Unfortunately, I've only got the sensor working > >> >> well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka > >> >> 84:42:336). I'm running experiments now trying to adjust the htot and > >> >> vtot values to create different required rates, and also to try to get > >> >> faster Mipi rates working. Any information you have on the > >> >> requirements of the htot and vtot values with respect to vact and hact > >> >> values would likely be helpful. > >> >> > >> >> I'm also keeping an eye on the scaler clock, which I think may be > >> >> affecting certain resolutions, but haven't been able to see it make a > >> >> difference yet (see register 0x3824 and 0x460c) > >> >> > >> >> I plan on pushing a set of patches once I get this figured out, we can > >> >> discuss what I should base them on when I get closer to that point. > >> >> I'm new to this process :) > >> > > >> > I'm also interested in getting the ov5640 driver working with MIPI CSI-2. > >> > Studying the datasheet and the driver code, I found the stream on > >> > sequence to > >> > be a bit weird. In particular the configuration of > >> > OV5640_REG_IO_MIPI_CTRL00, > >> > OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention. > >> > > >> > OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of > >> > init > >> > mode data and never touched for MIPI CSI-2 (the register is only touched > >> > in > >> > ov5640_set_stream_dvp). The value means > >> > > >> > - mipi_lane_mode: 010 is documented as "debug mode", I would have > >> > expected 000 > >> > for one lane or 001 for two lanes. > >> > >> I noticed this too, but it seems that 010 is the correct value for two > >> lane mode. I think this is a typo in the datasheet. > >> > >> > > >> > - MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as > >> > "Power down > >> > PHY HS TX", so I suppose 0 is correct. > >> > > >> > - MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as > >> > "Power down > >> > PHY LP RX module", so I suppose 0 is correct. I however wonder why > >> > there's a > >> > RX PHY, it could be a typo. > >> > > >> > - mipi_en: 1 means MIPI enable, which should be correct. > >> > > >> > - BIT(0) is undocumented. > >> > > >> > OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus > >> > retains > >> > its default value of 0x00, and is controlled when starting and stopping > >> > the > >> > stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the > >> > "sleep mode" state of lane 2, lane 1 and clock lane respectively, and > >> > should > >> > be LP11 in my opinion (that's the PHY stop state). However, setting them > >> > to > >> > 0x00 when starting the stream mean that LP00 is selected as the sleep > >> > state at > >> > stream start, and LP11 when stopping the stream. Maybe "sleep mode" means > >> > LPDT, but I would expect that to be controlled by the idle status bit in > >> > register 0x4800. > >> > > >> > >> I did not need to mess with the accesses to 0x3019 in order to get > >> things working on my system. I'm not sure of this registers actual > >> behavior, but it might affect idling while not streaming (after power > >> on). My pipeline currently only powers the sensor while streaming, so > >> I might be missing some ramifications of this register. > >> > >> > OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of > >> > init mode > >> > data, and BIT(5) is then cleared at stream on time and set at stream off > >> > time. > >> > This means: > >> > > >> > - Clock lane gate enable: Clock lane is free running > >> > - Line sync enable: Do not send line short packets for each line (I > >> > assume > >> > that's LS/LE) > >> > - Lane select: Use lane1 as default data lane. > >> > - Idle status: MIPI bus will be LP11 when no packet to transmit. I would > >> > have > >> > expected the idle status to correspond to LPDT, and thus be LP00 (as > >> > opposed > >> > to the stop state that should be LP11, which I believe is named "sleep > >> > mode" > >> > in the datasheet and controlled in register 0x3019). > >> > > >> > BIT(5) is the clock lane gate enable, so at stream on time the clock is > >> > set to > >> > free running, and at stream off time to "Gate clock lane when no
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi, On 3 May 2018 at 17:16, Maxime Ripard wrote: > Hi, > > On Wed, May 02, 2018 at 11:11:55AM -0700, Sam Bobrowicz wrote: >> > On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote: >> >> FYI, still hard at work on this. Did some more experiments last week >> >> that seemed to corroborate the clock tree in the spreadsheet. It also >> >> seems that the output of the P divider cell, SCLK cell and MIPI Rate >> >> cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) >> >> in order for the sensor to work properly on my platform, and that the >> >> SCLK value must be close to the "rate" variable that you calculate and >> >> pass to set_mipi_pclk. Unfortunately, I've only got the sensor working >> >> well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka >> >> 84:42:336). I'm running experiments now trying to adjust the htot and >> >> vtot values to create different required rates, and also to try to get >> >> faster Mipi rates working. Any information you have on the >> >> requirements of the htot and vtot values with respect to vact and hact >> >> values would likely be helpful. >> >> >> >> I'm also keeping an eye on the scaler clock, which I think may be >> >> affecting certain resolutions, but haven't been able to see it make a >> >> difference yet (see register 0x3824 and 0x460c) >> >> >> >> I plan on pushing a set of patches once I get this figured out, we can >> >> discuss what I should base them on when I get closer to that point. >> >> I'm new to this process :) >> > >> > I'm also interested in getting the ov5640 driver working with MIPI CSI-2. >> > Studying the datasheet and the driver code, I found the stream on sequence >> > to >> > be a bit weird. In particular the configuration of >> > OV5640_REG_IO_MIPI_CTRL00, >> > OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention. >> > >> > OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of >> > init >> > mode data and never touched for MIPI CSI-2 (the register is only touched in >> > ov5640_set_stream_dvp). The value means >> > >> > - mipi_lane_mode: 010 is documented as "debug mode", I would have expected >> > 000 >> > for one lane or 001 for two lanes. >> >> I noticed this too, but it seems that 010 is the correct value for two >> lane mode. I think this is a typo in the datasheet. >> >> > >> > - MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power >> > down >> > PHY HS TX", so I suppose 0 is correct. >> > >> > - MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power >> > down >> > PHY LP RX module", so I suppose 0 is correct. I however wonder why there's >> > a >> > RX PHY, it could be a typo. >> > >> > - mipi_en: 1 means MIPI enable, which should be correct. >> > >> > - BIT(0) is undocumented. >> > >> > OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus >> > retains >> > its default value of 0x00, and is controlled when starting and stopping the >> > stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the >> > "sleep mode" state of lane 2, lane 1 and clock lane respectively, and >> > should >> > be LP11 in my opinion (that's the PHY stop state). However, setting them to >> > 0x00 when starting the stream mean that LP00 is selected as the sleep >> > state at >> > stream start, and LP11 when stopping the stream. Maybe "sleep mode" means >> > LPDT, but I would expect that to be controlled by the idle status bit in >> > register 0x4800. >> > >> >> I did not need to mess with the accesses to 0x3019 in order to get >> things working on my system. I'm not sure of this registers actual >> behavior, but it might affect idling while not streaming (after power >> on). My pipeline currently only powers the sensor while streaming, so >> I might be missing some ramifications of this register. >> >> > OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init >> > mode >> > data, and BIT(5) is then cleared at stream on time and set at stream off >> > time. >> > This means: >> > >> > - Clock lane gate enable: Clock lane is free running >> > - Line sync enable: Do not send line short packets for each line (I assume >> > that's LS/LE) >> > - Lane select: Use lane1 as default data lane. >> > - Idle status: MIPI bus will be LP11 when no packet to transmit. I would >> > have >> > expected the idle status to correspond to LPDT, and thus be LP00 (as >> > opposed >> > to the stop state that should be LP11, which I believe is named "sleep >> > mode" >> > in the datasheet and controlled in register 0x3019). >> > >> > BIT(5) is the clock lane gate enable, so at stream on time the clock is >> > set to >> > free running, and at stream off time to "Gate clock lane when no packet to >> > transmit". Couldn't we always enable clock gating ? >> >> Good question, it might be worth testing. Same as above, I didn't need >> to mess with this reg either. >> >> > Do you have any insight on this ? Have you modified th
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi, > Good news, MIPI is now working on my platform. I had to make several > changes to how the mipi clocking is calculated in order to get things > stable, but I think I got it figured out. Maxime's changes were really > helpful. Great, I also try to make it work with MIPI-CSI2, If you have found the magic formula to configure the registers, I would be pleased to test it on my side. > > I will try to get some patches out today or tomorrow that should get > you up and running. > > Maxime, I'd prefer to create the patches myself for the experience. > I've read all of the submission guidelines and I understand the > general process, but how should I submit them to the mailing list > since they are based to your patches, which are still under review? > Should I send the patch series to the mailing list myself and just > mention this patch series, maybe with the In-Reply-To header? Or > should I just post a link to them here so you can review them and add > them to a new version of your patch series? Yes, I think your patch(es) should be integrated in the Maxime's series. Regards, Loic
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi, On Wed, May 02, 2018 at 11:11:55AM -0700, Sam Bobrowicz wrote: > > On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote: > >> FYI, still hard at work on this. Did some more experiments last week > >> that seemed to corroborate the clock tree in the spreadsheet. It also > >> seems that the output of the P divider cell, SCLK cell and MIPI Rate > >> cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) > >> in order for the sensor to work properly on my platform, and that the > >> SCLK value must be close to the "rate" variable that you calculate and > >> pass to set_mipi_pclk. Unfortunately, I've only got the sensor working > >> well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka > >> 84:42:336). I'm running experiments now trying to adjust the htot and > >> vtot values to create different required rates, and also to try to get > >> faster Mipi rates working. Any information you have on the > >> requirements of the htot and vtot values with respect to vact and hact > >> values would likely be helpful. > >> > >> I'm also keeping an eye on the scaler clock, which I think may be > >> affecting certain resolutions, but haven't been able to see it make a > >> difference yet (see register 0x3824 and 0x460c) > >> > >> I plan on pushing a set of patches once I get this figured out, we can > >> discuss what I should base them on when I get closer to that point. > >> I'm new to this process :) > > > > I'm also interested in getting the ov5640 driver working with MIPI CSI-2. > > Studying the datasheet and the driver code, I found the stream on sequence > > to > > be a bit weird. In particular the configuration of > > OV5640_REG_IO_MIPI_CTRL00, > > OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention. > > > > OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init > > mode data and never touched for MIPI CSI-2 (the register is only touched in > > ov5640_set_stream_dvp). The value means > > > > - mipi_lane_mode: 010 is documented as "debug mode", I would have expected > > 000 > > for one lane or 001 for two lanes. > > I noticed this too, but it seems that 010 is the correct value for two > lane mode. I think this is a typo in the datasheet. > > > > > - MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power > > down > > PHY HS TX", so I suppose 0 is correct. > > > > - MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power > > down > > PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a > > RX PHY, it could be a typo. > > > > - mipi_en: 1 means MIPI enable, which should be correct. > > > > - BIT(0) is undocumented. > > > > OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus > > retains > > its default value of 0x00, and is controlled when starting and stopping the > > stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the > > "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should > > be LP11 in my opinion (that's the PHY stop state). However, setting them to > > 0x00 when starting the stream mean that LP00 is selected as the sleep state > > at > > stream start, and LP11 when stopping the stream. Maybe "sleep mode" means > > LPDT, but I would expect that to be controlled by the idle status bit in > > register 0x4800. > > > > I did not need to mess with the accesses to 0x3019 in order to get > things working on my system. I'm not sure of this registers actual > behavior, but it might affect idling while not streaming (after power > on). My pipeline currently only powers the sensor while streaming, so > I might be missing some ramifications of this register. > > > OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init > > mode > > data, and BIT(5) is then cleared at stream on time and set at stream off > > time. > > This means: > > > > - Clock lane gate enable: Clock lane is free running > > - Line sync enable: Do not send line short packets for each line (I assume > > that's LS/LE) > > - Lane select: Use lane1 as default data lane. > > - Idle status: MIPI bus will be LP11 when no packet to transmit. I would > > have > > expected the idle status to correspond to LPDT, and thus be LP00 (as opposed > > to the stop state that should be LP11, which I believe is named "sleep mode" > > in the datasheet and controlled in register 0x3019). > > > > BIT(5) is the clock lane gate enable, so at stream on time the clock is set > > to > > free running, and at stream off time to "Gate clock lane when no packet to > > transmit". Couldn't we always enable clock gating ? > > Good question, it might be worth testing. Same as above, I didn't need > to mess with this reg either. > > > Do you have any insight on this ? Have you modified the MIPI CSI-2 > > configuration to get the CSI-2 output working ? > > Good news, MIPI is now working on my platform. I had to make several > changes to how the mipi clocking is ca
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
On Fri, Apr 27, 2018 at 2:27 AM, Laurent Pinchart wrote: > Hi Sam, > > On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote: >> FYI, still hard at work on this. Did some more experiments last week >> that seemed to corroborate the clock tree in the spreadsheet. It also >> seems that the output of the P divider cell, SCLK cell and MIPI Rate >> cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) >> in order for the sensor to work properly on my platform, and that the >> SCLK value must be close to the "rate" variable that you calculate and >> pass to set_mipi_pclk. Unfortunately, I've only got the sensor working >> well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka >> 84:42:336). I'm running experiments now trying to adjust the htot and >> vtot values to create different required rates, and also to try to get >> faster Mipi rates working. Any information you have on the >> requirements of the htot and vtot values with respect to vact and hact >> values would likely be helpful. >> >> I'm also keeping an eye on the scaler clock, which I think may be >> affecting certain resolutions, but haven't been able to see it make a >> difference yet (see register 0x3824 and 0x460c) >> >> I plan on pushing a set of patches once I get this figured out, we can >> discuss what I should base them on when I get closer to that point. >> I'm new to this process :) > > I'm also interested in getting the ov5640 driver working with MIPI CSI-2. > Studying the datasheet and the driver code, I found the stream on sequence to > be a bit weird. In particular the configuration of OV5640_REG_IO_MIPI_CTRL00, > OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention. > > OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init > mode data and never touched for MIPI CSI-2 (the register is only touched in > ov5640_set_stream_dvp). The value means > > - mipi_lane_mode: 010 is documented as "debug mode", I would have expected 000 > for one lane or 001 for two lanes. I noticed this too, but it seems that 010 is the correct value for two lane mode. I think this is a typo in the datasheet. > > - MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power down > PHY HS TX", so I suppose 0 is correct. > > - MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power down > PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a > RX PHY, it could be a typo. > > - mipi_en: 1 means MIPI enable, which should be correct. > > - BIT(0) is undocumented. > > OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus retains > its default value of 0x00, and is controlled when starting and stopping the > stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the > "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should > be LP11 in my opinion (that's the PHY stop state). However, setting them to > 0x00 when starting the stream mean that LP00 is selected as the sleep state at > stream start, and LP11 when stopping the stream. Maybe "sleep mode" means > LPDT, but I would expect that to be controlled by the idle status bit in > register 0x4800. > I did not need to mess with the accesses to 0x3019 in order to get things working on my system. I'm not sure of this registers actual behavior, but it might affect idling while not streaming (after power on). My pipeline currently only powers the sensor while streaming, so I might be missing some ramifications of this register. > OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init mode > data, and BIT(5) is then cleared at stream on time and set at stream off time. > This means: > > - Clock lane gate enable: Clock lane is free running > - Line sync enable: Do not send line short packets for each line (I assume > that's LS/LE) > - Lane select: Use lane1 as default data lane. > - Idle status: MIPI bus will be LP11 when no packet to transmit. I would have > expected the idle status to correspond to LPDT, and thus be LP00 (as opposed > to the stop state that should be LP11, which I believe is named "sleep mode" > in the datasheet and controlled in register 0x3019). > > BIT(5) is the clock lane gate enable, so at stream on time the clock is set to > free running, and at stream off time to "Gate clock lane when no packet to > transmit". Couldn't we always enable clock gating ? Good question, it might be worth testing. Same as above, I didn't need to mess with this reg either. > Do you have any insight on this ? Have you modified the MIPI CSI-2 > configuration to get the CSI-2 output working ? > > -- > Regards, > > Laurent Pinchart > > > Good news, MIPI is now working on my platform. I had to make several changes to how the mipi clocking is calculated in order to get things stable, but I think I got it figured out. Maxime's changes were really helpful. I will try to get some patches out today or tomorrow that should get you up and
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi Sam, On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote: > FYI, still hard at work on this. Did some more experiments last week > that seemed to corroborate the clock tree in the spreadsheet. It also > seems that the output of the P divider cell, SCLK cell and MIPI Rate > cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) > in order for the sensor to work properly on my platform, and that the > SCLK value must be close to the "rate" variable that you calculate and > pass to set_mipi_pclk. Unfortunately, I've only got the sensor working > well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka > 84:42:336). I'm running experiments now trying to adjust the htot and > vtot values to create different required rates, and also to try to get > faster Mipi rates working. Any information you have on the > requirements of the htot and vtot values with respect to vact and hact > values would likely be helpful. > > I'm also keeping an eye on the scaler clock, which I think may be > affecting certain resolutions, but haven't been able to see it make a > difference yet (see register 0x3824 and 0x460c) > > I plan on pushing a set of patches once I get this figured out, we can > discuss what I should base them on when I get closer to that point. > I'm new to this process :) I'm also interested in getting the ov5640 driver working with MIPI CSI-2. Studying the datasheet and the driver code, I found the stream on sequence to be a bit weird. In particular the configuration of OV5640_REG_IO_MIPI_CTRL00, OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention. OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init mode data and never touched for MIPI CSI-2 (the register is only touched in ov5640_set_stream_dvp). The value means - mipi_lane_mode: 010 is documented as "debug mode", I would have expected 000 for one lane or 001 for two lanes. - MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY HS TX", so I suppose 0 is correct. - MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power down PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a RX PHY, it could be a typo. - mipi_en: 1 means MIPI enable, which should be correct. - BIT(0) is undocumented. OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus retains its default value of 0x00, and is controlled when starting and stopping the stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should be LP11 in my opinion (that's the PHY stop state). However, setting them to 0x00 when starting the stream mean that LP00 is selected as the sleep state at stream start, and LP11 when stopping the stream. Maybe "sleep mode" means LPDT, but I would expect that to be controlled by the idle status bit in register 0x4800. OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init mode data, and BIT(5) is then cleared at stream on time and set at stream off time. This means: - Clock lane gate enable: Clock lane is free running - Line sync enable: Do not send line short packets for each line (I assume that's LS/LE) - Lane select: Use lane1 as default data lane. - Idle status: MIPI bus will be LP11 when no packet to transmit. I would have expected the idle status to correspond to LPDT, and thus be LP00 (as opposed to the stop state that should be LP11, which I believe is named "sleep mode" in the datasheet and controlled in register 0x3019). BIT(5) is the clock lane gate enable, so at stream on time the clock is set to free running, and at stream off time to "Gate clock lane when no packet to transmit". Couldn't we always enable clock gating ? Do you have any insight on this ? Have you modified the MIPI CSI-2 configuration to get the CSI-2 output working ? -- Regards, Laurent Pinchart
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi Samuel, On Tue, Apr 24, 2018 at 03:11:19PM -0700, Sam Bobrowicz wrote: > FYI, still hard at work on this. Did some more experiments last week > that seemed to corroborate the clock tree in the spreadsheet. Ok, good, I'll send an updated version next week taking this into account then. Thanks! > It also seems that the output of the P divider cell, SCLK cell and > MIPI Rate cell in the spreadsheet must have a ratio of 2x:1x:8x > (respectively) in order for the sensor to work properly on my > platform, and that the SCLK value must be close to the "rate" > variable that you calculate and pass to set_mipi_pclk. It might be quite simple to support actually. Most of the other dividers were hardcoded in the driver, so maybe it's the case for those as well. I'll check and see how it goes. > Unfortunately, I've only got the sensor working well for 1080p@15Hz > and 720p@30Hz, both with a SCLK of 42MHz (aka 84:42:336). I'm > running experiments now trying to adjust the htot and vtot values to > create different required rates, and also to try to get faster Mipi > rates working. Any information you have on the requirements of the > htot and vtot values with respect to vact and hact values would > likely be helpful. Unfortunately, I don't have an answer to that one. > I'm also keeping an eye on the scaler clock, which I think may be > affecting certain resolutions, but haven't been able to see it make a > difference yet (see register 0x3824 and 0x460c) > > I plan on pushing a set of patches once I get this figured out, we can > discuss what I should base them on when I get closer to that point. > I'm new to this process :) I was planning on sending a new version based on your feedback for the MIPI-CSI2 formula, most likely next week. I guess you could test them and see how it goes. Or send patches on top of this version if you prefer :) You have more documentation on how to do that here: https://www.kernel.org/doc/Documentation/process/submitting-patches.rst Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
FYI, still hard at work on this. Did some more experiments last week that seemed to corroborate the clock tree in the spreadsheet. It also seems that the output of the P divider cell, SCLK cell and MIPI Rate cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively) in order for the sensor to work properly on my platform, and that the SCLK value must be close to the "rate" variable that you calculate and pass to set_mipi_pclk. Unfortunately, I've only got the sensor working well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka 84:42:336). I'm running experiments now trying to adjust the htot and vtot values to create different required rates, and also to try to get faster Mipi rates working. Any information you have on the requirements of the htot and vtot values with respect to vact and hact values would likely be helpful. I'm also keeping an eye on the scaler clock, which I think may be affecting certain resolutions, but haven't been able to see it make a difference yet (see register 0x3824 and 0x460c) I plan on pushing a set of patches once I get this figured out, we can discuss what I should base them on when I get closer to that point. I'm new to this process :) --Sam --- Sam Bobrowicz Elite Embedded Consulting LLC elite-embedded.com On Thu, Apr 19, 2018 at 5:32 AM, Maxime Ripard wrote: > Hi Samuel, > > On Wed, Apr 18, 2018 at 04:39:06PM -0700, Samuel Bobrowicz wrote: >> I applied your patches, and they are a big improvement for what I am >> trying to do, but things still aren't working right on my platform. >> >> How confident are you that the MIPI mode will work with this version >> of the driver? > > Not too confident. Like I said, I did all my tests on a parallel > camera with a scope, so I'm pretty confident for the parallel bus. But > I haven't been able to test the MIPI-CSI side of things and tried to > deduce it from the datasheet. > > tl; dr: I might very well be wrong. > >> I am having issues that I believe are due to incorrect clock >> generation. Our engineers did some reverse engineering of the clock >> tree themselves, and came up with a slightly different model. I've >> captured their model in a spreadsheet here: >> https://tinyurl.com/pll-calc . Just modify the register and xclk >> values to see the clocks change. Do your tests disagree with this >> potential model? > > At least on the parallel side, it looks fairly similar, so I guess we > can come to an agreement :) > > There's just the SCLK2x divider that is no longer in the path to PCLK > but has been replaced with BIT Divider that has the same value, so it > should work as well. > >> I'm not sure which model is more correct, but my tests suggest the >> high speed MIPI clock is generated directly off the PLL. This means >> the PLL multiplier you are generating in your algorithm is not high >> enough to satisfy the bandwidth. If this is the case, MIPI mode will >> require a different set of parameters that enable some of the >> downstream dividers, so that the PLL multiplier can be higher while >> the PCLK value still matches the needed rate calculated from the >> resolution. >> >> Any thoughts on this before I dive in and start tweaking the algorithm >> in mipi mode? > > Like I said, I did that analysis by plugging the camera to a scope and > look at the PCLK generated for various combinations. Your analysis > seems not too far off for the setup I've tested, so I guess this makes > sense. And let me know how it works for MIPI-CSI2 so that I can update > the patches :) > > Maxime > > -- > Maxime Ripard, Bootlin (formerly Free Electrons) > Embedded Linux and Kernel engineering > https://bootlin.com
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi Samuel, On Wed, Apr 18, 2018 at 04:39:06PM -0700, Samuel Bobrowicz wrote: > I applied your patches, and they are a big improvement for what I am > trying to do, but things still aren't working right on my platform. > > How confident are you that the MIPI mode will work with this version > of the driver? Not too confident. Like I said, I did all my tests on a parallel camera with a scope, so I'm pretty confident for the parallel bus. But I haven't been able to test the MIPI-CSI side of things and tried to deduce it from the datasheet. tl; dr: I might very well be wrong. > I am having issues that I believe are due to incorrect clock > generation. Our engineers did some reverse engineering of the clock > tree themselves, and came up with a slightly different model. I've > captured their model in a spreadsheet here: > https://tinyurl.com/pll-calc . Just modify the register and xclk > values to see the clocks change. Do your tests disagree with this > potential model? At least on the parallel side, it looks fairly similar, so I guess we can come to an agreement :) There's just the SCLK2x divider that is no longer in the path to PCLK but has been replaced with BIT Divider that has the same value, so it should work as well. > I'm not sure which model is more correct, but my tests suggest the > high speed MIPI clock is generated directly off the PLL. This means > the PLL multiplier you are generating in your algorithm is not high > enough to satisfy the bandwidth. If this is the case, MIPI mode will > require a different set of parameters that enable some of the > downstream dividers, so that the PLL multiplier can be higher while > the PCLK value still matches the needed rate calculated from the > resolution. > > Any thoughts on this before I dive in and start tweaking the algorithm > in mipi mode? Like I said, I did that analysis by plugging the camera to a scope and look at the PCLK generated for various combinations. Your analysis seems not too far off for the setup I've tested, so I guess this makes sense. And let me know how it works for MIPI-CSI2 so that I can update the patches :) Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi Maxime, I applied your patches, and they are a big improvement for what I am trying to do, but things still aren't working right on my platform. How confident are you that the MIPI mode will work with this version of the driver? I am having issues that I believe are due to incorrect clock generation. Our engineers did some reverse engineering of the clock tree themselves, and came up with a slightly different model. I've captured their model in a spreadsheet here: https://tinyurl.com/pll-calc . Just modify the register and xclk values to see the clocks change. Do your tests disagree with this potential model? I'm not sure which model is more correct, but my tests suggest the high speed MIPI clock is generated directly off the PLL. This means the PLL multiplier you are generating in your algorithm is not high enough to satisfy the bandwidth. If this is the case, MIPI mode will require a different set of parameters that enable some of the downstream dividers, so that the PLL multiplier can be higher while the PCLK value still matches the needed rate calculated from the resolution. Any thoughts on this before I dive in and start tweaking the algorithm in mipi mode? Sam --- Sam Bobrowicz Elite Embedded Consulting LLC elite-embedded.com On Tue, Apr 17, 2018 at 9:01 AM, Maxime Ripard wrote: > On Mon, Apr 16, 2018 at 04:22:39PM -0700, Samuel Bobrowicz wrote: >> I've been digging around the ov5640.c code for a few weeks now, these >> look like some solid improvements. I'll give them a shot and let you >> know how they work. > > Great, thanks! > >> On that note, I'm bringing up a module that uses dual lane MIPI with a >> 12MHz fixed oscillator for xclk (Digilent's Pcam 5c). The mainline >> version of the driver seems to only support xclk of 22MHz (or maybe >> 24MHz), despite allowing xclk values from 6-24MHz. Will any of these >> patches add support for a 12MHz xclk while in MIPI mode? > > My setup has a 24MHz crystal, and work with a parallel bus so I > haven't been able to test yours. However, yeah, I guess my patches > will improve your situation a lot. > > Maxime > > -- > Maxime Ripard, Bootlin (formerly Free Electrons) > Embedded Linux and Kernel engineering > https://bootlin.com
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
On Mon, Apr 16, 2018 at 04:22:39PM -0700, Samuel Bobrowicz wrote: > I've been digging around the ov5640.c code for a few weeks now, these > look like some solid improvements. I'll give them a shot and let you > know how they work. Great, thanks! > On that note, I'm bringing up a module that uses dual lane MIPI with a > 12MHz fixed oscillator for xclk (Digilent's Pcam 5c). The mainline > version of the driver seems to only support xclk of 22MHz (or maybe > 24MHz), despite allowing xclk values from 6-24MHz. Will any of these > patches add support for a 12MHz xclk while in MIPI mode? My setup has a 24MHz crystal, and work with a parallel bus so I haven't been able to test yours. However, yeah, I guess my patches will improve your situation a lot. Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
I've been digging around the ov5640.c code for a few weeks now, these look like some solid improvements. I'll give them a shot and let you know how they work. On that note, I'm bringing up a module that uses dual lane MIPI with a 12MHz fixed oscillator for xclk (Digilent's Pcam 5c). The mainline version of the driver seems to only support xclk of 22MHz (or maybe 24MHz), despite allowing xclk values from 6-24MHz. Will any of these patches add support for a 12MHz xclk while in MIPI mode? Sam --- Sam Bobrowicz Elite Embedded Consulting LLC elite-embedded.com On Mon, Apr 16, 2018 at 5:36 AM, Maxime Ripard wrote: > Hi, > > Here is a "small" series that mostly cleans up the ov5640 driver code, > slowly getting rid of the big data array for more understandable code > (hopefully). > > The biggest addition would be the clock rate computation at runtime, > instead of relying on those arrays to setup the clock tree > properly. As a side effect, it fixes the framerate that was off by > around 10% on the smaller resolutions, and we now support 60fps. > > This also introduces a bunch of new features. > > Let me know what you think, > Maxime > > Changes from v1: > - Integrated Hugues' suggestions to fix v4l2-compliance > - Fixed the bus width with JPEG > - Dropped the clock rate calculation loops for something simpler as > suggested by Sakari > - Cache the exposure value instead of using the control value > - Rebased on top of 4.17 > > Maxime Ripard (10): > media: ov5640: Don't force the auto exposure state at start time > media: ov5640: Init properly the SCLK dividers > media: ov5640: Change horizontal and vertical resolutions name > media: ov5640: Add horizontal and vertical totals > media: ov5640: Program the visible resolution > media: ov5640: Adjust the clock based on the expected rate > media: ov5640: Compute the clock rate at runtime > media: ov5640: Enhance FPS handling > media: ov5640: Add 60 fps support > media: ov5640: Remove duplicate auto-exposure setup > > Mylène Josserand (2): > media: ov5640: Add auto-focus feature > media: ov5640: Add light frequency control > > drivers/media/i2c/ov5640.c | 752 + > 1 file changed, 422 insertions(+), 330 deletions(-) > > -- > 2.17.0 >
[PATCH v2 00/12] media: ov5640: Misc cleanup and improvements
Hi, Here is a "small" series that mostly cleans up the ov5640 driver code, slowly getting rid of the big data array for more understandable code (hopefully). The biggest addition would be the clock rate computation at runtime, instead of relying on those arrays to setup the clock tree properly. As a side effect, it fixes the framerate that was off by around 10% on the smaller resolutions, and we now support 60fps. This also introduces a bunch of new features. Let me know what you think, Maxime Changes from v1: - Integrated Hugues' suggestions to fix v4l2-compliance - Fixed the bus width with JPEG - Dropped the clock rate calculation loops for something simpler as suggested by Sakari - Cache the exposure value instead of using the control value - Rebased on top of 4.17 Maxime Ripard (10): media: ov5640: Don't force the auto exposure state at start time media: ov5640: Init properly the SCLK dividers media: ov5640: Change horizontal and vertical resolutions name media: ov5640: Add horizontal and vertical totals media: ov5640: Program the visible resolution media: ov5640: Adjust the clock based on the expected rate media: ov5640: Compute the clock rate at runtime media: ov5640: Enhance FPS handling media: ov5640: Add 60 fps support media: ov5640: Remove duplicate auto-exposure setup Mylène Josserand (2): media: ov5640: Add auto-focus feature media: ov5640: Add light frequency control drivers/media/i2c/ov5640.c | 752 + 1 file changed, 422 insertions(+), 330 deletions(-) -- 2.17.0
