Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock

[Philippe CORNU <philippe.cornu@st.com>]

Hi Philipp,

On 10/27/2017 10:06 AM, Philipp Zabel wrote:
> Hi Philippe,
> 
> On Thu, 2017-10-26 at 18:09 +0200, Philippe Cornu wrote:
>> The pixel clock is optional. When available, it offers a better
>> preciseness for timing computations.
>>
>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> index d9cca4f..8b3787d 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>> @@ -225,6 +225,7 @@ struct dw_mipi_dsi {
>>   	void __iomem *base;
>>   
>>   	struct clk *pclk;
>> +	struct clk *px_clk;
>>   
>>   	unsigned int lane_mbps; /* per lane */
>>   	u32 channel;
>> @@ -753,24 +754,28 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge,
>>   	struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
>>   	const struct dw_mipi_dsi_phy_ops *phy_ops = dsi->plat_data->phy_ops;
>>   	void *priv_data = dsi->plat_data->priv_data;
>> +	struct drm_display_mode px_clk_mode = *mode;
>>   	int ret;
>>   
>>   	clk_prepare_enable(dsi->pclk);
>>   
>> -	ret = phy_ops->get_lane_mbps(priv_data, mode, dsi->mode_flags,
>> +	if (dsi->px_clk)
>> +		px_clk_mode.clock = clk_get_rate(dsi->px_clk) / 1000;
> 
> I don't understand why is the pixel clock rate divided by 1000 different
> from the mode clock in the first place? If px_clk is just the DPI input
> pixel clock signal from the display controller, I'd expect the mode to
> be adjusted correctly.
> 

The display panel requests a given px clk frequency. Then, the set rate 
is performed by the "crtc" (ie. the display controller). Depending of 
the hw platform, the real frequency is different from the requested one 
as pll work with dividers (and sometimes pll are shared between several 
IPs adding more constraints...) so preciseness is not "perfect".

This preciseness is "hw platform dependent" that is why this px clock is 
used here only if the platform populates the related dt.

>> +
>> +	ret = phy_ops->get_lane_mbps(priv_data, &px_clk_mode, dsi->mode_flags,
>>   				     dsi->lanes, dsi->format, &dsi->lane_mbps);
>>   	if (ret)
>>   		DRM_DEBUG_DRIVER("Phy get_lane_mbps() failed\n");
>>   
>>   	pm_runtime_get_sync(dsi->dev);
>>   	dw_mipi_dsi_init(dsi);
>> -	dw_mipi_dsi_dpi_config(dsi, mode);
>> +	dw_mipi_dsi_dpi_config(dsi, &px_clk_mode);
>>   	dw_mipi_dsi_packet_handler_config(dsi);
>>   	dw_mipi_dsi_video_mode_config(dsi);
>> -	dw_mipi_dsi_video_packet_config(dsi, mode);
>> +	dw_mipi_dsi_video_packet_config(dsi, &px_clk_mode);
>>   	dw_mipi_dsi_command_mode_config(dsi);
>> -	dw_mipi_dsi_line_timer_config(dsi, mode);
>> -	dw_mipi_dsi_vertical_timing_config(dsi, mode);
>> +	dw_mipi_dsi_line_timer_config(dsi, &px_clk_mode);
>> +	dw_mipi_dsi_vertical_timing_config(dsi, &px_clk_mode);
>>   
>>   	dw_mipi_dsi_dphy_init(dsi);
>>   	dw_mipi_dsi_dphy_timing_config(dsi);
>> @@ -784,7 +789,7 @@ void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge,
>>   
>>   	dw_mipi_dsi_dphy_enable(dsi);
>>   
>> -	dw_mipi_dsi_wait_for_two_frames(mode);
>> +	dw_mipi_dsi_wait_for_two_frames(&px_clk_mode);
>>   
>>   	/* Switch to cmd mode for panel-bridge pre_enable & panel prepare */
>>   	dw_mipi_dsi_set_mode(dsi, 0);
>> @@ -878,6 +883,13 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge)
>>   		return ERR_PTR(ret);
>>   	}
>>   
>> +	dsi->px_clk = devm_clk_get(dev, "px_clk");
> 
> Is "px_clk" what the pixel clock is called in the datasheet?
> 
> If this is probed from DT, the new optional property must be documented
> in the binding docs.
> 

The px clk (and "px_clk") are already documented in 
Documentation/devicetree/bindings/display/bridge/dw_mipi_dsi.txt as 
optional.

   - "px_clk" is the pixel clock for the DPI/RGB input. (optional)

Do you think bindings is clear enough?

>> +	if (IS_ERR(dsi->px_clk)) {
>> +		ret = PTR_ERR(dsi->px_clk);
> 
> Better leave ret == 0 for -ENOENT, but return the error for all others.
> 

Ok, I will do more test here.

Many thanks for your comments,
Philippe :-)

>> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
>> +		dsi->px_clk = NULL;
>> +	}
>> +
>>   	/*
>>   	 * Note that the reset was not defined in the initial device tree, so
>>   	 * we have to be prepared for it not being found.
> 
> regards
> Philipp
>