Message ID | 20180212230132.5402-3-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Niklas Thank you for this patch ... I know it has been a lot of work getting this and the VIN together! On 13/02/18 00:01, Niklas Söderlund wrote: > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > connected between the video sources and the video grabbers (VIN). > > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> I don't think there's actually anything major in the below - so with any comments addressed, or specifically ignored you can add my: Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> tag if you like. > --- > drivers/media/platform/rcar-vin/Kconfig | 12 + > drivers/media/platform/rcar-vin/Makefile | 1 + > drivers/media/platform/rcar-vin/rcar-csi2.c | 884 ++++++++++++++++++++++++++++ > 3 files changed, 897 insertions(+) > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c > > diff --git a/drivers/media/platform/rcar-vin/Kconfig b/drivers/media/platform/rcar-vin/Kconfig > index af4c98b44d2e22cb..6875f30c1ae42631 100644 > --- a/drivers/media/platform/rcar-vin/Kconfig > +++ b/drivers/media/platform/rcar-vin/Kconfig > @@ -1,3 +1,15 @@ > +config VIDEO_RCAR_CSI2 > + tristate "R-Car MIPI CSI-2 Receiver" > + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF I think I recall recently seeing that depending upon OF is redundant and not necessary (though I think that's minor in this instance) > + depends on ARCH_RENESAS || COMPILE_TEST > + select V4L2_FWNODE > + ---help--- > + Support for Renesas R-Car MIPI CSI-2 receiver. > + Supports R-Car Gen3 SoCs. > + > + To compile this driver as a module, choose M here: the > + module will be called rcar-csi2. > + > config VIDEO_RCAR_VIN > tristate "R-Car Video Input (VIN) Driver" > depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF && HAS_DMA && MEDIA_CONTROLLER > diff --git a/drivers/media/platform/rcar-vin/Makefile b/drivers/media/platform/rcar-vin/Makefile > index 48c5632c21dc060b..5ab803d3e7c1aa57 100644 > --- a/drivers/media/platform/rcar-vin/Makefile > +++ b/drivers/media/platform/rcar-vin/Makefile > @@ -1,3 +1,4 @@ > rcar-vin-objs = rcar-core.o rcar-dma.o rcar-v4l2.o > > +obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o > obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin.o > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > new file mode 100644 > index 0000000000000000..c0c2a763151bc928 > --- /dev/null > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -0,0 +1,884 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Driver for Renesas R-Car MIPI CSI-2 Receiver > + * > + * Copyright (C) 2018 Renesas Electronics Corp. > + */ > + > +#include <linux/delay.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_graph.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/sys_soc.h> > + > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-device.h> > +#include <media/v4l2-fwnode.h> > +#include <media/v4l2-mc.h> > +#include <media/v4l2-subdev.h> > + > +/* Register offsets and bits */ > + > +/* Control Timing Select */ > +#define TREF_REG 0x00 > +#define TREF_TREF BIT(0) > + > +/* Software Reset */ > +#define SRST_REG 0x04 > +#define SRST_SRST BIT(0) > + > +/* PHY Operation Control */ > +#define PHYCNT_REG 0x08 > +#define PHYCNT_SHUTDOWNZ BIT(17) > +#define PHYCNT_RSTZ BIT(16) > +#define PHYCNT_ENABLECLK BIT(4) > +#define PHYCNT_ENABLE_3 BIT(3) > +#define PHYCNT_ENABLE_2 BIT(2) > +#define PHYCNT_ENABLE_1 BIT(1) > +#define PHYCNT_ENABLE_0 BIT(0) > + > +/* Checksum Control */ > +#define CHKSUM_REG 0x0c > +#define CHKSUM_ECC_EN BIT(1) > +#define CHKSUM_CRC_EN BIT(0) > + > +/* > + * Channel Data Type Select > + * VCDT[0-15]: Channel 1 VCDT[16-31]: Channel 2 > + * VCDT2[0-15]: Channel 3 VCDT2[16-31]: Channel 4 > + */ > +#define VCDT_REG 0x10 > +#define VCDT2_REG 0x14 > +#define VCDT_VCDTN_EN BIT(15) > +#define VCDT_SEL_VC(n) (((n) & 0x3) << 8) > +#define VCDT_SEL_DTN_ON BIT(6) > +#define VCDT_SEL_DT(n) (((n) & 0x3f) << 0) > + > +/* Frame Data Type Select */ > +#define FRDT_REG 0x18 > + > +/* Field Detection Control */ > +#define FLD_REG 0x1c > +#define FLD_FLD_NUM(n) (((n) & 0xff) << 16) > +#define FLD_FLD_EN4 BIT(3) > +#define FLD_FLD_EN3 BIT(2) > +#define FLD_FLD_EN2 BIT(1) > +#define FLD_FLD_EN BIT(0) > + > +/* Automatic Standby Control */ > +#define ASTBY_REG 0x20 > + > +/* Long Data Type Setting 0 */ > +#define LNGDT0_REG 0x28 > + > +/* Long Data Type Setting 1 */ > +#define LNGDT1_REG 0x2c > + > +/* Interrupt Enable */ > +#define INTEN_REG 0x30 > + > +/* Interrupt Source Mask */ > +#define INTCLOSE_REG 0x34 > + > +/* Interrupt Status Monitor */ > +#define INTSTATE_REG 0x38 > +#define INTSTATE_INT_ULPS_START BIT(7) > +#define INTSTATE_INT_ULPS_END BIT(6) > + > +/* Interrupt Error Status Monitor */ > +#define INTERRSTATE_REG 0x3c > + > +/* Short Packet Data */ > +#define SHPDAT_REG 0x40 > + > +/* Short Packet Count */ > +#define SHPCNT_REG 0x44 > + > +/* LINK Operation Control */ > +#define LINKCNT_REG 0x48 > +#define LINKCNT_MONITOR_EN BIT(31) > +#define LINKCNT_REG_MONI_PACT_EN BIT(25) > +#define LINKCNT_ICLK_NONSTOP BIT(24) > + > +/* Lane Swap */ > +#define LSWAP_REG 0x4c > +#define LSWAP_L3SEL(n) (((n) & 0x3) << 6) > +#define LSWAP_L2SEL(n) (((n) & 0x3) << 4) > +#define LSWAP_L1SEL(n) (((n) & 0x3) << 2) > +#define LSWAP_L0SEL(n) (((n) & 0x3) << 0) > + > +/* PHY Test Interface Write Register */ > +#define PHTW_REG 0x50 > + > +/* PHY Test Interface Clear */ > +#define PHTC_REG 0x58 > +#define PHTC_TESTCLR BIT(0) > + > +/* PHY Frequency Control */ > +#define PHYPLL_REG 0x68 > +#define PHYPLL_HSFREQRANGE(n) ((n) << 16) > + > +struct phypll_hsfreqrange { > + u16 mbps; > + u16 reg; > +}; > + > +static const struct phypll_hsfreqrange hsfreqrange_h3_v3h_m3n[] = { > + { .mbps = 80, .reg = 0x00 }, > + { .mbps = 90, .reg = 0x10 }, > + { .mbps = 100, .reg = 0x20 }, > + { .mbps = 110, .reg = 0x30 }, > + { .mbps = 120, .reg = 0x01 }, > + { .mbps = 130, .reg = 0x11 }, > + { .mbps = 140, .reg = 0x21 }, > + { .mbps = 150, .reg = 0x31 }, > + { .mbps = 160, .reg = 0x02 }, > + { .mbps = 170, .reg = 0x12 }, > + { .mbps = 180, .reg = 0x22 }, > + { .mbps = 190, .reg = 0x32 }, > + { .mbps = 205, .reg = 0x03 }, > + { .mbps = 220, .reg = 0x13 }, > + { .mbps = 235, .reg = 0x23 }, > + { .mbps = 250, .reg = 0x33 }, > + { .mbps = 275, .reg = 0x04 }, > + { .mbps = 300, .reg = 0x14 }, > + { .mbps = 325, .reg = 0x25 }, > + { .mbps = 350, .reg = 0x35 }, > + { .mbps = 400, .reg = 0x05 }, > + { .mbps = 450, .reg = 0x26 }, > + { .mbps = 500, .reg = 0x36 }, > + { .mbps = 550, .reg = 0x37 }, > + { .mbps = 600, .reg = 0x07 }, > + { .mbps = 650, .reg = 0x18 }, > + { .mbps = 700, .reg = 0x28 }, > + { .mbps = 750, .reg = 0x39 }, > + { .mbps = 800, .reg = 0x09 }, > + { .mbps = 850, .reg = 0x19 }, > + { .mbps = 900, .reg = 0x29 }, > + { .mbps = 950, .reg = 0x3a }, > + { .mbps = 1000, .reg = 0x0a }, > + { .mbps = 1050, .reg = 0x1a }, > + { .mbps = 1100, .reg = 0x2a }, > + { .mbps = 1150, .reg = 0x3b }, > + { .mbps = 1200, .reg = 0x0b }, > + { .mbps = 1250, .reg = 0x1b }, > + { .mbps = 1300, .reg = 0x2b }, > + { .mbps = 1350, .reg = 0x3c }, > + { .mbps = 1400, .reg = 0x0c }, > + { .mbps = 1450, .reg = 0x1c }, > + { .mbps = 1500, .reg = 0x2c }, > + /* guard */ > + { .mbps = 0, .reg = 0x00 }, > +}; > + > +static const struct phypll_hsfreqrange hsfreqrange_m3w_h3es1[] = { > + { .mbps = 80, .reg = 0x00 }, > + { .mbps = 90, .reg = 0x10 }, > + { .mbps = 100, .reg = 0x20 }, > + { .mbps = 110, .reg = 0x30 }, > + { .mbps = 120, .reg = 0x01 }, > + { .mbps = 130, .reg = 0x11 }, > + { .mbps = 140, .reg = 0x21 }, > + { .mbps = 150, .reg = 0x31 }, > + { .mbps = 160, .reg = 0x02 }, > + { .mbps = 170, .reg = 0x12 }, > + { .mbps = 180, .reg = 0x22 }, > + { .mbps = 190, .reg = 0x32 }, > + { .mbps = 205, .reg = 0x03 }, > + { .mbps = 220, .reg = 0x13 }, > + { .mbps = 235, .reg = 0x23 }, > + { .mbps = 250, .reg = 0x33 }, > + { .mbps = 275, .reg = 0x04 }, > + { .mbps = 300, .reg = 0x14 }, > + { .mbps = 325, .reg = 0x05 }, > + { .mbps = 350, .reg = 0x15 }, > + { .mbps = 400, .reg = 0x25 }, > + { .mbps = 450, .reg = 0x06 }, > + { .mbps = 500, .reg = 0x16 }, > + { .mbps = 550, .reg = 0x07 }, > + { .mbps = 600, .reg = 0x17 }, > + { .mbps = 650, .reg = 0x08 }, > + { .mbps = 700, .reg = 0x18 }, > + { .mbps = 750, .reg = 0x09 }, > + { .mbps = 800, .reg = 0x19 }, > + { .mbps = 850, .reg = 0x29 }, > + { .mbps = 900, .reg = 0x39 }, > + { .mbps = 950, .reg = 0x0A }, > + { .mbps = 1000, .reg = 0x1A }, > + { .mbps = 1050, .reg = 0x2A }, > + { .mbps = 1100, .reg = 0x3A }, > + { .mbps = 1150, .reg = 0x0B }, > + { .mbps = 1200, .reg = 0x1B }, > + { .mbps = 1250, .reg = 0x2B }, > + { .mbps = 1300, .reg = 0x3B }, > + { .mbps = 1350, .reg = 0x0C }, > + { .mbps = 1400, .reg = 0x1C }, > + { .mbps = 1450, .reg = 0x2C }, > + { .mbps = 1500, .reg = 0x3C }, > + /* guard */ > + { .mbps = 0, .reg = 0x00 }, > +}; > + > +/* PHY ESC Error Monitor */ > +#define PHEERM_REG 0x74 > + > +/* PHY Clock Lane Monitor */ > +#define PHCLM_REG 0x78 > + > +/* PHY Data Lane Monitor */ > +#define PHDLM_REG 0x7c > + > +/* CSI0CLK Frequency Configuration Preset Register */ > +#define CSI0CLKFCPR_REG 0x260 > +#define CSI0CLKFREQRANGE(n) ((n & 0x3f) << 16) > + > +struct rcar_csi2_format { > + u32 code; > + unsigned int datatype; > + unsigned int bpp; > +}; > + > +static const struct rcar_csi2_format rcar_csi2_formats[] = { > + { .code = MEDIA_BUS_FMT_RGB888_1X24, .datatype = 0x24, .bpp = 24 }, > + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 }, > + { .code = MEDIA_BUS_FMT_UYVY8_2X8, .datatype = 0x1e, .bpp = 16 }, > + { .code = MEDIA_BUS_FMT_YUYV10_2X10, .datatype = 0x1e, .bpp = 16 }, > +}; > + > +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++) > + if (rcar_csi2_formats[i].code == code) > + return rcar_csi2_formats + i; I would have written this as: return &rcar_csi2_formats[i]; but I think your way is valid too :) I have a nice 'for_each_entity_in_array' macro I keep meaning to post which would be nice here too - but hey - for another day. > + > + return NULL; > +} > + > +enum rcar_csi2_pads { > + RCAR_CSI2_SINK, > + RCAR_CSI2_SOURCE_VC0, > + RCAR_CSI2_SOURCE_VC1, > + RCAR_CSI2_SOURCE_VC2, > + RCAR_CSI2_SOURCE_VC3, > + NR_OF_RCAR_CSI2_PAD, > +}; > + > +struct rcar_csi2_info { > + const struct phypll_hsfreqrange *hsfreqrange; > + unsigned int csi0clkfreqrange; > + bool clear_ulps; > + bool init_phtw; > +}; > + > +struct rcar_csi2 { > + struct device *dev; > + void __iomem *base; > + const struct rcar_csi2_info *info; > + > + struct v4l2_subdev subdev; > + struct media_pad pads[NR_OF_RCAR_CSI2_PAD]; > + > + struct v4l2_async_notifier notifier; > + struct v4l2_async_subdev asd; > + struct v4l2_subdev *remote; > + > + struct v4l2_mbus_framefmt mf; > + > + struct mutex lock; > + int stream_count; > + > + unsigned short lanes; > + unsigned char lane_swap[4]; > +}; > + > +static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct rcar_csi2, subdev); > +} > + > +static inline struct rcar_csi2 *notifier_to_csi2(struct v4l2_async_notifier *n) > +{ > + return container_of(n, struct rcar_csi2, notifier); > +} > + > +static u32 rcar_csi2_read(struct rcar_csi2 *priv, unsigned int reg) > +{ > + return ioread32(priv->base + reg); > +} > + > +static void rcar_csi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data) > +{ > + iowrite32(data, priv->base + reg); > +} > + > +static void rcar_csi2_reset(struct rcar_csi2 *priv) > +{ > + rcar_csi2_write(priv, SRST_REG, SRST_SRST); > + usleep_range(100, 150); > + rcar_csi2_write(priv, SRST_REG, 0); > +} > + > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv) > +{ > + int timeout; > + > + /* Wait for the clock and data lanes to enter LP-11 state. */ > + for (timeout = 100; timeout > 0; timeout--) { > + const u32 lane_mask = (1 << priv->lanes) - 1; > + > + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 && The '1' is not very clear here.. Could this be: if ((rcar_csi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATE) && > + (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > + return 0; > + > + msleep(20); > + } > + > + dev_err(priv->dev, "Timeout waiting for LP-11 state\n"); Does LP == ULP ? I presume these mean 'low power' / 'ultra low power' ? Although the PHCLM lane mask refers to the STOPSTATE, so I guess this is also waiting for the lanes to be idle. > + > + return -ETIMEDOUT; > +} > + > +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp, > + u32 *phypll) > +{ > + const struct phypll_hsfreqrange *hsfreq; > + struct v4l2_subdev *source; > + struct v4l2_ctrl *ctrl; > + u64 mbps; > + > + if (!priv->remote) > + return -ENODEV; > + > + source = priv->remote; > + > + /* Read the pixel rate control from remote */ > + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > + if (!ctrl) { > + dev_err(priv->dev, "no pixel rate control in subdev %s\n", > + source->name); > + return -EINVAL; > + } > + > + /* Calculate the phypll */ > + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > + do_div(mbps, priv->lanes * 1000000); > + > + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) > + if (hsfreq->mbps >= mbps) > + break; > + > + if (!hsfreq->mbps) { > + dev_err(priv->dev, "Unsupported PHY speed (%llu Mbps)", mbps); > + return -ERANGE; > + } > + > + dev_dbg(priv->dev, "PHY HSFREQRANGE requested %llu got %u Mbps\n", mbps, > + hsfreq->mbps); > + > + *phypll = PHYPLL_HSFREQRANGE(hsfreq->reg); > + > + return 0; > +} > + > +static int rcar_csi2_start(struct rcar_csi2 *priv) > +{ > + const struct rcar_csi2_format *format; > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; > + unsigned int i; > + int ret; > + > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > + priv->mf.width, priv->mf.height, > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > + > + /* Code is validated in set_fmt */ > + format = rcar_csi2_code_to_fmt(priv->mf.code); > + > + /* > + * Enable all Virtual Channels > + * > + * NOTE: It's not possible to get individual datatype for each > + * source virtual channel. Once this is possible in V4L2 > + * it should be used here. > + */ > + for (i = 0; i < 4; i++) { Does 4 represent the maximum number of lanes? or can we have 4 VCs on a single lane ? > + u32 vcdt_part; > + > + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON | > + VCDT_SEL_DT(format->datatype); > + > + /* Store in correct reg and offset */ > + if (i < 2) > + vcdt |= vcdt_part << ((i % 2) * 16); > + else > + vcdt2 |= vcdt_part << ((i % 2) * 16); > + } > + > + switch (priv->lanes) { > + case 1: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > + break; > + case 2: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > + break; > + case 4: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > + break; > + default: > + return -EINVAL; > + } > + > + ret = rcar_csi2_calc_phypll(priv, format->bpp, &phypll); > + if (ret) > + return ret; > + > + /* Clear Ultra Low Power interrupt */ > + if (priv->info->clear_ulps) > + rcar_csi2_write(priv, INTSTATE_REG, > + INTSTATE_INT_ULPS_START | > + INTSTATE_INT_ULPS_END); > + > + /* Init */ > + rcar_csi2_write(priv, TREF_REG, TREF_TREF); > + rcar_csi2_reset(priv); > + rcar_csi2_write(priv, PHTC_REG, 0); > + > + /* Configure */ > + rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 | > + FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN); > + rcar_csi2_write(priv, VCDT_REG, vcdt); > + rcar_csi2_write(priv, VCDT2_REG, vcdt2); > + /* Lanes are zero indexed */ > + rcar_csi2_write(priv, LSWAP_REG, > + LSWAP_L0SEL(priv->lane_swap[0] - 1) | > + LSWAP_L1SEL(priv->lane_swap[1] - 1) | > + LSWAP_L2SEL(priv->lane_swap[2] - 1) | > + LSWAP_L3SEL(priv->lane_swap[3] - 1)); > + > + if (priv->info->init_phtw) { > + /* > + * This is for H3 ES2.0 > + * > + * NOTE: Additional logic is needed here when > + * support for V3H and/or M3-N is added > + */ > + rcar_csi2_write(priv, PHTW_REG, 0x01cc01e2); > + rcar_csi2_write(priv, PHTW_REG, 0x010101e3); > + rcar_csi2_write(priv, PHTW_REG, 0x010101e4); > + rcar_csi2_write(priv, PHTW_REG, 0x01100104); > + rcar_csi2_write(priv, PHTW_REG, 0x01030100); > + rcar_csi2_write(priv, PHTW_REG, 0x01800100); Voodoo magic ? I think just say yes here and move on ... I've looked at the flow chart in 25.3.9 ... I think I'll just believe this works :D especially as I've seen it working, but does it matter that the values aren't read back? Perhaps these could be moved to a (set of) table(s) and a loop to support the V3H/M3-N later (but not now) > + } > + > + /* Start */ > + rcar_csi2_write(priv, PHYPLL_REG, phypll); > + > + /* Set frequency range if we have it */ > + if (priv->info->csi0clkfreqrange) > + rcar_csi2_write(priv, CSI0CLKFCPR_REG, > + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange)); > + > + rcar_csi2_write(priv, PHYCNT_REG, phycnt); > + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN | > + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP); > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ); > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | > + PHYCNT_RSTZ); > + > + return rcar_csi2_wait_phy_start(priv); > +} > + > +static void rcar_csi2_stop(struct rcar_csi2 *priv) > +{ > + rcar_csi2_write(priv, PHYCNT_REG, 0); > + > + rcar_csi2_reset(priv); > +} > + > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable) > +{ > + struct rcar_csi2 *priv = sd_to_csi2(sd); > + struct v4l2_subdev *nextsd; > + int ret = 0; > + > + mutex_lock(&priv->lock); > + > + if (!priv->remote) { > + ret = -ENODEV; > + goto out; > + } > + > + nextsd = priv->remote; > + > + if (enable && priv->stream_count == 0) { > + pm_runtime_get_sync(priv->dev); > + > + ret = rcar_csi2_start(priv); > + if (ret) { > + pm_runtime_put(priv->dev); > + goto out; > + } > + > + ret = v4l2_subdev_call(nextsd, video, s_stream, 1); Would it be nicer to pass 'enable' through instead of the '1'? (of course enable==1 here) > + if (ret) { > + rcar_csi2_stop(priv); > + pm_runtime_put(priv->dev); > + goto out; > + } > + } else if (!enable && priv->stream_count == 1) { > + rcar_csi2_stop(priv); > + v4l2_subdev_call(nextsd, video, s_stream, 0); likewise here... > + pm_runtime_put(priv->dev); > + } What's the 'nextsd' in this instance? We only call the s_stream on nextsd if it is the first one to be started, or the last one to be stopped... I can understand this logic for calling rcar_csi2_stop/rcar_csi2_start ... but shouldn't we always call : v4l2_subdev_call(nextsd, video, s_stream, enable); ? in GMSL context, I guess 'nextsd' is the MAX9286? At which point - it would be called for start/stop, priv->stream_count times, and would also have to provide it's own call balancing... Call me crazy and ignore me here if you like :-) > + > + priv->stream_count += enable ? 1 : -1; > +out: > + mutex_unlock(&priv->lock); > + > + return ret; > +} > + > +static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *format) > +{ > + struct rcar_csi2 *priv = sd_to_csi2(sd); > + struct v4l2_mbus_framefmt *framefmt; > + > + if (!rcar_csi2_code_to_fmt(format->format.code)) > + return -EINVAL; > + > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > + priv->mf = format->format; > + } else { > + framefmt = v4l2_subdev_get_try_format(sd, cfg, 0); > + *framefmt = format->format; > + } > + > + return 0; > +} > + > +static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *format) > +{ > + struct rcar_csi2 *priv = sd_to_csi2(sd); > + > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > + format->format = priv->mf; > + else > + format->format = *v4l2_subdev_get_try_format(sd, cfg, 0); > + > + return 0; > +} > + > +static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = { > + .s_stream = rcar_csi2_s_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = { > + .set_fmt = rcar_csi2_set_pad_format, > + .get_fmt = rcar_csi2_get_pad_format, > +}; > + > +static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = { > + .video = &rcar_csi2_video_ops, > + .pad = &rcar_csi2_pad_ops, > +}; > + > +/* ----------------------------------------------------------------------------- > + * Async and registered of subdevices and links "Async handling and registration of subdevices and links" ? or does of mean OF here ? > + */ > + > +static int rcar_csi2_notify_bound(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *subdev, > + struct v4l2_async_subdev *asd) > +{ > + struct rcar_csi2 *priv = notifier_to_csi2(notifier); > + int pad; > + > + pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode, > + MEDIA_PAD_FL_SOURCE); > + if (pad < 0) { > + dev_err(priv->dev, "Failed to find pad for %s\n", subdev->name); > + return pad; > + } > + > + priv->remote = subdev; > + > + dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); > + > + return media_create_pad_link(&subdev->entity, pad, > + &priv->subdev.entity, 0, > + MEDIA_LNK_FL_ENABLED | > + MEDIA_LNK_FL_IMMUTABLE); > +} > + > +static void rcar_csi2_notify_unbind(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *subdev, > + struct v4l2_async_subdev *asd) > +{ > + struct rcar_csi2 *priv = notifier_to_csi2(notifier); > + > + priv->remote = NULL; > + > + dev_dbg(priv->dev, "Unbind %s\n", subdev->name); > +} > + > +static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = { > + .bound = rcar_csi2_notify_bound, > + .unbind = rcar_csi2_notify_unbind, > +}; > + > +static int rcar_csi2_parse_v4l2(struct rcar_csi2 *priv, > + struct v4l2_fwnode_endpoint *vep) > +{ > + unsigned int i; > + > + /* Only port 0 endpoint 0 is valid */ > + if (vep->base.port || vep->base.id) > + return -ENOTCONN; > + > + if (vep->bus_type != V4L2_MBUS_CSI2) { > + dev_err(priv->dev, "Unsupported bus: 0x%x\n", vep->bus_type); > + return -EINVAL; > + } > + > + priv->lanes = vep->bus.mipi_csi2.num_data_lanes; > + if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { > + dev_err(priv->dev, "Unsupported number of data-lanes: %u\n", > + priv->lanes); > + return -EINVAL; > + } > + > + for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { > + priv->lane_swap[i] = i < priv->lanes ? > + vep->bus.mipi_csi2.data_lanes[i] : i; > + > + /* Check for valid lane number */ > + if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) { > + dev_err(priv->dev, "data-lanes must be in 1-4 range\n"); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv) > +{ > + struct device_node *ep; > + struct v4l2_fwnode_endpoint v4l2_ep; > + int ret; > + > + ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0); > + if (!ep) { > + dev_err(priv->dev, "Not connected to subdevice\n"); > + return -EINVAL; > + } > + > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); > + if (ret) { > + dev_err(priv->dev, "Could not parse v4l2 endpoint\n"); > + of_node_put(ep); > + return -EINVAL; > + } > + > + ret = rcar_csi2_parse_v4l2(priv, &v4l2_ep); > + if (ret) { > + of_node_put(ep); > + return ret; > + } > + > + priv->asd.match.fwnode = > + fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep)); > + priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > + > + of_node_put(ep); > + > + priv->notifier.subdevs = devm_kzalloc(priv->dev, > + sizeof(*priv->notifier.subdevs), > + GFP_KERNEL); > + if (priv->notifier.subdevs == NULL) > + return -ENOMEM; > + > + priv->notifier.num_subdevs = 1; > + priv->notifier.subdevs[0] = &priv->asd; > + priv->notifier.ops = &rcar_csi2_notify_ops; > + > + dev_dbg(priv->dev, "Found '%pOF'\n", > + to_of_node(priv->asd.match.fwnode)); > + > + return v4l2_async_subdev_notifier_register(&priv->subdev, > + &priv->notifier); > +} > + > +/* ----------------------------------------------------------------------------- > + * Platform Device Driver > + */ > + > +static const struct media_entity_operations rcar_csi2_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, > +}; > + > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv, > + struct platform_device *pdev) > +{ > + struct resource *res; > + int irq; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + return 0; > +} > + > +static const struct rcar_csi2_info rcar_csi2_info_r8a7795 = { > + .hsfreqrange = hsfreqrange_h3_v3h_m3n, > + .clear_ulps = true, > + .init_phtw = true, > + .csi0clkfreqrange = 0x20, 0x20 ? > +}; > + > +static const struct rcar_csi2_info rcar_csi2_info_r8a7795es1 = { > + .hsfreqrange = hsfreqrange_m3w_h3es1, > +}; > + > +static const struct rcar_csi2_info rcar_csi2_info_r8a7796 = { > + .hsfreqrange = hsfreqrange_m3w_h3es1, > +}; > + > +static const struct of_device_id rcar_csi2_of_table[] = { > + { > + .compatible = "renesas,r8a7795-csi2", > + .data = &rcar_csi2_info_r8a7795, > + }, > + { > + .compatible = "renesas,r8a7796-csi2", > + .data = &rcar_csi2_info_r8a7796, > + }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, rcar_csi2_of_table); > + > +static const struct soc_device_attribute r8a7795es1[] = { > + { > + .soc_id = "r8a7795", .revision = "ES1.*", > + .data = &rcar_csi2_info_r8a7795es1, > + }, > + { /* sentinel */} > +}; > + > +static int rcar_csi2_probe(struct platform_device *pdev) > +{ > + const struct soc_device_attribute *attr; > + struct rcar_csi2 *priv; > + unsigned int i; > + int ret; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->info = of_device_get_match_data(&pdev->dev); > + > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ /* * r8a7795 ES1.x behaves differently than the ES2.0+ but doesn't have it's own * compatible string */ > + attr = soc_device_match(r8a7795es1); > + if (attr) > + priv->info = attr->data; > + > + priv->dev = &pdev->dev; > + > + mutex_init(&priv->lock); > + priv->stream_count = 0; > + > + ret = rcar_csi2_probe_resources(priv, pdev); > + if (ret) { > + dev_err(priv->dev, "Failed to get resources\n"); > + return ret; > + } > + > + platform_set_drvdata(pdev, priv); > + > + ret = rcar_csi2_parse_dt(priv); > + if (ret) > + return ret; > + > + priv->subdev.owner = THIS_MODULE; > + priv->subdev.dev = &pdev->dev; > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", > + KBUILD_MODNAME, dev_name(&pdev->dev)); > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > + > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > + priv->subdev.entity.ops = &rcar_csi2_entity_ops; > + > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, > + priv->pads); > + if (ret) > + goto error; > + > + pm_runtime_enable(&pdev->dev); > + > + ret = v4l2_async_register_subdev(&priv->subdev); > + if (ret < 0) > + goto error; > + > + dev_info(priv->dev, "%d lanes found\n", priv->lanes); > + > + return 0; > + > +error: > + v4l2_async_notifier_unregister(&priv->notifier); > + v4l2_async_notifier_cleanup(&priv->notifier); > + > + return ret; > +} > + > +static int rcar_csi2_remove(struct platform_device *pdev) > +{ > + struct rcar_csi2 *priv = platform_get_drvdata(pdev); > + > + v4l2_async_notifier_unregister(&priv->notifier); > + v4l2_async_notifier_cleanup(&priv->notifier); > + v4l2_async_unregister_subdev(&priv->subdev); > + > + pm_runtime_disable(&pdev->dev); > + > + return 0; > +} > + > +static struct platform_driver __refdata rcar_csi2_pdrv = { > + .remove = rcar_csi2_remove, > + .probe = rcar_csi2_probe, > + .driver = { > + .name = "rcar-csi2", > + .of_match_table = rcar_csi2_of_table, > + }, > +}; > + > +module_platform_driver(rcar_csi2_pdrv); > + > +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>"); > +MODULE_DESCRIPTION("Renesas R-Car MIPI CSI-2 receiver"); > +MODULE_LICENSE("GPL"); > Well ... phew ... done :) I can go to bed.
Hi Niklas, On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote: > + switch (priv->lanes) { > + case 1: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > + break; > + case 2: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > + break; > + case 4: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > + break; > + default: > + return -EINVAL; > + } I guess you could have a simpler construct here using this: phycnt = PHYCNT_ENABLECLK; switch (priv->lanes) { case 4: phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2; case 2: phycnt |= PHYCNT_ENABLE_1; case 1: phycnt |= PHYCNT_ENABLE_0; break; default: return -EINVAL; } But that's really up to you. > +static int rcar_csi2_probe(struct platform_device *pdev) > +{ > + const struct soc_device_attribute *attr; > + struct rcar_csi2 *priv; > + unsigned int i; > + int ret; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->info = of_device_get_match_data(&pdev->dev); > + > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ > + attr = soc_device_match(r8a7795es1); > + if (attr) > + priv->info = attr->data; > + > + priv->dev = &pdev->dev; > + > + mutex_init(&priv->lock); > + priv->stream_count = 0; > + > + ret = rcar_csi2_probe_resources(priv, pdev); > + if (ret) { > + dev_err(priv->dev, "Failed to get resources\n"); > + return ret; > + } > + > + platform_set_drvdata(pdev, priv); > + > + ret = rcar_csi2_parse_dt(priv); > + if (ret) > + return ret; > + > + priv->subdev.owner = THIS_MODULE; > + priv->subdev.dev = &pdev->dev; > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", > + KBUILD_MODNAME, dev_name(&pdev->dev)); > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > + > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > + priv->subdev.entity.ops = &rcar_csi2_entity_ops; > + > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, > + priv->pads); > + if (ret) > + goto error; > + > + pm_runtime_enable(&pdev->dev); Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the device uninitialised at probe, and pm_runtime_get_sync will not initialise it either if CONFIG_PM is not enabled. I guess you could call your runtime_resume function unconditionally, and mark the device as active in runtime_pm using pm_runtime_set_active. Looks good otherwise, once fixed (and if relevant): Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com> Maxime
Hi Niklas, Thank you for the patch. On Tuesday, 13 February 2018 01:01:32 EEST Niklas Söderlund wrote: > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > connected between the video sources and the video grabbers (VIN). > > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/platform/rcar-vin/Kconfig | 12 + > drivers/media/platform/rcar-vin/Makefile | 1 + > drivers/media/platform/rcar-vin/rcar-csi2.c | 884 +++++++++++++++++++++++++ > 3 files changed, 897 insertions(+) > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c [snip] > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644 > index 0000000000000000..c0c2a763151bc928 > --- /dev/null > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c [snip] > +static const struct phypll_hsfreqrange hsfreqrange_m3w_h3es1[] = { > + { .mbps = 80, .reg = 0x00 }, > + { .mbps = 90, .reg = 0x10 }, > + { .mbps = 100, .reg = 0x20 }, > + { .mbps = 110, .reg = 0x30 }, > + { .mbps = 120, .reg = 0x01 }, > + { .mbps = 130, .reg = 0x11 }, > + { .mbps = 140, .reg = 0x21 }, > + { .mbps = 150, .reg = 0x31 }, > + { .mbps = 160, .reg = 0x02 }, > + { .mbps = 170, .reg = 0x12 }, > + { .mbps = 180, .reg = 0x22 }, > + { .mbps = 190, .reg = 0x32 }, > + { .mbps = 205, .reg = 0x03 }, > + { .mbps = 220, .reg = 0x13 }, > + { .mbps = 235, .reg = 0x23 }, > + { .mbps = 250, .reg = 0x33 }, > + { .mbps = 275, .reg = 0x04 }, > + { .mbps = 300, .reg = 0x14 }, > + { .mbps = 325, .reg = 0x05 }, > + { .mbps = 350, .reg = 0x15 }, > + { .mbps = 400, .reg = 0x25 }, > + { .mbps = 450, .reg = 0x06 }, > + { .mbps = 500, .reg = 0x16 }, > + { .mbps = 550, .reg = 0x07 }, > + { .mbps = 600, .reg = 0x17 }, > + { .mbps = 650, .reg = 0x08 }, > + { .mbps = 700, .reg = 0x18 }, > + { .mbps = 750, .reg = 0x09 }, > + { .mbps = 800, .reg = 0x19 }, > + { .mbps = 850, .reg = 0x29 }, > + { .mbps = 900, .reg = 0x39 }, > + { .mbps = 950, .reg = 0x0A }, > + { .mbps = 1000, .reg = 0x1A }, > + { .mbps = 1050, .reg = 0x2A }, > + { .mbps = 1100, .reg = 0x3A }, > + { .mbps = 1150, .reg = 0x0B }, > + { .mbps = 1200, .reg = 0x1B }, > + { .mbps = 1250, .reg = 0x2B }, > + { .mbps = 1300, .reg = 0x3B }, > + { .mbps = 1350, .reg = 0x0C }, > + { .mbps = 1400, .reg = 0x1C }, > + { .mbps = 1450, .reg = 0x2C }, > + { .mbps = 1500, .reg = 0x3C }, All the other hex values in the file are lowercase, I'd do the same here. > + /* guard */ > + { .mbps = 0, .reg = 0x00 }, > +}; [snip] > +static int rcar_csi2_start(struct rcar_csi2 *priv) > +{ > + const struct rcar_csi2_format *format; > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; > + unsigned int i; > + int ret; > + > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > + priv->mf.width, priv->mf.height, > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > + > + /* Code is validated in set_fmt */ > + format = rcar_csi2_code_to_fmt(priv->mf.code); You could store the format pointer iin the rcar_csi2 structure to avoid looking it up here. > + /* > + * Enable all Virtual Channels > + * > + * NOTE: It's not possible to get individual datatype for each > + * source virtual channel. Once this is possible in V4L2 > + * it should be used here. > + */ > + for (i = 0; i < 4; i++) { > + u32 vcdt_part; > + > + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON | > + VCDT_SEL_DT(format->datatype); > + > + /* Store in correct reg and offset */ > + if (i < 2) > + vcdt |= vcdt_part << ((i % 2) * 16); > + else > + vcdt2 |= vcdt_part << ((i % 2) * 16); > + } > + > + switch (priv->lanes) { > + case 1: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > + break; > + case 2: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > + break; > + case 4: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > + break; > + default: > + return -EINVAL; > + } > + > + ret = rcar_csi2_calc_phypll(priv, format->bpp, &phypll); > + if (ret) > + return ret; > + > + /* Clear Ultra Low Power interrupt */ > + if (priv->info->clear_ulps) > + rcar_csi2_write(priv, INTSTATE_REG, > + INTSTATE_INT_ULPS_START | > + INTSTATE_INT_ULPS_END); > + > + /* Init */ > + rcar_csi2_write(priv, TREF_REG, TREF_TREF); > + rcar_csi2_reset(priv); > + rcar_csi2_write(priv, PHTC_REG, 0); > + > + /* Configure */ > + rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 | > + FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN); > + rcar_csi2_write(priv, VCDT_REG, vcdt); > + rcar_csi2_write(priv, VCDT2_REG, vcdt2); > + /* Lanes are zero indexed */ > + rcar_csi2_write(priv, LSWAP_REG, > + LSWAP_L0SEL(priv->lane_swap[0] - 1) | > + LSWAP_L1SEL(priv->lane_swap[1] - 1) | > + LSWAP_L2SEL(priv->lane_swap[2] - 1) | > + LSWAP_L3SEL(priv->lane_swap[3] - 1)); > + > + if (priv->info->init_phtw) { > + /* > + * This is for H3 ES2.0 > + * > + * NOTE: Additional logic is needed here when > + * support for V3H and/or M3-N is added > + */ > + rcar_csi2_write(priv, PHTW_REG, 0x01cc01e2); > + rcar_csi2_write(priv, PHTW_REG, 0x010101e3); > + rcar_csi2_write(priv, PHTW_REG, 0x010101e4); > + rcar_csi2_write(priv, PHTW_REG, 0x01100104); > + rcar_csi2_write(priv, PHTW_REG, 0x01030100); > + rcar_csi2_write(priv, PHTW_REG, 0x01800100); > + } > + > + /* Start */ > + rcar_csi2_write(priv, PHYPLL_REG, phypll); > + > + /* Set frequency range if we have it */ > + if (priv->info->csi0clkfreqrange) > + rcar_csi2_write(priv, CSI0CLKFCPR_REG, > + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange)); > + > + rcar_csi2_write(priv, PHYCNT_REG, phycnt); > + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN | > + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP); > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ); > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | > + PHYCNT_RSTZ); > + > + return rcar_csi2_wait_phy_start(priv); > +} [snip] > +static int rcar_csi2_parse_v4l2(struct rcar_csi2 *priv, > + struct v4l2_fwnode_endpoint *vep) > +{ > + unsigned int i; > + > + /* Only port 0 endpoint 0 is valid */ > + if (vep->base.port || vep->base.id) > + return -ENOTCONN; > + > + if (vep->bus_type != V4L2_MBUS_CSI2) { > + dev_err(priv->dev, "Unsupported bus: 0x%x\n", vep->bus_type); I think you can print this in decimal with %u. > + return -EINVAL; > + } > + > + priv->lanes = vep->bus.mipi_csi2.num_data_lanes; > + if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { > + dev_err(priv->dev, "Unsupported number of data-lanes: %u\n", > + priv->lanes); > + return -EINVAL; > + } > + > + for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { > + priv->lane_swap[i] = i < priv->lanes ? > + vep->bus.mipi_csi2.data_lanes[i] : i; > + > + /* Check for valid lane number */ > + if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) { > + dev_err(priv->dev, "data-lanes must be in 1-4 range\n"); > + return -EINVAL; > + } > + } > + > + return 0; > +} [snip] > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv, > + struct platform_device *pdev) > +{ > + struct resource *res; > + int irq; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; You don't seem to use the IRQ. Is this meant to catch invalid DT that don't specify an IRQ, to make sure we'll always have one available when we'll need to later ? > + return 0; > +} [snip] > +static int rcar_csi2_probe(struct platform_device *pdev) > +{ > + const struct soc_device_attribute *attr; > + struct rcar_csi2 *priv; > + unsigned int i; > + int ret; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->info = of_device_get_match_data(&pdev->dev); > + > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ > + attr = soc_device_match(r8a7795es1); > + if (attr) > + priv->info = attr->data; > + > + priv->dev = &pdev->dev; > + > + mutex_init(&priv->lock); > + priv->stream_count = 0; > + > + ret = rcar_csi2_probe_resources(priv, pdev); > + if (ret) { > + dev_err(priv->dev, "Failed to get resources\n"); > + return ret; > + } > + > + platform_set_drvdata(pdev, priv); > + > + ret = rcar_csi2_parse_dt(priv); > + if (ret) > + return ret; > + > + priv->subdev.owner = THIS_MODULE; > + priv->subdev.dev = &pdev->dev; > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", > + KBUILD_MODNAME, dev_name(&pdev->dev)); > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > + > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > + priv->subdev.entity.ops = &rcar_csi2_entity_ops; > + > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, > + priv->pads); > + if (ret) > + goto error; > + > + pm_runtime_enable(&pdev->dev); > + > + ret = v4l2_async_register_subdev(&priv->subdev); > + if (ret < 0) > + goto error; > + > + dev_info(priv->dev, "%d lanes found\n", priv->lanes); priv->lanes is unsigned you should use %u. > + > + return 0; > + > +error: > + v4l2_async_notifier_unregister(&priv->notifier); > + v4l2_async_notifier_cleanup(&priv->notifier); > + > + return ret; > +} [snip] With these small issues fixed and Kieran's and Maxime's comments addressed as you see fit, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Hello, On Wednesday, 14 March 2018 00:23:49 EEST Kieran Bingham wrote: > Hi Niklas > > Thank you for this patch ... > I know it has been a lot of work getting this and the VIN together! > > On 13/02/18 00:01, Niklas Söderlund wrote: > > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > > connected between the video sources and the video grabbers (VIN). > > > > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > > I don't think there's actually anything major in the below - so with any > comments addressed, or specifically ignored you can add my: > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > tag if you like. > > > --- > > > > drivers/media/platform/rcar-vin/Kconfig | 12 + > > drivers/media/platform/rcar-vin/Makefile | 1 + > > drivers/media/platform/rcar-vin/rcar-csi2.c | 884 +++++++++++++++++++++++ > > 3 files changed, 897 insertions(+) > > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c [snip] > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > > b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644 > > index 0000000000000000..c0c2a763151bc928 > > --- /dev/null > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > @@ -0,0 +1,884 @@ > > +// SPDX-License-Identifier: GPL-2.0+ Do you intend making it GPL-2.0+ or did you mean GPL-2.0 ? > > +/* > > + * Driver for Renesas R-Car MIPI CSI-2 Receiver > > + * > > + * Copyright (C) 2018 Renesas Electronics Corp. > > + */ [snip] > > +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int > > code) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++) > > + if (rcar_csi2_formats[i].code == code) > > + return rcar_csi2_formats + i; > > I would have written this as: > return &rcar_csi2_formats[i]; but I think your way is valid too :) I would have done the same. > I have a nice 'for_each_entity_in_array' macro I keep meaning to post which > would be nice here too - but hey - for another day. > > > + > > + return NULL; > > +} [snip] > > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv) > > +{ > > + int timeout; > > + > > + /* Wait for the clock and data lanes to enter LP-11 state. */ > > + for (timeout = 100; timeout > 0; timeout--) { > > + const u32 lane_mask = (1 << priv->lanes) - 1; > > + > > + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 && > > The '1' is not very clear here.. Could this be: > > if ((rcar_csi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATE) && That reads a bit better, yes. > > + (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > > + return 0; > > + > > + msleep(20); > > + } > > + > > + dev_err(priv->dev, "Timeout waiting for LP-11 state\n"); > > Does LP == ULP ? I presume these mean 'low power' / 'ultra low power' ? > Although the PHCLM lane mask refers to the STOPSTATE, so I guess this is > also waiting for the lanes to be idle. LP-11 refers to the low-power single-ended state where both the + and - lanes are at high level. It is the bus idle state for D-PHY. The ultra-low power state is ULPS and is different (if I remember correctly the lines go to the LP-00 state). > > + > > + return -ETIMEDOUT; > > +} [snip] > > +static int rcar_csi2_start(struct rcar_csi2 *priv) > > +{ > > + const struct rcar_csi2_format *format; > > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; > > + unsigned int i; > > + int ret; > > + > > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > > + priv->mf.width, priv->mf.height, > > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > > + > > + /* Code is validated in set_fmt */ > > + format = rcar_csi2_code_to_fmt(priv->mf.code); > > + > > + /* > > + * Enable all Virtual Channels > > + * > > + * NOTE: It's not possible to get individual datatype for each > > + * source virtual channel. Once this is possible in V4L2 > > + * it should be used here. > > + */ > > + for (i = 0; i < 4; i++) { > > Does 4 represent the maximum number of lanes? or can we have 4 VCs on a > single lane ? Virtual channels and data lanes are independent. Of course the more virtual channels you need to carry the more bandwidth you will likely need, so you might need more data lanes, but there's no direct correlation. > > + u32 vcdt_part; > > + > > + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON | > > + VCDT_SEL_DT(format->datatype); > > + > > + /* Store in correct reg and offset */ > > + if (i < 2) > > + vcdt |= vcdt_part << ((i % 2) * 16); > > + else > > + vcdt2 |= vcdt_part << ((i % 2) * 16); > > + } > > + > > + switch (priv->lanes) { > > + case 1: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > > + break; > > + case 2: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + case 4: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + ret = rcar_csi2_calc_phypll(priv, format->bpp, &phypll); > > + if (ret) > > + return ret; > > + > > + /* Clear Ultra Low Power interrupt */ > > + if (priv->info->clear_ulps) > > + rcar_csi2_write(priv, INTSTATE_REG, > > + INTSTATE_INT_ULPS_START | > > + INTSTATE_INT_ULPS_END); > > + > > + /* Init */ > > + rcar_csi2_write(priv, TREF_REG, TREF_TREF); > > + rcar_csi2_reset(priv); > > + rcar_csi2_write(priv, PHTC_REG, 0); > > + > > + /* Configure */ > > + rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 | > > + FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN); > > + rcar_csi2_write(priv, VCDT_REG, vcdt); > > + rcar_csi2_write(priv, VCDT2_REG, vcdt2); > > + /* Lanes are zero indexed */ > > + rcar_csi2_write(priv, LSWAP_REG, > > + LSWAP_L0SEL(priv->lane_swap[0] - 1) | > > + LSWAP_L1SEL(priv->lane_swap[1] - 1) | > > + LSWAP_L2SEL(priv->lane_swap[2] - 1) | > > + LSWAP_L3SEL(priv->lane_swap[3] - 1)); > > + > > + if (priv->info->init_phtw) { > > + /* > > + * This is for H3 ES2.0 > > + * > > + * NOTE: Additional logic is needed here when > > + * support for V3H and/or M3-N is added > > + */ > > + rcar_csi2_write(priv, PHTW_REG, 0x01cc01e2); > > + rcar_csi2_write(priv, PHTW_REG, 0x010101e3); > > + rcar_csi2_write(priv, PHTW_REG, 0x010101e4); > > + rcar_csi2_write(priv, PHTW_REG, 0x01100104); > > + rcar_csi2_write(priv, PHTW_REG, 0x01030100); > > + rcar_csi2_write(priv, PHTW_REG, 0x01800100); > > Voodoo magic ? > > I think just say yes here and move on ... > I've looked at the flow chart in 25.3.9 ... I think I'll just believe this > works :D especially as I've seen it working, but does it matter that the > values aren't read back? > > Perhaps these could be moved to a (set of) table(s) and a loop to support > the V3H/M3-N later (but not now) I think we can fix this later, yes. > > + } > > + > > + /* Start */ > > + rcar_csi2_write(priv, PHYPLL_REG, phypll); > > + > > + /* Set frequency range if we have it */ > > + if (priv->info->csi0clkfreqrange) > > + rcar_csi2_write(priv, CSI0CLKFCPR_REG, > > + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange)); > > + > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt); > > + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN | > > + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP); > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ); > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | > > + PHYCNT_RSTZ); > > + > > + return rcar_csi2_wait_phy_start(priv); > > +} [snip] > > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable) > > +{ > > + struct rcar_csi2 *priv = sd_to_csi2(sd); > > + struct v4l2_subdev *nextsd; > > + int ret = 0; > > + > > + mutex_lock(&priv->lock); > > + > > + if (!priv->remote) { > > + ret = -ENODEV; > > + goto out; > > + } > > + > > + nextsd = priv->remote; > > + > > + if (enable && priv->stream_count == 0) { > > + pm_runtime_get_sync(priv->dev); > > + > > + ret = rcar_csi2_start(priv); > > + if (ret) { > > + pm_runtime_put(priv->dev); > > + goto out; > > + } > > + > > + ret = v4l2_subdev_call(nextsd, video, s_stream, 1); > > Would it be nicer to pass 'enable' through instead of the '1'? (of course > enable==1 here) The caller shouldn't set enable to a value other than 0 or 1, but in case it does, 1 seems better to me (and I find it more readable too). > > + if (ret) { > > + rcar_csi2_stop(priv); > > + pm_runtime_put(priv->dev); > > + goto out; > > + } > > + } else if (!enable && priv->stream_count == 1) { > > + rcar_csi2_stop(priv); > > + v4l2_subdev_call(nextsd, video, s_stream, 0); > > likewise here... Likewise, I find 0 more readable :-) > > + pm_runtime_put(priv->dev); > > + } > > What's the 'nextsd' in this instance? We only call the s_stream on nextsd if > it is the first one to be started, or the last one to be stopped... > > I can understand this logic for calling rcar_csi2_stop/rcar_csi2_start ... > but shouldn't we always call : > v4l2_subdev_call(nextsd, video, s_stream, enable); ? > > in GMSL context, I guess 'nextsd' is the MAX9286? > At which point - it would be called for start/stop, priv->stream_count > times, and would also have to provide it's own call balancing... > > Call me crazy and ignore me here if you like :-) I'll let Niklas explain :-) > > + > > + priv->stream_count += enable ? 1 : -1; > > +out: > > + mutex_unlock(&priv->lock); > > + > > + return ret; > > +} [snip] > > +static const struct rcar_csi2_info rcar_csi2_info_r8a7795 = { > > + .hsfreqrange = hsfreqrange_h3_v3h_m3n, > > + .clear_ulps = true, > > + .init_phtw = true, > > + .csi0clkfreqrange = 0x20, > > 0x20 ? The datasheet states that the field should be set to 0x20 and doesn't offer any other explanation :-/ > > +}; [snip] > > +static int rcar_csi2_probe(struct platform_device *pdev) > > +{ > > + const struct soc_device_attribute *attr; > > + struct rcar_csi2 *priv; > > + unsigned int i; > > + int ret; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->info = of_device_get_match_data(&pdev->dev); > > + > > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ > > /* > * r8a7795 ES1.x behaves differently than the ES2.0+ but doesn't have it's > own > * compatible string > */ With a period at the end of the sentence if would be perfect :-) > > + attr = soc_device_match(r8a7795es1); > > + if (attr) > > + priv->info = attr->data; > > + > > + priv->dev = &pdev->dev; > > + > > + mutex_init(&priv->lock); > > + priv->stream_count = 0; > > + > > + ret = rcar_csi2_probe_resources(priv, pdev); > > + if (ret) { > > + dev_err(priv->dev, "Failed to get resources\n"); > > + return ret; > > + } > > + > > + platform_set_drvdata(pdev, priv); > > + > > + ret = rcar_csi2_parse_dt(priv); > > + if (ret) > > + return ret; > > + > > + priv->subdev.owner = THIS_MODULE; > > + priv->subdev.dev = &pdev->dev; > > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); > > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); > > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", > > + KBUILD_MODNAME, dev_name(&pdev->dev)); > > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > > + > > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > > + priv->subdev.entity.ops = &rcar_csi2_entity_ops; > > + > > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; > > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) > > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; > > + > > + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, > > + priv->pads); > > + if (ret) > > + goto error; > > + > > + pm_runtime_enable(&pdev->dev); > > + > > + ret = v4l2_async_register_subdev(&priv->subdev); > > + if (ret < 0) > > + goto error; > > + > > + dev_info(priv->dev, "%d lanes found\n", priv->lanes); > > + > > + return 0; > > + > > +error: > > + v4l2_async_notifier_unregister(&priv->notifier); > > + v4l2_async_notifier_cleanup(&priv->notifier); > > + > > + return ret; > > +} [snip] > Well ... phew ... done :) I can go to bed. I wish I could do the same :-)
Hi Maxime, On Thursday, 29 March 2018 14:30:39 EEST Maxime Ripard wrote: > On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote: > > + switch (priv->lanes) { > > + case 1: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > > + break; > > + case 2: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + case 4: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + default: > > + return -EINVAL; > > + } > > I guess you could have a simpler construct here using this: > > phycnt = PHYCNT_ENABLECLK; > > switch (priv->lanes) { > case 4: > phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2; > case 2: > phycnt |= PHYCNT_ENABLE_1; > case 1: > phycnt |= PHYCNT_ENABLE_0; > break; > > default: > return -EINVAL; > } > > But that's really up to you. Wouldn't Niklas' version generate simpler code as it uses direct assignments ? > > +static int rcar_csi2_probe(struct platform_device *pdev) > > +{ > > + const struct soc_device_attribute *attr; > > + struct rcar_csi2 *priv; > > + unsigned int i; > > + int ret; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->info = of_device_get_match_data(&pdev->dev); > > + > > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ > > + attr = soc_device_match(r8a7795es1); > > + if (attr) > > + priv->info = attr->data; > > + > > + priv->dev = &pdev->dev; > > + > > + mutex_init(&priv->lock); > > + priv->stream_count = 0; > > + > > + ret = rcar_csi2_probe_resources(priv, pdev); > > + if (ret) { > > + dev_err(priv->dev, "Failed to get resources\n"); > > + return ret; > > + } > > + > > + platform_set_drvdata(pdev, priv); > > + > > + ret = rcar_csi2_parse_dt(priv); > > + if (ret) > > + return ret; > > + > > + priv->subdev.owner = THIS_MODULE; > > + priv->subdev.dev = &pdev->dev; > > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); > > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); > > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", > > + KBUILD_MODNAME, dev_name(&pdev->dev)); > > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > > + > > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > > + priv->subdev.entity.ops = &rcar_csi2_entity_ops; > > + > > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; > > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) > > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; > > + > > + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, > > + priv->pads); > > + if (ret) > > + goto error; > > + > > + pm_runtime_enable(&pdev->dev); > > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the > device uninitialised at probe, and pm_runtime_get_sync will not > initialise it either if CONFIG_PM is not enabled. I guess you could > call your runtime_resume function unconditionally, and mark the device > as active in runtime_pm using pm_runtime_set_active. > > Looks good otherwise, once fixed (and if relevant): > Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com>
Hi Maxime and Niklas, On Thu, Mar 29, 2018 at 01:30:39PM +0200, Maxime Ripard wrote: > Hi Niklas, > > On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote: > > + switch (priv->lanes) { > > + case 1: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > > + break; > > + case 2: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + case 4: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + default: > > + return -EINVAL; > > + } > > I guess you could have a simpler construct here using this: > > phycnt = PHYCNT_ENABLECLK; > > switch (priv->lanes) { > case 4: > phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2; > case 2: > phycnt |= PHYCNT_ENABLE_1; > case 1: > phycnt |= PHYCNT_ENABLE_0; > break; > > default: > return -EINVAL; > } > > But that's really up to you. > > > +static int rcar_csi2_probe(struct platform_device *pdev) > > +{ > > + const struct soc_device_attribute *attr; > > + struct rcar_csi2 *priv; > > + unsigned int i; > > + int ret; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->info = of_device_get_match_data(&pdev->dev); > > + > > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ > > + attr = soc_device_match(r8a7795es1); > > + if (attr) > > + priv->info = attr->data; > > + > > + priv->dev = &pdev->dev; > > + > > + mutex_init(&priv->lock); > > + priv->stream_count = 0; > > + > > + ret = rcar_csi2_probe_resources(priv, pdev); > > + if (ret) { > > + dev_err(priv->dev, "Failed to get resources\n"); > > + return ret; > > + } > > + > > + platform_set_drvdata(pdev, priv); > > + > > + ret = rcar_csi2_parse_dt(priv); > > + if (ret) > > + return ret; > > + > > + priv->subdev.owner = THIS_MODULE; > > + priv->subdev.dev = &pdev->dev; > > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); > > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); > > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", > > + KBUILD_MODNAME, dev_name(&pdev->dev)); > > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > > + > > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > > + priv->subdev.entity.ops = &rcar_csi2_entity_ops; > > + > > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; > > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) > > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; > > + > > + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, > > + priv->pads); > > + if (ret) > > + goto error; > > + > > + pm_runtime_enable(&pdev->dev); > > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the > device uninitialised at probe, and pm_runtime_get_sync will not > initialise it either if CONFIG_PM is not enabled. I guess you could > call your runtime_resume function unconditionally, and mark the device > as active in runtime_pm using pm_runtime_set_active. There doesn't seem to be any runtime_resume function. Was there supposed to be one? Assuming runtime PM would actually do something here, you might add pm_runtime_idle() to power the device off after probing. I guess pm_runtime_set_active() should precede pm_runtime_enable().
On Wed, Apr 4, 2018 at 5:26 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thursday, 29 March 2018 14:30:39 EEST Maxime Ripard wrote: >> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote: >> > + switch (priv->lanes) { >> > + case 1: >> > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; >> > + break; >> > + case 2: >> > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; >> > + break; >> > + case 4: >> > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | >> > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; >> > + break; >> > + default: >> > + return -EINVAL; >> > + } >> >> I guess you could have a simpler construct here using this: >> >> phycnt = PHYCNT_ENABLECLK; >> >> switch (priv->lanes) { >> case 4: >> phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2; >> case 2: >> phycnt |= PHYCNT_ENABLE_1; >> case 1: >> phycnt |= PHYCNT_ENABLE_0; >> break; >> >> default: >> return -EINVAL; >> } >> >> But that's really up to you. > > Wouldn't Niklas' version generate simpler code as it uses direct assignments ? Alternatively, you could check for a valid number of lanes, and use knowledge about the internal lane bits: phycnt = PHYCNT_ENABLECLK; phycnt |= (1 << priv->lanes) - 1; Gr{oetje,eeting}s, Geert
Hi Geert, On Thursday, 5 April 2018 10:33:55 EEST Geert Uytterhoeven wrote: > On Wed, Apr 4, 2018 at 5:26 PM, Laurent Pinchart wrote: > > On Thursday, 29 March 2018 14:30:39 EEST Maxime Ripard wrote: > >> On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote: > >> > + switch (priv->lanes) { > >> > + case 1: > >> > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > >> > + break; > >> > + case 2: > >> > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | > >> > PHYCNT_ENABLE_0; > >> > + break; > >> > + case 4: > >> > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | > >> > PHYCNT_ENABLE_2 | > >> > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > >> > + break; > >> > + default: > >> > + return -EINVAL; > >> > + } > >> > >> I guess you could have a simpler construct here using this: > >> > >> phycnt = PHYCNT_ENABLECLK; > >> > >> switch (priv->lanes) { > >> > >> case 4: > >> phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2; > >> > >> case 2: > >> phycnt |= PHYCNT_ENABLE_1; > >> > >> case 1: > >> phycnt |= PHYCNT_ENABLE_0; > >> break; > >> > >> default: > >> return -EINVAL; > >> > >> } > >> > >> But that's really up to you. > > > > Wouldn't Niklas' version generate simpler code as it uses direct > > assignments ? > Alternatively, you could check for a valid number of lanes, and use > knowledge about the internal lane bits: > > phycnt = PHYCNT_ENABLECLK; > phycnt |= (1 << priv->lanes) - 1; If Niklas is fine with that, I like it too.
Hi Niklas, thanks for the VIN and CSI-2 effort! On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote: > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > connected between the video sources and the video grabbers (VIN). > > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/platform/rcar-vin/Kconfig | 12 + > drivers/media/platform/rcar-vin/Makefile | 1 + > drivers/media/platform/rcar-vin/rcar-csi2.c | 884 ++++++++++++++++++++++++++++ > 3 files changed, 897 insertions(+) > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c > [snip] > + > +static const struct rcar_csi2_format rcar_csi2_formats[] = { > + { .code = MEDIA_BUS_FMT_RGB888_1X24, .datatype = 0x24, .bpp = 24 }, > + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 }, > + { .code = MEDIA_BUS_FMT_UYVY8_2X8, .datatype = 0x1e, .bpp = 16 }, > + { .code = MEDIA_BUS_FMT_YUYV10_2X10, .datatype = 0x1e, .bpp = 16 }, Shouldn't YUYV10_2X10 format have 20 bits per pixel? > +}; > + > +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++) > + if (rcar_csi2_formats[i].code == code) > + return rcar_csi2_formats + i; > + > + return NULL; > +} > + > +enum rcar_csi2_pads { > + RCAR_CSI2_SINK, > + RCAR_CSI2_SOURCE_VC0, > + RCAR_CSI2_SOURCE_VC1, > + RCAR_CSI2_SOURCE_VC2, > + RCAR_CSI2_SOURCE_VC3, > + NR_OF_RCAR_CSI2_PAD, > +}; > + > +struct rcar_csi2_info { > + const struct phypll_hsfreqrange *hsfreqrange; > + unsigned int csi0clkfreqrange; > + bool clear_ulps; > + bool init_phtw; > +}; > + > +struct rcar_csi2 { > + struct device *dev; > + void __iomem *base; > + const struct rcar_csi2_info *info; > + > + struct v4l2_subdev subdev; > + struct media_pad pads[NR_OF_RCAR_CSI2_PAD]; > + > + struct v4l2_async_notifier notifier; > + struct v4l2_async_subdev asd; > + struct v4l2_subdev *remote; > + > + struct v4l2_mbus_framefmt mf; > + > + struct mutex lock; > + int stream_count; > + > + unsigned short lanes; > + unsigned char lane_swap[4]; > +}; > + > +static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct rcar_csi2, subdev); > +} > + > +static inline struct rcar_csi2 *notifier_to_csi2(struct v4l2_async_notifier *n) > +{ > + return container_of(n, struct rcar_csi2, notifier); > +} > + > +static u32 rcar_csi2_read(struct rcar_csi2 *priv, unsigned int reg) > +{ > + return ioread32(priv->base + reg); > +} > + > +static void rcar_csi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data) > +{ > + iowrite32(data, priv->base + reg); > +} > + > +static void rcar_csi2_reset(struct rcar_csi2 *priv) > +{ > + rcar_csi2_write(priv, SRST_REG, SRST_SRST); > + usleep_range(100, 150); > + rcar_csi2_write(priv, SRST_REG, 0); > +} > + > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv) > +{ > + int timeout; > + > + /* Wait for the clock and data lanes to enter LP-11 state. */ > + for (timeout = 100; timeout > 0; timeout--) { > + const u32 lane_mask = (1 << priv->lanes) - 1; > + > + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 && Nitpicking: if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) && Don't you prefer to provide defines also for bit fields instead of using magic values? In this case something like PHCLM_REG_STOPSTATE_CLK would do. Also, from tables 25.[17-20] it seems to me that for H3 and V3 you have to set INSTATE to an hardcoded value after having validated PHDLM. Maybe it is not necessary, just pointing it out. > + (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > + return 0; > + > + msleep(20); > + } > + > + dev_err(priv->dev, "Timeout waiting for LP-11 state\n"); > + > + return -ETIMEDOUT; > +} > + > +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp, > + u32 *phypll) > +{ > + const struct phypll_hsfreqrange *hsfreq; > + struct v4l2_subdev *source; > + struct v4l2_ctrl *ctrl; > + u64 mbps; > + > + if (!priv->remote) > + return -ENODEV; > + > + source = priv->remote; > + > + /* Read the pixel rate control from remote */ > + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > + if (!ctrl) { > + dev_err(priv->dev, "no pixel rate control in subdev %s\n", > + source->name); > + return -EINVAL; > + } > + > + /* Calculate the phypll */ > + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > + do_div(mbps, priv->lanes * 1000000); > + > + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) > + if (hsfreq->mbps >= mbps) > + break; > + > + if (!hsfreq->mbps) { > + dev_err(priv->dev, "Unsupported PHY speed (%llu Mbps)", mbps); > + return -ERANGE; > + } > + > + dev_dbg(priv->dev, "PHY HSFREQRANGE requested %llu got %u Mbps\n", mbps, > + hsfreq->mbps); > + > + *phypll = PHYPLL_HSFREQRANGE(hsfreq->reg); > + > + return 0; > +} > + > +static int rcar_csi2_start(struct rcar_csi2 *priv) > +{ > + const struct rcar_csi2_format *format; > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; > + unsigned int i; > + int ret; > + > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > + priv->mf.width, priv->mf.height, > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > + > + /* Code is validated in set_fmt */ > + format = rcar_csi2_code_to_fmt(priv->mf.code); > + > + /* > + * Enable all Virtual Channels > + * > + * NOTE: It's not possible to get individual datatype for each > + * source virtual channel. Once this is possible in V4L2 > + * it should be used here. > + */ > + for (i = 0; i < 4; i++) { > + u32 vcdt_part; > + > + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON | > + VCDT_SEL_DT(format->datatype); > + > + /* Store in correct reg and offset */ > + if (i < 2) > + vcdt |= vcdt_part << ((i % 2) * 16); > + else > + vcdt2 |= vcdt_part << ((i % 2) * 16); > + } > + > + switch (priv->lanes) { > + case 1: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > + break; > + case 2: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > + break; > + case 4: > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > + break; Even simpler this could be written as phycnt = PHYCNT_ENABLECLK | (1 << priv->lanes) - 1; > + default: > + return -EINVAL; Can this happen? You have validated priv->lanes already when parsing DT > + } > + > + ret = rcar_csi2_calc_phypll(priv, format->bpp, &phypll); > + if (ret) > + return ret; > + > + /* Clear Ultra Low Power interrupt */ > + if (priv->info->clear_ulps) > + rcar_csi2_write(priv, INTSTATE_REG, > + INTSTATE_INT_ULPS_START | > + INTSTATE_INT_ULPS_END); > + > + /* Init */ > + rcar_csi2_write(priv, TREF_REG, TREF_TREF); > + rcar_csi2_reset(priv); > + rcar_csi2_write(priv, PHTC_REG, 0); > + > + /* Configure */ > + rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 | > + FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN); On the FLD_FLD_NUM(2) mask. Why 2? I read on the datasheet "the register must not be changed from default value" and I read defaul to be 0x0000 Also, please consider a make as all other fields are enabled unconditionally. > + rcar_csi2_write(priv, VCDT_REG, vcdt); > + rcar_csi2_write(priv, VCDT2_REG, vcdt2); > + /* Lanes are zero indexed */ > + rcar_csi2_write(priv, LSWAP_REG, > + LSWAP_L0SEL(priv->lane_swap[0] - 1) | > + LSWAP_L1SEL(priv->lane_swap[1] - 1) | > + LSWAP_L2SEL(priv->lane_swap[2] - 1) | > + LSWAP_L3SEL(priv->lane_swap[3] - 1)); EDIT: (This comment is way too long for the real value it has, but since I already wrote it, and my initial doubt clarified while I was writing, resulting in a much less serious issues, I'm gonna keep it all anyway. Sorry about this :) Why - 1 ? Is this because it is assumed clock lane is in position 0? Is this fixed by design? What I read in datasheet for LSWAP_REG is: L[0-3]SEL 0 = Use PHY lane 0 1 = Use PHY lane 1 2 = Use PHY lane 2 3 = Use PHY lane 3 priv->lane_swap[i] is collected parsing 'data_lanes' property and should reflect the actual physical lane value assigned to logical lane numbers. If 'data_lanes' is, say <1 2> I expect priv->lane_swap[0] = 1; priv->lane_swap[1] = 2; priv->lane_swap[1] = 3; //assigned by your parsing routine priv->lane_swap[1] = 4; //assigned by your parsing routine And I understand LSWAP counts instead from [0-3] so, ok, I get why you subtract one. But now I wonder what happens if instead, lane position is specified counting from 0 in DT. Ah, I see you refuse lane_swap values < 1! So It should be assumed clock is by HW design on lane 0, so wouldn't you need to mention in DT bindings that the HW has clock lanes fixed in position 0 and the accepted values for the 'data_lanes' property ranges in the [1-4] interval? > + > + if (priv->info->init_phtw) { > + /* > + * This is for H3 ES2.0 > + * > + * NOTE: Additional logic is needed here when > + * support for V3H and/or M3-N is added > + */ > + rcar_csi2_write(priv, PHTW_REG, 0x01cc01e2); > + rcar_csi2_write(priv, PHTW_REG, 0x010101e3); > + rcar_csi2_write(priv, PHTW_REG, 0x010101e4); > + rcar_csi2_write(priv, PHTW_REG, 0x01100104); > + rcar_csi2_write(priv, PHTW_REG, 0x01030100); > + rcar_csi2_write(priv, PHTW_REG, 0x01800100); > + } > + > + /* Start */ > + rcar_csi2_write(priv, PHYPLL_REG, phypll); > + > + /* Set frequency range if we have it */ > + if (priv->info->csi0clkfreqrange) > + rcar_csi2_write(priv, CSI0CLKFCPR_REG, > + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange)); > + > + rcar_csi2_write(priv, PHYCNT_REG, phycnt); > + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN | > + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP); > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ); > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | > + PHYCNT_RSTZ); Nit: from tables 25.[17-20] it seems to me you do not have to re-issue PHYCNT_SHUTDOWNZ when writing PHYCNT_RSTZ to PHYCNT_REG. > + > + return rcar_csi2_wait_phy_start(priv); > +} > + > +static void rcar_csi2_stop(struct rcar_csi2 *priv) > +{ > + rcar_csi2_write(priv, PHYCNT_REG, 0); > + > + rcar_csi2_reset(priv); > +} > + > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable) > +{ > + struct rcar_csi2 *priv = sd_to_csi2(sd); > + struct v4l2_subdev *nextsd; > + int ret = 0; > + > + mutex_lock(&priv->lock); > + > + if (!priv->remote) { > + ret = -ENODEV; > + goto out; > + } Can this happen? The 'bind' callback sets priv->remote and it gets assigned back to NULL only on 'unbind'. Wouldn't it be better to remove the link in the media graph and let the system return an EPIPE before calling this? > + > + nextsd = priv->remote; > + > + if (enable && priv->stream_count == 0) { > + pm_runtime_get_sync(priv->dev); > + > + ret = rcar_csi2_start(priv); > + if (ret) { > + pm_runtime_put(priv->dev); > + goto out; > + } > + > + ret = v4l2_subdev_call(nextsd, video, s_stream, 1); > + if (ret) { > + rcar_csi2_stop(priv); > + pm_runtime_put(priv->dev); > + goto out; > + } > + } else if (!enable && priv->stream_count == 1) { > + rcar_csi2_stop(priv); > + v4l2_subdev_call(nextsd, video, s_stream, 0); > + pm_runtime_put(priv->dev); > + } > + > + priv->stream_count += enable ? 1 : -1; > +out: > + mutex_unlock(&priv->lock); > + > + return ret; > +} > + > +static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *format) > +{ > + struct rcar_csi2 *priv = sd_to_csi2(sd); > + struct v4l2_mbus_framefmt *framefmt; > + > + if (!rcar_csi2_code_to_fmt(format->format.code)) > + return -EINVAL; > + > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > + priv->mf = format->format; > + } else { > + framefmt = v4l2_subdev_get_try_format(sd, cfg, 0); > + *framefmt = format->format; > + } > + > + return 0; > +} > + > +static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *format) > +{ > + struct rcar_csi2 *priv = sd_to_csi2(sd); > + > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > + format->format = priv->mf; > + else > + format->format = *v4l2_subdev_get_try_format(sd, cfg, 0); > + > + return 0; > +} > + > +static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = { > + .s_stream = rcar_csi2_s_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = { > + .set_fmt = rcar_csi2_set_pad_format, > + .get_fmt = rcar_csi2_get_pad_format, > +}; > + > +static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = { > + .video = &rcar_csi2_video_ops, > + .pad = &rcar_csi2_pad_ops, > +}; > + > +/* ----------------------------------------------------------------------------- > + * Async and registered of subdevices and links > + */ > + > +static int rcar_csi2_notify_bound(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *subdev, > + struct v4l2_async_subdev *asd) > +{ > + struct rcar_csi2 *priv = notifier_to_csi2(notifier); > + int pad; > + > + pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode, > + MEDIA_PAD_FL_SOURCE); > + if (pad < 0) { > + dev_err(priv->dev, "Failed to find pad for %s\n", subdev->name); > + return pad; > + } > + > + priv->remote = subdev; > + > + dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); > + > + return media_create_pad_link(&subdev->entity, pad, > + &priv->subdev.entity, 0, > + MEDIA_LNK_FL_ENABLED | > + MEDIA_LNK_FL_IMMUTABLE); > +} > + > +static void rcar_csi2_notify_unbind(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *subdev, > + struct v4l2_async_subdev *asd) > +{ > + struct rcar_csi2 *priv = notifier_to_csi2(notifier); > + > + priv->remote = NULL; > + > + dev_dbg(priv->dev, "Unbind %s\n", subdev->name); > +} > + > +static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = { > + .bound = rcar_csi2_notify_bound, > + .unbind = rcar_csi2_notify_unbind, > +}; > + > +static int rcar_csi2_parse_v4l2(struct rcar_csi2 *priv, > + struct v4l2_fwnode_endpoint *vep) > +{ > + unsigned int i; > + > + /* Only port 0 endpoint 0 is valid */ > + if (vep->base.port || vep->base.id) > + return -ENOTCONN; > + > + if (vep->bus_type != V4L2_MBUS_CSI2) { > + dev_err(priv->dev, "Unsupported bus: 0x%x\n", vep->bus_type); > + return -EINVAL; > + } > + > + priv->lanes = vep->bus.mipi_csi2.num_data_lanes; > + if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { Is this an HW limitation? Like the 'data_lanes' comment, if it is, shouldn't you mention in bindings that the accepted lane numbers is limited to the [1,2,4] values. > + dev_err(priv->dev, "Unsupported number of data-lanes: %u\n", > + priv->lanes); > + return -EINVAL; > + } > + > + for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { > + priv->lane_swap[i] = i < priv->lanes ? > + vep->bus.mipi_csi2.data_lanes[i] : i; > + > + /* Check for valid lane number */ > + if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) { > + dev_err(priv->dev, "data-lanes must be in 1-4 range\n"); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv) > +{ > + struct device_node *ep; > + struct v4l2_fwnode_endpoint v4l2_ep; > + int ret; > + > + ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0); > + if (!ep) { > + dev_err(priv->dev, "Not connected to subdevice\n"); > + return -EINVAL; > + } > + > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); > + if (ret) { > + dev_err(priv->dev, "Could not parse v4l2 endpoint\n"); > + of_node_put(ep); > + return -EINVAL; > + } > + > + ret = rcar_csi2_parse_v4l2(priv, &v4l2_ep); > + if (ret) { > + of_node_put(ep); > + return ret; > + } > + > + priv->asd.match.fwnode = > + fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep)); > + priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > + > + of_node_put(ep); > + > + priv->notifier.subdevs = devm_kzalloc(priv->dev, > + sizeof(*priv->notifier.subdevs), > + GFP_KERNEL); > + if (priv->notifier.subdevs == NULL) Nit: you can use ! for NULL comparison. I think checkpatch --strict complains for this. > + return -ENOMEM; > + > + priv->notifier.num_subdevs = 1; > + priv->notifier.subdevs[0] = &priv->asd; > + priv->notifier.ops = &rcar_csi2_notify_ops; > + > + dev_dbg(priv->dev, "Found '%pOF'\n", > + to_of_node(priv->asd.match.fwnode)); > + > + return v4l2_async_subdev_notifier_register(&priv->subdev, > + &priv->notifier); > +} > + > +/* ----------------------------------------------------------------------------- > + * Platform Device Driver > + */ > + > +static const struct media_entity_operations rcar_csi2_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, > +}; > + > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv, > + struct platform_device *pdev) > +{ > + struct resource *res; > + int irq; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + return 0; > +} > + > +static const struct rcar_csi2_info rcar_csi2_info_r8a7795 = { > + .hsfreqrange = hsfreqrange_h3_v3h_m3n, > + .clear_ulps = true, > + .init_phtw = true, > + .csi0clkfreqrange = 0x20, > +}; > + > +static const struct rcar_csi2_info rcar_csi2_info_r8a7795es1 = { > + .hsfreqrange = hsfreqrange_m3w_h3es1, > +}; > + > +static const struct rcar_csi2_info rcar_csi2_info_r8a7796 = { > + .hsfreqrange = hsfreqrange_m3w_h3es1, > +}; > + > +static const struct of_device_id rcar_csi2_of_table[] = { > + { > + .compatible = "renesas,r8a7795-csi2", > + .data = &rcar_csi2_info_r8a7795, > + }, > + { > + .compatible = "renesas,r8a7796-csi2", > + .data = &rcar_csi2_info_r8a7796, > + }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, rcar_csi2_of_table); > + > +static const struct soc_device_attribute r8a7795es1[] = { > + { > + .soc_id = "r8a7795", .revision = "ES1.*", > + .data = &rcar_csi2_info_r8a7795es1, > + }, > + { /* sentinel */} > +}; > + > +static int rcar_csi2_probe(struct platform_device *pdev) > +{ > + const struct soc_device_attribute *attr; > + struct rcar_csi2 *priv; > + unsigned int i; > + int ret; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->info = of_device_get_match_data(&pdev->dev); > + > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ > + attr = soc_device_match(r8a7795es1); > + if (attr) > + priv->info = attr->data; > + > + priv->dev = &pdev->dev; > + > + mutex_init(&priv->lock); > + priv->stream_count = 0; > + > + ret = rcar_csi2_probe_resources(priv, pdev); > + if (ret) { > + dev_err(priv->dev, "Failed to get resources\n"); > + return ret; > + } > + > + platform_set_drvdata(pdev, priv); > + > + ret = rcar_csi2_parse_dt(priv); > + if (ret) > + return ret; > + > + priv->subdev.owner = THIS_MODULE; > + priv->subdev.dev = &pdev->dev; > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", > + KBUILD_MODNAME, dev_name(&pdev->dev)); > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > + > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > + priv->subdev.entity.ops = &rcar_csi2_entity_ops; > + > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, > + priv->pads); > + if (ret) > + goto error; > + > + pm_runtime_enable(&pdev->dev); > + > + ret = v4l2_async_register_subdev(&priv->subdev); > + if (ret < 0) > + goto error; > + > + dev_info(priv->dev, "%d lanes found\n", priv->lanes); > + > + return 0; > + > +error: > + v4l2_async_notifier_unregister(&priv->notifier); > + v4l2_async_notifier_cleanup(&priv->notifier); > + > + return ret; > +} > + > +static int rcar_csi2_remove(struct platform_device *pdev) > +{ > + struct rcar_csi2 *priv = platform_get_drvdata(pdev); > + > + v4l2_async_notifier_unregister(&priv->notifier); > + v4l2_async_notifier_cleanup(&priv->notifier); > + v4l2_async_unregister_subdev(&priv->subdev); > + > + pm_runtime_disable(&pdev->dev); > + > + return 0; > +} > + > +static struct platform_driver __refdata rcar_csi2_pdrv = { > + .remove = rcar_csi2_remove, > + .probe = rcar_csi2_probe, > + .driver = { > + .name = "rcar-csi2", > + .of_match_table = rcar_csi2_of_table, > + }, > +}; > + > +module_platform_driver(rcar_csi2_pdrv); > + > +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>"); > +MODULE_DESCRIPTION("Renesas R-Car MIPI CSI-2 receiver"); > +MODULE_LICENSE("GPL"); "GPL v2" ? No serious issues though. So when fixed/clarified feel free to append my Reviewed-by tag, if relevant at all. Thanks j > -- > 2.16.1 >
A few corrections, On Thu, Apr 05, 2018 at 11:10:01AM +0200, jacopo mondi wrote: > Hi Niklas, > thanks for the VIN and CSI-2 effort! > > On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote: > > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > > connected between the video sources and the video grabbers (VIN). > > > > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > > --- > > drivers/media/platform/rcar-vin/Kconfig | 12 + > > drivers/media/platform/rcar-vin/Makefile | 1 + > > drivers/media/platform/rcar-vin/rcar-csi2.c | 884 ++++++++++++++++++++++++++++ > > 3 files changed, 897 insertions(+) > > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c > > > > [snip] > > > + > > +static const struct rcar_csi2_format rcar_csi2_formats[] = { > > + { .code = MEDIA_BUS_FMT_RGB888_1X24, .datatype = 0x24, .bpp = 24 }, > > + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 }, > > + { .code = MEDIA_BUS_FMT_UYVY8_2X8, .datatype = 0x1e, .bpp = 16 }, > > + { .code = MEDIA_BUS_FMT_YUYV10_2X10, .datatype = 0x1e, .bpp = 16 }, > > Shouldn't YUYV10_2X10 format have 20 bits per pixel? > > > +}; > > + > > +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++) > > + if (rcar_csi2_formats[i].code == code) > > + return rcar_csi2_formats + i; > > + > > + return NULL; > > +} > > + > > +enum rcar_csi2_pads { > > + RCAR_CSI2_SINK, > > + RCAR_CSI2_SOURCE_VC0, > > + RCAR_CSI2_SOURCE_VC1, > > + RCAR_CSI2_SOURCE_VC2, > > + RCAR_CSI2_SOURCE_VC3, > > + NR_OF_RCAR_CSI2_PAD, > > +}; > > + > > +struct rcar_csi2_info { > > + const struct phypll_hsfreqrange *hsfreqrange; > > + unsigned int csi0clkfreqrange; > > + bool clear_ulps; > > + bool init_phtw; > > +}; > > + > > +struct rcar_csi2 { > > + struct device *dev; > > + void __iomem *base; > > + const struct rcar_csi2_info *info; > > + > > + struct v4l2_subdev subdev; > > + struct media_pad pads[NR_OF_RCAR_CSI2_PAD]; > > + > > + struct v4l2_async_notifier notifier; > > + struct v4l2_async_subdev asd; > > + struct v4l2_subdev *remote; > > + > > + struct v4l2_mbus_framefmt mf; > > + > > + struct mutex lock; > > + int stream_count; > > + > > + unsigned short lanes; > > + unsigned char lane_swap[4]; > > +}; > > + > > +static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > > +{ > > + return container_of(sd, struct rcar_csi2, subdev); > > +} > > + > > +static inline struct rcar_csi2 *notifier_to_csi2(struct v4l2_async_notifier *n) > > +{ > > + return container_of(n, struct rcar_csi2, notifier); > > +} > > + > > +static u32 rcar_csi2_read(struct rcar_csi2 *priv, unsigned int reg) > > +{ > > + return ioread32(priv->base + reg); > > +} > > + > > +static void rcar_csi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data) > > +{ > > + iowrite32(data, priv->base + reg); > > +} > > + > > +static void rcar_csi2_reset(struct rcar_csi2 *priv) > > +{ > > + rcar_csi2_write(priv, SRST_REG, SRST_SRST); > > + usleep_range(100, 150); > > + rcar_csi2_write(priv, SRST_REG, 0); > > +} > > + > > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv) > > +{ > > + int timeout; > > + > > + /* Wait for the clock and data lanes to enter LP-11 state. */ > > + for (timeout = 100; timeout > 0; timeout--) { > > + const u32 lane_mask = (1 << priv->lanes) - 1; > > + > > + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 && > > Nitpicking: > if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) && > > Don't you prefer to provide defines also for bit fields instead of > using magic values? In this case something like > PHCLM_REG_STOPSTATE_CLK would do. > > Also, from tables 25.[17-20] it seems to me that for H3 and V3 you > have to set INSTATE to an hardcoded value after having validated PHDLM. > Maybe it is not necessary, just pointing it out. > > > + (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > > + return 0; > > + > > + msleep(20); > > + } > > + > > + dev_err(priv->dev, "Timeout waiting for LP-11 state\n"); > > + > > + return -ETIMEDOUT; > > +} > > + > > +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp, > > + u32 *phypll) > > +{ > > + const struct phypll_hsfreqrange *hsfreq; > > + struct v4l2_subdev *source; > > + struct v4l2_ctrl *ctrl; > > + u64 mbps; > > + > > + if (!priv->remote) > > + return -ENODEV; > > + > > + source = priv->remote; > > + > > + /* Read the pixel rate control from remote */ > > + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); > > + if (!ctrl) { > > + dev_err(priv->dev, "no pixel rate control in subdev %s\n", > > + source->name); > > + return -EINVAL; > > + } > > + > > + /* Calculate the phypll */ > > + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > + do_div(mbps, priv->lanes * 1000000); > > + > > + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) > > + if (hsfreq->mbps >= mbps) > > + break; > > + > > + if (!hsfreq->mbps) { > > + dev_err(priv->dev, "Unsupported PHY speed (%llu Mbps)", mbps); > > + return -ERANGE; > > + } > > + > > + dev_dbg(priv->dev, "PHY HSFREQRANGE requested %llu got %u Mbps\n", mbps, > > + hsfreq->mbps); > > + > > + *phypll = PHYPLL_HSFREQRANGE(hsfreq->reg); > > + > > + return 0; > > +} > > + > > +static int rcar_csi2_start(struct rcar_csi2 *priv) > > +{ > > + const struct rcar_csi2_format *format; > > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; > > + unsigned int i; > > + int ret; > > + > > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > > + priv->mf.width, priv->mf.height, > > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > > + > > + /* Code is validated in set_fmt */ > > + format = rcar_csi2_code_to_fmt(priv->mf.code); > > + > > + /* > > + * Enable all Virtual Channels > > + * > > + * NOTE: It's not possible to get individual datatype for each > > + * source virtual channel. Once this is possible in V4L2 > > + * it should be used here. > > + */ > > + for (i = 0; i < 4; i++) { > > + u32 vcdt_part; > > + > > + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON | > > + VCDT_SEL_DT(format->datatype); > > + > > + /* Store in correct reg and offset */ > > + if (i < 2) > > + vcdt |= vcdt_part << ((i % 2) * 16); > > + else > > + vcdt2 |= vcdt_part << ((i % 2) * 16); > > + } > > + > > + switch (priv->lanes) { > > + case 1: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > > + break; > > + case 2: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + case 4: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > Even simpler this could be written as > > phycnt = PHYCNT_ENABLECLK | (1 << priv->lanes) - 1; Suggested by Geert already, sorry I missed that (and I'm probably missing () around (1 << priv->lanes) - 1 > > > + default: > > + return -EINVAL; > > Can this happen? You have validated priv->lanes already when parsing > DT > > > + } > > + > > + ret = rcar_csi2_calc_phypll(priv, format->bpp, &phypll); > > + if (ret) > > + return ret; > > + > > + /* Clear Ultra Low Power interrupt */ > > + if (priv->info->clear_ulps) > > + rcar_csi2_write(priv, INTSTATE_REG, > > + INTSTATE_INT_ULPS_START | > > + INTSTATE_INT_ULPS_END); > > + > > + /* Init */ > > + rcar_csi2_write(priv, TREF_REG, TREF_TREF); > > + rcar_csi2_reset(priv); > > + rcar_csi2_write(priv, PHTC_REG, 0); > > + > > + /* Configure */ > > + rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 | > > + FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN); > > On the FLD_FLD_NUM(2) mask. Why 2? > I read on the datasheet "the register must not be changed from default > value" and I read defaul to be 0x0000 > > Also, please consider a make as all other fields are enabled > unconditionally. s/make/mask I felt like I should have corrected this, otherwise the previous statement does not make sense :) Thanks j > > > + rcar_csi2_write(priv, VCDT_REG, vcdt); > > + rcar_csi2_write(priv, VCDT2_REG, vcdt2); > > + /* Lanes are zero indexed */ > > + rcar_csi2_write(priv, LSWAP_REG, > > + LSWAP_L0SEL(priv->lane_swap[0] - 1) | > > + LSWAP_L1SEL(priv->lane_swap[1] - 1) | > > + LSWAP_L2SEL(priv->lane_swap[2] - 1) | > > + LSWAP_L3SEL(priv->lane_swap[3] - 1)); > > EDIT: > (This comment is way too long for the real value it has, but since I > already wrote it, and my initial doubt clarified while I was writing, > resulting in a much less serious issues, I'm gonna keep it all anyway. > Sorry about this :) > > Why - 1 ? > Is this because it is assumed clock lane is in position 0? Is this > fixed by design? > > What I read in datasheet for LSWAP_REG is: > L[0-3]SEL 0 = Use PHY lane 0 > 1 = Use PHY lane 1 > 2 = Use PHY lane 2 > 3 = Use PHY lane 3 > > priv->lane_swap[i] is collected parsing 'data_lanes' property and > should reflect the actual physical lane value assigned to logical lane > numbers. If 'data_lanes' is, say <1 2> I expect > > priv->lane_swap[0] = 1; > priv->lane_swap[1] = 2; > priv->lane_swap[1] = 3; //assigned by your parsing routine > priv->lane_swap[1] = 4; //assigned by your parsing routine > > And I understand LSWAP counts instead from [0-3] so, ok, I get why you > subtract one. But now I wonder what happens if instead, lane position > is specified counting from 0 in DT. Ah, I see you refuse lane_swap > values < 1! So It should be assumed clock is by HW design on lane 0, > so wouldn't you need to mention in DT bindings that the HW has clock > lanes fixed in position 0 and the accepted values for the 'data_lanes' > property ranges in the [1-4] interval? > > > + > > + if (priv->info->init_phtw) { > > + /* > > + * This is for H3 ES2.0 > > + * > > + * NOTE: Additional logic is needed here when > > + * support for V3H and/or M3-N is added > > + */ > > + rcar_csi2_write(priv, PHTW_REG, 0x01cc01e2); > > + rcar_csi2_write(priv, PHTW_REG, 0x010101e3); > > + rcar_csi2_write(priv, PHTW_REG, 0x010101e4); > > + rcar_csi2_write(priv, PHTW_REG, 0x01100104); > > + rcar_csi2_write(priv, PHTW_REG, 0x01030100); > > + rcar_csi2_write(priv, PHTW_REG, 0x01800100); > > + } > > + > > + /* Start */ > > + rcar_csi2_write(priv, PHYPLL_REG, phypll); > > + > > + /* Set frequency range if we have it */ > > + if (priv->info->csi0clkfreqrange) > > + rcar_csi2_write(priv, CSI0CLKFCPR_REG, > > + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange)); > > + > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt); > > + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN | > > + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP); > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ); > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | > > + PHYCNT_RSTZ); > > Nit: from tables 25.[17-20] it seems to me you do not have to re-issue > PHYCNT_SHUTDOWNZ when writing PHYCNT_RSTZ to PHYCNT_REG. > > > + > > + return rcar_csi2_wait_phy_start(priv); > > +} > > + > > +static void rcar_csi2_stop(struct rcar_csi2 *priv) > > +{ > > + rcar_csi2_write(priv, PHYCNT_REG, 0); > > + > > + rcar_csi2_reset(priv); > > +} > > + > > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable) > > +{ > > + struct rcar_csi2 *priv = sd_to_csi2(sd); > > + struct v4l2_subdev *nextsd; > > + int ret = 0; > > + > > + mutex_lock(&priv->lock); > > + > > + if (!priv->remote) { > > + ret = -ENODEV; > > + goto out; > > + } > > Can this happen? > > The 'bind' callback sets priv->remote and it gets assigned back to > NULL only on 'unbind'. Wouldn't it be better to remove the link in the > media graph and let the system return an EPIPE before calling this? > > > + > > + nextsd = priv->remote; > > + > > + if (enable && priv->stream_count == 0) { > > + pm_runtime_get_sync(priv->dev); > > + > > + ret = rcar_csi2_start(priv); > > + if (ret) { > > + pm_runtime_put(priv->dev); > > + goto out; > > + } > > + > > + ret = v4l2_subdev_call(nextsd, video, s_stream, 1); > > + if (ret) { > > + rcar_csi2_stop(priv); > > + pm_runtime_put(priv->dev); > > + goto out; > > + } > > + } else if (!enable && priv->stream_count == 1) { > > + rcar_csi2_stop(priv); > > + v4l2_subdev_call(nextsd, video, s_stream, 0); > > + pm_runtime_put(priv->dev); > > + } > > + > > + priv->stream_count += enable ? 1 : -1; > > +out: > > + mutex_unlock(&priv->lock); > > + > > + return ret; > > +} > > + > > +static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *format) > > +{ > > + struct rcar_csi2 *priv = sd_to_csi2(sd); > > + struct v4l2_mbus_framefmt *framefmt; > > + > > + if (!rcar_csi2_code_to_fmt(format->format.code)) > > + return -EINVAL; > > + > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > + priv->mf = format->format; > > + } else { > > + framefmt = v4l2_subdev_get_try_format(sd, cfg, 0); > > + *framefmt = format->format; > > + } > > + > > + return 0; > > +} > > + > > +static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *format) > > +{ > > + struct rcar_csi2 *priv = sd_to_csi2(sd); > > + > > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) > > + format->format = priv->mf; > > + else > > + format->format = *v4l2_subdev_get_try_format(sd, cfg, 0); > > + > > + return 0; > > +} > > + > > +static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = { > > + .s_stream = rcar_csi2_s_stream, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = { > > + .set_fmt = rcar_csi2_set_pad_format, > > + .get_fmt = rcar_csi2_get_pad_format, > > +}; > > + > > +static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = { > > + .video = &rcar_csi2_video_ops, > > + .pad = &rcar_csi2_pad_ops, > > +}; > > + > > +/* ----------------------------------------------------------------------------- > > + * Async and registered of subdevices and links > > + */ > > + > > +static int rcar_csi2_notify_bound(struct v4l2_async_notifier *notifier, > > + struct v4l2_subdev *subdev, > > + struct v4l2_async_subdev *asd) > > +{ > > + struct rcar_csi2 *priv = notifier_to_csi2(notifier); > > + int pad; > > + > > + pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode, > > + MEDIA_PAD_FL_SOURCE); > > + if (pad < 0) { > > + dev_err(priv->dev, "Failed to find pad for %s\n", subdev->name); > > + return pad; > > + } > > + > > + priv->remote = subdev; > > + > > + dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); > > + > > + return media_create_pad_link(&subdev->entity, pad, > > + &priv->subdev.entity, 0, > > + MEDIA_LNK_FL_ENABLED | > > + MEDIA_LNK_FL_IMMUTABLE); > > +} > > + > > +static void rcar_csi2_notify_unbind(struct v4l2_async_notifier *notifier, > > + struct v4l2_subdev *subdev, > > + struct v4l2_async_subdev *asd) > > +{ > > + struct rcar_csi2 *priv = notifier_to_csi2(notifier); > > + > > + priv->remote = NULL; > > + > > + dev_dbg(priv->dev, "Unbind %s\n", subdev->name); > > +} > > + > > +static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = { > > + .bound = rcar_csi2_notify_bound, > > + .unbind = rcar_csi2_notify_unbind, > > +}; > > + > > +static int rcar_csi2_parse_v4l2(struct rcar_csi2 *priv, > > + struct v4l2_fwnode_endpoint *vep) > > +{ > > + unsigned int i; > > + > > + /* Only port 0 endpoint 0 is valid */ > > + if (vep->base.port || vep->base.id) > > + return -ENOTCONN; > > + > > + if (vep->bus_type != V4L2_MBUS_CSI2) { > > + dev_err(priv->dev, "Unsupported bus: 0x%x\n", vep->bus_type); > > + return -EINVAL; > > + } > > + > > + priv->lanes = vep->bus.mipi_csi2.num_data_lanes; > > + if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { > > Is this an HW limitation? Like the 'data_lanes' comment, if it is, > shouldn't you mention in bindings that the accepted lane numbers is > limited to the [1,2,4] values. > > > + dev_err(priv->dev, "Unsupported number of data-lanes: %u\n", > > + priv->lanes); > > + return -EINVAL; > > + } > > + > > + for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { > > + priv->lane_swap[i] = i < priv->lanes ? > > + vep->bus.mipi_csi2.data_lanes[i] : i; > > + > > + /* Check for valid lane number */ > > + if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) { > > + dev_err(priv->dev, "data-lanes must be in 1-4 range\n"); > > + return -EINVAL; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv) > > +{ > > + struct device_node *ep; > > + struct v4l2_fwnode_endpoint v4l2_ep; > > + int ret; > > + > > + ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0); > > + if (!ep) { > > + dev_err(priv->dev, "Not connected to subdevice\n"); > > + return -EINVAL; > > + } > > + > > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); > > + if (ret) { > > + dev_err(priv->dev, "Could not parse v4l2 endpoint\n"); > > + of_node_put(ep); > > + return -EINVAL; > > + } > > + > > + ret = rcar_csi2_parse_v4l2(priv, &v4l2_ep); > > + if (ret) { > > + of_node_put(ep); > > + return ret; > > + } > > + > > + priv->asd.match.fwnode = > > + fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep)); > > + priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > > + > > + of_node_put(ep); > > + > > + priv->notifier.subdevs = devm_kzalloc(priv->dev, > > + sizeof(*priv->notifier.subdevs), > > + GFP_KERNEL); > > + if (priv->notifier.subdevs == NULL) > > Nit: > you can use ! for NULL comparison. I think checkpatch --strict > complains for this. > > > + return -ENOMEM; > > + > > + priv->notifier.num_subdevs = 1; > > + priv->notifier.subdevs[0] = &priv->asd; > > + priv->notifier.ops = &rcar_csi2_notify_ops; > > + > > + dev_dbg(priv->dev, "Found '%pOF'\n", > > + to_of_node(priv->asd.match.fwnode)); > > + > > + return v4l2_async_subdev_notifier_register(&priv->subdev, > > + &priv->notifier); > > +} > > + > > +/* ----------------------------------------------------------------------------- > > + * Platform Device Driver > > + */ > > + > > +static const struct media_entity_operations rcar_csi2_entity_ops = { > > + .link_validate = v4l2_subdev_link_validate, > > +}; > > + > > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv, > > + struct platform_device *pdev) > > +{ > > + struct resource *res; > > + int irq; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + priv->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(priv->base)) > > + return PTR_ERR(priv->base); > > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) > > + return irq; > > + > > + return 0; > > +} > > + > > +static const struct rcar_csi2_info rcar_csi2_info_r8a7795 = { > > + .hsfreqrange = hsfreqrange_h3_v3h_m3n, > > + .clear_ulps = true, > > + .init_phtw = true, > > + .csi0clkfreqrange = 0x20, > > +}; > > + > > +static const struct rcar_csi2_info rcar_csi2_info_r8a7795es1 = { > > + .hsfreqrange = hsfreqrange_m3w_h3es1, > > +}; > > + > > +static const struct rcar_csi2_info rcar_csi2_info_r8a7796 = { > > + .hsfreqrange = hsfreqrange_m3w_h3es1, > > +}; > > + > > +static const struct of_device_id rcar_csi2_of_table[] = { > > + { > > + .compatible = "renesas,r8a7795-csi2", > > + .data = &rcar_csi2_info_r8a7795, > > + }, > > + { > > + .compatible = "renesas,r8a7796-csi2", > > + .data = &rcar_csi2_info_r8a7796, > > + }, > > + { /* sentinel */ }, > > +}; > > +MODULE_DEVICE_TABLE(of, rcar_csi2_of_table); > > + > > +static const struct soc_device_attribute r8a7795es1[] = { > > + { > > + .soc_id = "r8a7795", .revision = "ES1.*", > > + .data = &rcar_csi2_info_r8a7795es1, > > + }, > > + { /* sentinel */} > > +}; > > + > > +static int rcar_csi2_probe(struct platform_device *pdev) > > +{ > > + const struct soc_device_attribute *attr; > > + struct rcar_csi2 *priv; > > + unsigned int i; > > + int ret; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->info = of_device_get_match_data(&pdev->dev); > > + > > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ > > + attr = soc_device_match(r8a7795es1); > > + if (attr) > > + priv->info = attr->data; > > + > > + priv->dev = &pdev->dev; > > + > > + mutex_init(&priv->lock); > > + priv->stream_count = 0; > > + > > + ret = rcar_csi2_probe_resources(priv, pdev); > > + if (ret) { > > + dev_err(priv->dev, "Failed to get resources\n"); > > + return ret; > > + } > > + > > + platform_set_drvdata(pdev, priv); > > + > > + ret = rcar_csi2_parse_dt(priv); > > + if (ret) > > + return ret; > > + > > + priv->subdev.owner = THIS_MODULE; > > + priv->subdev.dev = &pdev->dev; > > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); > > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); > > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", > > + KBUILD_MODNAME, dev_name(&pdev->dev)); > > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > > + > > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > > + priv->subdev.entity.ops = &rcar_csi2_entity_ops; > > + > > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; > > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) > > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; > > + > > + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, > > + priv->pads); > > + if (ret) > > + goto error; > > + > > + pm_runtime_enable(&pdev->dev); > > + > > + ret = v4l2_async_register_subdev(&priv->subdev); > > + if (ret < 0) > > + goto error; > > + > > + dev_info(priv->dev, "%d lanes found\n", priv->lanes); > > + > > + return 0; > > + > > +error: > > + v4l2_async_notifier_unregister(&priv->notifier); > > + v4l2_async_notifier_cleanup(&priv->notifier); > > + > > + return ret; > > +} > > + > > +static int rcar_csi2_remove(struct platform_device *pdev) > > +{ > > + struct rcar_csi2 *priv = platform_get_drvdata(pdev); > > + > > + v4l2_async_notifier_unregister(&priv->notifier); > > + v4l2_async_notifier_cleanup(&priv->notifier); > > + v4l2_async_unregister_subdev(&priv->subdev); > > + > > + pm_runtime_disable(&pdev->dev); > > + > > + return 0; > > +} > > + > > +static struct platform_driver __refdata rcar_csi2_pdrv = { > > + .remove = rcar_csi2_remove, > > + .probe = rcar_csi2_probe, > > + .driver = { > > + .name = "rcar-csi2", > > + .of_match_table = rcar_csi2_of_table, > > + }, > > +}; > > + > > +module_platform_driver(rcar_csi2_pdrv); > > + > > +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>"); > > +MODULE_DESCRIPTION("Renesas R-Car MIPI CSI-2 receiver"); > > +MODULE_LICENSE("GPL"); > > "GPL v2" ? > > No serious issues though. So when fixed/clarified feel free to append my > Reviewed-by tag, if relevant at all. > > Thanks > j > > > -- > > 2.16.1 > >
Hi Kieran, Thanks for your feedback. On 2018-03-13 23:23:49 +0100, Kieran Bingham wrote: > Hi Niklas > > Thank you for this patch ... > I know it has been a lot of work getting this and the VIN together! > > On 13/02/18 00:01, Niklas Söderlund wrote: > > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver > > supports the R-Car Gen3 SoCs where separate CSI-2 hardware blocks are > > connected between the video sources and the video grabbers (VIN). > > > > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > > I don't think there's actually anything major in the below - so with any > comments addressed, or specifically ignored you can add my: > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Thanks, see bellow for answers to your questions. I have removed the questions Laurent already have provided a reply for, thanks for doing that! > > tag if you like. > > > > --- > > drivers/media/platform/rcar-vin/Kconfig | 12 + > > drivers/media/platform/rcar-vin/Makefile | 1 + > > drivers/media/platform/rcar-vin/rcar-csi2.c | 884 ++++++++++++++++++++++++++++ > > 3 files changed, 897 insertions(+) > > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c > > > > diff --git a/drivers/media/platform/rcar-vin/Kconfig b/drivers/media/platform/rcar-vin/Kconfig > > index af4c98b44d2e22cb..6875f30c1ae42631 100644 > > --- a/drivers/media/platform/rcar-vin/Kconfig > > +++ b/drivers/media/platform/rcar-vin/Kconfig > > @@ -1,3 +1,15 @@ > > +config VIDEO_RCAR_CSI2 > > + tristate "R-Car MIPI CSI-2 Receiver" > > + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF > > I think I recall recently seeing that depending upon OF is redundant and not > necessary (though I think that's minor in this instance) I can't seem to find anything to indicate this, but as you state if there is such a transition ongoing we can delete this later IMHO. $ find . -name Kconfig -exec grep OF {} \; | grep "depends on" | wc -l 622 But I'm happy to be proven wrong and remove it now as well. [snip] > > +static const struct rcar_csi2_format > > *rcar_csi2_code_to_fmt(unsigned int code) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++) > > + if (rcar_csi2_formats[i].code == code) > > + return rcar_csi2_formats + i; > > I would have written this as: > return &rcar_csi2_formats[i]; but I think your way is valid too :) This seems to be the popular opinion among reviewers so I will adopt this style for the next version :-) > > I have a nice 'for_each_entity_in_array' macro I keep meaning to post which > would be nice here too - but hey - for another day. Looking forward to it. [snip] > > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv) > > +{ > > + int timeout; > > + > > + /* Wait for the clock and data lanes to enter LP-11 state. */ > > + for (timeout = 100; timeout > 0; timeout--) { > > + const u32 lane_mask = (1 << priv->lanes) - 1; > > + > > + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 && > > The '1' is not very clear here.. Could this be: > > if ((rcar_csi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATE) && Great catch I have clearly missed this define. I will call it PHCLM_STOPSTATECLK since that it's what the latest datasheet I have calls it. Thanks for pointing this out. [snip] > > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable) > > +{ > > + struct rcar_csi2 *priv = sd_to_csi2(sd); > > + struct v4l2_subdev *nextsd; > > + int ret = 0; > > + > > + mutex_lock(&priv->lock); > > + > > + if (!priv->remote) { > > + ret = -ENODEV; > > + goto out; > > + } > > + > > + nextsd = priv->remote; > > + > > + if (enable && priv->stream_count == 0) { > > + pm_runtime_get_sync(priv->dev); > > + > > + ret = rcar_csi2_start(priv); > > + if (ret) { > > + pm_runtime_put(priv->dev); > > + goto out; > > + } > > + > > + ret = v4l2_subdev_call(nextsd, video, s_stream, 1); > > Would it be nicer to pass 'enable' through instead of the '1'? (of course > enable==1 here) > > > + if (ret) { > > + rcar_csi2_stop(priv); > > + pm_runtime_put(priv->dev); > > + goto out; > > + } > > + } else if (!enable && priv->stream_count == 1) { > > + rcar_csi2_stop(priv); > > + v4l2_subdev_call(nextsd, video, s_stream, 0); > > likewise here... > I agree with Laurent's comment here that it's much clearer to use 1/0 instead of 'enable' when it comes to read the code and will keep it as such :-) > > + pm_runtime_put(priv->dev); > > + } > > What's the 'nextsd' in this instance? We only call the s_stream on nextsd if it > is the first one to be started, or the last one to be stopped... The 'nextsd' here is the next subdevice towards the video source, and it's a CSI-2 transmitter. > > I can understand this logic for calling rcar_csi2_stop/rcar_csi2_start ... but > shouldn't we always call : > v4l2_subdev_call(nextsd, video, s_stream, enable); ? It might seem logical to do so yes, in my first try to add multiplexed stream support to v4l2 I even attempted to move s_stream to pad ops structure and extend it with a stream number argument :-) But after discussing it and toying with different solution this turned out to be a bad idea. The CSI-2 transmitter and CSI-2 receiver needs to know about all links when they are started for the first time. So the first on in turns on everything and the last one out turns off the lights make sens here. The user shall only enable the links it wish to use and they shall all be started at the same time. Then we need this logic since we still need to start streaming from the different VIN instances individually and the CSI-2 receiver is the logical sync point to handle this as it has all the knowledge of the links it is involved in. > > in GMSL context, I guess 'nextsd' is the MAX9286? Yes. > At which point - it would be called for start/stop, priv->stream_count times, > and would also have to provide it's own call balancing... Yes and what could it do different for each call even if it implemented call balancing? It have no information about which of the enabled multiplexed streams are started. The logic here is a s_stream() shall start all enabled streams, if a user don't want a stream he shall not enabled it in the first place. As all enabled links will be subject to format validation in the media pipeline I think this will sort it self out ;-P > > Call me crazy and ignore me here if you like :-) I will not call you crazy, but I will ignore this comment :-) [snip] > > + > > +/* ----------------------------------------------------------------------------- > > + * Async and registered of subdevices and links > > "Async handling and registration of subdevices and links" ? > > or does of mean OF here ? No of means of, your English is much better then mine and I will update it for next version, thanks. [snip] > > + > > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ > > /* > * r8a7795 ES1.x behaves differently than the ES2.0+ but doesn't have it's own > * compatible string > */ > Ditto. [snip]
Hi Laurent, Thanks for your feedback. On 2018-04-04 18:25:23 +0300, Laurent Pinchart wrote: [snip] > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > > > b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644 > > > index 0000000000000000..c0c2a763151bc928 > > > --- /dev/null > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > @@ -0,0 +1,884 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > Do you intend making it GPL-2.0+ or did you mean GPL-2.0 ? Wops I intended to make it GPL-2.0 thanks for catching this.
Hi Maxime, Thanks for your feedback. On 2018-03-29 13:30:39 +0200, Maxime Ripard wrote: > Hi Niklas, > > On Tue, Feb 13, 2018 at 12:01:32AM +0100, Niklas Söderlund wrote: > > + switch (priv->lanes) { > > + case 1: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > > + break; > > + case 2: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + case 4: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + default: > > + return -EINVAL; > > + } > > I guess you could have a simpler construct here using this: > > phycnt = PHYCNT_ENABLECLK; > > switch (priv->lanes) { > case 4: > phycnt |= PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2; > case 2: > phycnt |= PHYCNT_ENABLE_1; > case 1: > phycnt |= PHYCNT_ENABLE_0; > break; > > default: > return -EINVAL; > } > > But that's really up to you. Thanks for the suggestion and sparking of the discussion, I think I will go with Geert at.al approach of: phycnt = PHYCNT_ENABLECLK; phycnt |= (1 << priv->lanes) - 1; > > > +static int rcar_csi2_probe(struct platform_device *pdev) > > +{ > > + const struct soc_device_attribute *attr; > > + struct rcar_csi2 *priv; > > + unsigned int i; > > + int ret; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->info = of_device_get_match_data(&pdev->dev); > > + > > + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ > > + attr = soc_device_match(r8a7795es1); > > + if (attr) > > + priv->info = attr->data; > > + > > + priv->dev = &pdev->dev; > > + > > + mutex_init(&priv->lock); > > + priv->stream_count = 0; > > + > > + ret = rcar_csi2_probe_resources(priv, pdev); > > + if (ret) { > > + dev_err(priv->dev, "Failed to get resources\n"); > > + return ret; > > + } > > + > > + platform_set_drvdata(pdev, priv); > > + > > + ret = rcar_csi2_parse_dt(priv); > > + if (ret) > > + return ret; > > + > > + priv->subdev.owner = THIS_MODULE; > > + priv->subdev.dev = &pdev->dev; > > + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); > > + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); > > + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", > > + KBUILD_MODNAME, dev_name(&pdev->dev)); > > + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > > + > > + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; > > + priv->subdev.entity.ops = &rcar_csi2_entity_ops; > > + > > + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; > > + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) > > + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; > > + > > + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, > > + priv->pads); > > + if (ret) > > + goto error; > > + > > + pm_runtime_enable(&pdev->dev); > > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the > device uninitialised at probe, and pm_runtime_get_sync will not > initialise it either if CONFIG_PM is not enabled. I guess you could > call your runtime_resume function unconditionally, and mark the device > as active in runtime_pm using pm_runtime_set_active. Yes CONFIG_PM is selected by ARCH_RENESAS. Thanks for letting me know about this in any case I was not aware of this behavior in the case CONFIG_PM where not enabled. > > Looks good otherwise, once fixed (and if relevant): > Reviewed-by: Maxime Ripard <maxime.ripard@bootlin.com> Thanks!
Hi Geert and Laurent, Thanks for the feedback. On 2018-04-05 11:26:45 +0300, Laurent Pinchart wrote: [snip] > > Alternatively, you could check for a valid number of lanes, and use > > knowledge about the internal lane bits: > > > > phycnt = PHYCNT_ENABLECLK; > > phycnt |= (1 << priv->lanes) - 1; > > If Niklas is fine with that, I like it too. I'm fine what that thanks for the suggestion Geert!
Hi Sakari, Thanks for your feedback. On 2018-04-04 23:13:57 +0300, Sakari Ailus wrote: [snip] > > > + pm_runtime_enable(&pdev->dev); > > > > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the > > device uninitialised at probe, and pm_runtime_get_sync will not > > initialise it either if CONFIG_PM is not enabled. I guess you could > > call your runtime_resume function unconditionally, and mark the device > > as active in runtime_pm using pm_runtime_set_active. > > There doesn't seem to be any runtime_resume function. Was there supposed > to be one? No there is not suppose to be one. > > Assuming runtime PM would actually do something here, you might add > pm_runtime_idle() to power the device off after probing. > > I guess pm_runtime_set_active() should precede pm_runtime_enable(). The CSI-2 is in the always on power domain so the calls to pm_runtime_get_sync() and pm_runtime_put() are there in the s_stream() callback to enable and disable the module clock. I'm no expert on PM but in my testing the pm_ calls in this driver seems to be correct. 1. In probe I call pm_runtime_enable(). And rudimentary tests shows the clock is off (but I might miss something) as I wish it to be until stream on time. 2. In s_stream() I call pm_runtime_get_sync() before writing any register when starting a stream. And likewise I call pm_runtime_put() when stopping and I no longer need to write to a register. 3. In remove() I call pm_runtime_disable(). Am I missing something obvious here?
Hi Laurent, Thanks for your feedback. I have addressed all your comment's but one for the next version. Please indicate if you are fine with this and I can still keep your review tag :-) On 2018-04-04 18:15:16 +0300, Laurent Pinchart wrote: [snip] > > +static int rcar_csi2_start(struct rcar_csi2 *priv) > > +{ > > + const struct rcar_csi2_format *format; > > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; > > + unsigned int i; > > + int ret; > > + > > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > > + priv->mf.width, priv->mf.height, > > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > > + > > + /* Code is validated in set_fmt */ > > + format = rcar_csi2_code_to_fmt(priv->mf.code); > > You could store the format pointer iin the rcar_csi2 structure to avoid > looking it up here. I could do that, but then I would duplicate the storage of the code between the two cached values priv->mf and priv-><cached pointer to rcar_csi2>. I could find that acceptable but if you don't strongly disagree I would prefer to keep the current way of looking it up here :-) [snip] > > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv, > > + struct platform_device *pdev) > > +{ > > + struct resource *res; > > + int irq; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + priv->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(priv->base)) > > + return PTR_ERR(priv->base); > > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) > > + return irq; > > You don't seem to use the IRQ. Is this meant to catch invalid DT that don't > specify an IRQ, to make sure we'll always have one available when we'll need > to later ? Yes, as you deducted this is currently only to catch invalid DT. In the DT documentation I list it as a mandatory property. I think there might be potential use-case to at some point add interrupt support of for the some of the error interrupts which can be enabled, specially now when we seen similar patches for VIN floating around. > > + > > + return 0; > > + > > +error: > > + v4l2_async_notifier_unregister(&priv->notifier); > > + v4l2_async_notifier_cleanup(&priv->notifier); > > + > > + return ret; > > +} > > [snip] > > With these small issues fixed and Kieran's and Maxime's comments addressed as > you see fit, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks, I will hold of adding it until you indicate if you are OK with the one comment I'm not fully agreeing with you on.
Hi Jacopo, Thanks for your feedback. Comments I have snipped out from this reply are addressed, thanks for bringing them to my attention! On 2018-04-05 11:10:01 +0200, Jacopo Mondi wrote: [snip] > > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv) > > +{ > > + int timeout; > > + > > + /* Wait for the clock and data lanes to enter LP-11 state. */ > > + for (timeout = 100; timeout > 0; timeout--) { > > + const u32 lane_mask = (1 << priv->lanes) - 1; > > + > > + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 && > > Nitpicking: > if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) && > > Don't you prefer to provide defines also for bit fields instead of > using magic values? In this case something like > PHCLM_REG_STOPSTATE_CLK would do. Thanks addressed per your and Kieran's suggestion. > > Also, from tables 25.[17-20] it seems to me that for H3 and V3 you > have to set INSTATE to an hardcoded value after having validated PHDLM. > Maybe it is not necessary, just pointing it out. I assume you mean Figures 25.[17-20] and not Tables as the last table in chapter 25 is Table 25.15 and the register in question is INTSTATE :-) And to clarify this is documented for H3 which this driver supports and V3H and M3-N which this driver dose not yet support. And the constant you are to set it to is ULPS_START | UPLS_END. This is a good catch as this was introduced in a later version of the datasheet and the current code where the ULPS_START | UPLS_END is set before confirming LP-11 have kept on working. Check the priv->info->clear_ulps usage in rcar_csi2_start(). I do think it's better to follow the flow-chart in the new datasheet so I will move this to the end of rcar_csi2_start() to reflect that (provided that the end result still works :-) Thanks for pointing this out! [snip] > > +static int rcar_csi2_start(struct rcar_csi2 *priv) > > +{ > > + const struct rcar_csi2_format *format; > > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; > > + unsigned int i; > > + int ret; > > + > > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > > + priv->mf.width, priv->mf.height, > > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > > + > > + /* Code is validated in set_fmt */ > > + format = rcar_csi2_code_to_fmt(priv->mf.code); > > + > > + /* > > + * Enable all Virtual Channels > > + * > > + * NOTE: It's not possible to get individual datatype for each > > + * source virtual channel. Once this is possible in V4L2 > > + * it should be used here. > > + */ > > + for (i = 0; i < 4; i++) { > > + u32 vcdt_part; > > + > > + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON | > > + VCDT_SEL_DT(format->datatype); > > + > > + /* Store in correct reg and offset */ > > + if (i < 2) > > + vcdt |= vcdt_part << ((i % 2) * 16); > > + else > > + vcdt2 |= vcdt_part << ((i % 2) * 16); > > + } > > + > > + switch (priv->lanes) { > > + case 1: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > > + break; > > + case 2: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > + case 4: > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > + break; > > Even simpler this could be written as > > phycnt = PHYCNT_ENABLECLK | (1 << priv->lanes) - 1; Fixed per your and Geert's suggestion. > > > + default: > > + return -EINVAL; > > Can this happen? You have validated priv->lanes already when parsing > DT This can't happen but I like to have a catch all in any case, but since I took yours and Geert's suggestion above this issue goes away :-) > > > + } > > + > > + ret = rcar_csi2_calc_phypll(priv, format->bpp, &phypll); > > + if (ret) > > + return ret; > > + > > + /* Clear Ultra Low Power interrupt */ > > + if (priv->info->clear_ulps) > > + rcar_csi2_write(priv, INTSTATE_REG, > > + INTSTATE_INT_ULPS_START | > > + INTSTATE_INT_ULPS_END); > > + > > + /* Init */ > > + rcar_csi2_write(priv, TREF_REG, TREF_TREF); > > + rcar_csi2_reset(priv); > > + rcar_csi2_write(priv, PHTC_REG, 0); > > + > > + /* Configure */ > > + rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 | > > + FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN); > > On the FLD_FLD_NUM(2) mask. Why 2? > I read on the datasheet "the register must not be changed from default > value" and I read defaul to be 0x0000 This is based on feedback from Renesas. The register is not properly documented. I'm working on improving it but for now I would like to keep it as FLD_FLD_NUM(2) and make a neater and documented fix in a follow up commit. In short it controls which field is captured first (ODD/EVEN). > > Also, please consider a make as all other fields are enabled > unconditionally. No I don't like the idea of a mask here, I feel it better to be explicit. > > > + rcar_csi2_write(priv, VCDT_REG, vcdt); > > + rcar_csi2_write(priv, VCDT2_REG, vcdt2); > > + /* Lanes are zero indexed */ > > + rcar_csi2_write(priv, LSWAP_REG, > > + LSWAP_L0SEL(priv->lane_swap[0] - 1) | > > + LSWAP_L1SEL(priv->lane_swap[1] - 1) | > > + LSWAP_L2SEL(priv->lane_swap[2] - 1) | > > + LSWAP_L3SEL(priv->lane_swap[3] - 1)); > > EDIT: > (This comment is way too long for the real value it has, but since I > already wrote it, and my initial doubt clarified while I was writing, > resulting in a much less serious issues, I'm gonna keep it all anyway. > Sorry about this :) > > Why - 1 ? > Is this because it is assumed clock lane is in position 0? Is this > fixed by design? > > What I read in datasheet for LSWAP_REG is: > L[0-3]SEL 0 = Use PHY lane 0 > 1 = Use PHY lane 1 > 2 = Use PHY lane 2 > 3 = Use PHY lane 3 > > priv->lane_swap[i] is collected parsing 'data_lanes' property and > should reflect the actual physical lane value assigned to logical lane > numbers. If 'data_lanes' is, say <1 2> I expect > > priv->lane_swap[0] = 1; > priv->lane_swap[1] = 2; > priv->lane_swap[1] = 3; //assigned by your parsing routine > priv->lane_swap[1] = 4; //assigned by your parsing routine > > And I understand LSWAP counts instead from [0-3] so, ok, I get why you > subtract one. But now I wonder what happens if instead, lane position > is specified counting from 0 in DT. Ah, I see you refuse lane_swap > values < 1! So It should be assumed clock is by HW design on lane 0, > so wouldn't you need to mention in DT bindings that the HW has clock > lanes fixed in position 0 and the accepted values for the 'data_lanes' > property ranges in the [1-4] interval? My understanding of video-interfaces.txt which is reference in the DT documentation is that the clock is always 0 and the data lanes are numbers 1-4 for CSI-2. > > > + > > + if (priv->info->init_phtw) { > > + /* > > + * This is for H3 ES2.0 > > + * > > + * NOTE: Additional logic is needed here when > > + * support for V3H and/or M3-N is added > > + */ > > + rcar_csi2_write(priv, PHTW_REG, 0x01cc01e2); > > + rcar_csi2_write(priv, PHTW_REG, 0x010101e3); > > + rcar_csi2_write(priv, PHTW_REG, 0x010101e4); > > + rcar_csi2_write(priv, PHTW_REG, 0x01100104); > > + rcar_csi2_write(priv, PHTW_REG, 0x01030100); > > + rcar_csi2_write(priv, PHTW_REG, 0x01800100); > > + } > > + > > + /* Start */ > > + rcar_csi2_write(priv, PHYPLL_REG, phypll); > > + > > + /* Set frequency range if we have it */ > > + if (priv->info->csi0clkfreqrange) > > + rcar_csi2_write(priv, CSI0CLKFCPR_REG, > > + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange)); > > + > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt); > > + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN | > > + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP); > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ); > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | > > + PHYCNT_RSTZ); > > Nit: from tables 25.[17-20] it seems to me you do not have to re-issue > PHYCNT_SHUTDOWNZ when writing PHYCNT_RSTZ to PHYCNT_REG. You are correct, I miss read '.... Here, the ENABLE_0 to ENABLE_3 and ENABLECLK values set above should be retained' as all previous PHYCNT bits should be retained not just the ones explicitly listed. I will give this a test and if it still works I will remove it for the next version. > > > + > > + return rcar_csi2_wait_phy_start(priv); > > +} > > + > > +static void rcar_csi2_stop(struct rcar_csi2 *priv) > > +{ > > + rcar_csi2_write(priv, PHYCNT_REG, 0); > > + > > + rcar_csi2_reset(priv); > > +} > > + > > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable) > > +{ > > + struct rcar_csi2 *priv = sd_to_csi2(sd); > > + struct v4l2_subdev *nextsd; > > + int ret = 0; > > + > > + mutex_lock(&priv->lock); > > + > > + if (!priv->remote) { > > + ret = -ENODEV; > > + goto out; > > + } > > Can this happen? Sure, the video source is unbound and as you note bellow the unbind callback sets priv->remote = NULL. Or even simpler the driver for the video source is not loaded at all so the rcar-csi2 drivers bound callback is never called. > > The 'bind' callback sets priv->remote and it gets assigned back to > NULL only on 'unbind'. Wouldn't it be better to remove the link in the > media graph and let the system return an EPIPE before calling this? That would be very complex as some driver in the pipeline (VIN?) would have to parse the full graph in DT and somehow with subdevice support helpers to traverse the DT port to MC pad mapping using helpers that do not exist yet (and are hard to use if the remote subdevice driver is not loaded) to build a full view of the pipeline and return -EPIPE if not all entities are present in the media graph :-) [snip] > > +static int rcar_csi2_parse_v4l2(struct rcar_csi2 *priv, > > + struct v4l2_fwnode_endpoint *vep) > > +{ > > + unsigned int i; > > + > > + /* Only port 0 endpoint 0 is valid */ > > + if (vep->base.port || vep->base.id) > > + return -ENOTCONN; > > + > > + if (vep->bus_type != V4L2_MBUS_CSI2) { > > + dev_err(priv->dev, "Unsupported bus: 0x%x\n", vep->bus_type); > > + return -EINVAL; > > + } > > + > > + priv->lanes = vep->bus.mipi_csi2.num_data_lanes; > > + if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { > > Is this an HW limitation? Like the 'data_lanes' comment, if it is, > shouldn't you mention in bindings that the accepted lane numbers is > limited to the [1,2,4] values. Yes, see chapter 25.4.8 Lane Setting. [snip] > > No serious issues though. So when fixed/clarified feel free to append my > Reviewed-by tag, if relevant at all. Thanks! I feel I managed to address all of your review comments but would still like your confirmation for me to add your tag. Thanks again for the review!
Hi Niklas, On Sun, Apr 15, 2018 at 10:47:37PM +0200, Niklas Söderlund wrote: > Hi Sakari, > > Thanks for your feedback. > > On 2018-04-04 23:13:57 +0300, Sakari Ailus wrote: > > [snip] > > > > > + pm_runtime_enable(&pdev->dev); > > > > > > Is CONFIG_PM mandatory on Renesas SoCs? If not, you end up with the > > > device uninitialised at probe, and pm_runtime_get_sync will not > > > initialise it either if CONFIG_PM is not enabled. I guess you could > > > call your runtime_resume function unconditionally, and mark the device > > > as active in runtime_pm using pm_runtime_set_active. > > > > There doesn't seem to be any runtime_resume function. Was there supposed > > to be one? > > No there is not suppose to be one. Ok. > > > > > Assuming runtime PM would actually do something here, you might add > > pm_runtime_idle() to power the device off after probing. > > > > I guess pm_runtime_set_active() should precede pm_runtime_enable(). > > The CSI-2 is in the always on power domain so the calls to > pm_runtime_get_sync() and pm_runtime_put() are there in the s_stream() > callback to enable and disable the module clock. I'm no expert on PM but > in my testing the pm_ calls in this driver seems to be correct. > > 1. In probe I call pm_runtime_enable(). And rudimentary tests shows the > clock is off (but I might miss something) as I wish it to be until > stream on time. > 2. In s_stream() I call pm_runtime_get_sync() before writing any > register when starting a stream. And likewise I call pm_runtime_put() > when stopping and I no longer need to write to a register. > 3. In remove() I call pm_runtime_disable(). > > Am I missing something obvious here? Looking at the code again, it seems fine in this respect.
Hi Niklas, On Mon, Apr 16, 2018 at 01:16:35AM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your feedback. > > Comments I have snipped out from this reply are addressed, thanks for > bringing them to my attention! > > On 2018-04-05 11:10:01 +0200, Jacopo Mondi wrote: > > [snip] > > > > +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv) > > > +{ > > > + int timeout; > > > + > > > + /* Wait for the clock and data lanes to enter LP-11 state. */ > > > + for (timeout = 100; timeout > 0; timeout--) { > > > + const u32 lane_mask = (1 << priv->lanes) - 1; > > > + > > > + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 && > > > > Nitpicking: > > if ((rcar_csi2_read(priv, PHCLM_REG) & 0x01) && > > > > Don't you prefer to provide defines also for bit fields instead of > > using magic values? In this case something like > > PHCLM_REG_STOPSTATE_CLK would do. > > Thanks addressed per your and Kieran's suggestion. > > > > > Also, from tables 25.[17-20] it seems to me that for H3 and V3 you > > have to set INSTATE to an hardcoded value after having validated PHDLM. > > Maybe it is not necessary, just pointing it out. > > I assume you mean Figures 25.[17-20] and not Tables as the last table in > chapter 25 is Table 25.15 and the register in question is INTSTATE :-) > And to clarify this is documented for H3 which this driver supports and > V3H and M3-N which this driver dose not yet support. And the constant > you are to set it to is ULPS_START | UPLS_END. Yes, Figures, not Tables, sorry about this. > > This is a good catch as this was introduced in a later version of the > datasheet and the current code where the ULPS_START | UPLS_END is set > before confirming LP-11 have kept on working. Check the > priv->info->clear_ulps usage in rcar_csi2_start(). I do think it's > better to follow the flow-chart in the new datasheet so I will move this > to the end of rcar_csi2_start() to reflect that (provided that the end > result still works :-) Thanks for pointing this out! > I see... Actually, I don't see M3-N in the manual version I'm looking at. Anyway, I just hope this per-soc specificities are limited. > [snip] > > > > +static int rcar_csi2_start(struct rcar_csi2 *priv) > > > +{ > > > + const struct rcar_csi2_format *format; > > > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; > > > + unsigned int i; > > > + int ret; > > > + > > > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > > > + priv->mf.width, priv->mf.height, > > > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > > > + > > > + /* Code is validated in set_fmt */ > > > + format = rcar_csi2_code_to_fmt(priv->mf.code); > > > + > > > + /* > > > + * Enable all Virtual Channels > > > + * > > > + * NOTE: It's not possible to get individual datatype for each > > > + * source virtual channel. Once this is possible in V4L2 > > > + * it should be used here. > > > + */ > > > + for (i = 0; i < 4; i++) { > > > + u32 vcdt_part; > > > + > > > + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON | > > > + VCDT_SEL_DT(format->datatype); > > > + > > > + /* Store in correct reg and offset */ > > > + if (i < 2) > > > + vcdt |= vcdt_part << ((i % 2) * 16); > > > + else > > > + vcdt2 |= vcdt_part << ((i % 2) * 16); > > > + } > > > + > > > + switch (priv->lanes) { > > > + case 1: > > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; > > > + break; > > > + case 2: > > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > > + break; > > > + case 4: > > > + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | > > > + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; > > > + break; > > > > Even simpler this could be written as > > > > phycnt = PHYCNT_ENABLECLK | (1 << priv->lanes) - 1; > > Fixed per your and Geert's suggestion. > > > > > > + default: > > > + return -EINVAL; > > > > Can this happen? You have validated priv->lanes already when parsing > > DT > > This can't happen but I like to have a catch all in any case, but since > I took yours and Geert's suggestion above this issue goes away :-) > Does gcc complains about the missing default case? > > > > > + } > > > + > > > + ret = rcar_csi2_calc_phypll(priv, format->bpp, &phypll); > > > + if (ret) > > > + return ret; > > > + > > > + /* Clear Ultra Low Power interrupt */ > > > + if (priv->info->clear_ulps) > > > + rcar_csi2_write(priv, INTSTATE_REG, > > > + INTSTATE_INT_ULPS_START | > > > + INTSTATE_INT_ULPS_END); > > > + > > > + /* Init */ > > > + rcar_csi2_write(priv, TREF_REG, TREF_TREF); > > > + rcar_csi2_reset(priv); > > > + rcar_csi2_write(priv, PHTC_REG, 0); > > > + > > > + /* Configure */ > > > + rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 | > > > + FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN); > > > > On the FLD_FLD_NUM(2) mask. Why 2? > > I read on the datasheet "the register must not be changed from default > > value" and I read defaul to be 0x0000 > > This is based on feedback from Renesas. The register is not properly > documented. I'm working on improving it but for now I would like to keep > it as FLD_FLD_NUM(2) and make a neater and documented fix in a follow up > commit. In short it controls which field is captured first (ODD/EVEN). > Would you mind adding a small comment there? > > > > Also, please consider a make as all other fields are enabled > > unconditionally. > > No I don't like the idea of a mask here, I feel it better to be > explicit. > Up to you :) > > > > > + rcar_csi2_write(priv, VCDT_REG, vcdt); > > > + rcar_csi2_write(priv, VCDT2_REG, vcdt2); > > > + /* Lanes are zero indexed */ > > > + rcar_csi2_write(priv, LSWAP_REG, > > > + LSWAP_L0SEL(priv->lane_swap[0] - 1) | > > > + LSWAP_L1SEL(priv->lane_swap[1] - 1) | > > > + LSWAP_L2SEL(priv->lane_swap[2] - 1) | > > > + LSWAP_L3SEL(priv->lane_swap[3] - 1)); > > > > EDIT: > > (This comment is way too long for the real value it has, but since I > > already wrote it, and my initial doubt clarified while I was writing, > > resulting in a much less serious issues, I'm gonna keep it all anyway. > > Sorry about this :) > > > > Why - 1 ? > > Is this because it is assumed clock lane is in position 0? Is this > > fixed by design? > > > > What I read in datasheet for LSWAP_REG is: > > L[0-3]SEL 0 = Use PHY lane 0 > > 1 = Use PHY lane 1 > > 2 = Use PHY lane 2 > > 3 = Use PHY lane 3 > > > > priv->lane_swap[i] is collected parsing 'data_lanes' property and > > should reflect the actual physical lane value assigned to logical lane > > numbers. If 'data_lanes' is, say <1 2> I expect > > > > priv->lane_swap[0] = 1; > > priv->lane_swap[1] = 2; > > priv->lane_swap[1] = 3; //assigned by your parsing routine > > priv->lane_swap[1] = 4; //assigned by your parsing routine > > > > And I understand LSWAP counts instead from [0-3] so, ok, I get why you > > subtract one. But now I wonder what happens if instead, lane position > > is specified counting from 0 in DT. Ah, I see you refuse lane_swap > > values < 1! So It should be assumed clock is by HW design on lane 0, > > so wouldn't you need to mention in DT bindings that the HW has clock > > lanes fixed in position 0 and the accepted values for the 'data_lanes' > > property ranges in the [1-4] interval? > > My understanding of video-interfaces.txt which is reference in the DT > documentation is that the clock is always 0 and the data lanes are > numbers 1-4 for CSI-2. > Quoting the DT bindings: - data-lanes: an array of physical data lane indexes. Position of an entry determines the logical lane number, while the value of an entry indicates physical lane, e.g. for 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;", assuming the clock lane is on hardware lane 0. If the hardware does not support lane reordering, monotonically incremented values shall be used from 0 or 1 onwards, depending on whether or not there is also a clock lane. This property is valid for serial busses only (e.g. MIPI CSI-2). So yes, you're correct. I got confused by the "depending on whether or not there is also a clock lane.". I assume this phrase is to accommodate C-PHYs that embed the clock lane in each data 'trios'. > > > > > + > > > + if (priv->info->init_phtw) { > > > + /* > > > + * This is for H3 ES2.0 > > > + * > > > + * NOTE: Additional logic is needed here when > > > + * support for V3H and/or M3-N is added > > > + */ > > > + rcar_csi2_write(priv, PHTW_REG, 0x01cc01e2); > > > + rcar_csi2_write(priv, PHTW_REG, 0x010101e3); > > > + rcar_csi2_write(priv, PHTW_REG, 0x010101e4); > > > + rcar_csi2_write(priv, PHTW_REG, 0x01100104); > > > + rcar_csi2_write(priv, PHTW_REG, 0x01030100); > > > + rcar_csi2_write(priv, PHTW_REG, 0x01800100); > > > + } > > > + > > > + /* Start */ > > > + rcar_csi2_write(priv, PHYPLL_REG, phypll); > > > + > > > + /* Set frequency range if we have it */ > > > + if (priv->info->csi0clkfreqrange) > > > + rcar_csi2_write(priv, CSI0CLKFCPR_REG, > > > + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange)); > > > + > > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt); > > > + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN | > > > + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP); > > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ); > > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | > > > + PHYCNT_RSTZ); > > > > Nit: from tables 25.[17-20] it seems to me you do not have to re-issue > > PHYCNT_SHUTDOWNZ when writing PHYCNT_RSTZ to PHYCNT_REG. > > You are correct, I miss read '.... Here, the ENABLE_0 to ENABLE_3 and > ENABLECLK values set above should be retained' as all previous PHYCNT > bits should be retained not just the ones explicitly listed. I will give > this a test and if it still works I will remove it for the next version. > > > > > > + > > > + return rcar_csi2_wait_phy_start(priv); > > > +} > > > + > > > +static void rcar_csi2_stop(struct rcar_csi2 *priv) > > > +{ > > > + rcar_csi2_write(priv, PHYCNT_REG, 0); > > > + > > > + rcar_csi2_reset(priv); > > > +} > > > + > > > +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable) > > > +{ > > > + struct rcar_csi2 *priv = sd_to_csi2(sd); > > > + struct v4l2_subdev *nextsd; > > > + int ret = 0; > > > + > > > + mutex_lock(&priv->lock); > > > + > > > + if (!priv->remote) { > > > + ret = -ENODEV; > > > + goto out; > > > + } > > > > Can this happen? > > Sure, the video source is unbound and as you note bellow the unbind > callback sets priv->remote = NULL. Or even simpler the driver for the > video source is not loaded at all so the rcar-csi2 drivers bound > callback is never called. My reasoning was that if the driver for the video source is not loaded, so you won't have any 'bound()' callback, you won't have any video device created to call s_stream() on. Am I biased by how the R-Car VIN works? > > > > > The 'bind' callback sets priv->remote and it gets assigned back to > > NULL only on 'unbind'. Wouldn't it be better to remove the link in the > > media graph and let the system return an EPIPE before calling this? > > That would be very complex as some driver in the pipeline (VIN?) would > have to parse the full graph in DT and somehow with subdevice support > helpers to traverse the DT port to MC pad mapping using helpers that do > not exist yet (and are hard to use if the remote subdevice driver is not > loaded) to build a full view of the pipeline and return -EPIPE if not > all entities are present in the media graph :-) Ack. I thought the pipeline validation was performed during the pipeline traversal by the media controller framework, and not that is was responsability of the video capture driver (eg VIN) > > [snip] > > > > +static int rcar_csi2_parse_v4l2(struct rcar_csi2 *priv, > > > + struct v4l2_fwnode_endpoint *vep) > > > +{ > > > + unsigned int i; > > > + > > > + /* Only port 0 endpoint 0 is valid */ > > > + if (vep->base.port || vep->base.id) > > > + return -ENOTCONN; > > > + > > > + if (vep->bus_type != V4L2_MBUS_CSI2) { > > > + dev_err(priv->dev, "Unsupported bus: 0x%x\n", vep->bus_type); > > > + return -EINVAL; > > > + } > > > + > > > + priv->lanes = vep->bus.mipi_csi2.num_data_lanes; > > > + if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { > > > > Is this an HW limitation? Like the 'data_lanes' comment, if it is, > > shouldn't you mention in bindings that the accepted lane numbers is > > limited to the [1,2,4] values. > > Yes, see chapter 25.4.8 Lane Setting. What about mentioning it in DT? Not an expert, but this is an HW limitation, so it can fit there. > > [snip] > > > > > No serious issues though. So when fixed/clarified feel free to append my > > Reviewed-by tag, if relevant at all. > > Thanks! I feel I managed to address all of your review comments but > would still like your confirmation for me to add your tag. Thanks again > for the review! Since you have to send a new version anyhow, I'll reply with my tag to that one. Thanks j > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 2018-04-16 01:16:35 +0200, Niklas Söderlund wrote: [snip] > > > + > > > + /* Set frequency range if we have it */ > > > + if (priv->info->csi0clkfreqrange) > > > + rcar_csi2_write(priv, CSI0CLKFCPR_REG, > > > + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange)); > > > + > > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt); > > > + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN | > > > + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP); > > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ); > > > + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | > > > + PHYCNT_RSTZ); > > > > Nit: from tables 25.[17-20] it seems to me you do not have to re-issue > > PHYCNT_SHUTDOWNZ when writing PHYCNT_RSTZ to PHYCNT_REG. > > You are correct, I miss read '.... Here, the ENABLE_0 to ENABLE_3 and > ENABLECLK values set above should be retained' as all previous PHYCNT > bits should be retained not just the ones explicitly listed. I will give > this a test and if it still works I will remove it for the next version. This change breaks capture and LP-11 is never detected. So I will continue to retain the PHYCNT_SHUTDOWNZ here.
Hi Niklas, On Monday, 16 April 2018 00:26:06 EEST Niklas Söderlund wrote: > Hi Laurent, > > Thanks for your feedback. > > I have addressed all your comment's but one for the next version. Please > indicate if you are fine with this and I can still keep your review tag > > :-) > > On 2018-04-04 18:15:16 +0300, Laurent Pinchart wrote: > > [snip] > > > > +static int rcar_csi2_start(struct rcar_csi2 *priv) > > > +{ > > > + const struct rcar_csi2_format *format; > > > + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; > > > + unsigned int i; > > > + int ret; > > > + > > > + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", > > > + priv->mf.width, priv->mf.height, > > > + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); > > > + > > > + /* Code is validated in set_fmt */ > > > + format = rcar_csi2_code_to_fmt(priv->mf.code); > > > > You could store the format pointer iin the rcar_csi2 structure to avoid > > looking it up here. > > I could do that, but then I would duplicate the storage of the code > between the two cached values priv->mf and priv-><cached pointer to > rcar_csi2>. I could find that acceptable but if you don't strongly > disagree I would prefer to keep the current way of looking it up here > :-) I'm OK with that, even if I think that duplicating the code would be a very small price to pay for not having to look up the format information structure. > [snip] > > > > +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv, > > > + struct platform_device *pdev) > > > +{ > > > + struct resource *res; > > > + int irq; > > > + > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + priv->base = devm_ioremap_resource(&pdev->dev, res); > > > + if (IS_ERR(priv->base)) > > > + return PTR_ERR(priv->base); > > > + > > > + irq = platform_get_irq(pdev, 0); > > > + if (irq < 0) > > > + return irq; > > > > You don't seem to use the IRQ. Is this meant to catch invalid DT that > > don't > > specify an IRQ, to make sure we'll always have one available when we'll > > need to later ? > > Yes, as you deducted this is currently only to catch invalid DT. In the > DT documentation I list it as a mandatory property. I think there might > be potential use-case to at some point add interrupt support of for the > some of the error interrupts which can be enabled, specially now when we > seen similar patches for VIN floating around. > > > > + > > > + return 0; > > > + > > > +error: > > > + v4l2_async_notifier_unregister(&priv->notifier); > > > + v4l2_async_notifier_cleanup(&priv->notifier); > > > + > > > + return ret; > > > +} > > > > [snip] > > > > With these small issues fixed and Kieran's and Maxime's comments addressed > > as you see fit, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks, I will hold of adding it until you indicate if you are OK with > the one comment I'm not fully agreeing with you on.
diff --git a/drivers/media/platform/rcar-vin/Kconfig b/drivers/media/platform/rcar-vin/Kconfig index af4c98b44d2e22cb..6875f30c1ae42631 100644 --- a/drivers/media/platform/rcar-vin/Kconfig +++ b/drivers/media/platform/rcar-vin/Kconfig @@ -1,3 +1,15 @@ +config VIDEO_RCAR_CSI2 + tristate "R-Car MIPI CSI-2 Receiver" + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF + depends on ARCH_RENESAS || COMPILE_TEST + select V4L2_FWNODE + ---help--- + Support for Renesas R-Car MIPI CSI-2 receiver. + Supports R-Car Gen3 SoCs. + + To compile this driver as a module, choose M here: the + module will be called rcar-csi2. + config VIDEO_RCAR_VIN tristate "R-Car Video Input (VIN) Driver" depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF && HAS_DMA && MEDIA_CONTROLLER diff --git a/drivers/media/platform/rcar-vin/Makefile b/drivers/media/platform/rcar-vin/Makefile index 48c5632c21dc060b..5ab803d3e7c1aa57 100644 --- a/drivers/media/platform/rcar-vin/Makefile +++ b/drivers/media/platform/rcar-vin/Makefile @@ -1,3 +1,4 @@ rcar-vin-objs = rcar-core.o rcar-dma.o rcar-v4l2.o +obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin.o diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644 index 0000000000000000..c0c2a763151bc928 --- /dev/null +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c @@ -0,0 +1,884 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Driver for Renesas R-Car MIPI CSI-2 Receiver + * + * Copyright (C) 2018 Renesas Electronics Corp. + */ + +#include <linux/delay.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_graph.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/sys_soc.h> + +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> +#include <media/v4l2-fwnode.h> +#include <media/v4l2-mc.h> +#include <media/v4l2-subdev.h> + +/* Register offsets and bits */ + +/* Control Timing Select */ +#define TREF_REG 0x00 +#define TREF_TREF BIT(0) + +/* Software Reset */ +#define SRST_REG 0x04 +#define SRST_SRST BIT(0) + +/* PHY Operation Control */ +#define PHYCNT_REG 0x08 +#define PHYCNT_SHUTDOWNZ BIT(17) +#define PHYCNT_RSTZ BIT(16) +#define PHYCNT_ENABLECLK BIT(4) +#define PHYCNT_ENABLE_3 BIT(3) +#define PHYCNT_ENABLE_2 BIT(2) +#define PHYCNT_ENABLE_1 BIT(1) +#define PHYCNT_ENABLE_0 BIT(0) + +/* Checksum Control */ +#define CHKSUM_REG 0x0c +#define CHKSUM_ECC_EN BIT(1) +#define CHKSUM_CRC_EN BIT(0) + +/* + * Channel Data Type Select + * VCDT[0-15]: Channel 1 VCDT[16-31]: Channel 2 + * VCDT2[0-15]: Channel 3 VCDT2[16-31]: Channel 4 + */ +#define VCDT_REG 0x10 +#define VCDT2_REG 0x14 +#define VCDT_VCDTN_EN BIT(15) +#define VCDT_SEL_VC(n) (((n) & 0x3) << 8) +#define VCDT_SEL_DTN_ON BIT(6) +#define VCDT_SEL_DT(n) (((n) & 0x3f) << 0) + +/* Frame Data Type Select */ +#define FRDT_REG 0x18 + +/* Field Detection Control */ +#define FLD_REG 0x1c +#define FLD_FLD_NUM(n) (((n) & 0xff) << 16) +#define FLD_FLD_EN4 BIT(3) +#define FLD_FLD_EN3 BIT(2) +#define FLD_FLD_EN2 BIT(1) +#define FLD_FLD_EN BIT(0) + +/* Automatic Standby Control */ +#define ASTBY_REG 0x20 + +/* Long Data Type Setting 0 */ +#define LNGDT0_REG 0x28 + +/* Long Data Type Setting 1 */ +#define LNGDT1_REG 0x2c + +/* Interrupt Enable */ +#define INTEN_REG 0x30 + +/* Interrupt Source Mask */ +#define INTCLOSE_REG 0x34 + +/* Interrupt Status Monitor */ +#define INTSTATE_REG 0x38 +#define INTSTATE_INT_ULPS_START BIT(7) +#define INTSTATE_INT_ULPS_END BIT(6) + +/* Interrupt Error Status Monitor */ +#define INTERRSTATE_REG 0x3c + +/* Short Packet Data */ +#define SHPDAT_REG 0x40 + +/* Short Packet Count */ +#define SHPCNT_REG 0x44 + +/* LINK Operation Control */ +#define LINKCNT_REG 0x48 +#define LINKCNT_MONITOR_EN BIT(31) +#define LINKCNT_REG_MONI_PACT_EN BIT(25) +#define LINKCNT_ICLK_NONSTOP BIT(24) + +/* Lane Swap */ +#define LSWAP_REG 0x4c +#define LSWAP_L3SEL(n) (((n) & 0x3) << 6) +#define LSWAP_L2SEL(n) (((n) & 0x3) << 4) +#define LSWAP_L1SEL(n) (((n) & 0x3) << 2) +#define LSWAP_L0SEL(n) (((n) & 0x3) << 0) + +/* PHY Test Interface Write Register */ +#define PHTW_REG 0x50 + +/* PHY Test Interface Clear */ +#define PHTC_REG 0x58 +#define PHTC_TESTCLR BIT(0) + +/* PHY Frequency Control */ +#define PHYPLL_REG 0x68 +#define PHYPLL_HSFREQRANGE(n) ((n) << 16) + +struct phypll_hsfreqrange { + u16 mbps; + u16 reg; +}; + +static const struct phypll_hsfreqrange hsfreqrange_h3_v3h_m3n[] = { + { .mbps = 80, .reg = 0x00 }, + { .mbps = 90, .reg = 0x10 }, + { .mbps = 100, .reg = 0x20 }, + { .mbps = 110, .reg = 0x30 }, + { .mbps = 120, .reg = 0x01 }, + { .mbps = 130, .reg = 0x11 }, + { .mbps = 140, .reg = 0x21 }, + { .mbps = 150, .reg = 0x31 }, + { .mbps = 160, .reg = 0x02 }, + { .mbps = 170, .reg = 0x12 }, + { .mbps = 180, .reg = 0x22 }, + { .mbps = 190, .reg = 0x32 }, + { .mbps = 205, .reg = 0x03 }, + { .mbps = 220, .reg = 0x13 }, + { .mbps = 235, .reg = 0x23 }, + { .mbps = 250, .reg = 0x33 }, + { .mbps = 275, .reg = 0x04 }, + { .mbps = 300, .reg = 0x14 }, + { .mbps = 325, .reg = 0x25 }, + { .mbps = 350, .reg = 0x35 }, + { .mbps = 400, .reg = 0x05 }, + { .mbps = 450, .reg = 0x26 }, + { .mbps = 500, .reg = 0x36 }, + { .mbps = 550, .reg = 0x37 }, + { .mbps = 600, .reg = 0x07 }, + { .mbps = 650, .reg = 0x18 }, + { .mbps = 700, .reg = 0x28 }, + { .mbps = 750, .reg = 0x39 }, + { .mbps = 800, .reg = 0x09 }, + { .mbps = 850, .reg = 0x19 }, + { .mbps = 900, .reg = 0x29 }, + { .mbps = 950, .reg = 0x3a }, + { .mbps = 1000, .reg = 0x0a }, + { .mbps = 1050, .reg = 0x1a }, + { .mbps = 1100, .reg = 0x2a }, + { .mbps = 1150, .reg = 0x3b }, + { .mbps = 1200, .reg = 0x0b }, + { .mbps = 1250, .reg = 0x1b }, + { .mbps = 1300, .reg = 0x2b }, + { .mbps = 1350, .reg = 0x3c }, + { .mbps = 1400, .reg = 0x0c }, + { .mbps = 1450, .reg = 0x1c }, + { .mbps = 1500, .reg = 0x2c }, + /* guard */ + { .mbps = 0, .reg = 0x00 }, +}; + +static const struct phypll_hsfreqrange hsfreqrange_m3w_h3es1[] = { + { .mbps = 80, .reg = 0x00 }, + { .mbps = 90, .reg = 0x10 }, + { .mbps = 100, .reg = 0x20 }, + { .mbps = 110, .reg = 0x30 }, + { .mbps = 120, .reg = 0x01 }, + { .mbps = 130, .reg = 0x11 }, + { .mbps = 140, .reg = 0x21 }, + { .mbps = 150, .reg = 0x31 }, + { .mbps = 160, .reg = 0x02 }, + { .mbps = 170, .reg = 0x12 }, + { .mbps = 180, .reg = 0x22 }, + { .mbps = 190, .reg = 0x32 }, + { .mbps = 205, .reg = 0x03 }, + { .mbps = 220, .reg = 0x13 }, + { .mbps = 235, .reg = 0x23 }, + { .mbps = 250, .reg = 0x33 }, + { .mbps = 275, .reg = 0x04 }, + { .mbps = 300, .reg = 0x14 }, + { .mbps = 325, .reg = 0x05 }, + { .mbps = 350, .reg = 0x15 }, + { .mbps = 400, .reg = 0x25 }, + { .mbps = 450, .reg = 0x06 }, + { .mbps = 500, .reg = 0x16 }, + { .mbps = 550, .reg = 0x07 }, + { .mbps = 600, .reg = 0x17 }, + { .mbps = 650, .reg = 0x08 }, + { .mbps = 700, .reg = 0x18 }, + { .mbps = 750, .reg = 0x09 }, + { .mbps = 800, .reg = 0x19 }, + { .mbps = 850, .reg = 0x29 }, + { .mbps = 900, .reg = 0x39 }, + { .mbps = 950, .reg = 0x0A }, + { .mbps = 1000, .reg = 0x1A }, + { .mbps = 1050, .reg = 0x2A }, + { .mbps = 1100, .reg = 0x3A }, + { .mbps = 1150, .reg = 0x0B }, + { .mbps = 1200, .reg = 0x1B }, + { .mbps = 1250, .reg = 0x2B }, + { .mbps = 1300, .reg = 0x3B }, + { .mbps = 1350, .reg = 0x0C }, + { .mbps = 1400, .reg = 0x1C }, + { .mbps = 1450, .reg = 0x2C }, + { .mbps = 1500, .reg = 0x3C }, + /* guard */ + { .mbps = 0, .reg = 0x00 }, +}; + +/* PHY ESC Error Monitor */ +#define PHEERM_REG 0x74 + +/* PHY Clock Lane Monitor */ +#define PHCLM_REG 0x78 + +/* PHY Data Lane Monitor */ +#define PHDLM_REG 0x7c + +/* CSI0CLK Frequency Configuration Preset Register */ +#define CSI0CLKFCPR_REG 0x260 +#define CSI0CLKFREQRANGE(n) ((n & 0x3f) << 16) + +struct rcar_csi2_format { + u32 code; + unsigned int datatype; + unsigned int bpp; +}; + +static const struct rcar_csi2_format rcar_csi2_formats[] = { + { .code = MEDIA_BUS_FMT_RGB888_1X24, .datatype = 0x24, .bpp = 24 }, + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = 0x1e, .bpp = 16 }, + { .code = MEDIA_BUS_FMT_UYVY8_2X8, .datatype = 0x1e, .bpp = 16 }, + { .code = MEDIA_BUS_FMT_YUYV10_2X10, .datatype = 0x1e, .bpp = 16 }, +}; + +static const struct rcar_csi2_format *rcar_csi2_code_to_fmt(unsigned int code) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++) + if (rcar_csi2_formats[i].code == code) + return rcar_csi2_formats + i; + + return NULL; +} + +enum rcar_csi2_pads { + RCAR_CSI2_SINK, + RCAR_CSI2_SOURCE_VC0, + RCAR_CSI2_SOURCE_VC1, + RCAR_CSI2_SOURCE_VC2, + RCAR_CSI2_SOURCE_VC3, + NR_OF_RCAR_CSI2_PAD, +}; + +struct rcar_csi2_info { + const struct phypll_hsfreqrange *hsfreqrange; + unsigned int csi0clkfreqrange; + bool clear_ulps; + bool init_phtw; +}; + +struct rcar_csi2 { + struct device *dev; + void __iomem *base; + const struct rcar_csi2_info *info; + + struct v4l2_subdev subdev; + struct media_pad pads[NR_OF_RCAR_CSI2_PAD]; + + struct v4l2_async_notifier notifier; + struct v4l2_async_subdev asd; + struct v4l2_subdev *remote; + + struct v4l2_mbus_framefmt mf; + + struct mutex lock; + int stream_count; + + unsigned short lanes; + unsigned char lane_swap[4]; +}; + +static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) +{ + return container_of(sd, struct rcar_csi2, subdev); +} + +static inline struct rcar_csi2 *notifier_to_csi2(struct v4l2_async_notifier *n) +{ + return container_of(n, struct rcar_csi2, notifier); +} + +static u32 rcar_csi2_read(struct rcar_csi2 *priv, unsigned int reg) +{ + return ioread32(priv->base + reg); +} + +static void rcar_csi2_write(struct rcar_csi2 *priv, unsigned int reg, u32 data) +{ + iowrite32(data, priv->base + reg); +} + +static void rcar_csi2_reset(struct rcar_csi2 *priv) +{ + rcar_csi2_write(priv, SRST_REG, SRST_SRST); + usleep_range(100, 150); + rcar_csi2_write(priv, SRST_REG, 0); +} + +static int rcar_csi2_wait_phy_start(struct rcar_csi2 *priv) +{ + int timeout; + + /* Wait for the clock and data lanes to enter LP-11 state. */ + for (timeout = 100; timeout > 0; timeout--) { + const u32 lane_mask = (1 << priv->lanes) - 1; + + if ((rcar_csi2_read(priv, PHCLM_REG) & 1) == 1 && + (rcar_csi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) + return 0; + + msleep(20); + } + + dev_err(priv->dev, "Timeout waiting for LP-11 state\n"); + + return -ETIMEDOUT; +} + +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv, unsigned int bpp, + u32 *phypll) +{ + const struct phypll_hsfreqrange *hsfreq; + struct v4l2_subdev *source; + struct v4l2_ctrl *ctrl; + u64 mbps; + + if (!priv->remote) + return -ENODEV; + + source = priv->remote; + + /* Read the pixel rate control from remote */ + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE); + if (!ctrl) { + dev_err(priv->dev, "no pixel rate control in subdev %s\n", + source->name); + return -EINVAL; + } + + /* Calculate the phypll */ + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; + do_div(mbps, priv->lanes * 1000000); + + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) + if (hsfreq->mbps >= mbps) + break; + + if (!hsfreq->mbps) { + dev_err(priv->dev, "Unsupported PHY speed (%llu Mbps)", mbps); + return -ERANGE; + } + + dev_dbg(priv->dev, "PHY HSFREQRANGE requested %llu got %u Mbps\n", mbps, + hsfreq->mbps); + + *phypll = PHYPLL_HSFREQRANGE(hsfreq->reg); + + return 0; +} + +static int rcar_csi2_start(struct rcar_csi2 *priv) +{ + const struct rcar_csi2_format *format; + u32 phycnt, phypll, vcdt = 0, vcdt2 = 0; + unsigned int i; + int ret; + + dev_dbg(priv->dev, "Input size (%ux%u%c)\n", + priv->mf.width, priv->mf.height, + priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i'); + + /* Code is validated in set_fmt */ + format = rcar_csi2_code_to_fmt(priv->mf.code); + + /* + * Enable all Virtual Channels + * + * NOTE: It's not possible to get individual datatype for each + * source virtual channel. Once this is possible in V4L2 + * it should be used here. + */ + for (i = 0; i < 4; i++) { + u32 vcdt_part; + + vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON | + VCDT_SEL_DT(format->datatype); + + /* Store in correct reg and offset */ + if (i < 2) + vcdt |= vcdt_part << ((i % 2) * 16); + else + vcdt2 |= vcdt_part << ((i % 2) * 16); + } + + switch (priv->lanes) { + case 1: + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_0; + break; + case 2: + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; + break; + case 4: + phycnt = PHYCNT_ENABLECLK | PHYCNT_ENABLE_3 | PHYCNT_ENABLE_2 | + PHYCNT_ENABLE_1 | PHYCNT_ENABLE_0; + break; + default: + return -EINVAL; + } + + ret = rcar_csi2_calc_phypll(priv, format->bpp, &phypll); + if (ret) + return ret; + + /* Clear Ultra Low Power interrupt */ + if (priv->info->clear_ulps) + rcar_csi2_write(priv, INTSTATE_REG, + INTSTATE_INT_ULPS_START | + INTSTATE_INT_ULPS_END); + + /* Init */ + rcar_csi2_write(priv, TREF_REG, TREF_TREF); + rcar_csi2_reset(priv); + rcar_csi2_write(priv, PHTC_REG, 0); + + /* Configure */ + rcar_csi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 | + FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN); + rcar_csi2_write(priv, VCDT_REG, vcdt); + rcar_csi2_write(priv, VCDT2_REG, vcdt2); + /* Lanes are zero indexed */ + rcar_csi2_write(priv, LSWAP_REG, + LSWAP_L0SEL(priv->lane_swap[0] - 1) | + LSWAP_L1SEL(priv->lane_swap[1] - 1) | + LSWAP_L2SEL(priv->lane_swap[2] - 1) | + LSWAP_L3SEL(priv->lane_swap[3] - 1)); + + if (priv->info->init_phtw) { + /* + * This is for H3 ES2.0 + * + * NOTE: Additional logic is needed here when + * support for V3H and/or M3-N is added + */ + rcar_csi2_write(priv, PHTW_REG, 0x01cc01e2); + rcar_csi2_write(priv, PHTW_REG, 0x010101e3); + rcar_csi2_write(priv, PHTW_REG, 0x010101e4); + rcar_csi2_write(priv, PHTW_REG, 0x01100104); + rcar_csi2_write(priv, PHTW_REG, 0x01030100); + rcar_csi2_write(priv, PHTW_REG, 0x01800100); + } + + /* Start */ + rcar_csi2_write(priv, PHYPLL_REG, phypll); + + /* Set frequency range if we have it */ + if (priv->info->csi0clkfreqrange) + rcar_csi2_write(priv, CSI0CLKFCPR_REG, + CSI0CLKFREQRANGE(priv->info->csi0clkfreqrange)); + + rcar_csi2_write(priv, PHYCNT_REG, phycnt); + rcar_csi2_write(priv, LINKCNT_REG, LINKCNT_MONITOR_EN | + LINKCNT_REG_MONI_PACT_EN | LINKCNT_ICLK_NONSTOP); + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ); + rcar_csi2_write(priv, PHYCNT_REG, phycnt | PHYCNT_SHUTDOWNZ | + PHYCNT_RSTZ); + + return rcar_csi2_wait_phy_start(priv); +} + +static void rcar_csi2_stop(struct rcar_csi2 *priv) +{ + rcar_csi2_write(priv, PHYCNT_REG, 0); + + rcar_csi2_reset(priv); +} + +static int rcar_csi2_s_stream(struct v4l2_subdev *sd, int enable) +{ + struct rcar_csi2 *priv = sd_to_csi2(sd); + struct v4l2_subdev *nextsd; + int ret = 0; + + mutex_lock(&priv->lock); + + if (!priv->remote) { + ret = -ENODEV; + goto out; + } + + nextsd = priv->remote; + + if (enable && priv->stream_count == 0) { + pm_runtime_get_sync(priv->dev); + + ret = rcar_csi2_start(priv); + if (ret) { + pm_runtime_put(priv->dev); + goto out; + } + + ret = v4l2_subdev_call(nextsd, video, s_stream, 1); + if (ret) { + rcar_csi2_stop(priv); + pm_runtime_put(priv->dev); + goto out; + } + } else if (!enable && priv->stream_count == 1) { + rcar_csi2_stop(priv); + v4l2_subdev_call(nextsd, video, s_stream, 0); + pm_runtime_put(priv->dev); + } + + priv->stream_count += enable ? 1 : -1; +out: + mutex_unlock(&priv->lock); + + return ret; +} + +static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *format) +{ + struct rcar_csi2 *priv = sd_to_csi2(sd); + struct v4l2_mbus_framefmt *framefmt; + + if (!rcar_csi2_code_to_fmt(format->format.code)) + return -EINVAL; + + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { + priv->mf = format->format; + } else { + framefmt = v4l2_subdev_get_try_format(sd, cfg, 0); + *framefmt = format->format; + } + + return 0; +} + +static int rcar_csi2_get_pad_format(struct v4l2_subdev *sd, + struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_format *format) +{ + struct rcar_csi2 *priv = sd_to_csi2(sd); + + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) + format->format = priv->mf; + else + format->format = *v4l2_subdev_get_try_format(sd, cfg, 0); + + return 0; +} + +static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = { + .s_stream = rcar_csi2_s_stream, +}; + +static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = { + .set_fmt = rcar_csi2_set_pad_format, + .get_fmt = rcar_csi2_get_pad_format, +}; + +static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = { + .video = &rcar_csi2_video_ops, + .pad = &rcar_csi2_pad_ops, +}; + +/* ----------------------------------------------------------------------------- + * Async and registered of subdevices and links + */ + +static int rcar_csi2_notify_bound(struct v4l2_async_notifier *notifier, + struct v4l2_subdev *subdev, + struct v4l2_async_subdev *asd) +{ + struct rcar_csi2 *priv = notifier_to_csi2(notifier); + int pad; + + pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode, + MEDIA_PAD_FL_SOURCE); + if (pad < 0) { + dev_err(priv->dev, "Failed to find pad for %s\n", subdev->name); + return pad; + } + + priv->remote = subdev; + + dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad); + + return media_create_pad_link(&subdev->entity, pad, + &priv->subdev.entity, 0, + MEDIA_LNK_FL_ENABLED | + MEDIA_LNK_FL_IMMUTABLE); +} + +static void rcar_csi2_notify_unbind(struct v4l2_async_notifier *notifier, + struct v4l2_subdev *subdev, + struct v4l2_async_subdev *asd) +{ + struct rcar_csi2 *priv = notifier_to_csi2(notifier); + + priv->remote = NULL; + + dev_dbg(priv->dev, "Unbind %s\n", subdev->name); +} + +static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = { + .bound = rcar_csi2_notify_bound, + .unbind = rcar_csi2_notify_unbind, +}; + +static int rcar_csi2_parse_v4l2(struct rcar_csi2 *priv, + struct v4l2_fwnode_endpoint *vep) +{ + unsigned int i; + + /* Only port 0 endpoint 0 is valid */ + if (vep->base.port || vep->base.id) + return -ENOTCONN; + + if (vep->bus_type != V4L2_MBUS_CSI2) { + dev_err(priv->dev, "Unsupported bus: 0x%x\n", vep->bus_type); + return -EINVAL; + } + + priv->lanes = vep->bus.mipi_csi2.num_data_lanes; + if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { + dev_err(priv->dev, "Unsupported number of data-lanes: %u\n", + priv->lanes); + return -EINVAL; + } + + for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { + priv->lane_swap[i] = i < priv->lanes ? + vep->bus.mipi_csi2.data_lanes[i] : i; + + /* Check for valid lane number */ + if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) { + dev_err(priv->dev, "data-lanes must be in 1-4 range\n"); + return -EINVAL; + } + } + + return 0; +} + +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv) +{ + struct device_node *ep; + struct v4l2_fwnode_endpoint v4l2_ep; + int ret; + + ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0); + if (!ep) { + dev_err(priv->dev, "Not connected to subdevice\n"); + return -EINVAL; + } + + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); + if (ret) { + dev_err(priv->dev, "Could not parse v4l2 endpoint\n"); + of_node_put(ep); + return -EINVAL; + } + + ret = rcar_csi2_parse_v4l2(priv, &v4l2_ep); + if (ret) { + of_node_put(ep); + return ret; + } + + priv->asd.match.fwnode = + fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep)); + priv->asd.match_type = V4L2_ASYNC_MATCH_FWNODE; + + of_node_put(ep); + + priv->notifier.subdevs = devm_kzalloc(priv->dev, + sizeof(*priv->notifier.subdevs), + GFP_KERNEL); + if (priv->notifier.subdevs == NULL) + return -ENOMEM; + + priv->notifier.num_subdevs = 1; + priv->notifier.subdevs[0] = &priv->asd; + priv->notifier.ops = &rcar_csi2_notify_ops; + + dev_dbg(priv->dev, "Found '%pOF'\n", + to_of_node(priv->asd.match.fwnode)); + + return v4l2_async_subdev_notifier_register(&priv->subdev, + &priv->notifier); +} + +/* ----------------------------------------------------------------------------- + * Platform Device Driver + */ + +static const struct media_entity_operations rcar_csi2_entity_ops = { + .link_validate = v4l2_subdev_link_validate, +}; + +static int rcar_csi2_probe_resources(struct rcar_csi2 *priv, + struct platform_device *pdev) +{ + struct resource *res; + int irq; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + priv->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(priv->base)) + return PTR_ERR(priv->base); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + return 0; +} + +static const struct rcar_csi2_info rcar_csi2_info_r8a7795 = { + .hsfreqrange = hsfreqrange_h3_v3h_m3n, + .clear_ulps = true, + .init_phtw = true, + .csi0clkfreqrange = 0x20, +}; + +static const struct rcar_csi2_info rcar_csi2_info_r8a7795es1 = { + .hsfreqrange = hsfreqrange_m3w_h3es1, +}; + +static const struct rcar_csi2_info rcar_csi2_info_r8a7796 = { + .hsfreqrange = hsfreqrange_m3w_h3es1, +}; + +static const struct of_device_id rcar_csi2_of_table[] = { + { + .compatible = "renesas,r8a7795-csi2", + .data = &rcar_csi2_info_r8a7795, + }, + { + .compatible = "renesas,r8a7796-csi2", + .data = &rcar_csi2_info_r8a7796, + }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, rcar_csi2_of_table); + +static const struct soc_device_attribute r8a7795es1[] = { + { + .soc_id = "r8a7795", .revision = "ES1.*", + .data = &rcar_csi2_info_r8a7795es1, + }, + { /* sentinel */} +}; + +static int rcar_csi2_probe(struct platform_device *pdev) +{ + const struct soc_device_attribute *attr; + struct rcar_csi2 *priv; + unsigned int i; + int ret; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->info = of_device_get_match_data(&pdev->dev); + + /* r8a7795 ES1.x behaves different then ES2.0+ but no own compat */ + attr = soc_device_match(r8a7795es1); + if (attr) + priv->info = attr->data; + + priv->dev = &pdev->dev; + + mutex_init(&priv->lock); + priv->stream_count = 0; + + ret = rcar_csi2_probe_resources(priv, pdev); + if (ret) { + dev_err(priv->dev, "Failed to get resources\n"); + return ret; + } + + platform_set_drvdata(pdev, priv); + + ret = rcar_csi2_parse_dt(priv); + if (ret) + return ret; + + priv->subdev.owner = THIS_MODULE; + priv->subdev.dev = &pdev->dev; + v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops); + v4l2_set_subdevdata(&priv->subdev, &pdev->dev); + snprintf(priv->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s %s", + KBUILD_MODNAME, dev_name(&pdev->dev)); + priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; + + priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER; + priv->subdev.entity.ops = &rcar_csi2_entity_ops; + + priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK; + for (i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++) + priv->pads[i].flags = MEDIA_PAD_FL_SOURCE; + + ret = media_entity_pads_init(&priv->subdev.entity, NR_OF_RCAR_CSI2_PAD, + priv->pads); + if (ret) + goto error; + + pm_runtime_enable(&pdev->dev); + + ret = v4l2_async_register_subdev(&priv->subdev); + if (ret < 0) + goto error; + + dev_info(priv->dev, "%d lanes found\n", priv->lanes); + + return 0; + +error: + v4l2_async_notifier_unregister(&priv->notifier); + v4l2_async_notifier_cleanup(&priv->notifier); + + return ret; +} + +static int rcar_csi2_remove(struct platform_device *pdev) +{ + struct rcar_csi2 *priv = platform_get_drvdata(pdev); + + v4l2_async_notifier_unregister(&priv->notifier); + v4l2_async_notifier_cleanup(&priv->notifier); + v4l2_async_unregister_subdev(&priv->subdev); + + pm_runtime_disable(&pdev->dev); + + return 0; +} + +static struct platform_driver __refdata rcar_csi2_pdrv = { + .remove = rcar_csi2_remove, + .probe = rcar_csi2_probe, + .driver = { + .name = "rcar-csi2", + .of_match_table = rcar_csi2_of_table, + }, +}; + +module_platform_driver(rcar_csi2_pdrv); + +MODULE_AUTHOR("Niklas Söderlund <niklas.soderlund@ragnatech.se>"); +MODULE_DESCRIPTION("Renesas R-Car MIPI CSI-2 receiver"); +MODULE_LICENSE("GPL");