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

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

[Sakari Ailus]

Hi Paul and Laurent,

This is an update to an old patchset for CSI-2 configuration for OMAP 3430
and 3630, and the corresponding patch to the ISP driver. Both have been
tested on the 3630 only so far.

Additional patches for the N9(50) camera support that mostly aren't yet
upstreamable, are available in rm696-016/002-devel branch here:

<URL:https://git.gitorious.org/omap3camera/mainline.git>

Kind regards,

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

[PATCH v2 1/2] omap3: Provide means for changing CSI2 PHY configuration

[Sakari Ailus]

The OMAP 3630 has configuration how the ISP CSI-2 PHY pins are connected to
the actual CSI-2 receivers outside the ISP itself. Allow changing this
configuration from the ISP driver.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 arch/arm/mach-omap2/control.c              |   86 ++++++++++++++++++++++++++++
 arch/arm/mach-omap2/control.h              |   15 +++++
 arch/arm/mach-omap2/include/mach/control.h |   13 ++++
 3 files changed, 114 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-omap2/include/mach/control.h

diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
index 3223b81..11bb900 100644
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -12,9 +12,12 @@
  */
 #undef DEBUG
 
+#include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
 
+#include <mach/control.h>
+
 #include <plat/hardware.h>
 #include <plat/sdrc.h>
 
@@ -607,4 +610,87 @@ int omap3_ctrl_save_padconf(void)
 	return 0;
 }
 
