Hi, Jitao:

On Sun, 2019-05-19 at 17:25 +0800, Jitao Shi 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.

In [1], you have agreed this patch just for delay not for probe fail Remove 'probe fail'.in commit message.

[1] http://lists.infradead.org/pipermail/linux-mediatek/2019-May/019836.html

So move the mipi_dsi_host_register to probe from bind.

Signed-off-by: Jitao Shi jitao.shi@mediatek.com

drivers/gpu/drm/mediatek/mtk_dsi.c | 50 ++++++++++++++++++------------ 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index b00eb2d2e086..6c4ac37f983d 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;

This is for debug message, not for the timing of mipi_dsi_host_register(), so move to an independent patch.

Regards, CK

• ret = mipi_dsi_host_register(&dsi->host);

• if (ret < 0) {

• dev_err(dev, "failed to register DSI host: %d\n", ret);
  

• 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;
  

• dev_err(&pdev->dev, "failed to get dsi irq_num: %d\n", irq_num);
  

• ret = irq_num;
  

• goto err_unregister_host;
  

  }

  irq_set_status_flags(irq_num, IRQ_TYPE_LEVEL_LOW);

@@ -1163,14 +1163,24 @@ 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;
  

• 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) {

• dev_err(&pdev->dev, "failed to add component: %d\n", ret);
  

• 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)

Hi, Jitao:

On Sun, 2019-05-19 at 17:25 +0800, Jitao Shi wrote:

New DSI IP has shadow register and working reg. The register values are writen to shadow register. And then trigger with commit reg, the register values will be moved working register.

This fucntion is defualt on. But this driver doesn't use this function. So add the disable control.

Signed-off-by: Jitao Shi jitao.shi@mediatek.com

drivers/gpu/drm/mediatek/mtk_dsi.c | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index a48db056df6c..fd367985c7fd 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -131,6 +131,10 @@ #define VM_CMD_EN BIT(0) #define TS_VFP_EN BIT(5)

+#define DSI_SHADOW_DEBUG 0x190U +#define FORCE_COMMIT BIT(0) +#define BYPASS_SHADOW BIT(1)

One more 'tab' for bitwise definition.

Regards, CK

#define CONFIG (0xff << 0) #define SHORT_PACKET 0 #define LONG_PACKET 2 @@ -157,6 +161,7 @@ struct phy;

struct mtk_dsi_driver_data { const u32 reg_cmdq_off;

• bool has_shadow_ctl;

};

struct mtk_dsi { @@ -594,6 +599,11 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) }

mtk_dsi_enable(dsi);

• if (dsi->driver_data->has_shadow_ctl)

• writel(FORCE_COMMIT | BYPASS_SHADOW,
  

•        dsi->regs + DSI_SHADOW_DEBUG);
  

• mtk_dsi_reset_engine(dsi); mtk_dsi_phy_timconfig(dsi);

On Sun, May 19, 2019 at 05:25:36PM +0800, Jitao Shi wrote:

Change the method of frame rate calc which can get more accurate frame rate.

data rate = pixel_clock * bit_per_pixel / lanes Adjust hfp_wc to adapt the additional phy_data

if MIPI_DSI_MODE_VIDEO_BURST hfp_wc = hfp * bpp - data_phy_cycles * lanes - 12 - 6; else hfp_wc = hfp * bpp - data_phy_cycles * lanes - 12;

Note: //(2: 1 for sync, 1 for phy idle) data_phy_cycles = T_hs_exit + T_lpx + T_hs_prepare + T_hs_zero + 2;

bpp: bit per pixel

Signed-off-by: Jitao Shi jitao.shi@mediatek.com Tested-by: Ryan Case ryandcase@chromium.org

---

drivers/gpu/drm/mediatek/mtk_dsi.c | 119 +++++++++++++++++++++-------- 1 file changed, 86 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 1165ff944889..3f51b2000c68 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -158,6 +158,25 @@ (type == MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM) || \ (type == MIPI_DSI_DCS_READ))

+struct mtk_phy_timing {

• u32 lpx;
• u32 da_hs_prepare;
• u32 da_hs_zero;
• u32 da_hs_trail;
• u32 ta_go;
• u32 ta_sure;
• u32 ta_get;
• u32 da_hs_exit;
• u32 clk_hs_zero;
• u32 clk_hs_trail;
• u32 clk_hs_prepare;
• u32 clk_hs_post;
• u32 clk_hs_exit;

+};

struct phy;

