Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i8771445c:Fastmail
Date:   Mon, 19 Jun 2023 18:36:19 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Frank Oltmanns <frank@oltmanns.dev>
Cc:     Andre Przywara <andre.przywara@arm.com>,
        Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Roman Beranek <me@crly.cz>,
        Samuel Holland <samuel@sholland.org>,
        Stephen Boyd <sboyd@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-sunxi@lists.linux.dev
Subject: Re: [PATCH 1/2] clk: sunxi-ng: nkm: consider alternative parent
 rates when finding rate
Message-ID: <mdpeq63lziegp2pyp7a5fj6o2psvmm4mgeo354diq2u2j267vd@l63zrttiuvkk>
References: <20230605190745.366882-1-frank@oltmanns.dev>
 <20230605190745.366882-2-frank@oltmanns.dev>
 <2bvcukogzhcdbfsrruylgw5fbezaqjpcojqaambfoiv5fc2upy@ffumn5sevbeb>
 <875y7y1f0d.fsf@oltmanns.dev>
 <sijbhopoxuw5wodyry3smg7tm4wzoc7k6xakt4qdvxqsok32mv@u7rh4dct5ya6>
 <87a5x41w5r.fsf@oltmanns.dev>
 <unoskbtcteluxj7g3xkwc7ngcmglvcbm5ah25m7huhqxwd4dj3@nmfxbedwyu54>
 <87wn07zmxp.fsf@oltmanns.dev>
 <wnxjbbjfcnrg7ogwkfwbnldvyqbpt23b6mnrieb2zffooaojts@sudsbfjbjt64>
 <d40e94c4-9dab-4511-bd48-1d9081f4262a@oltmanns.dev>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="g7ta4xd7nex3465f"
Content-Disposition: inline
In-Reply-To: <d40e94c4-9dab-4511-bd48-1d9081f4262a@oltmanns.dev>
Precedence: bulk


--g7ta4xd7nex3465f
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 15, 2023 at 06:04:53PM +0200, Frank Oltmanns wrote:
> 15.06.2023 16:47:33 Maxime Ripard <maxime@cerno.tech>:
> > On Tue, Jun 13, 2023 at 12:17:06PM +0200, Frank Oltmanns wrote:
> >> Hi Maxime,
> >>
> >> I'll now only respond to one aspect of your mail, because it's the
> >> foundation for the whole behaviour.
> >>
> >> On 2023-06-13 at 11:10:08 +0200, Maxime Ripard <maxime@cerno.tech> wro=
te:
> >> [...]
> >>>>>> =E2=80=A6
> >>>>>
> >>>>> It doesn't really matter though. The output of that function must be
> >>>>> stable and must return the same set of factors and parent rate for a
> >>>>> given target rate.
> >>>>>
> >>>>
> >>>> I'm not sure if we're talking about the same thing here. Of course t=
he
> >>>> set of factors and parent rate for a given target rate will be diffe=
rent
> >>>> depending on the fact if we can or cannot adjust the parent rate,
> >>>> agreed?
> >>>
> >>> Yes, but here you also have a different behaviour in clk_round_rate()
> >>> and in clk_set_rate(), which isn't ok.
> >>>
> >>> Basically, clk_set_rate() + clk_get_rate() must be equal to
> >>> clk_round_rate().
> >>>
> >>> If you change if you look for parents depending on whether you're bei=
ng
> >>> called in clk_round_rate() and clk_set_rate(), then you're breaking t=
hat
> >>> expectation.
> >>>
> >>>> Let me compare my implementation to ccu_mp.
> >>>>
> >>>> ccu_mp_round_rate either calls the function ccu_mp_find_best or
> >>>> ccu_mp_find_best_with_parent_adj, depending on CLK_SET_RATE_PARENT.
> >>>
> >>> Yes, and it's fine: the flag is per-clock, and the output is the same
> >>> depending on whether we're being called by clk_round_rate() and
> >>> clk_set_rate().
> >>>
> >>
> >> The output is really not the same.
> >>
> >> ccu_mp_set_rate() always calls ccu_mp_find_best(). It never (!) consid=
ers
> >> changing the parent, independent of any flags.
> >>
> >> ccu_mp_round_rate() is calling ccu_mp_find_best() OR
> >> ccu_mp_find_best_with_parent_adj() depending on the flag.
> >>
> >> If I understand you correctly, you consider that a bug.
> >
> > No, sorry, you're right.
> >
> > clk_set_rate will call round_rate first, which will (possibly) pick up a
> > new parent, and by the time set_rate is called our parent will have been
> > changed already so we will just call find_best again considering only
> > that parent.
>=20
> Ok, no worries. That was my understanding, so your previous statement sha=
ttered my worldview. ;) That's why I may have seemed a bit alarmed.
>=20
> >
> > The set of factors and dividers should remain the same there, but I
> > don't think that's a concern.
>=20
> Ack. The output is stable when called with the same rate.
>=20
> > That leaves us with the rounding stuff, and the overall function
> > arguments. I like the structure of ccu_mp better, is there a reason to
> > deviate from it?
>=20
> I'm still pondering the rounding stuff. I'm just not sure why you are
> so relaxed about the fact that when calling round_rate with 449064000
> we get 449035712, but when we call get round_rate with 449035712 we
> get 449018181, and when we call round_rate with 449018181, we get
> 449018180.

I guess there's a couple of reasons:

  - You mentioned that you were going to fix the rate issue later :)

  - At the end of the day, it's not a huge offset and shouldn't cause
    any big trouble

> But ultimately, you have the final word, of course. But I need some
> time to be sure, that this does not become a problem in some cases.

Don't get me wrong, it should be fixed (and ideally, we should get some
unit tests to make sure that it doesn't behave that way). I don't think
it's urgent though, or that we introduce workarounds in one particular
clock type.

So we can definitely focus on the parent stuff first, and then get the
rate stuff figured out.

Maxime

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

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

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCZJCEAwAKCRDj7w1vZxhR
xTLqAQClbbvPDJ2cdhcYv+Jv7TNenlGRGGQLG7tFj1lyWz4roQEA7AYDdNJQZ9t0
AlA5tZURWiLpemHC6+21q8YDv3Bp1g8=
=Zcgb
-----END PGP SIGNATURE-----

--g7ta4xd7nex3465f--