Am Mittwoch, den 27.07.2016, 16:31 +0800 schrieb Bibby Hsieh:

From: Junzhi Zhao junzhi.zhao@mediatek.com

The mtk_hdmi_send_infoframe have to be run after PLL and PIXEL clock of HDMI enable. Make sure that HDMI inforframes can be sent successfully.

Signed-off-by: Junzhi Zhao junzhi.zhao@mediatek.com Signed-off-by: Bibby Hsieh bibby.hsieh@mediatek.com

---

drivers/gpu/drm/mediatek/mtk_hdmi.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index ba812ef..d8609f5 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1133,12 +1133,6 @@ static int mtk_hdmi_output_set_display_mode(struct mtk_hdmi *hdmi, phy_power_on(hdmi->phy); mtk_hdmi_aud_output_config(hdmi, mode);

• mtk_hdmi_setup_audio_infoframe(hdmi);
• mtk_hdmi_setup_avi_infoframe(hdmi, mode);
• mtk_hdmi_setup_spd_infoframe(hdmi, "mediatek", "On-chip HDMI");
• if (mode->flags & DRM_MODE_FLAG_3D_MASK)

• mtk_hdmi_setup_vendor_specific_infoframe(hdmi, mode);
  

• mtk_hdmi_hw_vid_black(hdmi, false); mtk_hdmi_hw_aud_unmute(hdmi); mtk_hdmi_hw_send_av_unmute(hdmi);

@@ -1401,14 +1395,25 @@ static void mtk_hdmi_bridge_pre_enable(struct drm_bridge *bridge) hdmi->powered = true; }

+static void mtk_hdmi_send_infoframe(struct mtk_hdmi *hdmi,

• 		    struct drm_display_mode *mode)
  

+{

• mtk_hdmi_setup_audio_infoframe(hdmi);
• mtk_hdmi_setup_avi_infoframe(hdmi, mode);
• mtk_hdmi_setup_spd_infoframe(hdmi, "mediatek", "On-chip HDMI");
• if (mode->flags & DRM_MODE_FLAG_3D_MASK)

• mtk_hdmi_setup_vendor_specific_infoframe(hdmi, mode);
  

+}

static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge) { struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);

• phy_power_on(hdmi->phy); mtk_hdmi_output_set_display_mode(hdmi, &hdmi->mode); clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]); clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);

• phy_power_on(hdmi->phy);

This change is not described in the patch description. Why is the phy power on moved after the pixel clock enable?

• mtk_hdmi_send_infoframe(hdmi, &hdmi->mode);

  hdmi->enabled = true;

}

regards Philipp

Hi, Philipp,

Thanks for your review.

On Wed, 2016-07-27 at 11:25 +0200, Philipp Zabel wrote:

Am Mittwoch, den 27.07.2016, 16:31 +0800 schrieb Bibby Hsieh:

...

From: Junzhi Zhao junzhi.zhao@mediatek.com

In order to improve 4K resolution performance, we have to enhance the HDMI driving currend

                                       ^

Typo, s/currend/current/

Besides that, this patch looks good to me.

Ok, I will fix that, thanks.

regards Philipp

...

when clock rate is greater than 165MHz.

Signed-off-by: Junzhi Zhao junzhi.zhao@mediatek.com Signed-off-by: Bibby Hsieh bibby.hsieh@mediatek.com

---

drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c | 42 +++++++++++++++++------- 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c b/drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c index 8a24754..51cb9cf 100644 --- a/drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c +++ b/drivers/gpu/drm/mediatek/mtk_mt8173_hdmi_phy.c @@ -265,6 +265,9 @@ static int mtk_hdmi_pll_set_rate(struct clk_hw *hw, unsigned long rate, struct mtk_hdmi_phy *hdmi_phy = to_mtk_hdmi_phy(hw); unsigned int pre_div; unsigned int div;

• unsigned int pre_ibias;

• unsigned int hdmi_ibias;

• unsigned int imp_en;

  dev_dbg(hdmi_phy->dev, "%s: %lu Hz, parent: %lu Hz\n", __func__, rate, parent_rate);

@@ -298,18 +301,31 @@ static int mtk_hdmi_pll_set_rate(struct clk_hw *hw, unsigned long rate, (0x1 << PLL_BR_SHIFT), RG_HDMITX_PLL_BP | RG_HDMITX_PLL_BC | RG_HDMITX_PLL_BR);

• mtk_hdmi_phy_clear_bits(hdmi_phy, HDMI_CON3, RG_HDMITX_PRD_IMP_EN);

• if (rate < 165000000) {

• mtk_hdmi_phy_clear_bits(hdmi_phy, HDMI_CON3,
  

• 			RG_HDMITX_PRD_IMP_EN);
  

• pre_ibias = 0x3;
  

• imp_en = 0x0;
  

• hdmi_ibias = hdmi_phy->ibias;
  

• } else {

• mtk_hdmi_phy_set_bits(hdmi_phy, HDMI_CON3,
  

• 		      RG_HDMITX_PRD_IMP_EN);
  

• pre_ibias = 0x6;
  

• imp_en = 0xf;
  

• hdmi_ibias = hdmi_phy->ibias_up;
  

• } mtk_hdmi_phy_mask(hdmi_phy, HDMI_CON4,

• 	  (0x3 << PRD_IBIAS_CLK_SHIFT) |
  

• 	  (0x3 << PRD_IBIAS_D2_SHIFT) |
  

• 	  (0x3 << PRD_IBIAS_D1_SHIFT) |
  

• 	  (0x3 << PRD_IBIAS_D0_SHIFT),
  

• 	  (pre_ibias << PRD_IBIAS_CLK_SHIFT) |
  

• 	  (pre_ibias << PRD_IBIAS_D2_SHIFT) |
  

• 	  (pre_ibias << PRD_IBIAS_D1_SHIFT) |
  

• 	  (pre_ibias << PRD_IBIAS_D0_SHIFT),
    RG_HDMITX_PRD_IBIAS_CLK |
    RG_HDMITX_PRD_IBIAS_D2 |
    RG_HDMITX_PRD_IBIAS_D1 |
    RG_HDMITX_PRD_IBIAS_D0);
  

  mtk_hdmi_phy_mask(hdmi_phy, HDMI_CON3,

• 	  (0x0 << DRV_IMP_EN_SHIFT), RG_HDMITX_DRV_IMP_EN);
  

• 	  (imp_en << DRV_IMP_EN_SHIFT),
  

• 	  RG_HDMITX_DRV_IMP_EN);
  

  mtk_hdmi_phy_mask(hdmi_phy, HDMI_CON6, (hdmi_phy->drv_imp_clk << DRV_IMP_CLK_SHIFT) | (hdmi_phy->drv_imp_d2 << DRV_IMP_D2_SHIFT) |

@@ -318,12 +334,14 @@ static int mtk_hdmi_pll_set_rate(struct clk_hw *hw, unsigned long rate, RG_HDMITX_DRV_IMP_CLK | RG_HDMITX_DRV_IMP_D2 | RG_HDMITX_DRV_IMP_D1 | RG_HDMITX_DRV_IMP_D0); mtk_hdmi_phy_mask(hdmi_phy, HDMI_CON5,

• 	  (hdmi_phy->ibias << DRV_IBIAS_CLK_SHIFT) |
  

• 	  (hdmi_phy->ibias << DRV_IBIAS_D2_SHIFT) |
  

• 	  (hdmi_phy->ibias << DRV_IBIAS_D1_SHIFT) |
  

• 	  (hdmi_phy->ibias << DRV_IBIAS_D0_SHIFT),
  

• 	  RG_HDMITX_DRV_IBIAS_CLK | RG_HDMITX_DRV_IBIAS_D2 |
  

• 	  RG_HDMITX_DRV_IBIAS_D1 | RG_HDMITX_DRV_IBIAS_D0);
  

• 	  (hdmi_ibias << DRV_IBIAS_CLK_SHIFT) |
  

• 	  (hdmi_ibias << DRV_IBIAS_D2_SHIFT) |
  

• 	  (hdmi_ibias << DRV_IBIAS_D1_SHIFT) |
  

• 	  (hdmi_ibias << DRV_IBIAS_D0_SHIFT),
  

• 	  RG_HDMITX_DRV_IBIAS_CLK |
  

• 	  RG_HDMITX_DRV_IBIAS_D2 |
  

• 	  RG_HDMITX_DRV_IBIAS_D1 |
  

• 	  RG_HDMITX_DRV_IBIAS_D0);
  

  return 0;

}

Am Mittwoch, den 27.07.2016, 16:31 +0800 schrieb Bibby Hsieh:

From: Junzhi Zhao junzhi.zhao@mediatek.com

Pixel clock should be 297MHz when resolution is 4K.

This patch does two different things, please don't conflate them.

First, it adds support for 16*3 and 2*3 factors between PLL and pixel clock. This should be a separate patch and does what this patch description says.

The second part moves the clocks into an array and adds an indirection to clock registration depending on of_match_node. This should be a separate patch. I suppose this change is in preparation for mt2701 support, but I'm not sure yet if it is needed at all.

regards Philipp

Signed-off-by: Junzhi Zhao junzhi.zhao@mediatek.com Signed-off-by: Bibby Hsieh bibby.hsieh@mediatek.com

---

drivers/gpu/drm/mediatek/mtk_dpi.c | 149 +++++++++++++++++++++++------------- 1 file changed, 96 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c index d05ca79..fa390e0 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi.c +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c @@ -60,14 +60,25 @@ enum mtk_dpi_out_color_format { MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL };

+enum mtk_dpi_clk_id {

• MTK_DPI_CLK_DPI_ENGINE,
• MTK_DPI_CLK_DPI_PIXEL,
• MTK_DPI_CLK_TVD_PLL,
• MTK_DPI_CLK_COUNT,

+};

+static const char * const mtk_dpi_clk_names[MTK_DPI_CLK_COUNT] = {

• [MTK_DPI_CLK_DPI_ENGINE] = "engine",
• [MTK_DPI_CLK_DPI_PIXEL] = "pixel",
• [MTK_DPI_CLK_TVD_PLL] = "pll",

+};

struct mtk_dpi { struct mtk_ddp_comp ddp_comp; struct drm_encoder encoder; void __iomem *regs; struct device *dev;

• struct clk *engine_clk;
• struct clk *pixel_clk;
• struct clk *tvd_clk;

• struct clk *clk[MTK_DPI_CLK_COUNT]; int irq; struct drm_display_mode mode; enum mtk_dpi_out_color_format color_format;

@@ -76,6 +87,7 @@ struct mtk_dpi { enum mtk_dpi_out_channel_swap channel_swap; bool power_sta; u8 power_ctl;

• void *data;

};

static inline struct mtk_dpi *mtk_dpi_from_encoder(struct drm_encoder *e) @@ -114,6 +126,10 @@ struct mtk_dpi_yc_limit { u16 c_bottom; };

+struct mtk_dpi_conf {

• int (*clk_config)(struct mtk_dpi *dpi, struct drm_display_mode *mode);

+};

static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask) { u32 tmp = readl(dpi->regs + offset) & ~mask; @@ -377,8 +393,8 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi, enum mtk_dpi_power_ctl pctl) return;

mtk_dpi_disable(dpi);

• clk_disable_unprepare(dpi->pixel_clk);
• clk_disable_unprepare(dpi->engine_clk);

• clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]);
• clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]); dpi->power_sta = false;

}

@@ -395,13 +411,13 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum mtk_dpi_power_ctl pctl) if (dpi->power_sta) return 0;

• ret = clk_prepare_enable(dpi->engine_clk);

• ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]); if (ret) { dev_err(dpi->dev, "Failed to enable engine clock: %d\n", ret); goto err_eng; }

• ret = clk_prepare_enable(dpi->pixel_clk);

• ret = clk_prepare_enable(dpi->clk[MTK_DPI_CLK_DPI_PIXEL]); if (ret) { dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret); goto err_pixel;

@@ -412,7 +428,7 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi, enum mtk_dpi_power_ctl pctl) return 0;

err_pixel:

• clk_disable_unprepare(dpi->engine_clk);

• clk_disable_unprepare(dpi->clk[MTK_DPI_CLK_DPI_ENGINE]);

err_eng: dpi->power_ctl &= ~pctl; return ret; @@ -428,34 +444,18 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi, struct mtk_dpi_sync_param vsync_leven = { 0 }; struct mtk_dpi_sync_param vsync_rodd = { 0 }; struct mtk_dpi_sync_param vsync_reven = { 0 };

• unsigned long pix_rate;
• unsigned long pll_rate;
• unsigned int factor;

• struct mtk_dpi_conf *conf;

• int ret;

  if (!dpi) { dev_err(dpi->dev, "invalid argument\n"); return -EINVAL; }

• pix_rate = 1000UL * mode->clock;
• if (mode->clock <= 74000)

• factor = 8 * 3;
  

• else

• factor = 4 * 3;
  

• pll_rate = pix_rate * factor;
• dev_dbg(dpi->dev, "Want PLL %lu Hz, pixel clock %lu Hz\n",

• pll_rate, pix_rate);
  

