diff mbox series

[4/8] media: imx-pxp: explicitly disable unused blocks

Message ID 20230105134729.59542-5-m.tretter@pengutronix.de
State Superseded
Headers show
Series [1/8] media: dt-bindings: media: fsl-pxp: convert to yaml | expand

Commit Message

Michael Tretter Jan. 5, 2023, 1:47 p.m. UTC
Various multiplexers in the pipeline are not used with the currently
configured data path. Disable all unused multiplexers by selecting the
"no output" (3) option.

The datasheet doesn't explicitly require this, but the PXP has been seen
to hang after processing a few hundreds of frames otherwise.

As at it, add documentation for the multiplexers that are actually
relevant for the data path.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/media/platform/nxp/imx-pxp.c | 30 +++++++++++++++++-----------
 1 file changed, 18 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart Jan. 6, 2023, 12:26 p.m. UTC | #1
Hi Michael,

Thank you for the patch.

On Thu, Jan 05, 2023 at 02:47:25PM +0100, Michael Tretter wrote:
> Various multiplexers in the pipeline are not used with the currently
> configured data path. Disable all unused multiplexers by selecting the
> "no output" (3) option.
> 
> The datasheet doesn't explicitly require this, but the PXP has been seen
> to hang after processing a few hundreds of frames otherwise.

On which platform(s) have you noticed that ?

> As at it, add documentation for the multiplexers that are actually
> relevant for the data path.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/media/platform/nxp/imx-pxp.c | 30 +++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> index a957fee88829..6ffd07cda965 100644
> --- a/drivers/media/platform/nxp/imx-pxp.c
> +++ b/drivers/media/platform/nxp/imx-pxp.c
> @@ -731,22 +731,28 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
>  	u32 ctrl0;
>  
>  	ctrl0 = 0;
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
> +	/* Bypass Dithering x3CH */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
> +	/* Select Rotation */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
> +	/* Select LUT */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
> +	/* Select MUX8 for LUT */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
> +	/* Select CSC 2 */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
> +	/* Bypass Rotation 2 */
>  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
> -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
> +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);

The muxes being disabled look fine to me, but the values of MUX8, MUX12
and MUX14 look strange based on the i.MX7D reference manual. Maybe the
register values were different in previous SoCs ? I haven't found any
other relevant reference manual that document the mux values, I may have
overlooked something.

Anyway, this isn't an issue with this patch, so

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

>  
>  	return ctrl0;
>  }
> @@ -760,8 +766,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
>  	ctrl0 = pxp_data_path_ctrl0(ctx);
>  
>  	ctrl1 = 0;
> -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
> -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
> +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
> +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
>  
>  	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
>  	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
Michael Tretter Jan. 6, 2023, 2:08 p.m. UTC | #2
On Fri, 06 Jan 2023 14:26:40 +0200, Laurent Pinchart wrote:
> On Thu, Jan 05, 2023 at 02:47:25PM +0100, Michael Tretter wrote:
> > Various multiplexers in the pipeline are not used with the currently
> > configured data path. Disable all unused multiplexers by selecting the
> > "no output" (3) option.
> > 
> > The datasheet doesn't explicitly require this, but the PXP has been seen
> > to hang after processing a few hundreds of frames otherwise.
> 
> On which platform(s) have you noticed that ?

I didn't observe this myself, but took this information from the comment in
your earlier patch [0] that disables the unused multiplexers.

https://lore.kernel.org/linux-media/20200510223100.11641-2-laurent.pinchart@ideasonboard.com/

> 
> > As at it, add documentation for the multiplexers that are actually
> > relevant for the data path.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/media/platform/nxp/imx-pxp.c | 30 +++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > index a957fee88829..6ffd07cda965 100644
> > --- a/drivers/media/platform/nxp/imx-pxp.c
> > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > @@ -731,22 +731,28 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> >  	u32 ctrl0;
> >  
> >  	ctrl0 = 0;
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
> > +	/* Bypass Dithering x3CH */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
> > +	/* Select Rotation */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
> > +	/* Select LUT */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
> > +	/* Select MUX8 for LUT */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
> > +	/* Select CSC 2 */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
> > +	/* Bypass Rotation 2 */
> >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
> > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
> > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);
> 
> The muxes being disabled look fine to me, but the values of MUX8, MUX12
> and MUX14 look strange based on the i.MX7D reference manual. Maybe the
> register values were different in previous SoCs ? I haven't found any
> other relevant reference manual that document the mux values, I may have
> overlooked something.

The MUX8, MUX12 and MUX14 are documented in the i.MX6ULL reference manual
section 41.11.51 and their location and function in the data path is shown in
Figure 41-1. "PXP Architecture" on page 2490.

Michael

