Re: [PATCH RFC 1/8] drm/bridge: dw-hdmi: Add SCDC and TMDS Scrambling support

[Neil Armstrong]

Hi Laurent,

On 23/11/2018 15:25, Laurent Pinchart wrote:
> Hi Neil,
> 
> Thank you for the patch.
> 
> On Friday, 23 November 2018 16:02:14 EET Neil Armstrong wrote:
>> Add support for SCDC Setup for TMDS Clock > 3.4GHz and enable TMDS
>> Scrambling when supported or mandatory.
>>
>> This patch also adds an helper to setup the control bit to support
>> the hight TMDS Bit Period/TMDS Clock-Period Ratio as required with
> 
> s/hight/high/ ?

Thanks for catching !

> 
>> TMDS Clock > 3.4GHz for HDMI2.0 3840x2160@60/50 modes.
> 
> Why do you need a helper for this, is there no way it could be handled 
> internally ?

I could, but all the platforms would also do it internally... seems better
to have common helper, no ? And it will be usable by the PHY models handler
in the dw-hdmi driver aswell.

> 
>> These changes were based on work done by Huicong Xu <xhc@xxxxxxxxxxxxxx>
>> and Nickey Yang <nickey.yang@xxxxxxxxxxxxxx> to support HDMI2.0 modes
>> on the Rockchip 4.4 BSP kernel at [1]
>>
>> [1] https://github.com/rockchip-linux/kernel/tree/release-4.4
>>
>> Cc: Nickey Yang <nickey.yang@xxxxxxxxxxxxxx>
>> Cc: Huicong Xu <xhc@xxxxxxxxxxxxxx>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 45 +++++++++++++++++++++--
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.h |  1 +
>>  include/drm/bridge/dw_hdmi.h              |  1 +
>>  3 files changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
>> 5971976284bf..523508af70b0 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -28,6 +28,7 @@
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_edid.h>
>>  #include <drm/drm_encoder_slave.h>
>> +#include <drm/drm_scdc_helper.h>
>>  #include <drm/bridge/dw_hdmi.h>
>>
>>  #include <uapi/linux/media-bus-format.h>
>> @@ -1015,6 +1016,20 @@ void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi,
>> unsigned short data, }
>>  EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_write);
>>
>> +void dw_hdmi_set_high_tmds_clock_ratio(struct dw_hdmi *hdmi)
>> +{
>> +	unsigned long mtmdsclock = hdmi->hdmi_data.video_mode.mpixelclock;
>> +
>> +	/* Control for TMDS Bit Period/TMDS Clock-Period Ratio */
>> +	if (hdmi->connector.display_info.hdmi.scdc.supported) {
>> +		if (mtmdsclock > 340000000)
> 
> You use the 3.4GHz frequency in a few places, how about creating a macro ?

Good idea.

> 
>> +			drm_scdc_set_high_tmds_clock_ratio(hdmi->ddc, 1);
>> +		else
>> +			drm_scdc_set_high_tmds_clock_ratio(hdmi->ddc, 0);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(dw_hdmi_set_high_tmds_clock_ratio);
>> +
>>  static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable)
>> {
>>  	hdmi_mask_writeb(hdmi, !enable, HDMI_PHY_CONF0,
>> @@ -1340,11 +1355,12 @@ static void hdmi_tx_hdcp_config(struct dw_hdmi
>> *hdmi)
>>
>>  static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode
>> *mode) {
>> +	bool is_hdmi2_sink = hdmi->connector.display_info.hdmi.scdc.supported;
>>  	struct hdmi_avi_infoframe frame;
>>  	u8 val;
>>
>>  	/* Initialise info frame from DRM mode */
>> -	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, false);
>> +	drm_hdmi_avi_infoframe_from_display_mode(&frame, mode, is_hdmi2_sink);
>>
>>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
>>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
>> @@ -1503,7 +1519,8 @@ static void
>> hdmi_config_vendor_specific_infoframe(struct dw_hdmi *hdmi, static void
>> hdmi_av_composer(struct dw_hdmi *hdmi,
>>  			     const struct drm_display_mode *mode)
>>  {
>> -	u8 inv_val;
>> +	u8 inv_val, bytes;
>> +	struct drm_hdmi_info *hdmi_info = &hdmi->connector.display_info.hdmi;
>>  	struct hdmi_vmode *vmode = &hdmi->hdmi_data.video_mode;
>>  	int hblank, vblank, h_de_hs, v_de_vs, hsync_len, vsync_len;
>>  	unsigned int vdisplay;
>> @@ -1513,7 +1530,9 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
>>  	dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock);
>>
>>  	/* Set up HDMI_FC_INVIDCONF */
>> -	inv_val = (hdmi->hdmi_data.hdcp_enable ?
>> +	inv_val = (hdmi->hdmi_data.hdcp_enable ||
>> +		   vmode->mpixelclock > 340000000 ||
>> +		   hdmi_info->scdc.scrambling.low_rates ?
>>  		HDMI_FC_INVIDCONF_HDCP_KEEPOUT_ACTIVE :
>>  		HDMI_FC_INVIDCONF_HDCP_KEEPOUT_INACTIVE);
>>
>> @@ -1562,6 +1581,26 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
>>  		vsync_len /= 2;
>>  	}
>>
>> +	/* Scrambling Control */
>> +	if (hdmi_info->scdc.supported) {
>> +		if (vmode->mpixelclock > 340000000 ||
>> +		    hdmi_info->scdc.scrambling.low_rates) {
>> +			drm_scdc_readb(&hdmi->i2c->adap, SCDC_SINK_VERSION,
>> +				       &bytes);
>> +			drm_scdc_writeb(&hdmi->i2c->adap, SCDC_SOURCE_VERSION,
>> +					bytes);
> 
> Shouldn't the source version be min(sink version, highest supported source 
> version) ?

How should the "highest supported source version" be defined ?

> 
>> +			drm_scdc_set_scrambling(&hdmi->i2c->adap, 1);
>> +			hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ,
>> +				    HDMI_MC_SWRSTZ);
>> +			hdmi_writeb(hdmi, 1, HDMI_FC_SCRAMBLER_CTRL);
>> +		} else {
>> +			hdmi_writeb(hdmi, 0, HDMI_FC_SCRAMBLER_CTRL);
>> +			hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ,
>> +				    HDMI_MC_SWRSTZ);
>> +			drm_scdc_set_scrambling(&hdmi->i2c->adap, 0);
>> +		}
>> +	}
>> +
>>  	/* Set up horizontal active pixel width */
>>  	hdmi_writeb(hdmi, mode->hdisplay >> 8, HDMI_FC_INHACTV1);
>>  	hdmi_writeb(hdmi, mode->hdisplay, HDMI_FC_INHACTV0);
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h index
>> 9d90eb9c46e5..3f3c616eba97 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
>> @@ -255,6 +255,7 @@
>>  #define HDMI_FC_MASK2                           0x10DA
>>  #define HDMI_FC_POL2                            0x10DB
>>  #define HDMI_FC_PRCONF                          0x10E0
>> +#define HDMI_FC_SCRAMBLER_CTRL                  0x10E1
>>
>>  #define HDMI_FC_GMD_STAT                        0x1100
>>  #define HDMI_FC_GMD_EN                          0x1101
>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>> index ccb5aa8468e0..d7cc5d094270 100644
>> --- a/include/drm/bridge/dw_hdmi.h
>> +++ b/include/drm/bridge/dw_hdmi.h
>> @@ -156,6 +156,7 @@ void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool
>> hpd, bool rx_sense); 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);
>> +void dw_hdmi_set_high_tmds_clock_ratio(struct dw_hdmi *hdmi);
>>
>>  /* PHY configuration */
>>  void dw_hdmi_phy_i2c_set_addr(struct dw_hdmi *hdmi, u8 address);
> 
> 

Thanks for the review,

Neil