From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752424AbdJ0PRJ (ORCPT ); Fri, 27 Oct 2017 11:17:09 -0400 Received: from mx07-00178001.pphosted.com ([62.209.51.94]:44479 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751799AbdJ0PRG (ORCPT ); Fri, 27 Oct 2017 11:17:06 -0400 From: Philippe CORNU To: Philipp Zabel , Archit Taneja , Andrzej Hajda , "Laurent Pinchart" , David Airlie , Benjamin Gaignard , "Bhumika Goyal" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" CC: Yannick FERTRE , Vincent ABRIOU , Alexandre TORGUE , "Maxime Coquelin" , Gabriel FERNANDEZ , Ludovic BARRE , "Fabien DESSENNE" , Mickael REULIER Subject: Re: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock Thread-Topic: [PATCH] drm/bridge/synopsys: dsi: add optional pixel clock Thread-Index: AQHTTvqBZidsu0J1UkaY+a46cKfqn6L3rbYA Date: Fri, 27 Oct 2017 15:16:44 +0000 Message-ID: <12d812f5-6c46-41e9-e0d4-44be96f47428@st.com> References: <1509034175-23767-1-git-send-email-philippe.cornu@st.com> <1509091595.7559.2.camel@pengutronix.de> In-Reply-To: <1509091595.7559.2.camel@pengutronix.de> Accept-Language: fr-FR, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.49] Content-Type: text/plain; charset="utf-8" Content-ID: <842B2A475369AE41AD989C8A725F4A02@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-27_08:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v9RFHFdx008999 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 >> --- >> 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 >