On Wed, Jun 29, 2022 at 01:48:42AM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-06-28 02:47:53)
> > Hi,
> > 
> > On Mon, Jun 27, 2022 at 04:31:04PM -0700, Stephen Boyd wrote:
> > > 
> > > Ok, so this driver must want the new API.
> > > 
> > > What is happening here though? The driver is setting 'disable_4kp60' at
> > > bind/probe time based on a clk_round_rate() returning a frequency.
> > 
> > The main issue that we're trying to address is that whether or not HDMI
> > modes with a rate over 340MHz (so most likely 4k/60Hz but others are
> > affected) need a bootloader parameter to be set. If it isn't set, we
> > can't output those modes.
> > 
> > Since it's a bootloader parameter though the kernel can't access it. The
> > main hint that we can use to figure out whether it's been enabled is
> > that the maximum clock frequency reported by the firmware will be
> > higher. So this code will try to round a frequency higher than the
> > maximum allowed when that setting isn't there, and thus figure out
> > whether it's enabled or not.
> 
> So the kernel is at least able to ask for the max frequency during
> rounding and figure out that the frequency can't be larger than that. Is
> that right?

Yes, the firmware has a call that allows to query the boundaries of a
given clock it manages, and we then used whatever value it returns to
set the clk_hw rate range.

See https://elixir.bootlin.com/linux/latest/source/drivers/clk/bcm/clk-raspberrypi.c#L287

> > If it's not, we prevent any of these modes from being exposed to
> > userspace or being used.
> > 
> > > That returned value could change at runtime though based on rate
> > > constraints, or simply because the clk driver decides that the wind is
> > > blowing differently today and thus calling clk_set_rate() with that
> > > frequency will cause the clk to be wildly different than before.
> > 
> > Yeah, that's true
> > 
> > > I don't understand how we can decide to disable 4kp60 at probe time.
> > 
> > We're trying to infer a bootloader/firmware parameter, so the only way
> > it can change is through a reboot.
> 
> Got it.
> 
> > 
> > > Why doesn't the driver try to set the rate it wants (or the rate range
> > > it wants) and then if that succeeds it knows 4kp60 is achievable and
> > > if not then it rejects the attempt by userspace to set such a
> > > resolution.
> > 
> > We can't really do that. The clock here drives the HDMI output so it can
> > only change when we change the mode. However, because of the atomic
> > commits in KMS, we can't fail when we actually change the mode, we have
> > to fail beforehand when we check that the new state is sane.
> 
> Alright. I feel like we've discussed this before.
> 
> > 
> > We also can't change the clock rate then, because there's no guarantee
> > that the state being checked is actually going to be committed, and
> > because we still have the hardware running when we check it so we would
> > modify the clock while the hardware is running.
> > 
> > I had another go in the RaspberryPi downstream kernel for this:
> > https://github.com/raspberrypi/linux/commit/df368502ecbe1de26cf02a9b7837da9e967d64ca
> > 
> 
> It really looks to me like we're trying to hide the firmware API behind
> the clk API. When the clk API doesn't provide what's needed, we add an
> API to expose internal clk details that the clk consumer should already
> know because it sets them.

That would work for me

> That's my main concern here. The driver is querying an OPP table
> through the clk framework.
> 
> Why can't we either expose the firmware API directly to this driver or
> have the firmware driver probe and populate/trim an OPP table for this
> display device so that it can query the OPP table at probe time to
> determine the maximum frequency supported. The clk framework isn't in
> the business of exposing OPP tables, that's what the OPP framework is
> for.

Is it really an OPP?

My understanding at least is that an OPP table is a discrete set of
frequencies that a device can operate at, but you still need the
frequency generator somewhere else?

What we're discussing here definitely looks more like a clock to me:
it's is the frequency generator, and can expose a continuous set of
frequencies between a range. It just turns out that depending on a
parameter that range changes a bit, and it then affect our capabilities.

Maxime