+static int omap3630_ctrl_csi2_phy_cfg(u32 phy, u32 flags)
+{
+	u32 cam_phy_ctrl =
+		omap_ctrl_readl(OMAP3630_CONTROL_CAMERA_PHY_CTRL);
+	u32 shift, mode;
+
+	switch (phy) {
+	case OMAP3_CTRL_CSI2_PHY1_CCP2B:
+		cam_phy_ctrl &= ~OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
+		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
+		break;
+	case OMAP3_CTRL_CSI2_PHY1_CSI2C:
+		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
+		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
+		break;
+	case OMAP3_CTRL_CSI2_PHY2_CCP2B:
+		cam_phy_ctrl |= OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
+		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
+		break;
+	case OMAP3_CTRL_CSI2_PHY2_CSI2A:
+		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
+		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
+		break;
+	default:
+		pr_warn("bad phy %d\n", phy);
+		return -EINVAL;
+	}
+
+	/* Select data/clock or data/strobe mode for CCP2 */
+	switch (phy) {
+	case OMAP3_CTRL_CSI2_PHY1_CCP2B:
+	case OMAP3_CTRL_CSI2_PHY2_CCP2B:
+		if (flags & OMAP3_CTRL_CSI2_CFG_CCP2_DATA_STROBE)
+			mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_STROBE;
+		else
+			mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_CLOCK;
+		break;
+	}
+
+	cam_phy_ctrl &= ~(OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_MASK << shift);
+	cam_phy_ctrl |= mode << shift;
+
+	omap_ctrl_writel(cam_phy_ctrl,
+			 OMAP3630_CONTROL_CAMERA_PHY_CTRL);
+
+	return 0;
+}
+
+static int omap3430_ctrl_csi2_phy_cfg(u32 phy, bool on, u32 flags)
+{
+	uint32_t csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
+		| OMAP343X_CONTROL_CSIRXFE_RESET;
+
+	/* Nothing to configure here. */
+	if (phy == OMAP3_CTRL_CSI2_PHY2_CSI2A)
+		return 0;
+
+	if (phy != OMAP3_CTRL_CSI2_PHY1_CCP2B)
+		return -EINVAL;
+
+	if (!on) {
+		omap_ctrl_writel(0, OMAP343X_CONTROL_CSIRXFE);
+		return 0;
+	}
+
+	if (flags & OMAP3_CTRL_CSI2_CFG_CCP2_DATA_STROBE)
+		csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+
+	omap_ctrl_writel(csirxfe, OMAP343X_CONTROL_CSIRXFE);
+
+	return 0;
+}
+
+int omap3_ctrl_csi2_phy_cfg(u32 phy, bool on, u32 flags)
+{
+	if (cpu_is_omap3630() && on)
+		return omap3630_ctrl_csi2_phy_cfg(phy, flags);
+	if (cpu_is_omap3430())
+		return omap3430_ctrl_csi2_phy_cfg(phy, on, flags);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(omap3_ctrl_csi2_phy_cfg);
+
 #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index b8cdc85..7b2ee5d 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -132,6 +132,11 @@
 #define OMAP343X_CONTROL_MEM_DFTRW1	(OMAP2_CONTROL_GENERAL + 0x000c)
 #define OMAP343X_CONTROL_DEVCONF1	(OMAP2_CONTROL_GENERAL + 0x0068)
 #define OMAP343X_CONTROL_CSIRXFE		(OMAP2_CONTROL_GENERAL + 0x006c)
+#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)
 #define OMAP343X_CONTROL_SEC_STATUS		(OMAP2_CONTROL_GENERAL + 0x0070)
 #define OMAP343X_CONTROL_SEC_ERR_STATUS		(OMAP2_CONTROL_GENERAL + 0x0074)
 #define OMAP343X_CONTROL_SEC_ERR_STATUS_DEBUG	(OMAP2_CONTROL_GENERAL + 0x0078)
@@ -189,6 +194,16 @@
 #define OMAP3630_CONTROL_FUSE_OPP50_VDD2        (OMAP2_CONTROL_GENERAL + 0x0128)
 #define OMAP3630_CONTROL_FUSE_OPP100_VDD2       (OMAP2_CONTROL_GENERAL + 0x012C)
 #define OMAP3630_CONTROL_CAMERA_PHY_CTRL	(OMAP2_CONTROL_GENERAL + 0x02f0)
+#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)
+
 
 /* OMAP44xx control efuse offsets */
 #define OMAP44XX_CONTROL_FUSE_IVA_OPP50		0x22C
diff --git a/arch/arm/mach-omap2/include/mach/control.h b/arch/arm/mach-omap2/include/mach/control.h
new file mode 100644
index 0000000..afd0bed
--- /dev/null
+++ b/arch/arm/mach-omap2/include/mach/control.h
@@ -0,0 +1,13 @@
+#ifndef __MACH_CONTROL_H__
+#define __MACH_CONTROL_H__
+
+#define OMAP3_CTRL_CSI2_PHY2_CSI2A	0
+#define OMAP3_CTRL_CSI2_PHY2_CCP2B	1
+#define OMAP3_CTRL_CSI2_PHY1_CCP2B	2
+#define OMAP3_CTRL_CSI2_PHY1_CSI2C	3
+
+#define OMAP3_CTRL_CSI2_CFG_CCP2_DATA_STROBE	(1 << 0)
+
+int omap3_ctrl_csi2_phy_cfg(u32 phy, bool on, u32 flags);
+
+#endif /* __MACH_CONTROL_H__ */
-- 
1.7.2.5

[PATCH v2 2/2] 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 |  161 +++++++++++++++------------
 drivers/media/platform/omap3isp/ispcsiphy.h |   10 --
 3 files changed, 90 insertions(+), 84 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 8be7487..a2f992c 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -127,9 +127,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 348f67e..1d16e66 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -28,41 +28,13 @@
 #include <linux/device.h>
 #include <linux/regulator/consumer.h>
 
+#include <mach/control.h>
+
 #include "isp.h"
 #include "ispreg.h"
 #include "ispcsiphy.h"
 
 /*
- * 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.
  */
@@ -107,46 +79,32 @@ 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;
-
-	isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
-}
+#define TCLK_TERM	0
+#define TCLK_MISS	1
+#define TCLK_SETTLE	14
 
-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;
+	unsigned int phy_num;
+	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++) {
+	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 +120,72 @@ 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);
+	switch (subdevs->interface) {
+	case ISP_INTERFACE_CSI2A_PHY2:
+		phy_num = OMAP3_CTRL_CSI2_PHY2_CSI2A;
+		break;
+	case ISP_INTERFACE_CSI2C_PHY1:
+		phy_num = OMAP3_CTRL_CSI2_PHY1_CSI2C;
+		break;
+	case ISP_INTERFACE_CCP2B_PHY1:
+		phy_num = OMAP3_CTRL_CSI2_PHY1_CCP2B;
+		break;
+	case ISP_INTERFACE_CCP2B_PHY2:
+		phy_num = OMAP3_CTRL_CSI2_PHY2_CCP2B;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	omap3_ctrl_csi2_phy_cfg(phy_num, true, 0);
+
+	/* 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, csi2->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, csi2->phy->phy_regs, ISPCSIPHY_REG0);
+
+	reg = isp_reg_readl(csi2->isp, csi2->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, csi2->phy->phy_regs, ISPCSIPHY_REG1);
+
+	/* DPHY lane configuration */
+	reg = isp_reg_readl(csi2->isp, csi2->phy->cfg_regs, ISPCSI2_PHY_CFG);
+
+	for (i = 0; i < csi2->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, csi2->phy->cfg_regs, ISPCSI2_PHY_CFG);
 
 	return 0;
 }
