DSI panel driver need attach function which is inculde in
mipi_dsi_host_ops.
If mipi_dsi_host_register is not in probe, dsi panel will
probe fail or more delay.
So move the mipi_dsi_host_register to probe from bind.
Signed-off-by: Jitao Shi <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 49 ++++++++++++++++++------------
1 file changed, 30 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 27b507eb4a99..93fa255b4aad 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -1045,12 +1045,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
return ret;
}
- ret = mipi_dsi_host_register(&dsi->host);
- if (ret < 0) {
- dev_err(dev, "failed to register DSI host: %d\n", ret);
- goto err_ddp_comp_unregister;
- }
-
ret = mtk_dsi_create_conn_enc(drm, dsi);
if (ret) {
DRM_ERROR("Encoder create failed with %d\n", ret);
@@ -1060,8 +1054,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
return 0;
err_unregister:
- mipi_dsi_host_unregister(&dsi->host);
-err_ddp_comp_unregister:
mtk_ddp_comp_unregister(drm, &dsi->ddp_comp);
return ret;
}
@@ -1097,31 +1089,37 @@ static int mtk_dsi_probe(struct platform_device *pdev)
dsi->host.ops = &mtk_dsi_ops;
dsi->host.dev = dev;
+ dsi->dev = dev;
+ ret = mipi_dsi_host_register(&dsi->host);
+ if (ret < 0) {
+ dev_err(dev, "failed to register DSI host: %d\n", ret);
+ return -EPROBE_DEFER;
+ }
ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
&dsi->panel, &dsi->bridge);
if (ret)
- return ret;
+ goto err_unregister_host;
dsi->engine_clk = devm_clk_get(dev, "engine");
if (IS_ERR(dsi->engine_clk)) {
ret = PTR_ERR(dsi->engine_clk);
dev_err(dev, "Failed to get engine clock: %d\n", ret);
- return ret;
+ goto err_unregister_host;
}
dsi->digital_clk = devm_clk_get(dev, "digital");
if (IS_ERR(dsi->digital_clk)) {
ret = PTR_ERR(dsi->digital_clk);
dev_err(dev, "Failed to get digital clock: %d\n", ret);
- return ret;
+ goto err_unregister_host;
}
dsi->hs_clk = devm_clk_get(dev, "hs");
if (IS_ERR(dsi->hs_clk)) {
ret = PTR_ERR(dsi->hs_clk);
dev_err(dev, "Failed to get hs clock: %d\n", ret);
- return ret;
+ goto err_unregister_host;
}
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1129,33 +1127,35 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (IS_ERR(dsi->regs)) {
ret = PTR_ERR(dsi->regs);
dev_err(dev, "Failed to ioremap memory: %d\n", ret);
- return ret;
+ goto err_unregister_host;
}
dsi->phy = devm_phy_get(dev, "dphy");
if (IS_ERR(dsi->phy)) {
ret = PTR_ERR(dsi->phy);
dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
- return ret;
+ goto err_unregister_host;
}
comp_id = mtk_ddp_comp_get_id(dev->of_node, MTK_DSI);
if (comp_id < 0) {
dev_err(dev, "Failed to identify by alias: %d\n", comp_id);
- return comp_id;
+ ret = comp_id;
+ goto err_unregister_host;
}
ret = mtk_ddp_comp_init(dev, dev->of_node, &dsi->ddp_comp, comp_id,
&mtk_dsi_funcs);
if (ret) {
dev_err(dev, "Failed to initialize component: %d\n", ret);
- return ret;
+ goto err_unregister_host;
}
irq_num = platform_get_irq(pdev, 0);
if (irq_num < 0) {
dev_err(&pdev->dev, "failed to request dsi irq resource\n");
- return -EPROBE_DEFER;
+ ret = -EPROBE_DEFER;
+ goto err_unregister_host;
}
irq_set_status_flags(irq_num, IRQ_TYPE_LEVEL_LOW);
@@ -1163,14 +1163,25 @@ static int mtk_dsi_probe(struct platform_device *pdev)
IRQF_TRIGGER_LOW, dev_name(&pdev->dev), dsi);
if (ret) {
dev_err(&pdev->dev, "failed to request mediatek dsi irq\n");
- return -EPROBE_DEFER;
+ ret = -EPROBE_DEFER;
+ goto err_unregister_host;
}
init_waitqueue_head(&dsi->irq_wait_queue);
platform_set_drvdata(pdev, dsi);
- return component_add(&pdev->dev, &mtk_dsi_component_ops);
+ ret = component_add(&pdev->dev, &mtk_dsi_component_ops);
+ if (ret) {
+ ret = -EPROBE_DEFER;
+ goto err_unregister_host;
+ }
+
+ return 0;
+
+err_unregister_host:
+ mipi_dsi_host_unregister(&dsi->host);
+ return ret;
}
static int mtk_dsi_remove(struct platform_device *pdev)
--
2.20.1
Config the different CMDQ reg address in driver data.
Signed-off-by: Jitao Shi <[email protected]>
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 39 ++++++++++++++++++++++++------
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 93fa255b4aad..80db02a25cb0 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -156,6 +156,10 @@
struct phy;
+struct mtk_dsi_driver_data {
+ const u32 reg_cmdq_off;
+};
+
struct mtk_dsi {
struct mtk_ddp_comp ddp_comp;
struct device *dev;
@@ -182,6 +186,7 @@ struct mtk_dsi {
bool enabled;
u32 irq_data;
wait_queue_head_t irq_wait_queue;
+ struct mtk_dsi_driver_data *driver_data;
};
static inline struct mtk_dsi *encoder_to_dsi(struct drm_encoder *e)
@@ -934,6 +939,7 @@ static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
const char *tx_buf = msg->tx_buf;
u8 config, cmdq_size, cmdq_off, type = msg->type;
u32 reg_val, cmdq_mask, i;
+ u32 reg_cmdq_off = dsi->driver_data->reg_cmdq_off;
if (MTK_DSI_HOST_IS_READ(type))
config = BTA;
@@ -953,9 +959,11 @@ static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
}
for (i = 0; i < msg->tx_len; i++)
- writeb(tx_buf[i], dsi->regs + DSI_CMDQ0 + cmdq_off + i);
+ mtk_dsi_mask(dsi, (reg_cmdq_off + cmdq_off + i) & (~0x3U),
+ (0xffUL << (((i + cmdq_off) & 3U) * 8U)),
+ tx_buf[i] << (((i + cmdq_off) & 3U) * 8U));
- mtk_dsi_mask(dsi, DSI_CMDQ0, cmdq_mask, reg_val);
+ mtk_dsi_mask(dsi, reg_cmdq_off, cmdq_mask, reg_val);
mtk_dsi_mask(dsi, DSI_CMDQ_SIZE, CMDQ_SIZE, cmdq_size);
}
@@ -1074,10 +1082,27 @@ static const struct component_ops mtk_dsi_component_ops = {
.unbind = mtk_dsi_unbind,
};
+static const struct mtk_dsi_driver_data mt8173_dsi_driver_data = {
+ .reg_cmdq_off = 0x200,
+};
+
+static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = {
+ .reg_cmdq_off = 0x180,
+};
+
+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 },
+ { },
+};
+
static int mtk_dsi_probe(struct platform_device *pdev)
{
struct mtk_dsi *dsi;
struct device *dev = &pdev->dev;
+ const struct of_device_id *of_id;
struct resource *regs;
int irq_num;
int comp_id;
@@ -1101,6 +1126,10 @@ static int mtk_dsi_probe(struct platform_device *pdev)
if (ret)
goto err_unregister_host;
+ of_id = of_match_device(mtk_dsi_of_match, &pdev->dev);
+ dsi->driver_data = (struct mtk_dsi_driver_data *)
+ of_id->data;
+
dsi->engine_clk = devm_clk_get(dev, "engine");
if (IS_ERR(dsi->engine_clk)) {
ret = PTR_ERR(dsi->engine_clk);
@@ -1194,12 +1223,6 @@ static int mtk_dsi_remove(struct platform_device *pdev)
return 0;
}
-static const struct of_device_id mtk_dsi_of_match[] = {
- { .compatible = "mediatek,mt2701-dsi" },
- { .compatible = "mediatek,mt8173-dsi" },
- { },
-};
-
struct platform_driver mtk_dsi_driver = {
.probe = mtk_dsi_probe,
.remove = mtk_dsi_remove,
--
2.20.1
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 <[email protected]>
---
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);
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 Thu, Feb 14, 2019 at 12:42 PM Jitao Shi <[email protected]> wrote:
>
> Config the different CMDQ reg address in driver data.
>
> Signed-off-by: Jitao Shi <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 39 ++++++++++++++++++++++++------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 93fa255b4aad..80db02a25cb0 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -156,6 +156,10 @@
>
> struct phy;
>
> +struct mtk_dsi_driver_data {
> + const u32 reg_cmdq_off;
> +};
> +
> struct mtk_dsi {
> struct mtk_ddp_comp ddp_comp;
> struct device *dev;
> @@ -182,6 +186,7 @@ struct mtk_dsi {
> bool enabled;
> u32 irq_data;
> wait_queue_head_t irq_wait_queue;
> + struct mtk_dsi_driver_data *driver_data;
> };
>
> static inline struct mtk_dsi *encoder_to_dsi(struct drm_encoder *e)
> @@ -934,6 +939,7 @@ static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
> const char *tx_buf = msg->tx_buf;
> u8 config, cmdq_size, cmdq_off, type = msg->type;
> u32 reg_val, cmdq_mask, i;
> + u32 reg_cmdq_off = dsi->driver_data->reg_cmdq_off;
>
> if (MTK_DSI_HOST_IS_READ(type))
> config = BTA;
> @@ -953,9 +959,11 @@ static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
> }
>
> for (i = 0; i < msg->tx_len; i++)
> - writeb(tx_buf[i], dsi->regs + DSI_CMDQ0 + cmdq_off + i);
> + mtk_dsi_mask(dsi, (reg_cmdq_off + cmdq_off + i) & (~0x3U),
> + (0xffUL << (((i + cmdq_off) & 3U) * 8U)),
> + tx_buf[i] << (((i + cmdq_off) & 3U) * 8U));
I found the writeb call _much_ clearer ... Either switch back to that,
or create a new mtk_disk_mask_byte function maybe?
>
> - mtk_dsi_mask(dsi, DSI_CMDQ0, cmdq_mask, reg_val);
> + mtk_dsi_mask(dsi, reg_cmdq_off, cmdq_mask, reg_val);
You're removing DSI_CMDQ0 usage in this patch, so remove the #define
in this patch too (instead of doing that in 3/3).
> mtk_dsi_mask(dsi, DSI_CMDQ_SIZE, CMDQ_SIZE, cmdq_size);
> }
>
> @@ -1074,10 +1082,27 @@ static const struct component_ops mtk_dsi_component_ops = {
> .unbind = mtk_dsi_unbind,
> };
>
> +static const struct mtk_dsi_driver_data mt8173_dsi_driver_data = {
> + .reg_cmdq_off = 0x200,
> +};
> +
> +static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = {
> + .reg_cmdq_off = 0x180,
> +};
> +
> +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 },
> + { },
> +};
> +
> static int mtk_dsi_probe(struct platform_device *pdev)
> {
> struct mtk_dsi *dsi;
> struct device *dev = &pdev->dev;
> + const struct of_device_id *of_id;
> struct resource *regs;
> int irq_num;
> int comp_id;
> @@ -1101,6 +1126,10 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> if (ret)
> goto err_unregister_host;
>
> + of_id = of_match_device(mtk_dsi_of_match, &pdev->dev);
> + dsi->driver_data = (struct mtk_dsi_driver_data *)
> + of_id->data;
This fits in 80 chars. Also, of_id->data is a void*, so no cast needed.
> +
> dsi->engine_clk = devm_clk_get(dev, "engine");
> if (IS_ERR(dsi->engine_clk)) {
> ret = PTR_ERR(dsi->engine_clk);
> @@ -1194,12 +1223,6 @@ static int mtk_dsi_remove(struct platform_device *pdev)
> return 0;
> }
>
> -static const struct of_device_id mtk_dsi_of_match[] = {
> - { .compatible = "mediatek,mt2701-dsi" },
> - { .compatible = "mediatek,mt8173-dsi" },
> - { },
> -};
Any reason you moved this up?
> -
> struct platform_driver mtk_dsi_driver = {
> .probe = mtk_dsi_probe,
> .remove = mtk_dsi_remove,
> --
> 2.20.1
>
On Thu, Feb 14, 2019 at 12:43 PM Jitao Shi <[email protected]> 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 <[email protected]>
> ---
> 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
>
Just some comments on the error path, I'm not sure about the change itself.
On Thu, Feb 14, 2019 at 12:42 PM Jitao Shi <[email protected]> wrote:
>
> DSI panel driver need attach function which is inculde in
> mipi_dsi_host_ops.
>
> If mipi_dsi_host_register is not in probe, dsi panel will
> probe fail or more delay.
>
> So move the mipi_dsi_host_register to probe from bind.
>
> Signed-off-by: Jitao Shi <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 49 ++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 27b507eb4a99..93fa255b4aad 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -1045,12 +1045,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> return ret;
> }
>
> - ret = mipi_dsi_host_register(&dsi->host);
> - if (ret < 0) {
> - dev_err(dev, "failed to register DSI host: %d\n", ret);
> - goto err_ddp_comp_unregister;
> - }
> -
> ret = mtk_dsi_create_conn_enc(drm, dsi);
> if (ret) {
> DRM_ERROR("Encoder create failed with %d\n", ret);
> @@ -1060,8 +1054,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> return 0;
>
> err_unregister:
> - mipi_dsi_host_unregister(&dsi->host);
> -err_ddp_comp_unregister:
> mtk_ddp_comp_unregister(drm, &dsi->ddp_comp);
> return ret;
> }
> @@ -1097,31 +1089,37 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>
> dsi->host.ops = &mtk_dsi_ops;
> dsi->host.dev = dev;
> + dsi->dev = dev;
> + ret = mipi_dsi_host_register(&dsi->host);
> + if (ret < 0) {
> + dev_err(dev, "failed to register DSI host: %d\n", ret);
> + return -EPROBE_DEFER;
return ret
> + }
>
> ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> &dsi->panel, &dsi->bridge);
> if (ret)
> - return ret;
> + goto err_unregister_host;
>
> dsi->engine_clk = devm_clk_get(dev, "engine");
> if (IS_ERR(dsi->engine_clk)) {
> ret = PTR_ERR(dsi->engine_clk);
> dev_err(dev, "Failed to get engine clock: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> dsi->digital_clk = devm_clk_get(dev, "digital");
> if (IS_ERR(dsi->digital_clk)) {
> ret = PTR_ERR(dsi->digital_clk);
> dev_err(dev, "Failed to get digital clock: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> dsi->hs_clk = devm_clk_get(dev, "hs");
> if (IS_ERR(dsi->hs_clk)) {
> ret = PTR_ERR(dsi->hs_clk);
> dev_err(dev, "Failed to get hs clock: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1129,33 +1127,35 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> if (IS_ERR(dsi->regs)) {
> ret = PTR_ERR(dsi->regs);
> dev_err(dev, "Failed to ioremap memory: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> dsi->phy = devm_phy_get(dev, "dphy");
> if (IS_ERR(dsi->phy)) {
> ret = PTR_ERR(dsi->phy);
> dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> comp_id = mtk_ddp_comp_get_id(dev->of_node, MTK_DSI);
> if (comp_id < 0) {
> dev_err(dev, "Failed to identify by alias: %d\n", comp_id);
> - return comp_id;
> + ret = comp_id;
> + goto err_unregister_host;
> }
>
> ret = mtk_ddp_comp_init(dev, dev->of_node, &dsi->ddp_comp, comp_id,
> &mtk_dsi_funcs);
> if (ret) {
> dev_err(dev, "Failed to initialize component: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> irq_num = platform_get_irq(pdev, 0);
> if (irq_num < 0) {
> dev_err(&pdev->dev, "failed to request dsi irq resource\n");
> - return -EPROBE_DEFER;
> + ret = -EPROBE_DEFER;
I think you need to do ret = irq_num here, too (and yes, it was
probably wrong before?)
> + goto err_unregister_host;
> }
>
> irq_set_status_flags(irq_num, IRQ_TYPE_LEVEL_LOW);
> @@ -1163,14 +1163,25 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> IRQF_TRIGGER_LOW, dev_name(&pdev->dev), dsi);
> if (ret) {
> dev_err(&pdev->dev, "failed to request mediatek dsi irq\n");
> - return -EPROBE_DEFER;
> + ret = -EPROBE_DEFER;
Ditto, I'd not reassign ret.
> + goto err_unregister_host;
> }
>
> init_waitqueue_head(&dsi->irq_wait_queue);
>
> platform_set_drvdata(pdev, dsi);
>
> - return component_add(&pdev->dev, &mtk_dsi_component_ops);
> + ret = component_add(&pdev->dev, &mtk_dsi_component_ops);
> + if (ret) {
> + ret = -EPROBE_DEFER;
Ditto (this one _was_ right before).
> + goto err_unregister_host;
> + }
> +
> + return 0;
> +
> +err_unregister_host:
> + mipi_dsi_host_unregister(&dsi->host);
> + return ret;
> }
>
> static int mtk_dsi_remove(struct platform_device *pdev)
> --
> 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 <[email protected]>
> ---
> 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, Feb 14, 2019 at 12:42:41PM +0800, Jitao Shi wrote:
> DSI panel driver need attach function which is inculde in
> mipi_dsi_host_ops.
Which function is required from dsi_host?
Sean
>
> If mipi_dsi_host_register is not in probe, dsi panel will
> probe fail or more delay.
>
> So move the mipi_dsi_host_register to probe from bind.
>
> Signed-off-by: Jitao Shi <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 49 ++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 27b507eb4a99..93fa255b4aad 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -1045,12 +1045,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> return ret;
> }
>
> - ret = mipi_dsi_host_register(&dsi->host);
> - if (ret < 0) {
> - dev_err(dev, "failed to register DSI host: %d\n", ret);
> - goto err_ddp_comp_unregister;
> - }
> -
> ret = mtk_dsi_create_conn_enc(drm, dsi);
> if (ret) {
> DRM_ERROR("Encoder create failed with %d\n", ret);
> @@ -1060,8 +1054,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> return 0;
>
> err_unregister:
> - mipi_dsi_host_unregister(&dsi->host);
> -err_ddp_comp_unregister:
> mtk_ddp_comp_unregister(drm, &dsi->ddp_comp);
> return ret;
> }
> @@ -1097,31 +1089,37 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>
> dsi->host.ops = &mtk_dsi_ops;
> dsi->host.dev = dev;
> + dsi->dev = dev;
> + ret = mipi_dsi_host_register(&dsi->host);
> + if (ret < 0) {
> + dev_err(dev, "failed to register DSI host: %d\n", ret);
> + return -EPROBE_DEFER;
> + }
>
> ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> &dsi->panel, &dsi->bridge);
> if (ret)
> - return ret;
> + goto err_unregister_host;
>
> dsi->engine_clk = devm_clk_get(dev, "engine");
> if (IS_ERR(dsi->engine_clk)) {
> ret = PTR_ERR(dsi->engine_clk);
> dev_err(dev, "Failed to get engine clock: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> dsi->digital_clk = devm_clk_get(dev, "digital");
> if (IS_ERR(dsi->digital_clk)) {
> ret = PTR_ERR(dsi->digital_clk);
> dev_err(dev, "Failed to get digital clock: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> dsi->hs_clk = devm_clk_get(dev, "hs");
> if (IS_ERR(dsi->hs_clk)) {
> ret = PTR_ERR(dsi->hs_clk);
> dev_err(dev, "Failed to get hs clock: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1129,33 +1127,35 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> if (IS_ERR(dsi->regs)) {
> ret = PTR_ERR(dsi->regs);
> dev_err(dev, "Failed to ioremap memory: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> dsi->phy = devm_phy_get(dev, "dphy");
> if (IS_ERR(dsi->phy)) {
> ret = PTR_ERR(dsi->phy);
> dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> comp_id = mtk_ddp_comp_get_id(dev->of_node, MTK_DSI);
> if (comp_id < 0) {
> dev_err(dev, "Failed to identify by alias: %d\n", comp_id);
> - return comp_id;
> + ret = comp_id;
> + goto err_unregister_host;
> }
>
> ret = mtk_ddp_comp_init(dev, dev->of_node, &dsi->ddp_comp, comp_id,
> &mtk_dsi_funcs);
> if (ret) {
> dev_err(dev, "Failed to initialize component: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> irq_num = platform_get_irq(pdev, 0);
> if (irq_num < 0) {
> dev_err(&pdev->dev, "failed to request dsi irq resource\n");
> - return -EPROBE_DEFER;
> + ret = -EPROBE_DEFER;
> + goto err_unregister_host;
> }
>
> irq_set_status_flags(irq_num, IRQ_TYPE_LEVEL_LOW);
> @@ -1163,14 +1163,25 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> IRQF_TRIGGER_LOW, dev_name(&pdev->dev), dsi);
> if (ret) {
> dev_err(&pdev->dev, "failed to request mediatek dsi irq\n");
> - return -EPROBE_DEFER;
> + ret = -EPROBE_DEFER;
> + goto err_unregister_host;
> }
>
> init_waitqueue_head(&dsi->irq_wait_queue);
>
> platform_set_drvdata(pdev, dsi);
>
> - return component_add(&pdev->dev, &mtk_dsi_component_ops);
> + ret = component_add(&pdev->dev, &mtk_dsi_component_ops);
> + if (ret) {
> + ret = -EPROBE_DEFER;
> + goto err_unregister_host;
> + }
> +
> + return 0;
> +
> +err_unregister_host:
> + mipi_dsi_host_unregister(&dsi->host);
> + return ret;
> }
>
> static int mtk_dsi_remove(struct platform_device *pdev)
> --
> 2.20.1
>
--
Sean Paul, Software Engineer, Google / Chromium OS
On Thu, 2019-02-14 at 15:48 -0500, Sean Paul wrote:
> On Thu, Feb 14, 2019 at 12:42:41PM +0800, Jitao Shi wrote:
> > DSI panel driver need attach function which is inculde in
> > mipi_dsi_host_ops.
>
> Which function is required from dsi_host?
>
> Sean
>
mipi_dsi_attach request the mipi_dsi_host_register ready.
for example.
mipi_dsi_attach is called by panel_simple_dsi_probe.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/panel/panel-simple.c?h=v5.0-rc6#n2987
jitao
> >
> > If mipi_dsi_host_register is not in probe, dsi panel will
> > probe fail or more delay.
> >
> > So move the mipi_dsi_host_register to probe from bind.
> >
> > Signed-off-by: Jitao Shi <[email protected]>
> > ---
> > drivers/gpu/drm/mediatek/mtk_dsi.c | 49 ++++++++++++++++++------------
> > 1 file changed, 30 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 27b507eb4a99..93fa255b4aad 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -1045,12 +1045,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> > return ret;
> > }
> >
> > - ret = mipi_dsi_host_register(&dsi->host);
> > - if (ret < 0) {
> > - dev_err(dev, "failed to register DSI host: %d\n", ret);
> > - goto err_ddp_comp_unregister;
> > - }
> > -
> > ret = mtk_dsi_create_conn_enc(drm, dsi);
> > if (ret) {
> > DRM_ERROR("Encoder create failed with %d\n", ret);
> > @@ -1060,8 +1054,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> > return 0;
> >
> > err_unregister:
> > - mipi_dsi_host_unregister(&dsi->host);
> > -err_ddp_comp_unregister:
> > mtk_ddp_comp_unregister(drm, &dsi->ddp_comp);
> > return ret;
> > }
> > @@ -1097,31 +1089,37 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> >
> > dsi->host.ops = &mtk_dsi_ops;
> > dsi->host.dev = dev;
> > + dsi->dev = dev;
> > + ret = mipi_dsi_host_register(&dsi->host);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to register DSI host: %d\n", ret);
> > + return -EPROBE_DEFER;
> > + }
> >
> > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> > &dsi->panel, &dsi->bridge);
> > if (ret)
> > - return ret;
> > + goto err_unregister_host;
> >
> > dsi->engine_clk = devm_clk_get(dev, "engine");
> > if (IS_ERR(dsi->engine_clk)) {
> > ret = PTR_ERR(dsi->engine_clk);
> > dev_err(dev, "Failed to get engine clock: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > dsi->digital_clk = devm_clk_get(dev, "digital");
> > if (IS_ERR(dsi->digital_clk)) {
> > ret = PTR_ERR(dsi->digital_clk);
> > dev_err(dev, "Failed to get digital clock: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > dsi->hs_clk = devm_clk_get(dev, "hs");
> > if (IS_ERR(dsi->hs_clk)) {
> > ret = PTR_ERR(dsi->hs_clk);
> > dev_err(dev, "Failed to get hs clock: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > @@ -1129,33 +1127,35 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> > if (IS_ERR(dsi->regs)) {
> > ret = PTR_ERR(dsi->regs);
> > dev_err(dev, "Failed to ioremap memory: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > dsi->phy = devm_phy_get(dev, "dphy");
> > if (IS_ERR(dsi->phy)) {
> > ret = PTR_ERR(dsi->phy);
> > dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > comp_id = mtk_ddp_comp_get_id(dev->of_node, MTK_DSI);
> > if (comp_id < 0) {
> > dev_err(dev, "Failed to identify by alias: %d\n", comp_id);
> > - return comp_id;
> > + ret = comp_id;
> > + goto err_unregister_host;
> > }
> >
> > ret = mtk_ddp_comp_init(dev, dev->of_node, &dsi->ddp_comp, comp_id,
> > &mtk_dsi_funcs);
> > if (ret) {
> > dev_err(dev, "Failed to initialize component: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > irq_num = platform_get_irq(pdev, 0);
> > if (irq_num < 0) {
> > dev_err(&pdev->dev, "failed to request dsi irq resource\n");
> > - return -EPROBE_DEFER;
> > + ret = -EPROBE_DEFER;
> > + goto err_unregister_host;
> > }
> >
> > irq_set_status_flags(irq_num, IRQ_TYPE_LEVEL_LOW);
> > @@ -1163,14 +1163,25 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> > IRQF_TRIGGER_LOW, dev_name(&pdev->dev), dsi);
> > if (ret) {
> > dev_err(&pdev->dev, "failed to request mediatek dsi irq\n");
> > - return -EPROBE_DEFER;
> > + ret = -EPROBE_DEFER;
> > + goto err_unregister_host;
> > }
> >
> > init_waitqueue_head(&dsi->irq_wait_queue);
> >
> > platform_set_drvdata(pdev, dsi);
> >
> > - return component_add(&pdev->dev, &mtk_dsi_component_ops);
> > + ret = component_add(&pdev->dev, &mtk_dsi_component_ops);
> > + if (ret) {
> > + ret = -EPROBE_DEFER;
> > + goto err_unregister_host;
> > + }
> > +
> > + return 0;
> > +
> > +err_unregister_host:
> > + mipi_dsi_host_unregister(&dsi->host);
> > + return ret;
> > }
> >
> > static int mtk_dsi_remove(struct platform_device *pdev)
> > --
> > 2.20.1
> >
>
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 <[email protected]>
> > ---
> > 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 14:02 +0800, Nicolas Boichat wrote:
> Just some comments on the error path, I'm not sure about the change itself.
>
> On Thu, Feb 14, 2019 at 12:42 PM Jitao Shi <[email protected]> wrote:
> >
> > DSI panel driver need attach function which is inculde in
> > mipi_dsi_host_ops.
> >
> > If mipi_dsi_host_register is not in probe, dsi panel will
> > probe fail or more delay.
> >
> > So move the mipi_dsi_host_register to probe from bind.
> >
> > Signed-off-by: Jitao Shi <[email protected]>
> > ---
> > drivers/gpu/drm/mediatek/mtk_dsi.c | 49 ++++++++++++++++++------------
> > 1 file changed, 30 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 27b507eb4a99..93fa255b4aad 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -1045,12 +1045,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> > return ret;
> > }
> >
> > - ret = mipi_dsi_host_register(&dsi->host);
> > - if (ret < 0) {
> > - dev_err(dev, "failed to register DSI host: %d\n", ret);
> > - goto err_ddp_comp_unregister;
> > - }
> > -
> > ret = mtk_dsi_create_conn_enc(drm, dsi);
> > if (ret) {
> > DRM_ERROR("Encoder create failed with %d\n", ret);
> > @@ -1060,8 +1054,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> > return 0;
> >
> > err_unregister:
> > - mipi_dsi_host_unregister(&dsi->host);
> > -err_ddp_comp_unregister:
> > mtk_ddp_comp_unregister(drm, &dsi->ddp_comp);
> > return ret;
> > }
> > @@ -1097,31 +1089,37 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> >
> > dsi->host.ops = &mtk_dsi_ops;
> > dsi->host.dev = dev;
> > + dsi->dev = dev;
> > + ret = mipi_dsi_host_register(&dsi->host);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to register DSI host: %d\n", ret);
> > + return -EPROBE_DEFER;
>
> return ret
>
Ok, I'll fix it next version.
> > + }
> >
> > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> > &dsi->panel, &dsi->bridge);
> > if (ret)
> > - return ret;
> > + goto err_unregister_host;
> >
> > dsi->engine_clk = devm_clk_get(dev, "engine");
> > if (IS_ERR(dsi->engine_clk)) {
> > ret = PTR_ERR(dsi->engine_clk);
> > dev_err(dev, "Failed to get engine clock: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > dsi->digital_clk = devm_clk_get(dev, "digital");
> > if (IS_ERR(dsi->digital_clk)) {
> > ret = PTR_ERR(dsi->digital_clk);
> > dev_err(dev, "Failed to get digital clock: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > dsi->hs_clk = devm_clk_get(dev, "hs");
> > if (IS_ERR(dsi->hs_clk)) {
> > ret = PTR_ERR(dsi->hs_clk);
> > dev_err(dev, "Failed to get hs clock: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > @@ -1129,33 +1127,35 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> > if (IS_ERR(dsi->regs)) {
> > ret = PTR_ERR(dsi->regs);
> > dev_err(dev, "Failed to ioremap memory: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > dsi->phy = devm_phy_get(dev, "dphy");
> > if (IS_ERR(dsi->phy)) {
> > ret = PTR_ERR(dsi->phy);
> > dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > comp_id = mtk_ddp_comp_get_id(dev->of_node, MTK_DSI);
> > if (comp_id < 0) {
> > dev_err(dev, "Failed to identify by alias: %d\n", comp_id);
> > - return comp_id;
> > + ret = comp_id;
> > + goto err_unregister_host;
> > }
> >
> > ret = mtk_ddp_comp_init(dev, dev->of_node, &dsi->ddp_comp, comp_id,
> > &mtk_dsi_funcs);
> > if (ret) {
> > dev_err(dev, "Failed to initialize component: %d\n", ret);
> > - return ret;
> > + goto err_unregister_host;
> > }
> >
> > irq_num = platform_get_irq(pdev, 0);
> > if (irq_num < 0) {
> > dev_err(&pdev->dev, "failed to request dsi irq resource\n");
> > - return -EPROBE_DEFER;
> > + ret = -EPROBE_DEFER;
>
> I think you need to do ret = irq_num here, too (and yes, it was
> probably wrong before?)
>
Yes, the "ret = irq_num" is right.
We regard all the return error case as "-EPROBE_DEFER".
Actually the error case (ex. EXIO) should return error case.
I'll fix it next version.
> > + goto err_unregister_host;
> > }
> >
> > irq_set_status_flags(irq_num, IRQ_TYPE_LEVEL_LOW);
> > @@ -1163,14 +1163,25 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> > IRQF_TRIGGER_LOW, dev_name(&pdev->dev), dsi);
> > if (ret) {
> > dev_err(&pdev->dev, "failed to request mediatek dsi irq\n");
> > - return -EPROBE_DEFER;
> > + ret = -EPROBE_DEFER;
>
> Ditto, I'd not reassign ret.
I'll fix it next version.
>
> > + goto err_unregister_host;
> > }
> >
> > init_waitqueue_head(&dsi->irq_wait_queue);
> >
> > platform_set_drvdata(pdev, dsi);
> >
> > - return component_add(&pdev->dev, &mtk_dsi_component_ops);
> > + ret = component_add(&pdev->dev, &mtk_dsi_component_ops);
> > + if (ret) {
> > + ret = -EPROBE_DEFER;
>
> Ditto (this one _was_ right before).
I'll fix it next version.
>
> > + goto err_unregister_host;
> > + }
> > +
> > + return 0;
> > +
> > +err_unregister_host:
> > + mipi_dsi_host_unregister(&dsi->host);
> > + return ret;
> > }
> >
> > static int mtk_dsi_remove(struct platform_device *pdev)
> > --
> > 2.20.1
> >
On Thu, 2019-02-14 at 13:48 +0800, Nicolas Boichat wrote:
> On Thu, Feb 14, 2019 at 12:42 PM Jitao Shi <[email protected]> wrote:
> >
> > Config the different CMDQ reg address in driver data.
> >
> > Signed-off-by: Jitao Shi <[email protected]>
> > ---
> > drivers/gpu/drm/mediatek/mtk_dsi.c | 39 ++++++++++++++++++++++++------
> > 1 file changed, 31 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 93fa255b4aad..80db02a25cb0 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -156,6 +156,10 @@
> >
> > struct phy;
> >
> > +struct mtk_dsi_driver_data {
> > + const u32 reg_cmdq_off;
> > +};
> > +
> > struct mtk_dsi {
> > struct mtk_ddp_comp ddp_comp;
> > struct device *dev;
> > @@ -182,6 +186,7 @@ struct mtk_dsi {
> > bool enabled;
> > u32 irq_data;
> > wait_queue_head_t irq_wait_queue;
> > + struct mtk_dsi_driver_data *driver_data;
> > };
> >
> > static inline struct mtk_dsi *encoder_to_dsi(struct drm_encoder *e)
> > @@ -934,6 +939,7 @@ static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
> > const char *tx_buf = msg->tx_buf;
> > u8 config, cmdq_size, cmdq_off, type = msg->type;
> > u32 reg_val, cmdq_mask, i;
> > + u32 reg_cmdq_off = dsi->driver_data->reg_cmdq_off;
> >
> > if (MTK_DSI_HOST_IS_READ(type))
> > config = BTA;
> > @@ -953,9 +959,11 @@ static void mtk_dsi_cmdq(struct mtk_dsi *dsi, const struct mipi_dsi_msg *msg)
> > }
> >
> > for (i = 0; i < msg->tx_len; i++)
> > - writeb(tx_buf[i], dsi->regs + DSI_CMDQ0 + cmdq_off + i);
> > + mtk_dsi_mask(dsi, (reg_cmdq_off + cmdq_off + i) & (~0x3U),
> > + (0xffUL << (((i + cmdq_off) & 3U) * 8U)),
> > + tx_buf[i] << (((i + cmdq_off) & 3U) * 8U));
>
> I found the writeb call _much_ clearer ... Either switch back to that,
> or create a new mtk_disk_mask_byte function maybe?
I'll fix it next version.
>
> >
> > - mtk_dsi_mask(dsi, DSI_CMDQ0, cmdq_mask, reg_val);
> > + mtk_dsi_mask(dsi, reg_cmdq_off, cmdq_mask, reg_val);
>
> You're removing DSI_CMDQ0 usage in this patch, so remove the #define
> in this patch too (instead of doing that in 3/3).
I'll fix it next version.
>
> > mtk_dsi_mask(dsi, DSI_CMDQ_SIZE, CMDQ_SIZE, cmdq_size);
> > }
> >
> > @@ -1074,10 +1082,27 @@ static const struct component_ops mtk_dsi_component_ops = {
> > .unbind = mtk_dsi_unbind,
> > };
> >
> > +static const struct mtk_dsi_driver_data mt8173_dsi_driver_data = {
> > + .reg_cmdq_off = 0x200,
> > +};
> > +
> > +static const struct mtk_dsi_driver_data mt2701_dsi_driver_data = {
> > + .reg_cmdq_off = 0x180,
> > +};
> > +
> > +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 },
> > + { },
> > +};
> > +
> > static int mtk_dsi_probe(struct platform_device *pdev)
> > {
> > struct mtk_dsi *dsi;
> > struct device *dev = &pdev->dev;
> > + const struct of_device_id *of_id;
> > struct resource *regs;
> > int irq_num;
> > int comp_id;
> > @@ -1101,6 +1126,10 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> > if (ret)
> > goto err_unregister_host;
> >
> > + of_id = of_match_device(mtk_dsi_of_match, &pdev->dev);
> > + dsi->driver_data = (struct mtk_dsi_driver_data *)
> > + of_id->data;
>
> This fits in 80 chars. Also, of_id->data is a void*, so no cast needed.
I'll fix it next version.
>
> > +
> > dsi->engine_clk = devm_clk_get(dev, "engine");
> > if (IS_ERR(dsi->engine_clk)) {
> > ret = PTR_ERR(dsi->engine_clk);
> > @@ -1194,12 +1223,6 @@ static int mtk_dsi_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > -static const struct of_device_id mtk_dsi_of_match[] = {
> > - { .compatible = "mediatek,mt2701-dsi" },
> > - { .compatible = "mediatek,mt8173-dsi" },
> > - { },
> > -};
>
> Any reason you moved this up?
of_match_device is called by mtk_dsi_probe.
Best regards
Jitao
>
> > -
> > struct platform_driver mtk_dsi_driver = {
> > .probe = mtk_dsi_probe,
> > .remove = mtk_dsi_remove,
> > --
> > 2.20.1
> >
On Thu, 2019-02-14 at 13:54 +0800, Nicolas Boichat wrote:
> On Thu, Feb 14, 2019 at 12:43 PM Jitao Shi <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
>
> On Thu, 2019-02-14 at 13:54 +0800, Nicolas Boichat wrote:
> > On Thu, Feb 14, 2019 at 12:43 PM Jitao Shi <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> > >
>
>