Re: [PATCH 4/5] drm/msm/dsi: implement 6G v2.0+ DSI command broadcast

On Monday 12 March 2018 06:53 PM, Sibi S wrote:

From: Archit Taneja architt@codeaurora.org

I'm a bit uncertain about using this patch in its current state. Some reasons below.

Add command broadcast support for DSI 6G v2.0+ controller on SDM845

Signed-off-by: Sibi S sibis@codeaurora.org

drivers/gpu/drm/msm/dsi/dsi.h | 5 +++ drivers/gpu/drm/msm/dsi/dsi_cfg.c | 14 +++++++- drivers/gpu/drm/msm/dsi/dsi_host.c | 62 ++++++++++++++++++++++++++++++-- drivers/gpu/drm/msm/dsi/dsi_manager.c | 66 +++++++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h index dfa049d..22342c30 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.h +++ b/drivers/gpu/drm/msm/dsi/dsi.h @@ -96,7 +96,9 @@ struct msm_dsi { struct drm_connector *msm_dsi_manager_connector_init(u8 id); struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id); int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg); +int msm_dsi_manager_cmd_xfer_6g_v2(int id, const struct mipi_dsi_msg *msg); bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len); +bool msm_dsi_manager_cmd_xfer_trigger_6g_v2(int id, u32 dma_base, u32 len); void msm_dsi_manager_attach_dsi_device(int id, u32 device_flags); int msm_dsi_manager_register(struct msm_dsi *msm_dsi); void msm_dsi_manager_unregister(struct msm_dsi *msm_dsi); @@ -152,6 +154,9 @@ static inline int msm_dsi_pll_set_usecase(struct msm_dsi_pll *pll, struct msm_dsi_host; int msm_dsi_host_xfer_prepare(struct mipi_dsi_host *host, const struct mipi_dsi_msg *msg); +int msm_dsi_host_xfer_prepare_6g_v2(struct mipi_dsi_host *host,

• 			const struct mipi_dsi_msg *msg,
  

• 			bool broadcast, bool master);
  

  void msm_dsi_host_xfer_restore(struct mipi_dsi_host *host, const struct mipi_dsi_msg *msg); int msm_dsi_host_cmd_tx(struct mipi_dsi_host *host,

diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.c b/drivers/gpu/drm/msm/dsi/dsi_cfg.c index dc51aaa..dcdfb1b 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.c +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.c @@ -157,6 +157,18 @@ .dma_base_get = dsi_dma_base_get_6g, .calc_clk_rate = dsi_calc_clk_rate_6g, };

+const static struct msm_dsi_host_cfg_ops msm_dsi_6g_v2_host_ops = {

• .link_clk_enable = dsi_link_clk_enable_6g,
• .link_clk_disable = dsi_link_clk_disable_6g,
• .clk_init_ver = dsi_clk_init_6g_v2,
• .tx_buf_alloc = dsi_tx_buf_alloc_6g,
• .tx_buf_get = dsi_tx_buf_get_6g,
• .tx_buf_put = dsi_tx_buf_put_6g,
• .dma_base_get = dsi_dma_base_get_6g,
• .calc_clk_rate = dsi_calc_clk_rate_6g,

+};

• static const struct msm_dsi_cfg_handler dsi_cfg_handlers[] = { {MSM_DSI_VER_MAJOR_V2, MSM_DSI_V2_VER_MINOR_8064, &apq8064_dsi_cfg, &msm_dsi_v2_host_ops},

@@ -175,7 +187,7 @@ {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V1_4_1, &msm8996_dsi_cfg, &msm_dsi_6g_host_ops}, {MSM_DSI_VER_MAJOR_6G, MSM_DSI_6G_VER_MINOR_V2_2_1,

• &sdm845_dsi_cfg, &msm_dsi_6g_host_ops},
  

• &sdm845_dsi_cfg, &msm_dsi_6g_v2_host_ops},
  

  };

  const struct msm_dsi_cfg_handler *msm_dsi_cfg_get(u32 major, u32 minor)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index b755b69..bd61cad 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -1251,9 +1251,14 @@ static int dsi_cmd_dma_tx(struct msm_dsi_host *msm_host, int len) reinit_completion(&msm_host->dma_comp);

dsi_wait4video_eng_busy(msm_host);

• if ((cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) &&

• 	(cfg_hnd->minor < MSM_DSI_6G_VER_MINOR_V2_2_1))
  

• triggered = msm_dsi_manager_cmd_xfer_trigger(msm_host->id,
  

• 						dma_base, len);
  

• else

• triggered = msm_dsi_manager_cmd_xfer_trigger_6g_v2(
  

• 				  msm_host->id, dma_base, len);
  
  

