Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 26 Jan 2022 16:03:38 +0100
From:   Maxime Ripard <maxime@cerno.tech>
To:     Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Cc:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        dave.stevenson@raspberrypi.com, devicetree@vger.kernel.org,
        kernel-list@raspberrypi.com, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
        linux-rpi-kernel@lists.infradead.org, lukasz@jany.st,
        mchehab@kernel.org, naush@raspberrypi.com, robh@kernel.org,
        tomi.valkeinen@ideasonboard.com
Subject: Re: [RFC PATCH v2 4/7] media: bcm2835-unicam: Add support for for
 CCP2/CSI2 camera interface
Message-ID: <20220126150338.j6bep2v5wmi3xnnq@houat>
References: <20220121081810.155500-1-jeanmichel.hautbois@ideasonboard.com>
 <20220121081810.155500-5-jeanmichel.hautbois@ideasonboard.com>
 <YeySvkxEfpzTj+pC@pendragon.ideasonboard.com>
 <0212af6d-a5de-05bb-161b-4271692dee59@ideasonboard.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="on6ulyuqio35z6hz"
Content-Disposition: inline
In-Reply-To: <0212af6d-a5de-05bb-161b-4271692dee59@ideasonboard.com>
Precedence: bulk


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

Hi,

On Wed, Jan 26, 2022 at 03:21:40PM +0100, Jean-Michel Hautbois wrote:
> On 23/01/2022 00:26, Laurent Pinchart wrote:
> > > +struct unicam_device {
> > > +	struct kref kref;
> > > +
> > > +	/* V4l2 specific parameters */
> > > +	struct v4l2_async_subdev asd;
> > > +
> > > +	/* peripheral base address */
> > > +	void __iomem *base;
> > > +	/* clock gating base address */
> > > +	void __iomem *clk_gate_base;
> > > +	/* lp clock handle */
> > > +	struct clk *clock;
> > > +	/* vpu clock handle */
> > > +	struct clk *vpu_clock;
> > > +	/* vpu clock request */
> > > +	struct clk_request *vpu_req;
> >=20
> > Not used (and that may be a problem).
>=20
> In the original linux-rpi tree, there is this portion of code in
> unicam_start_streaming:
>=20
> dev->vpu_req =3D clk_request_start(dev->vpu_clock, MIN_VPU_CLOCK_RATE);
> if (!dev->vpu_req) {
> 	unicam_err(dev, "failed to set up VPU clock\n");
> 	goto error_pipeline;
> }
>=20
> ret =3D clk_prepare_enable(dev->vpu_clock);
> if (ret) {
> 	unicam_err(dev, "Failed to enable VPU clock: %d\n", ret);
> 	goto error_pipeline;
> }
>=20
> And this is used as this because it depends on the non-merged series "clk:
> [PATCH v2 0/3] clk: Implement a clock request API" [1]
>=20
> [1]: https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tec=
h/
>=20
> That's why I modified the code and called:
> clk_set_min_rate(dev->vpu_clock, UNICAM_MIN_VPU_CLOCK_RATE);

I assume this would depend on the framerate and resolution though?

> Dave, is it ok or do we need absolutely the clock request API ?

The main issue is that clk_set_min_rate will never scale the clock back
if you (or the HVS) don't have those constraints anymore. So you
eventually make the clock run at the maximum you'll ever need all the
time (which would be around 500MHz in our case).

The clock request API was an attempt at making the clock scale back to
its minimum when we no longer needed it. The current discussion points
towards changing the behavior of clk_set_min_rate:

https://lore.kernel.org/linux-clk/20220125141549.747889-1-maxime@cerno.tech/

So it looks like we won't need the clk_request API after all.

Maxime

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

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

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYfFiygAKCRDj7w1vZxhR
xdfXAQCarLQAdpwfxO5Jb9U7xXxxxeMYjmaLBjYqqCwcxeevpgEArH2HxByy/UWC
mIUewELV3aqNR2Ii+BmPLCeY4FU03gw=
=hcXf
-----END PGP SIGNATURE-----

--on6ulyuqio35z6hz--