* [PATCH v2 0/2] media: v4l: Add support for the Cadence MIPI-CSI2 RX
@ 2017-07-20 9:23 Maxime Ripard
2017-07-20 9:23 ` [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings Maxime Ripard
2017-07-20 9:23 ` [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver Maxime Ripard
0 siblings, 2 replies; 17+ messages in thread
From: Maxime Ripard @ 2017-07-20 9:23 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Mark Rutland, Rob Herring
Cc: Laurent Pinchart, linux-media, devicetree, Cyprian Wronka,
Neil Webb, Richard Sproul, Alan Douglas, Steve Creaney,
Thomas Petazzoni, Boris Brezillon, Niklas Söderlund,
Hans Verkuil, Sakari Ailus, Maxime Ripard
Hi,
Here is an attempt at supporting the MIPI-CSI2 RX block from Cadence.
This IP block is able to receive CSI data over up to 4 lanes, and
split it to over 4 streams. Those streams are basically the interfaces
to the video grabbers that will perform the capture.
It is able to map streams to both CSI datatypes and virtual channels,
dynamically. This is unclear at this point what the right way to
support it would be, so the driver only uses a static mapping between
the virtual channels and streams, and ignores the data types.
This serie depends on the version 5 of the serie "v4l2-async: add subnotifier
registration for subdevices" from Niklas Söderlund.
Let me know what you think!
Maxime
Changes from v1:
- Amended the DT bindings as suggested by Rob
- Rebase on top of 4.13-rc1 and latest Niklas' serie iteration
Maxime Ripard (2):
dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
v4l: cadence: Add Cadence MIPI-CSI2 RX driver
.../devicetree/bindings/media/cdns-csi2rx.txt | 87 +++++
drivers/media/platform/Kconfig | 1 +
drivers/media/platform/Makefile | 2 +
drivers/media/platform/cadence/Kconfig | 12 +
drivers/media/platform/cadence/Makefile | 1 +
drivers/media/platform/cadence/cdns-csi2rx.c | 413 +++++++++++++++++++++
6 files changed, 516 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt
create mode 100644 drivers/media/platform/cadence/Kconfig
create mode 100644 drivers/media/platform/cadence/Makefile
create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c
--
2.13.3
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-07-20 9:23 [PATCH v2 0/2] media: v4l: Add support for the Cadence MIPI-CSI2 RX Maxime Ripard
@ 2017-07-20 9:23 ` Maxime Ripard
2017-07-24 19:56 ` Rob Herring
` (2 more replies)
2017-07-20 9:23 ` [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver Maxime Ripard
1 sibling, 3 replies; 17+ messages in thread
From: Maxime Ripard @ 2017-07-20 9:23 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Mark Rutland, Rob Herring
Cc: Laurent Pinchart, linux-media, devicetree, Cyprian Wronka,
Neil Webb, Richard Sproul, Alan Douglas, Steve Creaney,
Thomas Petazzoni, Boris Brezillon, Niklas Söderlund,
Hans Verkuil, Sakari Ailus, Maxime Ripard
The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up to
4 CSI-2 lanes, and can route the frames to up to 4 streams, depending on
the hardware implementation.
It can operate with an external D-PHY, an internal one or no D-PHY at all
in some configurations.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
.../devicetree/bindings/media/cdns-csi2rx.txt | 87 ++++++++++++++++++++++
1 file changed, 87 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt
diff --git a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
new file mode 100644
index 000000000000..e08547abe885
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
@@ -0,0 +1,87 @@
+Cadence MIPI-CSI2 RX controller
+===============================
+
+The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to 4 CSI
+lanes in input, and 4 different pixel streams in output.
+
+Required properties:
+ - compatible: must be set to "cdns,csi2rx" and an SoC-specific compatible
+ - reg: base address and size of the memory mapped region
+ - clocks: phandles to the clocks driving the controller
+ - clock-names: must contain:
+ * sys_clk: main clock
+ * p_clk: register bank clock
+ * p_free_clk: free running register bank clock
+ * pixel_ifX_clk: pixel stream output clock, one for each stream
+ implemented in hardware, between 0 and 3
+ * dphy_rx_clk: D-PHY byte clock, if implemented in hardware
+ - phys: phandle to the external D-PHY
+ - phy-names: must contain dphy, if the implementation uses an
+ external D-PHY
+
+Required subnodes:
+ - ports: A ports node with endpoint definitions as defined in
+ Documentation/devicetree/bindings/media/video-interfaces.txt. The
+ first port subnode should be the input endpoint, the second one the
+ outputs
+
+ The output port should have as many endpoints as stream supported by
+ the hardware implementation, between 1 and 4, their ID being the
+ stream output number used in the implementation.
+
+Example:
+
+csi2rx: csi-bridge@0d060000 {
+ compatible = "cdns,csi2rx";
+ reg = <0x0d060000 0x1000>;
+ clocks = <&byteclock>, <&byteclock>, <&byteclock>,
+ <&coreclock>, <&coreclock>,
+ <&coreclock>, <&coreclock>;
+ clock-names = "sys_clk", "p_clk", "p_free_clk",
+ "pixel_if0_clk", "pixel_if1_clk",
+ "pixel_if2_clk", "pixel_if3_clk";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ csi2rx_in_sensor: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <&sensor_out_csi2rx>;
+ clock-lanes = <0>;
+ data-lanes = <1 2>;
+ };
+ };
+
+ port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ csi2rx_out_grabber0: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <&grabber0_in_csi2rx>;
+ };
+
+ csi2rx_out_grabber1: endpoint@1 {
+ reg = <1>;
+ remote-endpoint = <&grabber1_in_csi2rx>;
+ };
+
+ csi2rx_out_grabber2: endpoint@2 {
+ reg = <2>;
+ remote-endpoint = <&grabber2_in_csi2rx>;
+ };
+
+ csi2rx_out_grabber3: endpoint@3 {
+ reg = <3>;
+ remote-endpoint = <&grabber3_in_csi2rx>;
+ };
+ };
+ };
+};
--
2.13.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
2017-07-20 9:23 [PATCH v2 0/2] media: v4l: Add support for the Cadence MIPI-CSI2 RX Maxime Ripard
2017-07-20 9:23 ` [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings Maxime Ripard
@ 2017-07-20 9:23 ` Maxime Ripard
2017-07-23 1:09 ` kbuild test robot
` (2 more replies)
1 sibling, 3 replies; 17+ messages in thread
From: Maxime Ripard @ 2017-07-20 9:23 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Mark Rutland, Rob Herring
Cc: Laurent Pinchart, linux-media, devicetree, Cyprian Wronka,
Neil Webb, Richard Sproul, Alan Douglas, Steve Creaney,
Thomas Petazzoni, Boris Brezillon, Niklas Söderlund,
Hans Verkuil, Sakari Ailus, Maxime Ripard
The Cadence CSI-2 RX Controller is an hardware block meant to be used as a
bridge between a CSI-2 bus and pixel grabbers.
It supports operating with internal or external D-PHY, with up to 4 lanes,
or without any D-PHY. The current code only supports the former case.
It also support dynamic mapping of the CSI-2 virtual channels to the
associated pixel grabbers, but that isn't allowed at the moment either.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
drivers/media/platform/Kconfig | 1 +
drivers/media/platform/Makefile | 2 +
drivers/media/platform/cadence/Kconfig | 12 +
drivers/media/platform/cadence/Makefile | 1 +
drivers/media/platform/cadence/cdns-csi2rx.c | 413 +++++++++++++++++++++++++++
5 files changed, 429 insertions(+)
create mode 100644 drivers/media/platform/cadence/Kconfig
create mode 100644 drivers/media/platform/cadence/Makefile
create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 1313cd533436..a79d96e9b723 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -26,6 +26,7 @@ config VIDEO_VIA_CAMERA
#
# Platform multimedia device configuration
#
+source "drivers/media/platform/cadence/Kconfig"
source "drivers/media/platform/davinci/Kconfig"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 9beadc760467..1d31eb51e9bb 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -2,6 +2,8 @@
# Makefile for the video capture/playback device drivers.
#
+obj-$(CONFIG_VIDEO_CADENCE) += cadence/
+
obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
diff --git a/drivers/media/platform/cadence/Kconfig b/drivers/media/platform/cadence/Kconfig
new file mode 100644
index 000000000000..d1b6bbb6a0eb
--- /dev/null
+++ b/drivers/media/platform/cadence/Kconfig
@@ -0,0 +1,12 @@
+config VIDEO_CADENCE
+ bool "Cadence Video Devices"
+
+if VIDEO_CADENCE
+
+config VIDEO_CADENCE_CSI2RX
+ tristate "Cadence MIPI-CSI2 RX Controller v1.3"
+ depends on MEDIA_CONTROLLER
+ depends on VIDEO_V4L2_SUBDEV_API
+ select V4L2_FWNODE
+
+endif
diff --git a/drivers/media/platform/cadence/Makefile b/drivers/media/platform/cadence/Makefile
new file mode 100644
index 000000000000..99a4086b7448
--- /dev/null
+++ b/drivers/media/platform/cadence/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_CADENCE_CSI2RX) += cdns-csi2rx.o
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
new file mode 100644
index 000000000000..9a58f275f53c
--- /dev/null
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -0,0 +1,413 @@
+/*
+ * Driver for Cadence MIPI-CSI2 RX Controller v1.3
+ *
+ * Copyright (C) 2017 Cadence Design Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define CSI2RX_DEVICE_CFG_REG 0x000
+
+#define CSI2RX_SOFT_RESET_REG 0x004
+#define CSI2RX_SOFT_RESET_PROTOCOL BIT(1)
+#define CSI2RX_SOFT_RESET_FRONT BIT(0)
+
+#define CSI2RX_STATIC_CFG_REG 0x008
+
+#define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100)
+
+#define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000)
+#define CSI2RX_STREAM_CTRL_START BIT(0)
+
+#define CSI2RX_STREAM_DATA_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x008)
+#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT BIT(31)
+#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n) BIT((n) + 16)
+
+#define CSI2RX_STREAM_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x00c)
+#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF (1 << 8)
+
+#define CSI2RX_STREAMS_MAX 4
+
+enum csi2rx_pads {
+ CSI2RX_PAD_SINK,
+ CSI2RX_PAD_SOURCE_VC0,
+ CSI2RX_PAD_SOURCE_VC1,
+ CSI2RX_PAD_SOURCE_VC2,
+ CSI2RX_PAD_SOURCE_VC3,
+ CSI2RX_PAD_MAX,
+};
+
+struct csi2rx_priv {
+ struct device *dev;
+
+ void __iomem *base;
+ struct clk *sys_clk;
+ struct clk *p_clk;
+ struct clk *p_free_clk;
+ struct clk *pixel_clk[CSI2RX_STREAMS_MAX];
+ struct clk *dphy_rx_clk;
+
+ u8 lanes;
+ u8 max_lanes;
+ u8 max_streams;
+ bool cdns_dphy;
+
+ struct v4l2_subdev subdev;
+ struct media_pad pads[CSI2RX_PAD_MAX];
+
+ /* Remote sensor */
+ struct v4l2_async_subdev asd;
+ struct device_node *sensor_node;
+ struct v4l2_subdev *sensor_subdev;
+ int sensor_pad;
+};
+
+static inline
+struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
+{
+ return container_of(subdev, struct csi2rx_priv, subdev);
+}
+
+static void csi2rx_reset(struct csi2rx_priv *csi2rx)
+{
+ writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
+ csi2rx->base + CSI2RX_SOFT_RESET_REG);
+
+ udelay(10);
+
+ writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
+}
+
+static int csi2rx_start(struct csi2rx_priv *csi2rx)
+{
+ u32 reg;
+ int i;
+
+ csi2rx_reset(csi2rx);
+
+ // TODO: modify the mapping of the DPHY lanes?
+ reg = readl(csi2rx->base + CSI2RX_STATIC_CFG_REG);
+ reg &= ~GENMASK(11, 8);
+ writel(reg | (csi2rx->lanes << 8),
+ csi2rx->base + CSI2RX_STATIC_CFG_REG);
+
+ /*
+ * Create a static mapping between the CSI virtual channels
+ * and the output stream.
+ *
+ * This should be enhanced, but v4l2 lacks the support for
+ * changing that mapping dynamically.
+ */
+ for (i = 0; i < csi2rx->max_streams; i++) {
+ writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
+ csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
+
+ writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |
+ CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
+ csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
+
+ writel(CSI2RX_STREAM_CTRL_START,
+ csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+ }
+
+ return 0;
+}
+
+static int csi2rx_stop(struct csi2rx_priv *csi2rx)
+{
+ int i;
+
+ for (i = 0; i < csi2rx->max_streams; i++)
+ writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+
+ return 0;
+}
+
+static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
+
+ v4l2_subdev_call(csi2rx->sensor_subdev, video, s_stream,
+ enable);
+
+ if (enable)
+ csi2rx_start(csi2rx);
+ else
+ csi2rx_stop(csi2rx);
+
+ return 0;
+}
+
+static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
+ .s_stream = csi2rx_s_stream,
+};
+
+static struct v4l2_subdev_ops csi2rx_subdev_ops = {
+ .video = &csi2rx_video_ops,
+};
+
+static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *s_subdev,
+ struct v4l2_async_subdev *asd)
+{
+ struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ csi2rx->sensor_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
+ &csi2rx->sensor_node->fwnode,
+ MEDIA_PAD_FL_SOURCE);
+ if (csi2rx->sensor_pad < 0) {
+ dev_err(csi2rx->dev, "Couldn't find output pad for subdev %s\n",
+ s_subdev->name);
+ return csi2rx->sensor_pad;
+ }
+
+ csi2rx->sensor_subdev = s_subdev;
+
+ dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
+ csi2rx->sensor_pad);
+
+ return 0;
+}
+
+static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
+{
+ struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ return media_create_pad_link(&csi2rx->sensor_subdev->entity,
+ csi2rx->sensor_pad,
+ &csi2rx->subdev.entity, 0,
+ MEDIA_LNK_FL_ENABLED |
+ MEDIA_LNK_FL_IMMUTABLE);
+}
+
+static void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *s_subdev,
+ struct v4l2_async_subdev *asd)
+{
+ struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
+ struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+ dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
+ csi2rx->sensor_pad);
+
+ csi2rx->sensor_subdev = NULL;
+ csi2rx->sensor_pad = -EINVAL;
+}
+
+static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
+ struct platform_device *pdev)
+{
+ struct resource *res;
+ u32 reg;
+ int i;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(csi2rx->base)) {
+ dev_err(&pdev->dev, "Couldn't map our registers\n");
+ return PTR_ERR(csi2rx->base);
+ }
+
+ reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
+ csi2rx->max_lanes = (reg & 7) + 1;
+ csi2rx->max_streams = ((reg >> 4) & 7);
+ csi2rx->cdns_dphy = reg & BIT(3);
+
+ csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
+ if (IS_ERR(csi2rx->sys_clk)) {
+ dev_err(&pdev->dev, "Couldn't get sys clock\n");
+ return PTR_ERR(csi2rx->sys_clk);
+ }
+
+ csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
+ if (IS_ERR(csi2rx->p_clk)) {
+ dev_err(&pdev->dev, "Couldn't get P clock\n");
+ return PTR_ERR(csi2rx->p_clk);
+ }
+
+ csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
+ if (IS_ERR(csi2rx->p_free_clk)) {
+ dev_err(&pdev->dev, "Couldn't get free running P clock\n");
+ return PTR_ERR(csi2rx->p_free_clk);
+ }
+
+ for (i = 0; i < csi2rx->max_streams; i++) {
+ char clk_name[16];
+
+ snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
+ csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
+ if (IS_ERR(csi2rx->pixel_clk[i])) {
+ dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
+ return PTR_ERR(csi2rx->pixel_clk[i]);
+ }
+ }
+
+ if (csi2rx->cdns_dphy) {
+ csi2rx->dphy_rx_clk = devm_clk_get(&pdev->dev, "dphy_rx_clk");
+ if (IS_ERR(csi2rx->dphy_rx_clk)) {
+ dev_err(&pdev->dev, "Couldn't get D-PHY RX clock\n");
+ return PTR_ERR(csi2rx->dphy_rx_clk);
+ }
+ }
+
+ return 0;
+}
+
+static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
+{
+ struct v4l2_fwnode_endpoint v4l2_ep;
+ struct v4l2_async_subdev **subdevs;
+ struct device_node *ep, *remote;
+ int ret = 0;
+
+ ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
+ if (!ep)
+ return -EINVAL;
+
+ ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
+ if (ret) {
+ dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
+ goto out;
+ }
+
+ if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
+ dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
+ v4l2_ep.bus_type);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ csi2rx->lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
+ if (csi2rx->lanes > csi2rx->max_lanes) {
+ dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
+ csi2rx->lanes);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ remote = of_graph_get_remote_port_parent(ep);
+ if (!remote) {
+ dev_err(csi2rx->dev, "No device found for endpoint %pOF\n", ep);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
+
+ csi2rx->sensor_node = remote;
+ csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
+ csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+
+ subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
+ if (subdevs == NULL)
+ return -ENOMEM;
+ subdevs[0] = &csi2rx->asd;
+
+ ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
+ csi2rx_async_bound,
+ csi2rx_async_complete,
+ csi2rx_async_unbind);
+ if (ret < 0) {
+ dev_err(csi2rx->dev, "Failed to register our notifier\n");
+ return ret;
+ }
+
+out:
+ of_node_put(ep);
+ return ret;
+}
+
+static int csi2rx_probe(struct platform_device *pdev)
+{
+ struct csi2rx_priv *csi2rx;
+ int i, ret;
+
+ csi2rx = devm_kzalloc(&pdev->dev, sizeof(*csi2rx), GFP_KERNEL);
+ if (!csi2rx)
+ return -ENOMEM;
+ platform_set_drvdata(pdev, csi2rx);
+ csi2rx->dev = &pdev->dev;
+
+ ret = csi2rx_get_resources(csi2rx, pdev);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to get our resources\n");
+ return ret;
+ }
+
+ ret = csi2rx_parse_dt(csi2rx);
+ if (ret)
+ return ret;
+
+ csi2rx->subdev.owner = THIS_MODULE;
+ csi2rx->subdev.dev = &pdev->dev;
+ v4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);
+ v4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);
+ snprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
+ KBUILD_MODNAME, dev_name(&pdev->dev));
+
+ /* Create our media pads */
+ csi2rx->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
+ csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+ for (i = CSI2RX_PAD_SOURCE_VC0; i < CSI2RX_PAD_MAX; i++)
+ csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
+
+ ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
+ csi2rx->pads);
+ if (ret)
+ return ret;
+
+ ret = v4l2_async_register_subdev(&csi2rx->subdev);
+ if (ret < 0)
+ return ret;
+
+ dev_info(&pdev->dev,
+ "Probed CSI2RX with %d/%d lanes, %d streams, %s D-PHY\n",
+ csi2rx->lanes, csi2rx->max_lanes, csi2rx->max_streams,
+ csi2rx->cdns_dphy ? "Cadence" : "no");
+
+ return 0;
+}
+
+static int csi2rx_remove(struct platform_device *pdev)
+{
+ struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev);
+
+ v4l2_async_unregister_subdev(&csi2rx->subdev);
+
+ return 0;
+}
+
+static const struct of_device_id csi2rx_of_table[] = {
+ { .compatible = "cdns,csi2rx" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, csi2rx_of_table);
+
+static struct platform_driver csi2rx_driver = {
+ .probe = csi2rx_probe,
+ .remove = csi2rx_remove,
+
+ .driver = {
+ .name = "cdns-csi2rx",
+ .of_match_table = csi2rx_of_table,
+ },
+};
+module_platform_driver(csi2rx_driver);
--
2.13.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
2017-07-20 9:23 ` [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver Maxime Ripard
@ 2017-07-23 1:09 ` kbuild test robot
2017-08-07 19:24 ` Benoit Parrot
2017-08-07 20:42 ` Laurent Pinchart
2 siblings, 0 replies; 17+ messages in thread
From: kbuild test robot @ 2017-07-23 1:09 UTC (permalink / raw)
To: Maxime Ripard
Cc: kbuild-all, Mauro Carvalho Chehab, Mark Rutland, Rob Herring,
Laurent Pinchart, linux-media, devicetree, Cyprian Wronka,
Neil Webb, Richard Sproul, Alan Douglas, Steve Creaney,
Thomas Petazzoni, Boris Brezillon, Niklas Söderlund,
Hans Verkuil, Sakari Ailus, Maxime Ripard
[-- Attachment #1: Type: text/plain, Size: 8667 bytes --]
Hi Maxime,
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.13-rc1 next-20170721]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Maxime-Ripard/media-v4l-Add-support-for-the-Cadence-MIPI-CSI2-RX/20170723-083419
base: git://linuxtv.org/media_tree.git master
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64
All error/warnings (new ones prefixed by >>):
drivers/media/platform/cadence/cdns-csi2rx.c: In function 'csi2rx_async_bound':
>> drivers/media/platform/cadence/cdns-csi2rx.c:169:31: error: implicit declaration of function 'subnotifier_to_v4l2_subdev' [-Werror=implicit-function-declaration]
struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/platform/cadence/cdns-csi2rx.c:169:31: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
drivers/media/platform/cadence/cdns-csi2rx.c: In function 'csi2rx_async_complete':
drivers/media/platform/cadence/cdns-csi2rx.c:191:31: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/platform/cadence/cdns-csi2rx.c: In function 'csi2rx_async_unbind':
drivers/media/platform/cadence/cdns-csi2rx.c:205:31: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/media/platform/cadence/cdns-csi2rx.c: In function 'csi2rx_parse_dt':
>> drivers/media/platform/cadence/cdns-csi2rx.c:324:8: error: implicit declaration of function 'v4l2_async_subdev_notifier_register' [-Werror=implicit-function-declaration]
ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/subnotifier_to_v4l2_subdev +169 drivers/media/platform/cadence/cdns-csi2rx.c
164
165 static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
166 struct v4l2_subdev *s_subdev,
167 struct v4l2_async_subdev *asd)
168 {
> 169 struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
170 struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
171
172 csi2rx->sensor_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
173 &csi2rx->sensor_node->fwnode,
174 MEDIA_PAD_FL_SOURCE);
175 if (csi2rx->sensor_pad < 0) {
176 dev_err(csi2rx->dev, "Couldn't find output pad for subdev %s\n",
177 s_subdev->name);
178 return csi2rx->sensor_pad;
179 }
180
181 csi2rx->sensor_subdev = s_subdev;
182
183 dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
184 csi2rx->sensor_pad);
185
186 return 0;
187 }
188
189 static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
190 {
> 191 struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
192 struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
193
194 return media_create_pad_link(&csi2rx->sensor_subdev->entity,
195 csi2rx->sensor_pad,
196 &csi2rx->subdev.entity, 0,
197 MEDIA_LNK_FL_ENABLED |
198 MEDIA_LNK_FL_IMMUTABLE);
199 }
200
201 static void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
202 struct v4l2_subdev *s_subdev,
203 struct v4l2_async_subdev *asd)
204 {
205 struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
206 struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
207
208 dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
209 csi2rx->sensor_pad);
210
211 csi2rx->sensor_subdev = NULL;
212 csi2rx->sensor_pad = -EINVAL;
213 }
214
215 static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
216 struct platform_device *pdev)
217 {
218 struct resource *res;
219 u32 reg;
220 int i;
221
222 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
223 csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
224 if (IS_ERR(csi2rx->base)) {
225 dev_err(&pdev->dev, "Couldn't map our registers\n");
226 return PTR_ERR(csi2rx->base);
227 }
228
229 reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
230 csi2rx->max_lanes = (reg & 7) + 1;
231 csi2rx->max_streams = ((reg >> 4) & 7);
232 csi2rx->cdns_dphy = reg & BIT(3);
233
234 csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
235 if (IS_ERR(csi2rx->sys_clk)) {
236 dev_err(&pdev->dev, "Couldn't get sys clock\n");
237 return PTR_ERR(csi2rx->sys_clk);
238 }
239
240 csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
241 if (IS_ERR(csi2rx->p_clk)) {
242 dev_err(&pdev->dev, "Couldn't get P clock\n");
243 return PTR_ERR(csi2rx->p_clk);
244 }
245
246 csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
247 if (IS_ERR(csi2rx->p_free_clk)) {
248 dev_err(&pdev->dev, "Couldn't get free running P clock\n");
249 return PTR_ERR(csi2rx->p_free_clk);
250 }
251
252 for (i = 0; i < csi2rx->max_streams; i++) {
253 char clk_name[16];
254
255 snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
256 csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
257 if (IS_ERR(csi2rx->pixel_clk[i])) {
258 dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
259 return PTR_ERR(csi2rx->pixel_clk[i]);
260 }
261 }
262
263 if (csi2rx->cdns_dphy) {
264 csi2rx->dphy_rx_clk = devm_clk_get(&pdev->dev, "dphy_rx_clk");
265 if (IS_ERR(csi2rx->dphy_rx_clk)) {
266 dev_err(&pdev->dev, "Couldn't get D-PHY RX clock\n");
267 return PTR_ERR(csi2rx->dphy_rx_clk);
268 }
269 }
270
271 return 0;
272 }
273
274 static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
275 {
276 struct v4l2_fwnode_endpoint v4l2_ep;
277 struct v4l2_async_subdev **subdevs;
278 struct device_node *ep, *remote;
279 int ret = 0;
280
281 ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
282 if (!ep)
283 return -EINVAL;
284
285 ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
286 if (ret) {
287 dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
288 goto out;
289 }
290
291 if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
292 dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
293 v4l2_ep.bus_type);
294 ret = -EINVAL;
295 goto out;
296 }
297
298 csi2rx->lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
299 if (csi2rx->lanes > csi2rx->max_lanes) {
300 dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
301 csi2rx->lanes);
302 ret = -EINVAL;
303 goto out;
304 }
305
306 remote = of_graph_get_remote_port_parent(ep);
307 if (!remote) {
308 dev_err(csi2rx->dev, "No device found for endpoint %pOF\n", ep);
309 ret = -EINVAL;
310 goto out;
311 }
312
313 dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
314
315 csi2rx->sensor_node = remote;
316 csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
317 csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
318
319 subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
320 if (subdevs == NULL)
321 return -ENOMEM;
322 subdevs[0] = &csi2rx->asd;
323
> 324 ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
325 csi2rx_async_bound,
326 csi2rx_async_complete,
327 csi2rx_async_unbind);
328 if (ret < 0) {
329 dev_err(csi2rx->dev, "Failed to register our notifier\n");
330 return ret;
331 }
332
333 out:
334 of_node_put(ep);
335 return ret;
336 }
337
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48411 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-07-20 9:23 ` [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings Maxime Ripard
@ 2017-07-24 19:56 ` Rob Herring
2017-08-07 19:56 ` Benoit Parrot
2017-08-07 20:18 ` Laurent Pinchart
2 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2017-07-24 19:56 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mauro Carvalho Chehab, Mark Rutland, Laurent Pinchart,
linux-media, devicetree, Cyprian Wronka, Neil Webb,
Richard Sproul, Alan Douglas, Steve Creaney, Thomas Petazzoni,
Boris Brezillon, Niklas Söderlund, Hans Verkuil,
Sakari Ailus
On Thu, Jul 20, 2017 at 11:23:01AM +0200, Maxime Ripard wrote:
> The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up to
> 4 CSI-2 lanes, and can route the frames to up to 4 streams, depending on
> the hardware implementation.
>
> It can operate with an external D-PHY, an internal one or no D-PHY at all
> in some configurations.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> .../devicetree/bindings/media/cdns-csi2rx.txt | 87 ++++++++++++++++++++++
> 1 file changed, 87 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
2017-07-20 9:23 ` [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver Maxime Ripard
2017-07-23 1:09 ` kbuild test robot
@ 2017-08-07 19:24 ` Benoit Parrot
2017-08-25 14:03 ` Maxime Ripard
2017-08-07 20:42 ` Laurent Pinchart
2 siblings, 1 reply; 17+ messages in thread
From: Benoit Parrot @ 2017-08-07 19:24 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring,
Laurent Pinchart, linux-media, devicetree, Cyprian Wronka,
Neil Webb, Richard Sproul, Alan Douglas, Steve Creaney,
Thomas Petazzoni, Boris Brezillon, Niklas Söderlund,
Hans Verkuil, Sakari Ailus
Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Thu [2017-Jul-20 11:23:02 +0200]:
> The Cadence CSI-2 RX Controller is an hardware block meant to be used as a
> bridge between a CSI-2 bus and pixel grabbers.
>
> It supports operating with internal or external D-PHY, with up to 4 lanes,
> or without any D-PHY. The current code only supports the former case.
>
> It also support dynamic mapping of the CSI-2 virtual channels to the
> associated pixel grabbers, but that isn't allowed at the moment either.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> drivers/media/platform/Kconfig | 1 +
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/cadence/Kconfig | 12 +
> drivers/media/platform/cadence/Makefile | 1 +
> drivers/media/platform/cadence/cdns-csi2rx.c | 413 +++++++++++++++++++++++++++
> 5 files changed, 429 insertions(+)
> create mode 100644 drivers/media/platform/cadence/Kconfig
> create mode 100644 drivers/media/platform/cadence/Makefile
> create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 1313cd533436..a79d96e9b723 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -26,6 +26,7 @@ config VIDEO_VIA_CAMERA
> #
> # Platform multimedia device configuration
> #
> +source "drivers/media/platform/cadence/Kconfig"
>
> source "drivers/media/platform/davinci/Kconfig"
>
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 9beadc760467..1d31eb51e9bb 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -2,6 +2,8 @@
> # Makefile for the video capture/playback device drivers.
> #
>
> +obj-$(CONFIG_VIDEO_CADENCE) += cadence/
> +
> obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
>
> obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
> diff --git a/drivers/media/platform/cadence/Kconfig b/drivers/media/platform/cadence/Kconfig
> new file mode 100644
> index 000000000000..d1b6bbb6a0eb
> --- /dev/null
> +++ b/drivers/media/platform/cadence/Kconfig
> @@ -0,0 +1,12 @@
> +config VIDEO_CADENCE
> + bool "Cadence Video Devices"
> +
> +if VIDEO_CADENCE
> +
> +config VIDEO_CADENCE_CSI2RX
> + tristate "Cadence MIPI-CSI2 RX Controller v1.3"
> + depends on MEDIA_CONTROLLER
> + depends on VIDEO_V4L2_SUBDEV_API
> + select V4L2_FWNODE
> +
> +endif
> diff --git a/drivers/media/platform/cadence/Makefile b/drivers/media/platform/cadence/Makefile
> new file mode 100644
> index 000000000000..99a4086b7448
> --- /dev/null
> +++ b/drivers/media/platform/cadence/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_CADENCE_CSI2RX) += cdns-csi2rx.o
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> new file mode 100644
> index 000000000000..9a58f275f53c
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -0,0 +1,413 @@
> +/*
> + * Driver for Cadence MIPI-CSI2 RX Controller v1.3
> + *
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define CSI2RX_DEVICE_CFG_REG 0x000
> +
> +#define CSI2RX_SOFT_RESET_REG 0x004
> +#define CSI2RX_SOFT_RESET_PROTOCOL BIT(1)
> +#define CSI2RX_SOFT_RESET_FRONT BIT(0)
> +
> +#define CSI2RX_STATIC_CFG_REG 0x008
> +
> +#define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100)
> +
> +#define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000)
> +#define CSI2RX_STREAM_CTRL_START BIT(0)
> +
> +#define CSI2RX_STREAM_DATA_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x008)
> +#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT BIT(31)
> +#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n) BIT((n) + 16)
> +
> +#define CSI2RX_STREAM_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x00c)
> +#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF (1 << 8)
> +
> +#define CSI2RX_STREAMS_MAX 4
> +
Just to confirm here that "streams" in this case are equivalent to
"Virtual Channel", correct?
> +enum csi2rx_pads {
> + CSI2RX_PAD_SINK,
> + CSI2RX_PAD_SOURCE_VC0,
> + CSI2RX_PAD_SOURCE_VC1,
> + CSI2RX_PAD_SOURCE_VC2,
> + CSI2RX_PAD_SOURCE_VC3,
> + CSI2RX_PAD_MAX,
> +};
> +
> +struct csi2rx_priv {
> + struct device *dev;
> +
> + void __iomem *base;
> + struct clk *sys_clk;
> + struct clk *p_clk;
> + struct clk *p_free_clk;
> + struct clk *pixel_clk[CSI2RX_STREAMS_MAX];
> + struct clk *dphy_rx_clk;
> +
> + u8 lanes;
> + u8 max_lanes;
> + u8 max_streams;
> + bool cdns_dphy;
> +
> + struct v4l2_subdev subdev;
> + struct media_pad pads[CSI2RX_PAD_MAX];
> +
> + /* Remote sensor */
> + struct v4l2_async_subdev asd;
> + struct device_node *sensor_node;
> + struct v4l2_subdev *sensor_subdev;
> + int sensor_pad;
> +};
> +
> +static inline
> +struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
> +{
> + return container_of(subdev, struct csi2rx_priv, subdev);
> +}
> +
> +static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> +{
> + writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
> + csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +
> + udelay(10);
Shouldn't we use usleep_range() instead?
> +
> + writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +}
> +
> +static int csi2rx_start(struct csi2rx_priv *csi2rx)
> +{
> + u32 reg;
> + int i;
> +
> + csi2rx_reset(csi2rx);
> +
> + // TODO: modify the mapping of the DPHY lanes?
> + reg = readl(csi2rx->base + CSI2RX_STATIC_CFG_REG);
> + reg &= ~GENMASK(11, 8);
> + writel(reg | (csi2rx->lanes << 8),
> + csi2rx->base + CSI2RX_STATIC_CFG_REG);
> +
> + /*
> + * Create a static mapping between the CSI virtual channels
> + * and the output stream.
> + *
> + * This should be enhanced, but v4l2 lacks the support for
> + * changing that mapping dynamically.
> + */
> + for (i = 0; i < csi2rx->max_streams; i++) {
> + writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> + csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> +
> + writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |
> + CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
> + csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
> +
> + writel(CSI2RX_STREAM_CTRL_START,
> + csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> + }
> +
> + return 0;
> +}
> +
> +static int csi2rx_stop(struct csi2rx_priv *csi2rx)
> +{
> + int i;
> +
> + for (i = 0; i < csi2rx->max_streams; i++)
> + writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +
> + return 0;
> +}
> +
Here, it is entirely possible that the "dma/buffer" engine which will
make use of this receiver creates separates video nodes for each streams.
In which case, you could theoretically have multiple user space capture
on-going.
But the "start" and "stop" method above would disrupt any of the other
stream. Unless you start and stop all 4 capture streams in lock step.
Eventually, the sub device might be a port aggregator which has up to
4 sensors on the source pad and feed each camera traffic on its own
Virtual Channel.
I know there isn't support in the framework for this currently but it
is something to think about.
Regards,
Benoit
> +static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> +
> + v4l2_subdev_call(csi2rx->sensor_subdev, video, s_stream,
> + enable);
> +
> + if (enable)
> + csi2rx_start(csi2rx);
> + else
> + csi2rx_stop(csi2rx);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> + .s_stream = csi2rx_s_stream,
> +};
> +
> +static struct v4l2_subdev_ops csi2rx_subdev_ops = {
> + .video = &csi2rx_video_ops,
> +};
> +
> +static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *s_subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + csi2rx->sensor_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
> + &csi2rx->sensor_node->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (csi2rx->sensor_pad < 0) {
> + dev_err(csi2rx->dev, "Couldn't find output pad for subdev %s\n",
> + s_subdev->name);
> + return csi2rx->sensor_pad;
> + }
> +
> + csi2rx->sensor_subdev = s_subdev;
> +
> + dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
> + csi2rx->sensor_pad);
> +
> + return 0;
> +}
> +
> +static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + return media_create_pad_link(&csi2rx->sensor_subdev->entity,
> + csi2rx->sensor_pad,
> + &csi2rx->subdev.entity, 0,
> + MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *s_subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
> + csi2rx->sensor_pad);
> +
> + csi2rx->sensor_subdev = NULL;
> + csi2rx->sensor_pad = -EINVAL;
> +}
> +
> +static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
> + struct platform_device *pdev)
> +{
> + struct resource *res;
> + u32 reg;
> + int i;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(csi2rx->base)) {
> + dev_err(&pdev->dev, "Couldn't map our registers\n");
> + return PTR_ERR(csi2rx->base);
> + }
> +
> + reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
> + csi2rx->max_lanes = (reg & 7) + 1;
> + csi2rx->max_streams = ((reg >> 4) & 7);
> + csi2rx->cdns_dphy = reg & BIT(3);
> +
> + csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
> + if (IS_ERR(csi2rx->sys_clk)) {
> + dev_err(&pdev->dev, "Couldn't get sys clock\n");
> + return PTR_ERR(csi2rx->sys_clk);
> + }
> +
> + csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
> + if (IS_ERR(csi2rx->p_clk)) {
> + dev_err(&pdev->dev, "Couldn't get P clock\n");
> + return PTR_ERR(csi2rx->p_clk);
> + }
> +
> + csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
> + if (IS_ERR(csi2rx->p_free_clk)) {
> + dev_err(&pdev->dev, "Couldn't get free running P clock\n");
> + return PTR_ERR(csi2rx->p_free_clk);
> + }
> +
> + for (i = 0; i < csi2rx->max_streams; i++) {
> + char clk_name[16];
> +
> + snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
> + csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
> + if (IS_ERR(csi2rx->pixel_clk[i])) {
> + dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
> + return PTR_ERR(csi2rx->pixel_clk[i]);
> + }
> + }
> +
> + if (csi2rx->cdns_dphy) {
> + csi2rx->dphy_rx_clk = devm_clk_get(&pdev->dev, "dphy_rx_clk");
> + if (IS_ERR(csi2rx->dphy_rx_clk)) {
> + dev_err(&pdev->dev, "Couldn't get D-PHY RX clock\n");
> + return PTR_ERR(csi2rx->dphy_rx_clk);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> +{
> + struct v4l2_fwnode_endpoint v4l2_ep;
> + struct v4l2_async_subdev **subdevs;
> + struct device_node *ep, *remote;
> + int ret = 0;
> +
> + ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
> + if (!ep)
> + return -EINVAL;
> +
> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> + if (ret) {
> + dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
> + goto out;
> + }
> +
> + if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
> + dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
> + v4l2_ep.bus_type);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + csi2rx->lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
> + if (csi2rx->lanes > csi2rx->max_lanes) {
> + dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
> + csi2rx->lanes);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + remote = of_graph_get_remote_port_parent(ep);
> + if (!remote) {
> + dev_err(csi2rx->dev, "No device found for endpoint %pOF\n", ep);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
> +
> + csi2rx->sensor_node = remote;
> + csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
> + csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +
> + subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
> + if (subdevs == NULL)
> + return -ENOMEM;
> + subdevs[0] = &csi2rx->asd;
> +
> + ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
> + csi2rx_async_bound,
> + csi2rx_async_complete,
> + csi2rx_async_unbind);
> + if (ret < 0) {
> + dev_err(csi2rx->dev, "Failed to register our notifier\n");
> + return ret;
> + }
> +
> +out:
> + of_node_put(ep);
> + return ret;
> +}
> +
> +static int csi2rx_probe(struct platform_device *pdev)
> +{
> + struct csi2rx_priv *csi2rx;
> + int i, ret;
> +
> + csi2rx = devm_kzalloc(&pdev->dev, sizeof(*csi2rx), GFP_KERNEL);
> + if (!csi2rx)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, csi2rx);
> + csi2rx->dev = &pdev->dev;
> +
> + ret = csi2rx_get_resources(csi2rx, pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to get our resources\n");
> + return ret;
> + }
> +
> + ret = csi2rx_parse_dt(csi2rx);
> + if (ret)
> + return ret;
> +
> + csi2rx->subdev.owner = THIS_MODULE;
> + csi2rx->subdev.dev = &pdev->dev;
> + v4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);
> + v4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);
> + snprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
> + KBUILD_MODNAME, dev_name(&pdev->dev));
> +
> + /* Create our media pads */
> + csi2rx->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> + csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> + for (i = CSI2RX_PAD_SOURCE_VC0; i < CSI2RX_PAD_MAX; i++)
> + csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> + ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
> + csi2rx->pads);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_async_register_subdev(&csi2rx->subdev);
> + if (ret < 0)
> + return ret;
> +
> + dev_info(&pdev->dev,
> + "Probed CSI2RX with %d/%d lanes, %d streams, %s D-PHY\n",
> + csi2rx->lanes, csi2rx->max_lanes, csi2rx->max_streams,
> + csi2rx->cdns_dphy ? "Cadence" : "no");
> +
> + return 0;
> +}
> +
> +static int csi2rx_remove(struct platform_device *pdev)
> +{
> + struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev);
> +
> + v4l2_async_unregister_subdev(&csi2rx->subdev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id csi2rx_of_table[] = {
> + { .compatible = "cdns,csi2rx" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, csi2rx_of_table);
> +
> +static struct platform_driver csi2rx_driver = {
> + .probe = csi2rx_probe,
> + .remove = csi2rx_remove,
> +
> + .driver = {
> + .name = "cdns-csi2rx",
> + .of_match_table = csi2rx_of_table,
> + },
> +};
> +module_platform_driver(csi2rx_driver);
> --
> 2.13.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-07-20 9:23 ` [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings Maxime Ripard
2017-07-24 19:56 ` Rob Herring
@ 2017-08-07 19:56 ` Benoit Parrot
2017-08-07 20:18 ` Laurent Pinchart
2 siblings, 0 replies; 17+ messages in thread
From: Benoit Parrot @ 2017-08-07 19:56 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring,
Laurent Pinchart, linux-media, devicetree, Cyprian Wronka,
Neil Webb, Richard Sproul, Alan Douglas, Steve Creaney,
Thomas Petazzoni, Boris Brezillon, Niklas Söderlund,
Hans Verkuil, Sakari Ailus
Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Thu [2017-Jul-20 11:23:01 +0200]:
> The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up to
> 4 CSI-2 lanes, and can route the frames to up to 4 streams, depending on
> the hardware implementation.
>
> It can operate with an external D-PHY, an internal one or no D-PHY at all
> in some configurations.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> .../devicetree/bindings/media/cdns-csi2rx.txt | 87 ++++++++++++++++++++++
> 1 file changed, 87 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt
>
Acked-by: Benoit Parrot <bparrot@ti.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-07-20 9:23 ` [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings Maxime Ripard
2017-07-24 19:56 ` Rob Herring
2017-08-07 19:56 ` Benoit Parrot
@ 2017-08-07 20:18 ` Laurent Pinchart
2017-08-22 8:53 ` Maxime Ripard
2 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2017-08-07 20:18 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring, linux-media,
devicetree, Cyprian Wronka, Neil Webb, Richard Sproul,
Alan Douglas, Steve Creaney, Thomas Petazzoni, Boris Brezillon,
Niklas Söderlund, Hans Verkuil, Sakari Ailus
Hi Maxime,
Thank you for the patch.
On Thursday 20 Jul 2017 11:23:01 Maxime Ripard wrote:
> The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up to
> 4 CSI-2 lanes, and can route the frames to up to 4 streams, depending on
> the hardware implementation.
>
> It can operate with an external D-PHY, an internal one or no D-PHY at all
> in some configurations.
Without any PHY ? I'm curious, how does that work ?
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> .../devicetree/bindings/media/cdns-csi2rx.txt | 87 ++++++++++++++++++
> 1 file changed, 87 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt
>
> diff --git a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt new file mode
> 100644
> index 000000000000..e08547abe885
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> @@ -0,0 +1,87 @@
> +Cadence MIPI-CSI2 RX controller
> +===============================
> +
> +The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to 4
> CSI
> +lanes in input, and 4 different pixel streams in output.
> +
> +Required properties:
> + - compatible: must be set to "cdns,csi2rx" and an SoC-specific compatible
> + - reg: base address and size of the memory mapped region
> + - clocks: phandles to the clocks driving the controller
> + - clock-names: must contain:
> + * sys_clk: main clock
> + * p_clk: register bank clock
> + * p_free_clk: free running register bank clock
> + * pixel_ifX_clk: pixel stream output clock, one for each stream
> + implemented in hardware, between 0 and 3
Nitpicking, I would write the name is pixel_if[0-3]_clk, it took me a few
seconds to see that the X was a placeholder.
> + * dphy_rx_clk: D-PHY byte clock, if implemented in hardware
> + - phys: phandle to the external D-PHY
> + - phy-names: must contain dphy, if the implementation uses an
> + external D-PHY
I would move the last two properties in an optional category as they're
effectively optional. I think you should also explain a bit more clearly that
the phys property must not be present if the phy-names property is not
present.
> +
> +Required subnodes:
> + - ports: A ports node with endpoint definitions as defined in
> + Documentation/devicetree/bindings/media/video-interfaces.txt.
> The
> + first port subnode should be the input endpoint, the second one
> the
> + outputs
> +
> + The output port should have as many endpoints as stream supported by
> + the hardware implementation, between 1 and 4, their ID being the
> + stream output number used in the implementation.
I don't think that's correct. The IP has four independent outputs, it should
have 4 output ports for a total for 5 ports. Multiple endpoints per port would
describe multiple connections from the same output to different sinks.
> +Example:
> +
> +csi2rx: csi-bridge@0d060000 {
> + compatible = "cdns,csi2rx";
> + reg = <0x0d060000 0x1000>;
> + clocks = <&byteclock>, <&byteclock>, <&byteclock>,
> + <&coreclock>, <&coreclock>,
> + <&coreclock>, <&coreclock>;
> + clock-names = "sys_clk", "p_clk", "p_free_clk",
> + "pixel_if0_clk", "pixel_if1_clk",
> + "pixel_if2_clk", "pixel_if3_clk";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> +
> + csi2rx_in_sensor: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&sensor_out_csi2rx>;
> + clock-lanes = <0>;
> + data-lanes = <1 2>;
> + };
> + };
> +
> + port@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> +
> + csi2rx_out_grabber0: endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&grabber0_in_csi2rx>;
> + };
> +
> + csi2rx_out_grabber1: endpoint@1 {
> + reg = <1>;
> + remote-endpoint = <&grabber1_in_csi2rx>;
> + };
> +
> + csi2rx_out_grabber2: endpoint@2 {
> + reg = <2>;
> + remote-endpoint = <&grabber2_in_csi2rx>;
> + };
> +
> + csi2rx_out_grabber3: endpoint@3 {
> + reg = <3>;
> + remote-endpoint = <&grabber3_in_csi2rx>;
> + };
> + };
> + };
> +};
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
2017-07-20 9:23 ` [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver Maxime Ripard
2017-07-23 1:09 ` kbuild test robot
2017-08-07 19:24 ` Benoit Parrot
@ 2017-08-07 20:42 ` Laurent Pinchart
2017-08-25 14:30 ` Maxime Ripard
2 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2017-08-07 20:42 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring, linux-media,
devicetree, Cyprian Wronka, Neil Webb, Richard Sproul,
Alan Douglas, Steve Creaney, Thomas Petazzoni, Boris Brezillon,
Niklas Söderlund, Hans Verkuil, Sakari Ailus
Hi Maxime,
Thank you for the patch.
On Thursday 20 Jul 2017 11:23:02 Maxime Ripard wrote:
> The Cadence CSI-2 RX Controller is an hardware block meant to be used as a
> bridge between a CSI-2 bus and pixel grabbers.
>
> It supports operating with internal or external D-PHY, with up to 4 lanes,
> or without any D-PHY. The current code only supports the former case.
>
> It also support dynamic mapping of the CSI-2 virtual channels to the
> associated pixel grabbers, but that isn't allowed at the moment either.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> drivers/media/platform/Kconfig | 1 +
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/cadence/Kconfig | 12 +
> drivers/media/platform/cadence/Makefile | 1 +
> drivers/media/platform/cadence/cdns-csi2rx.c | 413 ++++++++++++++++++++++++
> 5 files changed, 429 insertions(+)
> create mode 100644 drivers/media/platform/cadence/Kconfig
> create mode 100644 drivers/media/platform/cadence/Makefile
> create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c
[snip]
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> b/drivers/media/platform/cadence/cdns-csi2rx.c new file mode 100644
> index 000000000000..9a58f275f53c
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -0,0 +1,413 @@
> +/*
> + * Driver for Cadence MIPI-CSI2 RX Controller v1.3
> + *
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by
> the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define CSI2RX_DEVICE_CFG_REG 0x000
> +
> +#define CSI2RX_SOFT_RESET_REG 0x004
> +#define CSI2RX_SOFT_RESET_PROTOCOL BIT(1)
> +#define CSI2RX_SOFT_RESET_FRONT BIT(0)
> +
> +#define CSI2RX_STATIC_CFG_REG 0x008
> +
> +#define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100)
> +
> +#define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) +
0x000)
> +#define CSI2RX_STREAM_CTRL_START BIT(0)
> +
> +#define CSI2RX_STREAM_DATA_CFG_REG(n) (CSI2RX_STREAM_BASE(n)
+ 0x008)
> +#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT BIT(31)
> +#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n) BIT((n) + 16)
> +
> +#define CSI2RX_STREAM_CFG_REG(n) (CSI2RX_STREAM_BASE(n) +
0x00c)
> +#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF (1 << 8)
> +
> +#define CSI2RX_STREAMS_MAX 4
> +
> +enum csi2rx_pads {
> + CSI2RX_PAD_SINK,
> + CSI2RX_PAD_SOURCE_VC0,
> + CSI2RX_PAD_SOURCE_VC1,
> + CSI2RX_PAD_SOURCE_VC2,
> + CSI2RX_PAD_SOURCE_VC3,
> + CSI2RX_PAD_MAX,
> +};
> +
> +struct csi2rx_priv {
> + struct device *dev;
> +
> + void __iomem *base;
> + struct clk *sys_clk;
> + struct clk *p_clk;
> + struct clk *p_free_clk;
> + struct clk *pixel_clk[CSI2RX_STREAMS_MAX];
> + struct clk *dphy_rx_clk;
> +
> + u8 lanes;
> + u8 max_lanes;
> + u8 max_streams;
> + bool cdns_dphy;
> +
> + struct v4l2_subdev subdev;
> + struct media_pad pads[CSI2RX_PAD_MAX];
> +
> + /* Remote sensor */
> + struct v4l2_async_subdev asd;
> + struct device_node *sensor_node;
> + struct v4l2_subdev *sensor_subdev;
> + int sensor_pad;
The remove subdev might not be a sensor. I would rename those three fields to
source_* (don't forget to update the comment accordingly).
> +};
> +
> +static inline
> +struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
> +{
> + return container_of(subdev, struct csi2rx_priv, subdev);
> +}
> +
> +static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> +{
> + writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
> + csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +
> + udelay(10);
> +
> + writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +}
> +
> +static int csi2rx_start(struct csi2rx_priv *csi2rx)
> +{
> + u32 reg;
> + int i;
i is never negative, you can make it an unsigned int.
> + csi2rx_reset(csi2rx);
> +
> + // TODO: modify the mapping of the DPHY lanes?
The mapping should be read from DT and applied here.
> + reg = readl(csi2rx->base + CSI2RX_STATIC_CFG_REG);
> + reg &= ~GENMASK(11, 8);
Could you define a macro for this register field ?
> + writel(reg | (csi2rx->lanes << 8),
> + csi2rx->base + CSI2RX_STATIC_CFG_REG);
> +
> + /*
> + * Create a static mapping between the CSI virtual channels
> + * and the output stream.
> + *
> + * This should be enhanced, but v4l2 lacks the support for
> + * changing that mapping dynamically.
V4L2 also lacks the ability to start/stop streams independently. That will
hopefully be implemented at some point.
> + */
> + for (i = 0; i < csi2rx->max_streams; i++) {
> + writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> + csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> +
> + writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |
> + CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
> + csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
> +
> + writel(CSI2RX_STREAM_CTRL_START,
> + csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> + }
> +
> + return 0;
> +}
> +
> +static int csi2rx_stop(struct csi2rx_priv *csi2rx)
> +{
> + int i;
i is never negative, you can make it an unsigned int.
> + for (i = 0; i < csi2rx->max_streams; i++)
> + writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +
> + return 0;
> +}
> +
> +static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> +
> + v4l2_subdev_call(csi2rx->sensor_subdev, video, s_stream,
> + enable);
Shouldn't you handle errors here ?
I'm not familiar with this IP core, but most D-PHYs need to synchronize to the
input and must be started before the source.
> + if (enable)
> + csi2rx_start(csi2rx);
> + else
> + csi2rx_stop(csi2rx);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> + .s_stream = csi2rx_s_stream,
> +};
> +
> +static struct v4l2_subdev_ops csi2rx_subdev_ops = {
This structure can be made const.
> + .video = &csi2rx_video_ops,
> +};
> +
> +static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *s_subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + csi2rx->sensor_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
> + &csi2rx->sensor_node-
>fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (csi2rx->sensor_pad < 0) {
> + dev_err(csi2rx->dev, "Couldn't find output pad for subdev
%s\n",
> + s_subdev->name);
> + return csi2rx->sensor_pad;
> + }
> +
> + csi2rx->sensor_subdev = s_subdev;
> +
> + dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
> + csi2rx->sensor_pad);
> +
> + return 0;
> +}
> +
> +static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + return media_create_pad_link(&csi2rx->sensor_subdev->entity,
> + csi2rx->sensor_pad,
> + &csi2rx->subdev.entity, 0,
> + MEDIA_LNK_FL_ENABLED |
> + MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *s_subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> + dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
> + csi2rx->sensor_pad);
> +
> + csi2rx->sensor_subdev = NULL;
> + csi2rx->sensor_pad = -EINVAL;
> +}
> +
> +static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
> + struct platform_device *pdev)
> +{
> + struct resource *res;
> + u32 reg;
> + int i;
i is never negative, you can make it an unsigned int.
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(csi2rx->base)) {
> + dev_err(&pdev->dev, "Couldn't map our registers\n");
devm_ioremap_resource() prints an error message, no need to duplicate it.
> + return PTR_ERR(csi2rx->base);
> + }
> +
> + reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
Shouldn't you enable the register access clock(s) before reading the register
?
> + csi2rx->max_lanes = (reg & 7) + 1;
Up to 8 lanes ? Shouldn't this be (reg & 3) + 1 ?
> + csi2rx->max_streams = ((reg >> 4) & 7);
Should you validate the value as you use it as an array access index ?
> + csi2rx->cdns_dphy = reg & BIT(3);
> +
> + csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
> + if (IS_ERR(csi2rx->sys_clk)) {
> + dev_err(&pdev->dev, "Couldn't get sys clock\n");
If you wrote this as
dev_err(&pdev->dev, "Couldn't get %s clock\n", "sys");
and the next messages in a similar fashion, most of the message wouldn't be
duplicated, resulting in smaller code size.
> + return PTR_ERR(csi2rx->sys_clk);
> + }
> +
> + csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
> + if (IS_ERR(csi2rx->p_clk)) {
> + dev_err(&pdev->dev, "Couldn't get P clock\n");
> + return PTR_ERR(csi2rx->p_clk);
> + }
> +
> + csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
> + if (IS_ERR(csi2rx->p_free_clk)) {
> + dev_err(&pdev->dev, "Couldn't get free running P clock\n");
> + return PTR_ERR(csi2rx->p_free_clk);
> + }
> +
> + for (i = 0; i < csi2rx->max_streams; i++) {
> + char clk_name[16];
Isn't 13 enough ?
> +
> + snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
> + csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
> + if (IS_ERR(csi2rx->pixel_clk[i])) {
> + dev_err(&pdev->dev, "Couldn't get clock %s\n",
clk_name);
> + return PTR_ERR(csi2rx->pixel_clk[i]);
> + }
> + }
> +
> + if (csi2rx->cdns_dphy) {
> + csi2rx->dphy_rx_clk = devm_clk_get(&pdev->dev, "dphy_rx_clk");
> + if (IS_ERR(csi2rx->dphy_rx_clk)) {
> + dev_err(&pdev->dev, "Couldn't get D-PHY RX clock\n");
> + return PTR_ERR(csi2rx->dphy_rx_clk);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> +{
> + struct v4l2_fwnode_endpoint v4l2_ep;
> + struct v4l2_async_subdev **subdevs;
> + struct device_node *ep, *remote;
> + int ret = 0;
No need to initialize ret to 0.
> +
> + ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
> + if (!ep)
> + return -EINVAL;
> +
> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> + if (ret) {
> + dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
> + goto out;
> + }
> +
> + if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
> + dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
> + v4l2_ep.bus_type);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + csi2rx->lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
> + if (csi2rx->lanes > csi2rx->max_lanes) {
> + dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
> + csi2rx->lanes);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + remote = of_graph_get_remote_port_parent(ep);
> + if (!remote) {
> + dev_err(csi2rx->dev, "No device found for endpoint %pOF\n",
ep);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
> +
> + csi2rx->sensor_node = remote;
> + csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
> + csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +
> + subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
Could you store this in the csi2rx_priv structure to avoid a dynamic
allocation ?
> + if (subdevs == NULL)
> + return -ENOMEM;
> + subdevs[0] = &csi2rx->asd;
> +
> + ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
> + csi2rx_async_bound,
> + csi2rx_async_complete,
> + csi2rx_async_unbind);
> + if (ret < 0) {
> + dev_err(csi2rx->dev, "Failed to register our notifier\n");
> + return ret;
> + }
I would register the notifier in the probe function, as this is not DT
parsing.
> +out:
> + of_node_put(ep);
> + return ret;
> +}
> +
> +static int csi2rx_probe(struct platform_device *pdev)
> +{
> + struct csi2rx_priv *csi2rx;
> + int i, ret;
i is never negative, you can make it an unsigned int.
> +
> + csi2rx = devm_kzalloc(&pdev->dev, sizeof(*csi2rx), GFP_KERNEL);
Please don't use devm_kzalloc() to allocate structures that can be accessed
from userspace (in this case because it embeds the subdev structure,
accessible at least through the subdevs ioctls). This is incompatible with
proper unplug handling. There are many other issues that we will need to solve
in the V4L2 core to handling unplugging properly, but let's not add a new one.
> + if (!csi2rx)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, csi2rx);
> + csi2rx->dev = &pdev->dev;
> +
> + ret = csi2rx_get_resources(csi2rx, pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to get our resources\n");
You already print error messages in the csi2rx_get_resources() function,
there's no need to add a new one.
> + return ret;
> + }
> +
> + ret = csi2rx_parse_dt(csi2rx);
> + if (ret)
> + return ret;
> +
> + csi2rx->subdev.owner = THIS_MODULE;
> + csi2rx->subdev.dev = &pdev->dev;
> + v4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);
> + v4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);
> + snprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
> + KBUILD_MODNAME, dev_name(&pdev->dev));
> +
> + /* Create our media pads */
> + csi2rx->subdev.entity.function =
MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> + csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> + for (i = CSI2RX_PAD_SOURCE_VC0; i < CSI2RX_PAD_MAX; i++)
> + csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> + ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
> + csi2rx->pads);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_async_register_subdev(&csi2rx->subdev);
> + if (ret < 0)
> + return ret;
> +
> + dev_info(&pdev->dev,
> + "Probed CSI2RX with %d/%d lanes, %d streams, %s D-PHY\n",
Please use %u for unsigned integers.
> + csi2rx->lanes, csi2rx->max_lanes, csi2rx->max_streams,
> + csi2rx->cdns_dphy ? "Cadence" : "no");
As the driver currently doesn't support external PHYs, should you treat the
presence of an external PHY in DT as an error ?
> + return 0;
> +}
> +
> +static int csi2rx_remove(struct platform_device *pdev)
> +{
> + struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev);
> +
> + v4l2_async_unregister_subdev(&csi2rx->subdev);
> +
> + return 0;
> +}
[snip]
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-08-07 20:18 ` Laurent Pinchart
@ 2017-08-22 8:53 ` Maxime Ripard
2017-08-22 9:01 ` Laurent Pinchart
0 siblings, 1 reply; 17+ messages in thread
From: Maxime Ripard @ 2017-08-22 8:53 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring, linux-media,
devicetree, Cyprian Wronka, Neil Webb, Richard Sproul,
Alan Douglas, Steve Creaney, Thomas Petazzoni, Boris Brezillon,
Niklas Söderlund, Hans Verkuil, Sakari Ailus
[-- Attachment #1: Type: text/plain, Size: 3986 bytes --]
Hi Laurent,
Thanks a lot for reviewing those patches.
On Mon, Aug 07, 2017 at 11:18:03PM +0300, Laurent Pinchart wrote:
> Hi Maxime,
>
> Thank you for the patch.
>
> On Thursday 20 Jul 2017 11:23:01 Maxime Ripard wrote:
> > The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up to
> > 4 CSI-2 lanes, and can route the frames to up to 4 streams, depending on
> > the hardware implementation.
> >
> > It can operate with an external D-PHY, an internal one or no D-PHY at all
> > in some configurations.
>
> Without any PHY ? I'm curious, how does that work ?
We're currently working on an FPGA exactly with that configuration. So
I guess the answer would be "it doesn't on an ASIC" :)
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> > .../devicetree/bindings/media/cdns-csi2rx.txt | 87 ++++++++++++++++++
> > 1 file changed, 87 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> >
> > diff --git a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> > b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt new file mode
> > 100644
> > index 000000000000..e08547abe885
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> > @@ -0,0 +1,87 @@
> > +Cadence MIPI-CSI2 RX controller
> > +===============================
> > +
> > +The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to 4
> > CSI
> > +lanes in input, and 4 different pixel streams in output.
> > +
> > +Required properties:
> > + - compatible: must be set to "cdns,csi2rx" and an SoC-specific compatible
> > + - reg: base address and size of the memory mapped region
> > + - clocks: phandles to the clocks driving the controller
> > + - clock-names: must contain:
> > + * sys_clk: main clock
> > + * p_clk: register bank clock
> > + * p_free_clk: free running register bank clock
> > + * pixel_ifX_clk: pixel stream output clock, one for each stream
> > + implemented in hardware, between 0 and 3
>
> Nitpicking, I would write the name is pixel_if[0-3]_clk, it took me a few
> seconds to see that the X was a placeholder.
Ok.
> > + * dphy_rx_clk: D-PHY byte clock, if implemented in hardware
> > + - phys: phandle to the external D-PHY
> > + - phy-names: must contain dphy, if the implementation uses an
> > + external D-PHY
>
> I would move the last two properties in an optional category as they're
> effectively optional. I think you should also explain a bit more clearly that
> the phys property must not be present if the phy-names property is not
> present.
It's not really optional. The IP has a configuration register that
allows you to see if it's been synthesized with or without a PHY. If
the right bit is set, that property will be mandatory, if not, it's
useless.
Maybe it's just semantics, but to me, optional means that it can
operate with or without it under any circumstances. It's not really
the case here.
> > +
> > +Required subnodes:
> > + - ports: A ports node with endpoint definitions as defined in
> > + Documentation/devicetree/bindings/media/video-interfaces.txt.
> > The
> > + first port subnode should be the input endpoint, the second one
> > the
> > + outputs
> > +
> > + The output port should have as many endpoints as stream supported by
> > + the hardware implementation, between 1 and 4, their ID being the
> > + stream output number used in the implementation.
>
> I don't think that's correct. The IP has four independent outputs, it should
> have 4 output ports for a total for 5 ports. Multiple endpoints per port would
> describe multiple connections from the same output to different sinks.
Ok.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-08-22 8:53 ` Maxime Ripard
@ 2017-08-22 9:01 ` Laurent Pinchart
2017-08-22 19:25 ` Cyprian Wronka
0 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2017-08-22 9:01 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring, linux-media,
devicetree, Cyprian Wronka, Neil Webb, Richard Sproul,
Alan Douglas, Steve Creaney, Thomas Petazzoni, Boris Brezillon,
Niklas Söderlund, Hans Verkuil, Sakari Ailus
Hi Maxime,
On Tuesday, 22 August 2017 11:53:20 EEST Maxime Ripard wrote:
> On Mon, Aug 07, 2017 at 11:18:03PM +0300, Laurent Pinchart wrote:
> > On Thursday 20 Jul 2017 11:23:01 Maxime Ripard wrote:
> >> The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up
> >> to 4 CSI-2 lanes, and can route the frames to up to 4 streams, depending
> >> on the hardware implementation.
> >>
> >> It can operate with an external D-PHY, an internal one or no D-PHY at
> >> all in some configurations.
> >
> > Without any PHY ? I'm curious, how does that work ?
>
> We're currently working on an FPGA exactly with that configuration. So
> I guess the answer would be "it doesn't on an ASIC" :)
What's connected to the input of the CSI-2 receiver ?
> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >> ---
> >>
> >> .../devicetree/bindings/media/cdns-csi2rx.txt | 87 ++++++++++++++++
> >> 1 file changed, 87 insertions(+)
> >> create mode 100644
> >> Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> >> b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt new file mode
> >> 100644
> >> index 000000000000..e08547abe885
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> >> @@ -0,0 +1,87 @@
> >> +Cadence MIPI-CSI2 RX controller
> >> +===============================
> >> +
> >> +The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to
> >> 4> CSI
> >> +lanes in input, and 4 different pixel streams in output.
> >> +
> >> +Required properties:
> >> + - compatible: must be set to "cdns,csi2rx" and an SoC-specific
> >> compatible
> >> + - reg: base address and size of the memory mapped region
> >> + - clocks: phandles to the clocks driving the controller
> >> + - clock-names: must contain:
> >> + * sys_clk: main clock
> >> + * p_clk: register bank clock
> >> + * p_free_clk: free running register bank clock
> >> + * pixel_ifX_clk: pixel stream output clock, one for each stream
> >> + implemented in hardware, between 0 and 3
> >
> > Nitpicking, I would write the name is pixel_if[0-3]_clk, it took me a few
> > seconds to see that the X was a placeholder.
>
> Ok.
>
> >> + * dphy_rx_clk: D-PHY byte clock, if implemented in hardware
> >> + - phys: phandle to the external D-PHY
> >> + - phy-names: must contain dphy, if the implementation uses an
> >> + external D-PHY
> >
> > I would move the last two properties in an optional category as they're
> > effectively optional. I think you should also explain a bit more clearly
> > that the phys property must not be present if the phy-names property is
> > not present.
>
> It's not really optional. The IP has a configuration register that
> allows you to see if it's been synthesized with or without a PHY. If
> the right bit is set, that property will be mandatory, if not, it's
> useless.
Just to confirm, the PHY is a separate IP core, right ? Is the CSI-2 receiver
input interface different when used with a PHY and when used without one ?
Could a third-party PHY be used as well ? If so, would the PHY synthesis bit
be set or not ?
> Maybe it's just semantics, but to me, optional means that it can
> operate with or without it under any circumstances. It's not really
> the case here.
It'sa semantic issue, but documenting a property as required when it can be
ommitted under some circumstances seems even weirder to me :-) I understand
the optional category as "can be ommitted in certain circumstances".
> >> +
> >> +Required subnodes:
> >> + - ports: A ports node with endpoint definitions as defined in
> >> +
> >> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> The
> >> + first port subnode should be the input endpoint, the second
> >> one the
> >> + outputs
> >> +
> >> + The output port should have as many endpoints as stream supported by
> >> + the hardware implementation, between 1 and 4, their ID being the
> >> + stream output number used in the implementation.
> >
> > I don't think that's correct. The IP has four independent outputs, it
> > should have 4 output ports for a total for 5 ports. Multiple endpoints
> > per port would describe multiple connections from the same output to
> > different sinks.
>
> Ok.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-08-22 9:01 ` Laurent Pinchart
@ 2017-08-22 19:25 ` Cyprian Wronka
2017-08-22 21:03 ` Laurent Pinchart
0 siblings, 1 reply; 17+ messages in thread
From: Cyprian Wronka @ 2017-08-22 19:25 UTC (permalink / raw)
To: Laurent Pinchart, Maxime Ripard
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring, linux-media,
devicetree, Neil Webb, Richard Sproul, Alan Douglas,
Steve Creaney, Thomas Petazzoni, Boris Brezillon,
Niklas Söderlund, Hans Verkuil, Sakari Ailus
Hi Laurent,
On 22/08/2017, 02:01, "Laurent Pinchart" <laurent.pinchart@ideasonboard.com> wrote:
EXTERNAL MAIL
Hi Maxime,
On Tuesday, 22 August 2017 11:53:20 EEST Maxime Ripard wrote:
> On Mon, Aug 07, 2017 at 11:18:03PM +0300, Laurent Pinchart wrote:
> > On Thursday 20 Jul 2017 11:23:01 Maxime Ripard wrote:
> >> The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up
> >> to 4 CSI-2 lanes, and can route the frames to up to 4 streams, depending
> >> on the hardware implementation.
> >>
> >> It can operate with an external D-PHY, an internal one or no D-PHY at
> >> all in some configurations.
> >
> > Without any PHY ? I'm curious, how does that work ?
>
> We're currently working on an FPGA exactly with that configuration. So
> I guess the answer would be "it doesn't on an ASIC" :)
What's connected to the input of the CSI-2 receiver ?
It is connected to an instance of a CSI TX digital interface.
> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >> ---
> >>
> >> .../devicetree/bindings/media/cdns-csi2rx.txt | 87 ++++++++++++++++
> >> 1 file changed, 87 insertions(+)
> >> create mode 100644
> >> Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> >> b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt new file mode
> >> 100644
> >> index 000000000000..e08547abe885
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> >> @@ -0,0 +1,87 @@
> >> +Cadence MIPI-CSI2 RX controller
> >> +===============================
> >> +
> >> +The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to
> >> 4> CSI
> >> +lanes in input, and 4 different pixel streams in output.
> >> +
> >> +Required properties:
> >> + - compatible: must be set to "cdns,csi2rx" and an SoC-specific
> >> compatible
> >> + - reg: base address and size of the memory mapped region
> >> + - clocks: phandles to the clocks driving the controller
> >> + - clock-names: must contain:
> >> + * sys_clk: main clock
> >> + * p_clk: register bank clock
> >> + * p_free_clk: free running register bank clock
> >> + * pixel_ifX_clk: pixel stream output clock, one for each stream
> >> + implemented in hardware, between 0 and 3
> >
> > Nitpicking, I would write the name is pixel_if[0-3]_clk, it took me a few
> > seconds to see that the X was a placeholder.
>
> Ok.
>
> >> + * dphy_rx_clk: D-PHY byte clock, if implemented in hardware
> >> + - phys: phandle to the external D-PHY
> >> + - phy-names: must contain dphy, if the implementation uses an
> >> + external D-PHY
> >
> > I would move the last two properties in an optional category as they're
> > effectively optional. I think you should also explain a bit more clearly
> > that the phys property must not be present if the phy-names property is
> > not present.
>
> It's not really optional. The IP has a configuration register that
> allows you to see if it's been synthesized with or without a PHY. If
> the right bit is set, that property will be mandatory, if not, it's
> useless.
Just to confirm, the PHY is a separate IP core, right ? Is the CSI-2 receiver
input interface different when used with a PHY and when used without one ?
Could a third-party PHY be used as well ? If so, would the PHY synthesis bit
be set or not ?
The PHY (in our case a D-PHY) is a separate entity, it can be from a 3rd party as the IP interface is standard, the SoC integrator would set the bit accordingly based on whether any PHY is present or not.
There is also an option of routing digital output from a CSI-TX to a CSI-RX and in such case a PHY would not need to be used (as in the case of our current platform).
> Maybe it's just semantics, but to me, optional means that it can
> operate with or without it under any circumstances. It's not really
> the case here.
It'sa semantic issue, but documenting a property as required when it can be
ommitted under some circumstances seems even weirder to me :-) I understand
the optional category as "can be ommitted in certain circumstances".
> >> +
> >> +Required subnodes:
> >> + - ports: A ports node with endpoint definitions as defined in
> >> +
> >> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >> The
> >> + first port subnode should be the input endpoint, the second
> >> one the
> >> + outputs
> >> +
> >> + The output port should have as many endpoints as stream supported by
> >> + the hardware implementation, between 1 and 4, their ID being the
> >> + stream output number used in the implementation.
> >
> > I don't think that's correct. The IP has four independent outputs, it
> > should have 4 output ports for a total for 5 ports. Multiple endpoints
> > per port would describe multiple connections from the same output to
> > different sinks.
>
> Ok.
Regards,
Cyprian
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-08-22 19:25 ` Cyprian Wronka
@ 2017-08-22 21:03 ` Laurent Pinchart
2017-08-25 14:44 ` Maxime Ripard
0 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2017-08-22 21:03 UTC (permalink / raw)
To: Cyprian Wronka
Cc: Maxime Ripard, Mauro Carvalho Chehab, Mark Rutland, Rob Herring,
linux-media, devicetree, Neil Webb, Richard Sproul, Alan Douglas,
Steve Creaney, Thomas Petazzoni, Boris Brezillon,
Niklas Söderlund, Hans Verkuil, Sakari Ailus
Hi Cyprian,
On Tuesday, 22 August 2017 22:25:49 EEST Cyprian Wronka wrote:
> On 22/08/2017, 02:01, Laurent Pinchart wrote:
>> On Tuesday, 22 August 2017 11:53:20 EEST Maxime Ripard wrote:
>>> On Mon, Aug 07, 2017 at 11:18:03PM +0300, Laurent Pinchart wrote:
>>>> On Thursday 20 Jul 2017 11:23:01 Maxime Ripard wrote:
>>>> The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that
>>>>> supports up to 4 CSI-2 lanes, and can route the frames to up to 4
>>>>> streams, depending on the hardware implementation.
>>>>>
>>>>> It can operate with an external D-PHY, an internal one or no D-PHY
>>>>> at all in some configurations.
>>>>
>>>> Without any PHY ? I'm curious, how does that work ?
>>>
>>> We're currently working on an FPGA exactly with that configuration.
>>> So I guess the answer would be "it doesn't on an ASIC" :)
>>>
>> What's connected to the input of the CSI-2 receiver ?
>>
> It is connected to an instance of a CSI TX digital interface.
That makes sense. I suppose it's a test setup
>>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>>> ---
>>>>>
>>>>>
>>>>> .../devicetree/bindings/media/cdns-csi2rx.txt> | 87
>>>>> ++++++++++++++++
>>>>> 1 file changed, 87 insertions(+)
>>>>> create mode 100644
>>>>> Documentation/devicetree/bindings/media/cdns-csi2rx.txt
>>>>>
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
>>>>> b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt new file
>>>>> mode 100644
>>>>> index 000000000000..e08547abe885
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
>>>>> @@ -0,0 +1,87 @@
>>>>> +Cadence MIPI-CSI2 RX controller
>>>>> +===============================
>>>>> +
>>>>> +The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting
>>>>> up to 4 CSI
>>>>> +lanes in input, and 4 different pixel streams in output.
>>>>> +
>>>>> +Required properties:
>>>>> + - compatible: must be set to "cdns,csi2rx" and an SoC-specific
>>>>> compatible
>>>>> + - reg: base address and size of the memory mapped region
>>>>> + - clocks: phandles to the clocks driving the controller
>>>>> + - clock-names: must contain:
>>>>> + * sys_clk: main clock
>>>>> + * p_clk: register bank clock
>>>>> + * p_free_clk: free running register bank clock
>>>>> + * pixel_ifX_clk: pixel stream output clock, one for each
>>>>> stream
>>>>> + implemented in hardware, between 0 and 3
>>>>
>>>> Nitpicking, I would write the name is pixel_if[0-3]_clk, it took me
>>>> a few seconds to see that the X was a placeholder.
>>>
>>> Ok.
>>>
>>>>> + * dphy_rx_clk: D-PHY byte clock, if implemented in hardware
>>>>> + - phys: phandle to the external D-PHY
>>>>> + - phy-names: must contain dphy, if the implementation uses an
>>>>> + external D-PHY
>>>>
>>>> I would move the last two properties in an optional category as
>>>> they're effectively optional. I think you should also explain a bit more
>>>> clearly that the phys property must not be present if the phy-names
>>>> property is not present.
>>>
>>> It's not really optional. The IP has a configuration register that
>>> allows you to see if it's been synthesized with or without a PHY. If
>>> the right bit is set, that property will be mandatory, if not, it's
>>> useless.
>>
>> Just to confirm, the PHY is a separate IP core, right ? Is the CSI-2
>> receiver input interface different when used with a PHY and when used
>> without one ? Could a third-party PHY be used as well ? If so, would the
>> PHY synthesis bit be set or not ?
>
> The PHY (in our case a D-PHY) is a separate entity, it can be from a 3rd
> party as the IP interface is standard, the SoC integrator would set the bit
> accordingly based on whether any PHY is present or not. There is also an
> option of routing digital output from a CSI-TX to a CSI-RX and in such case
> a PHY would not need to be used (as in the case of our current platform).
OK, thank you for the clarification.
Maxime mentioned that a bit can be read from a register to notify whether a
PHY has been synthesized or not. Does it influence the CSI-2 RX input
interface at all, or is the CSI-2 RX IP core synthesized the same way
regardless of whether a PHY is present or not ?
>>> Maybe it's just semantics, but to me, optional means that it can
>>> operate with or without it under any circumstances. It's not really
>>> the case here.
>>
>> It's a semantic issue, but documenting a property as required when it can
>> be ommitted under some circumstances seems even weirder to me :-) I
>> understand the optional category as "can be ommitted in certain
>> circumstances".
>>
>>>>> +
>>>>> +Required subnodes:
>>>>> + - ports: A ports node with endpoint definitions as defined in
>>>>> + Documentation/devicetree/bindings/media/video-interfaces.txt. The
>>>>> + first port subnode should be the input endpoint, the
>>>>> second one the
>>>>> + outputs
>>>>> +
>>>>> + The output port should have as many endpoints as stream
>>>>> supported by
>>>>> + the hardware implementation, between 1 and 4, their ID being
>>>>> the
>>>>> + stream output number used in the implementation.
>>>>
>>>> I don't think that's correct. The IP has four independent outputs,
>>>> it should have 4 output ports for a total for 5 ports. Multiple
>>>> endpoints per port would describe multiple connections from the same
>>>> output to different sinks.
>>>
>>> Ok.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
2017-08-07 19:24 ` Benoit Parrot
@ 2017-08-25 14:03 ` Maxime Ripard
0 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2017-08-25 14:03 UTC (permalink / raw)
To: Benoit Parrot
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring,
Laurent Pinchart, linux-media, devicetree, Cyprian Wronka,
Neil Webb, Richard Sproul, Alan Douglas, Steve Creaney,
Thomas Petazzoni, Boris Brezillon, Niklas Söderlund,
Hans Verkuil, Sakari Ailus
[-- Attachment #1: Type: text/plain, Size: 2511 bytes --]
Hi Benoit,
On Mon, Aug 07, 2017 at 02:24:24PM -0500, Benoit Parrot wrote:
> > +#define CSI2RX_STREAMS_MAX 4
> > +
>
> Just to confirm here that "streams" in this case are equivalent to
> "Virtual Channel", correct?
Kind of, yes, but it's a bit more complicated than that. The streams
are the output of the CSI2-RX block, so the interface to the capture
devices.
You can associate any CSI-2 virtual channel (in input) to any stream
(in output). However, as we don't have a way to change that routing at
runtime using v4l2 (yet [1]), the driver currently hardcode that
routing to have CSI-2 VC0 sent to stream 0, VC1 to stream 1, etc.
> > +static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> > +{
> > + writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
> > + csi2rx->base + CSI2RX_SOFT_RESET_REG);
> > +
> > + udelay(10);
>
> Shouldn't we use usleep_range() instead?
We totally should.
> > +static int csi2rx_stop(struct csi2rx_priv *csi2rx)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < csi2rx->max_streams; i++)
> > + writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> > +
> > + return 0;
> > +}
> > +
>
> Here, it is entirely possible that the "dma/buffer" engine which will
> make use of this receiver creates separates video nodes for each streams.
> In which case, you could theoretically have multiple user space capture
> on-going.
> But the "start" and "stop" method above would disrupt any of the other
> stream. Unless you start and stop all 4 capture streams in lock step.
>
> Eventually, the sub device might be a port aggregator which has up to
> 4 sensors on the source pad and feed each camera traffic on its own
> Virtual Channel.
>
> I know there isn't support in the framework for this currently but it
> is something to think about.
I guess you could make the same argument if you have several engines,
each one connected to a stream.
One way to work around that could be to add some reference counting in
the start and stop functions, and keep enabling all the outputs.
This wouldn't solve the underlying issue that all the stream would be
enabled and we don't really have a way to tell which one we want to
enable, but at least we wouldn't disrupt active streams when the first
one goes away.
Maxime
1: https://www.mail-archive.com/linux-media@vger.kernel.org/msg116955.html
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
2017-08-07 20:42 ` Laurent Pinchart
@ 2017-08-25 14:30 ` Maxime Ripard
0 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2017-08-25 14:30 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Mark Rutland, Rob Herring, linux-media,
devicetree, Cyprian Wronka, Neil Webb, Richard Sproul,
Alan Douglas, Steve Creaney, Thomas Petazzoni, Boris Brezillon,
Niklas Söderlund, Hans Verkuil, Sakari Ailus
[-- Attachment #1: Type: text/plain, Size: 4149 bytes --]
Hi Laurent,
I've removed the comments I'll take into account
On Mon, Aug 07, 2017 at 11:42:01PM +0300, Laurent Pinchart wrote:
> > + csi2rx_reset(csi2rx);
> > +
> > + // TODO: modify the mapping of the DPHY lanes?
>
> The mapping should be read from DT and applied here.
As far as I understand it, this is a mapping between logical and
physical lanes before forwarding it to the D-PHY. Would data-lanes
apply for such a case?
> > +static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> > +
> > + v4l2_subdev_call(csi2rx->sensor_subdev, video, s_stream,
> > + enable);
>
> Shouldn't you handle errors here ?
Yes.
> I'm not familiar with this IP core, but most D-PHYs need to synchronize to the
> input and must be started before the source.
We don't have any kind of D-PHY support at the moment, but I guess we
should put it there once we do.
> > + return PTR_ERR(csi2rx->base);
> > + }
> > +
> > + reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
>
> Shouldn't you enable the register access clock(s) before reading the
> register ?
Argh, you're right.
>
> > + csi2rx->max_lanes = (reg & 7) + 1;
>
> Up to 8 lanes ? Shouldn't this be (reg & 3) + 1 ?
The bit field is indeed documented 3 bits wide, hence the 7.
> > + csi2rx->max_streams = ((reg >> 4) & 7);
>
> Should you validate the value as you use it as an array access index ?
I should, yes.
> > + csi2rx->cdns_dphy = reg & BIT(3);
> > +
> > + csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
> > + if (IS_ERR(csi2rx->sys_clk)) {
> > + dev_err(&pdev->dev, "Couldn't get sys clock\n");
>
> If you wrote this as
>
> dev_err(&pdev->dev, "Couldn't get %s clock\n", "sys");
>
> and the next messages in a similar fashion, most of the message wouldn't be
> duplicated, resulting in smaller code size.
I'm usually not a big fan of this, since one side effect is also that
you can't grep / search it anymore, and I'm not sure it's worth the
hassle for a couple dozen bytes.
>
> > + return PTR_ERR(csi2rx->sys_clk);
> > + }
> > +
> > + csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
> > + if (IS_ERR(csi2rx->p_clk)) {
> > + dev_err(&pdev->dev, "Couldn't get P clock\n");
> > + return PTR_ERR(csi2rx->p_clk);
> > + }
> > +
> > + csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
> > + if (IS_ERR(csi2rx->p_free_clk)) {
> > + dev_err(&pdev->dev, "Couldn't get free running P clock\n");
> > + return PTR_ERR(csi2rx->p_free_clk);
> > + }
> > +
> > + for (i = 0; i < csi2rx->max_streams; i++) {
> > + char clk_name[16];
>
> Isn't 13 enough ?
It is, if you have an int short enough.
> > + csi2rx->sensor_node = remote;
> > + csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
> > + csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +
> > + subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
>
> Could you store this in the csi2rx_priv structure to avoid a dynamic
> allocation ?
It's used only in this function, so it seems a bit overkill to do
that. But I guess I could just allocate it on the stack.
> > +
> > + csi2rx = devm_kzalloc(&pdev->dev, sizeof(*csi2rx), GFP_KERNEL);
>
> Please don't use devm_kzalloc() to allocate structures that can be accessed
> from userspace (in this case because it embeds the subdev structure,
> accessible at least through the subdevs ioctls). This is incompatible with
> proper unplug handling. There are many other issues that we will need to solve
> in the V4L2 core to handling unplugging properly, but let's not add a new one.
What's wrong with kzalloc in such a case? As far as I know, the
userspace can access such a memory, as long as you use copy_to_user,
right? Or is it because of the life-cycle of the allocation that would
be gone while the userspace might not be yet? I'm not sure what would
be a proper fix for it though.
Thanks for your review!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-08-22 21:03 ` Laurent Pinchart
@ 2017-08-25 14:44 ` Maxime Ripard
2017-08-25 16:34 ` Laurent Pinchart
0 siblings, 1 reply; 17+ messages in thread
From: Maxime Ripard @ 2017-08-25 14:44 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Cyprian Wronka, Mauro Carvalho Chehab, Mark Rutland, Rob Herring,
linux-media, devicetree, Neil Webb, Richard Sproul, Alan Douglas,
Steve Creaney, Thomas Petazzoni, Boris Brezillon,
Niklas Söderlund, Hans Verkuil, Sakari Ailus
[-- Attachment #1: Type: text/plain, Size: 2459 bytes --]
Hi Laurent,
On Wed, Aug 23, 2017 at 12:03:32AM +0300, Laurent Pinchart wrote:
> >>>>> + - phys: phandle to the external D-PHY
> >>>>> + - phy-names: must contain dphy, if the implementation uses an
> >>>>> + external D-PHY
> >>>>
> >>>> I would move the last two properties in an optional category as
> >>>> they're effectively optional. I think you should also explain a bit more
> >>>> clearly that the phys property must not be present if the phy-names
> >>>> property is not present.
> >>>
> >>> It's not really optional. The IP has a configuration register that
> >>> allows you to see if it's been synthesized with or without a PHY. If
> >>> the right bit is set, that property will be mandatory, if not, it's
> >>> useless.
> >>
> >> Just to confirm, the PHY is a separate IP core, right ? Is the CSI-2
> >> receiver input interface different when used with a PHY and when used
> >> without one ? Could a third-party PHY be used as well ? If so, would the
> >> PHY synthesis bit be set or not ?
> >
> > The PHY (in our case a D-PHY) is a separate entity, it can be from a 3rd
> > party as the IP interface is standard, the SoC integrator would set the bit
> > accordingly based on whether any PHY is present or not. There is also an
> > option of routing digital output from a CSI-TX to a CSI-RX and in such case
> > a PHY would not need to be used (as in the case of our current platform).
>
> OK, thank you for the clarification.
>
> Maxime mentioned that a bit can be read from a register to notify whether a
> PHY has been synthesized or not. Does it influence the CSI-2 RX input
> interface at all, or is the CSI-2 RX IP core synthesized the same way
> regardless of whether a PHY is present or not ?
So we got an answer to this, and the physical interface remains the
same.
However, the PHY bit is set only when there's an internal D-PHY, which
means we have basically three cases:
- No D-PHY at all, D-PHY presence bit not set
- Internal D-PHY, D-PHY presence bit set
- External D-PHY, D-PHY presence bit not set
I guess that solves our discussion about whether the phys property
should be marked optional or not. It should indeed be optional, and
when it's not there, the D-PHY presence bit will tell whether we have
to program the internal D-PHY or not.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings
2017-08-25 14:44 ` Maxime Ripard
@ 2017-08-25 16:34 ` Laurent Pinchart
0 siblings, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2017-08-25 16:34 UTC (permalink / raw)
To: Maxime Ripard
Cc: Cyprian Wronka, Mauro Carvalho Chehab, Mark Rutland, Rob Herring,
linux-media, devicetree, Neil Webb, Richard Sproul, Alan Douglas,
Steve Creaney, Thomas Petazzoni, Boris Brezillon,
Niklas Söderlund, Hans Verkuil, Sakari Ailus
Hi Maxime,
On Friday, 25 August 2017 17:44:40 EEST Maxime Ripard wrote:
> On Wed, Aug 23, 2017 at 12:03:32AM +0300, Laurent Pinchart wrote:
> >>>>>> + - phys: phandle to the external D-PHY
> >>>>>> + - phy-names: must contain dphy, if the implementation uses an
> >>>>>> + external D-PHY
> >>>>>
> >>>>> I would move the last two properties in an optional category as
> >>>>> they're effectively optional. I think you should also explain a bit
> >>>>> more clearly that the phys property must not be present if the phy-
> >>>>> names property is not present.
> >>>>
> >>>> It's not really optional. The IP has a configuration register that
> >>>> allows you to see if it's been synthesized with or without a PHY. If
> >>>> the right bit is set, that property will be mandatory, if not, it's
> >>>> useless.
> >>>
> >>> Just to confirm, the PHY is a separate IP core, right ? Is the CSI-2
> >>> receiver input interface different when used with a PHY and when used
> >>> without one ? Could a third-party PHY be used as well ? If so, would
> >>> the PHY synthesis bit be set or not ?
> >>
> >> The PHY (in our case a D-PHY) is a separate entity, it can be from a 3rd
> >> party as the IP interface is standard, the SoC integrator would set the
> >> bit accordingly based on whether any PHY is present or not. There is also
> >> an option of routing digital output from a CSI-TX to a CSI-RX and in such
> >> case a PHY would not need to be used (as in the case of our current
> >> platform).
> >
> > OK, thank you for the clarification.
> >
> > Maxime mentioned that a bit can be read from a register to notify whether
> > a PHY has been synthesized or not. Does it influence the CSI-2 RX input
> > interface at all, or is the CSI-2 RX IP core synthesized the same way
> > regardless of whether a PHY is present or not ?
>
> So we got an answer to this, and the physical interface remains the
> same.
>
> However, the PHY bit is set only when there's an internal D-PHY, which
> means we have basically three cases:
> - No D-PHY at all, D-PHY presence bit not set
> - Internal D-PHY, D-PHY presence bit set
> - External D-PHY, D-PHY presence bit not set
>
> I guess that solves our discussion about whether the phys property
> should be marked optional or not. It should indeed be optional, and
> when it's not there, the D-PHY presence bit will tell whether we have
> to program the internal D-PHY or not.
Is the internal D-PHY programmed through the register space of the CSI2-RX ?
If so I agree with you.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-08-25 16:34 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-20 9:23 [PATCH v2 0/2] media: v4l: Add support for the Cadence MIPI-CSI2 RX Maxime Ripard
2017-07-20 9:23 ` [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings Maxime Ripard
2017-07-24 19:56 ` Rob Herring
2017-08-07 19:56 ` Benoit Parrot
2017-08-07 20:18 ` Laurent Pinchart
2017-08-22 8:53 ` Maxime Ripard
2017-08-22 9:01 ` Laurent Pinchart
2017-08-22 19:25 ` Cyprian Wronka
2017-08-22 21:03 ` Laurent Pinchart
2017-08-25 14:44 ` Maxime Ripard
2017-08-25 16:34 ` Laurent Pinchart
2017-07-20 9:23 ` [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver Maxime Ripard
2017-07-23 1:09 ` kbuild test robot
2017-08-07 19:24 ` Benoit Parrot
2017-08-25 14:03 ` Maxime Ripard
2017-08-07 20:42 ` Laurent Pinchart
2017-08-25 14:30 ` Maxime Ripard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).