Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
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(>dev, "sys_clk"); > > + if (IS_ERR(csi2rx->sys_clk)) { > > + dev_err(>dev, "Couldn't get sys clock\n"); > > If you wrote this as > > dev_err(>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(>dev, "p_clk"); > > + if (IS_ERR(csi2rx->p_clk)) { > > + dev_err(>dev, "Couldn't get P clock\n"); > > + return PTR_ERR(csi2rx->p_clk); > > + } > > + > > + csi2rx->p_free_clk = devm_clk_get(>dev, "p_free_clk"); > > + if (IS_ERR(csi2rx->p_free_clk)) { > > + dev_err(>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 = >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(>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 signature.asc Description: PGP signature
Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
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 signature.asc Description: PGP signature
Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
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> --- > 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 ..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 > +#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_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; > + boolcdns_dphy; > + > + struct v4l2_subdev subdev; > + struct media_padpads[CSI2RX_PAD_MAX]; > + > + /* Remote sensor */ > + struct v4l2_async_subdevasd; > + 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
Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
Maxime Ripardwrote 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 > --- > 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 ..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 ..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 > +#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_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
Re: [PATCH v2 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver
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(>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, 166struct v4l2_subdev *s_subdev, 167struct 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(_subdev->entity, 173 >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(>sensor_subdev->entity, 195 csi2rx->sensor_pad, 196 >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