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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B5C26C6FD1C for ; Sat, 25 Mar 2023 12:11:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: Message-ID:In-reply-to:References:Date:Subject:Cc:To:From:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=dMSQPbmEHW9GxxW+toFXuTqjPW8BWNKyO113ChTFsHs=; b=KwA2WkP9duIrkrZuLj77essc8l RWCkSxzpgZkYrD/N3paDAwFY29ZgnvWIFWbFXIHMp4Kga2Kg2MA5Rbv2zz18Mo+oZ94dQwl0lvxog Gr5LSpMVDaxtAy4aAeJ6dbW+mjWgVre0DTrBdk0by8F65rH8ZZ7WcRNSDr3Yqd6moHP9RBBXHwhKP 8aaAxlL+qf1hRojvqHizC8R8l/3CvZPFcl+/sP4iec79z+l9/FS6FsTnajquaHnFh8kxYnsvE6Wzo 1OxrPiin5AjU2NvSsQSrFDB0cFU9wN+fMvHa4A30lcOy/GVwsdzxER5tlK0gcwwzxXqSx4xy9q4mA 7hgZsBwg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pg2im-006fAK-08; Sat, 25 Mar 2023 12:10:08 +0000 Received: from mout-p-201.mailbox.org ([2001:67c:2050:0:465::201]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pg2ii-006f9L-0g for linux-arm-kernel@lists.infradead.org; Sat, 25 Mar 2023 12:10:06 +0000 Received: from smtp2.mailbox.org (smtp2.mailbox.org [IPv6:2001:67c:2050:b231:465::2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-201.mailbox.org (Postfix) with ESMTPS id 4PkHvP2qKjz9sW6; Sat, 25 Mar 2023 13:09:53 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oltmanns.dev; s=MBO0001; t=1679746193; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=QalUgYTR7L02Pm/gpKnsde2aCBCLEQnrH09k5/MdlqE=; b=fK8Dw61CyQMFkvLPY0C5GZzBqzfKwKLL9ub62cJXQZpF70SVtceBlF8/sJaEciLa8exyNp dzf43/oEBdYAsXWhujvF3E+HE/SMU812X2riTfovbQtzke/6Q9YE9zDfVIgAe2FRRsqBW+ VYpIsZEJAslBFZzwj9ERjXeKehSwZSTZMCU3EpfLE9j9xuQSOkiz//Si/hG1cGoCJH5wu1 fM/frIuk6Rntpszv+Zw2XoMyTkHiMWdF17/2wydEtOnfLwJKKsDAen+zgC/nl19AcbZR3G qrZn8jks90hm2z9b93b2CDvoJa5CYGqunGPr2HEWsEQu3cqqoCJZXsKgQ9P+qA== From: Frank Oltmanns To: Roman Beranek , Maxime Ripard Cc: Chen-Yu Tsai , David Airlie , Daniel Vetter , Jernej Skrabec , Samuel Holland , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG Date: Sat, 25 Mar 2023 12:40:04 +0100 References: <20230320161636.24411-1-romanberanek@icloud.com> In-reply-to: <20230320161636.24411-1-romanberanek@icloud.com> Message-ID: <87wn356ni4.fsf@oltmanns.dev> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Rspamd-Queue-Id: 4PkHvP2qKjz9sW6 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230325_051004_403420_30323D99 X-CRM114-Status: GOOD ( 24.84 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --=-=-= Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On 2023-03-20 at 17:16:36 +0100, Roman Beranek wr= ote: > In the case of DSI output, the value of SUN4I_TCON0_DCLK_DIV (4) does > not represent the actual dotclock divider, PLL_MIPI instead runs at > (bpp / lanes )-multiple [1] of the dotclock. [2] Setting 4 as dotclock > divder thus leads to reduced frame rate, specifically by 1/3 on 4-lane > panels, and by 2/3 on 2-lane panels respectively. > > As sun4i_dotclock driver stores its calculated divider directly in > the register, conditional handling of the DSI output scenario is needed. > Instead of reading the divider from SUN4I_TCON0_DCLK_REG, retrieve > the value from tcon->dclk_min_div. > > [1] bits per pixel / number of DSI lanes > [2] > > Signed-off-by: Roman Beranek > =E2=80=94 > drivers/gpu/drm/sun4i/sun4i_dotclock.c | 6 +++++- > drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 +++=E2=80=93 > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff =E2=80=93git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/= drm/sun4i/sun4i_dotclock.c > index 417ade3d2565..26fa99aff590 100644 > =E2=80=94 a/drivers/gpu/drm/sun4i/sun4i_dotclock.c > +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c > @@ -11,6 +11,7 @@ > > #include =E2=80=9Csun4i_tcon.h=E2=80=9D > #include =E2=80=9Csun4i_dotclock.h=E2=80=9D > +#include =E2=80=9Csun6i_mipi_dsi.h=E2=80=9D > > struct sun4i_dclk { > struct clk_hw hw; > @@ -56,6 +57,9 @@ static unsigned long sun4i_dclk_recalc_rate(struct clk_= hw *hw, > struct sun4i_dclk *dclk =3D hw_to_dclk(hw); > u32 val; > > + if (dclk->tcon->is_dsi) > + return parent_rate / dclk->tcon->dclk_min_div; > + > regmap_read(dclk->regmap, SUN4I_TCON0_DCLK_REG, &val); > > val >>=3D SUN4I_TCON0_DCLK_DIV_SHIFT; > @@ -116,7 +120,7 @@ static int sun4i_dclk_set_rate(struct clk_hw *hw, uns= igned long rate, > unsigned long parent_rate) > { > struct sun4i_dclk *dclk =3D hw_to_dclk(hw); > - u8 div =3D parent_rate / rate; > + u8 div =3D dclk->tcon->is_dsi ? SUN6I_DSI_TCON_DIV : parent_rate / rate; > > return regmap_update_bits(dclk->regmap, SUN4I_TCON0_DCLK_REG, > GENMASK(6, 0), div); > diff =E2=80=93git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/= sun4i/sun4i_tcon.c > index 523a6d787921..7f5d3c135058 100644 > =E2=80=94 a/drivers/gpu/drm/sun4i/sun4i_tcon.c > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > @@ -367,8 +367,9 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tco= n *tcon, > u32 block_space, start_delay; > u32 tcon_div; > > - tcon->dclk_min_div =3D SUN6I_DSI_TCON_DIV; > - tcon->dclk_max_div =3D SUN6I_DSI_TCON_DIV; > + tcon->is_dsi =3D true; > + tcon->dclk_min_div =3D bpp / lanes; > + tcon->dclk_max_div =3D bpp / lanes; Claiming to set the divider to a different value (bpp / lanes) than what we= =E2=80=99re actually using in the end (SUN6I_DSIO_TCON_DIV) is somehow bugg= ing me. I feel like the proposal that I submitted is more direct: Actually, I had the following third patch prepared that adjusted the dotclo= ck rate so that the required PLL rate is set. But again, this seems very in= direct, so that=E2=80=99s why I refrained from submitting it and I submitte= d the linked patch above instead. Anyway, here is the third proposal: =E2=80=94 a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encod= er *encoder) regulator_disable(dsi->regulator); } +static bool sun6i_dsi_encoder_mode_fixup( =E2=81=83 struct drm_encoder *encoder, =E2=81=83 const struct drm_display_mode *mode, =E2=81=83 struct drm_display_mode *adjusted_mode) +{ =E2=81=83 if (encoder->encoder_type =3D=3D DRM_MODE_ENCODER_DSI) { =E2=81=83 /* =E2=81=83 * For DSI the PLL rate has to respect the bits per pixel and =E2=81=83 * number of lanes. =E2=81=83 * =E2=81=83 * According to the BSP code: =E2=81=83 * PLL rate =3D DOTCLOCK * bpp / lanes =E2=81=83 * =E2=81=83 * Therefore, the clock has to be adjusted in order to set the =E2=81=83 * correct PLL rate when actually setting the clock. =E2=81=83 */ =E2=81=83 struct sun6i_dsi *dsi =3D encoder_to_sun6i_dsi(encoder); =E2=81=83 struct mipi_dsi_device *device =3D dsi->device; =E2=81=83 u8 bpp =3D mipi_dsi_pixel_format_to_bpp(device->format); =E2=81=83 u8 lanes =3D device->lanes; =E2=81=83=20 =E2=81=83 adjusted_mode->crtc_clock =3D mode->crtc_clock =E2=81=83 * bpp / (lanes * SUN6I_DSI_TCON_DIV); =E2=81=83 } =E2=81=83=20 =E2=81=83 return true; +} =E2=81=83 static int sun6i_dsi_get_modes(struct drm_connector *connector) { struct sun6i_dsi *dsi =3D connector_to_sun6i_dsi(connector); @@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_conne= ctor_funcs =3D { static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = =3D { .disable =3D sun6i_dsi_encoder_disable, .enable =3D sun6i_dsi_encoder_enable, =E2=81=83 .mode_fixup =3D sun6i_dsi_encoder_mode_fixup, }; static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi, Maxime, Roman, CC, what do you think? Can we achieve consensus? If I=E2=80= =99m not mistaken, all of the three proposal are a step in the right direct= ion, because they correct faulty behavior. Don=E2=80=99t you think? Thanks, Frank > > sun4i_tcon0_mode_set_common(tcon, mode); > > diff =E2=80=93git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/= sun4i/sun4i_tcon.h > index fa23aa23fe4a..d8150ba2f319 100644 > =E2=80=94 a/drivers/gpu/drm/sun4i/sun4i_tcon.h > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h > @@ -271,6 +271,7 @@ struct sun4i_tcon { > struct clk *dclk; > u8 dclk_max_div; > u8 dclk_min_div; > + bool is_dsi; > > /* Reset control */ > struct reset_control *lcd_rst; --=-=-= Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --=-=-=--