• clk_set_rate(dpi->tvd_clk, pll_rate);
• pll_rate = clk_get_rate(dpi->tvd_clk);
• pix_rate = pll_rate / factor;
• clk_set_rate(dpi->pixel_clk, pix_rate);
• pix_rate = clk_get_rate(dpi->pixel_clk);
• dev_dbg(dpi->dev, "Got PLL %lu Hz, pixel clock %lu Hz\n",

• pll_rate, pix_rate);
  

• conf = (struct mtk_dpi_conf *)dpi->data;

• ret = conf->clk_config(dpi, mode);

• if (ret)

• dev_err(dpi->dev, "dpi clk set fail\n");
  
  

  limit.c_bottom = 0x0010; limit.c_top = 0x0FE0;

@@ -656,20 +656,83 @@ static const struct component_ops mtk_dpi_component_ops = { .unbind = mtk_dpi_unbind, };

+static int mtk_dpi_parse_clk_from_dt(struct mtk_dpi *dpi,

• 		     struct device_node *np)
  

+{

• int i;
• for (i = 0; i < ARRAY_SIZE(mtk_dpi_clk_names); i++) {

• dpi->clk[i] = of_clk_get_by_name(np,
  

• 				  mtk_dpi_clk_names[i]);
  

• if (IS_ERR(dpi->clk[i]))
  

• 	return PTR_ERR(dpi->clk[i]);
  

• }
• return 0;

+}

+static int mt8173_clk_config(struct mtk_dpi *dpi, struct drm_display_mode *mode) +{

• unsigned long pix_rate;
• unsigned long pll_rate;
• unsigned int factor;
• pix_rate = 1000UL * mode->clock;
• if (mode->clock <= 27000)

• factor = 16 * 3;
  

• else if (mode->clock <= 74250)

• factor = 8 * 3;
  

• else if (mode->clock <= 167000)

• factor = 4 * 3;
  

• else

• factor = 2 * 3;
  

• pll_rate = pix_rate * factor;
• dev_dbg(dpi->dev, "Want PLL %lu Hz, pixel clock %lu Hz\n",

• pll_rate, pix_rate);
  

• pll_rate = clk_get_rate(dpi->clk[MTK_DPI_CLK_TVD_PLL]);
• pix_rate = pll_rate / factor;
• clk_set_rate(dpi->clk[MTK_DPI_CLK_DPI_PIXEL], pix_rate);
• dev_dbg(dpi->dev, "Got PLL %lu Hz, pixel clock %lu Hz\n",

• pll_rate, pix_rate);
  

• return 0;

+}

+static const struct mtk_dpi_conf mt8173_conf = {

• .clk_config = mt8173_clk_config,

+};

+static const struct of_device_id mtk_dpi_of_ids[] = {

• { .compatible = "mediatek,mt8173-dpi",

• .data = &mt8173_conf,
  

• },
• {}

+};

static int mtk_dpi_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct mtk_dpi *dpi; struct resource *mem;

• struct device_node *np = dev->of_node; struct device_node *ep, *bridge_node = NULL; int comp_id;

• const struct of_device_id *match;

• struct mtk_dpi_conf *conf; int ret;

• match = of_match_node(mtk_dpi_of_ids, dev->of_node);

• if (!match)

• return -ENODEV;
  

• dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL); if (!dpi) return -ENOMEM;

  dpi->dev = dev;

• dpi->data = (void *)match->data;

• conf = (struct mtk_dpi_conf *)match->data;

  mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); dpi->regs = devm_ioremap_resource(dev, mem);

@@ -679,24 +742,9 @@ static int mtk_dpi_probe(struct platform_device *pdev) return ret; }

• dpi->engine_clk = devm_clk_get(dev, "engine");
• if (IS_ERR(dpi->engine_clk)) {

• ret = PTR_ERR(dpi->engine_clk);
  

• dev_err(dev, "Failed to get engine clock: %d\n", ret);
  

• return ret;
  

• }
• dpi->pixel_clk = devm_clk_get(dev, "pixel");
• if (IS_ERR(dpi->pixel_clk)) {

• ret = PTR_ERR(dpi->pixel_clk);
  

• dev_err(dev, "Failed to get pixel clock: %d\n", ret);
  

• return ret;
  

• }
• dpi->tvd_clk = devm_clk_get(dev, "pll");
• if (IS_ERR(dpi->tvd_clk)) {

• ret = PTR_ERR(dpi->tvd_clk);
  

• dev_err(dev, "Failed to get tvdpll clock: %d\n", ret);
  

• ret = mtk_dpi_parse_clk_from_dt(dpi, np);
• if (ret) {

• dev_err(dev, "parse tvd div clk failed!");
  

  return ret; }

@@ -754,11 +802,6 @@ static int mtk_dpi_remove(struct platform_device *pdev) return 0; }

-static const struct of_device_id mtk_dpi_of_ids[] = {

• { .compatible = "mediatek,mt8173-dpi", },
• {}

-};

struct platform_driver mtk_dpi_driver = { .probe = mtk_dpi_probe, .remove = mtk_dpi_remove,

On Wed, Jul 27, 2016 at 04:31:32PM +0800, Bibby Hsieh wrote:

From: Junzhi Zhao junzhi.zhao@mediatek.com

Pixel clock should be 297MHz when resolution is 4K.

Signed-off-by: Junzhi Zhao junzhi.zhao@mediatek.com Signed-off-by: Bibby Hsieh bibby.hsieh@mediatek.com

---

drivers/gpu/drm/mediatek/mtk_dpi.c | 149 +++++++++++++++++++++++------------- 1 file changed, 96 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c index d05ca79..fa390e0 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi.c +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c @@ -60,14 +60,25 @@ enum mtk_dpi_out_color_format { MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL };

+enum mtk_dpi_clk_id {

• MTK_DPI_CLK_DPI_ENGINE,
• MTK_DPI_CLK_DPI_PIXEL,
• MTK_DPI_CLK_TVD_PLL,
• MTK_DPI_CLK_COUNT,

+};

+static const char * const mtk_dpi_clk_names[MTK_DPI_CLK_COUNT] = {

• [MTK_DPI_CLK_DPI_ENGINE] = "engine",
• [MTK_DPI_CLK_DPI_PIXEL] = "pixel",
• [MTK_DPI_CLK_TVD_PLL] = "pll",

+};

struct mtk_dpi { struct mtk_ddp_comp ddp_comp; struct drm_encoder encoder; void __iomem *regs; struct device *dev;

• struct clk *engine_clk;
• struct clk *pixel_clk;
• struct clk *tvd_clk;

• struct clk *clk[MTK_DPI_CLK_COUNT];

This looks to me like a step backwards. All accesses to these clocks are now very clumsy and I don't see any advantage in using this array rather than individually named clocks.

Also, that change seems completely unrelated to the description in the commit message.

int irq; struct drm_display_mode mode; enum mtk_dpi_out_color_format color_format; @@ -76,6 +87,7 @@ struct mtk_dpi { enum mtk_dpi_out_channel_swap channel_swap; bool power_sta; u8 power_ctl;

• void *data;

This should probably be const. It's a bad idea to cast away the const from the of_device_id.data that this is assigned from.

+static const struct of_device_id mtk_dpi_of_ids[] = {

• { .compatible = "mediatek,mt8173-dpi",

• .data = &mt8173_conf,
  

• },

Please align this in some standard way. It all fits on one line, so the most obvious choice would've been:

{ .compatible = "mediatek,mt8173-dpi", .data = &mt8173_conf },

• {}

+};

static int mtk_dpi_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct mtk_dpi *dpi; struct resource *mem;

• struct device_node *np = dev->of_node; struct device_node *ep, *bridge_node = NULL; int comp_id;

• const struct of_device_id *match;

• struct mtk_dpi_conf *conf; int ret;

• match = of_match_node(mtk_dpi_of_ids, dev->of_node);

• if (!match)

• return -ENODEV;
  

You introduce a variable np = dev->of_node above, but then you add code that uses dev->of_node directly?

That said, you should use of_device_get_match_data() anyway, so that you can omit...

• dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL); if (!dpi) return -ENOMEM;

  dpi->dev = dev;

• dpi->data = (void *)match->data;

• conf = (struct mtk_dpi_conf *)match->data;

... the dereferences here. Also, like I said before you should make dpi->data and conf both const to avoid casting away the constness of the data. If you do that you can even drop the casts because you'd be casting from a const void * to a const foo *, which gets casted automatically.

@@ -754,11 +802,6 @@ static int mtk_dpi_remove(struct platform_device *pdev) return 0; }

-static const struct of_device_id mtk_dpi_of_ids[] = {

• { .compatible = "mediatek,mt8173-dpi", },
• {}

-};

Another advantage of using of_device_get_match_data() is that it doesn't need direct access to the of_device_id table (it'll obtain it via an indirect dev->driver->of_match_table), so you don't have to move this around and make this patch overall less churn.

Thierry