Re: [PATCH 1/1] drm/mediatek: Add support for mediatek SOC MT2712
Hi, CK: I've some reply for comment On Tue, 2018-05-15 at 10:30 +0800, CK Hu wrote: > Hi, Stu: > > I've some inline comments. > > On Mon, 2018-05-14 at 17:59 +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> > --- > > drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 77 > > ++--- > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 4 ++ > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 3 ++ > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 44 + > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 7 ++- > > 5 files changed, 127 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > > b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > > index 8130f3dab661..641f4361b006 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > > @@ -29,6 +29,8 @@ > > #define DISP_REG_CONFIG_DISP_COLOR0_SEL_IN 0x084 > > #define DISP_REG_CONFIG_DISP_COLOR1_SEL_IN 0x088 > > #define DISP_REG_CONFIG_DPI_SEL_IN 0x0ac > > +#define DISP_REG_CONFIG_DISP_RDMA2_SOUT0x0b8 > > +#define DISP_REG_CONFIG_DISP_RDMA0_MOUT_EN 0x0c4 > > #define DISP_REG_CONFIG_DISP_RDMA1_MOUT_EN 0x0c8 > > #define DISP_REG_CONFIG_MMSYS_CG_CON0 0x100 > > > > @@ -41,6 +43,7 @@ > > #define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * (n)) > > #define DISP_REG_MUTEX_MOD(n) (0x2c + 0x20 * (n)) > > #define DISP_REG_MUTEX_SOF(n) (0x30 + 0x20 * (n)) > > +#define DISP_REG_MUTEX_MOD2(n) (0x34 + 0x20 * (n)) > > > > #define INT_MUTEX BIT(1) > > > > @@ -60,6 +63,25 @@ > > #define MT8173_MUTEX_MOD_DISP_PWM1 BIT(24) > > #define MT8173_MUTEX_MOD_DISP_OD BIT(25) > > > > +#define MT2712_MUTEX_MOD_DISP_OVL0 BIT(11) > > +#define MT2712_MUTEX_MOD_DISP_OVL1 BIT(12) > > +#define MT2712_MUTEX_MOD_DISP_RDMA0BIT(13) > > +#define MT2712_MUTEX_MOD_DISP_RDMA1BIT(14) > > +#define MT2712_MUTEX_MOD_DISP_RDMA2BIT(15) > > +#define MT2712_MUTEX_MOD_DISP_WDMA0BIT(16) > > +#define MT2712_MUTEX_MOD_DISP_WDMA1BIT(17) > > +#define MT2712_MUTEX_MOD_DISP_COLOR0 BIT(18) > > +#define MT2712_MUTEX_MOD_DISP_COLOR1 BIT(19) > > +#define MT2712_MUTEX_MOD_DISP_AAL 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_OD 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)) > > +#define MT2712_MUTEX_MOD2_DISP_OD1 (BIT(2) | BIT(31)) > > + > > It looks like that MUTEX_MOD definition varies for each SoC. I think > such definition should be passed from dts to prevent modify driver for > each SoC. For example, the clock definition varies for each SoC, and its > definition is placed in [1]. The dts [2] include the header file and > pass the clock definition to driver. > [1] > https://elixir.bootlin.com/linux/v4.17-rc5/source/include/dt-bindings/clock/mt2712-clk.h > > [2] > https://elixir.bootlin.com/linux/v4.17-rc5/source/arch/arm64/boot/dts/mediatek/mt2712e.dtsi > This idea is good but weak relation for this MT2712 patch In the future, we would commit other patch serial for this issue > > #define MT2701_MUTEX_MOD_DISP_OVL BIT(3) > > #define MT2701_MUTEX_MOD_DISP_WDMA BIT(6) > > #define MT2701_MUTEX_MOD_DISP_COLORBIT(7) > > @@ -74,6 +96,7 @@ > > > > #define OVL0_MOUT_EN_COLOR00x1 > > #define OD_MOUT_EN_RDMA0 0x1 > > +#define OD1_MOUT_EN_RDMA1 BIT(16) > > #define UFOE_MOUT_EN_DSI0 0x1 > > #define COLOR0_SEL_IN_OVL0 0x1 > > #define OVL1_MOUT_EN_COLOR10x1 > > @@ -108,6 +131,26 @@ static const unsigned int > > mt2701_mutex_mod[DDP_COMPONENT_ID_MAX] = { > > [DDP_COMPONENT_WDMA0] = MT2701_MUTEX_MOD_DISP_WDMA, > > }; > > > > +static const unsigned int mt2712_mutex_mod[DDP_COMPONENT_ID_MAX] = { > > + [DDP_COMPONENT_AAL] = MT2712_MUTEX_MOD_DISP_AAL, > > + [DDP_COMPONENT_AAL1] = MT2712_MUTEX_MOD2_DISP_AAL1, > > + [DDP_COMPONENT_COLOR0] = MT2712_MUTEX_MOD_DISP_COLOR0, > > + [DDP_COMPONENT_COLOR1] = MT2712_MUTEX_MOD_DISP_COLOR1, > > + [DDP_COMPONENT_OD] = MT2712_MUTEX_MOD_DISP_OD, > > + [DDP_COMPONENT_OD1] = MT2712_MUTEX_MOD2_DISP_OD1, > > + [DDP_COMPONENT_OVL0] = MT2712_MUTEX_MOD_DISP_OVL0, > > + [DDP_COMPONENT_OVL1] = MT2712_MUTEX_MOD_DISP_OVL1, > > + [DDP_COMPONENT_PWM0] = MT2712_MUTEX_MOD_DISP_PWM0, > > + [DDP_COMPONENT_PWM1] =
Re: [PATCH 1/1] drm/mediatek: Add support for mediatek SOC MT2712
Hi, Stu: On Tue, 2018-05-22 at 15:47 +0800, Stu Hsieh wrote: > Hi, CK: > > I've some reply for comment > > On Tue, 2018-05-15 at 10:30 +0800, CK Hu wrote: > > Hi, Stu: > > > > I've some inline comments. > > > > On Mon, 2018-05-14 at 17:59 +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> > > --- > > > drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 77 > > > ++--- > > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 4 ++ > > > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 3 ++ > > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 44 + > > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 7 ++- > > > 5 files changed, 127 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > > > b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > > > index 8130f3dab661..641f4361b006 100644 > > > > > > #define INT_MUTEXBIT(1) > > > > > > @@ -60,6 +63,25 @@ > > > #define MT8173_MUTEX_MOD_DISP_PWM1 BIT(24) > > > #define MT8173_MUTEX_MOD_DISP_OD BIT(25) > > > > > > +#define MT2712_MUTEX_MOD_DISP_OVL0 BIT(11) > > > +#define MT2712_MUTEX_MOD_DISP_OVL1 BIT(12) > > > +#define MT2712_MUTEX_MOD_DISP_RDMA0 BIT(13) > > > +#define MT2712_MUTEX_MOD_DISP_RDMA1 BIT(14) > > > +#define MT2712_MUTEX_MOD_DISP_RDMA2 BIT(15) > > > +#define MT2712_MUTEX_MOD_DISP_WDMA0 BIT(16) > > > +#define MT2712_MUTEX_MOD_DISP_WDMA1 BIT(17) > > > +#define MT2712_MUTEX_MOD_DISP_COLOR0 BIT(18) > > > +#define MT2712_MUTEX_MOD_DISP_COLOR1 BIT(19) > > > +#define MT2712_MUTEX_MOD_DISP_AALBIT(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_OD 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)) > > > +#define MT2712_MUTEX_MOD2_DISP_OD1 (BIT(2) | BIT(31)) > > > + > > > > It looks like that MUTEX_MOD definition varies for each SoC. I think > > such definition should be passed from dts to prevent modify driver for > > each SoC. For example, the clock definition varies for each SoC, and its > > definition is placed in [1]. The dts [2] include the header file and > > pass the clock definition to driver. > > [1] > > https://elixir.bootlin.com/linux/v4.17-rc5/source/include/dt-bindings/clock/mt2712-clk.h > > > > [2] > > https://elixir.bootlin.com/linux/v4.17-rc5/source/arch/arm64/boot/dts/mediatek/mt2712e.dtsi > > > This idea is good but weak relation for this MT2712 patch > In the future, we would commit other patch serial for this issue > If you have a plan to pass these definition from device tree, for me, it's ok to temporarily write down the definition in driver. > > > #define MT2701_MUTEX_MOD_DISP_OVLBIT(3) > > > #define MT2701_MUTEX_MOD_DISP_WDMA BIT(6) > > > #define MT2701_MUTEX_MOD_DISP_COLOR BIT(7) > > > @@ -74,6 +96,7 @@ > > > > > > #define OVL0_MOUT_EN_COLOR0 0x1 > > > #define OD_MOUT_EN_RDMA0 0x1 > > > +#define OD1_MOUT_EN_RDMA1BIT(16) > > > #define UFOE_MOUT_EN_DSI00x1 > > > #define COLOR0_SEL_IN_OVL0 0x1 > > > #define OVL1_MOUT_EN_COLOR1 0x1 > > > @@ -108,6 +131,26 @@ static const unsigned int > > > mt2701_mutex_mod[DDP_COMPONENT_ID_MAX] = { > > > [DDP_COMPONENT_WDMA0] = MT2701_MUTEX_MOD_DISP_WDMA, > > > }; > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > > index a2ca90fc403c..41baf6653bfc 100644 > > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > > @@ -146,6 +146,32 @@ static const enum mtk_ddp_comp_id > > > mt2701_mtk_ddp_ext[] = { > > > DDP_COMPONENT_DPI0, > > > }; > > > > > > +static const enum mtk_ddp_comp_id mt2712_mtk_ddp_main[] = { > > > + DDP_COMPONENT_OVL0, > > > + DDP_COMPONENT_COLOR0, > > > + DDP_COMPONENT_AAL, > > > + DDP_COMPONENT_OD, > > > + DDP_COMPONENT_RDMA0, > > > + DDP_COMPONENT_DPI0, > > > + DDP_COMPONENT_PWM0, > > > +}; > > > + > > > +static const enum mtk_ddp_comp_id mt2712_mtk_ddp_ext[] = { > > > + DDP_COMPONENT_OVL1, > > > + DDP_COMPONENT_COLOR1, > > > + DDP_COMPONENT_AAL1, > > > + DDP_COMPONENT_OD1, > > > + DDP_COMPONENT_RDMA1, > > > + DDP_COMPONENT_DPI1, > > > + DDP_COMPONENT_PWM1, > > > +}; > > > + > > > +static const enum mtk_ddp_comp_id
Re: [PATCH 1/1] drm/mediatek: Add support for mediatek SOC MT2712
Hi, Stu: I've some inline comments. On Mon, 2018-05-14 at 17:59 +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> --- > drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 77 > ++--- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 4 ++ > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 3 ++ > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 44 + > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 7 ++- > 5 files changed, 127 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > index 8130f3dab661..641f4361b006 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c > @@ -29,6 +29,8 @@ > #define DISP_REG_CONFIG_DISP_COLOR0_SEL_IN 0x084 > #define DISP_REG_CONFIG_DISP_COLOR1_SEL_IN 0x088 > #define DISP_REG_CONFIG_DPI_SEL_IN 0x0ac > +#define DISP_REG_CONFIG_DISP_RDMA2_SOUT 0x0b8 > +#define DISP_REG_CONFIG_DISP_RDMA0_MOUT_EN 0x0c4 > #define DISP_REG_CONFIG_DISP_RDMA1_MOUT_EN 0x0c8 > #define DISP_REG_CONFIG_MMSYS_CG_CON00x100 > > @@ -41,6 +43,7 @@ > #define DISP_REG_MUTEX_RST(n)(0x28 + 0x20 * (n)) > #define DISP_REG_MUTEX_MOD(n)(0x2c + 0x20 * (n)) > #define DISP_REG_MUTEX_SOF(n)(0x30 + 0x20 * (n)) > +#define DISP_REG_MUTEX_MOD2(n) (0x34 + 0x20 * (n)) > > #define INT_MUTEXBIT(1) > > @@ -60,6 +63,25 @@ > #define MT8173_MUTEX_MOD_DISP_PWM1 BIT(24) > #define MT8173_MUTEX_MOD_DISP_OD BIT(25) > > +#define MT2712_MUTEX_MOD_DISP_OVL0 BIT(11) > +#define MT2712_MUTEX_MOD_DISP_OVL1 BIT(12) > +#define MT2712_MUTEX_MOD_DISP_RDMA0 BIT(13) > +#define MT2712_MUTEX_MOD_DISP_RDMA1 BIT(14) > +#define MT2712_MUTEX_MOD_DISP_RDMA2 BIT(15) > +#define MT2712_MUTEX_MOD_DISP_WDMA0 BIT(16) > +#define MT2712_MUTEX_MOD_DISP_WDMA1 BIT(17) > +#define MT2712_MUTEX_MOD_DISP_COLOR0 BIT(18) > +#define MT2712_MUTEX_MOD_DISP_COLOR1 BIT(19) > +#define MT2712_MUTEX_MOD_DISP_AALBIT(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_OD 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)) > +#define MT2712_MUTEX_MOD2_DISP_OD1 (BIT(2) | BIT(31)) > + It looks like that MUTEX_MOD definition varies for each SoC. I think such definition should be passed from dts to prevent modify driver for each SoC. For example, the clock definition varies for each SoC, and its definition is placed in [1]. The dts [2] include the header file and pass the clock definition to driver. [1] https://elixir.bootlin.com/linux/v4.17-rc5/source/include/dt-bindings/clock/mt2712-clk.h [2] https://elixir.bootlin.com/linux/v4.17-rc5/source/arch/arm64/boot/dts/mediatek/mt2712e.dtsi > #define MT2701_MUTEX_MOD_DISP_OVLBIT(3) > #define MT2701_MUTEX_MOD_DISP_WDMA BIT(6) > #define MT2701_MUTEX_MOD_DISP_COLOR BIT(7) > @@ -74,6 +96,7 @@ > > #define OVL0_MOUT_EN_COLOR0 0x1 > #define OD_MOUT_EN_RDMA0 0x1 > +#define OD1_MOUT_EN_RDMA1BIT(16) > #define UFOE_MOUT_EN_DSI00x1 > #define COLOR0_SEL_IN_OVL0 0x1 > #define OVL1_MOUT_EN_COLOR1 0x1 > @@ -108,6 +131,26 @@ static const unsigned int > mt2701_mutex_mod[DDP_COMPONENT_ID_MAX] = { > [DDP_COMPONENT_WDMA0] = MT2701_MUTEX_MOD_DISP_WDMA, > }; > > +static const unsigned int mt2712_mutex_mod[DDP_COMPONENT_ID_MAX] = { > + [DDP_COMPONENT_AAL] = MT2712_MUTEX_MOD_DISP_AAL, > + [DDP_COMPONENT_AAL1] = MT2712_MUTEX_MOD2_DISP_AAL1, > + [DDP_COMPONENT_COLOR0] = MT2712_MUTEX_MOD_DISP_COLOR0, > + [DDP_COMPONENT_COLOR1] = MT2712_MUTEX_MOD_DISP_COLOR1, > + [DDP_COMPONENT_OD] = MT2712_MUTEX_MOD_DISP_OD, > + [DDP_COMPONENT_OD1] = MT2712_MUTEX_MOD2_DISP_OD1, > + [DDP_COMPONENT_OVL0] = MT2712_MUTEX_MOD_DISP_OVL0, > + [DDP_COMPONENT_OVL1] = MT2712_MUTEX_MOD_DISP_OVL1, > + [DDP_COMPONENT_PWM0] = MT2712_MUTEX_MOD_DISP_PWM0, > + [DDP_COMPONENT_PWM1] = MT2712_MUTEX_MOD_DISP_PWM1, > + [DDP_COMPONENT_PWM2] = MT2712_MUTEX_MOD_DISP_PWM2, > + [DDP_COMPONENT_RDMA0] = MT2712_MUTEX_MOD_DISP_RDMA0, > + [DDP_COMPONENT_RDMA1] = MT2712_MUTEX_MOD_DISP_RDMA1, > + [DDP_COMPONENT_RDMA2] = MT2712_MUTEX_MOD_DISP_RDMA2, > + [DDP_COMPONENT_UFOE] = MT2712_MUTEX_MOD_DISP_UFOE, > + [DDP_COMPONENT_WDMA0] = MT2712_MUTEX_MOD_DISP_WDMA0, > +