[PATCH v3 0/3] OMAP 3 CSI-2 configuration

[PATCH v3 0/3] OMAP 3 CSI-2 configuration

[Sakari Ailus]

Hi all,

This is an update to an old patchset for CSI-2 configuration for OMAP 3430
and 3630r. The patches have been tested on the 3630 only so far, and I don't
plan to test them on 3430 in the near future.

I changed quite a few things after a discussion with Tony a few days ago.
The ISP driver now maps the relevant register from the control block and
uses it directly. Which register is required is determined by the ISP
revision: this is theoretically wrong, but since we only support OMAP 3430
and 3630 which have different ISPs it should be all right. If we need to
support more OMAPs in the future we could revisit how that's being
determined.

Comments, questions and other kind of feedback is very welcome.

Kind regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v3 1/3] omap3isp: Add CSI configuration registers from control block to ISP resources

[Sakari Ailus]

Add the registers used to configure the CSI-2 receiver PHY on OMAP3430 and
3630 and map them in the ISP driver. The register is part of the control
block but it only is needed by the ISP driver.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 arch/arm/mach-omap2/devices.c         |   10 ++++++++++
 drivers/media/platform/omap3isp/isp.c |    6 ++++--
 drivers/media/platform/omap3isp/isp.h |    2 ++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index c00c689..9e4d5da 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -201,6 +201,16 @@ static struct resource omap3isp_resources[] = {
 		.flags		= IORESOURCE_MEM,
 	},
 	{
+		.start		= OMAP343X_CTRL_BASE + OMAP343X_CONTROL_CSIRXFE,
+		.end		= OMAP343X_CTRL_BASE + OMAP343X_CONTROL_CSIRXFE + 3,
+		.flags		= IORESOURCE_MEM,
+	},
+	{
+		.start		= OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL,
+		.end		= OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL + 3,
+		.flags		= IORESOURCE_MEM,
+	},
+	{
 		.start		= INT_34XX_CAM_IRQ,
 		.flags		= IORESOURCE_IRQ,
 	}
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index d7aa513..88fba2c 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -100,7 +100,8 @@ static const struct isp_res_mapping isp_res_maps[] = {
 		       1 << OMAP3_ISP_IOMEM_RESZ |
 		       1 << OMAP3_ISP_IOMEM_SBL |
 		       1 << OMAP3_ISP_IOMEM_CSI2A_REGS1 |
-		       1 << OMAP3_ISP_IOMEM_CSIPHY2,
+		       1 << OMAP3_ISP_IOMEM_CSIPHY2 |
+		       1 << OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE,
 	},
 	{
 		.isp_rev = ISP_REVISION_15_0,
@@ -117,7 +118,8 @@ static const struct isp_res_mapping isp_res_maps[] = {
 		       1 << OMAP3_ISP_IOMEM_CSI2A_REGS2 |
 		       1 << OMAP3_ISP_IOMEM_CSI2C_REGS1 |
 		       1 << OMAP3_ISP_IOMEM_CSIPHY1 |
-		       1 << OMAP3_ISP_IOMEM_CSI2C_REGS2,
+		       1 << OMAP3_ISP_IOMEM_CSI2C_REGS2 |
+		       1 << OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL,
 	},
 };
 
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 8be7487..6fed222 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -72,6 +72,8 @@ enum isp_mem_resources {
 	OMAP3_ISP_IOMEM_CSI2C_REGS1,
 	OMAP3_ISP_IOMEM_CSIPHY1,
 	OMAP3_ISP_IOMEM_CSI2C_REGS2,
+	OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE,
+	OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL,
 	OMAP3_ISP_IOMEM_LAST
 };
 
-- 
1.7.2.5

Re: [PATCH v3 1/3] omap3isp: Add CSI configuration registers from control block to ISP resources

[Tony Lindgren]

* Sakari Ailus <sakari.ailus@iki.fi> [121007 13:09]:
> Add the registers used to configure the CSI-2 receiver PHY on OMAP3430 and
> 3630 and map them in the ISP driver. The register is part of the control
> block but it only is needed by the ISP driver.

Just checking.. These do get reserved with request_mem_region()
in isp_map_mem_resource() before they get ioremapped, right?

And camera is the only user for these registers and they are
not shared with anything else?

If so, then this is OK to merge via the media patches:

Acked-by: Tony Lindgren <tony@atomide.com>
 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  arch/arm/mach-omap2/devices.c         |   10 ++++++++++
