On Samstag, 23. Juli 2022 13:28:36 CEST Nicolas Frattaroli 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@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@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);

#endif return 0; }

Hello,

could I get a review on this?

Thank you for your time, Nicolas Frattaroli

On Sat, 23 Jul 2022 at 13:29, Nicolas Frattaroli frattaroli.nicolas@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@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@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