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

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

Hi Andrzej,

On 10/27/2017 08:41 AM, Andrzej Hajda wrote:
> On 26.10.2017 18:09, 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;
>> +
>> +	ret = phy_ops->get_lane_mbps(priv_data, &px_clk_mode, dsi->mode_flags,
>>   				     dsi->lanes, dsi->format, &dsi->lane_mbps);
> 
> Just small suggestion: if pixel clock rate matters, maybe better is to
> fix it in adjusted_mode in mode_fixup callback.
> 

Well, it was my first idea but unfortunately, "_adjusted_" stuffs are 
not used by many drivers, and it means that other drivers using this 
generic dw dsi driver will have to handle the "_adjusted_" stuffs too in 
their own crtc or dsi specific parts...

So that is why I have chosen to use the "optional" pixel clock (as 
defined in the documentation): as it is optional, it is up to the 
"specific dt part" to use it or not, whatever the "crtc" driver (often 
handling the set rate, like in stm32 ltdc.c), avoiding then more patches 
in the specific parts and more importantly in the crtc parts...

What is your opinion then?

>>   	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");
>> +	if (IS_ERR(dsi->px_clk)) {
>> +		ret = PTR_ERR(dsi->px_clk);
>> +		dev_dbg(dev, "Unable to get optional px_clk: %d\n", ret);
>> +		dsi->px_clk = NULL;
>> +	}
>> +
> 
> devm_clk_get is called from bridge::attach callback, do we have
> guarantee that in ::detach callback the clock will be removed?
> And what if the clock is defined in dts, but it cannot be get due to
> other reasons? I guess the code should fail then, ie you should have
> different paths for -ENOENT and for other errors.
> 

I will do more tests related to your above comments,

Many thanks for your comments and support,
Philippe :-)

>   --
> Regards
> Andrzej
> 
>>   	/*
>>   	 * Note that the reset was not defined in the initial device tree, so
>>   	 * we have to be prepared for it not being found.
> 
>