Received-SPF: pass (google.com: domain of linux-kernel+bounces-98628-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 11 Mar 2024 10:34:37 +0100
From: Maxime Ripard <mripard@kernel.org>
To: Yang Xiwen <forbidden405@outlook.com>
Cc: Michael Turquette <mturquette@baylibre.com>, 
	Stephen Boyd <sboyd@kernel.org>, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] clk: set initial best mux parent to current parent
 with CLK_MUX_ROUND_CLOSEST
Message-ID: <20240311-speedy-bat-of-art-4facfc@houat>
References: <20240307-mux-v3-1-0885fc1ab2c9@outlook.com>
 <20240307-hot-hummingbird-of-atheism-87503c@houat>
 <SEZPR06MB6959882F5DA673456A3AA85D96202@SEZPR06MB6959.apcprd06.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="bftwjk7fcovlmr3o"
Content-Disposition: inline
In-Reply-To: <SEZPR06MB6959882F5DA673456A3AA85D96202@SEZPR06MB6959.apcprd06.prod.outlook.com>


--bftwjk7fcovlmr3o
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 07, 2024 at 07:18:05PM +0800, Yang Xiwen wrote:
> On 3/7/2024 4:48 PM, Maxime Ripard wrote:
> > Hi,
> >=20
> > On Thu, Mar 07, 2024 at 10:03:50AM +0800, Yang Xiwen via B4 Relay wrote:
> > > From: Yang Xiwen <forbidden405@outlook.com>
> > >=20
> > > Originally, the initial clock rate is hardcoded to 0, this can lead to
> > > some problem when setting a very small rate with CLK_MUX_ROUND_CLOSES=
T.
> > >=20
> > > For example, if the lowest possible rate provided by the mux is 1000H=
z,
> > > setting a rate below 500Hz will fail, because no clock can provide a
> > > better rate than the non-existant 0Hz. But it should succeed with 100=
0Hz
> > > being set.
> > >=20
> > > Setting the initial best parent to current parent could solve this bu=
g.
> > >=20
> > > Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> > I don't think it would be the way to go. The biggest issue to me is that
> > it's inconsistent, and only changing the behaviour for a given flag
> > doesn't solve that.
>=20
>=20
> I think the current behavior is odd but conforms to the document if
> CLK_MUX_ROUND_CLOSEST is not specified.

clk_mux_determine_rate_flags isn't documented, and the determine_rate
clk_ops documentation doesn't mention it can return an error.

> If i understand correctly, the default behavior of mux clocks is to
> select the closest rate lower than requested rate, and
> CLK_MUX_ROUND_CLOSEST removes the "lower than" limitation, which is
> what this version tries to accomplish.

The situation is not as clear-cut as you make it to be, unfortunately.
The determine_rate clk_ops implementation states:

  Given a target rate as input, returns the closest rate actually
  supported by the clock, and optionally the parent clock that should be
  used to provide the clock rate.

So CLK_MUX_ROUND_CLOSEST shouldn't exist, because that's what
determine_rate expects so it should always be there.

Now, the "actually supported by the clock" can be interpreted in
multiple ways, and most importantly, doesn't state what the behaviour is
if we can't find a rate actually supported by the clock.

But now, this situation has been ambiguous for a while and thus drivers
kind of relied on that ambiguity.

So the way to fix it up is:

  - Assess what drivers are relying on
  - Document the current behaviour in clk_ops determine_rate
  - Make clk_mux_determine_rate_flags consistent with that
  - Run that through kernelci to make sure we don't have any regression

Maxime

--bftwjk7fcovlmr3o
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCZe7QLAAKCRDj7w1vZxhR
xRtAAP4jnq0AU2eBH/EQxTHEF/Kw0cZq4fwDzKSVbh0+2DnA3QD/bl06o0eOOtCO
eXKHNHCmleWeqbn3XdXUpoe3JNQYFwc=
=Bdd2
-----END PGP SIGNATURE-----

--bftwjk7fcovlmr3o--