From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4DA78C433E9 for ; Wed, 17 Mar 2021 08:20:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1579C64F92 for ; Wed, 17 Mar 2021 08:20:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229643AbhCQIT6 (ORCPT ); Wed, 17 Mar 2021 04:19:58 -0400 Received: from mail.kernel.org ([198.145.29.99]:39022 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229566AbhCQITu (ORCPT ); Wed, 17 Mar 2021 04:19:50 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 1B25864F96; Wed, 17 Mar 2021 08:19:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1615969190; bh=4yKu9VDlHkcv0zuSCsLQWbJ6QFg9jp9i/0uWyfylwF0=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=KKz2N9EceRi5DoHpwycCjdEkagsWwXbpblOET9ldxgBI1kJv/egj0J2PbQ/GO4goF OsQICnd303ZNSbTgG6mOqmzSiKSgNuKZsqjeAFir4Gy9gg4m6iE5EfzB02ZRn8xBo7 YKCljROt8l6RprEvFsNwaY0TI+ogEX1Suh31++Vddm/GJ6nR4/27uRZn/v3/jQ2uaZ WJNnQFetDYfIodE0o1SW8ikvv/hLruak/Fv0t32YKnHaELQLfBNA/pUTMevlHSy6QA hLGdel3myRrFQTu9GrdgN2LUJ78tPGPzs7HaihHTbUByantLUveZxOxbVpWKjetxeM hbfCzDSatt6vA== Subject: Re: [PATCH] drm/tilcdc: fix LCD pixel clock setting To: Dario Binacchi , linux-kernel@vger.kernel.org, Jyri Sarha Cc: Daniel Vetter , David Airlie , dri-devel@lists.freedesktop.org References: <20210314151342.23404-1-dariobin@libero.it> From: Tomi Valkeinen Message-ID: <7df3a270-1cc4-7a71-5e55-49a0dfb2c21f@kernel.org> Date: Wed, 17 Mar 2021 10:19:46 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210314151342.23404-1-dariobin@libero.it> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 14/03/2021 17:13, Dario Binacchi wrote: > As reported by TI spruh73x RM, the LCD pixel clock (LCD_PCLK) frequency > is obtained by dividing LCD_CLK, the LCD controller reference clock, > for CLKDIV: > > LCD_PCLK = LCD_CLK / CLKDIV > > where CLKDIV must be greater than 1. > > Therefore LCD_CLK must be set to 'req_rate * CLKDIV' instead of req_rate The above doesn't make sense, the code already sets LCD_CLK to 'req_rate * clkdiv', not req_rate. > and the real LCD_CLK rate must be compared with 'req_rate * CLKDIV' and > not with req_rate. This is true, the code looks at the wrong value. > Passing req_rate instead of 'req_rate * CLKDIV' to the tilcdc_pclk_diff > routine caused it to fail even if LCD_CLK was properly set. > > Signed-off-by: Dario Binacchi > > --- > > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index 30213708fc99..02f56c9a5da5 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -203,7 +203,7 @@ 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); > - unsigned long clk_rate, real_rate, req_rate; > + unsigned long clk_rate, real_rate, req_rate, clk_div_rate; > unsigned int clkdiv; > int ret; > > @@ -211,10 +211,11 @@ static void tilcdc_crtc_set_clk(struct drm_crtc *crtc) > > /* mode.clock is in KHz, set_rate wants parameter in Hz */ > req_rate = crtc->mode.clock * 1000; > - > - ret = clk_set_rate(priv->clk, req_rate * clkdiv); > + /* LCD clock divisor input rate */ > + clk_div_rate = req_rate * clkdiv; "clk_div_rate" sounds a bit odd to me. Why not lcd_fck_rate, as that's the name used later? Or lcd_clk_rate. Or maybe lcd_clk_req_rate... > + ret = clk_set_rate(priv->clk, clk_div_rate); > clk_rate = clk_get_rate(priv->clk); > - if (ret < 0 || tilcdc_pclk_diff(req_rate, clk_rate) > 5) { > + if (ret < 0 || tilcdc_pclk_diff(clk_div_rate, clk_rate) > 5) { > /* > * If we fail to set the clock rate (some architectures don't > * use the common clock framework yet and may not implement > I think this fix is fine, but looking at the current code, it's calling tilcdc_pclk_diff(), but doesn't actually provide pixel clocks to the function, but fclk. Tomi