Re: [RFC PATCH v2 4/7] media: bcm2835-unicam: Add support for for CCP2/CSI2 camera interface

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
Date: Wed, 26 Jan 2022 16:03:38 +0100	[thread overview]
Message-ID: <20220126150338.j6bep2v5wmi3xnnq@houat> (raw)
In-Reply-To: <0212af6d-a5de-05bb-161b-4271692dee59@ideasonboard.com>

[-- Attachment #1.1: Type: text/plain, Size: 2200 bytes --]

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;
> > 
> > Not used (and that may be a problem).
> 
> In the original linux-rpi tree, there is this portion of code in
> unicam_start_streaming:
> 
> dev->vpu_req = 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;
> }
> 
> ret = clk_prepare_enable(dev->vpu_clock);
> if (ret) {
> 	unicam_err(dev, "Failed to enable VPU clock: %d\n", ret);
> 	goto error_pipeline;
> }
> 
> 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]
> 
> [1]: https://lore.kernel.org/all/20210914093515.260031-1-maxime@cerno.tech/
> 
> 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel