Re: [RFC PATCH v1] media: rppx1: Update shadowed registers

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Barnabás,

Thanks for your patch.

On 2026-10-26 14:58:14 +0100, Barnabás Pőcze wrote:
> Certain registers are shadowed and changes to those must be explicly
> applied. Updating the shadowed registers after a frame is complete can
> be done by writing `RPPX1_RPP_HDR_UPD_REGS_GEN_CFG_UPD` to the
> `RPPX1_RPP_HDR_UPD_REG` register.
> 
> This is already done in `rppx1_start()`, but this auto-update feature
> is documented as "self reset at picture end". Therefore setting it
> at start will only auto-update the registers once, after the first
> frame.
> 
> For example, this causes that the LSC module cannot be toggled after
> the first frame. This behaviour can be tested by reading the first
> bit of `LSC_STATUS_REG`, which will show the same value after the
> first frame regardless of what was last written to `LSC_CTRL_REG`.
> 
> Fix this by unconditionall triggering an auto-update when a parameter
> buffer is processed in `rppx1_params()`.

Nice catch. Indeed all my CI test of the different modules setup one 
use-case and do not change any shadowed registers.

> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze+renesas@ideasonboard.com>
> ---
> 
> I made the minimal amount of changes to get the desired behaviour because
> I don't know how the maintainers would prefer the registers in headers.
> So I simply copied only a subset, but it feels a bit suboptimal like this,
> hence this is an RFC.

I prefers to have as small scope as possible for the defines, but of 
course it needs to be useful. As the scope prior to this change fit 
inside the .c file I kept it there.

Now it does not and I think it would make sens to move all of the 
register defines in rppx1.c to rppx1.h to keep them together. With that 
changed I think I'm happy with this change.

