Re: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
Hi Sylwester, Sylwester Nawrocki wrote: On 01/08/2012 12:16 PM, Sakari Ailus wrote: Shouldn't lane configuration be retrieved from the sensor instead ? Sensors could use different lane configuration depending on the mode. This could also be implemented later when needed, but I don't think it would be too difficult to get it right now. I think we'd first need to standardise the CSI-2 bus configuration. I don't see a practical need to make the lane configuration dynamic. You could just use a lower frequency to achieve the same if you really need to. Ideally it might be nice to do but there's really nothing I know that required or even benefited from it --- at least for now. Does this mean that lane configuration needs to be duplicated in board code, on for the SMIA++ platform data and one of the OMAP3 ISP platform data ? It's mostly the number of lanes, and the polarity --- in theory it is possible to invert the signals on the bus, albeit I'm not sure if anyone does that; I can't see a reason for that, but hey, I don't know why it's possible to specify polarity either. :-) I think it just enables to swap D+ and D- functions on the physical pins. Good thinking. :-) Yeah, it's differential, so yes, I think so too now that you mention it. I've never seen polarity configuration option in any datasheet, neither MIPI CSI-2 or D-PHY mentions that. Does OMAP3 ISP really allow MIPI-CSI lane signal polarity configuration ? MIPI-CSI2 uses differential signals after all. What would be a point of changing polarity ? I don't know. It's also the same for CSI-1 on OMAP 3. This is actually one of the issues here: also device specific configuration is required. The standard configuration must contain probably at least what the spec defines. If both sides support mapping of the lanes, a mapping that matches on both sides has to be provided. In Samsung SoC (both sensor and host interface) I've seen only possibility to configure the number of data lanes, FWIW I think it is assumed that when you use e.g. 2 data lanes always lane1 and lane2 are utilised for transmission, for 3 lanes - lane 1,2,3, etc. Also I've never seen on schematics that someone wires data lane3 and lane4 when only 2 lanes are utilised, so this makes me wonder if the lane mapping is ever needed. Has anyone different experience with that ? Also the standard seem to specify that Data1+ lane at a transmitter(Tx) is connected to Data1+ lane at a receiver(Rx), Data1-(Tx) to Data1-(Rx), Data2+(Tx) to Data2+(Rx), etc. I think this is needed due to explicitly defined data distribution and merging scheme among the lanes, i.e. to allow interworking of various receivers and transmitters. Thus it seems all we need need is just a number of data lanes used. The standard of course specifies that the data lanes must be connected correctly. :-) It can't specify which SoC pins do they use, so for added flexibility it's good to be able to reorder them. Have you ever worked with single-layer PCBs by any chance? :-) More layers are used these days but it still doesn't solve all possible issues. Yes, I have. I know what you mean. It just seemed uncommon to me to reorder the signals. But since H/W doing that exists..and that might become more widely used in the future it might make sense to standardize lane configuration. We'll need different mapping configuration for the receiver and the transmitter, and not all the hardware supports it. It won't be many bytes, though. So I think I can say reordering generally must be supported by software if the hardware can do that. Yes, however there is always a board specific information involved, isn't it ? I.e. transmitter can reorder signals between its pins, the same can happen at a receiver and additionally the transmitter's pins can be connected differently to the receiver pins, depending on the board ? Exactly. That's the reason, I think, why the hardware does support it. Then do we make board specific information part of sensor's or host platform data ? It probably should be at both, let's take an evaluation and a camera daughter boards as an example. We also need device tree bindings for that, if possible the best would be to design common bindings, at least basic ones, to which device specific ones could be added. Sounds good to me. -- Sakari Ailus sakari.ai...@maxwell.research.nokia.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
Hi Laurent,
Laurent Pinchart wrote:
Hi Sakari,
On Saturday 07 January 2012 23:51:24 Sakari Ailus wrote:
Laurent Pinchart wrote:
On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote:
From: Sakari Ailus sakari.ai...@iki.fi
Configure CSI-2 phy based on platform data in the ISP driver rather than
in platform code.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
[snip]
diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 100644
--- a/drivers/media/video/omap3isp/ispcsiphy.c
+++ b/drivers/media/video/omap3isp/ispcsiphy.c
@@ -28,6 +28,8 @@
#include linux/device.h
#include linux/regulator/consumer.h
+#include ../../../../arch/arm/mach-omap2/control.h
+
#include mach/control.h
(untested) ?
I'm afraid it won't work. The above directory isn't in any include path
and likely shouldn't be. That file is included locally elsewhere, I
believe.
You're right, I spoke too fast.
I wonder what would be the right way to access that register definition
I need from the file:
OMAP343X_CTRL_BASE
OMAP3630_CONTROL_CAMERA_PHY_CTRL
Maybe the file (or part of it) should be moved to an include directory ?
Yes, but which one?
#include isp.h
#include ispreg.h
#include ispcsiphy.h
@@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy
*phy) isp_reg_writel(phy-isp, reg, phy-phy_regs, ISPCSIPHY_REG1);
}
-static int csiphy_config(struct isp_csiphy *phy,
- struct isp_csiphy_dphy_cfg *dphy,
- struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM 0
+#define TCLK_MISS 1
+#define TCLK_SETTLE 14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+ struct v4l2_subdev *csi2_subdev,
+ struct v4l2_subdev *sensor,
+ struct v4l2_mbus_framefmt *sensor_fmt)
{
+ struct isp_v4l2_subdevs_group *subdevs = sensor-host_priv;
+ struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
+ struct isp_csiphy_dphy_cfg csi2phy;
+ int csi2_ddrclk_khz;
+ struct isp_csiphy_lanes_cfg *lanes;
unsigned int used_lanes = 0;
unsigned int i;
+ u32 cam_phy_ctrl;
+
+ if (subdevs-interface == ISP_INTERFACE_CCP2B_PHY1
+ || subdevs-interface == ISP_INTERFACE_CCP2B_PHY2)
+ lanes = subdevs-bus.ccp2.lanecfg;
+ else
+ lanes = subdevs-bus.csi2.lanecfg;
Shouldn't lane configuration be retrieved from the sensor instead ?
Sensors could use different lane configuration depending on the mode.
This could also be implemented later when needed, but I don't think it
would be too difficult to get it right now.
I think we'd first need to standardise the CSI-2 bus configuration. I
don't see a practical need to make the lane configuration dynamic. You
could just use a lower frequency to achieve the same if you really need to.
Ideally it might be nice to do but there's really nothing I know that
required or even benefited from it --- at least for now.
Does this mean that lane configuration needs to be duplicated in board code,
on for the SMIA++ platform data and one of the OMAP3 ISP platform data ?
It's mostly the number of lanes, and the polarity --- in theory it is
possible to invert the signals on the bus, albeit I'm not sure if anyone
does that; I can't see a reason for that, but hey, I don't know why it's
possible to specify polarity either. :-)
If both sides support mapping of the lanes, a mapping that matches on
both sides has to be provided.
I agree we should standardise the configuration of CSI-2 as well as the
configuration of other busses. However, I would prefer to do that later
on since I'm already depending on a number of other patches on the
SMIA++ driver.
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
Hi, On 01/08/2012 11:26 AM, Sakari Ailus wrote: Shouldn't lane configuration be retrieved from the sensor instead ? Sensors could use different lane configuration depending on the mode. This could also be implemented later when needed, but I don't think it would be too difficult to get it right now. I think we'd first need to standardise the CSI-2 bus configuration. I don't see a practical need to make the lane configuration dynamic. You could just use a lower frequency to achieve the same if you really need to. Ideally it might be nice to do but there's really nothing I know that required or even benefited from it --- at least for now. Does this mean that lane configuration needs to be duplicated in board code, on for the SMIA++ platform data and one of the OMAP3 ISP platform data ? It's mostly the number of lanes, and the polarity --- in theory it is possible to invert the signals on the bus, albeit I'm not sure if anyone does that; I can't see a reason for that, but hey, I don't know why it's possible to specify polarity either. :-) I've never seen polarity configuration option in any datasheet, neither MIPI CSI-2 or D-PHY mentions that. Does OMAP3 ISP really allow MIPI-CSI lane signal polarity configuration ? MIPI-CSI2 uses differential signals after all. What would be a point of changing polarity ? If both sides support mapping of the lanes, a mapping that matches on both sides has to be provided. In Samsung SoC (both sensor and host interface) I've seen only possibility to configure the number of data lanes, FWIW I think it is assumed that when you use e.g. 2 data lanes always lane1 and lane2 are utilised for transmission, for 3 lanes - lane 1,2,3, etc. Also I've never seen on schematics that someone wires data lane3 and lane4 when only 2 lanes are utilised, so this makes me wonder if the lane mapping is ever needed. Has anyone different experience with that ? Also the standard seem to specify that Data1+ lane at a transmitter(Tx) is connected to Data1+ lane at a receiver(Rx), Data1-(Tx) to Data1-(Rx), Data2+(Tx) to Data2+(Rx), etc. I think this is needed due to explicitly defined data distribution and merging scheme among the lanes, i.e. to allow interworking of various receivers and transmitters. Thus it seems all we need need is just a number of data lanes used. I agree we should standardise the configuration of CSI-2 as well as the configuration of other busses. However, I would prefer to do that later on since I'm already depending on a number of other patches on the SMIA++ driver. -- Regards, Sylwester -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
Hi Sakari,
On Sunday 08 January 2012 11:26:02 Sakari Ailus wrote:
Laurent Pinchart wrote:
On Saturday 07 January 2012 23:51:24 Sakari Ailus wrote:
Laurent Pinchart wrote:
On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote:
From: Sakari Ailus sakari.ai...@iki.fi
Configure CSI-2 phy based on platform data in the ISP driver rather
than in platform code.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
[snip]
diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece
100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c
+++ b/drivers/media/video/omap3isp/ispcsiphy.c
@@ -28,6 +28,8 @@
#include linux/device.h
#include linux/regulator/consumer.h
+#include ../../../../arch/arm/mach-omap2/control.h
+
#include mach/control.h
(untested) ?
I'm afraid it won't work. The above directory isn't in any include path
and likely shouldn't be. That file is included locally elsewhere, I
believe.
You're right, I spoke too fast.
I wonder what would be the right way to access that register definition
I need from the file:
OMAP343X_CTRL_BASE
OMAP3630_CONTROL_CAMERA_PHY_CTRL
Maybe the file (or part of it) should be moved to an include directory ?
Yes, but which one?
Good question. The content of control.h doesn't seem to have been meant to be
exported. Maybe you should ask on linux-o...@vger.kernel.org ?
#include isp.h
#include ispreg.h
#include ispcsiphy.h
@@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy
*phy) isp_reg_writel(phy-isp, reg, phy-phy_regs, ISPCSIPHY_REG1);
}
-static int csiphy_config(struct isp_csiphy *phy,
- struct isp_csiphy_dphy_cfg *dphy,
- struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM 0
+#define TCLK_MISS 1
+#define TCLK_SETTLE 14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+ struct v4l2_subdev *csi2_subdev,
+ struct v4l2_subdev *sensor,
+ struct v4l2_mbus_framefmt *sensor_fmt)
{
+struct isp_v4l2_subdevs_group *subdevs = sensor-host_priv;
+struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
+struct isp_csiphy_dphy_cfg csi2phy;
+int csi2_ddrclk_khz;
+struct isp_csiphy_lanes_cfg *lanes;
unsigned int used_lanes = 0;
unsigned int i;
+u32 cam_phy_ctrl;
+
+if (subdevs-interface == ISP_INTERFACE_CCP2B_PHY1
+|| subdevs-interface == ISP_INTERFACE_CCP2B_PHY2)
+lanes = subdevs-bus.ccp2.lanecfg;
+else
+lanes = subdevs-bus.csi2.lanecfg;
Shouldn't lane configuration be retrieved from the sensor instead ?
Sensors could use different lane configuration depending on the mode.
This could also be implemented later when needed, but I don't think it
would be too difficult to get it right now.
I think we'd first need to standardise the CSI-2 bus configuration. I
don't see a practical need to make the lane configuration dynamic. You
could just use a lower frequency to achieve the same if you really need
to.
Ideally it might be nice to do but there's really nothing I know that
required or even benefited from it --- at least for now.
Does this mean that lane configuration needs to be duplicated in board
code, on for the SMIA++ platform data and one of the OMAP3 ISP platform
data ?
It's mostly the number of lanes, and the polarity --- in theory it is
possible to invert the signals on the bus, albeit I'm not sure if anyone
does that; I can't see a reason for that, but hey, I don't know why it's
possible to specify polarity either. :-)
If both sides support mapping of the lanes, a mapping that matches on
both sides has to be provided.
I agree we should standardise the configuration of CSI-2 as well as the
configuration of other busses. However, I would prefer to do that later
on since I'm already depending on a number of other patches on the
SMIA++ driver.
OK.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
Hi Sylwester, Sylwester Nawrocki wrote: Hi, On 01/08/2012 11:26 AM, Sakari Ailus wrote: Shouldn't lane configuration be retrieved from the sensor instead ? Sensors could use different lane configuration depending on the mode. This could also be implemented later when needed, but I don't think it would be too difficult to get it right now. I think we'd first need to standardise the CSI-2 bus configuration. I don't see a practical need to make the lane configuration dynamic. You could just use a lower frequency to achieve the same if you really need to. Ideally it might be nice to do but there's really nothing I know that required or even benefited from it --- at least for now. Does this mean that lane configuration needs to be duplicated in board code, on for the SMIA++ platform data and one of the OMAP3 ISP platform data ? It's mostly the number of lanes, and the polarity --- in theory it is possible to invert the signals on the bus, albeit I'm not sure if anyone does that; I can't see a reason for that, but hey, I don't know why it's possible to specify polarity either. :-) I've never seen polarity configuration option in any datasheet, neither MIPI CSI-2 or D-PHY mentions that. Does OMAP3 ISP really allow MIPI-CSI lane signal polarity configuration ? MIPI-CSI2 uses differential signals after all. What would be a point of changing polarity ? I don't know. It's also the same for CSI-1 on OMAP 3. This is actually one of the issues here: also device specific configuration is required. The standard configuration must contain probably at least what the spec defines. If both sides support mapping of the lanes, a mapping that matches on both sides has to be provided. In Samsung SoC (both sensor and host interface) I've seen only possibility to configure the number of data lanes, FWIW I think it is assumed that when you use e.g. 2 data lanes always lane1 and lane2 are utilised for transmission, for 3 lanes - lane 1,2,3, etc. Also I've never seen on schematics that someone wires data lane3 and lane4 when only 2 lanes are utilised, so this makes me wonder if the lane mapping is ever needed. Has anyone different experience with that ? Also the standard seem to specify that Data1+ lane at a transmitter(Tx) is connected to Data1+ lane at a receiver(Rx), Data1-(Tx) to Data1-(Rx), Data2+(Tx) to Data2+(Rx), etc. I think this is needed due to explicitly defined data distribution and merging scheme among the lanes, i.e. to allow interworking of various receivers and transmitters. Thus it seems all we need need is just a number of data lanes used. The standard of course specifies that the data lanes must be connected correctly. :-) It can't specify which SoC pins do they use, so for added flexibility it's good to be able to reorder them. Have you ever worked with single-layer PCBs by any chance? :-) More layers are used these days but it still doesn't solve all possible issues. So I think I can say reordering generally must be supported by software if the hardware can do that. -- Sakari Ailus sakari.ai...@maxwell.research.nokia.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
Hi Sakari, On 01/08/2012 12:16 PM, Sakari Ailus wrote: Shouldn't lane configuration be retrieved from the sensor instead ? Sensors could use different lane configuration depending on the mode. This could also be implemented later when needed, but I don't think it would be too difficult to get it right now. I think we'd first need to standardise the CSI-2 bus configuration. I don't see a practical need to make the lane configuration dynamic. You could just use a lower frequency to achieve the same if you really need to. Ideally it might be nice to do but there's really nothing I know that required or even benefited from it --- at least for now. Does this mean that lane configuration needs to be duplicated in board code, on for the SMIA++ platform data and one of the OMAP3 ISP platform data ? It's mostly the number of lanes, and the polarity --- in theory it is possible to invert the signals on the bus, albeit I'm not sure if anyone does that; I can't see a reason for that, but hey, I don't know why it's possible to specify polarity either. :-) I think it just enables to swap D+ and D- functions on the physical pins. I've never seen polarity configuration option in any datasheet, neither MIPI CSI-2 or D-PHY mentions that. Does OMAP3 ISP really allow MIPI-CSI lane signal polarity configuration ? MIPI-CSI2 uses differential signals after all. What would be a point of changing polarity ? I don't know. It's also the same for CSI-1 on OMAP 3. This is actually one of the issues here: also device specific configuration is required. The standard configuration must contain probably at least what the spec defines. If both sides support mapping of the lanes, a mapping that matches on both sides has to be provided. In Samsung SoC (both sensor and host interface) I've seen only possibility to configure the number of data lanes, FWIW I think it is assumed that when you use e.g. 2 data lanes always lane1 and lane2 are utilised for transmission, for 3 lanes - lane 1,2,3, etc. Also I've never seen on schematics that someone wires data lane3 and lane4 when only 2 lanes are utilised, so this makes me wonder if the lane mapping is ever needed. Has anyone different experience with that ? Also the standard seem to specify that Data1+ lane at a transmitter(Tx) is connected to Data1+ lane at a receiver(Rx), Data1-(Tx) to Data1-(Rx), Data2+(Tx) to Data2+(Rx), etc. I think this is needed due to explicitly defined data distribution and merging scheme among the lanes, i.e. to allow interworking of various receivers and transmitters. Thus it seems all we need need is just a number of data lanes used. The standard of course specifies that the data lanes must be connected correctly. :-) It can't specify which SoC pins do they use, so for added flexibility it's good to be able to reorder them. Have you ever worked with single-layer PCBs by any chance? :-) More layers are used these days but it still doesn't solve all possible issues. Yes, I have. I know what you mean. It just seemed uncommon to me to reorder the signals. But since H/W doing that exists..and that might become more widely used in the future it might make sense to standardize lane configuration. So I think I can say reordering generally must be supported by software if the hardware can do that. Yes, however there is always a board specific information involved, isn't it ? I.e. transmitter can reorder signals between its pins, the same can happen at a receiver and additionally the transmitter's pins can be connected differently to the receiver pins, depending on the board ? Then do we make board specific information part of sensor's or host platform data ? It probably should be at both, let's take an evaluation and a camera daughter boards as an example. We also need device tree bindings for that, if possible the best would be to design common bindings, at least basic ones, to which device specific ones could be added. -- Regards, Sylwester -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
Hi Laurent,
Thanks for the review!!!
Laurent Pinchart wrote:
On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote:
From: Sakari Ailus sakari.ai...@iki.fi
Configure CSI-2 phy based on platform data in the ISP driver rather than in
platform code.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
---
drivers/media/video/omap3isp/isp.c | 38 --
drivers/media/video/omap3isp/isp.h |3 -
drivers/media/video/omap3isp/ispcsiphy.c | 83 +++
drivers/media/video/omap3isp/ispcsiphy.h |4 ++
4 files changed, 111 insertions(+), 17 deletions(-)
diff --git a/drivers/media/video/omap3isp/isp.c
b/drivers/media/video/omap3isp/isp.c index b818cac..6020fd7 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -737,7 +737,7 @@ static int isp_pipeline_enable(struct isp_pipeline
*pipe, struct isp_device *isp = pipe-output-isp;
struct media_entity *entity;
struct media_pad *pad;
-struct v4l2_subdev *subdev;
+struct v4l2_subdev *subdev = NULL, *prev_subdev;
unsigned long flags;
int ret;
@@ -759,11 +759,41 @@ static int isp_pipeline_enable(struct isp_pipeline
*pipe, break;
entity = pad-entity;
+prev_subdev = subdev;
subdev = media_entity_to_v4l2_subdev(entity);
-ret = v4l2_subdev_call(subdev, video, s_stream, mode);
-if (ret 0 ret != -ENOIOCTLCMD)
-return ret;
+/* Configure CSI-2 receiver based on sensor format. */
+if (prev_subdev == isp-isp_csi2a.subdev
+|| prev_subdev == isp-isp_csi2c.subdev) {
+struct v4l2_subdev_format fmt;
+
+fmt.pad = pad-index;
+fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ret = v4l2_subdev_call(subdev, pad, get_fmt,
+ NULL, fmt);
+if (ret 0)
+return -EPIPE;
+
+ret = omap3isp_csiphy_config(
+isp, prev_subdev, subdev,
+fmt.format);
+if (ret 0)
+return -EPIPE;
+
+/* Start CSI-2 after configuration. */
+ret = v4l2_subdev_call(prev_subdev, video,
+ s_stream, mode);
+if (ret 0 ret != -ENOIOCTLCMD)
+return ret;
+}
+
+/* Start any other subdev except the CSI-2 receivers. */
+if (subdev != isp-isp_csi2a.subdev
+ subdev != isp-isp_csi2c.subdev) {
+ret = v4l2_subdev_call(subdev, video, s_stream, mode);
+if (ret 0 ret != -ENOIOCTLCMD)
+return ret;
+}
What about moving this to the CSI2 s_stream subdev operation ?
Done.
if (subdev == isp-isp_ccdc.subdev) {
v4l2_subdev_call(isp-isp_aewb.subdev, video,
diff --git a/drivers/media/video/omap3isp/isp.h
b/drivers/media/video/omap3isp/isp.h index 705946e..c5935ae 100644
--- a/drivers/media/video/omap3isp/isp.h
+++ b/drivers/media/video/omap3isp/isp.h
@@ -126,9 +126,6 @@ struct isp_reg {
struct isp_platform_callback {
u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
-int (*csiphy_config)(struct isp_csiphy *phy,
- struct isp_csiphy_dphy_cfg *dphy,
- struct isp_csiphy_lanes_cfg *lanes);
void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
};
diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 100644
--- a/drivers/media/video/omap3isp/ispcsiphy.c
+++ b/drivers/media/video/omap3isp/ispcsiphy.c
@@ -28,6 +28,8 @@
#include linux/device.h
#include linux/regulator/consumer.h
+#include ../../../../arch/arm/mach-omap2/control.h
+
#include mach/control.h
(untested) ?
I'm afraid it won't work. The above directory isn't in any include path
and likely shouldn't be. That file is included locally elsewhere, I believe.
I wonder what would be the right way to access that register definition
I need from the file:
OMAP343X_CTRL_BASE
OMAP3630_CONTROL_CAMERA_PHY_CTRL
#include isp.h
#include ispreg.h
#include ispcsiphy.h
@@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy
*phy) isp_reg_writel(phy-isp, reg, phy-phy_regs, ISPCSIPHY_REG1);
}
-static int csiphy_config(struct isp_csiphy *phy,
- struct isp_csiphy_dphy_cfg *dphy,
- struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM 0
+#define TCLK_MISS 1
+#define TCLK_SETTLE 14
+
+int
Re: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
Hi Sakari,
On Saturday 07 January 2012 23:51:24 Sakari Ailus wrote:
Laurent Pinchart wrote:
On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote:
From: Sakari Ailus sakari.ai...@iki.fi
Configure CSI-2 phy based on platform data in the ISP driver rather than
in platform code.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
[snip]
diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 100644
--- a/drivers/media/video/omap3isp/ispcsiphy.c
+++ b/drivers/media/video/omap3isp/ispcsiphy.c
@@ -28,6 +28,8 @@
#include linux/device.h
#include linux/regulator/consumer.h
+#include ../../../../arch/arm/mach-omap2/control.h
+
#include mach/control.h
(untested) ?
I'm afraid it won't work. The above directory isn't in any include path
and likely shouldn't be. That file is included locally elsewhere, I
believe.
You're right, I spoke too fast.
I wonder what would be the right way to access that register definition
I need from the file:
OMAP343X_CTRL_BASE
OMAP3630_CONTROL_CAMERA_PHY_CTRL
Maybe the file (or part of it) should be moved to an include directory ?
#include isp.h
#include ispreg.h
#include ispcsiphy.h
@@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy
*phy) isp_reg_writel(phy-isp, reg, phy-phy_regs, ISPCSIPHY_REG1);
}
-static int csiphy_config(struct isp_csiphy *phy,
- struct isp_csiphy_dphy_cfg *dphy,
- struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM 0
+#define TCLK_MISS 1
+#define TCLK_SETTLE 14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+ struct v4l2_subdev *csi2_subdev,
+ struct v4l2_subdev *sensor,
+ struct v4l2_mbus_framefmt *sensor_fmt)
{
+ struct isp_v4l2_subdevs_group *subdevs = sensor-host_priv;
+ struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
+ struct isp_csiphy_dphy_cfg csi2phy;
+ int csi2_ddrclk_khz;
+ struct isp_csiphy_lanes_cfg *lanes;
unsigned int used_lanes = 0;
unsigned int i;
+ u32 cam_phy_ctrl;
+
+ if (subdevs-interface == ISP_INTERFACE_CCP2B_PHY1
+ || subdevs-interface == ISP_INTERFACE_CCP2B_PHY2)
+ lanes = subdevs-bus.ccp2.lanecfg;
+ else
+ lanes = subdevs-bus.csi2.lanecfg;
Shouldn't lane configuration be retrieved from the sensor instead ?
Sensors could use different lane configuration depending on the mode.
This could also be implemented later when needed, but I don't think it
would be too difficult to get it right now.
I think we'd first need to standardise the CSI-2 bus configuration. I
don't see a practical need to make the lane configuration dynamic. You
could just use a lower frequency to achieve the same if you really need to.
Ideally it might be nice to do but there's really nothing I know that
required or even benefited from it --- at least for now.
Does this mean that lane configuration needs to be duplicated in board code,
on for the SMIA++ platform data and one of the OMAP3 ISP platform data ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
Hi Sakari,
Thanks for the patch.
On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote:
From: Sakari Ailus sakari.ai...@iki.fi
Configure CSI-2 phy based on platform data in the ISP driver rather than in
platform code.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
---
drivers/media/video/omap3isp/isp.c | 38 --
drivers/media/video/omap3isp/isp.h |3 -
drivers/media/video/omap3isp/ispcsiphy.c | 83 +++
drivers/media/video/omap3isp/ispcsiphy.h |4 ++
4 files changed, 111 insertions(+), 17 deletions(-)
diff --git a/drivers/media/video/omap3isp/isp.c
b/drivers/media/video/omap3isp/isp.c index b818cac..6020fd7 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -737,7 +737,7 @@ static int isp_pipeline_enable(struct isp_pipeline
*pipe, struct isp_device *isp = pipe-output-isp;
struct media_entity *entity;
struct media_pad *pad;
- struct v4l2_subdev *subdev;
+ struct v4l2_subdev *subdev = NULL, *prev_subdev;
unsigned long flags;
int ret;
@@ -759,11 +759,41 @@ static int isp_pipeline_enable(struct isp_pipeline
*pipe, break;
entity = pad-entity;
+ prev_subdev = subdev;
subdev = media_entity_to_v4l2_subdev(entity);
- ret = v4l2_subdev_call(subdev, video, s_stream, mode);
- if (ret 0 ret != -ENOIOCTLCMD)
- return ret;
+ /* Configure CSI-2 receiver based on sensor format. */
+ if (prev_subdev == isp-isp_csi2a.subdev
+ || prev_subdev == isp-isp_csi2c.subdev) {
+ struct v4l2_subdev_format fmt;
+
+ fmt.pad = pad-index;
+ fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(subdev, pad, get_fmt,
+NULL, fmt);
+ if (ret 0)
+ return -EPIPE;
+
+ ret = omap3isp_csiphy_config(
+ isp, prev_subdev, subdev,
+ fmt.format);
+ if (ret 0)
+ return -EPIPE;
+
+ /* Start CSI-2 after configuration. */
+ ret = v4l2_subdev_call(prev_subdev, video,
+s_stream, mode);
+ if (ret 0 ret != -ENOIOCTLCMD)
+ return ret;
+ }
+
+ /* Start any other subdev except the CSI-2 receivers. */
+ if (subdev != isp-isp_csi2a.subdev
+ subdev != isp-isp_csi2c.subdev) {
+ ret = v4l2_subdev_call(subdev, video, s_stream, mode);
+ if (ret 0 ret != -ENOIOCTLCMD)
+ return ret;
+ }
What about moving this to the CSI2 s_stream subdev operation ?
if (subdev == isp-isp_ccdc.subdev) {
v4l2_subdev_call(isp-isp_aewb.subdev, video,
diff --git a/drivers/media/video/omap3isp/isp.h
b/drivers/media/video/omap3isp/isp.h index 705946e..c5935ae 100644
--- a/drivers/media/video/omap3isp/isp.h
+++ b/drivers/media/video/omap3isp/isp.h
@@ -126,9 +126,6 @@ struct isp_reg {
struct isp_platform_callback {
u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
- int (*csiphy_config)(struct isp_csiphy *phy,
- struct isp_csiphy_dphy_cfg *dphy,
- struct isp_csiphy_lanes_cfg *lanes);
void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
};
diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 100644
--- a/drivers/media/video/omap3isp/ispcsiphy.c
+++ b/drivers/media/video/omap3isp/ispcsiphy.c
@@ -28,6 +28,8 @@
#include linux/device.h
#include linux/regulator/consumer.h
+#include ../../../../arch/arm/mach-omap2/control.h
+
#include mach/control.h
(untested) ?
#include isp.h
#include ispreg.h
#include ispcsiphy.h
@@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy
*phy) isp_reg_writel(phy-isp, reg, phy-phy_regs, ISPCSIPHY_REG1);
}
-static int csiphy_config(struct isp_csiphy *phy,
- struct isp_csiphy_dphy_cfg *dphy,
- struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM0
+#define TCLK_MISS1
+#define TCLK_SETTLE 14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+struct v4l2_subdev *csi2_subdev,
+struct v4l2_subdev *sensor,
+struct v4l2_mbus_framefmt *sensor_fmt)
{
+ struct isp_v4l2_subdevs_group *subdevs = sensor-host_priv;
+ struct
[RFC 13/17] omap3isp: Configure CSI-2 phy based on platform data
From: Sakari Ailus sakari.ai...@iki.fi
Configure CSI-2 phy based on platform data in the ISP driver rather than in
platform code.
Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
---
drivers/media/video/omap3isp/isp.c | 38 --
drivers/media/video/omap3isp/isp.h |3 -
drivers/media/video/omap3isp/ispcsiphy.c | 83 ++
drivers/media/video/omap3isp/ispcsiphy.h |4 ++
4 files changed, 111 insertions(+), 17 deletions(-)
diff --git a/drivers/media/video/omap3isp/isp.c
b/drivers/media/video/omap3isp/isp.c
index b818cac..6020fd7 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -737,7 +737,7 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe,
struct isp_device *isp = pipe-output-isp;
struct media_entity *entity;
struct media_pad *pad;
- struct v4l2_subdev *subdev;
+ struct v4l2_subdev *subdev = NULL, *prev_subdev;
unsigned long flags;
int ret;
@@ -759,11 +759,41 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe,
break;
entity = pad-entity;
+ prev_subdev = subdev;
subdev = media_entity_to_v4l2_subdev(entity);
- ret = v4l2_subdev_call(subdev, video, s_stream, mode);
- if (ret 0 ret != -ENOIOCTLCMD)
- return ret;
+ /* Configure CSI-2 receiver based on sensor format. */
+ if (prev_subdev == isp-isp_csi2a.subdev
+ || prev_subdev == isp-isp_csi2c.subdev) {
+ struct v4l2_subdev_format fmt;
+
+ fmt.pad = pad-index;
+ fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(subdev, pad, get_fmt,
+ NULL, fmt);
+ if (ret 0)
+ return -EPIPE;
+
+ ret = omap3isp_csiphy_config(
+ isp, prev_subdev, subdev,
+ fmt.format);
+ if (ret 0)
+ return -EPIPE;
+
+ /* Start CSI-2 after configuration. */
+ ret = v4l2_subdev_call(prev_subdev, video,
+ s_stream, mode);
+ if (ret 0 ret != -ENOIOCTLCMD)
+ return ret;
+ }
+
+ /* Start any other subdev except the CSI-2 receivers. */
+ if (subdev != isp-isp_csi2a.subdev
+subdev != isp-isp_csi2c.subdev) {
+ ret = v4l2_subdev_call(subdev, video, s_stream, mode);
+ if (ret 0 ret != -ENOIOCTLCMD)
+ return ret;
+ }
if (subdev == isp-isp_ccdc.subdev) {
v4l2_subdev_call(isp-isp_aewb.subdev, video,
diff --git a/drivers/media/video/omap3isp/isp.h
b/drivers/media/video/omap3isp/isp.h
index 705946e..c5935ae 100644
--- a/drivers/media/video/omap3isp/isp.h
+++ b/drivers/media/video/omap3isp/isp.h
@@ -126,9 +126,6 @@ struct isp_reg {
struct isp_platform_callback {
u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
- int (*csiphy_config)(struct isp_csiphy *phy,
-struct isp_csiphy_dphy_cfg *dphy,
-struct isp_csiphy_lanes_cfg *lanes);
void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
};
diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
b/drivers/media/video/omap3isp/ispcsiphy.c
index 5be37ce..f027ece 100644
--- a/drivers/media/video/omap3isp/ispcsiphy.c
+++ b/drivers/media/video/omap3isp/ispcsiphy.c
@@ -28,6 +28,8 @@
#include linux/device.h
#include linux/regulator/consumer.h
+#include ../../../../arch/arm/mach-omap2/control.h
+
#include isp.h
#include ispreg.h
#include ispcsiphy.h
@@ -138,15 +140,78 @@ static void csiphy_dphy_config(struct isp_csiphy *phy)
isp_reg_writel(phy-isp, reg, phy-phy_regs, ISPCSIPHY_REG1);
}
-static int csiphy_config(struct isp_csiphy *phy,
-struct isp_csiphy_dphy_cfg *dphy,
-struct isp_csiphy_lanes_cfg *lanes)
+/*
+ * TCLK values are OK at their reset values
+ */
+#define TCLK_TERM 0
+#define TCLK_MISS 1
+#define TCLK_SETTLE14
+
+int omap3isp_csiphy_config(struct isp_device *isp,
+ struct v4l2_subdev *csi2_subdev,
+ struct v4l2_subdev *sensor,
+ struct v4l2_mbus_framefmt *sensor_fmt)
{
+ struct isp_v4l2_subdevs_group *subdevs = sensor-host_priv;
+ struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
+ struct isp_csiphy_dphy_cfg csi2phy;
+ int csi2_ddrclk_khz;
+ struct