@@ -190,8 +210,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) {
@@ -227,8 +248,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 v2 2/2] omap3isp: Configure CSI-2 phy based on platform data

[Tony Lindgren]

Moi Sakari

* Sakari Ailus <sakari.ailus@iki.fi> [120926 14:51]:
> 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".

Can you please do one more patch to get rid of the last remaining
cpu_is_omapxxxx check in drivers/media/platform/omap3isp/isp.c?

That data should come from platform_data (or device tree) as
we going to make cpu_is_omap privat to mach-omap2.

Regards,

Tony

Re: [PATCH v2 1/2] omap3: Provide means for changing CSI2 PHY configuration

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Thursday 27 September 2012 00:50:35 Sakari Ailus wrote:
> The OMAP 3630 has configuration how the ISP CSI-2 PHY pins are connected to
> the actual CSI-2 receivers outside the ISP itself. Allow changing this
> configuration from the ISP driver.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

Just one small comment below, otherwise

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm/mach-omap2/control.c              |   86 +++++++++++++++++++++++++
>  arch/arm/mach-omap2/control.h              |   15 +++++
>  arch/arm/mach-omap2/include/mach/control.h |   13 ++++
>  3 files changed, 114 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-omap2/include/mach/control.h
> 
> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
> index 3223b81..11bb900 100644
> --- a/arch/arm/mach-omap2/control.c
> +++ b/arch/arm/mach-omap2/control.c
> @@ -12,9 +12,12 @@
>   */
>  #undef DEBUG
> 
> +#include <linux/export.h>
>  #include <linux/kernel.h>
>  #include <linux/io.h>
> 
> +#include <mach/control.h>
> +
>  #include <plat/hardware.h>
>  #include <plat/sdrc.h>
> 
> @@ -607,4 +610,87 @@ int omap3_ctrl_save_padconf(void)
>  	return 0;
>  }
> 
> +static int omap3630_ctrl_csi2_phy_cfg(u32 phy, u32 flags)
> +{
> +	u32 cam_phy_ctrl =
> +		omap_ctrl_readl(OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> +	u32 shift, mode;
> +
> +	switch (phy) {
> +	case OMAP3_CTRL_CSI2_PHY1_CCP2B:
> +		cam_phy_ctrl &= ~OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
> +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
> +		break;
> +	case OMAP3_CTRL_CSI2_PHY1_CSI2C:
> +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY1_SHIFT;
> +		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
> +		break;
> +	case OMAP3_CTRL_CSI2_PHY2_CCP2B:
> +		cam_phy_ctrl |= OMAP3630_CONTROL_CAMERA_PHY_CTRL_CSI1_RX_SEL_PHY2;
> +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
> +		break;
> +	case OMAP3_CTRL_CSI2_PHY2_CSI2A:
> +		shift = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_PHY2_SHIFT;
> +		mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_DPHY;
> +		break;
> +	default:
> +		pr_warn("bad phy %d\n", phy);
> +		return -EINVAL;
> +	}
> +
> +	/* Select data/clock or data/strobe mode for CCP2 */
> +	switch (phy) {
> +	case OMAP3_CTRL_CSI2_PHY1_CCP2B:
> +	case OMAP3_CTRL_CSI2_PHY2_CCP2B:
> +		if (flags & OMAP3_CTRL_CSI2_CFG_CCP2_DATA_STROBE)
> +			mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_STROBE;
> +		else
> +			mode = OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_CCP2_DATA_CLOCK;
> +		break;
> +	}
> +
> +	cam_phy_ctrl &= ~(OMAP3630_CONTROL_CAMERA_PHY_CTRL_CAMMODE_MASK << 
shift);
> +	cam_phy_ctrl |= mode << shift;
> +
> +	omap_ctrl_writel(cam_phy_ctrl,
> +			 OMAP3630_CONTROL_CAMERA_PHY_CTRL);

This can fit on one line.

> +
> +	return 0;
> +}
> +
> +static int omap3430_ctrl_csi2_phy_cfg(u32 phy, bool on, u32 flags)
> +{
> +	uint32_t csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
> +		| OMAP343X_CONTROL_CSIRXFE_RESET;
> +
> +	/* Nothing to configure here. */
> +	if (phy == OMAP3_CTRL_CSI2_PHY2_CSI2A)
> +		return 0;
> +
> +	if (phy != OMAP3_CTRL_CSI2_PHY1_CCP2B)
> +		return -EINVAL;
> +
> +	if (!on) {
> +		omap_ctrl_writel(0, OMAP343X_CONTROL_CSIRXFE);
> +		return 0;
> +	}
> +
> +	if (flags & OMAP3_CTRL_CSI2_CFG_CCP2_DATA_STROBE)
> +		csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> +
> +	omap_ctrl_writel(csirxfe, OMAP343X_CONTROL_CSIRXFE);
> +
> +	return 0;
> +}
> +
> +int omap3_ctrl_csi2_phy_cfg(u32 phy, bool on, u32 flags)
> +{
> +	if (cpu_is_omap3630() && on)
> +		return omap3630_ctrl_csi2_phy_cfg(phy, flags);
> +	if (cpu_is_omap3430())
> +		return omap3430_ctrl_csi2_phy_cfg(phy, on, flags);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(omap3_ctrl_csi2_phy_cfg);
> +
>  #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
> index b8cdc85..7b2ee5d 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -132,6 +132,11 @@
>  #define OMAP343X_CONTROL_MEM_DFTRW1	(OMAP2_CONTROL_GENERAL + 0x000c)
>  #define OMAP343X_CONTROL_DEVCONF1	(OMAP2_CONTROL_GENERAL + 0x0068)
>  #define OMAP343X_CONTROL_CSIRXFE		(OMAP2_CONTROL_GENERAL + 0x006c)
> +#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)
>  #define OMAP343X_CONTROL_SEC_STATUS		(OMAP2_CONTROL_GENERAL + 0x0070)
>  #define OMAP343X_CONTROL_SEC_ERR_STATUS		(OMAP2_CONTROL_GENERAL + 
0x0074)
>  #define OMAP343X_CONTROL_SEC_ERR_STATUS_DEBUG	(OMAP2_CONTROL_GENERAL +
> 0x0078)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Thursday 27 September 2012 00:50:36 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 |  161 +++++++++++++-----------
>  drivers/media/platform/omap3isp/ispcsiphy.h |   10 --
>  3 files changed, 90 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index 8be7487..a2f992c 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -127,9 +127,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 348f67e..1d16e66 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -28,41 +28,13 @@
>  #include <linux/device.h>
>  #include <linux/regulator/consumer.h>
> 
> +#include <mach/control.h>
> +
>  #include "isp.h"
>  #include "ispreg.h"
>  #include "ispcsiphy.h"
> 
>  /*
> - * 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.
>   */
> @@ -107,46 +79,32 @@ 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;
> -
> -	isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
> -}
> +#define TCLK_TERM	0
> +#define TCLK_MISS	1
> +#define TCLK_SETTLE	14
> 
> -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;
> +	unsigned int phy_num;
> +	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++) {
> +	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 +120,72 @@ 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);
> +	switch (subdevs->interface) {
> +	case ISP_INTERFACE_CSI2A_PHY2:
> +		phy_num = OMAP3_CTRL_CSI2_PHY2_CSI2A;
> +		break;
> +	case ISP_INTERFACE_CSI2C_PHY1:
> +		phy_num = OMAP3_CTRL_CSI2_PHY1_CSI2C;
> +		break;
> +	case ISP_INTERFACE_CCP2B_PHY1:
> +		phy_num = OMAP3_CTRL_CSI2_PHY1_CCP2B;
> +		break;
> +	case ISP_INTERFACE_CCP2B_PHY2:
> +		phy_num = OMAP3_CTRL_CSI2_PHY2_CCP2B;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	omap3_ctrl_csi2_phy_cfg(phy_num, true, 0);
> +
> +	/* 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;

Board code previously used op_sys_clk_freq_hz / 1000 / (2 * 
hweight32(used_lanes)). Looking at the SMIA++ PLL code, pipe->external_rate is 
equal to op_sys_clk_freq_hz / bits_per_pixel * lane_op_clock_ratio. Both 
values are identical if lane_op_clock_ratio is set to 1, which is the case if 
the SMIAPP_QUIRK_FLAG_OP_PIX_CLOCK_PER_LANE quirk is not set. Have you 
verified that the new CSI2 DDR clock frequency calculation is correct when the 
quirk is set ?

> +	reg = isp_reg_readl(csi2->isp, csi2->phy->phy_regs, ISPCSIPHY_REG0);

Isn't csi2->phy == phy ? You could then just write

	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG0);

and similarly below.

> +	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, csi2->phy->phy_regs, ISPCSIPHY_REG0);
> +
> +	reg = isp_reg_readl(csi2->isp, csi2->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, csi2->phy->phy_regs, ISPCSIPHY_REG1);
> +
> +	/* DPHY lane configuration */
> +	reg = isp_reg_readl(csi2->isp, csi2->phy->cfg_regs, ISPCSI2_PHY_CFG);
> +
> +	for (i = 0; i < csi2->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, csi2->phy->cfg_regs, ISPCSI2_PHY_CFG);
> 
>  	return 0;
>  }
> @@ -190,8 +210,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) {
> @@ -227,8 +248,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 v2 2/2] omap3isp: Configure CSI-2 phy based on platform data