> 
> Anyway, this isn't an issue with this patch, so
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  
> >  	return ctrl0;
> >  }
> > @@ -760,8 +766,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
> >  	ctrl0 = pxp_data_path_ctrl0(ctx);
> >  
> >  	ctrl1 = 0;
> > -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
> > -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
> > +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
> > +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
> >  
> >  	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> >  	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
Laurent Pinchart Jan. 6, 2023, 6:39 p.m. UTC | #3
Hi Michael,

On Fri, Jan 06, 2023 at 03:08:55PM +0100, Michael Tretter wrote:
> On Fri, 06 Jan 2023 14:26:40 +0200, Laurent Pinchart wrote:
> > On Thu, Jan 05, 2023 at 02:47:25PM +0100, Michael Tretter wrote:
> > > Various multiplexers in the pipeline are not used with the currently
> > > configured data path. Disable all unused multiplexers by selecting the
> > > "no output" (3) option.
> > > 
> > > The datasheet doesn't explicitly require this, but the PXP has been seen
> > > to hang after processing a few hundreds of frames otherwise.
> > 
> > On which platform(s) have you noticed that ?
> 
> I didn't observe this myself, but took this information from the comment in
> your earlier patch [0] that disables the unused multiplexers.
> 
> https://lore.kernel.org/linux-media/20200510223100.11641-2-laurent.pinchart@ideasonboard.com/

I suppose I should trust myself :-)

> > > As at it, add documentation for the multiplexers that are actually
> > > relevant for the data path.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  drivers/media/platform/nxp/imx-pxp.c | 30 +++++++++++++++++-----------
> > >  1 file changed, 18 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > > index a957fee88829..6ffd07cda965 100644
> > > --- a/drivers/media/platform/nxp/imx-pxp.c
> > > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > > @@ -731,22 +731,28 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
> > >  	u32 ctrl0;
> > >  
> > >  	ctrl0 = 0;
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
> > > +	/* Bypass Dithering x3CH */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
> > > +	/* Select Rotation */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
> > > +	/* Select LUT */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
> > > +	/* Select MUX8 for LUT */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
> > > +	/* Select CSC 2 */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
> > > +	/* Bypass Rotation 2 */
> > >  	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
> > > -	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
> > > +	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);
> > 
> > The muxes being disabled look fine to me, but the values of MUX8, MUX12
> > and MUX14 look strange based on the i.MX7D reference manual. Maybe the
> > register values were different in previous SoCs ? I haven't found any
> > other relevant reference manual that document the mux values, I may have
> > overlooked something.
> 
> The MUX8, MUX12 and MUX14 are documented in the i.MX6ULL reference manual
> section 41.11.51 and their location and function in the data path is shown in
> Figure 41-1. "PXP Architecture" on page 2490.

I've seen that, but as far as I can tell, the description of the
register in section 41.11.51 doesn't tell what the different MUX* field
value map to. Figure 41-1 shows that, for instance, MUX8 handles CSC2
bypass, but it doesn't tell what value corresponds to what path.

> > Anyway, this isn't an issue with this patch, so
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > >  
> > >  	return ctrl0;
> > >  }
> > > @@ -760,8 +766,8 @@ static void pxp_set_data_path(struct pxp_ctx *ctx)
> > >  	ctrl0 = pxp_data_path_ctrl0(ctx);
> > >  
> > >  	ctrl1 = 0;
> > > -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
> > > -	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
> > > +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
> > > +	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
> > >  
> > >  	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
> > >  	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);
diff mbox series

Patch

diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
index a957fee88829..6ffd07cda965 100644
--- a/drivers/media/platform/nxp/imx-pxp.c
+++ b/drivers/media/platform/nxp/imx-pxp.c
@@ -731,22 +731,28 @@  static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx)
 	u32 ctrl0;
 
 	ctrl0 = 0;
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX15_SEL(3);
+	/* Bypass Dithering x3CH */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX14_SEL(1);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX13_SEL(3);
+	/* Select Rotation */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX12_SEL(0);
+	/* Select LUT */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX11_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX10_SEL(3);
+	/* Select MUX8 for LUT */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX9_SEL(1);
+	/* Select CSC 2 */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX8_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX7_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX6_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX5_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX4_SEL(3);
+	/* Bypass Rotation 2 */
 	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX3_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(0);
-	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(0);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX2_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX1_SEL(3);
+	ctrl0 |= BF_PXP_DATA_PATH_CTRL0_MUX0_SEL(3);
 
 	return ctrl0;
 }
@@ -760,8 +766,8 @@  static void pxp_set_data_path(struct pxp_ctx *ctx)
 	ctrl0 = pxp_data_path_ctrl0(ctx);
 
 	ctrl1 = 0;
-	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(1);
-	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(1);
+	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX17_SEL(3);
+	ctrl1 |= BF_PXP_DATA_PATH_CTRL1_MUX16_SEL(3);
 
 	writel(ctrl0, dev->mmio + HW_PXP_DATA_PATH_CTRL0);
 	writel(ctrl1, dev->mmio + HW_PXP_DATA_PATH_CTRL1);