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

Commit Message

Laurent Pinchart March 1, 2017, 10:39 p.m. UTC

From: Kieran Bingham <[e-mail]>

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 <[e-mail]>
Signed-off-by: Laurent Pinchart <[e-mail]>
---
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(-)

Comments

Jose Abreu March 2, 2017, 12:50 p.m. UTC | #1

Hi Laurent,


On 01-03-2017 22:39, Laurent Pinchart wrote:
> From: Kieran Bingham <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Laurent Pinchart <[e-mail]>
> ---
> 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.

Best regards,
Jose Miguel Abreu

Laurent Pinchart March 2, 2017, 1:41 p.m. UTC | #2

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Signed-off-by: Laurent Pinchart
> > <[e-mail]>
> > ---
> > 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 ;-)

Jose Abreu March 2, 2017, 2:50 p.m. UTC | #3

Hi Laurent,


On 02-03-2017 13:41, Laurent Pinchart wrote:
>
>> 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 ;-)
>

Well, if I want to keep my job I can't share the datasheet, and I
do want to keep my job :)

As far as I know this shouldn't change much. I already used (it
was like a year ago) the dw-hdmi driver in a HDMI TX 2.0 PHY. But
I am not following your logic here, sorry: You are mentioning a
custom phy, right? If its custom then it should implement its own
phy_ops. And I don't think that phy logic should go into core,
this should all be extracted because I really believe it will
make the code easier to read. Imagine this organization:

    Folders/Files:
        drm/bridge/dw-hdmi/dw-hdmi-core.c
        drm/bridge/dw-hdmi/dw-hdmi-phy-synopsys.c
        drm/bridge/dw-hdmi/dw-hdmi-phy-*renesas*.c
        drm/bridge/dw-hdmi/dw-hdmi-phy-something.c
    Structures:
        dw_hdmi_pdata
        dw_hdmi_phy_pdata
        dw_hdmi_phy_ops

As the phy is interacted using controller we would need to pass
some callbacks to the phy, but ultimately the phy would be a
platform driver which could have its own compatible string that
would be associated with controller (not sure exactly about this
because I only used this in non-dt).

This is just an idea though. I followed this logic in the RX side
and its very easy to add a new phy now, its a matter of creating
a new file, implement ops and associate it with controller. Of
course some phys will be very similar, for that we can use minor
tweaks via id detection.

What do you think?

Best regards,
Jose Miguel Abreu

Laurent Pinchart March 2, 2017, 3:38 p.m. UTC | #4

Hi Jose,

On Thursday 02 Mar 2017 14:50:02 Jose Abreu wrote:
> On 02-03-2017 13:41, Laurent Pinchart wrote:
> >> 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 ;-)
>
> Well, if I want to keep my job I can't share the datasheet, and I
> do want to keep my job :)

That's so selfish :-D

> As far as I know this shouldn't change much. I already used (it
> was like a year ago) the dw-hdmi driver in a HDMI TX 2.0 PHY.

I really wonder what exactly has been integrated in the Renesas R-Car Gen3 
SoC. The PHY is certainly reported as an HDMI TX 2.0 PHY, but the registers 
seem different compared to the 2.0 PHY you've tested.

> But I am not following your logic here, sorry: You are mentioning a
> custom phy, right?

Custom is probably a bad name. From what I've been told it's a standard 
Synopsys PHY, but I can't rule out vendor-specific customizations.

> If its custom then it should implement its own phy_ops. And I don't think
> that phy logic should go into core, this should all be extracted because I
> really believe it will make the code easier to read. Imagine this
> organization:
> 
>     Folders/Files:
>         drm/bridge/dw-hdmi/dw-hdmi-core.c
>         drm/bridge/dw-hdmi/dw-hdmi-phy-synopsys.c
>         drm/bridge/dw-hdmi/dw-hdmi-phy-*renesas*.c
>         drm/bridge/dw-hdmi/dw-hdmi-phy-something.c
>     Structures:
>         dw_hdmi_pdata
>         dw_hdmi_phy_pdata
>         dw_hdmi_phy_ops

