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:

quoted

On Wed, Nov 17, 2021 at 02:51:48PM +0000, Rui Miguel Silva wrote:

quoted

Hi Martin,
Thanks for the patch.

On Wed Nov 17, 2021 at 9:27 AM WET, Martin Kepplinger wrote:

quoted

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 ?

quoted

quoted

Signed-off-by: Martin Kepplinger <redacted>
---
 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.

quoted

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.

quoted

quoted

+};
+
 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?

quoted

 };
 
 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?

quoted

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

quoted

 	imx7_csi_hw_enable(csi);
+
+	if (csi->type == IMX8MQ)
+		imx8mq_baseaddr_switch(csi);

change this to new types and names?

quoted

 }
 
 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");

quoted

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

quoted

quoted

quoted

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