From: Mark yao <mark.yao@rock-chips.com>
To: Sean Paul <seanpaul@chromium.org>
Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
linux-rockchip@lists.infradead.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version
Date: Thu, 13 Jul 2017 09:45:12 +0800 [thread overview]
Message-ID: <5966D0A8.6060003@rock-chips.com> (raw)
In-Reply-To: <20170712175305.n7kk7dzvfs2vywz3@art_vandelay>
On 2017年07月13日 01:53, Sean Paul wrote:
> On Wed, Jul 12, 2017 at 10:03:38AM +0800, Mark Yao wrote:
>
> Please add a commit message describing *what* and *why* you are making the
> change.
Got it, I will fix it at next version.
Thanks.
>
>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>> ---
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 66 +++++++++++++++++++++--------
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 18 ++++++--
>> drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 20 ++++++---
>> 3 files changed, 77 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 7a5f809..a9180fd 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -42,33 +42,60 @@
>> #include "rockchip_drm_psr.h"
>> #include "rockchip_drm_vop.h"
>>
>> -#define __REG_SET_RELAXED(x, off, mask, shift, v, write_mask) \
>> - vop_mask_write(x, off, mask, shift, v, write_mask, true)
>> +#define VOP_REG_SUPPORT(vop, reg) \
>> + (!reg.major || (reg.major == VOP_MAJOR(vop->data->version) && \
>> + reg.begin_minor <= VOP_MINOR(vop->data->version) && \
>> + reg.end_minor >= VOP_MINOR(vop->data->version) && \
>> + reg.mask))
> This would be better suited as a static inline function. As would many of the
> macros below.
Got it, will fix it at next version.
>
>>
>> -#define __REG_SET_NORMAL(x, off, mask, shift, v, write_mask) \
>> - vop_mask_write(x, off, mask, shift, v, write_mask, false)
>> +#define VOP_WIN_SUPPORT(vop, win, name) \
>> + VOP_REG_SUPPORT(vop, win->phy->name)
>>
>> -#define REG_SET(x, base, reg, v, mode) \
>> - __REG_SET_##mode(x, base + reg.offset, \
>> - reg.mask, reg.shift, v, reg.write_mask)
>> -#define REG_SET_MASK(x, base, reg, mask, v, mode) \
>> - __REG_SET_##mode(x, base + reg.offset, \
>> - mask, reg.shift, v, reg.write_mask)
>> +#define VOP_CTRL_SUPPORT(vop, name) \
>> + VOP_REG_SUPPORT(vop, vop->data->ctrl->name)
>> +
>> +#define VOP_INTR_SUPPORT(vop, name) \
>> + VOP_REG_SUPPORT(vop, vop->data->intr->name)
>> +
>> +#define __REG_SET(x, off, mask, shift, v, write_mask, relaxed) \
>> + vop_mask_write(x, off, mask, shift, v, write_mask, relaxed)
> There's really no point to this, just call vop_mask_write directly.
Got it, will fix it at next version.
>
>> +
>> +#define _REG_SET(vop, name, off, reg, mask, v, relaxed) \
>> + do { \
>> + if (VOP_REG_SUPPORT(vop, reg)) \
>> + __REG_SET(vop, off + reg.offset, mask, reg.shift, \
> s/mask/reg.mask & mask/
Got it, will fix it at next version.
>
>> + v, reg.write_mask, relaxed); \
>> + else \
>> + dev_dbg(vop->dev, "Warning: not support "#name"\n"); \
>> + } while (0)
>
> Also better as static inline, IMO.
Good idea, I will try it.
>
>> +
>> +#define REG_SET(x, name, off, reg, v, relaxed) \
>> + _REG_SET(x, name, off, reg, reg.mask, v, relaxed)
> s/reg.mask/~0/
Got it, will fix it at next version.
>
>> +#define REG_SET_MASK(x, name, off, reg, mask, v, relaxed) \
>> + _REG_SET(x, name, off, reg, reg.mask & mask, v, relaxed)
> s/reg.mask &//
>
> Also, these can become static inline functions as well.
Got it, will fix it at next version.
>
>>
>> #define VOP_WIN_SET(x, win, name, v) \
>> - REG_SET(x, win->base, win->phy->name, v, RELAXED)
>> + REG_SET(x, name, win->base, win->phy->name, v, true)
>> +#define VOP_WIN_SET_EXT(x, win, ext, name, v) \
>> + REG_SET(x, name, 0, win->ext->name, v, true)
>> #define VOP_SCL_SET(x, win, name, v) \
>> - REG_SET(x, win->base, win->phy->scl->name, v, RELAXED)
>> + REG_SET(x, name, win->base, win->phy->scl->name, v, true)
>> #define VOP_SCL_SET_EXT(x, win, name, v) \
>> - REG_SET(x, win->base, win->phy->scl->ext->name, v, RELAXED)
>> + REG_SET(x, name, win->base, win->phy->scl->ext->name, v, true)
>> +
>> #define VOP_CTRL_SET(x, name, v) \
>> - REG_SET(x, 0, (x)->data->ctrl->name, v, NORMAL)
>> + REG_SET(x, name, 0, (x)->data->ctrl->name, v, false)
>>
>> #define VOP_INTR_GET(vop, name) \
>> vop_read_reg(vop, 0, &vop->data->ctrl->name)
>>
>> -#define VOP_INTR_SET(vop, name, mask, v) \
>> - REG_SET_MASK(vop, 0, vop->data->intr->name, mask, v, NORMAL)
>> +#define VOP_INTR_SET(vop, name, v) \
>> + REG_SET(vop, name, 0, vop->data->intr->name, \
>> + v, false)
>> +#define VOP_INTR_SET_MASK(vop, name, mask, v) \
>> + REG_SET_MASK(vop, name, 0, vop->data->intr->name, \
>> + mask, v, false)
>> +
>> #define VOP_INTR_SET_TYPE(vop, name, type, v) \
>> do { \
>> int i, reg = 0, mask = 0; \
>> @@ -78,13 +105,16 @@
>> mask |= 1 << i; \
>> } \
>> } \
>> - VOP_INTR_SET(vop, name, mask, reg); \
>> + VOP_INTR_SET_MASK(vop, name, mask, reg); \
>> } while (0)
>> #define VOP_INTR_GET_TYPE(vop, name, type) \
>> vop_get_intr_type(vop, &vop->data->intr->name, type)
>>
>> +#define VOP_CTRL_GET(x, name) \
>> + vop_read_reg(x, 0, &vop->data->ctrl->name)
>> +
>> #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)
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> index 084d3b2..e4de890 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> @@ -15,6 +15,14 @@
>> #ifndef _ROCKCHIP_DRM_VOP_H
>> #define _ROCKCHIP_DRM_VOP_H
>>
>> +/*
>> + * major: IP major vertion, used for IP structure
> s/vertion/version
Got it, will fix it at next version.
Thanks.
>
>> + * minor: big feature change under same structure
>> + */
>> +#define VOP_VERSION(major, minor) ((major) << 8 | (minor))
>> +#define VOP_MAJOR(version) ((version) >> 8)
>> +#define VOP_MINOR(version) ((version) & 0xff)
>> +
>> enum vop_data_format {
>> VOP_FMT_ARGB8888 = 0,
>> VOP_FMT_RGB888,
>> @@ -25,10 +33,13 @@ enum vop_data_format {
>> };
>>
>> struct vop_reg {
>> - uint32_t offset;
>> - uint32_t shift;
>> uint32_t mask;
>> - bool write_mask;
>> + uint32_t offset:12;
>> + uint32_t shift:5;
>> + uint32_t begin_minor:4;
>> + uint32_t end_minor:4;
>> + uint32_t major:3;
>> + uint32_t write_mask:1;
> Why are you packing this?
make struct vop_reg not too big, jus u64 size, struct vop_reg use a lot on register define, packing it would reduce register table size.
>
>> };
>>
>> struct vop_ctrl {
>> @@ -134,6 +145,7 @@ struct vop_win_data {
>> };
>>
>> struct vop_data {
>> + uint32_t version;
>> const struct vop_ctrl *ctrl;
>> const struct vop_intr *intr;
>> const struct vop_win_data *win;
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>> index 00e9d79..7744603 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>> @@ -20,17 +20,25 @@
>> #include "rockchip_drm_vop.h"
>> #include "rockchip_vop_reg.h"
>>
>> -#define VOP_REG(off, _mask, s) \
>> +#define VOP_REG_VER_MASK(off, _mask, s, _write_mask, _major, \
>> + _begin_minor, _end_minor) \
>> {.offset = off, \
>> .mask = _mask, \
>> .shift = s, \
>> - .write_mask = false,}
>> + .write_mask = _write_mask, \
>> + .major = _major, \
>> + .begin_minor = _begin_minor, \
>> + .end_minor = _end_minor,}
>> +
>> +#define VOP_REG_VER(off, _mask, s, _major, _begin_minor, _end_minor) \
>> + VOP_REG_VER_MASK(off, _mask, s, false, \
>> + _major, _begin_minor, _end_minor)
>> +
>> +#define VOP_REG(off, _mask, s) \
>> + VOP_REG_VER(off, _mask, s, 0, 0, -1)
>>
>> #define VOP_REG_MASK(off, _mask, s) \
>> - {.offset = off, \
>> - .mask = _mask, \
>> - .shift = s, \
>> - .write_mask = true,}
>> + VOP_REG_VER_MASK(off, _mask, s, true, 0, 0, -1)
>>
>> static const uint32_t formats_win_full[] = {
>> DRM_FORMAT_XRGB8888,
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ｍark Yao
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
WARNING: multiple messages have this Message-ID (diff)
From: mark.yao@rock-chips.com (Mark yao)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version
Date: Thu, 13 Jul 2017 09:45:12 +0800 [thread overview]
Message-ID: <5966D0A8.6060003@rock-chips.com> (raw)
In-Reply-To: <20170712175305.n7kk7dzvfs2vywz3@art_vandelay>
On 2017?07?13? 01:53, Sean Paul wrote:
> On Wed, Jul 12, 2017 at 10:03:38AM +0800, Mark Yao wrote:
>
> Please add a commit message describing *what* and *why* you are making the
> change.
Got it, I will fix it at next version.
Thanks.
>
>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>> ---
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 66 +++++++++++++++++++++--------
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 18 ++++++--
>> drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 20 ++++++---
>> 3 files changed, 77 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 7a5f809..a9180fd 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -42,33 +42,60 @@
>> #include "rockchip_drm_psr.h"
>> #include "rockchip_drm_vop.h"
>>
>> -#define __REG_SET_RELAXED(x, off, mask, shift, v, write_mask) \
>> - vop_mask_write(x, off, mask, shift, v, write_mask, true)
>> +#define VOP_REG_SUPPORT(vop, reg) \
>> + (!reg.major || (reg.major == VOP_MAJOR(vop->data->version) && \
>> + reg.begin_minor <= VOP_MINOR(vop->data->version) && \
>> + reg.end_minor >= VOP_MINOR(vop->data->version) && \
>> + reg.mask))
> This would be better suited as a static inline function. As would many of the
> macros below.
Got it, will fix it at next version.
>
>>
>> -#define __REG_SET_NORMAL(x, off, mask, shift, v, write_mask) \
>> - vop_mask_write(x, off, mask, shift, v, write_mask, false)
>> +#define VOP_WIN_SUPPORT(vop, win, name) \
>> + VOP_REG_SUPPORT(vop, win->phy->name)
>>
>> -#define REG_SET(x, base, reg, v, mode) \
>> - __REG_SET_##mode(x, base + reg.offset, \
>> - reg.mask, reg.shift, v, reg.write_mask)
>> -#define REG_SET_MASK(x, base, reg, mask, v, mode) \
>> - __REG_SET_##mode(x, base + reg.offset, \
>> - mask, reg.shift, v, reg.write_mask)
>> +#define VOP_CTRL_SUPPORT(vop, name) \
>> + VOP_REG_SUPPORT(vop, vop->data->ctrl->name)
>> +
>> +#define VOP_INTR_SUPPORT(vop, name) \
>> + VOP_REG_SUPPORT(vop, vop->data->intr->name)
>> +
>> +#define __REG_SET(x, off, mask, shift, v, write_mask, relaxed) \
>> + vop_mask_write(x, off, mask, shift, v, write_mask, relaxed)
> There's really no point to this, just call vop_mask_write directly.
Got it, will fix it at next version.
>
>> +
>> +#define _REG_SET(vop, name, off, reg, mask, v, relaxed) \
>> + do { \
>> + if (VOP_REG_SUPPORT(vop, reg)) \
>> + __REG_SET(vop, off + reg.offset, mask, reg.shift, \
> s/mask/reg.mask & mask/
Got it, will fix it at next version.
>
>> + v, reg.write_mask, relaxed); \
>> + else \
>> + dev_dbg(vop->dev, "Warning: not support "#name"\n"); \
>> + } while (0)
>
> Also better as static inline, IMO.
Good idea, I will try it.
>
>> +
>> +#define REG_SET(x, name, off, reg, v, relaxed) \
>> + _REG_SET(x, name, off, reg, reg.mask, v, relaxed)
> s/reg.mask/~0/
Got it, will fix it at next version.
>
>> +#define REG_SET_MASK(x, name, off, reg, mask, v, relaxed) \
>> + _REG_SET(x, name, off, reg, reg.mask & mask, v, relaxed)
> s/reg.mask &//
>
> Also, these can become static inline functions as well.
Got it, will fix it at next version.
>
>>
>> #define VOP_WIN_SET(x, win, name, v) \
>> - REG_SET(x, win->base, win->phy->name, v, RELAXED)
>> + REG_SET(x, name, win->base, win->phy->name, v, true)
>> +#define VOP_WIN_SET_EXT(x, win, ext, name, v) \
>> + REG_SET(x, name, 0, win->ext->name, v, true)
>> #define VOP_SCL_SET(x, win, name, v) \
>> - REG_SET(x, win->base, win->phy->scl->name, v, RELAXED)
>> + REG_SET(x, name, win->base, win->phy->scl->name, v, true)
>> #define VOP_SCL_SET_EXT(x, win, name, v) \
>> - REG_SET(x, win->base, win->phy->scl->ext->name, v, RELAXED)
>> + REG_SET(x, name, win->base, win->phy->scl->ext->name, v, true)
>> +
>> #define VOP_CTRL_SET(x, name, v) \
>> - REG_SET(x, 0, (x)->data->ctrl->name, v, NORMAL)
>> + REG_SET(x, name, 0, (x)->data->ctrl->name, v, false)
>>
>> #define VOP_INTR_GET(vop, name) \
>> vop_read_reg(vop, 0, &vop->data->ctrl->name)
>>
>> -#define VOP_INTR_SET(vop, name, mask, v) \
>> - REG_SET_MASK(vop, 0, vop->data->intr->name, mask, v, NORMAL)
>> +#define VOP_INTR_SET(vop, name, v) \
>> + REG_SET(vop, name, 0, vop->data->intr->name, \
>> + v, false)
>> +#define VOP_INTR_SET_MASK(vop, name, mask, v) \
>> + REG_SET_MASK(vop, name, 0, vop->data->intr->name, \
>> + mask, v, false)
>> +
>> #define VOP_INTR_SET_TYPE(vop, name, type, v) \
>> do { \
>> int i, reg = 0, mask = 0; \
>> @@ -78,13 +105,16 @@
>> mask |= 1 << i; \
>> } \
>> } \
>> - VOP_INTR_SET(vop, name, mask, reg); \
>> + VOP_INTR_SET_MASK(vop, name, mask, reg); \
>> } while (0)
>> #define VOP_INTR_GET_TYPE(vop, name, type) \
>> vop_get_intr_type(vop, &vop->data->intr->name, type)
>>
>> +#define VOP_CTRL_GET(x, name) \
>> + vop_read_reg(x, 0, &vop->data->ctrl->name)
>> +
>> #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)
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> index 084d3b2..e4de890 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> @@ -15,6 +15,14 @@
>> #ifndef _ROCKCHIP_DRM_VOP_H
>> #define _ROCKCHIP_DRM_VOP_H
>>
>> +/*
>> + * major: IP major vertion, used for IP structure
> s/vertion/version
Got it, will fix it at next version.
Thanks.
>
>> + * minor: big feature change under same structure
>> + */
>> +#define VOP_VERSION(major, minor) ((major) << 8 | (minor))
>> +#define VOP_MAJOR(version) ((version) >> 8)
>> +#define VOP_MINOR(version) ((version) & 0xff)
>> +
>> enum vop_data_format {
>> VOP_FMT_ARGB8888 = 0,
>> VOP_FMT_RGB888,
>> @@ -25,10 +33,13 @@ enum vop_data_format {
>> };
>>
>> struct vop_reg {
>> - uint32_t offset;
>> - uint32_t shift;
>> uint32_t mask;
>> - bool write_mask;
>> + uint32_t offset:12;
>> + uint32_t shift:5;
>> + uint32_t begin_minor:4;
>> + uint32_t end_minor:4;
>> + uint32_t major:3;
>> + uint32_t write_mask:1;
> Why are you packing this?
make struct vop_reg not too big, jus u64 size, struct vop_reg use a lot on register define, packing it would reduce register table size.
>
>> };
>>
>> struct vop_ctrl {
>> @@ -134,6 +145,7 @@ struct vop_win_data {
>> };
>>
>> struct vop_data {
>> + uint32_t version;
>> const struct vop_ctrl *ctrl;
>> const struct vop_intr *intr;
>> const struct vop_win_data *win;
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>> index 00e9d79..7744603 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>> @@ -20,17 +20,25 @@
>> #include "rockchip_drm_vop.h"
>> #include "rockchip_vop_reg.h"
>>
>> -#define VOP_REG(off, _mask, s) \
>> +#define VOP_REG_VER_MASK(off, _mask, s, _write_mask, _major, \
>> + _begin_minor, _end_minor) \
>> {.offset = off, \
>> .mask = _mask, \
>> .shift = s, \
>> - .write_mask = false,}
>> + .write_mask = _write_mask, \
>> + .major = _major, \
>> + .begin_minor = _begin_minor, \
>> + .end_minor = _end_minor,}
>> +
>> +#define VOP_REG_VER(off, _mask, s, _major, _begin_minor, _end_minor) \
>> + VOP_REG_VER_MASK(off, _mask, s, false, \
>> + _major, _begin_minor, _end_minor)
>> +
>> +#define VOP_REG(off, _mask, s) \
>> + VOP_REG_VER(off, _mask, s, 0, 0, -1)
>>
>> #define VOP_REG_MASK(off, _mask, s) \
>> - {.offset = off, \
>> - .mask = _mask, \
>> - .shift = s, \
>> - .write_mask = true,}
>> + VOP_REG_VER_MASK(off, _mask, s, true, 0, 0, -1)
>>
>> static const uint32_t formats_win_full[] = {
>> DRM_FORMAT_XRGB8888,
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
?ark Yao
WARNING: multiple messages have this Message-ID (diff)
From: Mark yao <mark.yao@rock-chips.com>
To: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>, Heiko Stuebner <heiko@sntech.de>,
linux-rockchip@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version
Date: Thu, 13 Jul 2017 09:45:12 +0800 [thread overview]
Message-ID: <5966D0A8.6060003@rock-chips.com> (raw)
In-Reply-To: <20170712175305.n7kk7dzvfs2vywz3@art_vandelay>
On 2017年07月13日 01:53, Sean Paul wrote:
> On Wed, Jul 12, 2017 at 10:03:38AM +0800, Mark Yao wrote:
>
> Please add a commit message describing *what* and *why* you are making the
> change.
Got it, I will fix it at next version.
Thanks.
>
>> Signed-off-by: Mark Yao <mark.yao@rock-chips.com>
>> ---
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 66 +++++++++++++++++++++--------
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 18 ++++++--
>> drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 20 ++++++---
>> 3 files changed, 77 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 7a5f809..a9180fd 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -42,33 +42,60 @@
>> #include "rockchip_drm_psr.h"
>> #include "rockchip_drm_vop.h"
>>
>> -#define __REG_SET_RELAXED(x, off, mask, shift, v, write_mask) \
>> - vop_mask_write(x, off, mask, shift, v, write_mask, true)
>> +#define VOP_REG_SUPPORT(vop, reg) \
>> + (!reg.major || (reg.major == VOP_MAJOR(vop->data->version) && \
>> + reg.begin_minor <= VOP_MINOR(vop->data->version) && \
>> + reg.end_minor >= VOP_MINOR(vop->data->version) && \
>> + reg.mask))
> This would be better suited as a static inline function. As would many of the
> macros below.
Got it, will fix it at next version.
>
>>
>> -#define __REG_SET_NORMAL(x, off, mask, shift, v, write_mask) \
>> - vop_mask_write(x, off, mask, shift, v, write_mask, false)
>> +#define VOP_WIN_SUPPORT(vop, win, name) \
>> + VOP_REG_SUPPORT(vop, win->phy->name)
>>
>> -#define REG_SET(x, base, reg, v, mode) \
>> - __REG_SET_##mode(x, base + reg.offset, \
>> - reg.mask, reg.shift, v, reg.write_mask)
>> -#define REG_SET_MASK(x, base, reg, mask, v, mode) \
>> - __REG_SET_##mode(x, base + reg.offset, \
>> - mask, reg.shift, v, reg.write_mask)
>> +#define VOP_CTRL_SUPPORT(vop, name) \
>> + VOP_REG_SUPPORT(vop, vop->data->ctrl->name)
>> +
>> +#define VOP_INTR_SUPPORT(vop, name) \
>> + VOP_REG_SUPPORT(vop, vop->data->intr->name)
>> +
>> +#define __REG_SET(x, off, mask, shift, v, write_mask, relaxed) \
>> + vop_mask_write(x, off, mask, shift, v, write_mask, relaxed)
> There's really no point to this, just call vop_mask_write directly.
Got it, will fix it at next version.
>
>> +
>> +#define _REG_SET(vop, name, off, reg, mask, v, relaxed) \
>> + do { \
>> + if (VOP_REG_SUPPORT(vop, reg)) \
>> + __REG_SET(vop, off + reg.offset, mask, reg.shift, \
> s/mask/reg.mask & mask/
Got it, will fix it at next version.
>
>> + v, reg.write_mask, relaxed); \
>> + else \
>> + dev_dbg(vop->dev, "Warning: not support "#name"\n"); \
>> + } while (0)
>
> Also better as static inline, IMO.
Good idea, I will try it.
>
>> +
>> +#define REG_SET(x, name, off, reg, v, relaxed) \
>> + _REG_SET(x, name, off, reg, reg.mask, v, relaxed)
> s/reg.mask/~0/
Got it, will fix it at next version.
>
>> +#define REG_SET_MASK(x, name, off, reg, mask, v, relaxed) \
>> + _REG_SET(x, name, off, reg, reg.mask & mask, v, relaxed)
> s/reg.mask &//
>
> Also, these can become static inline functions as well.
Got it, will fix it at next version.
>
>>
>> #define VOP_WIN_SET(x, win, name, v) \
>> - REG_SET(x, win->base, win->phy->name, v, RELAXED)
>> + REG_SET(x, name, win->base, win->phy->name, v, true)
>> +#define VOP_WIN_SET_EXT(x, win, ext, name, v) \
>> + REG_SET(x, name, 0, win->ext->name, v, true)
>> #define VOP_SCL_SET(x, win, name, v) \
>> - REG_SET(x, win->base, win->phy->scl->name, v, RELAXED)
>> + REG_SET(x, name, win->base, win->phy->scl->name, v, true)
>> #define VOP_SCL_SET_EXT(x, win, name, v) \
>> - REG_SET(x, win->base, win->phy->scl->ext->name, v, RELAXED)
>> + REG_SET(x, name, win->base, win->phy->scl->ext->name, v, true)
>> +
>> #define VOP_CTRL_SET(x, name, v) \
>> - REG_SET(x, 0, (x)->data->ctrl->name, v, NORMAL)
>> + REG_SET(x, name, 0, (x)->data->ctrl->name, v, false)
>>
>> #define VOP_INTR_GET(vop, name) \
>> vop_read_reg(vop, 0, &vop->data->ctrl->name)
>>
>> -#define VOP_INTR_SET(vop, name, mask, v) \
>> - REG_SET_MASK(vop, 0, vop->data->intr->name, mask, v, NORMAL)
>> +#define VOP_INTR_SET(vop, name, v) \
>> + REG_SET(vop, name, 0, vop->data->intr->name, \
>> + v, false)
>> +#define VOP_INTR_SET_MASK(vop, name, mask, v) \
>> + REG_SET_MASK(vop, name, 0, vop->data->intr->name, \
>> + mask, v, false)
>> +
>> #define VOP_INTR_SET_TYPE(vop, name, type, v) \
>> do { \
>> int i, reg = 0, mask = 0; \
>> @@ -78,13 +105,16 @@
>> mask |= 1 << i; \
>> } \
>> } \
>> - VOP_INTR_SET(vop, name, mask, reg); \
>> + VOP_INTR_SET_MASK(vop, name, mask, reg); \
>> } while (0)
>> #define VOP_INTR_GET_TYPE(vop, name, type) \
>> vop_get_intr_type(vop, &vop->data->intr->name, type)
>>
>> +#define VOP_CTRL_GET(x, name) \
>> + vop_read_reg(x, 0, &vop->data->ctrl->name)
>> +
>> #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)
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> index 084d3b2..e4de890 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> @@ -15,6 +15,14 @@
>> #ifndef _ROCKCHIP_DRM_VOP_H
>> #define _ROCKCHIP_DRM_VOP_H
>>
>> +/*
>> + * major: IP major vertion, used for IP structure
> s/vertion/version
Got it, will fix it at next version.
Thanks.
>
>> + * minor: big feature change under same structure
>> + */
>> +#define VOP_VERSION(major, minor) ((major) << 8 | (minor))
>> +#define VOP_MAJOR(version) ((version) >> 8)
>> +#define VOP_MINOR(version) ((version) & 0xff)
>> +
>> enum vop_data_format {
>> VOP_FMT_ARGB8888 = 0,
>> VOP_FMT_RGB888,
>> @@ -25,10 +33,13 @@ enum vop_data_format {
>> };
>>
>> struct vop_reg {
>> - uint32_t offset;
>> - uint32_t shift;
>> uint32_t mask;
>> - bool write_mask;
>> + uint32_t offset:12;
>> + uint32_t shift:5;
>> + uint32_t begin_minor:4;
>> + uint32_t end_minor:4;
>> + uint32_t major:3;
>> + uint32_t write_mask:1;
> Why are you packing this?
make struct vop_reg not too big, jus u64 size, struct vop_reg use a lot on register define, packing it would reduce register table size.
>
>> };
>>
>> struct vop_ctrl {
>> @@ -134,6 +145,7 @@ struct vop_win_data {
>> };
>>
>> struct vop_data {
>> + uint32_t version;
>> const struct vop_ctrl *ctrl;
>> const struct vop_intr *intr;
>> const struct vop_win_data *win;
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>> index 00e9d79..7744603 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>> @@ -20,17 +20,25 @@
>> #include "rockchip_drm_vop.h"
>> #include "rockchip_vop_reg.h"
>>
>> -#define VOP_REG(off, _mask, s) \
>> +#define VOP_REG_VER_MASK(off, _mask, s, _write_mask, _major, \
>> + _begin_minor, _end_minor) \
>> {.offset = off, \
>> .mask = _mask, \
>> .shift = s, \
>> - .write_mask = false,}
>> + .write_mask = _write_mask, \
>> + .major = _major, \
>> + .begin_minor = _begin_minor, \
>> + .end_minor = _end_minor,}
>> +
>> +#define VOP_REG_VER(off, _mask, s, _major, _begin_minor, _end_minor) \
>> + VOP_REG_VER_MASK(off, _mask, s, false, \
>> + _major, _begin_minor, _end_minor)
>> +
>> +#define VOP_REG(off, _mask, s) \
>> + VOP_REG_VER(off, _mask, s, 0, 0, -1)
>>
>> #define VOP_REG_MASK(off, _mask, s) \
>> - {.offset = off, \
>> - .mask = _mask, \
>> - .shift = s, \
>> - .write_mask = true,}
>> + VOP_REG_VER_MASK(off, _mask, s, true, 0, 0, -1)
>>
>> static const uint32_t formats_win_full[] = {
>> DRM_FORMAT_XRGB8888,
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ｍark Yao
next prev parent reply other threads:[~2017-07-13 1:45 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-12 2:03 [PATCH v2 0/5] drm/rockchip: add all full framework vop support Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 2:03 ` [PATCH v2 1/5] drm/rockchip: vop: get rid of register init table Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 16:47 ` Sean Paul
2017-07-12 16:47 ` Sean Paul
2017-07-12 16:47 ` Sean Paul
2017-07-13 1:33 ` Mark yao
2017-07-13 1:33 ` Mark yao
2017-07-13 20:29 ` Sean Paul
2017-07-13 20:29 ` Sean Paul
2017-07-13 20:29 ` Sean Paul
2017-07-17 2:10 ` Mark yao
2017-07-17 2:10 ` Mark yao
2017-07-17 2:10 ` Mark yao
2017-07-12 2:03 ` [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 17:53 ` Sean Paul
2017-07-12 17:53 ` Sean Paul
2017-07-12 17:53 ` Sean Paul
2017-07-13 1:45 ` Mark yao [this message]
2017-07-13 1:45 ` Mark yao
2017-07-13 1:45 ` Mark yao
2017-07-13 20:33 ` Sean Paul
2017-07-13 20:33 ` Sean Paul
2017-07-13 20:33 ` Sean Paul
2017-07-12 2:03 ` [PATCH v2 3/5] drm/rockchip: vop: move line_flag_num to interrupt registers Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 17:54 ` Sean Paul
2017-07-12 17:54 ` Sean Paul
2017-07-12 17:54 ` Sean Paul
2017-07-13 1:46 ` Mark yao
2017-07-13 1:46 ` Mark yao
2017-07-13 1:46 ` Mark yao
2017-07-12 2:03 ` [PATCH v2 4/5] drm/rockchip: vop: add a series of vop support Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 2:03 ` Mark Yao
2017-07-12 18:33 ` Sean Paul
2017-07-12 18:33 ` Sean Paul
2017-07-12 18:33 ` Sean Paul
2017-07-13 1:31 ` Mark yao
2017-07-13 20:28 ` Sean Paul
2017-07-13 20:28 ` Sean Paul
2017-07-13 20:28 ` Sean Paul
2017-07-12 2:04 ` [PATCH v2 5/5] dt-bindings: display: fill Documents for series of vop Mark Yao
2017-07-12 2:04 ` Mark Yao
2017-07-12 2:04 ` Mark Yao
[not found] ` <1499825042-7652-1-git-send-email-mark.yao-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2017-07-17 17:24 ` Rob Herring
2017-07-17 17:24 ` Rob Herring
2017-07-17 17:24 ` Rob Herring
2017-07-20 2:10 ` Mark yao
2017-07-20 2:10 ` Mark yao
2017-07-20 2:10 ` Mark yao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5966D0A8.6060003@rock-chips.com \
--to=mark.yao@rock-chips.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=seanpaul@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.