The clock dependencies between sensor subdevs and the host interface drivers

Hi everyone,

Nowadays many of the V4L2 camera device drivers heavily depend on the board code to set up voltage supplies, clocks, and some control signals, like 'reset' and 'standby' signals for the sensors. Those things are often being done by means of the driver specific platform data callbacks.

There has been recently quite a lot effort on ARM towards migration to the device tree. Unfortunately the custom platform data callbacks effectively prevent the boards to be booted and configured through the device tree bindings.

The following is usually handled in the board files:

1) sensor/frontend power supply
2) sensor's master clock (provided by the host device)
3) reset and standby signals (GPIO)
4) other signals applied by the host processor to the sensor device, e.g. I2C level shifter enable, etc.

For 1), the regulator API should possibly be used. It should be applicable for most, if not all cases.
3) and 4) are a bit hard as there might be huge differences across boards as how many GPIOs are used, what are the required delays between changes of their states, etc. Still we could try to find a common description of the behaviour and pass such information to the drivers.

For 2) I'd like to propose adding a callback to struct v4l2_device, for instance as in the below patch. The host driver would implement such an operation and the sensor subdev driver would use it in its s_power op.

If there is more than one output clock at the host, to distinguish which clock applies to a given sensor the host driver could be passed the assignment information in it's platform data.

AFAICS in the omap3isp case the clock control is done through the board code. I wonder what prevents making direct calls between the drivers, as the sensor subdevs call a board code callback there, which in turn only calls into the omap3isp driver.
What am I missing ?

In order to support fully DT based builds it would be desired to change that method so the board specific callbacks in platform data structures are avoided.

include/media/v4l2-device.h +11 -3

index d61febf..08b6699 100644
@@ -36,6 +36,15 @@
 
 struct v4l2_ctrl_handler;
 
+struct v4l2_device_ops {
+	/* notify callback called by some sub-devices */
+	void (*notify)(struct v4l2_subdev *sd,
+			unsigned int notification, void *arg);
+	/* clock control callback */
+	void (*set_clock)(struct v4l2_subdev *sd,
+			  u_long *frequency, bool enable);
+};
+
 struct v4l2_device {
 	/* dev->driver_data points to this struct.
 	   Note: dev might be NULL if there is no parent device

@@ -51,9 +60,8 @@ struct v4l2_device {
 	spinlock_t lock;
 	/* unique device name, by default the driver name + bus ID */
 	char name[V4L2_DEVICE_NAME_SIZE];
-	/* notify callback called by some sub-devices. */
-	void (*notify)(struct v4l2_subdev *sd,
-			unsigned int notification, void *arg);
+	/* ops for sub-devices */
+	struct v4l2_device_ops ops;
 	/* The control handler. May be NULL. */
 	struct v4l2_ctrl_handler *ctrl_handler;
 	/* Device's priority state */

Hi Sylwester,

Quoted 27 lines · On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote:

Hi everyone,

Nowadays many of the V4L2 camera device drivers heavily depend on the board code to set up voltage supplies, clocks, and some control signals, like 'reset' and 'standby' signals for the sensors. Those things are often being done by means of the driver specific platform data callbacks.

There has been recently quite a lot effort on ARM towards migration to the device tree. Unfortunately the custom platform data callbacks effectively prevent the boards to be booted and configured through the device tree bindings.

The following is usually handled in the board files:

1) sensor/frontend power supply
2) sensor's master clock (provided by the host device)
3) reset and standby signals (GPIO)
4) other signals applied by the host processor to the sensor device, e.g. I2C level shifter enable, etc.

For 1), the regulator API should possibly be used. It should be applicable for most, if not all cases.
3) and 4) are a bit hard as there might be huge differences across boards as how many GPIOs are used, what are the required delays between changes of their states, etc. Still we could try to find a common description of the behaviour and pass such information to the drivers.

Quoted 3 lines

For 2) I'd like to propose adding a callback to struct v4l2_device, for instance as in the below patch. The host driver would implement such an operation and the sensor subdev driver would use it in its s_power op.

What about using a struct clk object ? There has been lots of work in the ARM tree to make struct clk generic. I'm not sure if all patches have been pushed to mainline yet, but I think that's the direction we should follow.

Quoted 4 lines

If there is more than one output clock at the host, to distinguish which clock applies to a given sensor the host driver could be passed the assignment information in it's platform data.

Quoted 5 lines

AFAICS in the omap3isp case the clock control is done through the board code. I wonder what prevents making direct calls between the drivers, as the sensor subdevs call a board code callback there, which in turn only calls into the omap3isp driver.
What am I missing ?

We go through board code to remove dependencies between drivers. My goal is to implement this through struct clk, but I haven't had time to work on that yet.

