| Message ID | 20190214044243.129920-3-jitao.shi@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/mediatek: move mipi_dsi_host_register to probe | expand |
On Thu, Feb 14, 2019 at 12:43 PM Jitao Shi <jitao.shi@mediatek.com> wrote: > > MT8183 dsi has two changes with mt8173. > 1. Add the register double buffer control, but we no need it, So make > it default off. Can you describe a little bit more what this is about? That's shadow registers, right? > 2. Add picture size control. > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dsi.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 80db02a25cb0..20cb53f05d42 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -78,6 +78,7 @@ > #define DSI_VBP_NL 0x24 > #define DSI_VFP_NL 0x28 > #define DSI_VACT_NL 0x2C > +#define DSI_SIZE_CON 0x38 > #define DSI_HSA_WC 0x50 > #define DSI_HBP_WC 0x54 > #define DSI_HFP_WC 0x58 > @@ -131,7 +132,10 @@ > #define VM_CMD_EN BIT(0) > #define TS_VFP_EN BIT(5) > > -#define DSI_CMDQ0 0x180 As I said earlier, move this to 2/3. > +#define DSI_SHADOW_DEBUG 0x190U > +#define FORCE_COMMIT BIT(0) > +#define BYPASS_SHADOW BIT(1) > + > #define CONFIG (0xff << 0) > #define SHORT_PACKET 0 > #define LONG_PACKET 2 > @@ -158,6 +162,7 @@ struct phy; > > struct mtk_dsi_driver_data { > const u32 reg_cmdq_off; > + bool has_size_ctl; > }; > > struct mtk_dsi { > @@ -426,6 +431,9 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL); > writel(vm->vactive, dsi->regs + DSI_VACT_NL); > > + if (dsi->driver_data->has_size_ctl) > + writel(vm->vactive << 16 | vm->hactive, dsi->regs + DSI_SIZE_CON); > + > horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10); > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) > @@ -595,6 +603,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) > } > > mtk_dsi_enable(dsi); > + > + /* DSI no need this double buffer, disable it when writing register */ "DSI does not need double buffering, disable it when writing register" > + writel(FORCE_COMMIT | BYPASS_SHADOW, dsi->regs + DSI_SHADOW_DEBUG); So you do this on all MT* variants, is that ok? > mtk_dsi_reset_engine(dsi); > mtk_dsi_phy_timconfig(dsi); > > @@ -1090,11 +1101,18 @@ static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = { > .reg_cmdq_off = 0x180, > }; > > +static const struct mtk_dsi_driver_data mt8183_dsi_driver_data = { > + .reg_cmdq_off = 0x200, > + .has_size_ctl = true, > +}; > + > static const struct of_device_id mtk_dsi_of_match[] = { > { .compatible = "mediatek,mt2701-dsi", > .data = &mt2701_dsi_driver_data }, > { .compatible = "mediatek,mt8173-dsi", > .data = &mt8173_dsi_driver_data }, > + { .compatible = "mediatek,mt8183-dsi", > + .data = &mt8183_dsi_driver_data }, > { }, > }; > > -- > 2.20.1 >
On 14/02/2019 05:42, Jitao Shi wrote: > MT8183 dsi has two changes with mt8173. > 1. Add the register double buffer control, but we no need it, So make > it default off. > 2. Add picture size control. > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_dsi.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 80db02a25cb0..20cb53f05d42 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -78,6 +78,7 @@ > #define DSI_VBP_NL 0x24 > #define DSI_VFP_NL 0x28 > #define DSI_VACT_NL 0x2C > +#define DSI_SIZE_CON 0x38 > #define DSI_HSA_WC 0x50 > #define DSI_HBP_WC 0x54 > #define DSI_HFP_WC 0x58 > @@ -131,7 +132,10 @@ > #define VM_CMD_EN BIT(0) > #define TS_VFP_EN BIT(5) > > -#define DSI_CMDQ0 0x180 > +#define DSI_SHADOW_DEBUG 0x190U > +#define FORCE_COMMIT BIT(0) > +#define BYPASS_SHADOW BIT(1) > + > #define CONFIG (0xff << 0) > #define SHORT_PACKET 0 > #define LONG_PACKET 2 > @@ -158,6 +162,7 @@ struct phy; > > struct mtk_dsi_driver_data { > const u32 reg_cmdq_off; > + bool has_size_ctl; > }; > > struct mtk_dsi { > @@ -426,6 +431,9 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL); > writel(vm->vactive, dsi->regs + DSI_VACT_NL); > > + if (dsi->driver_data->has_size_ctl) > + writel(vm->vactive << 16 | vm->hactive, dsi->regs + DSI_SIZE_CON); > + > horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10); > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) > @@ -595,6 +603,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) > } > > mtk_dsi_enable(dsi); > + > + /* DSI no need this double buffer, disable it when writing register */ > + writel(FORCE_COMMIT | BYPASS_SHADOW, dsi->regs + DSI_SHADOW_DEBUG); Is this a mt8183 thing? Did you assure that this does not introduce regressions on other SoCs, or does it fix any? I think this should be a independent patch. If it fixes an actual issue, then please provide a fixes tag in that patch. Thanks, Matthias > mtk_dsi_reset_engine(dsi); > mtk_dsi_phy_timconfig(dsi); > > @@ -1090,11 +1101,18 @@ static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = { > .reg_cmdq_off = 0x180, > }; > > +static const struct mtk_dsi_driver_data mt8183_dsi_driver_data = { > + .reg_cmdq_off = 0x200, > + .has_size_ctl = true, > +}; > + > static const struct of_device_id mtk_dsi_of_match[] = { > { .compatible = "mediatek,mt2701-dsi", > .data = &mt2701_dsi_driver_data }, > { .compatible = "mediatek,mt8173-dsi", > .data = &mt8173_dsi_driver_data }, > + { .compatible = "mediatek,mt8183-dsi", > + .data = &mt8183_dsi_driver_data }, > { }, > }; > >
On Thu, 2019-02-14 at 10:54 +0100, Matthias Brugger wrote: > > On 14/02/2019 05:42, Jitao Shi wrote: > > MT8183 dsi has two changes with mt8173. > > 1. Add the register double buffer control, but we no need it, So make > > it default off. > > 2. Add picture size control. > > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dsi.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > > index 80db02a25cb0..20cb53f05d42 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > > @@ -78,6 +78,7 @@ > > #define DSI_VBP_NL 0x24 > > #define DSI_VFP_NL 0x28 > > #define DSI_VACT_NL 0x2C > > +#define DSI_SIZE_CON 0x38 > > #define DSI_HSA_WC 0x50 > > #define DSI_HBP_WC 0x54 > > #define DSI_HFP_WC 0x58 > > @@ -131,7 +132,10 @@ > > #define VM_CMD_EN BIT(0) > > #define TS_VFP_EN BIT(5) > > > > -#define DSI_CMDQ0 0x180 > > +#define DSI_SHADOW_DEBUG 0x190U > > +#define FORCE_COMMIT BIT(0) > > +#define BYPASS_SHADOW BIT(1) > > + > > #define CONFIG (0xff << 0) > > #define SHORT_PACKET 0 > > #define LONG_PACKET 2 > > @@ -158,6 +162,7 @@ struct phy; > > > > struct mtk_dsi_driver_data { > > const u32 reg_cmdq_off; > > + bool has_size_ctl; > > }; > > > > struct mtk_dsi { > > @@ -426,6 +431,9 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > > writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL); > > writel(vm->vactive, dsi->regs + DSI_VACT_NL); > > > > + if (dsi->driver_data->has_size_ctl) > > + writel(vm->vactive << 16 | vm->hactive, dsi->regs + DSI_SIZE_CON); > > + > > horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10); > > > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) > > @@ -595,6 +603,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) > > } > > > > mtk_dsi_enable(dsi); > > + > > + /* DSI no need this double buffer, disable it when writing register */ > > + writel(FORCE_COMMIT | BYPASS_SHADOW, dsi->regs + DSI_SHADOW_DEBUG); > > Is this a mt8183 thing? Did you assure that this does not introduce regressions > on other SoCs, or does it fix any? > > I think this should be a independent patch. If it fixes an actual issue, then > please provide a fixes tag in that patch. > > Thanks, > Matthias > Yes, this is for mt8183. But this reg is reverse on other mtk soc. It is unsuitable. And i'll put it in mt8183 driver data next version. Best Regards Jitao > > mtk_dsi_reset_engine(dsi); > > mtk_dsi_phy_timconfig(dsi); > > > > @@ -1090,11 +1101,18 @@ static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = { > > .reg_cmdq_off = 0x180, > > }; > > > > +static const struct mtk_dsi_driver_data mt8183_dsi_driver_data = { > > + .reg_cmdq_off = 0x200, > > + .has_size_ctl = true, > > +}; > > + > > static const struct of_device_id mtk_dsi_of_match[] = { > > { .compatible = "mediatek,mt2701-dsi", > > .data = &mt2701_dsi_driver_data }, > > { .compatible = "mediatek,mt8173-dsi", > > .data = &mt8173_dsi_driver_data }, > > + { .compatible = "mediatek,mt8183-dsi", > > + .data = &mt8183_dsi_driver_data }, > > { }, > > }; > > > >
On Thu, 2019-02-14 at 13:54 +0800, Nicolas Boichat wrote: > On Thu, Feb 14, 2019 at 12:43 PM Jitao Shi <jitao.shi@mediatek.com> wrote: > > > > MT8183 dsi has two changes with mt8173. > > 1. Add the register double buffer control, but we no need it, So make > > it default off. > > Can you describe a little bit more what this is about? That's shadow > registers, right? > Yes, it is shadow registers. Jitao > > 2. Add picture size control. > > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/mtk_dsi.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > > index 80db02a25cb0..20cb53f05d42 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > > @@ -78,6 +78,7 @@ > > #define DSI_VBP_NL 0x24 > > #define DSI_VFP_NL 0x28 > > #define DSI_VACT_NL 0x2C > > +#define DSI_SIZE_CON 0x38 > > #define DSI_HSA_WC 0x50 > > #define DSI_HBP_WC 0x54 > > #define DSI_HFP_WC 0x58 > > @@ -131,7 +132,10 @@ > > #define VM_CMD_EN BIT(0) > > #define TS_VFP_EN BIT(5) > > > > -#define DSI_CMDQ0 0x180 > > As I said earlier, move this to 2/3. > Thank for you review. I'll move it to 2/3 next version. Best Regards Jitao > > +#define DSI_SHADOW_DEBUG 0x190U > > +#define FORCE_COMMIT BIT(0) > > +#define BYPASS_SHADOW BIT(1) > > + > > #define CONFIG (0xff << 0) > > #define SHORT_PACKET 0 > > #define LONG_PACKET 2 > > @@ -158,6 +162,7 @@ struct phy; > > > > struct mtk_dsi_driver_data { > > const u32 reg_cmdq_off; > > + bool has_size_ctl; > > }; > > > > struct mtk_dsi { > > @@ -426,6 +431,9 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > > writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL); > > writel(vm->vactive, dsi->regs + DSI_VACT_NL); > > > > + if (dsi->driver_data->has_size_ctl) > > + writel(vm->vactive << 16 | vm->hactive, dsi->regs + DSI_SIZE_CON); > > + > > horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10); > > > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) > > @@ -595,6 +603,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) > > } > > > > mtk_dsi_enable(dsi); > > + > > + /* DSI no need this double buffer, disable it when writing register */ > > "DSI does not need double buffering, disable it when writing register" > I'll fix it next version. > > + writel(FORCE_COMMIT | BYPASS_SHADOW, dsi->regs + DSI_SHADOW_DEBUG); > > So you do this on all MT* variants, is that ok? > > > mtk_dsi_reset_engine(dsi); > > mtk_dsi_phy_timconfig(dsi); > > > > @@ -1090,11 +1101,18 @@ static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = { > > .reg_cmdq_off = 0x180, > > }; > > > > +static const struct mtk_dsi_driver_data mt8183_dsi_driver_data = { > > + .reg_cmdq_off = 0x200, > > + .has_size_ctl = true, > > +}; > > + > > static const struct of_device_id mtk_dsi_of_match[] = { > > { .compatible = "mediatek,mt2701-dsi", > > .data = &mt2701_dsi_driver_data }, > > { .compatible = "mediatek,mt8173-dsi", > > .data = &mt8173_dsi_driver_data }, > > + { .compatible = "mediatek,mt8183-dsi", > > + .data = &mt8183_dsi_driver_data }, > > { }, > > }; > > > > -- > > 2.20.1 > >
On Sun, Feb 17, 2019 at 10:48 PM Jitao Shi <jitao.shi@mediatek.com> wrote: > > On Thu, 2019-02-14 at 13:54 +0800, Nicolas Boichat wrote: > > On Thu, Feb 14, 2019 at 12:43 PM Jitao Shi <jitao.shi@mediatek.com> wrote: > > > > > > MT8183 dsi has two changes with mt8173. > > > 1. Add the register double buffer control, but we no need it, So make > > > it default off. > > > > Can you describe a little bit more what this is about? That's shadow > > registers, right? > > > > Yes, it is shadow registers. > > Jitao > > > > 2. Add picture size control. > > > > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > > > --- > > > drivers/gpu/drm/mediatek/mtk_dsi.c | 20 +++++++++++++++++++- > > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > > > index 80db02a25cb0..20cb53f05d42 100644 > > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > > > @@ -78,6 +78,7 @@ > > > #define DSI_VBP_NL 0x24 > > > #define DSI_VFP_NL 0x28 > > > #define DSI_VACT_NL 0x2C > > > +#define DSI_SIZE_CON 0x38 > > > #define DSI_HSA_WC 0x50 > > > #define DSI_HBP_WC 0x54 > > > #define DSI_HFP_WC 0x58 > > > @@ -131,7 +132,10 @@ > > > #define VM_CMD_EN BIT(0) > > > #define TS_VFP_EN BIT(5) > > > > > > -#define DSI_CMDQ0 0x180 > > > > As I said earlier, move this to 2/3. > > > > Thank for you review. > I'll move it to 2/3 next version. > > Best Regards > Jitao > > > > +#define DSI_SHADOW_DEBUG 0x190U > > > +#define FORCE_COMMIT BIT(0) > > > +#define BYPASS_SHADOW BIT(1) > > > + > > > #define CONFIG (0xff << 0) > > > #define SHORT_PACKET 0 > > > #define LONG_PACKET 2 > > > @@ -158,6 +162,7 @@ struct phy; > > > > > > struct mtk_dsi_driver_data { > > > const u32 reg_cmdq_off; > > > + bool has_size_ctl; > > > }; > > > > > > struct mtk_dsi { > > > @@ -426,6 +431,9 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > > > writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL); > > > writel(vm->vactive, dsi->regs + DSI_VACT_NL); > > > > > > + if (dsi->driver_data->has_size_ctl) > > > + writel(vm->vactive << 16 | vm->hactive, dsi->regs + DSI_SIZE_CON); > > > + > > > horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10); > > > > > > if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) > > > @@ -595,6 +603,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) > > > } > > > > > > mtk_dsi_enable(dsi); > > > + > > > + /* DSI no need this double buffer, disable it when writing register */ > > > > "DSI does not need double buffering, disable it when writing register" > > > > I'll fix it next version. In that case, please say something about "shadow registers". (maybe it's just me, but usually double-buffering is associated with framebuffer data, not register settings) > > > + writel(FORCE_COMMIT | BYPASS_SHADOW, dsi->regs + DSI_SHADOW_DEBUG); > > > > So you do this on all MT* variants, is that ok? > > > > > mtk_dsi_reset_engine(dsi); > > > mtk_dsi_phy_timconfig(dsi); > > > > > > @@ -1090,11 +1101,18 @@ static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = { > > > .reg_cmdq_off = 0x180, > > > }; > > > > > > +static const struct mtk_dsi_driver_data mt8183_dsi_driver_data = { > > > + .reg_cmdq_off = 0x200, > > > + .has_size_ctl = true, > > > +}; > > > + > > > static const struct of_device_id mtk_dsi_of_match[] = { > > > { .compatible = "mediatek,mt2701-dsi", > > > .data = &mt2701_dsi_driver_data }, > > > { .compatible = "mediatek,mt8173-dsi", > > > .data = &mt8173_dsi_driver_data }, > > > + { .compatible = "mediatek,mt8183-dsi", > > > + .data = &mt8183_dsi_driver_data }, > > > { }, > > > }; > > > > > > -- > > > 2.20.1 > > > > >
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 80db02a25cb0..20cb53f05d42 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -78,6 +78,7 @@ #define DSI_VBP_NL 0x24 #define DSI_VFP_NL 0x28 #define DSI_VACT_NL 0x2C +#define DSI_SIZE_CON 0x38 #define DSI_HSA_WC 0x50 #define DSI_HBP_WC 0x54 #define DSI_HFP_WC 0x58 @@ -131,7 +132,10 @@ #define VM_CMD_EN BIT(0) #define TS_VFP_EN BIT(5) -#define DSI_CMDQ0 0x180 +#define DSI_SHADOW_DEBUG 0x190U +#define FORCE_COMMIT BIT(0) +#define BYPASS_SHADOW BIT(1) + #define CONFIG (0xff << 0) #define SHORT_PACKET 0 #define LONG_PACKET 2 @@ -158,6 +162,7 @@ struct phy; struct mtk_dsi_driver_data { const u32 reg_cmdq_off; + bool has_size_ctl; }; struct mtk_dsi { @@ -426,6 +431,9 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL); writel(vm->vactive, dsi->regs + DSI_VACT_NL); + if (dsi->driver_data->has_size_ctl) + writel(vm->vactive << 16 | vm->hactive, dsi->regs + DSI_SIZE_CON); + horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10); if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) @@ -595,6 +603,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) } mtk_dsi_enable(dsi); + + /* DSI no need this double buffer, disable it when writing register */ + writel(FORCE_COMMIT | BYPASS_SHADOW, dsi->regs + DSI_SHADOW_DEBUG); mtk_dsi_reset_engine(dsi); mtk_dsi_phy_timconfig(dsi); @@ -1090,11 +1101,18 @@ static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = { .reg_cmdq_off = 0x180, }; +static const struct mtk_dsi_driver_data mt8183_dsi_driver_data = { + .reg_cmdq_off = 0x200, + .has_size_ctl = true, +}; + static const struct of_device_id mtk_dsi_of_match[] = { { .compatible = "mediatek,mt2701-dsi", .data = &mt2701_dsi_driver_data }, { .compatible = "mediatek,mt8173-dsi", .data = &mt8173_dsi_driver_data }, + { .compatible = "mediatek,mt8183-dsi", + .data = &mt8183_dsi_driver_data }, { }, };
MT8183 dsi has two changes with mt8173. 1. Add the register double buffer control, but we no need it, So make it default off. 2. Add picture size control. Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> --- drivers/gpu/drm/mediatek/mtk_dsi.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)