From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0F38EC433F5 for ; Wed, 17 Nov 2021 17:57:37 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C68EF61357 for ; Wed, 17 Nov 2021 17:57:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org C68EF61357 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ideasonboard.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=NNmvyeR8dl72Fm9D/9y/10xO0IJYnqRq7/h3iq64VjQ=; b=HCw9j6yYf95cX2 15vVhPDAbkxOCfbX/xer6q24TSi3MKOR4HEd7Hj3sbN/j6Xqtjqkwo04mHoyxomJlZPOGuA5I0w2Q +e3C31Nrv5mqKNMYYP+ONf6NDDNmbtmfwkh59/9PaZnx/K5WPa+NPk1d3QrWONPmx//sBHoKuATJq t4e8D9gIhgIl8paZskuKFrdAfp+qSzkHygC4yJG5K66gsVRp20pjILyGWW1ar+1AxZe6gyYFqgAty uGyQlbr6ZHJW8Gy4Mu/f11AnfHoIlfGwwPUuB21Fwa4WTvpQZYKYnQ42rEHieX2KpR82IhY8BA2bt 2anvEmPckYdwGNJESnUw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mnP9n-005yi4-H5; Wed, 17 Nov 2021 17:55:40 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mnOwM-005tqm-7C for linux-arm-kernel@lists.infradead.org; Wed, 17 Nov 2021 17:41:48 +0000 Received: from pendragon.ideasonboard.com (85-76-75-165-nat.elisa-mobile.fi [85.76.75.165]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 64C3BE7; Wed, 17 Nov 2021 18:41:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1637170904; bh=X+Ea3yq/ecIIxI90gZXX8XHXduCO/x1WBs+zNTf/gmk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=PBG3QFC/oRctznL+v/6SNxWNWNT3YPBc6BvsnCKILKTRt9uQwC1KFPnZDVdMLi6Wf QIEaxIWomS1qhAAB+p/M865yQ0zu2TcxBl0gn+7+JvPFLUvOS45RHdsd4zW62vyhf8 RCZRQNik7/L5o0wEzX+Ozan+MYPbEtZtBBGFCboo= Date: Wed, 17 Nov 2021 19:41:18 +0200 From: Laurent Pinchart To: Rui Miguel Silva Cc: Martin Kepplinger , mchehab@kernel.org, robh@kernel.org, shawnguo@kernel.org, kernel@pengutronix.de, kernel@puri.sm, linux-imx@nxp.com, linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq Message-ID: References: <20211117092710.3084034-1-martin.kepplinger@puri.sm> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211117_094146_488667_E390BD77 X-CRM114-Status: GOOD ( 44.45 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Rui, On Wed, Nov 17, 2021 at 05:36:49PM +0000, Rui Miguel Silva wrote: > On Wed Nov 17, 2021 at 5:16 PM WET, Laurent Pinchart wrote: > > > On Wed, Nov 17, 2021 at 02:51:48PM +0000, Rui Miguel Silva wrote: > > > Hi Martin, > > > Thanks for the patch. > > > > > > On Wed Nov 17, 2021 at 9:27 AM WET, Martin Kepplinger wrote: > > > > > > > Modeled after the NXP driver mx6s_capture.c that this driver is based on, > > > > imx8mq needs different settings for the baseaddr_switch mechanism. Define > > > > the needed bits and set that for imx8mq. > > > > > > > > Without these settings, the system will "sometimes" hang completely when > > > > starting to stream (the interrupt will never be called). > > > > Do we know why ? Are all the bits that you set required ? > > > > > > Signed-off-by: Martin Kepplinger > > > > --- > > > > drivers/staging/media/imx/imx7-media-csi.c | 34 ++++++++++++++++++++-- > > > > 1 file changed, 32 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > > > > index 2288dadb2683..8619cf2fc694 100644 > > > > --- a/drivers/staging/media/imx/imx7-media-csi.c > > > > +++ b/drivers/staging/media/imx/imx7-media-csi.c > > > > @@ -12,6 +12,7 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -122,6 +123,10 @@ > > > > #define BIT_DATA_FROM_MIPI BIT(22) > > > > #define BIT_MIPI_YU_SWAP BIT(21) > > > > #define BIT_MIPI_DOUBLE_CMPNT BIT(20) > > > > +#define BIT_MASK_OPTION_FIRST_FRAME (0 << 18) > > > > +#define BIT_MASK_OPTION_CSI_EN (1 << 18) > > > > +#define BIT_MASK_OPTION_SECOND_FRAME (2 << 18) > > > > +#define BIT_MASK_OPTION_ON_DATA (3 << 18) > > > > #define BIT_BASEADDR_CHG_ERR_EN BIT(9) > > > > #define BIT_BASEADDR_SWITCH_SEL BIT(5) > > > > #define BIT_BASEADDR_SWITCH_EN BIT(4) > > > > @@ -154,6 +159,12 @@ > > > > #define CSI_CSICR18 0x48 > > > > #define CSI_CSICR19 0x4c > > > > > > > > +enum imx_soc { > > > > + IMX6UL = 0, > > > > + IMX7, > > > > + IMX8MQ, > > > > > > maybe instead of this enum we could use a bool in structure... > > > > An enum would be more extensible, but we shouldn't define different > > values for IMX6UL and IMX7 if they're compatible. Maybe an enum > > imx_csi_model with two values (IMX_CSI_IMX7 and IMX_CSI_IMX8MQ ?). > > If there are only 2 possible values, for now, I think a enum would be > overkill. But do not have a strong feeling about it. So, an enum it is > more extensible and ok too, but with the IMX_CSI or even better > IMX7_CSI prefix. I won't push strongly in one direction or another, I just have a slight preference for an enum, as it integrates better with device match data. > > Are there other SoCs in the i.MX8 family that require this ? The BSP > > driver sets the baseaddr switch mechanism for i.MX8MM too, but it seems > > to work fine without it. > > > > > >+}; > > > > + > > > > struct imx7_csi { > > > > struct device *dev; > > > > struct v4l2_subdev sd; > > > > @@ -189,6 +200,8 @@ struct imx7_csi { > > > > bool is_csi2; > > > > > > > > struct completion last_eof_completion; > > > > + > > > > + enum imx_soc type; > > > > > > here, bool is_imx8mq? > > > > > > > }; > > > > > > > > static struct imx7_csi * > > > > @@ -537,6 +550,16 @@ static void imx7_csi_deinit(struct imx7_csi *csi, > > > > clk_disable_unprepare(csi->mclk); > > > > } > > > > > > > > +static void imx8mq_baseaddr_switch(struct imx7_csi *csi) > > > > > > I think this function needs a better name. First add the imx7_csi > > > prefix that all functions have, and also you are setting it specific > > > to second frame and the function should not be specific to imx8. > > > > > > maybe something: > > > > > > imx7_csi_write_on_second_frame_enable, maybe? > > > > > > > +{ > > > > + u32 cr18 = imx7_csi_reg_read(csi, CSI_CSICR18); > > > > + > > > > + cr18 |= BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL | > > > > + BIT_BASEADDR_CHG_ERR_EN; > > > > + cr18 |= BIT_MASK_OPTION_SECOND_FRAME; > > > > + imx7_csi_reg_write(csi, cr18, CSI_CSICR18); > > > > +} > > > > + > > > > static void imx7_csi_enable(struct imx7_csi *csi) > > > > { > > > > /* Clear the Rx FIFO and reflash the DMA controller. */ > > > > @@ -551,7 +574,11 @@ static void imx7_csi_enable(struct imx7_csi *csi) > > > > > > > > /* Enable the RxFIFO DMA and the CSI. */ > > > > imx7_csi_dmareq_rff_enable(csi); > > > > + > > > > > > unrelated new line. > > > > > > > imx7_csi_hw_enable(csi); > > > > + > > > > + if (csi->type == IMX8MQ) > > > > + imx8mq_baseaddr_switch(csi); > > > > > > change this to new types and names? > > > > > > > } > > > > > > > > static void imx7_csi_disable(struct imx7_csi *csi) > > > > @@ -1155,6 +1182,8 @@ static int imx7_csi_probe(struct platform_device *pdev) > > > > if (IS_ERR(csi->regbase)) > > > > return PTR_ERR(csi->regbase); > > > > > > > > + csi->type = (enum imx_soc)of_device_get_match_data(&pdev->dev); > > > > > > here something: > > > csi->is_imx8mq = of_device_is_compatible(np, "fsl,imx8mq-csi"); > > > > > > > + > > > > spin_lock_init(&csi->irqlock); > > > > mutex_init(&csi->lock); > > > > > > > > @@ -1249,8 +1278,9 @@ static int imx7_csi_remove(struct platform_device *pdev) > > > > } > > > > > > > > static const struct of_device_id imx7_csi_of_match[] = { > > > > - { .compatible = "fsl,imx7-csi" }, > > > > - { .compatible = "fsl,imx6ul-csi" }, > > > > + { .compatible = "fsl,imx8mq-csi", .data = (void *)IMX8MQ }, > > > > > > and with the above you should not need to add the data field here. > > > > I like match data personally (especially if we keep a device model > > enum). This is exactly what match data has been designed for, to avoid > > is_compatible() checks. > > agree. > > > > > + { .compatible = "fsl,imx7-csi", .data = (void *)IMX7 }, > > > > + { .compatible = "fsl,imx6ul-csi", .data = (void *)IMX6UL }, > > > > { }, > > > > }; > > > > MODULE_DEVICE_TABLE(of, imx7_csi_of_match); -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel