Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751018AbdGMBpW (ORCPT ); Wed, 12 Jul 2017 21:45:22 -0400 Received: from lucky1.263xmail.com ([211.157.147.132]:44718 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750755AbdGMBpV (ORCPT ); Wed, 12 Jul 2017 21:45:21 -0400 X-263anti-spam: KSV:0;BIG:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ADDR-CHECKED4: 1 X-ABS-CHECKED: 0 X-SKE-CHECKED: 0 X-ANTISPAM-LEVEL: 2 X-RL-SENDER: mark.yao@rock-chips.com X-FST-TO: linux-kernel@vger.kernel.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: mark.yao@rock-chips.com X-UNIQUE-TAG: <0f4deff924c5227ba1887e88464a5c7b> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v2 2/5] drm/rockchip: vop: support verify registers with vop version To: Sean Paul References: <1499824991-7391-1-git-send-email-mark.yao@rock-chips.com> <1499825019-7503-1-git-send-email-mark.yao@rock-chips.com> <20170712175305.n7kk7dzvfs2vywz3@art_vandelay> Cc: David Airlie , Heiko Stuebner , linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org From: Mark yao Message-ID: <5966D0A8.6060003@rock-chips.com> Date: Thu, 13 Jul 2017 09:45:12 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20170712175305.n7kk7dzvfs2vywz3@art_vandelay> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8234 Lines: 250 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 >> --- >> 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