Quoted 29 lines

In order to support fully DT based builds it would be desired to change that method so the board specific callbacks in platform data structures are avoided.

include/media/v4l2-device.h +9 -0

index d61febf..08b6699 100644
@@ -36,6 +36,15 @@
 
 struct v4l2_ctrl_handler;
 
+struct v4l2_device_ops {
+	/* notify callback called by some sub-devices */
+	void (*notify)(struct v4l2_subdev *sd,
+			unsigned int notification, void *arg);
+	/* clock control callback */
+	void (*set_clock)(struct v4l2_subdev *sd,
+			  u_long *frequency, bool enable);
+};
+
 struct v4l2_device {
 	/* dev->driver_data points to this struct.
 	   Note: dev might be NULL if there is no parent device

@@ -51,9 +60,8 @@ struct v4l2_device {
 	spinlock_t lock;
 	/* unique device name, by default the driver name + bus ID */
 	char name[V4L2_DEVICE_NAME_SIZE];

Quoted 8 lines

include/media/v4l2-device.h +2 -3

-	/* notify callback called by some sub-devices. */
-	void (*notify)(struct v4l2_subdev *sd,
-			unsigned int notification, void *arg);
+	/* ops for sub-devices */
+	struct v4l2_device_ops ops;
 	/* The control handler. May be NULL. */
 	struct v4l2_ctrl_handler *ctrl_handler;
 	/* Device's priority state */

Signature · 3 lines

Regards,

Laurent Pinchart

Quoted 51 lines · Laurent Pinchart

Quoted 31 lines · On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote:

Hi everyone,

Nowadays many of the V4L2 camera device drivers heavily depend on the board code to set up voltage supplies, clocks, and some control signals, like 'reset' and 'standby' signals for the sensors. Those things are often being done by means of the driver specific platform data callbacks.

There has been recently quite a lot effort on ARM towards migration to the device tree. Unfortunately the custom platform data callbacks effectively prevent the boards to be booted and configured through the device tree bindings.

The following is usually handled in the board files:

1) sensor/frontend power supply
2) sensor's master clock (provided by the host device)
3) reset and standby signals (GPIO)
4) other signals applied by the host processor to the sensor device, e.g.

I2C level shifter enable, etc.

For 1), the regulator API should possibly be used. It should be applicable for most, if not all cases.
3) and 4) are a bit hard as there might be huge differences across boards as how many GPIOs are used, what are the required delays between changes of their states, etc. Still we could try to find a common description of the behaviour and pass such information to the drivers.

Quoted 3 lines

For 2) I'd like to propose adding a callback to struct v4l2_device, for instance as in the below patch. The host driver would implement such an operation and the sensor subdev driver would use it in its s_power op.

What about using a struct clk object ? There has been lots of work in the ARM tree to make struct clk generic. I'm not sure if all patches have been pushed to mainline yet, but I think that's the direction we should follow.

Quoted 9 lines

If there is more than one output clock at the host, to distinguish which clock applies to a given sensor the host driver could be passed the assignment information in it's platform data.

AFAICS in the omap3isp case the clock control is done through the board code. I wonder what prevents making direct calls between the drivers, as the sensor subdevs call a board code callback there, which in turn only calls into the omap3isp driver.
What am I missing ?

Quoted 3 lines

We go through board code to remove dependencies between drivers. My goal is to implement this through struct clk, but I haven't had time to work on that yet.

Is this now a different topic from what I have proposed in mbus configuration RFC initially or have you changed your opinion?

Hi Sylwester,

Quoted 27 lines · On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote:

Hi everyone,

Nowadays many of the V4L2 camera device drivers heavily depend on the board code to set up voltage supplies, clocks, and some control signals, like 'reset' and 'standby' signals for the sensors. Those things are often being done by means of the driver specific platform data callbacks.

There has been recently quite a lot effort on ARM towards migration to the device tree. Unfortunately the custom platform data callbacks effectively prevent the boards to be booted and configured through the device tree bindings.

The following is usually handled in the board files:

1) sensor/frontend power supply
2) sensor's master clock (provided by the host device)
3) reset and standby signals (GPIO)
4) other signals applied by the host processor to the sensor device, e.g. I2C level shifter enable, etc.

For 1), the regulator API should possibly be used. It should be applicable for most, if not all cases.
3) and 4) are a bit hard as there might be huge differences across boards as how many GPIOs are used, what are the required delays between changes of their states, etc. Still we could try to find a common description of the behaviour and pass such information to the drivers.

Quoted 3 lines

For 2) I'd like to propose adding a callback to struct v4l2_device, for instance as in the below patch. The host driver would implement such an operation and the sensor subdev driver would use it in its s_power op.

