From: <Eugen.Hristev@microchip.com>
To: <jacopo@jmondi.org>
Cc: <linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<laurent.pinchart@ideasonboard.com>, <sakari.ailus@iki.fi>,
<Nicolas.Ferre@microchip.com>
Subject: Re: [PATCH v2 03/25] media: atmel: introduce microchip csi2dc driver
Date: Fri, 10 Dec 2021 11:07:15 +0000 [thread overview]
Message-ID: <3d5cf51e-3ea0-9fff-b9f8-cd0eadb4ea2b@microchip.com> (raw)
In-Reply-To: <20211206120630.mlciq2662azznesu@uno.localdomain>
On 12/6/21 2:06 PM, Jacopo Mondi wrote:
> Hi Eugen
>
> On Mon, Dec 06, 2021 at 11:42:25AM +0000, Eugen.Hristev@microchip.com wrote:
>> On 12/6/21 12:51 PM, Jacopo Mondi wrote:
>>> Hello Eugen
>>>
>>> thanks for addressing all my previous comments, just a few more
>>> things and the driver looks good to me
>>
>> Thanks for reviewing. I tried my best to accomodate your suggestions.
>>>
>>> On Fri, Nov 12, 2021 at 04:24:47PM +0200, Eugen Hristev wrote:
>>>> Microchip CSI2DC (CSI2 Demultiplexer Controller) is a misc bridge device
>>>> that converts a byte stream in IDI Synopsys format (coming from a CSI2HOST)
>>>> to a pixel stream that can be captured by a sensor controller.
>>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>>> ---
>>>> Changes in v2:
>>>> - implement try formats
>>>> - added parallel mode
>>>> - moved to fwlink endpoints
>>>> - many other minor corrections from Jacopo's review
>>>>
>>>> Changes in this revision:
>>>> - addressed comments by Jacopo and Laurent as in this thread:
>>>> https://www.spinics.net/lists/linux-media/msg181044.html
>>>>
>>>> Previous change log :
>>>> Changes in v5:
>>>> - only in bindings
>>>>
>>>> Changes in v4:
>>>> - now using get_mbus_config ops to get data from the subdevice, like the
>>>> virtual channel id, and the clock type.
>>>> - now having possibility to select any of the RAW10 data modes
>>>> - at completion time, select which formats are also available in the subdevice,
>>>> and move to the dynamic list accordingly
>>>> - changed the pipeline integration, do not advertise subdev ready at probe time.
>>>> wait until completion is done, and then start a workqueue that will register
>>>> this device as a subdevice for the next element in pipeline.
>>>> - moved the s_power code into a different function called now csi2dc_power
>>>> that is called with CONFIG_PM functions. This is also called at completion,
>>>> to have the device ready in case CONFIG_PM is not selected on the platform.
>>>> - merged try_fmt into set_fmt
>>>> - driver cleanup, wrapped lines over 80 characters
>>>>
>>>> Changes in v2:
>>>> - moved driver to platform/atmel
>>>> - fixed minor things as per Sakari's review
>>>> - still some things from v2 review are not yet addressed, to be followed up
>>>>
>>>>
>>>> drivers/media/platform/Makefile | 1 +
>>>> drivers/media/platform/atmel/Kconfig | 15 +
>>>> drivers/media/platform/atmel/Makefile | 1 +
>>>> .../media/platform/atmel/microchip-csi2dc.c | 797 ++++++++++++++++++
>>>> 4 files changed, 814 insertions(+)
>>>> create mode 100644 drivers/media/platform/atmel/microchip-csi2dc.c
>>>>
>>>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>>>> index 832357240e89..b18e5f704145 100644
>>>> --- a/drivers/media/platform/Makefile
>>>> +++ b/drivers/media/platform/Makefile
>>>> @@ -69,6 +69,7 @@ obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin/
>>>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel/
>>>> obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel/
>>>> obj-$(CONFIG_VIDEO_ATMEL_XISC) += atmel/
>>>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += atmel/
>>>>
>>>> obj-$(CONFIG_VIDEO_STM32_DCMI) += stm32/
>>>>
>>>> diff --git a/drivers/media/platform/atmel/Kconfig b/drivers/media/platform/atmel/Kconfig
>>>> index dda2f27da317..f83bee373d82 100644
>>>> --- a/drivers/media/platform/atmel/Kconfig
>>>> +++ b/drivers/media/platform/atmel/Kconfig
>>>> @@ -40,3 +40,18 @@ config VIDEO_ATMEL_ISI
>>>> help
>>>> This module makes the ATMEL Image Sensor Interface available
>>>> as a v4l2 device.
>>>> +
>>>> +config VIDEO_MICROCHIP_CSI2DC
>>>> + tristate "Microchip CSI2 Demux Controller"
>>>> + depends on VIDEO_V4L2 && COMMON_CLK && OF
>>>> + depends on ARCH_AT91 || COMPILE_TEST
>>>> + select MEDIA_CONTROLLER
>>>> + select VIDEO_V4L2_SUBDEV_API
>>>> + select V4L2_FWNODE
>>>> + help
>>>> + CSI2 Demux Controller driver. CSI2DC is a helper chip
>>>> + that converts IDI interface byte stream to a parallel pixel stream.
>>>> + It supports various RAW formats as input.
>>>> +
>>>> + To compile this driver as a module, choose M here: the
>>>> + module will be called microchip-csi2dc.
>>>> diff --git a/drivers/media/platform/atmel/Makefile b/drivers/media/platform/atmel/Makefile
>>>> index 46d264ab7948..39f0a7eba702 100644
>>>> --- a/drivers/media/platform/atmel/Makefile
>>>> +++ b/drivers/media/platform/atmel/Makefile
>>>> @@ -6,3 +6,4 @@ obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
>>>> obj-$(CONFIG_VIDEO_ATMEL_ISC_BASE) += atmel-isc-base.o
>>>> obj-$(CONFIG_VIDEO_ATMEL_ISC) += atmel-isc.o
>>>> obj-$(CONFIG_VIDEO_ATMEL_XISC) += atmel-xisc.o
>>>> +obj-$(CONFIG_VIDEO_MICROCHIP_CSI2DC) += microchip-csi2dc.o
>>>> diff --git a/drivers/media/platform/atmel/microchip-csi2dc.c b/drivers/media/platform/atmel/microchip-csi2dc.c
>>>> new file mode 100644
>>>> index 000000000000..2b106f6fd5d0
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/atmel/microchip-csi2dc.c
>>>> @@ -0,0 +1,797 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Microchip CSI2 Demux Controller (CSI2DC) driver
>>>> + *
>>>> + * Copyright (C) 2018 Microchip Technology, Inc.
>>>> + *
>>>> + * Author: Eugen Hristev <eugen.hristev@microchip.com>
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of_graph.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/videodev2.h>
>>>> +
>>>> +#include <media/v4l2-fwnode.h>
>>>> +#include <media/v4l2-subdev.h>
>>>> +
>>>> +/* Global configuration register */
>>>> +#define CSI2DC_GCFG 0x0
>>>> +
>>>> +/* MIPI sensor pixel clock is free running */
>>>> +#define CSI2DC_GCFG_MIPIFRN BIT(0)
>>>> +/* GPIO parallel interface selection */
>>>> +#define CSI2DC_GCFG_GPIOSEL BIT(1)
>>>> +/* Output waveform inter-line minimum delay */
>>>> +#define CSI2DC_GCFG_HLC(v) ((v) << 4)
>>>> +#define CSI2DC_GCFG_HLC_MASK GENMASK(7, 4)
>>>> +/* SAMA7G5 requires a HLC delay of 15 */
>>>> +#define SAMA7G5_HLC (15)
>>>> +
>>>> +/* Global control register */
>>>> +#define CSI2DC_GCTLR 0x04
>>>> +#define CSI2DC_GCTLR_SWRST BIT(0)
>>>> +
>>>> +/* Global status register */
>>>> +#define CSI2DC_GS 0x08
>>>> +
>>>> +/* SSP interrupt status register */
>>>> +#define CSI2DC_SSPIS 0x28
>>>> +/* Pipe update register */
>>>> +#define CSI2DC_PU 0xc0
>>>> +/* Video pipe attributes update */
>>>> +#define CSI2DC_PU_VP BIT(0)
>>>> +
>>>> +/* Pipe update status register */
>>>> +#define CSI2DC_PUS 0xc4
>>>> +
>>>> +/* Video pipeline enable register */
>>>> +#define CSI2DC_VPE 0xf8
>>>> +#define CSI2DC_VPE_ENABLE BIT(0)
>>>> +
>>>> +/* Video pipeline configuration register */
>>>> +#define CSI2DC_VPCFG 0xfc
>>>> +/* Data type */
>>>> +#define CSI2DC_VPCFG_DT(v) ((v) << 0)
>>>> +#define CSI2DC_VPCFG_DT_MASK GENMASK(5, 0)
>>>> +/* Virtual channel identifier */
>>>> +#define CSI2DC_VPCFG_VC(v) ((v) << 6)
>>>> +#define CSI2DC_VPCFG_VC_MASK GENMASK(7, 6)
>>>> +/* Decompression enable */
>>>> +#define CSI2DC_VPCFG_DE BIT(8)
>>>> +/* Decoder mode */
>>>> +#define CSI2DC_VPCFG_DM(v) ((v) << 9)
>>>> +#define CSI2DC_VPCFG_DM_DECODER8TO12 0
>>>> +/* Decoder predictor 2 selection */
>>>> +#define CSI2DC_VPCFG_DP2 BIT(12)
>>>> +/* Recommended memory storage */
>>>> +#define CSI2DC_VPCFG_RMS BIT(13)
>>>> +/* Post adjustment */
>>>> +#define CSI2DC_VPCFG_PA BIT(14)
>>>> +
>>>> +/* Video pipeline column register */
>>>> +#define CSI2DC_VPCOL 0x100
>>>> +/* Column number */
>>>> +#define CSI2DC_VPCOL_COL(v) ((v) << 0)
>>>> +#define CSI2DC_VPCOL_COL_MASK GENMASK(15, 0)
>>>> +
>>>> +/* Video pipeline row register */
>>>> +#define CSI2DC_VPROW 0x104
>>>> +/* Row number */
>>>> +#define CSI2DC_VPROW_ROW(v) ((v) << 0)
>>>> +#define CSI2DC_VPROW_ROW_MASK GENMASK(15, 0)
>>>> +
>>>> +/* Version register */
>>>> +#define CSI2DC_VERSION 0x1fc
>>>> +
>>>> +/* register read/write helpers */
>>>> +#define csi2dc_readl(st, reg) readl_relaxed((st)->base + (reg))
>>>> +#define csi2dc_writel(st, reg, val) writel_relaxed((val), \
>>>> + (st)->base + (reg))
>>>> +
>>>> +/* supported RAW data types */
>>>> +#define CSI2DC_DT_RAW6 0x28
>>>> +#define CSI2DC_DT_RAW7 0x29
>>>> +#define CSI2DC_DT_RAW8 0x2a
>>>> +#define CSI2DC_DT_RAW10 0x2b
>>>> +#define CSI2DC_DT_RAW12 0x2c
>>>> +#define CSI2DC_DT_RAW14 0x2d
>>>> +
>>>> +/*
>>>> + * struct csi2dc_format - CSI2DC format type struct
>>>> + * @mbus_code: Media bus code for the format
>>>> + * @dt: Data type constant for this format
>>>> + */
>>>> +struct csi2dc_format {
>>>> + u32 mbus_code;
>>>> + u32 dt;
>>>> +};
>>>> +
>>>> +static const struct csi2dc_format csi2dc_formats[] = {
>>>> + {
>>>> + .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
>>>> + .dt = CSI2DC_DT_RAW8,
>>>> + }, {
>>>> + .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
>>>> + .dt = CSI2DC_DT_RAW8,
>>>> + }, {
>>>> + .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
>>>> + .dt = CSI2DC_DT_RAW8,
>>>> + }, {
>>>> + .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
>>>> + .dt = CSI2DC_DT_RAW8,
>>>> + }, {
>>>> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
>>>> + .dt = CSI2DC_DT_RAW10,
>>>> + }, {
>>>> + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
>>>> + .dt = CSI2DC_DT_RAW10,
>>>> + }, {
>>>> + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
>>>> + .dt = CSI2DC_DT_RAW10,
>>>> + }, {
>>>> + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
>>>> + .dt = CSI2DC_DT_RAW10,
>>>> + }, {
>>>> + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
>>>
>>> Is this intentionally not associated with a CSI-2 DT code ?
>>
>> Yes. It is not a raw format.
>> I do not have a sensor that could stream YUYV8_2X8 with CSI-2 .
>> I tested this mode with parallel sensor, and in this case, CSI-2 DT
>> value does not make sense and it's not used.
>> The idea would be that when and if I get my hands on a sensor that could
>> actually do this with CSI2, I would update the driver here (if it works.
>> I am not sure it actually does ...)
>
> I understand, but the first user of your driver that connects a
> YUV-capable sensor will be very disappointed, as debugging the fact
> that the DT identifier is silently set to 0 will be quite painful (as
> if I'm not mistaken fields not initialized in a designated initializer
> list are set to 0).
>
> As the DT identifiers are defined by the CSI-2 specification and will
> not change, I think you can safely add it here.
>
> Thanks
> j
>
>>>
>>>> + },
>>>> +};
>>>> +
>>>> +enum mipi_csi_pads {
>>>> + CSI2DC_PAD_SINK = 0,
>>>> + CSI2DC_PAD_SOURCE = 1,
>>>> + CSI2DC_PADS_NUM = 2,
>>>> +};
>>>> +
>>>> +/*
>>>> + * struct csi2dc_device - CSI2DC device driver data/config struct
>>>> + * @base: Register map base address
>>>> + * @csi2dc_sd: v4l2 subdevice for the csi2dc device
>>>> + * This is the subdevice that the csi2dc device itself
>>>> + * registers in v4l2 subsystem
>>>> + * @dev: struct device for this csi2dc device
>>>> + * @pclk: Peripheral clock reference
>>>> + * Input clock that clocks the hardware block internal
>>>> + * logic
>>>> + * @scck: Sensor Controller clock reference
>>>> + * Input clock that is used to generate the pixel clock
>>>> + * @format: Current saved format used in g/s fmt
>>>> + * @cur_fmt: Current state format
>>>> + * @try_fmt: Try format that is being tried
>>>> + * @pads: Media entity pads for the csi2dc subdevice
>>>> + * @clk_gated: Whether the clock is gated or free running
>>>> + * @video_pipe: Whether video pipeline is configured
>>>> + * @parallel_mode: The underlying subdevice is connected on a parallel bus
>>>> + * @vc: Current set virtual channel
>>>> + * @notifier: Async notifier that is used to bound the underlying
>>>> + * subdevice to the csi2dc subdevice
>>>> + * @input_sd: Reference to the underlying subdevice bound to the
>>>> + * csi2dc subdevice
>>>> + * @remote_pad: Pad number of the underlying subdevice that is linked
>>>> + * to the csi2dc subdevice sink pad.
>>>> + */
>>>> +struct csi2dc_device {
>>>> + void __iomem *base;
>>>> + struct v4l2_subdev csi2dc_sd;
>>>> + struct device *dev;
>>>> + struct clk *pclk;
>>>> + struct clk *scck;
>>>> +
>>>> + struct v4l2_mbus_framefmt format;
>>>> +
>>>> + const struct csi2dc_format *cur_fmt;
>>>> + const struct csi2dc_format *try_fmt;
>>>> +
>>>> + struct media_pad pads[CSI2DC_PADS_NUM];
>>>> +
>>>> + bool clk_gated;
>>>> + bool video_pipe;
>>>> + bool parallel_mode;
>>>> + u32 vc;
>>>> +
>>>> + struct v4l2_async_notifier notifier;
>>>> +
>>>> + struct v4l2_subdev *input_sd;
>>>> +
>>>> + u32 remote_pad;
>>>> +};
>>>> +
>>>> +static inline struct csi2dc_device *
>>>> +csi2dc_sd_to_csi2dc_device(struct v4l2_subdev *csi2dc_sd)
>>>> +{
>>>> + return container_of(csi2dc_sd, struct csi2dc_device, csi2dc_sd);
>>>> +}
>>>> +
>>>> +static int csi2dc_enum_mbus_code(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_state *sd_state,
>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>> +{
>>>> + if (code->index >= ARRAY_SIZE(csi2dc_formats))
>>>> + return -EINVAL;
>>>> +
>>>> + code->code = csi2dc_formats[code->index].mbus_code;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int csi2dc_get_fmt(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_state *sd_state,
>>>> + struct v4l2_subdev_format *format)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> + struct v4l2_mbus_framefmt *v4l2_try_fmt;
>>>> +
>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>>> + v4l2_try_fmt = v4l2_subdev_get_try_format(csi2dc_sd, sd_state,
>>>> + format->pad);
>>>> + format->format = *v4l2_try_fmt;
>>>> +
>>>> + return 0;
>>>> + }
>>>> +
>>>> + format->format = csi2dc->format;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int csi2dc_set_fmt(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_state *sd_state,
>>>> + struct v4l2_subdev_format *req_fmt)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> + const struct csi2dc_format *fmt, *try_fmt = NULL;
>>>> + struct v4l2_mbus_framefmt *v4l2_try_fmt;
>>>> + unsigned int i;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(csi2dc_formats); i++) {
>>>> + fmt = &csi2dc_formats[i];
>>>> + if (req_fmt->format.code == fmt->mbus_code)
>>>> + try_fmt = fmt;
>>>> + fmt++;
>>>> + }
>>>> +
>>>> + /* in case we could not find the desired format, default to something */
>>>> + if (!try_fmt) {
>>>> + try_fmt = &csi2dc_formats[0];
>>>> +
>>>> + dev_dbg(csi2dc->dev,
>>>> + "CSI2DC unsupported format 0x%x, defaulting to 0x%x\n",
>>>> + req_fmt->format.code, csi2dc_formats[0].mbus_code);
>>>> + }
>>>> +
>>>> + req_fmt->format.code = try_fmt->mbus_code;
>>>> + req_fmt->format.colorspace = V4L2_COLORSPACE_SRGB;
>>>> + req_fmt->format.field = V4L2_FIELD_NONE;
>>>> +
>>>> + if (req_fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>>>> + v4l2_try_fmt = v4l2_subdev_get_try_format(csi2dc_sd, sd_state,
>>>> + req_fmt->pad);
>>>> + *v4l2_try_fmt = req_fmt->format;
>>>> + /* Trying on the pad sink makes the source sink change too */
>>>> + if (req_fmt->pad == CSI2DC_PAD_SINK) {
>>>
>>> s/source sink/source pad/
>>>
>>>> + v4l2_try_fmt =
>>>> + v4l2_subdev_get_try_format(csi2dc_sd,
>>>> + sd_state,
>>>> + CSI2DC_PAD_SOURCE);
>>>> + *v4l2_try_fmt = req_fmt->format;
>>>
>>> Shouldn't this be the same for the active format then ? If this device
>>> operates by transporting the same exact format from the sink to the
>>> source, shouldn't you disable setting a format on the source, and
>>> always propagate to the sink ? I would have done so, but maybe
>>> Sakari/Laurent could tell you if that's fine.
>>
>> Hm. I will try. To see if it works and v4l2-compliance is happy. ( I did
>> this propagation for try format because it wasn't )
>>
>>>
>>>> + }
>>>> + /* if we are just trying, we are done */
>>>> + return 0;
>>>> + }
>>>> +
>>>> + /* save the format for later requests */
>>>> + csi2dc->format = req_fmt->format;
>>>> +
>>>> + /* update config */
>>>> + csi2dc->cur_fmt = try_fmt;
>>>> +
>>>> + dev_dbg(csi2dc->dev, "new format set: 0x%x @%dx%d\n",
>>>> + csi2dc->format.code, csi2dc->format.width,
>>>> + csi2dc->format.height);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int csi2dc_power(struct csi2dc_device *csi2dc, int on)
>>>> +{
>>>> + int ret = 0;
>>>> +
>>>> + if (on) {
>>>> + ret = clk_prepare_enable(csi2dc->pclk);
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev, "failed to enable pclk:%d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = clk_prepare_enable(csi2dc->scck);
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev, "failed to enable scck:%d\n", ret);
>>>> + clk_disable_unprepare(csi2dc->pclk);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /* if powering up, deassert reset line */
>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, CSI2DC_GCTLR_SWRST);
>>>> + } else {
>>>> + /* if powering down, assert reset line */
>>>> + csi2dc_writel(csi2dc, CSI2DC_GCTLR, 0);
>>>> +
>>>> + clk_disable_unprepare(csi2dc->scck);
>>>> + clk_disable_unprepare(csi2dc->pclk);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int csi2dc_get_mbus_config(struct csi2dc_device *csi2dc)
>>>> +{
>>>> + struct v4l2_mbus_config mbus_config = { 0 };
>>>> + int ret;
>>>> +
>>>> + ret = v4l2_subdev_call(csi2dc->input_sd, pad, get_mbus_config,
>>>> + csi2dc->remote_pad, &mbus_config);
>>>> + if (ret == -ENOIOCTLCMD) {
>>>> + dev_dbg(csi2dc->dev,
>>>> + "no remote mbus configuration available\n");
>>>> + goto csi2dc_get_mbus_config_defaults;
>>>> + }
>>>> +
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev,
>>>> + "failed to get remote mbus configuration\n");
>>>> + goto csi2dc_get_mbus_config_defaults;
>>>> + }
>>>> +
>>>> + if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_0)
>>>> + csi2dc->vc = 0;
>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_1)
>>>> + csi2dc->vc = 1;
>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_2)
>>>> + csi2dc->vc = 2;
>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_CHANNEL_3)
>>>> + csi2dc->vc = 3;
>>>> +
>>>> + dev_dbg(csi2dc->dev, "subdev sending on channel %d\n", csi2dc->vc);
>>>> +
>>>> + csi2dc->clk_gated = mbus_config.flags &
>>>> + V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
>>>> +
>>>> + dev_dbg(csi2dc->dev, "mbus_config: %s clock\n",
>>>> + csi2dc->clk_gated ? "gated" : "free running");
>>>> +
>>>> + return 0;
>>>> +
>>>> +csi2dc_get_mbus_config_defaults:
>>>> + csi2dc->vc = 0; /* Virtual ID 0 by default */
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void csi2dc_vp_update(struct csi2dc_device *csi2dc)
>>>> +{
>>>> + u32 vp, gcfg;
>>>> +
>>>> + if (!csi2dc->video_pipe) {
>>>> + dev_err(csi2dc->dev, "video pipeline unavailable\n");
>>>> + return;
>>>> + }
>>>> +
>>>> + if (csi2dc->parallel_mode) {
>>>> + /* In parallel mode, GPIO parallel interface must be selected */
>>>> + gcfg = csi2dc_readl(csi2dc, CSI2DC_GCFG);
>>>> + gcfg |= CSI2DC_GCFG_GPIOSEL;
>>>> + csi2dc_writel(csi2dc, CSI2DC_GCFG, gcfg);
>>>> + return;
>>>> + }
>>>> +
>>>> + /* serial video pipeline */
>>>> +
>>>> + csi2dc_writel(csi2dc, CSI2DC_GCFG,
>>>> + (SAMA7G5_HLC & CSI2DC_GCFG_HLC_MASK) |
>>>> + (csi2dc->clk_gated ? 0 : CSI2DC_GCFG_MIPIFRN));
>>>> +
>>>> + csi2dc_writel(csi2dc, CSI2DC_VPCOL,
>>>> + CSI2DC_VPCOL_COL(0xfff) & CSI2DC_VPCOL_COL_MASK);
>>>> + csi2dc_writel(csi2dc, CSI2DC_VPROW,
>>>> + CSI2DC_VPROW_ROW(0xfff) & CSI2DC_VPROW_ROW_MASK);
>>>> +
>>>> + vp = CSI2DC_VPCFG_DT(csi2dc->cur_fmt->dt) & CSI2DC_VPCFG_DT_MASK;
>>>> + vp |= CSI2DC_VPCFG_VC(csi2dc->vc) & CSI2DC_VPCFG_VC_MASK;
>>>> + vp &= ~CSI2DC_VPCFG_DE;
>>>> + vp |= CSI2DC_VPCFG_DM(CSI2DC_VPCFG_DM_DECODER8TO12);
>>>> + vp &= ~CSI2DC_VPCFG_DP2;
>>>> + vp &= ~CSI2DC_VPCFG_RMS;
>>>> + vp |= CSI2DC_VPCFG_PA;
>>>> +
>>>> + csi2dc_writel(csi2dc, CSI2DC_VPCFG, vp);
>>>> + csi2dc_writel(csi2dc, CSI2DC_VPE, CSI2DC_VPE_ENABLE);
>>>> + csi2dc_writel(csi2dc, CSI2DC_PU, CSI2DC_PU_VP);
>>>> +}
>>>> +
>>>> +static int csi2dc_s_stream(struct v4l2_subdev *csi2dc_sd, int enable)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = csi2dc_sd_to_csi2dc_device(csi2dc_sd);
>>>> + int ret;
>>>> +
>>>> + if (enable) {
>>>> + ret = pm_runtime_resume_and_get(csi2dc->dev);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + csi2dc_get_mbus_config(csi2dc);
>>>> +
>>>> + csi2dc_vp_update(csi2dc);
>>>> +
>>>> + return v4l2_subdev_call(csi2dc->input_sd, video, s_stream,
>>>> + true);
>>>> + }
>>>> + /* stop streaming scenario */
>>>> + ret = v4l2_subdev_call(csi2dc->input_sd, video, s_stream, false);
>>>> +
>>>> + pm_runtime_put_sync(csi2dc->dev);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int csi2dc_init_cfg(struct v4l2_subdev *csi2dc_sd,
>>>> + struct v4l2_subdev_state *sd_state)
>>>> +{
>>>> + struct v4l2_mbus_framefmt *v4l2_try_fmt =
>>>> + v4l2_subdev_get_try_format(csi2dc_sd, sd_state, 0);
>>>> +
>>>> + v4l2_try_fmt->height = 480;
>>>> + v4l2_try_fmt->width = 640;
>>>> + v4l2_try_fmt->code = csi2dc_formats[0].mbus_code;
>>>> + v4l2_try_fmt->colorspace = V4L2_COLORSPACE_SRGB;
>>>> + v4l2_try_fmt->field = V4L2_FIELD_NONE;
>>>> + v4l2_try_fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>>> + v4l2_try_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
>>>> + v4l2_try_fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct v4l2_subdev_pad_ops csi2dc_pad_ops = {
>>>> + .enum_mbus_code = csi2dc_enum_mbus_code,
>>>> + .set_fmt = csi2dc_set_fmt,
>>>> + .get_fmt = csi2dc_get_fmt,
>>>> + .init_cfg = csi2dc_init_cfg,
>>>> +};
>>>> +
>>>> +static const struct v4l2_subdev_video_ops csi2dc_video_ops = {
>>>> + .s_stream = csi2dc_s_stream,
>>>> +};
>>>> +
>>>> +static const struct v4l2_subdev_ops csi2dc_subdev_ops = {
>>>> + .pad = &csi2dc_pad_ops,
>>>> + .video = &csi2dc_video_ops,
>>>> +};
>>>> +
>>>> +static int csi2dc_async_bound(struct v4l2_async_notifier *notifier,
>>>> + struct v4l2_subdev *subdev,
>>>> + struct v4l2_async_subdev *asd)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = container_of(notifier,
>>>> + struct csi2dc_device, notifier);
>>>> + int pad;
>>>> + int ret;
>>>> +
>>>> + csi2dc->input_sd = subdev;
>>>> +
>>>> + pad = media_entity_get_fwnode_pad(&subdev->entity, asd->match.fwnode,
>>>> + MEDIA_PAD_FL_SOURCE);
>>>> + if (pad < 0) {
>>>> + dev_err(csi2dc->dev, "Failed to find pad for %s\n",
>>>> + subdev->name);
>>>> + return pad;
>>>> + }
>>>> +
>>>> + csi2dc->remote_pad = pad;
>>>> +
>>>> + ret = media_create_pad_link(&csi2dc->input_sd->entity,
>>>> + csi2dc->remote_pad,
>>>> + &csi2dc->csi2dc_sd.entity, 0,
>>>> + MEDIA_LNK_FL_ENABLED);
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev,
>>>> + "Failed to create pad link: %s to %s\n",
>>>> + csi2dc->input_sd->entity.name,
>>>> + csi2dc->csi2dc_sd.entity.name);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + dev_dbg(csi2dc->dev, "link with %s pad: %d\n",
>>>> + csi2dc->input_sd->name, csi2dc->remote_pad);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct v4l2_async_notifier_operations csi2dc_async_ops = {
>>>> + .bound = csi2dc_async_bound,
>>>> +};
>>>> +
>>>> +static int csi2dc_prepare_notifier(struct csi2dc_device *csi2dc,
>>>> + struct fwnode_handle *input_fwnode)
>>>> +{
>>>> + struct v4l2_async_subdev *asd;
>>>> + int ret = 0;
>>>> +
>>>> + v4l2_async_nf_init(&csi2dc->notifier);
>>>> +
>>>> + asd = v4l2_async_nf_add_fwnode_remote(&csi2dc->notifier,
>>>> + input_fwnode,
>>>> + struct v4l2_async_subdev);
>>>> +
>>>> + fwnode_handle_put(input_fwnode);
>>>> +
>>>> + if (IS_ERR(asd)) {
>>>> + ret = PTR_ERR(asd);
>>>> + dev_err(csi2dc->dev,
>>>> + "failed to add async notifier for node %pOF: %d\n",
>>>> + to_of_node(input_fwnode), ret);
>>>> + v4l2_async_nf_cleanup(&csi2dc->notifier);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + csi2dc->notifier.ops = &csi2dc_async_ops;
>>>> +
>>>> + ret = v4l2_async_subdev_nf_register(&csi2dc->csi2dc_sd,
>>>> + &csi2dc->notifier);
>>>
>>> Drop this blank line
>>>
>>>> +
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev, "fail to register async notifier: %d\n",
>>>> + ret);
>>>> + v4l2_async_nf_cleanup(&csi2dc->notifier);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int csi2dc_of_parse(struct csi2dc_device *csi2dc,
>>>> + struct device_node *of_node)
>>>> +{
>>>> + struct fwnode_handle *input_fwnode, *output_fwnode;
>>>> + struct v4l2_fwnode_endpoint input_endpoint = { 0 },
>>>> + output_endpoint = { 0 };
>>>> + int ret;
>>>> +
>>>> + input_fwnode = fwnode_graph_get_next_endpoint(of_fwnode_handle(of_node),
>>>> + NULL);
>>>> +
>>>> + if (!input_fwnode) {
>>>> + dev_err(csi2dc->dev,
>>>> + "missing port node at %pOF, input node is mandatory.\n",
>>>> + of_node);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ret = v4l2_fwnode_endpoint_parse(input_fwnode, &input_endpoint);
>>>> +
>>>
>>> Drop this blank line
>>>
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev, "endpoint not defined at %pOF\n", of_node);
>>>> + goto csi2dc_of_parse_err;
>>>> + }
>>>> +
>>>> + if (input_endpoint.bus_type == V4L2_MBUS_PARALLEL ||
>>>> + input_endpoint.bus_type == V4L2_MBUS_BT656) {
>>>> + csi2dc->parallel_mode = true;
>>>> + dev_dbg(csi2dc->dev,
>>>> + "subdevice connected on parallel interface\n");
>>>> + }
>>>> +
>>>> + if (input_endpoint.bus_type == V4L2_MBUS_CSI2_DPHY) {
>>>> + csi2dc->clk_gated = input_endpoint.bus.mipi_csi2.flags &
>>>> + V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
>>>> + dev_dbg(csi2dc->dev,
>>>> + "subdevice connected on serial interface\n");
>>>> + dev_dbg(csi2dc->dev, "DT: %s clock\n",
>>>> + csi2dc->clk_gated ? "gated" : "free running");
>>>> + }
>>>> +
>>>> + output_fwnode = fwnode_graph_get_next_endpoint
>>>> + (of_fwnode_handle(of_node), input_fwnode);
>>>> +
>>>> + if (output_fwnode)
>>>> + ret = v4l2_fwnode_endpoint_parse(output_fwnode,
>>>> + &output_endpoint);
>>>> +
>>>> + fwnode_handle_put(output_fwnode);
>>>> +
>>>> + if (!output_fwnode || ret) {
>>>> + dev_info(csi2dc->dev,
>>>> + "missing output node at %pOF, data pipe available only.\n",
>>>> + of_node);
>>>> + } else {
>>>> + if (output_endpoint.bus_type != V4L2_MBUS_PARALLEL &&
>>>> + output_endpoint.bus_type != V4L2_MBUS_BT656) {
>>>> + dev_err(csi2dc->dev,
>>>> + "output port must be parallel/bt656.\n");
>>>> + ret = -EINVAL;
>>>> + goto csi2dc_of_parse_err;
>>>> + }
>>>> +
>>>> + csi2dc->video_pipe = true;
>>>> +
>>>> + dev_dbg(csi2dc->dev,
>>>> + "block %pOF [%d.%d]->[%d.%d] video pipeline\n",
>>>> + of_node, input_endpoint.base.port,
>>>> + input_endpoint.base.id, output_endpoint.base.port,
>>>> + output_endpoint.base.id);
>>>> + }
>>>> +
>>>> + /* prepare async notifier for subdevice completion */
>>>> + return csi2dc_prepare_notifier(csi2dc, input_fwnode);
>>>> +
>>>> +csi2dc_of_parse_err:
>>>> + fwnode_handle_put(input_fwnode);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void csi2dc_default_format(struct csi2dc_device *csi2dc)
>>>> +{
>>>> + csi2dc->cur_fmt = &csi2dc_formats[0];
>>>> +
>>>> + csi2dc->format.height = 480;
>>>> + csi2dc->format.width = 640;
>>>> + csi2dc->format.code = csi2dc_formats[0].mbus_code;
>>>> + csi2dc->format.colorspace = V4L2_COLORSPACE_SRGB;
>>>> + csi2dc->format.field = V4L2_FIELD_NONE;
>>>> + csi2dc->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>>> + csi2dc->format.quantization = V4L2_QUANTIZATION_DEFAULT;
>>>> + csi2dc->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>>> +}
>>>> +
>>>> +static int csi2dc_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct device *dev = &pdev->dev;
>>>> + struct csi2dc_device *csi2dc;
>>>> + int ret = 0;
>>>> + u32 ver;
>>>> +
>>>> + csi2dc = devm_kzalloc(dev, sizeof(*csi2dc), GFP_KERNEL);
>>>> + if (!csi2dc)
>>>> + return -ENOMEM;
>>>> +
>>>> + csi2dc->dev = dev;
>>>> +
>>>> + csi2dc->base = devm_platform_ioremap_resource(pdev, 0);
>>>> + if (IS_ERR(csi2dc->base)) {
>>>> + dev_err(dev, "base address not set\n");
>>>> + return PTR_ERR(csi2dc->base);
>>>> + }
>>>> +
>>>> + csi2dc->pclk = devm_clk_get(dev, "pclk");
>>>> + if (IS_ERR(csi2dc->pclk)) {
>>>> + ret = PTR_ERR(csi2dc->pclk);
>>>> + dev_err(dev, "failed to get pclk: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + csi2dc->scck = devm_clk_get(dev, "scck");
>>>> + if (IS_ERR(csi2dc->scck)) {
>>>> + ret = PTR_ERR(csi2dc->scck);
>>>> + dev_err(dev, "failed to get scck: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + v4l2_subdev_init(&csi2dc->csi2dc_sd, &csi2dc_subdev_ops);
>>>> +
>>>> + csi2dc->csi2dc_sd.owner = THIS_MODULE;
>>>> + csi2dc->csi2dc_sd.dev = dev;
>>>> + snprintf(csi2dc->csi2dc_sd.name, sizeof(csi2dc->csi2dc_sd.name),
>>>> + "csi2dc");
>>>> +
>>>> + csi2dc->csi2dc_sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>>> + csi2dc->csi2dc_sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>>>> +
>>>> + platform_set_drvdata(pdev, csi2dc);
>>>> +
>>>> + ret = csi2dc_of_parse(csi2dc, dev->of_node);
>>>> + if (ret)
>>>> + goto csi2dc_probe_cleanup_entity;
>>>> +
>>>> + csi2dc->pads[CSI2DC_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>>> + if (csi2dc->video_pipe)
>>>> + csi2dc->pads[CSI2DC_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
>>>> +
>>>> + ret = media_entity_pads_init(&csi2dc->csi2dc_sd.entity,
>>>> + csi2dc->video_pipe ? CSI2DC_PADS_NUM : 1,
>>>> + csi2dc->pads);
>>>> + if (ret < 0) {
>>>> + dev_err(dev, "media entity init failed\n");
>>>> + goto csi2dc_probe_cleanup_notifier;
>>>> + }
>>>> +
>>>> + csi2dc_default_format(csi2dc);
>>>> +
>>>> + /* turn power on to validate capabilities */
>>>> + ret = csi2dc_power(csi2dc, true);
>>>> + if (ret < 0)
>>>> + goto csi2dc_probe_cleanup_notifier;
>>>> +
>>>> + pm_runtime_set_active(dev);
>>>> + pm_runtime_enable(dev);
>>>> + ver = csi2dc_readl(csi2dc, CSI2DC_VERSION);
>>>
>>> Shouldn't you verify that the version is the expected one, or that, at
>>> least the device can be accessed ?
>>
>> If the read from the register worked, the device can be accessed. If the
>> device won't respond to this, we will get a bus fault or a freeze.
>> And printing the version number can help all the time by inspecting the
>> bootlog to see if something valid was printed.
>> Version changes from product to product but seeing a bootlog later will
>> make us easily understand which product was it, and if the version was
>> read correctly
>>
>>>
>>>> + pm_request_idle(dev);
>>>
>>> I'm not well versed when it comes to runtime_pm but I can read that
>>>
>>> Documentation/power/runtime_pm.rst- `int pm_request_idle(struct device *dev);`
>>> Documentation/power/runtime_pm.rst- - submit a request to execute the subsystem-level idle callback for the
>>> Documentation/power/runtime_pm.rst- device (the request is represented by a work item in pm_wq); returns 0 on
>>> Documentation/power/runtime_pm.rst- success or error code if the request has not been queued up
>>>
>>> And looking below you have
>>>
>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {
>>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
>>> +};
>>>
>>> Which expands to
>>>
>>> include/linux/pm.h: SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \
>>>
>>> so it seems to me you've no idle func defined, hence you above call
>>> has no effect so you're device stays powered on after your above call
>>> to
>>>
>>> ret = csi2dc_power(csi2dc, true);
>>>
>>> Also trying to suspend using runtime_pm while having powered on by
>>> calling the resume function directly would leave the pm status
>>> unbalanced ?
>>>
>>> I would raher call pm_runtime_resume() and
>>> pm_runtime_suspend() or call the power routine directly, and later
>>> enable pm_runtime
>>>
>>> csi2dc_power(true)
>>> readl();
>>> csi2fd_power(false);
>>>
>>> pm_runtime_set_active();
>>> pm_runtime_enable();
>>>
>>> v4l2_async_register();
>>>
>>> Again, not well versed in runtime_pm, so don't assume the above makes
>>> sense :)
>>
>> I am not versed in runtime_pm either, I will try to remove this
>> 'pm_request_idle' to see what happens at the first
>> pm_runtime_resume_and_get, and print messages in power on/power off to
>> see how and when they are called.
>>
>
Hi Jacopo,
I added traces in my power on / power off functions to see who calls them.
Here is the log :
power on
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at
drivers/media/platform/atmel/microchip-csi2dc.c:331 csi2dc_power+0xd0/0x134
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 5.16.0-rc4+ #62
Hardware name: Microchip SAMA7
[<c010b9dc>] (unwind_backtrace) from [<c010975c>] (show_stack+0x10/0x14)
[<c010975c>] (show_stack) from [<c0755aa0>] (__warn+0xd0/0x128)
[<c0755aa0>] (__warn) from [<c0755b70>] (warn_slowpath_fmt+0x78/0xac)
[<c0755b70>] (warn_slowpath_fmt) from [<c0501114>] (csi2dc_power+0xd0/0x134)
[<c0501114>] (csi2dc_power) from [<c05012dc>] (csi2dc_probe+0x158/0x23c)
[<c05012dc>] (csi2dc_probe) from [<c04067cc>] (platform_probe+0x5c/0xb8)
[<c04067cc>] (platform_probe) from [<c04043f8>]
(really_probe.part.0+0x9c/0x32c)
[<c04043f8>] (really_probe.part.0) from [<c0404730>]
(__driver_probe_device+0xa8/0x13c)
[<c0404730>] (__driver_probe_device) from [<c04047f8>]
(driver_probe_device+0x34/0x108)
[<c04047f8>] (driver_probe_device) from [<c0404ea0>]
(__driver_attach+0xb4/0x180)
[<c0404ea0>] (__driver_attach) from [<c040245c>]
(bus_for_each_dev+0x74/0xc0)
[<c040245c>] (bus_for_each_dev) from [<c0403804>]
(bus_add_driver+0x15c/0x1e0)
[<c0403804>] (bus_add_driver) from [<c040572c>] (driver_register+0x8c/0x124)
[<c040572c>] (driver_register) from [<c01014b0>]
(do_one_initcall+0x54/0x1d0)
[<c01014b0>] (do_one_initcall) from [<c0b0114c>]
(kernel_init_freeable+0x19c/0x200)
[<c0b0114c>] (kernel_init_freeable) from [<c075c9e0>]
(kernel_init+0x18/0x128)
[<c075c9e0>] (kernel_init) from [<c0100140>] (ret_from_fork+0x14/0x34)
Exception stack(0xc4033fb0 to 0xc4033ff8)
3fa0: 00000000 00000000 00000000
00000000
3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
3fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace a5faddd6c4e11ca2 ]---
microchip-csi2dc e1404000.csi2dc: Microchip CSI2DC version 145
power off
------------[ cut here ]------------
WARNING: CPU: 0 PID: 28 at
drivers/media/platform/atmel/microchip-csi2dc.c:335
csi2dc_runtime_suspend+0x34/0x6c
Modules linked in:
CPU: 0 PID: 28 Comm: kworker/0:9 Tainted: G W 5.16.0-rc4+ #62
Hardware name: Microchip SAMA7
Workqueue: pm pm_runtime_work
[<c010b9dc>] (unwind_backtrace) from [<c010975c>] (show_stack+0x10/0x14)
[<c010975c>] (show_stack) from [<c0755aa0>] (__warn+0xd0/0x128)
[<c0755aa0>] (__warn) from [<c0755b70>] (warn_slowpath_fmt+0x78/0xac)
[<c0755b70>] (warn_slowpath_fmt) from [<c075b4cc>]
(csi2dc_runtime_suspend+0x34/0x6c)
[<c075b4cc>] (csi2dc_runtime_suspend) from [<c04114b8>]
(__rpm_callback+0x30/0xe4)
[<c04114b8>] (__rpm_callback) from [<c04115cc>] (rpm_callback+0x60/0x64)
[<c04115cc>] (rpm_callback) from [<c0410e28>] (rpm_suspend+0xd0/0x4bc)
[<c0410e28>] (rpm_suspend) from [<c041244c>] (pm_runtime_work+0x90/0x98)
[<c041244c>] (pm_runtime_work) from [<c012b568>]
(process_one_work+0x198/0x410)
[<c012b568>] (process_one_work) from [<c012b85c>] (worker_thread+0x7c/0x570)
[<c012b85c>] (worker_thread) from [<c0132168>] (kthread+0x154/0x174)
[<c0132168>] (kthread) from [<c0100140>] (ret_from_fork+0x14/0x34)
Exception stack(0xc4327fb0 to 0xc4327ff8)
7fa0: 00000000 00000000 00000000
00000000
7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
---[ end trace a5faddd6c4e11ca3 ]---
>
So it looks like after probing is finished, the runtime PM system
switches the device back to suspend mode (as probably there are no 'get'
operations on it ).
I removed the pm_request_idle as it looks you are right, it does nothing.
When trying to capture a frame, this happens:
# cam -c1 -C1
[0:01:14.779664000] [229] INFO IPAManager ipa_manager.cpp:138 libcamera
is not installed. Adding '//src/ipa' to the IPA search path
[0:01:14.784039600] [229] WARN IPAManager ipa_manager.cpp:149 No IPA
found in '/usr/lib/libcamera'
[0:01:14.784219400] [229] INFO Camera camera_manager.cpp:293 libcamera
v0.0.0+60036-2021.11-rc1-182-gd72a7997-dirty (2021-11-15T11:27:09+02:00)
[0:01:14.800801800] [230] WARN CameraSensor camera_sensor.cpp:197
'imx219 1-0010': Recommended V4L2 control 0x009a0922 not supported
[0:01:14.800952200] [230] WARN CameraSensor camera_sensor.cpp:249
'imx219 1-0010': The sensor kernel driver needs to be fixed
[0:01:14.801018400] [230] WARN CameraSensor camera_sensor.cpp:251
'imx219 1-0010': See Documentation/sensor_driver_requirements.rst in the
libcamera sources for more information
[0:01:14.803197200] [230] WARN CameraSensor camera_sensor.cpp:414
'imx219 1-0010': Failed to retrieve the camera location
[0:01:14.805444000] [230] WARN V4L2 v4l2_pixelformat.cpp:287
Unsupported V4L2 pixel format Y16
[0:01:14.805541200] [230] WARN V4L2 v4l2_pixelformat.cpp:287
Unsupported V4L2 pixel format AR12
[0:01:14.805612200] [230] WARN V4L2 v4l2_pixelformat.cpp:287
Unsupported V4L2 pixel format AR15
Using camera /base/soc/flexcom@e2818000/i2c@600/camera@10 as cam0
[0:01:14.806657000] [229] INFO Camera camera.cpp:945 configuring
streams: (0) 3264x2464-R8
power on
------------[ cut here ]------------
WARNING: CPU: 0 PID: 230 at
drivers/media/platform/atmel/microchip-csi2dc.c:331 csi2dc_power+0xd0/0x134
Modules linked in: imx219 dw_csi dw_dphy
CPU: 0 PID: 230 Comm: cam Tainted: G W 5.16.0-rc4+ #63
Hardware name: Microchip SAMA7
[<c010b9dc>] (unwind_backtrace) from [<c010975c>] (show_stack+0x10/0x14)
[<c010975c>] (show_stack) from [<c0755aa0>] (__warn+0xd0/0x128)
[<c0755aa0>] (__warn) from [<c0755b70>] (warn_slowpath_fmt+0x78/0xac)
[<c0755b70>] (warn_slowpath_fmt) from [<c0501114>] (csi2dc_power+0xd0/0x134)
[<c0501114>] (csi2dc_power) from [<c04114b8>] (__rpm_callback+0x30/0xe4)
[<c04114b8>] (__rpm_callback) from [<c04115cc>] (rpm_callback+0x60/0x64)
[<c04115cc>] (rpm_callback) from [<c0411984>] (rpm_resume+0x3b4/0x550)
[<c0411984>] (rpm_resume) from [<c0411b54>] (__pm_runtime_resume+0x34/0x3c)
[<c0411b54>] (__pm_runtime_resume) from [<c0500dcc>]
(csi2dc_s_stream+0xb8/0x330)
[<c0500dcc>] (csi2dc_s_stream) from [<c04fee9c>]
(isc_start_streaming+0x3a4/0x498)
[<c04fee9c>] (isc_start_streaming) from [<c04f5280>]
(vb2_start_streaming+0x98/0x1a8)
[<c04f5280>] (vb2_start_streaming) from [<c04f57d0>]
(vb2_core_qbuf+0x440/0x5d4)
[<c04f57d0>] (vb2_core_qbuf) from [<c04f9490>] (vb2_qbuf+0x80/0xdc)
[<c04f9490>] (vb2_qbuf) from [<c04dd0dc>] (__video_do_ioctl+0x234/0x490)
[<c04dd0dc>] (__video_do_ioctl) from [<c04de410>]
(video_usercopy+0x1b4/0x550)
[<c04de410>] (video_usercopy) from [<c01e77b8>] (sys_ioctl+0x224/0xa0c)
[<c01e77b8>] (sys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x50)
Exception stack(0xc46e7fa8 to 0xc46e7ff0)
7fa0: b5d07088 b5d113c8 00000011 c044560f b66da594
b6e14000
7fc0: b5d07088 b5d113c8 b66da594 00000036 b66da5d8 fffffff8 b66da6cc
b66da8e0
7fe0: b6fb63a8 b66da57c b6f70d88 b6ba9fec
---[ end trace 7204e1871cae96e7 ]---
cam0dw-csi e1400000.csi2host: number of lanes: 2
phy phy-e1400040.dphy.0: PHY Stop state not reached 0
75.378082power off
------------[ cut here ]----e-------
4WARNING: CPU: 0 PID: 230 at
drivers/media/platform/atmel/microchip-csi2dc.c:335
csi2dc_runtime_suspend+0x34/0x6c
Modules linked in: imx219 dw_csi dw_dphy
CPU: 0 PID: 230 Comm: cam Tainted: G W 5.16.0-rc4+ #63
Hardware name: Microchip SAMA7
[<c010b9dc>] (unwind_backtrace) from [<c010975c>] (show_stack+0x10/0x14)
[<c010975c>] (show_stack) from [<c0755aa0>] (__warn+0xd0/0x128)
[<c0755aa0>] (__warn) from [<c0755b70>] (warn_slowpath_fmt+0x78/0xac)
[<c0755b70>] (warn_slowpath_fmt) from [<c075b4cc>]
(csi2dc_runtime_suspend+0x34/0x6c)
[<c075b4cc>] (csi2dc_runtime_suspend) from [<c04114b8>]
(__rpm_callback+0x30/0xe4)
[<c04114b8>] (__rpm_callback) from [<c04115cc>] (rpm_callback+0x60/0x64)
[<c04115cc>] (rpm_callback) from [<c0410e28>] (rpm_suspend+0xd0/0x4bc)
[<c0410e28>] (rpm_suspend) from [<c041136c>] (__pm_runtime_idle+0x3c/0x4c)
[<c041136c>] (__pm_runtime_idle) from [<c0500d98>]
(csi2dc_s_stream+0x84/0x330)
[<c0500d98>] (csi2dc_s_stream) from [<c04fcc7c>]
(isc_stop_streaming+0x134/0x174)
[<c04fcc7c>] (isc_stop_streaming) from [<c04f5dd8>]
(__vb2_queue_cancel+0x28/0x2a4)
[<c04f5dd8>] (__vb2_queue_cancel) from [<c04f606c>]
(vb2_core_streamoff+0x18/0xac)
[<c04f606c>] (vb2_core_streamoff) from [<c04dd0dc>]
(__video_do_ioctl+0x234/0x490)
[<c04dd0dc>] (__video_do_ioctl) from [<c04de410>]
(video_usercopy+0x1b4/0x550)
[<c04de410>] (video_usercopy) from [<c01e77b8>] (sys_ioctl+0x224/0xa0c)
[<c01e77b8>] (sys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x50)
Exception stack(0xc46e7fa8 to 0xc46e7ff0)
7fa0: b5d07088 b6fb5d58 00000011 40045613 b5d071bc
00000001
7fc0: b5d07088 b6fb5d58 b5d01e20 00000036 b5d008b8 00000000 b5d01e20
0054ec00
7fe0: b6fb63a8 b66da7b4 b6f70d88 b6ba9fec
---[ end trace 7204e1871cae96e8 ]---
#
#
Which is expected, on start streaming, the power on is called, and on
stop streaming the power off.
So it looks like the driver does pm_runtime_set_active, meaning the
power state is active (which is something that I do by calling first
csi2dc_power(true)), and after probing is done, it's automatically sent
to suspend mode.
So if I do csi2dc_power(false) before finish probing, there will be a
problem because it will be called again by the subsystem, leading to
issues with double clock unpreparing... etc.
So you are fine with this sequence?
..init this..
..init that..
csi2dc_power(true)
readl();
pm_runtime_set_active();
pm_runtime_enable();
..finish probing..
Thanks again,
Eugen
>>
>>>
>>>> +
>>>> + /*
>>>> + * we must register the subdev after PM runtime has been requested,
>>>> + * otherwise we might bound immediately and request pm_runtime_resume
>>>> + * before runtime_enable.
>>>> + */
>>>> + ret = v4l2_async_register_subdev(&csi2dc->csi2dc_sd);
>>>> + if (ret) {
>>>> + dev_err(csi2dc->dev, "failed to register the subdevice\n");
>>>> + goto csi2dc_probe_cleanup_notifier;
>>>> + }
>>>> +
>>>> + dev_info(dev, "Microchip CSI2DC version %x\n", ver);
>>>> +
>>>> + return 0;
>>>> +
>>>> +csi2dc_probe_cleanup_notifier:
>>>> + v4l2_async_nf_cleanup(&csi2dc->notifier);
>>>> +csi2dc_probe_cleanup_entity:
>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>>
>>> I wasn't honestly expecting this
>>>
>>> /**
>>> * media_entity_cleanup() - free resources associated with an entity
>>> *
>>> * @entity: entity where the pads belong
>>> *
>>> * This function must be called during the cleanup phase after unregistering
>>> * the entity (currently, it does nothing).
>>> */
>>> #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
>>> static inline void media_entity_cleanup(struct media_entity *entity) {}
>>>
>>> :)
>>
>> I saw it as well. But I thought that who knows, in the future it might
>> do something, so it's better to have it in the driver rather than not
>> have it at all.
>>
>>
>> Thanks again for reviewing,
>> Eugen
>>
>>>
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int csi2dc_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = platform_get_drvdata(pdev);
>>>> +
>>>> + pm_runtime_disable(&pdev->dev);
>>>> +
>>>> + v4l2_async_unregister_subdev(&csi2dc->csi2dc_sd);
>>>> + v4l2_async_nf_unregister(&csi2dc->notifier);
>>>> + v4l2_async_nf_cleanup(&csi2dc->notifier);
>>>> + media_entity_cleanup(&csi2dc->csi2dc_sd.entity);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused csi2dc_runtime_suspend(struct device *dev)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev);
>>>> +
>>>> + return csi2dc_power(csi2dc, false);
>>>> +}
>>>> +
>>>> +static int __maybe_unused csi2dc_runtime_resume(struct device *dev)
>>>> +{
>>>> + struct csi2dc_device *csi2dc = dev_get_drvdata(dev);
>>>> +
>>>> + return csi2dc_power(csi2dc, true);
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops csi2dc_dev_pm_ops = {
>>>> + SET_RUNTIME_PM_OPS(csi2dc_runtime_suspend, csi2dc_runtime_resume, NULL)
>>>> +};
>>>> +
>>>> +static const struct of_device_id csi2dc_of_match[] = {
>>>> + { .compatible = "microchip,sama7g5-csi2dc" },
>>>> + { }
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, csi2dc_of_match);
>>>> +
>>>> +static struct platform_driver csi2dc_driver = {
>>>> + .probe = csi2dc_probe,
>>>> + .remove = csi2dc_remove,
>>>> + .driver = {
>>>> + .name = "microchip-csi2dc",
>>>> + .pm = &csi2dc_dev_pm_ops,
>>>> + .of_match_table = of_match_ptr(csi2dc_of_match),
>>>> + },
>>>> +};
>>>> +
>>>> +module_platform_driver(csi2dc_driver);
>>>> +
>>>> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@microchip.com>");
>>>> +MODULE_DESCRIPTION("Microchip CSI2 Demux Controller driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> --
>>>> 2.25.1
>>>>
>>
next prev parent reply other threads:[~2021-12-10 11:07 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-12 14:24 [PATCH v2 00/25] media: atmel: atmel-isc: implement media controller Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 01/25] MAINTAINERS: add microchip csi2dc Eugen Hristev
2021-11-29 23:14 ` Laurent Pinchart
2021-12-13 13:40 ` Nicolas Ferre
2021-11-12 14:24 ` [PATCH v2 02/25] dt-bindings: media: atmel: csi2dc: add bindings for " Eugen Hristev
2021-11-29 21:10 ` Rob Herring
2021-11-29 21:37 ` Sakari Ailus
2021-12-06 8:57 ` Eugen.Hristev
2021-11-29 23:14 ` Laurent Pinchart
2021-11-12 14:24 ` [PATCH v2 03/25] media: atmel: introduce microchip csi2dc driver Eugen Hristev
2021-12-06 10:51 ` Jacopo Mondi
2021-12-06 11:42 ` Eugen.Hristev
2021-12-06 12:06 ` Jacopo Mondi
2021-12-10 11:07 ` Eugen.Hristev [this message]
2021-12-10 12:59 ` Jacopo Mondi
2021-11-12 14:24 ` [PATCH v2 04/25] MAINTAINERS: atmel-isc: add new file atmel-isc-clk.c Eugen Hristev
2021-11-29 23:18 ` Laurent Pinchart
2021-11-12 14:24 ` [PATCH v2 05/25] media: atmel: atmel-isc: split the clock code into separate source file Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 06/25] media: atmel: atmel-isc: replace video device name with module name Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 07/25] media: atmel: atmel-sama7g5-isc: fix ispck leftover Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 08/25] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 09/25] media: atmel: atmel-isc-base: remove frameintervals VIDIOC Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 10/25] media: atmel: atmel-isc-base: report frame sizes as full supported range Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 11/25] media: atmel: atmel-isc-base: implement mbus_code support in enumfmt Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 12/25] media: atmel: atmel-isc-base: fix bytesperline value for planar formats Eugen Hristev
2021-12-06 16:38 ` Jacopo Mondi
2021-12-07 11:21 ` Eugen.Hristev
2021-11-12 14:24 ` [PATCH v2 13/25] MAINTAINERS: atmel-isc: add new file atmel-isc-mc.c Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 14/25] media: atmel: atmel-isc: implement media controller Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 15/25] ARM: dts: at91: sama7g5: add nodes for video capture Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 16/25] ARM: configs: at91: sama7: add xisc and csi2dc Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 17/25] ARM: multi_v7_defconfig: add atmel video pipeline modules Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 18/25] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 19/25] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 20/25] media: atmel: atmel-isc-base: add wb debug messages Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 21/25] media: atmel: atmel-isc-base: clamp wb gain coefficients Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 22/25] media: atmel: atmel-sama7g5-isc: fix UYVY input format mbus_code typo Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 23/25] media: atmel: atmel-isc: add raw Bayer 8bit 10bit output formats Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 24/25] media: atmel: atmel-isc: compact the controller formats list Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 25/25] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3d5cf51e-3ea0-9fff-b9f8-cd0eadb4ea2b@microchip.com \
--to=eugen.hristev@microchip.com \
--cc=Nicolas.Ferre@microchip.com \
--cc=devicetree@vger.kernel.org \
--cc=jacopo@jmondi.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=sakari.ailus@iki.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).