[v6,4/7] drm/rockchip: vop: group vop registers
diff mbox

Message ID 1501049966-6070-1-git-send-email-mark.yao@rock-chips.com
State New
Headers show

Commit Message

yao mark July 26, 2017, 6:19 a.m. UTC
Grouping the vop registers facilitates make register
definition clearer, and also is useful for different vop
reuse the same group register.

Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  99 ++++++++++++------------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  60 ++++++++-------
 drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 112 +++++++++++++++-------------
 3 files changed, 144 insertions(+), 127 deletions(-)

Comments

JeffyChen July 27, 2017, 8:49 a.m. UTC | #1
Hi mark,

On 07/26/2017 02:19 PM, Mark yao wrote:
> Grouping the vop registers facilitates make register
> definition clearer, and also is useful for different vop
> reuse the same group register.
>
> Signed-off-by: Mark Yao<mark.yao@rock-chips.com>

Reviewed-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Heiko Stübner July 27, 2017, 9:51 a.m. UTC | #2
Hi Mark,

Am Mittwoch, 26. Juli 2017, 14:19:25 CEST schrieb Mark Yao:
> Grouping the vop registers facilitates make register
> definition clearer, and also is useful for different vop
> reuse the same group register.
> 
> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  99 ++++++++++++------------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  60 ++++++++-------
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 112
> +++++++++++++++------------- 3 files changed, 144 insertions(+), 127
> deletions(-)

This breaks display support on both rk3036 and rk3288 and I end up
with a null pointer dereference in

[   10.640297] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[   10.654430] pgd = c0204000
[   10.657452] [00000000] *pgd=00000000
[   10.661473] Internal error: Oops: 5 [#1] SMP ARM
[   10.666635] Modules linked in: snd_pcm media snd_timer phy_rockchip_dp snd soundcore rockchipdrm dw_hdmi analogix_dp rtc_rk808 pwm_rockchip clk_rk808 spi_rockchip
[   10.682897] CPU: 2 PID: 143 Comm: kworker/2:2 Not tainted 4.13.0-rc2-01791-g2b86603d0515 #355
[   10.692430] Hardware name: Rockchip (Device Tree)
[   10.697692] Workqueue: events deferred_probe_work_func
[   10.702152] Linux video capture interface: v2.00
[   10.708590] task: ee38c800 task.stack: ed2e6000
[   10.713656] PC is at vop_reg_set.constprop.4+0x4/0xa8 [rockchipdrm]
[   10.720668] LR is at vop_bind+0x568/0x8a0 [rockchipdrm]
[   10.726507] pc : [<bf04db28>]    lr : [<bf04e134>]    psr: 40010013
[   10.733514] sp : ed2e7d68  ip : 00000004  fp : bf054988
[   10.739350] r10: bf054988  r9 : 00000000  r8 : 00000001
[   10.745189] r7 : ed66f500  r6 : ee29da10  r5 : 00000000  r4 : ed22e010
[   10.752487] r3 : ffffffff  r2 : 00000000  r1 : 00000000  r0 : ed22e010
[   10.759785] Flags: nZcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   10.767763] Control: 10c5387d  Table: 2d4e806a  DAC: 00000051
[   10.774188] Process kworker/2:2 (pid: 143, stack limit = 0xed2e6220)
[...]
[   11.058818] [<bf04db28>] (vop_reg_set.constprop.4 [rockchipdrm]) from [<bf04e134>] (vop_bind+0x568/0x8a0 [rockchipdrm])
[   11.058828] [<bf04e134>] (vop_bind [rockchipdrm]) from [<c0870400>] (component_bind_all+0x11c/0x23c)
[   11.058836] [<c0870400>] (component_bind_all) from [<bf04c1cc>] (rockchip_drm_bind+0x90/0x1d4 [rockchipdrm])
[   11.058843] [<bf04c1cc>] (rockchip_drm_bind [rockchipdrm]) from [<c0870854>] (try_to_bring_up_master+0x148/0x184)
[   11.058847] [<c0870854>] (try_to_bring_up_master) from [<c0870928>] (component_add+0x98/0x144)
[   11.058853] [<c0870928>] (component_add) from [<bf050d90>] (rockchip_dp_probe+0x7c/0x8c [rockchipdrm])
[   11.058860] [<bf050d90>] (rockchip_dp_probe [rockchipdrm]) from [<c0877660>] (platform_drv_probe+0x50/0xb0)
[   11.058865] [<c0877660>] (platform_drv_probe) from [<c0875b48>] (driver_probe_device+0x230/0x2e4)
[   11.058869] [<c0875b48>] (driver_probe_device) from [<c0874208>] (bus_for_each_drv+0x60/0x94)
[   11.058873] [<c0874208>] (bus_for_each_drv) from [<c0875838>] (__device_attach+0xb0/0x114)
[   11.058876] [<c0875838>] (__device_attach) from [<c0874ec8>] (bus_probe_device+0x84/0x8c)
[   11.058879] [<c0874ec8>] (bus_probe_device) from [<c087534c>] (deferred_probe_work_func+0x68/0x94)
[   11.058884] [<c087534c>] (deferred_probe_work_func) from [<c035a884>] (process_one_work+0x200/0x504)
[   11.058889] [<c035a884>] (process_one_work) from [<c035b610>] (worker_thread+0x38/0x594)
[   11.058894] [<c035b610>] (worker_thread) from [<c036045c>] (kthread+0x128/0x158)
[   11.058900] [<c036045c>] (kthread) from [<c0307d18>] (ret_from_fork+0x14/0x3c)
[   11.058904] Code: eaffffe0 e3a03004 eaffffef e92d4070 (e5914000)
[   11.058930] ---[ end trace 9caa88bbcb1af5e4 ]---

I'll try to investigate a bit more, but maybe you'll be able to
find the issue faster than me in the meantime.


