Re: [PATCH v2 2/4] drm/mediatek: Add support for mediatek SOC MT2712

[CK Hu]

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@xxxxxxxxxxxx>
> > > ---
> > >  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 @@
> > >  
> > >  
> > 
> > 
> 
>