[U-Boot] [PATCH v2] miniarm-rk3288: set isp/vop qos priority level

[Simon Glass]

Hi,

On 15 December 2016 at 00:18, Nickey.Yang <nickey.yang at rock-chips.com> wrote:
> Hi Kever & Simon,
>
>
>
> 在 2016年12月14日 15:07, Kever Yang 写道:
>>
>> Hi Simon,
>>
>> On 12/12/2016 04:27 AM, Simon Glass wrote:
>>>
>>> Hi Nickey,
>>>
>>> On 8 December 2016 at 21:39, Nickey Yang <nickey.yang at rock-chips.com>
>>> wrote:
>>>>
>>>> isp-camera image will be broken when enter dual screen display mode.
>>>> We set isp qos high to solve this problem.
>>>>
>>>> Signed-off-by: Nickey Yang <nickey.yang at rock-chips.com>
>>>> ---
>>>>   arch/arm/include/asm/arch-rockchip/qos_rk3288.h | 21
>>>> +++++++++++++++++++++
>>>>   board/rockchip/miniarm_rk3288/miniarm-rk3288.c  | 21
>>>> +++++++++++++++++++++
>>>>   2 files changed, 42 insertions(+)
>>>>   create mode 100644 arch/arm/include/asm/arch-rockchip/qos_rk3288.h
>>>>
>>>> diff --git a/arch/arm/include/asm/arch-rockchip/qos_rk3288.h
>>>> b/arch/arm/include/asm/arch-rockchip/qos_rk3288.h
>>>> new file mode 100644
>>>> index 0000000..d3d6c3e
>>>> --- /dev/null
>>>> +++ b/arch/arm/include/asm/arch-rockchip/qos_rk3288.h
>>>> @@ -0,0 +1,21 @@
>>>> +/*
>>>> + * Copyright 2016 Rockchip Inc.
>>>> + *
>>>> + * SPDX-License-Identifier:     GPL-2.0+
>>>> + */
>>>> +#ifndef _ASM_ARCH_QOS_RK3288_H
>>>> +#define _ASM_ARCH_QOS_RK3288_H
>>>> +
>>>> +/* cpu axi qos priority */
>>>> +#define CPU_AXI_QOS_PRIORITY_LEVEL(h, l) \
>>>> +       ((((h) & 3) << 2) | ((l) & 3))
>>>
>>> Can you instead define
>>>
>>> XXX_SHIFT   2
>>> XXX_MASK  (3 << XXX_SHIFT)
>>>
>>> and then use these in the .c code?
>>>
>>>> +
>>>> +#define CPU_AXI_QOS_PRIORITY    0x08
>>>> +
>>>> +#define VIO0_VOP_QOS            0xffad0400
>>>> +#define VIO1_VOP_QOS            0xffad0000
>>>> +#define VIO1_ISP_R_QOS          0xffad0900
>>>> +#define VIO1_ISP_W0_QOS         0xffad0100
>>>> +#define VIO1_ISP_W1_QOS         0xffad0180
>>>> +
>>>> +#endif
>>>> diff --git a/board/rockchip/miniarm_rk3288/miniarm-rk3288.c
>>>> b/board/rockchip/miniarm_rk3288/miniarm-rk3288.c
>>>> index 79541a3..ba0f3a3 100644
>>>> --- a/board/rockchip/miniarm_rk3288/miniarm-rk3288.c
>>>> +++ b/board/rockchip/miniarm_rk3288/miniarm-rk3288.c
>>>> @@ -5,3 +5,24 @@
>>>>    */
>>>>
>>>>   #include <common.h>
>>>> +#include <asm/io.h>
>>>> +#include <asm/arch/qos_rk3288.h>
>>>> +
>>>> +int rk_board_late_init(void)
>>>> +{
>>>> +       /* set isp qos to higher priority */
>>>> +       writel(CPU_AXI_QOS_PRIORITY_LEVEL(2, 2),
>>>> +              VIO1_ISP_R_QOS + CPU_AXI_QOS_PRIORITY);
>>>> +       writel(CPU_AXI_QOS_PRIORITY_LEVEL(2, 2),
>>>> +              VIO1_ISP_W0_QOS + CPU_AXI_QOS_PRIORITY);
>>>> +       writel(CPU_AXI_QOS_PRIORITY_LEVEL(2, 2),
>>>> +              VIO1_ISP_W1_QOS + CPU_AXI_QOS_PRIORITY);
>>>> +
>>>> +       /* set vop qos to higher priority */
>>>> +       writel(CPU_AXI_QOS_PRIORITY_LEVEL(2, 2),
>>>> +              VIO0_VOP_QOS + CPU_AXI_QOS_PRIORITY);
>>>> +       writel(CPU_AXI_QOS_PRIORITY_LEVEL(2, 2),
>>>> +              VIO1_VOP_QOS + CPU_AXI_QOS_PRIORITY);
>>>
>>> Can you add a register struct for this in
>>> arch/arm/include/asm/arch-rockchip/ ?
>>>
>>> Also I think it would be best to put this code somewhere in
>>> arch/arm/mach-rockchip and call it from your late init routine.
>>
>>
>> I understand people don't like source cod in hardcode(or nearly) and out
>> of
>> any framwork, but there do have some different one time init program for
>> different SoC or board, I think it's OK to add then in soc_init() or
>> board_init()
>> with appropriate comment.
>>
>> Back to the case here, setting the ISP NOC niu to higher priority is only
>> needed
>> for the miniarm board for the use case in that board. It might have other
>> requirement
>> for other board for different use cases, so it's OK to add the code in
>> board file.
>> I think you would like to have some API function for this setting then
>> different board can use it
>> instead of do SoC setting in board file, right?
>> Before other board need this blob of code, would it be more clear to put
>> the code in board file?
>>
> or in arch/arm/mach-rockchip/rk3288-board.c
> +    __weak int rk_qos_init(void)
> +{
> +    /* do  set  vop  qos level  */
> +   }
>
> int board_late_init(void)
>  {
>         setup_boot_mode();
> +       rk_qos_init();
>
> }
>
> in board/rockchip/miniarm_rk3288/miniarm-rk3288.c
> +int rk_qos_init(void)
> +{
> +    /* do  set vop  qos level  */
> +    /* do  set isp qos level  */
> +}
>
> Whether this is  a better way?
>
>
>> @Nickey, I'm sure the patch V3 is not what we need. Simon's suggestion
>> could be detail in this:
>> - add a rk3288_qos_init() in arch/arm/mach-rockchip/rk3288-board.c
>> - call the rk3288_qos_init() in rk_board_late_init() of
>> board/rockchip/miniarm_rk3288/miniarm-rk3288.c

How about something like:

if (!fdt_node_check_compatible(gd-fdt_blob, 0, "rockchip,rk3288-miniarm"))
   rk_qos_init()

Then it doesn't need to be weak.

Regards,
Simon