>  drivers/media/platform/omap3isp/isp.c |    6 ++++--
>  drivers/media/platform/omap3isp/isp.h |    2 ++
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> index c00c689..9e4d5da 100644
> --- a/arch/arm/mach-omap2/devices.c
> +++ b/arch/arm/mach-omap2/devices.c
> @@ -201,6 +201,16 @@ static struct resource omap3isp_resources[] = {
>  		.flags		= IORESOURCE_MEM,
>  	},
>  	{
> +		.start		= OMAP343X_CTRL_BASE + OMAP343X_CONTROL_CSIRXFE,
> +		.end		= OMAP343X_CTRL_BASE + OMAP343X_CONTROL_CSIRXFE + 3,
> +		.flags		= IORESOURCE_MEM,
> +	},
> +	{
> +		.start		= OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL,
> +		.end		= OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL + 3,
> +		.flags		= IORESOURCE_MEM,
> +	},
> +	{
>  		.start		= INT_34XX_CAM_IRQ,
>  		.flags		= IORESOURCE_IRQ,
>  	}
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index d7aa513..88fba2c 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -100,7 +100,8 @@ static const struct isp_res_mapping isp_res_maps[] = {
>  		       1 << OMAP3_ISP_IOMEM_RESZ |
>  		       1 << OMAP3_ISP_IOMEM_SBL |
>  		       1 << OMAP3_ISP_IOMEM_CSI2A_REGS1 |
> -		       1 << OMAP3_ISP_IOMEM_CSIPHY2,
> +		       1 << OMAP3_ISP_IOMEM_CSIPHY2 |
> +		       1 << OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE,
>  	},
>  	{
>  		.isp_rev = ISP_REVISION_15_0,
> @@ -117,7 +118,8 @@ static const struct isp_res_mapping isp_res_maps[] = {
>  		       1 << OMAP3_ISP_IOMEM_CSI2A_REGS2 |
>  		       1 << OMAP3_ISP_IOMEM_CSI2C_REGS1 |
>  		       1 << OMAP3_ISP_IOMEM_CSIPHY1 |
> -		       1 << OMAP3_ISP_IOMEM_CSI2C_REGS2,
> +		       1 << OMAP3_ISP_IOMEM_CSI2C_REGS2 |
> +		       1 << OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL,
>  	},
>  };
>  
> diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
> index 8be7487..6fed222 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -72,6 +72,8 @@ enum isp_mem_resources {
>  	OMAP3_ISP_IOMEM_CSI2C_REGS1,
>  	OMAP3_ISP_IOMEM_CSIPHY1,
>  	OMAP3_ISP_IOMEM_CSI2C_REGS2,
> +	OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE,
> +	OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL,
>  	OMAP3_ISP_IOMEM_LAST
>  };
>  
> -- 
> 1.7.2.5
> 

Re: [PATCH v3 1/3] omap3isp: Add CSI configuration registers from control block to ISP resources

[Sakari Ailus]

Moikka,

On Mon, Oct 08, 2012 at 03:06:46PM -0700, Tony Lindgren wrote:
> * Sakari Ailus <sakari.ailus@iki.fi> [121007 13:09]:
> > Add the registers used to configure the CSI-2 receiver PHY on OMAP3430 and
> > 3630 and map them in the ISP driver. The register is part of the control
> > block but it only is needed by the ISP driver.
> 
> Just checking.. These do get reserved with request_mem_region()
> in isp_map_mem_resource() before they get ioremapped, right?

That's right. The code doing that can be found in
drivers/media/platform/omap3isp/isp.c in a function called
isp_map_mem_resource().

> And camera is the only user for these registers and they are
> not shared with anything else?

Correct. These registers only contain bits for configuring the CSI-2 PHY
routing and CCP2 clock/strobe mode.

> If so, then this is OK to merge via the media patches:
> 
> Acked-by: Tony Lindgren <tony@atomide.com>

Thanks! :-)

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v3 2/3] omap3isp: Add PHY routing configuration

[Sakari Ailus]

Add PHY routing configuration for both 3430 and 3630. Also add register bit
definitions of CSIRXFE and CAMERA_PHY_CTRL registers on OMAP 3430 and 3630,
respectively.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/platform/omap3isp/ispcsiphy.c |   86 +++++++++++++++++++++++++++
 drivers/media/platform/omap3isp/ispreg.h    |   22 +++++++
 2 files changed, 108 insertions(+), 0 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 348f67e..f13bfbd 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -32,6 +32,92 @@
 #include "ispreg.h"
 #include "ispcsiphy.h"
 
