Re: [PATCH v4 7/9] drm: bridge: dw-hdmi: Add support for custom PHY configuration

Hi Jose,

On Thursday 02 Mar 2017 12:50:13 Jose Abreu wrote:

On 01-03-2017 22:39, Laurent Pinchart wrote:

...

From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

The DWC HDMI TX controller interfaces with a companion PHY. While Synopsys provides multiple standard PHYs, SoC vendors can also integrate a custom PHY.

Modularize PHY configuration to support vendor PHYs through platform data. The existing PHY configuration code was originally written to support the DWC HDMI 3D TX PHY, and seems to be compatible with the DWC MLP PHY. The HDMI 2.0 PHY will require a separate configuration function.

Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com Signed-off-by: Laurent Pinchart

laurent.pinchart+renesas@ideasonboard.com

Changes since v1:

• Check pdata->phy_configure in hdmi_phy_configure() to avoid dereferencing NULL pointer.

---

drivers/gpu/drm/bridge/dw-hdmi.c | 109 ++++++++++++++++++++++------------ include/drm/bridge/dw_hdmi.h | 7 +++ 2 files changed, 81 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index cb2703862be2..b835d81bb471 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -118,6 +118,9 @@ struct dw_hdmi_phy_data { const char *name; unsigned int gen; bool has_svsret;

• int (*configure)(struct dw_hdmi *hdmi,

• 	 const struct dw_hdmi_plat_data *pdata,
  

• 	 unsigned long mpixelclock);
  

};

struct dw_hdmi { @@ -860,8 +863,8 @@ static bool hdmi_phy_wait_i2c_done(struct dw_hdmi *hdmi, int msec) return true; }

-static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,

• 		 unsigned char addr)
  

+void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,

• 	   unsigned char addr)
  

{ hdmi_writeb(hdmi, 0xFF, HDMI_IH_I2CMPHY_STAT0); hdmi_writeb(hdmi, addr, HDMI_PHY_I2CM_ADDRESS_ADDR); @@ -873,6 +876,7 @@ static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data, HDMI_PHY_I2CM_OPERATION_ADDR); hdmi_phy_wait_i2c_done(hdmi, 1000); } +EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_write);

static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable) { @@ -993,37 +997,67 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi) return 0; }

-static int hdmi_phy_configure(struct dw_hdmi *hdmi) +/*

• • PHY configuration function for the DWC HDMI 3D TX PHY. Based on the

available

• • information the DWC MHL PHY has the same register layout and is thus

also

• • supported by this function.
• */

+static int hdmi_phy_configure_dwc_hdmi_3d_tx(struct dw_hdmi *hdmi,

• const struct dw_hdmi_plat_data *pdata,
  

• unsigned long mpixelclock)
  

{

• const struct dw_hdmi_phy_data *phy = hdmi->phy.data;

• const struct dw_hdmi_plat_data *pdata = hdmi->plat_data; const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg; const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr; const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;

  /* PLL/MPLL Cfg - always match on final entry */ for (; mpll_config->mpixelclock != ~0UL; mpll_config++)

• if (hdmi->hdmi_data.video_mode.mpixelclock <=
  

•     mpll_config->mpixelclock)
  

• if (mpixelclock <= mpll_config->mpixelclock)
  break;
  
  

  for (; curr_ctrl->mpixelclock != ~0UL; curr_ctrl++)

• if (hdmi->hdmi_data.video_mode.mpixelclock <=
  

•     curr_ctrl->mpixelclock)
  

• if (mpixelclock <= curr_ctrl->mpixelclock)
  break;
  
  

  for (; phy_config->mpixelclock != ~0UL; phy_config++)

• if (hdmi->hdmi_data.video_mode.mpixelclock <=
  

•     phy_config->mpixelclock)
  

• if (mpixelclock <= phy_config->mpixelclock)
  break;
  
  

  if (mpll_config->mpixelclock == ~0UL || curr_ctrl->mpixelclock == ~0UL ||

•    phy_config->mpixelclock == ~0UL) {
  

• dev_err(hdmi->dev, "Pixel clock %d - unsupported by HDMI\n",
  

• 	hdmi->hdmi_data.video_mode.mpixelclock);
  

•    phy_config->mpixelclock == ~0UL)
  

  return -EINVAL;

• }

• dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[0].cpce,

• 	      HDMI_3D_TX_PHY_CPCE_CTRL);
  

• dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[0].gmp,

• 	      HDMI_3D_TX_PHY_GMPCTRL);
  

• dw_hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[0],

• 	      HDMI_3D_TX_PHY_CURRCTRL);
  

• dw_hdmi_phy_i2c_write(hdmi, 0, HDMI_3D_TX_PHY_PLLPHBYCTRL);
• dw_hdmi_phy_i2c_write(hdmi, HDMI_3D_TX_PHY_MSM_CTRL_CKO_SEL_FB_CLK,

• 	      HDMI_3D_TX_PHY_MSM_CTRL);
  

• dw_hdmi_phy_i2c_write(hdmi, phy_config->term, HDMI_3D_TX_PHY_TXTERM);
• dw_hdmi_phy_i2c_write(hdmi, phy_config->sym_ctr,

• 	      HDMI_3D_TX_PHY_CKSYMTXCTRL);
  

• dw_hdmi_phy_i2c_write(hdmi, phy_config->vlev_ctr,

• 	      HDMI_3D_TX_PHY_VLEVCTRL);
  

• /* Override and disable clock termination. */
• dw_hdmi_phy_i2c_write(hdmi, HDMI_3D_TX_PHY_CKCALCTRL_OVERRIDE,

• 	      HDMI_3D_TX_PHY_CKCALCTRL);
  

• return 0;

+}

+static int hdmi_phy_configure(struct dw_hdmi *hdmi) +{

• const struct dw_hdmi_phy_data *phy = hdmi->phy.data;

• const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;

• unsigned long mpixelclock = hdmi->hdmi_data.video_mode.mpixelclock;

• int ret;

  dw_hdmi_phy_power_off(hdmi);

@@ -1042,26 +1076,16 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi) HDMI_PHY_I2CM_SLAVE_ADDR);

hdmi_phy_test_clear(hdmi, 0);

• hdmi_phy_i2c_write(hdmi, mpll_config->res[0].cpce,

• 	   HDMI_3D_TX_PHY_CPCE_CTRL);
  

• hdmi_phy_i2c_write(hdmi, mpll_config->res[0].gmp,

• 	   HDMI_3D_TX_PHY_GMPCTRL);
  

• hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[0],

• 	   HDMI_3D_TX_PHY_CURRCTRL);
  

• hdmi_phy_i2c_write(hdmi, 0, HDMI_3D_TX_PHY_PLLPHBYCTRL);
• hdmi_phy_i2c_write(hdmi, HDMI_3D_TX_PHY_MSM_CTRL_CKO_SEL_FB_CLK,

• 	   HDMI_3D_TX_PHY_MSM_CTRL);
  

• hdmi_phy_i2c_write(hdmi, phy_config->term, HDMI_3D_TX_PHY_TXTERM);
• hdmi_phy_i2c_write(hdmi, phy_config->sym_ctr,

• 	   HDMI_3D_TX_PHY_CKSYMTXCTRL);
  

• hdmi_phy_i2c_write(hdmi, phy_config->vlev_ctr,

• 	   HDMI_3D_TX_PHY_VLEVCTRL);
  

• /* Override and disable clock termination. */
• hdmi_phy_i2c_write(hdmi, HDMI_3D_TX_PHY_CKCALCTRL_OVERRIDE,

• 	   HDMI_3D_TX_PHY_CKCALCTRL);
  

• /* Write to the PHY as configured by the platform */

• if (pdata->configure_phy)

