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, 3 Jul 2023 13:36:36 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Frank Oltmanns <frank@oltmanns.dev>
Cc:     Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Samuel Holland <samuel@sholland.org>,
        Andre Przywara <andre.przywara@arm.com>,
        Roman Beranek <me@crly.cz>, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 5/8] clk: sunxi-ng: nkm: Support finding closest rate
Message-ID: <3ikbk67av3j2drphpwncbvfzbd6jw4ytcdri2sn3tmnsny4r5h@db3uxmavz6n2>
References: <20230702-pll-mipi_set_rate_parent-v3-0-46dcb8aa9cbc@oltmanns.dev>
 <20230702-pll-mipi_set_rate_parent-v3-5-46dcb8aa9cbc@oltmanns.dev>
 <87wmzhsbu0.fsf@oltmanns.dev>
 <zvxviyera3zxe2ro5ej52vz2o2vrinu6zo7osddojbys4tuqhd@3f2okcbpf5jr>
 <87jzvhs740.fsf@oltmanns.dev>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="gegykcvgx7vzvvkm"
Content-Disposition: inline
In-Reply-To: <87jzvhs740.fsf@oltmanns.dev>
Precedence: bulk


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

On Mon, Jul 03, 2023 at 10:59:43AM +0200, Frank Oltmanns wrote:
>=20
> On 2023-07-03 at 09:25:59 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> > [[PGP Signed Part:Undecided]]
> > On Mon, Jul 03, 2023 at 09:17:43AM +0200, Frank Oltmanns wrote:
> >>
> >> On 2023-07-02 at 19:55:24 +0200, Frank Oltmanns <frank@oltmanns.dev> w=
rote:
> >> > When finding the best rate for a NKM clock, consider rates that are
> >> > higher than the requested rate, if the CCU_FEATURE_CLOSEST_RATE flag=
 is
> >> > set.
> >> >
> >> > Accommodate ccu_mux_helper_determine_rate to this change.
> >> >
> >> > Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> >> > ---
> >> >  drivers/clk/sunxi-ng/ccu_mux.c | 23 +++++++++++++++-----
> >> >  drivers/clk/sunxi-ng/ccu_nkm.c | 48 +++++++++++++++++++++++++++++++=
-----------
> >> >  2 files changed, 54 insertions(+), 17 deletions(-)
> >> >
> >> > diff --git a/drivers/clk/sunxi-ng/ccu_mux.c b/drivers/clk/sunxi-ng/c=
cu_mux.c
> >> > index 1d557e323169..8594d6a4addd 100644
> >> > --- a/drivers/clk/sunxi-ng/ccu_mux.c
> >> > +++ b/drivers/clk/sunxi-ng/ccu_mux.c
> >> > @@ -113,7 +113,7 @@ int ccu_mux_helper_determine_rate(struct ccu_com=
mon *common,
> >> >  	}
> >> >
> >> >  	for (i =3D 0; i < clk_hw_get_num_parents(hw); i++) {
> >> > -		unsigned long tmp_rate, parent_rate;
> >> > +		unsigned long tmp_rate, parent_rate, best_diff =3D ULONG_MAX;
> >> >  		struct clk_hw *parent;
> >> >
> >> >  		parent =3D clk_hw_get_parent_by_index(hw, i);
> >> > @@ -139,10 +139,23 @@ int ccu_mux_helper_determine_rate(struct ccu_c=
ommon *common,
> >> >  			goto out;
> >> >  		}
> >> >
> >> > -		if ((req->rate - tmp_rate) < (req->rate - best_rate)) {
> >> > -			best_rate =3D tmp_rate;
> >> > -			best_parent_rate =3D parent_rate;
> >> > -			best_parent =3D parent;
> >> > +		if (common->features & CCU_FEATURE_CLOSEST_RATE) {
> >> > +			unsigned long tmp_diff =3D req->rate > tmp_rate ?
> >> > +						 req->rate - tmp_rate :
> >> > +						 tmp_rate - req->rate;
> >> > +
> >> > +			if (tmp_diff < best_diff) {
> >> > +				best_rate =3D tmp_rate;
> >> > +				best_parent_rate =3D parent_rate;
> >> > +				best_parent =3D parent;
> >> > +				best_diff =3D tmp_diff;
> >> > +			}
> >> > +		} else {
> >> > +			if ((req->rate - tmp_rate) < (req->rate - best_rate)) {
> >> > +				best_rate =3D tmp_rate;
> >> > +				best_parent_rate =3D parent_rate;
> >> > +				best_parent =3D parent;
> >> > +			}
> >> >  		}
> >> >  	}
> >> >
> >> > diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/c=
cu_nkm.c
> >> > index d83843e69c25..36d9e987e4d8 100644
> >> > --- a/drivers/clk/sunxi-ng/ccu_nkm.c
> >> > +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
> >> > @@ -18,9 +18,11 @@ struct _ccu_nkm {
> >> >  };
> >> >
> >> >  static unsigned long ccu_nkm_find_best_with_parent_adj(unsigned lon=
g *parent, unsigned long rate,
> >> > -						       struct _ccu_nkm *nkm, struct clk_hw *phw)
> >> > +						       struct _ccu_nkm *nkm, struct clk_hw *phw,
> >> > +						       unsigned long features)
> >> >  {
> >> > -	unsigned long best_rate =3D 0, best_parent_rate =3D *parent, tmp_p=
arent =3D *parent;
> >> > +	unsigned long best_rate =3D 0, best_parent_rate =3D 0, tmp_parent =
=3D *parent;
> >> > +	unsigned long best_diff =3D ULONG_MAX;
> >> >  	unsigned long best_n =3D 0, best_k =3D 0, best_m =3D 0;
> >> >  	unsigned long _n, _k, _m;
> >> >
> >> > @@ -28,16 +30,26 @@ static unsigned long ccu_nkm_find_best_with_pare=
nt_adj(unsigned long *parent, un
> >> >  		for (_n =3D nkm->min_n; _n <=3D nkm->max_n; _n++) {
> >> >  			for (_m =3D nkm->min_m; _m <=3D nkm->max_m; _m++) {
> >> >  				unsigned long tmp_rate;
> >> > +				unsigned long tmp_diff;
> >> >
> >> >  				tmp_parent =3D clk_hw_round_rate(phw, rate * _m / (_n * _k));
> >> >
> >> >  				tmp_rate =3D tmp_parent * _n * _k / _m;
> >> > -				if (tmp_rate > rate)
> >> > -					continue;
> >> >
> >> > -				if ((rate - tmp_rate) < (rate - best_rate)) {
> >> > +				if (features & CCU_FEATURE_CLOSEST_RATE) {
> >> > +					tmp_diff =3D rate > tmp_rate ?
> >> > +						   rate - tmp_rate :
> >> > +						   tmp_rate - rate;
> >> > +				} else {
> >> > +					if (tmp_rate > rate)
> >> > +						continue;
> >> > +					tmp_diff =3D rate - tmp_diff;
> >>
> >> Sorry, this should of course be tmp_diff =3D rate - tmp_rate. I'll fix
> >> that in v4. Also I'll do tests on my phone where
> >> CCU_FEATURE_CLOSEST_RATE is not set (i.e., without PATCH 8), so see if
> >> it replicates the old behaviour. I'll also look into adding kunit test=
s,
> >> so that this doesn't happen again. I'm not sure if this is feasible, b=
ut
> >> I'll ask here for advise, if/when I encounter obstacles.
> >
> > While this would obviously be great, I don't think we have the
> > infrastructure just yet to allow to easily add kunit tests for entire
> > clocks.
>=20
> I think, clk_test.c provides a good blueprint. I tried to do that for
> clk-fractional-divider [1], but Stephen wanted to go a different route,
> so I dropped it. You could look at clk_fd_test_init() in [1]. A similar
> approach might work for the sunxi-ng clocks. I don't see any real
> blockers, but maybe that's me being naive.

The main issue will be probing and mocking. Those clocks are meant to be
probed through the device tree and expect to have underlying registers
accessible.

We would need some way to mock / prevent any register access, while
still registering a clock with its device tree node, parent, etc. for
the tests to be meaningful. And that's not going to be an easy thing to
do :)

Maxime

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

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

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCZKKyxAAKCRDj7w1vZxhR
xV/lAQDhvphlvWpOwfrzo0B9hdjf3WpLvcLri3WR/earG6778QD+MDIjfM9bpfqo
pt2yxU9b6TJVfjAZY2uEWLuuBiW9TwA=
=lP/K
-----END PGP SIGNATURE-----

--gegykcvgx7vzvvkm--