[Laurent Pinchart]

Hi Tony,

On Wednesday 26 September 2012 15:00:19 Tony Lindgren wrote:
> Moi Sakari
> 
> * Sakari Ailus <sakari.ailus@iki.fi> [120926 14:51]:
> > 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".
> 
> Can you please do one more patch to get rid of the last remaining
> cpu_is_omapxxxx check in drivers/media/platform/omap3isp/isp.c?

I'm working on that, I'll submit a patch.

> That data should come from platform_data (or device tree) as
> we going to make cpu_is_omap privat to mach-omap2.

-- 
Regards,

Laurent Pinchart

[PATCH] omap3isp: Replace cpu_is_omap3630() with ISP revision check

[Laurent Pinchart]

Drivers must not rely on cpu_is_omap* macros (they will soon become
private). Use the ISP revision instead to identify the hardware.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/isp.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index d7aa513..6034dca 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1345,10 +1345,7 @@ static int isp_enable_clocks(struct isp_device *isp)
 	 * has to be twice of what is set on OMAP3430 to get
 	 * the required value for cam_mclk
 	 */
-	if (cpu_is_omap3630())
-		divisor = 1;
-	else
-		divisor = 2;
+	divisor = isp->revision == ISP_REVISION_15_0 ? 1 : 2;
 
 	r = clk_enable(isp->clock[ISP_CLK_CAM_ICK]);
 	if (r) {
@@ -2093,7 +2090,11 @@ static int __devinit isp_probe(struct platform_device *pdev)
 	isp->isp_csiphy1.vdd = regulator_get(&pdev->dev, "VDD_CSIPHY1");
 	isp->isp_csiphy2.vdd = regulator_get(&pdev->dev, "VDD_CSIPHY2");
 
-	/* Clocks */
+	/* Clocks
+	 *
+	 * The ISP clock tree is revision-dependent. We thus need to enable ICLK
+	 * manually to read the revision before calling __omap3isp_get().
+	 */
 	ret = isp_map_mem_resource(pdev, isp, OMAP3_ISP_IOMEM_MAIN);
 	if (ret < 0)
 		goto error;
@@ -2102,6 +2103,16 @@ static int __devinit isp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error;
 
+	ret = clk_enable(isp->clock[ISP_CLK_CAM_ICK]);
+	if (ret < 0)
+		goto error;
+
+	isp->revision = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_REVISION);
+	dev_info(isp->dev, "Revision %d.%d found\n",
+		 (isp->revision & 0xf0) >> 4, isp->revision & 0x0f);
+
+	clk_disable(isp->clock[ISP_CLK_CAM_ICK]);
+
 	if (__omap3isp_get(isp, false) == NULL) {
 		ret = -ENODEV;
 		goto error;
@@ -2112,10 +2123,6 @@ static int __devinit isp_probe(struct platform_device *pdev)
 		goto error_isp;
 
 	/* Memory resources */
-	isp->revision = isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_REVISION);
-	dev_info(isp->dev, "Revision %d.%d found\n",
-		 (isp->revision & 0xf0) >> 4, isp->revision & 0x0f);
-
 	for (m = 0; m < ARRAY_SIZE(isp_res_maps); m++)
 		if (isp->revision == isp_res_maps[m].isp_rev)
 			break;
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] omap3: Provide means for changing CSI2 PHY configuration