That looks good to me.

> As the phy is interacted using controller we would need to pass
> some callbacks to the phy, but ultimately the phy would be a
> platform driver which could have its own compatible string that
> would be associated with controller (not sure exactly about this
> because I only used this in non-dt).

We already have glue code, having to provide separate glue and PHY drivers 
seems a bit overkill to me. If we could get rid of glue code by adding a node 
for the PHY in DT that would be nice, but if we have to keep the glue then we 
can as well use platform data there.

> This is just an idea though. I followed this logic in the RX side
> and its very easy to add a new phy now, its a matter of creating
> a new file, implement ops and associate it with controller. Of
> course some phys will be very similar, for that we can use minor
> tweaks via id detection.
> 
> What do you think?

It sounds neat overall, but I wonder whether it should really block this 
series, or be added on top of it :-) I think we're already moving in the right 
direction here.

Jose Abreu March 3, 2017, 3:57 p.m. UTC | #5

Hi Laurent,


On 02-03-2017 15:38, Laurent Pinchart wrote:
> Hi Jose,
>
> On Thursday 02 Mar 2017 14:50:02 Jose Abreu wrote:
>> On 02-03-2017 13:41, Laurent Pinchart wrote:
>>>> 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 ;-)
>> Well, if I want to keep my job I can't share the datasheet, and I
>> do want to keep my job :)
> That's so selfish :-D
>
>> As far as I know this shouldn't change much. I already used (it
>> was like a year ago) the dw-hdmi driver in a HDMI TX 2.0 PHY.
> I really wonder what exactly has been integrated in the Renesas R-Car Gen3 
> SoC. The PHY is certainly reported as an HDMI TX 2.0 PHY, but the registers 
> seem different compared to the 2.0 PHY you've tested.
>
>> But I am not following your logic here, sorry: You are mentioning a
>> custom phy, right?
> Custom is probably a bad name. From what I've been told it's a standard 
> Synopsys PHY, but I can't rule out vendor-specific customizations.
>
>> If its custom then it should implement its own phy_ops. And I don't think
>> that phy logic should go into core, this should all be extracted because I
>> really believe it will make the code easier to read. Imagine this
>> organization:
>>
>>     Folders/Files:
>>         drm/bridge/dw-hdmi/dw-hdmi-core.c
>>         drm/bridge/dw-hdmi/dw-hdmi-phy-synopsys.c
>>         drm/bridge/dw-hdmi/dw-hdmi-phy-*renesas*.c
>>         drm/bridge/dw-hdmi/dw-hdmi-phy-something.c
>>     Structures:
>>         dw_hdmi_pdata
>>         dw_hdmi_phy_pdata
>>         dw_hdmi_phy_ops
> That looks good to me.
>
>> As the phy is interacted using controller we would need to pass
>> some callbacks to the phy, but ultimately the phy would be a
>> platform driver which could have its own compatible string that
>> would be associated with controller (not sure exactly about this
>> because I only used this in non-dt).
> We already have glue code, having to provide separate glue and PHY drivers 
> seems a bit overkill to me. If we could get rid of glue code by adding a node 
> for the PHY in DT that would be nice, but if we have to keep the glue then we 
> can as well use platform data there.
>
>> This is just an idea though. I followed this logic in the RX side
>> and its very easy to add a new phy now, its a matter of creating
>> a new file, implement ops and associate it with controller. Of
>> course some phys will be very similar, for that we can use minor
>> tweaks via id detection.
>>
>> What do you think?
> It sounds neat overall, but I wonder whether it should really block this 
> series, or be added on top of it :-) I think we're already moving in the right 
> direction here.
>

This series is definitely a good starting point and a good
improvement. We can think later about abstracting even more the
phy from the controller. I think this will be a major step and
will reflect better how the HW is modeled.

You can add my reviewed-by in all the patches in this series in
your next submission (or in the merge).

Also, if you do a next submission what do you think about moving
all the dw-hdmi files to a new folder called for example
'synopsys' or 'dw-hdmi'? Because we already have 5 files. I think
'synopsys' instead of 'dw-hdmi' would be better because in the
future we may add support for other protocols (for example
display port).

