* [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq @ 2021-11-17 9:27 Martin Kepplinger 2021-11-17 9:27 ` [PATCH 2/2] dt-bindings: media: document imx8mq support for imx7-csi Martin Kepplinger 2021-11-17 14:51 ` [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq Rui Miguel Silva 0 siblings, 2 replies; 8+ messages in thread From: Martin Kepplinger @ 2021-11-17 9:27 UTC (permalink / raw) To: rmfrfs, laurent.pinchart, mchehab, robh, shawnguo Cc: kernel, kernel, linux-imx, linux-media, devicetree, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] dt-bindings: media: document imx8mq support for imx7-csi 2021-11-17 9:27 [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq Martin Kepplinger @ 2021-11-17 9:27 ` Martin Kepplinger 2021-11-17 17:19 ` Laurent Pinchart 2021-11-17 14:51 ` [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq Rui Miguel Silva 1 sibling, 1 reply; 8+ messages in thread From: Martin Kepplinger @ 2021-11-17 9:27 UTC (permalink / raw) To: rmfrfs, laurent.pinchart, mchehab, robh, shawnguo Cc: kernel, kernel, linux-imx, linux-media, devicetree, linux-arm-kernel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] dt-bindings: media: document imx8mq support for imx7-csi 2021-11-17 9:27 ` [PATCH 2/2] dt-bindings: media: document imx8mq support for imx7-csi Martin Kepplinger @ 2021-11-17 17:19 ` Laurent Pinchart 0 siblings, 0 replies; 8+ messages in thread From: Laurent Pinchart @ 2021-11-17 17:19 UTC (permalink / raw) To: Martin Kepplinger Cc: rmfrfs, mchehab, robh, shawnguo, kernel, kernel, linux-imx, linux-media, devicetree, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq 2021-11-17 9:27 [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq Martin Kepplinger 2021-11-17 9:27 ` [PATCH 2/2] dt-bindings: media: document imx8mq support for imx7-csi Martin Kepplinger @ 2021-11-17 14:51 ` Rui Miguel Silva 2021-11-17 17:16 ` Laurent Pinchart 1 sibling, 1 reply; 8+ messages in thread From: Rui Miguel Silva @ 2021-11-17 14:51 UTC (permalink / raw) To: Martin Kepplinger, laurent.pinchart, mchehab, robh, shawnguo Cc: kernel, kernel, linux-imx, linux-media, devicetree, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq 2021-11-17 14:51 ` [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq Rui Miguel Silva @ 2021-11-17 17:16 ` Laurent Pinchart 2021-11-17 17:36 ` Rui Miguel Silva 2021-11-17 17:41 ` Martin Kepplinger 0 siblings, 2 replies; 8+ messages in thread From: Laurent Pinchart @ 2021-11-17 17:16 UTC (permalink / raw) To: Rui Miguel Silva Cc: Martin Kepplinger, mchehab, robh, shawnguo, kernel, kernel, linux-imx, linux-media, devicetree, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq 2021-11-17 17:16 ` Laurent Pinchart @ 2021-11-17 17:36 ` Rui Miguel Silva 2021-11-17 17:41 ` Laurent Pinchart 2021-11-17 17:41 ` Martin Kepplinger 1 sibling, 1 reply; 8+ messages in thread From: Rui Miguel Silva @ 2021-11-17 17:36 UTC (permalink / raw) To: Laurent Pinchart Cc: Martin Kepplinger, mchehab, robh, shawnguo, kernel, kernel, linux-imx, linux-media, devicetree, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq 2021-11-17 17:36 ` Rui Miguel Silva @ 2021-11-17 17:41 ` Laurent Pinchart 0 siblings, 0 replies; 8+ messages in thread From: Laurent Pinchart @ 2021-11-17 17:41 UTC (permalink / raw) To: Rui Miguel Silva Cc: Martin Kepplinger, mchehab, robh, shawnguo, kernel, kernel, linux-imx, linux-media, devicetree, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq 2021-11-17 17:16 ` Laurent Pinchart 2021-11-17 17:36 ` Rui Miguel Silva @ 2021-11-17 17:41 ` Martin Kepplinger 1 sibling, 0 replies; 8+ messages in thread From: Martin Kepplinger @ 2021-11-17 17:41 UTC (permalink / raw) To: Laurent Pinchart, Rui Miguel Silva Cc: mchehab, robh, shawnguo, kernel, kernel, linux-imx, linux-media, devicetree, linux-arm-kernel, linux-kernel 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); > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-11-17 17:42 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-17 9:27 [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq Martin Kepplinger 2021-11-17 9:27 ` [PATCH 2/2] dt-bindings: media: document imx8mq support for imx7-csi Martin Kepplinger 2021-11-17 17:19 ` Laurent Pinchart 2021-11-17 14:51 ` [PATCH 1/2] media: imx: imx7-media-csi: add support for imx8mq Rui Miguel Silva 2021-11-17 17:16 ` Laurent Pinchart 2021-11-17 17:36 ` Rui Miguel Silva 2021-11-17 17:41 ` Laurent Pinchart 2021-11-17 17:41 ` Martin Kepplinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).