From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from comms.puri.sm (comms.puri.sm [159.203.221.185]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B226D72 for ; Thu, 15 Jul 2021 06:50:07 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by comms.puri.sm (Postfix) with ESMTP id DFDE6DFE44; Wed, 14 Jul 2021 23:50:00 -0700 (PDT) Received: from comms.puri.sm ([127.0.0.1]) by localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id EVlryjTOvjER; Wed, 14 Jul 2021 23:49:56 -0700 (PDT) Message-ID: <33f9ab8ea253c01d3311346bc871d7f62213215f.camel@puri.sm> Subject: Re: [PATCH v6 2/3] media: imx: add a driver for i.MX8MQ mipi csi rx phy and controller From: Martin Kepplinger To: Laurent Pinchart Cc: festevam@gmail.com, krzk@kernel.org, devicetree@vger.kernel.org, kernel@pengutronix.de, kernel@puri.sm, linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, m.felsch@pengutronix.de, mchehab@kernel.org, phone-devel@vger.kernel.org, robh@kernel.org, shawnguo@kernel.org, slongerbeam@gmail.com Date: Thu, 15 Jul 2021 08:49:51 +0200 In-Reply-To: References: <20210714111931.324485-1-martin.kepplinger@puri.sm> <20210714111931.324485-3-martin.kepplinger@puri.sm> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.3-1 Content-Transfer-Encoding: 8bit Precedence: bulk X-Mailing-List: linux-staging@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Am Mittwoch, dem 14.07.2021 um 21:24 +0300 schrieb Laurent Pinchart: > Hi Martin, > > Thank you for the patch. thank you for reviewing. > > On Wed, Jul 14, 2021 at 01:19:30PM +0200, Martin Kepplinger wrote: > > Add a driver to support the i.MX8MQ MIPI CSI receiver. The hardware > > side > > is based on > > > > https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c?h=imx_5.4.70_2.3.0 > > > > It's built as part of VIDEO_IMX7_CSI because that's documented to > > support > > i.MX8M platforms. This driver adds i.MX8MQ support where currently > > only the > > i.MX8MM platform has been supported. > > > > Signed-off-by: Martin Kepplinger > > --- > >  drivers/staging/media/imx/Makefile           |   1 + > >  drivers/staging/media/imx/imx8mq-mipi-csi2.c | 949 > > +++++++++++++++++++ > >  2 files changed, 950 insertions(+) > >  create mode 100644 drivers/staging/media/imx/imx8mq-mipi-csi2.c > > > > diff --git a/drivers/staging/media/imx/Makefile > > b/drivers/staging/media/imx/Makefile > > index 6ac33275cc97..19c2fc54d424 100644 > > --- a/drivers/staging/media/imx/Makefile > > +++ b/drivers/staging/media/imx/Makefile > > @@ -16,3 +16,4 @@ obj-$(CONFIG_VIDEO_IMX_CSI) += imx6-mipi-csi2.o > >   > >  obj-$(CONFIG_VIDEO_IMX7_CSI) += imx7-media-csi.o > >  obj-$(CONFIG_VIDEO_IMX7_CSI) += imx7-mipi-csis.o > > +obj-$(CONFIG_VIDEO_IMX7_CSI) += imx8mq-mipi-csi2.o > > diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c > > b/drivers/staging/media/imx/imx8mq-mipi-csi2.c > > new file mode 100644 > > index 000000000000..949b3ef7a20a > > --- /dev/null > > +++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c > > @@ -0,0 +1,949 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Freescale i.MX8MQ SoC series MIPI-CSI2 receiver driver > > Maybe they should be called NXP these days :-) > > > + * > > + * Copyright (C) 2021 Purism SPC > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define MIPI_CSI2_DRIVER_NAME                  "imx8mq-mipi-csi2" > > +#define > > MIPI_CSI2_SUBDEV_NAME                  MIPI_CSI2_DRIVER_NAME > > + > > +#define MIPI_CSI2_PAD_SINK                     0 > > +#define MIPI_CSI2_PAD_SOURCE                   1 > > +#define MIPI_CSI2_PADS_NUM                     2 > > + > > +#define MIPI_CSI2_DEF_PIX_WIDTH                        640 > > +#define MIPI_CSI2_DEF_PIX_HEIGHT               480 > > + > > +/* Register map definition */ > > + > > +/* i.MX8MQ CSI-2 controller CSR */ > > +#define CSI2RX_CFG_NUM_LANES                   0x100 > > +#define CSI2RX_CFG_DISABLE_DATA_LANES          0x104 > > +#define CSI2RX_BIT_ERR                         0x108 > > +#define CSI2RX_IRQ_STATUS                      0x10c > > +#define CSI2RX_IRQ_MASK                                0x110 > > +#define CSI2RX_IRQ_MASK_ALL                    0x1ff > > +#define CSI2RX_IRQ_MASK_ULPS_STATUS_CHANGE     0x8 > > +#define CSI2RX_ULPS_STATUS                     0x114 > > +#define CSI2RX_PPI_ERRSOT_HS                   0x118 > > +#define CSI2RX_PPI_ERRSOTSYNC_HS               0x11c > > +#define CSI2RX_PPI_ERRESC                      0x120 > > +#define CSI2RX_PPI_ERRSYNCESC                  0x124 > > +#define CSI2RX_PPI_ERRCONTROL                  0x128 > > +#define CSI2RX_CFG_DISABLE_PAYLOAD_0           0x12c > > +#define CSI2RX_CFG_VID_P_FIFO_SEND_LEVEL       0x188 > > +#define CSI2RX_CFG_DISABLE_PAYLOAD_1           0x130 > > + > > +enum { > > +       ST_POWERED      = 1, > > +       ST_STREAMING    = 2, > > +       ST_SUSPENDED    = 4, > > +}; > > + > > +static const char * const imx8mq_mipi_csi_clk_id[] = { > > +       "core", > > +       "esc", > > +       "ui", > > +}; > > + > > +#define CSI2_NUM_CLKS  ARRAY_SIZE(imx8mq_mipi_csi_clk_id) > > + > > +#define        GPR_CSI2_1_RX_ENABLE            BIT(13) > > +#define        GPR_CSI2_1_VID_INTFC_ENB        BIT(12) > > +#define        GPR_CSI2_1_HSEL                 BIT(10) > > +#define        GPR_CSI2_1_CONT_CLK_MODE        BIT(8) > > +#define        GPR_CSI2_1_S_PRG_RXHS_SETTLE(x) (((x) & 0x3f) << 2) > > + > > +/* > > + * The send level configures the number of entries that must > > accumulate in > > + * the Pixel FIFO before the data will be transferred to the video > > output. > > + * See > > https://community.nxp.com/t5/i-MX-Processors/IMX8M-MIPI-CSI-Host-Controller-send-level/m-p/864005/highlight/true#M131704 > > + */ > > +#define CSI2RX_SEND_LEVEL                      64 > > + > > +struct csi_state { > > +       struct device *dev; > > +       void __iomem *regs; > > +       struct clk_bulk_data clks[CSI2_NUM_CLKS]; > > +       struct reset_control *rst; > > +       struct regulator *mipi_phy_regulator; > > + > > +       struct v4l2_subdev sd; > > +       struct media_pad pads[MIPI_CSI2_PADS_NUM]; > > +       struct v4l2_async_notifier notifier; > > +       struct v4l2_subdev *src_sd; > > + > > +       struct v4l2_fwnode_bus_mipi_csi2 bus; > > + > > +       struct mutex lock; /* Protect csi2_fmt, format_mbus, state, > > hs_settle*/ > > Missing space before */ > > > +       const struct csi2_pix_format *csi2_fmt; > > +       struct v4l2_mbus_framefmt format_mbus[MIPI_CSI2_PADS_NUM]; > > +       u32 state; > > +       u32 hs_settle; > > + > > +       struct regmap *phy_gpr; > > +       u8 phy_gpr_reg; > > + > > +       struct icc_path                 *icc_path; > > +       s32                             icc_path_bw; > > +}; > > + > > +/* --------------------------------------------------------------- > > -------------- > > + * Format helpers > > + */ > > + > > +struct csi2_pix_format { > > +       u32 code; > > +       u8 width; > > +}; > > + > > +static const struct csi2_pix_format imx8mq_mipi_csi_formats[] = { > > +       /* RAW (Bayer and greyscale) formats. */ > > +       { > > +               .code = MEDIA_BUS_FMT_SBGGR8_1X8, > > +               .width = 8, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SGBRG8_1X8, > > +               .width = 8, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SGRBG8_1X8, > > +               .width = 8, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SRGGB8_1X8, > > +               .width = 8, > > +       }, { > > +               .code = MEDIA_BUS_FMT_Y8_1X8, > > +               .width = 8, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SBGGR10_1X10, > > +               .width = 10, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SGBRG10_1X10, > > +               .width = 10, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SGRBG10_1X10, > > +               .width = 10, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SRGGB10_1X10, > > +               .width = 10, > > +       }, { > > +               .code = MEDIA_BUS_FMT_Y10_1X10, > > +               .width = 10, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SBGGR12_1X12, > > +               .width = 12, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SGBRG12_1X12, > > +               .width = 12, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SGRBG12_1X12, > > +               .width = 12, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SRGGB12_1X12, > > +               .width = 12, > > +       }, { > > +               .code = MEDIA_BUS_FMT_Y12_1X12, > > +               .width = 12, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SBGGR14_1X14, > > +               .width = 14, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SGBRG14_1X14, > > +               .width = 14, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SGRBG14_1X14, > > +               .width = 14, > > +       }, { > > +               .code = MEDIA_BUS_FMT_SRGGB14_1X14, > > +               .width = 14, > > +       }, { > > +       /* YUV formats */ > > +               .code = MEDIA_BUS_FMT_YUYV8_2X8, > > +               .width = 16, > > +       }, { > > +               .code = MEDIA_BUS_FMT_YUYV8_1X16, > > +               .width = 16, > > +       } > > +}; > > + > > +static const struct csi2_pix_format *find_csi2_format(u32 code) > > +{ > > +       unsigned int i; > > + > > +       for (i = 0; i < ARRAY_SIZE(imx8mq_mipi_csi_formats); i++) > > +               if (code == imx8mq_mipi_csi_formats[i].code) > > +                       return &imx8mq_mipi_csi_formats[i]; > > +       return NULL; > > +} > > + > > +/* --------------------------------------------------------------- > > -------------- > > + * Hardware configuration > > + */ > > + > > +static inline void imx8mq_mipi_csi_write(struct csi_state *state, > > u32 reg, u32 val) > > +{ > > +       writel(val, state->regs + reg); > > +} > > + > > +static int imx8mq_mipi_csi_sw_reset(struct csi_state *state) > > +{ > > +       int ret; > > + > > +       ret = reset_control_assert(state->rst); > > That's peculiar, is there no need to deassert reset ? I tried different things here that would look more intuitive, but in the end only this worked, which is directly taken from https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c?h=imx_5.4.70_2.3.0#n105 (actual register value read from DT) that results in exactly the same register bits set like this assertation.