Heiko
Heiko Stübner July 27, 2017, 10:10 a.m. UTC | #3
Am Donnerstag, 27. Juli 2017, 11:51:06 CEST schrieb Heiko Stübner:
> Hi Mark,
> 
> Am Mittwoch, 26. Juli 2017, 14:19:25 CEST schrieb Mark Yao:
> > Grouping the vop registers facilitates make register
> > definition clearer, and also is useful for different vop
> > reuse the same group register.
> > 
> > Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
> > ---
> > 
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  99
> >  ++++++++++++------------
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  60 ++++++++-------
> >  drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 112
> > 
> > +++++++++++++++------------- 3 files changed, 144 insertions(+), 127
> > deletions(-)
> 
> This breaks display support on both rk3036 and rk3288 and I end up
> with a null pointer dereference in
> 
> [   10.640297] Unable to handle kernel NULL pointer dereference at virtual
> address 00000000 [   10.654430] pgd = c0204000
> [   10.657452] [00000000] *pgd=00000000
> [   10.661473] Internal error: Oops: 5 [#1] SMP ARM
> [   10.666635] Modules linked in: snd_pcm media snd_timer phy_rockchip_dp
> snd soundcore rockchipdrm dw_hdmi analogix_dp rtc_rk808 pwm_rockchip
> clk_rk808 spi_rockchip [   10.682897] CPU: 2 PID: 143 Comm: kworker/2:2 Not
> tainted 4.13.0-rc2-01791-g2b86603d0515 #355 [   10.692430] Hardware name:
> Rockchip (Device Tree)
> [   10.697692] Workqueue: events deferred_probe_work_func
> [   10.702152] Linux video capture interface: v2.00
> [   10.708590] task: ee38c800 task.stack: ed2e6000
> [   10.713656] PC is at vop_reg_set.constprop.4+0x4/0xa8 [rockchipdrm]
> [   10.720668] LR is at vop_bind+0x568/0x8a0 [rockchipdrm]

The obvious reason for that is

> @@ -164,14 +153,20 @@ static inline uint32_t vop_read_reg(struct vop *vop,
> uint32_t base, return (vop_readl(vop, base + reg->offset) >> reg->shift) &
> reg->mask; }
> 
> -static inline void vop_mask_write(struct vop *vop, uint32_t offset,
> -				  uint32_t mask, uint32_t shift, uint32_t v,
> -				  bool write_mask, bool relaxed)
> +static void vop_reg_set(struct vop *vop, const struct vop_reg *reg,
> +			uint32_t _offset, uint32_t _mask, uint32_t v,
> +			const char *reg_name)
>  {
> -	if (!mask)
> +	int offset = reg->offset + _offset;
> +	int mask = reg->mask & _mask;
> +	int shift = reg->shift;
> +
> +	if (!reg || !reg->mask) {
> +		dev_dbg(vop->dev, "Warning: not support %s\n", reg_name);
>  		return;
> +	}

where the check for !reg happens after it got already dereferenced.
But even with that fixed I end up with

on rk3288:
[    7.254823] rockchip-vop ff930000.vop: Warning: not support global_regdone_en
[    7.262847] rockchip-vop ff930000.vop: Warning: not support gate
[    7.269580] rockchip-vop ff930000.vop: Warning: not support gate
[    7.302765] rockchip-vop ff940000.vop: Warning: not support global_regdone_en
[    7.310758] rockchip-vop ff940000.vop: Warning: not support gate
[    7.317475] rockchip-vop ff940000.vop: Warning: not support gate
[    7.425724] rockchip-vop ff930000.vop: Warning: not support edp_pin_pol
[    7.526298] rockchip-vop ff940000.vop: Warning: not support hdmi_pin_pol

on rk3036:
[   12.389138] rockchip-vop 10118000.vop: Warning: not support global_regdone_en
[   12.397324] rockchip-vop 10118000.vop: Warning: not support gate
[   12.404165] rockchip-vop 10118000.vop: Warning: not support gate
[   13.747361] rockchip-vop 10118000.vop: Warning: not support hdmi_pin_pol
[   13.747371] rockchip-vop 10118000.vop: Warning: not support hdmi_en
[   13.747379] rockchip-vop 10118000.vop: Warning: not support hpost_st_end
[   13.747385] rockchip-vop 10118000.vop: Warning: not support vpost_st_end
[   13.747461] rockchip-vop 10118000.vop: Warning: not support src_alpha_ctl
[   13.767098] rockchip-vop 10118000.vop: Warning: not support src_alpha_ctl
[   13.786060] rockchip-vop 10118000.vop: Warning: not support src_alpha_ctl

While reqdone and friends are obviously features of newer vops, at least
the hdmi pin-pol is available on both these socs.

With this patch applied (and null-ptr fixed) I end up without hdmi output
on both socs.


Heiko
yao mark July 28, 2017, 1:02 a.m. UTC | #4
Hi Heiko

Thanks for the test.

On 2017年07月27日 18:10, Heiko Stübner wrote:
> Am Donnerstag, 27. Juli 2017, 11:51:06 CEST schrieb Heiko Stübner:
>> Hi Mark,
>>
>> Am Mittwoch, 26. Juli 2017, 14:19:25 CEST schrieb Mark Yao:
>>> Grouping the vop registers facilitates make register
>>> definition clearer, and also is useful for different vop
>>> reuse the same group register.
>>>
>>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>> ---
>>>
>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  99
>>>   ++++++++++++------------
>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  60 ++++++++-------
>>>   drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 112
>>>
>>> +++++++++++++++------------- 3 files changed, 144 insertions(+), 127
>>> deletions(-)
>> This breaks display support on both rk3036 and rk3288 and I end up
>> with a null pointer dereference in
>>
>> [   10.640297] Unable to handle kernel NULL pointer dereference at virtual
>> address 00000000 [   10.654430] pgd = c0204000
>> [   10.657452] [00000000] *pgd=00000000
>> [   10.661473] Internal error: Oops: 5 [#1] SMP ARM
>> [   10.666635] Modules linked in: snd_pcm media snd_timer phy_rockchip_dp
>> snd soundcore rockchipdrm dw_hdmi analogix_dp rtc_rk808 pwm_rockchip
>> clk_rk808 spi_rockchip [   10.682897] CPU: 2 PID: 143 Comm: kworker/2:2 Not
>> tainted 4.13.0-rc2-01791-g2b86603d0515 #355 [   10.692430] Hardware name:
>> Rockchip (Device Tree)
>> [   10.697692] Workqueue: events deferred_probe_work_func
>> [   10.702152] Linux video capture interface: v2.00
>> [   10.708590] task: ee38c800 task.stack: ed2e6000
>> [   10.713656] PC is at vop_reg_set.constprop.4+0x4/0xa8 [rockchipdrm]
>> [   10.720668] LR is at vop_bind+0x568/0x8a0 [rockchipdrm]
> The obvious reason for that is
>
>> @@ -164,14 +153,20 @@ static inline uint32_t vop_read_reg(struct vop *vop,
>> uint32_t base, return (vop_readl(vop, base + reg->offset) >> reg->shift) &
>> reg->mask; }
>>
>> -static inline void vop_mask_write(struct vop *vop, uint32_t offset,
>> -				  uint32_t mask, uint32_t shift, uint32_t v,
>> -				  bool write_mask, bool relaxed)
>> +static void vop_reg_set(struct vop *vop, const struct vop_reg *reg,
>> +			uint32_t _offset, uint32_t _mask, uint32_t v,
>> +			const char *reg_name)
>>   {
>> -	if (!mask)
>> +	int offset = reg->offset + _offset;
>> +	int mask = reg->mask & _mask;
>> +	int shift = reg->shift;

Does the crash is that using reg->offset/mask/shift before !reg checking?

>> +
>> +	if (!reg || !reg->mask) {
>> +		dev_dbg(vop->dev, "Warning: not support %s\n", reg_name);
>>   		return;
>> +	}
> where the check for !reg happens after it got already dereferenced.
> But even with that fixed I end up with
>
> on rk3288:
> [    7.254823] rockchip-vop ff930000.vop: Warning: not support global_regdone_en
> [    7.262847] rockchip-vop ff930000.vop: Warning: not support gate
> [    7.269580] rockchip-vop ff930000.vop: Warning: not support gate
> [    7.302765] rockchip-vop ff940000.vop: Warning: not support global_regdone_en
> [    7.310758] rockchip-vop ff940000.vop: Warning: not support gate
> [    7.317475] rockchip-vop ff940000.vop: Warning: not support gate
> [    7.425724] rockchip-vop ff930000.vop: Warning: not support edp_pin_pol
> [    7.526298] rockchip-vop ff940000.vop: Warning: not support hdmi_pin_pol

Rk3288 does not support independent pin_pol settings, all output interfaces share the pin_pol register,
So not support hdmi_pin_pol here is correct.

hdmi pin pol would be set by following register:
     .pin_pol = VOP_REG(RK3288_DSP_CTRL0, 0xf, 4),

>
> on rk3036:
> [   12.389138] rockchip-vop 10118000.vop: Warning: not support global_regdone_en
> [   12.397324] rockchip-vop 10118000.vop: Warning: not support gate
> [   12.404165] rockchip-vop 10118000.vop: Warning: not support gate
> [   13.747361] rockchip-vop 10118000.vop: Warning: not support hdmi_pin_pol
> [   13.747371] rockchip-vop 10118000.vop: Warning: not support hdmi_en
> [   13.747379] rockchip-vop 10118000.vop: Warning: not support hpost_st_end
> [   13.747385] rockchip-vop 10118000.vop: Warning: not support vpost_st_end
> [   13.747461] rockchip-vop 10118000.vop: Warning: not support src_alpha_ctl
> [   13.767098] rockchip-vop 10118000.vop: Warning: not support src_alpha_ctl
> [   13.786060] rockchip-vop 10118000.vop: Warning: not support src_alpha_ctl
>
> While reqdone and friends are obviously features of newer vops, at least
> the hdmi pin-pol is available on both these socs.
>
> With this patch applied (and null-ptr fixed) I end up without hdmi output
> on both socs.

Hmmm, I am confused, from code review, I didn't see what will cause hdmi not work on rk3036 and rk3288,

Give me some time, I try to bringup my popmetal rk3288 board to do the test.

Heiko, Thanks very much for your test.

>
>
> Heiko
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>
yao mark July 28, 2017, 2:45 a.m. UTC | #5
Hi Heiko

On 2017年07月28日 09:02, Mark yao wrote:
> Hi Heiko
>
> Thanks for the test.
>
> On 2017年07月27日 18:10, Heiko Stübner wrote:
>> Am Donnerstag, 27. Juli 2017, 11:51:06 CEST schrieb Heiko Stübner:
>>> Hi Mark,
>>>
>>> Am Mittwoch, 26. Juli 2017, 14:19:25 CEST schrieb Mark Yao:
>>>> Grouping the vop registers facilitates make register
>>>> definition clearer, and also is useful for different vop
>>>> reuse the same group register.
>>>>
>>>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>>>> ---
>>>>
>>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  99
>>>>   ++++++++++++------------
>>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.h |  60 ++++++++-------
>>>>   drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 112
>>>>
>>>> +++++++++++++++------------- 3 files changed, 144 insertions(+), 127
>>>> deletions(-)
>>> This breaks display support on both rk3036 and rk3288 and I end up
>>> with a null pointer dereference in
>>>
>>> [   10.640297] Unable to handle kernel NULL pointer dereference at virtual
>>> address 00000000 [   10.654430] pgd = c0204000
>>> [   10.657452] [00000000] *pgd=00000000
>>> [   10.661473] Internal error: Oops: 5 [#1] SMP ARM
>>> [   10.666635] Modules linked in: snd_pcm media snd_timer phy_rockchip_dp
>>> snd soundcore rockchipdrm dw_hdmi analogix_dp rtc_rk808 pwm_rockchip
>>> clk_rk808 spi_rockchip [   10.682897] CPU: 2 PID: 143 Comm: kworker/2:2 Not
>>> tainted 4.13.0-rc2-01791-g2b86603d0515 #355 [   10.692430] Hardware name:
>>> Rockchip (Device Tree)
>>> [   10.697692] Workqueue: events deferred_probe_work_func
>>> [   10.702152] Linux video capture interface: v2.00
>>> [   10.708590] task: ee38c800 task.stack: ed2e6000
>>> [   10.713656] PC is at vop_reg_set.constprop.4+0x4/0xa8 [rockchipdrm]
>>> [   10.720668] LR is at vop_bind+0x568/0x8a0 [rockchipdrm]
>> The obvious reason for that is
>>
>>> @@ -164,14 +153,20 @@ static inline uint32_t vop_read_reg(struct vop *vop,
>>> uint32_t base, return (vop_readl(vop, base + reg->offset) >> reg->shift) &
>>> reg->mask; }
>>>
>>> -static inline void vop_mask_write(struct vop *vop, uint32_t offset,
>>> -                  uint32_t mask, uint32_t shift, uint32_t v,
>>> -                  bool write_mask, bool relaxed)
>>> +static void vop_reg_set(struct vop *vop, const struct vop_reg *reg,
>>> +            uint32_t _offset, uint32_t _mask, uint32_t v,
>>> +            const char *reg_name)
>>>   {
>>> -    if (!mask)
>>> +    int offset = reg->offset + _offset;
>>> +    int mask = reg->mask & _mask;
>>> +    int shift = reg->shift;
>
> Does the crash is that using reg->offset/mask/shift before !reg checking?

I reproduce this cause, from objdump, it crash on "int mask = reg->mask & _mask; "
Seems difference gcc has difference behavior, my aarch64 gcc maybe optimize it,
only access when the value be used. I think that is the reason why rk3399 works on my test.

I will fix it at next version.

>
>>> +
>>> +    if (!reg || !reg->mask) {
>>> +        dev_dbg(vop->dev, "Warning: not support %s\n", reg_name);
>>>           return;
>>> +    }
>> where the check for !reg happens after it got already dereferenced.
>> But even with that fixed I end up with
>>
>> on rk3288:
>> [    7.254823] rockchip-vop ff930000.vop: Warning: not support global_regdone_en
>> [    7.262847] rockchip-vop ff930000.vop: Warning: not support gate
>> [    7.269580] rockchip-vop ff930000.vop: Warning: not support gate
>> [    7.302765] rockchip-vop ff940000.vop: Warning: not support global_regdone_en
>> [    7.310758] rockchip-vop ff940000.vop: Warning: not support gate
>> [    7.317475] rockchip-vop ff940000.vop: Warning: not support gate
>> [    7.425724] rockchip-vop ff930000.vop: Warning: not support edp_pin_pol
>> [    7.526298] rockchip-vop ff940000.vop: Warning: not support hdmi_pin_pol
>
> Rk3288 does not support independent pin_pol settings, all output interfaces share the pin_pol register,
> So not support hdmi_pin_pol here is correct.
>
> hdmi pin pol would be set by following register:
>     .pin_pol = VOP_REG(RK3288_DSP_CTRL0, 0xf, 4),
>
>>
>> on rk3036:
>> [   12.389138] rockchip-vop 10118000.vop: Warning: not support global_regdone_en
>> [   12.397324] rockchip-vop 10118000.vop: Warning: not support gate
>> [   12.404165] rockchip-vop 10118000.vop: Warning: not support gate
>> [   13.747361] rockchip-vop 10118000.vop: Warning: not support hdmi_pin_pol
>> [   13.747371] rockchip-vop 10118000.vop: Warning: not support hdmi_en
>> [   13.747379] rockchip-vop 10118000.vop: Warning: not support hpost_st_end
>> [   13.747385] rockchip-vop 10118000.vop: Warning: not support vpost_st_end
>> [   13.747461] rockchip-vop 10118000.vop: Warning: not support src_alpha_ctl
>> [   13.767098] rockchip-vop 10118000.vop: Warning: not support src_alpha_ctl
>> [   13.786060] rockchip-vop 10118000.vop: Warning: not support src_alpha_ctl
>>
>> While reqdone and friends are obviously features of newer vops, at least
>> the hdmi pin-pol is available on both these socs.
>>
>> With this patch applied (and null-ptr fixed) I end up without hdmi output
>> on both socs.
>
> Hmmm, I am confused, from code review, I didn't see what will cause hdmi not work on rk3036 and rk3288,
>
> Give me some time, I try to bringup my popmetal rk3288 board to do the test.
>
> Heiko, Thanks very much for your test.

Hi Heiko

After fix null pointer problem, On rk3288 popmetal board, hdmi works good on my test, Can you double check it?

Thanks.

>
>>
>>
>> Heiko
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>>
>>
>
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fd47da5..92d098b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -42,30 +42,19 @@ 
 #include "rockchip_drm_psr.h"
 #include "rockchip_drm_vop.h"
 
-#define REG_SET(x, base, reg, v) \
-		vop_mask_write(x, base + reg.offset, reg.mask, reg.shift, \
-			       v, reg.write_mask, reg.relaxed)
-#define REG_SET_MASK(x, base, reg, mask, v) \
-		vop_mask_write(x, base + reg.offset, \
-			       mask, reg.shift, v, reg.write_mask, reg.relaxed)
-
 #define VOP_WIN_SET(x, win, name, v) \
-		REG_SET(x, win->base, win->phy->name, v)
+		vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name)
 #define VOP_SCL_SET(x, win, name, v) \
-		REG_SET(x, win->base, win->phy->scl->name, v)
+		vop_reg_set(vop, &win->phy->scl->name, win->base, ~0, v, #name)
 #define VOP_SCL_SET_EXT(x, win, name, v) \
-		REG_SET(x, win->base, win->phy->scl->ext->name, v)
-#define VOP_CTRL_SET(x, name, v) \
-		REG_SET(x, 0, (x)->data->ctrl->name, v)
-
-#define VOP_INTR_GET(vop, name) \
-		vop_read_reg(vop, 0, &vop->data->ctrl->name)
-
-#define VOP_INTR_SET(vop, name, v) \
-		REG_SET(vop, 0, vop->data->intr->name, v)
+		vop_reg_set(vop, &win->phy->scl->ext->name, \
+			    win->base, ~0, v, #name)
 
 #define VOP_INTR_SET_MASK(vop, name, mask, v) \
-		REG_SET_MASK(vop, 0, vop->data->intr->name, mask, v)
+		vop_reg_set(vop, &vop->data->intr->name, 0, mask, v, #name)
+
+#define VOP_REG_SET(vop, group, name, v) \
+		    vop_reg_set(vop, &vop->data->group->name, 0, ~0, v, #name)
 
 #define VOP_INTR_SET_TYPE(vop, name, type, v) \
 	do { \
@@ -82,7 +71,7 @@ 
 		vop_get_intr_type(vop, &vop->data->intr->name, type)
 
 #define VOP_WIN_GET(x, win, name) \
-		vop_read_reg(x, win->base, &win->phy->name)
+		vop_read_reg(x, win->offset, win->phy->name)
 
 #define VOP_WIN_GET_YRGBADDR(vop, win) \
 		vop_readl(vop, win->base + win->phy->yrgb_mst.offset)
@@ -164,14 +153,20 @@  static inline uint32_t vop_read_reg(struct vop *vop, uint32_t base,
 	return (vop_readl(vop, base + reg->offset) >> reg->shift) & reg->mask;
 }
 
-static inline void vop_mask_write(struct vop *vop, uint32_t offset,
-				  uint32_t mask, uint32_t shift, uint32_t v,
-				  bool write_mask, bool relaxed)
+static void vop_reg_set(struct vop *vop, const struct vop_reg *reg,
+			uint32_t _offset, uint32_t _mask, uint32_t v,
+			const char *reg_name)
 {
-	if (!mask)
+	int offset = reg->offset + _offset;
+	int mask = reg->mask & _mask;
+	int shift = reg->shift;
+
+	if (!reg || !reg->mask) {
+		dev_dbg(vop->dev, "Warning: not support %s\n", reg_name);
 		return;
+	}
 
-	if (write_mask) {
+	if (reg->write_mask) {
 		v = ((v << shift) & 0xffff) | (mask << (shift + 16));
 	} else {
 		uint32_t cached_val = vop->regsbak[offset >> 2];
@@ -180,7 +175,7 @@  static inline void vop_mask_write(struct vop *vop, uint32_t offset,
 		vop->regsbak[offset >> 2] = v;
 	}
 
-	if (relaxed)
+	if (reg->relaxed)
 		writel_relaxed(v, vop->regs + offset);
 	else
 		writel(v, vop->regs + offset);
@@ -202,7 +197,7 @@  static inline uint32_t vop_get_intr_type(struct vop *vop,
 
 static inline void vop_cfg_done(struct vop *vop)
 {
-	VOP_CTRL_SET(vop, cfg_done, 1);
+	VOP_REG_SET(vop, common, cfg_done, 1);
 }
 
 static bool has_rb_swapped(uint32_t format)
@@ -540,7 +535,7 @@  static int vop_enable(struct drm_crtc *crtc)
 
 	spin_lock(&vop->reg_lock);
 
-	VOP_CTRL_SET(vop, standby, 0);
+	VOP_REG_SET(vop, common, standby, 1);
 
 	spin_unlock(&vop->reg_lock);
 
@@ -600,7 +595,7 @@  static void vop_crtc_disable(struct drm_crtc *crtc)
 
 	spin_lock(&vop->reg_lock);
 
-	VOP_CTRL_SET(vop, standby, 1);
+	VOP_REG_SET(vop, common, standby, 1);
 
 	spin_unlock(&vop->reg_lock);
 
@@ -923,7 +918,7 @@  static void vop_crtc_enable(struct drm_crtc *crtc)
 
 		spin_lock(&vop->reg_lock);
 
-		VOP_CTRL_SET(vop, standby, 1);
+		VOP_REG_SET(vop, common, standby, 1);
 
 		spin_unlock(&vop->reg_lock);
 
@@ -937,29 +932,29 @@  static void vop_crtc_enable(struct drm_crtc *crtc)
 		   BIT(HSYNC_POSITIVE) : 0;
 	pin_pol |= (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC) ?
 		   BIT(VSYNC_POSITIVE) : 0;
-	VOP_CTRL_SET(vop, pin_pol, pin_pol);
+	VOP_REG_SET(vop, output, pin_pol, pin_pol);
 
 	switch (s->output_type) {
 	case DRM_MODE_CONNECTOR_LVDS:
-		VOP_CTRL_SET(vop, rgb_en, 1);
-		VOP_CTRL_SET(vop, rgb_pin_pol, pin_pol);
+		VOP_REG_SET(vop, output, rgb_en, 1);
+		VOP_REG_SET(vop, output, rgb_pin_pol, pin_pol);
 		break;
 	case DRM_MODE_CONNECTOR_eDP:
-		VOP_CTRL_SET(vop, edp_pin_pol, pin_pol);
-		VOP_CTRL_SET(vop, edp_en, 1);
+		VOP_REG_SET(vop, output, edp_pin_pol, pin_pol);
+		VOP_REG_SET(vop, output, edp_en, 1);
 		break;
 	case DRM_MODE_CONNECTOR_HDMIA:
-		VOP_CTRL_SET(vop, hdmi_pin_pol, pin_pol);
-		VOP_CTRL_SET(vop, hdmi_en, 1);
+		VOP_REG_SET(vop, output, hdmi_pin_pol, pin_pol);
+		VOP_REG_SET(vop, output, hdmi_en, 1);
 		break;
 	case DRM_MODE_CONNECTOR_DSI:
-		VOP_CTRL_SET(vop, mipi_pin_pol, pin_pol);
-		VOP_CTRL_SET(vop, mipi_en, 1);
+		VOP_REG_SET(vop, output, mipi_pin_pol, pin_pol);
+		VOP_REG_SET(vop, output, mipi_en, 1);
 		break;
 	case DRM_MODE_CONNECTOR_DisplayPort:
 		pin_pol &= ~BIT(DCLK_INVERT);
-		VOP_CTRL_SET(vop, dp_pin_pol, pin_pol);
-		VOP_CTRL_SET(vop, dp_en, 1);
+		VOP_REG_SET(vop, output, dp_pin_pol, pin_pol);
+		VOP_REG_SET(vop, output, dp_en, 1);
 		break;
 	default:
 		DRM_DEV_ERROR(vop->dev, "unsupported connector_type [%d]\n",
@@ -972,25 +967,25 @@  static void vop_crtc_enable(struct drm_crtc *crtc)
 	if (s->output_mode == ROCKCHIP_OUT_MODE_AAAA &&
 	    !(vop_data->feature & VOP_FEATURE_OUTPUT_RGB10))
 		s->output_mode = ROCKCHIP_OUT_MODE_P888;
-	VOP_CTRL_SET(vop, out_mode, s->output_mode);
+	VOP_REG_SET(vop, common, out_mode, s->output_mode);
 
-	VOP_CTRL_SET(vop, htotal_pw, (htotal << 16) | hsync_len);
+	VOP_REG_SET(vop, modeset, htotal_pw, (htotal << 16) | hsync_len);
 	val = hact_st << 16;
 	val |= hact_end;
-	VOP_CTRL_SET(vop, hact_st_end, val);
-	VOP_CTRL_SET(vop, hpost_st_end, val);
+	VOP_REG_SET(vop, modeset, hact_st_end, val);
+	VOP_REG_SET(vop, modeset, hpost_st_end, val);
 
-	VOP_CTRL_SET(vop, vtotal_pw, (vtotal << 16) | vsync_len);
+	VOP_REG_SET(vop, modeset, vtotal_pw, (vtotal << 16) | vsync_len);
 	val = vact_st << 16;
 	val |= vact_end;
-	VOP_CTRL_SET(vop, vact_st_end, val);
-	VOP_CTRL_SET(vop, vpost_st_end, val);
+	VOP_REG_SET(vop, modeset, vact_st_end, val);
+	VOP_REG_SET(vop, modeset, vpost_st_end, val);
 
-	VOP_INTR_SET(vop, line_flag_num[0], vact_end);
+	VOP_REG_SET(vop, intr, line_flag_num[0], vact_end);
 
 	clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
 
-	VOP_CTRL_SET(vop, standby, 0);
+	VOP_REG_SET(vop, common, standby, 0);
 
 	rockchip_drm_psr_activate(&vop->crtc);
 }
@@ -1452,8 +1447,8 @@  static int vop_initial(struct vop *vop)
 
 	memcpy(vop->regsbak, vop->regs, vop->len);
 
-	VOP_CTRL_SET(vop, global_regdone_en, 1);
-	VOP_CTRL_SET(vop, dsp_blank, 0);
+	VOP_REG_SET(vop, misc, global_regdone_en, 1);
+	VOP_REG_SET(vop, common, dsp_blank, 0);
 
 	for (i = 0; i < vop_data->win_size; i++) {
 		const struct vop_win_data *win = &vop_data->win[i];
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 850f8e4..3ba962c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -25,43 +25,50 @@  enum vop_data_format {
 };
 
 struct vop_reg {
-	uint32_t offset;
-	uint32_t shift;
 	uint32_t mask;
+	uint16_t offset;
+	uint8_t shift;
 	bool write_mask;
 	bool relaxed;
 };
 
-struct vop_ctrl {
-	struct vop_reg standby;
-	struct vop_reg data_blank;
-	struct vop_reg gate_en;
-	struct vop_reg mmu_en;
-	struct vop_reg rgb_en;
+struct vop_modeset {
+	struct vop_reg htotal_pw;
+	struct vop_reg hact_st_end;
+	struct vop_reg hpost_st_end;
+	struct vop_reg vtotal_pw;
+	struct vop_reg vact_st_end;
+	struct vop_reg vpost_st_end;
+};
+
+struct vop_output {
+	struct vop_reg pin_pol;
+	struct vop_reg dp_pin_pol;
+	struct vop_reg edp_pin_pol;
+	struct vop_reg hdmi_pin_pol;
+	struct vop_reg mipi_pin_pol;
+	struct vop_reg rgb_pin_pol;
+	struct vop_reg dp_en;
 	struct vop_reg edp_en;
 	struct vop_reg hdmi_en;
 	struct vop_reg mipi_en;
-	struct vop_reg dp_en;
+	struct vop_reg rgb_en;
+};
+
+struct vop_common {
+	struct vop_reg cfg_done;
 	struct vop_reg dsp_blank;
-	struct vop_reg out_mode;
+	struct vop_reg data_blank;
 	struct vop_reg dither_down;
 	struct vop_reg dither_up;
-	struct vop_reg pin_pol;
-	struct vop_reg rgb_pin_pol;
-	struct vop_reg hdmi_pin_pol;
-	struct vop_reg edp_pin_pol;
-	struct vop_reg mipi_pin_pol;
-	struct vop_reg dp_pin_pol;
-
-	struct vop_reg htotal_pw;
-	struct vop_reg hact_st_end;
-	struct vop_reg vtotal_pw;
-	struct vop_reg vact_st_end;
-	struct vop_reg hpost_st_end;
-	struct vop_reg vpost_st_end;
+	struct vop_reg gate_en;
+	struct vop_reg mmu_en;
+	struct vop_reg out_mode;
+	struct vop_reg standby;
+};
 
+struct vop_misc {
 	struct vop_reg global_regdone_en;
-	struct vop_reg cfg_done;
 };
 
 struct vop_intr {
@@ -135,8 +142,11 @@  struct vop_win_data {
 };
 
 struct vop_data {
-	const struct vop_ctrl *ctrl;
 	const struct vop_intr *intr;
+	const struct vop_common *common;
+	const struct vop_misc *misc;
+	const struct vop_modeset *modeset;
+	const struct vop_output *output;
 	const struct vop_win_data *win;
 	unsigned int win_size;
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
index 0a5f0d2..20607a8 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -117,26 +117,34 @@ 
 	.intrs = rk3036_vop_intrs,
 	.nintrs = ARRAY_SIZE(rk3036_vop_intrs),
 	.line_flag_num[0] = VOP_REG(RK3036_INT_STATUS, 0xfff, 12),
-	.status = VOP_REG(RK3036_INT_STATUS, 0xf, 0),
-	.enable = VOP_REG(RK3036_INT_STATUS, 0xf, 4),
-	.clear = VOP_REG(RK3036_INT_STATUS, 0xf, 8),
+	.status = VOP_REG_SYNC(RK3036_INT_STATUS, 0xf, 0),
+	.enable = VOP_REG_SYNC(RK3036_INT_STATUS, 0xf, 4),
+	.clear = VOP_REG_SYNC(RK3036_INT_STATUS, 0xf, 8),
 };
 
-static const struct vop_ctrl rk3036_ctrl_data = {
-	.standby = VOP_REG_SYNC(RK3036_SYS_CTRL, 0x1, 30),
-	.out_mode = VOP_REG(RK3036_DSP_CTRL0, 0xf, 0),
-	.pin_pol = VOP_REG(RK3036_DSP_CTRL0, 0xf, 4),
-	.dsp_blank = VOP_REG(RK3036_DSP_CTRL1, 0x1, 24),
+static const struct vop_modeset rk3036_modeset = {
 	.htotal_pw = VOP_REG(RK3036_DSP_HTOTAL_HS_END, 0x1fff1fff, 0),
 	.hact_st_end = VOP_REG(RK3036_DSP_HACT_ST_END, 0x1fff1fff, 0),
 	.vtotal_pw = VOP_REG(RK3036_DSP_VTOTAL_VS_END, 0x1fff1fff, 0),
 	.vact_st_end = VOP_REG(RK3036_DSP_VACT_ST_END, 0x1fff1fff, 0),
+};
+
+static const struct vop_output rk3036_output = {
+	.pin_pol = VOP_REG(RK3036_DSP_CTRL0, 0xf, 4),
+};
+
+static const struct vop_common rk3036_common = {
+	.standby = VOP_REG_SYNC(RK3036_SYS_CTRL, 0x1, 30),
+	.out_mode = VOP_REG(RK3036_DSP_CTRL0, 0xf, 0),
+	.dsp_blank = VOP_REG(RK3036_DSP_CTRL1, 0x1, 24),
 	.cfg_done = VOP_REG_SYNC(RK3036_REG_CFG_DONE, 0x1, 0),
 };
 
 static const struct vop_data rk3036_vop = {
-	.ctrl = &rk3036_ctrl_data,
 	.intr = &rk3036_intr,
+	.common = &rk3036_common,
+	.modeset = &rk3036_modeset,
+	.output = &rk3036_output,
 	.win = rk3036_vop_win_data,
 	.win_size = ARRAY_SIZE(rk3036_vop_win_data),
 };
@@ -206,27 +214,32 @@ 
 	.dst_alpha_ctl = VOP_REG(RK3288_WIN2_DST_ALPHA_CTRL, 0xff, 0),
 };
 
-static const struct vop_ctrl rk3288_ctrl_data = {
-	.standby = VOP_REG_SYNC(RK3288_SYS_CTRL, 0x1, 22),
-	.gate_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 23),
-	.mmu_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 20),
+static const struct vop_modeset rk3288_modeset = {
+	.htotal_pw = VOP_REG(RK3288_DSP_HTOTAL_HS_END, 0x1fff1fff, 0),
+	.hact_st_end = VOP_REG(RK3288_DSP_HACT_ST_END, 0x1fff1fff, 0),
+	.vtotal_pw = VOP_REG(RK3288_DSP_VTOTAL_VS_END, 0x1fff1fff, 0),
+	.vact_st_end = VOP_REG(RK3288_DSP_VACT_ST_END, 0x1fff1fff, 0),
+	.hpost_st_end = VOP_REG(RK3288_POST_DSP_HACT_INFO, 0x1fff1fff, 0),
+	.vpost_st_end = VOP_REG(RK3288_POST_DSP_VACT_INFO, 0x1fff1fff, 0),
+};
+
+static const struct vop_output rk3288_output = {
+	.pin_pol = VOP_REG(RK3288_DSP_CTRL0, 0xf, 4),
 	.rgb_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 12),
 	.hdmi_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 13),
 	.edp_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 14),
 	.mipi_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 15),
+};
+
+static const struct vop_common rk3288_common = {
+	.standby = VOP_REG_SYNC(RK3288_SYS_CTRL, 0x1, 22),
+	.gate_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 23),
+	.mmu_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 20),
 	.dither_down = VOP_REG(RK3288_DSP_CTRL1, 0xf, 1),
 	.dither_up = VOP_REG(RK3288_DSP_CTRL1, 0x1, 6),
 	.data_blank = VOP_REG(RK3288_DSP_CTRL0, 0x1, 19),
 	.dsp_blank = VOP_REG(RK3288_DSP_CTRL0, 0x3, 18),
 	.out_mode = VOP_REG(RK3288_DSP_CTRL0, 0xf, 0),
-	.pin_pol = VOP_REG(RK3288_DSP_CTRL0, 0xf, 4),
-	.htotal_pw = VOP_REG(RK3288_DSP_HTOTAL_HS_END, 0x1fff1fff, 0),
-	.hact_st_end = VOP_REG(RK3288_DSP_HACT_ST_END, 0x1fff1fff, 0),
-	.vtotal_pw = VOP_REG(RK3288_DSP_VTOTAL_VS_END, 0x1fff1fff, 0),
-	.vact_st_end = VOP_REG(RK3288_DSP_VACT_ST_END, 0x1fff1fff, 0),
-	.hpost_st_end = VOP_REG(RK3288_POST_DSP_HACT_INFO, 0x1fff1fff, 0),
-	.vpost_st_end = VOP_REG(RK3288_POST_DSP_VACT_INFO, 0x1fff1fff, 0),
-	.global_regdone_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 11),
 	.cfg_done = VOP_REG_SYNC(RK3288_REG_CFG_DONE, 0x1, 0),
 };
 
@@ -266,37 +279,13 @@ 
 static const struct vop_data rk3288_vop = {
 	.feature = VOP_FEATURE_OUTPUT_RGB10,
 	.intr = &rk3288_vop_intr,
-	.ctrl = &rk3288_ctrl_data,
+	.common = &rk3288_common,
+	.modeset = &rk3288_modeset,
+	.output = &rk3288_output,
 	.win = rk3288_vop_win_data,
 	.win_size = ARRAY_SIZE(rk3288_vop_win_data),
 };
 
-static const struct vop_ctrl rk3399_ctrl_data = {
-	.standby = VOP_REG_SYNC(RK3399_SYS_CTRL, 0x1, 22),
-	.gate_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 23),
-	.dp_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 11),
-	.rgb_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 12),
-	.hdmi_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 13),
-	.edp_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 14),
-	.mipi_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 15),
-	.dither_down = VOP_REG(RK3399_DSP_CTRL1, 0xf, 1),
-	.dither_up = VOP_REG(RK3399_DSP_CTRL1, 0x1, 6),
-	.data_blank = VOP_REG(RK3399_DSP_CTRL0, 0x1, 19),
-	.out_mode = VOP_REG(RK3399_DSP_CTRL0, 0xf, 0),
-	.rgb_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 16),
-	.dp_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 16),
-	.hdmi_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 20),
-	.edp_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 24),
-	.mipi_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 28),
-	.htotal_pw = VOP_REG(RK3399_DSP_HTOTAL_HS_END, 0x1fff1fff, 0),
-	.hact_st_end = VOP_REG(RK3399_DSP_HACT_ST_END, 0x1fff1fff, 0),
-	.vtotal_pw = VOP_REG(RK3399_DSP_VTOTAL_VS_END, 0x1fff1fff, 0),
-	.vact_st_end = VOP_REG(RK3399_DSP_VACT_ST_END, 0x1fff1fff, 0),
-	.hpost_st_end = VOP_REG(RK3399_POST_DSP_HACT_INFO, 0x1fff1fff, 0),
-	.vpost_st_end = VOP_REG(RK3399_POST_DSP_VACT_INFO, 0x1fff1fff, 0),
-	.cfg_done = VOP_REG_MASK_SYNC(RK3399_REG_CFG_DONE, 0x1, 0),
-};
-
 static const int rk3399_vop_intrs[] = {
 	FS_INTR,
 	0, 0,
@@ -317,10 +306,30 @@ 
 	.clear = VOP_REG_MASK_SYNC(RK3399_INTR_CLEAR0, 0xffff, 0),
 };
 