+static void csiphy_routing_cfg_3630(struct isp_csiphy *phy, u32 iface,
+				    bool ccp2_strobe)
+{
+	u32 cam_phy_ctrl =
+		isp_reg_readl(phy->isp,
+			      OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL, 0);
+	u32 shift, mode;
+
+	switch (iface) {
+	case ISP_INTERFACE_CCP2B_PHY1:
+		cam_phy_ctrl &=
+			~OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
+		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
+		break;
+	case ISP_INTERFACE_CSI2C_PHY1:
+		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
+		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
+		break;
+	case ISP_INTERFACE_CCP2B_PHY2:
+		cam_phy_ctrl |=
+			OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
+		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
+		break;
+	case ISP_INTERFACE_CSI2A_PHY2:
+		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
+		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
+		break;
+	default:
+		pr_warn("bad iface %d\n", iface);
+		return;
+	}
+
+	/* Select data/clock or data/strobe mode for CCP2 */
+	switch (iface) {
+	case ISP_INTERFACE_CCP2B_PHY1:
+	case ISP_INTERFACE_CCP2B_PHY2:
+		if (ccp2_strobe)
+			mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_STROBE;
+		else
+			mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_CLOCK;
+	}
+
+	cam_phy_ctrl &=
+		~(OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_MASK << shift);
+	cam_phy_ctrl |= mode << shift;
+
+	isp_reg_writel(phy->isp, cam_phy_ctrl,
+		       OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL, 0);
+}
+
+static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
+				    bool ccp2_strobe)
+{
+	uint32_t csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
+		| OMAP343X_CONTROL_CSIRXFE_RESET;
+
+	/* Nothing to configure here. */
+	if (iface == ISP_INTERFACE_CSI2A_PHY2)
+		return;
+
+	if (iface != ISP_INTERFACE_CCP2B_PHY1)
+		return;
+
+	if (!on) {
+		isp_reg_writel(phy->isp, 0,
+			       OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE, 0);
+		return;
+	}
+
+	if (ccp2_strobe)
+		csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+
+	isp_reg_writel(phy->isp, csirxfe,
+		       OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE, 0);
+}
+
+static void csiphy_routing_cfg(struct isp_csiphy *phy, u32 iface, bool on,
+			       bool ccp2_strobe)
+{
+	if (phy->isp->mmio_base[OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL]
+	    && on)
+		return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
+	if (phy->isp->mmio_base[OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE])
+		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe);
+}
+
 /*
  * csiphy_lanes_config - Configuration of CSIPHY lanes.
  *
diff --git a/drivers/media/platform/omap3isp/ispreg.h b/drivers/media/platform/omap3isp/ispreg.h
index 084ea77..66d2b42 100644
--- a/drivers/media/platform/omap3isp/ispreg.h
+++ b/drivers/media/platform/omap3isp/ispreg.h
@@ -1583,4 +1583,26 @@
 #define ISPCSIPHY_REG2_CCP2_SYNC_PATTERN_MASK		\
 	(0x7fffff << ISPCSIPHY_REG2_CCP2_SYNC_PATTERN_SHIFT)
 
+/* -----------------------------------------------------------------------------
+ * CONTROL registers for CSI-2 phy routing
+ */
+
+/* OMAP343X_CONTROL_CSIRXFE */
+#define OMAP343X_CONTROL_CSIRXFE_CSIB_INV	(1 << 7)
+#define OMAP343X_CONTROL_CSIRXFE_RESENABLE	(1 << 8)
+#define OMAP343X_CONTROL_CSIRXFE_SELFORM	(1 << 10)
+#define OMAP343X_CONTROL_CSIRXFE_PWRDNZ		(1 << 12)
+#define OMAP343X_CONTROL_CSIRXFE_RESET		(1 << 13)
+
+/* OMAP3630_CONTROL_CAMERA_PHY_CTRL */
+#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT	2
+#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT	0
+#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY		0x0
+#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_STROBE 0x1
+#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_CLOCK 0x2
+#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_GPI		0x3
+#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_MASK		0x3
+/* CCP2B: set to receive data from PHY2 instead of PHY1 */
+#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2	(1 << 4)
+
 #endif	/* OMAP3_ISP_REG_H */
-- 
1.7.2.5