• triggered = msm_dsi_manager_cmd_xfer_trigger(

• 				msm_host->id, dma_base, len);
  

  if (triggered) { ret = wait_for_completion_timeout(&msm_host->dma_comp, msecs_to_jiffies(200));

@@ -1602,13 +1607,21 @@ static ssize_t dsi_host_transfer(struct mipi_dsi_host *host, const struct mipi_dsi_msg *msg) { struct msm_dsi_host *msm_host = to_msm_dsi_host(host);

• const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd; int ret;

  if (!msg || !msm_host->power_on) return -EINVAL;

  mutex_lock(&msm_host->cmd_mutex);

• ret = msm_dsi_manager_cmd_xfer(msm_host->id, msg);

• if (((cfg_hnd->major == MSM_DSI_VER_MAJOR_6G) &&

• 	(cfg_hnd->minor < MSM_DSI_6G_VER_MINOR_V2_2_1)) ||
  

• 	(cfg_hnd->major == MSM_DSI_VER_MAJOR_V2))
  

• ret = msm_dsi_manager_cmd_xfer(msm_host->id, msg);
  

• else

• ret = msm_dsi_manager_cmd_xfer_6g_v2(msm_host->id, msg);
  

• mutex_unlock(&msm_host->cmd_mutex);

  return ret;

@@ -1992,6 +2005,49 @@ int msm_dsi_host_xfer_prepare(struct mipi_dsi_host *host, return 0; }

+static void dsi_host_prepare_broadcast_6g_v2(struct msm_dsi_host *msm_host,

• 			     bool broadcast, bool master)
  

+{

• u32 val = dsi_read(msm_host, REG_DSI_CMD_DMA_CTRL);
• if (broadcast)

• val |= BIT(31);
  

• else

• val &= ~BIT(31);
  

• if (master)

• val |= BIT(30);
  

• else

• val &= ~BIT(30);
  

• dsi_write(msm_host, REG_DSI_CMD_DMA_CTRL, val);
• ...

+int msm_dsi_host_xfer_prepare_6g_v2(struct mipi_dsi_host *host,

• 		    const struct mipi_dsi_msg *msg,
  

• 		    bool broadcast, bool master)
  

+{

• struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
• /* TODO: vote for bus bandwidth */
• if (!(msg->flags & MIPI_DSI_MSG_USE_LPM))

• dsi_set_tx_power_mode(0, msm_host);
  

• dsi_host_prepare_broadcast_6g_v2(msm_host, broadcast, master);
• msm_host->dma_cmd_ctrl_restore = dsi_read(msm_host, REG_DSI_CTRL);
• dsi_write(msm_host, REG_DSI_CTRL,

• msm_host->dma_cmd_ctrl_restore |
  

• DSI_CTRL_CMD_MODE_EN |
  

• DSI_CTRL_ENABLE);
  

• if ((broadcast && master) || !broadcast)

• dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_CMD_DMA_DONE, 1);
  

• return 0;

+}

This part above (i.e, the prepare for 6G v2.0+) looks okay.

• void msm_dsi_host_xfer_restore(struct mipi_dsi_host *host, const struct mipi_dsi_msg *msg) {

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index 4cb1cb6..393e3ff 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -812,6 +812,56 @@ int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg) return ret; }

+int msm_dsi_manager_cmd_xfer_6g_v2(int id, const struct mipi_dsi_msg *msg) +{

• struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
• struct msm_dsi *msm_dsi0 = dsi_mgr_get_dsi(DSI_0);
• struct mipi_dsi_host *host = msm_dsi->host;
• bool is_read = (msg->rx_buf && msg->rx_len);
• bool need_sync = (IS_SYNC_NEEDED() && !is_read);
• int ret;
• if (!msg->tx_buf || !msg->tx_len)

• return 0;
  

• if (need_sync && (id == DSI_0))

• return is_read ? msg->rx_len : msg->tx_len;
  

• if (need_sync && msm_dsi0) {

• ret = msm_dsi_host_xfer_prepare_6g_v2(msm_dsi0->host, msg,
  

• 				      true, true);
  

• if (ret) {
  

• 	pr_err("%s: failed to prepare non-trigger host, %d\n",
  

• 		__func__, ret);
  

• 	return ret;
  

• }
  

• }
• ret = msm_dsi_host_xfer_prepare_6g_v2(msm_dsi->host, msg,

• 			      need_sync ? true : false,
  

• 			      false);
  

• if (ret) {

• pr_err("%s: failed to prepare host, %d\n",
  

• 	__func__, ret);
  

• return ret;
  

• }
• if (need_sync && msm_dsi0) {

• ret = is_read ? msm_dsi_host_cmd_rx(msm_dsi0->host, msg) :
  

• 		msm_dsi_host_cmd_tx(msm_dsi0->host, msg);
  

• } else {

• ret = is_read ? msm_dsi_host_cmd_rx(msm_dsi->host, msg) :
  

• 		msm_dsi_host_cmd_tx(msm_dsi->host, msg);
  

• }
• msm_dsi_host_xfer_restore(host, msg);
• if (need_sync && msm_dsi0)

• msm_dsi_host_xfer_restore(msm_dsi0->host, msg);
  

• return ret;

+}

I think the func above has problems, I'd mostly copied it from the original msm_dsi_manager_cmd_xfer func. We don't know if the 'need_sync' feature is something that's even needed for SDM845. Also, we have assumed here msm_dsi0 will always be the clock master, we might have to make it flexible to work with either DSI being the clock master.

• bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len) { struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);

@@ -829,6 +879,22 @@ bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len) return true; }

+bool msm_dsi_manager_cmd_xfer_trigger_6g_v2(int id, u32 dma_base, u32 len) +{

• struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
• struct msm_dsi *msm_dsi0 = dsi_mgr_get_dsi(DSI_0);
• if (IS_SYNC_NEEDED() && (id == DSI_1))

• return false;
  

• msm_dsi_host_cmd_xfer_commit(msm_dsi1->host, dma_base, len);
• if (IS_SYNC_NEEDED() && msm_dsi0)

• msm_dsi_host_cmd_xfer_commit(msm_dsi0->host, dma_base, len);
  

This again is something I'd hacked to get the display running, we can't assume that msm_dsi0 is going to be the clock master.

Thanks, Archit

• return true;

+}

• void msm_dsi_manager_attach_dsi_device(int id, u32 device_flags) { struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);