Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1527125197.6406.10.camel@mtksdaap41>
Subject: Re: [PATCH v2 2/4] drm/mediatek: Add support for mediatek SOC MT2712
From:   CK Hu <ck.hu@mediatek.com>
To:     Stu Hsieh <stu.hsieh@mediatek.com>
CC:     Philipp Zabel <p.zabel@pengutronix.de>,
        David Airlie <airlied@linux.ie>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        <dri-devel@lists.freedesktop.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-mediatek@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <srv_heupstream@mediatek.com>
Date:   Thu, 24 May 2018 09:26:37 +0800
In-Reply-To: <1527067696.19074.28.camel@mtksdaap41>
References: <20180523022517.12103-1-stu.hsieh@mediatek.com>
         <20180523022517.12103-3-stu.hsieh@mediatek.com>
         <1527053036.10410.18.camel@mtksdaap41>
         <1527067696.19074.28.camel@mtksdaap41>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi, Stu:

On Wed, 2018-05-23 at 17:28 +0800, Stu Hsieh wrote:
> On Wed, 2018-05-23 at 13:23 +0800, CK Hu wrote:
> > Hi, Stu:
> > 
> > I've some inline comment.
> > 
> > On Wed, 2018-05-23 at 10:25 +0800, Stu Hsieh wrote:
> > > This patch add support for the Mediatek MT2712 DISP subsystem.
> > > There are two OVL engine and three disp output in MT2712.
> > > 
> > > Signed-off-by: Stu Hsieh <stu.hsieh@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_drm_ddp.c      | 50 +++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |  8 +++--
> > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  7 ++--
> > >  drivers/gpu/drm/mediatek/mtk_drm_drv.c      | 47 +++++++++++++++++++++++++--
> > >  drivers/gpu/drm/mediatek/mtk_drm_drv.h      |  7 ++--
> > >  5 files changed, 108 insertions(+), 11 deletions(-)
> > > 
> > > +#define MT2712_MUTEX_MOD_DISP_AAL0		BIT(20)
> > > +#define MT2712_MUTEX_MOD_DISP_UFOE		BIT(22)
> > > +#define MT2712_MUTEX_MOD_DISP_PWM0		BIT(23)
> > > +#define MT2712_MUTEX_MOD_DISP_PWM1		BIT(24)
> > > +#define MT2712_MUTEX_MOD_DISP_PWM2		BIT(10)
> > > +#define MT2712_MUTEX_MOD_DISP_OD0		BIT(25)
> > > +/* modules more than 32, add BIT(31) when using DISP_REG_MUTEX_MOD2 bit */
> > > +#define MT2712_MUTEX_MOD2_DISP_AAL1		(BIT(1) | BIT(31))
> > 
> > I think a better definition is
> > 
> > #define MT2712_MUTEX_MOD2_DISP_AAL1		BIT(33)
> > 
> > when you need to access this register,
> > 
> > if (ddp->mutex_mod[id] < BIT(32)) {
> > 	offset = DISP_REG_MUTEX_MOD(mutex->id);
> > 	reg = readl_relaxed(ddp->regs + offset);
> > 	reg |= ddp->mutex_mod[id];
> > 	writel_relaxed(reg, ddp->regs + offset);
> > } else {
> > 	offset = DISP_REG_MUTEX_MOD2(mutex->id);
> > 	reg = readl_relaxed(ddp->regs + offset);
> > 	reg |= (ddp->mutex_mod[id] >> 32);
> > 	writel_relaxed(reg, ddp->regs + offset);
> > }
> > 
> > because DISP_REG_MUTEX_MOD BIT(31) could be used for some module.
> 
> This modification is workable, but result some build warning like
> following:
> 1. #define BIT(nr)   (1UL << (nr)) in include/linux/bitops.h
> 2. [DDP_COMPONENT_AAL1] = MT2712_MUTEX_MOD2_DISP_AAL1,
>    => we need to modify the definition about "static const unsigned int
> mt2712_mutex_mod"
> 

Currently, mutex_mod is a bitwise definition. I think it could be
changed to index definition such as


#define MT2712_MUTEX_MOD_DISP_PWM2		10
#define MT2712_MUTEX_MOD_DISP_OD0		25
#define MT2712_MUTEX_MOD2_DISP_AAL1		33

when you need to access this register,

if (ddp->mutex_mod[id] < 32) {
	offset = DISP_REG_MUTEX_MOD(mutex->id);
	reg = readl_relaxed(ddp->regs + offset);
	reg |= 1 << ddp->mutex_mod[id];
	writel_relaxed(reg, ddp->regs + offset);
} else {
	offset = DISP_REG_MUTEX_MOD2(mutex->id);
	reg = readl_relaxed(ddp->regs + offset);
	reg |= 1 << (ddp->mutex_mod[id] - 32);
	writel_relaxed(reg, ddp->regs + offset);
}

Regards,
CK

> > > +#define MT2712_MUTEX_MOD2_DISP_OD1		(BIT(2) | BIT(31))
> > > +
> > >  #define MT2701_MUTEX_MOD_DISP_OVL		BIT(3)
> > >  #define MT2701_MUTEX_MOD_DISP_WDMA		BIT(6)
> > >  #define MT2701_MUTEX_MOD_DISP_COLOR		BIT(7)
> > > @@ -74,6 +96,7 @@
> > >  
> > >  
> > 
> > 
> 
>