Re: [PATCH v3 2/3] omap3isp: Add PHY routing configuration

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Sunday 07 October 2012 23:07:51 Sakari Ailus wrote:
> Add PHY routing configuration for both 3430 and 3630. Also add register bit
> definitions of CSIRXFE and CAMERA_PHY_CTRL registers on OMAP 3430 and 3630,
> respectively.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/platform/omap3isp/ispcsiphy.c |   86 ++++++++++++++++++++++++
>  drivers/media/platform/omap3isp/ispreg.h    |   22 +++++++
>  2 files changed, 108 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> b/drivers/media/platform/omap3isp/ispcsiphy.c index 348f67e..f13bfbd 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -32,6 +32,92 @@
>  #include "ispreg.h"
>  #include "ispcsiphy.h"
> 
> +static void csiphy_routing_cfg_3630(struct isp_csiphy *phy, u32 iface,
> +				    bool ccp2_strobe)
> +{
> +	u32 cam_phy_ctrl =

If you call the variable "value" or "ctrl" two statements below could fit on 
one line, but that's up to you :-)

> +		isp_reg_readl(phy->isp,
> +			      OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL, 0);
> +	u32 shift, mode;
> +
> +	switch (iface) {
> +	case ISP_INTERFACE_CCP2B_PHY1:
> +		cam_phy_ctrl &=
> +			~OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
> +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
> +		break;
> +	case ISP_INTERFACE_CSI2C_PHY1:
> +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
> +		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
> +		break;
> +	case ISP_INTERFACE_CCP2B_PHY2:
> +		cam_phy_ctrl |=
> +			OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
> +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
> +		break;
> +	case ISP_INTERFACE_CSI2A_PHY2:
> +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
> +		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
> +		break;
> +	default:
> +		pr_warn("bad iface %d\n", iface);

As you already know, dev_warn() is a better idea. Can this actually happen ?

> +		return;
> +	}
> +
> +	/* Select data/clock or data/strobe mode for CCP2 */
> +	switch (iface) {
> +	case ISP_INTERFACE_CCP2B_PHY1:
> +	case ISP_INTERFACE_CCP2B_PHY2:
> +		if (ccp2_strobe)
> +			mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_STROBE;
> +		else
> +			mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_CLOCK;
> +	}
> +
> +	cam_phy_ctrl &=
> +		~(OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_MASK << shift);
> +	cam_phy_ctrl |= mode << shift;
> +
> +	isp_reg_writel(phy->isp, cam_phy_ctrl,
> +		       OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL, 0);
> +}
> +
> +static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool
> on,
> +				    bool ccp2_strobe)
> +{
> +	uint32_t csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
> +		| OMAP343X_CONTROL_CSIRXFE_RESET;
> +
> +	/* Nothing to configure here. */
> +	if (iface == ISP_INTERFACE_CSI2A_PHY2)
> +		return;
> +
> +	if (iface != ISP_INTERFACE_CCP2B_PHY1)
> +		return;
> +
> +	if (!on) {
> +		isp_reg_writel(phy->isp, 0,
> +			       OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE, 0);
> +		return;
> +	}
> +
> +	if (ccp2_strobe)
> +		csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> +
> +	isp_reg_writel(phy->isp, csirxfe,
> +		       OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE, 0);
> +}
> +
> +static void csiphy_routing_cfg(struct isp_csiphy *phy, u32 iface, bool on,
> +			       bool ccp2_strobe)
> +{
> +	if (phy->isp->mmio_base[OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL]
> +	    && on)
> +		return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
> +	if (phy->isp->mmio_base[OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE])
> +		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe);
> +}
> +
>  /*
>   * csiphy_lanes_config - Configuration of CSIPHY lanes.
>   *
> diff --git a/drivers/media/platform/omap3isp/ispreg.h
> b/drivers/media/platform/omap3isp/ispreg.h index 084ea77..66d2b42 100644
> --- a/drivers/media/platform/omap3isp/ispreg.h
> +++ b/drivers/media/platform/omap3isp/ispreg.h
> @@ -1583,4 +1583,26 @@
>  #define ISPCSIPHY_REG2_CCP2_SYNC_PATTERN_MASK		\
>  	(0x7fffff << ISPCSIPHY_REG2_CCP2_SYNC_PATTERN_SHIFT)
> 
> +/*
> ---------------------------------------------------------------------------
> -- + * CONTROL registers for CSI-2 phy routing
> + */
> +
> +/* OMAP343X_CONTROL_CSIRXFE */
> +#define OMAP343X_CONTROL_CSIRXFE_CSIB_INV	(1 << 7)
> +#define OMAP343X_CONTROL_CSIRXFE_RESENABLE	(1 << 8)
> +#define OMAP343X_CONTROL_CSIRXFE_SELFORM	(1 << 10)
> +#define OMAP343X_CONTROL_CSIRXFE_PWRDNZ		(1 << 12)
> +#define OMAP343X_CONTROL_CSIRXFE_RESET		(1 << 13)
> +
> +/* OMAP3630_CONTROL_CAMERA_PHY_CTRL */
> +#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT	2
> +#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT	0
> +#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY		0x0
> +#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_STROBE 0x1
> +#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_CLOCK 0x2
> +#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_GPI		0x3
> +#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_MASK		0x3
> +/* CCP2B: set to receive data from PHY2 instead of PHY1 */
> +#define OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2	(1 << 4)
> +
>  #endif	/* OMAP3_ISP_REG_H */
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/3] omap3isp: Add PHY routing configuration

[Sakari Ailus]

Hi, Laurent!

Thanks for the comments!