> 
> ---
>  .../platform/dreamchip/rppx1/rpp_params.c     |  3 +++
>  .../media/platform/dreamchip/rppx1/rppx1.c    | 23 -------------------
>  .../media/platform/dreamchip/rppx1/rppx1.h    | 21 +++++++++++++++++
>  3 files changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/platform/dreamchip/rppx1/rpp_params.c b/drivers/media/platform/dreamchip/rppx1/rpp_params.c
> index a75a27a8afd09..cbc9847ba5be6 100644
> --- a/drivers/media/platform/dreamchip/rppx1/rpp_params.c
> +++ b/drivers/media/platform/dreamchip/rppx1/rpp_params.c
> @@ -110,6 +110,9 @@ int rppx1_params(struct rppx1 *rpp, struct vb2_buffer *vb, size_t max_size,
>  		}
>  	}
>  
> +	/* Trigger update of shadowed registers after frame is done. */
> +	write(priv, RPPX1_RPP_HDR_UPD_REG, RPPX1_RPP_HDR_UPD_REGS_GEN_CFG_UPD);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(rppx1_params);
> diff --git a/drivers/media/platform/dreamchip/rppx1/rppx1.c b/drivers/media/platform/dreamchip/rppx1/rppx1.c
> index c502be7979146..4794672e088cf 100644
> --- a/drivers/media/platform/dreamchip/rppx1/rppx1.c
> +++ b/drivers/media/platform/dreamchip/rppx1/rppx1.c
> @@ -15,7 +15,6 @@
>  #include "rppx1.h"
>  
>  /* RPP_HDR Base Addresses */
> -#define RPPX1_HDRREGS_BASE			0x0000
>  #define RPPX1_HDR_IRQ_BASE			0x0200
>  #define RPPX1_RPP_OUT_BASE			0x0800
>  #define RPPX1_RPP_RMAP_BASE			0x0c00
> @@ -26,25 +25,6 @@
>  #define RPPX1_RPP_MVOUT_BASE			0xc000
>  #define RPPX1_RPP_FUSA_BASE			0xf000
>  
> -#define RPPX1_RPP_HDRREGS_VERSION_REG		(RPPX1_HDRREGS_BASE + 0x0000)
> -
> -#define RPPX1_RPP_HDR_UPD_REG			(RPPX1_HDRREGS_BASE + 0x0004)
> -#define RPPX1_RPP_HDR_UPD_REGS_GEN_CFG_UPD	BIT(1)
> -#define RPPX1_RPP_HDR_UPD_REGS_CFG_UPD		BIT(0)
> -
> -#define RPPX1_RESERVED_3_REG			(RPPX1_HDRREGS_BASE + 0x0008)
> -
> -#define RPPX1_RPP_HDR_INFORM_ENABLE_REG		(RPPX1_HDRREGS_BASE + 0x000c)
> -#define RPPX1_RPP_HDR_INFORM_ENABLE_ENABLE	1
> -#define RPPX1_RPP_HDR_INFORM_ENABLE_DISABLE	0
> -
> -#define RPPX1_RPP_HDR_OUT_IF_ON_REG		(RPPX1_HDRREGS_BASE + 0x0010)
> -#define RPPX1_RPP_HDR_OUT_IF_OFF_REG		(RPPX1_HDRREGS_BASE + 0x0014)
> -
> -#define RPPX1_RPP_HDR_SAFETY_ACCESS_PROTECTION_REG	(RPPX1_HDRREGS_BASE + 0x0018)
> -#define RPPX1_RPP_HDR_SAFETY_ACCESS_PROTECTION_ENABLE	1
> -#define RPPX1_RPP_HDR_SAFETY_ACCESS_PROTECTION_DISABLE	0
> -
>  #define RPPX1_RPP_ISM				(RPPX1_HDR_IRQ_BASE + 0x00)
>  #define RPPX1_RPP_RIS				(RPPX1_HDR_IRQ_BASE + 0x04)
>  #define RPPX1_RPP_MIS				(RPPX1_HDR_IRQ_BASE + 0x08)
> @@ -349,9 +329,6 @@ int rppx1_start(struct rppx1 *rpp,
>  	/* Immediate commit update for shadows. */
>  	rppx1_write(rpp, RPPX1_RPP_HDR_UPD_REG, RPPX1_RPP_HDR_UPD_REGS_CFG_UPD);
>  
> -	/* Then for operation update shadows with picture synchronization. */
> -	rppx1_write(rpp, RPPX1_RPP_HDR_UPD_REG, RPPX1_RPP_HDR_UPD_REGS_GEN_CFG_UPD);
> -
>  	/* Clear any pending interrupts. */
>  	rppx1_interrupt(rpp, NULL);
>  
> diff --git a/drivers/media/platform/dreamchip/rppx1/rppx1.h b/drivers/media/platform/dreamchip/rppx1/rppx1.h
> index 3bf3955afbaed..c6bd82f504560 100644
> --- a/drivers/media/platform/dreamchip/rppx1/rppx1.h
> +++ b/drivers/media/platform/dreamchip/rppx1/rppx1.h
> @@ -13,6 +13,27 @@
>  
>  #include "rpp_module.h"
>  
> +#define RPPX1_HDRREGS_BASE			0x0000
> +
> +#define RPPX1_RPP_HDRREGS_VERSION_REG		(RPPX1_HDRREGS_BASE + 0x0000)
> +
> +#define RPPX1_RPP_HDR_UPD_REG			(RPPX1_HDRREGS_BASE + 0x0004)
> +#define RPPX1_RPP_HDR_UPD_REGS_GEN_CFG_UPD	BIT(1)
> +#define RPPX1_RPP_HDR_UPD_REGS_CFG_UPD		BIT(0)
> +
> +#define RPPX1_RESERVED_3_REG			(RPPX1_HDRREGS_BASE + 0x0008)
> +
> +#define RPPX1_RPP_HDR_INFORM_ENABLE_REG		(RPPX1_HDRREGS_BASE + 0x000c)
> +#define RPPX1_RPP_HDR_INFORM_ENABLE_ENABLE	1
> +#define RPPX1_RPP_HDR_INFORM_ENABLE_DISABLE	0
> +
> +#define RPPX1_RPP_HDR_OUT_IF_ON_REG		(RPPX1_HDRREGS_BASE + 0x0010)
> +#define RPPX1_RPP_HDR_OUT_IF_OFF_REG		(RPPX1_HDRREGS_BASE + 0x0014)
> +
> +#define RPPX1_RPP_HDR_SAFETY_ACCESS_PROTECTION_REG	(RPPX1_HDRREGS_BASE + 0x0018)
> +#define RPPX1_RPP_HDR_SAFETY_ACCESS_PROTECTION_ENABLE	1
> +#define RPPX1_RPP_HDR_SAFETY_ACCESS_PROTECTION_DISABLE	0
> +
>  #define RPPX1_IRQ_ID_256HIST			BIT(27)
>  #define RPPX1_IRQ_ID_PRE2_DPCC			BIT(25)
>  #define RPPX1_IRQ_ID_PRE1_DPCC			BIT(24)
> -- 
> 2.56.0
> 

-- 
Kind Regards,
Niklas Söderlund