• ret = pdata->configure_phy(hdmi, pdata, mpixelclock);
  

• else

• ret = phy->configure(hdmi, pdata, mpixelclock);
  

• if (ret) {

• dev_err(hdmi->dev, "PHY configuration failed (clock %lu)\n",
  

• 	mpixelclock);
  

• return ret;
  

• }

  return dw_hdmi_phy_power_on(hdmi);

} @@ -1895,24 +1919,31 @@ static const struct dw_hdmi_phy_data dw_hdmi_phys[] = {> .name = "DWC MHL PHY + HEAC PHY", .gen = 2, .has_svsret = true,

• .configure = hdmi_phy_configure_dwc_hdmi_3d_tx,
  

  }, { .type = DW_HDMI_PHY_DWC_MHL_PHY, .name = "DWC MHL PHY", .gen = 2, .has_svsret = true,

• .configure = hdmi_phy_configure_dwc_hdmi_3d_tx,
  

  }, { .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY_HEAC, .name = "DWC HDMI 3D TX PHY + HEAC PHY", .gen = 2,

• .configure = hdmi_phy_configure_dwc_hdmi_3d_tx,
  

  }, { .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY, .name = "DWC HDMI 3D TX PHY", .gen = 2,

• .configure = hdmi_phy_configure_dwc_hdmi_3d_tx,
  

  }, { .type = DW_HDMI_PHY_DWC_HDMI20_TX_PHY, .name = "DWC HDMI 2.0 TX PHY", .gen = 2, .has_svsret = true,
• }, {

• .type = DW_HDMI_PHY_VENDOR_PHY,
  

• .name = "Vendor PHY",
  

  }

};

@@ -1943,6 +1974,14 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) hdmi->phy.ops = &dw_hdmi_synopsys_phy_ops; hdmi->phy.name = dw_hdmi_phys[i].name; hdmi->phy.data = (void *)&dw_hdmi_phys[i];

• 	if (!dw_hdmi_phys[i].configure &&
  

• 	    !hdmi->plat_data->configure_phy) {
  

• 		dev_err(hdmi->dev, "%s requires platform
  

support\n",

• 			hdmi->phy.name);
  

• 		return -ENODEV;
  

• 	}
  

• return 0;
  

  } }

diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index 0f583ca7e66e..dd330259a3dc 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -78,6 +78,9 @@ struct dw_hdmi_plat_data { const struct dw_hdmi_mpll_config *mpll_cfg; const struct dw_hdmi_curr_ctrl *cur_ctr; const struct dw_hdmi_phy_config *phy_config;

• int (*configure_phy)(struct dw_hdmi *hdmi,

• 	     const struct dw_hdmi_plat_data *pdata,
  

• 	     unsigned long mpixelclock);
  

};

int dw_hdmi_probe(struct platform_device *pdev, @@ -91,4 +94,8 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate); void dw_hdmi_audio_enable(struct dw_hdmi *hdmi); void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);

+/* PHY configuration */ +void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,

• 	   unsigned char addr);
  

#endif /* __IMX_HDMI_H__ */

Hmm, this is kind of confusing. Why do you need a phy_ops and then a separate configure_phy function? Can't we just use phy_ops always? If its external phy then it would need to implement all phy_ops functions.

The phy_ops are indeed meant to support vendor PHYs. The .configure_phy() operation is meant to support Synopsys PHYs that don't comply with the HDMI TX MHL and 3D PHYs I2C register layout and thus need a different configure function, but can share the other operations with the HDMI TX MHL and 3D PHYs. Ideally I'd like to move that code to the dw-hdmi core, but at the moment I don't have enough information to decide whether the register layout corresponds to the standard Synopsys HDMI TX 2.0 PHY or if it has been modified by the vendor. Once we'll have a second SoC using the same PHY we should have more information to consolidate the code. Of course, if you can share the HDMI TX 2.0 PHY datasheet, I won't mind reworking the code now ;-)