linux-kernel - Re: [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <CFS51AQQ7SCD.7FK8RLAWLXRH@arch-thunder>
Date:   Wed, 17 Nov 2021 14:51:48 +0000
From:   "Rui Miguel Silva" <rmfrfs@...il.com>
To:     "Martin Kepplinger" <martin.kepplinger@...i.sm>,
        <laurent.pinchart@...asonboard.com>, <mchehab@...nel.org>,
        <robh@...nel.org>, <shawnguo@...nel.org>
Cc:     <kernel@...gutronix.de>, <kernel@...i.sm>, <linux-imx@....com>,
        <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq

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).
>
> Signed-off-by: Martin Kepplinger <martin.kepplinger@...i.sm>
> ---
>  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 <linux/interrupt.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> @@ -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...
>
>+};
> +
>  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.

------
Cheers,
     Rui

> +	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7 },
> +	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX6UL },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
> -- 
> 2.30.2