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:   Wed, 14 Sep 2022 17:15:02 +0100
From:   Maxime Ripard <maxime@cerno.tech>
To:     Stephen Boyd <sboyd@kernel.org>
Cc:     Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@linux.ie>, Emma Anholt <emma@anholt.net>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Ray Jui <rjui@broadcom.com>,
        Scott Branden <sbranden@broadcom.com>,
        linux-arm-kernel@lists.infradead.org,
        linux-rpi-kernel@lists.infradead.org,
        dri-devel@lists.freedesktop.org, Dom Cobley <popcornmix@gmail.com>,
        linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/7] clk: bcm: rpi: Add a function to retrieve the
 maximum
Message-ID: <20220914161502.faiaccuxydyrdr6e@penduick>
References: <20220815-rpi-fix-4k-60-v1-0-c52bd642f7c6@cerno.tech>
 <20220815-rpi-fix-4k-60-v1-2-c52bd642f7c6@cerno.tech>
 <20220914155035.88E45C433C1@smtp.kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="lzv5z4zggisvled2"
Content-Disposition: inline
In-Reply-To: <20220914155035.88E45C433C1@smtp.kernel.org>
Precedence: bulk


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

Hi Stephen,

Thanks for reviewing that series

On Wed, Sep 14, 2022 at 08:50:33AM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-08-15 08:31:24)
> > @@ -254,6 +255,33 @@ static int raspberrypi_fw_dumb_determine_rate(stru=
ct clk_hw *hw,
> >         return 0;
> >  }
> > =20
> > +unsigned long rpi_firmware_clk_get_max_rate(struct clk *clk)
> > +{
> > +       const struct raspberrypi_clk_data *data;
> > +       struct raspberrypi_clk *rpi;
> > +       struct clk_hw *hw;
> > +       u32 max_rate;
> > +       int ret;
> > +
> > +       if (!clk)
> > +               return 0;
> > +
> > +       hw =3D  __clk_get_hw(clk);
>=20
> Ideally we don't add more users of this API. I should document that :/

What should be the proper way to implement this?

> It begs the question though, why do we need this API to take a 'struct
> clk'?  Can it simply hardcode the data->id value for the clk you care
> about and call rpi_firmware_property() directly (or some wrapper of it)?

You mean push it down to the consumer?

We will have two users of that function eventually. The KMS driver, and
the codec driver that isn't upstream yet. AFAIK, both are using a
different clock, so we can' really hardcode it, and duplicating it at
the consumer level would be weird.

> Furthermore, I wonder if even that part needs to be implemented.  Why
> not make a direct call to rpi_firmware_property() and get the max rate?
> All of that can live in the drm driver. Making it a generic API that
> takes a 'struct clk' means that it looks like any clk can be passed,
> when that isn't true. It would be better to restrict it to the one use
> case so that the scope of the problem doesn't grow. I understand that it
> duplicates a few lines of code, but that looks like a fair tradeoff vs.
> exposing an API that can be used for other clks in the future.

So we'll want to have that function shared between the KMS and codec
drivers eventually. The clock id used by both drivers is stored in the
DT so we would create a function (outside of the clock drivers) that
would parse the clocks property, get the ID, and then queries the
firmware for it. Would that make sense?

Maxime

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

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

iHUEABMIAB0WIQTXEe0+DlZaRlgM8LOIQ8rmN6G3ywUCYyH+BgAKCRCIQ8rmN6G3
y8NdAQDNNVcy/hcyBCUGwW5D6Ba/bwnP5KpSp6uvbGHgAZ7/HQD/XIuX+7fGvFsy
YBf5MZIJ0Vmh5SvM654kpU7dr8di1Hw=
=F783
-----END PGP SIGNATURE-----

--lzv5z4zggisvled2--