What about using a struct clk object ? There has been lots of work in the ARM tree to make struct clk generic. I'm not sure if all patches have been pushed to mainline yet, but I think that's the direction we should follow.

Quoted 35 lines · Laurent Pinchart

Quoted 33 lines · On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote:

Hi everyone,

Nowadays many of the V4L2 camera device drivers heavily depend on the board code to set up voltage supplies, clocks, and some control signals, like 'reset' and 'standby' signals for the sensors. Those things are often being done by means of the driver specific platform data callbacks.

There has been recently quite a lot effort on ARM towards migration to the device tree. Unfortunately the custom platform data callbacks effectively prevent the boards to be booted and configured through the device tree bindings.

The following is usually handled in the board files:

1) sensor/frontend power supply
2) sensor's master clock (provided by the host device)
3) reset and standby signals (GPIO)
4) other signals applied by the host processor to the sensor device, e.g.

I2C level shifter enable, etc.

For 1), the regulator API should possibly be used. It should be applicable for most, if not all cases.
3) and 4) are a bit hard as there might be huge differences across boards as how many GPIOs are used, what are the required delays between changes of their states, etc. Still we could try to find a common description of the behaviour and pass such information to the drivers.

For 2) I'd like to propose adding a callback to struct v4l2_device, for instance as in the below patch. The host driver would implement such an operation and the sensor subdev driver would use it in its s_power op.

Quoted 4 lines

What about using a struct clk object ? There has been lots of work in the ARM tree to make struct clk generic. I'm not sure if all patches have been pushed to mainline yet, but I think that's the direction we should follow.

But is the 'struct clk' tried to be unified across all archs, not only ARM ? I'm afraid it's not the case.

By "using a struct clk object" do you also mean implementing some/all ops of this object by the driver which exports it ?

I suppose we can't rely only on the clock controller functionality exposed through the clock API.

Some devices may need to be brought to an active state before the clock can be used outside. Some may have internal frequency dividers which need to be handled in addition to the clock path in the clock controller.

For instance, on Exynos4 the FIMC devices belong to a power domain that needs to be enabled so the clock is not blocked, and this is done through the runtime PM calls.

Normally the host device driver runtime resumes the device when /dev/video* is opened. But we might want to use the clock before it happens, when only a /dev/v4l-subdev* is opened, to play with the sensor device only. In this situation the host device needs to be runtime resumed first.

Thus the driver would need to (re)implement the clock ops to also handle the details which are not covered by the clock controller driver.

I also wonder how could we support the boards which choose to use some extra external oscillator to provide clock to the sensors, rather than the one derived from the host.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

The following is usually handled in the board files:

1) sensor/frontend power supply
2) sensor's master clock (provided by the host device)

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

For 2) I'd like to propose adding a callback to struct v4l2_device, for instance as in the below patch. The host driver would implement such an operation and the sensor subdev driver would use it in its s_power op.

What about using a struct clk object ? There has been lots of work in the ARM tree to make struct clk generic. I'm not sure if all patches have been pushed to mainline yet, but I think that's the direction we should follow.

Quoted 2 lines

But is the 'struct clk' tried to be unified across all archs, not only ARM ? I'm afraid it's not the case.

If the goals haven't changed since https://lkml.org/lkml/2011/5/20/85, the new struct clk will be unified across all architectures.

Quoted 3 lines

By "using a struct clk object" do you also mean implementing some/all ops of this object by the driver which exports it ?

That's correct.

Quoted 16 lines

I suppose we can't rely only on the clock controller functionality exposed through the clock API.

Some devices may need to be brought to an active state before the clock can be used outside. Some may have internal frequency dividers which need to be handled in addition to the clock path in the clock controller.

For instance, on Exynos4 the FIMC devices belong to a power domain that needs to be enabled so the clock is not blocked, and this is done through the runtime PM calls.

Normally the host device driver runtime resumes the device when /dev/video* is opened. But we might want to use the clock before it happens, when only a /dev/v4l-subdev* is opened, to play with the sensor device only. In this situation the host device needs to be runtime resumed first.

Quoted 2 lines

Thus the driver would need to (re)implement the clock ops to also handle the details which are not covered by the clock controller driver.

The subdev driver would call clk_get(), which would end up being implemented by the driver for whatever hardware block provides the clock. The driver would then runtime_pm_resume() the hardware to start the clock.

Quoted 4 lines

I also wonder how could we support the boards which choose to use some extra external oscillator to provide clock to the sensors, rather than the one derived from the host.

In that case the clock is always running. I'm not sure if we should create a dummy clk object, or just pass a NULL clock and a fixed frequency to the sensor driver.