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

[Michael Tretter <m.tretter@pengutronix.de>]

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