Side note: I think you missed in the cc Archit Taneja

Best regards,
Jose Miguel Abreu

Laurent Pinchart March 3, 2017, 4:56 p.m. UTC | #6

Hi Jose,

(CC'ing Archit)

On Friday 03 Mar 2017 15:57:57 Jose Abreu wrote:
> On 02-03-2017 15:38, Laurent Pinchart wrote:
> > On Thursday 02 Mar 2017 14:50:02 Jose Abreu wrote:
> >> On 02-03-2017 13:41, Laurent Pinchart wrote:
> >>>> 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 ;-)
> >> 
> >> Well, if I want to keep my job I can't share the datasheet, and I
> >> do want to keep my job :)
> > 
> > That's so selfish :-D
> > 
> >> As far as I know this shouldn't change much. I already used (it
> >> was like a year ago) the dw-hdmi driver in a HDMI TX 2.0 PHY.
> > 
> > I really wonder what exactly has been integrated in the Renesas R-Car Gen3
> > SoC. The PHY is certainly reported as an HDMI TX 2.0 PHY, but the
> > registers seem different compared to the 2.0 PHY you've tested.
> > 
> >> But I am not following your logic here, sorry: You are mentioning a
> >> custom phy, right?
> > 
> > Custom is probably a bad name. From what I've been told it's a standard
> > Synopsys PHY, but I can't rule out vendor-specific customizations.
> > 
> >> If its custom then it should implement its own phy_ops. And I don't think
> >> that phy logic should go into core, this should all be extracted because
> >> I really believe it will make the code easier to read. Imagine this
> >> 
> >> organization:
> >>     Folders/Files:
> >>         drm/bridge/dw-hdmi/dw-hdmi-core.c
> >>         drm/bridge/dw-hdmi/dw-hdmi-phy-synopsys.c
> >>         drm/bridge/dw-hdmi/dw-hdmi-phy-*renesas*.c
> >>         drm/bridge/dw-hdmi/dw-hdmi-phy-something.c
> >>     
> >>     Structures:
> >>         dw_hdmi_pdata
> >>         dw_hdmi_phy_pdata
> >>         dw_hdmi_phy_ops
> > 
> > That looks good to me.
> > 
> >> As the phy is interacted using controller we would need to pass
> >> some callbacks to the phy, but ultimately the phy would be a
> >> platform driver which could have its own compatible string that
> >> would be associated with controller (not sure exactly about this
> >> because I only used this in non-dt).
> > 
> > We already have glue code, having to provide separate glue and PHY drivers
> > seems a bit overkill to me. If we could get rid of glue code by adding a
> > node for the PHY in DT that would be nice, but if we have to keep the
> > glue then we can as well use platform data there.
> > 
> >> This is just an idea though. I followed this logic in the RX side
> >> and its very easy to add a new phy now, its a matter of creating
> >> a new file, implement ops and associate it with controller. Of
> >> course some phys will be very similar, for that we can use minor
> >> tweaks via id detection.
> >> 
> >> What do you think?
> > 
> > It sounds neat overall, but I wonder whether it should really block this
> > series, or be added on top of it :-) I think we're already moving in the
> > right direction here.
> 
> This series is definitely a good starting point and a good
> improvement. We can think later about abstracting even more the
> phy from the controller. I think this will be a major step and
> will reflect better how the HW is modeled.
> 
> You can add my reviewed-by in all the patches in this series in
> your next submission (or in the merge).

Thank you !

> Also, if you do a next submission what do you think about moving
> all the dw-hdmi files to a new folder called for example
> 'synopsys' or 'dw-hdmi'? Because we already have 5 files. I think
> 'synopsys' instead of 'dw-hdmi' would be better because in the
> future we may add support for other protocols (for example
> display port).

I've posted a patch for that.

> Side note: I think you missed in the cc Archit Taneja

Oops, indeed :-/ Fixed on this patch, and I'll make sure to CC him on v5.

Patch

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__ */