From: Liu Ying <Ying.Liu@freescale.com>
To: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>,
<dri-devel@lists.freedesktop.org>, <stefan.wahren@i2se.com>,
<devicetree@vger.kernel.org>, <kernel@pengutronix.de>,
<sboyd@codeaurora.org>, <linux-kernel@vger.kernel.org>,
<a.hajda@samsung.com>, <andy.yan@rock-chips.com>,
<mturquette@linaro.org>, <linux-arm-kernel@lists.infradead.org>,
Tomi Valkeinen <tomi.valkeinen@ti.com>
Subject: Re: [PATCH RFC v9 01/20] clk: divider: Correct parent clk round rate if no bestdiv is normally found
Date: Thu, 12 Feb 2015 22:06:27 +0800 [thread overview]
Message-ID: <20150212140625.GA32487@victor> (raw)
In-Reply-To: <20150212134131.GX12209@pengutronix.de>
On Thu, Feb 12, 2015 at 02:41:31PM +0100, Sascha Hauer wrote:
> On Thu, Feb 12, 2015 at 12:56:46PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Feb 12, 2015 at 01:24:05PM +0100, Sascha Hauer wrote:
> > > On Thu, Feb 12, 2015 at 06:39:45PM +0800, Liu Ying wrote:
> > > > On Thu, Feb 12, 2015 at 10:33:56AM +0100, Sascha Hauer wrote:
> > > > > On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote:
> > > > > > If no best divider is normally found, we will try to use the maximum divider.
> > > > > > We should not set the parent clock rate to be 1Hz by force for being rounded.
> > > > > > Instead, we should take the maximum divider as a base and calculate a correct
> > > > > > parent clock rate for being rounded.
> > > > >
> > > > > Please add an explanation why you think the current code is wrong and
> > > > > what this actually fixes, maybe an example?
> > > >
> > > > The MIPI DSI panel's pixel clock rate is 26.4MHz and it's derived from PLL5 on
> > > > the MX6DL SabreSD board.
> > > >
> > > > These are the clock tree summaries with or without the patch applied:
> > > > 1) With the patch applied:
> > > > pll5_bypass_src 1 1 24000000 0 0
> > > > pll5 1 1 844800048 0 0
> > > > pll5_bypass 1 1 844800048 0 0
> > > > pll5_video 1 1 844800048 0 0
> > > > pll5_post_div 1 1 211200012 0 0
> > > > pll5_video_div 1 1 211200012 0 0
> > > > ipu1_di0_pre_sel 1 1 211200012 0 0
> > > > ipu1_di0_pre 1 1 26400002 0 0
> > > > ipu1_di0_sel 1 1 26400002 0 0
> > > > ipu1_di0 1 1 26400002 0 0
> > > >
> > > > 2) Without the patch applied:
> > > > pll5_bypass_src 1 1 24000000 0 0
> > > > pll5 1 1 648000000 0 0
> > > > pll5_bypass 1 1 648000000 0 0
> > > > pll5_video 1 1 648000000 0 0
> > > > pll5_post_div 1 1 162000000 0 0
> > > > pll5_video_div 1 1 40500000 0 0
> > > > ipu1_di0_pre_sel 1 1 40500000 0 0
> > > > ipu1_di0_pre 1 1 20250000 0 0
> > > > ipu1_di0_sel 1 1 20250000 0 0
> > > > ipu1_di0 1 1 20250000 0 0
> > >
> > > This seems to be broken since:
> > >
> > > | commit b11d282dbea27db1788893115dfca8a7856bf205
> > > | Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > > | Date: Thu Feb 13 12:03:59 2014 +0200
> > > |
> > > | clk: divider: fix rate calculation for fractional rates
> > >
> > > This patch fixed a case when clk_set_rate(clk_round_rate(rate)) resulted
> > > in a lower frequency than clk_round_rate(rate) returned.
> > >
> > > Since then the MULT_ROUND_UP in clk_divider_bestdiv() is inconsistent to
> > > the rest of the divider. Maybe this should be a simple rate * i now, but
> > > I'm unsure what side effects this has.
> > >
> > > I think your patch only fixes the behaviour in your case by accident,
> > > it's not a correct fix for this issue.
> >
> > Well, it's defined that:
> >
> > new_rate = clk_round_rate(clk, rate);
> >
> > returns the rate which you would get if you did:
> >
> > clk_set_rate(clk, rate);
> > new_rate = clk_get_rate(clk);
> >
> > The reasoning here is that clk_round_rate() gives you a way to query what
> > rate you would get if you were to ask for the rate to be set, without
> > effecting a change in the hardware.
> >
> > The idea that you should call clk_round_rate() first before clk_set_rate()
> > and pass the returned rounded rate into clk_set_rate() is really idiotic
> > given that. Please don't do it, and please remove code which does it, and
> > in review comment on it. Thanks.
>
> Tomis patch is based on the assumption that clk_set_rate(clk_round_rate(rate))
> is equal to clk_round_rate(rate). So when this assumption is wrong then
> it should simply be reverted.
> So Liu, could you test if reverting Tomis patch fixes your problem?
Yes, I'll test tomorrow when I have access to my board.
Regards,
Liu Ying
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
next prev parent reply other threads:[~2015-02-12 14:01 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-12 6:01 [PATCH RFC v9 00/20] Add support for i.MX MIPI DSI DRM driver Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 01/20] clk: divider: Correct parent clk round rate if no bestdiv is normally found Liu Ying
2015-02-12 9:33 ` Sascha Hauer
2015-02-12 10:39 ` Liu Ying
2015-02-12 12:24 ` Sascha Hauer
2015-02-12 12:56 ` Russell King - ARM Linux
2015-02-12 13:41 ` Sascha Hauer
2015-02-12 14:06 ` Liu Ying [this message]
2015-02-13 2:58 ` Liu Ying
2015-02-13 2:58 ` Travis
2015-02-13 14:35 ` Tomi Valkeinen
2015-02-13 18:57 ` Sascha Hauer
2015-02-16 11:18 ` Tomi Valkeinen
2015-02-17 10:32 ` Sascha Hauer
2015-02-16 11:27 ` Russell King - ARM Linux
2015-02-20 19:13 ` Mike Turquette
2015-02-20 19:20 ` Russell King - ARM Linux
2015-02-21 8:56 ` Uwe Kleine-König
2015-02-21 10:40 ` [PATCH 0/3] clk: divider: three exactness fixes (and a rant) Uwe Kleine-König
2015-02-21 10:40 ` [PATCH 1/3] clk: divider: fix calculation of maximal parent rate for a given divider Uwe Kleine-König
2015-02-23 7:32 ` Sascha Hauer
2015-03-05 8:35 ` Uwe Kleine-König
2015-02-21 10:40 ` [PATCH 2/3] clk: divider: fix selection of divider when rounding to closest Uwe Kleine-König
2015-02-23 9:46 ` Maxime Coquelin
2015-02-21 10:40 ` [PATCH 3/3] clk: divider: fix calculation of initial best " Uwe Kleine-König
2015-02-23 9:42 ` Maxime Coquelin
2015-02-23 7:23 ` [PATCH 0/3] clk: divider: three exactness fixes (and a rant) Sascha Hauer
2015-03-06 18:57 ` Mike Turquette
2015-03-06 19:28 ` Uwe Kleine-König
2015-03-06 19:40 ` Stephen Boyd
2015-03-09 9:58 ` Philipp Zabel
2015-03-09 19:05 ` Stephen Boyd
2015-03-09 20:23 ` Uwe Kleine-König
2015-03-09 21:07 ` Mike Turquette
2015-03-09 21:58 ` Uwe Kleine-König
2015-03-09 22:40 ` Stephen Boyd
2015-03-09 23:34 ` Uwe Kleine-König
2015-03-12 1:21 ` Stephen Boyd
2015-03-12 8:57 ` Philipp Zabel
2015-03-13 7:50 ` Uwe Kleine-König
2015-03-13 8:13 ` Philipp Zabel
2015-03-06 19:44 ` Stephen Boyd
2015-03-06 21:09 ` Uwe Kleine-König
2015-02-12 6:01 ` [PATCH RFC v9 02/20] ARM: imx6q: Add GPR3 MIPI muxing control register field shift bits definition Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 03/20] ARM: imx6q: clk: Add the video_27m clock Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 04/20] ARM: imx6q: clk: Change hdmi_isfr clock's parent to be " Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 05/20] ARM: imx6q: clk: Change hsi_tx clock to be a shared clock gate Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 06/20] ARM: imx6q: clk: Add support for mipi_core_cfg clock as " Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 07/20] ARM: imx6q: clk: Add support for mipi_ipg " Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 08/20] ARM: dts: imx6qdl: Move existing MIPI DSI ports into a new 'ports' node Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 09/20] drm/dsi: Add a helper to get bits per pixel of MIPI DSI pixel format Liu Ying
2015-02-12 9:26 ` Daniel Vetter
2015-02-13 5:01 ` Liu Ying
2015-03-03 11:07 ` Philipp Zabel
2015-04-03 3:28 ` Liu Ying
2015-04-09 7:10 ` Thierry Reding
2015-02-12 6:01 ` [PATCH RFC v9 10/20] Documentation: dt-bindings: Add bindings for Synopsys DW MIPI DSI DRM bridge driver Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 11/20] drm/bridge: Add Synopsys DesignWare MIPI DSI host controller driver Liu Ying
2015-04-09 8:43 ` Thierry Reding
2015-04-16 5:39 ` Archit Taneja
2015-04-22 12:13 ` Heiko Stübner
2015-02-12 6:01 ` [PATCH RFC v9 12/20] Documentation: dt-bindings: Add bindings for i.MX specific Synopsys DW MIPI DSI driver Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 13/20] drm: imx: Support Synopsys DesignWare MIPI DSI host controller Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 14/20] Documentation: dt-bindings: Add bindings for Himax HX8369A DRM panel driver Liu Ying
2015-04-09 7:20 ` Thierry Reding
2015-02-12 6:01 ` [PATCH RFC v9 15/20] drm: panel: Add support for Himax HX8369A MIPI DSI panel Liu Ying
2015-04-09 8:09 ` Thierry Reding
2015-02-12 6:01 ` [PATCH RFC v9 16/20] ARM: dtsi: imx6qdl: Add support for MIPI DSI host controller Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 17/20] ARM: dts: imx6qdl-sabresd: Add support for TRULY TFT480800-16-E MIPI DSI panel Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 18/20] ARM: imx_v6_v7_defconfig: Cleanup for imx drm being moved out of staging Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 19/20] ARM: imx_v6_v7_defconfig: Add support for MIPI DSI host controller Liu Ying
2015-02-12 6:01 ` [PATCH RFC v9 20/20] ARM: imx_v6_v7_defconfig: Add support for Himax HX8369A panel Liu Ying
2015-03-02 13:24 ` [PATCH RFC v9 00/20] Add support for i.MX MIPI DSI DRM driver Shawn Guo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150212140625.GA32487@victor \
--to=ying.liu@freescale.com \
--cc=a.hajda@samsung.com \
--cc=andy.yan@rock-chips.com \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=mturquette@linaro.org \
--cc=s.hauer@pengutronix.de \
--cc=sboyd@codeaurora.org \
--cc=stefan.wahren@i2se.com \
--cc=tomi.valkeinen@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).