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

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

[Martin Kepplinger]

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@puri.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,
+};
+
 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;
 };
 
 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)
+{
+	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);
+
 	imx7_csi_hw_enable(csi);
+
+	if (csi->type == IMX8MQ)
+		imx8mq_baseaddr_switch(csi);
 }
 
 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);
+
 	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 },
+	{ .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

[PATCH 2/2] dt-bindings: media: document imx8mq support for imx7-csi

[Martin Kepplinger]

Add the fsl,imx8mq-csi compatible string to the bindings for nxp,imx7-csi
since the driver explicitly handles that now.

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
index 5922a2795167..4f7b78265336 100644
--- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
+++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
@@ -17,6 +17,7 @@ properties:
   compatible:
     oneOf:
       - enum:
+          - fsl,imx8mq-csi
           - fsl,imx7-csi
           - fsl,imx6ul-csi
       - items:
-- 
2.30.2

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

[Rui Miguel Silva]

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@puri.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

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

[Laurent Pinchart]

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 <martin.kepplinger@puri.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...

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 ?).

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.

> > +	{ .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

Re: [PATCH 2/2] dt-bindings: media: document imx8mq support for imx7-csi

[Laurent Pinchart]

Hi Martin,

Thank you for the patch.

On Wed, Nov 17, 2021 at 10:27:10AM +0100, Martin Kepplinger wrote:
> Add the fsl,imx8mq-csi compatible string to the bindings for nxp,imx7-csi
> since the driver explicitly handles that now.

The commit message should describe why a different compatible string is
needed, without mentioning the driver, as DT bindings are not
driver-dependent.

With that fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> ---
>  Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> index 5922a2795167..4f7b78265336 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> @@ -17,6 +17,7 @@ properties:
>    compatible:
>      oneOf:
>        - enum:
> +          - fsl,imx8mq-csi
>            - fsl,imx7-csi
>            - fsl,imx6ul-csi
>        - items:

-- 
Regards,

Laurent Pinchart

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

[Rui Miguel Silva]

Hi Laurent,
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 <martin.kepplinger@puri.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...
>
> 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.

>
> 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.

Many thanks Laurent,
------
Cheers,
     Rui
>
> > > +	{ .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

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

[Laurent Pinchart]

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 <martin.kepplinger@puri.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...
> >
> > 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

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

[Martin Kepplinger]

Am Mittwoch, dem 17.11.2021 um 19:16 +0200 schrieb Laurent Pinchart:
> 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 ?

tbh I don't know much more details about why. I debugged this quite a
while and yes, from what I saw I need all these bits set.

rx fifo overflow should be taken care of by the underrun buffer, but
since imx8mq has an erratum that hangs the system in such case, see
below, can it in any way be one reason for this to be needed?

https://community.nxp.com/t5/i-MX-Processors/IMX8MQ-MIPI-CSI2-Base-address-switching-change-error/m-p/1216509/highlight/true#M167970

intuitively I'd say it's unrelated though.

> 
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.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...
> 
> 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 ?).
> 
> 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.

I'm looking at
https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/media/platform/mxc/capture/mx6s_capture.c?h=imx_5.4.70_2.3.0
for the comparison that is not explicitly targeting imx8mm, right?

Anyway it looks like we need these bits on imx8mq only. Sorry, but
maybe somebody from NXP could tell us more about the reasons?

thanks a lot for reviewing, I'll queue a v2.

> 
> > > +};
> > > +
> > >  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.
> 
> > > +       { .compatible = "fsl,imx7-csi", .data = (void *)IMX7 },
> > > +       { .compatible = "fsl,imx6ul-csi", .data = (void *)IMX6UL
> > > },
> > >         { },
> > >  };
> > >  MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
>