[Sakari Ailus]

Hi Laurent,

Thanks for the review!

Laurent Pinchart wrote:
> Hi Sakari,
>
> Thanks for the patch.
>
> On Thursday 27 September 2012 00:50:35 Sakari Ailus wrote:
>> The OMAP 3630 has configuration how the ISP CSI-2 PHY pins are connected to
>> the actual CSI-2 receivers outside the ISP itself. Allow changing this
>> configuration from the ISP driver.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>
> Just one small comment below, otherwise
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> ---
>>   arch/arm/mach-omap2/control.c              |   86 +++++++++++++++++++++++++
>>   arch/arm/mach-omap2/control.h              |   15 +++++
>>   arch/arm/mach-omap2/include/mach/control.h |   13 ++++
>>   3 files changed, 114 insertions(+), 0 deletions(-)
>>   create mode 100644 arch/arm/mach-omap2/include/mach/control.h
>>
>> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
>> index 3223b81..11bb900 100644
>> --- a/arch/arm/mach-omap2/control.c
>> +++ b/arch/arm/mach-omap2/control.c
...
>> +	cam_phy_ctrl |= mode << shift;
>> +
>> +	omap_ctrl_writel(cam_phy_ctrl,
>> +			 OMAP3630_CONTROL_CAMERA_PHY_CTRL);
>
> This can fit on one line.

I'll fix that for the next version. I noticed there were a few too long 
lines, too; I've broken them up where it makes sense, and removed a 
useless break statement.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

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

[Sakari Ailus]

Hi Laurent,

Thanks for the review.

Laurent Pinchart wrote:
> On Thursday 27 September 2012 00:50:36 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 |  161 +++++++++++++-----------
>>   drivers/media/platform/omap3isp/ispcsiphy.h |   10 --
>>   3 files changed, 90 insertions(+), 84 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
>> b/drivers/media/platform/omap3isp/ispcsiphy.c index 348f67e..1d16e66 100644
>> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
>> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
...
>> @@ -162,10 +120,72 @@ 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);
>> +	switch (subdevs->interface) {
>> +	case ISP_INTERFACE_CSI2A_PHY2:
>> +		phy_num = OMAP3_CTRL_CSI2_PHY2_CSI2A;
>> +		break;
>> +	case ISP_INTERFACE_CSI2C_PHY1:
>> +		phy_num = OMAP3_CTRL_CSI2_PHY1_CSI2C;
>> +		break;
>> +	case ISP_INTERFACE_CCP2B_PHY1:
>> +		phy_num = OMAP3_CTRL_CSI2_PHY1_CCP2B;
>> +		break;
>> +	case ISP_INTERFACE_CCP2B_PHY2:
>> +		phy_num = OMAP3_CTRL_CSI2_PHY2_CCP2B;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	omap3_ctrl_csi2_phy_cfg(phy_num, true, 0);
>> +
>> +	/* 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;
>
> Board code previously used op_sys_clk_freq_hz / 1000 / (2 *
> hweight32(used_lanes)). Looking at the SMIA++ PLL code, pipe->external_rate is
> equal to op_sys_clk_freq_hz / bits_per_pixel * lane_op_clock_ratio. Both
> values are identical if lane_op_clock_ratio is set to 1, which is the case if
> the SMIAPP_QUIRK_FLAG_OP_PIX_CLOCK_PER_LANE quirk is not set. Have you
> verified that the new CSI2 DDR clock frequency calculation is correct when the
> quirk is set ?

Good point. The presence of that quirk flag means that the bit rate (or 
clock) is per-lane, and not all lanes together as it should be. This is 
why the value is multiplied by the number of lanes. It should have no 
visibility outside the SMIA++ driver itself; if it does, then it's a bug 
in the SMIA++ driver.

>> +	reg = isp_reg_readl(csi2->isp, csi2->phy->phy_regs, ISPCSIPHY_REG0);
>
> Isn't csi2->phy == phy ? You could then just write
>
> 	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG0);
>
> and similarly below.

Fixed.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH] omap3isp: Replace cpu_is_omap3630() with ISP revision check

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [120927 07:38]:
> Drivers must not rely on cpu_is_omap* macros (they will soon become
> private). Use the ISP revision instead to identify the hardware.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Great, thanks for fixing this:

Acked-by: Tony Lindgren <tony@atomide.com>

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

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [120927 02:52]:
> Hi Tony,
> 
> On Wednesday 26 September 2012 15:00:19 Tony Lindgren wrote:
> > Moi Sakari
> > 
> > * Sakari Ailus <sakari.ailus@iki.fi> [120926 14:51]:
> > > 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".
> > 
> > Can you please do one more patch to get rid of the last remaining
> > cpu_is_omapxxxx check in drivers/media/platform/omap3isp/isp.c?
> 
> I'm working on that, I'll submit a patch.

Thanks acked it.

Regards,

Tony

Re: [PATCH v2 1/2] omap3: Provide means for changing CSI2 PHY configuration

[Kevin Hilman]

Hi Sakari,

Sakari Ailus <sakari.ailus@iki.fi> writes:

> The OMAP 3630 has configuration how the ISP CSI-2 PHY pins are connected to
> the actual CSI-2 receivers outside the ISP itself. Allow changing this
> configuration from the ISP driver.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

These control module registers (CSIRXFE, CAMERA_PHY_CTRL) are in the
CORE powerdomain, so they will be lost during off-mode transitions.  So,
I suspect you'll also want to add them to the save/restore functions in
control.c in order for this to work across off-mode transitions.

Kevin

Re: [PATCH v2 1/2] omap3: Provide means for changing CSI2 PHY configuration

[Sakari Ailus]

Hi Kevin,

Thanks for the comments!

On Tue, Oct 09, 2012 at 01:50:04PM -0700, Kevin Hilman wrote:
> Hi Sakari,
> 
> Sakari Ailus <sakari.ailus@iki.fi> writes:
> 
> > The OMAP 3630 has configuration how the ISP CSI-2 PHY pins are connected to
> > the actual CSI-2 receivers outside the ISP itself. Allow changing this
> > configuration from the ISP driver.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> These control module registers (CSIRXFE, CAMERA_PHY_CTRL) are in the
> CORE powerdomain, so they will be lost during off-mode transitions.  So,
> I suspect you'll also want to add them to the save/restore functions in
> control.c in order for this to work across off-mode transitions.

I've got another patch that implements this in the ISP driver instead.

<URL:http://www.spinics.net/lists/linux-media/msg54781.html>

The ISP also can't wake up the MPU from the off mode, so I don't think
losing the register contents is necessarily an issue. The registers will be
written to a new value whenever streaming is started. Perhaps adding a note
about that would be worthwhile.

Regards,

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

Re: [PATCH v2 1/2] omap3: Provide means for changing CSI2 PHY configuration

[Kevin Hilman]

Sakari Ailus <sakari.ailus@iki.fi> writes:

> Hi Kevin,
>
> Thanks for the comments!
>
> On Tue, Oct 09, 2012 at 01:50:04PM -0700, Kevin Hilman wrote:
>> Hi Sakari,
>> 
>> Sakari Ailus <sakari.ailus@iki.fi> writes:
>> 
>> > The OMAP 3630 has configuration how the ISP CSI-2 PHY pins are connected to
>> > the actual CSI-2 receivers outside the ISP itself. Allow changing this
>> > configuration from the ISP driver.
>> >
>> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>> 
>> These control module registers (CSIRXFE, CAMERA_PHY_CTRL) are in the
>> CORE powerdomain, so they will be lost during off-mode transitions.  So,
>> I suspect you'll also want to add them to the save/restore functions in
>> control.c in order for this to work across off-mode transitions.
>
> I've got another patch that implements this in the ISP driver instead.
>
> <URL:http://www.spinics.net/lists/linux-media/msg54781.html>

Oops, sorry.  I should've looked at v3. 

> The ISP also can't wake up the MPU from the off mode, so I don't think
> losing the register contents is necessarily an issue. The registers will be
> written to a new value whenever streaming is started. Perhaps adding a note
> about that would be worthwhile.

Yes, I suggest mentioning that these register are not saved/restored and
why would be helpful.

Kevin