[PATCH] rockchip: rk3328: Set VOP QoS to high priority

[Philipp Tomsich]

On Sat, 23 Jul 2022 at 13:29, Nicolas Frattaroli
<frattaroli.nicolas at gmail.com> wrote:
>
> The default priority for the quality of service for the video
> output results in unsightly glitches on the output whenever there
> is memory pressure on the system, which happens a lot.
>
> This sets the VOP QoS to high priority, which fixes this issue.
>
> Signed-off-by: Nicolas Frattaroli <frattaroli.nicolas at gmail.com>
> ---
>  arch/arm/mach-rockchip/rk3328/rk3328.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/mach-rockchip/rk3328/rk3328.c b/arch/arm/mach-rockchip/rk3328/rk3328.c
> index de17b88682..2373586b14 100644
> --- a/arch/arm/mach-rockchip/rk3328/rk3328.c
> +++ b/arch/arm/mach-rockchip/rk3328/rk3328.c
> @@ -19,6 +19,8 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define GRF_BASE               0xFF100000
>  #define UART2_BASE             0xFF130000
>  #define FW_DDR_CON_REG         0xFF7C0040
> +#define QOS_VOP_OFFSET         0xFF760080
> +#define QOS_VOP_PRIORITY       0x8
>
>  const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
>         [BROM_BOOTSOURCE_EMMC] = "/mmc at ff520000",
> @@ -54,6 +56,9 @@ int arch_cpu_init(void)
>
>         /* Disable the ddr secure region setting to make it non-secure */
>         rk_setreg(FW_DDR_CON_REG, 0x200);
> +#else
> +       printf("Setting VOP QoS\n");
> +       rk_setreg(QOS_VOP_OFFSET + QOS_VOP_PRIORITY, 0x2);

The change itself is easy enough to give a Reviewed-by on for (yes,
the address is correct, and 0x2 is a valid setting — please use a
symbolic constant for the 0x2, though), but...

Thinking about why you put this into '#else' had me question whether
this indeed belongs here...
This is part of the SoC init, as it sets up the arbitration in the
NOC: so at least whether arch_cpu_init is the right place to have it
is debatable (or if it should go into its own driver).

However, the overarching question is whether this should only be done
as part of the system configuration under the control of other drivers
(e.g., the Linux VOP driver) or under the control of the system
designer (i.e., based on device-tree or /proc entries in Linux).
Consider the case where someone is building a network/storage
appliance that uses VOP for console output: in such an application,
priority would be given to the peripherals critical to that
application instead of VOP.

So, I'd like to hear more discussion on whether this should be
unconditionally done in the bootloader before we move forward.

Thanks,
Philipp.

>  #endif
>         return 0;
>  }
> --
> 2.37.1
>