| Message ID | 1324412889-17961-13-git-send-email-sakari.ailus@maxwell.research.nokia.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1Rd6If-00034t-Gl; Tue, 20 Dec 2011 21:28:38 +0100 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.75/mailfrontend-2) with esmtp id 1Rd6Ie-0004Yp-IG; Tue, 20 Dec 2011 21:28:37 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752969Ab1LTU2e (ORCPT <rfc822;patchwork@linuxtv.org> + 3 others); Tue, 20 Dec 2011 15:28:34 -0500 Received: from smtp.nokia.com ([147.243.128.26]:17045 "EHLO mgw-da02.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751944Ab1LTU2S (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 20 Dec 2011 15:28:18 -0500 Received: from maxwell.research.nokia.com (maxwell.research.nokia.com [172.21.50.162]) by mgw-da02.nokia.com (Switch-3.4.4/Switch-3.4.4) with ESMTP id pBKKSDKs024117 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 20 Dec 2011 22:28:15 +0200 Received: from lanttu (unknown [192.168.239.74]) by maxwell.research.nokia.com (Postfix) with ESMTPS id A342838663A; Tue, 20 Dec 2011 22:28:13 +0200 (EET) Received: from sakke by lanttu with local (Exim 4.72) (envelope-from <sakari.ailus@maxwell.research.nokia.com>) id 1Rd6IJ-0004jP-DD; Tue, 20 Dec 2011 22:28:15 +0200 From: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> To: linux-media@vger.kernel.org Cc: laurent.pinchart@ideasonboard.com, dacohen@gmail.com, snjw23@gmail.com Subject: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data Date: Tue, 20 Dec 2011 22:28:05 +0200 Message-Id: <1324412889-17961-13-git-send-email-sakari.ailus@maxwell.research.nokia.com> X-Mailer: git-send-email 1.7.2.5 In-Reply-To: <4EF0EFC9.6080501@maxwell.research.nokia.com> References: <4EF0EFC9.6080501@maxwell.research.nokia.com> X-Nokia-AV: Clean Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 5.6.1.2065439, Antispam-Engine: 2.7.2.376379, Antispam-Data: 2011.12.20.201815 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, __ANY_URI 0, __CP_MEDIA_BODY 0, __CP_URI_IN_BODY 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __LINES_OF_YELLING 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS ' X-LSpam-Score: -1.9 (-) X-LSpam-Report: No, score=-1.9 required=5.0 tests=BAYES_00=-1.9 autolearn=ham |
Commit Message
Sakari Ailus
Dec. 20, 2011, 8:28 p.m. UTC
From: Sakari Ailus <sakari.ailus@iki.fi> Configure CSI-2 phy based on platform data in the ISP driver rather than in platform code. Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> --- drivers/media/video/omap3isp/isp.c | 38 ++++++++++++-- drivers/media/video/omap3isp/isp.h | 3 - drivers/media/video/omap3isp/ispcsiphy.c | 83 ++++++++++++++++++++++++++---- drivers/media/video/omap3isp/ispcsiphy.h | 4 ++ 4 files changed, 111 insertions(+), 17 deletions(-)
Comments
Hi Sakari, Thanks for the patch. On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote: > From: Sakari Ailus <sakari.ailus@iki.fi> > > Configure CSI-2 phy based on platform data in the ISP driver rather than in > platform code. > > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> > --- > drivers/media/video/omap3isp/isp.c | 38 ++++++++++++-- > drivers/media/video/omap3isp/isp.h | 3 - > drivers/media/video/omap3isp/ispcsiphy.c | 83 +++++++++++++++++++++++---- > drivers/media/video/omap3isp/ispcsiphy.h | 4 ++ > 4 files changed, 111 insertions(+), 17 deletions(-) > > diff --git a/drivers/media/video/omap3isp/isp.c > b/drivers/media/video/omap3isp/isp.c index b818cac..6020fd7 100644 > --- a/drivers/media/video/omap3isp/isp.c > +++ b/drivers/media/video/omap3isp/isp.c > @@ -737,7 +737,7 @@ static int isp_pipeline_enable(struct isp_pipeline > *pipe, struct isp_device *isp = pipe->output->isp; > struct media_entity *entity; > struct media_pad *pad; > - struct v4l2_subdev *subdev; > + struct v4l2_subdev *subdev = NULL, *prev_subdev; > unsigned long flags; > int ret; > > @@ -759,11 +759,41 @@ static int isp_pipeline_enable(struct isp_pipeline > *pipe, break; > > entity = pad->entity; > + prev_subdev = subdev; > subdev = media_entity_to_v4l2_subdev(entity); > > - ret = v4l2_subdev_call(subdev, video, s_stream, mode); > - if (ret < 0 && ret != -ENOIOCTLCMD) > - return ret; > + /* Configure CSI-2 receiver based on sensor format. */ > + if (prev_subdev == &isp->isp_csi2a.subdev > + || prev_subdev == &isp->isp_csi2c.subdev) { > + struct v4l2_subdev_format fmt; > + > + fmt.pad = pad->index; > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + ret = v4l2_subdev_call(subdev, pad, get_fmt, > + NULL, &fmt); > + if (ret < 0) > + return -EPIPE; > + > + ret = omap3isp_csiphy_config( > + isp, prev_subdev, subdev, > + &fmt.format); > + if (ret < 0) > + return -EPIPE; > + > + /* Start CSI-2 after configuration. */ > + ret = v4l2_subdev_call(prev_subdev, video, > + s_stream, mode); > + if (ret < 0 && ret != -ENOIOCTLCMD) > + return ret; > + } > + > + /* Start any other subdev except the CSI-2 receivers. */ > + if (subdev != &isp->isp_csi2a.subdev > + && subdev != &isp->isp_csi2c.subdev) { > + ret = v4l2_subdev_call(subdev, video, s_stream, mode); > + if (ret < 0 && ret != -ENOIOCTLCMD) > + return ret; > + } What about moving this to the CSI2 s_stream subdev operation ? > > if (subdev == &isp->isp_ccdc.subdev) { > v4l2_subdev_call(&isp->isp_aewb.subdev, video, > diff --git a/drivers/media/video/omap3isp/isp.h > b/drivers/media/video/omap3isp/isp.h index 705946e..c5935ae 100644 > --- a/drivers/media/video/omap3isp/isp.h > +++ b/drivers/media/video/omap3isp/isp.h > @@ -126,9 +126,6 @@ struct isp_reg { > > struct isp_platform_callback { > u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel); > - int (*csiphy_config)(struct isp_csiphy *phy, > - struct isp_csiphy_dphy_cfg *dphy, > - struct isp_csiphy_lanes_cfg *lanes); > void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk); > }; > > diff --git a/drivers/media/video/omap3isp/ispcsiphy.c > b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 100644 > --- a/drivers/media/video/omap3isp/ispcsiphy.c > +++ b/drivers/media/video/omap3isp/ispcsiphy.c > @@ -28,6 +28,8 @@ > #include <linux/device.h> > #include <linux/regulator/consumer.h> > > +#include "../../../../arch/arm/mach-omap2/control.h" > + #include <mach/control.h> (untested) ? > #include "isp.h" > #include "ispreg.h" > #include "ispcsiphy.h" > @@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy > *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1); > } > > -static int csiphy_config(struct isp_csiphy *phy, > - struct isp_csiphy_dphy_cfg *dphy, > - struct isp_csiphy_lanes_cfg *lanes) > +/* > + * TCLK values are OK at their reset values > + */ > +#define TCLK_TERM 0 > +#define TCLK_MISS 1 > +#define TCLK_SETTLE 14 > + > +int omap3isp_csiphy_config(struct isp_device *isp, > + struct v4l2_subdev *csi2_subdev, > + struct v4l2_subdev *sensor, > + struct v4l2_mbus_framefmt *sensor_fmt) > { > + struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv; > + struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev); > + struct isp_csiphy_dphy_cfg csi2phy; > + int csi2_ddrclk_khz; > + struct isp_csiphy_lanes_cfg *lanes; > unsigned int used_lanes = 0; > unsigned int i; > + u32 cam_phy_ctrl; > + > + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1 > + || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2) > + lanes = subdevs->bus.ccp2.lanecfg; > + else > + lanes = subdevs->bus.csi2.lanecfg; Shouldn't lane configuration be retrieved from the sensor instead ? Sensors could use different lane configuration depending on the mode. This could also be implemented later when needed, but I don't think it would be too difficult to get it right now. > + > + if (!lanes) { > + dev_err(isp->dev, "no lane configuration\n"); > + return -EINVAL; > + } > + > + cam_phy_ctrl = omap_readl( > + OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL); > + /* > + * SCM.CONTROL_CAMERA_PHY_CTRL > + * - bit[4] : CSIPHY1 data sent to CSIB > + * - bit [3:2] : CSIPHY1 config: 00 d-phy, 01/10 ccp2 > + * - bit [1:0] : CSIPHY2 config: 00 d-phy, 01/10 ccp2 > + */ > + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1) > + cam_phy_ctrl |= 1 << 2; > + else if (subdevs->interface == ISP_INTERFACE_CSI2C_PHY1) > + cam_phy_ctrl &= 1 << 2; > + > + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY2) > + cam_phy_ctrl |= 1; > + else if (subdevs->interface == ISP_INTERFACE_CSI2A_PHY2) > + cam_phy_ctrl &= 1; > + > + /* FIXME: Do 34xx / 35xx require something here? */ > + if (cpu_is_omap3630()) > + omap_writel(cam_phy_ctrl, > + OMAP343X_CTRL_BASE > + + OMAP3630_CONTROL_CAMERA_PHY_CTRL); You use cam_phy_ctrl inside the if statement only, you can move its computation there. > + > + csi2_ddrclk_khz = sensor_fmt->pixelrate > + / (2 * csi2->phy->num_data_lanes) > + * omap3isp_video_format_info(sensor_fmt->code)->bpp; > + > + /* > + * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1. > + * THS_SETTLE: Programmed value = ceil(90 ns/DDRClk period) + 3. > + */ > + csi2phy.ths_term = DIV_ROUND_UP(25 * csi2_ddrclk_khz, 2000000) - 1; > + csi2phy.ths_settle = DIV_ROUND_UP(90 * csi2_ddrclk_khz, 1000000) + 3; > + csi2phy.tclk_term = TCLK_TERM; > + csi2phy.tclk_miss = TCLK_MISS; > + csi2phy.tclk_settle = TCLK_SETTLE; > > /* Clock and data lanes verification */ > - for (i = 0; i < phy->num_data_lanes; i++) { > + for (i = 0; i < csi2->phy->num_data_lanes; i++) { > if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3) > return -EINVAL; > > @@ -162,10 +227,10 @@ static int csiphy_config(struct isp_csiphy *phy, > if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos)) > return -EINVAL; > > - mutex_lock(&phy->mutex); > - phy->dphy = *dphy; > - phy->lanes = *lanes; > - mutex_unlock(&phy->mutex); > + mutex_lock(&csi2->phy->mutex); > + csi2->phy->dphy = csi2phy; > + csi2->phy->lanes = *lanes; > + mutex_unlock(&csi2->phy->mutex); > > return 0; > } > @@ -225,8 +290,6 @@ int omap3isp_csiphy_init(struct isp_device *isp) > struct isp_csiphy *phy1 = &isp->isp_csiphy1; > struct isp_csiphy *phy2 = &isp->isp_csiphy2; > > - isp->platform_cb.csiphy_config = csiphy_config; > - > phy2->isp = isp; > phy2->csi2 = &isp->isp_csi2a; > phy2->num_data_lanes = ISP_CSIPHY2_NUM_DATA_LANES; > diff --git a/drivers/media/video/omap3isp/ispcsiphy.h > b/drivers/media/video/omap3isp/ispcsiphy.h index e93a661..9f93222 100644 > --- a/drivers/media/video/omap3isp/ispcsiphy.h > +++ b/drivers/media/video/omap3isp/ispcsiphy.h > @@ -56,6 +56,10 @@ struct isp_csiphy { > struct isp_csiphy_dphy_cfg dphy; > }; > > +int omap3isp_csiphy_config(struct isp_device *isp, > + struct v4l2_subdev *csi2_subdev, > + struct v4l2_subdev *sensor, > + struct v4l2_mbus_framefmt *fmt); > int omap3isp_csiphy_acquire(struct isp_csiphy *phy); > void omap3isp_csiphy_release(struct isp_csiphy *phy); > int omap3isp_csiphy_init(struct isp_device *isp);
Hi Laurent, Thanks for the review!!! Laurent Pinchart wrote: > On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote: >> From: Sakari Ailus <sakari.ailus@iki.fi> >> >> Configure CSI-2 phy based on platform data in the ISP driver rather than in >> platform code. >> >> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> >> --- >> drivers/media/video/omap3isp/isp.c | 38 ++++++++++++-- >> drivers/media/video/omap3isp/isp.h | 3 - >> drivers/media/video/omap3isp/ispcsiphy.c | 83 +++++++++++++++++++++++---- >> drivers/media/video/omap3isp/ispcsiphy.h | 4 ++ >> 4 files changed, 111 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/media/video/omap3isp/isp.c >> b/drivers/media/video/omap3isp/isp.c index b818cac..6020fd7 100644 >> --- a/drivers/media/video/omap3isp/isp.c >> +++ b/drivers/media/video/omap3isp/isp.c >> @@ -737,7 +737,7 @@ static int isp_pipeline_enable(struct isp_pipeline >> *pipe, struct isp_device *isp = pipe->output->isp; >> struct media_entity *entity; >> struct media_pad *pad; >> - struct v4l2_subdev *subdev; >> + struct v4l2_subdev *subdev = NULL, *prev_subdev; >> unsigned long flags; >> int ret; >> >> @@ -759,11 +759,41 @@ static int isp_pipeline_enable(struct isp_pipeline >> *pipe, break; >> >> entity = pad->entity; >> + prev_subdev = subdev; >> subdev = media_entity_to_v4l2_subdev(entity); >> >> - ret = v4l2_subdev_call(subdev, video, s_stream, mode); >> - if (ret < 0 && ret != -ENOIOCTLCMD) >> - return ret; >> + /* Configure CSI-2 receiver based on sensor format. */ >> + if (prev_subdev == &isp->isp_csi2a.subdev >> + || prev_subdev == &isp->isp_csi2c.subdev) { >> + struct v4l2_subdev_format fmt; >> + >> + fmt.pad = pad->index; >> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; >> + ret = v4l2_subdev_call(subdev, pad, get_fmt, >> + NULL, &fmt); >> + if (ret < 0) >> + return -EPIPE; >> + >> + ret = omap3isp_csiphy_config( >> + isp, prev_subdev, subdev, >> + &fmt.format); >> + if (ret < 0) >> + return -EPIPE; >> + >> + /* Start CSI-2 after configuration. */ >> + ret = v4l2_subdev_call(prev_subdev, video, >> + s_stream, mode); >> + if (ret < 0 && ret != -ENOIOCTLCMD) >> + return ret; >> + } >> + >> + /* Start any other subdev except the CSI-2 receivers. */ >> + if (subdev != &isp->isp_csi2a.subdev >> + && subdev != &isp->isp_csi2c.subdev) { >> + ret = v4l2_subdev_call(subdev, video, s_stream, mode); >> + if (ret < 0 && ret != -ENOIOCTLCMD) >> + return ret; >> + } > > What about moving this to the CSI2 s_stream subdev operation ? Done. >> >> if (subdev == &isp->isp_ccdc.subdev) { >> v4l2_subdev_call(&isp->isp_aewb.subdev, video, >> diff --git a/drivers/media/video/omap3isp/isp.h >> b/drivers/media/video/omap3isp/isp.h index 705946e..c5935ae 100644 >> --- a/drivers/media/video/omap3isp/isp.h >> +++ b/drivers/media/video/omap3isp/isp.h >> @@ -126,9 +126,6 @@ struct isp_reg { >> >> struct isp_platform_callback { >> u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel); >> - int (*csiphy_config)(struct isp_csiphy *phy, >> - struct isp_csiphy_dphy_cfg *dphy, >> - struct isp_csiphy_lanes_cfg *lanes); >> void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk); >> }; >> >> diff --git a/drivers/media/video/omap3isp/ispcsiphy.c >> b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 100644 >> --- a/drivers/media/video/omap3isp/ispcsiphy.c >> +++ b/drivers/media/video/omap3isp/ispcsiphy.c >> @@ -28,6 +28,8 @@ >> #include <linux/device.h> >> #include <linux/regulator/consumer.h> >> >> +#include "../../../../arch/arm/mach-omap2/control.h" >> + > > #include <mach/control.h> > > (untested) ? I'm afraid it won't work. The above directory isn't in any include path and likely shouldn't be. That file is included locally elsewhere, I believe. I wonder what would be the right way to access that register definition I need from the file: OMAP343X_CTRL_BASE OMAP3630_CONTROL_CAMERA_PHY_CTRL >> #include "isp.h" >> #include "ispreg.h" >> #include "ispcsiphy.h" >> @@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy >> *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1); >> } >> >> -static int csiphy_config(struct isp_csiphy *phy, >> - struct isp_csiphy_dphy_cfg *dphy, >> - struct isp_csiphy_lanes_cfg *lanes) >> +/* >> + * TCLK values are OK at their reset values >> + */ >> +#define TCLK_TERM 0 >> +#define TCLK_MISS 1 >> +#define TCLK_SETTLE 14 >> + >> +int omap3isp_csiphy_config(struct isp_device *isp, >> + struct v4l2_subdev *csi2_subdev, >> + struct v4l2_subdev *sensor, >> + struct v4l2_mbus_framefmt *sensor_fmt) >> { >> + struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv; >> + struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev); >> + struct isp_csiphy_dphy_cfg csi2phy; >> + int csi2_ddrclk_khz; >> + struct isp_csiphy_lanes_cfg *lanes; >> unsigned int used_lanes = 0; >> unsigned int i; >> + u32 cam_phy_ctrl; >> + >> + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1 >> + || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2) >> + lanes = subdevs->bus.ccp2.lanecfg; >> + else >> + lanes = subdevs->bus.csi2.lanecfg; > > Shouldn't lane configuration be retrieved from the sensor instead ? Sensors > could use different lane configuration depending on the mode. This could also > be implemented later when needed, but I don't think it would be too difficult > to get it right now. I think we'd first need to standardise the CSI-2 bus configuration. I don't see a practical need to make the lane configuration dynamic. You could just use a lower frequency to achieve the same if you really need to. Ideally it might be nice to do but there's really nothing I know that required or even benefited from it --- at least for now. >> + >> + if (!lanes) { >> + dev_err(isp->dev, "no lane configuration\n"); >> + return -EINVAL; >> + } >> + >> + cam_phy_ctrl = omap_readl( >> + OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL); >> + /* >> + * SCM.CONTROL_CAMERA_PHY_CTRL >> + * - bit[4] : CSIPHY1 data sent to CSIB >> + * - bit [3:2] : CSIPHY1 config: 00 d-phy, 01/10 ccp2 >> + * - bit [1:0] : CSIPHY2 config: 00 d-phy, 01/10 ccp2 >> + */ >> + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1) >> + cam_phy_ctrl |= 1 << 2; >> + else if (subdevs->interface == ISP_INTERFACE_CSI2C_PHY1) >> + cam_phy_ctrl &= 1 << 2; >> + >> + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY2) >> + cam_phy_ctrl |= 1; >> + else if (subdevs->interface == ISP_INTERFACE_CSI2A_PHY2) >> + cam_phy_ctrl &= 1; >> + >> + /* FIXME: Do 34xx / 35xx require something here? */ >> + if (cpu_is_omap3630()) >> + omap_writel(cam_phy_ctrl, >> + OMAP343X_CTRL_BASE >> + + OMAP3630_CONTROL_CAMERA_PHY_CTRL); > > You use cam_phy_ctrl inside the if statement only, you can move its > computation there. Will fix. >> + >> + csi2_ddrclk_khz = sensor_fmt->pixelrate >> + / (2 * csi2->phy->num_data_lanes) >> + * omap3isp_video_format_info(sensor_fmt->code)->bpp; >> + >> + /* >> + * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1. >> + * THS_SETTLE: Programmed value = ceil(90 ns/DDRClk period) + 3. >> + */ >> + csi2phy.ths_term = DIV_ROUND_UP(25 * csi2_ddrclk_khz, 2000000) - 1; >> + csi2phy.ths_settle = DIV_ROUND_UP(90 * csi2_ddrclk_khz, 1000000) + 3; >> + csi2phy.tclk_term = TCLK_TERM; >> + csi2phy.tclk_miss = TCLK_MISS; >> + csi2phy.tclk_settle = TCLK_SETTLE; >> >> /* Clock and data lanes verification */ >> - for (i = 0; i < phy->num_data_lanes; i++) { >> + for (i = 0; i < csi2->phy->num_data_lanes; i++) { >> if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3) >> return -EINVAL; >> >> @@ -162,10 +227,10 @@ static int csiphy_config(struct isp_csiphy *phy, >> if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos)) >> return -EINVAL; >> >> - mutex_lock(&phy->mutex); >> - phy->dphy = *dphy; >> - phy->lanes = *lanes; >> - mutex_unlock(&phy->mutex); >> + mutex_lock(&csi2->phy->mutex); >> + csi2->phy->dphy = csi2phy; >> + csi2->phy->lanes = *lanes; >> + mutex_unlock(&csi2->phy->mutex); >> >> return 0; >> } >> @@ -225,8 +290,6 @@ int omap3isp_csiphy_init(struct isp_device *isp) >> struct isp_csiphy *phy1 = &isp->isp_csiphy1; >> struct isp_csiphy *phy2 = &isp->isp_csiphy2; >> >> - isp->platform_cb.csiphy_config = csiphy_config; >> - >> phy2->isp = isp; >> phy2->csi2 = &isp->isp_csi2a; >> phy2->num_data_lanes = ISP_CSIPHY2_NUM_DATA_LANES; >> diff --git a/drivers/media/video/omap3isp/ispcsiphy.h >> b/drivers/media/video/omap3isp/ispcsiphy.h index e93a661..9f93222 100644 >> --- a/drivers/media/video/omap3isp/ispcsiphy.h >> +++ b/drivers/media/video/omap3isp/ispcsiphy.h >> @@ -56,6 +56,10 @@ struct isp_csiphy { >> struct isp_csiphy_dphy_cfg dphy; >> }; >> >> +int omap3isp_csiphy_config(struct isp_device *isp, >> + struct v4l2_subdev *csi2_subdev, >> + struct v4l2_subdev *sensor, >> + struct v4l2_mbus_framefmt *fmt); >> int omap3isp_csiphy_acquire(struct isp_csiphy *phy); >> void omap3isp_csiphy_release(struct isp_csiphy *phy); >> int omap3isp_csiphy_init(struct isp_device *isp); >
Hi Sakari, On Saturday 07 January 2012 23:51:24 Sakari Ailus wrote: > Laurent Pinchart wrote: > > On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote: > >> From: Sakari Ailus <sakari.ailus@iki.fi> > >> > >> Configure CSI-2 phy based on platform data in the ISP driver rather than > >> in platform code. > >> > >> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> [snip] > >> diff --git a/drivers/media/video/omap3isp/ispcsiphy.c > >> b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 100644 > >> --- a/drivers/media/video/omap3isp/ispcsiphy.c > >> +++ b/drivers/media/video/omap3isp/ispcsiphy.c > >> @@ -28,6 +28,8 @@ > >> > >> #include <linux/device.h> > >> #include <linux/regulator/consumer.h> > >> > >> +#include "../../../../arch/arm/mach-omap2/control.h" > >> + > > > > #include <mach/control.h> > > > > (untested) ? > > I'm afraid it won't work. The above directory isn't in any include path > and likely shouldn't be. That file is included locally elsewhere, I > believe. You're right, I spoke too fast. > I wonder what would be the right way to access that register definition > I need from the file: > > OMAP343X_CTRL_BASE > OMAP3630_CONTROL_CAMERA_PHY_CTRL Maybe the file (or part of it) should be moved to an include directory ? > >> #include "isp.h" > >> #include "ispreg.h" > >> #include "ispcsiphy.h" > >> > >> @@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy > >> *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1); > >> > >> } > >> > >> -static int csiphy_config(struct isp_csiphy *phy, > >> - struct isp_csiphy_dphy_cfg *dphy, > >> - struct isp_csiphy_lanes_cfg *lanes) > >> +/* > >> + * TCLK values are OK at their reset values > >> + */ > >> +#define TCLK_TERM 0 > >> +#define TCLK_MISS 1 > >> +#define TCLK_SETTLE 14 > >> + > >> +int omap3isp_csiphy_config(struct isp_device *isp, > >> + struct v4l2_subdev *csi2_subdev, > >> + struct v4l2_subdev *sensor, > >> + struct v4l2_mbus_framefmt *sensor_fmt) > >> > >> { > >> > >> + struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv; > >> + struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev); > >> + struct isp_csiphy_dphy_cfg csi2phy; > >> + int csi2_ddrclk_khz; > >> + struct isp_csiphy_lanes_cfg *lanes; > >> > >> unsigned int used_lanes = 0; > >> unsigned int i; > >> > >> + u32 cam_phy_ctrl; > >> + > >> + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1 > >> + || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2) > >> + lanes = subdevs->bus.ccp2.lanecfg; > >> + else > >> + lanes = subdevs->bus.csi2.lanecfg; > > > > Shouldn't lane configuration be retrieved from the sensor instead ? > > Sensors could use different lane configuration depending on the mode. > > This could also be implemented later when needed, but I don't think it > > would be too difficult to get it right now. > > I think we'd first need to standardise the CSI-2 bus configuration. I > don't see a practical need to make the lane configuration dynamic. You > could just use a lower frequency to achieve the same if you really need to. > > Ideally it might be nice to do but there's really nothing I know that > required or even benefited from it --- at least for now. Does this mean that lane configuration needs to be duplicated in board code, on for the SMIA++ platform data and one of the OMAP3 ISP platform data ?
Hi Laurent, Laurent Pinchart wrote: > Hi Sakari, > > On Saturday 07 January 2012 23:51:24 Sakari Ailus wrote: >> Laurent Pinchart wrote: >>> On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote: >>>> From: Sakari Ailus <sakari.ailus@iki.fi> >>>> >>>> Configure CSI-2 phy based on platform data in the ISP driver rather than >>>> in platform code. >>>> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> > > [snip] > >>>> diff --git a/drivers/media/video/omap3isp/ispcsiphy.c >>>> b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 100644 >>>> --- a/drivers/media/video/omap3isp/ispcsiphy.c >>>> +++ b/drivers/media/video/omap3isp/ispcsiphy.c >>>> @@ -28,6 +28,8 @@ >>>> >>>> #include <linux/device.h> >>>> #include <linux/regulator/consumer.h> >>>> >>>> +#include "../../../../arch/arm/mach-omap2/control.h" >>>> + >>> >>> #include <mach/control.h> >>> >>> (untested) ? >> >> I'm afraid it won't work. The above directory isn't in any include path >> and likely shouldn't be. That file is included locally elsewhere, I >> believe. > > You're right, I spoke too fast. > >> I wonder what would be the right way to access that register definition >> I need from the file: >> >> OMAP343X_CTRL_BASE >> OMAP3630_CONTROL_CAMERA_PHY_CTRL > > Maybe the file (or part of it) should be moved to an include directory ? Yes, but which one? >>>> #include "isp.h" >>>> #include "ispreg.h" >>>> #include "ispcsiphy.h" >>>> >>>> @@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy >>>> *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1); >>>> >>>> } >>>> >>>> -static int csiphy_config(struct isp_csiphy *phy, >>>> - struct isp_csiphy_dphy_cfg *dphy, >>>> - struct isp_csiphy_lanes_cfg *lanes) >>>> +/* >>>> + * TCLK values are OK at their reset values >>>> + */ >>>> +#define TCLK_TERM 0 >>>> +#define TCLK_MISS 1 >>>> +#define TCLK_SETTLE 14 >>>> + >>>> +int omap3isp_csiphy_config(struct isp_device *isp, >>>> + struct v4l2_subdev *csi2_subdev, >>>> + struct v4l2_subdev *sensor, >>>> + struct v4l2_mbus_framefmt *sensor_fmt) >>>> >>>> { >>>> >>>> + struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv; >>>> + struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev); >>>> + struct isp_csiphy_dphy_cfg csi2phy; >>>> + int csi2_ddrclk_khz; >>>> + struct isp_csiphy_lanes_cfg *lanes; >>>> >>>> unsigned int used_lanes = 0; >>>> unsigned int i; >>>> >>>> + u32 cam_phy_ctrl; >>>> + >>>> + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1 >>>> + || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2) >>>> + lanes = subdevs->bus.ccp2.lanecfg; >>>> + else >>>> + lanes = subdevs->bus.csi2.lanecfg; >>> >>> Shouldn't lane configuration be retrieved from the sensor instead ? >>> Sensors could use different lane configuration depending on the mode. >>> This could also be implemented later when needed, but I don't think it >>> would be too difficult to get it right now. >> >> I think we'd first need to standardise the CSI-2 bus configuration. I >> don't see a practical need to make the lane configuration dynamic. You >> could just use a lower frequency to achieve the same if you really need to. >> >> Ideally it might be nice to do but there's really nothing I know that >> required or even benefited from it --- at least for now. > > Does this mean that lane configuration needs to be duplicated in board code, > on for the SMIA++ platform data and one of the OMAP3 ISP platform data ? It's mostly the number of lanes, and the polarity --- in theory it is possible to invert the signals on the bus, albeit I'm not sure if anyone does that; I can't see a reason for that, but hey, I don't know why it's possible to specify polarity either. :-) If both sides support mapping of the lanes, a mapping that matches on both sides has to be provided. I agree we should standardise the configuration of CSI-2 as well as the configuration of other busses. However, I would prefer to do that later on since I'm already depending on a number of other patches on the SMIA++ driver.
Hi, On 01/08/2012 11:26 AM, Sakari Ailus wrote: >>>> Shouldn't lane configuration be retrieved from the sensor instead ? >>>> Sensors could use different lane configuration depending on the mode. >>>> This could also be implemented later when needed, but I don't think it >>>> would be too difficult to get it right now. >>> >>> I think we'd first need to standardise the CSI-2 bus configuration. I >>> don't see a practical need to make the lane configuration dynamic. You >>> could just use a lower frequency to achieve the same if you really need to. >>> >>> Ideally it might be nice to do but there's really nothing I know that >>> required or even benefited from it --- at least for now. >> >> Does this mean that lane configuration needs to be duplicated in board code, >> on for the SMIA++ platform data and one of the OMAP3 ISP platform data ? > > It's mostly the number of lanes, and the polarity --- in theory it is > possible to invert the signals on the bus, albeit I'm not sure if anyone > does that; I can't see a reason for that, but hey, I don't know why it's > possible to specify polarity either. :-) I've never seen polarity configuration option in any datasheet, neither MIPI CSI-2 or D-PHY mentions that. Does OMAP3 ISP really allow MIPI-CSI lane signal polarity configuration ? MIPI-CSI2 uses differential signals after all. What would be a point of changing polarity ? > If both sides support mapping of the lanes, a mapping that matches on > both sides has to be provided. In Samsung SoC (both sensor and host interface) I've seen only possibility to configure the number of data lanes, FWIW I think it is assumed that when you use e.g. 2 data lanes always lane1 and lane2 are utilised for transmission, for 3 lanes - lane 1,2,3, etc. Also I've never seen on schematics that someone wires data lane3 and lane4 when only 2 lanes are utilised, so this makes me wonder if the lane mapping is ever needed. Has anyone different experience with that ? Also the standard seem to specify that Data1+ lane at a transmitter(Tx) is connected to Data1+ lane at a receiver(Rx), Data1-(Tx) to Data1-(Rx), Data2+(Tx) to Data2+(Rx), etc. I think this is needed due to explicitly defined data distribution and merging scheme among the lanes, i.e. to allow interworking of various receivers and transmitters. Thus it seems all we need need is just a number of data lanes used. > I agree we should standardise the configuration of CSI-2 as well as the > configuration of other busses. However, I would prefer to do that later > on since I'm already depending on a number of other patches on the > SMIA++ driver. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sakari, On Sunday 08 January 2012 11:26:02 Sakari Ailus wrote: > Laurent Pinchart wrote: > > On Saturday 07 January 2012 23:51:24 Sakari Ailus wrote: > >> Laurent Pinchart wrote: > >>> On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote: > >>>> From: Sakari Ailus <sakari.ailus@iki.fi> > >>>> > >>>> Configure CSI-2 phy based on platform data in the ISP driver rather > >>>> than in platform code. > >>>> > >>>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> > > > > [snip] > > > >>>> diff --git a/drivers/media/video/omap3isp/ispcsiphy.c > >>>> b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece > >>>> 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c > >>>> +++ b/drivers/media/video/omap3isp/ispcsiphy.c > >>>> @@ -28,6 +28,8 @@ > >>>> > >>>> #include <linux/device.h> > >>>> #include <linux/regulator/consumer.h> > >>>> > >>>> +#include "../../../../arch/arm/mach-omap2/control.h" > >>>> + > >>> > >>> #include <mach/control.h> > >>> > >>> (untested) ? > >> > >> I'm afraid it won't work. The above directory isn't in any include path > >> and likely shouldn't be. That file is included locally elsewhere, I > >> believe. > > > > You're right, I spoke too fast. > > > >> I wonder what would be the right way to access that register definition > >> > >> I need from the file: > >> OMAP343X_CTRL_BASE > >> OMAP3630_CONTROL_CAMERA_PHY_CTRL > > > > Maybe the file (or part of it) should be moved to an include directory ? > > Yes, but which one? Good question. The content of control.h doesn't seem to have been meant to be exported. Maybe you should ask on linux-omap@vger.kernel.org ? > >>>> #include "isp.h" > >>>> #include "ispreg.h" > >>>> #include "ispcsiphy.h" > >>>> > >>>> @@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy > >>>> *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1); > >>>> > >>>> } > >>>> > >>>> -static int csiphy_config(struct isp_csiphy *phy, > >>>> - struct isp_csiphy_dphy_cfg *dphy, > >>>> - struct isp_csiphy_lanes_cfg *lanes) > >>>> +/* > >>>> + * TCLK values are OK at their reset values > >>>> + */ > >>>> +#define TCLK_TERM 0 > >>>> +#define TCLK_MISS 1 > >>>> +#define TCLK_SETTLE 14 > >>>> + > >>>> +int omap3isp_csiphy_config(struct isp_device *isp, > >>>> + struct v4l2_subdev *csi2_subdev, > >>>> + struct v4l2_subdev *sensor, > >>>> + struct v4l2_mbus_framefmt *sensor_fmt) > >>>> > >>>> { > >>>> > >>>> + struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv; > >>>> + struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev); > >>>> + struct isp_csiphy_dphy_cfg csi2phy; > >>>> + int csi2_ddrclk_khz; > >>>> + struct isp_csiphy_lanes_cfg *lanes; > >>>> > >>>> unsigned int used_lanes = 0; > >>>> unsigned int i; > >>>> > >>>> + u32 cam_phy_ctrl; > >>>> + > >>>> + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1 > >>>> + || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2) > >>>> + lanes = subdevs->bus.ccp2.lanecfg; > >>>> + else > >>>> + lanes = subdevs->bus.csi2.lanecfg; > >>> > >>> Shouldn't lane configuration be retrieved from the sensor instead ? > >>> Sensors could use different lane configuration depending on the mode. > >>> This could also be implemented later when needed, but I don't think it > >>> would be too difficult to get it right now. > >> > >> I think we'd first need to standardise the CSI-2 bus configuration. I > >> don't see a practical need to make the lane configuration dynamic. You > >> could just use a lower frequency to achieve the same if you really need > >> to. > >> > >> Ideally it might be nice to do but there's really nothing I know that > >> required or even benefited from it --- at least for now. > > > > Does this mean that lane configuration needs to be duplicated in board > > code, on for the SMIA++ platform data and one of the OMAP3 ISP platform > > data ? > > It's mostly the number of lanes, and the polarity --- in theory it is > possible to invert the signals on the bus, albeit I'm not sure if anyone > does that; I can't see a reason for that, but hey, I don't know why it's > possible to specify polarity either. :-) > > If both sides support mapping of the lanes, a mapping that matches on > both sides has to be provided. > > I agree we should standardise the configuration of CSI-2 as well as the > configuration of other busses. However, I would prefer to do that later > on since I'm already depending on a number of other patches on the > SMIA++ driver. OK.
Hi Sylwester, Sylwester Nawrocki wrote: > Hi, > > On 01/08/2012 11:26 AM, Sakari Ailus wrote: >>>>> Shouldn't lane configuration be retrieved from the sensor instead ? >>>>> Sensors could use different lane configuration depending on the mode. >>>>> This could also be implemented later when needed, but I don't think it >>>>> would be too difficult to get it right now. >>>> >>>> I think we'd first need to standardise the CSI-2 bus configuration. I >>>> don't see a practical need to make the lane configuration dynamic. You >>>> could just use a lower frequency to achieve the same if you really need to. >>>> >>>> Ideally it might be nice to do but there's really nothing I know that >>>> required or even benefited from it --- at least for now. >>> >>> Does this mean that lane configuration needs to be duplicated in board code, >>> on for the SMIA++ platform data and one of the OMAP3 ISP platform data ? >> >> It's mostly the number of lanes, and the polarity --- in theory it is >> possible to invert the signals on the bus, albeit I'm not sure if anyone >> does that; I can't see a reason for that, but hey, I don't know why it's >> possible to specify polarity either. :-) > > I've never seen polarity configuration option in any datasheet, neither > MIPI CSI-2 or D-PHY mentions that. Does OMAP3 ISP really allow MIPI-CSI > lane signal polarity configuration ? MIPI-CSI2 uses differential signals > after all. What would be a point of changing polarity ? I don't know. It's also the same for CSI-1 on OMAP 3. This is actually one of the issues here: also device specific configuration is required. The standard configuration must contain probably at least what the spec defines. >> If both sides support mapping of the lanes, a mapping that matches on >> both sides has to be provided. > > In Samsung SoC (both sensor and host interface) I've seen only possibility > to configure the number of data lanes, FWIW I think it is assumed that > when you use e.g. 2 data lanes always lane1 and lane2 are utilised for > transmission, for 3 lanes - lane 1,2,3, etc. Also I've never seen on > schematics that someone wires data lane3 and lane4 when only 2 lanes > are utilised, so this makes me wonder if the lane mapping is ever needed. > > Has anyone different experience with that ? > > Also the standard seem to specify that Data1+ lane at a transmitter(Tx) is > connected to Data1+ lane at a receiver(Rx), Data1-(Tx) to Data1-(Rx), > Data2+(Tx) to Data2+(Rx), etc. I think this is needed due to explicitly > defined data distribution and merging scheme among the lanes, i.e. to allow > interworking of various receivers and transmitters. > > Thus it seems all we need need is just a number of data lanes used. The standard of course specifies that the data lanes must be connected correctly. :-) It can't specify which SoC pins do they use, so for added flexibility it's good to be able to reorder them. Have you ever worked with single-layer PCBs by any chance? :-) More layers are used these days but it still doesn't solve all possible issues. So I think I can say reordering generally must be supported by software if the hardware can do that.
Hi Sakari, On 01/08/2012 12:16 PM, Sakari Ailus wrote: >>>>>> Shouldn't lane configuration be retrieved from the sensor instead ? >>>>>> Sensors could use different lane configuration depending on the mode. >>>>>> This could also be implemented later when needed, but I don't think it >>>>>> would be too difficult to get it right now. >>>>> >>>>> I think we'd first need to standardise the CSI-2 bus configuration. I >>>>> don't see a practical need to make the lane configuration dynamic. You >>>>> could just use a lower frequency to achieve the same if you really need to. >>>>> >>>>> Ideally it might be nice to do but there's really nothing I know that >>>>> required or even benefited from it --- at least for now. >>>> >>>> Does this mean that lane configuration needs to be duplicated in board code, >>>> on for the SMIA++ platform data and one of the OMAP3 ISP platform data ? >>> >>> It's mostly the number of lanes, and the polarity --- in theory it is >>> possible to invert the signals on the bus, albeit I'm not sure if anyone >>> does that; I can't see a reason for that, but hey, I don't know why it's >>> possible to specify polarity either. :-) I think it just enables to swap D+ and D- functions on the physical pins. >> I've never seen polarity configuration option in any datasheet, neither >> MIPI CSI-2 or D-PHY mentions that. Does OMAP3 ISP really allow MIPI-CSI >> lane signal polarity configuration ? MIPI-CSI2 uses differential signals >> after all. What would be a point of changing polarity ? > > I don't know. It's also the same for CSI-1 on OMAP 3. > > This is actually one of the issues here: also device specific > configuration is required. The standard configuration must contain > probably at least what the spec defines. > >>> If both sides support mapping of the lanes, a mapping that matches on >>> both sides has to be provided. >> >> In Samsung SoC (both sensor and host interface) I've seen only possibility >> to configure the number of data lanes, FWIW I think it is assumed that >> when you use e.g. 2 data lanes always lane1 and lane2 are utilised for >> transmission, for 3 lanes - lane 1,2,3, etc. Also I've never seen on >> schematics that someone wires data lane3 and lane4 when only 2 lanes >> are utilised, so this makes me wonder if the lane mapping is ever needed. >> >> Has anyone different experience with that ? >> >> Also the standard seem to specify that Data1+ lane at a transmitter(Tx) is >> connected to Data1+ lane at a receiver(Rx), Data1-(Tx) to Data1-(Rx), >> Data2+(Tx) to Data2+(Rx), etc. I think this is needed due to explicitly >> defined data distribution and merging scheme among the lanes, i.e. to allow >> interworking of various receivers and transmitters. >> >> Thus it seems all we need need is just a number of data lanes used. > > The standard of course specifies that the data lanes must be connected > correctly. :-) It can't specify which SoC pins do they use, so for added > flexibility it's good to be able to reorder them. > > Have you ever worked with single-layer PCBs by any chance? :-) More > layers are used these days but it still doesn't solve all possible issues. Yes, I have. I know what you mean. It just seemed uncommon to me to reorder the signals. But since H/W doing that exists..and that might become more widely used in the future it might make sense to standardize lane configuration. > So I think I can say reordering generally must be supported by software > if the hardware can do that. Yes, however there is always a board specific information involved, isn't it ? I.e. transmitter can reorder signals between its pins, the same can happen at a receiver and additionally the transmitter's pins can be connected differently to the receiver pins, depending on the board ? Then do we make board specific information part of sensor's or host platform data ? It probably should be at both, let's take an evaluation and a camera daughter boards as an example. We also need device tree bindings for that, if possible the best would be to design common bindings, at least basic ones, to which device specific ones could be added. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester, Sylwester Nawrocki wrote: > On 01/08/2012 12:16 PM, Sakari Ailus wrote: >>>>>>> Shouldn't lane configuration be retrieved from the sensor instead ? >>>>>>> Sensors could use different lane configuration depending on the mode. >>>>>>> This could also be implemented later when needed, but I don't think it >>>>>>> would be too difficult to get it right now. >>>>>> >>>>>> I think we'd first need to standardise the CSI-2 bus configuration. I >>>>>> don't see a practical need to make the lane configuration dynamic. You >>>>>> could just use a lower frequency to achieve the same if you really need to. >>>>>> >>>>>> Ideally it might be nice to do but there's really nothing I know that >>>>>> required or even benefited from it --- at least for now. >>>>> >>>>> Does this mean that lane configuration needs to be duplicated in board code, >>>>> on for the SMIA++ platform data and one of the OMAP3 ISP platform data ? >>>> >>>> It's mostly the number of lanes, and the polarity --- in theory it is >>>> possible to invert the signals on the bus, albeit I'm not sure if anyone >>>> does that; I can't see a reason for that, but hey, I don't know why it's >>>> possible to specify polarity either. :-) > > I think it just enables to swap D+ and D- functions on the physical pins. Good thinking. :-) Yeah, it's differential, so yes, I think so too now that you mention it. >>> I've never seen polarity configuration option in any datasheet, neither >>> MIPI CSI-2 or D-PHY mentions that. Does OMAP3 ISP really allow MIPI-CSI >>> lane signal polarity configuration ? MIPI-CSI2 uses differential signals >>> after all. What would be a point of changing polarity ? >> >> I don't know. It's also the same for CSI-1 on OMAP 3. >> >> This is actually one of the issues here: also device specific >> configuration is required. The standard configuration must contain >> probably at least what the spec defines. >> >>>> If both sides support mapping of the lanes, a mapping that matches on >>>> both sides has to be provided. >>> >>> In Samsung SoC (both sensor and host interface) I've seen only possibility >>> to configure the number of data lanes, FWIW I think it is assumed that >>> when you use e.g. 2 data lanes always lane1 and lane2 are utilised for >>> transmission, for 3 lanes - lane 1,2,3, etc. Also I've never seen on >>> schematics that someone wires data lane3 and lane4 when only 2 lanes >>> are utilised, so this makes me wonder if the lane mapping is ever needed. >>> >>> Has anyone different experience with that ? >>> >>> Also the standard seem to specify that Data1+ lane at a transmitter(Tx) is >>> connected to Data1+ lane at a receiver(Rx), Data1-(Tx) to Data1-(Rx), >>> Data2+(Tx) to Data2+(Rx), etc. I think this is needed due to explicitly >>> defined data distribution and merging scheme among the lanes, i.e. to allow >>> interworking of various receivers and transmitters. >>> >>> Thus it seems all we need need is just a number of data lanes used. >> >> The standard of course specifies that the data lanes must be connected >> correctly. :-) It can't specify which SoC pins do they use, so for added >> flexibility it's good to be able to reorder them. >> >> Have you ever worked with single-layer PCBs by any chance? :-) More >> layers are used these days but it still doesn't solve all possible issues. > > Yes, I have. I know what you mean. It just seemed uncommon to me to reorder > the signals. But since H/W doing that exists..and that might become more > widely used in the future it might make sense to standardize lane > configuration. We'll need different mapping configuration for the receiver and the transmitter, and not all the hardware supports it. It won't be many bytes, though. >> So I think I can say reordering generally must be supported by software >> if the hardware can do that. > > Yes, however there is always a board specific information involved, isn't it ? > I.e. transmitter can reorder signals between its pins, the same can happen at > a receiver and additionally the transmitter's pins can be connected differently > to the receiver pins, depending on the board ? Exactly. That's the reason, I think, why the hardware does support it. > Then do we make board specific information part of sensor's or host platform > data ? It probably should be at both, let's take an evaluation and a camera > daughter boards as an example. > > We also need device tree bindings for that, if possible the best would be to > design common bindings, at least basic ones, to which device specific ones > could be added. Sounds good to me.
diff --git a/drivers/media/video/omap3isp/isp.c b/drivers/media/video/omap3isp/isp.c index b818cac..6020fd7 100644 --- a/drivers/media/video/omap3isp/isp.c +++ b/drivers/media/video/omap3isp/isp.c @@ -737,7 +737,7 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe, struct isp_device *isp = pipe->output->isp; struct media_entity *entity; struct media_pad *pad; - struct v4l2_subdev *subdev; + struct v4l2_subdev *subdev = NULL, *prev_subdev; unsigned long flags; int ret; @@ -759,11 +759,41 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe, break; entity = pad->entity; + prev_subdev = subdev; subdev = media_entity_to_v4l2_subdev(entity); - ret = v4l2_subdev_call(subdev, video, s_stream, mode); - if (ret < 0 && ret != -ENOIOCTLCMD) - return ret; + /* Configure CSI-2 receiver based on sensor format. */ + if (prev_subdev == &isp->isp_csi2a.subdev + || prev_subdev == &isp->isp_csi2c.subdev) { + struct v4l2_subdev_format fmt; + + fmt.pad = pad->index; + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; + ret = v4l2_subdev_call(subdev, pad, get_fmt, + NULL, &fmt); + if (ret < 0) + return -EPIPE; + + ret = omap3isp_csiphy_config( + isp, prev_subdev, subdev, + &fmt.format); + if (ret < 0) + return -EPIPE; + + /* Start CSI-2 after configuration. */ + ret = v4l2_subdev_call(prev_subdev, video, + s_stream, mode); + if (ret < 0 && ret != -ENOIOCTLCMD) + return ret; + } + + /* Start any other subdev except the CSI-2 receivers. */ + if (subdev != &isp->isp_csi2a.subdev + && subdev != &isp->isp_csi2c.subdev) { + ret = v4l2_subdev_call(subdev, video, s_stream, mode); + if (ret < 0 && ret != -ENOIOCTLCMD) + return ret; + } if (subdev == &isp->isp_ccdc.subdev) { v4l2_subdev_call(&isp->isp_aewb.subdev, video, diff --git a/drivers/media/video/omap3isp/isp.h b/drivers/media/video/omap3isp/isp.h index 705946e..c5935ae 100644 --- a/drivers/media/video/omap3isp/isp.h +++ b/drivers/media/video/omap3isp/isp.h @@ -126,9 +126,6 @@ struct isp_reg { struct isp_platform_callback { u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel); - int (*csiphy_config)(struct isp_csiphy *phy, - struct isp_csiphy_dphy_cfg *dphy, - struct isp_csiphy_lanes_cfg *lanes); void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk); }; diff --git a/drivers/media/video/omap3isp/ispcsiphy.c b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c +++ b/drivers/media/video/omap3isp/ispcsiphy.c @@ -28,6 +28,8 @@ #include <linux/device.h> #include <linux/regulator/consumer.h> +#include "../../../../arch/arm/mach-omap2/control.h" + #include "isp.h" #include "ispreg.h" #include "ispcsiphy.h" @@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1); } -static int csiphy_config(struct isp_csiphy *phy, - struct isp_csiphy_dphy_cfg *dphy, - struct isp_csiphy_lanes_cfg *lanes) +/* + * TCLK values are OK at their reset values + */ +#define TCLK_TERM 0 +#define TCLK_MISS 1 +#define TCLK_SETTLE 14 + +int omap3isp_csiphy_config(struct isp_device *isp, + struct v4l2_subdev *csi2_subdev, + struct v4l2_subdev *sensor, + struct v4l2_mbus_framefmt *sensor_fmt) { + struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv; + struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev); + struct isp_csiphy_dphy_cfg csi2phy; + int csi2_ddrclk_khz; + struct isp_csiphy_lanes_cfg *lanes; unsigned int used_lanes = 0; unsigned int i; + u32 cam_phy_ctrl; + + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1 + || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2) + lanes = subdevs->bus.ccp2.lanecfg; + else + lanes = subdevs->bus.csi2.lanecfg; + + if (!lanes) { + dev_err(isp->dev, "no lane configuration\n"); + return -EINVAL; + } + + cam_phy_ctrl = omap_readl( + OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL); + /* + * SCM.CONTROL_CAMERA_PHY_CTRL + * - bit[4] : CSIPHY1 data sent to CSIB + * - bit [3:2] : CSIPHY1 config: 00 d-phy, 01/10 ccp2 + * - bit [1:0] : CSIPHY2 config: 00 d-phy, 01/10 ccp2 + */ + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1) + cam_phy_ctrl |= 1 << 2; + else if (subdevs->interface == ISP_INTERFACE_CSI2C_PHY1) + cam_phy_ctrl &= 1 << 2; + + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY2) + cam_phy_ctrl |= 1; + else if (subdevs->interface == ISP_INTERFACE_CSI2A_PHY2) + cam_phy_ctrl &= 1; + + /* FIXME: Do 34xx / 35xx require something here? */ + if (cpu_is_omap3630()) + omap_writel(cam_phy_ctrl, + OMAP343X_CTRL_BASE + + OMAP3630_CONTROL_CAMERA_PHY_CTRL); + + csi2_ddrclk_khz = sensor_fmt->pixelrate + / (2 * csi2->phy->num_data_lanes) + * omap3isp_video_format_info(sensor_fmt->code)->bpp; + + /* + * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1. + * THS_SETTLE: Programmed value = ceil(90 ns/DDRClk period) + 3. + */ + csi2phy.ths_term = DIV_ROUND_UP(25 * csi2_ddrclk_khz, 2000000) - 1; + csi2phy.ths_settle = DIV_ROUND_UP(90 * csi2_ddrclk_khz, 1000000) + 3; + csi2phy.tclk_term = TCLK_TERM; + csi2phy.tclk_miss = TCLK_MISS; + csi2phy.tclk_settle = TCLK_SETTLE; /* Clock and data lanes verification */ - for (i = 0; i < phy->num_data_lanes; i++) { + for (i = 0; i < csi2->phy->num_data_lanes; i++) { if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3) return -EINVAL; @@ -162,10 +227,10 @@ static int csiphy_config(struct isp_csiphy *phy, if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos)) return -EINVAL; - mutex_lock(&phy->mutex); - phy->dphy = *dphy; - phy->lanes = *lanes; - mutex_unlock(&phy->mutex); + mutex_lock(&csi2->phy->mutex); + csi2->phy->dphy = csi2phy; + csi2->phy->lanes = *lanes; + mutex_unlock(&csi2->phy->mutex); return 0; } @@ -225,8 +290,6 @@ int omap3isp_csiphy_init(struct isp_device *isp) struct isp_csiphy *phy1 = &isp->isp_csiphy1; struct isp_csiphy *phy2 = &isp->isp_csiphy2; - isp->platform_cb.csiphy_config = csiphy_config; - phy2->isp = isp; phy2->csi2 = &isp->isp_csi2a; phy2->num_data_lanes = ISP_CSIPHY2_NUM_DATA_LANES; diff --git a/drivers/media/video/omap3isp/ispcsiphy.h b/drivers/media/video/omap3isp/ispcsiphy.h index e93a661..9f93222 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.h +++ b/drivers/media/video/omap3isp/ispcsiphy.h @@ -56,6 +56,10 @@ struct isp_csiphy { struct isp_csiphy_dphy_cfg dphy; }; +int omap3isp_csiphy_config(struct isp_device *isp, + struct v4l2_subdev *csi2_subdev, + struct v4l2_subdev *sensor, + struct v4l2_mbus_framefmt *fmt); int omap3isp_csiphy_acquire(struct isp_csiphy *phy); void omap3isp_csiphy_release(struct isp_csiphy *phy); int omap3isp_csiphy_init(struct isp_device *isp);