On Tue, Oct 09, 2012 at 02:17:48AM +0200, Laurent Pinchart wrote:
...
> > @@ -32,6 +32,92 @@
> >  #include "ispreg.h"
> >  #include "ispcsiphy.h"
> > 
> > +static void csiphy_routing_cfg_3630(struct isp_csiphy *phy, u32 iface,
> > +				    bool ccp2_strobe)
> > +{
> > +	u32 cam_phy_ctrl =
> 
> If you call the variable "value" or "ctrl" two statements below could fit on 
> one line, but that's up to you :-)

I'll call it "reg". :)


> > +		isp_reg_readl(phy->isp,
> > +			      OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL, 0);
> > +	u32 shift, mode;
> > +
> > +	switch (iface) {
> > +	case ISP_INTERFACE_CCP2B_PHY1:
> > +		cam_phy_ctrl &=
> > +			~OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
> > +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
> > +		break;
> > +	case ISP_INTERFACE_CSI2C_PHY1:
> > +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
> > +		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
> > +		break;
> > +	case ISP_INTERFACE_CCP2B_PHY2:
> > +		cam_phy_ctrl |=
> > +			OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
> > +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
> > +		break;
> > +	case ISP_INTERFACE_CSI2A_PHY2:
> > +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
> > +		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
> > +		break;
> > +	default:
> > +		pr_warn("bad iface %d\n", iface);
> 
> As you already know, dev_warn() is a better idea. Can this actually happen ?

I don't think so; it's checked in isp.c in isp_register_entities().. I'll
remove it.

Cheers.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[PATCH v3 3/3] omap3isp: Configure CSI-2 phy based on platform data

[Sakari Ailus]

Configure CSI-2 phy based on platform data in the ISP driver. For that, the
new V4L2_CID_IMAGE_SOURCE_PIXEL_RATE control is used. Previously the same
was configured from the board code.

This patch is dependent on "omap3: Provide means for changing CSI2 PHY
configuration".

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/isp.h       |    3 -
 drivers/media/platform/omap3isp/ispcsiphy.c |  140 +++++++++++++-------------
 drivers/media/platform/omap3isp/ispcsiphy.h |   10 --
 3 files changed, 70 insertions(+), 83 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 6fed222..accb3b0 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -129,9 +129,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);
 };
 
 /*
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index f13bfbd..4ac1081 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -119,36 +119,6 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy, u32 iface, bool on,
 }
 
 /*
- * csiphy_lanes_config - Configuration of CSIPHY lanes.
- *
- * Updates HW configuration.
- * Called with phy->mutex taken.
- */
-static void csiphy_lanes_config(struct isp_csiphy *phy)
-{
-	unsigned int i;
-	u32 reg;
-
-	reg = isp_reg_readl(phy->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
-
-	for (i = 0; i < phy->num_data_lanes; i++) {
-		reg &= ~(ISPCSI2_PHY_CFG_DATA_POL_MASK(i + 1) |
-			 ISPCSI2_PHY_CFG_DATA_POSITION_MASK(i + 1));
-		reg |= (phy->lanes.data[i].pol <<
-			ISPCSI2_PHY_CFG_DATA_POL_SHIFT(i + 1));
-		reg |= (phy->lanes.data[i].pos <<
-			ISPCSI2_PHY_CFG_DATA_POSITION_SHIFT(i + 1));
-	}
-
-	reg &= ~(ISPCSI2_PHY_CFG_CLOCK_POL_MASK |
-		 ISPCSI2_PHY_CFG_CLOCK_POSITION_MASK);
-	reg |= phy->lanes.clk.pol << ISPCSI2_PHY_CFG_CLOCK_POL_SHIFT;
-	reg |= phy->lanes.clk.pos << ISPCSI2_PHY_CFG_CLOCK_POSITION_SHIFT;
-
-	isp_reg_writel(phy->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
-}
-
-/*
  * csiphy_power_autoswitch_enable
  * @enable: Sets or clears the autoswitch function enable flag.
  */
@@ -193,43 +163,28 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 }
 
 /*
- * csiphy_dphy_config - Configure CSI2 D-PHY parameters.
- *
- * Called with phy->mutex taken.
+ * TCLK values are OK at their reset values
  */
-static void csiphy_dphy_config(struct isp_csiphy *phy)
-{
-	u32 reg;
-
-	/* Set up ISPCSIPHY_REG0 */
-	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG0);
-
-	reg &= ~(ISPCSIPHY_REG0_THS_TERM_MASK |
-		 ISPCSIPHY_REG0_THS_SETTLE_MASK);
-	reg |= phy->dphy.ths_term << ISPCSIPHY_REG0_THS_TERM_SHIFT;
-	reg |= phy->dphy.ths_settle << ISPCSIPHY_REG0_THS_SETTLE_SHIFT;
-
-	isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
-
-	/* Set up ISPCSIPHY_REG1 */
-	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG1);
-
-	reg &= ~(ISPCSIPHY_REG1_TCLK_TERM_MASK |
-		 ISPCSIPHY_REG1_TCLK_MISS_MASK |
-		 ISPCSIPHY_REG1_TCLK_SETTLE_MASK);
-	reg |= phy->dphy.tclk_term << ISPCSIPHY_REG1_TCLK_TERM_SHIFT;
-	reg |= phy->dphy.tclk_miss << ISPCSIPHY_REG1_TCLK_MISS_SHIFT;
-	reg |= phy->dphy.tclk_settle << ISPCSIPHY_REG1_TCLK_SETTLE_SHIFT;
+#define TCLK_TERM	0
+#define TCLK_MISS	1
+#define TCLK_SETTLE	14
 