struct mtk_dsi_driver_data { @@ -182,12 +201,13 @@ struct mtk_dsi { struct clk *digital_clk; struct clk *hs_clk;

• u32 data_rate;

• u64 data_rate;

This results in 64-bit divide operations and thus build failures with 32-bit builds. More on that below.

unsigned long mode_flags; enum mipi_dsi_pixel_format format; unsigned int lanes; struct videomode vm;

• struct mtk_phy_timing phy_timing; int refcount; bool enabled; u32 irq_data;

@@ -221,17 +241,39 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi) { u32 timcon0, timcon1, timcon2, timcon3; u32 ui, cycle_time;

• struct mtk_phy_timing *timing = &dsi->phy_timing;
• ui = 1000000000 / dsi->data_rate;
• cycle_time = 8000000000 / dsi->data_rate;

This results in 64-bit divide operations. On top of that, 8000000000 is larger than 0xffffffff, resulting in an integer overflow on 32-bit systems; it should be provided as 8000000000ULL.

• timing->lpx = NS_TO_CYCLE(60, cycle_time);
• timing->da_hs_prepare = NS_TO_CYCLE((40 + 5 * ui), cycle_time);
• timing->da_hs_zero = NS_TO_CYCLE((110 + 6 * ui), cycle_time);
• timing->da_hs_trail = NS_TO_CYCLE(((0x4 * ui) + 80), cycle_time);
• if (timing->da_hs_zero > timing->da_hs_prepare)

• timing->da_hs_zero -= timing->da_hs_prepare;
  

• timing->ta_go = 4 * timing->lpx;
• timing->ta_sure = 3 * timing->lpx / 2;
• timing->ta_get = 5 * timing->lpx;
• timing->da_hs_exit = 2 * timing->lpx;
• timing->clk_hs_zero = NS_TO_CYCLE(0x150, cycle_time);
• timing->clk_hs_trail = NS_TO_CYCLE(0x64, cycle_time) + 0xa;

• ui = 1000 / dsi->data_rate + 0x01;
• cycle_time = 8000 / dsi->data_rate + 0x01;

• timing->clk_hs_prepare = NS_TO_CYCLE(0x40, cycle_time);
• timing->clk_hs_post = NS_TO_CYCLE(80 + 52 * ui, cycle_time);
• timing->clk_hs_exit = 2 * timing->lpx;

• timcon0 = T_LPX | T_HS_PREP << 8 | T_HS_ZERO << 16 | T_HS_TRAIL << 24;
• timcon1 = 4 * T_LPX | (3 * T_LPX / 2) << 8 | 5 * T_LPX << 16 |

•   T_HS_EXIT << 24;
  

• timcon2 = ((NS_TO_CYCLE(0x64, cycle_time) + 0xa) << 24) |

•   (NS_TO_CYCLE(0x150, cycle_time) << 16);
  

• timcon3 = NS_TO_CYCLE(0x40, cycle_time) | (2 * T_LPX) << 16 |

•   NS_TO_CYCLE(80 + 52 * ui, cycle_time) << 8;
  

• timcon0 = timing->lpx | timing->da_hs_prepare << 8 |

•   timing->da_hs_zero << 16 | timing->da_hs_trail << 24;
  

• timcon1 = timing->ta_go | timing->ta_sure << 8 |

•   timing->ta_get << 16 | timing->da_hs_exit << 24;
  

• timcon2 = 1 << 8 | timing->clk_hs_zero << 16 |

•   timing->clk_hs_trail << 24;
  

• timcon3 = timing->clk_hs_prepare | timing->clk_hs_post << 8 |

•   timing->clk_hs_exit << 16;
  
  

  writel(timcon0, dsi->regs + DSI_PHY_TIMECON0); writel(timcon1, dsi->regs + DSI_PHY_TIMECON1);

@@ -418,7 +460,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) u32 horizontal_sync_active_byte; u32 horizontal_backporch_byte; u32 horizontal_frontporch_byte;

• u32 dsi_tmp_buf_bpp;

• u32 dsi_tmp_buf_bpp, data_phy_cycles;

• struct mtk_phy_timing *timing = &dsi->phy_timing;

  struct videomode *vm = &dsi->vm;

@@ -433,7 +476,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) 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);
  

• writel(vm->vactive << 16 | vm->hactive,
  

•        dsi->regs + DSI_SIZE_CON);
  
  

  horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10);

@@ -444,7 +488,34 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) horizontal_backporch_byte = ((vm->hback_porch + vm->hsync_len) * dsi_tmp_buf_bpp - 10);

• horizontal_frontporch_byte = (vm->hfront_porch * dsi_tmp_buf_bpp - 12);

