Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Maxime Ripard]

On Tue, Sep 12, 2017 at 02:13:12PM -0500, Benoit Parrot wrote:
> > > + /*
> > > +  * 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.
> > > +  *
> > > +  * We also cannot enable and disable independant streams here,
> > > +  * hence the reference counting.
> > > +  */
> > > + for (i = 0; i < csi2rx->max_streams; i++) {
> > > + clk_prepare_enable(csi2rx->pixel_clk[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));
> 
> I see here that we are setting the data_type to 0 (as we are not
> setting it) so effectively capturing everything on the channel(s).
> Will we be adding a method to select/filter specific data type?  For
> instance if we only want to grab YUV data in one stream and only
> RGB24 in another. Of course that would not be possible here as is...

Ah, right, I forgot about that. I've actually started that discussion
on another thread for a transceiver, without much success though:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg117920.html

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Maxime Ripard]

Hi Benoit,

Thanks for your comments,

On Tue, Sep 12, 2017 at 01:23:39PM -0500, Benoit Parrot wrote:
> > +static int csi2rx_probe(struct platform_device *pdev)
> > +{
> > +   struct v4l2_async_subdev **subdevs;
> > +   struct csi2rx_priv *csi2rx;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   /*
> > +* Since the v4l2_subdev structure is embedded in our
> > +* csi2rx_priv structure, and that the structure is exposed to
> > +* the user-space, we cannot just use the devm_variant
> > +* here. Indeed, that would lead to a use-after-free in a
> > +* open() - unbind - close() pattern.
> > +*/
> > +   csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
> > +   if (!csi2rx)
> > +   return -ENOMEM;
> > +   platform_set_drvdata(pdev, csi2rx);
> > +   csi2rx->dev = >dev;

[snip]

> > +
> > +   subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
> > +   if (!subdevs) {
> > +   ret = -ENOMEM;
> > +   goto err_free_priv;
> > +   }
> > +   subdevs[0] = >asd;
> > +
> 
> Shouldn't the comment related to lifetime of memory allocation be
> also applied here?  A reference to the "subdevs" pointer is taken
> internally so it might suffer the same fate.  Not sure how many
> "struct v4l2_async_subdev **subdevs" we would end up needing but
> since here we are only dealing with one, why not just make it a
> member of the struct csi2rx_priv object.

As far as I know, only the notifier will use that array. The notifier
will be removed before that array is de-allocated, and the user-space
never has access to it, so I'm not sure the same issue arises here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Benoit Parrot]

Benoit Parrot  wrote on Tue [2017-Sep-12 13:23:39 -0500]:



> > +static int csi2rx_start(struct csi2rx_priv *csi2rx)
> > +{
> > +   unsigned int i;
> > +   u32 reg;
> > +   int ret;
> > +
> > +   /*
> > +* We're not the first users, there's no need to enable the
> > +* whole controller.
> > +*/
> > +   if (atomic_inc_return(>count) > 1)
> > +   return 0;
> > +
> > +   clk_prepare_enable(csi2rx->p_clk);
> > +
> > +   printk("%s %d\n", __func__, __LINE__);
> 
> Some left over debug...
> 
> > +
> > +   csi2rx_reset(csi2rx);
> > +
> > +   reg = csi2rx->num_lanes << 8;
> > +   for (i = 0; i < csi2rx->num_lanes; i++)
> > +   reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);
> > +
> > +   writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG);
> > +
> > +   ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /*
> > +* 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.
> > +*
> > +* We also cannot enable and disable independant streams here,
> > +* hence the reference counting.
> > +*/
> > +   for (i = 0; i < csi2rx->max_streams; i++) {
> > +   clk_prepare_enable(csi2rx->pixel_clk[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));

I see here that we are setting the data_type to 0 (as we are not setting
it) so effectively capturing everything on the channel(s).
Will we be adding a method to select/filter specific data type?
For instance if we only want to grab YUV data in one stream and only
RGB24 in another. Of course that would not be possible here as is...

Benoit

> > +
> > +   writel(CSI2RX_STREAM_CTRL_START,
> > +  csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> > +   }
> > +
> > +   clk_prepare_enable(csi2rx->sys_clk);
> > +
> > +   clk_disable_unprepare(csi2rx->p_clk);
> > +
> > +   return 0;
> > +}

Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Benoit Parrot]

Maxime,

Thanks for the patch.

Maxime Ripard  wrote on Mon [2017-Sep-04 
15:03:35 +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 
> ---
>  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 | 494 
> +++
>  5 files changed, 510 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/cadence/cdns-csi2rx.c 
> b/drivers/media/platform/cadence/cdns-csi2rx.c
> new file mode 100644
> index ..e662b1890bba
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -0,0 +1,494 @@
> +/*
> + * 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define CSI2RX_DEVICE_CFG_REG0x000
> +
> +#define CSI2RX_SOFT_RESET_REG0x004
> +#define CSI2RX_SOFT_RESET_PROTOCOL   BIT(1)
> +#define CSI2RX_SOFT_RESET_FRONT  BIT(0)
> +
> +#define CSI2RX_STATIC_CFG_REG0x008
> +#define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane)((plane) << (16 + 
> (llane) * 4))
> +#define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8)
> +
> +#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_STREAM0,
> + CSI2RX_PAD_SOURCE_STREAM1,
> + CSI2RX_PAD_SOURCE_STREAM2,
> + CSI2RX_PAD_SOURCE_STREAM3,
> + CSI2RX_PAD_MAX,
> +};
> +
> +struct csi2rx_priv {
> + struct device   *dev;
> + atomic_tcount;
> +
> + void __iomem*base;
> + struct clk  *sys_clk;
> + struct clk  *p_clk;
> + struct clk  *pixel_clk[CSI2RX_STREAMS_MAX];
> + struct phy  *dphy;
> +
> + u8  lanes[4];
> + u8  num_lanes;
> + u8  max_lanes;
> + u8  max_streams;
> + boolhas_internal_dphy;
> +
> + struct v4l2_subdev  subdev;
> + struct media_padpads[CSI2RX_PAD_MAX];
> +
> + /* Remote source */
> + struct v4l2_async_subdevasd;
> + struct device_node  *source_node;
> + struct v4l2_subdev  *source_subdev;
> + int source_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);
> +
> + usleep_range(10, 20);
> +
> + writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +}
> +
> +static int csi2rx_start(struct csi2rx_priv *csi2rx)
> +{
> + unsigned int i;
> + u32 reg;
> + int ret;
> +
> + /*
> +  * 

[PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[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 
---
 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 | 494 +++
 5 files changed, 510 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 ..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 ..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 ..e662b1890bba
--- /dev/null
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -0,0 +1,494 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define CSI2RX_DEVICE_CFG_REG  0x000
+
+#define CSI2RX_SOFT_RESET_REG  0x004
+#define CSI2RX_SOFT_RESET_PROTOCOL BIT(1)
+#define CSI2RX_SOFT_RESET_FRONTBIT(0)
+
+#define CSI2RX_STATIC_CFG_REG  0x008
+#define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane)  ((plane) << (16 + 
(llane) * 4))
+#define CSI2RX_STATIC_CFG_LANES_MASK   GENMASK(11, 8)
+
+#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_SELECTBIT(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_STREAM0,
+   CSI2RX_PAD_SOURCE_STREAM1,
+   CSI2RX_PAD_SOURCE_STREAM2,
+   CSI2RX_PAD_SOURCE_STREAM3,
+   CSI2RX_PAD_MAX,
+};
+
+struct csi2rx_priv {
+   struct device   *dev;
+   atomic_tcount;
+
+   void __iomem*base;
+   struct clk  *sys_clk;
+   struct clk  *p_clk;
+   struct clk  *pixel_clk[CSI2RX_STREAMS_MAX];
+