+static const struct vop_output rk3399_output = {
+	.dp_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 16),
+	.rgb_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 16),
+	.hdmi_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 20),
+	.edp_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 24),
+	.mipi_pin_pol = VOP_REG(RK3399_DSP_CTRL1, 0xf, 28),
+	.dp_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 11),
+	.rgb_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 12),
+	.hdmi_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 13),
+	.edp_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 14),
+	.mipi_en = VOP_REG(RK3288_SYS_CTRL, 0x1, 15),
+};
+
+static const struct vop_misc rk3399_misc = {
+	.global_regdone_en = VOP_REG(RK3399_SYS_CTRL, 0x1, 11),
+};
+
 static const struct vop_data rk3399_vop_big = {
 	.feature = VOP_FEATURE_OUTPUT_RGB10,
 	.intr = &rk3399_vop_intr,
-	.ctrl = &rk3399_ctrl_data,
+	.common = &rk3288_common,
+	.modeset = &rk3288_modeset,
+	.output = &rk3399_output,
+	.misc = &rk3399_misc,
 	/*
 	 * rk3399 vop big windows register layout is same as rk3288.
 	 */
@@ -337,7 +346,10 @@ 
 
 static const struct vop_data rk3399_vop_lit = {
 	.intr = &rk3399_vop_intr,
-	.ctrl = &rk3399_ctrl_data,
+	.common = &rk3288_common,
+	.modeset = &rk3288_modeset,
+	.output = &rk3399_output,
+	.misc = &rk3399_misc,
 	/*
 	 * rk3399 vop lit windows register layout is same as rk3288,
 	 * but cut off the win1 and win3 windows.