Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752223AbcJJHj4 (ORCPT ); Mon, 10 Oct 2016 03:39:56 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:43003 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752126AbcJJHjz (ORCPT ); Mon, 10 Oct 2016 03:39:55 -0400 Subject: Re: [PATCH v3] drm: tilcdc: add a workaround for failed clk_set_rate() To: Bartosz Golaszewski , Tomi Valkeinen , David Airlie , Kevin Hilman , Michael Turquette , Sekhar Nori References: <1475167437-8920-1-git-send-email-bgolaszewski@baylibre.com> CC: linux-drm , LKML From: Jyri Sarha Message-ID: Date: Mon, 10 Oct 2016 10:39:26 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <1475167437-8920-1-git-send-email-bgolaszewski@baylibre.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4553 Lines: 130 On 09/29/16 19:43, Bartosz Golaszewski wrote: > Some architectures don't use the common clock framework and don't > implement all the clk interfaces for every clock. This is the case > for da850-lcdk where clk_set_rate() only works for PLL0 and PLL1. > > Trying to set the clock rate for the LCDC clock results in -EINVAL > being returned. > > As a workaround for that: if the call to clk_set_rate() fails, fall > back to adjusting the clock divider instead. Proper divider value is > calculated by dividing the current clock rate by the required pixel > clock rate in HZ. > > This code is based on a hack initially developed internally for > baylibre by Karl Beldan . > > Tested with a da850-lcdk with an LCD display connected over VGA. > > Signed-off-by: Bartosz Golaszewski Forgot to mention, but I've picked this up too, but I won't send a pull request for 4.9 anymore. BR, Jyri > --- > v1 -> v2: > - rebased on top of current drm-next > - removed unnecessary error messages > - removed an extra newline > - added a warning if the effective pixel clock rate differs much from > the requested rate > > v2 -> v3: > - removed WARN_ONCE() in favor of dev_warn() > - added the real and calculated clock rates to the warning message > - tweaked the variable names > - used a local variable to remove redundant 'crtc->mode.clock * 1000' > > Retested with > > modetest -M tilcdc -s 26:800x600@RG16 > modetest -M tilcdc -s 26:1024x768@RG16 > > using some additional work-in-progress changes on top of this patch. > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 57 ++++++++++++++++++++++++++++++++---- > 1 file changed, 51 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index 52ebe8f..87cfde9 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -320,23 +320,68 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc, > return true; > } > > +/* > + * Calculate the percentage difference between the requested pixel clock rate > + * and the effective rate resulting from calculating the clock divider value. > + */ > +static unsigned int tilcdc_pclk_diff(unsigned long rate, > + unsigned long real_rate) > +{ > + int r = rate / 100, rr = real_rate / 100; > + > + return (unsigned int)(abs(((rr - r) * 100) / r)); > +} > + > static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > struct tilcdc_drm_private *priv = dev->dev_private; > struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); > - const unsigned clkdiv = 2; /* using a fixed divider of 2 */ > + unsigned long clk_rate, real_rate, req_rate; > + unsigned int clkdiv; > int ret; > > + clkdiv = 2; /* first try using a standard divider of 2 */ > + > /* mode.clock is in KHz, set_rate wants parameter in Hz */ > - ret = clk_set_rate(priv->clk, crtc->mode.clock * 1000 * clkdiv); > + req_rate = crtc->mode.clock * 1000; > + > + ret = clk_set_rate(priv->clk, req_rate * clkdiv); > + clk_rate = clk_get_rate(priv->clk); > if (ret < 0) { > - dev_err(dev->dev, "failed to set display clock rate to: %d\n", > - crtc->mode.clock); > - return; > + /* > + * If we fail to set the clock rate (some architectures don't > + * use the common clock framework yet and may not implement > + * all the clk API calls for every clock), try the next best > + * thing: adjusting the clock divider, unless clk_get_rate() > + * failed as well. > + */ > + if (!clk_rate) { > + /* Nothing more we can do. Just bail out. */ > + dev_err(dev->dev, > + "failed to set the pixel clock - unable to read current lcdc clock rate\n"); > + return; > + } > + > + clkdiv = DIV_ROUND_CLOSEST(clk_rate, req_rate); > + > + /* > + * Emit a warning if the real clock rate resulting from the > + * calculated divider differs much from the requested rate. > + * > + * 5% is an arbitrary value - LCDs are usually quite tolerant > + * about pixel clock rates. > + */ > + real_rate = clkdiv * req_rate; > + > + if (tilcdc_pclk_diff(clk_rate, real_rate) > 5) { > + dev_warn(dev->dev, > + "effective pixel clock rate (%luHz) differs from the calculated rate (%luHz)\n", > + clk_rate, real_rate); > + } > } > > - tilcdc_crtc->lcd_fck_rate = clk_get_rate(priv->clk); > + tilcdc_crtc->lcd_fck_rate = clk_rate; > > DBG("lcd_clk=%u, mode clock=%d, div=%u", > tilcdc_crtc->lcd_fck_rate, crtc->mode.clock, clkdiv); >