-	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)
+static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 {
+	struct isp_csi2_device *csi2 = phy->csi2;
+	struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
+	struct isp_v4l2_subdevs_group *subdevs = pipe->external->host_priv;
+	struct isp_csiphy_lanes_cfg *lanes;
+	int csi2_ddrclk_khz;
 	unsigned int used_lanes = 0;
 	unsigned int i;
+	u32 reg;
+
+	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
+	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
+		lanes = &subdevs->bus.ccp2.lanecfg;
+	else
+		lanes = &subdevs->bus.csi2.lanecfg;
 
 	/* Clock and data lanes verification */
 	for (i = 0; i < phy->num_data_lanes; i++) {
@@ -248,10 +203,56 @@ 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);
+	csiphy_routing_cfg(phy, subdevs->interface, true,
+			   subdevs->bus.ccp2.phy_layer);
+
+	/* DPHY timing configuration */
+	/* CSI-2 is DDR and we only count used lanes. */
+	csi2_ddrclk_khz = pipe->external_rate / 1000
+		/ (2 * hweight32(used_lanes)) * pipe->external_width;
+
+	reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG0);
+
+	reg &= ~(ISPCSIPHY_REG0_THS_TERM_MASK |
+		 ISPCSIPHY_REG0_THS_SETTLE_MASK);
+	/* THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1. */
+	reg |= (DIV_ROUND_UP(25 * csi2_ddrclk_khz, 2000000) - 1)
+		<< ISPCSIPHY_REG0_THS_TERM_SHIFT;
+	/* THS_SETTLE: Programmed value = ceil(90 ns/DDRClk period) + 3. */
+	reg |= (DIV_ROUND_UP(90 * csi2_ddrclk_khz, 1000000) + 3)
+		<< ISPCSIPHY_REG0_THS_SETTLE_SHIFT;
+
+	isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
+
+	reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG1);
+
+	reg &= ~(ISPCSIPHY_REG1_TCLK_TERM_MASK |
+		 ISPCSIPHY_REG1_TCLK_MISS_MASK |
+		 ISPCSIPHY_REG1_TCLK_SETTLE_MASK);
+	reg |= TCLK_TERM << ISPCSIPHY_REG1_TCLK_TERM_SHIFT;
+	reg |= TCLK_MISS << ISPCSIPHY_REG1_TCLK_MISS_SHIFT;
+	reg |= TCLK_SETTLE << ISPCSIPHY_REG1_TCLK_SETTLE_SHIFT;
+
+	isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
+
+	/* DPHY lane configuration */
+	reg = isp_reg_readl(csi2->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
+
+	for (i = 0; i < phy->num_data_lanes; i++) {
+		reg &= ~(ISPCSI2_PHY_CFG_DATA_POL_MASK(i + 1) |
+			 ISPCSI2_PHY_CFG_DATA_POSITION_MASK(i + 1));
+		reg |= (lanes->data[i].pol <<
+			ISPCSI2_PHY_CFG_DATA_POL_SHIFT(i + 1));
+		reg |= (lanes->data[i].pos <<
+			ISPCSI2_PHY_CFG_DATA_POSITION_SHIFT(i + 1));
+	}
+
+	reg &= ~(ISPCSI2_PHY_CFG_CLOCK_POL_MASK |
+		 ISPCSI2_PHY_CFG_CLOCK_POSITION_MASK);
+	reg |= lanes->clk.pol << ISPCSI2_PHY_CFG_CLOCK_POL_SHIFT;
+	reg |= lanes->clk.pos << ISPCSI2_PHY_CFG_CLOCK_POSITION_SHIFT;
+
+	isp_reg_writel(csi2->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
 
 	return 0;
 }
@@ -276,8 +277,9 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 	if (rval < 0)
 		goto done;
 
-	csiphy_dphy_config(phy);
-	csiphy_lanes_config(phy);
+	rval = omap3isp_csiphy_config(phy);
+	if (rval < 0)
+		goto done;
 
 	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
 	if (rval) {
@@ -313,8 +315,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/platform/omap3isp/ispcsiphy.h b/drivers/media/platform/omap3isp/ispcsiphy.h
index e93a661..14551fd 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.h
+++ b/drivers/media/platform/omap3isp/ispcsiphy.h
@@ -32,14 +32,6 @@
 struct isp_csi2_device;
 struct regulator;
 
-struct isp_csiphy_dphy_cfg {
-	u8 ths_term;
-	u8 ths_settle;
-	u8 tclk_term;
-	unsigned tclk_miss:1;
-	u8 tclk_settle;
-};
-
 struct isp_csiphy {
 	struct isp_device *isp;
 	struct mutex mutex;	/* serialize csiphy configuration */
@@ -52,8 +44,6 @@ struct isp_csiphy {
 	unsigned int phy_regs;
 
 	u8 num_data_lanes;	/* number of CSI2 Data Lanes supported */
-	struct isp_csiphy_lanes_cfg lanes;
-	struct isp_csiphy_dphy_cfg dphy;
 };
 
 int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
-- 
1.7.2.5

Re: [PATCH v3 3/3] omap3isp: Configure CSI-2 phy based on platform data

[Laurent Pinchart]

On Sunday 07 October 2012 23:07:52 Sakari Ailus wrote:
> Configure CSI-2 phy based on platform data in the ISP driver. For that, the
> new V4L2_CID_IMAGE_SOURCE_PIXEL_RATE control is used. Previously the same
> was configured from the board code.
> 
> This patch is dependent on "omap3: Provide means for changing CSI2 PHY
> configuration".
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/omap3isp/isp.h       |    3 -
>  drivers/media/platform/omap3isp/ispcsiphy.c |  140 ++++++++++++------------
>  drivers/media/platform/omap3isp/ispcsiphy.h |   10 --
>  3 files changed, 70 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index 6fed222..accb3b0 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -129,9 +129,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);
>  };
> 
>  /*
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> b/drivers/media/platform/omap3isp/ispcsiphy.c index f13bfbd..4ac1081 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -119,36 +119,6 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
> u32 iface, bool on, }
> 
>  /*
> - * csiphy_lanes_config - Configuration of CSIPHY lanes.
> - *
> - * Updates HW configuration.
> - * Called with phy->mutex taken.
> - */
> -static void csiphy_lanes_config(struct isp_csiphy *phy)
> -{
> -	unsigned int i;
> -	u32 reg;
> -
> -	reg = isp_reg_readl(phy->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
> -
> -	for (i = 0; i < phy->num_data_lanes; i++) {
> -		reg &= ~(ISPCSI2_PHY_CFG_DATA_POL_MASK(i + 1) |
> -			 ISPCSI2_PHY_CFG_DATA_POSITION_MASK(i + 1));
> -		reg |= (phy->lanes.data[i].pol <<
> -			ISPCSI2_PHY_CFG_DATA_POL_SHIFT(i + 1));
> -		reg |= (phy->lanes.data[i].pos <<
> -			ISPCSI2_PHY_CFG_DATA_POSITION_SHIFT(i + 1));
> -	}
> -
> -	reg &= ~(ISPCSI2_PHY_CFG_CLOCK_POL_MASK |
> -		 ISPCSI2_PHY_CFG_CLOCK_POSITION_MASK);
> -	reg |= phy->lanes.clk.pol << ISPCSI2_PHY_CFG_CLOCK_POL_SHIFT;
> -	reg |= phy->lanes.clk.pos << ISPCSI2_PHY_CFG_CLOCK_POSITION_SHIFT;
> -
> -	isp_reg_writel(phy->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
> -}
> -
> -/*
>   * csiphy_power_autoswitch_enable
>   * @enable: Sets or clears the autoswitch function enable flag.
>   */
> @@ -193,43 +163,28 @@ static int csiphy_set_power(struct isp_csiphy *phy,
> u32 power) }
> 
>  /*
> - * csiphy_dphy_config - Configure CSI2 D-PHY parameters.
> - *
> - * Called with phy->mutex taken.
> + * TCLK values are OK at their reset values
>   */
> -static void csiphy_dphy_config(struct isp_csiphy *phy)
> -{
> -	u32 reg;
> -
> -	/* Set up ISPCSIPHY_REG0 */
> -	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG0);
> -
> -	reg &= ~(ISPCSIPHY_REG0_THS_TERM_MASK |
> -		 ISPCSIPHY_REG0_THS_SETTLE_MASK);
> -	reg |= phy->dphy.ths_term << ISPCSIPHY_REG0_THS_TERM_SHIFT;
> -	reg |= phy->dphy.ths_settle << ISPCSIPHY_REG0_THS_SETTLE_SHIFT;
> -
> -	isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
> -
> -	/* Set up ISPCSIPHY_REG1 */
> -	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG1);
> -
> -	reg &= ~(ISPCSIPHY_REG1_TCLK_TERM_MASK |
> -		 ISPCSIPHY_REG1_TCLK_MISS_MASK |
> -		 ISPCSIPHY_REG1_TCLK_SETTLE_MASK);
> -	reg |= phy->dphy.tclk_term << ISPCSIPHY_REG1_TCLK_TERM_SHIFT;
> -	reg |= phy->dphy.tclk_miss << ISPCSIPHY_REG1_TCLK_MISS_SHIFT;
> -	reg |= phy->dphy.tclk_settle << ISPCSIPHY_REG1_TCLK_SETTLE_SHIFT;
> +#define TCLK_TERM	0
> +#define TCLK_MISS	1
> +#define TCLK_SETTLE	14
> 
> -	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)
> +static int omap3isp_csiphy_config(struct isp_csiphy *phy)
>  {
> +	struct isp_csi2_device *csi2 = phy->csi2;
> +	struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
> +	struct isp_v4l2_subdevs_group *subdevs = pipe->external->host_priv;
> +	struct isp_csiphy_lanes_cfg *lanes;
> +	int csi2_ddrclk_khz;
>  	unsigned int used_lanes = 0;
>  	unsigned int i;
> +	u32 reg;
> +
> +	if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
> +	    || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
> +		lanes = &subdevs->bus.ccp2.lanecfg;
> +	else
> +		lanes = &subdevs->bus.csi2.lanecfg;
> 
>  	/* Clock and data lanes verification */
>  	for (i = 0; i < phy->num_data_lanes; i++) {
> @@ -248,10 +203,56 @@ 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);
> +	csiphy_routing_cfg(phy, subdevs->interface, true,
> +			   subdevs->bus.ccp2.phy_layer);

As the second argument is always true, it could make sense to remove it (or to 
add another call to csiphy_routing_cfg with on set to false).

> +
> +	/* DPHY timing configuration */
> +	/* CSI-2 is DDR and we only count used lanes. */
> +	csi2_ddrclk_khz = pipe->external_rate / 1000
> +		/ (2 * hweight32(used_lanes)) * pipe->external_width;
> +
> +	reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG0);
> +
> +	reg &= ~(ISPCSIPHY_REG0_THS_TERM_MASK |
> +		 ISPCSIPHY_REG0_THS_SETTLE_MASK);
> +	/* THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1. */
> +	reg |= (DIV_ROUND_UP(25 * csi2_ddrclk_khz, 2000000) - 1)
> +		<< ISPCSIPHY_REG0_THS_TERM_SHIFT;
> +	/* THS_SETTLE: Programmed value = ceil(90 ns/DDRClk period) + 3. */
> +	reg |= (DIV_ROUND_UP(90 * csi2_ddrclk_khz, 1000000) + 3)
> +		<< ISPCSIPHY_REG0_THS_SETTLE_SHIFT;
> +
> +	isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
> +
> +	reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG1);
> +
> +	reg &= ~(ISPCSIPHY_REG1_TCLK_TERM_MASK |
> +		 ISPCSIPHY_REG1_TCLK_MISS_MASK |
> +		 ISPCSIPHY_REG1_TCLK_SETTLE_MASK);
> +	reg |= TCLK_TERM << ISPCSIPHY_REG1_TCLK_TERM_SHIFT;
> +	reg |= TCLK_MISS << ISPCSIPHY_REG1_TCLK_MISS_SHIFT;
> +	reg |= TCLK_SETTLE << ISPCSIPHY_REG1_TCLK_SETTLE_SHIFT;
> +
> +	isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
> +
> +	/* DPHY lane configuration */
> +	reg = isp_reg_readl(csi2->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
> +
> +	for (i = 0; i < phy->num_data_lanes; i++) {
> +		reg &= ~(ISPCSI2_PHY_CFG_DATA_POL_MASK(i + 1) |
> +			 ISPCSI2_PHY_CFG_DATA_POSITION_MASK(i + 1));
> +		reg |= (lanes->data[i].pol <<
> +			ISPCSI2_PHY_CFG_DATA_POL_SHIFT(i + 1));
> +		reg |= (lanes->data[i].pos <<
> +			ISPCSI2_PHY_CFG_DATA_POSITION_SHIFT(i + 1));
> +	}
> +
> +	reg &= ~(ISPCSI2_PHY_CFG_CLOCK_POL_MASK |
> +		 ISPCSI2_PHY_CFG_CLOCK_POSITION_MASK);
> +	reg |= lanes->clk.pol << ISPCSI2_PHY_CFG_CLOCK_POL_SHIFT;
> +	reg |= lanes->clk.pos << ISPCSI2_PHY_CFG_CLOCK_POSITION_SHIFT;
> +
> +	isp_reg_writel(csi2->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
> 
>  	return 0;
>  }
> @@ -276,8 +277,9 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
>  	if (rval < 0)
>  		goto done;
> 
> -	csiphy_dphy_config(phy);
> -	csiphy_lanes_config(phy);
> +	rval = omap3isp_csiphy_config(phy);
> +	if (rval < 0)
> +		goto done;
> 
>  	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
>  	if (rval) {
> @@ -313,8 +315,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/platform/omap3isp/ispcsiphy.h
> b/drivers/media/platform/omap3isp/ispcsiphy.h index e93a661..14551fd 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.h
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.h
> @@ -32,14 +32,6 @@
>  struct isp_csi2_device;
>  struct regulator;
> 
> -struct isp_csiphy_dphy_cfg {
> -	u8 ths_term;
> -	u8 ths_settle;
> -	u8 tclk_term;
> -	unsigned tclk_miss:1;
> -	u8 tclk_settle;
> -};
> -
>  struct isp_csiphy {
>  	struct isp_device *isp;
>  	struct mutex mutex;	/* serialize csiphy configuration */
> @@ -52,8 +44,6 @@ struct isp_csiphy {
>  	unsigned int phy_regs;
> 
>  	u8 num_data_lanes;	/* number of CSI2 Data Lanes supported */
> -	struct isp_csiphy_lanes_cfg lanes;
> -	struct isp_csiphy_dphy_cfg dphy;
>  };
> 
>  int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/3] omap3isp: Configure CSI-2 phy based on platform data

[Sakari Ailus]

Hi Laurent,

Thanks for the comments.

On Tue, Oct 09, 2012 at 02:33:28AM +0200, Laurent Pinchart wrote:
...
> > @@ -248,10 +203,56 @@ 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);
> > +	csiphy_routing_cfg(phy, subdevs->interface, true,
> > +			   subdevs->bus.ccp2.phy_layer);
> 
> As the second argument is always true, it could make sense to remove it (or to 
> add another call to csiphy_routing_cfg with on set to false).

That's a good point. In principle another call should be added for 3430 to
turn the PHY off. That's not being done on the N900.

I'll see where it could be added.

Regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk