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

[PATCH 1/1] 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>
---
Hi,

This patch does what was discussed some time ago: provide a bit more high
level interface than the registers for the ISP driver to change the CSI-2
PHY mappings.

omap_writel()/omap_readl() functions are no longer there so this works as a
convenient push to write a patch such as this. ;-)

 arch/arm/mach-omap2/control.c              |   32 ++++++++++++++++++++++++++++
 arch/arm/mach-omap2/include/mach/control.h |   11 +++++++++
 2 files changed, 43 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 08e674b..78c27b2 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>
 
@@ -564,4 +567,33 @@ int omap3_ctrl_save_padconf(void)
 	return 0;
 }
 
+void omap3_ctrl_csi2_select(u32 csi2cfg)
+{
+	/* FIXME: Do 34xx / 35xx require something here? */
+	if (cpu_is_omap3630()) {
+		u32 cam_phy_ctrl =
+			omap_ctrl_readl(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 (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY1_CCP2B)
+			cam_phy_ctrl |= 1 << 2;
+		else if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY1_CSI2C)
+			cam_phy_ctrl &= ~(1 << 2);
+
+		if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY2_CCP2B)
+			cam_phy_ctrl |= 1;
+		else if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY2_CSI2A)
+			cam_phy_ctrl &= ~1;
+
+		omap_ctrl_writel(cam_phy_ctrl,
+				 OMAP3630_CONTROL_CAMERA_PHY_CTRL);
+	}
+}
+EXPORT_SYMBOL_GPL(omap3_ctrl_csi2_select);
+
 #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
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..4490b7a
--- /dev/null
+++ b/arch/arm/mach-omap2/include/mach/control.h
@@ -0,0 +1,11 @@
+#ifndef __MACH_CONTROL_H__
+#define __MACH_CONTROL_H__
+
+#define OMAP3_CTRL_CSI2_SELECT_PHY1_CCP2B      (1 << 0)
+#define OMAP3_CTRL_CSI2_SELECT_PHY1_CSI2C      (1 << 1)
+#define OMAP3_CTRL_CSI2_SELECT_PHY2_CCP2B      (1 << 2)
+#define OMAP3_CTRL_CSI2_SELECT_PHY2_CSI2A      (1 << 3)
+
+void omap3_ctrl_csi2_select(u32 csi2cfg);
+
+#endif /* __MACH_CONTROL_H__ */
-- 
1.7.2.5

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

[Sakari Ailus]

Hi,

On Wed, May 09, 2012 at 08:00:41AM +0300, 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>

I just realised also this one is missing:

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

I wonder if we could have these in... :-)

Cheers,

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

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

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Wednesday 09 May 2012 08:00:41 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>
> ---
> Hi,
> 
> This patch does what was discussed some time ago: provide a bit more high
> level interface than the registers for the ISP driver to change the CSI-2
> PHY mappings.
> 
> omap_writel()/omap_readl() functions are no longer there so this works as a
> convenient push to write a patch such as this. ;-)
> 
>  arch/arm/mach-omap2/control.c              |   32 +++++++++++++++++++++++++
>  arch/arm/mach-omap2/include/mach/control.h |   11 +++++++++
>  2 files changed, 43 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 08e674b..78c27b2 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>
> 
> @@ -564,4 +567,33 @@ int omap3_ctrl_save_padconf(void)
>  	return 0;
>  }
> 
> +void omap3_ctrl_csi2_select(u32 csi2cfg)
> +{
> +	/* FIXME: Do 34xx / 35xx require something here? */

Well, maybe it's time to find out whether it does ?

> +	if (cpu_is_omap3630()) {

I would test !cpu_is_omap3630() and return, that would lower the indentation 
level.

> +		u32 cam_phy_ctrl =
> +			omap_ctrl_readl(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 (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY1_CCP2B)
> +			cam_phy_ctrl |= 1 << 2;
> +		else if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY1_CSI2C)
> +			cam_phy_ctrl &= ~(1 << 2);
> +
> +		if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY2_CCP2B)
> +			cam_phy_ctrl |= 1;
> +		else if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY2_CSI2A)
> +			cam_phy_ctrl &= ~1;
> +
> +		omap_ctrl_writel(cam_phy_ctrl,
> +				 OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(omap3_ctrl_csi2_select);
> +
>  #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
> 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..4490b7a
> --- /dev/null
> +++ b/arch/arm/mach-omap2/include/mach/control.h
> @@ -0,0 +1,11 @@
> +#ifndef __MACH_CONTROL_H__
> +#define __MACH_CONTROL_H__
> +
> +#define OMAP3_CTRL_CSI2_SELECT_PHY1_CCP2B      (1 << 0)
> +#define OMAP3_CTRL_CSI2_SELECT_PHY1_CSI2C      (1 << 1)
> +#define OMAP3_CTRL_CSI2_SELECT_PHY2_CCP2B      (1 << 2)
> +#define OMAP3_CTRL_CSI2_SELECT_PHY2_CSI2A      (1 << 3)

As you only test for equality, does a bitmask really make sense ?

> +void omap3_ctrl_csi2_select(u32 csi2cfg);
> +
> +#endif /* __MACH_CONTROL_H__ */
-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

Thanks for the review!

On Wed, May 09, 2012 at 01:01:34PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thanks for the patch.
> 
> On Wednesday 09 May 2012 08:00:41 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>
> > ---
> > Hi,
> > 
> > This patch does what was discussed some time ago: provide a bit more high
> > level interface than the registers for the ISP driver to change the CSI-2
> > PHY mappings.
> > 
> > omap_writel()/omap_readl() functions are no longer there so this works as a
> > convenient push to write a patch such as this. ;-)
> > 
> >  arch/arm/mach-omap2/control.c              |   32 +++++++++++++++++++++++++
> >  arch/arm/mach-omap2/include/mach/control.h |   11 +++++++++
> >  2 files changed, 43 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 08e674b..78c27b2 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>
> > 
> > @@ -564,4 +567,33 @@ int omap3_ctrl_save_padconf(void)
> >  	return 0;
> >  }
> > 
> > +void omap3_ctrl_csi2_select(u32 csi2cfg)
> > +{
> > +	/* FIXME: Do 34xx / 35xx require something here? */
> 
> Well, maybe it's time to find out whether it does ?

I'm not aware of anyone having a sensor with CSI-2 bus connected to a 3430;
that's why this hasn't been found out. If we add the support we're at least
going to be unable to test it. For that reason I was going to leave this
as-is. What do you think?

> > +	if (cpu_is_omap3630()) {
> 
> I would test !cpu_is_omap3630() and return, that would lower the indentation 
> level.

Now, yes. I'll do that if you agree with me on the previous question since
otherwise this won't work too well. :)

> > +		u32 cam_phy_ctrl =
> > +			omap_ctrl_readl(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 (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY1_CCP2B)
> > +			cam_phy_ctrl |= 1 << 2;
> > +		else if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY1_CSI2C)
> > +			cam_phy_ctrl &= ~(1 << 2);
> > +
> > +		if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY2_CCP2B)
> > +			cam_phy_ctrl |= 1;
> > +		else if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY2_CSI2A)
> > +			cam_phy_ctrl &= ~1;
> > +
> > +		omap_ctrl_writel(cam_phy_ctrl,
> > +				 OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(omap3_ctrl_csi2_select);
> > +
> >  #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
> > 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..4490b7a
> > --- /dev/null
> > +++ b/arch/arm/mach-omap2/include/mach/control.h
> > @@ -0,0 +1,11 @@
> > +#ifndef __MACH_CONTROL_H__
> > +#define __MACH_CONTROL_H__
> > +
> > +#define OMAP3_CTRL_CSI2_SELECT_PHY1_CCP2B      (1 << 0)
> > +#define OMAP3_CTRL_CSI2_SELECT_PHY1_CSI2C      (1 << 1)
> > +#define OMAP3_CTRL_CSI2_SELECT_PHY2_CCP2B      (1 << 2)
> > +#define OMAP3_CTRL_CSI2_SELECT_PHY2_CSI2A      (1 << 3)
> 
> As you only test for equality, does a bitmask really make sense ?

No. I'll change that.

Cheers,

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

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

[Laurent Pinchart]

Hi Sakari,

On Wednesday 09 May 2012 16:43:08 Sakari Ailus wrote:
> On Wed, May 09, 2012 at 01:01:34PM +0200, Laurent Pinchart wrote:
> > On Wednesday 09 May 2012 08:00:41 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>
> > > ---
> > > Hi,
> > > 
> > > This patch does what was discussed some time ago: provide a bit more
> > > high level interface than the registers for the ISP driver to change the
> > > CSI-2 PHY mappings.
> > > 
> > > omap_writel()/omap_readl() functions are no longer there so this works
> > > as a convenient push to write a patch such as this. ;-)
> > > 
> > >  arch/arm/mach-omap2/control.c              |   32 +++++++++++++++++++++
> > >  arch/arm/mach-omap2/include/mach/control.h |   11 +++++++++
> > >  2 files changed, 43 insertions(+), 0 deletions(-)
> > >  create mode 100644 arch/arm/mach-omap2/include/mach/control.h

[snip]

> > > +void omap3_ctrl_csi2_select(u32 csi2cfg)
> > > +{
> > > +	/* FIXME: Do 34xx / 35xx require something here? */
> > 
> > Well, maybe it's time to find out whether it does ?
> 
> I'm not aware of anyone having a sensor with CSI-2 bus connected to a 3430;
> that's why this hasn't been found out. If we add the support we're at least
> going to be unable to test it. For that reason I was going to leave this
> as-is. What do you think?

I haven't seen any SCM register in the 3430 TRM related to CSI2 routing (most 
probably because the SoC seems to have the CSI1/CCP2 receiver hardwired to the 
CSIb block, and the CSI2 receiver hardwired to the CSIa block). If 34xx/35xx 
need something here, I don't know what.

What the 3430 would need is a ccp2 configuration function to access the 
CONTROL_CSIRXFE register.

> > > +	if (cpu_is_omap3630()) {
> > 
> > I would test !cpu_is_omap3630() and return, that would lower the
> > indentation level.
> 
> Now, yes. I'll do that if you agree with me on the previous question since
> otherwise this won't work too well. :)
> 
> > > +		u32 cam_phy_ctrl =
> > > +			omap_ctrl_readl(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 (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY1_CCP2B)
> > > +			cam_phy_ctrl |= 1 << 2;
> > > +		else if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY1_CSI2C)
> > > +			cam_phy_ctrl &= ~(1 << 2);

Looking a bit more at the TRM, don't we need to provide a way to select 
between data/clock and data/strobe modes, and to select which PHY to connect 
to the ISP CSIb receiver ?

> > > +
> > > +		if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY2_CCP2B)
> > > +			cam_phy_ctrl |= 1;
> > > +		else if (csi2cfg == OMAP3_CTRL_CSI2_SELECT_PHY2_CSI2A)
> > > +			cam_phy_ctrl &= ~1;
> > > +
> > > +		omap_ctrl_writel(cam_phy_ctrl,
> > > +				 OMAP3630_CONTROL_CAMERA_PHY_CTRL);
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL_GPL(omap3_ctrl_csi2_select);

-- 
Regards,

Laurent Pinchart

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

[Paul Walmsley]

Hello Sakari,

On Wed, 9 May 2012, 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 checking on this one.  Any update?

A few comments: it would be nice to have some kerneldoc for this function.  
Also it would be nice to use macros rather than the raw bitshifts in this 
function.  And one level of indentation can be saved if you bail out early 
if the function is not running on a 3630.

- Paul

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

[Sakari Ailus]

Hi Paul,

On Wed, Jul 04, 2012 at 04:53:32AM -0600, Paul Walmsley wrote:
> Hello Sakari,
> 
> On Wed, 9 May 2012, 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 checking on this one.  Any update?

Not yet; I need to address Laurent's comments also. I might have time to do
that during the next week.

> A few comments: it would be nice to have some kerneldoc for this function.  
> Also it would be nice to use macros rather than the raw bitshifts in this 
> function.  And one level of indentation can be saved if you bail out early 
> if the function is not running on a 3630.

Thanks. I'll do the changes for the next version.

Cheers,

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