Re: [PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Hi Laurent, On 05/15/2011 11:10 PM, Laurent Pinchart wrote: On Sunday 15 May 2011 11:33:14 Sylwester Nawrocki wrote: On 05/14/2011 05:29 PM, Laurent Pinchart wrote: On Wednesday 11 May 2011 17:17:10 Sylwester Nawrocki wrote: snip I perhaps need to isolate functions out from of s5pcsis_resume/suspend and then call that in s_power op and s5pcsis_resume/suspend. Don't really like this idea though... It would virtually render pm_runtime_* calls unusable in this sub-device driver, those would make sense only in the host driver.. I think using the pm_runtime_* calls make sense, they could replace the subdev s_power operation in the long term. We'll need to evaluate that (I'm not sure if runtime PM is available on all platforms targeted by V4L2 for instance). That sounds like a really good direction. It would let us have clear standardized rules for power handling in V4L2. And with new chips of more complicated topologies the power handling details could be taken care of by the Runtime PM subsystem. As the runtime PM becomes more granular it appears a good alternative for simple subdev s_power call. Unfortunately still only a few architectures support runtime PM at device bus level in the mainline. What I identified is: arm/mach-exynos4 arm/mach-omap1 arm/mach-omap2 arm/mach-shmobile So very few, no x86 here. As we have I2C, SPI and platform device v4l subdevs ideally those buses should support Runtime PM. I just wanted to put all what is needed to control device's power in the PM helpers and then use pm_runtime_* calls where required. -- Regards, Sylwester Nawrocki -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
On Tue, May 17, 2011 at 10:06:23PM +0200, Sylwester Nawrocki wrote: As we have I2C, SPI and platform device v4l subdevs ideally those buses should support Runtime PM. They all do so. -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Hi Laurent, thanks again for your review. On 05/14/2011 05:29 PM, Laurent Pinchart wrote: On Wednesday 11 May 2011 17:17:10 Sylwester Nawrocki wrote: Add the subdev driver for the MIPI CSIS units available in S5P and Exynos4 SoC series. This driver supports both CSIS0 and CSIS1 MIPI-CSI2 receivers. The driver requires Runtime PM to be enabled for proper operation. Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com [snip] diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644 index 000..d50efcb --- /dev/null +++ b/drivers/media/video/s5p-fimc/mipi-csis.c @@ -0,0 +1,722 @@ [snip] +static void s5pcsis_enable_interrupts(struct csis_state *state, bool on) +{ +u32 val = s5pcsis_read(state, S5PCSIS_INTMSK); + +val = on ? val | S5PCSIS_INTMSK_EN_ALL : + val ~S5PCSIS_INTMSK_EN_ALL; +s5pcsis_write(state, S5PCSIS_INTMSK, val); Shouldn't you clear all interrupt flags by writing to S5PCSIS_INTSRC before enabling interrupts, just in case ? In the start streaming sequence the device is first reset, then the receiver and PHY is enabled and finally interrupts are enabled. All interrupt sources are by default disabled after reset. Enabling interrupts is the last thing done in the start streaming sequence. By writing to S5PCSIS_INTSRC here any raised interrupt could be cleared and possibly lost before being handled. +} [snip] +static void s5pcsis_set_hsync_settle(struct csis_state *state, int settle) +{ +u32 val = s5pcsis_read(state, S5PCSIS_DPHYCTRL); + +val= ~S5PCSIS_DPHYCTRL_HSS_MASK | (settle 27); Do you mean val = (val ~S5PCSIS_DPHYCTRL_HSS_MASK) | (settle 27); ? Huh, naturally, yes, that was my intention.. Thank you for spotting this. I used to have problems before when the settle time parameter wasn't set properly, looks like the new boards are more tolerant.. +s5pcsis_write(state, S5PCSIS_DPHYCTRL, val); +} [snip] +static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, + struct v4l2_subdev_format *fmt) +{ +struct csis_state *state = sd_to_csis_state(sd); +struct csis_pix_format const *csis_fmt; +struct v4l2_mbus_framefmt *mf; + +mf = __s5pcsis_get_format(state, fh, fmt-pad, fmt-which); + +if (fmt-pad == CSIS_PAD_SOURCE) { +if (mf) { +mutex_lock(state-lock); +fmt-format = *mf; +mutex_unlock(state-lock); +} +return 0; +} +csis_fmt = s5pcsis_try_format(fmt-format); +if (mf) { +mutex_lock(state-lock); +*mf = fmt-format; +if (mf ==state-format) /* Store the active format */ I would replace the test by if (fmt-which == V4L2_SUBDEV_FORMAT_TRY) It's more explicit. I agree, I'll change that. +state-csis_fmt = csis_fmt; +mutex_unlock(state-lock); +} +return 0; +} [snip] +static int s5pcsis_suspend(struct device *dev) +{ +struct s5p_platform_mipi_csis *pdata = dev-platform_data; +struct platform_device *pdev = to_platform_device(dev); +struct v4l2_subdev *sd = platform_get_drvdata(pdev); +struct csis_state *state = sd_to_csis_state(sd); +int ret; + +v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n, + __func__, state-flags); + +mutex_lock(state-lock); +if (state-flags ST_POWERED) { +s5pcsis_stop_stream(state); +ret = pdata-phy_enable(state-pdev, false); +if (ret) +goto unlock; + +if (state-supply regulator_disable(state-supply)) +goto unlock; + +clk_disable(state-clock[CSIS_CLK_GATE]); +state-flags= ~ST_POWERED; +} +state-flags |= ST_SUSPENDED; + unlock: +mutex_unlock(state-lock); +return ret ? -EAGAIN : 0; +} + +static int s5pcsis_resume(struct device *dev) +{ +struct s5p_platform_mipi_csis *pdata = dev-platform_data; +struct platform_device *pdev = to_platform_device(dev); +struct v4l2_subdev *sd = platform_get_drvdata(pdev); +struct csis_state *state = sd_to_csis_state(sd); +int ret = 0; + +v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n, + __func__, state-flags); + +mutex_lock(state-lock); +if (!(state-flags ST_SUSPENDED)) +goto unlock; + +if (!(state-flags ST_POWERED)) { If the device wasn't powered before being suspended, it should stay powered off. I'm not sure, shortly after system wide resume the device is powered off by PM runtime core anyway. There is no other means in this driver to enable power except using pm_runtime_* calls. The device is being powered on or off only through these runtime PM helpers, i.e.
Re: [PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Hi Sylwester, On Sunday 15 May 2011 11:33:14 Sylwester Nawrocki wrote: On 05/14/2011 05:29 PM, Laurent Pinchart wrote: On Wednesday 11 May 2011 17:17:10 Sylwester Nawrocki wrote: [snip] +static int s5pcsis_suspend(struct device *dev) +{ + struct s5p_platform_mipi_csis *pdata = dev-platform_data; + struct platform_device *pdev = to_platform_device(dev); + struct v4l2_subdev *sd = platform_get_drvdata(pdev); + struct csis_state *state = sd_to_csis_state(sd); + int ret; + + v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n, + __func__, state-flags); + + mutex_lock(state-lock); + if (state-flags ST_POWERED) { + s5pcsis_stop_stream(state); + ret = pdata-phy_enable(state-pdev, false); + if (ret) + goto unlock; + + if (state-supply regulator_disable(state-supply)) + goto unlock; + + clk_disable(state-clock[CSIS_CLK_GATE]); + state-flags= ~ST_POWERED; + } + state-flags |= ST_SUSPENDED; + unlock: + mutex_unlock(state-lock); + return ret ? -EAGAIN : 0; +} + +static int s5pcsis_resume(struct device *dev) +{ + struct s5p_platform_mipi_csis *pdata = dev-platform_data; + struct platform_device *pdev = to_platform_device(dev); + struct v4l2_subdev *sd = platform_get_drvdata(pdev); + struct csis_state *state = sd_to_csis_state(sd); + int ret = 0; + + v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n, + __func__, state-flags); + + mutex_lock(state-lock); + if (!(state-flags ST_SUSPENDED)) + goto unlock; + + if (!(state-flags ST_POWERED)) { If the device wasn't powered before being suspended, it should stay powered off. I'm not sure, shortly after system wide resume the device is powered off by PM runtime core anyway. There is no other means in this driver to enable power except using pm_runtime_* calls. The device is being powered on or off only through these runtime PM helpers, i.e. s5pcsis_resume/s5pcsis_suspend. (full source can be found here: http://tinyurl.com/6fozx34) OK, it should be fine then. The pm_runtime_resume helper is guaranteed by the PM core to be called only on device in 'suspended' state. From Documentation/power/runtime_pm.txt: ... * Once the subsystem-level resume callback has completed successfully, the PM core regards the device as fully operational, which means that the device _must_ be able to complete I/O operations as needed. The run-time PM status of the device is then 'active'. ... If s5pcsis_resume would return 0 without really enabling device clocks and the external voltage regulator then the runtime PM core idea about the device's state would be wrong. I'm not a PM expert but documentation says that it's better to leave device fully functional after system wide driver's resume() helper returns. From Documentation/power/devices.txt: ... When resuming from standby or memory sleep, the phases are: resume_noirq, resume, complete. (...) At the end of these phases, drivers should be as functional as they were before suspending: I/O can be performed using DMA and IRQs, and the relevant clocks are gated on. Even if the device was in a low-power state before the system sleep because of runtime power management, afterwards it should be back in its full-power state. There are multiple reasons why it's best to do this; they are discussed in more detail in Documentation/power/runtime_pm.txt. ... Unfortunately there seem to be no standard way to instruct the PM core to enable power of a few (I2C/client platform) devices forming the video pipeline in an arbitrary order. The parent devices of my platforms devices are already the power domain devices. So it might be a good idea to forbid enabling sub-device's power when the host driver is not using it, to have full control of the pipeline devices power enable sequence at any time. I perhaps need to isolate functions out from of s5pcsis_resume/suspend and then call that in s_power op and s5pcsis_resume/suspend. Don't really like this idea though... It would virtually render pm_runtime_* calls unusable in this sub-device driver, those would make sense only in the host driver.. I think using the pm_runtime_* calls make sense, they could replace the subdev s_power operation in the long term. We'll need to evaluate that (I'm not sure if runtime PM is available on all platforms targetted by V4L2 for instance). I just wanted to put all what is needed to control device's power in the PM helpers and then use pm_runtime_* calls where required. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Hi Sylwester, On Wednesday 11 May 2011 17:17:10 Sylwester Nawrocki wrote: Add the subdev driver for the MIPI CSIS units available in S5P and Exynos4 SoC series. This driver supports both CSIS0 and CSIS1 MIPI-CSI2 receivers. The driver requires Runtime PM to be enabled for proper operation. Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com [snip] diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644 index 000..d50efcb --- /dev/null +++ b/drivers/media/video/s5p-fimc/mipi-csis.c @@ -0,0 +1,722 @@ [snip] +static void s5pcsis_enable_interrupts(struct csis_state *state, bool on) +{ + u32 val = s5pcsis_read(state, S5PCSIS_INTMSK); + + val = on ? val | S5PCSIS_INTMSK_EN_ALL : +val ~S5PCSIS_INTMSK_EN_ALL; + s5pcsis_write(state, S5PCSIS_INTMSK, val); Shouldn't you clear all interrupt flags by writing to S5PCSIS_INTSRC before enabling interrupts, just in case ? +} [snip] +static void s5pcsis_set_hsync_settle(struct csis_state *state, int settle) +{ + u32 val = s5pcsis_read(state, S5PCSIS_DPHYCTRL); + + val = ~S5PCSIS_DPHYCTRL_HSS_MASK | (settle 27); Do you mean val = (val ~S5PCSIS_DPHYCTRL_HSS_MASK) | (settle 27); ? + s5pcsis_write(state, S5PCSIS_DPHYCTRL, val); +} [snip] +static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, + struct v4l2_subdev_format *fmt) +{ + struct csis_state *state = sd_to_csis_state(sd); + struct csis_pix_format const *csis_fmt; + struct v4l2_mbus_framefmt *mf; + + mf = __s5pcsis_get_format(state, fh, fmt-pad, fmt-which); + + if (fmt-pad == CSIS_PAD_SOURCE) { + if (mf) { + mutex_lock(state-lock); + fmt-format = *mf; + mutex_unlock(state-lock); + } + return 0; + } + csis_fmt = s5pcsis_try_format(fmt-format); + if (mf) { + mutex_lock(state-lock); + *mf = fmt-format; + if (mf == state-format) /* Store the active format */ I would replace the test by if (fmt-which == V4L2_SUBDEV_FORMAT_TRY) It's more explicit. + state-csis_fmt = csis_fmt; + mutex_unlock(state-lock); + } + return 0; +} [snip] +static int s5pcsis_suspend(struct device *dev) +{ + struct s5p_platform_mipi_csis *pdata = dev-platform_data; + struct platform_device *pdev = to_platform_device(dev); + struct v4l2_subdev *sd = platform_get_drvdata(pdev); + struct csis_state *state = sd_to_csis_state(sd); + int ret; + + v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n, + __func__, state-flags); + + mutex_lock(state-lock); + if (state-flags ST_POWERED) { + s5pcsis_stop_stream(state); + ret = pdata-phy_enable(state-pdev, false); + if (ret) + goto unlock; + + if (state-supply regulator_disable(state-supply)) + goto unlock; + + clk_disable(state-clock[CSIS_CLK_GATE]); + state-flags = ~ST_POWERED; + } + state-flags |= ST_SUSPENDED; + unlock: + mutex_unlock(state-lock); + return ret ? -EAGAIN : 0; +} + +static int s5pcsis_resume(struct device *dev) +{ + struct s5p_platform_mipi_csis *pdata = dev-platform_data; + struct platform_device *pdev = to_platform_device(dev); + struct v4l2_subdev *sd = platform_get_drvdata(pdev); + struct csis_state *state = sd_to_csis_state(sd); + int ret = 0; + + v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n, + __func__, state-flags); + + mutex_lock(state-lock); + if (!(state-flags ST_SUSPENDED)) + goto unlock; + + if (!(state-flags ST_POWERED)) { If the device wasn't powered before being suspended, it should stay powered off. + if (state-supply) + ret = regulator_enable(state-supply); + if (ret) + goto unlock; + + ret = pdata-phy_enable(state-pdev, true); + if (!ret) { + state-flags |= ST_POWERED; + } else { + regulator_disable(state-supply); + goto unlock; + } + clk_enable(state-clock[CSIS_CLK_GATE]); + } + if (state-flags ST_STREAMING) + s5pcsis_start_stream(state); + + state-flags = ~ST_SUSPENDED; + unlock: + mutex_unlock(state-lock); + return ret ? -EAGAIN : 0; +} [snip] -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Add the subdev driver for the MIPI CSIS units available in S5P and Exynos4 SoC series. This driver supports both CSIS0 and CSIS1 MIPI-CSI2 receivers. The driver requires Runtime PM to be enabled for proper operation. Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/media/video/Kconfig |9 + drivers/media/video/s5p-fimc/Makefile|6 +- drivers/media/video/s5p-fimc/mipi-csis.c | 722 ++ drivers/media/video/s5p-fimc/mipi-csis.h | 22 + 4 files changed, 757 insertions(+), 2 deletions(-) create mode 100644 drivers/media/video/s5p-fimc/mipi-csis.c create mode 100644 drivers/media/video/s5p-fimc/mipi-csis.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index a705493..9c701dd 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -946,6 +946,15 @@ config VIDEO_SAMSUNG_S5P_FIMC To compile this driver as a module, choose M here: the module will be called s5p-fimc. +config VIDEO_S5P_MIPI_CSIS + tristate Samsung S5P and EXYNOS4 MIPI CSI receiver driver + depends on VIDEO_V4L2 PM_RUNTIME VIDEO_V4L2_SUBDEV_API + ---help--- + This is a v4l2 driver for Samsung S5P/EXYNOS4 MIPI-CSI receiver. + + To compile this driver as a module, choose M here: the + module will be called s5p-csis. + # # USB Multimedia device configuration # diff --git a/drivers/media/video/s5p-fimc/Makefile b/drivers/media/video/s5p-fimc/Makefile index 7ea1b14..df6954a 100644 --- a/drivers/media/video/s5p-fimc/Makefile +++ b/drivers/media/video/s5p-fimc/Makefile @@ -1,3 +1,5 @@ +s5p-fimc-objs := fimc-core.o fimc-reg.o fimc-capture.o +s5p-csis-objs := mipi-csis.o -obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) := s5p-fimc.o -s5p-fimc-y := fimc-core.o fimc-reg.o fimc-capture.o +obj-$(CONFIG_VIDEO_S5P_MIPI_CSIS) += s5p-csis.o +obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) += s5p-fimc.o diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644 index 000..d50efcb --- /dev/null +++ b/drivers/media/video/s5p-fimc/mipi-csis.c @@ -0,0 +1,722 @@ +/* + * Samsung S5P SoC series MIPI-CSI receiver driver + * + * Copyright (C) 2011 Samsung Electronics Co., Ltd. + * Contact: Sylwester Nawrocki, s.nawro...@samsung.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include linux/clk.h +#include linux/delay.h +#include linux/device.h +#include linux/errno.h +#include linux/interrupt.h +#include linux/io.h +#include linux/irq.h +#include linux/kernel.h +#include linux/memory.h +#include linux/module.h +#include linux/platform_device.h +#include linux/pm_runtime.h +#include linux/regulator/consumer.h +#include linux/slab.h +#include linux/spinlock.h +#include linux/videodev2.h +#include media/v4l2-subdev.h +#include plat/mipi_csis.h +#include mipi-csis.h + +static int debug; +module_param(debug, int, 0644); +MODULE_PARM_DESC(debug, Debug level (0-1)); + +/* Register map definition */ + +/* CSIS global control */ +#define S5PCSIS_CTRL 0x00 +#define S5PCSIS_CTRL_DPDN_DEFAULT (0 31) +#define S5PCSIS_CTRL_DPDN_SWAP (1 31) +#define S5PCSIS_CTRL_ALIGN_32BIT (1 20) +#define S5PCSIS_CTRL_UPDATE_SHADOW (1 16) +#define S5PCSIS_CTRL_WCLK_EXTCLK (1 8) +#define S5PCSIS_CTRL_RESET (1 4) +#define S5PCSIS_CTRL_ENABLE(1 0) + +/* D-PHY control */ +#define S5PCSIS_DPHYCTRL 0x04 +#define S5PCSIS_DPHYCTRL_HSS_MASK (0x1f 27) +#define S5PCSIS_DPHYCTRL_ENABLE(0x1f 0) + +#define S5PCSIS_CONFIG 0x08 +#define S5PCSIS_CFG_FMT_YCBCR422_8BIT (0x1e 2) +#define S5PCSIS_CFG_FMT_RAW8 (0x2a 2) +#define S5PCSIS_CFG_FMT_RAW10 (0x2b 2) +#define S5PCSIS_CFG_FMT_RAW12 (0x2c 2) +/* User defined formats, x = 1...4 */ +#define S5PCSIS_CFG_FMT_USER(x)((0x30 + x - 1) 2) +#define S5PCSIS_CFG_FMT_MASK (0x3f 2) +#define S5PCSIS_CFG_NR_LANE_MASK 3 + +/* Interrupt mask. */ +#define S5PCSIS_INTMSK 0x10 +#define S5PCSIS_INTMSK_EN_ALL 0xf03f +#define S5PCSIS_INTSRC 0x14 + +/* Pixel resolution */ +#define S5PCSIS_RESOL 0x2c +#define CSIS_MAX_PIX_WIDTH 0x +#define CSIS_MAX_PIX_HEIGHT0x + +enum { + CSIS_CLK_MUX, + CSIS_CLK_GATE, +}; + +static char *csi_clock_name[] = { + [CSIS_CLK_MUX] = sclk_csis, + [CSIS_CLK_GATE] = csis, +}; +#define NUM_CSIS_CLOCKSARRAY_SIZE(csi_clock_name) + +enum { + ST_POWERED = 1, + ST_STREAMING= 2, + ST_SUSPENDED= 4, +}; + +/** + * struct csis_state - the driver's internal state data structure + *