From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Mike Turquette <mturquette@linaro.org>
Cc: "Stephen Boyd" <sboyd@codeaurora.org>,
linux-arm-kernel@lists.infradead.org,
"Sören Brinkmann" <soren.brinkmann@xilinx.com>,
linux-kernel@vger.kernel.org, kernel@pengutronix.de
Subject: Re: [PATCH 0/3] clk: divider: three exactness fixes (and a rant)
Date: Fri, 6 Mar 2015 20:28:13 +0100 [thread overview]
Message-ID: <20150306192813.GG10717@pengutronix.de> (raw)
In-Reply-To: <20150306185730.11109.45998@quantum>
Hello Mike,
On Fri, Mar 06, 2015 at 10:57:30AM -0800, Mike Turquette wrote:
> Quoting Uwe Kleine-König (2015-02-21 02:40:22)
> > Hello,
> >
> > TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST.
> >
> > I stared at clk-divider.c for some time now given Sascha's failing test
> > case. I found a fix for the failure (which happens to be what Sascha
> > suspected).
> >
> > The other two patches fix problems only present when handling dividers
> > that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still
> > heavily broken however. So having a 4bit-divider and a parent clk of
> > 10000 (as in Sascha's test case) requesting
> >
> > clk_set_rate(clk, 666)
> >
> > sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the
> > choice of parent_rate in clk_divider_bestdiv's loop is wrong for
> > CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is
> > non-trivial and for sure more than one rate must be tested here. This is
> > complicated by the fact that clk_round_rate might return a value bigger
> > than the requested rate which convinces me (once more) that it's a bad
> > idea to allow that. Even if this was fixed for .round_rate,
> > clk_divider_set_rate is still broken because it also uses
> >
> > div = DIV_ROUND_UP(parent_rate, rate);
> >
> > to calculate the (pretended) best divider to get near rate.
> >
> > Note this makes at least two reasons to remove support for
> > CLK_DIVIDER_ROUND_CLOSEST!
> >
> > Instead I'd favour creating a function
> >
> > clk_round_rate_nearest
> >
> > as was suggested some time ago by Soren Brinkmann and me[1] that doesn't
>
> Uwe,
>
> Thanks for the fixes. I'm thinking of taking all three for 4.0. I also
> agree on clk_round_rate_nearest (along with a _ceil and _floor version
> as well). That's something we can do for 4.1 probably.
I'd say that we make round_rate the _floor version. I guess in most
cases that already does the right thing. Also I think 4.1 is very
ambitious, so my suggestion for 4.1 is:
- add a WARN_ON_ONCE to clk_round_rate catching calls that return a
value bigger than requested.
- implement clk_round_rate_nearest using clk_round_rate and the
assumption that it returns a value that is <= the requested rate.
I think without that there are too many special cases to handle and
probably not even a reliable way to determine the nearest rate.
- while we're at it tightening the requirements for clk_round_rate
let's also specify the expected rounding. I'd vote for the
mathematical rounding, that is
clk_round_rate(someclk, 333)
explicitly is allowed to return a rate bigger than 333 as long as it
is less than 333.5.
At one point while developing patch 1 I had the dividers fixed for the
rounding issue. I think I still have that patch somewhere so can post it
as RFC.
> There are currently 3 users of CLK_DIVIDER_ROUND_CLOSEST:
>
> Loongson
> QCOM
> ST
>
> So now is probably the right time to remove the flag if we're going to
> do it.
"now" is before we have clk_round_rate_nearest, right?
So what do you want to do with these three users? Move them to the
default implementation?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
next prev parent reply other threads:[~2015-03-06 19:28 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
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 [this message]
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=20150306192813.GG10717@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mturquette@linaro.org \
--cc=sboyd@codeaurora.org \
--cc=soren.brinkmann@xilinx.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).