• data_phy_cycles = timing->lpx + timing->da_hs_prepare +

• 		  timing->da_hs_zero + timing->da_hs_exit + 2;
  

• if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {

• if (vm->hfront_porch * dsi_tmp_buf_bpp >
  

•     data_phy_cycles * dsi->lanes + 18) {
  

• 	horizontal_frontporch_byte = vm->hfront_porch *
  

• 				     dsi_tmp_buf_bpp -
  

• 				     data_phy_cycles *
  

• 				     dsi->lanes - 18;
  

• } else {
  

• 	DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
  

• 	horizontal_frontporch_byte = vm->hfront_porch *
  

• 				     dsi_tmp_buf_bpp;
  

• }
  

• } else {

• if (vm->hfront_porch * dsi_tmp_buf_bpp >
  

•     data_phy_cycles * dsi->lanes + 12) {
  

• 	horizontal_frontporch_byte = vm->hfront_porch *
  

• 				     dsi_tmp_buf_bpp -
  

• 				     data_phy_cycles *
  

• 				     dsi->lanes - 12;
  

• } else {
  

• 	DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
  

• 	horizontal_frontporch_byte = vm->hfront_porch *
  

• 				     dsi_tmp_buf_bpp;
  

• }
  

• }

  writel(horizontal_sync_active_byte, dsi->regs + DSI_HSA_WC); writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);

@@ -544,8 +615,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) { struct device *dev = dsi->dev; int ret;

• u64 pixel_clock, total_bits;
• u32 htotal, htotal_bits, bit_per_pixel, overhead_cycles, overhead_bits;

• u32 bit_per_pixel;

  if (++dsi->refcount != 1) return 0;

@@ -564,24 +634,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) break; }

• /**

• * htotal_time = htotal * byte_per_pixel / num_lanes
  

• * overhead_time = lpx + hs_prepare + hs_zero + hs_trail + hs_exit
  

• * mipi_ratio = (htotal_time + overhead_time) / htotal_time
  

• * data_rate = pixel_clock * bit_per_pixel * mipi_ratio / num_lanes;
  

• */
  

• pixel_clock = dsi->vm.pixelclock;
• htotal = dsi->vm.hactive + dsi->vm.hback_porch + dsi->vm.hfront_porch +

• 	dsi->vm.hsync_len;
  

• htotal_bits = htotal * bit_per_pixel;
• overhead_cycles = T_LPX + T_HS_PREP + T_HS_ZERO + T_HS_TRAIL +

• 	T_HS_EXIT;
  

• overhead_bits = overhead_cycles * dsi->lanes * 8;
• total_bits = htotal_bits + overhead_bits;
• dsi->data_rate = DIV_ROUND_UP_ULL(pixel_clock * total_bits,

• 			  htotal * dsi->lanes);
  

• dsi->data_rate = dsi->vm.pixelclock * bit_per_pixel / dsi->lanes;

pixelclock and bit_per_pixel are not u64, and neither is dsi->lanes. The above will thus be a 32-bit operation on 32-bit systems, and never really assign a true 64-bit value to data_rate. On top of that, clk_set_rate() expects an unsigned long argument. Declaring data_rate as anything but unsigned long has therefore no value.

Note that the old code took possible overflow conditions due to interim results larger than u32 / unsigned long into account. This is no longer the case. I don't know if this is a concern, but it might be worth watching out for. If it is not a concern because the code is not anymore expected to run on 32-bit systems, it should be restricted to 64-bit builds.

Guenter

ret = clk_set_rate(dsi->hs_clk, dsi->data_rate); if (ret < 0) {

Hi, Jitao:

On Sun, 2019-05-19 at 17:25 +0800, Jitao Shi wrote:

mtk_mipi_tx is the phy of mtk_dsi. mtk_dsi get the phy(mtk_mipi_tx) in probe().

So, mtk_mipi_tx init should be ahead of mtk_dsi. Or mtk_dsi will defer to wait mtk_mipi_tx probe done.

Reviewed-by: CK Hu ck.hu@mediatek.com

Signed-off-by: Jitao Shi jitao.shi@mediatek.com

drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index cf59ea9bccfd..583d533d9574 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -633,8 +633,8 @@ static struct platform_driver * const mtk_drm_drivers[] = { &mtk_disp_rdma_driver, &mtk_dpi_driver, &mtk_drm_platform_driver,

• &mtk_dsi_driver, &mtk_mipi_tx_driver,

• &mtk_dsi_driver,

};

static int __init mtk_drm_init(void)